Date:   Thu, 10 Mar 2022 09:06:08 +0000
From:   Ben Dooks <ben.dooks@...ethink.co.uk>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-pci@...r.kernel.org, paul.walmsley@...ive.com,
        greentime.hu@...ive.com, lorenzo.pieralisi@....com,
        robh@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] PCI: fu740: Force Gen1 to fix initial device probing on
 some boards

On 10/03/2022 00:15, Bjorn Helgaas wrote:
> On Tue, Mar 08, 2022 at 09:45:36AM +0000, Ben Dooks wrote:
>> On 28/02/2022 23:22, Ben Dooks wrote:
>>> The fu740 PCIe core does not probe any devices on the SiFive Unmatched
>>> board without this fix (or having U-Boot explicitly start the PCIe via
>>> either boot-script or user command). The fix is to start the link at
>>> Gen1 speeds and once the link is up then change the speed back.
>>>
>>> The U-Boot driver claims to set the link-speed to Gen1 to get the probe
>>> to work (and U-Boot does print link up at Gen1) in the following code:
>>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>
>>> --
>>> Note, this patch has had significant re-work since the previous 4
>>> sets, including trying to fix style, message, reliance on the U-Boot
>>> fix and the comments about usage of LINK_CAP and reserved fields.
>>
>> The internal feedback is this version is passing on our CI.
>>
>> If there are no comments on this soon, I will post this as either the
>> v5 of the original or as a new patch.
> 
> Seems like this isn't quite baked yet.  Lorenzo has the v4 of this on
> his pci/fu740 branch, but I'm going to drop that for now because (a)
> this one is better and (b) it'd be nice to have an ack from a FU740
> maintainer (Paul or Greentime).

Yes. I'll fix the comments up and try and get this out later in the
week. I hope the GPIO patch is easier and can be merged on its own.

It would be great if someone at SiFive could comment on this, I don't
really have a lot of info other than this doesn't work for any of our
boards. I just assume that everyone else boots from NVME and it just
works for them.

> I'm also not clear on whether this works around a general FU740 defect
> or something specific to the Unmatched board or the ASMedia ASM2824
> switch.  This patch currently limits to 2.5GT/s on *all* FU740
> devices.

I am not sure on this either, all I have is a pair of Unmatched boards
and neither work without this fix in. Has the FU740 been used by anyone
other than this board?

I have not verified with a speed test any of this yet. It should do
2.5GT/s as initial probe and then attempt to get the link back to
the maximum speed once the device has probed. It seems once the
ASM2824 has done the first initialisation it will then continue
back at the higher speed.

Once the ASM2824 is working it seems the rest will follow.

> I'd prefer to use "2.5GT/s" instead of "Gen1" in the subject, commit
> log, and comments because it's more specific and matches the
> PCI_EXP_LNKCAP_SLS_2_5GB in the code.

Ok, will do.

>>> ---
>>>    drivers/pci/controller/dwc/pcie-fu740.c | 51 ++++++++++++++++++++++++-
>>>    1 file changed, 50 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
>>> index 842b7202b96e..16ad52f53490 100644
>>> --- a/drivers/pci/controller/dwc/pcie-fu740.c
>>> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
>>> @@ -181,10 +181,59 @@ static int fu740_pcie_start_link(struct dw_pcie *pci)
>>>    {
>>>    	struct device *dev = pci->dev;
>>>    	struct fu740_pcie *afp = dev_get_drvdata(dev);
>>> +	u8 cap_exp = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>> +	int ret;
>>> +	u32 orig, tmp;
>>> +
>>> +	/*
>>> +	 * Force Gen1 when starting link, due to some devices not
>>> +	 * probing at higher speeds. This happens with the PCIe switch
>>> +	 * on the Unmatched board. The fix in U-Boot is to force Gen1
>>> +	 * and hope later resets will clear this capaility.
> 
> s/capaility/capability/
> 
> But the sentence still doesn't quite make sense.  Are you saying that
> if we bring the link up at 2.5GT/s, it will stay there?
> 
> And that a future reset may clear Link Capabilities?  Actually, I
> guess you don't want it *cleared*, you would just want it to
> accurately reflect the real max link speed, which would not be 0000b
> in the register (since that's not even a defined encoding).

So what I've seen is if U-boot does the initial probe at 2.5GT/s and
then Linux comes along and does the reset itself, the LINKCAP gets
set back to the original full speed then the device probe works under
Linux. I've not verified with any NVME speed test yet.

> And the reset would also cause link retrain that would then use the
> real max link speed?

I think so, the only verification I have done is lspci to check what
the link is reporting.

>>> +	dev_dbg(dev, "cap_exp at %x\n", cap_exp);
>>> +	dw_pcie_dbi_ro_wr_en(pci);
>>> +
>>> +	tmp = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCAP);
>>> +	orig = tmp & PCI_EXP_LNKCAP_SLS;
>>> +	tmp &= ~PCI_EXP_LNKCAP_SLS;
>>> +	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
>>> +	dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCAP, tmp);
>>>    	/* Enable LTSSM */
>>>    	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
>>> -	return 0;
>>> +
>>> +	ret = dw_pcie_wait_for_link(pci);
>>> +	if (ret) {
>>> +		dev_err(dev, "error: link did not start\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	tmp = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCAP);
>>> +	if ((tmp & PCI_EXP_LNKCAP_SLS) != orig) {
>>> +		dev_dbg(dev, "changing speed back to original\n");
>>> +
>>> +		tmp &= ~PCI_EXP_LNKCAP_SLS;
>>> +		tmp |= orig;
>>> +		dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCAP, tmp);
>>> +
>>> +		tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
>>> +		tmp |= PORT_LOGIC_SPEED_CHANGE;
>>> +		dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>>> +
>>> +		ret = dw_pcie_wait_for_link(pci);
>>> +		if (ret) {
>>> +			dev_err(dev, "error: link did not start at new speed\n");
>>> +			goto err;
>>> +		}
>>> +	}
>>> +
>>> +	ret = 0;
>>> +err:
>>> +	// todo - if we do have an unliekly error, what do we do here?
> 
> Wrong comment style (use /* */, not //), and s/unliekly/unlikely/

Ok, if no-one has a better idea I am just going to return the error
code for now.

> 
>>> +	dw_pcie_dbi_ro_wr_dis(pci);
>>> +	return ret;
>>>    }
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Powered by blists - more mailing lists