[U-Boot] [RFC PATCH 0/19] Create generic board init and move ARM and x86 to it

[Graeme Russ]

Hi Simon,

In case you haven't noticed (I did not Cc you or Wolfgang, sorry 'bout
that) I've posted the cleaned up version of my x86 init refactoring...

On 02/01/12 10:48, Simon Glass wrote:
> Hi Graeme,
> 
> On Sat, Dec 31, 2011 at 3:52 AM, Graeme Russ <graeme.russ at gmail.com> wrote:
>> Hi Simon,
>>
>> On 31/12/11 13:02, Simon Glass wrote:
>>> Hi Graeme,
>>>
>>> On Fri, Dec 30, 2011 at 7:49 AM, Graeme Russ <graeme.russ at gmail.com> wrote:
>>>> Hi Simon,
>>>>

[snip]

>>
>> Ah yes, looking a bit closer I see this now in patches 5 & 6. However, I
>> think if you look at my patches 9, 10, 12 and 13 I'm doing essentially the
>> same thing for x86. The difference is, I'm moving live code, rather than
>> creating dead code then switching over. I personally do not think your
>> approach is very safe - If there is a breakage, it is spread over multiple
>> patches - With the 'move code around' approach, all breakages are confined
>> to a single patch
> 
> See my other email on this topic though - I feel that one test/fix
> cycle is better than testing every board for every little patch over
> an extended period.

The problem is not one of how sparsely the test/fix cycles are spread over
time, it is one of spreading the breakage over multiple patches - If you
are replacing functionality then add the new functionality, add the hooks
to use it and delete the old in a single patch. That way, if you change
breaks something, the revert is trivial. If you multi-patch approach breaks
something, the revert becomes more difficult.

>>> I don't think it is a huge job to do this for PowerPC also, and that
>>> seems to be the most feature-full architecture.
>>
>> I agree - The init architecture is the same, but the sequence is bigger
>> (and not in the same order)
> 
> Yes that's what I'm finding.
> 
>>
>>> Also it does depend on expectations. I would hope that moving an
>>> architecture over would be a fairly small task:
>>>
>>> - getting generic relocation working
>>> - adding functions for anything that is missing from board init code
>>> - removing things which don't work on the architecture?
>>> - worrying about differences in ordering between functions
>>
>> I see it as:
>>  1) Rework the arch init sequence to be a pure list of 'int foo(void)'
>>    functions, adding helpers and wrappers where necessary
>>  2) Switch over to use the generic init processing loop
>>  3) Factor out common helpers and wrappers across all arches
>>  4) Factor out common functionality (e.g. relocation)
>>  5) After all arches are switched, remove wrappers by changing the
>>    function signature of the wrapped function
>>  6) Move helpers into relevant driver/common source file
>>
>> I think 1 & 2 need to be done for at least ARM, x86 and PPC before starting
>> 3 & 4 and 1-4 needs to be done for all arches before doing 5 & 6
> 
> OK yes we could do this. But I think the end result is different.
> 
> I am keen to create a single common/board_f.c implementation for
> pre-reloc init and common/board_r.c for post-reloc init. I believe
> where your approach ends up with with separate arch/xxx/board.c files
> still, with their own initcall list, and a number of private

Correct, but as a first step, the processing loop code is made common,
leaving only the init arrays per-arch

> functions. Now this is better than where we are now, but my original
> complaint was that I want to be able to add features to a single place
> and have them work across all architectures. With your end-result I
> still need to figure out where to put the new feature in each initcall
> list and create 10 patches, one for each arch, to plumb it in.

Agree, and my ultimate end goal (as per my previous initcall patch series)
is to remove the per-arch init list as well...

> IMO the ordering differences between architectures are mostly
> unnecessary - an artifact of forking the code - and we should aim to
> minimise these differences.
> 
> Putting everything in one file helps to clarify exactly what the
> differences are between archs, and discussions can take place as to
> why and whether this differences can be removed. If we accept that PPC
> is the one true way (in terms of U-Boot code base), then the job is to
> make other arch's code more like PPC. This is best done in one place I
> think.

I see two issues with your approach:
  1) You want to take a 'big bang' approach and change every arch over in
     one set of patches. I admire your vision, but question you realism ;)
     I took one look across all the init sequences and fled in horror. Such
     a big-bang approach is sure to cause some _major_ board breakage
  2) As Wolfgang has eluded to, a unified set of init arrays will be more
     #ifdef than code

> After a bit of thought, I worry also that your approach will create an
> extended period of time where U-Boot is unstable, as patch are patch

How so - Take a look at my x86 patch series. It only impacts x86 and it can
be easily tested. Other arches are going to be even easier as the most
delicate part of my series was getting global data to behave properly. For
the other arches, it will be a simple matter of moving code around. What I
did for x86 was fold a single init function which was outside the init
array into the array (and creating a helper or wrapper as needed) at a time
in a series of small patches. I then squashed them all together

> is added to refactor the code. Granted, many of these patches are
> small and the risk is small. But why bother? At the end we still have
> 10 different board files, lots of differences and no one willing to
> change anything because it requires an entire test cycle again.

Because what we end up with is 10 sets of init_f, init_f_r and init_r
arrays - All the processing loop and entry points are common and all the
helpers and wrappers have been factored out as far as possible.

