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 <[email protected]>
---
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
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e7351a683545..f60e98da58cd 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -338,17 +338,6 @@ config ARCH_MULTIPLATFORM
source "arch/arm/Kconfig.platforms"
-config ARCH_AIROHA
- bool "Airoha SoC Support"
- depends on ARCH_MULTI_V7
- select ARM_AMBA
- select ARM_GIC
- select ARM_GIC_V3
- select ARM_PSCI
- select HAVE_ARM_ARCH_TIMER
- help
- Support for Airoha EN7523 SoCs
-
#
# This is sorted alphabetically by mach-* pathname. However, plat-*
# Kconfigs may be included either alphabetically (according to the
diff --git a/arch/arm/Kconfig.platforms b/arch/arm/Kconfig.platforms
index 4b5fad18ca8b..38457d5a18ff 100644
--- a/arch/arm/Kconfig.platforms
+++ b/arch/arm/Kconfig.platforms
@@ -67,6 +67,17 @@ config ARCH_VIRT
select ARM_PSCI
select HAVE_ARM_ARCH_TIMER
+config ARCH_AIROHA
+ bool "Airoha SoC Support"
+ depends on ARCH_MULTI_V7
+ select ARM_AMBA
+ select ARM_GIC
+ select ARM_GIC_V3
+ select ARM_PSCI
+ select HAVE_ARM_ARCH_TIMER
+ help
+ Support for Airoha EN7523 SoCs
+
config MACH_ASM9260
bool "Alphascale ASM9260"
depends on ARCH_MULTI_V5
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 32e99aa282bf..e20c8af34d51 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -167,7 +167,6 @@ textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
# Machine directory name. This list is sorted alphanumerically
# by CONFIG_* macro name.
machine-$(CONFIG_ARCH_ACTIONS) += actions
-machine-$(CONFIG_ARCH_AIROHA) += airoha
machine-$(CONFIG_ARCH_ALPINE) += alpine
machine-$(CONFIG_ARCH_ARTPEC) += artpec
machine-$(CONFIG_ARCH_ASPEED) += aspeed
diff --git a/arch/arm/mach-airoha/Makefile b/arch/arm/mach-airoha/Makefile
deleted file mode 100644
index a5857d0d02eb..000000000000
--- a/arch/arm/mach-airoha/Makefile
+++ /dev/null
@@ -1,2 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-only
-obj-y += airoha.o
diff --git a/arch/arm/mach-airoha/airoha.c b/arch/arm/mach-airoha/airoha.c
deleted file mode 100644
index ea23b5abb478..000000000000
--- a/arch/arm/mach-airoha/airoha.c
+++ /dev/null
@@ -1,16 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Device Tree support for Airoha SoCs
- *
- * Copyright (c) 2022 Felix Fietkau <[email protected]>
- */
-#include <asm/mach/arch.h>
-
-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
--
2.39.2
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!)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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