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

[Naveen Krishna Ch <naveenkrishna.ch@gmail.com>]

Hi Tomasz,

On 27 August 2014 18:05, Tomasz Figa <t.figa@samsung.com> wrote:
> 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?

Ok. Will fix.

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

Ok.

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

Ok. Thanks for the pointer.

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

These rates are from the user manual. I will check in detail about these rates.

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

Ok. Fixed.

Thanks.

>
> Best regards,
> Tomasz

-- 
Shine bright,
(: Nav :)