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
