Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1552932 > unrolled thread

[PATCH 0/9] Serial slave device bus

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/9] Serial slave device bus Rob Herring <robh@kernel.org> - 2017-01-06 17:30 +0100
    [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding Rob Herring <robh@kernel.org> - 2017-01-06 17:30 +0100
      Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding Arnd Bergmann <arnd@arndb.de> - 2017-01-06 20:30 +0100
        Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding Rob Herring <robh@kernel.org> - 2017-01-06 21:50 +0100
      Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device  binding One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2017-01-10 21:00 +0100
      Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device  binding Pavel Machek <pavel@ucw.cz> - 2017-01-10 22:50 +0100
    [PATCH 3/9] tty_port: make tty_port_register_device wrap tty_port_register_device_attr Rob Herring <robh@kernel.org> - 2017-01-06 17:30 +0100
    [PATCH 5/9] tty_port: Add port client functions Rob Herring <robh@kernel.org> - 2017-01-06 17:30 +0100
    Re: [PATCH 0/9] Serial slave device bus Arnd Bergmann <arnd@arndb.de> - 2017-01-06 20:30 +0100
    Re: [PATCH 0/9] Serial slave device bus Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-07 12:10 +0100
      Re: [PATCH 0/9] Serial slave device bus Rob Herring <robh@kernel.org> - 2017-01-10 18:30 +0100
        Re: [PATCH 0/9] Serial slave device bus Marcel Holtmann <marcel@holtmann.org> - 2017-01-10 19:40 +0100
    Re: [PATCH 0/9] Serial slave device bus Sebastian Reichel <sre@kernel.org> - 2017-01-08 23:50 +0100
    Re: [PATCH 0/9] Serial slave device bus "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-01-10 12:50 +0100
      Re: [PATCH 0/9] Serial slave device bus Marcel Holtmann <marcel@holtmann.org> - 2017-01-10 13:10 +0100
        Re: [PATCH 0/9] Serial slave device bus "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-01-10 13:20 +0100
          Re: [PATCH 0/9] Serial slave device bus Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-10 13:30 +0100
            Re: [PATCH 0/9] Serial slave device bus "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-01-10 13:50 +0100
      Re: [PATCH 0/9] Serial slave device bus Rob Herring <robh@kernel.org> - 2017-01-13 15:50 +0100
        Re: [PATCH 0/9] Serial slave device bus "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-01-16 07:50 +0100
    Re: [PATCH 0/9] Serial slave device bus Marcel Holtmann <marcel@holtmann.org> - 2017-01-10 13:10 +0100
    Re: [PATCH 0/9] Serial slave device bus Pavel Machek <pavel@ucw.cz> - 2017-01-10 23:10 +0100

Page 1 of 2  [1] 2  Next page →

#1552932 — [PATCH 0/9] Serial slave device bus

Here goes another attempt at a serial device bus (aka uart slaves, tty
slaves, etc.).

After some discussions with Dmitry at LPC, I decided to move away from
extending serio and moved back to making a new bus type instead. He didn't
think using serio was a good fit, and serio has a number of peculiarities
in regards to sysfs and it's driver model. I don't think we want to inherit
those for serial slave devices.

This version sits on top of tty_port rather than uart_port as Alan
requested. Once I created a struct tty rather than moving everything
needed to tty_port, it became a lot easier and less invasive to the tty
core code.

I have hacked up versions of the BT ldisc and TI ST drivers moved over to
use the serdev bus. I have BT working on the HiKey board which has TI BT.
With the serdev bus support, it eliminates the need for the TI userspace
UIM daemon.

This series and the mentioned drivers can be found here[1].

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2

Alan Cox (1):
  tty_port: allow a port to be opened with a tty that has no file handle

Rob Herring (8):
  tty: move the non-file related parts of tty_release to new
    tty_release_struct
  tty_port: make tty_port_register_device wrap
    tty_port_register_device_attr
  tty: constify tty_ldisc_receive_buf buffer pointer
  tty_port: Add port client functions
  dt/bindings: Add a serial/UART attached device binding
  serdev: Introduce new bus for serial attached devices
  serdev: add a tty port controller driver
  tty_port: register tty ports with serdev bus

 .../devicetree/bindings/serial/slave-device.txt    |  34 ++
 MAINTAINERS                                        |   8 +
 drivers/char/Kconfig                               |   1 +
 drivers/tty/Makefile                               |   1 +
 drivers/tty/serdev/Kconfig                         |  16 +
 drivers/tty/serdev/Makefile                        |   5 +
 drivers/tty/serdev/core.c                          | 388 +++++++++++++++++++++
 drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
 drivers/tty/tty_buffer.c                           |  19 +-
 drivers/tty/tty_io.c                               |  44 ++-
 drivers/tty/tty_port.c                             |  60 +++-
 include/linux/serdev.h                             | 227 ++++++++++++
 include/linux/tty.h                                |  12 +-
 13 files changed, 1017 insertions(+), 42 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
 create mode 100644 drivers/tty/serdev/Kconfig
 create mode 100644 drivers/tty/serdev/Makefile
 create mode 100644 drivers/tty/serdev/core.c
 create mode 100644 drivers/tty/serdev/serdev-ttyport.c
 create mode 100644 include/linux/serdev.h

--
2.10.1

[toc] | [next] | [standalone]

#1552934 — [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

Add a common binding for describing serial/UART attached devices. Common
examples are Bluetooth, WiFi, NFC and GPS devices.

Serial attached devices are represented as child nodes of a UART node.
This may need to be extended for more complex devices with multiple
interfaces, but for the simple cases a child node is sufficient.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/serial/slave-device.txt    | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt

diff --git a/Documentation/devicetree/bindings/serial/slave-device.txt b/Documentation/devicetree/bindings/serial/slave-device.txt
new file mode 100644
index 000000000000..9b7c2d651345
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/slave-device.txt
@@ -0,0 +1,34 @@
+Serial Slave Device DT binding
+
+This documents the binding structure and common properties for serial
+attached devices. Common examples include Bluetooth, WiFi, NFC and GPS
+devices.
+
+qSerial attached devices shall be a child node of the host UART device the
+slave device is attached to. It is expected that the attached device is
+the only child node of the UART device. The slave device node name shall
+reflect the generic type of device for the node.
+
+Required Properties:
+
+- compatible 	: A string reflecting the vendor and specific device the node
+		  represents.
+
+Optional Properties:
+
+- reg		: A single cell representing the port/line number of the
+		  host UART. Only used if the host UART is a single node
+		  with multiple ports.
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+	interrupts = <1>;
+
+	bluetooth {
+		compatible = "brcm,bcm43341-bt";
+		interrupt-parent = <&gpio>;
+		interrupts = <10>;
+	};
+};
--
2.10.1

[toc] | [prev] | [next] | [standalone]

#1553054 — Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

On Friday, January 6, 2017 10:26:32 AM CET Rob Herring wrote:
> +Optional Properties:
> +
> +- reg          : A single cell representing the port/line number of the
> +                 host UART. Only used if the host UART is a single node
> +                 with multiple ports.
> +

If there is a 'reg' property in the child, I guess we should also
document a #address-cells/#size-cells value for the parent.

Can you give an example of a multi-port serial device we support?
I was expecting that we already need a device node per port anyway,
to make the console work.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1553102 — Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

On Fri, Jan 6, 2017 at 1:21 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, January 6, 2017 10:26:32 AM CET Rob Herring wrote:
>> +Optional Properties:
>> +
>> +- reg          : A single cell representing the port/line number of the
>> +                 host UART. Only used if the host UART is a single node
>> +                 with multiple ports.
>> +
>
> If there is a 'reg' property in the child, I guess we should also
> document a #address-cells/#size-cells value for the parent.
>
> Can you give an example of a multi-port serial device we support?

A 16550 DUART chip. Not sure if we have any bindings for one though.
Maybe the chip would be the parent node containing 2 child ns16550
nodes.

> I was expecting that we already need a device node per port anyway,
> to make the console work.

Yes, good point. I think I'll just drop it for now.

Rob

[toc] | [prev] | [next] | [standalone]

#1555879 — Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

On Fri,  6 Jan 2017 10:26:32 -0600
Rob Herring <robh@kernel.org> wrote:

> Add a common binding for describing serial/UART attached devices. Common
> examples are Bluetooth, WiFi, NFC and GPS devices.
> 
> Serial attached devices are represented as child nodes of a UART node.
> This may need to be extended for more complex devices with multiple
> interfaces, but for the simple cases a child node is sufficient.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/serial/slave-device.txt    | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-device.txt b/Documentation/devicetree/bindings/serial/slave-device.txt
> new file mode 100644
> index 000000000000..9b7c2d651345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-device.txt
> @@ -0,0 +1,34 @@
> +Serial Slave Device DT binding
> +
> +This documents the binding structure and common properties for serial
> +attached devices. Common examples include Bluetooth, WiFi, NFC and GPS
> +devices.
> +
> +qSerial

Stray 'q' ??

[toc] | [prev] | [next] | [standalone]

#1555952 — Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[Multipart message — attachments visible in raw view] — view raw

On Fri 2017-01-06 10:26:32, Rob Herring wrote:
> Add a common binding for describing serial/UART attached devices. Common
> examples are Bluetooth, WiFi, NFC and GPS devices.
> 
> Serial attached devices are represented as child nodes of a UART node.
> This may need to be extended for more complex devices with multiple
> interfaces, but for the simple cases a child node is sufficient.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

Looks ok to me.

Acked-by: Pavel Machek <pavel@ucw.cz>

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1552936 — [PATCH 3/9] tty_port: make tty_port_register_device wrap tty_port_register_device_attr

