| Message ID | YYCOTx68LXu1Tn1i@fedora (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] PCI: imx6: Replace legacy gpio interface for gpiod interface | expand |
On Mon, Nov 01, 2021 at 10:03:11PM -0300, Maíra Canal wrote: > Considering the current transition of the GPIO subsystem, remove all > dependencies of the legacy GPIO interface (linux/gpio.h and linux > /of_gpio.h) and replace it with the descriptor-based GPIO approach. > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > --- > V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard > V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep > --- > drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ > 1 file changed, 10 insertions(+), 20 deletions(-) Maira, Lucas, what's this patch status ? Please let me know. Thanks, Lorenzo > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 80fc98acf097..f08865ac0b40 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -11,13 +11,12 @@ > #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_gpio.h> > #include <linux/of_device.h> > #include <linux/of_address.h> > #include <linux/pci.h> > @@ -63,7 +62,7 @@ struct imx6_pcie_drvdata { > > struct imx6_pcie { > struct dw_pcie *pci; > - int reset_gpio; > + struct gpio_desc *reset_gpio; > bool gpio_active_high; > struct clk *pcie_bus; > struct clk *pcie_phy; > @@ -526,11 +525,11 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > usleep_range(200, 500); > > /* Some boards don't have PCIe reset GPIO. */ > - if (gpio_is_valid(imx6_pcie->reset_gpio)) { > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > + if (imx6_pcie->reset_gpio) { > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, > imx6_pcie->gpio_active_high); > msleep(100); > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, > !imx6_pcie->gpio_active_high); > } > > @@ -1025,22 +1024,13 @@ 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_gpio = devm_gpiod_get_optional(dev, "reset", > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > + if (IS_ERR(imx6_pcie->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpio), > + "unable to get reset gpio\n"); > > /* Fetch clocks */ > imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy"); > -- > 2.31.1 >
On Fri, Apr 08, 2022 at 03:23:39PM +0100, Lorenzo Pieralisi wrote: > On Mon, Nov 01, 2021 at 10:03:11PM -0300, Maíra Canal wrote: > > Considering the current transition of the GPIO subsystem, remove all > > dependencies of the legacy GPIO interface (linux/gpio.h and linux > > /of_gpio.h) and replace it with the descriptor-based GPIO approach. > > > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > > --- > > V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard > > V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ > > 1 file changed, 10 insertions(+), 20 deletions(-) > > Maira, Lucas, > > what's this patch status ? Please let me know. Lorenzo, Thank you for the feedback. Since I sent v3, I didn't get any feedback from the community. If you have any feedback, I would gladly work on it. Thanks, Maíra Canal > > Thanks, > Lorenzo > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 80fc98acf097..f08865ac0b40 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -11,13 +11,12 @@ > > #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_gpio.h> > > #include <linux/of_device.h> > > #include <linux/of_address.h> > > #include <linux/pci.h> > > @@ -63,7 +62,7 @@ struct imx6_pcie_drvdata { > > > > struct imx6_pcie { > > struct dw_pcie *pci; > > - int reset_gpio; > > + struct gpio_desc *reset_gpio; > > bool gpio_active_high; > > struct clk *pcie_bus; > > struct clk *pcie_phy; > > @@ -526,11 +525,11 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > > usleep_range(200, 500); > > > > /* Some boards don't have PCIe reset GPIO. */ > > - if (gpio_is_valid(imx6_pcie->reset_gpio)) { > > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > > + if (imx6_pcie->reset_gpio) { > > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, > > imx6_pcie->gpio_active_high); > > msleep(100); > > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, > > !imx6_pcie->gpio_active_high); > > } > > > > @@ -1025,22 +1024,13 @@ 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_gpio = devm_gpiod_get_optional(dev, "reset", > > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > > + if (IS_ERR(imx6_pcie->reset_gpio)) > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpio), > > + "unable to get reset gpio\n"); > > > > /* Fetch clocks */ > > imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy"); > > -- > > 2.31.1 > >
On Fri, Apr 08, 2022 at 11:46:32AM -0300, Maíra Canal wrote: > On Fri, Apr 08, 2022 at 03:23:39PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Nov 01, 2021 at 10:03:11PM -0300, Maíra Canal wrote: > > > Considering the current transition of the GPIO subsystem, remove all > > > dependencies of the legacy GPIO interface (linux/gpio.h and linux > > > /of_gpio.h) and replace it with the descriptor-based GPIO approach. > > > > > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > > > --- > > > V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard > > > V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep > > > --- > > > drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ > > > 1 file changed, 10 insertions(+), 20 deletions(-) > > > > Maira, Lucas, > > > > what's this patch status ? Please let me know. > > Thank you for the feedback. Since I sent v3, I didn't get any feedback from the community. > > If you have any feedback, I would gladly work on it. Just for you to know that it will likely conflict with 'PCI: imx6: Fix PERST# start-up sequence' [0] that is also still waiting for some additional feedback. Francesco [0] https://lore.kernel.org/all/20220404081509.94356-1-francesco.dolcini@toradex.com/
[+Linus] On Fri, Apr 08, 2022 at 11:46:32AM -0300, Maíra Canal wrote: > On Fri, Apr 08, 2022 at 03:23:39PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Nov 01, 2021 at 10:03:11PM -0300, Maíra Canal wrote: > > > Considering the current transition of the GPIO subsystem, remove all > > > dependencies of the legacy GPIO interface (linux/gpio.h and linux > > > /of_gpio.h) and replace it with the descriptor-based GPIO approach. > > > > > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > > > --- > > > V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard > > > V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep > > > --- > > > drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ > > > 1 file changed, 10 insertions(+), 20 deletions(-) > > > > Maira, Lucas, > > > > what's this patch status ? Please let me know. > > > Lorenzo, > > Thank you for the feedback. Since I sent v3, I didn't get any feedback from the community. > > If you have any feedback, I would gladly work on it. I would ask Linus to have a look please given that it is GPIO code. Original thread: https://lore.kernel.org/linux-pci/YYCOTx68LXu1Tn1i@fedora Thanks, Lorenzo
Hi Maíra, hi Lorenzo, Am Montag, dem 11.04.2022 um 13:59 +0200 schrieb Francesco Dolcini: > On Fri, Apr 08, 2022 at 11:46:32AM -0300, Maíra Canal wrote: > > On Fri, Apr 08, 2022 at 03:23:39PM +0100, Lorenzo Pieralisi wrote: > > > On Mon, Nov 01, 2021 at 10:03:11PM -0300, Maíra Canal wrote: > > > > Considering the current transition of the GPIO subsystem, remove all > > > > dependencies of the legacy GPIO interface (linux/gpio.h and linux > > > > /of_gpio.h) and replace it with the descriptor-based GPIO approach. > > > > > > > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > > > > --- > > > > V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard > > > > V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep > > > > --- > > > > drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ > > > > 1 file changed, 10 insertions(+), 20 deletions(-) > > > > > > Maira, Lucas, > > > > > > what's this patch status ? Please let me know. The patch looks fine to me now, however... > > > > Thank you for the feedback. Since I sent v3, I didn't get any feedback from the community. > > > > If you have any feedback, I would gladly work on it. > > Just for you to know that it will likely conflict with > 'PCI: imx6: Fix PERST# start-up sequence' [0] that is also still waiting > for some additional feedback. ... they do conflict and I guess the functional fix for the reset sequence should have priority over this cleanup. Maíra, may I ask you to respin your patch as soon as Francescos fix is in? I promise to not let it fall through the cracks again. Regards, Lucas
On 04/11, Lucas Stach wrote: > Hi Maíra, hi Lorenzo, > > Am Montag, dem 11.04.2022 um 13:59 +0200 schrieb Francesco Dolcini: > > On Fri, Apr 08, 2022 at 11:46:32AM -0300, Maíra Canal wrote: > > > On Fri, Apr 08, 2022 at 03:23:39PM +0100, Lorenzo Pieralisi wrote: > > > > On Mon, Nov 01, 2021 at 10:03:11PM -0300, Maíra Canal wrote: > > > > > Considering the current transition of the GPIO subsystem, remove all > > > > > dependencies of the legacy GPIO interface (linux/gpio.h and linux > > > > > /of_gpio.h) and replace it with the descriptor-based GPIO approach. > > > > > > > > > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > > > > > --- > > > > > V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard > > > > > V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep > > > > > --- > > > > > drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ > > > > > 1 file changed, 10 insertions(+), 20 deletions(-) > > ... they do conflict and I guess the functional fix for the reset > sequence should have priority over this cleanup. Maíra, may I ask you > to respin your patch as soon as Francescos fix is in? I promise to not > let it fall through the cracks again. Hi Lucas That's fine. After Francescos patch is applied, I resend the fix. Thank you for the feedback! Sincerly, Maíra Canal > > Regards, > Lucas >
Hi Maira and sorry for being slow on reviews. On Tue, Nov 2, 2021 at 2:04 AM Maíra Canal <maira.canal@usp.br> wrote: > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, > !imx6_pcie->gpio_active_high); Hm I see you got advised to use the raw api. I'm not so sure about that I like v1 better. > + imx6_pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > + if (IS_ERR(imx6_pcie->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpio), > + "unable to get reset gpio\n"); Where is this descriptor coming from? Device trees? Can't we just fix the DTS file(s) in that case given how wrong they are if they don't set GPIO_ACTIVE_LOW flag on this IRQ. Yours, Linus Walleij
Hi Linus, Am Donnerstag, dem 21.04.2022 um 01:24 +0200 schrieb Linus Walleij: > Hi Maira and sorry for being slow on reviews. > > On Tue, Nov 2, 2021 at 2:04 AM Maíra Canal <maira.canal@usp.br> wrote: > > > - gpio_set_value_cansleep(imx6_pcie->reset_gpio, > > + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, > > !imx6_pcie->gpio_active_high); > > Hm I see you got advised to use the raw api. I'm not so sure about > that I like v1 better. > > > + imx6_pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); > > + if (IS_ERR(imx6_pcie->reset_gpio)) > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpio), > > + "unable to get reset gpio\n"); > > Where is this descriptor coming from? Device trees? Can't we just fix the > DTS file(s) in that case given how wrong they are if they don't set > GPIO_ACTIVE_LOW flag on this IRQ. The binding explicitly describes the GPIO as not polarity aware and has a separate property "reset-gpio-active-high" to avoid breaking old DTBs. I don't think it's helpful to dismiss this explicit backward compat just because the driver code looks nicer that way. Regards, Lucas
On Mon, Apr 25, 2022 at 2:07 PM Lucas Stach <l.stach@pengutronix.de> wrote: > The binding explicitly describes the GPIO as not polarity aware and has > a separate property "reset-gpio-active-high" to avoid breaking old > DTBs. I don't think it's helpful to dismiss this explicit backward > compat just because the driver code looks nicer that way. I see. We handle such things a specific way. Look in drivers/gpio/gpiolib-of.c, especially the function of_gpio_flags_quirks(). Here we special-case all bindings which for some reason introduced something necessary custom, like in this case not using the proper polarity flag. Add code to this file in the proper place to handle and hide the old style DTBs using "reset-gpio-active-high" as active high flag and assuming active low otherwise in this file. I imagine it begins with if (IS_ENABLED(CONFIG_PCI_IMX6)) { ... } Then modify the code in drivers/pci/controller/dwc/pci-imx6.c to act as if gpiolib handles polarity inversion. Include all changes to all files in the same patch so this is changed in tandem (one technical step). Yours, Linus Walleij
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 80fc98acf097..f08865ac0b40 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -11,13 +11,12 @@ #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_gpio.h> #include <linux/of_device.h> #include <linux/of_address.h> #include <linux/pci.h> @@ -63,7 +62,7 @@ struct imx6_pcie_drvdata { struct imx6_pcie { struct dw_pcie *pci; - int reset_gpio; + struct gpio_desc *reset_gpio; bool gpio_active_high; struct clk *pcie_bus; struct clk *pcie_phy; @@ -526,11 +525,11 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) usleep_range(200, 500); /* Some boards don't have PCIe reset GPIO. */ - if (gpio_is_valid(imx6_pcie->reset_gpio)) { - gpio_set_value_cansleep(imx6_pcie->reset_gpio, + if (imx6_pcie->reset_gpio) { + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, imx6_pcie->gpio_active_high); msleep(100); - gpio_set_value_cansleep(imx6_pcie->reset_gpio, + gpiod_set_raw_value_cansleep(imx6_pcie->reset_gpio, !imx6_pcie->gpio_active_high); } @@ -1025,22 +1024,13 @@ 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_gpio = devm_gpiod_get_optional(dev, "reset", + imx6_pcie->gpio_active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); + if (IS_ERR(imx6_pcie->reset_gpio)) + return dev_err_probe(dev, PTR_ERR(imx6_pcie->reset_gpio), + "unable to get reset gpio\n"); /* Fetch clocks */ imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (linux/gpio.h and linux /of_gpio.h) and replace it with the descriptor-based GPIO approach. Signed-off-by: Maíra Canal <maira.canal@usp.br> --- V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard V2 -> v3: Change gpiod_set_value_cansleep for gpiod_set_raw_value_cansleep --- drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-)