[PATCH 0/9] pci: spacemit: fixes for the K1 PCIe host driver

[PATCH 0/9] pci: spacemit: fixes for the K1 PCIe host driver

[Dave Klotz]

Fixes for "pci: spacemit: add DesignWare PCIe host controller driver",
following the Linux driver and the bring-up order that works on the
boards I have:

- power the slot from the root port's vpcie3v3-supply (the vendor
  k1x,pwr_on property is not in any upstream device tree);
- keep the PHY interface in reset until LTSSM enable, and soft-reset
  the controller before bring-up so a warm reset starts clean;
- check clock, reset and iATU setup errors;
- report link up from the SMLH/RDLH bits in the link window;
- leave the link speed alone unless max-link-speed is set, instead of
  writing Gen1 into LNKCAP and LNKCTL2, so the OS can still train the
  link at its full speed;
- drop ASPM L1 from LNKCAP as Linux does for NVMe drives;
- set the link width from num-lanes;
- keep the probe successful when the link is down (an empty slot), and
  answer config cycles behind a down link with all ones.

Tested on an OrangePi RV2: an NVMe drive on port B is found and
FreeBSD boots from it by EFI; FreeBSD then retrains the link to x2 at
5 GT/s (with the Gen1 cap in place it could not). Port C, empty, reports
"Link down" and nothing hangs. Under U-Boot this drive still trains x1
at 2.5 GT/s.

Two observations about the RV2 device tree, not addressed here:
pcie1_3_cfg and pcie2_4_cfg mux CLKREQ#, and with CLKREQ# muxed the
controller gates its DBI clock whenever the endpoint leaves CLKREQ# high,
so the next DBI access hangs; I run the board with PERST# only in those
groups. And port B needs num-lanes = <2>, which the tree does not set.

Dave Klotz (9):
  pci: spacemit: power the slot from vpcie3v3-supply
  pci: spacemit: hold the PHY interface in reset until link start
  pci: spacemit: soft reset the controller before bring-up
  pci: spacemit: check clock, reset and iATU setup results
  pci: spacemit: report link up from the link status register
  pci: spacemit: do not force the link to 2.5 GT/s
  pci: spacemit: do not advertise ASPM L1
  pci: spacemit: set the link width from num-lanes
  pci: spacemit: do not fail the probe when the link is down

 drivers/pci/pcie_dw_spacemit.c | 231 +++++++++++++++++++++++----------
 drivers/pci/pcie_dw_spacemit.h |  12 --
 2 files changed, 162 insertions(+), 81 deletions(-)


base-commit: a000f2af240eaa996cec92f8d44bad96fdf3f898
-- 
2.55.0

[PATCH 1/9] pci: spacemit: power the slot from vpcie3v3-supply

[Dave Klotz]

The driver takes the slot power switch from a "k1x,pwr_on" GPIO and a
"power-on-status" polarity flag. Those are vendor properties: neither
the spacemit,k1-pcie binding nor any upstream device tree has them. The
binding describes the 3.3 V slot supply as a regulator, vpcie3v3-supply,
on the root port node, and that is ignored. On boards whose M.2 slot
sits behind a GPIO-switched regulator, such as the OrangePi RV2
(GPIO 116), the slot is never powered and the link cannot train.

Look up vpcie3v3-supply on the root port, falling back to the host
bridge node where some board device trees also put it and where Linux's
pcie-spacemit-k1 driver reads it, and enable it in place of the GPIO.
PERST# is still held for 100 ms after the supply is turned on. Drop the
vendor GPIO properties.

Unlike Linux, only warn when the supply is described but cannot be
found: the Banana Pi F3 and MUSE Pi Pro describe always-on fixed
supplies, and spacemit_k1_defconfig does not build the fixed regulator
driver, so failing the probe would break ports that work today.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 72 +++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 09c56d8a4c..7ca5a5ad1a 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -8,7 +8,6 @@
  */
 #include <asm/global_data.h>
 #include <asm/io.h>
-#include <asm-generic/gpio.h>
 #include <clk.h>
 #include <dm.h>
 #include <dm/device_compat.h>
@@ -21,6 +20,7 @@
 #include <log.h>
 #include <pci.h>
 #include <power-domain.h>
+#include <power/regulator.h>
 #include <reset.h>
 
 #include "pcie_dw_common.h"
@@ -39,8 +39,7 @@ struct pcie_dw_spacemit {
 	/* reset, clock resources */
 	struct clk_bulk		clks;
 	struct reset_ctl_bulk	rsts;
-	struct gpio_desc		pwr_on_gpio;
-	int			power_on_status;
+	struct udevice		*vpcie3v3;	/* slot supply, if described */
 };
 
 static inline u32 spacemit_pcie_readl(struct pcie_dw_spacemit *pcie, u32 offset)
