diff mbox

[v2] Revert "PCI: imx6: Add support for active-low reset GPIO"

Message ID 1459201536-5558-1-git-send-email-festevam@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Fabio Estevam March 28, 2016, 9:45 p.m. UTC
From: Fabio Estevam <fabio.estevam@nxp.com>

Commit 5c5fb40de8f14 ("PCI: imx6: Add support for active-low reset GPIO")
cause regressions on some boards like MX6 Gateworks Ventana, for example.

The reason for the breakage is that this commit sets the gpio polarity
in the wrong logic level.

Also, the commit log is wrong because active-low reset GPIO is what the
driver used to support since the beginning.

So keep the old behavior that ignores the gpio polarity specified in
the device tree and treat the PCI reset GPIO as active-low.

Cc: <stable@vger.kernel.org> # 4.5
Reported-by: Krzysztof Ha?asa <khalasa@piap.pl>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Changes since v1:
- Improve wording of commit log (Tim)

 drivers/pci/host/pci-imx6.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Tim Harvey March 29, 2016, 1:43 p.m. UTC | #1
On Mon, Mar 28, 2016 at 2:45 PM, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Commit 5c5fb40de8f14 ("PCI: imx6: Add support for active-low reset GPIO")
> cause regressions on some boards like MX6 Gateworks Ventana, for example.
>
> The reason for the breakage is that this commit sets the gpio polarity
> in the wrong logic level.
>
> Also, the commit log is wrong because active-low reset GPIO is what the
> driver used to support since the beginning.
>
> So keep the old behavior that ignores the gpio polarity specified in
> the device tree and treat the PCI reset GPIO as active-low.
>
> Cc: <stable@vger.kernel.org> # 4.5
> Reported-by: Krzysztof Ha?asa <khalasa@piap.pl>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
> Changes since v1:
> - Improve wording of commit log (Tim)
>
>  drivers/pci/host/pci-imx6.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index eb5a275..2f817fa 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -32,7 +32,7 @@
>  #define to_imx6_pcie(x)        container_of(x, struct imx6_pcie, pp)
>
>  struct imx6_pcie {
> -       struct gpio_desc        *reset_gpio;
> +       int                     reset_gpio;
>         struct clk              *pcie_bus;
>         struct clk              *pcie_phy;
>         struct clk              *pcie;
> @@ -309,10 +309,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>         usleep_range(200, 500);
>
>         /* Some boards don't have PCIe reset GPIO. */
> -       if (imx6_pcie->reset_gpio) {
> -               gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +               gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
>                 msleep(100);
> -               gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +               gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
>         }
>         return 0;
>
> @@ -523,6 +523,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>         struct imx6_pcie *imx6_pcie;
>         struct pcie_port *pp;
> +       struct device_node *np = pdev->dev.of_node;
>         struct resource *dbi_base;
>         struct device_node *node = pdev->dev.of_node;
>         int ret;
> @@ -544,8 +545,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>                 return PTR_ERR(pp->dbi_base);
>
>         /* Fetch GPIOs */
> -       imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> -                                                       GPIOD_OUT_LOW);
> +       imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +               ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> +                                           GPIOF_OUT_INIT_LOW, "PCIe reset");
> +               if (ret) {
> +                       dev_err(&pdev->dev, "unable to get reset gpio\n");
> +                       return ret;
> +               }
> +       }
>
>         /* Fetch clocks */
>         imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
> --
> 1.9.1
>

Fabio/Krzysztof - thanks for finding this regression that breaks PCI
on most IMX6. I verified reverting this resolves Gateworks Ventana PCI
breakage in v4.5.

Acked-by: Tim Harvey <tharvey@gateworks.com>

Regards,

Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Petr Štetiar March 30, 2016, 12:21 p.m. UTC | #2
Tim Harvey <tharvey@gateworks.com> [2016-03-29 06:43:19]:

> > Commit 5c5fb40de8f14 ("PCI: imx6: Add support for active-low reset GPIO")
> > cause regressions on some boards like MX6 Gateworks Ventana, for example.

Sorry for the problems.

