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

[Lucas Stach <l.stach@pengutronix.de>]

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