Thanks Simon,
On 03/16/2017 03:06 PM, Simon Glass wrote:
> Hi Vikas,
>
> On 14 March 2017 at 11:27, Vikas Manocha <vikas.mano...@st.com> wrote:
>> This patch adds armv7m instruction & data cache support.
>>
>> Signed-off-by: Vikas Manocha <vikas.mano...@st.com>
>> cc: Christophe KERELLO <christophe.kere...@st.com>
>> ---
>>
>> Changed in v3:
>> - uint32 replcaed with u32.
>> - multiple read of hardware register replaced with single.
>> - pointers replaced with macros for base address.
>> - register names added as comment for system control block registers.
>>
>> Changed in v2:
>> - changed strucures for memory mapped cache registers to macros
>> - added lines better readability.
>> - replaced magic numbers with macros.
>>
>> arch/arm/cpu/armv7m/Makefile | 2 +-
>> arch/arm/cpu/armv7m/cache.c | 291
>> ++++++++++++++++++++++++++++++++++++++++++
>> arch/arm/include/asm/armv7m.h | 26 +++-
>> arch/arm/lib/Makefile | 2 +
>> 4 files changed, 318 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..9021525
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7m/cache.c
>> @@ -0,0 +1,291 @@
>> +/*
>> + * (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>
>
> put this one below common.h
oh yes, ok.
>
>> +
>> +/* Cache maintenance operation registers */
>> +
>> +#define IC_IALLU (V7M_CACHE_MAINT_BASE + 0x00)
>> +#define INVAL_ICACHE_POU 0
>> +#define IC_IMVALU (V7M_CACHE_MAINT_BASE + 0x08)
>> +#define DC_IMVAC (V7M_CACHE_MAINT_BASE + 0x0C)
>> +#define DC_ISW (V7M_CACHE_MAINT_BASE + 0x10)
>> +#define DC_CMVAU (V7M_CACHE_MAINT_BASE + 0x14)
>> +#define DC_CMVAC (V7M_CACHE_MAINT_BASE + 0x18)
>> +#define DC_CSW (V7M_CACHE_MAINT_BASE + 0x1C)
>> +#define DC_CIMVAC (V7M_CACHE_MAINT_BASE + 0x20)
>> +#define DC_CISW (V7M_CACHE_MAINT_BASE + 0x24)
>> +#define WAYS_SHIFT 30
>> +#define SETS_SHIFT 5
>> +
>> +/* armv7m processor feature registers */
>> +
>> +#define CLIDR (V7M_PROC_FTR_BASE + 0x00)
>> +#define CTR (V7M_PROC_FTR_BASE + 0x04)
>> +#define CCSIDR (V7M_PROC_FTR_BASE + 0x08)
>> +#define MASK_NUM_WAYS GENMASK(12, 3)
>> +#define MASK_NUM_SETS GENMASK(27, 13)
>> +#define CLINE_SIZE_MASK GENMASK(2, 0)
>> +#define NUM_WAYS_SHIFT 3
>> +#define NUM_SETS_SHIFT 13
>> +#define CSSELR (V7M_PROC_FTR_BASE + 0x0C)
>> +#define SEL_I_OR_D BIT(0)
>> +
>> +enum cache_type {
>> + DCACHE = 0,
>
> Do you need the =0 ?
No :-), thanks.
>
>> + 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,
>
> Can you add comments for the rest?
sure.
>
>> +};
>> +
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>> +struct dcache_config {
>> + u32 ways;
>> + u32 sets;
>> +};
>> +
>> +static void get_cache_ways_sets(struct dcache_config *cache)
>> +{
>> + u32 cache_size_id = readl(CCSIDR);
>
> blank line here
ok.
>
>> + cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
>> + cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
>> +}
>> +
>> +static u32 *get_action_reg_set_ways(enum cache_action action)
>
> Can you please add a function comment? What does this return?
this function returns the io register to perform required cache action like
clean or clean & invalidate
by sets/ways. The procedure to perform on these io register is same for
cleaning & clean/invalidate.
I will add the comment in code.
>
>> +{
>> + switch (action) {
>> + case INVALIDATE_SET_WAY:
>> + return (u32 *)DC_ISW;
>
> Can you drop these casts by using a C structure or by putting thecast
> in the #define?
C structures was the first choice but they were removed after v1 as per review
comment & replaced with #defines.
ok for the casting in #defines.
>
>> + case FLUSH_SET_WAY:
>> + return (u32 *)DC_CSW;
>> + case FLUSH_INVAL_SET_WAY:
>> + return (u32 *)DC_CISW;
>> + default:
>> + break;
>> + };
>> +
>> + return NULL;
>> +}
>> +
>> +static u32 *get_action_reg_range(enum cache_action action)
>> +{
>> + switch (action) {
>> + case INVALIDATE_POU:
>> + return (u32 *)IC_IMVALU;
>> + case INVALIDATE_POC:
>> + return (u32 *)DC_IMVAC;
>> + case FLUSH_POU:
>> + return (u32 *)DC_CMVAU;
>> + case FLUSH_POC:
>> + return (u32 *)DC_CMVAC;
>> + case FLUSH_INVAL_POC:
>> + return (u32 *)DC_CIMVAC;
>> + default:
>> + break;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static u32 get_cline_size(enum cache_type type)
>
> Why u32? Should it be uint or ulong?
armv7m is 32bit arch, cacheline size (32 bytes for cortex M7) can never be more
than u32.
Please let me know if i am missing something.
>
>> +{
>> + u32 size;
>> +
>> + if (type == DCACHE)
>> + clrbits_le32(CSSELR, BIT(SEL_I_OR_D));
>> + else if (type == ICACHE)
>> + setbits_le32(CSSELR, BIT(SEL_I_OR_D));
>> + dsb();
>> +
>> + size = readl(CCSIDR) & CLINE_SIZE_MASK;
>> + /* Size enocoded as 2 less than log(no_of_words_in_cache_line) base
>> 2 */
>> + size = 1 << (size + 2);
>> + debug("cache line size is %d\n", size);
>> +
>> + return size;
>> +}
>> +
>> +static __attribute__((unused)) int action_cache_range(enum cache_action
>> action,
>> + u32 start_addr, int64_t size)
>
> Function comment.
this function is to perform the required action like invalidate/clean on a
range of cache addresses.
I will add comment.
>
> Can you use __used ?
I figured these attribute will not be required after using the cache prototypes
of common.h like
invalidate_dcache_range().
>
>> +{
>> + u32 cline_size;
>> + u32 *action_reg;
>> + 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);
>> + /* Align start address to cache line boundary */
>> + start_addr &= ~(cline_size - 1);
>> + do {
>> + writel(start_addr, action_reg);
>> + size -= cline_size;
>> + start_addr += cline_size;
>> + } while (size > cline_size);
>> + dsb();
>> + isb();
>> + debug("cache action on range done\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int action_dcache_all(enum cache_action action)
>
> Function comment.
this function is to perform the required action like invalidate/clean on all
cached addresses.
I will add comment for it.
Cheers,
Vikas
>
> Regards,
> Simon
> .
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
