While at it, remove of_gpio.h leftover from some of the drivers.
In v3:
- added precursor patch 1 to avoid build errors (LKP)
- used GPIOD_OUT_LOW instead of GPIOD_ASIS (Mani)
- added tags (Mani, Frank)
In v2:
- combined previously sent patches into a series (Manivannan)
- added tags (Rob, Manivannan)
- converted iMX.6 driver (Manivannan)
- dropped leftover in aadvark drivers (Manivannan)
Andy Shevchenko (5):
PCI: dra7xx: Add missing header inclusion
PCI: aardvark: Remove unused of_gpio.h
PCI: dwc: Remove unused of_gpio.h
PCI: imx6: Convert to agnostic GPIO API
PCI: kirin: Convert to agnostic GPIO API
drivers/pci/controller/dwc/pci-dra7xx.c | 2 +-
drivers/pci/controller/dwc/pci-imx6.c | 37 +++-----
drivers/pci/controller/dwc/pci-meson.c | 1 -
drivers/pci/controller/dwc/pcie-kirin.c | 105 +++++++--------------
drivers/pci/controller/dwc/pcie-qcom.c | 1 -
drivers/pci/controller/dwc/pcie-tegra194.c | 2 -
drivers/pci/controller/pci-aardvark.c | 1 -
7 files changed, 50 insertions(+), 99 deletions(-)
--
2.43.0.rc1.1336.g36b5255a03ac
The of_gpio.h is going to be removed. In preparation of that convert
the driver to the agnostic API.
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Frank Li <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 37 ++++++++++-----------------
1 file changed, 14 insertions(+), 23 deletions(-)
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");
--
2.43.0.rc1.1336.g36b5255a03ac
On Mon, Apr 29, 2024 at 01:23:17PM +0300, Andy Shevchenko wrote:
> While at it, remove of_gpio.h leftover from some of the drivers.
Can this be moved forward?
Or should I do something additionally with it?
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko
<[email protected]> 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 <[email protected]>
> Reviewed-by: Frank Li <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
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
> <[email protected]> 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 <[email protected]>
> > Reviewed-by: Frank Li <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> 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?
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Linus Walleij <[email protected]>
> Sent: 2024年5月6日 20:10
> To: Andy Shevchenko <[email protected]>
> Cc: Manivannan Sadhasivam <[email protected]>; Frank Li
> <[email protected]>; Krzysztof Wilczyński <[email protected]>; Uwe
> Kleine-König <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Vignesh Raghavendra <[email protected]>;
> Siddharth Vadapalli <[email protected]>; Lorenzo Pieralisi
> <[email protected]>; Krzysztof Wilczyński <[email protected]>; Rob Herring
> <[email protected]>; Bjorn Helgaas <[email protected]>; Hongxing Zhu
> <[email protected]>; Lucas Stach <[email protected]>; Shawn Guo
> <[email protected]>; Sascha Hauer <[email protected]>; Pengutronix
> Kernel Team <[email protected]>; Fabio Estevam <[email protected]>;
> Yue Wang <[email protected]>; Neil Armstrong
> <[email protected]>; Kevin Hilman <[email protected]>; Jerome
> Brunet <[email protected]>; Martin Blumenstingl
> <[email protected]>; Xiaowei Song
> <[email protected]>; Binghui Wang <[email protected]>;
> Thierry Reding <[email protected]>; Jonathan Hunter
> <[email protected]>; Thomas Petazzoni <[email protected]>;
> Pali Rohár <[email protected]>
> Subject: Re: [PATCH v3 4/5] PCI: imx6: Convert to agnostic GPIO API
>
> On Mon, Apr 29, 2024 at 12:25 PM Andy Shevchenko
> <[email protected]> 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 <[email protected]>
> > Reviewed-by: Frank Li <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> 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
> <[email protected]> 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 <[email protected]>
> > Reviewed-by: Frank Li <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> 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
> > <[email protected]> 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 <[email protected]>
> > > Reviewed-by: Frank Li <[email protected]>
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> >
> > 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
<[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
> > > > Reviewed-by: Frank Li <[email protected]>
> > > > Signed-off-by: Andy Shevchenko <[email protected]>
> > >
> > > 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