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

Hi Rick,

On Tue, May 28, 2019 at 5:44 PM Andes uboot@andestech.com wrote:

From: Rick Chen rick@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@andestech.com Cc: Greentime Hu greentime@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@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@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