[PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w]

From: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

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

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #3.10+
Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 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

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On Fri, Nov 22, 2013 at 03:07:08PM +0100, ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org wrote:
> From: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> 
> There are still I2C unexpected behaviors which are solved by reducing TWI
> internal frequency.
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #3.10+
> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[Ludovic Desroches]

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

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

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-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org wrote:
> > > From: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > > 
> > > There are still I2C unexpected behaviors which are solved by reducing TWI
> > > internal frequency.
> > > 
> > > Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #3.10+
> > > Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > 
> > 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 :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

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

Thanks for the review Wolfram.

Regards

Ludovic

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[Nicolas Ferre]

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-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org wrote:
>>>>> From: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>>>>
>>>>> There are still I2C unexpected behaviors which are solved by reducing TWI
>>>>> internal frequency.
>>>>>
>>>>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #3.10+
>>>>> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>>>
>>>> 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-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Bye,

> Thanks for the review Wolfram.
>
> Regards
>
> Ludovic
>
>


-- 
Nicolas Ferre

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[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 ?

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

Re: [PATCH] ARM: at91: sama5d3: reduce TWI internal clock frequency

[Nicolas Ferre]

On 22/11/2013 16:01, boris brezillon :
> Hi Ludovic,
>
> On 22/11/2013 15:07, ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org wrote:
>> From: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #3.10+
>> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>    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",
>>
>
>
>


-- 
Nicolas Ferre