Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen <me@linux.beauty>]

On Wed, 25 Jan 2023 17:55:34 +0800,
Hi Krzysztof,

Krzysztof Kozlowski wrote:
>
> On 25/01/2023 10:28, Li Chen wrote:
> >
> > Hi Krzysztof,
> >
> > Sorry for my late reply.
> >
> > On Mon, 23 Jan 2023 16:11:08 +0800,
> > Krzysztof Kozlowski wrote:
> >>
> >> On 23/01/2023 08:32, Li Chen wrote:
> >>> This patch introduce clock bindings for Ambarella.
> >>>
> >>> Signed-off-by: Li Chen <lchen@ambarella.com>
> >>> Change-Id: I29018a23ed3a5b79a1103e859a5c7ed7bb83a261
> >>
> >> All the same problems plus new:
> >>
> >> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> >> prefix is already stating that these are bindings.
> >
> > Well noted.
> >
> >>> ---
> >>>  .../clock/ambarella,composite-clock.yaml      | 52 ++++++++++++++++
> >>>  .../bindings/clock/ambarella,pll-clock.yaml   | 59 +++++++++++++++++++
> >>>  MAINTAINERS                                   |  2 +
> >>>  3 files changed, 113 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >>>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >>> new file mode 100644
> >>> index 000000000000..fac1cb9379c4
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >>> @@ -0,0 +1,52 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/clock/ambarella,composite-clock.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Ambarella Composite Clock
> >>> +
> >>> +maintainers:
> >>> +  - Li Chen <lchen@ambarella.com>
> >>> +
> >>
> >> Missing description.
> >
> > Thanks, description as below will be added in v2:
> >
> > "Ambarella SoCs integrates some composite clocks, like uart0, which aggrate the functionality
> > of the basic clock types, like mux and div."
> >
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>
> >> Drop items.
> >
> > Ok.
> >
> >>> +      - const: ambarella,composite-clock
> >>
> >> Missing SoC specific compatible. This is anyway not really correct
> >> compatible...
> >
> > Most Ambarella's compatibles don't contain SoC name, because we prefer
> > to use syscon + offsets in dts to tell driver the correct register offsets, or
> > ues struct soc_device and SoC identity stores in a given physical address.
>
> That's not correct hardware description. Drop the syscon and offsets.

Ok.

> > 
> > So compatibles like "ambarella,composite-clock" and "ambarella,pinctrl" are
> > used widely in Ambarella kernels.
>
> What do you do downstream does not matter. You can invent any crazy idea
> and it is not an argument that it should be done like that. Usually
> downstream code is incorrect...

Yeah, I understand it.
I really hope to learn the standard/right ways and
I am very grateful for your careful reviews.

> > Feel free to correct me if you think this
> > is not a good idea.
>
> This is bad idea. Compatibles should be specific. Devices should not use
> syscons to poke other registers, unless strictly necessary, but have
> strictly defined MMIO address space and use it.

Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.

But I have three questions:

0. why syscon + offsets is a bad idea copared to specific compatibles?
1. when would it be a good idea to use syscon in device tree?
2. syscon VS reg, which is preferred in device tree?

Thanks in advanced.

> >
> >>> +
> >>> +  clocks: true
> >>
> >> No, needs constraints.
> >
> > Ok. I will list all clocks name
> >
> >>> +  assigned-clocks: true
> >>> +  assigned-clock-parents: true
> >>> +  assigned-clock-rates: true
> >>
> >> Drop these three.
> >
> > Ok
> >
> >>> +  clock-output-names: true
> >>
> >> Missing constraints.
> >
> > Ok, I will add "maxItems: 1"
> >
> >>> +  amb,mux-regmap: true
> >>
> >> NAK.
> >>
> >> It's enough. The patches have very, very poor quality.
> >>
> >> Missing description, missing type/$ref, wrong prefix.
> >
> > Sorry, I forget to run dt_binding_check, I will spend some
> > time learning the binding and check, sorry for it.
> >
> >>> +  amb,div-regmap: true
> >>> +  amb,div-width: true
> >>> +  amb,div-shift: true
> >>
> >> These two are arguments to phandle.
> >
> > I will add description and $ref to regmap and width/shift.
>
> Drop all these syscon properties.

Ok, so I should replace these regmaps with reg, right?

> >
> >>> +
> >>> +  '#clock-cells':
> >>> +    const: 0
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - clocks
> >>> +  - '#clock-cells'
> >>> +
> >>> +additionalProperties: false
> >>
> >> So why you decided to add it here and not in other places?
> >
> > I didn't understand it well. I will add it to other places in v2,
> > thanks for pointint out it.
> >
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +      gclk_uart0: gclk-uart0 {
> >>
> >> Wrong indentation.
> >
> > Well noted.
> >
> >>> +        #clock-cells = <0>;
> >>> +        compatible = "ambarella,composite-clock";
> >>> +        clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
> >>> +        clock-output-names = "gclk_uart0";
> >>> +        assigned-clocks = <&gclk_uart0>;
> >>> +        assigned-clock-parents = <&osc>;
> >>> +        assigned-clock-rates = <24000000>;
> >>> +        amb,mux-regmap = <&rct_syscon 0x1c8>;
> >>> +        amb,div-regmap = <&rct_syscon 0x038>;
> >>> +        amb,div-width = <24>;
> >>> +        amb,div-shift = <0>;
> >>> +      };
> >>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >>> new file mode 100644
> >>> index 000000000000..65c1feb60041
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >>> @@ -0,0 +1,59 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/clock/ambarella,pll-clock.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Ambarella PLL Clock
> >>> +
> >>> +maintainers:
> >>> +  - Li Chen <lchen@ambarella.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - ambarella,pll-clock
> >>> +      - ambarella,clkpll-v0
> >>> +
> >>> +if:
> >>
> >> No, this does not work like that. It sits under "allOf", located after
> >> "required:".
> >
> > Thanks, I will learn "allOf" and use it in v2. BTW, we use the two compatibles as below:
> > clocks {
> >                 compatible = "ambarella,clkpll-v0";
>
> Nope.
>
> >                 ...
> >                 gclk_core: gclk-core {
> >                         #clock-cells = <0>;
> >                         compatible = "ambarella,pll-clock";
>
> Also nope.
>
> >                         clocks = <&osc>;
> >                         clock-output-names = "gclk_core";
> >                         amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>;
>
> Nope, nope, nope.
>
> You need proper clock-controller with its own MMIO address space.
>
> >                 };
> >                 ...
> > }
> >
> > I'm not sure can I describe the two compatibles in this single yaml, can you give some advice? thanks!
>
> There are plenty of examples, including example-schema.

Ok, I will learn more and fix it.

Regards,
Li