distroboot: Fix ubifs

Fix multiple issues in ubifs distroboot code:

U-Boot supports attaching only one MTD device as UBI at the time. So
always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
command attach MTD device always as UBI device ubi0.

Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
Distroboot scripts require ${bootfstype} variable to be properly set and it
is already set for all other boot types.

Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
device does not have partitions, but has volumes. Distroboot scripts
require something to be set in ${distro_bootpart} variable, so set it to
the UBI volume which is currently mounted by ubifs.

Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
differs from the other partition code that it requires "ubi" prefix before
number.

Explicitly unmount ubifs volume after loading all data from it. This allows
to detach UBI device from MTD device.

Move definition of MTD device with UBI and UBI volume with ubifs filesystem
from global env variables ${bootubipart} and ${bootubivol} into the
distroboot "func" macro, defined in board include config files. UBIFS
distroboot macros then set ${bootubipart} and ${bootubivol} local variables
for compatibility with existing distroboot scripts.

This last change allows to define more UBIFS target devices and make it
clear what is boot MTD/UBI device.

All board include config files are adjusted to use this new scheme of
specifying boot MTD/UBI device.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
CI test passed on https://github.com/u-boot/u-boot/pull/179
---
 include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
 include/configs/am335x_guardian.h  |  3 +--
 include/configs/colibri-imx6ull.h  |  1 -
 include/configs/colibri_imx7.h     |  1 -
 include/configs/kontron-sl-mx6ul.h |  2 +-
 include/configs/mys_6ulx.h         |  2 +-
 include/configs/npi_imx6ull.h      |  2 +-
 include/configs/omap3_beagle.h     |  4 +---
 include/configs/omap3_evm.h        |  4 +---
 include/configs/pcl063.h           |  2 +-
 include/configs/stm32mp15_common.h |  2 +-
 include/configs/uniphier.h         |  2 +-
 12 files changed, 25 insertions(+), 27 deletions(-)

On Tuesday 31 May 2022 10:32:36 Pali Rohár wrote:
> Fix multiple issues in ubifs distroboot code:
> 
> U-Boot supports attaching only one MTD device as UBI at the time. So
> always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> command attach MTD device always as UBI device ubi0.
> 
> Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> Distroboot scripts require ${bootfstype} variable to be properly set and it
> is already set for all other boot types.
> 
> Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> device does not have partitions, but has volumes. Distroboot scripts
> require something to be set in ${distro_bootpart} variable, so set it to
> the UBI volume which is currently mounted by ubifs.
> 
> Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> differs from the other partition code that it requires "ubi" prefix before
> number.
> 
> Explicitly unmount ubifs volume after loading all data from it. This allows
> to detach UBI device from MTD device.
> 
> Move definition of MTD device with UBI and UBI volume with ubifs filesystem
> from global env variables ${bootubipart} and ${bootubivol} into the
> distroboot "func" macro, defined in board include config files. UBIFS
> distroboot macros then set ${bootubipart} and ${bootubivol} local variables
> for compatibility with existing distroboot scripts.
> 
> This last change allows to define more UBIFS target devices and make it
> clear what is boot MTD/UBI device.
> 
> All board include config files are adjusted to use this new scheme of
> specifying boot MTD/UBI device.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> CI test passed on https://github.com/u-boot/u-boot/pull/179
> ---

PING?

