[OpenBIOS] [RFC] Switch OFMEM to use phys_addr_t for physical addresses

[Mark Cave-Ayland]

Andreas Färber wrote:

> Looking good so far, except for one issue.
> 
>> Index: libopenbios/ofmem_common.c
>> ===================================================================
>> --- libopenbios/ofmem_common.c    (revision 946)
>> +++ libopenbios/ofmem_common.c    (working copy)
> 
>> @@ -463,7 +463,7 @@
>> }
>>
>> /* if align != 0, phys is ignored. Returns -1 on error */
>> -ucell ofmem_claim_phys( ucell phys, ucell size, ucell align )
>> +ucell ofmem_claim_phys( phys_addr_t phys, ucell size, ucell align )
>> {
>>     OFMEM_TRACE("ofmem_claim phys=" FMT_ucellx " size=" FMT_ucellx
>>                 " align=" FMT_ucellx "\n",
> 
> This will break the trace code on sparc32 and ppc64. We'll need some 
> target-specific define to replace FMT_ucellx in the trace code for the 
> "phys" parameter since it may be larger.

Ah good spot - would FMT_phys_addr_t be a suitable name for this? I'll 
make the defaults for SPARC64 and PPC the same as those for ucell.

>> @@ -514,7 +516,8 @@
>> static void
>> mem_claim( void )
>> {
>> -    ucell phys=-1UL, size, align;
>> +    ucell size, align;
>> +    phys_addr_t phys=-1UL;
> 
> I'd appreciate if you could enhance the code you refactored with spaces 
> around the assignment, i.e. phys_addr_t phys = -1UL;, for readability.
> 
>> @@ -548,7 +552,8 @@
>> static void
>> mem_retain ( void )
>> {
>> -    ucell phys=-1UL, size, align;
>> +    ucell size, align;
>> +    phys_addr_t phys=-1UL;
> 
> Dito.
> 
>> @@ -393,7 +394,7 @@
>>     }
>> }

Yup no worries - I know some people don't like whitespace cleanups in 
the same patch, but it seems fine to me.

>> -void ofmem_map_pages(ucell phys, ucell virt, ucell size, ucell mode)
>> +void ofmem_map_pages(phys_addr_t phys, ucell virt, ucell size, ucell 
>> mode)
>> {
>>     return map_pages(phys, virt, size, mode);
>> }
> 
> Note that I find it slightly odd that there is a (trivial) 
> ofmem_map_ranges() function in arch/sparc64/lib.c, but that's unrelated 
> to phys_addr_t.
> What's the difference to ofmem_map() in ofmem_common.c? We should either 
> share it, rename it or drop it mid-term.

It looks like it's called from ofmem_arch_early_map_pages() depending 
upon whether or not the TLB entry is marked as locked. I know that the 
OFMEM API does still need a bit a work looking at this comment in ofmem.h:

/* TODO: temporary migration interface */

I'm fairly sure we could simplify this if we had help from people 
experienced with other platforms, but I'm not sure it's something I yet 
feel confident enough to do myself. It's something we could definitely 
consider for the future though.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: +44 870 608 0063

Sirius Labs: http://www.siriusit.co.uk/labs