[PATCH 0/2] Allow parsing more than one memory node

[PATCH 0/2] Allow parsing more than one memory node

[Clement Leger]

Currently, barebox can only parse one memory node. This means that
all other memory nodes (with device_type == "memory") will be ignored
while parsing memory.
This patchset add a function of_probe_memory which will call
of_add_memory for each found memory node. Additionally, of_add_memory
has been modified to allow being called multiple times and correctly
add all memory banks.

Clement Leger (2):
  of: base: parse all available memory nodes
  of: base: allow of_add_memory to be called multiple times

 drivers/of/base.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

-- 
2.15.0.276.g89ea799


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

[PATCH 1/2] of: base: parse all available memory nodes

[Clement Leger]

Currently, barebox only parse one memory node which is either the
"/memory" node or the first node with device_type == "memory".
However, the use of multiple memory nodes with device_type = "memory"
property is allowed by the device tree specification and already
correctly parsed by Linux kernel.
In order to fix that, add of_probe_memory function which loop over all
available memory nodes. In order to try to keep existing legacy search
based on "memory" node name, try to find this node and add it. If the
memory node contains the device_type property, then it will only be
added once.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 drivers/of/base.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9ede05227..b1a96ee8f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2061,9 +2061,32 @@ const struct of_device_id of_default_bus_match_table[] = {
 	}
 };
 
