Re: [RFC 0/2] clk: samsung: add composite clocks

Re: [RFC 0/2] clk: samsung: add composite clocks

[Heiko Stübner]

Am Montag, 20. Mai 2013, 16:17:06 schrieb Rahul Sharma:
> This patch adds support for composite clocks for samsung SoCs.
> Many drivers need access to a common clock which support gating
> and/or muxing and/or rate control operations. For example hdmi
> which needs to switch between parents and call enable/disable for
> "sclk_hdmi".
> 
> This patch set also adds composite clock for exyno5250 hdmi. Based
> on the review comment, I will extended this to other exynos SoCs
> clocks files.

I think I remember reading somewhere that the target of the common clock 
framework was to prevent every SoC from introducing their own special clock 
types and instead  create these structures from separate clocks (mux clk + 
gate clk) and not to have every SoC create their own custom clock types.

The Samsung clock drivers at the moment follow this paradigm of combining the 
existing "simple" clocks and only introduce new clock types for the pll 
clocks, that really need special handling.

So it would probably good to keep it this way and define your clocks from 
their individual components, as all the other Samsung clocks currently do.


Heiko

[RFC 0/2] clk: samsung: add composite clocks

[Rahul Sharma]

This patch adds support for composite clocks for samsung SoCs.
Many drivers need access to a common clock which support gating
and/or muxing and/or rate control operations. For example hdmi
which needs to switch between parents and call enable/disable for
"sclk_hdmi".

This patch set also adds composite clock for exyno5250 hdmi. Based
on the review comment, I will extended this to other exynos SoCs
clocks files.

This patch series is based on for-next branch of tree:
git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git

Rahul Sharma (2):
  clk: samsung: add support for composite clocks
  clk: samsung: add exynos5250 composite clock for hdmi

 drivers/clk/samsung/clk-exynos5250.c |   20 ++++-
 drivers/clk/samsung/clk.c            |  149 ++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk.h            |   49 +++++++++++
 3 files changed, 215 insertions(+), 3 deletions(-)

-- 
1.7.10.4

[RFC 1/2] clk: samsung: add support for composite clocks

[Rahul Sharma]

Earlier to CCF, many drivers need access to a common clock which
support gating and/or muxing and/or rate control operations. For
example hdmi which needs to switch between parents and call
enable/disable for "sclk_hdmi".

This patch add support for composite clocks which address above
driver requirements wrt clocks. By using composite clocks, drivers
also need not be modified for different S0Cs. This will handled
inside CCF.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/clk/samsung/clk.c |  149 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk.h |   49 +++++++++++++++
 2 files changed, 198 insertions(+)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index cd3c40a..fa6ceb2 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -280,6 +280,155 @@ void __init samsung_clk_register_gate(struct samsung_gate_clock *list,
 	}
 }
 
+static void __samsung_clk_register_composite
+		(struct samsung_composite_clock *entry)
+{
+	struct clk *clk = NULL;
+	struct clk_mux *mux = NULL;
+	struct clk_gate *gate = NULL;
+	struct clk_divider *divider = NULL;
+	struct clk_fixed_rate *fixrate = NULL;
+	struct clk_fixed_factor *fixfactor = NULL;
+	struct clk_hw *mux_hw, *gate_hw, *rate_hw;
+	const struct clk_ops *mux_ops, *gate_ops, *rate_ops;
+	unsigned int cf, ret;
+
+	cf = entry->composition_flags;
+	mux_hw = NULL;
+	gate_hw = NULL;
+	rate_hw = NULL;
+	mux_ops = NULL;
+	gate_ops = NULL;
+	rate_ops = NULL;
+
+	/* register a mux clock, if specified */
+	if (cf & SAMSUNG_CLK_TYPE_MUX) {
+		mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+		if (!mux) {
+			pr_err("%s: fail to allocate mux clk %s\n",
+				__func__, entry->name);
+			goto fail_mux;
+		}
+		mux_hw = &mux->hw;
+		mux_ops = &clk_mux_ops;
+
+		mux->reg = reg_base + entry->mux_clk.offset;
+		mux->shift = entry->mux_clk.shift;
+		mux->mask = entry->mux_clk.width;
+		mux->flags = entry->mux_clk.mux_flags;
+		mux->lock = &lock;
+	}
+
+	/* register a gate clock, if specified */
+	if (cf & SAMSUNG_CLK_TYPE_GATE) {
+		gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+		if (!gate) {
+			pr_err("%s: fail to allocate gate clk %s\n",
+				__func__, entry->name);
+			goto fail_gate;
+		}
+		gate_hw = &gate->hw;
+		gate_ops = &clk_gate_ops;
+
+		gate->reg = reg_base + entry->gate_clk.offset;
+		gate->bit_idx = entry->gate_clk.bit_idx;
+		gate->flags = entry->gate_clk.gate_flags;
+		gate->lock = &lock;
+	}
+
+	/* register a rate clock, if specified */
+	if (cf & SAMSUNG_CLK_TYPE_DIVIDER) {
+		divider = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+		if (!divider) {
+			pr_err("%s: fail to allocate div clk %s\n",
+				__func__, entry->name);
+			goto fail_rate;
+		}
+		rate_hw = &divider->hw;
+		rate_ops = &clk_divider_ops;
+
+		divider->reg = reg_base + entry->div_clk.offset;
+		divider->shift = entry->div_clk.shift;
+		divider->width = entry->div_clk.width;
+		divider->flags = entry->div_clk.div_flags;
+
+	} else if (cf & SAMSUNG_CLK_TYPE_FIXED_FACTOR) {
+		fixfactor = kzalloc(sizeof(struct clk_fixed_factor),
+				GFP_KERNEL);
+		if (!fixfactor) {
+			pr_err("%s: fail to allocate fixfactor clk %s\n",
+				__func__, entry->name);
+			goto fail_rate;
+		}
+		rate_hw = &fixfactor->hw;
+		rate_ops = &clk_fixed_factor_ops;
+
+		fixfactor->mult = entry->fixed_factor_clk.mult;
+		fixfactor->div = entry->fixed_factor_clk.div;
+
+	} else if (cf & SAMSUNG_CLK_TYPE_FIXED_RATE) {
+		fixrate = kzalloc(sizeof(struct clk_fixed_rate),
+				GFP_KERNEL);
+		if (!fixrate) {
+			pr_err("%s: fail to allocate fixrate clk %s\n",
+				__func__, entry->name);
+			goto fail_rate;
+		}
+		rate_hw = &fixrate->hw;
+		rate_ops = &clk_fixed_rate_ops;
+
+		fixrate->fixed_rate = entry->fixed_rate_clk.fixed_rate;
+	}
+
+	clk = clk_register_composite(NULL, entry->name,
+			entry->parent_names, entry->num_parents,
+			mux_hw, mux_ops,
+			rate_hw, rate_ops,
+			gate_hw, gate_ops,
+			CLK_IS_ROOT);
+	if (IS_ERR(clk)) {
+		pr_err("%s: failed to register clock %s\n", __func__,
+			entry->name);
+		goto fail_clk_register;
+	}
+
+	/* register a clock lookup only if a clock alias is specified */
+	if (entry->alias) {
+		ret = clk_register_clkdev(clk, entry->alias,
+			entry->dev_name);
+		if (ret)
+			pr_err("%s: failed to register lookup %s\n",
+					__func__, entry->alias);
+		goto fail_clk_register;
+	}
+
+	samsung_clk_add_lookup(clk, entry->id);
+	return;
+
+fail_clk_register:
+	kfree(divider);
+	kfree(fixfactor);
+	kfree(fixrate);
+fail_rate:
+	kfree(gate);
+fail_gate:
+	kfree(mux);
+fail_mux:
+	pr_err("%s: failed to register composite clock %s\n",
+			__func__, entry->name);
+	return;
+}
+
+/* register a list of composite clocks */
+void __init samsung_clk_register_composite(struct samsung_composite_clock *list,
+						unsigned int nr_clk)
+{
+	unsigned int idx;
+
+	for (idx = 0; idx < nr_clk; idx++, list++)
+		__samsung_clk_register_composite(list);
+}
+
 /*
  * obtain the clock speed of all external fixed clock sources from device
  * tree and register it
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index e4ad6ea..d52ada2 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -20,6 +20,16 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 
+/*
+ * flags used represent the type of clock.
+ */
+
+#define SAMSUNG_CLK_TYPE_GATE		BIT(0)
+#define SAMSUNG_CLK_TYPE_MUX		BIT(1)
+#define SAMSUNG_CLK_TYPE_DIVIDER	BIT(2)
+#define SAMSUNG_CLK_TYPE_FIXED_FACTOR	BIT(3)
+#define SAMSUNG_CLK_TYPE_FIXED_RATE	BIT(4)
+
 /**
  * struct samsung_clock_alias: information about mux clock
  * @id: platform specific id of the clock.
@@ -249,6 +259,43 @@ struct samsung_gate_clock {
 #define PNAME(x) static const char *x[] __initdata
 
 /**
+ * struct samsung_composite_clock: information about composite clock
+ * @id: platform specific id of the clock.
+ * @dev_name: name of the device to which this clock belongs.
+ * @name: name of this mux clock.
+ * @parent_names: array of pointer to parent clock names.
+ * @num_parents: number of parents listed in @parent_names.
+ * @flags: optional flags for basic clock.
+ * @alias: optional clock alias name to be assigned to this clock.
+ * @composition_flags: mandatory flags represent the components this clock.
+ * @gate_clk: optional component clock of type gate.
+ * @mux_clk: optional component clock of type mux.
+ * @div_clk:  optional component clock of type divider.
+ * @fixed_factor_clk: optional component clock of type fixed factor.
+ * @fixed_rate_clk: optional component clock of type fixed rate.
+ */
+
+struct samsung_composite_clock {
+	unsigned int		id;
+	const char		*dev_name;
+	const char		*name;
+	const char		**parent_names;
+	u8			num_parents;
+	unsigned long		flags;
+	const char		*alias;
+	unsigned long		composition_flags;
+
+	/* gate clock */
+	struct samsung_gate_clock		gate_clk;
+	/* mux clock */
+	struct samsung_mux_clock		mux_clk;
+	/* rate clocks */
+	struct samsung_div_clock		div_clk;
+	struct samsung_fixed_factor_clock	fixed_factor_clk;
+	struct samsung_fixed_rate_clock		fixed_rate_clk;
+};
+
+/**
  * struct samsung_clk_reg_dump: register dump of clock controller registers.
  * @offset: clock register offset from the controller base address.
  * @value: the value to be register at offset.
@@ -281,6 +328,8 @@ extern void __init samsung_clk_register_div(struct samsung_div_clock *clk_list,
 		unsigned int nr_clk);
 extern void __init samsung_clk_register_gate(
 		struct samsung_gate_clock *clk_list, unsigned int nr_clk);
+extern void __init samsung_clk_register_composite(
+		struct samsung_composite_clock *list, unsigned int nr_clk);
 
 extern unsigned long _get_rate(const char *clk_name);
 
-- 
1.7.10.4

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

[Rahul Sharma]

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.

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@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"));
-- 
1.7.10.4

Re: [RFC 0/2] clk: samsung: add composite clocks

[Rahul Sharma]

On Mon, May 20, 2013 at 7:44 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Montag, 20. Mai 2013, 16:17:06 schrieb Rahul Sharma:
>> This patch adds support for composite clocks for samsung SoCs.
>> Many drivers need access to a common clock which support gating
>> and/or muxing and/or rate control operations. For example hdmi
>> which needs to switch between parents and call enable/disable for
>> "sclk_hdmi".
>>
>> This patch set also adds composite clock for exyno5250 hdmi. Based
>> on the review comment, I will extended this to other exynos SoCs
>> clocks files.
>
> I think I remember reading somewhere that the target of the common clock
> framework was to prevent every SoC from introducing their own special clock
> types and instead  create these structures from separate clocks (mux clk +
> gate clk) and not to have every SoC create their own custom clock types.
>
> The Samsung clock drivers at the moment follow this paradigm of combining the
> existing "simple" clocks and only introduce new clock types for the pll
> clocks, that really need special handling.
>
> So it would probably good to keep it this way and define your clocks from
> their individual components, as all the other Samsung clocks currently do.
>

Thanks Heiko, I agree, but I am not trying to introduce a new type here,
instead using the existing generic support for composite clocks for
exynos as well.

These have not been added for Samsung SoCs so far but I do not see any
harm in using them also. With them, drivers do not need to get and configure
each clock component separately. This ensures less and more reasonable
changes in the drivers during migration to CCF.

Please help me understand about the loss when using composite clocks.

regards,
Rahul Sharma

>
> Heiko

Re: [RFC 0/2] clk: samsung: add composite clocks

[Heiko Stübner]

Am Montag, 20. Mai 2013, 20:19:29 schrieb Rahul Sharma:
> On Mon, May 20, 2013 at 7:44 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Montag, 20. Mai 2013, 16:17:06 schrieb Rahul Sharma:
> >> This patch adds support for composite clocks for samsung SoCs.
> >> Many drivers need access to a common clock which support gating
> >> and/or muxing and/or rate control operations. For example hdmi
> >> which needs to switch between parents and call enable/disable for
> >> "sclk_hdmi".
> >> 
> >> This patch set also adds composite clock for exyno5250 hdmi. Based
> >> on the review comment, I will extended this to other exynos SoCs
> >> clocks files.
> > 
> > I think I remember reading somewhere that the target of the common clock
> > framework was to prevent every SoC from introducing their own special
> > clock types and instead  create these structures from separate clocks
> > (mux clk + gate clk) and not to have every SoC create their own custom
> > clock types.
> > 
> > The Samsung clock drivers at the moment follow this paradigm of combining
> > the existing "simple" clocks and only introduce new clock types for the
> > pll clocks, that really need special handling.
> > 
> > So it would probably good to keep it this way and define your clocks from
> > their individual components, as all the other Samsung clocks currently
> > do.
> 
> Thanks Heiko, I agree, but I am not trying to introduce a new type here,
> instead using the existing generic support for composite clocks for
> exynos as well.
> 
> These have not been added for Samsung SoCs so far but I do not see any
> harm in using them also. With them, drivers do not need to get and
> configure each clock component separately. This ensures less and more
> reasonable changes in the drivers during migration to CCF.
> 
> Please help me understand about the loss when using composite clocks.

hehe ... it seems I remembered something outdated ...

The last time (before march 12th) I looked at the ccf, it was "use simple 
clocktypes". But it seems in the meantime the separate composite-clock you use 
was introduced. And I didn't read your patch careful enough to see that you're 
using the (now) existing composite clock.

So, sorry for the noise :-)


Heiko

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

[Tomasz Figa]

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()?

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

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@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"));

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

[Saravana Kannan]

On 05/20/2013 11:57 AM, Tomasz Figa 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()?
>
> 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
> 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.
>

+1 to all these comments.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [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@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@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"));

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

[Tomasz Figa]

Hi,

Cc'ing Mike, clock subsystem maintainer.

On Tuesday 21 of May 2013 09:42:25 Rahul Sharma wrote:
> On Tue, May 21, 2013 at 12:27 AM, Tomasz Figa <tomasz.figa@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.

Such functionality is not available at the moment, but it shouldn't be a 
problem to extend the core to handle that and in addition I believe it 
would be useful for other platforms as well.

> 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

No.

You can call clk_set_rate() on gate clocks, if they have 
CLK_SET_RATE_PARENT flag set.

I think similar feature could be added for clk_set_parent(). Mike, what do 
you think?

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

I don't think this is a problem at all.

Actually many drivers actually don't need any mux reconfiguration at all, 
just one time initialization of parents, that should be rather done in 
clock driver based on some SoC-/board-specific data. We're already working 
on a solution for this problem.

Anyway, I think it is already way too late for such deep reorganization. 
Clock device tree bindings have been already defined, so all the clock 
numbers that are currently defined must be left supported, because device 
tree bindings are considered ABI.

Best regards,
Tomasz

> 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@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"));