Re: [PATCH] PCI: mt7621: increase PERST_DELAY_MS

[Bjorn Helgaas <helgaas@kernel.org>]

Hi Sergio,

s/increase/Increase/ in subject, to match history.

On Sat, Nov 19, 2022 at 12:08:37PM +0100, Sergio Paracuellos wrote:
> Some devices using this SoC and PCI's like ZBT WE1326 and Netgear R6220 need
> more time to get the PCI ports properly working after reset. Hence, increase
> PERST_DELAY_MS definition used for this purpose from 100 ms to 500 ms to get
> into confiable boots and working PCI for these devices.

confiable?

It looks like we sleep for PERST_DELAY_MS twice during probe:

  mt7621_pcie_probe
    mt7621_pcie_init_ports
      mt7621_pcie_reset_assert
        list_for_each_entry(port, &pcie->ports, list)
          mt7621_control_assert
          mt7621_rst_gpio_pcie_assert
        msleep(PERST_DELAY_MS)                      #1
      mt7621_pcie_reset_rc_deassert
        list_for_each_entry(port, &pcie->ports, list)
          mt7621_control_deassert

      mt7621_pcie_reset_ep_deassert
        list_for_each_entry(port, &pcie->ports, list)
          mt7621_rst_gpio_pcie_deassert
        msleep(PERST_DELAY_MS)                      #2

so this increases the minimum probe time from 200 ms to 1000 ms.  It
looks like these delays have different purposes and might not need to
be the same.

I assume mt7621_pcie_reset_assert() asserts PERST#, and the sleep #1
is enforcing T_PVPERL, i.e., the minimum time between power becoming
stable and PERST# being inactive, which PCIe CEM r2.0, sec 2.6.2, says
is at least 100 ms.  We probably don't know how long it takes for
power to become stable, and the previous PERST_DELAY_MS of 100 ms
didn't include any time for that, so it makes sense to me to increase
it.

But what about sleep #2?  That happens *after* PERST# is deasserted,
so it seems like that must be for a different purpose, and if so,
deserves its own separate #define.  PCIe r6.0, sec 6.6.1 specifies a
minimum 100 ms after exiting Conventional Reset before sending config
requests.  Is that what this delay is for?  If so, maybe it doesn't
need to be increased?  Or maybe not as much?

Bjorn

> Fixes: 475fe234bdfd ("staging: mt7621-pci: change value for 'PERST_DELAY_MS'")
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/pci/controller/pcie-mt7621.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 4bd1abf26008..438889045fa6 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -60,7 +60,7 @@
>  #define PCIE_PORT_LINKUP		BIT(0)
>  #define PCIE_PORT_CNT			3
>  
> -#define PERST_DELAY_MS			100
> +#define PERST_DELAY_MS			500
>  
>  /**
>   * struct mt7621_pcie_port - PCIe port information
> -- 
> 2.25.1
>