[PATCH v3 part1 04/11] ARM64 / ACPI: Introduce arm-core.c and its related head file

[Graeme Gregory]

On Fri, Apr 25, 2014 at 07:38:48PM +0100, Mark Rutland wrote:
> On Fri, Apr 25, 2014 at 05:53:20PM +0100, Graeme Gregory wrote:
> > On Fri, Apr 25, 2014 at 04:51:47PM +0100, Mark Rutland wrote:
> > > Hi Hanjun,
> > > 
> > > On Fri, Apr 25, 2014 at 02:20:10PM +0100, Hanjun Guo wrote:
> > > > ACPI core need lots extern variables and functions which should
> > > > be provided by arch dependent code to make itself compilable. so
> > > > introduce arm_core.c and its related header file here.
> > > > 
> > > > acpi_boot_table_init() will be called in setup_arch() before
> > > > paging_init(), so we should use eary_ioremap() mechanism here
> > > > to get the RSDP and all the table pointers, with this patch,
> > > > we can get ACPI boot-time tables from firmware on ARM64.
> > > > 
> > > > Signed-off-by: Al Stone <al.stone at linaro.org>
> > > > Signed-off-by: Graeme Gregory <graeme.gregory at linaro.org>
> > > > Signed-off-by: Hanjun Guo <hanjun.guo at linaro.org>
> > > > Signed-off-by: Tomasz Nowicki <tomasz.nowicki at linaro.org>
> > > > ---
> > > >  arch/arm64/include/asm/acpi.h |   53 +++++++++++++++++++
> > > >  arch/arm64/kernel/setup.c     |    4 ++
> > > >  drivers/acpi/Makefile         |    2 +
> > > >  drivers/acpi/plat/Makefile    |    1 +
> > > >  drivers/acpi/plat/arm-core.c  |  113 +++++++++++++++++++++++++++++++++++++++++
> > > >  5 files changed, 173 insertions(+)
> > > >  create mode 100644 drivers/acpi/plat/Makefile
> > > >  create mode 100644 drivers/acpi/plat/arm-core.c
> > > > 
> > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > > > index e3e990e..3ac9dfb 100644
> > > > --- a/arch/arm64/include/asm/acpi.h
> > > > +++ b/arch/arm64/include/asm/acpi.h
> > > > @@ -19,6 +19,43 @@
> > > >  #ifndef _ASM_ACPI_H
> > > >  #define _ASM_ACPI_H
> > > >  
> > > > +#include <asm/cacheflush.h>
> > > > +
> > > > +#include <linux/init.h>
> > > > +
> > > > +#define COMPILER_DEPENDENT_INT64	s64
> > > > +#define COMPILER_DEPENDENT_UINT64	u64
> > > 
> > > Is there any reason this can't be in a common ACPI header shared be ia64
> > > and x86 too? Given we already have generic types for this it seems
> > > pointless to define this in each architecture.
> > > 
> > > It looks like include/acpi/actypes.h tries to do that already...
> > > 
> > Yes I think we can replace that with uint64_t and int64_t types.
> > 

I have dug into this deeper and since include/acpi/platform/aclinux.h
defines ACPI_USE_SYSTEM_INTTYPES then these defines should not be used
at all and it should be safe for us to just not have them in arm64

I guess they are historic in x86/ia64

