| Message ID | 20211214120213.15649-2-povik@protonmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support for Apple SoCs' NCO blocks | expand |
On Tue, 14 Dec 2021 12:02:48 +0000, Martin Povišer wrote: > The NCO block found on Apple SoCs is a programmable clock generator > performing fractional division of a high frequency input clock. > > Signed-off-by: Martin Povišer <povik@protonmail.com> > --- > .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ > 1 file changed, 70 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml > 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: ./Documentation/devicetree/bindings/clock/apple,nco.yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1567695 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 Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote: > The NCO block found on Apple SoCs is a programmable clock generator > performing fractional division of a high frequency input clock. > > Signed-off-by: Martin Povišer <povik@protonmail.com> > --- > .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ > 1 file changed, 70 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml > > diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml > new file mode 100644 > index 000000000000..5029824ab179 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml > @@ -0,0 +1,70 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/apple,nco.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple SoCs' NCO block > + > +maintainers: > + - Martin Povišer <povik@protonmail.com> > + > +description: | > + The NCO (Numerically Controlled Oscillator) block found on Apple SoCs > + such as the t8103 (M1) is a programmable clock generator performing > + fractional division of a high frequency input clock. > + > + It carries a number of independent channels and is typically used for > + generation of audio bitclocks. > + > +properties: > + compatible: > + items: > + - enum: > + - apple,t6000-nco > + - apple,t8103-nco > + - const: apple,nco > + > + apple,nchannels: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + The number of output channels the NCO block has been > + synthesized for. I'd assume there is some max number? Do you really need to know this? If this is just to validate the clock cell values are less than this, then just drop that and the property. It's not the kernel's job to validate the DT. > + > + clocks: > + description: > + Specifies the reference clock from which the output clocks > + are derived through fractional division. > + maxItems: 1 > + > + '#clock-cells': > + const: 1 > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - apple,nchannels > + - clocks > + - '#clock-cells' > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + nco_clkref: clock-ref { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <900000000>; > + clock-output-names = "nco-ref"; > + }; > + > + nco: clock-generator@23b044000 { clock-controller@... > + compatible = "apple,t8103-nco", "apple,nco"; > + reg = <0x3b044000 0x14000>; You really have 0x14000 worth of registers here because all of that will be mapped into virtual memory? Doesn't matter so much on 64-bit, but it did for 32-bit. > + #clock-cells = <1>; > + clocks = <&nco_clkref>; > + apple,nchannels = <5>; > + }; > -- > 2.33.0 > > >
Hi Rob, > On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote: > > On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote: >> The NCO block found on Apple SoCs is a programmable clock generator >> performing fractional division of a high frequency input clock. >> >> Signed-off-by: Martin Povišer <povik@protonmail.com> >> --- >> .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ >> 1 file changed, 70 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml >> >> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml >> new file mode 100644 >> index 000000000000..5029824ab179 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml >> @@ -0,0 +1,70 @@ >> + >> + apple,nchannels: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + The number of output channels the NCO block has been >> + synthesized for. > > I'd assume there is some max number? There might be some limit to the underlying IP but we wouldn’t know. What we know about the hardware comes from blackbox reversing, and that doesn't suggest a particular limit to the number of channels we might see on the SoC block in future. > Do you really need to know this? If this is just to validate the clock > cell values are less than this, then just drop that and the property. > It's not the kernel's job to validate the DT. Well strictly speaking the driver could do clock registration on-demand at the cost of additional book-keeping, in which case we could drop the property, but I would prefer we don’t do that. Rather than providing validation the property simplifies drivers. Another option is calculating the no. of channels from size of the reg range, but I assume that’s worse than having the nchannels property. >> + >> + nco: clock-generator@23b044000 { > > clock-controller@... Okay, will change. > >> + compatible = "apple,t8103-nco", "apple,nco"; >> + reg = <0x3b044000 0x14000>; > > You really have 0x14000 worth of registers here because all of that > will be mapped into virtual memory? Doesn't matter so much on 64-bit, > but it did for 32-bit. There is about 5 registers per channel with 0x4000 stride between them, blame Apple (or Samsung? I don’t know...). -- Martin
On Tue, Dec 14, 2021 at 2:08 PM Martin Povišer <povik@protonmail.com> wrote: > > Hi Rob, > > > On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote: > > > > On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote: > >> The NCO block found on Apple SoCs is a programmable clock generator > >> performing fractional division of a high frequency input clock. > >> > >> Signed-off-by: Martin Povišer <povik@protonmail.com> > >> --- > >> .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ > >> 1 file changed, 70 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml > >> new file mode 100644 > >> index 000000000000..5029824ab179 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml > >> @@ -0,0 +1,70 @@ > > >> + > >> + apple,nchannels: > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + description: > >> + The number of output channels the NCO block has been > >> + synthesized for. > > > > I'd assume there is some max number? > > There might be some limit to the underlying IP but we wouldn’t know. > What we know about the hardware comes from blackbox reversing, and that > doesn't suggest a particular limit to the number of channels we might > see on the SoC block in future. All the more reason to not put the size in the DT, but imply from the compatible. Unless it varies by instance... Though I guess you would need DT updates anyways to use the new clock. > > Do you really need to know this? If this is just to validate the clock > > cell values are less than this, then just drop that and the property. > > It's not the kernel's job to validate the DT. > > Well strictly speaking the driver could do clock registration on-demand > at the cost of additional book-keeping, in which case we could drop > the property, but I would prefer we don’t do that. Rather than providing > validation the property simplifies drivers. > > Another option is calculating the no. of channels from size of the reg > range, but I assume that’s worse than having the nchannels property. > > >> + > >> + nco: clock-generator@23b044000 { > > > > clock-controller@... > > Okay, will change. > > > > >> + compatible = "apple,t8103-nco", "apple,nco"; > >> + reg = <0x3b044000 0x14000>; > > > > You really have 0x14000 worth of registers here because all of that > > will be mapped into virtual memory? Doesn't matter so much on 64-bit, > > but it did for 32-bit. > > There is about 5 registers per channel with 0x4000 stride between them, > blame Apple (or Samsung? I don’t know...). I would think you could walk the 0x4000 until you hit registers that behave differently. The register size / 0x4000 gives you the number of channels, too. Another question, how do you know this is 1 block with N channels vs. N blocks just happening to be next to each other in the memory map? Rob
> On 15. 12. 2021, at 0:53, Rob Herring <robh@kernel.org> wrote: > > On Tue, Dec 14, 2021 at 2:08 PM Martin Povišer <povik@protonmail.com> wrote: >> >> Hi Rob, >> >>> On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote: >>> >>> On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote: >>>> The NCO block found on Apple SoCs is a programmable clock generator >>>> performing fractional division of a high frequency input clock. >>>> >>>> Signed-off-by: Martin Povišer <povik@protonmail.com> >>>> --- >>>> .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ >>>> 1 file changed, 70 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml >>>> new file mode 100644 >>>> index 000000000000..5029824ab179 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml >>>> @@ -0,0 +1,70 @@ >> >>>> + >>>> + apple,nchannels: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: >>>> + The number of output channels the NCO block has been >>>> + synthesized for. >>> >>> I'd assume there is some max number? >> >> There might be some limit to the underlying IP but we wouldn’t know. >> What we know about the hardware comes from blackbox reversing, and that >> doesn't suggest a particular limit to the number of channels we might >> see on the SoC block in future. > > All the more reason to not put the size in the DT, but imply from the > compatible. Unless it varies by instance... > > Though I guess you would need DT updates anyways to use the new clock. > >>> Do you really need to know this? If this is just to validate the clock >>> cell values are less than this, then just drop that and the property. >>> It's not the kernel's job to validate the DT. >> >> Well strictly speaking the driver could do clock registration on-demand >> at the cost of additional book-keeping, in which case we could drop >> the property, but I would prefer we don’t do that. Rather than providing >> validation the property simplifies drivers. >> >> Another option is calculating the no. of channels from size of the reg >> range, but I assume that’s worse than having the nchannels property. >> >>>> + >>>> + nco: clock-generator@23b044000 { >>> >>> clock-controller@... >> >> Okay, will change. >> >>> >>>> + compatible = "apple,t8103-nco", "apple,nco"; >>>> + reg = <0x3b044000 0x14000>; >>> >>> You really have 0x14000 worth of registers here because all of that >>> will be mapped into virtual memory? Doesn't matter so much on 64-bit, >>> but it did for 32-bit. >> >> There is about 5 registers per channel with 0x4000 stride between them, >> blame Apple (or Samsung? I don’t know...). > > I would think you could walk the 0x4000 until you hit registers that > behave differently. > > The register size / 0x4000 gives you the number of channels, too. Right now that’s what I am inclined to use in v2. > Another question, how do you know this is 1 block with N channels vs. > N blocks just happening to be next to each other in the memory map? We don’t. We only see Apple describe it as such in their devicetree, and so far for all practical purposes it could be one block. I guess if we derive the number of channels from register size, there’s the fallback of breaking up the nodes per channel in future. Martin
On Wed, Dec 15, 2021, at 09:28, Martin Povišer wrote: >> On 15. 12. 2021, at 0:53, Rob Herring <robh@kernel.org> wrote: >> >> On Tue, Dec 14, 2021 at 2:08 PM Martin Povišer <povik@protonmail.com> wrote: >>> >>> Hi Rob, >>> >>>> On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote: >>>> >>>> On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote: >>>>> The NCO block found on Apple SoCs is a programmable clock generator >>>>> performing fractional division of a high frequency input clock. >>>>> >>>>> Signed-off-by: Martin Povišer <povik@protonmail.com> >>>>> --- >>>>> .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ >>>>> 1 file changed, 70 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml >>>>> new file mode 100644 >>>>> index 000000000000..5029824ab179 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml >>>>> @@ -0,0 +1,70 @@ >>> >>>>> + >>>>> + apple,nchannels: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: >>>>> + The number of output channels the NCO block has been >>>>> + synthesized for. >>>> >>>> I'd assume there is some max number? >>> >>> There might be some limit to the underlying IP but we wouldn’t know. >>> What we know about the hardware comes from blackbox reversing, and that >>> doesn't suggest a particular limit to the number of channels we might >>> see on the SoC block in future. >> >> All the more reason to not put the size in the DT, but imply from the >> compatible. Unless it varies by instance... >> >> Though I guess you would need DT updates anyways to use the new clock. >> >>>> Do you really need to know this? If this is just to validate the clock >>>> cell values are less than this, then just drop that and the property. >>>> It's not the kernel's job to validate the DT. >>> >>> Well strictly speaking the driver could do clock registration on-demand >>> at the cost of additional book-keeping, in which case we could drop >>> the property, but I would prefer we don’t do that. Rather than providing >>> validation the property simplifies drivers. >>> >>> Another option is calculating the no. of channels from size of the reg >>> range, but I assume that’s worse than having the nchannels property. >>> >>>>> + >>>>> + nco: clock-generator@23b044000 { >>>> >>>> clock-controller@... >>> >>> Okay, will change. >>> >>>> >>>>> + compatible = "apple,t8103-nco", "apple,nco"; >>>>> + reg = <0x3b044000 0x14000>; >>>> >>>> You really have 0x14000 worth of registers here because all of that >>>> will be mapped into virtual memory? Doesn't matter so much on 64-bit, >>>> but it did for 32-bit. >>> >>> There is about 5 registers per channel with 0x4000 stride between them, >>> blame Apple (or Samsung? I don’t know...). >> >> I would think you could walk the 0x4000 until you hit registers that >> behave differently. >> >> The register size / 0x4000 gives you the number of channels, too. > > Right now that’s what I am inclined to use in v2. > >> Another question, how do you know this is 1 block with N channels vs. >> N blocks just happening to be next to each other in the memory map? > > We don’t. We only see Apple describe it as such in their devicetree, and > so far for all practical purposes it could be one block. Fwiw, the Apple device tree cannot be trusted in general. It also pretends that two IOMMUs that need to programmed identically are a single dive, sometimes includes MMIO ranges that are much too large and also contains at least a single "virtual" device that only exists for what I assume to be a workaround for some XNU quirk(s). (the GPU IOMMU has a separate node with no MMIO or anything which only attaches a small shim driver that then calls back into the main GPU driver. That device is also only used from within that GPU driver.). Are there any dependencies between these individual channels? Is there some common initialization required for all of them? From a quick glance and my uninformed opinion it looks like these are separate to me. They only all need this LSFR table which could still be shared. Sven
> On 15. 12. 2021, at 9:43, Sven Peter <sven@svenpeter.dev> wrote: > > Are there any dependencies between these individual channels? > Is there some common initialization required for all of them? > > From a quick glance and my uninformed opinion it looks like these are > separate to me. They only all need this LSFR table which could still be > shared. That’s right. > > Sven Martin
diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml new file mode 100644 index 000000000000..5029824ab179 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml @@ -0,0 +1,70 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/apple,nco.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple SoCs' NCO block + +maintainers: + - Martin Povišer <povik@protonmail.com> + +description: | + The NCO (Numerically Controlled Oscillator) block found on Apple SoCs + such as the t8103 (M1) is a programmable clock generator performing + fractional division of a high frequency input clock. + + It carries a number of independent channels and is typically used for + generation of audio bitclocks. + +properties: + compatible: + items: + - enum: + - apple,t6000-nco + - apple,t8103-nco + - const: apple,nco + + apple,nchannels: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + The number of output channels the NCO block has been + synthesized for. + + clocks: + description: + Specifies the reference clock from which the output clocks + are derived through fractional division. + maxItems: 1 + + '#clock-cells': + const: 1 + + reg: + maxItems: 1 + +required: + - compatible + - apple,nchannels + - clocks + - '#clock-cells' + - reg + +additionalProperties: false + +examples: + - | + nco_clkref: clock-ref { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <900000000>; + clock-output-names = "nco-ref"; + }; + + nco: clock-generator@23b044000 { + compatible = "apple,t8103-nco", "apple,nco"; + reg = <0x3b044000 0x14000>; + #clock-cells = <1>; + clocks = <&nco_clkref>; + apple,nchannels = <5>; + };
The NCO block found on Apple SoCs is a programmable clock generator performing fractional division of a high frequency input clock. Signed-off-by: Martin Povišer <povik@protonmail.com> --- .../devicetree/bindings/clock/apple,nco.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml -- 2.33.0