[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[Michal Simek]

Detect mmc alias at run time for setting up proper boot_targets sequence.
The first target has to correspond with boot mode.

The purpose of this patch is to get rid of CONFIG_ZYNQ_SDHCI0/1
parameters in full U-Boot.
Unfortunately this patch can't remove it because there is missing
mmc implementation for SPL_DM_SEQ_ALIAS.

Also xilinx_zynqmp.h only setup boot commands for mmc0 and mmc1.
It means using aliases with higher number won't work. But switching
between mmc0 and mmc1 should work properly.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Not sure how exactly to tune BOOT_TARGET_DEVICES_MMC to have functions
for different aliases ID. I can simply setup devnum based on dev->seq
and also generate the whole bootcmd_mmc0

bootcmd_mmc0=setenv devnum 0; run mmc_boot
bootcmd_mmc1=setenv devnum 1; run mmc_boot

The second patch is doing that.
But still if you setup alias to higher number mmc core is not mmc dev
command is not able to work with it.
---
 board/xilinx/zynqmp/zynqmp.c | 45 ++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index f7fe1fdfff7b..96ea0f578d30 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -16,6 +16,7 @@
 #include <asm/arch/sys_proto.h>
 #include <asm/arch/psu_init_gpl.h>
 #include <asm/io.h>
+#include <dm/device.h>
 #include <dm/uclass.h>
 #include <usb.h>
 #include <dwc3-uboot.h>
@@ -454,6 +455,9 @@ int board_late_init(void)
 {
 	u32 reg = 0;
 	u8 bootmode;
+	struct udevice *dev;
+	int bootseq = -1;
+	int bootseq_len = 0;
 	int env_targets_len = 0;
 	const char *mode;
 	char *new_targets;
@@ -499,7 +503,15 @@ int board_late_init(void)
 		break;
 	case SD_MODE:
 		puts("SD_MODE\n");
-		mode = "mmc0";
+		if (uclass_get_device_by_name(UCLASS_MMC,
+					      "sdhci at ff160000", &dev)) {
+			puts("Boot from SD0 but without SD0 enabled!\n");
+			return -1;
+		}
+		debug("mmc0 device found at %p, seq %d\n", dev, dev->seq);
+
+		mode = "mmc";
+		bootseq = dev->seq;
 		env_set("modeboot", "sdboot");
 		break;
 	case SD1_LSHFT_MODE:
@@ -507,12 +519,15 @@ int board_late_init(void)
 		/* fall through */
 	case SD_MODE1:
 		puts("SD_MODE1\n");
-#if defined(CONFIG_ZYNQ_SDHCI0) && defined(CONFIG_ZYNQ_SDHCI1)
-		mode = "mmc1";
-		env_set("sdbootdev", "1");
-#else
-		mode = "mmc0";
-#endif
+		if (uclass_get_device_by_name(UCLASS_MMC,
+					      "sdhci at ff170000", &dev)) {
+			puts("Boot from SD1 but without SD1 enabled!\n");
+			return -1;
+		}
+		debug("mmc1 device found at %p, seq %d\n", dev, dev->seq);
+
+		mode = "mmc";
+		bootseq = dev->seq;
 		env_set("modeboot", "sdboot");
 		break;
 	case NAND_MODE:
@@ -526,6 +541,11 @@ int board_late_init(void)
 		break;
 	}
 
+	if (bootseq >= 0) {
+		bootseq_len = snprintf(NULL, 0, "%i", bootseq);
+		debug("Bootseq len: %x\n", bootseq_len);
+	}
+
 	/*
 	 * One terminating char + one byte for space between mode
 	 * and default boot_targets
@@ -534,10 +554,15 @@ int board_late_init(void)
 	if (env_targets)
 		env_targets_len = strlen(env_targets);
 
-	new_targets = calloc(1, strlen(mode) + env_targets_len + 2);
+	new_targets = calloc(1, strlen(mode) + env_targets_len + 2 +
+			     bootseq_len);
 
-	sprintf(new_targets, "%s %s", mode,
-		env_targets ? env_targets : "");
+	if (bootseq >= 0)
+		sprintf(new_targets, "%s%x %s", mode, bootseq,
+			env_targets ? env_targets : "");
+	else
+		sprintf(new_targets, "%s %s", mode,
+			env_targets ? env_targets : "");
 
 	env_set("boot_targets", new_targets);
 
-- 
2.17.0

[U-Boot] [RFC PATCH 2/2] arm64: zynqmp: Try to create bootcm_mmcX at run time

[Michal Simek]

Just an attept to create boot commands for mmcs above 2 which is not
generated.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Probably some ifdefs around are missing.
---
 board/xilinx/zynqmp/zynqmp.c    | 28 ++++++++++++++++++++++++++++
 include/configs/xilinx_zynqmp.h |  7 -------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index 96ea0f578d30..37fa8f4f4d3f 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -451,6 +451,32 @@ void reset_cpu(ulong addr)
 {
 }
 
+static int create_mmc_boot_commands(void)
+{
+/* Size of strings below + one terminating char \0 + 3 possitions for seq */
+#define MAX_COMMAND_LEN	15
+#define MAX_BODY_LEN	32
+
+	int ret;
+	struct uclass *uc;
+	struct udevice *dev;
+	char body[MAX_BODY_LEN];
+	char command[MAX_COMMAND_LEN];
+
+	ret = uclass_get(UCLASS_MMC, &uc);
+	if (ret)
+		return ret;
+
+	uclass_foreach_dev(dev, uc) {
+		snprintf(body, MAX_BODY_LEN, "setenv devnum %x; run mmc_boot",
+			 dev->seq);
+		snprintf(command, MAX_COMMAND_LEN, "bootcmd_mmc%x", dev->seq);
+		env_set(command, body);
+	}
+
+	return 0;
+}
+
 int board_late_init(void)
 {
 	u32 reg = 0;
@@ -546,6 +572,8 @@ int board_late_init(void)
 		debug("Bootseq len: %x\n", bootseq_len);
 	}
 