@@ -185,30 +184,26 @@ static int pcie_dw_init_id(struct pcie_dw_spacemit *pci)
 	return 0;
 }
 
-static int k1x_power_on(struct pcie_dw_spacemit *pci, int on)
+/*
+ * The binding describes the 3.3 V slot supply on the root port node. Some
+ * board device trees also, or only, put it on the host bridge node, which
+ * is where Linux's driver looks for it.
+ */
+static int spacemit_pcie_get_vpcie3v3(struct udevice *dev, ofnode port,
+				      struct udevice **supplyp)
 {
-	struct pcie_dw *dwp = &pci->dw;
-	int gpio_val = 0;
-
-	if (on) {
-		if (pci->power_on_status)
-			gpio_val = 1;
-		else
-			gpio_val = 0;
-	} else {
-		if (pci->power_on_status)
-			gpio_val = 0;
-		else
-			gpio_val = 1;
-	}
-
-	if (dm_gpio_is_valid(&pci->pwr_on_gpio)) {
-		dev_info(dwp->dev, "PCIe interface power %s, set gpio %d to %d\n",
-			 on ? "on" : "off", pci->pwr_on_gpio.offset, gpio_val);
-		dm_gpio_set_value(&pci->pwr_on_gpio, gpio_val);
+	ofnode supply;
+
+	supply = ofnode_parse_phandle(port, "vpcie3v3-supply", 0);
+	if (!ofnode_valid(supply))
+		supply = ofnode_parse_phandle(dev_ofnode(dev),
+					      "vpcie3v3-supply", 0);
+	if (!ofnode_valid(supply)) {
+		*supplyp = NULL;
+		return 0;
 	}
 
-	return 0;
+	return uclass_get_device_by_ofnode(UCLASS_REGULATOR, supply, supplyp);
 }
 
 static int pcie_dw_spacemit_probe(struct udevice *dev)
@@ -241,6 +236,13 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 		return -ENODEV;
 	}
 
+	ret = spacemit_pcie_get_vpcie3v3(dev, port, &pci->vpcie3v3);
+	if (ret) {
+		/* e.g. an always-on fixed supply with no driver built in */
+		dev_warn(dev, "vpcie3v3 supply not available: %d\n", ret);
+		pci->vpcie3v3 = NULL;
+	}
+
 	ret = generic_phy_get_by_index_nodev(port, 0, &phy0);
 	if (!ret) {
 		reg = spacemit_pcie_readl(pci, PCIECTRL_K1X_CONF_DEVICE_CMD);
@@ -271,8 +273,15 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 
 	pcie_set_mode(pci, DW_PCIE_RC_TYPE);
 
-	/* power on the interface */
-	k1x_power_on(pci, 1);
+	/* Power the slot; PERST# stays asserted for 100 ms after this */
+	if (pci->vpcie3v3) {
+		ret = regulator_set_enable_if_allowed(pci->vpcie3v3, true);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie3v3 supply: %d\n",
+				ret);
+			return ret;
+		}
+	}
 
 	spacemit_pcie_host_init(pci);
 	pcie_dw_setup_host(&pci->dw);
@@ -356,17 +365,6 @@ static int pcie_dw_spacemit_of_to_plat(struct udevice *dev)
 		return ret;
 	}
 
-	gpio_request_by_name(dev, "k1x,pwr_on", 0, &pcie->pwr_on_gpio,
-			     GPIOD_IS_OUT);
-	if (!dm_gpio_is_valid(&pcie->pwr_on_gpio))
-		dev_info(dev, "has no power on gpio.\n");
-
-	ret = dev_read_u32(dev, "power-on-status", &pcie->power_on_status);
-	if (ret) {
-		dev_info(dev, "has no power-on-status flag, use default.\n");
-		pcie->power_on_status = 1;
-	}
-
 	return 0;
 }
 
-- 
2.55.0

[PATCH 2/9] pci: spacemit: hold the PHY interface in reset until link start

[Dave Klotz]

The probe sets APP_HOLD_PHY_RST and clears it again straight away,
before the PHY is initialised, so the controller's PIPE interface comes
out of reset while the PHY is still unconfigured and its PLL unlocked.

