[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[MyungJoo Ham]

This patch allows clk in init_clocks[] and init_clocks_disable[] to use
clksrc_clk.clk in clksrcs[].

For example, clk "lcd" may need to set "sclk-fimd" as a parent if
the div and src settting of sclk-fimd changes and a user of "lcd" wants
to get the exact information about the rate. If "sclk-fimd" is set as a
parent of "lcd", then the clk framework can provide the wanted
information; otherwise, the change in "sclk-fimd" does not update "lcd"
properly.

To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
order to allow clk in init_clocks[] and init_clocks_disable[] may refer
one of clksrcs[] as its parent. (enum clksrc_refer)

For example, "lcd" may set "sclk-fimd" as a parent if the following line
replaces the line 878:

		.parent		= &clk_hclk_dsys.clk,

to

		.parent		= &clksrcs[_clksrc_sclk_fimd],

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv210/clock.c |  986 ++++++++++++++++++++++-------------------
 1 files changed, 520 insertions(+), 466 deletions(-)

diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
index aa2b1c5..e33c6ed 100644
--- a/arch/arm/mach-s5pv210/clock.c
+++ b/arch/arm/mach-s5pv210/clock.c
@@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
 	.reg_src	= { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
 };
 
+static struct clk *clkset_uart_list[] = {
+	[6] = &clk_mout_mpll.clk,
+	[7] = &clk_mout_epll.clk,
+};
+
+static struct clksrc_sources clkset_uart = {
+	.sources	= clkset_uart_list,
+	.nr_sources	= ARRAY_SIZE(clkset_uart_list),
+};
+
+static struct clk *clkset_group1_list[] = {
+	[0] = &clk_sclk_a2m.clk,
+	[1] = &clk_mout_mpll.clk,
+	[2] = &clk_mout_epll.clk,
+	[3] = &clk_sclk_vpll.clk,
+};
+
+static struct clksrc_sources clkset_group1 = {
+	.sources	= clkset_group1_list,
+	.nr_sources	= ARRAY_SIZE(clkset_group1_list),
+};
+
+static struct clk *clkset_sclk_onenand_list[] = {
+	[0] = &clk_hclk_psys.clk,
+	[1] = &clk_hclk_dsys.clk,
+};
+
+static struct clksrc_sources clkset_sclk_onenand = {
+	.sources	= clkset_sclk_onenand_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_onenand_list),
+};
+
+static struct clk *clkset_sclk_dac_list[] = {
+	[0] = &clk_sclk_vpll.clk,
+	[1] = &clk_sclk_hdmiphy,
+};
+
+static struct clksrc_sources clkset_sclk_dac = {
+	.sources	= clkset_sclk_dac_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_dac_list),
+};
+
+static struct clksrc_clk clk_sclk_dac = {
+	.clk		= {
+		.name		= "sclk_dac",
+		.id		= -1,
+		.ctrlbit	= (1 << 2),
+		.enable		= s5pv210_clk_mask0_ctrl,
+	},
+	.sources	= &clkset_sclk_dac,
+	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 8, .size = 1 },
+};
+
+static struct clksrc_clk clk_sclk_pixel = {
+	.clk		= {
+		.name		= "sclk_pixel",
+		.id		= -1,
+		.parent		= &clk_sclk_vpll.clk,
+	},
+	.reg_div	= { .reg = S5P_CLK_DIV1, .shift = 0, .size = 4},
+};
+
+static struct clk *clkset_sclk_hdmi_list[] = {
+	[0] = &clk_sclk_pixel.clk,
+	[1] = &clk_sclk_hdmiphy,
+};
+
+static struct clksrc_sources clkset_sclk_hdmi = {
+	.sources	= clkset_sclk_hdmi_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_hdmi_list),
+};
+
+static struct clksrc_clk clk_sclk_hdmi = {
+	.clk		= {
+		.name		= "sclk_hdmi",
+		.id		= -1,
+		.ctrlbit	= (1 << 0),
+		.enable		= s5pv210_clk_mask0_ctrl,
+	},
+	.sources	= &clkset_sclk_hdmi,
+	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 0, .size = 1 },
+};
+
+static struct clk *clkset_sclk_mixer_list[] = {
+	[0] = &clk_sclk_dac.clk,
+	[1] = &clk_sclk_hdmi.clk,
+};
+
+static struct clksrc_sources clkset_sclk_mixer = {
+	.sources	= clkset_sclk_mixer_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_mixer_list),
+};
+
+static struct clk *clkset_sclk_audio0_list[] = {
+	[0] = &clk_ext_xtal_mux,
+	[1] = &clk_pcmcdclk0,
+	[2] = &clk_sclk_hdmi27m,
+	[3] = &clk_sclk_usbphy0,
+	[4] = &clk_sclk_usbphy1,
+	[5] = &clk_sclk_hdmiphy,
+	[6] = &clk_mout_mpll.clk,
+	[7] = &clk_mout_epll.clk,
+	[8] = &clk_sclk_vpll.clk,
+};
+
+static struct clksrc_sources clkset_sclk_audio0 = {
+	.sources	= clkset_sclk_audio0_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_audio0_list),
+};
+
+static struct clksrc_clk clk_sclk_audio0 = {
+	.clk		= {
+		.name		= "sclk_audio",
+		.id		= 0,
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 24),
+	},
+	.sources = &clkset_sclk_audio0,
+	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 0, .size = 4 },
+	.reg_div = { .reg = S5P_CLK_DIV6, .shift = 0, .size = 4 },
+};
+
+static struct clk *clkset_sclk_audio1_list[] = {
+	[0] = &clk_ext_xtal_mux,
+	[1] = &clk_pcmcdclk1,
+	[2] = &clk_sclk_hdmi27m,
+	[3] = &clk_sclk_usbphy0,
+	[4] = &clk_sclk_usbphy1,
+	[5] = &clk_sclk_hdmiphy,
+	[6] = &clk_mout_mpll.clk,
+	[7] = &clk_mout_epll.clk,
+	[8] = &clk_sclk_vpll.clk,
+};
+
+static struct clksrc_sources clkset_sclk_audio1 = {
+	.sources	= clkset_sclk_audio1_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_audio1_list),
+};
+
+static struct clksrc_clk clk_sclk_audio1 = {
+	.clk		= {
+		.name		= "sclk_audio",
+		.id		= 1,
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 25),
+	},
+	.sources = &clkset_sclk_audio1,
+	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 4, .size = 4 },
+	.reg_div = { .reg = S5P_CLK_DIV6, .shift = 4, .size = 4 },
+};
+
+static struct clk *clkset_sclk_audio2_list[] = {
+	[0] = &clk_ext_xtal_mux,
+	[1] = &clk_pcmcdclk0,
+	[2] = &clk_sclk_hdmi27m,
+	[3] = &clk_sclk_usbphy0,
+	[4] = &clk_sclk_usbphy1,
+	[5] = &clk_sclk_hdmiphy,
+	[6] = &clk_mout_mpll.clk,
+	[7] = &clk_mout_epll.clk,
+	[8] = &clk_sclk_vpll.clk,
+};
+
+static struct clksrc_sources clkset_sclk_audio2 = {
+	.sources	= clkset_sclk_audio2_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_audio2_list),
+};
+
+static struct clksrc_clk clk_sclk_audio2 = {
+	.clk		= {
+		.name		= "sclk_audio",
+		.id		= 2,
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 26),
+	},
+	.sources = &clkset_sclk_audio2,
+	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 8, .size = 4 },
+	.reg_div = { .reg = S5P_CLK_DIV6, .shift = 8, .size = 4 },
+};
+
+static struct clk *clkset_sclk_spdif_list[] = {
+	[0] = &clk_sclk_audio0.clk,
+	[1] = &clk_sclk_audio1.clk,
+	[2] = &clk_sclk_audio2.clk,
+};
+
+static struct clksrc_sources clkset_sclk_spdif = {
+	.sources	= clkset_sclk_spdif_list,
+	.nr_sources	= ARRAY_SIZE(clkset_sclk_spdif_list),
+};
+
+static struct clk *clkset_group2_list[] = {
+	[0] = &clk_ext_xtal_mux,
+	[1] = &clk_xusbxti,
+	[2] = &clk_sclk_hdmi27m,
+	[3] = &clk_sclk_usbphy0,
+	[4] = &clk_sclk_usbphy1,
+	[5] = &clk_sclk_hdmiphy,
+	[6] = &clk_mout_mpll.clk,
+	[7] = &clk_mout_epll.clk,
+	[8] = &clk_sclk_vpll.clk,
+};
+
+static struct clksrc_sources clkset_group2 = {
+	.sources	= clkset_group2_list,
+	.nr_sources	= ARRAY_SIZE(clkset_group2_list),
+};
+
+enum clksrc_refer {
+	_clksrc_sclk_dmc,
+	_clksrc_sclk_onenand,
+	_clksrc_uclk1_0,
+	_clksrc_uclk1_1,
+	_clksrc_uclk1_2,
+	_clksrc_uclk1_3,
+	_clksrc_sclk_mixer,
+	_clksrc_sclk_spdif,
+	_clksrc_sclk_fimc_0,
+	_clksrc_sclk_fimc_1,
+	_clksrc_sclk_fimc_2,
+	_clksrc_sclk_cam_0,
+	_clksrc_sclk_cam_1,
+	_clksrc_sclk_fimd,
+	_clksrc_mmc_bus_0,
+	_clksrc_mmc_bus_1,
+	_clksrc_mmc_bus_2,
+	_clksrc_mmc_bus_3,
+	_clksrc_sclk_mfc,
+	_clksrc_sclk_g2d,
+	_clksrc_sclk_g3d,
+	_clksrc_sclk_csis,
+	_clksrc_sclk_spi_0,
+	_clksrc_sclk_spi_1,
+	_clksrc_sclk_pwi,
+	_clksrc_sclk_pwm,
+};
+
+static struct clksrc_clk clksrcs[] = {
+	[_clksrc_sclk_dmc] = {
+		.clk	= {
+			.name		= "sclk_dmc",
+			.id		= -1,
+		},
+		.sources = &clkset_group1,
+		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
+		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
+	},
+	[_clksrc_sclk_onenand] = {
+		.clk	= {
+			.name		= "sclk_onenand",
+			.id		= -1,
+		},
+		.sources = &clkset_sclk_onenand,
+		.reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
+		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
+	},
+	[_clksrc_uclk1_0] = {
+		.clk	= {
+			.name		= "uclk1",
+			.id		= 0,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 12),
+		},
+		.sources = &clkset_uart,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 16, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 16, .size = 4 },
+	},
+	[_clksrc_uclk1_1] = {
+		.clk		= {
+			.name		= "uclk1",
+			.id		= 1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 13),
+		},
+		.sources = &clkset_uart,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 20, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 20, .size = 4 },
+	},
+	[_clksrc_uclk1_2] = {
+		.clk		= {
+			.name		= "uclk1",
+			.id		= 2,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 14),
+		},
+		.sources = &clkset_uart,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 24, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 24, .size = 4 },
+	},
+	[_clksrc_uclk1_3] = {
+		.clk		= {
+			.name		= "uclk1",
+			.id		= 3,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 15),
+		},
+		.sources = &clkset_uart,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 28, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 28, .size = 4 },
+	},
+	[_clksrc_sclk_mixer] = {
+		.clk	= {
+			.name		= "sclk_mixer",
+			.id		= -1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 1),
+		},
+		.sources = &clkset_sclk_mixer,
+		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 4, .size = 1 },
+	},
+	[_clksrc_sclk_spdif] = {
+		.clk		= {
+			.name		= "sclk_spdif",
+			.id		= -1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 27),
+		},
+		.sources = &clkset_sclk_spdif,
+		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 12, .size = 2 },
+	},
+	[_clksrc_sclk_fimc_0] = {
+		.clk	= {
+			.name		= "sclk_fimc",
+			.id		= 0,
+			.enable		= s5pv210_clk_mask1_ctrl,
+			.ctrlbit	= (1 << 2),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 12, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV3, .shift = 12, .size = 4 },
+	},
+	[_clksrc_sclk_fimc_1] = {
+		.clk	= {
+			.name		= "sclk_fimc",
+			.id		= 1,
+			.enable		= s5pv210_clk_mask1_ctrl,
+			.ctrlbit	= (1 << 3),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 16, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV3, .shift = 16, .size = 4 },
+	},
+	[_clksrc_sclk_fimc_2] = {
+		.clk	= {
+			.name		= "sclk_fimc",
+			.id		= 2,
+			.enable		= s5pv210_clk_mask1_ctrl,
+			.ctrlbit	= (1 << 4),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 20, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV3, .shift = 20, .size = 4 },
+	},
+	[_clksrc_sclk_cam_0] = {
+		.clk		= {
+			.name		= "sclk_cam",
+			.id		= 0,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 3),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 12, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 12, .size = 4 },
+	},
+	[_clksrc_sclk_cam_1] = {
+		.clk		= {
+			.name		= "sclk_cam",
+			.id		= 1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 4),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 16, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 16, .size = 4 },
+	},
+	[_clksrc_sclk_fimd] = {
+		.clk		= {
+			.name		= "sclk_fimd",
+			.id		= -1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 5),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 20, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 20, .size = 4 },
+	},
+	[_clksrc_mmc_bus_0] = {
+		.clk		= {
+			.name		= "sclk_mmc",
+			.id		= 0,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 8),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 0, .size = 4 },
+	},
+	[_clksrc_mmc_bus_1] = {
+		.clk		= {
+			.name		= "sclk_mmc",
+			.id		= 1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 9),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 4, .size = 4 },
+	},
+	[_clksrc_mmc_bus_2] = {
+		.clk		= {
+			.name		= "sclk_mmc",
+			.id		= 2,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 10),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 8, .size = 4 },
+	},
+	[_clksrc_mmc_bus_3] = {
+		.clk		= {
+			.name		= "sclk_mmc",
+			.id		= 3,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 11),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 12, .size = 4 },
+	},
+	[_clksrc_sclk_mfc] = {
+		.clk		= {
+			.name		= "sclk_mfc",
+			.id		= -1,
+			.enable		= s5pv210_clk_ip0_ctrl,
+			.ctrlbit	= (1 << 16),
+		},
+		.sources = &clkset_group1,
+		.reg_src = { .reg = S5P_CLK_SRC2, .shift = 4, .size = 2 },
+		.reg_div = { .reg = S5P_CLK_DIV2, .shift = 4, .size = 4 },
+	},
+	[_clksrc_sclk_g2d] = {
+		.clk		= {
+			.name		= "sclk_g2d",
+			.id		= -1,
+			.enable		= s5pv210_clk_ip0_ctrl,
+			.ctrlbit	= (1 << 12),
+		},
+		.sources = &clkset_group1,
+		.reg_src = { .reg = S5P_CLK_SRC2, .shift = 8, .size = 2 },
+		.reg_div = { .reg = S5P_CLK_DIV2, .shift = 8, .size = 4 },
+	},
+	[_clksrc_sclk_g3d] = {
+		.clk		= {
+			.name		= "sclk_g3d",
+			.id		= -1,
+			.enable		= s5pv210_clk_ip0_ctrl,
+			.ctrlbit	= (1 << 8),
+		},
+		.sources = &clkset_group1,
+		.reg_src = { .reg = S5P_CLK_SRC2, .shift = 0, .size = 2 },
+		.reg_div = { .reg = S5P_CLK_DIV2, .shift = 0, .size = 4 },
+	},
+	[_clksrc_sclk_csis] = {
+		.clk		= {
+			.name		= "sclk_csis",
+			.id		= -1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 6),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 24, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 28, .size = 4 },
+	},
+	[_clksrc_sclk_spi_0] = {
+		.clk		= {
+			.name		= "sclk_spi",
+			.id		= 0,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 16),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 0, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV5, .shift = 0, .size = 4 },
+	},
+	[_clksrc_sclk_spi_1] = {
+		.clk		= {
+			.name		= "sclk_spi",
+			.id		= 1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 17),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 4, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV5, .shift = 4, .size = 4 },
+	},
+	[_clksrc_sclk_pwi] = {
+		.clk		= {
+			.name		= "sclk_pwi",
+			.id		= -1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 29),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 20, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 24, .size = 4 },
+	},
+	[_clksrc_sclk_pwm] = {
+		.clk		= {
+			.name		= "sclk_pwm",
+			.id		= -1,
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 19),
+		},
+		.sources = &clkset_group2,
+		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 12, .size = 4 },
+		.reg_div = { .reg = S5P_CLK_DIV5, .shift = 12, .size = 4 },
+	},
+};
+
 static unsigned long s5pv210_clk_imem_get_rate(struct clk *clk)
 {
 	return clk_get_rate(clk->parent) / 2;
@@ -752,472 +1272,6 @@ static struct clk init_clocks[] = {
 	},
 };
 
