state fixes/clarifications

state fixes/clarifications

[Sascha Hauer]

There are some common pitfalls when using the barebox state framework.
This series has some fixes clarifications to hopefully make usage easier.
First of all it doesn't seem to be clear that the state node in the device
tree the Kernel is started with is created by barebox; a previously existing
node will be replaced. For this reason we now issue a warning when a device
tree contains a state node which does not have any effect.
The userspace barebox-state utility needs a alias which for the state node
(it may work with a full path to the node, like /state, but this has some
additional surprises). We therefore make the alias mandatory so that we
can issue a clear error message when no alias exists and we can make
sure barebox-state will find an alias.

----------------------------------------------------------------
Sascha Hauer (4):
      state: Binding: remove @0 from node name
      state: warn when a state node will be overwritten
      state: Make an alias mandatory
      state: Create alias in of_state_fixup()

 .../devicetree/bindings/barebox/barebox,state.rst  |  8 ++++++-
 common/state/state.c                               | 28 +++++++++++++++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/4] state: Binding: remove @0 from node name

[Sascha Hauer]

The '@0' suffix is for nodes which have a reg property, so
do not use this suffix in the state example node which does
not have a reg property.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/devicetree/bindings/barebox/barebox,state.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/barebox/barebox,state.rst b/Documentation/devicetree/bindings/barebox/barebox,state.rst
index 00fb592614..51b874a8be 100644
--- a/Documentation/devicetree/bindings/barebox/barebox,state.rst
+++ b/Documentation/devicetree/bindings/barebox/barebox,state.rst
@@ -74,7 +74,7 @@ Optional properties:
 
 Example::
 
