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

[Liu Ying]

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@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%7C87515adc8fc3401f410808daff9b3279%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103339276325657%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FFz5gSIPc6vyvb1IJ1Umu62WpzjNLIiQIi2sOA3RQGc%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. 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?

[1] 
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20221017075702.4182846-1-victor.liu@xxxxxxx/

Regards,
Liu Ying

> 
> > 
> > > 
> > > 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
> > "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...
> 
> Because the device is not simple-mfd and should have never been made
> that. I am surprised it passed Rob's review, I guess it slipped
> through
> the cracks.
> 
> Now you have to live with borken bindings. You have a lesson for
> future
> - put some effort to design them correctly from the beginning, so you
> won't have problems. Bindings should be complete from the beginning,
> not
> "I'll develop whatever is needed to match my driver and I will not
> care
> about future".
> 
> Best regards,
> Krzysztof
>