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 <[email protected]>
---
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(-)
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 <[email protected]>
> > ---
> > 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 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 <[email protected]>
> ---
> 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
>
[+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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > ---
> > > 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/[email protected]/
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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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