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.