On 03/13/2017 10:45 PM, vikas wrote:
> 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.

Clearly, I'm not getting through here. Something like CACHE_V7M_REG_FOO
is much easier to grep for than FOO.

>>> +#define WAYS_SHIFT         30
>>> +#define SETS_SHIFT         5
>>
>> Is this always 30 and 5 , on all CPUs ?
> 
> Yes for all armv7m arch.

OK

>>> +/* 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.

Eh? This is just a value, you can use it directly ...

[...]

>>> +   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.

So basically this is a workaround to silence the compiler which
correctly warns you about dead code ? I think you know what to do (hint:
remove dead code ...)

>>
>> 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.

Effective how ?

>>
>>> +   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.

Could use better explanation / comment ...

>>
>>> +   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" ?

It reduces indent ...

>>
>>> +           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.

Most of which should come from DT, but OK ...

>>
>>>  #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.

And unlike the verbose comments above, which describe what the register
actually does, they are totally useless ...

-- 
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to