[v2,5/7] PCI: aardvark: disable LOS state by default

From: Victor Gu <xigu@marvell.com>

Some PCIe devices do not support LOS, and will cause timeouts if the
root complex forces the LOS state. This patch disables the LOS state
by default.

This is part of fixing bug
https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
reported as the user to be important to get a Intel 7260 mini-PCIe
WiFi card working.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

I assume you mean "ASPM L0s"?

On Thu, Sep 28, 2017 at 02:58:36PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> Some PCIe devices do not support LOS, and will cause timeouts if the
> root complex forces the LOS state. This patch disables the LOS state
> by default.

Per PCIe r3.1, sec 5.4.1.3, software should not enable L0s in either
direction unless both ends support L0s.

I'm unclear on what the bug is here.  Is the generic ASPM code
incorrectly enabling L0s when one end doesn't support it?  That seems
unlikely, but if so, it should be fixed in the generic ASPM code.

Are both ends advertising L0s support, but there's a hardware erratum
on the Aardvark end that keeps it from working correctly?  If so, we
should say that explicitly and include a reference to a published
hardware erratum.

Something else?

> This is part of fixing bug
> https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
> reported as the user to be important to get a Intel 7260 mini-PCIe
> WiFi card working.

The bugzilla link is a good start, but "reported by the user to be
important" is meaningless by itself.  What we need here is the details
of what's broken and how this fixes it.  Unfortunately the bugzilla
doesn't have those details.

> Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Victor Gu <xigu@marvell.com>
> Reviewed-by: Evan Wang <xswang@marvell.com>
> Reviewed-by: Nadav Haklai <nadavh@marvell.com>
> [Thomas: tweak commit log.]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-aardvark.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index 23e243135ebe..a173f31853df 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -368,8 +368,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  
>  	advk_pcie_wait_for_link(pcie);
>  
> -	reg = PCIE_CORE_LINK_L0S_ENTRY |
> -		(1 << PCIE_CORE_LINK_WIDTH_SHIFT);
> +	reg = (1 << PCIE_CORE_LINK_WIDTH_SHIFT);
>  	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
>  
>  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> -- 
> 2.13.5
>

Hello,

On Thu, 5 Oct 2017 12:46:07 -0500, Bjorn Helgaas wrote:

> On Thu, Sep 28, 2017 at 02:58:36PM +0200, Thomas Petazzoni wrote:
> > From: Victor Gu <xigu@marvell.com>
> > 
> > Some PCIe devices do not support LOS, and will cause timeouts if the
> > root complex forces the LOS state. This patch disables the LOS state
> > by default.  
> 
> Per PCIe r3.1, sec 5.4.1.3, software should not enable L0s in either
> direction unless both ends support L0s.
> 
> I'm unclear on what the bug is here.  Is the generic ASPM code
> incorrectly enabling L0s when one end doesn't support it?  That seems
> unlikely, but if so, it should be fixed in the generic ASPM code.
> 
> Are both ends advertising L0s support, but there's a hardware erratum
> on the Aardvark end that keeps it from working correctly?  If so, we
> should say that explicitly and include a reference to a published
> hardware erratum.
> 
> Something else?

I'll do some more research on this and get back to you.

> > This is part of fixing bug
> > https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
> > reported as the user to be important to get a Intel 7260 mini-PCIe
> > WiFi card working.  
> 
> The bugzilla link is a good start, but "reported by the user to be
> important" is meaningless by itself.  What we need here is the details
> of what's broken and how this fixes it.  Unfortunately the bugzilla
> doesn't have those details.

Yes, the issue is that the bug report doesn't have much details, and I
don't have the specific PCIe card that was used by the bug reporter.

Best regards,

Thomas