Linux's pcie-spacemit-k1 driver sets APP_HOLD_PHY_RST at probe and
clears it only in start_link(), in the same write that sets LTSSM_EN.
FreeBSD's spacemit_pcie driver, which runs these controllers on the
OrangePi RV2 and R2S, does the same and notes that releasing the PIPE
interface before the PHY is ready leaves the LTSSM stuck in Detect.

Keep the bit set through PHY init. The write that enables the LTSSM
already clears it.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 7ca5a5ad1a..04b3161e94 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -245,12 +245,14 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 
 	ret = generic_phy_get_by_index_nodev(port, 0, &phy0);
 	if (!ret) {
+		/*
+		 * Hold the PHY interface in reset through PHY init; it is
+		 * released together with LTSSM_EN when link training starts.
+		 */
 		reg = spacemit_pcie_readl(pci, PCIECTRL_K1X_CONF_DEVICE_CMD);
 		reg |= DEVICE_TYPE_RC | APP_HOLD_PHY_RST | PCIE_PHY_LANE_CTRL_MASK;
 		reg &= ~GLOBAL_PHY_RST;
 		spacemit_pcie_writel(pci, PCIECTRL_K1X_CONF_DEVICE_CMD, reg);
-		reg &= ~(u32)APP_HOLD_PHY_RST;
-		spacemit_pcie_writel(pci, PCIECTRL_K1X_CONF_DEVICE_CMD, reg);
 
 		ret = generic_phy_init(&phy0);
 		if (ret) {
-- 
2.55.0

[PATCH 3/9] pci: spacemit: soft reset the controller before bring-up

[Dave Klotz]

The probe brings the controller up from whatever state it was left in.
After a warm reboot, or when an earlier boot stage has used the port,
the link can still be trained; the driver then prints "PCI Link already
up before configuration!" and skips the link configuration altogether.

Linux's pcie-spacemit-k1 driver and FreeBSD's spacemit_pcie driver both
pulse PCIE_SOFT_RESET in the APMU control logic register for 2 ms
before enabling the application clocks and resets. Do the same, and
drop the "already up" shortcut, which can no longer be taken.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 04b3161e94..b85de73ad3 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -106,11 +106,6 @@ static int pcie_dw_spacemit_pcie_link_up(struct pcie_dw_spacemit *pci, u32 cap_s
 {
 	u32 reg;
 
-	if (is_link_up(pci)) {
-		printf("PCI Link already up before configuration!\n");
-		return 1;
-	}
-
 	/* DW pre link configurations */
 	pcie_dw_configure(pci, cap_speed);
 
@@ -133,6 +128,22 @@ static int pcie_dw_spacemit_pcie_link_up(struct pcie_dw_spacemit *pci, u32 cap_s
 	return 1;
 }
 
+/*
+ * Reset the controller, so that nothing left over from an earlier boot
+ * stage or a warm reboot (a trained link, a stale LTSSM state) survives.
+ */
+static void spacemit_pcie_toggle_soft_reset(struct pcie_dw_spacemit *pci)
+{
+	u32 reg;
+
+	reg = spacemit_pcie_readl(pci, PCIE_CTRL_LOGIC);
+	spacemit_pcie_writel(pci, PCIE_CTRL_LOGIC, reg | PCIE_SOFT_RESET);
+	/* Read back so that the write has landed before the delay */
+	spacemit_pcie_readl(pci, PCIE_CTRL_LOGIC);
+	mdelay(2);
+	spacemit_pcie_writel(pci, PCIE_CTRL_LOGIC, reg & ~PCIE_SOFT_RESET);
+}
+
 static int pcie_set_mode(struct pcie_dw_spacemit *pci,
 			 enum dw_pcie_device_mode mode)
 {
@@ -216,6 +227,8 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 	int ret;
 	u32 reg;
 
+	spacemit_pcie_toggle_soft_reset(pci);
+
 	/* enable pcie clk and deassert resets */
 	clk_enable_bulk(&pci->clks);
 	reset_deassert_bulk(&pci->rsts);
-- 
2.55.0

[PATCH 4/9] pci: spacemit: check clock, reset and iATU setup results

[Dave Klotz]

clk_enable_bulk(), reset_deassert_bulk() and the outbound iATU setup
can all fail, and the probe ignores all three. A controller whose
clocks or resets did not come up is then accessed anyway, and a failed
memory window is reported as a successful probe.

Fail the probe with the error instead, as Linux's driver does for its
clocks and resets, and undo the clock enable when the resets fail.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index b85de73ad3..5e0871986b 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -230,8 +230,18 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 	spacemit_pcie_toggle_soft_reset(pci);
 
 	/* enable pcie clk and deassert resets */
-	clk_enable_bulk(&pci->clks);
-	reset_deassert_bulk(&pci->rsts);
+	ret = clk_enable_bulk(&pci->clks);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks: %d\n", ret);
+		return ret;
+	}
+
+	ret = reset_deassert_bulk(&pci->rsts);
+	if (ret) {
+		dev_err(dev, "failed to deassert resets: %d\n", ret);
+		clk_disable_bulk(&pci->clks);
+		return ret;
+	}
 
 	reg = spacemit_pcie_readl(pci, PCIECTRL_K1X_CONF_DEVICE_CMD);
 	reg &= ~LTSSM_EN;
@@ -317,6 +327,8 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 					       pci->dw.mem.phys_start,
 					       pci->dw.mem.bus_start,
 					       pci->dw.mem.size);
+	if (ret)
+		return -EIO;
 
 	return 0;
 }
-- 
2.55.0

[PATCH 5/9] pci: spacemit: report link up from the link status register

[Dave Klotz]

The driver decides the link is up when the LTSSM state in the
DesignWare PORT_DEBUG0 register reads L0. L0 is a physical layer state;
the data link layer may not be up yet, and config requests sent before
it is are lost.

Linux's pcie-spacemit-k1 driver reads the link status register in the
"link" window instead, and needs both SMLH_LINK_UP (physical layer) and
RDLH_LINK_UP (data link layer); FreeBSD's spacemit_pcie driver does the
same. The window is already mapped (as phy_ahb) and the bits already
defined, but nothing read them. Use them, and drop the now unused
PORT_DEBUG0 definitions.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 9 ++++++---
 drivers/pci/pcie_dw_spacemit.h | 5 -----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 5e0871986b..3394ddbd18 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -81,10 +81,13 @@ static int is_link_up(struct pcie_dw_spacemit *pci)
 {
 	u32 val;
 
-	val = readl(pci->dw.dbi_base + PCIE_PORT_DEBUG0);
-	val &= PORT_LOGIC_LTSSM_STATE_MASK;
+	/*
+	 * As in Linux, the link is up once both the physical layer (SMLH)
+	 * and the data link layer (RDLH) report it.
+	 */
+	val = spacemit_pcie_phy_ahb_readl(pci, K1X_PHY_AHB_LINK_STS);
 
-	return (val == PORT_LOGIC_LTSSM_STATE_L0);
+	return (val & SMLH_LINK_UP) && (val & RDLH_LINK_UP);
 }
 
 static int wait_link_up(struct pcie_dw_spacemit *pci)
diff --git a/drivers/pci/pcie_dw_spacemit.h b/drivers/pci/pcie_dw_spacemit.h
index 2c17f113b7..6f198f7d58 100644
--- a/drivers/pci/pcie_dw_spacemit.h
+++ b/drivers/pci/pcie_dw_spacemit.h
@@ -22,11 +22,6 @@
 #define PCIE_MISC_CONTROL_1_OFF	0x8bc
 #define PCIE_DBI_RO_WR_EN		BIT(0)
 
-#define PLR_OFFSET			0x700
-#define PCIE_PORT_DEBUG0		(PLR_OFFSET + 0x28)
-#define PORT_LOGIC_LTSSM_STATE_MASK	0x1f
-#define PORT_LOGIC_LTSSM_STATE_L0	0x11
-
 #define PCIE_LINK_UP_TIMEOUT_MS		1000
 
 /* Vendor and device IDs */
-- 
2.55.0

[PATCH 6/9] pci: spacemit: do not force the link to 2.5 GT/s

[Dave Klotz]

The driver writes Gen1 into the Supported Link Speeds field of the root
port's Link Capabilities and into the Target Link Speed of Link
Control 2, so the K1's 5 GT/s capable ports never train above 2.5 GT/s,
and an operating system that keeps the root port as found cannot
retrain them faster either. It also writes them at the fixed offsets
0x7c and 0xa0 instead of through the PCI Express capability.

Linux's DesignWare core leaves both fields alone unless the device tree
limits the speed with "max-link-speed", and then writes that value to
both. Do the same, finding the registers through the capability list,
and drop the driver's copies of the speed definitions that
pcie_dw_common.h already provides.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 48 ++++++++++++++++++++++++----------
 drivers/pci/pcie_dw_spacemit.h |  7 -----
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 3394ddbd18..6ac1d6bea4 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -40,6 +40,7 @@ struct pcie_dw_spacemit {
 	struct clk_bulk		clks;
 	struct reset_ctl_bulk	rsts;
 	struct udevice		*vpcie3v3;	/* slot supply, if described */
+	u32			max_link_speed;	/* 0: hardware default */
 };
 
 static inline u32 spacemit_pcie_readl(struct pcie_dw_spacemit *pcie, u32 offset)
@@ -58,21 +59,33 @@ static inline u32 spacemit_pcie_phy_ahb_readl(struct pcie_dw_spacemit *pcie, u32
 	return readl(pcie->phy_ahb + offset);
 }
 
-static void pcie_dw_configure(struct pcie_dw_spacemit *pci, u32 cap_speed)
+static void pcie_dw_configure(struct pcie_dw_spacemit *pci)
 {
-	u32 val;
+	void __iomem *cap;
+	u32 speed = pci->max_link_speed;
+	u16 lnkctl2;
+	u32 lnkcap;
 
-	dw_pcie_dbi_write_enable(&pci->dw, true);
+	cap = pci->dw.dbi_base + pcie_dw_find_capability(&pci->dw,
+							 PCI_CAP_ID_EXP);
 
-	val = readl(pci->dw.dbi_base + PCIE_LINK_CAPABILITY);
-	val &= ~TARGET_LINK_SPEED_MASK;
-	val |= cap_speed;
-	writel(val, pci->dw.dbi_base + PCIE_LINK_CAPABILITY);
+	dw_pcie_dbi_write_enable(&pci->dw, true);
 
-	val = readl(pci->dw.dbi_base + PCIE_LINK_CTL_2);
-	val &= ~TARGET_LINK_SPEED_MASK;
-	val |= cap_speed;
-	writel(val, pci->dw.dbi_base + PCIE_LINK_CTL_2);
+	/*
+	 * Keep the hardware's supported speeds unless the device tree limits
+	 * them with "max-link-speed", as Linux's DesignWare core does.
+	 */
+	if (speed) {
+		lnkcap = readl(cap + PCI_EXP_LNKCAP);
+		lnkcap &= ~PCI_EXP_LNKCAP_SLS;
+		lnkcap |= speed;
+		writel(lnkcap, cap + PCI_EXP_LNKCAP);
+
+		lnkctl2 = readw(cap + PCI_EXP_LNKCTL2);
+		lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
+		lnkctl2 |= speed;
+		writew(lnkctl2, cap + PCI_EXP_LNKCTL2);
+	}
 
 	dw_pcie_dbi_write_enable(&pci->dw, false);
 }
@@ -105,12 +118,12 @@ static int wait_link_up(struct pcie_dw_spacemit *pci)
 	return 1;
 }
 
-static int pcie_dw_spacemit_pcie_link_up(struct pcie_dw_spacemit *pci, u32 cap_speed)
+static int pcie_dw_spacemit_pcie_link_up(struct pcie_dw_spacemit *pci)
 {
 	u32 reg;
 
 	/* DW pre link configurations */
-	pcie_dw_configure(pci, cap_speed);
+	pcie_dw_configure(pci);
 
 	/* Initiate link training */
 	reg = spacemit_pcie_readl(pci, PCIECTRL_K1X_CONF_DEVICE_CMD);
@@ -315,7 +328,7 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 	pcie_dw_setup_host(&pci->dw);
 	pcie_dw_init_id(pci);
 
-	if (!pcie_dw_spacemit_pcie_link_up(pci, LINK_SPEED_GEN_1)) {
+	if (!pcie_dw_spacemit_pcie_link_up(pci)) {
 		printf("PCIE-%d: Link down\n", dev_seq(dev));
 		return -ENODEV;
 	}
@@ -380,6 +393,13 @@ static int pcie_dw_spacemit_of_to_plat(struct udevice *dev)
 		return -EINVAL;
 	pcie->apmu_offset = args.args[0];
 
+	pcie->max_link_speed = dev_read_u32_default(dev, "max-link-speed", 0);
+	if (pcie->max_link_speed > LINK_SPEED_GEN_4) {
+		dev_warn(dev, "invalid max-link-speed %u, ignored\n",
+			 pcie->max_link_speed);
+		pcie->max_link_speed = 0;
+	}
+
 	/* Derive port ID from DBI base address */
 	pcie->port_id = (dbi_addr - SPACEMIT_PCIE_DBI_BASE) / SPACEMIT_PCIE_DBI_STRIDE;
 
diff --git a/drivers/pci/pcie_dw_spacemit.h b/drivers/pci/pcie_dw_spacemit.h
index 6f198f7d58..21686de246 100644
--- a/drivers/pci/pcie_dw_spacemit.h
+++ b/drivers/pci/pcie_dw_spacemit.h
@@ -12,13 +12,6 @@
 #include <linux/bitops.h>
 
 /* PCIe DW common registers */
-#define PCIE_LINK_CAPABILITY		0x7c
-#define PCIE_LINK_CTL_2			0xa0
-#define TARGET_LINK_SPEED_MASK		0xf
-#define LINK_SPEED_GEN_1		0x1
-#define LINK_SPEED_GEN_2		0x2
-#define LINK_SPEED_GEN_3		0x3
-
 #define PCIE_MISC_CONTROL_1_OFF	0x8bc
 #define PCIE_DBI_RO_WR_EN		BIT(0)
 
-- 
2.55.0

[PATCH 7/9] pci: spacemit: do not advertise ASPM L1

[Dave Klotz]

The root port advertises ASPM L1 support in its Link Capabilities, so
software that configures ASPM from what the port reports, such as an
EFI application or an operating system that does not reinitialise the
controller, may put the link into L1.

Linux's pcie-spacemit-k1 driver clears PCI_EXP_LNKCAP_ASPM_L1 at init,
as a workaround for errors reported on some NVMe drives. Do the same
before link training starts, so the port is set up as Linux sets it up.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 6ac1d6bea4..3b5c2dc4e0 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -71,6 +71,12 @@ static void pcie_dw_configure(struct pcie_dw_spacemit *pci)
 
 	dw_pcie_dbi_write_enable(&pci->dw, true);
 
+	/*
+	 * Do not advertise ASPM L1: as Linux's driver notes, some NVMe drives
+	 * report errors with it.
+	 */
+	clrbits_le32(cap + PCI_EXP_LNKCAP, PCI_EXP_LNKCAP_ASPM_L1);
+
 	/*
 	 * Keep the hardware's supported speeds unless the device tree limits
 	 * them with "max-link-speed", as Linux's DesignWare core does.
-- 
2.55.0

[PATCH 8/9] pci: spacemit: set the link width from num-lanes

[Dave Klotz]

The driver never reads the standard "num-lanes" property, so a board
cannot set the width of a port: the OrangePi RV2's M.2 slot on port B
is wired for two lanes, and other boards may use only one of the two.

Linux's DesignWare core reads "num-lanes" and, when it is set,
programs the port link mode, the link width and speed control register
and the Maximum Link Width in Link Capabilities from it, leaving the
hardware default otherwise. dw_pcie_link_set_max_link_width() in
pcie_dw_common does exactly that; call it before link training.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 3b5c2dc4e0..9b299ca17a 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -41,6 +41,7 @@ struct pcie_dw_spacemit {
 	struct reset_ctl_bulk	rsts;
 	struct udevice		*vpcie3v3;	/* slot supply, if described */
 	u32			max_link_speed;	/* 0: hardware default */
+	u32			num_lanes;	/* 0: hardware default */
 };
 
 static inline u32 spacemit_pcie_readl(struct pcie_dw_spacemit *pcie, u32 offset)
@@ -93,6 +94,9 @@ static void pcie_dw_configure(struct pcie_dw_spacemit *pci)
 		writew(lnkctl2, cap + PCI_EXP_LNKCTL2);
 	}
 
+	/* Link width from "num-lanes"; without it, keep the hardware's */
+	dw_pcie_link_set_max_link_width(&pci->dw, pci->num_lanes);
+
 	dw_pcie_dbi_write_enable(&pci->dw, false);
 }
 
@@ -406,6 +410,8 @@ static int pcie_dw_spacemit_of_to_plat(struct udevice *dev)
 		pcie->max_link_speed = 0;
 	}
 
+	pcie->num_lanes = dev_read_u32_default(dev, "num-lanes", 0);
+
 	/* Derive port ID from DBI base address */
 	pcie->port_id = (dbi_addr - SPACEMIT_PCIE_DBI_BASE) / SPACEMIT_PCIE_DBI_STRIDE;
 
-- 
2.55.0

[PATCH 9/9] pci: spacemit: do not fail the probe when the link is down

[Dave Klotz]

When the link does not come up within the training timeout, the probe
returns -ENODEV. An empty slot is a normal configuration, but the
failed probe leaves the root port unusable, skips the outbound memory
window, and makes every later lookup of the bus (each "pci" command, a
boot device scan) probe the controller again, repeating the whole
bring-up and the one second wait.

Linux's DesignWare core carries on when the link does not come up and
only refuses config requests to the buses behind the root port while
the link is down (dw_pcie_other_conf_map_bus()); FreeBSD's
spacemit_pcie driver also treats a down link as non-fatal. Do the
same: report the link as down, finish the probe, and answer config
reads behind the root port with all ones, dropping writes, while the
link is down.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 drivers/pci/pcie_dw_spacemit.c | 53 +++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie_dw_spacemit.c b/drivers/pci/pcie_dw_spacemit.c
index 9b299ca17a..61c759172d 100644
--- a/drivers/pci/pcie_dw_spacemit.c
+++ b/drivers/pci/pcie_dw_spacemit.c
@@ -338,15 +338,18 @@ static int pcie_dw_spacemit_probe(struct udevice *dev)
 	pcie_dw_setup_host(&pci->dw);
 	pcie_dw_init_id(pci);
 
