[U-Boot] [PATCH v2 00/12] System Firmware Loader for TI K3 family SoCs

[U-Boot] [PATCH v2 00/12] System Firmware Loader for TI K3 family SoCs

[Andreas Dannenberg]

Updated version of the SYSFW loader series for K3 family AM654x devices.
The fundamantal approach of tapping into the SPL loader framework has
been kept for reasons discussed already. The series also still uses
"early BSS" in SPL's board_init_f(). I'm well aware of the concerns
previously brought up regarding this mainly by Simon Glass but I have
not been able to find a better / more universal solution for this yet
(one proposal was to move SYSFW loading into board_init_r() which is not
easily solvable as SYSFW is needed to bring up DDR on K3 SoCs). Long
story short I propose to consider the current proposed approach
nevertheless (as it is also used by other platforms) at least as an
initial step, and then migrate once a better solution is available.

I have not yet included support for TI's newest K3 family J721E SoC
which Lokesh posted an initial patch series [5] for due to the
complex dependencies of all the different series we have currently
posted/pending (if I were to add support for J721E which eventually will
be required then the SYSFW loader series would have Lokesh's series as a
pre-requisite as well).

This being said I would like to propose the following staging sequence
for the different TI K3 SoCs patches currently under review:

Step 1) Faiz' "Add Support for eMMC in Am65x-evm" series [1]. It needs
        a small update to actuall allow for eMMC boot I posted earlier.
Step 2) The SYSFW loader series proposed here
Step 3) An updated version (v3) of the AM654x EEPROM support [3].
        Will post this today. 
Step 4) An updated version of Lokesh's "arm: k3: Allow for exclusive
        and shared device requests" series. In addition to a rebase
        such an updated series should include updating power domain
        properties for devices that were added during the previous
        steps.
Step 5) An updated version of Lokesh's "arm: k3: arm64: Initial support
        Texas Instrument's J721E Platform" series [5] also adding
        in the few lines of codes to leverage SYSFW.
Step 6 & beyond) Various rproc patches, etc.


The above is to allow for things to build in a logical order while
avoiding merge conflicts. 1+2+3 added will provide a pretty good initial
working U-Boot using eMMC and SD media for AM654x which is a device
available today plus a foundation for everything else, hence it is at
the top of the list.


Changes since initial submission:
- Dropped patch "armv7R: dts: k3: am654: Update mmc nodes for loading
  sysfw". This is taken care off by the "Add Support for eMMC in
  Am65x-evm" patch series [1] which this series was rebased on.
- Replaced patch "spl: Allow skipping clearing BSS during relocation"
  with a functionally equivalent patch "spl: Allow performing BSS init
  early before board_init_f()" which is a bit more elegant solution
  which itself is a slight evolution what previously posted by Simon
  Goldschmidt [2]
- Collected various review tags


[1] https://patchwork.ozlabs.org/project/uboot/list/?series=111723
[2] https://patchwork.ozlabs.org/patch/1067363/
[3] https://patchwork.ozlabs.org/project/uboot/list/?series=109266
[4] https://patchwork.ozlabs.org/project/uboot/list/?series=109163
[5] https://patchwork.ozlabs.org/project/uboot/list/?series=109296


--
Andreas Dannenberg
Texas Instruments Inc


Andreas Dannenberg (10):
  mmc: am654_sdhci: Allow driver to probe without PDs specified
  spl: Allow performing BSS init early before board_init_f()
  spl: Make image loader infrastructure more universal
  arm: K3: Introduce System Firmware loader framework
  armV7R: K3: am654: Allow using SPL BSS pre-relocation
  armv7R: K3: am654: Use full malloc implementation in SPL
  armV7R: K3: am654: Load SYSFW binary and config from boot media
  configs: am65x_evm_r5: All sysfw to be loaded via MMC
  configs: am65x_hs_evm_r5: All sysfw to be loaded via MMC
  configs: am65x_hs_evm: Add Support for eMMC boot

Faiz Abbas (2):
  configs: am65x_evm: Add Support for eMMC boot
  am65x: README: Add eMMC layout and flash instructions

 arch/arm/lib/crt0.S                          |  53 ++--
 arch/arm/mach-k3/Kconfig                     |  39 +++
 arch/arm/mach-k3/Makefile                    |   3 +
 arch/arm/mach-k3/am6_init.c                  |  27 +-
 arch/arm/mach-k3/include/mach/sysfw-loader.h |  12 +
 arch/arm/mach-k3/sysfw-loader.c              | 260 +++++++++++++++++++
 board/ti/am65x/Kconfig                       |   1 +
 board/ti/am65x/README                        |  52 ++++
 common/spl/Kconfig                           |  10 +
 common/spl/spl_fit.c                         |  14 +
 common/spl/spl_mmc.c                         |  76 ++++--
 configs/am65x_evm_a53_defconfig              |   2 +
 configs/am65x_evm_r5_defconfig               |   7 +-
 configs/am65x_hs_evm_a53_defconfig           |   2 +
 configs/am65x_hs_evm_r5_defconfig            |   7 +-
 drivers/mmc/am654_sdhci.c                    |  16 +-
 include/configs/am65x_evm.h                  |  30 ++-
 include/spl.h                                |  26 ++
 18 files changed, 580 insertions(+), 57 deletions(-)
 create mode 100644 arch/arm/mach-k3/include/mach/sysfw-loader.h
 create mode 100644 arch/arm/mach-k3/sysfw-loader.c

-- 
2.17.1

[U-Boot] [PATCH v2 01/12] mmc: am654_sdhci: Allow driver to probe without PDs specified

[Andreas Dannenberg]

We would like to use the driver even without power domains being
specified for cases such as during early boot when the required power
domains have already gotten enabled by the SoC's boot ROM and such
explicit initialization is not needed and possible.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
 drivers/mmc/am654_sdhci.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index fe633aa39a..fb0fb58070 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -215,14 +215,14 @@ static int am654_sdhci_probe(struct udevice *dev)
 	int ret;
 
 	ret = power_domain_get_by_index(dev, &sdhci_pwrdmn, 0);
-	if (ret) {
-		dev_err(dev, "failed to get power domain\n");
-		return ret;
-	}
-
-	ret = power_domain_on(&sdhci_pwrdmn);
-	if (ret) {
-		dev_err(dev, "Power domain on failed\n");
+	if (!ret) {
+		ret = power_domain_on(&sdhci_pwrdmn);
+		if (ret) {
+			dev_err(dev, "Power domain on failed (%d)\n", ret);
+			return ret;
+		}
+	} else if (ret != -ENOENT && ret != -ENODEV && ret != -ENOSYS) {
+		dev_err(dev, "failed to get power domain (%d)\n", ret);
 		return ret;
 	}
 
-- 
2.17.1

[U-Boot] [PATCH v2 02/12] spl: Allow performing BSS init early before board_init_f()

[Andreas Dannenberg]

On some platform we have sufficient memory available early on to allow
setting up and using a basic BSS prior to entering board_init_f(). Doing
so can for example be used to carry state over to board_init_r() without
having to resort to extending U-Boot's global data structure.

To support such scenarios add a Kconfig option called CONFIG_SPL_EARLY_BSS
to allow moving the initialization of BSS prior to entering board_init_f(),
if enabled. Note that using this option usually should go along with using
CONFIG_SPL_SEPARATE_BSS and configuring BSS to be located in memory
actually available prior to board_init_f().

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 arch/arm/lib/crt0.S | 53 ++++++++++++++++++++++++++++++---------------
 common/spl/Kconfig  | 10 +++++++++
 2 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 30fba20e1b..c74641dcd9 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -57,6 +57,33 @@
  * For more information see 'Board Initialisation Flow in README.
  */
 
+/*
+ * Macro for clearing BSS during SPL execution. Usually called during the
+ * relocation process for most boards before entering board_init_r(), but
+ * can also be done early before entering board_init_f() on plaforms that
+ * can afford it due to sufficient memory being available early.
+ */
+
+.macro SPL_CLEAR_BSS
+	ldr	r0, =__bss_start	/* this is auto-relocated! */
+
+#ifdef CONFIG_USE_ARCH_MEMSET
+	ldr	r3, =__bss_end		/* this is auto-relocated! */
+	mov	r1, #0x00000000		/* prepare zero to clear BSS */
+
+	subs	r2, r3, r0		/* r2 = memset len */
+	bl	memset
+#else
+	ldr	r1, =__bss_end		/* this is auto-relocated! */
+	mov	r2, #0x00000000		/* prepare zero to clear BSS */
+
+clbss_l:cmp	r0, r1			/* while not at end of BSS */
+	strlo	r2, [r0]		/* clear 32-bit BSS word */
+	addlo	r0, r0, #4		/* move to next */
+	blo	clbss_l
+#endif
+.endm
+
 /*
  * entry point of crt0 sequence
  */
@@ -82,6 +109,10 @@ ENTRY(_main)
 	mov	r9, r0
 	bl	board_init_f_init_reserve
 
+#if defined(CONFIG_SPL_EARLY_BSS)
+	SPL_CLEAR_BSS
+#endif
+
 	mov	r0, #0
 	bl	board_init_f
 
@@ -119,6 +150,11 @@ here:
 	bl	c_runtime_cpu_setup	/* we still call old routine here */
 #endif
 #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_FRAMEWORK)
+
+#if !defined(CONFIG_SPL_EARLY_BSS)
+	SPL_CLEAR_BSS
+#endif
+
 # ifdef CONFIG_SPL_BUILD
 	/* Use a DRAM stack for the rest of SPL, if requested */
 	bl	spl_relocate_stack_gd
@@ -126,23 +162,6 @@ here:
 	movne	sp, r0
 	movne	r9, r0
 # endif
-	ldr	r0, =__bss_start	/* this is auto-relocated! */
-
-#ifdef CONFIG_USE_ARCH_MEMSET
-	ldr	r3, =__bss_end		/* this is auto-relocated! */
-	mov	r1, #0x00000000		/* prepare zero to clear BSS */
-
-	subs	r2, r3, r0		/* r2 = memset len */
-	bl	memset
-#else
-	ldr	r1, =__bss_end		/* this is auto-relocated! */
-	mov	r2, #0x00000000		/* prepare zero to clear BSS */
-
-clbss_l:cmp	r0, r1			/* while not at end of BSS */
-	strlo	r2, [r0]		/* clear 32-bit BSS word */
-	addlo	r0, r0, #4		/* move to next */
-	blo	clbss_l
-#endif
 
 #if ! defined(CONFIG_SPL_BUILD)
 	bl coloured_LED_init
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index c7cd34449a..b0460e1d17 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -188,6 +188,16 @@ config TPL_BANNER_PRINT
 	  info. Disabling this option could be useful to reduce SPL boot time
 	  (e.g. approx. 6 ms faster, when output on i.MX6 with 115200 baud).
 
+config SPL_EARLY_BSS
+	depends on ARM && !ARM64
+	bool "Allows initializing BSS early before entering board_init_f"
+	help
+	  On some platform we have sufficient memory available early on to
+	  allow setting up and using a basic BSS prior to entering
+	  board_init_f. Activating this option will also de-activate the
+	  clearing of BSS during the SPL relocation process, thus allowing
+	  to carry state from board_init_f to board_init_r by way of BSS.
+
 config SPL_DISPLAY_PRINT
 	bool "Display a board-specific message in SPL"
 	help
-- 
2.17.1

[U-Boot] [PATCH v2 03/12] spl: Make image loader infrastructure more universal

[Andreas Dannenberg]

The current U-Boot SPL image loader infrastructure is very powerful,
able to initialize and load from a variety of boot media however it
is strongly geared towards loading specific types of images in a very
specific way. To address the need being able to use this infrastructure
to load arbitrary image files go ahead and refactor it as follows:

- Refactor existing spl_mmc_load_image function into superset function,
  accepting additional arguments such as filenames and media load offset
  (same concept can also be applied toother spl_XXX_load_image functions)
- Extend the loader function to "remember" their peripheral initialization
  status so that the init is only done once during the boot process,
- Extend the FIT image loading function to allow skipping the parsing/
  processing of the FIT contents (so that this can be done separately
  in a more customized fashion)
- Populate the SPL_LOAD_IMAGE_METHOD() list with a trampoline function,
  invoking the newly refactored superset functions in a way to maintain
  compatibility with the existing behavior

This refactoring initially covers MMC/SD card loading (RAW and FS-based).

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
 common/spl/spl_fit.c | 14 ++++++++
 common/spl/spl_mmc.c | 76 +++++++++++++++++++++++++++++---------------
 include/spl.h        | 26 +++++++++++++++
 3 files changed, 91 insertions(+), 25 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 87ecf0bb9e..969f7775c1 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -340,6 +340,16 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 #endif
 }
 
+/*
+ * Weak default function to allow customizing SPL fit loading for load-only
+ * use cases by allowing to skip the parsing/processing of the FIT contents
+ * (so that this can be done separately in a more customized fashion)
+ */
+__weak bool spl_load_simple_fit_skip_processing(void)
+{
+	return false;
+}
+
 int spl_load_simple_fit(struct spl_image_info *spl_image,
 			struct spl_load_info *info, ulong sector, void *fit)
 {
@@ -389,6 +399,10 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (count == 0)
 		return -EIO;
 
+	/* skip further processing if requested to enable load-only use cases */
+	if (spl_load_simple_fit_skip_processing())
+		return 0;
+
 	/* find the node holding the images information */
 	images = fdt_path_offset(fit, FIT_IMAGES_PATH);
 	if (images < 0) {
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 324d91c884..b3619889f7 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -151,7 +151,8 @@ static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
 static int mmc_load_image_raw_partition(struct spl_image_info *spl_image,
-					struct mmc *mmc, int partition)
+					struct mmc *mmc, int partition,
+					unsigned long sector)
 {
 	disk_partition_t info;
 	int err;
@@ -180,8 +181,7 @@ static int mmc_load_image_raw_partition(struct spl_image_info *spl_image,
 	}
 
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR
-	return mmc_load_image_raw_sector(spl_image, mmc,
-			info.start + CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
+	return mmc_load_image_raw_sector(spl_image, mmc, info.start + sector);
 #else
 	return mmc_load_image_raw_sector(spl_image, mmc, info.start);
 #endif
@@ -234,7 +234,8 @@ static int mmc_load_image_raw_os(struct spl_image_info *spl_image,
 #endif
 
 #ifdef CONFIG_SYS_MMCSD_FS_BOOT_PARTITION
-static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc)
+static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc,
+			      const char *filename)
 {
 	int err = -ENOSYS;
 
@@ -248,7 +249,7 @@ static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc)
 #ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
 	err = spl_load_image_fat(spl_image, mmc_get_blk_desc(mmc),
 				 CONFIG_SYS_MMCSD_FS_BOOT_PARTITION,
-				 CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
+				 filename);
 	if (!err)
 		return err;
 #endif
@@ -263,7 +264,7 @@ static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc)
 #ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
 	err = spl_load_image_ext(spl_image, mmc_get_blk_desc(mmc),
 				 CONFIG_SYS_MMCSD_FS_BOOT_PARTITION,
-				 CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
+				 filename);
 	if (!err)
 		return err;
 #endif
@@ -276,7 +277,8 @@ static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc)
 	return err;
 }
 #else
-static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc)
+static int spl_mmc_do_fs_boot(struct spl_image_info *spl_image, struct mmc *mmc,
+			      const char *filename)
 {
 	return -ENOSYS;
 }
@@ -301,24 +303,31 @@ int spl_boot_partition(const u32 boot_device)
 }
 #endif
 
