Re: [PATCH 2/2] clk: spacemit: introduce i2s pre-clock and fix i2s clock

[Haylen Chu <heylenay@4d2.org>]

On Fri, Aug 08, 2025 at 10:10:48AM +0800, Troy Mitchell wrote:
> Hi, Haylen!
> 
> On Thu, Aug 07, 2025 at 03:02:00AM +0000, Haylen Chu wrote:
> > On Thu, Aug 07, 2025 at 09:30:11AM +0800, Troy Mitchell wrote:
> > > Defining i2s_bclk and i2s_sysclk as fixed-rate clocks is insufficient
> > > for real I2S use cases.
> > 
> > This is a little misleading: they're modeled as gates with fixed-factor
> > for now whose rate is calculated from their parents instead of defined
> > statically. You could avoid possible confusion by replacing "fixed-rate"
> > with "fixed-factor".
> >
> I'll change it in next version.
> 
> > 
> > > 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.
> > > 
> > > The i2s_sysclk behaves as an M/N fractional divider in hardware,
> > > with an additional gate control.
> > > 
> > > To properly model this, CCU_DDN_GATE_DEFINE is introduced.
> > 
> > Could it be represented as a DDN clock taking a gate as parent? Just
> > like what is described in the published clock diagram. Generally I'd
> > like to avoid introducing more clock types, there're already a lot.
> Uh, our new chip(K3) may uses this macro that I introduced..
> so I don't wanna take a gate as parent everywhere..
> how about we leave it? ;)

I wasn't proposing a workaround. What will go wrong if a gate is taken
as parent of DDN everywhere? Not to mention this DDN variant actually
duplicates the code in ccu_mix.c.

> > 
> > > The original DDN operations applied an implicit divide-by-2, which should
> > > not be a default behavior.
> > > 
> > > This patch removes that assumption, letting each clock define its
> > > actual behavior explicitly.
> > > 
> > > The i2s_bclk is a non-linear, discrete divider clock.
> > > The previous macro only supported linear dividers,
> > > so CCU_DIV_TABLE_GATE_DEFINE is introduced to support
> > > the hardware accurately.
> > 
> > The divider IS linear, from the perspective of functionality, it just
> > implies a 1/2 factor. Could it be represented as an usual divider and a
> > 1/2 fixed factor?
> ditto.
> 
> I know you don't wanna introduce new macro..

It's not about new macros. It's about new clock types. And the solution
I proposed for the divider with a factor isn't meant to be a workaround.

For the divider's case, I think combining a fixed-factor and a normal
divider looks more clean than introducing a new LUT. It solves the
problem for K1, right?

> But K3 requires this, so whether it is introduced now or future,
> the final result is the same.

Could you please confirm whether K3's dividers requiring this patch are
really non-linear or just imply a fixed-factor?

> Please leave it..
> > 
> > > The I2S-related clock registers can be found here [1].
> > 
> > So this patch actually does four separate things,
> > 
> > - Introduce a gate-capable variant of DDN clocks
> > - Make the pre-divider of DDN clocks flexible
> > - Support looking up mappings between register values and divisors
> >   through a table when calculating rates of dividers
> > - Fix the definition of i2s_bclk and i2s_sysclk
> > 
> > IMHO it's better to split them into separate patches for clearness.
> Ok, I will split them into separate patches.

Thanks, that'll be easier to review.

> ...
> > 
> > ...
> > 
> > > diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
> > > index c59bd7a38e5b4219121341b9c0d9ffda13a9c3e2..253db8a602fe43a1109e2ba248af11109c7baa22 100644
> > > --- a/include/soc/spacemit/k1-syscon.h
> > > +++ b/include/soc/spacemit/k1-syscon.h
> > > @@ -29,10 +29,11 @@ to_spacemit_ccu_adev(struct auxiliary_device *adev)
> > >  #define APBS_PLL3_SWCR3			0x12c
> > >  
> > >  /* MPMU register offset */
> > > +#define MPMU_FCCR			0x0008
> > >  #define MPMU_POSR			0x0010
> > > -#define  POSR_PLL1_LOCK			BIT(27)
> > > -#define  POSR_PLL2_LOCK			BIT(28)
> > > -#define  POSR_PLL3_LOCK			BIT(29)
> > > +#define POSR_PLL1_LOCK			BIT(27)
> > > +#define POSR_PLL2_LOCK			BIT(28)
> > > +#define POSR_PLL3_LOCK			BIT(29)
> > 
> > This reformatting doesn't seem related to the patch.
> It's worth that create a new commit to reformatting it?

IIRC, the indentation is intended to show the relationship between
register bits and offsets, which seems easier to read for me. Do you
have a good reason to change it?

>                 - Troy
> > 
> > >  #define MPMU_SUCCR			0x0014
> > >  #define MPMU_ISCCR			0x0044
> > >  #define MPMU_WDTPCR			0x0200
> > > 
> > > -- 
> > > 2.50.1
> > > 
> > 
> > Best regards,
> > Haylen Chu
> > 

Best regards,
Haylen Chu