Date:   Tue, 23 Mar 2021 09:31:34 +0800
From:   Jianjun Wang <jianjun.wang@...iatek.com>
To:     Pali Rohár <pali@...nel.org>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>, <maz@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Ryder Lee <ryder.lee@...iatek.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "Matthias Brugger" <matthias.bgg@...il.com>,
        <linux-pci@...r.kernel.org>, <linux-mediatek@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        "Sj Huang" <sj.huang@...iatek.com>, <youlin.pei@...iatek.com>,
        <chuanjia.liu@...iatek.com>, <qizhong.cheng@...iatek.com>,
        <sin_jieyang@...iatek.com>, <drinkcat@...omium.org>,
        <Rex-BC.Chen@...iatek.com>, <anson.chuang@...iatek.com>
Subject: Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

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@google.com/
> > > 
> > > And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100:
> > > 
> > > https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@triplefau.lt/
> > 
> > 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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Powered by blists - more mailing lists