[PATCH 0/4] PCI: imx6: Add pci host wakeup support

[PATCH 0/4] PCI: imx6: Add pci host wakeup support

[Sherry Sun]

Add pci host wakeup feature for imx platforms. The host wake pin is a
standard feature in the PCIe bus specification, so we can add this
property under PCI dts node to support the host gpio wakeup feature.

Example of configuring the corresponding dts property under the PCI node:
    host-wake-gpio = <&gpio5 21 GPIO_ACTIVE_LOW>;

Sherry Sun (4):
  PCI: imx6: Add pci host wakeup support on imx platforms.
  dt-bindings: imx6q-pcie: Add host-wake-gpio property
  arm64: dts: imx8mp-evk: add host-wake-gpio property for pci bus
  arm64: dts: imx8mq-evk: add host-wake-gpio property for pci bus

 .../bindings/pci/fsl,imx6q-pcie.yaml          |  4 ++
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts  |  2 +
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts  |  2 +
 drivers/pci/controller/dwc/pci-imx6.c         | 69 +++++++++++++++++++
 4 files changed, 77 insertions(+)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] PCI: imx6: Add pci host wakeup support on imx platforms.

[Sherry Sun]

Add pci host wakeup feature for imx platforms.
Example of configuring the corresponding dts property under the PCI
node:
host-wake-gpio = <&gpio5 21 GPIO_ACTIVE_LOW>;

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 69 +++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 74703362aeec..050c9140f4a3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -72,6 +72,7 @@ struct imx6_pcie_drvdata {
 struct imx6_pcie {
 	struct dw_pcie		*pci;
 	int			reset_gpio;
+	int			host_wake_irq;
 	bool			gpio_active_high;
 	bool			link_is_up;
 	struct clk		*pcie_bus;
@@ -1237,11 +1238,46 @@ static int imx6_pcie_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int imx6_pcie_suspend(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	if (imx6_pcie->host_wake_irq >= 0)
+		enable_irq_wake(imx6_pcie->host_wake_irq);
+
+	return 0;
+}
+
+static int imx6_pcie_resume(struct device *dev)
+{
+	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
+
+	if (imx6_pcie->host_wake_irq >= 0)
+		disable_irq_wake(imx6_pcie->host_wake_irq);
+
+	return 0;
+}
+
 static const struct dev_pm_ops imx6_pcie_pm_ops = {
 	NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
 				  imx6_pcie_resume_noirq)
+	SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend, imx6_pcie_resume)
 };
 
+irqreturn_t host_wake_irq_handler(int irq, void *priv)
+{
+	struct imx6_pcie *imx6_pcie = priv;
+	struct device *dev = imx6_pcie->pci->dev;
+
+	dev_dbg(dev, "%s: host wakeup by pcie", __func__);
+
+	/* Notify PM core we are wakeup source */
+	pm_wakeup_event(dev, 0);
+	pm_system_wakeup();
+
+	return IRQ_HANDLED;
+}
+
 static int imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1250,6 +1286,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	struct device_node *np;
 	struct resource *dbi_base;
 	struct device_node *node = dev->of_node;
+	struct gpio_desc *host_wake_gpio;
 	int ret;
 	u16 val;
 
@@ -1457,6 +1494,32 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 			val |= PCI_MSI_FLAGS_ENABLE;
 			dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
 		}
+
+		/* host wakeup support */
+		imx6_pcie->host_wake_irq = -1;
+		host_wake_gpio = devm_gpiod_get_optional(dev, "host-wake", GPIOD_IN);
+		if (IS_ERR(host_wake_gpio))
+			return PTR_ERR(host_wake_gpio);
+
+		if (host_wake_gpio != NULL) {
+			imx6_pcie->host_wake_irq = gpiod_to_irq(host_wake_gpio);
+			ret = devm_request_threaded_irq(dev, imx6_pcie->host_wake_irq, NULL,
+							host_wake_irq_handler,
+							IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+							"host_wake", imx6_pcie);
+			if (ret) {
+				dev_err(dev, "Failed to request host_wake_irq %d (%d)\n",
+					imx6_pcie->host_wake_irq, ret);
+				imx6_pcie->host_wake_irq = -1;
+				return ret;
+			}
+
+			if (device_init_wakeup(dev, true)) {
+				dev_err(dev, "fail to init host_wake_irq\n");
+				imx6_pcie->host_wake_irq = -1;
+				return ret;
+			}
+		}
 	}
 
 	return 0;
