[PATCH] pci: pci_mvebu: Add support for reset-gpios

[PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/pci_mvebu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index d80f87e0cfc6..269c027db204 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -22,6 +22,7 @@
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
+#include <asm-generic/gpio.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -60,6 +61,7 @@ struct mvebu_pcie {
 	struct resource mem;
 	void __iomem *iobase;
 	struct resource io;
+	struct gpio_desc reset_gpio;
 	u32 intregs;
 	u32 port;
 	u32 lane;
@@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	struct udevice *ctlr = pci_get_controller(dev);
 	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
 	u32 reg;
+	int ret;
+
+	/* Request for optional PERST# GPIO */
+	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
+	if (ret && ret != -ENOENT) {
+		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
+		return ret;
+	}
 
 	/*
 	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
@@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
 		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
 
+	/* Release PERST# via GPIO when it was defined */
+	if (dm_gpio_is_valid(&pcie->reset_gpio))
+		dm_gpio_set_value(&pcie->reset_gpio, 0);
+
 	mvebu_pcie_wait_for_link(pcie);
 
 	return 0;
-- 
2.20.1

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 28.07.22 15:03, Pali Rohár wrote:
> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index d80f87e0cfc6..269c027db204 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -22,6 +22,7 @@
>   #include <asm/io.h>
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>
> +#include <asm-generic/gpio.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   #include <linux/errno.h>
> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>   	struct resource mem;
>   	void __iomem *iobase;
>   	struct resource io;
> +	struct gpio_desc reset_gpio;
>   	u32 intregs;
>   	u32 port;
>   	u32 lane;
> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	struct udevice *ctlr = pci_get_controller(dev);
>   	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>   	u32 reg;
> +	int ret;
> +
> +	/* Request for optional PERST# GPIO */
> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> +	if (ret && ret != -ENOENT) {
> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> +		return ret;
> +	}
>   
>   	/*
>   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>   
> +	/* Release PERST# via GPIO when it was defined */
> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> +

So you're only releasing the GPIO (setting to inactive) here. Wouldn't
it make sense to first use the GPIO (if available via DT) to actually
reset the PCI device? How is this done in the Kernel driver(s)?

Thanks,
Stefan

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
> On 28.07.22 15:03, Pali Rohár wrote:
> > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >   drivers/pci/pci_mvebu.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index d80f87e0cfc6..269c027db204 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -22,6 +22,7 @@
> >   #include <asm/io.h>
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> > +#include <asm-generic/gpio.h>
> >   #include <linux/bitops.h>
> >   #include <linux/delay.h>
> >   #include <linux/errno.h>
> > @@ -60,6 +61,7 @@ struct mvebu_pcie {
> >   	struct resource mem;
> >   	void __iomem *iobase;
> >   	struct resource io;
> > +	struct gpio_desc reset_gpio;
> >   	u32 intregs;
> >   	u32 port;
> >   	u32 lane;
> > @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	struct udevice *ctlr = pci_get_controller(dev);
> >   	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> >   	u32 reg;
> > +	int ret;
> > +
> > +	/* Request for optional PERST# GPIO */
> > +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> > +	if (ret && ret != -ENOENT) {
> > +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> > +		return ret;
> > +	}
> >   	/*
> >   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> > @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
> >   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
> > +	/* Release PERST# via GPIO when it was defined */
> > +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> > +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> > +
> 
> So you're only releasing the GPIO (setting to inactive) here. Wouldn't
> it make sense to first use the GPIO (if available via DT) to actually
> reset the PCI device? How is this done in the Kernel driver(s)?
> 
> Thanks,
> Stefan

This is something which I do not know what is the best. Kernel driver
pci-mvebu.c has same logic - just release from reset at startup.

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 28.07.22 17:10, Pali Rohár wrote:
> On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
>> On 28.07.22 15:03, Pali Rohár wrote:
>>> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>> ---
>>>    drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
>>> index d80f87e0cfc6..269c027db204 100644
>>> --- a/drivers/pci/pci_mvebu.c
>>> +++ b/drivers/pci/pci_mvebu.c
>>> @@ -22,6 +22,7 @@
>>>    #include <asm/io.h>
>>>    #include <asm/arch/cpu.h>
>>>    #include <asm/arch/soc.h>
>>> +#include <asm-generic/gpio.h>
>>>    #include <linux/bitops.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/errno.h>
>>> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>>>    	struct resource mem;
>>>    	void __iomem *iobase;
>>>    	struct resource io;
>>> +	struct gpio_desc reset_gpio;
>>>    	u32 intregs;
>>>    	u32 port;
>>>    	u32 lane;
>>> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	struct udevice *ctlr = pci_get_controller(dev);
>>>    	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>>>    	u32 reg;
>>> +	int ret;
>>> +
>>> +	/* Request for optional PERST# GPIO */
>>> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
>>> +	if (ret && ret != -ENOENT) {
>>> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
>>> +		return ret;
>>> +	}
>>>    	/*
>>>    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
>>> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>>>    		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>>> +	/* Release PERST# via GPIO when it was defined */
>>> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
>>> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
>>> +
>>
>> So you're only releasing the GPIO (setting to inactive) here. Wouldn't
>> it make sense to first use the GPIO (if available via DT) to actually
>> reset the PCI device? How is this done in the Kernel driver(s)?
>>
>> Thanks,
>> Stefan
> 
> This is something which I do not know what is the best. Kernel driver
> pci-mvebu.c has same logic - just release from reset at startup.

I see. Could you please check some other PCI Kernel drivers, how they
handle reset-gpios signalling?

Thanks,
Stefan

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

On Thursday 28 July 2022 17:13:12 Stefan Roese wrote:
> On 28.07.22 17:10, Pali Rohár wrote:
> > On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
> > > On 28.07.22 15:03, Pali Rohár wrote:
> > > > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >    drivers/pci/pci_mvebu.c | 14 ++++++++++++++
> > > >    1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > > > index d80f87e0cfc6..269c027db204 100644
> > > > --- a/drivers/pci/pci_mvebu.c
> > > > +++ b/drivers/pci/pci_mvebu.c
> > > > @@ -22,6 +22,7 @@
> > > >    #include <asm/io.h>
> > > >    #include <asm/arch/cpu.h>
> > > >    #include <asm/arch/soc.h>
> > > > +#include <asm-generic/gpio.h>
> > > >    #include <linux/bitops.h>
> > > >    #include <linux/delay.h>
> > > >    #include <linux/errno.h>
> > > > @@ -60,6 +61,7 @@ struct mvebu_pcie {
> > > >    	struct resource mem;
> > > >    	void __iomem *iobase;
> > > >    	struct resource io;
> > > > +	struct gpio_desc reset_gpio;
> > > >    	u32 intregs;
> > > >    	u32 port;
> > > >    	u32 lane;
> > > > @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	struct udevice *ctlr = pci_get_controller(dev);
> > > >    	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> > > >    	u32 reg;
> > > > +	int ret;
> > > > +
> > > > +	/* Request for optional PERST# GPIO */
> > > > +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> > > > +	if (ret && ret != -ENOENT) {
> > > > +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> > > > +		return ret;
> > > > +	}
> > > >    	/*
> > > >    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> > > > @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
> > > >    		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
> > > > +	/* Release PERST# via GPIO when it was defined */
> > > > +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> > > > +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> > > > +
> > > 
> > > So you're only releasing the GPIO (setting to inactive) here. Wouldn't
> > > it make sense to first use the GPIO (if available via DT) to actually
> > > reset the PCI device? How is this done in the Kernel driver(s)?
> > > 
> > > Thanks,
> > > Stefan
> > 
> > This is something which I do not know what is the best. Kernel driver
> > pci-mvebu.c has same logic - just release from reset at startup.
> 
> I see. Could you please check some other PCI Kernel drivers, how they
> handle reset-gpios signalling?
> 
> Thanks,
> Stefan

Every kernel driver is doing it differently. Touching PERST# in
bootloader is for two different purposes (which is board specific):

1) Board is designed in a way that PCIe stays in reset after CPU starts
   and it is up to the SW to release PCIe reset, including reset of
   endpoint cards.

   I think that this is recommended design due to how are PCIe
   requirements for timings and dependences on other stuff (like PCIe
   clock stabilization, power on, etc...).

   This is also what I prefer for designing new boards.

2) Board is designed in a way that endpoint PCIe card is after CPU reset
   immediately released from the reset or maybe that PCIe card is not
   reset at all after CPU reset.

   In this case lot of "broken" cards do not work until they are
   manually reset from software, which requires flipping PERST# pin
   manually (via GPIO).

Bootloader / U-Boot starts as the first SW and depends on specific state
of CPU reset. So it can expects that nothing release PERST# from 1)
before it.

In operating system it is harder as kernel cannot do this assumption.

And to make it even harder, there is important question: How long should
be PERST# signal active to ensure that endpoint card is correctly reset?

Without knowing answer to this question, we do not know and cannot
implement 2) properly.

