[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 at 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 at 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