Re: [PATCH 06/10] ARM: mach-airoha: Rework support and directory structure

[Arnd Bergmann]

On Thu, Jul 13, 2023, at 20:44, Andrew Davis wrote:
> On 5/15/23 11:31 AM, Russell King (Oracle) wrote:
>> On Mon, May 15, 2023 at 11:02:30AM -0500, Andrew Davis wrote:
>>> Having a platform need a mach-* directory should be seen as a negative,
>>> it means the platform needs special non-standard handling. ARM64 support
>>> does not allow mach-* directories at all. While we may not get to that
>>> given all the non-standard architectures we support, we should still try
>>> to get as close as we can and reduce the number of mach directories.
>>>
>>> The mach-airoha/ directory, and files within, provide just one "feature":
>>> having the kernel print the machine name if the DTB does not also contain
>>> a "model" string (which they always do). To reduce the number of mach-*
>>> directories let's do without that feature and remove this directory.
>> 
>> I'm guessing this is copy-n-pasted description. However:
>>> -static const char * const airoha_board_dt_compat[] = {
>>> -	"airoha,en7523",
>>> -	NULL,
>>> -};
>>> -
>>> -DT_MACHINE_START(MEDIATEK_DT, "Airoha Cortex-A53 (Device Tree)")
>>> -	.dt_compat	= airoha_board_dt_compat,
>>> -MACHINE_END
>> 
>> If this is actually used, then it will have the effect of providing a
>> "machine" that has both l2c_aux_mask and l2c_aux_val as zero, whereas
>> the default one has l2c_aux_mask set to ~0.
>> 
>
> Given we set l2c_aux_mask to ~0 as a default for "Generic" DT system I
> had assumed this was safe, but no I cannot prove it for this board as
> I don't have one.
>
> I wonder if we should have some way to set this in DT, that would
> let us drop some more MACHINE defines that exist only to set
> the l2c_aux_val/mask..

Going from an empty machine description to the default one is
generally safe as long as there is no actual l2x0 cache controller
in the system that would incorrectly get enabled by this in case
it is intentionally left disabled. I'm not aware of any such case,
but it's possible.

For the Airoha chip, we know this is safe because ARMv8 and later
ARMv7 cores (A7, A15 and A17) never have this type of cache
controller.

So your patch is fine, just mention in the description that
the change in the cache controller handling is correct.

      Arnd