+static void of_probe_memory(void)
+{
+	struct device_node *memory = root_node, *legacy_memory;
+
+	/* Parse node based on name (for legacy dt) */
+	legacy_memory = of_find_node_by_path("/memory");
+	if (legacy_memory)
+		of_add_memory(legacy_memory, false);
+
+	/* Then, parse all available node with "memory" device_type */
+	while (1) {
+		memory = of_find_node_by_type(memory, "memory");
+		if (!memory)
+			break;
+
+		/* Skip potentially already added legacy memory node */
+		if (memory == legacy_memory)
+			continue;
+
+		of_add_memory(memory, false);
+	}
+}
+
 int of_probe(void)
 {
-	struct device_node *memory, *firmware;
+	struct device_node *firmware;
 
 	if(!root_node)
 		return -ENODEV;
@@ -2074,11 +2097,7 @@ int of_probe(void)
 	if (of_model)
 		barebox_set_model(of_model);
 
-	memory = of_find_node_by_path("/memory");
-	if (!memory)
-		memory = of_find_node_by_type(root_node, "memory");
-	if (memory)
-		of_add_memory(memory, false);
+	of_probe_memory();
 
 	firmware = of_find_node_by_path("/firmware");
 	if (firmware)
-- 
2.15.0.276.g89ea799


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

Re: [PATCH 1/2] of: base: parse all available memory nodes

[Sascha Hauer]

On Mon, Mar 16, 2020 at 12:00:07PM +0100, Clement Leger wrote:
> Currently, barebox only parse one memory node which is either the
> "/memory" node or the first node with device_type == "memory".
> However, the use of multiple memory nodes with device_type = "memory"
> property is allowed by the device tree specification and already
> correctly parsed by Linux kernel.
> In order to fix that, add of_probe_memory function which loop over all
> available memory nodes. In order to try to keep existing legacy search
> based on "memory" node name, try to find this node and add it. If the
> memory node contains the device_type property, then it will only be
> added once.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
>  drivers/of/base.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 9ede05227..b1a96ee8f 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2061,9 +2061,32 @@ const struct of_device_id of_default_bus_match_table[] = {
>  	}
>  };
>  
> +static void of_probe_memory(void)
> +{
> +	struct device_node *memory = root_node, *legacy_memory;
> +
> +	/* Parse node based on name (for legacy dt) */
> +	legacy_memory = of_find_node_by_path("/memory");
> +	if (legacy_memory)
> +		of_add_memory(legacy_memory, false);
> +
> +	/* Then, parse all available node with "memory" device_type */
> +	while (1) {
> +		memory = of_find_node_by_type(memory, "memory");
> +		if (!memory)
> +			break;
> +
> +		/* Skip potentially already added legacy memory node */
> +		if (memory == legacy_memory)
> +			continue;
> +
> +		of_add_memory(memory, false);
> +	}

AFAIK the device_type = "memory" property was mandatory in the early
days as well, there shouldn't be any /memory nodes without this
property. Given that, is the add-legacy-node-first still necessary?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

Re: [PATCH 1/2] of: base: parse all available memory nodes

[Clément Leger]

Hi Sascha,

----- On 23 Mar, 2020, at 11:12, Sascha Hauer s.hauer@pengutronix.de wrote:

> On Mon, Mar 16, 2020 at 12:00:07PM +0100, Clement Leger wrote:
>> Currently, barebox only parse one memory node which is either the
>> "/memory" node or the first node with device_type == "memory".
>> However, the use of multiple memory nodes with device_type = "memory"
>> property is allowed by the device tree specification and already
>> correctly parsed by Linux kernel.
>> In order to fix that, add of_probe_memory function which loop over all
>> available memory nodes. In order to try to keep existing legacy search
>> based on "memory" node name, try to find this node and add it. If the
>> memory node contains the device_type property, then it will only be
>> added once.
>> 
>> Signed-off-by: Clement Leger <cleger@kalray.eu>
>> ---
>>  drivers/of/base.c | 31 +++++++++++++++++++++++++------
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 9ede05227..b1a96ee8f 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -2061,9 +2061,32 @@ const struct of_device_id of_default_bus_match_table[] =
>> {
>>  	}
>>  };
>>  
>> +static void of_probe_memory(void)
>> +{
>> +	struct device_node *memory = root_node, *legacy_memory;
>> +
>> +	/* Parse node based on name (for legacy dt) */
>> +	legacy_memory = of_find_node_by_path("/memory");
>> +	if (legacy_memory)
>> +		of_add_memory(legacy_memory, false);
>> +
>> +	/* Then, parse all available node with "memory" device_type */
>> +	while (1) {
>> +		memory = of_find_node_by_type(memory, "memory");
>> +		if (!memory)
>> +			break;
>> +
>> +		/* Skip potentially already added legacy memory node */
>> +		if (memory == legacy_memory)
>> +			continue;
>> +
>> +		of_add_memory(memory, false);
>> +	}
> 
> AFAIK the device_type = "memory" property was mandatory in the early
> days as well, there shouldn't be any /memory nodes without this
> property. Given that, is the add-legacy-node-first still necessary?

Agreed, I did that after speaking with someone on IRC which stated
(rightfully), that I should keep the legacy behavior if possible.
However, I agree that since the device_type = "memory" property should
have always been there, there is no reason to keep this behavior.

If you are ok with that, I will remove this chunk of code.

> 
> Sascha
> 
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 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

Re: [PATCH 1/2] of: base: parse all available memory nodes

[Ahmad Fatoum]

Hi,

On 3/23/20 11:21 AM, Clément Leger wrote:
>> AFAIK the device_type = "memory" property was mandatory in the early
>> days as well, there shouldn't be any /memory nodes without this
>> property. Given that, is the add-legacy-node-first still necessary?
> 
> Agreed, I did that after speaking with someone on IRC which stated
> (rightfully), that I should keep the legacy behavior if possible.

My concern was about HW that passes a device tree to barebox (PowerPC
possibly?), where it might have worked before and now won't anymore.

If this is no real concern, it can be removed.

Cheers
Ahmad

> However, I agree that since the device_type = "memory" property should
> have always been there, there is no reason to keep this behavior.
> 
> If you are ok with that, I will remove this chunk of code.
> 
>>
>> Sascha
>>
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 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
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

Re: [PATCH 1/2] of: base: parse all available memory nodes

[Sascha Hauer]

On Mon, Mar 23, 2020 at 11:31:26AM +0100, Ahmad Fatoum wrote:
> Hi,
> 
> On 3/23/20 11:21 AM, Clément Leger wrote:
> >> AFAIK the device_type = "memory" property was mandatory in the early
> >> days as well, there shouldn't be any /memory nodes without this
> >> property. Given that, is the add-legacy-node-first still necessary?
> > 
> > Agreed, I did that after speaking with someone on IRC which stated
> > (rightfully), that I should keep the legacy behavior if possible.
> 
> My concern was about HW that passes a device tree to barebox (PowerPC
> possibly?), where it might have worked before and now won't anymore.
> 
> If this is no real concern, it can be removed.

There is no PowerPC hardware supported by barebox that passes a device
tree to barebox. I don't think this is necessary.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

Re: [PATCH 1/2] of: base: parse all available memory nodes

[Ahmad Fatoum]

On 3/25/20 10:29 AM, Sascha Hauer wrote:
> On Mon, Mar 23, 2020 at 11:31:26AM +0100, Ahmad Fatoum wrote:
>> Hi,
>>
>> On 3/23/20 11:21 AM, Clément Leger wrote:
>>>> AFAIK the device_type = "memory" property was mandatory in the early
>>>> days as well, there shouldn't be any /memory nodes without this
>>>> property. Given that, is the add-legacy-node-first still necessary?
>>>
>>> Agreed, I did that after speaking with someone on IRC which stated
>>> (rightfully), that I should keep the legacy behavior if possible.
>>
>> My concern was about HW that passes a device tree to barebox (PowerPC
>> possibly?), where it might have worked before and now won't anymore.
>>
>> If this is no real concern, it can be removed.
> 
> There is no PowerPC hardware supported by barebox that passes a device
> tree to barebox. I don't think this is necessary.

Turns out we had quite a few device trees that lacked a device_type = "memory"
property: https://lists.infradead.org/pipermail/barebox/2020-June/042269.html

The property used to be there, but when upstream removed skeleton.dtsi and
either there was no upstream memory node to extend or it was renamed, we
ended up with an invalid memory node.


> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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 2/2] of: base: allow of_add_memory to be called multiple times

