Message-ID: <efcd1fe0-52f8-ba84-1310-9f4c24ad35bc@rempel-privat.de>
Date:   Fri, 19 Aug 2016 09:49:11 +0200
From:   Oleksij Rempel <linux@...pel-privat.de>
To:     "H. Nikolaus Schaller" <hns@...delico.com>,
        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

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?

Correct?

Am 19.08.2016 um 09:29 schrieb H. Nikolaus Schaller:
> 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
> 
> 


-- 
Regards,
Oleksij



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

Powered by blists - more mailing lists