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

[Krzysztof Kozlowski]

On 30/01/2023 02:45, Liu Ying 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@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%7C58af8a86f0134b6bde3408db01e68522%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638105861813147063%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mHv%2BTAHMAR8coxDmXucoMbxv%2BuMEdHWHTyLz16OUY50%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.

Where are the clocks in that driver? Having MFD for something without
resources does not make any sense - this is why we have simple-mfd.

But I see in your [1] Rob's suggestion about adding the compatible to
simple-pm-bus.c, therefore it looks correct approach.

Best regards,
Krzysztof