Re: [PATCH 09/11] sunxi: Add support for SUNIV architecture

Andre Przywara
2022-01-26T14:13:34Z

On Tue, 4 Jan 2022 19:35:06 -0500
Jesse Taube <mr.bossman075@gmail.com> wrote:

Hi Jesse,

I was checking some bits and pieces here, so sorry for the delay. I saw
your v2, and will review that ASAP, so that we get one step closer. Please
don't send a v3 before that.

If you have some time, can you also meanwhile check if this series is
bisectable, meaning that every patch compiles? I have the feeling there is
something off, but didn't check it. Pick an H3 and an A64 board, and
compile it after each patch. I can do this as well, if you don't find the
time for this.

In general I am tempted to merge this still in this cycle, since we don't
have other big changes, but we would need to settle this by early next
week then.

See below for more work ;-) (Sorry!)

> From: Icenowy Zheng <icenowy@aosc.io>
>
> Add support for the suniv architecture, which is newer ARM9 SoCs by
> Allwinner. The design of it seems to be a mixture of sun3i, sun4i and
> sun6i.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> ---
> arch/arm/mach-sunxi/Kconfig | 16 +++++++++--
> arch/arm/mach-sunxi/board.c | 31 +++++++++++++++++++--
> arch/arm/mach-sunxi/clock.c | 3 +-
> arch/arm/mach-sunxi/clock_sun6i.c | 46 ++++++++++++++++++++++++++++++-
> arch/arm/mach-sunxi/cpu_info.c | 2 ++
> 5 files changed, 91 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 2c18cf02d1..9bb7717731 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -1,7 +1,8 @@
> if ARCH_SUNXI
>
> config SPL_LDSCRIPT
> - default "arch/arm/cpu/armv7/sunxi/u-boot-spl.lds" if !ARM64
> + default "arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds" if MACH_SUNIV
> + default "arch/arm/cpu/armv7/sunxi/u-boot-spl.lds" if !ARM64 && !MACH_SUNIV
>
> config IDENT_STRING
> default " Allwinner Technology"
> @@ -183,6 +184,12 @@ choice
> prompt "Sunxi SoC Variant"
> optional
>
> +config MACH_SUNIV
> + bool "suniv (Allwinner F1C100s/F1C200s/F1C600/R6)"
> + select CPU_ARM926EJS
> + select SUNXI_GEN_SUN6I
> + select SUPPORT_SPL
> +
> config MACH_SUN4I
> bool "sun4i (Allwinner A10)"
> select CPU_V7A
> @@ -587,6 +594,7 @@ config DRAM_ODT_CORRECTION
> endif
>
> config SYS_CLK_FREQ
> + default 408000000 if MACH_SUNIV
> default 1008000000 if MACH_SUN4I
> default 1008000000 if MACH_SUN5I
> default 1008000000 if MACH_SUN6I
> @@ -598,6 +606,7 @@ config SYS_CLK_FREQ
> default 1008000000 if MACH_SUN50I_H616
>
> config SYS_CONFIG_NAME
> + default "suniv" if MACH_SUNIV
> default "sun4i" if MACH_SUN4I
> default "sun5i" if MACH_SUN5I
> default "sun6i" if MACH_SUN6I
> @@ -805,7 +814,7 @@ config VIDEO_SUNXI
>
> config VIDEO_HDMI
> bool "HDMI output support"
> - depends on VIDEO_SUNXI && !MACH_SUN8I
> + depends on VIDEO_SUNXI && !MACH_SUN8I && !MACH_SUNIV
> default y
> ---help---
> Say Y here to add support for outputting video over HDMI.
> @@ -1005,6 +1014,7 @@ config GMAC_TX_DELAY
> Set the GMAC Transmit Clock Delay Chain value.
>
> config SPL_STACK_R_ADDR
> + default 0x81e00000 if MACH_SUNIV
> default 0x4fe00000 if MACH_SUN4I
> default 0x4fe00000 if MACH_SUN5I
> default 0x4fe00000 if MACH_SUN6I
> @@ -1016,7 +1026,7 @@ config SPL_STACK_R_ADDR
>
> config SPL_SPI_SUNXI
> bool "Support for SPI Flash on Allwinner SoCs in SPL"
> - depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I || MACH_SUNXI_H3_H5 || MACH_SUN50I || MACH_SUN8I_R40 || MACH_SUN50I_H6
> + depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I || MACH_SUNXI_H3_H5 || MACH_SUN50I || MACH_SUN8I_R40 || MACH_SUN50I_H6 || MACH_SUNIV

