Re: [PATCH v3 1/3] Convert CONFIG_SYS_L2_PL310 to Kconfig

[Marek Vasut <marex@denx.de>]

On 8/9/22 18:36, Tom Rini wrote:

[...]

>>> It's tricky to say when to "select" or just "default y if .." or "imply"
>>> a given option. It's not only a matter of "can you disable X and the
>>> system is functional" but "would you normally want to". There are
>>> certainly system bring-up cases and similar where you want to disable
>>> L2CC because you're tracking down something. But it's really a feature
>>> of the chip and expected to work and something we want enabled, and the
>>> scope of when it would be disabled is very very narrow. Looking back at
>>> the patch itself, the config.h files that enabled this are almost all a
>>> SoC-common file (ti_am335x_common.h should have been setting this I bet,
>>> something else for the TODO pile for me), so my thought is that it
>>> should be a select'd option, but if there's strong opinion that it's
>>> useful to make it easy to turn off when needed, imply'd instead by the
>>> various ARCH's in question instead.
>>>
>>> That said, thinking about the missing dependency I listed above, that's
>>> how to disable L2 when needed, so perhaps select SYS_L2_PL310 if
>>> !SYS_L2CACHE_OFF under the various ARCH's is the right way.
>>
>> Uh, I didn't realize we also had this SYS_L2CACHE_OFF .
> 
> Yeah, and it's also one of the odd "Enable this to turn oFF ..."
> options, but I think in this case, that makes the most sense.  Note that
> SYS_L2CACHE_OFF defaults to 'n' so do assume we have an L2 cache, and
> also the option is ARM-specific.
> 
>> In that case, the SYS_L2_PL310 selects the L2CC driver to be compiled in and
>> that should likely be per-arch select indeed. And then SYS_L2CACHE_OFF
>> should be 'default' or 'imply', so the user can configure it for the various
>> hardware bring up cases ?
> 
> Yeah, I think we're in agreement here.

Yes