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

[Troy Mitchell]

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? ;)

> 
> > 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..
But K3 requires this, so whether it is introduced now or future,
the final result is the same.

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.

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

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