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.
> > > +
> > > + /* 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/
> 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/
>