I was not able to find answer for this question in PCIe specs, so I
have asked it on mailing linux-pci mailing list:
https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
(and it is still without clear answer)

And if we go deeper to the PCIe initialization (as PERST# signal is part
of it) I described some proposal for Linux kernel how it should be
implemented correctly:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

So all this stuff needs to be rewritten, ideally into some common
framework. Until that happens, every driver would continue doing it by
its own -- and differently.

Yes, it is a mess, but I do not know what we can do with it.

Btw, in past I did also investigation of those resets and delays, and
really every kernel driver is doing it differently:
https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Francesco Dolcini]

On Thu, Jul 28, 2022 at 05:30:00PM +0200, Pali Rohár wrote:
> On Thursday 28 July 2022 17:13:12 Stefan Roese wrote:
> > On 28.07.22 17:10, Pali Rohár wrote:
> > > On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
> > > > On 28.07.22 15:03, Pali Rohár wrote:
> > > > > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > > > So you're only releasing the GPIO (setting to inactive) here. Wouldn't
> > > > it make sense to first use the GPIO (if available via DT) to actually
> > > > reset the PCI device? How is this done in the Kernel driver(s)?
> > > 
> > > This is something which I do not know what is the best. Kernel driver
> > > pci-mvebu.c has same logic - just release from reset at startup.
> > 
> > I see. Could you please check some other PCI Kernel drivers, how they
> > handle reset-gpios signalling?
> 
> Every kernel driver is doing it differently. Touching PERST# in
> bootloader is for two different purposes (which is board specific):
> 
> 1) Board is designed in a way that PCIe stays in reset after CPU starts
>    and it is up to the SW to release PCIe reset, including reset of
>    endpoint cards.
> 
>    I think that this is recommended design due to how are PCIe
>    requirements for timings and dependences on other stuff (like PCIe
>    clock stabilization, power on, etc...).
> 
>    This is also what I prefer for designing new boards.
> 
> 2) Board is designed in a way that endpoint PCIe card is after CPU reset
>    immediately released from the reset or maybe that PCIe card is not
>    reset at all after CPU reset.
> 
>    In this case lot of "broken" cards do not work until they are
>    manually reset from software, which requires flipping PERST# pin
>    manually (via GPIO).

