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

[Marek Vasut]

On 4/10/20 4:58 PM, Patrick DELAUNAY wrote:
> 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.

That's a good idea, let's see.