* [PATCHv4 1/2] PCI: fu740: fix finding GPIOs @ 2022-02-21 21:03 ` Ben Dooks 0 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-21 21:03 UTC (permalink / raw) To: paul.walmsley, greentime.hu Cc: lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv, Ben Dooks The calls to devm_gpiod_get_optional() have the -gpios at the end of the name but the GPIO core code is already adding the suffix during the lookup. This means the PCIe driver is not finding the necessary reset or power lines to allow initialisation of the PCIe. This bug has not been noticed as if U-Boot has setup the GPIO lines for the hardware when it does the PCIe initialisation (either by booting from PCIe or user command to access PCIe) then the PCIe will work in Linux. The U-Boot as supplied by SiFive does not by default initialise any PCIe component. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/pci/controller/dwc/pcie-fu740.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c index 00cde9a248b5..842b7202b96e 100644 --- a/drivers/pci/controller/dwc/pcie-fu740.c +++ b/drivers/pci/controller/dwc/pcie-fu740.c @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev) return PTR_ERR(afp->mgmt_base); /* Fetch GPIOs */ - afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW); + afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(afp->reset)) return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n"); - afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW); + afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW); if (IS_ERR(afp->pwren)) return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCHv4 1/2] PCI: fu740: fix finding GPIOs @ 2022-02-21 21:03 ` Ben Dooks 0 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-21 21:03 UTC (permalink / raw) To: paul.walmsley, greentime.hu Cc: lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv, Ben Dooks The calls to devm_gpiod_get_optional() have the -gpios at the end of the name but the GPIO core code is already adding the suffix during the lookup. This means the PCIe driver is not finding the necessary reset or power lines to allow initialisation of the PCIe. This bug has not been noticed as if U-Boot has setup the GPIO lines for the hardware when it does the PCIe initialisation (either by booting from PCIe or user command to access PCIe) then the PCIe will work in Linux. The U-Boot as supplied by SiFive does not by default initialise any PCIe component. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/pci/controller/dwc/pcie-fu740.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c index 00cde9a248b5..842b7202b96e 100644 --- a/drivers/pci/controller/dwc/pcie-fu740.c +++ b/drivers/pci/controller/dwc/pcie-fu740.c @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev) return PTR_ERR(afp->mgmt_base); /* Fetch GPIOs */ - afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW); + afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(afp->reset)) return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n"); - afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW); + afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW); if (IS_ERR(afp->pwren)) return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n"); -- 2.34.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe 2022-02-21 21:03 ` Ben Dooks @ 2022-02-21 21:03 ` Ben Dooks -1 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-21 21:03 UTC (permalink / raw) To: paul.walmsley, greentime.hu Cc: lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv, Ben Dooks The fu740 PCIe core does not probe any devices on the SiFive Unmatched board without this fix from U-Boot (or having U-Boot explicitly start the PCIe via either boot-script or user command). 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 code is assumed to be correct and does fix the issue on the Unmatched. The code is at: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271 The code has been this way since the driver was commited in: https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f 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. + */ +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); + /* Enable LTSSM */ writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE); return 0; -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe @ 2022-02-21 21:03 ` Ben Dooks 0 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-21 21:03 UTC (permalink / raw) To: paul.walmsley, greentime.hu Cc: lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv, Ben Dooks The fu740 PCIe core does not probe any devices on the SiFive Unmatched board without this fix from U-Boot (or having U-Boot explicitly start the PCIe via either boot-script or user command). 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 code is assumed to be correct and does fix the issue on the Unmatched. The code is at: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271 The code has been this way since the driver was commited in: https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f 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. + */ +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); + /* Enable LTSSM */ writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE); return 0; -- 2.34.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe 2022-02-21 21:03 ` Ben Dooks @ 2022-02-23 20:51 ` Bjorn Helgaas -1 siblings, 0 replies; 14+ messages in thread From: Bjorn Helgaas @ 2022-02-23 20:51 UTC (permalink / raw) To: Ben Dooks Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote: > The fu740 PCIe core does not probe any devices on the SiFive Unmatched > board without this fix from U-Boot (or having U-Boot explicitly start > the PCIe via either boot-script or user command). > > 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 code is assumed to be > correct and does fix the issue on the Unmatched. The code is at: Maybe something like: Limit the link to Gen1 speed. since "the fix claims" and "the code is assumed" is sort of weasel-worded. The subject says "for initial device probe," but if you change PCI_EXP_LNKCAP, I assume that limits the link speed forever, even after a retrain? > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271 Maybe use this so the link doesn't become stale when more things are added to pcie_dw_sifive.c: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271 > The code has been this way since the driver was commited in: > https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f s/commited/committed/ > 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 usual multi-line comment style. > +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp ) > +{ > + unsigned val; u32, since that's what dw_pcie_readl_dbi() returns and dw_pcie_writel_dbi() expects. > + > + dw_pcie_dbi_ro_wr_en(dw); > + > + val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP); I assume 0x70 is the offset of the PCIe Capability. There should be a #define for that. > + pr_info("%s: link-cap was %08x\n", __func__, val); dev_info(pci->dev, "..."); > + dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf); I don't understand this. Per PCIe r6.0, sec 7.5.3.6, 1111b is a reserved encoding for the low four bits of PCI_EXP_LNKCAP. If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four bits should be 0001b to indicate Supported Link Speeds Vector field bit 0, which is defined as 2.5 GT/s. > + 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); I guess the "Unmatched" board is the only thing we need to care about here? Are there or will there be other boards that don't need this? > /* Enable LTSSM */ > writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE); > return 0; > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe @ 2022-02-23 20:51 ` Bjorn Helgaas 0 siblings, 0 replies; 14+ messages in thread From: Bjorn Helgaas @ 2022-02-23 20:51 UTC (permalink / raw) To: Ben Dooks Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote: > The fu740 PCIe core does not probe any devices on the SiFive Unmatched > board without this fix from U-Boot (or having U-Boot explicitly start > the PCIe via either boot-script or user command). > > 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 code is assumed to be > correct and does fix the issue on the Unmatched. The code is at: Maybe something like: Limit the link to Gen1 speed. since "the fix claims" and "the code is assumed" is sort of weasel-worded. The subject says "for initial device probe," but if you change PCI_EXP_LNKCAP, I assume that limits the link speed forever, even after a retrain? > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271 Maybe use this so the link doesn't become stale when more things are added to pcie_dw_sifive.c: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271 > The code has been this way since the driver was commited in: > https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f s/commited/committed/ > 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 usual multi-line comment style. > +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp ) > +{ > + unsigned val; u32, since that's what dw_pcie_readl_dbi() returns and dw_pcie_writel_dbi() expects. > + > + dw_pcie_dbi_ro_wr_en(dw); > + > + val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP); I assume 0x70 is the offset of the PCIe Capability. There should be a #define for that. > + pr_info("%s: link-cap was %08x\n", __func__, val); dev_info(pci->dev, "..."); > + dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf); I don't understand this. Per PCIe r6.0, sec 7.5.3.6, 1111b is a reserved encoding for the low four bits of PCI_EXP_LNKCAP. If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four bits should be 0001b to indicate Supported Link Speeds Vector field bit 0, which is defined as 2.5 GT/s. > + 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); I guess the "Unmatched" board is the only thing we need to care about here? Are there or will there be other boards that don't need this? > /* Enable LTSSM */ > writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE); > return 0; > -- > 2.34.1 > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe 2022-02-23 20:51 ` Bjorn Helgaas @ 2022-02-23 21:19 ` Maciej W. Rozycki -1 siblings, 0 replies; 14+ messages in thread From: Maciej W. Rozycki @ 2022-02-23 21:19 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ben Dooks, paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On Wed, 23 Feb 2022, Bjorn Helgaas wrote: > > + 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); > > I guess the "Unmatched" board is the only thing we need to care about > here? Are there or will there be other boards that don't need this? I wonder if this is the other side of a supposed erratum observed here: <https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/> Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream ports don't want to train with a Pericom part above Gen1. Of course we don't know an ASM2824 is there until we have successfully negotiated the link, so we may have to generalise my proposal if we can find a way similar to what I have done for U-boot that does not disturb Linux's operation. This is because there are PCIe option cards out there with the ASM2824 onboard, so it could be possible for the problem to trigger anywhere where the conditions for the erratum are met. Also in that case retraining should work with the cap removed to get a higher final speed just as with the Pericom part. Maciej _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe @ 2022-02-23 21:19 ` Maciej W. Rozycki 0 siblings, 0 replies; 14+ messages in thread From: Maciej W. Rozycki @ 2022-02-23 21:19 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ben Dooks, paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On Wed, 23 Feb 2022, Bjorn Helgaas wrote: > > + 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); > > I guess the "Unmatched" board is the only thing we need to care about > here? Are there or will there be other boards that don't need this? I wonder if this is the other side of a supposed erratum observed here: <https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/> Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream ports don't want to train with a Pericom part above Gen1. Of course we don't know an ASM2824 is there until we have successfully negotiated the link, so we may have to generalise my proposal if we can find a way similar to what I have done for U-boot that does not disturb Linux's operation. This is because there are PCIe option cards out there with the ASM2824 onboard, so it could be possible for the problem to trigger anywhere where the conditions for the erratum are met. Also in that case retraining should work with the cap removed to get a higher final speed just as with the Pericom part. Maciej ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe 2022-02-23 21:19 ` Maciej W. Rozycki @ 2022-02-28 23:15 ` Ben Dooks -1 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-28 23:15 UTC (permalink / raw) To: Maciej W. Rozycki, Bjorn Helgaas Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On 23/02/2022 21:19, Maciej W. Rozycki wrote: > On Wed, 23 Feb 2022, Bjorn Helgaas wrote: > >>> + 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); >> >> I guess the "Unmatched" board is the only thing we need to care about >> here? Are there or will there be other boards that don't need this? > > I wonder if this is the other side of a supposed erratum observed here: > > <https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/> > > Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream > ports don't want to train with a Pericom part above Gen1. > > Of course we don't know an ASM2824 is there until we have successfully > negotiated the link, so we may have to generalise my proposal if we can > find a way similar to what I have done for U-boot that does not disturb > Linux's operation. This is because there are PCIe option cards out there > with the ASM2824 onboard, so it could be possible for the problem to > trigger anywhere where the conditions for the erratum are met. > > Also in that case retraining should work with the cap removed to get a > higher final speed just as with the Pericom part. Possibly. I have just been working on a patch to better force Gen1 speeds and then restore the config which is working on my Unmatched board. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe @ 2022-02-28 23:15 ` Ben Dooks 0 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-28 23:15 UTC (permalink / raw) To: Maciej W. Rozycki, Bjorn Helgaas Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On 23/02/2022 21:19, Maciej W. Rozycki wrote: > On Wed, 23 Feb 2022, Bjorn Helgaas wrote: > >>> + 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); >> >> I guess the "Unmatched" board is the only thing we need to care about >> here? Are there or will there be other boards that don't need this? > > I wonder if this is the other side of a supposed erratum observed here: > > <https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/> > > Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream > ports don't want to train with a Pericom part above Gen1. > > Of course we don't know an ASM2824 is there until we have successfully > negotiated the link, so we may have to generalise my proposal if we can > find a way similar to what I have done for U-boot that does not disturb > Linux's operation. This is because there are PCIe option cards out there > with the ASM2824 onboard, so it could be possible for the problem to > trigger anywhere where the conditions for the erratum are met. > > Also in that case retraining should work with the cap removed to get a > higher final speed just as with the Pericom part. Possibly. I have just been working on a patch to better force Gen1 speeds and then restore the config which is working on my Unmatched board. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe 2022-02-23 20:51 ` Bjorn Helgaas @ 2022-02-28 17:45 ` Ben Dooks -1 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-28 17:45 UTC (permalink / raw) To: Bjorn Helgaas Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On 23/02/2022 20:51, Bjorn Helgaas wrote: > On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote: >> The fu740 PCIe core does not probe any devices on the SiFive Unmatched >> board without this fix from U-Boot (or having U-Boot explicitly start >> the PCIe via either boot-script or user command). >> >> 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 code is assumed to be >> correct and does fix the issue on the Unmatched. The code is at: > > Maybe something like: > > Limit the link to Gen1 speed. > > since "the fix claims" and "the code is assumed" is sort of > weasel-worded. > > The subject says "for initial device probe," but if you change > PCI_EXP_LNKCAP, I assume that limits the link speed forever, even > after a retrain? Yes, thought I had checked this but having just gone back and booted things again, the following is observed: - U-Boot "pci enum" and then unpatched kernel -> link 8GT/s - U-Boot "pci enum" and patched kernel -> link 2.5GT/s - U-Boot no pci and patched kernel > link 2.5 GT/s Clearly there needs to be some fix for this, either have two rounds of soft-reset on the first probe, or if the device does probe at a degraded link, do something about it. I am not clear what the correct fix for this is. 1) Detect no U-Boot initialisation and force a two stage probe 2) If no devices on initial probe, go around and do #1 3) Always do a two stage reset 4) Something else. Do any other systems see this issue? I assume we need to go back and re-do this. At least this is a 100% reliable repeat on the Unmatched board in our test rack. >> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271 > > Maybe use this so the link doesn't become stale when more things are > added to pcie_dw_sifive.c: > > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271 Thanks, useful to know. >> The code has been this way since the driver was commited in: >> https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f > > s/commited/committed/ Oops, > >> 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 usual multi-line comment style. Ok. > >> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp ) >> +{ >> + unsigned val; > > u32, since that's what dw_pcie_readl_dbi() returns and > dw_pcie_writel_dbi() expects. > >> + >> + dw_pcie_dbi_ro_wr_en(dw); >> + >> + val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP); > > I assume 0x70 is the offset of the PCIe Capability. There should be a > #define for that. Will fix. >> + pr_info("%s: link-cap was %08x\n", __func__, val); > > dev_info(pci->dev, "..."); > >> + dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf); > > I don't understand this. Per PCIe r6.0, sec 7.5.3.6, 1111b is a > reserved encoding for the low four bits of PCI_EXP_LNKCAP. > > If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four > bits should be 0001b to indicate Supported Link Speeds Vector field > bit 0, which is defined as 2.5 GT/s. Yeah, this does not make sense now. I sort of assumed it was W1C type thing (write-1-clear). I'm just wondering if this is now some sort of undefined behaviour or they originally meant to clear out some bits only... It does work, just shows the following from lspci; LnkCap: Port #0, Speed unknown, Width x8, ASPM L0s L1, Exit Latency L0s <4us, L1 <4us ClockPM- Surprise- LLActRep+ BwNot- ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s (downgraded), Width x8 (ok) TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt- >> + 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); > > I guess the "Unmatched" board is the only thing we need to care about > here? Are there or will there be other boards that don't need this? > >> /* Enable LTSSM */ >> writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE); >> return 0; >> -- >> 2.34.1 >> > -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe @ 2022-02-28 17:45 ` Ben Dooks 0 siblings, 0 replies; 14+ messages in thread From: Ben Dooks @ 2022-02-28 17:45 UTC (permalink / raw) To: Bjorn Helgaas Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On 23/02/2022 20:51, Bjorn Helgaas wrote: > On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote: >> The fu740 PCIe core does not probe any devices on the SiFive Unmatched >> board without this fix from U-Boot (or having U-Boot explicitly start >> the PCIe via either boot-script or user command). >> >> 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 code is assumed to be >> correct and does fix the issue on the Unmatched. The code is at: > > Maybe something like: > > Limit the link to Gen1 speed. > > since "the fix claims" and "the code is assumed" is sort of > weasel-worded. > > The subject says "for initial device probe," but if you change > PCI_EXP_LNKCAP, I assume that limits the link speed forever, even > after a retrain? Yes, thought I had checked this but having just gone back and booted things again, the following is observed: - U-Boot "pci enum" and then unpatched kernel -> link 8GT/s - U-Boot "pci enum" and patched kernel -> link 2.5GT/s - U-Boot no pci and patched kernel > link 2.5 GT/s Clearly there needs to be some fix for this, either have two rounds of soft-reset on the first probe, or if the device does probe at a degraded link, do something about it. I am not clear what the correct fix for this is. 1) Detect no U-Boot initialisation and force a two stage probe 2) If no devices on initial probe, go around and do #1 3) Always do a two stage reset 4) Something else. Do any other systems see this issue? I assume we need to go back and re-do this. At least this is a 100% reliable repeat on the Unmatched board in our test rack. >> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271 > > Maybe use this so the link doesn't become stale when more things are > added to pcie_dw_sifive.c: > > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271 Thanks, useful to know. >> The code has been this way since the driver was commited in: >> https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f > > s/commited/committed/ Oops, > >> 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 usual multi-line comment style. Ok. > >> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp ) >> +{ >> + unsigned val; > > u32, since that's what dw_pcie_readl_dbi() returns and > dw_pcie_writel_dbi() expects. > >> + >> + dw_pcie_dbi_ro_wr_en(dw); >> + >> + val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP); > > I assume 0x70 is the offset of the PCIe Capability. There should be a > #define for that. Will fix. >> + pr_info("%s: link-cap was %08x\n", __func__, val); > > dev_info(pci->dev, "..."); > >> + dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf); > > I don't understand this. Per PCIe r6.0, sec 7.5.3.6, 1111b is a > reserved encoding for the low four bits of PCI_EXP_LNKCAP. > > If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four > bits should be 0001b to indicate Supported Link Speeds Vector field > bit 0, which is defined as 2.5 GT/s. Yeah, this does not make sense now. I sort of assumed it was W1C type thing (write-1-clear). I'm just wondering if this is now some sort of undefined behaviour or they originally meant to clear out some bits only... It does work, just shows the following from lspci; LnkCap: Port #0, Speed unknown, Width x8, ASPM L0s L1, Exit Latency L0s <4us, L1 <4us ClockPM- Surprise- LLActRep+ BwNot- ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s (downgraded), Width x8 (ok) TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt- >> + 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); > > I guess the "Unmatched" board is the only thing we need to care about > here? Are there or will there be other boards that don't need this? > >> /* Enable LTSSM */ >> writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE); >> return 0; >> -- >> 2.34.1 >> > -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] PCI: fu740: fix finding GPIOs 2022-02-21 21:03 ` Ben Dooks @ 2022-02-23 20:51 ` Bjorn Helgaas -1 siblings, 0 replies; 14+ messages in thread From: Bjorn Helgaas @ 2022-02-23 20:51 UTC (permalink / raw) To: Ben Dooks Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On Mon, Feb 21, 2022 at 09:03:46PM +0000, Ben Dooks wrote: > The calls to devm_gpiod_get_optional() have the -gpios at the end of > the name but the GPIO core code is already adding the suffix during > the lookup. This means the PCIe driver is not finding the necessary > reset or power lines to allow initialisation of the PCIe. > > This bug has not been noticed as if U-Boot has setup the GPIO lines > for the hardware when it does the PCIe initialisation (either by > booting from PCIe or user command to access PCIe) then the PCIe > will work in Linux. The U-Boot as supplied by SiFive does not by > default initialise any PCIe component. Lorenzo, if you apply this, would you mind s/fix/Fix/ in the subject? Or maybe even update to "Drop redundant '-gpios' from DT GPIO lookup"? > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > drivers/pci/controller/dwc/pcie-fu740.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c > index 00cde9a248b5..842b7202b96e 100644 > --- a/drivers/pci/controller/dwc/pcie-fu740.c > +++ b/drivers/pci/controller/dwc/pcie-fu740.c > @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev) > return PTR_ERR(afp->mgmt_base); > > /* Fetch GPIOs */ > - afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW); > + afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(afp->reset)) > return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n"); > > - afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW); > + afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW); > if (IS_ERR(afp->pwren)) > return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n"); > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] PCI: fu740: fix finding GPIOs @ 2022-02-23 20:51 ` Bjorn Helgaas 0 siblings, 0 replies; 14+ messages in thread From: Bjorn Helgaas @ 2022-02-23 20:51 UTC (permalink / raw) To: Ben Dooks Cc: paul.walmsley, greentime.hu, lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-kernel, linux-riscv On Mon, Feb 21, 2022 at 09:03:46PM +0000, Ben Dooks wrote: > The calls to devm_gpiod_get_optional() have the -gpios at the end of > the name but the GPIO core code is already adding the suffix during > the lookup. This means the PCIe driver is not finding the necessary > reset or power lines to allow initialisation of the PCIe. > > This bug has not been noticed as if U-Boot has setup the GPIO lines > for the hardware when it does the PCIe initialisation (either by > booting from PCIe or user command to access PCIe) then the PCIe > will work in Linux. The U-Boot as supplied by SiFive does not by > default initialise any PCIe component. Lorenzo, if you apply this, would you mind s/fix/Fix/ in the subject? Or maybe even update to "Drop redundant '-gpios' from DT GPIO lookup"? > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > drivers/pci/controller/dwc/pcie-fu740.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c > index 00cde9a248b5..842b7202b96e 100644 > --- a/drivers/pci/controller/dwc/pcie-fu740.c > +++ b/drivers/pci/controller/dwc/pcie-fu740.c > @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev) > return PTR_ERR(afp->mgmt_base); > > /* Fetch GPIOs */ > - afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW); > + afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(afp->reset)) > return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n"); > > - afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW); > + afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW); > if (IS_ERR(afp->pwren)) > return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n"); > > -- > 2.34.1 > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-02-28 23:16 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-21 21:03 [PATCHv4 1/2] PCI: fu740: fix finding GPIOs Ben Dooks 2022-02-21 21:03 ` Ben Dooks 2022-02-21 21:03 ` [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe Ben Dooks 2022-02-21 21:03 ` Ben Dooks 2022-02-23 20:51 ` Bjorn Helgaas 2022-02-23 20:51 ` Bjorn Helgaas 2022-02-23 21:19 ` Maciej W. Rozycki 2022-02-23 21:19 ` Maciej W. Rozycki 2022-02-28 23:15 ` Ben Dooks 2022-02-28 23:15 ` Ben Dooks 2022-02-28 17:45 ` Ben Dooks 2022-02-28 17:45 ` Ben Dooks 2022-02-23 20:51 ` [PATCHv4 1/2] PCI: fu740: fix finding GPIOs Bjorn Helgaas 2022-02-23 20:51 ` Bjorn Helgaas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.