From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Mon, 10 Jun 2019 10:32:13 +0800 Subject: [U-Boot] [PATCH 1/6] dm: cache: add v5l2 cache controller driver In-Reply-To: References: <20190528093914.4672-1-uboot@andestech.com> <20190528093914.4672-2-uboot@andestech.com> <4f596c121c216a5a5a7a34eac411ce993a4997c7.camel@aisec.fraunhofer.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Hi Rick, On Mon, Jun 10, 2019 at 10:26 AM Rick Chen wrote: > > Hi Lukas > > > > > Hi Rick, > > > > On Wed, 2019-06-05 at 16:58 +0800, Rick Chen wrote: > > > Hi Bin > > > > > > Bin Meng 於 2019年6月4日 週二 上午10:48寫道： > > > > Hi Rick, > > > > > > > > On Tue, May 28, 2019 at 5:44 PM Andes wrote: > > > > > From: Rick Chen > > > > > > > > > > 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 > > > > > Cc: Greentime Hu > > > > > --- > > > > > 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 > > > > > + */ > > > > > + > > > > > +#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 > > > > > + */ > > > > > + > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > + > > > > > +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. :) > > You are right. > I will modify it as compatible = "v5l2cache" Before you do that, could you please check whether this L2 cache driver will be supported in the Linux kernel too? If yes, I think you need go through the kernel upstream process, during which the compatible string is to be discussed and approved. Regards, Bin