-  state: state@0 {
+  state: state {
   	magic = <0x27031977>;
   	compatible = "barebox,state";
   	backend-type = "raw";
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/4] state: warn when a state node will be overwritten

[Sascha Hauer]

People do not seem to know that a state node in the kernel
device tree will be overwritten by barebox. Print a warning
when this happens so that people can get an idea why changes
in the kernel device tree state node do not have any effect.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/state/state.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/common/state/state.c b/common/state/state.c
index 6db77f2c18..c3c8b768c7 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -416,6 +416,15 @@ static int of_state_fixup(struct device_node *root, void *ctx)
 	if (node) {
 		/* replace existing node - it will be deleted later */
 		parent = node->parent;
+
+		/*
+		 * barebox replaces the state node in the device tree it starts the
+		 * kernel with, so a state node that already exists in the device tree
+		 * will be overwritten. Warn about this so people do not wonder why
+		 * changes in the kernels state node do not have any effect.
+		 */
+		if (root != of_get_root_node())
+			dev_warn(&state->dev, "Warning: Kernel devicetree contains state node, replacing it\n");
 	} else {
 		char *of_path, *c;
 
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/4] state: warn when a state node will be overwritten

[Uwe Kleine-König]

On Thu, May 04, 2017 at 01:02:17PM +0200, Sascha Hauer wrote:
> People do not seem to know that a state node in the kernel
> device tree will be overwritten by barebox. Print a warning
> when this happens so that people can get an idea why changes
> in the kernel device tree state node do not have any effect.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  common/state/state.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/common/state/state.c b/common/state/state.c
> index 6db77f2c18..c3c8b768c7 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -416,6 +416,15 @@ static int of_state_fixup(struct device_node *root, void *ctx)
>  	if (node) {
>  		/* replace existing node - it will be deleted later */
>  		parent = node->parent;
> +
> +		/*
> +		 * barebox replaces the state node in the device tree it starts the
> +		 * kernel with, so a state node that already exists in the device tree
> +		 * will be overwritten. Warn about this so people do not wonder why
> +		 * changes in the kernels state node do not have any effect.
> +		 */
> +		if (root != of_get_root_node())
> +			dev_warn(&state->dev, "Warning: Kernel devicetree contains state node, replacing it\n");

Would it make sense to only issue this warning if there are differences
between the replacing and the replaced node? Otherwise I think this
warning will trigger in many cases where it doesn't really matter. At
least in my projects I take care to have only minimal differences
between barebox' and linux' device tree and so the latter usually also
contains the state stuff.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/4] state: warn when a state node will be overwritten

[Sascha Hauer]

On Mon, May 08, 2017 at 08:15:25AM +0200, Uwe Kleine-König wrote:
> On Thu, May 04, 2017 at 01:02:17PM +0200, Sascha Hauer wrote:
> > People do not seem to know that a state node in the kernel
> > device tree will be overwritten by barebox. Print a warning
> > when this happens so that people can get an idea why changes
> > in the kernel device tree state node do not have any effect.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  common/state/state.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/common/state/state.c b/common/state/state.c
> > index 6db77f2c18..c3c8b768c7 100644
> > --- a/common/state/state.c
> > +++ b/common/state/state.c
> > @@ -416,6 +416,15 @@ static int of_state_fixup(struct device_node *root, void *ctx)
> >  	if (node) {
> >  		/* replace existing node - it will be deleted later */
> >  		parent = node->parent;
> > +
> > +		/*
> > +		 * barebox replaces the state node in the device tree it starts the
> > +		 * kernel with, so a state node that already exists in the device tree
> > +		 * will be overwritten. Warn about this so people do not wonder why
> > +		 * changes in the kernels state node do not have any effect.
> > +		 */
> > +		if (root != of_get_root_node())
> > +			dev_warn(&state->dev, "Warning: Kernel devicetree contains state node, replacing it\n");
> 
> Would it make sense to only issue this warning if there are differences
> between the replacing and the replaced node? Otherwise I think this
> warning will trigger in many cases where it doesn't really matter. At
> least in my projects I take care to have only minimal differences
> between barebox' and linux' device tree and so the latter usually also
> contains the state stuff.

Generally I'm fine with showing the warning only when there are
differences. However, we do not have any code to compare two device tree
branches.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/4] state: Make an alias mandatory

[Sascha Hauer]

The userspace barebox-state utility gets confused when no alias exists.
Make the alias mandatory, so that people make it right^tm without having
to ask.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/devicetree/bindings/barebox/barebox,state.rst | 6 ++++++
 common/state/state.c                                        | 6 ++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/barebox/barebox,state.rst b/Documentation/devicetree/bindings/barebox/barebox,state.rst
index 51b874a8be..06a0d100c8 100644
--- a/Documentation/devicetree/bindings/barebox/barebox,state.rst
+++ b/Documentation/devicetree/bindings/barebox/barebox,state.rst
@@ -32,6 +32,8 @@ Required properties:
 * ``backend``: contains a phandle to the device/partition which holds the
   actual state data.
 * ``backend-type``: should be ``raw`` or ``dtb``.
+* additionally a state node must have an alias in the /aliases/ node pointing
+  to it.
 
 Optional properties:
 
@@ -74,6 +76,10 @@ Optional properties:
 
 Example::
 
+  /aliases {
+  	state = &state;
+  };
+
   state: state {
   	magic = <0x27031977>;
   	compatible = "barebox,state";
diff --git a/common/state/state.c b/common/state/state.c
index c3c8b768c7..41bee0fdc5 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -567,8 +567,10 @@ struct state *state_new_from_node(struct device_node *node, char *path,
 	uint32_t stridesize;
 
 	alias = of_alias_get(node);
-	if (!alias)
-		alias = node->name;
+	if (!alias) {
+		pr_err("State node %s does not have an alias in the /aliases/ node\n", node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
 	state = state_new(alias);
 	if (IS_ERR(state))
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/4] state: Create alias in of_state_fixup()

[Sascha Hauer]

When the kernel device tree is fixed up we assume that it doesn't
have a state node, so we must also assume that it doesn't have
a alias. Create it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/state/state.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/common/state/state.c b/common/state/state.c
index 41bee0fdc5..b4a634fa99 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -407,7 +407,7 @@ static int of_state_fixup(struct device_node *root, void *ctx)
 {
 	struct state *state = ctx;
 	const char *compatible = "barebox,state";
-	struct device_node *new_node, *node, *parent, *backend_node;
+	struct device_node *new_node, *node, *parent, *backend_node, *aliases;
 	struct property *p;
 	int ret;
 	phandle phandle;
@@ -520,6 +520,17 @@ static int of_state_fixup(struct device_node *root, void *ctx)
 	if (ret)
 		goto out;
 
+	aliases = of_create_node(root, "/aliases");
+	if (!aliases) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = of_set_property(aliases, state->name, new_node->full_name,
+			      strlen(new_node->full_name) + 1, 1);
+	if (ret)
+		goto out;
+
 	/* delete existing node */
 	if (node)
 		of_delete_node(node);
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox