Messages in this thread | | | Subject | Re: [PATCH] driver: bus: simple-pm-bus: Add Freescale i.MX8qm/qxp CSR compatible strings | From | Liu Ying <> | Date | Tue, 31 Jan 2023 13:26:33 +0800 |
| |
On Mon, 2023-01-30 at 09:13 +0100, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Sun, Jan 29, 2023 at 11:49 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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: > > > > > > 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> > > > > > > > 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%7C721a36c64fab482d5d3908db0299e1d8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638106632175447990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jnxUn2N33xjrmNfGXw02SeG5EN%2Fqluz%2BjZYRk0%2BHlrU%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 beg to differ (but not w.r.t. the other resources): if any "glue" > device > between parent and child hierarchies does not call > pm_runtime_enable(), > Runtime PM (which is a Linux thing, not a DT thing) for the children > may not work correctly, as it won't propagate correctly to the > parent. > So IMHO this is something to fix in Linux, not in DT. > > > > 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. > > Simple > > bindings are for simple cases and this turns out not the simple > > case. > > Or drop the ".data = ONLY_BUS" for "simple-mfd"?
Saravana said it's wrong to deleting ONLY_BUS for "simple-mfd". See Saravana's comments in [1].
[1] https://lore.kernel.org/linux-arm-kernel/CAGETcx_QVaYYHsD9HZmBu404K-oXRCPm4N4GRrYu4pGyw2DHbg@mail.gmail.com/
Regards, Liu Ying
> (and treat "simple-bus" the same as "simple-pm-bus", too ;-) > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > 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
| |