linux-kernel - Re: [PATCH v3 3/3] clk: spacemit: fix i2s clock

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]

Message-ID: <aKL6ormE1N72fwVG@ketchup>
Date: Mon, 18 Aug 2025 10:04:18 +0000
From: Haylen Chu <heylenay@....org>
To: Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
	Alex Elder <elder@...cstar.com>,
	Inochi Amaoto <inochiama@...look.com>
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Jinmei Wei <weijinmei@...ux.spacemit.com>
Subject: Re: [PATCH v3 3/3] clk: spacemit: fix i2s clock

On Mon, Aug 18, 2025 at 05:28:22PM +0800, Troy Mitchell wrote:
> Defining i2s_bclk and i2s_sysclk as fixed-rate clocks is insufficient
> for real I2S use cases.
> 
> Moreover, the current I2S clock configuration does not work as expected
> due to missing parent clocks.
> 
> This patch adds the missing parent clocks, defines i2s_sysclk as
> a DDN clock, and i2s_bclk as a DIV clock.
> 
> A special note for i2s_bclk:
> 
> From the definition of register, The i2s_bclk is a non-linear,
> discrete divider clock.

No, it IS linear. It just comes with a 1/2 factor according to your code
(I'm assuming there's a typo in the table below).

> In calculus and related areas, a linear function is a function whose
> graph is a straight line, that is, a polynomial function of degree
> zero or one. (From Wikipedia)

> The following table shows the correspondence between index
> and frequency division coefficients:
> 
> | index |  div  |
> |-------|-------|
> |   0   |   2   |
> |   1   |   4   |
> |   2   |   6   |
> |   2   |   8   |

Index = 2 appears twice in the table. Is this a typo?

> From a software perspective, introducing i2s_bclk_factor as the
> parent of i2s_bclk is sufficient to address the issue.
> 
> The I2S-related clock registers can be found here [1].
> 
> Link:
> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb
> [1]
> 
> Fixes: 1b72c59db0add ("clk: spacemit: Add clock support for SpacemiT K1 SoC")
> Co-developer: Jinmei Wei <weijinmei@...ux.spacemit.com>
> Suggested-by: Haylen Chu <heylenay@....org>
> Signed-off-by: Jinmei Wei <weijinmei@...ux.spacemit.com>
> Signed-off-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> ---
>  drivers/clk/spacemit/ccu-k1.c    | 29 +++++++++++++++++++++++++++--
>  drivers/clk/spacemit/ccu_mix.h   |  2 +-
>  include/soc/spacemit/k1-syscon.h |  1 +
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index 7155824673fb450971439873b6b6163faf48c7e5..b2c426b629a37a9901bbced26fc55c5f1b34eba5 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c

...

> + * i2s_bclk is a non-linear discrete divider clock.
> + * Using i2s_bclk_factor as its parent simplifies software handling
> + * and avoids dealing with the non-linear division directly.
> + */

And thus this comment is wrong and misleading. Suggest something like,

	Divider of i2s_bclk always implies a 1/2 factor, which is
	described by i2s_bclk_factor.

> +CCU_DIV_GATE_DEFINE(i2s_bclk, CCU_PARENT_HW(i2s_bclk_factor), MPMU_ISCCR, 27, 2, BIT(29), 0);

>  static const struct clk_parent_data apb_parents[] = {
>  	CCU_PARENT_HW(pll1_d96_25p6),
> @@ -756,6 +777,10 @@ static struct clk_hw *k1_ccu_mpmu_hws[] = {
>  	[CLK_I2S_BCLK]		= &i2s_bclk.common.hw,
>  	[CLK_APB]		= &apb_clk.common.hw,
>  	[CLK_WDT_BUS]		= &wdt_bus_clk.common.hw,
> +	[CLK_I2S_153P6]		= &i2s_153p6.common.hw,
> +	[CLK_I2S_153P6_BASE]	= &i2s_153p6_base.common.hw,
> +	[CLK_I2S_SYSCLK_SRC]	= &i2s_sysclk_src.common.hw,
> +	[CLK_I2S_BCLK_FACTOR]	= &i2s_bclk_factor.common.hw,
>  };
>  
>  static const struct spacemit_ccu_data k1_ccu_mpmu_data = {

Best regards,
Haylen Chu