[PATCH fixup v1] of: base: register DT root as device

[PATCH fixup v1] of: base: register DT root as device

[Oleksij Rempel]

A usual board file contains at least one of_machine_is_compatible().
Some of the have a rather long list with complicated version logic.

To avoid own implementation for driver management, register the root node
of device tree as platform device. So, the main platform bus can attach
proper board driver. After this patch a typical board.c file can reuse
existing driver infrastructure.

After this patch, you will be able to see all registered board drivers
with drvinfo as fallow:
...
board-embest-riot
board-protonic-imx6
    dt-root.of
...

With devinfo, you'll be able to get some board specific information,
if this is implemented:
barebox@Protonic PRTI6Q board:/ devinfo dt-root.of
Driver: board-protonic-imx6
Bus: platform
Parameters:
  boardid: 0 (type: uint32)
  boardrev: 1 (type: uint32)

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/of/base.c     | 2 ++
 drivers/of/platform.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4c633bcd49..6f6cdc9379 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2149,6 +2149,8 @@ int of_probe(void)
 	if (firmware)
 		of_platform_populate(firmware, NULL, NULL);
 
+	of_platform_device_create(root_node, NULL);
+
 	of_clk_init(root_node, NULL);
 	of_platform_populate(root_node, of_default_bus_match_table, NULL);
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ca84cede23..767beebe06 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -50,6 +50,11 @@ static void of_device_make_bus_id(struct device_d *dev)
 	const __be32 *reg;
 	u64 addr;
 
