dt-bindings: clock: ti: Convert interface.txt to json-schema

  Convert the OMAP interface clock device tree binding to json-schema
and fix up reg property which is optional and taken from parent if
not specified.
Specify the creator of the original binding as a maintainer.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 .../bindings/clock/ti/interface.txt           | 57 ------------
 .../bindings/clock/ti/ti,interface-clock.yaml | 90 +++++++++++++++++++
 2 files changed, 90 insertions(+), 57 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/ti/interface.txt
 create mode 100644 Documentation/devicetree/bindings/clock/ti/ti,interface-clock.yaml
  

On 27/11/2023 21:23, Andreas Kemnade wrote:
> Convert the OMAP interface clock device tree binding to json-schema
> and fix up reg property which is optional and taken from parent if
> not specified.
> Specify the creator of the original binding as a maintainer.
> 

...

> diff --git a/Documentation/devicetree/bindings/clock/ti/ti,interface-clock.yaml b/Documentation/devicetree/bindings/clock/ti/ti,interface-clock.yaml
> new file mode 100644
> index 0000000000000..48a54caeb3857
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti/ti,interface-clock.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ti/ti,interface-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments interface clock.
> +
> +maintainers:
> +  - Tero Kristo <kristo@kernel.org>
> +
> +description: |
> +  This binding uses the common clock binding[1]. This clock is

Drop first sentence, useless. Can a clock binding not use common clock
binding?