-static struct clk *clkset_uart_list[] = {
-	[6] = &clk_mout_mpll.clk,
-	[7] = &clk_mout_epll.clk,
-};
-
-static struct clksrc_sources clkset_uart = {
-	.sources	= clkset_uart_list,
-	.nr_sources	= ARRAY_SIZE(clkset_uart_list),
-};
-
-static struct clk *clkset_group1_list[] = {
-	[0] = &clk_sclk_a2m.clk,
-	[1] = &clk_mout_mpll.clk,
-	[2] = &clk_mout_epll.clk,
-	[3] = &clk_sclk_vpll.clk,
-};
-
-static struct clksrc_sources clkset_group1 = {
-	.sources	= clkset_group1_list,
-	.nr_sources	= ARRAY_SIZE(clkset_group1_list),
-};
-
-static struct clk *clkset_sclk_onenand_list[] = {
-	[0] = &clk_hclk_psys.clk,
-	[1] = &clk_hclk_dsys.clk,
-};
-
-static struct clksrc_sources clkset_sclk_onenand = {
-	.sources	= clkset_sclk_onenand_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_onenand_list),
-};
-
-static struct clk *clkset_sclk_dac_list[] = {
-	[0] = &clk_sclk_vpll.clk,
-	[1] = &clk_sclk_hdmiphy,
-};
-
-static struct clksrc_sources clkset_sclk_dac = {
-	.sources	= clkset_sclk_dac_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_dac_list),
-};
-
-static struct clksrc_clk clk_sclk_dac = {
-	.clk		= {
-		.name		= "sclk_dac",
-		.id		= -1,
-		.ctrlbit	= (1 << 2),
-		.enable		= s5pv210_clk_mask0_ctrl,
-	},
-	.sources	= &clkset_sclk_dac,
-	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 8, .size = 1 },
-};
-
-static struct clksrc_clk clk_sclk_pixel = {
-	.clk		= {
-		.name		= "sclk_pixel",
-		.id		= -1,
-		.parent		= &clk_sclk_vpll.clk,
-	},
-	.reg_div	= { .reg = S5P_CLK_DIV1, .shift = 0, .size = 4},
-};
-
-static struct clk *clkset_sclk_hdmi_list[] = {
-	[0] = &clk_sclk_pixel.clk,
-	[1] = &clk_sclk_hdmiphy,
-};
-
-static struct clksrc_sources clkset_sclk_hdmi = {
-	.sources	= clkset_sclk_hdmi_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_hdmi_list),
-};
-
-static struct clksrc_clk clk_sclk_hdmi = {
-	.clk		= {
-		.name		= "sclk_hdmi",
-		.id		= -1,
-		.ctrlbit	= (1 << 0),
-		.enable		= s5pv210_clk_mask0_ctrl,
-	},
-	.sources	= &clkset_sclk_hdmi,
-	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 0, .size = 1 },
-};
-
-static struct clk *clkset_sclk_mixer_list[] = {
-	[0] = &clk_sclk_dac.clk,
-	[1] = &clk_sclk_hdmi.clk,
-};
-
-static struct clksrc_sources clkset_sclk_mixer = {
-	.sources	= clkset_sclk_mixer_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_mixer_list),
-};
-
-static struct clk *clkset_sclk_audio0_list[] = {
-	[0] = &clk_ext_xtal_mux,
-	[1] = &clk_pcmcdclk0,
-	[2] = &clk_sclk_hdmi27m,
-	[3] = &clk_sclk_usbphy0,
-	[4] = &clk_sclk_usbphy1,
-	[5] = &clk_sclk_hdmiphy,
-	[6] = &clk_mout_mpll.clk,
-	[7] = &clk_mout_epll.clk,
-	[8] = &clk_sclk_vpll.clk,
-};
-
-static struct clksrc_sources clkset_sclk_audio0 = {
-	.sources	= clkset_sclk_audio0_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_audio0_list),
-};
-
-static struct clksrc_clk clk_sclk_audio0 = {
-	.clk		= {
-		.name		= "sclk_audio",
-		.id		= 0,
-		.enable		= s5pv210_clk_mask0_ctrl,
-		.ctrlbit	= (1 << 24),
-	},
-	.sources = &clkset_sclk_audio0,
-	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 0, .size = 4 },
-	.reg_div = { .reg = S5P_CLK_DIV6, .shift = 0, .size = 4 },
-};
-
-static struct clk *clkset_sclk_audio1_list[] = {
-	[0] = &clk_ext_xtal_mux,
-	[1] = &clk_pcmcdclk1,
-	[2] = &clk_sclk_hdmi27m,
-	[3] = &clk_sclk_usbphy0,
-	[4] = &clk_sclk_usbphy1,
-	[5] = &clk_sclk_hdmiphy,
-	[6] = &clk_mout_mpll.clk,
-	[7] = &clk_mout_epll.clk,
-	[8] = &clk_sclk_vpll.clk,
-};
-
-static struct clksrc_sources clkset_sclk_audio1 = {
-	.sources	= clkset_sclk_audio1_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_audio1_list),
-};
-
-static struct clksrc_clk clk_sclk_audio1 = {
-	.clk		= {
-		.name		= "sclk_audio",
-		.id		= 1,
-		.enable		= s5pv210_clk_mask0_ctrl,
-		.ctrlbit	= (1 << 25),
-	},
-	.sources = &clkset_sclk_audio1,
-	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 4, .size = 4 },
-	.reg_div = { .reg = S5P_CLK_DIV6, .shift = 4, .size = 4 },
-};
-
-static struct clk *clkset_sclk_audio2_list[] = {
-	[0] = &clk_ext_xtal_mux,
-	[1] = &clk_pcmcdclk0,
-	[2] = &clk_sclk_hdmi27m,
-	[3] = &clk_sclk_usbphy0,
-	[4] = &clk_sclk_usbphy1,
-	[5] = &clk_sclk_hdmiphy,
-	[6] = &clk_mout_mpll.clk,
-	[7] = &clk_mout_epll.clk,
-	[8] = &clk_sclk_vpll.clk,
-};
-
-static struct clksrc_sources clkset_sclk_audio2 = {
-	.sources	= clkset_sclk_audio2_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_audio2_list),
-};
-
-static struct clksrc_clk clk_sclk_audio2 = {
-	.clk		= {
-		.name		= "sclk_audio",
-		.id		= 2,
-		.enable		= s5pv210_clk_mask0_ctrl,
-		.ctrlbit	= (1 << 26),
-	},
-	.sources = &clkset_sclk_audio2,
-	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 8, .size = 4 },
-	.reg_div = { .reg = S5P_CLK_DIV6, .shift = 8, .size = 4 },
-};
-
-static struct clk *clkset_sclk_spdif_list[] = {
-	[0] = &clk_sclk_audio0.clk,
-	[1] = &clk_sclk_audio1.clk,
-	[2] = &clk_sclk_audio2.clk,
-};
-
-static struct clksrc_sources clkset_sclk_spdif = {
-	.sources	= clkset_sclk_spdif_list,
-	.nr_sources	= ARRAY_SIZE(clkset_sclk_spdif_list),
-};
-
-static struct clk *clkset_group2_list[] = {
-	[0] = &clk_ext_xtal_mux,
-	[1] = &clk_xusbxti,
-	[2] = &clk_sclk_hdmi27m,
-	[3] = &clk_sclk_usbphy0,
-	[4] = &clk_sclk_usbphy1,
-	[5] = &clk_sclk_hdmiphy,
-	[6] = &clk_mout_mpll.clk,
-	[7] = &clk_mout_epll.clk,
-	[8] = &clk_sclk_vpll.clk,
-};
-
-static struct clksrc_sources clkset_group2 = {
-	.sources	= clkset_group2_list,
-	.nr_sources	= ARRAY_SIZE(clkset_group2_list),
-};
-
-static struct clksrc_clk clksrcs[] = {
-	{
-		.clk	= {
-			.name		= "sclk_dmc",
-			.id		= -1,
-		},
-		.sources = &clkset_group1,
-		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
-		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
-	}, {
-		.clk	= {
-			.name		= "sclk_onenand",
-			.id		= -1,
-		},
-		.sources = &clkset_sclk_onenand,
-		.reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
-		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
-	}, {
-		.clk	= {
-			.name		= "uclk1",
-			.id		= 0,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 12),
-		},
-		.sources = &clkset_uart,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 16, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 16, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "uclk1",
-			.id		= 1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 13),
-		},
-		.sources = &clkset_uart,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 20, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 20, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "uclk1",
-			.id		= 2,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 14),
-		},
-		.sources = &clkset_uart,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 24, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 24, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "uclk1",
-			.id		= 3,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 15),
-		},
-		.sources = &clkset_uart,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 28, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 28, .size = 4 },
-	}, {
-		.clk	= {
-			.name		= "sclk_mixer",
-			.id		= -1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 1),
-		},
-		.sources = &clkset_sclk_mixer,
-		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 4, .size = 1 },
-	}, {
-		.clk		= {
-			.name		= "sclk_spdif",
-			.id		= -1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 27),
-		},
-		.sources = &clkset_sclk_spdif,
-		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 12, .size = 2 },
-	}, {
-		.clk	= {
-			.name		= "sclk_fimc",
-			.id		= 0,
-			.enable		= s5pv210_clk_mask1_ctrl,
-			.ctrlbit	= (1 << 2),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 12, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV3, .shift = 12, .size = 4 },
-	}, {
-		.clk	= {
-			.name		= "sclk_fimc",
-			.id		= 1,
-			.enable		= s5pv210_clk_mask1_ctrl,
-			.ctrlbit	= (1 << 3),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 16, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV3, .shift = 16, .size = 4 },
-	}, {
-		.clk	= {
-			.name		= "sclk_fimc",
-			.id		= 2,
-			.enable		= s5pv210_clk_mask1_ctrl,
-			.ctrlbit	= (1 << 4),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 20, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV3, .shift = 20, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_cam",
-			.id		= 0,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 3),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 12, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 12, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_cam",
-			.id		= 1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 4),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 16, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 16, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_fimd",
-			.id		= -1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 5),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 20, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 20, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_mmc",
-			.id		= 0,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 8),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 0, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_mmc",
-			.id		= 1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 9),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 4, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_mmc",
-			.id		= 2,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 10),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 8, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_mmc",
-			.id		= 3,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 11),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV4, .shift = 12, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_mfc",
-			.id		= -1,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 16),
-		},
-		.sources = &clkset_group1,
-		.reg_src = { .reg = S5P_CLK_SRC2, .shift = 4, .size = 2 },
-		.reg_div = { .reg = S5P_CLK_DIV2, .shift = 4, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_g2d",
-			.id		= -1,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 12),
-		},
-		.sources = &clkset_group1,
-		.reg_src = { .reg = S5P_CLK_SRC2, .shift = 8, .size = 2 },
-		.reg_div = { .reg = S5P_CLK_DIV2, .shift = 8, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_g3d",
-			.id		= -1,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 8),
-		},
-		.sources = &clkset_group1,
-		.reg_src = { .reg = S5P_CLK_SRC2, .shift = 0, .size = 2 },
-		.reg_div = { .reg = S5P_CLK_DIV2, .shift = 0, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_csis",
-			.id		= -1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 6),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 24, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV1, .shift = 28, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_spi",
-			.id		= 0,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 16),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 0, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV5, .shift = 0, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_spi",
-			.id		= 1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 17),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 4, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV5, .shift = 4, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_pwi",
-			.id		= -1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 29),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 20, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 24, .size = 4 },
-	}, {
-		.clk		= {
-			.name		= "sclk_pwm",
-			.id		= -1,
-			.enable		= s5pv210_clk_mask0_ctrl,
-			.ctrlbit	= (1 << 19),
-		},
-		.sources = &clkset_group2,
-		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 12, .size = 4 },
-		.reg_div = { .reg = S5P_CLK_DIV5, .shift = 12, .size = 4 },
-	},
-};
-
 /* Clock initialisation code */
 static struct clksrc_clk *sysclks[] = {
 	&clk_mout_apll,
-- 
1.6.3.3

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Kukjin Kim]

