[rtems commit] bsps/arm: Enable L2C for Cortex-A9 MPCore BSPs

[Chris Johns]

On 21/11/2014 6:31 pm, Sebastian Huber wrote:
>
> On 21/11/14 06:10, Chris Johns wrote:
>> On 21/11/2014 12:53 am, Sebastian Huber wrote:
>>> Module:    rtems
>>> Branch:    master
>>> Commit:    50440c065e247899ee739d56cb1392c259289031
>>> Changeset:
>>> http://git.rtems.org/rtems/commit/?id=50440c065e247899ee739d56cb1392c259289031
>>>
>>>
>>> Author:    Sebastian Huber <sebastian.huber at embedded-brains.de>
>>> Date:      Wed Nov 19 15:30:24 2014 +0100
>>>
>>> bsps/arm: Enable L2C for Cortex-A9 MPCore BSPs
>>>
>>> ---
>>>
>>> +RTEMS_BSPOPTS_SET([BSP_DATA_CACHE_ENABLED],[*],[1])
>>> +RTEMS_BSPOPTS_HELP([BSP_DATA_CACHE_ENABLED],[enable data cache])
>>> +
>>> +RTEMS_BSPOPTS_SET([BSP_INSTRUCTION_CACHE_ENABLED],[*],[1])
>>> +RTEMS_BSPOPTS_HELP([BSP_INSTRUCTION_CACHE_ENABLED],[enable
>>> instruction cache])
>>> +
>>
>> To disable I provide configure with:
>>
>>  BSP_DATA_CACHE_ENABLED=0 BSP_INSTRUCTION_CACHE_ENABLED=0
>>
>> however this:
>>
>>> +#if defined(BSP_DATA_CACHE_ENABLED) ||
>>> defined(BSP_INSTRUCTION_CACHE_ENABLED)
>>> +    /* Enable unified L2 cache */
>>> +    rtems_cache_enable_data();
>>> +#endif
>>
>> and this:
>>
>>> +#if !defined(RTEMS_SMP) \
>>> +  && (defined(BSP_DATA_CACHE_ENABLED) \
>>> +    || defined(BSP_INSTRUCTION_CACHE_ENABLED))
>>> +  /* Enable unified L2 cache */
>>> +  rtems_cache_enable_data();
>>> +#endif
>>
>> only check for defined and it is always defined. These should check
>> for the value only ie:
>>
>> #if BSP_DATA_CACHE_ENABLED || BSP_INSTRUCTION_CACHE_ENABLED
>>
>> ?
>
> You should use
>
> BSP_DATA_CACHE_ENABLED= BSP_INSTRUCTION_CACHE_ENABLED=
>
> for the configure.
>

Urrr <bletch> sure if I have to. This is the first I have ever heard of 
doing this and it is confusing because the configure.ac code uses the 
value '1' and for me that implied a value of '0' had the opposite 
effect. I seem to remember other opts such as the reset one in the PC 
BSP taking 1 or 0.

My preference these days is not to rely on 'defined()' tests. It avoids 
this type of error and the code reads much better.

>>
>> This is not the only case of BSPOPTS'less'ness we have in RTEMS but
>> this one is an issue for me. Have I told you recently how much I
>> dislike the BSPOPTS support.
>
> Yes, I know, but what is the alternative?  These defines are already
> used for a lot of PowerPC BSPs and I don't think it is good to invent
> yet another solution for the same problem on ARM.

I am not suggesting we change anything with this build system. I am 
suggesting we need to address this issue in 5.0 and a new build system 
and my concern is the mixed use of defined() and/or value testing in the 
code base. We need one consistent method and BSPOPTS has no structure 
and so the code has no formal approach and it is confusing to users 
because you need to check all references to see what it actually means. 
It is ok if you wrote the code but if you have not it is a slog to 
figure it all out.

Chris