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

Mon May 1 00:51:06 PDT 2023

Am 29. April 2023 11:03:07 MESZ schrieb Frank Wunderlich <frank-w at public-files.de>:
>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.

Seems this does not make it work...

Here are the bootlogs with different versions of this patch:

https://forum.banana-pi.org/t/bpi-r3-problem-with-pcie/15152/22

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