| 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
| ||
|
Date: Tue, 20 Dec 2022 23:14:39 +0000
From: Conor Dooley <conor@...nel.org>
To: Hal Feng <hal.feng@...rfivetech.com>
Cc: linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
linux-clk@...r.kernel.org, Palmer Dabbelt <palmer@...belt.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Stephen Boyd <sboyd@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Emil Renner Berthing <emil.renner.berthing@...onical.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 07/11] dt-bindings: clock: Add StarFive JH7110 system
clock and reset generator
On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@...il.dk>
>
> Add bindings for the system clock and reset generator (SYSCRG) on the
> JH7110 RISC-V SoC by StarFive Ltd.
>
> Signed-off-by: Emil Renner Berthing <kernel@...il.dk>
> Signed-off-by: Hal Feng <hal.feng@...rfivetech.com>
> ---
> .../clock/starfive,jh7110-syscrg.yaml | 80 +++++++
> MAINTAINERS | 8 +-
> .../dt-bindings/clock/starfive,jh7110-crg.h | 207 ++++++++++++++++++
> .../dt-bindings/reset/starfive,jh7110-crg.h | 142 ++++++++++++
> 4 files changed, 434 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> create mode 100644 include/dt-bindings/clock/starfive,jh7110-crg.h
> create mode 100644 include/dt-bindings/reset/starfive,jh7110-crg.h
>
> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> new file mode 100644
> index 000000000000..ec81504dcb27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/starfive,jh7110-syscrg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 System Clock and Reset Generator
> +
> +maintainers:
> + - Emil Renner Berthing <kernel@...il.dk>
> +
> +properties:
> + compatible:
> + const: starfive,jh7110-syscrg
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: Main Oscillator (24 MHz)
> + - description: GMAC1 RMII reference
> + - description: GMAC1 RGMII RX
> + - description: External I2S TX bit clock
> + - description: External I2S TX left/right channel clock
> + - description: External I2S RX bit clock
> + - description: External I2S RX left/right channel clock
> + - description: External TDM clock
> + - description: External audio master clock
So, from peeking at the clock driver & the dt - it looks like a bunch of
these are not actually required?
I'd have ploughed through this, but having read Krzysztof's comments on
the DTS I'm not sure that this binding is correct.
https://lore.kernel.org/linux-riscv/20221220011247.35560-1-hal.feng@starfivetech.com/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
I *think* the DT is correct - the fixed clocks are all inputs from clock
sources on the board and as such they are empty in soc.dtsi and are
populated in board.dts?
However, are they all actually required? In the driver I see:
JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
JH7110_SYSCLK_GMAC1_RGMII_RXIN,
JH7110_SYSCLK_GMAC1_RMII_RTX),
That macro is:
#define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = { \
.name = _name, \
.flags = 0, \
.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT, \
.parents = { __VA_ARGS__ }, \
}
AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
Does that mean you need to populate only one of GMAC1 RMII reference
and GMAC1 RMGII RX and the other is optional?
What have I missed?
> +
> + clock-names:
> + items:
> + - const: osc
> + - const: gmac1_rmii_refin
> + - const: gmac1_rgmii_rxin
> + - const: i2stx_bclk_ext
> + - const: i2stx_lrck_ext
> + - const: i2srx_bclk_ext
> + - const: i2srx_lrck_ext
> + - const: tdm_ext
> + - const: mclk_ext
If all clocks are in fact required though, isn't this kinda pointless to
have since we already know that the order is fixed from the "clocks"
property?
Krzk/Rob?
> +
> + '#clock-cells':
> + const: 1
> + description:
> + See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
> +
> + '#reset-cells':
> + const: 1
> + description:
> + See <dt-bindings/reset/starfive,jh7110-crg.h> for valid indices.
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - '#clock-cells'
> + - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + clock-controller@...20000 {
> + compatible = "starfive,jh7110-syscrg";
> + reg = <0x13020000 0x10000>;
> + clocks = <&osc>, <&gmac1_rmii_refin>,
> + <&gmac1_rgmii_rxin>,
> + <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> + <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> + <&tdm_ext>, <&mclk_ext>;
> + clock-names = "osc", "gmac1_rmii_refin",
> + "gmac1_rgmii_rxin",
> + "i2stx_bclk_ext", "i2stx_lrck_ext",
> + "i2srx_bclk_ext", "i2srx_lrck_ext",
> + "tdm_ext", "mclk_ext";
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> + };
Also, whatever happened to GMAC0? It has fixed-clocks defined in the DTS
but doesn't appear in the binding or driver?
Thanks,
Conor.
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists