diff mbox series

[RFC,v14,4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

Message ID 20230208111645.3863534-5-mmaddireddy@nvidia.com
State New
Headers
Series Add DT based PCIe wake support in PCI core driver |

Commit Message

Manikanta Maddireddy Feb. 8, 2023, 11:16 a.m. UTC 
  Add PCIe port node under the PCIe controller-1 device tree node to support
PCIe WAKE# interrupt for WiFi.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---

Changes in v14:
New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.

 .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Thierry Reding Feb. 8, 2023, 11:37 a.m. UTC | #1 
On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> Add PCIe port node under the PCIe controller-1 device tree node to support
> PCIe WAKE# interrupt for WiFi.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> 
>  .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> index 8a9747855d6b..9c89be263141 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>  
>  			phys = <&p2u_hsio_3>;
>  			phy-names = "p2u-0";
> +
> +			pci@0,0 {
> +				reg = <0x0000 0 0 0 0>;
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +
> +				interrupt-parent = <&gpio>;
> +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> +				interrupt-names = "wakeup";
> +			};

Don't we need to wire this to the PMC interrupt controller and the wake
event corresponding to the L2 GPIO? Otherwise none of the wake logic in
PMC will get invoked.

Thierry
Manikanta Maddireddy Feb. 8, 2023, 12:13 p.m. UTC | #2 
On 2/8/2023 5:07 PM, Thierry Reding wrote:
> On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
>> Add PCIe port node under the PCIe controller-1 device tree node to support
>> PCIe WAKE# interrupt for WiFi.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>
>>   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> index 8a9747855d6b..9c89be263141 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>>   
>>   			phys = <&p2u_hsio_3>;
>>   			phy-names = "p2u-0";
>> +
>> +			pci@0,0 {
>> +				reg = <0x0000 0 0 0 0>;
>> +				#address-cells = <3>;
>> +				#size-cells = <2>;
>> +				ranges;
>> +
>> +				interrupt-parent = <&gpio>;
>> +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
>> +				interrupt-names = "wakeup";
>> +			};
> Don't we need to wire this to the PMC interrupt controller and the wake
> event corresponding to the L2 GPIO? Otherwise none of the wake logic in
> PMC will get invoked.
>
> Thierry
PCIe wake is gpio based not pmc, only wake support is provided by PMC 
controller.
I verified this patch and able to wake up Tegra from suspend.
Petlozu, correct me if my understanding is wrong.


Manikanta
Thierry Reding Feb. 8, 2023, 4:14 p.m. UTC | #3 
On Wed, Feb 08, 2023 at 05:43:35PM +0530, Manikanta Maddireddy wrote:
> 
> On 2/8/2023 5:07 PM, Thierry Reding wrote:
> > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> > > Add PCIe port node under the PCIe controller-1 device tree node to support
> > > PCIe WAKE# interrupt for WiFi.
> > > 
> > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > ---
> > > 
> > > Changes in v14:
> > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> > > 
> > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > index 8a9747855d6b..9c89be263141 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > >   			phys = <&p2u_hsio_3>;
> > >   			phy-names = "p2u-0";
> > > +
> > > +			pci@0,0 {
> > > +				reg = <0x0000 0 0 0 0>;
> > > +				#address-cells = <3>;
> > > +				#size-cells = <2>;
> > > +				ranges;
> > > +
> > > +				interrupt-parent = <&gpio>;
> > > +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> > > +				interrupt-names = "wakeup";
> > > +			};
> > Don't we need to wire this to the PMC interrupt controller and the wake
> > event corresponding to the L2 GPIO? Otherwise none of the wake logic in
> > PMC will get invoked.
> > 
> > Thierry
> PCIe wake is gpio based not pmc, only wake support is provided by PMC
> controller.
> I verified this patch and able to wake up Tegra from suspend.
> Petlozu, correct me if my understanding is wrong.

The way that this usually works is that you need to use something like
this:

	interrupt-parent = <&pmc>;
	interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
	interrupt-names = "wakeup";

This will then cause the PMC's interrupt chip callbacks to setup all the
wake-related interrupts and use the internal wake event tables to
forward the GPIO/IRQ corresponding to the PMC wake event to the GPIO
controller or GIC, respectively.

If you use &gpio as the interrupt parent, none of the PMC logic will be
invoked, so unless this is somehow set up correctly by default, the PMC
wouldn't be able to wake up the system.

Thierry
Petlozu Pravareshwar Feb. 9, 2023, 10:53 a.m. UTC | #4 
> 
> On Wed, Feb 08, 2023 at 05:43:35PM +0530, Manikanta Maddireddy wrote:
> >
> > On 2/8/2023 5:07 PM, Thierry Reding wrote:
> > > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy
> wrote:
> > > > Add PCIe port node under the PCIe controller-1 device tree node to
> > > > support PCIe WAKE# interrupt for WiFi.
> > > >
> > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > ---
> > > >
> > > > Changes in v14:
> > > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX
> Orin.
> > > >
> > > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11
> +++++++++++
> > > >   1 file changed, 11 insertions(+)
> > > >
> > > > diff --git
> > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > index 8a9747855d6b..9c89be263141 100644
> > > > ---
> > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-
> 0000.dt
> > > > +++ s
> > > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > >   			phys = <&p2u_hsio_3>;
> > > >   			phy-names = "p2u-0";
> > > > +
> > > > +			pci@0,0 {
> > > > +				reg = <0x0000 0 0 0 0>;
> > > > +				#address-cells = <3>;
> > > > +				#size-cells = <2>;
> > > > +				ranges;
> > > > +
> > > > +				interrupt-parent = <&gpio>;
> > > > +				interrupts = <TEGRA234_MAIN_GPIO(L, 2)
> IRQ_TYPE_LEVEL_LOW>;
> > > > +				interrupt-names = "wakeup";
> > > > +			};
> > > Don't we need to wire this to the PMC interrupt controller and the
> > > wake event corresponding to the L2 GPIO? Otherwise none of the wake
> > > logic in PMC will get invoked.
> > >
> > > Thierry
> > PCIe wake is gpio based not pmc, only wake support is provided by PMC
> > controller.
> > I verified this patch and able to wake up Tegra from suspend.
> > Petlozu, correct me if my understanding is wrong.
> 
> The way that this usually works is that you need to use something like
> this:
> 
> 	interrupt-parent = <&pmc>;
> 	interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
> 	interrupt-names = "wakeup";
> 
> This will then cause the PMC's interrupt chip callbacks to setup all the wake-
> related interrupts and use the internal wake event tables to forward the
> GPIO/IRQ corresponding to the PMC wake event to the GPIO controller or
> GIC, respectively.
> 
> If you use &gpio as the interrupt parent, none of the PMC logic will be
> invoked, so unless this is somehow set up correctly by default, the PMC
> wouldn't be able to wake up the system.
> 
> Thierry
Thierry,
Since PMC's IRQ domain is made as parent of GPIO controller's IRQ domain,
I think, for GPIO based wakes setting &gpio as the interrupt parent can still
invoke PMC logic to program the required registers to enable such wakes.
Related commit in this regard:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpio/gpio-tegra186.c?id=2a36550567307b881ce570a81189682ae1c9d08d

Thanks.
Thierry Reding Feb. 9, 2023, 11:12 a.m. UTC | #5 
On Thu, Feb 09, 2023 at 10:53:25AM +0000, Petlozu Pravareshwar wrote:
> > 
> > On Wed, Feb 08, 2023 at 05:43:35PM +0530, Manikanta Maddireddy wrote:
> > >
> > > On 2/8/2023 5:07 PM, Thierry Reding wrote:
> > > > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy
> > wrote:
> > > > > Add PCIe port node under the PCIe controller-1 device tree node to
> > > > > support PCIe WAKE# interrupt for WiFi.
> > > > >
> > > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > > ---
> > > > >
> > > > > Changes in v14:
> > > > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX
> > Orin.
> > > > >
> > > > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11
> > +++++++++++
> > > > >   1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > index 8a9747855d6b..9c89be263141 100644
> > > > > ---
> > > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-
> > 0000.dt
> > > > > +++ s
> > > > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > > >   			phys = <&p2u_hsio_3>;
> > > > >   			phy-names = "p2u-0";
> > > > > +
> > > > > +			pci@0,0 {
> > > > > +				reg = <0x0000 0 0 0 0>;
> > > > > +				#address-cells = <3>;
> > > > > +				#size-cells = <2>;
> > > > > +				ranges;
> > > > > +
> > > > > +				interrupt-parent = <&gpio>;
> > > > > +				interrupts = <TEGRA234_MAIN_GPIO(L, 2)
> > IRQ_TYPE_LEVEL_LOW>;
> > > > > +				interrupt-names = "wakeup";
> > > > > +			};
> > > > Don't we need to wire this to the PMC interrupt controller and the
> > > > wake event corresponding to the L2 GPIO? Otherwise none of the wake
> > > > logic in PMC will get invoked.
> > > >
> > > > Thierry
> > > PCIe wake is gpio based not pmc, only wake support is provided by PMC
> > > controller.
> > > I verified this patch and able to wake up Tegra from suspend.
> > > Petlozu, correct me if my understanding is wrong.
> > 
> > The way that this usually works is that you need to use something like
> > this:
> > 
> > 	interrupt-parent = <&pmc>;
> > 	interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
> > 	interrupt-names = "wakeup";
> > 
> > This will then cause the PMC's interrupt chip callbacks to setup all the wake-
> > related interrupts and use the internal wake event tables to forward the
> > GPIO/IRQ corresponding to the PMC wake event to the GPIO controller or
> > GIC, respectively.
> > 
> > If you use &gpio as the interrupt parent, none of the PMC logic will be
> > invoked, so unless this is somehow set up correctly by default, the PMC
> > wouldn't be able to wake up the system.
> > 
> > Thierry
> Thierry,
> Since PMC's IRQ domain is made as parent of GPIO controller's IRQ domain,
> I think, for GPIO based wakes setting &gpio as the interrupt parent can still
> invoke PMC logic to program the required registers to enable such wakes.
> Related commit in this regard:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpio/gpio-tegra186.c?id=2a36550567307b881ce570a81189682ae1c9d08d

Heh... nicely self-owned =). You're right, no need for the detour in DT
with those, the GPIO driver will hook up the IRQ hierarchy itself. We
already do this for the "power" key in the various gpio-keys, so it
should work fine.

Sorry for the noise,
Thierry
Manivannan Sadhasivam Dec. 6, 2023, 3:36 p.m. UTC | #6 
On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> Add PCIe port node under the PCIe controller-1 device tree node to support
> PCIe WAKE# interrupt for WiFi.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> 
>  .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> index 8a9747855d6b..9c89be263141 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>  
>  			phys = <&p2u_hsio_3>;
>  			phy-names = "p2u-0";
> +
> +			pci@0,0 {
> +				reg = <0x0000 0 0 0 0>;
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +
> +				interrupt-parent = <&gpio>;
> +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> +				interrupt-names = "wakeup";

WAKE# should be part of the PCIe controller, not device. And the interrupt name
should be "wake".

- Mani

> +			};
>  		};
>  
>  		pcie@14160000 {
> -- 
> 2.25.1
>
Manikanta Maddireddy Dec. 7, 2023, 7:24 a.m. UTC | #7 
On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
>> Add PCIe port node under the PCIe controller-1 device tree node to support
>> PCIe WAKE# interrupt for WiFi.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>
>>   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> index 8a9747855d6b..9c89be263141 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>>
>>                        phys = <&p2u_hsio_3>;
>>                        phy-names = "p2u-0";
>> +
>> +                     pci@0,0 {
>> +                             reg = <0x0000 0 0 0 0>;
>> +                             #address-cells = <3>;
>> +                             #size-cells = <2>;
>> +                             ranges;
>> +
>> +                             interrupt-parent = <&gpio>;
>> +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
>> +                             interrupt-names = "wakeup";
> WAKE# should be part of the PCIe controller, not device. And the interrupt name
> should be "wake".
>
> - Mani
Hi,

Please refer to the discussion in below link, WAKE# is per PCI bridge.
https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/

I carried wakeup name defined in previous version, but wake seems to be 
sufficient.

Thanks,
Manikanta
>
>> +                     };
>>                };
>>
>>                pcie@14160000 {
>> --
>> 2.25.1
>>
> --
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam Dec. 7, 2023, 7:59 a.m. UTC | #8 
On Thu, Dec 07, 2023 at 12:54:04PM +0530, Manikanta Maddireddy wrote:
> 
> On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> > > Add PCIe port node under the PCIe controller-1 device tree node to support
> > > PCIe WAKE# interrupt for WiFi.
> > > 
> > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > ---
> > > 
> > > Changes in v14:
> > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> > > 
> > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > index 8a9747855d6b..9c89be263141 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > 
> > >                        phys = <&p2u_hsio_3>;
> > >                        phy-names = "p2u-0";
> > > +
> > > +                     pci@0,0 {
> > > +                             reg = <0x0000 0 0 0 0>;
> > > +                             #address-cells = <3>;
> > > +                             #size-cells = <2>;
> > > +                             ranges;
> > > +
> > > +                             interrupt-parent = <&gpio>;
> > > +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> > > +                             interrupt-names = "wakeup";
> > WAKE# should be part of the PCIe controller, not device. And the interrupt name
> > should be "wake".
> > 
> > - Mani
> Hi,
> 
> Please refer to the discussion in below link, WAKE# is per PCI bridge.
> https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/
> 

PCIe Host controller (RC) usually represents host bridge + PCI-PCI bridge. We do
not represent the PCI-PCI bridge in devicetree for any platforms, but only RC as
a whole.

Moreover, PERST# is already defined in RC node. So it becomes confusing if
WAKE# is defined in a child node representing bridge.

So please move WAKE# to RC node.

- Mani

> I carried wakeup name defined in previous version, but wake seems to be
> sufficient.
> 
> Thanks,
> Manikanta
> > 
> > > +                     };
> > >                };
> > > 
> > >                pcie@14160000 {
> > > --
> > > 2.25.1
> > > 
> > --
> > மணிவண்ணன் சதாசிவம்
Manikanta Maddireddy Dec. 7, 2023, 8:53 a.m. UTC | #9 
On 07-12-2023 13:29, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Dec 07, 2023 at 12:54:04PM +0530, Manikanta Maddireddy wrote:
>> On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
>>>> Add PCIe port node under the PCIe controller-1 device tree node to support
>>>> PCIe WAKE# interrupt for WiFi.
>>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> ---
>>>>
>>>> Changes in v14:
>>>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>>>
>>>>    .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> index 8a9747855d6b..9c89be263141 100644
>>>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>>>>
>>>>                         phys = <&p2u_hsio_3>;
>>>>                         phy-names = "p2u-0";
>>>> +
>>>> +                     pci@0,0 {
>>>> +                             reg = <0x0000 0 0 0 0>;
>>>> +                             #address-cells = <3>;
>>>> +                             #size-cells = <2>;
>>>> +                             ranges;
>>>> +
>>>> +                             interrupt-parent = <&gpio>;
>>>> +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
>>>> +                             interrupt-names = "wakeup";
>>> WAKE# should be part of the PCIe controller, not device. And the interrupt name
>>> should be "wake".
>>>
>>> - Mani
>> Hi,
>>
>> Please refer to the discussion in below link, WAKE# is per PCI bridge.
>> https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/
>>
> PCIe Host controller (RC) usually represents host bridge + PCI-PCI bridge. We do
> not represent the PCI-PCI bridge in devicetree for any platforms, but only RC as
> a whole.
>
> Moreover, PERST# is already defined in RC node. So it becomes confusing if
> WAKE# is defined in a child node representing bridge.
>
> So please move WAKE# to RC node.
>
> - Mani

Hi,

We can define PCI-PCI bridge in device tree, refer to below device tree 
which has 3 ports under a controller,
with PERST#(reset-gpios) defined per port.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/apple/t8103.dtsi#n749

Also, of_pci_setup_wake_irq() in below patch is parsing "wakeup" from 
PCI bridge, not from the host bridge.
https://patchwork.ozlabs.org/project/linux-pci/patch/20230208111645.3863534-4-mmaddireddy@nvidia.com/

If a controller has only one port it has to define a PCI bridge under 
controller device tree node and
add wakeup interrupt property, refer to below patch from original author.

https://www.spinics.net/lists/linux-pci/msg135569.html

Thanks,
Manikanta
>
>> I carried wakeup name defined in previous version, but wake seems to be
>> sufficient.
>>
>> Thanks,
>> Manikanta
>>>> +                     };
>>>>                 };
>>>>
>>>>                 pcie@14160000 {
>>>> --
>>>> 2.25.1
>>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
> --
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam Dec. 7, 2023, 9:31 a.m. UTC | #10 
On Thu, Dec 07, 2023 at 02:23:46PM +0530, Manikanta Maddireddy wrote:
> 
> On 07-12-2023 13:29, Manivannan Sadhasivam wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, Dec 07, 2023 at 12:54:04PM +0530, Manikanta Maddireddy wrote:
> > > On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> > > > > Add PCIe port node under the PCIe controller-1 device tree node to support
> > > > > PCIe WAKE# interrupt for WiFi.
> > > > > 
> > > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > > ---
> > > > > 
> > > > > Changes in v14:
> > > > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> > > > > 
> > > > >    .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
> > > > >    1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > index 8a9747855d6b..9c89be263141 100644
> > > > > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > > > 
> > > > >                         phys = <&p2u_hsio_3>;
> > > > >                         phy-names = "p2u-0";
> > > > > +
> > > > > +                     pci@0,0 {
> > > > > +                             reg = <0x0000 0 0 0 0>;
> > > > > +                             #address-cells = <3>;
> > > > > +                             #size-cells = <2>;
> > > > > +                             ranges;
> > > > > +
> > > > > +                             interrupt-parent = <&gpio>;
> > > > > +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> > > > > +                             interrupt-names = "wakeup";
> > > > WAKE# should be part of the PCIe controller, not device. And the interrupt name
> > > > should be "wake".
> > > > 
> > > > - Mani
> > > Hi,
> > > 
> > > Please refer to the discussion in below link, WAKE# is per PCI bridge.
> > > https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/
> > > 
> > PCIe Host controller (RC) usually represents host bridge + PCI-PCI bridge. We do
> > not represent the PCI-PCI bridge in devicetree for any platforms, but only RC as
> > a whole.
> > 
> > Moreover, PERST# is already defined in RC node. So it becomes confusing if
> > WAKE# is defined in a child node representing bridge.
> > 
> > So please move WAKE# to RC node.
> > 
> > - Mani
> 
> Hi,
> 
> We can define PCI-PCI bridge in device tree, refer to below device tree
> which has 3 ports under a controller,
> with PERST#(reset-gpios) defined per port.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/apple/t8103.dtsi#n749
> 

Hmm. For RCs with single bridge, we never defined a DT node (atleast on Qcom
platforms). But I think it is the time to fix them.

> Also, of_pci_setup_wake_irq() in below patch is parsing "wakeup" from PCI
> bridge, not from the host bridge.
> https://patchwork.ozlabs.org/project/linux-pci/patch/20230208111645.3863534-4-mmaddireddy@nvidia.com/
> 

I didn't say that WAKE# should be parsed from host bridge, it doesn't make
sense. But I get your point.

> If a controller has only one port it has to define a PCI bridge under
> controller device tree node and
> add wakeup interrupt property, refer to below patch from original author.
> 
> https://www.spinics.net/lists/linux-pci/msg135569.html
> 

Yes, I agree. Thanks for the clarification.

- Mani

> Thanks,
> Manikanta
> > 
> > > I carried wakeup name defined in previous version, but wake seems to be
> > > sufficient.
> > > 
> > > Thanks,
> > > Manikanta
> > > > > +                     };
> > > > >                 };
> > > > > 
> > > > >                 pcie@14160000 {
> > > > > --
> > > > > 2.25.1
> > > > > 
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> > --
> > மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
index 8a9747855d6b..9c89be263141 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
@@ -2147,6 +2147,17 @@  pcie@14100000 {
 
 			phys = <&p2u_hsio_3>;
 			phy-names = "p2u-0";
+
+			pci@0,0 {
+				reg = <0x0000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+
+				interrupt-parent = <&gpio>;
+				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
+				interrupt-names = "wakeup";
+			};
 		};
 
 		pcie@14160000 {