Those card that you mention as broken are often just fine, from
the standard point, they are just a pain ...

> 
> Bootloader / U-Boot starts as the first SW and depends on specific state
> of CPU reset. So it can expects that nothing release PERST# from 1)
> before it.

I think in general this last one is not a correct assumption, reset could just
not be asserted at that point because of the hardware design.

I recently had issue because of that (Linux Kernel, not U-Boot), and
this is the fix that was merged:

a6809941c1f1 ("PCI: imx6: Fix PERST# start-up sequence")

(and I also had myself similar issue while working on Intel platform
firmware, e.g. UEFI).

Said that I agree that the topic is way more complicated than it
should be :-/


> 
> In operating system it is harder as kernel cannot do this assumption.
> 
> And to make it even harder, there is important question: How long should
> be PERST# signal active to ensure that endpoint card is correctly reset?
> 
> Without knowing answer to this question, we do not know and cannot
> implement 2) properly.
> 
> I was not able to find answer for this question in PCIe specs, so I
> have asked it on mailing linux-pci mailing list:
> https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> (and it is still without clear answer)
> 
> And if we go deeper to the PCIe initialization (as PERST# signal is part
> of it) I described some proposal for Linux kernel how it should be
> implemented correctly:
> https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> 
> So all this stuff needs to be rewritten, ideally into some common
> framework. Until that happens, every driver would continue doing it by
> its own -- and differently.
> 
> Yes, it is a mess, but I do not know what we can do with it.
> 
> Btw, in past I did also investigation of those resets and delays, and
> really every kernel driver is doing it differently:
> https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> 

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

On Friday 29 July 2022 10:09:52 Francesco Dolcini wrote:
> On Thu, Jul 28, 2022 at 05:30:00PM +0200, Pali Rohár wrote:
> > On Thursday 28 July 2022 17:13:12 Stefan Roese wrote:
> > > On 28.07.22 17:10, Pali Rohár wrote:
> > > > On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
> > > > > On 28.07.22 15:03, Pali Rohár wrote:
> > > > > > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > > > > So you're only releasing the GPIO (setting to inactive) here. Wouldn't
> > > > > it make sense to first use the GPIO (if available via DT) to actually
> > > > > reset the PCI device? How is this done in the Kernel driver(s)?
> > > > 
> > > > This is something which I do not know what is the best. Kernel driver
> > > > pci-mvebu.c has same logic - just release from reset at startup.
> > > 
> > > I see. Could you please check some other PCI Kernel drivers, how they
> > > handle reset-gpios signalling?
> > 
> > Every kernel driver is doing it differently. Touching PERST# in
> > bootloader is for two different purposes (which is board specific):
> > 
> > 1) Board is designed in a way that PCIe stays in reset after CPU starts
> >    and it is up to the SW to release PCIe reset, including reset of
> >    endpoint cards.
> > 
> >    I think that this is recommended design due to how are PCIe
> >    requirements for timings and dependences on other stuff (like PCIe
> >    clock stabilization, power on, etc...).
> > 
> >    This is also what I prefer for designing new boards.
> > 
> > 2) Board is designed in a way that endpoint PCIe card is after CPU reset
> >    immediately released from the reset or maybe that PCIe card is not
> >    reset at all after CPU reset.
> > 
> >    In this case lot of "broken" cards do not work until they are
> >    manually reset from software, which requires flipping PERST# pin
> >    manually (via GPIO).
> 
> Those card that you mention as broken are often just fine, from
> the standard point, they are just a pain ...
> 
> > 
> > Bootloader / U-Boot starts as the first SW and depends on specific state
> > of CPU reset. So it can expects that nothing release PERST# from 1)
> > before it.
> 
> I think in general this last one is not a correct assumption, reset could just
> not be asserted at that point because of the hardware design.

