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 02:34:00PM +0800, Troy Mitchell wrote:
> 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...

I don't agree. It just costs one extra line and one extra macro.

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

Thanks for the change, it's fine for me, too.

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

Thanks, this will help.

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

I think the original form is easier to read, isn't it? Even if you don't
think so, this change obviously exceeds scope of this patch and please
separate a series for discussion.

> Have a nice day!
> 
>                 - Troy

Best regards,
Haylen Chu

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