RE: [PATCH v8 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

[Ryan Chen <ryan_chen@aspeedtech.com>]

> 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@kernel.org>
> >> Sent: Tuesday, February 11, 2025 4:18 PM
> >> To: Ryan Chen <ryan_chen@aspeedtech.com>
> >> Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> >> <sboyd@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; Joel
> >> Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>;
> >> linux-clk@vger.kernel.org; Rob Herring <robh@kernel.org>; Krzysztof
> >> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> >> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> >> 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@aspeedtech.com>
> >>> ---
> >>>  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