On 2023/8/3 1:18, Bjorn Helgaas wrote: > On Tue, Aug 01, 2023 at 03:05:46PM +0800, Kevin Xie wrote: >> On 2023/8/1 7:12, Bjorn Helgaas wrote: >> ... > >> > The delay required by sec 6.6.1 is a minimum of 100ms following exit >> > from reset or, for fast links, 100ms after link training completes. >> > >> > The comment at the call of advk_pcie_wait_for_link() [2] says it is >> > the delay required by sec 6.6.1, but that doesn't seem right to me. >> > >> > For one thing, I don't think 6.6.1 says anything about "link up" being >> > the end of a delay. So if we want to do the delay required by 6.6.1, >> > "wait_for_link()" doesn't seem like quite the right name. >> > >> > For another, all the *_wait_for_link() functions can return success >> > after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, >> > but 90ms is quite possible. If we avoided the 0ms return and >> > LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough >> > for slow links, where we need 100ms following "exit from reset." >> > >> > But it's still not enough for fast links where we need 100ms "after >> > link training completes" because we don't know when training >> > completed. If training completed 89ms into *_wait_for_link(), we only >> > delay 1ms after that. >> >> That's the point, we will add a extra 100ms after PERST# de-assert >> in the patch-v3 according to Base Spec r6.0 - 6.6.1: >> msleep(100); >> gpiod_set_value_cansleep(pcie->reset_gpio, 0); >> >> + /* As the requirement in PCIe base spec r6.0, system must wait a >> + * minimum of 100 ms following exit from a Conventional Reset >> + * before sending a Configuration Request to the device.*/ >> + msleep(100); >> + >> if (starfive_pcie_host_wait_for_link(pcie)) >> return -EIO; > > For fast links (links that support > 5.0 GT/s), the 100ms starts > *after* link training completes. The above looks OK if starfive only > supports slow links, but then I'm not sure why we would need > starfive_pcie_host_wait_for_link(). > Yes, the maximum speed of JH7110 PCIe is 5.0 GT/s (Gen2x1). About starfive_pcie_host_wait_for_link(): JH7110 SoC only has one root port in each PCIe controller (2 in total) and they do not support hot-plug yet. Thus, We add starfive_pcie_host_wait_for_link() to poll if it is a empty slot. If nothing here, we will exit the probe() of this controller, and it will not go into pci_host_probe() too. This may not be a very standard logic, should we remove it or rewrite in a better way? > Bjorn
