Thanks Marek,
On 03/11/2017 10:02 PM, Marek Vasut wrote:
> On 03/12/2017 01:13 AM, Vikas Manocha wrote:
>> This patch adds armv7m instruction & data cache support.
>>
>> Signed-off-by: Vikas Manocha <vikas.mano...@st.com>
>> ---
>>
>> Changed in v2:
>> - changed strucures for memory mapped cache registers to MACROs
>
> Macro is written in lowercase, FYI ...
ok.
>
>> - added lines better readability.
>> - replaced magic numbers with MACROs.
>>
>> arch/arm/cpu/armv7m/Makefile | 2 +-
>> arch/arm/cpu/armv7m/cache.c | 294
>> ++++++++++++++++++++++++++++++++++++++++++
>> arch/arm/include/asm/armv7m.h | 26 +++-
>> arch/arm/lib/Makefile | 2 +
>> 4 files changed, 321 insertions(+), 3 deletions(-)
>> create mode 100644 arch/arm/cpu/armv7m/cache.c
>>
>> diff --git a/arch/arm/cpu/armv7m/Makefile b/arch/arm/cpu/armv7m/Makefile
>> index aff60e8..41efe11 100644
>> --- a/arch/arm/cpu/armv7m/Makefile
>> +++ b/arch/arm/cpu/armv7m/Makefile
>> @@ -6,4 +6,4 @@
>> #
>>
>> extra-y := start.o
>> -obj-y += cpu.o
>> +obj-y += cpu.o cache.o
>> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
>> new file mode 100644
>> index 0000000..cc17366
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7m/cache.c
>> @@ -0,0 +1,294 @@
>> +/*
>> + * (C) Copyright 2017
>> + * Vikas Manocha, ST Micoelectronics, vikas.mano...@st.com.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/armv7m.h>
>> +#include <asm/io.h>
>> +#include <errno.h>
>> +
>> +/* Cache maintenance operation registers */
>> +
>> +#define IC_IALLU 0x00
>> +#define INVAL_ICACHE_POU 0
>> +
>> +#define IC_IMVALU 0x08
>> +#define DC_IMVAC 0x0C
>> +#define DC_ISW 0x10
>> +#define DC_CMVAU 0x14
>> +#define DC_CMVAC 0x18
>> +#define DC_CSW 0x1C
>> +#define DC_CIMVAC 0x20
>> +#define DC_CISW 0x24
>
> Would be nice to have some more distinguishing name here, so one can
> easily git grep for those reg names and make sense of their name without
> reading the datasheet .
these names are consistent with the arch manual to help relating them with
manual.
>
>> +#define WAYS_SHIFT 30
>> +#define SETS_SHIFT 5
>
> Is this always 30 and 5 , on all CPUs ?
Yes for all armv7m arch.
>
>> +/* armv7m processor feature registers */
>> +
>> +#define CLIDR 0x00
>> +#define CTR 0x04
>> +
>> +#define CCSIDR 0x08
>> +#define MASK_NUM_WAYS GENMASK(12, 3)
>> +#define MASK_NUM_SETS GENMASK(27, 13)
>> +#define NUM_WAYS_SHIFT 3
>> +#define NUM_SETS_SHIFT 13
>> +
>> +#define CSSELR 0x0C
>> +#define SEL_I_OR_D BIT(0)
>> +
>> +void *const v7m_cache_maint = (void *)V7M_CACHE_MAINT_BASE;
>> +void *const v7m_processor = (void *)V7M_PROC_FTR_BASE;
>
> Needed ? Why don't you just use the macro directly ?
Yes it is possible. I was trying to avoid typecasting macro to pointer each
time before passing to
functions required it as address pointer.
>
>> +
>> +
>> +enum cache_type {
>> + DCACHE = 0,
>> + ICACHE,
>> +};
>> +
>> +/* PoU : Point of Unification, Poc: Point of Coherency */
>> +enum cache_action {
>> + INVALIDATE_POU, /* for i-cache invalidate by address */
>> + INVALIDATE_POC, /* for d-cache invalidate by address */
>> + INVALIDATE_SET_WAY, /* for d-cache invalidate by sets/ways */
>> + FLUSH_POU,
>> + FLUSH_POC,
>> + FLUSH_SET_WAY,
>> + FLUSH_INVAL_POC,
>> + FLUSH_INVAL_SET_WAY,
>> +};
>> +
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>> +struct dcache_config {
>> + uint32_t ways;
>> + uint32_t sets;
>
> u32 , this is not userspace ...
ok, i will change all.
>
>> +};
>> +
>> +static void get_cache_ways_sets(struct dcache_config *cache)
>> +{
>> + cache->ways = (readl(v7m_processor + CCSIDR) & MASK_NUM_WAYS)
>> + >> NUM_WAYS_SHIFT;
>> + cache->sets = (readl(v7m_processor + CCSIDR) & MASK_NUM_SETS)
>> + >> NUM_SETS_SHIFT;
>
> Do you need to read the reg twice ?
Good point, we can read once in temp & find out ways & sets.
>
>> +}
>> +
>> +static uint32_t *get_action_reg_set_ways(enum cache_action action)
>> +{
>> + switch (action) {
>> + case INVALIDATE_SET_WAY:
>> + return v7m_cache_maint + DC_ISW;
>> + case FLUSH_SET_WAY:
>> + return v7m_cache_maint + DC_CSW;
>> + case FLUSH_INVAL_SET_WAY:
>> + return v7m_cache_maint + DC_CISW;
>> + default:
>> + break;
>> + };
>> +
>> + return NULL;
>> +}
>> +
>> +static uint32_t *get_action_reg_range(enum cache_action action)
>> +{
>> + switch (action) {
>> + case INVALIDATE_POU:
>> + return v7m_cache_maint + IC_IMVALU;
>> + case INVALIDATE_POC:
>> + return v7m_cache_maint + DC_IMVAC;
>> + case FLUSH_POU:
>> + return v7m_cache_maint + DC_CMVAU;
>> + case FLUSH_POC:
>> + return v7m_cache_maint + DC_CMVAC;
>> + case FLUSH_INVAL_POC:
>> + return v7m_cache_maint + DC_CIMVAC;
>> + default:
>> + break;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static uint32_t get_cline_size(enum cache_type type)
>> +{
>> + uint32_t size;
>> +
>> + if (type == DCACHE)
>> + clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));
>> + else if (type == ICACHE)
>> + setbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));
>> + dsb();
>> +
>> + size = readl(v7m_processor + CCSIDR) & GENMASK(2, 0);
>
> define the mask as some macro ....
ok
>
>> + size = size * 4 + 4; /* 0 means 4, 1 means 8 & so on */
>
> 2 means 12 or 16 ? The comment is useless ...
>
> Is the size basically 1 << (size + 2) ?
agree, I will add better comment in v3.
>
>> + debug("cache line size is %d\n", size);
>> +
>> + return size;
>> +}
>> +
>> +int action_cache_range(enum cache_action action, uint32_t start_addr,
>> + int64_t size)
>
> static ?
this function at present is not being used as we are invalidating/flushing all
cache but helper function
to flush/invalidate parts/range of cache.
Making it static leads to "function not used" compilation warning. attribute
"unused" can be used also
but not sure...
Please suggest.
>
> You're never checking if start_addr and size are cache-line aligned ,
> see arm926ejs and armv7a
>
>> +{
>> + uint32_t cline_size;
>> + uint32_t *action_reg;
>
> u32 , fix globally
>
>> + enum cache_type type;
>> +
>> + action_reg = get_action_reg_range(action);
>> + if (!action_reg)
>> + return -EINVAL;
>> + if (action == INVALIDATE_POU)
>> + type = ICACHE;
>> + else
>> + type = DCACHE;
>> +
>> + /* cache line size is minium size for the cache action */
>> + cline_size = get_cline_size(type);
>> + do {
>> + writel(start_addr, action_reg);
>> + size -= cline_size;
>> + start_addr += cline_size;
>> + } while (size > cline_size);
>> + debug("cache action on range done\n");
>> + dsb();
>> + isb();
>> +
>> + return 0;
>> +}
>> +
>> +static int action_dcache_all(enum cache_action action)
>> +{
>> + struct dcache_config cache;
>> + uint32_t *action_reg;
>> + int i, j;
>> +
>> + action_reg = get_action_reg_set_ways(action);
>> + if (!action_reg)
>> + return -EINVAL;
>> +
>> + clrbits_le32(v7m_processor + CSSELR, BIT(SEL_I_OR_D));
>> + dsb();
>
> Needed ?
Yes to make cache selection effective.
>
>> + get_cache_ways_sets(&cache); /* Get number of ways & sets */
>> + debug("cache: ways= %d, sets= %d\n", cache.ways + 1, cache.sets + 1);
>> + for (i = cache.sets; i >= 0; i--) {
>> + for (j = cache.ways; j >= 0; j--) {
>> + writel((j << WAYS_SHIFT) | (i << SETS_SHIFT),
>> + action_reg);
>> + }
>> + }
>> + dsb();
>> + isb();
>
> Are all those barriers needed ?
Yes, to make the write effective & flush the pipeline.
>
>> + return 0;
>> +}
>> +
>> +void dcache_enable(void)
>> +{
>> + if (dcache_status()) /* return if cache already enabled */
>> + return;
>> +
>> + if (action_dcache_all(INVALIDATE_SET_WAY)) {
>> + printf("ERR: D-cache not enabled\n");
>> + return;
>> + }
>> +
>> + setbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_DCACHE));
>> + dsb();
>> + isb();
>> +}
>> +
>> +void dcache_disable(void)
>> +{
>> + /* if dcache is enabled-> dcache disable & then flush */
>> + if (dcache_status()) {
>
> Invert the condition here ...
not sure if it is helpful to check "if cache is disabled" instead of present
test of "if cache is enabled" ?
>
>> + if (action_dcache_all(FLUSH_SET_WAY)) {
>> + printf("ERR: D-cahe not flushed\n");
>> + return;
>> + }
>> +
>> + clrbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_DCACHE));
>> + dsb();
>> + isb();
>> + }
>> +}
>> +
>> +int dcache_status(void)
>> +{
>> + return (readl(&V7M_SCB->ccr) & BIT(V7M_CCR_DCACHE)) != 0;
>> +}
>> +
>> +#else
>> +void dcache_enable(void)
>> +{
>> + return;
>> +}
>> +
>> +void dcache_disable(void)
>> +{
>> + return;
>> +}
>> +
>> +int dcache_status(void)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +#ifndef CONFIG_SYS_ICACHE_OFF
>> +
>> +void invalidate_icache_all(void)
>> +{
>> + writel(INVAL_ICACHE_POU, v7m_cache_maint + IC_IALLU);
>> + dsb();
>> + isb();
>> +}
>> +
>> +void icache_enable(void)
>> +{
>> + if (icache_status())
>> + return;
>> +
>> + invalidate_icache_all();
>> + setbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_ICACHE));
>> + dsb();
>> + isb();
>> +}
>> +
>> +int icache_status(void)
>> +{
>> + return (readl(&V7M_SCB->ccr) & BIT(V7M_CCR_ICACHE)) != 0;
>> +}
>> +
>> +void icache_disable(void)
>> +{
>> + isb();
>> + clrbits_le32(&V7M_SCB->ccr, BIT(V7M_CCR_ICACHE));
>> + isb();
>> +}
>> +#else
>> +void icache_enable(void)
>> +{
>> + return;
>> +}
>> +
>> +void icache_disable(void)
>> +{
>> + return;
>> +}
>> +
>> +int icache_status(void)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +void enable_caches(void)
>> +{
>> +#ifndef CONFIG_SYS_ICACHE_OFF
>> + icache_enable();
>> +#endif
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>> + dcache_enable();
>> +#endif
>> +}
>> diff --git a/arch/arm/include/asm/armv7m.h b/arch/arm/include/asm/armv7m.h
>> index 54d8a2b..67cb0e4 100644
>> --- a/arch/arm/include/asm/armv7m.h
>> +++ b/arch/arm/include/asm/armv7m.h
>> @@ -16,8 +16,15 @@
>> .thumb
>> #endif
>>
>> -#define V7M_SCB_BASE 0xE000ED00
>> -#define V7M_MPU_BASE 0xE000ED90
>> +/* armv7m fixed base addresses */
>> +#define V7M_SCS_BASE 0xE000E000
>> +#define V7M_NVIC_BASE (V7M_SCS_BASE + 0x0100)
>> +#define V7M_SCB_BASE (V7M_SCS_BASE + 0x0D00)
>> +#define V7M_PROC_FTR_BASE (V7M_SCS_BASE + 0x0D78)
>> +#define V7M_MPU_BASE (V7M_SCS_BASE + 0x0D90)
>> +#define V7M_FPU_BASE (V7M_SCS_BASE + 0x0F30)
>> +#define V7M_CACHE_MAINT_BASE (V7M_SCS_BASE + 0x0F50)
>> +#define V7M_ACCESS_CNTL_BASE (V7M_SCS_BASE + 0x0F90)
>
> Does all this stuff need to be in global namespace ?
No. I added all armv7m architecture stuff at one place to have complete view
just like soc's peripherals
base addresses defines.
>
>> #define V7M_SCB_VTOR 0x08
>>
>> @@ -27,6 +34,18 @@ struct v7m_scb {
>> uint32_t icsr; /* Interrupt Control and State Register */
>> uint32_t vtor; /* Vector Table Offset Register */
>> uint32_t aircr; /* App Interrupt and Reset Control Register */
>> + uint32_t scr; /* offset 0x10 */
>> + uint32_t ccr; /* offset 0x14 */
>> + uint32_t shpr1; /* offset 0x18 */
>> + uint32_t shpr2; /* offset 0x1c */
>> + uint32_t shpr3; /* offset 0x20 */
>> + uint32_t shcrs; /* offset 0x24 */
>> + uint32_t cfsr; /* offset 0x28 */
>> + uint32_t hfsr; /* offset 0x2C */
>> + uint32_t res; /* offset 0x30 */
>> + uint32_t mmar; /* offset 0x34 */
>> + uint32_t bfar; /* offset 0x38 */
>> + uint32_t afsr; /* offset 0x3C */
>
> The comments are real useless compared to the previous comments in this
> block ...
They provide the cross-check of address offsets & help in adding space of
reserved area.
I will add names of registers also in v3.
Cheers,
Vikas
>
>> };
>> #define V7M_SCB ((struct v7m_scb *)V7M_SCB_BASE)
>>
>> @@ -39,6 +58,9 @@ struct v7m_scb {
>>
>> #define V7M_ICSR_VECTACT_MSK 0xFF
>>
>> +#define V7M_CCR_DCACHE 16
>> +#define V7M_CCR_ICACHE 17
>> +
>> struct v7m_mpu {
>> uint32_t type; /* Type Register */
>> uint32_t ctrl; /* Control Register */
>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
>> index 166fa9e..52b36b3 100644
>> --- a/arch/arm/lib/Makefile
>> +++ b/arch/arm/lib/Makefile
>> @@ -55,8 +55,10 @@ endif
>>
>> obj-y += cache.o
>> ifndef CONFIG_ARM64
>> +ifndef CONFIG_CPU_V7M
>> obj-y += cache-cp15.o
>> endif
>> +endif
>>
>> obj-y += psci-dt.o
>>
>>
>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