I think this is premature without the corresponding patch to
spl_spi_sunxi.c.

> help
> Enable support for SPI Flash. This option allows SPL to read from
> sunxi SPI Flash. It uses the same method as the boot ROM, so does
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index 3ef179742c..2fee86b49b 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -86,7 +86,8 @@ static int gpio_init(void)
> sunxi_gpio_set_cfgpin(SUNXI_GPB(22), SUNXI_GPIO_INPUT);
> sunxi_gpio_set_cfgpin(SUNXI_GPB(23), SUNXI_GPIO_INPUT);
> #endif
> -#if defined(CONFIG_MACH_SUN8I) && !defined(CONFIG_MACH_SUN8I_R40)
> +#if (defined(CONFIG_MACH_SUN8I) && !defined(CONFIG_MACH_SUN8I_R40)) || \
> + defined(CONFIG_MACH_SUNIV)
> sunxi_gpio_set_cfgpin(SUNXI_GPF(2), SUN8I_GPF_UART0);
> sunxi_gpio_set_cfgpin(SUNXI_GPF(4), SUN8I_GPF_UART0);
> #else
> @@ -94,6 +95,10 @@ static int gpio_init(void)
> sunxi_gpio_set_cfgpin(SUNXI_GPF(4), SUNXI_GPF_UART0);
> #endif
> sunxi_gpio_set_pull(SUNXI_GPF(4), 1);
> +#elif CONFIG_CONS_INDEX == 1 && defined(CONFIG_MACH_SUNIV)
> + sunxi_gpio_set_cfgpin(SUNXI_GPE(0), SUNIV_GPE_UART0);
> + sunxi_gpio_set_cfgpin(SUNXI_GPE(1), SUNIV_GPE_UART0);
> + sunxi_gpio_set_pull(SUNXI_GPE(1), SUNXI_GPIO_PULL_UP);
> #elif CONFIG_CONS_INDEX == 1 && (defined(CONFIG_MACH_SUN4I) || \
> defined(CONFIG_MACH_SUN7I) || \
> defined(CONFIG_MACH_SUN8I_R40))
> @@ -219,7 +224,8 @@ void s_init(void)
> /* No H3 BSP, boot0 seems to not modify SUNXI_SRAMC_BASE + 0x44 */
> #endif
>
> -#if !defined(CONFIG_ARM_CORTEX_CPU_IS_UP) && !defined(CONFIG_ARM64)
> +#if !defined(CONFIG_ARM_CORTEX_CPU_IS_UP) && !defined(CONFIG_ARM64) && \
> + !defined(CONFIG_MACH_SUNIV)

That looks correct for now, but should become obsolete with my
lowlevel_init cleanup series.

