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

[Maxime Ripard]

On Mon, Jan 23, 2017 at 10:55:55PM +0000, André Przywara wrote:
> 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?

Yep, that works for me (with the config option)

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170126/493de45e/attachment.sig>