[PATCH] ARM: SAMSUNG: Set clock parent if provided

[PATCH] ARM: SAMSUNG: Set clock parent if provided

[Shaik Ameer Basha]

s3c_set_clksrc() updates the clock source as per u-boot settings.
This patch adds the functionality to overwrite u-boot settings,
if user provides the clock parent field. In case of wrong source
provided by the user, it will retain the u-boot settings.

Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
---
 arch/arm/plat-samsung/clock-clksrc.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
index 786a410..4fecd80 100644
--- a/arch/arm/plat-samsung/clock-clksrc.c
+++ b/arch/arm/plat-samsung/clock-clksrc.c
@@ -150,7 +150,12 @@ void __init_or_cpufreq s3c_set_clksrc(struct clksrc_clk *clk, bool announce)
 		return;
 	}
 
-	clk->clk.parent = srcs->sources[clksrc];
+	if (clk->clk.parent) {
+		if (s3c_setparent_clksrc(&clk->clk, clk->clk.parent))
+			clk->clk.parent = srcs->sources[clksrc];
+	} else {
+		clk->clk.parent = srcs->sources[clksrc];
+	}
 
 	if (announce)
 		printk(KERN_INFO "%s: source is %s (%d), rate is %ld\n",
-- 
1.7.9.5

Re: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Rahul Sharma]

On Thu, Mar 7, 2013 at 11:01 AM, Shaik Ameer Basha
<shaik.ameer@samsung.com> wrote:
> s3c_set_clksrc() updates the clock source as per u-boot settings.
> This patch adds the functionality to overwrite u-boot settings,
> if user provides the clock parent field. In case of wrong source
> provided by the user, it will retain the u-boot settings.
>
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
> ---
>  arch/arm/plat-samsung/clock-clksrc.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
> index 786a410..4fecd80 100644
> --- a/arch/arm/plat-samsung/clock-clksrc.c
> +++ b/arch/arm/plat-samsung/clock-clksrc.c
> @@ -150,7 +150,12 @@ void __init_or_cpufreq s3c_set_clksrc(struct clksrc_clk *clk, bool announce)
>                 return;
>         }
>
> -       clk->clk.parent = srcs->sources[clksrc];
> +       if (clk->clk.parent) {
> +               if (s3c_setparent_clksrc(&clk->clk, clk->clk.parent))
> +                       clk->clk.parent = srcs->sources[clksrc];

IMO, it make sense to return the error value if failed. Now you
are masking the failed cases (due to invalid parent clk) and proceeding
with uboot values which is not the intention when parent clock
is provided.

Regards,
Rahul Sharma,

> +       } else {
> +               clk->clk.parent = srcs->sources[clksrc];
> +       }
>
>         if (announce)
>                 printk(KERN_INFO "%s: source is %s (%d), rate is %ld\n",
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--

Re: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Shaik Ameer Basha]

Hi Rahul,

On Thu, Mar 7, 2013 at 2:58 PM, Rahul Sharma <r.sh.open@gmail.com> wrote:
> On Thu, Mar 7, 2013 at 11:01 AM, Shaik Ameer Basha
> <shaik.ameer@samsung.com> wrote:
>> s3c_set_clksrc() updates the clock source as per u-boot settings.
>> This patch adds the functionality to overwrite u-boot settings,
>> if user provides the clock parent field. In case of wrong source
>> provided by the user, it will retain the u-boot settings.
>>
>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>> ---
>>  arch/arm/plat-samsung/clock-clksrc.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
>> index 786a410..4fecd80 100644
>> --- a/arch/arm/plat-samsung/clock-clksrc.c
>> +++ b/arch/arm/plat-samsung/clock-clksrc.c
>> @@ -150,7 +150,12 @@ void __init_or_cpufreq s3c_set_clksrc(struct clksrc_clk *clk, bool announce)
>>                 return;
>>         }
>>
>> -       clk->clk.parent = srcs->sources[clksrc];
>> +       if (clk->clk.parent) {
>> +               if (s3c_setparent_clksrc(&clk->clk, clk->clk.parent))
>> +                       clk->clk.parent = srcs->sources[clksrc];
>
> IMO, it make sense to return the error value if failed. Now you
> are masking the failed cases (due to invalid parent clk) and proceeding
> with uboot values which is not the intention when parent clock
> is provided.

yes you are right. But as the function s3c_set_clksrc() is not
expected to return any error codes,
I tried to use default u-boot settings.
We can add a warning message to let user know in case of wrong parent
assignment.

Regards,
Shaik Ameer Basha

>
> Regards,
> Rahul Sharma,
>
>> +       } else {
>> +               clk->clk.parent = srcs->sources[clksrc];
>> +       }
>>
>>         if (announce)
>>                 printk(KERN_INFO "%s: source is %s (%d), rate is %ld\n",
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --

Re: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Rahul Sharma]

