lkml.org 
[lkml]   [2013]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 2/3] arm: Add [U]EFI runtime services support
Date
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.

> +/*
> + * 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.

> +static __init int is_discardable_region(efi_memory_desc_t *md)
> +{
> +#ifdef KEEP_ALL_REGIONS
> + return 0;
> +#endif

IS_ENABLED() again.

> + 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:

> + 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.

> +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?

Arnd


\
 
 \ /
  Last update: 2013-12-06 03:41    [W:0.947 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site