Re: [RFC PATCH 0/3] UART slave device bus

[H. Nikolaus Schaller]

Hi,

> Am 19.08.2016 um 09:49 schrieb Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>:
> 
> Hallo Nikolaus,
> 
> do i understand it correctly. This driver is to make kind of interchip
> communication and represent uart as a bus to allow use this bus from
> multiple kernel driver or expose it to user space?

The idea for UART slave devices is to handle devices connected on an
embedded board to an UART port in kernel. Currently most such devices
are just passed through to some /dev/tty and handled by user-space daemons.

So it is not necessarily about multiple kernel drivers to use the same UART, although
that could also be required.

A single one is already difficult... And some scenarios need to shield the UART
from user space (currently there is always one /dev/tty per UART - unless the
UART is completely disabled).

Some ideas where it might be needed:
* bluetooth HCI over UART
* a weird GPS device whose power state can only reliably be detected by monitoring data activity
* other chips (microcontrollers) connected through UART - similar to I2C slave devices
* it potentially could help to better implement IrDA (although that is mostly legacy)

What it is not about are UART/RS232 converters connected through USB or virtual
serial ports created for WWAN modems (e.g. /dev/ttyACM, /dev/ttyHSO). Or BT devices
connected through USB (even if they also run HCI protocol).

> 
> Correct?
> 
> Am 19.08.2016 um 09:29 schrieb H. Nikolaus Schaller:
>> Hi,
>> 
>>> Am 19.08.2016 um 07:21 schrieb Sebastian Reichel <sre@xxxxxxxxxx>:
>>> 
>>> 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@xxxxxxxxxx> 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
>> 
>> 
> 
> 
> --
> Regards,
> Oleksij
>

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail