ARM: at91: sama5d3: reduce TWI internal clock frequency
diff mbox

Message ID 1385129228-11225-1-git-send-email-ludovic.desroches@atmel.com
State Superseded
Headers show

Commit Message

ludovic.desroches@atmel.com Nov. 22, 2013, 2:07 p.m. UTC
From: Ludovic Desroches <ludovic.desroches@atmel.com>

There are still I2C unexpected behaviors which are solved by reducing TWI
internal frequency.

Cc: <stable@vger.kernel.org> #3.10+
Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 arch/arm/mach-at91/sama5d3.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Wolfram Sang Nov. 22, 2013, 2:33 p.m. UTC | #1
On Fri, Nov 22, 2013 at 03:07:08PM +0100, ludovic.desroches@atmel.com wrote:
> From: Ludovic Desroches <ludovic.desroches@atmel.com>
> 
> There are still I2C unexpected behaviors which are solved by reducing TWI
> internal frequency.
> 
> Cc: <stable@vger.kernel.org> #3.10+
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>

I think the commit message needs more details. Is this a true bugfix
because the real bus frequency was too high because of the wrong
divider? Is this a workaround which makes things work but will make the
bus frequency slower than it should be?

> ---
>  arch/arm/mach-at91/sama5d3.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
> index 4012797..4ee0de5 100644
> --- a/arch/arm/mach-at91/sama5d3.c
> +++ b/arch/arm/mach-at91/sama5d3.c
> @@ -95,19 +95,19 @@ static struct clk twi0_clk = {
>  	.name		= "twi0_clk",
>  	.pid		= SAMA5D3_ID_TWI0,
>  	.type		= CLK_TYPE_PERIPHERAL,
> -	.div		= AT91_PMC_PCR_DIV2,
> +	.div		= AT91_PMC_PCR_DIV8,
>  };
>  static struct clk twi1_clk = {
>  	.name		= "twi1_clk",
>  	.pid		= SAMA5D3_ID_TWI1,
>  	.type		= CLK_TYPE_PERIPHERAL,
> -	.div		= AT91_PMC_PCR_DIV2,
> +	.div		= AT91_PMC_PCR_DIV8,
>  };
>  static struct clk twi2_clk = {
>  	.name		= "twi2_clk",
>  	.pid		= SAMA5D3_ID_TWI2,
>  	.type		= CLK_TYPE_PERIPHERAL,
> -	.div		= AT91_PMC_PCR_DIV2,
> +	.div		= AT91_PMC_PCR_DIV8,
>  };
>  static struct clk mmc0_clk = {
>  	.name		= "mci0_clk",
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Nov. 22, 2013, 2:58 p.m. UTC | #2
On Fri, Nov 22, 2013 at 03:51:41PM +0100, Ludovic Desroches wrote:
> Hi Wolfram,
> 
> On Fri, Nov 22, 2013 at 03:33:51PM +0100, Wolfram Sang wrote:
> > On Fri, Nov 22, 2013 at 03:07:08PM +0100, ludovic.desroches@atmel.com wrote:
> > > From: Ludovic Desroches <ludovic.desroches@atmel.com>
> > > 
> > > There are still I2C unexpected behaviors which are solved by reducing TWI
> > > internal frequency.
> > > 
> > > Cc: <stable@vger.kernel.org> #3.10+
> > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > 
> > I think the commit message needs more details. Is this a true bugfix
> > because the real bus frequency was too high because of the wrong
> > divider? Is this a workaround which makes things work but will make the
> > bus frequency slower than it should be?
> 
> This fix doesn't concern the i2c bus frequency, only the internal IP frequency.
> 
> TWI has been validated at 66MHz. With some devices, transfer hangs during i2c
> frame transmission. This issue disappears when reducing the internal frequency
> of the IP. Maybe there is some oversampling on i2c signals.
> Unfortunately, I have no clear status about the root cause that's why
> the commit message was imprecise.

This paragraph is a way better commit message IMO :)
Boris Brezillon Nov. 22, 2013, 3:01 p.m. UTC | #3
Hi Ludovic,

On 22/11/2013 15:07, ludovic.desroches@atmel.com wrote:
> From: Ludovic Desroches <ludovic.desroches@atmel.com>
>
> There are still I2C unexpected behaviors which are solved by reducing TWI
> internal frequency.

I guess I should do the same for the dt version of sama5 clks.
Nicolas, what do you think ?

Best Regards,

Boris