> > The reason for the breakage is that this commit sets the gpio polarity
> > in the wrong logic level.

It's just a nitpick :-) but this commit doesn't set the polarity in the wrong
logic level, 

> > -       if (imx6_pcie->reset_gpio) {
> > -               gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> > +       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > +               gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> >                 msleep(100);
> > -               gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> > +               gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);

as you can see, the polarity was simply wrong from the beginning[1]. My patch
has just probably made the error more exposed.

1. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/host/pci-imx6.c?id=bb38919ec56e0758c3ae56dfc091dcde1391353e

-- ynezz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam March 30, 2016, 12:36 p.m. UTC | #3
Hi Petr,

On Wed, Mar 30, 2016 at 9:21 AM, Petr Štetiar <ynezz@true.cz> wrote:

> It's just a nitpick :-) but this commit doesn't set the polarity in the wrong
> logic level,

Yes, it does. That's why it causes regressions.

> as you can see, the polarity was simply wrong from the beginning[1]. My patch
> has just probably made the error more exposed.

No, polarity was active low since the beginning.

In order to make the Toradex board to work without breaking old dtb's
is to introduce a property like 'phy-reset-active-high' and handle it
in the driver.

Take a look at the FEC driver for a an example:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/freescale/fec_main.c?id=962d8cdc3133435aed2928637f73e272128a326c
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Petr Štetiar March 30, 2016, 1 p.m. UTC | #4
Fabio Estevam <festevam@gmail.com> [2016-03-30 09:36:35]:

Hi Fabio,

> In order to make the Toradex board to work without breaking old dtb's is to
> introduce a property like 'phy-reset-active-high' and handle it in the
> driver.

do we really need to do this?  With Krzysztof's patch it should work for both
cases correctly. If I understand gpiolib correctly, you don't need to
introduce reset-active-high, if you use gpiod_* functions, then the GPIO
polarity is handled correctly inside the gpiolib framework[1] and all you need
is to define pin's polarity correctly via gpio pin definition. Or am I missing
something?

Then we need to just fix the broken DTBs.

1. http://lxr.free-electrons.com/source/drivers/gpio/gpiolib.c#L1759

-- ynezz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam March 30, 2016, 1:07 p.m. UTC | #5
Hi Petr,

On Wed, Mar 30, 2016 at 10:00 AM, Petr Štetiar <ynezz@true.cz> wrote:

> do we really need to do this?  With Krzysztof's patch it should work for both
> cases correctly. If I understand gpiolib correctly, you don't need to
> introduce reset-active-high, if you use gpiod_* functions, then the GPIO
> polarity is handled correctly inside the gpiolib framework[1] and all you need
> is to define pin's polarity correctly via gpio pin definition. Or am I missing
> something?
>
> Then we need to just fix the broken DTBs.

If we apply Krzysztof's patch then we break PCI in imx6qdl-sabresd.

In this board we have:

reset-gpio = <&gpio7 12 0>;

, which is wrong. Of course, we should fix this to be 'reset-gpio =
<&gpio7 12 GPIO_ACTIVE_LOW>;'

However we should not break old dtb's. That's why reverting the patch
is the only solution we have so that old dtb's still work.

We have had this same problem for the FEC PHY reset. The solution
there was to add a new property to treat the active-high case.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Petr Štetiar March 30, 2016, 1:16 p.m. UTC | #6
Fabio Estevam <festevam@gmail.com> [2016-03-30 10:07:16]:

> If we apply Krzysztof's patch then we break PCI in imx6qdl-sabresd.
> 
> In this board we have:
> 
> reset-gpio = <&gpio7 12 0>;
> 
> , which is wrong. Of course, we should fix this to be 'reset-gpio =
> <&gpio7 12 GPIO_ACTIVE_LOW>;'
> 
> However we should not break old dtb's. That's why reverting the patch
> is the only solution we have so that old dtb's still work.

But this only apply for stable right? I understand this and it's fine with me.

> We have had this same problem for the FEC PHY reset. The solution
> there was to add a new property to treat the active-high case.

