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

Reply via email to