[PATCH] S5PV210 Correct clock source control register properties

[PATCH] S5PV210 Correct clock source control register properties

[MyungJoo Ham]

The clock source controls (struct clksrc_clk) have been often accessing
CLK_GATE_IPx, which are for clock gating and are accessed by each clock
as well as the clock source. This duplicated clock issue may incur
lockup problems when there are two modules accessing the same clock with
different names.

Besides, the clock source control is supposed to control (setting values
of MASK, SRC, DIV), not to turn on/off individual clock.

Another point is that there are registers (CLK_SRC_MASK0/1) specialized
for masking clock source control in S5PV210/S5PC110 according to the
user manual (rev 20100201).

Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
CLK_GATE_IPx) for the clock source control is safer and fits to the
semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
except for MFC, G2D, and G3D.

In this patch,

- DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.

- CAM 0/1 did not have enable/disable control. They now access
  CLK_SRC_MASK0.

- MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
  defined to control MFC, G3D, and G2D, we kept them to access
CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
later.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
---
 arch/arm/mach-s5pv210/clock.c |  101 ++++++++++++++++++++++-------------------
 1 files changed, 55 insertions(+), 46 deletions(-)

diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
index ec5ad8c..08c1063 100644
--- a/arch/arm/mach-s5pv210/clock.c
+++ b/arch/arm/mach-s5pv210/clock.c
@@ -183,6 +183,11 @@ static int s5pv210_clk_mask0_ctrl(struct clk *clk, int enable)
 	return s5p_gatectrl(S5P_CLK_SRC_MASK0, clk, enable);
 }
 
+static int s5pv210_clk_mask1_ctrl(struct clk *clk, int enable)
+{
+	return s5p_gatectrl(S5P_CLK_SRC_MASK1, clk, enable);
+}
+
 static struct clk clk_sclk_hdmi27m = {
 	.name		= "sclk_hdmi27m",
 	.id		= -1,
@@ -497,8 +502,8 @@ static struct clksrc_clk clk_sclk_dac = {
 	.clk		= {
 		.name		= "sclk_dac",
 		.id		= -1,
-		.ctrlbit	= (1 << 10),
-		.enable		= s5pv210_clk_ip1_ctrl,
+		.ctrlbit	= (1 << 2),
+		.enable		= s5pv210_clk_mask0_ctrl,
 	},
 	.sources	= &clkset_sclk_dac,
 	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 8, .size = 1 },
@@ -527,8 +532,8 @@ static struct clksrc_clk clk_sclk_hdmi = {
 	.clk		= {
 		.name		= "sclk_hdmi",
 		.id		= -1,
-		.enable		= s5pv210_clk_ip1_ctrl,
-		.ctrlbit	= (1 << 11),
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 0),
 	},
 	.sources	= &clkset_sclk_hdmi,
 	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 0, .size = 1 },
@@ -565,8 +570,8 @@ static struct clksrc_clk clk_sclk_audio0 = {
 	.clk		= {
 		.name		= "sclk_audio",
 		.id		= 0,
-		.enable		= s5pv210_clk_ip3_ctrl,
-		.ctrlbit	= (1 << 4),
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 24),
 	},
 	.sources = &clkset_sclk_audio0,
 	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 0, .size = 4 },
@@ -594,8 +599,8 @@ static struct clksrc_clk clk_sclk_audio1 = {
 	.clk		= {
 		.name		= "sclk_audio",
 		.id		= 1,
-		.enable		= s5pv210_clk_ip3_ctrl,
-		.ctrlbit	= (1 << 5),
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 25),
 	},
 	.sources = &clkset_sclk_audio1,
 	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 4, .size = 4 },
@@ -623,8 +628,8 @@ static struct clksrc_clk clk_sclk_audio2 = {
 	.clk		= {
 		.name		= "sclk_audio",
 		.id		= 2,
-		.enable		= s5pv210_clk_ip3_ctrl,
-		.ctrlbit	= (1 << 6),
+		.enable		= s5pv210_clk_mask0_ctrl,
+		.ctrlbit	= (1 << 26),
 	},
 	.sources = &clkset_sclk_audio2,
 	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 8, .size = 4 },
@@ -680,8 +685,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk	= {
 			.name		= "uclk1",
 			.id		= 0,
-			.ctrlbit	= (1<<17),
-			.enable		= s5pv210_clk_ip3_ctrl,
+			.ctrlbit	= (1 << 12),
+			.enable		= s5pv210_clk_mask0_ctrl,
 		},
 		.sources = &clkset_uart,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 16, .size = 4 },
@@ -690,8 +695,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "uclk1",
 			.id		= 1,
-			.enable		= s5pv210_clk_ip3_ctrl,
-			.ctrlbit	= (1 << 18),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 13),
 		},
 		.sources = &clkset_uart,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 20, .size = 4 },
@@ -700,8 +705,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "uclk1",
 			.id		= 2,
-			.enable		= s5pv210_clk_ip3_ctrl,
-			.ctrlbit	= (1 << 19),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 14),
 		},
 		.sources = &clkset_uart,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 24, .size = 4 },
@@ -710,8 +715,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "uclk1",
 			.id		= 3,
-			.enable		= s5pv210_clk_ip3_ctrl,
-			.ctrlbit	= (1 << 20),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 15),
 		},
 		.sources = &clkset_uart,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 28, .size = 4 },
@@ -720,8 +725,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk	= {
 			.name		= "sclk_mixer",
 			.id		= -1,
-			.enable		= s5pv210_clk_ip1_ctrl,
-			.ctrlbit	= (1 << 9),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 1),
 		},
 		.sources = &clkset_sclk_mixer,
 		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 4, .size = 1 },
@@ -738,8 +743,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk	= {
 			.name		= "sclk_fimc",
 			.id		= 0,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 24),
+			.enable		= s5pv210_clk_mask1_ctrl,
+			.ctrlbit	= (1 << 2),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 12, .size = 4 },
@@ -748,8 +753,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk	= {
 			.name		= "sclk_fimc",
 			.id		= 1,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 25),
+			.enable		= s5pv210_clk_mask1_ctrl,
+			.ctrlbit	= (1 << 3),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 16, .size = 4 },
@@ -758,8 +763,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk	= {
 			.name		= "sclk_fimc",
 			.id		= 2,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 26),
+			.enable		= s5pv210_clk_mask1_ctrl,
+			.ctrlbit	= (1 << 4),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 20, .size = 4 },
@@ -768,6 +773,8 @@ static struct clksrc_clk clksrcs[] = {
 		.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 },
@@ -776,6 +783,8 @@ static struct clksrc_clk clksrcs[] = {
 		.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 },
@@ -784,8 +793,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_fimd",
 			.id		= -1,
-			.enable		= s5pv210_clk_ip1_ctrl,
-			.ctrlbit	= (1 << 0),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 5),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 20, .size = 4 },
@@ -794,8 +803,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 0,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit	= (1 << 16),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 8),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
@@ -804,8 +813,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 1,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit	= (1 << 17),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 9),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
@@ -814,8 +823,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 2,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit	= (1 << 18),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 10),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
@@ -824,8 +833,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 3,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit	= (1 << 19),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 11),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 },
@@ -864,8 +873,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_csis",
 			.id		= -1,
-			.enable		= s5pv210_clk_ip0_ctrl,
-			.ctrlbit	= (1 << 31),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 6),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 24, .size = 4 },
@@ -874,8 +883,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_spi",
 			.id		= 0,
-			.enable		= s5pv210_clk_ip3_ctrl,
-			.ctrlbit	= (1 << 12),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 16),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 0, .size = 4 },
@@ -884,8 +893,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_spi",
 			.id		= 1,
-			.enable		= s5pv210_clk_ip3_ctrl,
-			.ctrlbit	= (1 << 13),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 17),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 4, .size = 4 },
@@ -894,8 +903,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_pwi",
 			.id		= -1,
-			.enable		= &s5pv210_clk_ip4_ctrl,
-			.ctrlbit	= (1 << 2),
+			.enable		= &s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 29),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 20, .size = 4 },
@@ -904,8 +913,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_pwm",
 			.id		= -1,
-			.enable		= s5pv210_clk_ip3_ctrl,
-			.ctrlbit	= (1 << 23),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit	= (1 << 19),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 12, .size = 4 },
-- 
1.6.3.3

[PATCH] S5PV210 Correct clock source control register properties

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> The clock source controls (struct clksrc_clk) have been often accessing
> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
> as well as the clock source. This duplicated clock issue may incur
> lockup problems when there are two modules accessing the same clock with
> different names.
> 
Actually, S5V210/S5PC110 CLK_GATE_IPx can disable the clock operation of each
IP if it is not required. And this reduces dynamic power. CLK_SRC_MASKx is
used to control clock mux of each IP's SCLK.

Basically CLK_SRC_MASKx is for mux control (output masking) of each IP
source, and actually that can _not_ control relevant every source clock of
each IP. So CLK_GATE_IPx exists, can do it.

I know, in the several case, i.e, camera and fimc, each sclk_xxx need to
control. And can be controlled each sclk_xxx by using your patch, but in this
case, need to additional control relevant clock gating for dynamic clock
gating of each IP in its device driver.

