Re: [PATCH 08/11] configs: sunxi: Add common SUNIV header

[Andre Przywara <andre.przywara@arm.com>]

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

Hi,

> From: Icenowy Zheng <icenowy@aosc.io>
> 
> Adds support for SUNIV and the F1C100s.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> ---
>  include/configs/suniv.h        | 14 +++++++
>  include/configs/sunxi-common.h | 67 ++++++++++++++++++++++++----------
>  2 files changed, 62 insertions(+), 19 deletions(-)
>  create mode 100644 include/configs/suniv.h
> 
> diff --git a/include/configs/suniv.h b/include/configs/suniv.h
> new file mode 100644
> index 0000000000..6118cd5e1a
> --- /dev/null
> +++ b/include/configs/suniv.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Configuration settings for new Allwinner F-series (suniv) CPU
> + */
> +
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +/*
> + * Include common sunxi configuration where most the settings are
> + */
> +#include <configs/sunxi-common.h>
> +
> +#endif /* __CONFIG_H */
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 7260eb72a4..62004a09c1 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -22,7 +22,12 @@
>  /* Serial & console */
>  #define CONFIG_SYS_NS16550_SERIAL
>  /* ns16550 reg in the low bits of cpu reg */
> +#ifndef CONFIG_MACH_SUNIV

Can you flip this around to avoid the negative logic?

>  #define CONFIG_SYS_NS16550_CLK		24000000
> +#else
> +/* suniv doesn't have apb2 and uart is connected to apb1 */
> +#define CONFIG_SYS_NS16550_CLK		100000000
> +#endif
>  #ifndef CONFIG_DM_SERIAL
>  # define CONFIG_SYS_NS16550_REG_SIZE	-4
>  # define CONFIG_SYS_NS16550_COM1		SUNXI_UART0_BASE
> @@ -49,6 +54,16 @@
>   * since it needs to fit in with the other values. By also #defining it
>   * we get warnings if the Kconfig value mismatches. */
>  #define CONFIG_SPL_BSS_START_ADDR	0x2ff80000
> +#elif defined(CONFIG_MACH_SUNIV)
> +#define SDRAM_OFFSET(x) 0x8##x
> +#define CONFIG_SYS_SDRAM_BASE		0x80000000
> +#define CONFIG_SYS_LOAD_ADDR		0x81000000 /* default load address */

I am not sure this actually works? SYS_LOAD_ADDR lives in /Kconfig, you
would need to add a line there.

> +/* Note SPL_STACK_R_ADDR is set through Kconfig, we include it here
> + * since it needs to fit in with the other values. By also #defining it
> + * we get warnings if the Kconfig value mismatches.
> + */
> +#define CONFIG_SPL_STACK_R_ADDR		0x81e00000
> +#define CONFIG_SPL_BSS_START_ADDR	0x81f80000
>  #else
>  #define SDRAM_OFFSET(x) 0x4##x
>  #define CONFIG_SYS_SDRAM_BASE		0x40000000
> @@ -109,6 +124,8 @@
>  #endif
>  
>  #define CONFIG_SYS_MMC_MAX_DEVICE	4
> +#elif defined(CONFIG_ENV_IS_IN_SPI_FLASH)
> +#define CONFIG_ENV_SECT_SIZE 0x1000

This lives in Kconfig, and should not be needed anyway. I think this
gets covered by my SPI fix series? Please remove those two lines.

>  #endif
>  
>  /*
> @@ -187,32 +204,44 @@
>  #define RAMDISK_ADDR_R    __stringify(SDRAM_OFFSET(FF00000))
>  
>  #else
> -/*
> - * 160M RAM (256M minimum minus 64MB heap + 32MB for u-boot, stack, fb, etc.
> - * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
> - * 1M script, 1M pxe, 1M dt overlay and the ramdisk at the end.
> - */
> -#ifndef CONFIG_MACH_SUN8I_V3S
> -#define BOOTM_SIZE        __stringify(0xa000000)
> -#define KERNEL_ADDR_R     __stringify(SDRAM_OFFSET(2000000))
> -#define FDT_ADDR_R        __stringify(SDRAM_OFFSET(3000000))
> -#define SCRIPT_ADDR_R     __stringify(SDRAM_OFFSET(3100000))
> -#define PXEFILE_ADDR_R    __stringify(SDRAM_OFFSET(3200000))
> -#define FDTOVERLAY_ADDR_R __stringify(SDRAM_OFFSET(3300000))
> -#define RAMDISK_ADDR_R    __stringify(SDRAM_OFFSET(3400000))
> -#else
> +#if defined(CONFIG_MACH_SUN8I_V3S)

