RFC Create generic board init and move ARM and x86 to it
Get series with b4
b4 am [email protected]Versions newest first
- v1 RFC Latest0 of 19 patches 0 repliesNew
Patches in v1 No applies recorded yet
None of the 19 patches the cover letter announces are in this archive.
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [U-Boot] [RFC PATCH 0/19] Create generic board init and move ARM and x86 to it
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...
Quoted 11 lines · Simon Glass
Hi Graeme,
Quoted 8 lines · Graeme Russ
Hi Simon,
Quoted 5 lines · Simon Glass
Hi Graeme,
Quoted 2 lines · Graeme Russ
Hi Simon,
[snip]
Quoted 9 lines
Quoted 8 lines
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
Quoted 3 lines
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.
Quoted 30 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 19 lines
Quoted 8 lines
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 functionsI 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 fileQuoted 2 lines
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
Quoted 6 lines
OK yes we could do this. But I think the end result is different.
I am keen to create a single
common/board_f.cimplementation for pre-reloc init andcommon/board_r.cfor post-reloc init. I believe where your approach ends up with with separatearch/xxx/board.cfiles 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
Quoted 5 lines
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...
Quoted 10 lines
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
Quoted 2 lines
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
Quoted 4 lines
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
Quoted 4 lines
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...
Quoted 13 lines
Anyway if we do go with what you propose I still think it is better than what we have.
Quoted 10 lines
[snip]
Quoted 7 lines
Quoted 6 lines
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()andhang()and change the function signatures of the functions that require wrappers and move some #ifdef's into more appropriate locations - One example inboard.c:
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Well it seems like a lot of work to refactor each
arch/xxx/board.cfile 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.
Quoted 7 lines
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
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
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.cto 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
Quoted 3 lines
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.
Quoted 3 lines
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
Quoted 13 lines
Quoted 13 lines
[snip]
Quoted 10 lines
Quoted 7 lines
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
Quoted 7 lines
Quoted 4 lines
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 allarch/foo/lib/board.cwill 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