> Besides, the clock source control is supposed to control (setting values
> of MASK, SRC, DIV), not to turn on/off individual clock.
> 
> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
> for masking clock source control in S5PV210/S5PC110 according to the
> user manual (rev 20100201).
> 
> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
> CLK_GATE_IPx) for the clock source control is safer and fits to the
> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
> except for MFC, G2D, and G3D.
> 
> In this patch,
> 
> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
> 
> - CAM 0/1 did not have enable/disable control. They now access
>   CLK_SRC_MASK0.
> 
> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>   defined to control MFC, G3D, and G2D, we kept them to access
> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
> later.
> 
Hmm..need to check about multimedia usage.

And in my opinion, it would be helpful to understand that should be short and
clear.

> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
>  arch/arm/mach-s5pv210/clock.c |  101
++++++++++++++++++++++-------------------
>  1 files changed, 55 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> index ec5ad8c..08c1063 100644
> --- a/arch/arm/mach-s5pv210/clock.c
> +++ b/arch/arm/mach-s5pv210/clock.c
> @@ -183,6 +183,11 @@ static int s5pv210_clk_mask0_ctrl(struct clk *clk, int
enable)
>  	return s5p_gatectrl(S5P_CLK_SRC_MASK0, clk, enable);
>  }
> 
> +static int s5pv210_clk_mask1_ctrl(struct clk *clk, int enable)
> +{
> +	return s5p_gatectrl(S5P_CLK_SRC_MASK1, clk, enable);
> +}
> +
>  static struct clk clk_sclk_hdmi27m = {
>  	.name		= "sclk_hdmi27m",
>  	.id		= -1,
> @@ -497,8 +502,8 @@ static struct clksrc_clk clk_sclk_dac = {
>  	.clk		= {
>  		.name		= "sclk_dac",
>  		.id		= -1,
> -		.ctrlbit	= (1 << 10),
> -		.enable		= s5pv210_clk_ip1_ctrl,
> +		.ctrlbit	= (1 << 2),
> +		.enable		= s5pv210_clk_mask0_ctrl,
>  	},
>  	.sources	= &clkset_sclk_dac,
>  	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 8, .size = 1 },
> @@ -527,8 +532,8 @@ static struct clksrc_clk clk_sclk_hdmi = {
>  	.clk		= {
>  		.name		= "sclk_hdmi",
>  		.id		= -1,
> -		.enable		= s5pv210_clk_ip1_ctrl,
> -		.ctrlbit	= (1 << 11),
> +		.enable		= s5pv210_clk_mask0_ctrl,
> +		.ctrlbit	= (1 << 0),
>  	},
>  	.sources	= &clkset_sclk_hdmi,
>  	.reg_src	= { .reg = S5P_CLK_SRC1, .shift = 0, .size = 1 },
> @@ -565,8 +570,8 @@ static struct clksrc_clk clk_sclk_audio0 = {
>  	.clk		= {
>  		.name		= "sclk_audio",
>  		.id		= 0,
> -		.enable		= s5pv210_clk_ip3_ctrl,
> -		.ctrlbit	= (1 << 4),
> +		.enable		= s5pv210_clk_mask0_ctrl,
> +		.ctrlbit	= (1 << 24),
>  	},
>  	.sources = &clkset_sclk_audio0,
>  	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 0, .size = 4 },
> @@ -594,8 +599,8 @@ static struct clksrc_clk clk_sclk_audio1 = {
>  	.clk		= {
>  		.name		= "sclk_audio",
>  		.id		= 1,
> -		.enable		= s5pv210_clk_ip3_ctrl,
> -		.ctrlbit	= (1 << 5),
> +		.enable		= s5pv210_clk_mask0_ctrl,
> +		.ctrlbit	= (1 << 25),
>  	},
>  	.sources = &clkset_sclk_audio1,
>  	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 4, .size = 4 },
> @@ -623,8 +628,8 @@ static struct clksrc_clk clk_sclk_audio2 = {
>  	.clk		= {
>  		.name		= "sclk_audio",
>  		.id		= 2,
> -		.enable		= s5pv210_clk_ip3_ctrl,
> -		.ctrlbit	= (1 << 6),
> +		.enable		= s5pv210_clk_mask0_ctrl,
> +		.ctrlbit	= (1 << 26),
>  	},
>  	.sources = &clkset_sclk_audio2,
>  	.reg_src = { .reg = S5P_CLK_SRC6, .shift = 8, .size = 4 },
> @@ -680,8 +685,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk	= {
>  			.name		= "uclk1",
>  			.id		= 0,
> -			.ctrlbit	= (1<<17),
> -			.enable		= s5pv210_clk_ip3_ctrl,
> +			.ctrlbit	= (1 << 12),
> +			.enable		= s5pv210_clk_mask0_ctrl,
>  		},
>  		.sources = &clkset_uart,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 16, .size = 4 },
> @@ -690,8 +695,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "uclk1",
>  			.id		= 1,
> -			.enable		= s5pv210_clk_ip3_ctrl,
> -			.ctrlbit	= (1 << 18),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 13),
>  		},
>  		.sources = &clkset_uart,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 20, .size = 4 },
> @@ -700,8 +705,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "uclk1",
>  			.id		= 2,
> -			.enable		= s5pv210_clk_ip3_ctrl,
> -			.ctrlbit	= (1 << 19),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 14),
>  		},
>  		.sources = &clkset_uart,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 24, .size = 4 },
> @@ -710,8 +715,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "uclk1",
>  			.id		= 3,
> -			.enable		= s5pv210_clk_ip3_ctrl,
> -			.ctrlbit	= (1 << 20),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 15),
>  		},
>  		.sources = &clkset_uart,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 28, .size = 4 },
> @@ -720,8 +725,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk	= {
>  			.name		= "sclk_mixer",
>  			.id		= -1,
> -			.enable		= s5pv210_clk_ip1_ctrl,
> -			.ctrlbit	= (1 << 9),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 1),
>  		},
>  		.sources = &clkset_sclk_mixer,
>  		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 4, .size = 1 },
> @@ -738,8 +743,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk	= {
>  			.name		= "sclk_fimc",
>  			.id		= 0,
> -			.enable		= s5pv210_clk_ip0_ctrl,
> -			.ctrlbit	= (1 << 24),
> +			.enable		= s5pv210_clk_mask1_ctrl,
> +			.ctrlbit	= (1 << 2),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 12, .size = 4 },
> @@ -748,8 +753,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk	= {
>  			.name		= "sclk_fimc",
>  			.id		= 1,
> -			.enable		= s5pv210_clk_ip0_ctrl,
> -			.ctrlbit	= (1 << 25),
> +			.enable		= s5pv210_clk_mask1_ctrl,
> +			.ctrlbit	= (1 << 3),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 16, .size = 4 },
> @@ -758,8 +763,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk	= {
>  			.name		= "sclk_fimc",
>  			.id		= 2,
> -			.enable		= s5pv210_clk_ip0_ctrl,
> -			.ctrlbit	= (1 << 26),
> +			.enable		= s5pv210_clk_mask1_ctrl,
> +			.ctrlbit	= (1 << 4),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC3, .shift = 20, .size = 4 },
> @@ -768,6 +773,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.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 },
> @@ -776,6 +783,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.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 },
> @@ -784,8 +793,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_fimd",
>  			.id		= -1,
> -			.enable		= s5pv210_clk_ip1_ctrl,
> -			.ctrlbit	= (1 << 0),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 5),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 20, .size = 4 },
> @@ -794,8 +803,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_mmc",
>  			.id		= 0,
> -			.enable		= s5pv210_clk_ip2_ctrl,
> -			.ctrlbit	= (1 << 16),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 8),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
> @@ -804,8 +813,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_mmc",
>  			.id		= 1,
> -			.enable		= s5pv210_clk_ip2_ctrl,
> -			.ctrlbit	= (1 << 17),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 9),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
> @@ -814,8 +823,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_mmc",
>  			.id		= 2,
> -			.enable		= s5pv210_clk_ip2_ctrl,
> -			.ctrlbit	= (1 << 18),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 10),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
> @@ -824,8 +833,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_mmc",
>  			.id		= 3,
> -			.enable		= s5pv210_clk_ip2_ctrl,
> -			.ctrlbit	= (1 << 19),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 11),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 },
> @@ -864,8 +873,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_csis",
>  			.id		= -1,
> -			.enable		= s5pv210_clk_ip0_ctrl,
> -			.ctrlbit	= (1 << 31),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 6),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC1, .shift = 24, .size = 4 },
> @@ -874,8 +883,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_spi",
>  			.id		= 0,
> -			.enable		= s5pv210_clk_ip3_ctrl,
> -			.ctrlbit	= (1 << 12),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 16),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 0, .size = 4 },
> @@ -884,8 +893,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_spi",
>  			.id		= 1,
> -			.enable		= s5pv210_clk_ip3_ctrl,
> -			.ctrlbit	= (1 << 13),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 17),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 4, .size = 4 },
> @@ -894,8 +903,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_pwi",
>  			.id		= -1,
> -			.enable		= &s5pv210_clk_ip4_ctrl,
> -			.ctrlbit	= (1 << 2),
> +			.enable		= &s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 29),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC6, .shift = 20, .size = 4 },
> @@ -904,8 +913,8 @@ static struct clksrc_clk clksrcs[] = {
>  		.clk		= {
>  			.name		= "sclk_pwm",
>  			.id		= -1,
> -			.enable		= s5pv210_clk_ip3_ctrl,
> -			.ctrlbit	= (1 << 23),
> +			.enable		= s5pv210_clk_mask0_ctrl,
> +			.ctrlbit	= (1 << 19),
>  		},
>  		.sources = &clkset_group2,
>  		.reg_src = { .reg = S5P_CLK_SRC5, .shift = 12, .size = 4 },
> --

I'm not sure that need to control of all sclk_xxx like above method, mux
output masking.

Thanks.

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

[PATCH] S5PV210 Correct clock source control register properties

[MyungJoo Ham]

On Fri, Jun 18, 2010 at 2:36 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> The clock source controls (struct clksrc_clk) have been often accessing
>> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
>> as well as the clock source. This duplicated clock issue may incur
>> lockup problems when there are two modules accessing the same clock with
>> different names.
>>
> Actually, S5V210/S5PC110 CLK_GATE_IPx can disable the clock operation of each
> IP if it is not required. And this reduces dynamic power. CLK_SRC_MASKx is
> used to control clock mux of each IP's SCLK.
>
> Basically CLK_SRC_MASKx is for mux control (output masking) of each IP
> source, and actually that can _not_ control relevant every source clock of
> each IP. So CLK_GATE_IPx exists, can do it.
>
> I know, in the several case, i.e, camera and fimc, each sclk_xxx need to
> control. And can be controlled each sclk_xxx by using your patch, but in this
> case, need to additional control relevant clock gating for dynamic clock
> gating of each IP in its device driver.

We've changed clksrc_clk's not to access CLK_GATE_IPx because of the
duplicated clock (or doppelganger) problem. We need to either get rid
of ".enable / .ctrlbit" from
struct clk of struct clksrc_clk, change them to use "CLK_SRC_MASKx",
or get rid of their twins
(e.g., "hsmmc" for "mmc_bus") as long as there are clock definitions
with different names
accessing the same CLK_GATE_IPx. Letting clksrc_clk access
CLK_GATE_IPx creates potential
kernel lockups.

Drivers should get clocks defined in "static struct clk init_clocks[],
init_clocks_disable[]" in order
to access CLK_GATE_IPx though we still have some missing clocks there;
e.g., mfc, g2d, and g3d.
Again, there should be NO duplicated clock definitions if we are going
to use .enable function that
accesses the same address & bit.

----------- from the previous patch -----------
Please note that each clock definition should access different control
register; otherwise, the system may suffer lockups. For example, if we
have two clock definitions "a" and "b" which access the same register
(and the shift value). Then, when we do:

       module B
       1 clk = clk_get("b");
       2 clk->clk_enable(clk);
       3 do something with clk.
       4 clk->clk_disable(clk);

       module A
       1 clk = clk_get("a");
       2 clk->clk_enable(clk);
       3 do something with clk
       4 clk->clk_disable(clk);

       And if the execution order is

       B1->B2->A1->A2->A3->A4->B3 ...

       Then, the system may hang at the point B3.
------------------------------------------------------------------------------------

>
>> Besides, the clock source control is supposed to control (setting values
>> of MASK, SRC, DIV), not to turn on/off individual clock.
>>
>> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
>> for masking clock source control in S5PV210/S5PC110 according to the
>> user manual (rev 20100201).
>>
>> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
>> CLK_GATE_IPx) for the clock source control is safer and fits to the
>> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
>> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
>> except for MFC, G2D, and G3D.
>>
>> In this patch,
>>
>> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
>> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
>> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
>>
>> - CAM 0/1 did not have enable/disable control. They now access
>> ? CLK_SRC_MASK0.
>>
>> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>> ? defined to control MFC, G3D, and G2D, we kept them to access
>> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
>> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
>> later.
>>
> Hmm..need to check about multimedia usage.
>
> And in my opinion, it would be helpful to understand that should be short and
> clear.
>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> ---
>> ?arch/arm/mach-s5pv210/clock.c | ?101
> ++++++++++++++++++++++-------------------
>> ?1 files changed, 55 insertions(+), 46 deletions(-)
>>
>> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> index ec5ad8c..08c1063 100644
>> --- a/arch/arm/mach-s5pv210/clock.c
>> +++ b/arch/arm/mach-s5pv210/clock.c
>> @@ -183,6 +183,11 @@ static int s5pv210_clk_mask0_ctrl(struct clk *clk, int
> enable)
>> ? ? ? return s5p_gatectrl(S5P_CLK_SRC_MASK0, clk, enable);
>> ?}
>>
>> +static int s5pv210_clk_mask1_ctrl(struct clk *clk, int enable)
>> +{
>> + ? ? return s5p_gatectrl(S5P_CLK_SRC_MASK1, clk, enable);
>> +}
>> +
>> ?static struct clk clk_sclk_hdmi27m = {
>> ? ? ? .name ? ? ? ? ? = "sclk_hdmi27m",
>> ? ? ? .id ? ? ? ? ? ? = -1,
>> @@ -497,8 +502,8 @@ static struct clksrc_clk clk_sclk_dac = {
>> ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_dac",
>> ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 10),
>> - ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip1_ctrl,
>> + ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 2),
>> + ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> ? ? ? },
>> ? ? ? .sources ? ? ? ?= &clkset_sclk_dac,
>> ? ? ? .reg_src ? ? ? ?= { .reg = S5P_CLK_SRC1, .shift = 8, .size = 1 },
>> @@ -527,8 +532,8 @@ static struct clksrc_clk clk_sclk_hdmi = {
>> ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_hdmi",
>> ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip1_ctrl,
>> - ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 11),
>> + ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 0),
>> ? ? ? },
>> ? ? ? .sources ? ? ? ?= &clkset_sclk_hdmi,
>> ? ? ? .reg_src ? ? ? ?= { .reg = S5P_CLK_SRC1, .shift = 0, .size = 1 },
>> @@ -565,8 +570,8 @@ static struct clksrc_clk clk_sclk_audio0 = {
>> ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_audio",
>> ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 0,
>> - ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 4),
>> + ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 24),
>> ? ? ? },
>> ? ? ? .sources = &clkset_sclk_audio0,
>> ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 0, .size = 4 },
>> @@ -594,8 +599,8 @@ static struct clksrc_clk clk_sclk_audio1 = {
>> ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_audio",
>> ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 1,
>> - ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 5),
>> + ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 25),
>> ? ? ? },
>> ? ? ? .sources = &clkset_sclk_audio1,
>> ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 4, .size = 4 },
>> @@ -623,8 +628,8 @@ static struct clksrc_clk clk_sclk_audio2 = {
>> ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_audio",
>> ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 2,
>> - ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 6),
>> + ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 26),
>> ? ? ? },
>> ? ? ? .sources = &clkset_sclk_audio2,
>> ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 8, .size = 4 },
>> @@ -680,8 +685,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "uclk1",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 0,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1<<17),
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 12),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_uart,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 16, .size = 4 },
>> @@ -690,8 +695,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "uclk1",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 18),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 13),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_uart,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 20, .size = 4 },
>> @@ -700,8 +705,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "uclk1",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 2,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 19),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 14),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_uart,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 24, .size = 4 },
>> @@ -710,8 +715,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "uclk1",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 3,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 20),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 15),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_uart,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 28, .size = 4 },
>> @@ -720,8 +725,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_mixer",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip1_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 9),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 1),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_sclk_mixer,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC1, .shift = 4, .size = 1 },
>> @@ -738,8 +743,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_fimc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 0,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip0_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 24),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask1_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 2),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC3, .shift = 12, .size = 4 },
>> @@ -748,8 +753,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_fimc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip0_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 25),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask1_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 3),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC3, .shift = 16, .size = 4 },
>> @@ -758,8 +763,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_fimc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 2,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip0_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 26),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask1_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 4),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC3, .shift = 20, .size = 4 },
>> @@ -768,6 +773,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .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 },
>> @@ -776,6 +783,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .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 },
>> @@ -784,8 +793,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_fimd",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip1_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 0),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 5),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC1, .shift = 20, .size = 4 },
>> @@ -794,8 +803,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_mmc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 0,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 16),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 8),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
>> @@ -804,8 +813,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_mmc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 17),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 9),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
>> @@ -814,8 +823,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_mmc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 2,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 18),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 10),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
>> @@ -824,8 +833,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_mmc",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 3,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 19),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 11),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 },
>> @@ -864,8 +873,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_csis",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip0_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 31),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 6),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC1, .shift = 24, .size = 4 },
>> @@ -874,8 +883,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_spi",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 0,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 12),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 16),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC5, .shift = 0, .size = 4 },
>> @@ -884,8 +893,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_spi",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = 1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 13),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 17),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC5, .shift = 4, .size = 4 },
>> @@ -894,8 +903,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_pwi",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = &s5pv210_clk_ip4_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 2),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = &s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 29),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC6, .shift = 20, .size = 4 },
>> @@ -904,8 +913,8 @@ static struct clksrc_clk clksrcs[] = {
>> ? ? ? ? ? ? ? .clk ? ? ? ? ? ?= {
>> ? ? ? ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "sclk_pwm",
>> ? ? ? ? ? ? ? ? ? ? ? .id ? ? ? ? ? ? = -1,
>> - ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip3_ctrl,
>> - ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 23),
>> + ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
>> + ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ?= (1 << 19),
>> ? ? ? ? ? ? ? },
>> ? ? ? ? ? ? ? .sources = &clkset_group2,
>> ? ? ? ? ? ? ? .reg_src = { .reg = S5P_CLK_SRC5, .shift = 12, .size = 4 },
>> --
>
> I'm not sure that need to control of all sclk_xxx like above method, mux
> output masking.
>
> 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
>

