| Message ID | 20240429102510.2665280-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: controller: Move to agnostic GPIO API | expand |
On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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@linaro.org> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> I think there is a bug here, the code is respecting the OF property "reset-gpio-active-high" but the code in drivers/gpio/gpiolib-of.h actually has a quirk for this so you can just delete all the active high handling and rely on 1 = asserted and 0 = deasserted when using GPIO descriptors. Just delete this thing: imx6_pcie->gpio_active_high = of_property_read_bool(node, "reset-gpio-active-high"); Yours, Linus Walleij
On Mon, May 06, 2024 at 02:10:24PM +0200, Linus Walleij wrote: > On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > 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@linaro.org> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I think there is a bug here, the code is respecting the OF property > "reset-gpio-active-high" > but the code in drivers/gpio/gpiolib-of.h actually has a quirk for > this so you can just > delete all the active high handling and rely on 1 = asserted and 0 = > deasserted when > using GPIO descriptors. > > Just delete this thing: > imx6_pcie->gpio_active_high = of_property_read_bool(node, > "reset-gpio-active-high"); Good catch! Thank you, I'll update it in the next version. Can you review the rest meanwhile?
> -----Original Message----- > From: Linus Walleij <linus.walleij@linaro.org> > Sent: 2024年5月6日 20:10 > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; Frank Li > <frank.li@nxp.com>; Krzysztof Wilczyński <kwilczynski@kernel.org>; Uwe > Kleine-König <u.kleine-koenig@pengutronix.de>; linux-omap@vger.kernel.org; > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; imx@lists.linux.dev; > linux-amlogic@lists.infradead.org; linux-arm-msm@vger.kernel.org; > linux-tegra@vger.kernel.org; Vignesh Raghavendra <vigneshr@ti.com>; > Siddharth Vadapalli <s-vadapalli@ti.com>; Lorenzo Pieralisi > <lpieralisi@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Rob Herring > <robh@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Hongxing Zhu > <hongxing.zhu@nxp.com>; Lucas Stach <l.stach@pengutronix.de>; Shawn Guo > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix > Kernel Team <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; > Yue Wang <yue.wang@amlogic.com>; Neil Armstrong > <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Jerome > Brunet <jbrunet@baylibre.com>; Martin Blumenstingl > <martin.blumenstingl@googlemail.com>; Xiaowei Song > <songxiaowei@hisilicon.com>; Binghui Wang <wangbinghui@hisilicon.com>; > Thierry Reding <thierry.reding@gmail.com>; Jonathan Hunter > <jonathanh@nvidia.com>; Thomas Petazzoni <thomas.petazzoni@bootlin.com>; > Pali Rohár <pali@kernel.org> > Subject: Re: [PATCH v3 4/5] PCI: imx6: Convert to agnostic GPIO API > > On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > 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@linaro.org> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I think there is a bug here, the code is respecting the OF property > "reset-gpio-active-high" Yes, you're right. As I remember that this property is used for the buggy hardware design. In general implementation, the PERST# is active low. In pci_imx6 driver, it used to call the following callbacks to toggle perst# gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0); msleep(100); gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1); But, some buggy hardware designs use this GPIO signal active high. And the correct toggle sequence for thos board is reversed. gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1); msleep(100); gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0); So, this property is added for those buggy hardware designs. Best Regards Richard Zhu > but the code in drivers/gpio/gpiolib-of.h actually has a quirk for this so you can > just delete all the active high handling and rely on 1 = asserted and 0 = deasserted > when using GPIO descriptors. > > Just delete this thing: > imx6_pcie->gpio_active_high = of_property_read_bool(node, > "reset-gpio-active-high"); > > Yours, > Linus Walleij
On Mon, May 06, 2024 at 02:10:24PM +0200, Linus Walleij wrote: > On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > 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@linaro.org> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I think there is a bug here, the code is respecting the OF property > "reset-gpio-active-high" > but the code in drivers/gpio/gpiolib-of.h actually has a quirk for > this so you can just > delete all the active high handling and rely on 1 = asserted and 0 = > deasserted when > using GPIO descriptors. > Wow... So this bug is present even before this series, right? > Just delete this thing: > imx6_pcie->gpio_active_high = of_property_read_bool(node, > "reset-gpio-active-high"); But this is just a bandaid IMO. The flag for the PERST# GPIO should be properly set in the board dts as per the board design. - Mani
On Tue, May 07, 2024 at 01:44:56PM +0530, Manivannan Sadhasivam wrote: > On Mon, May 06, 2024 at 02:10:24PM +0200, Linus Walleij wrote: > > On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > 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@linaro.org> > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > I think there is a bug here, the code is respecting the OF property > > "reset-gpio-active-high" > > but the code in drivers/gpio/gpiolib-of.h actually has a quirk for > > this so you can just > > delete all the active high handling and rely on 1 = asserted and 0 = > > deasserted when > > using GPIO descriptors. > > > > Wow... > > So this bug is present even before this series, right? > > > Just delete this thing: > > imx6_pcie->gpio_active_high = of_property_read_bool(node, > > "reset-gpio-active-high"); > > But this is just a bandaid IMO. The flag for the PERST# GPIO should be properly > set in the board dts as per the board design. > Hmm, no. I was confused by the property. But this quirk in gpiolib-of.c is going to be applied while changing the GPIO state also or just during request time? - Mani
On Tue, May 7, 2024 at 10:28 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > On Tue, May 07, 2024 at 01:44:56PM +0530, Manivannan Sadhasivam wrote: > > On Mon, May 06, 2024 at 02:10:24PM +0200, Linus Walleij wrote: > > > On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > 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@linaro.org> > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > I think there is a bug here, the code is respecting the OF property > > > "reset-gpio-active-high" > > > but the code in drivers/gpio/gpiolib-of.h actually has a quirk for > > > this so you can just > > > delete all the active high handling and rely on 1 = asserted and 0 = > > > deasserted when > > > using GPIO descriptors. > > > > > > > Wow... > > > > So this bug is present even before this series, right? > > > > > Just delete this thing: > > > imx6_pcie->gpio_active_high = of_property_read_bool(node, > > > "reset-gpio-active-high"); > > > > But this is just a bandaid IMO. The flag for the PERST# GPIO should be properly > > set in the board dts as per the board design. > > > > Hmm, no. I was confused by the property. But this quirk in gpiolib-of.c is going > to be applied while changing the GPIO state also or just during request time? It's applied permanentlt at request and then the descriptors maintain their polarity state over the course of their lifetime. Yours, Linus Walleij
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 917c69edee1d..d620f1e1a43c 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,7 +106,7 @@ struct imx6_pcie_drvdata { struct imx6_pcie { struct dw_pcie *pci; - int reset_gpio; + struct gpio_desc *reset_gpiod; bool gpio_active_high; bool link_is_up; struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS]; @@ -721,9 +720,8 @@ 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_raw_value_cansleep(imx6_pcie->reset_gpiod, + imx6_pcie->gpio_active_high); } static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) @@ -771,10 +769,10 @@ 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_raw_value_cansleep(imx6_pcie->reset_gpiod, + !imx6_pcie->gpio_active_high); /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */ msleep(100); } @@ -1285,22 +1283,15 @@ 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; - } + imx6_pcie->reset_gpiod = + devm_gpiod_get_optional(dev, "reset", + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); + 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");