-int spl_mmc_load_image(struct spl_image_info *spl_image,
-		       struct spl_boot_device *bootdev)
+int spl_mmc_load(struct spl_image_info *spl_image,
+		 struct spl_boot_device *bootdev,
+		 const char *filename,
+		 int raw_part,
+		 unsigned long raw_sect)
 {
-	struct mmc *mmc = NULL;
+	static struct mmc *mmc;
 	u32 boot_mode;
 	int err = 0;
 	__maybe_unused int part;
 
-	err = spl_mmc_find_device(&mmc, bootdev->boot_device);
-	if (err)
-		return err;
+	/* Perform peripheral init only once */
+	if (!mmc) {
+		err = spl_mmc_find_device(&mmc, bootdev->boot_device);
+		if (err)
+			return err;
 
-	err = mmc_init(mmc);
-	if (err) {
+		err = mmc_init(mmc);
+		if (err) {
+			mmc = NULL;
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("spl: mmc init failed with error: %d\n", err);
+			printf("spl: mmc init failed with error: %d\n", err);
 #endif
-		return err;
+			return err;
+		}
 	}
 
 	boot_mode = spl_boot_mode(bootdev->boot_device);
@@ -356,17 +365,13 @@ int spl_mmc_load_image(struct spl_image_info *spl_image,
 				return err;
 		}
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
-		err = spl_boot_partition(bootdev->boot_device);
-		if (!err)
-			return err;
-
-		err = mmc_load_image_raw_partition(spl_image, mmc, err);
+		err = mmc_load_image_raw_partition(spl_image, mmc, raw_part,
+						   raw_sect);
 		if (!err)
 			return err;
 #endif
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR
-		err = mmc_load_image_raw_sector(spl_image, mmc,
-			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
+		err = mmc_load_image_raw_sector(spl_image, mmc, raw_sect);
 		if (!err)
 			return err;
 #endif
@@ -374,7 +379,7 @@ int spl_mmc_load_image(struct spl_image_info *spl_image,
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
 
-		err = spl_mmc_do_fs_boot(spl_image, mmc);
+		err = spl_mmc_do_fs_boot(spl_image, mmc, filename);
 		if (!err)
 			return err;
 
@@ -388,6 +393,27 @@ int spl_mmc_load_image(struct spl_image_info *spl_image,
 	return err;
 }
 
+int spl_mmc_load_image(struct spl_image_info *spl_image,
+		       struct spl_boot_device *bootdev)
+{
+	return spl_mmc_load(spl_image, bootdev,
+#ifdef CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
+			    CONFIG_SPL_FS_LOAD_PAYLOAD_NAME,
+#else
+			    NULL,
+#endif
+#ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
+			    spl_boot_partition(bootdev->boot_device),
+#else
+			    0,
+#endif
+#ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR
+			    CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
+#else
+			    0);
+#endif
+}
+
 SPL_LOAD_IMAGE_METHOD("MMC1", 0, BOOT_DEVICE_MMC1, spl_mmc_load_image);
 SPL_LOAD_IMAGE_METHOD("MMC2", 0, BOOT_DEVICE_MMC2, spl_mmc_load_image);
 SPL_LOAD_IMAGE_METHOD("MMC2_2", 0, BOOT_DEVICE_MMC2_2, spl_mmc_load_image);
diff --git a/include/spl.h b/include/spl.h
index a9aaef345f..a90f971a23 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -108,6 +108,15 @@ struct spl_load_info {
  */
 binman_sym_extern(ulong, u_boot_any, image_pos);
 
+/**
+ * spl_load_simple_fit_skip_processing() - Hook to allow skipping the FIT
+ *	image processing during spl_load_simple_fit().
+ *
+ * Return true to skip FIT processing, false to preserve the full code flow
+ * of spl_load_simple_fit().
+ */
+bool spl_load_simple_fit_skip_processing(void);
+
 /**
  * spl_load_simple_fit() - Loads a fit image from a device.
  * @spl_image:	Image description to set up
@@ -330,6 +339,23 @@ int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr);
 int spl_mmc_load_image(struct spl_image_info *spl_image,
 		       struct spl_boot_device *bootdev);
 
+/**
+ * spl_mmc_load() - Load an image file from MMC/SD media
+ *
+ * @param spl_image	Image data filled in by loading process
+ * @param bootdev	Describes which device to load from
+ * @param filename	Name of file to load (in FS mode)
+ * @param raw_part	Partition to load from (in RAW mode)
+ * @param raw_sect	Sector to load from (in RAW mode)
+ *
+ * @return 0 on success, otherwise error code
+ */
+int spl_mmc_load(struct spl_image_info *spl_image,
+		 struct spl_boot_device *bootdev,
+		 const char *filename,
+		 int raw_part,
+		 unsigned long raw_sect);
+
 /**
  * spl_invoke_atf - boot using an ARM trusted firmware image
  */
-- 
2.17.1

[U-Boot] [PATCH v2 04/12] arm: K3: Introduce System Firmware loader framework

[Andreas Dannenberg]

Introduce a framework that allows loading the System Firmware (SYSFW)
binary as well as the associated configuration data from an image tree
blob named "sysfw.itb" from an FS-based MMC boot media or from an MMC
RAW mode partition or sector.

To simplify the handling of and loading from the different boot media
we tap into the existing U-Boot SPL framework usually used for loading
U-Boot by building on an earlier commit that exposes some of that
functionality.

Note that this initial implementation only supports FS and RAW-based
eMMC/SD card boot.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/mach-k3/Kconfig                     |  39 +++
 arch/arm/mach-k3/Makefile                    |   3 +
 arch/arm/mach-k3/include/mach/sysfw-loader.h |  12 +
 arch/arm/mach-k3/sysfw-loader.c              | 260 +++++++++++++++++++
 4 files changed, 314 insertions(+)
 create mode 100644 arch/arm/mach-k3/include/mach/sysfw-loader.h
 create mode 100644 arch/arm/mach-k3/sysfw-loader.c

diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
index e677a2e01b..f25f822205 100644
--- a/arch/arm/mach-k3/Kconfig
+++ b/arch/arm/mach-k3/Kconfig
@@ -58,6 +58,45 @@ config SYS_K3_BOOT_CORE_ID
 	int
 	default 16
 
+config K3_LOAD_SYSFW
+	bool
+	depends on SPL
+
+config K3_SYSFW_IMAGE_NAME
+	string "File name of SYSFW firmware and configuration blob"
+	depends on K3_LOAD_SYSFW
+	default	"sysfw.itb"
+	help
+	  Filename of the combined System Firmware and configuration image tree
+	  blob to be loaded when booting from a filesystem.
+
+config K3_SYSFW_IMAGE_MMCSD_RAW_MODE_SECT
+	hex "MMC sector to load SYSFW firmware and configuration blob from"
+	depends on K3_LOAD_SYSFW && SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR
+	default 0x3600
+	help
+	  Address on the MMC to load the combined System Firmware and
+	  configuration image tree blob from, when the MMC is being used
+	  in raw mode. Units: MMC sectors (1 sector = 512 bytes).
+
+config K3_SYSFW_IMAGE_MMCSD_RAW_MODE_PART
+	hex "MMC partition to load SYSFW firmware and configuration blob from"
+	depends on K3_LOAD_SYSFW && SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
+	default 2
+	help
+	  Partition on the MMC to the combined System Firmware and configuration
+	  image tree blob from, when the MMC is being used in raw mode.
+
+config K3_SYSFW_IMAGE_SIZE_MAX
+	int "Amount of memory dynamically allocated for loading SYSFW blob"
+	depends on K3_LOAD_SYSFW
+	default	269000
+	help
+	  Amount of memory (in bytes) reserved through dynamic allocation at
+	  runtime for loading the combined System Firmware and configuration image
+	  tree blob. Keep it as tight as possible, as this directly affects the
+	  overall SPL memory footprint.
+
 config SYS_K3_SPL_ATF
 	bool "Start Cortex-A from SPL"
 	depends on SPL && CPU_V7R
diff --git a/arch/arm/mach-k3/Makefile b/arch/arm/mach-k3/Makefile
index 0c3a4f7db1..3af7f2ec96 100644
--- a/arch/arm/mach-k3/Makefile
+++ b/arch/arm/mach-k3/Makefile
@@ -7,4 +7,7 @@ obj-$(CONFIG_SOC_K3_AM6) += am6_init.o
 obj-$(CONFIG_ARM64) += arm64-mmu.o
 obj-$(CONFIG_CPU_V7R) += r5_mpu.o lowlevel_init.o
 obj-$(CONFIG_TI_SECURE_DEVICE) += security.o
+ifeq ($(CONFIG_SPL_BUILD),y)
+obj-$(CONFIG_K3_LOAD_SYSFW) += sysfw-loader.o
+endif
 obj-y += common.o
diff --git a/arch/arm/mach-k3/include/mach/sysfw-loader.h b/arch/arm/mach-k3/include/mach/sysfw-loader.h
new file mode 100644
index 0000000000..36eb265348
--- /dev/null
+++ b/arch/arm/mach-k3/include/mach/sysfw-loader.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andreas Dannenberg <dannenberg@ti.com>
+ */
+
+#ifndef _SYSFW_LOADER_H_
+#define _SYSFW_LOADER_H_
+
+void k3_sysfw_loader(void (*config_pm_done_callback)(void));
+
+#endif
diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c
new file mode 100644
index 0000000000..2ede82004a
--- /dev/null
+++ b/arch/arm/mach-k3/sysfw-loader.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * K3: System Firmware Loader
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andreas Dannenberg <dannenberg@ti.com>
+ */
+
+#include <common.h>
+#include <spl.h>
+#include <malloc.h>
+#include <remoteproc.h>
+#include <linux/soc/ti/ti_sci_protocol.h>
+#include <asm/arch/sys_proto.h>
+
+/* Name of the FIT image nodes for SYSFW and its config data */
+#define SYSFW_FIRMWARE			"sysfw.bin"
+#define SYSFW_CFG_BOARD			"board-cfg.bin"
+#define SYSFW_CFG_PM			"pm-cfg.bin"
+#define SYSFW_CFG_RM			"rm-cfg.bin"
+#define SYSFW_CFG_SEC			"sec-cfg.bin"
+
+static bool sysfw_loaded;
+static void *sysfw_load_address;
+
+/*
+ * Populate SPL hook to override the default load address used by the SPL
+ * loader function with a custom address for SYSFW loading.
+ */
+struct image_header *spl_get_load_buffer(ssize_t offset, size_t size)
+{
+	if (sysfw_loaded)
+		return (struct image_header *)(CONFIG_SYS_TEXT_BASE + offset);
+	else if (sysfw_load_address)
+		return sysfw_load_address;
+	else
+		panic("SYSFW load address not defined!");
+}
+
+/*
+ * Populate SPL hook to skip the default SPL loader FIT post-processing steps
+ * during SYSFW loading and return to the calling function so we can perform
+ * our own custom processing.
+ */
+bool spl_load_simple_fit_skip_processing(void)
+{
+	return !sysfw_loaded;
+}
+
+static int fit_get_data_by_name(const void *fit, int images, const char *name,
+				const void **addr, size_t *size)
+{
+	int node_offset;
+
+	node_offset = fdt_subnode_offset(fit, images, name);
+	if (node_offset < 0)
+		return -ENOENT;
+
+	return fit_image_get_data(fit, node_offset, addr, size);
+}
+
+static void k3_sysfw_load_using_fit(void *fit)
+{
+	int images;
+	const void *sysfw_addr;
+	size_t sysfw_size;
+	int ret;
+
+	/* Find the node holding the images information */
+	images = fdt_path_offset(fit, FIT_IMAGES_PATH);
+	if (images < 0)
+		panic("Cannot find /images node (%d)\n", images);
+
+	/* Extract System Firmware (SYSFW) image from FIT */
+	ret = fit_get_data_by_name(fit, images, SYSFW_FIRMWARE,
+				   &sysfw_addr, &sysfw_size);
+	if (ret < 0)
+		panic("Error accessing %s node in FIT (%d)\n", SYSFW_FIRMWARE,
+		      ret);
+
+	/*
+	 * Start up system controller firmware
+	 *
+	 * It is assumed that remoteproc device 0 is the corresponding
+	 * system-controller that runs SYSFW. Make sure DT reflects the same.
+	 */
+	ret = rproc_dev_init(0);
+	if (ret)
+		panic("rproc failed to be initialized (%d)\n", ret);
+
+	ret = rproc_load(0, (ulong)sysfw_addr, (ulong)sysfw_size);
+	if (ret)
+		panic("Firmware failed to start on rproc (%d)\n", ret);
+
+	ret = rproc_start(0);
+	if (ret)
+		panic("Firmware init failed on rproc (%d)\n", ret);
+}
+
+static void k3_sysfw_configure_using_fit(void *fit,
+					 struct ti_sci_handle *ti_sci)
+{
+	struct ti_sci_board_ops *board_ops = &ti_sci->ops.board_ops;
+	int images;
+	const void *cfg_fragment_addr;
+	size_t cfg_fragment_size;
+	int ret;
+
+	/* Find the node holding the images information */
+	images = fdt_path_offset(fit, FIT_IMAGES_PATH);
+	if (images < 0)
+		panic("Cannot find /images node (%d)\n", images);
+
+	/* Extract board configuration from FIT */
+	ret = fit_get_data_by_name(fit, images, SYSFW_CFG_BOARD,
+				   &cfg_fragment_addr, &cfg_fragment_size);
+	if (ret < 0)
+		panic("Error accessing %s node in FIT (%d)\n", SYSFW_CFG_BOARD,
+		      ret);
+
+	/* Apply board configuration to SYSFW */
+	ret = board_ops->board_config(ti_sci,
+				      (u64)(u32)cfg_fragment_addr,
+				      (u32)cfg_fragment_size);
+	if (ret)
+		panic("Failed to set board configuration (%d)\n", ret);
+
+	/* Extract power/clock (PM) specific configuration from FIT */
+	ret = fit_get_data_by_name(fit, images, SYSFW_CFG_PM,
+				   &cfg_fragment_addr, &cfg_fragment_size);
+	if (ret < 0)
+		panic("Error accessing %s node in FIT (%d)\n", SYSFW_CFG_PM,
+		      ret);
+
+	/* Apply power/clock (PM) specific configuration to SYSFW */
+	ret = board_ops->board_config_pm(ti_sci,
+					 (u64)(u32)cfg_fragment_addr,
+					 (u32)cfg_fragment_size);
+	if (ret)
+		panic("Failed to set board PM configuration (%d)\n", ret);
+
+	/* Extract resource management (RM) specific configuration from FIT */
+	ret = fit_get_data_by_name(fit, images, SYSFW_CFG_RM,
+				   &cfg_fragment_addr, &cfg_fragment_size);
+	if (ret < 0)
+		panic("Error accessing %s node in FIT (%d)\n", SYSFW_CFG_RM,
+		      ret);
+
+	/* Apply resource management (RM) configuration to SYSFW */
+	ret = board_ops->board_config_rm(ti_sci,
+					 (u64)(u32)cfg_fragment_addr,
+					 (u32)cfg_fragment_size);
+	if (ret)
+		panic("Failed to set board RM configuration (%d)\n", ret);
+
+	/* Extract security specific configuration from FIT */
+	ret = fit_get_data_by_name(fit, images, SYSFW_CFG_SEC,
+				   &cfg_fragment_addr, &cfg_fragment_size);
+	if (ret < 0)
+		panic("Error accessing %s node in FIT (%d)\n", SYSFW_CFG_SEC,
+		      ret);
+
+	/* Apply security configuration to SYSFW */
+	ret = board_ops->board_config_security(ti_sci,
+					       (u64)(u32)cfg_fragment_addr,
+					       (u32)cfg_fragment_size);
+	if (ret)
+		panic("Failed to set board security configuration (%d)\n",
+		      ret);
+}
+
+void k3_sysfw_loader(void (*config_pm_done_callback)(void))
+{
+	struct spl_image_info spl_image = { 0 };
+	struct spl_boot_device bootdev = { 0 };
+	struct ti_sci_handle *ti_sci;
+	int ret;
+
+	/* Reserve a block of aligned memory for loading the SYSFW image */
+	sysfw_load_address = memalign(ARCH_DMA_MINALIGN,
+				      CONFIG_K3_SYSFW_IMAGE_SIZE_MAX);
+	if (!sysfw_load_address)
+		panic("Error allocating %u bytes of memory for SYSFW image\n",
+		      CONFIG_K3_SYSFW_IMAGE_SIZE_MAX);
+
+	debug("%s: allocated %u bytes at 0x%p\n", __func__,
+	      CONFIG_K3_SYSFW_IMAGE_SIZE_MAX, sysfw_load_address);
+
+	/* Set load address for legacy modes that bypass spl_get_load_buffer */
+	spl_image.load_addr = (uintptr_t)sysfw_load_address;
+
+	bootdev.boot_device = spl_boot_device();
+
+	/* Load combined System Controller firmware and config data image */
+	switch (bootdev.boot_device) {
+#if CONFIG_IS_ENABLED(MMC_SUPPORT)
+	case BOOT_DEVICE_MMC1:
+	case BOOT_DEVICE_MMC2:
+	case BOOT_DEVICE_MMC2_2:
+		ret = spl_mmc_load(&spl_image, &bootdev,
+#ifdef CONFIG_K3_SYSFW_IMAGE_NAME
+				   CONFIG_K3_SYSFW_IMAGE_NAME,
+#else
+				   NULL,
+#endif
+#ifdef CONFIG_K3_SYSFW_IMAGE_MMCSD_RAW_MODE_PART
+				   CONFIG_K3_SYSFW_IMAGE_MMCSD_RAW_MODE_PART,
+#else
+				   0,
+#endif
+#ifdef CONFIG_K3_SYSFW_IMAGE_MMCSD_RAW_MODE_SECT
+				   CONFIG_K3_SYSFW_IMAGE_MMCSD_RAW_MODE_SECT);
+#else
+				   0);
+#endif
+		break;
+#endif
+	default:
+		panic("Loading SYSFW image from device %u not supported!\n",
+		      bootdev.boot_device);
+	}
+
+	if (ret)
+		panic("Error %d occurred during loading SYSFW image!\n", ret);
+
+	/*
+	 * Now that SYSFW got loaded set helper flag to restore regular SPL
+	 * loader behavior so we can later boot into the next stage as expected.
+	 */
+	sysfw_loaded = true;
+
+	/* Ensure the SYSFW image is in FIT format */
+	if (image_get_magic((const image_header_t *)sysfw_load_address) !=
+	    FDT_MAGIC)
+		panic("SYSFW image not in FIT format!\n");
+
+	/* Extract and start SYSFW */
+	k3_sysfw_load_using_fit(sysfw_load_address);
+
+	/* Get handle for accessing SYSFW services */
+	ti_sci = get_ti_sci_handle();
+
+	/* Parse and apply the different SYSFW configuration fragments */
+	k3_sysfw_configure_using_fit(sysfw_load_address, ti_sci);
+
+	/*
+	 * Now that all clocks and PM aspects are setup, invoke a user-
+	 * provided callback function. Usually this callback would be used
+	 * to setup or re-configure the U-Boot console UART.
+	 */
+	if (config_pm_done_callback)
+		config_pm_done_callback();
+
+	/* Output System Firmware version info */
+	printf("SYSFW ABI: %d.%d (firmware rev 0x%04x '%.*s')\n",
+	       ti_sci->version.abi_major, ti_sci->version.abi_minor,
+	       ti_sci->version.firmware_revision,
+	       sizeof(ti_sci->version.firmware_description),
+	       ti_sci->version.firmware_description);
+}
-- 
2.17.1

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Andreas Dannenberg]

In order to be able to use more advanced driver functionality which often
relies on having BSS initialized during early boot prior to relocation
several things need to be in place:

1) Memory needs to be available for BSS to use. For this, we locate BSS
   at the top of the MCU SRAM area, with the stack starting right below
   it,
2) We need to move the initialization of BSS prior to entering
   board_init_f(). We will do this with a separate commit by turning on
   the respective CONFIG option.

In this commit we also clean up the assignment of the initial SP address
as part of the refactoring, taking into account the pre-decrement post-
increment nature in which the SP is used on ARM.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 include/configs/am65x_evm.h | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/configs/am65x_evm.h b/include/configs/am65x_evm.h
index 51abab3943..1319745963 100644
--- a/include/configs/am65x_evm.h
+++ b/include/configs/am65x_evm.h
@@ -19,6 +19,29 @@
 #define CONFIG_SYS_SDRAM_BASE1		0x880000000
 
 /* SPL Loader Configuration */
+#ifdef CONFIG_TARGET_AM654_A53_EVM
+#define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SPL_TEXT_BASE +	\
+					 CONFIG_SYS_K3_NON_SECURE_MSRAM_SIZE)
+#else
+/*
+ * Maximum size in memory allocated to the SPL BSS. Keep it as tight as
+ * possible (to allow the build to go through), as this directly affects
+ * our memory footprint. The less we use for BSS the more we have available
+ * for everything else.
+ */
+#define CONFIG_SPL_BSS_MAX_SIZE		0x5000
+/*
+ * Link BSS to be within SPL in a dedicated region located near the top of
+ * the MCU SRAM, this way making it available also before relocation. Note
+ * that we are not using the actual top of the MCU SRAM as there is a memory
+ * location filled in by the boot ROM that we want to read out without any
+ * interference from the C context.
+ */
+#define CONFIG_SPL_BSS_START_ADDR	(CONFIG_SYS_K3_BOOT_PARAM_TABLE_INDEX -\
+					 CONFIG_SPL_BSS_MAX_SIZE)
+/* Set the stack right below the SPL BSS section */
+#define CONFIG_SYS_INIT_SP_ADDR         CONFIG_SPL_BSS_START_ADDR
+#endif
 
 #ifdef CONFIG_SYS_K3_SPL_ATF
 #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME	"tispl.bin"
@@ -29,8 +52,6 @@
 #endif
 
 #define CONFIG_SPL_MAX_SIZE		CONFIG_SYS_K3_MAX_DOWNLODABLE_IMAGE_SIZE
-#define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SPL_TEXT_BASE +	\
-					CONFIG_SYS_K3_NON_SECURE_MSRAM_SIZE - 4)
 
 #define CONFIG_SYS_BOOTM_LEN		SZ_64M
 
-- 
2.17.1

[U-Boot] [PATCH v2 06/12] armv7R: K3: am654: Use full malloc implementation in SPL

[Andreas Dannenberg]

Switch to using the full malloc scheme in post-relocation SPL to allow
better utilization of available memory for example by allowing memory
to get freed. Initially allocate a 16MB-sized region in DDR starting
at address 0x84000000 for this purpose.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 configs/am65x_evm_r5_defconfig    | 1 -
 configs/am65x_hs_evm_r5_defconfig | 1 -
 include/configs/am65x_evm.h       | 3 +++
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
index 6a261c20ac..5abd686931 100644
--- a/configs/am65x_evm_r5_defconfig
+++ b/configs/am65x_evm_r5_defconfig
@@ -18,7 +18,6 @@ CONFIG_SPL_LOAD_FIT=y
 CONFIG_USE_BOOTCOMMAND=y
 # CONFIG_DISPLAY_CPUINFO is not set
 CONFIG_SPL_TEXT_BASE=0x41c00000
-CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C_SUPPORT=y
diff --git a/configs/am65x_hs_evm_r5_defconfig b/configs/am65x_hs_evm_r5_defconfig
index 0b12f15782..49eca7de64 100644
--- a/configs/am65x_hs_evm_r5_defconfig
+++ b/configs/am65x_hs_evm_r5_defconfig
@@ -20,7 +20,6 @@ CONFIG_SPL_FIT_IMAGE_POST_PROCESS=y
 CONFIG_USE_BOOTCOMMAND=y
 # CONFIG_DISPLAY_CPUINFO is not set
 CONFIG_SPL_TEXT_BASE=0x41c00000
-CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C_SUPPORT=y
diff --git a/include/configs/am65x_evm.h b/include/configs/am65x_evm.h
index 1319745963..ea365b979b 100644
--- a/include/configs/am65x_evm.h
+++ b/include/configs/am65x_evm.h
@@ -41,6 +41,9 @@
 					 CONFIG_SPL_BSS_MAX_SIZE)
 /* Set the stack right below the SPL BSS section */
 #define CONFIG_SYS_INIT_SP_ADDR         CONFIG_SPL_BSS_START_ADDR
+/* Configure R5 SPL post-relocation malloc pool in DDR */
+#define CONFIG_SYS_SPL_MALLOC_START	0x84000000
+#define CONFIG_SYS_SPL_MALLOC_SIZE	SZ_16M
 #endif
 
 #ifdef CONFIG_SYS_K3_SPL_ATF
-- 
2.17.1

[U-Boot] [PATCH v2 07/12] armV7R: K3: am654: Load SYSFW binary and config from boot media

[Andreas Dannenberg]

Use the System Firmware (SYSFW) loader framework to load and start
the SYSFW as part of the AM654 early initialization sequence. While
at it also initialize the WKUP_UART0 pinmux as it is used by SYSFW
to print diagnostic messages.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 arch/arm/mach-k3/am6_init.c | 27 ++++++++++++++++++++++++++-
 board/ti/am65x/Kconfig      |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-k3/am6_init.c b/arch/arm/mach-k3/am6_init.c
index 60a580305d..e326f575e5 100644
--- a/arch/arm/mach-k3/am6_init.c
+++ b/arch/arm/mach-k3/am6_init.c
@@ -10,8 +10,11 @@
 #include <asm/io.h>
 #include <spl.h>
 #include <asm/arch/hardware.h>
+#include <asm/arch/sysfw-loader.h>
 #include "common.h"
 #include <dm.h>
+#include <dm/uclass-internal.h>
+#include <dm/pinctrl.h>
 
 #ifdef CONFIG_SPL_BUILD
 static void mmr_unlock(u32 base, u32 partition)
@@ -63,7 +66,7 @@ static void store_boot_index_from_rom(void)
 
 void board_init_f(ulong dummy)
 {
-#if defined(CONFIG_K3_AM654_DDRSS)
+#if defined(CONFIG_K3_LOAD_SYSFW) || defined(CONFIG_K3_AM654_DDRSS)
 	struct udevice *dev;
 	int ret;
 #endif
@@ -83,8 +86,30 @@ void board_init_f(ulong dummy)
 	/* Init DM early in-order to invoke system controller */
 	spl_early_init();
 
+#ifdef CONFIG_K3_LOAD_SYSFW
+	/*
+	 * Process pinctrl for the serial0 a.k.a. WKUP_UART0 module and continue
+	 * regardless of the result of pinctrl. Do this without probing the
+	 * device, but instead by searching the device that would request the
+	 * given sequence number if probed. The UART will be used by the system
+	 * firmware (SYSFW) image for various purposes and SYSFW depends on us
+	 * to initialize its pin settings.
+	 */
+	ret = uclass_find_device_by_seq(UCLASS_SERIAL, 0, true, &dev);
+	if (!ret)
+		pinctrl_select_state(dev, "default");
+
+	/*
+	 * Load, start up, and configure system controller firmware. Provide
+	 * the U-Boot console init function to the SYSFW post-PM configuration
+	 * callback hook, effectively switching on (or over) the console
+	 * output.
+	 */
+	k3_sysfw_loader(preloader_console_init);
+#else
 	/* Prepare console output */
 	preloader_console_init();
+#endif
 
 #ifdef CONFIG_K3_AM654_DDRSS
 	ret = uclass_get_device(UCLASS_RAM, 0, &dev);
diff --git a/board/ti/am65x/Kconfig b/board/ti/am65x/Kconfig
index 98172c28f5..60bb834aca 100644
--- a/board/ti/am65x/Kconfig
+++ b/board/ti/am65x/Kconfig
@@ -18,6 +18,7 @@ config TARGET_AM654_R5_EVM
 	select CPU_V7R
 	select SYS_THUMB_BUILD
 	select SOC_K3_AM6
+	select K3_LOAD_SYSFW
 	select K3_AM654_DDRSS
 	imply SYS_K3_SPL_ATF
 
-- 
2.17.1

[U-Boot] [PATCH v2 08/12] configs: am65x_evm_r5: All sysfw to be loaded via MMC

[Andreas Dannenberg]

Enable all the relevant configs that enables support for loading
sysfw via MMC.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 configs/am65x_evm_r5_defconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
index 5abd686931..e3eb0d9cc5 100644
--- a/configs/am65x_evm_r5_defconfig
+++ b/configs/am65x_evm_r5_defconfig
@@ -3,7 +3,7 @@ CONFIG_ARCH_K3=y
 CONFIG_SPL_GPIO_SUPPORT=y
 CONFIG_SPL_LIBCOMMON_SUPPORT=y
 CONFIG_SPL_LIBGENERIC_SUPPORT=y
-CONFIG_SYS_MALLOC_F_LEN=0x2000
+CONFIG_SYS_MALLOC_F_LEN=0x55000
 CONFIG_SOC_K3_AM6=y
 CONFIG_TARGET_AM654_R5_EVM=y
 CONFIG_SPL_MMC_SUPPORT=y
@@ -20,6 +20,7 @@ CONFIG_USE_BOOTCOMMAND=y
 CONFIG_SPL_TEXT_BASE=0x41c00000
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
+CONFIG_SPL_EARLY_BSS=y
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
@@ -89,3 +90,4 @@ CONFIG_SYSRESET_TI_SCI=y
 CONFIG_TIMER=y
 CONFIG_SPL_TIMER=y
 CONFIG_OMAP_TIMER=y
+CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
-- 
2.17.1

[U-Boot] [PATCH v2 09/12] configs: am65x_hs_evm_r5: All sysfw to be loaded via MMC

[Andreas Dannenberg]

Enable all the relevant configs that enables support for loading
sysfw via MMC.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 configs/am65x_hs_evm_r5_defconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/configs/am65x_hs_evm_r5_defconfig b/configs/am65x_hs_evm_r5_defconfig
index 49eca7de64..642f005bd6 100644
--- a/configs/am65x_hs_evm_r5_defconfig
+++ b/configs/am65x_hs_evm_r5_defconfig
@@ -4,7 +4,7 @@ CONFIG_TI_SECURE_DEVICE=y
 CONFIG_SPL_GPIO_SUPPORT=y
 CONFIG_SPL_LIBCOMMON_SUPPORT=y
 CONFIG_SPL_LIBGENERIC_SUPPORT=y
-CONFIG_SYS_MALLOC_F_LEN=0x2000
+CONFIG_SYS_MALLOC_F_LEN=0x55000
 CONFIG_SOC_K3_AM6=y
 CONFIG_TARGET_AM654_R5_EVM=y
 CONFIG_SPL_MMC_SUPPORT=y
@@ -22,6 +22,7 @@ CONFIG_USE_BOOTCOMMAND=y
 CONFIG_SPL_TEXT_BASE=0x41c00000
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
+CONFIG_SPL_EARLY_BSS=y
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
@@ -89,3 +90,4 @@ CONFIG_SYSRESET_TI_SCI=y
 CONFIG_TIMER=y
 CONFIG_SPL_TIMER=y
 CONFIG_OMAP_TIMER=y
+CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
-- 
2.17.1

[U-Boot] [PATCH v2 10/12] configs: am65x_evm: Add Support for eMMC boot

[Andreas Dannenberg]

From: Faiz Abbas <faiz_abbas@ti.com>

Add configs to support RAW boot mode in eMMC.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 configs/am65x_evm_a53_defconfig | 2 ++
 configs/am65x_evm_r5_defconfig  | 2 ++
 include/configs/am65x_evm.h     | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig
index 43d2ccc5ed..d5f8a01fae 100644
--- a/configs/am65x_evm_a53_defconfig
+++ b/configs/am65x_evm_a53_defconfig
@@ -22,6 +22,8 @@ CONFIG_SPL_TEXT_BASE=0x80080000
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x1400
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
diff --git a/configs/am65x_evm_r5_defconfig b/configs/am65x_evm_r5_defconfig
index e3eb0d9cc5..d81018b31d 100644
--- a/configs/am65x_evm_r5_defconfig
+++ b/configs/am65x_evm_r5_defconfig
@@ -21,6 +21,8 @@ CONFIG_SPL_TEXT_BASE=0x41c00000
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_EARLY_BSS=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x400
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
diff --git a/include/configs/am65x_evm.h b/include/configs/am65x_evm.h
index ea365b979b..1d291f5724 100644
--- a/include/configs/am65x_evm.h
+++ b/include/configs/am65x_evm.h
@@ -107,6 +107,8 @@
 #define CONFIG_SYS_REDUNDAND_ENVIRONMENT
 #endif
 
+#define CONFIG_SUPPORT_EMMC_BOOT
+
 /* Now for the remaining common defines */
 #include <configs/ti_armv7_common.h>
 
-- 
2.17.1

[U-Boot] [PATCH v2 11/12] configs: am65x_hs_evm: Add Support for eMMC boot

[Andreas Dannenberg]

Add configs to support RAW boot mode in eMMC.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 configs/am65x_hs_evm_a53_defconfig | 2 ++
 configs/am65x_hs_evm_r5_defconfig  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/configs/am65x_hs_evm_a53_defconfig b/configs/am65x_hs_evm_a53_defconfig
index 9c55cd37f6..62e9d720fe 100644
--- a/configs/am65x_hs_evm_a53_defconfig
+++ b/configs/am65x_hs_evm_a53_defconfig
@@ -25,6 +25,8 @@ CONFIG_SPL_TEXT_BASE=0x80080000
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x1400
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
diff --git a/configs/am65x_hs_evm_r5_defconfig b/configs/am65x_hs_evm_r5_defconfig
index 642f005bd6..9e01899031 100644
--- a/configs/am65x_hs_evm_r5_defconfig
+++ b/configs/am65x_hs_evm_r5_defconfig
@@ -23,6 +23,8 @@ CONFIG_SPL_TEXT_BASE=0x41c00000
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_EARLY_BSS=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x400
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
-- 
2.17.1

[U-Boot] [PATCH v2 12/12] am65x: README: Add eMMC layout and flash instructions

[Andreas Dannenberg]

From: Faiz Abbas <faiz_abbas@ti.com>

Add instructions for flashing boot images to the eMMC with a
layout of the address where each image needs to be flashed.

Also add instructions to flash filesystem partition in user
partition and boot kernel from the rootfs.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 board/ti/am65x/README | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/board/ti/am65x/README b/board/ti/am65x/README
index 0b82bd557b..16384e05ea 100644
--- a/board/ti/am65x/README
+++ b/board/ti/am65x/README
@@ -209,3 +209,55 @@ Image formats:
                 | |    Secure config  | |
                 | +-------------------+ |
                 +-----------------------+
+
+eMMC:
+-----
+ROM supports booting from eMMC from boot0 partition offset 0x0
+
+Flashing images to eMMC:
+
+The following commands can be used to download tiboot3.bin, tispl.bin,
+u-boot.img, and sysfw.itb from an SD card and write them to the eMMC boot0
+partition at respective addresses.
+
+=> mmc dev 0 1
+=> fatload mmc 1 ${loadaddr} tiboot3.bin
+=> mmc write ${loadaddr} 0x0 0x400
+=> fatload mmc 1 ${loadaddr} tispl.bin
+=> mmc write ${loadaddr} 0x400 0x1000
+=> fatload mmc 1 ${loadaddr} u-boot.img
+=> mmc write ${loadaddr} 0x1400 0x2000
+=> fatload mmc 1 ${loadaddr} sysfw.itb
+=> mmc write ${loadaddr} 0x3600 0x800
+
+To give the ROM access to the boot partition, the following commands must be
+used for the first time:
+=> mmc partconf 0 1 1 1
+=> mmc bootbus 0 1 0 0
+
+To create a software partition for the rootfs, the following command can be
+used:
+=> gpt write mmc 0 ${partitions}
+
+eMMC layout:
+
+           boot0 partition (8 MB)                        user partition
+   0x0+----------------------------------+      0x0+-------------------------+
+      |     tiboot3.bin (512 KB)         |         |                         |
+ 0x400+----------------------------------+         |                         |
+      |       tispl.bin (2 MB)           |         |                         |
+0x1400+----------------------------------+         |        rootfs           |
+      |       u-boot.img (4 MB)          |         |                         |
+0x3400+----------------------------------+         |                         |
+      |      environment (128 KB)        |         |                         |
+0x3500+----------------------------------+         |                         |
+      |   backup environment (128 KB)    |         |                         |
+0x3600+----------------------------------+         |                         |
+      |          sysfw (1 MB)            |         |                         |
+0x3E00+----------------------------------+         +-------------------------+
+
+Kernel image and DT are expected to be present in the /boot folder of rootfs.
+To boot kernel from eMMC, use the following commands:
+=> setenv mmcdev 0
+=> setenv bootpart 0
+=> boot
-- 
2.17.1

[U-Boot] [PATCH v2 00/12] System Firmware Loader for TI K3 family SoCs

[Lokesh Vutla]

On 05/06/19 4:25 AM, Andreas Dannenberg wrote:
> Updated version of the SYSFW loader series for K3 family AM654x devices.
> The fundamantal approach of tapping into the SPL loader framework has
> been kept for reasons discussed already. The series also still uses
> "early BSS" in SPL's board_init_f(). I'm well aware of the concerns
> previously brought up regarding this mainly by Simon Glass but I have
> not been able to find a better / more universal solution for this yet
> (one proposal was to move SYSFW loading into board_init_r() which is not
> easily solvable as SYSFW is needed to bring up DDR on K3 SoCs). Long
> story short I propose to consider the current proposed approach
> nevertheless (as it is also used by other platforms) at least as an
> initial step, and then migrate once a better solution is available.

tested this series on AM654 evm using SD boot. FWIW:
Tested-by: Lokesh Vutla <lokeshvutla@ti.com>



> 
> I have not yet included support for TI's newest K3 family J721E SoC
> which Lokesh posted an initial patch series [5] for due to the
> complex dependencies of all the different series we have currently
> posted/pending (if I were to add support for J721E which eventually will
> be required then the SYSFW loader series would have Lokesh's series as a
> pre-requisite as well).
> 
> This being said I would like to propose the following staging sequence
> for the different TI K3 SoCs patches currently under review:
> 
> Step 1) Faiz' "Add Support for eMMC in Am65x-evm" series [1]. It needs
>         a small update to actuall allow for eMMC boot I posted earlier.
> Step 2) The SYSFW loader series proposed here
> Step 3) An updated version (v3) of the AM654x EEPROM support [3].
>         Will post this today. 

I would like to see the above 3 series be merged first. Will take care of the
rest of the J721e support and other.

> Step 4) An updated version of Lokesh's "arm: k3: Allow for exclusive
>         and shared device requests" series. In addition to a rebase
>         such an updated series should include updating power domain
>         properties for devices that were added during the previous
>         steps.
> Step 5) An updated version of Lokesh's "arm: k3: arm64: Initial support
>         Texas Instrument's J721E Platform" series [5] also adding
>         in the few lines of codes to leverage SYSFW.
> Step 6 & beyond) Various rproc patches, etc.

I have a slightly different order that you mentioned. Will repost everything
once the first 3 steps are sorted out.

Thanks and regards,
Lokesh

> 
> 
> The above is to allow for things to build in a logical order while
> avoiding merge conflicts. 1+2+3 added will provide a pretty good initial
> working U-Boot using eMMC and SD media for AM654x which is a device
> available today plus a foundation for everything else, hence it is at
> the top of the list.
> 
> 
> Changes since initial submission:
> - Dropped patch "armv7R: dts: k3: am654: Update mmc nodes for loading
>   sysfw". This is taken care off by the "Add Support for eMMC in
>   Am65x-evm" patch series [1] which this series was rebased on.
> - Replaced patch "spl: Allow skipping clearing BSS during relocation"
>   with a functionally equivalent patch "spl: Allow performing BSS init
>   early before board_init_f()" which is a bit more elegant solution
>   which itself is a slight evolution what previously posted by Simon
>   Goldschmidt [2]
> - Collected various review tags
> 
> 
> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=111723
> [2] https://patchwork.ozlabs.org/patch/1067363/
> [3] https://patchwork.ozlabs.org/project/uboot/list/?series=109266
> [4] https://patchwork.ozlabs.org/project/uboot/list/?series=109163
> [5] https://patchwork.ozlabs.org/project/uboot/list/?series=109296
> 
> 
> --
> Andreas Dannenberg
> Texas Instruments Inc
> 
> 
> Andreas Dannenberg (10):
>   mmc: am654_sdhci: Allow driver to probe without PDs specified
>   spl: Allow performing BSS init early before board_init_f()
>   spl: Make image loader infrastructure more universal
>   arm: K3: Introduce System Firmware loader framework
>   armV7R: K3: am654: Allow using SPL BSS pre-relocation
>   armv7R: K3: am654: Use full malloc implementation in SPL
>   armV7R: K3: am654: Load SYSFW binary and config from boot media
>   configs: am65x_evm_r5: All sysfw to be loaded via MMC
>   configs: am65x_hs_evm_r5: All sysfw to be loaded via MMC
>   configs: am65x_hs_evm: Add Support for eMMC boot
> 
> Faiz Abbas (2):
>   configs: am65x_evm: Add Support for eMMC boot
>   am65x: README: Add eMMC layout and flash instructions
> 
>  arch/arm/lib/crt0.S                          |  53 ++--
>  arch/arm/mach-k3/Kconfig                     |  39 +++
>  arch/arm/mach-k3/Makefile                    |   3 +
>  arch/arm/mach-k3/am6_init.c                  |  27 +-
>  arch/arm/mach-k3/include/mach/sysfw-loader.h |  12 +
>  arch/arm/mach-k3/sysfw-loader.c              | 260 +++++++++++++++++++
>  board/ti/am65x/Kconfig                       |   1 +
>  board/ti/am65x/README                        |  52 ++++
>  common/spl/Kconfig                           |  10 +
>  common/spl/spl_fit.c                         |  14 +
>  common/spl/spl_mmc.c                         |  76 ++++--
>  configs/am65x_evm_a53_defconfig              |   2 +
>  configs/am65x_evm_r5_defconfig               |   7 +-
>  configs/am65x_hs_evm_a53_defconfig           |   2 +
>  configs/am65x_hs_evm_r5_defconfig            |   7 +-
>  drivers/mmc/am654_sdhci.c                    |  16 +-
>  include/configs/am65x_evm.h                  |  30 ++-
>  include/spl.h                                |  26 ++
>  18 files changed, 580 insertions(+), 57 deletions(-)
>  create mode 100644 arch/arm/mach-k3/include/mach/sysfw-loader.h
>  create mode 100644 arch/arm/mach-k3/sysfw-loader.c
> 

[U-Boot] [PATCH v2 00/12] System Firmware Loader for TI K3 family SoCs

[Andreas Dannenberg]

On Wed, Jun 05, 2019 at 11:52:52AM +0530, Lokesh Vutla wrote:
> 
> 
> On 05/06/19 4:25 AM, Andreas Dannenberg wrote:
> > Updated version of the SYSFW loader series for K3 family AM654x devices.
> > The fundamantal approach of tapping into the SPL loader framework has
> > been kept for reasons discussed already. The series also still uses
> > "early BSS" in SPL's board_init_f(). I'm well aware of the concerns
> > previously brought up regarding this mainly by Simon Glass but I have
> > not been able to find a better / more universal solution for this yet
> > (one proposal was to move SYSFW loading into board_init_r() which is not
> > easily solvable as SYSFW is needed to bring up DDR on K3 SoCs). Long
> > story short I propose to consider the current proposed approach
> > nevertheless (as it is also used by other platforms) at least as an
> > initial step, and then migrate once a better solution is available.
> 
> tested this series on AM654 evm using SD boot. FWIW:
> Tested-by: Lokesh Vutla <lokeshvutla@ti.com>

Thanks Lokesh.


> > I have not yet included support for TI's newest K3 family J721E SoC
> > which Lokesh posted an initial patch series [5] for due to the
> > complex dependencies of all the different series we have currently
> > posted/pending (if I were to add support for J721E which eventually will
> > be required then the SYSFW loader series would have Lokesh's series as a
> > pre-requisite as well).
> > 
> > This being said I would like to propose the following staging sequence
> > for the different TI K3 SoCs patches currently under review:
> > 
> > Step 1) Faiz' "Add Support for eMMC in Am65x-evm" series [1]. It needs
> >         a small update to actuall allow for eMMC boot I posted earlier.
> > Step 2) The SYSFW loader series proposed here
> > Step 3) An updated version (v3) of the AM654x EEPROM support [3].
> >         Will post this today. 
> 
> I would like to see the above 3 series be merged first. Will take care of the
> rest of the J721e support and other.

Yes that's what I'm proposing.


> > Step 4) An updated version of Lokesh's "arm: k3: Allow for exclusive
> >         and shared device requests" series. In addition to a rebase
> >         such an updated series should include updating power domain
> >         properties for devices that were added during the previous
> >         steps.
> > Step 5) An updated version of Lokesh's "arm: k3: arm64: Initial support
> >         Texas Instrument's J721E Platform" series [5] also adding
> >         in the few lines of codes to leverage SYSFW.
> > Step 6 & beyond) Various rproc patches, etc.
> 
> I have a slightly different order that you mentioned. Will repost everything
> once the first 3 steps are sorted out.

I don't have an issue with changing the order of what happens past step
3. But without the first three things (or first two, really) in place, we
are kind of stuck turning in circles... :)

--
Andreas Dannenberg
Texas Instruments Inc

[U-Boot] [PATCH v2 01/12] mmc: am654_sdhci: Allow driver to probe without PDs specified

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:44PM -0500, Andreas Dannenberg wrote:

> We would like to use the driver even without power domains being
> specified for cases such as during early boot when the required power
> domains have already gotten enabled by the SoC's boot ROM and such
> explicit initialization is not needed and possible.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/7c6ea382/attachment.sig>

[U-Boot] [PATCH v2 02/12] spl: Allow performing BSS init early before board_init_f()

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:45PM -0500, Andreas Dannenberg wrote:

> On some platform we have sufficient memory available early on to allow
> setting up and using a basic BSS prior to entering board_init_f(). Doing
> so can for example be used to carry state over to board_init_r() without
> having to resort to extending U-Boot's global data structure.
> 
> To support such scenarios add a Kconfig option called CONFIG_SPL_EARLY_BSS
> to allow moving the initialization of BSS prior to entering board_init_f(),
> if enabled. Note that using this option usually should go along with using
> CONFIG_SPL_SEPARATE_BSS and configuring BSS to be located in memory
> actually available prior to board_init_f().
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/e3bb78df/attachment.sig>

[U-Boot] [PATCH v2 03/12] spl: Make image loader infrastructure more universal

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:46PM -0500, Andreas Dannenberg wrote:

> The current U-Boot SPL image loader infrastructure is very powerful,
> able to initialize and load from a variety of boot media however it
> is strongly geared towards loading specific types of images in a very
> specific way. To address the need being able to use this infrastructure
> to load arbitrary image files go ahead and refactor it as follows:
> 
> - Refactor existing spl_mmc_load_image function into superset function,
>   accepting additional arguments such as filenames and media load offset
>   (same concept can also be applied toother spl_XXX_load_image functions)
> - Extend the loader function to "remember" their peripheral initialization
>   status so that the init is only done once during the boot process,
> - Extend the FIT image loading function to allow skipping the parsing/
>   processing of the FIT contents (so that this can be done separately
>   in a more customized fashion)
> - Populate the SPL_LOAD_IMAGE_METHOD() list with a trampoline function,
>   invoking the newly refactored superset functions in a way to maintain
>   compatibility with the existing behavior
> 
> This refactoring initially covers MMC/SD card loading (RAW and FS-based).
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/4881b045/attachment.sig>

[U-Boot] [PATCH v2 04/12] arm: K3: Introduce System Firmware loader framework

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:47PM -0500, Andreas Dannenberg wrote:

> Introduce a framework that allows loading the System Firmware (SYSFW)
> binary as well as the associated configuration data from an image tree
> blob named "sysfw.itb" from an FS-based MMC boot media or from an MMC
> RAW mode partition or sector.
> 
> To simplify the handling of and loading from the different boot media
> we tap into the existing U-Boot SPL framework usually used for loading
> U-Boot by building on an earlier commit that exposes some of that
> functionality.
> 
> Note that this initial implementation only supports FS and RAW-based
> eMMC/SD card boot.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/5e3853bc/attachment.sig>

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:

> In order to be able to use more advanced driver functionality which often
> relies on having BSS initialized during early boot prior to relocation
> several things need to be in place:
> 
> 1) Memory needs to be available for BSS to use. For this, we locate BSS
>    at the top of the MCU SRAM area, with the stack starting right below
>    it,
> 2) We need to move the initialization of BSS prior to entering
>    board_init_f(). We will do this with a separate commit by turning on
>    the respective CONFIG option.
> 
> In this commit we also clean up the assignment of the initial SP address
> as part of the refactoring, taking into account the pre-decrement post-
> increment nature in which the SP is used on ARM.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/8609141b/attachment.sig>

[U-Boot] [PATCH v2 06/12] armv7R: K3: am654: Use full malloc implementation in SPL

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:49PM -0500, Andreas Dannenberg wrote:

> Switch to using the full malloc scheme in post-relocation SPL to allow
> better utilization of available memory for example by allowing memory
> to get freed. Initially allocate a 16MB-sized region in DDR starting
> at address 0x84000000 for this purpose.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/43c62896/attachment.sig>

[U-Boot] [PATCH v2 07/12] armV7R: K3: am654: Load SYSFW binary and config from boot media

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:50PM -0500, Andreas Dannenberg wrote:

> Use the System Firmware (SYSFW) loader framework to load and start
> the SYSFW as part of the AM654 early initialization sequence. While
> at it also initialize the WKUP_UART0 pinmux as it is used by SYSFW
> to print diagnostic messages.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/73270e08/attachment.sig>

[U-Boot] [PATCH v2 08/12] configs: am65x_evm_r5: All sysfw to be loaded via MMC

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:51PM -0500, Andreas Dannenberg wrote:

> Enable all the relevant configs that enables support for loading
> sysfw via MMC.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/ac3c21a6/attachment.sig>

[U-Boot] [PATCH v2 09/12] configs: am65x_hs_evm_r5: All sysfw to be loaded via MMC

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:52PM -0500, Andreas Dannenberg wrote:

> Enable all the relevant configs that enables support for loading
> sysfw via MMC.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/0442b89d/attachment.sig>

[U-Boot] [PATCH v2 10/12] configs: am65x_evm: Add Support for eMMC boot

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:53PM -0500, Andreas Dannenberg wrote:

> From: Faiz Abbas <faiz_abbas@ti.com>
> 
> Add configs to support RAW boot mode in eMMC.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/85e5b7b8/attachment.sig>

[U-Boot] [PATCH v2 11/12] configs: am65x_hs_evm: Add Support for eMMC boot

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:54PM -0500, Andreas Dannenberg wrote:

> Add configs to support RAW boot mode in eMMC.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/bc53c2fc/attachment.sig>

[U-Boot] [PATCH v2 12/12] am65x: README: Add eMMC layout and flash instructions

[Tom Rini]

On Tue, Jun 04, 2019 at 05:55:55PM -0500, Andreas Dannenberg wrote:

> From: Faiz Abbas <faiz_abbas@ti.com>
> 
> Add instructions for flashing boot images to the eMMC with a
> layout of the address where each image needs to be flashed.
> 
> Also add instructions to flash filesystem partition in user
> partition and boot kernel from the rootfs.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/81c54548/attachment.sig>

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
>
> > In order to be able to use more advanced driver functionality which often
> > relies on having BSS initialized during early boot prior to relocation
> > several things need to be in place:
> >
> > 1) Memory needs to be available for BSS to use. For this, we locate BSS
> >    at the top of the MCU SRAM area, with the stack starting right below
> >    it,
> > 2) We need to move the initialization of BSS prior to entering
> >    board_init_f(). We will do this with a separate commit by turning on
> >    the respective CONFIG option.
> >
> > In this commit we also clean up the assignment of the initial SP address
> > as part of the refactoring, taking into account the pre-decrement post-
> > increment nature in which the SP is used on ARM.
> >
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>
> Applied to u-boot/master, thanks!

Wait, why has this been merged? Unfortunately, I haven't followed this series,
but in a discussion about a similar patch I sent [1], using BSS from
board_init_f
was turned down. And Simon Glass rather convinced me that this is the current
API U-Boot has (and is documented in README).

So either we must change this API and its documentation (and I would expect the
author of this patch to combine the README change with the code change), or this
patch would have to be rejected.

Again, I'm sorry I only see this now. In thought to remember a
discussion in this
thread, but I clearly remember that wrong...

[1] https://patchwork.ozlabs.org/patch/1057237/

Regards,
Simon

>
> --
> Tom

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Tom Rini]

On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> >
> > > In order to be able to use more advanced driver functionality which often
> > > relies on having BSS initialized during early boot prior to relocation
> > > several things need to be in place:
> > >
> > > 1) Memory needs to be available for BSS to use. For this, we locate BSS
> > >    at the top of the MCU SRAM area, with the stack starting right below
> > >    it,
> > > 2) We need to move the initialization of BSS prior to entering
> > >    board_init_f(). We will do this with a separate commit by turning on
> > >    the respective CONFIG option.
> > >
> > > In this commit we also clean up the assignment of the initial SP address
> > > as part of the refactoring, taking into account the pre-decrement post-
> > > increment nature in which the SP is used on ARM.
> > >
> > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >
> > Applied to u-boot/master, thanks!
> 
> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> but in a discussion about a similar patch I sent [1], using BSS from
> board_init_f
> was turned down. And Simon Glass rather convinced me that this is the current
> API U-Boot has (and is documented in README).
> 
> So either we must change this API and its documentation (and I would expect the
> author of this patch to combine the README change with the code change), or this
> patch would have to be rejected.
> 
> Again, I'm sorry I only see this now. In thought to remember a
> discussion in this
> thread, but I clearly remember that wrong...
> 
> [1] https://patchwork.ozlabs.org/patch/1057237/

And I had missed that other thread.  Lokesh, since I think Andreas is
out currently can you expand a little on what we can/can't do on this
platform?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190720/ebeb479b/attachment.sig>

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Lokesh Vutla]

Hi Tom,

On 20/07/19 9:21 PM, Tom Rini wrote:
> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
>>>
>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
>>>
>>>> In order to be able to use more advanced driver functionality which often
>>>> relies on having BSS initialized during early boot prior to relocation
>>>> several things need to be in place:
>>>>
>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
>>>>    at the top of the MCU SRAM area, with the stack starting right below
>>>>    it,
>>>> 2) We need to move the initialization of BSS prior to entering
>>>>    board_init_f(). We will do this with a separate commit by turning on
>>>>    the respective CONFIG option.
>>>>
>>>> In this commit we also clean up the assignment of the initial SP address
>>>> as part of the refactoring, taking into account the pre-decrement post-
>>>> increment nature in which the SP is used on ARM.
>>>>
>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>
>>> Applied to u-boot/master, thanks!
>>
>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
>> but in a discussion about a similar patch I sent [1], using BSS from
>> board_init_f
>> was turned down. And Simon Glass rather convinced me that this is the current
>> API U-Boot has (and is documented in README).
>>
>> So either we must change this API and its documentation (and I would expect the
>> author of this patch to combine the README change with the code change), or this
>> patch would have to be rejected.
>>
>> Again, I'm sorry I only see this now. In thought to remember a
>> discussion in this
>> thread, but I clearly remember that wrong...
>>
>> [1] https://patchwork.ozlabs.org/patch/1057237/
> 
> And I had missed that other thread.  Lokesh, since I think Andreas is
> out currently can you expand a little on what we can/can't do on this
> platform?  Thanks!

The reason why BSS is needed very early in this platform is for the following
reasons:
- System co-processor is the central resource manager in SoC and should be
loaded and started very early in the boot process. Without that no peripheral or
memory can be initialized. So for loading system co-processor image, we only
have limited SRAM and a peripheral initialized by ROM.
- System co-processor(DMSC) is being represented as remote-core in
Device-tree(We are strictly following DM and DT model for the entire SoC).
- Since DM is also followed by each peripheral device and remote core, DM should
be enabled very early and many peripheral drivers are dependent on BSS usage.
So, BSS has been made available very early.

Hope this is clear. Let me know if more details are required, I will be happy to
explain.

Thanks and regards,
Lokesh

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

Hi Lokesh,

thanks for following up on this.

On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
> Hi Tom,
>
> On 20/07/19 9:21 PM, Tom Rini wrote:
> > On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> >> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> >>>
> >>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> >>>
> >>>> In order to be able to use more advanced driver functionality which often
> >>>> relies on having BSS initialized during early boot prior to relocation
> >>>> several things need to be in place:
> >>>>
> >>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
> >>>>    at the top of the MCU SRAM area, with the stack starting right below
> >>>>    it,
> >>>> 2) We need to move the initialization of BSS prior to entering
> >>>>    board_init_f(). We will do this with a separate commit by turning on
> >>>>    the respective CONFIG option.
> >>>>
> >>>> In this commit we also clean up the assignment of the initial SP address
> >>>> as part of the refactoring, taking into account the pre-decrement post-
> >>>> increment nature in which the SP is used on ARM.
> >>>>
> >>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >>>
> >>> Applied to u-boot/master, thanks!
> >>
> >> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> >> but in a discussion about a similar patch I sent [1], using BSS from
> >> board_init_f
> >> was turned down. And Simon Glass rather convinced me that this is the current
> >> API U-Boot has (and is documented in README).
> >>
> >> So either we must change this API and its documentation (and I would expect the
> >> author of this patch to combine the README change with the code change), or this
> >> patch would have to be rejected.
> >>
> >> Again, I'm sorry I only see this now. In thought to remember a
> >> discussion in this
> >> thread, but I clearly remember that wrong...
> >>
> >> [1] https://patchwork.ozlabs.org/patch/1057237/
> >
> > And I had missed that other thread.  Lokesh, since I think Andreas is
> > out currently can you expand a little on what we can/can't do on this
> > platform?  Thanks!
>
> The reason why BSS is needed very early in this platform is for the following
> reasons:
> - System co-processor is the central resource manager in SoC and should be
> loaded and started very early in the boot process. Without that no peripheral or
> memory can be initialized. So for loading system co-processor image, we only
> have limited SRAM and a peripheral initialized by ROM.
> - System co-processor(DMSC) is being represented as remote-core in
> Device-tree(We are strictly following DM and DT model for the entire SoC).
> - Since DM is also followed by each peripheral device and remote core, DM should
> be enabled very early and many peripheral drivers are dependent on BSS usage.
> So, BSS has been made available very early.
>
> Hope this is clear. Let me know if more details are required, I will be happy to
> explain.

Don't get me wrong: I'm not against using BSS early. I just want to ensure this
stays consistent throught U-Boot.

The reasons you stated still don't make it clear to me *why* bss is needed
early. There are other boards using DM early that don't need this. In my
opinion, DM drivers normally don't rely on BSS but keep all their state in
heap memory. If you only need BSS early because drivers rely on BSS, you might
have to fix those drivers?

Further, allowing BSS early is still against what the main README says, so I
want to raise the question again: shouldn't this main README be changed if we
suddenly allow BSS to be used early (because that main README says we can'that
do that)?

Regards,
Simon


>
> Thanks and regards,
> Lokesh

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Lokesh Vutla]

Hi Simon,

On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> Hi Lokesh,
> 
> thanks for following up on this.
> 
> On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>
>> Hi Tom,
>>
>> On 20/07/19 9:21 PM, Tom Rini wrote:
>>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
>>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
>>>>>
>>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
>>>>>
>>>>>> In order to be able to use more advanced driver functionality which often
>>>>>> relies on having BSS initialized during early boot prior to relocation
>>>>>> several things need to be in place:
>>>>>>
>>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
>>>>>>    at the top of the MCU SRAM area, with the stack starting right below
>>>>>>    it,
>>>>>> 2) We need to move the initialization of BSS prior to entering
>>>>>>    board_init_f(). We will do this with a separate commit by turning on
>>>>>>    the respective CONFIG option.
>>>>>>
>>>>>> In this commit we also clean up the assignment of the initial SP address
>>>>>> as part of the refactoring, taking into account the pre-decrement post-
>>>>>> increment nature in which the SP is used on ARM.
>>>>>>
>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>
>>>>> Applied to u-boot/master, thanks!
>>>>
>>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
>>>> but in a discussion about a similar patch I sent [1], using BSS from
>>>> board_init_f
>>>> was turned down. And Simon Glass rather convinced me that this is the current
>>>> API U-Boot has (and is documented in README).
>>>>
>>>> So either we must change this API and its documentation (and I would expect the
>>>> author of this patch to combine the README change with the code change), or this
>>>> patch would have to be rejected.
>>>>
>>>> Again, I'm sorry I only see this now. In thought to remember a
>>>> discussion in this
>>>> thread, but I clearly remember that wrong...
>>>>
>>>> [1] https://patchwork.ozlabs.org/patch/1057237/
>>>
>>> And I had missed that other thread.  Lokesh, since I think Andreas is
>>> out currently can you expand a little on what we can/can't do on this
>>> platform?  Thanks!
>>
>> The reason why BSS is needed very early in this platform is for the following
>> reasons:
>> - System co-processor is the central resource manager in SoC and should be
>> loaded and started very early in the boot process. Without that no peripheral or
>> memory can be initialized. So for loading system co-processor image, we only
>> have limited SRAM and a peripheral initialized by ROM.
>> - System co-processor(DMSC) is being represented as remote-core in
>> Device-tree(We are strictly following DM and DT model for the entire SoC).
>> - Since DM is also followed by each peripheral device and remote core, DM should
>> be enabled very early and many peripheral drivers are dependent on BSS usage.
>> So, BSS has been made available very early.
>>
>> Hope this is clear. Let me know if more details are required, I will be happy to
>> explain.
> 
> Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> stays consistent throught U-Boot.

I understand and agree that it should be consistent. Just discussed this with
Andreas, who is courteous enough to update the details in his vacation.

> 
> The reasons you stated still don't make it clear to me *why* bss is needed
> early. There are other boards using DM early that don't need this. In my
> opinion, DM drivers normally don't rely on BSS but keep all their state in

This statement doesn't hold true for all the drviers. At least the mmc driver
uses "initialized" variable stored in BSS to avoid initializing mmc multiple
times[0]. In the past we en counted other drivers using it. I guess the idea
here is to enable the BSS support generically instead of fixing each of every
driver.

> heap memory. If you only need BSS early because drivers rely on BSS, you might
> have to fix those drivers?

So, correct me here, why should driver be restricted to not use BSS?

Also doing a grep for bss usage very early in board_init_f produced many results:
➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
arch/arm/mach-socfpga/spl_gen5.c
arch/arm/mach-zynqmp/spl.c
arch/mips/mach-jz47xx/jz4780/jz4780.c
board/barco/platinum/spl_picon.c
board/barco/platinum/spl_titanium.c
board/compulab/cl-som-imx7/spl.c
board/congatec/cgtqmx6eval/cgtqmx6eval.c
board/dhelectronics/dh_imx6/dh_imx6_spl.c
board/el/el6x/el6x.c
board/freescale/imx8mq_evk/spl.c
board/freescale/imx8qm_mek/spl.c
board/freescale/imx8qxp_mek/spl.c
board/freescale/ls1021aiot/ls1021aiot.c
board/freescale/ls1021aqds/ls1021aqds.c
board/freescale/ls1021atwr/ls1021atwr.c
board/freescale/mx6sabreauto/mx6sabreauto.c
board/freescale/mx6sabresd/mx6sabresd.c
board/freescale/mx6slevk/mx6slevk.c
board/freescale/mx6sxsabresd/mx6sxsabresd.c
board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
board/liebherr/display5/spl.c
board/logicpd/imx6/imx6logic.c
board/phytec/pcm058/pcm058.c
board/phytec/pfla02/pfla02.c
board/sks-kinkel/sksimx6/sksimx6.c
board/solidrun/mx6cuboxi/mx6cuboxi.c
board/technexion/pico-imx6ul/spl.c
board/technexion/pico-imx7d/spl.c
board/toradex/apalis_imx6/apalis_imx6.c
board/toradex/colibri_imx6/colibri_imx6.c
board/udoo/neo/neo.c
board/variscite/dart_6ul/spl.c
board/woodburn/woodburn.c

There might be some false positive cases but most of the above files are
utilizing bss in board_init_f.

> 
> Further, allowing BSS early is still against what the main README says, so I
> want to raise the question again: shouldn't this main README be changed if we
> suddenly allow BSS to be used early (because that main README says we can'that
> do that)?

I do agree on this part. We should fix README in this case.

[0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832

Thanks and regards,
Lokesh

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
> Hi Simon,
>
> On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> > Hi Lokesh,
> >
> > thanks for following up on this.
> >
> > On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> >>
> >> Hi Tom,
> >>
> >> On 20/07/19 9:21 PM, Tom Rini wrote:
> >>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> >>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> >>>>>
> >>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> >>>>>
> >>>>>> In order to be able to use more advanced driver functionality which often
> >>>>>> relies on having BSS initialized during early boot prior to relocation
> >>>>>> several things need to be in place:
> >>>>>>
> >>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
> >>>>>>    at the top of the MCU SRAM area, with the stack starting right below
> >>>>>>    it,
> >>>>>> 2) We need to move the initialization of BSS prior to entering
> >>>>>>    board_init_f(). We will do this with a separate commit by turning on
> >>>>>>    the respective CONFIG option.
> >>>>>>
> >>>>>> In this commit we also clean up the assignment of the initial SP address
> >>>>>> as part of the refactoring, taking into account the pre-decrement post-
> >>>>>> increment nature in which the SP is used on ARM.
> >>>>>>
> >>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> >>>>>
> >>>>> Applied to u-boot/master, thanks!
> >>>>
> >>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> >>>> but in a discussion about a similar patch I sent [1], using BSS from
> >>>> board_init_f
> >>>> was turned down. And Simon Glass rather convinced me that this is the current
> >>>> API U-Boot has (and is documented in README).
> >>>>
> >>>> So either we must change this API and its documentation (and I would expect the
> >>>> author of this patch to combine the README change with the code change), or this
> >>>> patch would have to be rejected.
> >>>>
> >>>> Again, I'm sorry I only see this now. In thought to remember a
> >>>> discussion in this
> >>>> thread, but I clearly remember that wrong...
> >>>>
> >>>> [1] https://patchwork.ozlabs.org/patch/1057237/
> >>>
> >>> And I had missed that other thread.  Lokesh, since I think Andreas is
> >>> out currently can you expand a little on what we can/can't do on this
> >>> platform?  Thanks!
> >>
> >> The reason why BSS is needed very early in this platform is for the following
> >> reasons:
> >> - System co-processor is the central resource manager in SoC and should be
> >> loaded and started very early in the boot process. Without that no peripheral or
> >> memory can be initialized. So for loading system co-processor image, we only
> >> have limited SRAM and a peripheral initialized by ROM.
> >> - System co-processor(DMSC) is being represented as remote-core in
> >> Device-tree(We are strictly following DM and DT model for the entire SoC).
> >> - Since DM is also followed by each peripheral device and remote core, DM should
> >> be enabled very early and many peripheral drivers are dependent on BSS usage.
> >> So, BSS has been made available very early.
> >>
> >> Hope this is clear. Let me know if more details are required, I will be happy to
> >> explain.
> >
> > Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> > stays consistent throught U-Boot.
>
> I understand and agree that it should be consistent. Just discussed this with
> Andreas, who is courteous enough to update the details in his vacation.

We don't have to rush here, I don't have a problem waiting for Andreas to
answer when he's back.

>
> >
> > The reasons you stated still don't make it clear to me *why* bss is needed
> > early. There are other boards using DM early that don't need this. In my
> > opinion, DM drivers normally don't rely on BSS but keep all their state in
>
> This statement doesn't hold true for all the drviers. At least the mmc driver
> uses "initialized" variable stored in BSS to avoid initializing mmc multiple
> times[0]. In the past we en counted other drivers using it. I guess the idea
> here is to enable the BSS support generically instead of fixing each of every
> driver.

So this driver is generally not usable in pre-relocation phase? The README
document is pretty clear about BSS not being available in board_init_f. I know
this text is old, but it seems still valid.

And if this is really a workaround because it's easier to use this workaround
instead of fixing drivers that invalidly use BSS, is this what we want?

>
> > heap memory. If you only need BSS early because drivers rely on BSS, you might
> > have to fix those drivers?
>
> So, correct me here, why should driver be restricted to not use BSS?

