[U-Boot] [PATCH v3] powerpc: Restore core of mpc8xx

[Christophe LEROY]

Hello Heiko,

Le 22/06/2017 à 15:06, Heiko Schocher a écrit :
> Hello Christophe,
> 
> Am 22.06.2017 um 13:20 schrieb Christophe LEROY:
>>
>>
>> Le 22/06/2017 à 11:59, Wolfgang Denk a écrit :
>>> Dear Christophe,
>>>
>>> In message <0784ad6e-86ab-9c1d-1b81-a5cacaf2b01f at c-s.fr> you wrote:
>>>>
>>>>> Please see my previous comments to Tom's message.  At least, please
>>>>> clean up the directory structure and get rid of unrelated (4xx) and
>>>>> untested/broken code (bedbug, probably kgdb, pcmcia, usb).
>>>>
>>>> I got 4xx unrelated stuff out in v3, as mentioned above.
>>>
>>> I still see these files in your patch:
>>>
>>>     drivers/net/4xx_enet.c
>>>     include/configs/CPCI4052.h
>>>     include/configs/MIP405.h
>>>     include/configs/PIP405.h
>>>     include/configs/PLU405.h
>>
>> Those are
>>
>> #undef    CONFIG_IDE_8xx_DIRECT
>>
>> My patch brings back CONFIG_IDE_8xx_DIRECT, so if somebody has decided 
>> that this CONFIG_ shall
>> explicitly be undefined for 4xx targets, I believe there is a reason 
>> and I cannot decide by myself
>> to remove that.
> 
> That;s exactly a part of crap ... now, as this symbol is gone, no need
> for it to undef it in some none 8xx configs ...

Well. Ok for that one, it seems no one defines it anymore. But in 
principle, better safe than sorry.

> 
>>> To me these look very much like PPC4xx related.  In addition, it
>>> makes zero sense to keep PPC405 board config files without any
>>> related board code.
>>
>> Why do you say that ? There is a lot of code in arch/powerpc/ppc4xx/ 
>> and in configs/ you find the
>> following defconfigs:
>> -rw-r--r--. 1 root root 641 Jun 22 01:34 CPCI4052_defconfig
>> -rw-r--r--. 1 root root 871 Jun 22 01:34 MIP405_defconfig
>> -rw-r--r--. 1 root root 799 Jun 22 01:34 MIP405T_defconfig
>> -rw-r--r--. 1 root root 869 Jun 22 01:34 PIP405_defconfig
>> -rw-r--r--. 1 root root 675 Jun 22 01:34 PLU405_defconfig
>> -rw-r--r--. 1 root root 588 Jun 22 01:34 PMC405DE_defconfig
>> -rw-r--r--. 1 root root 449 Jun 22 01:34 VOM405_defconfig
>> -rw-r--r--. 1 root root 747 Jun 22 01:34 xilinx-ppc405-generic_defconfig
> 
> Actually I have the 4xx removal patches on my todo list, but did not
> find time yet to submit these.  I hope I will find time for this
> next week.  As far as the MPC8xx discussion is related, please
> consider 4xx as removed, too.

Ok, noted. Please take care to not remove anything that is common with 
8xx before 8xx is back.

> 
>>> On the other hand I do not see a single board configuration for 8xx,
>>> so you cannot even compile the code for a MPC8xx board, or am I
>>> missing something?
>>
>> Yes you are right, Tom asked me to bring back only the core part for 
>> the time being, until I have
>> cleaned up the code for our two boards.
> 
> Ok... but should we not have at least one (minimal) board configuration
> so you can at least build the code and see if it compiles?  Whithout
> such test, the risk of broken, non-compiling code is huge!

Yes I realised that after Tom suggested to perform a travis-CI build, 
I'm working on it, should probably get something early next week, maybe 
tonight

> 
>>>>> Also, are you going to support the whole family, including MPC823(E)?
>>>>> If not, we should also omit stuff like the 823 video support.
>>>>
>>>> I agree, but please no big-bang now. It is already complicated enough.
>>>
>>> I try to get used to the idea of doing code level cleanup later,
>>> even though my personal feeling for re-adding known-to-be-broken
>>> code is not exactly positive.
>>>
>>>
>>> But at least at file level we should perform the needed cleanup now:
>>> - omit unrelated stuff
>>> - omit known to be unsupported/untested/broken code
>>> - move drivers to the correct places according to today's file
>>>    system hierarchy.
>>>
>>> As I explained before, I doubt that you will be able to test things
>>> like  drivers/video/mpc8xx_lcd.c  on your boards, and I strongly
>>> doubt that you are willing to invest efforts in adapting and testing
>>> things like PCMCIA, bedbug, SIL680 support, or USB on your hardware.
>>
>> Yes I agree, that's probably the first things I will remove (except 
>> USB probably).
> 
> I am here in line with Wolfgang. Why bringing in code, you do not know
> about, if you need it?
> 
> Make a list of features you need for your board and only add
> this code. Or do you not know what you need?

