[PATCH] S5PV210 Correct clock source control register properties
Kukjin Kim
kgene.kim at samsung.com
Fri Jun 18 23:55:05 EDT 2010
MyungJoo Ham wrote:
>
> On Fri, Jun 18, 2010 at 6:50 PM, Kukjin Kim <kgene.kim at 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 at 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 at samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
More information about the linux-arm-kernel
mailing list