linux-kernel - Re: [PATCH v4 09/19] dt-bindings: clock: Add StarFive JH7110 system clock and reset generator

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:   Wed, 22 Feb 2023 10:40:59 +0000
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC:     Hal Feng <hal.feng@...rfivetech.com>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor@...nel.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Ben Dooks <ben.dooks@...ive.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Emil Renner Berthing <emil.renner.berthing@...onical.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 09/19] dt-bindings: clock: Add StarFive JH7110 system
 clock and reset generator

On Wed, Feb 22, 2023 at 10:13:19AM +0100, Krzysztof Kozlowski wrote:
> On 21/02/2023 03:46, 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.
> > 
> > Reviewed-by: Rob Herring <robh@...nel.org>
> > Signed-off-by: Emil Renner Berthing <kernel@...il.dk>
> > Signed-off-by: Hal Feng <hal.feng@...rfivetech.com>
> 
> I don't know what is happening here as neither this nor other patchset
> explains anything. Please stop writing what you do in the patches, but
> explain why. What is easy to get.
> 
> (...)
> 
> 
> > +
> > +#define JH7110_SYSCLK_PLL0_OUT			190
> > +#define JH7110_SYSCLK_PLL1_OUT			191
> > +#define JH7110_SYSCLK_PLL2_OUT			192
> 
> NAK. Do not add incorrect bindings just to remove it THE SAME TIME.

For some context, the PLL driver series [1] does the following, which is
where this complaint stems from:
> diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
> index 5e4f21ca0642..086a6ddcf380 100644
> --- a/include/dt-bindings/clock/starfive,jh7110-crg.h
> +++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
> @@ -6,6 +6,12 @@
>  #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>  #define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>  
> +/* PLL clocks */
> +#define JH7110_CLK_PLL0_OUT			0
> +#define JH7110_CLK_PLL1_OUT			1
> +#define JH7110_CLK_PLL2_OUT			2
> +#define JH7110_PLLCLK_END			3
> +
>  /* SYSCRG clocks */
>  #define JH7110_SYSCLK_CPU_ROOT			0
>  #define JH7110_SYSCLK_CPU_CORE			1
> @@ -198,11 +204,7 @@
>  #define JH7110_SYSCLK_TDM_TDM_INV		188
>  #define JH7110_SYSCLK_JTAG_CERTIFICATION_TRNG	189
>  
> -#define JH7110_SYSCLK_PLL0_OUT			190
> -#define JH7110_SYSCLK_PLL1_OUT			191
> -#define JH7110_SYSCLK_PLL2_OUT			192

I was talking to Emil, who pointed out that these defines aren't
actually ever used in the dts, so there's nothing really gained
by adding them here in the first place.
Seems like this series could simply move these defines into the driver
(as the PLL addition series also does) and then we would not have to
be worried about breaking the ABI in the future?

Thanks,
Conor.

1 - https://patchwork.kernel.org/project/linux-riscv/patch/20230221141147.303642-3-xingyu.wu@starfivetech.com/


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)