[PATCH 0/4] sunxi: minor cleanups and refactoring

[PATCH 0/4] sunxi: minor cleanups and refactoring

[Andre Przywara]

Just a collection of patches I came up with while trying to refactor and
reorganise the Allwinner SPL code.
Patch 1/4 aims to replace Tom's patch [1], which made
CONFIG_MMC_SUNXI_SLOT a proper Kconfig variable. A closer inspection
reveals that we don't really need that at all.
Patch 2 and 3 are just random cleanups.
Patch 4 is a low hanging fruit in the effort to separate ARM specific
code from strictly Allwinner platform routines. It also removes some
#ifdef's.

Please have a look!

Cheers,
Andre

[1] https://lore.kernel.org/u-boot/20221127152536.1556469-6-trini@konsulko.com/

Andre Przywara (4):
  sunxi: remove unused CONFIG_MMC_SUNXI_SLOT
  sunxi: remove bogus mmc_pinmux_setup() prototype
  sunxi: board: annotate #endif lines
  sunxi: move arch timer setup out of board/ directory

 arch/arm/mach-sunxi/Makefile      |  4 +++
 arch/arm/mach-sunxi/arch_timer.c  | 39 +++++++++++++++++++++
 arch/arm/mach-sunxi/board.c       | 22 +++++++-----
 arch/arm/mach-sunxi/clock_sun6i.c |  4 +--
 board/sunxi/board.c               | 58 +++++++------------------------
 include/configs/sunxi-common.h    |  3 --
 scripts/config_whitelist.txt      |  1 -
 7 files changed, 72 insertions(+), 59 deletions(-)
 create mode 100644 arch/arm/mach-sunxi/arch_timer.c

-- 
2.35.5

[PATCH 1/4] sunxi: remove unused CONFIG_MMC_SUNXI_SLOT

[Andre Przywara]

There is a CONFIG_MMC_SUNXI_SLOT definition in our sunxi_common.h config
header, which was used to note the first MMC controller to initialise.
The definition in that header was always set to 0, with no easy way of
overriding this, and certainly none of the existing boards made any use
of that (non-)feature.
Remove that definition and replace it with a constant 0 in the only
user, in board.c. It turns out that this is safe, as this is only used
in the SPL, and the BROM also unconditionally initialises MMC0.
This also removes the last legacy config symbol with SUN*I in it from
the whitelist.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c            | 9 +++++++--
 include/configs/sunxi-common.h | 3 ---
 scripts/config_whitelist.txt   | 1 -
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 21a2407e062..4d2491b5a86 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -525,9 +525,14 @@ static void mmc_pinmux_setup(int sdc)
 
 int board_mmc_init(struct bd_info *bis)
 {
+	/*
+	 * The BROM always accesses MMC port 0 (typically an SD card), and
+	 * most boards seem to have such a slot. The others haven't reported
+	 * any problem with unconditionally enabling this in the SPL.
+	 */
 	if (!IS_ENABLED(CONFIG_UART0_PORT_F)) {
-		mmc_pinmux_setup(CONFIG_MMC_SUNXI_SLOT);
-		if (!sunxi_mmc_init(CONFIG_MMC_SUNXI_SLOT))
+		mmc_pinmux_setup(0);
+		if (!sunxi_mmc_init(0))
 			return -1;
 	}
 
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 720768629d6..e89ad42ce8d 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -75,9 +75,6 @@
 #define CONFIG_SYS_NAND_MAX_ECCPOS 1664
 #endif
 
-/* mmc config */
-#define CONFIG_MMC_SUNXI_SLOT		0
-
 /*
  * Miscellaneous configurable options
  */
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index c0f55e41a50..ea71f9d2344 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -153,7 +153,6 @@ CONFIG_MISC_COMMON
 CONFIG_MIU_2BIT_21_7_INTERLEAVED
 CONFIG_MIU_2BIT_INTERLEAVED
 CONFIG_MMC_DEFAULT_DEV
-CONFIG_MMC_SUNXI_SLOT
 CONFIG_MONITOR_IS_IN_RAM
 CONFIG_MPC85XX_FEC
 CONFIG_MPC85XX_FEC_NAME
-- 
2.35.5

Re: [PATCH 1/4] sunxi: remove unused CONFIG_MMC_SUNXI_SLOT

[Samuel Holland]

On 12/13/22 18:22, Andre Przywara wrote:
> There is a CONFIG_MMC_SUNXI_SLOT definition in our sunxi_common.h config
> header, which was used to note the first MMC controller to initialise.
> The definition in that header was always set to 0, with no easy way of
> overriding this, and certainly none of the existing boards made any use
> of that (non-)feature.
> Remove that definition and replace it with a constant 0 in the only
> user, in board.c. It turns out that this is safe, as this is only used
> in the SPL, and the BROM also unconditionally initialises MMC0.
> This also removes the last legacy config symbol with SUN*I in it from
> the whitelist.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  board/sunxi/board.c            | 9 +++++++--
>  include/configs/sunxi-common.h | 3 ---
>  scripts/config_whitelist.txt   | 1 -
>  3 files changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Samuel Holland <samuel@sholland.org>
Tested-by: Samuel Holland <samuel@sholland.org>

[PATCH 2/4] sunxi: remove bogus mmc_pinmux_setup() prototype

[Andre Przywara]

Since all callers of mmc_pinmux_setup() are located after the definition
of that function, there is no need for a forward declaration (anymore?).

Remove the prototype along with its #ifdef guards.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 4d2491b5a86..4fe749feb39 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -184,10 +184,6 @@ enum env_location env_get_location(enum env_operation op, int prio)
 	return ENVL_UNKNOWN;
 }
 
