Re: [PATCH 2/3] console: Add simplified 'serdev' framework from Linux kernel

[Andrey Smirnov]

On Wed, Apr 11, 2018 at 1:39 AM, Sascha Hauer  wrote:
> On Mon, Apr 09, 2018 at 09:40:46AM -0700, Andrey Smirnov wrote:
>> On Mon, Apr 2, 2018 at 11:54 PM, Sascha Hauer  wrote:
>> > Hi Andrey,
>> >
>> > Some comments inside.
>> >
>> >
>> > On Mon, Mar 26, 2018 at 06:09:14AM -0700, Andrey Smirnov wrote:
>> >> Port 'serdev' UART-slave deivce framework found in recent Linux
>> >> kernels (post 4.13) in order to be able to port 'serdev' slave drivers
>> >> from Linux.
>> >>
>> >> Signed-off-by: Andrey Smirnov 
>> >> @@ -323,6 +324,17 @@ int console_register(struct console_device *newcdev)
>> >>   dev->parent = newcdev->dev;
>> >>   platform_device_register(dev);
>> >>
>> >> + newcdev->open_count = 0;
>> >> +
>> >> + /*
>> >> +  * If our console deive is a serdev, we skip the creation of
>> >
>> > s/deive/device/
>>
>> Will fix in v2.
>>
>> >
>> >> +  * corresponding entry in /dev as well as registration in
>> >> +  * console_list and just go straigh to populating child
>> >
>> > s/straigh/straight/
>>
>> Ditto.
>>
>> >
>> >> +  * devices.
>> >> +  */
>> >> + if (serdev_node)
>> >> + return of_platform_populate(serdev_node, NULL, dev);
>> >
>> > How is this going to be used? A serdev driver binds to the serdev_node
>> > and then it probably needs to get a pointer to the console device,
>> > right? How does the driver accomplish this?
>> >
>>
>> Serdev slave driver doesn't hold explicit pointer to console device,
>> instead accessing it via point to serdev_device. The latter could
>> obtained by calling to_serdev_device(dev->parent), where dev is
>> device_d given to slave driver's probe function.
>>
>>
>> >> +/**
>> >> + * struct serdev_device - Basic representation of an serdev device
>> >> + *
>> >> + * @dev: Corresponding device
>> >> + * @fifo:Circular buffer used for console draining
>> >> + * @buf: Buffer used to pass Rx data to consumers
>> >> + * @poller   Async poller used to poll this serdev
>> >> + * @polling_interval:Async poller periodicity
>> >> + * @polling_window:  Duration of a single busy loop poll
>> >> + * @receive_buf: Function called with data received from device;
>> >> + *   returns number of bytes accepted;
>> >> + */
>> >> +struct serdev_device {
>> >> + struct device_d *dev;
>> >> + struct kfifo *fifo;
>> >> + unsigned char *buf;
>> >> + struct poller_async poller;
>> >> + uint64_t polling_interval;
>> >> + uint64_t polling_window;
>> >> +
>> >> + int (*receive_buf)(struct serdev_device *, const unsigned char *,
>> >> +size_t);
>> >> +};
>> >> +
>> >> +int serdev_device_open(struct serdev_device *);
>> >> +unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned 
>> >> int);
>> >> +int serdev_device_write(struct serdev_device *, const unsigned char *,
>> >> + size_t, unsigned long);
>> >
>> > So a serdev driver uses serdev_device_write() to send characters out. To
>> > receive characters it has to implement serdev_device->receive_buf,
>> > right?
>>
>> Right. I tried to implement exactly the same API that Linux's serdev
>> API provides.
>>
>> > What kind of devices did you implement this for?
>>
>> I ported serdev in support of porting the driver for RAVE SP which is
>> a small microcontroller device found many ZII board including RDU2. It
>> implement a whole bunch of various functionality including watchdog,
>> parameter EEPROM, sensor access, backlight control, button input event
>> generation, etc.
>>
>> > For devices which send data without request (GPS?) this seems the way to 
>> > go. For
>> > others a synchronous receive function might be good, no?
>> >
>>
>> I didn't implement anything like that mostly because Linux serdev API
>> doesn't and any ported driver wouldn't have any need for those
>> functions. But in general, I am not sure how useful synchronous
>> receive function would be. In my experience, devices like that usually
>> implement some binary transport protocol with packetization/escape
>> sequences on top of UART, which usually requires a state machine
>> operating with byte granularity as the data comes in to parse
>> responses correctly and synchronous APIs are not extremely useful for
>> that kind of a use-case.
>>
>> FWIW, since serdev API is integrated into poller infrastructure it is
>> pretty trivial to write blocking code with it. Here's how I use it in
>> my driver to implement request-response type of a function:
>>
>> rave_sp_write(sp, data, data_size);
>> /*
>> * is_timeout will implicitly poll serdev via poller
>> * infrastructure
>> */
>> while (!is_timeout(start, SECOND) && !reply.received)
>>;
>
> I understand that synchronous receiving might not be that useful. Given
> how simple it is we could add a synchronous receive wrapper function
> just for the 

