diff mbox series

[2/4] dt-bindings: imx6q-pcie: Add host-wake-gpio property

Message ID 20231208091355.1417292-3-sherry.sun@nxp.com
State New
Headers
Series PCI: imx6: Add pci host wakeup support |

Commit Message

Sherry Sun Dec. 8, 2023, 9:13 a.m. UTC 
  Add host-wake-gpio property that can be used to wakeup the host
processor.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Lucas Stach Dec. 8, 2023, 10 a.m. UTC | #1 
Hi Sherry,

Am Freitag, dem 08.12.2023 um 17:13 +0800 schrieb Sherry Sun:
> Add host-wake-gpio property that can be used to wakeup the host
> processor.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> index 81bbb8728f0f..944f0f961809 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> @@ -72,6 +72,10 @@ properties:
>        L=operation state) (optional required).
>      type: boolean
>  
> +  host-wake-gpio:

There is only one wake signal in PCIe and it has a defined direction,
so there is no point in specifying that it is a host wakeup. Also GPIO
handles without a traling 's' are deprecated. So this should be

wake-gpios

> +    description: Should specify the GPIO for controlling the PCI bus device
> +      wake signal, used to wakeup the host processor. Default to active-low.

The description is wrong. For the RC complex case (which is the binding
you are modifying here) the controller does not control the wake
signal, but instead uses it as a input. The description should reflect
that.

The default is also quite useless, as your implementation does not
allow to change it. Please translate the GPIO active flags from the DT
to the proper IRQ flags and drop this default here. The DT should
simply carry the proper polarity.

Regards,
Lucas

> +
>  required:
>    - compatible
>    - reg
Rob Herring Dec. 8, 2023, 8:55 p.m. UTC | #2 
On Fri, Dec 08, 2023 at 11:00:19AM +0100, Lucas Stach wrote:
> Hi Sherry,
> 
> Am Freitag, dem 08.12.2023 um 17:13 +0800 schrieb Sherry Sun:
> > Add host-wake-gpio property that can be used to wakeup the host
> > processor.
> > 
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > index 81bbb8728f0f..944f0f961809 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > @@ -72,6 +72,10 @@ properties:
> >        L=operation state) (optional required).
> >      type: boolean
> >  
> > +  host-wake-gpio:
> 
> There is only one wake signal in PCIe and it has a defined direction,
> so there is no point in specifying that it is a host wakeup. Also GPIO
> handles without a traling 's' are deprecated. So this should be
> 
> wake-gpios

Any standard PCI slot signals need to be documented in common PCI 
schema. And they should start going into root port nodes rather than the 
host bridge node because it's the root ports that correspond to slots 
rather than the host bridge. We've just taken shortcuts because many 
host bridges only have 1 root port.

Note that I'm in the middle of splitting pci-bus.yaml into host bridge, 
PCI-PCI bridge (and RP), and common device schemas.

Rob
Bjorn Helgaas Dec. 8, 2023, 10:27 p.m. UTC | #3 
On Fri, Dec 08, 2023 at 02:55:45PM -0600, Rob Herring wrote:
> ...

> And they should start going into root port nodes rather than the 
> host bridge node because it's the root ports that correspond to slots 
> rather than the host bridge. We've just taken shortcuts because many 
> host bridges only have 1 root port.
> 
> Note that I'm in the middle of splitting pci-bus.yaml into host bridge, 
> PCI-PCI bridge (and RP), and common device schemas.

Hooray!  Thanks for working on that; the conflation of host bridge and
Root Port is a real annoyance.

Bjorn
Sherry Sun Dec. 11, 2023, 9:14 a.m. UTC | #4 
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2023年12月8日 18:00
> To: Sherry Sun <sherry.sun@nxp.com>; Hongxing Zhu
> <hongxing.zhu@nxp.com>; lpieralisi@kernel.org; kw@linux.com;
> robh@kernel.org; bhelgaas@google.com; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-pci@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] dt-bindings: imx6q-pcie: Add host-wake-gpio
> property
> 
> Hi Sherry,
> 
> Am Freitag, dem 08.12.2023 um 17:13 +0800 schrieb Sherry Sun:
> > Add host-wake-gpio property that can be used to wakeup the host
> > processor.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > index 81bbb8728f0f..944f0f961809 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > @@ -72,6 +72,10 @@ properties:
> >        L=operation state) (optional required).
> >      type: boolean
> >
> > +  host-wake-gpio:
> 
> There is only one wake signal in PCIe and it has a defined direction, so there
> is no point in specifying that it is a host wakeup. Also GPIO handles without a
> traling 's' are deprecated. So this should be
> 
> wake-gpios

Hi Lucas, thanks for the comment, will change it in V2.

> 
> > +    description: Should specify the GPIO for controlling the PCI bus device
> > +      wake signal, used to wakeup the host processor. Default to active-low.
> 
> The description is wrong. For the RC complex case (which is the binding you
> are modifying here) the controller does not control the wake signal, but
> instead uses it as a input. The description should reflect that.
> 
> The default is also quite useless, as your implementation does not allow to
> change it. Please translate the GPIO active flags from the DT to the proper
> IRQ flags and drop this default here. The DT should simply carry the proper
> polarity.
> 

Will improve the description in V2, thanks.

Best Regards
Sherry
Sherry Sun Dec. 11, 2023, 10:52 a.m. UTC | #5 
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2023年12月9日 4:56
> To: Lucas Stach <l.stach@pengutronix.de>
> Cc: Sherry Sun <sherry.sun@nxp.com>; Hongxing Zhu
> <hongxing.zhu@nxp.com>; lpieralisi@kernel.org; kw@linux.com;
> bhelgaas@google.com; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; linux-pci@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] dt-bindings: imx6q-pcie: Add host-wake-gpio
> property
> 
> On Fri, Dec 08, 2023 at 11:00:19AM +0100, Lucas Stach wrote:
> > Hi Sherry,
> >
> > Am Freitag, dem 08.12.2023 um 17:13 +0800 schrieb Sherry Sun:
> > > Add host-wake-gpio property that can be used to wakeup the host
> > > processor.
> > >
> > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > index 81bbb8728f0f..944f0f961809 100644
> > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > @@ -72,6 +72,10 @@ properties:
> > >        L=operation state) (optional required).
> > >      type: boolean
> > >
> > > +  host-wake-gpio:
> >
> > There is only one wake signal in PCIe and it has a defined direction,
> > so there is no point in specifying that it is a host wakeup. Also GPIO
> > handles without a traling 's' are deprecated. So this should be
> >
> > wake-gpios
> 
> Any standard PCI slot signals need to be documented in common PCI schema.
> And they should start going into root port nodes rather than the host bridge
> node because it's the root ports that correspond to slots rather than the host
> bridge. We've just taken shortcuts because many host bridges only have 1
> root port.
> 
> Note that I'm in the middle of splitting pci-bus.yaml into host bridge, PCI-PCI
> bridge (and RP), and common device schemas.
> 

Hi Rob, thanks for your comment, I am new to PCIe, can you please provide more details on which common PCI schema the WAKE# property should be added in (maybe snps,dw-pcie.yaml or something else)?

Best Regards
Sherry
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index 81bbb8728f0f..944f0f961809 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -72,6 +72,10 @@  properties:
       L=operation state) (optional required).
     type: boolean
 
+  host-wake-gpio:
+    description: Should specify the GPIO for controlling the PCI bus device
+      wake signal, used to wakeup the host processor. Default to active-low.
+
 required:
   - compatible
   - reg