ps. I forgot to erase .enable and .ctrlbit of "sclk_fimc"s. The user
manual states that
the three "sclk_fimc" clocks should be controlled identically at the
same time; thus,
there should be no individual clock masking control on them. This will
be done later.
(we may let them control (0x7<<2) so that they are controlled@the
same time, however,
we create "doppelganger" clocks there by doing so.)


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

[PATCH] S5PV210 Correct clock source control register properties

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> The clock source controls (struct clksrc_clk) have been often accessing
> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
> as well as the clock source. This duplicated clock issue may incur
> lockup problems when there are two modules accessing the same clock with
> different names.
> 
Could you please explain further about the above the statement " accessed by
each clock as well as the clock source"

> Besides, the clock source control is supposed to control (setting values
> of MASK, SRC, DIV), not to turn on/off individual clock.
> 
That is not quite true. Only S5PV210/S5PC110 have the gate for the clock mux.
S5P6440 and S5PC100, for instance, do not have a gate for the output for
_just_ the clock mux. And the reason why the Clock Mux Gate is provide for
the mux is explained in Section 3.4 of the user manual.

> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
> for masking clock source control in S5PV210/S5PC110 according to the
> user manual (rev 20100201).
> 
> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
> CLK_GATE_IPx) for the clock source control is safer and fits to the
> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
> except for MFC, G2D, and G3D.
> 
Can you please explain how the use CLK_SRC_MASKx makes it safer. The reason
why MFC, G2D and G3D do not have a corresponding bit in CLK_SRC_MASKx is
mentioned in Section 3.4 of the user manual.

