[U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform

[Dr. Philipp Tomsich]

Kever,

> On 13 Apr 2018, at 09:51, Kever Yang <kever.yang at rock-chips.com> wrote:
> On 04/08/2018 09:45 AM, Kever Yang wrote:
> 
>>>> +__weak int arch_cpu_init(void)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +__weak int rk_board_init_f(void)
>>>> +{
>>>> +    return 0;
>>>> +}
>>> This doesn't really help in modularising our board-support and I am
>>> not a fan of adding something like 'rk_board_init_f' in the first place.
>>> 
>>> Instead this should be implemented in a way that actually makes the
>>> code structure easier and more resilient for the future (having __weak
>>> functions at the architecture-level doesn't really help)... in fact
>>> the only other uses of __weak in the U-Boot source-base are within
>>> SPL, as there's no other way to provide hooks there.
>> I know your proposal is to use DM for board init, then could you make it
>> more
>> clear about how to handle this in your solution?
>> We need to do:
>> - same board init flow for all rockchip platform;
>> - something different but common in soc level;
>> - something different in board level;
> 
> I didn't see your response for this, could you send out your patches?

This isn’t at the stage of a patch-set yet… I had asked for comments to
this, so we could design this in a way that benefits all platforms.

> I admit that I'm not very clear about the limitation of '__weak' function,
> but I do see there are many '__weak' function in common/board_f/r.c,
> and my common board file is connect to the board_r.c.

I like __weak as a way to provide a hook for something that is part of the
common API (so it’s ok, if spl.c uses this).  However, I don’t want individual
platforms to suddenly expose new extension points.

And with the two examples above (arch_cpu_init and rk_board_init_f), you
basically highlight what’s wrong about using __weak at this level:
1	arch_cpu_init is an extension point to do low-level initialisation for a
	CPU (not a board).  Implementation for it usually live below arch/arm/cpu
	and takes care of things like MMU maintenance.  Now we suddenly provide
	this below arch/arm/mach-rockchip … and using a __weak function.
	This goes against everything that users will expect.  So just move it
	to arch/arm/cpu (you’ll probably need to have 2 separate ones for
	armv7 and armv8) and nothing unexpected will ever happen.
2	If we rk_board_init_f here, we are again changing the extension API
	of U-Boot: board_init_f belongs to each board (i.e. any board can expect
	to override it w/o ill effect), but now you’d suddenly create a link error.
	Instead users need to override rk_board_init_f.  This is a documentation
	nightmare (and the current solution would be to provide a common
	function that all board_init_f implementations could call as their head
	or tail…).

My question—as to whether the DM could/should be extended to CPUs,
SoCs, architectures and boards—was meant to discuss exactly these
observed issues in how boards and SoCs today interact.

I usually don’t mind to touch APIs and extend them (or the driver model),
but a solution for how to handle board/SoC/CPU init sequences is nothing
I want to start before getting an actual design discussion going and reaching
something resembling a consensus of how this aspect of U-Boot should be
structured in a year’s (or two years’) time.

> 
> @Simon, @Tom,
>     Could you kindly give some comment here?
> 
> Thanks,
> - Kever