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

[Andreas Färber]

Hi,

Am 07.11.2010 um 12:16 schrieb Mark Cave-Ayland:
> Here is my first attempt at switching over OFMEM to use phys_addr_t  
> for physical addresses. It was mainly a case of going through all of  
> the OFMEM APIs by hand and then updating the definitions used to  
> hold physical addresses, along with swapping over range_t to use  
> phys_addr_t on the assumption that sizeof(phys_addr_t) >=  
> sizeof(ucell).

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.

Btw if we start deviating from ucell, we might as well use size_t size  
in a second step, but I don't see a real use case yet.
> Index: arch/sparc64/ofmem_sparc64.c
>
> --- arch/sparc64/ofmem_sparc64.c	(revision 946)
> +++ arch/sparc64/ofmem_sparc64.c	(working copy)
> @@ -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 @@
>     }
> }
>
> -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.

Regards,
Andreas