Message-Id: <ACEF800E-8DB3-4345-ADE2-15C99F3659D5@goldelico.com>
Date:   Fri, 19 Aug 2016 09:29:38 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        Jiri Slaby <jslaby@...e.com>, Pavel Machek <pavel@....cz>,
        Peter Hurley <peter@...leysoftware.com>,
        NeilBrown <neil@...wn.name>, Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 0/3] UART slave device bus

Hi,

> Am 19.08.2016 um 07:21 schrieb Sebastian Reichel <sre@...nel.org>:
> 
> Hi,
> 
> On Thu, Aug 18, 2016 at 06:08:24PM -0500, Rob Herring wrote:
>> On Thu, Aug 18, 2016 at 3:29 PM, Sebastian Reichel <sre@...nel.org> wrote:
>>> Thanks for going forward and implementing this. I also started,
>>> but was far from a functional state.
>>> 
>>> On Wed, Aug 17, 2016 at 08:14:42PM -0500, Rob Herring wrote:
>>>> Currently, devices attached via a UART are not well supported in
>>>> the kernel. The problem is the device support is done in tty line
>>>> disciplines, various platform drivers to handle some sideband, and
>>>> in userspace with utilities such as hciattach.
>>>> 
>>>> There have been several attempts to improve support, but they suffer from
>>>> still being tied into the tty layer and/or abusing the platform bus. This
>>>> is a prototype to show creating a proper UART bus for UART devices. It is
>>>> tied into the serial core (really struct uart_port) below the tty layer
>>>> in order to use existing serial drivers.
>>>> 
>>>> This is functional with minimal testing using the loopback driver and
>>>> pl011 (w/o DMA) UART under QEMU (modified to add a DT node for the slave
>>>> device). It still needs lots of work and polish.
>>>> 
>>>> TODOs:
>>>> - Figure out the port locking. mutex plus spinlock plus refcounting? I'm
>>>>  hoping all that complexity is from the tty layer and not needed here.
>>>> - Split out the controller for uart_ports into separate driver. Do we see
>>>>  a need for controller drivers that are not standard serial drivers?
>>>> - Implement/test the removal paths
>>>> - Fix the receive callbacks for more than character at a time (i.e. DMA)
>>>> - Need better receive buffering than just a simple circular buffer or
>>>>  perhaps a different receive interface (e.g. direct to client buffer)?
>>>> - Test with other UART drivers
>>>> - Convert a real driver/line discipline over to UART bus.
>>>> 
>>>> Before I spend more time on this, I'm looking mainly for feedback on the
>>>> general direction and structure (the interface with the existing serial
>>>> drivers in particular).
>>> 
>>> I had a look at the uart_dev API:
>>> 
>>> int uart_dev_config(struct uart_device *udev, int baud, int parity, int bits, int flow);
>>> int uart_dev_connect(struct uart_device *udev);
>>> 
>>>  The flow control configuration should be done separately. e.g.:
>>>  uart_dev_flow_control(struct uart_device *udev, bool enable);
>> 
>> No objection, but out of curiosity, why?
> 
> Nokia's bluetooth uart protocol disables flow control during speed
> changes.
> 
>>> int uart_dev_tx(struct uart_device *udev, u8 *buf, size_t count);
>>> int uart_dev_rx(struct uart_device *udev, u8 *buf, size_t count);
>>> 
>>>  UART communication does not have to be host-initiated, so this
>>>  API requires polling. Either some function similar to poll in
>>>  userspace is needed, or it should be implemented as callback.
>> 
>> What's the userspace need?
> 
> I meant "Either some function similar to userspace's poll() is
> needed, ...". Something like uart_dev_wait_for_rx()
> 
> Alternatively the rx function could be a callback, that
> is called when there is new data.
> 
>> I'm assuming the only immediate consumers are in-kernel.
> 
> Yes, but the driver should be notified about incoming data.

Yes, this is very important.

If possible, please do a callback for every character that arrives.
And not only if the rx buffer becomes full, to give the slave driver
a chance to trigger actions almost immediately after every character.
This probably runs in interrupt context and can happen often.

In our proposal some months ago we have implemented such
an rx_notification callback for every character. This allows to work
without rx buffer and implement one in the driver if needed. This
gives the driver full control over the rx buffer dimensions.

And we have made the callback to return a boolean flag which
tells if the character is to be queued in the tty layer so that the
driver can decide for every byte if it is to be hidden from user
space or passed. Since we pass a pointer, the driver could even
modify the character passed back, but we have not used this
feature.

This should cover most (but certainly not all) situations of
implementing protocol engines in uart slave drivers.

Our API therefore was defined as:

void uart_register_slave(struct uart_port *uport, void *slave);
void uart_register_rx_notification(struct uart_port *uport,
		bool (*function)(void *slave, unsigned int *c),
				struct ktermios *termios);

Registering a slave appears to be comparable to uart_dev_connect()
and registering an rx_notification combines uart_dev_config() and
setting the callback.

Unregistration is done by passing a NULL pointer for 'slave' or 'function'.

If there will be a very similar API with a callback like this, we won't have
to change our driver architecture much.

If there is a uart_dev_wait_for_rx() with buffer it is much more difficult
to handle.

BR,
Nikolaus



Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists