Hi Sean,
On Wed, Jul 8, 2020 at 6:48 PM Sean Anderson seanga2@gmail.com wrote:
On 7/8/20 6:09 AM, Bin Meng wrote:
Hi Sean,
On Wed, Jul 8, 2020 at 5:34 PM Sean Anderson seanga2@gmail.com wrote:
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;}
Yes, for Andes PLMT, it should be a timer device. However for SiFive CLINT, it is a multi-function device, and this does not fit very well.
The IPI is not a device as far as the rest of u-boot is concerned. I think we can just use uclass_get_device_by_driver in riscv_ipi_init. There is a patch to add a clint driver for Linux right now [1], and it does a similar thing. In that patch, the IPI is initialized by the timer driver.
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();Yes, this partial revert works.
By the way, what is the exact call to udelay (or something similar) which triggers this bug? It must occur between the end of the first riscv_cpu_bind and the rest of arch_cpu_init_dm. Before that, there is no timer device, and after that there are clearly no delays.
Here is the calling stack:
#0 get_ticks () at lib/time.c:99 #1 0x0000000008004fa0 in __udelay (usec=usec@entry=70) at lib/time.c:175 #2 0x0000000008004fdc in udelay (usec=<optimized out>) at lib/time.c:187 #3 0x0000000008005e84 in sifive_fu540_prci_wrpll_set_rate (pc=pc@entry=0x800e0b0 <__prci_init_clocks>, rate=rate@entry=1000000000, parent_rate=<optimized out>) at drivers/clk/sifive/fu540-prci.c:473 #4 0x0000000008005ffc in sifive_fu540_prci_set_rate (clk=<optimized out>, rate=1000000000) at drivers/clk/sifive/fu540-prci.c:682 #5 0x0000000008005872 in clk_set_default_rates (stage=0, dev=0x81cf0d8) at drivers/clk/clk-uclass.c:296 #6 clk_set_defaults (dev=dev@entry=0x81cf0d8, stage=stage@entry=0) at drivers/clk/clk-uclass.c:327 #7 0x0000000008006470 in device_probe (dev=0x81cf0d8) at drivers/core/device.c:481 #8 0x000000000800642a in device_probe (dev=dev@entry=0x81cf188) at drivers/core/device.c:425 #9 0x0000000008006c0e in uclass_get_device_tail (dev=0x81cf188, ret=<optimized out>, devp=0x81c8b38) at drivers/core/uclass.c:440 #10 0x0000000008006cf2 in uclass_first_device (id=id@entry=UCLASS_CPU, devp=devp@entry=0x81c8b38) at drivers/core/uclass.c:561 #11 0x000000000800a618 in cpu_probe_all () at drivers/cpu/cpu-uclass.c:21 #12 0x0000000008000322 in riscv_cpu_probe () at arch/riscv/cpu/cpu.c:79 #13 arch_cpu_init_dm () at arch/riscv/cpu/cpu.c:79 #14 0x00000000080007c2 in board_init_f (dummy=<optimized out>) at board/sifive/fu540/spl.c:67 #15 0x00000000080000be in wait_for_gd_init () at arch/riscv/cpu/start.S:167
Regards, Bin