| 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
| ||
|
Message-ID: <714caf6e-5f81-6d73-7629-b2c675f1f1d4@linaro.org>
Date: Mon, 18 Apr 2022 18:28:39 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Cixi Geng <gengcixi@...il.com>, mturquette@...libre.com,
sboyd@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, orsonzhai@...il.com,
baolin.wang7@...il.com, zhang.lyra@...il.com
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 1/3] dt-bindings: clk: sprd: Add bindings for ums512
clock controller
On 18/04/2022 14:56, Cixi Geng wrote:
> From: Cixi Geng <cixi.geng1@...soc.com>
>
> Add a new bindings to describe ums512 clock compatible string.
>
> Signed-off-by: Cixi Geng <cixi.geng1@...soc.com>
> ---
> .../bindings/clock/sprd,ums512-clk.yaml | 112 ++++++++++++++++++
> 1 file changed, 112 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> new file mode 100644
> index 000000000000..89824d7c6be4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 Unisoc Inc.
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/clock/sprd,ums512-clk.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: UMS512 Clock Control Unit Device Tree Bindings
Remove "Device Tree Bindings". You could do the same also in the
subject, because you repeat the prefix ("dt-bindings: clk: sprd: Add
ums512 clock controller").
> +
> +maintainers:
> + - Orson Zhai <orsonzhai@...il.com>
> + - Baolin Wang <baolin.wang7@...il.com>
> + - Chunyan Zhang <zhang.lyra@...il.com>
> +
> +properties:
> + "#clock-cells":
> + const: 1
> +
> + compatible:
Put the compatible first, by convention and makes finding matching
bindings easier.
> + oneOf:
> + - items:
> + - const: sprd,ums512-glbregs
> + - const: syscon
> + - const: simple-mfd
Why do you need simple-mfd for these? This looks like a regular syscon,
so usually does not come with children. What is more, why this "usual
syscon" is a separate clock controller in these bindings?
> + - items:
> + - enum:
> + - sprd,ums512-apahb-gate
> + - sprd,ums512-ap-clk
> + - sprd,ums512-aonapb-clk
> + - sprd,ums512-pmu-gate
> + - sprd,ums512-g0-pll
> + - sprd,ums512-g2-pll
> + - sprd,ums512-g3-pll
> + - sprd,ums512-gc-pll
> + - sprd,ums512-aon-gate
> + - sprd,ums512-audcpapb-gate
> + - sprd,ums512-audcpahb-gate
> + - sprd,ums512-gpu-clk
> + - sprd,ums512-mm-clk
> + - sprd,ums512-mm-gate-clk
> + - sprd,ums512-apapb-gate
> +
> + clocks:
> + minItems: 1
maxItems needed
> + description: |
> + The input parent clock(s) phandle for this clock, only list fixed
> + clocks which are declared in devicetree.
The description does not make sense. These are bindings for a clock
controller, but you say here "for this clock", so what does "this" mean
here?
> +
> + clock-names:
> + minItems: 1
> + items:
> + - const: ext-26m
> + - const: ext-32k
> + - const: ext-4m
> + - const: rco-100m
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - '#clock-cells'
Isn't reg also required? Always? Do you have examples where it is not
required? How do you configure the clocks without "reg"? I see you
copied a lot from sprd,sc9863a-clk.yaml but that file does not look correct.
You have nodes with reg but without unit address ("rpll"). These nodes
are modeled as children but they are not children - it's a workaround
for exposing syscon, isn't it? The sc9863a looks like broken design, so
please do not duplicate it here.
> +
> +if:
> + properties:
> + compatible:
> + enum:
> + - sprd,ums512-ap-clk
> + - sprd,ums512-aonapb-clk
> + - sprd,ums512-mm-clk
> +then:
> + required:
> + - reg
> +
> +else:
> + description: |
> + Other UMS512 clock nodes should be the child of a syscon node in
> + which compatible string should be:
> + "sprd,ums512-glbregs", "syscon", "simple-mfd"
> +
> + The 'reg' property for the clock node is also required if there is a sub
> + range of registers for the clocks.
> +
> +additionalProperties: true
false
> +
> +examples:
> + - |
> + ap_clk: clock-controller@...00000 {
> + compatible = "sprd,ums512-ap-clk";
> + reg = <0x20200000 0x1000>;
> + clocks = <&ext_26m>;
> + clock-names = "ext-26m";
> + #clock-cells = <1>;
> + };
> +
> + - |
> + ap_apb_regs: syscon@...00000 {
> + compatible = "sprd,ums512-glbregs", "syscon", "simple-mfd";
So here is the answer why you added MFD, but I still don't get why do
you need it for one child? It is quite a dance here and in your drivers,
instead of adding "syscon" to your clock controller.
This also pollutes the actual bindings because you did not add
properties required for clock controllers, because of describing here
the syscon part.
> + reg = <0x71000000 0x3000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #clock-cells = <1>;
> + ranges = <0 0x71000000 0x3000>;
> +
> + apahb_gate: clock-controller@0 {
> + compatible = "sprd,ums512-apahb-gate";
> + reg = <0x0 0x2000>;
> + #clock-cells = <1>;
> + };
> + };
> +
> +...
Best regards,
Krzysztof
Powered by blists - more mailing lists