Re: [PATCH 2/3] console: Add simplified 'serdev' framework from Linux kernel

[Sascha Hauer]

On Mon, Apr 09, 2018 at 09:40:46AM -0700, Andrey Smirnov wrote:
> On Mon, Apr 2, 2018 at 11:54 PM, Sascha Hauer  wrote:
> > Hi Andrey,
> >
> > Some comments inside.
> >
> >
> > On Mon, Mar 26, 2018 at 06:09:14AM -0700, Andrey Smirnov wrote:
> >> Port 'serdev' UART-slave deivce framework found in recent Linux
> >> kernels (post 4.13) in order to be able to port 'serdev' slave drivers
> >> from Linux.
> >>
> >> Signed-off-by: Andrey Smirnov 
> >> @@ -323,6 +324,17 @@ int console_register(struct console_device *newcdev)
> >>   dev->parent = newcdev->dev;
> >>   platform_device_register(dev);
> >>
> >> + newcdev->open_count = 0;
> >> +
> >> + /*
> >> +  * If our console deive is a serdev, we skip the creation of
> >
> > s/deive/device/
> 
> Will fix in v2.
> 
> >
> >> +  * corresponding entry in /dev as well as registration in
> >> +  * console_list and just go straigh to populating child
> >
> > s/straigh/straight/
> 
> Ditto.
> 
> >
> >> +  * devices.
> >> +  */
> >> + if (serdev_node)
> >> + return of_platform_populate(serdev_node, NULL, dev);
> >
> > How is this going to be used? A serdev driver binds to the serdev_node
> > and then it probably needs to get a pointer to the console device,
> > right? How does the driver accomplish this?
> >
> 
> Serdev slave driver doesn't hold explicit pointer to console device,
> instead accessing it via point to serdev_device. The latter could
> obtained by calling to_serdev_device(dev->parent), where dev is
> device_d given to slave driver's probe function.
> 
> 
> >> +/**
> >> + * struct serdev_device - Basic representation of an serdev device
> >> + *
> >> + * @dev: Corresponding device
> >> + * @fifo:Circular buffer used for console draining
> >> + * @buf: Buffer used to pass Rx data to consumers
> >> + * @poller   Async poller used to poll this serdev
> >> + * @polling_interval:Async poller periodicity
> >> + * @polling_window:  Duration of a single busy loop poll
> >> + * @receive_buf: Function called with data received from device;
> >> + *   returns number of bytes accepted;
> >> + */
> >> +struct serdev_device {
> >> + struct device_d *dev;
> >> + struct kfifo *fifo;
> >> + unsigned char *buf;
> >> + struct poller_async poller;
> >> + uint64_t polling_interval;
> >> + uint64_t polling_window;
> >> +
> >> + int (*receive_buf)(struct serdev_device *, const unsigned char *,
> >> +size_t);
> >> +};
> >> +
> >> +int serdev_device_open(struct serdev_device *);
> >> +unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned 
> >> int);
> >> +int serdev_device_write(struct serdev_device *, const unsigned char *,
> >> + size_t, unsigned long);
> >
> > So a serdev driver uses serdev_device_write() to send characters out. To
> > receive characters it has to implement serdev_device->receive_buf,
> > right?
> 
> Right. I tried to implement exactly the same API that Linux's serdev
> API provides.
> 
> > What kind of devices did you implement this for?
> 
> I ported serdev in support of porting the driver for RAVE SP which is
> a small microcontroller device found many ZII board including RDU2. It
> implement a whole bunch of various functionality including watchdog,
> parameter EEPROM, sensor access, backlight control, button input event
> generation, etc.
> 
> > For devices which send data without request (GPS?) this seems the way to 
> > go. For
> > others a synchronous receive function might be good, no?
> >
> 
> I didn't implement anything like that mostly because Linux serdev API
> doesn't and any ported driver wouldn't have any need for those
> functions. But in general, I am not sure how useful synchronous
> receive function would be. In my experience, devices like that usually
> implement some binary transport protocol with packetization/escape
> sequences on top of UART, which usually requires a state machine
> operating with byte granularity as the data comes in to parse
> responses correctly and synchronous APIs are not extremely useful for
> that kind of a use-case.
> 
> FWIW, since serdev API is integrated into poller infrastructure it is
> pretty trivial to write blocking code with it. Here's how I use it in
> my driver to implement request-response type of a function:
> 
> rave_sp_write(sp, data, data_size);
> /*
> * is_timeout will implicitly poll serdev via poller
> * infrastructure
> */
> while (!is_timeout(start, SECOND) && !reply.received)
>;

