[U-Boot] [PATCH 00/16] RFC: Board init using driver model

[Simon Glass]

Hi Tom,

On 22 March 2017 at 09:13, Tom Rini <trini at konsulko.com> wrote:
> On Wed, Mar 22, 2017 at 08:43:54AM -0600, Simon Glass wrote:
>> Hi Tom,
>>
>> On 22 March 2017 at 08:37, Tom Rini <trini at konsulko.com> wrote:
>> > On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
>> >> Hi Tom,
>> >>
>> >> On 19 March 2017 at 18:47, Tom Rini <trini at konsulko.com> wrote:
>> >> > On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
>> >> >> At present we have a lot of ad-hoc init functions related to boards, for
>> >> >> example board_early_init_f(), board_misc_init_f() and dram_init().
>> >> >>
>> >> >> There are used in different ways by different boards as useful hooks to
>> >> >> do the required init and sequence it correctly. Some functions are always
>> >> >> enabled but have a __weak default. Some are controlled by the existence
>> >> >> of a CONFIG.
>> >> >>
>> >> >> There are two main init sequences: board_init_f() (f for running from
>> >> >> read-only flash) which runs before relocation and board_init_r() (r for
>> >> >> relocated) which runs afterwards.
>> >> >>
>> >> >> One problem with the current sequence is that it has a lot of
>> >> >> arch-specific #ifdefs around various functions. There are also #ifdefs
>> >> >> for various features. There has been quite a bit of discussion about how
>> >> >> to tidy this up and at least one RFC series[1].
>> >> >>
>> >> >> Now that we have driver model we can use this to deal with the init
>> >> >> sequences. This approach has several advantages:
>> >> >>
>> >> >> - We have a path to remove the #ifdefs
>> >> >> - It is easy for multiple parts of the code to implement the same hook
>> >> >> - We can track what is called and what is not
>> >> >> - We don't need weak functions
>> >> >> - We can eventually adjust the sequence to improve naming or to add new
>> >> >> init phases
>> >> >> - It provides a model for how we might deal with ft_board_setup() and
>> >> >> friends
>> >> >>
>> >> >> This series starts the process of replacing the pre-relocation init
>> >> >> sequence with a driver-model solution. It defines a uclass, adds tests
>> >> >> and converts sandbox and a few x86 boards over to use this new setup.
>> >> >>
>> >> >> This series is not ready for use yet as the rest of the init sequence
>> >> >> hooks need to be converted. But there is enough here to show the idea.
>> >> >>
>> >> >> Comments welcome.
>> >> >>
>> >> >> [1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
>> >> >
>> >> > How does this look, size wise?  With all of these conversions and
>> >> > clean-ups, we really need to be size concious as well as it all keeps
>> >> > adding up.  Thanks!
>> >>
>> >> It include size a bit - e.g. x86 808 bytes of text, although that does
>> >> include a few extra features.
>> >
>> > How about if we don't include some of the extra debug/demo type features
>> > (which are useful at times, certainly) ?  We keep adding things that add
>> > a few bytes here and a few bytes there and it all adds up.
>>
>> Yes it's very important that the base version doesn't increase size,
>> or at least only minimally. I should have examined that more closely
>> in the RFC - my intent was really to get comments on the approach,
>>
>> >
>> > [snip]
>> >> I think I can use a linker-list approach to reduce the overhead. But I
>> >> still think the driver has value as it provides a means of adding
>> >> hooks to do board-specific things from drivers, something that we keep
>> >> running into. Also the idea of a board 'driver' seems conceptually
>> >> useful.
>> >>
>> >> So one approach would be to have:
>> >>
>> >> 1. A linker-list-based board hook setup, where you can declare, for example:
>> >>
>> >> static int ivybridge_dram_init(void)
>> >> {
>> >>  ...
>> >> }
>> >> U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
>> >>
>> >> This should be pretty cheap, perhaps <200 bytes with some care.
>> >>
>> >>
>> >> 2. An optional BOARD uclass which can be used for more involved
>> >> situations, but with a higher code size penalty.
>> >
>> > OK.  But I also recall that we had talked about trying to condense and
>> > re-factor things.  My worry about an approach like this is it allows for
>> > us to more easily get back into the bad habits of having each
>> > architecture solve similar problems with different solutions.
>>
>> Yes that's true and I've been pushing back on this for a while. For
>> example there is quite a bit of pressure to add board-specific init
>> code to drivers with driver model. So far I think we have been able to
>> avoid this using device tree and other drivers. For example if MMC
>> needs a clock we can find the required clock by phandle and call the
>> clock driver.
>>
>> So are you thinking we should limit this to just a simple hook
>> approach for now, and then consider the board uclass down the track?
>
> Looking over init_sequence_[rf], I can certainly see the case for "ug,
> this is ugly and we need to make it better" (and I now wonder if we
> don't have a lot more places where we need INIT_FUNC_WATCHDOG_RESET,
> anyhow...).  But for the moment we seem to be able to resist adding more
> calls here.  And I would like to see if we can rework / reduce the
> current table before we try and simplify and clean-up the mechanism that
> we use to handle them.

I agree, and I have some concern that making it easier to extend the
init sequence might end up with less consistency between archs as to
the sequence we go through during init.

For now I've done two series to tidy up board_f. There is more to do
though. We can park this series until we get a bit closer (it might be
quite a while).

Regards,
Simon