lkml.org 
[lkml]   [2014]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH v2 08/18] ARM64 / ACPI: Get the enable method for SMP initialization in ACPI way
    From
    Date
    Hi Hanjun,

    On Mon, 2014-08-04 at 23:28 +0800, Hanjun Guo wrote:
    > --- a/arch/arm64/include/asm/acpi.h
    > +++ b/arch/arm64/include/asm/acpi.h
    > @@ -14,6 +14,27 @@
    >
    > /* Basic configuration for ACPI */
    > #ifdef CONFIG_ACPI

    By having this preprocessor conditional in the header leads
    to a proliferation of preprocessor conditionals since any
    code that includes this header will also need to have
    preprocessor conditionals. Another down side of having
    this is that this code will not get a build test for
    builds with CONFIG_ACPI=n.

    > +/*
    > + * ACPI 5.1 only has two explicit methods to
    > + * boot up SMP, PSCI and Parking protocol,
    > + * but the Parking protocol is only defined
    > + * for ARMv7 now, so make PSCI as the only
    > + * way for the SMP boot protocol before some
    > + * updates for the ACPI spec or the Parking
    > + * protocol spec.
    > + *
    > + * This enum is intend to make the boot method
    > + * scalable when above updates are happended,
    > + * which NOT means to support all of them.
    > + */
    > +enum acpi_smp_boot_protocol {
    > + ACPI_SMP_BOOT_PSCI,
    > + ACPI_SMP_BOOT_PARKING_PROTOCOL,
    > + ACPI_SMP_BOOT_PROTOCOL_MAX
    > +};
    > +
    > +enum acpi_smp_boot_protocol smp_boot_protocol(void);

    The name smp_boot_protocol() seems like it would be a generic
    routine, but it is acpi specific. Maybe:

    enum acpi_boot_protocol_type {...};

    enum acpi_boot_protocol_type acpi_boot_protocol(void);

    > --- a/arch/arm64/kernel/cpu_ops.c
    > +++ b/arch/arm64/kernel/cpu_ops.c
    > @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
    > return NULL;
    > }
    >
    > +#ifdef CONFIG_ACPI
    > +/*
    > + * Get a cpu's boot method in the ACPI way.
    > + */
    > +static char * __init acpi_get_cpu_boot_method(void)
    > +{
    > + /*
    > + * For ACPI 5.1, only two kind of methods are provided,
    > + * Parking protocol and PSCI, but Parking protocol is
    > + * specified for ARMv7 only, so make PSCI as the only method
    > + * for SMP initialization before the ACPI spec or Parking
    > + * protocol spec is updated.
    > + */
    > + switch (smp_boot_protocol()) {
    > + case ACPI_SMP_BOOT_PSCI:
    > + return "psci";
    > + case ACPI_SMP_BOOT_PARKING_PROTOCOL:
    > + default:
    > + return NULL;
    > + }
    > +}
    > +#else
    > +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
    > +#endif

    Since this is inside a C source file, the inline keyword
    isn't needed since the optimizer will inline regardless.

    With that said, I think it would be cleaner to have this
    as:

    static char * __init acpi_get_cpu_boot_method(void)
    {
    #ifdef CONFIG_ACPI
    return NULL;
    #else
    ...
    #endif
    }

    Or better to make smp_boot_protocol() callable regardless
    of CONFIG_ACPI and then no preprocessor conditionals at all
    would be needed.

    > +
    > /*
    > - * Read a cpu's enable method from the device tree and record it in cpu_ops.
    > + * Read a cpu's enable method and record it in cpu_ops.
    > */
    > int __init cpu_read_ops(struct device_node *dn, int cpu)
    > {
    > - const char *enable_method = of_get_property(dn, "enable-method", NULL);
    > + const char *enable_method;
    > +
    > + if (!acpi_disabled) {
    > + enable_method = acpi_get_cpu_boot_method();
    > + goto get_ops;
    > + }
    > +
    > + enable_method = of_get_property(dn, "enable-method", NULL);
    > if (!enable_method) {
    > /*
    > * The boot CPU may not have an enable method (e.g. when
    > @@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
    > return -ENOENT;
    > }
    >
    > +get_ops:
    > cpu_ops[cpu] = cpu_get_ops(enable_method);
    > if (!cpu_ops[cpu]) {
    > - pr_warn("%s: unsupported enable-method property: %s\n",
    > - dn->full_name, enable_method);
    > + if (acpi_disabled) {
    > + pr_warn("%s: unsupported enable-method property: %s\n",
    > + dn->full_name, enable_method);
    > + } else {
    > + pr_warn("CPU %d: boot protocol unsupported or unknown\n",
    > + cpu);
    > + }
    > +

    Can't we have this more integrated, maybe something like this?

    enable_method = acpi_disabled ? of_get_property(dn, "enable-method", NULL)
    : acpi_get_cpu_boot_method();
    message = acpi_disabled ? dn->full_name : "";

    ...

    pr_warn("CPU %d: %s unsupported enable-method property: %s\n",
    cpu, message, enable_method)

    -Geoff



    \
     
     \ /
      Last update: 2014-08-18 21:41    [W:4.152 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site