[U-Boot-Users] How to patch u-boot?

[Haavard Skinnemoen]

On Tue, 22 Jan 2008 13:30:07 +0100
Harald Welte <laforge at openmoko.org> wrote:

> On Tue, Jan 22, 2008 at 10:32:53AM +0100, Haavard Skinnemoen wrote:

> > Hey, is that the thanks I get for actually commenting at your mmc
> > driver? I don't recall rejecting anything, nor do I have the power to
> > do so.
> 
> well, sorry.  But since there were no other significant comments, it was
> sort of the most important/influential one.

No problem. I just thought it was a bit funny that you first complained
about lack of comments, and then went on to complain that the comments
you _did_ get were too thorough ;-)

> > I did suggest that you move the MMC protocol definitions to a common
> > header file. That's not a huge task, but you could have said "no, I
> > don't want to do that right now. Maybe later." and it would have been
> > perfectly fine. Instead, you said "will do so in the next version of
> > the patch." But no next version of the patch ever showed up, despite
> > quite a few comments from others as well.
> 
> well, obiously I try to implement the suggestions of the people on this
> list, even if it means significant additional delays.  There is no point
> in submitting the same stuff without implementing the comments from the
> review.

Yes, I agree in general. But I also think it should be perfectly ok to
say "no, sorry, that's too much work at this point", although it may
start a discussion about whether it _really_ is too much work. You
should never silently ignore any comments, of course.

Review comments are really just suggestions about how to improve your
work further, but you shouldn't ignore them unless you have a good
reason to do so, and let the reviewer and other know.

> > Btw, I find the "lack of hardware" argument a bit disturbing. Doing
> > some cleanups really shouldn't require anyone to have access to all the
> > affected hardware 
> 
> Well, the big issue is that most of those cleanups are deep down in the
> low-level head.S/start.S initialization, affecting the ordering of how
> PLL's are initialized, the resume-from-ram code paths, etc.

There's mmc code down at that level...?

> Thos can be very tricky, and especially with the level of documentation
> that Samsung provides about chipset bugs (close to zero), it can be very
> easy to break something on one chip without noticing.  They have done
> things along the lines of re-defining the meaning of the same bits in
> the same registers from 'pull-up' to 'pull-down' :)

If you're talking about cleanups in general, then yeah, those low-level
bits can be quite fragile. I think the best you can do is to test it at
your own hardware and clearly state that your patch touches some tricky
low-level bits of code and needs testing.

If nobody steps up to test the code, I guess we have a problem. IMHO,
I'd say screw them and commit the patch anyway, after a reasonable
grace period. If it turns out to break something, you'll at least find
out which boards are properly maintained and which boards aren't.

> > -- IMO much of the responsibility for testing that nothing breaks on
> > real hardware lies on the custodians and other users that actually
> > have access to the affected boards. 
> 
> > But a few cross toolchains are definitely required to verify that it
> > actually compiles on a handful of configurations before submitting.
> 
> This would basically mean I had to know which toolchains other users of
> boards using the existing 24xx support are using.

U-boot is supposed to compile with standard toolchains isn't it?

Sometimes it doesn't, but that may be an interesting piece of
information on its own, which you should report to the mailing list.

Yeah, I know, avr32 currently requires a vendor-supplied toolchain, but
we are working on getting it upstream (honest!) and because of this, we
really don't expect people to do compile-testing with the avr32
toolchain.

> Is there somewhere a list of the toolchains that the code has to compile
> against, including pointers on where to obtain them?

I have a standard ARM toolchain and a standard ppc toolchain, both
compiled from the Ubuntu gcc-4.2 sources. Those two architectures cover
a lot of configurations, so I'd expect them to uncover the vast
majority of problems with a patch.

ppc is currently broken though, since nobody seems to be interested in
merging my one-line fix...

Haavard