[PATCH 00/18] vbe: Series part D

[PATCH 00/18] vbe: Series part D

[Simon Glass]

This includes various patches towards implementing the VBE abrec
bootmeth in U-Boot.


Simon Glass (18):
  sandbox: Add missing header file
  bootstd: Add stub for bootdev_setup_for_sibling_blk()
  gzip: Correct function comment for gunzip()
  fdtdec: Support separate BSS for all XPL builds
  tiny-printf: Correct return values
  tpl: Support numbered aliases in device tree
  ram: Support driver model in TPL
  serial: Support debug UART in TPL
  armv8: Support not having separate BSS
  arm: cache: Drop a stale comment
  arm: Fix up a stale comment in sections.c
  mmc: Support driver model in TPL
  mmc: Add more debugging for SPL
  mmc: Log the error when init fails
  mmc: rockchip: Log some error returns
  mmc: rockchip: Allow clocks to be missing
  rockchip: mmc: Fix a missing colon
  rockchip: Provided SPL control over efuse presence

 arch/arm/cpu/armv8/u-boot-spl.lds   | 12 ++++++++++++
 arch/arm/lib/cache.c                |  2 --
 arch/arm/lib/sections.c             |  2 +-
 arch/sandbox/include/asm/sections.h |  1 +
 common/spl/spl_mmc.c                | 13 +++++++++++++
 drivers/core/Kconfig                |  8 ++++++++
 drivers/misc/Makefile               |  2 +-
 drivers/mmc/Kconfig                 | 12 ++++++++++++
 drivers/mmc/mmc.c                   |  2 +-
 drivers/mmc/rockchip_dw_mmc.c       | 10 ++++------
 drivers/mmc/rockchip_sdhci.c        | 11 +++++------
 drivers/ram/Kconfig                 |  9 +++++++++
 drivers/serial/Kconfig              |  7 +++++++
 include/bootdev.h                   |  8 ++++++++
 include/gzip.h                      |  6 ++++--
 lib/fdtdec.c                        |  2 +-
 lib/tiny-printf.c                   | 15 ++++++---------
 17 files changed, 93 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH 01/18] sandbox: Add missing header file

[Simon Glass]

This file uses __aligned so should include the header which defines
that.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/sandbox/include/asm/sections.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/sandbox/include/asm/sections.h b/arch/sandbox/include/asm/sections.h
index 88837bb35c8..5e1577419b0 100644
--- a/arch/sandbox/include/asm/sections.h
+++ b/arch/sandbox/include/asm/sections.h
@@ -10,6 +10,7 @@
 #define __SANDBOX_SECTIONS_H
 
 #include <asm-generic/sections.h>
+#include <linux/compiler_attributes.h>
 
 struct sandbox_cmdline_option;
 
-- 
2.34.1

[PATCH 02/18] bootstd: Add stub for bootdev_setup_for_sibling_blk()

[Simon Glass]

When bootstd is not enabled, bootdevs should not be set up. Add a
static inline function to see to this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/bootdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/bootdev.h b/include/bootdev.h
index 2cee8832d26..ad4af0d1310 100644
--- a/include/bootdev.h
+++ b/include/bootdev.h
@@ -395,6 +395,7 @@ int bootdev_next_prio(struct bootflow_iter *iter, struct udevice **devp);
  */
 int bootdev_setup_for_dev(struct udevice *parent, const char *drv_name);
 
+#if CONFIG_IS_ENABLED(BOOTSTD)
 /**
  * bootdev_setup_for_sibling_blk() - Bind a new bootdev device for a blk device
  *
@@ -409,6 +410,13 @@ int bootdev_setup_for_dev(struct udevice *parent, const char *drv_name);
  * Return: 0 if OK, -ve on error
  */
 int bootdev_setup_for_sibling_blk(struct udevice *blk, const char *drv_name);
+#else
+static int bootdev_setup_for_sibling_blk(struct udevice *blk,
+					 const char *drv_name)
+{
+	return 0;
+}
+#endif
 
 /**
  * bootdev_get_sibling_blk() - Locate the block device for a bootdev
-- 
2.34.1

[PATCH 03/18] gzip: Correct function comment for gunzip()

[Simon Glass]

This doesn't describe the length parameter correctly. Fix it and
zunzip() too.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/gzip.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/gzip.h b/include/gzip.h
index 5e0d0ec07fb..304002ffc42 100644
--- a/include/gzip.h
+++ b/include/gzip.h
@@ -28,7 +28,8 @@ int gzip_parse_header(const unsigned char *src, unsigned long len);
  * @dst: Destination for uncompressed data
  * @dstlen: Size of destination buffer
  * @src: Source data to decompress
- * @lenp: Returns length of uncompressed data
+ * @lenp: On entry, length of data at @src. On exit, number of bytes used from
+ * @src
  * Return: 0 if OK, -1 on error
  */
 int gunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp);
@@ -39,7 +40,8 @@ int gunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp);
  * @dst: Destination for uncompressed data
  * @dstlen: Size of destination buffer
  * @src: Source data to decompress
- * @lenp: On entry, length data at @src. On exit, number of bytes used from @src
+ * @lenp: On entry, length of data at @src. On exit, number of bytes used from
+ * @src
  * @stoponerr: 0 to continue when a decode error is found, 1 to stop
  * @offset: start offset within the src buffer
  * Return: 0 if OK, -1 on error
-- 
2.34.1

[PATCH 04/18] fdtdec: Support separate BSS for all XPL builds

[Simon Glass]

Adjust the condition so that separate BSS can be deselected for TPL and
VPL.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 5edc8dd2f9f..106bb406365 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1232,7 +1232,7 @@ static void *fdt_find_separate(void)
 
 #ifdef CONFIG_SPL_BUILD
 	/* FDT is at end of BSS unless it is in a different memory region */
-	if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS))
+	if (CONFIG_IS_ENABLED(SEPARATE_BSS))
 		fdt_blob = (ulong *)_image_binary_end;
 	else
 		fdt_blob = (ulong *)__bss_end;
-- 
2.34.1

[PATCH 05/18] tiny-printf: Correct return values

[Simon Glass]

The sprintf() etc. functions are supposed to return the length of the
string written, but do not. Fix this by checking the amount of buffer
space used.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/tiny-printf.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 9a70c6095b3..64dee779c4a 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -365,16 +365,15 @@ int sprintf(char *buf, const char *fmt, ...)
 {
 	struct printf_info info;
 	va_list va;
-	int ret;
 
 	va_start(va, fmt);
 	info.outstr = buf;
 	info.putc = putc_outstr;
-	ret = _vprintf(&info, fmt, va);
+	_vprintf(&info, fmt, va);
 	va_end(va);
 	*info.outstr = '\0';
 
-	return ret;
+	return info.outstr - buf;
 }
 
 #if CONFIG_IS_ENABLED(LOG)
@@ -382,14 +381,13 @@ int sprintf(char *buf, const char *fmt, ...)
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list va)
 {
 	struct printf_info info;
-	int ret;
 
 	info.outstr = buf;
 	info.putc = putc_outstr;
-	ret = _vprintf(&info, fmt, va);
+	_vprintf(&info, fmt, va);
 	*info.outstr = '\0';
 
-	return ret;
+	return info.outstr - buf;
 }
 #endif
 
@@ -398,16 +396,15 @@ int snprintf(char *buf, size_t size, const char *fmt, ...)
 {
 	struct printf_info info;
 	va_list va;
-	int ret;
 
 	va_start(va, fmt);
 	info.outstr = buf;
 	info.putc = putc_outstr;
-	ret = _vprintf(&info, fmt, va);
+	_vprintf(&info, fmt, va);
 	va_end(va);
 	*info.outstr = '\0';
 
-	return ret;
+	return info.outstr - buf;
 }
 
 void print_grouped_ull(unsigned long long int_val, int digits)
-- 
2.34.1

[PATCH 06/18] tpl: Support numbered aliases in device tree

[Simon Glass]

Add an option so that this feature can be enabled in TPL for boards
which need it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/core/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 1a7be4d9b4d..d811120aef5 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -151,6 +151,14 @@ config SPL_DM_SEQ_ALIAS
 	  numbered devices (e.g. serial0 = &serial0). This feature can be
 	  disabled if it is not required, to save code space in SPL.
 
+config TPL_DM_SEQ_ALIAS
+	bool "Support numbered aliases in device tree in TPL"
+	depends on TPL_DM
+	help
+	  Most boards will have a '/aliases' node containing the path to
+	  numbered devices (e.g. serial0 = &serial0). This feature can be
+	  disabled if it is not required, to save code space in SPL.
+
 config VPL_DM_SEQ_ALIAS
 	bool "Support numbered aliases in device tree in VPL"
 	depends on VPL_DM
-- 
2.34.1

[PATCH 07/18] ram: Support driver model in TPL

[Simon Glass]

Some boards want to use RAM in TPL so add an option for that.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/ram/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
index a64d2dff68d..f7e357f24da 100644
--- a/drivers/ram/Kconfig
+++ b/drivers/ram/Kconfig
@@ -26,6 +26,15 @@ config TPL_RAM
 	  TPL, enable this option. It might provide a cleaner interface to
 	  setting up RAM (e.g. SDRAM / DDR) within TPL.
 
+config VPL_RAM
+	bool "Enable RAM support in VPL"
+	depends on RAM && VPL
+	help
+	  The RAM subsystem adds a small amount of overhead to the image.
+	  If this is acceptable and you have a need to use RAM drivers in
+	  VPL, enable this option. It might provide a cleaner interface to
+	  setting up RAM (e.g. SDRAM / DDR) within VPL.
+
 config STM32_SDRAM
 	bool "Enable STM32 SDRAM support"
 	depends on RAM
-- 
2.34.1

Re: [PATCH 07/18] ram: Support driver model in TPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Tue, Aug 27, 2024 at 07:45:26PM -0600, Simon Glass wrote:

> Some boards want to use RAM in TPL so add an option for that.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/ram/Kconfig | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
> index a64d2dff68d..f7e357f24da 100644
> --- a/drivers/ram/Kconfig
> +++ b/drivers/ram/Kconfig
> @@ -26,6 +26,15 @@ config TPL_RAM
>  	  TPL, enable this option. It might provide a cleaner interface to
>  	  setting up RAM (e.g. SDRAM / DDR) within TPL.
>  
> +config VPL_RAM
> +	bool "Enable RAM support in VPL"
> +	depends on RAM && VPL

Subject and commit say TPL, but this is VPL.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 08/18] serial: Support debug UART in TPL

[Simon Glass]

Some boards want to use the debug UART in TPL so add an option for that.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/serial/Kconfig | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 3a1e5a6f287..8b27ad9a77e 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -539,6 +539,13 @@ config TPL_DEBUG_UART_BASE
 	help
 	  This is the base address of your UART for memory-mapped UARTs for TPL.
 
+config VPL_DEBUG_UART_BASE
+	hex "Base address of UART for VPL"
+	depends on VPL && DEBUG_UART
+	default DEBUG_UART_BASE
+	help
+	  This is the base address of your UART for memory-mapped UARTs for VPL.
+
 config DEBUG_UART_CLOCK
 	int "UART input clock"
 	depends on DEBUG_UART
-- 
2.34.1

[PATCH 09/18] armv8: Support not having separate BSS

[Simon Glass]

Separate BSS is current mandatory on armv8 but this is not useful for
early boot phases. Add support for the combined BSS.

Use an #ifdef to avoid using CONFIG_SPL_BSS_START_ADDR which is not
valid in this case.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/arm/cpu/armv8/u-boot-spl.lds | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
index 215cedd69a8..fed69644b55 100644
--- a/arch/arm/cpu/armv8/u-boot-spl.lds
+++ b/arch/arm/cpu/armv8/u-boot-spl.lds
@@ -13,8 +13,10 @@
 
 MEMORY { .sram : ORIGIN = IMAGE_TEXT_BASE,
 		LENGTH = IMAGE_MAX_SIZE }
+#ifdef CONFIG_SPL_SEPARATE_BSS
 MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR,
 		LENGTH = CONFIG_SPL_BSS_MAX_SIZE }
+#endif
 
 OUTPUT_FORMAT("elf64-littleaarch64", "elf64-littleaarch64", "elf64-littleaarch64")
 OUTPUT_ARCH(aarch64)
@@ -56,12 +58,22 @@ SECTIONS
 	_end = .;
 	_image_binary_end = .;
 
+#ifdef CONFIG_SPL_SEPARATE_BSS
 	.bss : {
 		__bss_start = .;
 		*(.bss*)
 		. = ALIGN(8);
 		__bss_end = .;
 	} >.sdram
+#else
+	.bss (NOLOAD) : {
+		__bss_start = .;
+		*(.bss*)
+		 . = ALIGN(8);
+		__bss_end = .;
+	} >.sram
+#endif
+	__bss_size = __bss_end - __bss_start;
 
 	/DISCARD/ : { *(.rela*) }
 	/DISCARD/ : { *(.dynsym) }
-- 
2.34.1

[PATCH 10/18] arm: cache: Drop a stale comment

[Simon Glass]

This header includes more than just dummy functions, so drop this
comment.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/arm/lib/cache.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index b2ae74a59f1..648edf308f6 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -4,8 +4,6 @@
  * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
  */
 
-/* for now: just dummy functions to satisfy the linker */
-
 #include <config.h>
 #include <cpu_func.h>
 #include <log.h>
-- 
2.34.1

[PATCH 11/18] arm: Fix up a stale comment in sections.c

[Simon Glass]

There are currently four symbols here, so drop the word 'two'.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/arm/lib/sections.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index 07efabaa7dc..8955aa6111c 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -5,7 +5,7 @@
 #include <linux/compiler.h>
 
 /**
- * These two symbols are declared in a C file so that the linker
+ * These symbols are declared in a C file so that the linker
  * uses R_ARM_RELATIVE relocation, rather than the R_ARM_ABS32 one
  * it would use if the symbols were defined in the linker file.
  * Using only R_ARM_RELATIVE relocation ensures that references to
-- 
2.34.1

[PATCH 12/18] mmc: Support driver model in TPL

[Simon Glass]

Some boards want to use DM_MMC in TPL so add an option for that.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/mmc/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 982e84dc3bc..2b55ddb7af2 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -61,6 +61,18 @@ config SPL_DM_MMC
 	  appear as block devices in U-Boot and can support filesystems such
 	  as EXT4 and FAT.
 
+config TPL_DM_MMC
+	bool "Enable MMC controllers using Driver Model in TPL"
+	depends on TPL_DM && DM_MMC
+	default y
+	select TPL_BLK
+	help
+	  This enables the MultiMediaCard (MMC) uclass which supports MMC and
+	  Secure Digital I/O (SDIO) cards. Both removable (SD, micro-SD, etc.)
+	  and non-removable (e.g. eMMC chip) devices are supported. These
+	  appear as block devices in U-Boot and can support filesystems such
+	  as EXT4 and FAT.
+
 if MMC
 
 config MMC_SDHCI_ADMA_HELPERS
-- 
2.34.1

Re: [PATCH 12/18] mmc: Support driver model in TPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Tue, Aug 27, 2024 at 07:45:31PM -0600, Simon Glass wrote:

> Some boards want to use DM_MMC in TPL so add an option for that.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/mmc/Kconfig | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index 982e84dc3bc..2b55ddb7af2 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -61,6 +61,18 @@ config SPL_DM_MMC
>  	  appear as block devices in U-Boot and can support filesystems such
>  	  as EXT4 and FAT.
>  
> +config TPL_DM_MMC
> +	bool "Enable MMC controllers using Driver Model in TPL"
> +	depends on TPL_DM && DM_MMC
> +	default y

This shouldn't be "default y" as it's adding 128 bytes to TPL on a lot
of platforms.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 13/18] mmc: Add more debugging for SPL

[Simon Glass]

When MMC booting fails it is sometimes hard to figure out what went
wrong as there is no error code. It isn't even clear which MMC device
was chosen, since SPL can have its own numbering.

Add some debugging to help with this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/spl/spl_mmc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 3fd81608e9f..c0e8d407e5d 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -50,6 +50,7 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
 	ret = spl_load(spl_image, bootdev, &load, 0, sector << bd->log2blksz);
 	if (ret) {
 		puts("mmc_load_image_raw_sector: mmc block read error\n");
+		log_debug("(error=%d)\n", ret);
 		return ret;
 	}
 
@@ -76,6 +77,12 @@ static int spl_mmc_find_device(struct mmc **mmcp, int mmc_dev)
 	int ret;
 
 #if CONFIG_IS_ENABLED(DM_MMC)
+	struct udevice *dev;
+	struct uclass *uc;
+
+	log_debug("Selecting MMC dev %d; seqs:\n", mmc_dev);
+	uclass_id_foreach_dev(UCLASS_MMC, dev, uc)
+		log_debug("%d: %s\n", dev_seq(dev), dev->name);
 	ret = mmc_init_device(mmc_dev);
 #else
 	ret = mmc_initialize(NULL);
@@ -91,6 +98,10 @@ static int spl_mmc_find_device(struct mmc **mmcp, int mmc_dev)
 		       mmc_dev, ret);
 		return ret;
 	}
+#if CONFIG_IS_ENABLED(DM_MMC)
+	log_debug("mmc %d: %s\n", mmc_dev,
+		  CONFIG_IS_ENABLED(DM_MMC) ? (*mmcp)->dev->name : "?");
+#endif
 
 	return 0;
 }
@@ -342,6 +353,8 @@ int spl_mmc_load(struct spl_image_info *spl_image,
 
 	/* Perform peripheral init only once for an mmc device */
 	mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
