Hi,
Am 30.12.2010 um 20:08 schrieb Mark Cave-Ayland:
On 30/12/10 18:20, Andreas Färber wrote:
This does not really address my comments...
Ah okay - I was still hoping that no news was good news and I admit I wanted something in place to unblock the SPARC32 OFMEM patchset.
+/* Private functions for mapping between physical/virtual addresses */ +inline phys_addr_t +va2pa(unsigned long va) +{
- if (va >= OF_CODE_START && va < OF_CODE_START + OF_CODE_SIZE) {
The latter will overflow on 32-bit ppc, and (va < 0) == 0 so that branch would never be taken. Is there no compiler warning?
- return (phys_addr_t)get_rom_base() - OF_CODE_START + va;
Again, what's the reason for duplicating this calculation?
- } else {
- return (phys_addr_t)va;
- }
+}
Only because I was in two minds with regards to whether to keep pa2va and va2pa as "symmetrical" as possible. I'm happy to go with whatever you think is best.
Well I did point out that we're missing the opposite direction, so I was hoping you would add an ofmem_translate_inverse() or so, so that both could almost symmetrically use those functions, not just for ppc.
What about the following trivial implementation instead?
phys_addr_t va2pa(unsigned long va) { ucell mode; return ea_to_phys(va, &mode); // calls ofmem_translate() internally for the 'else' case }
Yes - that seems to work in my tests here.
+inline unsigned long +pa2va(phys_addr_t pa) +{
- if ((pa - get_rom_base() + OF_CODE_START >= OF_CODE_START) &&
- (pa - get_rom_base() + OF_CODE_START < OF_CODE_START +
OF_CODE_SIZE))
- return (unsigned long)pa - get_rom_base() + OF_CODE_START;
- else
- return (unsigned long)pa;
+}
Trailing whitespace?
Missing braces for QEMU style.
Ooops my bad. Does the attached patch look better?
Mostly, apart from the above issue. If you don't mind, I would apply this part myself tomorrow, moving your two functions down to avoid the static prototype and without my explanatory C++-style comment.
Andreas