+	create_mmc_boot_commands();
+
 	/*
 	 * One terminating char + one byte for space between mode
 	 * and default boot_targets
diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h
index c8a0dbb7e3b5..a2590998429e 100644
--- a/include/configs/xilinx_zynqmp.h
+++ b/include/configs/xilinx_zynqmp.h
@@ -155,12 +155,6 @@
 	"scriptaddr=0x02000000\0" \
 	"ramdisk_addr_r=0x02100000\0" \
 
-#if defined(CONFIG_MMC_SDHCI_ZYNQ)
-# define BOOT_TARGET_DEVICES_MMC(func)	func(MMC, mmc, 0) func(MMC, mmc, 1)
-#else
-# define BOOT_TARGET_DEVICES_MMC(func)
-#endif
-
 #if defined(CONFIG_SATA_CEVA)
 # define BOOT_TARGET_DEVICES_SCSI(func)	func(SCSI, scsi, 0)
 #else
@@ -186,7 +180,6 @@
 #endif
 
 #define BOOT_TARGET_DEVICES(func) \
-	BOOT_TARGET_DEVICES_MMC(func) \
 	BOOT_TARGET_DEVICES_USB(func) \
 	BOOT_TARGET_DEVICES_SCSI(func) \
 	BOOT_TARGET_DEVICES_PXE(func) \
-- 
2.17.0

[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[Alexander Graf]

On 25.04.18 14:38, Michal Simek wrote:
> Detect mmc alias at run time for setting up proper boot_targets sequence.
> The first target has to correspond with boot mode.
> 
> The purpose of this patch is to get rid of CONFIG_ZYNQ_SDHCI0/1
> parameters in full U-Boot.
> Unfortunately this patch can't remove it because there is missing
> mmc implementation for SPL_DM_SEQ_ALIAS.
> 
> Also xilinx_zynqmp.h only setup boot commands for mmc0 and mmc1.
> It means using aliases with higher number won't work. But switching
> between mmc0 and mmc1 should work properly.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Not sure how exactly to tune BOOT_TARGET_DEVICES_MMC to have functions
> for different aliases ID. I can simply setup devnum based on dev->seq
> and also generate the whole bootcmd_mmc0
> 
> bootcmd_mmc0=setenv devnum 0; run mmc_boot
> bootcmd_mmc1=setenv devnum 1; run mmc_boot
> 
> The second patch is doing that.
> But still if you setup alias to higher number mmc core is not mmc dev
> command is not able to work with it.

I don't understand this sentence.

> ---
>  board/xilinx/zynqmp/zynqmp.c | 45 ++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index f7fe1fdfff7b..96ea0f578d30 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -16,6 +16,7 @@
>  #include <asm/arch/sys_proto.h>
>  #include <asm/arch/psu_init_gpl.h>
>  #include <asm/io.h>
> +#include <dm/device.h>
>  #include <dm/uclass.h>
>  #include <usb.h>
>  #include <dwc3-uboot.h>
> @@ -454,6 +455,9 @@ int board_late_init(void)
>  {
>  	u32 reg = 0;
>  	u8 bootmode;
> +	struct udevice *dev;
> +	int bootseq = -1;
> +	int bootseq_len = 0;
>  	int env_targets_len = 0;
>  	const char *mode;
>  	char *new_targets;
> @@ -499,7 +503,15 @@ int board_late_init(void)
>  		break;
>  	case SD_MODE:
>  		puts("SD_MODE\n");
> -		mode = "mmc0";
> +		if (uclass_get_device_by_name(UCLASS_MMC,
> +					      "sdhci at ff160000", &dev)) {

I think you need to check whether probing actually succeeded, no?
Otherwise seq is invalid (-1).

So this should be if (uclass...() || (dev && dev->flags &
DM_FLAG_ACTIVATED))

> +			puts("Boot from SD0 but without SD0 enabled!\n");
> +			return -1;
> +		}
> +		debug("mmc0 device found at %p, seq %d\n", dev, dev->seq);
> +
> +		mode = "mmc";
> +		bootseq = dev->seq;
>  		env_set("modeboot", "sdboot");
>  		break;
>  	case SD1_LSHFT_MODE:
> @@ -507,12 +519,15 @@ int board_late_init(void)
>  		/* fall through */
>  	case SD_MODE1:
>  		puts("SD_MODE1\n");
> -#if defined(CONFIG_ZYNQ_SDHCI0) && defined(CONFIG_ZYNQ_SDHCI1)
> -		mode = "mmc1";
> -		env_set("sdbootdev", "1");
> -#else
> -		mode = "mmc0";
> -#endif
> +		if (uclass_get_device_by_name(UCLASS_MMC,
> +					      "sdhci at ff170000", &dev)) {
> +			puts("Boot from SD1 but without SD1 enabled!\n");
> +			return -1;
> +		}
> +		debug("mmc1 device found at %p, seq %d\n", dev, dev->seq);
> +
> +		mode = "mmc";
> +		bootseq = dev->seq;
>  		env_set("modeboot", "sdboot");
>  		break;
>  	case NAND_MODE:
> @@ -526,6 +541,11 @@ int board_late_init(void)
>  		break;
>  	}
>  
> +	if (bootseq >= 0) {
> +		bootseq_len = snprintf(NULL, 0, "%i", bootseq);

This seems like quite a heavy way to figure out the number of digits of
an integer ;).


Alex

> +		debug("Bootseq len: %x\n", bootseq_len);
> +	}
> +
>  	/*
>  	 * One terminating char + one byte for space between mode
>  	 * and default boot_targets
> @@ -534,10 +554,15 @@ int board_late_init(void)
>  	if (env_targets)
>  		env_targets_len = strlen(env_targets);
>  
> -	new_targets = calloc(1, strlen(mode) + env_targets_len + 2);
> +	new_targets = calloc(1, strlen(mode) + env_targets_len + 2 +
> +			     bootseq_len);
>  
> -	sprintf(new_targets, "%s %s", mode,
> -		env_targets ? env_targets : "");
> +	if (bootseq >= 0)
> +		sprintf(new_targets, "%s%x %s", mode, bootseq,
> +			env_targets ? env_targets : "");
> +	else
> +		sprintf(new_targets, "%s %s", mode,
> +			env_targets ? env_targets : "");
>  
>  	env_set("boot_targets", new_targets);
>  
> 

[U-Boot] [RFC PATCH 2/2] arm64: zynqmp: Try to create bootcm_mmcX at run time

[Alexander Graf]

On 25.04.18 14:38, Michal Simek wrote:
> Just an attept to create boot commands for mmcs above 2 which is not
> generated.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Probably some ifdefs around are missing.
> ---
>  board/xilinx/zynqmp/zynqmp.c    | 28 ++++++++++++++++++++++++++++
>  include/configs/xilinx_zynqmp.h |  7 -------
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 96ea0f578d30..37fa8f4f4d3f 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -451,6 +451,32 @@ void reset_cpu(ulong addr)
>  {
>  }
>  
> +static int create_mmc_boot_commands(void)
> +{
> +/* Size of strings below + one terminating char \0 + 3 possitions for seq */
> +#define MAX_COMMAND_LEN	15
> +#define MAX_BODY_LEN	32
> +
> +	int ret;
> +	struct uclass *uc;
> +	struct udevice *dev;
> +	char body[MAX_BODY_LEN];
> +	char command[MAX_COMMAND_LEN];
> +
> +	ret = uclass_get(UCLASS_MMC, &uc);
> +	if (ret)
> +		return ret;
> +
> +	uclass_foreach_dev(dev, uc) {
> +		snprintf(body, MAX_BODY_LEN, "setenv devnum %x; run mmc_boot",
> +			 dev->seq);
> +		snprintf(command, MAX_COMMAND_LEN, "bootcmd_mmc%x", dev->seq);
> +		env_set(command, body);
> +	}

