[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex at denx.de>
> Sent: vendredi 10 avril 2020 10:14
> 
> On 4/9/20 8:06 PM, Patrick DELAUNAY wrote:
> > Dear Marek,
> 
> Hi,
> 
> >> Sent: jeudi 9 avril 2020 12:21
> >> To: Patrick DELAUNAY <patrick.delaunay at st.com>; u-boot at lists.denx.de
> >>
> >> On 4/9/20 12:01 PM, Patrick DELAUNAY wrote:
> >>> Dear Marek,
> >>
> >> Hi,
> >>
> >>>> From: Uboot-stm32
> >>>> <uboot-stm32-bounces at st-md-mailman.stormreply.com>
> >>>> On Behalf Of Patrick DELAUNAY
> >>>>
> >>>> Dear Marek,
> >>>>
> >>>>> From: Marek Vasut <marex at denx.de>
> >>>>> Sent: vendredi 3 avril 2020 23:29
> >>>>>
> >>>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> >>>>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default
> >>>>>> option to use according the compilation flags
> >>>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
> >>>>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
> >>>>>
> >>>>> Can't you unify these macros into a single Kconfig "select"
> >>>>> statement instead , and then just select the matching cache
> >>>>> configuration in
> >> Kconfig ?
> >>>>
> >>>> Yes I will try, with 2 steps
> >>>> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
> >>>
> >>> First step done...
> >>> I will push it as a separate patchset I think.
> >>>
> >>>> - add new option CONFIG_SYS_ARM_CACHE_OPTION
> >>>
> >>> In fact it is to difficult to use select because each defines
> >>> DCACHE_XXX value can have several values
> >>>
> >>> they are build according CONFIG_ARM64 / LPAE
> >>>
> >>> But, I can't use this define in Kconfig
> >>>
> >>> I try :
> >>> option	ARM_OPTION
> >>> 	int "option"
> >>> 	default DCACHE_WRITETHROUGHT if
> >> CONFIG_SYS_ARM_CACHE_WRITETHROUGH
> >>> 	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_
> >> WRITEALLOC
> >>> 	default DCACHE_WRITEBACK if
> >> CONFIG_SYS_ARM_CACHE_WRITEBACK
> >>>
> >>> int and hex is invalid, and string can't be use with "".
> >>>
> >>> And I don't found way to build it automatically when option is activated.
> >>>
> >>> Any idea ?
> >>
> >> Maybe you can have a select in the Kconfig to set some differently
> >> named option, e.g.
> >>
> >> DCACHE_MODE_WRITE{THROUGH,ALLOC,BACK}
> >>
> >> and then an ifdef in some header file, e.g.
> >>
> >> #ifdef CONFIG_DCACHE_MODE_WRITETHROUGH #define
> ARM_CACHE_MODE
> >> DCACHE_WRITETHROUGH ...
> >>
> >> And then use ARM_CACHE_MODE where you need a value and
> >> CONFIG_DCACHE_MODE{...} where you need a config option check.
> >>
> >> Does this work ?
> >
> > I try with string and default (as select is allowed on for bolean or
> > trisate), And I failed :-<
> >
> > I don't found a way to have the de-stringficate the KConfig option to
> > generated the correct define
> 
> The result is a boolean , isn't it ? One out of N configs ends up being defined and
> the rest are not defined.
> 
> > config SYS_ARM_CACHE_POLICY
> > 	string "Name of the ARM data write cache policy"
> > 	default WRITEBACK if SYS_ARM_CACHE_WRITEBACK
> > 	default WRITETHROUGH if SYS_ARM_CACHE_WRITEBACK
> > 	default WRITEALLOC if SYS_ARM_CACHE_WRITEALLOC
> >
> > #define DCACHE_DEFAULT_OPTION	DCACHE_ ##
> CONFIG_SYS_ARM_CACHE_POLICY
> >
> > => error: ‘DCACHE_CONFIG_SYS_ARM_CACHE_POLICY’ undeclared (first
> use in this function); did you mean ‘CONFIG_SYS_ARM_CACHE_POLICY’?
> >
> > #define DCACHE_OPTION(s)	DCACHE_ ##
> CONFIG_SYS_ARM_CACHE_POLICY
> >
> > #define DCACHE_DEFAULT_OPTION
> 	DCACHE_OPTION(CONFIG_SYS_ARM_CACHE_POLICY)
> >
> > arch/arm/include/asm/system.h:488:26: error: ‘DCACHE_’ undeclared (first use
> in this function); did you mean ‘DCACHE_OFF’?
> > arch/arm/lib/cache-cp15.c:99:25: error: expected ‘)’ before string
> > constant
> >
> > The stringification is possible but not the inverse operation (remove the quote)...
> >
> > In my .config, CONFIG_SYS_ARM_CACHE_POLICY="WRITEBACK"
> 
> What about this:
> 
> choice
> prompt "Cache policy"
>  default CACHE_WRITEBACK
> 
> config CACHE_WRITEBACK
>  bool "Writeback"
> 
> config ...
> 
> endchoice
> 
> and then in some header
> 
> #ifdef CONFIG_CACHE_WRITEBACK
> #define CONFIF_SYS_ARM_CACHE_WRITEBACK
> #else
> ...
> 
> Would that work ?

Yes, it can work it seems complicated

I push v2 for CONFIG_SYS_ARM_CACHE_* migration in Kconfig

My proposal becomes:

+#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITETHROUGH
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEALLOC
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEBACK)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEBACK
+#endif
+

I think it is is more clear solution.


I can use macro magic to build DCACHE_DEFAULT_OPTION 

+#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
+#define ARM_CACHE_POLICY	WRITETHROUGH
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
+#define ARM_CACHE_POLICY	WRITEALLOC
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEBACK)
+#define ARM_CACHE_POLICY	WRITEBACK
+#endif
 
+#define _DCACHE_OPTION(policy)	DCACHE_ ## policy
+#define DCACHE_OPTION(policy)	_DCACHE_OPTION(policy)
+#define DCACHE_DEFAULT_OPTION	DCACHE_OPTION(ARM_CACHE_POLICY)
+

> 
> But I feel I might really be missing the question here.

I think I will push V2 in one week (I am out of office next week) to wait other feedback.

It could be more clear with an updated patchset.

Regards