MyungJoo Ham wrote:
> 
Hi,

Cc'ed Ben Dooks.

> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
> clksrc_clk.clk in clksrcs[].
> 
> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
> the div and src settting of sclk-fimd changes and a user of "lcd" wants
> to get the exact information about the rate. If "sclk-fimd" is set as a
> parent of "lcd", then the clk framework can provide the wanted
> information; otherwise, the change in "sclk-fimd" does not update "lcd"
> properly.
> 
I don't think so.

This can be problem because the parent clock of LCD(FIMD) is not always
SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock of
'lcd' should be dsys clock, not sclk_fimd.
Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
not. So should separate them and need to separate control.

As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.

> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
> one of clksrcs[] as its parent. (enum clksrc_refer)
> 
> For example, "lcd" may set "sclk-fimd" as a parent if the following line
> replaces the line 878:
> 
> 		.parent		= &clk_hclk_dsys.clk,
> 
> to
> 
> 		.parent		= &clksrcs[_clksrc_sclk_fimd],
> 


> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-s5pv210/clock.c |  986
++++++++++++++++++++++-------------------
>  1 files changed, 520 insertions(+), 466 deletions(-)
> 
> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> index aa2b1c5..e33c6ed 100644
> --- a/arch/arm/mach-s5pv210/clock.c
> +++ b/arch/arm/mach-s5pv210/clock.c
> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
>  	.reg_src	= { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
>  };
> 

And...hmm...maybe made this patch with wrong way...

> +static struct clk *clkset_uart_list[] = {
> +	[6] = &clk_mout_mpll.clk,
> +	[7] = &clk_mout_epll.clk,
> +};
> +

Following is from this patch...

-static struct clk *clkset_uart_list[] = {
-	[6] = &clk_mout_mpll.clk,
-	[7] = &clk_mout_epll.clk,
-};
-