> /* Enable SMP mode for CPU0, by setting bit 6 of Auxiliary Ctl reg */
> asm volatile(
> "mrc p15, 0, r0, c1, c0, 1\n"
> @@ -328,10 +334,31 @@ unsigned long spl_mmc_get_uboot_raw_sector(struct mmc *mmc,
> return sector;
> }
>
> +#ifndef CONFIG_MACH_SUNIV

Can you please flip this around to avoid the negative logic?

> u32 spl_boot_device(void)
> {
> return sunxi_get_boot_device();
> }
> +#else
> +/*
> + * suniv BROM do not pass the boot media type to SPL, so we try with the
> + * boot sequence in BROM: mmc0->spinor->fail.
> + */
> +void board_boot_order(u32 *spl_boot_list)
> +{
> + /*
> + * See the comments above in sunxi_get_boot_device() for information
> + * about FEL boot.
> + */
> + if (!is_boot0_magic(SPL_ADDR + 4)) {
> + spl_boot_list[0] = BOOT_DEVICE_BOARD;
> + return;
> + }
> +
> + spl_boot_list[0] = BOOT_DEVICE_MMC1;

So does that mean that it tries MMC first, even when booted via SPI? So if
there is a *non*-bootable microSD card in, it will read something from
sector 80, and will execute that if this is a FIT or legacy image?

I wonder if we actually have some indication of SPI booting, for instance
the pinmux or clock settings?
Otherwise we would really need to mimic the BROM, and read and verify the
eGON header again, to be reliable.

I might be talked into ignoring this issue for now, if there will be a fix
patch later on.

> + spl_boot_list[1] = BOOT_DEVICE_SPI;
> +}
> +#endif
>
> void board_init_f(ulong dummy)
> {
> diff --git a/arch/arm/mach-sunxi/clock.c b/arch/arm/mach-sunxi/clock.c
> index de7e875298..da3a0eb058 100644
> --- a/arch/arm/mach-sunxi/clock.c
> +++ b/arch/arm/mach-sunxi/clock.c
> @@ -35,7 +35,8 @@ int clock_init(void)
> }
>
> /* These functions are shared between various SoCs so put them here. */
> -#if defined CONFIG_SUNXI_GEN_SUN6I && !defined CONFIG_MACH_SUN9I
> +#if defined CONFIG_SUNXI_GEN_SUN6I && !defined CONFIG_MACH_SUN9I && \
> + !defined CONFIG_MACH_SUNIV
> int clock_twi_onoff(int port, int state)
> {
> struct sunxi_ccm_reg *const ccm =
> diff --git a/arch/arm/mach-sunxi/clock_sun6i.c b/arch/arm/mach-sunxi/clock_sun6i.c
> index 8e84062bd7..b0b3ea4d30 100644
> --- a/arch/arm/mach-sunxi/clock_sun6i.c
> +++ b/arch/arm/mach-sunxi/clock_sun6i.c
> @@ -23,7 +23,8 @@ void clock_init_safe(void)
> struct sunxi_ccm_reg * const ccm =
> (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>
> -#if !defined(CONFIG_MACH_SUNXI_H3_H5) && !defined(CONFIG_MACH_SUN50I)
> +#if !defined(CONFIG_MACH_SUNXI_H3_H5) && !defined(CONFIG_MACH_SUN50I) && \
> + !defined(CONFIG_MACH_SUNIV)
> struct sunxi_prcm_reg * const prcm =
> (struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
>
> @@ -49,9 +50,11 @@ void clock_init_safe(void)
>
> writel(AHB1_ABP1_DIV_DEFAULT, &ccm->ahb1_apb1_div);
>
> +#ifndef CONFIG_MACH_SUNIV
> writel(MBUS_CLK_DEFAULT, &ccm->mbus0_clk_cfg);
> if (IS_ENABLED(CONFIG_MACH_SUN6I))
> writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg);
> +#endif
>
> #if defined(CONFIG_MACH_SUN8I_R40) && defined(CONFIG_SUNXI_AHCI)
> setbits_le32(&ccm->sata_pll_cfg, CCM_SATA_PLL_DEFAULT);
> @@ -87,6 +90,7 @@ void clock_init_uart(void)
> struct sunxi_ccm_reg *const ccm =
> (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>
> +#ifndef CONFIG_MACH_SUNIV

Please again negate and swap the branches.

> /* uart clock source is apb2 */
> writel(APB2_CLK_SRC_OSC24M|
> APB2_CLK_RATE_N_1|
> @@ -102,6 +106,24 @@ void clock_init_uart(void)
> setbits_le32(&ccm->apb2_reset_cfg,
> 1 << (APB2_RESET_UART_SHIFT +
> CONFIG_CONS_INDEX - 1));
> +#else
> + /* suniv doesn't have apb2, so uart clock source is apb1 */
> + writel(PLL6_CFG_DEFAULT, &ccm->pll6_cfg);
> + while (!(readl(&ccm->pll6_cfg) & CCM_PLL6_CTRL_LOCK))
> + ;
> +
> + writel(AHB1_ABP1_DIV_DEFAULT, &ccm->ahb1_apb1_div);

This is done is clock_init_safe() already, which is called before
clock_init_uart(). I feel we should drop it here, not only because
touching a PLL again might cause problems.

> +
> + /* open the clock for uart */
> + setbits_le32(&ccm->apb1_gate,
> + CLK_GATE_OPEN << (APB1_GATE_UART_SHIFT +
> + CONFIG_CONS_INDEX - 1));
> +
> + /* deassert uart reset */
> + setbits_le32(&ccm->apb1_reset_cfg,
> + 1 << (APB1_RESET_UART_SHIFT +
> + CONFIG_CONS_INDEX - 1));
> +#endif
> #else
> /* enable R_PIO and R_UART clocks, and de-assert resets */
> prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_UART);
> @@ -125,10 +147,15 @@ void clock_set_pll1(unsigned int clk)
> }
>
> /* Switch to 24MHz clock while changing PLL1 */
> +#ifndef CONFIG_MACH_SUNIV

Same negation here. And can you please turn those preprocessor guards into
if (IS_ENABLED(CONFIG_MACH_SUNIV)) ...
where applicable? That's always preferred, if all symbols used inside both
branches are defined in either case, as in here, for instance.
As the compiler will see that it's a compile-time constant, it will remove
the non-applicable branch. So the effect on the code is mostly the same,
but both branches are "compile-tested" and it's more readable, as the
nesting is done properly.

> writel(AXI_DIV_3 << AXI_DIV_SHIFT |
> ATB_DIV_2 << ATB_DIV_SHIFT |
> CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT,
> &ccm->cpu_axi_cfg);
> +#else
> + writel(CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT,
> + &ccm->cpu_axi_cfg);
> +#endif
>
> /*
> * sun6i: PLL1 rate = ((24000000 * n * k) >> 0) / m (p is ignored)
> @@ -137,13 +164,26 @@ void clock_set_pll1(unsigned int clk)
> writel(CCM_PLL1_CTRL_EN | CCM_PLL1_CTRL_P(p) |
> CCM_PLL1_CTRL_N(clk / (24000000 * k / m)) |
> CCM_PLL1_CTRL_K(k) | CCM_PLL1_CTRL_M(m), &ccm->pll1_cfg);
> +#ifndef CONFIG_MACH_SUNIV

Guess ...

> sdelay(200);
> +#else
> + /* ARM926EJ-S code does not have sdelay */

I wonder if we should just copy a definition of it to
arch/arm/cpu/arm926ejs. The ARMv7 assembly should just work.

> + volatile int i = 200;
> +
> + while (i > 0)
> + i--;
> +#endif
>
> /* Switch CPU to PLL1 */
> +#ifndef CONFIG_MACH_SUNIV

Negate and if(IS_ENABLED(...)), please.

> writel(AXI_DIV_3 << AXI_DIV_SHIFT |
> ATB_DIV_2 << ATB_DIV_SHIFT |
> CPU_CLK_SRC_PLL1 << CPU_CLK_SRC_SHIFT,
> &ccm->cpu_axi_cfg);
> +#else
> + writel(CPU_CLK_SRC_PLL1 << CPU_CLK_SRC_SHIFT,
> + &ccm->cpu_axi_cfg);
> +#endif
> }
> #endif
>
> @@ -317,7 +357,11 @@ unsigned int clock_get_pll6(void)
> uint32_t rval = readl(&ccm->pll6_cfg);
> int n = ((rval & CCM_PLL6_CTRL_N_MASK) >> CCM_PLL6_CTRL_N_SHIFT) + 1;
> int k = ((rval & CCM_PLL6_CTRL_K_MASK) >> CCM_PLL6_CTRL_K_SHIFT) + 1;
> +#ifndef CONFIG_MACH_SUNIV

Negate and if(IS_ENABLED(...)), please.

> return 24000000 * n * k / 2;
> +#else
> + return 24000000 * n * k;
> +#endif
> }

This whole file is admittedly quite messy. We should be able to move the
video clocks out here and into our DM clock driver, but this is
non-trivial, I believe, so I won't ask you doing this ;-)

Cheers,
Andre

>
> unsigned int clock_get_mipi_pll(void)
> diff --git a/arch/arm/mach-sunxi/cpu_info.c b/arch/arm/mach-sunxi/cpu_info.c
> index ba33ef2430..7eef178859 100644
> --- a/arch/arm/mach-sunxi/cpu_info.c
> +++ b/arch/arm/mach-sunxi/cpu_info.c
> @@ -57,6 +57,8 @@ int print_cpuinfo(void)
> {
> #ifdef CONFIG_MACH_SUN4I
> puts("CPU: Allwinner A10 (SUN4I)\n");
> +#elif defined CONFIG_MACH_SUNIV
> + puts("CPU: Allwinner F Series (SUNIV)\n");
> #elif defined CONFIG_MACH_SUN5I
> u32 val = readl(SUNXI_SID_BASE + 0x08);
> switch ((val >> 12) & 0xf) {