Yes, that is why I marked the last part that applies only for hardware
design from option 1) mentioned above.

> I recently had issue because of that (Linux Kernel, not U-Boot), and
> this is the fix that was merged:
> 
> a6809941c1f1 ("PCI: imx6: Fix PERST# start-up sequence")
> 
> (and I also had myself similar issue while working on Intel platform
> firmware, e.g. UEFI).
> 
> Said that I agree that the topic is way more complicated than it
> should be :-/

Yes, it is :-( I proposed some options for kernel but it is lot of work
for which probably nobody has time...

> > 
> > In operating system it is harder as kernel cannot do this assumption.
> > 
> > And to make it even harder, there is important question: How long should
> > be PERST# signal active to ensure that endpoint card is correctly reset?
> > 
> > Without knowing answer to this question, we do not know and cannot
> > implement 2) properly.
> > 
> > I was not able to find answer for this question in PCIe specs, so I
> > have asked it on mailing linux-pci mailing list:
> > https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > (and it is still without clear answer)
> > 
> > And if we go deeper to the PCIe initialization (as PERST# signal is part
> > of it) I described some proposal for Linux kernel how it should be
> > implemented correctly:
> > https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> > 
> > So all this stuff needs to be rewritten, ideally into some common
> > framework. Until that happens, every driver would continue doing it by
> > its own -- and differently.
> > 
> > Yes, it is a mess, but I do not know what we can do with it.
> > 
> > Btw, in past I did also investigation of those resets and delays, and
> > really every kernel driver is doing it differently:
> > https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> > 
> 

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 28.07.22 17:30, Pali Rohár wrote:
> On Thursday 28 July 2022 17:13:12 Stefan Roese wrote:
>> On 28.07.22 17:10, Pali Rohár wrote:
>>> On Thursday 28 July 2022 17:05:38 Stefan Roese wrote:
>>>> On 28.07.22 15:03, Pali Rohár wrote:
>>>>> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>>>> ---
>>>>>     drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>>>>>     1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
>>>>> index d80f87e0cfc6..269c027db204 100644
>>>>> --- a/drivers/pci/pci_mvebu.c
>>>>> +++ b/drivers/pci/pci_mvebu.c
>>>>> @@ -22,6 +22,7 @@
>>>>>     #include <asm/io.h>
>>>>>     #include <asm/arch/cpu.h>
>>>>>     #include <asm/arch/soc.h>
>>>>> +#include <asm-generic/gpio.h>
>>>>>     #include <linux/bitops.h>
>>>>>     #include <linux/delay.h>
>>>>>     #include <linux/errno.h>
>>>>> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>>>>>     	struct resource mem;
>>>>>     	void __iomem *iobase;
>>>>>     	struct resource io;
>>>>> +	struct gpio_desc reset_gpio;
>>>>>     	u32 intregs;
>>>>>     	u32 port;
>>>>>     	u32 lane;
>>>>> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>>>     	struct udevice *ctlr = pci_get_controller(dev);
>>>>>     	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>>>>>     	u32 reg;
>>>>> +	int ret;
>>>>> +
>>>>> +	/* Request for optional PERST# GPIO */
>>>>> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
>>>>> +	if (ret && ret != -ENOENT) {
>>>>> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
>>>>> +		return ret;
>>>>> +	}
>>>>>     	/*
>>>>>     	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
>>>>> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>>>     	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>>>>>     		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>>>>> +	/* Release PERST# via GPIO when it was defined */
>>>>> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
>>>>> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
>>>>> +
>>>>
>>>> So you're only releasing the GPIO (setting to inactive) here. Wouldn't
>>>> it make sense to first use the GPIO (if available via DT) to actually
>>>> reset the PCI device? How is this done in the Kernel driver(s)?
>>>>
>>>> Thanks,
>>>> Stefan
>>>
>>> This is something which I do not know what is the best. Kernel driver
>>> pci-mvebu.c has same logic - just release from reset at startup.
>>
>> I see. Could you please check some other PCI Kernel drivers, how they
>> handle reset-gpios signalling?
>>
>> Thanks,
>> Stefan
> 
> Every kernel driver is doing it differently. Touching PERST# in
> bootloader is for two different purposes (which is board specific):
> 
> 1) Board is designed in a way that PCIe stays in reset after CPU starts
>     and it is up to the SW to release PCIe reset, including reset of
>     endpoint cards.
> 
>     I think that this is recommended design due to how are PCIe
>     requirements for timings and dependences on other stuff (like PCIe
>     clock stabilization, power on, etc...).
> 
>     This is also what I prefer for designing new boards.
> 
> 2) Board is designed in a way that endpoint PCIe card is after CPU reset
>     immediately released from the reset or maybe that PCIe card is not
>     reset at all after CPU reset.
> 
>     In this case lot of "broken" cards do not work until they are
>     manually reset from software, which requires flipping PERST# pin
>     manually (via GPIO).
> 
> Bootloader / U-Boot starts as the first SW and depends on specific state
> of CPU reset. So it can expects that nothing release PERST# from 1)
> before it.
> 
> In operating system it is harder as kernel cannot do this assumption.
> 
> And to make it even harder, there is important question: How long should
> be PERST# signal active to ensure that endpoint card is correctly reset?
> 
> Without knowing answer to this question, we do not know and cannot
> implement 2) properly.
> 
> I was not able to find answer for this question in PCIe specs, so I
> have asked it on mailing linux-pci mailing list:
> https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> (and it is still without clear answer)
> 
> And if we go deeper to the PCIe initialization (as PERST# signal is part
> of it) I described some proposal for Linux kernel how it should be
> implemented correctly:
> https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> 
> So all this stuff needs to be rewritten, ideally into some common
> framework. Until that happens, every driver would continue doing it by
> its own -- and differently.
> 
> Yes, it is a mess, but I do not know what we can do with it.
> 
> Btw, in past I did also investigation of those resets and delays, and
> really every kernel driver is doing it differently:
> https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/

