* [PATCH fixup v1] of: base: register DT root as device @ 2020-08-12 8:55 Oleksij Rempel 2020-08-12 9:42 ` Sascha Hauer ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Oleksij Rempel @ 2020-08-12 8:55 UTC (permalink / raw) To: barebox; +Cc: 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 8:55 [PATCH fixup v1] of: base: register DT root as device Oleksij Rempel @ 2020-08-12 9:42 ` Sascha Hauer 2020-08-12 12:40 ` Ahmad Fatoum 2020-08-17 4:45 ` Sascha Hauer 2 siblings, 0 replies; 11+ messages in thread From: Sascha Hauer @ 2020-08-12 9:42 UTC (permalink / raw) To: Oleksij Rempel; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 8:55 [PATCH fixup v1] of: base: register DT root as device Oleksij Rempel 2020-08-12 9:42 ` Sascha Hauer @ 2020-08-12 12:40 ` Ahmad Fatoum 2020-08-12 13:08 ` Ahmad Fatoum 2020-08-12 15:13 ` Oleksij Rempel 2020-08-17 4:45 ` Sascha Hauer 2 siblings, 2 replies; 11+ messages in thread From: Ahmad Fatoum @ 2020-08-12 12:40 UTC (permalink / raw) To: Oleksij Rempel, barebox 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 12:40 ` Ahmad Fatoum @ 2020-08-12 13:08 ` Ahmad Fatoum 2020-08-12 15:13 ` Oleksij Rempel 1 sibling, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2020-08-12 13:08 UTC (permalink / raw) To: Oleksij Rempel, barebox; +Cc: has 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 12:40 ` Ahmad Fatoum 2020-08-12 13:08 ` Ahmad Fatoum @ 2020-08-12 15:13 ` Oleksij Rempel 2020-08-12 15:37 ` Ahmad Fatoum 1 sibling, 1 reply; 11+ messages in thread From: Oleksij Rempel @ 2020-08-12 15:13 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox [-- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 15:13 ` Oleksij Rempel @ 2020-08-12 15:37 ` Ahmad Fatoum 2020-08-12 16:11 ` Oleksij Rempel 0 siblings, 1 reply; 11+ messages in thread From: Ahmad Fatoum @ 2020-08-12 15:37 UTC (permalink / raw) To: Oleksij Rempel; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 15:37 ` Ahmad Fatoum @ 2020-08-12 16:11 ` Oleksij Rempel 2020-08-12 18:15 ` Ahmad Fatoum 2020-08-12 18:24 ` Lucas Stach 0 siblings, 2 replies; 11+ messages in thread From: Oleksij Rempel @ 2020-08-12 16:11 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox [-- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 16:11 ` Oleksij Rempel @ 2020-08-12 18:15 ` Ahmad Fatoum 2020-08-12 18:24 ` Lucas Stach 1 sibling, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2020-08-12 18:15 UTC (permalink / raw) To: Oleksij Rempel; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 16:11 ` Oleksij Rempel 2020-08-12 18:15 ` Ahmad Fatoum @ 2020-08-12 18:24 ` Lucas Stach 2020-08-13 4:45 ` Oleksij Rempel 1 sibling, 1 reply; 11+ messages in thread From: Lucas Stach @ 2020-08-12 18:24 UTC (permalink / raw) To: Oleksij Rempel, Ahmad Fatoum; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 18:24 ` Lucas Stach @ 2020-08-13 4:45 ` Oleksij Rempel 0 siblings, 0 replies; 11+ messages in thread From: Oleksij Rempel @ 2020-08-13 4:45 UTC (permalink / raw) To: Lucas Stach; +Cc: barebox, Ahmad Fatoum 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH fixup v1] of: base: register DT root as device 2020-08-12 8:55 [PATCH fixup v1] of: base: register DT root as device Oleksij Rempel 2020-08-12 9:42 ` Sascha Hauer 2020-08-12 12:40 ` Ahmad Fatoum @ 2020-08-17 4:45 ` Sascha Hauer 2 siblings, 0 replies; 11+ messages in thread From: Sascha Hauer @ 2020-08-17 4:45 UTC (permalink / raw) To: Oleksij Rempel; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-08-17 4:45 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-12 8:55 [PATCH fixup v1] of: base: register DT root as device Oleksij Rempel 2020-08-12 9:42 ` Sascha Hauer 2020-08-12 12:40 ` Ahmad Fatoum 2020-08-12 13:08 ` Ahmad Fatoum 2020-08-12 15:13 ` Oleksij Rempel 2020-08-12 15:37 ` Ahmad Fatoum 2020-08-12 16:11 ` Oleksij Rempel 2020-08-12 18:15 ` Ahmad Fatoum 2020-08-12 18:24 ` Lucas Stach 2020-08-13 4:45 ` Oleksij Rempel 2020-08-17 4:45 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox