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

Sat Apr 29 02:03:07 PDT 2023

Hi

Am 29. April 2023 05:15:51 MESZ schrieb "Jianjun Wang (王建军)" <Jianjun.Wang at mediatek.com>:
>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.

Thanks for taking time for analysing it.

>> 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 see there is the msleep before the reset,where the patch splits reset and perst (and add the sleep between).

So imho the best way is to move the existing msleep between these "splitted" signals instead of adding a second one. I have to verify it with someone who have the issue currently.

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

I gave the questions to board vendor in forum.

>[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);
Maybe the better way is to drop this msleep when adding the new one below the reset asserts?

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

regards Frank