Re: [PATCH 02/16] dt-bindings: clock: Add ARTPEC-8 CMU bindings

[Hakyeong Kim <hgkim05@coasia.com>]

On Thu, 2025-07-10 at 09:10 +0200, Krzysztof Kozlowski wrote:
> On 10/07/2025 02:20, ksk4725@coasia.com wrote:
> > From: Hakyeong Kim <hgkim05@coasia.com>
> > 
> > Add dt-schema for ARTPEC-8 SoC clock controller.
> > 
> > Add device-tree binding definitions for following CMU blocks:
> > - CMU_CMU
> > - CMU_BUS
> > - CMU_CORE
> > - CMU_CPUCL
> > - CMU_FSYS
> > - CMU_IMEM
> > - CMU_PERI
> > 
> > Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> > Signed-off-by: Hakyeong Kim <hgkim05@coasia.com>
> 
> Confusing order, unless you really understand this, but considering
> you
> did not add your own SoB I claim you do not understand this. What
> does
> Ravi's SoB mean here?

Ok, I will update the SoB section in all the patches.

> 
> > ---
> >  .../bindings/clock/axis,artpec8-clock.yaml    | 224
> > ++++++++++++++++++
> >  1 file changed, 224 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/clock/axis,artpec8-clock.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/axis,artpec8-
> > clock.yaml b/Documentation/devicetree/bindings/clock/axis,artpec8-
> > clock.yaml
> > new file mode 100644
> > index 000000000000..baacea10599b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/axis,artpec8-
> > clock.yaml
> > @@ -0,0 +1,224 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/axis,artpec8-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Axis ARTPEC-8 SoC clock controller
> > +
> > +maintainers:
> > +  - Jesper Nilsson <jesper.nilsson@axis.com>
> > +
> > +description: |
> > +  ARTPEC-8 clock controller is comprised of several CMU units,
> > generating
> > +  clocks for different domains. Those CMU units are modeled as
> > separate device
> > +  tree nodes, and might depend on each other. The root clock in
> > that root tree
> > +  is an external clock: OSCCLK (25 MHz). This external clock must
> > be defined
> > +  as a fixed-rate clock in dts.
> > +
> > +  CMU_CMU is a top-level CMU, where all base clocks are prepared
> > using PLLs and
> > +  dividers; all other clocks of function blocks (other CMUs) are
> > usually
> > +  derived from CMU_CMU.
> > +
> > +  Each clock is assigned an identifier and client nodes can use
> > this identifier
> > +  to specify the clock which they consume. All clocks available
> > for usage
> > +  in clock consumer nodes are defined as preprocessor macros in
> > +  'include/dt-bindings/clock/axis,artpec8-clk.h' header.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - axis,artpec8-cmu-cmu
> > +      - axis,artpec8-cmu-bus
> > +      - axis,artpec8-cmu-core
> > +      - axis,artpec8-cmu-cpucl
> > +      - axis,artpec8-cmu-fsys
> > +      - axis,artpec8-cmu-imem
> > +      - axis,artpec8-cmu-peri
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 5
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  reg:
> > +    maxItems: 1
> 
> reg goes second, after compatible (Samsung bindings are not the best
> example because I converted them long time ago before many coding
> style
> practices were encouraged)

Ok, I will move the reg property.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#clock-cells"
> > +  - clocks
> > +  - clock-names
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> 
> Drop contains.

Ok, I will remove the contains wherever applicable.

> 
> > +            const: axis,artpec8-cmu-cmu
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: External reference clock (25 MHz)
> > +
> > +        clock-names:
> > +          items:
> > +            - const: fin_pll
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: axis,artpec8-cmu-bus
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: External reference clock (25 MHz)
> > +            - description: CMU_BUS BUS clock (from CMU_CMU)
> > +            - description: CMU_BUS DLP clock (from CMU_CMU)
> > +
> > +        clock-names:
> > +          items:
> > +            - const: fin_pll
> > +            - const: dout_clkcmu_bus_bus
> > +            - const: dout_clkcmu_bus_dlp
> 
> All these names should be changed to match what is the input. Look at
> latest bindings, we moved away from that style.

Ok, I will modify clock-names to "bus" and "dlp" and will update at the
other related places also.

Thanks,
Hakyeong Kim

> 
> 
> 
> Best regards,
> Krzysztof