Re: [PATCH] Revert "riscv: Clean up IPI initialization code"

8 Jul 2020

On 7/8/20 1:02 AM, Bin Meng wrote:
...
From: Bin Meng bin.meng@windriver.com
This reverts commit 40686c394e533fec765fe237936e353c84e73fff.
Commit 40686c394e53 ("riscv: Clean up IPI initialization code")
caused U-Boot failed to boot on SiFive HiFive Unleashed board.
The codes inside arch_cpu_init_dm() may call U-Boot timer APIs
before the call to riscv_init_ipi(). At that time the timer register
base (e.g.: the SiFive CLINT device in this case) is unknown yet.
In general, I don't think the existing implementation for timers on
riscv (storage of base address in gd_t and initialization on first use)
is necessary at all. riscv_timer_probe gets called before riscv_get_time
gets called for the first time, and any initialization can be done
there. In addition, there is already a way to select and initialize
timers in the form of the /chosen/tick-timer property.
For example, the following (untested) patch converts the andestech timer
to a uclass timer driver. It fails to link since riscv_get_time is no
longer provided, but that function is only ever used by the riscv-timer
driver.
---
 arch/riscv/dts/ae350_32.dts |  2 ++
 arch/riscv/dts/ae350_64.dts |  2 ++
 arch/riscv/lib/andes_plmt.c | 51 +++++++++++++++++++++----------------
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/arch/riscv/dts/ae350_32.dts b/arch/riscv/dts/ae350_32.dts
index 3f8525fe56..f8f7ec8073 100644
--- a/arch/riscv/dts/ae350_32.dts
+++ b/arch/riscv/dts/ae350_32.dts
@@ -14,6 +14,7 @@
    chosen {
    	bootargs = "console=ttyS0,38400n8  debug loglevel=7";
    	stdout-path = "uart0:38400n8";
+		tick-timer = "/soc/plmt0@e6000000";
    };
cpus {
@@ -162,6 +163,7 @@
    			&CPU2_intc 7
    			&CPU3_intc 7>;
    		reg = <0xe6000000 0x100000>;
+			clock-frequency = <60000000>;
    	};
    };
diff --git a/arch/riscv/dts/ae350_64.dts b/arch/riscv/dts/ae350_64.dts
index 482c707503..f014f053aa 100644
--- a/arch/riscv/dts/ae350_64.dts
+++ b/arch/riscv/dts/ae350_64.dts
@@ -14,6 +14,7 @@
    chosen {
    	bootargs = "console=ttyS0,38400n8  debug loglevel=7";
    	stdout-path = "uart0:38400n8";
+		tick-timer = "/soc/plmt0@e6000000";
    };
cpus {
@@ -162,6 +163,7 @@
    			&CPU2_intc 7
    			&CPU3_intc 7>;
    		reg = <0x0 0xe6000000 0x0 0x100000>;
+			clock-frequency = <60000000>;
    	};
    };
diff --git a/arch/riscv/lib/andes_plmt.c b/arch/riscv/lib/andes_plmt.c
index a7e90ca992..653fa55390 100644
--- a/arch/riscv/lib/andes_plmt.c
+++ b/arch/riscv/lib/andes_plmt.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright (C) 2019, Rick Chen rick@andestech.com
+ * Copyright (C) 2020, Sean Anderson seanga2@gmail.com
  *
  * U-Boot syscon driver for Andes's Platform Level Machine Timer (PLMT).
  * The PLMT block holds memory-mapped mtime register
@@ -9,46 +10,52 @@
#include <common.h>
 #include <dm.h>
-#include <regmap.h>
-#include <syscon.h>
+#include <timer.h>
 #include <asm/io.h>
-#include <asm/syscon.h>
 #include <linux/err.h>
/* mtime register */
 #define MTIME_REG(base)			((ulong)(base))