Let me know your opinion.

> In this patch,
> 
> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
> 
> - CAM 0/1 did not have enable/disable control. They now access
>   CLK_SRC_MASK0.
> 
> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>   defined to control MFC, G3D, and G2D, we kept them to access
> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
> later.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
>  arch/arm/mach-s5pv210/clock.c |  101
++++++++++++++++++++++-------------------
>  1 files changed, 55 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> index ec5ad8c..08c1063 100644
> --- a/arch/arm/mach-s5pv210/clock.c
> +++ b/arch/arm/mach-s5pv210/clock.c

(snip)

Thanks.

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

[PATCH] S5PV210 Correct clock source control register properties

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> On Fri, Jun 18, 2010 at 2:36 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> >> The clock source controls (struct clksrc_clk) have been often accessing
> >> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
> >> as well as the clock source. This duplicated clock issue may incur
> >> lockup problems when there are two modules accessing the same clock with
> >> different names.
> >>
> > Actually, S5V210/S5PC110 CLK_GATE_IPx can disable the clock operation of
> each
> > IP if it is not required. And this reduces dynamic power. CLK_SRC_MASKx is
> > used to control clock mux of each IP's SCLK.
> >
> > Basically CLK_SRC_MASKx is for mux control (output masking) of each IP
> > source, and actually that can _not_ control relevant every source clock of
> > each IP. So CLK_GATE_IPx exists, can do it.
> >
> > I know, in the several case, i.e, camera and fimc, each sclk_xxx need to
> > control. And can be controlled each sclk_xxx by using your patch, but in this
> > case, need to additional control relevant clock gating for dynamic clock
> > gating of each IP in its device driver.
> 
> We've changed clksrc_clk's not to access CLK_GATE_IPx because of the
> duplicated clock (or doppelganger) problem. We need to either get rid
> of ".enable / .ctrlbit" from
> struct clk of struct clksrc_clk, change them to use "CLK_SRC_MASKx",
> or get rid of their twins
> (e.g., "hsmmc" for "mmc_bus") as long as there are clock definitions
> with different names
> accessing the same CLK_GATE_IPx. Letting clksrc_clk access
> CLK_GATE_IPx creates potential
> kernel lockups.
> 
Hmm..duplicated clock problem?

The clk_enable and clk_disable functions maintain a reference count and base the decision about enabling/disabling the clock on the reference count. Could you please explain how kernel lockups will occur in this case (considering the use of reference count in clk_enable and clk_disable)

> Drivers should get clocks defined in "static struct clk init_clocks[],
> init_clocks_disable[]" in order
> to access CLK_GATE_IPx though we still have some missing clocks there;
> e.g., mfc, g2d, and g3d.
> Again, there should be NO duplicated clock definitions if we are going
> to use .enable function that
> accesses the same address & bit.
> 
> ----------- from the previous patch -----------
> Please note that each clock definition should access different control
> register; otherwise, the system may suffer lockups. For example, if we
> have two clock definitions "a" and "b" which access the same register
> (and the shift value). Then, when we do:
> 
>        module B
>        1 clk = clk_get("b");
>        2 clk->clk_enable(clk);
>        3 do something with clk.
>        4 clk->clk_disable(clk);
> 
>        module A
>        1 clk = clk_get("a");
>        2 clk->clk_enable(clk);
>        3 do something with clk
>        4 clk->clk_disable(clk);
> 
>        And if the execution order is
> 
>        B1->B2->A1->A2->A3->A4->B3 ...
> 
>        Then, the system may hang at the point B3.
> ------------------------------------------------------------------------------------
> 
> >
> >> Besides, the clock source control is supposed to control (setting values
> >> of MASK, SRC, DIV), not to turn on/off individual clock.
> >>
> >> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
> >> for masking clock source control in S5PV210/S5PC110 according to the
> >> user manual (rev 20100201).
> >>
> >> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
> >> CLK_GATE_IPx) for the clock source control is safer and fits to the
> >> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
> >> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
> >> except for MFC, G2D, and G3D.
> >>
> >> In this patch,
> >>
> >> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
> >> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
> >> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
> >>
> >> - CAM 0/1 did not have enable/disable control. They now access
> >>   CLK_SRC_MASK0.
> >>
> >> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
> >>   defined to control MFC, G3D, and G2D, we kept them to access
> >> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
> >> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
> >> later.
> >>
> > Hmm..need to check about multimedia usage.
> >
> > And in my opinion, it would be helpful to understand that should be short and
> > clear.
> >
> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> ---
> >>  arch/arm/mach-s5pv210/clock.c |  101
> > ++++++++++++++++++++++-------------------
> >>  1 files changed, 55 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> >> index ec5ad8c..08c1063 100644
> >> --- a/arch/arm/mach-s5pv210/clock.c
> >> +++ b/arch/arm/mach-s5pv210/clock.c

(snip)

> >> --
> >
> > I'm not sure that need to control of all sclk_xxx like above method, mux
> > output masking.
> >

Please let me know your opinion about above question based on scenario.

> > 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
> >
> 
> ps. I forgot to erase .enable and .ctrlbit of "sclk_fimc"s. The user
> manual states that
> the three "sclk_fimc" clocks should be controlled identically at the
> same time; thus,
> there should be no individual clock masking control on them. This will
> be done later.
> (we may let them control (0x7<<2) so that they are controlled@the
> same time, however,
> we create "doppelganger" clocks there by doing so.)
> 
> 


Thanks.

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

[PATCH] S5PV210 Correct clock source control register properties

[MyungJoo Ham]

On Fri, Jun 18, 2010 at 6:58 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> On Fri, Jun 18, 2010 at 2:36 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > MyungJoo Ham wrote:
>> >>
>> >> The clock source controls (struct clksrc_clk) have been often accessing
>> >> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
>> >> as well as the clock source. This duplicated clock issue may incur
>> >> lockup problems when there are two modules accessing the same clock with
>> >> different names.
>> >>
>> > Actually, S5V210/S5PC110 CLK_GATE_IPx can disable the clock operation of
>> each
>> > IP if it is not required. And this reduces dynamic power. CLK_SRC_MASKx is
>> > used to control clock mux of each IP's SCLK.
>> >
>> > Basically CLK_SRC_MASKx is for mux control (output masking) of each IP
>> > source, and actually that can _not_ control relevant every source clock of
>> > each IP. So CLK_GATE_IPx exists, can do it.
>> >
>> > I know, in the several case, i.e, camera and fimc, each sclk_xxx need to
>> > control. And can be controlled each sclk_xxx by using your patch, but in this
>> > case, need to additional control relevant clock gating for dynamic clock
>> > gating of each IP in its device driver.
>>
>> We've changed clksrc_clk's not to access CLK_GATE_IPx because of the
>> duplicated clock (or doppelganger) problem. We need to either get rid
>> of ".enable / .ctrlbit" from
>> struct clk of struct clksrc_clk, change them to use "CLK_SRC_MASKx",
>> or get rid of their twins
>> (e.g., "hsmmc" for "mmc_bus") as long as there are clock definitions
>> with different names
>> accessing the same CLK_GATE_IPx. Letting clksrc_clk access
>> CLK_GATE_IPx creates potential
>> kernel lockups.
>>
> Hmm..duplicated clock problem?
>

> The clk_enable and clk_disable functions maintain a reference count and base the decision about enabling/disabling the clock on the reference count. Could you please explain how kernel lockups will occur in this case (considering the use of reference count in clk_enable and clk_disable)
>

It's because each struct clk keeps its own count value, not a shared
count value.

The lockup example (rephrased example of the previous reply)



(not an exact code / assume that initially the clock is off)

struct clk clkA = {.name="a", .ctrlbit=1, .enable=func};
struct clk clkB = {.name="b", .ctrlbit=1, .enable=func};

module A
{
 struct clk *a = clk_get("a");
 clk_enable(a);
 while (1)
   access registers related with the clock;
 clk_disable(a);
}

module B
{
 struct clk *b = clk_get("b");
 clk_enable(b);
   do a short job related with the clock;
 clk_disable(b);
}

Imagine when A started first, but while A stays in the loop, B runs
and exits. Then, in the module A, accessing registers may fail, which,
in turn, can lead to a lockup.


In this example, they do NOT share their count value, but access the
same register; i.e., a.count is not related with b.count while
a.enable(a.ctrlbit) will do the exactly same thing with
b.enable(b.ctrlbit). When there are one module using clkA and another
using clkB, the system may suffer from lockups.

Unless we change "struct clk" or "plat/clock.c:clk_disable" so that
they can refer to other "struct clk" (which does not appear to be a
good idea anyway), we should not define a clock more than once.


>> Drivers should get clocks defined in "static struct clk init_clocks[],
>> init_clocks_disable[]" in order
>> to access CLK_GATE_IPx though we still have some missing clocks there;
>> e.g., mfc, g2d, and g3d.
>> Again, there should be NO duplicated clock definitions if we are going
>> to use .enable function that
>> accesses the same address & bit.
>>
>> ----------- from the previous patch -----------
>> Please note that each clock definition should access different control
>> register; otherwise, the system may suffer lockups. For example, if we
>> have two clock definitions "a" and "b" which access the same register
>> (and the shift value). Then, when we do:
>>
>> ? ? ? ?module B
>> ? ? ? ?1 clk = clk_get("b");
>> ? ? ? ?2 clk->clk_enable(clk);
>> ? ? ? ?3 do something with clk.
>> ? ? ? ?4 clk->clk_disable(clk);
>>
>> ? ? ? ?module A
>> ? ? ? ?1 clk = clk_get("a");
>> ? ? ? ?2 clk->clk_enable(clk);
>> ? ? ? ?3 do something with clk
>> ? ? ? ?4 clk->clk_disable(clk);
>>
>> ? ? ? ?And if the execution order is
>>
>> ? ? ? ?B1->B2->A1->A2->A3->A4->B3 ...
>>
>> ? ? ? ?Then, the system may hang at the point B3.
>> ------------------------------------------------------------------------------------
>>
>> >
>> >> Besides, the clock source control is supposed to control (setting values
>> >> of MASK, SRC, DIV), not to turn on/off individual clock.
>> >>
>> >> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
>> >> for masking clock source control in S5PV210/S5PC110 according to the
>> >> user manual (rev 20100201).
>> >>
>> >> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
>> >> CLK_GATE_IPx) for the clock source control is safer and fits to the
>> >> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
>> >> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
>> >> except for MFC, G2D, and G3D.
>> >>
>> >> In this patch,
>> >>
>> >> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
>> >> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
>> >> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
>> >>
>> >> - CAM 0/1 did not have enable/disable control. They now access
>> >> ? CLK_SRC_MASK0.
>> >>
>> >> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>> >> ? defined to control MFC, G3D, and G2D, we kept them to access
>> >> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
>> >> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
>> >> later.
>> >>
>> > Hmm..need to check about multimedia usage.
>> >
>> > And in my opinion, it would be helpful to understand that should be short and
>> > clear.
>> >
>> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> >> ---
>> >> ?arch/arm/mach-s5pv210/clock.c | ?101
>> > ++++++++++++++++++++++-------------------
>> >> ?1 files changed, 55 insertions(+), 46 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> >> index ec5ad8c..08c1063 100644
>> >> --- a/arch/arm/mach-s5pv210/clock.c
>> >> +++ b/arch/arm/mach-s5pv210/clock.c
>
> (snip)
>
>> >> --
>> >
>> > I'm not sure that need to control of all sclk_xxx like above method, mux
>> > output masking.
>> >
>
> Please let me know your opinion about above question based on scenario.
>
>> > 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
>> >
>>
>> ps. I forgot to erase .enable and .ctrlbit of "sclk_fimc"s. The user
>> manual states that
>> the three "sclk_fimc" clocks should be controlled identically at the
>> same time; thus,
>> there should be no individual clock masking control on them. This will
>> be done later.
>> (we may let them control (0x7<<2) so that they are controlled@the
>> same time, however,
>> we create "doppelganger" clocks there by doing so.)
>>
>>
>
>
> 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] S5PV210 Correct clock source control register properties

[MyungJoo Ham]

On Fri, Jun 18, 2010 at 6:50 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> The clock source controls (struct clksrc_clk) have been often accessing
>> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
>> as well as the clock source. This duplicated clock issue may incur
>> lockup problems when there are two modules accessing the same clock with
>> different names.
>>
> Could you please explain further about the above the statement " accessed by
> each clock as well as the clock source"

For example, "CLK_GATE_IP2[16:19]" is accessed by "hsmmc" of struct
clk (a clock) and "sclk_mmc" of struct clksrc_clk's struct clk (a
clock source's clk representation).

>
>> Besides, the clock source control is supposed to control (setting values
>> of MASK, SRC, DIV), not to turn on/off individual clock.
>>
> That is not quite true. Only S5PV210/S5PC110 have the gate for the clock mux.
> S5P6440 and S5PC100, for instance, do not have a gate for the output for
> _just_ the clock mux. And the reason why the Clock Mux Gate is provide for
> the mux is explained in Section 3.4 of the user manual.

In cases where clock mux gating is not available and we have each
clock (supplied by this cluck mux) defined separatedly with .enable
and .ctrlbit, we'd better omit .enable/.ctrlbit from struct
clksrc_clk's struct clk.

>
>> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
>> for masking clock source control in S5PV210/S5PC110 according to the
>> user manual (rev 20100201).
>>
>> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
>> CLK_GATE_IPx) for the clock source control is safer and fits to the
>> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
>> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
>> except for MFC, G2D, and G3D.
>>
> Can you please explain how the use CLK_SRC_MASKx makes it safer. The reason
> why MFC, G2D and G3D do not have a corresponding bit in CLK_SRC_MASKx is
> mentioned in Section 3.4 of the user manual.
>
> Let me know your opinion.
>

It's "EXCEPT FOR MFC, G2D, G3D". I'm not saying that using
CLK_SRC_MASKx is safer for these three. For other cases, I guess the
lock up example should be fine.

However, in most cases, this lockup issue does not make visible problem because:
1. normally, drivers use both clksrc_clk's clk and clk at the same
time. for example
"mmc_bus".clk_enable;
"hsmmc".clk_enable;
 blahblah
"hsmmc".clk_disable;
"mmc_bus".clk_disable;
or
2. drivers sharing the same clock uses only one side of the two.
or
3. the execution block between clk_enable and clk_disable is extremely short.

Nevertheless, this potential lockup issue is critical and we'd better
address it.


