[U-Boot] [PATCH v2 1/2] armv7m: add instruction & data cache support

Thu Mar 16 22:06:13 UTC 2017

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