On Mon, 18 Apr 2022, Andy Shevchenko wrote:
> > 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?
You mean the `->mcr' part? It's required to keep the CLKSEL bit set at
all times.
> Also, looking at the below if the two *_icr_*() functions were moved
> from 8250_port, how they have been used before? Dead code?
They continue being used throughout 8250_port.c: `serial_icr_read' in
`autoconfig_has_efr' and `serial_icr_write' in `serial8250_stop_tx',
`__start_tx', and `serial8250_do_startup'. If they were dead, GCC would
complain about their presence (without `__maybe_unused' annotation or an
equivalent syntax to get the `unused' function atttribute).
Now `serial_icr_write' is also used by `pci_oxsemi_tornado_set_divisor',
and for consistency I chose to export `serial_icr_read' as well.
Let me know if these explanations have cleared your concerns.
> > + /* 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.
As previously discussed I maintain this is a reasonable compromise until
we have issues with BOTHER and other regular termios interfaces such as
B200 fixed; specifically the 16-bit UART_DIV_MAX limitation making it
impossible to request baud rates below 300bps, as also noted in the change
description, and the inability to set exact clock rates which may be
required in some applications relying on the presence of the SPD_CUST
feature.
NB I agree that wiring SPD_CUST to the 38400bps baud rate hasn't been
particularly fortunate, but that's what used to be available in terms of
the API back in 1990s, as I previously explained.
I have now posted v5 with the grammatical issues fixed and an additional
update to make use of the DIV_ROUND_CLOSEST macro I have come across while
looking into an unrelated issue.
Thank you for your review.
Maciej
