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
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 <[email protected]>
Reviewed-by: Richard Zhu <[email protected]>
---
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
Add host-wake-gpio property that can be used to wakeup the host
processor.
Signed-off-by: Sherry Sun <[email protected]>
Reviewed-by: Richard Zhu <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Richard Zhu <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Richard Zhu <[email protected]>
---
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 = <®_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
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 <[email protected]>
> Reviewed-by: Richard Zhu <[email protected]>
> ---
> 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
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 <[email protected]>
> Reviewed-by: Richard Zhu <[email protected]>
> ---
> 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);
> }
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 <[email protected]>
> > Reviewed-by: Richard Zhu <[email protected]>
> > ---
> > 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
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
> -----Original Message-----
> From: Lucas Stach <[email protected]>
> Sent: 2023年12月8日 18:00
> To: Sherry Sun <[email protected]>; Hongxing Zhu
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: dl-linux-imx <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]
> 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 <[email protected]>
> > Reviewed-by: Richard Zhu <[email protected]>
> > ---
> > 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
> -----Original Message-----
> From: Lucas Stach <[email protected]>
> Sent: 2023年12月8日 18:16
> To: Sherry Sun <[email protected]>; Hongxing Zhu
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: dl-linux-imx <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]
> 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 <[email protected]>
> > Reviewed-by: Richard Zhu <[email protected]>
> > ---
> > 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
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 2023??12??9?? 4:56
> To: Lucas Stach <[email protected]>
> Cc: Sherry Sun <[email protected]>; Hongxing Zhu
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx <linux-
> [email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]
> 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 <[email protected]>
> > > Reviewed-by: Richard Zhu <[email protected]>
> > > ---
> > > 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