tty_register_device is just a wrapper for tty_register_device_attr with
NULL passed for drvdata and attr_grp. So similarly make
tty_port_register_device a wrapper of tty_port_register_device_attr so that
additions don't have to be made in both functions.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/tty/tty_port.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 606d9e5bf28f..1d8804843103 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -67,8 +67,7 @@ struct device *tty_port_register_device(struct tty_port *port,
 		struct tty_driver *driver, unsigned index,
 		struct device *device)
 {
-	tty_port_link_device(port, driver, index);
-	return tty_register_device(driver, index, device);
+	return tty_port_register_device_attr(port, driver, index, device, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device);
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1552937 — [PATCH 5/9] tty_port: Add port client functions

Introduce a client (upward direction) operations struct for tty_port
clients. Initially supported operations are for receiving data and write
wake-up. This will allow for having clients other than an ldisc.

Convert the calls to the ldisc to use the client ops as the default
operations.

Signed-off-by: Rob Herring <robh@kernel.org>
---

The major change here is the access of the tty ptr and the reference taken
on the ldisc are moved into the client_ops rx function for the the ldisc.
I *think* this should be okay, but no doubt I don't understand all the
intricacies of the locking here. It does make the implementation a bit
cleaner in that the tty buffer handling is free from struct tty and the
ldisc.


 drivers/tty/tty_buffer.c | 17 +++--------------
 drivers/tty/tty_port.c   | 39 ++++++++++++++++++++++++++++++++++++++-
 include/linux/tty.h      |  9 ++++++++-
 3 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index f4dc3e296dd5..4e7a4e9dcf4d 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -437,7 +437,7 @@ int tty_ldisc_receive_buf(struct tty_ldisc *ld, const unsigned char *p,
 EXPORT_SYMBOL_GPL(tty_ldisc_receive_buf);

 static int
-receive_buf(struct tty_ldisc *ld, struct tty_buffer *head, int count)
+receive_buf(struct tty_port *port, struct tty_buffer *head, int count)
 {
 	unsigned char *p = char_buf_ptr(head, head->read);
 	char	      *f = NULL;
@@ -445,7 +445,7 @@ receive_buf(struct tty_ldisc *ld, struct tty_buffer *head, int count)
 	if (~head->flags & TTYB_NORMAL)
 		f = flag_buf_ptr(head, head->read);

-	return tty_ldisc_receive_buf(ld, p, f, count);
+	return port->client_ops->receive_buf(port, p, f, count);
 }

 /**
@@ -465,16 +465,6 @@ static void flush_to_ldisc(struct work_struct *work)
 {
 	struct tty_port *port = container_of(work, struct tty_port, buf.work);
 	struct tty_bufhead *buf = &port->buf;
-	struct tty_struct *tty;
-	struct tty_ldisc *disc;
-
-	tty = READ_ONCE(port->itty);
-	if (tty == NULL)
-		return;
-
-	disc = tty_ldisc_ref(tty);
-	if (disc == NULL)
-		return;

 	mutex_lock(&buf->lock);

@@ -504,7 +494,7 @@ static void flush_to_ldisc(struct work_struct *work)
 			continue;
 		}

-		count = receive_buf(disc, head, count);
+		count = receive_buf(port, head, count);
 		if (!count)
 			break;
 		head->read += count;
@@ -512,7 +502,6 @@ static void flush_to_ldisc(struct work_struct *work)

 	mutex_unlock(&buf->lock);

-	tty_ldisc_deref(disc);
 }

 /**
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 1d8804843103..232a8cbf47bc 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -17,6 +17,41 @@
 #include <linux/delay.h>
 #include <linux/module.h>

+static int tty_port_default_receive_buf(struct tty_port *port,
+					const unsigned char *p,
+					const unsigned char *f, size_t count)
+{
+	int ret;
+	struct tty_struct *tty;
+	struct tty_ldisc *disc;
+
+	tty = READ_ONCE(port->itty);
+	if (!tty)
+		return 0;
+
+	disc = tty_ldisc_ref(tty);
+	if (!disc)
+		return 0;
+
+	ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
+
+	tty_ldisc_deref(disc);
+
+	return ret;
+}
+
+static void tty_port_default_wakeup(struct tty_port *port)
+{
+	/* tty_port_tty_wakeup already took a reference to the tty */
+	tty_wakeup(port->tty);
+}
+
+static const struct tty_port_client_operations default_client_ops = {
+	.receive_buf = tty_port_default_receive_buf,
+	.write_wakeup = tty_port_default_wakeup,
+};
+
+
 void tty_port_init(struct tty_port *port)
 {
 	memset(port, 0, sizeof(*port));
@@ -28,6 +63,7 @@ void tty_port_init(struct tty_port *port)
 	spin_lock_init(&port->lock);
 	port->close_delay = (50 * HZ) / 100;
 	port->closing_wait = (3000 * HZ) / 100;
+	port->client_ops = &default_client_ops;
 	kref_init(&port->kref);
 }
 EXPORT_SYMBOL(tty_port_init);
@@ -275,7 +311,8 @@ void tty_port_tty_wakeup(struct tty_port *port)
 	struct tty_struct *tty = tty_port_tty_get(port);

 	if (tty) {
-		tty_wakeup(tty);
+		if (test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
+			port->client_ops->write_wakeup(port);
 		tty_kref_put(tty);
 	}
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 21c0fabfed60..1017e904c0a3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -217,12 +217,18 @@ struct tty_port_operations {
 	/* Called on the final put of a port */
 	void (*destruct)(struct tty_port *port);
 };
-
+
+struct tty_port_client_operations {
+	int (*receive_buf)(struct tty_port *port, const unsigned char *, const unsigned char *, size_t);
+	void (*write_wakeup)(struct tty_port *port);
+};
+
 struct tty_port {
 	struct tty_bufhead	buf;		/* Locked internally */
 	struct tty_struct	*tty;		/* Back pointer */
 	struct tty_struct	*itty;		/* internal back ptr */
 	const struct tty_port_operations *ops;	/* Port operations */
+	const struct tty_port_client_operations *client_ops; /* Port client operations */
 	spinlock_t		lock;		/* Lock protecting tty field */
 	int			blocked_open;	/* Waiting to open */
 	int			count;		/* Usage count */
@@ -241,6 +247,7 @@ struct tty_port {
 						   based drain is needed else
 						   set to size of fifo */
 	struct kref		kref;		/* Ref counter */
+	void 			*client_data;
 };

 /* tty_port::iflags bits -- use atomic bit ops */
--
2.10.1

[toc] | [prev] | [next] | [standalone]

#1553055

On Friday, January 6, 2017 10:26:26 AM CET Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.

Using serio was originally my idea, but since you seem to have discussed
this in more detail than I ever had, the new version is certainly fine with
me too.

> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> I have hacked up versions of the BT ldisc and TI ST drivers moved over to
> use the serdev bus. I have BT working on the HiKey board which has TI BT.
> With the serdev bus support, it eliminates the need for the TI userspace
> UIM daemon.
> 
> This series and the mentioned drivers can be found here[1].

I took a quick look at the series and have no immediate concerns,
just one detail about the DT binding that seems odd to me.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1553644

On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He
> didn't
> think using serio was a good fit, and serio has a number of
> peculiarities
> in regards to sysfs and it's driver model. I don't think we want to
> inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the
> tty
> core code.
> 
> I have hacked up versions of the BT ldisc and TI ST drivers moved over
> to
> use the serdev bus. I have BT working on the HiKey board which has TI
> BT.
> With the serdev bus support, it eliminates the need for the TI
> userspace
> UIM daemon.
> 
> This series and the mentioned drivers can be found here[1].

For patches 1-4:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Regarding to naming. Why can't we just name bus "serial"? If you are
worrying about folder name under drivers/tty, I can propose at lease
couple of options serialdev, serialbus.

> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> serial-bus-v2
> 
> Alan Cox (1):
>   tty_port: allow a port to be opened with a tty that has no file
> handle
> 
> Rob Herring (8):
>   tty: move the non-file related parts of tty_release to new
>     tty_release_struct
>   tty_port: make tty_port_register_device wrap
>     tty_port_register_device_attr
>   tty: constify tty_ldisc_receive_buf buffer pointer
>   tty_port: Add port client functions
>   dt/bindings: Add a serial/UART attached device binding
>   serdev: Introduce new bus for serial attached devices
>   serdev: add a tty port controller driver
>   tty_port: register tty ports with serdev bus
> 
>  .../devicetree/bindings/serial/slave-device.txt    |  34 ++
>  MAINTAINERS                                        |   8 +
>  drivers/char/Kconfig                               |   1 +
>  drivers/tty/Makefile                               |   1 +
>  drivers/tty/serdev/Kconfig                         |  16 +
>  drivers/tty/serdev/Makefile                        |   5 +
>  drivers/tty/serdev/core.c                          | 388
> +++++++++++++++++++++
>  drivers/tty/serdev/serdev-ttyport.c                | 244
> +++++++++++++
>  drivers/tty/tty_buffer.c                           |  19 +-
>  drivers/tty/tty_io.c                               |  44 ++-
>  drivers/tty/tty_port.c                             |  60 +++-
>  include/linux/serdev.h                             | 227 ++++++++++++
>  include/linux/tty.h                                |  12 +-
>  13 files changed, 1017 insertions(+), 42 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-
> device.txt
>  create mode 100644 drivers/tty/serdev/Kconfig
>  create mode 100644 drivers/tty/serdev/Makefile
>  create mode 100644 drivers/tty/serdev/core.c
>  create mode 100644 drivers/tty/serdev/serdev-ttyport.c
>  create mode 100644 include/linux/serdev.h

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1555734

On Sat, Jan 7, 2017 at 5:00 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>> slaves, etc.).
>>
>> After some discussions with Dmitry at LPC, I decided to move away from
>> extending serio and moved back to making a new bus type instead. He
>> didn't
>> think using serio was a good fit, and serio has a number of
>> peculiarities
>> in regards to sysfs and it's driver model. I don't think we want to
>> inherit
>> those for serial slave devices.
>>
>> This version sits on top of tty_port rather than uart_port as Alan
>> requested. Once I created a struct tty rather than moving everything
>> needed to tty_port, it became a lot easier and less invasive to the
>> tty
>> core code.
>>
>> I have hacked up versions of the BT ldisc and TI ST drivers moved over
>> to
>> use the serdev bus. I have BT working on the HiKey board which has TI
>> BT.
>> With the serdev bus support, it eliminates the need for the TI
>> userspace
>> UIM daemon.
>>
>> This series and the mentioned drivers can be found here[1].
>
> For patches 1-4:
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks.

> Regarding to naming. Why can't we just name bus "serial"? If you are
> worrying about folder name under drivers/tty, I can propose at lease
> couple of options serialdev, serialbus.

Naming is hard, right?

I have don't have too much opinion on what the name should be. I just
came up with something unique and inspired by serio. It is a bit
easier to grep for serdev rather than just serial.

Rob

[toc] | [prev] | [next] | [standalone]

#1555803

Hi Rob,

>>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>>> slaves, etc.).
>>> 
>>> After some discussions with Dmitry at LPC, I decided to move away from
>>> extending serio and moved back to making a new bus type instead. He
>>> didn't
>>> think using serio was a good fit, and serio has a number of
>>> peculiarities
>>> in regards to sysfs and it's driver model. I don't think we want to
>>> inherit
>>> those for serial slave devices.
>>> 
>>> This version sits on top of tty_port rather than uart_port as Alan
>>> requested. Once I created a struct tty rather than moving everything
>>> needed to tty_port, it became a lot easier and less invasive to the
>>> tty
>>> core code.
>>> 
>>> I have hacked up versions of the BT ldisc and TI ST drivers moved over
>>> to
>>> use the serdev bus. I have BT working on the HiKey board which has TI
>>> BT.
>>> With the serdev bus support, it eliminates the need for the TI
>>> userspace
>>> UIM daemon.
>>> 
>>> This series and the mentioned drivers can be found here[1].
>> 
>> For patches 1-4:
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks.
> 
>> Regarding to naming. Why can't we just name bus "serial"? If you are
>> worrying about folder name under drivers/tty, I can propose at lease
>> couple of options serialdev, serialbus.
> 
> Naming is hard, right?
> 
> I have don't have too much opinion on what the name should be. I just
> came up with something unique and inspired by serio. It is a bit
> easier to grep for serdev rather than just serial.

while I do not really care for the function names and how they are named, but I think the sysfs bus name should be just “serial” or something generic like it. It is suppose to be like “usb”, “net”, “bluetooth” etc. Duplicating the word “bus” in it would seem kinda pointless. And that part is ABI and we should be sure that we want it that way. Function names and source directory names can be easily renamed later on.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1553963

[Multipart message — attachments visible in raw view] — view raw

Hi Rob,

On Fri, Jan 06, 2017 at 10:26:26AM -0600, Rob Herring wrote:
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> I have hacked up versions of the BT ldisc and TI ST drivers moved over to
> use the serdev bus. I have BT working on the HiKey board which has TI BT.
> With the serdev bus support, it eliminates the need for the TI userspace
> UIM daemon.
> 
> This series and the mentioned drivers can be found here[1].
> 
> Rob

After adding kernel docs in patch 1, the DT binding change and mine
and Andy's comments in patch 7 the series is

"Reviewed-By: Sebastian Reichel <sre@kernel.org>"

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1555185

Hi Rob,

> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
> 
> Here goes another attempt at a serial device bus (aka uart slaves, tty
> slaves, etc.).
> 
> After some discussions with Dmitry at LPC, I decided to move away from
> extending serio and moved back to making a new bus type instead. He didn't
> think using serio was a good fit, and serio has a number of peculiarities
> in regards to sysfs and it's driver model. I don't think we want to inherit
> those for serial slave devices.
> 
> This version sits on top of tty_port rather than uart_port as Alan
> requested. Once I created a struct tty rather than moving everything
> needed to tty_port, it became a lot easier and less invasive to the tty
> core code.
> 
> I have hacked up versions of the BT ldisc and TI ST drivers moved over to
> use the serdev bus. I have BT working on the HiKey board which has TI BT.
> With the serdev bus support, it eliminates the need for the TI userspace
> UIM daemon.
> 
> This series and the mentioned drivers can be found here[1].
> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2
> 
> Alan Cox (1):
>  tty_port: allow a port to be opened with a tty that has no file handle
> 
> Rob Herring (8):
>  tty: move the non-file related parts of tty_release to new
>    tty_release_struct
>  tty_port: make tty_port_register_device wrap
>    tty_port_register_device_attr
>  tty: constify tty_ldisc_receive_buf buffer pointer
>  tty_port: Add port client functions
>  dt/bindings: Add a serial/UART attached device binding
>  serdev: Introduce new bus for serial attached devices
>  serdev: add a tty port controller driver
>  tty_port: register tty ports with serdev bus
> 
> .../devicetree/bindings/serial/slave-device.txt    |  34 ++
> MAINTAINERS                                        |   8 +
> drivers/char/Kconfig                               |   1 +
> drivers/tty/Makefile                               |   1 +
> drivers/tty/serdev/Kconfig                         |  16 +
> drivers/tty/serdev/Makefile                        |   5 +
> drivers/tty/serdev/core.c                          | 388 +++++++++++++++++++++
> drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
> drivers/tty/tty_buffer.c                           |  19 +-
> drivers/tty/tty_io.c                               |  44 ++-
> drivers/tty/tty_port.c                             |  60 +++-
> include/linux/serdev.h                             | 227 ++++++++++++
> include/linux/tty.h                                |  12 +-
> 13 files changed, 1017 insertions(+), 42 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
> create mode 100644 drivers/tty/serdev/Kconfig
> create mode 100644 drivers/tty/serdev/Makefile
> create mode 100644 drivers/tty/serdev/core.c
> create mode 100644 drivers/tty/serdev/serdev-ttyport.c
> create mode 100644 include/linux/serdev.h
> 
> --
> 2.10.1
> 

First of all many thanks for making another proposal!

Instead of looking into the implementation details of your code I have
hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave
driver) so that it makes use of your new serdev API.

