[U-Boot] [PATCH 5/6] cmd_nvedit.c: allow board-specific code before/after saving the environment

[Wolfgang Denk]

Dear Timur,

In message <4FB57D2D.3090902 at freescale.com> you wrote:
> 
> > I think whoever told you this was right.  Let it break.
> 
> Come on, Wolfgang.  That's not acceptable.

Why not?  But adding arbitrary complexity and ugliness into U-Boot
mainline is acceptable?  Why?


> > We cannot add pre- and post-hooks all ever the place for brain-dead
> > designs that need to do this and that before and after doing perfectly
> > things.
> 
> Well, I already have code in U-boot that does this.  If you look at
> board/freescale/p1022ds/diu.c, you'll see that I override each of the NOR
> flash accessors.  This is horribly inefficient, but it works.

This is already streching the code to the limits, and the only reason
you ever got this thrugh is that it's FSL specific code, you you have
to deal yourself with this ugliness.  I don;t think you will find good
arguments to convince me adding such stuff into common code, though.

> Unfortunately, it only covers NOR flash.  The new design covers NOR and NAND.

Well, your code does not really cover NOR flash.  I think it covers
only a small subset of the use cases, but horribly fails else.

For example, what happens when I just use "md" or "itest *addr" or
similar trying to read NOR flash while the display is on?

You _do_ have a broken hardware design, and if you cannot access NOR
flash with display running, and vice versa, than just accept this
fact.

Feel free to provide commands to switch mode, but don't lard the code
with hooks here and there trying to fix what cannot be fixed.


> The last two patches of this patchset are a vast improvement, but they
> require a board hook (and using Mike's idea, only one hook is necessary,
> not two).
> 
> As for 'all over the place", I think it's unfair to say my one board hook
> function is going to result in "all over the place" hacks.

Well, you try again to fix just a single use-case, leaving all the
others unsolved.  It makes zero sense to fix "saveenv" while not
fixing all other access modes to the same storage device.

Yes, it is a pain if you have to run something like

	=> busmux flash;saveenv;busmux diu

but the ugliness comes from the hardware design, so we have no reasoin
to camouflage it.  Let people see what is going on under the hood - if
this is done in a clean way, you don't have to be ashamed.

> > It makes no sense adding this to saveenv, because there will be othe
> > rplaces in the code that need to to the same - like if it's NAND
> > flash, you will probabaly need to do the same for all NAND related
> > commands.
> 
> Actually, the same code works for saving the environment to NAND flash.
> This is how the board code will look:

But this again works only for thesingle use case of the "saveenv"
command.

This makes no sense - it's still broken, and trying to fix it using
this approach does not scale and/or does not work.

> If you have a better idea, then I'm all ears.  I could implement my own
> version of saveenv(), but the current design of U-boot does not allow me
> to have two functions called saveenv().

Your problem is not just "saveenv".  Your problem is that you need to
switch modes.  So provide a command to switch modes, and use this.

Yes, this is a pain, and it is less from perfect. But you cannot have
a perfect solution on broken hardware.  It makes no sense to waste
lots of efforts on this.  If you're ugly, you are still ugly even if
wearing expensive clothes.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Keep your head and your heart going in the right  direction  and  you
will not have to worry about your feet.