I understand that synchronous receiving might not be that useful. Given
how simple it is we could add a synchronous receive wrapper function
just for the sake of completeness, even if it only provides an example
how the code can be used.

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux 

Re: [PATCH 2/3] console: Add simplified 'serdev' framework from Linux kernel

[Andrey Smirnov]

On Mon, Apr 2, 2018 at 11:54 PM, Sascha Hauer  wrote:
> Hi Andrey,
>
> Some comments inside.
>
>
> On Mon, Mar 26, 2018 at 06:09:14AM -0700, Andrey Smirnov wrote:
>> Port 'serdev' UART-slave deivce framework found in recent Linux
>> kernels (post 4.13) in order to be able to port 'serdev' slave drivers
>> from Linux.
>>
>> Signed-off-by: Andrey Smirnov 
>> @@ -323,6 +324,17 @@ int console_register(struct console_device *newcdev)
>>   dev->parent = newcdev->dev;
>>   platform_device_register(dev);
>>
>> + newcdev->open_count = 0;
>> +
>> + /*
>> +  * If our console deive is a serdev, we skip the creation of
>
> s/deive/device/

Will fix in v2.

>
>> +  * corresponding entry in /dev as well as registration in
>> +  * console_list and just go straigh to populating child
>
> s/straigh/straight/

Ditto.

>
>> +  * devices.
>> +  */
>> + if (serdev_node)
>> + return of_platform_populate(serdev_node, NULL, dev);
>
> How is this going to be used? A serdev driver binds to the serdev_node
> and then it probably needs to get a pointer to the console device,
> right? How does the driver accomplish this?
>

Serdev slave driver doesn't hold explicit pointer to console device,
instead accessing it via point to serdev_device. The latter could
obtained by calling to_serdev_device(dev->parent), where dev is
device_d given to slave driver's probe function.


>> +/**
>> + * struct serdev_device - Basic representation of an serdev device
>> + *
>> + * @dev: Corresponding device
>> + * @fifo:Circular buffer used for console draining
>> + * @buf: Buffer used to pass Rx data to consumers
>> + * @poller   Async poller used to poll this serdev
>> + * @polling_interval:Async poller periodicity
>> + * @polling_window:  Duration of a single busy loop poll
>> + * @receive_buf: Function called with data received from device;
>> + *   returns number of bytes accepted;
>> + */
>> +struct serdev_device {
>> + struct device_d *dev;
>> + struct kfifo *fifo;
>> + unsigned char *buf;
>> + struct poller_async poller;
>> + uint64_t polling_interval;
>> + uint64_t polling_window;
>> +
>> + int (*receive_buf)(struct serdev_device *, const unsigned char *,
>> +size_t);
>> +};
>> +
>> +int serdev_device_open(struct serdev_device *);
>> +unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned 
>> int);
>> +int serdev_device_write(struct serdev_device *, const unsigned char *,
>> + size_t, unsigned long);
>
> So a serdev driver uses serdev_device_write() to send characters out. To
> receive characters it has to implement serdev_device->receive_buf,
> right?

Right. I tried to implement exactly the same API that Linux's serdev
API provides.

