Patchwork [2/2] ARM: mach-imx: CAN clock fix for i.MX53

login
register
mail settings
Submitter stigge@antcom.de
Date Oct. 25, 2012, 11:26 a.m.
Message ID <1351164400-27940-2-git-send-email-stigge@antcom.de>
Download mbox | patch
Permalink /patch/194120/
State New
Headers show

Comments

stigge@antcom.de - Oct. 25, 2012, 11:26 a.m.
This patch fixes CAN clocking on i.MX53.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 arch/arm/mach-imx/clk-imx51-imx53.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Sascha Hauer - Oct. 26, 2012, 8:59 a.m.
On Thu, Oct 25, 2012 at 01:26:40PM +0200, Roland Stigge wrote:
> This patch fixes CAN clocking on i.MX53.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> ---
>  arch/arm/mach-imx/clk-imx51-imx53.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-2.6.orig/arch/arm/mach-imx/clk-imx51-imx53.c
> +++ linux-2.6/arch/arm/mach-imx/clk-imx51-imx53.c
> @@ -426,10 +426,10 @@ int __init mx53_clocks_init(unsigned lon
>  	clk[usb_phy2_gate] = imx_clk_gate2("usb_phy2_gate", "usb_phy_sel", MXC_CCM_CCGR4, 12);
>  	clk[can_sel] = imx_clk_mux("can_sel", MXC_CCM_CSCMR2, 6, 2,
>  				mx53_can_sel, ARRAY_SIZE(mx53_can_sel));
> -	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR6, 22);
> -	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR6, 20);
> -	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 8);
> -	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 6);

I just rechecked. The above matches the i.MX53 Reference Manual rev 2.1

> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
> +	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
> +	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
> +	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);

This doesn't.

Can you elaborate why you think this is necessary?

Sascha
stigge@antcom.de - Oct. 26, 2012, 9:16 a.m.
Hi!

On 10/26/2012 10:59 AM, Sascha Hauer wrote:
> On Thu, Oct 25, 2012 at 01:26:40PM +0200, Roland Stigge wrote:
>> This patch fixes CAN clocking on i.MX53.
>>
>> Signed-off-by: Roland Stigge <stigge@antcom.de>
>>
>> ---
>>  arch/arm/mach-imx/clk-imx51-imx53.c |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> --- linux-2.6.orig/arch/arm/mach-imx/clk-imx51-imx53.c
>> +++ linux-2.6/arch/arm/mach-imx/clk-imx51-imx53.c
>> @@ -426,10 +426,10 @@ int __init mx53_clocks_init(unsigned lon
>>  	clk[usb_phy2_gate] = imx_clk_gate2("usb_phy2_gate", "usb_phy_sel", MXC_CCM_CCGR4, 12);
>>  	clk[can_sel] = imx_clk_mux("can_sel", MXC_CCM_CSCMR2, 6, 2,
>>  				mx53_can_sel, ARRAY_SIZE(mx53_can_sel));
>> -	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR6, 22);
>> -	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR6, 20);
>> -	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 8);
>> -	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 6);
> 
> I just rechecked. The above matches the i.MX53 Reference Manual rev 2.1
> 
>> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
>> +	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>> +	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
>> +	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
> 
> This doesn't.

This may be right, but unfortunately, since the introduction of your
can1 + can2 clocking change, the first block stopped working for me.

My above patch is basically a rollback which works for the first block.

An interesting hint is: can2 (which was the only one defined _before_
your can1 + can2 change) didn't work before and afterwards at all. (But
I'm not using it anyway.)

Thanks,