Of course I know what I need.

> 
>>> [In which way is the sil680 driver related to mpx8xx, btw?]

If it is not related, then it will go when I do the clean up. Lets do 
the things in order and perform atomic commits.

>>>
>>>
>>> So when we know in advance that this old, crappy code will never be
>>> tested, why should we spend effforts even in adding it?
>>
>> Just because it was there 2 weeks ago. I prefer starting from a known 
>> situation.
> 
> ???

Here I really feel like being an hostage. Just because I didn't noticed 
early enough that the 8xx code was about to go away, now I have to pay 
the ransom of doing a huge cleanup to get it back ? To be honnest I 
really find it unfair.
I have volonteered to maintain the 8xx and I plan to perform the cleanup 
that have been awaited. Why don't you welcome that and allow me to do as 
if we were 2 weeks ago and the code had not go away yet ?

> 
> The current (also known) situation is that we have no 8xx code in
> mainline, and we are free to re-add the necessary things to their
> correct locations - and omit everything where we do not know for
> sure that it will really be needed in the future.  If later we lear
> that something is still missing, this is more easy to add then,
> instead of cleaning up again and removing unneeded parts.

But doing that way we loose the history, years of good work by 
respective people. We take the risk to forget things to add back.
I prefer forgetting to remove something than forgetting to re-add 
something. Doing as you suggest we loose any opportunity to bisect when 
something that was working in previous versions is not working anymore. 
etc ...
git is all about doing atomic commits, to allow detailed explanation, to 
allow bissects, to ease reviews, etc.

You were looking for someone to take care of the 8xx, I'm here now. I 
need 8xx to be maintained for at least the next ten years, so I will 
take charge and do what needs to be done, but if it was not with a Knife 
under the throat it would be more rewarding.

> 
>>>> We have something that fits and that was unexpectedly removed entirely
>>>> last week. Lets come back first to a well known - working situation and
>>>> do the removal of unnecessary parts step by step as if we were 3 weeks
>>>> ago. The more we wait to get 8xx back in, the more difficult it will 
>>>> be.
>>>
>>> To be frank: you don't have a very extensive track record of working
>>> with the U-Boot community.  At the moment all we have is your claim
>>> that you will care about 8xx in the furture.  In the past, we have
>>> seen many precedents of "please add this code now, I know it is not
>>> really good, but I will fix it later, promised" - and then the code
>>> was in mainline, until somebody else digged into cleaning it up.
>>
>> Well, I don't know what to answer to that, I understand your 
>> reluctance but you probably also
>> understand mine, I try crawling in one direction to get my boards in 
>> while others are removing more
>> and more stuff (51xx, 82xx, 5xx ...) which makes the come back of 8xx 
>> more difficult each day.
> 
> And your solution is reverting none 8xx patches? Come on! Why is this a
> problem for bringing 8xx back?

Conflicts require manual actions, which often leads to human errors. 
That's my haunt here.

> 
> I do not understand you here ... you have the old core 8xx files, and 
> you only
> have to add them to the current base, and bring them up again to compile 
> ...
> 
> yes it is work, here and there, because mpc82xx and mpc8xx code wasn't 
> cleanly
> separated, unexplainable undef of 8xx defines in 4xx related configs ...

Yes I handled it, I reverted everything then I re-did the removal of 
82xx etc ...

> 
> Time to clean it up!

Yes I'm here for that. Start from the existing and clean step by step, 
with atomic commits so that everyone can see what has been done, how and 
why.

> 
> U-Boot code changes from day to day ... do you mean, we should stop
> adding patches until you have your patches in?

No, I mean we should get 8xx core back and allow cleaning it up in an 
ordered manner that allows us to keep track of history. Yes U-boot code 
changes from day to day, and that is exactly what I'd like to be allowed 
to do with the 8xx, clean it up day by day and follow the evolution of 
U-boot.