> What kind of devices did you implement this for?

I ported serdev in support of porting the driver for RAVE SP which is
a small microcontroller device found many ZII board including RDU2. It
implement a whole bunch of various functionality including watchdog,
parameter EEPROM, sensor access, backlight control, button input event
generation, etc.

> For devices which send data without request (GPS?) this seems the way to go. 
> For
> others a synchronous receive function might be good, no?
>

I didn't implement anything like that mostly because Linux serdev API
doesn't and any ported driver wouldn't have any need for those
functions. But in general, I am not sure how useful synchronous
receive function would be. In my experience, devices like that usually
implement some binary transport protocol with packetization/escape
sequences on top of UART, which usually requires a state machine
operating with byte granularity as the data comes in to parse
responses correctly and synchronous APIs are not extremely useful for
that kind of a use-case.

FWIW, since serdev API is integrated into poller infrastructure it is
pretty trivial to write blocking code with it. Here's how I use it in
my driver to implement request-response type of a function:

rave_sp_write(sp, data, data_size);
/*
* is_timeout will implicitly poll serdev via poller
* infrastructure
*/
while (!is_timeout(start, SECOND) && !reply.received)
   ;


Thanks,
Andrey Smirnov

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] console: Add simplified 'serdev' framework from Linux kernel

[Sascha Hauer]

Hi Andrey,

Some comments inside.


On Mon, Mar 26, 2018 at 06:09:14AM -0700, Andrey Smirnov wrote:
> Port 'serdev' UART-slave deivce framework found in recent Linux
> kernels (post 4.13) in order to be able to port 'serdev' slave drivers
> from Linux.
> 
> Signed-off-by: Andrey Smirnov 
> @@ -323,6 +324,17 @@ int console_register(struct console_device *newcdev)
>   dev->parent = newcdev->dev;
>   platform_device_register(dev);
>  
> + newcdev->open_count = 0;
> +
> + /*
> +  * If our console deive is a serdev, we skip the creation of

s/deive/device/

> +  * corresponding entry in /dev as well as registration in
> +  * console_list and just go straigh to populating child

s/straigh/straight/

> +  * devices.
> +  */
> + if (serdev_node)
> + return of_platform_populate(serdev_node, NULL, dev);

How is this going to be used? A serdev driver binds to the serdev_node
and then it probably needs to get a pointer to the console device,
right? How does the driver accomplish this?

> +/**
> + * struct serdev_device - Basic representation of an serdev device
> + *
> + * @dev: Corresponding device
> + * @fifo:Circular buffer used for console draining
> + * @buf: Buffer used to pass Rx data to consumers
> + * @poller   Async poller used to poll this serdev
> + * @polling_interval:Async poller periodicity
> + * @polling_window:  Duration of a single busy loop poll
> + * @receive_buf: Function called with data received from device;
> + *   returns number of bytes accepted;
> + */
> +struct serdev_device {
> + struct device_d *dev;
> + struct kfifo *fifo;
> + unsigned char *buf;
> + struct poller_async poller;
> + uint64_t polling_interval;
> + uint64_t polling_window;
> +
> + int (*receive_buf)(struct serdev_device *, const unsigned char *,
> +size_t);
> +};
> +
> +int serdev_device_open(struct serdev_device *);
> +unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned 
> int);
> +int serdev_device_write(struct serdev_device *, const unsigned char *,
> + size_t, unsigned long);

So a serdev driver uses serdev_device_write() to send characters out. To
receive characters it has to implement serdev_device->receive_buf,
right? What kind of devices did you implement this for? For devices
which send data without request (GPS?) this seems the way to go. For
others a synchronous receive function might be good, no?

Sascha

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] console: Add simplified 'serdev' framework from Linux kernel

[Andrey Smirnov]

Port 'serdev' UART-slave deivce framework found in recent Linux
kernels (post 4.13) in order to be able to port 'serdev' slave drivers
from Linux.

Signed-off-by: Andrey Smirnov 
---
 common/Kconfig|  6 
 common/Makefile   |  1 +
 common/console.c  | 24 +--
 common/serdev.c   | 87 +++
 include/console.h | 27 +
 include/serdev.h  | 36 +++
 6 files changed, 178 insertions(+), 3 deletions(-)
 create mode 100644 common/serdev.c
 create mode 100644 include/serdev.h

