PCI: imx6: Replace legacy gpio interface for gpiod interface
Subsystems (4)
PATCH v3 1/1 PCI: imx6: Replace legacy gpio interface for gpiod interface
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.
---
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(-)
drivers/pci/controller/dwc/pci-imx6.c +10 -20
@@ -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" );
Signature · 1 line
2.31.1
Quoted 11 lines · Maíra Canal
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (
linux/gpio.hand 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: Changegpiod_set_value_cansleepforgpiod_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
Quoted 74 lines
drivers/pci/controller/dwc/pci-imx6.c +10
-20@@ -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" );Signature · 1 line
2.31.1
Quoted 13 lines · Lorenzo Pieralisi
Quoted 11 lines · Maíra Canal
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (
linux/gpio.hand 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: Changegpiod_set_value_cansleepforgpiod_set_raw_value_cansleep--- drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-)
Quoted 3 lines
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
Quoted 78 lines
Thanks,
LorenzoQuoted 74 lines
drivers/pci/controller/dwc/pci-imx6.c +10
-20@@ -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" );Signature · 1 line
2.31.1
Quoted 14 lines · Maíra Canal
Quoted 13 lines · Lorenzo Pieralisi
Quoted 11 lines · Maíra Canal
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (
linux/gpio.hand 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: Changegpiod_set_value_cansleepforgpiod_set_raw_value_cansleep--- drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-)
Quoted 7 lines
Quoted 3 lines
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
Lucas Stach
maintainerHi Maíra, hi Lorenzo,
Am Montag, dem 11.04.2022 um 13:59 +0200 schrieb Francesco Dolcini:
Quoted 15 lines · Maíra Canal
Quoted 14 lines · Lorenzo Pieralisi
Quoted 13 lines · Maíra Canal
Quoted 11 lines
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (
linux/gpio.hand 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: Changegpiod_set_value_cansleepforgpiod_set_raw_value_cansleep--- drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-)
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Maira, Lucas,
what's this patch status ? Please let me know.
The patch looks fine to me now, however...
Quoted 8 lines
Quoted 4 lines
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
Quoted 18 lines · Lucas Stach
Hi Maíra, hi Lorenzo,
Am Montag, dem 11.04.2022 um 13:59 +0200 schrieb Francesco Dolcini:
Quoted 7 lines · Maíra Canal
Quoted 6 lines · Lorenzo Pieralisi
Quoted 5 lines · Maíra Canal
Quoted 4 lines
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (
linux/gpio.hand linux /of_gpio.h) and replace it with the descriptor-based GPIO approach.Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Signed-off-by Maíra Canal <[email protected]>---
V1 -> V2: Rewrite commit log and subject line to match PCI subsystem standard V2 -> v3: Changegpiod_set_value_cansleepforgpiod_set_raw_value_cansleep--- drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-)
Quoted 4 lines
... 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
Quoted 4 lines
Regards,
Lucas
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[+Linus]
Quoted 16 lines · Maíra Canal
Quoted 15 lines · Lorenzo Pieralisi
Quoted 11 lines · Maíra Canal
Considering the current transition of the GPIO subsystem, remove all dependencies of the legacy GPIO interface (
linux/gpio.hand 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: Changegpiod_set_value_cansleepforgpiod_set_raw_value_cansleep--- drivers/pci/controller/dwc/pci-imx6.c | 30 +++++++++------------------ 1 file changed, 10 insertions(+), 20 deletions(-)Maira, Lucas,
Quoted 8 lines
Quoted 1 line
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 Maira and sorry for being slow on reviews.
drivers/pci/controller/dwc/pci-imx6.c static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) · line 533
Quoted 3 lines · Maíra Canal
drivers/pci/controller/dwc/pci-imx6.c +1
-1-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.
drivers/pci/controller/dwc/pci-imx6.c static int imx6_pcie_probe(struct platform_device *pdev) · line 1033
Quoted 5 lines
drivers/pci/controller/dwc/pci-imx6.c +5
-0+ 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
Lucas Stach
maintainerHi Linus,
Am Donnerstag, dem 21.04.2022 um 01:24 +0200 schrieb Linus Walleij:
Quoted 17 lines
Hi Maira and sorry for being slow on reviews.
drivers/pci/controller/dwc/pci-imx6.c static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) · line 533
Quoted 3 lines · Maíra Canal
drivers/pci/controller/dwc/pci-imx6.c +1
-1-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.
drivers/pci/controller/dwc/pci-imx6.c static int imx6_pcie_probe(struct platform_device *pdev) · line 1033
Quoted 5 lines
drivers/pci/controller/dwc/pci-imx6.c +5
-0+ 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" );
Quoted 3 lines
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_LOWflag 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
Quoted 4 lines · Lucas Stach
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