-DECLARE_GLOBAL_DATA_PTR;
-
-#define PLMT_BASE_GET(void)						\
-	do {								\
-		long *ret;						\
-									\
-		if (!gd->arch.plmt) {					\
-			ret = syscon_get_first_range(RISCV_SYSCON_PLMT); \
-			if (IS_ERR(ret))				\
-				return PTR_ERR(ret);			\
-			gd->arch.plmt = ret;				\
-		}							\
-	} while (0)
-
-int riscv_get_time(u64 *time)
+static int andes_plmt_get_count(struct udevice* dev, u64 *count)
 {
-	PLMT_BASE_GET();
+	*count = readq((void __iomem *)MTIME_REG(dev->priv));
-	*time = readq((void __iomem *)MTIME_REG(gd->arch.plmt));
+	return 0;
+}
+
+static const struct timer_ops andes_plmt_ops = {
+	.get_count = andes_plmt_get_count,
+};
+
+static int andes_plmt_probe(struct udevice *dev)
+{
+	int ret;
+	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	u32 clock_rate;
+
+	dev->priv = dev_read_addr_ptr(dev);
+	if (!dev->priv)
+		return -EINVAL;
+
+	ret = dev_read_u32(dev, "clock-frequency", &clock_rate);
+	if (ret)
+		return ret;
+	uc_priv->clock_rate = clock_rate;
return 0;
 }
static const struct udevice_id andes_plmt_ids[] = {
-	{ .compatible = "riscv,plmt0", .data = RISCV_SYSCON_PLMT },
+	{ .compatible = "riscv,plmt0" },
    { }
 };
U_BOOT_DRIVER(andes_plmt) = {
    .name		= "andes_plmt",
-	.id		= UCLASS_SYSCON,
+	.id		= UCLASS_TIMER,
    .of_match	= andes_plmt_ids,
+	.ops		= &andes_plmt_ops,
+	.probe		= andes_plmt_probe,
    .flags		= DM_FLAG_PRE_RELOC,
 };
-- 
2.26.2

> It might be the name riscv_init_ipi() that misleads people to only
> consider it is related to IPI, but in fact the timer capability is
> provided by the same SiFive CLINT device that provides the IPI.
> Timer capability is needed for both UP and SMP.

Ideally, it *is* only related to IPIs. As outlined above, timers can be
implemented without using global data at all by leveraging existing
systems. The dependency here was unintended.

> As the commit was a clean up attempt which did not bring in any
> other benefits than to break the SiFive HiFive Unleashed board,
> revert it.

This refactor does have benefits. It makes the IPI code more similar to
U-boot initialization idioms. It also removes some quite (imo) ugly
macros. I think there is a much more minimal revert below which can be
used as a stopgap.

---
 arch/riscv/lib/sifive_clint.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/riscv/lib/sifive_clint.c b/arch/riscv/lib/sifive_clint.c
index 78fc6c868d..3dfafd9130 100644
--- a/arch/riscv/lib/sifive_clint.c
+++ b/arch/riscv/lib/sifive_clint.c
@@ -24,8 +24,22 @@

 DECLARE_GLOBAL_DATA_PTR;

+#define CLINT_BASE_GET(void)						\
+	do {								\
+		long *ret;						\
+									\
+		if (!gd->arch.clint) {					\
+			ret = syscon_get_first_range(RISCV_SYSCON_CLINT); \
+			if (IS_ERR(ret))				\
+				return PTR_ERR(ret);			\
+			gd->arch.clint = ret;				\
+		}							\
+	} while (0)
+
 int riscv_get_time(u64 *time)
 {
+	CLINT_BASE_GET();
+
    *time = readq((void __iomem *)MTIME_REG(gd->arch.clint));

    return 0;
@@ -33,6 +47,8 @@ int riscv_get_time(u64 *time)

 int riscv_set_timecmp(int hart, u64 cmp)
 {
+	CLINT_BASE_GET();
+
    writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart));

    return 0;
-- 
2.26.2

Alternatively, the following patch would also (indirectly) work, though
it is more brittle.

---
 arch/riscv/cpu/cpu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index bbd6c15352..1fe22d28b3 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -76,6 +76,12 @@ int arch_cpu_init_dm(void)
 {
    int ret;

+#ifdef CONFIG_SMP
+	ret = riscv_init_ipi();
+	if (ret)
+		return ret;
+#endif
+
    ret = riscv_cpu_probe();
    if (ret)
    	return ret;
@@ -107,12 +113,6 @@ int arch_cpu_init_dm(void)
 #endif
    }

-#ifdef CONFIG_SMP
-	ret = riscv_init_ipi();
-	if (ret)
-		return ret;
-#endif
-
    return 0;
 }

-- 
2.26.2


    