>  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
>  include/configs/am335x_guardian.h  |  3 +--
>  include/configs/colibri-imx6ull.h  |  1 -
>  include/configs/colibri_imx7.h     |  1 -
>  include/configs/kontron-sl-mx6ul.h |  2 +-
>  include/configs/mys_6ulx.h         |  2 +-
>  include/configs/npi_imx6ull.h      |  2 +-
>  include/configs/omap3_beagle.h     |  4 +---
>  include/configs/omap3_evm.h        |  4 +---
>  include/configs/pcl063.h           |  2 +-
>  include/configs/stm32mp15_common.h |  2 +-
>  include/configs/uniphier.h         |  2 +-
>  12 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index c55023889cab..c6e9c497413d 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -70,18 +70,23 @@
>  #ifdef CONFIG_CMD_UBIFS
>  #define BOOTENV_SHARED_UBIFS \
>  	"ubifs_boot=" \
> -		"env exists bootubipart || " \
> -			"env set bootubipart UBI; " \
> -		"env exists bootubivol || " \
> -			"env set bootubivol boot; " \
>  		"if ubi part ${bootubipart} && " \
> -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> +			"ubifsmount ubi0:${bootubivol}; " \
>  		"then " \
>  			"devtype=ubi; " \
> +			"devnum=ubi0; " \
> +			"bootfstype=ubifs; " \
> +			"distro_bootpart=${bootubivol}; " \
>  			"run scan_dev_for_boot; " \
> +			"ubifsumount; " \
>  		"fi\0"
> -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart, bootubivol) \
> +	"bootcmd_ubifs" #instance "=" \
> +		"bootubipart=" #bootubipart "; " \
> +		"bootubivol=" #bootubivol "; " \
> +		"run ubifs_boot\0"
> +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance, bootubipart, bootubivol) \
> +	#devtypel #instance " "
>  #else
>  #define BOOTENV_SHARED_UBIFS
>  #define BOOTENV_DEV_UBIFS \
> @@ -411,13 +416,13 @@
>  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
>  #endif
>  
> -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
>  #define BOOTENV_BOOT_TARGETS \
>  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
>  
> -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
>  #define BOOTENV \
>  	BOOTENV_SHARED_HOST \
>  	BOOTENV_SHARED_MMC \
> diff --git a/include/configs/am335x_guardian.h b/include/configs/am335x_guardian.h
> index b92703205cde..340715dad5c6 100644
> --- a/include/configs/am335x_guardian.h
> +++ b/include/configs/am335x_guardian.h
> @@ -29,7 +29,7 @@
>  	"ramdisk_addr_r=0x88080000\0" \
>  
>  #define BOOT_TARGET_DEVICES(func) \
> -	func(UBIFS, ubifs, 0)
> +	func(UBIFS, ubifs, 0, UBI, rootfs)
>  
>  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE ".dtb\0"
>  
> @@ -54,7 +54,6 @@
>  	GUARDIAN_DEFAULT_PROD_ENV \
>  	"autoload=no\0" \
>  	"backlight_brightness=50\0" \
> -	"bootubivol=rootfs\0" \
>  	"distro_bootcmd=" \
>  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
>  		"setenv ethact usb_ether; " \
> diff --git a/include/configs/colibri-imx6ull.h b/include/configs/colibri-imx6ull.h
> index 9e5212acb2ee..a0a0e1767fe0 100644
> --- a/include/configs/colibri-imx6ull.h
> +++ b/include/configs/colibri-imx6ull.h
> @@ -91,7 +91,6 @@
>  	UBI_BOOTCMD \
>  	UBOOT_UPDATE \
>  	"boot_script_dhcp=boot.scr\0" \
> -	"bootubipart=ubi\0" \
>  	"console=ttymxc0\0" \
>  	"defargs=user_debug=30\0" \
>  	"fdt_board=eval-v3\0" \
> diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> index 3dba7bcef258..3cad17777975 100644
> --- a/include/configs/colibri_imx7.h
> +++ b/include/configs/colibri_imx7.h
> @@ -131,7 +131,6 @@
>  	UBOOT_UPDATE \
>  	"boot_file=zImage\0" \
>  	"boot_script_dhcp=boot.scr\0" \
> -	"bootubipart=ubi\0" \
>  	"console=ttymxc0\0" \
>  	"defargs=\0" \
>  	"fdt_board=eval-v3\0" \
> diff --git a/include/configs/kontron-sl-mx6ul.h b/include/configs/kontron-sl-mx6ul.h
> index 7bc402d578e8..b4808d2bbf75 100644
> --- a/include/configs/kontron-sl-mx6ul.h
> +++ b/include/configs/kontron-sl-mx6ul.h
> @@ -45,7 +45,7 @@
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 1) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(USB, usb, 0) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
> diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> index 6801fc109eae..663820177a3e 100644
> --- a/include/configs/mys_6ulx.h
> +++ b/include/configs/mys_6ulx.h
> @@ -59,7 +59,7 @@
>  
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
>  
> diff --git a/include/configs/npi_imx6ull.h b/include/configs/npi_imx6ull.h
> index c250fa650603..ebb887544e08 100644
> --- a/include/configs/npi_imx6ull.h
> +++ b/include/configs/npi_imx6ull.h
> @@ -67,7 +67,7 @@
>  
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
>  
> diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
> index 158773acedb9..f5da08cf3359 100644
> --- a/include/configs/omap3_beagle.h
> +++ b/include/configs/omap3_beagle.h
> @@ -65,7 +65,7 @@
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
>  	func(LEGACY_MMC, legacy_mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
>  	func(NAND, nand, 0)
>  
>  #else /* !CONFIG_MTD_RAW_NAND */
> @@ -87,8 +87,6 @@
>  	"bootenv=uEnv.txt\0" \
>  	"bootfile=zImage\0" \
>  	"bootpart=0:2\0" \
> -	"bootubivol=rootfs\0" \
> -	"bootubipart=rootfs\0" \
>  	"usbtty=cdc_acm\0" \
>  	"mpurate=auto\0" \
>  	"buddy=none\0" \
> diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> index eeb9ef8c741a..cc98e03096ab 100644
> --- a/include/configs/omap3_evm.h
> +++ b/include/configs/omap3_evm.h
> @@ -60,7 +60,7 @@
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
>  	func(LEGACY_MMC, legacy_mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
>  	func(NAND, nand, 0)
>  
>  #else /* !CONFIG_MTD_RAW_NAND */
> @@ -88,8 +88,6 @@
>  	"bootenv=uEnv.txt\0" \
>  	"bootfile=zImage\0" \
>  	"bootpart=0:2\0" \
> -	"bootubivol=rootfs\0" \
> -	"bootubipart=rootfs\0" \
>  	"optargs=\0" \
>  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
>  	"nandrootfstype=ubifs rootwait\0" \
> diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> index 31b7d07a24cd..c3f7e7eb2c4b 100644
> --- a/include/configs/pcl063.h
> +++ b/include/configs/pcl063.h
> @@ -71,7 +71,7 @@
>  
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
>  
> diff --git a/include/configs/stm32mp15_common.h b/include/configs/stm32mp15_common.h
> index 6b40cdb01779..68ea56e69c98 100644
> --- a/include/configs/stm32mp15_common.h
> +++ b/include/configs/stm32mp15_common.h
> @@ -77,7 +77,7 @@
>  #endif
>  
>  #ifdef CONFIG_CMD_UBIFS
> -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
>  #else
>  #define BOOT_TARGET_UBIFS(func)
>  #endif
> diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> index f813f88cdd7a..640a29067d85 100644
> --- a/include/configs/uniphier.h
> +++ b/include/configs/uniphier.h
> @@ -20,7 +20,7 @@
>  #endif
>  
>  #ifdef CONFIG_CMD_UBIFS
> -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
>  #else
>  #define BOOT_TARGET_DEVICE_UBIFS(func)
>  #endif
> -- 
> 2.20.1
>

Am 23.06.22 um 18:09 schrieb Pali Rohár:
> On Tuesday 31 May 2022 10:32:36 Pali Rohár wrote:
>> Fix multiple issues in ubifs distroboot code:
>>
>> U-Boot supports attaching only one MTD device as UBI at the time. So
>> always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
>> ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
>> command attach MTD device always as UBI device ubi0.
>>
>> Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
>> Distroboot scripts require ${bootfstype} variable to be properly set and it
>> is already set for all other boot types.
>>
>> Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
>> device does not have partitions, but has volumes. Distroboot scripts
>> require something to be set in ${distro_bootpart} variable, so set it to
>> the UBI volume which is currently mounted by ubifs.
>>
>> Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
>> differs from the other partition code that it requires "ubi" prefix before
>> number.
>>
>> Explicitly unmount ubifs volume after loading all data from it. This allows
>> to detach UBI device from MTD device.
>>
>> Move definition of MTD device with UBI and UBI volume with ubifs filesystem
>> from global env variables ${bootubipart} and ${bootubivol} into the
>> distroboot "func" macro, defined in board include config files. UBIFS
>> distroboot macros then set ${bootubipart} and ${bootubivol} local variables
>> for compatibility with existing distroboot scripts.
>>
>> This last change allows to define more UBIFS target devices and make it
>> clear what is boot MTD/UBI device.
>>
>> All board include config files are adjusted to use this new scheme of
>> specifying boot MTD/UBI device.
>>
>> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> PING?
> 

Sorry, I currently don't have the time to properly review and/or test
this. Though, in general the changes look good and I can at least provide:

Acked-by: Frieder Schrempf <frieder.schrempf@kontron.de>

I think it would help to split this up in smaller chunks, so people can
review it more easily...

Hello Pali,

had a look at this patch, and have some questions.  See below.

Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Rohár:
> Fix multiple issues in ubifs distroboot code:
> 
> U-Boot supports attaching only one MTD device as UBI at the time. So
> always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> command attach MTD device always as UBI device ubi0.

Agreed.

> Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> Distroboot scripts require ${bootfstype} variable to be properly set and it
> is already set for all other boot types.

From grepping through u-boot source, I can not quite follow.  Where is that 
variable set and where is it used?

> Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> device does not have partitions, but has volumes. Distroboot scripts
> require something to be set in ${distro_bootpart} variable, so set it to
> the UBI volume which is currently mounted by ubifs.
> 
> Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> differs from the other partition code that it requires "ubi" prefix before
> number.

Okay.

> Explicitly unmount ubifs volume after loading all data from it. This allows
> to detach UBI device from MTD device.

Okay.

> Move definition of MTD device with UBI and UBI volume with ubifs filesystem
> from global env variables ${bootubipart} and ${bootubivol} into the
> distroboot "func" macro, defined in board include config files. UBIFS
> distroboot macros then set ${bootubipart} and ${bootubivol} local variables
> for compatibility with existing distroboot scripts.

This might be problematic.  For a downstream board we use the 'bootubivol' env 
variable to switch between different volumes for update purposes.  Means on 
firmware update we modify U-Boot environment from Linux (fw_setenv) after 
successfully updating the inactive volume.

> This last change allows to define more UBIFS target devices and make it
> clear what is boot MTD/UBI device.

How would one switch the default UBI volume to be booted from then?

Greets
Alex

> All board include config files are adjusted to use this new scheme of
> specifying boot MTD/UBI device.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> CI test passed on https://github.com/u-boot/u-boot/pull/179
> ---
>  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
>  include/configs/am335x_guardian.h  |  3 +--
>  include/configs/colibri-imx6ull.h  |  1 -
>  include/configs/colibri_imx7.h     |  1 -
>  include/configs/kontron-sl-mx6ul.h |  2 +-
>  include/configs/mys_6ulx.h         |  2 +-
>  include/configs/npi_imx6ull.h      |  2 +-
>  include/configs/omap3_beagle.h     |  4 +---
>  include/configs/omap3_evm.h        |  4 +---
>  include/configs/pcl063.h           |  2 +-
>  include/configs/stm32mp15_common.h |  2 +-
>  include/configs/uniphier.h         |  2 +-
>  12 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/include/config_distro_bootcmd.h
> b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -70,18 +70,23 @@
>  #ifdef CONFIG_CMD_UBIFS
>  #define BOOTENV_SHARED_UBIFS \
>  	"ubifs_boot=" \
> -		"env exists bootubipart || " \
> -			"env set bootubipart UBI; " \
> -		"env exists bootubivol || " \
> -			"env set bootubivol boot; " \
>  		"if ubi part ${bootubipart} && " \
> -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> +			"ubifsmount ubi0:${bootubivol}; " \
>  		"then " \
>  			"devtype=ubi; " \
> +			"devnum=ubi0; " \
> +			"bootfstype=ubifs; " \
> +			"distro_bootpart=${bootubivol}; " \
>  			"run scan_dev_for_boot; " \
> +			"ubifsumount; " \
>  		"fi\0"
> -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart,
> bootubivol) \ +	"bootcmd_ubifs" #instance "=" \
> +		"bootubipart=" #bootubipart "; " \
> +		"bootubivol=" #bootubivol "; " \
> +		"run ubifs_boot\0"
> +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance, bootubipart,
> bootubivol) \ +	#devtypel #instance " "
>  #else
>  #define BOOTENV_SHARED_UBIFS
>  #define BOOTENV_DEV_UBIFS \
> @@ -411,13 +416,13 @@
>  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
>  #endif
> 
> -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
>  #define BOOTENV_BOOT_TARGETS \
>  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
> 
> -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
>  #define BOOTENV \
>  	BOOTENV_SHARED_HOST \
>  	BOOTENV_SHARED_MMC \
> diff --git a/include/configs/am335x_guardian.h
> b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6 100644
> --- a/include/configs/am335x_guardian.h
> +++ b/include/configs/am335x_guardian.h
> @@ -29,7 +29,7 @@
>  	"ramdisk_addr_r=0x88080000\0" \
> 
>  #define BOOT_TARGET_DEVICES(func) \
> -	func(UBIFS, ubifs, 0)
> +	func(UBIFS, ubifs, 0, UBI, rootfs)
> 
>  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE
> ".dtb\0"
> 
> @@ -54,7 +54,6 @@
>  	GUARDIAN_DEFAULT_PROD_ENV \
>  	"autoload=no\0" \
>  	"backlight_brightness=50\0" \
> -	"bootubivol=rootfs\0" \
>  	"distro_bootcmd=" \
>  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
>  		"setenv ethact usb_ether; " \
> diff --git a/include/configs/colibri-imx6ull.h
> b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0 100644
> --- a/include/configs/colibri-imx6ull.h
> +++ b/include/configs/colibri-imx6ull.h
> @@ -91,7 +91,6 @@
>  	UBI_BOOTCMD \
>  	UBOOT_UPDATE \
>  	"boot_script_dhcp=boot.scr\0" \
> -	"bootubipart=ubi\0" \
>  	"console=ttymxc0\0" \
>  	"defargs=user_debug=30\0" \
>  	"fdt_board=eval-v3\0" \
> diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> index 3dba7bcef258..3cad17777975 100644
> --- a/include/configs/colibri_imx7.h
> +++ b/include/configs/colibri_imx7.h
> @@ -131,7 +131,6 @@
>  	UBOOT_UPDATE \
>  	"boot_file=zImage\0" \
>  	"boot_script_dhcp=boot.scr\0" \
> -	"bootubipart=ubi\0" \
>  	"console=ttymxc0\0" \
>  	"defargs=\0" \
>  	"fdt_board=eval-v3\0" \
> diff --git a/include/configs/kontron-sl-mx6ul.h
> b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75
> 100644
> --- a/include/configs/kontron-sl-mx6ul.h
> +++ b/include/configs/kontron-sl-mx6ul.h
> @@ -45,7 +45,7 @@
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 1) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(USB, usb, 0) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
> diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> index 6801fc109eae..663820177a3e 100644
> --- a/include/configs/mys_6ulx.h
> +++ b/include/configs/mys_6ulx.h
> @@ -59,7 +59,7 @@
> 
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
> 
> diff --git a/include/configs/npi_imx6ull.h b/include/configs/npi_imx6ull.h
> index c250fa650603..ebb887544e08 100644
> --- a/include/configs/npi_imx6ull.h
> +++ b/include/configs/npi_imx6ull.h
> @@ -67,7 +67,7 @@
> 
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
> 
> diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
> index 158773acedb9..f5da08cf3359 100644
> --- a/include/configs/omap3_beagle.h
> +++ b/include/configs/omap3_beagle.h
> @@ -65,7 +65,7 @@
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
>  	func(LEGACY_MMC, legacy_mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
>  	func(NAND, nand, 0)
> 
>  #else /* !CONFIG_MTD_RAW_NAND */
> @@ -87,8 +87,6 @@
>  	"bootenv=uEnv.txt\0" \
>  	"bootfile=zImage\0" \
>  	"bootpart=0:2\0" \
> -	"bootubivol=rootfs\0" \
> -	"bootubipart=rootfs\0" \
>  	"usbtty=cdc_acm\0" \
>  	"mpurate=auto\0" \
>  	"buddy=none\0" \
> diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> index eeb9ef8c741a..cc98e03096ab 100644
> --- a/include/configs/omap3_evm.h
> +++ b/include/configs/omap3_evm.h
> @@ -60,7 +60,7 @@
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
>  	func(LEGACY_MMC, legacy_mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
>  	func(NAND, nand, 0)
> 
>  #else /* !CONFIG_MTD_RAW_NAND */
> @@ -88,8 +88,6 @@
>  	"bootenv=uEnv.txt\0" \
>  	"bootfile=zImage\0" \
>  	"bootpart=0:2\0" \
> -	"bootubivol=rootfs\0" \
> -	"bootubipart=rootfs\0" \
>  	"optargs=\0" \
>  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
>  	"nandrootfstype=ubifs rootwait\0" \
> diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> index 31b7d07a24cd..c3f7e7eb2c4b 100644
> --- a/include/configs/pcl063.h
> +++ b/include/configs/pcl063.h
> @@ -71,7 +71,7 @@
> 
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 0) \
> -	func(UBIFS, ubifs, 0) \
> +	func(UBIFS, ubifs, 0, UBI, boot) \
>  	func(PXE, pxe, na) \
>  	func(DHCP, dhcp, na)
> 
> diff --git a/include/configs/stm32mp15_common.h
> b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98
> 100644
> --- a/include/configs/stm32mp15_common.h
> +++ b/include/configs/stm32mp15_common.h
> @@ -77,7 +77,7 @@
>  #endif
> 
>  #ifdef CONFIG_CMD_UBIFS
> -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
>  #else
>  #define BOOT_TARGET_UBIFS(func)
>  #endif
> diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> index f813f88cdd7a..640a29067d85 100644
> --- a/include/configs/uniphier.h
> +++ b/include/configs/uniphier.h
> @@ -20,7 +20,7 @@
>  #endif
> 
>  #ifdef CONFIG_CMD_UBIFS
> -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
>  #else
>  #define BOOT_TARGET_DEVICE_UBIFS(func)
>  #endif

Hello!

On Wednesday 29 June 2022 15:36:57 Alexander Dahl wrote:
> Hello Pali,
> 
> had a look at this patch, and have some questions.  See below.
> 
> Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Rohár:
> > Fix multiple issues in ubifs distroboot code:
> > 
> > U-Boot supports attaching only one MTD device as UBI at the time. So
> > always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> > ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> > command attach MTD device always as UBI device ubi0.
> 
> Agreed.
> 
> > Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> > Distroboot scripts require ${bootfstype} variable to be properly set and it
> > is already set for all other boot types.
> 
> From grepping through u-boot source, I can not quite follow.  Where is that 
> variable set and where is it used?

For non-ubifs boot targets, env variable ${bootfstype} is set by
scan_dev_for_boot_part shell fragment. Then it calls user boot script
which can use ${bootfstype} variable. In most cases it is used for
constructing cmdline for kernel, mainly as "roofstype=${bootfstype}".

> > Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> > device does not have partitions, but has volumes. Distroboot scripts
> > require something to be set in ${distro_bootpart} variable, so set it to
> > the UBI volume which is currently mounted by ubifs.
> > 
> > Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> > differs from the other partition code that it requires "ubi" prefix before
> > number.
> 
> Okay.
> 
> > Explicitly unmount ubifs volume after loading all data from it. This allows
> > to detach UBI device from MTD device.
> 
> Okay.
> 
> > Move definition of MTD device with UBI and UBI volume with ubifs filesystem
> > from global env variables ${bootubipart} and ${bootubivol} into the
> > distroboot "func" macro, defined in board include config files. UBIFS
> > distroboot macros then set ${bootubipart} and ${bootubivol} local variables
> > for compatibility with existing distroboot scripts.
> 
> This might be problematic.  For a downstream board we use the 'bootubivol' env 
> variable to switch between different volumes for update purposes.  Means on 
> firmware update we modify U-Boot environment from Linux (fw_setenv) after 
> successfully updating the inactive volume.

Interesting... I even did not thought about such use case as it is not
possible to do it with other boot filesystems.

> > This last change allows to define more UBIFS target devices and make it
> > clear what is boot MTD/UBI device.
> 
> How would one switch the default UBI volume to be booted from then?

With this my change, it is probably not easily possible. One option is
to define more boot targets, e.g.:

  func(UBIFS, ubifs, 0, UBI, volume0)
  func(UBIFS, ubifs, 1, UBI, volume1)

And then change env ${boottargets} from ubifs0 to ubifs1, which can be
done via fs_setenv.

As with this change is finally possible to specify more ubifs targets
in BOOT_TARGET_DEVICES() u-boot macro.

This would be same as switching between other u-boot distroboot targets.

...

But I can understand that you want to have this (probably undocumented)
behavior of switching UBI volume via env variable ${bootubivol}.

So what about allowing both configuration in U-Boot, with stored boot
volume in ENV and with hardcoded boot volume in config file?

And which / how many boards use this functionality?

> Greets
> Alex
> 
> > All board include config files are adjusted to use this new scheme of
> > specifying boot MTD/UBI device.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> > CI test passed on https://github.com/u-boot/u-boot/pull/179
> > ---
> >  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
> >  include/configs/am335x_guardian.h  |  3 +--
> >  include/configs/colibri-imx6ull.h  |  1 -
> >  include/configs/colibri_imx7.h     |  1 -
> >  include/configs/kontron-sl-mx6ul.h |  2 +-
> >  include/configs/mys_6ulx.h         |  2 +-
> >  include/configs/npi_imx6ull.h      |  2 +-
> >  include/configs/omap3_beagle.h     |  4 +---
> >  include/configs/omap3_evm.h        |  4 +---
> >  include/configs/pcl063.h           |  2 +-
> >  include/configs/stm32mp15_common.h |  2 +-
> >  include/configs/uniphier.h         |  2 +-
> >  12 files changed, 25 insertions(+), 27 deletions(-)
> > 
> > diff --git a/include/config_distro_bootcmd.h
> > b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d 100644
> > --- a/include/config_distro_bootcmd.h
> > +++ b/include/config_distro_bootcmd.h
> > @@ -70,18 +70,23 @@
> >  #ifdef CONFIG_CMD_UBIFS
> >  #define BOOTENV_SHARED_UBIFS \
> >  	"ubifs_boot=" \
> > -		"env exists bootubipart || " \
> > -			"env set bootubipart UBI; " \
> > -		"env exists bootubivol || " \
> > -			"env set bootubivol boot; " \
> >  		"if ubi part ${bootubipart} && " \
> > -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> > +			"ubifsmount ubi0:${bootubivol}; " \
> >  		"then " \
> >  			"devtype=ubi; " \
> > +			"devnum=ubi0; " \
> > +			"bootfstype=ubifs; " \
> > +			"distro_bootpart=${bootubivol}; " \
> >  			"run scan_dev_for_boot; " \
> > +			"ubifsumount; " \
> >  		"fi\0"
> > -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> > -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> > +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart,
> > bootubivol) \ +	"bootcmd_ubifs" #instance "=" \
> > +		"bootubipart=" #bootubipart "; " \
> > +		"bootubivol=" #bootubivol "; " \
> > +		"run ubifs_boot\0"
> > +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance, bootubipart,
> > bootubivol) \ +	#devtypel #instance " "
> >  #else
> >  #define BOOTENV_SHARED_UBIFS
> >  #define BOOTENV_DEV_UBIFS \
> > @@ -411,13 +416,13 @@
> >  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
> >  #endif
> > 
> > -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> > -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> > +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> > +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
> >  #define BOOTENV_BOOT_TARGETS \
> >  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
> > 
> > -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> > -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> > +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> > +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
> >  #define BOOTENV \
> >  	BOOTENV_SHARED_HOST \
> >  	BOOTENV_SHARED_MMC \
> > diff --git a/include/configs/am335x_guardian.h
> > b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6 100644
> > --- a/include/configs/am335x_guardian.h
> > +++ b/include/configs/am335x_guardian.h
> > @@ -29,7 +29,7 @@
> >  	"ramdisk_addr_r=0x88080000\0" \
> > 
> >  #define BOOT_TARGET_DEVICES(func) \
> > -	func(UBIFS, ubifs, 0)
> > +	func(UBIFS, ubifs, 0, UBI, rootfs)
> > 
> >  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE
> > ".dtb\0"
> > 
> > @@ -54,7 +54,6 @@
> >  	GUARDIAN_DEFAULT_PROD_ENV \
> >  	"autoload=no\0" \
> >  	"backlight_brightness=50\0" \
> > -	"bootubivol=rootfs\0" \
> >  	"distro_bootcmd=" \
> >  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
> >  		"setenv ethact usb_ether; " \
> > diff --git a/include/configs/colibri-imx6ull.h
> > b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0 100644
> > --- a/include/configs/colibri-imx6ull.h
> > +++ b/include/configs/colibri-imx6ull.h
> > @@ -91,7 +91,6 @@
> >  	UBI_BOOTCMD \
> >  	UBOOT_UPDATE \
> >  	"boot_script_dhcp=boot.scr\0" \
> > -	"bootubipart=ubi\0" \
> >  	"console=ttymxc0\0" \
> >  	"defargs=user_debug=30\0" \
> >  	"fdt_board=eval-v3\0" \
> > diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> > index 3dba7bcef258..3cad17777975 100644
> > --- a/include/configs/colibri_imx7.h
> > +++ b/include/configs/colibri_imx7.h
> > @@ -131,7 +131,6 @@
> >  	UBOOT_UPDATE \
> >  	"boot_file=zImage\0" \
> >  	"boot_script_dhcp=boot.scr\0" \
> > -	"bootubipart=ubi\0" \
> >  	"console=ttymxc0\0" \
> >  	"defargs=\0" \
> >  	"fdt_board=eval-v3\0" \
> > diff --git a/include/configs/kontron-sl-mx6ul.h
> > b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75
> > 100644
> > --- a/include/configs/kontron-sl-mx6ul.h
> > +++ b/include/configs/kontron-sl-mx6ul.h
> > @@ -45,7 +45,7 @@
> >  #define BOOT_TARGET_DEVICES(func) \
> >  	func(MMC, mmc, 1) \
> >  	func(MMC, mmc, 0) \
> > -	func(UBIFS, ubifs, 0) \
> > +	func(UBIFS, ubifs, 0, UBI, boot) \
> >  	func(USB, usb, 0) \
> >  	func(PXE, pxe, na) \
> >  	func(DHCP, dhcp, na)
> > diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> > index 6801fc109eae..663820177a3e 100644
> > --- a/include/configs/mys_6ulx.h
> > +++ b/include/configs/mys_6ulx.h
> > @@ -59,7 +59,7 @@
> > 
> >  #define BOOT_TARGET_DEVICES(func) \
> >  	func(MMC, mmc, 0) \
> > -	func(UBIFS, ubifs, 0) \
> > +	func(UBIFS, ubifs, 0, UBI, boot) \
> >  	func(PXE, pxe, na) \
> >  	func(DHCP, dhcp, na)
> > 
> > diff --git a/include/configs/npi_imx6ull.h b/include/configs/npi_imx6ull.h
> > index c250fa650603..ebb887544e08 100644
> > --- a/include/configs/npi_imx6ull.h
> > +++ b/include/configs/npi_imx6ull.h
> > @@ -67,7 +67,7 @@
> > 
> >  #define BOOT_TARGET_DEVICES(func) \
> >  	func(MMC, mmc, 0) \
> > -	func(UBIFS, ubifs, 0) \
> > +	func(UBIFS, ubifs, 0, UBI, boot) \
> >  	func(PXE, pxe, na) \
> >  	func(DHCP, dhcp, na)
> > 
> > diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
> > index 158773acedb9..f5da08cf3359 100644
> > --- a/include/configs/omap3_beagle.h
> > +++ b/include/configs/omap3_beagle.h
> > @@ -65,7 +65,7 @@
> >  #define BOOT_TARGET_DEVICES(func) \
> >  	func(MMC, mmc, 0) \
> >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > -	func(UBIFS, ubifs, 0) \
> > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> >  	func(NAND, nand, 0)
> > 
> >  #else /* !CONFIG_MTD_RAW_NAND */
> > @@ -87,8 +87,6 @@
> >  	"bootenv=uEnv.txt\0" \
> >  	"bootfile=zImage\0" \
> >  	"bootpart=0:2\0" \
> > -	"bootubivol=rootfs\0" \
> > -	"bootubipart=rootfs\0" \
> >  	"usbtty=cdc_acm\0" \
> >  	"mpurate=auto\0" \
> >  	"buddy=none\0" \
> > diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> > index eeb9ef8c741a..cc98e03096ab 100644
> > --- a/include/configs/omap3_evm.h
> > +++ b/include/configs/omap3_evm.h
> > @@ -60,7 +60,7 @@
> >  #define BOOT_TARGET_DEVICES(func) \
> >  	func(MMC, mmc, 0) \
> >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > -	func(UBIFS, ubifs, 0) \
> > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> >  	func(NAND, nand, 0)
> > 
> >  #else /* !CONFIG_MTD_RAW_NAND */
> > @@ -88,8 +88,6 @@
> >  	"bootenv=uEnv.txt\0" \
> >  	"bootfile=zImage\0" \
> >  	"bootpart=0:2\0" \
> > -	"bootubivol=rootfs\0" \
> > -	"bootubipart=rootfs\0" \
> >  	"optargs=\0" \
> >  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
> >  	"nandrootfstype=ubifs rootwait\0" \
> > diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> > index 31b7d07a24cd..c3f7e7eb2c4b 100644
> > --- a/include/configs/pcl063.h
> > +++ b/include/configs/pcl063.h
> > @@ -71,7 +71,7 @@
> > 
> >  #define BOOT_TARGET_DEVICES(func) \
> >  	func(MMC, mmc, 0) \
> > -	func(UBIFS, ubifs, 0) \
> > +	func(UBIFS, ubifs, 0, UBI, boot) \
> >  	func(PXE, pxe, na) \
> >  	func(DHCP, dhcp, na)
> > 
> > diff --git a/include/configs/stm32mp15_common.h
> > b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98
> > 100644
> > --- a/include/configs/stm32mp15_common.h
> > +++ b/include/configs/stm32mp15_common.h
> > @@ -77,7 +77,7 @@
> >  #endif
> > 
> >  #ifdef CONFIG_CMD_UBIFS
> > -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> > +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
> >  #else
> >  #define BOOT_TARGET_UBIFS(func)
> >  #endif
> > diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> > index f813f88cdd7a..640a29067d85 100644
> > --- a/include/configs/uniphier.h
> > +++ b/include/configs/uniphier.h
> > @@ -20,7 +20,7 @@
> >  #endif
> > 
> >  #ifdef CONFIG_CMD_UBIFS
> > -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> > +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
> >  #else
> >  #define BOOT_TARGET_DEVICE_UBIFS(func)
> >  #endif
> 
> 
> 
>

Hello,

Am Mittwoch, 29. Juni 2022, 15:55:27 CEST schrieb Pali Rohár:
> Hello!
> 
> On Wednesday 29 June 2022 15:36:57 Alexander Dahl wrote:
> > Hello Pali,
> > 
> > had a look at this patch, and have some questions.  See below.
> > 
> > Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Rohár:
> > > Fix multiple issues in ubifs distroboot code:
> > > 
> > > U-Boot supports attaching only one MTD device as UBI at the time. So
> > > always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> > > ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> > > command attach MTD device always as UBI device ubi0.
> > 
> > Agreed.
> > 
> > > Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> > > Distroboot scripts require ${bootfstype} variable to be properly set and
> > > it
> > > is already set for all other boot types.
> > 
> > From grepping through u-boot source, I can not quite follow.  Where is
> > that
> > variable set and where is it used?
> 
> For non-ubifs boot targets, env variable ${bootfstype} is set by
> scan_dev_for_boot_part shell fragment. Then it calls user boot script
> which can use ${bootfstype} variable. In most cases it is used for
> constructing cmdline for kernel, mainly as "roofstype=${bootfstype}".

So this is no variable stored in env, but used in scripts only, right? 
Especially in scripts not part of U-Boot itself?  Maybe this should be 
documented somewhere then?

> > > Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> > > device does not have partitions, but has volumes. Distroboot scripts
> > > require something to be set in ${distro_bootpart} variable, so set it to
> > > the UBI volume which is currently mounted by ubifs.
> > > 
> > > Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> > > differs from the other partition code that it requires "ubi" prefix
> > > before
> > > number.
> > 
> > Okay.
> > 
> > > Explicitly unmount ubifs volume after loading all data from it. This
> > > allows
> > > to detach UBI device from MTD device.
> > 
> > Okay.
> > 
> > > Move definition of MTD device with UBI and UBI volume with ubifs
> > > filesystem
> > > from global env variables ${bootubipart} and ${bootubivol} into the
> > > distroboot "func" macro, defined in board include config files. UBIFS
> > > distroboot macros then set ${bootubipart} and ${bootubivol} local
> > > variables
> > > for compatibility with existing distroboot scripts.
> > 
> > This might be problematic.  For a downstream board we use the 'bootubivol'
> > env variable to switch between different volumes for update purposes. 
> > Means on firmware update we modify U-Boot environment from Linux
> > (fw_setenv) after successfully updating the inactive volume.
> 
> Interesting... I even did not thought about such use case as it is not
> possible to do it with other boot filesystems.

Well yes, it's somewhat of a hack, but it uses existing distroboot 
infrastructure.  We did comparable switching of volumes for active rootfs 
without distroboot before.  On newer targets we use distroboot together with 
RAUC and have one fix separate boot volume containing the boot script (the 
boot script is not part of those rootfs[01] anymore), which handles loading 
one or the other rootfs, no need to alter bootubivol anymore, it's constant.

> > > This last change allows to define more UBIFS target devices and make it
> > > clear what is boot MTD/UBI device.
> > 
> > How would one switch the default UBI volume to be booted from then?
> 
> With this my change, it is probably not easily possible. One option is
> to define more boot targets, e.g.:
> 
>   func(UBIFS, ubifs, 0, UBI, volume0)
>   func(UBIFS, ubifs, 1, UBI, volume1)
> 
> And then change env ${boottargets} from ubifs0 to ubifs1, which can be
> done via fs_setenv.

This should work, yes.  Maybe it's a little more complicated than this, e.g. 
switching 
from 'mmc0 ubifs0' to 'mmc0 ubifs1' 
or 
from 'ubifs0 ubifs1' to 'ubifs1 ubifs0'
but that's board dependent and no big deal in general I guess.

> As with this change is finally possible to specify more ubifs targets
> in BOOT_TARGET_DEVICES() u-boot macro.
> 
> This would be same as switching between other u-boot distroboot targets.

Makes sense.

> 
> ...
> 
> But I can understand that you want to have this (probably undocumented)
> behavior of switching UBI volume via env variable ${bootubivol}.
> 
> So what about allowing both configuration in U-Boot, with stored boot
> volume in ENV and with hardcoded boot volume in config file?

Just had another look at how 'bootcmd_ubifs0' is set with your patch, and I 
think with switching 'boot_targets' instead of 'bootubivol' it should work 
(not tested).  Only problem would be a migration strategy for old boards or a 
more sophisticated logic in Linux to determine what env variable has to be 
updated, but don't let that be your concern.

> And which / how many boards use this functionality?

Seriously, I have no idea.  You never know what all those downstream users do, 
especially in this case where functionality is in a boot script not part of U-
Boot itself.  Maybe I'm the only one using it like this, maybe not.  As of 
today I would recommend to use some update framework like RAUC anyways.

Greets
Alex

