[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 <20090901123829.55fcb24e at hskinnemoen-d830> you wrote:
> >
> > And that is EXACTLY why I'm trying to advocate: Keep the additional
> > 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.

> 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...

> > But as a matter of principle, I don't want to reduce the performance
> > _and_ increase the code size just because a driver that used to work
> > got broken. I want to fix the driver instead!
> 
> 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.

> > > If you want to run with data caches enabled by default, then it would
> > > probably make more sense if you invested efforts in extending the CFI
> > > driver to provide hooks / callbacks to (temporarily) switch of data
> > > cache for the memory range in question. This wouls allow you to run
> > > with DC enabled on the flash area, and still use the CFI driver.
> > 
> > But that is exactly how map_physmem() works -- it allows the CFI driver
> > (or any other driver) to request the caches to be bypassed for a given
> > physical address range, possibly resulting in a different virtual
> > address (though for backwards compatibility, all platforms except AVR32
> > simply return the physical address unchanged.)
> 
> I have to admit that I have no idea how map_physmem() used to work or
> how it works now; at this point, I don;t care about implementation
> details, I try to focus only on the overall design.

It works exactly the same way now as it used to work. The difference is
that its return value (which is basically just an architecture-specific
cookie, aka. virtual address) is exposed to a much larger part of the
system.

> I think  your  double  mapping  is  a  hightly  architecture-specific
> feature  which I do not like at all, but if you are happy with it and
> it works for you  I  cannot  and  will  not  prevent  it.

Yes, it was always meant to be an architecture-specific thing. Though I
think some MIPS- and SH-based processors do something very similar.

But in order to utilize architecture-specific features, we need an
architecture-independent abstraction and that's basically what
map_physmem() is.

>  But  after
> 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.

> 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.

> Detlev Zundel suggested to use this as  a  chance  to  come  up  with
> something  like a cache API which then could be used by other drivers
> as well.

My suggestion is to use map_physmem() and unmap_physmem(). It exists
today, and it works, provided that we keep its usage internal to the
driver and don't expose whatever architecture-specific values it
returns.

> 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?

> > > Such changes will have it much easier to find their way into mainline
> > > than adding a proprietary flash driver.
> > 
> > It certainly won't be a proprietary driver; if anything, it would be a
> > variant of the driver currently used by ATSTK1000.
> 
> 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?
At least STK1000 has a working flash driver.

Haavard