Becky Bruce becky.bruce@freescale.com wrote:
Becky: the fact that Haavard's code is breaking is not considered an indication of a deficiency in his code.
I'm not calling the code deficient, just a bit inconsistent, and it wasn't clear to me why. When code uses the same value 1) by mapping it and 2) by dereferencing it directly, that's a bit strange. Why map it in some cases and in not others?
I agree, that is inconsistent. However, you "fixed" the code which was actually doing it correctly...we should instead fix the code which is incorrectly using the physical address as a virtual one.
While doing that, we could also consider dropping that hideous start array altogether and instead provide a handful of accessors for locating sector start addresses on the flash.
How is that supposed to work when VA!=PA or when the VA can change? This is one of the reasons that it seemed to make sense to modify the driver as I did - it should now be able to work when VA!=PA as well as when we're 1:1. I could find no users that seemed to need the dynamic mapping. However, if anyone does need to *dynamically* map the flash in and out with a different VA each time, then we do need to do things a bit differently and we should look into a solution for that.
I don't think anything needs a dynamic mapping right now, but if we're going to _ever_ support such systems in the future (and your 36-bit PA system is a likely candidate, isn't it?) we have to stop locking ourselves into a static mapping.
Clearly, I'm not the expert on the CFI code, so when I published that patch I expected someone to smack me if I was being a moron :)
I apologize for not doing so earlier ;-)
Anyway, I have to take most of the blame for this situation...if I had paid attention and flagged the problem earlier, much of this could have been avoided.
I'm hoping we can avoid too much pointing of fingers and work towards a reasonably future-proof solution which works well on all platforms.
If the CFI driver kind of allowed for VAs before (but incompletely / incorrectly), then this dis not cause problems on any systems using a strict 1:1 mapping.
Any changes to the code to correctly support other mappings must be done in a way that they (1) do not break and (2) do not add additional burdon on systems with a simple 1:1 mapping.
Agreed, there shouldn't be any burden on those systems.
Agree too, as long as "those systems" does not include common code which needs to run on all systems.
Everything is treated as virtual unless it's being used for hardware setup.
Thisis NOT correct. U-Boot usually does NOT use virtual addresses. Only very few systems do, and these must care not to disturb the majority of systems which do no need to differentiate between physical and virtual addresses.
I'm not saying it *is* a VA as far as U-Boot knows, but that it is *treated* as one, as mentioned above. And this code was not expected to disturb the 1:1 case.
Exposing VAs to the user interface and board code is IMO a very bad idea, however. And that is what's happening now -- instead of localizing the VAs to the CFI flash driver, it is now spread all over the place.
A lot of code had been just using the PA as a VA, because things were always mapped 1-1.
Not only were, but _are_ and _will_be_.
Of course - that should continue to be the default case unless it needs to differ for some reason.
Yes, and that's why the external interface (at least the command line and board configuration files) needs to use PA.
Indeed I'm deeply trouble when log standing rules get silently bent and even broken.
I agree 100% that 1:1 support should not be disturbed, since that's the default case. There was absolutely no intent here to bend any "log<sic ;-)> standing rules", and nothing has been done here that should have any impact on 1:1 as far as I'm aware.
Agree, 1:1 isn't the issue, it's two different systems with !1:1 which have started making incompatible changes to the CFI driver.
Haavard