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

[Oleksij Rempel <o.rempel@pengutronix.de>]

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