lkml.org 
[lkml]   [2021]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 2/2] serial: 8250: Add new 8250-core based Broadcom STB driver
    On Tue, Jan 19, 2021 at 8:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
    > On 1/19/2021 7:21 AM, Andy Shevchenko wrote:
    > > On Fri, Jan 15, 2021 at 11:19 PM Al Cooper <alcooperx@gmail.com> wrote:
    > >>
    > >> 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.
    >
    > > - 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.

    > > - 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

    \
     
     \ /
      Last update: 2021-01-20 17:50    [W:3.789 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site