> 
> > Greets
> > Alex
> > 
> > > All board include config files are adjusted to use this new scheme of
> > > specifying boot MTD/UBI device.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > > CI test passed on https://github.com/u-boot/u-boot/pull/179
> > > ---
> > > 
> > >  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
> > >  include/configs/am335x_guardian.h  |  3 +--
> > >  include/configs/colibri-imx6ull.h  |  1 -
> > >  include/configs/colibri_imx7.h     |  1 -
> > >  include/configs/kontron-sl-mx6ul.h |  2 +-
> > >  include/configs/mys_6ulx.h         |  2 +-
> > >  include/configs/npi_imx6ull.h      |  2 +-
> > >  include/configs/omap3_beagle.h     |  4 +---
> > >  include/configs/omap3_evm.h        |  4 +---
> > >  include/configs/pcl063.h           |  2 +-
> > >  include/configs/stm32mp15_common.h |  2 +-
> > >  include/configs/uniphier.h         |  2 +-
> > >  12 files changed, 25 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/include/config_distro_bootcmd.h
> > > b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d
> > > 100644
> > > --- a/include/config_distro_bootcmd.h
> > > +++ b/include/config_distro_bootcmd.h
> > > @@ -70,18 +70,23 @@
> > > 
> > >  #ifdef CONFIG_CMD_UBIFS
> > >  #define BOOTENV_SHARED_UBIFS \
> > >  
> > >  	"ubifs_boot=" \
> > > 
> > > -		"env exists bootubipart || " \
> > > -			"env set bootubipart UBI; " \
> > > -		"env exists bootubivol || " \
> > > -			"env set bootubivol boot; " \
> > > 
> > >  		"if ubi part ${bootubipart} && " \
> > > 
> > > -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> > > +			"ubifsmount ubi0:${bootubivol}; " \
> > > 
> > >  		"then " \
> > >  		
> > >  			"devtype=ubi; " \
> > > 
> > > +			"devnum=ubi0; " \
> > > +			"bootfstype=ubifs; " \
> > > +			"distro_bootpart=${bootubivol}; " \
> > > 
> > >  			"run scan_dev_for_boot; " \
> > > 
> > > +			"ubifsumount; " \
> > > 
> > >  		"fi\0"
> > > 
> > > -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> > > -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> > > +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart,
> > > bootubivol) \ +	"bootcmd_ubifs" #instance "=" \
> > > +		"bootubipart=" #bootubipart "; " \
> > > +		"bootubivol=" #bootubivol "; " \
> > > +		"run ubifs_boot\0"
> > > +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance,
> > > bootubipart,
> > > bootubivol) \ +	#devtypel #instance " "
> > > 
> > >  #else
> > >  #define BOOTENV_SHARED_UBIFS
> > >  #define BOOTENV_DEV_UBIFS \
> > > 
> > > @@ -411,13 +416,13 @@
> > > 
> > >  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
> > >  
> > >  #endif
> > > 
> > > -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> > > -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> > > +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> > > +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ##
> > > __VA_ARGS__)
> > > 
> > >  #define BOOTENV_BOOT_TARGETS \
> > >  
> > >  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
> > > 
> > > -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> > > -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> > > +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> > > +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
> > > 
> > >  #define BOOTENV \
> > >  
> > >  	BOOTENV_SHARED_HOST \
> > >  	BOOTENV_SHARED_MMC \
> > > 
> > > diff --git a/include/configs/am335x_guardian.h
> > > b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6
> > > 100644
> > > --- a/include/configs/am335x_guardian.h
> > > +++ b/include/configs/am335x_guardian.h
> > > @@ -29,7 +29,7 @@
> > > 
> > >  	"ramdisk_addr_r=0x88080000\0" \
> > >  
> > >  #define BOOT_TARGET_DEVICES(func) \
> > > 
> > > -	func(UBIFS, ubifs, 0)
> > > +	func(UBIFS, ubifs, 0, UBI, rootfs)
> > > 
> > >  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE
> > > 
> > > ".dtb\0"
> > > 
> > > @@ -54,7 +54,6 @@
> > > 
> > >  	GUARDIAN_DEFAULT_PROD_ENV \
> > >  	"autoload=no\0" \
> > >  	"backlight_brightness=50\0" \
> > > 
> > > -	"bootubivol=rootfs\0" \
> > > 
> > >  	"distro_bootcmd=" \
> > >  	
> > >  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
> > >  		"setenv ethact usb_ether; " \
> > > 
> > > diff --git a/include/configs/colibri-imx6ull.h
> > > b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0
> > > 100644
> > > --- a/include/configs/colibri-imx6ull.h
> > > +++ b/include/configs/colibri-imx6ull.h
> > > @@ -91,7 +91,6 @@
> > > 
> > >  	UBI_BOOTCMD \
> > >  	UBOOT_UPDATE \
> > >  	"boot_script_dhcp=boot.scr\0" \
> > > 
> > > -	"bootubipart=ubi\0" \
> > > 
> > >  	"console=ttymxc0\0" \
> > >  	"defargs=user_debug=30\0" \
> > >  	"fdt_board=eval-v3\0" \
> > > 
> > > diff --git a/include/configs/colibri_imx7.h
> > > b/include/configs/colibri_imx7.h index 3dba7bcef258..3cad17777975
> > > 100644
> > > --- a/include/configs/colibri_imx7.h
> > > +++ b/include/configs/colibri_imx7.h
> > > @@ -131,7 +131,6 @@
> > > 
> > >  	UBOOT_UPDATE \
> > >  	"boot_file=zImage\0" \
> > >  	"boot_script_dhcp=boot.scr\0" \
> > > 
> > > -	"bootubipart=ubi\0" \
> > > 
> > >  	"console=ttymxc0\0" \
> > >  	"defargs=\0" \
> > >  	"fdt_board=eval-v3\0" \
> > > 
> > > diff --git a/include/configs/kontron-sl-mx6ul.h
> > > b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75
> > > 100644
> > > --- a/include/configs/kontron-sl-mx6ul.h
> > > +++ b/include/configs/kontron-sl-mx6ul.h
> > > @@ -45,7 +45,7 @@
> > > 
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >  
> > >  	func(MMC, mmc, 1) \
> > >  	func(MMC, mmc, 0) \
> > > 
> > > -	func(UBIFS, ubifs, 0) \
> > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > 
> > >  	func(USB, usb, 0) \
> > >  	func(PXE, pxe, na) \
> > >  	func(DHCP, dhcp, na)
> > > 
> > > diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> > > index 6801fc109eae..663820177a3e 100644
> > > --- a/include/configs/mys_6ulx.h
> > > +++ b/include/configs/mys_6ulx.h
> > > @@ -59,7 +59,7 @@
> > > 
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >  
> > >  	func(MMC, mmc, 0) \
> > > 
> > > -	func(UBIFS, ubifs, 0) \
> > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > 
> > >  	func(PXE, pxe, na) \
> > >  	func(DHCP, dhcp, na)
> > > 
> > > diff --git a/include/configs/npi_imx6ull.h
> > > b/include/configs/npi_imx6ull.h
> > > index c250fa650603..ebb887544e08 100644
> > > --- a/include/configs/npi_imx6ull.h
> > > +++ b/include/configs/npi_imx6ull.h
> > > @@ -67,7 +67,7 @@
> > > 
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >  
> > >  	func(MMC, mmc, 0) \
> > > 
> > > -	func(UBIFS, ubifs, 0) \
> > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > 
> > >  	func(PXE, pxe, na) \
> > >  	func(DHCP, dhcp, na)
> > > 
> > > diff --git a/include/configs/omap3_beagle.h
> > > b/include/configs/omap3_beagle.h index 158773acedb9..f5da08cf3359
> > > 100644
> > > --- a/include/configs/omap3_beagle.h
> > > +++ b/include/configs/omap3_beagle.h
> > > @@ -65,7 +65,7 @@
> > > 
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >  
> > >  	func(MMC, mmc, 0) \
> > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > 
> > > -	func(UBIFS, ubifs, 0) \
> > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > 
> > >  	func(NAND, nand, 0)
> > >  
> > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > 
> > > @@ -87,8 +87,6 @@
> > > 
> > >  	"bootenv=uEnv.txt\0" \
> > >  	"bootfile=zImage\0" \
> > >  	"bootpart=0:2\0" \
> > > 
> > > -	"bootubivol=rootfs\0" \
> > > -	"bootubipart=rootfs\0" \
> > > 
> > >  	"usbtty=cdc_acm\0" \
> > >  	"mpurate=auto\0" \
> > >  	"buddy=none\0" \
> > > 
> > > diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> > > index eeb9ef8c741a..cc98e03096ab 100644
> > > --- a/include/configs/omap3_evm.h
> > > +++ b/include/configs/omap3_evm.h
> > > @@ -60,7 +60,7 @@
> > > 
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >  
> > >  	func(MMC, mmc, 0) \
> > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > 
> > > -	func(UBIFS, ubifs, 0) \
> > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > 
> > >  	func(NAND, nand, 0)
> > >  
> > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > 
> > > @@ -88,8 +88,6 @@
> > > 
> > >  	"bootenv=uEnv.txt\0" \
> > >  	"bootfile=zImage\0" \
> > >  	"bootpart=0:2\0" \
> > > 
> > > -	"bootubivol=rootfs\0" \
> > > -	"bootubipart=rootfs\0" \
> > > 
> > >  	"optargs=\0" \
> > >  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
> > >  	"nandrootfstype=ubifs rootwait\0" \
> > > 
> > > diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> > > index 31b7d07a24cd..c3f7e7eb2c4b 100644
> > > --- a/include/configs/pcl063.h
> > > +++ b/include/configs/pcl063.h
> > > @@ -71,7 +71,7 @@
> > > 
> > >  #define BOOT_TARGET_DEVICES(func) \
> > >  
> > >  	func(MMC, mmc, 0) \
> > > 
> > > -	func(UBIFS, ubifs, 0) \
> > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > 
> > >  	func(PXE, pxe, na) \
> > >  	func(DHCP, dhcp, na)
> > > 
> > > diff --git a/include/configs/stm32mp15_common.h
> > > b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98
> > > 100644
> > > --- a/include/configs/stm32mp15_common.h
> > > +++ b/include/configs/stm32mp15_common.h
> > > @@ -77,7 +77,7 @@
> > > 
> > >  #endif
> > >  
> > >  #ifdef CONFIG_CMD_UBIFS
> > > 
> > > -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
> > > 
> > >  #else
> > >  #define BOOT_TARGET_UBIFS(func)
> > >  #endif
> > > 
> > > diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> > > index f813f88cdd7a..640a29067d85 100644
> > > --- a/include/configs/uniphier.h
> > > +++ b/include/configs/uniphier.h
> > > @@ -20,7 +20,7 @@
> > > 
> > >  #endif
> > >  
> > >  #ifdef CONFIG_CMD_UBIFS
> > > 
> > > -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, 
boot)
> > > 
> > >  #else
> > >  #define BOOT_TARGET_DEVICE_UBIFS(func)
> > >  #endif

Hello!

On Thursday 30 June 2022 08:46:59 Alexander Dahl wrote:
> Hello,
> 
> Am Mittwoch, 29. Juni 2022, 15:55:27 CEST schrieb Pali Rohár:
> > Hello!
> > 
> > On Wednesday 29 June 2022 15:36:57 Alexander Dahl wrote:
> > > Hello Pali,
> > > 
> > > had a look at this patch, and have some questions.  See below.
> > > 
> > > Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Rohár:
> > > > Fix multiple issues in ubifs distroboot code:
> > > > 
> > > > U-Boot supports attaching only one MTD device as UBI at the time. So
> > > > always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> > > > ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> > > > command attach MTD device always as UBI device ubi0.
> > > 
> > > Agreed.
> > > 
> > > > Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> > > > Distroboot scripts require ${bootfstype} variable to be properly set and
> > > > it
> > > > is already set for all other boot types.
> > > 
> > > From grepping through u-boot source, I can not quite follow.  Where is
> > > that
> > > variable set and where is it used?
> > 
> > For non-ubifs boot targets, env variable ${bootfstype} is set by
> > scan_dev_for_boot_part shell fragment. Then it calls user boot script
> > which can use ${bootfstype} variable. In most cases it is used for
> > constructing cmdline for kernel, mainly as "roofstype=${bootfstype}".
> 
> So this is no variable stored in env, but used in scripts only, right? 

Filesystem type is stored in (temporary) env variable ${bootfstype},
filled by u-boot env fragment ${scan_dev_for_boot_part}. But seems that
${bootfstype} is not used by u-boot.

> Especially in scripts not part of U-Boot itself?  Maybe this should be 
> documented somewhere then?

Yea, I agree, something which could be documented.

> > > > Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> > > > device does not have partitions, but has volumes. Distroboot scripts
> > > > require something to be set in ${distro_bootpart} variable, so set it to
> > > > the UBI volume which is currently mounted by ubifs.
> > > > 
> > > > Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> > > > differs from the other partition code that it requires "ubi" prefix
> > > > before
> > > > number.
> > > 
> > > Okay.
> > > 
> > > > Explicitly unmount ubifs volume after loading all data from it. This
> > > > allows
> > > > to detach UBI device from MTD device.
> > > 
> > > Okay.
> > > 
> > > > Move definition of MTD device with UBI and UBI volume with ubifs
> > > > filesystem
> > > > from global env variables ${bootubipart} and ${bootubivol} into the
> > > > distroboot "func" macro, defined in board include config files. UBIFS
> > > > distroboot macros then set ${bootubipart} and ${bootubivol} local
> > > > variables
> > > > for compatibility with existing distroboot scripts.
> > > 
> > > This might be problematic.  For a downstream board we use the 'bootubivol'
> > > env variable to switch between different volumes for update purposes. 
> > > Means on firmware update we modify U-Boot environment from Linux
> > > (fw_setenv) after successfully updating the inactive volume.
> > 
> > Interesting... I even did not thought about such use case as it is not
> > possible to do it with other boot filesystems.
> 
> Well yes, it's somewhat of a hack, but it uses existing distroboot 
> infrastructure.  We did comparable switching of volumes for active rootfs 
> without distroboot before.  On newer targets we use distroboot together with 
> RAUC and have one fix separate boot volume containing the boot script (the 
> boot script is not part of those rootfs[01] anymore), which handles loading 
> one or the other rootfs, no need to alter bootubivol anymore, it's constant.
> 
> > > > This last change allows to define more UBIFS target devices and make it
> > > > clear what is boot MTD/UBI device.
> > > 
> > > How would one switch the default UBI volume to be booted from then?
> > 
> > With this my change, it is probably not easily possible. One option is
> > to define more boot targets, e.g.:
> > 
> >   func(UBIFS, ubifs, 0, UBI, volume0)
> >   func(UBIFS, ubifs, 1, UBI, volume1)
> > 
> > And then change env ${boottargets} from ubifs0 to ubifs1, which can be
> > done via fs_setenv.
> 
> This should work, yes.  Maybe it's a little more complicated than this, e.g. 
> switching 
> from 'mmc0 ubifs0' to 'mmc0 ubifs1' 
> or 
> from 'ubifs0 ubifs1' to 'ubifs1 ubifs0'
> but that's board dependent and no big deal in general I guess.

Ok.

> > As with this change is finally possible to specify more ubifs targets
> > in BOOT_TARGET_DEVICES() u-boot macro.
> > 
> > This would be same as switching between other u-boot distroboot targets.
> 
> Makes sense.
> 
> > 
> > ...
> > 
> > But I can understand that you want to have this (probably undocumented)
> > behavior of switching UBI volume via env variable ${bootubivol}.
> > 
> > So what about allowing both configuration in U-Boot, with stored boot
> > volume in ENV and with hardcoded boot volume in config file?
> 
> Just had another look at how 'bootcmd_ubifs0' is set with your patch, and I 
> think with switching 'boot_targets' instead of 'bootubivol' it should work 
> (not tested).  Only problem would be a migration strategy for old boards or a 
> more sophisticated logic in Linux to determine what env variable has to be 
> updated, but don't let that be your concern.

Note that until you reset env variables to default, u-boot will
continue using "old" style and because all logic is stored only in env
(not in u-boot runtime code itself), there should be no issue with
upgrading u-boot to new version which uses ubifs0 and ubifs1.

Anyway, does it mean that we need to extend this patch to allow still
storing/updating ubi boot volume in env file?

> > And which / how many boards use this functionality?
> 
> Seriously, I have no idea.  You never know what all those downstream users do, 
> especially in this case where functionality is in a boot script not part of U-
> Boot itself.  Maybe I'm the only one using it like this, maybe not.  As of 
> today I would recommend to use some update framework like RAUC anyways.

I think boards which you maintain or use.

