On Wed, 22 Feb 2023 10:40:59 +0000, Conor Dooley wrote: > 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@xxxxxxxx> >> > >> > Add bindings for the system clock and reset generator (SYSCRG) on the >> > JH7110 RISC-V SoC by StarFive Ltd. >> > >> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx> >> > Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx> >> > Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx> >> >> 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? Sorry for that I didn't synchronize this with Xingyu. I'll move these PLL definitions into the driver in the next version. Best regards, Hal > > 1 - https://patchwork.kernel.org/project/linux-riscv/patch/20230221141147.303642-3-xingyu.wu@xxxxxxxxxxxxxxxx/ >
