Hi Bjorn,
On Sat, Nov 19, 2022 at 7:03 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> Hi Sergio,
>
> s/increase/Increase/ in subject, to match history.
I missed that, sorry.
>
> 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 seems my spanish confused my mind here :). I meant trustable.
>
> 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?
Sure, let me review this part a bit and come back to you with a proper
patch for fixing the issue and taking into account your comments. I
don't have the devices that are having this issue and I need a bit of
testing before submitting anything to be sure the fix is correct.
Thanks a lot for your comments.
Best regards,
Sergio Paracuellos
>
> Bjorn
>
> > Fixes: 475fe234bdfd ("staging: mt7621-pci: change value for 'PERST_DELAY_MS'")
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > ---
> > 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
> >
