| Message ID | 1456148818-26257-2-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 22/02/16 14:46, Aleksey Makarov wrote: > From: Leif Lindholm <leif.lindholm@linaro.org> > > In order to support selecting earlycon via either ACPI or DT, move > the decision on whether to attempt ACPI configuration into the > early_param handling. Then make acpi_boot_table_init() bail out if > acpi_disabled. > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > arch/arm64/kernel/acpi.c | 54 ++++++++++++++++++++++++++---------------------- > 1 file changed, 29 insertions(+), 25 deletions(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index d1ce8e2..7a944f7 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -44,6 +44,19 @@ EXPORT_SYMBOL(acpi_pci_disabled); > static bool param_acpi_off __initdata; > static bool param_acpi_force __initdata; > > +static int __init dt_scan_depth1_nodes(unsigned long node, > + const char *uname, int depth, > + void *data) > +{ > + /* > + * Return 1 as soon as we encounter a node at depth 1 that is > + * not the /chosen node. > + */ > + if (depth == 1 && (strcmp(uname, "chosen") != 0)) > + return 1; > + return 0; > +} > + > static int __init parse_acpi(char *arg) > { > if (!arg) > @@ -57,23 +70,27 @@ static int __init parse_acpi(char *arg) > else > return -EINVAL; /* Core will print when we return error */ > > - return 0; > -} > -early_param("acpi", parse_acpi); > + /* > + * Enable ACPI instead of device tree unless > + * - ACPI has been disabled explicitly (acpi=off), or > + * - the device tree is not empty (it has more than just a /chosen node) > + * and ACPI has not been force enabled (acpi=force) > + */ > + if (param_acpi_off || > + (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) > + return 0; > > -static int __init dt_scan_depth1_nodes(unsigned long node, > - const char *uname, int depth, > - void *data) > -{ > /* > - * Return 1 as soon as we encounter a node at depth 1 that is > - * not the /chosen node. > + * ACPI is disabled at this point. Enable it in order to parse > + * the ACPI tables and carry out sanity checks > */ > - if (depth == 1 && (strcmp(uname, "chosen") != 0)) > - return 1; > + enable_acpi(); > + So we only enable ACPI if we pass acpi=force as kernel parameter? I'm not sure if this is what you wanted to do. Regards, Matthias -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 22, 2016 at 04:45:17PM +0100, Matthias Brugger wrote: > > > On 22/02/16 14:46, Aleksey Makarov wrote: > >From: Leif Lindholm <leif.lindholm@linaro.org> > > > >In order to support selecting earlycon via either ACPI or DT, move > >the decision on whether to attempt ACPI configuration into the > >early_param handling. Then make acpi_boot_table_init() bail out if > >acpi_disabled. > > > >Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > >--- > > arch/arm64/kernel/acpi.c | 54 ++++++++++++++++++++++++++---------------------- > > 1 file changed, 29 insertions(+), 25 deletions(-) > > > >diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > >index d1ce8e2..7a944f7 100644 > >--- a/arch/arm64/kernel/acpi.c > >+++ b/arch/arm64/kernel/acpi.c > >@@ -44,6 +44,19 @@ EXPORT_SYMBOL(acpi_pci_disabled); > > static bool param_acpi_off __initdata; > > static bool param_acpi_force __initdata; > > > >+static int __init dt_scan_depth1_nodes(unsigned long node, > >+ const char *uname, int depth, > >+ void *data) > >+{ > >+ /* > >+ * Return 1 as soon as we encounter a node at depth 1 that is > >+ * not the /chosen node. > >+ */ > >+ if (depth == 1 && (strcmp(uname, "chosen") != 0)) > >+ return 1; > >+ return 0; > >+} > >+ > > static int __init parse_acpi(char *arg) > > { > > if (!arg) > >@@ -57,23 +70,27 @@ static int __init parse_acpi(char *arg) > > else > > return -EINVAL; /* Core will print when we return error */ > > > >- return 0; > >-} > >-early_param("acpi", parse_acpi); > >+ /* > >+ * Enable ACPI instead of device tree unless > >+ * - ACPI has been disabled explicitly (acpi=off), or > >+ * - the device tree is not empty (it has more than just a /chosen node) > >+ * and ACPI has not been force enabled (acpi=force) > >+ */ > >+ if (param_acpi_off || > >+ (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) > >+ return 0; > > > >-static int __init dt_scan_depth1_nodes(unsigned long node, > >- const char *uname, int depth, > >- void *data) > >-{ > > /* > >- * Return 1 as soon as we encounter a node at depth 1 that is > >- * not the /chosen node. > >+ * ACPI is disabled at this point. Enable it in order to parse > >+ * the ACPI tables and carry out sanity checks > > */ > >- if (depth == 1 && (strcmp(uname, "chosen") != 0)) > >- return 1; > >+ enable_acpi(); > >+ > > So we only enable ACPI if we pass acpi=force as kernel parameter? > I'm not sure if this is what you wanted to do. > The current preference from ARM64 maintainers was that is both ACPI tables and a DT were presented then DT should take precedence. With no DT provided the code should use ACPI. Graeme -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/02/16 14:57, Graeme Gregory wrote: > On Mon, Feb 22, 2016 at 04:45:17PM +0100, Matthias Brugger wrote: >> >> >> On 22/02/16 14:46, Aleksey Makarov wrote: >>> From: Leif Lindholm <leif.lindholm@linaro.org> >>> >>> In order to support selecting earlycon via either ACPI or DT, move >>> the decision on whether to attempt ACPI configuration into the >>> early_param handling. Then make acpi_boot_table_init() bail out if >>> acpi_disabled. >>> >>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> >>> --- >>> arch/arm64/kernel/acpi.c | 54 ++++++++++++++++++++++++++---------------------- >>> 1 file changed, 29 insertions(+), 25 deletions(-) >>> >>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>> index d1ce8e2..7a944f7 100644 >>> --- a/arch/arm64/kernel/acpi.c >>> +++ b/arch/arm64/kernel/acpi.c >>> @@ -44,6 +44,19 @@ EXPORT_SYMBOL(acpi_pci_disabled); >>> static bool param_acpi_off __initdata; >>> static bool param_acpi_force __initdata; >>> >>> +static int __init dt_scan_depth1_nodes(unsigned long node, >>> + const char *uname, int depth, >>> + void *data) >>> +{ >>> + /* >>> + * Return 1 as soon as we encounter a node at depth 1 that is >>> + * not the /chosen node. >>> + */ >>> + if (depth == 1 && (strcmp(uname, "chosen") != 0)) >>> + return 1; >>> + return 0; >>> +} >>> + >>> static int __init parse_acpi(char *arg) >>> { >>> if (!arg) >>> @@ -57,23 +70,27 @@ static int __init parse_acpi(char *arg) >>> else >>> return -EINVAL; /* Core will print when we return error */ If argument of parse_acpi is neither "off" nor "force" we return with -EINVAL here. Actually parse_acpi will be only called if we pass "acpi=" as kernel parameter. Therefor we can get rid of "acpi=off" as this is the _new_ standard. IMHO we should introduce "acpi=on" if we really want to change the standard behavior. >>> >>> - return 0; >>> -} >>> -early_param("acpi", parse_acpi); >>> + /* >>> + * Enable ACPI instead of device tree unless >>> + * - ACPI has been disabled explicitly (acpi=off), or >>> + * - the device tree is not empty (it has more than just a /chosen node) >>> + * and ACPI has not been force enabled (acpi=force) >>> + */ >>> + if (param_acpi_off || >>> + (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) >>> + return 0; Or param_acpi_off is true or param_acpi_force is true, the depth of the DT has no influence. >>> >>> -static int __init dt_scan_depth1_nodes(unsigned long node, >>> - const char *uname, int depth, >>> - void *data) >>> -{ >>> /* >>> - * Return 1 as soon as we encounter a node at depth 1 that is >>> - * not the /chosen node. >>> + * ACPI is disabled at this point. Enable it in order to parse >>> + * the ACPI tables and carry out sanity checks >>> */ >>> - if (depth == 1 && (strcmp(uname, "chosen") != 0)) >>> - return 1; >>> + enable_acpi(); >>> + >> >> So we only enable ACPI if we pass acpi=force as kernel parameter? >> I'm not sure if this is what you wanted to do. >> > > The current preference from ARM64 maintainers was that is both ACPI > tables and a DT were presented then DT should take precedence. > > With no DT provided the code should use ACPI. From my understanding in this patch that can never happen. On which version is this set based on? I'm looking on v4.5-rc5 ATM. Regards, Matthias -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matthias, Thank you for review. The bug is fixed in the next version of the patchset. Aleksey Makarov On 02/23/2016 05:37 PM, Matthias Brugger wrote: > > > On 23/02/16 14:57, Graeme Gregory wrote: >> On Mon, Feb 22, 2016 at 04:45:17PM +0100, Matthias Brugger wrote: >>> >>> >>> On 22/02/16 14:46, Aleksey Makarov wrote: >>>> From: Leif Lindholm <leif.lindholm@linaro.org> >>>> >>>> In order to support selecting earlycon via either ACPI or DT, move >>>> the decision on whether to attempt ACPI configuration into the >>>> early_param handling. Then make acpi_boot_table_init() bail out if >>>> acpi_disabled. >>>> >>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> >>>> --- >>>> arch/arm64/kernel/acpi.c | 54 ++++++++++++++++++++++++++---------------------- >>>> 1 file changed, 29 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>> index d1ce8e2..7a944f7 100644 >>>> --- a/arch/arm64/kernel/acpi.c >>>> +++ b/arch/arm64/kernel/acpi.c >>>> @@ -44,6 +44,19 @@ EXPORT_SYMBOL(acpi_pci_disabled); >>>> static bool param_acpi_off __initdata; >>>> static bool param_acpi_force __initdata; >>>> >>>> +static int __init dt_scan_depth1_nodes(unsigned long node, >>>> + const char *uname, int depth, >>>> + void *data) >>>> +{ >>>> + /* >>>> + * Return 1 as soon as we encounter a node at depth 1 that is >>>> + * not the /chosen node. >>>> + */ >>>> + if (depth == 1 && (strcmp(uname, "chosen") != 0)) >>>> + return 1; >>>> + return 0; >>>> +} >>>> + >>>> static int __init parse_acpi(char *arg) >>>> { >>>> if (!arg) >>>> @@ -57,23 +70,27 @@ static int __init parse_acpi(char *arg) >>>> else >>>> return -EINVAL; /* Core will print when we return error */ > > If argument of parse_acpi is neither "off" nor "force" we return with -EINVAL here. Actually parse_acpi will be only called if we pass "acpi=" as kernel parameter. Therefor we can get rid of "acpi=off" as this is the _new_ standard. IMHO we should introduce "acpi=on" if we really want to change the standard behavior. > >>>> >>>> - return 0; >>>> -} >>>> -early_param("acpi", parse_acpi); >>>> + /* >>>> + * Enable ACPI instead of device tree unless >>>> + * - ACPI has been disabled explicitly (acpi=off), or >>>> + * - the device tree is not empty (it has more than just a /chosen node) >>>> + * and ACPI has not been force enabled (acpi=force) >>>> + */ >>>> + if (param_acpi_off || >>>> + (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) >>>> + return 0; > > Or param_acpi_off is true or param_acpi_force is true, the depth of the DT has no influence. > >>>> >>>> -static int __init dt_scan_depth1_nodes(unsigned long node, >>>> - const char *uname, int depth, >>>> - void *data) >>>> -{ >>>> /* >>>> - * Return 1 as soon as we encounter a node at depth 1 that is >>>> - * not the /chosen node. >>>> + * ACPI is disabled at this point. Enable it in order to parse >>>> + * the ACPI tables and carry out sanity checks >>>> */ >>>> - if (depth == 1 && (strcmp(uname, "chosen") != 0)) >>>> - return 1; >>>> + enable_acpi(); >>>> + >>> >>> So we only enable ACPI if we pass acpi=force as kernel parameter? >>> I'm not sure if this is what you wanted to do. >>> >> >> The current preference from ARM64 maintainers was that is both ACPI >> tables and a DT were presented then DT should take precedence. >> >> With no DT provided the code should use ACPI. > > From my understanding in this patch that can never happen. > > On which version is this set based on? > I'm looking on v4.5-rc5 ATM. > > Regards, > Matthias > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index d1ce8e2..7a944f7 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -44,6 +44,19 @@ EXPORT_SYMBOL(acpi_pci_disabled); static bool param_acpi_off __initdata; static bool param_acpi_force __initdata; +static int __init dt_scan_depth1_nodes(unsigned long node, + const char *uname, int depth, + void *data) +{ + /* + * Return 1 as soon as we encounter a node at depth 1 that is + * not the /chosen node. + */ + if (depth == 1 && (strcmp(uname, "chosen") != 0)) + return 1; + return 0; +} + static int __init parse_acpi(char *arg) { if (!arg) @@ -57,23 +70,27 @@ static int __init parse_acpi(char *arg) else return -EINVAL; /* Core will print when we return error */ - return 0; -} -early_param("acpi", parse_acpi); + /* + * Enable ACPI instead of device tree unless + * - ACPI has been disabled explicitly (acpi=off), or + * - the device tree is not empty (it has more than just a /chosen node) + * and ACPI has not been force enabled (acpi=force) + */ + if (param_acpi_off || + (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) + return 0; -static int __init dt_scan_depth1_nodes(unsigned long node, - const char *uname, int depth, - void *data) -{ /* - * Return 1 as soon as we encounter a node at depth 1 that is - * not the /chosen node. + * ACPI is disabled at this point. Enable it in order to parse + * the ACPI tables and carry out sanity checks */ - if (depth == 1 && (strcmp(uname, "chosen") != 0)) - return 1; + enable_acpi(); + return 0; } +early_param("acpi", parse_acpi); + /* * __acpi_map_table() will be called before page_init(), so early_ioremap() * or early_memremap() should be called here to for ACPI table mapping. @@ -181,23 +198,10 @@ out: */ void __init acpi_boot_table_init(void) { - /* - * Enable ACPI instead of device tree unless - * - ACPI has been disabled explicitly (acpi=off), or - * - the device tree is not empty (it has more than just a /chosen node) - * and ACPI has not been force enabled (acpi=force) - */ - if (param_acpi_off || - (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) + if (acpi_disabled) return; /* - * ACPI is disabled at this point. Enable it in order to parse - * the ACPI tables and carry out sanity checks - */ - enable_acpi(); - - /* * If ACPI tables are initialized and FADT sanity checks passed, * leave ACPI enabled and carry on booting; otherwise disable ACPI * on initialization error.