[PATCH 1/1] PCI: armada8k: add device reset to link-down handle

[PATCH 1/1] PCI: armada8k: add device reset to link-down handle

[Jenishkumar Maheshbhai Patel]

Added pcie reset via gpio support as described in the
designware-pcie.txt DT binding document.
In cases link down cause still exist in device.
The device need to be reset to reestablish the link.
If reset-gpio pin provided in the device tree, then the linkdown
handle resets the device before reestablishing link.

Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@marvell.com>
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 24 ++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index b1b48c2016f7..9a48ef60be51 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -23,6 +23,7 @@
 #include <linux/of_pci.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/of_gpio.h>
 
 #include "pcie-designware.h"
 
@@ -37,6 +38,8 @@ struct armada8k_pcie {
 	struct regmap *sysctrl_base;
 	u32 mac_rest_bitmask;
 	struct work_struct recover_link_work;
+	enum of_gpio_flags flags;
+	struct gpio_desc *reset_gpio;
 };
 
 #define PCIE_VENDOR_REGS_OFFSET		0x8000
@@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws)
 	}
 	pci_lock_rescan_remove();
 	pci_stop_and_remove_bus_device(root_port);
+	/* Reset device if reset gpio is set */
+	if (pcie->reset_gpio) {
+		/* assert and then deassert the reset signal */
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+		msleep(100);
+		gpiod_set_value_cansleep(pcie->reset_gpio,
+					 (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1);
+	}
 	/*
-	 * Sleep needed to make sure all pcie transactions and access
-	 * are flushed before resetting the mac
+	 * Sleep used for two reasons.
+	 * First make sure all pcie transactions and access are flushed before resetting the mac
+	 * and second to make sure pci device is ready in case we reset the device
 	 */
 	msleep(100);
 
@@ -376,6 +388,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
 	struct armada8k_pcie *pcie;
 	struct device *dev = &pdev->dev;
 	struct resource *base;
+	int reset_gpio;
 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
@@ -420,6 +433,13 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
 		goto fail_clkreg;
 	}
 
+	/* Config reset gpio for pcie if the reset connected to gpio */
+	reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+					     "reset-gpios", 0,
+					     &pcie->flags);
+	if (gpio_is_valid(reset_gpio))
+		pcie->reset_gpio = gpio_to_desc(reset_gpio);
+
 	pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
 						       "marvell,system-controller");
 	if (IS_ERR(pcie->sysctrl_base)) {
-- 
2.25.1

Re: [PATCH 1/1] PCI: armada8k: add device reset to link-down handle

[Russell King (Oracle)]

On Mon, Nov 11, 2024 at 11:03:10PM -0800, Jenishkumar Maheshbhai Patel wrote:
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index b1b48c2016f7..9a48ef60be51 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -23,6 +23,7 @@
>  #include <linux/of_pci.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/of_gpio.h>
>  
>  #include "pcie-designware.h"
>  
> @@ -37,6 +38,8 @@ struct armada8k_pcie {
>  	struct regmap *sysctrl_base;
>  	u32 mac_rest_bitmask;
>  	struct work_struct recover_link_work;
> +	enum of_gpio_flags flags;
> +	struct gpio_desc *reset_gpio;
>  };
>  
>  #define PCIE_VENDOR_REGS_OFFSET		0x8000
> @@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws)
>  	}
>  	pci_lock_rescan_remove();
>  	pci_stop_and_remove_bus_device(root_port);
> +	/* Reset device if reset gpio is set */
> +	if (pcie->reset_gpio) {
> +		/* assert and then deassert the reset signal */
> +		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> +		msleep(100);
> +		gpiod_set_value_cansleep(pcie->reset_gpio,
> +					 (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1);

This looks wrong. resets are normally active-low.
gpiod_set_value_cansleep() should be called with '1' to indicate active
state, and '0' to indicate de-active state. DT should specify whether
the signal is active high or active low.

> +	/* Config reset gpio for pcie if the reset connected to gpio */
> +	reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
> +					     "reset-gpios", 0,
> +					     &pcie->flags);
> +	if (gpio_is_valid(reset_gpio))
> +		pcie->reset_gpio = gpio_to_desc(reset_gpio);
> +

Just use devm_fwnode_gpiod_get() here, which will also handle the
cleanup.

To see how this should be done, look at
drivers/pci/controller/pci-mvebu.c which gets the polarity settings
correct too, as mentioned above.

Lastly, as this patch is related to the breaking DT change, it
_should_ have been sent as a patch series (which means the same
Cc list on each patch) so that reviewers can see the full story
and comments on the other patches.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/1] PCI: armada8k: add device reset to link-down handle