On Thu, Mar 7, 2013 at 3:12 PM, Shaik Ameer Basha
<shaik.samsung@gmail.com> wrote:
> Hi Rahul,
>
> On Thu, Mar 7, 2013 at 2:58 PM, Rahul Sharma <r.sh.open@gmail.com> wrote:
>> On Thu, Mar 7, 2013 at 11:01 AM, Shaik Ameer Basha
>> <shaik.ameer@samsung.com> wrote:
>>> s3c_set_clksrc() updates the clock source as per u-boot settings.
>>> This patch adds the functionality to overwrite u-boot settings,
>>> if user provides the clock parent field. In case of wrong source
>>> provided by the user, it will retain the u-boot settings.
>>>
>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>>> ---
>>>  arch/arm/plat-samsung/clock-clksrc.c |    7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
>>> index 786a410..4fecd80 100644
>>> --- a/arch/arm/plat-samsung/clock-clksrc.c
>>> +++ b/arch/arm/plat-samsung/clock-clksrc.c
>>> @@ -150,7 +150,12 @@ void __init_or_cpufreq s3c_set_clksrc(struct clksrc_clk *clk, bool announce)
>>>                 return;
>>>         }
>>>
>>> -       clk->clk.parent = srcs->sources[clksrc];
>>> +       if (clk->clk.parent) {
>>> +               if (s3c_setparent_clksrc(&clk->clk, clk->clk.parent))
>>> +                       clk->clk.parent = srcs->sources[clksrc];
>>
>> IMO, it make sense to return the error value if failed. Now you
>> are masking the failed cases (due to invalid parent clk) and proceeding
>> with uboot values which is not the intention when parent clock
>> is provided.
>
> yes you are right. But as the function s3c_set_clksrc() is not
> expected to return any error codes,
> I tried to use default u-boot settings.
> We can add a warning message to let user know in case of wrong parent
> assignment.
>

Agreed. Printing error val with KERN_ERR and return would be better.

> Regards,
> Shaik Ameer Basha
>
>>
>> Regards,
>> Rahul Sharma,
>>
>>> +       } else {
>>> +               clk->clk.parent = srcs->sources[clksrc];
>>> +       }
>>>
>>>         if (announce)
>>>                 printk(KERN_INFO "%s: source is %s (%d), rate is %ld\n",
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> --



-- 
Regards,
Rahul Sharma,
email to: rahul.sharma@samsung.com
Samsung India.

Re: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Sylwester Nawrocki]

On 03/07/2013 06:31 AM, Shaik Ameer Basha wrote:
> s3c_set_clksrc() updates the clock source as per u-boot settings.
> This patch adds the functionality to overwrite u-boot settings,
> if user provides the clock parent field. In case of wrong source
> provided by the user, it will retain the u-boot settings.
> 
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
> ---
>  arch/arm/plat-samsung/clock-clksrc.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
> index 786a410..4fecd80 100644
> --- a/arch/arm/plat-samsung/clock-clksrc.c
> +++ b/arch/arm/plat-samsung/clock-clksrc.c
> @@ -150,7 +150,12 @@ void __init_or_cpufreq s3c_set_clksrc(struct clksrc_clk *clk, bool announce)
>  		return;
>  	}
>  
> -	clk->clk.parent = srcs->sources[clksrc];
> +	if (clk->clk.parent) {
> +		if (s3c_setparent_clksrc(&clk->clk, clk->clk.parent))
> +			clk->clk.parent = srcs->sources[clksrc];
> +	} else {
> +		clk->clk.parent = srcs->sources[clksrc];
> +	}

May I ask what do you need this for ? This code won't be used for
Exynos4 and Exynos5 SoCs starting from 3.10. And it is going to be
removed once other platforms are converted to the new Samsung clocks
driver.

