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

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

[Tomasz Figa]

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@linaro.org>
> ---
>  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.

>  	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.

>  	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.

>  	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.

This is especially important in case of regulators, which currently have 
to be hacked by using vdd_arm as regulator name in device tree.

CCing people that might be interested in this topic.

Best regards,
Tomasz

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

[Tushar Behera]

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@linaro.org>
>> ---
>>  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.

>>  	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").

>>  	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.

> 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.

-- 
Tushar Behera

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

[Tomasz Figa]

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@linaro.org>
> >> ---
> >> 
> >>  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.

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

[Tushar Behera]

On 06/11/2013 12:23 AM, Tomasz Figa wrote:
> 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:

[ ... ]

>>>>  	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.
> 

Is it ok if I split this patch into 2, one adding clock alias
'mout_apll' and another one fixing the alias names 'mout_mpll',
'moutcore' and 'armclk'?

[ ... ]

>>> 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.
> 

If you are going to submit the cpufreq driver patches for v3.11, then we
can ignore this patchset. Otherwise, I would prefer to get these patches
merged for v3.11 to get cpufreq working. Once the driver changes are
incorporated, we can very well modify these later.

Thanks.
-- 
Tushar Behera

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

[Tushar Behera]

On 06/17/2013 10:20 AM, Tushar Behera wrote:
> On 06/11/2013 12:23 AM, Tomasz Figa wrote:
>> 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:
> 
> [ ... ]
> 
>>>>>  	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.
>>
> 
> Is it ok if I split this patch into 2, one adding clock alias
> 'mout_apll' and another one fixing the alias names 'mout_mpll',
> 'moutcore' and 'armclk'?
> 

I have to fix up another clock for exynos4x12 too. I feel all these
modifications are too small to justify different patches. I would modify
the commit message appropriately.


> [ ... ]
> 
>>>> 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.
>>
> 
> If you are going to submit the cpufreq driver patches for v3.11, then we
> can ignore this patchset. Otherwise, I would prefer to get these patches
> merged for v3.11 to get cpufreq working. Once the driver changes are
> incorporated, we can very well modify these later.
> 
> Thanks.
> 


-- 
Tushar Behera

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

[Tomasz Figa]

Hi Tushar,

On Wednesday 19 of June 2013 10:20:14 Tushar Behera wrote:
> On 06/17/2013 10:20 AM, Tushar Behera wrote:
> > On 06/11/2013 12:23 AM, Tomasz Figa wrote:
> >> 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:
> > [ ... ]
> > 
> >>>>>  	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.> 
> > Is it ok if I split this patch into 2, one adding clock alias
> > 'mout_apll' and another one fixing the alias names 'mout_mpll',
> > 'moutcore' and 'armclk'?
> 
> I have to fix up another clock for exynos4x12 too. I feel all these
> modifications are too small to justify different patches. I would modify
> the commit message appropriately.

Fine.

> > [ ... ]
> > 
> >>>> 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.
> > 
> > If you are going to submit the cpufreq driver patches for v3.11, then
> > we can ignore this patchset. Otherwise, I would prefer to get these
> > patches merged for v3.11 to get cpufreq working. Once the driver
> > changes are incorporated, we can very well modify these later.

I hope we can get my patches merged for 3.11, but just to be safe and have 
things working for now, please proceed with yours.

I need a bit more time for this, because IMHO the current design of our 
cpufreq drivers is inappropriate (directly accessing CMU registers) with 
common clock framework already existing and it needs to be redesigned.

Best regards,
Tomasz