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