diff mbox series

[v7,1/8] of/platform: Allow overlays to create platform devices from the root node

Message ID 20240710201246.1802189-2-sboyd@kernel.org (mailing list archive)
State Accepted, archived
Headers show
Series clk: Add kunit tests for fixed rate and parent data | expand

Commit Message

Stephen Boyd July 10, 2024, 8:12 p.m. UTC 
We'd like to apply overlays to the root node in KUnit so we can test
platform devices created as children of the root node.

On some architectures (powerpc), the root node isn't marked with
OF_POPULATED_BUS. If an overlay tries to modify the root node on these
platforms it will fail, while on other platforms, such as ARM, it will
succeed. This is because the root node is marked with OF_POPULATED_BUS
by of_platform_default_populate_init() calling
of_platform_default_populate() with NULL as the first argument.

Loosen the requirement here so that platform devices can be created for
nodes created as children of the root node via DT overlays even if the
platform bus wasn't populated for the root node.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/platform.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Stephen Boyd July 15, 2024, 7:24 p.m. UTC | #1 
Quoting Stephen Boyd (2024-07-10 13:12:37)
> We'd like to apply overlays to the root node in KUnit so we can test
> platform devices created as children of the root node.
> 
> On some architectures (powerpc), the root node isn't marked with
> OF_POPULATED_BUS. If an overlay tries to modify the root node on these
> platforms it will fail, while on other platforms, such as ARM, it will
> succeed. This is because the root node is marked with OF_POPULATED_BUS
> by of_platform_default_populate_init() calling
> of_platform_default_populate() with NULL as the first argument.
> 
> Loosen the requirement here so that platform devices can be created for
> nodes created as children of the root node via DT overlays even if the
> platform bus wasn't populated for the root node.
> 
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---

Applied to clk-next
Geert Uytterhoeven July 16, 2024, 9:48 a.m. UTC | #2 
Hi Stephen,

On Wed, Jul 10, 2024 at 10:14 PM Stephen Boyd <sboyd@kernel.org> wrote:
> We'd like to apply overlays to the root node in KUnit so we can test
> platform devices created as children of the root node.
>
> On some architectures (powerpc), the root node isn't marked with
> OF_POPULATED_BUS. If an overlay tries to modify the root node on these
> platforms it will fail, while on other platforms, such as ARM, it will
> succeed. This is because the root node is marked with OF_POPULATED_BUS
> by of_platform_default_populate_init() calling
> of_platform_default_populate() with NULL as the first argument.
>
> Loosen the requirement here so that platform devices can be created for
> nodes created as children of the root node via DT overlays even if the
> platform bus wasn't populated for the root node.
>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>

