| Message ID | 20221119110837.2419466-1-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: mt7621: increase PERST_DELAY_MS | expand |
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 >
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 > >
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
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
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
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(-)