[U-Boot] [PATCH 5/6] Support environment anywhere within erase area

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808312127560.6742 at axis700.grange> you wrote:
> 
> Erase area - this is all we erase, as opposed to one erase sector. On NOR 
> this is limited by the environment size, on NAND by the number of blocks - 
> the fifth parameter in the configuration file. This area may contain other 
> useful data, which is first read in, then the whole area is erased, the 
> environment is replaced in the read-in data, and it is written back - this 
> is what I call the back-up process in the code.

We should not do that. We should erase only those sectors / blocks
that we are actually attemmpting to write to.

> > And where's the difference between NAND and NOR flash? For  NOR,  the
> > minimum "erase region" is a "block", either, which also can be 256KiB
> > large.
> 
> This patch enables this for NOR - NAND support comes first with patch 6. 
> So, it just enables placing the actual environment at any offset in the 
> "erase area".

I  don't  understand.  Why  would  such  an  offset  be  needed?  The
envrionment always starts right at the beginning of a sector or erase
unit  or  block or however the storage device might call the smallest
unit it can handle.

> > > +	/*
> > > +	 * Support environment anywhere within erase sectors: read out the
> > > +	 * complete area to be erased, replace the environment image, write
> > > +	 * the whole block back again.
> > > +	 */
> 
> This comment should actually serve as an explanation...

But I don't understand it. What are "erase sectors"? How is the "area
to be erased" defined? And what is the "block" (a flash  block?)  you
are writing?

> > You are talking about "several pages" above. Where is this refelected
> > in the code?
> 
> You mean in the commit comment? There I am talking about the future code - 
> NAND case, which is not yet in this patch.

How shall anybody understand this, then? Description and code are not
in sync - this is bad.

> > Frankly, I don't understand what you are trying to do. Please explain
> > your implementation.
> 
> Hope, it is a bit clearer now. If not, please ask, will try to explain 
> again.

You must provide a description of what you are actually doing.  I  do
not understand your code.

> Indeed, this patch series changes the programme in a non-trivial way, 
> that's why I had to split this "NAND-support" into several patches, still 
> some of them seem to be not clear enough.

Well, splitting complex things that obviously  belong  together  into
smaller  pieces  and  confronting  the  reviewer  with  a  puzzle  of
unrelated bits does not exactly make things "clear".

See previous message - I guess the whole splitting  is  just  contra-
productive.

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
If something is different, it's either better or worse,  and  usually
both.                                                    - Larry Wall