Please merge this with the #else above into an #elif.

>  /*
>   * 64M RAM minus 2MB heap + 16MB for u-boot, stack, fb, etc.
>   * 16M uncompressed kernel, 8M compressed kernel, 1M fdt,
>   * 1M script, 1M pxe, 1M dt overlay and the ramdisk at the end.
>   */
> -#define BOOTM_SIZE        __stringify(0x2e00000)
> -#define KERNEL_ADDR_R     __stringify(SDRAM_OFFSET(1000000))
> -#define FDT_ADDR_R        __stringify(SDRAM_OFFSET(1800000))
> -#define SCRIPT_ADDR_R     __stringify(SDRAM_OFFSET(1900000))
> -#define PXEFILE_ADDR_R    __stringify(SDRAM_OFFSET(1A00000))
> +#define BOOTM_SIZE     __stringify(0x2e00000)
> +#define KERNEL_ADDR_R  __stringify(SDRAM_OFFSET(1000000))
> +#define FDT_ADDR_R     __stringify(SDRAM_OFFSET(1800000))
> +#define SCRIPT_ADDR_R  __stringify(SDRAM_OFFSET(1900000))

This part should vanish: the alignment is off, and PXEFILE_ADDR_R got
dropped. I guess a rebase artefact ...

>  #define FDTOVERLAY_ADDR_R __stringify(SDRAM_OFFSET(1B00000))
>  #define RAMDISK_ADDR_R    __stringify(SDRAM_OFFSET(1C00000))
> +#elif defined(CONFIG_MACH_SUNIV)

Please add an empty line above that one, to separate the SoC classes.

> +/*
> + * 32M RAM minus 1MB heap + 8MB for u-boot, stack, fb, etc.
> + * 8M uncompressed kernel, 4M compressed kernel, 512K fdt,
> + * 512K script, 512K pxe and the ramdisk at the end.

This comment does not match the values below (which are also not
vertically aligned to the rest, as above).
compressed kernel is 7M, 320K fdt, 707K script, 320K PXE, 64K dt
overlay.
I suggest to swap the script and PXE sizes.

> + */
> +#define BOOTM_SIZE     __stringify(0x1700000)
> +#define KERNEL_ADDR_R  __stringify(SDRAM_OFFSET(0500000))
> +#define FDT_ADDR_R     __stringify(SDRAM_OFFSET(0C00000))
> +#define SCRIPT_ADDR_R  __stringify(SDRAM_OFFSET(0C50000))
> +#define PXEFILE_ADDR_R __stringify(SDRAM_OFFSET(0D00000))
> +#define FDTOVERLAY_ADDR_R __stringify(SDRAM_OFFSET(0D50000))
> +#define RAMDISK_ADDR_R __stringify(SDRAM_OFFSET(0D60000))
> +#else

Same here, an empty line above, so that it reads
  ...
  < previous definitions>
  <empty line>
  #elif/#else
  /*
   * comment ....
  ...

> +/*
> + * 160M RAM (256M minimum minus 64MB heap + 32MB for u-boot, stack, fb, etc.
> + * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
> + * 1M script, 1M pxe and the ramdisk at the end.
> + */
> +#define BOOTM_SIZE     __stringify(0xa000000)
> +#define KERNEL_ADDR_R  __stringify(SDRAM_OFFSET(2000000))
> +#define FDT_ADDR_R     __stringify(SDRAM_OFFSET(3000000))
> +#define SCRIPT_ADDR_R  __stringify(SDRAM_OFFSET(3100000))
> +#define PXEFILE_ADDR_R __stringify(SDRAM_OFFSET(3200000))
> +#define FDTOVERLAY_ADDR_R __stringify(SDRAM_OFFSET(3300000))
> +#define RAMDISK_ADDR_R    __stringify(SDRAM_OFFSET(3400000))

Again, this breaks the vertical alignment. Ideally it would vanish from
the diff.

Cheers,
Andre


>  #endif
>  #endif
>