Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652011 > unrolled thread

Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate  calculation method Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-28 02:10 +0200
    RE: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate  calculation method "A.S. Dong" <aisheng.dong@nxp.com> - 2017-05-31 16:20 +0200
      Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate  calculation method Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-02 19:20 +0200

#1652011 — Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> On new LPUART versions, the oversampling ratio for the receiver can be
> changed from 4x (00011) to 32x (11111) which could help us get a more
> accurate baud rate divider.
>
> The idea is to use the best OSR (over-sampling rate) possible.
> Note, OSR is typically hard-set to 16 in other LPUART instantiations.
> Loop to find the best OSR value possible, one that generates minimum
> baud diff iterate through the rest of the supported values of OSR.

> +lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int baudrate)
> +{
> +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
> +       u32 clk = sport->port.uartclk;
> +
> +       /*
> +        * The idea is to use the best OSR (over-sampling rate) possible.
> +        * Note, OSR is typically hard-set to 16 in other LPUART instantiations.
> +        * Loop to find the best OSR value possible, one that generates minimum
> +        * baud_diff iterate through the rest of the supported values of OSR.
> +        *
> +        * Calculation Formula:
> +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
> +        */
> +       baud_diff = baudrate;
> +       osr = 0;
> +       sbr = 0;
> +

> +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {

I _think_ you may simplify this and avoid for-loop if you reconsider approach.

> +               /* calculate the temporary sbr value  */
> +               tmp_sbr = (clk / (baudrate * tmp_osr));
> +               if (tmp_sbr == 0)
> +                       tmp_sbr = 1;
> +
> +               /*
> +                * calculate the baud rate difference based on the temporary
> +                * osr and sbr values
> +                */

> +               tmp_diff = clk / (tmp_osr * tmp_sbr) - baudrate;

(32 - 4 + 1) times division is called...

> +
> +               /* select best values between sbr and sbr+1 */
> +               tmp = clk / (tmp_osr * (tmp_sbr + 1));
> +               if (tmp_diff > (baudrate - tmp)) {
> +                       tmp_diff = baudrate - tmp;
> +                       tmp_sbr++;
> +               }
> +
> +               if (tmp_diff <= baud_diff) {
> +                       baud_diff = tmp_diff;
> +                       osr = tmp_osr;
> +                       sbr = tmp_sbr;
> +               }
> +       }

> +       /* handle buadrate outside acceptable rate */
> +       if (baud_diff > ((baudrate / 100) * 3))
> +               dev_warn(sport->port.dev,
> +                        "unacceptable baud rate difference of more than 3%%\n");

Shouldn't you fall back to previous setting?

> +
> +       tmp = lpuart32_read(sport->port.membase + UARTBAUD);
> +

> +       if ((osr > 3) && (osr < 8))

Isn't it

if (osr ^ BIT(2) < BIT(2))

?

> +               tmp |= UARTBAUD_BOTHEDGE;

> +}

> +       if (of_device_is_compatible(port->dev->of_node, "fsl,imx7ulp-lpuart")) {
> +               lpuart32_serial_setbrg(sport, baud);

> +       } else {
> +               sbr = sport->port.uartclk / (16 * baud);
> +               bd &= ~UARTBAUD_SBR_MASK;
> +               bd |= sbr & UARTBAUD_SBR_MASK;
> +               bd |= UARTBAUD_BOTHEDGE;
> +               bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
> +               lpuart32_write(bd, sport->port.membase + UARTBAUD);
> +       }

Perhaps it makes sense to split this to a helper function as well (in
a separate patch).

-- 
With Best Regards,
Andy Shevchenko

[toc] | [next] | [standalone]

