> Subject: Re: [PATCH v8 1/3] dt-binding: clock: ast2700: modify soc0/1 clock > define > > On 15/02/2025 03:14, Ryan Chen wrote: > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx> > >> Sent: Tuesday, February 11, 2025 4:18 PM > >> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx> > >> Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>; Stephen Boyd > >> <sboyd@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; Joel > >> Stanley <joel@xxxxxxxxx>; Andrew Jeffery <andrew@xxxxxxxx>; > >> linux-clk@xxxxxxxxxxxxxxx; Rob Herring <robh@xxxxxxxxxx>; Krzysztof > >> Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; > >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-aspeed@xxxxxxxxxxxxxxxx; > >> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > >> Subject: Re: [PATCH v8 1/3] dt-binding: clock: ast2700: modify soc0/1 > >> clock define > >> > >> On Mon, Feb 10, 2025 at 04:50:02PM +0800, Ryan Chen wrote: > >>> remove soc0 clock: > >> > >> Why? Your commit msg must explain why. What is obvious from the diff, > >> isn't it? > > Thank you for your feedback. I will add explanation in next commit patch. > >> > >>> SOC0_CLK_UART_DIV13 > >>> SOC0_CLK_HPLL_DIV_AHB > >>> SOC0_CLK_MPLL_DIV_AHB > >>> add soc0 clock: > >>> SOC0_CLK_AHBMUX > >>> SOC0_CLK_MPHYSRC > >>> SOC0_CLK_U2PHY_REFCLKSRC > >>> add soc1 clock: > >>> SOC1_CLK_I3C > >>> > >>> Signed-off-by: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx> > >>> --- > >>> include/dt-bindings/clock/aspeed,ast2700-scu.h | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/dt-bindings/clock/aspeed,ast2700-scu.h > >>> b/include/dt-bindings/clock/aspeed,ast2700-scu.h > >>> index 63021af3caf5..c7389530629d 100644 > >>> --- a/include/dt-bindings/clock/aspeed,ast2700-scu.h > >>> +++ b/include/dt-bindings/clock/aspeed,ast2700-scu.h > >>> @@ -13,18 +13,17 @@ > >>> #define SCU0_CLK_24M 1 > >>> #define SCU0_CLK_192M 2 > >>> #define SCU0_CLK_UART 3 > >>> -#define SCU0_CLK_UART_DIV13 3 > >> > >> NAK, ABI break without any explanation. > > > > The `SCU0_CLK_UART_DIV13` was originally defined as a separate clock > identifier, reviewing the AST2700 clock driver implement, I realized it is no > longer necessary. > > The clk-ast2700.c driver I have **integrated the SOC0 UART clock > (`soc0_uartclk`) with `ast2700_clk_uart_div_table`**. > > The UART clock source will get from ast2700_clk_uart_div_table, that will div > from source 24M div13 or div1. > > Wrap your replies correctly. > > So all this means you exported clocks which are not clocks? > How are ABI consumers behaving now? In my original through will be separate clock identifier one is uart clk, another is uart clk_div13. uart_clk src -> uart clk uart clk_src div 13 and I real implement in v6, not use in clk-ast2700.c. But I keep the SCU0_CLK_UART_DIV13 in dt-binding define, not remove it when submit. The SCU0_CLK_UART_DIV13 is redundant.> uart clk src -> uart_div_table -> uart clk > Anyway, any ABI impact must be clearly justified in commit msg. > Thanks for your feedback I will add the commit message by following. -Remove redundant SCU0_CLK_UART_DIV13. SCU0_CLK_UART_DIV13 is not use at clk-ast2700.c The clock source tree is uart clk src -> uart_div_table -> uart clk -Change SCU0_CLK_HPLL_DIV_AHB to SCU0_CLK_AHBMUX Modify clock tree implement in clk-ast2700.c. old clock-ast2700.c CLK_AHB implement mpll_div_ahb / hpll_div_ahb to be ahb clock source. mpll-> mpll_div_ahb -> clk_ahb hpll-> hpll_div_ahb new clock-ast2700.c implement by SCU0_CLK_AHBMUX for more understand clock source divide tree. Add define SCU0_CLK_AHBMUX replace SCU0_CLK_HPLL_DIV_AHB mpll-> ahb_mux -> div_table -> clk_ahb hpll-> -new add clock SOC0_CLK_MPHYSRC: UFS MPHY clock source. SOC0_CLK_U2PHY_REFCLKSRC : USB2.0 phy clock reference source SOC1_CLK_I3C: I3C clock source. > > > Best regards, > Krzysztof