Regards,
Sylwester

Re: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Shaik Ameer Basha]

Hi Sylwester,

On Thu, Mar 7, 2013 at 9:05 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 03/07/2013 06:31 AM, Shaik Ameer Basha wrote:
>> s3c_set_clksrc() updates the clock source as per u-boot settings.
>> This patch adds the functionality to overwrite u-boot settings,
>> if user provides the clock parent field. In case of wrong source
>> provided by the user, it will retain the u-boot settings.
>>
>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>> ---
>>  arch/arm/plat-samsung/clock-clksrc.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/plat-samsung/clock-clksrc.c b/arch/arm/plat-samsung/clock-clksrc.c
>> index 786a410..4fecd80 100644
>> --- a/arch/arm/plat-samsung/clock-clksrc.c
>> +++ b/arch/arm/plat-samsung/clock-clksrc.c
>> @@ -150,7 +150,12 @@ void __init_or_cpufreq s3c_set_clksrc(struct clksrc_clk *clk, bool announce)
>>               return;
>>       }
>>
>> -     clk->clk.parent = srcs->sources[clksrc];
>> +     if (clk->clk.parent) {
>> +             if (s3c_setparent_clksrc(&clk->clk, clk->clk.parent))
>> +                     clk->clk.parent = srcs->sources[clksrc];
>> +     } else {
>> +             clk->clk.parent = srcs->sources[clksrc];
>> +     }
>
> May I ask what do you need this for ? This code won't be used for
> Exynos4 and Exynos5 SoCs starting from 3.10. And it is going to be
> removed once other platforms are converted to the new Samsung clocks
> driver.

I had some issues in exynos5 with out this implementation.
But yes... you are right, once we move to common clock framework (CCF)
we don't require this change, as CCF doesn't use this (for exynos 4/5).

What about all old non-dt based platforms?
Cant they use this change until they move to DT and CCF?

Regards,
Shaik Ameer Basha

>
> Regards,
> Sylwester

Re: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Sylwester Nawrocki]

Hi Shaik,

On 03/15/2013 10:00 AM, Shaik Ameer Basha wrote:
> On Thu, Mar 7, 2013 at 9:05 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
...
>> May I ask what do you need this for ? This code won't be used for
>> Exynos4 and Exynos5 SoCs starting from 3.10. And it is going to be
>> removed once other platforms are converted to the new Samsung clocks
>> driver.
> 
> I had some issues in exynos5 with out this implementation.
> But yes... you are right, once we move to common clock framework (CCF)
> we don't require this change, as CCF doesn't use this (for exynos 4/5).
> 
> What about all old non-dt based platforms?
> Cant they use this change until they move to DT and CCF?

I have nothing against the patch, I was just curious if you need
it for the new SOCs where the new clocks driver is supposed to be used.
Of course other Samsung platforms could switch to CCF before having
support for the device tree.

Regards,
Sylwester

RE: [PATCH] ARM: SAMSUNG: Set clock parent if provided

[Kukjin Kim]

Sylwester Nawrocki wrote:
> 
> Hi Shaik,
> 
> On 03/15/2013 10:00 AM, Shaik Ameer Basha wrote:
> > On Thu, Mar 7, 2013 at 9:05 PM, Sylwester Nawrocki
> > <s.nawrocki@samsung.com> wrote:
> ...
> >> May I ask what do you need this for ? This code won't be used for
> >> Exynos4 and Exynos5 SoCs starting from 3.10. And it is going to be
> >> removed once other platforms are converted to the new Samsung clocks
> >> driver.
> >
> > I had some issues in exynos5 with out this implementation.
> > But yes... you are right, once we move to common clock framework (CCF)
> > we don't require this change, as CCF doesn't use this (for exynos 4/5).
> >
> > What about all old non-dt based platforms?
> > Cant they use this change until they move to DT and CCF?
> 
> I have nothing against the patch, I was just curious if you need
> it for the new SOCs where the new clocks driver is supposed to be used.
> Of course other Samsung platforms could switch to CCF before having
> support for the device tree.
> 
Sorry for late response on this...

Yes, I think, as many guys said, this is not required for now for exynos4/5.

But the reason is different, I think, because the parent clock should be set depends on each board. That's why there are many parent clocks hardware providing. Sometimes just one parent clock can be used as a default though.

- Kukjin