-#ifdef CONFIG_DM_MMC
-static void mmc_pinmux_setup(int sdc);
-#endif
-
 /* add board specific code here */
 int board_init(void)
 {
-- 
2.35.5

Re: [PATCH 2/4] sunxi: remove bogus mmc_pinmux_setup() prototype

[Samuel Holland]

On 12/13/22 18:22, Andre Przywara wrote:
> Since all callers of mmc_pinmux_setup() are located after the definition
> of that function, there is no need for a forward declaration (anymore?).
> 
> Remove the prototype along with its #ifdef guards.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  board/sunxi/board.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Samuel Holland <samuel@sholland.org>
Tested-by: Samuel Holland <samuel@sholland.org>

[PATCH 3/4] sunxi: board: annotate #endif lines

[Andre Przywara]

The legacy Allwinner code is cluttered with #ifdef's, some of them even
nested, which makes the code hard to read and error prone.
Eventually we will get rid of most of them, but for now let's at least
annotate the #endif lines with the corresponding symbol the bracket
started with.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-sunxi/board.c       |  8 ++++----
 arch/arm/mach-sunxi/clock_sun6i.c |  4 ++--
 board/sunxi/board.c               | 11 +++++------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
index 86233637bfc..0c4b6dd1ca3 100644
--- a/arch/arm/mach-sunxi/board.c
+++ b/arch/arm/mach-sunxi/board.c
@@ -73,7 +73,7 @@ phys_size_t board_get_usable_ram_top(phys_size_t total_size)
 
 	return gd->ram_top;
 }
-#endif
+#endif /* CONFIG_ARM64 */
 
 #ifdef CONFIG_SPL_BUILD
 static int gpio_init(void)
@@ -196,7 +196,7 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
 	return 0;
 }
 SPL_LOAD_IMAGE_METHOD("FEL", 0, BOOT_DEVICE_BOARD, spl_board_load_image);
-#endif
+#endif /* CONFIG_SPL_BUILD */
 
 #define SUNXI_INVALID_BOOT_SOURCE	-1
 
@@ -457,7 +457,7 @@ void board_init_f(ulong dummy)
 #endif
 	sunxi_board_init();
 }
-#endif
+#endif /* CONFIG_SPL_BUILD */
 
 #if !CONFIG_IS_ENABLED(SYSRESET)
 void reset_cpu(void)
@@ -490,7 +490,7 @@ void reset_cpu(void)
 	while (1) { }
 #endif
 }
-#endif
+#endif /* CONFIG_SYSRESET */
 
 #if !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) && defined(CONFIG_CPU_V7A)
 void enable_caches(void)
diff --git a/arch/arm/mach-sunxi/clock_sun6i.c b/arch/arm/mach-sunxi/clock_sun6i.c
index cda6949dff0..6bd75a15f6d 100644
--- a/arch/arm/mach-sunxi/clock_sun6i.c
+++ b/arch/arm/mach-sunxi/clock_sun6i.c
@@ -63,7 +63,7 @@ void clock_init_safe(void)
 	setbits_le32(&ccm->sata_clk_cfg, CCM_SATA_CTRL_ENABLE);
 #endif
 }
