[1/3] dt-bindings: clk: sprd: Add UMS9620 support

Message ID	20231229085156.1490233-2-chunyan.zhang@unisoc.com
State	Superseded
Headers	show Return-Path: <devicetree+bounces-28880-incoming-dt=patchwork.ozlabs.org@vger.kernel.org> From: Chunyan Zhang <chunyan.zhang@unisoc.com> To: Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org> CC: <linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Orson Zhai <orsonzhai@gmail.com>, Baolin Wang <baolin.wang@linux.alibaba.com>, Chunyan Zhang <chunyan.zhang@unisoc.com>, Chunyan Zhang <zhang.lyra@gmail.com> Subject: [PATCH 1/3] dt-bindings: clk: sprd: Add UMS9620 support Date: Fri, 29 Dec 2023 16:51:54 +0800 Message-ID: <20231229085156.1490233-2-chunyan.zhang@unisoc.com> In-Reply-To: <20231229085156.1490233-1-chunyan.zhang@unisoc.com> References: <20231229085156.1490233-1-chunyan.zhang@unisoc.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain
Series	Add UMS9620 clocks driver and bindings \| expand [0/3] Add UMS9620 clocks driver and bindings [1/3] dt-bindings: clk: sprd: Add UMS9620 support [2/3] clk: sprd: Add dt-bindings include file for UMS9620

Context	Check	Description
robh/checkpatch	success
robh/patch-applied	success
robh/dtbs-check	warning	build log
robh/dt-meta-schema	success

Chunyan Zhang Dec. 29, 2023, 8:51 a.m. UTC

Add UMS9620's clock compatible strings and descriptions for clock
and clock-names for UMS9620.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 .../bindings/clock/sprd,ums512-clk.yaml       | 79 ++++++++++++++++---
 1 file changed, 68 insertions(+), 11 deletions(-)

Krzysztof Kozlowski Jan. 4, 2024, 8:46 a.m. UTC | #1

On 29/12/2023 09:51, Chunyan Zhang wrote:
> Add UMS9620's clock compatible strings and descriptions for clock
> and clock-names for UMS9620.
> 
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
>  .../bindings/clock/sprd,ums512-clk.yaml       | 79 ++++++++++++++++---
>  1 file changed, 68 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> index 43d2b6c31357..dcad41a1fea7 100644
> --- a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> @@ -30,25 +30,38 @@ properties:
>        - sprd,ums512-mm-clk
>        - sprd,ums512-mm-gate-clk
>        - sprd,ums512-apapb-gate
> +      - sprd,ums9620-pmu-gate
> +      - sprd,ums9620-g1-pll
> +      - sprd,ums9620-g5l-pll
> +      - sprd,ums9620-g5r-pll
> +      - sprd,ums9620-g8-pll
> +      - sprd,ums9620-g9-pll
> +      - sprd,ums9620-g10-pll
> +      - sprd,ums9620-apapb-gate
> +      - sprd,ums9620-ap-clk
> +      - sprd,ums9620-apahb-gate
> +      - sprd,ums9620-aon-gate
> +      - sprd,ums9620-aonapb-clk
> +      - sprd,ums9620-topdvfs-clk
> +      - sprd,ums9620-ipaapb-gate
> +      - sprd,ums9620-ipa-clk
> +      - sprd,ums9620-ipaglb-gate
> +      - sprd,ums9620-ipadispc-gate
> +      - sprd,ums9620-pcieapb-gate
> +      - sprd,ums9620-pcie-clk
> +      - sprd,ums9620-mm-gate
> +      - sprd,ums9620-mm-clk
> +      - sprd,ums9620-dpu-vsp-gate
> +      - sprd,ums9620-dpu-vsp-clk
>  
>    "#clock-cells":
>      const: 1
>  
>    clocks:
> -    minItems: 1
> -    maxItems: 4

No, constraints stay here.

