[PATCH v2] S5PV210 Correct clock register properties
From: Kukjin Kim <hidden>
Date: 2010-06-23 07:58:08
MyungJoo Ham wrote:
On Wed, Jun 23, 2010 at 2:21 PM, Kukjin Kim [off-list ref] wrote:quoted
MyungJoo Ham wrote:quoted
Note that this patch revises and merges the two previous patches: [PATCH] S5PV210 Correct clock register properties [PATCH] S5PV210 Correct clock source control register properties 1. Corrected shift values of I2S and UART clocks (CLK_GATE_IP3), whichwerequoted
defined incorrectly. 2. Corrected shift values of sclk_audio, uclk1, sclk_fimd, sclk_mmc, sclk_spi, sclk_pwm, which had duplicated .enable/.ctrlbit with their twins defined in struct clk init_clocks_disable[] and struct clk init_clocks[]. We've changed their .enable/.ctrlbit to use CLK_SRC_MASK register to avoid the duplicated clock problem described below.--- Duplicated Clock Problem ---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 A clk = clk_get("a"); clk->clk_enable(clk); module B (context switch) clk = clk_get("b"); clk->clk_enable(clk); do something with clk. clk->clk_disable(clk); module A (context switch) do something with clk * At this point, the system may hang. Therefore, there should be no clock definitions with the same contol register/shift. If we need to create "aliases", then, creating child clocks sharing the clock should be fine. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> --- arch/arm/mach-s5pv210/clock.c | 72++++++++++++++++++++--------------------quoted
1 files changed, 36 insertions(+), 36 deletions(-)diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c index 154bca4..10a9f84 100644 --- a/arch/arm/mach-s5pv210/clock.c +++ b/arch/arm/mach-s5pv210/clock.c@@ -406,13 +406,13 @@ static struct clk init_clocks_disable[] = { .id = 0, .parent = &clk_p, .enable = s5pv210_clk_ip3_ctrl, - .ctrlbit = (1<<4), + .ctrlbit = (1<<5),If possible, please add space between <<, like (1 << 5)Ok, then, we'll probably need to modify almost every clk definition in this file as most of shift operators don't have spaces around them. Do you still want to do so?
I mean just about modified stuff in here. So right now, no need additional work for others.
quoted
quoted
}, { .name = "i2s_v32", .id = 1, .parent = &clk_p, .enable = s5pv210_clk_ip3_ctrl, - .ctrlbit = (1<<4), + .ctrlbit = (1<<6), } };@@ -429,25 +429,25 @@ static struct clk init_clocks[] = { .id = 0, .parent = &clk_pclk_psys.clk, .enable = s5pv210_clk_ip3_ctrl, - .ctrlbit = (1<<7), + .ctrlbit = (1<<17), }, { .name = "uart", .id = 1, .parent = &clk_pclk_psys.clk, .enable = s5pv210_clk_ip3_ctrl, - .ctrlbit = (1<<8), + .ctrlbit = (1<<18), }, { .name = "uart", .id = 2, .parent = &clk_pclk_psys.clk, .enable = s5pv210_clk_ip3_ctrl, - .ctrlbit = (1<<9), + .ctrlbit = (1<<19), }, { .name = "uart", .id = 3, .parent = &clk_pclk_psys.clk, .enable = s5pv210_clk_ip3_ctrl, - .ctrlbit = (1<<10), + .ctrlbit = (1<<20), }, };@@ -565,8 +565,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 +594,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 +623,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 +680,8 @@ static struct clksrc_clk clksrcs[] = { .clk = { .name = "uclk1", .id = 0, - .ctrlbit = (1<<17), - .enable = s5pv210_clk_ip3_ctrl, + .enable = s5pv210_clk_mask0_ctrl, + .ctrlbit = (1 << 12), }, .sources = &clkset_uart, .reg_src = { .reg = S5P_CLK_SRC4, .shift = 16, .size = 4 },@@ -690,8 +690,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 +700,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 +710,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 },@@ -784,8 +784,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 +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 },@@ -874,8 +874,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 +884,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 },@@ -904,8 +904,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 }, --Looks good. But missed something such as sclk_dac, sclk_hdmi, sclk_mixer, sclk_fimc, sclk_cam, sclk_csis, and sclk_pwi. Any reason?I was going to separate duplicated clocks only in this one and modifying sclk_[dac|hdmi|mixer|fimc|cam|csis|pwi] would have required to define additional clks. However, if we agree on adding clks for [dac|hdmi|mixer|fimc|cam|csis|pwi], I'll add those in the next revision (PATCH V3) and let sclk_[dac|hdmi|mixer|fimc|cam|csis|pwi] to access MASK registers.
yeah..I see. That's 'duplicated clock problem' as you commented at once and bug :-( So should be fixed :-) Thanks. Best regards, Kgene. -- Kukjin Kim [off-list ref], Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.