-	if (!pcie_dw_spacemit_pcie_link_up(pci)) {
+	/*
+	 * An empty slot, or a device that does not train, is not an error:
+	 * the root port is still usable and the config accessors keep
+	 * requests for the buses behind it off the wire.
+	 */
+	if (!pcie_dw_spacemit_pcie_link_up(pci))
 		printf("PCIE-%d: Link down\n", dev_seq(dev));
-		return -ENODEV;
-	}
-
-	printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev),
-	       pcie_dw_get_link_speed(&pci->dw),
-	       pcie_dw_get_link_width(&pci->dw),
-	       hose->first_busno);
+	else
+		printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev),
+		       pcie_dw_get_link_speed(&pci->dw),
+		       pcie_dw_get_link_width(&pci->dw),
+		       hose->first_busno);
 
 	ret = pcie_dw_prog_outbound_atu_unroll(&pci->dw, PCIE_ATU_REGION_INDEX0,
 					       PCIE_ATU_TYPE_MEM,
@@ -430,9 +433,39 @@ static int pcie_dw_spacemit_of_to_plat(struct udevice *dev)
 	return 0;
 }
 
+/*
+ * Nothing answers behind the root port while the link is down, so do not
+ * send config requests there; Linux's DesignWare core does the same.
+ */
+static int pcie_dw_spacemit_read_config(const struct udevice *bus,
+					pci_dev_t bdf, uint offset,
+					ulong *valuep, enum pci_size_t size)
+{
+	struct pcie_dw_spacemit *pci = dev_get_priv(bus);
+
+	if (PCI_BUS(bdf) != pci->dw.first_busno && !is_link_up(pci)) {
+		*valuep = pci_get_ff(size);
+		return 0;
+	}
+
+	return pcie_dw_read_config(bus, bdf, offset, valuep, size);
+}
+
+static int pcie_dw_spacemit_write_config(struct udevice *bus, pci_dev_t bdf,
+					 uint offset, ulong value,
+					 enum pci_size_t size)
+{
+	struct pcie_dw_spacemit *pci = dev_get_priv(bus);
+
+	if (PCI_BUS(bdf) != pci->dw.first_busno && !is_link_up(pci))
+		return 0;
+
+	return pcie_dw_write_config(bus, bdf, offset, value, size);
+}
+
 static const struct dm_pci_ops pcie_dw_spacemit_ops = {
-	.read_config	= pcie_dw_read_config,
-	.write_config	= pcie_dw_write_config,
+	.read_config	= pcie_dw_spacemit_read_config,
+	.write_config	= pcie_dw_spacemit_write_config,
 };
 
 static const struct udevice_id pcie_dw_spacemit_ids[] = {
-- 
2.55.0

[PATCH 0/2] spacemit: k1: SPL robustness for boards without a PMIC or SPI NOR

[Dave Klotz]

Two small SPL changes for K1 boards that do not look like the BPI-F3
and MusePi Pro:

1. pmic_init() and nor_early_init() panic when the device tree gives
   the SPL no PMIC or no SPI controller. A board that boots from eMMC or
   microSD with its rails already up (the OrangePi R2S and RV2) stops
   there. Log it and carry on.
2. Provide board_boot_order(): the device the BootROM booted from, as
   detected now, then the other MMC controller and SPI NOR, so an
   unexpected BootROM path still finds U-Boot.

Tested on an OrangePi RV2 booting from microSD (no SPI controller in
the SPL): "No SPI controller", then the SPL loads U-Boot from the card
through the detected MMC2. The fallback order was not exercised, since
the detection was right.

Dave Klotz (2):
  spacemit: k1: do not require a PMIC or an SPI controller in SPL
  spacemit: k1: fall back to the other boot devices in SPL

 board/spacemit/k1/spl.c | 48 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)


