Re: [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters
On Thu, Sep 10, 2015 at 02:52:25PM +0100, Stefano Stabellini wrote: > > > In any case this should be separate from the shim ABI discussion. > > > > I disagree; I think this is very much relevant to the ABI discussion. > > That's not to say that I insist on a particular approach, but I think > > that they need to be considered together. > > Let's suppose Xen didn't expose any RuntimeServices at all, would that > make it easier to discuss about the EFI stub parameters? Possibly :) > In the grant > scheme of things, they are not that important, as Ian wrote what is > important is how to pass the RSDP. So, we have discussed in the past having the ability to get at configuration tables when UEFI is not available. Say, for example, that we wanted SMBIOS support on a platform with U-Boot firmware. Since all that is needed then is a UEFI System Table with a pointer to a configuration table array, this should be fairly straightforward to implement statically. The other parameters would not be necessary. It would however require minor changes to the arm64 kernel UEFI support. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-acpi] [RFC 3/5] acpi/serial: add DBG2 earlycon support
On Tue, Sep 08, 2015 at 12:38:59PM -0400, Mark Salter wrote:
> On Tue, 2015-09-08 at 13:43 +0100, Leif Lindholm wrote:
> > The ACPI DBG2 table defines a debug console. Add support for parsing it
> > and using it to select earlycon destination when no arguments provided.
> >
> > Signed-off-by: Leif Lindholm
> > ---
> > arch/arm64/kernel/acpi.c | 2 +
> > drivers/acpi/Makefile | 1 +
> > drivers/acpi/console.c| 103
> > ++
> > drivers/of/fdt.c | 2 +-
> > drivers/tty/serial/earlycon.c | 16 ---
> > include/linux/acpi.h | 4 ++
> > include/linux/serial_core.h | 9 ++--
> > 7 files changed, 126 insertions(+), 11 deletions(-)
> > create mode 100644 drivers/acpi/console.c
> >
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index b9a5623..be7600a 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -207,6 +207,8 @@ void __init acpi_boot_table_init(void)
> > if (!param_acpi_force)
> > disable_acpi();
> > }
> > +
> > + acpi_early_console_probe();
> > }
> >
> > void __init acpi_gic_init(void)
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index b5e7cd8..a89587d 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -10,6 +10,7 @@ ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT
> > #
> > obj-y += tables.o
> > obj-$(CONFIG_X86) += blacklist.o
> > +obj-y += console.o
>
> obj-$(CONFIG_SERIAL_EARLYCON) += console.o
>
> to eliminate whole-file #ifdef
Yes, that makes more sense for this patch standalone, but I felt it
would be a bit weird to add the conditionality here only to delete it
in the subsequent patch. I don't feel strongly about it.
> >
> > #
> > # ACPI Core Subsystem (Interpreter)
> > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c
> > new file mode 100644
> > index 000..a985890
> > --- /dev/null
> > +++ b/drivers/acpi/console.c
> > @@ -0,0 +1,103 @@
> > +/*
> > + * Copyright (c) 2012, Intel Corporation
> > + * Copyright (c) 2015, Linaro Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#define DEBUG
> > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x))
> > +
> > +#ifdef CONFIG_SERIAL_EARLYCON
> > +static int use_earlycon __initdata;
> > +static int __init setup_acpi_earlycon(char *buf)
> > +{
> > + if (!buf)
> > + use_earlycon = 1;
> > +
> > + return 0;
> > +}
> > +early_param("earlycon", setup_acpi_earlycon);
> > +
> > +extern struct earlycon_id __earlycon_table[];
> > +
> > +static __initdata struct {
> > + int id;
> > + const char *name;
> > +} subtypes[] = {
> > + {0, "uart8250"},
> > + {1, "uart8250"},
> > + {2, NULL},
> > + {3, "pl011"},
> > +};
>
> Instead of having a table here, why not have an ACPI_EARLYCON_DECLARE()
> where individual drivers can provide an id similar to OF_EARLYCON_DECLARE()
> providing compatible strings?
The IDs are defined by the DBG2 specification, so it felt more
natural to encapsulate it here. However, a comment to that effect
would be useful. Or would you still prefer
ACPI_EARLYCON_DECLARE(0, uart8250)
ACPI_EARLYCON_DECLARE(1, uart8250)
...
?
> > +
> > +static int __init acpi_setup_earlycon(unsigned long addr, const char
> > *driver)
> > +{
> > + const struct earlycon_id *match;
> > +
> > + for (match = __earlycon_table; match->name[0]; match++)
> > + if (strcmp(driver, match->name) == 0)
> > + return setup_earlycon_driver(addr, match->setup);
> > +
> > + return -ENODEV;
> > +}
> > +
> > +static int __init acpi_parse_dbg2(struct acpi_table_header *table)
> > +{
> > + struct acpi_table_dbg2 *dbg2;
> > + struct acpi_dbg2_device *entry;
> > + void *tbl_end;
> > +
> > + dbg2 = (struct acpi_table_dbg2 *)table;
> &g
Re: [RFC 3/5] acpi/serial: add DBG2 earlycon support
On Tue, Sep 08, 2015 at 02:09:51PM +0100, Mark Rutland wrote:
> On Tue, Sep 08, 2015 at 01:43:35PM +0100, Leif Lindholm wrote:
> > The ACPI DBG2 table defines a debug console. Add support for parsing it
> > and using it to select earlycon destination when no arguments provided.
> >
> > Signed-off-by: Leif Lindholm
>
> [...]
>
> > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c
> > new file mode 100644
> > index 000..a985890
> > --- /dev/null
> > +++ b/drivers/acpi/console.c
> > @@ -0,0 +1,103 @@
> > +/*
> > + * Copyright (c) 2012, Intel Corporation
> > + * Copyright (c) 2015, Linaro Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#define DEBUG
>
> Why?
Kept around from Lv Zheng's 2012 set. Will drop.
> > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x))
>
> Use ARRAY_SIZE (from kernel.h).
I was sure there was something like that, but my grep-fu failed me.
Will fix.
> > +
> > +#ifdef CONFIG_SERIAL_EARLYCON
> > +static int use_earlycon __initdata;
> > +static int __init setup_acpi_earlycon(char *buf)
> > +{
> > + if (!buf)
> > + use_earlycon = 1;
> > +
> > + return 0;
> > +}
> > +early_param("earlycon", setup_acpi_earlycon);
>
> It seems a shame to add this after folding the OF case into the earlycon
> code. What necessitates this being a separate early_param? Why is it too
> early to parse DBG2?
Currently, we don't even know where our ACPI tables are at this point
(efi_init() is called two functions after parse_early_param() in
setup_arch). More specifically, because acpi_boot_table_init() is
called even later than that.
If we moved both of those earlier, we could drop the extra earlycon
param handling for ACPI. That would of course reduce the ability to
have dynamically configurable debug messages for both of these.
> [...]
>
> > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> > index 2bda09a..c063cbb 100644
> > --- a/drivers/tty/serial/earlycon.c
> > +++ b/drivers/tty/serial/earlycon.c
> > @@ -13,6 +13,7 @@
> >
> > #define pr_fmt(fmt)KBUILD_MODNAME ": " fmt
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -184,12 +185,16 @@ static int __init param_setup_earlycon(char *buf)
> > int err;
> >
> > /*
> > -* Just 'earlycon' is a valid param for devicetree earlycons;
> > -* don't generate a warning from parse_early_params() in that case
> > +* Just 'earlycon' is a valid param for devicetree or ACPI earlycons;
> > +* ACPI cannot be parsed yet, so return without action if enabled.
> > +* Otherwise, attempt initialization using DT.
> > */
> > - if (!buf || !buf[0])
> > - if (IS_ENABLED(CONFIG_OF_FLATTREE))
> > + if (!buf || !buf[0]) {
> > + if (!acpi_disabled)
> > + return 0;
> > + else if (IS_ENABLED(CONFIG_OF_FLATTREE))
> > return early_init_dt_scan_chosen_serial();
> > + }
>
> It would be much nicer if we could handle the ACPI earlycon parsing in
> the same place. As above, I assume I'm missing something that prevents
> that.
I don't disagree.
/
Leif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] HACK: serial: move pl011 initcall to device_initcall
On Tue, Sep 08, 2015 at 01:56:08PM +0100, Mark Rutland wrote:
> On Tue, Sep 08, 2015 at 01:43:37PM +0100, Leif Lindholm wrote:
> > ---
> > drivers/tty/serial/amba-pl011.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/amba-pl011.c
> > b/drivers/tty/serial/amba-pl011.c
> > index 452dbba..31cf985 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -2806,7 +2806,7 @@ static void __exit pl011_exit(void)
> > * While this can be a module, if builtin it's most likely the console
> > * So let's leave module_exit but move module_init to an earlier place
> > */
> > -arch_initcall(pl011_init);
> > +device_initcall(pl011_init);
> > module_exit(pl011_exit);
>
> What's the ordering constraint that you're trying to solve with this?
Basically that _CRS is not available until the ACPI subsystem is up -
so when uart_add_one_port() calls acpi_console_check(), that function
looks for a match against variables that have not been set yet.
> The cover didn't mention anything, and there's no commit message.
That's because I really don't want it committed :)
> Mark.
>
> >
> > MODULE_AUTHOR("ARM Ltd/Deep Blue Solutions Ltd");
> > --
> > 2.1.4
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/5] of/serial: move earlycon early_param handling to serial
On Tue, Sep 08, 2015 at 01:52:45PM +0100, Mark Rutland wrote:
> On Tue, Sep 08, 2015 at 01:43:34PM +0100, Leif Lindholm wrote:
> > We have multiple "earlycon" early_param handlers - merge the DT one into
> > the main earlycon one. This means the earlycon early_param handler does
> > not just return success if no options are specified.
> >
> > Signed-off-by: Leif Lindholm
> > ---
> > drivers/of/fdt.c | 11 +--
> > drivers/tty/serial/earlycon.c | 4 +++-
> > include/linux/of_fdt.h| 1 +
> > 3 files changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 6e82bc42..fcfc4c7 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -793,7 +793,7 @@ static inline void
> > early_init_dt_check_for_initrd(unsigned long node)
> > #ifdef CONFIG_SERIAL_EARLYCON
> > extern struct of_device_id __earlycon_of_table[];
> >
> > -static int __init early_init_dt_scan_chosen_serial(void)
> > +int __init early_init_dt_scan_chosen_serial(void)
> > {
> > int offset;
> > const char *p;
> > @@ -834,15 +834,6 @@ static int __init
> > early_init_dt_scan_chosen_serial(void)
> > }
> > return -ENODEV;
> > }
> > -
> > -static int __init setup_of_earlycon(char *buf)
> > -{
> > - if (buf)
> > - return 0;
> > -
> > - return early_init_dt_scan_chosen_serial();
> > -}
> > -early_param("earlycon", setup_of_earlycon);
> > #endif
> >
> > /**
> > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> > index f096360..2bda09a 100644
> > --- a/drivers/tty/serial/earlycon.c
> > +++ b/drivers/tty/serial/earlycon.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -187,7 +188,8 @@ static int __init param_setup_earlycon(char *buf)
> > * don't generate a warning from parse_early_params() in that case
> > */
> > if (!buf || !buf[0])
> > - return 0;
> > + if (IS_ENABLED(CONFIG_OF_FLATTREE))
> > + return early_init_dt_scan_chosen_serial();
> >
> > err = setup_earlycon(buf);
> > if (err == -ENOENT || err == -EALREADY)
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index df9ef38..772c47c 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -63,6 +63,7 @@ extern int early_init_dt_scan_chosen(unsigned long node,
> > const char *uname,
> > int depth, void *data);
> > extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
> > int depth, void *data);
> > +extern int early_init_dt_scan_chosen_serial(void);
>
> I think you need a static inline stub to prevent link errors in
> !CONFIG_OF_FLATTREE kernels, as we do for
> early_init_fdt_scan_reserved_mem and friends.
Will add, thanks.
>
> Mark.
>
> > extern void early_init_fdt_scan_reserved_mem(void);
> > extern void early_init_fdt_reserve_self(void);
> > extern void early_init_dt_add_memory_arch(u64 base, u64 size);
> > --
> > 2.1.4
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 1/5] arm64: move acpi/dt decision earlier in boot process
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
---
arch/arm64/kernel/acpi.c | 53 +---
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 19de753..b9a5623 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -39,6 +39,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)
@@ -52,22 +65,25 @@ 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()
@@ -176,23 +192,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.
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 2/5] of/serial: move earlycon early_param handling to serial
We have multiple "earlycon" early_param handlers - merge the DT one into
the main earlycon one. This means the earlycon early_param handler does
not just return success if no options are specified.
Signed-off-by: Leif Lindholm
---
drivers/of/fdt.c | 11 +--
drivers/tty/serial/earlycon.c | 4 +++-
include/linux/of_fdt.h| 1 +
3 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6e82bc42..fcfc4c7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -793,7 +793,7 @@ static inline void early_init_dt_check_for_initrd(unsigned
long node)
#ifdef CONFIG_SERIAL_EARLYCON
extern struct of_device_id __earlycon_of_table[];
-static int __init early_init_dt_scan_chosen_serial(void)
+int __init early_init_dt_scan_chosen_serial(void)
{
int offset;
const char *p;
@@ -834,15 +834,6 @@ static int __init early_init_dt_scan_chosen_serial(void)
}
return -ENODEV;
}
-
-static int __init setup_of_earlycon(char *buf)
-{
- if (buf)
- return 0;
-
- return early_init_dt_scan_chosen_serial();
-}
-early_param("earlycon", setup_of_earlycon);
#endif
/**
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index f096360..2bda09a 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -187,7 +188,8 @@ static int __init param_setup_earlycon(char *buf)
* don't generate a warning from parse_early_params() in that case
*/
if (!buf || !buf[0])
- return 0;
+ if (IS_ENABLED(CONFIG_OF_FLATTREE))
+ return early_init_dt_scan_chosen_serial();
err = setup_earlycon(buf);
if (err == -ENOENT || err == -EALREADY)
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index df9ef38..772c47c 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -63,6 +63,7 @@ extern int early_init_dt_scan_chosen(unsigned long node,
const char *uname,
int depth, void *data);
extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
int depth, void *data);
+extern int early_init_dt_scan_chosen_serial(void);
extern void early_init_fdt_scan_reserved_mem(void);
extern void early_init_fdt_reserve_self(void);
extern void early_init_dt_add_memory_arch(u64 base, u64 size);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 4/5] tty/console: use SPCR table to define console
From: Torez Smith
If console= is not added to the kernel command line, the console
is not registered until much further into the booting process. This patch
adds support to parse the SPCR ACPI table to pull console support out,
then use the appropriate drivers to set up console support earlier in the
boot process.
Signed-off-by: Jon Masters
[rebased and cleaned up]
Signed-off-by: Torez Smith
[reworked to use _CRS, moved to drivers/acpi]
Signed-off-by: Leif Lindholm
---
drivers/acpi/console.c | 157 +++
drivers/tty/serial/serial_core.c | 14 +++-
include/linux/acpi.h | 11 ++-
3 files changed, 179 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c
index a985890..02883a1 100644
--- a/drivers/acpi/console.c
+++ b/drivers/acpi/console.c
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2012, Intel Corporation
+ * Copyright (c) 2015, Red Hat, Inc.
* Copyright (c) 2015, Linaro Ltd.
*
* This program is free software; you can redistribute it and/or modify
@@ -12,11 +13,17 @@
#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt
#include
+#include
#include
#include
+#include
#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x))
+static u64 acpi_serial_addr;
+static struct acpi_device *acpi_serial_device;
+static char *acpi_serial_options;
+
#ifdef CONFIG_SERIAL_EARLYCON
static int use_earlycon __initdata;
static int __init setup_acpi_earlycon(char *buf)
@@ -101,3 +108,153 @@ int __init acpi_early_console_probe(void)
return 0;
}
#endif /* CONFIG_SERIAL_EARLYCON */
+
+/*
+ * Parse the SPCR table. If we are not working with version 2 or
+ * higher, bail.
+ * Otherwise, pull out the baud rate and address to the console device.
+ */
+static int __init acpi_parse_spcr(struct acpi_table_header *table)
+{
+ struct acpi_table_spcr *spcr = (struct acpi_table_spcr *)table;
+
+ if (table->revision < 2)
+ return -EOPNOTSUPP;
+
+ /* Handle possible alignment issues */
+ memcpy(&acpi_serial_addr,
+ &spcr->serial_port.address, sizeof(acpi_serial_addr));
+
+ /*
+* The baud rate the BIOS used for redirection. Valid values are
+* 3 = 9600
+* 4 = 19200
+* 6 = 57600
+* 7 = 115200
+* 0-2, 5, 8 - 255 = reserved
+ */
+ switch (spcr->baud_rate) {
+ case 3:
+ acpi_serial_options = "9600";
+ break;
+ case 4:
+ acpi_serial_options = "19200";
+ break;
+ case 6:
+ acpi_serial_options = "57600";
+ break;
+ case 7:
+ acpi_serial_options = "115200";
+ break;
+ default:
+ acpi_serial_options = "";
+ break;
+ }
+
+ pr_info("SPCR serial device: 0x%llx (options: %s)\n",
+ acpi_serial_addr, acpi_serial_options);
+
+ return 0;
+}
+
+/*
+ * Parse an ACPI "Device" to determine if it represents the
+ * data found in the SPCR table. If the associated Device has
+ * and Address entry, and, that Address matches the one found
+ * in our SPCR table, it's the entry we are interested in.
+ *
+ */
+static acpi_status acpi_spcr_device_scan(acpi_handle handle,
+u32 level, void *context, void **retv)
+{
+ unsigned long long addr = 0;
+ struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ acpi_status status = AE_OK;
+ struct acpi_device *adev;
+ struct list_head resource_list;
+ struct resource_entry *rentry;
+
+ status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &name_buffer);
+ if (ACPI_FAILURE(status))
+ return status;
+
+ adev = acpi_bus_get_acpi_device(handle);
+ if (!adev) {
+ pr_err("Err locating SPCR device from ACPI handle\n");
+ return AE_OK; /* skip this one */
+ }
+
+ /*
+* Read device address from _CRS.
+*/
+ INIT_LIST_HEAD(&resource_list);
+ if (acpi_dev_get_resources(adev, &resource_list, NULL, NULL) <= 0)
+ return AE_OK;
+
+ list_for_each_entry(rentry, &resource_list, node) {
+ if (resource_type(rentry->res) == IORESOURCE_MEM)
+ addr = rentry->res->start;
+ }
+ acpi_dev_free_resource_list(&resource_list);
+
+ if (addr == acpi_serial_addr) {
+ acpi_serial_device = adev;
+
+ pr_info("SPCR serial console: %s (0x%llx)\n",
+ (char *)(name_buffer.pointer), addr);
+
+ return AE_OK; /* harmless to continue */
+ }
+
+ /* continue */
+ return AE_OK; /* continue */
+}
+
+static int __ini
[RFC 5/5] HACK: serial: move pl011 initcall to device_initcall
---
drivers/tty/serial/amba-pl011.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 452dbba..31cf985 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2806,7 +2806,7 @@ static void __exit pl011_exit(void)
* While this can be a module, if builtin it's most likely the console
* So let's leave module_exit but move module_init to an earlier place
*/
-arch_initcall(pl011_init);
+device_initcall(pl011_init);
module_exit(pl011_exit);
MODULE_AUTHOR("ARM Ltd/Deep Blue Solutions Ltd");
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 3/5] acpi/serial: add DBG2 earlycon support
The ACPI DBG2 table defines a debug console. Add support for parsing it
and using it to select earlycon destination when no arguments provided.
Signed-off-by: Leif Lindholm
---
arch/arm64/kernel/acpi.c | 2 +
drivers/acpi/Makefile | 1 +
drivers/acpi/console.c| 103 ++
drivers/of/fdt.c | 2 +-
drivers/tty/serial/earlycon.c | 16 ---
include/linux/acpi.h | 4 ++
include/linux/serial_core.h | 9 ++--
7 files changed, 126 insertions(+), 11 deletions(-)
create mode 100644 drivers/acpi/console.c
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index b9a5623..be7600a 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -207,6 +207,8 @@ void __init acpi_boot_table_init(void)
if (!param_acpi_force)
disable_acpi();
}
+
+ acpi_early_console_probe();
}
void __init acpi_gic_init(void)
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b5e7cd8..a89587d 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -10,6 +10,7 @@ ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT
#
obj-y += tables.o
obj-$(CONFIG_X86) += blacklist.o
+obj-y += console.o
#
# ACPI Core Subsystem (Interpreter)
diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c
new file mode 100644
index 000..a985890
--- /dev/null
+++ b/drivers/acpi/console.c
@@ -0,0 +1,103 @@
+/*
+ * Copyright (c) 2012, Intel Corporation
+ * Copyright (c) 2015, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#define DEBUG
+#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+
+#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x))
+
+#ifdef CONFIG_SERIAL_EARLYCON
+static int use_earlycon __initdata;
+static int __init setup_acpi_earlycon(char *buf)
+{
+ if (!buf)
+ use_earlycon = 1;
+
+ return 0;
+}
+early_param("earlycon", setup_acpi_earlycon);
+
+extern struct earlycon_id __earlycon_table[];
+
+static __initdata struct {
+ int id;
+ const char *name;
+} subtypes[] = {
+ {0, "uart8250"},
+ {1, "uart8250"},
+ {2, NULL},
+ {3, "pl011"},
+};
+
+static int __init acpi_setup_earlycon(unsigned long addr, const char *driver)
+{
+ const struct earlycon_id *match;
+
+ for (match = __earlycon_table; match->name[0]; match++)
+ if (strcmp(driver, match->name) == 0)
+ return setup_earlycon_driver(addr, match->setup);
+
+ return -ENODEV;
+}
+
+static int __init acpi_parse_dbg2(struct acpi_table_header *table)
+{
+ struct acpi_table_dbg2 *dbg2;
+ struct acpi_dbg2_device *entry;
+ void *tbl_end;
+
+ dbg2 = (struct acpi_table_dbg2 *)table;
+ if (!dbg2) {
+ pr_debug("DBG2 not present.\n");
+ return -ENODEV;
+ }
+
+ tbl_end = (void *)table + table->length;
+
+ entry = (struct acpi_dbg2_device *)((void *)dbg2 + dbg2->info_offset);
+
+ while (((void *)entry) + sizeof(struct acpi_dbg2_device) < tbl_end) {
+ struct acpi_generic_address *addr;
+
+ if (entry->revision != 0) {
+ pr_debug("DBG2 revision %d not supported.\n",
+entry->revision);
+ return -ENODEV;
+ }
+
+ addr = (void *)entry + entry->base_address_offset;
+
+ pr_debug("DBG2 PROBE - console (%04x:%04x).\n",
+entry->port_type, entry->port_subtype);
+
+ if (use_earlycon &&
+ (entry->port_type == ACPI_DBG2_SERIAL_PORT) &&
+ (entry->port_subtype < NUM_ELEMS(subtypes)))
+ acpi_setup_earlycon(addr->address,
+ subtypes[entry->port_subtype].name);
+
+ entry = (struct acpi_dbg2_device *)
+ ((void *)entry + entry->length);
+ }
+
+ return 0;
+}
+
+int __init acpi_early_console_probe(void)
+{
+ acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2);
+
+ return 0;
+}
+#endif /* CONFIG_SERIAL_EARLYCON */
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index fcfc4c7..a96209f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -829,7 +829,7 @@ int __init early_init_dt_scan_chosen_serial(void)
if (!addr)
return -ENXIO;
- of_setup_earlycon(addr, match->data);
+ setup_earlycon_driver(addr, match->data);
[RFC 0/5] console/acpi: add DBG2 and SPCR console configuration
Support for configuring bootconsole and console via the ACPI tables DBG2 (Debug Port Table 2) [1] and SPCR (Serial Port Console Redirection Table) [2], defined by Microsoft, has been discussed on and off over the years. [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx [2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx Licensing concerns have prevented this happening in the past, but as of 10 August 2015, these tables have both been released also under OWF 1.0 (http://www.openwebfoundation.org/legal/the-owf-1-0-agreements/owfa-1-0) which is think is noncontroversially GPL-compatible? This set is a first attempt at implementing this. Submitting as an RFC since the SPCR handling currently depends on the console driver being initialized after subsystem initcalls. Workaround to enable testing surrounding infrastructure in 5/5, _really_ not intended to be merged. (Suggestions for acceptable ways of working around this appreciated.) For testing the DBG2 stuff with pl011, you would need: - A patch to unbreak pl011 earlycon, like http://permalink.gmane.org/gmane.linux.ports.arm.kernel/433219 - A QEMU that generates DBG2 tables, like current HEAD with the addition of http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg01719.html SPCR support is included in QEMU's ARM mach-virt since 2.4 release. DBG2 support has an Intel copyright notice added to it since my starting point was Lv Zheng's 2012 DBGP/DBG2 set (although not much of the original remains - this is quite a cut-down version). Leif Lindholm (4): arm64: move acpi/dt decision earlier in boot process of/serial: move earlycon early_param handling to serial acpi/serial: add DBG2 earlycon support HACK: serial: move pl011 initcall to device_initcall Torez Smith (1): tty/console: use SPCR table to define console arch/arm64/kernel/acpi.c | 55 + drivers/acpi/Makefile| 1 + drivers/acpi/console.c | 260 +++ drivers/of/fdt.c | 13 +- drivers/tty/serial/amba-pl011.c | 2 +- drivers/tty/serial/earlycon.c| 18 ++- drivers/tty/serial/serial_core.c | 14 ++- include/linux/acpi.h | 13 ++ include/linux/of_fdt.h | 1 + include/linux/serial_core.h | 9 +- 10 files changed, 337 insertions(+), 49 deletions(-) create mode 100644 drivers/acpi/console.c -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node
On Tue, Aug 25, 2015 at 04:51:22PM +0200, Ard Biesheuvel wrote: > >>Arm kernel should either fetch memory information from > >>efi or DT. > > > > Absolutely. > > > >>Currently arm kernel fetch both efi memory information and > >>reserved buffer from DTB at the same time. > > > > No, it does not. > > It should not, but it does. Due to an oversight, the stub removes > /memreserve/ entries but ignores the reserved-memory node completely. Urgh. > This was reported here in fact > > http://thread.gmane.org/gmane.linux.kernel.efi/5736/focus=5742 > > but there has not been a followup to this series. Are all of those patches still relevant, or did some of them go in already? Haojian: can you give that patch a spin and see if it does what you need, combined with adding the reserved areas to the UEFI memory map? / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node
On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote: > Since we discussed a lot on this, let's make a conclusion on it. > > 1. UEFI could append the reserved buffer in it's memory mapping. Yes. It needs to. (I will let Mark comment on points 2-4.) > 5. A patch is necessary in kernel. If efi stub feature is enabled, >arm kernel should not parse memory node or reserved memory buffer in >DT any more. This is already the case. The stub deletes any present memory nodes and reserved entries in drivers/firmware/efi/libstub/fdt.c:update_fdt(). Then, during setup_arch(), arch/arm64/kernel/efi.c:efi_init() calls reserve_regions(), which adds only those memory regions available for use by Linux as RAM to memblock. >Arm kernel should either fetch memory information from >efi or DT. Absolutely. >Currently arm kernel fetch both efi memory information and >reserved buffer from DTB at the same time. No, it does not. Regards, Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node
On Tue, Aug 25, 2015 at 06:15:10PM +0800, Haojian Zhuang wrote: > > > 1. We need support both UEFI and uboot. So the reserved buffer have to > > > be declared in DTB since they are used by kernel driver, not UEFI. > > > > The buffer may need to be declared in DTB also, but it most certanily > > needs to be declared in UEFI. > > > > And for the U-Boot case, since it is not memory available to Linux, it > > should not be declared as "memory". > > Something are messed at here. We have these buffer are used in mailbox. > They should be allocated as non-cacheable. That is a completely different issue, and if that is not currently possible, then we need to fix that. But it needs to be fixed in the right place. > If these buffers are contained in memory memblock in kernel, it means > that they exist in kernel page table with cachable property. When it's > used in mailbox driver with non-cachable property, it'll only cause > cache maintenance issue. So Leo declared these buffers as reserved > in DT with "no-map" property. It's the key. It could avoid the cache > maintenance issue. Yes, when not booting with UEFI. > > > 2. UEFI just loads grub. It's no time to run any other custom EFI > > > application. > > > > Apart from being completely irrelevant, how are you intending to > > validate that GRUB never touches these memory regions? > > GRUB is just a part of bootloader. When linux kernel is running, > who cares GRUB? GRUB's lifetime is already finished. We don't care once Linux is running - we care between UEFI boot services starting and Linux memblock being initialised. > By the way, UEFI code region is at [0x3Dxx_, 0x3DFF_]. Those > mailbox buffer is in [0x05e0_, 0x06f0_]. Then I can make sure > UEFI won't touch the reserved buffer. And if a UEFI application explicitly requests to map an area elsewhere, will your UEFI reject that request? How will it do that without having information in its memory map about areas it must not access? > Even if UEFI touched the reserved > buffer, is it an issue? Definitely it's not. UEFI's lifetime is end > when linux kernel is running at hikey. Even if UEFI runtime service > is enabled, the runtime data area is at [0x38xx_, 0x38xx_]. The runtime data area is currently, in your current image, at [0x38xx_, 0x38xx_]. What happens if a UEFI application registers a configuration table? Or registers a protocol for use at runtime? Areas of memory that are not available for UEFI _must_ be marked as such in the UEFI memory map. Once they are, we can deal with them in the kernel. If this is not currently being done, that is a bug that needs fixing. > > Build a version once, test it, and hope the results remain valid > > forever? And then when you move the regions and the previously working > > GRUB now tramples all over them? Or when something changes in upstream > > GRUB and its memory allocations drifts into the secretly untouchable > > regions? > > As I said above, UEFI won't touch it. And even UEFI touch it, kernel > doesn't care since UEFI's lifetime is end. UEFI's lifetime doesn't end until reset. > > Are you then going to hack GRUB, release a special HiKey version of > > GRUB, not support any other versions, and still can your firmware > > UEFI? > > I don't need to hack GRUB at all. You will if you're running it under a "UEFI" which has areas you can't touch and aren't telling it about that. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node
On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote: > On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote: > > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote: > > > > If your EFI memory map describes the memory as mappable, it is wrong. > > > > > > When kernel is working, kernel will create its own page table based on > > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll > > > be moved to reserved memblock. Why is it wrong? > > > > > > In the second, UEFI is firmware. When it's stable, nobody should change > > > it without any reason. > > > > Much like the memory map. > > > > > These reserved memory are used in mailbox driver. > > > Look. It's driver, so it could be changed at any time. > > > > No, it is a set of regions of memory set aside for use by a different > > master in the system as well as communications with that master. > > > > The fact that there is a driver somewhere that is aware of this is > > entirely beside the point. All agents in the system must adher to this > > protocol. > > > > > Why do you want > > > to UEFI knowing this memory range? Do you hope UEFI to change when > > > mailbox driver is changed? > > > > Yes. > > > > UEFI is a runtime environment. Having random magic areas not to be > > touched will cause random pieces of software running under it to break > > horribly or break other things horribly. > > Unless you mark them as reserved in the UEFI memory map. > > At which point the Linux kernel will automatically ignore them, and > > the proposed patch is redundant. > > > > So, yes, if you want a system that can boot reliably, run testsuites > > (like SCT or FWTS), run applications (like fastboot ... or the EFI > > stub kernel itself), then any memory regions that is reserved for > > mailbox communication (or other masters in the system) _must_ be > > marked in the EFI memory map. > > 1. We need support both UEFI and uboot. So the reserved buffer have to > be declared in DTB since they are used by kernel driver, not UEFI. The buffer may need to be declared in DTB also, but it most certanily needs to be declared in UEFI. And for the U-Boot case, since it is not memory available to Linux, it should not be declared as "memory". > 2. UEFI just loads grub. It's no time to run any other custom EFI > application. Apart from being completely irrelevant, how are you intending to validate that GRUB never touches these memory regions? Build a version once, test it, and hope the results remain valid forever? And then when you move the regions and the previously working GRUB now tramples all over them? Or when something changes in upstream GRUB and its memory allocations drifts into the secretly untouchable regions? Are you then going to hack GRUB, release a special HiKey version of GRUB, not support any other versions, and still can your firmware UEFI? Repeat again and again for any other UEFI applications - including fastboot, SCT, FWTS and the UEFI stub kernel. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node
On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote: > > If your EFI memory map describes the memory as mappable, it is wrong. > > When kernel is working, kernel will create its own page table based on > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll > be moved to reserved memblock. Why is it wrong? > > In the second, UEFI is firmware. When it's stable, nobody should change > it without any reason. Much like the memory map. > These reserved memory are used in mailbox driver. > Look. It's driver, so it could be changed at any time. No, it is a set of regions of memory set aside for use by a different master in the system as well as communications with that master. The fact that there is a driver somewhere that is aware of this is entirely beside the point. All agents in the system must adher to this protocol. > Why do you want > to UEFI knowing this memory range? Do you hope UEFI to change when > mailbox driver is changed? Yes. UEFI is a runtime environment. Having random magic areas not to be touched will cause random pieces of software running under it to break horribly or break other things horribly. Unless you mark them as reserved in the UEFI memory map. At which point the Linux kernel will automatically ignore them, and the proposed patch is redundant. So, yes, if you want a system that can boot reliably, run testsuites (like SCT or FWTS), run applications (like fastboot ... or the EFI stub kernel itself), then any memory regions that is reserved for mailbox communication (or other masters in the system) _must_ be marked in the EFI memory map. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] of: handle both '/' and ':' in path strings
On Tue, Mar 17, 2015 at 12:30:31PM -0700, Brian Norris wrote:
> Commit 106937e8ccdc ("of: fix handling of '/' in options for
> of_find_node_by_path()") caused a regression in OF handling of
> stdout-path. While it fixes some cases which have '/' after the ':', it
> breaks cases where there is more than one '/' *before* the ':'.
>
> For example, it breaks this boot string
>
> stdout-path = "/rdb/serial@f040ab00:115200";
>
> So rather than doing sequentialized checks (first for '/', then for ':';
> or vice versa), to get the correct behavior we need to check for the
> first occurrence of either one of them.
>
> It so happens that the handy strcspn() helper can do just that.
>
> Fixes: 106937e8ccdc ("of: fix handling of '/' in options for
> of_find_node_by_path()")
> Signed-off-by: Brian Norris
> Cc: sta...@vger.kernel.org
> ---
> This is for -stable only because the regression is marked for stable. Not sure
> the first one deserves to go to -stable, actually...
>
> drivers/of/base.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index adb8764861c0..966d6fdcf427 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -715,13 +715,8 @@ static struct device_node *__of_find_node_by_path(struct
> device_node *parent,
> {
> struct device_node *child;
> int len;
> - const char *end;
>
> - end = strchr(path, ':');
> - if (!end)
> - end = strchrnul(path, '/');
> -
> - len = end - path;
> + len = strcspn(path, "/:");
> if (!len)
> return NULL;
>
> --
> 1.9.1
Yeah, that's neater that the fix I sent out earlier today.
Acked-by: Leif Lindholm
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] of: unittest: Add option string test case with longer path
On Tue, Mar 17, 2015 at 12:30:32PM -0700, Brian Norris wrote:
> There were regressions seen with commit 106937e8ccdc ("of: fix handling
> of '/' in options for of_find_node_by_path()"), where we couldn't handle
> extra '/' before the ':'. Let's test for this now.
>
> Confirmed that this test fails without the previous patch and passes
> when patched. All other tests pass.
>
> Signed-off-by: Brian Norris
> ---
> drivers/of/unittest.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index aba8946cac46..52c45c7df07f 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -97,6 +97,11 @@ static void __init of_selftest_find_node_by_name(void)
>"option path test, subcase #1 failed\n");
> of_node_put(np);
>
> + np =
> of_find_node_opts_by_path("/testcase-data/testcase-device1:test/option",
> &options);
> + selftest(np && !strcmp("test/option", options),
> + "option path test, subcase #2 failed\n");
> + of_node_put(np);
> +
> np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
> selftest(np, "NULL option path test failed\n");
> of_node_put(np);
> --
> 1.9.1
Acked-by: Leif Lindholm
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] fix the node option breakage
Commit 106937e8ccdc
("of: fix handling of '/' in options for of_find_node_by_path()")
broke handling of paths with both ':' and '/'.
Revert that commit and replace it with the squash of 106937e8ccdc
and the fix verified by Hans de Goede.
The tests added by Peter Hurley and committed together with
106937e8ccdc are still valid, but selftest will report 2 errors
until the new fix goes in.
If acceptable, 2/2 should go to stable for 3.19.
Leif Lindholm (2):
Revert "of: fix handling of '/' in options for of_find_node_by_path()"
of: fix handling of '/' in paths with options
drivers/of/base.c | 10 --
drivers/of/unittest.c | 5 +
2 files changed, 9 insertions(+), 6 deletions(-)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Revert "of: fix handling of '/' in options for of_find_node_by_path()"
This reverts commit 106937e8ccdc
("of: fix handling of '/' in options for of_find_node_by_path()")
This fix breaks handling of strings containing both ':' and '/'.
Signed-off-by: Leif Lindholm
Fixes: 106937e8ccdc
---
drivers/of/base.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index adb8764..3b1aa08 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -714,17 +714,16 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
const char *path)
{
struct device_node *child;
- int len;
- const char *end;
+ int len = strchrnul(path, '/') - path;
+ int term;
- end = strchr(path, ':');
- if (!end)
- end = strchrnul(path, '/');
-
- len = end - path;
if (!len)
return NULL;
+ term = strchrnul(path, ':') - path;
+ if (term < len)
+ len = term;
+
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -769,12 +768,8 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
/* The path could begin with an alias */
if (*path != '/') {
- int len;
- const char *p = separator;
-
- if (!p)
- p = strchrnul(path, '/');
- len = p - path;
+ char *p = strchrnul(path, '/');
+ int len = separator ? separator - path : p - path;
/* of_aliases must not be NULL */
if (!of_aliases)
@@ -799,8 +794,6 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
path++; /* Increment past '/' delimiter */
np = __of_find_node_by_path(np, path);
path = strchrnul(path, '/');
- if (separator && separator < path)
- break;
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] of: fix handling of '/' in paths with options
Ensure proper handling of paths with appended options (after ':'),
where those options may contain a '/'.
Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
Reported-by: Peter Hurley
Tested-by: Hans de Goede
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 21 +
drivers/of/unittest.c | 5 +
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3b1aa08..2ee7265 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -714,16 +714,15 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
const char *path)
{
struct device_node *child;
- int len = strchrnul(path, '/') - path;
- int term;
+ int len;
+ const char *p1, *p2;
+ p1 = strchrnul(path, ':');
+ p2 = strchrnul(path, '/');
+ len = (p1 < p2 ? p1 : p2) - path;
if (!len)
return NULL;
- term = strchrnul(path, ':') - path;
- if (term < len)
- len = term;
-
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -768,8 +767,12 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
/* The path could begin with an alias */
if (*path != '/') {
- char *p = strchrnul(path, '/');
- int len = separator ? separator - path : p - path;
+ int len;
+ const char *p = separator;
+
+ if (!p)
+ p = strchrnul(path, '/');
+ len = p - path;
/* of_aliases must not be NULL */
if (!of_aliases)
@@ -794,6 +797,8 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
path++; /* Increment past '/' delimiter */
np = __of_find_node_by_path(np, path);
path = strchrnul(path, '/');
+ if (separator && separator < path)
+ break;
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index aba8946..8d94349 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -97,6 +97,11 @@ static void __init of_selftest_find_node_by_name(void)
"option path test, subcase #1 failed\n");
of_node_put(np);
+ np =
of_find_node_opts_by_path("/testcase-data/phandle-tests:test/option", &options);
+ selftest(np && !strcmp("test/option", options),
+"option path test, subcase #2 failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
selftest(np, "NULL option path test failed\n");
of_node_put(np);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On Tue, Mar 17, 2015 at 09:25:32AM +0100, Hans de Goede wrote: > On 17-03-15 01:19, Andreas Schwab wrote: > >Peter Hurley writes: > > > >>I don't see this as a regression, but rather a misconfiguration. > > > >It breaks booting on PowerMac. > > Actually that is more likely to be caused by: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of?id=106937e8ccdcf0f4b95fbf0fe9abd42766cade33 If so, I would appreciate a test of my proposed fix: http://www.spinics.net/lists/stable/msg82878.html Regards, Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION stable] "of: fix handling of '/' in options for of_find_node_by_path()" breaks stdout-path
Hi Hans,
On Mon, Mar 16, 2015 at 04:53:22PM +0100, Hans de Goede wrote:
> While updating my local working tree to 4.0-rc4 this morning I noticed that I
> no longer
> got any console (neither video output not serial console) on an Allwinner A20
> ARM
> board.
>
> This is caused by:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of?id=106937e8ccdcf0f4b95fbf0fe9abd42766cade33
>
> Reverting this commit fixes the serial console being gone for me.
Sorry about that.
> After this there still is an issue that tty0 no longer is seen as console,
> where it
> used to be properly used as console in 3.19, I'll investigate that further
> and send
> a separate mail about this.
>
> Greg, this commit has a: "Cc: # 3.19" please do not
> apply
> this commit to stable!
>
> u-boot sets stdout-path to this value on the board in question:
> "/soc@01c0/serial@01c28000:115200"
>
> Looking at the first hunk of the patch in question, the problem is obvious:
>
> @@ -714,16 +714,17 @@ static struct device_node
> *__of_find_node_by_path(struct device_node *parent,
> const char *path)
> {
> struct device_node *child;
> - int len = strchrnul(path, '/') - path;
> - int term;
> + int len;
> + const char *end;
> + end = strchr(path, ':');
> + if (!end)
> + end = strchrnul(path, '/');
> +
> + len = end - path;
> if (!len)
>return NULL;
> - term = strchrnul(path, ':') - path;
> - if (term < len)
> - len = term;
> -
> __for_each_child_of_node(parent, child) {
> const char *name = strrchr(child->full_name, '/');
> if (WARN(!name, "malformed device_node %s\n", child->full_name))
>
> The new code to determine len will match (when starting at root) the name of
> all child nodes against: "soc@01c0/serial@01c28000" as it checks for
> the ":" first and then uses everything before it. Where as the old code
> would match against: "soc@01c0" which is the correct thing to do.
>
> The best fix I can come up with is to check for both ":" and "/" and use
> the earlier one as end to calculate the length. I've not coded this out /
> tested this due to -ENOTIME. Note that I've also not audited the rest of
> the patch for similar issues.
>
> I will happily test any patches to fix this.
Can you give this one a try for the first part of your problem?
And if you're happy with that, I can revert the previous version and
send a new combined fix.
Rob/Grant: am I OK to assume the existence of the phandle-tests
subnode for my unrelated test?
/
Leif
>From cbb150ddd277e5fe1c109e6a675f075f0611f71d Mon Sep 17 00:00:00 2001
From: Leif Lindholm
Date: Mon, 16 Mar 2015 17:58:22 +
Subject: [PATCH] of: fix regression in of_find_node_opts_by_path()
This fixes a regression for dealing with paths that contain both a ':'
option separator and a '/' in the path preceding it. And adds a test
case to prove it.
Fixes: 106937e8ccdc ("of: fix handling of '/' in options for
of_find_node_by_path()")
Reported-by: Hans de Goede
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 10 --
drivers/of/unittest.c | 5 +
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index adb8764..2ee7265 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -715,13 +715,11 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
{
struct device_node *child;
int len;
- const char *end;
+ const char *p1, *p2;
- end = strchr(path, ':');
- if (!end)
- end = strchrnul(path, '/');
-
- len = end - path;
+ p1 = strchrnul(path, ':');
+ p2 = strchrnul(path, '/');
+ len = (p1 < p2 ? p1 : p2) - path;
if (!len)
return NULL;
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index aba8946..8d94349 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -97,6 +97,11 @@ static void __init of_selftest_find_node_by_name(void)
"option path test, subcase #1 failed\n");
of_node_put(np);
+ np =
of_find_node_opts_by_path("/testcase-data/phandle-tests:test/option", &options);
+ selftest(np && !strcmp("test/option", options),
+"option path test, subcase #2 failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
selftest(np, "NULL option path test failed\n");
of_node_put(np);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] of: fix handling of '/' in path options
On Wed, Mar 11, 2015 at 07:49:33AM -0500, Rob Herring wrote:
> On Mon, Mar 9, 2015 at 1:03 PM, Leif Lindholm
> wrote:
> > Commit 7914a7c5651a ("of: support passing console options with
> > stdout-path") neglected to deal with '/'s appearing past the ':'
> > terminator.
> >
> > This mini-series fixes this oversight and adds the tests to prove it.
> >
> > Leif Lindholm (1):
> > of: fix handling of '/' in options for of_find_node_by_path()
> >
> > Peter Hurley (1):
> > of: unittest: Add options string testcase variants
>
> Are there changes from the previous versions? I've already pulled those in.
No, just sent together as a series since I didn't realise you had.
/
Leif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] of: unittest: Add options string testcase variants
From: Peter Hurley
Add testcase variants with '/' in the options string to test for
scan beyond end path name terminated by ':'.
Signed-off-by: Peter Hurley
---
drivers/of/unittest.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 0cf9a23..b2d7547 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -92,6 +92,11 @@ static void __init of_selftest_find_node_by_name(void)
"option path test failed\n");
of_node_put(np);
+ np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
+ selftest(np && !strcmp("test/option", options),
+"option path test, subcase #1 failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
selftest(np, "NULL option path test failed\n");
of_node_put(np);
@@ -102,6 +107,12 @@ static void __init of_selftest_find_node_by_name(void)
"option alias path test failed\n");
of_node_put(np);
+ np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
+ &options);
+ selftest(np && !strcmp("test/alias/option", options),
+"option alias path test, subcase #1 failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
selftest(np, "NULL option alias path test failed\n");
of_node_put(np);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] of: fix handling of '/' in options for of_find_node_by_path()
Ensure proper handling of paths with appended options (after ':'),
where those options may contain a '/'.
Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
Reported-by: Peter Hurley
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0a8aeb8..8b904e5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -714,16 +714,17 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
const char *path)
{
struct device_node *child;
- int len = strchrnul(path, '/') - path;
- int term;
+ int len;
+ const char *end;
+ end = strchr(path, ':');
+ if (!end)
+ end = strchrnul(path, '/');
+
+ len = end - path;
if (!len)
return NULL;
- term = strchrnul(path, ':') - path;
- if (term < len)
- len = term;
-
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -768,8 +769,12 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
/* The path could begin with an alias */
if (*path != '/') {
- char *p = strchrnul(path, '/');
- int len = separator ? separator - path : p - path;
+ int len;
+ const char *p = separator;
+
+ if (!p)
+ p = strchrnul(path, '/');
+ len = p - path;
/* of_aliases must not be NULL */
if (!of_aliases)
@@ -794,6 +799,8 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
path++; /* Increment past '/' delimiter */
np = __of_find_node_by_path(np, path);
path = strchrnul(path, '/');
+ if (separator && separator < path)
+ break;
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] of: fix handling of '/' in path options
Commit 7914a7c5651a ("of: support passing console options with
stdout-path") neglected to deal with '/'s appearing past the ':'
terminator.
This mini-series fixes this oversight and adds the tests to prove it.
Leif Lindholm (1):
of: fix handling of '/' in options for of_find_node_by_path()
Peter Hurley (1):
of: unittest: Add options string testcase variants
drivers/of/base.c | 23 +++
drivers/of/unittest.c | 11 +++
2 files changed, 26 insertions(+), 8 deletions(-)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()
Hi Peter,
On Wed, Mar 04, 2015 at 10:45:02AM -0500, Peter Hurley wrote:
> The path parsing gets lost if the string after ':' contains '/'.
Doh!
Thanks for reporting (and sorry for slow response).
> The selftests below fail with:
> [1.365528] ### dt-test ### FAIL of_selftest_find_node_by_name():99 option
> path test failed
> [1.365610] ### dt-test ### FAIL of_selftest_find_node_by_name():115
> option alias path test failed
>
> Regards,
> Peter Hurley
>
>
> --- >% ---
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 41a4a13..07ba5aa 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -94,6 +94,11 @@ static void __init of_selftest_find_node_by_name(void)
>"option path test failed\n");
> of_node_put(np);
>
> + np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
> + selftest(np && !strcmp("test/option", options),
> + "option path test failed\n");
> + of_node_put(np);
> +
> np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
> selftest(np, "NULL option path test failed\n");
> of_node_put(np);
> @@ -104,6 +109,12 @@ static void __init of_selftest_find_node_by_name(void)
>"option alias path test failed\n");
> of_node_put(np);
>
> + np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
> +&options);
> + selftest(np && !strcmp("test/alias/option", options),
> + "option alias path test failed\n");
> + of_node_put(np);
> +
> np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
> selftest(np, "NULL option alias path test failed\n");
> of_node_put(np);
Could you give the below a spin, and if it works for you, send me the
above tests as a full patch so that I can post both as a series?
Regards,
Leif
>From bf4ab0b2e33902ba88809a3c4a2cdf07efd02dde Mon Sep 17 00:00:00 2001
From: Leif Lindholm
Date: Fri, 6 Mar 2015 16:38:54 +
Subject: [PATCH] of: fix handling of '/' in options for of_find_node_by_path()
Ensure proper handling of paths with appended options (after ':'),
where those options may contain a '/'.
Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
Reported-by: Peter Hurley
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0a8aeb8..8b904e5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -714,16 +714,17 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
const char *path)
{
struct device_node *child;
- int len = strchrnul(path, '/') - path;
- int term;
+ int len;
+ const char *end;
+ end = strchr(path, ':');
+ if (!end)
+ end = strchrnul(path, '/');
+
+ len = end - path;
if (!len)
return NULL;
- term = strchrnul(path, ':') - path;
- if (term < len)
- len = term;
-
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -768,8 +769,12 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
/* The path could begin with an alias */
if (*path != '/') {
- char *p = strchrnul(path, '/');
- int len = separator ? separator - path : p - path;
+ int len;
+ const char *p = separator;
+
+ if (!p)
+ p = strchrnul(path, '/');
+ len = p - path;
/* of_aliases must not be NULL */
if (!of_aliases)
@@ -794,6 +799,8 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
path++; /* Increment past '/' delimiter */
np = __of_find_node_by_path(np, path);
path = strchrnul(path, '/');
+ if (separator && separator < path)
+ break;
}
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] of: add optional options parameter to? of_find_node_by_path()
On Fri, Nov 28, 2014 at 03:25:12PM +, Grant Likely wrote:
> On Fri, 28 Nov 2014 11:34:28 +
> , Leif Lindholm
> wrote:
> > On Fri, Nov 28, 2014 at 12:44:03AM +, Grant Likely wrote:
> > > > + separator = strchr(path, ':');
> > > > + if (separator && opts)
> > > > + *opts = separator + 1;
> > > > +
> > >
> > > What about when there are no opts? Do we require the caller to make sure
> > > opts is NULL before calling the function (which sounds like a good
> > > source of bugs) or do we clear it on successful return?
> > >
> > > I think if opts is passed in, but there are no options, then it should
> > > set *opts = NULL.
> >
> > Yeah, oops.
> >
> > > There should be test cases for this also. Must set opts to NULL on
> > > successful return, and (I think) should leave opts alone on an
> > > unsuccessful search.
> >
> > I would actually argue for always nuking the opts - since that could
> > (theoretically) prevent something working by accident in spite of
> > error conditions.
> >
> > How about the below?
>
> Perfect, applied with one fixup below...
Thanks!
And since it's Friday... *cough*
>From 5c469dad81961164c444da7d6c4e1c5b1c097aab Mon Sep 17 00:00:00 2001
From: Leif Lindholm
Date: Fri, 28 Nov 2014 16:27:25 +
Subject: [PATCH] of: fix options clearing when path is "/"
The addition of the optional options extraction on
of_find_node_opts_by_path failed to clear incoming options pointer
if the specified path was "/".
Resolve this case, and add a test to detect it.
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 11 ++-
drivers/of/selftest.c |5 +
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5e16c6b..32664d1 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -743,15 +743,16 @@ struct device_node *of_find_node_opts_by_path(const char
*path, const char **opt
struct device_node *np = NULL;
struct property *pp;
unsigned long flags;
- char *separator;
+ char *separator = NULL;
+
+ if (opts) {
+ separator = strchr(path, ':');
+ *opts = separator ? separator + 1 : NULL;
+ }
if (strcmp(path, "/") == 0)
return of_node_get(of_allnodes);
- separator = strchr(path, ':');
- if (opts)
- *opts = separator ? separator + 1 : NULL;
-
/* The path could begin with an alias */
if (*path != '/') {
char *p = strchrnul(path, '/');
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 721b2a4..914f0ae 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -100,6 +100,11 @@ static void __init of_selftest_find_node_by_name(void)
np = of_find_node_opts_by_path("testcase-alias", &options);
selftest(np && !options, "option clearing test failed\n");
of_node_put(np);
+
+ options = "testoption";
+ np = of_find_node_opts_by_path("/", &options);
+ selftest(np && !options, "option clearing root node test failed\n");
+ of_node_put(np);
}
static void __init of_selftest_dynamic(void)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()
On Fri, Nov 28, 2014 at 12:44:03AM +, Grant Likely wrote:
> On Thu, 27 Nov 2014 17:56:06 +
> , Leif Lindholm
> wrote:
> > Update of_find_node_by_path():
> > 1) Rename function to of_find_node_opts_by_path(), adding an optional
> >pointer argument. Provide a static inline wrapper version of
> >of_find_node_by_path() which calls the new function with NULL as
> >the optional argument.
> > 2) Ignore any part of the path beyond and including the ':' separator.
> > 3) Set the new provided pointer argument to the beginning of the string
> >following the ':' separator.
> > 4: Add tests.
> >
> > Signed-off-by: Leif Lindholm
> > ---
> > drivers/of/base.c | 21 +
> > drivers/of/selftest.c | 12
> > include/linux/of.h| 14 +-
> > 3 files changed, 42 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 3823edf..7f0e5f7 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -699,10 +699,15 @@ static struct device_node
> > *__of_find_node_by_path(struct device_node *parent,
> > {
> > struct device_node *child;
> > int len = strchrnul(path, '/') - path;
> > + int term;
> >
> > if (!len)
> > return NULL;
> >
> > + term = strchrnul(path, ':') - path;
> > + if (term < len)
> > + len = term;
> > +
> > __for_each_child_of_node(parent, child) {
> > const char *name = strrchr(child->full_name, '/');
> > if (WARN(!name, "malformed device_node %s\n", child->full_name))
> > @@ -715,11 +720,14 @@ static struct device_node
> > *__of_find_node_by_path(struct device_node *parent,
> > }
> >
> > /**
> > - * of_find_node_by_path - Find a node matching a full OF path
> > + * of_find_node_opts_by_path - Find a node matching a full OF path
> > * @path: Either the full path to match, or if the path does not
> > *start with '/', the name of a property of the /aliases
> > *node (an alias). In the case of an alias, the node
> > *matching the alias' value will be returned.
> > + * @opts: Address of a pointer into which to store the start of
> > + *an options string appended to the end of the path with
> > + *a ':' separator.
> > *
> > * Valid paths:
> > * /foo/barFull path
> > @@ -729,19 +737,24 @@ static struct device_node
> > *__of_find_node_by_path(struct device_node *parent,
> > * Returns a node pointer with refcount incremented, use
> > * of_node_put() on it when done.
> > */
> > -struct device_node *of_find_node_by_path(const char *path)
> > +struct device_node *of_find_node_opts_by_path(const char *path, const char
> > **opts)
> > {
> > struct device_node *np = NULL;
> > struct property *pp;
> > unsigned long flags;
> > + char *separator;
> >
> > if (strcmp(path, "/") == 0)
> > return of_node_get(of_allnodes);
> >
> > + separator = strchr(path, ':');
> > + if (separator && opts)
> > + *opts = separator + 1;
> > +
>
> What about when there are no opts? Do we require the caller to make sure
> opts is NULL before calling the function (which sounds like a good
> source of bugs) or do we clear it on successful return?
>
> I think if opts is passed in, but there are no options, then it should
> set *opts = NULL.
Yeah, oops.
> There should be test cases for this also. Must set opts to NULL on
> successful return, and (I think) should leave opts alone on an
> unsuccessful search.
I would actually argue for always nuking the opts - since that could
(theoretically) prevent something working by accident in spite of
error conditions.
How about the below?
/
Leif
>From 2e1a44e539967d96366d074ae158092900e0c822 Mon Sep 17 00:00:00 2001
From: Leif Lindholm
Date: Thu, 27 Nov 2014 09:24:31 +
Subject: [PATCH] of: add optional options parameter to of_find_node_by_path()
Update of_find_node_by_path():
1) Rename function to of_find_node_opts_by_path(), adding an optional
pointer argument. Provide a static inline wrapper version of
of_find_node_by_path() which calls the new function with NULL as
the optional argument.
2) Ignore any part of the path beyond and including the ':' separator.
3) Set the new provided pointer argument to the beginning of the string
[PATCH v3 3/3] of: support passing console options with stdout-path
Support specifying console options (like with console=ttyXN,)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
Signed-off-by: Leif Lindholm
---
drivers/of/base.c |9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7f0e5f7..6d2d45e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
struct device_node *of_chosen;
struct device_node *of_aliases;
struct device_node *of_stdout;
+static const char *of_stdout_options;
struct kset *of_kset;
@@ -1844,7 +1845,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
align))
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
if (name)
- of_stdout = of_find_node_by_path(name);
+ of_stdout = of_find_node_opts_by_path(name,
&of_stdout_options);
}
if (!of_aliases)
@@ -1968,9 +1969,13 @@ EXPORT_SYMBOL_GPL(of_prop_next_string);
*/
bool of_console_check(struct device_node *dn, char *name, int index)
{
+ char *console_options;
+
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index, NULL);
+
+ console_options = kstrdup(of_stdout_options, GFP_KERNEL);
+ return !add_preferred_console(name, index, console_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path
Add a global binding for the chosen node.
Include a description of the stdout-path, and an explicit statement on
its extra options in the context of a UART console.
Opening description stolen from www.devicetree.org, and part of the
remaining text provided by Mark Rutland.
Signed-off-by: Leif Lindholm
---
Documentation/devicetree/bindings/chosen.txt | 42 ++
1 file changed, 42 insertions(+)
create mode 100644 Documentation/devicetree/bindings/chosen.txt
diff --git a/Documentation/devicetree/bindings/chosen.txt
b/Documentation/devicetree/bindings/chosen.txt
new file mode 100644
index 000..9cd74e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -0,0 +1,42 @@
+The chosen node
+---
+
+The chosen node does not represent a real device, but serves as a place
+for passing data between firmware and the operating system, like boot
+arguments. Data in the chosen node does not represent the hardware.
+
+
+stdout-path property
+
+
+Device trees may specify the device to be used for boot console output
+with a stdout-path property under /chosen, as described in ePAPR, e.g.
+
+/ {
+ chosen {
+ stdout-path = "/serial@f00:115200";
+ };
+
+ serial@f00 {
+ compatible = "vendor,some-uart";
+ reg = <0xf00 0x10>;
+ };
+};
+
+If the character ":" is present in the value, this terminates the path.
+The meaning of any characters following the ":" is device-specific, and
+must be specified in the relevant binding documentation.
+
+For UART devices, the format supported by uart_parse_options() is the
+expected one. In this case, the format of the string is:
+
+ {{{}}}
+
+where
+
+ baud- baud rate in decimal
+ parity - 'n' (none), 'o', (odd) or 'e' (even)
+ bits- number of data bits
+ flow- 'r' (rts)
+
+For example: 115200n8r
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()
Update of_find_node_by_path():
1) Rename function to of_find_node_opts_by_path(), adding an optional
pointer argument. Provide a static inline wrapper version of
of_find_node_by_path() which calls the new function with NULL as
the optional argument.
2) Ignore any part of the path beyond and including the ':' separator.
3) Set the new provided pointer argument to the beginning of the string
following the ':' separator.
4: Add tests.
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 21 +
drivers/of/selftest.c | 12
include/linux/of.h| 14 +-
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..7f0e5f7 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
{
struct device_node *child;
int len = strchrnul(path, '/') - path;
+ int term;
if (!len)
return NULL;
+ term = strchrnul(path, ':') - path;
+ if (term < len)
+ len = term;
+
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
}
/**
- * of_find_node_by_path - Find a node matching a full OF path
+ * of_find_node_opts_by_path - Find a node matching a full OF path
* @path: Either the full path to match, or if the path does not
*start with '/', the name of a property of the /aliases
*node (an alias). In the case of an alias, the node
*matching the alias' value will be returned.
+ * @opts: Address of a pointer into which to store the start of
+ *an options string appended to the end of the path with
+ *a ':' separator.
*
* Valid paths:
* /foo/barFull path
@@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
* Returns a node pointer with refcount incremented, use
* of_node_put() on it when done.
*/
-struct device_node *of_find_node_by_path(const char *path)
+struct device_node *of_find_node_opts_by_path(const char *path, const char
**opts)
{
struct device_node *np = NULL;
struct property *pp;
unsigned long flags;
+ char *separator;
if (strcmp(path, "/") == 0)
return of_node_get(of_allnodes);
+ separator = strchr(path, ':');
+ if (separator && opts)
+ *opts = separator + 1;
+
/* The path could begin with an alias */
if (*path != '/') {
char *p = strchrnul(path, '/');
- int len = p - path;
+ int len = separator ? separator - path : p - path;
/* of_aliases must not be NULL */
if (!of_aliases)
@@ -770,7 +783,7 @@ struct device_node *of_find_node_by_path(const char *path)
raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
}
-EXPORT_SYMBOL(of_find_node_by_path);
+EXPORT_SYMBOL(of_find_node_opts_by_path);
/**
* of_find_node_by_name - Find a node by its "name" property
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index e2d79af..c298065 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -43,6 +43,7 @@ static bool selftest_live_tree;
static void __init of_selftest_find_node_by_name(void)
{
struct device_node *np;
+ const char *options;
np = of_find_node_by_path("/testcase-data");
selftest(np && !strcmp("/testcase-data", np->full_name),
@@ -83,6 +84,17 @@ static void __init of_selftest_find_node_by_name(void)
np = of_find_node_by_path("testcase-alias/missing-path");
selftest(!np, "non-existent alias with relative path returned node
%s\n", np->full_name);
of_node_put(np);
+
+ np = of_find_node_opts_by_path("/testcase-data:testoption", &options);
+ selftest(np && !strcmp("testoption", options),
+"option path test failed\n");
+ of_node_put(np);
+
+ np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
+ &options);
+ selftest(np && !strcmp("testaliasoption", options),
+"option alias path test failed\n");
+ of_node_put(np);
}
static void __init of_selftest_dynamic(void)
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc..a
[PATCH v3 0/3] of: support passing console options with stdout-path
This set started its life as a small, self-contained, patch enabling specifying console options in the stdout-path property, but then grew into a mahoosive behemoth with one of the patches taking 1m42s for get_maintainer to process. It has now once again reverted to a smaller, more pleasant, state of being. Changes since v2: - Revert the invasive bit, creating a replacement wrapper function for of_get_node_by_path() callers. - Make the of_get_node_opts_by_path() function return a const pointer to the argument. - Added binding for /chosen node, with a description of stdout-path. Changes since v1: - Change interface of of_get_node_by_path() to take an additional argument, and update all of its callers to keep working. - Rework original patch to use this interface. Leif Lindholm (3): devicetree: of: Add bindings for chosen node, stdout-path of: add optional options parameter to of_find_node_by_path() of: support passing console options with stdout-path Documentation/devicetree/bindings/chosen.txt | 42 ++ drivers/of/base.c| 30 ++ drivers/of/selftest.c| 12 include/linux/of.h | 14 - 4 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 Documentation/devicetree/bindings/chosen.txt -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] of: support passing console options with stdout-path
On Thu, Nov 27, 2014 at 12:15:43PM +, Mark Rutland wrote:
> On Wed, Nov 26, 2014 at 09:48:47PM +, Andrew Lunn wrote:
> > On Wed, Nov 26, 2014 at 09:07:33PM +, Grant Likely wrote:
> > > On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn wrote:
> > > > On Wed, Nov 26, 2014 at 05:40:40PM +, Leif Lindholm wrote:
> > > >> Support specifying console options (like with console=ttyXN,)
> > > >> by appending them to the stdout-path property after a separating ':'.
> > > >>
> > > >> Example:
> > > >> stdout-path = "uart0:115200";
> > > >
> > > > Hi Leif
> > > >
> > > > This should be documented somewhere under
> > > > Documentation/devicetree/bindings/
> > > >
> > > > Not sure where thought. Maybe a top level chosen.txt?
> > >
> > > Actually, this one doesn't. It is already documented in ePAPR
> >
> > Hi Grant
> >
> > Humm, do i have an old version of ePAPR?
> >
> > All i see is that in Table 3-4 It says:
> >
> > stdout-path O A string that specifies the full path to the
> >node representing the device to be used for
> >boot console output. If the character ":" is
> >present in the value it terminates the
> >path. The value may be an alias.
> >
> >If the stdin-path property is not specified,
> >stdout-path should be assumed to define the input device.
> >
> > So what is before the : is defined. What comes afterwards,
> > baudrate/parity/bits/flow control does not appear to the defined in
> > ePAPR. Should we not document the extension being added here?
>
> I believe that we should, and it should be relatively trivial to add a
> document stating that the format and meaning of the parts after the ':'
> are device-specific.
Device-specific is a bit broad though?
> So how about Documentation/devicetree/bindings/serial/stdout-path.txt,
> with something like the following:
There is, however, nothing serial-specific about this functionality -
it console-specific.
Could it be bindings/console/stdout-path.txt?
> >8
> Device trees may specify the device to be used for boot console output
> with a stdout-path property under /chosen, as described in ePAPR, e.g.
>
> / {
> chosen {
> stdout-path = "/serial@f00:115200";
> };
>
> serial@f00 {
> compatible = "vendor,some-uart";
> reg = <0xf00 0x10>;
> };
> };
>
> If the character ":" is present in the value, this terminates the path.
> The meaning of any cahracters following the ":" is device-specific, and
> must be specified in the relevant binding documentation.
> >8
>
> The more difficult part is documenting those (and I'm still uneasy about
> conflating the Linux driver command line options with the DT binding for
> that reason).
For the current situation, which _is_ serial-specific, could we then
add a serial/stdout-path.txt describing the mapping to
uart_parse_options - making that interface an implicit requirement for
the use of stdout-path?
/
Leif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] of: add optional options parameter to of_find_node_by_path()
On Wed, Nov 26, 2014 at 09:06:33PM +, Grant Likely wrote:
> On Wed, Nov 26, 2014 at 5:40 PM, Leif Lindholm
> wrote:
> > Update of_find_node_by_path():
> > 1) Ignore any part of the path beyond and including the ':' separator.
> > 2) Set the new provided pointer argument to the beginning of the string
> >following the ':' separator.
> >
> > Coccinelle fixup using:
> >
> > @@
> > expression E1;
> > @@
> >
> > - of_find_node_by_path(E1)
> > + of_find_node_by_path(E1, NULL)
> >
> > drivers/of/resolver.c manually updated, since spatch fails to parse
> > it correctly.
> >
> > Signed-off-by: Leif Lindholm
>
> Okay, so you're probably going to kill me for the next comment...
> After actually looking at this I can see that it's going to be a hard
> patch to merge because of conflicts. It will need to be merged at the
> end of a merge window to catch all the users, but that will mean that
> the important part of the patch won't be able to be queued up in
> linux-next.
Not to worry - I'll simply keep this to guilt trip you with at some
point in the future.
Seeing get_maintainer take 1m42s on a quad-i7 with the entire kernel
tree in disk cache was nearly reward enough :)
> So you were right the first time around. Create a new function name
> that adds the extra argument and make of_find_node_by_path() a static
> inline wrapper. That way I can queue it up into linux-next immediately
> and the cleanup across the tree can be generated and submitted at the
> very end of the merge window.
That will also make it a lot less invasive to potentially get it
backported to debian-kernel, so we can have this support for the
Jessie installer.
I'll whip that up tomorrow morning.
/
Leif
> > @@ -380,7 +380,8 @@ static inline struct device_node
> > *of_find_matching_node_and_match(
> > return NULL;
> > }
> >
> > -static inline struct device_node *of_find_node_by_path(const char *path)
> > +static inline struct device_node *of_find_node_by_path(const char *path,
> > + char **opts)
>
> const char **opts
>
> > {
> > return NULL;
> > }
> > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > index e695517..0056963 100644
> > --- a/sound/soc/fsl/fsl_ssi.c
> > +++ b/sound/soc/fsl/fsl_ssi.c
> > @@ -1427,7 +1427,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
> > * device tree. We also pass the address of the CPU DAI driver
> > * structure.
> > */
> > - sprop = of_get_property(of_find_node_by_path("/"), "compatible",
> > NULL);
> > + sprop = of_get_property(of_find_node_by_path("/", NULL),
> > "compatible", NULL);
> > /* Sometimes the compatible name has a "fsl," prefix, so we strip
> > it. */
> > p = strrchr(sprop, ',');
> > if (p)
> > --
> > 1.7.10.4
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] of: support passing console options with stdout-path
This used to be a simple little patch, enabling support for passing console parameters via stdout-path. It is now a two-part monstrosity series, changing a core dt function interface and touching 107 files. Due to its invasiveness, checkpatch throws both errors and warnings on 1/2, as it maintains existing whitespace errors. Changes since v1: - Change interface of of_get_node_by_path() to take an additional argument, and update all of its callers to keep working. - Rework original patch to use this interface. Leif Lindholm (2): of: add optional options parameter to of_find_node_by_path() of: support passing console options with stdout-path arch/arm/kernel/devtree.c |2 +- arch/arm/mach-bcm/kona_smp.c|2 +- arch/arm/mach-imx/clk.c |2 +- arch/arm/mach-imx/cpu.c |2 +- arch/arm/mach-integrator/integrator_ap.c|4 +- arch/arm/mach-mxs/mach-mxs.c|2 +- arch/arm/mach-nomadik/cpu-8815.c|2 +- arch/arm/mach-shmobile/timer.c |2 +- arch/arm/mach-u300/core.c |2 +- arch/arm64/kernel/topology.c|2 +- arch/microblaze/kernel/reset.c |2 +- arch/powerpc/include/asm/kvm_para.h |2 +- arch/powerpc/kernel/btext.c |2 +- arch/powerpc/kernel/ibmebus.c |4 +- arch/powerpc/kernel/legacy_serial.c |4 +- arch/powerpc/kernel/machine_kexec.c |2 +- arch/powerpc/kernel/machine_kexec_64.c |2 +- arch/powerpc/kernel/pci_32.c|4 +- arch/powerpc/kernel/proc_powerpc.c |2 +- arch/powerpc/kernel/rtas.c |2 +- arch/powerpc/kernel/rtas_pci.c |2 +- arch/powerpc/kernel/setup-common.c |6 +- arch/powerpc/kernel/setup_64.c |2 +- arch/powerpc/mm/numa.c | 15 +++-- arch/powerpc/platforms/52xx/efika.c |4 +- arch/powerpc/platforms/85xx/xes_mpc85xx.c |2 +- arch/powerpc/platforms/8xx/mpc885ads_setup.c|4 +- arch/powerpc/platforms/cell/celleb_setup.c |2 +- arch/powerpc/platforms/cell/qpace_setup.c |2 +- arch/powerpc/platforms/cell/ras.c |2 +- arch/powerpc/platforms/cell/setup.c |4 +- arch/powerpc/platforms/cell/spufs/inode.c |2 +- arch/powerpc/platforms/chrp/pci.c |4 +- arch/powerpc/platforms/chrp/setup.c | 12 ++-- arch/powerpc/platforms/embedded6xx/ls_uart.c|2 +- arch/powerpc/platforms/maple/pci.c |2 +- arch/powerpc/platforms/maple/setup.c|2 +- arch/powerpc/platforms/pasemi/pci.c |2 +- arch/powerpc/platforms/pasemi/setup.c |2 +- arch/powerpc/platforms/powermac/feature.c |6 +- arch/powerpc/platforms/powermac/pci.c |2 +- arch/powerpc/platforms/powermac/setup.c |7 ++- arch/powerpc/platforms/powermac/smp.c |4 +- arch/powerpc/platforms/powermac/udbg_scc.c |2 +- arch/powerpc/platforms/powernv/opal-async.c |2 +- arch/powerpc/platforms/powernv/opal-sysparam.c |2 +- arch/powerpc/platforms/powernv/opal.c |4 +- arch/powerpc/platforms/powernv/setup.c |2 +- arch/powerpc/platforms/ps3/os-area.c|4 +- arch/powerpc/platforms/pseries/dlpar.c |8 +-- arch/powerpc/platforms/pseries/hotplug-memory.c |6 +- arch/powerpc/platforms/pseries/io_event_irq.c |2 +- arch/powerpc/platforms/pseries/lparcfg.c|6 +- arch/powerpc/platforms/pseries/pseries_energy.c |4 +- arch/powerpc/platforms/pseries/ras.c|4 +- arch/powerpc/platforms/pseries/reconfig.c |6 +- arch/powerpc/platforms/pseries/setup.c |4 +- arch/powerpc/sysdev/mpic_msgr.c |2 +- arch/powerpc/sysdev/mv64x60_dev.c |2 +- arch/powerpc/sysdev/mv64x60_udbg.c |2 +- arch/sparc/kernel/chmc.c|2 +- arch/sparc/kernel/irq_64.c |2 +- arch/sparc/kernel/leon_kernel.c |2 +- arch/sparc/kernel/leon_smp.c|2 +- arch/sparc/kernel/of_device_32.c|2 +- arch/sparc/kernel/of_device_64.c|2 +- arch/sparc/kernel/prom_32.c |2 +- arch/sparc/kernel/time_64.c |2 +- arch/x86/platform/olpc/olpc.c |2 +- drivers/ata/pata_macio.c|2 +- drivers/cpufreq/pmac64-cpufreq.c|5 +- drivers/cpufreq/powernv-cpufreq.c |2 +- drivers/cpuidle/cpuidle-big_little.c|2 +- drivers/cpuidle/cpuidle
[PATCH v2 2/2] of: support passing console options with stdout-path
Support specifying console options (like with console=ttyXN,)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
Signed-off-by: Leif Lindholm
---
drivers/of/base.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3e764bd..d265514 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
struct device_node *of_chosen;
struct device_node *of_aliases;
struct device_node *of_stdout;
+static char *of_stdout_options;
struct kset *of_kset;
@@ -1844,7 +1845,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
align))
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
if (name)
- of_stdout = of_find_node_by_path(name, NULL);
+ of_stdout = of_find_node_by_path(name,
&of_stdout_options);
}
if (!of_aliases)
@@ -1970,7 +1971,7 @@ bool of_console_check(struct device_node *dn, char *name,
int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index, NULL);
+ return !add_preferred_console(name, index, of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] of: add optional options parameter to of_find_node_by_path()
Update of_find_node_by_path(): 1) Ignore any part of the path beyond and including the ':' separator. 2) Set the new provided pointer argument to the beginning of the string following the ':' separator. Coccinelle fixup using: @@ expression E1; @@ - of_find_node_by_path(E1) + of_find_node_by_path(E1, NULL) drivers/of/resolver.c manually updated, since spatch fails to parse it correctly. Signed-off-by: Leif Lindholm --- arch/arm/kernel/devtree.c |2 +- arch/arm/mach-bcm/kona_smp.c|2 +- arch/arm/mach-imx/clk.c |2 +- arch/arm/mach-imx/cpu.c |2 +- arch/arm/mach-integrator/integrator_ap.c|4 +- arch/arm/mach-mxs/mach-mxs.c|2 +- arch/arm/mach-nomadik/cpu-8815.c|2 +- arch/arm/mach-shmobile/timer.c |2 +- arch/arm/mach-u300/core.c |2 +- arch/arm64/kernel/topology.c|2 +- arch/microblaze/kernel/reset.c |2 +- arch/powerpc/include/asm/kvm_para.h |2 +- arch/powerpc/kernel/btext.c |2 +- arch/powerpc/kernel/ibmebus.c |4 +- arch/powerpc/kernel/legacy_serial.c |4 +- arch/powerpc/kernel/machine_kexec.c |2 +- arch/powerpc/kernel/machine_kexec_64.c |2 +- arch/powerpc/kernel/pci_32.c|4 +- arch/powerpc/kernel/proc_powerpc.c |2 +- arch/powerpc/kernel/rtas.c |2 +- arch/powerpc/kernel/rtas_pci.c |2 +- arch/powerpc/kernel/setup-common.c |6 +- arch/powerpc/kernel/setup_64.c |2 +- arch/powerpc/mm/numa.c | 15 +++-- arch/powerpc/platforms/52xx/efika.c |4 +- arch/powerpc/platforms/85xx/xes_mpc85xx.c |2 +- arch/powerpc/platforms/8xx/mpc885ads_setup.c|4 +- arch/powerpc/platforms/cell/celleb_setup.c |2 +- arch/powerpc/platforms/cell/qpace_setup.c |2 +- arch/powerpc/platforms/cell/ras.c |2 +- arch/powerpc/platforms/cell/setup.c |4 +- arch/powerpc/platforms/cell/spufs/inode.c |2 +- arch/powerpc/platforms/chrp/pci.c |4 +- arch/powerpc/platforms/chrp/setup.c | 12 ++-- arch/powerpc/platforms/embedded6xx/ls_uart.c|2 +- arch/powerpc/platforms/maple/pci.c |2 +- arch/powerpc/platforms/maple/setup.c|2 +- arch/powerpc/platforms/pasemi/pci.c |2 +- arch/powerpc/platforms/pasemi/setup.c |2 +- arch/powerpc/platforms/powermac/feature.c |6 +- arch/powerpc/platforms/powermac/pci.c |2 +- arch/powerpc/platforms/powermac/setup.c |7 ++- arch/powerpc/platforms/powermac/smp.c |4 +- arch/powerpc/platforms/powermac/udbg_scc.c |2 +- arch/powerpc/platforms/powernv/opal-async.c |2 +- arch/powerpc/platforms/powernv/opal-sysparam.c |2 +- arch/powerpc/platforms/powernv/opal.c |4 +- arch/powerpc/platforms/powernv/setup.c |2 +- arch/powerpc/platforms/ps3/os-area.c|4 +- arch/powerpc/platforms/pseries/dlpar.c |8 +-- arch/powerpc/platforms/pseries/hotplug-memory.c |6 +- arch/powerpc/platforms/pseries/io_event_irq.c |2 +- arch/powerpc/platforms/pseries/lparcfg.c|6 +- arch/powerpc/platforms/pseries/pseries_energy.c |4 +- arch/powerpc/platforms/pseries/ras.c|4 +- arch/powerpc/platforms/pseries/reconfig.c |6 +- arch/powerpc/platforms/pseries/setup.c |4 +- arch/powerpc/sysdev/mpic_msgr.c |2 +- arch/powerpc/sysdev/mv64x60_dev.c |2 +- arch/powerpc/sysdev/mv64x60_udbg.c |2 +- arch/sparc/kernel/chmc.c|2 +- arch/sparc/kernel/irq_64.c |2 +- arch/sparc/kernel/leon_kernel.c |2 +- arch/sparc/kernel/leon_smp.c|2 +- arch/sparc/kernel/of_device_32.c|2 +- arch/sparc/kernel/of_device_64.c|2 +- arch/sparc/kernel/prom_32.c |2 +- arch/sparc/kernel/time_64.c |2 +- arch/x86/platform/olpc/olpc.c |2 +- drivers/ata/pata_macio.c|2 +- drivers/cpufreq/pmac64-cpufreq.c|5 +- drivers/cpufreq/powernv-cpufreq.c |2 +- drivers/cpuidle/cpuidle-big_little.c|2 +- drivers/cpuidle/cpuidle-powernv.c |2 +- drivers/edac/cpc925_edac.c |2 +- drivers/hwmon/ibmpowernv.c |4 +- drivers/ide/pmac.c |
Re: [PATCH] of: support passing console options with stdout-path
On Tue, Nov 25, 2014 at 02:58:54PM +, Grant Likely wrote:
> > + len = strchrnul(path, ':') - path;
> > +
> > for_each_property_of_node(of_aliases, pp) {
> > if (strlen(pp->name) == len && !strncmp(pp->name, path,
> > len)) {
> > np = of_find_node_by_path(pp->value);
>
> Can you add a testcase to drivers/of/unittests.c for this new path
> parsing? Then we know it's correct!
Will do.
> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
> > align))
> > name = of_get_property(of_chosen, "linux,stdout-path",
> > NULL);
> > if (IS_ENABLED(CONFIG_PPC) && !name)
> > name = of_get_property(of_aliases, "stdout", NULL);
> > - if (name)
> > + if (name) {
> > of_stdout = of_find_node_by_path(name);
> > + of_stdout_options = strchr(name, ':');
> > + if (of_stdout_options != NULL)
> > + of_stdout_options++;
> > + }
>
> Not so good. The ':' should actually be a generic part of
> of_find_node_by_path(), it doesn't have to be limited to stdout parsing.
> There are other places that would use it. I would add a second,
> optional, argument to of_find_node_by_path() that will update a pointer
> to the arguments after the ':'.
So, I agree this would be nicer...
However, I'm counting 157 callers of this function outside of
drivers/of and 43 inside. Any chance you'd let me get away with a
separate of_find_extra_options_by_path()?
/
Leif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: support passing console options with stdout-path
On Tue, Nov 25, 2014 at 12:07:29PM +, Ian Campbell wrote:
> My concern is that this is Linux specific, other OSes may have different
> ideas about how stdout options should be formatted within this property.
> (At least I don't know of any standardisation of the 115200n8 thing --
> I'd love to be corrected!)
>
> If I were a firmware author I'd be wary of specifying a stdout-path with
> a Linux specific suffix.
> Search ePAPR for baud it seems that the generic serial binding includes
> a current-speed property in 6.2.1.2. It then goes on a bit ambiguously
> to talk about the NS16550 in 6.2.2 but I think 6.2.1.2 was intended to
> be generic. No mention of stop-bits/parity etc though, they are assumed
> to be set already I think
>
> One thought I had was to define a dt-stdout "pseudo-console" so that
> console=dt-stdout,115200n8 or something could be used.
I'll wait for others to comment on the above.
> Anyway I applied your patch to v3.18-rc5 and ran it on a Mustang and it
> didn't work for some reason. I'm using:
>
> fdt set /chosen stdout-path "/soc/serial@1c02:115200"
> setenv bootargs "earlycon=uart8250,mmio32,0x1c02 root=/dev/sda3
> rw debug"
>
> So I get earlycon but then no proper console. Removing earlycon just
> makes that stop working.
Right, so having debugged this a bit offline, I'm amazed I booted
anything at all, given how badly I broke path scanning...
Please ignore previous version - a fixed one follows:
/
Leif
>From aef87fd958902afe881720286d525e10997462b8 Mon Sep 17 00:00:00 2001
From: Leif Lindholm
Date: Mon, 24 Nov 2014 22:23:58 +
Subject: [PATCH] of: support passing console options with stdout-path
Support specifying console options (like with console=ttyXN,)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
This patch also modifies of_find_node_by_path() to match only the
portion of the path before a ':'.
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..ecd6290 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
struct device_node *of_chosen;
struct device_node *of_aliases;
struct device_node *of_stdout;
+static char *of_stdout_options;
struct kset *of_kset;
@@ -699,10 +700,15 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
{
struct device_node *child;
int len = strchrnul(path, '/') - path;
+ int term;
if (!len)
return NULL;
+ term = strchrnul(path, ':') - path;
+ if (term < len)
+ len = term;
+
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -742,11 +748,16 @@ struct device_node *of_find_node_by_path(const char *path)
if (*path != '/') {
char *p = strchrnul(path, '/');
int len = p - path;
+ int term;
/* of_aliases must not be NULL */
if (!of_aliases)
return NULL;
+ term = strchrnul(path, ':') - path;
+ if (term < len)
+ len = term;
+
for_each_property_of_node(of_aliases, pp) {
if (strlen(pp->name) == len && !strncmp(pp->name, path,
len)) {
np = of_find_node_by_path(pp->value);
@@ -1830,8 +1841,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
align))
name = of_get_property(of_chosen, "linux,stdout-path",
NULL);
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
- if (name)
+ if (name) {
of_stdout = of_find_node_by_path(name);
+ of_stdout_options = strchr(name, ':');
+ if (of_stdout_options != NULL)
+ of_stdout_options++;
+ }
}
if (!of_aliases)
@@ -1957,7 +1972,7 @@ bool of_console_check(struct device_node *dn, char *name,
int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index, NULL);
+ return !add_preferred_console(name, index, of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: support passing console options with stdout-path
On Tue, Nov 25, 2014 at 10:35:04AM +, Mark Rutland wrote:
> On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
>
> I would very much like to be able to use this -- it will allow
> distributions to boot on a board without having to know _anything_ about
> the console UART until userspace is up, which would make it possible to
> have a completely generic installer image, without requiring the
> platform to provide bootargs.
>
> My only concern is that this conflates the set of kernel command line
> options for the UART wit the DT binding for it. I don't know how good
> people are at keeping those options stable, and I know they are
> currently not documented -- we would need to add a stdout-path options
> section to relevant UART bindings.
I don't disagree.
Current options are fairly well defined and stable, at least for any
driver that uses uart_parse_options() (documented in
Documentation/serial/driver).
>From drivers/tty/serial/serial_core.c:
* uart_parse_options decodes a string containing the serial console
* options. The format of the string is ,
* eg: 115200n8r
/
Leif
>
> >
> > This patch also modifies of_find_node_by_path() to match only the
> > portion of the path before a ':'.
> >
> > Signed-off-by: Leif Lindholm
> > ---
> > drivers/of/base.c | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 3823edf..89c6b33 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
> > struct device_node *of_chosen;
> > struct device_node *of_aliases;
> > struct device_node *of_stdout;
> > +static char *of_stdout_options;
> >
> > struct kset *of_kset;
> >
> > @@ -703,6 +704,8 @@ static struct device_node
> > *__of_find_node_by_path(struct device_node *parent,
> > if (!len)
> > return NULL;
> >
> > + len = strchrnul(path, ':') - path;
> > +
> > __for_each_child_of_node(parent, child) {
> > const char *name = strrchr(child->full_name, '/');
> > if (WARN(!name, "malformed device_node %s\n", child->full_name))
> > @@ -747,6 +750,8 @@ struct device_node *of_find_node_by_path(const char
> > *path)
> > if (!of_aliases)
> > return NULL;
> >
> > + len = strchrnul(path, ':') - path;
> > +
> > for_each_property_of_node(of_aliases, pp) {
> > if (strlen(pp->name) == len && !strncmp(pp->name, path,
> > len)) {
> > np = of_find_node_by_path(pp->value);
> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
> > align))
> > name = of_get_property(of_chosen, "linux,stdout-path",
> > NULL);
> > if (IS_ENABLED(CONFIG_PPC) && !name)
> > name = of_get_property(of_aliases, "stdout", NULL);
> > - if (name)
> > + if (name) {
> > of_stdout = of_find_node_by_path(name);
> > + of_stdout_options = strchr(name, ':');
> > + if (of_stdout_options != NULL)
> > + of_stdout_options++;
> > + }
> > }
> >
> > if (!of_aliases)
> > @@ -1957,7 +1966,7 @@ bool of_console_check(struct device_node *dn, char
> > *name, int index)
> > {
> > if (!dn || dn != of_stdout || console_set_on_cmdline)
> > return false;
> > - return !add_preferred_console(name, index, NULL);
> > + return !add_preferred_console(name, index, of_stdout_options);
> > }
> > EXPORT_SYMBOL_GPL(of_console_check);
> >
> > --
> > 1.9.1
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: support passing console options with stdout-path
On Tue, Nov 25, 2014 at 12:00:16AM +0100, Andrew Lunn wrote: > On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote: > > Support specifying console options (like with console=ttyXN,) > > by appending them to the stdout-path property after a separating ':'. > > > > Example: > > stdout-path = "uart0:115200"; > > > > This patch also modifies of_find_node_by_path() to match only the > > portion of the path before a ':'. > > Hi Leif > > These appears to somewhat conform to ePAPR, which says: > > A string that specifies the full path to the node representing > the device to be used for boot console output. If the character > ":" is present in the value it terminates the path. > > So you can put any random junk after the :. However, are we going to > have backward/forward compatibility problems, and problems with > bootloaders? The current kernel code does not look for the :. So a new > DT blob on an old kernel will not work so well. I _think_ this will be less of a problem in practice than it could be in theory. The reason this is needed is that at least several platforms have different baudrate settings in firmware than the default provided by the kernel for their UART. As a result, stdout-path becomes semi-useless; the only thing it gives you is the ability to get earlycon output without specifying a specific device (and then the console turns to garbage once non-earlycon is enabled). Hence, a platform that gets along happily today without the ability to specify console options in stdout-path would have no pressing need to add it to its dt. This should permit at least a very long, soft, transition path. (console= on kernel command line continues to override stdout-path.) > More worrying, barebox does not support the : either. So there is a > danger your bootloader suddenly goes silent after a dt blob update. _That_ would be most unfortunate. > Would it be safer to add a new property in chosen? My preference would be not, given the above, but the important thing is to get the functionality in so we get rid of mandatory console=. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] of: support passing console options with stdout-path
Support specifying console options (like with console=ttyXN,)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
This patch also modifies of_find_node_by_path() to match only the
portion of the path before a ':'.
Signed-off-by: Leif Lindholm
---
drivers/of/base.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..89c6b33 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
struct device_node *of_chosen;
struct device_node *of_aliases;
struct device_node *of_stdout;
+static char *of_stdout_options;
struct kset *of_kset;
@@ -703,6 +704,8 @@ static struct device_node *__of_find_node_by_path(struct
device_node *parent,
if (!len)
return NULL;
+ len = strchrnul(path, ':') - path;
+
__for_each_child_of_node(parent, child) {
const char *name = strrchr(child->full_name, '/');
if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -747,6 +750,8 @@ struct device_node *of_find_node_by_path(const char *path)
if (!of_aliases)
return NULL;
+ len = strchrnul(path, ':') - path;
+
for_each_property_of_node(of_aliases, pp) {
if (strlen(pp->name) == len && !strncmp(pp->name, path,
len)) {
np = of_find_node_by_path(pp->value);
@@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
align))
name = of_get_property(of_chosen, "linux,stdout-path",
NULL);
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
- if (name)
+ if (name) {
of_stdout = of_find_node_by_path(name);
+ of_stdout_options = strchr(name, ':');
+ if (of_stdout_options != NULL)
+ of_stdout_options++;
+ }
}
if (!of_aliases)
@@ -1957,7 +1966,7 @@ bool of_console_check(struct device_node *dn, char *name,
int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index, NULL);
+ return !add_preferred_console(name, index, of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "magic" handling of memory nodes
On Thu, Apr 24, 2014 at 10:57:03AM -0600, Stephen Warren wrote:
> > Secondly, the only reason these platforms could ever have worked is
> > because they include .dtsi files that define a memory node with a
> > type explicitly set. Since this node already exists, its contents get
> > overridden, but the type tag remains. Of course, this only happens
> > with nodes called explicitly "memory" - but it happens regardless of
> > what other things they contain.
>
> That's precisely how DT includes/overrides are supposed to work, and is
> entirely expected and normal.
Understood.
> Since skeleton.dtsi already says:
>
> memory { device_type = "memory"; reg = <0 0>; };
>
> ... then any .dts which includes that already has the device_type
> property set, so there's no need to repeat that property. Subsequent
> changes to /memory/reg have no impact on /memory/device_type; any new
> node definitions simply over-write any previous definitions of a
> redefined property, but leave unmentioned properties unchanged (unless
> you /delete-property/).
>
> If skeleton.dtsi were changed to remove that property then yes a lot of
> files would then need to set it, but why would it be removed?
Thank you for the explanation.
All of the device trees with memory@ nodes do explicitly specify the
device_type.
I do still find the setup (as opposed to the mechanism) somewhat
counterintuituve. The effect is pretty much that of a potentially
architecture-specific magic keyword.
/
Leif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
"magic" handling of memory nodes
Hi,
Following on the special handling of nodes called memory@0, I went to
have a look at the various platforms that do not actually declare a
device_type = "memory" for their "memory" nodes.
Firstly, we currently have 162(ish, I did a sloppy grep) such .dts{i}
files in the kernel tree.
Secondly, the only reason these platforms could ever have worked is
because they include .dtsi files that define a memory node with a
type explicitly set. Since this node already exists, its contents get
overridden, but the type tag remains. Of course, this only happens
with nodes called explicitly "memory" - but it happens regardless of
what other things they contain.
In the ARM tree, most of these seem to stem from the inclusion of
skeleton.dtsi.
I don't really know what could/should be done about this, but it
does not feel optimal.
/
Leif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only
On Wed, Apr 23, 2014 at 02:10:58PM +0100, Grant Likely wrote:
> > Does anyone have a LongTrail DT to hand, and if so does the root have a
> > compatible string? From grepping through the kernel I could only find a
> > model string ("IBM,LongTrail").
>
> Actually, on LongTrail this can be removed from the common code
> entirely. It has real open firmware and PowerPC already has the
> infrastructure for fixing up the device tree.
>
> Here's a draft patch that I've compile tested, but nothing else.
I would certainly be happy with that.
Consider my 3/3 withdrawn.
And if the kernel proper will stop honoring nodes with no type,
there is no need for the stub to treat those specially either.
/
Leif
> g.
>
> ---
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 078145acf7fb..18b2c3fee98f 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -2587,9 +2587,18 @@ static void __init fixup_device_tree_chrp(void)
> phandle ph;
> u32 prop[6];
> u32 rloc = 0x01006000; /* IO space; PCI device = 12 */
> - char *name;
> + char *name, strprop[16];
> int rc;
>
> + /* Deal with missing device_type in LongTrail memory node */
> + name = "/memory@0";
> + ph = call_prom("finddevice", 1, 1, ADDR(name));
> + rc = prom_getprop(ph, "device_type", strprop, sizeof(strprop));
> + if (rc == PROM_ERROR || strcmp(strprop, "memory") != 0) {
> + prom_printf("Fixing up missing device_type on /memory@0
> node...\n");
> + prom_setprop(ph, name, "device_type", "memory",
> sizeof("memory"));
> + }
> +
> name = "/pci@8000/isa@c";
> ph = call_prom("finddevice", 1, 1, ADDR(name));
> if (!PHANDLE_VALID(ph)) {
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 7a2ef7bb8022..7cda0d279cbe 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -886,14 +886,7 @@ int __init early_init_dt_scan_memory(unsigned long node,
> const char *uname,
> unsigned long l;
>
> /* We are scanning "memory" nodes only */
> - if (type == NULL) {
> - /*
> - * The longtrail doesn't have a device_type on the
> - * /memory node, so look for the node called /memory@0.
> - */
> - if (depth != 1 || strcmp(uname, "memory@0") != 0)
> - return 0;
> - } else if (strcmp(type, "memory") != 0)
> + if (!type || strcmp(type, "memory") != 0)
> return 0;
>
> reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only
On Wed, Apr 23, 2014 at 02:15:08PM +0100, Grant Likely wrote: > > The reason for me doing that is because we (including you) agreed at > > the discussion held during LCU13 that this was the safest way of > > preventing "mischief" like userland trying to read information from > > /proc/device-tree... > > I'm not the most consistent of people. I often change my mind and then > have no recollection of ever thinking differently. And that is fine, but you were not the only person agreeing. > Userland reading from /proc/device-tree isn't so much a problem, but > kernel drivers doing it might be. > > But, regardless of whether or not the stub clears out the memory > nodes, it is still I think good practice for the kernel to make the > decision to ignore memory nodes, and not rely on them being cleared > correctly. I also remember you saying that relaxing restrictions later on is a lot easier than tightening them. On that basis, can we please get the UEFI set merged before we start redefining the stub/kernel protocol which was agreed at LCU (November last year) after spending a month or two trying to get sufficient number of interested parties in the same room? / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only
On Tue, Apr 22, 2014 at 02:08:29PM +0100, Grant Likely wrote: > > I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here. > > I completely agree. OK. So do we keep this around on unaffected architectures in perpetuity? Or can there be some cut-off date when the majority of DT-enabled platforms can stop including this workaround which permits new incorrect DTs to be implemented so long as they are incorrect in this particular way? > > Really, I would like to see quirks like this fixed up out of line from > > the normal flow somewhat like how PCI quirks are handled. So in this > > example, we would just add the missing property to the dtb for the > > broken platform before doing the memory scan. That does then require > > libfdt which is something I'm working on. > > In fact, for Leif's purposes, I would rather have a flag when booting via > UEFI, and make the kernel skip the memory node parsing if the UEFI > memory map is available. Then the stub doesn't need to do any munging of > the DTB at all. The reason for me doing that is because we (including you) agreed at the discussion held during LCU13 that this was the safest way of preventing "mischief" like userland trying to read information from /proc/device-tree... / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only
On Fri, Apr 18, 2014 at 04:28:17PM -0500, Rob Herring wrote: > >> > Apart from the current code permitting recreating a 15+ year old > >> > firmware bug into completely new platform ports? > >> > >> I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here. > > > > In addition to, or instead of, the QUIRK ifdef? > > Instead of because I don't see how you handle the ARM board > compatibility with the ifdef. (And please, no ifdef for that board). Umm, according to my memory as well as my sent mail folder, I cc:d you on v2 of part 3. Could you have a look at that, please? A WARN_ON would still mean this ancient workaround for a specific ppc32 platform remains enabled on ~10 architectures that don't use it. > >> Really, I would like to see quirks like this fixed up out of line from > >> the normal flow somewhat like how PCI quirks are handled. So in this > >> example, we would just add the missing property to the dtb for the > >> broken platform before doing the memory scan. That does then require > >> libfdt which is something I'm working on. > > > > Getting rid of all this handling from generic code would clearly be > > preferable. Is that code going in in the near future, or could we add > > the quirk as a stopgap? > > Some sort of quirk infrastructure is not going to happen soon. It's > just an idea bouncing in my head ATM. Mmm... > > What would be the effect of the UEFI code adding all its memblocks, > > minus the reserved areas, and then the DT code doing a memblock_add > > of all RAM (including reserved areas)? Would memblock_reserve()s on > > the protected regions suffice to prevent crazy stuff from happening? > > So use UEFI to add the memory, but then add reserved areas with DT? No, to add memory and reserved areas based on UEFI memory map. And then add any memory@0/!type nodes as well, if they're left around. > I'm not sure I follow, but even if I did I don't know memblock code > well enough to say what it would do. If we did end up with stray memory@0/!type nodes, we could initialise memblock multiple times with overlapping but incompatible areas. And I don't know if that would be a problem. Which makes me a little bit nervous. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only
On Fri, Apr 18, 2014 at 10:37:58AM -0500, Rob Herring wrote: > >> But why do you need this? > > > > Apart from the current code permitting recreating a 15+ year old > > firmware bug into completely new platform ports? > > I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here. In addition to, or instead of, the QUIRK ifdef? > Really, I would like to see quirks like this fixed up out of line from > the normal flow somewhat like how PCI quirks are handled. So in this > example, we would just add the missing property to the dtb for the > broken platform before doing the memory scan. That does then require > libfdt which is something I'm working on. Getting rid of all this handling from generic code would clearly be preferable. Is that code going in in the near future, or could we add the quirk as a stopgap? > > Because the UEFI stub for arm/arm64 needs to delete all of the "memory" > > nodes from the DT. And it would be nice to at least not have to compile > > the "and also delete anything called memory@0" into the arm64 image. Or > > any image not including support for affected platforms. > > I don't see why you would handle that in the EFI stub. Given our lack > of validation, I can see there is a chance this happens but I think it > is pretty small. Given we only have a ARM board, I'd say we are doing > surprisingly well. I'm not too bothered personally, but Mark Rutland handed me a patch to improve the memory node handling in the stub, and he seemed to really want this there. You guys can fight it out :) What would be the effect of the UEFI code adding all its memblocks, minus the reserved areas, and then the DT code doing a memblock_add of all RAM (including reserved areas)? Would memblock_reserve()s on the protected regions suffice to prevent crazy stuff from happening? / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only
Hi Geert,
On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm
> wrote:
> > In order to deal with an firmware bug on a specific ppc32 platform
> > (longtrail), early_init_dt_scan_memory() looks for a node called
> > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
>
> This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
> where you added the missing property in patches 1 and 2 of the series)?
As Rob said in response to 0/3, the MIPSs would likely not be affected,
since they embed the DT.
> For the Longtrail, I don't care much anymore, as mine died in 2004.
> AFAIK, there have never been many users anyway.
There are still a few mentions of it under arch/powerpc/, so I wouldn't
want to be the one to kill it off...
How about the below v2 3/3 to address the ARM platform?
Regards,
Leif
>From 6fa0b837ad71780334eb97d63c507165b6c57add Mon Sep 17 00:00:00 2001
From: Leif Lindholm
Date: Thu, 17 Apr 2014 14:24:47 +0100
Subject: [PATCH] of: arm: powerpc: Restrict memory@0 node handling to
affected platforms
In order to deal with a firmware bug on a specific ppc32 platform
(longtrail), early_init_dt_scan_memory() looks for a node called
memory@0 on all platforms, for all nodes lacking a device_type.
Restrict this quirk to ppc32 and the arm mach-ux500 platforms (one of
which has depended on this special handling).
Signed-off-by: Leif Lindholm
Cc: Grant Likely
Cc: Lee Jones
Cc: Mark Rutland
Cc: devicetree@vger.kernel.org
---
arch/arm/mach-ux500/Kconfig |1 +
arch/powerpc/Kconfig|1 +
drivers/of/Kconfig |3 +++
drivers/of/fdt.c| 10 +-
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index b41a42d..e6b0c3b 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -13,6 +13,7 @@ config ARCH_U8500
select CLKSRC_NOMADIK_MTU
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if SMP
+ select OF_MEMORY_AT_0_QUIRK
select PINCTRL
select PINCTRL_ABX500
select PINCTRL_NOMADIK
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e099899..d78452d 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -3,6 +3,7 @@ source "arch/powerpc/platforms/Kconfig.cputype"
config PPC32
bool
default y if !PPC64
+ select OF_MEMORY_AT_0_QUIRK
config 32BIT
bool
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 889005f..230c747 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -77,4 +77,7 @@ config OF_RESERVED_MEM
help
Helpers to allow for reservation of memory regions
+config OF_MEMORY_AT_0_QUIRK
+ def_bool n
+
endmenu # OF
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index fa16a91..1b80b94 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -887,14 +887,22 @@ int __init early_init_dt_scan_memory(unsigned long node,
const char *uname,
/* We are scanning "memory" nodes only */
if (type == NULL) {
+#ifdef CONFIG_OF_MEMORY_AT_0_QUIRK
/*
* The longtrail doesn't have a device_type on the
* /memory node, so look for the node called /memory@0.
+* Converted to generic quirk to handle later platforms
+* with inforrect DTs that work only because of this
+* special handling.
*/
if (depth != 1 || strcmp(uname, "memory@0") != 0)
return 0;
- } else if (strcmp(type, "memory") != 0)
+#else
+ return 0;
+#endif
+ } else if (strcmp(type, "memory") != 0) {
return 0;
+ }
reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
if (reg == NULL)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only
On Thu, Apr 17, 2014 at 07:43:13PM -0500, Rob Herring wrote: > On Thu, Apr 17, 2014 at 12:41 PM, Leif Lindholm > wrote: > > drivers/of/fdt.c contains a workaround for a missing memory type > > entry on longtrail firmware. Make that quirk PPC32 only, and while > > at it - fix up the .dts files in the tree currently working only > > because of that quirk. > > But why do you need this? Apart from the current code permitting recreating a 15+ year old firmware bug into completely new platform ports? Because the UEFI stub for arm/arm64 needs to delete all of the "memory" nodes from the DT. And it would be nice to at least not have to compile the "and also delete anything called memory@0" into the arm64 image. Or any image not including support for affected platforms. > > arch/arm/boot/dts/ste-ccu8540.dts |1 + > > arch/mips/lantiq/dts/easy50712.dts|1 + > > arch/mips/ralink/dts/mt7620a_eval.dts |1 + > > arch/mips/ralink/dts/rt2880_eval.dts |1 + > > arch/mips/ralink/dts/rt3052_eval.dts |1 + > > arch/mips/ralink/dts/rt3883_eval.dts |1 + > > I'm not worried about these MIPS dts files because they are all > built-in, but you are breaking compatibility with old dtbs for this > ARM board. Yeah, sorry. Sending out a v2 of part 3 shortly. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] of: Handle memory@0 node on PPC32 only
In order to deal with an firmware bug on a specific ppc32 platform
(longtrail), early_init_dt_scan_memory() looks for a node called
memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
Signed-off-by: Leif Lindholm
Cc: linuxppc-...@lists.ozlabs.org
Cc: Grant Likely
Cc: Mark Rutland
Cc: devicetree@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
drivers/of/fdt.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index fa16a91..7368472 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -887,14 +887,19 @@ int __init early_init_dt_scan_memory(unsigned long node,
const char *uname,
/* We are scanning "memory" nodes only */
if (type == NULL) {
+#ifdef CONFIG_PPC32
/*
* The longtrail doesn't have a device_type on the
* /memory node, so look for the node called /memory@0.
*/
if (depth != 1 || strcmp(uname, "memory@0") != 0)
return 0;
- } else if (strcmp(type, "memory") != 0)
+#else
+ return 0;
+#endif
+ } else if (strcmp(type, "memory") != 0) {
return 0;
+ }
reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
if (reg == NULL)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] arm: dts: add device_type="memory" for ste-ccu8540
The current .dts for ste-ccu8540 lacks a 'device_type = "memory"' for
its memory node, relying on an old ppc quirk in order to discover its
memory. Add this, to permit that quirk to be made ppc only.
Signed-off-by: Leif Lindholm
Cc: linux-arm-ker...@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: Mark Rutland
Cc: Lee Jones
---
arch/arm/boot/dts/ste-ccu8540.dts |1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/ste-ccu8540.dts
b/arch/arm/boot/dts/ste-ccu8540.dts
index 7f3baf5..32dd55e 100644
--- a/arch/arm/boot/dts/ste-ccu8540.dts
+++ b/arch/arm/boot/dts/ste-ccu8540.dts
@@ -18,6 +18,7 @@
compatible = "st-ericsson,ccu8540", "st-ericsson,u8540";
memory@0 {
+ device_type = "memory";
reg = <0x2000 0x1f00>, <0xc000 0x3f00>;
};
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] mips: dts: add device_type="memory" where missing
A few platforms lack a 'device_type = "memory"' for their memory
nodes, relying on an old ppc quirk in order to discover its memory.
Add this, to permit that quirk to be made ppc only.
Signed-off-by: Leif Lindholm
Cc: linux-m...@linux-mips.org
Cc: devicetree@vger.kernel.org
Cc: John Crispin
Cc: Mark Rutland
---
arch/mips/lantiq/dts/easy50712.dts|1 +
arch/mips/ralink/dts/mt7620a_eval.dts |1 +
arch/mips/ralink/dts/rt2880_eval.dts |1 +
arch/mips/ralink/dts/rt3052_eval.dts |1 +
arch/mips/ralink/dts/rt3883_eval.dts |1 +
5 files changed, 5 insertions(+)
diff --git a/arch/mips/lantiq/dts/easy50712.dts
b/arch/mips/lantiq/dts/easy50712.dts
index fac1f5b..143b8a3 100644
--- a/arch/mips/lantiq/dts/easy50712.dts
+++ b/arch/mips/lantiq/dts/easy50712.dts
@@ -8,6 +8,7 @@
};
memory@0 {
+ device_type = "memory";
reg = <0x0 0x200>;
};
diff --git a/arch/mips/ralink/dts/mt7620a_eval.dts
b/arch/mips/ralink/dts/mt7620a_eval.dts
index 35eb874..709f581 100644
--- a/arch/mips/ralink/dts/mt7620a_eval.dts
+++ b/arch/mips/ralink/dts/mt7620a_eval.dts
@@ -7,6 +7,7 @@
model = "Ralink MT7620A evaluation board";
memory@0 {
+ device_type = "memory";
reg = <0x0 0x200>;
};
diff --git a/arch/mips/ralink/dts/rt2880_eval.dts
b/arch/mips/ralink/dts/rt2880_eval.dts
index 322d700..0a685db 100644
--- a/arch/mips/ralink/dts/rt2880_eval.dts
+++ b/arch/mips/ralink/dts/rt2880_eval.dts
@@ -7,6 +7,7 @@
model = "Ralink RT2880 evaluation board";
memory@0 {
+ device_type = "memory";
reg = <0x800 0x200>;
};
diff --git a/arch/mips/ralink/dts/rt3052_eval.dts
b/arch/mips/ralink/dts/rt3052_eval.dts
index 0ac73ea..ec9e9a0 100644
--- a/arch/mips/ralink/dts/rt3052_eval.dts
+++ b/arch/mips/ralink/dts/rt3052_eval.dts
@@ -7,6 +7,7 @@
model = "Ralink RT3052 evaluation board";
memory@0 {
+ device_type = "memory";
reg = <0x0 0x200>;
};
diff --git a/arch/mips/ralink/dts/rt3883_eval.dts
b/arch/mips/ralink/dts/rt3883_eval.dts
index 2fa6b33..e8df21a 100644
--- a/arch/mips/ralink/dts/rt3883_eval.dts
+++ b/arch/mips/ralink/dts/rt3883_eval.dts
@@ -7,6 +7,7 @@
model = "Ralink RT3883 evaluation board";
memory@0 {
+ device_type = "memory";
reg = <0x0 0x200>;
};
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only
drivers/of/fdt.c contains a workaround for a missing memory type entry on longtrail firmware. Make that quirk PPC32 only, and while at it - fix up the .dts files in the tree currently working only because of that quirk. Cc: devicetree@vger.kernel.org Cc: linuxppc-...@lists.ozlabs.org Cc: linux-ker...@vger.kernel.org Cc: Mark Rutland Leif Lindholm (3): arm: dts: add device_type="memory" for ste-ccu8540 mips: dts: add device_type="memory" where missing of: Handle memory@0 node on PPC32 only arch/arm/boot/dts/ste-ccu8540.dts |1 + arch/mips/lantiq/dts/easy50712.dts|1 + arch/mips/ralink/dts/mt7620a_eval.dts |1 + arch/mips/ralink/dts/rt2880_eval.dts |1 + arch/mips/ralink/dts/rt3052_eval.dts |1 + arch/mips/ralink/dts/rt3883_eval.dts |1 + drivers/of/fdt.c |7 ++- 7 files changed, 12 insertions(+), 1 deletion(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ACPI vs DT at runtime
On Tue, Nov 19, 2013 at 02:38:40PM +, Mark Rutland wrote: > > > For that case we will also require a nailed-down boot > > > protocol that allows for either DTB or ACPI. > > > > The latest documentation patch for the "arm/arm64 UEFI boot protocol" > > implies that UEFI on ARM is already capable of passing a DTB to the > > kernel: > > > > "The implementation depends on receiving information about the UEFI > > environment in a Flattened Device Tree (FDT) - so is only available with > > CONFIG_OF." > > > > Maybe we just need to better document it? > > Yes, we just need to document it. Better document it than what is currently in Booting? > As far as I'm aware, there are two ways we might boot the kernel: Nope, just one. > 1) Via the current boot protocol, passing a DTB in a particular > register. This is the only bit relevant to the kernel proper. It does not change with UEFI/ACPI. > 2) As an EFI application. As I understand it in this case the DTB will > be saved in a system table (I may have got the terminology wrong here), > and the EFI stub will need to look it up to pass it to the kernel. This applies only to the stub itself, and relates to dealing with a firmware that provides a preloaded tree. As well as an obscurity relating to _stub_ command line passing. It has also not been included in the patch sets sent out so far. > As long as that's well defined and does not preclude ACPI, then I am > happy. This is already well defined. I'll agree it may not be explicit enough. If we do need any documentation changes, I feel they belong in Booting, to explain that a minimal DT will be used even with ACPI in order _not_ to break the existing boot protocol. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ACPI vs DT at runtime
On Tue, Nov 19, 2013 at 11:35:57AM +, Mark Rutland wrote: > > The UEFI spec pulls in portions of ACPI. I do not know the full extent > > of the interaction between the two, but I know that they are not > > completely decoupled. As you have pointed out we are not experienced > > with ACPI or UEFI, so I don't think we can make statements that one is > > perfectly fine without the other. > > Given Leif's comments it seems that they are decoupled sufficiently to > be considered separately. Well, UEFI should be considered separately from ACPI. I am not convinced it makes sense to consider ACPI for any case that does not also include UEFI. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ACPI vs DT at runtime
Hej, On Mon, Nov 18, 2013 at 03:29:58PM -0800, Olof Johansson wrote: > >> The server guys really want UEFI for their boot protocols, > >> installation managers, etc, etc. That's fine, let them do that, but > >> that doesn't mean we need to bring the same APIs all the way into the > >> kernel. > > > > There is zero dependency on ACPI in the UEFI support code, or indeed in > > UEFI itself. Both runtime services support and stub loader have been > > designed hardware-description agnostic. > > > > Are you saying that we should not support the kernel interfaces to UEFI > > on ARM*, or are you simply mentioning it in passing because it is the > > bit responsible for populating the pointer to the ACPI tables? > > Good question. UEFI and ACPI usually gets grouped together when they > really are separate (even though ACPI _without_ UEFI is highly > unlikely). > > So, to clarify: What I meant with the above is that UEFI as a > bootloader is fine as far as I am concerned. I'm also in general ok > with the introduction of efivars that you're doing, etc. Thank you for this clarification. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ACPI vs DT at runtime
Hi Olof, Just in case this thread fails to reach its predicted triple-digits, I would like to revisit something you mentioned in this original email and then didn't expand on. On Thu, Nov 14, 2013 at 05:44:10PM -0800, Olof Johansson wrote: > The more I start to see early UEFI/ACPI code, the more I am certain > that we want none of that crap in the kernel. It's making things > considerably messier, while we're already very busy trying to convert > everything over and enable DT -- we'll be preempting that effort just > to add even more boilerplate everywhere and total progress will be > hurt. > > The server guys really want UEFI for their boot protocols, > installation managers, etc, etc. That's fine, let them do that, but > that doesn't mean we need to bring the same APIs all the way into the > kernel. There is zero dependency on ACPI in the UEFI support code, or indeed in UEFI itself. Both runtime services support and stub loader have been designed hardware-description agnostic. Are you saying that we should not support the kernel interfaces to UEFI on ARM*, or are you simply mentioning it in passing because it is the bit responsible for populating the pointer to the ACPI tables? / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: arm/arm64 UEFI boot protocol
On Wed, Nov 13, 2013 at 07:14:28AM +0900, Grant Likely wrote: > In the interest of bike shedding, I would name the properties > "linux,uefi-mmap-start" and "linux,uefi-mmap-size", but otherwise it > is fine by me. That makes sense to me, and makes for a prettier table :) > Should you have a property for the descriptor version > as returned by GetMemoryMap()? Yes, I should, really. I did in an earlier version, but dropped it on realising that this value has not changed since the inception of EFI. But it needs to be passed back to SetVirtualAddressMap(), so it should be kept. Mark never dropped it from his. So, "linux,uefi-mmap-desc-ver"? / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: arm/arm64 UEFI boot protocol
On Tue, Nov 12, 2013 at 04:42:41PM +, Will Deacon wrote: > > So, rather than sending out complete sets of patches for all these until > > consensus is reached, below is my proposed suggestion for a > > Documentation/arm/uefi.txt, to be shared for arm and arm64. If noone has > > strong objections to this, we can quickly send updated (and hopefully > > final-ish) versions of these sets. > > In the absence of code, I'll play English teacher then :) Yey - a reply! > > UEFI, the Unified Extensible Firmware Interface is a specification > > Interface, is ok > > governing the behaviours of compatible firmware interfaces. > > It is maintained by the UEFI Forum - http://www.uefi.org/. > > Support for the AArch32 (arm) architecture was added in version 2.3 of > > the specification, and support for AAaarch64 (arm64) was added in > > version 2.4. > > AArch64 ok > > UEFI is an evolution of its predecessor 'EFI', so the terms EFI and > > EFI are used somewhat interchangeably in this document and associated > > UEFI ok > > UEFI stub > > = > > The "stub" is a feature that turns the Image/zImage/bzImage into a valid > > We don't support bzImage for arm or arm64. Well, it works the same on x86, and the feature is called the same. Once we get the basic support in, I'm planning to write a Documentation/uefi.txt and move all of the non architecture-specific to. > > UEFI PE/COFF executable, including a loader application that makes it > > possible to load the kernel directly from the UEFI shell, boot menu, or > > one of the lightweight bootloaders like Gummiboot or rEFInd. > > The kernel image built with stub support remains a valid kernel image > > for booting in the legacy fashion. > > By legacy, you mean non-UEFI rather than EFI? I consider UEFI as a choice > rather than the future ;p So, "historical"? "Traditional"? Or just "non-UEFI"? > > UEFI kernel support on ARM > > == > > Wait, isn't this all under Documentation/arm/? You could probably combine > some of these sections together, since the scope is really tied to ARM here. Same reason as above, but I could squash for now. > > The implementation depends on receiving information about the UEFI > > environment in a Flattened Device Tree (FDT) - so is only available with > > CONFIG_OF. > > You could mention this in your earlier list of dependencies. Ditto. > > UEFI support also depends on early_memremap(). > > Why is that worthy of note? Can a user even turn that off? Not really. > > The stub populates the FDT /chosen node with, and the kernel scans for > > the following parameters: > > Sentence doesn't read very well. Maybe stick some brackets around (and the > kernel scans for)? ok > > __ > > Name | Size | Description > > > > linux,uefi-system-table | 64-bit | Physical address of the UEFI System > > Table. > > > > linux,uefi-mmap | 64-bit | Physical address of the UEFI memory > > map, > > || populated by the UEFI GetMemoryMap() > > call. > > > > linux,uefi-mmap-size | 32-bit | Size in bytes of the UEFI memory map > > || pointed to in previous entry. > > > > linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI > > || memory map. > > Do we actually need to define these sizes here, or can they be dealt with > using the usual #address-cells property? Also, I think you should describe > the binding in a separate document somewhere under Documentation/devicetree, > then cross-reference it from here. This is not a generic ABI, so I don't think it belongs in there (it will never be in a .dtb, and the only way it can be generated by something that isn't the stub is by lying). But I don't really care either way - as long as some general agreement can be had. > > > > linux,uefi-stub-kern-ver | string | Copy of linux_banner from build. > > > > Are you sure you want to refer to kernel symbols here? If somebody renames > that variable, they're not going to fix this file. There is a comment above the definition of linux_banner that says: /* FIXED STRINGS! Don't touch! */ Sounded reliable enough to me :) / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/ma
arm/arm64 UEFI boot protocol
Hi all, We currently have a few sets of patches floating around, for UEFI runtime services, UEFI stub and GRUB support for Linux/UEFI on arm/arm64. The last version of Documentation/arm/uefi.txt I sent out raised a few questions with regards to the boot protocol between UEFI and Linux, and there is also upcoming support for ACPI which could use a few clarifications in this area. So, rather than sending out complete sets of patches for all these until consensus is reached, below is my proposed suggestion for a Documentation/arm/uefi.txt, to be shared for arm and arm64. If noone has strong objections to this, we can quickly send updated (and hopefully final-ish) versions of these sets. / Leif --- UEFI, the Unified Extensible Firmware Interface is a specification governing the behaviours of compatible firmware interfaces. It is maintained by the UEFI Forum - http://www.uefi.org/. Support for the AArch32 (arm) architecture was added in version 2.3 of the specification, and support for AAaarch64 (arm64) was added in version 2.4. UEFI is an evolution of its predecessor 'EFI', so the terms EFI and EFI are used somewhat interchangeably in this document and associated source code. As a rule, anything new uses 'UEFI', whereas 'EFI' refers to legacy code or specifications. UEFI support in Linux = Booting on a platform with firmware compliant with the UEFI specification makes it possible for the kernel to support additional features: - UEFI Runtime Services - Retrieving various configuration information through the standardised interface of UEFI configuration tables. (ACPI, SMBIOS, ...) For actually enabling [U]EFI support, enable: - CONFIG_EFI=y - CONFIG_EFI_VARS=y or m UEFI stub = The "stub" is a feature that turns the Image/zImage/bzImage into a valid UEFI PE/COFF executable, including a loader application that makes it possible to load the kernel directly from the UEFI shell, boot menu, or one of the lightweight bootloaders like Gummiboot or rEFInd. The kernel image built with stub support remains a valid kernel image for booting in the legacy fashion. UEFI kernel support on ARM == The implementation depends on receiving information about the UEFI environment in a Flattened Device Tree (FDT) - so is only available with CONFIG_OF. UEFI support also depends on early_memremap(). UEFI kernel support on the ARM architectures (arm and arm64) is only available when boot is performed through the stub. When booting in UEFI mode, the kernel ignores any memory nodes in the DT, and instead reads the UEFI memory map. To prevent confusion, the stub deletes any memory nodes from a provided DT. The stub populates the FDT /chosen node with, and the kernel scans for the following parameters: Name | Size | Description linux,uefi-system-table | 64-bit | Physical address of the UEFI System Table. linux,uefi-mmap | 64-bit | Physical address of the UEFI memory map, || populated by the UEFI GetMemoryMap() call. linux,uefi-mmap-size | 32-bit | Size in bytes of the UEFI memory map || pointed to in previous entry. linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI || memory map. linux,uefi-stub-kern-ver | string | Copy of linux_banner from build. For verbose debug messages, specify 'uefi_debug' on the kernel command line. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
