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 | 36 ++++++++-------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 917c69edee1d..62a4994c5501 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,8 +106,7 @@ struct imx6_pcie_drvdata {
struct imx6_pcie {
struct dw_pcie *pci;
- int reset_gpio;
- bool gpio_active_high;
+ struct gpio_desc *reset_gpiod;
bool link_is_up;
struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
struct regmap *iomuxc_gpr;
@@ -721,9 +719,7 @@ 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_value_cansleep(imx6_pcie->reset_gpiod, 1);
}
static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
@@ -771,10 +767,9 @@ 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_value_cansleep(imx6_pcie->reset_gpiod, 0);
/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
msleep(100);
}
@@ -1285,22 +1280,11 @@ 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", GPIOD_OUT_HIGH);
+ 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, May 06, 2024 at 05:20:40PM +0300, Andy Shevchenko 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]>
> ---
Can I include your patch in my imx6 pci improvement patches
https://lore.kernel.org/linux-pci/[email protected]/T/#t
There is patch, which rename function from imx6 -> imx. That will avoid
conflict.
Frank Li
> drivers/pci/controller/dwc/pci-imx6.c | 36 ++++++++-------------------
> 1 file changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 917c69edee1d..62a4994c5501 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,8 +106,7 @@ struct imx6_pcie_drvdata {
>
> struct imx6_pcie {
> struct dw_pcie *pci;
> - int reset_gpio;
> - bool gpio_active_high;
> + struct gpio_desc *reset_gpiod;
> bool link_is_up;
> struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
> struct regmap *iomuxc_gpr;
> @@ -721,9 +719,7 @@ 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_value_cansleep(imx6_pcie->reset_gpiod, 1);
> }
>
> static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> @@ -771,10 +767,9 @@ 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_value_cansleep(imx6_pcie->reset_gpiod, 0);
> /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> msleep(100);
> }
> @@ -1285,22 +1280,11 @@ 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", GPIOD_OUT_HIGH);
> + 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
>
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: 2024??5??6?? 22:21
> To: Manivannan Sadhasivam <[email protected]>; Frank Li
> <[email protected]>; Krzysztof Wilczy??ski <[email protected]>; Andy
> Shevchenko <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: 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]>; Linus Walleij <[email protected]>
> Subject: [PATCH v4 4/5] PCI: imx6: Convert to agnostic GPIO API
>
> 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 | 36 ++++++++-------------------
> 1 file changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 917c69edee1d..62a4994c5501 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,8 +106,7 @@ struct imx6_pcie_drvdata {
>
> struct imx6_pcie {
> struct dw_pcie *pci;
> - int reset_gpio;
> - bool gpio_active_high;
> + struct gpio_desc *reset_gpiod;
> bool link_is_up;
> struct clk_bulk_data clks[IMX6_PCIE_MAX_CLKS];
> struct regmap *iomuxc_gpr;
> @@ -721,9 +719,7 @@ 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_value_cansleep(imx6_pcie->reset_gpiod, 1);
> }
>
> static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) @@
> -771,10 +767,9 @@ 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_value_cansleep(imx6_pcie->reset_gpiod, 0);
> /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> msleep(100);
> }
> @@ -1285,22 +1280,11 @@ 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;
> - }
Hi Andy:
Please correct me if my understand is wrong.
The "reset-gpio-active-high" property is added for some buggy board designs.
On these buggy boards, the reset gpio is active high.
In the other words, the PERST# is active and remote endpoint device would
be in reset stat when this gpio is high on these buggy boards.
I'm afraid that the PCIe would be broken on these boards, If these codes
are removed totally, and toggle the reset GPIO pin like below.
...
gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
msleep(100);
gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
...
By the way, this reset GPIO pin should be high at end in
imx6_pcie_deassert_core_reset() if the imx6_pcie->gpio_active_high is zero.
Best Regards
Richard Zhu
> + imx6_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> + 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
Tue, May 07, 2024 at 03:22:33PM -0400, Frank Li kirjoitti:
> On Mon, May 06, 2024 at 05:20:40PM +0300, Andy Shevchenko wrote:
> > The of_gpio.h is going to be removed. In preparation of that convert
> > the driver to the agnostic API.
..
> Can I include your patch in my imx6 pci improvement patches
> https://lore.kernel.org/linux-pci/[email protected]/T/#t
>
> There is patch, which rename function from imx6 -> imx. That will avoid
> conflict.
You can, of course, but it will be not my decision to make. Ask PCI maintainers
who is supposed to take this series.
--
With Best Regards,
Andy Shevchenko
Thu, May 09, 2024 at 01:24:45AM +0000, Hongxing Zhu kirjoitti:
> > -----Original Message-----
> > From: Andy Shevchenko <[email protected]>
> > Sent: 2024年5月6日 22:21
..
> > - 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;
> > - }
> Please correct me if my understand is wrong.
> The "reset-gpio-active-high" property is added for some buggy board designs.
> On these buggy boards, the reset gpio is active high.
This is my understanding too.
> In the other words, the PERST# is active and remote endpoint device would
> be in reset stat when this gpio is high on these buggy boards.
Yes.
> I'm afraid that the PCIe would be broken on these boards, If these codes
> are removed totally,
No. Linus W. explained in the previous version review round how it's supposed
to work.
> and toggle the reset GPIO pin like below.
> ...
> gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> msleep(100);
> gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> ...
It's not the code that this patch adds. I'm not sure if I understand starting
from here what you mean.
> By the way, this reset GPIO pin should be high at end in
> imx6_pcie_deassert_core_reset() if the imx6_pcie->gpio_active_high is zero.
This seems a terminology mixup. You probably meant "inactive".
And this is exactly the case with this patch.
If you start thinking in terms of "active"/"inactive" you will see that there
is no contradiction and no behaviour change. The quirk itself is located in
gpiolib-of.c..
--
With Best Regards,
Andy Shevchenko
On Mon, May 6, 2024 at 4:21 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]>
Would maybe mention in the commit that the quirk to gpiolib
is already in place (people are already confused by it) but no big
deal.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij