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
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 <[email protected]>
---
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/bar Full 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..a36be70 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
const struct of_device_id *matches,
const struct of_device_id **match);
-extern struct device_node *of_find_node_by_path(const char *path);
+extern struct device_node *of_find_node_opts_by_path(const char *path,
+ const char **opts);
+static inline struct device_node *of_find_node_by_path(const char *path)
+{
+ return of_find_node_opts_by_path(path, NULL);
+}
+
extern struct device_node *of_find_node_by_phandle(phandle handle);
extern struct device_node *of_get_parent(const struct device_node *node);
extern struct device_node *of_get_next_parent(struct device_node *node);
@@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
return NULL;
}
+static inline struct device_node *of_find_node_opts_by_path(const char *path,
+ const char **opts)
+{
+ return NULL;
+}
+
static inline struct device_node *of_get_parent(const struct device_node *node)
{
return NULL;
--
1.7.10.4
Support specifying console options (like with console=ttyXN,<options>)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
Signed-off-by: Leif Lindholm <[email protected]>
---
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
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 http://www.devicetree.org, and part of the
remaining text provided by Mark Rutland.
Signed-off-by: Leif Lindholm <[email protected]>
---
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 0000000..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:
+
+ <baud>{<parity>{<bits>{<flow>}}}
+
+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
On Thu, Nov 27, 2014 at 05:56:05PM +0000, Leif Lindholm wrote:
> 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 http://www.devicetree.org, and part of the
> remaining text provided by Mark Rutland.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> 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 0000000..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:
Please drop the mention of uart_parse_options and just describe the
format. Linux internal details are irrelevant to the contract of the
binding.
Otherwise this looks good to me!
Thanks,
Mark.
> +
> + <baud>{<parity>{<bits>{<flow>}}}
> +
> +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
>
>
On Thu, 27 Nov 2014 18:41:33 +0000
, Mark Rutland <[email protected]>
wrote:
> On Thu, Nov 27, 2014 at 05:56:05PM +0000, Leif Lindholm wrote:
> > 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 http://www.devicetree.org, and part of the
> > remaining text provided by Mark Rutland.
> >
> > Signed-off-by: Leif Lindholm <[email protected]>
> > ---
> > 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 0000000..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:
>
> Please drop the mention of uart_parse_options and just describe the
> format. Linux internal details are irrelevant to the contract of the
> binding.
>
> Otherwise this looks good to me!
I've fixed it up and merged it. No need to respin. Thanks!
g.
On Thu, 27 Nov 2014 17:56:06 +0000
, Leif Lindholm <[email protected]>
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 <[email protected]>
> ---
> 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/bar Full 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.
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.
Otherwise the patch looks good. If it wasn't for the above ambiguity I
would merge it right now.
g.
> /* 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..a36be70 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
> const struct of_device_id *matches,
> const struct of_device_id **match);
>
> -extern struct device_node *of_find_node_by_path(const char *path);
> +extern struct device_node *of_find_node_opts_by_path(const char *path,
> + const char **opts);
> +static inline struct device_node *of_find_node_by_path(const char *path)
> +{
> + return of_find_node_opts_by_path(path, NULL);
> +}
> +
> extern struct device_node *of_find_node_by_phandle(phandle handle);
> extern struct device_node *of_get_parent(const struct device_node *node);
> extern struct device_node *of_get_next_parent(struct device_node *node);
> @@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
> return NULL;
> }
>
> +static inline struct device_node *of_find_node_opts_by_path(const char *path,
> + const char **opts)
> +{
> + return NULL;
> +}
> +
> static inline struct device_node *of_get_parent(const struct device_node *node)
> {
> return NULL;
> --
> 1.7.10.4
>
On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> On Thu, 27 Nov 2014 17:56:06 +0000
> , Leif Lindholm <[email protected]>
> 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 <[email protected]>
> > ---
> > 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/bar Full 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 <[email protected]>
Date: Thu, 27 Nov 2014 09:24:31 +0000
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
following the ':' separator.
4: Add tests.
Signed-off-by: Leif Lindholm <[email protected]>
---
drivers/of/base.c | 21 +++++++++++++++++----
drivers/of/selftest.c | 17 +++++++++++++++++
include/linux/of.h | 14 +++++++++++++-
3 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..99f0fd9 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/bar Full 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 (opts)
+ *opts = separator ? separator + 1 : NULL;
+
/* 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..721b2a4 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,22 @@ 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);
+
+ options = "testoption";
+ np = of_find_node_opts_by_path("testcase-alias", &options);
+ selftest(np && !options, "option clearing 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..a36be70 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
const struct of_device_id *matches,
const struct of_device_id **match);
-extern struct device_node *of_find_node_by_path(const char *path);
+extern struct device_node *of_find_node_opts_by_path(const char *path,
+ const char **opts);
+static inline struct device_node *of_find_node_by_path(const char *path)
+{
+ return of_find_node_opts_by_path(path, NULL);
+}
+
extern struct device_node *of_find_node_by_phandle(phandle handle);
extern struct device_node *of_get_parent(const struct device_node *node);
extern struct device_node *of_get_next_parent(struct device_node *node);
@@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
return NULL;
}
+static inline struct device_node *of_find_node_opts_by_path(const char *path,
+ const char **opts)
+{
+ return NULL;
+}
+
static inline struct device_node *of_get_parent(const struct device_node *node)
{
return NULL;
--
1.7.10.4
On Fri, 28 Nov 2014 11:34:28 +0000
, Leif Lindholm <[email protected]>
wrote:
> On Fri, Nov 28, 2014 at 12:44:03AM +0000, 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...
>
> /
> Leif
>
> From 2e1a44e539967d96366d074ae158092900e0c822 Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <[email protected]>
> Date: Thu, 27 Nov 2014 09:24:31 +0000
> 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
> following the ':' separator.
> 4: Add tests.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> @@ -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;
const char *separator.
Thanks,
g.
On Fri, Nov 28, 2014 at 3:25 PM, Grant Likely <[email protected]> wrote:
> On Fri, 28 Nov 2014 11:34:28 +0000
> , Leif Lindholm <[email protected]>
> wrote:
>> On Fri, Nov 28, 2014 at 12:44:03AM +0000, 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...
And by the way, let me say well done on this patch. It is elegantly
implemented within the framework already there.
g.
On Thu, 27 Nov 2014 17:56:07 +0000
, Leif Lindholm <[email protected]>
wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
>
> Example:
> stdout-path = "uart0:115200";
>
> Signed-off-by: Leif Lindholm <[email protected]>
Applied with a slight change below.
Thanks!
> ---
> 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);
I changed this to simply:
return !add_preferred_console(name, index,
kstrdup(of_stdout_options, GFP_KERNEL));
Not a big deal, just makes for a slightly shorter function.
g.
On Fri, Nov 28, 2014 at 03:25:12PM +0000, Grant Likely wrote:
> On Fri, 28 Nov 2014 11:34:28 +0000
> , Leif Lindholm <[email protected]>
> wrote:
> > On Fri, Nov 28, 2014 at 12:44:03AM +0000, 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 <[email protected]>
Date: Fri, 28 Nov 2014 16:27:25 +0000
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 <[email protected]>
---
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
On Fri, Nov 28, 2014 at 4:38 PM, Leif Lindholm <[email protected]> wrote:
> On Fri, Nov 28, 2014 at 03:25:12PM +0000, Grant Likely wrote:
>> On Fri, 28 Nov 2014 11:34:28 +0000
>> , Leif Lindholm <[email protected]>
>> wrote:
>> > On Fri, Nov 28, 2014 at 12:44:03AM +0000, 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!
Fixups merged. Thanks.
g.
>
> And since it's Friday... *cough*
>
> From 5c469dad81961164c444da7d6c4e1c5b1c097aab Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <[email protected]>
> Date: Fri, 28 Nov 2014 16:27:25 +0000
> 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 <[email protected]>
> ---
> 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
>
On 11/27/2014 9:56 AM, Leif Lindholm wrote:
> 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 http://www.devicetree.org, and part of the
> remaining text provided by Mark Rutland.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> 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 0000000..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
The code in patch 3/3 adds the extra options feature to the properties:
stdout-path
linux,stdout-path
stdout [if (IS_ENABLED(CONFIG_PPC) ... ]
> +--------------------
> +
> +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:
> +
> + <baud>{<parity>{<bits>{<flow>}}}
> +
> +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
>
On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <[email protected]> wrote:
> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>> 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 http://www.devicetree.org, and part of the
>> remaining text provided by Mark Rutland.
>>
>> Signed-off-by: Leif Lindholm <[email protected]>
>> ---
>> 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 0000000..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
>
> The code in patch 3/3 adds the extra options feature to the properties:
>
> stdout-path
> linux,stdout-path
> stdout [if (IS_ENABLED(CONFIG_PPC) ... ]
I don't understand what you mean here. Are you suggesting a change to
this patch? Is there something deficient in it?
g.
>
>> +--------------------
>> +
>> +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:
>> +
>> + <baud>{<parity>{<bits>{<flow>}}}
>> +
>> +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
>>
>
On 12/3/2014 7:12 AM, Grant Likely wrote:
> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <[email protected]> wrote:
>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>>> 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 http://www.devicetree.org, and part of the
>>> remaining text provided by Mark Rutland.
>>>
>>> Signed-off-by: Leif Lindholm <[email protected]>
>>> ---
>>> 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 0000000..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
>>
>> The code in patch 3/3 adds the extra options feature to the properties:
>>
>> stdout-path
>> linux,stdout-path
>> stdout [if (IS_ENABLED(CONFIG_PPC) ... ]
>
> I don't understand what you mean here. Are you suggesting a change to
> this patch? Is there something deficient in it?
Assuming that the code change in patch 3 is as desired, then the chosen.txt
documentation applies to all three properties, not just stdout-path. So
yes, a change is suggested for the documentation.
-Frank
On Wed, Dec 3, 2014 at 7:46 PM, Frank Rowand <[email protected]> wrote:
> On 12/3/2014 7:12 AM, Grant Likely wrote:
>> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <[email protected]> wrote:
>>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>>>> 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 http://www.devicetree.org, and part of the
>>>> remaining text provided by Mark Rutland.
>>>>
>>>> Signed-off-by: Leif Lindholm <[email protected]>
>>>> ---
>>>> 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 0000000..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
>>>
>>> The code in patch 3/3 adds the extra options feature to the properties:
>>>
>>> stdout-path
>>> linux,stdout-path
>>> stdout [if (IS_ENABLED(CONFIG_PPC) ... ]
>>
>> I don't understand what you mean here. Are you suggesting a change to
>> this patch? Is there something deficient in it?
>
> Assuming that the code change in patch 3 is as desired, then the chosen.txt
> documentation applies to all three properties, not just stdout-path. So
> yes, a change is suggested for the documentation.
how about if this is added:
Implementation note: Linux will look for the property "linux,stdout-path" or
on PowerPC "stdout" if "stdout-path" is not found. However, the
"linux,stdout-path" and "stdout" properties are deprecated. New platforms
should only use the "stdout-path" property.
g.
On 12/3/2014 1:45 PM, Grant Likely wrote:
> On Wed, Dec 3, 2014 at 7:46 PM, Frank Rowand <[email protected]> wrote:
>> On 12/3/2014 7:12 AM, Grant Likely wrote:
>>> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <[email protected]> wrote:
>>>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>>>>> 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 http://www.devicetree.org, and part of the
>>>>> remaining text provided by Mark Rutland.
>>>>>
>>>>> Signed-off-by: Leif Lindholm <[email protected]>
>>>>> ---
>>>>> 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 0000000..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
>>>>
>>>> The code in patch 3/3 adds the extra options feature to the properties:
>>>>
>>>> stdout-path
>>>> linux,stdout-path
>>>> stdout [if (IS_ENABLED(CONFIG_PPC) ... ]
>>>
>>> I don't understand what you mean here. Are you suggesting a change to
>>> this patch? Is there something deficient in it?
>>
>> Assuming that the code change in patch 3 is as desired, then the chosen.txt
>> documentation applies to all three properties, not just stdout-path. So
>> yes, a change is suggested for the documentation.
>
> how about if this is added:
>
> Implementation note: Linux will look for the property "linux,stdout-path" or
> on PowerPC "stdout" if "stdout-path" is not found. However, the
> "linux,stdout-path" and "stdout" properties are deprecated. New platforms
> should only use the "stdout-path" property.
>
> g.
>
Yep, perfect.
-Frank
On Wed, 03 Dec 2014 15:07:49 -0800
, Frank Rowand <[email protected]>
wrote:
> On 12/3/2014 1:45 PM, Grant Likely wrote:
> > On Wed, Dec 3, 2014 at 7:46 PM, Frank Rowand <[email protected]> wrote:
> >> On 12/3/2014 7:12 AM, Grant Likely wrote:
> >>> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <[email protected]> wrote:
> >>>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
> >>>>> 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 http://www.devicetree.org, and part of the
> >>>>> remaining text provided by Mark Rutland.
> >>>>>
> >>>>> Signed-off-by: Leif Lindholm <[email protected]>
> >>>>> ---
> >>>>> 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 0000000..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
> >>>>
> >>>> The code in patch 3/3 adds the extra options feature to the properties:
> >>>>
> >>>> stdout-path
> >>>> linux,stdout-path
> >>>> stdout [if (IS_ENABLED(CONFIG_PPC) ... ]
> >>>
> >>> I don't understand what you mean here. Are you suggesting a change to
> >>> this patch? Is there something deficient in it?
> >>
> >> Assuming that the code change in patch 3 is as desired, then the chosen.txt
> >> documentation applies to all three properties, not just stdout-path. So
> >> yes, a change is suggested for the documentation.
> >
> > how about if this is added:
> >
> > Implementation note: Linux will look for the property "linux,stdout-path" or
> > on PowerPC "stdout" if "stdout-path" is not found. However, the
> > "linux,stdout-path" and "stdout" properties are deprecated. New platforms
> > should only use the "stdout-path" property.
> >
> > g.
> >
>
> Yep, perfect.
Great, thanks. Merged.
g.
On 11/27/2014 12:56 PM, Leif Lindholm wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
>
> Example:
> stdout-path = "uart0:115200";
This format breaks DT earlycon because libfdt doesn't recognize ':' as a
path terminator.
It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
but perhaps it would better to teach fdt_path_offset() to recognize ':'?
Regards,
Peter Hurley
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> 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);
>
>
On Thu, Feb 26, 2015 at 06:55:22AM -0500, Peter Hurley wrote:
> On 11/27/2014 12:56 PM, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
>
> This format breaks DT earlycon because libfdt doesn't recognize ':' as a
> path terminator.
>
> It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
> but perhaps it would better to teach fdt_path_offset() to recognize ':'?
Hi Peter
ePAPR says for stdout-path in Chosen:
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.
So it is probably wise to implement this properly.
Andrew
Hi Andrew,
On 02/26/2015 08:46 AM, Andrew Lunn wrote:
> On Thu, Feb 26, 2015 at 06:55:22AM -0500, Peter Hurley wrote:
>> On 11/27/2014 12:56 PM, Leif Lindholm wrote:
>>> Support specifying console options (like with console=ttyXN,<options>)
>>> by appending them to the stdout-path property after a separating ':'.
>>>
>>> Example:
>>> stdout-path = "uart0:115200";
>>
>> This format breaks DT earlycon because libfdt doesn't recognize ':' as a
>> path terminator.
>>
>> It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
>> but perhaps it would better to teach fdt_path_offset() to recognize ':'?
>
> Hi Peter
>
> ePAPR says for stdout-path in Chosen:
>
> 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.
>
> So it is probably wise to implement this properly.
Sorry if I was unclear. My question was not _whether_ to fix this
for earlycon, but rather _how_.
IOW, is the ':' character accepted as a path terminator for
1. all nodes, so fix this in fdt_path_offset();
2. only chosen nodes;
3. unique to stdout-path, so fix this in early_init_dt_scan_chosen_serial()?
Regards,
Peter Hurley
> Sorry if I was unclear. My question was not _whether_ to fix this
> for earlycon, but rather _how_.
>
> IOW, is the ':' character accepted as a path terminator for
> 1. all nodes, so fix this in fdt_path_offset();
> 2. only chosen nodes;
> 3. unique to stdout-path, so fix this in early_init_dt_scan_chosen_serial()?
Ah, sorry.
The ':' character is accepted as a path terminator for stdout-path and
stdin-path within chosen node. It does not appear to be mentioned
anywhere else in the standard.
Andrew