> 
> If you are working on this right now in a private repository branched
> from the current mainline tree, then it is really little effort to
> rebase your work tree every few hours against the then current
> mainline tree. The 8xx code is not used anywhere else, so you
> should see only a few merge conflicts in Makefiles or Kconfig files
> while you go, and all these would be trivial to fix.

rebasing every few hours ? If I could use that time to do something more 
usefull, wouldn't it be better for everyone ?

> 
> Actually cleaning up before submitting should be less work for you
> than re-adding the old cod now and cleaning it up in mainline later.
> Not to mention that you can squash unneeded intermediate results in
> your tree before the submission, so the patch series would even be
> smaller and easier to review.

Not sure it will be easier to review if a single patch encompasses 
several unrelated changes.

My main concern here is the loss of history and the hopelessness of 
doing a 'git bisect' to find out why something that was working in last 
year's release is not working anymore.
If for instance we re-introduce in a different place and with a 
different name some files you have just removed, we definitly loose the 
automatic history of those files. If we get it back at its original 
place and move it after, the history is kept, 'git blame' will show it up.

> 
>>> I've been using MPC8xx for 18 years, and I know all too well in how
>>> many places the code is in urgent need of a cleanup (heck, I'm even
>>> responsible for major parts of that mess - but those were the times
>>> way back then).  I'm not sure if you already anticipate the size of
>>> job you have taken.  Just looking at the POST code - are you aware
>>> how much effort it will be to adapt it to your board and to test it?
>>> And this is mandatory requirement if you want to have it added. It
>>> must be maintained, i. e. tested.
>>
>> We have it running on our boards and it works, it is just not in a 
>> clean submittable (doesn't
>> respect the Codying Style, has several hard coded stuff in core parts, 
>> etc ...), so I cannot submit
> 
> Ah! Now we came closer to the root of your problem!
> 
> I exactly feared such a statement as "several hard coded stuff in core 
> parts" !!
> 
> Which changes in core parts?
> Why do you need them?

We most likely don't need them, I have to (re)move them, however it will 
require some time.

> 
> And you do not want to adapt them at the moment, correct?

No, that's not correct. I want to adapt it, but again, I don't think it 
is a good idea to do it in a few hours, in a hurry.

> 
>> it just now, and I fear that by end august/early septembre, when I 
>> come with my cleaned board code,
>> so much modification have been done to uboot core that 8xx cannot be 
>> back without tones of reverts.
> 
> No, if you bring in with your patchset also the core 8xx support, you have
> a clean base for your patchset! And as I mentioned, yes code is in
> flow, but you can use git for rebasing with current mainline and
> fix merge problems ...
> 
>>> It would be very sad if we add this code now, and in a few
>>> weeks/months you say: oh, we don't need this, and I don't find time
>>> to care about it, let's remove it again.  And this is what's bound
>>> to happen - I bet a beer or two that you have never used most of
>>> this code before, nor had a look at the code.
>>>
>>>
>>> I think it is much, much better to perform at least the file level
>>> cleanup in a sngle step right at the beginning, starting with the
>>> minimum set of needed files for your system.  Adding needed things
>>> later is a much better guaranteee for good code than removing
>>> unmaintained crap when it starts hurting.
>>>
>>
>> Ok but please stop removing or deapply modifying additional stuff that 
>> impacts the 8xx.
> 
> Heh ... 8xx is removed. What should impact now 8xx support?

When you removed 8xx, you just removed #ifdefs.
When you removed 5xx, as it was the last #ifdef, you remove the content 
of the #ifdef, hence removed the code that I have now to bring back.

That's what I would like to avoid, as it will inevitably lead to errors.

> 
> I cannot follow here your argumentation!
> 
>>> So please check your own board configuration and come up with a list
>>> of needed features, and re-add only those, cleaning up file
>>> hierarchy while you go.  See .sig below (non-random today).
>>
>> Yes but that's not something that is doable in a few hours. So be 
>> compassionate and don't jeopardise
>> it.
> 
> Yes, exactly, not work for a few hours ... that has nothing to do
> with being compassionate or jeopardise! Please do the necessary
> changes, send a cleaner patchset and we are happy about having
> mpc8xx back in a maintained state!

Yes but with a completely lost history. That would really be a pitty.

Christophe

> 
> bye,
> Heiko
>>
>> Thanks
>> Christophe
>>
>>>
>>> Thanks.
>>>
>>> Best regards,
>>>
>>> Wolfgang Denk
>>>
>>
>