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

[Graeme Russ]

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

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

> 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

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

> 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

> 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

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

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

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

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

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

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

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

> 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

Regards,

Graeme