diff mbox

ARM: at91: sama5d3: reduce TWI internal clock frequency

Message ID 1385129228-11225-1-git-send-email-ludovic.desroches@atmel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ludovic Desroches 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
Ludovic Desroches Nov. 22, 2013, 2:51 p.m. UTC | #2 
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.


Regards

Ludovic
> 
> > ---
> >  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 | #3 
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 | #4 
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",
>
Ludovic Desroches Nov. 22, 2013, 3:07 p.m. UTC | #5 
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.

Thanks for the review Wolfram.

Regards

Ludovic
Nicolas Ferre Nov. 22, 2013, 3:09 p.m. UTC | #6 
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 | #7 
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",
>>
>
>
>
diff mbox

Patch

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",