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

[Simon Glass]

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]
>
>>> However, I think this approach is not the right one - and I think the CFI
>>> driver backs me up. Your plan is to create generic code which you want ALL
>>> arches to cross over to, but you only look to migrate two initially and
>>> migrate the rest 'later'. This is similar to what happened with the CFI
>>> driver, and there are still boards with custom flash.c files which are
>>> completely redundant.
>>>
>>> But, creating a single patch-set to migrate everyone in one go is going to
>>> be too massive a job to do in one go, and too prone to introducing breakage.
>>
>> Yes to some extent. However, my patch basically splits the two
>> board_init_x() functions into parts, and puts them in a function
>> table.
>
> 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.

>
>> 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
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.

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.

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
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.

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 :-)

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
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 :-)

>
>> 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.

>
>> 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
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?

>
> [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.

>
>> doing one architecture at a time is best - with the proviso that we
>> need to pick archs that have the most features (so ARM and PowerPC I
>> suppose) to make sure we are not deluding ourselves as to the
>> simplicity of the task.
>
> ARM + PPC + x86 = 95% of the complexity. If we pull them off, the others
> are trivial cases.

OK good.

>
>> So perhaps a generic board init that is the default can be switched
>> off on board-by-board basic would be the right approach. Then we can
>> flick the switch while providing for those affected to still get work
>> done until bugs are reported and found?
>>
>>>
>>>> Generic relocation is used (see previous series) but now rather than
>>>> calling relocate_code() to do everything, we call the individual
>>>> relocation steps one by one. Again this makes it easier to leave things
>>>> out, particularly for SPL.
>>>
>>> Note that not all arches need and/or use ELF relocation - Attacking this
>>> first does not move towards unity of board.c
>>
>> It is a feature used by about half, and it does include the complexity
>> of jumping from pre-reloc to post-reloc init. I think it was a
>> reasonable first target.
>
> It is an excellent first target after making the init sequence generic -
> skipping relocation is then trivial (you just don't include it in the init
> sequence)

Yes

>
> [snip]
>
>>> The big issue I have is that we now have two RFC's which have major impacts
>>> on x86 and one of us is going to have to give way to the other :)
>>
>> Well I posted my x86 patches so they are not just sitting in the long
>> grass on my machine. They are not complete, and I would likely need
>> your input on these anyway.
>
> Most of your patches are a subset of my complete init re-write (and credit
> where it is due, you initial relocation RFC was the catalyst - I wanted to
> make your life a little easier by simplifying things in x86-land)

Yes, they are a subset - but hopefully generic enough to work for any arch.

>
>>> My patch set is mostly re-factoring and has no impact on anybody else and
>>> is practically complete - I'll do one last RFC, but I think it's already
>>> close to 'PATCH' status
>>
>> That's a separate series from my point of view. I can't comment on the
>> wierd x96-isms in the series, but I did read through each patch and it
>> all seems sensible to me.
>
> Thanks, that is enough for me for now. I'll request a proper review when I
> submit the [PATCH] series

OK

>
> [snip]
>
>>> There are several x86 patches which simply do not belong in this series
>>> (and some of which already have non-RFC patches submitted)
>>
>> OK, these are the warnings I think. Probably I should build without
>> -Werrors and try to ignore them!
>
> Propagate them to the top of your list to get them out of the way. I need
> to add them to my series (with original attribution of course) or commit
> them as is (some need tweaking)

OK

>
>>> I honestly think we should get the x86 init sequence patches finalised
>>> first for several reasons:
>>>
>>>  - Because x86 is so small, it provides a good test-bed - ELF relocation
>>>   was first finalised on x86 (it came and went with varying levels of
>>>   success previously)
>>>  - They bring x86 in line with other arches re: global data
>>>  - They are now fully run-tested
>>
>> No disagreement there, and anyway as you can see the relocation stuff
>> mostly came from your x86 code. It will provide a good template for
>> getting x86 working properly with the generic board init.
>>
>> What do you need from me to get that through?
>
> Patience :) - I'll try to get a final series up this week
>
>> Per Wolfgang's request to go with PPC as an early-adopter, this is
>> somewhat in conflict, since as you say, x86 is less feature-full than
>> ARM and much less than PowerPC.
>
> We need to get the PPC (plus ARM and x86) init sequence distilled to pure
> init loops before moving forward.

Well for now I'm going to continue looking at PPC init and seeing if I
can roll into into the series. Whichever way we go it is a useful
resource.

>
>> Can anyone recommend a PowerPC board with a quick U-Boot program-run
>> cycle that I can easily get that will let me try out things there?
>
> If all we are doing is moving and wrapping code, the review and test should
> be fairly straighfoward

Hmmm that's mostly what I am doing too, with the addition that I am
trying to reason about the ordering of the code.

Regards,
Simon

>
> Regards,
>
> Graeme