Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

[Pali Rohár]

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/
>