+	if (!node->parent) {
+		dev_set_name(dev, "dt-root.of");
+		return;
+	}
+
 	/* Construct the name, using parent nodes if necessary to ensure uniqueness */
 	while (node->parent) {
 		/*
-- 
2.28.0


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

Re: [PATCH fixup v1] of: base: register DT root as device

[Sascha Hauer]

On Wed, Aug 12, 2020 at 10:55:52AM +0200, Oleksij Rempel wrote:
> A usual board file contains at least one of_machine_is_compatible().
> Some of the have a rather long list with complicated version logic.
> 
> To avoid own implementation for driver management, register the root node
> of device tree as platform device. So, the main platform bus can attach
> proper board driver. After this patch a typical board.c file can reuse
> existing driver infrastructure.
> 
> After this patch, you will be able to see all registered board drivers
> with drvinfo as fallow:
> ...
> board-embest-riot
> board-protonic-imx6
>     dt-root.of
> ...
> 
> With devinfo, you'll be able to get some board specific information,
> if this is implemented:
> barebox@Protonic PRTI6Q board:/ devinfo dt-root.of
> Driver: board-protonic-imx6
> Bus: platform
> Parameters:
>   boardid: 0 (type: uint32)
>   boardrev: 1 (type: uint32)
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/of/base.c     | 2 ++
>  drivers/of/platform.c | 5 +++++
>  2 files changed, 7 insertions(+)

Replaced original patch with this one.

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 fixup v1] of: base: register DT root as device

[Ahmad Fatoum]

Hello,

On 8/12/20 10:55 AM, Oleksij Rempel wrote:
> A usual board file contains at least one of_machine_is_compatible().
> Some of the have a rather long list with complicated version logic.
> 
> To avoid own implementation for driver management, register the root node
> of device tree as platform device. So, the main platform bus can attach
> proper board driver. After this patch a typical board.c file can reuse
> existing driver infrastructure.
> 
> After this patch, you will be able to see all registered board drivers
> with drvinfo as fallow:
> ...
> board-embest-riot
> board-protonic-imx6
>     dt-root.of
> ...
> 
> With devinfo, you'll be able to get some board specific information,
> if this is implemented:
> barebox@Protonic PRTI6Q board:/ devinfo dt-root.of
> Driver: board-protonic-imx6
> Bus: platform
> Parameters:
>   boardid: 0 (type: uint32)
>   boardrev: 1 (type: uint32)
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/of/base.c     | 2 ++
>  drivers/of/platform.c | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 4c633bcd49..6f6cdc9379 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2149,6 +2149,8 @@ int of_probe(void)
>  	if (firmware)
>  		of_platform_populate(firmware, NULL, NULL);
>  
> +	of_platform_device_create(root_node, NULL);
> +
>  	of_clk_init(root_node, NULL);
>  	of_platform_populate(root_node, of_default_bus_match_table, NULL);
>  
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ca84cede23..767beebe06 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -50,6 +50,11 @@ static void of_device_make_bus_id(struct device_d *dev)
>  	const __be32 *reg;
>  	u64 addr;
>  
> +	if (!node->parent) {
> +		dev_set_name(dev, "dt-root.of");

Couldn't we drop the dt-? just let it be root.of.
dashes make use of device parameters less convenient should we
want to use those in future IIRC.

> +		return;
> +	}

of_platform_device_create does:

[-] check if device is available: not applicable to root node
[-] populate io resources: not applicable to root node
[-] use of_device_make_bus_id to get a name: not applicable to root node (prior to this patch)
[-] configure dma: not applicable to root node
[x] call platform_device_register

So why not just call platform_device_register yourself?
I'd assume you just need:

 dev = xzalloc(sizeof(*dev));
 dev->id = DEVICE_ID_SINGLE;
 dev->device_node = root_node;
 dev_set_name(dev, "dt-root.of")

 ret = platform_device_register(dev);
 if (!ret)
         return dev;


I'll test your patch layer anyway and report back if it fixes the issue I observed.
(Looks like it should though).

> +
>  	/* Construct the name, using parent nodes if necessary to ensure uniqueness */
>  	while (node->parent) {
>  		/*
> 

-- 
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 fixup v1] of: base: register DT root as device

[Ahmad Fatoum]

On 8/12/20 2:40 PM, Ahmad Fatoum wrote:
> I'll test your patch layer anyway and report back if it fixes the issue I observed.
> (Looks like it should though).

Ye, it works, but I'd prefer it be done differently.

Cheers
Ahmad
 

-- 
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 fixup v1] of: base: register DT root as device

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 3316 bytes --]

On Wed, Aug 12, 2020 at 02:40:05PM +0200, Ahmad Fatoum wrote:
> Hello,
> 
> On 8/12/20 10:55 AM, Oleksij Rempel wrote:
> > A usual board file contains at least one of_machine_is_compatible().
> > Some of the have a rather long list with complicated version logic.
> > 
> > To avoid own implementation for driver management, register the root node
> > of device tree as platform device. So, the main platform bus can attach
> > proper board driver. After this patch a typical board.c file can reuse
> > existing driver infrastructure.
> > 
> > After this patch, you will be able to see all registered board drivers
> > with drvinfo as fallow:
> > ...
> > board-embest-riot
> > board-protonic-imx6
> >     dt-root.of
> > ...
> > 
> > With devinfo, you'll be able to get some board specific information,
> > if this is implemented:
> > barebox@Protonic PRTI6Q board:/ devinfo dt-root.of
> > Driver: board-protonic-imx6
> > Bus: platform
> > Parameters:
> >   boardid: 0 (type: uint32)
> >   boardrev: 1 (type: uint32)
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/of/base.c     | 2 ++
> >  drivers/of/platform.c | 5 +++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 4c633bcd49..6f6cdc9379 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -2149,6 +2149,8 @@ int of_probe(void)
> >  	if (firmware)
> >  		of_platform_populate(firmware, NULL, NULL);
> >  
> > +	of_platform_device_create(root_node, NULL);
> > +
> >  	of_clk_init(root_node, NULL);
> >  	of_platform_populate(root_node, of_default_bus_match_table, NULL);
> >  
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index ca84cede23..767beebe06 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -50,6 +50,11 @@ static void of_device_make_bus_id(struct device_d *dev)
> >  	const __be32 *reg;
> >  	u64 addr;
> >  
> > +	if (!node->parent) {
> > +		dev_set_name(dev, "dt-root.of");
> 
> Couldn't we drop the dt-? just let it be root.of.
> dashes make use of device parameters less convenient should we
> want to use those in future IIRC.

dt is used to make clear: it is root of dt and not some random root of
what ever.

> > +		return;
> > +	}
> 
> of_platform_device_create does:
> 
> [-] check if device is available: not applicable to root node
> [-] populate io resources: not applicable to root node
> [-] use of_device_make_bus_id to get a name: not applicable to root node (prior to this patch)
> [-] configure dma: not applicable to root node
> [x] call platform_device_register

You make this assumption, just because this node has no parents?
Does it means, a parent less child may have no resources to do some work?
You should be ashamed of yourself! :D

But really, what prevents you to assign board specific resource to a
root node. It is just node as many others.

Regards,
Oleksij
-- 
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 |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

Re: [PATCH fixup v1] of: base: register DT root as device

[Ahmad Fatoum]

Hello,

On 8/12/20 5:13 PM, Oleksij Rempel wrote:
>>> +		dev_set_name(dev, "dt-root.of");
>>
>> Couldn't we drop the dt-? just let it be root.of.
>> dashes make use of device parameters less convenient should we
>> want to use those in future IIRC.
> 
> dt is used to make clear: it is root of dt and not some random root of
> what ever.

It's redundant, there is already a .of suffix.
I like machine.of more though.

>> of_platform_device_create does:
>>
>> [-] check if device is available: not applicable to root node
>> [-] populate io resources: not applicable to root node
>> [-] use of_device_make_bus_id to get a name: not applicable to root node (prior to this patch)
>> [-] configure dma: not applicable to root node
>> [x] call platform_device_register
> 
> You make this assumption, just because this node has no parents?
> Does it means, a parent less child may have no resources to do some work?
> You should be ashamed of yourself! :D
> 
> But really, what prevents you to assign board specific resource to a
> root node. It is just node as many others.

It makes no sense for the root node to have resources.
What is a machine-wide interrupt? Or a machine-wide MMIO region?
What size would that region even have, when you have no parent
bus that defines address/size cells?

Do you have any examples of oftree resources for the root node?

I'd rather not litter core code with an if-clause that evaluates to
true only once, to support your (IMHO wrong) use of a helper.

of_device_make_bus_id is taken from Linux and does per comment:

This routine will first try using the translated bus address to
derive a unique name. If it cannot, then it will prepend names from
parent nodes until a unique name can be derived.

IMO, it should stay that way.

Cheers
Ahmad


> 
> Regards,
> Oleksij
> 

-- 
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 fixup v1] of: base: register DT root as device

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 2865 bytes --]

On Wed, Aug 12, 2020 at 05:37:33PM +0200, Ahmad Fatoum wrote:
> Hello,
> 
> On 8/12/20 5:13 PM, Oleksij Rempel wrote:
> >>> +		dev_set_name(dev, "dt-root.of");
> >>
> >> Couldn't we drop the dt-? just let it be root.of.
> >> dashes make use of device parameters less convenient should we
> >> want to use those in future IIRC.
> > 
> > dt is used to make clear: it is root of dt and not some random root of
> > what ever.
> 
> It's redundant, there is already a .of suffix.
> I like machine.of more though.
> 
> >> of_platform_device_create does:
> >>
> >> [-] check if device is available: not applicable to root node
> >> [-] populate io resources: not applicable to root node
> >> [-] use of_device_make_bus_id to get a name: not applicable to root node (prior to this patch)
> >> [-] configure dma: not applicable to root node
> >> [x] call platform_device_register
> > 
> > You make this assumption, just because this node has no parents?
> > Does it means, a parent less child may have no resources to do some work?
> > You should be ashamed of yourself! :D
> > 
> > But really, what prevents you to assign board specific resource to a
> > root node. It is just node as many others.
> 
> It makes no sense for the root node to have resources.
> What is a machine-wide interrupt? Or a machine-wide MMIO region?
> What size would that region even have, when you have no parent
> bus that defines address/size cells?

yes, you are right.

> Do you have any examples of oftree resources for the root node?

Do you have any example of the root node used as device?

> I'd rather not litter core code with an if-clause that evaluates to
> true only once,

How many ifs are added in this patch and how many ifs will by added by
your suggestion? 

> to support your (IMHO wrong)  use of a helper.

Interesting change of conversation. Please stay technical.

> of_device_make_bus_id is taken from Linux and does per comment:
> 
> This routine will first try using the translated bus address to
> derive a unique name. If it cannot, then it will prepend names from
> parent nodes until a unique name can be derived.
> 
> IMO, it should stay that way.

Ok, i'll send a patch to rename of_device_make_bus_id to of_device_make_id.
In this case it will reflect new reality and keep the code readable.

If you have arguments in following topics:
- it will significantly affect performance
- it will affect size of executable
- it will affect maintainability

Please use them

Regards,
Oleksij
-- 
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 |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

Re: [PATCH fixup v1] of: base: register DT root as device

[Ahmad Fatoum]

Hi,

On 8/12/20 6:11 PM, Oleksij Rempel wrote:
>>> dt is used to make clear: it is root of dt and not some random root of
>>> what ever.
>>
>> It's redundant, there is already a .of suffix.
>> I like machine.of more though.

Makes me think. If we have a top level "dt-root" node,
won't this clash?


>> Do you have any examples of oftree resources for the root node?
> 
> Do you have any example of the root node used as device?

I am asking about an example of this particular usage in the root node
in device tree sources or bindings. You are asking about handling of the
device tree root node in the barebox device/driver model.

I don't see how that addresses my question.

>> I'd rather not litter core code with an if-clause that evaluates to
>> true only once,
> 
> How many ifs are added in this patch and how many ifs will by added by
> your suggestion? 

Mine adds none.

>> to support your (IMHO wrong)  use of a helper.
> 
> Interesting change of conversation. Please stay technical.

My technical opinion is that the helper is wrongly used.

>> of_device_make_bus_id is taken from Linux and does per comment:
>>
>> This routine will first try using the translated bus address to
>> derive a unique name. If it cannot, then it will prepend names from
>> parent nodes until a unique name can be derived.
>>
>> IMO, it should stay that way.
> 
> Ok, i'll send a patch to rename of_device_make_bus_id to of_device_make_id.
> In this case it will reflect new reality and keep the code readable.
> 
> If you have arguments in following topics:
> - it will significantly affect performance
> - it will affect size of executable
> - it will affect maintainability
> 
> Please use them

Shouldn't you be the one who argues in favor of his code?

It's ultimately Sascha's choice what to merge. To me this is
poor code, check my previous arguments.

Cheers,
Ahmad

> 
> Regards,
> Oleksij
> 

-- 
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 fixup v1] of: base: register DT root as device

[Lucas Stach]

Hi Oleksij,

Am Mittwoch, den 12.08.2020, 18:11 +0200 schrieb Oleksij Rempel:
> On Wed, Aug 12, 2020 at 05:37:33PM +0200, Ahmad Fatoum wrote:
> > Hello,
> > 
> > On 8/12/20 5:13 PM, Oleksij Rempel wrote:
> > > > > +		dev_set_name(dev, "dt-root.of");
> > > > 
> > > > Couldn't we drop the dt-? just let it be root.of.
> > > > dashes make use of device parameters less convenient should we
> > > > want to use those in future IIRC.
> > > 
> > > dt is used to make clear: it is root of dt and not some random root of
> > > what ever.
> > 
> > It's redundant, there is already a .of suffix.
> > I like machine.of more though.
> > 
> > > > of_platform_device_create does:
> > > > 
> > > > [-] check if device is available: not applicable to root node
> > > > [-] populate io resources: not applicable to root node
> > > > [-] use of_device_make_bus_id to get a name: not applicable to root node (prior to this patch)
> > > > [-] configure dma: not applicable to root node
> > > > [x] call platform_device_register
> > > 
> > > You make this assumption, just because this node has no parents?
> > > Does it means, a parent less child may have no resources to do some work?
> > > You should be ashamed of yourself! :D
> > > 
> > > But really, what prevents you to assign board specific resource to a
> > > root node. It is just node as many others.
> > 
> > It makes no sense for the root node to have resources.
> > What is a machine-wide interrupt? Or a machine-wide MMIO region?
> > What size would that region even have, when you have no parent
> > bus that defines address/size cells?
> 
> yes, you are right.
> 
> > Do you have any examples of oftree resources for the root node?
> 
> Do you have any example of the root node used as device?
> 
> > I'd rather not litter core code with an if-clause that evaluates to
> > true only once,
> 
> How many ifs are added in this patch and how many ifs will by added by
> your suggestion? 
> 
> > to support your (IMHO wrong)  use of a helper.
> 
> Interesting change of conversation. Please stay technical.
> 
> > of_device_make_bus_id is taken from Linux and does per comment:
> > 
> > This routine will first try using the translated bus address to
> > derive a unique name. If it cannot, then it will prepend names from
> > parent nodes until a unique name can be derived.
> > 
> > IMO, it should stay that way.
> 
> Ok, i'll send a patch to rename of_device_make_bus_id to of_device_make_id.
> In this case it will reflect new reality and keep the code readable.
> 
> If you have arguments in following topics:
> - it will significantly affect performance
> - it will affect size of executable
> - it will affect maintainability
> 
> Please use them

Please tone it down a bit. From my PoV most of Ahmad's points were
technical and quite to the point.

I guess the miscommunication between you two is that you are trying to
treat the root node just as every other node, while that's just not the
case. As Ahmad rightfully said the root node is quite special: the
device we are talking about here is not populated by a
of_platform_populate call as done for all the other nodes. The root
node can also not have resources on its own (phandles to other
resources yes, but no own resources) as it's the special node
describing how to interpret resource descriptions in all the other
child nodes.

Spreading the special cases used for the root node between different
areas of the codebase actually makes maintenance harder. It might be
obvious to you how things work together now, since you worked on those
areas recently, but in the long run people will have to piece this
knowledge together by reading different parts of the codebase. If we
can contain the special cases for the root node in a single place (by
doing all the custom device setup in of_probe) we should try to do so.

Renaming of_device_make_bus_id to of_device_make_id is a step in
totally the wrong direction, as it takes us further away from the
common ancestry in the Linux kernel codebase, just to cater for the
special case of the root node.

Also my vote is on the "machine.of" name for the device, but that's
really just bikeshedding, so please don't let this distract you from
the other, much more interesting points I tried to make in this mail.
;)

Regards,
Lucas


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

Re: [PATCH fixup v1] of: base: register DT root as device

[Oleksij Rempel]

On Wed, Aug 12, 2020 at 08:24:10PM +0200, Lucas Stach wrote:
> Hi Oleksij,
> 
> Am Mittwoch, den 12.08.2020, 18:11 +0200 schrieb Oleksij Rempel:
> > On Wed, Aug 12, 2020 at 05:37:33PM +0200, Ahmad Fatoum wrote:
> > > Hello,
> > > 
> > > On 8/12/20 5:13 PM, Oleksij Rempel wrote:
> > > > > > +		dev_set_name(dev, "dt-root.of");
> > > > > 
> > > > > Couldn't we drop the dt-? just let it be root.of.
> > > > > dashes make use of device parameters less convenient should we
> > > > > want to use those in future IIRC.
> > > > 
> > > > dt is used to make clear: it is root of dt and not some random root of
> > > > what ever.
> > > 
> > > It's redundant, there is already a .of suffix.
> > > I like machine.of more though.
> > > 
> > > > > of_platform_device_create does:
> > > > > 
> > > > > [-] check if device is available: not applicable to root node
> > > > > [-] populate io resources: not applicable to root node
> > > > > [-] use of_device_make_bus_id to get a name: not applicable to root node (prior to this patch)
> > > > > [-] configure dma: not applicable to root node
> > > > > [x] call platform_device_register
> > > > 
> > > > You make this assumption, just because this node has no parents?
> > > > Does it means, a parent less child may have no resources to do some work?
> > > > You should be ashamed of yourself! :D
> > > > 
> > > > But really, what prevents you to assign board specific resource to a
> > > > root node. It is just node as many others.
> > > 
> > > It makes no sense for the root node to have resources.
> > > What is a machine-wide interrupt? Or a machine-wide MMIO region?
> > > What size would that region even have, when you have no parent
> > > bus that defines address/size cells?
> > 
> > yes, you are right.
> > 
> > > Do you have any examples of oftree resources for the root node?
> > 
> > Do you have any example of the root node used as device?
> > 
> > > I'd rather not litter core code with an if-clause that evaluates to
> > > true only once,
> > 
> > How many ifs are added in this patch and how many ifs will by added by
> > your suggestion? 
> > 
> > > to support your (IMHO wrong)  use of a helper.
> > 
> > Interesting change of conversation. Please stay technical.
> > 
> > > of_device_make_bus_id is taken from Linux and does per comment:
> > > 
> > > This routine will first try using the translated bus address to
> > > derive a unique name. If it cannot, then it will prepend names from
> > > parent nodes until a unique name can be derived.
> > > 
> > > IMO, it should stay that way.
> > 
> > Ok, i'll send a patch to rename of_device_make_bus_id to of_device_make_id.
> > In this case it will reflect new reality and keep the code readable.
> > 
> > If you have arguments in following topics:
> > - it will significantly affect performance
> > - it will affect size of executable
> > - it will affect maintainability
> > 
> > Please use them
> 
> Please tone it down a bit. From my PoV most of Ahmad's points were
> technical and quite to the point.
> 
> I guess the miscommunication between you two is that you are trying to
> treat the root node just as every other node

ack

> while that's just not the
> case. As Ahmad rightfully said the root node is quite special: the
> device we are talking about here is not populated by a
> of_platform_populate call as done for all the other nodes. The root
> node can also not have resources on its own (phandles to other
> resources yes, but no own resources) as it's the special node
> describing how to interpret resource descriptions in all the other
> child nodes.

ack. And all of this resources are optional, so of_platform_populate()
can already without additional work handle lack of them, as on ever node
without resources.

> Spreading the special cases used for the root node between different
> areas of the codebase actually makes maintenance harder.

This will happen on any of this patch version. We will have a special case
which should be handled.

> It might be
> obvious to you how things work together now, since you worked on those
> areas recently, but in the long run people will have to piece this
> knowledge together by reading different parts of the codebase. If we
> can contain the special cases for the root node in a single place (by
> doing all the custom device setup in of_probe) we should try to do so.

currently, it is the single point i would agree. On other hand, we
already have a function which handles all of this exceptions. It is
common, mostly executed, perfectly tested code path.

As we already seen, this patch introduced an issue which was not detected
on some SoCs, because we have no exception handlers at this early stage.
It means, the reworked version, will never be tested in the same way as
the current version. Since testability is one aspect of maintainability,
i would prefer to add an comment to the code to clarify things.

> Renaming of_device_make_bus_id to of_device_make_id is a step in
> totally the wrong direction, as it takes us further away from the
> common ancestry in the Linux kernel codebase, just to cater for the
> special case of the root node.

Yes, but it was never an issue for us. Why now?
Any way, i can move this part of code out of of_device_make_bus_id()

> Also my vote is on the "machine.of" name for the device, but that's
> really just bikeshedding, so please don't let this distract you from
> the other, much more interesting points I tried to make in this mail.
> ;)

:)

Regards,
Oleksij
-- 
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 fixup v1] of: base: register DT root as device

[Sascha Hauer]

On Wed, Aug 12, 2020 at 10:55:52AM +0200, Oleksij Rempel wrote:
> A usual board file contains at least one of_machine_is_compatible().
> Some of the have a rather long list with complicated version logic.
> 
> To avoid own implementation for driver management, register the root node
> of device tree as platform device. So, the main platform bus can attach
> proper board driver. After this patch a typical board.c file can reuse
> existing driver infrastructure.
> 
> After this patch, you will be able to see all registered board drivers
> with drvinfo as fallow:
> ...
> board-embest-riot
> board-protonic-imx6
>     dt-root.of
> ...
> 
> With devinfo, you'll be able to get some board specific information,
> if this is implemented:
> barebox@Protonic PRTI6Q board:/ devinfo dt-root.of
> Driver: board-protonic-imx6
> Bus: platform
> Parameters:
>   boardid: 0 (type: uint32)
>   boardrev: 1 (type: uint32)
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/of/base.c     | 2 ++
>  drivers/of/platform.c | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 4c633bcd49..6f6cdc9379 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2149,6 +2149,8 @@ int of_probe(void)
>  	if (firmware)
>  		of_platform_populate(firmware, NULL, NULL);
>  
> +	of_platform_device_create(root_node, NULL);

I am with Ahmad here: The bulk of of_platform_device_create() is unused
when called with the root node...

> +
>  	of_clk_init(root_node, NULL);
>  	of_platform_populate(root_node, of_default_bus_match_table, NULL);
>  
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ca84cede23..767beebe06 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -50,6 +50,11 @@ static void of_device_make_bus_id(struct device_d *dev)
>  	const __be32 *reg;
>  	u64 addr;
>  
> +	if (!node->parent) {
> +		dev_set_name(dev, "dt-root.of");
> +		return;
> +	}

...and one of the few things that it actually does needs patching.
Please open code creating and registering the device for the root 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