Re: [PATCH 1/2] of: base: parse all available memory nodes
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- | ___ 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
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- | ___ 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
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- | > > ___ > 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- | ___ 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
Hi Sascha,
- On 23 Mar, 2020, at 11:12, Sascha Hauer s.ha...@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
>> ---
>> 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- |
___
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
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
> ---
> 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- |
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
