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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>