Hi Sean,
On Wed, Jul 8, 2020 at 5:34 PM Sean Anderson <sean...@gmail.com> wrote:
>
> On 7/8/20 1:02 AM, Bin Meng wrote:
> > From: Bin Meng <bin.m...@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 <r...@andestech.com>
> + * Copyright (C) 2020, Sean Anderson <sean...@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.
>
> 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.
> *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
No, this one does not work. At least it should be #if
CONFIG_IS_ENABLED(SMP) to consider the SPL case.
But even considering SPL, we should also consider UP as well because
timer is unconditionally needed regardless of UP/SMP.
> +
> 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
Regards,
Bin