[Clement Leger]

Currently, of_add_memory can't be called multiple times because it will
always create memory banks by restarting at value 0. This means that, when
adding a second memory bank by calling again of_add_memory, it will be
named ram0 and overwrite the previous one. Fix that by using a static
variable to store the current memory bank number in order to be preserved
from one call to another.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 drivers/of/base.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b1a96ee8f..12c58bfb6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2019,6 +2019,8 @@ int of_set_property(struct device_node *np, const char *name, const void *val, i
 	return 0;
 }
 
+static int mem_bank_num;
+
 int of_add_memory(struct device_node *node, bool dump)
 {
 	const char *device_type;
@@ -2030,14 +2032,13 @@ int of_add_memory(struct device_node *node, bool dump)
 		return -ENXIO;
 
 	while (!of_address_to_resource(node, n, &res)) {
-		if (!resource_size(&res)) {
-			n++;
+		n++;
+		if (!resource_size(&res))
 			continue;
-		}
 
-		of_add_memory_bank(node, dump, n,
+		of_add_memory_bank(node, dump, mem_bank_num,
 				res.start, resource_size(&res));
-		n++;
+		mem_bank_num++;
 	}
 
 	return 0;
-- 
2.15.0.276.g89ea799


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

[PATCH v2 0/2] Allow parsing more than one memory node

[Clement Leger]

Currently, barebox can only parse one memory node. This means that
all other memory nodes (with device_type == "memory") will be ignored
while parsing memory.
This patchset add a function of_probe_memory which will call
of_add_memory for each found memory node. Additionally, of_add_memory
has been modified to allow being called multiple times and correctly
add all memory banks.

Changes v1 -> v2
 - Reorder patches
 - Remove legacy "memory" node handling

Clement Leger (2):
  of: base: allow of_add_memory to be called multiple times
  of: base: parse all available memory nodes

 drivers/of/base.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

-- 
2.17.1


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

[PATCH v2 1/2] of: base: allow of_add_memory to be called multiple times

[Clement Leger]

Currently, of_add_memory can't be called multiple times because it will
always create memory banks by restarting at value 0. This means that, when
adding a second memory bank by calling again of_add_memory, it will be
named ram0 and overwrite the previous one. Fix that by using a static
variable to store the current memory bank number in order to be preserved
from one call to another.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 drivers/of/base.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9ede05227..2ea8d7516 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2019,6 +2019,8 @@ int of_set_property(struct device_node *np, const char *name, const void *val, i
 	return 0;
 }
 
+static int mem_bank_num;
+
 int of_add_memory(struct device_node *node, bool dump)
 {
 	const char *device_type;
@@ -2030,14 +2032,13 @@ int of_add_memory(struct device_node *node, bool dump)
 		return -ENXIO;
 
 	while (!of_address_to_resource(node, n, &res)) {
-		if (!resource_size(&res)) {
-			n++;
+		n++;
+		if (!resource_size(&res))
 			continue;
-		}
 
-		of_add_memory_bank(node, dump, n,
+		of_add_memory_bank(node, dump, mem_bank_num,
 				res.start, resource_size(&res));
-		n++;
+		mem_bank_num++;
 	}
 
 	return 0;
-- 
2.17.1


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

[PATCH v2 2/2] of: base: parse all available memory nodes

[Clement Leger]

Currently, barebox only parse one memory node which is either the
"/memory" node or the first node with device_type == "memory".
However, the use of multiple memory nodes with device_type = "memory"
property is allowed by the device tree specification and already
correctly parsed by Linux kernel.
In order to fix that, add of_probe_memory function which loop over all
available memory nodes matching device_type == "memory".

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 drivers/of/base.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2ea8d7516..6d13b66a0 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2062,9 +2062,23 @@ const struct of_device_id of_default_bus_match_table[] = {
 	}
 };
 
+static void of_probe_memory(void)
+{
+	struct device_node *memory = root_node;
+
+	/* Parse all available node with "memory" device_type */
+	while (1) {
+		memory = of_find_node_by_type(memory, "memory");
+		if (!memory)
+			break;
+
+		of_add_memory(memory, false);
+	}
+}
+
 int of_probe(void)
 {
-	struct device_node *memory, *firmware;
+	struct device_node *firmware;
 
 	if(!root_node)
 		return -ENODEV;
@@ -2075,11 +2089,7 @@ int of_probe(void)
 	if (of_model)
 		barebox_set_model(of_model);
 
-	memory = of_find_node_by_path("/memory");
-	if (!memory)
-		memory = of_find_node_by_type(root_node, "memory");
-	if (memory)
-		of_add_memory(memory, false);
+	of_probe_memory();
 
 	firmware = of_find_node_by_path("/firmware");
 	if (firmware)
-- 
2.17.1


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

Re: [PATCH v2 0/2] Allow parsing more than one memory node

[Sascha Hauer]

On Wed, Mar 25, 2020 at 06:27:35PM +0100, Clement Leger wrote:
> Currently, barebox can only parse one memory node. This means that
> all other memory nodes (with device_type == "memory") will be ignored
> while parsing memory.
> This patchset add a function of_probe_memory which will call
> of_add_memory for each found memory node. Additionally, of_add_memory
> has been modified to allow being called multiple times and correctly
> add all memory banks.
> 
> Changes v1 -> v2
>  - Reorder patches
>  - Remove legacy "memory" node handling
> 
> Clement Leger (2):
>   of: base: allow of_add_memory to be called multiple times
>   of: base: parse all available memory nodes

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

Re: [PATCH 0/2] Allow parsing more than one memory node

[Sam Ravnborg]

Hi Clement.

On Mon, Mar 16, 2020 at 12:00:06PM +0100, Clement Leger wrote:
> Currently, barebox can only parse one memory node. This means that
> all other memory nodes (with device_type == "memory") will be ignored
> while parsing memory.
> This patchset add a function of_probe_memory which will call
> of_add_memory for each found memory node. Additionally, of_add_memory
> has been modified to allow being called multiple times and correctly
> add all memory banks.
> 
> Clement Leger (2):
>   of: base: parse all available memory nodes
>   of: base: allow of_add_memory to be called multiple times

With this order of the patches, after applying patch 1 the
code will then use the last and not the first memory node.
And only after the second patch it will work.

I think to improve bisectability the order of the two patches should be
reversed.

	Sam

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

Re: [PATCH 0/2] Allow parsing more than one memory node

[Clément Leger]

Hi Sam,

----- On 17 Mar, 2020, at 08:35, Sam Ravnborg sam@ravnborg.org wrote:

> Hi Clement.
> 
> On Mon, Mar 16, 2020 at 12:00:06PM +0100, Clement Leger wrote:
>> Currently, barebox can only parse one memory node. This means that
>> all other memory nodes (with device_type == "memory") will be ignored
>> while parsing memory.
>> This patchset add a function of_probe_memory which will call
>> of_add_memory for each found memory node. Additionally, of_add_memory
>> has been modified to allow being called multiple times and correctly
>> add all memory banks.
>> 
>> Clement Leger (2):
>>   of: base: parse all available memory nodes
>>   of: base: allow of_add_memory to be called multiple times
> 
> With this order of the patches, after applying patch 1 the
> code will then use the last and not the first memory node.
> And only after the second patch it will work.
> 
> I think to improve bisectability the order of the two patches should be
> reversed.

Oh yes indeed ! I will send a V2 after others have commented it.

> 
> 	Sam

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