Am 29. April 2023 11:03:07 MESZ schrieb Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>: >Hi > >Am 29. April 2023 05:15:51 MESZ schrieb "Jianjun Wang (王建军)" <Jianjun.Wang@xxxxxxxxxxxx>: >>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@xxxxxxxxxxxx>: >>> > 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@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@xxxxxxxxx> >>> > > > 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@xxxxxxxxxxxxxxx> >>> > > > --- >>> > > > 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