+	log_debug("boot_device=%d, mmc_dev=%d\n", bootdev->boot_device,
+		  mmc_dev);
 	if (!mmc || spl_mmc_get_mmc_devnum(mmc) != mmc_dev) {
 		ret = spl_mmc_find_device(&mmc, mmc_dev);
 		if (ret)
-- 
2.34.1

Re: [PATCH 13/18] mmc: Add more debugging for SPL

[Marek Vasut]

On 8/28/24 3:45 AM, Simon Glass wrote:
> When MMC booting fails it is sometimes hard to figure out what went
> wrong as there is no error code. It isn't even clear which MMC device
> was chosen, since SPL can have its own numbering.
> 
> Add some debugging to help with this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>   common/spl/spl_mmc.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index 3fd81608e9f..c0e8d407e5d 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -50,6 +50,7 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
>   	ret = spl_load(spl_image, bootdev, &load, 0, sector << bd->log2blksz);
>   	if (ret) {
>   		puts("mmc_load_image_raw_sector: mmc block read error\n");
> +		log_debug("(error=%d)\n", ret);
>   		return ret;
>   	}
>   
> @@ -76,6 +77,12 @@ static int spl_mmc_find_device(struct mmc **mmcp, int mmc_dev)
>   	int ret;
>   
>   #if CONFIG_IS_ENABLED(DM_MMC)
> +	struct udevice *dev;
> +	struct uclass *uc;
> +
> +	log_debug("Selecting MMC dev %d; seqs:\n", mmc_dev);
> +	uclass_id_foreach_dev(UCLASS_MMC, dev, uc)
> +		log_debug("%d: %s\n", dev_seq(dev), dev->name);
>   	ret = mmc_init_device(mmc_dev);
>   #else
>   	ret = mmc_initialize(NULL);
> @@ -91,6 +98,10 @@ static int spl_mmc_find_device(struct mmc **mmcp, int mmc_dev)
>   		       mmc_dev, ret);
>   		return ret;
>   	}
> +#if CONFIG_IS_ENABLED(DM_MMC)

if (CONFIG_IS_ENABLED(...)) ?

Re: [PATCH 13/18] mmc: Add more debugging for SPL

[Simon Glass]

Hi Marek,

On Tue, 27 Aug 2024 at 22:10, Marek Vasut <marex@denx.de> wrote:
>
> On 8/28/24 3:45 AM, Simon Glass wrote:
> > When MMC booting fails it is sometimes hard to figure out what went
> > wrong as there is no error code. It isn't even clear which MMC device
> > was chosen, since SPL can have its own numbering.
> >
> > Add some debugging to help with this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   common/spl/spl_mmc.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > index 3fd81608e9f..c0e8d407e5d 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -50,6 +50,7 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
> >       ret = spl_load(spl_image, bootdev, &load, 0, sector << bd->log2blksz);
> >       if (ret) {
> >               puts("mmc_load_image_raw_sector: mmc block read error\n");
> > +             log_debug("(error=%d)\n", ret);
> >               return ret;
> >       }
> >
> > @@ -76,6 +77,12 @@ static int spl_mmc_find_device(struct mmc **mmcp, int mmc_dev)
> >       int ret;
> >
> >   #if CONFIG_IS_ENABLED(DM_MMC)
> > +     struct udevice *dev;
> > +     struct uclass *uc;
> > +
> > +     log_debug("Selecting MMC dev %d; seqs:\n", mmc_dev);
> > +     uclass_id_foreach_dev(UCLASS_MMC, dev, uc)
> > +             log_debug("%d: %s\n", dev_seq(dev), dev->name);
> >       ret = mmc_init_device(mmc_dev);
> >   #else
> >       ret = mmc_initialize(NULL);
> > @@ -91,6 +98,10 @@ static int spl_mmc_find_device(struct mmc **mmcp, int mmc_dev)
> >                      mmc_dev, ret);
> >               return ret;
> >       }
> > +#if CONFIG_IS_ENABLED(DM_MMC)
>
> if (CONFIG_IS_ENABLED(...)) ?

We could, but don't need that?

Regards,
Simon

Re: [PATCH 13/18] mmc: Add more debugging for SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Tue, Aug 27, 2024 at 07:45:32PM -0600, Simon Glass wrote:

> When MMC booting fails it is sometimes hard to figure out what went
> wrong as there is no error code. It isn't even clear which MMC device
> was chosen, since SPL can have its own numbering.
> 
> Add some debugging to help with this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
[snip]
> +#if CONFIG_IS_ENABLED(DM_MMC)
> +	log_debug("mmc %d: %s\n", mmc_dev,
> +		  CONFIG_IS_ENABLED(DM_MMC) ? (*mmcp)->dev->name : "?");
> +#endif

Please rework to clean this up. I'm not sure we can drop the #if here
(as if ->dev->name is invalid in the non-DM_MMC case, compilation will
still fail), but it's silly to have a check for
CONFIG_IS_ENABLED(DM_MMC) inside #if CONFIG_IS_ENABLED(DM_MMC)

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 14/18] mmc: Log the error when init fails

[Simon Glass]

Add an error-return log to the call in mmc_init_device()

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/mmc/mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 9644aa7aa43..eece4784288 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -3153,7 +3153,7 @@ int mmc_init_device(int num)
 	if (uclass_get_device_by_seq(UCLASS_MMC, num, &dev)) {
 		ret = uclass_get_device(UCLASS_MMC, num, &dev);
 		if (ret)
-			return ret;
+			return log_msg_ret("ini", ret);
 	}
 
 	m = mmc_get_mmc_dev(dev);
-- 
2.34.1

[PATCH 15/18] mmc: rockchip: Log some error returns

[Simon Glass]

Add a little logging to some places in this driver, to aid debugging
when something goes wrong.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/mmc/rockchip_dw_mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
index fb77b049834..7e341665aa3 100644
--- a/drivers/mmc/rockchip_dw_mmc.c
+++ b/drivers/mmc/rockchip_dw_mmc.c
@@ -80,7 +80,7 @@ static int rockchip_dwmmc_of_to_plat(struct udevice *dev)
 	priv->fifo_depth = dev_read_u32_default(dev, "fifo-depth", 0);
 
 	if (priv->fifo_depth < 0)
-		return -EINVAL;
+		return log_msg_ret("rkp", -EINVAL);
 	priv->fifo_mode = dev_read_bool(dev, "fifo-mode");
 
 #ifdef CONFIG_SPL_BUILD
@@ -96,7 +96,7 @@ static int rockchip_dwmmc_of_to_plat(struct udevice *dev)
 		int val = dev_read_u32_default(dev, "max-frequency", -EINVAL);
 
 		if (val < 0)
-			return val;
+			return log_msg_ret("rkc", val);
 
 		priv->minmax[0] = 400000;  /* 400 kHz */
 		priv->minmax[1] = val;
-- 
2.34.1

[PATCH 16/18] mmc: rockchip: Allow clocks to be missing

[Simon Glass]

Allow MMC init when clock support is not enabled in a particular phase.

Refactor the setting of priv->emmc_clk so it is a bit clearer.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/mmc/rockchip_dw_mmc.c | 6 ++----
 drivers/mmc/rockchip_sdhci.c  | 9 ++++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
index 7e341665aa3..5ba99d68b7d 100644
--- a/drivers/mmc/rockchip_dw_mmc.c
+++ b/drivers/mmc/rockchip_dw_mmc.c
@@ -131,13 +131,11 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
 	priv->minmax[1] = dtplat->max_frequency;
 
 	ret = clk_get_by_phandle(dev, &dtplat->clocks[1], &priv->clk);
-	if (ret < 0)
-		return ret;
 #else
 	ret = clk_get_by_index(dev, 1, &priv->clk);
-	if (ret < 0)
-		return ret;
 #endif
+	if (ret < 0 && ret != -ENOSYS)
+		return log_msg_ret("clk", ret);
 	host->fifo_depth = priv->fifo_depth;
 	host->fifo_mode = priv->fifo_mode;
 
diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index 35667b86b50..15b4a39770a 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -571,20 +571,19 @@ static int rockchip_sdhci_probe(struct udevice *dev)
 	struct rockchip_sdhc *priv = dev_get_priv(dev);
 	struct mmc_config *cfg = &plat->cfg;
 	struct sdhci_host *host = &priv->host;
-	struct clk clk;
+	struct clk *clk = &priv->emmc_clk;
 	int ret;
 
 	host->max_clk = cfg->f_max;
-	ret = clk_get_by_index(dev, 0, &clk);
+	ret = clk_get_by_index(dev, 0, clk);
 	if (!ret) {
-		ret = clk_set_rate(&clk, host->max_clk);
+		ret = clk_set_rate(clk, host->max_clk);
 		if (IS_ERR_VALUE(ret))
 			printf("%s clk set rate fail!\n", __func__);
-	} else {
+	} else if (ret != -ENOSYS) {
 		printf("%s fail to get clk\n", __func__);
 	}
 
-	priv->emmc_clk = clk;
 	priv->dev = dev;
 
 	if (data->get_phy) {
-- 
2.34.1

[PATCH 17/18] rockchip: mmc: Fix a missing colon

[Simon Glass]

Add a missing colon in rk3399_emmc_get_phy().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/mmc/rockchip_sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index 15b4a39770a..6c5c8781c3e 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -230,7 +230,7 @@ static int rk3399_emmc_get_phy(struct udevice *dev)
 
 	grf_base = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
 	if (IS_ERR_OR_NULL(grf_base)) {
-		printf("%s Get syscon grf failed", __func__);
+		printf("%s Get syscon grf failed\n", __func__);
 		return -ENODEV;
 	}
 	grf_phy_offset = ofnode_read_u32_default(phy_node, "reg", 0);
-- 
2.34.1

Re: [PATCH 17/18] rockchip: mmc: Fix a missing colon

[Sughosh Ganu]

On Wed, 28 Aug 2024 at 07:18, Simon Glass <sjg@chromium.org> wrote:
>
> Add a missing colon in rk3399_emmc_get_phy().
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

The change seems to be adding a newline, and not a colon as the commit
message is suggesting.

-sughosh

>
>  drivers/mmc/rockchip_sdhci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
> index 15b4a39770a..6c5c8781c3e 100644
> --- a/drivers/mmc/rockchip_sdhci.c
> +++ b/drivers/mmc/rockchip_sdhci.c
> @@ -230,7 +230,7 @@ static int rk3399_emmc_get_phy(struct udevice *dev)
>
>         grf_base = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
>         if (IS_ERR_OR_NULL(grf_base)) {
> -               printf("%s Get syscon grf failed", __func__);
> +               printf("%s Get syscon grf failed\n", __func__);
>                 return -ENODEV;
>         }
>         grf_phy_offset = ofnode_read_u32_default(phy_node, "reg", 0);
> --
> 2.34.1
>

[PATCH 18/18] rockchip: Provided SPL control over efuse presence

[Simon Glass]

This driver should not generally be present in SPL, even if misc devices
are enabled. Update the Makefile rule accordingly.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/misc/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e53d52c47b3..ff984d7b191 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -68,7 +68,7 @@ obj-$(CONFIG_QFW_MMIO) += qfw_mmio.o
 obj-$(CONFIG_QFW_SMBIOS) += qfw_smbios.o
 obj-$(CONFIG_SANDBOX) += qfw_sandbox.o
 endif
-obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
+obj-$(CONFIG_$(SPL_TPL_)ROCKCHIP_EFUSE) += rockchip-efuse.o
 obj-$(CONFIG_ROCKCHIP_OTP) += rockchip-otp.o
 obj-$(CONFIG_$(SPL_TPL_)ROCKCHIP_IODOMAIN) += rockchip-io-domain.o
 obj-$(CONFIG_SANDBOX) += syscon_sandbox.o misc_sandbox.o
-- 
2.34.1

Re: [PATCH 00/18] vbe: Series part D

[Peter Robinson]

Hi Simon,

> This includes various patches towards implementing the VBE abrec

What is abrec?

> bootmeth in U-Boot.
>
>
> Simon Glass (18):
>   sandbox: Add missing header file
>   bootstd: Add stub for bootdev_setup_for_sibling_blk()
>   gzip: Correct function comment for gunzip()

Is this from upstream gzip code somewher?

>   fdtdec: Support separate BSS for all XPL builds
>   tiny-printf: Correct return values
>   tpl: Support numbered aliases in device tree
>   ram: Support driver model in TPL
>   serial: Support debug UART in TPL
>   armv8: Support not having separate BSS
>   arm: cache: Drop a stale comment
>   arm: Fix up a stale comment in sections.c
>   mmc: Support driver model in TPL
>   mmc: Add more debugging for SPL
>   mmc: Log the error when init fails
>   mmc: rockchip: Log some error returns
>   mmc: rockchip: Allow clocks to be missing
>   rockchip: mmc: Fix a missing colon
>   rockchip: Provided SPL control over efuse presence

I'm not sure what most of these patches have to do with VBE?

>  arch/arm/cpu/armv8/u-boot-spl.lds   | 12 ++++++++++++
>  arch/arm/lib/cache.c                |  2 --
>  arch/arm/lib/sections.c             |  2 +-
>  arch/sandbox/include/asm/sections.h |  1 +
>  common/spl/spl_mmc.c                | 13 +++++++++++++
>  drivers/core/Kconfig                |  8 ++++++++
>  drivers/misc/Makefile               |  2 +-
>  drivers/mmc/Kconfig                 | 12 ++++++++++++
>  drivers/mmc/mmc.c                   |  2 +-
>  drivers/mmc/rockchip_dw_mmc.c       | 10 ++++------
>  drivers/mmc/rockchip_sdhci.c        | 11 +++++------
>  drivers/ram/Kconfig                 |  9 +++++++++
>  drivers/serial/Kconfig              |  7 +++++++
>  include/bootdev.h                   |  8 ++++++++
>  include/gzip.h                      |  6 ++++--
>  lib/fdtdec.c                        |  2 +-
>  lib/tiny-printf.c                   | 15 ++++++---------
>  17 files changed, 93 insertions(+), 29 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH 00/18] vbe: Series part D

[Simon Glass]

Hi Peter,

On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> Hi Simon,
>
> > This includes various patches towards implementing the VBE abrec
>
> What is abrec?

It's a VBE method which supports A, B and recovery images and permits
updating from SPL onwards. The idea is that you can safely update
without bricking a device.

>
> > bootmeth in U-Boot.
> >
> >
> > Simon Glass (18):
> >   sandbox: Add missing header file
> >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> >   gzip: Correct function comment for gunzip()
>
> Is this from upstream gzip code somewher?

I'm not sure, actually.

>
> >   fdtdec: Support separate BSS for all XPL builds

Needed so that VPL can use DDT

> >   tiny-printf: Correct return values

I can't remember, sorry.

> >   tpl: Support numbered aliases in device tree

The MMC needs to be accessed using its sequence number

> >   ram: Support driver model in TPL

That should say VPL...but again I'm not quite sure

> >   serial: Support debug UART in TPL

This allows debugging of the jump from TPL to VPL

> >   armv8: Support not having separate BSS

VPL doesn't want a separate BSS since SDRAM isn't available that early

> >   arm: cache: Drop a stale comment

Just something I noticed

> >   arm: Fix up a stale comment in sections.c

Another thing I noticed

> >   mmc: Support driver model in TPL

TPL needs to read VPL from MMC

> >   mmc: Add more debugging for SPL
> >   mmc: Log the error when init fails
> >   mmc: rockchip: Log some error returns
> >   mmc: rockchip: Allow clocks to be missing

These all help with debugging reading of VPL from the correct MMC device

> >   rockchip: mmc: Fix a missing colon

Just something I noticed

> >   rockchip: Provided SPL control over efuse presence

We don't want this driver in TPL or VPL

>
> I'm not sure what most of these patches have to do with VBE?

Basically there are a lot of little tweaks needed.

>
> >  arch/arm/cpu/armv8/u-boot-spl.lds   | 12 ++++++++++++
> >  arch/arm/lib/cache.c                |  2 --
> >  arch/arm/lib/sections.c             |  2 +-
> >  arch/sandbox/include/asm/sections.h |  1 +
> >  common/spl/spl_mmc.c                | 13 +++++++++++++
> >  drivers/core/Kconfig                |  8 ++++++++
> >  drivers/misc/Makefile               |  2 +-
> >  drivers/mmc/Kconfig                 | 12 ++++++++++++
> >  drivers/mmc/mmc.c                   |  2 +-
> >  drivers/mmc/rockchip_dw_mmc.c       | 10 ++++------
> >  drivers/mmc/rockchip_sdhci.c        | 11 +++++------
> >  drivers/ram/Kconfig                 |  9 +++++++++
> >  drivers/serial/Kconfig              |  7 +++++++
> >  include/bootdev.h                   |  8 ++++++++
> >  include/gzip.h                      |  6 ++++--
> >  lib/fdtdec.c                        |  2 +-
> >  lib/tiny-printf.c                   | 15 ++++++---------
> >  17 files changed, 93 insertions(+), 29 deletions(-)
> >
> > --
> > 2.34.1
> >

Regards,
SImon

Re: [PATCH 00/18] vbe: Series part D

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2663 bytes --]

On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> Hi Peter,
> 
> On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > > This includes various patches towards implementing the VBE abrec
> >
> > What is abrec?
> 
> It's a VBE method which supports A, B and recovery images and permits
> updating from SPL onwards. The idea is that you can safely update
> without bricking a device.
> 
> >
> > > bootmeth in U-Boot.
> > >
> > >
> > > Simon Glass (18):
> > >   sandbox: Add missing header file
> > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > >   gzip: Correct function comment for gunzip()
> >
> > Is this from upstream gzip code somewher?
> 
> I'm not sure, actually.
> 
> >
> > >   fdtdec: Support separate BSS for all XPL builds
> 
> Needed so that VPL can use DDT
> 
> > >   tiny-printf: Correct return values
> 
> I can't remember, sorry.
> 
> > >   tpl: Support numbered aliases in device tree
> 
> The MMC needs to be accessed using its sequence number
> 
> > >   ram: Support driver model in TPL
> 
> That should say VPL...but again I'm not quite sure
> 
> > >   serial: Support debug UART in TPL
> 
> This allows debugging of the jump from TPL to VPL
> 
> > >   armv8: Support not having separate BSS
> 
> VPL doesn't want a separate BSS since SDRAM isn't available that early
> 
> > >   arm: cache: Drop a stale comment
> 
> Just something I noticed
> 
> > >   arm: Fix up a stale comment in sections.c
> 
> Another thing I noticed
> 
> > >   mmc: Support driver model in TPL
> 
> TPL needs to read VPL from MMC
> 
> > >   mmc: Add more debugging for SPL
> > >   mmc: Log the error when init fails
> > >   mmc: rockchip: Log some error returns
> > >   mmc: rockchip: Allow clocks to be missing
> 
> These all help with debugging reading of VPL from the correct MMC device
> 
> > >   rockchip: mmc: Fix a missing colon
> 
> Just something I noticed
> 
> > >   rockchip: Provided SPL control over efuse presence
> 
> We don't want this driver in TPL or VPL
> 
> >
> > I'm not sure what most of these patches have to do with VBE?
> 
> Basically there are a lot of little tweaks needed.

Well, I think this gets back to some common feedback. You have a lot of
little bugfixes, which is good, but they get sprinkled in to the next
big series you post, which is bad and hard to review. And that leads to
fixes not getting merged / reviewed timely because nominally simple fix
A is in part 3 of a series to introduce something larger and to which
there is feedback to work through.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/18] vbe: Series part D

[Simon Glass]

Hi Tom,

On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > Hi Peter,
> >
> > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > > This includes various patches towards implementing the VBE abrec
> > >
> > > What is abrec?
> >
> > It's a VBE method which supports A, B and recovery images and permits
> > updating from SPL onwards. The idea is that you can safely update
> > without bricking a device.
> >
> > >
> > > > bootmeth in U-Boot.
> > > >
> > > >
> > > > Simon Glass (18):
> > > >   sandbox: Add missing header file
> > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > >   gzip: Correct function comment for gunzip()
> > >
> > > Is this from upstream gzip code somewher?
> >
> > I'm not sure, actually.
> >
> > >
> > > >   fdtdec: Support separate BSS for all XPL builds
> >
> > Needed so that VPL can use DDT
> >
> > > >   tiny-printf: Correct return values
> >
> > I can't remember, sorry.
> >
> > > >   tpl: Support numbered aliases in device tree
> >
> > The MMC needs to be accessed using its sequence number
> >
> > > >   ram: Support driver model in TPL
> >
> > That should say VPL...but again I'm not quite sure
> >
> > > >   serial: Support debug UART in TPL
> >
> > This allows debugging of the jump from TPL to VPL
> >
> > > >   armv8: Support not having separate BSS
> >
> > VPL doesn't want a separate BSS since SDRAM isn't available that early
> >
> > > >   arm: cache: Drop a stale comment
> >
> > Just something I noticed
> >
> > > >   arm: Fix up a stale comment in sections.c
> >
> > Another thing I noticed
> >
> > > >   mmc: Support driver model in TPL
> >
> > TPL needs to read VPL from MMC
> >
> > > >   mmc: Add more debugging for SPL
> > > >   mmc: Log the error when init fails
> > > >   mmc: rockchip: Log some error returns
> > > >   mmc: rockchip: Allow clocks to be missing
> >
> > These all help with debugging reading of VPL from the correct MMC device
> >
> > > >   rockchip: mmc: Fix a missing colon
> >
> > Just something I noticed
> >
> > > >   rockchip: Provided SPL control over efuse presence
> >
> > We don't want this driver in TPL or VPL
> >
> > >
> > > I'm not sure what most of these patches have to do with VBE?
> >
> > Basically there are a lot of little tweaks needed.
>
> Well, I think this gets back to some common feedback. You have a lot of
> little bugfixes, which is good, but they get sprinkled in to the next
> big series you post, which is bad and hard to review. And that leads to
> fixes not getting merged / reviewed timely because nominally simple fix
> A is in part 3 of a series to introduce something larger and to which
> there is feedback to work through.

I actually thought I was splitting them up quite nicely. What do you suggest?

Regards,
Simon

Re: [PATCH 00/18] vbe: Series part D

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]

On Thu, Aug 29, 2024 at 07:06:32PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > > Hi Peter,
> > >
> > > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > > This includes various patches towards implementing the VBE abrec
> > > >
> > > > What is abrec?
> > >
> > > It's a VBE method which supports A, B and recovery images and permits
> > > updating from SPL onwards. The idea is that you can safely update
> > > without bricking a device.
> > >
> > > >
> > > > > bootmeth in U-Boot.
> > > > >
> > > > >
> > > > > Simon Glass (18):
> > > > >   sandbox: Add missing header file
> > > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > > >   gzip: Correct function comment for gunzip()
> > > >
> > > > Is this from upstream gzip code somewher?
> > >
> > > I'm not sure, actually.
> > >
> > > >
> > > > >   fdtdec: Support separate BSS for all XPL builds
> > >
> > > Needed so that VPL can use DDT
> > >
> > > > >   tiny-printf: Correct return values
> > >
> > > I can't remember, sorry.
> > >
> > > > >   tpl: Support numbered aliases in device tree
> > >
> > > The MMC needs to be accessed using its sequence number
> > >
> > > > >   ram: Support driver model in TPL
> > >
> > > That should say VPL...but again I'm not quite sure
> > >
> > > > >   serial: Support debug UART in TPL
> > >
> > > This allows debugging of the jump from TPL to VPL
> > >
> > > > >   armv8: Support not having separate BSS
> > >
> > > VPL doesn't want a separate BSS since SDRAM isn't available that early
> > >
> > > > >   arm: cache: Drop a stale comment
> > >
> > > Just something I noticed
> > >
> > > > >   arm: Fix up a stale comment in sections.c
> > >
> > > Another thing I noticed
> > >
> > > > >   mmc: Support driver model in TPL
> > >
> > > TPL needs to read VPL from MMC
> > >
> > > > >   mmc: Add more debugging for SPL
> > > > >   mmc: Log the error when init fails
> > > > >   mmc: rockchip: Log some error returns
> > > > >   mmc: rockchip: Allow clocks to be missing
> > >
> > > These all help with debugging reading of VPL from the correct MMC device
> > >
> > > > >   rockchip: mmc: Fix a missing colon
> > >
> > > Just something I noticed
> > >
> > > > >   rockchip: Provided SPL control over efuse presence
> > >
> > > We don't want this driver in TPL or VPL
> > >
> > > >
> > > > I'm not sure what most of these patches have to do with VBE?
> > >
> > > Basically there are a lot of little tweaks needed.
> >
> > Well, I think this gets back to some common feedback. You have a lot of
> > little bugfixes, which is good, but they get sprinkled in to the next
> > big series you post, which is bad and hard to review. And that leads to
> > fixes not getting merged / reviewed timely because nominally simple fix
> > A is in part 3 of a series to introduce something larger and to which
> > there is feedback to work through.
> 
> I actually thought I was splitting them up quite nicely. What do you suggest?

