Hi Marek,
On 03/27/2017 02:27 PM, Marek Vasut wrote:
> On 03/27/2017 10:41 PM, Vikas Manocha wrote:
>> Hi Marek,
>>
>> On 03/27/2017 01:34 PM, Marek Vasut wrote:
>>> On 03/27/2017 10:02 PM, Vikas Manocha wrote:
>>>> This patch adds armv7m instruction & data cache support.
>>>>
>>>> Signed-off-by: Vikas Manocha <vikas.mano...@st.com>
>>>> cc: Christophe KERELLO <christophe.kere...@st.com>
>>>> ---
>>>>
>>>> Changed in v4:
>>>> - invalidate_dcache_range() & flush_dcache_range() function added.
>>>> - blank lines added.
>>>> - comments added for registers, functions & barriers.
>>>> - register names changed for better clarity.
>>>> - typecasting moved to macro definitions.
>>>>
>>>> Changed in v3:
>>>> - uint32 replcaed with u32.
>>>> - multiple read of hardware register replaced with single.
>>>> - pointers replaced with macros for base address.
>>>> - register names added as comment for system control block registers.
>>>>
>>>> Changed in v2:
>>>> - changed strucures for memory mapped cache registers to macros
>>>> - added lines better readability.
>>>> - replaced magic numbers with macros.
>>>>
>>>> arch/arm/cpu/armv7m/Makefile | 3 +-
>>>> arch/arm/cpu/armv7m/cache.c | 336
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> arch/arm/include/asm/armv7m.h | 26 +++-
>>>> arch/arm/lib/Makefile | 2 +
>>>> 4 files changed, 363 insertions(+), 4 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 e1a6c40..93c9085 100644
>>>> --- a/arch/arm/cpu/armv7m/Makefile
>>>> +++ b/arch/arm/cpu/armv7m/Makefile
>>>> @@ -6,6 +6,5 @@
>>>> #
>>>>
>>>> extra-y := start.o
>>>> -obj-y += cpu.o
>>>> -
>>>> +obj-y += cpu.o cache.o
>>>> obj-$(CONFIG_SYS_ARCH_TIMER) += systick-timer.o
>>>> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
>>>> new file mode 100644
>>>> index 0000000..162cfe3
>>>> --- /dev/null
>>>> +++ b/arch/arm/cpu/armv7m/cache.c
>>>> @@ -0,0 +1,336 @@
>>>> +/*
>>>> + * (C) Copyright 2017
>>>> + * Vikas Manocha, ST Micoelectronics, vikas.mano...@st.com.
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <errno.h>
>>>> +#include <asm/armv7m.h>
>>>> +#include <asm/io.h>
>>>> +
>>>> +/* Cache maintenance operation registers */
>>>> +
>>>> +#define V7M_CACHE_REG_ICIALLU ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x00))
>>> ^^^^^^^^^^^^^^^^^^^^^
>>> Drop this whole part,
>>> it's a register offset, not address. Just calculate the address
>>> in-place. The cast is also not needed.
>>
>> These are fixed addresses in armv7m architecture.
>
> So what ?
So, no need to calculate in-plcae. It was your suggestion also.
>
>> In place calculations were replaced by
>> this fixed address macro as per your comment.
>
> And then one of the two things will happen:
> a) these addresses will be moved
> b) someone will look at the surrounding code, where _REG_ means register
> offset all over the place and will screw things up ...
moving these addresses...its fixed by architecture.
>
>> casting was moved here from code as per Simon's suggestion on on v3.
>>
>>>
>>>> +#define INVAL_ICACHE_POU 0
>>>> +#define V7M_CACHE_REG_ICIMVALU ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x08))
>>>> +#define V7M_CACHE_REG_DCIMVAC ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x0C))
>>>> +#define V7M_CACHE_REG_DCISW ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x10))
>>>> +#define V7M_CACHE_REG_DCCMVAU ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x14))
>>>> +#define V7M_CACHE_REG_DCCMVAC ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x18))
>>>> +#define V7M_CACHE_REG_DCCSW ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x1C))
>>>> +#define V7M_CACHE_REG_DCCIMVAC ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x20))
>>>> +#define V7M_CACHE_REG_DCCISW ((u32 *)(V7M_CACHE_MAINT_BASE +
>>>> 0x24))
>>>> +#define WAYS_SHIFT 30
>>>> +#define SETS_SHIFT 5
>>>> +
>>>> +/* armv7m processor feature registers */
>>>> +
>>>> +#define V7M_PROC_REG_CLIDR ((u32 *)(V7M_PROC_FTR_BASE +
>>>> 0x00))
>>>> +#define V7M_PROC_REG_CTR ((u32 *)(V7M_PROC_FTR_BASE + 0x04))
>>>> +#define V7M_PROC_REG_CCSIDR ((u32 *)(V7M_PROC_FTR_BASE +
>>>> 0x08))
>>>> +#define MASK_NUM_WAYS GENMASK(12, 3)
>>>> +#define MASK_NUM_SETS GENMASK(27, 13)
>>>> +#define CLINE_SIZE_MASK GENMASK(2, 0)
>>>> +#define NUM_WAYS_SHIFT 3
>>>> +#define NUM_SETS_SHIFT 13
>>>
>>> Well use those macros in MASK_* above ...
>>
>> macros are being used, shifts are required after masking.
>
> Then look at the GENMASK() above and the hardcoded values ...
hardcoded in GENMASK...really ?
>
>>>> +#define V7M_PROC_REG_CSSELR ((u32 *)(V7M_PROC_FTR_BASE +
>>>> 0x0C))
>>>> +#define SEL_I_OR_D BIT(0)
>>>> +
>>>> +enum cache_type {
>>>> + DCACHE,
>>>> + ICACHE,
>>>> +};
>>>> +
>>>> +/* PoU : Point of Unification, Poc: Point of Coherency */
>>>> +enum cache_action {
>>>> + INVALIDATE_POU, /* i-cache invalidate by address */
>>>> + INVALIDATE_POC, /* d-cache invalidate by address */
>>>> + INVALIDATE_SET_WAY, /* d-cache invalidate by sets/ways */
>>>> + FLUSH_POU, /* d-cache clean by address to the PoU */
>>>> + FLUSH_POC, /* d-cache clean by address to the PoC */
>>>> + FLUSH_SET_WAY, /* d-cache clean by sets/ways */
>>>> + FLUSH_INVAL_POC, /* d-cache clean & invalidate by addr to PoC */
>>>> + FLUSH_INVAL_SET_WAY, /* d-cache clean & invalidate by set/ways */
>>>> +};
>>>> +
>>>> +#ifndef CONFIG_SYS_DCACHE_OFF
>>>> +struct dcache_config {
>>>> + u32 ways;
>>>> + u32 sets;
>>>> +};
>>>> +
>>>> +static void get_cache_ways_sets(struct dcache_config *cache)
>>>> +{
>>>> + u32 cache_size_id = readl(V7M_PROC_REG_CCSIDR);
>>>> +
>>>> + cache->ways = (cache_size_id & MASK_NUM_WAYS) >> NUM_WAYS_SHIFT;
>>>> + cache->sets = (cache_size_id & MASK_NUM_SETS) >> NUM_SETS_SHIFT;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return the io register to perform required cache action like clean or
>>>> clean
>>>> + * & invalidate by sets/ways.
>>>> + */
>>>> +static u32 *get_action_reg_set_ways(enum cache_action action)
>>>> +{
>>>> + switch (action) {
>>>> + case INVALIDATE_SET_WAY:
>>>> + return V7M_CACHE_REG_DCISW;
>>>> + case FLUSH_SET_WAY:
>>>> + return V7M_CACHE_REG_DCCSW;
>>>> + case FLUSH_INVAL_SET_WAY:
>>>> + return V7M_CACHE_REG_DCCISW;
>>>> + default:
>>>> + break;
>>>> + };
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return the io register to perform required cache action like clean or
>>>> clean
>>>> + * & invalidate by adddress or range.
>>>> + */
>>>> +static u32 *get_action_reg_range(enum cache_action action)
>>>> +{
>>>> + switch (action) {
>>>> + case INVALIDATE_POU:
>>>> + return V7M_CACHE_REG_ICIMVALU;
>>>> + case INVALIDATE_POC:
>>>> + return V7M_CACHE_REG_DCIMVAC;
>>>> + case FLUSH_POU:
>>>> + return V7M_CACHE_REG_DCCMVAU;
>>>> + case FLUSH_POC:
>>>> + return V7M_CACHE_REG_DCCMVAC;
>>>> + case FLUSH_INVAL_POC:
>>>> + return V7M_CACHE_REG_DCCIMVAC;
>>>> + default:
>>>> + break;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static u32 get_cline_size(enum cache_type type)
>>>> +{
>>>> + u32 size;
>>>> +
>>>> + if (type == DCACHE)
>>>> + clrbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>>>> + else if (type == ICACHE)
>>>> + setbits_le32(V7M_PROC_REG_CSSELR, BIT(SEL_I_OR_D));
>>>> + /* Make sure cache selection is effective for next memory access */
>>>> + dsb();
>>>> +
>>>> + size = readl(V7M_PROC_REG_CCSIDR) & CLINE_SIZE_MASK;
>>>> + /* Size enocoded as 2 less than log(no_of_words_in_cache_line) base 2 */
>>>> + size = 1 << (size + 2);
>>>> + debug("cache line size is %d\n", size);
>>>> +
>>>> + return size;
>>>> +}
>>>> +
>>>> +/* Perform the action like invalidate/clean on a range of cache addresses
>>>> */
>>>> +static int action_cache_range(enum cache_action action, u32 start_addr,
>>>> + int64_t size)
>>>> +{
>>>> + u32 cline_size;
>>>> + u32 *action_reg;
>>>> + enum cache_type type;
>>>
>>> Does this ever check whether start_addr and size is unaligned ?
>>
>> address alignment is being done below before cache action.
>
> Ah, so we hide bugs which are introduced by unaligned accesses ? Well
> please remove that and add address alignment check like the rest ...
which bug ? user passing unaligned address ?
others are doing the same.
>
>>> Also, you use u32 all over the place, but size is signed-i64 ? Why?
>>
>> Yes it is for condition "size > cline_size" when range/size is 4GB.
>
> Divide that by page size and you no longer need to use 64bit type on
> 32bit system ...
page size... its armv7m.
>
>>>> + 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);
>>>> + /* Align start address to cache line boundary */
>>>> + start_addr &= ~(cline_size - 1);
>>>> + debug("total size for cache action = %llx\n", size);
>>>> + do {
>>>> + writel(start_addr, action_reg);
>>>> + size -= cline_size;
>>>> + start_addr += cline_size;
>>>> + } while (size > cline_size);
>>>> +
>>>> + /* Make sure cache action is effective for next memory access */
>>>> + dsb();
>>>> + isb(); /* Make sure instruction stream sees it */
>>>> + debug("cache action on range done\n");
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> [...]
>>>
>>>>
>>>> -#define V7M_SCB_BASE 0xE000ED00
>>>> -#define V7M_MPU_BASE 0xE000ED90
>>>> +/* armv7m fixed base addresses */
>>>> +#define V7M_SCS_BASE 0xE000E000
>>>
>>> Is all of this really used in the code ?
>>
>> Not all of them, it is for the sake of armv7m address space completeness.
>
> Not sure we need them all then ...
thats what i said, not needed all for now. They are there for sake of
completeness, what's the harm ?
Cheers,
Vikas
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