>
> Cc: <stable@vger.kernel.org> #3.10+
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>   arch/arm/mach-at91/sama5d3.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
> index 4012797..4ee0de5 100644
> --- a/arch/arm/mach-at91/sama5d3.c
> +++ b/arch/arm/mach-at91/sama5d3.c
> @@ -95,19 +95,19 @@ static struct clk twi0_clk = {
>   	.name		= "twi0_clk",
>   	.pid		= SAMA5D3_ID_TWI0,
>   	.type		= CLK_TYPE_PERIPHERAL,
> -	.div		= AT91_PMC_PCR_DIV2,
> +	.div		= AT91_PMC_PCR_DIV8,
>   };
>   static struct clk twi1_clk = {
>   	.name		= "twi1_clk",
>   	.pid		= SAMA5D3_ID_TWI1,
>   	.type		= CLK_TYPE_PERIPHERAL,
> -	.div		= AT91_PMC_PCR_DIV2,
> +	.div		= AT91_PMC_PCR_DIV8,
>   };
>   static struct clk twi2_clk = {
>   	.name		= "twi2_clk",
>   	.pid		= SAMA5D3_ID_TWI2,
>   	.type		= CLK_TYPE_PERIPHERAL,
> -	.div		= AT91_PMC_PCR_DIV2,
> +	.div		= AT91_PMC_PCR_DIV8,
>   };
>   static struct clk mmc0_clk = {
>   	.name		= "mci0_clk",
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Ferre Nov. 22, 2013, 3:09 p.m. UTC | #4
On 22/11/2013 16:07, Ludovic Desroches :
> On Fri, Nov 22, 2013 at 03:58:35PM +0100, Wolfram Sang wrote:
>> On Fri, Nov 22, 2013 at 03:51:41PM +0100, Ludovic Desroches wrote:
>>> Hi Wolfram,
>>>
>>> On Fri, Nov 22, 2013 at 03:33:51PM +0100, Wolfram Sang wrote:
>>>> On Fri, Nov 22, 2013 at 03:07:08PM +0100, ludovic.desroches@atmel.com wrote:
>>>>> From: Ludovic Desroches <ludovic.desroches@atmel.com>
>>>>>
>>>>> There are still I2C unexpected behaviors which are solved by reducing TWI
>>>>> internal frequency.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> #3.10+
>>>>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>>>>
>>>> I think the commit message needs more details. Is this a true bugfix
>>>> because the real bus frequency was too high because of the wrong
>>>> divider? Is this a workaround which makes things work but will make the
>>>> bus frequency slower than it should be?
>>>
>>> This fix doesn't concern the i2c bus frequency, only the internal IP frequency.
>>>
>>> TWI has been validated at 66MHz. With some devices, transfer hangs during i2c
>>> frame transmission. This issue disappears when reducing the internal frequency
>>> of the IP. Maybe there is some oversampling on i2c signals.
>>> Unfortunately, I have no clear status about the root cause that's why
>>> the commit message was imprecise.
>>
>> This paragraph is a way better commit message IMO :)
>>
>
> Ok I'll update it.


Ludo, you can also add my:

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Bye,

> Thanks for the review Wolfram.
>
> Regards
>
> Ludovic
>
>
Nicolas Ferre Nov. 22, 2013, 3:11 p.m. UTC | #5
On 22/11/2013 16:01, boris brezillon :
> Hi Ludovic,
>
> On 22/11/2013 15:07, ludovic.desroches@atmel.com wrote:
>> From: Ludovic Desroches <ludovic.desroches@atmel.com>
>>
>> There are still I2C unexpected behaviors which are solved by reducing TWI
>> internal frequency.
>
> I guess I should do the same for the dt version of sama5 clks.
> Nicolas, what do you think ?

Yes, you can queue these modification on top of your "CCF related fixes" 
branch.

Bye,

>> Cc: <stable@vger.kernel.org> #3.10+
>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>> ---
>>    arch/arm/mach-at91/sama5d3.c |    6 +++---
>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
>> index 4012797..4ee0de5 100644
>> --- a/arch/arm/mach-at91/sama5d3.c
>> +++ b/arch/arm/mach-at91/sama5d3.c
>> @@ -95,19 +95,19 @@ static struct clk twi0_clk = {
>>    	.name		= "twi0_clk",
>>    	.pid		= SAMA5D3_ID_TWI0,
>>    	.type		= CLK_TYPE_PERIPHERAL,
>> -	.div		= AT91_PMC_PCR_DIV2,
>> +	.div		= AT91_PMC_PCR_DIV8,
>>    };
>>    static struct clk twi1_clk = {
>>    	.name		= "twi1_clk",
>>    	.pid		= SAMA5D3_ID_TWI1,
>>    	.type		= CLK_TYPE_PERIPHERAL,
>> -	.div		= AT91_PMC_PCR_DIV2,
>> +	.div		= AT91_PMC_PCR_DIV8,
>>    };
>>    static struct clk twi2_clk = {
>>    	.name		= "twi2_clk",
>>    	.pid		= SAMA5D3_ID_TWI2,
>>    	.type		= CLK_TYPE_PERIPHERAL,
>> -	.div		= AT91_PMC_PCR_DIV2,
>> +	.div		= AT91_PMC_PCR_DIV8,
>>    };
>>    static struct clk mmc0_clk = {
>>    	.name		= "mci0_clk",
>>
>
>
>

Patch
diff mbox

diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
index 4012797..4ee0de5 100644
--- a/arch/arm/mach-at91/sama5d3.c
+++ b/arch/arm/mach-at91/sama5d3.c
@@ -95,19 +95,19 @@  static struct clk twi0_clk = {
 	.name		= "twi0_clk",
 	.pid		= SAMA5D3_ID_TWI0,
 	.type		= CLK_TYPE_PERIPHERAL,
-	.div		= AT91_PMC_PCR_DIV2,
+	.div		= AT91_PMC_PCR_DIV8,
 };
 static struct clk twi1_clk = {
 	.name		= "twi1_clk",
 	.pid		= SAMA5D3_ID_TWI1,
 	.type		= CLK_TYPE_PERIPHERAL,
-	.div		= AT91_PMC_PCR_DIV2,
+	.div		= AT91_PMC_PCR_DIV8,
 };
 static struct clk twi2_clk = {
 	.name		= "twi2_clk",
 	.pid		= SAMA5D3_ID_TWI2,
 	.type		= CLK_TYPE_PERIPHERAL,
-	.div		= AT91_PMC_PCR_DIV2,
+	.div		= AT91_PMC_PCR_DIV8,
 };
 static struct clk mmc0_clk = {
 	.name		= "mci0_clk",