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

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.

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