Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Friday 06 December 2013, Leif Lindholm wrote:
> 
> 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.

That's ok, too. I usually prefer the IS_ENABLED() method if there is only
one caller, since it's harder to get wrong, but the result is similar.

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

Ok, makes sense.

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

I see. Maybe put the typecast in that final assignment then?

	Arnd