[PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

Instead of rounding the divider down, improve the baud-rate generators
accuracy by rounding to the nearest integer.

Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
---
 arch/powerpc/sysdev/cpm2.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/sysdev/cpm2.c b/arch/powerpc/sysdev/cpm2.c
index f1c3395..474d176 100644
--- a/arch/powerpc/sysdev/cpm2.c
+++ b/arch/powerpc/sysdev/cpm2.c
@@ -129,7 +129,8 @@ void __cpm2_setbrg(uint brg, uint rate, uint clk, int div16, int src)
 		brg -= 4;
 	}
 	bp += brg;
-	val = (((clk / rate) - 1) << 1) | CPM_BRG_EN | src;
+	/* Round the clock divider to the nearest integer. */
+	val = (((clk * 2 / rate) - 1) & ~1) | CPM_BRG_EN | src;
 	if (div16)
 		val |= CPM_BRG_DIV16;
 
-- 
1.5.6.3

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Kumar Gala]

On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:

> Instead of rounding the divider down, improve the baud-rate generators
> accuracy by rounding to the nearest integer.
>
> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
> ---
> arch/powerpc/sysdev/cpm2.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)

is this fixing a bug or just making things better?

- k

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

Hi Kumar,

On Thursday 20 November 2008 16:32:21 Kumar Gala wrote:
> On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:
> > Instead of rounding the divider down, improve the baud-rate generators
> > accuracy by rounding to the nearest integer.
> >
> > Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
> > ---
> > arch/powerpc/sysdev/cpm2.c |    3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
>
> is this fixing a bug or just making things better?

I guess it depends on your definition of bug :-) The problem has always been 
present in the kernel sources, and people seem to have coped with it until 
today. It is still a bug in my opinion, but I suppose the patch can wait until 
2.6.29 especially if we want to get it tested (it slightly changes baud rates 
after all).

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Kumar Gala]

On Nov 20, 2008, at 9:57 AM, Laurent Pinchart wrote:

> Hi Kumar,
>
> On Thursday 20 November 2008 16:32:21 Kumar Gala wrote:
>> On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:
>>> Instead of rounding the divider down, improve the baud-rate  
>>> generators
>>> accuracy by rounding to the nearest integer.
>>>
>>> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
>>> ---
>>> arch/powerpc/sysdev/cpm2.c |    3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> is this fixing a bug or just making things better?
>
> I guess it depends on your definition of bug :-) The problem has  
> always been
> present in the kernel sources, and people seem to have coped with it  
> until
> today. It is still a bug in my opinion, but I suppose the patch can  
> wait until
> 2.6.29 especially if we want to get it tested (it slightly changes  
> baud rates
> after all).

Ok. I'll put this in the .29 queue.  Can you provide a bit more detail  
on the issue this is resolving.

- k

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

On Thursday 20 November 2008 17:00:54 Kumar Gala wrote:
> On Nov 20, 2008, at 9:57 AM, Laurent Pinchart wrote:
> > On Thursday 20 November 2008 16:32:21 Kumar Gala wrote:
> >> On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:
> >>> Instead of rounding the divider down, improve the baud-rate
> >>> generators
> >>> accuracy by rounding to the nearest integer.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
> >>> ---
> >>> arch/powerpc/sysdev/cpm2.c |    3 ++-
> >>> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> is this fixing a bug or just making things better?
> >
> > I guess it depends on your definition of bug :-) The problem has
> > always been present in the kernel sources, and people seem to have coped
> > with it until today. It is still a bug in my opinion, but I suppose the
> > patch can wait until 2.6.29 especially if we want to get it tested (it
> > slightly changes baud rates after all).
>
> Ok. I'll put this in the .29 queue.  Can you provide a bit more detail
> on the issue this is resolving.

Sure.

Let's assume a 25 MHz BRG clock. When setting a 115200 bds baud rate for an 
SMC port in UART mode (16x oversampling), the divisor should be

25e6/(16*115200) ~= 13.563368

The current code rounds this down. The resulting baud rate is

25e6/(16*13) ~= 120192 bds

The relative error is thus (115200 - 120192)/115200 ~= 4.33%

With the new code, the divisor is rounded to the nearest integer (14). The 
resulting baud rate is

25e6/(16*14) ~= 111607 bds

and the relative error becomes (115200 - 111607)/115200 ~= 3.12%

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

Hi Kumar,