Pali, thanks for the detailed explanation.

With this in mind, I think it's best right now to accept your patch
as-is, as this does minimal change (risk of breakage) in this GPIO
area.

Reviewed-by: Stefan Roese <sr@denx.de>

Thank,
Stefan

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 28.07.22 15:03, Pali Rohár wrote:
> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index d80f87e0cfc6..269c027db204 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -22,6 +22,7 @@
>   #include <asm/io.h>
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>
> +#include <asm-generic/gpio.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   #include <linux/errno.h>
> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>   	struct resource mem;
>   	void __iomem *iobase;
>   	struct resource io;
> +	struct gpio_desc reset_gpio;
>   	u32 intregs;
>   	u32 port;
>   	u32 lane;
> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	struct udevice *ctlr = pci_get_controller(dev);
>   	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>   	u32 reg;
> +	int ret;
> +
> +	/* Request for optional PERST# GPIO */
> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> +	if (ret && ret != -ENOENT) {
> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> +		return ret;
> +	}
>   
>   	/*
>   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>   
> +	/* Release PERST# via GPIO when it was defined */
> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> +
>   	mvebu_pcie_wait_for_link(pcie);
>   
>   	return 0;

While running some build tests, this error shows on some Kirkwood
boards:

$ make pogo_v4_defconfig
$ make -s -j20
===================== WARNING ======================
This board does not use CONFIG_TIMER (Driver Model
for Timer drivers). Please update the board to use
CONFIG_TIMER before the v2023.01 release. Failure to
update by the deadline may result in board removal.
See doc/develop/driver-model/migration.rst for more info.
====================================================
===================== WARNING ======================
This board does not use CONFIG_DM_SERIAL (Driver Model
for Serial drivers). Please update the board to use
CONFIG_DM_SERIAL before the v2023.04 release. Failure to
update by the deadline may result in board removal.
See doc/develop/driver-model/migration.rst for more info.
====================================================
/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd: 
drivers/pci/pci_mvebu.o: in function `mvebu_pcie_probe':
/home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:424: 
undefined reference to `gpio_request_by_name'
/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd: 
/home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:552: 
undefined reference to `dm_gpio_set_value'
make: *** [Makefile:1818: u-boot] Error 1

Could you please take a look and make sure, that world build succeeds?

Thanks,
Stefan

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

On Friday 05 August 2022 13:01:27 Stefan Roese wrote:
> On 28.07.22 15:03, Pali Rohár wrote:
> > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >   drivers/pci/pci_mvebu.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index d80f87e0cfc6..269c027db204 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -22,6 +22,7 @@
> >   #include <asm/io.h>
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> > +#include <asm-generic/gpio.h>
> >   #include <linux/bitops.h>
> >   #include <linux/delay.h>
> >   #include <linux/errno.h>
> > @@ -60,6 +61,7 @@ struct mvebu_pcie {
> >   	struct resource mem;
> >   	void __iomem *iobase;
> >   	struct resource io;
> > +	struct gpio_desc reset_gpio;
> >   	u32 intregs;
> >   	u32 port;
> >   	u32 lane;
> > @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	struct udevice *ctlr = pci_get_controller(dev);
> >   	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> >   	u32 reg;
> > +	int ret;
> > +
> > +	/* Request for optional PERST# GPIO */
> > +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> > +	if (ret && ret != -ENOENT) {
> > +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> > +		return ret;
> > +	}
> >   	/*
> >   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> > @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
> >   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
> > +	/* Release PERST# via GPIO when it was defined */
> > +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> > +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> > +
> >   	mvebu_pcie_wait_for_link(pcie);
> >   	return 0;
> 
> While running some build tests, this error shows on some Kirkwood
> boards:
> 
> $ make pogo_v4_defconfig
> $ make -s -j20
> ===================== WARNING ======================
> This board does not use CONFIG_TIMER (Driver Model
> for Timer drivers). Please update the board to use
> CONFIG_TIMER before the v2023.01 release. Failure to
> update by the deadline may result in board removal.
> See doc/develop/driver-model/migration.rst for more info.
> ====================================================
> ===================== WARNING ======================
> This board does not use CONFIG_DM_SERIAL (Driver Model
> for Serial drivers). Please update the board to use
> CONFIG_DM_SERIAL before the v2023.04 release. Failure to
> update by the deadline may result in board removal.
> See doc/develop/driver-model/migration.rst for more info.
> ====================================================
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> drivers/pci/pci_mvebu.o: in function `mvebu_pcie_probe':
> /home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:424:
> undefined reference to `gpio_request_by_name'
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> /home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:552:
> undefined reference to `dm_gpio_set_value'
> make: *** [Makefile:1818: u-boot] Error 1
> 
> Could you please take a look and make sure, that world build succeeds?
> 
> Thanks,
> Stefan