>      description: |
>        The input parent clock(s) phandle for the clock, only list
>        fixed clocks which are declared in devicetree.
>  
> -  clock-names:

No, you cannot just drop properties. Widest constraints stay here.

> -    minItems: 1
> -    items:
> -      - const: ext-26m
> -      - const: ext-32k
> -      - const: ext-4m
> -      - const: rco-100m
> -
>    reg:
>      maxItems: 1
>  
> @@ -57,7 +70,43 @@ required:
>    - '#clock-cells'
>    - reg
>  
> -additionalProperties: false
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            pattern: "^sprd,ums512-.*"
> +
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 1
> +          maxItems: 4
> +
> +        clock-names:
> +          minItems: 1
> +          items:
> +            - const: ext-26m
> +            - const: ext-32k
> +            - const: ext-4m
> +            - const: rco-100m
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            pattern: "^sprd,ums9620-.*"
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 1
> +          maxItems: 6
> +
> +        clock-names:
> +          minItems: 1
> +          maxItems: 6

Missing names.

Most likely you are trying to stuff too many devices into one binding. I
suggest split, could be with some shared/common schema (see Qualcomm GCC
bindings).

> +
> +unevaluatedProperties: false
>  
>  examples:
>    - |
> @@ -68,4 +117,12 @@ examples:
>        clock-names = "ext-26m";
>        #clock-cells = <1>;
>      };
> +
> +    aonapb_gate: clock-controller@0 {
> +      compatible = "sprd,ums9620-aon-gate";

Drop entire example, no need for new example for every new compatible.



Best regards,
Krzysztof

Chunyan Zhang Jan. 4, 2024, 9:09 a.m. UTC | #2

Hi Krzysztof,

On Thu, 4 Jan 2024 at 16:46, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/12/2023 09:51, Chunyan Zhang wrote:
> > Add UMS9620's clock compatible strings and descriptions for clock
> > and clock-names for UMS9620.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> >  .../bindings/clock/sprd,ums512-clk.yaml       | 79 ++++++++++++++++---
> >  1 file changed, 68 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> > index 43d2b6c31357..dcad41a1fea7 100644
> > --- a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> > +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> > @@ -30,25 +30,38 @@ properties:
> >        - sprd,ums512-mm-clk
> >        - sprd,ums512-mm-gate-clk
> >        - sprd,ums512-apapb-gate
> > +      - sprd,ums9620-pmu-gate
> > +      - sprd,ums9620-g1-pll
> > +      - sprd,ums9620-g5l-pll
> > +      - sprd,ums9620-g5r-pll
> > +      - sprd,ums9620-g8-pll
> > +      - sprd,ums9620-g9-pll
> > +      - sprd,ums9620-g10-pll
> > +      - sprd,ums9620-apapb-gate
> > +      - sprd,ums9620-ap-clk
> > +      - sprd,ums9620-apahb-gate
> > +      - sprd,ums9620-aon-gate
> > +      - sprd,ums9620-aonapb-clk
> > +      - sprd,ums9620-topdvfs-clk
> > +      - sprd,ums9620-ipaapb-gate
> > +      - sprd,ums9620-ipa-clk
> > +      - sprd,ums9620-ipaglb-gate
> > +      - sprd,ums9620-ipadispc-gate
> > +      - sprd,ums9620-pcieapb-gate
> > +      - sprd,ums9620-pcie-clk
> > +      - sprd,ums9620-mm-gate
> > +      - sprd,ums9620-mm-clk
> > +      - sprd,ums9620-dpu-vsp-gate
> > +      - sprd,ums9620-dpu-vsp-clk
> >
> >    "#clock-cells":
> >      const: 1
> >
> >    clocks:
> > -    minItems: 1
> > -    maxItems: 4
>
> No, constraints stay here.
>
> >      description: |
> >        The input parent clock(s) phandle for the clock, only list
> >        fixed clocks which are declared in devicetree.
> >
> > -  clock-names:
>
> No, you cannot just drop properties. Widest constraints stay here.
>
> > -    minItems: 1
> > -    items:
> > -      - const: ext-26m
> > -      - const: ext-32k
> > -      - const: ext-4m
> > -      - const: rco-100m
> > -
> >    reg:
> >      maxItems: 1
> >
> > @@ -57,7 +70,43 @@ required:
> >    - '#clock-cells'
> >    - reg
> >
> > -additionalProperties: false
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            pattern: "^sprd,ums512-.*"
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 1
> > +          maxItems: 4
> > +
> > +        clock-names:
> > +          minItems: 1
> > +          items:
> > +            - const: ext-26m
> > +            - const: ext-32k
> > +            - const: ext-4m
> > +            - const: rco-100m
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            pattern: "^sprd,ums9620-.*"
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 1
> > +          maxItems: 6
> > +
> > +        clock-names:
> > +          minItems: 1
> > +          maxItems: 6
>
> Missing names.

The names are fixed, but the order are not.

For example:
clk_a {
    clocks = <&ext_26m>, <&ext_32k>;
    clock-names = "ext-26m", "ext-32k";
};

clk_b {
    clocks = <&ext_26m>, <&ext_4m>;
    clock-names = "ext-26m", "ext-4m";
};

How to list the names for this kind of case? (Please forgive me for
being lazy, didn't find a similar case after a fast search)

Thanks,
Chunyan

Krzysztof Kozlowski Jan. 4, 2024, 9:21 a.m. UTC | #3

On 04/01/2024 10:09, Chunyan Zhang wrote:
> Hi Krzysztof,
> 
> On Thu, 4 Jan 2024 at 16:46, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 29/12/2023 09:51, Chunyan Zhang wrote:
>>> Add UMS9620's clock compatible strings and descriptions for clock
>>> and clock-names for UMS9620.
>>>
>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
>>> ---
>>>  .../bindings/clock/sprd,ums512-clk.yaml       | 79 ++++++++++++++++---
>>>  1 file changed, 68 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>>> index 43d2b6c31357..dcad41a1fea7 100644
>>> --- a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>>> @@ -30,25 +30,38 @@ properties:
>>>        - sprd,ums512-mm-clk
>>>        - sprd,ums512-mm-gate-clk
>>>        - sprd,ums512-apapb-gate
>>> +      - sprd,ums9620-pmu-gate
>>> +      - sprd,ums9620-g1-pll
>>> +      - sprd,ums9620-g5l-pll
>>> +      - sprd,ums9620-g5r-pll
>>> +      - sprd,ums9620-g8-pll
>>> +      - sprd,ums9620-g9-pll
>>> +      - sprd,ums9620-g10-pll
>>> +      - sprd,ums9620-apapb-gate
>>> +      - sprd,ums9620-ap-clk
>>> +      - sprd,ums9620-apahb-gate
>>> +      - sprd,ums9620-aon-gate
>>> +      - sprd,ums9620-aonapb-clk
>>> +      - sprd,ums9620-topdvfs-clk
>>> +      - sprd,ums9620-ipaapb-gate
>>> +      - sprd,ums9620-ipa-clk
>>> +      - sprd,ums9620-ipaglb-gate
>>> +      - sprd,ums9620-ipadispc-gate
>>> +      - sprd,ums9620-pcieapb-gate
>>> +      - sprd,ums9620-pcie-clk
>>> +      - sprd,ums9620-mm-gate
>>> +      - sprd,ums9620-mm-clk
>>> +      - sprd,ums9620-dpu-vsp-gate
>>> +      - sprd,ums9620-dpu-vsp-clk
>>>
>>>    "#clock-cells":
>>>      const: 1
>>>
>>>    clocks:
>>> -    minItems: 1
>>> -    maxItems: 4
>>
>> No, constraints stay here.
>>
>>>      description: |
>>>        The input parent clock(s) phandle for the clock, only list
>>>        fixed clocks which are declared in devicetree.
>>>
>>> -  clock-names:
>>
>> No, you cannot just drop properties. Widest constraints stay here.
>>
>>> -    minItems: 1
>>> -    items:
>>> -      - const: ext-26m
>>> -      - const: ext-32k
>>> -      - const: ext-4m
>>> -      - const: rco-100m
>>> -
>>>    reg:
>>>      maxItems: 1
>>>
>>> @@ -57,7 +70,43 @@ required:
>>>    - '#clock-cells'
>>>    - reg
>>>
>>> -additionalProperties: false
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            pattern: "^sprd,ums512-.*"
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 1
>>> +          maxItems: 4
>>> +
>>> +        clock-names:
>>> +          minItems: 1
>>> +          items:
>>> +            - const: ext-26m
>>> +            - const: ext-32k
>>> +            - const: ext-4m
>>> +            - const: rco-100m
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            pattern: "^sprd,ums9620-.*"
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 1
>>> +          maxItems: 6
>>> +
>>> +        clock-names:
>>> +          minItems: 1
>>> +          maxItems: 6
>>
>> Missing names.
> 
> The names are fixed, but the order are not.

Order must be fixed.

> 
> For example:
> clk_a {
>     clocks = <&ext_26m>, <&ext_32k>;
>     clock-names = "ext-26m", "ext-32k";
> };
> 
> clk_b {
>     clocks = <&ext_26m>, <&ext_4m>;
>     clock-names = "ext-26m", "ext-4m";

And here the order is fixed...

> };
> 
> How to list the names for this kind of case? (Please forgive me for
> being lazy, didn't find a similar case after a fast search)

The same ums512 lists them or mentioned other bindings. You somehow cut
the quote...

Best regards,
Krzysztof

Chunyan Zhang Jan. 4, 2024, 9:26 a.m. UTC | #4

On Thu, 4 Jan 2024 at 17:21, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/01/2024 10:09, Chunyan Zhang wrote:
> > Hi Krzysztof,
> >
> > On Thu, 4 Jan 2024 at 16:46, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 29/12/2023 09:51, Chunyan Zhang wrote:
> >>> Add UMS9620's clock compatible strings and descriptions for clock
> >>> and clock-names for UMS9620.
> >>>
> >>> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >>> ---
> >>>  .../bindings/clock/sprd,ums512-clk.yaml       | 79 ++++++++++++++++---
> >>>  1 file changed, 68 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> >>> index 43d2b6c31357..dcad41a1fea7 100644
> >>> --- a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> >>> +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> >>> @@ -30,25 +30,38 @@ properties:
> >>>        - sprd,ums512-mm-clk
> >>>        - sprd,ums512-mm-gate-clk
> >>>        - sprd,ums512-apapb-gate
> >>> +      - sprd,ums9620-pmu-gate
> >>> +      - sprd,ums9620-g1-pll
> >>> +      - sprd,ums9620-g5l-pll
> >>> +      - sprd,ums9620-g5r-pll
> >>> +      - sprd,ums9620-g8-pll
> >>> +      - sprd,ums9620-g9-pll
> >>> +      - sprd,ums9620-g10-pll
> >>> +      - sprd,ums9620-apapb-gate
> >>> +      - sprd,ums9620-ap-clk
> >>> +      - sprd,ums9620-apahb-gate
> >>> +      - sprd,ums9620-aon-gate
> >>> +      - sprd,ums9620-aonapb-clk
> >>> +      - sprd,ums9620-topdvfs-clk
> >>> +      - sprd,ums9620-ipaapb-gate
> >>> +      - sprd,ums9620-ipa-clk
> >>> +      - sprd,ums9620-ipaglb-gate
> >>> +      - sprd,ums9620-ipadispc-gate
> >>> +      - sprd,ums9620-pcieapb-gate
> >>> +      - sprd,ums9620-pcie-clk
> >>> +      - sprd,ums9620-mm-gate
> >>> +      - sprd,ums9620-mm-clk
> >>> +      - sprd,ums9620-dpu-vsp-gate
> >>> +      - sprd,ums9620-dpu-vsp-clk
> >>>
> >>>    "#clock-cells":
> >>>      const: 1
> >>>
> >>>    clocks:
> >>> -    minItems: 1
> >>> -    maxItems: 4
> >>
> >> No, constraints stay here.
> >>
> >>>      description: |
> >>>        The input parent clock(s) phandle for the clock, only list
> >>>        fixed clocks which are declared in devicetree.
> >>>
> >>> -  clock-names:
> >>
> >> No, you cannot just drop properties. Widest constraints stay here.
> >>
> >>> -    minItems: 1
> >>> -    items:
> >>> -      - const: ext-26m
> >>> -      - const: ext-32k
> >>> -      - const: ext-4m
> >>> -      - const: rco-100m
> >>> -
> >>>    reg:
> >>>      maxItems: 1
> >>>
> >>> @@ -57,7 +70,43 @@ required:
> >>>    - '#clock-cells'
> >>>    - reg
> >>>
> >>> -additionalProperties: false
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            pattern: "^sprd,ums512-.*"
> >>> +
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          minItems: 1
> >>> +          maxItems: 4
> >>> +
> >>> +        clock-names:
> >>> +          minItems: 1
> >>> +          items:
> >>> +            - const: ext-26m
> >>> +            - const: ext-32k
> >>> +            - const: ext-4m
> >>> +            - const: rco-100m
> >>> +
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            pattern: "^sprd,ums9620-.*"
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          minItems: 1
> >>> +          maxItems: 6
> >>> +
> >>> +        clock-names:
> >>> +          minItems: 1
> >>> +          maxItems: 6
> >>
> >> Missing names.
> >
> > The names are fixed, but the order are not.
>
> Order must be fixed.
>
> >
> > For example:
> > clk_a {
> >     clocks = <&ext_26m>, <&ext_32k>;
> >     clock-names = "ext-26m", "ext-32k";
> > };
> >
> > clk_b {
> >     clocks = <&ext_26m>, <&ext_4m>;
> >     clock-names = "ext-26m", "ext-4m";
>
> And here the order is fixed...

The order is not fixed, 'clk_b' will cause dtb_check error, since it
skips the second one i.e. ext-32k in the clock-names list.

>
> > };
> >
> > How to list the names for this kind of case? (Please forgive me for
> > being lazy, didn't find a similar case after a fast search)
>
> The same ums512 lists them or mentioned other bindings. You somehow cut
> the quote...
>
> Best regards,
> Krzysztof
>

Krzysztof Kozlowski Jan. 4, 2024, 9:31 a.m. UTC | #5

On 04/01/2024 10:26, Chunyan Zhang wrote:
>>>> Missing names.
>>>
>>> The names are fixed, but the order are not.
>>
>> Order must be fixed.

Did you read this? It must be. If it is not, your patchset has issues
you must fix.


>>
>>>
>>> For example:
>>> clk_a {
>>>     clocks = <&ext_26m>, <&ext_32k>;
>>>     clock-names = "ext-26m", "ext-32k";
>>> };
>>>
>>> clk_b {
>>>     clocks = <&ext_26m>, <&ext_4m>;
>>>     clock-names = "ext-26m", "ext-4m";
>>
>> And here the order is fixed...
> 
> The order is not fixed, 'clk_b' will cause dtb_check error, since it
> skips the second one i.e. ext-32k in the clock-names list.

Then why do you use the same compatible for two different devices?

Best regards,
Krzysztof

[1/3] dt-bindings: clk: sprd: Add UMS9620 support

Checks

Commit Message

Comments

Patch