Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 14 Feb 2022 10:12:07 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Ben Dooks <ben.dooks@codethink.co.uk>
Cc:     linux-kernel@vger.kernel.org, bhelgaas@google.comv,
        linux-pci@vger.kernel.org, paul.walmsley@sifive.com,
        greentime.hu@sifive.com, david.abdurachmanov@gmail.com
Subject: Re: [PATCH 2/2] PCI: fu740: Force gen1 for initial device probe
Message-ID: <20220214161207.GA10207@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220214082144.1176084-3-ben.dooks@codethink.co.uk>
Precedence: bulk

On Mon, Feb 14, 2022 at 08:21:44AM +0000, Ben Dooks wrote:
> The fu740 dw pcie core does not probe devices without this fix from
> U-boot. The fix claims to set the link-speed to gen1 to get the probe
> to work. As this is a copy from U-boot, the commentary is assumed to
> be correct.

s/dw/DW/ (to match below)
s/pcie/PCIe/
s/U-boot/U-Boot/ (twice, and again below)

Is there a stable URL to the place in U-Boot where this is copied
from?

> Without this in, and without U-boot starting the PCIe the Unmatched
> board does not show any PCIe devices after the DW root port.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> index 842b7202b96e..19501ec8c487 100644
> --- a/drivers/pci/controller/dwc/pcie-fu740.c
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
>  	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
>  }
>  
> +/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
> + * as found on the SiFive Unmatched board.
> + */

s/u-boot/U-Boot/

Use this comment style to match the rest of the file:

  /*
   * Comment...
   */

> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
> +{
> +	unsigned val;
> +
> +	dw_pcie_dbi_ro_wr_en(dw);
> +
> +	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);
> +	pr_info("%s: link-cap was %08x\n", __func__, val);
> +	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);
> +
> +	dw_pcie_dbi_ro_wr_dis(dw);
> +}
> +
>  static int fu740_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct device *dev = pci->dev;
>  	struct fu740_pcie *afp = dev_get_drvdata(dev);
>  
> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
> +	fu740_pcie_force_gen1(pci, afp);

Is Unmatched the *only* board with this controller, i.e., do we want
to do this for every single FU740 device?

If this is an FU740 defect that will affect anything that uses it, we
should say that, and we shouldn't call out "Unmatched" specifically.

> +
>  	/* Enable LTSSM */
>  	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
>  	return 0;
> -- 
> 2.34.1
>