Why not just remove the ones you don't need? You could leave the
template in and just remove bootcmd_mmc0 if you don't see an mmc0 device.


Alex

> +
> +	return 0;
> +}
> +
>  int board_late_init(void)
>  {
>  	u32 reg = 0;
> @@ -546,6 +572,8 @@ int board_late_init(void)
>  		debug("Bootseq len: %x\n", bootseq_len);
>  	}
>  
> +	create_mmc_boot_commands();
> +
>  	/*
>  	 * One terminating char + one byte for space between mode
>  	 * and default boot_targets
> diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h
> index c8a0dbb7e3b5..a2590998429e 100644
> --- a/include/configs/xilinx_zynqmp.h
> +++ b/include/configs/xilinx_zynqmp.h
> @@ -155,12 +155,6 @@
>  	"scriptaddr=0x02000000\0" \
>  	"ramdisk_addr_r=0x02100000\0" \
>  
> -#if defined(CONFIG_MMC_SDHCI_ZYNQ)
> -# define BOOT_TARGET_DEVICES_MMC(func)	func(MMC, mmc, 0) func(MMC, mmc, 1)
> -#else
> -# define BOOT_TARGET_DEVICES_MMC(func)
> -#endif
> -
>  #if defined(CONFIG_SATA_CEVA)
>  # define BOOT_TARGET_DEVICES_SCSI(func)	func(SCSI, scsi, 0)
>  #else
> @@ -186,7 +180,6 @@
>  #endif
>  
>  #define BOOT_TARGET_DEVICES(func) \
> -	BOOT_TARGET_DEVICES_MMC(func) \
>  	BOOT_TARGET_DEVICES_USB(func) \
>  	BOOT_TARGET_DEVICES_SCSI(func) \
>  	BOOT_TARGET_DEVICES_PXE(func) \
> 

[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[Michal Simek]

On 26.4.2018 08:14, Alexander Graf wrote:
> 
> 
> On 25.04.18 14:38, Michal Simek wrote:
>> Detect mmc alias at run time for setting up proper boot_targets sequence.
>> The first target has to correspond with boot mode.
>>
>> The purpose of this patch is to get rid of CONFIG_ZYNQ_SDHCI0/1
>> parameters in full U-Boot.
>> Unfortunately this patch can't remove it because there is missing
>> mmc implementation for SPL_DM_SEQ_ALIAS.
>>
>> Also xilinx_zynqmp.h only setup boot commands for mmc0 and mmc1.
>> It means using aliases with higher number won't work. But switching
>> between mmc0 and mmc1 should work properly.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Not sure how exactly to tune BOOT_TARGET_DEVICES_MMC to have functions
>> for different aliases ID. I can simply setup devnum based on dev->seq
>> and also generate the whole bootcmd_mmc0
>>
>> bootcmd_mmc0=setenv devnum 0; run mmc_boot
>> bootcmd_mmc1=setenv devnum 1; run mmc_boot
>>
>> The second patch is doing that.
>> But still if you setup alias to higher number mmc core is not mmc dev
>> command is not able to work with it.
> 
> I don't understand this sentence.

Imagine case that you have mmc1000 = &sdhci0;
That 1000 is integer and u-boot is in hex that's why 1000 = 0x3e8.

It means boot_targets will start with "mmc3e8 +static one"

That's why
bootcmd_mmc3e8=setenv devnum 3e8; run mmc_boot

needs to be generated. But xilinx_zynqmp.h is only setting these lines
for mmc0 and mmc1.

> 
>> ---
>>  board/xilinx/zynqmp/zynqmp.c | 45 ++++++++++++++++++++++++++++--------
>>  1 file changed, 35 insertions(+), 10 deletions(-)
>>
>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
>> index f7fe1fdfff7b..96ea0f578d30 100644
>> --- a/board/xilinx/zynqmp/zynqmp.c
>> +++ b/board/xilinx/zynqmp/zynqmp.c
>> @@ -16,6 +16,7 @@
>>  #include <asm/arch/sys_proto.h>
>>  #include <asm/arch/psu_init_gpl.h>
>>  #include <asm/io.h>
>> +#include <dm/device.h>
>>  #include <dm/uclass.h>
>>  #include <usb.h>
>>  #include <dwc3-uboot.h>
>> @@ -454,6 +455,9 @@ int board_late_init(void)
>>  {
>>  	u32 reg = 0;
>>  	u8 bootmode;
>> +	struct udevice *dev;
>> +	int bootseq = -1;
>> +	int bootseq_len = 0;
>>  	int env_targets_len = 0;
>>  	const char *mode;
>>  	char *new_targets;
>> @@ -499,7 +503,15 @@ int board_late_init(void)
>>  		break;
>>  	case SD_MODE:
>>  		puts("SD_MODE\n");
>> -		mode = "mmc0";
>> +		if (uclass_get_device_by_name(UCLASS_MMC,
>> +					      "sdhci at ff160000", &dev)) {
> 
> I think you need to check whether probing actually succeeded, no?
> Otherwise seq is invalid (-1).
> 
> So this should be if (uclass...() || (dev && dev->flags &
> DM_FLAG_ACTIVATED))

tbh this needs to be tested by sw rewriting boot modes before this is
called.


> 
>> +			puts("Boot from SD0 but without SD0 enabled!\n");
>> +			return -1;
>> +		}
>> +		debug("mmc0 device found at %p, seq %d\n", dev, dev->seq);
>> +
>> +		mode = "mmc";
>> +		bootseq = dev->seq;
>>  		env_set("modeboot", "sdboot");
>>  		break;
>>  	case SD1_LSHFT_MODE:
>> @@ -507,12 +519,15 @@ int board_late_init(void)
>>  		/* fall through */
>>  	case SD_MODE1:
>>  		puts("SD_MODE1\n");
>> -#if defined(CONFIG_ZYNQ_SDHCI0) && defined(CONFIG_ZYNQ_SDHCI1)
>> -		mode = "mmc1";
>> -		env_set("sdbootdev", "1");
>> -#else
>> -		mode = "mmc0";
>> -#endif
>> +		if (uclass_get_device_by_name(UCLASS_MMC,
>> +					      "sdhci at ff170000", &dev)) {
>> +			puts("Boot from SD1 but without SD1 enabled!\n");
>> +			return -1;
>> +		}
>> +		debug("mmc1 device found at %p, seq %d\n", dev, dev->seq);
>> +
>> +		mode = "mmc";
>> +		bootseq = dev->seq;
>>  		env_set("modeboot", "sdboot");
>>  		break;
>>  	case NAND_MODE:
>> @@ -526,6 +541,11 @@ int board_late_init(void)
>>  		break;
>>  	}
>>  
>> +	if (bootseq >= 0) {
>> +		bootseq_len = snprintf(NULL, 0, "%i", bootseq);
> 
> This seems like quite a heavy way to figure out the number of digits of
> an integer ;).

