[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 at 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 at denx.de>

Thank,
Stefan