[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver
Auer, Lukas
lukas.auer at aisec.fraunhofer.de
Sun Jun 9 17:56:13 UTC 2019
Hi Rick,
On Wed, 2019-06-05 at 16:58 +0800, Rick Chen wrote:
> Hi Bin
>
> Bin Meng <bmeng.cn at gmail.com> 於 2019年6月4日 週二 上午10:48寫道：
> > Hi Rick,
> >
> > On Tue, May 28, 2019 at 5:44 PM Andes <uboot at andestech.com> wrote:
> > > From: Rick Chen <rick at andestech.com>
> > >
> > > Add a v5l2 cache controller driver that is usually found on
> > > Andes RISC-V ae350 platform. It will parse the cache settings
> > > from the dtb.
> > >
> > > In this version tag and data ram control timing can be adjusted
> > > by the requirement from the dtb.
> > >
> > > Signed-off-by: Rick Chen <rick at andestech.com>
> > > Cc: Greentime Hu <greentime at andestech.com>
> > > ---
> > > arch/riscv/include/asm/global_data.h | 3 ++
> > > arch/riscv/include/asm/v5l2cache.h | 61 +++++++++++++++++++++
> > > drivers/cache/Kconfig | 9 ++++
> > > drivers/cache/Makefile | 1 +
> > > drivers/cache/cache-v5l2.c | 102 +++++++++++++++++++++++++++++++++++
> > > 5 files changed, 176 insertions(+)
> > > create mode 100644 arch/riscv/include/asm/v5l2cache.h
> > > create mode 100644 drivers/cache/cache-v5l2.c
> > >
> > > diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> > > index b74bd7e..6e52d5d 100644
> > > --- a/arch/riscv/include/asm/global_data.h
> > > +++ b/arch/riscv/include/asm/global_data.h
> > > @@ -24,6 +24,9 @@ struct arch_global_data {
> > > #ifdef CONFIG_ANDES_PLMT
> > > void __iomem *plmt; /* plmt base address */
> > > #endif
> > > +#ifdef CONFIG_V5L2_CACHE
> > > + void __iomem *v5l2; /* v5l2 base address */
> > > +#endif
> >
> > Please do not expose this to global data if it is only used inside a
> > driver. Anything that is here is for "global" usage.
>
> OK.
> I will remove it.
>
> > > #ifdef CONFIG_SMP
> > > struct ipi_data ipi[CONFIG_NR_CPUS];
> > > #endif
> > > diff --git a/arch/riscv/include/asm/v5l2cache.h b/arch/riscv/include/asm/v5l2cache.h
> > > new file mode 100644
> > > index 0000000..8ed1c6c
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/v5l2cache.h
> >
> > Please create arch/riscv/include/asm/arch-ax25/v5l2cache.h. Speaking
> > of the name, would it be more readable to name it as v5_l2cache.h? Or
> > even add more information to v5, for me, I don't know what v5 stands
> > for :)
>
> OK
> I will create arch/riscv/include/asm/arch-ax25/v5_l2cache.h
>
> > > @@ -0,0 +1,61 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * Copyright (C) 2019 Andes Technology Corporation
> > > + * Rick Chen, Andes Technology Corporation <rick at andestech.com>
> > > + */
> > > +
> > > +#ifndef _ASM_V5_L2CACHE_H
> > > +#define _ASM_V5_L2CACHE_H
> > > +
> > > +struct l2cache {
> > > + volatile u64 configure;
> >
> > checkpatch.pl will report warnings against volatile usage. I think we
> > should drop these.
>
> I know that checkpatch.pl will report this warning.
> But I still need to add volatile. It help to fix some unpredictable problem.
> Without this some driver code flows will be optimized and go wrong somehow.
>
> > > + volatile u64 control;
> > > + volatile u64 hpm0;
> > > + volatile u64 hpm1;
> > > + volatile u64 hpm2;
> > > + volatile u64 hpm3;
> > > + volatile u64 error_status;
> > > + volatile u64 ecc_error;
> > > + volatile u64 cctl_command0;
> > > + volatile u64 cctl_access_line0;
> > > + volatile u64 cctl_command1;
> > > + volatile u64 cctl_access_line1;
> > > + volatile u64 cctl_command2;
> > > + volatile u64 cctl_access_line2;
> > > + volatile u64 cctl_command3;
> > > + volatile u64 cctl_access_line4;
> > > + volatile u64 cctl_status;
> > > +};
> > > +
> > > +/* Control Register */
> > > +#define L2_ENABLE 0x1
> > > +/* prefetch */
> > > +#define IPREPETCH_OFF 3
> > > +#define DPREPETCH_OFF 5
> > > +#define IPREPETCH_MSK (3 << IPREPETCH_OFF)
> > > +#define DPREPETCH_MSK (3 << DPREPETCH_OFF)
> > > +/* tag ram */
> > > +#define TRAMOCTL_OFF 8
> > > +#define TRAMICTL_OFF 10
> > > +#define TRAMOCTL_MSK (3 << TRAMOCTL_OFF)
> > > +#define TRAMICTL_MSK BIT(TRAMICTL_OFF)
> > > +/* data ram */
> > > +#define DRAMOCTL_OFF 11
> > > +#define DRAMICTL_OFF 13
> > > +#define DRAMOCTL_MSK (3 << DRAMOCTL_OFF)
> > > +#define DRAMICTL_MSK BIT(DRAMICTL_OFF)
> > > +
> > > +/* CCTL Command Register */
> > > +#define CCTL_CMD_REG(base, hart) ((ulong)(base) + 0x40 + (hart) * 0x10)
> > > +#define L2_WBINVAL_ALL 0x12
> > > +
> > > +/* CCTL Status Register */
> > > +#define CCTL_STATUS_MSK(hart) (0xf << ((hart) * 4))
> > > +#define CCTL_STATUS_IDLE(hart) (0 << ((hart) * 4))
> > > +#define CCTL_STATUS_PROCESS(hart) (1 << ((hart) * 4))
> > > +#define CCTL_STATUS_ILLEGAL(hart) (2 << ((hart) * 4))
> > > +
> > > +void v5l2_enable(void);
> > > +void v5l2_disable(void);
> > > +
> > > +#endif /* _ASM_V5_L2CACHE_H */
> > > diff --git a/drivers/cache/Kconfig b/drivers/cache/Kconfig
> > > index 24def7a..665689a 100644
> > > --- a/drivers/cache/Kconfig
> > > +++ b/drivers/cache/Kconfig
> > > @@ -22,4 +22,13 @@ config L2X0_CACHE
> > > ARMv7(32-bit) devices. The driver configures the cache settings
> > > found in the device tree.
> > >
> > > +config V5L2_CACHE
> > > + tristate "Andes V5L2 cache driver"
> >
> > This should be bool. U-Boot does not support "tristate"
>
> I will modify it as bool.
>
> > > + select CACHE
> > > + depends on RISCV_NDS_CACHE
> > > + help
> > > + Support Andes V5L2 cache controller in AE350 platform.
> > > + It will configure tag and data ram timing control from the
> > > + device tree and enable L2 cache.
> > > +
> > > endmenu
> > > diff --git a/drivers/cache/Makefile b/drivers/cache/Makefile
> > > index 9deb961..4a6458c 100644
> > > --- a/drivers/cache/Makefile
> > > +++ b/drivers/cache/Makefile
> > > @@ -2,3 +2,4 @@
> > > obj-$(CONFIG_CACHE) += cache-uclass.o
> > > obj-$(CONFIG_SANDBOX) += sandbox_cache.o
> > > obj-$(CONFIG_L2X0_CACHE) += cache-l2x0.o
> > > +obj-$(CONFIG_V5L2_CACHE) += cache-v5l2.o
> > > diff --git a/drivers/cache/cache-v5l2.c b/drivers/cache/cache-v5l2.c
> > > new file mode 100644
> > > index 0000000..7022feb
> > > --- /dev/null
> > > +++ b/drivers/cache/cache-v5l2.c
> > > @@ -0,0 +1,102 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2019 Andes Technology Corporation
> > > + * Rick Chen, Andes Technology Corporation <rick at andestech.com>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <command.h>
> > > +#include <dm.h>
> > > +#include <asm/io.h>
> > > +#include <dm/ofnode.h>
> > > +#include <asm/v5l2cache.h>
> > > +
> > > +DECLARE_GLOBAL_DATA_PTR;
> > > +
> > > +void v5l2_enable(void)
> >
> > This should really be avoided. It looks current DM cache uclass driver
> > is lacking of ops to do cache enable/disable. Please improve the DM
> > cache uclass driver first.
>
> OK
> I will improve the DM cache uclass driver.
>
> > > +{
> > > + struct l2cache *regs = gd->arch.v5l2;
> > > +
> > > + if (regs)
> > > + setbits_le32(®s->control, L2_ENABLE);
> > > +}
> > > +
> > > +void v5l2_disable(void)
> > > +{
> > > + volatile struct l2cache *regs = gd->arch.v5l2;
> > > + u8 hart = gd->arch.boot_hart;
> > > + void __iomem *cctlcmd = (void __iomem *)CCTL_CMD_REG(gd->arch.v5l2, hart);
> > > +
> > > + if ((regs) && (readl(®s->control) & L2_ENABLE)) {
> > > + writel(L2_WBINVAL_ALL, cctlcmd);
> > > +
> > > + while ((readl(®s->cctl_status) & CCTL_STATUS_MSK(hart))) {
> > > + if ((readl(®s->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> > > + printf("L2 flush illegal! hanging...");
> > > + hang();
> > > + }
> > > + }
> > > + clrbits_le32(®s->control, L2_ENABLE);
> > > + }
> > > +}
> > > +
> > > +static void v5l2_of_parse_and_init(struct udevice *dev)
> >
> > The entire function below should really be created as the driver's
> > ofdata_to_platdata() function, inside which all DT properties are
> > parsed and saved to driver's platdata. After that, there is no need to
> > get register base from global data.
>
> I will use ofdata_to_platdata() to parse dtb information and save it
> into platdata instead of saving in global data.
>
> > > +{
> > > + struct l2cache *regs;
> > > + u32 ctl_val, prefetch;
> > > + u32 tram_ctl[2];
> > > + u32 dram_ctl[2];
> > > +
> > > + regs = (struct l2cache *)dev_read_addr(dev);
> > > + gd->arch.v5l2 = regs;
> > > + ctl_val = readl(®s->control);
> > > +
> > > + if (!(ctl_val & L2_ENABLE))
> > > + ctl_val |= L2_ENABLE;
> > > +
> > > + /* Instruction and data fetch prefetch depth */
> > > + if (!dev_read_u32(dev, "andes,inst-prefetch", &prefetch)) {
> > > + ctl_val &= ~(IPREPETCH_MSK);
> > > + ctl_val |= (prefetch << IPREPETCH_OFF);
> > > + }
> > > +
> > > + if (!dev_read_u32(dev, "andes,data-prefetch", &prefetch)) {
> > > + ctl_val &= ~(DPREPETCH_MSK);
> > > + ctl_val |= (prefetch << DPREPETCH_OFF);
> > > + }
> > > +
> > > + /* Set tag RAM and data RAM setup and output cycle */
> > > + if (!dev_read_u32_array(dev, "andes,tag-ram-ctl", tram_ctl, 2)) {
> > > + ctl_val &= ~(TRAMOCTL_MSK | TRAMICTL_MSK);
> > > + ctl_val |= tram_ctl[0] << TRAMOCTL_OFF;
> > > + ctl_val |= tram_ctl[1] << TRAMICTL_OFF;
> > > + }
> > > +
> > > + if (!dev_read_u32_array(dev, "andes,data-ram-ctl", dram_ctl, 2)) {
> > > + ctl_val &= ~(DRAMOCTL_MSK | DRAMICTL_MSK);
> > > + ctl_val |= dram_ctl[0] << DRAMOCTL_OFF;
> > > + ctl_val |= dram_ctl[1] << DRAMICTL_OFF;
> > > + }
> > > +
> > > + writel(ctl_val, ®s->control);
> > > +}
> > > +
> > > +static int v5l2_probe(struct udevice *dev)
> > > +{
> > > + v5l2_of_parse_and_init(dev);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct udevice_id v5l2_cache_ids[] = {
> > > + { .compatible = "cache" },
> >
> > I suspect this compatible string is too generic. Has this been
> > reviewed by DT community upstream?
> >
>
> It refer to many dts examples from arm,
> For example :
> arch/arm/dts/fsl-imx8-ca35.dtsi
> A35_L2: l2-cache0 {
> compatible = "cache";
> };
>
None of these have a driver for the cache controller, which is why it
is sufficient to just use a generic compatible string.
I agree with Bin that you should choose a more specific compatible
string. This is likely to cause problems in the future otherwise. For
example, if Andes develops a new L2 cache controller, it must be
differentiated from this one using the compatible string. The new
controller would therefore have to use a different compatible string.
It is good practice to already use a more specific one now to avoid the
headache later. :)
Thanks,
Lukas
More information about the U-Boot
mailing list