>> In this patch,
>>
>> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
>> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
>> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
>>
>> - CAM 0/1 did not have enable/disable control. They now access
>> ? CLK_SRC_MASK0.
>>
>> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>> ? defined to control MFC, G3D, and G2D, we kept them to access
>> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
>> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
>> later.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> ---
>> ?arch/arm/mach-s5pv210/clock.c | ?101
> ++++++++++++++++++++++-------------------
>> ?1 files changed, 55 insertions(+), 46 deletions(-)
>>
>> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> index ec5ad8c..08c1063 100644
>> --- a/arch/arm/mach-s5pv210/clock.c
>> +++ b/arch/arm/mach-s5pv210/clock.c
>
> (snip)
>
> 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] S5PV210 Correct clock source control register properties

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> On Fri, Jun 18, 2010 at 6:50 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> >> The clock source controls (struct clksrc_clk) have been often accessing
> >> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
> >> as well as the clock source. This duplicated clock issue may incur
> >> lockup problems when there are two modules accessing the same clock with
> >> different names.
> >>
> > Could you please explain further about the above the statement " accessed by
> > each clock as well as the clock source"
> 
> For example, "CLK_GATE_IP2[16:19]" is accessed by "hsmmc" of struct
> clk (a clock) and "sclk_mmc" of struct clksrc_clk's struct clk (a
> clock source's clk representation).
> 
> >
> >> Besides, the clock source control is supposed to control (setting values
> >> of MASK, SRC, DIV), not to turn on/off individual clock.
> >>
> > That is not quite true. Only S5PV210/S5PC110 have the gate for the clock mux.
> > S5P6440 and S5PC100, for instance, do not have a gate for the output for
> > _just_ the clock mux. And the reason why the Clock Mux Gate is provide for
> > the mux is explained in Section 3.4 of the user manual.
> 
> In cases where clock mux gating is not available and we have each
> clock (supplied by this cluck mux) defined separatedly with .enable
> and .ctrlbit, we'd better omit .enable/.ctrlbit from struct
> clksrc_clk's struct clk.
> 
> >
> >> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
> >> for masking clock source control in S5PV210/S5PC110 according to the
> >> user manual (rev 20100201).
> >>
> >> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
> >> CLK_GATE_IPx) for the clock source control is safer and fits to the
> >> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
> >> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
> >> except for MFC, G2D, and G3D.
> >>
> > Can you please explain how the use CLK_SRC_MASKx makes it safer. The reason
> > why MFC, G2D and G3D do not have a corresponding bit in CLK_SRC_MASKx is
> > mentioned in Section 3.4 of the user manual.
> >
> > Let me know your opinion.
> >
> 
> It's "EXCEPT FOR MFC, G2D, G3D". I'm not saying that using
> CLK_SRC_MASKx is safer for these three. For other cases, I guess the
> lock up example should be fine.
> 
> However, in most cases, this lockup issue does not make visible problem because:
> 1. normally, drivers use both clksrc_clk's clk and clk at the same
> time. for example
> "mmc_bus".clk_enable;
> "hsmmc".clk_enable;
>  blahblah
> "hsmmc".clk_disable;
> "mmc_bus".clk_disable;
> or
> 2. drivers sharing the same clock uses only one side of the two.
> or
> 3. the execution block between clk_enable and clk_disable is extremely short.
> 
> Nevertheless, this potential lockup issue is critical and we'd better
> address it.
> 
> 
> >> In this patch,
> >>
> >> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
> >> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
> >> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
> >>
> >> - CAM 0/1 did not have enable/disable control. They now access
> >>   CLK_SRC_MASK0.
> >>
> >> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
> >>   defined to control MFC, G3D, and G2D, we kept them to access
> >> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
> >> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
> >> later.
> >>
> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> ---
> >>  arch/arm/mach-s5pv210/clock.c |  101
> > ++++++++++++++++++++++-------------------
> >>  1 files changed, 55 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> >> index ec5ad8c..08c1063 100644
> >> --- a/arch/arm/mach-s5pv210/clock.c
> >> +++ b/arch/arm/mach-s5pv210/clock.c
> >
> > (snip)
> >
> > --

Strictly speaking, 'duplicated clock problem' and 'clock gating issue which is controlled by CLK_GATE_IPx or CLK_SRC_MASKx' are different problem.
But mixed above two things in your patch.

I think just need bug fix.

See below mmc case.

Following is for clock gating mmc IP by using CLK_GATE_IPx and can control(clock gating) clocks, 'aclk_hsmmcX' and 'sclk_mmcX' in the mmc IP.

	{
		.name	= "hsmmc",
		.id	= 0,
		.parent	= &clk_hclk_psys.clk,
		.enable	= s5pv210_clk_ip2_ctrl,
		.ctrlbit	= (1<<16),
	}, {
		.name	= "hsmmc",
		.id	= 1,
		.parent	= &clk_hclk_psys.clk,
		.enable	= s5pv210_clk_ip2_ctrl,
		.ctrlbit	= (1<<17),
	}, {
		.name	= "hsmmc",
		.id	= 2,
		.parent	= &clk_hclk_psys.clk,
		.enable	= s5pv210_clk_ip2_ctrl,
		.ctrlbit	= (1<<18),
	}, {
		.name	= "hsmmc",
		.id	= 3,
		.parent	= &clk_hclk_psys.clk,
		.enable	= s5pv210_clk_ip2_ctrl,
		.ctrlbit	= (1<<19),
	}...

And following is for clock gating just 'sclk_mmcX' by using CLK_SRC_MASKx.
But as you know should be 's5pv210_clk_mask0_ctrl' instead of 's5pv210_clk_ip2_ctrl' like below.

@@ -794,8 +794,8 @@ static struct clksrc_clk clksrcs[] = {
		.clk		= {
			.name		= "sclk_mmc",
			.id		= 0,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit		= (1 << 16),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit		= (1 << 8),
		},
		.sources = &clkset_group2,
		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
@@ -804,8 +804,8 @@ static struct clksrc_clk clksrcs[] = {
		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 1,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit		= (1 << 17),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit		= (1 << 9),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
@@ -814,8 +814,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 2,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit		= (1 << 18),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit		= (1 << 10),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
@@ -824,8 +824,8 @@ static struct clksrc_clk clksrcs[] = {
 		.clk		= {
 			.name		= "sclk_mmc",
 			.id		= 3,
-			.enable		= s5pv210_clk_ip2_ctrl,
-			.ctrlbit		= (1 << 19),
+			.enable		= s5pv210_clk_mask0_ctrl,
+			.ctrlbit		= (1 << 11),
 		},
 		.sources = &clkset_group2,
 		.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }


I think there is no lockup issue(problem) after bug fix about 'sclk_xxx'.
Of course, carefully used distinguished clock gating in the relevant device driver.
Because as I said, CLK_GATE_IPx can disable all of the clock operation of each IP.
If no need CLK_GATE_IPx, just do clock gating each 'sclk_xxx'.

Could you please re-submit new stuff for bug fix?

Thanks.

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

[PATCH] S5PV210 Correct clock source control register properties

[Kyungmin Park]

On Sat, Jun 19, 2010 at 12:55 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> On Fri, Jun 18, 2010 at 6:50 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > MyungJoo Ham wrote:
>> >>
>> >> The clock source controls (struct clksrc_clk) have been often accessing
>> >> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
>> >> as well as the clock source. This duplicated clock issue may incur
>> >> lockup problems when there are two modules accessing the same clock with
>> >> different names.
>> >>
>> > Could you please explain further about the above the statement " accessed by
>> > each clock as well as the clock source"
>>
>> For example, "CLK_GATE_IP2[16:19]" is accessed by "hsmmc" of struct
>> clk (a clock) and "sclk_mmc" of struct clksrc_clk's struct clk (a
>> clock source's clk representation).
>>
>> >
>> >> Besides, the clock source control is supposed to control (setting values
>> >> of MASK, SRC, DIV), not to turn on/off individual clock.
>> >>
>> > That is not quite true. Only S5PV210/S5PC110 have the gate for the clock mux.
>> > S5P6440 and S5PC100, for instance, do not have a gate for the output for
>> > _just_ the clock mux. And the reason why the Clock Mux Gate is provide for
>> > the mux is explained in Section 3.4 of the user manual.
>>
>> In cases where clock mux gating is not available and we have each
>> clock (supplied by this cluck mux) defined separatedly with .enable
>> and .ctrlbit, we'd better omit .enable/.ctrlbit from struct
>> clksrc_clk's struct clk.
>>
>> >
>> >> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
>> >> for masking clock source control in S5PV210/S5PC110 according to the
>> >> user manual (rev 20100201).
>> >>
>> >> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
>> >> CLK_GATE_IPx) for the clock source control is safer and fits to the
>> >> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
>> >> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
>> >> except for MFC, G2D, and G3D.
>> >>
>> > Can you please explain how the use CLK_SRC_MASKx makes it safer. The reason
>> > why MFC, G2D and G3D do not have a corresponding bit in CLK_SRC_MASKx is
>> > mentioned in Section 3.4 of the user manual.
>> >
>> > Let me know your opinion.
>> >
>>
>> It's "EXCEPT FOR MFC, G2D, G3D". I'm not saying that using
>> CLK_SRC_MASKx is safer for these three. For other cases, I guess the
>> lock up example should be fine.
>>
>> However, in most cases, this lockup issue does not make visible problem because:
>> 1. normally, drivers use both clksrc_clk's clk and clk at the same
>> time. for example
>> "mmc_bus".clk_enable;
>> "hsmmc".clk_enable;
>> ?blahblah
>> "hsmmc".clk_disable;
>> "mmc_bus".clk_disable;
>> or
>> 2. drivers sharing the same clock uses only one side of the two.
>> or
>> 3. the execution block between clk_enable and clk_disable is extremely short.
>>
>> Nevertheless, this potential lockup issue is critical and we'd better
>> address it.
>>
>>
>> >> In this patch,
>> >>
>> >> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
>> >> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
>> >> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
>> >>
>> >> - CAM 0/1 did not have enable/disable control. They now access
>> >> ? CLK_SRC_MASK0.
>> >>
>> >> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>> >> ? defined to control MFC, G3D, and G2D, we kept them to access
>> >> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
>> >> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
>> >> later.
>> >>
>> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> >> ---
>> >> ?arch/arm/mach-s5pv210/clock.c | ?101
>> > ++++++++++++++++++++++-------------------
>> >> ?1 files changed, 55 insertions(+), 46 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> >> index ec5ad8c..08c1063 100644
>> >> --- a/arch/arm/mach-s5pv210/clock.c
>> >> +++ b/arch/arm/mach-s5pv210/clock.c
>> >
>> > (snip)
>> >
>> > --
>
> Strictly speaking, 'duplicated clock problem' and 'clock gating issue which is controlled by CLK_GATE_IPx or CLK_SRC_MASKx' are different problem.
> But mixed above two things in your patch.
>
> I think just need bug fix.
>
> See below mmc case.
>
> Following is for clock gating mmc IP by using CLK_GATE_IPx and can control(clock gating) clocks, 'aclk_hsmmcX' and 'sclk_mmcX' in the mmc IP.
>
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 0,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<16),
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 1,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<17),
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 2,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<18),
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 3,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<19),
> ? ? ? ?}...
>
> And following is for clock gating just 'sclk_mmcX' by using CLK_SRC_MASKx.
> But as you know should be 's5pv210_clk_mask0_ctrl' instead of 's5pv210_clk_ip2_ctrl' like below.

