| Message ID | 20240721170840.15569-3-five231003@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ti: davinci, keystone: txt to yaml | expand |
On 21/07/2024 18:28, Kousik Sanagavarapu wrote: > diff --git a/Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml > new file mode 100644 > index 000000000000..1829c407147d > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml Use fallback as filename, so ti,keystone-wdt.yaml > @@ -0,0 +1,52 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/watchdog/ti,davinci-wdt.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TI DaVinci/Keystone Watchdog Timer Controller > + > +maintainers: > + - Kousik Sanagavarapu <five231003@gmail.com> > + > +description: | > + TI's Watchdog Timer Controller for DaVinci and Keystone Processors. > + > + Datasheets > + > + Davinci DM646x - https://www.ti.com/lit/ug/spruer5b/spruer5b.pdf > + Keystone - https://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf > + > +allOf: > + - $ref: watchdog.yaml# > + > +properties: > + compatible: > + enum: > + - ti,davinci-wdt > + - ti,keystone-wdt This does not match the original binding and commit msg did not explain why such change is necessary. This also does not match DTS. Best regards, Krzysztof
On Mon, Jul 22, 2024 at 10:15:03AM +0200, Krzysztof Kozlowski wrote: > On 21/07/2024 18:28, Kousik Sanagavarapu wrote: > > +properties: > > + compatible: > > + enum: > > + - ti,davinci-wdt > > + - ti,keystone-wdt > > This does not match the original binding and commit msg did not explain > why such change is necessary. I don't understand. Do you mean both the compatibles are always compulsory? Meaning compatible: items: - const: ti,davinci-wdt - const: ti,keystone-wdt It is enum because I intended it to align with the subsequent patch which changes DTS. > This also does not match DTS. Yes. I've asked about changing the DTS in the subsequent patch. Thanks for the review
On 22/07/2024 15:12, Kousik Sanagavarapu wrote: > On Mon, Jul 22, 2024 at 10:15:03AM +0200, Krzysztof Kozlowski wrote: >> On 21/07/2024 18:28, Kousik Sanagavarapu wrote: >>> +properties: >>> + compatible: >>> + enum: >>> + - ti,davinci-wdt >>> + - ti,keystone-wdt >> >> This does not match the original binding and commit msg did not explain >> why such change is necessary. > > I don't understand. Do you mean both the compatibles are always > compulsory? Meaning > > compatible: > items: > - const: ti,davinci-wdt > - const: ti,keystone-wdt Yes, this is what old binding said. > > It is enum because I intended it to align with the subsequent patch > which changes DTS. > >> This also does not match DTS. > > Yes. I've asked about changing the DTS in the subsequent patch. > Changing the DTS cannot be the reason to affect users and DTS... It's tautology. You change DTS because you intent to change DTS? Best regards, Krzysztof
On Mon, Jul 22, 2024 at 03:50:15PM +0200, Krzysztof Kozlowski wrote: > On 22/07/2024 15:12, Kousik Sanagavarapu wrote: > > On Mon, Jul 22, 2024 at 10:15:03AM +0200, Krzysztof Kozlowski wrote: > >> On 21/07/2024 18:28, Kousik Sanagavarapu wrote: > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - ti,davinci-wdt > >>> + - ti,keystone-wdt > >> > >> This does not match the original binding and commit msg did not explain > >> why such change is necessary. > > > > I don't understand. Do you mean both the compatibles are always > > compulsory? Meaning > > > > compatible: > > items: > > - const: ti,davinci-wdt > > - const: ti,keystone-wdt > > Yes, this is what old binding said. That was what I thought initially too, but the example in the old binding says otherwise and also the DTS from ti/davinci/da850.dtsi says wdt: watchdog@21000 { compatible = "ti,davinci-wdt"; reg = <0x21000 0x1000>; clocks = <&pll0_auxclk>; status = "disabled"; }; Or am I seeing it the wrong way? > > > > It is enum because I intended it to align with the subsequent patch > > which changes DTS. > > > >> This also does not match DTS. > > > > Yes. I've asked about changing the DTS in the subsequent patch. > > > > Changing the DTS cannot be the reason to affect users and DTS... It's > tautology. You change DTS because you intent to change DTS? Not exactly. I thought that the DTS was wrong when it said compatible = "ti,keystone-wdt", "ti,davinci-wdt"; while it should have been compatible = "ti,keystone-wdt"; I was not sure about this though and hence marked both the patches as RFC, in case I was interpretting them the wrong way. Thanks
On 22/07/2024 16:02, Kousik Sanagavarapu wrote: > On Mon, Jul 22, 2024 at 03:50:15PM +0200, Krzysztof Kozlowski wrote: >> On 22/07/2024 15:12, Kousik Sanagavarapu wrote: >>> On Mon, Jul 22, 2024 at 10:15:03AM +0200, Krzysztof Kozlowski wrote: >>>> On 21/07/2024 18:28, Kousik Sanagavarapu wrote: >>>>> +properties: >>>>> + compatible: >>>>> + enum: >>>>> + - ti,davinci-wdt >>>>> + - ti,keystone-wdt >>>> >>>> This does not match the original binding and commit msg did not explain >>>> why such change is necessary. >>> >>> I don't understand. Do you mean both the compatibles are always >>> compulsory? Meaning >>> >>> compatible: >>> items: >>> - const: ti,davinci-wdt >>> - const: ti,keystone-wdt >> >> Yes, this is what old binding said. > > That was what I thought initially too, but the example in the old > binding says otherwise and also the DTS from ti/davinci/da850.dtsi > says > > wdt: watchdog@21000 { > compatible = "ti,davinci-wdt"; > reg = <0x21000 0x1000>; > clocks = <&pll0_auxclk>; > status = "disabled"; > }; > > Or am I seeing it the wrong way? > >>> >>> It is enum because I intended it to align with the subsequent patch >>> which changes DTS. >>> >>>> This also does not match DTS. >>> >>> Yes. I've asked about changing the DTS in the subsequent patch. >>> >> >> Changing the DTS cannot be the reason to affect users and DTS... It's >> tautology. You change DTS because you intent to change DTS? > > Not exactly. I thought that the DTS was wrong when it said > > compatible = "ti,keystone-wdt", "ti,davinci-wdt"; > > while it should have been > > compatible = "ti,keystone-wdt"; > > I was not sure about this though and hence marked both the patches as > RFC, in case I was interpretting them the wrong way. Ah, right, the DTS says keystone+davinci while old binding suggested davinci+keystone. Considering there is no driver binding to keystone, I think the answer is obvious - intention was keystone+davinci. Anyway, commit msg should mention why you are doing something else than pure conversion. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt deleted file mode 100644 index aa10b8ec36e2..000000000000 --- a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt +++ /dev/null @@ -1,24 +0,0 @@ -Texas Instruments DaVinci/Keystone Watchdog Timer (WDT) Controller - -Required properties: -- compatible : Should be "ti,davinci-wdt", "ti,keystone-wdt" -- reg : Should contain WDT registers location and length - -Optional properties: -- timeout-sec : Contains the watchdog timeout in seconds -- clocks : the clock feeding the watchdog timer. - Needed if platform uses clocks. - See clock-bindings.txt - -Documentation: -Davinci DM646x - https://www.ti.com/lit/ug/spruer5b/spruer5b.pdf -Keystone - https://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf - -Examples: - -wdt: wdt@2320000 { - compatible = "ti,davinci-wdt"; - reg = <0x02320000 0x80>; - timeout-sec = <30>; - clocks = <&clkwdtimer0>; -}; diff --git a/Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml new file mode 100644 index 000000000000..1829c407147d --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml @@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/ti,davinci-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI DaVinci/Keystone Watchdog Timer Controller + +maintainers: + - Kousik Sanagavarapu <five231003@gmail.com> + +description: | + TI's Watchdog Timer Controller for DaVinci and Keystone Processors. + + Datasheets + + Davinci DM646x - https://www.ti.com/lit/ug/spruer5b/spruer5b.pdf + Keystone - https://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf + +allOf: + - $ref: watchdog.yaml# + +properties: + compatible: + enum: + - ti,davinci-wdt + - ti,keystone-wdt + + reg: + maxItems: 1 + + power-domains: + maxItems: 1 + + clocks: + maxItems: 1 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + watchdog@22f0080 { + compatible = "ti,davinci-wdt"; + reg = <0x022f0080 0x80>; + clocks = <&clkwdtimer0>; + }; + +...
Convert txt bindings of TI's DaVinci/Keystone Watchdog Timer Controller to dtschema to allow for validation. Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- This patch was submitted to the lists before by Nik https://lore.kernel.org/linux-devicetree/20231024195839.49607-1-n2h9z4@gmail.com/ Although it seems that the right way include the "power-domians" property was not decided upon (read through the thread). I grepped for instances of "power-domains" in ti related SoCs and other subsystems and it seems that there is always only 1 such "power-domains" phandle Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml The existing dts code also confirms this arch/arm/boot/dts/ti/keystone/keystone-k2g.dtsi Again, I guess it would be great if someone could point out if this is right - so RFC. Also, shouldn't "clocks" be "required"? - RFC. .../bindings/watchdog/davinci-wdt.txt | 24 --------- .../bindings/watchdog/ti,davinci-wdt.yaml | 52 +++++++++++++++++++ 2 files changed, 52 insertions(+), 24 deletions(-) delete mode 100644 Documentation/devicetree/bindings/watchdog/davinci-wdt.txt create mode 100644 Documentation/devicetree/bindings/watchdog/ti,davinci-wdt.yaml