@@ -1466,6 +1529,12 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
 {
 	struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev);
 
+	if (imx6_pcie->host_wake_irq >= 0) {
+		device_init_wakeup(&pdev->dev, false);
+		disable_irq(imx6_pcie->host_wake_irq);
+		imx6_pcie->host_wake_irq = -1;
+	}
+
 	/* bring down link, so bootloader gets clean state in case of reboot */
 	imx6_pcie_assert_core_reset(imx6_pcie);
 }
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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:
+    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
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] arm64: dts: imx8mp-evk: add host-wake-gpio property for pci bus

[Sherry Sun]

The host wake pin is a standard feature in the PCIe bus specification,
so we add this property under PCI dts node to enable the host wake
function.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
index f87fa5a948cc..cc9e5e599a33 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
@@ -542,6 +542,7 @@ &pcie {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_pcie0>;
 	reset-gpio = <&gpio2 7 GPIO_ACTIVE_LOW>;
+	host-wake-gpio = <&gpio5 21 GPIO_ACTIVE_LOW>;
 	vpcie-supply = <&reg_pcie0>;
 	status = "okay";
 };
@@ -772,6 +773,7 @@ pinctrl_pcie0: pcie0grp {
 		fsl,pins = <
 			MX8MP_IOMUXC_I2C4_SCL__PCIE_CLKREQ_B	0x60 /* open drain, pull up */
 			MX8MP_IOMUXC_SD1_DATA5__GPIO2_IO07	0x40
+			MX8MP_IOMUXC_I2C4_SDA__GPIO5_IO21	0x1c4
 		>;
 	};
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] arm64: dts: imx8mq-evk: add host-wake-gpio property for pci bus

[Sherry Sun]

The host wake pin is a standard feature in the PCIe bus specification,
so we add this property under PCI dts node to enable the host wake
function.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
index 7507548cdb16..ac824046c594 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
@@ -367,6 +367,7 @@ &pcie1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_pcie1>;
 	reset-gpio = <&gpio5 12 GPIO_ACTIVE_LOW>;
+	host-wake-gpio = <&gpio5 11 GPIO_ACTIVE_LOW>;
 	clocks = <&clk IMX8MQ_CLK_PCIE2_ROOT>,
 		 <&pcie0_refclk>,
 		 <&clk IMX8MQ_CLK_PCIE2_PHY>,
@@ -545,6 +546,7 @@ pinctrl_pcie1: pcie1grp {
 		fsl,pins = <
 			MX8MQ_IOMUXC_I2C4_SDA_PCIE2_CLKREQ_B		0x76
 			MX8MQ_IOMUXC_ECSPI2_MISO_GPIO5_IO12		0x16
+			MX8MQ_IOMUXC_ECSPI2_MOSI_GPIO5_IO11		0x41
 		>;
 	};
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lucas Stach]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] PCI: imx6: Add pci host wakeup support on imx platforms.

[Lucas Stach]

