[U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver

Bin Meng bmeng.cn at gmail.com
Tue Jun 4 02:48:37 UTC 2019


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.

>  #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 :)

> @@ -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.

> +       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"

> +       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.

> +{
> +       struct l2cache *regs = gd->arch.v5l2;
> +
> +       if (regs)
> +               setbits_le32(&regs->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(&regs->control) & L2_ENABLE)) {
> +               writel(L2_WBINVAL_ALL, cctlcmd);
> +
> +               while ((readl(&regs->cctl_status) & CCTL_STATUS_MSK(hart))) {
> +                       if ((readl(&regs->cctl_status) & CCTL_STATUS_ILLEGAL(hart))) {
> +                               printf("L2 flush illegal! hanging...");
> +                               hang();
> +                       }
> +               }
> +               clrbits_le32(&regs->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.

> +{
> +       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(&regs->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, &regs->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?

> +       {}
> +};
> +
> +U_BOOT_DRIVER(v5l2_cache) = {
> +       .name   = "v5l2_cache",
> +       .id     = UCLASS_CACHE,
> +       .of_match = v5l2_cache_ids,
> +       .probe  = v5l2_probe,
> +       .flags  = DM_FLAG_PRE_RELOC,
> +};
> --

Regards,
Bin


More information about the U-Boot mailing list