* [PATCH] Revert "riscv: Clean up IPI initialization code"
@ 2020-07-08 5:02 Bin Meng
2020-07-08 9:34 ` Sean Anderson
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2020-07-08 5:02 UTC (permalink / raw)
To: u-boot
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.
*
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] Revert "riscv: Clean up IPI initialization code"
2020-07-08 5:02 [PATCH] Revert "riscv: Clean up IPI initialization code" Bin Meng
@ 2020-07-08 9:34 ` Sean Anderson
2020-07-08 10:09 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Sean Anderson @ 2020-07-08 9:34 UTC (permalink / raw)
To: u-boot
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 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@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,
};
--
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;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] Revert "riscv: Clean up IPI initialization code"
2020-07-08 9:34 ` Sean Anderson
@ 2020-07-08 10:09 ` Bin Meng
2020-07-08 10:48 ` Sean Anderson
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2020-07-08 10:09 UTC (permalink / raw)
To: u-boot
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 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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Revert "riscv: Clean up IPI initialization code"
2020-07-08 10:09 ` Bin Meng
@ 2020-07-08 10:48 ` Sean Anderson
2020-07-16 2:41 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Sean Anderson @ 2020-07-08 10:48 UTC (permalink / raw)
To: u-boot
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 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@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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Revert "riscv: Clean up IPI initialization code"
2020-07-08 10:48 ` Sean Anderson
@ 2020-07-16 2:41 ` Bin Meng
0 siblings, 0 replies; 5+ messages in thread
From: Bin Meng @ 2020-07-16 2:41 UTC (permalink / raw)
To: u-boot
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 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@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 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-16 2:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-08 5:02 [PATCH] Revert "riscv: Clean up IPI initialization code" Bin Meng
2020-07-08 9:34 ` Sean Anderson
2020-07-08 10:09 ` Bin Meng
2020-07-08 10:48 ` Sean Anderson
2020-07-16 2:41 ` Bin Meng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.