| Submitter | Laurent Pinchart |
|---|---|
| Date | 2008-11-20 12:32:23 |
| Message ID | <200811201332.24061.laurentp@cse-semaphore.com> |
| Download | mbox | patch |
| Permalink | /patch/9741/ |
| State | New |
| Delegated to: | Kumar Gala |
| Headers | show |
Comments
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
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).
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
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%
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
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
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
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;
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(-)