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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210318000211.ykjsfavfc7suu2sb@pali>
Date:   Thu, 18 Mar 2021 01:02:11 +0100
From:   Pali Rohár <pali@...nel.org>
To:     Jianjun Wang <jianjun.wang@...iatek.com>
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 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@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/

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