Re: [U-Boot] [PATCH v3 1/7] riscv: Add a SYSCON driver for Andestech's PLIC
Hi Lukas
Auer, Lukas lukas.auer@aisec.fraunhofer.de 於 2019年4月1日 週一 下午5:08寫道：
Hi Rick,
On Mon, 2019-04-01 at 16:24 +0800, Andes wrote:
From: Rick Chen rick@andestech.com
The Platform-Level Interrupt Controller (PLIC) block holds memory-mapped claim and pending registers associated with software interrupt. It is required for handling IPI.
Signed-off-by: Rick Chen rick@andestech.com Cc: Greentime Hu greentime@andestech.com
V3:
- Rename plic_init() as enable_ipi().
- Declase as static.
- Remove PLIC_BASE_GET() from enable_ipi().
Take a look at patman [1], it makes it really easy to handle different versions of a patch series. :)
OK Thanks :)
arch/riscv/Kconfig | 9 +++ arch/riscv/include/asm/global_data.h | 3 + arch/riscv/include/asm/syscon.h | 2 +- arch/riscv/lib/Makefile | 1 + arch/riscv/lib/andes_plic.c | 110 +++++++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 arch/riscv/lib/andes_plic.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 3a4470d..511768b 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -109,6 +109,15 @@ config SIFIVE_CLINT The SiFive CLINT block holds memory-mapped control and status registers associated with software and timer interrupts.
+config ANDES_PLIC
booldepends on RISCV_MMODEselect REGMAPselect SYSCONhelpThe Andes PLIC block holds memory-mapped claim and pending registersassociated with software interrupt.config RISCV_RDTIME bool default y if RISCV_SMODE diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 80e3165..b867910 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -18,6 +18,9 @@ struct arch_global_data { #ifdef CONFIG_SIFIVE_CLINT void __iomem *clint; /* clint base address */ #endif +#ifdef CONFIG_ANDES_PLIC
void __iomem *plic; /* plic base address */+#endif #ifdef CONFIG_SMP struct ipi_data ipi[CONFIG_NR_CPUS]; #endif diff --git a/arch/riscv/include/asm/syscon.h b/arch/riscv/include/asm/syscon.h index d311ee6..c1b4b86 100644 --- a/arch/riscv/include/asm/syscon.h +++ b/arch/riscv/include/asm/syscon.h @@ -9,11 +9,11 @@ /*
- System controllers in a RISC-V system
nit: can you also drop the empty comment line above?
OK
*/
- So far only SiFive's Core Local Interruptor (CLINT) is defined.
enum { RISCV_NONE, RISCV_SYSCON_CLINT, /* Core Local Interruptor (CLINT) */
RISCV_SYSCON_PLIC, /* Platform Level Interrupt Controller (PLIC) */};
#endif /* _ASM_SYSCON_H */ diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 35dbf64..1bf554b 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o obj-$(CONFIG_RISCV_RDTIME) += rdtime.o obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o +obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-y += interrupts.o obj-y += reset.o obj-$(CONFIG_SBI_IPI) += sbi_ipi.o diff --git a/arch/riscv/lib/andes_plic.c b/arch/riscv/lib/andes_plic.c new file mode 100644 index 0000000..67ab561 --- /dev/null +++ b/arch/riscv/lib/andes_plic.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2019, Rick Chen rick@andestech.com
- U-Boot syscon driver for Andes's Platform Level Interrupt Controller (PLIC).
- The PLIC block holds memory-mapped claim and pending registers
- associated with software interrupt.
- */
+#include <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/lists.h> +#include <dm/uclass-internal.h> +#include <regmap.h> +#include <syscon.h> +#include <asm/io.h> +#include <asm/syscon.h> +#include <cpu.h>
+/* pending register */ +#define PENDING_REG(base, hart) ((ulong)(base) + 0x1000 + (hart) * 8) +/* enable register */ +#define ENABLE_REG(base, hart) ((ulong)(base) + 0x2000 + (hart) * 0x80) +/* claim register */ +#define CLAIM_REG(base, hart) ((ulong)(base) + 0x200004 + (hart) * 0x1000)
+#define ENABLE_HART_IPI (0x80808080) +#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 harts) +{
int i;int en = ENABLE_HART_IPI;for (i = 0; i < harts ;i++){en = en >> i;writel(en, (void __iomem *)ENABLE_REG(gd->arch.plic, i));}return 0;+}
+static int init_plic(void) +{
struct udevice *dev;int ret;ret = uclass_find_first_device(UCLASS_CPU, &dev);if (ret)return ret;if (ret == 0 && dev != NULL) {ret = cpu_get_count(dev);You should check first, if cpu_get_count() has returned an error before using its return value.
OK I will check the return value from cpu_get_count().
enable_ipi(ret);return 0;}return -ENODEV;+}
+int riscv_send_ipi(int hart) +{
PLIC_BASE_GET();writel(SEND_IPI_TO_HART(hart), (void __iomem *)PENDING_REG(gd->arch.plic, gd->arch.boot_hart));return 0;+}
+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));return 0;+}
+static const struct udevice_id andes_plic_ids[] = {
{ .compatible = "riscv,plic1", .data = RISCV_SYSCON_PLIC },Would a compatible string of "andes,plic1" be more suitable?
I got and sync the dts from our kernel team. If I shall change the string, I need to discuss with them. Some of them are not in office this week. So I prefer not change in this series.
{ }+};
+U_BOOT_DRIVER(nds_plic) = {
nit: andes_plic
OK
.name = "andes_plic",
.id = UCLASS_SYSCON,.of_match = andes_plic_ids,.flags = DM_FLAG_PRE_RELOC,+};
There are a couple of checkpatch issues in this patch. I have copied the relevant ones below.
I will check patchs in the next patchsets
Thanks Rick
CHECK: spaces preferred around that '>>' (ctx:VxV) #135: FILE: arch/riscv/lib/andes_plic.c:29: +#define SEND_IPI_TO_HART(hart) (0x80>>hart)
ERROR: that open brace { should be on the previous line #158: FILE: arch/riscv/lib/andes_plic.c:52:
for (i = 0; i < harts ;i++){ERROR: space required after that ';' (ctx:WxV) #158: FILE: arch/riscv/lib/andes_plic.c:52:
for (i = 0; i < harts ;i++)CHECK: Comparison to NULL could be written "dev" #176: FILE: arch/riscv/lib/andes_plic.c:70:
if (ret == 0 && dev != NULL) {WARNING: line over 80 characters #189: FILE: arch/riscv/lib/andes_plic.c:83:
writel(SEND_IPI_TO_HART(hart), (void __iomem *)PENDING_REG(gd-arch.plic, gd->arch.boot_hart));
Thanks, Lukas
Hi Rick,
On Tue, 2019-04-02 at 10:12 +0800, Rick Chen wrote:
Hi Lukas
Auer, Lukas lukas.auer@aisec.fraunhofer.de 於 2019年4月1日 週一 下午5:08寫道：
Hi Rick,
On Mon, 2019-04-01 at 16:24 +0800, Andes wrote:
From: Rick Chen rick@andestech.com
The Platform-Level Interrupt Controller (PLIC) block holds memory-mapped claim and pending registers associated with software interrupt. It is required for handling IPI.
Signed-off-by: Rick Chen rick@andestech.com Cc: Greentime Hu greentime@andestech.com
V3:
- Rename plic_init() as enable_ipi().
- Declase as static.
- Remove PLIC_BASE_GET() from enable_ipi().
Take a look at patman [1], it makes it really easy to handle different versions of a patch series. :)
OK Thanks :)
arch/riscv/Kconfig | 9 +++ arch/riscv/include/asm/global_data.h | 3 + arch/riscv/include/asm/syscon.h | 2 +- arch/riscv/lib/Makefile | 1 + arch/riscv/lib/andes_plic.c | 110 +++++++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 arch/riscv/lib/andes_plic.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 3a4470d..511768b 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -109,6 +109,15 @@ config SIFIVE_CLINT The SiFive CLINT block holds memory-mapped control and status registers associated with software and timer interrupts.
+config ANDES_PLIC
booldepends on RISCV_MMODEselect REGMAPselect SYSCONhelpThe Andes PLIC block holds memory-mapped claim and pending registersassociated with software interrupt.config RISCV_RDTIME bool default y if RISCV_SMODE diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 80e3165..b867910 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -18,6 +18,9 @@ struct arch_global_data { #ifdef CONFIG_SIFIVE_CLINT void __iomem *clint; /* clint base address */ #endif +#ifdef CONFIG_ANDES_PLIC
void __iomem *plic; /* plic base address */+#endif #ifdef CONFIG_SMP struct ipi_data ipi[CONFIG_NR_CPUS]; #endif diff --git a/arch/riscv/include/asm/syscon.h b/arch/riscv/include/asm/syscon.h index d311ee6..c1b4b86 100644 --- a/arch/riscv/include/asm/syscon.h +++ b/arch/riscv/include/asm/syscon.h @@ -9,11 +9,11 @@ /*
- System controllers in a RISC-V system
nit: can you also drop the empty comment line above?
OK
*/
- So far only SiFive's Core Local Interruptor (CLINT) is defined.
enum { RISCV_NONE, RISCV_SYSCON_CLINT, /* Core Local Interruptor (CLINT) */
RISCV_SYSCON_PLIC, /* Platform Level Interrupt Controller (PLIC) */};
#endif /* _ASM_SYSCON_H */ diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 35dbf64..1bf554b 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_CMD_GO) += boot.o obj-y += cache.o obj-$(CONFIG_RISCV_RDTIME) += rdtime.o obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o +obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-y += interrupts.o obj-y += reset.o obj-$(CONFIG_SBI_IPI) += sbi_ipi.o diff --git a/arch/riscv/lib/andes_plic.c b/arch/riscv/lib/andes_plic.c new file mode 100644 index 0000000..67ab561 --- /dev/null +++ b/arch/riscv/lib/andes_plic.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2019, Rick Chen rick@andestech.com
- U-Boot syscon driver for Andes's Platform Level Interrupt Controller (PLIC).
- The PLIC block holds memory-mapped claim and pending registers
- associated with software interrupt.
- */
+#include <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/lists.h> +#include <dm/uclass-internal.h> +#include <regmap.h> +#include <syscon.h> +#include <asm/io.h> +#include <asm/syscon.h> +#include <cpu.h>
+/* pending register */ +#define PENDING_REG(base, hart) ((ulong)(base) + 0x1000 + (hart) * 8) +/* enable register */ +#define ENABLE_REG(base, hart) ((ulong)(base) + 0x2000 + (hart) * 0x80) +/* claim register */ +#define CLAIM_REG(base, hart) ((ulong)(base) + 0x200004 + (hart) * 0x1000)
+#define ENABLE_HART_IPI (0x80808080) +#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 harts) +{
int i;int en = ENABLE_HART_IPI;for (i = 0; i < harts ;i++){en = en >> i;writel(en, (void __iomem *)ENABLE_REG(gd->arch.plic, i));}return 0;+}
+static int init_plic(void) +{
struct udevice *dev;int ret;ret = uclass_find_first_device(UCLASS_CPU, &dev);if (ret)return ret;if (ret == 0 && dev != NULL) {ret = cpu_get_count(dev);You should check first, if cpu_get_count() has returned an error before using its return value.
OK I will check the return value from cpu_get_count().
enable_ipi(ret);return 0;}return -ENODEV;+}
+int riscv_send_ipi(int hart) +{
PLIC_BASE_GET();writel(SEND_IPI_TO_HART(hart), (void __iomem *)PENDING_REG(gd->arch.plic, gd->arch.boot_hart));return 0;+}
+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));return 0;+}
+static const struct udevice_id andes_plic_ids[] = {
{ .compatible = "riscv,plic1", .data = RISCV_SYSCON_PLIC },Would a compatible string of "andes,plic1" be more suitable?
I got and sync the dts from our kernel team. If I shall change the string, I need to discuss with them. Some of them are not in office this week. So I prefer not change in this series.
That makes sense. I think it's fine, if you sync the device tree once it is included in the upstream kernel.
Thanks, Lukas
{ }+};
+U_BOOT_DRIVER(nds_plic) = {
nit: andes_plic
OK
.name = "andes_plic",
.id = UCLASS_SYSCON,.of_match = andes_plic_ids,.flags = DM_FLAG_PRE_RELOC,+};
There are a couple of checkpatch issues in this patch. I have copied the relevant ones below.
I will check patchs in the next patchsets
Thanks Rick
CHECK: spaces preferred around that '>>' (ctx:VxV) #135: FILE: arch/riscv/lib/andes_plic.c:29: +#define SEND_IPI_TO_HART(hart) (0x80>>hart)
ERROR: that open brace { should be on the previous line #158: FILE: arch/riscv/lib/andes_plic.c:52:
for (i = 0; i < harts ;i++){ERROR: space required after that ';' (ctx:WxV) #158: FILE: arch/riscv/lib/andes_plic.c:52:
for (i = 0; i < harts ;i++)CHECK: Comparison to NULL could be written "dev" #176: FILE: arch/riscv/lib/andes_plic.c:70:
if (ret == 0 && dev != NULL) {WARNING: line over 80 characters #189: FILE: arch/riscv/lib/andes_plic.c:83:
writel(SEND_IPI_TO_HART(hart), (void __iomem *)PENDING_REG(gd-arch.plic, gd->arch.boot_hart));
Thanks, Lukas
participants (2)
-
Auer, Lukas -
Rick Chen