Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 2/2] serial: 8250: Add new 8250-core based Broadcom STB
 driver
To:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Al Cooper <alcooperx@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        devicetree <devicetree@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        USB <linux-usb@vger.kernel.org>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Rob Herring <robh+dt@kernel.org>
References: <20210115211543.33563-1-alcooperx@gmail.com>
 <20210115211543.33563-3-alcooperx@gmail.com>
 <CAHp75VdQPQK8jTF3QDKx6mF1QzOg-qiuHrTiojnWn7GskokfoA@mail.gmail.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <71d58a3e-2707-69d7-8074-c67235912e06@gmail.com>
Date:   Tue, 19 Jan 2021 10:16:16 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <CAHp75VdQPQK8jTF3QDKx6mF1QzOg-qiuHrTiojnWn7GskokfoA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.


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