> Greets
> Alex
> 
> > 
> > > Greets
> > > Alex
> > > 
> > > > All board include config files are adjusted to use this new scheme of
> > > > specifying boot MTD/UBI device.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > > CI test passed on https://github.com/u-boot/u-boot/pull/179
> > > > ---
> > > > 
> > > >  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
> > > >  include/configs/am335x_guardian.h  |  3 +--
> > > >  include/configs/colibri-imx6ull.h  |  1 -
> > > >  include/configs/colibri_imx7.h     |  1 -
> > > >  include/configs/kontron-sl-mx6ul.h |  2 +-
> > > >  include/configs/mys_6ulx.h         |  2 +-
> > > >  include/configs/npi_imx6ull.h      |  2 +-
> > > >  include/configs/omap3_beagle.h     |  4 +---
> > > >  include/configs/omap3_evm.h        |  4 +---
> > > >  include/configs/pcl063.h           |  2 +-
> > > >  include/configs/stm32mp15_common.h |  2 +-
> > > >  include/configs/uniphier.h         |  2 +-
> > > >  12 files changed, 25 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/include/config_distro_bootcmd.h
> > > > b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d
> > > > 100644
> > > > --- a/include/config_distro_bootcmd.h
> > > > +++ b/include/config_distro_bootcmd.h
> > > > @@ -70,18 +70,23 @@
> > > > 
> > > >  #ifdef CONFIG_CMD_UBIFS
> > > >  #define BOOTENV_SHARED_UBIFS \
> > > >  
> > > >  	"ubifs_boot=" \
> > > > 
> > > > -		"env exists bootubipart || " \
> > > > -			"env set bootubipart UBI; " \
> > > > -		"env exists bootubivol || " \
> > > > -			"env set bootubivol boot; " \
> > > > 
> > > >  		"if ubi part ${bootubipart} && " \
> > > > 
> > > > -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> > > > +			"ubifsmount ubi0:${bootubivol}; " \
> > > > 
> > > >  		"then " \
> > > >  		
> > > >  			"devtype=ubi; " \
> > > > 
> > > > +			"devnum=ubi0; " \
> > > > +			"bootfstype=ubifs; " \
> > > > +			"distro_bootpart=${bootubivol}; " \
> > > > 
> > > >  			"run scan_dev_for_boot; " \
> > > > 
> > > > +			"ubifsumount; " \
> > > > 
> > > >  		"fi\0"
> > > > 
> > > > -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> > > > -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> > > > +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart,
> > > > bootubivol) \ +	"bootcmd_ubifs" #instance "=" \
> > > > +		"bootubipart=" #bootubipart "; " \
> > > > +		"bootubivol=" #bootubivol "; " \
> > > > +		"run ubifs_boot\0"
> > > > +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance,
> > > > bootubipart,
> > > > bootubivol) \ +	#devtypel #instance " "
> > > > 
> > > >  #else
> > > >  #define BOOTENV_SHARED_UBIFS
> > > >  #define BOOTENV_DEV_UBIFS \
> > > > 
> > > > @@ -411,13 +416,13 @@
> > > > 
> > > >  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
> > > >  
> > > >  #endif
> > > > 
> > > > -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> > > > -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> > > > +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> > > > +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ##
> > > > __VA_ARGS__)
> > > > 
> > > >  #define BOOTENV_BOOT_TARGETS \
> > > >  
> > > >  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
> > > > 
> > > > -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> > > > -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> > > > +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> > > > +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
> > > > 
> > > >  #define BOOTENV \
> > > >  
> > > >  	BOOTENV_SHARED_HOST \
> > > >  	BOOTENV_SHARED_MMC \
> > > > 
> > > > diff --git a/include/configs/am335x_guardian.h
> > > > b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6
> > > > 100644
> > > > --- a/include/configs/am335x_guardian.h
> > > > +++ b/include/configs/am335x_guardian.h
> > > > @@ -29,7 +29,7 @@
> > > > 
> > > >  	"ramdisk_addr_r=0x88080000\0" \
> > > >  
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0)
> > > > +	func(UBIFS, ubifs, 0, UBI, rootfs)
> > > > 
> > > >  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE
> > > > 
> > > > ".dtb\0"
> > > > 
> > > > @@ -54,7 +54,6 @@
> > > > 
> > > >  	GUARDIAN_DEFAULT_PROD_ENV \
> > > >  	"autoload=no\0" \
> > > >  	"backlight_brightness=50\0" \
> > > > 
> > > > -	"bootubivol=rootfs\0" \
> > > > 
> > > >  	"distro_bootcmd=" \
> > > >  	
> > > >  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
> > > >  		"setenv ethact usb_ether; " \
> > > > 
> > > > diff --git a/include/configs/colibri-imx6ull.h
> > > > b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0
> > > > 100644
> > > > --- a/include/configs/colibri-imx6ull.h
> > > > +++ b/include/configs/colibri-imx6ull.h
> > > > @@ -91,7 +91,6 @@
> > > > 
> > > >  	UBI_BOOTCMD \
> > > >  	UBOOT_UPDATE \
> > > >  	"boot_script_dhcp=boot.scr\0" \
> > > > 
> > > > -	"bootubipart=ubi\0" \
> > > > 
> > > >  	"console=ttymxc0\0" \
> > > >  	"defargs=user_debug=30\0" \
> > > >  	"fdt_board=eval-v3\0" \
> > > > 
> > > > diff --git a/include/configs/colibri_imx7.h
> > > > b/include/configs/colibri_imx7.h index 3dba7bcef258..3cad17777975
> > > > 100644
> > > > --- a/include/configs/colibri_imx7.h
> > > > +++ b/include/configs/colibri_imx7.h
> > > > @@ -131,7 +131,6 @@
> > > > 
> > > >  	UBOOT_UPDATE \
> > > >  	"boot_file=zImage\0" \
> > > >  	"boot_script_dhcp=boot.scr\0" \
> > > > 
> > > > -	"bootubipart=ubi\0" \
> > > > 
> > > >  	"console=ttymxc0\0" \
> > > >  	"defargs=\0" \
> > > >  	"fdt_board=eval-v3\0" \
> > > > 
> > > > diff --git a/include/configs/kontron-sl-mx6ul.h
> > > > b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75
> > > > 100644
> > > > --- a/include/configs/kontron-sl-mx6ul.h
> > > > +++ b/include/configs/kontron-sl-mx6ul.h
> > > > @@ -45,7 +45,7 @@
> > > > 
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > >  
> > > >  	func(MMC, mmc, 1) \
> > > >  	func(MMC, mmc, 0) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0) \
> > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > 
> > > >  	func(USB, usb, 0) \
> > > >  	func(PXE, pxe, na) \
> > > >  	func(DHCP, dhcp, na)
> > > > 
> > > > diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> > > > index 6801fc109eae..663820177a3e 100644
> > > > --- a/include/configs/mys_6ulx.h
> > > > +++ b/include/configs/mys_6ulx.h
> > > > @@ -59,7 +59,7 @@
> > > > 
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > >  
> > > >  	func(MMC, mmc, 0) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0) \
> > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > 
> > > >  	func(PXE, pxe, na) \
> > > >  	func(DHCP, dhcp, na)
> > > > 
> > > > diff --git a/include/configs/npi_imx6ull.h
> > > > b/include/configs/npi_imx6ull.h
> > > > index c250fa650603..ebb887544e08 100644
> > > > --- a/include/configs/npi_imx6ull.h
> > > > +++ b/include/configs/npi_imx6ull.h
> > > > @@ -67,7 +67,7 @@
> > > > 
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > >  
> > > >  	func(MMC, mmc, 0) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0) \
> > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > 
> > > >  	func(PXE, pxe, na) \
> > > >  	func(DHCP, dhcp, na)
> > > > 
> > > > diff --git a/include/configs/omap3_beagle.h
> > > > b/include/configs/omap3_beagle.h index 158773acedb9..f5da08cf3359
> > > > 100644
> > > > --- a/include/configs/omap3_beagle.h
> > > > +++ b/include/configs/omap3_beagle.h
> > > > @@ -65,7 +65,7 @@
> > > > 
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > >  
> > > >  	func(MMC, mmc, 0) \
> > > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0) \
> > > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > > 
> > > >  	func(NAND, nand, 0)
> > > >  
> > > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > > 
> > > > @@ -87,8 +87,6 @@
> > > > 
> > > >  	"bootenv=uEnv.txt\0" \
> > > >  	"bootfile=zImage\0" \
> > > >  	"bootpart=0:2\0" \
> > > > 
> > > > -	"bootubivol=rootfs\0" \
> > > > -	"bootubipart=rootfs\0" \
> > > > 
> > > >  	"usbtty=cdc_acm\0" \
> > > >  	"mpurate=auto\0" \
> > > >  	"buddy=none\0" \
> > > > 
> > > > diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> > > > index eeb9ef8c741a..cc98e03096ab 100644
> > > > --- a/include/configs/omap3_evm.h
> > > > +++ b/include/configs/omap3_evm.h
> > > > @@ -60,7 +60,7 @@
> > > > 
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > >  
> > > >  	func(MMC, mmc, 0) \
> > > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0) \
> > > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > > 
> > > >  	func(NAND, nand, 0)
> > > >  
> > > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > > 
> > > > @@ -88,8 +88,6 @@
> > > > 
> > > >  	"bootenv=uEnv.txt\0" \
> > > >  	"bootfile=zImage\0" \
> > > >  	"bootpart=0:2\0" \
> > > > 
> > > > -	"bootubivol=rootfs\0" \
> > > > -	"bootubipart=rootfs\0" \
> > > > 
> > > >  	"optargs=\0" \
> > > >  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
> > > >  	"nandrootfstype=ubifs rootwait\0" \
> > > > 
> > > > diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> > > > index 31b7d07a24cd..c3f7e7eb2c4b 100644
> > > > --- a/include/configs/pcl063.h
> > > > +++ b/include/configs/pcl063.h
> > > > @@ -71,7 +71,7 @@
> > > > 
> > > >  #define BOOT_TARGET_DEVICES(func) \
> > > >  
> > > >  	func(MMC, mmc, 0) \
> > > > 
> > > > -	func(UBIFS, ubifs, 0) \
> > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > 
> > > >  	func(PXE, pxe, na) \
> > > >  	func(DHCP, dhcp, na)
> > > > 
> > > > diff --git a/include/configs/stm32mp15_common.h
> > > > b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98
> > > > 100644
> > > > --- a/include/configs/stm32mp15_common.h
> > > > +++ b/include/configs/stm32mp15_common.h
> > > > @@ -77,7 +77,7 @@
> > > > 
> > > >  #endif
> > > >  
> > > >  #ifdef CONFIG_CMD_UBIFS
> > > > 
> > > > -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > > +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
> > > > 
> > > >  #else
> > > >  #define BOOT_TARGET_UBIFS(func)
> > > >  #endif
> > > > 
> > > > diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> > > > index f813f88cdd7a..640a29067d85 100644
> > > > --- a/include/configs/uniphier.h
> > > > +++ b/include/configs/uniphier.h
> > > > @@ -20,7 +20,7 @@
> > > > 
> > > >  #endif
> > > >  
> > > >  #ifdef CONFIG_CMD_UBIFS
> > > > 
> > > > -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > > +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, 
> boot)
> > > > 
> > > >  #else
> > > >  #define BOOT_TARGET_DEVICE_UBIFS(func)
> > > >  #endif
> 
> 
> 
> 
>

Hello Pali,

