linux-kernel - Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Mar 2022 22:42:44 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Tim Chang <jia-wei.chang@...iatek.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        fan.chen@...iatek.com, louis.yu@...iatek.com,
        roger.lu@...iatek.com, Allen-yy.Lin@...iatek.com,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        hsinyi@...gle.com,
        Jia-Wei Chang <jia-wei.chang@...iatek.corp-partner.google.com>
Subject: Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq
 dt-bindings

On 07/03/2022 13:25, Tim Chang wrote:
> add devicetree binding of mtk cci devfreq on MediaTek SoC.

Start with capital letter.

> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@...iatek.corp-partner.google.com>

This does not match your From. Please fix this in all your submissions.

> ---
>  .../devicetree/bindings/devfreq/mtk-cci.yaml  | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> new file mode 100644
> index 000000000000..e64ac4c56758
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/mtk-cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Cache Coherent Interconnect (CCI) Devfreq driver Device Tree Bindings

Similarly to your other patches - the title describes hardware. Please
fix it in all your submissions of all your series.

Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this
maybe "bus frequency scaling"? Although later you call the device node
as cci.

> +
> +maintainers:
> +  - Jia-Wei Chang <jia-wei.chang@...iatek.com>
> +
> +description: |
> +  This module is used to create CCI DEVFREQ.
> +  The performance will depend on both CCI frequency and CPU frequency.
> +  For MT8186, CCI co-buck with Little core.
> +  Contain CCI opp table for voltage and frequency scaling.

Half of this description (first and last sentence) does not describe the
actual hardware. Please describe hardware, not driver.

> +
> +properties:
> +  compatible:
> +    const: "mediatek,mt8186-cci"

No need for quotes.

> +
> +  clocks:
> +    items:
> +      - description:
> +          The first one is the multiplexer for clock input of CPU cluster.
> +      - description:
> +          The other is used as an intermediate clock source when the original
> +          CPU is under transition and not stable yet.
> +
> +  clock-names:
> +    items:
> +      - const: "cci"
> +      - const: "intermediate"

No need for quotes.

> +
> +  operating-points-v2:
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> +
> +  opp-table: true

Same comments as your CPU freq bindings apply.

> +
> +  proc-supply:
> +    description:
> +      Phandle of the regulator for CCI that provides the supply voltage.
> +
> +  sram-supply:
> +    description:
> +      Phandle of the regulator for sram of CCI that provides the supply
> +      voltage. When present, the cci devfreq driver needs to do
> +      "voltage tracking" to step by step scale up/down Vproc and Vsram to fit
> +      SoC specific needs. When absent, the voltage scaling flow is handled by
> +      hardware, hence no software "voltage tracking" is needed.
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - operating-points-v2
> +  - proc-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt8186-clk.h>
> +    cci: cci {

Node names should be generic and describe type of device. Are you sure
this is a CCI? Maybe "interconnect" suits it better?

> +      compatible = "mediatek,mt8186-cci";
> +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
> +      clock-names = "cci", "intermediate";
> +      operating-points-v2 = <&cci_opp>;
> +      proc-supply = <&mt6358_vproc12_reg>;
> +      sram-supply = <&mt6358_vsram_proc12_reg>;
> +    };


Best regards,
Krzysztof