Right, that's the patch of Mr. Ham. Inn case of clksrc_clk use the
mask_ctrl instead of ip_ctrl.
The original codes use it by mix-up. his patch addresses it.

Thank you,
Kyungmin Park

>
> @@ -794,8 +794,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 0,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 16),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 8),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
> @@ -804,8 +804,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 1,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 17),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 9),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
> @@ -814,8 +814,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 2,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 18),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 10),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
> @@ -824,8 +824,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 3,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 19),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 11),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }
>
>
> I think there is no lockup issue(problem) after bug fix about 'sclk_xxx'.
> Of course, carefully used distinguished clock gating in the relevant device driver.
> Because as I said, CLK_GATE_IPx can disable all of the clock operation of each IP.
> If no need CLK_GATE_IPx, just do clock gating each 'sclk_xxx'.
>
> Could you please re-submit new stuff for bug fix?
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
>

[PATCH] S5PV210 Correct clock source control register properties

[MyungJoo Ham]

On Sat, Jun 19, 2010 at 12:55 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> MyungJoo Ham wrote:
>>
>> On Fri, Jun 18, 2010 at 6:50 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > MyungJoo Ham wrote:
>> >>
>> >> The clock source controls (struct clksrc_clk) have been often accessing
>> >> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
>> >> as well as the clock source. This duplicated clock issue may incur
>> >> lockup problems when there are two modules accessing the same clock with
>> >> different names.
>> >>
>> > Could you please explain further about the above the statement " accessed by
>> > each clock as well as the clock source"
>>
>> For example, "CLK_GATE_IP2[16:19]" is accessed by "hsmmc" of struct
>> clk (a clock) and "sclk_mmc" of struct clksrc_clk's struct clk (a
>> clock source's clk representation).
>>
>> >
>> >> Besides, the clock source control is supposed to control (setting values
>> >> of MASK, SRC, DIV), not to turn on/off individual clock.
>> >>
>> > That is not quite true. Only S5PV210/S5PC110 have the gate for the clock mux.
>> > S5P6440 and S5PC100, for instance, do not have a gate for the output for
>> > _just_ the clock mux. And the reason why the Clock Mux Gate is provide for
>> > the mux is explained in Section 3.4 of the user manual.
>>
>> In cases where clock mux gating is not available and we have each
>> clock (supplied by this cluck mux) defined separatedly with .enable
>> and .ctrlbit, we'd better omit .enable/.ctrlbit from struct
>> clksrc_clk's struct clk.
>>
>> >
>> >> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
>> >> for masking clock source control in S5PV210/S5PC110 according to the
>> >> user manual (rev 20100201).
>> >>
>> >> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
>> >> CLK_GATE_IPx) for the clock source control is safer and fits to the
>> >> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
>> >> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
>> >> except for MFC, G2D, and G3D.
>> >>
>> > Can you please explain how the use CLK_SRC_MASKx makes it safer. The reason
>> > why MFC, G2D and G3D do not have a corresponding bit in CLK_SRC_MASKx is
>> > mentioned in Section 3.4 of the user manual.
>> >
>> > Let me know your opinion.
>> >
>>
>> It's "EXCEPT FOR MFC, G2D, G3D". I'm not saying that using
>> CLK_SRC_MASKx is safer for these three. For other cases, I guess the
>> lock up example should be fine.
>>
>> However, in most cases, this lockup issue does not make visible problem because:
>> 1. normally, drivers use both clksrc_clk's clk and clk at the same
>> time. for example
>> "mmc_bus".clk_enable;
>> "hsmmc".clk_enable;
>> ?blahblah
>> "hsmmc".clk_disable;
>> "mmc_bus".clk_disable;
>> or
>> 2. drivers sharing the same clock uses only one side of the two.
>> or
>> 3. the execution block between clk_enable and clk_disable is extremely short.
>>
>> Nevertheless, this potential lockup issue is critical and we'd better
>> address it.
>>
>>
>> >> In this patch,
>> >>
>> >> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
>> >> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
>> >> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
>> >>
>> >> - CAM 0/1 did not have enable/disable control. They now access
>> >> ? CLK_SRC_MASK0.
>> >>
>> >> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no clocks
>> >> ? defined to control MFC, G3D, and G2D, we kept them to access
>> >> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
>> >> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
>> >> later.
>> >>
>> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> >> ---
>> >> ?arch/arm/mach-s5pv210/clock.c | ?101
>> > ++++++++++++++++++++++-------------------
>> >> ?1 files changed, 55 insertions(+), 46 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
>> >> index ec5ad8c..08c1063 100644
>> >> --- a/arch/arm/mach-s5pv210/clock.c
>> >> +++ b/arch/arm/mach-s5pv210/clock.c
>> >
>> > (snip)
>> >
>> > --
>
> Strictly speaking, 'duplicated clock problem' and 'clock gating issue which is controlled by CLK_GATE_IPx or CLK_SRC_MASKx' are different problem.
> But mixed above two things in your patch.
>
> I think just need bug fix.
>
> See below mmc case.
>
> Following is for clock gating mmc IP by using CLK_GATE_IPx and can control(clock gating) clocks, 'aclk_hsmmcX' and 'sclk_mmcX' in the mmc IP.
>
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 0,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<16),
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 1,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<17),
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 2,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<18),
> ? ? ? ?}, {
> ? ? ? ? ? ? ? ?.name ? = "hsmmc",
> ? ? ? ? ? ? ? ?.id ? ? = 3,
> ? ? ? ? ? ? ? ?.parent = &clk_hclk_psys.clk,
> ? ? ? ? ? ? ? ?.enable = s5pv210_clk_ip2_ctrl,
> ? ? ? ? ? ? ? ?.ctrlbit ? ? ? ?= (1<<19),
> ? ? ? ?}...
>
> And following is for clock gating just 'sclk_mmcX' by using CLK_SRC_MASKx.
> But as you know should be 's5pv210_clk_mask0_ctrl' instead of 's5pv210_clk_ip2_ctrl' like below.
>
> @@ -794,8 +794,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 0,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 16),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 8),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
> @@ -804,8 +804,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 1,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 17),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 9),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
> @@ -814,8 +814,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 2,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 18),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 10),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
> @@ -824,8 +824,8 @@ static struct clksrc_clk clksrcs[] = {
> ? ? ? ? ? ? ? ?.clk ? ? ? ? ? ?= {
> ? ? ? ? ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "sclk_mmc",
> ? ? ? ? ? ? ? ? ? ? ? ?.id ? ? ? ? ? ? = 3,
> - ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_ip2_ctrl,
> - ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 19),
> + ? ? ? ? ? ? ? ? ? ? ? .enable ? ? ? ? = s5pv210_clk_mask0_ctrl,
> + ? ? ? ? ? ? ? ? ? ? ? .ctrlbit ? ? ? ? ? ? ? ?= (1 << 11),
> ? ? ? ? ? ? ? ?},
> ? ? ? ? ? ? ? ?.sources = &clkset_group2,
> ? ? ? ? ? ? ? ?.reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }
>
>
> I think there is no lockup issue(problem) after bug fix about 'sclk_xxx'.
> Of course, carefully used distinguished clock gating in the relevant device driver.
> Because as I said, CLK_GATE_IPx can disable all of the clock operation of each IP.
> If no need CLK_GATE_IPx, just do clock gating each 'sclk_xxx'.
>
> Could you please re-submit new stuff for bug fix?


Ah, I've got it. So, your intention is to keep clocks with
CLK_SRC/CLK_DIV as a form struct clksrc_clk[] list to control clock
gating and get rid of them from struct clk[] list, right? As long as
we are not going to list a clock in both struct clksrc_clk list and
struct clk list, it should be fine.

However, what about merging hsmmc and sclk_mmc? It there any reason to
keep them separated? (e.g., remove "hsmmc" and let "sclk_mmc" use
CLK_GATE_IPx.

>
> 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] S5PV210 Correct clock source control register properties

[Kukjin Kim]

