| Message ID | 20230208111645.3863534-1-mmaddireddy@nvidia.com |
|---|---|
| Headers | show |
| Series | Add DT based PCIe wake support in PCI core driver | expand |
Hi Manikanta, I don't see any update on this series after comments. Is there any plans to take up this series. Thanks & Regards, Krishna Chaitanya. On 2/8/2023 4:46 PM, Manikanta Maddireddy wrote: > Below series [1] attempted to support DT based PCIe wake feature in generic > PCI core driver. This series was left at v13 and final comments are not > addressed. I am continuing this series from v14 by addressing all comments > in v13. I dropped rockchip device tree patch because I don't have hardware > to verify it. Instead, I verified these patches on NVIDIA Jetson AGX Orin > Developer Kit and included its device tree changes in this series. > > [1] https://lore.kernel.org/all/20171226023646.17722-1-jeffy.chen@rock-chips.com/ > > Changes in v14: > Updated commit message for DT bindings patch to reflect that DT properties > are tied to PCI-PCI Bridge. > Addressed review comments on PCI interrupt parsing patch. > Dropped rockchip device tree patch. > Added Jetson AGX OrinDeveloper Kit device tree and Tegra PMC patches. > > Changes in v13: > Fix compiler error reported by kbuild test robot <fengguang.wu@intel.com> > > Changes in v12: > Only add irq definitions for PCI devices and rewrite the commit message. > Enable the wake irq in noirq stage to avoid possible irq storm. > > Changes in v11: > Address Brian's comments. > Only support 1-per-device PCIe WAKE# pin as suggested. > Move to pcie port as Brian suggested. > > Changes in v10: > Use device_set_wakeup_capable() instead of device_set_wakeup_enable(), > since dedicated wakeirq will be lost in device_set_wakeup_enable(false). > > Changes in v9: > Add section for PCI devices and rewrite the commit message. > Fix check error in .cleanup(). > Move dedicated wakeirq setup to setup() callback and use > device_set_wakeup_enable() to enable/disable. > Rewrite the commit message. > > Changes in v8: > Add optional "pci", and rewrite commit message. > Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal. > Rewrite the commit message. > > Changes in v7: > Move PCIE_WAKE handling into pci core. > > Changes in v6: > Fix device_init_wake error handling, and add some comments. > > Changes in v5: > Move to pci.txt > Rebase. > Use "wakeup" instead of "wake" > > Changes in v3: > Fix error handling. > > Changes in v2: > Use dev_pm_set_dedicated_wake_irq. > > Jeffy Chen (3): > dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq > of/irq: Adjust of_pci_irq parsing for multiple interrupts > PCI / PM: Add support for the PCIe WAKE# signal for OF > > Manikanta Maddireddy (2): > arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller > soc/tegra: pmc: Add Tegra234 PCIe wake event > > Documentation/devicetree/bindings/pci/pci.txt | 8 +++ > .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 11 ++++ > drivers/pci/of.c | 63 ++++++++++++++++++- > drivers/pci/pci-driver.c | 10 +++ > drivers/pci/pci.c | 7 +++ > drivers/pci/pci.h | 8 +++ > drivers/soc/tegra/pmc.c | 1 + > 7 files changed, 105 insertions(+), 3 deletions(-) >
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 >
Hi Krishna Chaitanya, Unfortunately I cannot follow up on this series now. If you have a platform with same requirement, please verify and publish new version. Thanks, Manikanta On 06-12-2023 20:14, Krishna Chaitanya Chundru wrote: > External email: Use caution opening links or attachments > > > Hi Manikanta, > > I don't see any update on this series after comments. > > Is there any plans to take up this series. > > Thanks & Regards, > > Krishna Chaitanya. > > On 2/8/2023 4:46 PM, Manikanta Maddireddy wrote: >> Below series [1] attempted to support DT based PCIe wake feature in >> generic >> PCI core driver. This series was left at v13 and final comments are not >> addressed. I am continuing this series from v14 by addressing all >> comments >> in v13. I dropped rockchip device tree patch because I don't have >> hardware >> to verify it. Instead, I verified these patches on NVIDIA Jetson AGX >> Orin >> Developer Kit and included its device tree changes in this series. >> >> [1] >> https://lore.kernel.org/all/20171226023646.17722-1-jeffy.chen@rock-chips.com/ >> >> Changes in v14: >> Updated commit message for DT bindings patch to reflect that DT >> properties >> are tied to PCI-PCI Bridge. >> Addressed review comments on PCI interrupt parsing patch. >> Dropped rockchip device tree patch. >> Added Jetson AGX OrinDeveloper Kit device tree and Tegra PMC patches. >> >> Changes in v13: >> Fix compiler error reported by kbuild test robot >> <fengguang.wu@intel.com> >> >> Changes in v12: >> Only add irq definitions for PCI devices and rewrite the commit message. >> Enable the wake irq in noirq stage to avoid possible irq storm. >> >> Changes in v11: >> Address Brian's comments. >> Only support 1-per-device PCIe WAKE# pin as suggested. >> Move to pcie port as Brian suggested. >> >> Changes in v10: >> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(), >> since dedicated wakeirq will be lost in device_set_wakeup_enable(false). >> >> Changes in v9: >> Add section for PCI devices and rewrite the commit message. >> Fix check error in .cleanup(). >> Move dedicated wakeirq setup to setup() callback and use >> device_set_wakeup_enable() to enable/disable. >> Rewrite the commit message. >> >> Changes in v8: >> Add optional "pci", and rewrite commit message. >> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal. >> Rewrite the commit message. >> >> Changes in v7: >> Move PCIE_WAKE handling into pci core. >> >> Changes in v6: >> Fix device_init_wake error handling, and add some comments. >> >> Changes in v5: >> Move to pci.txt >> Rebase. >> Use "wakeup" instead of "wake" >> >> Changes in v3: >> Fix error handling. >> >> Changes in v2: >> Use dev_pm_set_dedicated_wake_irq. >> >> Jeffy Chen (3): >> dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq >> of/irq: Adjust of_pci_irq parsing for multiple interrupts >> PCI / PM: Add support for the PCIe WAKE# signal for OF >> >> Manikanta Maddireddy (2): >> arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller >> soc/tegra: pmc: Add Tegra234 PCIe wake event >> >> Documentation/devicetree/bindings/pci/pci.txt | 8 +++ >> .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 11 ++++ >> drivers/pci/of.c | 63 ++++++++++++++++++- >> drivers/pci/pci-driver.c | 10 +++ >> drivers/pci/pci.c | 7 +++ >> drivers/pci/pci.h | 8 +++ >> drivers/soc/tegra/pmc.c | 1 + >> 7 files changed, 105 insertions(+), 3 deletions(-) >>
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 >> > -- > மணிவண்ணன் சதாசிவம்
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 > > > > > -- > > மணிவண்ணன் சதாசிவம்
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 >>>> >>> -- >>> மணிவண்ணன் சதாசிவம் > -- > மணிவண்ணன் சதாசிவம்
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 > > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம் > > -- > > மணிவண்ணன் சதாசிவம்