Am Freitag, dem 08.12.2023 um 17:13 +0800 schrieb Sherry Sun:
> Add pci host wakeup feature for imx platforms.
> Example of configuring the corresponding dts property under the PCI
> node:
> host-wake-gpio = <&gpio5 21 GPIO_ACTIVE_LOW>;
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 69 +++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 74703362aeec..050c9140f4a3 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -72,6 +72,7 @@ struct imx6_pcie_drvdata {
>  struct imx6_pcie {
>  	struct dw_pcie		*pci;
>  	int			reset_gpio;
> +	int			host_wake_irq;
>  	bool			gpio_active_high;
>  	bool			link_is_up;
>  	struct clk		*pcie_bus;
> @@ -1237,11 +1238,46 @@ static int imx6_pcie_resume_noirq(struct device *dev)
>  	return 0;
>  }
>  
> +static int imx6_pcie_suspend(struct device *dev)
> +{
> +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> +	if (imx6_pcie->host_wake_irq >= 0)
> +		enable_irq_wake(imx6_pcie->host_wake_irq);
> +
> +	return 0;
> +}
> +
> +static int imx6_pcie_resume(struct device *dev)
> +{
> +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> +
> +	if (imx6_pcie->host_wake_irq >= 0)
> +		disable_irq_wake(imx6_pcie->host_wake_irq);
> +
> +	return 0;
> +}
> +
>  static const struct dev_pm_ops imx6_pcie_pm_ops = {
>  	NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
>  				  imx6_pcie_resume_noirq)
> +	SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend, imx6_pcie_resume)
>  };
>  
> +irqreturn_t host_wake_irq_handler(int irq, void *priv)
> +{
> +	struct imx6_pcie *imx6_pcie = priv;
> +	struct device *dev = imx6_pcie->pci->dev;
> +
> +	dev_dbg(dev, "%s: host wakeup by pcie", __func__);
> +
Not sure how much value this debug print carries. If you want to keep
it, drop the __func__. There is no other place in this driver handling
the wakeup, so the function name in the print is pure noise.

> +	/* Notify PM core we are wakeup source */
> +	pm_wakeup_event(dev, 0);
> +	pm_system_wakeup();
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1250,6 +1286,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  	struct device_node *np;
>  	struct resource *dbi_base;
>  	struct device_node *node = dev->of_node;
> +	struct gpio_desc *host_wake_gpio;
>  	int ret;
>  	u16 val;
>  
> @@ -1457,6 +1494,32 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  			val |= PCI_MSI_FLAGS_ENABLE;
>  			dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
>  		}
> +
> +		/* host wakeup support */
> +		imx6_pcie->host_wake_irq = -1;
> +		host_wake_gpio = devm_gpiod_get_optional(dev, "host-wake", GPIOD_IN);
> +		if (IS_ERR(host_wake_gpio))
> +			return PTR_ERR(host_wake_gpio);
> +
> +		if (host_wake_gpio != NULL) {
> +			imx6_pcie->host_wake_irq = gpiod_to_irq(host_wake_gpio);
> +			ret = devm_request_threaded_irq(dev, imx6_pcie->host_wake_irq, NULL,
> +							host_wake_irq_handler,
> +							IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +							"host_wake", imx6_pcie);
> +			if (ret) {
> +				dev_err(dev, "Failed to request host_wake_irq %d (%d)\n",
> +					imx6_pcie->host_wake_irq, ret);
> +				imx6_pcie->host_wake_irq = -1;

What's the point of resetting host_wake_irq to -1 in those error paths?
Nobody is going to access this member anymore after the error. Just
drop this.

You could simplify all those error paths to
if (err)
    return dev_err_probe(...);

> +				return ret;
> +			}
> +
> +			if (device_init_wakeup(dev, true)) {
> +				dev_err(dev, "fail to init host_wake_irq\n");
> +				imx6_pcie->host_wake_irq = -1;
> +				return ret;
> +			}
> +		}
>  	}
>  
>  	return 0;
> @@ -1466,6 +1529,12 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev);
>  
> +	if (imx6_pcie->host_wake_irq >= 0) {
> +		device_init_wakeup(&pdev->dev, false);
> +		disable_irq(imx6_pcie->host_wake_irq);
> +		imx6_pcie->host_wake_irq = -1;
> +	}
> +
>  	/* bring down link, so bootloader gets clean state in case of reboot */
>  	imx6_pcie_assert_core_reset(imx6_pcie);
>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sherry Sun]

> -----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/4] PCI: imx6: Add pci host wakeup support on imx platforms.

