| Message ID | 20230515160234.289631-6-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] ARM: Kconfig: move platform selection into its own Kconfig file | expand |
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
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 <nbd@nbd.name> - */ -#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
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