Hi Marek,
On 03/16/2017 03:06 PM, Marek Vasut wrote:
> 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 ...
ok, will make them V7M_CACHE_REG_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 ...
>>
>> 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.
ok.
>
>>> [...]
>>>
>>>>>> + 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 ...)
Ah, A different prototype is there in common.h for cache range support like
invalidate_dcache_range().
I will correct it in next version.
>>
>> 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 ?
I don't think so, esp as per cache requirements. armv7 cache driver is also
using like this.
>
> This should be in a comment in the code anyway ...
sure, i will add the comment.
Cheers,
Vikas
>
> [...]
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