Roland
Marc Kleine-Budde - Oct. 26, 2012, 10:30 a.m.
On 10/26/2012 11:16 AM, Roland Stigge wrote:
> Hi!
> 
> On 10/26/2012 10:59 AM, Sascha Hauer wrote:
>> On Thu, Oct 25, 2012 at 01:26:40PM +0200, Roland Stigge wrote:
>>> This patch fixes CAN clocking on i.MX53.
>>>
>>> Signed-off-by: Roland Stigge <stigge@antcom.de>
>>>
>>> ---
>>>  arch/arm/mach-imx/clk-imx51-imx53.c |    8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> --- linux-2.6.orig/arch/arm/mach-imx/clk-imx51-imx53.c
>>> +++ linux-2.6/arch/arm/mach-imx/clk-imx51-imx53.c
>>> @@ -426,10 +426,10 @@ int __init mx53_clocks_init(unsigned lon
>>>  	clk[usb_phy2_gate] = imx_clk_gate2("usb_phy2_gate", "usb_phy_sel", MXC_CCM_CCGR4, 12);
>>>  	clk[can_sel] = imx_clk_mux("can_sel", MXC_CCM_CSCMR2, 6, 2,
>>>  				mx53_can_sel, ARRAY_SIZE(mx53_can_sel));
>>> -	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR6, 22);
>>> -	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR6, 20);
>>> -	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 8);
>>> -	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 6);
>>
>> I just rechecked. The above matches the i.MX53 Reference Manual rev 2.1

An old rev 1 manual has lists the same bits.

>>> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
>>> +	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>>> +	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
>>> +	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>>
>> This doesn't.
> 
> This may be right, but unfortunately, since the introduction of your
> can1 + can2 clocking change, the first block stopped working for me.

You are effectively using can2's clock for can1. Are you sure you
haven't mixed up can1 and can2?

> My above patch is basically a rollback which works for the first block.
> 
> An interesting hint is: can2 (which was the only one defined _before_
> your can1 + can2 change) didn't work before and afterwards at all. (But
> I'm not using it anyway.)

Marc
stigge@antcom.de - Oct. 26, 2012, 11:05 a.m.
On 10/26/2012 12:30 PM, Marc Kleine-Budde wrote:
>>>> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate",
>>>> "can_sel", MXC_CCM_CCGR4, 6); +	clk[can1_ipg_gate] =
>>>> imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8); +
>>>> clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate",
>>>> "can_sel", MXC_CCM_CCGR4, 6); +	clk[can2_ipg_gate] =
>>>> imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>>> 
>>> This doesn't.
>> 
>> This may be right, but unfortunately, since the introduction of
>> your can1 + can2 clocking change, the first block stopped working
>> for me.
> 
> You are effectively using can2's clock for can1. Are you sure you 
> haven't mixed up can1 and can2?

Just using the above patch and patch 1/2 from this series (missing can
pinmuxing), doing like this in custom .dts:

+                       can1: can@53fc8000 {
+                               pinctrl-names = "default";
+                               pinctrl-0 = <&pinctrl_can1_1>;
+                               clock-frequency = <66500000>;
+                               status = "okay";
+                       };

Then, the can0 interface appears which can be upped as network interface.

Quite straightforward, isn't it?

Roland
stigge@antcom.de - Oct. 26, 2012, 1:52 p.m.
On 10/26/2012 01:05 PM, Roland Stigge wrote:
> On 10/26/2012 12:30 PM, Marc Kleine-Budde wrote:
>>>>> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate",
>>>>> "can_sel", MXC_CCM_CCGR4, 6); +	clk[can1_ipg_gate] =
>>>>> imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8); +
>>>>> clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate",
>>>>> "can_sel", MXC_CCM_CCGR4, 6); +	clk[can2_ipg_gate] =
>>>>> imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>>>>
>>>> This doesn't.
>>>
>>> This may be right, but unfortunately, since the introduction of
>>> your can1 + can2 clocking change, the first block stopped working
>>> for me.
>>
>> You are effectively using can2's clock for can1. Are you sure you 
>> haven't mixed up can1 and can2?
> 
> Just using the above patch and patch 1/2 from this series (missing can
> pinmuxing), doing like this in custom .dts:
> 
> +                       can1: can@53fc8000 {
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&pinctrl_can1_1>;
> +                               clock-frequency = <66500000>;

That was the problem, since in flexcan.c probe():


	if (pdev->dev.of_node)
		of_property_read_u32(pdev->dev.of_node,
						"clock-frequency", &clock_freq);

	if (!clock_freq) {
		clk_ipg = devm_clk_get(&pdev->dev, "ipg");
		if (IS_ERR(clk_ipg)) {
			dev_err(&pdev->dev, "no ipg clock defined\n");
			err = PTR_ERR(clk_ipg);
			goto failed_clock;
		}
		clock_freq = clk_get_rate(clk_ipg);

		clk_per = devm_clk_get(&pdev->dev, "per");
		if (IS_ERR(clk_per)) {
			dev_err(&pdev->dev, "no per clock defined\n");
			err = PTR_ERR(clk_per);
			goto failed_clock;
		}
	}

Sorry for the noise.

The other patches are still current.

Thanks,

Roland
Marc Kleine-Budde - Oct. 26, 2012, 2:05 p.m.
On 10/26/2012 03:52 PM, Roland Stigge wrote:
> On 10/26/2012 01:05 PM, Roland Stigge wrote:
>> On 10/26/2012 12:30 PM, Marc Kleine-Budde wrote:
>>>>>> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate",
>>>>>> "can_sel", MXC_CCM_CCGR4, 6); +	clk[can1_ipg_gate] =
>>>>>> imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8); +
>>>>>> clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate",
>>>>>> "can_sel", MXC_CCM_CCGR4, 6); +	clk[can2_ipg_gate] =
>>>>>> imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>>>>>
>>>>> This doesn't.
>>>>
>>>> This may be right, but unfortunately, since the introduction of
>>>> your can1 + can2 clocking change, the first block stopped working
>>>> for me.
>>>
>>> You are effectively using can2's clock for can1. Are you sure you 
>>> haven't mixed up can1 and can2?
>>
>> Just using the above patch and patch 1/2 from this series (missing can
>> pinmuxing), doing like this in custom .dts:
>>
>> +                       can1: can@53fc8000 {
>> +                               pinctrl-names = "default";
>> +                               pinctrl-0 = <&pinctrl_can1_1>;
>> +                               clock-frequency = <66500000>;
> 
> That was the problem, since in flexcan.c probe():
> 
> 
> 	if (pdev->dev.of_node)
> 		of_property_read_u32(pdev->dev.of_node,
> 						"clock-frequency", &clock_freq);
> 
> 	if (!clock_freq) {
> 		clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> 		if (IS_ERR(clk_ipg)) {
> 			dev_err(&pdev->dev, "no ipg clock defined\n");
> 			err = PTR_ERR(clk_ipg);
> 			goto failed_clock;
> 		}
> 		clock_freq = clk_get_rate(clk_ipg);
> 
> 		clk_per = devm_clk_get(&pdev->dev, "per");
> 		if (IS_ERR(clk_per)) {
> 			dev_err(&pdev->dev, "no per clock defined\n");
> 			err = PTR_ERR(clk_per);
> 			goto failed_clock;
> 		}
> 	}
> 
> Sorry for the noise.

We have the possibility to specify the clock frequency in the device
tree as the power pc has no support for generic clock yet.

Marc
Sascha Hauer - Oct. 29, 2012, 9:20 p.m.
Roland,

Do you have updates on this one? I think this patch is wrong, specifying
the clock rate in the devicetree is only for PowerPC, so I think this
needs further investigation.

Sascha

On Thu, Oct 25, 2012 at 01:26:40PM +0200, Roland Stigge wrote:
> This patch fixes CAN clocking on i.MX53.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> ---
>  arch/arm/mach-imx/clk-imx51-imx53.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-2.6.orig/arch/arm/mach-imx/clk-imx51-imx53.c
> +++ linux-2.6/arch/arm/mach-imx/clk-imx51-imx53.c
> @@ -426,10 +426,10 @@ int __init mx53_clocks_init(unsigned lon
>  	clk[usb_phy2_gate] = imx_clk_gate2("usb_phy2_gate", "usb_phy_sel", MXC_CCM_CCGR4, 12);
>  	clk[can_sel] = imx_clk_mux("can_sel", MXC_CCM_CSCMR2, 6, 2,
>  				mx53_can_sel, ARRAY_SIZE(mx53_can_sel));
> -	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR6, 22);
> -	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR6, 20);
> -	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 8);
> -	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 6);
> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
> +	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
> +	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
> +	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>  	clk[i2c3_gate] = imx_clk_gate2("i2c3_gate", "per_root", MXC_CCM_CCGR1, 22);
>  
>  	for (i = 0; i < ARRAY_SIZE(clk); i++)
>
stigge@antcom.de - Oct. 29, 2012, 9:31 p.m.
On 29/10/12 22:20, Sascha Hauer wrote:
> Roland,
> 
> Do you have updates on this one? I think this patch is wrong, specifying
> the clock rate in the devicetree is only for PowerPC, so I think this
> needs further investigation.