Post-relocation drivers might be free to use BSS (although you lose the
per-instance storage when using BSS instead of the driver's priv data),
but pre-relocation drivers are not.
That's the current definition in U-Boot. This patch changes it by
adding the option
to use BSS early. This bears the danger of code being changed in a way that
it requires BSS to be available early and might not work on other boards that
actually cannot provide BSS early (e.g. before SDRAM is up or whatever).

>
> Also doing a grep for bss usage very early in board_init_f produced many results:
> ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
> arch/arm/mach-socfpga/spl_gen5.c

Right, that's my responsibility, and there's a patch in Marek's queue
to fix this:
the DDR driver used BSS and I simply moved it's BSS variables to its driver.
Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.

> arch/arm/mach-zynqmp/spl.c
> arch/mips/mach-jz47xx/jz4780/jz4780.c
> board/barco/platinum/spl_picon.c
> board/barco/platinum/spl_titanium.c
> board/compulab/cl-som-imx7/spl.c
> board/congatec/cgtqmx6eval/cgtqmx6eval.c
> board/dhelectronics/dh_imx6/dh_imx6_spl.c
> board/el/el6x/el6x.c
> board/freescale/imx8mq_evk/spl.c
> board/freescale/imx8qm_mek/spl.c
> board/freescale/imx8qxp_mek/spl.c
> board/freescale/ls1021aiot/ls1021aiot.c
> board/freescale/ls1021aqds/ls1021aqds.c
> board/freescale/ls1021atwr/ls1021atwr.c
> board/freescale/mx6sabreauto/mx6sabreauto.c
> board/freescale/mx6sabresd/mx6sabresd.c
> board/freescale/mx6slevk/mx6slevk.c
> board/freescale/mx6sxsabresd/mx6sxsabresd.c
> board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
> board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
> board/liebherr/display5/spl.c
> board/logicpd/imx6/imx6logic.c
> board/phytec/pcm058/pcm058.c
> board/phytec/pfla02/pfla02.c
> board/sks-kinkel/sksimx6/sksimx6.c
> board/solidrun/mx6cuboxi/mx6cuboxi.c
> board/technexion/pico-imx6ul/spl.c
> board/technexion/pico-imx7d/spl.c
> board/toradex/apalis_imx6/apalis_imx6.c
> board/toradex/colibri_imx6/colibri_imx6.c
> board/udoo/neo/neo.c
> board/variscite/dart_6ul/spl.c
> board/woodburn/woodburn.c
>
> There might be some false positive cases but most of the above files are
> utilizing bss in board_init_f.

So all these boards include a hack that's against what's the currently
documented status.

>
> >
> > Further, allowing BSS early is still against what the main README says, so I
> > want to raise the question again: shouldn't this main README be changed if we
> > suddenly allow BSS to be used early (because that main README says we can'that
> > do that)?
>
> I do agree on this part. We should fix README in this case.

My point (Simon Glass has convinced me in the previous discussion I
mentioned) is that there *are* boards that can't use BSS early. You can't just
allow all code to use BSS early as you risk breaking such boards.

If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
and changing the README.

>
> [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832

Right, that one looks strange. It seems to me that's some kind of leftover
code from pre-DM? I would have expected the UCLASS for mmc drivers
to include this 'initialized' flag instead of this ugly "static in
function" thing.

Regards,
Simon

>
> Thanks and regards,
> Lokesh
>

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Andreas Dannenberg]

Hi Simon,
thanks for your patience waiting for a response. Please see comments inlined...

On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
> On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> >
> > Hi Simon,
> >
> > On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> > > Hi Lokesh,
> > >
> > > thanks for following up on this.
> > >
> > > On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > >>
> > >> Hi Tom,
> > >>
> > >> On 20/07/19 9:21 PM, Tom Rini wrote:
> > >>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> > >>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> > >>>>>
> > >>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> > >>>>>
> > >>>>>> In order to be able to use more advanced driver functionality which often
> > >>>>>> relies on having BSS initialized during early boot prior to relocation
> > >>>>>> several things need to be in place:
> > >>>>>>
> > >>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
> > >>>>>>    at the top of the MCU SRAM area, with the stack starting right below
> > >>>>>>    it,
> > >>>>>> 2) We need to move the initialization of BSS prior to entering
> > >>>>>>    board_init_f(). We will do this with a separate commit by turning on
> > >>>>>>    the respective CONFIG option.
> > >>>>>>
> > >>>>>> In this commit we also clean up the assignment of the initial SP address
> > >>>>>> as part of the refactoring, taking into account the pre-decrement post-
> > >>>>>> increment nature in which the SP is used on ARM.
> > >>>>>>
> > >>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > >>>>>
> > >>>>> Applied to u-boot/master, thanks!
> > >>>>
> > >>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> > >>>> but in a discussion about a similar patch I sent [1], using BSS from
> > >>>> board_init_f
> > >>>> was turned down. And Simon Glass rather convinced me that this is the current
> > >>>> API U-Boot has (and is documented in README).
> > >>>>
> > >>>> So either we must change this API and its documentation (and I would expect the
> > >>>> author of this patch to combine the README change with the code change), or this
> > >>>> patch would have to be rejected.
> > >>>>
> > >>>> Again, I'm sorry I only see this now. In thought to remember a
> > >>>> discussion in this
> > >>>> thread, but I clearly remember that wrong...
> > >>>>
> > >>>> [1] https://patchwork.ozlabs.org/patch/1057237/
> > >>>
> > >>> And I had missed that other thread.  Lokesh, since I think Andreas is
> > >>> out currently can you expand a little on what we can/can't do on this
> > >>> platform?  Thanks!
> > >>
> > >> The reason why BSS is needed very early in this platform is for the following
> > >> reasons:
> > >> - System co-processor is the central resource manager in SoC and should be
> > >> loaded and started very early in the boot process. Without that no peripheral or
> > >> memory can be initialized. So for loading system co-processor image, we only
> > >> have limited SRAM and a peripheral initialized by ROM.
> > >> - System co-processor(DMSC) is being represented as remote-core in
> > >> Device-tree(We are strictly following DM and DT model for the entire SoC).
> > >> - Since DM is also followed by each peripheral device and remote core, DM should
> > >> be enabled very early and many peripheral drivers are dependent on BSS usage.
> > >> So, BSS has been made available very early.
> > >>
> > >> Hope this is clear. Let me know if more details are required, I will be happy to
> > >> explain.
> > >
> > > Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> > > stays consistent throught U-Boot.
> >
> > I understand and agree that it should be consistent. Just discussed this with
> > Andreas, who is courteous enough to update the details in his vacation.
> 
> We don't have to rush here, I don't have a problem waiting for Andreas to
> answer when he's back.
> 
> >
> > >
> > > The reasons you stated still don't make it clear to me *why* bss is needed
> > > early. There are other boards using DM early that don't need this. In my
> > > opinion, DM drivers normally don't rely on BSS but keep all their state in
> >
> > This statement doesn't hold true for all the drviers. At least the mmc driver
> > uses "initialized" variable stored in BSS to avoid initializing mmc multiple
> > times[0]. In the past we en counted other drivers using it. I guess the idea
> > here is to enable the BSS support generically instead of fixing each of every
> > driver.
> 
> So this driver is generally not usable in pre-relocation phase? The README
> document is pretty clear about BSS not being available in board_init_f. I know
> this text is old, but it seems still valid.
> 
> And if this is really a workaround because it's easier to use this workaround
> instead of fixing drivers that invalidly use BSS, is this what we want?
> 
> >
> > > heap memory. If you only need BSS early because drivers rely on BSS, you might
> > > have to fix those drivers?
> >
> > So, correct me here, why should driver be restricted to not use BSS?
> 
> Post-relocation drivers might be free to use BSS (although you lose the
> per-instance storage when using BSS instead of the driver's priv data),
> but pre-relocation drivers are not.
> That's the current definition in U-Boot. This patch changes it by
> adding the option
> to use BSS early. This bears the danger of code being changed in a way that
> it requires BSS to be available early and might not work on other boards that
> actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
> 
> >
> > Also doing a grep for bss usage very early in board_init_f produced many results:
> > ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
> > arch/arm/mach-socfpga/spl_gen5.c
> 
> Right, that's my responsibility, and there's a patch in Marek's queue
> to fix this:
> the DDR driver used BSS and I simply moved it's BSS variables to its driver.
> Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
> 
> > arch/arm/mach-zynqmp/spl.c
> > arch/mips/mach-jz47xx/jz4780/jz4780.c
> > board/barco/platinum/spl_picon.c
> > board/barco/platinum/spl_titanium.c
> > board/compulab/cl-som-imx7/spl.c
> > board/congatec/cgtqmx6eval/cgtqmx6eval.c
> > board/dhelectronics/dh_imx6/dh_imx6_spl.c
> > board/el/el6x/el6x.c
> > board/freescale/imx8mq_evk/spl.c
> > board/freescale/imx8qm_mek/spl.c
> > board/freescale/imx8qxp_mek/spl.c
> > board/freescale/ls1021aiot/ls1021aiot.c
> > board/freescale/ls1021aqds/ls1021aqds.c
> > board/freescale/ls1021atwr/ls1021atwr.c
> > board/freescale/mx6sabreauto/mx6sabreauto.c
> > board/freescale/mx6sabresd/mx6sabresd.c
> > board/freescale/mx6slevk/mx6slevk.c
> > board/freescale/mx6sxsabresd/mx6sxsabresd.c
> > board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
> > board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
> > board/liebherr/display5/spl.c
> > board/logicpd/imx6/imx6logic.c
> > board/phytec/pcm058/pcm058.c
> > board/phytec/pfla02/pfla02.c
> > board/sks-kinkel/sksimx6/sksimx6.c
> > board/solidrun/mx6cuboxi/mx6cuboxi.c
> > board/technexion/pico-imx6ul/spl.c
> > board/technexion/pico-imx7d/spl.c
> > board/toradex/apalis_imx6/apalis_imx6.c
> > board/toradex/colibri_imx6/colibri_imx6.c
> > board/udoo/neo/neo.c
> > board/variscite/dart_6ul/spl.c
> > board/woodburn/woodburn.c
> >
> > There might be some false positive cases but most of the above files are
> > utilizing bss in board_init_f.
> 
> So all these boards include a hack that's against what's the currently
> documented status.

Yes implementation and doc need to stay consistent.

> >
> > >
> > > Further, allowing BSS early is still against what the main README says, so I
> > > want to raise the question again: shouldn't this main README be changed if we
> > > suddenly allow BSS to be used early (because that main README says we can'that
> > > do that)?
> >
> > I do agree on this part. We should fix README in this case.

I can prepare a PATCH to propose an update to the README, it definitely
should stay in sync with the implementation, independent of the path we
are choosing to potentially make any improvements moving forward.
 
> My point (Simon Glass has convinced me in the previous discussion I
> mentioned) is that there *are* boards that can't use BSS early. You can't just
> allow all code to use BSS early as you risk breaking such boards.

That's why the early BSS option has to be turned on explicitly. Nobody
requires you to use early BSS. It should be considered an option for
certain limited use cases (and described as such in an update to README,
like there are many other very "special" options in U-Boot that have no
wide use). But I guess one of your concerns as you alluded to earlier
is that it may result in incompatibilities moving forward as this
essentially lowers the "barrier of entry" to using this feature,
potentially spilling into drivers or other common files?

https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
 
> If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
> and changing the README.
> 
> >
> > [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
> 
> Right, that one looks strange. It seems to me that's some kind of leftover
> code from pre-DM? I would have expected the UCLASS for mmc drivers
> to include this 'initialized' flag instead of this ugly "static in
> function" thing.

I did a quick search, some other users of BSS that could potentially have an
impact in the context of "early FW loading from SPL board_init_f()" include...

'static int fat_registered' in...
https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19

'static struct mmc *mmc' in ...
https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
(actually I'm responsible for the 'static' there)

'static int usb_stor_curr_dev' in...
https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18

Then, there are also quite a few instances in drivers/, many of them not
relevant to operating from SPL's board_init_f() context, with some of
them however possibly being affected like these:

'static struct device_node *of_aliases' in...
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35

'static int reloc_done' in...
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90

'static bool sf_mtd_registered' in....
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13

or

'static ulong next_reset' in...
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76

Now, I have not validated each and every one of those (beyond 'fat_registered'
which I know is problematic), and there are more, for having an impact or not.
Whether all of those need any fixes or improvements set aside for a moment, at
a minimum doesn't it make you concerned about stability of code execution
without initialized BSS, no?


--
Andreas Dannenberg
Texas Instruments Inc

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

Hi Andreas,

On Wed, Aug 7, 2019 at 11:24 PM Andreas Dannenberg <dannenberg@ti.com> wrote:
>
> Hi Simon,
> thanks for your patience waiting for a response. Please see comments inlined...
>
> On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
> > On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> > > > Hi Lokesh,
> > > >
> > > > thanks for following up on this.
> > > >
> > > > On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > > >>
> > > >> Hi Tom,
> > > >>
> > > >> On 20/07/19 9:21 PM, Tom Rini wrote:
> > > >>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> > > >>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> > > >>>>>
> > > >>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> > > >>>>>
> > > >>>>>> In order to be able to use more advanced driver functionality which often
> > > >>>>>> relies on having BSS initialized during early boot prior to relocation
> > > >>>>>> several things need to be in place:
> > > >>>>>>
> > > >>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
> > > >>>>>>    at the top of the MCU SRAM area, with the stack starting right below
> > > >>>>>>    it,
> > > >>>>>> 2) We need to move the initialization of BSS prior to entering
> > > >>>>>>    board_init_f(). We will do this with a separate commit by turning on
> > > >>>>>>    the respective CONFIG option.
> > > >>>>>>
> > > >>>>>> In this commit we also clean up the assignment of the initial SP address
> > > >>>>>> as part of the refactoring, taking into account the pre-decrement post-
> > > >>>>>> increment nature in which the SP is used on ARM.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > > >>>>>
> > > >>>>> Applied to u-boot/master, thanks!
> > > >>>>
> > > >>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> > > >>>> but in a discussion about a similar patch I sent [1], using BSS from
> > > >>>> board_init_f
> > > >>>> was turned down. And Simon Glass rather convinced me that this is the current
> > > >>>> API U-Boot has (and is documented in README).
> > > >>>>
> > > >>>> So either we must change this API and its documentation (and I would expect the
> > > >>>> author of this patch to combine the README change with the code change), or this
> > > >>>> patch would have to be rejected.
> > > >>>>
> > > >>>> Again, I'm sorry I only see this now. In thought to remember a
> > > >>>> discussion in this
> > > >>>> thread, but I clearly remember that wrong...
> > > >>>>
> > > >>>> [1] https://patchwork.ozlabs.org/patch/1057237/
> > > >>>
> > > >>> And I had missed that other thread.  Lokesh, since I think Andreas is
> > > >>> out currently can you expand a little on what we can/can't do on this
> > > >>> platform?  Thanks!
> > > >>
> > > >> The reason why BSS is needed very early in this platform is for the following
> > > >> reasons:
> > > >> - System co-processor is the central resource manager in SoC and should be
> > > >> loaded and started very early in the boot process. Without that no peripheral or
> > > >> memory can be initialized. So for loading system co-processor image, we only
> > > >> have limited SRAM and a peripheral initialized by ROM.
> > > >> - System co-processor(DMSC) is being represented as remote-core in
> > > >> Device-tree(We are strictly following DM and DT model for the entire SoC).
> > > >> - Since DM is also followed by each peripheral device and remote core, DM should
> > > >> be enabled very early and many peripheral drivers are dependent on BSS usage.
> > > >> So, BSS has been made available very early.
> > > >>
> > > >> Hope this is clear. Let me know if more details are required, I will be happy to
> > > >> explain.
> > > >
> > > > Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> > > > stays consistent throught U-Boot.
> > >
> > > I understand and agree that it should be consistent. Just discussed this with
> > > Andreas, who is courteous enough to update the details in his vacation.
> >
> > We don't have to rush here, I don't have a problem waiting for Andreas to
> > answer when he's back.
> >
> > >
> > > >
> > > > The reasons you stated still don't make it clear to me *why* bss is needed
> > > > early. There are other boards using DM early that don't need this. In my
> > > > opinion, DM drivers normally don't rely on BSS but keep all their state in
> > >
> > > This statement doesn't hold true for all the drviers. At least the mmc driver
> > > uses "initialized" variable stored in BSS to avoid initializing mmc multiple
> > > times[0]. In the past we en counted other drivers using it. I guess the idea
> > > here is to enable the BSS support generically instead of fixing each of every
> > > driver.
> >
> > So this driver is generally not usable in pre-relocation phase? The README
> > document is pretty clear about BSS not being available in board_init_f. I know
> > this text is old, but it seems still valid.
> >
> > And if this is really a workaround because it's easier to use this workaround
> > instead of fixing drivers that invalidly use BSS, is this what we want?
> >
> > >
> > > > heap memory. If you only need BSS early because drivers rely on BSS, you might
> > > > have to fix those drivers?
> > >
> > > So, correct me here, why should driver be restricted to not use BSS?
> >
> > Post-relocation drivers might be free to use BSS (although you lose the
> > per-instance storage when using BSS instead of the driver's priv data),
> > but pre-relocation drivers are not.
> > That's the current definition in U-Boot. This patch changes it by
> > adding the option
> > to use BSS early. This bears the danger of code being changed in a way that
> > it requires BSS to be available early and might not work on other boards that
> > actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
> >
> > >
> > > Also doing a grep for bss usage very early in board_init_f produced many results:
> > > ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
> > > arch/arm/mach-socfpga/spl_gen5.c
> >
> > Right, that's my responsibility, and there's a patch in Marek's queue
> > to fix this:
> > the DDR driver used BSS and I simply moved it's BSS variables to its driver.
> > Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
> >
> > > arch/arm/mach-zynqmp/spl.c
> > > arch/mips/mach-jz47xx/jz4780/jz4780.c
> > > board/barco/platinum/spl_picon.c
> > > board/barco/platinum/spl_titanium.c
> > > board/compulab/cl-som-imx7/spl.c
> > > board/congatec/cgtqmx6eval/cgtqmx6eval.c
> > > board/dhelectronics/dh_imx6/dh_imx6_spl.c
> > > board/el/el6x/el6x.c
> > > board/freescale/imx8mq_evk/spl.c
> > > board/freescale/imx8qm_mek/spl.c
> > > board/freescale/imx8qxp_mek/spl.c
> > > board/freescale/ls1021aiot/ls1021aiot.c
> > > board/freescale/ls1021aqds/ls1021aqds.c
> > > board/freescale/ls1021atwr/ls1021atwr.c
> > > board/freescale/mx6sabreauto/mx6sabreauto.c
> > > board/freescale/mx6sabresd/mx6sabresd.c
> > > board/freescale/mx6slevk/mx6slevk.c
> > > board/freescale/mx6sxsabresd/mx6sxsabresd.c
> > > board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
> > > board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
> > > board/liebherr/display5/spl.c
> > > board/logicpd/imx6/imx6logic.c
> > > board/phytec/pcm058/pcm058.c
> > > board/phytec/pfla02/pfla02.c
> > > board/sks-kinkel/sksimx6/sksimx6.c
> > > board/solidrun/mx6cuboxi/mx6cuboxi.c
> > > board/technexion/pico-imx6ul/spl.c
> > > board/technexion/pico-imx7d/spl.c
> > > board/toradex/apalis_imx6/apalis_imx6.c
> > > board/toradex/colibri_imx6/colibri_imx6.c
> > > board/udoo/neo/neo.c
> > > board/variscite/dart_6ul/spl.c
> > > board/woodburn/woodburn.c
> > >
> > > There might be some false positive cases but most of the above files are
> > > utilizing bss in board_init_f.
> >
> > So all these boards include a hack that's against what's the currently
> > documented status.
>
> Yes implementation and doc need to stay consistent.
>
> > >
> > > >
> > > > Further, allowing BSS early is still against what the main README says, so I
> > > > want to raise the question again: shouldn't this main README be changed if we
> > > > suddenly allow BSS to be used early (because that main README says we can'that
> > > > do that)?
> > >
> > > I do agree on this part. We should fix README in this case.
>
> I can prepare a PATCH to propose an update to the README, it definitely
> should stay in sync with the implementation, independent of the path we
> are choosing to potentially make any improvements moving forward.

That would be very welcome!

>
> > My point (Simon Glass has convinced me in the previous discussion I
> > mentioned) is that there *are* boards that can't use BSS early. You can't just
> > allow all code to use BSS early as you risk breaking such boards.
>
> That's why the early BSS option has to be turned on explicitly. Nobody
> requires you to use early BSS. It should be considered an option for
> certain limited use cases (and described as such in an update to README,
> like there are many other very "special" options in U-Boot that have no
> wide use). But I guess one of your concerns as you alluded to earlier
> is that it may result in incompatibilities moving forward as this
> essentially lowers the "barrier of entry" to using this feature,
> potentially spilling into drivers or other common files?

Well, yes, that's my main concern. I'm not against boards using BSS early.
In fact, I started an attempt for socfpga_a10 to use BSS early some months
ago. However, I am concerned that this gets the default for some boards and
people just don't notice using BSS. Given the lack of discussion on this
patch before it was merged, I can easly imagine patches making use of BSS
slipping in.

>
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
>
> > If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
> > and changing the README.
> >
> > >
> > > [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
> >
> > Right, that one looks strange. It seems to me that's some kind of leftover
> > code from pre-DM? I would have expected the UCLASS for mmc drivers
> > to include this 'initialized' flag instead of this ugly "static in
> > function" thing.
>
> I did a quick search, some other users of BSS that could potentially have an
> impact in the context of "early FW loading from SPL board_init_f()" include...
>
> 'static int fat_registered' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19
>
> 'static struct mmc *mmc' in ...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
> (actually I'm responsible for the 'static' there)
>
> 'static int usb_stor_curr_dev' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18
>
> Then, there are also quite a few instances in drivers/, many of them not
> relevant to operating from SPL's board_init_f() context, with some of
> them however possibly being affected like these:
>
> 'static struct device_node *of_aliases' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35
>
> 'static int reloc_done' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90
>
> 'static bool sf_mtd_registered' in....
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13
>
> or
>
> 'static ulong next_reset' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76

Great to have that list! What I'm still missing is a more detailed reason
*why* you need BSS early If it's because of one of the invalid usages above,
it might be better to fix them. If it's because of memory shortage: why
wouldn't the heap be enough to solve that problem? If it's because of heap
shortage when using simple malloc (I had that on socfpga_a10): maybe it can
be fixed in a different way?

>
> Now, I have not validated each and every one of those (beyond 'fat_registered'
> which I know is problematic), and there are more, for having an impact or not.
> Whether all of those need any fixes or improvements set aside for a moment, at
> a minimum doesn't it make you concerned about stability of code execution
> without initialized BSS, no?

Of course it does! I know there are files using BSS when they shouldn't;
socfpga_gen5 SPL was one of them. The problem is that either people don't know
they shouldn't be using BSS or they don't care (because it just works).

What I'm afraid of with this new config option is that using BSS early might
become more or less the standard (as there *are* many boards having it
available from the start), which might easily result in boards that cannot use
BSS ealry being broken.

Sadly, I failed to come up with a way of how to detect such invalid usage of
BSS at compile time or runtime.

Regards,
Simon

>
>
> --
> Andreas Dannenberg
> Texas Instruments Inc

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Andreas Dannenberg]

Simon,

On Thu, Aug 08, 2019 at 09:29:03AM +0200, Simon Goldschmidt wrote:
> Hi Andreas,
> 
> On Wed, Aug 7, 2019 at 11:24 PM Andreas Dannenberg <dannenberg@ti.com> wrote:
> >
> > Hi Simon,
> > thanks for your patience waiting for a response. Please see comments inlined...
> >
> > On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
> > > On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> > > > > Hi Lokesh,
> > > > >
> > > > > thanks for following up on this.
> > > > >
> > > > > On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > > > >>
> > > > >> Hi Tom,
> > > > >>
> > > > >> On 20/07/19 9:21 PM, Tom Rini wrote:
> > > > >>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> > > > >>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> > > > >>>>>
> > > > >>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> > > > >>>>>
> > > > >>>>>> In order to be able to use more advanced driver functionality which often
> > > > >>>>>> relies on having BSS initialized during early boot prior to relocation
> > > > >>>>>> several things need to be in place:
> > > > >>>>>>
> > > > >>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
> > > > >>>>>>    at the top of the MCU SRAM area, with the stack starting right below
> > > > >>>>>>    it,
> > > > >>>>>> 2) We need to move the initialization of BSS prior to entering
> > > > >>>>>>    board_init_f(). We will do this with a separate commit by turning on
> > > > >>>>>>    the respective CONFIG option.
> > > > >>>>>>
> > > > >>>>>> In this commit we also clean up the assignment of the initial SP address
> > > > >>>>>> as part of the refactoring, taking into account the pre-decrement post-
> > > > >>>>>> increment nature in which the SP is used on ARM.
> > > > >>>>>>
> > > > >>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > > > >>>>>
> > > > >>>>> Applied to u-boot/master, thanks!
> > > > >>>>
> > > > >>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
> > > > >>>> but in a discussion about a similar patch I sent [1], using BSS from
> > > > >>>> board_init_f
> > > > >>>> was turned down. And Simon Glass rather convinced me that this is the current
> > > > >>>> API U-Boot has (and is documented in README).
> > > > >>>>
> > > > >>>> So either we must change this API and its documentation (and I would expect the
> > > > >>>> author of this patch to combine the README change with the code change), or this
> > > > >>>> patch would have to be rejected.
> > > > >>>>
> > > > >>>> Again, I'm sorry I only see this now. In thought to remember a
> > > > >>>> discussion in this
> > > > >>>> thread, but I clearly remember that wrong...
> > > > >>>>
> > > > >>>> [1] https://patchwork.ozlabs.org/patch/1057237/
> > > > >>>
> > > > >>> And I had missed that other thread.  Lokesh, since I think Andreas is
> > > > >>> out currently can you expand a little on what we can/can't do on this
> > > > >>> platform?  Thanks!
> > > > >>
> > > > >> The reason why BSS is needed very early in this platform is for the following
> > > > >> reasons:
> > > > >> - System co-processor is the central resource manager in SoC and should be
> > > > >> loaded and started very early in the boot process. Without that no peripheral or
> > > > >> memory can be initialized. So for loading system co-processor image, we only
> > > > >> have limited SRAM and a peripheral initialized by ROM.
> > > > >> - System co-processor(DMSC) is being represented as remote-core in
> > > > >> Device-tree(We are strictly following DM and DT model for the entire SoC).
> > > > >> - Since DM is also followed by each peripheral device and remote core, DM should
> > > > >> be enabled very early and many peripheral drivers are dependent on BSS usage.
> > > > >> So, BSS has been made available very early.
> > > > >>
> > > > >> Hope this is clear. Let me know if more details are required, I will be happy to
> > > > >> explain.
> > > > >
> > > > > Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> > > > > stays consistent throught U-Boot.
> > > >
> > > > I understand and agree that it should be consistent. Just discussed this with
> > > > Andreas, who is courteous enough to update the details in his vacation.
> > >
> > > We don't have to rush here, I don't have a problem waiting for Andreas to
> > > answer when he's back.
> > >
> > > >
> > > > >
> > > > > The reasons you stated still don't make it clear to me *why* bss is needed
> > > > > early. There are other boards using DM early that don't need this. In my
> > > > > opinion, DM drivers normally don't rely on BSS but keep all their state in
> > > >
> > > > This statement doesn't hold true for all the drviers. At least the mmc driver
> > > > uses "initialized" variable stored in BSS to avoid initializing mmc multiple
> > > > times[0]. In the past we en counted other drivers using it. I guess the idea
> > > > here is to enable the BSS support generically instead of fixing each of every
> > > > driver.
> > >
> > > So this driver is generally not usable in pre-relocation phase? The README
> > > document is pretty clear about BSS not being available in board_init_f. I know
> > > this text is old, but it seems still valid.
> > >
> > > And if this is really a workaround because it's easier to use this workaround
> > > instead of fixing drivers that invalidly use BSS, is this what we want?
> > >
> > > >
> > > > > heap memory. If you only need BSS early because drivers rely on BSS, you might
> > > > > have to fix those drivers?
> > > >
> > > > So, correct me here, why should driver be restricted to not use BSS?
> > >
> > > Post-relocation drivers might be free to use BSS (although you lose the
> > > per-instance storage when using BSS instead of the driver's priv data),
> > > but pre-relocation drivers are not.
> > > That's the current definition in U-Boot. This patch changes it by
> > > adding the option
> > > to use BSS early. This bears the danger of code being changed in a way that
> > > it requires BSS to be available early and might not work on other boards that
> > > actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
> > >
> > > >
> > > > Also doing a grep for bss usage very early in board_init_f produced many results:
> > > > ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
> > > > arch/arm/mach-socfpga/spl_gen5.c
> > >
> > > Right, that's my responsibility, and there's a patch in Marek's queue
> > > to fix this:
> > > the DDR driver used BSS and I simply moved it's BSS variables to its driver.
> > > Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
> > >
> > > > arch/arm/mach-zynqmp/spl.c
> > > > arch/mips/mach-jz47xx/jz4780/jz4780.c
> > > > board/barco/platinum/spl_picon.c
> > > > board/barco/platinum/spl_titanium.c
> > > > board/compulab/cl-som-imx7/spl.c
> > > > board/congatec/cgtqmx6eval/cgtqmx6eval.c
> > > > board/dhelectronics/dh_imx6/dh_imx6_spl.c
> > > > board/el/el6x/el6x.c
> > > > board/freescale/imx8mq_evk/spl.c
> > > > board/freescale/imx8qm_mek/spl.c
> > > > board/freescale/imx8qxp_mek/spl.c
> > > > board/freescale/ls1021aiot/ls1021aiot.c
> > > > board/freescale/ls1021aqds/ls1021aqds.c
> > > > board/freescale/ls1021atwr/ls1021atwr.c
> > > > board/freescale/mx6sabreauto/mx6sabreauto.c
> > > > board/freescale/mx6sabresd/mx6sabresd.c
> > > > board/freescale/mx6slevk/mx6slevk.c
> > > > board/freescale/mx6sxsabresd/mx6sxsabresd.c
> > > > board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
> > > > board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
> > > > board/liebherr/display5/spl.c
> > > > board/logicpd/imx6/imx6logic.c
> > > > board/phytec/pcm058/pcm058.c
> > > > board/phytec/pfla02/pfla02.c
> > > > board/sks-kinkel/sksimx6/sksimx6.c
> > > > board/solidrun/mx6cuboxi/mx6cuboxi.c
> > > > board/technexion/pico-imx6ul/spl.c
> > > > board/technexion/pico-imx7d/spl.c
> > > > board/toradex/apalis_imx6/apalis_imx6.c
> > > > board/toradex/colibri_imx6/colibri_imx6.c
> > > > board/udoo/neo/neo.c
> > > > board/variscite/dart_6ul/spl.c
> > > > board/woodburn/woodburn.c
> > > >
> > > > There might be some false positive cases but most of the above files are
> > > > utilizing bss in board_init_f.
> > >
> > > So all these boards include a hack that's against what's the currently
> > > documented status.
> >
> > Yes implementation and doc need to stay consistent.
> >
> > > >
> > > > >
> > > > > Further, allowing BSS early is still against what the main README says, so I
> > > > > want to raise the question again: shouldn't this main README be changed if we
> > > > > suddenly allow BSS to be used early (because that main README says we can'that
> > > > > do that)?
> > > >
> > > > I do agree on this part. We should fix README in this case.
> >
> > I can prepare a PATCH to propose an update to the README, it definitely
> > should stay in sync with the implementation, independent of the path we
> > are choosing to potentially make any improvements moving forward.
> 
> That would be very welcome!

Proposal submitted.
 
> > > My point (Simon Glass has convinced me in the previous discussion I
> > > mentioned) is that there *are* boards that can't use BSS early. You can't just
> > > allow all code to use BSS early as you risk breaking such boards.
> >
> > That's why the early BSS option has to be turned on explicitly. Nobody
> > requires you to use early BSS. It should be considered an option for
> > certain limited use cases (and described as such in an update to README,
> > like there are many other very "special" options in U-Boot that have no
> > wide use). But I guess one of your concerns as you alluded to earlier
> > is that it may result in incompatibilities moving forward as this
> > essentially lowers the "barrier of entry" to using this feature,
> > potentially spilling into drivers or other common files?
> 
> Well, yes, that's my main concern. I'm not against boards using BSS early.
> In fact, I started an attempt for socfpga_a10 to use BSS early some months
> ago. However, I am concerned that this gets the default for some boards and
> people just don't notice using BSS. Given the lack of discussion on this
> patch before it was merged, I can easly imagine patches making use of BSS
> slipping in.


Understood.
 
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
> >
> > > If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
> > > and changing the README.
> > >
> > > >
> > > > [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
> > >
> > > Right, that one looks strange. It seems to me that's some kind of leftover
> > > code from pre-DM? I would have expected the UCLASS for mmc drivers
> > > to include this 'initialized' flag instead of this ugly "static in
> > > function" thing.
> >
> > I did a quick search, some other users of BSS that could potentially have an
> > impact in the context of "early FW loading from SPL board_init_f()" include...
> >
> > 'static int fat_registered' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19
> >
> > 'static struct mmc *mmc' in ...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
> > (actually I'm responsible for the 'static' there)
> >
> > 'static int usb_stor_curr_dev' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18
> >
> > Then, there are also quite a few instances in drivers/, many of them not
> > relevant to operating from SPL's board_init_f() context, with some of
> > them however possibly being affected like these:
> >
> > 'static struct device_node *of_aliases' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35
> >
> > 'static int reloc_done' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90
> >
> > 'static bool sf_mtd_registered' in....
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13
> >
> > or
> >
> > 'static ulong next_reset' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76
> 
> Great to have that list! What I'm still missing is a more detailed reason
> *why* you need BSS early If it's because of one of the invalid usages above,
> it might be better to fix them. If it's because of memory shortage: why
> wouldn't the heap be enough to solve that problem? If it's because of heap
> shortage when using simple malloc (I had that on socfpga_a10): maybe it can
> be fixed in a different way?

Ok back to my specific scenario, hopefully I'm adding at least some new
aspects now rather than repeating what was discussed already in different
threads...

From SPL I'm required to load (and start) our "System Firmware" which is
a prerequisite for bringing up DDR. We know that DDR bringup itself
should happen in SPL's board_init_f(), hence the need for loading stuff
from board_init_f() when no DDR is yet available (only on-chip memory).

I'm using the same loader framework to do the loading from
board_init_f() that SPL later uses from a board_init_r() context to
load U-Boot proper, ATF, and other files depending on the platform.

Now let's focus on two static variables that play a role in this
context, 'fat_registered' spl_fat.c and '*mmc' in spl_mmc.c. Those
static variables are essentially used to remember the initialization
state of the FAT driver and the MMC loader, so that it doesn't get
re-initialized the second time those get called (during SPL's main usage
of loading U-Boot, etc.). So essentially the desire is to carry this
initialization state from SPL's board_init_f() to board_init_r().

You suggested simple malloc. We have that available and use it (DM uses
it), but how could this play in here? Sure I can reserve some memory
from board_init_f(), or the drivers under discussion, and store the
initialization state there, but now I'd have the need to carry the
pointer to that initialization data forward somehow. spl_fat.c is not a
DM driver, it inherently doesn't have anything I can "tack on" additional
data fields. I don't quite see how I can make this work more elegantly
but I'm open to suggestions...

(Mr. Glass had suggested in one of the threads why I don't do the
DDR initialization in board_init_r() then, which I experimented with,
but the changes I had to make to common U-Boot files were rather drastic
so I abandoned this attempt).


> > Now, I have not validated each and every one of those (beyond 'fat_registered'
> > which I know is problematic), and there are more, for having an impact or not.
> > Whether all of those need any fixes or improvements set aside for a moment, at
> > a minimum doesn't it make you concerned about stability of code execution
> > without initialized BSS, no?
> 
> Of course it does! I know there are files using BSS when they shouldn't;
> socfpga_gen5 SPL was one of them. The problem is that either people don't know
> they shouldn't be using BSS or they don't care (because it just works).
> 
> What I'm afraid of with this new config option is that using BSS early might
> become more or less the standard (as there *are* many boards having it
> available from the start), which might easily result in boards that cannot use
> BSS ealry being broken.
> 
> Sadly, I failed to come up with a way of how to detect such invalid usage of
> BSS at compile time or runtime.

I used the JTAG debugger to look at the BSS region during SPL execution
to see which piece of code would touch it when.... But yeah that is not
something that is automatic and easily scalable across platforms...

--
Andreas Dannenberg
Texas Instruments Inc

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

Am 08.08.2019 um 20:29 schrieb Andreas Dannenberg:
> Simon,
> 
> On Thu, Aug 08, 2019 at 09:29:03AM +0200, Simon Goldschmidt wrote:
>> Hi Andreas,
>>
>> On Wed, Aug 7, 2019 at 11:24 PM Andreas Dannenberg <dannenberg@ti.com> wrote:
>>>
>>> Hi Simon,
>>> thanks for your patience waiting for a response. Please see comments inlined...
>>>
>>> On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
>>>> On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
>>>>>> Hi Lokesh,
>>>>>>
>>>>>> thanks for following up on this.
>>>>>>
>>>>>> On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>>>>>
>>>>>>> Hi Tom,
>>>>>>>
>>>>>>> On 20/07/19 9:21 PM, Tom Rini wrote:
>>>>>>>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
>>>>>>>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
>>>>>>>>>>
>>>>>>>>>>> In order to be able to use more advanced driver functionality which often
>>>>>>>>>>> relies on having BSS initialized during early boot prior to relocation
>>>>>>>>>>> several things need to be in place:
>>>>>>>>>>>
>>>>>>>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
>>>>>>>>>>>     at the top of the MCU SRAM area, with the stack starting right below
>>>>>>>>>>>     it,
>>>>>>>>>>> 2) We need to move the initialization of BSS prior to entering
>>>>>>>>>>>     board_init_f(). We will do this with a separate commit by turning on
>>>>>>>>>>>     the respective CONFIG option.
>>>>>>>>>>>
>>>>>>>>>>> In this commit we also clean up the assignment of the initial SP address
>>>>>>>>>>> as part of the refactoring, taking into account the pre-decrement post-
>>>>>>>>>>> increment nature in which the SP is used on ARM.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>>>>
>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>
>>>>>>>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
>>>>>>>>> but in a discussion about a similar patch I sent [1], using BSS from
>>>>>>>>> board_init_f
>>>>>>>>> was turned down. And Simon Glass rather convinced me that this is the current
>>>>>>>>> API U-Boot has (and is documented in README).
>>>>>>>>>
>>>>>>>>> So either we must change this API and its documentation (and I would expect the
>>>>>>>>> author of this patch to combine the README change with the code change), or this
>>>>>>>>> patch would have to be rejected.
>>>>>>>>>
>>>>>>>>> Again, I'm sorry I only see this now. In thought to remember a
>>>>>>>>> discussion in this
>>>>>>>>> thread, but I clearly remember that wrong...
>>>>>>>>>
>>>>>>>>> [1] https://patchwork.ozlabs.org/patch/1057237/
>>>>>>>>
>>>>>>>> And I had missed that other thread.  Lokesh, since I think Andreas is
>>>>>>>> out currently can you expand a little on what we can/can't do on this
>>>>>>>> platform?  Thanks!
>>>>>>>
>>>>>>> The reason why BSS is needed very early in this platform is for the following
>>>>>>> reasons:
>>>>>>> - System co-processor is the central resource manager in SoC and should be
>>>>>>> loaded and started very early in the boot process. Without that no peripheral or
>>>>>>> memory can be initialized. So for loading system co-processor image, we only
>>>>>>> have limited SRAM and a peripheral initialized by ROM.
>>>>>>> - System co-processor(DMSC) is being represented as remote-core in
>>>>>>> Device-tree(We are strictly following DM and DT model for the entire SoC).
>>>>>>> - Since DM is also followed by each peripheral device and remote core, DM should
>>>>>>> be enabled very early and many peripheral drivers are dependent on BSS usage.
>>>>>>> So, BSS has been made available very early.
>>>>>>>
>>>>>>> Hope this is clear. Let me know if more details are required, I will be happy to
>>>>>>> explain.
>>>>>>
>>>>>> Don't get me wrong: I'm not against using BSS early. I just want to ensure this
>>>>>> stays consistent throught U-Boot.
>>>>>
>>>>> I understand and agree that it should be consistent. Just discussed this with
>>>>> Andreas, who is courteous enough to update the details in his vacation.
>>>>
>>>> We don't have to rush here, I don't have a problem waiting for Andreas to
>>>> answer when he's back.
>>>>
>>>>>
>>>>>>
>>>>>> The reasons you stated still don't make it clear to me *why* bss is needed
>>>>>> early. There are other boards using DM early that don't need this. In my
>>>>>> opinion, DM drivers normally don't rely on BSS but keep all their state in
>>>>>
>>>>> This statement doesn't hold true for all the drviers. At least the mmc driver
>>>>> uses "initialized" variable stored in BSS to avoid initializing mmc multiple
>>>>> times[0]. In the past we en counted other drivers using it. I guess the idea
>>>>> here is to enable the BSS support generically instead of fixing each of every
>>>>> driver.
>>>>
>>>> So this driver is generally not usable in pre-relocation phase? The README
>>>> document is pretty clear about BSS not being available in board_init_f. I know
>>>> this text is old, but it seems still valid.
>>>>
>>>> And if this is really a workaround because it's easier to use this workaround
>>>> instead of fixing drivers that invalidly use BSS, is this what we want?
>>>>
>>>>>
>>>>>> heap memory. If you only need BSS early because drivers rely on BSS, you might
>>>>>> have to fix those drivers?
>>>>>
>>>>> So, correct me here, why should driver be restricted to not use BSS?
>>>>
>>>> Post-relocation drivers might be free to use BSS (although you lose the
>>>> per-instance storage when using BSS instead of the driver's priv data),
>>>> but pre-relocation drivers are not.
>>>> That's the current definition in U-Boot. This patch changes it by
>>>> adding the option
>>>> to use BSS early. This bears the danger of code being changed in a way that
>>>> it requires BSS to be available early and might not work on other boards that
>>>> actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
>>>>
>>>>>
>>>>> Also doing a grep for bss usage very early in board_init_f produced many results:
>>>>> ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
>>>>> arch/arm/mach-socfpga/spl_gen5.c
>>>>
>>>> Right, that's my responsibility, and there's a patch in Marek's queue
>>>> to fix this:
>>>> the DDR driver used BSS and I simply moved it's BSS variables to its driver.
>>>> Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
>>>>
>>>>> arch/arm/mach-zynqmp/spl.c
>>>>> arch/mips/mach-jz47xx/jz4780/jz4780.c
>>>>> board/barco/platinum/spl_picon.c
>>>>> board/barco/platinum/spl_titanium.c
>>>>> board/compulab/cl-som-imx7/spl.c
>>>>> board/congatec/cgtqmx6eval/cgtqmx6eval.c
>>>>> board/dhelectronics/dh_imx6/dh_imx6_spl.c
>>>>> board/el/el6x/el6x.c
>>>>> board/freescale/imx8mq_evk/spl.c
>>>>> board/freescale/imx8qm_mek/spl.c
>>>>> board/freescale/imx8qxp_mek/spl.c
>>>>> board/freescale/ls1021aiot/ls1021aiot.c
>>>>> board/freescale/ls1021aqds/ls1021aqds.c
>>>>> board/freescale/ls1021atwr/ls1021atwr.c
>>>>> board/freescale/mx6sabreauto/mx6sabreauto.c
>>>>> board/freescale/mx6sabresd/mx6sabresd.c
>>>>> board/freescale/mx6slevk/mx6slevk.c
>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c
>>>>> board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
>>>>> board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
>>>>> board/liebherr/display5/spl.c
>>>>> board/logicpd/imx6/imx6logic.c
>>>>> board/phytec/pcm058/pcm058.c
>>>>> board/phytec/pfla02/pfla02.c
>>>>> board/sks-kinkel/sksimx6/sksimx6.c
>>>>> board/solidrun/mx6cuboxi/mx6cuboxi.c
>>>>> board/technexion/pico-imx6ul/spl.c
>>>>> board/technexion/pico-imx7d/spl.c
>>>>> board/toradex/apalis_imx6/apalis_imx6.c
>>>>> board/toradex/colibri_imx6/colibri_imx6.c
>>>>> board/udoo/neo/neo.c
>>>>> board/variscite/dart_6ul/spl.c
>>>>> board/woodburn/woodburn.c
>>>>>
>>>>> There might be some false positive cases but most of the above files are
>>>>> utilizing bss in board_init_f.
>>>>
>>>> So all these boards include a hack that's against what's the currently
>>>> documented status.
>>>
>>> Yes implementation and doc need to stay consistent.
>>>
>>>>>
>>>>>>
>>>>>> Further, allowing BSS early is still against what the main README says, so I
>>>>>> want to raise the question again: shouldn't this main README be changed if we
>>>>>> suddenly allow BSS to be used early (because that main README says we can'that
>>>>>> do that)?
>>>>>
>>>>> I do agree on this part. We should fix README in this case.
>>>
>>> I can prepare a PATCH to propose an update to the README, it definitely
>>> should stay in sync with the implementation, independent of the path we
>>> are choosing to potentially make any improvements moving forward.
>>
>> That would be very welcome!
> 
> Proposal submitted.
>   
>>>> My point (Simon Glass has convinced me in the previous discussion I
>>>> mentioned) is that there *are* boards that can't use BSS early. You can't just
>>>> allow all code to use BSS early as you risk breaking such boards.
>>>
>>> That's why the early BSS option has to be turned on explicitly. Nobody
>>> requires you to use early BSS. It should be considered an option for
>>> certain limited use cases (and described as such in an update to README,
>>> like there are many other very "special" options in U-Boot that have no
>>> wide use). But I guess one of your concerns as you alluded to earlier
>>> is that it may result in incompatibilities moving forward as this
>>> essentially lowers the "barrier of entry" to using this feature,
>>> potentially spilling into drivers or other common files?
>>
>> Well, yes, that's my main concern. I'm not against boards using BSS early.
>> In fact, I started an attempt for socfpga_a10 to use BSS early some months
>> ago. However, I am concerned that this gets the default for some boards and
>> people just don't notice using BSS. Given the lack of discussion on this
>> patch before it was merged, I can easly imagine patches making use of BSS
>> slipping in.
> 
> 
> Understood.
>   
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
>>>
>>>> If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
>>>> and changing the README.
>>>>
>>>>>
>>>>> [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
>>>>
>>>> Right, that one looks strange. It seems to me that's some kind of leftover
>>>> code from pre-DM? I would have expected the UCLASS for mmc drivers
>>>> to include this 'initialized' flag instead of this ugly "static in
>>>> function" thing.
>>>
>>> I did a quick search, some other users of BSS that could potentially have an
>>> impact in the context of "early FW loading from SPL board_init_f()" include...
>>>
>>> 'static int fat_registered' in...
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19
>>>
>>> 'static struct mmc *mmc' in ...
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
>>> (actually I'm responsible for the 'static' there)
>>>
>>> 'static int usb_stor_curr_dev' in...
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18
>>>
>>> Then, there are also quite a few instances in drivers/, many of them not
>>> relevant to operating from SPL's board_init_f() context, with some of
>>> them however possibly being affected like these:
>>>
>>> 'static struct device_node *of_aliases' in...
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35
>>>
>>> 'static int reloc_done' in...
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90
>>>
>>> 'static bool sf_mtd_registered' in....
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13
>>>
>>> or
>>>
>>> 'static ulong next_reset' in...
>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76
>>
>> Great to have that list! What I'm still missing is a more detailed reason
>> *why* you need BSS early If it's because of one of the invalid usages above,
>> it might be better to fix them. If it's because of memory shortage: why
>> wouldn't the heap be enough to solve that problem? If it's because of heap
>> shortage when using simple malloc (I had that on socfpga_a10): maybe it can
>> be fixed in a different way?
> 
> Ok back to my specific scenario, hopefully I'm adding at least some new
> aspects now rather than repeating what was discussed already in different
> threads...
> 
>  From SPL I'm required to load (and start) our "System Firmware" which is
> a prerequisite for bringing up DDR. We know that DDR bringup itself
> should happen in SPL's board_init_f(), hence the need for loading stuff
> from board_init_f() when no DDR is yet available (only on-chip memory).
> 
> I'm using the same loader framework to do the loading from
> board_init_f() that SPL later uses from a board_init_r() context to
> load U-Boot proper, ATF, and other files depending on the platform. >
> Now let's focus on two static variables that play a role in this
> context, 'fat_registered' spl_fat.c and '*mmc' in spl_mmc.c. Those
> static variables are essentially used to remember the initialization
> state of the FAT driver and the MMC loader, so that it doesn't get
> re-initialized the second time those get called (during SPL's main usage
> of loading U-Boot, etc.). So essentially the desire is to carry this
> initialization state from SPL's board_init_f() to board_init_r().

OK, so essentially, you've added CONFIG_SPL_EARLY_BSS because FAT and 
MMC contain variables in BSS? That would mean we could drop that config 
option after fixing those two (given that they can be fixed)?

> 
> You suggested simple malloc. We have that available and use it (DM uses
> it), but how could this play in here? Sure I can reserve some memory
> from board_init_f(), or the drivers under discussion, and store the
> initialization state there, but now I'd have the need to carry the
> pointer to that initialization data forward somehow. spl_fat.c is not a
> DM driver, it inherently doesn't have anything I can "tack on" additional
> data fields. I don't quite see how I can make this work more elegantly
> but I'm open to suggestions...

No, sorry, what I wrote was probably a bit confusing. I wrote that as a 
result of my work on socfpga_a10. There, we have the firmware loader 
framework loading things e.g. from MMC. During my test of unifying the 
socfpga config header files (both gen5 and a10 combined), I stumbled 
accross the fact that you cannot use standard malloc in SPL when the 
devicetree is initialized during board_init_f as dlmalloc.c makes heavy 
use of BSS. You can only use simple malloc there, because its state is 
kept in 'gd'.

But it seems that wasn't your problem?

A next problem with simple malloc is that you can't free anything and I 
think I remember code passages around file system loading that make 
heavy use of malloc/free. But that again doesn't seem to be your problem 
here?

> 
> (Mr. Glass had suggested in one of the threads why I don't do the
> DDR initialization in board_init_r() then, which I experimented with,
> but the changes I had to make to common U-Boot files were rather drastic
> so I abandoned this attempt).

Yes, I can understand that that's not an ideal way to move forward...

To come back on the original issue, I'd still propose to add these 
static variables to 'gd' or to some sub-struct referenced from 'gd'. I 
see a high risk for others to run into these issues that you have hidden 
for your platform by enabling CONFIG_SPL_EARLY_BSS.

> 
> 
>>> Now, I have not validated each and every one of those (beyond 'fat_registered'
>>> which I know is problematic), and there are more, for having an impact or not.
>>> Whether all of those need any fixes or improvements set aside for a moment, at
>>> a minimum doesn't it make you concerned about stability of code execution
>>> without initialized BSS, no?
>>
>> Of course it does! I know there are files using BSS when they shouldn't;
>> socfpga_gen5 SPL was one of them. The problem is that either people don't know
>> they shouldn't be using BSS or they don't care (because it just works).
>>
>> What I'm afraid of with this new config option is that using BSS early might
>> become more or less the standard (as there *are* many boards having it
>> available from the start), which might easily result in boards that cannot use
>> BSS ealry being broken.
>>
>> Sadly, I failed to come up with a way of how to detect such invalid usage of
>> BSS at compile time or runtime.
> 
> I used the JTAG debugger to look at the BSS region during SPL execution
> to see which piece of code would touch it when.... But yeah that is not
> something that is automatic and easily scalable across platforms...

No, that's not what I meant :-) I do have access to a JTAG debugger for 
my platform, too, but I don't want to run this manual step with every 
update to check BSS...

Regards,
Simon

> 
> --
> Andreas Dannenberg
> Texas Instruments Inc
> 

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Andreas Dannenberg]

Hi Simon,

On Thu, Aug 08, 2019 at 09:01:03PM +0200, Simon Goldschmidt wrote:
> Am 08.08.2019 um 20:29 schrieb Andreas Dannenberg:

<snip>

> > Ok back to my specific scenario, hopefully I'm adding at least some new
> > aspects now rather than repeating what was discussed already in different
> > threads...
> > 
> >  From SPL I'm required to load (and start) our "System Firmware" which is
> > a prerequisite for bringing up DDR. We know that DDR bringup itself
> > should happen in SPL's board_init_f(), hence the need for loading stuff
> > from board_init_f() when no DDR is yet available (only on-chip memory).
> > 
> > I'm using the same loader framework to do the loading from
> > board_init_f() that SPL later uses from a board_init_r() context to
> > load U-Boot proper, ATF, and other files depending on the platform. >
> > Now let's focus on two static variables that play a role in this
> > context, 'fat_registered' spl_fat.c and '*mmc' in spl_mmc.c. Those
> > static variables are essentially used to remember the initialization
> > state of the FAT driver and the MMC loader, so that it doesn't get
> > re-initialized the second time those get called (during SPL's main usage
> > of loading U-Boot, etc.). So essentially the desire is to carry this
> > initialization state from SPL's board_init_f() to board_init_r().
> 
> OK, so essentially, you've added CONFIG_SPL_EARLY_BSS because FAT and MMC
> contain variables in BSS? That would mean we could drop that config option
> after fixing those two (given that they can be fixed)?

Yes I think I could drop it in this case, I'm not married to
CONFIG_SPL_EARLY_BSS in any way. But we still have all the other
platforms that use memset() to directly clear BSS from SPL's
board_init_f()...  Ideally any improvements should be made across
the board.

(And I just need to throw that out there) one could also imagine moving
said platforms to also use CONFIG_SPL_EARLY_BSS (replacing their custom
memset approach) to at least unify the approach...

> > it), but how could this play in here? Sure I can reserve some memory
> > from board_init_f(), or the drivers under discussion, and store the
> > initialization state there, but now I'd have the need to carry the
> > pointer to that initialization data forward somehow. spl_fat.c is not a
> > DM driver, it inherently doesn't have anything I can "tack on" additional
> > data fields. I don't quite see how I can make this work more elegantly
> > but I'm open to suggestions...
> 
> No, sorry, what I wrote was probably a bit confusing. I wrote that as a
> result of my work on socfpga_a10. There, we have the firmware loader
> framework loading things e.g. from MMC. During my test of unifying the
> socfpga config header files (both gen5 and a10 combined), I stumbled accross
> the fact that you cannot use standard malloc in SPL when the devicetree is
> initialized during board_init_f as dlmalloc.c makes heavy use of BSS. You
> can only use simple malloc there, because its state is kept in 'gd'.
> 
> But it seems that wasn't your problem?
> 
> A next problem with simple malloc is that you can't free anything and I
> think I remember code passages around file system loading that make heavy
> use of malloc/free. But that again doesn't seem to be your problem here?

I had this exact problem initially, as I was loading two files from
board_init_f (the "system firmware" and it's config data), which I since
consolidated into a FIT image avoiding the issue altogether. The FAT
driver specifically is very wasteful allocating 64KB chunks of memory
(you can reduce the sector size to alleviate some of that) and with
simple malloc it would eat through the little RAM I had too quickly...
So for a while I was using full malloc from board_init_f() to make the
FAT driver happy. And I had also found I need BSS for that after a
little pain and suffering with the JTAG debugger. But this is no
longer n issue when only loading one file.

> > (Mr. Glass had suggested in one of the threads why I don't do the
> > DDR initialization in board_init_r() then, which I experimented with,
> > but the changes I had to make to common U-Boot files were rather drastic
> > so I abandoned this attempt).
> 
> Yes, I can understand that that's not an ideal way to move forward...
> 
> To come back on the original issue, I'd still propose to add these static
> variables to 'gd' or to some sub-struct referenced from 'gd'. I see a high
> risk for others to run into these issues that you have hidden for your
> platform by enabling CONFIG_SPL_EARLY_BSS.

I was always hesitant even thinking about adding stuff to gd due to the
large impact so I haven't really considered this as a feasible path.
But if we were to go down this road possibly we could add a bitfield
variable of some type (to be considerate of memory use) containing a
collection of "initialized" flags used by different drivers that really
need it?

--
Andreas Dannenberg
Texas Instruments Inc

<snip>

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

Am 08.08.2019 um 21:43 schrieb Andreas Dannenberg:
> Hi Simon,
> 
> On Thu, Aug 08, 2019 at 09:01:03PM +0200, Simon Goldschmidt wrote:
>> Am 08.08.2019 um 20:29 schrieb Andreas Dannenberg:
> 
> <snip>
> 
>>> Ok back to my specific scenario, hopefully I'm adding at least some new
>>> aspects now rather than repeating what was discussed already in different
>>> threads...
>>>
>>>   From SPL I'm required to load (and start) our "System Firmware" which is
>>> a prerequisite for bringing up DDR. We know that DDR bringup itself
>>> should happen in SPL's board_init_f(), hence the need for loading stuff
>>> from board_init_f() when no DDR is yet available (only on-chip memory).
>>>
>>> I'm using the same loader framework to do the loading from
>>> board_init_f() that SPL later uses from a board_init_r() context to
>>> load U-Boot proper, ATF, and other files depending on the platform. >
>>> Now let's focus on two static variables that play a role in this
>>> context, 'fat_registered' spl_fat.c and '*mmc' in spl_mmc.c. Those
>>> static variables are essentially used to remember the initialization
>>> state of the FAT driver and the MMC loader, so that it doesn't get
>>> re-initialized the second time those get called (during SPL's main usage
>>> of loading U-Boot, etc.). So essentially the desire is to carry this
>>> initialization state from SPL's board_init_f() to board_init_r().
>>
>> OK, so essentially, you've added CONFIG_SPL_EARLY_BSS because FAT and MMC
>> contain variables in BSS? That would mean we could drop that config option
>> after fixing those two (given that they can be fixed)?
> 
> Yes I think I could drop it in this case, I'm not married to
> CONFIG_SPL_EARLY_BSS in any way. But we still have all the other
> platforms that use memset() to directly clear BSS from SPL's
> board_init_f()...  Ideally any improvements should be made across
> the board.
> 
> (And I just need to throw that out there) one could also imagine moving
> said platforms to also use CONFIG_SPL_EARLY_BSS (replacing their custom
> memset approach) to at least unify the approach...

That could well be a valid approach. However, ideally, we'd first check 
why these boards really need that memset. I fixed socfgpa_gen5 by fixing 
the sdram driver to not use BSS... (a rather simple fix that even ended 
up reducing code size by keeping the data on the stack).

> 
>>> it), but how could this play in here? Sure I can reserve some memory
>>> from board_init_f(), or the drivers under discussion, and store the
>>> initialization state there, but now I'd have the need to carry the
>>> pointer to that initialization data forward somehow. spl_fat.c is not a
>>> DM driver, it inherently doesn't have anything I can "tack on" additional
>>> data fields. I don't quite see how I can make this work more elegantly
>>> but I'm open to suggestions...
>>
>> No, sorry, what I wrote was probably a bit confusing. I wrote that as a
>> result of my work on socfpga_a10. There, we have the firmware loader
>> framework loading things e.g. from MMC. During my test of unifying the
>> socfpga config header files (both gen5 and a10 combined), I stumbled accross
>> the fact that you cannot use standard malloc in SPL when the devicetree is
>> initialized during board_init_f as dlmalloc.c makes heavy use of BSS. You
>> can only use simple malloc there, because its state is kept in 'gd'.
>>
>> But it seems that wasn't your problem?
>>
>> A next problem with simple malloc is that you can't free anything and I
>> think I remember code passages around file system loading that make heavy
>> use of malloc/free. But that again doesn't seem to be your problem here?
> 
> I had this exact problem initially, as I was loading two files from
> board_init_f (the "system firmware" and it's config data), which I since
> consolidated into a FIT image avoiding the issue altogether. The FAT
> driver specifically is very wasteful allocating 64KB chunks of memory
> (you can reduce the sector size to alleviate some of that) and with
> simple malloc it would eat through the little RAM I had too quickly...
> So for a while I was using full malloc from board_init_f() to make the
> FAT driver happy. And I had also found I need BSS for that after a
> little pain and suffering with the JTAG debugger. But this is no
> longer n issue when only loading one file.

Yeah, well, that issue remains even if we'd fix the already mentioned 
problems...

> 
>>> (Mr. Glass had suggested in one of the threads why I don't do the
>>> DDR initialization in board_init_r() then, which I experimented with,
>>> but the changes I had to make to common U-Boot files were rather drastic
>>> so I abandoned this attempt).
>>
>> Yes, I can understand that that's not an ideal way to move forward...
>>
>> To come back on the original issue, I'd still propose to add these static
>> variables to 'gd' or to some sub-struct referenced from 'gd'. I see a high
>> risk for others to run into these issues that you have hidden for your
>> platform by enabling CONFIG_SPL_EARLY_BSS.
> 
> I was always hesitant even thinking about adding stuff to gd due to the
> large impact so I haven't really considered this as a feasible path.
> But if we were to go down this road possibly we could add a bitfield
> variable of some type (to be considerate of memory use) containing a
> collection of "initialized" flags used by different drivers that really
> need it?

That might have a smaller impact on U-Boot/SPL code quality than 
allowing early bss...

However, making that change might only make sense of other boards don't 
end up with still needing early bss.

In the end, I'm not married with "no-early-BSS", too: I always have a 
hard time imagining why boards cannot provide early BSS but have RAM for 
'gd'. The only reason can be that BSS is too big for whatever little 
amount we have before setting up DDR. But if you have a board that uses 
DM before DDR is up, you'll end up with a fair amount of RAM (heap) 
usage anyway, so why is BSS bad here but heap (that is never freed) is good?

Moving global variables that are used early into a different section 
(".bss_early") might be a possible solution, too. That might solve 
things where an 'init' flag isn't enough. However, your suggestion of an 
'initialized' flag array in 'gd' is probably smaller...

Regards,
Simon

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Simon Goldschmidt]

Am 07.08.2019 um 23:23 schrieb Andreas Dannenberg:
> Hi Simon,
> thanks for your patience waiting for a response. Please see comments inlined...
> 
> On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
>> On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
>>>> Hi Lokesh,
>>>>
>>>> thanks for following up on this.
>>>>
>>>> On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>>>>
>>>>> Hi Tom,
>>>>>
>>>>> On 20/07/19 9:21 PM, Tom Rini wrote:
>>>>>> On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
>>>>>>> On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
>>>>>>>>
>>>>>>>>> In order to be able to use more advanced driver functionality which often
>>>>>>>>> relies on having BSS initialized during early boot prior to relocation
>>>>>>>>> several things need to be in place:
>>>>>>>>>
>>>>>>>>> 1) Memory needs to be available for BSS to use. For this, we locate BSS
>>>>>>>>>     at the top of the MCU SRAM area, with the stack starting right below
>>>>>>>>>     it,
>>>>>>>>> 2) We need to move the initialization of BSS prior to entering
>>>>>>>>>     board_init_f(). We will do this with a separate commit by turning on
>>>>>>>>>     the respective CONFIG option.
>>>>>>>>>
>>>>>>>>> In this commit we also clean up the assignment of the initial SP address
>>>>>>>>> as part of the refactoring, taking into account the pre-decrement post-
>>>>>>>>> increment nature in which the SP is used on ARM.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>>
>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>
>>>>>>> Wait, why has this been merged? Unfortunately, I haven't followed this series,
>>>>>>> but in a discussion about a similar patch I sent [1], using BSS from
>>>>>>> board_init_f
>>>>>>> was turned down. And Simon Glass rather convinced me that this is the current
>>>>>>> API U-Boot has (and is documented in README).
>>>>>>>
>>>>>>> So either we must change this API and its documentation (and I would expect the
>>>>>>> author of this patch to combine the README change with the code change), or this
>>>>>>> patch would have to be rejected.
>>>>>>>
>>>>>>> Again, I'm sorry I only see this now. In thought to remember a
>>>>>>> discussion in this
>>>>>>> thread, but I clearly remember that wrong...
>>>>>>>
>>>>>>> [1] https://patchwork.ozlabs.org/patch/1057237/
>>>>>>
>>>>>> And I had missed that other thread.  Lokesh, since I think Andreas is
>>>>>> out currently can you expand a little on what we can/can't do on this
>>>>>> platform?  Thanks!
>>>>>
>>>>> The reason why BSS is needed very early in this platform is for the following
>>>>> reasons:
>>>>> - System co-processor is the central resource manager in SoC and should be
>>>>> loaded and started very early in the boot process. Without that no peripheral or
>>>>> memory can be initialized. So for loading system co-processor image, we only
>>>>> have limited SRAM and a peripheral initialized by ROM.
>>>>> - System co-processor(DMSC) is being represented as remote-core in
>>>>> Device-tree(We are strictly following DM and DT model for the entire SoC).
>>>>> - Since DM is also followed by each peripheral device and remote core, DM should
>>>>> be enabled very early and many peripheral drivers are dependent on BSS usage.
>>>>> So, BSS has been made available very early.
>>>>>
>>>>> Hope this is clear. Let me know if more details are required, I will be happy to
>>>>> explain.
>>>>
>>>> Don't get me wrong: I'm not against using BSS early. I just want to ensure this
>>>> stays consistent throught U-Boot.
>>>
>>> I understand and agree that it should be consistent. Just discussed this with
>>> Andreas, who is courteous enough to update the details in his vacation.
>>
>> We don't have to rush here, I don't have a problem waiting for Andreas to
>> answer when he's back.
>>
>>>
>>>>
>>>> The reasons you stated still don't make it clear to me *why* bss is needed
>>>> early. There are other boards using DM early that don't need this. In my
>>>> opinion, DM drivers normally don't rely on BSS but keep all their state in
>>>
>>> This statement doesn't hold true for all the drviers. At least the mmc driver
>>> uses "initialized" variable stored in BSS to avoid initializing mmc multiple
>>> times[0]. In the past we en counted other drivers using it. I guess the idea
>>> here is to enable the BSS support generically instead of fixing each of every
>>> driver.
>>
>> So this driver is generally not usable in pre-relocation phase? The README
>> document is pretty clear about BSS not being available in board_init_f. I know
>> this text is old, but it seems still valid.
>>
>> And if this is really a workaround because it's easier to use this workaround
>> instead of fixing drivers that invalidly use BSS, is this what we want?
>>
>>>
>>>> heap memory. If you only need BSS early because drivers rely on BSS, you might
>>>> have to fix those drivers?
>>>
>>> So, correct me here, why should driver be restricted to not use BSS?
>>
>> Post-relocation drivers might be free to use BSS (although you lose the
>> per-instance storage when using BSS instead of the driver's priv data),
>> but pre-relocation drivers are not.
>> That's the current definition in U-Boot. This patch changes it by
>> adding the option
>> to use BSS early. This bears the danger of code being changed in a way that
>> it requires BSS to be available early and might not work on other boards that
>> actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
>>
>>>
>>> Also doing a grep for bss usage very early in board_init_f produced many results:
>>> ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
>>> arch/arm/mach-socfpga/spl_gen5.c
>>
>> Right, that's my responsibility, and there's a patch in Marek's queue
>> to fix this:
>> the DDR driver used BSS and I simply moved it's BSS variables to its driver.
>> Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
>>
>>> arch/arm/mach-zynqmp/spl.c
>>> arch/mips/mach-jz47xx/jz4780/jz4780.c
>>> board/barco/platinum/spl_picon.c
>>> board/barco/platinum/spl_titanium.c
>>> board/compulab/cl-som-imx7/spl.c
>>> board/congatec/cgtqmx6eval/cgtqmx6eval.c
>>> board/dhelectronics/dh_imx6/dh_imx6_spl.c
>>> board/el/el6x/el6x.c
>>> board/freescale/imx8mq_evk/spl.c
>>> board/freescale/imx8qm_mek/spl.c
>>> board/freescale/imx8qxp_mek/spl.c
>>> board/freescale/ls1021aiot/ls1021aiot.c
>>> board/freescale/ls1021aqds/ls1021aqds.c
>>> board/freescale/ls1021atwr/ls1021atwr.c
>>> board/freescale/mx6sabreauto/mx6sabreauto.c
>>> board/freescale/mx6sabresd/mx6sabresd.c
>>> board/freescale/mx6slevk/mx6slevk.c
>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c
>>> board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
>>> board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
>>> board/liebherr/display5/spl.c
>>> board/logicpd/imx6/imx6logic.c
>>> board/phytec/pcm058/pcm058.c
>>> board/phytec/pfla02/pfla02.c
>>> board/sks-kinkel/sksimx6/sksimx6.c
>>> board/solidrun/mx6cuboxi/mx6cuboxi.c
>>> board/technexion/pico-imx6ul/spl.c
>>> board/technexion/pico-imx7d/spl.c
>>> board/toradex/apalis_imx6/apalis_imx6.c
>>> board/toradex/colibri_imx6/colibri_imx6.c
>>> board/udoo/neo/neo.c
>>> board/variscite/dart_6ul/spl.c
>>> board/woodburn/woodburn.c
>>>
>>> There might be some false positive cases but most of the above files are
>>> utilizing bss in board_init_f.
>>
>> So all these boards include a hack that's against what's the currently
>> documented status.
> 
> Yes implementation and doc need to stay consistent.
> 
>>>
>>>>
>>>> Further, allowing BSS early is still against what the main README says, so I
>>>> want to raise the question again: shouldn't this main README be changed if we
>>>> suddenly allow BSS to be used early (because that main README says we can'that
>>>> do that)?
>>>
>>> I do agree on this part. We should fix README in this case.
> 
> I can prepare a PATCH to propose an update to the README, it definitely
> should stay in sync with the implementation, independent of the path we
> are choosing to potentially make any improvements moving forward.
>   
>> My point (Simon Glass has convinced me in the previous discussion I
>> mentioned) is that there *are* boards that can't use BSS early. You can't just
>> allow all code to use BSS early as you risk breaking such boards.
> 
> That's why the early BSS option has to be turned on explicitly. Nobody
> requires you to use early BSS. It should be considered an option for
> certain limited use cases (and described as such in an update to README,
> like there are many other very "special" options in U-Boot that have no
> wide use). But I guess one of your concerns as you alluded to earlier
> is that it may result in incompatibilities moving forward as this
> essentially lowers the "barrier of entry" to using this feature,
> potentially spilling into drivers or other common files?
> 
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
>   
>> If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
>> and changing the README.
>>
>>>
>>> [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
>>
>> Right, that one looks strange. It seems to me that's some kind of leftover
>> code from pre-DM? I would have expected the UCLASS for mmc drivers
>> to include this 'initialized' flag instead of this ugly "static in
>> function" thing.
> 
> I did a quick search, some other users of BSS that could potentially have an
> impact in the context of "early FW loading from SPL board_init_f()" include...
> 
> 'static int fat_registered' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19
> 
> 'static struct mmc *mmc' in ...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
> (actually I'm responsible for the 'static' there)
> 
> 'static int usb_stor_curr_dev' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18
> 
> Then, there are also quite a few instances in drivers/, many of them not
> relevant to operating from SPL's board_init_f() context, with some of
> them however possibly being affected like these:
> 
> 'static struct device_node *of_aliases' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35
> 
> 'static int reloc_done' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90
> 
> 'static bool sf_mtd_registered' in....
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13
> 
> or
> 
> 'static ulong next_reset' in...
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76
> 
> Now, I have not validated each and every one of those (beyond 'fat_registered'
> which I know is problematic), and there are more, for having an impact or not.
> Whether all of those need any fixes or improvements set aside for a moment, at
> a minimum doesn't it make you concerned about stability of code execution
> without initialized BSS, no?

Having searched more through that list, I'd say let's grab it and fix 
these. All of these have the potential to disturb more boards using that 
code in the future.

The very least we should do is delcare/access such BSS storage via some 
kind of guarding accessor/macro that checks if BSS is available (which 
by default would be checking gd->flags for GD_FLG_RELOC).

The boolean-type ints might probably become bits in gd->flags, but I'm 
not sure for the static pointer values...

Regards,
Simon

[U-Boot] [PATCH v2 05/12] armV7R: K3: am654: Allow using SPL BSS pre-relocation

[Andreas Dannenberg]

Hi Simon,

On Tue, Aug 13, 2019 at 10:38:22PM +0200, Simon Goldschmidt wrote:
> Am 07.08.2019 um 23:23 schrieb Andreas Dannenberg:
> > Hi Simon,
> > thanks for your patience waiting for a response. Please see comments inlined...
> > 
> > On Thu, Jul 25, 2019 at 11:52:55AM +0200, Simon Goldschmidt wrote:
> > > On Thu, Jul 25, 2019 at 10:23 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > > > 
> > > > Hi Simon,
> > > > 
> > > > On 25/07/19 12:31 PM, Simon Goldschmidt wrote:
> > > > > Hi Lokesh,
> > > > > 
> > > > > thanks for following up on this.
> > > > > 
> > > > > On Thu, Jul 25, 2019 at 6:36 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > > > > > 
> > > > > > Hi Tom,
> > > > > > 
> > > > > > On 20/07/19 9:21 PM, Tom Rini wrote:
> > > > > > > On Fri, Jul 19, 2019 at 07:29:37AM +0200, Simon Goldschmidt wrote:
> > > > > > > > On Fri, Jul 19, 2019 at 2:29 AM Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > 
> > > > > > > > > On Tue, Jun 04, 2019 at 05:55:48PM -0500, Andreas Dannenberg wrote:
> > > > > > > > > 
> > > > > > > > > > In order to be able to use more advanced driver functionality which often
> > > > > > > > > > relies on having BSS initialized during early boot prior to relocation
> > > > > > > > > > several things need to be in place:
> > > > > > > > > > 
> > > > > > > > > > 1) Memory needs to be available for BSS to use. For this, we locate BSS
> > > > > > > > > >     at the top of the MCU SRAM area, with the stack starting right below
> > > > > > > > > >     it,
> > > > > > > > > > 2) We need to move the initialization of BSS prior to entering
> > > > > > > > > >     board_init_f(). We will do this with a separate commit by turning on
> > > > > > > > > >     the respective CONFIG option.
> > > > > > > > > > 
> > > > > > > > > > In this commit we also clean up the assignment of the initial SP address
> > > > > > > > > > as part of the refactoring, taking into account the pre-decrement post-
> > > > > > > > > > increment nature in which the SP is used on ARM.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > > > > > > > > 
> > > > > > > > > Applied to u-boot/master, thanks!
> > > > > > > > 
> > > > > > > > Wait, why has this been merged? Unfortunately, I haven't followed this series,
> > > > > > > > but in a discussion about a similar patch I sent [1], using BSS from
> > > > > > > > board_init_f
> > > > > > > > was turned down. And Simon Glass rather convinced me that this is the current
> > > > > > > > API U-Boot has (and is documented in README).
> > > > > > > > 
> > > > > > > > So either we must change this API and its documentation (and I would expect the
> > > > > > > > author of this patch to combine the README change with the code change), or this
> > > > > > > > patch would have to be rejected.
> > > > > > > > 
> > > > > > > > Again, I'm sorry I only see this now. In thought to remember a
> > > > > > > > discussion in this
> > > > > > > > thread, but I clearly remember that wrong...
> > > > > > > > 
> > > > > > > > [1] https://patchwork.ozlabs.org/patch/1057237/
> > > > > > > 
> > > > > > > And I had missed that other thread.  Lokesh, since I think Andreas is
> > > > > > > out currently can you expand a little on what we can/can't do on this
> > > > > > > platform?  Thanks!
> > > > > > 
> > > > > > The reason why BSS is needed very early in this platform is for the following
> > > > > > reasons:
> > > > > > - System co-processor is the central resource manager in SoC and should be
> > > > > > loaded and started very early in the boot process. Without that no peripheral or
> > > > > > memory can be initialized. So for loading system co-processor image, we only
> > > > > > have limited SRAM and a peripheral initialized by ROM.
> > > > > > - System co-processor(DMSC) is being represented as remote-core in
> > > > > > Device-tree(We are strictly following DM and DT model for the entire SoC).
> > > > > > - Since DM is also followed by each peripheral device and remote core, DM should
> > > > > > be enabled very early and many peripheral drivers are dependent on BSS usage.
> > > > > > So, BSS has been made available very early.
> > > > > > 
> > > > > > Hope this is clear. Let me know if more details are required, I will be happy to
> > > > > > explain.
> > > > > 
> > > > > Don't get me wrong: I'm not against using BSS early. I just want to ensure this
> > > > > stays consistent throught U-Boot.
> > > > 
> > > > I understand and agree that it should be consistent. Just discussed this with
> > > > Andreas, who is courteous enough to update the details in his vacation.
> > > 
> > > We don't have to rush here, I don't have a problem waiting for Andreas to
> > > answer when he's back.
> > > 
> > > > 
> > > > > 
> > > > > The reasons you stated still don't make it clear to me *why* bss is needed
> > > > > early. There are other boards using DM early that don't need this. In my
> > > > > opinion, DM drivers normally don't rely on BSS but keep all their state in
> > > > 
> > > > This statement doesn't hold true for all the drviers. At least the mmc driver
> > > > uses "initialized" variable stored in BSS to avoid initializing mmc multiple
> > > > times[0]. In the past we en counted other drivers using it. I guess the idea
> > > > here is to enable the BSS support generically instead of fixing each of every
> > > > driver.
> > > 
> > > So this driver is generally not usable in pre-relocation phase? The README
> > > document is pretty clear about BSS not being available in board_init_f. I know
> > > this text is old, but it seems still valid.
> > > 
> > > And if this is really a workaround because it's easier to use this workaround
> > > instead of fixing drivers that invalidly use BSS, is this what we want?
> > > 
> > > > 
> > > > > heap memory. If you only need BSS early because drivers rely on BSS, you might
> > > > > have to fix those drivers?
> > > > 
> > > > So, correct me here, why should driver be restricted to not use BSS?
> > > 
> > > Post-relocation drivers might be free to use BSS (although you lose the
> > > per-instance storage when using BSS instead of the driver's priv data),
> > > but pre-relocation drivers are not.
> > > That's the current definition in U-Boot. This patch changes it by
> > > adding the option
> > > to use BSS early. This bears the danger of code being changed in a way that
> > > it requires BSS to be available early and might not work on other boards that
> > > actually cannot provide BSS early (e.g. before SDRAM is up or whatever).
> > > 
> > > > 
> > > > Also doing a grep for bss usage very early in board_init_f produced many results:
> > > > ➜  u-boot git:(master) git grep -in "memset(__bss_start" | cut -d :  -f 1
> > > > arch/arm/mach-socfpga/spl_gen5.c
> > > 
> > > Right, that's my responsibility, and there's a patch in Marek's queue
> > > to fix this:
> > > the DDR driver used BSS and I simply moved it's BSS variables to its driver.
> > > Fixing the DDR driver allows me to remove that ugly "memset(__bss_start" hack.
> > > 
> > > > arch/arm/mach-zynqmp/spl.c
> > > > arch/mips/mach-jz47xx/jz4780/jz4780.c
> > > > board/barco/platinum/spl_picon.c
> > > > board/barco/platinum/spl_titanium.c
> > > > board/compulab/cl-som-imx7/spl.c
> > > > board/congatec/cgtqmx6eval/cgtqmx6eval.c
> > > > board/dhelectronics/dh_imx6/dh_imx6_spl.c
> > > > board/el/el6x/el6x.c
> > > > board/freescale/imx8mq_evk/spl.c
> > > > board/freescale/imx8qm_mek/spl.c
> > > > board/freescale/imx8qxp_mek/spl.c
> > > > board/freescale/ls1021aiot/ls1021aiot.c
> > > > board/freescale/ls1021aqds/ls1021aqds.c
> > > > board/freescale/ls1021atwr/ls1021atwr.c
> > > > board/freescale/mx6sabreauto/mx6sabreauto.c
> > > > board/freescale/mx6sabresd/mx6sabresd.c
> > > > board/freescale/mx6slevk/mx6slevk.c
> > > > board/freescale/mx6sxsabresd/mx6sxsabresd.c
> > > > board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c
> > > > board/k+p/kp_imx6q_tpc/kp_imx6q_tpc_spl.c
> > > > board/liebherr/display5/spl.c
> > > > board/logicpd/imx6/imx6logic.c
> > > > board/phytec/pcm058/pcm058.c
> > > > board/phytec/pfla02/pfla02.c
> > > > board/sks-kinkel/sksimx6/sksimx6.c
> > > > board/solidrun/mx6cuboxi/mx6cuboxi.c
> > > > board/technexion/pico-imx6ul/spl.c
> > > > board/technexion/pico-imx7d/spl.c
> > > > board/toradex/apalis_imx6/apalis_imx6.c
> > > > board/toradex/colibri_imx6/colibri_imx6.c
> > > > board/udoo/neo/neo.c
> > > > board/variscite/dart_6ul/spl.c
> > > > board/woodburn/woodburn.c
> > > > 
> > > > There might be some false positive cases but most of the above files are
> > > > utilizing bss in board_init_f.
> > > 
> > > So all these boards include a hack that's against what's the currently
> > > documented status.
> > 
> > Yes implementation and doc need to stay consistent.
> > 
> > > > 
> > > > > 
> > > > > Further, allowing BSS early is still against what the main README says, so I
> > > > > want to raise the question again: shouldn't this main README be changed if we
> > > > > suddenly allow BSS to be used early (because that main README says we can'that
> > > > > do that)?
> > > > 
> > > > I do agree on this part. We should fix README in this case.
> > 
> > I can prepare a PATCH to propose an update to the README, it definitely
> > should stay in sync with the implementation, independent of the path we
> > are choosing to potentially make any improvements moving forward.
> > > My point (Simon Glass has convinced me in the previous discussion I
> > > mentioned) is that there *are* boards that can't use BSS early. You can't just
> > > allow all code to use BSS early as you risk breaking such boards.
> > 
> > That's why the early BSS option has to be turned on explicitly. Nobody
> > requires you to use early BSS. It should be considered an option for
> > certain limited use cases (and described as such in an update to README,
> > like there are many other very "special" options in U-Boot that have no
> > wide use). But I guess one of your concerns as you alluded to earlier
> > is that it may result in incompatibilities moving forward as this
> > essentially lowers the "barrier of entry" to using this feature,
> > potentially spilling into drivers or other common files?
> > 
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/Kconfig#L251
> > > If we can ensure this doesn't happen, I'm OK with adding/keeping this patch
> > > and changing the README.
> > > 
> > > > 
> > > > [0] https://gitlab.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c#L1832
> > > 
> > > Right, that one looks strange. It seems to me that's some kind of leftover
> > > code from pre-DM? I would have expected the UCLASS for mmc drivers
> > > to include this 'initialized' flag instead of this ugly "static in
> > > function" thing.
> > 
> > I did a quick search, some other users of BSS that could potentially have an
> > impact in the context of "early FW loading from SPL board_init_f()" include...
> > 
> > 'static int fat_registered' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_fat.c#L19
> > 
> > 'static struct mmc *mmc' in ...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_mmc.c#L312
> > (actually I'm responsible for the 'static' there)
> > 
> > 'static int usb_stor_curr_dev' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/common/spl/spl_usb.c#L18
> > 
> > Then, there are also quite a few instances in drivers/, many of them not
> > relevant to operating from SPL's board_init_f() context, with some of
> > them however possibly being affected like these:
> > 
> > 'static struct device_node *of_aliases' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/core/of_access.c#L35
> > 
> > 'static int reloc_done' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf-uclass.c#L90
> > 
> > 'static bool sf_mtd_registered' in....
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mtd/spi/sf_mtd.c#L13
> > 
> > or
> > 
> > 'static ulong next_reset' in...
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/watchdog/wdt-uclass.c#L76
> > 
> > Now, I have not validated each and every one of those (beyond 'fat_registered'
> > which I know is problematic), and there are more, for having an impact or not.
> > Whether all of those need any fixes or improvements set aside for a moment, at
> > a minimum doesn't it make you concerned about stability of code execution
> > without initialized BSS, no?
> 
> Having searched more through that list, I'd say let's grab it and fix these.
> All of these have the potential to disturb more boards using that code in
> the future.

I've seen JJ has some patches already (not posted to the official ML yet
but I suppose it will happen soon) that will address at least two of my
concerns, amongst other things of what I think is a good cleanup effort...

"spl: mmc: do not use a static variable to prevent multiple initialization"
"spl: fat: remove usage of the fat_registered flag"

...so posted to the U-Boot ML that will help us here. Let's see what his plans
are to post those and go from there (JJ?).

> The very least we should do is delcare/access such BSS storage via some kind
> of guarding accessor/macro that checks if BSS is available (which by default
> would be checking gd->flags for GD_FLG_RELOC).
> 
> The boolean-type ints might probably become bits in gd->flags, but I'm not
> sure for the static pointer values...

We should see that we can avoid those.

Also I have since "re-discovered" two more uses of static that I
actually introduced into TI's "System Firmware" loader that gets executed from
SPL's board_init_f()...

https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-k3/sysfw-loader.c#L23

I use those statics to customize the SPL image loader load address (via
hooking into spl_get_load_buffer) depending on whether I load my own
file (system firmware) into a custom memory buffer or the next stage of
U-Boot (to CONFIG_SYS_TEXT_BASE). I'll need to see how to potentially do
that differently without leaning on gd...

--
Andreas Dannenberg
Texas Instruments Inc



> 
> Regards,
> Simon