Here are some observations which I hope they give directions where your
work can be improved:

1. it was quite easy to convert the driver to a serdev_device_driver :)

The general driver structure could be taken unchanged and it was mainly
platform_device -> serdev_device_driver and replacing my notification
handlers by serdev_device_ops.

Communication with the chip seems to work well. At least if it is unexpectedly
turned on the driver receives the wrong NMEA records and turns the GPS
chip off. That is the core of our power management scheme and why
we need a serdev driver for this chip at all.

So the general API for getting read/write access to the serial interface
(and setting baud rate) from a device driver seems to be fine!


2. When I try to open the tty from user space to fetch the serial data I
just get

root@letux:~# cat /dev/ttyO1
[  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
[  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
^C
root@letux:~#

So it does not seem to be possible to read the data from the tty any more.

Maybe there can be some function serdev_device_set_shared(dev, flag).
If set to exclusive the /dev node would be hidden from user-space.


3. for completely implementing my w2sg0004 driver (and two others) it would
be nice to have additional serdev_device_ops:

a) to be notified about user-space clients doing open/close on the tty
b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)

There may be other means (ldisc?) to get these notifications, but that
needs the serdev driver to register with two different subsystems.

Another approach could be to completely rewrite the driver so that it wraps
and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
communication. Then it would be notified for all user-space and serial
interface activities as a man-in-the-middle.

But I expect that it delays the communication and is quite some overhead.


4. It seems as if I have to modprobe the driver explicitly (it is not
located and loaded automatically based on the compatible string in DT
like i2c clients).


BR and thanks for your progress,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1555211

Hi Nikolaus,