Please ignore it for now. As discussed with Marc, the issue seemed to be
caused by me (wrongfully) using the "clock-frequency" dt property which
caused probe() to ignore the clocks.

A colleague of mine will notify me if the problem surfaces again. So for
now I assume it's gone.

Maybe the above strategy regarding "clock-frequency" needs to be
documented better or changed, though, to prevent more confusion like in
the above case.

Thanks,

Roland

> On Thu, Oct 25, 2012 at 01:26:40PM +0200, Roland Stigge wrote:
>> This patch fixes CAN clocking on i.MX53.
>>
>> Signed-off-by: Roland Stigge <stigge@antcom.de>
>>
>> ---
>>  arch/arm/mach-imx/clk-imx51-imx53.c |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> --- linux-2.6.orig/arch/arm/mach-imx/clk-imx51-imx53.c
>> +++ linux-2.6/arch/arm/mach-imx/clk-imx51-imx53.c
>> @@ -426,10 +426,10 @@ int __init mx53_clocks_init(unsigned lon
>>  	clk[usb_phy2_gate] = imx_clk_gate2("usb_phy2_gate", "usb_phy_sel", MXC_CCM_CCGR4, 12);
>>  	clk[can_sel] = imx_clk_mux("can_sel", MXC_CCM_CSCMR2, 6, 2,
>>  				mx53_can_sel, ARRAY_SIZE(mx53_can_sel));
>> -	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR6, 22);
>> -	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR6, 20);
>> -	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 8);
>> -	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 6);
>> +	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
>> +	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>> +	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
>> +	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
>>  	clk[i2c3_gate] = imx_clk_gate2("i2c3_gate", "per_root", MXC_CCM_CCGR1, 22);
>>  
>>  	for (i = 0; i < ARRAY_SIZE(clk); i++)
>>
>

Patch

--- linux-2.6.orig/arch/arm/mach-imx/clk-imx51-imx53.c
+++ linux-2.6/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -426,10 +426,10 @@  int __init mx53_clocks_init(unsigned lon
 	clk[usb_phy2_gate] = imx_clk_gate2("usb_phy2_gate", "usb_phy_sel", MXC_CCM_CCGR4, 12);
 	clk[can_sel] = imx_clk_mux("can_sel", MXC_CCM_CSCMR2, 6, 2,
 				mx53_can_sel, ARRAY_SIZE(mx53_can_sel));
-	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR6, 22);
-	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR6, 20);
-	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 8);
-	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 6);
+	clk[can1_serial_gate] = imx_clk_gate2("can1_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
+	clk[can1_ipg_gate] = imx_clk_gate2("can1_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
+	clk[can2_serial_gate] = imx_clk_gate2("can2_serial_gate", "can_sel", MXC_CCM_CCGR4, 6);
+	clk[can2_ipg_gate] = imx_clk_gate2("can2_ipg_gate", "ipg", MXC_CCM_CCGR4, 8);
 	clk[i2c3_gate] = imx_clk_gate2("i2c3_gate", "per_root", MXC_CCM_CCGR1, 22);
 
 	for (i = 0; i < ARRAY_SIZE(clk); i++)