[PATCH] Revert "riscv: Clean up IPI initialization code"
Sean Anderson
seanga2 at gmail.com
Wed Jul 8 11:34:11 CEST 2020
On 7/8/20 1:02 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng at 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 at 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 at 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 at andestech.com>
+ * Copyright (C) 2020, Sean Anderson <seanga2 at 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
More information about the U-Boot
mailing list