[U-Boot] [PATCH v2 2/3] avr32: Use uncached() macro to get an address for SDRAM init

[Detlev Zundel <dzu@denx.de>]

Hi Haavard,

> Detlev Zundel <dzu@denx.de> wrote:
>> > Problem is that in order to make the CFI driver work on avr32, we need
>> > to change the map_physmem() macro to return the physical address
>> > unchanged.
>> 
>> I see.  And I presume you cannot tell the situation apart inside
>> map_physmem?
>
> I don't think so. How do you propose we do that?

I don't know, that's why I'm asking.  

Let's take a step back and please excuse my ignorant question - why
exactly does the CFI driver need the physical address unchanged?  Isn't
the real constraint that the address requested by CFI is uncached?  Why
can't this be done by map_physmem()?

>> > The map_physmem() macro currently does exactly the same thing as the
>> > uncached() macro, and the unmap is a noop, but the next patch changes
>> > it in order to fix the CFI driver. If the next patch is applied without
>> > this patch being applied first, the SDRAM driver will do cached
>> > accesses during initialization, and that may cause the initialization
>> > to fail.
>> 
>> Why not include a note to this extent into the git commit message?  This
>> would be a great help for other people to later understand why this
>> change has been done the "backward way" that it was.
>
> The commit message already contains this:
>
> The paging system which is required to set up caching properties has not
> yet been initialized when the SDRAM is initialized. So when the
> map_physmem() function is converted to return the physical address
> unchanged, the SDRAM initialization will break on some boards.
>
> which is essentially the same thing, isn't it?

For me this is not the same - it does not include a word why the change
which you agree "looks backward" is something that we want to do.  For
me something like this would give me more information:

  Unfortunately we cannot make "map_physmem()/unmap_physmem()" on the
  AVR32 platform work with the CFI driver as it works on other
  platforms. [I don't understand why this is the case, so your
  explanation would go here ;) ] To still fix the issue we deliberately
  replace these generic routines by AVR32 specific routines.  If someone
  can fix this using the generic patterns, patches are welcome.

I believe that good docmumentation should include such pro- and con
reasoning so that code changes can be comprehended even after the fact.

Cheers
  Detlev

-- 
Emacs seems a more likely candidate  to contain a mail system than the
mail system to contain an Emacs, so this is the way it was done.
                                            -- Bernard S. Greenberg
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de