There is probably missing DM_GPIO. Now I started CI test on github:
https://github.com/u-boot/u-boot/pull/204
After that I will send v2.

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 05.08.22 13:45, Pali Rohár wrote:
> On Friday 05 August 2022 13:01:27 Stefan Roese wrote:
>> On 28.07.22 15:03, Pali Rohár wrote:
>>> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>> ---
>>>    drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
>>> index d80f87e0cfc6..269c027db204 100644
>>> --- a/drivers/pci/pci_mvebu.c
>>> +++ b/drivers/pci/pci_mvebu.c
>>> @@ -22,6 +22,7 @@
>>>    #include <asm/io.h>
>>>    #include <asm/arch/cpu.h>
>>>    #include <asm/arch/soc.h>
>>> +#include <asm-generic/gpio.h>
>>>    #include <linux/bitops.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/errno.h>
>>> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>>>    	struct resource mem;
>>>    	void __iomem *iobase;
>>>    	struct resource io;
>>> +	struct gpio_desc reset_gpio;
>>>    	u32 intregs;
>>>    	u32 port;
>>>    	u32 lane;
>>> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	struct udevice *ctlr = pci_get_controller(dev);
>>>    	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>>>    	u32 reg;
>>> +	int ret;
>>> +
>>> +	/* Request for optional PERST# GPIO */
>>> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
>>> +	if (ret && ret != -ENOENT) {
>>> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
>>> +		return ret;
>>> +	}
>>>    	/*
>>>    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
>>> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>>>    		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>>> +	/* Release PERST# via GPIO when it was defined */
>>> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
>>> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
>>> +
>>>    	mvebu_pcie_wait_for_link(pcie);
>>>    	return 0;
>>
>> While running some build tests, this error shows on some Kirkwood
>> boards:
>>
>> $ make pogo_v4_defconfig
>> $ make -s -j20
>> ===================== WARNING ======================
>> This board does not use CONFIG_TIMER (Driver Model
>> for Timer drivers). Please update the board to use
>> CONFIG_TIMER before the v2023.01 release. Failure to
>> update by the deadline may result in board removal.
>> See doc/develop/driver-model/migration.rst for more info.
>> ====================================================
>> ===================== WARNING ======================
>> This board does not use CONFIG_DM_SERIAL (Driver Model
>> for Serial drivers). Please update the board to use
>> CONFIG_DM_SERIAL before the v2023.04 release. Failure to
>> update by the deadline may result in board removal.
>> See doc/develop/driver-model/migration.rst for more info.
>> ====================================================
>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
>> drivers/pci/pci_mvebu.o: in function `mvebu_pcie_probe':
>> /home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:424:
>> undefined reference to `gpio_request_by_name'
>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
>> /home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:552:
>> undefined reference to `dm_gpio_set_value'
>> make: *** [Makefile:1818: u-boot] Error 1
>>
>> Could you please take a look and make sure, that world build succeeds?
>>
>> Thanks,
>> Stefan
> 
> There is probably missing DM_GPIO. Now I started CI test on github:
> https://github.com/u-boot/u-boot/pull/204
> After that I will send v2.

Perfect. Thanks.

Thanks,
Stefan

Re: [PATCH] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

On Friday 05 August 2022 13:48:36 Stefan Roese wrote:
> On 05.08.22 13:45, Pali Rohár wrote:
> > On Friday 05 August 2022 13:01:27 Stefan Roese wrote:
> > > On 28.07.22 15:03, Pali Rohár wrote:
> > > > Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >    drivers/pci/pci_mvebu.c | 14 ++++++++++++++
> > > >    1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > > > index d80f87e0cfc6..269c027db204 100644
> > > > --- a/drivers/pci/pci_mvebu.c
> > > > +++ b/drivers/pci/pci_mvebu.c
> > > > @@ -22,6 +22,7 @@
> > > >    #include <asm/io.h>
> > > >    #include <asm/arch/cpu.h>
> > > >    #include <asm/arch/soc.h>
> > > > +#include <asm-generic/gpio.h>
> > > >    #include <linux/bitops.h>
> > > >    #include <linux/delay.h>
> > > >    #include <linux/errno.h>
> > > > @@ -60,6 +61,7 @@ struct mvebu_pcie {
> > > >    	struct resource mem;
> > > >    	void __iomem *iobase;
> > > >    	struct resource io;
> > > > +	struct gpio_desc reset_gpio;
> > > >    	u32 intregs;
> > > >    	u32 port;
> > > >    	u32 lane;
> > > > @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	struct udevice *ctlr = pci_get_controller(dev);
> > > >    	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> > > >    	u32 reg;
> > > > +	int ret;
> > > > +
> > > > +	/* Request for optional PERST# GPIO */
> > > > +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> > > > +	if (ret && ret != -ENOENT) {
> > > > +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> > > > +		return ret;
> > > > +	}
> > > >    	/*
> > > >    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> > > > @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
> > > >    		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
> > > > +	/* Release PERST# via GPIO when it was defined */
> > > > +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> > > > +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> > > > +
> > > >    	mvebu_pcie_wait_for_link(pcie);
> > > >    	return 0;
> > > 
> > > While running some build tests, this error shows on some Kirkwood
> > > boards:
> > > 
> > > $ make pogo_v4_defconfig
> > > $ make -s -j20
> > > ===================== WARNING ======================
> > > This board does not use CONFIG_TIMER (Driver Model
> > > for Timer drivers). Please update the board to use
> > > CONFIG_TIMER before the v2023.01 release. Failure to
> > > update by the deadline may result in board removal.
> > > See doc/develop/driver-model/migration.rst for more info.
> > > ====================================================
> > > ===================== WARNING ======================
> > > This board does not use CONFIG_DM_SERIAL (Driver Model
> > > for Serial drivers). Please update the board to use
> > > CONFIG_DM_SERIAL before the v2023.04 release. Failure to
> > > update by the deadline may result in board removal.
> > > See doc/develop/driver-model/migration.rst for more info.
> > > ====================================================
> > > /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> > > drivers/pci/pci_mvebu.o: in function `mvebu_pcie_probe':
> > > /home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:424:
> > > undefined reference to `gpio_request_by_name'
> > > /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> > > /home/stefan/git/u-boot/u-boot-marvell/drivers/pci/pci_mvebu.c:552:
> > > undefined reference to `dm_gpio_set_value'
> > > make: *** [Makefile:1818: u-boot] Error 1
> > > 
> > > Could you please take a look and make sure, that world build succeeds?
> > > 
> > > Thanks,
> > > Stefan
> > 
> > There is probably missing DM_GPIO. Now I started CI test on github:
> > https://github.com/u-boot/u-boot/pull/204
> > After that I will send v2.
> 
> Perfect. Thanks.
> 
> Thanks,
> Stefan

