| Message ID | 20231127202359.145778-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: clock: ti: Convert interface.txt to json-schema | expand |
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>; }; }; };
diff --git a/Documentation/devicetree/bindings/clock/ti/interface.txt b/Documentation/devicetree/bindings/clock/ti/interface.txt deleted file mode 100644 index d3eb5ca92a7fe..0000000000000 --- a/Documentation/devicetree/bindings/clock/ti/interface.txt +++ /dev/null @@ -1,57 +0,0 @@ -Binding for Texas Instruments interface clock. - -Binding status: Unstable - ABI compatibility may be broken in the future - -This binding uses the common clock binding[1]. This clock is -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 - -Required properties: -- compatible : shall be one of: - "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 -- #clock-cells : from common clock binding; shall be set to 0 -- clocks : link to phandle of parent clock -- reg : base address for the control register - -Optional properties: -- clock-output-names : from common clock binding. -- ti,bit-shift : bit shift for the bit enabling/disabling the clock (default 0) - -Examples: - aes1_ick: aes1_ick@48004a14 { - #clock-cells = <0>; - compatible = "ti,omap3-interface-clock"; - clocks = <&security_l4_ick2>; - reg = <0x48004a14 0x4>; - ti,bit-shift = <3>; - }; - - cam_ick: cam_ick@48004f10 { - #clock-cells = <0>; - compatible = "ti,omap3-no-wait-interface-clock"; - clocks = <&l4_ick>; - reg = <0x48004f10 0x4>; - ti,bit-shift = <0>; - }; - - ssi_ick_3430es2: ssi_ick_3430es2@48004a10 { - #clock-cells = <0>; - compatible = "ti,omap3-ssi-interface-clock"; - clocks = <&ssi_l4_ick>; - reg = <0x48004a10 0x4>; - ti,bit-shift = <0>; - }; 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 + 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 + "#clock-cells": + const: 0 + + clocks: + maxItems: 1 + + clock-output-names: + maxItems: 1 + + reg: + description: + if not specified, value from parent is used + 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' + +additionalProperties: false + +examples: + - | + bus { + #address-cells = <1>; + #size-cells = <1>; + + aes1_ick: aes1-ick@48004a14 { + #clock-cells = <0>; + compatible = "ti,omap3-interface-clock"; + clocks = <&security_l4_ick2>; + reg = <0x48004a14 0x4>; + ti,bit-shift = <3>; + }; + + cam_ick: cam-ick@48004f10 { + #clock-cells = <0>; + compatible = "ti,omap3-no-wait-interface-clock"; + clocks = <&l4_ick>; + reg = <0x48004f10 0x4>; + ti,bit-shift = <0>; + }; + + ssi_ick_3430es2: ssi-ick-3430es2@48004a10 { + #clock-cells = <0>; + compatible = "ti,omap3-ssi-interface-clock"; + clocks = <&ssi_l4_ick>; + reg = <0x48004a10 0x4>; + ti,bit-shift = <0>; + }; + };
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