[U-Boot] [PATCH] atngw100: Use virtual address in CONFIG_ENV_ADDR

[Haavard Skinnemoen]

Wolfgang Denk <wd at denx.de> wrote:
> Dear Haavard Skinnemoen,
> 
> In message <20090901134257.59961e79 at hskinnemoen-d830> you wrote:
> >
> > > > complexity (which can be kept to a minimum) localized to a handful of
> > > > drivers, and don't change the command line interface or the board
> > > > configuration interface.
> > > 
> > > We're not doing this. At least not intentionally.
> > 
> > Good. Then let's please put a stop to the madness of exposing virtual
> > addresses all over the place.
> 
> But that's what we've been doing all the time. You just did not notice
> it because of the usual 1:1 mapping.

Up until this commit, yes:

commit 09ce9921a7d8b1ce764656b14b42217bbf4faa38
Author: Becky Bruce <beckyb at kernel.crashing.org>
Date:   Mon Feb 2 16:34:51 2009 -0600

    flash/cfi_flash: Use virtual sector start address, not phys

after that, NGW100 support is broken because virtual addresses are no
longer an implementation detail and are being exposed all over the
place.

> On a 32 bit system with 36 bit physical addresses you cannot use a
> physical address when running the "md" command, for example.

Yes you can, if you teach the "md" command to map it at a virtual
address first. Again, map_physmem() makes this possible without
changing the external interface in any way.

> we
> always assumed that the 32 bit VA we used matched 1:1 to a PA with
> the missing high bits set to 0, right?

Yes, but how can you possibly access an arbitrary 36-bit address using
that setup?

> > > The discussion we had was based on our knowledge about existing
> > > systems, and needs to also handle more complex situations like for
> > > example 32 bit systems with 36 bit physical addresses.
> > 
> > As far as I understand, the only difference for such systems is that
> > keeping 64-bit physical addresses around are a bit more costly than
> > passing around 32-bit virtual pointers. But presumably those systems
> > have enough memory to make it a non-issue...
> 
> That's not true. There are 32 bit systems with bigger physical
> address spaces. 

Which part of what I said isn't true? The part about some systems might
require 64-bit variables to store a physical address or that such
variables take more storage than a 32-bit virtual address?

> And note that this is not a new thing. We have been doing this allt
> he time - just without ever explicitly mentioning it, because so far
> nobody ever complained about it.

Doing what exactly? Limiting 36-bit PA systems to only use the lower
4GB of memory because VA must always equal PA come hell or high water?

> > > Agreed - assuming it is possible without hurting the majority of
> > > other existing configurations.
> > 
> > Yes, that is indeed possible, as evidenced by the fact that it used to
> > work without hurting any other configurations. It took another "special
> > case" to break it.
> 
> My understanding is that the special case is yours - using a non-1:1
> mapping.

But the other system must be a special case too -- otherwise it would
work fine without commit 09ce9921a7d8b1ce764656b14b42217bbf4faa38. That
commit does not make any difference whatsoever on 1:1 systems.

If the other system isn't a special case, why don't we just revert that
commit?

> > > discussions  with  Stefan Roese and Detlev Zundel it seems to me that
> > > map_physmem() is probably not the right approach (judging  only  from
> > > the  name).  We  should  not try to fix cache attributes by modifying
> > > addresses / address maps
> > 
> > And why not? That's what Linux does, and it works wonderfully across 26
> > different architectures.
> 
> We do not want to implement a full-fledged OS with virtual memory and
> on-demand paging and all that stuff in U-Boot.

Then why are you forcing me to implement it for AVR32?!?

> > > Instead, we should really extend the CFI driver such that it does not
> > > matter if the addresses you are passing refer to cached or uncached
> > > memory, and then provide hooks in the CFI driver to allow for testing
> > > if cache is enabled, and switching cache off if it is.
> > 
> > What's the advantage of such an approach? It sounds much more
> > complicated from the driver's point of view as well.
> 
> The advantage is that other drivers with similar needs can use it as
> well.

But they can use map_physmem() today! It allows you to specify exactly
what caching properties you need. The fact that it _may_ return a
virtual address which is different from the physical one just allows
more flexibility in how the architecture chooses to implement it!

> > > To me such an approach makes much more sense, as it  is  generic  and
> > > can be used by other drivers and other architectures - even if it may
> > > come at the cost of more effort on your systems.
> > 
> > In what way is it more generic? In what way can map/unmap_physmem() not
> > be used with other drivers and architectures?
> 
> On other architectures it is not possible to map the same memory area
> multiple times with different attributes.

So what? They can just return the physical address unchanged. It's not
_mandatory_ to remap anything...

>  Remapping  addresses  thus
> cannot  help  -  you  really  have  to  switch  the  MMO resp. memory
> controller setinngs to turn data cache on or off.

Yes, and map_physmem() allows an implementation to do that.

> > > Well, but board/atmel/atstk1000/flash.c _is_ a proprietary driver for
> > > some flash chips that seem to be CFI conformant at first glance.  You
> > > would not get such a driver into mailine any more. 
> > 
> > So I guess dropping support for ATNGW100 is the only remaining option?
> 
> No, why? We're discussing ways to fix the problems, aren;t we?

Because we don't seem to be getting anywhere. You're ignoring all my
arguments about why it is necessary to care about virtual addresses
internally in the driver, yet you think it's fine to expose virtual
addresses to the rest of the world, causing incompatibilities all over
the place.

If your design doesn't fit existing and working hardware, it's your
design which is broken, not the hardware. Especially when other
software gets along with it just fine, and even u-boot used to work
without problems.

> > At least STK1000 has a working flash driver.
> 
> Only because it was added so long ago, before we were more consequent
> about using the generic driver. 

And thank $DEITY for that!

> It would be a good idea to clean up this board  support,  remove  the
> proprietary  flash driver and use the CFI driver instead. Patches are
> welcome.

You must be joking. Replacing a working driver with a broken one? Why
the hell would anyone do that?

Haavard