diff --git a/common/Kconfig b/common/Kconfig
index 81cf72a95..601bc95a4 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -712,6 +712,12 @@ config CONSOLE_NONE
 
 endchoice
 
+config SERIAL_DEV_BUS
+   bool "Serial device bus"
+   depends on CONSOLE_FULL
+   help
+ Core support for devices connected via a serial port.
+
 choice
prompt "Console activation strategy"
depends on CONSOLE_FULL
diff --git a/common/Makefile b/common/Makefile
index a9abcd1bc..c945dd78e 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_FIRMWARE)+= firmware.o
 obj-$(CONFIG_UBIFORMAT)+= ubiformat.o
 obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
 obj-$(CONFIG_BOOT) += boot.o
+obj-$(CONFIG_SERIAL_DEV_BUS)   += serdev.o
 
 ifdef CONFIG_PASSWORD
 
diff --git a/common/console.c b/common/console.c
index f4c799fa5..13b1360b5 100644
--- a/common/console.c
+++ b/common/console.c
@@ -305,10 +305,11 @@ static ssize_t fops_write(struct cdev* dev, const void* 
buf, size_t count,
 
 int console_register(struct console_device *newcdev)
 {
+   struct device_node *serdev_node = console_is_serdev_node(newcdev);
struct device_d *dev = >class_dev;
int activate = 0, ret;
 
-   if (initialized == CONSOLE_UNINITIALIZED)
+   if (!serdev_node && initialized == CONSOLE_UNINITIALIZED)
console_init_early();
 
if (newcdev->devname) {
@@ -323,6 +324,17 @@ int console_register(struct console_device *newcdev)
dev->parent = newcdev->dev;
platform_device_register(dev);
 
+   newcdev->open_count = 0;
+
+   /*
+* If our console deive is a serdev, we skip the creation of
+* corresponding entry in /dev as well as registration in
+* console_list and just go straigh to populating child
+* devices.
+*/
+   if (serdev_node)
+   return of_platform_populate(serdev_node, NULL, dev);
+
if (newcdev->setbrg) {
ret = newcdev->setbrg(newcdev, CONFIG_BAUDRATE);
if (ret)
@@ -335,8 +347,6 @@ int console_register(struct console_device *newcdev)
if (newcdev->putc && !newcdev->puts)
newcdev->puts = __console_puts;
 
-   newcdev->open_count = 0;
-
dev_add_param_string(dev, "active", console_active_set, 
console_active_get,
 >active_string, newcdev);
 
@@ -386,6 +396,14 @@ int console_unregister(struct console_device *cdev)
struct device_d *dev = >class_dev;
int status;
 
+   /*
+* We don't do any sophisticated serdev device de-population
+* and instead claim this console busy, preventing its
+* de-initialization, 'till the very end of our execution.
+*/
+   if (console_is_serdev_node(cdev))
+   return -EBUSY;
+
devfs_remove(>devfs);
 
list_del(>list);
diff --git a/common/serdev.c b/common/serdev.c
new file mode 100644
index 0..58985c4b1
--- /dev/null
+++ b/common/serdev.c
@@ -0,0 +1,87 @@
+
+#include 
+#include 
+
+static void serdev_device_poller(void *context)
+{
+   struct serdev_device *serdev = context;
+   struct console_device *cdev = to_console_device(serdev);
+   unsigned char *buf = serdev->buf;
+   int ret, len;
+
+   /*
+* Since this callback is a part of poller infrastructure we
+* want to use _non_interruptible version of the function
+* below to prevent recursion from happening (regular
+* console_drain will call is_timeout, which might end up
+* calling this function again).
+*/
+   len = console_drain_non_interruptible(cdev, serdev->fifo, buf,
+ PAGE_SIZE,
+ serdev->polling_window);
+   while (len > 0) {
+   ret = serdev->receive_buf(serdev, buf, len);
+   len -= ret;
+   buf += ret;
+   }
+
+   /*
+* Re-schedule ourselves in 'serdev->polling_interval'
+* nanoseconds
+*/
+   poller_call_async(>poller,
+ serdev->polling_interval,
+ serdev_device_poller,
+ serdev);
+}
+
+int serdev_device_open(struct serdev_device *serdev)
+{