| Message ID | 20241205111939.1796244-16-dario.binacchi@amarulasolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support spread spectrum clocking for i.MX8N PLLs | expand |
On Thu, Dec 05, 2024 at 12:17:50PM +0100, Dario Binacchi wrote: > The patch adds the DT bindings for enabling and tuning spread spectrum > clocking generation. > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > (no changes since v4) > > Changes in v4: > - Drop "fsl,ssc-clocks" property. The other added properties now refer > to the clock list. > - Updated minItems and maxItems of > - clocks > - clock-names > - fsl,ssc-modfreq-hz > - fsl,ssc-modrate-percent > - fsl,ssc-modmethod > - Updated the dts examples > > Changes in v3: > - Added in v3 > - The dt-bindings have been moved from fsl,imx8m-anatop.yaml to > imx8m-clock.yaml. The anatop device (fsl,imx8m-anatop.yaml) is > indeed more or less a syscon, so it represents a memory area > accessible by ccm (imx8m-clock.yaml) to setup the PLLs. > > Changes in v2: > - Add "allOf:" and place it after "required:" block, like in the > example schema. > - Move the properties definition to the top-level. > - Drop unit types as requested by the "make dt_binding_check" command. > > .../bindings/clock/imx8m-clock.yaml | 77 +++++++++++++++++-- > 1 file changed, 71 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > index c643d4a81478..83036f6d2274 100644 > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > @@ -29,12 +29,12 @@ properties: > maxItems: 2 > > clocks: > - minItems: 6 > - maxItems: 7 > + minItems: 7 > + maxItems: 10 ABI break without mentioning, without any explanation in the commit msg. > > clock-names: > - minItems: 6 > - maxItems: 7 > + minItems: 7 > + maxItems: 10 > > '#clock-cells': > const: 1 > @@ -43,6 +43,34 @@ properties: > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h > for the full list of i.MX8M clock IDs. > > + fsl,ssc-modfreq-hz: > + description: > + The values of modulation frequency (Hz unit) for each clock > + supporting spread spectrum. > + minItems: 7 > + maxItems: 10 Why all cloks receive now spread spectrum? I had impression - and all your previous versions were doing this - that you have only three or four clocks with SSC. Do existing clocks 1-6 support SSC? > + > + fsl,ssc-modrate-percent: > + description: > + The percentage values of modulation rate for each clock > + supporting spread spectrum. > + minItems: 7 > + maxItems: 10 > + > + fsl,ssc-modmethod: > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > + description: > + The modulation techniques for each clock supporting spread > + spectrum. > + minItems: 7 > + maxItems: 10 > + items: > + enum: > + - "" Drop "", not sure why do you need it. > + - down-spread > + - up-spread > + - center-spread > + > required: > - compatible > - reg > @@ -76,6 +104,10 @@ allOf: > - const: clk_ext2 > - const: clk_ext3 > - const: clk_ext4 > + fsl,ssc-modfreq-hz: false > + fsl,ssc-modrate-percent: false > + fsl,ssc-modmethod: false > + > else: > properties: > clocks: > @@ -86,6 +118,10 @@ allOf: > - description: ext2 clock input > - description: ext3 clock input > - description: ext4 clock input > + - description: audio1 PLL input > + - description: audio2 PLL input > + - description: dram PLL input > + - description: video PLL input Also ABI break.... Best regards, Krzysztof
On Fri, Dec 6, 2024 at 2:04 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Thu, Dec 05, 2024 at 12:17:50PM +0100, Dario Binacchi wrote: > > The patch adds the DT bindings for enabling and tuning spread spectrum > > clocking generation. > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > > > --- > > > > (no changes since v4) > > > > Changes in v4: > > - Drop "fsl,ssc-clocks" property. The other added properties now refer > > to the clock list. > > - Updated minItems and maxItems of > > - clocks > > - clock-names > > - fsl,ssc-modfreq-hz > > - fsl,ssc-modrate-percent > > - fsl,ssc-modmethod > > - Updated the dts examples > > > > Changes in v3: > > - Added in v3 > > - The dt-bindings have been moved from fsl,imx8m-anatop.yaml to > > imx8m-clock.yaml. The anatop device (fsl,imx8m-anatop.yaml) is > > indeed more or less a syscon, so it represents a memory area > > accessible by ccm (imx8m-clock.yaml) to setup the PLLs. > > > > Changes in v2: > > - Add "allOf:" and place it after "required:" block, like in the > > example schema. > > - Move the properties definition to the top-level. > > - Drop unit types as requested by the "make dt_binding_check" command. > > > > .../bindings/clock/imx8m-clock.yaml | 77 +++++++++++++++++-- > > 1 file changed, 71 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > index c643d4a81478..83036f6d2274 100644 > > --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml > > @@ -29,12 +29,12 @@ properties: > > maxItems: 2 > > > > clocks: > > - minItems: 6 > > - maxItems: 7 > > + minItems: 7 > > + maxItems: 10 > > ABI break without mentioning, without any explanation in the commit msg. > > > > > clock-names: > > - minItems: 6 > > - maxItems: 7 > > + minItems: 7 > > + maxItems: 10 > > > > '#clock-cells': > > const: 1 > > @@ -43,6 +43,34 @@ properties: > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h > > for the full list of i.MX8M clock IDs. > > > > + fsl,ssc-modfreq-hz: > > + description: > > + The values of modulation frequency (Hz unit) for each clock > > + supporting spread spectrum. > > + minItems: 7 > > + maxItems: 10 > > Why all cloks receive now spread spectrum? I had impression - and all > your previous versions were doing this - that you have only three or > four clocks with SSC. Exactly. Indeed, the first six values are not valid as SSC properties but are only used to reach the point where the first PLL with SSC (i.e., audio_pll1) can be indexed, which is in position 7 in the clocks list. This was the rationale I followed. And it is explicitly outlined in the example section. The "" for the fsl,ssc-method property is precisely aimed at specifying a "no SSC" method, which also fixes the warning: fsl,ssc-method:0: '' is not one of ['down-spread', 'up-spread', 'center-spread'] raised by make dt_binding_check DT_SCHEMA_FILES=imx8m-clock.yaml Or would it be acceptable to specify a list of SSC values that applies only to the last 4 PLLs in the clocks list? I feel like I might be missing something. Could you kindly suggest what to do or provide a DTS example to show me what you expect? Thanks and regards, Dario > > Do existing clocks 1-6 support SSC? > > > + > > + fsl,ssc-modrate-percent: > > + description: > > + The percentage values of modulation rate for each clock > > + supporting spread spectrum. > > + minItems: 7 > > + maxItems: 10 > > + > > + fsl,ssc-modmethod: > > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > > + description: > > + The modulation techniques for each clock supporting spread > > + spectrum. > > + minItems: 7 > > + maxItems: 10 > > + items: > > + enum: > > + - "" > > Drop "", not sure why do you need it. > > > + - down-spread > > + - up-spread > > + - center-spread > > + > > required: > > - compatible > > - reg > > @@ -76,6 +104,10 @@ allOf: > > - const: clk_ext2 > > - const: clk_ext3 > > - const: clk_ext4 > > + fsl,ssc-modfreq-hz: false > > + fsl,ssc-modrate-percent: false > > + fsl,ssc-modmethod: false > > + > > else: > > properties: > > clocks: > > @@ -86,6 +118,10 @@ allOf: > > - description: ext2 clock input > > - description: ext3 clock input > > - description: ext4 clock input > > + - description: audio1 PLL input > > + - description: audio2 PLL input > > + - description: dram PLL input > > + - description: video PLL input > > Also ABI break.... > > Best regards, > Krzysztof >
On 08/12/2024 17:47, Dario Binacchi wrote: >>> '#clock-cells': >>> const: 1 >>> @@ -43,6 +43,34 @@ properties: >>> ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h >>> for the full list of i.MX8M clock IDs. >>> >>> + fsl,ssc-modfreq-hz: >>> + description: >>> + The values of modulation frequency (Hz unit) for each clock >>> + supporting spread spectrum. >>> + minItems: 7 >>> + maxItems: 10 >> >> Why all cloks receive now spread spectrum? I had impression - and all >> your previous versions were doing this - that you have only three or >> four clocks with SSC. > > Exactly. Indeed, the first six values are not valid as SSC properties but are > only used to reach the point where the first PLL with SSC (i.e., audio_pll1) > can be indexed, which is in position 7 in the clocks list. > This was the rationale I followed. > And it is explicitly outlined in the example section. > The "" for the fsl,ssc-method property is precisely aimed at specifying a > "no SSC" method, which also fixes the warning: > > fsl,ssc-method:0: '' is not one of ['down-spread', 'up-spread', 'center-spread'] > > raised by > make dt_binding_check DT_SCHEMA_FILES=imx8m-clock.yaml > > Or would it be acceptable to specify a list of SSC values that applies only to > the last 4 PLLs in the clocks list? Lists are strictly ordered, so their items are known. You take some clocks, of which last clocks are "foo" and "bar". Then you have new property for configuring SSC for clocks - again list - "foo" and "bar". > > I feel like I might be missing something. > > Could you kindly suggest what to do or provide a DTS example to show me > what you expect? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml index c643d4a81478..83036f6d2274 100644 --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml @@ -29,12 +29,12 @@ properties: maxItems: 2 clocks: - minItems: 6 - maxItems: 7 + minItems: 7 + maxItems: 10 clock-names: - minItems: 6 - maxItems: 7 + minItems: 7 + maxItems: 10 '#clock-cells': const: 1 @@ -43,6 +43,34 @@ properties: ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h for the full list of i.MX8M clock IDs. + fsl,ssc-modfreq-hz: + description: + The values of modulation frequency (Hz unit) for each clock + supporting spread spectrum. + minItems: 7 + maxItems: 10 + + fsl,ssc-modrate-percent: + description: + The percentage values of modulation rate for each clock + supporting spread spectrum. + minItems: 7 + maxItems: 10 + + fsl,ssc-modmethod: + $ref: /schemas/types.yaml#/definitions/non-unique-string-array + description: + The modulation techniques for each clock supporting spread + spectrum. + minItems: 7 + maxItems: 10 + items: + enum: + - "" + - down-spread + - up-spread + - center-spread + required: - compatible - reg @@ -76,6 +104,10 @@ allOf: - const: clk_ext2 - const: clk_ext3 - const: clk_ext4 + fsl,ssc-modfreq-hz: false + fsl,ssc-modrate-percent: false + fsl,ssc-modmethod: false + else: properties: clocks: @@ -86,6 +118,10 @@ allOf: - description: ext2 clock input - description: ext3 clock input - description: ext4 clock input + - description: audio1 PLL input + - description: audio2 PLL input + - description: dram PLL input + - description: video PLL input clock-names: items: @@ -95,20 +131,49 @@ allOf: - const: clk_ext2 - const: clk_ext3 - const: clk_ext4 + - const: audio_pll1 + - const: audio_pll2 + - const: dram_pll + - const: video_pll additionalProperties: false examples: # Clock Control Module node: - | + #include <dt-bindings/clock/imx8mm-clock.h> + clock-controller@30380000 { compatible = "fsl,imx8mm-ccm"; reg = <0x30380000 0x10000>; #clock-cells = <1>; clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>, - <&clk_ext3>, <&clk_ext4>; + <&clk_ext3>, <&clk_ext4>, + <&anatop IMX8MM_ANATOP_AUDIO_PLL1>, + <&anatop IMX8MM_ANATOP_AUDIO_PLL1>, + <&anatop IMX8MM_ANATOP_DRAM_PLL>, + <&anatop IMX8MM_ANATOP_VIDEO_PLL>; clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2", - "clk_ext3", "clk_ext4"; + "clk_ext3", "clk_ext4", "audio_pll1", "audio_pll2", + "dram_pll", "video_pll"; + fsl,ssc-modfreq-hz = <0>, <0>, <0>, <0>, + <0>, <0>, + <6818>, + <0>, + <0>, + <2419>; + fsl,ssc-modrate-percent = <0>, <0>, <0>, <0>, + <0>, <0>, + <3>, + <0>, + <0>, + <7>; + fsl,ssc-modmethod = "", "", "", "", + "", "", + "down-spread", + "", + "", + "center-spread"; }; - |
The patch adds the DT bindings for enabling and tuning spread spectrum clocking generation. Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- (no changes since v4) Changes in v4: - Drop "fsl,ssc-clocks" property. The other added properties now refer to the clock list. - Updated minItems and maxItems of - clocks - clock-names - fsl,ssc-modfreq-hz - fsl,ssc-modrate-percent - fsl,ssc-modmethod - Updated the dts examples Changes in v3: - Added in v3 - The dt-bindings have been moved from fsl,imx8m-anatop.yaml to imx8m-clock.yaml. The anatop device (fsl,imx8m-anatop.yaml) is indeed more or less a syscon, so it represents a memory area accessible by ccm (imx8m-clock.yaml) to setup the PLLs. Changes in v2: - Add "allOf:" and place it after "required:" block, like in the example schema. - Move the properties definition to the top-level. - Drop unit types as requested by the "make dt_binding_check" command. .../bindings/clock/imx8m-clock.yaml | 77 +++++++++++++++++-- 1 file changed, 71 insertions(+), 6 deletions(-)