diff mbox series

PCI: mediatek-gen3: handle PERST after reset

Message ID 20230402131347.99268-1-linux@fw-web.de (mailing list archive)
State Changes Requested
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: mediatek-gen3: handle PERST after reset | expand

Commit Message

Frank Wunderlich April 2, 2023, 1:13 p.m. UTC 
From: Frank Wunderlich <frank-w@public-files.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@public-files.de>
---
Patch is taken from user Ruslan aka RRKh61 (permitted me to send it
with me as author).

https://forum.banana-pi.org/t/bpi-r3-nvme-connection-issue/14563/17
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Frank Wunderlich April 26, 2023, 5:41 p.m. UTC | #1 
Hi

> Gesendet: Sonntag, 02. April 2023 um 15:13 Uhr
> Von: "Frank Wunderlich" <linux@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@public-files.de>
> ---
> Patch is taken from user Ruslan aka RRKh61 (permitted me to send it
> with me as author).
>
> https://forum.banana-pi.org/t/bpi-r3-nvme-connection-issue/14563/17
> ---
>  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
Jianjun Wang (王建军) April 27, 2023, 8:31 a.m. UTC | #2 
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?

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@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@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
>
Frank Wunderlich April 28, 2023, 5:50 a.m. UTC | #3 
Am 27. April 2023 10:31:07 MESZ schrieb "Jianjun Wang (王建军)" <Jianjun.Wang@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...

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

# 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@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@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
Bjorn Helgaas April 28, 2023, 8:09 p.m. UTC | #4 
On Sun, Apr 02, 2023 at 03:13:47PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> De-assert PERST in separate step after reset signals to fully comply
> the PCIe CEM clause 2.2.

I guess this refers to PCIe CEM r5.0, sec 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@public-files.de>
> ---
> Patch is taken from user Ruslan aka RRKh61 (permitted me to send it
> with me as author).
> 
> https://forum.banana-pi.org/t/bpi-r3-nvme-connection-issue/14563/17
> ---
>  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);

There should be a #define for the 100ms value since it is required by
the generic PCIe CEM spec, not by anything specific to mediatek.  If
one already exists, we should use it.  If not, we should add one.

pcie-tegra194.c and pcie-mediatek.c (at least) also have similar
delays and should also use the same #define.  There are several other
drivers that contain "msleep(100)", but I didn't look to see their
purpose.

> +	/* 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 */
> -- 
> 2.34.1
> 
>
Jianjun Wang (王建军) April 29, 2023, 3:15 a.m. UTC | #5 
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@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@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@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@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
Frank Wunderlich April 29, 2023, 9:03 a.m. UTC | #6 
Hi

Am 29. April 2023 05:15:51 MESZ schrieb "Jianjun Wang (王建军)" <Jianjun.Wang@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@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@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@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@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
Frank Wunderlich May 1, 2023, 7:51 a.m. UTC | #7 
Am 29. April 2023 11:03:07 MESZ schrieb Frank Wunderlich <frank-w@public-files.de>:
>Hi
>
>Am 29. April 2023 05:15:51 MESZ schrieb "Jianjun Wang (王建军)" <Jianjun.Wang@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@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@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@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@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
diff mbox series

Patch

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