Same? :-(
Why did you remove and add same code?

(snip)

And...see the below.

Added like below...

> +static struct clksrc_clk clksrcs[] = {
> +	[_clksrc_sclk_dmc] = {
> +		.clk	= {
> +			.name		= "sclk_dmc",
> +			.id		= -1,
> +		},
> +		.sources = &clkset_group1,
> +		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> +		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> +	},
> +	[_clksrc_sclk_onenand] = {
> +		.clk	= {
> +			.name		= "sclk_onenand",
> +			.id		= -1,
> +		},
> +		.sources = &clkset_sclk_onenand,
> +		.reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> +		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> +	},

(snip)

And removed like below...

> -static struct clksrc_clk clksrcs[] = {
> -	{
> -		.clk	= {
> -			.name		= "sclk_dmc",
> -			.id		= -1,
> -		},
> -		.sources = &clkset_group1,
> -		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> -		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> -	}, {
> -		.clk	= {
> -			.name		= "sclk_onenand",
> -			.id		= -1,
> -		},
> -		.sources = &clkset_sclk_onenand,
> -		.reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> -		.reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> -	}, {

But only changed small lines not every.
Following is from my local machine.

@@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
 };

 static struct clksrc_clk clksrcs[] = {
-       {
+       [_clksrc_sclk_dmc] = {
                .clk    = {
                        .name           = "sclk_dmc",
                        .id             = -1,
@@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
                .sources = &clkset_group1,
                .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
                .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
-       }, {
+       },
+       [_clksrc_sclk_onenand] = {
                .clk    = {
                        .name           = "sclk_onenand",
                        .id             = -1,
 (snip)

Please make sure your patch has no problem before submitting.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[MyungJoo Ham]

Hello,

On Fri, Jul 9, 2010 at 7:29 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
> Hi,
>
> Cc'ed Ben Dooks.
>
>> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
>> clksrc_clk.clk in clksrcs[].
>>
>> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
>> the div and src settting of sclk-fimd changes and a user of "lcd" wants
>> to get the exact information about the rate. If "sclk-fimd" is set as a
>> parent of "lcd", then the clk framework can provide the wanted
>> information; otherwise, the change in "sclk-fimd" does not update "lcd"
>> properly.
>>
> I don't think so.
>
> This can be problem because the parent clock of LCD(FIMD) is not always
> SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock of
> 'lcd' should be dsys clock, not sclk_fimd.
> Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
> not. So should separate them and need to separate control.

Well, yes SCLK_FIMD is not ALWAYS the parent of LCD(FIMD), but it is one of the
possible parents of LCD(FIMD); thus, SCLK_FIMD should be able to become the
parent. I'm not saying that SCLK_FIMD should be the parent, but SCLK_FIMD should
be ABLE to be the parent. That's why I made it just "possible', not making it an
actual parent.

However, in order to make it look better, we may need to make LCD(FIMD) easier
to switch between SCLK_ACLK and SCLK_FIMD; e.g., by making it another clksrc_clk
with source clocks of SCLK_ACLK and SCLK_FIMD controlling with CLK_GATE_IP.

>
> As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
> SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
> CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.
>
>> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
>> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
>> one of clksrcs[] as its parent. (enum clksrc_refer)
>>
>> For example, "lcd" may set "sclk-fimd" as a parent if the following line
>> replaces the line 878:
>>
>> ? ? ? ? ? ? ? .parent ? ? ? ? = &clk_hclk_dsys.clk,
>>
>> to
>>
>> ? ? ? ? ? ? ? .parent ? ? ? ? = &clksrcs[_clksrc_sclk_fimd],
>>
>
>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> ?arch/arm/mach-s5pv210/clock.c | ?986
> ++++++++++++++++++++++-------------------
>> ?1 files changed, 520 insertions(+), 466 deletions(-)
>>
>> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> index aa2b1c5..e33c6ed 100644
>> --- a/arch/arm/mach-s5pv210/clock.c
>> +++ b/arch/arm/mach-s5pv210/clock.c
>> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
>> ? ? ? .reg_src ? ? ? ?= { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
>> ?};
>>
>
> And...hmm...maybe made this patch with wrong way...
>
>> +static struct clk *clkset_uart_list[] = {
>> + ? ? [6] = &clk_mout_mpll.clk,
>> + ? ? [7] = &clk_mout_epll.clk,
>> +};
>> +
>
> Following is from this patch...
>
> -static struct clk *clkset_uart_list[] = {
> - ? ? ? [6] = &clk_mout_mpll.clk,
> - ? ? ? [7] = &clk_mout_epll.clk,
> -};
> -
>
> Same? :-(
> Why did you remove and add same code?

It is done so in order to make clksrc_clk to be able to be parents of
clk. (same with the
later parts)

To do so, I moved clksrc_clk to the front of clk definitions, which in
turn moved
clock source list to the front of clksrc_clk.

Or, in order to reduce the changed lines number, would it be better to declare
the list of struct clksrc_clk before and keep the locations of the lists?


>
> (snip)
>
> And...see the below.
>
> Added like below...
>
>> +static struct clksrc_clk clksrcs[] = {
>> + ? ? [_clksrc_sclk_dmc] = {
>> + ? ? ? ? ? ? .clk ? ?= {
>> + ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dmc",
>> + ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> + ? ? ? ? ? ? },
>> + ? ? ? ? ? ? .sources = &clkset_group1,
>> + ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> + ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> + ? ? },
>> + ? ? [_clksrc_sclk_onenand] = {
>> + ? ? ? ? ? ? .clk ? ?= {
>> + ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_onenand",
>> + ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> + ? ? ? ? ? ? },
>> + ? ? ? ? ? ? .sources = &clkset_sclk_onenand,
>> + ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
>> + ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
>> + ? ? },
>
> (snip)
>
> And removed like below...
>
>> -static struct clksrc_clk clksrcs[] = {
>> - ? ? {
>> - ? ? ? ? ? ? .clk ? ?= {
>> - ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dmc",
>> - ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? },
>> - ? ? ? ? ? ? .sources = &clkset_group1,
>> - ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> - ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> - ? ? }, {
>> - ? ? ? ? ? ? .clk ? ?= {
>> - ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_onenand",
>> - ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? },
>> - ? ? ? ? ? ? .sources = &clkset_sclk_onenand,
>> - ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
>> - ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
>> - ? ? }, {
>
> But only changed small lines not every.
> Following is from my local machine.
>
> @@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
> ?};
>
> ?static struct clksrc_clk clksrcs[] = {
> - ? ? ? {
> + ? ? ? [_clksrc_sclk_dmc] = {
> ? ? ? ? ? ? ? ?.clk ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_dmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = -1,
> @@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.sources = &clkset_group1,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> ? ? ? ? ? ? ? ?.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> - ? ? ? }, {
> + ? ? ? },
> + ? ? ? [_clksrc_sclk_onenand] = {
> ? ? ? ? ? ? ? ?.clk ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_onenand",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = -1,
> ?(snip)
>
> Please make sure your patch has no problem before submitting.
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> Hello,
> 
> On Fri, Jul 9, 2010 at 7:29 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> > Hi,
> >
> > Cc'ed Ben Dooks.
> >
> >> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
> >> clksrc_clk.clk in clksrcs[].
> >>
> >> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
> >> the div and src settting of sclk-fimd changes and a user of "lcd" wants
> >> to get the exact information about the rate. If "sclk-fimd" is set as a
> >> parent of "lcd", then the clk framework can provide the wanted
> >> information; otherwise, the change in "sclk-fimd" does not update "lcd"
> >> properly.
> >>
> > I don't think so.
> >
> > This can be problem because the parent clock of LCD(FIMD) is not always
> > SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock of
> > 'lcd' should be dsys clock, not sclk_fimd.
> > Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
> > not. So should separate them and need to separate control.
> 
> Well, yes SCLK_FIMD is not ALWAYS the parent of LCD(FIMD), but it is one of the
> possible parents of LCD(FIMD); thus, SCLK_FIMD should be able to become the
> parent. I'm not saying that SCLK_FIMD should be the parent, but SCLK_FIMD
> should
> be ABLE to be the parent. That's why I made it just "possible', not making it an
> actual parent.
> 
More correctly, the clk 'lcd' means clock that should be used basically at FIMD block.
So it means used clock on system bus, should be SCLK_ACLK.

Actually, if disabled clk 'lcd', we can't access FIMD block such as its register.

As I say again, clk 'lcd' is on dsys clock domain, because of these meaning, so the parent clock of 'lcd' should be dsys clock.

> However, in order to make it look better, we may need to make LCD(FIMD) easier
> to switch between SCLK_ACLK and SCLK_FIMD; e.g., by making it another
> clksrc_clk
> with source clocks of SCLK_ACLK and SCLK_FIMD controlling with CLK_GATE_IP.
> 
> >
> > As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
> > SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
> > CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.
> >
> >> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
> >> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
> >> one of clksrcs[] as its parent. (enum clksrc_refer)
> >>
> >> For example, "lcd" may set "sclk-fimd" as a parent if the following line
> >> replaces the line 878:
> >>
> >>               .parent         = &clk_hclk_dsys.clk,
> >>
> >> to
> >>
> >>               .parent         = &clksrcs[_clksrc_sclk_fimd],
> >>
> >
> >
> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >>  arch/arm/mach-s5pv210/clock.c |  986
> > ++++++++++++++++++++++-------------------
> >>  1 files changed, 520 insertions(+), 466 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> >> index aa2b1c5..e33c6ed 100644
> >> --- a/arch/arm/mach-s5pv210/clock.c
> >> +++ b/arch/arm/mach-s5pv210/clock.c
> >> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
> >>       .reg_src        = { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
> >>  };
> >>
> >
> > And...hmm...maybe made this patch with wrong way...
> >
> >> +static struct clk *clkset_uart_list[] = {
> >> +     [6] = &clk_mout_mpll.clk,
> >> +     [7] = &clk_mout_epll.clk,
> >> +};
> >> +
> >
> > Following is from this patch...
> >
> > -static struct clk *clkset_uart_list[] = {
> > -       [6] = &clk_mout_mpll.clk,
> > -       [7] = &clk_mout_epll.clk,
> > -};
> > -
> >
> > Same? :-(
> > Why did you remove and add same code?
> 
> It is done so in order to make clksrc_clk to be able to be parents of
> clk. (same with the
> later parts)
> 
> To do so, I moved clksrc_clk to the front of clk definitions, which in
> turn moved
> clock source list to the front of clksrc_clk.
> 
> Or, in order to reduce the changed lines number, would it be better to declare
> the list of struct clksrc_clk before and keep the locations of the lists?
> 
> 
> >
> > (snip)
> >
> > And...see the below.
> >
> > Added like below...
> >
> >> +static struct clksrc_clk clksrcs[] = {
> >> +     [_clksrc_sclk_dmc] = {
> >> +             .clk    = {
> >> +                     .name           = "sclk_dmc",
> >> +                     .id             = -1,
> >> +             },
> >> +             .sources = &clkset_group1,
> >> +             .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> +             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> +     },
> >> +     [_clksrc_sclk_onenand] = {
> >> +             .clk    = {
> >> +                     .name           = "sclk_onenand",
> >> +                     .id             = -1,
> >> +             },
> >> +             .sources = &clkset_sclk_onenand,
> >> +             .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> >> +             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> >> +     },
> >
> > (snip)
> >
> > And removed like below...
> >
> >> -static struct clksrc_clk clksrcs[] = {
> >> -     {
> >> -             .clk    = {
> >> -                     .name           = "sclk_dmc",
> >> -                     .id             = -1,
> >> -             },
> >> -             .sources = &clkset_group1,
> >> -             .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> -             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> -     }, {
> >> -             .clk    = {
> >> -                     .name           = "sclk_onenand",
> >> -                     .id             = -1,
> >> -             },
> >> -             .sources = &clkset_sclk_onenand,
> >> -             .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> >> -             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> >> -     }, {
> >
> > But only changed small lines not every.
> > Following is from my local machine.
> >
> > @@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
> >  };
> >
> >  static struct clksrc_clk clksrcs[] = {
> > -       {
> > +       [_clksrc_sclk_dmc] = {
> >                .clk    = {
> >                        .name           = "sclk_dmc",
> >                        .id             = -1,
> > @@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
> >                .sources = &clkset_group1,
> >                .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >                .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> > -       }, {
> > +       },
> > +       [_clksrc_sclk_onenand] = {
> >                .clk    = {
> >                        .name           = "sclk_onenand",
> >                        .id             = -1,
> >  (snip)
> >
> > Please make sure your patch has no problem before submitting.
> >
> > Thanks.
> >
> > Best regards,
> > Kgene.
> > --
> > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> > SW Solution Development Team, Samsung Electronics Co., Ltd.
> >
> >


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[MyungJoo Ham]

Hello,

On Mon, Jul 12, 2010 at 11:27 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> Hello,
>>
>> On Fri, Jul 9, 2010 at 7:29 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > MyungJoo Ham wrote:
>> >>
>> > Hi,
>> >
>> > Cc'ed Ben Dooks.
>> >
>> >> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
>> >> clksrc_clk.clk in clksrcs[].
>> >>
>> >> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
>> >> the div and src settting of sclk-fimd changes and a user of "lcd" wants
>> >> to get the exact information about the rate. If "sclk-fimd" is set as a
>> >> parent of "lcd", then the clk framework can provide the wanted
>> >> information; otherwise, the change in "sclk-fimd" does not update "lcd"
>> >> properly.
>> >>
>> > I don't think so.
>> >
>> > This can be problem because the parent clock of LCD(FIMD) is not always
>> > SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock of
>> > 'lcd' should be dsys clock, not sclk_fimd.
>> > Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
>> > not. So should separate them and need to separate control.
>>
>> Well, yes SCLK_FIMD is not ALWAYS the parent of LCD(FIMD), but it is one of the
>> possible parents of LCD(FIMD); thus, SCLK_FIMD should be able to become the
>> parent. I'm not saying that SCLK_FIMD should be the parent, but SCLK_FIMD
>> should
>> be ABLE to be the parent. That's why I made it just "possible', not making it an
>> actual parent.
>>
> More correctly, the clk 'lcd' means clock that should be used basically at FIMD block.
> So it means used clock on system bus, should be SCLK_ACLK.
>
> Actually, if disabled clk 'lcd', we can't access FIMD block such as its register.
>
> As I say again, clk 'lcd' is on dsys clock domain, because of these meaning, so the parent clock of 'lcd' should be dsys clock.

Ok, then, clk_get_rate(lcd) returns 166MHz, always, which in turn,
makes it more difficult
for lcd clock users to get the correct current rate when sclk_fimd's
DIV changes. When we need to show that lcd is in dsys domain, isn't it
enough to set dsys is an ancestor (grandparent) of lcd?

I thought it'd contain more semantics if lcd's parent is fimd and
fimd's parent is dsys; i.e., dsys is still an ancestor of lcd and
clk_get_rate(lcd) may return the clock value of lcd while DIV/SRC
changes. When lcd does not use sclk_fimd anymore, we can
set_parent(lcd, something) anytime (to do this more easily, we may
need to define lcd within clksrc_clk).


Besides, we may need to allow clock-clksrc.c's s3c_register_clksrc
function to use the default .parent value to set the SRC register
accordingly. For example (a patch under construction, not uploaded yet
and requires patches submitted before):


Author: MyungJoo Ham <myungjoo.ham@samsung.com>
Date:   Thu Jul 8 18:34:01 2010 +0900

    ARM: SAMSUNG SoC: clksrc register uses parent value to set default src.

    If .parent field is filled when a struct clksrc_clk is registered,
    s3c_register_clksrc sets the source clock register accordingly to the
    parent clock.

    In the previous versions, the parent value was "ignored" because the
    value of default register value overwrites the parent field.

diff --git a/arch/arm/plat-samsung/clock-clksrc.c
b/arch/arm/plat-samsung/clock-clksrc.c
index ae8b850..236fb59 100644
--- a/arch/arm/plat-samsung/clock-clksrc.c
+++ b/arch/arm/plat-samsung/clock-clksrc.c
@@ -177,7 +177,7 @@ static struct clk_ops clksrc_ops_nosrc = {

 void __init s3c_register_clksrc(struct clksrc_clk *clksrc, int size)
 {
-       int ret;
+       int ret, err = 0;

        for (; size > 0; size--, clksrc++) {
                if (!clksrc->reg_div.reg && !clksrc->reg_src.reg)
@@ -195,12 +195,25 @@ void __init s3c_register_clksrc(struct
clksrc_clk *clksrc, int size)
                                clksrc->clk.ops = &clksrc_ops;
                }

+               if (clksrc->clk.parent && clksrc->sources &&
+                               clksrc->reg_src.reg) {
+                       err = s3c_setparent_clksrc(&clksrc->clk,
+                                       clksrc->clk.parent);
+                       if (err < 0)
+                               printk(KERN_ERR "[%s:%d] setparent error@reg"
+                                               "ister_clksrc %s p = %s...\n",
+                                               __FILE__, __LINE__,
+                                               clksrc->clk.name,
+                                               clksrc->clk.parent->name);
+               }
+
                /* setup the clocksource, but do not announce it
                 * as it may be re-set by the setup routines
                 * called after the rest of the clocks have been
                 * registered
                 */
-               s3c_set_clksrc(clksrc, false);
+               if (err < 0)
+                       s3c_set_clksrc(clksrc, false);

                ret = s3c24xx_register_clock(&clksrc->clk);




>
>> However, in order to make it look better, we may need to make LCD(FIMD) easier
>> to switch between SCLK_ACLK and SCLK_FIMD; e.g., by making it another
>> clksrc_clk
>> with source clocks of SCLK_ACLK and SCLK_FIMD controlling with CLK_GATE_IP.
>>
>> >
>> > As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
>> > SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
>> > CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.
>> >
>> >> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
>> >> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
>> >> one of clksrcs[] as its parent. (enum clksrc_refer)
>> >>
>> >> For example, "lcd" may set "sclk-fimd" as a parent if the following line
>> >> replaces the line 878:
>> >>
>> >> ? ? ? ? ? ? ? .parent ? ? ? ? = &clk_hclk_dsys.clk,
>> >>
>> >> to
>> >>
>> >> ? ? ? ? ? ? ? .parent ? ? ? ? = &clksrcs[_clksrc_sclk_fimd],
>> >>
>> >
>> >
>> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> >> ---
>> >> ?arch/arm/mach-s5pv210/clock.c | ?986
>> > ++++++++++++++++++++++-------------------
>> >> ?1 files changed, 520 insertions(+), 466 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> >> index aa2b1c5..e33c6ed 100644
>> >> --- a/arch/arm/mach-s5pv210/clock.c
>> >> +++ b/arch/arm/mach-s5pv210/clock.c
>> >> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
>> >> ? ? ? .reg_src ? ? ? ?= { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
>> >> ?};
>> >>
>> >
>> > And...hmm...maybe made this patch with wrong way...
>> >
>> >> +static struct clk *clkset_uart_list[] = {
>> >> + ? ? [6] = &clk_mout_mpll.clk,
>> >> + ? ? [7] = &clk_mout_epll.clk,
>> >> +};
>> >> +
>> >
>> > Following is from this patch...
>> >
>> > -static struct clk *clkset_uart_list[] = {
>> > - ? ? ? [6] = &clk_mout_mpll.clk,
>> > - ? ? ? [7] = &clk_mout_epll.clk,
>> > -};
>> > -
>> >
>> > Same? :-(
>> > Why did you remove and add same code?
>>
>> It is done so in order to make clksrc_clk to be able to be parents of
>> clk. (same with the
>> later parts)
>>
>> To do so, I moved clksrc_clk to the front of clk definitions, which in
>> turn moved
>> clock source list to the front of clksrc_clk.
>>
>> Or, in order to reduce the changed lines number, would it be better to declare
>> the list of struct clksrc_clk before and keep the locations of the lists?
>>
>>
>> >
>> > (snip)
>> >
>> > And...see the below.
>> >
>> > Added like below...
>> >
>> >> +static struct clksrc_clk clksrcs[] = {
>> >> + ? ? [_clksrc_sclk_dmc] = {
>> >> + ? ? ? ? ? ? .clk ? ?= {
>> >> + ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dmc",
>> >> + ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> + ? ? ? ? ? ? },
>> >> + ? ? ? ? ? ? .sources = &clkset_group1,
>> >> + ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> >> + ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> >> + ? ? },
>> >> + ? ? [_clksrc_sclk_onenand] = {
>> >> + ? ? ? ? ? ? .clk ? ?= {
>> >> + ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_onenand",
>> >> + ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> + ? ? ? ? ? ? },
>> >> + ? ? ? ? ? ? .sources = &clkset_sclk_onenand,
>> >> + ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
>> >> + ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
>> >> + ? ? },
>> >
>> > (snip)
>> >
>> > And removed like below...
>> >
>> >> -static struct clksrc_clk clksrcs[] = {
>> >> - ? ? {
>> >> - ? ? ? ? ? ? .clk ? ?= {
>> >> - ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dmc",
>> >> - ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> - ? ? ? ? ? ? },
>> >> - ? ? ? ? ? ? .sources = &clkset_group1,
>> >> - ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> >> - ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> >> - ? ? }, {
>> >> - ? ? ? ? ? ? .clk ? ?= {
>> >> - ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_onenand",
>> >> - ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> - ? ? ? ? ? ? },
>> >> - ? ? ? ? ? ? .sources = &clkset_sclk_onenand,
>> >> - ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
>> >> - ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
>> >> - ? ? }, {
>> >
>> > But only changed small lines not every.
>> > Following is from my local machine.
>> >
>> > @@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
>> > ?};
>> >
>> > ?static struct clksrc_clk clksrcs[] = {
>> > - ? ? ? {
>> > + ? ? ? [_clksrc_sclk_dmc] = {
>> > ? ? ? ? ? ? ? ?.clk ? ?= {
>> > ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_dmc",
>> > ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = -1,
>> > @@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
>> > ? ? ? ? ? ? ? ?.sources = &clkset_group1,
>> > ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> > ? ? ? ? ? ? ? ?.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> > - ? ? ? }, {
>> > + ? ? ? },
>> > + ? ? ? [_clksrc_sclk_onenand] = {
>> > ? ? ? ? ? ? ? ?.clk ? ?= {
>> > ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_onenand",
>> > ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = -1,
>> > ?(snip)
>> >
>> > Please make sure your patch has no problem before submitting.
>> >
>> > Thanks.
>> >
>> > Best regards,
>> > Kgene.
>> > --
>> > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
>> > SW Solution Development Team, Samsung Electronics Co., Ltd.
>> >
>> >
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> Hello,
> 
> On Mon, Jul 12, 2010 at 11:27 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> >> Hello,
> >>
> >> On Fri, Jul 9, 2010 at 7:29 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> >> > MyungJoo Ham wrote:
> >> >>
> >> > Hi,
> >> >
> >> > Cc'ed Ben Dooks.
> >> >
> >> >> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
> >> >> clksrc_clk.clk in clksrcs[].
> >> >>
> >> >> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
> >> >> the div and src settting of sclk-fimd changes and a user of "lcd" wants
> >> >> to get the exact information about the rate. If "sclk-fimd" is set as a
> >> >> parent of "lcd", then the clk framework can provide the wanted
> >> >> information; otherwise, the change in "sclk-fimd" does not update "lcd"
> >> >> properly.
> >> >>
> >> > I don't think so.
> >> >
> >> > This can be problem because the parent clock of LCD(FIMD) is not always
> >> > SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock
> of
> >> > 'lcd' should be dsys clock, not sclk_fimd.
> >> > Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
> >> > not. So should separate them and need to separate control.
> >>
> >> Well, yes SCLK_FIMD is not ALWAYS the parent of LCD(FIMD), but it is one of
> the
> >> possible parents of LCD(FIMD); thus, SCLK_FIMD should be able to become
> the
> >> parent. I'm not saying that SCLK_FIMD should be the parent, but SCLK_FIMD
> >> should
> >> be ABLE to be the parent. That's why I made it just "possible', not making it an
> >> actual parent.
> >>
> > More correctly, the clk 'lcd' means clock that should be used basically at FIMD
> block.
> > So it means used clock on system bus, should be SCLK_ACLK.
> >
> > Actually, if disabled clk 'lcd', we can't access FIMD block such as its register.
> >
> > As I say again, clk 'lcd' is on dsys clock domain, because of these meaning, so
> the parent clock of 'lcd' should be dsys clock.
> 
> Ok, then, clk_get_rate(lcd) returns 166MHz, always, which in turn,
> makes it more difficult
> for lcd clock users to get the correct current rate when sclk_fimd's
> DIV changes. When we need to show that lcd is in dsys domain, isn't it
> enough to set dsys is an ancestor (grandparent) of lcd?
> 
> I thought it'd contain more semantics if lcd's parent is fimd and
> fimd's parent is dsys; i.e., dsys is still an ancestor of lcd and
> clk_get_rate(lcd) may return the clock value of lcd while DIV/SRC
> changes. When lcd does not use sclk_fimd anymore, we can
> set_parent(lcd, something) anytime (to do this more easily, we may
> need to define lcd within clksrc_clk).
> 
> 
Please refer to below diagram.

------------------------------------
           dsys bus
----------------+-------------------
                |
                |1.clk 'lcd'
                |
            +---+-----------+
            |   | FIMD block|
            | +-+           |
            | |             |
            | | |\          |
            | +-|m|         |
2.SCLK_FIMD |   |u|----+    |
------------+---|x|    |    |
            |   |/     |    |
            |          |    |
            +----------+----+
                       |
inside of SoC          |
-----------------------+--------------------------
outside of SoC         |
                       | 3.clk?
                       |
               +--------------+
               | LCD module   |
               +--------------+

In clock.c the clk 'lcd' means #1 in above diagram.
So you mean clk 'lcd' is #3', maybe confused :-(

> Besides, we may need to allow clock-clksrc.c's s3c_register_clksrc
> function to use the default .parent value to set the SRC register
> accordingly. For example (a patch under construction, not uploaded yet
> and requires patches submitted before):
> 
> 
> Author: MyungJoo Ham <myungjoo.ham@samsung.com>
> Date:   Thu Jul 8 18:34:01 2010 +0900
> 
>     ARM: SAMSUNG SoC: clksrc register uses parent value to set default src.
> 
>     If .parent field is filled when a struct clksrc_clk is registered,
>     s3c_register_clksrc sets the source clock register accordingly to the
>     parent clock.
> 
>     In the previous versions, the parent value was "ignored" because the
>     value of default register value overwrites the parent field.
> 
> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
> b/arch/arm/plat-samsung/clock-clksrc.c
> index ae8b850..236fb59 100644
> --- a/arch/arm/plat-samsung/clock-clksrc.c
> +++ b/arch/arm/plat-samsung/clock-clksrc.c
> @@ -177,7 +177,7 @@ static struct clk_ops clksrc_ops_nosrc = {
> 
>  void __init s3c_register_clksrc(struct clksrc_clk *clksrc, int size)
>  {
> -       int ret;
> +       int ret, err = 0;
> 
>         for (; size > 0; size--, clksrc++) {
>                 if (!clksrc->reg_div.reg && !clksrc->reg_src.reg)
> @@ -195,12 +195,25 @@ void __init s3c_register_clksrc(struct
> clksrc_clk *clksrc, int size)
>                                 clksrc->clk.ops = &clksrc_ops;
>                 }
> 
> +               if (clksrc->clk.parent && clksrc->sources &&
> +                               clksrc->reg_src.reg) {
> +                       err = s3c_setparent_clksrc(&clksrc->clk,
> +                                       clksrc->clk.parent);
> +                       if (err < 0)
> +                               printk(KERN_ERR "[%s:%d] setparent error
> at reg"
> +                                               "ister_clksrc %s p
> = %s...\n",
> +                                               __FILE__, __LINE__,
> +                                               clksrc->clk.name,
> +                                               clksrc->clk.parent->name);
> +               }
> +
>                 /* setup the clocksource, but do not announce it
>                  * as it may be re-set by the setup routines
>                  * called after the rest of the clocks have been
>                  * registered
>                  */
> -               s3c_set_clksrc(clksrc, false);
> +               if (err < 0)
> +                       s3c_set_clksrc(clksrc, false);
> 
>                 ret = s3c24xx_register_clock(&clksrc->clk);
> 
> 
> 
> 
> >
> >> However, in order to make it look better, we may need to make LCD(FIMD)
> easier
> >> to switch between SCLK_ACLK and SCLK_FIMD; e.g., by making it another
> >> clksrc_clk
> >> with source clocks of SCLK_ACLK and SCLK_FIMD controlling with
> CLK_GATE_IP.
> >>
> >> >
> >> > As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
> >> > SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
> >> > CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.
> >> >
> >> >> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
> >> >> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
> >> >> one of clksrcs[] as its parent. (enum clksrc_refer)
> >> >>
> >> >> For example, "lcd" may set "sclk-fimd" as a parent if the following line
> >> >> replaces the line 878:
> >> >>
> >> >>               .parent         = &clk_hclk_dsys.clk,
> >> >>
> >> >> to
> >> >>
> >> >>               .parent         = &clksrcs[_clksrc_sclk_fimd],
> >> >>
> >> >
> >> >
> >> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> >> ---
> >> >>  arch/arm/mach-s5pv210/clock.c |  986
> >> > ++++++++++++++++++++++-------------------
> >> >>  1 files changed, 520 insertions(+), 466 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> >> >> index aa2b1c5..e33c6ed 100644
> >> >> --- a/arch/arm/mach-s5pv210/clock.c
> >> >> +++ b/arch/arm/mach-s5pv210/clock.c
> >> >> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
> >> >>       .reg_src        = { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
> >> >>  };
> >> >>
> >> >
> >> > And...hmm...maybe made this patch with wrong way...
> >> >
> >> >> +static struct clk *clkset_uart_list[] = {
> >> >> +     [6] = &clk_mout_mpll.clk,
> >> >> +     [7] = &clk_mout_epll.clk,
> >> >> +};
> >> >> +
> >> >
> >> > Following is from this patch...
> >> >
> >> > -static struct clk *clkset_uart_list[] = {
> >> > -       [6] = &clk_mout_mpll.clk,
> >> > -       [7] = &clk_mout_epll.clk,
> >> > -};
> >> > -
> >> >
> >> > Same? :-(
> >> > Why did you remove and add same code?
> >>
> >> It is done so in order to make clksrc_clk to be able to be parents of
> >> clk. (same with the
> >> later parts)
> >>
> >> To do so, I moved clksrc_clk to the front of clk definitions, which in
> >> turn moved
> >> clock source list to the front of clksrc_clk.
> >>
> >> Or, in order to reduce the changed lines number, would it be better to declare
> >> the list of struct clksrc_clk before and keep the locations of the lists?
> >>
> >>
> >> >
> >> > (snip)
> >> >
> >> > And...see the below.
> >> >
> >> > Added like below...
> >> >
> >> >> +static struct clksrc_clk clksrcs[] = {
> >> >> +     [_clksrc_sclk_dmc] = {
> >> >> +             .clk    = {
> >> >> +                     .name           = "sclk_dmc",
> >> >> +                     .id             = -1,
> >> >> +             },
> >> >> +             .sources = &clkset_group1,
> >> >> +             .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> >> +             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> >> +     },
> >> >> +     [_clksrc_sclk_onenand] = {
> >> >> +             .clk    = {
> >> >> +                     .name           = "sclk_onenand",
> >> >> +                     .id             = -1,
> >> >> +             },
> >> >> +             .sources = &clkset_sclk_onenand,
> >> >> +             .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> >> >> +             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> >> >> +     },
> >> >
> >> > (snip)
> >> >
> >> > And removed like below...
> >> >
> >> >> -static struct clksrc_clk clksrcs[] = {
> >> >> -     {
> >> >> -             .clk    = {
> >> >> -                     .name           = "sclk_dmc",
> >> >> -                     .id             = -1,
> >> >> -             },
> >> >> -             .sources = &clkset_group1,
> >> >> -             .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> >> -             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> >> -     }, {
> >> >> -             .clk    = {
> >> >> -                     .name           = "sclk_onenand",
> >> >> -                     .id             = -1,
> >> >> -             },
> >> >> -             .sources = &clkset_sclk_onenand,
> >> >> -             .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> >> >> -             .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> >> >> -     }, {
> >> >
> >> > But only changed small lines not every.
> >> > Following is from my local machine.
> >> >
> >> > @@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
> >> >  };
> >> >
> >> >  static struct clksrc_clk clksrcs[] = {
> >> > -       {
> >> > +       [_clksrc_sclk_dmc] = {
> >> >                .clk    = {
> >> >                        .name           = "sclk_dmc",
> >> >                        .id             = -1,
> >> > @@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
> >> >                .sources = &clkset_group1,
> >> >                .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> >                .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> > -       }, {
> >> > +       },
> >> > +       [_clksrc_sclk_onenand] = {
> >> >                .clk    = {
> >> >                        .name           = "sclk_onenand",
> >> >                        .id             = -1,
> >> >  (snip)
> >> >
> >> > Please make sure your patch has no problem before submitting.
> >> >
> >> > Thanks.
> >> >
> >> > Best regards,
> >> > Kgene.
> >> > --
> >> > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> >> > SW Solution Development Team, Samsung Electronics Co., Ltd.
> >> >
> >> >
> >


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[MyungJoo Ham]

On Mon, Jul 12, 2010 at 2:12 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> Hello,
>>
>> On Mon, Jul 12, 2010 at 11:27 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > MyungJoo Ham wrote:
>> >>
>> >> Hello,
>> >>
>> >> On Fri, Jul 9, 2010 at 7:29 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> >> > MyungJoo Ham wrote:
>> >> >>
>> >> > Hi,
>> >> >
>> >> > Cc'ed Ben Dooks.
>> >> >
>> >> >> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
>> >> >> clksrc_clk.clk in clksrcs[].
>> >> >>
>> >> >> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
>> >> >> the div and src settting of sclk-fimd changes and a user of "lcd" wants
>> >> >> to get the exact information about the rate. If "sclk-fimd" is set as a
>> >> >> parent of "lcd", then the clk framework can provide the wanted
>> >> >> information; otherwise, the change in "sclk-fimd" does not update "lcd"
>> >> >> properly.
>> >> >>
>> >> > I don't think so.
>> >> >
>> >> > This can be problem because the parent clock of LCD(FIMD) is not always
>> >> > SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock
>> of
>> >> > 'lcd' should be dsys clock, not sclk_fimd.
>> >> > Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
>> >> > not. So should separate them and need to separate control.
>> >>
>> >> Well, yes SCLK_FIMD is not ALWAYS the parent of LCD(FIMD), but it is one of
>> the
>> >> possible parents of LCD(FIMD); thus, SCLK_FIMD should be able to become
>> the
>> >> parent. I'm not saying that SCLK_FIMD should be the parent, but SCLK_FIMD
>> >> should
>> >> be ABLE to be the parent. That's why I made it just "possible', not making it an
>> >> actual parent.
>> >>
>> > More correctly, the clk 'lcd' means clock that should be used basically at FIMD
>> block.
>> > So it means used clock on system bus, should be SCLK_ACLK.
>> >
>> > Actually, if disabled clk 'lcd', we can't access FIMD block such as its register.
>> >
>> > As I say again, clk 'lcd' is on dsys clock domain, because of these meaning, so
>> the parent clock of 'lcd' should be dsys clock.
>>
>> Ok, then, clk_get_rate(lcd) returns 166MHz, always, which in turn,
>> makes it more difficult
>> for lcd clock users to get the correct current rate when sclk_fimd's
>> DIV changes. When we need to show that lcd is in dsys domain, isn't it
>> enough to set dsys is an ancestor (grandparent) of lcd?
>>
>> I thought it'd contain more semantics if lcd's parent is fimd and
>> fimd's parent is dsys; i.e., dsys is still an ancestor of lcd and
>> clk_get_rate(lcd) may return the clock value of lcd while DIV/SRC
>> changes. When lcd does not use sclk_fimd anymore, we can
>> set_parent(lcd, something) anytime (to do this more easily, we may
>> need to define lcd within clksrc_clk).
>>
>>
> Please refer to below diagram.
>
> ------------------------------------
> ? ? ? ? ? dsys bus
> ----------------+-------------------
> ? ? ? ? ? ? ? ?|
> ? ? ? ? ? ? ? ?|1.clk 'lcd'
> ? ? ? ? ? ? ? ?|
> ? ? ? ? ? ?+---+-----------+
> ? ? ? ? ? ?| ? | FIMD block|
> ? ? ? ? ? ?| +-+ ? ? ? ? ? |
> ? ? ? ? ? ?| | ? ? ? ? ? ? |
> ? ? ? ? ? ?| | |\ ? ? ? ? ?|
> ? ? ? ? ? ?| +-|m| ? ? ? ? |
> 2.SCLK_FIMD | ? |u|----+ ? ?|
> ------------+---|x| ? ?| ? ?|
> ? ? ? ? ? ?| ? |/ ? ? | ? ?|
> ? ? ? ? ? ?| ? ? ? ? ?| ? ?|
> ? ? ? ? ? ?+----------+----+
> ? ? ? ? ? ? ? ? ? ? ? |
> inside of SoC ? ? ? ? ?|
> -----------------------+--------------------------
> outside of SoC ? ? ? ? |
> ? ? ? ? ? ? ? ? ? ? ? | 3.clk?
> ? ? ? ? ? ? ? ? ? ? ? |
> ? ? ? ? ? ? ? +--------------+
> ? ? ? ? ? ? ? | LCD module ? |
> ? ? ? ? ? ? ? +--------------+
>
> In clock.c the clk 'lcd' means #1 in above diagram.
> So you mean clk 'lcd' is #3', maybe confused :-(
>

Uh.. yes, I meant #3 by the clk LCD.

This CLK_GATE_IP - FIMD gates both ACLK_FIMD and SCLK_FIMD at the same
time; thus, this should be over the MUX of SCLK_FIMD, I thought. (User
Manual: Clock Controler - Clock Gating Control Register
(CLK_GATE_IP1)) If we should have a struct clk that represents
ACLK_FIMD, the clock representing ACLK_FIMD should not directly
control CLK_GATE_IP1 as CLK_GATE_IP1 gates SCLK_FIMD as well if the
User Manual is correct.

If we really need to represent ACLK_FIMD separately, why don't we
create "aclk_fimd" and let "fimd" or "lcd" become a struct clksrc_clk
that chooses between aclk_fimd and sclk_fimd? How about this? (and
other similar clocks)

Anyway, with this feature, we may now think about calling
"clk_disable(parent)" when a clk is doing
"clk_set_parent(another_parent)" if the clk is "clk_enable"d. I'd be
happy to hear any suggestions and comments on this.

E.g.,

struct clk *a = clk_get("A");
clk_set_parent(a, clk_A);
clk_enable(a);
some ops with a;
/* we need another clock speed with different source */
clk_set_parent(a, clk_B);
some ops with b;

In this example, we, at least, need to enable clk_B when
clk_set_parent is called and we'd better clk_disable(clk_A) if
clk_enable(clk_A) is called at clk_enable(a);


>> Besides, we may need to allow clock-clksrc.c's s3c_register_clksrc
>> function to use the default .parent value to set the SRC register
>> accordingly. For example (a patch under construction, not uploaded yet
>> and requires patches submitted before):
>>
>>
>> Author: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Date: ? Thu Jul 8 18:34:01 2010 +0900
>>
>> ? ? ARM: SAMSUNG SoC: clksrc register uses parent value to set default src.
>>
>> ? ? If .parent field is filled when a struct clksrc_clk is registered,
>> ? ? s3c_register_clksrc sets the source clock register accordingly to the
>> ? ? parent clock.
>>
>> ? ? In the previous versions, the parent value was "ignored" because the
>> ? ? value of default register value overwrites the parent field.
>>
>> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
>> b/arch/arm/plat-samsung/clock-clksrc.c
>> index ae8b850..236fb59 100644
>> --- a/arch/arm/plat-samsung/clock-clksrc.c
>> +++ b/arch/arm/plat-samsung/clock-clksrc.c
>> @@ -177,7 +177,7 @@ static struct clk_ops clksrc_ops_nosrc = {
>>
>> ?void __init s3c_register_clksrc(struct clksrc_clk *clksrc, int size)
>> ?{
>> - ? ? ? int ret;
>> + ? ? ? int ret, err = 0;
>>
>> ? ? ? ? for (; size > 0; size--, clksrc++) {
>> ? ? ? ? ? ? ? ? if (!clksrc->reg_div.reg && !clksrc->reg_src.reg)
>> @@ -195,12 +195,25 @@ void __init s3c_register_clksrc(struct
>> clksrc_clk *clksrc, int size)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clksrc->clk.ops = &clksrc_ops;
>> ? ? ? ? ? ? ? ? }
>>
>> + ? ? ? ? ? ? ? if (clksrc->clk.parent && clksrc->sources &&
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clksrc->reg_src.reg) {
>> + ? ? ? ? ? ? ? ? ? ? ? err = s3c_setparent_clksrc(&clksrc->clk,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clksrc->clk.parent);
>> + ? ? ? ? ? ? ? ? ? ? ? if (err < 0)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "[%s:%d] setparent error
>> at reg"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ister_clksrc %s p
>> = %s...\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __FILE__, __LINE__,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clksrc->clk.name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clksrc->clk.parent->name);
>> + ? ? ? ? ? ? ? }
>> +
>> ? ? ? ? ? ? ? ? /* setup the clocksource, but do not announce it
>> ? ? ? ? ? ? ? ? ?* as it may be re-set by the setup routines
>> ? ? ? ? ? ? ? ? ?* called after the rest of the clocks have been
>> ? ? ? ? ? ? ? ? ?* registered
>> ? ? ? ? ? ? ? ? ?*/
>> - ? ? ? ? ? ? ? s3c_set_clksrc(clksrc, false);
>> + ? ? ? ? ? ? ? if (err < 0)
>> + ? ? ? ? ? ? ? ? ? ? ? s3c_set_clksrc(clksrc, false);
>>
>> ? ? ? ? ? ? ? ? ret = s3c24xx_register_clock(&clksrc->clk);
>>
>>
>>
>>
>> >
>> >> However, in order to make it look better, we may need to make LCD(FIMD)
>> easier
>> >> to switch between SCLK_ACLK and SCLK_FIMD; e.g., by making it another
>> >> clksrc_clk
>> >> with source clocks of SCLK_ACLK and SCLK_FIMD controlling with
>> CLK_GATE_IP.
>> >>
>> >> >
>> >> > As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
>> >> > SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
>> >> > CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.
>> >> >
>> >> >> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
>> >> >> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
>> >> >> one of clksrcs[] as its parent. (enum clksrc_refer)
>> >> >>
>> >> >> For example, "lcd" may set "sclk-fimd" as a parent if the following line
>> >> >> replaces the line 878:
>> >> >>
>> >> >> ? ? ? ? ? ? ? .parent ? ? ? ? = &clk_hclk_dsys.clk,
>> >> >>
>> >> >> to
>> >> >>
>> >> >> ? ? ? ? ? ? ? .parent ? ? ? ? = &clksrcs[_clksrc_sclk_fimd],
>> >> >>
>> >> >
>> >> >
>> >> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> >> >> ---
>> >> >> ?arch/arm/mach-s5pv210/clock.c | ?986
>> >> > ++++++++++++++++++++++-------------------
>> >> >> ?1 files changed, 520 insertions(+), 466 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> >> >> index aa2b1c5..e33c6ed 100644
>> >> >> --- a/arch/arm/mach-s5pv210/clock.c
>> >> >> +++ b/arch/arm/mach-s5pv210/clock.c
>> >> >> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
>> >> >> ? ? ? .reg_src ? ? ? ?= { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
>> >> >> ?};
>> >> >>
>> >> >
>> >> > And...hmm...maybe made this patch with wrong way...
>> >> >
>> >> >> +static struct clk *clkset_uart_list[] = {
>> >> >> + ? ? [6] = &clk_mout_mpll.clk,
>> >> >> + ? ? [7] = &clk_mout_epll.clk,
>> >> >> +};
>> >> >> +
>> >> >
>> >> > Following is from this patch...
>> >> >
>> >> > -static struct clk *clkset_uart_list[] = {
>> >> > - ? ? ? [6] = &clk_mout_mpll.clk,
>> >> > - ? ? ? [7] = &clk_mout_epll.clk,
>> >> > -};
>> >> > -
>> >> >
>> >> > Same? :-(
>> >> > Why did you remove and add same code?
>> >>
>> >> It is done so in order to make clksrc_clk to be able to be parents of
>> >> clk. (same with the
>> >> later parts)
>> >>
>> >> To do so, I moved clksrc_clk to the front of clk definitions, which in
>> >> turn moved
>> >> clock source list to the front of clksrc_clk.
>> >>
>> >> Or, in order to reduce the changed lines number, would it be better to declare
>> >> the list of struct clksrc_clk before and keep the locations of the lists?
>> >>
>> >>
>> >> >
>> >> > (snip)
>> >> >
>> >> > And...see the below.
>> >> >
>> >> > Added like below...
>> >> >
>> >> >> +static struct clksrc_clk clksrcs[] = {
>> >> >> + ? ? [_clksrc_sclk_dmc] = {
>> >> >> + ? ? ? ? ? ? .clk ? ?= {
>> >> >> + ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dmc",
>> >> >> + ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> >> + ? ? ? ? ? ? },
>> >> >> + ? ? ? ? ? ? .sources = &clkset_group1,
>> >> >> + ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> >> >> + ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> >> >> + ? ? },
>> >> >> + ? ? [_clksrc_sclk_onenand] = {
>> >> >> + ? ? ? ? ? ? .clk ? ?= {
>> >> >> + ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_onenand",
>> >> >> + ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> >> + ? ? ? ? ? ? },
>> >> >> + ? ? ? ? ? ? .sources = &clkset_sclk_onenand,
>> >> >> + ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
>> >> >> + ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
>> >> >> + ? ? },
>> >> >
>> >> > (snip)
>> >> >
>> >> > And removed like below...
>> >> >
>> >> >> -static struct clksrc_clk clksrcs[] = {
>> >> >> - ? ? {
>> >> >> - ? ? ? ? ? ? .clk ? ?= {
>> >> >> - ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dmc",
>> >> >> - ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> >> - ? ? ? ? ? ? },
>> >> >> - ? ? ? ? ? ? .sources = &clkset_group1,
>> >> >> - ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> >> >> - ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> >> >> - ? ? }, {
>> >> >> - ? ? ? ? ? ? .clk ? ?= {
>> >> >> - ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_onenand",
>> >> >> - ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> >> >> - ? ? ? ? ? ? },
>> >> >> - ? ? ? ? ? ? .sources = &clkset_sclk_onenand,
>> >> >> - ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
>> >> >> - ? ? ? ? ? ? .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
>> >> >> - ? ? }, {
>> >> >
>> >> > But only changed small lines not every.
>> >> > Following is from my local machine.
>> >> >
>> >> > @@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
>> >> > ?};
>> >> >
>> >> > ?static struct clksrc_clk clksrcs[] = {
>> >> > - ? ? ? {
>> >> > + ? ? ? [_clksrc_sclk_dmc] = {
>> >> > ? ? ? ? ? ? ? ?.clk ? ?= {
>> >> > ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_dmc",
>> >> > ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = -1,
>> >> > @@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
>> >> > ? ? ? ? ? ? ? ?.sources = &clkset_group1,
>> >> > ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
>> >> > ? ? ? ? ? ? ? ?.reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
>> >> > - ? ? ? }, {
>> >> > + ? ? ? },
>> >> > + ? ? ? [_clksrc_sclk_onenand] = {
>> >> > ? ? ? ? ? ? ? ?.clk ? ?= {
>> >> > ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_onenand",
>> >> > ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = -1,
>> >> > ?(snip)
>> >> >
>> >> > Please make sure your patch has no problem before submitting.
>> >> >
>> >> > Thanks.
>> >> >
>> >> > Best regards,
>> >> > Kgene.
>> >> > --
>> >> > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
>> >> > SW Solution Development Team, Samsung Electronics Co., Ltd.
>> >> >
>> >> >
>> >
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Kukjin Kim]

MyungJoo Ham wrote:
> 

 (snip)

> >>
> > Please refer to below diagram.
> >
> > ------------------------------------
> >           dsys bus
> > ----------------+-------------------
> >                |
> >                |1.clk 'lcd'
> >                |
> >            +---+-----------+
> >            |   | FIMD block|
> >            | +-+           |
> >            | |             |
> >            | | |\          |
> >            | +-|m|         |
> > 2.SCLK_FIMD |   |u|----+    |
> > ------------+---|x|    |    |
> >            |   |/     |    |
> >            |          |    |
> >            +----------+----+
> >                       |
> > inside of SoC          |
> > -----------------------+--------------------------
> > outside of SoC         |
> >                       | 3.clk?
> >                       |
> >               +--------------+
> >               | LCD module   |
> >               +--------------+
> >
> > In clock.c the clk 'lcd' means #1 in above diagram.
> > So you mean clk 'lcd' is #3', maybe confused :-(
> >
> 
> Uh.. yes, I meant #3 by the clk LCD.
> 
> This CLK_GATE_IP - FIMD gates both ACLK_FIMD and SCLK_FIMD at the same
> time; thus, this should be over the MUX of SCLK_FIMD, I thought. (User
> Manual: Clock Controler - Clock Gating Control Register
> (CLK_GATE_IP1)) If we should have a struct clk that represents
> ACLK_FIMD, the clock representing ACLK_FIMD should not directly
> control CLK_GATE_IP1 as CLK_GATE_IP1 gates SCLK_FIMD as well if the
> User Manual is correct.
> 
> If we really need to represent ACLK_FIMD separately, why don't we
> create "aclk_fimd" and let "fimd" or "lcd" become a struct clksrc_clk
> that chooses between aclk_fimd and sclk_fimd? How about this? (and
> other similar clocks)
> 
> Anyway, with this feature, we may now think about calling
> "clk_disable(parent)" when a clk is doing
> "clk_set_parent(another_parent)" if the clk is "clk_enable"d. I'd be
> happy to hear any suggestions and comments on this.

Basically, each clock which is used in each device driver such as lcd
module clock, camemra module clock should be controlled in its device
driver.

Let's say, in the case of camera module clock, FIMC IP has a specific
divider which is used clock dividing for camera module clock. And the
divider which is in FIMC IP has own rate for camera module at that time. So
cannot/no need to define it in the common clock part such as clock.c.

In other words, can't implement set_rate() or get_rate() for camera module
clock in the common clock part, because FIMC IP block has own specific
divider for it.

> 
> E.g.,
> 
> struct clk *a = clk_get("A");
> clk_set_parent(a, clk_A);
> clk_enable(a);
> some ops with a;
> /* we need another clock speed with different source */
> clk_set_parent(a, clk_B);
> some ops with b;
> 
> In this example, we, at least, need to enable clk_B when
> clk_set_parent is called and we'd better clk_disable(clk_A) if
> clk_enable(clk_A) is called at clk_enable(a);
> 
> 

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

(snip)

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Kyungmin Park]

On Tue, Jul 13, 2010 at 1:21 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>
> ?(snip)
>
>> >>
>> > Please refer to below diagram.
>> >
>> > ------------------------------------
>> > ? ? ? ? ? dsys bus
>> > ----------------+-------------------
>> > ? ? ? ? ? ? ? ?|
>> > ? ? ? ? ? ? ? ?|1.clk 'lcd'
>> > ? ? ? ? ? ? ? ?|
>> > ? ? ? ? ? ?+---+-----------+
>> > ? ? ? ? ? ?| ? | FIMD block|
>> > ? ? ? ? ? ?| +-+ ? ? ? ? ? |
>> > ? ? ? ? ? ?| | ? ? ? ? ? ? |
>> > ? ? ? ? ? ?| | |\ ? ? ? ? ?|
>> > ? ? ? ? ? ?| +-|m| ? ? ? ? |
>> > 2.SCLK_FIMD | ? |u|----+ ? ?|
>> > ------------+---|x| ? ?| ? ?|
>> > ? ? ? ? ? ?| ? |/ ? ? | ? ?|
>> > ? ? ? ? ? ?| ? ? ? ? ?| ? ?|
>> > ? ? ? ? ? ?+----------+----+
>> > ? ? ? ? ? ? ? ? ? ? ? |
>> > inside of SoC ? ? ? ? ?|
>> > -----------------------+--------------------------
>> > outside of SoC ? ? ? ? |
>> > ? ? ? ? ? ? ? ? ? ? ? | 3.clk?
>> > ? ? ? ? ? ? ? ? ? ? ? |
>> > ? ? ? ? ? ? ? +--------------+
>> > ? ? ? ? ? ? ? | LCD module ? |
>> > ? ? ? ? ? ? ? +--------------+
>> >
>> > In clock.c the clk 'lcd' means #1 in above diagram.
>> > So you mean clk 'lcd' is #3', maybe confused :-(
>> >
>>
>> Uh.. yes, I meant #3 by the clk LCD.
>>
>> This CLK_GATE_IP - FIMD gates both ACLK_FIMD and SCLK_FIMD at the same
>> time; thus, this should be over the MUX of SCLK_FIMD, I thought. (User
>> Manual: Clock Controler - Clock Gating Control Register
>> (CLK_GATE_IP1)) If we should have a struct clk that represents
>> ACLK_FIMD, the clock representing ACLK_FIMD should not directly
>> control CLK_GATE_IP1 as CLK_GATE_IP1 gates SCLK_FIMD as well if the
>> User Manual is correct.
>>
>> If we really need to represent ACLK_FIMD separately, why don't we
>> create "aclk_fimd" and let "fimd" or "lcd" become a struct clksrc_clk
>> that chooses between aclk_fimd and sclk_fimd? How about this? (and
>> other similar clocks)
>>
>> Anyway, with this feature, we may now think about calling
>> "clk_disable(parent)" when a clk is doing
>> "clk_set_parent(another_parent)" if the clk is "clk_enable"d. I'd be
>> happy to hear any suggestions and comments on this.
>
> Basically, each clock which is used in each device driver such as lcd
> module clock, camemra module clock should be controlled in its device
> driver.
>
> Let's say, in the case of camera module clock, FIMC IP has a specific
> divider which is used clock dividing for camera module clock. And the
> divider which is in FIMC IP has own rate for camera module at that time. So
> cannot/no need to define it in the common clock part such as clock.c.
>
> In other words, can't implement set_rate() or get_rate() for camera module
> clock in the common clock part, because FIMC IP block has own specific
> divider for it.

To Pawel,
Can you reply his comments? usage of camera clock.

Thank you,
Kyungmin Park
>
>>
>> E.g.,
>>
>> struct clk *a = clk_get("A");
>> clk_set_parent(a, clk_A);
>> clk_enable(a);
>> some ops with a;
>> /* we need another clock speed with different source */
>> clk_set_parent(a, clk_B);
>> some ops with b;
>>
>> In this example, we, at least, need to enable clk_B when
>> clk_set_parent is called and we'd better clk_disable(clk_A) if
>> clk_enable(clk_A) is called at clk_enable(a);
>>
>>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
> (snip)
>
>

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Sylwester Nawrocki]

Hello,

On Tue, 13 Jul 2010 13:21:54 +0900, Kukjin Kim wrote:

> MyungJoo Ham wrote:
>> 
>> 
>  (snip)
> 
[snip]
> 
> Basically, each clock which is used in each device driver such as lcd
> module clock, camemra module clock should be controlled in its device
> driver.
> 
> Let's say, in the case of camera module clock, FIMC IP has a specific
> divider which is used clock dividing for camera module clock. And the
> divider which is in FIMC IP has own rate for camera module at that time.
> So cannot/no need to define it in the common clock part such as clock.c.

IMO FIMC clock divider control registers are located in the clock 
controller IO memory, not in the FIMC IP. But possibly you meant 
something different than SCLK_CAM0/1 here.

So if we are considering SCLK_CAM0/1 the FIMC driver should be able to 
configure dividers corresponding to these clock outputs through the clk 
API. 
There is core clock multiplexer in FIMC IP in some SoCs though, so in 
general you are right, it would not be possible to control all clocks
with clk API as for now, since the dividers are located in specific IPs 
(e.g. SDHCI controller, FIMD).

Regards,
Sylwester

> 
> In other words, can't implement set_rate() or get_rate() for camera
> module clock in the common clock part, because FIMC IP block has own
> specific divider for it.
> 
> 
[snip]

> Thanks.
> 
> Best regards,
> Kgene.

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[Marek Szyprowski]

Hello,

On Tuesday, July 13, 2010 6:22 AM Kukjin Kim wrote:

> MyungJoo Ham wrote:
>  (snip)
> > > Please refer to below diagram.
> > >
> > > ------------------------------------
> > >           dsys bus
> > > ----------------+-------------------
> > >                |
> > >                |1.clk 'lcd'
> > >                |
> > >            +---+-----------+
> > >            |   | FIMD block|
> > >            | +-+           |
> > >            | |             |
> > >            | | |\          |
> > >            | +-|m|         |
> > > 2.SCLK_FIMD |   |u|----+    |
> > > ------------+---|x|    |    |
> > >            |   |/     |    |
> > >            |          |    |
> > >            +----------+----+
> > >                       |
> > > inside of SoC          |
> > > -----------------------+--------------------------
> > > outside of SoC         |
> > >                       | 3.clk?
> > >                       |
> > >               +--------------+
> > >               | LCD module   |
> > >               +--------------+
> > >
> > > In clock.c the clk 'lcd' means #1 in above diagram.
> > > So you mean clk 'lcd' is #3', maybe confused :-(
> > >
> >
> > Uh.. yes, I meant #3 by the clk LCD.
> >
> > This CLK_GATE_IP - FIMD gates both ACLK_FIMD and SCLK_FIMD at the same
> > time; thus, this should be over the MUX of SCLK_FIMD, I thought. (User
> > Manual: Clock Controler - Clock Gating Control Register
> > (CLK_GATE_IP1)) If we should have a struct clk that represents
> > ACLK_FIMD, the clock representing ACLK_FIMD should not directly
> > control CLK_GATE_IP1 as CLK_GATE_IP1 gates SCLK_FIMD as well if the
> > User Manual is correct.
> >
> > If we really need to represent ACLK_FIMD separately, why don't we
> > create "aclk_fimd" and let "fimd" or "lcd" become a struct clksrc_clk
> > that chooses between aclk_fimd and sclk_fimd? How about this? (and
> > other similar clocks)
> >
> > Anyway, with this feature, we may now think about calling
> > "clk_disable(parent)" when a clk is doing
> > "clk_set_parent(another_parent)" if the clk is "clk_enable"d. I'd be
> > happy to hear any suggestions and comments on this.
> 
> Basically, each clock which is used in each device driver such as lcd
> module clock, camera module clock should be controlled in its device
> driver.
> 
> Let's say, in the case of camera module clock, FIMC IP has a specific
> divider which is used clock dividing for camera module clock. And the
> divider which is in FIMC IP has own rate for camera module at that time. So
> cannot/no need to define it in the common clock part such as clock.c.
> 
> In other words, can't implement set_rate() or get_rate() for camera module
> clock in the common clock part, because FIMC IP block has own specific
> divider for it.

Well, this 'problem' occurs at least in the following IP blocks: FIMD, FIMC
and SDHCI. It looks that some common solution is required for these drivers.
In all these 3 IPs the clock we are considering is an 'external' clock - it
is wired out of the IP block to its external peripheral device (lcd display,
camera sensor or mmc/sd card).

In the current drivers that are in mainline, only SDHCI driver changes the
parent clock source and/or the rate. FIMD (s3c-fb) is hardcoded to use host
bus (dsys) clock only (what is a limitation that might be an issue in some
cases).

We can notice that the code for selecting the parent clock in sdhci-s3c is
a bit messy. The parent clock names are passed in platform data, which in
turn need to be defined separately for each SoC type.

IMHO it should be possible to create a some kind of virtual clock, lets name 
it 'ext_hsmmc' (or 'external_hsmmc'). All it's parents should be properly
defined in the clock api. In this case methods like set_rate should check all
possible sources and select the one that gives the closest result, enabling
and disabling relevant parents. This virtual clock might need to have some
additional rate related ops (like get_min and get_max) and some function for
the driver to get the calculated divider and/or parent index (the driver
needs to write these to its registers, which clock api cannot access
directly).

This way we would get 2 clocks:
- a general 'hsmmc', which can be used for enabling and disabling the IP (dsys
  sourced)
- special 'external_hsmmc', which is used for controlling the clk signal
outgoing
  from SDHCI block.

The driver will need to get and enable these 2 clocks, but I don't think it is a
big overhead. Most of the code that is related to 'external' clock should be
there
already and will be simplified with this approach.

What do you think about such solution?

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH] ARM: S5PV210: allow clk to use clksrc as parents

[MyungJoo Ham]

Hello,

On Tue, Jul 13, 2010 at 1:21 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>
> ?(snip)
>
>> >>
>> > Please refer to below diagram.
>> >
>> > ------------------------------------
>> > ? ? ? ? ? dsys bus
>> > ----------------+-------------------
>> > ? ? ? ? ? ? ? ?|
>> > ? ? ? ? ? ? ? ?|1.clk 'lcd'
>> > ? ? ? ? ? ? ? ?|
>> > ? ? ? ? ? ?+---+-----------+
>> > ? ? ? ? ? ?| ? | FIMD block|
>> > ? ? ? ? ? ?| +-+ ? ? ? ? ? |
>> > ? ? ? ? ? ?| | ? ? ? ? ? ? |
>> > ? ? ? ? ? ?| | |\ ? ? ? ? ?|
>> > ? ? ? ? ? ?| +-|m| ? ? ? ? |
>> > 2.SCLK_FIMD | ? |u|----+ ? ?|
>> > ------------+---|x| ? ?| ? ?|
>> > ? ? ? ? ? ?| ? |/ ? ? | ? ?|
>> > ? ? ? ? ? ?| ? ? ? ? ?| ? ?|
>> > ? ? ? ? ? ?+----------+----+
>> > ? ? ? ? ? ? ? ? ? ? ? |
>> > inside of SoC ? ? ? ? ?|
>> > -----------------------+--------------------------
>> > outside of SoC ? ? ? ? |
>> > ? ? ? ? ? ? ? ? ? ? ? | 3.clk?
>> > ? ? ? ? ? ? ? ? ? ? ? |
>> > ? ? ? ? ? ? ? +--------------+
>> > ? ? ? ? ? ? ? | LCD module ? |
>> > ? ? ? ? ? ? ? +--------------+
>> >
>> > In clock.c the clk 'lcd' means #1 in above diagram.
>> > So you mean clk 'lcd' is #3', maybe confused :-(
>> >
>>
>> Uh.. yes, I meant #3 by the clk LCD.
>>
>> This CLK_GATE_IP - FIMD gates both ACLK_FIMD and SCLK_FIMD at the same
>> time; thus, this should be over the MUX of SCLK_FIMD, I thought. (User
>> Manual: Clock Controler - Clock Gating Control Register
>> (CLK_GATE_IP1)) If we should have a struct clk that represents
>> ACLK_FIMD, the clock representing ACLK_FIMD should not directly
>> control CLK_GATE_IP1 as CLK_GATE_IP1 gates SCLK_FIMD as well if the
>> User Manual is correct.
>>
>> If we really need to represent ACLK_FIMD separately, why don't we
>> create "aclk_fimd" and let "fimd" or "lcd" become a struct clksrc_clk
>> that chooses between aclk_fimd and sclk_fimd? How about this? (and
>> other similar clocks)
>>
>> Anyway, with this feature, we may now think about calling
>> "clk_disable(parent)" when a clk is doing
>> "clk_set_parent(another_parent)" if the clk is "clk_enable"d. I'd be
>> happy to hear any suggestions and comments on this.
>
> Basically, each clock which is used in each device driver such as lcd
> module clock, camemra module clock should be controlled in its device
> driver.
>
> Let's say, in the case of camera module clock, FIMC IP has a specific
> divider which is used clock dividing for camera module clock. And the
> divider which is in FIMC IP has own rate for camera module at that time. So
> cannot/no need to define it in the common clock part such as clock.c.
>
> In other words, can't implement set_rate() or get_rate() for camera module
> clock in the common clock part, because FIMC IP block has own specific
> divider for it.
>

Ah... I've got it. Although "lcd" of CLK_GATE_IP contols SCLK_FIMD and
part of HCLK that goes to FIMD at the same time, as long as the MUX of
SCLK_FIMD and that part of HCLK is in VIDCON (VIDCON0:CLKSEL_F), you
don't want the control to be reside in arch/arm/mach side.

I also think let LCD(anyone who's got w/ VIDCON0) driver controls and
owns the MUX info about SCLK_FIMD/HCLK. What I thought once is that
CLK_GATE_IP:CLK_FIMD controls the lcd clock after the MUX as it
controls both ACLK_FIMD(from HCLK) and SCLK_FIMD at the same time.


Thank you.

>>
>> E.g.,
>>
>> struct clk *a = clk_get("A");
>> clk_set_parent(a, clk_A);
>> clk_enable(a);
>> some ops with a;
>> /* we need another clock speed with different source */
>> clk_set_parent(a, clk_B);
>> some ops with b;
>>
>> In this example, we, at least, need to enable clk_B when
>> clk_set_parent is called and we'd better clk_disable(clk_A) if
>> clk_enable(clk_A) is called at clk_enable(a);
>>
>>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
> (snip)
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858