MyungJoo Ham wrote:
> 
> On Sat, Jun 19, 2010 at 12:55 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> >> On Fri, Jun 18, 2010 at 6:50 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> >> > MyungJoo Ham wrote:
> >> >>
> >> >> The clock source controls (struct clksrc_clk) have been often accessing
> >> >> CLK_GATE_IPx, which are for clock gating and are accessed by each clock
> >> >> as well as the clock source. This duplicated clock issue may incur
> >> >> lockup problems when there are two modules accessing the same clock with
> >> >> different names.
> >> >>
> >> > Could you please explain further about the above the statement " accessed
> by
> >> > each clock as well as the clock source"
> >>
> >> For example, "CLK_GATE_IP2[16:19]" is accessed by "hsmmc" of struct
> >> clk (a clock) and "sclk_mmc" of struct clksrc_clk's struct clk (a
> >> clock source's clk representation).
> >>
> >> >
> >> >> Besides, the clock source control is supposed to control (setting values
> >> >> of MASK, SRC, DIV), not to turn on/off individual clock.
> >> >>
> >> > That is not quite true. Only S5PV210/S5PC110 have the gate for the clock
> mux.
> >> > S5P6440 and S5PC100, for instance, do not have a gate for the output for
> >> > _just_ the clock mux. And the reason why the Clock Mux Gate is provide for
> >> > the mux is explained in Section 3.4 of the user manual.
> >>
> >> In cases where clock mux gating is not available and we have each
> >> clock (supplied by this cluck mux) defined separatedly with .enable
> >> and .ctrlbit, we'd better omit .enable/.ctrlbit from struct
> >> clksrc_clk's struct clk.
> >>
> >> >
> >> >> Another point is that there are registers (CLK_SRC_MASK0/1) specialized
> >> >> for masking clock source control in S5PV210/S5PC110 according to the
> >> >> user manual (rev 20100201).
> >> >>
> >> >> Therefore, accessing CLK_SRC_MASK0/1 (rather than accessing
> >> >> CLK_GATE_IPx) for the clock source control is safer and fits to the
> >> >> semantics of S5PV210/S5PC110 registers. And fortuneatly, each clock
> >> >> source defined in clock.c has corresponding bit at CLK_SRC_MASK0/1
> >> >> except for MFC, G2D, and G3D.
> >> >>
> >> > Can you please explain how the use CLK_SRC_MASKx makes it safer. The
> reason
> >> > why MFC, G2D and G3D do not have a corresponding bit in
> CLK_SRC_MASKx is
> >> > mentioned in Section 3.4 of the user manual.
> >> >
> >> > Let me know your opinion.
> >> >
> >>
> >> It's "EXCEPT FOR MFC, G2D, G3D". I'm not saying that using
> >> CLK_SRC_MASKx is safer for these three. For other cases, I guess the
> >> lock up example should be fine.
> >>
> >> However, in most cases, this lockup issue does not make visible problem
> because:
> >> 1. normally, drivers use both clksrc_clk's clk and clk at the same
> >> time. for example
> >> "mmc_bus".clk_enable;
> >> "hsmmc".clk_enable;
> >>  blahblah
> >> "hsmmc".clk_disable;
> >> "mmc_bus".clk_disable;
> >> or
> >> 2. drivers sharing the same clock uses only one side of the two.
> >> or
> >> 3. the execution block between clk_enable and clk_disable is extremely short.
> >>
> >> Nevertheless, this potential lockup issue is critical and we'd better
> >> address it.
> >>
> >>
> >> >> In this patch,
> >> >>
> >> >> - DAC, HDMI, AUDIO 0/1/2, UCLK(uart) 0/1/2/3, MIXER, FIMC 0/1/2,
> >> >> FIMC, MMC 0/1/2/3, CSIS, SPI 0/1, PWI, and PWM clock sources are
> >> >> modified to use CLK_SRC_MASK0/1, which were using CLK_GATE_IPx.
> >> >>
> >> >> - CAM 0/1 did not have enable/disable control. They now access
> >> >>   CLK_SRC_MASK0.
> >> >>
> >> >> - MFC, G3D, G2D were using CLK_GATE_IPx. However, as there are no
> clocks
> >> >>   defined to control MFC, G3D, and G2D, we kept them to access
> >> >> CLK_GATE_IPx. These may need to be modified (erase .enable, .ctrlbit
> >> >> from sclk_mfc, sclk_g2d, sclk_g3d and create clocks: g3d, g2d, mfc)
> >> >> later.
> >> >>
> >> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> >> ---
> >> >>  arch/arm/mach-s5pv210/clock.c |  101
> >> > ++++++++++++++++++++++-------------------
> >> >>  1 files changed, 55 insertions(+), 46 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> >> >> index ec5ad8c..08c1063 100644
> >> >> --- a/arch/arm/mach-s5pv210/clock.c
> >> >> +++ b/arch/arm/mach-s5pv210/clock.c
> >> >
> >> > (snip)
> >> >
> >> > --
> >
> > Strictly speaking, 'duplicated clock problem' and 'clock gating issue which is
> controlled by CLK_GATE_IPx or CLK_SRC_MASKx' are different problem.
> > But mixed above two things in your patch.
> >
> > I think just need bug fix.
> >
> > See below mmc case.
> >
> > Following is for clock gating mmc IP by using CLK_GATE_IPx and can
> control(clock gating) clocks, 'aclk_hsmmcX' and 'sclk_mmcX' in the mmc IP.
> >
> >        {
> >                .name   = "hsmmc",
> >                .id     = 0,
> >                .parent = &clk_hclk_psys.clk,
> >                .enable = s5pv210_clk_ip2_ctrl,
> >                .ctrlbit        = (1<<16),
> >        }, {
> >                .name   = "hsmmc",
> >                .id     = 1,
> >                .parent = &clk_hclk_psys.clk,
> >                .enable = s5pv210_clk_ip2_ctrl,
> >                .ctrlbit        = (1<<17),
> >        }, {
> >                .name   = "hsmmc",
> >                .id     = 2,
> >                .parent = &clk_hclk_psys.clk,
> >                .enable = s5pv210_clk_ip2_ctrl,
> >                .ctrlbit        = (1<<18),
> >        }, {
> >                .name   = "hsmmc",
> >                .id     = 3,
> >                .parent = &clk_hclk_psys.clk,
> >                .enable = s5pv210_clk_ip2_ctrl,
> >                .ctrlbit        = (1<<19),
> >        }...
> >
> > And following is for clock gating just 'sclk_mmcX' by using CLK_SRC_MASKx.
> > But as you know should be 's5pv210_clk_mask0_ctrl' instead of
> 's5pv210_clk_ip2_ctrl' like below.
> >
> > @@ -794,8 +794,8 @@ static struct clksrc_clk clksrcs[] = {
> >                .clk            = {
> >                        .name           = "sclk_mmc",
> >                        .id             = 0,
> > -                       .enable         = s5pv210_clk_ip2_ctrl,
> > -                       .ctrlbit                = (1 << 16),
> > +                       .enable         = s5pv210_clk_mask0_ctrl,
> > +                       .ctrlbit                = (1 << 8),
> >                },
> >                .sources = &clkset_group2,
> >                .reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 },
> > @@ -804,8 +804,8 @@ static struct clksrc_clk clksrcs[] = {
> >                .clk            = {
> >                        .name           = "sclk_mmc",
> >                        .id             = 1,
> > -                       .enable         = s5pv210_clk_ip2_ctrl,
> > -                       .ctrlbit                = (1 << 17),
> > +                       .enable         = s5pv210_clk_mask0_ctrl,
> > +                       .ctrlbit                = (1 << 9),
> >                },
> >                .sources = &clkset_group2,
> >                .reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 },
> > @@ -814,8 +814,8 @@ static struct clksrc_clk clksrcs[] = {
> >                .clk            = {
> >                        .name           = "sclk_mmc",
> >                        .id             = 2,
> > -                       .enable         = s5pv210_clk_ip2_ctrl,
> > -                       .ctrlbit                = (1 << 18),
> > +                       .enable         = s5pv210_clk_mask0_ctrl,
> > +                       .ctrlbit                = (1 << 10),
> >                },
> >                .sources = &clkset_group2,
> >                .reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 },
> > @@ -824,8 +824,8 @@ static struct clksrc_clk clksrcs[] = {
> >                .clk            = {
> >                        .name           = "sclk_mmc",
> >                        .id             = 3,
> > -                       .enable         = s5pv210_clk_ip2_ctrl,
> > -                       .ctrlbit                = (1 << 19),
> > +                       .enable         = s5pv210_clk_mask0_ctrl,
> > +                       .ctrlbit                = (1 << 11),
> >                },
> >                .sources = &clkset_group2,
> >                .reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }
> >
> >
> > I think there is no lockup issue(problem) after bug fix about 'sclk_xxx'.
> > Of course, carefully used distinguished clock gating in the relevant device driver.
> > Because as I said, CLK_GATE_IPx can disable all of the clock operation of each
> IP.
> > If no need CLK_GATE_IPx, just do clock gating each 'sclk_xxx'.
> >
> > Could you please re-submit new stuff for bug fix?
> 
> 
Hi,

> Ah, I've got it. So, your intention is to keep clocks with
> CLK_SRC/CLK_DIV as a form struct clksrc_clk[] list to control clock
> gating and get rid of them from struct clk[] list, right? As long as
> we are not going to list a clock in both struct clksrc_clk list and
> struct clk list, it should be fine.
> 
Yes, I think need them, because SRC and DIV of clksrc_clk are a thing about
MUX and DIVIDER of each IP input sources.
And as your patch, sclk_xxx should be controlled by CLK_SRC_MASKx.

> However, what about merging hsmmc and sclk_mmc? It there any reason to
> keep them separated? (e.g., remove "hsmmc" and let "sclk_mmc" use
> CLK_GATE_IPx.
> 
Actually, the usage or purpose of the CLK_GATE_IPx and CLK_SRC_MASKx is
differs.
As you know, all input source of each IP can be controlled(clock gating) by
CLK_GATE_IPx like hsmmc, and each special(?) clock like sclk_mmc can be
controlled by CLK_SRC_MASKx.
And as I commented, carefully used distinguished clock gating in the
relevant device driver.

Thanks.

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