Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1657609 > unrolled thread

[PATCH] external references for device tree overlays

Started byStefani Seibold <stefani.seibold.ext@huawei.com>
First post2017-06-05 15:50 +0200
Last post2017-06-12 20:50 +0200
Articles 14 — 5 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] external references for device tree overlays Stefani Seibold <stefani.seibold.ext@huawei.com> - 2017-06-05 15:50 +0200
    Re: [PATCH] external references for device tree overlays Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2017-06-05 20:50 +0200
      Re: [PATCH] external references for device tree overlays Stefani Seibold <stefani@seibold.net> - 2017-06-06 21:40 +0200
        Re: [PATCH] external references for device tree overlays Rob Herring <robh+dt@kernel.org> - 2017-06-07 00:10 +0200
        Re: [PATCH] external references for device tree overlays Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2017-06-07 10:20 +0200
          Re: [PATCH] external references for device tree overlays Rob Herring <robh+dt@kernel.org> - 2017-06-08 00:20 +0200
            Re: [PATCH] external references for device tree overlays Stefani Seibold <stefani@seibold.net> - 2017-06-08 09:00 +0200
          Re: [PATCH] external references for device tree overlays Stefani Seibold <stefani@seibold.net> - 2017-06-08 08:50 +0200
            Re: [PATCH] external references for device tree overlays Frank Rowand <frowand.list@gmail.com> - 2017-06-12 01:10 +0200
    Re: [PATCH] external references for device tree overlays Frank Rowand <frowand.list@gmail.com> - 2017-06-06 09:30 +0200
      Re: [PATCH] external references for device tree overlays Frank Rowand <frowand.list@gmail.com> - 2017-06-06 18:20 +0200
      Re: [PATCH] external references for device tree overlays Stefani Seibold <stefani@seibold.net> - 2017-06-06 21:30 +0200
        Re: [PATCH] external references for device tree overlays Frank Rowand <frowand.list@gmail.com> - 2017-06-07 02:50 +0200
          Re: [PATCH] external references for device tree overlays Frank Rowand <frowand.list@gmail.com> - 2017-06-12 20:50 +0200

#1657609 — [PATCH] external references for device tree overlays

FromStefani Seibold <stefani.seibold.ext@huawei.com>
Date2017-06-05 15:50 +0200
Subject[PATCH] external references for device tree overlays
Message-ID<tP0yn-3wI-17@gated-at.bofh.it>
From: Stefani Seibold <stefani@seibold.net>

This patch enables external references for symbols which are not
exported by the current device tree. For example

// RASPI example (only for testing)
/dts-v1/;
/plugin/;

/ {
    compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";

    fragment@0 {
        target-path = "/soc/i2s@7e203000";
        __overlay__ {
            #address-cells = <0x00000001>;
            #size-cells = <0x00000001>;
            test = "test";
            timer = <&timer>;
        };
    };

    __external_symbols__ {
        timer = "/soc/timer@7e003000";
    };
};

The "timer" symbol is not exported by the RASPI device tree, because it is
missing in the __symbols__ section of the device tree.

In case of the RASPI device tree this could be simple fixed by modifing
the device tree source, but when the device tree is provided by a closed
source BIOS this kind of missing symbol could not be fixed.

An additional benefit is to override a (possible broken) symbol exported
by the currect live device tree.

The patch is based and tested on linux 4.12-rc3.

Signed-off-by: Stefani Seibold <stefani.seibold.ext@huawei.com>
Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/of/overlay.c  | 19 +++++++++++++++++++
 drivers/of/resolver.c | 27 ++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7827786718d8..de6516ea0fcd 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -50,6 +50,7 @@ struct of_overlay {
 	int id;
 	struct list_head node;
 	int count;
+	struct device_node *tree;
 	struct of_overlay_info *ovinfo_tab;
 	struct of_changeset cset;
 };
@@ -422,6 +423,8 @@ int of_overlay_create(struct device_node *tree)
 	/* add to the tail of the overlay list */
 	list_add_tail(&ov->node, &ov_list);
 
+	ov->tree = tree;
+
 	of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
 
 	mutex_unlock(&of_mutex);
@@ -524,6 +527,7 @@ int of_overlay_destroy(int id)
 {
 	struct of_overlay *ov;
 	int err;
+	phandle phandle;
 
 	mutex_lock(&of_mutex);
 
@@ -540,6 +544,8 @@ int of_overlay_destroy(int id)
 		goto out;
 	}
 
+	phandle = ov->tree->phandle;
+
 	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
 	list_del(&ov->node);
 	__of_changeset_revert(&ov->cset);
@@ -549,6 +555,19 @@ int of_overlay_destroy(int id)
 	of_changeset_destroy(&ov->cset);
 	kfree(ov);
 
