[PATCH v4 4/5] PCI: imx6: Convert to agnostic GPIO API

[Hongxing Zhu]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Sent: 2024Äê5ÔÂ6ÈÕ 22:21
> To: Manivannan Sadhasivam <manivannan.sadhasivam at linaro.org>; Frank Li
> <frank.li at nxp.com>; Krzysztof Wilczy¨½ski <kwilczynski at kernel.org>; Andy
> Shevchenko <andriy.shevchenko at linux.intel.com>; linux-omap at vger.kernel.org;
> linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; imx at lists.linux.dev;
> linux-amlogic at lists.infradead.org; linux-arm-msm at vger.kernel.org;
> linux-tegra at vger.kernel.org
> Cc: Vignesh Raghavendra <vigneshr at ti.com>; Siddharth Vadapalli
> <s-vadapalli at ti.com>; Lorenzo Pieralisi <lpieralisi at kernel.org>; Krzysztof
> Wilczy¨½ski <kw at linux.com>; Rob Herring <robh at kernel.org>; Bjorn Helgaas
> <bhelgaas at google.com>; Hongxing Zhu <hongxing.zhu at nxp.com>; Lucas Stach
> <l.stach at pengutronix.de>; Shawn Guo <shawnguo at kernel.org>; Sascha Hauer
> <s.hauer at pengutronix.de>; Pengutronix Kernel Team <kernel at pengutronix.de>;
> Fabio Estevam <festevam at gmail.com>; Yue Wang <yue.wang at Amlogic.com>;
> Neil Armstrong <neil.armstrong at linaro.org>; Kevin Hilman
> <khilman at baylibre.com>; Jerome Brunet <jbrunet at baylibre.com>; Martin
> Blumenstingl <martin.blumenstingl at googlemail.com>; Xiaowei Song
> <songxiaowei at hisilicon.com>; Binghui Wang <wangbinghui at hisilicon.com>;
> Thierry Reding <thierry.reding at gmail.com>; Jonathan Hunter
> <jonathanh at nvidia.com>; Thomas Petazzoni <thomas.petazzoni at bootlin.com>;
> Pali Roh¨¢r <pali at kernel.org>; Linus Walleij <linus.walleij at linaro.org>
> Subject: [PATCH v4 4/5] PCI: imx6: Convert to agnostic GPIO API
> 
> The of_gpio.h is going to be removed. In preparation of that convert the driver to
> the agnostic API.
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam at linaro.org>
> Reviewed-by: Frank Li <Frank.Li at nxp.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 36 ++++++++-------------------
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 917c69edee1d..62a4994c5501 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -11,14 +11,13 @@
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>  #include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/of_gpio.h>
>  #include <linux/of_address.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> @@ -107,8 +106,7 @@ struct imx6_pcie_drvdata {
> 
>  struct imx6_pcie {
>  	struct dw_pcie		*pci;
> -	int			reset_gpio;
> -	bool			gpio_active_high;
> +	struct gpio_desc	*reset_gpiod;
>  	bool			link_is_up;
>  	struct clk_bulk_data	clks[IMX6_PCIE_MAX_CLKS];
>  	struct regmap		*iomuxc_gpr;
> @@ -721,9 +719,7 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
>  	}
> 
>  	/* Some boards don't have PCIe reset GPIO. */
> -	if (gpio_is_valid(imx6_pcie->reset_gpio))
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					imx6_pcie->gpio_active_high);
> +	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
>  }
> 
>  static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) @@
> -771,10 +767,9 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie
> *imx6_pcie)
>  	}
> 
>  	/* Some boards don't have PCIe reset GPIO. */
> -	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +	if (imx6_pcie->reset_gpiod) {
>  		msleep(100);
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					!imx6_pcie->gpio_active_high);
> +		gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 0);
>  		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
>  		msleep(100);
>  	}
> @@ -1285,22 +1280,11 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
>  		return PTR_ERR(pci->dbi_base);
> 
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = of_get_named_gpio(node, "reset-gpio", 0);
> -	imx6_pcie->gpio_active_high = of_property_read_bool(node,
> -						"reset-gpio-active-high");
> -	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> -		ret = devm_gpio_request_one(dev, imx6_pcie->reset_gpio,
> -				imx6_pcie->gpio_active_high ?
> -					GPIOF_OUT_INIT_HIGH :
> -					GPIOF_OUT_INIT_LOW,
> -				"PCIe reset");
> -		if (ret) {
> -			dev_err(dev, "unable to get reset gpio\n");
> -			return ret;
> -		}
> -	} else if (imx6_pcie->reset_gpio == -EPROBE_DEFER) {
> -		return imx6_pcie->reset_gpio;
> -	}
Hi Andy:
Please correct me if my understand is wrong.
The "reset-gpio-active-high" property is added for some buggy board designs.
On these buggy boards, the reset gpio is active high.
In the other words, the PERST# is active and remote endpoint device would
be in reset stat when this gpio is high on these buggy boards.

I'm afraid that the PCIe would be broken on these boards, If these codes
 are removed totally, and toggle the reset GPIO pin like below.
...
gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
msleep(100);
gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
...

By the way, this reset GPIO pin should be high at end in
imx6_pcie_deassert_core_reset() if the imx6_pcie->gpio_active_high is zero. 

Best Regards
Richard Zhu

> +	imx6_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> +	if (IS_ERR(imx6_pcie->reset_gpiod))
> +		return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpiod),
> +				     "unable to get reset gpio\n");
> +	gpiod_set_consumer_name(imx6_pcie->reset_gpiod, "PCIe reset");
> 
>  	if (imx6_pcie->drvdata->clks_cnt >= IMX6_PCIE_MAX_CLKS)
>  		return dev_err_probe(dev, -ENOMEM, "clks_cnt is too big\n");
> --
> 2.43.0.rc1.1336.g36b5255a03ac