Patchwork cpm2: Round the baud-rate clock divider to the nearest integer.

login
register
mail settings
Submitter Laurent Pinchart
Date Nov. 20, 2008, 12:32 p.m.
Message ID <200811201332.24061.laurentp@cse-semaphore.com>
Download mbox | patch
Permalink /patch/9741/
State Accepted
Commit 7b8909940a524d67b4352c29256ada476f50fbba
Delegated to: Kumar Gala
Headers show

Comments

Laurent Pinchart - Nov. 20, 2008, 12:32 p.m.
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(-)
Kumar Gala - Nov. 20, 2008, 3:32 p.m.
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
Laurent Pinchart - Nov. 20, 2008, 3:57 p.m.
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).
Kumar Gala - Nov. 20, 2008, 4 p.m.
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
Laurent Pinchart - Nov. 20, 2008, 4:42 p.m.
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%
Scott Wood - Nov. 20, 2008, 7:53 p.m.
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
Laurent Pinchart - Nov. 21, 2008, 4:04 p.m.
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
Scott Wood - Nov. 21, 2008, 5 p.m.
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
Laurent Pinchart - Jan. 15, 2009, 1:18 p.m.
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,
Kumar Gala - Jan. 15, 2009, 2:42 p.m.
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
Laurent Pinchart - Jan. 15, 2009, 2:58 p.m.
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,
Scott Wood - Jan. 15, 2009, 5:54 p.m.
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
Kumar Gala - Jan. 15, 2009, 6:57 p.m.
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
Laurent Pinchart - Jan. 16, 2009, 9:50 a.m.
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 ;-)
Kumar Gala - Jan. 27, 2009, 5:20 p.m.
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

Patch

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;