From patchwork Wed Jul 8 09:34:11 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sean Anderson X-Patchwork-Id: 241031 List-Id: U-Boot discussion From: seanga2 at gmail.com (Sean Anderson) Date: Wed, 8 Jul 2020 05:34:11 -0400 Subject: [PATCH] Revert "riscv: Clean up IPI initialization code" In-Reply-To: <1594184579-30271-1-git-send-email-bmeng.cn@gmail.com> References: <1594184579-30271-1-git-send-email-bmeng.cn@gmail.com> Message-ID: On 7/8/20 1:02 AM, Bin Meng wrote: > From: Bin Meng > > 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 + * Copyright (C) 2020, Sean Anderson * * 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 #include -#include -#include +#include #include -#include #include /* 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; }