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

[Ying Liu <victor.liu@nxp.com>]

Hi Geert, Krysztof, all,

On Monday, January 30, 2023 9:46 AM Ying Liu wrote:
> 
> On Sun, 2023-01-29 at 11:49 +0100, Krzysztof Kozlowski wrote:
> > On 29/01/2023 09:13, Liu Ying wrote:
> > > On Thu, 2023-01-26 at 13:45 +0100, Krzysztof Kozlowski wrote:
> > > > On 26/01/2023 03:54, Liu Ying wrote:
> > > > > On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
> > > > > > Hi Liu,
> > > > >
> > > > > Hi Geert,
> > > > >
> > > > > >
> > > > > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <victor.liu@nxp.com>
> > > > > > 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@kernel.org>
> > > > > > > Suggested-by: Lee Jones <lee@kernel.org>
> > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > >
> > > > > > 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%2Fpatch
> work.kernel.org%2Fproject%2Flinux-arm-
> kernel%2Fpatch%2F20221017075702.4182846-1-
> victor.liu%40nxp.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7C58af
> 8a86f0134b6bde3408db01e68522%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C638105861813147063%7CUnknown%7CTWFpbGZsb3d8eyJWI
> joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%7C%7C%7C&sdata=mHv%2BTAHMAR8coxDmXucoMbxv%2BuMEdHWH
> TyLz16OUY50%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.
> > > >
> > > > I don't think we want to start putting specific compatibles here.
> > > > We
> > > > don't do it for simple-mfd, syscon and simple-bus, so neither
> > > > should
> > > > we
> > > > do it here.
> > > >
> > > > >
> > > > > 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.
> > > >
> > > > The fsl,imx8qxp-csr.yaml bindings are broken anyway... You have
> > > > device
> > > > specific bindings for non-simple device but use simple-mfd. You
> > > > cannot.
> > > > simple-mfd means it is simple and none of the resources are
> > > > needed
> > > > for
> > > > children, but that binding contradicts it.
> > > >
> > > > Now you kind of try to extend it even more make it more and more
> > > > broken.
> > > >
> > > > Rework the bindings keeping them backwards compatible. The
> > > > combination
> > > > with simple-mfd should be deprecated and you can add whatever is
> > > > needed
> > > > for a proper setup.
> > >
> > > I did try to rework the bindings and make the combination with
> > > simple-
> > > mfd deprecated. However, it reminds me the problem that "simple-pm-
> > > bus"
> > > and "syscon" can not be in compatible string at the same time,
> > > otherwise, nodename should match '^syscon@[0-9a-f]+$' and '^bus@[0-
> > > 9a-
> > > f]+$' at the same time. I mentioned the problem in the same
> > > thread[1]
> > > where Rob and Lee suggest to go with this patch. "syscon" is needed
> > > since i.MX8qxp MIPI DSI/LVDS combo PHY node references the CSR
> > > module
> > > through a phandle, so dropping/deprecating "syscon" is a no-go.
> > >
> > > Also, as Rob mentioned in [1] "if register space is all mixed
> > > together,
> > > then it is the former and an MFD", I think the CSR module should
> > > fall
> > > into the simple-mfd category.
> >
> > You are now mixing MFD with simple-mfd. If you have clocks there or
> > any
> > other resources, it's not simple-mfd anymore.
> 
> I may try to make the combination with simple-mfd deprecated and add
> another combination with i.MX8qm/qxp CSR compatible strings and syscon
> only. Then, it will be a MFD, not simple-mfd.
> 
> >
> > > Take i.MX8qxp MIPI DSI/LVDS CSR module as
> > > an example, child device pxl2dpi register offset is 0x40, while
> > > child
> > > device ldb register offsets are 0x20 and 0xe0.
> > >
> > > Geert, Krzysztof, can you please consider to keep this patch as-is,
> > > since it seems that there is no other option?
> >
> > There are other options, why do you say there is no? Making it proper
> > binding/driver for its children without abusing simple bindings.
> 
> I don't quite understand your comment here, sorry. Here are the 3
> options I know:
> 
> 1) Add a new MFD driver for the CSR module
> I sent out a MFD driver[1] for the CSR module for review, but Rob and
> Lee provided comments there and suggested to use this patch.
> 
> 2) Use "simple-pm-bus" compatible string in the CSR module's compatbile
> property
> As mentioned before, "simple-pm-bus" contradicts with "syscon".
> 
> 3) Add the CSR module's specific compatible strings in
> simple_pm_bus_of_match[]
> This is what this patch does and suggested by Rob and Lee.
> 
> Looks like 3) is the only feasible option.
> 
> Geert, Krzysztof, any objections to keep this patch as-is?

Sorry for bring this up again, but option 3) is still the only feasible
option suggested by Rob and Lee, just as this patch does.

Can you consider to have it landed?

Regards,
Liu Ying

> 
> [1]
> https://patchwork.kernel.org/project/linux-arm-
> kernel/patch/20221017075702.4182846-1-victor.liu@nxp.com/
> 
> 
> Regards,
> Liu Ying
> 
> 
> > Simple
> > bindings are for simple cases and this turns out not the simple case.
> >
> > Best regards,
> > Krzysztof
> >