I would refer back to your own comments about what some of the patches
do and say that "Just something I noticed" should be a one off, and
little fixes for this-and-that should be a 1-2 part series and so on.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/18] vbe: Series part D

[Simon Glass]

Hi Tom,

On Fri, 30 Aug 2024 at 03:46, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 29, 2024 at 07:06:32PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > > > Hi Peter,
> > > >
> > > > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > > This includes various patches towards implementing the VBE abrec
> > > > >
> > > > > What is abrec?
> > > >
> > > > It's a VBE method which supports A, B and recovery images and permits
> > > > updating from SPL onwards. The idea is that you can safely update
> > > > without bricking a device.
> > > >
> > > > >
> > > > > > bootmeth in U-Boot.
> > > > > >
> > > > > >
> > > > > > Simon Glass (18):
> > > > > >   sandbox: Add missing header file
> > > > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > > > >   gzip: Correct function comment for gunzip()
> > > > >
> > > > > Is this from upstream gzip code somewher?
> > > >
> > > > I'm not sure, actually.
> > > >
> > > > >
> > > > > >   fdtdec: Support separate BSS for all XPL builds
> > > >
> > > > Needed so that VPL can use DDT
> > > >
> > > > > >   tiny-printf: Correct return values
> > > >
> > > > I can't remember, sorry.
> > > >
> > > > > >   tpl: Support numbered aliases in device tree
> > > >
> > > > The MMC needs to be accessed using its sequence number
> > > >
> > > > > >   ram: Support driver model in TPL
> > > >
> > > > That should say VPL...but again I'm not quite sure
> > > >
> > > > > >   serial: Support debug UART in TPL
> > > >
> > > > This allows debugging of the jump from TPL to VPL
> > > >
> > > > > >   armv8: Support not having separate BSS
> > > >
> > > > VPL doesn't want a separate BSS since SDRAM isn't available that early
> > > >
> > > > > >   arm: cache: Drop a stale comment
> > > >
> > > > Just something I noticed
> > > >
> > > > > >   arm: Fix up a stale comment in sections.c
> > > >
> > > > Another thing I noticed
> > > >
> > > > > >   mmc: Support driver model in TPL
> > > >
> > > > TPL needs to read VPL from MMC
> > > >
> > > > > >   mmc: Add more debugging for SPL
> > > > > >   mmc: Log the error when init fails
> > > > > >   mmc: rockchip: Log some error returns
> > > > > >   mmc: rockchip: Allow clocks to be missing
> > > >
> > > > These all help with debugging reading of VPL from the correct MMC device
> > > >
> > > > > >   rockchip: mmc: Fix a missing colon
> > > >
> > > > Just something I noticed
> > > >
> > > > > >   rockchip: Provided SPL control over efuse presence
> > > >
> > > > We don't want this driver in TPL or VPL
> > > >
> > > > >
> > > > > I'm not sure what most of these patches have to do with VBE?
> > > >
> > > > Basically there are a lot of little tweaks needed.
> > >
> > > Well, I think this gets back to some common feedback. You have a lot of
> > > little bugfixes, which is good, but they get sprinkled in to the next
> > > big series you post, which is bad and hard to review. And that leads to
> > > fixes not getting merged / reviewed timely because nominally simple fix
> > > A is in part 3 of a series to introduce something larger and to which
> > > there is feedback to work through.
> >
> > I actually thought I was splitting them up quite nicely. What do you suggest?
>
> I would refer back to your own comments about what some of the patches
> do and say that "Just something I noticed" should be a one off, and
> little fixes for this-and-that should be a 1-2 part series and so on.

I really don't have a great workflow for keeping track of more than
6-8 pending series and even that is sometimes a struggle. For testing
I need everything applied so I have every series in a sequence applied
locally.

I will have a think about what can be done. I wonder how others do it?

Regards,
Simon

Re: [PATCH 00/18] vbe: Series part D

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4529 bytes --]

On Fri, Sep 20, 2024 at 09:25:53AM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 30 Aug 2024 at 03:46, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Aug 29, 2024 at 07:06:32PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > > > > Hi Peter,
> > > > >
> > > > > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > > This includes various patches towards implementing the VBE abrec
> > > > > >
> > > > > > What is abrec?
> > > > >
> > > > > It's a VBE method which supports A, B and recovery images and permits
> > > > > updating from SPL onwards. The idea is that you can safely update
> > > > > without bricking a device.
> > > > >
> > > > > >
> > > > > > > bootmeth in U-Boot.
> > > > > > >
> > > > > > >
> > > > > > > Simon Glass (18):
> > > > > > >   sandbox: Add missing header file
> > > > > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > > > > >   gzip: Correct function comment for gunzip()
> > > > > >
> > > > > > Is this from upstream gzip code somewher?
> > > > >
> > > > > I'm not sure, actually.
> > > > >
> > > > > >
> > > > > > >   fdtdec: Support separate BSS for all XPL builds
> > > > >
> > > > > Needed so that VPL can use DDT
> > > > >
> > > > > > >   tiny-printf: Correct return values
> > > > >
> > > > > I can't remember, sorry.
> > > > >
> > > > > > >   tpl: Support numbered aliases in device tree
> > > > >
> > > > > The MMC needs to be accessed using its sequence number
> > > > >
> > > > > > >   ram: Support driver model in TPL
> > > > >
> > > > > That should say VPL...but again I'm not quite sure
> > > > >
> > > > > > >   serial: Support debug UART in TPL
> > > > >
> > > > > This allows debugging of the jump from TPL to VPL
> > > > >
> > > > > > >   armv8: Support not having separate BSS
> > > > >
> > > > > VPL doesn't want a separate BSS since SDRAM isn't available that early
> > > > >
> > > > > > >   arm: cache: Drop a stale comment
> > > > >
> > > > > Just something I noticed
> > > > >
> > > > > > >   arm: Fix up a stale comment in sections.c
> > > > >
> > > > > Another thing I noticed
> > > > >
> > > > > > >   mmc: Support driver model in TPL
> > > > >
> > > > > TPL needs to read VPL from MMC
> > > > >
> > > > > > >   mmc: Add more debugging for SPL
> > > > > > >   mmc: Log the error when init fails
> > > > > > >   mmc: rockchip: Log some error returns
> > > > > > >   mmc: rockchip: Allow clocks to be missing
> > > > >
> > > > > These all help with debugging reading of VPL from the correct MMC device
> > > > >
> > > > > > >   rockchip: mmc: Fix a missing colon
> > > > >
> > > > > Just something I noticed
> > > > >
> > > > > > >   rockchip: Provided SPL control over efuse presence
> > > > >
> > > > > We don't want this driver in TPL or VPL
> > > > >
> > > > > >
> > > > > > I'm not sure what most of these patches have to do with VBE?
> > > > >
> > > > > Basically there are a lot of little tweaks needed.
> > > >
> > > > Well, I think this gets back to some common feedback. You have a lot of
> > > > little bugfixes, which is good, but they get sprinkled in to the next
> > > > big series you post, which is bad and hard to review. And that leads to
> > > > fixes not getting merged / reviewed timely because nominally simple fix
> > > > A is in part 3 of a series to introduce something larger and to which
> > > > there is feedback to work through.
> > >
> > > I actually thought I was splitting them up quite nicely. What do you suggest?
> >
> > I would refer back to your own comments about what some of the patches
> > do and say that "Just something I noticed" should be a one off, and
> > little fixes for this-and-that should be a 1-2 part series and so on.
> 
> I really don't have a great workflow for keeping track of more than
> 6-8 pending series and even that is sometimes a struggle. For testing
> I need everything applied so I have every series in a sequence applied
> locally.
> 
> I will have a think about what can be done. I wonder how others do it?