On Thursday 20 November 2008 17:00:54 Kumar Gala wrote:
> On Nov 20, 2008, at 9:57 AM, Laurent Pinchart wrote:
> > On Thursday 20 November 2008 16:32:21 Kumar Gala wrote:
> >> On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:
> >>> Instead of rounding the divider down, improve the baud-rate
> >>> generators
> >>> accuracy by rounding to the nearest integer.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
> >>> ---
> >>> arch/powerpc/sysdev/cpm2.c |    3 ++-
> >>> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> is this fixing a bug or just making things better?
> >
> > I guess it depends on your definition of bug :-) The problem has
> > always been present in the kernel sources, and people seem to have coped
> > with it until today. It is still a bug in my opinion, but I suppose the
> > patch can wait until 2.6.29 especially if we want to get it tested (it
> > slightly changes baud rates after all).
>
> Ok. I'll put this in the .29 queue.  Can you provide a bit more detail
> on the issue this is resolving.

2.6.29-rc1 is out and I haven't seen the patch being applied anywhere. Is 
there any blocking issue ?

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Kumar Gala]

On Jan 15, 2009, at 7:18 AM, Laurent Pinchart wrote:

> Hi Kumar,
>
> On Thursday 20 November 2008 17:00:54 Kumar Gala wrote:
>> On Nov 20, 2008, at 9:57 AM, Laurent Pinchart wrote:
>>> On Thursday 20 November 2008 16:32:21 Kumar Gala wrote:
>>>> On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:
>>>>> Instead of rounding the divider down, improve the baud-rate
>>>>> generators
>>>>> accuracy by rounding to the nearest integer.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
>>>>> ---
>>>>> arch/powerpc/sysdev/cpm2.c |    3 ++-
>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> is this fixing a bug or just making things better?
>>>
>>> I guess it depends on your definition of bug :-) The problem has
>>> always been present in the kernel sources, and people seem to have  
>>> coped
>>> with it until today. It is still a bug in my opinion, but I  
>>> suppose the
>>> patch can wait until 2.6.29 especially if we want to get it tested  
>>> (it
>>> slightly changes baud rates after all).
>>
>> Ok. I'll put this in the .29 queue.  Can you provide a bit more  
>> detail
>> on the issue this is resolving.
>
> 2.6.29-rc1 is out and I haven't seen the patch being applied  
> anywhere. Is
> there any blocking issue ?

I wasn't sure if you and Scott had come to resolution on the rounding  
issue.

- k

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

On Thursday 15 January 2009 15:42:14 Kumar Gala wrote:
> On Jan 15, 2009, at 7:18 AM, Laurent Pinchart wrote:
> >
> > 2.6.29-rc1 is out and I haven't seen the patch being applied
> > anywhere. Is there any blocking issue ?
>
> I wasn't sure if you and Scott had come to resolution on the rounding
> issue.

I think we have. Scott, can you confirm ?

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Scott Wood]

On Thu, Jan 15, 2009 at 03:58:50PM +0100, Laurent Pinchart wrote:
> On Thursday 15 January 2009 15:42:14 Kumar Gala wrote:
> > On Jan 15, 2009, at 7:18 AM, Laurent Pinchart wrote:
> > >
> > > 2.6.29-rc1 is out and I haven't seen the patch being applied
> > > anywhere. Is there any blocking issue ?
> >
> > I wasn't sure if you and Scott had come to resolution on the rounding
> > issue.
> 
> I think we have. Scott, can you confirm ?

I think the patch is correct, and I need to look into why it gives me
problems on mpc8272ads (while solving similar issues on some other
boards).

-Scott

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Kumar Gala]

On Jan 15, 2009, at 11:54 AM, Scott Wood wrote:

> On Thu, Jan 15, 2009 at 03:58:50PM +0100, Laurent Pinchart wrote:
>> On Thursday 15 January 2009 15:42:14 Kumar Gala wrote:
>>> On Jan 15, 2009, at 7:18 AM, Laurent Pinchart wrote:
>>>>
>>>> 2.6.29-rc1 is out and I haven't seen the patch being applied
>>>> anywhere. Is there any blocking issue ?
>>>
>>> I wasn't sure if you and Scott had come to resolution on the  
>>> rounding
>>> issue.
>>
>> I think we have. Scott, can you confirm ?
>
> I think the patch is correct, and I need to look into why it gives me
> problems on mpc8272ads (while solving similar issues on some other
> boards).

Ok.

Laurent,

Is this critical for 2.6.29 or ok if I queue for .30?

- k

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

