[PATCH] Revert "riscv: Clean up IPI initialization code"
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.
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.
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.
Signed-off-by: Bin Meng bin.meng@windriver.com ---
arch/riscv/cpu/cpu.c | 6 ------ arch/riscv/include/asm/smp.h | 43 ------------------------------------- arch/riscv/lib/andes_plic.c | 34 +++++++++++++++++++----------- arch/riscv/lib/sbi_ipi.c | 5 ----- arch/riscv/lib/sifive_clint.c | 33 +++++++++++++++++++---------- arch/riscv/lib/smp.c | 49 ++++++++++++++++++++++++++++++++++++------- common/spl/spl_opensbi.c | 5 ----- 7 files changed, 86 insertions(+), 89 deletions(-)
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index bbd6c15..d8b98ad 100644 --- a/arch/riscv/cpu/cpu.c +++ b/arch/riscv/cpu/cpu.c @@ -107,12 +107,6 @@ int arch_cpu_init_dm(void) #endif }
-#ifdef CONFIG_SMP - ret = riscv_init_ipi(); - if (ret) - return ret; -#endif - return 0; }
diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h index 1b42885..74de92e 100644 --- a/arch/riscv/include/asm/smp.h +++ b/arch/riscv/include/asm/smp.h @@ -51,47 +51,4 @@ void handle_ipi(ulong hart); */ int smp_call_function(ulong addr, ulong arg0, ulong arg1, int wait);
-/** - * riscv_init_ipi() - Initialize inter-process interrupt (IPI) driver - * - * Platform code must provide this function. This function is called once after - * the cpu driver is initialized. No other riscv_*_ipi() calls will be made - * before this function is called. - * - * @return 0 if OK, -ve on error - */ -int riscv_init_ipi(void); - -/** - * riscv_send_ipi() - Send inter-processor interrupt (IPI) - * - * Platform code must provide this function. - * - * @hart: Hart ID of receiving hart - * @return 0 if OK, -ve on error - */ -int riscv_send_ipi(int hart); - -/** - * riscv_clear_ipi() - Clear inter-processor interrupt (IPI) - * - * Platform code must provide this function. - * - * @hart: Hart ID of hart to be cleared - * @return 0 if OK, -ve on error - */ -int riscv_clear_ipi(int hart); - -/** - * riscv_get_ipi() - Get status of inter-processor interrupt (IPI) - * - * Platform code must provide this function. - * - * @hart: Hart ID of hart to be checked - * @pending: Pointer to variable with result of the check, - * 1 if IPI is pending, 0 otherwise - * @return 0 if OK, -ve on error - */ -int riscv_get_ipi(int hart, int *pending); - #endif diff --git a/arch/riscv/lib/andes_plic.c b/arch/riscv/lib/andes_plic.c index 5cf29df..20529ab 100644 --- a/arch/riscv/lib/andes_plic.c +++ b/arch/riscv/lib/andes_plic.c @@ -30,6 +30,20 @@ #define SEND_IPI_TO_HART(hart) (0x80 >> (hart))
DECLARE_GLOBAL_DATA_PTR; +static int init_plic(void); + +#define PLIC_BASE_GET(void) \ + do { \ + long *ret; \ + \ + if (!gd->arch.plic) { \ + ret = syscon_get_first_range(RISCV_SYSCON_PLIC); \ + if (IS_ERR(ret)) \ + return PTR_ERR(ret); \ + gd->arch.plic = ret; \ + init_plic(); \ + } \ + } while (0)
static int enable_ipi(int hart) { @@ -79,21 +93,13 @@ static int init_plic(void) return -ENODEV; }
-int riscv_init_ipi(void) -{ - long *ret = syscon_get_first_range(RISCV_SYSCON_PLIC); - - if (IS_ERR(ret)) - return PTR_ERR(ret); - gd->arch.plic = ret; - - return init_plic(); -} - int riscv_send_ipi(int hart) { - unsigned int ipi = (SEND_IPI_TO_HART(hart) << (8 * gd->arch.boot_hart)); + unsigned int ipi; + + PLIC_BASE_GET();
+ ipi = (SEND_IPI_TO_HART(hart) << (8 * gd->arch.boot_hart)); writel(ipi, (void __iomem *)PENDING_REG(gd->arch.plic, gd->arch.boot_hart));
@@ -104,6 +110,8 @@ int riscv_clear_ipi(int hart) { u32 source_id;
+ PLIC_BASE_GET(); + source_id = readl((void __iomem *)CLAIM_REG(gd->arch.plic, hart)); writel(source_id, (void __iomem *)CLAIM_REG(gd->arch.plic, hart));
@@ -112,6 +120,8 @@ int riscv_clear_ipi(int hart)
int riscv_get_ipi(int hart, int *pending) { + PLIC_BASE_GET(); + *pending = readl((void __iomem *)PENDING_REG(gd->arch.plic, gd->arch.boot_hart)); *pending = !!(*pending & SEND_IPI_TO_HART(hart)); diff --git a/arch/riscv/lib/sbi_ipi.c b/arch/riscv/lib/sbi_ipi.c index d02e2b4..abafca9 100644 --- a/arch/riscv/lib/sbi_ipi.c +++ b/arch/riscv/lib/sbi_ipi.c @@ -8,11 +8,6 @@ #include <asm/encoding.h> #include <asm/sbi.h>
-int riscv_init_ipi(void) -{ - return 0; -} - int riscv_send_ipi(int hart) { ulong mask; diff --git a/arch/riscv/lib/sifive_clint.c b/arch/riscv/lib/sifive_clint.c index 78fc6c8..5e0d257 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,24 +47,17 @@ int riscv_get_time(u64 *time)
int riscv_set_timecmp(int hart, u64 cmp) { - writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart)); - - return 0; -} - -int riscv_init_ipi(void) -{ - long *ret = syscon_get_first_range(RISCV_SYSCON_CLINT); + CLINT_BASE_GET();
- if (IS_ERR(ret)) - return PTR_ERR(ret); - gd->arch.clint = ret; + writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart));
return 0; }
int riscv_send_ipi(int hart) { + CLINT_BASE_GET(); + writel(1, (void __iomem *)MSIP_REG(gd->arch.clint, hart));
return 0; @@ -58,6 +65,8 @@ int riscv_send_ipi(int hart)
int riscv_clear_ipi(int hart) { + CLINT_BASE_GET(); + writel(0, (void __iomem *)MSIP_REG(gd->arch.clint, hart));
return 0; @@ -65,6 +74,8 @@ int riscv_clear_ipi(int hart)
int riscv_get_ipi(int hart, int *pending) { + CLINT_BASE_GET(); + *pending = readl((void __iomem *)MSIP_REG(gd->arch.clint, hart));
return 0; diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c index ac22136..17adb35 100644 --- a/arch/riscv/lib/smp.c +++ b/arch/riscv/lib/smp.c @@ -12,6 +12,38 @@
DECLARE_GLOBAL_DATA_PTR;
+/** + * riscv_send_ipi() - Send inter-processor interrupt (IPI) + * + * Platform code must provide this function. + * + * @hart: Hart ID of receiving hart + * @return 0 if OK, -ve on error + */ +extern int riscv_send_ipi(int hart); + +/** + * riscv_clear_ipi() - Clear inter-processor interrupt (IPI) + * + * Platform code must provide this function. + * + * @hart: Hart ID of hart to be cleared + * @return 0 if OK, -ve on error + */ +extern int riscv_clear_ipi(int hart); + +/** + * riscv_get_ipi() - Get status of inter-processor interrupt (IPI) + * + * Platform code must provide this function. + * + * @hart: Hart ID of hart to be checked + * @pending: Pointer to variable with result of the check, + * 1 if IPI is pending, 0 otherwise + * @return 0 if OK, -ve on error + */ +extern int riscv_get_ipi(int hart, int *pending); + static int send_ipi_many(struct ipi_data *ipi, int wait) { ofnode node, cpus; @@ -92,7 +124,7 @@ void handle_ipi(ulong hart) */ ret = riscv_clear_ipi(hart); if (ret) { - pr_err("Cannot clear IPI of hart %ld (error %d)\n", hart, ret); + pr_err("Cannot clear IPI of hart %ld\n", hart); return; }
@@ -101,11 +133,14 @@ void handle_ipi(ulong hart)
int smp_call_function(ulong addr, ulong arg0, ulong arg1, int wait) { - struct ipi_data ipi = { - .addr = addr, - .arg0 = arg0, - .arg1 = arg1, - }; + int ret = 0; + struct ipi_data ipi; + + ipi.addr = addr; + ipi.arg0 = arg0; + ipi.arg1 = arg1; + + ret = send_ipi_many(&ipi, wait);
- return send_ipi_many(&ipi, wait); + return ret; } diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c index 3440bc0..14f335f 100644 --- a/common/spl/spl_opensbi.c +++ b/common/spl/spl_opensbi.c @@ -79,11 +79,6 @@ void spl_invoke_opensbi(struct spl_image_info *spl_image) invalidate_icache_all();
#ifdef CONFIG_SPL_SMP - /* Initialize the IPI before we use it */ - ret = riscv_init_ipi(); - if (ret) - hang(); - /* * Start OpenSBI on all secondary harts and wait for acknowledgment. *
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; }
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, };
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
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.
*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.
That sounds like a good idea.
But even considering SPL, we should also consider UP as well because timer is unconditionally needed regardless of UP/SMP.
Yeah, it looks like this patch will not work.
--Sean
[1] https://patchwork.kernel.org/patch/11563003/ (there is a v2 of this patch, but this one has some relevant commentary)
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
participants (2)
-
Bin Meng -
Sean Anderson