In next we should fix it correctly, apply Krzysztof's patch and fix broken
DTBs, right?

-- ynezz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach March 30, 2016, 1:20 p.m. UTC | #7
Am Mittwoch, den 30.03.2016, 15:16 +0200 schrieb Petr Štetiar:
> Fabio Estevam <festevam@gmail.com> [2016-03-30 10:07:16]:
> 
> > If we apply Krzysztof's patch then we break PCI in imx6qdl-sabresd.
> > 
> > In this board we have:
> > 
> > reset-gpio = <&gpio7 12 0>;
> > 
> > , which is wrong. Of course, we should fix this to be 'reset-gpio =
> > <&gpio7 12 GPIO_ACTIVE_LOW>;'
> > 
> > However we should not break old dtb's. That's why reverting the patch
> > is the only solution we have so that old dtb's still work.
> 
> But this only apply for stable right? I understand this and it's fine with me.
> 
> > We have had this same problem for the FEC PHY reset. The solution
> > there was to add a new property to treat the active-high case.
> 
> In next we should fix it correctly, apply Krzysztof's patch and fix broken
> DTBs, right?
> 
We can and should fix broken DTs, but keeping DT stability means we need
to be able to support old (unfixed) DTs with future kernels.

So we can not just fix the DTs and move on, as kernel and DT are two
separate entities, even if they are currently distributed through the
same source tree.

Regards,
Lucas

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam March 30, 2016, 1:25 p.m. UTC | #8
Hi Lucas,

On Wed, Mar 30, 2016 at 10:20 AM, Lucas Stach <l.stach@pengutronix.de> wrote:

> We can and should fix broken DTs, but keeping DT stability means we need
> to be able to support old (unfixed) DTs with future kernels.
>
> So we can not just fix the DTs and move on, as kernel and DT are two
> separate entities, even if they are currently distributed through the
> same source tree.

Correct.

Could you send your Acked-by for this patch then?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach March 30, 2016, 1:29 p.m. UTC | #9
Am Montag, den 28.03.2016, 18:45 -0300 schrieb Fabio Estevam:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Commit 5c5fb40de8f14 ("PCI: imx6: Add support for active-low reset GPIO")
> cause regressions on some boards like MX6 Gateworks Ventana, for example.
> 
> The reason for the breakage is that this commit sets the gpio polarity
> in the wrong logic level.
> 
> Also, the commit log is wrong because active-low reset GPIO is what the
> driver used to support since the beginning.
> 
> So keep the old behavior that ignores the gpio polarity specified in
> the device tree and treat the PCI reset GPIO as active-low.
> 
> Cc: <stable@vger.kernel.org> # 4.5
> Reported-by: Krzysztof Ha?asa <khalasa@piap.pl>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

We need to keep compatibility to existing DTs that are setting the GPIO
active polarity in the wrong way, so this revert is correct:

Acked-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> Changes since v1:
> - Improve wording of commit log (Tim)
> 
>  drivers/pci/host/pci-imx6.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index eb5a275..2f817fa 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -32,7 +32,7 @@
>  #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
>  
>  struct imx6_pcie {
> -	struct gpio_desc	*reset_gpio;
> +	int			reset_gpio;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -309,10 +309,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  	usleep_range(200, 500);
>  
>  	/* Some boards don't have PCIe reset GPIO. */
> -	if (imx6_pcie->reset_gpio) {
> -		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> +	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
>  		msleep(100);
> -		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
>  	}
>  	return 0;
>  
> @@ -523,6 +523,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie;
>  	struct pcie_port *pp;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct resource *dbi_base;
>  	struct device_node *node = pdev->dev.of_node;
>  	int ret;
> @@ -544,8 +545,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> -							GPIOD_OUT_LOW);
> +	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> +					    GPIOF_OUT_INIT_LOW, "PCIe reset");
> +		if (ret) {
> +			dev_err(&pdev->dev, "unable to get reset gpio\n");
> +			return ret;
> +		}
> +	}
>  
>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Hałasa March 31, 2016, 5:07 a.m. UTC | #10
Fabio Estevam <festevam@gmail.com> writes:

> In order to make the Toradex board to work without breaking old dtb's
> is to introduce a property like 'phy-reset-active-high' and handle it
> in the driver.

Well, I don't know. Most DTBs are distributed along the kernel on the
same boot medium. Both ways are far from the ideal. Reverting for now is
ok, but long-term I'd rather fix the buggy DTSs and remove extra stuff.

Old DTBs aren't compatible with new kernels anyway and I guess it will
be the case in the future again (e.g. IIRC pre-4.2 IMX6 DTBs don't boot
on 4.2+ - or was it 4.1?).
Lucas Stach March 31, 2016, 10:35 a.m. UTC | #11
Am Donnerstag, den 31.03.2016, 07:07 +0200 schrieb Krzysztof Ha?asa:
> Fabio Estevam <festevam@gmail.com> writes:
> 
> > In order to make the Toradex board to work without breaking old dtb's
> > is to introduce a property like 'phy-reset-active-high' and handle it
> > in the driver.
> 
> Well, I don't know. Most DTBs are distributed along the kernel on the
> same boot medium. Both ways are far from the ideal. Reverting for now is
> ok, but long-term I'd rather fix the buggy DTSs and remove extra stuff.
> 
> Old DTBs aren't compatible with new kernels anyway and I guess it will
> be the case in the future again (e.g. IIRC pre-4.2 IMX6 DTBs don't boot
> on 4.2+ - or was it 4.1?).

Sorry, but I strongly object to the argument of "DTs are not stable
anyways, so we might just continue to break them". We are trying to keep
them stable.

I think you are referring to the GPC interrupt domain change with your
above statement about DT stability breaking on 4.1/4.2. This is not the
case. While I only catched the boot regression in the -rc phase of this
kernel, the final released kernel will boot just fine with an old DTB.

The i.MX platform tries to keep DTs stable. No exceptions.

Regards,
Lucas

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Hałasa April 1, 2016, 7:26 a.m. UTC | #12
Lucas Stach <l.stach@pengutronix.de> writes:

> I think you are referring to the GPC interrupt domain change with your
> above statement about DT stability breaking on 4.1/4.2. This is not the
> case. While I only catched the boot regression in the -rc phase of this
> kernel, the final released kernel will boot just fine with an old DTB.

It seems it was the other way around:
    BIG FAT WARNING: because the DTs were so far lying by not
    exposing the fact that the GPC block is actually the first
    interrupt controller in the chain, kernels with this patch
    applied wont have any suspend-resume facility when booted
    with old DTs, and old kernels with updated DTs won't even boot.

... perhaps combined with something else. I don't remember.

Nevertheless, the ugly DTS entries are just this - ugly. We should have
a way to remove them eventually.
Fabio Estevam April 5, 2016, 7:10 p.m. UTC | #13
Hi Bjorn,

On Wed, Mar 30, 2016 at 10:29 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Montag, den 28.03.2016, 18:45 -0300 schrieb Fabio Estevam:
>> From: Fabio Estevam <fabio.estevam@nxp.com>
>>
>> Commit 5c5fb40de8f14 ("PCI: imx6: Add support for active-low reset GPIO")
>> cause regressions on some boards like MX6 Gateworks Ventana, for example.
>>
>> The reason for the breakage is that this commit sets the gpio polarity
>> in the wrong logic level.
>>
>> Also, the commit log is wrong because active-low reset GPIO is what the
>> driver used to support since the beginning.
>>
>> So keep the old behavior that ignores the gpio polarity specified in
>> the device tree and treat the PCI reset GPIO as active-low.
>>
>> Cc: <stable@vger.kernel.org> # 4.5
>> Reported-by: Krzysztof Ha?asa <khalasa@piap.pl>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>
> We need to keep compatibility to existing DTs that are setting the GPIO
> active polarity in the wrong way, so this revert is correct:
>
> Acked-by: Lucas Stach <l.stach@pengutronix.de>

Any chance to get this into 4.6-rc3 to fix the regression?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 5, 2016, 9:31 p.m. UTC | #14
Hi Fabio,