CI tests passed, now sending v2.

[PATCH v2] pci: pci_mvebu: Add support for reset-gpios

[Pali Rohár]

Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Stefan Roese <sr@denx.de>
---
Changes in v2:
* select DM_GPIO
* use asm/gpio.h
---
 drivers/pci/Kconfig     |  1 +
 drivers/pci/pci_mvebu.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 436acca898e4..22f4995453ed 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -301,6 +301,7 @@ config PCI_MVEBU
 	depends on (ARCH_KIRKWOOD || ARCH_MVEBU)
 	select MISC
 	select DM_RESET
+	select DM_GPIO
 	help
 	  Say Y here if you want to enable PCIe controller support on
 	  Kirkwood and Armada 370/XP/375/38x SoCs.
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index d80f87e0cfc6..5bd340a421b8 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -22,6 +22,7 @@
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
+#include <asm/gpio.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -60,6 +61,7 @@ struct mvebu_pcie {
 	struct resource mem;
 	void __iomem *iobase;
 	struct resource io;
+	struct gpio_desc reset_gpio;
 	u32 intregs;
 	u32 port;
 	u32 lane;
@@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	struct udevice *ctlr = pci_get_controller(dev);
 	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
 	u32 reg;
+	int ret;
+
+	/* Request for optional PERST# GPIO */
+	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
+	if (ret && ret != -ENOENT) {
+		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
+		return ret;
+	}
 
 	/*
 	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
@@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
 		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
 
+	/* Release PERST# via GPIO when it was defined */
+	if (dm_gpio_is_valid(&pcie->reset_gpio))
+		dm_gpio_set_value(&pcie->reset_gpio, 0);
+
 	mvebu_pcie_wait_for_link(pcie);
 
 	return 0;
