Re: [PATCH] driver: bus: simple-pm-bus: Add Freescale i.MX8qm/qxp CSR compatible strings

[Liu Ying]

On Thu, 2023-01-26 at 09:30 +0100, Geert Uytterhoeven wrote:
> Hi Liu,

Hi Geert,

> 
> On Thu, Jan 26, 2023 at 3:55 AM Liu Ying <victor.liu@xxxxxxx> wrote:
> > On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
> > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <victor.liu@xxxxxxx>
> > > wrote:
> > > > Freescale i.MX8qm/qxp CSR module matches with what the simple
> > > > power
> > > > managed bus driver does, considering it needs an IPG clock to
> > > > be
> > > > enabled before accessing it's child devices, the child devices
> > > > need
> > > > to be populated by the CSR module and the child devices' power
> > > > management operations need to be propagated to their parent
> > > > devices.
> > > > Add the CSR module's compatible strings to
> > > > simple_pm_bus_of_match[]
> > > > table to support the CSR module.
> > > > 
> > > > Suggested-by: Rob Herring <robh@xxxxxxxxxx>
> > > > Suggested-by: Lee Jones <lee@xxxxxxxxxx>
> > > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > 
> > > Thanks for your patch!
> > 
> > Thanks for your review!
> > 
> > > 
> > > > ---
> > > > The CSR module's dt-binding documentation can be found at
> > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > 
> > > > Suggested by Rob and Lee in this thread:
> > > > 
> > 
> > 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F20221017075702.4182846-1-victor.liu%40nxp.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7C5121c05b779f4003da7b08daff77a9ac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103186659610847%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=yyYJNJcWwDjZESJ%2FiAqkEhKbz2jRdDbgmQ%2Bm%2FWwdWZs%3D&reserved=0
> > > > 
> > > >  drivers/bus/simple-pm-bus.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-
> > > > pm-
> > > > bus.c
> > > > index 7afe1947e1c0..4a7575afe6c6 100644
> > > > --- a/drivers/bus/simple-pm-bus.c
> > > > +++ b/drivers/bus/simple-pm-bus.c
> > > > @@ -120,6 +120,8 @@ static const struct of_device_id
> > > > simple_pm_bus_of_match[] = {
> > > >         { .compatible = "simple-mfd",   .data = ONLY_BUS },
> > > >         { .compatible = "isa",          .data = ONLY_BUS },
> > > >         { .compatible = "arm,amba-bus", .data = ONLY_BUS },
> > > > +       { .compatible = "fsl,imx8qm-lvds-csr", },
> > > > +       { .compatible = "fsl,imx8qxp-mipi-lvds-csr", },
> > > 
> > > I did read the thread linked above, and I still think you should
> > > just
> > > add "simple-pm-bus" to the compatible value in DTS, so no driver
> > > change
> > > is needed, cfr.
> > > Documentation/devicetree/bindings/bus/renesas,bsc.yaml.
> > 
> > This means that i.MX8qm/qxp CSR module dt-binding documentation
> > needs
> > to be changed.  I'd like to know how Rob and Krzysztof think about
> > that.
> > 
> > > 
> > > If that doesn't work due to DT binding constraints, the latter
> > > should
> > > be fixed.
> > 
> > Adding "simple-pm-bus" to the compatible value in DTS doesn't work,
> > because "simple-mfd" is matched first and "ONLY_BUS" is set for
> 
> Is that because you have both "simple-mfd" and "simple-pm-bus",
> and the former is listed first in DTS?

Yes. For example, compatible is set as below for CSR module in i.MX8qm
LVDS subsystem.

compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-mfd", "simple-pm-
bus";

> Does it work if you change the order?

Yes, it works if I swap the positions of "simple-mfd" and "simple-pm-
bus".

Device tree specification says the compatible strings should be listed
from most specific to most general.  "simple-mfd" and "simple-pm-bus"
sound like two different things, so I don't have good idea about the
order.  Replacing "simple-mfd" with "simple-pm-bus" and moving
fsl,imx8qxp-csr.yaml to bus directory look more reasonable to me.  But,
I'd like to know how do device tree folks think.

Regards,
Liu Ying

> 
> > "simple-mfd".  s/simple-mfd/simple-pm-bus/ for the compatbile value
> > in
> > DTS does work, but it means that fsl,imx8qxp-csr.yaml needs to be
> > changed and moved from mfd directory to bus directory...
> 
> BTW, originally I didn't want to introduce "simple-pm-bus" at all,
> and make it just call pm_runtime_enable() for "simple-bus" (PM is
> everywhere anyway), but that was rejected...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a
> hacker. But
> when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds