On Fri, 2021-03-19 at 19:53 +0100, Pali Rohár wrote: > On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote: > > On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote: > > > On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote: > > > > On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote: > > > > > On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote: > > > > > > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port) > > > > > > +{ > > > > > ... > > > > > > + > > > > > > + /* Delay 100ms to wait the reference clocks become stable */ > > > > > > + msleep(100); > > > > > > + > > > > > > + /* De-assert PERST# signal */ > > > > > > + val &= ~PCIE_PE_RSTB; > > > > > > + writel_relaxed(val, port->base + PCIE_RST_CTRL_REG); > > > > > > > > > > Hello! This is a new driver which introduce yet another custom timeout > > > > > prior PERST# signal for PCIe card is de-asserted. Timeouts for other > > > > > drivers I collected in older email [2]. > > > > > > > > > > Please look at my email [1] about PCIe Warm Reset if you have any clue > > > > > about it. Lorenzo and Rob already expressed that this timeout should not > > > > > be driver specific. But nobody was able to "decode" and "understand" > > > > > PCIe spec yet about these timeouts. > > > > > > > > Hi Pali, > > > > > > > > I think this is more like a platform specific timeout, which is used to > > > > wait for the reference clocks to become stable and finish the reset flow > > > > of HW blocks. > > > > > > > > Here is the steps to start a link training in this HW: > > > > > > > > 1. Assert all reset signals which including the transaction layer, PIPE > > > > interface and internal bus interface; > > > > > > > > 2. De-assert reset signals except the PERST#, this will make the > > > > physical layer active and start to output the reference clock, but the > > > > EP device remains in the reset state. > > > > Before releasing the PERST# signal, the HW blocks needs at least 10ms > > > > to finish the reset flow, and ref-clk needs about 30us to become stable. > > > > > > > > 3. De-assert PERST# signal, wait LTSSM enter L0 state. > > > > > > > > This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since > > > > we are in the kernel stage, the power supply has already stabled, this > > > > timeout may not take that long. > > > > > > I think that this is not platform specific timeout or platform specific > > > steps. This matches generic steps as defined in PCIe CEM spec, section > > > 2.2.1. Initial Power-Up (G3 to S0). > > > > > > What is platform specific is just how to achieve these steps. > > > > > > Am I right? > > > > > > ... > > > > > > TPVPERL is one of my timeout candidates as minimal required timeout for > > > Warm Reset. I have wrote it in email: > > > > > > https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/ > > > > > > But I'm not sure as specially in none diagram is described just warm > > > reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal). > > > > > > ... > > > > > > Anyway, I would suggest to define constants for those timeouts. I guess > > > that in future we could be able to define "generic" timeout constants > > > which would not be in private driver section, but in some common header > > > file. > > > > I agree with this, but I'm not sure if we really need that long time in > > the kernel stage, because the power supply has already stable and it's > > really impact the boot time, especially when the platform have multi > > ports and not connect any EP device, we need to wait 200ms for each port > > when system bootup. > > Ports are independent. So you can initialize them in parallel, right? > > If you initialize each port in separate worker then during msleep calls > kernel can schedule other kernel thread to run and so it does not > increase boot time. While pcie is sleeping kernel can do other things. > So the result is that whole boot time is not increased, just reordered. > > > For this PCIe controller driver, I would like to change the timeout > > value to 10ms to comply with the HW design, and save some boot time. > > In case you can connect _any_ PCIe card to your HW then you cannot > decrease or change timeouts required by PCIe specs. Otherwise there can > be a card which would not be initialized correctly. > > I'm debugging driver for aardvark PCIe controller and I see that Compex > cards really needs these timeouts, otherwise link is down and card > cannot be detected. > > So I guess that there can be also other cards which requires other > timeouts as specified in PCIe specs. OK, I'll keep this timeout value. One more question, is there any chance that we can put this linkup flow to a more "standard" way, such as drivers provides the ops of the PERST# pin and let the framework to decide how to start a link training, or we just use macro to replace this timeout value in the future? Thanks. > > > > > > > > > > + > > > > > > + /* Check if the link is up or not */ > > > > > > + err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val, > > > > > > + !!(val & PCIE_PORT_LINKUP), 20, > > > > > > + 50 * USEC_PER_MSEC); > > > > > > > > > > IIRC, you need to wait at least 100ms after de-asserting PERST# signal > > > > > as it is required by PCIe specs and also because experiments proved that > > > > > some Compex wifi cards (e.g. WLE900VX) are not detected if you do not > > > > > wait this minimal time. > > > > > > > > Yes, this should be 100ms, I will fix it at next version, thanks for > > > > your review. > > > > > > In past Bjorn suggested to use msleep(PCI_PM_D3COLD_WAIT); macro for > > > this step during reviewing aardvark driver. > > > > > > https://lore.kernel.org/linux-pci/20190426161050.GA189964@xxxxxxxxxx/ > > > > > > And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100: > > > > > > https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@xxxxxxxxxxxx/ > > > > Sure, I will use PCI_PM_D3COLD_WAIT macro instead in the next version. > > > > Thanks. > > > > > > > > > Thanks. > > > > > > > > > > > + if (err) { > > > > > > + val = readl_relaxed(port->base + PCIE_LTSSM_STATUS_REG); > > > > > > + dev_err(port->dev, "PCIe link down, ltssm reg val: %#x\n", val); > > > > > > + return err; > > > > > > + } > > > > > > > > > > [1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/ > > > > > [2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/ > > > > > > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-mediatek