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

  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.

It also seems there was a copy/paste error and the "MEDIATEK_DT"
name was re-used in the DT_MACHINE_START macro. This may have caused
conflicts if this was built in a multi-arch configuration.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/Kconfig              | 11 -----------
 arch/arm/Kconfig.platforms    | 11 +++++++++++
 arch/arm/Makefile             |  1 -
 arch/arm/mach-airoha/Makefile |  2 --
 arch/arm/mach-airoha/airoha.c | 16 ----------------
 5 files changed, 11 insertions(+), 30 deletions(-)
 delete mode 100644 arch/arm/mach-airoha/Makefile
 delete mode 100644 arch/arm/mach-airoha/airoha.c
  

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.

This has the effect of _not_ calling l2x0_of_init() - but you don't
mention this. You probably should, and you should probably state why
that is safe (assuming you've even realised you've made this change!)
  

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..

> This has the effect of _not_ calling l2x0_of_init() - but you don't
> mention this. You probably should, and you should probably state why
> that is safe (assuming you've even realised you've made this change!)
> 

Reverse question, did the folks adding this support know this had
the effect of changing l2c_aux_mask from the default?

For now I'll resend this series with only the first 5 patches which
should be trivially safe. The later ones I can send after double
checking on l2x0_of_init().

Andrew
  

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