Hi,
A recent change has added a SERIAL_8250_16550A_VARIANTS option, which
lets one request the 8250 driver not to probe for 16550A device features
so as to reduce the driver's device startup time in virtual machines.
This has turned out problematic to a more recent update for the OxSemi
Tornado series PCIe devices, whose new baud rate generator handling code
actually requires switching hardware into the enhanced mode for correct
operation, which actually requires 16550A device features to have been
probed for.
This small patch series fixes the issue by letting individual device
subdrivers to request full 16550A device feature probing by means of a
flag regardless of the SERIAL_8250_16550A_VARIANTS setting chosen.
The changes have been verified with an OXPCIe952 device, in the native
UART mode and a 64-bit RISC-V system as well as in the legacy UART mode
and a 32-bit x86 system.
Credit to Anders for reporting this issue and then working through the
resolution.
Maciej
Oxford Semiconductor PCIe (Tornado) 950 serial port devices need to
operate in the enhanced mode via the EFR register for the Divide-by-M
N/8 baud rate generator prescaler to be used in their native UART mode.
Otherwise the prescaler is fixed at 1 causing grossly incorrect baud
rates to be programmed.
Accessing the EFR register requires 16550A features to have been probed
for, so request this to happen regardless of SERIAL_8250_16550A_VARIANTS
by setting UPF_FULL_PROBE in port flags.
Signed-off-by: Maciej W. Rozycki <[email protected]>
Reported-by: Anders Blomdell <[email protected]>
Fixes: 366f6c955d4d ("serial: 8250: Add proper clock handling for OxSemi PCIe devices")
Cc: [email protected] # v5.19+
---
drivers/tty/serial/8250/8250_pci.c | 5 +++++
1 file changed, 5 insertions(+)
linux-serial-8250-oxsemi-efr.diff
Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
+++ linux-macro/drivers/tty/serial/8250/8250_pci.c
@@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
serial8250_do_set_mctrl(port, mctrl);
}
+/*
+ * We require EFR features for clock programming, so set UPF_FULL_PROBE
+ * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS setting.
+ */
static int pci_oxsemi_tornado_setup(struct serial_private *priv,
const struct pciserial_board *board,
struct uart_8250_port *up, int idx)
@@ -1239,6 +1243,7 @@ static int pci_oxsemi_tornado_setup(stru
struct pci_dev *dev = priv->dev;
if (pci_oxsemi_tornado_p(dev)) {
+ up->port.flags |= UPF_FULL_PROBE;
up->port.get_divisor = pci_oxsemi_tornado_get_divisor;
up->port.set_divisor = pci_oxsemi_tornado_set_divisor;
up->port.set_mctrl = pci_oxsemi_tornado_set_mctrl;
On September 17, 2022 11:07:18 AM GMT+01:00, "Maciej W. Rozycki" <[email protected]> wrote:
>Hi,
>
> A recent change has added a SERIAL_8250_16550A_VARIANTS option, which
>lets one request the 8250 driver not to probe for 16550A device features
>so as to reduce the driver's device startup time in virtual machines.
>This has turned out problematic to a more recent update for the OxSemi
>Tornado series PCIe devices, whose new baud rate generator handling code
>actually requires switching hardware into the enhanced mode for correct
>operation, which actually requires 16550A device features to have been
>probed for.
>
> This small patch series fixes the issue by letting individual device
>subdrivers to request full 16550A device feature probing by means of a
>flag regardless of the SERIAL_8250_16550A_VARIANTS setting chosen.
>
> The changes have been verified with an OXPCIe952 device, in the native
>UART mode and a 64-bit RISC-V system as well as in the legacy UART mode
>and a 32-bit x86 system.
Seems reasonable to me, as long as the flag is only set by drivers that know they've found their hardware.
On Sat, 17 Sep 2022, Josh Triplett wrote:
> > This small patch series fixes the issue by letting individual device
> >subdrivers to request full 16550A device feature probing by means of a
> >flag regardless of the SERIAL_8250_16550A_VARIANTS setting chosen.
> >
> > The changes have been verified with an OXPCIe952 device, in the native
> >UART mode and a 64-bit RISC-V system as well as in the legacy UART mode
> >and a 32-bit x86 system.
>
> Seems reasonable to me, as long as the flag is only set by drivers that
> know they've found their hardware.
That has been my intent or otherwise the change would make no sense as
far as I am concerned.
In principle for most if not all PCI/e devices we could suppress UART
probing altogether and still support device's features as we could infer
the features from the vendor:device ID pair via a table of per-device
flags. This might even have worked if we started making one right from
the beginning as individual devices were added to our 8250/PCI driver.
Though I can imagine that for some devices no documentation was available
to the contributor and it could have been hard to determine whether a
feature actually discovered is really guaranteed for a given vendor:device
ID or whether there are additional constraints, such as a device revision.
I imagine especially early PCI serial port devices may have used discrete
UART chips behind a piece of PCI glue (just as we now see numerous PCIe
devices with the actual device placed behind a PCIe-to-PCI bridge onboard)
and then the set of features could have depended on the specific UART
chips chosen which may have changed in the course of the life of product.
At this point however I suspect it would be hard to (re)construct such a
table and in any case it could have been a maintenance burden, so I guess
we need to live with what we have.
Thank you for your input.
Maciej
On 17. 09. 22, 12:07, Maciej W. Rozycki wrote:
> Oxford Semiconductor PCIe (Tornado) 950 serial port devices need to
> operate in the enhanced mode via the EFR register for the Divide-by-M
> N/8 baud rate generator prescaler to be used in their native UART mode.
> Otherwise the prescaler is fixed at 1 causing grossly incorrect baud
> rates to be programmed.
>
> Accessing the EFR register requires 16550A features to have been probed
> for, so request this to happen regardless of SERIAL_8250_16550A_VARIANTS
> by setting UPF_FULL_PROBE in port flags.
>
> Signed-off-by: Maciej W. Rozycki <[email protected]>
> Reported-by: Anders Blomdell <[email protected]>
> Fixes: 366f6c955d4d ("serial: 8250: Add proper clock handling for OxSemi PCIe devices")
> Cc: [email protected] # v5.19+
> ---
> drivers/tty/serial/8250/8250_pci.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> linux-serial-8250-oxsemi-efr.diff
> Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> ===================================================================
> --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> @@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
> serial8250_do_set_mctrl(port, mctrl);
> }
>
> +/*
> + * We require EFR features for clock programming, so set UPF_FULL_PROBE
> + * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS setting.
> + */
It'd make more sense to me to move this comment right before the line
you add below.
> static int pci_oxsemi_tornado_setup(struct serial_private *priv,
> const struct pciserial_board *board,
> struct uart_8250_port *up, int idx)
> @@ -1239,6 +1243,7 @@ static int pci_oxsemi_tornado_setup(stru
> struct pci_dev *dev = priv->dev;
>
> if (pci_oxsemi_tornado_p(dev)) {
> + up->port.flags |= UPF_FULL_PROBE;
> up->port.get_divisor = pci_oxsemi_tornado_get_divisor;
> up->port.set_divisor = pci_oxsemi_tornado_set_divisor;
> up->port.set_mctrl = pci_oxsemi_tornado_set_mctrl;
thanks,
--
js
suse labs
On Mon, 19 Sep 2022, Jiri Slaby wrote:
> > linux-serial-8250-oxsemi-efr.diff
> > Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> > ===================================================================
> > --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> > +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> > @@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
> > serial8250_do_set_mctrl(port, mctrl);
> > }
> > +/*
> > + * We require EFR features for clock programming, so set UPF_FULL_PROBE
> > + * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS
> > setting.
> > + */
>
> It'd make more sense to me to move this comment right before the line you add
> below.
I favour the style where what a function does is documented above it, but
I won't insist on it if having a comment within is what we prefer here.
Maciej
On Mon, 19 Sep 2022, Maciej W. Rozycki wrote:
> > > linux-serial-8250-oxsemi-efr.diff
> > > Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> > > ===================================================================
> > > --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> > > +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> > > @@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
> > > serial8250_do_set_mctrl(port, mctrl);
> > > }
> > > +/*
> > > + * We require EFR features for clock programming, so set UPF_FULL_PROBE
> > > + * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS
> > > setting.
> > > + */
> >
> > It'd make more sense to me to move this comment right before the line you add
> > below.
>
> I favour the style where what a function does is documented above it, but
> I won't insist on it if having a comment within is what we prefer here.
Having looked at it again I changed my mind and decided it'll be more
consistent with the rest of the code if this comment remains above the
function after all.
My rationale is it is the only function for OxSemi Tornado devices still
without an introductory comment, the other functions have their internals
documented solely within their leading comments, and last but not least it
is obvious what the comment refers to, especially as the function is so
small (as to fit even in an 80x24 character glass TTY device).
I have posted v2 with your other suggestions applied. Thank you for your
review.
Maciej