On 03/16/2017 10:39 PM, vikas wrote:
> 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 <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.
>
> ok, i will make them V7M_IC_* for instruction cache & V7M_DC_* for data cache
> specific regs.
Which is still pretty cryptic ...
>>>>> +#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.
Could you give the patch a few days on the list to gather feedback ? I
believe I warned you about this before already, but the maintainers are
already saturated by patches, sending one revision after the other does
NOT help anyone and only congests the maintainers further.
>> [...]
>>
>>>>> + 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.
Shouldn't IO accessors already contain such barrier instructions too ?
This should be in a comment in the code anyway ...
[...]
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