So we have 10 sets of very clean code (statically initialised arrays) which
we can look at side-by-side and decide what to do. This may be either:
 - Decide it's all too hard and keep them arch specific (but at least
   the processing code is now common)
 - Create unified arrays using #ifdefs
 - Distil them into a set of 'common for most' which we use as the
   default and have separate ones for the corner cases
 - Change the way we do things entirely

Your approach immediately goes for option #2 or #3 on the assumption that
it is possible to do so - My approach may end up in exactly the same place,
but if not, it's not a wasted effort

> Maybe I am being too ambitious here, but since everything started with
> PPC's board.c, I don't see why we shouldn't head back in that
> direction and just create a single file again. It does create a big
> test problem, but only once :-)

No, it creates a _HUGE_ test problem - And what do we do with the corner
cases that fail in weird and wonderful ways...

> Anyway if we do go with what you propose I still think it is better
> than what we have.
> 
>>
>> [snip]
>>
>>>> If we work each individual arch to use a generic init sequence (like the
>>>> proposed x86 code) then the init processing naturally falls out as common
>>>> code and the patch to realise this is trivial. From there, we can start to
>>>> pull out common init code like init_baudrate() and hang() and change the
>>>> function signatures of the functions that require wrappers and move some
>>>> #ifdef's into more appropriate locations - One example in board.c:
>>>
>>> Well it seems like a lot of work to refactor each arch/xxx/board.c
>>> file into functions with a function pointer list, then later remove
>>> this code function by function.
>>
>> Yes, it is more work, but it is intrinsically safer (and git bisect-able)
> 
> Well I don't propose to create things which are not bisect-able. I

But you have - You create new functionality in one patch, add a number of
patches, then finally use that new functionality in a later patch.

> think what you mean is that there will be a commit which switches over
> to generic board for an architecture, and board init before that patch
> is quite different from board init afterwards. Yes that's right. Once
> the patch is reviewed, testing and committed, presumably most boards
> will work fine. The exception will be the boards which have unique
> features and no one had time to check them. That problem is not unique
> to either approach. Future patches welcome :-)

With my latest series, each patch made a functional change that was
manifest _in that patch_ so I was able to step through each and every
patch, building and running each one as I went (and just for the record,
the first run through was a dead-set shocker - the gd patches had a fair
bit more work to do)

You series does not do this - You can apply one patch, build, load and run
and say 'yep, it still runs' but when you apply the next patch (or the
next, or the next etc) and something breaks, it might not have been the
last patch you applied that broke it. Specifically, patches 5 & 6 and then
leap to patch 18 before we use that code in x86

>>> My feeling is that with a generic board, it should hopefully be a
>>> fairly small amount of work for someone familiar with an architecture
>>> to find the bugs and patch the generic code to suit their
>>
>> You seem to admit that your approach can introduce bugs
> 
> Yes I do. Both approaches introduce bugs. I think that is unavoidable
> so we should try to do this once and get the best result out of it
> that we can.

I comes down to identifying the exact patch that introduced the bug

>>> architecture. It is something that needs to be done once, not every
>>> time there is a new patch removing (almost) common code.
>>
>> The safer approach is to tackle each arch independently - Convert each
>> board.c to something similar to what I have shown with x86 and test those
>> arch-specific changes to make sure nothing has been broken. Then it is a
>> trivial matter of checking that there are no incompatibilities between the
>> arch and the common case (and adjusting as required) and switching over to
>> the common case
> 
> This switch-over is really no different from what I propose (except
> that I want to go further and have a single board file). It introduces
> the same bisect issue you mentioned, and I feel it doesn't really

No it does not - each arch is migrated independently towards a common
implementation (but that implementation is replicated in each arch) - So
for each arch, up until the point were we say 'they are now the same' we
can bisect a bug to a single patch in that arches migration path. Then we
look at the code and say 'yep, they are all the same' and create the common
code and switch them _all_ over in a single patch. This is bisectable at
every step.

> improve the situation. We have instability with each patch as we
> refactor the board init code on each arch, then instability when we
> switch over. Why not just have this pain once?

Because as soon as you break bisectability (is that even a word) your
'pain' is several orders of magnitude worse. A bisectable bug can be a
simple revert. A non-bisectable bug requires a new patch which may
introduce even more bugs

>> [snip]
>>
>>>> The other big benefit is that you only touch one architecture at a time up
>>>> until you 'pull the switch'. And when you do pull the switch, you should be
>>>> factoring out identical code so the chances of breaking something should be
>>>> vastly reduced. Take a look at the history of ARM relocation for example -
>>>> that was constrained to one arch but still the amount of breakage was massive.
>>>
>>> >From previous discussions, if something is optional then the switch
>>> will never happen. The code you are talking about is sometimes
>>> identical, sometimes slightly different. In some cases the order is
>>> different. I see many ways of introducing breakages. I do agree that
>>
>> The init sequence order is dictated arch-specifically through the init
>> arrays which will (for the time being) remain in the arch specific board.c.
>> But that is all arch/foo/lib/board.c will contain - init sequence arrays,
>> no 'code'
> 
> OK, see above. Better than what we have, for sure. I am hoping we can
> go further.

And so am I - I have not given up on initcall yet ;) (actually, I have an
idea but I'll start a new thread to discuss)

[snip]

Regards,

Graeme