base-commit: a000f2af240eaa996cec92f8d44bad96fdf3f898
-- 
2.55.0

[PATCH 1/2] spacemit: k1: do not require a PMIC or an SPI controller in SPL

[Dave Klotz]

pmic_init() and nor_early_init() panic when the board device tree
describes no PMIC or no SPI controller to the SPL. The OrangePi R2S
boots from eMMC with its rails already up, and boards that boot from
eMMC or microSD need no SPI NOR, so the SPL stops before it can load
U-Boot.

Log the missing device and carry on instead. If the SPL then has to
boot from SPI without a controller, spl_spi_load_image() reports it.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 board/spacemit/k1/spl.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/board/spacemit/k1/spl.c b/board/spacemit/k1/spl.c
index ef23db62d6..96ee4449c4 100644
--- a/board/spacemit/k1/spl.c
+++ b/board/spacemit/k1/spl.c
@@ -301,8 +301,16 @@ void pmic_init(void)
 	int ret;
 
 	ret = uclass_get_device(UCLASS_PMIC, 0, &pmic_dev);
-	if (ret)
-		panic("Fail to detect PMIC (%d)\n", ret);
+	if (ret) {
+		/*
+		 * Not every K1 board describes its PMIC to the SPL, and on some
+		 * the rails are already up when the SPL runs. Only boards whose
+		 * DDR bring-up depends on programming the bucks need one, so a
+		 * missing PMIC is not fatal here.
+		 */
+		log_info("No PMIC (%d), leaving rails as found\n", ret);
+		return;
+	}
 	set_vdd_core();
 	set_vdd_1v8();
 	set_vdd_mmc();
