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

[Troy Mitchell]

On Fri, Aug 08, 2025 at 03:59:12AM +0000, Haylen Chu wrote:
> 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? 
I think this a bit troublesome...

> Not to mention this DDN variant actually
> duplicates the code in ccu_mix.c.
>
So I’ve ultimately decided not to introduce DDN_GATE.
I’ll change the macro for i2s_sysclk_src from
CCU_MUX_DEFINE to CCU_MUX_GATE_DEFINE.

What do you think? From the clock tree perspective, this should be fine.
> 
> > > 
> > > > 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?
yes, It solves.

> 
> > 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?
I will confirm this point.

If I send v2 without removing the CCU_DIV_TABLE_GATE_DEFINE macro,
that would mean K3 really needs it.

> 
> > 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. 
Sry I ignore this..

But isn’t the POSR prefix already sufficient to indicate the relationship?

Have a nice day!

                - Troy

> 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
>