>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>> slaves, etc.).
>> 
>> After some discussions with Dmitry at LPC, I decided to move away from
>> extending serio and moved back to making a new bus type instead. He didn't
>> think using serio was a good fit, and serio has a number of peculiarities
>> in regards to sysfs and it's driver model. I don't think we want to inherit
>> those for serial slave devices.
>> 
>> This version sits on top of tty_port rather than uart_port as Alan
>> requested. Once I created a struct tty rather than moving everything
>> needed to tty_port, it became a lot easier and less invasive to the tty
>> core code.
>> 
>> I have hacked up versions of the BT ldisc and TI ST drivers moved over to
>> use the serdev bus. I have BT working on the HiKey board which has TI BT.
>> With the serdev bus support, it eliminates the need for the TI userspace
>> UIM daemon.
>> 
>> This series and the mentioned drivers can be found here[1].
>> 
>> Rob
>> 
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2
>> 
>> Alan Cox (1):
>> tty_port: allow a port to be opened with a tty that has no file handle
>> 
>> Rob Herring (8):
>> tty: move the non-file related parts of tty_release to new
>>   tty_release_struct
>> tty_port: make tty_port_register_device wrap
>>   tty_port_register_device_attr
>> tty: constify tty_ldisc_receive_buf buffer pointer
>> tty_port: Add port client functions
>> dt/bindings: Add a serial/UART attached device binding
>> serdev: Introduce new bus for serial attached devices
>> serdev: add a tty port controller driver
>> tty_port: register tty ports with serdev bus
>> 
>> .../devicetree/bindings/serial/slave-device.txt    |  34 ++
>> MAINTAINERS                                        |   8 +
>> drivers/char/Kconfig                               |   1 +
>> drivers/tty/Makefile                               |   1 +
>> drivers/tty/serdev/Kconfig                         |  16 +
>> drivers/tty/serdev/Makefile                        |   5 +
>> drivers/tty/serdev/core.c                          | 388 +++++++++++++++++++++
>> drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
>> drivers/tty/tty_buffer.c                           |  19 +-
>> drivers/tty/tty_io.c                               |  44 ++-
>> drivers/tty/tty_port.c                             |  60 +++-
>> include/linux/serdev.h                             | 227 ++++++++++++
>> include/linux/tty.h                                |  12 +-
>> 13 files changed, 1017 insertions(+), 42 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
>> create mode 100644 drivers/tty/serdev/Kconfig
>> create mode 100644 drivers/tty/serdev/Makefile
>> create mode 100644 drivers/tty/serdev/core.c
>> create mode 100644 drivers/tty/serdev/serdev-ttyport.c
>> create mode 100644 include/linux/serdev.h
>> 
>> --
>> 2.10.1
>> 
> 
> First of all many thanks for making another proposal!
> 
> Instead of looking into the implementation details of your code I have
> hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave
> driver) so that it makes use of your new serdev API.
> 
> Here are some observations which I hope they give directions where your
> work can be improved:
> 
> 1. it was quite easy to convert the driver to a serdev_device_driver :)
> 
> The general driver structure could be taken unchanged and it was mainly
> platform_device -> serdev_device_driver and replacing my notification
> handlers by serdev_device_ops.
> 
> Communication with the chip seems to work well. At least if it is unexpectedly
> turned on the driver receives the wrong NMEA records and turns the GPS
> chip off. That is the core of our power management scheme and why
> we need a serdev driver for this chip at all.
> 
> So the general API for getting read/write access to the serial interface
> (and setting baud rate) from a device driver seems to be fine!
> 
> 
> 2. When I try to open the tty from user space to fetch the serial data I
> just get
> 
> root@letux:~# cat /dev/ttyO1
> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
> ^C
> root@letux:~#
> 
> So it does not seem to be possible to read the data from the tty any more.
> 
> Maybe there can be some function serdev_device_set_shared(dev, flag).
> If set to exclusive the /dev node would be hidden from user-space.

I would welcome hiding the /dev node completely as an option. That is especially useful for systems where an upstream driver exists and hooks it up directly into the Bluetooth subsystem already.

> 3. for completely implementing my w2sg0004 driver (and two others) it would
> be nice to have additional serdev_device_ops:
> 
> a) to be notified about user-space clients doing open/close on the tty
> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
> 
> There may be other means (ldisc?) to get these notifications, but that
> needs the serdev driver to register with two different subsystems.
> 
> Another approach could be to completely rewrite the driver so that it wraps
> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
> communication. Then it would be notified for all user-space and serial
> interface activities as a man-in-the-middle.
> 
> But I expect that it delays the communication and is quite some overhead.

My important thing to fix with GPS devices is that we can enumerate them from userspace daemons correctly. So it either becomes its own GPS subsystem or we need sysfs attributes like DEVTYPE clearly identifying them as GPS devices (similar to what we did with network interfaces).

So the question is really if a driver only needs to do power management on open() and close() or if it also has to translate or transform the packets. There are devices who speak NMEA and all is good. And there are others that get plain raw data and need extra work in a daemon to translate it into NMEA or some form of position information.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1555215

Hi Marcel,

> Am 10.01.2017 um 13:02 schrieb Marcel Holtmann <marcel@holtmann.org>:
> 
> Hi Nikolaus,
> 
>>> Here goes another attempt at a serial device bus (aka uart slaves, tty
>>> slaves, etc.).
>>> 
>>> After some discussions with Dmitry at LPC, I decided to move away from
>>> extending serio and moved back to making a new bus type instead. He didn't
>>> think using serio was a good fit, and serio has a number of peculiarities
>>> in regards to sysfs and it's driver model. I don't think we want to inherit
>>> those for serial slave devices.
>>> 
>>> This version sits on top of tty_port rather than uart_port as Alan
>>> requested. Once I created a struct tty rather than moving everything
>>> needed to tty_port, it became a lot easier and less invasive to the tty
>>> core code.
>>> 
>>> I have hacked up versions of the BT ldisc and TI ST drivers moved over to
>>> use the serdev bus. I have BT working on the HiKey board which has TI BT.
>>> With the serdev bus support, it eliminates the need for the TI userspace
>>> UIM daemon.
>>> 
>>> This series and the mentioned drivers can be found here[1].
>>> 
>>> Rob
>>> 
>>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git serial-bus-v2
>>> 
>>> Alan Cox (1):
>>> tty_port: allow a port to be opened with a tty that has no file handle
>>> 
>>> Rob Herring (8):
>>> tty: move the non-file related parts of tty_release to new
>>>  tty_release_struct
>>> tty_port: make tty_port_register_device wrap
>>>  tty_port_register_device_attr
>>> tty: constify tty_ldisc_receive_buf buffer pointer
>>> tty_port: Add port client functions
>>> dt/bindings: Add a serial/UART attached device binding
>>> serdev: Introduce new bus for serial attached devices
>>> serdev: add a tty port controller driver
>>> tty_port: register tty ports with serdev bus
>>> 
>>> .../devicetree/bindings/serial/slave-device.txt    |  34 ++
>>> MAINTAINERS                                        |   8 +
>>> drivers/char/Kconfig                               |   1 +
>>> drivers/tty/Makefile                               |   1 +
>>> drivers/tty/serdev/Kconfig                         |  16 +
>>> drivers/tty/serdev/Makefile                        |   5 +
>>> drivers/tty/serdev/core.c                          | 388 +++++++++++++++++++++
>>> drivers/tty/serdev/serdev-ttyport.c                | 244 +++++++++++++
>>> drivers/tty/tty_buffer.c                           |  19 +-
>>> drivers/tty/tty_io.c                               |  44 ++-
>>> drivers/tty/tty_port.c                             |  60 +++-
>>> include/linux/serdev.h                             | 227 ++++++++++++
>>> include/linux/tty.h                                |  12 +-
>>> 13 files changed, 1017 insertions(+), 42 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
>>> create mode 100644 drivers/tty/serdev/Kconfig
>>> create mode 100644 drivers/tty/serdev/Makefile
>>> create mode 100644 drivers/tty/serdev/core.c
>>> create mode 100644 drivers/tty/serdev/serdev-ttyport.c
>>> create mode 100644 include/linux/serdev.h
>>> 
>>> --
>>> 2.10.1
>>> 
>> 
>> First of all many thanks for making another proposal!
>> 
>> Instead of looking into the implementation details of your code I have
>> hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave
>> driver) so that it makes use of your new serdev API.
>> 
>> Here are some observations which I hope they give directions where your
>> work can be improved:
>> 
>> 1. it was quite easy to convert the driver to a serdev_device_driver :)
>> 
>> The general driver structure could be taken unchanged and it was mainly
>> platform_device -> serdev_device_driver and replacing my notification
>> handlers by serdev_device_ops.
>> 
>> Communication with the chip seems to work well. At least if it is unexpectedly
>> turned on the driver receives the wrong NMEA records and turns the GPS
>> chip off. That is the core of our power management scheme and why
>> we need a serdev driver for this chip at all.
>> 
>> So the general API for getting read/write access to the serial interface
>> (and setting baud rate) from a device driver seems to be fine!
>> 
>> 
>> 2. When I try to open the tty from user space to fetch the serial data I
>> just get
>> 
>> root@letux:~# cat /dev/ttyO1
>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>> ^C
>> root@letux:~#
>> 
>> So it does not seem to be possible to read the data from the tty any more.
>> 
>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>> If set to exclusive the /dev node would be hidden from user-space.
> 
> I would welcome hiding the /dev node completely as an option. That is especially useful for systems where an upstream driver exists and hooks it up directly into the Bluetooth subsystem already.