On Mon, Mar 28, 2016 at 06:45:36PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Commit 5c5fb40de8f14 ("PCI: imx6: Add support for active-low reset GPIO")
> cause regressions on some boards like MX6 Gateworks Ventana, for example.
> 
> The reason for the breakage is that this commit sets the gpio polarity
> in the wrong logic level.
> 
> Also, the commit log is wrong because active-low reset GPIO is what the
> driver used to support since the beginning.
> 
> So keep the old behavior that ignores the gpio polarity specified in
> the device tree and treat the PCI reset GPIO as active-low.
> 
> Cc: <stable@vger.kernel.org> # 4.5
> Reported-by: Krzysztof Ha?asa <khalasa@piap.pl>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Applied with acks from Tim and Lucas to for-linus for v4.6, thanks!

If I understand correctly, the revert fixes Gateworks Ventana, but
breaks Toradex Apalis Ixora.  It would be really nice to get Petr's
patches to fix that system in at the same time.  Looks like we just
need a minor documentation update there?

Bjorn

> ---
> Changes since v1:
> - Improve wording of commit log (Tim)
> 
>  drivers/pci/host/pci-imx6.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index eb5a275..2f817fa 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -32,7 +32,7 @@
>  #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
>  
>  struct imx6_pcie {
> -	struct gpio_desc	*reset_gpio;
> +	int			reset_gpio;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -309,10 +309,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  	usleep_range(200, 500);
>  
>  	/* Some boards don't have PCIe reset GPIO. */
> -	if (imx6_pcie->reset_gpio) {
> -		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> +	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
>  		msleep(100);
> -		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
>  	}
>  	return 0;
>  
> @@ -523,6 +523,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie;
>  	struct pcie_port *pp;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct resource *dbi_base;
>  	struct device_node *node = pdev->dev.of_node;
>  	int ret;
> @@ -544,8 +545,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> -							GPIOD_OUT_LOW);
> +	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> +					    GPIOF_OUT_INIT_LOW, "PCIe reset");
> +		if (ret) {
> +			dev_err(&pdev->dev, "unable to get reset gpio\n");
> +			return ret;
> +		}
> +	}
>  
>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam April 5, 2016, 10:07 p.m. UTC | #15
On Tue, Apr 5, 2016 at 6:31 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> Applied with acks from Tim and Lucas to for-linus for v4.6, thanks!
>
> If I understand correctly, the revert fixes Gateworks Ventana, but
> breaks Toradex Apalis Ixora.  It would be really nice to get Petr's
> patches to fix that system in at the same time.  Looks like we just
> need a minor documentation update there?

Yes, that's correct.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam April 18, 2016, 2:41 a.m. UTC | #16
Bjorn,

On Tue, Apr 5, 2016 at 6:31 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> Applied with acks from Tim and Lucas to for-linus for v4.6, thanks!

Any chance for this patch getting applied to 4.6-rc5?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index eb5a275..2f817fa 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -32,7 +32,7 @@ 
 #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
 
 struct imx6_pcie {
-	struct gpio_desc	*reset_gpio;
+	int			reset_gpio;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -309,10 +309,10 @@  static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 	usleep_range(200, 500);
 
 	/* Some boards don't have PCIe reset GPIO. */
-	if (imx6_pcie->reset_gpio) {
-		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
 		msleep(100);
-		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
 	}
 	return 0;
 
@@ -523,6 +523,7 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct imx6_pcie *imx6_pcie;
 	struct pcie_port *pp;
+	struct device_node *np = pdev->dev.of_node;
 	struct resource *dbi_base;
 	struct device_node *node = pdev->dev.of_node;
 	int ret;
@@ -544,8 +545,15 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pp->dbi_base);
 
 	/* Fetch GPIOs */
-	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
-							GPIOD_OUT_LOW);
+	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
+					    GPIOF_OUT_INIT_LOW, "PCIe reset");
+		if (ret) {
+			dev_err(&pdev->dev, "unable to get reset gpio\n");
+			return ret;
+		}
+	}
 
 	/* Fetch clocks */
 	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");