> +  quite much similar to the basic gate-clock[2], however,
> +  it supports a number of additional features, including
> +  companion clock finding (match corresponding functional gate
> +  clock) and hardware autoidle enable / disable.
> +
> +  [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +  [2] Documentation/devicetree/bindings/clock/gpio-gate-clock.yaml
> +
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,omap3-interface-clock           # basic OMAP3 interface clock
> +      - ti,omap3-no-wait-interface-clock   # interface clock which has no hardware
> +                                           # capability for waiting clock to be ready
> +      - ti,omap3-hsotgusb-interface-clock  # interface clock with USB specific HW handling
> +      - ti,omap3-dss-interface-clock       # interface clock with DSS specific HW handling
> +      - ti,omap3-ssi-interface-clock       # interface clock with SSI specific HW handling
> +      - ti,am35xx-interface-clock          # interface clock with AM35xx specific HW handling
> +      - ti,omap2430-interface-clock        # interface clock with OMAP2430 specific HW handling

Blank line

> +  "#clock-cells":
> +    const: 0
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  reg:
> +    description:
> +      if not specified, value from parent is used

Eh, what? How? Either this is on the bus or not. You added it, because
it wasn't even in the original binding. Then please explain what does it
mean?

> +    maxItems: 1
> +
> +  ti,bit-shift:
> +    description:
> +      bit shift for the bit enabling/disabling the clock
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +
> +required:
> +  - compatible
> +  - clocks
> +  - '#clock-cells'

reg is required. Device cannot take "reg" from parent, DTS does not work
like this.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      aes1_ick: aes1-ick@48004a14 {

clock-controller@


> +        #clock-cells = <0>;
> +        compatible = "ti,omap3-interface-clock";
> +        clocks = <&security_l4_ick2>;
> +        reg = <0x48004a14 0x4>;
> +        ti,bit-shift = <3>;
> +      };
> +

And drop rest of examples - they do not bring anything different.



Best regards,
Krzysztof
  

On Tue, 28 Nov 2023 09:00:16 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> > +required:
> > +  - compatible
> > +  - clocks
> > +  - '#clock-cells'  
> 
> reg is required. Device cannot take "reg" from parent, DTS does not work
> like this.

Well, apparently they do... and I am just dealing with status quo and not
how it should be.
Look at commit 31fc1c63c2ae4a542e3c7ac572a10a59ece45c24
for the reasoning of not having reg.


well, look at drivers/clk/ti/clk.c
ti_clk_get_reg_addr();

...

       if (of_property_read_u32_index(node, "reg", index, &val)) {
                if (of_property_read_u32_index(node->parent, "reg",
                                               index, &val)) {
                        pr_err("%pOFn or parent must have reg[%d]!\n",
                               node, index);
                        return -EINVAL;
                }
        }


We have two usecases here (status quo in dts usage and code):
If these interface clocks are below a ti,clksel then we are describing 
multiple bits in the same register and therefore every child of ti,clksel
would have the same reg.
If the interface clock is not below a ti,clksel then we have reg.

Regards,
Andreas
  

On 28/11/2023 09:32, Andreas Kemnade wrote:
> On Tue, 28 Nov 2023 09:00:16 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>>> +required:
>>> +  - compatible
>>> +  - clocks
>>> +  - '#clock-cells'  
>>
>> reg is required. Device cannot take "reg" from parent, DTS does not work
>> like this.
> 
> Well, apparently they do... and I am just dealing with status quo and not
> how it should be.
> Look at commit 31fc1c63c2ae4a542e3c7ac572a10a59ece45c24

Who designed clock-controller binding with a device node per each clock?
This is ridiculous (although of course not your fault here)! Looking at
omap3xxx-clocks.dtsi - all its children should be just defined by the
driver, not by DTSI.


> for the reasoning of not having reg.
> 
> 
> well, look at drivers/clk/ti/clk.c
> ti_clk_get_reg_addr();

That's a driver implementation, not bindings, thus confusion.

> 
> ...
> 
>        if (of_property_read_u32_index(node, "reg", index, &val)) {
>                 if (of_property_read_u32_index(node->parent, "reg",
>                                                index, &val)) {
>                         pr_err("%pOFn or parent must have reg[%d]!\n",
>                                node, index);
>                         return -EINVAL;
>                 }
>         }
> 
> 
> We have two usecases here (status quo in dts usage and code):
> If these interface clocks are below a ti,clksel then we are describing 
> multiple bits in the same register and therefore every child of ti,clksel
> would have the same reg.

Regs can have bits, so that could still work.

> If the interface clock is not below a ti,clksel then we have reg.

This should be expressed in the bindings. It's fine to make the reg
optional (skip the description, it's confusing), but the ti,clksel
should reference this schema and enforce it on the children.

Best regards,
Krzysztof
  

* Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231128 08:41]:
> On 28/11/2023 09:32, Andreas Kemnade wrote:
> > On Tue, 28 Nov 2023 09:00:16 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >>> +required:
> >>> +  - compatible
> >>> +  - clocks
> >>> +  - '#clock-cells'  
> >>
> >> reg is required. Device cannot take "reg" from parent, DTS does not work
> >> like this.
> > 
> > Well, apparently they do... and I am just dealing with status quo and not
> > how it should be.
> > Look at commit 31fc1c63c2ae4a542e3c7ac572a10a59ece45c24
> 
> Who designed clock-controller binding with a device node per each clock?
> This is ridiculous (although of course not your fault here)! Looking at
> omap3xxx-clocks.dtsi - all its children should be just defined by the
> driver, not by DTSI.

Earlier all the clocks were separate nodes, the ti,clksel binding made
things a bit better by grouping the seprate clock nodes so we don't have
multiple nodes with the same reg.. But yeah clksel instance clocks should
be clock@6 with reg = <6> if the clock bits are at bit 6. That would be
fairly easy to do if that helps, but in general I doubt anybody's going
to spend much effort to fix the omap3 legacy clocks atthis point.

For omap4 and later, things are a bit better as they use the clkctrl clocks:

Documentation/devicetree/bindings/clock/ti-clkctrl.txt

I don't think omap3 has any clkctrl clocks but if it does then that could
be used.

Regards,

Tony
  

On Mon, Nov 27, 2023 at 09:23:59PM +0100, Andreas Kemnade wrote:
> Convert the OMAP interface clock device tree binding to json-schema
> and fix up reg property which is optional and taken from parent if
> not specified.
> Specify the creator of the original binding as a maintainer.

Great! This and other TI clocks are at the top of the list[1] of 
occurrences of undocumented (by schemas) compatibles: 

   3763 ['ti,omap3-interface-clock']
   3249 ['ti,divider-clock']
   1764 ['ti,mux-clock']
   1680 ['ti,gate-clock']
   1522 ['ti,wait-gate-clock']
   1459 ['ti,composite-clock']
   1343 ['ti,composite-mux-clock']
   1341 ['ti,clkctrl']
   1296 ['fsl,imx6q-ssi', 'fsl,imx51-ssi']
   1196 ['ti,composite-gate-clock']
   1032 ['ti,clockdomain']

Of course, that's largely due to OMAP being early clock adopter and 
trying to do fine-grained clocks in DT.

Rob

[1] https://gitlab.com/robherring/linux-dt/-/jobs/5620809910#L5618
  

Am Tue, 28 Nov 2023 09:41:23 +0100
schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:

[...] 
> > We have two usecases here (status quo in dts usage and code):
> > If these interface clocks are below a ti,clksel then we are
> > describing multiple bits in the same register and therefore every
> > child of ti,clksel would have the same reg.  
> 
> Regs can have bits, so that could still work.
> 
Yes, it could. Things could be designed in another way. But for now I
want to get rid of dtbs_check warnings and finally have some fun with
that and not always tempted to skip it and just copy over $bad_example.
So I am not in the mood of redesigning everything.

> > If the interface clock is not below a ti,clksel then we have reg.  
> 
> This should be expressed in the bindings. It's fine to make the reg
> optional (skip the description, it's confusing), but the ti,clksel
> should reference this schema and enforce it on the children.
> 
Well there are other compatibles below ti,clksel, too, so should we
rather add them when the other .txt files are converted?

Regards,
Andreas
  

On 28/11/2023 21:41, Andreas Kemnade wrote:
> Am Tue, 28 Nov 2023 09:41:23 +0100
> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>>> If the interface clock is not below a ti,clksel then we have reg.  
>>
>> This should be expressed in the bindings. It's fine to make the reg
>> optional (skip the description, it's confusing), but the ti,clksel
>> should reference this schema and enforce it on the children.
>>
> Well there are other compatibles below ti,clksel, too, so should we
> rather add them when the other .txt files are converted?

This binding should already be referenced by ti,clksel. When the other
are ready, you will change additionalProperties from object to false.

Best regards,
Krzysztof
  

* Rob Herring <robh@kernel.org> [231128 17:16]:
>    1341 ['ti,clkctrl']

Are the 1341 ti,clkctrl warnings node name underscore warnings?

If so I think I have a patch already for some of those that I need
to dig up and finish..

Regadrs,

Tony
  

Am Wed, 29 Nov 2023 09:15:57 +0100
schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:

> On 28/11/2023 21:41, Andreas Kemnade wrote:
> > Am Tue, 28 Nov 2023 09:41:23 +0100
> > schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:  
> >>> If the interface clock is not below a ti,clksel then we have reg.
> >>>    
> >>
> >> This should be expressed in the bindings. It's fine to make the reg
> >> optional (skip the description, it's confusing), but the ti,clksel
> >> should reference this schema and enforce it on the children.
> >>  
> > Well there are other compatibles below ti,clksel, too, so should we
> > rather add them when the other .txt files are converted?  
> 
> This binding should already be referenced by ti,clksel. When the other
> are ready, you will change additionalProperties from object to false.
>
I played around with it:

--- a/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
+++ b/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
@@ -33,6 +33,11 @@ properties:
     const: 2
     description: The CLKSEL register and bit offset
 
+patternProperties:
+  "-ick$":
+    $ref: /schemas/clock/ti/ti,interface-clock.yaml#
+    type: object
+
 required:
   - compatible
   - reg

 
That generates warnings, which look more serious than just a
non-converted compatible, so lowering the overall "signal-noise-ratio".

e.g.
from schema $id:
http://devicetree.org/schemas/clock/ti/ti,clksel.yaml#
/home/andi/linux-dtbs/arch/arm/boot/dts/ti/omap/omap3-overo-tobiduo.dtb:
clock@c40: clock-rm-ick: 'ti,index-starts-at-one', 'ti,max-div' do not
match any of the regexes: 'pinctrl-[0-9]+'

I think we should rather postpone such referencing.

Regards,
Andreas
  

On 01/12/2023 15:09, Andreas Kemnade wrote:
> Am Wed, 29 Nov 2023 09:15:57 +0100
> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> 
>> On 28/11/2023 21:41, Andreas Kemnade wrote:
>>> Am Tue, 28 Nov 2023 09:41:23 +0100
>>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:  
>>>>> If the interface clock is not below a ti,clksel then we have reg.
>>>>>    
>>>>
>>>> This should be expressed in the bindings. It's fine to make the reg
>>>> optional (skip the description, it's confusing), but the ti,clksel
>>>> should reference this schema and enforce it on the children.
>>>>  
>>> Well there are other compatibles below ti,clksel, too, so should we
>>> rather add them when the other .txt files are converted?  
>>
>> This binding should already be referenced by ti,clksel. When the other
>> are ready, you will change additionalProperties from object to false.
>>
> I played around with it:
> 
> --- a/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
> +++ b/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
> @@ -33,6 +33,11 @@ properties:
>      const: 2
>      description: The CLKSEL register and bit offset
>  
> +patternProperties:
> +  "-ick$":
> +    $ref: /schemas/clock/ti/ti,interface-clock.yaml#
> +    type: object
> +
>  required:
>    - compatible
>    - reg
> 
>  
> That generates warnings, which look more serious than just a
> non-converted compatible, so lowering the overall "signal-noise-ratio".
> 
> e.g.
> from schema $id:
> http://devicetree.org/schemas/clock/ti/ti,clksel.yaml#
> /home/andi/linux-dtbs/arch/arm/boot/dts/ti/omap/omap3-overo-tobiduo.dtb:
> clock@c40: clock-rm-ick: 'ti,index-starts-at-one', 'ti,max-div' do not
> match any of the regexes: 'pinctrl-[0-9]+'
> 
> I think we should rather postpone such referencing.

Are you sure in such case that your binding is correct? The warnings
suggest that not, therefore please do not postpone.

Best regards,
Krzysztof
  

On Fri, 1 Dec 2023 15:17:46 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 01/12/2023 15:09, Andreas Kemnade wrote:
> > Am Wed, 29 Nov 2023 09:15:57 +0100
> > schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> >   
> >> On 28/11/2023 21:41, Andreas Kemnade wrote:  
> >>> Am Tue, 28 Nov 2023 09:41:23 +0100
> >>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:    
> >>>>> If the interface clock is not below a ti,clksel then we have reg.
> >>>>>      
> >>>>
> >>>> This should be expressed in the bindings. It's fine to make the reg
> >>>> optional (skip the description, it's confusing), but the ti,clksel
> >>>> should reference this schema and enforce it on the children.
> >>>>    
> >>> Well there are other compatibles below ti,clksel, too, so should we
> >>> rather add them when the other .txt files are converted?    
> >>
> >> This binding should already be referenced by ti,clksel. When the other
> >> are ready, you will change additionalProperties from object to false.
> >>  
> > I played around with it:
> > 
> > --- a/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
> > +++ b/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
> > @@ -33,6 +33,11 @@ properties:
> >      const: 2
> >      description: The CLKSEL register and bit offset
> >  
> > +patternProperties:
> > +  "-ick$":
> > +    $ref: /schemas/clock/ti/ti,interface-clock.yaml#
> > +    type: object
> > +
> >  required:
> >    - compatible
> >    - reg
> > 
> >  
> > That generates warnings, which look more serious than just a
> > non-converted compatible, so lowering the overall "signal-noise-ratio".
> > 
> > e.g.
> > from schema $id:
> > http://devicetree.org/schemas/clock/ti/ti,clksel.yaml#
> > /home/andi/linux-dtbs/arch/arm/boot/dts/ti/omap/omap3-overo-tobiduo.dtb:
> > clock@c40: clock-rm-ick: 'ti,index-starts-at-one', 'ti,max-div' do not
> > match any of the regexes: 'pinctrl-[0-9]+'
> > 
> > I think we should rather postpone such referencing.  
> 
> Are you sure in such case that your binding is correct? The warnings
> suggest that not, therefore please do not postpone.
> 
well, there is not only stuff from clock/ti/ti,interface.yaml but also from
clock/ti/divider.txt below ti,clksel. So I have one warning about the missing
compatible there and also about the properties belonging to that compatible.

Regards,
Andreas
  

On 01/12/2023 15:41, Andreas Kemnade wrote:
> On Fri, 1 Dec 2023 15:17:46 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 01/12/2023 15:09, Andreas Kemnade wrote:
>>> Am Wed, 29 Nov 2023 09:15:57 +0100
>>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>>>   
>>>> On 28/11/2023 21:41, Andreas Kemnade wrote:  
>>>>> Am Tue, 28 Nov 2023 09:41:23 +0100
>>>>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:    
>>>>>>> If the interface clock is not below a ti,clksel then we have reg.
>>>>>>>      
>>>>>>
>>>>>> This should be expressed in the bindings. It's fine to make the reg
>>>>>> optional (skip the description, it's confusing), but the ti,clksel
>>>>>> should reference this schema and enforce it on the children.
>>>>>>    
>>>>> Well there are other compatibles below ti,clksel, too, so should we
>>>>> rather add them when the other .txt files are converted?    
>>>>
>>>> This binding should already be referenced by ti,clksel. When the other
>>>> are ready, you will change additionalProperties from object to false.
>>>>  
>>> I played around with it:
>>>
>>> --- a/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
>>> @@ -33,6 +33,11 @@ properties:
>>>      const: 2
>>>      description: The CLKSEL register and bit offset
>>>  
>>> +patternProperties:
>>> +  "-ick$":
>>> +    $ref: /schemas/clock/ti/ti,interface-clock.yaml#
>>> +    type: object
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>>
>>>  
>>> That generates warnings, which look more serious than just a
>>> non-converted compatible, so lowering the overall "signal-noise-ratio".
>>>
>>> e.g.
>>> from schema $id:
>>> http://devicetree.org/schemas/clock/ti/ti,clksel.yaml#
>>> /home/andi/linux-dtbs/arch/arm/boot/dts/ti/omap/omap3-overo-tobiduo.dtb:
>>> clock@c40: clock-rm-ick: 'ti,index-starts-at-one', 'ti,max-div' do not
>>> match any of the regexes: 'pinctrl-[0-9]+'
>>>
>>> I think we should rather postpone such referencing.  
>>
>> Are you sure in such case that your binding is correct? The warnings
>> suggest that not, therefore please do not postpone.
>>
> well, there is not only stuff from clock/ti/ti,interface.yaml but also from
> clock/ti/divider.txt below ti,clksel. So I have one warning about the missing
> compatible there and also about the properties belonging to that compatible.

Ah, you have other bindings for the "-ick" nodes? Then you cannot match
by pattern now, indeed. Maybe skipping ref but adding "compatible" into
node, like we do for Qualcomm mdss bindings, would work. But in general
all these should be converted at the same time.

Best regards,
Krzysztof
  

On Fri, 1 Dec 2023 15:45:06 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 01/12/2023 15:41, Andreas Kemnade wrote:
> > On Fri, 1 Dec 2023 15:17:46 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> On 01/12/2023 15:09, Andreas Kemnade wrote:  
> >>> Am Wed, 29 Nov 2023 09:15:57 +0100
> >>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> >>>     
> >>>> On 28/11/2023 21:41, Andreas Kemnade wrote:    
> >>>>> Am Tue, 28 Nov 2023 09:41:23 +0100
> >>>>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:      
> >>>>>>> If the interface clock is not below a ti,clksel then we have reg.
> >>>>>>>        
> >>>>>>
> >>>>>> This should be expressed in the bindings. It's fine to make the reg
> >>>>>> optional (skip the description, it's confusing), but the ti,clksel
> >>>>>> should reference this schema and enforce it on the children.
> >>>>>>      
> >>>>> Well there are other compatibles below ti,clksel, too, so should we
> >>>>> rather add them when the other .txt files are converted?      
> >>>>
> >>>> This binding should already be referenced by ti,clksel. When the other
> >>>> are ready, you will change additionalProperties from object to false.
> >>>>    
> >>> I played around with it:
> >>>
> >>> --- a/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
> >>> +++ b/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
> >>> @@ -33,6 +33,11 @@ properties:
> >>>      const: 2
> >>>      description: The CLKSEL register and bit offset
> >>>  
> >>> +patternProperties:
> >>> +  "-ick$":
> >>> +    $ref: /schemas/clock/ti/ti,interface-clock.yaml#
> >>> +    type: object
> >>> +
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>>
> >>>  
> >>> That generates warnings, which look more serious than just a
> >>> non-converted compatible, so lowering the overall "signal-noise-ratio".
> >>>
> >>> e.g.
> >>> from schema $id:
> >>> http://devicetree.org/schemas/clock/ti/ti,clksel.yaml#
> >>> /home/andi/linux-dtbs/arch/arm/boot/dts/ti/omap/omap3-overo-tobiduo.dtb:
> >>> clock@c40: clock-rm-ick: 'ti,index-starts-at-one', 'ti,max-div' do not
> >>> match any of the regexes: 'pinctrl-[0-9]+'
> >>>
> >>> I think we should rather postpone such referencing.    
> >>
> >> Are you sure in such case that your binding is correct? The warnings
> >> suggest that not, therefore please do not postpone.
> >>  
> > well, there is not only stuff from clock/ti/ti,interface.yaml but also from
> > clock/ti/divider.txt below ti,clksel. So I have one warning about the missing
> > compatible there and also about the properties belonging to that compatible.  
> 
> Ah, you have other bindings for the "-ick" nodes? Then you cannot match
> by pattern now, indeed. Maybe skipping ref but adding "compatible" into
> node, like we do for Qualcomm mdss bindings, would work. But in general
> all these should be converted at the same time.
> 
Yes, there are other bindings for the "-ick" nodes. But these bindings
are not exclusive to the "-ick" nodes. I personally would prefer not
having to do the whole clock/ti/*.txt directory at once.

Regards,
Andreas
  

On 03/12/2023 23:46, Andreas Kemnade wrote:
> On Fri, 1 Dec 2023 15:45:06 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 01/12/2023 15:41, Andreas Kemnade wrote:
>>> On Fri, 1 Dec 2023 15:17:46 +0100
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>   
>>>> On 01/12/2023 15:09, Andreas Kemnade wrote:  
>>>>> Am Wed, 29 Nov 2023 09:15:57 +0100
>>>>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>>>>>     
>>>>>> On 28/11/2023 21:41, Andreas Kemnade wrote:    
>>>>>>> Am Tue, 28 Nov 2023 09:41:23 +0100
>>>>>>> schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:      
>>>>>>>>> If the interface clock is not below a ti,clksel then we have reg.
>>>>>>>>>        
>>>>>>>>
>>>>>>>> This should be expressed in the bindings. It's fine to make the reg
>>>>>>>> optional (skip the description, it's confusing), but the ti,clksel
>>>>>>>> should reference this schema and enforce it on the children.
>>>>>>>>      
>>>>>>> Well there are other compatibles below ti,clksel, too, so should we
>>>>>>> rather add them when the other .txt files are converted?      
>>>>>>
>>>>>> This binding should already be referenced by ti,clksel. When the other
>>>>>> are ready, you will change additionalProperties from object to false.
>>>>>>    
>>>>> I played around with it:
>>>>>
>>>>> --- a/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/ti/ti,clksel.yaml
>>>>> @@ -33,6 +33,11 @@ properties:
>>>>>      const: 2
>>>>>      description: The CLKSEL register and bit offset
>>>>>  
>>>>> +patternProperties:
>>>>> +  "-ick$":
>>>>> +    $ref: /schemas/clock/ti/ti,interface-clock.yaml#
>>>>> +    type: object
>>>>> +
>>>>>  required:
>>>>>    - compatible
>>>>>    - reg
>>>>>
>>>>>  
>>>>> That generates warnings, which look more serious than just a
>>>>> non-converted compatible, so lowering the overall "signal-noise-ratio".
>>>>>
>>>>> e.g.
>>>>> from schema $id:
>>>>> http://devicetree.org/schemas/clock/ti/ti,clksel.yaml#
>>>>> /home/andi/linux-dtbs/arch/arm/boot/dts/ti/omap/omap3-overo-tobiduo.dtb:
>>>>> clock@c40: clock-rm-ick: 'ti,index-starts-at-one', 'ti,max-div' do not
>>>>> match any of the regexes: 'pinctrl-[0-9]+'
>>>>>
>>>>> I think we should rather postpone such referencing.    
>>>>
>>>> Are you sure in such case that your binding is correct? The warnings
>>>> suggest that not, therefore please do not postpone.
>>>>  
>>> well, there is not only stuff from clock/ti/ti,interface.yaml but also from
>>> clock/ti/divider.txt below ti,clksel. So I have one warning about the missing
>>> compatible there and also about the properties belonging to that compatible.  
>>
>> Ah, you have other bindings for the "-ick" nodes? Then you cannot match
>> by pattern now, indeed. Maybe skipping ref but adding "compatible" into
>> node, like we do for Qualcomm mdss bindings, would work. But in general
>> all these should be converted at the same time.
>>
> Yes, there are other bindings for the "-ick" nodes. But these bindings
> are not exclusive to the "-ick" nodes. I personally would prefer not
> having to do the whole clock/ti/*.txt directory at once.

This is what usually is expected for multiple schemas used together
(common for MFD). Don't convert part of device but everything needed for
the main node. It's not different here.

But sure, you can go with mdss approach.

Best regards,
Krzysztof
  

Hi,

* Rob Herring <robh@kernel.org> [231128 17:16]:
> On Mon, Nov 27, 2023 at 09:23:59PM +0100, Andreas Kemnade wrote:
> > Convert the OMAP interface clock device tree binding to json-schema
> > and fix up reg property which is optional and taken from parent if
> > not specified.
> > Specify the creator of the original binding as a maintainer.
> 
> Great! This and other TI clocks are at the top of the list[1] of 
> occurrences of undocumented (by schemas) compatibles: 
> 
>    3763 ['ti,omap3-interface-clock']
>    3249 ['ti,divider-clock']
>    1764 ['ti,mux-clock']
>    1680 ['ti,gate-clock']
>    1522 ['ti,wait-gate-clock']
>    1459 ['ti,composite-clock']
>    1343 ['ti,composite-mux-clock']
>    1341 ['ti,clkctrl']
>    1296 ['fsl,imx6q-ssi', 'fsl,imx51-ssi']
>    1196 ['ti,composite-gate-clock']
>    1032 ['ti,clockdomain']
> 
> Of course, that's largely due to OMAP being early clock adopter and 
> trying to do fine-grained clocks in DT.

So related to dealing with the warnings above, and the numerous
warnings for unique_unit_address, I suggest we update the clksel clock
children for the standard reg property as we already discussed a bit
earlier.

The suggested patch for the am3 clksel children is below for reference.
I have at least one issue to sort out before I can post proper patches.

The issue I'm seeing is that updating omap3 clkcsel clocks in a similar
way adds a new error that gets multiplied by about 50 times as the
dss_tv_fck and dss_96m_fck both seem to really be gated by the same
bit..

I think the dss_tv_fck might be derived from the dss_96m_fck really,
and the documentation is wrong. If anybody has more info on this please
let me know, otherwise I guess I'll just leave the clock@e00 not updated
for now.

Regards,

Tony

> [1] https://gitlab.com/robherring/linux-dt/-/jobs/5620809910#L5618
>

8< ---------------------------
diff --git a/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi b/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi
--- a/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi
@@ -108,30 +108,31 @@ clock@664 {
 		compatible = "ti,clksel";
 		reg = <0x664>;
 		#clock-cells = <2>;
-		#address-cells = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
 
-		ehrpwm0_tbclk: clock-ehrpwm0-tbclk {
+		ehrpwm0_tbclk: clock-ehrpwm0-tbclk@0 {
+			reg = <0>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "ehrpwm0_tbclk";
 			clocks = <&l4ls_gclk>;
-			ti,bit-shift = <0>;
 		};
 
-		ehrpwm1_tbclk: clock-ehrpwm1-tbclk {
+		ehrpwm1_tbclk: clock-ehrpwm1-tbclk@1 {
+			reg = <1>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "ehrpwm1_tbclk";
 			clocks = <&l4ls_gclk>;
-			ti,bit-shift = <1>;
 		};
 
-		ehrpwm2_tbclk: clock-ehrpwm2-tbclk {
+		ehrpwm2_tbclk: clock-ehrpwm2-tbclk@2 {
+			reg = <2>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "ehrpwm2_tbclk";
 			clocks = <&l4ls_gclk>;
-			ti,bit-shift = <2>;
 		};
 	};
 };
@@ -566,17 +567,19 @@ clock@52c {
 		compatible = "ti,clksel";
 		reg = <0x52c>;
 		#clock-cells = <2>;
-		#address-cells = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
 
-		gfx_fclk_clksel_ck: clock-gfx-fclk-clksel {
+		gfx_fclk_clksel_ck: clock-gfx-fclk-clksel@1 {
+			reg = <1>;
 			#clock-cells = <0>;
 			compatible = "ti,mux-clock";
 			clock-output-names = "gfx_fclk_clksel_ck";
 			clocks = <&dpll_core_m4_ck>, <&dpll_per_m2_ck>;
-			ti,bit-shift = <1>;
 		};
 
-		gfx_fck_div_ck: clock-gfx-fck-div {
+		gfx_fck_div_ck: clock-gfx-fck-div@0 {
+			reg = <0>;
 			#clock-cells = <0>;
 			compatible = "ti,divider-clock";
 			clock-output-names = "gfx_fck_div_ck";
@@ -589,30 +592,32 @@ clock@700 {
 		compatible = "ti,clksel";
 		reg = <0x700>;
 		#clock-cells = <2>;
-		#address-cells = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
 
-		sysclkout_pre_ck: clock-sysclkout-pre {
+		sysclkout_pre_ck: clock-sysclkout-pre@0 {
+			reg = <0>;
 			#clock-cells = <0>;
 			compatible = "ti,mux-clock";
 			clock-output-names = "sysclkout_pre_ck";
 			clocks = <&clk_32768_ck>, <&l3_gclk>, <&dpll_ddr_m2_ck>, <&dpll_per_m2_ck>, <&lcd_gclk>;
 		};
 
-		clkout2_div_ck: clock-clkout2-div {
+		clkout2_div_ck: clock-clkout2-div@3 {
+			reg = <3>;
 			#clock-cells = <0>;
 			compatible = "ti,divider-clock";
 			clock-output-names = "clkout2_div_ck";
 			clocks = <&sysclkout_pre_ck>;
-			ti,bit-shift = <3>;
 			ti,max-div = <8>;
 		};
 
-		clkout2_ck: clock-clkout2 {
+		clkout2_ck: clock-clkout2@7 {
+			reg = <7>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "clkout2_ck";
 			clocks = <&clkout2_div_ck>;
-			ti,bit-shift = <7>;
 		};
 	};
 };