[PATCH] Revert "riscv: Clean up IPI initialization code"
Bin Meng
bmeng.cn at gmail.com
Thu Jul 16 04:41:21 CEST 2020
Hi Sean,
On Wed, Jul 8, 2020 at 6:48 PM Sean Anderson <seanga2 at 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 at gmail.com> wrote:
> >>
> >> 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;
> >> }
> >
> > 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 at 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 at entry=0x800e0b0 <__prci_init_clocks>,
rate=rate at 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 at entry=0x81cf0d8, stage=stage at 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 at 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 at entry=UCLASS_CPU,
devp=devp at 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
More information about the U-Boot
mailing list