Am Mon, Jul 04, 2022 at 09:55:53AM +0200 schrieb Pali Rohár:
> Hello!
> 
> On Thursday 30 June 2022 08:46:59 Alexander Dahl wrote:
> > Hello,
> > 
> > Am Mittwoch, 29. Juni 2022, 15:55:27 CEST schrieb Pali Rohár:
> > > Hello!
> > > 
> > > On Wednesday 29 June 2022 15:36:57 Alexander Dahl wrote:
> > > > Hello Pali,
> > > > 
> > > > had a look at this patch, and have some questions.  See below.
> > > > 
> > > > Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Rohár:
> > > > > Fix multiple issues in ubifs distroboot code:
> > > > > 
> > > > > U-Boot supports attaching only one MTD device as UBI at the time. So
> > > > > always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> > > > > ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> > > > > command attach MTD device always as UBI device ubi0.
> > > > 
> > > > Agreed.
> > > > 
> > > > > Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> > > > > Distroboot scripts require ${bootfstype} variable to be properly set and
> > > > > it
> > > > > is already set for all other boot types.
> > > > 
> > > > From grepping through u-boot source, I can not quite follow.  Where is
> > > > that
> > > > variable set and where is it used?
> > > 
> > > For non-ubifs boot targets, env variable ${bootfstype} is set by
> > > scan_dev_for_boot_part shell fragment. Then it calls user boot script
> > > which can use ${bootfstype} variable. In most cases it is used for
> > > constructing cmdline for kernel, mainly as "roofstype=${bootfstype}".
> > 
> > So this is no variable stored in env, but used in scripts only, right? 
> 
> Filesystem type is stored in (temporary) env variable ${bootfstype},
> filled by u-boot env fragment ${scan_dev_for_boot_part}. But seems that
> ${bootfstype} is not used by u-boot.
> 
> > Especially in scripts not part of U-Boot itself?  Maybe this should be 
> > documented somewhere then?
> 
> Yea, I agree, something which could be documented.
> 
> > > > > Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> > > > > device does not have partitions, but has volumes. Distroboot scripts
> > > > > require something to be set in ${distro_bootpart} variable, so set it to
> > > > > the UBI volume which is currently mounted by ubifs.
> > > > > 
> > > > > Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> > > > > differs from the other partition code that it requires "ubi" prefix
> > > > > before
> > > > > number.
> > > > 
> > > > Okay.
> > > > 
> > > > > Explicitly unmount ubifs volume after loading all data from it. This
> > > > > allows
> > > > > to detach UBI device from MTD device.
> > > > 
> > > > Okay.
> > > > 
> > > > > Move definition of MTD device with UBI and UBI volume with ubifs
> > > > > filesystem
> > > > > from global env variables ${bootubipart} and ${bootubivol} into the
> > > > > distroboot "func" macro, defined in board include config files. UBIFS
> > > > > distroboot macros then set ${bootubipart} and ${bootubivol} local
> > > > > variables
> > > > > for compatibility with existing distroboot scripts.
> > > > 
> > > > This might be problematic.  For a downstream board we use the 'bootubivol'
> > > > env variable to switch between different volumes for update purposes. 
> > > > Means on firmware update we modify U-Boot environment from Linux
> > > > (fw_setenv) after successfully updating the inactive volume.
> > > 
> > > Interesting... I even did not thought about such use case as it is not
> > > possible to do it with other boot filesystems.
> > 
> > Well yes, it's somewhat of a hack, but it uses existing distroboot 
> > infrastructure.  We did comparable switching of volumes for active rootfs 
> > without distroboot before.  On newer targets we use distroboot together with 
> > RAUC and have one fix separate boot volume containing the boot script (the 
> > boot script is not part of those rootfs[01] anymore), which handles loading 
> > one or the other rootfs, no need to alter bootubivol anymore, it's constant.
> > 
> > > > > This last change allows to define more UBIFS target devices and make it
> > > > > clear what is boot MTD/UBI device.
> > > > 
> > > > How would one switch the default UBI volume to be booted from then?
> > > 
> > > With this my change, it is probably not easily possible. One option is
> > > to define more boot targets, e.g.:
> > > 
> > >   func(UBIFS, ubifs, 0, UBI, volume0)
> > >   func(UBIFS, ubifs, 1, UBI, volume1)
> > > 
> > > And then change env ${boottargets} from ubifs0 to ubifs1, which can be
> > > done via fs_setenv.
> > 
> > This should work, yes.  Maybe it's a little more complicated than this, e.g. 
> > switching 
> > from 'mmc0 ubifs0' to 'mmc0 ubifs1' 
> > or 
> > from 'ubifs0 ubifs1' to 'ubifs1 ubifs0'
> > but that's board dependent and no big deal in general I guess.
> 
> Ok.
> 
> > > As with this change is finally possible to specify more ubifs targets
> > > in BOOT_TARGET_DEVICES() u-boot macro.
> > > 
> > > This would be same as switching between other u-boot distroboot targets.
> > 
> > Makes sense.
> > 
> > > 
> > > ...
> > > 
> > > But I can understand that you want to have this (probably undocumented)
> > > behavior of switching UBI volume via env variable ${bootubivol}.
> > > 
> > > So what about allowing both configuration in U-Boot, with stored boot
> > > volume in ENV and with hardcoded boot volume in config file?
> > 
> > Just had another look at how 'bootcmd_ubifs0' is set with your patch, and I 
> > think with switching 'boot_targets' instead of 'bootubivol' it should work 
> > (not tested).  Only problem would be a migration strategy for old boards or a 
> > more sophisticated logic in Linux to determine what env variable has to be 
> > updated, but don't let that be your concern.
> 
> Note that until you reset env variables to default, u-boot will
> continue using "old" style and because all logic is stored only in env
> (not in u-boot runtime code itself), there should be no issue with
> upgrading u-boot to new version which uses ubifs0 and ubifs1.

Agreed.

Side note: U-Boot is not upgraded at runtime on those targets in
question.  UBI/UBIFS is used on raw NAND flash, and with the SoC used
it's not easily possible to upgrade U-Boot from within the running
system in a safe way (at91bootstrap loads U-Boot from a fix offset in
NAND flash, which makes a A/B with atomic switch or some fallback
mechanism not feasible).  I guess there are quite some boards where
the U-Boot binary is written once only when producing the board and
never touched again?

However there might be a newer U-Boot version in later iterations of
such a board, or maybe just in a later production firmware on
otherwise unmodified hardware.

> Anyway, does it mean that we need to extend this patch to allow still
> storing/updating ubi boot volume in env file?

I don't think so, at least not for the board I maintain.  Any upgrade
mechanism on Linux side would have to deal with the old and the new
variant, so it would have to detect the mechanism used on that actual
board and use that one, be it the 'bootubivol' variable or something
else.

> > > And which / how many boards use this functionality?
> > 
> > Seriously, I have no idea.  You never know what all those downstream users do, 
> > especially in this case where functionality is in a boot script not part of U-
> > Boot itself.  Maybe I'm the only one using it like this, maybe not.  As of 
> > today I would recommend to use some update framework like RAUC anyways.
> 
> I think boards which you maintain or use.

There's one board I maintain, which uses that 'bootubivol' variable in
the way described before.  I have another board on my TODO-List, but I
did not decide yet how upgrades should be done on that one.

As you might have guessed already, those boards are not in upstream
u-boot and probably never will.  I don't like that, but it's not my
decision.

Thanks for your commments on that topic anyways, much appreciated. :-)

Greets
Alex

> 
> 
> > Greets
> > Alex
> > 
> > > 
> > > > Greets
> > > > Alex
> > > > 
> > > > > All board include config files are adjusted to use this new scheme of
> > > > > specifying boot MTD/UBI device.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > ---
> > > > > CI test passed on https://github.com/u-boot/u-boot/pull/179
> > > > > ---
> > > > > 
> > > > >  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
> > > > >  include/configs/am335x_guardian.h  |  3 +--
> > > > >  include/configs/colibri-imx6ull.h  |  1 -
> > > > >  include/configs/colibri_imx7.h     |  1 -
> > > > >  include/configs/kontron-sl-mx6ul.h |  2 +-
> > > > >  include/configs/mys_6ulx.h         |  2 +-
> > > > >  include/configs/npi_imx6ull.h      |  2 +-
> > > > >  include/configs/omap3_beagle.h     |  4 +---
> > > > >  include/configs/omap3_evm.h        |  4 +---
> > > > >  include/configs/pcl063.h           |  2 +-
> > > > >  include/configs/stm32mp15_common.h |  2 +-
> > > > >  include/configs/uniphier.h         |  2 +-
> > > > >  12 files changed, 25 insertions(+), 27 deletions(-)
> > > > > 
> > > > > diff --git a/include/config_distro_bootcmd.h
> > > > > b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d
> > > > > 100644
> > > > > --- a/include/config_distro_bootcmd.h
> > > > > +++ b/include/config_distro_bootcmd.h
> > > > > @@ -70,18 +70,23 @@
> > > > > 
> > > > >  #ifdef CONFIG_CMD_UBIFS
> > > > >  #define BOOTENV_SHARED_UBIFS \
> > > > >  
> > > > >  	"ubifs_boot=" \
> > > > > 
> > > > > -		"env exists bootubipart || " \
> > > > > -			"env set bootubipart UBI; " \
> > > > > -		"env exists bootubivol || " \
> > > > > -			"env set bootubivol boot; " \
> > > > > 
> > > > >  		"if ubi part ${bootubipart} && " \
> > > > > 
> > > > > -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> > > > > +			"ubifsmount ubi0:${bootubivol}; " \
> > > > > 
> > > > >  		"then " \
> > > > >  		
> > > > >  			"devtype=ubi; " \
> > > > > 
> > > > > +			"devnum=ubi0; " \
> > > > > +			"bootfstype=ubifs; " \
> > > > > +			"distro_bootpart=${bootubivol}; " \
> > > > > 
> > > > >  			"run scan_dev_for_boot; " \
> > > > > 
> > > > > +			"ubifsumount; " \
> > > > > 
> > > > >  		"fi\0"
> > > > > 
> > > > > -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> > > > > -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> > > > > +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart,
> > > > > bootubivol) \ +	"bootcmd_ubifs" #instance "=" \
> > > > > +		"bootubipart=" #bootubipart "; " \
> > > > > +		"bootubivol=" #bootubivol "; " \
> > > > > +		"run ubifs_boot\0"
> > > > > +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance,
> > > > > bootubipart,
> > > > > bootubivol) \ +	#devtypel #instance " "
> > > > > 
> > > > >  #else
> > > > >  #define BOOTENV_SHARED_UBIFS
> > > > >  #define BOOTENV_DEV_UBIFS \
> > > > > 
> > > > > @@ -411,13 +416,13 @@
> > > > > 
> > > > >  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
> > > > >  
> > > > >  #endif
> > > > > 
> > > > > -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> > > > > -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> > > > > +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> > > > > +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ##
> > > > > __VA_ARGS__)
> > > > > 
> > > > >  #define BOOTENV_BOOT_TARGETS \
> > > > >  
> > > > >  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
> > > > > 
> > > > > -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> > > > > -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> > > > > +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> > > > > +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
> > > > > 
> > > > >  #define BOOTENV \
> > > > >  
> > > > >  	BOOTENV_SHARED_HOST \
> > > > >  	BOOTENV_SHARED_MMC \
> > > > > 
> > > > > diff --git a/include/configs/am335x_guardian.h
> > > > > b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6
> > > > > 100644
> > > > > --- a/include/configs/am335x_guardian.h
> > > > > +++ b/include/configs/am335x_guardian.h
> > > > > @@ -29,7 +29,7 @@
> > > > > 
> > > > >  	"ramdisk_addr_r=0x88080000\0" \
> > > > >  
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0)
> > > > > +	func(UBIFS, ubifs, 0, UBI, rootfs)
> > > > > 
> > > > >  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE
> > > > > 
> > > > > ".dtb\0"
> > > > > 
> > > > > @@ -54,7 +54,6 @@
> > > > > 
> > > > >  	GUARDIAN_DEFAULT_PROD_ENV \
> > > > >  	"autoload=no\0" \
> > > > >  	"backlight_brightness=50\0" \
> > > > > 
> > > > > -	"bootubivol=rootfs\0" \
> > > > > 
> > > > >  	"distro_bootcmd=" \
> > > > >  	
> > > > >  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
> > > > >  		"setenv ethact usb_ether; " \
> > > > > 
> > > > > diff --git a/include/configs/colibri-imx6ull.h
> > > > > b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0
> > > > > 100644
> > > > > --- a/include/configs/colibri-imx6ull.h
> > > > > +++ b/include/configs/colibri-imx6ull.h
> > > > > @@ -91,7 +91,6 @@
> > > > > 
> > > > >  	UBI_BOOTCMD \
> > > > >  	UBOOT_UPDATE \
> > > > >  	"boot_script_dhcp=boot.scr\0" \
> > > > > 
> > > > > -	"bootubipart=ubi\0" \
> > > > > 
> > > > >  	"console=ttymxc0\0" \
> > > > >  	"defargs=user_debug=30\0" \
> > > > >  	"fdt_board=eval-v3\0" \
> > > > > 
> > > > > diff --git a/include/configs/colibri_imx7.h
> > > > > b/include/configs/colibri_imx7.h index 3dba7bcef258..3cad17777975
> > > > > 100644
> > > > > --- a/include/configs/colibri_imx7.h
> > > > > +++ b/include/configs/colibri_imx7.h
> > > > > @@ -131,7 +131,6 @@
> > > > > 
> > > > >  	UBOOT_UPDATE \
> > > > >  	"boot_file=zImage\0" \
> > > > >  	"boot_script_dhcp=boot.scr\0" \
> > > > > 
> > > > > -	"bootubipart=ubi\0" \
> > > > > 
> > > > >  	"console=ttymxc0\0" \
> > > > >  	"defargs=\0" \
> > > > >  	"fdt_board=eval-v3\0" \
> > > > > 
> > > > > diff --git a/include/configs/kontron-sl-mx6ul.h
> > > > > b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75
> > > > > 100644
> > > > > --- a/include/configs/kontron-sl-mx6ul.h
> > > > > +++ b/include/configs/kontron-sl-mx6ul.h
> > > > > @@ -45,7 +45,7 @@
> > > > > 
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > >  
> > > > >  	func(MMC, mmc, 1) \
> > > > >  	func(MMC, mmc, 0) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0) \
> > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > 
> > > > >  	func(USB, usb, 0) \
> > > > >  	func(PXE, pxe, na) \
> > > > >  	func(DHCP, dhcp, na)
> > > > > 
> > > > > diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> > > > > index 6801fc109eae..663820177a3e 100644
> > > > > --- a/include/configs/mys_6ulx.h
> > > > > +++ b/include/configs/mys_6ulx.h
> > > > > @@ -59,7 +59,7 @@
> > > > > 
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > >  
> > > > >  	func(MMC, mmc, 0) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0) \
> > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > 
> > > > >  	func(PXE, pxe, na) \
> > > > >  	func(DHCP, dhcp, na)
> > > > > 
> > > > > diff --git a/include/configs/npi_imx6ull.h
> > > > > b/include/configs/npi_imx6ull.h
> > > > > index c250fa650603..ebb887544e08 100644
> > > > > --- a/include/configs/npi_imx6ull.h
> > > > > +++ b/include/configs/npi_imx6ull.h
> > > > > @@ -67,7 +67,7 @@
> > > > > 
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > >  
> > > > >  	func(MMC, mmc, 0) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0) \
> > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > 
> > > > >  	func(PXE, pxe, na) \
> > > > >  	func(DHCP, dhcp, na)
> > > > > 
> > > > > diff --git a/include/configs/omap3_beagle.h
> > > > > b/include/configs/omap3_beagle.h index 158773acedb9..f5da08cf3359
> > > > > 100644
> > > > > --- a/include/configs/omap3_beagle.h
> > > > > +++ b/include/configs/omap3_beagle.h
> > > > > @@ -65,7 +65,7 @@
> > > > > 
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > >  
> > > > >  	func(MMC, mmc, 0) \
> > > > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0) \
> > > > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > > > 
> > > > >  	func(NAND, nand, 0)
> > > > >  
> > > > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > > > 
> > > > > @@ -87,8 +87,6 @@
> > > > > 
> > > > >  	"bootenv=uEnv.txt\0" \
> > > > >  	"bootfile=zImage\0" \
> > > > >  	"bootpart=0:2\0" \
> > > > > 
> > > > > -	"bootubivol=rootfs\0" \
> > > > > -	"bootubipart=rootfs\0" \
> > > > > 
> > > > >  	"usbtty=cdc_acm\0" \
> > > > >  	"mpurate=auto\0" \
> > > > >  	"buddy=none\0" \
> > > > > 
> > > > > diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> > > > > index eeb9ef8c741a..cc98e03096ab 100644
> > > > > --- a/include/configs/omap3_evm.h
> > > > > +++ b/include/configs/omap3_evm.h
> > > > > @@ -60,7 +60,7 @@
> > > > > 
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > >  
> > > > >  	func(MMC, mmc, 0) \
> > > > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0) \
> > > > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > > > 
> > > > >  	func(NAND, nand, 0)
> > > > >  
> > > > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > > > 
> > > > > @@ -88,8 +88,6 @@
> > > > > 
> > > > >  	"bootenv=uEnv.txt\0" \
> > > > >  	"bootfile=zImage\0" \
> > > > >  	"bootpart=0:2\0" \
> > > > > 
> > > > > -	"bootubivol=rootfs\0" \
> > > > > -	"bootubipart=rootfs\0" \
> > > > > 
> > > > >  	"optargs=\0" \
> > > > >  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
> > > > >  	"nandrootfstype=ubifs rootwait\0" \
> > > > > 
> > > > > diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> > > > > index 31b7d07a24cd..c3f7e7eb2c4b 100644
> > > > > --- a/include/configs/pcl063.h
> > > > > +++ b/include/configs/pcl063.h
> > > > > @@ -71,7 +71,7 @@
> > > > > 
> > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > >  
> > > > >  	func(MMC, mmc, 0) \
> > > > > 
> > > > > -	func(UBIFS, ubifs, 0) \
> > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > 
> > > > >  	func(PXE, pxe, na) \
> > > > >  	func(DHCP, dhcp, na)
> > > > > 
> > > > > diff --git a/include/configs/stm32mp15_common.h
> > > > > b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98
> > > > > 100644
> > > > > --- a/include/configs/stm32mp15_common.h
> > > > > +++ b/include/configs/stm32mp15_common.h
> > > > > @@ -77,7 +77,7 @@
> > > > > 
> > > > >  #endif
> > > > >  
> > > > >  #ifdef CONFIG_CMD_UBIFS
> > > > > 
> > > > > -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > > > +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
> > > > > 
> > > > >  #else
> > > > >  #define BOOT_TARGET_UBIFS(func)
> > > > >  #endif
> > > > > 
> > > > > diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> > > > > index f813f88cdd7a..640a29067d85 100644
> > > > > --- a/include/configs/uniphier.h
> > > > > +++ b/include/configs/uniphier.h
> > > > > @@ -20,7 +20,7 @@
> > > > > 
> > > > >  #endif
> > > > >  
> > > > >  #ifdef CONFIG_CMD_UBIFS
> > > > > 
> > > > > -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > > > +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, 
> > boot)
> > > > > 
> > > > >  #else
> > > > >  #define BOOT_TARGET_DEVICE_UBIFS(func)
> > > > >  #endif
> > 
> > 
> > 
> > 
> >

