Message-ID: <b26a52ff-6b8a-8a64-7189-346cd2b0d705@linaro.org>
Date:   Wed, 25 Jan 2023 10:55:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.0
Subject: Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings
To:     Li Chen <me@linux.beauty>
Cc:     Li Chen <lchen@ambarella.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20230123073305.149940-1-lchen@ambarella.com>
 <20230123073305.149940-8-lchen@ambarella.com>
 <0c19efb4-3bca-f500-ca24-14b9d24369ef@linaro.org>
 <87y1prgdyu.wl-me@linux.beauty>
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <87y1prgdyu.wl-me@linux.beauty>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

> 
> 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...

> 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.

> 
>>> +
>>> +  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.

> 
>>> +
>>> +  '#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.

Best regards,
Krzysztof