[Bjorn Helgaas]

In subject, follow capitalization convention.  Use "git log
--oneline".

On Mon, Nov 11, 2024 at 11:03:10PM -0800, Jenishkumar Maheshbhai Patel wrote:
> Added pcie reset via gpio support as described in the
> designware-pcie.txt DT binding document.
> In cases link down cause still exist in device.
> The device need to be reset to reestablish the link.
> If reset-gpio pin provided in the device tree, then the linkdown
> handle resets the device before reestablishing link.

s/pcie/PCIe/
s/gpio/GPIO/

Add blank lines between paragraphs.  Rewrap to fill 75 columns.

> Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@marvell.com>
> ---
>  drivers/pci/controller/dwc/pcie-armada8k.c | 24 ++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index b1b48c2016f7..9a48ef60be51 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -23,6 +23,7 @@
>  #include <linux/of_pci.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/of_gpio.h>

Preserve (mostly) alpha sorted list of includes.

>  #include "pcie-designware.h"
>  
> @@ -37,6 +38,8 @@ struct armada8k_pcie {
>  	struct regmap *sysctrl_base;
>  	u32 mac_rest_bitmask;
>  	struct work_struct recover_link_work;
> +	enum of_gpio_flags flags;
> +	struct gpio_desc *reset_gpio;
>  };
>  
>  #define PCIE_VENDOR_REGS_OFFSET		0x8000
> @@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws)
>  	}
>  	pci_lock_rescan_remove();
>  	pci_stop_and_remove_bus_device(root_port);
> +	/* Reset device if reset gpio is set */
> +	if (pcie->reset_gpio) {
> +		/* assert and then deassert the reset signal */
> +		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> +		msleep(100);

Needs some sort of #define for this 100 ms.

> +		gpiod_set_value_cansleep(pcie->reset_gpio,
> +					 (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1);
> +	}
>  	/*
> -	 * Sleep needed to make sure all pcie transactions and access
> -	 * are flushed before resetting the mac
> +	 * Sleep used for two reasons.
> +	 * First make sure all pcie transactions and access are flushed before resetting the mac
> +	 * and second to make sure pci device is ready in case we reset the device
>  	 */
>  	msleep(100);

s/pcie/PCIe/ (throughout)
s/mac/MAC/

Explain the 100ms.  Hopefully this is something defined by PCIe base
or CEM spec.  Use or add #define as needed.

> @@ -376,6 +388,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>  	struct armada8k_pcie *pcie;
>  	struct device *dev = &pdev->dev;
>  	struct resource *base;
> +	int reset_gpio;
>  	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> @@ -420,6 +433,13 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>  		goto fail_clkreg;
>  	}
>  
> +	/* Config reset gpio for pcie if the reset connected to gpio */
> +	reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
> +					     "reset-gpios", 0,
> +					     &pcie->flags);
> +	if (gpio_is_valid(reset_gpio))
> +		pcie->reset_gpio = gpio_to_desc(reset_gpio);
> +
>  	pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>  						       "marvell,system-controller");
>  	if (IS_ERR(pcie->sysctrl_base)) {
> -- 
> 2.25.1
> 

RE: [PATCH 1/1] PCI: armada8k: add device reset to link-down handle

[Wilson Ding]

> In subject, follow capitalization convention.  Use "git log --oneline".
> 
> On Mon, Nov 11, 2024 at 11:03:10PM -0800, Jenishkumar Maheshbhai Patel
> wrote:
> > Added pcie reset via gpio support as described in the
> > designware-pcie.txt DT binding document.
> > In cases link down cause still exist in device.
> > The device need to be reset to reestablish the link.
> > If reset-gpio pin provided in the device tree, then the linkdown
> > handle resets the device before reestablishing link.
> 
> s/pcie/PCIe/
> s/gpio/GPIO/
> 
> Add blank lines between paragraphs.  Rewrap to fill 75 columns.
> 
> > Signed-off-by: Jenishkumar Maheshbhai Patel
> > <mailto:jpatel2@marvell.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-armada8k.c | 24
> > ++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c
> > b/drivers/pci/controller/dwc/pcie-armada8k.c
> > index b1b48c2016f7..9a48ef60be51 100644
> > --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/of_pci.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/regmap.h>
> > +#include <linux/of_gpio.h>
> 
> Preserve (mostly) alpha sorted list of includes.
> 
> >  #include "pcie-designware.h"
> >
> > @@ -37,6 +38,8 @@ struct armada8k_pcie {
> >  	struct regmap *sysctrl_base;
> >  	u32 mac_rest_bitmask;
> >  	struct work_struct recover_link_work;
> > +	enum of_gpio_flags flags;
> > +	struct gpio_desc *reset_gpio;
> >  };
> >
> >  #define PCIE_VENDOR_REGS_OFFSET		0x8000
> > @@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct
> work_struct *ws)
> >  	}
> >  	pci_lock_rescan_remove();
> >  	pci_stop_and_remove_bus_device(root_port);
> > +	/* Reset device if reset gpio is set */
> > +	if (pcie->reset_gpio) {
> > +		/* assert and then deassert the reset signal */
> > +		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> > +		msleep(100);
> 
> Needs some sort of #define for this 100 ms.
> 
> > +		gpiod_set_value_cansleep(pcie->reset_gpio,
> > +					 (pcie->flags &
> OF_GPIO_ACTIVE_LOW) ? 0 : 1);
> > +	}
> >  	/*
> > -	 * Sleep needed to make sure all pcie transactions and access
> > -	 * are flushed before resetting the mac
> > +	 * Sleep used for two reasons.
> > +	 * First make sure all pcie transactions and access are flushed before
> resetting the mac
> > +	 * and second to make sure pci device is ready in case we reset the
> > +device
> >  	 */
> >  	msleep(100);
> 
> s/pcie/PCIe/ (throughout)
> s/mac/MAC/
> 
> Explain the 100ms.  Hopefully this is something defined by PCIe base or CEM
> spec.  Use or add #define as needed.
> 

I will double check if this delay is necessary or not.

> > @@ -376,6 +388,7 @@ static int armada8k_pcie_probe(struct
> platform_device *pdev)
> >  	struct armada8k_pcie *pcie;
> >  	struct device *dev = &pdev->dev;
> >  	struct resource *base;
> > +	int reset_gpio;
> >  	int ret;
> >
> >  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); @@ -420,6
> > +433,13 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
> >  		goto fail_clkreg;
> >  	}
> >
> > +	/* Config reset gpio for pcie if the reset connected to gpio */
> > +	reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
> > +					     "reset-gpios", 0,
> > +					     &pcie->flags);
> > +	if (gpio_is_valid(reset_gpio))
> > +		pcie->reset_gpio = gpio_to_desc(reset_gpio);
> > +
> >  	pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev-
> >dev.of_node,
> >  						       "marvell,system-
> controller");
> >  	if (IS_ERR(pcie->sysctrl_base)) {
> > --
> > 2.25.1
> >