Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <alpine.DEB.2.21.2204161848030.9383@angie.orcam.me.uk> <alpine.DEB.2.21.2204162141010.9383@angie.orcam.me.uk>
In-Reply-To: <alpine.DEB.2.21.2204162141010.9383@angie.orcam.me.uk>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 18 Apr 2022 15:27:51 +0300
Message-ID: <CAHp75Vc6n+86jhhK2JRSZ=f6ZBV46Kz=Ayq=dWTHFbB-Hr+C7A@mail.gmail.com>
Subject: Re: [PATCH v4 5/5] serial: 8250: Add proper clock handling for OxSemi
 PCIe devices
To:     "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Apr 18, 2022 at 2:02 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> Oxford Semiconductor PCIe (Tornado) 950 serial port devices are driven
> by a fixed 62.5MHz clock input derived from the 100MHz PCI Express clock.
>
> We currently drive the device using its default oversampling rate of 16
> and the clock prescaler disabled, consequently yielding the baud base of
> 3906250.  This base is inadequate for some of the high-speed baud rates
> such as 460800bps, for which the closest rate possible can be obtained
> by dividing the baud base by 8, yielding the baud rate of 488281.25bps,
> which is off by 5.9638%.  This is enough for data communication to break
> with the remote end talking actual 460800bps where missed stop bits have

', where'

> been observed.
>
> We can do better however, by taking advantage of a reduced oversampling
> rate, which can be set to any integer value from 4 to 16 inclusive by
> programming the TCR register, and by using the clock prescaler, which
> can be set to any value from 1 to 63.875 in increments of 0.125 in the
> CPR/CPR2 register pair.  The prescaler has to be explicitly enabled
> though by setting bit 7 in the MCR or otherwise it is bypassed (in the
> enhanced mode that we enable) as if the value of 1 was used.
>
> Make use of these features then as follows:
>
> - Set the baud base to 15625000, reflecting the minimum oversampling
>   rate of 4 with the clock prescaler and divisor both set to 1.
>
> - Override the `set_mctrl' and set the MCR shadow there so as to have
>   MCR[7] always set and have the 8250 core propagate this settings.

these

> - Override the `get_divisor' handler and determine a good combination of
>   parameters by using a lookup table with predetermined value pairs of
>   the oversampling rate and the clock prescaler and finding a pair that
>   divides the input clock such that the quotient, when rounded to the
>   nearest integer, deviates the least from the exact result.  Calculate
>   the clock divisor accordingly.
>
>   Scale the resulting oversampling rate (only by powers of two) if
>   possible so as to maximise it, reducing the divisor accordingly, and
>   avoid a divisor overflow for very low baud rates by scaling the
>   oversampling rate and/or the prescaler even if that causes some
>   accuracy loss.
>
>   Also handle the historic spd_cust feature so as to allow one to set
>   all the three parameters manually to arbitrary values, by keeping the
>   low 16 bits for the divisor and then putting TCR in bits 19:16 and
>   CPR/CPR2 in bits 28:20, sanitising the bit pattern supplied such as
>   to clamp CPR/CPR2 values between 0.000 and 0.875 inclusive to 33.875.
>   This preserves compatibility with any existing setups, that is where
>   requesting a custom divisor that only has any bits set among the low
>   16 the oversampling rate of 16 and the clock prescaler of 33.875 will
>   be used as with the original 8250.
>
>   Finally abuse the `frac' argument to store the determined bit patterns
>   for the TCR, CPR and CPR2 registers.
>
> - Override the `set_divisor' handler so as to set the TCR, CPR and CPR2
>   registers from the `frac' value supplied.  Set the divisor as usually.

usual

> With the baud base set to 15625000 and the unsigned 16-bit UART_DIV_MAX
> limitation imposed by `serial8250_get_baud_rate' standard baud rates
> below 300bps become unavailable in the regular way, e.g. the rate of
> 200bps requires the baud base to be divided by 78125 and that is beyond
> the unsigned 16-bit range.  The historic spd_cust feature can still be
> used to obtain such rates if so required.
>
> See Documentation/tty/device_drivers/oxsemi-tornado.rst for more details.

I'm not sure I understand how this change can have the 8250_port
changes which were done in the previous patches. What did I miss?
Also, looking at the below if the two *_icr_*() functions were moved
from 8250_port, how they have been used before? Dead code?

...

> +       /* Old custom speed handling.  */
> +       if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {

This part is not needed.  We have a BOTHER mechanism in the kernel
that works for each driver that supports it in the generic way, hence
the user space tool wouldn't be patched to support this exact card
separately. Using SPD_CUST is a step back.

> +               unsigned int cust_div = port->custom_divisor;
> +
> +               quot = cust_div & UART_DIV_MAX;
> +               tcr = (cust_div >> 16) & OXSEMI_TORNADO_TCR_MASK;
> +               cpr = (cust_div >> 20) & OXSEMI_TORNADO_CPR_MASK;
> +               if (cpr < OXSEMI_TORNADO_CPR_MIN)
> +                       cpr = OXSEMI_TORNADO_CPR_DEF;
> +       } else {

> +       }

-- 
With Best Regards,
Andy Shevchenko