Re: [PATCH v4 3/4] clk: spacemit: Add clock support for Spacemit K1 SoC

[Haylen Chu]

Hi Alex,

Before answering the reply, I'd like to share some information on these
unconfirmed questions.

On Sun, Feb 16, 2025 at 11:34:06AM +0000, Haylen Chu wrote:
> On Thu, Feb 13, 2025 at 10:04:10PM -0600, Alex Elder wrote:
> > On 1/3/25 3:56 PM, Haylen Chu wrote:
> > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > > new file mode 100644
> > > index 000000000000..6fb0a12ec261
> > > --- /dev/null
> > > +++ b/drivers/clk/spacemit/ccu-k1.c

...

> The next clock is weird, and it's the only one of its kind.  It is not
> > represented in the clock tree diagram. It is a "factor 1" clock (so it
> > just passes the parent's rate through), and has no gate.  Do you know
> > why it's defined?  It is used only as one of the MPMU parent clocks.
> > Why isn't just the pll1_d7 clock used in its place?
> 
> It is represented in the diagram. The photo version of the diagram seems
> hard to search so I will ask the vendor to publish a PDF version if
> possible.
> 
> As the definition involves no hardware bits, I guess it's actually an
> alias listed to keep the tree structure in similar form. Will confirm
> this with the vendor.

Yes, it's confirmed as a placeholder.

> 
> > > +static CCU_FACTOR_DEFINE(pll1_d7_351p08, "pll1_d7_351p08", CCU_PARENT_HW(pll1_d7),
> > > +			 1, 1);
> > > +
> > > +static CCU_GATE_DEFINE(pll1_d6_409p6, "pll1_d6_409p6", CCU_PARENT_HW(pll1_d6),
> > > +		       MPMU_ACGR,
> > > +		       BIT(0), BIT(0), 0, 0);
> > > +static CCU_GATE_FACTOR_DEFINE(pll1_d12_204p8, "pll1_d12_204p8", CCU_PARENT_HW(pll1_d6),
> > > +			      MPMU_ACGR,
> > > +			      BIT(5), BIT(5), 0, 2, 1, 0);
> > > +
> > > +static CCU_GATE_DEFINE(pll1_d5_491p52, "pll1_d5_491p52", CCU_PARENT_HW(pll1_d5),
> > > +		       MPMU_ACGR,
> > > +		       BIT(21), BIT(21), 0, 0);
> > > +static CCU_GATE_FACTOR_DEFINE(pll1_d10_245p76, "pll1_d10_245p76", CCU_PARENT_HW(pll1_d5),
> > > +			      MPMU_ACGR,
> > > +			      BIT(18), BIT(18), 0, 2, 1, 0);
> > > +
> > > +static CCU_GATE_DEFINE(pll1_d4_614p4, "pll1_d4_614p4", CCU_PARENT_HW(pll1_d4),
> > > +		       MPMU_ACGR,
> > > +		       BIT(15), BIT(15), 0, 0);
> > > +static CCU_GATE_FACTOR_DEFINE(pll1_d52_47p26, "pll1_d52_47p26", CCU_PARENT_HW(pll1_d4),
> > > +			      MPMU_ACGR,
> > > +			      BIT(10), BIT(10), 0, 13, 1, 0);
> > > +static CCU_GATE_FACTOR_DEFINE(pll1_d78_31p5, "pll1_d78_31p5", CCU_PARENT_HW(pll1_d4),
> > > +			      MPMU_ACGR,
> > > +			      BIT(6), BIT(6), 0, 39, 2, 0);
> > > +
> > > +static CCU_GATE_DEFINE(pll1_d3_819p2, "pll1_d3_819p2", CCU_PARENT_HW(pll1_d3),
> > > +		       MPMU_ACGR,
> > > +		       BIT(14), BIT(14), 0, 0);
> > > +
> > > +static CCU_GATE_DEFINE(pll1_d2_1228p8, "pll1_d2_1228p8", CCU_PARENT_HW(pll1_d2),
> > > +		       MPMU_ACGR,
> > > +		       BIT(16), BIT(16), 0, 0);

...

> > I couldn't find the "ripc_clk" on the clock tree diagram.  It is
> > never used elsewhere, so I think this definition can go away.
> 
> I'm not sure whether the ripc_clk doesn't exist or it's just missing in
> both datasheet and clock tree diagram. Will confirm with the vendor.

It's just missing in the datasheet and clock tree diagram and now they
have been completed[1].

> > > +static CCU_GATE_DEFINE(ripc_clk, "ripc_clk", CCU_PARENT_NAME(vctcxo_24m),
> > > +		       MPMU_RIPCCR,
> > > +		       0x3, 0x3, 0x0,
> > > +		       0);
> > > +

....

> > > +static const struct clk_parent_data dpubit_parents[] = {
> > > +	CCU_PARENT_HW(pll1_d3_819p2),
> > > +	CCU_PARENT_HW(pll2_d2),
> > > +	CCU_PARENT_HW(pll2_d3),
> > > +	CCU_PARENT_HW(pll1_d2_1228p8),
> > > +	CCU_PARENT_HW(pll2_d4),
> > > +	CCU_PARENT_HW(pll2_d5),
> > 
> > The next two parent clocks are duplicates.  It looks this way on the
> > clock tree diagram as well.  Is this correct? Can you find out from
> > SpacemiT whether one of them is actually a different clock (like
> > pll2_d6 or something)?  It makes no sense to have two multiplexed
> > parent clocks with the same source.
> 
> Yes, will confirm it later. The register description[2] suggests it's
> wrong (there aren't two configuration for MIPI_BIT_CLK_SEL resulting in
> the same frequency).

The clock tree diagram (and the vendor driver) were wrong. The 7th.
parent should be pll2_d7 (427MHz * 7 is roughly 3GHz, which is PLL2's
frequency). The diagram has been corrected.

> > > +	CCU_PARENT_HW(pll2_d8),
> > > +	CCU_PARENT_HW(pll2_d8),
> > > +};
> > > +static CCU_DIV_FC_MUX_GATE_DEFINE(dpu_bit_clk, "dpu_bit_clk", dpubit_parents,
> > > +				  APMU_LCD_CLK_RES_CTRL1,
> > > +				  17, 3, BIT(31),
> > > +				  20, 3, BIT(16), BIT(16), 0x0,
> > > +				  0);
> > > +

...

> > > diff --git a/drivers/clk/spacemit/ccu_ddn.c b/drivers/clk/spacemit/ccu_ddn.c
> > > new file mode 100644
> > > index 000000000000..1df555888ecb
> > > --- /dev/null
> > > +++ b/drivers/clk/spacemit/ccu_ddn.c
> > > @@ -0,0 +1,140 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Spacemit clock type ddn
> > > + *
> > > + * Copyright (c) 2024 SpacemiT Technology Co. Ltd
> > > + * Copyright (c) 2024 Haylen Chu <heylenay@xxxxxxx>
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +
> > > +#include "ccu_ddn.h"
> > 
> > What does "DDN" stand for?
> 
> I'm not sure, the name is kept from the vendor driver. I could change it
> to a more descriptive name, like "fraction-factor".

It's abbreviated from "Divider Denominator Numerator", confirmed by the
vendor. Quite weird a name. I'll make the abbreviation and corresponding
explanation more clear in the next revision.

Thanks,
Haylen Chu

[1]: https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part208