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

Vikas Manocha vikas.manocha at st.com
Fri Mar 17 19:06:12 UTC 2017 

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

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

> 
> [...]
>

More information about the U-Boot mailing list