Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Thomas,

> Add a new clock provider for ARM clock domain. This clock provider
> is composed of multiple components which include mux_core, div_core,
> div_core2, div_corem0, div_corem1, div_periph, div_atb, div_pclk_dbg,
> div_copy and div_hpm. This composition of mutiple components into
> a single clock provider helps with faster completion of CPU clock
> speed switching during DVFS operations.
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos4.c |   96
> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
> insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c
> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -108,8 +108,11 @@
>  #define APLL_CON0		0x14100
>  #define E4210_MPLL_CON0		0x14108
>  #define SRC_CPU			0x14200
> +#define STAT_CPU		0x14400
>  #define DIV_CPU0		0x14500
>  #define DIV_CPU1		0x14504
> +#define DIV_STAT_CPU0		0x14600
> +#define DIV_STAT_CPU1		0x14604
>  #define GATE_SCLK_CPU		0x14800
>  #define GATE_IP_CPU		0x14900
>  #define E4X12_DIV_ISP0		0x18300
> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
> __initdata = { };
>  
>  /* list of all parent clock list */
> -PNAME(mout_apll_p)	= { "fin_pll", "fout_apll", };
> +PNAME(mout_apll_p)	= { "fin_pll", "fout_apll1", };
>  PNAME(mout_mpll_p)	= { "fin_pll", "fout_mpll", };
>  PNAME(mout_epll_p)	= { "fin_pll", "fout_epll", };
>  PNAME(mout_vpllsrc_p)	= { "fin_pll", "sclk_hdmi24m", };
> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133", "aclk160", };
>  PNAME(mout_onenand1_p) = {"mout_onenand", "sclk_vpll", };
>  
>  /* Exynos 4210-specific parent groups */
> +PNAME(armclk_p) = { "fout_apll", };

Here you only give no parent clock, but at samsung_coreclk_register()
it is expected to provide list of parents.

>  PNAME(sclk_vpll_p4210)	= { "mout_vpllsrc", "fout_vpll", };
>  PNAME(mout_core_p4210)	= { "mout_apll", "sclk_mpll", };
>  PNAME(sclk_ampll_p4210)	= { "sclk_mpll", "sclk_apll", };
> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0, NULL),
>  };
>  
> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
> corem0) \
> +		((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
> +		(periph << 12) | (corem1 << 8) | (corem0 << 4))
> +#define EXYNOS4210_DIV_CPU1(hpm, copy)	\
> +		((hpm << 4) | (copy << 0))
> +static const unsigned long exynos4210_armclk_data[][2] = {
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 5), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 4), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 3), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 3), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 3), },
> +	{ EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
> EXYNOS4210_DIV_CPU1(0, 3), }, +};
> +

What do you think about adding those parameters (like CPU dividers) as
an attribute to /cpus/cpu@0 node?

> +static const unsigned long exynos4210_armclk_freqs[] = {
> +	1200000 , 1000000, 800000, 500000, 400000, 200000,
> +};

Those freq's are going to be defined at /cpus/cpu@0 at operating-points
attribute (or if possible took from cpufreq_frequency table).

> +
> +static const struct samsung_core_clock_freq_table
> exyno4210_armclk_table = {
> +	.freq		= exynos4210_armclk_freqs,
> +	.freq_count	= ARRAY_SIZE(exynos4210_armclk_freqs),
> +	.data		= (void *)exynos4210_armclk_data,
> +};
> +
> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
> long drate,
> +					unsigned long prate)
> +{
> +	struct samsung_core_clock *armclk;
> +	const struct samsung_core_clock_freq_table *freq_tbl;
> +	unsigned long *freq_data;
> +	unsigned long mux_reg, idx;
> +	void __iomem *base;
> +
> +	if (drate == prate)
> +		return 0;
> +
> +	armclk = container_of(hw, struct samsung_core_clock, hw);
> +	freq_tbl = armclk->freq_table;
> +	freq_data = (unsigned long *)freq_tbl->data;
> +	base = armclk->ctrl_base;
> +
> +	for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
> += 2)
> +		if ((freq_tbl->freq[idx] * 1000) == drate)
> +			break;
> +
> +	if (!armclk->fout_pll)
> +		armclk->fout_pll = __clk_lookup("fout_apll");\
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
	
I'm a bit confused here for two reasons. Please correct me if I'm wrong.

1. You go into this ->set_rate() because of calling clk_set_rate at
"arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c

In a Exynos4210 we have:
XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
-> div_core2 -> arm_clk

In the code you call directly the fout_apll which changes
frequency. Then the change shall be propagated to all registered
clocks.
I think, that DIV and DIV1 shall be reduced before PLL change [*],
to reflect the changes at CCF.	


> +
> +	if (drate < prate) {
> +		mux_reg = readl(base + SRC_CPU);
> +		writel(mux_reg | (1 << 16), base + SRC_CPU);
> +		while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
> +			;
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]

2. I think, the above shall be done in a following way:

	clk_set_parent(mout_core, mout_mpll);
	clk_set_rate(armclk->fout_pll, drate);
	clk_set_parent(mout_core, mout_apll);

The direct write to registers [**] doesn't look compliant to CCF.


I'd rather thought about using "mout_core" instead of "arm_clk". Then we
would get access to the parent directly:

	struct clk *parent = clk_get_parent(hw->clk);

so we set the parents explicitly (at clk registration) and call
->recalc_rate for clocks which are lower in the tree (like "div_core",
"div_core2").

> +		clk_set_rate(armclk->fout_pll, drate);
> +	}
> +
> +	writel(freq_data[0], base + DIV_CPU0);
> +	while (readl(base + DIV_STAT_CPU0) != 0)
> +		;
> +	writel(freq_data[1], base + DIV_CPU1);
> +	while (readl(base + DIV_STAT_CPU1) != 0)
> +		;
> +
> +	if (drate > prate) {
> +		mux_reg = readl(base + SRC_CPU);
> +		writel(mux_reg | (1 << 16), base + SRC_CPU);
> +		while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
> +			;
> +
> +		clk_set_rate(armclk->fout_pll, drate);
> +	}
> +
> +	mux_reg = readl(base + SRC_CPU);
> +	writel(mux_reg & ~(1 << 16), base + SRC_CPU);
> +	while (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
> +			;
> +	return 0;
> +}
> +
> +static const struct clk_ops exynos4210_armclk_clk_ops = {
> +	.recalc_rate = samsung_core_clock_recalc_rate,
> +	.round_rate = samsung_core_clk_round_rate,
> +	.set_rate = exynos4210_armclk_set_rate,
> +};
> +
>  /* register exynos4 clocks */
>  static void __init exynos4_clk_init(struct device_node *np,
>  				    enum exynos4_soc exynos4_soc,
> @@ -1164,6 +1254,10 @@ static void __init exynos4_clk_init(struct
> device_node *np, ARRAY_SIZE(exynos4210_gate_clks));
>  		samsung_clk_register_alias(exynos4210_aliases,
>  			ARRAY_SIZE(exynos4210_aliases));
> +		samsung_coreclk_register("armclk", armclk_p,
> +			ARRAY_SIZE(armclk_p), "fout_apll",
> +			&exynos4210_armclk_clk_ops, arm_clk,
> +			&exyno4210_armclk_table);
>  	} else {
>  		samsung_clk_register_mux(exynos4x12_mux_clks,
>  			ARRAY_SIZE(exynos4x12_mux_clks));



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group