On Tuesday 05 July 2022 09:32:09 Alexander Dahl wrote:
> Hello Pali,
> 
> Am Mon, Jul 04, 2022 at 09:55:53AM +0200 schrieb Pali Rohár:
> > Hello!
> > 
> > On Thursday 30 June 2022 08:46:59 Alexander Dahl wrote:
> > > Hello,
> > > 
> > > Am Mittwoch, 29. Juni 2022, 15:55:27 CEST schrieb Pali Rohár:
> > > > Hello!
> > > > 
> > > > On Wednesday 29 June 2022 15:36:57 Alexander Dahl wrote:
> > > > > Hello Pali,
> > > > > 
> > > > > had a look at this patch, and have some questions.  See below.
> > > > > 
> > > > > Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Rohár:
> > > > > > Fix multiple issues in ubifs distroboot code:
> > > > > > 
> > > > > > U-Boot supports attaching only one MTD device as UBI at the time. So
> > > > > > always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> > > > > > ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> > > > > > command attach MTD device always as UBI device ubi0.
> > > > > 
> > > > > Agreed.
> > > > > 
> > > > > > Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> > > > > > Distroboot scripts require ${bootfstype} variable to be properly set and
> > > > > > it
> > > > > > is already set for all other boot types.
> > > > > 
> > > > > From grepping through u-boot source, I can not quite follow.  Where is
> > > > > that
> > > > > variable set and where is it used?
> > > > 
> > > > For non-ubifs boot targets, env variable ${bootfstype} is set by
> > > > scan_dev_for_boot_part shell fragment. Then it calls user boot script
> > > > which can use ${bootfstype} variable. In most cases it is used for
> > > > constructing cmdline for kernel, mainly as "roofstype=${bootfstype}".
> > > 
> > > So this is no variable stored in env, but used in scripts only, right? 
> > 
> > Filesystem type is stored in (temporary) env variable ${bootfstype},
> > filled by u-boot env fragment ${scan_dev_for_boot_part}. But seems that
> > ${bootfstype} is not used by u-boot.
> > 
> > > Especially in scripts not part of U-Boot itself?  Maybe this should be 
> > > documented somewhere then?
> > 
> > Yea, I agree, something which could be documented.
> > 
> > > > > > Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> > > > > > device does not have partitions, but has volumes. Distroboot scripts
> > > > > > require something to be set in ${distro_bootpart} variable, so set it to
> > > > > > the UBI volume which is currently mounted by ubifs.
> > > > > > 
> > > > > > Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> > > > > > differs from the other partition code that it requires "ubi" prefix
> > > > > > before
> > > > > > number.
> > > > > 
> > > > > Okay.
> > > > > 
> > > > > > Explicitly unmount ubifs volume after loading all data from it. This
> > > > > > allows
> > > > > > to detach UBI device from MTD device.
> > > > > 
> > > > > Okay.
> > > > > 
> > > > > > Move definition of MTD device with UBI and UBI volume with ubifs
> > > > > > filesystem
> > > > > > from global env variables ${bootubipart} and ${bootubivol} into the
> > > > > > distroboot "func" macro, defined in board include config files. UBIFS
> > > > > > distroboot macros then set ${bootubipart} and ${bootubivol} local
> > > > > > variables
> > > > > > for compatibility with existing distroboot scripts.
> > > > > 
> > > > > This might be problematic.  For a downstream board we use the 'bootubivol'
> > > > > env variable to switch between different volumes for update purposes. 
> > > > > Means on firmware update we modify U-Boot environment from Linux
> > > > > (fw_setenv) after successfully updating the inactive volume.
> > > > 
> > > > Interesting... I even did not thought about such use case as it is not
> > > > possible to do it with other boot filesystems.
> > > 
> > > Well yes, it's somewhat of a hack, but it uses existing distroboot 
> > > infrastructure.  We did comparable switching of volumes for active rootfs 
> > > without distroboot before.  On newer targets we use distroboot together with 
> > > RAUC and have one fix separate boot volume containing the boot script (the 
> > > boot script is not part of those rootfs[01] anymore), which handles loading 
> > > one or the other rootfs, no need to alter bootubivol anymore, it's constant.
> > > 
> > > > > > This last change allows to define more UBIFS target devices and make it
> > > > > > clear what is boot MTD/UBI device.
> > > > > 
> > > > > How would one switch the default UBI volume to be booted from then?
> > > > 
> > > > With this my change, it is probably not easily possible. One option is
> > > > to define more boot targets, e.g.:
> > > > 
> > > >   func(UBIFS, ubifs, 0, UBI, volume0)
> > > >   func(UBIFS, ubifs, 1, UBI, volume1)
> > > > 
> > > > And then change env ${boottargets} from ubifs0 to ubifs1, which can be
> > > > done via fs_setenv.
> > > 
> > > This should work, yes.  Maybe it's a little more complicated than this, e.g. 
> > > switching 
> > > from 'mmc0 ubifs0' to 'mmc0 ubifs1' 
> > > or 
> > > from 'ubifs0 ubifs1' to 'ubifs1 ubifs0'
> > > but that's board dependent and no big deal in general I guess.
> > 
> > Ok.
> > 
> > > > As with this change is finally possible to specify more ubifs targets
> > > > in BOOT_TARGET_DEVICES() u-boot macro.
> > > > 
> > > > This would be same as switching between other u-boot distroboot targets.
> > > 
> > > Makes sense.
> > > 
> > > > 
> > > > ...
> > > > 
> > > > But I can understand that you want to have this (probably undocumented)
> > > > behavior of switching UBI volume via env variable ${bootubivol}.
> > > > 
> > > > So what about allowing both configuration in U-Boot, with stored boot
> > > > volume in ENV and with hardcoded boot volume in config file?
> > > 
> > > Just had another look at how 'bootcmd_ubifs0' is set with your patch, and I 
> > > think with switching 'boot_targets' instead of 'bootubivol' it should work 
> > > (not tested).  Only problem would be a migration strategy for old boards or a 
> > > more sophisticated logic in Linux to determine what env variable has to be 
> > > updated, but don't let that be your concern.
> > 
> > Note that until you reset env variables to default, u-boot will
> > continue using "old" style and because all logic is stored only in env
> > (not in u-boot runtime code itself), there should be no issue with
> > upgrading u-boot to new version which uses ubifs0 and ubifs1.
> 
> Agreed.
> 
> Side note: U-Boot is not upgraded at runtime on those targets in
> question.  UBI/UBIFS is used on raw NAND flash, and with the SoC used
> it's not easily possible to upgrade U-Boot from within the running
> system in a safe way (at91bootstrap loads U-Boot from a fix offset in
> NAND flash, which makes a A/B with atomic switch or some fallback
> mechanism not feasible).  I guess there are quite some boards where
> the U-Boot binary is written once only when producing the board and
> never touched again?
> 
> However there might be a newer U-Boot version in later iterations of
> such a board, or maybe just in a later production firmware on
> otherwise unmodified hardware.
> 
> > Anyway, does it mean that we need to extend this patch to allow still
> > storing/updating ubi boot volume in env file?
> 
> I don't think so, at least not for the board I maintain.  Any upgrade
> mechanism on Linux side would have to deal with the old and the new
> variant, so it would have to detect the mechanism used on that actual
> board and use that one, be it the 'bootubivol' variable or something
> else.

Ok! So I think that my patch is then fine in the current form, right?

> > > > And which / how many boards use this functionality?
> > > 
> > > Seriously, I have no idea.  You never know what all those downstream users do, 
> > > especially in this case where functionality is in a boot script not part of U-
> > > Boot itself.  Maybe I'm the only one using it like this, maybe not.  As of 
> > > today I would recommend to use some update framework like RAUC anyways.
> > 
> > I think boards which you maintain or use.
> 
> There's one board I maintain, which uses that 'bootubivol' variable in
> the way described before.  I have another board on my TODO-List, but I
> did not decide yet how upgrades should be done on that one.
> 
> As you might have guessed already, those boards are not in upstream
> u-boot and probably never will.  I don't like that, but it's not my
> decision.

I think that it could be easier for you / your maintenance of your
boards if you have them in upstream u-boot.... But I know that managers
lot of time are not able to understand it, so sad.

> Thanks for your commments on that topic anyways, much appreciated. :-)
> 
> Greets
> Alex
> 
> > 
> > 
> > > Greets
> > > Alex
> > > 
> > > > 
> > > > > Greets
> > > > > Alex
> > > > > 
> > > > > > All board include config files are adjusted to use this new scheme of
> > > > > > specifying boot MTD/UBI device.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > ---
> > > > > > CI test passed on https://github.com/u-boot/u-boot/pull/179
> > > > > > ---
> > > > > > 
> > > > > >  include/config_distro_bootcmd.h    | 27 ++++++++++++++++-----------
> > > > > >  include/configs/am335x_guardian.h  |  3 +--
> > > > > >  include/configs/colibri-imx6ull.h  |  1 -
> > > > > >  include/configs/colibri_imx7.h     |  1 -
> > > > > >  include/configs/kontron-sl-mx6ul.h |  2 +-
> > > > > >  include/configs/mys_6ulx.h         |  2 +-
> > > > > >  include/configs/npi_imx6ull.h      |  2 +-
> > > > > >  include/configs/omap3_beagle.h     |  4 +---
> > > > > >  include/configs/omap3_evm.h        |  4 +---
> > > > > >  include/configs/pcl063.h           |  2 +-
> > > > > >  include/configs/stm32mp15_common.h |  2 +-
> > > > > >  include/configs/uniphier.h         |  2 +-
> > > > > >  12 files changed, 25 insertions(+), 27 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/config_distro_bootcmd.h
> > > > > > b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d
> > > > > > 100644
> > > > > > --- a/include/config_distro_bootcmd.h
> > > > > > +++ b/include/config_distro_bootcmd.h
> > > > > > @@ -70,18 +70,23 @@
> > > > > > 
> > > > > >  #ifdef CONFIG_CMD_UBIFS
> > > > > >  #define BOOTENV_SHARED_UBIFS \
> > > > > >  
> > > > > >  	"ubifs_boot=" \
> > > > > > 
> > > > > > -		"env exists bootubipart || " \
> > > > > > -			"env set bootubipart UBI; " \
> > > > > > -		"env exists bootubivol || " \
> > > > > > -			"env set bootubivol boot; " \
> > > > > > 
> > > > > >  		"if ubi part ${bootubipart} && " \
> > > > > > 
> > > > > > -			"ubifsmount ubi${devnum}:${bootubivol}; " \
> > > > > > +			"ubifsmount ubi0:${bootubivol}; " \
> > > > > > 
> > > > > >  		"then " \
> > > > > >  		
> > > > > >  			"devtype=ubi; " \
> > > > > > 
> > > > > > +			"devnum=ubi0; " \
> > > > > > +			"bootfstype=ubifs; " \
> > > > > > +			"distro_bootpart=${bootubivol}; " \
> > > > > > 
> > > > > >  			"run scan_dev_for_boot; " \
> > > > > > 
> > > > > > +			"ubifsumount; " \
> > > > > > 
> > > > > >  		"fi\0"
> > > > > > 
> > > > > > -#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
> > > > > > -#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
> > > > > > +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart,
> > > > > > bootubivol) \ +	"bootcmd_ubifs" #instance "=" \
> > > > > > +		"bootubipart=" #bootubipart "; " \
> > > > > > +		"bootubivol=" #bootubivol "; " \
> > > > > > +		"run ubifs_boot\0"
> > > > > > +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance,
> > > > > > bootubipart,
> > > > > > bootubivol) \ +	#devtypel #instance " "
> > > > > > 
> > > > > >  #else
> > > > > >  #define BOOTENV_SHARED_UBIFS
> > > > > >  #define BOOTENV_DEV_UBIFS \
> > > > > > 
> > > > > > @@ -411,13 +416,13 @@
> > > > > > 
> > > > > >  	BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE
> > > > > >  
> > > > > >  #endif
> > > > > > 
> > > > > > -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \
> > > > > > -	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance)
> > > > > > +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \
> > > > > > +	BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ##
> > > > > > __VA_ARGS__)
> > > > > > 
> > > > > >  #define BOOTENV_BOOT_TARGETS \
> > > > > >  
> > > > > >  	"boot_targets=" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0"
> > > > > > 
> > > > > > -#define BOOTENV_DEV(devtypeu, devtypel, instance) \
> > > > > > -	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
> > > > > > +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \
> > > > > > +	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__)
> > > > > > 
> > > > > >  #define BOOTENV \
> > > > > >  
> > > > > >  	BOOTENV_SHARED_HOST \
> > > > > >  	BOOTENV_SHARED_MMC \
> > > > > > 
> > > > > > diff --git a/include/configs/am335x_guardian.h
> > > > > > b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6
> > > > > > 100644
> > > > > > --- a/include/configs/am335x_guardian.h
> > > > > > +++ b/include/configs/am335x_guardian.h
> > > > > > @@ -29,7 +29,7 @@
> > > > > > 
> > > > > >  	"ramdisk_addr_r=0x88080000\0" \
> > > > > >  
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0)
> > > > > > +	func(UBIFS, ubifs, 0, UBI, rootfs)
> > > > > > 
> > > > > >  #define AM335XX_BOARD_FDTFILE "fdtfile=" CONFIG_DEFAULT_DEVICE_TREE
> > > > > > 
> > > > > > ".dtb\0"
> > > > > > 
> > > > > > @@ -54,7 +54,6 @@
> > > > > > 
> > > > > >  	GUARDIAN_DEFAULT_PROD_ENV \
> > > > > >  	"autoload=no\0" \
> > > > > >  	"backlight_brightness=50\0" \
> > > > > > 
> > > > > > -	"bootubivol=rootfs\0" \
> > > > > > 
> > > > > >  	"distro_bootcmd=" \
> > > > > >  	
> > > > > >  		"setenv rootflags \"bulk_read,chk_data_crc\"; " \
> > > > > >  		"setenv ethact usb_ether; " \
> > > > > > 
> > > > > > diff --git a/include/configs/colibri-imx6ull.h
> > > > > > b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0
> > > > > > 100644
> > > > > > --- a/include/configs/colibri-imx6ull.h
> > > > > > +++ b/include/configs/colibri-imx6ull.h
> > > > > > @@ -91,7 +91,6 @@
> > > > > > 
> > > > > >  	UBI_BOOTCMD \
> > > > > >  	UBOOT_UPDATE \
> > > > > >  	"boot_script_dhcp=boot.scr\0" \
> > > > > > 
> > > > > > -	"bootubipart=ubi\0" \
> > > > > > 
> > > > > >  	"console=ttymxc0\0" \
> > > > > >  	"defargs=user_debug=30\0" \
> > > > > >  	"fdt_board=eval-v3\0" \
> > > > > > 
> > > > > > diff --git a/include/configs/colibri_imx7.h
> > > > > > b/include/configs/colibri_imx7.h index 3dba7bcef258..3cad17777975
> > > > > > 100644
> > > > > > --- a/include/configs/colibri_imx7.h
> > > > > > +++ b/include/configs/colibri_imx7.h
> > > > > > @@ -131,7 +131,6 @@
> > > > > > 
> > > > > >  	UBOOT_UPDATE \
> > > > > >  	"boot_file=zImage\0" \
> > > > > >  	"boot_script_dhcp=boot.scr\0" \
> > > > > > 
> > > > > > -	"bootubipart=ubi\0" \
> > > > > > 
> > > > > >  	"console=ttymxc0\0" \
> > > > > >  	"defargs=\0" \
> > > > > >  	"fdt_board=eval-v3\0" \
> > > > > > 
> > > > > > diff --git a/include/configs/kontron-sl-mx6ul.h
> > > > > > b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75
> > > > > > 100644
> > > > > > --- a/include/configs/kontron-sl-mx6ul.h
> > > > > > +++ b/include/configs/kontron-sl-mx6ul.h
> > > > > > @@ -45,7 +45,7 @@
> > > > > > 
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > >  
> > > > > >  	func(MMC, mmc, 1) \
> > > > > >  	func(MMC, mmc, 0) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0) \
> > > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > > 
> > > > > >  	func(USB, usb, 0) \
> > > > > >  	func(PXE, pxe, na) \
> > > > > >  	func(DHCP, dhcp, na)
> > > > > > 
> > > > > > diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h
> > > > > > index 6801fc109eae..663820177a3e 100644
> > > > > > --- a/include/configs/mys_6ulx.h
> > > > > > +++ b/include/configs/mys_6ulx.h
> > > > > > @@ -59,7 +59,7 @@
> > > > > > 
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > >  
> > > > > >  	func(MMC, mmc, 0) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0) \
> > > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > > 
> > > > > >  	func(PXE, pxe, na) \
> > > > > >  	func(DHCP, dhcp, na)
> > > > > > 
> > > > > > diff --git a/include/configs/npi_imx6ull.h
> > > > > > b/include/configs/npi_imx6ull.h
> > > > > > index c250fa650603..ebb887544e08 100644
> > > > > > --- a/include/configs/npi_imx6ull.h
> > > > > > +++ b/include/configs/npi_imx6ull.h
> > > > > > @@ -67,7 +67,7 @@
> > > > > > 
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > >  
> > > > > >  	func(MMC, mmc, 0) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0) \
> > > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > > 
> > > > > >  	func(PXE, pxe, na) \
> > > > > >  	func(DHCP, dhcp, na)
> > > > > > 
> > > > > > diff --git a/include/configs/omap3_beagle.h
> > > > > > b/include/configs/omap3_beagle.h index 158773acedb9..f5da08cf3359
> > > > > > 100644
> > > > > > --- a/include/configs/omap3_beagle.h
> > > > > > +++ b/include/configs/omap3_beagle.h
> > > > > > @@ -65,7 +65,7 @@
> > > > > > 
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > >  
> > > > > >  	func(MMC, mmc, 0) \
> > > > > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0) \
> > > > > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > > > > 
> > > > > >  	func(NAND, nand, 0)
> > > > > >  
> > > > > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > > > > 
> > > > > > @@ -87,8 +87,6 @@
> > > > > > 
> > > > > >  	"bootenv=uEnv.txt\0" \
> > > > > >  	"bootfile=zImage\0" \
> > > > > >  	"bootpart=0:2\0" \
> > > > > > 
> > > > > > -	"bootubivol=rootfs\0" \
> > > > > > -	"bootubipart=rootfs\0" \
> > > > > > 
> > > > > >  	"usbtty=cdc_acm\0" \
> > > > > >  	"mpurate=auto\0" \
> > > > > >  	"buddy=none\0" \
> > > > > > 
> > > > > > diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
> > > > > > index eeb9ef8c741a..cc98e03096ab 100644
> > > > > > --- a/include/configs/omap3_evm.h
> > > > > > +++ b/include/configs/omap3_evm.h
> > > > > > @@ -60,7 +60,7 @@
> > > > > > 
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > >  
> > > > > >  	func(MMC, mmc, 0) \
> > > > > >  	func(LEGACY_MMC, legacy_mmc, 0) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0) \
> > > > > > +	func(UBIFS, ubifs, 0, rootfs, rootfs) \
> > > > > > 
> > > > > >  	func(NAND, nand, 0)
> > > > > >  
> > > > > >  #else /* !CONFIG_MTD_RAW_NAND */
> > > > > > 
> > > > > > @@ -88,8 +88,6 @@
> > > > > > 
> > > > > >  	"bootenv=uEnv.txt\0" \
> > > > > >  	"bootfile=zImage\0" \
> > > > > >  	"bootpart=0:2\0" \
> > > > > > 
> > > > > > -	"bootubivol=rootfs\0" \
> > > > > > -	"bootubipart=rootfs\0" \
> > > > > > 
> > > > > >  	"optargs=\0" \
> > > > > >  	"nandroot=ubi0:rootfs ubi.mtd=rootfs rw noinitrd\0" \
> > > > > >  	"nandrootfstype=ubifs rootwait\0" \
> > > > > > 
> > > > > > diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h
> > > > > > index 31b7d07a24cd..c3f7e7eb2c4b 100644
> > > > > > --- a/include/configs/pcl063.h
> > > > > > +++ b/include/configs/pcl063.h
> > > > > > @@ -71,7 +71,7 @@
> > > > > > 
> > > > > >  #define BOOT_TARGET_DEVICES(func) \
> > > > > >  
> > > > > >  	func(MMC, mmc, 0) \
> > > > > > 
> > > > > > -	func(UBIFS, ubifs, 0) \
> > > > > > +	func(UBIFS, ubifs, 0, UBI, boot) \
> > > > > > 
> > > > > >  	func(PXE, pxe, na) \
> > > > > >  	func(DHCP, dhcp, na)
> > > > > > 
> > > > > > diff --git a/include/configs/stm32mp15_common.h
> > > > > > b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98
> > > > > > 100644
> > > > > > --- a/include/configs/stm32mp15_common.h
> > > > > > +++ b/include/configs/stm32mp15_common.h
> > > > > > @@ -77,7 +77,7 @@
> > > > > > 
> > > > > >  #endif
> > > > > >  
> > > > > >  #ifdef CONFIG_CMD_UBIFS
> > > > > > 
> > > > > > -#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > > > > +#define BOOT_TARGET_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, boot)
> > > > > > 
> > > > > >  #else
> > > > > >  #define BOOT_TARGET_UBIFS(func)
> > > > > >  #endif
> > > > > > 
> > > > > > diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h
> > > > > > index f813f88cdd7a..640a29067d85 100644
> > > > > > --- a/include/configs/uniphier.h
> > > > > > +++ b/include/configs/uniphier.h
> > > > > > @@ -20,7 +20,7 @@
> > > > > > 
> > > > > >  #endif
> > > > > >  
> > > > > >  #ifdef CONFIG_CMD_UBIFS
> > > > > > 
> > > > > > -#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0)
> > > > > > +#define BOOT_TARGET_DEVICE_UBIFS(func)	func(UBIFS, ubifs, 0, UBI, 
> > > boot)
> > > > > > 
> > > > > >  #else
> > > > > >  #define BOOT_TARGET_DEVICE_UBIFS(func)
> > > > > >  #endif
> > > 
> > > 
> > > 
> > > 
> > >

On Wednesday 29 June 2022 12:29:40 Frieder Schrempf wrote:
> Am 23.06.22 um 18:09 schrieb Pali Rohár:
> > On Tuesday 31 May 2022 10:32:36 Pali Rohár wrote:
> >> Fix multiple issues in ubifs distroboot code:
> >>
> >> U-Boot supports attaching only one MTD device as UBI at the time. So
> >> always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> >> ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> >> command attach MTD device always as UBI device ubi0.
> >>
> >> Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> >> Distroboot scripts require ${bootfstype} variable to be properly set and it
> >> is already set for all other boot types.
> >>
> >> Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> >> device does not have partitions, but has volumes. Distroboot scripts
> >> require something to be set in ${distro_bootpart} variable, so set it to
> >> the UBI volume which is currently mounted by ubifs.
> >>
> >> Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> >> differs from the other partition code that it requires "ubi" prefix before
> >> number.
> >>
> >> Explicitly unmount ubifs volume after loading all data from it. This allows
> >> to detach UBI device from MTD device.
> >>
> >> Move definition of MTD device with UBI and UBI volume with ubifs filesystem
> >> from global env variables ${bootubipart} and ${bootubivol} into the
> >> distroboot "func" macro, defined in board include config files. UBIFS
> >> distroboot macros then set ${bootubipart} and ${bootubivol} local variables
> >> for compatibility with existing distroboot scripts.
> >>
> >> This last change allows to define more UBIFS target devices and make it
> >> clear what is boot MTD/UBI device.
> >>
> >> All board include config files are adjusted to use this new scheme of
> >> specifying boot MTD/UBI device.
> >>
> >> Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > PING?
> > 
> 
> Sorry, I currently don't have the time to properly review and/or test
> this. Though, in general the changes look good and I can at least provide:
> 
> Acked-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> I think it would help to split this up in smaller chunks, so people can
> review it more easily...

Splitting these changes is hard as it needs to be done atomtically to do
not break boards or do not break compilation.

I think it is better to have one bigger change than lot of smaller which
cause compile or runtime errors until all small changes are applied.

On Tue, May 31, 2022 at 10:32:36AM +0200, Pali Rohár wrote:

> Fix multiple issues in ubifs distroboot code:
> 
> U-Boot supports attaching only one MTD device as UBI at the time. So
> always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume
> ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part'
> command attach MTD device always as UBI device ubi0.
> 
> Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command.
> Distroboot scripts require ${bootfstype} variable to be properly set and it
> is already set for all other boot types.
> 
> Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI
> device does not have partitions, but has volumes. Distroboot scripts
> require something to be set in ${distro_bootpart} variable, so set it to
> the UBI volume which is currently mounted by ubifs.
> 
> Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code
> differs from the other partition code that it requires "ubi" prefix before
> number.
> 
> Explicitly unmount ubifs volume after loading all data from it. This allows
> to detach UBI device from MTD device.
> 
> Move definition of MTD device with UBI and UBI volume with ubifs filesystem
> from global env variables ${bootubipart} and ${bootubivol} into the
> distroboot "func" macro, defined in board include config files. UBIFS
> distroboot macros then set ${bootubipart} and ${bootubivol} local variables
> for compatibility with existing distroboot scripts.
> 
> This last change allows to define more UBIFS target devices and make it
> clear what is boot MTD/UBI device.
> 
> All board include config files are adjusted to use this new scheme of
> specifying boot MTD/UBI device.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Acked-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Applied to u-boot/next, thanks!