Re: [PATCH 2/2] clk: exynos4: Add alias for cpufreq related clocks

From: Tomasz Figa
Date: Mon Jun 10 2013 - 14:53:20 EST


On Monday 10 of June 2013 09:13:11 Tushar Behera wrote:
> On 06/08/2013 05:20 PM, Tomasz Figa wrote:
> > On Thursday 06 of June 2013 16:52:28 Tushar Behera wrote:
> >> cpufreq driver for EXYNOS4 based SoCs are not platform drivers, hence
> >> we cannot currently pass the clock names through a device tree node.
> >> Instead, we need to make them available through a global alias.
> >>
> >> 'armclk', 'moutcore', 'mout_mpll' and 'mout_apll' clock aliases are
> >> defined.
> >>
> >> Signed-off-by: Tushar Behera <tushar.behera@xxxxxxxxxx>
> >> ---
> >>
> >> drivers/clk/samsung/clk-exynos4.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk-exynos4.c
> >> b/drivers/clk/samsung/clk-exynos4.c index 3c1f888..1e4258a 100644
> >> --- a/drivers/clk/samsung/clk-exynos4.c
> >> +++ b/drivers/clk/samsung/clk-exynos4.c
> >> @@ -356,8 +356,8 @@ struct samsung_fixed_rate_clock
> >> exynos4210_fixed_rate_clks[] __initdata = {
> >>
> >> /* list of mux clocks supported in all exynos4 soc's */
> >> struct samsung_mux_clock exynos4_mux_clks[] __initdata = {
> >>
> >> - MUX_F(mout_apll, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
> >> - CLK_SET_RATE_PARENT, 0),
> >> + MUX_FA(mout_apll, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
> >> + CLK_SET_RATE_PARENT, 0, "mout_apll"),
> >>
> >> MUX(none, "mout_hdmi", mout_hdmi_p, SRC_TV, 0, 1),
> >> MUX(none, "mout_mfc1", sclk_evpll_p, SRC_MFC, 4, 1),
> >> MUX(none, "mout_mfc", mout_mfc_p, SRC_MFC, 8, 1),
> >>
> >> @@ -385,9 +385,9 @@ struct samsung_mux_clock exynos4210_mux_clks[]
> >> __initdata = { MUX(none, "mout_g2d", mout_g2d_p, E4210_SRC_IMAGE, 8,
> >> 1),
> >>
> >> MUX(none, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
> >> MUX(none, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
> >>
> >> - MUX_A(sclk_mpll, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1,
> >
> > "sclk_mpll"),
> >
> >> + MUX_A(sclk_mpll, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1,
> >
> > "mout_mpll"),
> >
> > This is not fully compliant with patch description. I'm not sure if
> > there weren't any users of the sclk_mpll alias.
>
> As of now, there are no other users of sclk_mpll other than a debug
> print within the same file.

OK.

> >> MUX_A(mout_core, "mout_core", mout_core_p4210,
> >>
> >> - SRC_CPU, 16, 1, "mout_core"),
> >> + SRC_CPU, 16, 1, "moutcore"),
> >
> > IMHO those typo corrections are not part of this patch.
>
> But the older drivers (before migration to CCF) were using the clock
> "moutcore" (not "mout_core").

I mean, this should be placed in a separate patch, as this change is not
"adding alias for cpufreq related clocks", but rather fixing a typo.

> >> MUX_A(sclk_vpll, "sclk_vpll", sclk_vpll_p4210,
> >>
> >> SRC_TOP0, 8, 1, "sclk_vpll"),
> >>
> >> MUX(mout_fimc0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
> >>
> >> @@ -534,7 +534,7 @@ struct samsung_div_clock exynos4_div_clks[]
> >> __initdata = { DIV(none, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8,
> >> 8),
> >>
> >> DIV(none, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
> >> DIV(none, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
> >>
> >> - DIV_A(arm_clk, "arm_clk", "div_core2", DIV_CPU0, 28, 3,
> >
> > "arm_clk"),
> >
> >> + DIV_A(arm_clk, "arm_clk", "div_core2", DIV_CPU0, 28, 3, "armclk"),
> >
> > Same here.
>
> Same as above, "armclk" is used elsewhere, not "arm_clk".
>
> >> DIV_A(sclk_apll, "sclk_apll", "mout_apll",
> >>
> >> DIV_CPU0, 24, 3, "sclk_apll"),
> >>
> >> DIV_F(none, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
> >
> > Basically I don't like the idea of those global aliases, which IMHO
> > should be completely dropped. Someone might not like it, but I'd go
> > with the conversion of our cpufreq drivers to platform drivers
> > instead, which could receive things like clocks and regulators using
> > DT-based lookups.
> I agree. Migration of exynos-cpufreq driver as a platform driver is the
> best solution. But unless someone picks up that work, cpufreq support
> for EXYNOS4 based systems is broken because of the incorrect clock
> aliases.

We have patches for this in our internal tree. I will clean them up a bit
and submit soon.

Thanks,
Tomasz

> > This is especially important in case of regulators, which currently
> > have to be hacked by using vdd_arm as regulator name in device tree.
> Agree.
>
> > CCing people that might be interested in this topic.
> >
> > Best regards,
> > Tomasz
>
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/