Thanks for your patch, which is now commit 98290f295fbcf18f
("of/platform: Allow overlays to create platform devices from the
root node") in clk/clk-next.

This causes i2c-demux-pinctrl to fail on the Koelsch development board:

        i2c-demux-pinctrl i2c-mux1: failed to setup demux-adapter 0 (-19)
        i2c-demux-pinctrl i2c-mux2: failed to setup demux-adapter 0 (-19)
        i2c-demux-pinctrl i2c-mux3: failed to setup demux-adapter 0 (-19)
        i2c-demux-pinctrl i2c-mux2: Failed to create device link
(0x180) with e6ef0000.video
        i2c-demux-pinctrl i2c-mux2: Failed to create device link
(0x180) with e6ef1000.video
        i2c-demux-pinctrl i2c-mux2: Failed to create device link
(0x180) with hdmi-in
        i2c-demux-pinctrl i2c-mux2: Failed to create device link
(0x180) with hdmi-out

and anything relying on I2C connected to these muxes fails, too.

Also, loading the 25LC040 DT overlay[1] on Ebisu using the out-of-tree
of-configfs now fails, too.

> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -732,11 +732,14 @@ static int of_platform_notify(struct notifier_block *nb,
>         struct of_reconfig_data *rd = arg;
>         struct platform_device *pdev_parent, *pdev;
>         bool children_left;
> +       struct device_node *parent;
>
>         switch (of_reconfig_get_state_change(action, rd)) {
>         case OF_RECONFIG_CHANGE_ADD:
> -               /* verify that the parent is a bus */
> -               if (!of_node_check_flag(rd->dn->parent, OF_POPULATED_BUS))
> +               parent = rd->dn->parent;
> +               /* verify that the parent is a bus (or the root node) */
> +               if (!of_node_is_root(parent) &&

Parent = /soc, so this returns early. Hence of_changeset_apply() [2]
didn't add the I2C mux bus, causing of_get_i2c_adapter_by_node() [3]
to fail.

> +                   of_node_check_flag(parent, OF_POPULATED_BUS))

Oh, you inverted the check for of_node_check_flag(); was that
intentional?  Re-adding the "!" fixes all issues for me.

>                         return NOTIFY_OK;       /* not for us */
>
>                 /* already populated? (driver using of_populate manually) */
> @@ -749,7 +752,7 @@ static int of_platform_notify(struct notifier_block *nb,
>                  */
>                 rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
>                 /* pdev_parent may be NULL when no bus platform device */
> -               pdev_parent = of_find_device_by_node(rd->dn->parent);
> +               pdev_parent = of_find_device_by_node(parent);
>                 pdev = of_platform_device_create(rd->dn, NULL,
>                                 pdev_parent ? &pdev_parent->dev : NULL);
>                 platform_device_put(pdev_parent);

[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tree/arch/arm64/boot/dts/renesas/r8a77990-ebisu-cn41-msiof0-25lc040.dtso?h=topic/renesas-overlays
[2] https://elixir.bootlin.com/linux/latest/source/drivers/i2c/muxes/i2c-demux-pinctrl.c#L60
[3] https://elixir.bootlin.com/linux/latest/source/drivers/i2c/muxes/i2c-demux-pinctrl.c#L64

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven July 16, 2024, 11:23 a.m. UTC | #3 
On Tue, Jul 16, 2024 at 11:48 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Jul 10, 2024 at 10:14 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > We'd like to apply overlays to the root node in KUnit so we can test
> > platform devices created as children of the root node.
> >
> > On some architectures (powerpc), the root node isn't marked with
> > OF_POPULATED_BUS. If an overlay tries to modify the root node on these
> > platforms it will fail, while on other platforms, such as ARM, it will
> > succeed. This is because the root node is marked with OF_POPULATED_BUS
> > by of_platform_default_populate_init() calling
> > of_platform_default_populate() with NULL as the first argument.
> >
> > Loosen the requirement here so that platform devices can be created for
> > nodes created as children of the root node via DT overlays even if the
> > platform bus wasn't populated for the root node.
> >
> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>

> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -732,11 +732,14 @@ static int of_platform_notify(struct notifier_block *nb,
> >         struct of_reconfig_data *rd = arg;
> >         struct platform_device *pdev_parent, *pdev;
> >         bool children_left;
> > +       struct device_node *parent;
> >
> >         switch (of_reconfig_get_state_change(action, rd)) {
> >         case OF_RECONFIG_CHANGE_ADD:
> > -               /* verify that the parent is a bus */
> > -               if (!of_node_check_flag(rd->dn->parent, OF_POPULATED_BUS))
> > +               parent = rd->dn->parent;
> > +               /* verify that the parent is a bus (or the root node) */
> > +               if (!of_node_is_root(parent) &&
> > +                   of_node_check_flag(parent, OF_POPULATED_BUS))
>
> Oh, you inverted the check for of_node_check_flag(); was that
> intentional?  Re-adding the "!" fixes all issues for me.

I have sent a fix.
https://lore.kernel.org/a9ada686e1f1c6f496e423deaf108f1bcfd94d7d.1721123679.git.geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..bda6da866cc8 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -732,11 +732,14 @@  static int of_platform_notify(struct notifier_block *nb,
 	struct of_reconfig_data *rd = arg;
 	struct platform_device *pdev_parent, *pdev;
 	bool children_left;
+	struct device_node *parent;
 
 	switch (of_reconfig_get_state_change(action, rd)) {
 	case OF_RECONFIG_CHANGE_ADD:
-		/* verify that the parent is a bus */
-		if (!of_node_check_flag(rd->dn->parent, OF_POPULATED_BUS))
+		parent = rd->dn->parent;
+		/* verify that the parent is a bus (or the root node) */
+		if (!of_node_is_root(parent) &&
+		    of_node_check_flag(parent, OF_POPULATED_BUS))
 			return NOTIFY_OK;	/* not for us */
 
 		/* already populated? (driver using of_populate manually) */
@@ -749,7 +752,7 @@  static int of_platform_notify(struct notifier_block *nb,
 		 */
 		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		/* pdev_parent may be NULL when no bus platform device */
-		pdev_parent = of_find_device_by_node(rd->dn->parent);
+		pdev_parent = of_find_device_by_node(parent);
 		pdev = of_platform_device_create(rd->dn, NULL,
 				pdev_parent ? &pdev_parent->dev : NULL);
 		platform_device_put(pdev_parent);