| Message ID | 20250210085004.1898895-2-ryan_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for AST2700 clk driver | expand |
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? > 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. > #define SCU0_CLK_PSP 4 > #define SCU0_CLK_HPLL 5 > #define SCU0_CLK_HPLL_DIV2 6 > #define SCU0_CLK_HPLL_DIV4 7 > -#define SCU0_CLK_HPLL_DIV_AHB 8 > +#define SCU0_CLK_AHBMUX 8 NAK, ABI break without any explanation. Best regards, Krzysztof
> -----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. > > > #define SCU0_CLK_PSP 4 > > #define SCU0_CLK_HPLL 5 > > #define SCU0_CLK_HPLL_DIV2 6 > > #define SCU0_CLK_HPLL_DIV4 7 > > -#define SCU0_CLK_HPLL_DIV_AHB 8 > > +#define SCU0_CLK_AHBMUX 8 > > NAK, ABI break without any explanation. V6 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 V8 clock-ast2700.c I 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 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
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? Anyway, any ABI impact must be clearly justified in commit msg. Best regards, Krzysztof
> 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
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 #define SCU0_CLK_PSP 4 #define SCU0_CLK_HPLL 5 #define SCU0_CLK_HPLL_DIV2 6 #define SCU0_CLK_HPLL_DIV4 7 -#define SCU0_CLK_HPLL_DIV_AHB 8 +#define SCU0_CLK_AHBMUX 8 #define SCU0_CLK_DPLL 9 #define SCU0_CLK_MPLL 10 #define SCU0_CLK_MPLL_DIV2 11 #define SCU0_CLK_MPLL_DIV4 12 #define SCU0_CLK_MPLL_DIV8 13 -#define SCU0_CLK_MPLL_DIV_AHB 14 +#define SCU0_CLK_MPHYSRC 14 #define SCU0_CLK_D0 15 #define SCU0_CLK_D1 16 #define SCU0_CLK_CRT0 17 @@ -68,6 +67,7 @@ #define SCU0_CLK_GATE_UFSCLK 53 #define SCU0_CLK_GATE_EMMCCLK 54 #define SCU0_CLK_GATE_RVAS1CLK 55 +#define SCU0_CLK_U2PHY_REFCLKSRC 56 /* SOC1 clk */ #define SCU1_CLKIN 0 @@ -160,4 +160,5 @@ #define SCU1_CLK_GATE_PORTDUSB2CLK 85 #define SCU1_CLK_GATE_LTPI1TXCLK 86 +#define SCU1_CLK_I3C 87 #endif
remove soc0 clock: 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(-)