On Thursday 15 January 2009 19:57:44 Kumar Gala wrote:
> On Jan 15, 2009, at 11:54 AM, Scott Wood wrote:
> > On Thu, Jan 15, 2009 at 03:58:50PM +0100, Laurent Pinchart wrote:
> >> On Thursday 15 January 2009 15:42:14 Kumar Gala wrote:
> >>> On Jan 15, 2009, at 7:18 AM, Laurent Pinchart wrote:
> >>>> 2.6.29-rc1 is out and I haven't seen the patch being applied
> >>>> anywhere. Is there any blocking issue ?
> >>>
> >>> I wasn't sure if you and Scott had come to resolution on the
> >>> rounding
> >>> issue.
> >>
> >> I think we have. Scott, can you confirm ?
> >
> > I think the patch is correct, and I need to look into why it gives me
> > problems on mpc8272ads (while solving similar issues on some other
> > boards).
>
> Ok.
>
> Laurent,
>
> Is this critical for 2.6.29 or ok if I queue for .30?

You can queue the patch for 2.6.30. Just don't forget this time ;-)

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Scott Wood]

On Thu, Nov 20, 2008 at 01:32:23PM +0100, Laurent Pinchart wrote:
> Instead of rounding the divider down, improve the baud-rate generators
> accuracy by rounding to the nearest integer.
> 
> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>

This makes things worse on mpc8272ads -- the result of the division is
13.56, but I only get error-free serial output when rounding down.  I
don't think the remote end has timing problems, as I don't see this with
any other board.

Possibly the crystal is a little off, or maybe it's just better to be a
little too fast than a little too slow for some reason?

Maybe we should change CPM_CLK to be MAIN_CLK/4 rather than MAIN_CLK/16
when the BRG clock is below a certain frequency.

-Scott

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Laurent Pinchart]

Hi Scott,

On Thursday 20 November 2008, Scott Wood wrote:
> On Thu, Nov 20, 2008 at 01:32:23PM +0100, Laurent Pinchart wrote:
> > Instead of rounding the divider down, improve the baud-rate generators
> > accuracy by rounding to the nearest integer.
> >
> > Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
>
> This makes things worse on mpc8272ads -- the result of the division is
> 13.56, but I only get error-free serial output when rounding down.  I
> don't think the remote end has timing problems, as I don't see this with
> any other board.

I get exactly the opposite here with an mpc8248 base board. Rounding down 
makes 115200bds completely unreliable :-/

Could you measure the serial baudrate with and without the patch using an 
oscilloscope ?

> Possibly the crystal is a little off,

That's always possible.

> or maybe it's just better to be a little too fast than a little too slow for
> some reason? 
>
> Maybe we should change CPM_CLK to be MAIN_CLK/4 rather than MAIN_CLK/16
> when the BRG clock is below a certain frequency.

Isn't that the job of the boot loader ? Accuracy would be improved, but power 
consumption will raise as well :-/

Best regards,

Laurent Pinchart

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Scott Wood]

Laurent Pinchart wrote:
> On Thursday 20 November 2008, Scott Wood wrote:
>> This makes things worse on mpc8272ads -- the result of the division is
>> 13.56, but I only get error-free serial output when rounding down.  I
>> don't think the remote end has timing problems, as I don't see this with
>> any other board.
> 
> I get exactly the opposite here with an mpc8248 base board. Rounding down 
> makes 115200bds completely unreliable :-/

Hmm...  In that case, I'm OK with the patch as it's technically more 
correct, and clearly what we want if the quotient comes out to something 
like 13.95.  Maybe mpc8272ads board code could apply some sort of tweak 
to the brg frequency.

The right answer is probably that if you want reliable standard baud 
rates, the board designer should ensure a suitable clock is available, 
rather than picking some random clock and hoping it divides nicely.

> Could you measure the serial baudrate with and without the patch using an 
> oscilloscope ?

Not easily; I'm a software person and would have to track down someone 
to lend me one and show me how to use it.  I may try using ntpdate to 
measure timebase drift, as it should be off by the same percentage.

>> Maybe we should change CPM_CLK to be MAIN_CLK/4 rather than MAIN_CLK/16
>> when the BRG clock is below a certain frequency.
> 
> Isn't that the job of the boot loader ?

Sure, I didn't necessarily mean "linux" by "we". :-)

> Accuracy would be improved, but power 
> consumption will raise as well :-/

Granted... though it's actually BRG_CLK and not CPM_CLK that I meant, so 
hopefully it wouldn't make too much of a difference in power.

-Scott

Re: [PATCH] cpm2: Round the baud-rate clock divider to the nearest integer.

[Kumar Gala]

On Nov 20, 2008, at 6:32 AM, Laurent Pinchart wrote:

> Instead of rounding the divider down, improve the baud-rate generators
> accuracy by rounding to the nearest integer.
>
> Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
> ---
> arch/powerpc/sysdev/cpm2.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)

applied to next (for 2.6.30)

- k