Re: [PATCH 04/14] clk: samsung: Add clock description for basic CMU blocks

[Tomasz Figa <t.figa@samsung.com>]

On 27.08.2014 11:48, Naveen Krishna Chatradhi wrote:
> This patch adds clock description for MUX, DIV, GATE and PLL
> clocks available in TOPC, TOP0, TOP1, BUS0, BUS1, FSYS0, FSYS1,
> CCORE, PERIC0, PERIC1, PERIS, DISP, G3D, MSCL and MFC blocks.

[snip]

> +Phy clocks:
> +
> +There are several clocks which are generated by specific PHYs.
> +These clocks are fed into the clock controller and then routed to
> +the hardware blocks. These clocks are defined as fixed clocks in the
> +driver with following names:
> +

The names are missing?

> +Required Properties for Clock Controller:
> +
> + - compatible: should be one of the following.
> +	1) "samsung,exynos7-clock-topc"
> +	2) "samsung,exynos7-clock-top0"
> +	3) "samsung,exynos7-clock-top1"
> +	4) "samsung,exynos7-clock-atlas"
> +	5) "samsung,exynos7-clock-ccore"

[snip]

> + - clocks: list of clock identifiers which are fed as the input to
> +	the given clock controller. Please refer the next section to find
> +	the input clocks for a given controller.
> +
> + - clock-names: list of names of clocks which are fed as the input
> +	to the given clock controller.

In the dtsi file added by patch 11/14, I don't see those being specified.

> +
> +Input clocks for topc clock controller:
> +	- fin_pll
> +	- fout_aud_pll

[snip]

> +/* PMS values for PLL 1451x */
> +static const struct samsung_pll_rate_table pll1451x_24mhz_tbl[] = {
> +	/* rate, m, p, s */
> +	PLL_35XX_RATE(660000000, 165, 3, 1),

This array needs to be sorted by output rate. However you might want to
rebase this series onto series [1] to eliminate this requirement. Be
aware that mentioned series might need a respin, though.

Also the rates seem to be too nicely rounded. Are they the real values
that can be obtained using the PLL equation (what is required by the PLL
framework) or a copy paste from the documentation? Similarly, series [1]
lessens the requirement a bit, due to recalculation of rates from
specified coefficients and warning about incorrect entries.

[1]
https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg35188.html

> +	PLL_35XX_RATE(1800000000, 150, 2, 0),
> +	PLL_35XX_RATE(1700000000, 425, 6, 0),
> +	PLL_35XX_RATE(1600000000, 200, 3, 0),
> +	PLL_35XX_RATE(1500000000, 125, 2, 0),
> +	PLL_35XX_RATE(1400000000, 175, 3, 0),
> +	PLL_35XX_RATE(1380000000, 115, 2, 0),
> +	PLL_35XX_RATE(1300000000, 325, 6, 0),
> +	PLL_35XX_RATE(1200000000, 100, 2, 0),
> +	PLL_35XX_RATE(1180000000, 295, 6, 0),
> +	PLL_35XX_RATE(1104000000, 276, 6, 0),

[snip]

> +	DIV(0, "dout_sclk_aud_pll", "mout_aud_pll_ctrl", DIV_TOPC3, 28, 3),
> +};
> +
> +static struct samsung_gate_clock topc_gate_clks[] __initdata = {
> +	GATE(ACLK_CCORE_532, "aclk_ccore_532", "dout_aclk_ccore_532",
> +		ENABLE_ACLK_TOPC0, 0, CLK_IGNORE_UNUSED, 0),

Why CLK_IGNORE_UNUSED? (The same question for all the gate clocks
defined with it in the driver.)

Best regards,
Tomasz