-- 
2.20.1

Re: [PATCH v2] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 05.08.22 16:03, Pali Rohár wrote:
> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>
> ---
> Changes in v2:
> * select DM_GPIO
> * use asm/gpio.h

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/Kconfig     |  1 +
>   drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 436acca898e4..22f4995453ed 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -301,6 +301,7 @@ config PCI_MVEBU
>   	depends on (ARCH_KIRKWOOD || ARCH_MVEBU)
>   	select MISC
>   	select DM_RESET
> +	select DM_GPIO
>   	help
>   	  Say Y here if you want to enable PCIe controller support on
>   	  Kirkwood and Armada 370/XP/375/38x SoCs.
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index d80f87e0cfc6..5bd340a421b8 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -22,6 +22,7 @@
>   #include <asm/io.h>
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>
> +#include <asm/gpio.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   #include <linux/errno.h>
> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>   	struct resource mem;
>   	void __iomem *iobase;
>   	struct resource io;
> +	struct gpio_desc reset_gpio;
>   	u32 intregs;
>   	u32 port;
>   	u32 lane;
> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	struct udevice *ctlr = pci_get_controller(dev);
>   	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>   	u32 reg;
> +	int ret;
> +
> +	/* Request for optional PERST# GPIO */
> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> +	if (ret && ret != -ENOENT) {
> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> +		return ret;
> +	}
>   
>   	/*
>   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>   
> +	/* Release PERST# via GPIO when it was defined */
> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> +
>   	mvebu_pcie_wait_for_link(pcie);
>   
>   	return 0;

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH v2] pci: pci_mvebu: Add support for reset-gpios

[Stefan Roese]

On 05.08.22 16:03, Pali Rohár wrote:
> Release PERST# signal via GPIO when "reset-gpios" is defined in device tree.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
> Changes in v2:
> * select DM_GPIO
> * use asm/gpio.h
> ---
>   drivers/pci/Kconfig     |  1 +
>   drivers/pci/pci_mvebu.c | 14 ++++++++++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 436acca898e4..22f4995453ed 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -301,6 +301,7 @@ config PCI_MVEBU
>   	depends on (ARCH_KIRKWOOD || ARCH_MVEBU)
>   	select MISC
>   	select DM_RESET
> +	select DM_GPIO
>   	help
>   	  Say Y here if you want to enable PCIe controller support on
>   	  Kirkwood and Armada 370/XP/375/38x SoCs.
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index d80f87e0cfc6..5bd340a421b8 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -22,6 +22,7 @@
>   #include <asm/io.h>
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>
> +#include <asm/gpio.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   #include <linux/errno.h>
> @@ -60,6 +61,7 @@ struct mvebu_pcie {
>   	struct resource mem;
>   	void __iomem *iobase;
>   	struct resource io;
> +	struct gpio_desc reset_gpio;
>   	u32 intregs;
>   	u32 port;
>   	u32 lane;
> @@ -416,6 +418,14 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	struct udevice *ctlr = pci_get_controller(dev);
>   	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>   	u32 reg;
> +	int ret;
> +
> +	/* Request for optional PERST# GPIO */
> +	ret = gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio, GPIOD_IS_OUT);
> +	if (ret && ret != -ENOENT) {
> +		printf("%s: unable to request reset-gpios: %d\n", pcie->name, ret);
> +		return ret;
> +	}
>   
>   	/*
>   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> @@ -537,6 +547,10 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>   
> +	/* Release PERST# via GPIO when it was defined */
> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
> +
>   	mvebu_pcie_wait_for_link(pcie);
>   
>   	return 0;

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de