On Thu, 15 Sep 2022, Anders Blomdell wrote: > > > 1. Disable new OxSemi Tornado clock code iff !SERIAL_8250_16550A_VARIANTS, > > > bringing back buggy calculation for rates above 115200bps and coarse > > > BOTHER granularity. > > > > > > 2. Same as above, but additionally limit the baud rates to 115200bps to > > > avoid buggy rates. > > > > Maybe this one? That feels odd that we do different things for this old > > config option, that's not good. So making this "just work" should be > > the best idea if at all possible. Though quite an invasive one too IMO to deal with a corner case, plus distributions would likely get it wrong, just as this case indicates, and would ship a crippled (though at least working) driver. > > > 3. Force SERIAL_8250_16550A_VARIANTS to "y" if SERIAL_8250_PCI != "n". > > > > > > 4. Remove SERIAL_8250_16550A_VARIANTS altogether and execute code it > > > guards unconditionally (does it still matter nowadays?). > > > > > > 5. Something else not yet determined. > We could force an EFR probe for this specific driver only. Having investigated the history of SERIAL_8250_16550A_VARIANTS and the possible options I came to a similar conclusion. I wasn't aware the option has about only just been introduced and must have confused it with one of the older ones such as SERIAL_8250_EXTENDED, which has been there since forever. > Pros: driver behaves the same regardless of CONFIG_SERIAL_8250_16550A_VARIANTS > other chips/drivers will not get probed > Cons: autoconfig code will be somewhat bigger since code after the test will > be reachable > change in unrelated part (8250_core.c) to propagate .probe flags But SERIAL_8250_16550A_VARIANTS is quite a recent addition and the motivation wasn't code size, so I wouldn't be concerned here. Another pro is the change is small and sweet, so no large chunks of code to maintain. And it is expandable easily to other subdrivers should they need a similar arrangement. > diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h > index ff84a3ed10ea..0855316468e2 100644 > --- a/include/linux/serial_8250.h > +++ b/include/linux/serial_8250.h > @@ -112,6 +112,7 @@ struct uart_8250_port { > unsigned char probe; > struct mctrl_gpios *gpios; > #define UART_PROBE_RSA (1 << 0) > +#define UART_PROBE_EFR (1 << 1) > /* > * Some bits in registers are cleared on a read, so they must I chose to do it differently though. And I got stumped, because when I got to verifying it I decided to double-check my code with a 32-bit system too, that is my x86 box. And surprisingly existing code worked correctly despite having SERIAL_8250_16550A_VARIANTS unset as if the fix wasn't needed in the first place. After a lot of fiddling I realised there is a feature of the OxSemi ASIC that is not documented in the datasheet, the existence of which however you can read about between the lines: "Extending CPR for Legacy UART "When operating in Legacy UART mode, the system drivers will assume the industry standard 1.8432MHz reference clock. As the Reference clock for the UARTs in the OXPCIe952 is 62.5MHz, all DLL/DLM values will be incorrect if no pre-scaling is done by the UART. In order to correct this effect the CPR register must divide the reference clock by 33.90842 which is approximated to 33.875 by default after a reset. As such the CPR has been extended to support a larger 6 bit integer range by using bit-0 of CPR2 to represent bit-6 of the integer portion of the clock pre-scalar" What it doesn't say is that in the legacy UART mode the Divide-by-M N/8 prescaler is implicitly enabled despite that the MCR[7] bit is forced 0 in the non-enhanced mode. This is opposite to how things work in the native UART mode where the MCR[7] bit is respected regardless. It is only when the enhanced mode is enabled via EFR[4] that the MCR[7] bit is respected both in the legacy and the native UART mode. It makes sense, because legacy 8250 UART drivers will expect to work with `base_baud' set to 115200 while using the MCR the way it has been with the original 16450 chip. Conversely legacy 16450 UART drivers do not support the native UART mode anyway, so it doesn't have to implement this special case. And I have the option card jumpered to work in the legacy UART mode in my x86 system (just so that I can verify both modes of operation at any time): 0000:04:00.0: ttyS2 at I/O 0x1000 (irq = 10, base_baud = 15625000) is a 16C950/954 0000:04:00.1: ttyS3 at I/O 0x1008 (irq = 5, base_baud = 15625000) is a 16C950/954 04:00.0 Serial controller [0700]: Oxford Semiconductor Ltd OXPCIe952 Legacy 950 UART #1 [1415:c144] 04:00.1 Serial controller [0700]: Oxford Semiconductor Ltd OXPCIe952 Legacy 950 UART #2 [1415:c145] vs the native UART mode (here in POWER9): 0001:01:00.0: ttyS0 at MMIO 0x600c080401000 (irq = 40, base_baud = 15625000) is a 16C950/954 0001:01:00.0: ttyS1 at MMIO 0x600c080401200 (irq = 40, base_baud = 15625000) is a 16C950/954 0001:01:00.0 Serial controller [0700]: Oxford Semiconductor Ltd OXPCIe952 Dual Native 950 UART [1415:c158] Oh well! With that phenomenon sorted I'll be posting the final fix shortly, as soon I have made a suitable change description referring the relevant sources. I take it you don't mind being mentioned by means of `Reported-by'? Maciej