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.2202100424280.34636@angie.orcam.me.uk> <alpine.DEB.2.21.2202111127200.34636@angie.orcam.me.uk>
In-Reply-To: <alpine.DEB.2.21.2202111127200.34636@angie.orcam.me.uk>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 12 Feb 2022 16:36:15 +0200
Message-ID: <CAHp75VdxUv-807W9OajsM95OVpbwtZ4ts-uJKt+X9e4fXHHuyg@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] 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>,
        Mike Skoog <mskoog@endruntechnologies.com>,
        Mike Korreng <mkorreng@endruntechnologies.com>,
        info@endruntechnologies.com,
        "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 Sat, Feb 12, 2022 at 10:41 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
> 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; also
>   make the console restorer use this shadow.
>
> - 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.
>
> 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.

...

> +       /* Old custom speed handling.  */

Exactly and we do not want to see this in the new code.

> +       if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
> +               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 {

I'll read and comment on the rest later on (hopefully next week).

P.S. I still think that overloading 8250_pci with custom quirks is not
a good idea.

-- 
With Best Regards,
Andy Shevchenko