I believe the answer is multiple branches and merging them as needed. Or
what I do is make each subsequent series a branch off the previous one
(and set the upstream appropriately) and then rebase as needed.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/18] vbe: Series part D

[Simon Glass]

Hi Tom,

On Fri, 20 Sept 2024 at 17:01, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Sep 20, 2024 at 09:25:53AM +0200, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 30 Aug 2024 at 03:46, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Aug 29, 2024 at 07:06:32PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > > > > > Hi Peter,
> > > > > >
> > > > > > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > > This includes various patches towards implementing the VBE abrec
> > > > > > >
> > > > > > > What is abrec?
> > > > > >
> > > > > > It's a VBE method which supports A, B and recovery images and permits
> > > > > > updating from SPL onwards. The idea is that you can safely update
> > > > > > without bricking a device.
> > > > > >
> > > > > > >
> > > > > > > > bootmeth in U-Boot.
> > > > > > > >
> > > > > > > >
> > > > > > > > Simon Glass (18):
> > > > > > > >   sandbox: Add missing header file
> > > > > > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > > > > > >   gzip: Correct function comment for gunzip()
> > > > > > >
> > > > > > > Is this from upstream gzip code somewher?
> > > > > >
> > > > > > I'm not sure, actually.
> > > > > >
> > > > > > >
> > > > > > > >   fdtdec: Support separate BSS for all XPL builds
> > > > > >
> > > > > > Needed so that VPL can use DDT
> > > > > >
> > > > > > > >   tiny-printf: Correct return values
> > > > > >
> > > > > > I can't remember, sorry.
> > > > > >
> > > > > > > >   tpl: Support numbered aliases in device tree
> > > > > >
> > > > > > The MMC needs to be accessed using its sequence number
> > > > > >
> > > > > > > >   ram: Support driver model in TPL
> > > > > >
> > > > > > That should say VPL...but again I'm not quite sure
> > > > > >
> > > > > > > >   serial: Support debug UART in TPL
> > > > > >
> > > > > > This allows debugging of the jump from TPL to VPL
> > > > > >
> > > > > > > >   armv8: Support not having separate BSS
> > > > > >
> > > > > > VPL doesn't want a separate BSS since SDRAM isn't available that early
> > > > > >
> > > > > > > >   arm: cache: Drop a stale comment
> > > > > >
> > > > > > Just something I noticed
> > > > > >
> > > > > > > >   arm: Fix up a stale comment in sections.c
> > > > > >
> > > > > > Another thing I noticed
> > > > > >
> > > > > > > >   mmc: Support driver model in TPL
> > > > > >
> > > > > > TPL needs to read VPL from MMC
> > > > > >
> > > > > > > >   mmc: Add more debugging for SPL
> > > > > > > >   mmc: Log the error when init fails
> > > > > > > >   mmc: rockchip: Log some error returns
> > > > > > > >   mmc: rockchip: Allow clocks to be missing
> > > > > >
> > > > > > These all help with debugging reading of VPL from the correct MMC device
> > > > > >
> > > > > > > >   rockchip: mmc: Fix a missing colon
> > > > > >
> > > > > > Just something I noticed
> > > > > >
> > > > > > > >   rockchip: Provided SPL control over efuse presence
> > > > > >
> > > > > > We don't want this driver in TPL or VPL
> > > > > >
> > > > > > >
> > > > > > > I'm not sure what most of these patches have to do with VBE?
> > > > > >
> > > > > > Basically there are a lot of little tweaks needed.
> > > > >
> > > > > Well, I think this gets back to some common feedback. You have a lot of
> > > > > little bugfixes, which is good, but they get sprinkled in to the next
> > > > > big series you post, which is bad and hard to review. And that leads to
> > > > > fixes not getting merged / reviewed timely because nominally simple fix
> > > > > A is in part 3 of a series to introduce something larger and to which
> > > > > there is feedback to work through.
> > > >
> > > > I actually thought I was splitting them up quite nicely. What do you suggest?
> > >
> > > I would refer back to your own comments about what some of the patches
> > > do and say that "Just something I noticed" should be a one off, and
> > > little fixes for this-and-that should be a 1-2 part series and so on.
> >
> > I really don't have a great workflow for keeping track of more than
> > 6-8 pending series and even that is sometimes a struggle. For testing
> > I need everything applied so I have every series in a sequence applied
> > locally.
> >
> > I will have a think about what can be done. I wonder how others do it?
>
> I believe the answer is multiple branches and merging them as needed. Or
> what I do is make each subsequent series a branch off the previous one
> (and set the upstream appropriately) and then rebase as needed.

OK. That is my current workflow, but it can get out of hand,
particularly something like VBE abrec where over 100 patches in 6
-or-so different series are needed. I suppose that is why I keep
putting over doing the final two series...

Regards,
Simon

Re: [PATCH 00/18] vbe: Series part D

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6039 bytes --]

On Fri, Sep 20, 2024 at 06:04:05PM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 20 Sept 2024 at 17:01, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Sep 20, 2024 at 09:25:53AM +0200, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 30 Aug 2024 at 03:46, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Aug 29, 2024 at 07:06:32PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > > > > > > Hi Peter,
> > > > > > >
> > > > > > > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > > This includes various patches towards implementing the VBE abrec
> > > > > > > >
> > > > > > > > What is abrec?
> > > > > > >
> > > > > > > It's a VBE method which supports A, B and recovery images and permits
> > > > > > > updating from SPL onwards. The idea is that you can safely update
> > > > > > > without bricking a device.
> > > > > > >
> > > > > > > >
> > > > > > > > > bootmeth in U-Boot.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Simon Glass (18):
> > > > > > > > >   sandbox: Add missing header file
> > > > > > > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > > > > > > >   gzip: Correct function comment for gunzip()
> > > > > > > >
> > > > > > > > Is this from upstream gzip code somewher?
> > > > > > >
> > > > > > > I'm not sure, actually.
> > > > > > >
> > > > > > > >
> > > > > > > > >   fdtdec: Support separate BSS for all XPL builds
> > > > > > >
> > > > > > > Needed so that VPL can use DDT
> > > > > > >
> > > > > > > > >   tiny-printf: Correct return values
> > > > > > >
> > > > > > > I can't remember, sorry.
> > > > > > >
> > > > > > > > >   tpl: Support numbered aliases in device tree
> > > > > > >
> > > > > > > The MMC needs to be accessed using its sequence number
> > > > > > >
> > > > > > > > >   ram: Support driver model in TPL
> > > > > > >
> > > > > > > That should say VPL...but again I'm not quite sure
> > > > > > >
> > > > > > > > >   serial: Support debug UART in TPL
> > > > > > >
> > > > > > > This allows debugging of the jump from TPL to VPL
> > > > > > >
> > > > > > > > >   armv8: Support not having separate BSS
> > > > > > >
> > > > > > > VPL doesn't want a separate BSS since SDRAM isn't available that early
> > > > > > >
> > > > > > > > >   arm: cache: Drop a stale comment
> > > > > > >
> > > > > > > Just something I noticed
> > > > > > >
> > > > > > > > >   arm: Fix up a stale comment in sections.c
> > > > > > >
> > > > > > > Another thing I noticed
> > > > > > >
> > > > > > > > >   mmc: Support driver model in TPL
> > > > > > >
> > > > > > > TPL needs to read VPL from MMC
> > > > > > >
> > > > > > > > >   mmc: Add more debugging for SPL
> > > > > > > > >   mmc: Log the error when init fails
> > > > > > > > >   mmc: rockchip: Log some error returns
> > > > > > > > >   mmc: rockchip: Allow clocks to be missing
> > > > > > >
> > > > > > > These all help with debugging reading of VPL from the correct MMC device
> > > > > > >
> > > > > > > > >   rockchip: mmc: Fix a missing colon
> > > > > > >
> > > > > > > Just something I noticed
> > > > > > >
> > > > > > > > >   rockchip: Provided SPL control over efuse presence
> > > > > > >
> > > > > > > We don't want this driver in TPL or VPL
> > > > > > >
> > > > > > > >
> > > > > > > > I'm not sure what most of these patches have to do with VBE?
> > > > > > >
> > > > > > > Basically there are a lot of little tweaks needed.
> > > > > >
> > > > > > Well, I think this gets back to some common feedback. You have a lot of
> > > > > > little bugfixes, which is good, but they get sprinkled in to the next
> > > > > > big series you post, which is bad and hard to review. And that leads to
> > > > > > fixes not getting merged / reviewed timely because nominally simple fix
> > > > > > A is in part 3 of a series to introduce something larger and to which
> > > > > > there is feedback to work through.
> > > > >
> > > > > I actually thought I was splitting them up quite nicely. What do you suggest?
> > > >
> > > > I would refer back to your own comments about what some of the patches
> > > > do and say that "Just something I noticed" should be a one off, and
> > > > little fixes for this-and-that should be a 1-2 part series and so on.
> > >
> > > I really don't have a great workflow for keeping track of more than
> > > 6-8 pending series and even that is sometimes a struggle. For testing
> > > I need everything applied so I have every series in a sequence applied
> > > locally.
> > >
> > > I will have a think about what can be done. I wonder how others do it?
> >
> > I believe the answer is multiple branches and merging them as needed. Or
> > what I do is make each subsequent series a branch off the previous one
> > (and set the upstream appropriately) and then rebase as needed.
> 
> OK. That is my current workflow, but it can get out of hand,
> particularly something like VBE abrec where over 100 patches in 6
> -or-so different series are needed. I suppose that is why I keep
> putting over doing the final two series...

That should make re-branching things easier? Get your whole set of
things in roughly good order then you can:
git branch vN-fixes-foo commit-hash1
git branch vM-fixes-bar commit-hash2
...
git branch -u vN-fixes-foo commit-hash1 vM-fixes-bar commit-hash2
...

And there's your series broken up and re-branched so you can have
however many branches you need to split everything up in to something
reviewable and mergeable in parts. Taking one of your branches that has
everything and rebasing that on a new upstream commit that has some
branches but not all merged should get things correct and drop the now
upstream parts.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/18] vbe: Series part D

[Simon Glass]

Hi Tom,

On Fri, 20 Sept 2024 at 18:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Sep 20, 2024 at 06:04:05PM +0200, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 20 Sept 2024 at 17:01, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Sep 20, 2024 at 09:25:53AM +0200, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 30 Aug 2024 at 03:46, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Aug 29, 2024 at 07:06:32PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 29 Aug 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Aug 28, 2024 at 04:16:55PM -0600, Simon Glass wrote:
> > > > > > > > Hi Peter,
> > > > > > > >
> > > > > > > > On Wed, 28 Aug 2024 at 03:59, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > > This includes various patches towards implementing the VBE abrec
> > > > > > > > >
> > > > > > > > > What is abrec?
> > > > > > > >
> > > > > > > > It's a VBE method which supports A, B and recovery images and permits
> > > > > > > > updating from SPL onwards. The idea is that you can safely update
> > > > > > > > without bricking a device.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > bootmeth in U-Boot.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Simon Glass (18):
> > > > > > > > > >   sandbox: Add missing header file
> > > > > > > > > >   bootstd: Add stub for bootdev_setup_for_sibling_blk()
> > > > > > > > > >   gzip: Correct function comment for gunzip()
> > > > > > > > >
> > > > > > > > > Is this from upstream gzip code somewher?
> > > > > > > >
> > > > > > > > I'm not sure, actually.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >   fdtdec: Support separate BSS for all XPL builds
> > > > > > > >
> > > > > > > > Needed so that VPL can use DDT
> > > > > > > >
> > > > > > > > > >   tiny-printf: Correct return values
> > > > > > > >
> > > > > > > > I can't remember, sorry.
> > > > > > > >
> > > > > > > > > >   tpl: Support numbered aliases in device tree
> > > > > > > >
> > > > > > > > The MMC needs to be accessed using its sequence number
> > > > > > > >
> > > > > > > > > >   ram: Support driver model in TPL
> > > > > > > >
> > > > > > > > That should say VPL...but again I'm not quite sure
> > > > > > > >
> > > > > > > > > >   serial: Support debug UART in TPL
> > > > > > > >
> > > > > > > > This allows debugging of the jump from TPL to VPL
> > > > > > > >
> > > > > > > > > >   armv8: Support not having separate BSS
> > > > > > > >
> > > > > > > > VPL doesn't want a separate BSS since SDRAM isn't available that early
> > > > > > > >
> > > > > > > > > >   arm: cache: Drop a stale comment
> > > > > > > >
> > > > > > > > Just something I noticed
> > > > > > > >
> > > > > > > > > >   arm: Fix up a stale comment in sections.c
> > > > > > > >
> > > > > > > > Another thing I noticed
> > > > > > > >
> > > > > > > > > >   mmc: Support driver model in TPL
> > > > > > > >
> > > > > > > > TPL needs to read VPL from MMC
> > > > > > > >
> > > > > > > > > >   mmc: Add more debugging for SPL
> > > > > > > > > >   mmc: Log the error when init fails
> > > > > > > > > >   mmc: rockchip: Log some error returns
> > > > > > > > > >   mmc: rockchip: Allow clocks to be missing
> > > > > > > >
> > > > > > > > These all help with debugging reading of VPL from the correct MMC device
> > > > > > > >
> > > > > > > > > >   rockchip: mmc: Fix a missing colon
> > > > > > > >
> > > > > > > > Just something I noticed
> > > > > > > >
> > > > > > > > > >   rockchip: Provided SPL control over efuse presence
> > > > > > > >
> > > > > > > > We don't want this driver in TPL or VPL
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I'm not sure what most of these patches have to do with VBE?
> > > > > > > >
> > > > > > > > Basically there are a lot of little tweaks needed.
> > > > > > >
> > > > > > > Well, I think this gets back to some common feedback. You have a lot of
> > > > > > > little bugfixes, which is good, but they get sprinkled in to the next
> > > > > > > big series you post, which is bad and hard to review. And that leads to
> > > > > > > fixes not getting merged / reviewed timely because nominally simple fix
> > > > > > > A is in part 3 of a series to introduce something larger and to which
> > > > > > > there is feedback to work through.
> > > > > >
> > > > > > I actually thought I was splitting them up quite nicely. What do you suggest?
> > > > >
> > > > > I would refer back to your own comments about what some of the patches
> > > > > do and say that "Just something I noticed" should be a one off, and
> > > > > little fixes for this-and-that should be a 1-2 part series and so on.
> > > >
> > > > I really don't have a great workflow for keeping track of more than
> > > > 6-8 pending series and even that is sometimes a struggle. For testing
> > > > I need everything applied so I have every series in a sequence applied
> > > > locally.
> > > >
> > > > I will have a think about what can be done. I wonder how others do it?
> > >
> > > I believe the answer is multiple branches and merging them as needed. Or
> > > what I do is make each subsequent series a branch off the previous one
> > > (and set the upstream appropriately) and then rebase as needed.
> >
> > OK. That is my current workflow, but it can get out of hand,
> > particularly something like VBE abrec where over 100 patches in 6
> > -or-so different series are needed. I suppose that is why I keep
> > putting over doing the final two series...
>
> That should make re-branching things easier? Get your whole set of
> things in roughly good order then you can:
> git branch vN-fixes-foo commit-hash1
> git branch vM-fixes-bar commit-hash2
> ...
> git branch -u vN-fixes-foo commit-hash1 vM-fixes-bar commit-hash2
> ...
>
> And there's your series broken up and re-branched so you can have
> however many branches you need to split everything up in to something
> reviewable and mergeable in parts. Taking one of your branches that has
> everything and rebasing that on a new upstream commit that has some
> branches but not all merged should get things correct and drop the now
> upstream parts.

Yes, that's what I do. But I have no tooling to manage it, so it is
quite error-prone. I have a manual note of the series order, but as I
say, if it gets too large I just lose track.

Regards,
Simon