-#endif
+#endif /* CONFIG_SPL_BUILD */
 
 void clock_init_sec(void)
 {
@@ -172,7 +172,7 @@ void clock_set_pll1(unsigned int clk)
 		       &ccm->cpu_axi_cfg);
 	}
 }
-#endif
+#endif /* CONFIG_SPL_BUILD */
 
 void clock_set_pll3(unsigned int clk)
 {
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 4fe749feb39..827e545032e 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -351,7 +351,7 @@ void board_nand_init(void)
 	sunxi_nand_init();
 #endif
 }
-#endif
+#endif /* CONFIG_NAND_SUNXI */
 
 #ifdef CONFIG_MMC
 static void mmc_pinmux_setup(int sdc)
@@ -554,7 +554,7 @@ int mmc_get_env_dev(void)
 	}
 }
 #endif
-#endif
+#endif /* CONFIG_MMC */
 
 #ifdef CONFIG_SPL_BUILD
 
@@ -670,7 +670,7 @@ void sunxi_board_init(void)
 	else
 		printf("Failed to set core voltage! Can't set CPU frequency\n");
 }
-#endif
+#endif /* CONFIG_SPL_BUILD */
 
 #ifdef CONFIG_USB_GADGET
 int g_dnl_board_usb_cable_connected(void)
@@ -699,7 +699,7 @@ int g_dnl_board_usb_cable_connected(void)
 
 	return sun4i_usb_phy_vbus_detect(&phy);
 }
-#endif
+#endif /* CONFIG_USB_GADGET */
 
 #ifdef CONFIG_SERIAL_TAG
 void get_board_serial(struct tag_serialnr *serialnr)
@@ -928,7 +928,6 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 }
 
 #ifdef CONFIG_SPL_LOAD_FIT
-
 static void set_spl_dt_name(const char *name)
 {
 	struct boot_file_head *spl = get_spl_header(SPL_ENV_HEADER_VERSION);
@@ -996,4 +995,4 @@ int board_fit_config_name_match(const char *name)
 
 	return ret;
 }
-#endif
+#endif /* CONFIG_SPL_LOAD_FIT */
-- 
2.35.5

Re: [PATCH 3/4] sunxi: board: annotate #endif lines

[Samuel Holland]

On 12/13/22 18:22, Andre Przywara wrote:
> The legacy Allwinner code is cluttered with #ifdef's, some of them even
> nested, which makes the code hard to read and error prone.
> Eventually we will get rid of most of them, but for now let's at least
> annotate the #endif lines with the corresponding symbol the bracket
> started with.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/mach-sunxi/board.c       |  8 ++++----
>  arch/arm/mach-sunxi/clock_sun6i.c |  4 ++--
>  board/sunxi/board.c               | 11 +++++------
>  3 files changed, 11 insertions(+), 12 deletions(-)

Reviewed-by: Samuel Holland <samuel@sholland.org>
Tested-by: Samuel Holland <samuel@sholland.org>

[PATCH 4/4] sunxi: move arch timer setup out of board/ directory

[Andre Przywara]

At the moment we have an #ifdef-protected routine in
board/sunxi/board.c, which sets up the arch timer CNTFRQ register, if
that hasn't been done already.
This only applies to ARMv7 SoCs running in secure state, and so is
much better located in the arch/arm/mach-sunxi directory.

Move that routine into a separate function and file in said directory,
which also allows to trigger the compilation for ARMv7 in the Makefile.
Also move the call to it into the arch/arm version of board.c.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-sunxi/Makefile     |  4 ++++
 arch/arm/mach-sunxi/arch_timer.c | 39 ++++++++++++++++++++++++++++++++
 arch/arm/mach-sunxi/board.c      | 14 ++++++++----
 board/sunxi/board.c              | 34 +---------------------------
 4 files changed, 54 insertions(+), 37 deletions(-)
 create mode 100644 arch/arm/mach-sunxi/arch_timer.c

diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
index 58f807cb82d..335c44476e9 100644
--- a/arch/arm/mach-sunxi/Makefile
+++ b/arch/arm/mach-sunxi/Makefile
@@ -26,8 +26,12 @@ obj-$(CONFIG_MACH_SUN8I)	+= clock_sun6i.o
 endif
 obj-$(CONFIG_MACH_SUN9I)	+= clock_sun9i.o gtbus_sun9i.o
 obj-$(CONFIG_SUN50I_GEN_H6)	+= clock_sun50i_h6.o
+
 ifndef CONFIG_ARM64
 obj-y	+= timer.o