Yes, that is why I would like to see it hidden/exposed as an option.

I don't really care if it is done by some DT property or such a function.

> 
>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>> be nice to have additional serdev_device_ops:
>> 
>> a) to be notified about user-space clients doing open/close on the tty
>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>> 
>> There may be other means (ldisc?) to get these notifications, but that
>> needs the serdev driver to register with two different subsystems.
>> 
>> Another approach could be to completely rewrite the driver so that it wraps
>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>> communication. Then it would be notified for all user-space and serial
>> interface activities as a man-in-the-middle.
>> 
>> But I expect that it delays the communication and is quite some overhead.
> 
> My important thing to fix with GPS devices is that we can enumerate them from userspace daemons correctly. So it either becomes its own GPS subsystem or we need sysfs attributes like DEVTYPE clearly identifying them as GPS devices (similar to what we did with network interfaces).
> 
> So the question is really if a driver only needs to do power management on open() and close() or if it also has to translate or transform the packets. There are devices who speak NMEA and all is good.

The device I want to upstream the driver speaks NMEA but should be powered down unless accessed...

> And there are others that get plain raw data and need extra work in a daemon to translate it into NMEA or some form of position information.

Indeed and that should also be possible.
In that case you likely must follow the man-in-the-middle approach,
quite similar to how Rob has updated the ti-st driver. I have seen code
where it creates some /dev/hci (if I remember correctly).

BR,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1555220

On Tue, 2017-01-10 at 13:10 +0100, H. Nikolaus Schaller wrote:

> > So the question is really if a driver only needs to do power
> > management on open() and close() or if it also has to translate or
> > transform the packets. There are devices who speak NMEA and all is
> > good.
> 
> The device I want to upstream the driver speaks NMEA but should be
> powered down unless accessed...

By the way, have you seen the series [1] I'm working on towards bringing
runtime PM for all (current) serial drivers?

[1] https://www.spinics.net/lists/linux-serial/msg24025.html

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1555229

Hi Andy,

> Am 10.01.2017 um 13:20 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> On Tue, 2017-01-10 at 13:10 +0100, H. Nikolaus Schaller wrote:
> 
>>> So the question is really if a driver only needs to do power
>>> management on open() and close() or if it also has to translate or
>>> transform the packets. There are devices who speak NMEA and all is
>>> good.
>> 
>> The device I want to upstream the driver speaks NMEA but should be
>> powered down unless accessed...
> 
> By the way, have you seen the series [1] I'm working on towards bringing
> runtime PM for all (current) serial drivers?
> 
> [1] https://www.spinics.net/lists/linux-serial/msg24025.html

sorry no.

Interesting: "The series has been tested on our hardware with serial console and RxD
used as GPIO to wake it."

We had implemented a driver ca. 5 years ago (Neil Brown did it) for our GPS chip by
doing exactly this (setting the RxD to GPIO and interrupt mode to know when it sends
data but the UART is off).

This would have been completely based on existing APIs (pinmux states, gpio)
and would not even have needed any general serial device bus driver API because
it did not tamper with the UART+tty layers but the RxD line.

So from a viewpoint of encapsulation it would have done what it should inside the
driver.

But there was no consensus to accept that to mainline.

What we need for this chip is quite special regarding PM. It is not about knowing
when to suspend/resume or power down/up the UART or host.

It is about that the chip might be in the wrong state, i.e. powered on when it
should be off and vice versa. This can only be detected by monitoring RxD
activity and taking action if it is in the unexpected state. And that must
be possible independently of some user space having /dev/ttyO1 opened.

BR,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1558444

On Fri, Jan 13, 2017 at 5:22 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> Hi Rob,
> was it intentional to answer privately only?

Damn gmail. Added everyone back.

>> Am 12.01.2017 um 23:07 schrieb Rob Herring <robh@kernel.org>:
>>
>> On Tue, Jan 10, 2017 at 5:44 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> Hi Rob,
>>>
>>>> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
>>>>

[...]

>>> 2. When I try to open the tty from user space to fetch the serial data I
>>> just get
>>>
>>> root@letux:~# cat /dev/ttyO1
>>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>>> ^C
>>> root@letux:~#
>>>
>>> So it does not seem to be possible to read the data from the tty any more.
>>>
>>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>>> If set to exclusive the /dev node would be hidden from user-space.
>>
>> I don't think sharing should be allowed. Either you have an in-kernel
>> driver or you handle it in userspace. Sharing is just asking for
>> trouble IMO.
>
> My tty-slave patch series works and has no trouble with sharing (the UART)
> because it was designed with this in mind.
>
> The only trouble is that it did not find maintainer's acceptance...
>
>> Though it could be supported later.
>
> Firstly, let me point out once again that we have a mobile device, battery
> powered and every component should be and stay turned off, if not needed by
> any consumer.

That's every device...

> The only component that can reliably detect if there is no consumer in user-space
> is the kernel. Hence it must be a kernel driver that powers the device on/off.
>
> On the other side, it should simply pass data unmodified to user space (because the
> chip provides cooked data), so we do not need a driver for doing any data processing.
> The data stream is provided perfectly (without proper power control) by simply
> accessing /dev/ttyO1 from user-space.
>
> So if there were no power control topic, we would not even ask for a driver.

I think this reasoning is exactly why we have no proper subsystem
today. We *almost* don't need one. It all works fine except I have
this one GPIO to control. Oh, and a regulator and firmware and
suspend/resume control and ...

> We only need the driver to detect the power state of the chip by *monitoring*
> the data stream that goes to user space.
>
> Of course shared writing to the chip would give trouble, but we do not need it.
> Therefore I am happy if this sharing is an option (with a big WARNING sign).
>
> If we would try to achieve the same power management in user-space we would have
> to run a power-consuming daemon and make sure that it *never* crashes (or people
> come and complain about short battery life even if they think they have GPS
> turned off).
>
> And we need to be able to maintain the daemon for many different distributions
> people want to run on our device. This takes years to get it into Debian and
> others where we are not developers at all.

Exactly one of the problems we're trying to solve. With your desired
approach though, you are still leaving the problem of having to know
which tty device the GPS chip is connected to which varies with each
board. Either you hardcode the tty device in userspace, provide some
sysfs file with the tty name (like TI-ST) or link, provide the tty
name in DT (which I'll NAK) or have userspace parse the DT to find the
connection. I've seen all but the last case. I want to solve this
problem, too, such that userspace just opens the BT, WiFi, GPS, etc.
device.

> So this data stream sharing/monitoring is the most important part for getting our
> chip supported by a kernel driver.
>
>>
>> I've updated the series to skip creating the /dev nodes.
>
> That is exactly what we do NOT need for this chip. Now I can't even access it
> any more when powered on...

It's a minor change. Essentially, it is call tty_register_device_attr
or not. There's the issue of the file open count warning which I don't
know how to solve.

>>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>>> be nice to have additional serdev_device_ops:
>>>
>>> a) to be notified about user-space clients doing open/close on the tty
>>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>>>
>>> There may be other means (ldisc?) to get these notifications, but that
>>> needs the serdev driver to register with two different subsystems.
>>>
>>> Another approach could be to completely rewrite the driver so that it wraps
>>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>>> communication. Then it would be notified for all user-space and serial
>>> interface activities as a man-in-the-middle.
>>
>> This was my thinking. If we only need data read and write, I don't
>> think we gain much using a tty vs. a new char dev.
>
> We gain re-use of the existing tty for its key purpose to mediate between an uart
> device and user-space.
>
> I have looked into the chardev approach and it appears to be quite some overkill
> to copy serial data from the UART to a user space file handle.
>
> About buffering: with the chardev approach we have to implement our own buffer
> in the driver and decide what happens on overflow while the tty layer already
> efficiently handles this.

Not exactly. There's already buffering in tty_buffer.c. That handles
overflow of the UART. Then there is a 4K circular buffer in n_tty. The
question really is how much of the per character and flag processing
of n_tty do you need as that is where the complexity is. I'd guess not
much of it. If it is needed, then perhaps n_tty.c could be refactored
to provide common functions.

> And it is a little strange that the device can either be accessed through
> /dev/ttyO1 if the driver is not loaded and only through /dev/gps if it is.

We have similar things with SPI, I2C, and USB. Either you have a
kernel driver or you have a userspace driver. The userspace drivers
have limitations and if those limitations are a problem, we right
kernel drivers instead.

> New problems arise if there were two such chips. Then we must be prepared
> that several instances provide different /dev/gps[1-9] nodes and that they
> can be identified and are stable. Maybe we have to introduce udev-rules...

That's a solved problem generally (though not all like the answer).

> So what seems to be an obvious and straightforward solution (from serdev perspective)
> is not, if we look into implementation details of the driver.

I can't solve all problems for all possible drivers on day one. It
does provide a solution for drivers that are already in the kernel.
I'd suggest we debate adding sharing capability vs. a GPS subsystem
separately. If there was already a GPS subsystem there would be no
debate. I don't think what's here is preventing either case.
Similarly, I don't pretend the configuration API (baud rate and
flow-control) is complete. I'm sure someone will need additional
functions, but those can all be incrementally added as needed.

>>> But I expect that it delays the communication and is quite some overhead.
>>>
>>>
>>> 4. It seems as if I have to modprobe the driver explicitly (it is not
>>> located and loaded automatically based on the compatible string in DT
>>> like i2c clients).
>>
>> I've added what I think should be needed for that. I pushed out a new
>> branch[1]. Can you give it a try?
>
> Yes, sure. I have tried and now our driver module is loaded as expected :)
>
> But in general we are turning away from a solution for our w2sg0004 driver
> (see above).
>
> BTW: I see an issue in our kernel (config?) that the console and initd
> blocks for approx. 60 seconds during boot when serdev is merged (even
> if not configured). This issue disappears when using the omap2plus_defconfig.
>
> But I have not digged into that (because it may be spurious or EPROBE_DEFER
> related).

I've not seen anything like that. Do you have a diff of your configs?
And what is your init system?

Rob

[toc] | [prev] | [next] | [standalone]

#1559474

Hi Rob,

> Am 13.01.2017 um 15:48 schrieb Rob Herring <robh@kernel.org>:
> 
> On Fri, Jan 13, 2017 at 5:22 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> Hi Rob,
>> was it intentional to answer privately only?
> 
> Damn gmail. Added everyone back.

No problem. Happens to everyone every now and then.

> 
>>> Am 12.01.2017 um 23:07 schrieb Rob Herring <robh@kernel.org>:
>>> 
>>> On Tue, Jan 10, 2017 at 5:44 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>> Hi Rob,
>>>> 
>>>>> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
>>>>> 
> 
> [...]
> 
>>>> 2. When I try to open the tty from user space to fetch the serial data I
>>>> just get
>>>> 
>>>> root@letux:~# cat /dev/ttyO1
>>>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>>>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>>>> ^C
>>>> root@letux:~#
>>>> 
>>>> So it does not seem to be possible to read the data from the tty any more.
>>>> 
>>>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>>>> If set to exclusive the /dev node would be hidden from user-space.
>>> 
>>> I don't think sharing should be allowed. Either you have an in-kernel
>>> driver or you handle it in userspace. Sharing is just asking for
>>> trouble IMO.
>> 
>> My tty-slave patch series works and has no trouble with sharing (the UART)
>> because it was designed with this in mind.
>> 
>> The only trouble is that it did not find maintainer's acceptance...
>> 
>>> Though it could be supported later.
>> 
>> Firstly, let me point out once again that we have a mobile device, battery
>> powered and every component should be and stay turned off, if not needed by
>> any consumer.
> 
> That's every device...
> 
>> The only component that can reliably detect if there is no consumer in user-space
>> is the kernel. Hence it must be a kernel driver that powers the device on/off.
>> 
>> On the other side, it should simply pass data unmodified to user space (because the
>> chip provides cooked data), so we do not need a driver for doing any data processing.
>> The data stream is provided perfectly (without proper power control) by simply
>> accessing /dev/ttyO1 from user-space.
>> 
>> So if there were no power control topic, we would not even ask for a driver.
> 
> I think this reasoning is exactly why we have no proper subsystem
> today. We *almost* don't need one. It all works fine except I have
> this one GPIO to control. Oh, and a regulator and firmware and
> suspend/resume control and ...

In our case we have no firmware, just the gpio.

> 
>> We only need the driver to detect the power state of the chip by *monitoring*
>> the data stream that goes to user space.
>> 
>> Of course shared writing to the chip would give trouble, but we do not need it.
>> Therefore I am happy if this sharing is an option (with a big WARNING sign).
>> 
>> If we would try to achieve the same power management in user-space we would have
>> to run a power-consuming daemon and make sure that it *never* crashes (or people
>> come and complain about short battery life even if they think they have GPS
>> turned off).
>> 
>> And we need to be able to maintain the daemon for many different distributions
>> people want to run on our device. This takes years to get it into Debian and
>> others where we are not developers at all.
> 
> Exactly one of the problems we're trying to solve. With your desired
> approach though, you are still leaving the problem of having to know
> which tty device the GPS chip is connected to which varies with each
> board.
> Either you hardcode the tty device in userspace, provide some
> sysfs file with the tty name (like TI-ST) or link, provide the tty
> name in DT (which I'll NAK) or have userspace parse the DT to find the
> connection. I've seen all but the last case. I want to solve this
> problem, too, such that userspace just opens the BT, WiFi, GPS, etc.
> device.

Yes, this is indeed an issue. There are also USB or Bluetooth based external
GPS devices which do not need a driver because they speak some standard
profile, identify themselves as GPS and create some tty port through standard
mechanisms.

> 
>> So this data stream sharing/monitoring is the most important part for getting our
>> chip supported by a kernel driver.
>> 
>>> 
>>> I've updated the series to skip creating the /dev nodes.
>> 
>> That is exactly what we do NOT need for this chip. Now I can't even access it
>> any more when powered on...
> 
> It's a minor change. Essentially, it is call tty_register_device_attr
> or not.

Fine!

> There's the issue of the file open count warning which I don't
> know how to solve.

Unfortunately I am not experienced with the tty layer to help here.

> 
>>>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>>>> be nice to have additional serdev_device_ops:
>>>> 
>>>> a) to be notified about user-space clients doing open/close on the tty
>>>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>>>> 
>>>> There may be other means (ldisc?) to get these notifications, but that
>>>> needs the serdev driver to register with two different subsystems.
>>>> 
>>>> Another approach could be to completely rewrite the driver so that it wraps
>>>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>>>> communication. Then it would be notified for all user-space and serial
>>>> interface activities as a man-in-the-middle.
>>> 
>>> This was my thinking. If we only need data read and write, I don't
>>> think we gain much using a tty vs. a new char dev.
>> 
>> We gain re-use of the existing tty for its key purpose to mediate between an uart
>> device and user-space.
>> 
>> I have looked into the chardev approach and it appears to be quite some overkill
>> to copy serial data from the UART to a user space file handle.
>> 
>> About buffering: with the chardev approach we have to implement our own buffer
>> in the driver and decide what happens on overflow while the tty layer already
>> efficiently handles this.
> 
> Not exactly. There's already buffering in tty_buffer.c. That handles
> overflow of the UART. Then there is a 4K circular buffer in n_tty. The
> question really is how much of the per character and flag processing
> of n_tty do you need as that is where the complexity is. I'd guess not
> much of it. If it is needed, then perhaps n_tty.c could be refactored
> to provide common functions.

Ah, that is good if the serdev driver can rely on / reuse this buffer.

Where I don't see immediately is how I can make the chardev read file operation
block until I get some receive_buf serdev_device_op and how I manage different
data size requests without introducing another driver-internal buffer.

So it could be better to alloc_tty_driver another tty and copy to its
read buffer. 

> 
>> And it is a little strange that the device can either be accessed through
>> /dev/ttyO1 if the driver is not loaded and only through /dev/gps if it is.
> 
> We have similar things with SPI, I2C, and USB. Either you have a
> kernel driver or you have a userspace driver. The userspace drivers
> have limitations and if those limitations are a problem, we right
> kernel drivers instead.
> 
>> New problems arise if there were two such chips. Then we must be prepared
>> that several instances provide different /dev/gps[1-9] nodes and that they
>> can be identified and are stable. Maybe we have to introduce udev-rules...
> 
> That's a solved problem generally (though not all like the answer).

Well, I have no problem doing it that way but it increases complexity of the
driver and makes it more difficult to configure in user space.

> 
>> So what seems to be an obvious and straightforward solution (from serdev perspective)
>> is not, if we look into implementation details of the driver.
> 
> I can't solve all problems for all possible drivers on day one. It
> does provide a solution for drivers that are already in the kernel.

Well, I hope you understand that I am a little impatient here...

It is more than 2 years ago that we proposed the first driver for this chip
for upstreaming (while we have something running on our own kernels much longer
time):

	http://lkml.iu.edu/hypermail/linux/kernel/1410.2/00634.html

And if I counted correctly, I am now working on the fourth completely different
proposal with no finalization in sight.

So this situation is a little circular: because we are not in kernel we have to
wait longer until we can get in...

> I'd suggest we debate adding sharing capability vs. a GPS subsystem
> separately.

Well, I would be perfectly happy without having to wait for a fully worked out
GPS subsystem. If it arrives in my (device's) life-time we can rework the driver
to fit into it.

This approach seems not to be an exception to me. For example the original misc/bmp085
driver has completely gone in 4.9 and been replaced by a generic iio driver.

> If there was already a GPS subsystem there would be no
> debate. I don't think what's here is preventing either case.
> Similarly, I don't pretend the configuration API (baud rate and
> flow-control) is complete. I'm sure someone will need additional
> functions, but those can all be incrementally added as needed.
> 
>>>> But I expect that it delays the communication and is quite some overhead.
>>>> 
>>>> 
>>>> 4. It seems as if I have to modprobe the driver explicitly (it is not
>>>> located and loaded automatically based on the compatible string in DT
>>>> like i2c clients).
>>> 
>>> I've added what I think should be needed for that. I pushed out a new
>>> branch[1]. Can you give it a try?
>> 
>> Yes, sure. I have tried and now our driver module is loaded as expected :)
>> 
>> But in general we are turning away from a solution for our w2sg0004 driver
>> (see above).
>> 
>> BTW: I see an issue in our kernel (config?) that the console and initd
>> blocks for approx. 60 seconds during boot when serdev is merged (even
>> if not configured). This issue disappears when using the omap2plus_defconfig.
>> 
>> But I have not digged into that (because it may be spurious or EPROBE_DEFER
>> related).
> 
> I've not seen anything like that. Do you have a diff of your configs?
> And what is your init system?

I did use Debian with sysv init and a getty on /dev/ttyO2 (OMAP3 console UART).

But let me cross-check some things before digging deeper into it. To exclude
that it is our fault and indeed in the serdev patch set.

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web