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

[Krzysztof Kozlowski]

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%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.

> 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.

Best regards,
Krzysztof