Hi Marek,
> -----Original Message-----
> From: U-Boot [mailto:[email protected]] On Behalf Of Vikas Manocha
> Sent: Monday, March 27, 2017 2:33 PM
> To: Marek Vasut <[email protected]>; [email protected]
> Cc: Christophe KERELLO <[email protected]>; Toshifumi NISHINAGA
> <[email protected]>; Stephen Warren
> <[email protected]>
> Subject: Re: [U-Boot] [PATCH v4 1/2] armv7m: add instruction & data cache
> support
>
> 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 <[email protected]>
> >>>> cc: Christophe KERELLO <[email protected]>
> >>>> ---
> >>>>
> >>>> 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, [email protected].
> >>>> + *
> >>>> + * 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.
Let me know if there is some open point for this patch.
Cheers,
Vikas
>
> >
> >> 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
> [email protected]
> https://lists.denx.de/listinfo/u-boot
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