+	if (phandle) {
+		struct device_node *node;
+		unsigned long flags;
+
+		raw_spin_lock_irqsave(&devtree_lock, flags);
+		for_each_of_allnodes(node) {
+			if (node->phandle >= phandle)
+				node->phandle = 0;
+		}
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	}
+
+
 	err = 0;
 
 out:
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 771f4844c781..31b5f32c9b27 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -286,13 +286,14 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 int of_resolve_phandles(struct device_node *overlay)
 {
 	struct device_node *child, *local_fixups, *refnode;
-	struct device_node *tree_symbols, *overlay_fixups;
+	struct device_node *tree_symbols, *ext_symbols, *overlay_fixups;
 	struct property *prop;
 	const char *refpath;
 	phandle phandle, phandle_delta;
 	int err;
 
 	tree_symbols = NULL;
+	ext_symbols = NULL;
 
 	if (!overlay) {
 		pr_err("null overlay\n");
@@ -321,6 +322,9 @@ int of_resolve_phandles(struct device_node *overlay)
 	for_each_child_of_node(overlay, child) {
 		if (!of_node_cmp(child->name, "__fixups__"))
 			overlay_fixups = child;
+		else
+		if (!of_node_cmp(child->name, "__external_symbols__"))
+			ext_symbols = child;
 	}
 
 	if (!overlay_fixups) {
@@ -329,20 +333,30 @@ int of_resolve_phandles(struct device_node *overlay)
 	}
 
 	tree_symbols = of_find_node_by_path("/__symbols__");
-	if (!tree_symbols) {
-		pr_err("no symbols in root of device tree.\n");
+	if (!tree_symbols && !ext_symbols) {
+		pr_err("no symbols for resolve in device tree.\n");
 		err = -EINVAL;
 		goto out;
 	}
 
+	phandle_delta = live_tree_max_phandle() + 1;
+
 	for_each_property_of_node(overlay_fixups, prop) {
 
 		/* skip properties added automatically */
 		if (!of_prop_cmp(prop->name, "name"))
 			continue;
 
-		err = of_property_read_string(tree_symbols,
-				prop->name, &refpath);
+		err = -1;
+
+		if (ext_symbols)
+			err = of_property_read_string(ext_symbols,
+					prop->name, &refpath);
+
+		if (err && tree_symbols)
+			err = of_property_read_string(tree_symbols,
+					prop->name, &refpath);
+
 		if (err)
 			goto out;
 
@@ -352,6 +366,9 @@ int of_resolve_phandles(struct device_node *overlay)
 			goto out;
 		}
 
+		if (!refnode->phandle)
+			refnode->phandle = ++phandle_delta;
+
 		phandle = refnode->phandle;
 		of_node_put(refnode);
 
-- 
2.13.0

[toc] | [next] | [standalone]

#1658068

FromPantelis Antoniou <pantelis.antoniou@konsulko.com>
Date2017-06-05 20:50 +0200
Message-ID<tP5eF-6xe-5@gated-at.bofh.it>
In reply to#1657609
Hi Stefani,

On Mon, 2017-06-05 at 14:59 +0200, Stefani Seibold wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> This patch enables external references for symbols which are not
> exported by the current device tree. For example
> 
> // RASPI example (only for testing)
> /dts-v1/;
> /plugin/;
> 
> / {
>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> 
>     fragment@0 {
>         target-path = "/soc/i2s@7e203000";
>         __overlay__ {
>             #address-cells = <0x00000001>;
>             #size-cells = <0x00000001>;
>             test = "test";
>             timer = <&timer>;
>         };
>     };
> 
>     __external_symbols__ {
>         timer = "/soc/timer@7e003000";
>     };
> };
> 

I understand the problem. I am just not fond of the __external_symbols__
solution.

There's a facility in the DT source language that allows to declare
pathspec labels.

The 'timer = <&timer>;' statement could be rewritten as 
'timer = <&{/soc/timer@7e0030000}>;'

Internally you can 'catch' that this refers to a symbol in the base tree
and then do the same symbol insertion as the patch you've submitted.

The benefit to the above is that you don't introduce manually edited
special nodes.

Regards

-- Pantelis

> The "timer" symbol is not exported by the RASPI device tree, because it is
> missing in the __symbols__ section of the device tree.
> 
> In case of the RASPI device tree this could be simple fixed by modifing
> the device tree source, but when the device tree is provided by a closed
> source BIOS this kind of missing symbol could not be fixed.
> 
> An additional benefit is to override a (possible broken) symbol exported
> by the currect live device tree.
> 
> The patch is based and tested on linux 4.12-rc3.
> 
> Signed-off-by: Stefani Seibold <stefani.seibold.ext@huawei.com>
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/of/overlay.c  | 19 +++++++++++++++++++
>  drivers/of/resolver.c | 27 ++++++++++++++++++++++-----
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 7827786718d8..de6516ea0fcd 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -50,6 +50,7 @@ struct of_overlay {
>  	int id;
>  	struct list_head node;
>  	int count;
> +	struct device_node *tree;
>  	struct of_overlay_info *ovinfo_tab;
>  	struct of_changeset cset;
>  };
> @@ -422,6 +423,8 @@ int of_overlay_create(struct device_node *tree)
>  	/* add to the tail of the overlay list */
>  	list_add_tail(&ov->node, &ov_list);
>  
> +	ov->tree = tree;
> +
>  	of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
>  
>  	mutex_unlock(&of_mutex);
> @@ -524,6 +527,7 @@ int of_overlay_destroy(int id)
>  {
>  	struct of_overlay *ov;
>  	int err;
> +	phandle phandle;
>  
>  	mutex_lock(&of_mutex);
>  
> @@ -540,6 +544,8 @@ int of_overlay_destroy(int id)
>  		goto out;
>  	}
>  
> +	phandle = ov->tree->phandle;
> +
>  	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
>  	list_del(&ov->node);
>  	__of_changeset_revert(&ov->cset);
> @@ -549,6 +555,19 @@ int of_overlay_destroy(int id)
>  	of_changeset_destroy(&ov->cset);
>  	kfree(ov);
>  
> +	if (phandle) {
> +		struct device_node *node;
> +		unsigned long flags;
> +
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
> +		for_each_of_allnodes(node) {
> +			if (node->phandle >= phandle)
> +				node->phandle = 0;
> +		}
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	}
> +
> +
>  	err = 0;
>  
>  out:
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 771f4844c781..31b5f32c9b27 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -286,13 +286,14 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
>  int of_resolve_phandles(struct device_node *overlay)
>  {
>  	struct device_node *child, *local_fixups, *refnode;
> -	struct device_node *tree_symbols, *overlay_fixups;
> +	struct device_node *tree_symbols, *ext_symbols, *overlay_fixups;
>  	struct property *prop;
>  	const char *refpath;
>  	phandle phandle, phandle_delta;
>  	int err;
>  
>  	tree_symbols = NULL;
> +	ext_symbols = NULL;
>  
>  	if (!overlay) {
>  		pr_err("null overlay\n");
> @@ -321,6 +322,9 @@ int of_resolve_phandles(struct device_node *overlay)
>  	for_each_child_of_node(overlay, child) {
>  		if (!of_node_cmp(child->name, "__fixups__"))
>  			overlay_fixups = child;
> +		else
> +		if (!of_node_cmp(child->name, "__external_symbols__"))
> +			ext_symbols = child;
>  	}
>  
>  	if (!overlay_fixups) {
> @@ -329,20 +333,30 @@ int of_resolve_phandles(struct device_node *overlay)
>  	}
>  
>  	tree_symbols = of_find_node_by_path("/__symbols__");
> -	if (!tree_symbols) {
> -		pr_err("no symbols in root of device tree.\n");
> +	if (!tree_symbols && !ext_symbols) {
> +		pr_err("no symbols for resolve in device tree.\n");
>  		err = -EINVAL;
>  		goto out;
>  	}
>  
> +	phandle_delta = live_tree_max_phandle() + 1;
> +
>  	for_each_property_of_node(overlay_fixups, prop) {
>  
>  		/* skip properties added automatically */
>  		if (!of_prop_cmp(prop->name, "name"))
>  			continue;
>  
> -		err = of_property_read_string(tree_symbols,
> -				prop->name, &refpath);
> +		err = -1;
> +
> +		if (ext_symbols)
> +			err = of_property_read_string(ext_symbols,
> +					prop->name, &refpath);
> +
> +		if (err && tree_symbols)
> +			err = of_property_read_string(tree_symbols,
> +					prop->name, &refpath);
> +
>  		if (err)
>  			goto out;
>  
> @@ -352,6 +366,9 @@ int of_resolve_phandles(struct device_node *overlay)
>  			goto out;
>  		}
>  
> +		if (!refnode->phandle)
> +			refnode->phandle = ++phandle_delta;
> +
>  		phandle = refnode->phandle;
>  		of_node_put(refnode);
>

[toc] | [prev] | [next] | [standalone]

#1659148

FromStefani Seibold <stefani@seibold.net>
Date2017-06-06 21:40 +0200
Message-ID<tPsuB-4DE-3@gated-at.bofh.it>
In reply to#1658068
Hi Pantelis,

thanks for the suggestion. This feature is not very well documented. I
tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
source is:

// rapsi example
/dts-v1/;
/plugin/;

/ {
    compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";

    fragment@0 {
        target-path = "/soc/i2s@7e203000";
        __overlay__ {
            #address-cells = <0x00000001>;
            #size-cells = <0x00000001>;
            test = "test";
            timer = <&{/soc/timer@7e0030000}>;
        };
    };
};


The resulting overlay is (decompiled with fdtdump):

/dts-v1/;
// magic:		0xd00dfeed
// totalsize:		0x19a (410)
// off_dt_struct:	0x38
// off_dt_strings:	0x148
// off_mem_rsvmap:	0x28
// version:		17
// last_comp_version:	16
// boot_cpuid_phys:	0x0
// size_dt_strings:	0x52
// size_dt_struct:	0x110

/ {
    compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
    fragment@0 {
        target-path = "/soc/i2s@7e203000";
        __overlay__ {
            #address-cells = <0x00000001>;
            #size-cells = <0x00000001>;
            test = "test";
            timer = <0xdeadbeef>;
        };
    };
    __fixups__ {
        /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";
    };
};

But this will not apply:

OF: resolver: overlay phandle fixup failed: -22
create_overlay: Failed to resolve tree


Anyway, the reason for my patch is that i can reference to nodes which
lacks a phandle. The phandle will be created on the fly and also
destroyed when the overlay is unloaded.

I have a real use case for this patch:

I have a BIOS on some ARM64 servers which provides broken device tree.
It also lacks some devices in this tree which needs references to other
devices which lacks a phandle.

Since the BIOSes are closed source i need a way to work arround this
problem without patching all the drivers involved to this devices.

Hope this helps to understand the reason for this patch.

- Stefani

Am Montag, den 05.06.2017, 21:43 +0300 schrieb Pantelis Antoniou:
> Hi Stefani,
> 
> On Mon, 2017-06-05 at 14:59 +0200, Stefani Seibold wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > This patch enables external references for symbols which are not
> > exported by the current device tree. For example
> > 
> > // RASPI example (only for testing)
> > /dts-v1/;
> > /plugin/;
> > 
> > / {
> >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> > 
> >     fragment@0 {
> >         target-path = "/soc/i2s@7e203000";
> >         __overlay__ {
> >             #address-cells = <0x00000001>;
> >             #size-cells = <0x00000001>;
> >             test = "test";
> >             timer = <&timer>;
> >         };
> >     };
> > 
> >     __external_symbols__ {
> >         timer = "/soc/timer@7e003000";
> >     };
> > };
> > 
> 
> I understand the problem. I am just not fond of the
> __external_symbols__
> solution.
> 
> There's a facility in the DT source language that allows to declare
> pathspec labels.
> 
> The 'timer = <&timer>;' statement could be rewritten as 
> 'timer = <&{/soc/timer@7e0030000}>;'
> 
> Internally you can 'catch' that this refers to a symbol in the base
> tree
> and then do the same symbol insertion as the patch you've submitted.
> 
> The benefit to the above is that you don't introduce manually edited
> special nodes.
> 
> Regards
> 
> -- Pantelis
> 
> > The "timer" symbol is not exported by the RASPI device tree,
> > because it is
> > missing in the __symbols__ section of the device tree.
> > 
> > In case of the RASPI device tree this could be simple fixed by
> > modifing
> > the device tree source, but when the device tree is provided by a
> > closed
> > source BIOS this kind of missing symbol could not be fixed.
> > 
> > An additional benefit is to override a (possible broken) symbol
> > exported
> > by the currect live device tree.
> > 
> > The patch is based and tested on linux 4.12-rc3.
> > 
> > Signed-off-by: Stefani Seibold <stefani.seibold.ext@huawei.com>
> > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > ---
> >  drivers/of/overlay.c  | 19 +++++++++++++++++++
> >  drivers/of/resolver.c | 27 ++++++++++++++++++++++-----
> >  2 files changed, 41 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 7827786718d8..de6516ea0fcd 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -50,6 +50,7 @@ struct of_overlay {
> >  	int id;
> >  	struct list_head node;
> >  	int count;
> > +	struct device_node *tree;
> >  	struct of_overlay_info *ovinfo_tab;
> >  	struct of_changeset cset;
> >  };
> > @@ -422,6 +423,8 @@ int of_overlay_create(struct device_node *tree)
> >  	/* add to the tail of the overlay list */
> >  	list_add_tail(&ov->node, &ov_list);
> >  
> > +	ov->tree = tree;
> > +
> >  	of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
> >  
> >  	mutex_unlock(&of_mutex);
> > @@ -524,6 +527,7 @@ int of_overlay_destroy(int id)
> >  {
> >  	struct of_overlay *ov;
> >  	int err;
> > +	phandle phandle;
> >  
> >  	mutex_lock(&of_mutex);
> >  
> > @@ -540,6 +544,8 @@ int of_overlay_destroy(int id)
> >  		goto out;
> >  	}
> >  
> > +	phandle = ov->tree->phandle;
> > +
> >  	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
> >  	list_del(&ov->node);
> >  	__of_changeset_revert(&ov->cset);
> > @@ -549,6 +555,19 @@ int of_overlay_destroy(int id)
> >  	of_changeset_destroy(&ov->cset);
> >  	kfree(ov);
> >  
> > +	if (phandle) {
> > +		struct device_node *node;
> > +		unsigned long flags;
> > +
> > +		raw_spin_lock_irqsave(&devtree_lock, flags);
> > +		for_each_of_allnodes(node) {
> > +			if (node->phandle >= phandle)
> > +				node->phandle = 0;
> > +		}
> > +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > +	}
> > +
> > +
> >  	err = 0;
> >  
> >  out:
> > diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> > index 771f4844c781..31b5f32c9b27 100644
> > --- a/drivers/of/resolver.c
> > +++ b/drivers/of/resolver.c
> > @@ -286,13 +286,14 @@ static int
> > adjust_local_phandle_references(struct device_node *local_fixups,
> >  int of_resolve_phandles(struct device_node *overlay)
> >  {
> >  	struct device_node *child, *local_fixups, *refnode;
> > -	struct device_node *tree_symbols, *overlay_fixups;
> > +	struct device_node *tree_symbols, *ext_symbols,
> > *overlay_fixups;
> >  	struct property *prop;
> >  	const char *refpath;
> >  	phandle phandle, phandle_delta;
> >  	int err;
> >  
> >  	tree_symbols = NULL;
> > +	ext_symbols = NULL;
> >  
> >  	if (!overlay) {
> >  		pr_err("null overlay\n");
> > @@ -321,6 +322,9 @@ int of_resolve_phandles(struct device_node
> > *overlay)
> >  	for_each_child_of_node(overlay, child) {
> >  		if (!of_node_cmp(child->name, "__fixups__"))
> >  			overlay_fixups = child;
> > +		else
> > +		if (!of_node_cmp(child->name,
> > "__external_symbols__"))
> > +			ext_symbols = child;
> >  	}
> >  
> >  	if (!overlay_fixups) {
> > @@ -329,20 +333,30 @@ int of_resolve_phandles(struct device_node
> > *overlay)
> >  	}
> >  
> >  	tree_symbols = of_find_node_by_path("/__symbols__");
> > -	if (!tree_symbols) {
> > -		pr_err("no symbols in root of device tree.\n");
> > +	if (!tree_symbols && !ext_symbols) {
> > +		pr_err("no symbols for resolve in device
> > tree.\n");
> >  		err = -EINVAL;
> >  		goto out;
> >  	}
> >  
> > +	phandle_delta = live_tree_max_phandle() + 1;
> > +
> >  	for_each_property_of_node(overlay_fixups, prop) {
> >  
> >  		/* skip properties added automatically */
> >  		if (!of_prop_cmp(prop->name, "name"))
> >  			continue;
> >  
> > -		err = of_property_read_string(tree_symbols,
> > -				prop->name, &refpath);
> > +		err = -1;
> > +
> > +		if (ext_symbols)
> > +			err = of_property_read_string(ext_symbols,
> > +					prop->name, &refpath);
> > +
> > +		if (err && tree_symbols)
> > +			err =
> > of_property_read_string(tree_symbols,
> > +					prop->name, &refpath);
> > +
> >  		if (err)
> >  			goto out;
> >  
> > @@ -352,6 +366,9 @@ int of_resolve_phandles(struct device_node
> > *overlay)
> >  			goto out;
> >  		}
> >  
> > +		if (!refnode->phandle)
> > +			refnode->phandle = ++phandle_delta;
> > +
> >  		phandle = refnode->phandle;
> >  		of_node_put(refnode);
> >  
> 
>

[toc] | [prev] | [next] | [standalone]

#1659252

FromRob Herring <robh+dt@kernel.org>
Date2017-06-07 00:10 +0200
Message-ID<tPuPM-6gH-11@gated-at.bofh.it>
In reply to#1659148
On Tue, Jun 6, 2017 at 2:17 PM, Stefani Seibold <stefani@seibold.net> wrote:
> Hi Pantelis,
>
> thanks for the suggestion. This feature is not very well documented. I
> tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
> source is:
>
> // rapsi example
> /dts-v1/;
> /plugin/;
>
> / {
>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>
>     fragment@0 {
>         target-path = "/soc/i2s@7e203000";
>         __overlay__ {
>             #address-cells = <0x00000001>;
>             #size-cells = <0x00000001>;
>             test = "test";
>             timer = <&{/soc/timer@7e0030000}>;
>         };
>     };
> };
>
>
> The resulting overlay is (decompiled with fdtdump):
>
> /dts-v1/;
> // magic:               0xd00dfeed
> // totalsize:           0x19a (410)
> // off_dt_struct:       0x38
> // off_dt_strings:      0x148
> // off_mem_rsvmap:      0x28
> // version:             17
> // last_comp_version:   16
> // boot_cpuid_phys:     0x0
> // size_dt_strings:     0x52
> // size_dt_struct:      0x110
>
> / {
>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>     fragment@0 {
>         target-path = "/soc/i2s@7e203000";
>         __overlay__ {
>             #address-cells = <0x00000001>;
>             #size-cells = <0x00000001>;
>             test = "test";
>             timer = <0xdeadbeef>;
>         };
>     };
>     __fixups__ {
>         /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";

Looks like you (Pantelis had the typo) have an extra 0 in 7e0030000
compared to your original example.

Rob

[toc] | [prev] | [next] | [standalone]

#1659508

FromPantelis Antoniou <pantelis.antoniou@konsulko.com>
Date2017-06-07 10:20 +0200
Message-ID<tPEm5-44h-9@gated-at.bofh.it>
In reply to#1659148
Hi Stefani,

On Tue, 2017-06-06 at 21:17 +0200, Stefani Seibold wrote:
> Hi Pantelis,
> 
> thanks for the suggestion. This feature is not very well documented. I
> tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
> source is:
> 
> // rapsi example
> /dts-v1/;
> /plugin/;
> 
> / {
>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> 
>     fragment@0 {
>         target-path = "/soc/i2s@7e203000";
>         __overlay__ {
>             #address-cells = <0x00000001>;
>             #size-cells = <0x00000001>;
>             test = "test";
>             timer = <&{/soc/timer@7e0030000}>;
>         };
>     };
> };
> 
> 
> The resulting overlay is (decompiled with fdtdump):
> 
> /dts-v1/;
> // magic:		0xd00dfeed
> // totalsize:		0x19a (410)
> // off_dt_struct:	0x38
> // off_dt_strings:	0x148
> // off_mem_rsvmap:	0x28
> // version:		17
> // last_comp_version:	16
> // boot_cpuid_phys:	0x0
> // size_dt_strings:	0x52
> // size_dt_struct:	0x110
> 
> / {
>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>     fragment@0 {
>         target-path = "/soc/i2s@7e203000";
>         __overlay__ {
>             #address-cells = <0x00000001>;
>             #size-cells = <0x00000001>;
>             test = "test";
>             timer = <0xdeadbeef>;
>         };
>     };
>     __fixups__ {
>         /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";
>     };
> };
> 
> But this will not apply:
> 
> OF: resolver: overlay phandle fixup failed: -22
> create_overlay: Failed to resolve tree
> 
> 

Yes, it will not work as it is; my point is that you don't need the
magic __*__ node.

You will need to modify the overlay application code to live insert a
phandle (if it doesn't exist) when it encounters a /path fixup.

> Anyway, the reason for my patch is that i can reference to nodes which
> lacks a phandle. The phandle will be created on the fly and also
> destroyed when the overlay is unloaded.
> 
> I have a real use case for this patch:
> 
> I have a BIOS on some ARM64 servers which provides broken device tree.
> It also lacks some devices in this tree which needs references to other
> devices which lacks a phandle.
> 
> Since the BIOSes are closed source i need a way to work arround this
> problem without patching all the drivers involved to this devices.
> 
> Hope this helps to understand the reason for this patch.
> 

FWIW your problem seems like something that would happen on the field.
We can berate the vendor of not providing the correct device tree, but
in the end workarounds for broken vendor things are common in the
kernel.

Regards

-- Pantelis

> - Stefani
> 
> Am Montag, den 05.06.2017, 21:43 +0300 schrieb Pantelis Antoniou:
> > Hi Stefani,
> > 
> > On Mon, 2017-06-05 at 14:59 +0200, Stefani Seibold wrote:
> > > From: Stefani Seibold <stefani@seibold.net>
> > > 
> > > This patch enables external references for symbols which are not
> > > exported by the current device tree. For example
> > > 
> > > // RASPI example (only for testing)
> > > /dts-v1/;
> > > /plugin/;
> > > 
> > > / {
> > >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> > > 
> > >     fragment@0 {
> > >         target-path = "/soc/i2s@7e203000";
> > >         __overlay__ {
> > >             #address-cells = <0x00000001>;
> > >             #size-cells = <0x00000001>;
> > >             test = "test";
> > >             timer = <&timer>;
> > >         };
> > >     };
> > > 
> > >     __external_symbols__ {
> > >         timer = "/soc/timer@7e003000";
> > >     };
> > > };
> > > 
> > 
> > I understand the problem. I am just not fond of the
> > __external_symbols__
> > solution.
> > 
> > There's a facility in the DT source language that allows to declare
> > pathspec labels.
> > 
> > The 'timer = <&timer>;' statement could be rewritten as 
> > 'timer = <&{/soc/timer@7e0030000}>;'
> > 
> > Internally you can 'catch' that this refers to a symbol in the base
> > tree
> > and then do the same symbol insertion as the patch you've submitted.
> > 
> > The benefit to the above is that you don't introduce manually edited
> > special nodes.
> > 
> > Regards
> > 
> > -- Pantelis
> > 
> > > The "timer" symbol is not exported by the RASPI device tree,
> > > because it is
> > > missing in the __symbols__ section of the device tree.
> > > 
> > > In case of the RASPI device tree this could be simple fixed by
> > > modifing
> > > the device tree source, but when the device tree is provided by a
> > > closed
> > > source BIOS this kind of missing symbol could not be fixed.
> > > 
> > > An additional benefit is to override a (possible broken) symbol
> > > exported
> > > by the currect live device tree.
> > > 
> > > The patch is based and tested on linux 4.12-rc3.
> > > 
> > > Signed-off-by: Stefani Seibold <stefani.seibold.ext@huawei.com>
> > > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > > ---
> > >  drivers/of/overlay.c  | 19 +++++++++++++++++++
> > >  drivers/of/resolver.c | 27 ++++++++++++++++++++++-----
> > >  2 files changed, 41 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > > index 7827786718d8..de6516ea0fcd 100644
> > > --- a/drivers/of/overlay.c
> > > +++ b/drivers/of/overlay.c
> > > @@ -50,6 +50,7 @@ struct of_overlay {
> > >  	int id;
> > >  	struct list_head node;
> > >  	int count;
> > > +	struct device_node *tree;
> > >  	struct of_overlay_info *ovinfo_tab;
> > >  	struct of_changeset cset;
> > >  };
> > > @@ -422,6 +423,8 @@ int of_overlay_create(struct device_node *tree)
> > >  	/* add to the tail of the overlay list */
> > >  	list_add_tail(&ov->node, &ov_list);
> > >  
> > > +	ov->tree = tree;
> > > +
> > >  	of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
> > >  
> > >  	mutex_unlock(&of_mutex);
> > > @@ -524,6 +527,7 @@ int of_overlay_destroy(int id)
> > >  {
> > >  	struct of_overlay *ov;
> > >  	int err;
> > > +	phandle phandle;
> > >  
> > >  	mutex_lock(&of_mutex);
> > >  
> > > @@ -540,6 +544,8 @@ int of_overlay_destroy(int id)
> > >  		goto out;
> > >  	}
> > >  
> > > +	phandle = ov->tree->phandle;
> > > +
> > >  	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
> > >  	list_del(&ov->node);
> > >  	__of_changeset_revert(&ov->cset);
> > > @@ -549,6 +555,19 @@ int of_overlay_destroy(int id)
> > >  	of_changeset_destroy(&ov->cset);
> > >  	kfree(ov);
> > >  
> > > +	if (phandle) {
> > > +		struct device_node *node;
> > > +		unsigned long flags;
> > > +
> > > +		raw_spin_lock_irqsave(&devtree_lock, flags);
> > > +		for_each_of_allnodes(node) {
> > > +			if (node->phandle >= phandle)
> > > +				node->phandle = 0;
> > > +		}
> > > +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > > +	}
> > > +
> > > +
> > >  	err = 0;
> > >  
> > >  out:
> > > diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> > > index 771f4844c781..31b5f32c9b27 100644
> > > --- a/drivers/of/resolver.c
> > > +++ b/drivers/of/resolver.c
> > > @@ -286,13 +286,14 @@ static int
> > > adjust_local_phandle_references(struct device_node *local_fixups,
> > >  int of_resolve_phandles(struct device_node *overlay)
> > >  {
> > >  	struct device_node *child, *local_fixups, *refnode;
> > > -	struct device_node *tree_symbols, *overlay_fixups;
> > > +	struct device_node *tree_symbols, *ext_symbols,
> > > *overlay_fixups;
> > >  	struct property *prop;
> > >  	const char *refpath;
> > >  	phandle phandle, phandle_delta;
> > >  	int err;
> > >  
> > >  	tree_symbols = NULL;
> > > +	ext_symbols = NULL;
> > >  
> > >  	if (!overlay) {
> > >  		pr_err("null overlay\n");
> > > @@ -321,6 +322,9 @@ int of_resolve_phandles(struct device_node
> > > *overlay)
> > >  	for_each_child_of_node(overlay, child) {
> > >  		if (!of_node_cmp(child->name, "__fixups__"))
> > >  			overlay_fixups = child;
> > > +		else
> > > +		if (!of_node_cmp(child->name,
> > > "__external_symbols__"))
> > > +			ext_symbols = child;
> > >  	}
> > >  
> > >  	if (!overlay_fixups) {
> > > @@ -329,20 +333,30 @@ int of_resolve_phandles(struct device_node
> > > *overlay)
> > >  	}
> > >  
> > >  	tree_symbols = of_find_node_by_path("/__symbols__");
> > > -	if (!tree_symbols) {
> > > -		pr_err("no symbols in root of device tree.\n");
> > > +	if (!tree_symbols && !ext_symbols) {
> > > +		pr_err("no symbols for resolve in device
> > > tree.\n");
> > >  		err = -EINVAL;
> > >  		goto out;
> > >  	}
> > >  
> > > +	phandle_delta = live_tree_max_phandle() + 1;
> > > +
> > >  	for_each_property_of_node(overlay_fixups, prop) {
> > >  
> > >  		/* skip properties added automatically */
> > >  		if (!of_prop_cmp(prop->name, "name"))
> > >  			continue;
> > >  
> > > -		err = of_property_read_string(tree_symbols,
> > > -				prop->name, &refpath);
> > > +		err = -1;
> > > +
> > > +		if (ext_symbols)
> > > +			err = of_property_read_string(ext_symbols,
> > > +					prop->name, &refpath);
> > > +
> > > +		if (err && tree_symbols)
> > > +			err =
> > > of_property_read_string(tree_symbols,
> > > +					prop->name, &refpath);
> > > +
> > >  		if (err)
> > >  			goto out;
> > >  
> > > @@ -352,6 +366,9 @@ int of_resolve_phandles(struct device_node
> > > *overlay)
> > >  			goto out;
> > >  		}
> > >  
> > > +		if (!refnode->phandle)
> > > +			refnode->phandle = ++phandle_delta;
> > > +
> > >  		phandle = refnode->phandle;
> > >  		of_node_put(refnode);
> > >  
> > 
> >

[toc] | [prev] | [next] | [standalone]

#1660284

FromRob Herring <robh+dt@kernel.org>
Date2017-06-08 00:20 +0200
Message-ID<tPRsZ-47t-5@gated-at.bofh.it>
In reply to#1659508
On Wed, Jun 7, 2017 at 3:11 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Stefani,
>
> On Tue, 2017-06-06 at 21:17 +0200, Stefani Seibold wrote:
>> Hi Pantelis,
>>
>> thanks for the suggestion. This feature is not very well documented. I
>> tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
>> source is:
>>
>> // rapsi example
>> /dts-v1/;
>> /plugin/;
>>
>> / {
>>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>>
>>     fragment@0 {
>>         target-path = "/soc/i2s@7e203000";
>>         __overlay__ {
>>             #address-cells = <0x00000001>;
>>             #size-cells = <0x00000001>;
>>             test = "test";
>>             timer = <&{/soc/timer@7e0030000}>;
>>         };
>>     };
>> };
>>
>>
>> The resulting overlay is (decompiled with fdtdump):
>>
>> /dts-v1/;
>> // magic:             0xd00dfeed
>> // totalsize:         0x19a (410)
>> // off_dt_struct:     0x38
>> // off_dt_strings:    0x148
>> // off_mem_rsvmap:    0x28
>> // version:           17
>> // last_comp_version: 16
>> // boot_cpuid_phys:   0x0
>> // size_dt_strings:   0x52
>> // size_dt_struct:    0x110
>>
>> / {
>>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>>     fragment@0 {
>>         target-path = "/soc/i2s@7e203000";
>>         __overlay__ {
>>             #address-cells = <0x00000001>;
>>             #size-cells = <0x00000001>;
>>             test = "test";
>>             timer = <0xdeadbeef>;
>>         };
>>     };
>>     __fixups__ {
>>         /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";
>>     };
>> };
>>
>> But this will not apply:
>>
>> OF: resolver: overlay phandle fixup failed: -22
>> create_overlay: Failed to resolve tree
>>
>>
>
> Yes, it will not work as it is; my point is that you don't need the
> magic __*__ node.
>
> You will need to modify the overlay application code to live insert a
> phandle (if it doesn't exist) when it encounters a /path fixup.

phandles only exist if something in the base tree refers to that node.
Adding them when they don't exist should definitely be something we
support for overlays. But don't call that a broken DT. That would be a
separate issue.

Rob

[toc] | [prev] | [next] | [standalone]

#1660796

FromStefani Seibold <stefani@seibold.net>
Date2017-06-08 09:00 +0200
Message-ID<tPZAd-Nh-3@gated-at.bofh.it>
In reply to#1660284
On Wed, 2017-06-07 at 17:19 -0500, Rob Herring wrote:
> On Wed, Jun 7, 2017 at 3:11 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
> > Hi Stefani,
> > 
> > On Tue, 2017-06-06 at 21:17 +0200, Stefani Seibold wrote:
> > > Hi Pantelis,
> > > 
> > > thanks for the suggestion. This feature is not very well
> > > documented. I
> > > tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
> > > source is:
> > > 
> > > // rapsi example
> > > /dts-v1/;
> > > /plugin/;
> > > 
> > > / {
> > >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> > > 
> > >     fragment@0 {
> > >         target-path = "/soc/i2s@7e203000";
> > >         __overlay__ {
> > >             #address-cells = <0x00000001>;
> > >             #size-cells = <0x00000001>;
> > >             test = "test";
> > >             timer = <&{/soc/timer@7e0030000}>;
> > >         };
> > >     };
> > > };
> > > 
> > > 
> > > The resulting overlay is (decompiled with fdtdump):
> > > 
> > > /dts-v1/;
> > > // magic:             0xd00dfeed
> > > // totalsize:         0x19a (410)
> > > // off_dt_struct:     0x38
> > > // off_dt_strings:    0x148
> > > // off_mem_rsvmap:    0x28
> > > // version:           17
> > > // last_comp_version: 16
> > > // boot_cpuid_phys:   0x0
> > > // size_dt_strings:   0x52
> > > // size_dt_struct:    0x110
> > > 
> > > / {
> > >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> > >     fragment@0 {
> > >         target-path = "/soc/i2s@7e203000";
> > >         __overlay__ {
> > >             #address-cells = <0x00000001>;
> > >             #size-cells = <0x00000001>;
> > >             test = "test";
> > >             timer = <0xdeadbeef>;
> > >         };
> > >     };
> > >     __fixups__ {
> > >         /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";
> > >     };
> > > };
> > > 
> > > But this will not apply:
> > > 
> > > OF: resolver: overlay phandle fixup failed: -22
> > > create_overlay: Failed to resolve tree
> > > 
> > > 
> > 
> > Yes, it will not work as it is; my point is that you don't need the
> > magic __*__ node.
> > 
> > You will need to modify the overlay application code to live insert
> > a
> > phandle (if it doesn't exist) when it encounters a /path fixup.
> 
> phandles only exist if something in the base tree refers to that
> node.
> Adding them when they don't exist should definitely be something we
> support for overlays. But don't call that a broken DT. That would be
> a
> separate issue.
> 

Believe me it is broken. Due a NDA i am not able to give you more
details about the vendor. But there forgot do provide an device node
which must refer to the attached network and interrupt controller.

- Stefani

[toc] | [prev] | [next] | [standalone]

#1660791

FromStefani Seibold <stefani@seibold.net>
Date2017-06-08 08:50 +0200
Message-ID<tPZqx-JX-1@gated-at.bofh.it>
In reply to#1659508
Hi Pantelis,

On Wed, 2017-06-07 at 11:11 +0300, Pantelis Antoniou wrote:
> Hi Stefani,
> 
> On Tue, 2017-06-06 at 21:17 +0200, Stefani Seibold wrote:
> > Hi Pantelis,
> > 
> > thanks for the suggestion. This feature is not very well
> > documented. I
> > tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
> > source is:
> > 
> > // rapsi example
> > /dts-v1/;
> > /plugin/;
> > 
> > / {
> >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> > 
> >     fragment@0 {
> >         target-path = "/soc/i2s@7e203000";
> >         __overlay__ {
> >             #address-cells = <0x00000001>;
> >             #size-cells = <0x00000001>;
> >             test = "test";
> >             timer = <&{/soc/timer@7e0030000}>;
> >         };
> >     };
> > };
> > 
> > 
> > The resulting overlay is (decompiled with fdtdump):
> > 
> > /dts-v1/;
> > // magic:		0xd00dfeed
> > // totalsize:		0x19a (410)
> > // off_dt_struct:	0x38
> > // off_dt_strings:	0x148
> > // off_mem_rsvmap:	0x28
> > // version:		17
> > // last_comp_version:	16
> > // boot_cpuid_phys:	0x0
> > // size_dt_strings:	0x52
> > // size_dt_struct:	0x110
> > 
> > / {
> >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> >     fragment@0 {
> >         target-path = "/soc/i2s@7e203000";
> >         __overlay__ {
> >             #address-cells = <0x00000001>;
> >             #size-cells = <0x00000001>;
> >             test = "test";
> >             timer = <0xdeadbeef>;
> >         };
> >     };
> >     __fixups__ {
> >         /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";
> >     };
> > };
> > 
> > But this will not apply:
> > 
> > OF: resolver: overlay phandle fixup failed: -22
> > create_overlay: Failed to resolve tree
> > 
> > 
> 
> Yes, it will not work as it is; my point is that you don't need the
> magic __*__ node.
> 

The magic __fixups__ node was inserted by the device tree compiler. I
use the dtc from https://github.com/pantoniou/dtc at commit
d990b8013889b816ec054c7e07a77db59c56c400.

> You will need to modify the overlay application code to live insert a
> phandle (if it doesn't exist) when it encounters a /path fixup.
> 

That is part of my patch!

> > Anyway, the reason for my patch is that i can reference to nodes
> > which
> > lacks a phandle. The phandle will be created on the fly and also
> > destroyed when the overlay is unloaded.
> > 
> > I have a real use case for this patch:
> > 
> > I have a BIOS on some ARM64 servers which provides broken device
> > tree.
> > It also lacks some devices in this tree which needs references to
> > other
> > devices which lacks a phandle.
> > 
> > Since the BIOSes are closed source i need a way to work arround
> > this
> > problem without patching all the drivers involved to this devices.
> > 
> > Hope this helps to understand the reason for this patch.
> > 
> 
> FWIW your problem seems like something that would happen on the
> field.
> We can berate the vendor of not providing the correct device tree,
> but
> in the end workarounds for broken vendor things are common in the
> kernel.
> 

Yes, that is the way how linux do the things. Linux has a long history
to bypassing bugs of BIOSes, ACPI or broken devices.

Greetings,
Stefani

[toc] | [prev] | [next] | [standalone]

#1663164

FromFrank Rowand <frowand.list@gmail.com>
Date2017-06-12 01:10 +0200
Message-ID<tRk9z-2JW-1@gated-at.bofh.it>
In reply to#1660791
On 06/07/17 23:48, Stefani Seibold wrote:
> Hi Pantelis,
> 
> On Wed, 2017-06-07 at 11:11 +0300, Pantelis Antoniou wrote:
>> Hi Stefani,
>>
>> On Tue, 2017-06-06 at 21:17 +0200, Stefani Seibold wrote:
>>> Hi Pantelis,
>>>
>>> thanks for the suggestion. This feature is not very well
>>> documented. I
>>> tried this on my rasp1 running 4.12.0-rc3 and it doesn't work. My
>>> source is:
>>>
>>> // rapsi example
>>> /dts-v1/;
>>> /plugin/;
>>>
>>> / {
>>>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>>>
>>>     fragment@0 {
>>>         target-path = "/soc/i2s@7e203000";
>>>         __overlay__ {
>>>             #address-cells = <0x00000001>;
>>>             #size-cells = <0x00000001>;
>>>             test = "test";
>>>             timer = <&{/soc/timer@7e0030000}>;
>>>         };
>>>     };
>>> };
>>>
>>>
>>> The resulting overlay is (decompiled with fdtdump):
>>>
>>> /dts-v1/;
>>> // magic:		0xd00dfeed
>>> // totalsize:		0x19a (410)
>>> // off_dt_struct:	0x38
>>> // off_dt_strings:	0x148
>>> // off_mem_rsvmap:	0x28
>>> // version:		17
>>> // last_comp_version:	16
>>> // boot_cpuid_phys:	0x0
>>> // size_dt_strings:	0x52
>>> // size_dt_struct:	0x110
>>>
>>> / {
>>>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>>>     fragment@0 {
>>>         target-path = "/soc/i2s@7e203000";
>>>         __overlay__ {
>>>             #address-cells = <0x00000001>;
>>>             #size-cells = <0x00000001>;
>>>             test = "test";
>>>             timer = <0xdeadbeef>;
>>>         };
>>>     };
>>>     __fixups__ {
>>>         /soc/timer@7e0030000 = "/fragment@0/__overlay__:timer:0";
>>>     };
>>> };
>>>
>>> But this will not apply:
>>>
>>> OF: resolver: overlay phandle fixup failed: -22
>>> create_overlay: Failed to resolve tree
>>>
>>>
>>
>> Yes, it will not work as it is; my point is that you don't need the
>> magic __*__ node.
>>
> 
> The magic __fixups__ node was inserted by the device tree compiler. I
> use the dtc from https://github.com/pantoniou/dtc at commit
> d990b8013889b816ec054c7e07a77db59c56c400.
> 
>> You will need to modify the overlay application code to live insert a
>> phandle (if it doesn't exist) when it encounters a /path fixup.
>>
> 
> That is part of my patch!
> 
>>> Anyway, the reason for my patch is that i can reference to nodes
>>> which
>>> lacks a phandle. The phandle will be created on the fly and also
>>> destroyed when the overlay is unloaded.
>>>
>>> I have a real use case for this patch:
>>>
>>> I have a BIOS on some ARM64 servers which provides broken device
>>> tree.
>>> It also lacks some devices in this tree which needs references to
>>> other
>>> devices which lacks a phandle.
>>>
>>> Since the BIOSes are closed source i need a way to work arround
>>> this
>>> problem without patching all the drivers involved to this devices.
>>>
>>> Hope this helps to understand the reason for this patch.
>>>
>>
>> FWIW your problem seems like something that would happen on the
>> field.
>> We can berate the vendor of not providing the correct device tree,
>> but
>> in the end workarounds for broken vendor things are common in the
>> kernel.
>>
> 
> Yes, that is the way how linux do the things. Linux has a long history
> to bypassing bugs of BIOSes, ACPI or broken devices.

ARM device tree in Linux is not like BIOSes or ACPI.

ARM device tree in Linux is GPL v2 licensed (yes, it may also be dual
licensed for other uses) and thus "free software".

One of the key points of "free software" is that you have access to the
source and the ability to modify it.

Instead of bypassing device tree bugs, you have the ability to fix
device tree bugs.  This is a fundamental difference.

-Frank

> 
> Greetings,
> Stefani
> 
>

[toc] | [prev] | [next] | [standalone]

#1658436

FromFrank Rowand <frowand.list@gmail.com>
Date2017-06-06 09:30 +0200
Message-ID<tPh6a-5ES-13@gated-at.bofh.it>
In reply to#1657609
On 06/05/17 05:59, Stefani Seibold wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> This patch enables external references for symbols which are not
> exported by the current device tree. For example
> 
> // RASPI example (only for testing)
> /dts-v1/;
> /plugin/;
> 
> / {
>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> 
>     fragment@0 {
>         target-path = "/soc/i2s@7e203000";
>         __overlay__ {
>             #address-cells = <0x00000001>;
>             #size-cells = <0x00000001>;
>             test = "test";
>             timer = <&timer>;
>         };
>     };
> 
>     __external_symbols__ {
>         timer = "/soc/timer@7e003000";
>     };
> };

My hope is that the dtc compiler will stop supporting specification of the
__symbols__ node in dts source, and only generate it automatically in the dtb.
That change to dtc would not allow any node name specified in a dts to begin
with an underscore.  Thus node __external_symbols__ would not be allowed.


> 
> The "timer" symbol is not exported by the RASPI device tree, because it is
> missing in the __symbols__ section of the device tree.
> 
> In case of the RASPI device tree this could be simple fixed by modifing
> the device tree source, but when the device tree is provided by a closed
> source BIOS this kind of missing symbol could not be fixed.

Is there a real example of this issue, or is this a theoretical concern?
If this is a real example, we should be discouraging such behavior.

The suggestion by Pantelis should work, but that is just a hack to get
you out of a bad situation, not a good practice.

> 
> An additional benefit is to override a (possible broken) symbol exported
> by the currect live device tree.
> 
> The patch is based and tested on linux 4.12-rc3.
> 
> Signed-off-by: Stefani Seibold <stefani.seibold.ext@huawei.com>
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/of/overlay.c  | 19 +++++++++++++++++++
>  drivers/of/resolver.c | 27 ++++++++++++++++++++++-----
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 7827786718d8..de6516ea0fcd 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c

< snip >

-Frank

[toc] | [prev] | [next] | [standalone]

#1658918

FromFrank Rowand <frowand.list@gmail.com>
Date2017-06-06 18:20 +0200
Message-ID<tPpn5-2GL-47@gated-at.bofh.it>
In reply to#1658436
On 06/06/17 00:20, Frank Rowand wrote:
> On 06/05/17 05:59, Stefani Seibold wrote:
>> From: Stefani Seibold <stefani@seibold.net>
>>
>> This patch enables external references for symbols which are not
>> exported by the current device tree. For example
>>
>> // RASPI example (only for testing)
>> /dts-v1/;
>> /plugin/;
>>
>> / {
>>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>>
>>     fragment@0 {
>>         target-path = "/soc/i2s@7e203000";
>>         __overlay__ {
>>             #address-cells = <0x00000001>;
>>             #size-cells = <0x00000001>;
>>             test = "test";
>>             timer = <&timer>;
>>         };
>>     };
>>
>>     __external_symbols__ {
>>         timer = "/soc/timer@7e003000";
>>     };
>> };
> 
> My hope is that the dtc compiler will stop supporting specification of the
> __symbols__ node in dts source, and only generate it automatically in the dtb.
> That change to dtc would not allow any node name specified in a dts to begin
> with an underscore.  Thus node __external_symbols__ would not be allowed.
> 
> 
>>
>> The "timer" symbol is not exported by the RASPI device tree, because it is
>> missing in the __symbols__ section of the device tree.
>>
>> In case of the RASPI device tree this could be simple fixed by modifing
>> the device tree source, but when the device tree is provided by a closed
>> source BIOS this kind of missing symbol could not be fixed.
> 
> Is there a real example of this issue, or is this a theoretical concern?
> If this is a real example, we should be discouraging such behavior.
> 
> The suggestion by Pantelis should work, but that is just a hack to get
> you out of a bad situation, not a good practice.

Digging a little bit deeper into the suggestion by Pantelis, I don't like
that approach either.  It is still rather hacky.

> 
>>
>> An additional benefit is to override a (possible broken) symbol exported
>> by the currect live device tree.
>>
>> The patch is based and tested on linux 4.12-rc3.
>>
>> Signed-off-by: Stefani Seibold <stefani.seibold.ext@huawei.com>
>> Signed-off-by: Stefani Seibold <stefani@seibold.net>
>> ---
>>  drivers/of/overlay.c  | 19 +++++++++++++++++++
>>  drivers/of/resolver.c | 27 ++++++++++++++++++++++-----
>>  2 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 7827786718d8..de6516ea0fcd 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
> 
> < snip >
> 
> -Frank
>

[toc] | [prev] | [next] | [standalone]

#1659127

FromStefani Seibold <stefani@seibold.net>
Date2017-06-06 21:30 +0200
Message-ID<tPskW-4zc-29@gated-at.bofh.it>
In reply to#1658436
Hi Frank,

On 06.06.2017, 00:20 -0700 Frank Rowand wrote::
> On 06/05/17 05:59, Stefani Seibold wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > This patch enables external references for symbols which are not
> > exported by the current device tree. For example
> > 
> > // RASPI example (only for testing)
> > /dts-v1/;
> > /plugin/;
> > 
> > / {
> >     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
> > 
> >     fragment@0 {
> >         target-path = "/soc/i2s@7e203000";
> >         __overlay__ {
> >             #address-cells = <0x00000001>;
> >             #size-cells = <0x00000001>;
> >             test = "test";
> >             timer = <&timer>;
> >         };
> >     };
> > 
> >     __external_symbols__ {
> >         timer = "/soc/timer@7e003000";
> >     };
> > };
> 
> My hope is that the dtc compiler will stop supporting specification
> of the
> __symbols__ node in dts source, and only generate it automatically in
> the dtb.
> That change to dtc would not allow any node name specified in a dts
> to begin
> with an underscore.  Thus node __external_symbols__ would not be
> allowed.
> 

The name is not so important to me, only the solution.

> > In case of the RASPI device tree this could be simple fixed by
> > modifing
> > the device tree source, but when the device tree is provided by a
> > closed
> > source BIOS this kind of missing symbol could not be fixed.
> 
> Is there a real example of this issue, or is this a theoretical
> concern?
> If this is a real example, we should be discouraging such behavior.
> 

Yes, I have a BIOS on some ARM64 servers which provides broken device tree. It also lacks some devices in this tree which needs references to other devices which lacks a phandle.


> The suggestion by Pantelis should work, but that is just a hack to
> get
> you out of a bad situation, not a good practice.
> 

I tried it, but it doesn't work. Look at my post to Pantelis.

- Stefani

[toc] | [prev] | [next] | [standalone]

#1659319

FromFrank Rowand <frowand.list@gmail.com>
Date2017-06-07 02:50 +0200
Message-ID<tPxkB-7IU-3@gated-at.bofh.it>
In reply to#1659127
On 06/06/17 12:22, Stefani Seibold wrote:
> Hi Frank,
> 
> On 06.06.2017, 00:20 -0700 Frank Rowand wrote::
>> On 06/05/17 05:59, Stefani Seibold wrote:
>>> From: Stefani Seibold <stefani@seibold.net>
>>>
>>> This patch enables external references for symbols which are not
>>> exported by the current device tree. For example
>>>
>>> // RASPI example (only for testing)
>>> /dts-v1/;
>>> /plugin/;
>>>
>>> / {
>>>     compatible = "brcm,bcm2835", "brcm,bcm2708", "brcm,bcm2709";
>>>
>>>     fragment@0 {
>>>         target-path = "/soc/i2s@7e203000";
>>>         __overlay__ {
>>>             #address-cells = <0x00000001>;
>>>             #size-cells = <0x00000001>;
>>>             test = "test";
>>>             timer = <&timer>;
>>>         };
>>>     };
>>>
>>>     __external_symbols__ {
>>>         timer = "/soc/timer@7e003000";
>>>     };
>>> };
>>
>> My hope is that the dtc compiler will stop supporting specification
>> of the
>> __symbols__ node in dts source, and only generate it automatically in
>> the dtb.
>> That change to dtc would not allow any node name specified in a dts
>> to begin
>> with an underscore.  Thus node __external_symbols__ would not be
>> allowed.
>>
> 
> The name is not so important to me, only the solution.
> 
>>> In case of the RASPI device tree this could be simple fixed by
>>> modifing
>>> the device tree source, but when the device tree is provided by a
>>> closed
>>> source BIOS this kind of missing symbol could not be fixed.
>>
>> Is there a real example of this issue, or is this a theoretical
>> concern?
>> If this is a real example, we should be discouraging such behavior.
>>
> 
> Yes, I have a BIOS on some ARM64 servers which provides broken device
> tree. It also lacks some devices in this tree which needs references
> to other devices which lacks a phandle.

Jon Masters is pushing a message that if the firmware on your arm64 server
is broken, then insist that the vendor fix it.  I think he was talking
about ACPI, but the same message should also apply to device tree.

If you are having trouble getting your vendor to fix it, ask Jon if he
is willing to help apply pressure.


>> The suggestion by Pantelis should work, but that is just a hack to
>> get
>> you out of a bad situation, not a good practice.
>>
> 
> I tried it, but it doesn't work. Look at my post to Pantelis.

Yes, I realized that the method Pantelis gave would also require
a code change.  I don't like that code change either.


> 
> - Stefani
> .
>

[toc] | [prev] | [next] | [standalone]

#1664178

FromFrank Rowand <frowand.list@gmail.com>
Date2017-06-12 20:50 +0200
Message-ID<tRCzw-5S5-13@gated-at.bofh.it>
In reply to#1659319
Adding back the Cc: list

On 06/11/17 23:13, Stefani Seibold wrote:
> Hi Frank,i 
> 
> i am in vaction for the next two weeks. I will rewrite the patch when i
> am back.
> 
> Regards,
> Stefani

Hi Stefani,

Please let us know how you intend to solve the problems you are facing
before you invest a lot of time developing the patch.

< snip >

-Frank

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web