| Message ID | 20210901053951.60952-2-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: sunxi-ng: Add a RTC CCU driver | expand |
On Wed, 01 Sep 2021 00:39:45 -0500, Samuel Holland wrote: > For these new SoCs, start requiring a complete list of input clocks. > > For H616, this means bus, hosc, and pll-32k. For R329, this means ahb, > bus, and hosc; and optionally ext-osc32k. > > I'm not sure how to best represent this in the binding... > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++-- > include/dt-bindings/clock/sun50i-rtc.h | 12 ++++ > 2 files changed, 61 insertions(+), 6 deletions(-) > create mode 100644 include/dt-bindings/clock/sun50i-rtc.h > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Traceback (most recent call last): File "/usr/local/bin/dt-doc-validate", line 67, in <module> ret = check_doc(f) File "/usr/local/bin/dt-doc-validate", line 33, in check_doc for error in sorted(dtschema.DTValidator.iter_schema_errors(testtree), key=lambda e: e.linecol): File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 723, in iter_schema_errors cls(meta_schema).annotate_error(error, meta_schema, error.schema_path) File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 706, in annotate_error schema = schema[p] KeyError: 'properties' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml: ignoring, error in schema: allOf: 5: if: properties warning: no schema found in file: ./Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.example.dt.yaml:0:0: /example-0/rtc@1f00000: failed to match any schema with compatible: ['allwinner,sun6i-a31-rtc'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1522863 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote: > For these new SoCs, start requiring a complete list of input clocks. > > For H616, this means bus, hosc, and pll-32k. For R329, this means ahb, > bus, and hosc; and optionally ext-osc32k. > > I'm not sure how to best represent this in the binding... > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++-- > include/dt-bindings/clock/sun50i-rtc.h | 12 ++++ > 2 files changed, 61 insertions(+), 6 deletions(-) > create mode 100644 include/dt-bindings/clock/sun50i-rtc.h > > diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml > index beeb90e55727..3e085db1294f 100644 > --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml > +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml > @@ -26,6 +26,8 @@ properties: > - const: allwinner,sun50i-a64-rtc > - const: allwinner,sun8i-h3-rtc > - const: allwinner,sun50i-h6-rtc > + - const: allwinner,sun50i-h616-rtc > + - const: allwinner,sun50i-r329-rtc Can you please make all the single entry cases a single 'enum'. > > reg: > maxItems: 1 > @@ -37,7 +39,24 @@ properties: > - description: RTC Alarm 1 > > clocks: > - maxItems: 1 > + minItems: 1 > + maxItems: 4 > + > + clock-names: > + minItems: 1 > + maxItems: 4 > + items: > + - anyOf: This says the first entry is any of these. What about the rest of them? > + - const: ahb > + description: AHB parent for SPI bus clock The description should go in 'clocks'. The order should be defined as well with the first clock being the one that existed previously. > + - const: bus > + description: AHB/APB bus clock for register access > + - const: ext-osc32k > + description: External 32768 Hz oscillator input > + - const: hosc > + description: 24 MHz oscillator input > + - const: pll-32k > + description: 32 kHz clock divided from a PLL > > clock-output-names: > minItems: 1 > @@ -85,6 +104,9 @@ allOf: > enum: > - allwinner,sun8i-h3-rtc > - allwinner,sun50i-h5-rtc > + - allwinner,sun50i-h6-rtc > + - allwinner,sun50i-h616-rtc > + - allwinner,sun50i-r329-rtc > > then: > properties: > @@ -96,13 +118,35 @@ allOf: > properties: > compatible: > contains: > - const: allwinner,sun50i-h6-rtc > + enum: > + - allwinner,sun50i-h616-rtc > + - allwinner,sun50i-r329-rtc > > then: > + clocks: > + minItems: 3 # bus, hosc, and (pll-32k [H616] or ahb [R329]) > + > + clock-names: > + minItems: 3 > + > + required: > + - clock-names > + > + else: > + required: > + - clock-output-names > + > + - if: > + properties: clock-names > + > + then: > + required: > + - clocks # hosc is required > + > + else: > properties: > - clock-output-names: > - minItems: 3 > - maxItems: 3 > + clocks: > + maxItems: 1 # only ext-osc32k is allowed > > - if: > properties: > @@ -127,7 +171,6 @@ required: > - compatible > - reg > - interrupts > - - clock-output-names > > additionalProperties: false > > diff --git a/include/dt-bindings/clock/sun50i-rtc.h b/include/dt-bindings/clock/sun50i-rtc.h > new file mode 100644 > index 000000000000..d45e3ff4e105 > --- /dev/null > +++ b/include/dt-bindings/clock/sun50i-rtc.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ Dual license please. > + > +#ifndef _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ > +#define _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ > + > +#define CLK_OSC32K 0 > +#define CLK_OSC32K_FANOUT 1 > +#define CLK_IOSC 2 > + > +#define CLK_RTC_SPI 8 > + > +#endif /* _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ */ > -- > 2.31.1 > >
On 9/2/21 10:27 AM, Rob Herring wrote: > On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote: >> For these new SoCs, start requiring a complete list of input clocks. >> >> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb, >> bus, and hosc; and optionally ext-osc32k. >> >> I'm not sure how to best represent this in the binding... >> >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++-- >> include/dt-bindings/clock/sun50i-rtc.h | 12 ++++ >> 2 files changed, 61 insertions(+), 6 deletions(-) >> create mode 100644 include/dt-bindings/clock/sun50i-rtc.h >> >> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml >> index beeb90e55727..3e085db1294f 100644 >> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml >> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml >> @@ -26,6 +26,8 @@ properties: >> - const: allwinner,sun50i-a64-rtc >> - const: allwinner,sun8i-h3-rtc >> - const: allwinner,sun50i-h6-rtc >> + - const: allwinner,sun50i-h616-rtc >> + - const: allwinner,sun50i-r329-rtc > > Can you please make all the single entry cases a single 'enum'. > >> >> reg: >> maxItems: 1 >> @@ -37,7 +39,24 @@ properties: >> - description: RTC Alarm 1 >> >> clocks: >> - maxItems: 1 >> + minItems: 1 >> + maxItems: 4 >> + >> + clock-names: >> + minItems: 1 >> + maxItems: 4 >> + items: >> + - anyOf: > > This says the first entry is any of these. What about the rest of them? Oh, right. The list below is the list of all possible clocks. >> + - const: ahb >> + description: AHB parent for SPI bus clock > > The description should go in 'clocks'. Will do for v2. > The order should be defined as well with the first clock being the > one that existed previously. The only way I know how to further refine the list is with minItems/maxItems. My problem is that 1) some clocks are only valid for certain SoCs, and 2) some clocks are optional, depending on how the board is wired. So there is no single order where the "valid" combinations are prefixes of the "possible" combinations of clocks. Or in other words, how can I say "clocks #1 and #2 from this list are required, and #4 is optional, but #3 is not allowed"? Some concrete examples, with the always-required clocks moved to the beginning: H6: - bus: required - hosc: required - ahb: not allowed - ext-osc32k: optional - pll-32k: not allowed H616: - bus: required - hosc: required - ahb: not allowed - ext-osc32k: not allowed - pll-32k: required R329: - bus: required - hosc: required - ahb: required - ext-osc32k: optional - pll-32k: not allowed Should I just move the entire clocks/clock-items properties to if/then blocks based on the compatible? >> + - const: bus >> + description: AHB/APB bus clock for register access >> + - const: ext-osc32k >> + description: External 32768 Hz oscillator input >> + - const: hosc >> + description: 24 MHz oscillator input >> + - const: pll-32k >> + description: 32 kHz clock divided from a PLL >> >> clock-output-names: >> minItems: 1 >> @@ -85,6 +104,9 @@ allOf: >> enum: >> - allwinner,sun8i-h3-rtc >> - allwinner,sun50i-h5-rtc >> + - allwinner,sun50i-h6-rtc >> + - allwinner,sun50i-h616-rtc >> + - allwinner,sun50i-r329-rtc >> >> then: >> properties: >> @@ -96,13 +118,35 @@ allOf: >> properties: >> compatible: >> contains: >> - const: allwinner,sun50i-h6-rtc >> + enum: >> + - allwinner,sun50i-h616-rtc >> + - allwinner,sun50i-r329-rtc >> >> then: >> + clocks: >> + minItems: 3 # bus, hosc, and (pll-32k [H616] or ahb [R329]) >> + >> + clock-names: >> + minItems: 3 >> + >> + required: >> + - clock-names >> + >> + else: >> + required: >> + - clock-output-names >> + >> + - if: >> + properties: clock-names >> + >> + then: >> + required: >> + - clocks # hosc is required >> + >> + else: >> properties: >> - clock-output-names: >> - minItems: 3 >> - maxItems: 3 >> + clocks: >> + maxItems: 1 # only ext-osc32k is allowed >> >> - if: >> properties: >> @@ -127,7 +171,6 @@ required: >> - compatible >> - reg >> - interrupts >> - - clock-output-names >> >> additionalProperties: false >> >> diff --git a/include/dt-bindings/clock/sun50i-rtc.h b/include/dt-bindings/clock/sun50i-rtc.h >> new file mode 100644 >> index 000000000000..d45e3ff4e105 >> --- /dev/null >> +++ b/include/dt-bindings/clock/sun50i-rtc.h >> @@ -0,0 +1,12 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ > > Dual license please. Will do for v2. Regards, Samuel >> + >> +#ifndef _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ >> +#define _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ >> + >> +#define CLK_OSC32K 0 >> +#define CLK_OSC32K_FANOUT 1 >> +#define CLK_IOSC 2 >> + >> +#define CLK_RTC_SPI 8 >> + >> +#endif /* _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ */ >> -- >> 2.31.1 >> >>
On Fri, Sep 3, 2021 at 10:36 AM Samuel Holland <samuel@sholland.org> wrote: > > On 9/2/21 10:27 AM, Rob Herring wrote: > > On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote: > >> For these new SoCs, start requiring a complete list of input clocks. > >> > >> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb, > >> bus, and hosc; and optionally ext-osc32k. > >> > >> I'm not sure how to best represent this in the binding... > >> > >> Signed-off-by: Samuel Holland <samuel@sholland.org> > >> --- > >> .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++-- > >> include/dt-bindings/clock/sun50i-rtc.h | 12 ++++ > >> 2 files changed, 61 insertions(+), 6 deletions(-) > >> create mode 100644 include/dt-bindings/clock/sun50i-rtc.h > >> > >> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml > >> index beeb90e55727..3e085db1294f 100644 > >> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml > >> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml > >> @@ -26,6 +26,8 @@ properties: > >> - const: allwinner,sun50i-a64-rtc > >> - const: allwinner,sun8i-h3-rtc > >> - const: allwinner,sun50i-h6-rtc > >> + - const: allwinner,sun50i-h616-rtc > >> + - const: allwinner,sun50i-r329-rtc > > > > Can you please make all the single entry cases a single 'enum'. > > > >> > >> reg: > >> maxItems: 1 > >> @@ -37,7 +39,24 @@ properties: > >> - description: RTC Alarm 1 > >> > >> clocks: > >> - maxItems: 1 > >> + minItems: 1 > >> + maxItems: 4 > >> + > >> + clock-names: > >> + minItems: 1 > >> + maxItems: 4 > >> + items: > >> + - anyOf: > > > > This says the first entry is any of these. What about the rest of them? > > Oh, right. The list below is the list of all possible clocks. > > >> + - const: ahb > >> + description: AHB parent for SPI bus clock > > > > The description should go in 'clocks'. > > Will do for v2. > > > The order should be defined as well with the first clock being the > > one that existed previously. > > The only way I know how to further refine the list is with > minItems/maxItems. My problem is that 1) some clocks are only valid for > certain SoCs, and 2) some clocks are optional, depending on how the > board is wired. So there is no single order where the "valid" > combinations are prefixes of the "possible" combinations of clocks. > > Or in other words, how can I say "clocks #1 and #2 from this list are > required, and #4 is optional, but #3 is not allowed"? This says you have up to 4 clocks, but only defines the 1st 2: maxItems: 4 items: - description: 1st clock - description: 2nd clock But I think you will be better off with just defining the range (minItems/maxItems) at the top level and then use if/then schemas. > > Some concrete examples, with the always-required clocks moved to the > beginning: > > H6: > - bus: required > - hosc: required > - ahb: not allowed > - ext-osc32k: optional > - pll-32k: not allowed Is this really 2 different 32k clock inputs to the h/w block? Doesn't seem like it given both are never valid. > > H616: > - bus: required > - hosc: required > - ahb: not allowed > - ext-osc32k: not allowed > - pll-32k: required > > R329: > - bus: required > - hosc: required > - ahb: required > - ext-osc32k: optional > - pll-32k: not allowed > > Should I just move the entire clocks/clock-items properties to if/then > blocks based on the compatible? Probably so. Rob
On 9/7/21 9:44 AM, Rob Herring wrote: > On Fri, Sep 3, 2021 at 10:36 AM Samuel Holland <samuel@sholland.org> wrote: >> >> On 9/2/21 10:27 AM, Rob Herring wrote: >>> On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote: >>>> For these new SoCs, start requiring a complete list of input clocks. >>>> >>>> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb, >>>> bus, and hosc; and optionally ext-osc32k. >>>> >>>> I'm not sure how to best represent this in the binding... >>>> >>>> Signed-off-by: Samuel Holland <samuel@sholland.org> >>>> --- >>>> .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++-- >>>> include/dt-bindings/clock/sun50i-rtc.h | 12 ++++ >>>> 2 files changed, 61 insertions(+), 6 deletions(-) >>>> create mode 100644 include/dt-bindings/clock/sun50i-rtc.h >>>> >>>> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml >>>> index beeb90e55727..3e085db1294f 100644 >>>> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml >>>> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml >>>> @@ -26,6 +26,8 @@ properties: >>>> - const: allwinner,sun50i-a64-rtc >>>> - const: allwinner,sun8i-h3-rtc >>>> - const: allwinner,sun50i-h6-rtc >>>> + - const: allwinner,sun50i-h616-rtc >>>> + - const: allwinner,sun50i-r329-rtc >>> >>> Can you please make all the single entry cases a single 'enum'. >>> >>>> >>>> reg: >>>> maxItems: 1 >>>> @@ -37,7 +39,24 @@ properties: >>>> - description: RTC Alarm 1 >>>> >>>> clocks: >>>> - maxItems: 1 >>>> + minItems: 1 >>>> + maxItems: 4 >>>> + >>>> + clock-names: >>>> + minItems: 1 >>>> + maxItems: 4 >>>> + items: >>>> + - anyOf: >>> >>> This says the first entry is any of these. What about the rest of them? >> >> Oh, right. The list below is the list of all possible clocks. >> >>>> + - const: ahb >>>> + description: AHB parent for SPI bus clock >>> >>> The description should go in 'clocks'. >> >> Will do for v2. >> >>> The order should be defined as well with the first clock being the >>> one that existed previously. >> >> The only way I know how to further refine the list is with >> minItems/maxItems. My problem is that 1) some clocks are only valid for >> certain SoCs, and 2) some clocks are optional, depending on how the >> board is wired. So there is no single order where the "valid" >> combinations are prefixes of the "possible" combinations of clocks. >> >> Or in other words, how can I say "clocks #1 and #2 from this list are >> required, and #4 is optional, but #3 is not allowed"? > > This says you have up to 4 clocks, but only defines the 1st 2: > > maxItems: 4 > items: > - description: 1st clock > - description: 2nd clock > > But I think you will be better off with just defining the range > (minItems/maxItems) at the top level and then use if/then schemas. Ah, thanks for the suggestions. >> >> Some concrete examples, with the always-required clocks moved to the >> beginning: >> >> H6: >> - bus: required >> - hosc: required >> - ahb: not allowed >> - ext-osc32k: optional >> - pll-32k: not allowed > > Is this really 2 different 32k clock inputs to the h/w block? Doesn't > seem like it given both are never valid. Yes, there are two separate 32k inputs. Both are valid at the same time on some SoCs like T5 (patch 7), but not on any of those I listed here. Regards, Samuel >> >> H616: >> - bus: required >> - hosc: required >> - ahb: not allowed >> - ext-osc32k: not allowed >> - pll-32k: required >> >> R329: >> - bus: required >> - hosc: required >> - ahb: required >> - ext-osc32k: optional >> - pll-32k: not allowed >> >> Should I just move the entire clocks/clock-items properties to if/then >> blocks based on the compatible? > > Probably so. > > Rob >
diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml index beeb90e55727..3e085db1294f 100644 --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml @@ -26,6 +26,8 @@ properties: - const: allwinner,sun50i-a64-rtc - const: allwinner,sun8i-h3-rtc - const: allwinner,sun50i-h6-rtc + - const: allwinner,sun50i-h616-rtc + - const: allwinner,sun50i-r329-rtc reg: maxItems: 1 @@ -37,7 +39,24 @@ properties: - description: RTC Alarm 1 clocks: - maxItems: 1 + minItems: 1 + maxItems: 4 + + clock-names: + minItems: 1 + maxItems: 4 + items: + - anyOf: + - const: ahb + description: AHB parent for SPI bus clock + - const: bus + description: AHB/APB bus clock for register access + - const: ext-osc32k + description: External 32768 Hz oscillator input + - const: hosc + description: 24 MHz oscillator input + - const: pll-32k + description: 32 kHz clock divided from a PLL clock-output-names: minItems: 1 @@ -85,6 +104,9 @@ allOf: enum: - allwinner,sun8i-h3-rtc - allwinner,sun50i-h5-rtc + - allwinner,sun50i-h6-rtc + - allwinner,sun50i-h616-rtc + - allwinner,sun50i-r329-rtc then: properties: @@ -96,13 +118,35 @@ allOf: properties: compatible: contains: - const: allwinner,sun50i-h6-rtc + enum: + - allwinner,sun50i-h616-rtc + - allwinner,sun50i-r329-rtc then: + clocks: + minItems: 3 # bus, hosc, and (pll-32k [H616] or ahb [R329]) + + clock-names: + minItems: 3 + + required: + - clock-names + + else: + required: + - clock-output-names + + - if: + properties: clock-names + + then: + required: + - clocks # hosc is required + + else: properties: - clock-output-names: - minItems: 3 - maxItems: 3 + clocks: + maxItems: 1 # only ext-osc32k is allowed - if: properties: @@ -127,7 +171,6 @@ required: - compatible - reg - interrupts - - clock-output-names additionalProperties: false diff --git a/include/dt-bindings/clock/sun50i-rtc.h b/include/dt-bindings/clock/sun50i-rtc.h new file mode 100644 index 000000000000..d45e3ff4e105 --- /dev/null +++ b/include/dt-bindings/clock/sun50i-rtc.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ +#define _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ + +#define CLK_OSC32K 0 +#define CLK_OSC32K_FANOUT 1 +#define CLK_IOSC 2 + +#define CLK_RTC_SPI 8 + +#endif /* _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ */
For these new SoCs, start requiring a complete list of input clocks. For H616, this means bus, hosc, and pll-32k. For R329, this means ahb, bus, and hosc; and optionally ext-osc32k. I'm not sure how to best represent this in the binding... Signed-off-by: Samuel Holland <samuel@sholland.org> --- .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++-- include/dt-bindings/clock/sun50i-rtc.h | 12 ++++ 2 files changed, 61 insertions(+), 6 deletions(-) create mode 100644 include/dt-bindings/clock/sun50i-rtc.h