[U-Boot] [PATCH v3 1/7] riscv: Add a SYSCON driver for Andestech's PLIC
Auer, Lukas
lukas.auer at aisec.fraunhofer.de
Tue Apr 2 21:14:51 UTC 2019
Hi Rick,
On Tue, 2019-04-02 at 10:12 +0800, Rick Chen wrote:
> Hi Lukas
>
> > Auer, Lukas <lukas.auer at 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 at 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 at andestech.com>
> > > Cc: Greentime Hu <greentime at 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 :)
>
> > [1]: https://github.com/u-boot/u-boot/blob/master/tools/patman/README
> >
> > > 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
> > > + bool
> > > + depends on RISCV_MMODE
> > > + select REGMAP
> > > + select SYSCON
> > > + help
> > > + The Andes PLIC block holds memory-mapped claim and pending registers
> > > + associated 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 at 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
More information about the U-Boot
mailing list