> > > > +
> > > > +/*
> > > > + * Calling conventions:
> > > > + *
> > > > + * ACPI_SYSTEM_XFACE        - Interfaces to host OS (handlers, threads)
> > > > + * ACPI_EXTERNAL_XFACE      - External ACPI interfaces
> > > > + * ACPI_INTERNAL_XFACE      - Internal ACPI interfaces
> > > > + * ACPI_INTERNAL_VAR_XFACE  - Internal variable-parameter list interfaces
> > > > + */
> > > > +#define ACPI_SYSTEM_XFACE
> > > > +#define ACPI_EXTERNAL_XFACE
> > > > +#define ACPI_INTERNAL_XFACE
> > > > +#define ACPI_INTERNAL_VAR_XFACE
> > > > +
> > > > +/* Asm macros */
> > > > +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()
> > > 
> > > This almost certainly does not do what you think it does.
> > > 
> > > flush_cache_all walks the architected levels of cache visible to the
> > > current CPU (i.e. those in CLIDR_EL1), and walks over each cache line at
> > > that level, cleaning and evicting it. It also flushes the I-cache (which
> > > I don't think you care about here).
> > > 
> > > This is NOT safe if the cache is enabled. Lines can migrate between
> > > levels in the middle of the sequence.
> > > 
> > > In an SMP system this does NOT guarantee that data is evicted to memory,
> > > even if the cache is disabled. Other CPUs with caches enabled can
> > > acquire a cacheline (even if dirty) and it can sit in their cache.
> > > 
> > > In a UP system or an SMP system where all other architected caches are
> > > disabled (and flushed) this does NOT guarantee that data hits memory. In
> > > the presence of a system-level cache this will simply flush the data out
> > > to said system-level rather than memory.
> > > 
> > > I believe the intent here is to have something analogous to WBINVD for
> > > use in idle. Unfortunately there simply isn't anything analogous.
> > > Luckily in the presence of PSCI, the PSCI implementation should do all
> > > of the cache maintenance required to prevent any data loss and/or
> > > corruption, and anything we need to have visible to noncacheable
> > > accesses (i.e. flushed out to memory) we should be able to flush by VA.
> > > 
> > > This maintenance is unsafe, and shouldn't be necessary on any sane
> > > system. Please get rid of it. I would very much like to get rid of
> > > flush_cache_all() before its misuse spreads further.
> > > 
> > Thanks for explanation Mark, you are correct on x86 it is defined as
> > wbinvd().
> > 
> > I think looking at where it is actually used we can make this an empty
> > macro on arm64 for now. Where it used are areas we don't currently
> > execute and need arm64 replacements or refactorising to remove x86isms.
> 
> That sounds good. Is it worth putting a warn or similar there just in
> case?
> 
> > 
> > > > +/* Basic configuration for ACPI */
> > > > +#ifdef	CONFIG_ACPI
> > > > +extern int acpi_disabled;
> > > > +extern int acpi_noirq;
> > > > +extern int acpi_pci_disabled;
> > > > +extern int acpi_strict;
> > > 
> > > This looks very odd. Why are these prototypes not coming from a header?
> > > If they're defined in the same place, why not move the disable_acpi
> > > function there?
> > > 
> > 
> > This is a header :-)
> 
> True; I must get my eyes tested. :)
> 
> Are these variables expected to be used by needed by other code, or are
> they just for the benefit of the static inlines in this header?
> 
> > I think this is a peculiarity of how acpica is incorporated into linux
> > but will check.
> 
> Ok.
> 
> > 
> > > > +static inline void disable_acpi(void)
> > > > +{
> > > > +	acpi_disabled = 1;
> > > > +	acpi_pci_disabled = 1;
> > > > +	acpi_noirq = 1;
> > > > +}
> > > 
> > > [...]
> > > 
> > > > +/*
> > > > + * __acpi_map_table() will be called before page_init(), so early_ioremap()
> > > > + * or early_memremap() should be called here.
> > > > + */
> > > > +char *__init __acpi_map_table(unsigned long phys, unsigned long size)
> > > > +{
> > > > +	if (!phys || !size)
> > > > +		return NULL;
> > > 
> > > Is there any reason that tables can't exist at physical address 0? It's
> > > entirely valid to have memory there.
> > > 
> > > [...]
> > > 
> > On ARM64 there is not, we can fix this.
> > 
> > > > +int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> > > > +{
> > > > +	*irq = -1;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
> > > 
> > > This appears to be missing a giant warning that it does nothing useful.
> > > 
> > > I was under the impression that we were meant to use 0 to represent the
> > > lack of an interrupt these days, too...
> > > 
> > We can fix this.
> 
> Sounds good!
> 
> Cheers,
> Mark.
> 

Thanks

Graeme