[U-Boot] [RFC PATCH 08/11] sunxi: SPL: add FIT config selector for Pine64 boards

[André Przywara]

On 23/01/17 17:29, Maxime Ripard wrote:
> On Sat, Jan 21, 2017 at 03:15:27PM +0000, André Przywara wrote:
>>>>> On Fri, Jan 20, 2017 at 01:53:28AM +0000, Andre Przywara wrote:  
>>>>>> For a board or platform to support FIT loading in the SPL, it has to
>>>>>> provide a board_fit_config_name_match() routine, which helps to select
>>>>>> one of possibly multiple DTBs contained in a FIT image.
>>>>>> Provide a simple function to cover the two different Pine64 models,
>>>>>> which can be easily told apart by looking at the amount of installed
>>>>>> RAM.
>>>>>>
>>>>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>>>>>> ---
>>>>>>  board/sunxi/board.c | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>>>>> index 5365638..bbbb826 100644
>>>>>> --- a/board/sunxi/board.c
>>>>>> +++ b/board/sunxi/board.c
>>>>>> @@ -726,3 +726,16 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>>>>  #endif
>>>>>>  	return 0;
>>>>>>  }
>>>>>> +
>>>>>> +#ifdef CONFIG_SPL_LOAD_FIT
>>>>>> +int board_fit_config_name_match(const char *name)
>>>>>> +{
>>>>>> +#ifdef CONFIG_MACH_SUN50I
>>>>>> +	if ((gd->ram_size > 512 * 1024 * 1024))
>>>>>> +		return !strcmp(name, "sun50i-a64-pine64-plus");
>>>>>> +	else
>>>>>> +		return !strcmp(name, "sun50i-a64-pine64");
>>>>>> +#endif
>>>>>> +	return -1;
>>>>>> +}  
>>>>>
>>>>> That looks fishy. It means that any A64 board with CONFIG_SPL_LOAD_FIT
>>>>> enabled will be considered a pine64 board?  
>>>>
>>>> Yes, at least for now, because that's the only A64 board we officially
>>>> support so far.
>>>> And yes again, it is fishy. It is more a demo or a placeholder for now,
>>>> because you _need_ an implementation of board_fit_config_name_match()
>>>> once you enable CONFIG_SPL_LOAD_FIT.
>>>>
>>>> One solution would be to have only one DTB supported per build, so
>>>> board_fit_config_name_match() always returns 0.
>>>>
>>>> Another (better) solution would be to store the board name in the SPL
>>>> header, which could be done later when flashing the image. Then this
>>>> function would just return strcmp(name, spl_header_name) or 0 if no name
>>>> is found for whatever reason.
>>>
>>> Yes, this is a good solution in the case if we have some non-removable
>>> storage on the board (SPI NOR flash, NAND, EEPROM or something else).
>>> We can discuss it separately.
>>
>> I totally agree. I rebased your patch to latest mainline already and
>> will send out something later.
>>
>>> But the current generation of Pine64 boards don't have any
>>> non-removable storage yet. My understanding is that you tried to
>>> provide the DTB selection, which is based on circumstantial evidences,
>>> such as the RAM size. IMHO this is also a valid selection method,
>>> because it can reduce the number of required OS image variants.
>>
>> Yes, the point is that for U-Boot's purposes the only difference between
>> the Pine64 and Pine64+ (apart from DRAM size, which is autodetected) is
>> using MII vs. RGMII for the Ethernet PHY. The sun8i_emac driver gets
>> this information from the DT only, so there is no point at all for
>> different defconfigs. And the DRAM size is a safe indicator for that
>> difference, at least if we confine our view to Pine64 boards.
>>
>> Now how other boards fit in here is a separate discussion IMO, so let's
>> solve one problem after the other.
>> I just thought that we could use:
>>
>> #ifdef CONFIG_SPL_LOAD_FIT
>> int board_fit_config_name_match(const char *name)
>> {
>> 	return strcmp(name, CONFIG_DEFAULT_DEVICE_TREE);
>> }
>> #endif
> 
> I guess an easy way around this would be to add a Kconfig option for
> the pine64, like I tried to do for the CHIP (but never ended up
> merging). That way, at least we won't impact the other board, and we
> can have that default.

Yes, that sounds indeed like a possibility.

I came up with this snippet yesterday (rough copy):

/* Check if there is a DT name stored in the SPL header and use that. */
        if (spl->offset_default_dt_name) {
                cmp_str = SPL_ADDR + spl->offset_default_dt_name;
        } else {
#ifdef CONFIG_DEFAULT_DEVICE_TREE
                cmp_str = CONFIG_DEFAULT_DEVICE_TREE;
#else
                return 0;
#endif
        };

        /* Differentiate the two Pine64 board DTs by their DRAM size. */
        if (strstr(name, "-pine64") && strstr(cmp_str, "-pine64")) {
                if ((gd->ram_size > 512 * 1024 * 1024))
                        return !strstr(name, "plus");
                else
                        return !!strstr(name, "plus");
        } else {
                return strcmp(name, cmp_str);
        }

That admittedly doesn't look very pretty, but should cover all the
cases: DT name from SPL header, default compile time DT, Pine64 check.
We could also guard the last part with a CONFIG_SUNXI_PINE64_DETECT
symbol to save some space in case this is not needed.

Does that make sense?

Cheers,
Andre.