Recursive function can be used too. But this is kind of nice even I
agree that it takes more time that for example this


static int intlen(u32 i)
{
	if (i < 10)
		return 1;

	return 1 + intlen(i / 10);
}

Thanks,
Michal

[U-Boot] [RFC PATCH 2/2] arm64: zynqmp: Try to create bootcm_mmcX at run time

[Michal Simek]

On 26.4.2018 08:23, Alexander Graf wrote:
> 
> 
> On 25.04.18 14:38, Michal Simek wrote:
>> Just an attept to create boot commands for mmcs above 2 which is not
>> generated.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Probably some ifdefs around are missing.
>> ---
>>  board/xilinx/zynqmp/zynqmp.c    | 28 ++++++++++++++++++++++++++++
>>  include/configs/xilinx_zynqmp.h |  7 -------
>>  2 files changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
>> index 96ea0f578d30..37fa8f4f4d3f 100644
>> --- a/board/xilinx/zynqmp/zynqmp.c
>> +++ b/board/xilinx/zynqmp/zynqmp.c
>> @@ -451,6 +451,32 @@ void reset_cpu(ulong addr)
>>  {
>>  }
>>  
>> +static int create_mmc_boot_commands(void)
>> +{
>> +/* Size of strings below + one terminating char \0 + 3 possitions for seq */
>> +#define MAX_COMMAND_LEN	15
>> +#define MAX_BODY_LEN	32
>> +
>> +	int ret;
>> +	struct uclass *uc;
>> +	struct udevice *dev;
>> +	char body[MAX_BODY_LEN];
>> +	char command[MAX_COMMAND_LEN];
>> +
>> +	ret = uclass_get(UCLASS_MMC, &uc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	uclass_foreach_dev(dev, uc) {
>> +		snprintf(body, MAX_BODY_LEN, "setenv devnum %x; run mmc_boot",
>> +			 dev->seq);
>> +		snprintf(command, MAX_COMMAND_LEN, "bootcmd_mmc%x", dev->seq);
>> +		env_set(command, body);
>> +	}
> 
> Why not just remove the ones you don't need? You could leave the
> template in and just remove bootcmd_mmc0 if you don't see an mmc0 device.

It is not a problem if that alias is mmc0 or mmc1 but if that alias is
mmc3 because none is generating that distro default boot command line.
Look at my reply on 1/2

M

[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[Alexander Graf]

> Am 26.04.2018 um 08:25 schrieb Michal Simek <michal.simek@xilinx.com>:
> 
>> On 26.4.2018 08:14, Alexander Graf wrote:
>> 
>> 
>>> On 25.04.18 14:38, Michal Simek wrote:
>>> Detect mmc alias at run time for setting up proper boot_targets sequence.
>>> The first target has to correspond with boot mode.
>>> 
>>> The purpose of this patch is to get rid of CONFIG_ZYNQ_SDHCI0/1
>>> parameters in full U-Boot.
>>> Unfortunately this patch can't remove it because there is missing
>>> mmc implementation for SPL_DM_SEQ_ALIAS.
>>> 
>>> Also xilinx_zynqmp.h only setup boot commands for mmc0 and mmc1.
>>> It means using aliases with higher number won't work. But switching
>>> between mmc0 and mmc1 should work properly.
>>> 
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>> ---
>>> 
>>> Not sure how exactly to tune BOOT_TARGET_DEVICES_MMC to have functions
>>> for different aliases ID. I can simply setup devnum based on dev->seq
>>> and also generate the whole bootcmd_mmc0
>>> 
>>> bootcmd_mmc0=setenv devnum 0; run mmc_boot
>>> bootcmd_mmc1=setenv devnum 1; run mmc_boot
>>> 
>>> The second patch is doing that.
>>> But still if you setup alias to higher number mmc core is not mmc dev
>>> command is not able to work with it.
>> 
>> I don't understand this sentence.
> 
> Imagine case that you have mmc1000 = &sdhci0;
> That 1000 is integer and u-boot is in hex that's why 1000 = 0x3e8.
> 
> It means boot_targets will start with "mmc3e8 +static one"
> 
> That's why
> bootcmd_mmc3e8=setenv devnum 3e8; run mmc_boot
> 
> needs to be generated. But xilinx_zynqmp.h is only setting these lines
> for mmc0 and mmc1.

Ok, so what we really want is a distro.c file and corresponding boot_target lines that generate boot target variables for every device enumerated in the system on boot.

The reason we don‘t have that yet I guess is because most targets target one SoC/board that has a known set of devices. The more generic we make things, the more we have to enumerate at runtime.


Alex

> 
>> 
>>> ---
>>> board/xilinx/zynqmp/zynqmp.c | 45 ++++++++++++++++++++++++++++--------
>>> 1 file changed, 35 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
>>> index f7fe1fdfff7b..96ea0f578d30 100644
>>> --- a/board/xilinx/zynqmp/zynqmp.c
>>> +++ b/board/xilinx/zynqmp/zynqmp.c
>>> @@ -16,6 +16,7 @@
>>> #include <asm/arch/sys_proto.h>
>>> #include <asm/arch/psu_init_gpl.h>
>>> #include <asm/io.h>
>>> +#include <dm/device.h>
>>> #include <dm/uclass.h>
>>> #include <usb.h>
>>> #include <dwc3-uboot.h>
>>> @@ -454,6 +455,9 @@ int board_late_init(void)
>>> {
>>>    u32 reg = 0;
>>>    u8 bootmode;
>>> +    struct udevice *dev;
>>> +    int bootseq = -1;
>>> +    int bootseq_len = 0;
>>>    int env_targets_len = 0;
>>>    const char *mode;
>>>    char *new_targets;
>>> @@ -499,7 +503,15 @@ int board_late_init(void)
>>>        break;
>>>    case SD_MODE:
>>>        puts("SD_MODE\n");
>>> -        mode = "mmc0";
>>> +        if (uclass_get_device_by_name(UCLASS_MMC,
>>> +                          "sdhci at ff160000", &dev)) {
>> 
>> I think you need to check whether probing actually succeeded, no?
>> Otherwise seq is invalid (-1).
>> 
>> So this should be if (uclass...() || (dev && dev->flags &
>> DM_FLAG_ACTIVATED))
> 
> tbh this needs to be tested by sw rewriting boot modes before this is
> called.
> 
> 
>> 
>>> +            puts("Boot from SD0 but without SD0 enabled!\n");
>>> +            return -1;
>>> +        }
>>> +        debug("mmc0 device found at %p, seq %d\n", dev, dev->seq);
>>> +
>>> +        mode = "mmc";
>>> +        bootseq = dev->seq;
>>>        env_set("modeboot", "sdboot");
>>>        break;
>>>    case SD1_LSHFT_MODE:
>>> @@ -507,12 +519,15 @@ int board_late_init(void)
>>>        /* fall through */
>>>    case SD_MODE1:
>>>        puts("SD_MODE1\n");
>>> -#if defined(CONFIG_ZYNQ_SDHCI0) && defined(CONFIG_ZYNQ_SDHCI1)
>>> -        mode = "mmc1";
>>> -        env_set("sdbootdev", "1");
>>> -#else
>>> -        mode = "mmc0";
>>> -#endif
>>> +        if (uclass_get_device_by_name(UCLASS_MMC,
>>> +                          "sdhci at ff170000", &dev)) {
>>> +            puts("Boot from SD1 but without SD1 enabled!\n");
>>> +            return -1;
>>> +        }
>>> +        debug("mmc1 device found at %p, seq %d\n", dev, dev->seq);
>>> +
>>> +        mode = "mmc";
>>> +        bootseq = dev->seq;
>>>        env_set("modeboot", "sdboot");
>>>        break;
>>>    case NAND_MODE:
>>> @@ -526,6 +541,11 @@ int board_late_init(void)
>>>        break;
>>>    }
>>> 
>>> +    if (bootseq >= 0) {
>>> +        bootseq_len = snprintf(NULL, 0, "%i", bootseq);
>> 
>> This seems like quite a heavy way to figure out the number of digits of
>> an integer ;).
> 
> Recursive function can be used too. But this is kind of nice even I
> agree that it takes more time that for example this
> 
> 
> static int intlen(u32 i)
> {
>    if (i < 10)
>        return 1;
> 
>    return 1 + intlen(i / 10);
> }
> 
> Thanks,
> Michal

[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[Michal Simek]

On 26.4.2018 09:08, Alexander Graf wrote:
> 
> 
>> Am 26.04.2018 um 08:25 schrieb Michal Simek <michal.simek@xilinx.com>:
>>
>>> On 26.4.2018 08:14, Alexander Graf wrote:
>>>
>>>
>>>> On 25.04.18 14:38, Michal Simek wrote:
>>>> Detect mmc alias at run time for setting up proper boot_targets sequence.
>>>> The first target has to correspond with boot mode.
>>>>
>>>> The purpose of this patch is to get rid of CONFIG_ZYNQ_SDHCI0/1
>>>> parameters in full U-Boot.
>>>> Unfortunately this patch can't remove it because there is missing
>>>> mmc implementation for SPL_DM_SEQ_ALIAS.
>>>>
>>>> Also xilinx_zynqmp.h only setup boot commands for mmc0 and mmc1.
>>>> It means using aliases with higher number won't work. But switching
>>>> between mmc0 and mmc1 should work properly.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> ---
>>>>
>>>> Not sure how exactly to tune BOOT_TARGET_DEVICES_MMC to have functions
>>>> for different aliases ID. I can simply setup devnum based on dev->seq
>>>> and also generate the whole bootcmd_mmc0
>>>>
>>>> bootcmd_mmc0=setenv devnum 0; run mmc_boot
>>>> bootcmd_mmc1=setenv devnum 1; run mmc_boot
>>>>
>>>> The second patch is doing that.
>>>> But still if you setup alias to higher number mmc core is not mmc dev
>>>> command is not able to work with it.
>>>
>>> I don't understand this sentence.
>>
>> Imagine case that you have mmc1000 = &sdhci0;
>> That 1000 is integer and u-boot is in hex that's why 1000 = 0x3e8.
>>
>> It means boot_targets will start with "mmc3e8 +static one"
>>
>> That's why
>> bootcmd_mmc3e8=setenv devnum 3e8; run mmc_boot
>>
>> needs to be generated. But xilinx_zynqmp.h is only setting these lines
>> for mmc0 and mmc1.
> 
> Ok, so what we really want is a distro.c file and corresponding boot_target lines that generate boot target variables for every device enumerated in the system on boot.
> 
> The reason we don‘t have that yet I guess is because most targets target one SoC/board that has a known set of devices. The more generic we make things, the more we have to enumerate at runtime.
> 

yes, and do this only in case of when variables are not restored. It
means only when default one are used.

And it is really a question if make sense to change current distro
default or just wait for EBBR.

For zynqmp device itself using mmc0/mmc1 should be enough.

Thanks,
Michal

[U-Boot] [RFC PATCH 1/2] arm64: zynqmp: Setup the first boot_target at run time

[Alexander Graf]

On 04/26/2018 10:20 AM, Michal Simek wrote:
> On 26.4.2018 09:08, Alexander Graf wrote:
>>
>>> Am 26.04.2018 um 08:25 schrieb Michal Simek <michal.simek@xilinx.com>:
>>>
>>>> On 26.4.2018 08:14, Alexander Graf wrote:
>>>>
>>>>
>>>>> On 25.04.18 14:38, Michal Simek wrote:
>>>>> Detect mmc alias at run time for setting up proper boot_targets sequence.
>>>>> The first target has to correspond with boot mode.
>>>>>
>>>>> The purpose of this patch is to get rid of CONFIG_ZYNQ_SDHCI0/1
>>>>> parameters in full U-Boot.
>>>>> Unfortunately this patch can't remove it because there is missing
>>>>> mmc implementation for SPL_DM_SEQ_ALIAS.
>>>>>
>>>>> Also xilinx_zynqmp.h only setup boot commands for mmc0 and mmc1.
>>>>> It means using aliases with higher number won't work. But switching
>>>>> between mmc0 and mmc1 should work properly.
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>> ---
>>>>>
>>>>> Not sure how exactly to tune BOOT_TARGET_DEVICES_MMC to have functions
>>>>> for different aliases ID. I can simply setup devnum based on dev->seq
>>>>> and also generate the whole bootcmd_mmc0
>>>>>
>>>>> bootcmd_mmc0=setenv devnum 0; run mmc_boot
>>>>> bootcmd_mmc1=setenv devnum 1; run mmc_boot
>>>>>
>>>>> The second patch is doing that.
>>>>> But still if you setup alias to higher number mmc core is not mmc dev
>>>>> command is not able to work with it.
>>>> I don't understand this sentence.
>>> Imagine case that you have mmc1000 = &sdhci0;
>>> That 1000 is integer and u-boot is in hex that's why 1000 = 0x3e8.
>>>
>>> It means boot_targets will start with "mmc3e8 +static one"
>>>
>>> That's why
>>> bootcmd_mmc3e8=setenv devnum 3e8; run mmc_boot
>>>
>>> needs to be generated. But xilinx_zynqmp.h is only setting these lines
>>> for mmc0 and mmc1.
>> Ok, so what we really want is a distro.c file and corresponding boot_target lines that generate boot target variables for every device enumerated in the system on boot.
>>
>> The reason we don‘t have that yet I guess is because most targets target one SoC/board that has a known set of devices. The more generic we make things, the more we have to enumerate at runtime.
>>
> yes, and do this only in case of when variables are not restored. It
> means only when default one are used.

Well, instead of the current "mmc0 mmc1" boot targets you would 
introduce an "mmc" boot target that probably needs to call C code to do 
the device walking. That way the environment would be unchanged 
regardless of the target system.

> And it is really a question if make sense to change current distro
> default or just wait for EBBR.

I'm not sure EBBR is going to help here.

> For zynqmp device itself using mmc0/mmc1 should be enough.

Yes, I think as a first approximation it's fine to have mmc0/mmc1 
entries in the static boot target list and just blank/remove the 
bootcmd_mmc{0,1} ones that don't exist on boot.


Alex