On Thu, 2022-08-04 at 11:26 -0700, Saravana Kannan wrote:
> On Thu, Aug 4, 2022 at 5:18 AM Rob Herring <robh+dt@xxxxxxxxxx>
> wrote:
> >
> > On Thu, Aug 4, 2022 at 12:10 AM Liu Ying <victor.liu@xxxxxxx>
> > wrote:
> > >
> > > There could be simple MFD device(s) connected to a simple PM bus
> > > as child
> > > node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a child
> > > match
> > > table as an argument to of_platform_populate() function call to
> > > specify
> > > the simple MFD devices so that they can be populated.
> >
> > There could be a simple-bus under it as well. You should just use
> > of_platform_default_populate() instead.
>
> I'm confused why we even need this patch. Wouldn't this driver
> automatically probe simple-mfd buses and populate its child devices?
> We already have it in simple_pm_bus_of_match.
First of all, this driver doesn't populate simple-mfd bus's child
devices because "ONLY_BUS" is set in simple_pm_bus_of_match[] for
simple-mfd.
The device tree I'm working with is something like this:
bus@560000000 {
compatible = "fsl,aips-bus", "simple-bus";
...
bus@562000000 {
compatible = "fsl,imx8qm-display-pixel-link-msi-bus", "simple-
pm-bus";
...
syscon@56241000 {
compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-mfd";
...
syscon_child {};
};
/* more regular mmap devices */
};
};
IIUC, default buses listed in of_default_bus_match_table[], including
simple-bus and simple-mfd, are populated by
of_platform_default_populate() in a recursive fashion, when
of_platform_default_populate_init() is called. However, simple-pm-bus
is not listed in that table. So, bus@562000000 (simple-pm-bus) is the
last one to be populated successfully and syscon@56241000 (simple-mfd)
is not populated (recursion stops).
Then, this patch adds a match table to populate syscon@56241000 (simple
-mfd) _and_ it's child nodes when bus@562000000 (simple-pm-bus) is
probed. of_platform_populate() will populate syscon@56241000 (simple-
mfd) and it's child devices (sycon_child) together. Hence, sycon_child
devices will be probed ok.
The problem is that syscon@56241000 (simple-mfd) fails to be probed
with return code -ENODEV as "ONLY_BUS" is set and "simple-mfd" is the
3rd compatible string. Even if it's probed ok, syscon@56241000 (simple-
mfd) is not power managed, which means syscon_child devices' PM
operations won't be propagated to bus@562000000 (simple-pm-bus) (?).
Anyway, somehow, syscon_child devices do work, based on my test.
With regard to PM, simple-bus is the same if it sits at simple-mfd's
place. So, maybe, simple-mfd and simple-bus should be power managed as
well? Or, simple-pm-bus should have no simple-mfd and simple-bus child
nodes at all?
>
> I'm wondering if you are trying to workaround the behavior of having
> "ONLY_BUS" set in simple_pm_bus_of_match for "simple-mfd". Have you
> tried deleting that field and see if it does what you want?
Without this patch, deleting "ONLY_BUS" works for me, as syscon_child
devices are populated when syscon@56241000 (simple-mfd) is probed.
Deleting "ONLY_BUS" may make simple-mfd a power managed device. Is it a
right thing to do?
Regards,
Liu Ying
>
> And we wouldn't need to use of_platform_default_populate() because
> this driver would take care of doing that recursively. Especially
> when
> you need the clocks and power domain to be able to access the child
> devices, you want the driver to probe and do that at each level
> before
> automatically recursively adding all the grand-children devices.
>
> -Saravana
>
> >
> > >
> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > ---
> > > v1->v3:
> > > * No change.
> > >
> > > drivers/bus/simple-pm-bus.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-
> > > bus.c
> > > index 6b8d6257ed8a..ff5f8ca5c024 100644
> > > --- a/drivers/bus/simple-pm-bus.c
> > > +++ b/drivers/bus/simple-pm-bus.c
> > > @@ -13,6 +13,11 @@
> > > #include <linux/platform_device.h>
> > > #include <linux/pm_runtime.h>
> > >
> > > +static const struct of_device_id simple_pm_bus_child_matches[] =
> > > {
> > > + { .compatible = "simple-mfd", },
> > > + {}
> > > +};
> > > +
> > > static int simple_pm_bus_probe(struct platform_device *pdev)
> > > {
> > > const struct device *dev = &pdev->dev;
> > > @@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct
> > > platform_device *pdev)
> > > pm_runtime_enable(&pdev->dev);
> > >
> > > if (np)
> > > - of_platform_populate(np, NULL, lookup, &pdev-
> > > >dev);
> > > + of_platform_populate(np,
> > > simple_pm_bus_child_matches, lookup, &pdev->dev);
> > >
> > > return 0;
> > > }
> > > --
> > > 2.25.1
> > >