+ifndef CONFIG_MACH_SUNIV
+obj-y	+= arch_timer.o
+endif
 endif
 
 ifdef CONFIG_SPL_BUILD
diff --git a/arch/arm/mach-sunxi/arch_timer.c b/arch/arm/mach-sunxi/arch_timer.c
new file mode 100644
index 00000000000..c0e4310741f
--- /dev/null
+++ b/arch/arm/mach-sunxi/arch_timer.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ARM Generic Timer CNTFRQ setup
+ *
+ * (C) Copyright 2013 Oliver Schinagl <oliver@schinagl.nl>
+ */
+
+#include <stdint.h>
+#include <log.h>
+#include <asm/armv7.h>
+
+void setup_arch_timer(void)
+{
+	uint32_t id_pfr1, freq;
+
+	asm volatile("mrc p15, 0, %0, c0, c1, 1" : "=r"(id_pfr1));
+	debug("id_pfr1: 0x%08x\n", id_pfr1);
+	/* Generic Timer Extension available? */
+	if (((id_pfr1 >> CPUID_ARM_GENTIMER_SHIFT) & 0xf) == 0)
+		return;
+
+	/*
+	 * CNTFRQ is a secure register, so we will crash if we try to
+	 * write this from the non-secure world (read is OK, though).
+	 * In case some bootcode has already set the correct value,
+	 * we avoid the risk of writing to it.
+	 */
+	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r"(freq));
+	if (freq == CONFIG_COUNTER_FREQUENCY)
+		return;
+
+	debug("arch timer frequency is %d Hz, should be %d, fixing ...\n",
+	      freq, CONFIG_COUNTER_FREQUENCY);
+	if (IS_ENABLED(CONFIG_NON_SECURE))
+		printf("arch timer frequency is wrong, but cannot adjust it\n");
+	else
+		asm volatile("mcr p15, 0, %0, c14, c0, 0"
+			     : : "r"(CONFIG_COUNTER_FREQUENCY));
+}
diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
index 0c4b6dd1ca3..943f89eaa78 100644
--- a/arch/arm/mach-sunxi/board.c
+++ b/arch/arm/mach-sunxi/board.c
@@ -347,10 +347,6 @@ u32 spl_boot_device(void)
 	return sunxi_get_boot_device();
 }
 
-__weak void sunxi_sram_init(void)
-{
-}
-
 /*
  * When booting from an eMMC boot partition, the SPL puts the same boot
  * source code into SRAM A1 as when loading the SPL from the normal
@@ -434,10 +430,20 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
 	return result;
 }
 
+__weak void sunxi_sram_init(void)
+{
+}
+
+__weak void setup_arch_timer(void)
+{
+}
+
 void board_init_f(ulong dummy)
 {
 	sunxi_sram_init();
 
+	setup_arch_timer();
+
 #if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3
 	/* Enable non-secure access to some peripherals */
 	tzpc_init();
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 827e545032e..55501fafc38 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -34,9 +34,6 @@
 #include <asm/global_data.h>
 #include <linux/delay.h>
 #include <u-boot/crc.h>
-#ifndef CONFIG_ARM64
-#include <asm/armv7.h>
-#endif
 #include <asm/gpio.h>
 #include <asm/io.h>
 #include <u-boot/crc.h>
@@ -187,39 +184,10 @@ enum env_location env_get_location(enum env_operation op, int prio)
 /* add board specific code here */
 int board_init(void)
 {
-	__maybe_unused int id_pfr1, ret, satapwr_pin, macpwr_pin;
+	int ret, satapwr_pin, macpwr_pin;
 
 	gd->bd->bi_boot_params = (PHYS_SDRAM_0 + 0x100);
 
-#if !defined(CONFIG_ARM64) && !defined(CONFIG_MACH_SUNIV)
-	asm volatile("mrc p15, 0, %0, c0, c1, 1" : "=r"(id_pfr1));
-	debug("id_pfr1: 0x%08x\n", id_pfr1);
-	/* Generic Timer Extension available? */
-	if ((id_pfr1 >> CPUID_ARM_GENTIMER_SHIFT) & 0xf) {
-		uint32_t freq;
-
-		debug("Setting CNTFRQ\n");
-
-		/*
-		 * CNTFRQ is a secure register, so we will crash if we try to
-		 * write this from the non-secure world (read is OK, though).
-		 * In case some bootcode has already set the correct value,
-		 * we avoid the risk of writing to it.
-		 */
-		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r"(freq));
-		if (freq != CONFIG_COUNTER_FREQUENCY) {
-			debug("arch timer frequency is %d Hz, should be %d, fixing ...\n",
-			      freq, CONFIG_COUNTER_FREQUENCY);
-#ifdef CONFIG_NON_SECURE
-			printf("arch timer frequency is wrong, but cannot adjust it\n");
-#else
-			asm volatile("mcr p15, 0, %0, c14, c0, 0"
-				     : : "r"(CONFIG_COUNTER_FREQUENCY));
-#endif
-		}
-	}
-#endif /* !CONFIG_ARM64 && !CONFIG_MACH_SUNIV */
-
 	ret = axp_gpio_init();
 	if (ret)
 		return ret;
-- 
2.35.5

Re: [PATCH 4/4] sunxi: move arch timer setup out of board/ directory

[Samuel Holland]

Hi Andre,

On 12/13/22 18:22, Andre Przywara wrote:
> At the moment we have an #ifdef-protected routine in
> board/sunxi/board.c, which sets up the arch timer CNTFRQ register, if
> that hasn't been done already.
> This only applies to ARMv7 SoCs running in secure state, and so is
> much better located in the arch/arm/mach-sunxi directory.
> 
> Move that routine into a separate function and file in said directory,
> which also allows to trigger the compilation for ARMv7 in the Makefile.
> Also move the call to it into the arch/arm version of board.c.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/mach-sunxi/Makefile     |  4 ++++
>  arch/arm/mach-sunxi/arch_timer.c | 39 ++++++++++++++++++++++++++++++++
>  arch/arm/mach-sunxi/board.c      | 14 ++++++++----
>  board/sunxi/board.c              | 34 +---------------------------
>  4 files changed, 54 insertions(+), 37 deletions(-)
>  create mode 100644 arch/arm/mach-sunxi/arch_timer.c
> 
> diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> index 58f807cb82d..335c44476e9 100644
> --- a/arch/arm/mach-sunxi/Makefile
> +++ b/arch/arm/mach-sunxi/Makefile
> @@ -26,8 +26,12 @@ obj-$(CONFIG_MACH_SUN8I)	+= clock_sun6i.o
>  endif
>  obj-$(CONFIG_MACH_SUN9I)	+= clock_sun9i.o gtbus_sun9i.o
>  obj-$(CONFIG_SUN50I_GEN_H6)	+= clock_sun50i_h6.o
> +
>  ifndef CONFIG_ARM64
>  obj-y	+= timer.o
> +ifndef CONFIG_MACH_SUNIV

Probably Allwinner will not release any new ARMv5 SoC, but would it be
more future-proof (and a bit more meaningful) to condition this on
CONFIG_CPU_V7A?

Alternatively, would it make sense to put this code inside
arch/arm/cpu/armv7/arch_timer.c? I don't know why we wouldn't want to
enable CONFIG_SYS_ARCH_TIMER on sunxi SoCs where it is available. And
making this generic could eventually remove the need for the version in
arch/arm/cpu/armv7/nonsec_virt.S. But this patch is still an improvement
as-is.

Tested-by: Samuel Holland <samuel@sholland.org>

> +obj-y	+= arch_timer.o
> +endif
>  endif
>  
>  ifdef CONFIG_SPL_BUILD
> diff --git a/arch/arm/mach-sunxi/arch_timer.c b/arch/arm/mach-sunxi/arch_timer.c
> new file mode 100644
> index 00000000000..c0e4310741f
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/arch_timer.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ARM Generic Timer CNTFRQ setup
> + *
> + * (C) Copyright 2013 Oliver Schinagl <oliver@schinagl.nl>

`git blame` points to cba69eeeaa67d3fb93ec6f3abab1f653abf895a9 and
d96ebc468d0dff6eb6f069bba03b3f0e33aa22de as the source for this code, so
this authorship is incorrect.

> + */
> +
> +#include <stdint.h>

This does not do what you expect, or really anything at all. The comment
there suggests using linux/types.h.

Regards,
Samuel

> +#include <log.h>
> +#include <asm/armv7.h>
> +
> +void setup_arch_timer(void)
> +{
> +	uint32_t id_pfr1, freq;
> +
> +	asm volatile("mrc p15, 0, %0, c0, c1, 1" : "=r"(id_pfr1));
> +	debug("id_pfr1: 0x%08x\n", id_pfr1);
> +	/* Generic Timer Extension available? */
> +	if (((id_pfr1 >> CPUID_ARM_GENTIMER_SHIFT) & 0xf) == 0)
> +		return;
> +
> +	/*
> +	 * CNTFRQ is a secure register, so we will crash if we try to
> +	 * write this from the non-secure world (read is OK, though).
> +	 * In case some bootcode has already set the correct value,
> +	 * we avoid the risk of writing to it.
> +	 */
> +	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r"(freq));
> +	if (freq == CONFIG_COUNTER_FREQUENCY)
> +		return;
> +
> +	debug("arch timer frequency is %d Hz, should be %d, fixing ...\n",
> +	      freq, CONFIG_COUNTER_FREQUENCY);
> +	if (IS_ENABLED(CONFIG_NON_SECURE))
> +		printf("arch timer frequency is wrong, but cannot adjust it\n");
> +	else
> +		asm volatile("mcr p15, 0, %0, c14, c0, 0"
> +			     : : "r"(CONFIG_COUNTER_FREQUENCY));
> +}
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index 0c4b6dd1ca3..943f89eaa78 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -347,10 +347,6 @@ u32 spl_boot_device(void)
>  	return sunxi_get_boot_device();
>  }
>  
> -__weak void sunxi_sram_init(void)
> -{
> -}
> -
>  /*
>   * When booting from an eMMC boot partition, the SPL puts the same boot
>   * source code into SRAM A1 as when loading the SPL from the normal
> @@ -434,10 +430,20 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
>  	return result;
>  }
>  
> +__weak void sunxi_sram_init(void)
> +{
> +}
> +
> +__weak void setup_arch_timer(void)
> +{
> +}
> +
>  void board_init_f(ulong dummy)
>  {
>  	sunxi_sram_init();
>  
> +	setup_arch_timer();
> +
>  #if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3
>  	/* Enable non-secure access to some peripherals */
>  	tzpc_init();
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 827e545032e..55501fafc38 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -34,9 +34,6 @@
>  #include <asm/global_data.h>
>  #include <linux/delay.h>
>  #include <u-boot/crc.h>
> -#ifndef CONFIG_ARM64
> -#include <asm/armv7.h>
> -#endif
>  #include <asm/gpio.h>
>  #include <asm/io.h>
>  #include <u-boot/crc.h>
> @@ -187,39 +184,10 @@ enum env_location env_get_location(enum env_operation op, int prio)
>  /* add board specific code here */
>  int board_init(void)
>  {
> -	__maybe_unused int id_pfr1, ret, satapwr_pin, macpwr_pin;
> +	int ret, satapwr_pin, macpwr_pin;
>  
>  	gd->bd->bi_boot_params = (PHYS_SDRAM_0 + 0x100);
>  
> -#if !defined(CONFIG_ARM64) && !defined(CONFIG_MACH_SUNIV)
> -	asm volatile("mrc p15, 0, %0, c0, c1, 1" : "=r"(id_pfr1));
> -	debug("id_pfr1: 0x%08x\n", id_pfr1);
> -	/* Generic Timer Extension available? */
> -	if ((id_pfr1 >> CPUID_ARM_GENTIMER_SHIFT) & 0xf) {
> -		uint32_t freq;
> -
> -		debug("Setting CNTFRQ\n");
> -
> -		/*
> -		 * CNTFRQ is a secure register, so we will crash if we try to
> -		 * write this from the non-secure world (read is OK, though).
> -		 * In case some bootcode has already set the correct value,
> -		 * we avoid the risk of writing to it.
> -		 */
> -		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r"(freq));
> -		if (freq != CONFIG_COUNTER_FREQUENCY) {
> -			debug("arch timer frequency is %d Hz, should be %d, fixing ...\n",
> -			      freq, CONFIG_COUNTER_FREQUENCY);
> -#ifdef CONFIG_NON_SECURE
> -			printf("arch timer frequency is wrong, but cannot adjust it\n");
> -#else
> -			asm volatile("mcr p15, 0, %0, c14, c0, 0"
> -				     : : "r"(CONFIG_COUNTER_FREQUENCY));
> -#endif
> -		}
> -	}
> -#endif /* !CONFIG_ARM64 && !CONFIG_MACH_SUNIV */
> -
>  	ret = axp_gpio_init();
>  	if (ret)
>  		return ret;

Re: [PATCH 4/4] sunxi: move arch timer setup out of board/ directory

[Andre Przywara]

On Wed, 14 Dec 2022 00:14:49 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

thanks for having a look!

> On 12/13/22 18:22, Andre Przywara wrote:
> > At the moment we have an #ifdef-protected routine in
> > board/sunxi/board.c, which sets up the arch timer CNTFRQ register, if
> > that hasn't been done already.
> > This only applies to ARMv7 SoCs running in secure state, and so is
> > much better located in the arch/arm/mach-sunxi directory.
> > 
> > Move that routine into a separate function and file in said directory,
> > which also allows to trigger the compilation for ARMv7 in the Makefile.
> > Also move the call to it into the arch/arm version of board.c.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/mach-sunxi/Makefile     |  4 ++++
> >  arch/arm/mach-sunxi/arch_timer.c | 39 ++++++++++++++++++++++++++++++++
> >  arch/arm/mach-sunxi/board.c      | 14 ++++++++----
> >  board/sunxi/board.c              | 34 +---------------------------
> >  4 files changed, 54 insertions(+), 37 deletions(-)
> >  create mode 100644 arch/arm/mach-sunxi/arch_timer.c
> > 
> > diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> > index 58f807cb82d..335c44476e9 100644
> > --- a/arch/arm/mach-sunxi/Makefile
> > +++ b/arch/arm/mach-sunxi/Makefile
> > @@ -26,8 +26,12 @@ obj-$(CONFIG_MACH_SUN8I)	+= clock_sun6i.o
> >  endif
> >  obj-$(CONFIG_MACH_SUN9I)	+= clock_sun9i.o gtbus_sun9i.o
> >  obj-$(CONFIG_SUN50I_GEN_H6)	+= clock_sun50i_h6.o
> > +
> >  ifndef CONFIG_ARM64
> >  obj-y	+= timer.o
> > +ifndef CONFIG_MACH_SUNIV  
> 
> Probably Allwinner will not release any new ARMv5 SoC, but would it be
> more future-proof (and a bit more meaningful) to condition this on
> CONFIG_CPU_V7A?

Ah, of course, I forgot about that symbol, and just saw !CONFIG_ARM64.

> Alternatively, would it make sense to put this code inside
> arch/arm/cpu/armv7/arch_timer.c? I don't know why we wouldn't want to
> enable CONFIG_SYS_ARCH_TIMER on sunxi SoCs where it is available. And
> making this generic could eventually remove the need for the version in
> arch/arm/cpu/armv7/nonsec_virt.S. But this patch is still an improvement
> as-is.

Yeah, that indeed sounds even better, I will give it a try. I guess
sunxi will stay the only user for now, but we can extend this later on.

Thanks for the suggestion!

Cheers,
Andre

> Tested-by: Samuel Holland <samuel@sholland.org>
> 
> > +obj-y	+= arch_timer.o
> > +endif
> >  endif
> >  
> >  ifdef CONFIG_SPL_BUILD
> > diff --git a/arch/arm/mach-sunxi/arch_timer.c b/arch/arm/mach-sunxi/arch_timer.c
> > new file mode 100644
> > index 00000000000..c0e4310741f
> > --- /dev/null
> > +++ b/arch/arm/mach-sunxi/arch_timer.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * ARM Generic Timer CNTFRQ setup
> > + *
> > + * (C) Copyright 2013 Oliver Schinagl <oliver@schinagl.nl>  
> 
> `git blame` points to cba69eeeaa67d3fb93ec6f3abab1f653abf895a9 and
> d96ebc468d0dff6eb6f069bba03b3f0e33aa22de as the source for this code, so
> this authorship is incorrect.
> 
> > + */
> > +
> > +#include <stdint.h>  
> 
> This does not do what you expect, or really anything at all. The comment
> there suggests using linux/types.h.
> 
> Regards,
> Samuel
> 
> > +#include <log.h>
> > +#include <asm/armv7.h>
> > +
> > +void setup_arch_timer(void)
> > +{
> > +	uint32_t id_pfr1, freq;
> > +
> > +	asm volatile("mrc p15, 0, %0, c0, c1, 1" : "=r"(id_pfr1));
> > +	debug("id_pfr1: 0x%08x\n", id_pfr1);
> > +	/* Generic Timer Extension available? */
> > +	if (((id_pfr1 >> CPUID_ARM_GENTIMER_SHIFT) & 0xf) == 0)
> > +		return;
> > +
> > +	/*
> > +	 * CNTFRQ is a secure register, so we will crash if we try to
> > +	 * write this from the non-secure world (read is OK, though).
> > +	 * In case some bootcode has already set the correct value,
> > +	 * we avoid the risk of writing to it.
> > +	 */
> > +	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r"(freq));
> > +	if (freq == CONFIG_COUNTER_FREQUENCY)
> > +		return;
> > +
> > +	debug("arch timer frequency is %d Hz, should be %d, fixing ...\n",
> > +	      freq, CONFIG_COUNTER_FREQUENCY);
> > +	if (IS_ENABLED(CONFIG_NON_SECURE))
> > +		printf("arch timer frequency is wrong, but cannot adjust it\n");
> > +	else
> > +		asm volatile("mcr p15, 0, %0, c14, c0, 0"
> > +			     : : "r"(CONFIG_COUNTER_FREQUENCY));
> > +}
> > diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> > index 0c4b6dd1ca3..943f89eaa78 100644
> > --- a/arch/arm/mach-sunxi/board.c
> > +++ b/arch/arm/mach-sunxi/board.c
> > @@ -347,10 +347,6 @@ u32 spl_boot_device(void)
> >  	return sunxi_get_boot_device();
> >  }
> >  
> > -__weak void sunxi_sram_init(void)
> > -{
> > -}
> > -
> >  /*
> >   * When booting from an eMMC boot partition, the SPL puts the same boot
> >   * source code into SRAM A1 as when loading the SPL from the normal
> > @@ -434,10 +430,20 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
> >  	return result;
> >  }
> >  
> > +__weak void sunxi_sram_init(void)
> > +{
> > +}
> > +
> > +__weak void setup_arch_timer(void)
> > +{
> > +}
> > +
> >  void board_init_f(ulong dummy)
> >  {
> >  	sunxi_sram_init();
> >  
> > +	setup_arch_timer();
> > +
> >  #if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3
> >  	/* Enable non-secure access to some peripherals */
> >  	tzpc_init();
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 827e545032e..55501fafc38 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -34,9 +34,6 @@
> >  #include <asm/global_data.h>
> >  #include <linux/delay.h>
> >  #include <u-boot/crc.h>
> > -#ifndef CONFIG_ARM64
> > -#include <asm/armv7.h>
> > -#endif
> >  #include <asm/gpio.h>
> >  #include <asm/io.h>
> >  #include <u-boot/crc.h>
> > @@ -187,39 +184,10 @@ enum env_location env_get_location(enum env_operation op, int prio)
> >  /* add board specific code here */
> >  int board_init(void)
> >  {
> > -	__maybe_unused int id_pfr1, ret, satapwr_pin, macpwr_pin;
> > +	int ret, satapwr_pin, macpwr_pin;
> >  
> >  	gd->bd->bi_boot_params = (PHYS_SDRAM_0 + 0x100);
> >  
> > -#if !defined(CONFIG_ARM64) && !defined(CONFIG_MACH_SUNIV)
> > -	asm volatile("mrc p15, 0, %0, c0, c1, 1" : "=r"(id_pfr1));
> > -	debug("id_pfr1: 0x%08x\n", id_pfr1);
> > -	/* Generic Timer Extension available? */
> > -	if ((id_pfr1 >> CPUID_ARM_GENTIMER_SHIFT) & 0xf) {
> > -		uint32_t freq;
> > -
> > -		debug("Setting CNTFRQ\n");
> > -
> > -		/*
> > -		 * CNTFRQ is a secure register, so we will crash if we try to
> > -		 * write this from the non-secure world (read is OK, though).
> > -		 * In case some bootcode has already set the correct value,
> > -		 * we avoid the risk of writing to it.
> > -		 */
> > -		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r"(freq));
> > -		if (freq != CONFIG_COUNTER_FREQUENCY) {
> > -			debug("arch timer frequency is %d Hz, should be %d, fixing ...\n",
> > -			      freq, CONFIG_COUNTER_FREQUENCY);
> > -#ifdef CONFIG_NON_SECURE
> > -			printf("arch timer frequency is wrong, but cannot adjust it\n");
> > -#else
> > -			asm volatile("mcr p15, 0, %0, c14, c0, 0"
> > -				     : : "r"(CONFIG_COUNTER_FREQUENCY));
> > -#endif
> > -		}
> > -	}
> > -#endif /* !CONFIG_ARM64 && !CONFIG_MACH_SUNIV */
> > -
> >  	ret = axp_gpio_init();
> >  	if (ret)
> >  		return ret;  
>