[Sherry Sun]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2023年12月8日 18:16
> 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 1/4] PCI: imx6: Add pci host wakeup support on imx
> platforms.
> 
> Am Freitag, dem 08.12.2023 um 17:13 +0800 schrieb Sherry Sun:
> > Add pci host wakeup feature for imx platforms.
> > Example of configuring the corresponding dts property under the PCI
> > node:
> > host-wake-gpio = <&gpio5 21 GPIO_ACTIVE_LOW>;
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 69
> > +++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 74703362aeec..050c9140f4a3 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -72,6 +72,7 @@ struct imx6_pcie_drvdata {  struct imx6_pcie {
> >  	struct dw_pcie		*pci;
> >  	int			reset_gpio;
> > +	int			host_wake_irq;
> >  	bool			gpio_active_high;
> >  	bool			link_is_up;
> >  	struct clk		*pcie_bus;
> > @@ -1237,11 +1238,46 @@ static int imx6_pcie_resume_noirq(struct
> device *dev)
> >  	return 0;
> >  }
> >
> > +static int imx6_pcie_suspend(struct device *dev) {
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	if (imx6_pcie->host_wake_irq >= 0)
> > +		enable_irq_wake(imx6_pcie->host_wake_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx6_pcie_resume(struct device *dev) {
> > +	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> > +
> > +	if (imx6_pcie->host_wake_irq >= 0)
> > +		disable_irq_wake(imx6_pcie->host_wake_irq);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct dev_pm_ops imx6_pcie_pm_ops = {
> >  	NOIRQ_SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend_noirq,
> >  				  imx6_pcie_resume_noirq)
> > +	SYSTEM_SLEEP_PM_OPS(imx6_pcie_suspend, imx6_pcie_resume)
> >  };
> >
> > +irqreturn_t host_wake_irq_handler(int irq, void *priv) {
> > +	struct imx6_pcie *imx6_pcie = priv;
> > +	struct device *dev = imx6_pcie->pci->dev;
> > +
> > +	dev_dbg(dev, "%s: host wakeup by pcie", __func__);
> > +
> Not sure how much value this debug print carries. If you want to keep it,
> drop the __func__. There is no other place in this driver handling the wakeup,
> so the function name in the print is pure noise.

Ok, will remove the debug print info here.

> 
> > +	/* Notify PM core we are wakeup source */
> > +	pm_wakeup_event(dev, 0);
> > +	pm_system_wakeup();
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int imx6_pcie_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> > @@ -1250,6 +1286,7 @@ static int imx6_pcie_probe(struct
> platform_device *pdev)
> >  	struct device_node *np;
> >  	struct resource *dbi_base;
> >  	struct device_node *node = dev->of_node;
> > +	struct gpio_desc *host_wake_gpio;
> >  	int ret;
> >  	u16 val;
> >
> > @@ -1457,6 +1494,32 @@ static int imx6_pcie_probe(struct
> platform_device *pdev)
> >  			val |= PCI_MSI_FLAGS_ENABLE;
> >  			dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS,
> val);
> >  		}
> > +
> > +		/* host wakeup support */
> > +		imx6_pcie->host_wake_irq = -1;
> > +		host_wake_gpio = devm_gpiod_get_optional(dev, "host-
> wake", GPIOD_IN);
> > +		if (IS_ERR(host_wake_gpio))
> > +			return PTR_ERR(host_wake_gpio);
> > +
> > +		if (host_wake_gpio != NULL) {
> > +			imx6_pcie->host_wake_irq =
> gpiod_to_irq(host_wake_gpio);
> > +			ret = devm_request_threaded_irq(dev, imx6_pcie-
> >host_wake_irq, NULL,
> > +
> 	host_wake_irq_handler,
> > +							IRQF_ONESHOT |
> IRQF_TRIGGER_FALLING,
> > +							"host_wake",
> imx6_pcie);
> > +			if (ret) {
> > +				dev_err(dev, "Failed to request
> host_wake_irq %d (%d)\n",
> > +					imx6_pcie->host_wake_irq, ret);
> > +				imx6_pcie->host_wake_irq = -1;
> 
> What's the point of resetting host_wake_irq to -1 in those error paths?
> Nobody is going to access this member anymore after the error. Just drop
> this.
> 
> You could simplify all those error paths to if (err)
>     return dev_err_probe(...);

Sounds reasonable, I will remove the host_wake_irq resetting here and use dev_err_probe() instead, thanks!

Best Regards
Sherry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sherry Sun]

> -----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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel