Thanks Marek,
On 03/16/2017 02:40 PM, Marek Vasut wrote:
> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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, [email protected].
>>>> + *
>>>> + * 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.
ok, i will make them V7M_IC_* for instruction cache & V7M_DC_* for data cache
specific regs.
>
>>>> +#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 ...
done in v3, i will send the v4 with rest of the modifications.
>
> [...]
>
>>>> + 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 ...)
ok.
>
>>>
>>> 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 ?
dsb barrier makes sure outstanding memory transactions are completed before
next instructions.
which means in this case the required cache memory is selected before following
action on sets/ways.
>
>>>
>>>> + 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 ...
dsb for same reason as above & isb to flush the instruction 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" ?
>
> It reduces indent ...
thanks, i will invert it in v4.
>
>>>
>>>> + 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 ...
done in v3, i will send v4 with rest of modifications.
Cheers,
Vikas
>
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
