I took another look at using the DMA framework for this and I still
don't think it fits. The problem is that the DMA hardware is so
tightly coupled to the 8250 UART hardware that a generic DMA model
where a DMA engine will read the data from the UART data register when
Data Ready is asserted and transfer it to memory, doesn't fit.  For
example:
- The flush timeout, parity error, framing error and overrun errors
are kept separately in the DMA hardware.
- The DMA hardware expects to be enabled before receiving input data
and it will continually write the data to ping pong data buffers as
long as a buffer empty bit is set on the next buffer.
- When DMA is enabled it bypasses some of the normal 8250 control and
data registers.

I think that trying to force this hardware into the current 8250 DMA
model using the DMA framework would not be a good approach.
I'll look at the other suggestions.

Thanks
Al



On Wed, Jan 20, 2021 at 11:47 AM Andy Shevchenko
[off-list ref] wrote:

On Tue, Jan 19, 2021 at 8:16 PM Florian Fainelli [off-list ref] wrote:

quoted

On 1/19/2021 7:21 AM, Andy Shevchenko wrote:

quoted

On Fri, Jan 15, 2021 at 11:19 PM Al Cooper [off-list ref] wrote:

quoted

Add a UART driver for the new Broadcom 8250 based STB UART. The new
UART is backward compatible with the standard 8250, but has some
additional features. The new features include a high accuracy baud
rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best
one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed
the baud rate selection logic for any requested baud rate.  This allows
for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers
are specified in Device Tree "reg" property. The DMA functionality can
be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

The driver also set the UPSTAT_AUTOCTS flag when hardware flow control
is enabled. This flag is needed for UARTs that don't assert a CTS
changed interrupt when CTS changes and AFE (Hardware Flow Control) is
enabled.

The driver also contains a workaround for a bug in the Synopsis 8250
core. The problem is that at high baud rates, the RX partial FIFO
timeout interrupt can occur but there is no RX data (DR not set in
the LSR register). In this case the driver will not read the Receive
Buffer Register, which clears the interrupt, and the system will get
continuous UART interrupts until the next RX character arrives. The
fix originally suggested by Synopsis was to read the Receive Buffer
Register and discard the character when the DR bit in the LSR was
not set, to clear the interrupt. The problem was that occasionally
a character would arrive just after the DR bit check and a valid
character would be discarded. The fix that was added will clear
receive interrupts to stop the interrupt, deassert RTS to insure
that no new data can arrive, wait for 1.5 character times for the
sender to react to RTS and then check for data and either do a dummy
read or a valid read. Sysfs error counters were also added and were
used to help create test software that would cause the error condition.
The counters can be found at:
/sys/devices/platform/rdb/*serial/rx_bad_timeout_late_char
/sys/devices/platform/rdb/*serial/rx_bad_timeout_no_char

Brief looking into the code raises several questions:
 - is it driver from the last decade?

Work on this driver started back in 2018, that was indeed the last decade.

quoted

 - why it's not using what kernel provides?
 - we have a lot of nice helpers:
   - DMA Engine API

Not sure this makes sense, given that the DMA hardware that was added to
this UART block is only used by the UART block and no other pieces of HW
in the system, nor will they ever be. Not sure it makes sense to pay the
cost of an extra indirection and subsystem unless there are at least two
consumers of that DMA hardware to warrant modeling it after a dmaengine
driver. I also remember that Al researched before whether 8250_dma.c
could work, and came to the conclusion that it would not, but I will let
him comment on the specifics.

I see. In any case I still believe that the driver can be shrinked by
a notable amount of lines.

quoted

quoted

   - BIT() and GENMASK() macros
   - tons of different helpers like regmap API (if you wish to dump
registers via debugfs)

Can you shrink this driver by 20-30% (I truly believe it's possible)
and split DMA driver to drivers/dma (which may already have something
similar there)?

See previous response.

--
With Best Regards,
Andy Shevchenko