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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 28 May 2017 03:04:15 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Dong Aisheng <aisheng.dong@....com>
Cc:     "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, fugang.duan@....com,
        Stefan Agner <stefan@...er.ch>, Mingkai.Hu@....com,
        yangbo.lu@....com
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@....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