Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Mon, 30 Jan 2023 09:13:20 +0100 | Subject | Re: [PATCH] driver: bus: simple-pm-bus: Add Freescale i.MX8qm/qxp CSR compatible strings |
| |
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%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. > > 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"? (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
| |