Aw: [PATCH] PCI: mediatek-gen3: handle PERST after reset

[Jianjun Wang (王建军)]

Hi Frank,

On Fri, 2023-04-28 at 07:50 +0200, Frank Wunderlich wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Am 27. April 2023 10:31:07 MESZ schrieb "Jianjun Wang (王建军)" <
> Jianjun.Wang at mediatek.com>:
> > Hi Frank,
> > 
> > Seems this patch has huge impact on boot time, I'm curious about
> > the
> > NVMe detection issue on mt7986, can you share the SSD model that
> > you
> > are using and the bootup logs with that SSD?
> 
> Which "huge" delay do you get in which setup? It adds a 100ms delay
> yes,but this seems needed to some devices working.i found several
> sources talking about the 100ms wake-up time...
Some products are very sensitive to the bootup time, especially the
platforms with many PCIe ports, adding this 100ms delay for each port
will cause the bootup time be increased by multiple times(depending on
the number of PCIe ports it uses), and also the wake-up time, since the
mtk_pcie_starup_port() function will be called on resume stage.
> 
> I do not have this issue,but some users in bpi-forum reorted it.
> Pcie-controller on mt7986/bpi-r3 does simply not detect the nvme and
> returned ETIMEDOUT (110).
Since we're already comply with the PCIe CEM specification sections
2.2(PERST# signal)[1], and this issue only occurs on a few platforms,
I'll inclined to it's might be a signal quality issue. Also I checked
the BPI-R3 schematic diagram[2], and noticed that there are no AC
Coupling capacitors on the transmitter side, which described in PCIe
CEM Spec sections 4.7.1, but this schematic diagram only have part of
it, can you help to check the board design or share the full schematic
diagram for further analysis? 

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-mediatek-gen3.c?h=v6.3#n344
[2]:
https://drive.google.com/file/d/1mxKb8CBbnzfNSd_4esmcX_NovxaXjEb8/view

Thanks.
> 
> # dmesg | grep 'pci'
> [ 5.235564] mtk-pcie-gen3 11280000.pcie: host bridge 
> /soc/pcie at 11280000 ranges:
> [ 5.242938] mtk-pcie-gen3 11280000.pcie: Parsing ranges property...
> [ 5.249235] mtk-pcie-gen3 11280000.pcie: MEM
> 0x0020000000..0x002fffffff -> 0x0020000000
> [ 5.478062] mtk-pcie-gen3 11280000.pcie: PCIe link down, current
> LTSSM state: detect.active (0x10 00001)
> [ 5.487491] mtk-pcie-gen3: probe of 11280000.pcie failed with error
> -110
> 
> One specific hardware is reported as example:
> 
> Adata Legend710 512GB x3
> 
> > Thanks.
> > 
> > On Wed, 2023-04-26 at 19:41 +0200, Frank Wunderlich wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > Hi
> > > 
> > > > Gesendet: Sonntag, 02. April 2023 um 15:13 Uhr
> > > > Von: "Frank Wunderlich" <linux at fw-web.de>
> > > > De-assert PERST in separate step after reset signals to fully
> > > > comply
> > > > the PCIe CEM clause 2.2.
> > > > 
> > > > This fixes some NVME detection issues on mt7986.
> > > > 
> > > > Fixes: d3bf75b579b9 ("PCI: mediatek-gen3: Add MediaTek Gen3
> > > > driver
> > > > for MT8192")
> > > > Signed-off-by: Frank Wunderlich <frank-w at public-files.de>
> > > > ---
> > > > Patch is taken from user Ruslan aka RRKh61 (permitted me to
> > > > send it
> > > > with me as author).
> > > > 
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://forum.banana-pi.org/t/bpi-r3-nvme-connection-issue/14563/17__;!!CTRNKA9wMg0ARbw!nCXEM685pkUpoiZYGKptPYccNrWMeN2D3jIO5_irwxZJ7c6ZzEeACIx-V2WeZHAP_0FKlDDIQ0RbDJ892prtoToDv30$
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > index b8612ce5f4d0..176b1a04565d 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > @@ -350,7 +350,13 @@ static int mtk_pcie_startup_port(struct
> > > > mtk_gen3_pcie *pcie)
> > > >       msleep(100);
> > > > 
> > > >       /* De-assert reset signals */
> > > > -     val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > > > PCIE_PE_RSTB);
> > > > +     val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);
> > > > +     writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > > +
> > > > +     msleep(100);
> > > > +
> > > > +     /* De-assert PERST# signals */
> > > > +     val &= ~(PCIE_PE_RSTB);
> > > >       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > > 
> > > >       /* Check if the link is up or not */
> > > 
> > > Hi
> > > 
> > > just a friendly reminder....do i miss any recipient?
> > > 
> > > regards Frank
> > > 
> 
> 
> regards Frank