diff mbox series

[1/2] dt-bindings: sound: Add Marvell MMP Audio Clock Controller binding

Message ID 20200511195534.1207927-2-lkundrak@v3.sk
State Not Applicable, archived
Headers show
Series MMP2 Audio clock controller driver | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Lubomir Rintel May 11, 2020, 7:55 p.m. UTC 
This describes the bindings for a controller that generates master and bit
clocks for the I2S interface.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 .../clock/marvell,mmp2-audio-clock.yaml       | 73 +++++++++++++++++++
 .../dt-bindings/clock/marvell,mmp2-audio.h    |  8 ++
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
 create mode 100644 include/dt-bindings/clock/marvell,mmp2-audio.h

Comments

Rob Herring May 12, 2020, 1:41 a.m. UTC | #1 
On Mon, 11 May 2020 21:55:33 +0200, Lubomir Rintel wrote:
> This describes the bindings for a controller that generates master and bit
> clocks for the I2S interface.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  .../clock/marvell,mmp2-audio-clock.yaml       | 73 +++++++++++++++++++
>  .../dt-bindings/clock/marvell,mmp2-audio.h    |  8 ++
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
>  create mode 100644 include/dt-bindings/clock/marvell,mmp2-audio.h
> 


My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.example.dts:20:18: fatal error: dt-bindings/power/marvell,mmp2.h: No such file or directory
         #include <dt-bindings/power/marvell,mmp2.h>
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1288040

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.
Stephen Boyd May 14, 2020, 9:06 p.m. UTC | #2 
Quoting Lubomir Rintel (2020-05-11 12:55:33)
> diff --git a/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
> new file mode 100644
> index 000000000000..b86e9fbfa56d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/marvell,mmp2-audio-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell MMP2 Audio Clock Controller
> +
> +maintainers:
> +  - Lubomir Rintel <lkundrak@v3.sk>
> +
> +description: |
> +  The audio clock controller generates and supplies the clocks to the audio
> +  codec.
> +
> +  Each clock is assigned an identifier and client nodes use this identifier
> +  to specify the clock which they consume.
> +
> +  All these identifiers could be found in <dt-bindings/clock/marvell,mmp2.h>.

Is this right? The patch puts them in mmp2-audio.h

> +
> +properties:
> +  compatible:
> +    enum:
> +      - marvell,mmp2-audio-clock
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Audio subsystem clock
> +      - description: The crystal oscillator clock
> +      - description: First I2S clock
> +      - description: Second I2S clock
> +
> +  clock-names:
> +    items:
> +      - const: audio
> +      - const: vctcxo
> +      - const: i2s0
> +      - const: i2s1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/marvell,mmp2.h>
> +    #include <dt-bindings/power/marvell,mmp2.h>
> +
> +    clocks@d42a0c30 {

clock-controller@d42a0c30

> +      compatible = "marvell,mmp2-audio-clock";
> +      reg = <0xd42a0c30 0x10>;

That is a very specific and tiny region. Presumably this is part of a
larger hardware block and thus shouldn't be described in DT this way.
Instead there should be one clock-controller node and a driver that
controls all the clks that it wants to inside that hardware block.

> +      clock-names = "audio", "vctcxo", "i2s0", "i2s1";
> +      clocks = <&soc_clocks MMP2_CLK_AUDIO>,
> +               <&soc_clocks MMP2_CLK_VCTCXO>,
> +               <&soc_clocks MMP2_CLK_I2S0>,
> +               <&soc_clocks MMP2_CLK_I2S1>;
> +      power-domains = <&soc_clocks MMP2_POWER_DOMAIN_AUDIO>;
> +      #clock-cells = <1>;
> +    };
> diff --git a/include/dt-bindings/clock/marvell,mmp2-audio.h b/include/dt-bindings/clock/marvell,mmp2-audio.h
> new file mode 100644
> index 000000000000..127b48ec0f0a
> --- /dev/null
> +++ b/include/dt-bindings/clock/marvell,mmp2-audio.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
> +#ifndef __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
> +#define __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
> +
> +#define MMP2_CLK_AUDIO_SYSCLK          1

Any reason to start at 1 vs. 0?

> +#define MMP2_CLK_AUDIO_SSPA0           2
> +#define MMP2_CLK_AUDIO_SSPA1           3
> +#endif
> -- 
> 2.26.2
>
Lubomir Rintel May 19, 2020, 10:21 p.m. UTC | #3 
On Thu, May 14, 2020 at 02:06:07PM -0700, Stephen Boyd wrote:
> Quoting Lubomir Rintel (2020-05-11 12:55:33)
> > diff --git a/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
> > new file mode 100644
> > index 000000000000..b86e9fbfa56d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/marvell,mmp2-audio-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Marvell MMP2 Audio Clock Controller
> > +
> > +maintainers:
> > +  - Lubomir Rintel <lkundrak@v3.sk>
> > +
> > +description: |
> > +  The audio clock controller generates and supplies the clocks to the audio
> > +  codec.
> > +
> > +  Each clock is assigned an identifier and client nodes use this identifier
> > +  to specify the clock which they consume.
> > +
> > +  All these identifiers could be found in <dt-bindings/clock/marvell,mmp2.h>.
> 
> Is this right? The patch puts them in mmp2-audio.h
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - marvell,mmp2-audio-clock
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Audio subsystem clock
> > +      - description: The crystal oscillator clock
> > +      - description: First I2S clock
> > +      - description: Second I2S clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: audio
> > +      - const: vctcxo
> > +      - const: i2s0
> > +      - const: i2s1
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/marvell,mmp2.h>
> > +    #include <dt-bindings/power/marvell,mmp2.h>
> > +
> > +    clocks@d42a0c30 {
> 
> clock-controller@d42a0c30
> 
> > +      compatible = "marvell,mmp2-audio-clock";
> > +      reg = <0xd42a0c30 0x10>;
> 
> That is a very specific and tiny region. Presumably this is part of a
> larger hardware block and thus shouldn't be described in DT this way.
> Instead there should be one clock-controller node and a driver that
> controls all the clks that it wants to inside that hardware block.

This resides in a block that's entirely separate from SoC's main clock
controllers ("power management units"). It is inside the audio block,
separate power island along with two I2S ("SSPA") controllers. The
addresses are weirdly interleaved, with the clock controller being
mapped between the two channels of the first SSPA:

  0xd42a0c00 - 0xd42a0c30 SSPA1 RX
  0xd42a0c30 - 0xd42a0c40 Clock Control
  0xd42a0c80 - 0xd42a0cb0 SSPA1 TX
  0xd42a0d00 - 0xd42a0d30 SSPA2 RX
  0xd42a0d80 - 0xd42a0cb0 SSPA2 TX

Despite the addresses being interwoven in this way, the Clock Controller
is pretty much independent of the SSPAs and deserves a separate device
node, regardless of how tiny its range is.

> > +      clock-names = "audio", "vctcxo", "i2s0", "i2s1";
> > +      clocks = <&soc_clocks MMP2_CLK_AUDIO>,
> > +               <&soc_clocks MMP2_CLK_VCTCXO>,
> > +               <&soc_clocks MMP2_CLK_I2S0>,
> > +               <&soc_clocks MMP2_CLK_I2S1>;
> > +      power-domains = <&soc_clocks MMP2_POWER_DOMAIN_AUDIO>;
> > +      #clock-cells = <1>;
> > +    };
> > diff --git a/include/dt-bindings/clock/marvell,mmp2-audio.h b/include/dt-bindings/clock/marvell,mmp2-audio.h
> > new file mode 100644
> > index 000000000000..127b48ec0f0a
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/marvell,mmp2-audio.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
> > +#ifndef __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
> > +#define __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
> > +
> > +#define MMP2_CLK_AUDIO_SYSCLK          1
> 
> Any reason to start at 1 vs. 0?
> 
> > +#define MMP2_CLK_AUDIO_SSPA0           2
> > +#define MMP2_CLK_AUDIO_SSPA1           3
> > +#endif
> > -- 
> > 2.26.2
> >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
new file mode 100644
index 000000000000..b86e9fbfa56d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
@@ -0,0 +1,73 @@ 
+# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/marvell,mmp2-audio-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell MMP2 Audio Clock Controller
+
+maintainers:
+  - Lubomir Rintel <lkundrak@v3.sk>
+
+description: |
+  The audio clock controller generates and supplies the clocks to the audio
+  codec.
+
+  Each clock is assigned an identifier and client nodes use this identifier
+  to specify the clock which they consume.
+
+  All these identifiers could be found in <dt-bindings/clock/marvell,mmp2.h>.
+
+properties:
+  compatible:
+    enum:
+      - marvell,mmp2-audio-clock
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Audio subsystem clock
+      - description: The crystal oscillator clock
+      - description: First I2S clock
+      - description: Second I2S clock
+
+  clock-names:
+    items:
+      - const: audio
+      - const: vctcxo
+      - const: i2s0
+      - const: i2s1
+
+  '#clock-cells':
+    const: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/marvell,mmp2.h>
+    #include <dt-bindings/power/marvell,mmp2.h>
+
+    clocks@d42a0c30 {
+      compatible = "marvell,mmp2-audio-clock";
+      reg = <0xd42a0c30 0x10>;
+      clock-names = "audio", "vctcxo", "i2s0", "i2s1";
+      clocks = <&soc_clocks MMP2_CLK_AUDIO>,
+               <&soc_clocks MMP2_CLK_VCTCXO>,
+               <&soc_clocks MMP2_CLK_I2S0>,
+               <&soc_clocks MMP2_CLK_I2S1>;
+      power-domains = <&soc_clocks MMP2_POWER_DOMAIN_AUDIO>;
+      #clock-cells = <1>;
+    };
diff --git a/include/dt-bindings/clock/marvell,mmp2-audio.h b/include/dt-bindings/clock/marvell,mmp2-audio.h
new file mode 100644
index 000000000000..127b48ec0f0a
--- /dev/null
+++ b/include/dt-bindings/clock/marvell,mmp2-audio.h
@@ -0,0 +1,8 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
+#ifndef __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
+#define __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
+
+#define MMP2_CLK_AUDIO_SYSCLK		1
+#define MMP2_CLK_AUDIO_SSPA0		2
+#define MMP2_CLK_AUDIO_SSPA1		3
+#endif