Re: [PATCH V3 1/3] dt-bindings: clk: sprd: Add bindings for ums512 clock controller

[Krzysztof Kozlowski]

On 24/04/2022 12:47, Cixi Geng wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 于2022年4月19日周二 14:38写道：
>>
>> On 19/04/2022 03:53, Cixi Geng wrote:
>>> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 于2022年4月19日周二 00:28写道：
>>>>
>>>> On 18/04/2022 14:56, Cixi Geng wrote:
>>>>> From: Cixi Geng <cixi.geng1@xxxxxxxxxx>
>>>>>
>>>>> Add a new bindings to describe ums512 clock compatible string.
>>>>>
>>>>> Signed-off-by: Cixi Geng <cixi.geng1@xxxxxxxxxx>
>>>>> ---
>>>>>  .../bindings/clock/sprd,ums512-clk.yaml       | 112 ++++++++++++++++++
>>>>>  1 file changed, 112 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..89824d7c6be4
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>>>>> @@ -0,0 +1,112 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +# Copyright 2022 Unisoc Inc.
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: "http://devicetree.org/schemas/clock/sprd,ums512-clk.yaml#";
>>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
>>>>> +
>>>>> +title: UMS512 Clock Control Unit Device Tree Bindings
>>>>
>>>> Remove "Device Tree Bindings". You could do the same also in the
>>>> subject, because you repeat the prefix ("dt-bindings: clk: sprd: Add
>>>> ums512 clock controller").
>>>>
>>>>> +
>>>>> +maintainers:
>>>>> +  - Orson Zhai <orsonzhai@xxxxxxxxx>
>>>>> +  - Baolin Wang <baolin.wang7@xxxxxxxxx>
>>>>> +  - Chunyan Zhang <zhang.lyra@xxxxxxxxx>
>>>>> +
>>>>> +properties:
>>>>> +  "#clock-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  compatible:
>>>>
>>>> Put the compatible first, by convention and makes finding matching
>>>> bindings easier.
>>>>
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - const: sprd,ums512-glbregs
>>>>> +          - const: syscon
>>>>> +          - const: simple-mfd
>>>>
>>>> Why do you need simple-mfd for these? This looks like a regular syscon,
>>>> so usually does not come with children. What is more, why this "usual
>>>> syscon" is a separate clock controller in these bindings?
>>> there is a warning log before add these const.  and the reason we need
>>> the simply-mfd
>>> is some clock is a child of syscon node,which should set these compatible.
>>> failed to match any schema with compatible: ['sprd,ums512-glbregs',
>>> 'syscon', 'simple-mfd']
>>
>> Neither here nor later you did not answer the question - why do you need
>> such complex construction, instead of adding syscon to the clock controller?
>>
>> Let me paste again my concerns:
>>
>>   You have nodes with reg but without unit address ("rpll"). These nodes
>>   are modeled as children but they are not children - it's a workaround
>>   for exposing syscon, isn't it? The sc9863a looks like broken design,
>>   so please do not duplicate it here.
>>
>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
>> Because of this you need to create complex ways to get the regmap for
>> the clock controller... Why not making it simple? Clock controller with
>> syscon?
> 
> I find the history discuss about the sp9863 clock[1] and last
> ums512-clk dt-bindings patch[2] which from chunyan.
> please refer to the reasons below.
> 
> These clocks are at the same register range with global registers.
> the registers shared with more than one devices  which  basically
> are multimedia devices. You may noticed that these are all gate
> clocks which are in the global registers  ranges and are used to
> controll the enable status of some devices or some part of devices.
> 
> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@xxxxxxxxxxxxxx/#r
> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@xxxxxxxxxxxxxxxxxxxxxxxxxx/#r

Which looks like discussion about different bindings. You had there a
clock controller and additional clock device using "sprd,syscon". Why
the rpll is a subdevice and not a part of clock controller. The same as
all other clocks coming from that clock-controller, right? What is so
special about rpll that is is a separate device, not part of the clock
controller? It's the same address space, isn't it?

Best regards,
Krzysztof