Re: [PATCH v5 06/39] dt-bindings: soc: Add Cirrus EP93xx
From: Nikita Shubin
Date: Mon Dec 11 2023 - 03:46:33 EST
Hello Robert!
On Mon, 2023-11-27 at 14:18 -0600, Rob Herring wrote:
> On Wed, Nov 22, 2023 at 11:59:44AM +0300, Nikita Shubin wrote:
> > Add device tree bindings for the Cirrus Logic EP93xx SoC.
> >
> > Signed-off-by: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> > ---
> > .../bindings/arm/cirrus/cirrus,ep9301.yaml | 38 ++++++++++
>
> > .../bindings/soc/cirrus/cirrus,ep9301-syscon.yaml | 83
> > ++++++++++++++++++++++
> > include/dt-bindings/soc/cirrus,ep9301-syscon.h | 46
> > ++++++++++++
>
> This looks like it is mostly clocks. I'd put it in bindings/clock/
> and
> include/dt-bindings/clock/ instead.
>
> > 3 files changed, 167 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/cirrus/cirrus,ep9301.yaml
> > b/Documentation/devicetree/bindings/arm/cirrus/cirrus,ep9301.yaml
> > new file mode 100644
> > index 000000000000..97dd8b6aefa9
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/arm/cirrus/cirrus,ep9301.yaml
> > @@ -0,0 +1,38 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/cirrus/cirrus,ep9301.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cirrus Logic EP93xx platforms
> > +
> > +description:
> > + The EP93xx SoC is a ARMv4T-based with 200 MHz ARM9 CPU.
> > +
> > +maintainers:
> > + - Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> > + - Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> > +
> > +properties:
> > + $nodename:
> > + const: '/'
> > + compatible:
> > + oneOf:
> > + - description: The TS-7250 is a compact, full-featured
> > Single Board Computer (SBC)
> > + based upon the Cirrus EP9302 ARM9 CPU
>
> Wrap lines at 80 unless there's some advantage to going to 100. Here
> there is not.
>
> > + items:
> > + - const: technologic,ts7250
> > + - const: cirrus,ep9301
> > +
> > + - description: The Liebherr BK3 is a derivate from ts7250
> > board
> > + items:
> > + - const: liebherr,bk3
> > + - const: cirrus,ep9301
> > +
> > + - description: EDB302 is an evaluation board by Cirrus
> > Logic,
> > + based on a Cirrus Logic EP9302 CPU
> > + items:
> > + - const: cirrus,edb9302
> > + - const: cirrus,ep9301
> > +
> > +additionalProperties: true
> > diff --git
> > a/Documentation/devicetree/bindings/soc/cirrus/cirrus,ep9301-
> > syscon.yaml
> > b/Documentation/devicetree/bindings/soc/cirrus/cirrus,ep9301-
> > syscon.yaml
> > new file mode 100644
> > index 000000000000..283cf2386d95
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/cirrus/cirrus,ep9301-
> > syscon.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/soc/cirrus/cirrus,ep9301-syscon.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cirrus Logic EP93xx Platforms System Controller
> > +
> > +maintainers:
> > + - Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> > + - Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - items:
> > + - enum:
> > + - cirrus,ep9302-syscon
> > + - cirrus,ep9307-syscon
> > + - cirrus,ep9312-syscon
> > + - cirrus,ep9315-syscon
>
> The registers of these are all the same as (or a superset of) the
> EP9301? That's what you are claiming. Perhaps already discussed, but
> there's no blurb here to explain the relationship.
>
> 'syscon' is a Linux term. Is this block really called this?
Citing documentation:
"The System Controller (Syscon) provides:
• Clock control
• Power management
• System configuration management
These central resources are controlled by a set of software-locked
registers, which can be
used to prevent accidental accesses. Syscon generates the various bus
and peripheral
clocks and controls the system startup configuration."
I'll put the following into description:
"Central resources are controlled by a set of software-locked
registers, which can be used to prevent accidental accesses. Syscon
generates the various bus and peripheral clocks and controls the system
startup configuration.
The System Controller (Syscon) provides:
- Clock control
- Power management
- System configuration management
Syscon registers are common for all EP93xx SoC's, through some actual
peripheral may be missing depending on actual SoC model."
>
> > + - const: cirrus,ep9301-syscon
> > + - const: syscon
> > + - const: simple-mfd
> > + - items:
> > + - const: cirrus,ep9301-syscon
> > + - const: syscon
> > + - const: simple-mfd
>
> Doesn't look like an simple-mfd to me. You don't have independent
> child
> blocks which don't depend on the parent node.
>
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + "#clock-cells":
> > + const: 1
> > +
> > + clocks:
> > + items:
> > + - description: reference clock
> > +
> > +patternProperties:
> > + '^pins-':
> > + type: object
> > + description: pin node
> > + $ref: /schemas/pinctrl/pinmux-node.yaml
> > +
> > + properties:
> > + function:
> > + enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio ]
> > +
> > + groups:
> > + enum: [ ssp, ac97, i2s_on_ssp, i2s_on_ac97, pwm1,
> > gpio1agrp,
> > + gpio2agrp, gpio3agrp, gpio4agrp, gpio6agrp,
> > gpio7agrp,
> > + rasteronsdram0grp, rasteronsdram3grp, keypadgrp,
> > idegrp ]
> > +
> > + required:
> > + - function
> > + - groups
> > +
> > + unevaluatedProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - "#clock-cells"
> > + - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + syscon@80930000 {
> > + compatible = "cirrus,ep9301-syscon",
> > + "syscon", "simple-mfd";
> > + reg = <0x80930000 0x1000>;
> > +
> > + #clock-cells = <1>;
> > + clocks = <&xtali>;
> > +
> > + spi_default_pins: pins-spi {
> > + function = "spi";
> > + groups = "ssp";
> > + };
> > + };
> > diff --git a/include/dt-bindings/soc/cirrus,ep9301-syscon.h
> > b/include/dt-bindings/soc/cirrus,ep9301-syscon.h
> > new file mode 100644
> > index 000000000000..6bb8f532e7d0
> > --- /dev/null
> > +++ b/include/dt-bindings/soc/cirrus,ep9301-syscon.h
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> > +#ifndef DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> > +#define DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> > +
> > +#define EP93XX_CLK_PLL1 0
> > +#define EP93XX_CLK_PLL2 1
> > +
> > +#define EP93XX_CLK_FCLK 2
> > +#define EP93XX_CLK_HCLK 3
> > +#define EP93XX_CLK_PCLK 4
> > +
> > +#define EP93XX_CLK_UART 5
> > +#define EP93XX_CLK_SPI 6
> > +#define EP93XX_CLK_PWM 7
> > +#define EP93XX_CLK_USB 8
> > +
> > +#define EP93XX_CLK_M2M0 9
> > +#define EP93XX_CLK_M2M1 10
> > +
> > +#define EP93XX_CLK_M2P0 11
> > +#define EP93XX_CLK_M2P1 12
> > +#define EP93XX_CLK_M2P2 13
> > +#define EP93XX_CLK_M2P3 14
> > +#define EP93XX_CLK_M2P4 15
> > +#define EP93XX_CLK_M2P5 16
> > +#define EP93XX_CLK_M2P6 17
> > +#define EP93XX_CLK_M2P7 18
> > +#define EP93XX_CLK_M2P8 19
> > +#define EP93XX_CLK_M2P9 20
> > +
> > +#define EP93XX_CLK_UART1 21
> > +#define EP93XX_CLK_UART2 22
> > +#define EP93XX_CLK_UART3 23
> > +
> > +#define EP93XX_CLK_ADC 24
> > +#define EP93XX_CLK_ADC_EN 25
> > +
> > +#define EP93XX_CLK_KEYPAD 26
> > +
> > +#define EP93XX_CLK_VIDEO 27
> > +
> > +#define EP93XX_CLK_I2S_MCLK 28
> > +#define EP93XX_CLK_I2S_SCLK 29
> > +#define EP93XX_CLK_I2S_LRCLK 30
> > +
> > +#endif /* DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H */
> >
> > --
> > 2.41.0
> >