Re: [PATCH 14/19] powerpc: mpc83xx: Switch to of_platform_populate

["J. Neuschäfer" <j.ne@posteo.net>]

On Thu, Jan 02, 2025 at 12:51:47PM -0600, Rob Herring wrote:
> On Thu, Jan 2, 2025 at 12:32 PM J. Neuschäfer via B4 Relay
> <devnull+j.ne.posteo.net@kernel.org> wrote:
> >
> > From: "J. Neuschäfer" <j.ne@posteo.net>
> >
> > Quoting from drivers/of/platform.c:
> >
> > > of_platform_populate() - [...]
> > > Similar to of_platform_bus_probe(), this function walks the device
> > > tree and creates devices from nodes.  It differs in that it follows
> > > the modern convention of requiring all device nodes to have a
> > > 'compatible' property, and it is suitable for creating devices which
> > > are children of the root node (of_platform_bus_probe will only create
> > > children of the root which are selected by the @matches argument).
> >
> > This is useful for new board ports because it means that the C code does
> > not have to anticipate every node that is placed directly under the root.
> >
> > As a consequence, the of_bus_ids list can be much shorter, and I've
> > trimmed it to the necessary parts:
> >
> >  - device-type = "soc" and compatible = "simple-bus" for the SoC bus
> >  - compatible = "gianfar" for the Ethernet controller (TSEC), which
> >    may contain an MDIO bus, which needs to be probed, as a subnode
> >
> > Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> > ---
> >  arch/powerpc/platforms/83xx/misc.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/83xx/misc.c b/arch/powerpc/platforms/83xx/misc.c
> > index 1135c1ab923cc120f377a0d98767fef686cad1fe..bf522ee007bbb1429233355f668fc8563d8ca4e2 100644
> > --- a/arch/powerpc/platforms/83xx/misc.c
> > +++ b/arch/powerpc/platforms/83xx/misc.c
> > @@ -94,18 +94,14 @@ void __init mpc83xx_ipic_init_IRQ(void)
> >
> >  static const struct of_device_id of_bus_ids[] __initconst = {
> >         { .type = "soc", },
> 
> of_platform_populate() won't work on this match unless there's a
> compatible in the node, too. Can we use compatible instead or are
> there a bunch of them?

In arch/powerpc/boot/dts, I can find the following cases of device_type
= "soc" without compatible = "simple-bus":

- arch/powerpc/boot/dts/tqm8xx.dts           (MPC8xx)
- arch/powerpc/boot/dts/mpc885ads.dts        (MPC8xx)
- arch/powerpc/boot/dts/mpc866ads.dts        (MPC8xx)
- arch/powerpc/boot/dts/ep88xc.dts           (MPC8xx)
- arch/powerpc/boot/dts/kuroboxHG.dts        (MPC82xx)
- arch/powerpc/boot/dts/kuroboxHD.dts        (MPC82xx)
- arch/powerpc/boot/dts/storcenter.dts       (MPC82xx)
- arch/powerpc/boot/dts/asp834x-redboot.dts  (MPC83xx!)
- arch/powerpc/boot/dts/ksi8560.dts          (MPC85xx)

i.e. there is one affected devicetree. I can simply patch that one in
the next iteration.

> 
> > -       { .compatible = "soc", },
> >         { .compatible = "simple-bus" },
> >         { .compatible = "gianfar" },
> > -       { .compatible = "gpio-leds", },
> > -       { .type = "qe", },
> > -       { .compatible = "fsl,qe", },
> 
> Better still would be if we could move the remaining ones to the
> default table and just call of_platform_default_populate().

of_platform_default_populate does sound preferable.

I'll investigate why exactly the "gianfar" match is necessary and how to
fix it in the corresponding driver (I don't think it's general enough to
warrant being listed in of_default_bus_match_table).


Best regards,
 jn