Hi Arnd,

Thank you for your comments.

On Fri, Dec 06, 2013 at 02:59:48AM +0100, Arnd Bergmann wrote:

On Thursday 28 November 2013, Leif Lindholm wrote:

...

@@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p) sanity_check_meminfo(); arm_memblock_init(&meminfo, mdesc); +#ifdef CONFIG_EFI

• uefi_memblock_arm_reserve_range();

+#endif

Better use

if (IS_ENABLED(CONFIG_EFI))

here for readability and better build-time checking of the interface when CONFIG_EFI is disabled.

I think I'll do what Grant suggested and stub it out in asm/uefi.h.

...

+/*

• • Returns 1 if 'facility' is enabled, 0 otherwise.
• */

+int efi_enabled(int facility) +{

• return test_bit(facility, &arm_uefi_facility) != 0;

+} +EXPORT_SYMBOL(efi_enabled);

I'd use EXPORT_SYMBOL_GPL() unless there is a documented reason why a symbol should be available to GPL-incompatible modules.

So ... this is duplicating x86 code which Matt Fleming has promised to make generic. I'd prefer to keep it identical to x86 until this copy goes away.

...

+static __init int is_discardable_region(efi_memory_desc_t *md) +{ +#ifdef KEEP_ALL_REGIONS

• return 0;

+#endif

IS_ENABLED() again.

Ok.

...

• if (md->attribute & EFI_MEMORY_RUNTIME)

• return 0;
  

• switch (md->type) {

+#ifdef KEEP_BOOT_SERVICES_REGIONS

• case EFI_BOOT_SERVICES_CODE:
• case EFI_BOOT_SERVICES_DATA:

+#endif

and I think it can be used here too:

switch (md->type) { case EFI_BOOT_SERVICES_CODE: case EFI_BOOT_SERVICES_DATA: if (IS_ENABLED(KEEP_BOOT_SERVICES_REGIONS)) return 1; /* fallthrough */ case EFI_ACPI_RECLAIM_MEMORY:

Will redesign for next version.

...

• memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap,

• 			 uefi_boot_mmap_size);
  

• if (!memmap.phys_map)

• return 1;
  

• p = memmap.phys_map;
• e = (void *)((u32)p + uefi_boot_mmap_size);

You are doing a lot of type casts here, which is normally an indication that you have the types wrong in some way. I can't spot a mistake here, but maybe you can give it some more thought and see if it can be changed.

Grant made much the same comments here. It is possible the change to the DT bindings to have all addresses 64-bit actually makes this go away. I will revisit for next version.

...

+static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry) +{

• u64 va;
• u64 paddr;
• u64 size;
• *entry = *md;
• paddr = entry->phys_addr;
• size = entry->num_pages << EFI_PAGE_SHIFT;
• /*

• * Map everything writeback-capable as coherent memory,
  

• * anything else as device.
  

• */
  

• if (md->attribute & EFI_MEMORY_WB)

• va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
  

• else

• va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);
  

Same here. Why is 'va' a u64?

Because the field in the structure (defined by UEFI) is 64-bit:

entry->virt_addr = va;

/ Leif