[U-Boot] [PATCH 07/11] flash/cfi_flash: Use virtual sector start address, not phys

[Kumar Gala]

On Jan 14, 2009, at 5:34 AM, Haavard Skinnemoen wrote:

> Kumar Gala wrote:
>>
>> On Dec 3, 2008, at 11:04 PM, Becky Bruce wrote:
>>
>>> include/flash.h was commented to say that the address in
>>> flash_info->start was a physical address.  However, from u-boot's
>>> point of view, and looking at most flash code, it makes more
>>> sense for this to be a virtual address.  So I corrected the
>>> comment to indicate that this was a virtual address.
>>>
>>> The only flash driver that was actually treating the address
>>> as physical was the mtd/cfi_flash driver.  However, this code
>>> was using it inconsistently as it actually directly dereferenced
>>> the "start" element, while it used map_physmem to get a
>>> virtual address in other places.  I changed this driver so
>>> that the code which initializes the info->start field calls
>>> map_physmem to get a virtual address, eliminating the need for
>>> further map_physmem calls.  The code is now consistent.
>>>
>>> The *only* place a physical address should be used is when defining
>>> the
>>> flash banks list that is used to initialize the flash_info  
>>> struct.  I
>>> have fixed the one platform that was impacted by this change
>>> (MPC8641D).
>>>
>>> Signed-off-by: Becky Bruce <beckyb at kernel.crashing.org>
>>> ---
>>> drivers/mtd/cfi_flash.c       |   53 +++++++++++++++++
>>> +----------------------
>>> include/configs/MPC8641HPCN.h |    2 +-
>>> include/flash.h               |    2 +-
>>> 3 files changed, 26 insertions(+), 31 deletions(-)
>>
>> Stefan,
>>
>> Have you reviewed this.  I'm not sure if remvoing the map_physmem()  
>> is
>> the right answer because I'm assuming Haavard added them for a reason
>> (AVR32?). Should we instead change info->start to be a phys_addr_t?
>
> There was definitely a reason: On AVR32, the caches are enabled by
> default, and I prefer that they stay that way since it makes  
> everything
> faster. But when dealing with the flash, we must bypass the caches by
> using a different virtual address which maps to the same physical
> memory.
>
> From what I can tell from a quick scan, this patch doesn't seem to
> change that; all it does is push the map_physmem() call a bit up the
> stack so it gets called less often. That seems like a good thing to  
> me.
>
> However, what I don't understand is if the 'start' array is really
> supposed to hold virtual addresses, why isn't it an array of pointers?

That is good to hear.  Clearly this patch doesn't deal with the  
mapping required by any AVR32 platforms.  If you had the resources to  
keep the flash mapping you create around "for ever" or not.

- k