#1654305

Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Sunday, May 28, 2017 8:04 AM
> To: A.S. Dong
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm
> Mailing List; Greg Kroah-Hartman; Jiri Slaby; Andy Duan; Stefan Agner;
> Mingkai Hu; Y.B. Lu
> Subject: Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud
> rate calculation method
> 
> On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > On new LPUART versions, the oversampling ratio for the receiver can be
> > changed from 4x (00011) to 32x (11111) which could help us get a more
> > accurate baud rate divider.
> >
> > The idea is to use the best OSR (over-sampling rate) possible.
> > Note, OSR is typically hard-set to 16 in other LPUART instantiations.
> > Loop to find the best OSR value possible, one that generates minimum
> > baud diff iterate through the rest of the supported values of OSR.
> 
> > +lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int
> > +baudrate) {
> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
> > +       u32 clk = sport->port.uartclk;
> > +
> > +       /*
> > +        * The idea is to use the best OSR (over-sampling rate) possible.
> > +        * Note, OSR is typically hard-set to 16 in other LPUART
> instantiations.
> > +        * Loop to find the best OSR value possible, one that generates
> minimum
> > +        * baud_diff iterate through the rest of the supported values of
> OSR.
> > +        *
> > +        * Calculation Formula:
> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
> > +        */
> > +       baud_diff = baudrate;
> > +       osr = 0;
> > +       sbr = 0;
> > +
> 
> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {
> 
> I _think_ you may simplify this and avoid for-loop if you reconsider
> approach.
> 

The algorithm is that we have to iterate all possible OSCs and find
the one with minimum baud_diff.

I'm not sure what alternative approach did you mean?

But there is indeed a optimization way, see below.

> > +               /* calculate the temporary sbr value  */
> > +               tmp_sbr = (clk / (baudrate * tmp_osr));
> > +               if (tmp_sbr == 0)
> > +                       tmp_sbr = 1;
> > +
> > +               /*
> > +                * calculate the baud rate difference based on the
> temporary
> > +                * osr and sbr values
> > +                */
> 
> > +               tmp_diff = clk / (tmp_osr * tmp_sbr) - baudrate;
> 
> (32 - 4 + 1) times division is called...
> 

Yes.

> > +
> > +               /* select best values between sbr and sbr+1 */
> > +               tmp = clk / (tmp_osr * (tmp_sbr + 1));
> > +               if (tmp_diff > (baudrate - tmp)) {
> > +                       tmp_diff = baudrate - tmp;
> > +                       tmp_sbr++;
> > +               }
> > +
> > +               if (tmp_diff <= baud_diff) {
> > +                       baud_diff = tmp_diff;
> > +                       osr = tmp_osr;
> > +                       sbr = tmp_sbr;

To optimize the looping, we probably could do:
If (!daud_diff)
	Break;

> > +               }
> > +       }
> 
> > +       /* handle buadrate outside acceptable rate */
> > +       if (baud_diff > ((baudrate / 100) * 3))
> > +               dev_warn(sport->port.dev,
> > +                        "unacceptable baud rate difference of more
> > + than 3%%\n");
> 
> Shouldn't you fall back to previous setting?
> 

Hmmm.. Is there defined semantic to do that or is there any other ones
doing that way?

I see most drivers not doing that.

> > +
> > +       tmp = lpuart32_read(sport->port.membase + UARTBAUD);
> > +
> 
> > +       if ((osr > 3) && (osr < 8))
> 
> Isn't it
> 
> if (osr ^ BIT(2) < BIT(2))
> 
> ?
> 

That is obvious hard to understand and I'd rather keep a more explicit way.

> > +               tmp |= UARTBAUD_BOTHEDGE;
> 
> > +}
> 
> > +       if (of_device_is_compatible(port->dev->of_node, "fsl,imx7ulp-
> lpuart")) {
> > +               lpuart32_serial_setbrg(sport, baud);
> 
> > +       } else {
> > +               sbr = sport->port.uartclk / (16 * baud);
> > +               bd &= ~UARTBAUD_SBR_MASK;
> > +               bd |= sbr & UARTBAUD_SBR_MASK;
> > +               bd |= UARTBAUD_BOTHEDGE;
> > +               bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
> > +               lpuart32_write(bd, sport->port.membase + UARTBAUD);
> > +       }
> 
> Perhaps it makes sense to split this to a helper function as well (in a
> separate patch).
> 

That will be removed according to Stefan's suggestion to get LS platforms
Start to test.

Regards
Dong Aisheng

[toc] | [prev] | [next] | [standalone]

#1656400

On Wed, May 31, 2017 at 5:18 PM, A.S. Dong <aisheng.dong@nxp.com> wrote:
>> On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:

By some reason my previous message went privately.
It didn't have anything major anyway and here I'm suggesting
optimization of finding factors of the formula in use. See below.

>> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
>> > +       u32 clk = sport->port.uartclk;
>> > +
>> > +       /*
>> > +        * The idea is to use the best OSR (over-sampling rate) possible.
>> > +        * Note, OSR is typically hard-set to 16 in other LPUART
>> instantiations.
>> > +        * Loop to find the best OSR value possible, one that generates
>> minimum
>> > +        * baud_diff iterate through the rest of the supported values of
>> OSR.
>> > +        *
>> > +        * Calculation Formula:
>> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
>> > +        */
>> > +       baud_diff = baudrate;
>> > +       osr = 0;
>> > +       sbr = 0;
>> > +
>>
>> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {

I missed one thing, what happened by default to OSR? What is the value in use?

>> I _think_ you may simplify this and avoid for-loop if you reconsider
>> approach.

> But there is indeed a optimization way, see below.

> To optimize the looping, we probably could do:
> If (!baud_diff)
>         Break;

It's a small one, we may have more interesting approach.

So, the algo is the following:

Assume the ranges like this:
OSR = [4 ... 32]
SBR = [2 ... 8192]

Then:

1. Get ratio factor as
      ratio = CLK / desired baud rate
2. If ratio < 8192 * 9 / 2, just use (ratio / 4, 4) as (OSR, SBR)
setting. (Needs clarification on OSR < 4)
3. if ratio >= 8192 * 31, just use those two numbers (8192, 31). You
can't do anything better there.
4. Otherwise, get a minimum required factor of OSR
      osr_min = ratio / 8192
5. Start your loop from osr_min + 1 to 31.

6 (optional). Of course you may not consider baud_diff > osr_min, it's
I suppose obvious

P.S. Note, all divisions by 2^n are just simple right shifts. Diffs
are calculated as multiplication of OSR and SBR in comparison to
ratio. One division so far.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web