[RFC 2/2] clk: samsung: add exynos5250 composite clock for hdmi

[Rahul Sharma]

On Tue, May 21, 2013 at 12:27 AM, Tomasz Figa <tomasz.figa at gmail.com> wrote:
> Hi Rahul,
>
> On Monday 20 of May 2013 19:47:08 Rahul Sharma wrote:
>> HDMI driver needs to change the parent of sclk_hdmi clock to
>> sclk_pixel or to sclk_hdmiphy, depends on the status of hdmiphy.
>> sclk_hdmi which is gate clock doesn't support the set_parent
>> operation.
>
> Wouldn't it be better to simply allow calling clk_set_parent() on gate
> clocks and propagate parent change to nearest mux, just like it is done
> with clk_set_rate()?

Sorry, I din't get you completly here. Allowing clk_set_parent() on gate
clocks is like changing the inherent property of the gate clock. I dont see
it possible without overiding the default gate_ops (clk_gate_ops). Please
cite me the code/patch doing the same for clk_set_rate.

What I did here is rather simple and utilising the exisiting composite clock
framework for exynos (as well). I register comp. clock with default
gate/mux/rate operations. No customised clk type /ops in this patch.

>
> It wouldn't require any SoC-specific composite clocks and keep the nice
> property of the clock tree, which is built from basic, generic clock
> blocks that nicely correspond to blocks shown in the documentation.
>
> We had discussed this already at SRPOL and got to the conclusion that it's
> a step backwards, making the clock driver more complex, because each
> composite block would have to be described using a structure with many

I respectfully disagree with above. If we adhere to generic composite
clocks (in drivers/clk/clk-composite.c) we donot need to add different
struct for different blocks. I have further restricted the ops overriding in
drivers/clk/samsung/clk.c. Please refer the previous patch.

> fields. In addition there are many special cases, for which the composite
> scheme wouldn't work anyway and they would end up with simple clocks
> attached after the composite block, defeating the purpose of your patch.
>

Purpose of the patch is to avoid spilling complextiy of clk path/block to
all over the drivers. Just for instance hdmi/fimd needs 2 extra mux clocks
and 1 divider clock for set_parent and set_rate operations (other than
gating operation) which was not required before CCF and still avoidable.
I am sure, there will be many more similar cases. This list of exposed
clock IDs will keep explanding when all drivers migrate to CCF.

We have to take a call on this.

regards,
Rahul Sharma.

> Best regards,
> Tomasz
>
>> This patch adds sclk_hdmi as a composite clock which is a
>> combination of mux clock and gate clock. Being a composite
>> clock, above clock supports both set_parent and enable/disable
>> functionality. Therefore hdmi driver need not be modified
>> different S0Cs. This will handled inside CCF.
>>
>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos5250.c |   20 +++++++++++++++++---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5250.c
>> b/drivers/clk/samsung/clk-exynos5250.c index 5c97e75..0c9e37a 100644
>> --- a/drivers/clk/samsung/clk-exynos5250.c
>> +++ b/drivers/clk/samsung/clk-exynos5250.c
>> @@ -231,7 +231,6 @@ struct samsung_mux_clock exynos5250_mux_clks[]
>> __initdata = { MUX(none, "mout_fimd1", mout_group1_p, SRC_DISP1_0, 0,
>> 4),
>>       MUX(none, "mout_mipi1", mout_group1_p, SRC_DISP1_0, 12, 4),
>>       MUX(none, "mout_dp", mout_group1_p, SRC_DISP1_0, 16, 4),
>> -     MUX(none, "mout_hdmi", mout_hdmi_p, SRC_DISP1_0, 20, 1),
>>       MUX(none, "mout_audio0", mout_audio0_p, SRC_MAU, 0, 4),
>>       MUX(none, "mout_mmc0", mout_group1_p, SRC_FSYS, 0, 4),
>>       MUX(none, "mout_mmc1", mout_group1_p, SRC_FSYS, 4, 4),
>> @@ -416,8 +415,6 @@ struct samsung_gate_clock exynos5250_gate_clks[]
>> __initdata = { SRC_MASK_DISP1_0, 12, CLK_SET_RATE_PARENT, 0),
>>       GATE(sclk_dp, "sclk_dp", "div_dp",
>>                       SRC_MASK_DISP1_0, 16, CLK_SET_RATE_PARENT, 0),
>> -     GATE(sclk_hdmi, "sclk_hdmi", "mout_hdmi",
>> -                     SRC_MASK_DISP1_0, 20, 0, 0),
>>       GATE(sclk_audio0, "sclk_audio0", "div_audio0",
>>                       SRC_MASK_MAU, 0, CLK_SET_RATE_PARENT, 0),
>>       GATE(sclk_mmc0, "sclk_mmc0", "div_mmc_pre0",
>> @@ -464,6 +461,21 @@ struct samsung_gate_clock exynos5250_gate_clks[]
>> __initdata = { GATE(hdmi, "hdmi", "aclk200", GATE_IP_DISP1, 6, 0, 0),
>>  };
>>
>> +struct samsung_composite_clock exynos5250_composite_clks[] __initdata =
>> { +   {
>> +             .id = sclk_hdmi,
>> +             .name = "sclk_hdmi",
>> +             .parent_names = mout_hdmi_p,
>> +             .num_parents = ARRAY_SIZE(mout_hdmi_p),
>> +             .mux_clk = MUX(none, NULL, mout_hdmi_p, SRC_DISP1_0, 20,
>> +                             1),
>> +             .gate_clk = GATE(none, NULL, NULL, SRC_MASK_DISP1_0, 20,
>> +                             0, 0),
>> +             .composition_flags = SAMSUNG_CLK_TYPE_GATE |
>> +                     SAMSUNG_CLK_TYPE_MUX,
>> +     },
>> +};
>> +
>>  static __initdata struct of_device_id ext_clk_match[] = {
>>       { .compatible = "samsung,clock-xxti", .data = (void *)0, },
>>       { },
>> @@ -515,6 +527,8 @@ void __init exynos5250_clk_init(struct device_node
>> *np) ARRAY_SIZE(exynos5250_div_clks));
>>       samsung_clk_register_gate(exynos5250_gate_clks,
>>                       ARRAY_SIZE(exynos5250_gate_clks));
>> +     samsung_clk_register_composite(exynos5250_composite_clks,
>> +                     ARRAY_SIZE(exynos5250_composite_clks));
>>
>>       pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
>>                       _get_rate("armclk"));