| Message ID | 20240422232404.213174-1-sboyd@kernel.org |
|---|---|
| Headers | show |
| Series | clk: Add kunit tests for fixed rate and parent data | expand |
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote: > > This patch series adds unit tests for the clk fixed rate basic type and > the clk registration functions that use struct clk_parent_data. To get > there, we add support for loading device tree overlays onto the live DTB > along with probing platform drivers to bind to device nodes in the > overlays. With this series, we're able to exercise some of the code in > the common clk framework that uses devicetree lookups to find parents > and the fixed rate clk code that scans device tree directly and creates > clks. Please review. > > I Cced everyone to all the patches so they get the full context. I'm > hoping I can take the whole pile through the clk tree as they all build > upon each other. Or the DT part can be merged through the DT tree to > reduce the dependencies. > > Changes from v3 (https://lore.kernel.org/r/20230327222159.3509818-1-sboyd@kernel.org): > * No longer depend on Frank's series[1] because it was merged upstream[2] > * Use kunit_add_action_or_reset() to shorten code > * Skip tests properly when CONFIG_OF_OVERLAY isn't set > > Changes from v2 (https://lore.kernel.org/r/20230315183729.2376178-1-sboyd@kernel.org): > * Overlays don't depend on __symbols__ node > * Depend on Frank's always create root node if CONFIG_OF series[1] > * Added kernel-doc to KUnit API doc > * Fixed some kernel-doc on functions > * More test cases for fixed rate clk > > Changes from v1 (https://lore.kernel.org/r/20230302013822.1808711-1-sboyd@kernel.org): > * Don't depend on UML, use unittest data approach to attach nodes > * Introduce overlay loading API for KUnit > * Move platform_device KUnit code to drivers/base/test > * Use #define macros for constants shared between unit tests and > overlays > * Settle on "test" as a vendor prefix > * Make KUnit wrappers have "_kunit" postfix > > [1] https://lore.kernel.org/r/20230317053415.2254616-1-frowand.list@gmail.com > [2] https://lore.kernel.org/r/20240308195737.GA1174908-robh@kernel.org > Thanks very much. I'm about halfway through reviewing these, and I like them a lot so far. Most of my thoughts are just naming ideas. I fear some of them may be the reverse of previous suggestions, as we've since landed the KUnit device wrappers in include/kunit/device.h, which we decided would live as part of KUnit, not as part of the device infrastructure. I don't enormously mind if we make the opposite decision for these, though it does seem a bit inconsistent if we do 'devices' differently from 'platform_devices'. Thoughts? The other thing I've noted so far is that the of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup tests fail (and BUG() with a NULL pointer) on powerpc: > [15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47 > [15:18:51] Expected pdev is not null, but is > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c <...> > [15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99 > [15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4 > [15:18:51] [FAILED] of_overlay_apply_kunit_platform_device > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c > [15:18:51] note: kunit_try_catch[698] exited with irqs disabled > [15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77 > [15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4 > [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup I've not had a chance to dig into it any further, yet, but it appears to work on all of the other architectures I tried. Otherwise, I think this would be fine to take via either the clk or DT and clk trees: there are no conflicts with the current KUnit changes for 6.10. At worst, we might hit some conflicts in the documentation, but there's nothing scheduled yet. Cheers, -- David > Stephen Boyd (10): > of: Add test managed wrappers for of_overlay_apply()/of_node_put() > dt-bindings: vendor-prefixes: Add "test" vendor for KUnit and friends > dt-bindings: test: Add KUnit empty node binding > of: Add a KUnit test for overlays and test managed APIs > platform: Add test managed platform_device/driver APIs > dt-bindings: kunit: Add fixed rate clk consumer test > clk: Add test managed clk provider/consumer APIs > clk: Add KUnit tests for clk fixed rate basic type > dt-bindings: clk: Add KUnit clk_parent_data test > clk: Add KUnit tests for clks registered with struct clk_parent_data > > Documentation/dev-tools/kunit/api/clk.rst | 10 + > Documentation/dev-tools/kunit/api/index.rst | 21 + > Documentation/dev-tools/kunit/api/of.rst | 13 + > .../dev-tools/kunit/api/platformdevice.rst | 10 + > .../bindings/clock/test,clk-parent-data.yaml | 47 ++ > .../bindings/test/test,clk-fixed-rate.yaml | 35 ++ > .../devicetree/bindings/test/test,empty.yaml | 30 ++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > drivers/base/test/Makefile | 3 + > drivers/base/test/platform_kunit-test.c | 140 ++++++ > drivers/base/test/platform_kunit.c | 174 +++++++ > drivers/clk/.kunitconfig | 2 + > drivers/clk/Kconfig | 9 + > drivers/clk/Makefile | 9 +- > drivers/clk/clk-fixed-rate_test.c | 377 +++++++++++++++ > drivers/clk/clk-fixed-rate_test.h | 8 + > drivers/clk/clk_kunit.c | 198 ++++++++ > drivers/clk/clk_parent_data_test.h | 10 + > drivers/clk/clk_test.c | 451 +++++++++++++++++- > drivers/clk/kunit_clk_fixed_rate_test.dtso | 19 + > drivers/clk/kunit_clk_parent_data_test.dtso | 28 ++ > drivers/of/.kunitconfig | 1 + > drivers/of/Kconfig | 10 + > drivers/of/Makefile | 2 + > drivers/of/kunit_overlay_test.dtso | 9 + > drivers/of/of_kunit.c | 99 ++++ > drivers/of/overlay_test.c | 115 +++++ > include/kunit/clk.h | 28 ++ > include/kunit/of.h | 94 ++++ > include/kunit/platform_device.h | 15 + > 30 files changed, 1967 insertions(+), 2 deletions(-) > create mode 100644 Documentation/dev-tools/kunit/api/clk.rst > create mode 100644 Documentation/dev-tools/kunit/api/of.rst > create mode 100644 Documentation/dev-tools/kunit/api/platformdevice.rst > create mode 100644 Documentation/devicetree/bindings/clock/test,clk-parent-data.yaml > create mode 100644 Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml > create mode 100644 Documentation/devicetree/bindings/test/test,empty.yaml > create mode 100644 drivers/base/test/platform_kunit-test.c > create mode 100644 drivers/base/test/platform_kunit.c > create mode 100644 drivers/clk/clk-fixed-rate_test.c > create mode 100644 drivers/clk/clk-fixed-rate_test.h > create mode 100644 drivers/clk/clk_kunit.c > create mode 100644 drivers/clk/clk_parent_data_test.h > create mode 100644 drivers/clk/kunit_clk_fixed_rate_test.dtso > create mode 100644 drivers/clk/kunit_clk_parent_data_test.dtso > create mode 100644 drivers/of/kunit_overlay_test.dtso > create mode 100644 drivers/of/of_kunit.c > create mode 100644 drivers/of/overlay_test.c > create mode 100644 include/kunit/clk.h > create mode 100644 include/kunit/of.h > create mode 100644 include/kunit/platform_device.h > > > base-commit: 4cece764965020c22cff7665b18a012006359095 > -- > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/ > https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git >
Quoting David Gow (2024-05-01 01:08:11) > > Thanks very much. I'm about halfway through reviewing these, and I > like them a lot so far. > > Most of my thoughts are just naming ideas. I fear some of them may be > the reverse of previous suggestions, as we've since landed the KUnit > device wrappers in include/kunit/device.h, which we decided would live > as part of KUnit, not as part of the device infrastructure. I don't > enormously mind if we make the opposite decision for these, though it > does seem a bit inconsistent if we do 'devices' differently from > 'platform_devices'. Thoughts? Let's discuss on one of the patches. > > The other thing I've noted so far is that the > of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup > tests fail (and BUG() with a NULL pointer) on powerpc: > > [15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47 > > [15:18:51] Expected pdev is not null, but is > > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c This seems to be because pdev is NULL and we call put_device(&pdev->dev) on it. We could be nicer and have an 'if (pdev)' check there. I wonder if that fixes the other two below? ---8<--- diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c index 223e5a5c23c5..85cfbe6bb132 100644 --- a/drivers/of/overlay_test.c +++ b/drivers/of/overlay_test.c @@ -45,7 +45,8 @@ static void of_overlay_apply_kunit_platform_device(struct kunit *test) pdev = of_find_device_by_node(np); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev); - put_device(&pdev->dev); + if (pdev) + put_device(&pdev->dev); } static int of_overlay_bus_match_compatible(struct device *dev, const void *data) @@ -77,8 +78,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test) KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np); pdev = of_find_device_by_node(np); - put_device(&pdev->dev); /* Not derefing 'pdev' after this */ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + put_device(&pdev->dev); /* Not derefing 'pdev' after this */ /* Remove overlay */ kunit_cleanup(&fake); @@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test) dev = bus_find_device(&platform_bus_type, NULL, kunit_compatible, of_overlay_bus_match_compatible); KUNIT_EXPECT_PTR_EQ(test, NULL, dev); - put_device(dev); + if (dev) + put_device(dev); } static struct kunit_case of_overlay_apply_kunit_test_cases[] = { > > [15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99 > > [15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4 > > [15:18:51] [FAILED] of_overlay_apply_kunit_platform_device > > > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c > > [15:18:51] note: kunit_try_catch[698] exited with irqs disabled > > [15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77 > > [15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4 > > [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup > > I've not had a chance to dig into it any further, yet, but it appears > to work on all of the other architectures I tried. Cool. I don't know why powerpc doesn't make devices. Maybe it has a similar design to sparc to create resources. I'll check it out.
Quoting Stephen Boyd (2024-05-02 18:27:42) > Quoting David Gow (2024-05-01 01:08:11) > > > > The other thing I've noted so far is that the > > of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup > > tests fail (and BUG() with a NULL pointer) on powerpc: > > > [15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47 > > > [15:18:51] Expected pdev is not null, but is > > > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c > > This seems to be because pdev is NULL and we call put_device(&pdev->dev) > on it. We could be nicer and have an 'if (pdev)' check there. I wonder > if that fixes the other two below? > > ---8<--- > diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c > index 223e5a5c23c5..85cfbe6bb132 100644 > --- a/drivers/of/overlay_test.c > +++ b/drivers/of/overlay_test.c > @@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test) > dev = bus_find_device(&platform_bus_type, NULL, kunit_compatible, > of_overlay_bus_match_compatible); > KUNIT_EXPECT_PTR_EQ(test, NULL, dev); > - put_device(dev); > + if (dev) > + put_device(dev); > } This last hunk isn't needed. > > static struct kunit_case of_overlay_apply_kunit_test_cases[] = { > > > > [15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99 > > > [15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4 > > > [15:18:51] [FAILED] of_overlay_apply_kunit_platform_device > > > > > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c > > > [15:18:51] note: kunit_try_catch[698] exited with irqs disabled > > > [15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77 > > > [15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4 > > > [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup > > > > I've not had a chance to dig into it any further, yet, but it appears > > to work on all of the other architectures I tried. > > Cool. I don't know why powerpc doesn't make devices. Maybe it has a > similar design to sparc to create resources. I'll check it out. > powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that in of_platform_default_populate_init() then the overlays can be applied. ---8<---- diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 389d4ea6bfc1..fa7b439e9402 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void) of_platform_device_create(node, buf, NULL); } + node = of_find_node_by_path("/"); + if (node) + of_node_set_flag(node, OF_POPULATED_BUS); + of_node_put(node); } else { /* * Handle certain compatibles explicitly, since we don't want to create I'm guessing this is wrong though, because I see bunch of powerpc specific code calling of_platform_bus_probe() which will set the flag on the actual platform bus nodes. Maybe we should just allow overlays to create devices at the root node regardless? Of course, the flag doc says "platform bus created for children" and if we never populated the root then that isn't entirely accurate. Rob, can you point me in the right direction? Do we need to use simple-bus in the test overlays and teach overlay code to populate that bus?
On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Stephen Boyd (2024-05-02 18:27:42) > > Quoting David Gow (2024-05-01 01:08:11) > > > > > > The other thing I've noted so far is that the > > > of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup > > > tests fail (and BUG() with a NULL pointer) on powerpc: > > > > [15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47 > > > > [15:18:51] Expected pdev is not null, but is > > > > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c > > > > This seems to be because pdev is NULL and we call put_device(&pdev->dev) > > on it. We could be nicer and have an 'if (pdev)' check there. I wonder > > if that fixes the other two below? > > > > ---8<--- > > diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c > > index 223e5a5c23c5..85cfbe6bb132 100644 > > --- a/drivers/of/overlay_test.c > > +++ b/drivers/of/overlay_test.c > > @@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test) > > dev = bus_find_device(&platform_bus_type, NULL, kunit_compatible, > > of_overlay_bus_match_compatible); > > KUNIT_EXPECT_PTR_EQ(test, NULL, dev); > > - put_device(dev); > > + if (dev) > > + put_device(dev); > > } > > This last hunk isn't needed. > > > > > static struct kunit_case of_overlay_apply_kunit_test_cases[] = { > > > > > > [15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99 > > > > [15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4 > > > > [15:18:51] [FAILED] of_overlay_apply_kunit_platform_device > > > > > > > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c > > > > [15:18:51] note: kunit_try_catch[698] exited with irqs disabled > > > > [15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77 > > > > [15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4 > > > > [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup > > > > > > I've not had a chance to dig into it any further, yet, but it appears > > > to work on all of the other architectures I tried. > > > > Cool. I don't know why powerpc doesn't make devices. Maybe it has a > > similar design to sparc to create resources. I'll check it out. > > > > powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that > in of_platform_default_populate_init() then the overlays can be applied. > > ---8<---- > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 389d4ea6bfc1..fa7b439e9402 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void) > of_platform_device_create(node, buf, NULL); > } > > + node = of_find_node_by_path("/"); > + if (node) > + of_node_set_flag(node, OF_POPULATED_BUS); I think you want to do this in of_platform_bus_probe() instead to mirror of_platform_populate(). These are supposed to be the same except that 'populate' only creates devices for nodes with compatible while 'probe' will create devices for all child nodes. Looks like we are missing some devlink stuff too. There may have been some issue for PPC with it. > + of_node_put(node); > } else { > /* > * Handle certain compatibles explicitly, since we don't want to create > > I'm guessing this is wrong though, because I see bunch of powerpc specific code > calling of_platform_bus_probe() which will set the flag on the actual platform > bus nodes. Maybe we should just allow overlays to create devices at the root > node regardless? Of course, the flag doc says "platform bus created for > children" and if we never populated the root then that isn't entirely accurate. > > Rob, can you point me in the right direction? Do we need to use simple-bus in > the test overlays and teach overlay code to populate that bus? Overlays adding things to the root node might be suspect, but probably there are some valid reasons to do so. If simple-bus makes sense here, then yes, you should use it. But if what's on it is not MMIO devices, don't. That's a warning in the schema now. Rob
Quoting Rob Herring (2024-05-15 06:06:09) > On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that > > in of_platform_default_populate_init() then the overlays can be applied. > > > > ---8<---- > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > index 389d4ea6bfc1..fa7b439e9402 100644 > > --- a/drivers/of/platform.c > > +++ b/drivers/of/platform.c > > @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void) > > of_platform_device_create(node, buf, NULL); > > } > > > > + node = of_find_node_by_path("/"); > > + if (node) > > + of_node_set_flag(node, OF_POPULATED_BUS); > > I think you want to do this in of_platform_bus_probe() instead to > mirror of_platform_populate(). These are supposed to be the same > except that 'populate' only creates devices for nodes with compatible > while 'probe' will create devices for all child nodes. Looks like we > are missing some devlink stuff too. There may have been some issue for > PPC with it. Got it. So this patch? ---8<--- diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 389d4ea6bfc1..acecefcfdba7 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root, if (of_match_node(matches, root)) { rc = of_platform_bus_create(root, matches, NULL, parent, false); } else for_each_child_of_node(root, child) { + of_node_set_flag(root, OF_POPULATED_BUS); if (!of_match_node(matches, child)) continue; rc = of_platform_bus_create(child, matches, NULL, parent, false); This doesn't work though. I see that prom_init() is called, which constructs a DTB and flattens it to be unflattened by unflatten_device_tree(). The powerpc machine type used by qemu is PLATFORM_PSERIES_LPAR. It looks like it never calls of_platform_bus_probe() from the pseries platform code. What about skipping the OF_POPULATED_BUS check, or skipping the check when the parent is the root node? This is the if condition that's giving the headache. ---8<--- diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 389d4ea6bfc1..38dfafc25d86 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -735,10 +735,6 @@ static int of_platform_notify(struct notifier_block *nb, 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)) - return NOTIFY_OK; /* not for us */ - /* already populated? (driver using of_populate manually) */ if (of_node_check_flag(rd->dn, OF_POPULATED)) return NOTIFY_OK; > > > + of_node_put(node); > > } else { > > /* > > * Handle certain compatibles explicitly, since we don't want to create > > > > I'm guessing this is wrong though, because I see bunch of powerpc specific code > > calling of_platform_bus_probe() which will set the flag on the actual platform > > bus nodes. Maybe we should just allow overlays to create devices at the root > > node regardless? Of course, the flag doc says "platform bus created for > > children" and if we never populated the root then that isn't entirely accurate. > > > > Rob, can you point me in the right direction? Do we need to use simple-bus in > > the test overlays and teach overlay code to populate that bus? > > Overlays adding things to the root node might be suspect, but probably > there are some valid reasons to do so. In this case we're using it to add nodes without a reg property to the root node. > If simple-bus makes sense here, > then yes, you should use it. But if what's on it is not MMIO devices, > don't. That's a warning in the schema now. > Ok. Sounds like adding these nodes to the root node is the right way then. I wonder if we can make MMIO devices appear on the kunit bus by adding DT support to the bus and then letting those nodes have reg properties that we "sinkhole" by making those iomem resources point to something else in the ioremap code. Then we can work in MMIO kunit emulation that way to let tests check code that works with readl/writel, e.g. the clk gate tests.
On Wed, May 15, 2024 at 4:15 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Rob Herring (2024-05-15 06:06:09) > > On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that > > > in of_platform_default_populate_init() then the overlays can be applied. > > > > > > ---8<---- > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > > index 389d4ea6bfc1..fa7b439e9402 100644 > > > --- a/drivers/of/platform.c > > > +++ b/drivers/of/platform.c > > > @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void) > > > of_platform_device_create(node, buf, NULL); > > > } > > > > > > + node = of_find_node_by_path("/"); > > > + if (node) > > > + of_node_set_flag(node, OF_POPULATED_BUS); > > > > I think you want to do this in of_platform_bus_probe() instead to > > mirror of_platform_populate(). These are supposed to be the same > > except that 'populate' only creates devices for nodes with compatible > > while 'probe' will create devices for all child nodes. Looks like we > > are missing some devlink stuff too. There may have been some issue for > > PPC with it. > > Got it. So this patch? > > ---8<--- > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 389d4ea6bfc1..acecefcfdba7 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root, > if (of_match_node(matches, root)) { > rc = of_platform_bus_create(root, matches, NULL, parent, false); > } else for_each_child_of_node(root, child) { > + of_node_set_flag(root, OF_POPULATED_BUS); No, the same spot as of_platform_populate has it. I guess this would be the same, but no reason to do this in the for_each_child_of_node loop... > if (!of_match_node(matches, child)) > continue; > rc = of_platform_bus_create(child, matches, NULL, parent, false); > > > This doesn't work though. I see that prom_init() is called, which > constructs a DTB and flattens it to be unflattened by > unflatten_device_tree(). The powerpc machine type used by qemu is > PLATFORM_PSERIES_LPAR. It looks like it never calls > of_platform_bus_probe() from the pseries platform code. Huh. Maybe pseries doesn't have any platform devices? Ideally, we'd still do it in of_platform_default_populate_init(), but if you look at the history, you'll see that broke some PPC boards (damn initcall ordering). > What about skipping the OF_POPULATED_BUS check, or skipping the check > when the parent is the root node? This is the if condition that's > giving the headache. I don't think we should just remove it, but a root node check seems fine. Rob
Quoting Rob Herring (2024-05-15 15:08:47) > On Wed, May 15, 2024 at 4:15 PM Stephen Boyd <sboyd@kernel.org> wrote: > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > index 389d4ea6bfc1..acecefcfdba7 100644 > > --- a/drivers/of/platform.c > > +++ b/drivers/of/platform.c > > @@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root, > > if (of_match_node(matches, root)) { > > rc = of_platform_bus_create(root, matches, NULL, parent, false); > > } else for_each_child_of_node(root, child) { > > + of_node_set_flag(root, OF_POPULATED_BUS); > > No, the same spot as of_platform_populate has it. I guess this would > be the same, but no reason to do this in the for_each_child_of_node > loop... Ok. I'm not intending to send this patch. > > > if (!of_match_node(matches, child)) > > continue; > > rc = of_platform_bus_create(child, matches, NULL, parent, false); > > > > > > This doesn't work though. I see that prom_init() is called, which > > constructs a DTB and flattens it to be unflattened by > > unflatten_device_tree(). The powerpc machine type used by qemu is > > PLATFORM_PSERIES_LPAR. It looks like it never calls > > of_platform_bus_probe() from the pseries platform code. > > Huh. Maybe pseries doesn't have any platform devices? Looks like it. > > Ideally, we'd still do it in of_platform_default_populate_init(), but > if you look at the history, you'll see that broke some PPC boards > (damn initcall ordering). > > > What about skipping the OF_POPULATED_BUS check, or skipping the check > > when the parent is the root node? This is the if condition that's > > giving the headache. > > I don't think we should just remove it, but a root node check seems fine. > Alright. I've added a check to see if the root node is the parent to allow it. That works well enough, so I'll send that in v5.