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] [day] [month] [year] [list]

Message-ID: <2CB30FE47E82A003+aKMlsDlLnLqh2fC0@troy-wujie14pro-arch>
Date: Mon, 18 Aug 2025 21:08:00 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Haylen Chu <heylenay@....org>,
	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

Hi Haylen,

I accept all ur suggestions and I'll send v4 ASAP.

Thanks for ur review!

                - Troy

On Mon, Aug 18, 2025 at 10:04:18AM +0000, Haylen Chu wrote:
> 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
>