@@ -412,9 +420,15 @@ void nor_early_init(void)
 	struct udevice *dev;
 	int ret;
 
+	/*
+	 * A board that boots from eMMC or microSD may describe no SPI
+	 * controller at all; that is not fatal unless the SPL boots from SPI.
+	 */
 	ret = uclass_get_device(UCLASS_SPI, 0, &dev);
-	if (ret)
-		panic("Fail to detect spi controller.\n");
+	if (ret) {
+		log_info("No SPI controller (%d)\n", ret);
+		return;
+	}
 	udelay(10);
 	ret = uclass_get_device(UCLASS_SPI_FLASH, 0, &dev);
 	if (ret)
-- 
2.55.0

[PATCH 2/2] spacemit: k1: fall back to the other boot devices in SPL

[Dave Klotz]

spl_boot_device() derives the boot device from BootROM state: the SD
API pointer the BootROM leaves in SRAM, the boot flag register, and the
boot strap. Not every BootROM path is covered by that. On the OrangePi
R2S, for example, the BootROM probes a card first ("try sd...") and only
then loads the FSBL from eMMC, and on the OrangePi RV2 it falls back
from the card to SPI NOR. If the detection names a device that holds no
image, the SPL stops with "failed to boot from all boot devices".

Provide board_boot_order(): the detected device first, then the other
MMC controller and SPI NOR when this SPL can read them.

Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
---
 board/spacemit/k1/spl.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/board/spacemit/k1/spl.c b/board/spacemit/k1/spl.c
index 96ee4449c4..cc0e26bb02 100644
--- a/board/spacemit/k1/spl.c
+++ b/board/spacemit/k1/spl.c
@@ -498,6 +498,32 @@ u32 spl_boot_device(void)
 	return ret;
 }
 
+/*
+ * Try the device the BootROM booted from first, then the other storage
+ * this SPL can read. The boot-mode detection relies on BootROM state; if
+ * it names a device that holds no image (the BootROM probed a card before
+ * falling back to eMMC, say), the SPL still finds U-Boot instead of
+ * stopping at "failed to boot from all boot devices".
+ */
+void board_boot_order(u32 *spl_boot_list)
+{
+	u32 fallbacks[3];
+	int i, n = 0, m = 0;
+
+	if (CONFIG_IS_ENABLED(MMC)) {
+		fallbacks[m++] = BOOT_DEVICE_MMC1;
+		fallbacks[m++] = BOOT_DEVICE_MMC2;
+	}
+	if (CONFIG_IS_ENABLED(SPI_LOAD))
+		fallbacks[m++] = BOOT_DEVICE_SPI;
+
+	spl_boot_list[n++] = spl_boot_device();
+	for (i = 0; i < m; i++)
+		if (fallbacks[i] != spl_boot_list[0])
+			spl_boot_list[n++] = fallbacks[i];
+	spl_boot_list[n] = BOOT_DEVICE_NONE;
+}
+
 void spl_board_init(void)
 {
 }
-- 
2.55.0