[PATCH] PCI: mt7621: increase PERST_DELAY_MS

[PATCH] PCI: mt7621: increase PERST_DELAY_MS

[Sergio Paracuellos]

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.

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

Re: [PATCH] PCI: mt7621: increase PERST_DELAY_MS

[Bjorn Helgaas]

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
> 

Re: [PATCH] PCI: mt7621: increase PERST_DELAY_MS

[Sergio Paracuellos]

Hi Bjorn,

On Sat, Nov 19, 2022 at 7:03 PM Bjorn Helgaas <helgaas@kernel.org> 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@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
> >

Re: [PATCH] PCI: mt7621: increase PERST_DELAY_MS

[Bjorn Helgaas]

On Sun, Nov 20, 2022 at 08:37:50AM +0100, Sergio Paracuellos wrote:
> On Sat, Nov 19, 2022 at 7:03 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 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.

Your English is WAY, WAY better than my Spanish :)

I assume this is more about just making boots more "reliable" than
something like the "Trusted Boot" or "Secure Boot" technologies [1,2]?

Bjorn

[1] https://trustedcomputinggroup.org/resource/trusted-boot/
[2] https://learn.microsoft.com/en-us/windows/security/trusted-boot

Re: [PATCH] PCI: mt7621: increase PERST_DELAY_MS

[Sergio Paracuellos]

On Mon, Nov 21, 2022 at 11:50 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sun, Nov 20, 2022 at 08:37:50AM +0100, Sergio Paracuellos wrote:
> > On Sat, Nov 19, 2022 at 7:03 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > 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.
>
> Your English is WAY, WAY better than my Spanish :)
>
> I assume this is more about just making boots more "reliable" than
> something like the "Trusted Boot" or "Secure Boot" technologies [1,2]?

You are right. Reliable is definitely the accurate word for this :)

Thanks,
     Sergio Paracuellos
>
> Bjorn
>
> [1] https://trustedcomputinggroup.org/resource/trusted-boot/
> [2] https://learn.microsoft.com/en-us/windows/security/trusted-boot