[PATCH 0/7] gnss: add new GNSS subsystem

[PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

This series adds a new subsystem for GNSS receivers (e.g. GPS
receivers).

While GNSS receivers are typically accessed using a UART interface they
often also support other I/O interfaces such as I2C, SPI and USB, while
yet other devices use iomem or even some form of remote-processor
messaging (rpmsg).
    
The new GNSS subsystem abstracts the underlying interface and provides a
new "gnss" class type, which exposes a character-device interface (e.g.
/dev/gnss0) to user space. This allows GNSS receivers to have a
representation in the Linux device model, something which is important
not least for power management purposes and which also allows for easy
detection and (eventually) identification of GNSS devices.

Note that the character-device interface provides raw access to whatever
protocol the receiver is (currently) using, such as NMEA 0183, UBX or
SiRF Binary. These protocols are expected to be continued to be handled
by user space for the time being, even if some hybrid solutions are also
conceivable (e.g. to have kernel drivers issue management commands).

This will still allow for better platform integration by allowing GNSS
devices and their resources (e.g. regulators and enable-gpios) to be
described by firmware and managed by kernel drivers rather than
platform-specific scripts and services.
    
While the current interface is kept minimal, it could be extended using
IOCTLs, sysfs or uevents as needs and proper abstraction levels are
identified and determined (e.g. for device and feature identification).

Another possible extension is to add generic 1PPS support.

I decided to go with a custom character-device interface rather than
pretend that these abstract GNSS devices are still TTY devices (e.g.
/dev/ttyGNSS0). Obviously, modifying line settings or reading modem
control signals does not make any sense for a device using, say, a
USB (not USB-serial) or iomem interface. This also means, however, that
user space would no longer be able to set the line speed to match a new
port configuration that can be set using the various GNSS protocols when
the underlying interface is indeed a UART; instead this would need to be
taken care of by the driver.

Also note that writes are always synchronous instead of requiring user
space to call tcdrain() after every command.

This all seems to work well-enough (e.g. with gpsd), but please let me
know if I've overlooked something which would indeed require a TTY
interface instead.

As proof-of-concept I have implemented drivers for receivers based on
two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
hardware these have so far only been tested using mockup devices and a
USB-serial-based GPS device (using out-of-tree code). [ Let me know if
you've got any evalutation kits to spare. ]

Finally, note that documentation (including kerneldoc) remains to be
written, but hopefully this will not hinder review given that the
current interfaces are fairly self-describing.

Johan


Johan Hovold (7):
  gnss: add GNSS receiver subsystem
  dt-bindings: add generic gnss binding
  gnss: add generic serial driver
  dt-bindings: gnss: add u-blox binding
  gnss: add driver for u-blox receivers
  dt-bindings: gnss: add sirfstar binding
  gnss: add driver for sirfstar-based receivers

 .../devicetree/bindings/gnss/gnss.txt         |  36 ++
 .../devicetree/bindings/gnss/sirfstar.txt     |  38 ++
 .../devicetree/bindings/gnss/u-blox.txt       |  31 ++
 .../devicetree/bindings/vendor-prefixes.txt   |   4 +
 MAINTAINERS                                   |   7 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/gnss/Kconfig                          |  43 ++
 drivers/gnss/Makefile                         |  16 +
 drivers/gnss/core.c                           | 385 ++++++++++++++++
 drivers/gnss/serial.c                         | 288 ++++++++++++
 drivers/gnss/serial.h                         |  47 ++
 drivers/gnss/sirf.c                           | 415 ++++++++++++++++++
 drivers/gnss/ubx.c                            | 133 ++++++
 include/linux/gnss.h                          |  64 +++
 15 files changed, 1510 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/gnss.txt
 create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt
 create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
 create mode 100644 drivers/gnss/Kconfig
 create mode 100644 drivers/gnss/Makefile
 create mode 100644 drivers/gnss/core.c
 create mode 100644 drivers/gnss/serial.c
 create mode 100644 drivers/gnss/serial.h
 create mode 100644 drivers/gnss/sirf.c
 create mode 100644 drivers/gnss/ubx.c
 create mode 100644 include/linux/gnss.h

-- 
2.17.0

[PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

Add a new subsystem for GNSS (e.g. GPS) receivers.

While GNSS receivers are typically accessed using a UART interface they
often also support other I/O interfaces such as I2C, SPI and USB, while
yet other devices use iomem or even some form of remote-processor
messaging (rpmsg).

The new GNSS subsystem abstracts the underlying interface and provides a
new "gnss" class type, which exposes a character-device interface (e.g.
/dev/gnss0) to user space. This allows GNSS receivers to have a
representation in the Linux device model, something which is important
not least for power management purposes.

Note that the character-device interface provides raw access to whatever
protocol the receiver is (currently) using, such as NMEA 0183, UBX or
SiRF Binary. These protocols are expected to be continued to be handled
by user space for the time being, even if some hybrid solutions are also
conceivable (e.g. to have kernel drivers issue management commands).

This will still allow for better platform integration by allowing GNSS
devices and their resources (e.g. regulators and enable-gpios) to be
described by firmware and managed by kernel drivers rather than
platform-specific scripts and services.

While the current interface is kept minimal, it could be extended using
IOCTLs, sysfs or uevents as needs and proper abstraction levels are
identified and determined (e.g. for device and feature identification).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 MAINTAINERS           |   6 +
 drivers/Kconfig       |   2 +
 drivers/Makefile      |   1 +
 drivers/gnss/Kconfig  |  11 ++
 drivers/gnss/Makefile |   7 +
 drivers/gnss/core.c   | 385 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/gnss.h  |  64 +++++++
 7 files changed, 476 insertions(+)
 create mode 100644 drivers/gnss/Kconfig
 create mode 100644 drivers/gnss/Makefile
 create mode 100644 drivers/gnss/core.c
 create mode 100644 include/linux/gnss.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d5a621..dc3df211c1a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5962,6 +5962,12 @@ F:	Documentation/isdn/README.gigaset
 F:	drivers/isdn/gigaset/
 F:	include/uapi/linux/gigaset_dev.h
 
+GNSS SUBSYSTEM
+M:	Johan Hovold <johan@kernel.org>
+S:	Maintained
+F:	drivers/gnss/
+F:	include/linux/gnss.h
+
 GO7007 MPEG CODEC
 M:	Hans Verkuil <hans.verkuil@cisco.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 95b9ccc08165..ab4d43923c4d 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -9,6 +9,8 @@ source "drivers/bus/Kconfig"
 
 source "drivers/connector/Kconfig"
 
+source "drivers/gnss/Kconfig"
+
 source "drivers/mtd/Kconfig"
 
 source "drivers/of/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..cc9a7c5f7d2c 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -185,3 +185,4 @@ obj-$(CONFIG_TEE)		+= tee/
 obj-$(CONFIG_MULTIPLEXER)	+= mux/
 obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus/
 obj-$(CONFIG_SIOX)		+= siox/
+obj-$(CONFIG_GNSS)		+= gnss/
diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
new file mode 100644
index 000000000000..103fcc70992e
--- /dev/null
+++ b/drivers/gnss/Kconfig
@@ -0,0 +1,11 @@
+#
+# GNSS receiver configuration
+#
+
+menuconfig GNSS
+	tristate "GNSS receiver support"
+	---help---
+	  Say Y here if you have a GNSS receiver (e.g. a GPS receiver).
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss.
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
new file mode 100644
index 000000000000..1f7a7baab1d9
--- /dev/null
+++ b/drivers/gnss/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the GNSS subsystem.
+#
+
+obj-$(CONFIG_GNSS)			+= gnss.o
+gnss-y := core.o
diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
new file mode 100644
index 000000000000..9b88c7e72233
--- /dev/null
+++ b/drivers/gnss/core.c
@@ -0,0 +1,385 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GNSS receiver core
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/cdev.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/gnss.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/wait.h>
+
+#define GNSS_MINORS	16
+
+static DEFINE_IDA(gnss_minors);
+static dev_t gnss_first;
+
+/* FIFO size must be a power of two */
+#define GNSS_READ_FIFO_SIZE	4096
+#define GNSS_WRITE_BUF_SIZE	1024
+
+#define to_gnss_device(d) container_of((d), struct gnss_device, dev)
+
+static int gnss_open(struct inode *inode, struct file *file)
+{
+	struct gnss_device *gdev;
+	int ret = 0;
+
+	gdev = container_of(inode->i_cdev, struct gnss_device, cdev);
+
+	get_device(&gdev->dev);
+
+	nonseekable_open(inode, file);
+	file->private_data = gdev;
+
+	down_write(&gdev->rwsem);
+	if (gdev->disconnected) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	if (gdev->count++ == 0) {
+		ret = gdev->ops->open(gdev);
+		if (ret)
+			gdev->count--;
+	}
+unlock:
+	up_write(&gdev->rwsem);
+
+	if (ret)
+		goto err_put_device;
+
+	return 0;
+
+err_put_device:
+	put_device(&gdev->dev);
+
+	return ret;
+}
+
+static int gnss_release(struct inode *inode, struct file *file)
+{
+	struct gnss_device *gdev = file->private_data;
+
+	down_write(&gdev->rwsem);
+	if (gdev->disconnected)
+		goto unlock;
+
+	if (--gdev->count == 0) {
+		gdev->ops->close(gdev);
+		kfifo_reset(&gdev->read_fifo);
+	}
+unlock:
+	up_write(&gdev->rwsem);
+
+	put_device(&gdev->dev);
+
+	return 0;
+}
+
+static ssize_t gnss_read(struct file *file, char __user *buf,
+		size_t count, loff_t *pos)
+{
+	struct gnss_device *gdev = file->private_data;
+	unsigned int copied;
+	int ret;
+
+	mutex_lock(&gdev->read_mutex);
+	while (kfifo_is_empty(&gdev->read_fifo)) {
+		mutex_unlock(&gdev->read_mutex);
+
+		if (gdev->disconnected)
+			return 0;
+
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(gdev->read_queue,
+				gdev->disconnected ||
+				!kfifo_is_empty(&gdev->read_fifo));
+		if (ret)
+			return -ERESTARTSYS;
+
+		mutex_lock(&gdev->read_mutex);
+	}
+
+	ret = kfifo_to_user(&gdev->read_fifo, buf, count, &copied);
+	if (ret)
+		goto out_unlock;
+
+	ret = copied;
+
+	dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, copied);
+
+out_unlock:
+	mutex_unlock(&gdev->read_mutex);
+
+	return ret;
+}
+
+static ssize_t gnss_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *pos)
+{
+	struct gnss_device *gdev = file->private_data;
+	size_t written = 0;
+	int ret;
+
+	dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, count);
+
+	if (gdev->disconnected) {
+		dev_dbg(&gdev->dev, "%s - disconnected\n", __func__);
+		return -EIO;
+	}
+
+	if (!count)
+		return 0;
+
+	if (!gdev->ops->write_raw)
+		return -EIO;
+
+	/* Ignoring O_NONBLOCK, write_raw() is synchronous. */
+
+	ret = mutex_lock_interruptible(&gdev->write_mutex);
+	if (ret)
+		return -ERESTARTSYS;
+
+	for (;;) {
+		size_t n = count - written;
+
+		if (n > GNSS_WRITE_BUF_SIZE)
+			n = GNSS_WRITE_BUF_SIZE;
+
+		if (copy_from_user(gdev->write_buf, buf, n)) {
+			ret = -EFAULT;
+			goto out_unlock;
+		}
+
+		/*
+		 * Assumes write_raw can always accept GNSS_WRITE_BUF_SIZE
+		 * bytes.
+		 *
+		 * FIXME: revisit
+		 */
+		down_read(&gdev->rwsem);
+		if (!gdev->disconnected)
+			ret = gdev->ops->write_raw(gdev, gdev->write_buf, n);
+		else
+			ret = -EIO;
+		up_read(&gdev->rwsem);
+
+		if (ret < 0)
+			break;
+
+		written += ret;
+		buf += ret;
+
+		if (written == count)
+			break;
+	}
+
+	if (written)
+		ret = written;
+out_unlock:
+	mutex_unlock(&gdev->write_mutex);
+
+	return ret;
+}
+
+static __poll_t gnss_poll(struct file *file, poll_table *wait)
+{
+	struct gnss_device *gdev = file->private_data;
+	__poll_t mask = 0;
+
+	poll_wait(file, &gdev->read_queue, wait);
+
+	if (!kfifo_is_empty(&gdev->read_fifo))
+		mask |= EPOLLIN | EPOLLRDNORM;
+	if (gdev->disconnected)
+		mask |= EPOLLHUP;
+
+	dev_dbg(&gdev->dev, "%s - mask = 0x%04x\n", __func__, mask);
+
+	return mask;
+}
+
+static const struct file_operations gnss_fops = {
+	.owner		= THIS_MODULE,
+	.open		= gnss_open,
+	.release	= gnss_release,
+	.read		= gnss_read,
+	.write		= gnss_write,
+	.poll		= gnss_poll,
+	.llseek		= no_llseek,
+};
+
+static struct class *gnss_class;
+
+static void gnss_device_release(struct device *dev)
+{
+	struct gnss_device *gdev = to_gnss_device(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	kfree(gdev->write_buf);
+	kfifo_free(&gdev->read_fifo);
+	ida_simple_remove(&gnss_minors, gdev->id);
+	kfree(gdev);
+}
+
+struct gnss_device *gnss_allocate_device(struct device *parent)
+{
+	struct gnss_device *gdev;
+	struct device *dev;
+	int id;
+	int ret;
+
+	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+	if (!gdev)
+		return NULL;
+
+	id = ida_simple_get(&gnss_minors, 0, GNSS_MINORS, GFP_KERNEL);
+	if (id < 0) {
+		kfree(gdev);
+		return ERR_PTR(id);
+	}
+
+	gdev->id = id;
+
+	dev = &gdev->dev;
+	device_initialize(dev);
+	dev->devt = gnss_first + id;
+	dev->class = gnss_class;
+	dev->parent = parent;
+	dev->release = gnss_device_release;
+	dev_set_drvdata(dev, gdev);
+	dev_set_name(dev, "gnss%d", id);
+
+	init_rwsem(&gdev->rwsem);
+	mutex_init(&gdev->read_mutex);
+	mutex_init(&gdev->write_mutex);
+	init_waitqueue_head(&gdev->read_queue);
+
+	ret = kfifo_alloc(&gdev->read_fifo, GNSS_READ_FIFO_SIZE, GFP_KERNEL);
+	if (ret)
+		goto err_put_device;
+
+	gdev->write_buf = kzalloc(GNSS_WRITE_BUF_SIZE, GFP_KERNEL);
+	if (!gdev->write_buf)
+		goto err_put_device;
+
+	cdev_init(&gdev->cdev, &gnss_fops);
+	gdev->cdev.owner = THIS_MODULE;
+
+	return gdev;
+
+err_put_device:
+	put_device(dev);
+
+	return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(gnss_allocate_device);
+
+void gnss_put_device(struct gnss_device *gdev)
+{
+	put_device(&gdev->dev);
+}
+EXPORT_SYMBOL_GPL(gnss_put_device);
+
+int gnss_register_device(struct gnss_device *gdev)
+{
+	int ret;
+
+	ret = cdev_device_add(&gdev->cdev, &gdev->dev);
+	if (ret) {
+		dev_err(&gdev->dev, "failed to add device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gnss_register_device);
+
+void gnss_deregister_device(struct gnss_device *gdev)
+{
+	down_write(&gdev->rwsem);
+	gdev->disconnected = true;
+	if (gdev->count) {
+		wake_up_interruptible(&gdev->read_queue);
+		gdev->ops->close(gdev);
+	}
+	up_write(&gdev->rwsem);
+
+	cdev_device_del(&gdev->cdev, &gdev->dev);
+}
+EXPORT_SYMBOL_GPL(gnss_deregister_device);
+
+/*
+ * Caller guarantees serialisation.
+ *
+ * Must not be called for a closed device.
+ */
+int gnss_insert_raw(struct gnss_device *gdev, const unsigned char *buf,
+		size_t count)
+{
+	int ret;
+
+	dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, count);
+
+	ret = kfifo_in(&gdev->read_fifo, buf, count);
+
+	wake_up_interruptible(&gdev->read_queue);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gnss_insert_raw);
+
+static int __init gnss_module_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&gnss_first, 0, GNSS_MINORS, "gnss");
+	if (ret < 0) {
+		pr_err("failed to allocate device numbers: %d\n", ret);
+		return ret;
+	}
+
+	gnss_class = class_create(THIS_MODULE, "gnss");
+	if (IS_ERR(gnss_class)) {
+		ret = PTR_ERR(gnss_class);
+		pr_err("failed to create class: %d\n", ret);
+		goto err_unregister_chrdev;
+	}
+
+	pr_info("GNSS driver registered with major %d\n", MAJOR(gnss_first));
+
+	return 0;
+
+err_unregister_chrdev:
+	unregister_chrdev_region(gnss_first, GNSS_MINORS);
+
+	return ret;
+}
+module_init(gnss_module_init);
+
+static void __exit gnss_module_exit(void)
+{
+	class_destroy(gnss_class);
+	unregister_chrdev_region(gnss_first, GNSS_MINORS);
+	ida_destroy(&gnss_minors);
+}
+module_exit(gnss_module_exit);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("GNSS receiver core");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/gnss.h b/include/linux/gnss.h
new file mode 100644
index 000000000000..cf69cfe4c48d
--- /dev/null
+++ b/include/linux/gnss.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GNSS receiver support
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#ifndef _LINUX_GNSS_H
+#define _LINUX_GNSS_H
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/kfifo.h>
+#include <linux/mutex.h>
+#include <linux/rwsem.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+
+struct gnss_device;
+
+struct gnss_operations {
+	int (*open)(struct gnss_device *gdev);
+	void (*close)(struct gnss_device *gdev);
+	int (*write_raw)(struct gnss_device *gdev, const unsigned char *buf,
+			size_t count);
+};
+
+struct gnss_device {
+	struct device dev;
+	struct cdev cdev;
+	int id;
+
+	struct rw_semaphore rwsem;
+	const struct gnss_operations *ops;
+	unsigned int count;
+	unsigned int disconnected:1;
+
+	struct mutex read_mutex;
+	struct kfifo read_fifo;
+	wait_queue_head_t read_queue;
+
+	struct mutex write_mutex;
+	char *write_buf;
+};
+
+struct gnss_device *gnss_allocate_device(struct device *parent);
+void gnss_put_device(struct gnss_device *gdev);
+int gnss_register_device(struct gnss_device *gdev);
+void gnss_deregister_device(struct gnss_device *gdev);
+
+int gnss_insert_raw(struct gnss_device *gdev, const unsigned char *buf,
+		size_t count);
+
+static inline void gnss_set_drvdata(struct gnss_device *gdev, void *data)
+{
+	dev_set_drvdata(&gdev->dev, data);
+}
+
+static inline void *gnss_get_drvdata(struct gnss_device *gdev)
+{
+	return dev_get_drvdata(&gdev->dev);
+}
+
+#endif /* _LINUX_GNSS_H */
-- 
2.17.0

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Greg Kroah-Hartman]

On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> +#define GNSS_MINORS	16

Why only 16?  Just have to start somewhere?

> +static DEFINE_IDA(gnss_minors);
> +static dev_t gnss_first;
> +
> +/* FIFO size must be a power of two */
> +#define GNSS_READ_FIFO_SIZE	4096
> +#define GNSS_WRITE_BUF_SIZE	1024
> +
> +#define to_gnss_device(d) container_of((d), struct gnss_device, dev)
> +
> +static int gnss_open(struct inode *inode, struct file *file)
> +{
> +	struct gnss_device *gdev;
> +	int ret = 0;
> +
> +	gdev = container_of(inode->i_cdev, struct gnss_device, cdev);
> +
> +	get_device(&gdev->dev);
> +
> +	nonseekable_open(inode, file);
> +	file->private_data = gdev;
> +
> +	down_write(&gdev->rwsem);

Just curious, why a rwsem?  They can be slower than a normal semaphore,
is this really a contentious lock?

> +	if (gdev->disconnected) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}
> +
> +	if (gdev->count++ == 0) {
> +		ret = gdev->ops->open(gdev);
> +		if (ret)
> +			gdev->count--;

Why do we care about the opens here?  Do you only want the "child
driver" to have open called once, like a tty driver?  Does it matter?

Again, just curious.


> +	}
> +unlock:
> +	up_write(&gdev->rwsem);
> +
> +	if (ret)
> +		goto err_put_device;
> +
> +	return 0;
> +
> +err_put_device:
> +	put_device(&gdev->dev);
> +
> +	return ret;

Those last 9 lines can be rewritten as just:

	if (!ret)
		put_device(&gdev->dev);

	return ret;

> +}
> +
> +static int gnss_release(struct inode *inode, struct file *file)
> +{
> +	struct gnss_device *gdev = file->private_data;
> +
> +	down_write(&gdev->rwsem);
> +	if (gdev->disconnected)
> +		goto unlock;
> +
> +	if (--gdev->count == 0) {
> +		gdev->ops->close(gdev);
> +		kfifo_reset(&gdev->read_fifo);
> +	}
> +unlock:
> +	up_write(&gdev->rwsem);
> +
> +	put_device(&gdev->dev);
> +
> +	return 0;
> +}
> +
> +static ssize_t gnss_read(struct file *file, char __user *buf,
> +		size_t count, loff_t *pos)
> +{
> +	struct gnss_device *gdev = file->private_data;
> +	unsigned int copied;
> +	int ret;
> +
> +	mutex_lock(&gdev->read_mutex);
> +	while (kfifo_is_empty(&gdev->read_fifo)) {
> +		mutex_unlock(&gdev->read_mutex);
> +
> +		if (gdev->disconnected)
> +			return 0;
> +
> +		if (file->f_flags & O_NONBLOCK)
> +			return -EAGAIN;
> +
> +		ret = wait_event_interruptible(gdev->read_queue,
> +				gdev->disconnected ||
> +				!kfifo_is_empty(&gdev->read_fifo));
> +		if (ret)
> +			return -ERESTARTSYS;
> +
> +		mutex_lock(&gdev->read_mutex);
> +	}
> +
> +	ret = kfifo_to_user(&gdev->read_fifo, buf, count, &copied);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = copied;
> +
> +	dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, copied);
> +
> +out_unlock:
> +	mutex_unlock(&gdev->read_mutex);
> +
> +	return ret;
> +}
> +
> +static ssize_t gnss_write(struct file *file, const char __user *buf,
> +		size_t count, loff_t *pos)
> +{
> +	struct gnss_device *gdev = file->private_data;
> +	size_t written = 0;
> +	int ret;
> +
> +	dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, count);

Nit, some of these initial dev_dbg() calls might be able to be removed,
as ftrace should handle the tracing code now, right?

> +
> +	if (gdev->disconnected) {
> +		dev_dbg(&gdev->dev, "%s - disconnected\n", __func__);
> +		return -EIO;
> +	}
> +
> +	if (!count)
> +		return 0;
> +
> +	if (!gdev->ops->write_raw)
> +		return -EIO;
> +
> +	/* Ignoring O_NONBLOCK, write_raw() is synchronous. */
> +
> +	ret = mutex_lock_interruptible(&gdev->write_mutex);
> +	if (ret)
> +		return -ERESTARTSYS;
> +
> +	for (;;) {
> +		size_t n = count - written;
> +
> +		if (n > GNSS_WRITE_BUF_SIZE)
> +			n = GNSS_WRITE_BUF_SIZE;
> +
> +		if (copy_from_user(gdev->write_buf, buf, n)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +
> +		/*
> +		 * Assumes write_raw can always accept GNSS_WRITE_BUF_SIZE
> +		 * bytes.
> +		 *
> +		 * FIXME: revisit
> +		 */
> +		down_read(&gdev->rwsem);
> +		if (!gdev->disconnected)
> +			ret = gdev->ops->write_raw(gdev, gdev->write_buf, n);
> +		else
> +			ret = -EIO;
> +		up_read(&gdev->rwsem);
> +
> +		if (ret < 0)
> +			break;
> +
> +		written += ret;
> +		buf += ret;
> +
> +		if (written == count)
> +			break;
> +	}
> +
> +	if (written)
> +		ret = written;
> +out_unlock:
> +	mutex_unlock(&gdev->write_mutex);
> +
> +	return ret;
> +}
> +
> +static __poll_t gnss_poll(struct file *file, poll_table *wait)
> +{
> +	struct gnss_device *gdev = file->private_data;
> +	__poll_t mask = 0;
> +
> +	poll_wait(file, &gdev->read_queue, wait);
> +
> +	if (!kfifo_is_empty(&gdev->read_fifo))
> +		mask |= EPOLLIN | EPOLLRDNORM;
> +	if (gdev->disconnected)
> +		mask |= EPOLLHUP;
> +
> +	dev_dbg(&gdev->dev, "%s - mask = 0x%04x\n", __func__, mask);
> +
> +	return mask;
> +}
> +
> +static const struct file_operations gnss_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= gnss_open,
> +	.release	= gnss_release,
> +	.read		= gnss_read,
> +	.write		= gnss_write,
> +	.poll		= gnss_poll,
> +	.llseek		= no_llseek,
> +};
> +
> +static struct class *gnss_class;
> +
> +static void gnss_device_release(struct device *dev)
> +{
> +	struct gnss_device *gdev = to_gnss_device(dev);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	kfree(gdev->write_buf);
> +	kfifo_free(&gdev->read_fifo);
> +	ida_simple_remove(&gnss_minors, gdev->id);
> +	kfree(gdev);
> +}
> +
> +struct gnss_device *gnss_allocate_device(struct device *parent)
> +{
> +	struct gnss_device *gdev;
> +	struct device *dev;
> +	int id;
> +	int ret;
> +
> +	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
> +	if (!gdev)
> +		return NULL;
> +
> +	id = ida_simple_get(&gnss_minors, 0, GNSS_MINORS, GFP_KERNEL);
> +	if (id < 0) {
> +		kfree(gdev);
> +		return ERR_PTR(id);
> +	}
> +
> +	gdev->id = id;
> +
> +	dev = &gdev->dev;
> +	device_initialize(dev);
> +	dev->devt = gnss_first + id;
> +	dev->class = gnss_class;
> +	dev->parent = parent;
> +	dev->release = gnss_device_release;
> +	dev_set_drvdata(dev, gdev);
> +	dev_set_name(dev, "gnss%d", id);
> +
> +	init_rwsem(&gdev->rwsem);
> +	mutex_init(&gdev->read_mutex);
> +	mutex_init(&gdev->write_mutex);
> +	init_waitqueue_head(&gdev->read_queue);
> +
> +	ret = kfifo_alloc(&gdev->read_fifo, GNSS_READ_FIFO_SIZE, GFP_KERNEL);
> +	if (ret)
> +		goto err_put_device;
> +
> +	gdev->write_buf = kzalloc(GNSS_WRITE_BUF_SIZE, GFP_KERNEL);
> +	if (!gdev->write_buf)
> +		goto err_put_device;
> +
> +	cdev_init(&gdev->cdev, &gnss_fops);
> +	gdev->cdev.owner = THIS_MODULE;

This protects this module from being unloaded, but how do you pass on
the module reference counts to the "child" gnss driver?  Am I just
missing that logic here somewhere?

Anyway, minor things, this looks really clean, nice job.

greg k-h

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> > +#define GNSS_MINORS	16
> 
> Why only 16?  Just have to start somewhere?

Yeah, a mostly arbitrarily picked number. I figured only developers
would have an interest in having even that many GNSS receivers in one
system. But it can be raised, now or later.

> > +static int gnss_open(struct inode *inode, struct file *file)
> > +{
> > +	struct gnss_device *gdev;
> > +	int ret = 0;
> > +
> > +	gdev = container_of(inode->i_cdev, struct gnss_device, cdev);
> > +
> > +	get_device(&gdev->dev);
> > +
> > +	nonseekable_open(inode, file);
> > +	file->private_data = gdev;
> > +
> > +	down_write(&gdev->rwsem);
> 
> Just curious, why a rwsem?  They can be slower than a normal semaphore,
> is this really a contentious lock?

I use the rwsem to deal with hotplugging; the underlying device can go
away at any time and the core makes sure that no further calls into the
corresponding driver is made once all currently executing callbacks
return.

> > +	if (gdev->disconnected) {
> > +		ret = -ENODEV;
> > +		goto unlock;
> > +	}
> > +
> > +	if (gdev->count++ == 0) {
> > +		ret = gdev->ops->open(gdev);
> > +		if (ret)
> > +			gdev->count--;
> 
> Why do we care about the opens here?  Do you only want the "child
> driver" to have open called once, like a tty driver?  Does it matter?

Exactly, core deals with the open counts so that the individual gnss
drivers need not. Serdev, for example, blows up if you try to open the
same device twice.

> Again, just curious.
> 
> > +	}
> > +unlock:
> > +	up_write(&gdev->rwsem);
> > +
> > +	if (ret)
> > +		goto err_put_device;
> > +
> > +	return 0;
> > +
> > +err_put_device:
> > +	put_device(&gdev->dev);
> > +
> > +	return ret;
> 
> Those last 9 lines can be rewritten as just:
> 
> 	if (!ret)
> 		put_device(&gdev->dev);
> 
> 	return ret;

Yeah, I usually prefer having an explicit return 0 in the success path
and clearly separate the error paths even if it adds few extra lines.
This case could become an exception though.

> > +static ssize_t gnss_write(struct file *file, const char __user *buf,
> > +		size_t count, loff_t *pos)
> > +{
> > +	struct gnss_device *gdev = file->private_data;
> > +	size_t written = 0;
> > +	int ret;
> > +
> > +	dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, count);
> 
> Nit, some of these initial dev_dbg() calls might be able to be removed,
> as ftrace should handle the tracing code now, right?

Right. The were probably mostly useful to me during development, and can
be added back later if it turns out they have any further value.

> > +static const struct file_operations gnss_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.open		= gnss_open,
> > +	.release	= gnss_release,
> > +	.read		= gnss_read,
> > +	.write		= gnss_write,
> > +	.poll		= gnss_poll,
> > +	.llseek		= no_llseek,
> > +};

> > +struct gnss_device *gnss_allocate_device(struct device *parent)
> > +{
> > +	struct gnss_device *gdev;
> > +	struct device *dev;
> > +	int id;
> > +	int ret;
> > +
> > +	gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
> > +	if (!gdev)
> > +		return NULL;
> > +
> > +	id = ida_simple_get(&gnss_minors, 0, GNSS_MINORS, GFP_KERNEL);
> > +	if (id < 0) {
> > +		kfree(gdev);
> > +		return ERR_PTR(id);
> > +	}
> > +
> > +	gdev->id = id;
> > +
> > +	dev = &gdev->dev;
> > +	device_initialize(dev);
> > +	dev->devt = gnss_first + id;
> > +	dev->class = gnss_class;
> > +	dev->parent = parent;
> > +	dev->release = gnss_device_release;
> > +	dev_set_drvdata(dev, gdev);
> > +	dev_set_name(dev, "gnss%d", id);
> > +
> > +	init_rwsem(&gdev->rwsem);
> > +	mutex_init(&gdev->read_mutex);
> > +	mutex_init(&gdev->write_mutex);
> > +	init_waitqueue_head(&gdev->read_queue);
> > +
> > +	ret = kfifo_alloc(&gdev->read_fifo, GNSS_READ_FIFO_SIZE, GFP_KERNEL);
> > +	if (ret)
> > +		goto err_put_device;
> > +
> > +	gdev->write_buf = kzalloc(GNSS_WRITE_BUF_SIZE, GFP_KERNEL);
> > +	if (!gdev->write_buf)
> > +		goto err_put_device;
> > +
> > +	cdev_init(&gdev->cdev, &gnss_fops);
> > +	gdev->cdev.owner = THIS_MODULE;
> 
> This protects this module from being unloaded, but how do you pass on
> the module reference counts to the "child" gnss driver?  Am I just
> missing that logic here somewhere?

Due to the hotplug support mentioned above, I do not need to pin the
"child" gnss driver modules. Their devices can go away at any time, be
it due to hotplugging, driver unbind through sysfs, or module unload.

> Anyway, minor things, this looks really clean, nice job.

Thanks again!

Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 12:56:45PM +0200, Johan Hovold wrote:
> On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:

> > > +static int gnss_open(struct inode *inode, struct file *file)
> > > +{
> > > +	struct gnss_device *gdev;
> > > +	int ret = 0;
> > > +
> > > +	gdev = container_of(inode->i_cdev, struct gnss_device, cdev);
> > > +
> > > +	get_device(&gdev->dev);
> > > +
> > > +	nonseekable_open(inode, file);
> > > +	file->private_data = gdev;
> > > +
> > > +	down_write(&gdev->rwsem);
> > 
> > Just curious, why a rwsem?  They can be slower than a normal semaphore,
> > is this really a contentious lock?
> 
> I use the rwsem to deal with hotplugging; the underlying device can go
> away at any time and the core makes sure that no further calls into the
> corresponding driver is made once all currently executing callbacks
> return.

I just did find one access to the gnss ops which was unsafe however; the
existence check for a write_raw callback in write() needs to be replaced
by a device flag.

Thanks,
Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Greg Kroah-Hartman]

On Wed, Apr 25, 2018 at 02:23:15PM +0200, Johan Hovold wrote:
> On Wed, Apr 25, 2018 at 12:56:45PM +0200, Johan Hovold wrote:
> > On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> 
> > > > +static int gnss_open(struct inode *inode, struct file *file)
> > > > +{
> > > > +	struct gnss_device *gdev;
> > > > +	int ret = 0;
> > > > +
> > > > +	gdev = container_of(inode->i_cdev, struct gnss_device, cdev);
> > > > +
> > > > +	get_device(&gdev->dev);
> > > > +
> > > > +	nonseekable_open(inode, file);
> > > > +	file->private_data = gdev;
> > > > +
> > > > +	down_write(&gdev->rwsem);
> > > 
> > > Just curious, why a rwsem?  They can be slower than a normal semaphore,
> > > is this really a contentious lock?
> > 
> > I use the rwsem to deal with hotplugging; the underlying device can go
> > away at any time and the core makes sure that no further calls into the
> > corresponding driver is made once all currently executing callbacks
> > return.
> 
> I just did find one access to the gnss ops which was unsafe however; the
> existence check for a write_raw callback in write() needs to be replaced
> by a device flag.

Ok, in looking at this closer, it makes more sense to me, and my worries
are not true, you are handling this properly, sorry for the noise.

I'll wait for the next resend of this series to review it again and
consider merging it.

thanks,

greg k-h

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Sun, Apr 29, 2018 at 03:35:11PM +0200, Greg Kroah-Hartman wrote:

> I'll wait for the next resend of this series to review it again and
> consider merging it.

Thanks, I'll send a v2 sometime next week.

Johan

[PATCH 2/7] dt-bindings: add generic gnss binding

[Johan Hovold]

Describe generic properties for GNSS receivers.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 .../devicetree/bindings/gnss/gnss.txt         | 36 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/gnss.txt

diff --git a/Documentation/devicetree/bindings/gnss/gnss.txt b/Documentation/devicetree/bindings/gnss/gnss.txt
new file mode 100644
index 000000000000..bcdaca043eaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/gnss.txt
@@ -0,0 +1,36 @@
+GNSS Receiver DT binding
+
+This documents the binding structure and common properties for GNSS receiver
+devices.
+
+A GNSS receiver node is a node named "gnss" and typically resides on a serial
+bus (e.g. UART, I2C or SPI).
+
+Please refer to the following documents for generic properties:
+
+	Documentation/devicetree/bindings/serial/slave-device.txt
+	Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Required Properties:
+
+- compatible 	: A string reflecting the vendor and specific device the node
+		  represents
+
+Optional Properties:
+- enable-gpios	: GPIO used to power on (or off) the device
+- timepulse-gpios	: Timepulse (e.g. 1PPS) GPIO
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "u-blox,neo-8";
+
+		vcc-supply = <&gnss_reg>;
+		timepulse-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>;
+
+		current-speed = <4800>;
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index dc3df211c1a7..fa219e80a1f8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5965,6 +5965,7 @@ F:	include/uapi/linux/gigaset_dev.h
 GNSS SUBSYSTEM
 M:	Johan Hovold <johan@kernel.org>
 S:	Maintained
+F:	Documentation/devicetree/bindings/gnss/
 F:	drivers/gnss/
 F:	include/linux/gnss.h
 
-- 
2.17.0

Re: [PATCH 2/7] dt-bindings: add generic gnss binding

[Rob Herring]

On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> Describe generic properties for GNSS receivers.
>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  .../devicetree/bindings/gnss/gnss.txt         | 36 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/gnss.txt

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

[PATCH 3/7] gnss: add generic serial driver

[Johan Hovold]

Add a generic serial GNSS driver (library) which provides a common
implementation for the gnss interface and power management (runtime and
system suspend). This allows GNSS drivers for specific chip to be
implemented by simply providing a set_power() callback to handle three
states: ACTIVE, STANDBY and OFF.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/Kconfig  |   7 +
 drivers/gnss/Makefile |   3 +
 drivers/gnss/serial.c | 288 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gnss/serial.h |  47 +++++++
 4 files changed, 345 insertions(+)
 create mode 100644 drivers/gnss/serial.c
 create mode 100644 drivers/gnss/serial.h

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index 103fcc70992e..f8ee54f99a8d 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -9,3 +9,10 @@ menuconfig GNSS
 
 	  To compile this driver as a module, choose M here: the module will
 	  be called gnss.
+
+if GNSS
+
+config GNSS_SERIAL
+	tristate
+
+endif # GNSS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index 1f7a7baab1d9..171aba71684d 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -5,3 +5,6 @@
 
 obj-$(CONFIG_GNSS)			+= gnss.o
 gnss-y := core.o
+
+obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
+gnss-serial-y := serial.o
diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
new file mode 100644
index 000000000000..06a4b511f380
--- /dev/null
+++ b/drivers/gnss/serial.c
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic serial GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+
+#include "serial.h"
+
+static int gnss_serial_open(struct gnss_device *gdev)
+{
+	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = gserial->serdev;
+	int ret;
+
+	ret = serdev_device_open(serdev);
+	if (ret)
+		return ret;
+
+	serdev_device_set_baudrate(serdev, gserial->speed);
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = pm_runtime_get_sync(&serdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&serdev->dev);
+		goto err_close;
+	}
+
+	return 0;
+
+err_close:
+	serdev_device_close(serdev);
+
+	return ret;
+}
+
+static void gnss_serial_close(struct gnss_device *gdev)
+{
+	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = gserial->serdev;
+
+	serdev_device_close(serdev);
+
+	pm_runtime_put(&serdev->dev);
+}
+
+static int gnss_serial_write_raw(struct gnss_device *gdev,
+		const unsigned char *buf, size_t count)
+{
+	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = gserial->serdev;
+	int ret;
+
+	/* write is only buffered synchronously */
+	ret = serdev_device_write(serdev, buf, count, 0);
+	if (ret < 0)
+		return ret;
+
+	/* FIXME: determine if interrupted? */
+	serdev_device_wait_until_sent(serdev, 0);
+
+	return count;
+}
+
+static const struct gnss_operations gnss_serial_gnss_ops = {
+	.open		= gnss_serial_open,
+	.close		= gnss_serial_close,
+	.write_raw	= gnss_serial_write_raw,
+};
+
+static int gnss_serial_receive_buf(struct serdev_device *serdev,
+		const unsigned char *buf, size_t count)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct gnss_device *gdev = gserial->gdev;
+
+	return gnss_insert_raw(gdev, buf, count);
+}
+
+static const struct serdev_device_ops gnss_serial_serdev_ops = {
+	.receive_buf	= gnss_serial_receive_buf,
+	.write_wakeup	= serdev_device_write_wakeup,
+};
+
+static int gnss_serial_set_power(struct gnss_serial *gserial,
+		enum gnss_serial_pm_state state)
+{
+	if (!gserial->ops || !gserial->ops->set_power)
+		return 0;
+
+	return gserial->ops->set_power(gserial, state);
+}
+
+/*
+ * FIXME: need to provide subdriver defaults or separate dt parsing from
+ * allocation.
+ */
+static int gnss_serial_parse_dt(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+	struct device_node *node = serdev->dev.of_node;
+	u32 speed = 4800;
+
+	of_property_read_u32(node, "current-speed", &speed);
+
+	gserial->speed = speed;
+
+	return 0;
+}
+
+struct gnss_serial *
+gnss_serial_allocate(struct serdev_device *serdev, size_t data_size)
+{
+	struct gnss_serial *gserial;
+	struct gnss_device *gdev;
+	int ret;
+
+	gserial = kzalloc(sizeof(*gserial) + data_size, GFP_KERNEL);
+	if (!gserial)
+		return ERR_PTR(-ENOMEM);
+
+	gdev = gnss_allocate_device(&serdev->dev);
+	if (!gdev) {
+		ret = -ENOMEM;
+		goto err_free_gserial;
+	}
+
+	gdev->ops = &gnss_serial_gnss_ops;
+	gnss_set_drvdata(gdev, gserial);
+
+	gserial->serdev = serdev;
+	gserial->gdev = gdev;
+
+	serdev_device_set_drvdata(serdev, gserial);
+	serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
+
+	ret = gnss_serial_parse_dt(serdev);
+	if (ret)
+		goto err_put_device;
+
+	return gserial;
+
+err_put_device:
+	gnss_put_device(gserial->gdev);
+err_free_gserial:
+	kfree(gserial);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(gnss_serial_allocate);
+
+void gnss_serial_free(struct gnss_serial *gserial)
+{
+	gnss_put_device(gserial->gdev);
+	kfree(gserial);
+};
+EXPORT_SYMBOL_GPL(gnss_serial_free);
+
+int gnss_serial_register(struct gnss_serial *gserial)
+{
+	struct serdev_device *serdev = gserial->serdev;
+	int ret;
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_enable(&serdev->dev);
+	} else {
+		ret = gnss_serial_set_power(gserial, GNSS_SERIAL_ACTIVE);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = gnss_register_device(gserial->gdev);
+	if (ret)
+		goto err_disable_rpm;
+
+	return 0;
+
+err_disable_rpm:
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(&serdev->dev);
+	else
+		gnss_serial_set_power(gserial, GNSS_SERIAL_OFF);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gnss_serial_register);
+
+void gnss_serial_deregister(struct gnss_serial *gserial)
+{
+	struct serdev_device *serdev = gserial->serdev;
+
+	gnss_deregister_device(gserial->gdev);
+
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(&serdev->dev);
+	else
+		gnss_serial_set_power(gserial, GNSS_SERIAL_OFF);
+}
+EXPORT_SYMBOL_GPL(gnss_serial_deregister);
+
+#ifdef CONFIG_PM
+static int gnss_serial_runtime_suspend(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return gnss_serial_set_power(gserial, GNSS_SERIAL_STANDBY);
+}
+
+static int gnss_serial_runtime_resume(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	return gnss_serial_set_power(gserial, GNSS_SERIAL_ACTIVE);
+}
+#endif /* CONFIG_PM */
+
+static int gnss_serial_prepare(struct device *dev)
+{
+	dev_dbg(dev, "%s - pm_runtime_suspended = %d\n", __func__,
+			pm_runtime_suspended(dev));
+
+	if (pm_runtime_suspended(dev))
+		return 1;
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int gnss_serial_suspend(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dev_dbg(dev, "%s - pm_runtime_suspended = %d\n", __func__,
+			pm_runtime_suspended(dev));
+
+	/*
+	 * FIXME: serdev currently lacks support for managing the underlying
+	 * device's wakeup settings. A workaround would be to close the serdev
+	 * device here if it is open.
+	 */
+
+	if (!pm_runtime_suspended(dev))
+		ret = gnss_serial_set_power(gserial, GNSS_SERIAL_STANDBY);
+
+	return ret;
+}
+
+static int gnss_serial_resume(struct device *dev)
+{
+	struct gnss_serial *gserial = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dev_dbg(dev, "%s - pm_runtime_suspended = %d\n", __func__,
+			pm_runtime_suspended(dev));
+
+	if (!pm_runtime_suspended(dev))
+		ret = gnss_serial_set_power(gserial, GNSS_SERIAL_ACTIVE);
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+const struct dev_pm_ops gnss_serial_pm_ops = {
+	.prepare	= gnss_serial_prepare,
+	SET_SYSTEM_SLEEP_PM_OPS(gnss_serial_suspend, gnss_serial_resume)
+	SET_RUNTIME_PM_OPS(gnss_serial_runtime_suspend, gnss_serial_runtime_resume, NULL)
+};
+EXPORT_SYMBOL_GPL(gnss_serial_pm_ops);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("Generic serial GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gnss/serial.h b/drivers/gnss/serial.h
new file mode 100644
index 000000000000..01e7573bc473
--- /dev/null
+++ b/drivers/gnss/serial.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Generic serial GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#ifndef _LINUX_GNSS_SERIAL_H
+#define _LINUX_GNSS_SERIAL_H
+
+#include <asm/termbits.h>
+#include <linux/pm.h>
+
+struct gnss_serial {
+	struct serdev_device *serdev;
+	struct gnss_device *gdev;
+	speed_t	speed;
+	const struct gnss_serial_ops *ops;
+	unsigned long drvdata[0];
+};
+
+enum gnss_serial_pm_state {
+	GNSS_SERIAL_OFF,
+	GNSS_SERIAL_ACTIVE,
+	GNSS_SERIAL_STANDBY,
+};
+
+struct gnss_serial_ops {
+	int (*set_power)(struct gnss_serial *gserial,
+			enum gnss_serial_pm_state state);
+};
+
+extern const struct dev_pm_ops gnss_serial_pm_ops;
+
+struct gnss_serial *gnss_serial_allocate(struct serdev_device *gserial,
+		size_t data_size);
+void gnss_serial_free(struct gnss_serial *gserial);
+
+int gnss_serial_register(struct gnss_serial *gserial);
+void gnss_serial_deregister(struct gnss_serial *gserial);
+
+static inline void *gnss_serial_get_drvdata(struct gnss_serial *gserial)
+{
+	return &gserial[1];
+}
+
+#endif /* _LINUX_GNSS_SERIAL_H */
-- 
2.17.0

Re: [PATCH 3/7] gnss: add generic serial driver

[Greg Kroah-Hartman]

On Tue, Apr 24, 2018 at 06:34:54PM +0200, Johan Hovold wrote:
> Add a generic serial GNSS driver (library) which provides a common
> implementation for the gnss interface and power management (runtime and
> system suspend). This allows GNSS drivers for specific chip to be
> implemented by simply providing a set_power() callback to handle three
> states: ACTIVE, STANDBY and OFF.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/gnss/Kconfig  |   7 +
>  drivers/gnss/Makefile |   3 +
>  drivers/gnss/serial.c | 288 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/gnss/serial.h |  47 +++++++
>  4 files changed, 345 insertions(+)
>  create mode 100644 drivers/gnss/serial.c
>  create mode 100644 drivers/gnss/serial.h
> 
> diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
> index 103fcc70992e..f8ee54f99a8d 100644
> --- a/drivers/gnss/Kconfig
> +++ b/drivers/gnss/Kconfig
> @@ -9,3 +9,10 @@ menuconfig GNSS
>  
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called gnss.
> +
> +if GNSS
> +
> +config GNSS_SERIAL
> +	tristate
> +

Maybe a real help entry?  Or is this only selected from child drivers?
I haven't gotten that far in the series, sorry...

Re: [PATCH 3/7] gnss: add generic serial driver

[Johan Hovold]

On Wed, Apr 25, 2018 at 10:57:49AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 24, 2018 at 06:34:54PM +0200, Johan Hovold wrote:
> > Add a generic serial GNSS driver (library) which provides a common
> > implementation for the gnss interface and power management (runtime and
> > system suspend). This allows GNSS drivers for specific chip to be
> > implemented by simply providing a set_power() callback to handle three
> > states: ACTIVE, STANDBY and OFF.
> > 
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/gnss/Kconfig  |   7 +
> >  drivers/gnss/Makefile |   3 +
> >  drivers/gnss/serial.c | 288 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/gnss/serial.h |  47 +++++++
> >  4 files changed, 345 insertions(+)
> >  create mode 100644 drivers/gnss/serial.c
> >  create mode 100644 drivers/gnss/serial.h
> > 
> > diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
> > index 103fcc70992e..f8ee54f99a8d 100644
> > --- a/drivers/gnss/Kconfig
> > +++ b/drivers/gnss/Kconfig
> > @@ -9,3 +9,10 @@ menuconfig GNSS
> >  
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called gnss.
> > +
> > +if GNSS
> > +
> > +config GNSS_SERIAL
> > +	tristate
> > +
> 
> Maybe a real help entry?  Or is this only selected from child drivers?
> I haven't gotten that far in the series, sorry...

Correct, it's only selected by child drivers.

Johan

Re: [PATCH 3/7] gnss: add generic serial driver

[Greg Kroah-Hartman]

On Tue, Apr 24, 2018 at 06:34:54PM +0200, Johan Hovold wrote:
> +static inline void *gnss_serial_get_drvdata(struct gnss_serial *gserial)
> +{
> +	return &gserial[1];
> +}

Oh that's the best hack I have seen in a long time.  I need to remember
that one for next time.

Anyway, in reading this driver, I don't think the module reference
counting is being handled here, so you might want to add that to the
gnss core to keep things from going south if modules are unloaded.

thanks,

greg k-h

Re: [PATCH 3/7] gnss: add generic serial driver

[Johan Hovold]

On Wed, Apr 25, 2018 at 11:00:31AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 24, 2018 at 06:34:54PM +0200, Johan Hovold wrote:
> > +static inline void *gnss_serial_get_drvdata(struct gnss_serial *gserial)
> > +{
> > +	return &gserial[1];
> > +}
> 
> Oh that's the best hack I have seen in a long time.  I need to remember
> that one for next time.
> 
> Anyway, in reading this driver, I don't think the module reference
> counting is being handled here, so you might want to add that to the
> gnss core to keep things from going south if modules are unloaded.

As I just mentioned in my reply to your comment about module references
in core, I think I got this covered through the hotplug support. Devices
can go away at anytime, including through a module unload.

Thanks,
Johan

[PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

Add binding for u-blox GNSS receivers.

Note that the u-blox product names encodes form factor (e.g. "neo"),
chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
chipset is used for the compatible strings (for now).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 .../devicetree/bindings/gnss/u-blox.txt       | 31 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt   |  1 +
 2 files changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt

diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
new file mode 100644
index 000000000000..bb54b83a177f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
@@ -0,0 +1,31 @@
+u-blox GNSS Receiver DT binding
+
+The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
+
+Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
+properties.
+
+Required Properties:
+
+- compatible 	: Must be one of
+
+			"u-blox,neo-8"
+			"u-blox,neo-m8"
+
+- vcc-supply	: Main voltage regulator (VCC)
+
+Optional Properties:
+
+- timepulse-gpios	: Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "u-blox,neo-8";
+
+		vcc-supply = <&gnss_reg>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b5f978a4cac6..2128dfdf73f1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -374,6 +374,7 @@ tronsmart	Tronsmart
 truly	Truly Semiconductors Limited
 tsd	Theobroma Systems Design und Consulting GmbH
 tyan	Tyan Computer Corporation
+u-blox	u-blox
 ucrobotics	uCRobotics
 ubnt	Ubiquiti Networks
 udoo	Udoo
-- 
2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> Add binding for u-blox GNSS receivers.
>
> Note that the u-blox product names encodes form factor (e.g. "neo"),
> chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> chipset is used for the compatible strings (for now).
>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  .../devicetree/bindings/gnss/u-blox.txt       | 31 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>  2 files changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>
> diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
> new file mode 100644
> index 000000000000..bb54b83a177f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> @@ -0,0 +1,31 @@
> +u-blox GNSS Receiver DT binding
> +
> +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> +
> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> +properties.
> +
> +Required Properties:
> +
> +- compatible   : Must be one of
> +
> +                       "u-blox,neo-8"
> +                       "u-blox,neo-m8"
> +
> +- vcc-supply   : Main voltage regulator (VCC)

What about V_BCKP?

> +
> +Optional Properties:

reg is required if using I2C, SPI, or USB.

Datasheet also shows an interrupt pin.

> +
> +- timepulse-gpios      : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)

Why the 3rd "TIMEPULSE"?

> +
> +Example:
> +
> +serial@1234 {
> +       compatible = "ns16550a";
> +
> +       gnss {
> +               compatible = "u-blox,neo-8";
> +
> +               vcc-supply = <&gnss_reg>;
> +       };
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index b5f978a4cac6..2128dfdf73f1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -374,6 +374,7 @@ tronsmart   Tronsmart
>  truly  Truly Semiconductors Limited
>  tsd    Theobroma Systems Design und Consulting GmbH
>  tyan   Tyan Computer Corporation
> +u-blox u-blox
>  ucrobotics     uCRobotics
>  ubnt   Ubiquiti Networks
>  udoo   Udoo
> --
> 2.17.0
>

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> > Add binding for u-blox GNSS receivers.
> >
> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> > chipset is used for the compatible strings (for now).
> >
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  .../devicetree/bindings/gnss/u-blox.txt       | 31 +++++++++++++++++++
> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >  2 files changed, 32 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > new file mode 100644
> > index 000000000000..bb54b83a177f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > @@ -0,0 +1,31 @@
> > +u-blox GNSS Receiver DT binding
> > +
> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> > +
> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> > +properties.
> > +
> > +Required Properties:
> > +
> > +- compatible   : Must be one of
> > +
> > +                       "u-blox,neo-8"
> > +                       "u-blox,neo-m8"
> > +
> > +- vcc-supply   : Main voltage regulator (VCC)
> 
> What about V_BCKP?

That's the backup supply for for the RTC and batter-backed RAM. In
configurations where a battery is not used it should be connected to
VCC.

How would you model that? I can enable a vbckp regulator at probe, but
what if someone then accurately describes the corresponding pin as being
connected to VCC? I guess we can check if the regulators are identical,
and then just have the driver ignore V_BKUP. Knowing whether there is
a (hopefully charged) battery connected could be useful.

I can't seem to find any other bindings that describe battery supplies,
but I'll add it here.

> > +
> > +Optional Properties:
> 
> reg is required if using I2C, SPI, or USB.

I'll add that (even if there is no driver support for these yet).

> Datasheet also shows an interrupt pin.

Not used currently, but I'll add it to the binding as well.

> > +
> > +- timepulse-gpios      : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
> 
> Why the 3rd "TIMEPULSE"?

That's the pin name, which in this case is identical to the property
name, so I'll drop it here.

Take a look at the sirf binding; vendors use different names for their
timepulse pins and in that case I added the actual pin names (1PPS, TM)
in parenthesis after the description.

Note that I mentioned "timepulse-gpios" in the generic binding with the
intent of trying to enforce a generic name for pins with such a
function (similarly for "enable-gpios", which I guess is already
established).

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
>> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
>> > Add binding for u-blox GNSS receivers.
>> >
>> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> > chipset is used for the compatible strings (for now).
>> >
>> > Signed-off-by: Johan Hovold <johan@kernel.org>
>> > ---
>> >  .../devicetree/bindings/gnss/u-blox.txt       | 31 +++++++++++++++++++
>> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >  2 files changed, 32 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > new file mode 100644
>> > index 000000000000..bb54b83a177f
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > @@ -0,0 +1,31 @@
>> > +u-blox GNSS Receiver DT binding
>> > +
>> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
>> > +
>> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> > +properties.
>> > +
>> > +Required Properties:
>> > +
>> > +- compatible   : Must be one of
>> > +
>> > +                       "u-blox,neo-8"
>> > +                       "u-blox,neo-m8"
>> > +
>> > +- vcc-supply   : Main voltage regulator (VCC)
>>
>> What about V_BCKP?
>
> That's the backup supply for for the RTC and batter-backed RAM. In
> configurations where a battery is not used it should be connected to
> VCC.
>
> How would you model that? I can enable a vbckp regulator at probe, but
> what if someone then accurately describes the corresponding pin as being
> connected to VCC?

You mean how to model a battery? It would just be a 'regulator'
because the regulator binding covers any supply really.

Then you just set both rails to the same supply phandle.

> I guess we can check if the regulators are identical,
> and then just have the driver ignore V_BKUP. Knowing whether there is
> a (hopefully charged) battery connected could be useful.

Regulators are ref counted, so just enable it twice. Or the driver can
just ignore it until it supports battery backup.

> I can't seem to find any other bindings that describe battery supplies,
> but I'll add it here.

There must be some. But then the battery and charger bindings are not
in the best shape.

>> > +
>> > +Optional Properties:
>>
>> reg is required if using I2C, SPI, or USB.
>
> I'll add that (even if there is no driver support for these yet).
>
>> Datasheet also shows an interrupt pin.
>
> Not used currently, but I'll add it to the binding as well.
>
>> > +
>> > +- timepulse-gpios      : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
>>
>> Why the 3rd "TIMEPULSE"?
>
> That's the pin name, which in this case is identical to the property
> name, so I'll drop it here.

Then what is the 2nd "Timepulse"?

Maybe just a "pin name: X" prefix so it is clear.

> Take a look at the sirf binding; vendors use different names for their
> timepulse pins and in that case I added the actual pin names (1PPS, TM)
> in parenthesis after the description.
>
> Note that I mentioned "timepulse-gpios" in the generic binding with the
> intent of trying to enforce a generic name for pins with such a
> function (similarly for "enable-gpios", which I guess is already
> established).

Yes, I think that's good.

Though with the enable-gpios I was debating the name for sirfstar a
bit because it isn't the normal drive it active to enable, but rather
a pulse to enable or disable.

Rob

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> >> > Add binding for u-blox GNSS receivers.
> >> >
> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> >> > chipset is used for the compatible strings (for now).
> >> >
> >> > Signed-off-by: Johan Hovold <johan@kernel.org>
> >> > ---
> >> >  .../devicetree/bindings/gnss/u-blox.txt       | 31 +++++++++++++++++++
> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >> >  2 files changed, 32 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > new file mode 100644
> >> > index 000000000000..bb54b83a177f
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > @@ -0,0 +1,31 @@
> >> > +u-blox GNSS Receiver DT binding
> >> > +
> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> >> > +
> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> >> > +properties.
> >> > +
> >> > +Required Properties:
> >> > +
> >> > +- compatible   : Must be one of
> >> > +
> >> > +                       "u-blox,neo-8"
> >> > +                       "u-blox,neo-m8"
> >> > +
> >> > +- vcc-supply   : Main voltage regulator (VCC)
> >>
> >> What about V_BCKP?
> >
> > That's the backup supply for for the RTC and batter-backed RAM. In
> > configurations where a battery is not used it should be connected to
> > VCC.
> >
> > How would you model that? I can enable a vbckp regulator at probe, but
> > what if someone then accurately describes the corresponding pin as being
> > connected to VCC?
> 
> You mean how to model a battery? It would just be a 'regulator'
> because the regulator binding covers any supply really.
> 
> Then you just set both rails to the same supply phandle.

Yes, but...

> > I guess we can check if the regulators are identical,
> > and then just have the driver ignore V_BKUP. Knowing whether there is
> > a (hopefully charged) battery connected could be useful.
> 
> Regulators are ref counted, so just enable it twice. Or the driver can
> just ignore it until it supports battery backup.

...my point was that in case there's no backup battery, you don't want
to enable vcc (via v_bckp) at probe (and instead have the device cold
boot whenever it's used).

Hence, the driver would need to check if the v_bckp-supply is identical
to vcc and not enable the former at probe in that case (i.e. similar to
if no v_bckp had been specified and we considered it optional).

> >> > +- timepulse-gpios      : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
> >>
> >> Why the 3rd "TIMEPULSE"?
> >
> > That's the pin name, which in this case is identical to the property
> > name, so I'll drop it here.
> 
> Then what is the 2nd "Timepulse"?

That's the generic function name.

> Maybe just a "pin name: X" prefix so it is clear.

For u-blox devices, where property-, function- and pin name coincide, I
could just change this to:

+- timepulse-gpios      : Timepulse GPIO

and then for the sirfstar binding, which will be used by devices from
multiple vendors which have decided to name their pins differently, I
can add a "pin name: " prefix for clarity?

> > Take a look at the sirf binding; vendors use different names for their
> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
> > in parenthesis after the description.
> >
> > Note that I mentioned "timepulse-gpios" in the generic binding with the
> > intent of trying to enforce a generic name for pins with such a
> > function (similarly for "enable-gpios", which I guess is already
> > established).
> 
> Yes, I think that's good.
> 
> Though with the enable-gpios I was debating the name for sirfstar a
> bit because it isn't the normal drive it active to enable, but rather
> a pulse to enable or disable.

I had some concerns along those lines as well, and if you agree I'll
change the property name to on_off-gpios (or onoff-gpios) which matches
the vendor data sheets for this pin, and which I think would be better.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Wed, May 2, 2018 at 3:16 AM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
>> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold <johan@kernel.org> wrote:
>> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
>> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
>> >> > Add binding for u-blox GNSS receivers.
>> >> >
>> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> >> > chipset is used for the compatible strings (for now).
>> >> >
>> >> > Signed-off-by: Johan Hovold <johan@kernel.org>
>> >> > ---
>> >> >  .../devicetree/bindings/gnss/u-blox.txt       | 31 +++++++++++++++++++
>> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >> >  2 files changed, 32 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > new file mode 100644
>> >> > index 000000000000..bb54b83a177f
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > @@ -0,0 +1,31 @@
>> >> > +u-blox GNSS Receiver DT binding
>> >> > +
>> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
>> >> > +
>> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> >> > +properties.
>> >> > +
>> >> > +Required Properties:
>> >> > +
>> >> > +- compatible   : Must be one of
>> >> > +
>> >> > +                       "u-blox,neo-8"
>> >> > +                       "u-blox,neo-m8"
>> >> > +
>> >> > +- vcc-supply   : Main voltage regulator (VCC)
>> >>
>> >> What about V_BCKP?
>> >
>> > That's the backup supply for for the RTC and batter-backed RAM. In
>> > configurations where a battery is not used it should be connected to
>> > VCC.
>> >
>> > How would you model that? I can enable a vbckp regulator at probe, but
>> > what if someone then accurately describes the corresponding pin as being
>> > connected to VCC?
>>
>> You mean how to model a battery? It would just be a 'regulator'
>> because the regulator binding covers any supply really.
>>
>> Then you just set both rails to the same supply phandle.
>
> Yes, but...
>
>> > I guess we can check if the regulators are identical,
>> > and then just have the driver ignore V_BKUP. Knowing whether there is
>> > a (hopefully charged) battery connected could be useful.
>>
>> Regulators are ref counted, so just enable it twice. Or the driver can
>> just ignore it until it supports battery backup.
>
> ...my point was that in case there's no backup battery, you don't want
> to enable vcc (via v_bckp) at probe (and instead have the device cold
> boot whenever it's used).

Wouldn't that result in very long acquisition times? I guess I was
thinking Vcc would be always on when running and V_bckp was just for
suspend.

> Hence, the driver would need to check if the v_bckp-supply is identical
> to vcc and not enable the former at probe in that case (i.e. similar to
> if no v_bckp had been specified and we considered it optional).

I guess if that's the intended operation, then making it optional is fine.

Rob

>
>> >> > +- timepulse-gpios      : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
>> >>
>> >> Why the 3rd "TIMEPULSE"?
>> >
>> > That's the pin name, which in this case is identical to the property
>> > name, so I'll drop it here.
>>
>> Then what is the 2nd "Timepulse"?
>
> That's the generic function name.
>
>> Maybe just a "pin name: X" prefix so it is clear.
>
> For u-blox devices, where property-, function- and pin name coincide, I
> could just change this to:
>
> +- timepulse-gpios      : Timepulse GPIO
>
> and then for the sirfstar binding, which will be used by devices from
> multiple vendors which have decided to name their pins differently, I
> can add a "pin name: " prefix for clarity?

Sounds good.

>> > Take a look at the sirf binding; vendors use different names for their
>> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
>> > in parenthesis after the description.
>> >
>> > Note that I mentioned "timepulse-gpios" in the generic binding with the
>> > intent of trying to enforce a generic name for pins with such a
>> > function (similarly for "enable-gpios", which I guess is already
>> > established).
>>
>> Yes, I think that's good.
>>
>> Though with the enable-gpios I was debating the name for sirfstar a
>> bit because it isn't the normal drive it active to enable, but rather
>> a pulse to enable or disable.
>
> I had some concerns along those lines as well, and if you agree I'll
> change the property name to on_off-gpios (or onoff-gpios) which matches
> the vendor data sheets for this pin, and which I think would be better.

Okay, just add a vendor prefix. And note that using '_' is discouraged.

Rob

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, May 02, 2018 at 08:16:11AM -0500, Rob Herring wrote:
> On Wed, May 2, 2018 at 3:16 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
> >> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold <johan@kernel.org> wrote:
> >> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> >> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> >> >> > Add binding for u-blox GNSS receivers.

> >> >> > +Required Properties:
> >> >> > +
> >> >> > +- compatible   : Must be one of
> >> >> > +
> >> >> > +                       "u-blox,neo-8"
> >> >> > +                       "u-blox,neo-m8"
> >> >> > +
> >> >> > +- vcc-supply   : Main voltage regulator (VCC)
> >> >>
> >> >> What about V_BCKP?

> > ...my point was that in case there's no backup battery, you don't want
> > to enable vcc (via v_bckp) at probe (and instead have the device cold
> > boot whenever it's used).
> 
> Wouldn't that result in very long acquisition times? I guess I was
> thinking Vcc would be always on when running and V_bckp was just for
> suspend.

Yes, the acquisition times would certainly be longer, but for a GNSS
receiver which is rarely used that might still be preferred. And since
I'm using runtime pm to manage the supply, this policy decision can be
deferred to user space and controlled through the power/control
attribute.

> > Hence, the driver would need to check if the v_bckp-supply is identical
> > to vcc and not enable the former at probe in that case (i.e. similar to
> > if no v_bckp had been specified and we considered it optional).
> 
> I guess if that's the intended operation, then making it optional is fine.

Ok.

> >> > Take a look at the sirf binding; vendors use different names for their
> >> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
> >> > in parenthesis after the description.
> >> >
> >> > Note that I mentioned "timepulse-gpios" in the generic binding with the
> >> > intent of trying to enforce a generic name for pins with such a
> >> > function (similarly for "enable-gpios", which I guess is already
> >> > established).
> >>
> >> Yes, I think that's good.
> >>
> >> Though with the enable-gpios I was debating the name for sirfstar a
> >> bit because it isn't the normal drive it active to enable, but rather
> >> a pulse to enable or disable.
> >
> > I had some concerns along those lines as well, and if you agree I'll
> > change the property name to on_off-gpios (or onoff-gpios) which matches
> > the vendor data sheets for this pin, and which I think would be better.
> 
> Okay, just add a vendor prefix. And note that using '_' is discouraged.

Ok, I'll name it "sirf,onoff-gpios".

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

> Am 02.05.2018 um 10:16 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
>> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold <johan@kernel.org> wrote:
>> 
>> Though with the enable-gpios I was debating the name for sirfstar a
>> bit because it isn't the normal drive it active to enable, but rather
>> a pulse to enable or disable.
> 
> I had some concerns along those lines as well, and if you agree I'll
> change the property name to on_off-gpios (or onoff-gpios) which matches
> the vendor data sheets for this pin, and which I think would be better.

In our original submission of our w2sg bindings we also did call that
"on-off-gpios", but Rob suggested to use the standard way of "enable-gpios":

https://patchwork.kernel.org/patch/9738981/

We changed this and got his acked-by for our w2sg bindings:

https://patchwork.kernel.org/patch/10054911/

We also have found an agreed wording for the description:

- enable-gpios:  the GPIO that controls the module's power toggle
         input. A positive impulse of sufficent length toggles the
         power state.

But what should I expect from someone who constantly ignores, rejects
and belittles previous work of others and prefers to reinvent almost
the same thing and claims that it is 100% original work?

I am so heavily upset this time because we had been asked by reviewers if
it was not Neil Brown who laid the foundation and yes, we of course looked
how to formulate proper attribution:

https://patchwork.kernel.org/patch/10059705/

I stand to my view that this is a good example how to ruin the willingness
of volunteers to send submissions for review, since the paycheck a volunteer
receives uses the currency of being welcome, recognition and visibility in
the final git log.

And I had expected that reviewers from kernel.org are more generous/liberal
with code flaws (finding them is IMHO the sole purpose of the review process)
and don't expect that everyone has the same background.

Anyways, let's forget this type of discussion and come back to a technical
level:

I have realized that the w2sg0004 is an exception (although a Sirf chip)
that it does not provide a WAKEUP signal. And another significant
difference is that we have to keep the serdev UART enabled even if there
is no user-space client. Otherwise we are not able to detect unexpected
activity. So we unfortunately can't move serdev open/close into the .open
and .close ops but need to open it in probe.

Therefore, it is in my opinion still better to have a separate driver for
the w2sg0004 instead of hacking the support of this chip into your WAKEUP
capable sirfstar driver. So I suggest that you make WAKEUP a required
property.

We had faced a comparable decision last year with the ov9650 and ov9655 camera
sensors which are almost the same. But not same enough to integrate both into
a single driver.

So (despite my grown doubts about the review process), I'll now submit a v7
of our w2sg0004 driver, adjusted to your gnss framework API and tested on
GTA04 hardware. It is only covering the w2sg0004 module and not even claiming
to handle the w2sg0084(i) to avoid overlap with your work.

BR,
Nikolaus

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Thu, 3 May 2018 11:35:21 +0200
H. Nikolaus Schaller <hns@goldelico.com> wrote:


> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> that it does not provide a WAKEUP signal. And another significant
> difference is that we have to keep the serdev UART enabled even if there
> is no user-space client. Otherwise we are not able to detect unexpected
> activity. So we unfortunately can't move serdev open/close into the .open
> and .close ops but need to open it in probe.
> 
how much power does it use to keep the uart enabled? Or should it
better be reprogrammed as gpio?

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

Hi Andreas,

> Am 03.05.2018 um 20:50 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> On Thu, 3 May 2018 11:35:21 +0200
> H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
> 
>> I have realized that the w2sg0004 is an exception (although a Sirf chip)
>> that it does not provide a WAKEUP signal. And another significant
>> difference is that we have to keep the serdev UART enabled even if there
>> is no user-space client. Otherwise we are not able to detect unexpected
>> activity. So we unfortunately can't move serdev open/close into the .open
>> and .close ops but need to open it in probe.
>> 
> how much power does it use to keep the uart enabled? Or should it
> better be reprogrammed as gpio?

I think it does not need much more (if at all) than a gpio controller on
the OMAP3 chip (I think the clocks are active anyways for use by the other
UARTs).

We had proposed years ago to reprogram the UART RX pin by pinmux-states
into an interrupt gpio but that was rejected because it was not general
enough and ugly in the device tree (an rx-gpios record where the rx-line
is already connected to the UART-rx).

Then we did experiment with tapping the UART driver and finally the
serdev API was developed to solve this problem. Hence we use it now this
way.

BR and thanks,
Nikolaus



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

[+cc Tony]

Hi,

On Fri, May 04, 2018 at 07:16:00AM +0200, H. Nikolaus Schaller wrote:
> > Am 03.05.2018 um 20:50 schrieb Andreas Kemnade <andreas@kemnade.info>:
> > On Thu, 3 May 2018 11:35:21 +0200
> > H. Nikolaus Schaller <hns@goldelico.com> wrote:
> > 
> >> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> >> that it does not provide a WAKEUP signal. And another significant
> >> difference is that we have to keep the serdev UART enabled even if there
> >> is no user-space client. Otherwise we are not able to detect unexpected
> >> activity. So we unfortunately can't move serdev open/close into the .open
> >> and .close ops but need to open it in probe.
> >> 
> > how much power does it use to keep the uart enabled? Or should it
> > better be reprogrammed as gpio?
> 
> I think it does not need much more (if at all) than a gpio controller on
> the OMAP3 chip (I think the clocks are active anyways for use by the other
> UARTs).
>
> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> into an interrupt gpio but that was rejected because it was not general
> enough and ugly in the device tree (an rx-gpios record where the rx-line
> is already connected to the UART-rx).
> 
> Then we did experiment with tapping the UART driver and finally the
> serdev API was developed to solve this problem. Hence we use it now this
> way.

Having any UART active on OMAP results in the SoC not entering
idle/off wasting energy. For normal (i.e. not connected to a peripheral)
TTYs you can enable runtime autosuspend and configure the RX pin as
wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
will lose the first few characters (since the serial device needs some
time to wakeup). This is for example supported by the N900 uart3
(debug uart):

$ grep -A4 "&uart3 {" arch/arm/boot/dts/omap3-n900.dts 
&uart3 {
	interrupts-extended = <&intc 74 &omap3_pmx_core OMAP3_UART3_RX>;
	pinctrl-names = "default";
	pinctrl-0 = <&uart3_pins>;
};

To get it working, you also need to enable autosuspend for the tty
in userspace (echo 3000 /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
This is not enabled by default due to the character loss characteristic
during wakeup.

Having said all of this, serdev does not yet support runtime PM (at
all). Tony is currently looking into it. Fortunately serdev allows
us to enable runtime PM by default (once implemented), since we know
the remote side and can (hopefully) avoid losing characters (i.e.
with sideband wakeup gpios).

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

Hi Sebastian,

> Am 04.05.2018 um 13:42 schrieb Sebastian Reichel <sre@kernel.org>:
> 
> [+cc Tony]
> 
> Hi,
> 
>> I think it does not need much more (if at all) than a gpio controller on
>> the OMAP3 chip (I think the clocks are active anyways for use by the other
>> UARTs).
>> 
>> We had proposed years ago to reprogram the UART RX pin by pinmux-states
>> into an interrupt gpio but that was rejected because it was not general
>> enough and ugly in the device tree (an rx-gpios record where the rx-line
>> is already connected to the UART-rx).
>> 
>> Then we did experiment with tapping the UART driver and finally the
>> serdev API was developed to solve this problem. Hence we use it now this
>> way.
> 
> Having any UART active on OMAP results in the SoC not entering
> idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> TTYs you can enable runtime autosuspend and configure the RX pin as
> wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> will lose the first few characters (since the serial device needs some
> time to wakeup). This is for example supported by the N900 uart3
> (debug uart):
> 
> $ grep -A4 "&uart3 {" arch/arm/boot/dts/omap3-n900.dts 
> &uart3 {
> 	interrupts-extended = <&intc 74 &omap3_pmx_core OMAP3_UART3_RX>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&uart3_pins>;
> };
> 
> To get it working, you also need to enable autosuspend for the tty
> in userspace (echo 3000 /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> This is not enabled by default due to the character loss characteristic
> during wakeup.
> 
> Having said all of this, serdev does not yet support runtime PM (at
> all). Tony is currently looking into it. Fortunately serdev allows
> us to enable runtime PM by default (once implemented), since we know
> the remote side and can (hopefully) avoid losing characters (i.e.
> with sideband wakeup gpios).

thanks for explaining this! We originally had similar thoughts when
proposing a w2sg0004 driver for the first time (years ago), but we can
not accept loosing characters...

Now, I could imagine a potential solution. The key situation why we keep
the serdev open and listening is if the driver did try to turn the module
off, but in fact did turn it on (because it was not in sync).

It should be possible to cover this by a timer that is started
in this case. If there is serdev data received after assuming the module
is turned off, the driver has detected the wrong case - and can safely
close the serdev until we want to have it powered on again.

If there is no response after turing off, the module power state is already
in sync and we can close the serdev as well - after the timeout (let's say
30 seconds). Then, the serdev UART can idle. We should open the serdev
and start this timer also in the probe function to catch an initially wrong
state.

But I think we should focus on the basics of this driver first. Then it can
be optimized later on.

BR and thanks,
Nikolaus

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]

Hi,

On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:
> > Am 04.05.2018 um 13:42 schrieb Sebastian Reichel <sre@kernel.org>:
> >> I think it does not need much more (if at all) than a gpio controller on
> >> the OMAP3 chip (I think the clocks are active anyways for use by the other
> >> UARTs).
> >> 
> >> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> >> into an interrupt gpio but that was rejected because it was not general
> >> enough and ugly in the device tree (an rx-gpios record where the rx-line
> >> is already connected to the UART-rx).
> >> 
> >> Then we did experiment with tapping the UART driver and finally the
> >> serdev API was developed to solve this problem. Hence we use it now this
> >> way.
> > 
> > Having any UART active on OMAP results in the SoC not entering
> > idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> > TTYs you can enable runtime autosuspend and configure the RX pin as
> > wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> > will lose the first few characters (since the serial device needs some
> > time to wakeup). This is for example supported by the N900 uart3
> > (debug uart):
> > 
> > $ grep -A4 "&uart3 {" arch/arm/boot/dts/omap3-n900.dts 
> > &uart3 {
> > 	interrupts-extended = <&intc 74 &omap3_pmx_core OMAP3_UART3_RX>;
> > 	pinctrl-names = "default";
> > 	pinctrl-0 = <&uart3_pins>;
> > };
> > 
> > To get it working, you also need to enable autosuspend for the tty
> > in userspace (echo 3000 /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> > This is not enabled by default due to the character loss characteristic
> > during wakeup.
> > 
> > Having said all of this, serdev does not yet support runtime PM (at
> > all). Tony is currently looking into it. Fortunately serdev allows
> > us to enable runtime PM by default (once implemented), since we know
> > the remote side and can (hopefully) avoid losing characters (i.e.
> > with sideband wakeup gpios).
> 
> thanks for explaining this! We originally had similar thoughts when
> proposing a w2sg0004 driver for the first time (years ago), but we can
> not accept loosing characters...
> 
> Now, I could imagine a potential solution. The key situation why we keep
> the serdev open and listening is if the driver did try to turn the module
> off, but in fact did turn it on (because it was not in sync).
>
> It should be possible to cover this by a timer that is started
> in this case. If there is serdev data received after assuming the module
> is turned off, the driver has detected the wrong case - and can safely
> close the serdev until we want to have it powered on again.
> 
> If there is no response after turing off, the module power state is already
> in sync and we can close the serdev as well - after the timeout (let's say
> 30 seconds). Then, the serdev UART can idle. We should open the serdev
> and start this timer also in the probe function to catch an initially wrong
> state.

That sounds like a good plan.

> But I think we should focus on the basics of this driver first. Then it can
> be optimized later on.

Definitely.

-- Sebastian.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Sebastian Reichel <sre@kernel.org> [180504 13:40]:
> Hi,
> 
> On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:
> > > Am 04.05.2018 um 13:42 schrieb Sebastian Reichel <sre@kernel.org>:
> > >> I think it does not need much more (if at all) than a gpio controller on
> > >> the OMAP3 chip (I think the clocks are active anyways for use by the other
> > >> UARTs).
> > >> 
> > >> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> > >> into an interrupt gpio but that was rejected because it was not general
> > >> enough and ugly in the device tree (an rx-gpios record where the rx-line
> > >> is already connected to the UART-rx).
> > >> 
> > >> Then we did experiment with tapping the UART driver and finally the
> > >> serdev API was developed to solve this problem. Hence we use it now this
> > >> way.
> > > 
> > > Having any UART active on OMAP results in the SoC not entering
> > > idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> > > TTYs you can enable runtime autosuspend and configure the RX pin as
> > > wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> > > will lose the first few characters (since the serial device needs some
> > > time to wakeup). This is for example supported by the N900 uart3
> > > (debug uart):
> > > 
> > > $ grep -A4 "&uart3 {" arch/arm/boot/dts/omap3-n900.dts 
> > > &uart3 {
> > > 	interrupts-extended = <&intc 74 &omap3_pmx_core OMAP3_UART3_RX>;
> > > 	pinctrl-names = "default";
> > > 	pinctrl-0 = <&uart3_pins>;
> > > };
> > > 
> > > To get it working, you also need to enable autosuspend for the tty
> > > in userspace (echo 3000 /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> > > This is not enabled by default due to the character loss characteristic
> > > during wakeup.
> > > 
> > > Having said all of this, serdev does not yet support runtime PM (at
> > > all). Tony is currently looking into it. Fortunately serdev allows
> > > us to enable runtime PM by default (once implemented), since we know
> > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > with sideband wakeup gpios).
> > 
> > thanks for explaining this! We originally had similar thoughts when
> > proposing a w2sg0004 driver for the first time (years ago), but we can
> > not accept loosing characters...
> > 
> > Now, I could imagine a potential solution. The key situation why we keep
> > the serdev open and listening is if the driver did try to turn the module
> > off, but in fact did turn it on (because it was not in sync).
> >
> > It should be possible to cover this by a timer that is started
> > in this case. If there is serdev data received after assuming the module
> > is turned off, the driver has detected the wrong case - and can safely
> > close the serdev until we want to have it powered on again.
> > 
> > If there is no response after turing off, the module power state is already
> > in sync and we can close the serdev as well - after the timeout (let's say
> > 30 seconds). Then, the serdev UART can idle. We should open the serdev
> > and start this timer also in the probe function to catch an initially wrong
> > state.
> 
> That sounds like a good plan.

I don't know the details, but sounds like you should might be able to
just the timer that comes with pm_runtime_use_autosuspend() :)

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:

> It should be possible to cover this by a timer that is started
> in this case. If there is serdev data received after assuming the module
> is turned off, the driver has detected the wrong case - and can safely
> close the serdev until we want to have it powered on again.
> 
> If there is no response after turing off, the module power state is already
> in sync and we can close the serdev as well - after the timeout (let's say
> 30 seconds). Then, the serdev UART can idle. We should open the serdev
> and start this timer also in the probe function to catch an initially wrong
> state.

Right, the only thing that worried me about that was that we cannot
really delay system suspend for 30 seconds even if a somewhat shorter
delay should be probably be sufficient (still a number of seconds).

Configuring the serial controller as a wakeup source which aborts
suspend or resumes if the driver gets it wrong might be preferred to
draining the battery in suspend however.

Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:

> Having said all of this, serdev does not yet support runtime PM (at
> all). Tony is currently looking into it. Fortunately serdev allows
> us to enable runtime PM by default (once implemented), since we know
> the remote side and can (hopefully) avoid losing characters (i.e.
> with sideband wakeup gpios).

I'm not sure we want generic runtime-pm support for the controllers in
the sense that the slave device state is always reflected by the serial
controller. Similar as for i2c and spi, we really only want to keep the
controller active when we are doing I/O, but we may want to keep a
client active for longer.

Take the u-blox driver in this series for example. As I'm using runtime
PM to manage device power, user-space can chose to prevent the receiver
from runtime suspending in order to avoid lengthy (re-)acquisition times
in setups without a backup battery (by means of the power/control
attribute).

Note that serdev not enabling runtime pm for controllers is roughly
equivalent to setting the .ignore_children flag, which is what we do for
i2c and spi controller, and possibly what we want here too.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

Hi,

* Johan Hovold <johan@kernel.org> [180507 03:03]:
> On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> 
> > Having said all of this, serdev does not yet support runtime PM (at
> > all). Tony is currently looking into it. Fortunately serdev allows
> > us to enable runtime PM by default (once implemented), since we know
> > the remote side and can (hopefully) avoid losing characters (i.e.
> > with sideband wakeup gpios).
> 
> I'm not sure we want generic runtime-pm support for the controllers in
> the sense that the slave device state is always reflected by the serial
> controller. Similar as for i2c and spi, we really only want to keep the
> controller active when we are doing I/O, but we may want to keep a
> client active for longer.

Yeah i2c seems to do the right thing where the bus takes care
of runtime PM.

> Take the u-blox driver in this series for example. As I'm using runtime
> PM to manage device power, user-space can chose to prevent the receiver
> from runtime suspending in order to avoid lengthy (re-)acquisition times
> in setups without a backup battery (by means of the power/control
> attribute).

Sorry I don't seem to have that one, care to paste the subject
line of that patch?

> Note that serdev not enabling runtime pm for controllers is roughly
> equivalent to setting the .ignore_children flag, which is what we do for
> i2c and spi controller, and possibly what we want here too.

We currently don't idle serdev at all even if not in use. What
I noticed is if I have these in my .config:

CONFIG_SERIAL_DEV_BUS=y
CONFIG_SERIAL_DEV_CTRL_TTYPORT=y

And no hci_serdev.ko driver loaded, then the 8250 port still stays
active and there are no sysfs entries to idle it.

Are you seeing this with your series?

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> Hi,
> 
> * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > 
> > > Having said all of this, serdev does not yet support runtime PM (at
> > > all). Tony is currently looking into it. Fortunately serdev allows
> > > us to enable runtime PM by default (once implemented), since we know
> > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > with sideband wakeup gpios).
> > 
> > I'm not sure we want generic runtime-pm support for the controllers in
> > the sense that the slave device state is always reflected by the serial
> > controller. Similar as for i2c and spi, we really only want to keep the
> > controller active when we are doing I/O, but we may want to keep a
> > client active for longer.
> 
> Yeah i2c seems to do the right thing where the bus takes care
> of runtime PM.

Yeah, but since serial is async in contrast to i2c/spi, we may not be
able to push this entirely into core. The serdev drivers may need to
indicate when they expect or need to do I/O by opening and closing the
port. And this is how I implemented these first couple of gnss drivers.

Also note that most serial driver do not do runtime pm while the port is
open (as OMAP does), and doing so also has the drawbacks of lost
characters etc. as Sebastian mentioned.

> > Take the u-blox driver in this series for example. As I'm using runtime
> > PM to manage device power, user-space can chose to prevent the receiver
> > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > in setups without a backup battery (by means of the power/control
> > attribute).
> 
> Sorry I don't seem to have that one, care to paste the subject
> line of that patch?

	"[PATCH 5/7] gnss: add driver for u-blox receivers"

	https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org

> > Note that serdev not enabling runtime pm for controllers is roughly
> > equivalent to setting the .ignore_children flag, which is what we do for
> > i2c and spi controller, and possibly what we want here too.
> 
> We currently don't idle serdev at all even if not in use. What
> I noticed is if I have these in my .config:
> 
> CONFIG_SERIAL_DEV_BUS=y
> CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> 
> And no hci_serdev.ko driver loaded, then the 8250 port still stays
> active and there are no sysfs entries to idle it.

Sounds like the 8250_omap driver is doing something funky. Why would
there not be any sysfs attributes to control runtime pm?

> Are you seeing this with your series?

I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
runtime pm at probe by setting a negative autosuspend timeout.

Changing this through sysfs causes the serial controller to runtime
suspend, but something is not right in my setup as it doesn't wake up on
incoming data.

I'd say the omap drivers are broken; the controller should definitely
idle when the port is closed (whether using serdev or not) without
having to fiddle with sysfs.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180507 16:36]:
> On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > Hi,
> > 
> > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > 
> > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > us to enable runtime PM by default (once implemented), since we know
> > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > with sideband wakeup gpios).
> > > 
> > > I'm not sure we want generic runtime-pm support for the controllers in
> > > the sense that the slave device state is always reflected by the serial
> > > controller. Similar as for i2c and spi, we really only want to keep the
> > > controller active when we are doing I/O, but we may want to keep a
> > > client active for longer.
> > 
> > Yeah i2c seems to do the right thing where the bus takes care
> > of runtime PM.
> 
> Yeah, but since serial is async in contrast to i2c/spi, we may not be
> able to push this entirely into core. The serdev drivers may need to
> indicate when they expect or need to do I/O by opening and closing the
> port. And this is how I implemented these first couple of gnss drivers.

OK

> Also note that most serial driver do not do runtime pm while the port is
> open (as OMAP does), and doing so also has the drawbacks of lost
> characters etc. as Sebastian mentioned.

Yes serdev seems really nice for the oob wake gpios :)

> > > Take the u-blox driver in this series for example. As I'm using runtime
> > > PM to manage device power, user-space can chose to prevent the receiver
> > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > in setups without a backup battery (by means of the power/control
> > > attribute).
> > 
> > Sorry I don't seem to have that one, care to paste the subject
> > line of that patch?
> 
> 	"[PATCH 5/7] gnss: add driver for u-blox receivers"
> 
> 	https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org

Thanks will take a look.

> > > Note that serdev not enabling runtime pm for controllers is roughly
> > > equivalent to setting the .ignore_children flag, which is what we do for
> > > i2c and spi controller, and possibly what we want here too.
> > 
> > We currently don't idle serdev at all even if not in use. What
> > I noticed is if I have these in my .config:
> > 
> > CONFIG_SERIAL_DEV_BUS=y
> > CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> > 
> > And no hci_serdev.ko driver loaded, then the 8250 port still stays
> > active and there are no sysfs entries to idle it.
> 
> Sounds like the 8250_omap driver is doing something funky. Why would
> there not be any sysfs attributes to control runtime pm?

I don't know, they are there for the ports that don't have any
serdev device. But if there is a serdev child node, the sysfs
disappear for the 8250 port like it's /dev/ttyS* entry. That is
even with no hci_serdev.ko loaded :)

> > Are you seeing this with your series?
> 
> I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
> runtime pm at probe by setting a negative autosuspend timeout.

Hmm I though we now have both 8250_omap and serial-omap behave the
same way for PM.

> Changing this through sysfs causes the serial controller to runtime
> suspend, but something is not right in my setup as it doesn't wake up on
> incoming data.

Do you have also a /dev/ttyO* entry created for the serdev port?

> I'd say the omap drivers are broken; the controller should definitely
> idle when the port is closed (whether using serdev or not) without
> having to fiddle with sysfs.

This is happening for the non-serdev ports for sure. FYI, there is
one patch needed for omap4 to idle unused ports that I posted
few days ago:

[PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts

But the serdev port is never idled, even if unused.

Can you check your serdev port clkctrl reg with rwmem or similar
tool when it's idle?

You can do it with:

rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
rwmem -s32 0x44e00038           # uart 6 on l4_per

And here's what I have on my bbb with 8250_omap:

0x44e004b4 = 0x00000002
0x44e0006c = 0x00030000
0x44e00070 = 0x00030000
0x44e00074 = 0x00030000
0x44e00078 = 0x00030000
0x44e00038 = 0x00030000

So all disabled except for the console UART.

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Mon, May 07, 2018 at 10:50:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180507 16:36]:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:

> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> > > 
> > > We currently don't idle serdev at all even if not in use. What
> > > I noticed is if I have these in my .config:
> > > 
> > > CONFIG_SERIAL_DEV_BUS=y
> > > CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> > > 
> > > And no hci_serdev.ko driver loaded, then the 8250 port still stays
> > > active and there are no sysfs entries to idle it.
> > 
> > Sounds like the 8250_omap driver is doing something funky. Why would
> > there not be any sysfs attributes to control runtime pm?
> 
> I don't know, they are there for the ports that don't have any
> serdev device. But if there is a serdev child node, the sysfs
> disappear for the 8250 port like it's /dev/ttyS* entry. That is
> even with no hci_serdev.ko loaded :)

It sounds to me like you have a udev rule that's matching on TTY devices
and setting an autosuspend timeout that allows the controller to runtime
suspend. Is that so?

With serdev such a rule would no longer be sufficient as it would no
longer configure all controllers. You can always set the autosuspend
directly through the platform device node, for example:

	/sys/bus/platform/devices/481aa000.serial

But the point is, we really don't want the runtime PM behaviour to be
dependent on the presence of such rules. The serial controllers should
always be idle when not in use (unless overridden by user space).

> > > Are you seeing this with your series?
> > 
> > I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
> > runtime pm at probe by setting a negative autosuspend timeout.
> 
> Hmm I though we now have both 8250_omap and serial-omap behave the
> same way for PM.

It looks like they've been implemented the same way (e.g. the negative
autosuspend timeout).

> > Changing this through sysfs causes the serial controller to runtime
> > suspend, but something is not right in my setup as it doesn't wake up on
> > incoming data.
> 
> Do you have also a /dev/ttyO* entry created for the serdev port?

No, serdev claims the port and no tty device is created.

> > I'd say the omap drivers are broken; the controller should definitely
> > idle when the port is closed (whether using serdev or not) without
> > having to fiddle with sysfs.
> 
> This is happening for the non-serdev ports for sure. FYI, there is
> one patch needed for omap4 to idle unused ports that I posted
> few days ago:
> 
> [PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts
> 
> But the serdev port is never idled, even if unused.

With the negative autosuspend set in both omap drivers probe functions,
this is the expected behaviour. Which I think we must fix.

> Can you check your serdev port clkctrl reg with rwmem or similar
> tool when it's idle?
> 
> You can do it with:
> 
> rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
> rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
> rwmem -s32 0x44e00038           # uart 6 on l4_per
> 
> And here's what I have on my bbb with 8250_omap:
> 
> 0x44e004b4 = 0x00000002
> 0x44e0006c = 0x00030000
> 0x44e00070 = 0x00030000
> 0x44e00074 = 0x00030000
> 0x44e00078 = 0x00030000
> 0x44e00038 = 0x00030000
> 
> So all disabled except for the console UART.

On BBB with omap-serial I have:

0x44e004b4 = 0x2	(uart 1, console, open)
0x44e0006c = 0x2	(uart 2, serdev, closed)
0x44e00070 = 0x30000	(uart 3, disabled in DT)
0x44e00074 = 0x30000	(uart 4, disabled in DT)
0x44e00078 = 0x2	(uart 5, serdev, closed)
0x44e00038 = 0x2	(uart 6, ttyO5, closed)

So no enabled and closed port is idle, whether using serdev or not.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180508 07:00]:
> On Mon, May 07, 2018 at 10:50:32AM -0700, Tony Lindgren wrote:
> > I don't know, they are there for the ports that don't have any
> > serdev device. But if there is a serdev child node, the sysfs
> > disappear for the 8250 port like it's /dev/ttyS* entry. That is
> > even with no hci_serdev.ko loaded :)
> 
> It sounds to me like you have a udev rule that's matching on TTY devices
> and setting an autosuspend timeout that allows the controller to runtime
> suspend. Is that so?

Oh you are right, I do have an old init script that idles the UARTs.

And that script is looking for tty[SO]*. Updating it to use
/sys/bus/platform/devices fixes the issue I was seeing:

uarts=$(find /sys/bus/platform/devices/4*.serial/power/ -type d)
for uart in $uarts; do
	echo -n 1000 > $uart/autosuspend_delay_ms
	echo -n enabled > $uart/wakeup
	echo -n auto > $uart/control
done

The above also idles the console so beware though.

> But the point is, we really don't want the runtime PM behaviour to be
> dependent on the presence of such rules. The serial controllers should
> always be idle when not in use (unless overridden by user space).

I agree, and that seems broken right now.

> > Do you have also a /dev/ttyO* entry created for the serdev port?
> 
> No, serdev claims the port and no tty device is created.

And this is no longer the issue when using /sys/bus/platform/devices
instead of tty[SO]* in my init script :)

> > > I'd say the omap drivers are broken; the controller should definitely
> > > idle when the port is closed (whether using serdev or not) without
> > > having to fiddle with sysfs.
> > 
> > This is happening for the non-serdev ports for sure. FYI, there is
> > one patch needed for omap4 to idle unused ports that I posted
> > few days ago:
> > 
> > [PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts
> > 
> > But the serdev port is never idled, even if unused.
> 
> With the negative autosuspend set in both omap drivers probe functions,
> this is the expected behaviour. Which I think we must fix.

Yes indeed. I've been using my script for years now and have
completely missed the fact that the unused ports are not idled
at all on start-up when unused.

> > Can you check your serdev port clkctrl reg with rwmem or similar
> > tool when it's idle?
> > 
> > You can do it with:
> > 
> > rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
> > rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
> > rwmem -s32 0x44e00038           # uart 6 on l4_per
> > 
> > And here's what I have on my bbb with 8250_omap:
> > 
> > 0x44e004b4 = 0x00000002
> > 0x44e0006c = 0x00030000
> > 0x44e00070 = 0x00030000
> > 0x44e00074 = 0x00030000
> > 0x44e00078 = 0x00030000
> > 0x44e00038 = 0x00030000
> > 
> > So all disabled except for the console UART.
> 
> On BBB with omap-serial I have:
> 
> 0x44e004b4 = 0x2	(uart 1, console, open)
> 0x44e0006c = 0x2	(uart 2, serdev, closed)
> 0x44e00070 = 0x30000	(uart 3, disabled in DT)
> 0x44e00074 = 0x30000	(uart 4, disabled in DT)
> 0x44e00078 = 0x2	(uart 5, serdev, closed)
> 0x44e00038 = 0x2	(uart 6, ttyO5, closed)
> 
> So no enabled and closed port is idle, whether using serdev or not.

OK so we really have currently just one PM related bug where
unused UARTs should be idled on init. And then the serdev PM
can be handled the way you're doing it and tested with the
above rwmem.

Hmm maybe all we need for serdev runtime PM is to set the parent
UART autoidle timeout to immediate (0 instead of -1) from the serdev
child driver to allow the serdev driver manage the PM.

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180508 08:22]:
> * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > With the negative autosuspend set in both omap drivers probe functions,
> > this is the expected behaviour. Which I think we must fix.
> 
> Yes indeed. I've been using my script for years now and have
> completely missed the fact that the unused ports are not idled
> at all on start-up when unused.

This might be all that's needed, care to try it and if it works
I'll send out two separate proper patches?

I'm seeing this now on my bbb after temporarily disabling my
UART idle init script:

# rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
0x44e004b4 = 0x00000002

# rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
0x44e0006c = 0x00030000
0x44e00070 = 0x00030000
0x44e00074 = 0x00030000
0x44e00078 = 0x00030000

# rwmem -s32 0x44e00038           # uart 6 on l4_per
0x44e00038 = 0x00030000

Regards,

Tony

8< ----------------------
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -685,8 +685,7 @@ static void omap_8250_shutdown(struct uart_port *port)
 		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
 	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
-	pm_runtime_mark_last_busy(port->dev);
-	pm_runtime_put_autosuspend(port->dev);
+	pm_runtime_put_sync(port->dev);
 	free_irq(port->irq, port);
 	dev_pm_clear_wake_irq(port->dev);
 }
@@ -1265,8 +1264,7 @@ static int omap8250_probe(struct platform_device *pdev)
 	}
 	priv->line = ret;
 	platform_set_drvdata(pdev, priv);
-	pm_runtime_mark_last_busy(&pdev->dev);
-	pm_runtime_put_autosuspend(&pdev->dev);
+	pm_runtime_put_sync(&pdev->dev);
 	return 0;
 err:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -821,8 +821,7 @@ static void serial_omap_shutdown(struct uart_port *port)
 	if (serial_in(up, UART_LSR) & UART_LSR_DR)
 		(void) serial_in(up, UART_RX);
 
-	pm_runtime_mark_last_busy(up->dev);
-	pm_runtime_put_autosuspend(up->dev);
+	pm_runtime_put_sync(up->dev);
 	free_irq(up->port.irq, up);
 	dev_pm_clear_wake_irq(up->dev);
 }
@@ -1751,8 +1750,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 	if (ret != 0)
 		goto err_add_port;
 
-	pm_runtime_mark_last_busy(up->dev);
-	pm_runtime_put_autosuspend(up->dev);
+	pm_runtime_put_sync(&pdev->dev);
 	return 0;
 
 err_add_port:
-- 
2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180508 08:47]:
> * Tony Lindgren <tony@atomide.com> [180508 08:22]:
> > * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > > With the negative autosuspend set in both omap drivers probe functions,
> > > this is the expected behaviour. Which I think we must fix.
> > 
> > Yes indeed. I've been using my script for years now and have
> > completely missed the fact that the unused ports are not idled
> > at all on start-up when unused.
> 
> This might be all that's needed, care to try it and if it works
> I'll send out two separate proper patches?
> 
> I'm seeing this now on my bbb after temporarily disabling my
> UART idle init script:
> 
> # rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
> 0x44e004b4 = 0x00000002
> 
> # rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
> 0x44e0006c = 0x00030000
> 0x44e00070 = 0x00030000
> 0x44e00074 = 0x00030000
> 0x44e00078 = 0x00030000
> 
> # rwmem -s32 0x44e00038           # uart 6 on l4_per
> 0x44e00038 = 0x00030000

Hmm I forgot to remove status = "disabled" from the other ports,
still not idling the unused ports with the patch below it seems.

Regards,

Tony

> 8< ----------------------
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -685,8 +685,7 @@ static void omap_8250_shutdown(struct uart_port *port)
>  		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>  	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> -	pm_runtime_mark_last_busy(port->dev);
> -	pm_runtime_put_autosuspend(port->dev);
> +	pm_runtime_put_sync(port->dev);
>  	free_irq(port->irq, port);
>  	dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1265,8 +1264,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  	}
>  	priv->line = ret;
>  	platform_set_drvdata(pdev, priv);
> -	pm_runtime_mark_last_busy(&pdev->dev);
> -	pm_runtime_put_autosuspend(&pdev->dev);
> +	pm_runtime_put_sync(&pdev->dev);
>  	return 0;
>  err:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -821,8 +821,7 @@ static void serial_omap_shutdown(struct uart_port *port)
>  	if (serial_in(up, UART_LSR) & UART_LSR_DR)
>  		(void) serial_in(up, UART_RX);
>  
> -	pm_runtime_mark_last_busy(up->dev);
> -	pm_runtime_put_autosuspend(up->dev);
> +	pm_runtime_put_sync(up->dev);
>  	free_irq(up->port.irq, up);
>  	dev_pm_clear_wake_irq(up->dev);
>  }
> @@ -1751,8 +1750,7 @@ static int serial_omap_probe(struct platform_device *pdev)
>  	if (ret != 0)
>  		goto err_add_port;
>  
> -	pm_runtime_mark_last_busy(up->dev);
> -	pm_runtime_put_autosuspend(up->dev);
> +	pm_runtime_put_sync(&pdev->dev);
>  	return 0;
>  
>  err_add_port:
> -- 
> 2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180508 08:54]:
> * Tony Lindgren <tony@atomide.com> [180508 08:47]:
> > * Tony Lindgren <tony@atomide.com> [180508 08:22]:
> > > * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > this is the expected behaviour. Which I think we must fix.
> > > 
> > > Yes indeed. I've been using my script for years now and have
> > > completely missed the fact that the unused ports are not idled
> > > at all on start-up when unused.
> > 
> > This might be all that's needed, care to try it and if it works
> > I'll send out two separate proper patches?
> > 
> > I'm seeing this now on my bbb after temporarily disabling my
> > UART idle init script:
> > 
> > # rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
> > 0x44e004b4 = 0x00000002
> > 
> > # rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
> > 0x44e0006c = 0x00030000
> > 0x44e00070 = 0x00030000
> > 0x44e00074 = 0x00030000
> > 0x44e00078 = 0x00030000
> > 
> > # rwmem -s32 0x44e00038           # uart 6 on l4_per
> > 0x44e00038 = 0x00030000
> 
> Hmm I forgot to remove status = "disabled" from the other ports,
> still not idling the unused ports with the patch below it seems.

No need to enable/disable autosuspend except in startup and shutdown
I think. The patch below works for me, now includes removal of the
status = "disabled" flags too. Only tested with 8250_omap on bbb
so far.

I wonder if other places still need fixing for autosuspend
like console support?

Regards,

Tony

8< -------------------
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -347,7 +347,6 @@
 			clock-frequency = <48000000>;
 			reg = <0x44e09000 0x2000>;
 			interrupts = <72>;
-			status = "disabled";
 			dmas = <&edma 26 0>, <&edma 27 0>;
 			dma-names = "tx", "rx";
 		};
@@ -358,7 +357,6 @@
 			clock-frequency = <48000000>;
 			reg = <0x48022000 0x2000>;
 			interrupts = <73>;
-			status = "disabled";
 			dmas = <&edma 28 0>, <&edma 29 0>;
 			dma-names = "tx", "rx";
 		};
@@ -369,7 +367,6 @@
 			clock-frequency = <48000000>;
 			reg = <0x48024000 0x2000>;
 			interrupts = <74>;
-			status = "disabled";
 			dmas = <&edma 30 0>, <&edma 31 0>;
 			dma-names = "tx", "rx";
 		};
@@ -380,7 +377,6 @@
 			clock-frequency = <48000000>;
 			reg = <0x481a6000 0x2000>;
 			interrupts = <44>;
-			status = "disabled";
 		};
 
 		uart4: serial@481a8000 {
@@ -389,7 +385,6 @@
 			clock-frequency = <48000000>;
 			reg = <0x481a8000 0x2000>;
 			interrupts = <45>;
-			status = "disabled";
 		};
 
 		uart5: serial@481aa000 {
@@ -398,7 +393,6 @@
 			clock-frequency = <48000000>;
 			reg = <0x481aa000 0x2000>;
 			interrupts = <46>;
-			status = "disabled";
 		};
 
 		i2c0: i2c@44e0b000 {
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
 			return ret;
 	}
 
+	pm_runtime_use_autosuspend(port->dev);
 	pm_runtime_get_sync(port->dev);
 
 	up->mcr = 0;
@@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
 		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
 	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
-	pm_runtime_mark_last_busy(port->dev);
-	pm_runtime_put_autosuspend(port->dev);
+	pm_runtime_dont_use_autosuspend(port->dev);
+	pm_runtime_put_sync(port->dev);
 	free_irq(port->irq, port);
 	dev_pm_clear_wake_irq(port->dev);
 }
@@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
 	spin_lock_init(&priv->rx_dma_lock);
 
 	device_init_wakeup(&pdev->dev, true);
-	pm_runtime_use_autosuspend(&pdev->dev);
+	/* See omap_8250_startup and shutdown for autosuspend */
 	pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 
 	pm_runtime_irq_safe(&pdev->dev);
@@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
 	}
 	priv->line = ret;
 	platform_set_drvdata(pdev, priv);
-	pm_runtime_mark_last_busy(&pdev->dev);
-	pm_runtime_put_autosuspend(&pdev->dev);
+	pm_runtime_put_sync(&pdev->dev);
+
 	return 0;
 err:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -740,6 +740,7 @@ static int serial_omap_startup(struct uart_port *port)
 
 	dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->port.line);
 
+	pm_runtime_use_autosuspend(up->dev);
 	pm_runtime_get_sync(up->dev);
 	/*
 	 * Clear the FIFO buffers and disable them.
@@ -821,8 +822,8 @@ static void serial_omap_shutdown(struct uart_port *port)
 	if (serial_in(up, UART_LSR) & UART_LSR_DR)
 		(void) serial_in(up, UART_RX);
 
-	pm_runtime_mark_last_busy(up->dev);
-	pm_runtime_put_autosuspend(up->dev);
+	pm_runtime_dont_use_autosuspend(up->dev);
+	pm_runtime_put_sync(up->dev);
 	free_irq(up->port.irq, up);
 	dev_pm_clear_wake_irq(up->dev);
 }
@@ -1733,7 +1734,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 		omap_up_info->autosuspend_timeout = -1;
 
 	device_init_wakeup(up->dev, true);
-	pm_runtime_use_autosuspend(&pdev->dev);
+	/* See serial_omap_startup and shutdown for autosuspend */
 	pm_runtime_set_autosuspend_delay(&pdev->dev,
 			omap_up_info->autosuspend_timeout);
 
@@ -1751,8 +1752,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 	if (ret != 0)
 		goto err_add_port;
 
-	pm_runtime_mark_last_busy(up->dev);
-	pm_runtime_put_autosuspend(up->dev);
+	pm_runtime_put_sync(&pdev->dev);
 	return 0;
 
 err_add_port:

OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Updating subject and adding linux-serial, linux-pm and linux-omap. ]

On Tue, May 08, 2018 at 09:49:04AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180508 08:54]:
> > * Tony Lindgren <tony@atomide.com> [180508 08:47]:
> > > * Tony Lindgren <tony@atomide.com> [180508 08:22]:
> > > > * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > > this is the expected behaviour. Which I think we must fix.
> > > > 
> > > > Yes indeed. I've been using my script for years now and have
> > > > completely missed the fact that the unused ports are not idled
> > > > at all on start-up when unused.
> > > 
> > > This might be all that's needed, care to try it and if it works
> > > I'll send out two separate proper patches?
> > > 
> > > I'm seeing this now on my bbb after temporarily disabling my
> > > UART idle init script:
> > > 
> > > # rwmem -s32 0x44e004b4           # uart 1 on l4_wkup
> > > 0x44e004b4 = 0x00000002
> > > 
> > > # rwmem -s32 0x44e0006c+0x10      # uart 2 - 5 on l4_per
> > > 0x44e0006c = 0x00030000
> > > 0x44e00070 = 0x00030000
> > > 0x44e00074 = 0x00030000
> > > 0x44e00078 = 0x00030000
> > > 
> > > # rwmem -s32 0x44e00038           # uart 6 on l4_per
> > > 0x44e00038 = 0x00030000
> > 
> > Hmm I forgot to remove status = "disabled" from the other ports,
> > still not idling the unused ports with the patch below it seems.
> 
> No need to enable/disable autosuspend except in startup and shutdown
> I think. The patch below works for me, now includes removal of the
> status = "disabled" flags too. Only tested with 8250_omap on bbb
> so far.
> 
> I wonder if other places still need fixing for autosuspend
> like console support?

While this seems to fix the idling of closed ports here too, I'm not
sure we can move use_autosuspend to startup() like this.

First, this flag should be set before registering the tty so that udev
can be used to update the attributes.

Second, this prevents setting the autosuspend delay through sysfs when
the port is closed (when autosuspend is disabled).

It seems we really should not be using the negative autosuspend to
configure the RPM behaviour the way these drivers do. Perhaps a new
mechanism is needed.

But I'm afraid I don't have time to look at this today.

Thanks,
Johan

> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
>  			return ret;
>  	}
>  
> +	pm_runtime_use_autosuspend(port->dev);
>  	pm_runtime_get_sync(port->dev);
>  
>  	up->mcr = 0;
> @@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
>  		serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>  	serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> -	pm_runtime_mark_last_busy(port->dev);
> -	pm_runtime_put_autosuspend(port->dev);
> +	pm_runtime_dont_use_autosuspend(port->dev);
> +	pm_runtime_put_sync(port->dev);
>  	free_irq(port->irq, port);
>  	dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  	spin_lock_init(&priv->rx_dma_lock);
>  
>  	device_init_wakeup(&pdev->dev, true);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> +	/* See omap_8250_startup and shutdown for autosuspend */
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>  
>  	pm_runtime_irq_safe(&pdev->dev);
> @@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
>  	}
>  	priv->line = ret;
>  	platform_set_drvdata(pdev, priv);
> -	pm_runtime_mark_last_busy(&pdev->dev);
> -	pm_runtime_put_autosuspend(&pdev->dev);
> +	pm_runtime_put_sync(&pdev->dev);
> +
>  	return 0;
>  err:

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180509 13:12]:
> While this seems to fix the idling of closed ports here too, I'm not
> sure we can move use_autosuspend to startup() like this.
> 
> First, this flag should be set before registering the tty so that udev
> can be used to update the attributes.
> 
> Second, this prevents setting the autosuspend delay through sysfs when
> the port is closed (when autosuspend is disabled).

Yeah I noticed that too yesterday.

> It seems we really should not be using the negative autosuspend to
> configure the RPM behaviour the way these drivers do. Perhaps a new
> mechanism is needed.

Hmm well simply defaulting to "on" instead of "auto" and setting the
autosuspend_ms to 3000 by default might be doable. I think that way
we can keep use_autosuspend() in probe. Let's hope there are no
existing use cases that would break with that.

> But I'm afraid I don't have time to look at this today.

Sure np thanks,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Sorry about the late reply. ]

On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180509 13:12]:

> > It seems we really should not be using the negative autosuspend to
> > configure the RPM behaviour the way these drivers do. Perhaps a new
> > mechanism is needed.
> 
> Hmm well simply defaulting to "on" instead of "auto" and setting the
> autosuspend_ms to 3000 by default might be doable. I think that way
> we can keep use_autosuspend() in probe. Let's hope there are no
> existing use cases that would break with that.

No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
either as that would also prevent the device from runtime suspending
just as the current negative autosuspend delay does.

I fail to see how we can implement this using the current toolbox. What
you're after here is really a mechanism for selecting between two
different runtime PM schemes at runtime:

	1. normal serial RPM, where the controller is active while the
	   port is open (this should be the safe default)

	2. aggressive serial RPM, where the controller is allowed to
	   suspend while the port is open even though this may result in
	   lost characters when waking up on incoming data

For normal ttys, we need a user-space interface for selecting between
the two, and for serdev we may want a way to select the RPM scheme from
within the kernel.

Note that with my serdev controller runtime PM patch, serdev core could
always opt for aggressive PM (as by default serdev core holds an RPM
reference for the controller while the port is open).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180517 10:12]:
> [ Sorry about the late reply. ]
> 
> On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180509 13:12]:
> 
> > > It seems we really should not be using the negative autosuspend to
> > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > mechanism is needed.
> > 
> > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > autosuspend_ms to 3000 by default might be doable. I think that way
> > we can keep use_autosuspend() in probe. Let's hope there are no
> > existing use cases that would break with that.
> 
> No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> either as that would also prevent the device from runtime suspending
> just as the current negative autosuspend delay does.

Well in that case we should just stick with -1 value for the
autosuspend. And just do pm_runtime_put_sync_suspend() after
probe and on close.

> I fail to see how we can implement this using the current toolbox. What
> you're after here is really a mechanism for selecting between two
> different runtime PM schemes at runtime:
> 
> 	1. normal serial RPM, where the controller is active while the
> 	   port is open (this should be the safe default)

Agreed. And that is the case already.

> 	2. aggressive serial RPM, where the controller is allowed to
> 	   suspend while the port is open even though this may result in
> 	   lost characters when waking up on incoming data

In this case it seems that the only thing needed is to just
configure the autosuspend delay for the parent port. The use of
-1 has been around since the start of runtime PM AFAIK, so maybe
we should just document it. I guess we could also introduce
pm_runtime_block_autoidle_unless_configured() :)

> For normal ttys, we need a user-space interface for selecting between
> the two, and for serdev we may want a way to select the RPM scheme from
> within the kernel.
> 
> Note that with my serdev controller runtime PM patch, serdev core could
> always opt for aggressive PM (as by default serdev core holds an RPM
> reference for the controller while the port is open).

So if your serdev controller was to set the parent autosuspend
delay on open() and set it back on close() this should work?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > [ Sorry about the late reply. ]
> > 
> > On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180509 13:12]:
> > 
> > > > It seems we really should not be using the negative autosuspend to
> > > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > > mechanism is needed.
> > > 
> > > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > > autosuspend_ms to 3000 by default might be doable. I think that way
> > > we can keep use_autosuspend() in probe. Let's hope there are no
> > > existing use cases that would break with that.
> > 
> > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > either as that would also prevent the device from runtime suspending
> > just as the current negative autosuspend delay does.
> 
> Well in that case we should just stick with -1 value for the
> autosuspend. And just do pm_runtime_put_sync_suspend() after
> probe and on close.

That won't work either as a negative autosuspend delay prevents runtime
suspend completely (by grabbing an extra RPM reference).

> > I fail to see how we can implement this using the current toolbox. What
> > you're after here is really a mechanism for selecting between two
> > different runtime PM schemes at runtime:
> > 
> > 	1. normal serial RPM, where the controller is active while the
> > 	   port is open (this should be the safe default)
> 
> Agreed. And that is the case already.

Yes, but it's not really the case today as since omap-serial (and
8250-omap) sets a negative autosuspend at probe and hence does not
runtime-suspend when the port is closed. So that's the long-standing bug
which needs fixing.

> > 	2. aggressive serial RPM, where the controller is allowed to
> > 	   suspend while the port is open even though this may result in
> > 	   lost characters when waking up on incoming data
> 
> In this case it seems that the only thing needed is to just
> configure the autosuspend delay for the parent port. The use of
> -1 has been around since the start of runtime PM AFAIK, so maybe
> we should just document it. I guess we could also introduce
> pm_runtime_block_autoidle_unless_configured() :)

The implications of a negative autosuspend delay are already documented
(in Documentation/power/runtime_pm.txt); it's just the omap drivers that
gets it wrong when trying to do things which aren't currently supported
(and never have been).

So I still think we need a new mechanism for this.

> > For normal ttys, we need a user-space interface for selecting between
> > the two, and for serdev we may want a way to select the RPM scheme from
> > within the kernel.
> > 
> > Note that with my serdev controller runtime PM patch, serdev core could
> > always opt for aggressive PM (as by default serdev core holds an RPM
> > reference for the controller while the port is open).
> 
> So if your serdev controller was to set the parent autosuspend
> delay on open() and set it back on close() this should work?

Is it really the job of a serdev driver to set the autosuspend delay of
a parent controller? Isn't this somethings which depends on the
characteristics of the controller (possibly configurable by user space)
such as the cost of runtime suspending and resuming?

The patch I posted works with what we have today; if a parent serial
controller driver uses aggressive runtime PM by default or after having
been configured through sysfs to do so.

What I'm getting at here is that the delay should be set by the serial
driver implementing aggressive runtime PM. Then all we need is a
mechanism to determine whether an extra RPM reference should be taken at
tty open or not (configurable by user space, defaulting to yes).

Specifically, the serial drivers themselves would always use
autosuspend and not have to deal with supporting the two RPM schemes
(normal vs aggressive runtime PM).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180521 13:50]:
> On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > > either as that would also prevent the device from runtime suspending
> > > just as the current negative autosuspend delay does.
> > 
> > Well in that case we should just stick with -1 value for the
> > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > probe and on close.
> 
> That won't work either as a negative autosuspend delay prevents runtime
> suspend completely (by grabbing an extra RPM reference).

Well so negative autosuspend delay is working as documented then :)

> > > I fail to see how we can implement this using the current toolbox. What
> > > you're after here is really a mechanism for selecting between two
> > > different runtime PM schemes at runtime:
> > > 
> > > 	1. normal serial RPM, where the controller is active while the
> > > 	   port is open (this should be the safe default)
> > 
> > Agreed. And that is the case already.
> 
> Yes, but it's not really the case today as since omap-serial (and
> 8250-omap) sets a negative autosuspend at probe and hence does not
> runtime-suspend when the port is closed. So that's the long-standing bug
> which needs fixing.

Yes the bug for closed ports needs to be fixed for sure.

> > > 	2. aggressive serial RPM, where the controller is allowed to
> > > 	   suspend while the port is open even though this may result in
> > > 	   lost characters when waking up on incoming data
> > 
> > In this case it seems that the only thing needed is to just
> > configure the autosuspend delay for the parent port. The use of
> > -1 has been around since the start of runtime PM AFAIK, so maybe
> > we should just document it. I guess we could also introduce
> > pm_runtime_block_autoidle_unless_configured() :)
> 
> The implications of a negative autosuspend delay are already documented
> (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> gets it wrong when trying to do things which aren't currently supported
> (and never have been).
> 
> So I still think we need a new mechanism for this.

Well if you have some better mechanism in mind let's try it out. Short of
sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
right now.

> > > For normal ttys, we need a user-space interface for selecting between
> > > the two, and for serdev we may want a way to select the RPM scheme from
> > > within the kernel.
> > > 
> > > Note that with my serdev controller runtime PM patch, serdev core could
> > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > reference for the controller while the port is open).
> > 
> > So if your serdev controller was to set the parent autosuspend
> > delay on open() and set it back on close() this should work?
> 
> Is it really the job of a serdev driver to set the autosuspend delay of
> a parent controller? Isn't this somethings which depends on the
> characteristics of the controller (possibly configurable by user space)
> such as the cost of runtime suspending and resuming?

Only in some cases will the serdev driver know it's safe to configure
the parent controller. Configuring the parent controller from userspace
works just fine as we've seen for years now.

> The patch I posted works with what we have today; if a parent serial
> controller driver uses aggressive runtime PM by default or after having
> been configured through sysfs to do so.

Yeah let's stick with configuring the parent controller from userspace
for now at least.

> What I'm getting at here is that the delay should be set by the serial
> driver implementing aggressive runtime PM. Then all we need is a
> mechanism to determine whether an extra RPM reference should be taken at
> tty open or not (configurable by user space, defaulting to yes).

OK yeah some additional on/off switch seems to be missing here.

> Specifically, the serial drivers themselves would always use
> autosuspend and not have to deal with supporting the two RPM schemes
> (normal vs aggressive runtime PM).

OK. So if I understand your idea right, we could have autosuspend timeout
set to 3000ms in the 8250_omap.c but still default to RPM blocked?
Then user can enable aggressive PM via /sys as desired, right?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180521 13:50]:
> > On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180517 10:12]:

> > > Well in that case we should just stick with -1 value for the
> > > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > > probe and on close.
> > 
> > That won't work either as a negative autosuspend delay prevents runtime
> > suspend completely (by grabbing an extra RPM reference).
> 
> Well so negative autosuspend delay is working as documented then :)

Yes, indeed. All too well. ;)

> > > > I fail to see how we can implement this using the current toolbox. What
> > > > you're after here is really a mechanism for selecting between two
> > > > different runtime PM schemes at runtime:
> > > > 
> > > > 	1. normal serial RPM, where the controller is active while the
> > > > 	   port is open (this should be the safe default)
> > > 
> > > Agreed. And that is the case already.
> > 
> > Yes, but it's not really the case today as since omap-serial (and
> > 8250-omap) sets a negative autosuspend at probe and hence does not
> > runtime-suspend when the port is closed. So that's the long-standing bug
> > which needs fixing.
> 
> Yes the bug for closed ports needs to be fixed for sure.

I did some forensic on this and it seems this problem has "always" been
there. Specifically, closed ports have never been runtime suspended
unless a non-negative autosuspend delay has been set by user space since
fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
driver") which was merged seven years ago.

So while it would certainly be nice to save some more power by default,
this would really be a new feature rather than a bug or regression fix
(which reduces the urgency for this issue somewhat too).

> > > > 	2. aggressive serial RPM, where the controller is allowed to
> > > > 	   suspend while the port is open even though this may result in
> > > > 	   lost characters when waking up on incoming data
> > > 
> > > In this case it seems that the only thing needed is to just
> > > configure the autosuspend delay for the parent port. The use of
> > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > we should just document it. I guess we could also introduce
> > > pm_runtime_block_autoidle_unless_configured() :)
> > 
> > The implications of a negative autosuspend delay are already documented
> > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > gets it wrong when trying to do things which aren't currently supported
> > (and never have been).
> > 
> > So I still think we need a new mechanism for this.
> 
> Well if you have some better mechanism in mind let's try it out. Short of
> sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> right now.

Yeah, that would be too much of a hack and likely wouldn't work either
(and we really should do away with those _force calls altogether).

I've been thinking a bit too much about this already, but it may be
possible to use the pm QoS framework for this. A resume latency can be
set through sysfs where "n/a" is defined to mean "no latency accepted"
(i.e. controller remains always-on while port is open) and "0" means
"any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Now, implementing this may get a little tricky as we want to be able to
change this setting on the fly (consider consoles) and we need to figure
out the interaction with serdev (user space should probably not be
allowed to request a resume latency for ports used by serdev).

I'd be happy to dig into this some more, but not in my spare time I'm
afraid.

> > > > For normal ttys, we need a user-space interface for selecting between
> > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > within the kernel.
> > > > 
> > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > reference for the controller while the port is open).
> > > 
> > > So if your serdev controller was to set the parent autosuspend
> > > delay on open() and set it back on close() this should work?
> > 
> > Is it really the job of a serdev driver to set the autosuspend delay of
> > a parent controller? Isn't this somethings which depends on the
> > characteristics of the controller (possibly configurable by user space)
> > such as the cost of runtime suspending and resuming?
> 
> Only in some cases will the serdev driver know it's safe to configure
> the parent controller. Configuring the parent controller from userspace
> works just fine as we've seen for years now.

Yes, user space may override the default settings provided by the serial
driver, but a serdev driver, in contrast, knows nothing about the
underlying serial hardware.

> > The patch I posted works with what we have today; if a parent serial
> > controller driver uses aggressive runtime PM by default or after having
> > been configured through sysfs to do so.
> 
> Yeah let's stick with configuring the parent controller from userspace
> for now at least.

Yep, status quo works for the time being (since this isn't a
regression). 

> > What I'm getting at here is that the delay should be set by the serial
> > driver implementing aggressive runtime PM. Then all we need is a
> > mechanism to determine whether an extra RPM reference should be taken at
> > tty open or not (configurable by user space, defaulting to yes).
> 
> OK yeah some additional on/off switch seems to be missing here.

As mentioned above, PM QoS resume latency may possibly be used, and
otherwise me may able to define a new (generic) QoS flag for this.

> > Specifically, the serial drivers themselves would always use
> > autosuspend and not have to deal with supporting the two RPM schemes
> > (normal vs aggressive runtime PM).
> 
> OK. So if I understand your idea right, we could have autosuspend timeout
> set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> Then user can enable aggressive PM via /sys as desired, right?

Not RPM blocked; the ports must always be able to suspend when the port
is closed. But user space should be able to enable the aggressive
(active) runtime PM via sysfs independently of the autosuspend delay,
yes.

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> > 
> > Yes the bug for closed ports needs to be fixed for sure.
> 
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).

Yes it's been there since the start.

> > > > > 	2. aggressive serial RPM, where the controller is allowed to
> > > > > 	   suspend while the port is open even though this may result in
> > > > > 	   lost characters when waking up on incoming data
> > > > 
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > > 
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > > 
> > > So I still think we need a new mechanism for this.
> > 
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
> 
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
> 
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Oh yeah, PM QoS might work here!

> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).

It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.

Andy, see the PM QoS comment above for console idling :)

> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.

Indeed.

> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > > within the kernel.
> > > > > 
> > > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > > 
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > > 
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> > 
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
> 
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
> 
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> > 
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
> 
> Yep, status quo works for the time being (since this isn't a
> regression). 
> 
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> > 
> > OK yeah some additional on/off switch seems to be missing here.
> 
> As mentioned above, PM QoS resume latency may possibly be used, and
> otherwise me may able to define a new (generic) QoS flag for this.

Good idea.

> > > Specifically, the serial drivers themselves would always use
> > > autosuspend and not have to deal with supporting the two RPM schemes
> > > (normal vs aggressive runtime PM).
> > 
> > OK. So if I understand your idea right, we could have autosuspend timeout
> > set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> > Then user can enable aggressive PM via /sys as desired, right?
> 
> Not RPM blocked; the ports must always be able to suspend when the port
> is closed. But user space should be able to enable the aggressive
> (active) runtime PM via sysfs independently of the autosuspend delay,
> yes.

Yup OK, I like the PM QoS approach.

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 24, 2018 at 06:32:37AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180524 09:20]:
> > On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:

> > > Well if you have some better mechanism in mind let's try it out. Short of
> > > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > > right now.
> > 
> > Yeah, that would be too much of a hack and likely wouldn't work either
> > (and we really should do away with those _force calls altogether).
> > 
> > I've been thinking a bit too much about this already, but it may be
> > possible to use the pm QoS framework for this. A resume latency can be
> > set through sysfs where "n/a" is defined to mean "no latency accepted"
> > (i.e. controller remains always-on while port is open) and "0" means
> > "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
> 
> Oh yeah, PM QoS might work here!

Actually, after reading a recent QoS related bug report, I realised that
a resume latency request of "n/a" is actually a third way of disabling
runtime PM, which similarly to the negative autosuspend would prevent
also a closed port from suspending.

Using a small positive resume latency for this feels like too much of a
hack, but defining a new QoS flag might still work.

Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

Hi,

On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > 
> > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > us to enable runtime PM by default (once implemented), since we know
> > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > with sideband wakeup gpios).
> > > 
> > > I'm not sure we want generic runtime-pm support for the controllers in
> > > the sense that the slave device state is always reflected by the serial
> > > controller. Similar as for i2c and spi, we really only want to keep the
> > > controller active when we are doing I/O, but we may want to keep a
> > > client active for longer.
> > 
> > Yeah i2c seems to do the right thing where the bus takes care
> > of runtime PM.
> 
> Yeah, but since serial is async in contrast to i2c/spi, we may not be
> able to push this entirely into core. The serdev drivers may need to
> indicate when they expect or need to do I/O by opening and closing the
> port. And this is how I implemented these first couple of gnss drivers.
>
> Also note that most serial driver do not do runtime pm while the port is
> open (as OMAP does), and doing so also has the drawbacks of lost
> characters etc. as Sebastian mentioned.

I think using open/close for runtime pm is good enough for GPS,
since it regularly sends data and draws lots of power anyways.
But devices, that have an out-of-band wakeup signal can do proper
runtime PM of the serial port without loosing characters.

Note, that OMAP does not reach deep idle states with active
serial port. This is not acceptable for low power devices.

> > > Take the u-blox driver in this series for example. As I'm using runtime
> > > PM to manage device power, user-space can chose to prevent the receiver
> > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > in setups without a backup battery (by means of the power/control
> > > attribute).
> > 
> > Sorry I don't seem to have that one, care to paste the subject
> > line of that patch?
> 
> 	"[PATCH 5/7] gnss: add driver for u-blox receivers"
> 
> 	https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org
> 
> > > Note that serdev not enabling runtime pm for controllers is roughly
> > > equivalent to setting the .ignore_children flag, which is what we do for
> > > i2c and spi controller, and possibly what we want here too.

For I2C/SPI this works, since receive operations are initiated by
the controller. Unfortunately its harder to implement for async
serial. But I agree, that we may want to have runtime PM for the
serdev client and .ignore_children is the way to go.

I think the client API should allow two things:

1. minimal runtime PM support: The controller is runtime enabled
on serdev open and disabled on serdev close. This may be enough
for some clients and useful for writing new drivers.

2. full runtime PM support: The controller is sleeping by default
even with serdev open. The calls to write/change port settings/...
automatically enables the device, similar to i2c/spi. But there
must be additional functions to enable/disable runtime PM based
on a wakeup gpio or similar out-of-band information. It may be
enough to just provide:

int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
    pm_runtime_get_sync(&serdev->dev);
}

int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
    pm_runtime_put_autosuspend(&serdev->dev);
}

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

[ Changing the subject line as this is thread is no longer about DT
  bindings.

  Also adding linux-serial and linux-pm while keeping some context. ]

On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > > 
> > > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > > us to enable runtime PM by default (once implemented), since we know
> > > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > > with sideband wakeup gpios).
> > > > 
> > > > I'm not sure we want generic runtime-pm support for the controllers in
> > > > the sense that the slave device state is always reflected by the serial
> > > > controller. Similar as for i2c and spi, we really only want to keep the
> > > > controller active when we are doing I/O, but we may want to keep a
> > > > client active for longer.
> > > 
> > > Yeah i2c seems to do the right thing where the bus takes care
> > > of runtime PM.
> > 
> > Yeah, but since serial is async in contrast to i2c/spi, we may not be
> > able to push this entirely into core. The serdev drivers may need to
> > indicate when they expect or need to do I/O by opening and closing the
> > port. And this is how I implemented these first couple of gnss drivers.
> >
> > Also note that most serial driver do not do runtime pm while the port is
> > open (as OMAP does), and doing so also has the drawbacks of lost
> > characters etc. as Sebastian mentioned.
> 
> I think using open/close for runtime pm is good enough for GPS,
> since it regularly sends data and draws lots of power anyways.
> But devices, that have an out-of-band wakeup signal can do proper
> runtime PM of the serial port without loosing characters.

Yeah, there may be some applications where this is possible. And this is
not the case for GPS, but not just because of a generally higher power
consumption, but due to the fact that we cannot afford having the first
message in every report burst be dropped.

> Note, that OMAP does not reach deep idle states with active
> serial port. This is not acceptable for low power devices.

Sure, but note that OMAP is the only serial driver which currently
implements this kind of aggressive runtime PM (besides a couple of
usb-serial drivers). This means that a serdev driver can never rely on
this being the case, and therefore needs to be restrictive about how
long the port is kept open if it cares about power at all.

> > > > Take the u-blox driver in this series for example. As I'm using runtime
> > > > PM to manage device power, user-space can chose to prevent the receiver
> > > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > > in setups without a backup battery (by means of the power/control
> > > > attribute).
> > > 
> > > Sorry I don't seem to have that one, care to paste the subject
> > > line of that patch?
> > 
> > 	"[PATCH 5/7] gnss: add driver for u-blox receivers"
> > 
> > 	https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org
> > 
> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> 
> For I2C/SPI this works, since receive operations are initiated by
> the controller. Unfortunately its harder to implement for async
> serial. But I agree, that we may want to have runtime PM for the
> serdev client and .ignore_children is the way to go.

It's really about adding runtime PM support to serdev controllers.
Serdev client drivers can already use runtime PM (as mentioned above).

The ignore_children flag would then allow the controller RPM state to be
managed independently of the child/slave device state when more fine
grained RPM control is desired.

> I think the client API should allow two things:
> 
> 1. minimal runtime PM support: The controller is runtime enabled
> on serdev open and disabled on serdev close. This may be enough
> for some clients and useful for writing new drivers.

Right, and we already have something like this today by means of how the
serial driver implement runtime PM (i.e. they are generally active while
the port is open).

> 2. full runtime PM support: The controller is sleeping by default
> even with serdev open. The calls to write/change port settings/...
> automatically enables the device, similar to i2c/spi. But there
> must be additional functions to enable/disable runtime PM based
> on a wakeup gpio or similar out-of-band information. It may be
> enough to just provide:
> 
> int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
>     pm_runtime_get_sync(&serdev->dev);
> }
> 
> int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
>     pm_runtime_put_autosuspend(&serdev->dev);
> }

I'm not a big fan of rpm wrappers and prefer using the RPM interface
directly, but that's a side note. In the above case we really want to
manage the controller (&serdev->ctrl->dev) rather than the client
however (.ignore_children should be set, remember).

Also note that a serial driver implementing aggressive RPM (e.g. using
autosuspend) would manage the RPM counts itself when changing terminal
settings etc, so the only thing that would be needed is for the
client/slave device to resume the controller pro-actively when it is
expecting incoming data.

To make this more concrete; an example could be a device with an OOB
wake-up signal, but using a request-response type protocol so that the
client driver knows when it's safe to allow the controller to again
suspend.

We can model this similarly to how we do it for usb-serial, namely that
the core takes an extra RPM reference at open, which a sufficiently
power aware driver can then chose to drop in order to allow for more
aggressive controller runtime PM (should the underlying device and driver
support it).

I've cooked up a patch which I'll be sending as a reply to this mail.

Thanks,
Johan

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 11:18:31AM +0200, Johan Hovold wrote:

> I've cooked up a patch which I'll be sending as a reply to this mail.

Forgot to add the in-reply-to header of course. For the interested, the
patch can be found here:

https://lkml.kernel.org/r/20180509094419.13470-1-johan@kernel.org

Johan

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Tony Lindgren]

* Johan Hovold <johan@kernel.org> [180509 09:20]:
> On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > I think using open/close for runtime pm is good enough for GPS,
> > since it regularly sends data and draws lots of power anyways.
> > But devices, that have an out-of-band wakeup signal can do proper
> > runtime PM of the serial port without loosing characters.
> 
> Yeah, there may be some applications where this is possible. And this is
> not the case for GPS, but not just because of a generally higher power
> consumption, but due to the fact that we cannot afford having the first
> message in every report burst be dropped.

Well most of the phone implementations use one or two out of band
GPIOs to first wake the UART before any data is sent. For serdev
this can be called from the serdev consumer write function for TX.
For RX, the serdev consumer needs to implement an interrupt handler
and wake up the parent UART before serdev RX.

> > Note, that OMAP does not reach deep idle states with active
> > serial port. This is not acceptable for low power devices.
> 
> Sure, but note that OMAP is the only serial driver which currently
> implements this kind of aggressive runtime PM (besides a couple of
> usb-serial drivers). This means that a serdev driver can never rely on
> this being the case, and therefore needs to be restrictive about how
> long the port is kept open if it cares about power at all.

Well by default we don't allow lossy UART. It needs to be manually
configured via /sys for the timeout. With serdev, this can all be
done with no /sys configuration needed for the cases with GPIO
wake irqs :)

Regards,

Tony

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 07:05:50AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180509 09:20]:
> > On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > > I think using open/close for runtime pm is good enough for GPS,
> > > since it regularly sends data and draws lots of power anyways.
> > > But devices, that have an out-of-band wakeup signal can do proper
> > > runtime PM of the serial port without loosing characters.
> > 
> > Yeah, there may be some applications where this is possible. And this is
> > not the case for GPS, but not just because of a generally higher power
> > consumption, but due to the fact that we cannot afford having the first
> > message in every report burst be dropped.
> 
> Well most of the phone implementations use one or two out of band
> GPIOs to first wake the UART before any data is sent. For serdev
> this can be called from the serdev consumer write function for TX.
> For RX, the serdev consumer needs to implement an interrupt handler
> and wake up the parent UART before serdev RX.

Right, but the client driver would then wake the parent controller when
expecting RX, while waking its own (serial-attached) device when wanting
to do TX.

Just for the record, we also cleared this up here:

	https://marc.info/?l=linux-kernel&m=152604434504868&w=2

> > > Note, that OMAP does not reach deep idle states with active
> > > serial port. This is not acceptable for low power devices.
> > 
> > Sure, but note that OMAP is the only serial driver which currently
> > implements this kind of aggressive runtime PM (besides a couple of
> > usb-serial drivers). This means that a serdev driver can never rely on
> > this being the case, and therefore needs to be restrictive about how
> > long the port is kept open if it cares about power at all.
> 
> Well by default we don't allow lossy UART. It needs to be manually
> configured via /sys for the timeout. With serdev, this can all be
> done with no /sys configuration needed for the cases with GPIO
> wake irqs :)

Indeed, but serdev client drivers must also work with serial drivers
which do not implement this kind of aggressive runtime PM, and then the
only way to save power is to close the port when not in use.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Thu, May 03, 2018 at 11:35:21AM +0200, H. Nikolaus Schaller wrote:

> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> that it does not provide a WAKEUP signal. And another significant
> difference is that we have to keep the serdev UART enabled even if there
> is no user-space client. Otherwise we are not able to detect unexpected
> activity. So we unfortunately can't move serdev open/close into the .open
> and .close ops but need to open it in probe.

As have also been discussed elsewhere in this thread it may be possible,
and it is definitely desirable, to only keep the port open when really
needed. But given the complexity of implementing this, starting with a
simpler and less power efficient method for sirf-chips without WAKEUP
may be acceptable.

> Therefore, it is in my opinion still better to have a separate driver for
> the w2sg0004 instead of hacking the support of this chip into your WAKEUP
> capable sirfstar driver. So I suggest that you make WAKEUP a required
> property.

I disagree. The sirf driver subsumes your particular wi2wi module and
configurations without WAKEUP are described by the datasheets for other
modules as well.

> We had faced a comparable decision last year with the ov9650 and ov9655 camera
> sensors which are almost the same. But not same enough to integrate both into
> a single driver.

But here we are talking about two configuration for the same chip (even
if your particular chip only supports one).

Johan

[PATCH 5/7] gnss: add driver for u-blox receivers

[Johan Hovold]

Add driver for serial-connected u-blox GNSS receivers.

Note that the driver uses the generic GNSS serial implementation and
therefore essentially only manages power abstracted into three power
states: ACTIVE, STANDBY, and OFF.

For u-blox receivers with a single supply and no enable-gpios, this
simply means that the main supply is disabled in STANDBY and OFF.

Note that timepulse-support is not yet implemented.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/Kconfig  |  13 +++++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/ubx.c    | 133 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 drivers/gnss/ubx.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index f8ee54f99a8d..784b8c0367d9 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -15,4 +15,17 @@ if GNSS
 config GNSS_SERIAL
 	tristate
 
+config GNSS_UBX_SERIAL
+	tristate "u-blox GNSS receiver support"
+	depends on SERIAL_DEV_BUS
+	select GNSS_SERIAL
+	---help---
+	  Say Y here if you have a u-blox GNSS receiver which uses a serial
+	  interface.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss-ubx.
+
+	  If unsure, say N.
+
 endif # GNSS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index 171aba71684d..d9295b20b7bc 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -8,3 +8,6 @@ gnss-y := core.o
 
 obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
 gnss-serial-y := serial.o
+
+obj-$(CONFIG_GNSS_UBX_SERIAL)		+= gnss-ubx.o
+gnss-ubx-y := ubx.o
diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
new file mode 100644
index 000000000000..2f2f00202b5b
--- /dev/null
+++ b/drivers/gnss/ubx.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * u-blox GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+
+#include "serial.h"
+
+struct ubx_data {
+	struct regulator *vcc;
+};
+
+static int ubx_set_active(struct gnss_serial *gserial)
+{
+	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
+	int ret;
+
+	dev_dbg(&gserial->serdev->dev, "%s\n", __func__);
+
+	ret = regulator_enable(data->vcc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ubx_set_standby(struct gnss_serial *gserial)
+{
+	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
+	int ret;
+
+	dev_dbg(&gserial->serdev->dev, "%s\n", __func__);
+
+	ret = regulator_disable(data->vcc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int
+ubx_set_power(struct gnss_serial *gserial, enum gnss_serial_pm_state state)
+{
+	switch (state) {
+	case GNSS_SERIAL_ACTIVE:
+		return ubx_set_active(gserial);
+	case GNSS_SERIAL_OFF:
+	case GNSS_SERIAL_STANDBY:
+		return ubx_set_standby(gserial);
+	}
+
+	return -EINVAL;
+}
+
+const struct gnss_serial_ops ubx_gserial_ops = {
+	.set_power = ubx_set_power,
+};
+
+static int ubx_probe(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial;
+	struct ubx_data *data;
+	int ret;
+
+	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	if (IS_ERR(gserial)) {
+		ret = PTR_ERR(gserial);
+		return ret;
+	}
+
+	gserial->ops = &ubx_gserial_ops;
+
+	data = gnss_serial_get_drvdata(gserial);
+
+	data->vcc = devm_regulator_get(&serdev->dev, "vcc");
+	if (IS_ERR(data->vcc)) {
+		ret = PTR_ERR(data->vcc);
+		goto err_free_gserial;
+	}
+
+	ret = gnss_serial_register(gserial);
+	if (ret)
+		goto err_free_gserial;
+
+	return 0;
+
+err_free_gserial:
+	gnss_serial_free(gserial);
+
+	return ret;
+}
+
+static void ubx_remove(struct serdev_device *serdev)
+{
+	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
+
+	gnss_serial_deregister(gserial);
+	gnss_serial_free(gserial);
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id ubx_of_match[] = {
+	{ .compatible = "u-blox,neo-8" },
+	{ .compatible = "u-blox,neo-m8" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ubx_of_match);
+#endif
+
+static struct serdev_device_driver ubx_driver = {
+	.driver	= {
+		.name		= "gnss-ubx",
+		.of_match_table	= of_match_ptr(ubx_of_match),
+		.pm		= &gnss_serial_pm_ops,
+	},
+	.probe	= ubx_probe,
+	.remove	= ubx_remove,
+};
+module_serdev_device_driver(ubx_driver);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("u-blox GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.0

[PATCH 6/7] dt-bindings: gnss: add sirfstar binding

[Johan Hovold]

Add binding for SiRFstar-based GNSS receivers.

Note that while four compatible-strings are initially added representing
devices which differ in which I/O interfaces they support, they
otherwise essentially share the same feature set.

Pin and supply names (and some recommended timings) vary slightly, but
the binding recommends using a common set of names.

Note that the wakeup gpio is not intended to be as a wakeup source, but
rather to detect the current power state of the device (active or
hibernate).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 .../devicetree/bindings/gnss/sirfstar.txt     | 38 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt   |  3 ++
 2 files changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt

diff --git a/Documentation/devicetree/bindings/gnss/sirfstar.txt b/Documentation/devicetree/bindings/gnss/sirfstar.txt
new file mode 100644
index 000000000000..5e6a02aec49a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -0,0 +1,38 @@
+SiRFstar-based GNSS Receiver DT binding
+
+SiRFstar chipsets are used in GNSS-receiver modules produced by several
+vendors and can use UART, SPI or I2C interfaces.
+
+Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
+properties.
+
+Required Properties:
+
+- compatible 	: Must be one of
+
+			"fastrax,uc430"
+			"linx,r4"
+			"wi2wi,w2sg0008i"
+			"wi2wi,w2sg0084i"
+
+- vcc-supply	: Main voltage regulator (3V3_IN, VDD, VCC)
+
+Optional Properties:
+
+- enable-gpios	: GPIO used to power on and off device (ON_OFF)
+- wakeup-gpios	: GPIO used to determine device power state (WAKEUP, RFPWRUP)
+- timepulse-gpios	: Timepulse (e.g 1PPS) GPIO (1PPS, TM)
+
+Example:
+
+serial@1234 {
+	compatible = "ns16550a";
+
+	gnss {
+		compatible = "wi2wi,w2sg0084i";
+
+		vcc-supply = <&gnss_reg>;
+		enable-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>;
+		wakeup-gpios = <&gpio0 17 GPIO_ACTIVE_HIGH>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 2128dfdf73f1..ddd81c82082d 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -120,6 +120,7 @@ excito	Excito
 ezchip	EZchip Semiconductor
 fairphone	Fairphone B.V.
 faraday	Faraday Technology Corporation
+fastrax	Fastrax Oy
 fcs	Fairchild Semiconductor
 firefly	Firefly
 focaltech	FocalTech Systems Co.,Ltd
@@ -197,6 +198,7 @@ licheepi	Lichee Pi
 linaro	Linaro Limited
 linksys	Belkin International, Inc. (Linksys)
 linux	Linux-specific binding
+linx	Linx Technologies
 lltc	Linear Technology Corporation
 lsi	LSI Corp. (LSI Logic)
 lwn	Liebherr-Werk Nenzing GmbH
@@ -390,6 +392,7 @@ vivante	Vivante Corporation
 vocore VoCore Studio
 voipac	Voipac Technologies s.r.o.
 vot	Vision Optical Technology Co., Ltd.
+wi2wi	Wi2Wi
 wd	Western Digital Corp.
 wetek	WeTek Electronics, limited.
 wexler	Wexler
-- 
2.17.0

Re: [PATCH 6/7] dt-bindings: gnss: add sirfstar binding

[Rob Herring]

On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> Add binding for SiRFstar-based GNSS receivers.
>
> Note that while four compatible-strings are initially added representing
> devices which differ in which I/O interfaces they support, they
> otherwise essentially share the same feature set.
>
> Pin and supply names (and some recommended timings) vary slightly, but
> the binding recommends using a common set of names.
>
> Note that the wakeup gpio is not intended to be as a wakeup source, but
> rather to detect the current power state of the device (active or
> hibernate).
>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  .../devicetree/bindings/gnss/sirfstar.txt     | 38 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt   |  3 ++
>  2 files changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt

One nit below, otherwise:

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

> diff --git a/Documentation/devicetree/bindings/gnss/sirfstar.txt b/Documentation/devicetree/bindings/gnss/sirfstar.txt
> new file mode 100644
> index 000000000000..5e6a02aec49a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
> @@ -0,0 +1,38 @@
> +SiRFstar-based GNSS Receiver DT binding
> +
> +SiRFstar chipsets are used in GNSS-receiver modules produced by several
> +vendors and can use UART, SPI or I2C interfaces.
> +
> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> +properties.
> +
> +Required Properties:
> +
> +- compatible   : Must be one of
> +
> +                       "fastrax,uc430"
> +                       "linx,r4"
> +                       "wi2wi,w2sg0008i"
> +                       "wi2wi,w2sg0084i"
> +
> +- vcc-supply   : Main voltage regulator (3V3_IN, VDD, VCC)
> +
> +Optional Properties:
> +
> +- enable-gpios : GPIO used to power on and off device (ON_OFF)
> +- wakeup-gpios : GPIO used to determine device power state (WAKEUP, RFPWRUP)
> +- timepulse-gpios      : Timepulse (e.g 1PPS) GPIO (1PPS, TM)
> +
> +Example:
> +
> +serial@1234 {
> +       compatible = "ns16550a";
> +
> +       gnss {
> +               compatible = "wi2wi,w2sg0084i";
> +
> +               vcc-supply = <&gnss_reg>;
> +               enable-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>;
> +               wakeup-gpios = <&gpio0 17 GPIO_ACTIVE_HIGH>;
> +       };
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 2128dfdf73f1..ddd81c82082d 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -120,6 +120,7 @@ excito      Excito
>  ezchip EZchip Semiconductor
>  fairphone      Fairphone B.V.
>  faraday        Faraday Technology Corporation
> +fastrax        Fastrax Oy
>  fcs    Fairchild Semiconductor
>  firefly        Firefly
>  focaltech      FocalTech Systems Co.,Ltd
> @@ -197,6 +198,7 @@ licheepi    Lichee Pi
>  linaro Linaro Limited
>  linksys        Belkin International, Inc. (Linksys)
>  linux  Linux-specific binding
> +linx   Linx Technologies
>  lltc   Linear Technology Corporation
>  lsi    LSI Corp. (LSI Logic)
>  lwn    Liebherr-Werk Nenzing GmbH
> @@ -390,6 +392,7 @@ vivante     Vivante Corporation
>  vocore VoCore Studio
>  voipac Voipac Technologies s.r.o.
>  vot    Vision Optical Technology Co., Ltd.
> +wi2wi  Wi2Wi

Wrong spot.

>  wd     Western Digital Corp.
>  wetek  WeTek Electronics, limited.
>  wexler Wexler
> --
> 2.17.0
>

Re: [PATCH 6/7] dt-bindings: gnss: add sirfstar binding

[Johan Hovold]

On Wed, Apr 25, 2018 at 01:25:26PM -0500, Rob Herring wrote:
> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold <johan@kernel.org> wrote:
> > Add binding for SiRFstar-based GNSS receivers.
> >
> > Note that while four compatible-strings are initially added representing
> > devices which differ in which I/O interfaces they support, they
> > otherwise essentially share the same feature set.
> >
> > Pin and supply names (and some recommended timings) vary slightly, but
> > the binding recommends using a common set of names.
> >
> > Note that the wakeup gpio is not intended to be as a wakeup source, but
> > rather to detect the current power state of the device (active or
> > hibernate).
> >
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  .../devicetree/bindings/gnss/sirfstar.txt     | 38 +++++++++++++++++++
> >  .../devicetree/bindings/vendor-prefixes.txt   |  3 ++
> >  2 files changed, 41 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt
> 
> One nit below, otherwise:
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

> > @@ -390,6 +392,7 @@ vivante     Vivante Corporation
> >  vocore VoCore Studio
> >  voipac Voipac Technologies s.r.o.
> >  vot    Vision Optical Technology Co., Ltd.
> > +wi2wi  Wi2Wi
> 
> Wrong spot.

Duh. I had first misspelled this w2wi (which happens to be a different
company), and then forgot to move the entry when I corrected my mistake.

> >  wd     Western Digital Corp.
> >  wetek  WeTek Electronics, limited.
> >  wexler Wexler

Thanks,
Johan

[PATCH 7/7] gnss: add driver for sirfstar-based receivers

[Johan Hovold]

Add driver for serial-connected SiRFstar-based GNSS receivers.

These devices typically boot into hibernate mode from which they can be
woken using a pulse on the ON_OFF (enable) input pin. Once active, a
pulse on the same ON_OFF pin is used to put the device back into
hibernate mode. The current state can be determined by sampling the
WAKEUP output.

Hardware configurations where WAKEUP has been connected to ON_OFF (and
where an initial WAKEUP pulse during boot is sufficient to have the
device boot into active mode) are also supported. In this case, device
power is managed using the main-supply regulator only.

Note that configurations where WAKEUP is left not connected, so that the
device power state can only indirectly be determined using the I/O
interface, is currently not supported. It should be fairly
straight-forward to extend the current implementation with such support
however (and this this is the main reason for not using the generic
serial implementation for this driver).

Note that timepulse-support is left unimplemented.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gnss/Kconfig  |  12 ++
 drivers/gnss/Makefile |   3 +
 drivers/gnss/sirf.c   | 415 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+)
 create mode 100644 drivers/gnss/sirf.c

diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
index 784b8c0367d9..6abc88514512 100644
--- a/drivers/gnss/Kconfig
+++ b/drivers/gnss/Kconfig
@@ -15,6 +15,18 @@ if GNSS
 config GNSS_SERIAL
 	tristate
 
+config GNSS_SIRF_SERIAL
+	tristate "SiRFstar GNSS receiver support"
+	depends on SERIAL_DEV_BUS
+	---help---
+	  Say Y here if you have a SiRFstar-based GNSS receiver which uses a
+	  serial interface.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gnss-sirf.
+
+	  If unsure, say N.
+
 config GNSS_UBX_SERIAL
 	tristate "u-blox GNSS receiver support"
 	depends on SERIAL_DEV_BUS
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
index d9295b20b7bc..5cf0ebe0330a 100644
--- a/drivers/gnss/Makefile
+++ b/drivers/gnss/Makefile
@@ -9,5 +9,8 @@ gnss-y := core.o
 obj-$(CONFIG_GNSS_SERIAL)		+= gnss-serial.o
 gnss-serial-y := serial.o
 
+obj-$(CONFIG_GNSS_SIRF_SERIAL)		+= gnss-sirf.o
+gnss-sirf-y := sirf.o
+
 obj-$(CONFIG_GNSS_UBX_SERIAL)		+= gnss-ubx.o
 gnss-ubx-y := ubx.o
diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
new file mode 100644
index 000000000000..9d46da1530ca
--- /dev/null
+++ b/drivers/gnss/sirf.c
@@ -0,0 +1,415 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SiRFstar GNSS receiver driver
+ *
+ * Copyright (C) 2018 Johan Hovold <johan@kernel.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/gnss.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+
+#define SIRF_BOOT_DELAY			500
+#define SIRF_ON_OFF_PULSE_TIME		100
+#define SIRF_ACTIVATE_TIMEOUT		200
+#define SIRF_HIBERNATE_TIMEOUT		200
+
+struct sirf_data {
+	struct gnss_device *gdev;
+	struct serdev_device *serdev;
+	speed_t	speed;
+	struct regulator *vcc;
+	struct gpio_desc *on_off;
+	struct gpio_desc *wakeup;
+	int irq;
+	bool active;
+	wait_queue_head_t power_wait;
+};
+
+static int sirf_open(struct gnss_device *gdev)
+{
+	struct sirf_data *data = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = data->serdev;
+	int ret;
+
+	ret = serdev_device_open(serdev);
+	if (ret)
+		return ret;
+
+	serdev_device_set_baudrate(serdev, data->speed);
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = pm_runtime_get_sync(&serdev->dev);
+	if (ret < 0) {
+		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
+		pm_runtime_put_noidle(&serdev->dev);
+		goto err_close;
+	}
+
+	return 0;
+
+err_close:
+	serdev_device_close(serdev);
+
+	return ret;
+}
+
+static void sirf_close(struct gnss_device *gdev)
+{
+	struct sirf_data *data = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = data->serdev;
+
+	serdev_device_close(serdev);
+
+	pm_runtime_put(&serdev->dev);
+}
+
+static int sirf_write_raw(struct gnss_device *gdev,
+		const unsigned char *buf, size_t count)
+{
+	struct sirf_data *data = gnss_get_drvdata(gdev);
+	struct serdev_device *serdev = data->serdev;
+	int ret;
+
+	/* write is only buffered synchronously */
+	ret = serdev_device_write(serdev, buf, count, 0);
+	if (ret < 0)
+		return ret;
+
+	/* FIXME: determine if interrupted? */
+	serdev_device_wait_until_sent(serdev, 0);
+
+	return count;
+}
+
+static const struct gnss_operations sirf_gnss_ops = {
+	.open		= sirf_open,
+	.close		= sirf_close,
+	.write_raw	= sirf_write_raw,
+};
+
+static int sirf_receive_buf(struct serdev_device *serdev,
+		const unsigned char *buf, size_t count)
+{
+	struct sirf_data *data = serdev_device_get_drvdata(serdev);
+	struct gnss_device *gdev = data->gdev;
+
+	return gnss_insert_raw(gdev, buf, count);
+}
+
+static const struct serdev_device_ops sirf_serdev_ops = {
+	.receive_buf	= sirf_receive_buf,
+	.write_wakeup	= serdev_device_write_wakeup,
+};
+
+static irqreturn_t sirf_wakeup_handler(int irq, void *dev_id)
+{
+	struct sirf_data *data = dev_id;
+	struct device *dev = &data->serdev->dev;
+	int ret;
+
+	ret = gpiod_get_value_cansleep(data->wakeup);
+	dev_dbg(dev, "%s - wakeup = %d\n", __func__, ret);
+	if (ret < 0)
+		goto out;
+
+	data->active = !!ret;
+	wake_up_interruptible(&data->power_wait);
+out:
+	return IRQ_HANDLED;
+}
+
+static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
+		unsigned long timeout)
+{
+	int ret;
+
+	ret = wait_event_interruptible_timeout(data->power_wait,
+			data->active == active, msecs_to_jiffies(timeout));
+	if (ret < 0)
+		return ret;
+
+	if (ret == 0) {
+		dev_warn(&data->serdev->dev, "timeout waiting for active state = %d\n",
+				active);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static void sirf_pulse_on_off(struct sirf_data *data)
+{
+	gpiod_set_value_cansleep(data->on_off, 1);
+	msleep(SIRF_ON_OFF_PULSE_TIME);
+	gpiod_set_value_cansleep(data->on_off, 0);
+}
+
+static int sirf_set_active(struct sirf_data *data, bool active)
+{
+	unsigned long timeout;
+	int retries = 3;
+	int ret;
+
+	if (active)
+		timeout = SIRF_ACTIVATE_TIMEOUT;
+	else
+		timeout = SIRF_HIBERNATE_TIMEOUT;
+
+	while (retries-- > 0) {
+		sirf_pulse_on_off(data);
+		ret = sirf_wait_for_power_state(data, active, timeout);
+		if (ret < 0) {
+			if (ret == -ETIMEDOUT)
+				continue;
+
+			return ret;
+		}
+
+		break;
+	}
+
+	if (retries == 0)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int sirf_runtime_suspend(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (!data->on_off)
+		return regulator_disable(data->vcc);
+
+	return sirf_set_active(data, false);
+}
+
+static int sirf_runtime_resume(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (!data->on_off)
+		return regulator_enable(data->vcc);
+
+	return sirf_set_active(data, true);
+}
+
+static int __maybe_unused sirf_suspend(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dev_dbg(dev, "%s - pm_runtime_suspended = %d\n", __func__,
+			pm_runtime_suspended(dev));
+
+	if (!pm_runtime_suspended(dev))
+		ret = sirf_runtime_suspend(dev);
+
+	if (data->wakeup)
+		disable_irq(data->irq);
+
+	return ret;
+}
+
+static int __maybe_unused sirf_resume(struct device *dev)
+{
+	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dev_dbg(dev, "%s - pm_runtime_suspended = %d\n", __func__,
+			pm_runtime_suspended(dev));
+
+	if (data->wakeup)
+		enable_irq(data->irq);
+
+	if (!pm_runtime_suspended(dev))
+		ret = sirf_runtime_resume(dev);
+
+	return ret;
+}
+
+static const struct dev_pm_ops sirf_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(sirf_suspend, sirf_resume)
+	SET_RUNTIME_PM_OPS(sirf_runtime_suspend, sirf_runtime_resume, NULL)
+};
+
+static int sirf_parse_dt(struct serdev_device *serdev)
+{
+	struct sirf_data *data = serdev_device_get_drvdata(serdev);
+	struct device_node *node = serdev->dev.of_node;
+	u32 speed = 9600;
+
+	of_property_read_u32(node, "current-speed", &speed);
+
+	data->speed = speed;
+
+	return 0;
+}
+
+static int sirf_probe(struct serdev_device *serdev)
+{
+	struct device *dev = &serdev->dev;
+	struct gnss_device *gdev;
+	struct sirf_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	gdev = gnss_allocate_device(dev);
+	if (!gdev)
+		return -ENOMEM;
+
+	gdev->ops = &sirf_gnss_ops;
+	gnss_set_drvdata(gdev, data);
+
+	data->serdev = serdev;
+	data->gdev = gdev;
+
+	init_waitqueue_head(&data->power_wait);
+
+	serdev_device_set_drvdata(serdev, data);
+	serdev_device_set_client_ops(serdev, &sirf_serdev_ops);
+
+	ret = sirf_parse_dt(serdev);
+	if (ret)
+		goto err_put_device;
+
+	data->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(data->vcc)) {
+		ret = PTR_ERR(data->vcc);
+		goto err_put_device;
+	}
+
+	data->on_off = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(data->on_off))
+		goto err_put_device;
+
+	if (data->on_off) {
+		data->wakeup = devm_gpiod_get_optional(dev, "wakeup", GPIOD_IN);
+		if (IS_ERR(data->wakeup))
+			goto err_put_device;
+
+		/*
+		 * Configurations where WAKEUP has been left not connected,
+		 * are currently not supported.
+		 */
+		if (!data->wakeup) {
+			dev_err(dev, "no wakeup gpio specified\n");
+			ret = -ENODEV;
+			goto err_put_device;
+		}
+	}
+
+	if (data->wakeup) {
+		ret = gpiod_to_irq(data->wakeup);
+		if (ret < 0)
+			goto err_put_device;
+
+		data->irq = ret;
+
+		ret = devm_request_threaded_irq(dev, data->irq, NULL,
+				sirf_wakeup_handler,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				"wakeup", data);
+		if (ret)
+			goto err_put_device;
+	}
+
+	if (data->on_off) {
+		ret = regulator_enable(data->vcc);
+		if (ret)
+			goto err_put_device;
+
+		/* Wait for chip to boot into hibernate mode */
+		msleep(SIRF_BOOT_DELAY);
+	}
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
+		pm_runtime_enable(dev);
+	} else {
+		ret = sirf_runtime_resume(dev);
+		if (ret < 0)
+			goto err_disable_vcc;
+	}
+
+	ret = gnss_register_device(gdev);
+	if (ret)
+		goto err_disable_rpm;
+
+	return 0;
+
+err_disable_rpm:
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(dev);
+	else
+		sirf_runtime_suspend(dev);
+err_disable_vcc:
+	if (data->on_off)
+		regulator_disable(data->vcc);
+err_put_device:
+	gnss_put_device(data->gdev);
+
+	return ret;
+}
+
+static void sirf_remove(struct serdev_device *serdev)
+{
+	struct sirf_data *data = serdev_device_get_drvdata(serdev);
+
+	gnss_deregister_device(data->gdev);
+
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_disable(&serdev->dev);
+	else
+		sirf_runtime_suspend(&serdev->dev);
+
+	if (data->on_off)
+		regulator_disable(data->vcc);
+
+	gnss_put_device(data->gdev);
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id sirf_of_match[] = {
+	{ .compatible = "fastrax,uc430" },
+	{ .compatible = "linx,r4" },
+	{ .compatible = "wi2wi,w2sg0008i" },
+	{ .compatible = "wi2wi,w2sg0084i" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sirf_of_match);
+#endif
+
+static struct serdev_device_driver sirf_driver = {
+	.driver	= {
+		.name		= "gnss-sirf",
+		.of_match_table	= of_match_ptr(sirf_of_match),
+		.pm		= &sirf_pm_ops,
+	},
+	.probe	= sirf_probe,
+	.remove	= sirf_remove,
+};
+module_serdev_device_driver(sirf_driver);
+
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
+MODULE_DESCRIPTION("SiRFstar GNSS receiver driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.0

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[H. Nikolaus Schaller]

Hi Johan,

> Am 24.04.2018 um 18:34 schrieb Johan Hovold <johan@kernel.org>:
> 
> This series adds a new subsystem for GNSS receivers (e.g. GPS
> receivers).

Great!

> 
> While GNSS receivers are typically accessed using a UART interface they
> often also support other I/O interfaces such as I2C, SPI and USB, while
> yet other devices use iomem or even some form of remote-processor
> messaging (rpmsg).
> 
> The new GNSS subsystem abstracts the underlying interface and provides a
> new "gnss" class type, which exposes a character-device interface (e.g.
> /dev/gnss0) to user space. This allows GNSS receivers to have a
> representation in the Linux device model, something which is important
> not least for power management purposes and which also allows for easy
> detection and (eventually) identification of GNSS devices.
> 
> Note that the character-device interface provides raw access to whatever
> protocol the receiver is (currently) using, such as NMEA 0183, UBX or
> SiRF Binary. These protocols are expected to be continued to be handled
> by user space for the time being, even if some hybrid solutions are also
> conceivable (e.g. to have kernel drivers issue management commands).
> 
> This will still allow for better platform integration by allowing GNSS
> devices and their resources (e.g. regulators and enable-gpios) to be
> described by firmware and managed by kernel drivers rather than
> platform-specific scripts and services.
> 
> While the current interface is kept minimal, it could be extended using
> IOCTLs, sysfs or uevents as needs and proper abstraction levels are
> identified and determined (e.g. for device and feature identification).
> 
> Another possible extension is to add generic 1PPS support.

would be nice but not first priority.

> 
> I decided to go with a custom character-device interface rather than
> pretend that these abstract GNSS devices are still TTY devices (e.g.
> /dev/ttyGNSS0). Obviously, modifying line settings or reading modem
> control signals does not make any sense for a device using, say, a
> USB (not USB-serial) or iomem interface. This also means, however, that
> user space would no longer be able to set the line speed to match a new
> port configuration that can be set using the various GNSS protocols when
> the underlying interface is indeed a UART; instead this would need to be
> taken care of by the driver.
> 
> Also note that writes are always synchronous instead of requiring user
> space to call tcdrain() after every command.
> 
> This all seems to work well-enough (e.g. with gpsd), but please let me
> know if I've overlooked something which would indeed require a TTY
> interface instead.

tcgetattr() / tcsetattr() for controlling line disciplines, e.g.
stty -icanon </dev/ttyGNSS0 do not work.

It may be possible to use non-blocking I/O or select() or threads
to achieve the same result but may be more complex.

But gpsd seems not to make use of it (or has a built-in fallback - I
have never looked into its code).

> 
> As proof-of-concept I have implemented drivers for receivers based on
> two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> hardware these have so far only been tested using mockup devices and a
> USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> you've got any evalutation kits to spare. ]

Ok, those drivers look nice on first glance.

BTW: I have refactored our w2sg00x4 driver into a gps-core (still creating
a /dev/GPS0) and a driver using a common API.

With that it should almost fit by coping some lines from your serdev based
device drivers.

I haven't found time to submit anything, so it is just living (and working)
in our private tree (please ignore w2cbw003 and trs3386 stuff):

http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/letux-base/hns/misc/w2sg-serdev-v6
http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=164c13373462596cabdd3b5308e5f7d8626a05af


> 
> Finally, note that documentation (including kerneldoc) remains to be
> written, but hopefully this will not hinder review given that the
> current interfaces are fairly self-describing.

I'll apply your patches and try to adjust our w2sg driver. And report
issues if they arise.

BR and thanks,
Nikolaus


> 
> Johan
> 
> 
> Johan Hovold (7):
>  gnss: add GNSS receiver subsystem
>  dt-bindings: add generic gnss binding
>  gnss: add generic serial driver
>  dt-bindings: gnss: add u-blox binding
>  gnss: add driver for u-blox receivers
>  dt-bindings: gnss: add sirfstar binding
>  gnss: add driver for sirfstar-based receivers
> 
> .../devicetree/bindings/gnss/gnss.txt         |  36 ++
> .../devicetree/bindings/gnss/sirfstar.txt     |  38 ++
> .../devicetree/bindings/gnss/u-blox.txt       |  31 ++
> .../devicetree/bindings/vendor-prefixes.txt   |   4 +
> MAINTAINERS                                   |   7 +
> drivers/Kconfig                               |   2 +
> drivers/Makefile                              |   1 +
> drivers/gnss/Kconfig                          |  43 ++
> drivers/gnss/Makefile                         |  16 +
> drivers/gnss/core.c                           | 385 ++++++++++++++++
> drivers/gnss/serial.c                         | 288 ++++++++++++
> drivers/gnss/serial.h                         |  47 ++
> drivers/gnss/sirf.c                           | 415 ++++++++++++++++++
> drivers/gnss/ubx.c                            | 133 ++++++
> include/linux/gnss.h                          |  64 +++
> 15 files changed, 1510 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gnss/gnss.txt
> create mode 100644 Documentation/devicetree/bindings/gnss/sirfstar.txt
> create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> create mode 100644 drivers/gnss/Kconfig
> create mode 100644 drivers/gnss/Makefile
> create mode 100644 drivers/gnss/core.c
> create mode 100644 drivers/gnss/serial.c
> create mode 100644 drivers/gnss/serial.h
> create mode 100644 drivers/gnss/sirf.c
> create mode 100644 drivers/gnss/ubx.c
> create mode 100644 include/linux/gnss.h
> 
> -- 
> 2.17.0
> 

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Tue, Apr 24, 2018 at 07:40:00PM +0200, H. Nikolaus Schaller wrote:
> Hi Johan,
> 
> > Am 24.04.2018 um 18:34 schrieb Johan Hovold <johan@kernel.org>:

> > As proof-of-concept I have implemented drivers for receivers based on
> > two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> > hardware these have so far only been tested using mockup devices and a
> > USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> > you've got any evalutation kits to spare. ]
> 
> Ok, those drivers look nice on first glance.
> 
> BTW: I have refactored our w2sg00x4 driver into a gps-core (still creating
> a /dev/GPS0) and a driver using a common API.
> 
> With that it should almost fit by coping some lines from your serdev based
> device drivers.

I think it should be done the other way round (if I understand you
correctly), that is, by adding support for configurations were WAKEUP is
left not connected to the sirf driver instead. I had that use-case in
mind when implementing the driver, and some ideas of how it should be
done, but did not get around to actually implement it yet.

Thanks,
Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[H. Nikolaus Schaller]

> Am 24.04.2018 um 19:50 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Tue, Apr 24, 2018 at 07:40:00PM +0200, H. Nikolaus Schaller wrote:
>> Hi Johan,
>> 
>>> Am 24.04.2018 um 18:34 schrieb Johan Hovold <johan@kernel.org>:
> 
>>> As proof-of-concept I have implemented drivers for receivers based on
>>> two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
>>> hardware these have so far only been tested using mockup devices and a
>>> USB-serial-based GPS device (using out-of-tree code). [ Let me know if
>>> you've got any evalutation kits to spare. ]
>> 
>> Ok, those drivers look nice on first glance.
>> 
>> BTW: I have refactored our w2sg00x4 driver into a gps-core (still creating
>> a /dev/GPS0) and a driver using a common API.
>> 
>> With that it should almost fit by coping some lines from your serdev based
>> device drivers.
> 
> I think it should be done the other way round (if I understand you
> correctly), that is, by adding support for configurations were WAKEUP is
> left not connected to the sirf driver instead.

Hm. Yes, the w2sg00x4 is a Sirf based chip.

> I had that use-case in mind when implementing

s/implementing/reinventing/

> the driver, and some ideas of how it should be
> done, but did not get around to actually implement it yet.

What do you need ideas for? We have that function working and
submitted year after year, but it was always rejected for API
reasons.

You could have simply reused what we have proposed [1] and just
adapt it to the new API instead of writing a new driver (which
is missing some features for us).

"proof-of-concept" is misleading if you expect this to become
*the* Sirf driver and we are just invited to add some features
to that. Making our own work and proposals completely obsolete.

What I find really strange and foul play is that we are in the
review process and then comes a hidden counter-proposal by the
reviewer.

Well, this is FOSS...

BR,
Nikolaus

[1]: https://patchwork.ozlabs.org/cover/843392/

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Tue, Apr 24, 2018 at 09:44:08PM +0200, H. Nikolaus Schaller wrote:

> > Am 24.04.2018 um 19:50 schrieb Johan Hovold <johan@kernel.org>:

> > I think it should be done the other way round (if I understand you
> > correctly), that is, by adding support for configurations were WAKEUP is
> > left not connected to the sirf driver instead.
> 
> Hm. Yes, the w2sg00x4 is a Sirf based chip.
> 
> > I had that use-case in mind when implementing
> 
> s/implementing/reinventing/
> 
> > the driver, and some ideas of how it should be
> > done, but did not get around to actually implement it yet.
> 
> What do you need ideas for? We have that function working and
> submitted year after year, but it was always rejected for API
> reasons.
> 
> You could have simply reused what we have proposed [1] and just
> adapt it to the new API instead of writing a new driver (which
> is missing some features for us).

Your code was broken or needed to be updated in several ways as I
pointed out in the thread you refer to.

It also did not support all those systems that use the same family of
chips, but which has the WAKEUP signal connected.

> "proof-of-concept" is misleading if you expect this to become
> *the* Sirf driver and we are just invited to add some features
> to that. Making our own work and proposals completely obsolete.
> 
> What I find really strange and foul play is that we are in the
> review process and then comes a hidden counter-proposal by the
> reviewer.

Dude, in the very same thread you refer to above, after being asked to
reiterate your proposal to find and appropriate abstraction level you
reply:

	"Yes, please feel free to write patches that implement it that
	way."

Now I've done just that for you, and then you whine about that too.

SiRF is a very common chip and I wanted to make sure that the common
setup with WAKEUP connected was supported from the start. I'll get to
your configuration in time too.

Johan

> [1]: https://patchwork.ozlabs.org/cover/843392/

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[H. Nikolaus Schaller]

Hi Johan,

> Am 25.04.2018 um 10:11 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Tue, Apr 24, 2018 at 09:44:08PM +0200, H. Nikolaus Schaller wrote:
> 
>>> Am 24.04.2018 um 19:50 schrieb Johan Hovold <johan@kernel.org>:
> 
>>> I think it should be done the other way round (if I understand you
>>> correctly), that is, by adding support for configurations were WAKEUP is
>>> left not connected to the sirf driver instead.
>> 
>> Hm. Yes, the w2sg00x4 is a Sirf based chip.
>> 
>>> I had that use-case in mind when implementing
>> 
>> s/implementing/reinventing/
>> 
>>> the driver, and some ideas of how it should be
>>> done, but did not get around to actually implement it yet.
>> 
>> What do you need ideas for? We have that function working and
>> submitted year after year, but it was always rejected for API
>> reasons.
>> 
>> You could have simply reused what we have proposed [1] and just
>> adapt it to the new API instead of writing a new driver (which
>> is missing some features for us).
> 
> Your code was broken or needed to be updated in several ways as I
> pointed out in the thread you refer to.

No, it was not broken (it is in daily use and no practical problems
are known), but surely does not cover some corner-cases (e.g. races).

I said we should focus firstly on the "abstraction level". So I
postponed these code issues from discussion.

In the end you announced to provide a new kernel API which would
make most of the code (all tty stuff) and related problems go away
anyways.

Therefore I simply did wait for this API to come, before fixing the
remaining issues and submitting a new v6.

> It also did not support all those systems that use the same family of
> chips, but which has the WAKEUP signal connected.

That is simply wrong.

It supports them. It just does not make use of the WAKEUP signal, but
uses a different mechanism to detect the power state of the chip.

So our driver is more complete because it gracefully handles a
WAKEUP line (by ignoring it) while yours doesn't handle the opposite
case of a missing WAKEUP.

> 
>> "proof-of-concept" is misleading if you expect this to become
>> *the* Sirf driver and we are just invited to add some features
>> to that. Making our own work and proposals completely obsolete.
>> 
>> What I find really strange and foul play is that we are in the
>> review process and then comes a hidden counter-proposal by the
>> reviewer.
> 
> Dude, in the very same thread you refer to above, after being asked to
> reiterate your proposal to find and appropriate abstraction level you
> reply:
> 
> 	"Yes, please feel free to write patches that implement it that
> 	way."

When looking back, I understand that as that I suggested to add a "proper
abstraction level" (i.e. a gps framework) so that *we* can make use of
it.

I never meant that you should completely replace our driver by an incomplete
solution.

> 
> Now I've done just that for you, and then you whine about that too.

At the end of the thread (which I take as the result of v5 review) you said:

"Yeah, I think this is a dead end. We need some kind of gps framework
with drivers that can implement the device specific bits.

I may have some time to look at little closer at it this week."

And I answered and laid down my plans for a v6:

"Ok, that would be fine if we can get that!

For a minimal set of API I think something like this (following hci_dev) would suffice:

...

If that would wrap all creation of some /dev/ttyGPS0 (or however it is called),
it would fit our needs for a driver and user-space for our system.

And I would be happy to get rid of creating and registering a /dev/ttyGPS0
in the w2sg0004 driver.

Then, the driver will not need to be touched if the GPS framework is improved
in some far future (e.g. to provide some additional ioctl for getting kalman-filtered
position+heading by doing sensor fusion with some iio-based accelerometer/gyro). 

So I am looking forward to some framework for review and integration testing."

I think it was clear and understandable what I expected (a new framework instead
of creating ttys in the driver) and what I did not (a replacement of our
w2sg0004 proposal with a variant with missing features).

The problem I have with this sort of review process is that I now have to
reintegrate the missing pieces of handling the non-WAKEUP case and LNA
control and rfkill into your code.

Instead of stripping down our code and adjusting to your API.

Alternatively I have to wait until you present some code of which you
think it works. Would you also test it? Probably no. Because of lack of hardware.
So I have to test your new code in an area where I have well-hung code being
in daily use.

And your new code may still be incomplete wrt. what our driver provides to users.

I hope that you understand that this is now more work for me in any case...

So in the end I will have:
* a new but still incomplete driver that is a regression compared to our old driver
* even more testing work in addition 5 years urging for acceptance
* no appreciation because you sign off and author everything

Is this a good deal or a reason to contribute to our common project?

If you want an example how to get rid of volunteers contributing, this may
be one...

> 
> SiRF is a very common chip and I wanted to make sure that the common
> setup with WAKEUP connected was supported from the start. I'll get to
> your configuration in time too.

As said, our driver would also work if the WAKEUP line exists, and is
described in DT, but is broken.

So we will probably stay with our out-of the kernel driver for the time being.

BR,
Nikolaus

> [1]: https://patchwork.ozlabs.org/cover/843392/

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Thu, Apr 26, 2018 at 12:10:02PM +0200, H. Nikolaus Schaller wrote:

> > Am 25.04.2018 um 10:11 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > On Tue, Apr 24, 2018 at 09:44:08PM +0200, H. Nikolaus Schaller wrote:
> > 
> >>> Am 24.04.2018 um 19:50 schrieb Johan Hovold <johan@kernel.org>:

> >> You could have simply reused what we have proposed [1] and just
> >> adapt it to the new API instead of writing a new driver (which
> >> is missing some features for us).
> > 
> > Your code was broken or needed to be updated in several ways as I
> > pointed out in the thread you refer to.
> 
> No, it was not broken (it is in daily use and no practical problems
> are known), but surely does not cover some corner-cases (e.g. races).

I'd certainly be sceptical about accepting code into the kernel from
anyone who completely ignores locking and considers concurrency to be a
corner case that he cannot be bothered with.

Sure, out-of-tree code is often sub-par, and that is partly why it
remains out of tree.

> I said we should focus firstly on the "abstraction level". So I
> postponed these code issues from discussion.
> 
> In the end you announced to provide a new kernel API which would
> make most of the code (all tty stuff) and related problems go away
> anyways.
> 
> Therefore I simply did wait for this API to come, before fixing the
> remaining issues and submitting a new v6.

Yes, and that's perfectly reasonable, but I wouldn't want to base my
work on something in that state.

> > It also did not support all those systems that use the same family of
> > chips, but which has the WAKEUP signal connected.
> 
> That is simply wrong.
> 
> It supports them. It just does not make use of the WAKEUP signal, but
> uses a different mechanism to detect the power state of the chip.

Sure, but in a highly inefficient way since without WAKEUP you can
never be certain that the device is in hibernate, but only assume so
after a lengthy (as in seconds) timeout has passed without any I/O.

> So our driver is more complete because it gracefully handles a
> WAKEUP line (by ignoring it) while yours doesn't handle the opposite
> case of a missing WAKEUP.

I never claimed to support your configuration from the start, but I kept
it in mind while implementing the framework in order to facilitate for you.

Heck, besides getting the whole frame work implemented for you --
thereby making this the closest to mainline support for your system that
you've been over the course of the past five years or however long
you've been at it -- there's now also an already integrated generic
sirf driver with all the basics in place (including runtime PM, and not
relying on legacy interfaces) that you can base your work off of.

> >> "proof-of-concept" is misleading if you expect this to become
> >> *the* Sirf driver and we are just invited to add some features
> >> to that. Making our own work and proposals completely obsolete.
> >> 
> >> What I find really strange and foul play is that we are in the
> >> review process and then comes a hidden counter-proposal by the
> >> reviewer.
> > 
> > Dude, in the very same thread you refer to above, after being asked to
> > reiterate your proposal to find and appropriate abstraction level you
> > reply:
> > 
> > 	"Yes, please feel free to write patches that implement it that
> > 	way."
> 
> When looking back, I understand that as that I suggested to add a "proper
> abstraction level" (i.e. a gps framework) so that *we* can make use of
> it.

Yes, you clearly weren't interesting in doing that work yourself.

> I never meant that you should completely replace our driver by an
> incomplete solution.

But surely you didn't expect your work not to require any kind of
integration into a new framework.

> > Now I've done just that for you, and then you whine about that too.
> 
> At the end of the thread (which I take as the result of v5 review) you said:
> 
> "Yeah, I think this is a dead end. We need some kind of gps framework
> with drivers that can implement the device specific bits.
> 
> I may have some time to look at little closer at it this week."
> 
> And I answered and laid down my plans for a v6:
> 
> "Ok, that would be fine if we can get that!
> 
> For a minimal set of API I think something like this (following hci_dev) would suffice:
> 
> ...
> 
> If that would wrap all creation of some /dev/ttyGPS0 (or however it is called),
> it would fit our needs for a driver and user-space for our system.
> 
> And I would be happy to get rid of creating and registering a /dev/ttyGPS0
> in the w2sg0004 driver.
> 
> Then, the driver will not need to be touched if the GPS framework is improved
> in some far future (e.g. to provide some additional ioctl for getting kalman-filtered
> position+heading by doing sensor fusion with some iio-based accelerometer/gyro). 
> 
> So I am looking forward to some framework for review and integration testing."
> 
> I think it was clear and understandable what I expected (a new framework instead
> of creating ttys in the driver) and what I did not (a replacement of our
> w2sg0004 proposal with a variant with missing features).

Sounds like you think you've given me some kind of order that I had to
fulfil. Sorry, but it doesn't work that way.

> The problem I have with this sort of review process is that I now have to
> reintegrate the missing pieces of handling the non-WAKEUP case and LNA
> control and rfkill into your code.

Yes, you're not getting everything for free.

Besides, I have concerns about using rfkill for GNSS (which does not
transmit any radio), and external LNA control is not something that is
a chip feature but rather something that should be handled by the
framework.

So this wouldn't be a simple code dump for you anyway.

> Instead of stripping down our code and adjusting to your API.
> 
> Alternatively I have to wait until you present some code of which you
> think it works. Would you also test it? Probably no. Because of lack
> of hardware.  So I have to test your new code in an area where I have
> well-hung code being in daily use.

Of course I'll test it, but I can't say solving your problems for you
again is at the top of my list, if it ever were.

> And your new code may still be incomplete wrt. what our driver
> provides to users.
> 
> I hope that you understand that this is now more work for me in any
> case...

I fail to see how not having to implement a GNSS framework can be
considered more work for you.

> So in the end I will have:
> * a new but still incomplete driver that is a regression compared to
> our old driver

It's not a regression, the new mainline driver just wouldn't support
your configuration yet.

> * even more testing work in addition 5 years urging for acceptance
> * no appreciation because you sign off and author everything

You'd sign off on your own work (or was it really Neil Brown who
implemented the driver that you've since been adapting?).

There's still room for adding support for sirf configurations without
WAKEUP, generic LNA support, and possibly rfkill.

> Is this a good deal or a reason to contribute to our common project?
> 
> If you want an example how to get rid of volunteers contributing, this may
> be one...

Come on...

> > SiRF is a very common chip and I wanted to make sure that the common
> > setup with WAKEUP connected was supported from the start. I'll get to
> > your configuration in time too.
> 
> As said, our driver would also work if the WAKEUP line exists, and is
> described in DT, but is broken.
> 
> So we will probably stay with our out-of the kernel driver for the
> time being.

Ok.

Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

Hi!

> This series adds a new subsystem for GNSS receivers (e.g. GPS
> receivers).

Actually... I'd just call it GPS subsystem. Yes, GPS is a bit
misleading, but so is GNSS. We'd like Loran to use similar interface,
right? We'd like to QZSS to use similar interface, too...

https://www.pcworld.com/article/205325/japan_launches_its_first_gps_satellite.html
. QZSS is not _global_ positioning system. Still they call it GPS. I'd
call it GPS too.

(Alternatively, we could have drivers/position and /dev/pos0...)

Looking closer... I'm not sure if it makes sense to push different
protocols (SiRF, NMEA, ...) through one device. Userland should know
what protocol to expect... Yes, there will be common code between
/dev/nmea0 and /dev/sirf0...

I don't know. I'd really like to see '/dev/input/event0'-like layer,
so that userland would not need to know about different protocols. But
your work solves some problems we have now...

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

On Tue, 24 Apr 2018 22:13:19 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > This series adds a new subsystem for GNSS receivers (e.g. GPS
> > receivers).  
> 
> Actually... I'd just call it GPS subsystem. Yes, GPS is a bit
> misleading, but so is GNSS. We'd like Loran to use similar interface,
> right? We'd like to QZSS to use similar interface, too...
> 
> https://www.pcworld.com/article/205325/japan_launches_its_first_gps_satellite.html
> . QZSS is not _global_ positioning system. Still they call it GPS. I'd
> call it GPS too.
> 
> (Alternatively, we could have drivers/position and /dev/pos0...)
> 
> Looking closer... I'm not sure if it makes sense to push different
> protocols (SiRF, NMEA, ...) through one device. Userland should know
> what protocol to expect... Yes, there will be common code between
> /dev/nmea0 and /dev/sirf0...
> 
> I don't know. I'd really like to see '/dev/input/event0'-like layer,
> so that userland would not need to know about different protocols. But
> your work solves some problems we have now...
> 
I am not really sure what to do here. The question is if we can remove
nmea parsing from userspace if the kernel does it? There is the
use-case of having external loggers storing nmea data and userspace
will access the logger data and needs to have nmea parsing for that
anyway.

But for other more exotic stuff, it would be helpful that the user
space does not need to handle the differences.

Hmm, maybe userspace could register something like uinput devices for
having more complex calculation. Maybe triangulating using gsm cell
reception data. And the uinput-like device would have properties
attached like accuracy, costs.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Tue, Apr 24, 2018 at 10:59:48PM +0200, Andreas Kemnade wrote:
> On Tue, 24 Apr 2018 22:13:19 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Hi!
> > 
> > > This series adds a new subsystem for GNSS receivers (e.g. GPS
> > > receivers).  
> > 
> > Actually... I'd just call it GPS subsystem. Yes, GPS is a bit
> > misleading, but so is GNSS. We'd like Loran to use similar interface,
> > right? We'd like to QZSS to use similar interface, too...
> > 
> > https://www.pcworld.com/article/205325/japan_launches_its_first_gps_satellite.html
> > . QZSS is not _global_ positioning system. Still they call it GPS. I'd
> > call it GPS too.
> > 
> > (Alternatively, we could have drivers/position and /dev/pos0...)
> > 
> > Looking closer... I'm not sure if it makes sense to push different
> > protocols (SiRF, NMEA, ...) through one device. Userland should know
> > what protocol to expect... Yes, there will be common code between
> > /dev/nmea0 and /dev/sirf0...
> > 
> > I don't know. I'd really like to see '/dev/input/event0'-like layer,
> > so that userland would not need to know about different protocols. But
> > your work solves some problems we have now...
> > 
> I am not really sure what to do here. The question is if we can remove
> nmea parsing from userspace if the kernel does it? There is the
> use-case of having external loggers storing nmea data and userspace
> will access the logger data and needs to have nmea parsing for that
> anyway.
> 
> But for other more exotic stuff, it would be helpful that the user
> space does not need to handle the differences.
> 
> Hmm, maybe userspace could register something like uinput devices for
> having more complex calculation. Maybe triangulating using gsm cell
> reception data. And the uinput-like device would have properties
> attached like accuracy, costs.

There's also seems to be trend towards exposing raw measurements and
experimenting with different Position-Velocity-Time algorithms in user
space.

But in any case, moving gpsd into the kernel is not my aim here.

With a GNSS subsystem in place which would give us device detection and
power management from the start, people can start experimenting with
more features (e.g. device identification, feature flags, or possibly
higher-level protocol interfaces).

Thanks,
Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Marcel Holtmann]

Hi Pavel,

>> This series adds a new subsystem for GNSS receivers (e.g. GPS
>> receivers).
> 
> Actually... I'd just call it GPS subsystem. Yes, GPS is a bit
> misleading, but so is GNSS. We'd like Loran to use similar interface,
> right? We'd like to QZSS to use similar interface, too…

GNSS is the term that is known and used by everybody in the industry to identify GPS, GLONASS, Galileo etc.

> https://www.pcworld.com/article/205325/japan_launches_its_first_gps_satellite.html
> . QZSS is not _global_ positioning system. Still they call it GPS. I'd
> call it GPS too.
> 
> (Alternatively, we could have drivers/position and /dev/pos0...)
> 
> Looking closer... I'm not sure if it makes sense to push different
> protocols (SiRF, NMEA, ...) through one device. Userland should know
> what protocol to expect... Yes, there will be common code between
> /dev/nmea0 and /dev/sirf0…

Device node naming is pointless, use DEVTYPE uevent property for identifying the expected protocol.

Regards

Marcel

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Tue, Apr 24, 2018 at 10:13:19PM +0200, Pavel Machek wrote:
> Hi!
> 
> > This series adds a new subsystem for GNSS receivers (e.g. GPS
> > receivers).
> 
> Actually... I'd just call it GPS subsystem. Yes, GPS is a bit
> misleading, but so is GNSS. We'd like Loran to use similar interface,
> right? We'd like to QZSS to use similar interface, too...
> 
> https://www.pcworld.com/article/205325/japan_launches_its_first_gps_satellite.html
> . QZSS is not _global_ positioning system. Still they call it GPS. I'd
> call it GPS too.

What Marcel said. Never heard of Loran, but apparently it's no longer in
use:

	https://en.wikipedia.org/wiki/Radio_navigation#Satellite_navigation

> (Alternatively, we could have drivers/position and /dev/pos0...)

If you find such a system in use and implement a driver for it, we'll
just let it be the odd bird.

> Looking closer... I'm not sure if it makes sense to push different
> protocols (SiRF, NMEA, ...) through one device. Userland should know
> what protocol to expect... Yes, there will be common code between
> /dev/nmea0 and /dev/sirf0...

That's not how GNSS devices work. It does not seem to be uncommon to
switch to a vendor protocol with a richer feature set and back to NMEA
(e.g. for configuration). Raw GNSS data may also be available over the
vendor protocol, etc. And some devices even support using to protocols
concurrently on one port.

I was going to call the device node /dev/gnssraw0 (cf. hidraw), but
since "raw" GNSS measurement already has a meaning in this space I
decided to drop that suffix. It can all be accessed over /dev/gnss0.

> I don't know. I'd really like to see '/dev/input/event0'-like layer,
> so that userland would not need to know about different protocols. But
> your work solves some problems we have now...

Yeah, and moving gpsd into the kernel is probably never going to happen.
But if it were, we probably wouldn't be using a character device to
access it anyway.

Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]


> As proof-of-concept I have implemented drivers for receivers based on
> two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> hardware these have so far only been tested using mockup devices and a
> USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> you've got any evalutation kits to spare. ]

Do you have easy access to Nokia N900 or Motorola Droid 4? They should
be reasonably cheap, and each provides unique challenge.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi!

> This series adds a new subsystem for GNSS receivers (e.g. GPS
> receivers).
> 
> While GNSS receivers are typically accessed using a UART interface they
> often also support other I/O interfaces such as I2C, SPI and USB, while
> yet other devices use iomem or even some form of remote-processor
> messaging (rpmsg).
>     
> The new GNSS subsystem abstracts the underlying interface and provides a
> new "gnss" class type, which exposes a character-device interface (e.g.
> /dev/gnss0) to user space. This allows GNSS receivers to have a
> representation in the Linux device model, something which is important
> not least for power management purposes and which also allows for easy
> detection and (eventually) identification of GNSS devices.

Actually... no, what you done is not GNSS subsystem. It is generic
subsystem for a link + some controls, typically for power.

Yes, that setup is common for GPS receivers, but it is there also for
modems (droid 4: usb + gpios) and bluetooth dongles (n900: serial +
gpios).

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 08:26:59AM +0200, Pavel Machek wrote:
> Hi!
> 
> > This series adds a new subsystem for GNSS receivers (e.g. GPS
> > receivers).
> > 
> > While GNSS receivers are typically accessed using a UART interface they
> > often also support other I/O interfaces such as I2C, SPI and USB, while
> > yet other devices use iomem or even some form of remote-processor
> > messaging (rpmsg).
> >     
> > The new GNSS subsystem abstracts the underlying interface and provides a
> > new "gnss" class type, which exposes a character-device interface (e.g.
> > /dev/gnss0) to user space. This allows GNSS receivers to have a
> > representation in the Linux device model, something which is important
> > not least for power management purposes and which also allows for easy
> > detection and (eventually) identification of GNSS devices.
> 
> Actually... no, what you done is not GNSS subsystem. It is generic
> subsystem for a link + some controls, typically for power.
> 
> Yes, that setup is common for GPS receivers, but it is there also for
> modems (droid 4: usb + gpios) and bluetooth dongles (n900: serial +
> gpios).

Those would be handled by other subsystems.

The idea here is that eventually there may be more GNSS specific bits
added, such as 1PPS support that I already mentioned. If someone is
crazy enough to put a location service in the kernel with a more
high-level interface (an in-kernel gpsd) they can try, but I'm more and
more convinced that needs to remain in user space.

I did also already mentioned however, that some hybrid approach may be
doable, such as handling port setup over NMEA/UBX/SiRF from within the
kernel.

Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Greg Kroah-Hartman]

On Tue, Apr 24, 2018 at 06:34:51PM +0200, Johan Hovold wrote:
> This series adds a new subsystem for GNSS receivers (e.g. GPS
> receivers).

YEAH!!!!

Thanks so much for doing this, great work!

> While GNSS receivers are typically accessed using a UART interface they
> often also support other I/O interfaces such as I2C, SPI and USB, while
> yet other devices use iomem or even some form of remote-processor
> messaging (rpmsg).
>     
> The new GNSS subsystem abstracts the underlying interface and provides a
> new "gnss" class type, which exposes a character-device interface (e.g.
> /dev/gnss0) to user space. This allows GNSS receivers to have a
> representation in the Linux device model, something which is important
> not least for power management purposes and which also allows for easy
> detection and (eventually) identification of GNSS devices.
> 
> Note that the character-device interface provides raw access to whatever
> protocol the receiver is (currently) using, such as NMEA 0183, UBX or
> SiRF Binary. These protocols are expected to be continued to be handled
> by user space for the time being, even if some hybrid solutions are also
> conceivable (e.g. to have kernel drivers issue management commands).
> 
> This will still allow for better platform integration by allowing GNSS
> devices and their resources (e.g. regulators and enable-gpios) to be
> described by firmware and managed by kernel drivers rather than
> platform-specific scripts and services.
>     
> While the current interface is kept minimal, it could be extended using
> IOCTLs, sysfs or uevents as needs and proper abstraction levels are
> identified and determined (e.g. for device and feature identification).
> 
> Another possible extension is to add generic 1PPS support.
> 
> I decided to go with a custom character-device interface rather than
> pretend that these abstract GNSS devices are still TTY devices (e.g.
> /dev/ttyGNSS0). Obviously, modifying line settings or reading modem
> control signals does not make any sense for a device using, say, a
> USB (not USB-serial) or iomem interface. This also means, however, that
> user space would no longer be able to set the line speed to match a new
> port configuration that can be set using the various GNSS protocols when
> the underlying interface is indeed a UART; instead this would need to be
> taken care of by the driver.
> 
> Also note that writes are always synchronous instead of requiring user
> space to call tcdrain() after every command.
> 
> This all seems to work well-enough (e.g. with gpsd), but please let me
> know if I've overlooked something which would indeed require a TTY
> interface instead.
> 
> As proof-of-concept I have implemented drivers for receivers based on
> two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> hardware these have so far only been tested using mockup devices and a
> USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> you've got any evalutation kits to spare. ]
> 
> Finally, note that documentation (including kerneldoc) remains to be
> written, but hopefully this will not hinder review given that the
> current interfaces are fairly self-describing.

Is this just a RFC, or a "here's a first cut at submitting this, review
it please!" submission?  I'm glad to review if you think it's worth it
at this point.

thanks,

greg k-h

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 10:48:19AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 24, 2018 at 06:34:51PM +0200, Johan Hovold wrote:
> > This series adds a new subsystem for GNSS receivers (e.g. GPS
> > receivers).
> 
> YEAH!!!!
> 
> Thanks so much for doing this, great work!

Thanks!

> > This all seems to work well-enough (e.g. with gpsd), but please let me
> > know if I've overlooked something which would indeed require a TTY
> > interface instead.
> > 
> > As proof-of-concept I have implemented drivers for receivers based on
> > two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> > hardware these have so far only been tested using mockup devices and a
> > USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> > you've got any evalutation kits to spare. ]
> > 
> > Finally, note that documentation (including kerneldoc) remains to be
> > written, but hopefully this will not hinder review given that the
> > current interfaces are fairly self-describing.
> 
> Is this just a RFC, or a "here's a first cut at submitting this, review
> it please!" submission?  I'm glad to review if you think it's worth it
> at this point.

It's more than just an RFC, so thanks for starting to review it.

Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4762 bytes --]

Hi Johan,

On Tue, Apr 24, 2018 at 06:34:51PM +0200, Johan Hovold wrote:
> This series adds a new subsystem for GNSS receivers (e.g. GPS
> receivers).
> 
> While GNSS receivers are typically accessed using a UART interface they
> often also support other I/O interfaces such as I2C, SPI and USB, while
> yet other devices use iomem or even some form of remote-processor
> messaging (rpmsg).
>     
> The new GNSS subsystem abstracts the underlying interface and provides a
> new "gnss" class type, which exposes a character-device interface (e.g.
> /dev/gnss0) to user space. This allows GNSS receivers to have a
> representation in the Linux device model, something which is important
> not least for power management purposes and which also allows for easy
> detection and (eventually) identification of GNSS devices.
> 
> Note that the character-device interface provides raw access to whatever
> protocol the receiver is (currently) using, such as NMEA 0183, UBX or
> SiRF Binary. These protocols are expected to be continued to be handled
> by user space for the time being, even if some hybrid solutions are also
> conceivable (e.g. to have kernel drivers issue management commands).
>
> This will still allow for better platform integration by allowing GNSS
> devices and their resources (e.g. regulators and enable-gpios) to be
> described by firmware and managed by kernel drivers rather than
> platform-specific scripts and services.
>     
> While the current interface is kept minimal, it could be extended using
> IOCTLs, sysfs or uevents as needs and proper abstraction levels are
> identified and determined (e.g. for device and feature identification).
> 
> Another possible extension is to add generic 1PPS support.
> 
> I decided to go with a custom character-device interface rather than
> pretend that these abstract GNSS devices are still TTY devices (e.g.
> /dev/ttyGNSS0). Obviously, modifying line settings or reading modem
> control signals does not make any sense for a device using, say, a
> USB (not USB-serial) or iomem interface. This also means, however, that
> user space would no longer be able to set the line speed to match a new
> port configuration that can be set using the various GNSS protocols when
> the underlying interface is indeed a UART; instead this would need to be
> taken care of by the driver.
> 
> Also note that writes are always synchronous instead of requiring user
> space to call tcdrain() after every command.
> 
> This all seems to work well-enough (e.g. with gpsd), but please let me
> know if I've overlooked something which would indeed require a TTY
> interface instead.
> 
> As proof-of-concept I have implemented drivers for receivers based on
> two common GNSS chipsets (SiRFstar and u-blox), but due to lack of
> hardware these have so far only been tested using mockup devices and a
> USB-serial-based GPS device (using out-of-tree code). [ Let me know if
> you've got any evalutation kits to spare. ]
> 
> Finally, note that documentation (including kerneldoc) remains to be
> written, but hopefully this will not hinder review given that the
> current interfaces are fairly self-describing.

Great work. I like your design decisions. I have quite a few devices
with have non-serial based GPS peripherals using binary protocols (*).
As far as I can see it should be possible to add support for those.

I have one concern, though. While providing raw data by
default is fine generally, it is a problem with device
auto-discovery. I think there should be some IOCTL from
the start, that can be used to inform userspace about
the raw protocol being used (i.e. "NMEA"). I fear, that
userspace may start to just assume raw = NMEA without
having this (especially since all initial drivers provide
NMEA).

(*) I have those in mind:

Nokia N900: The phone has GPS integrated into the modem and uses
ISI encapsulated data. The protocol has been reverse engineered
and it should be possible to write a kernel driver for handling
the GPS packets and dumping the raw data to /dev/gnss0. I don't
think this is particularly useful without a non-raw interface,
though. It would still require a custom userspace implementation.

Nokia N950/N9: Those phones have an I2C connected BCM4751, which
uses (proprietary, non-reverse-engineered) MEIF binary protocol.
Nokia's kernel had a driver, which provides a similar userspace
interface (char device with raw data from chip).

Droid 4: GPS is similar to N900, but different protocol and QMI
encapsulated. This one also has known protocol with userspace
implementation. I did not yet have a detailed look, if its possible
to (un)wrap this in the kernel.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

Hi!

> > Finally, note that documentation (including kerneldoc) remains to be
> > written, but hopefully this will not hinder review given that the
> > current interfaces are fairly self-describing.
> 
> Great work. I like your design decisions. I have quite a few devices
> with have non-serial based GPS peripherals using binary protocols (*).
> As far as I can see it should be possible to add support for those.
> 
> I have one concern, though. While providing raw data by
> default is fine generally, it is a problem with device
> auto-discovery. I think there should be some IOCTL from
> the start, that can be used to inform userspace about
> the raw protocol being used (i.e. "NMEA"). I fear, that
> userspace may start to just assume raw = NMEA without
> having this (especially since all initial drivers provide
> NMEA).

Yep, that would be nice.

> (*) I have those in mind:
> 
> Nokia N900: The phone has GPS integrated into the modem and uses
> ISI encapsulated data. The protocol has been reverse engineered
> and it should be possible to write a kernel driver for handling
> the GPS packets and dumping the raw data to /dev/gnss0. I don't
> think this is particularly useful without a non-raw interface,
> though. It would still require a custom userspace implementation.

Actually... in this case it would be nice to do the protocol
processing in kernel and just present reasonable interface to
userland... or maybe NMEA.

> Droid 4: GPS is similar to N900, but different protocol and QMI
> encapsulated. This one also has known protocol with userspace
> implementation. I did not yet have a detailed look, if its possible
> to (un)wrap this in the kernel.

So, this is actually NMEA over QMI. I do have patches libqmi that
provides NMEA on stdout.

But there seems to be another possibile interface (yes, that modem is
crazy, and you can talk to it over few different interfaces), and
that's NMEA over GSM07.10. That one should be feasible to decode in
kernel and just provide NMEA to userland.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

Hi,

On Fri, May 04, 2018 at 10:03:15PM +0200, Pavel Machek wrote:
> > > Finally, note that documentation (including kerneldoc) remains to be
> > > written, but hopefully this will not hinder review given that the
> > > current interfaces are fairly self-describing.
> > 
> > Great work. I like your design decisions. I have quite a few devices
> > with have non-serial based GPS peripherals using binary protocols (*).
> > As far as I can see it should be possible to add support for those.
> > 
> > I have one concern, though. While providing raw data by
> > default is fine generally, it is a problem with device
> > auto-discovery. I think there should be some IOCTL from
> > the start, that can be used to inform userspace about
> > the raw protocol being used (i.e. "NMEA"). I fear, that
> > userspace may start to just assume raw = NMEA without
> > having this (especially since all initial drivers provide
> > NMEA).
> 
> Yep, that would be nice.
> 
> > (*) I have those in mind:
> > 
> > Nokia N900: The phone has GPS integrated into the modem and uses
> > ISI encapsulated data. The protocol has been reverse engineered
> > and it should be possible to write a kernel driver for handling
> > the GPS packets and dumping the raw data to /dev/gnss0. I don't
> > think this is particularly useful without a non-raw interface,
> > though. It would still require a custom userspace implementation.
> 
> Actually... in this case it would be nice to do the protocol
> processing in kernel and just present reasonable interface to
> userland... or maybe NMEA.

Converting a binary protocol to NMEA, which needs to be string
parsed is not very nice. I think first step would be to write a
simple driver, which exposes the binary location protocol without
ISI header. Then in a second step we can think about a reasonable
interface, that should be supported by all GNSS devices.

> > Droid 4: GPS is similar to N900, but different protocol and QMI
> > encapsulated. This one also has known protocol with userspace
> > implementation. I did not yet have a detailed look, if its possible
> > to (un)wrap this in the kernel.
> 
> So, this is actually NMEA over QMI. I do have patches libqmi that
> provides NMEA on stdout.

Ok. So raw data is NMEA for this one. Should be reasonably easy to
write a driver exposing this via /dev/gnss device.

> But there seems to be another possibile interface (yes, that modem is
> crazy, and you can talk to it over few different interfaces), and
> that's NMEA over GSM07.10. That one should be feasible to decode in
> kernel and just provide NMEA to userland.

I think both should be feasible. I suggest to wait a bit more
until you and Tony figured out some more details. You've got
the libqmi patch as workaround for now and we want a stable API
later.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2664 bytes --]

Hi!

> > > (*) I have those in mind:
> > > 
> > > Nokia N900: The phone has GPS integrated into the modem and uses
> > > ISI encapsulated data. The protocol has been reverse engineered
> > > and it should be possible to write a kernel driver for handling
> > > the GPS packets and dumping the raw data to /dev/gnss0. I don't
> > > think this is particularly useful without a non-raw interface,
> > > though. It would still require a custom userspace implementation.
> > 
> > Actually... in this case it would be nice to do the protocol
> > processing in kernel and just present reasonable interface to
> > userland... or maybe NMEA.
> 
> Converting a binary protocol to NMEA, which needs to be string
> parsed is not very nice. I think first step would be to write a
> simple driver, which exposes the binary location protocol without
> ISI header. Then in a second step we can think about a reasonable
> interface, that should be supported by all GNSS devices.

Well, most of the userspace expects NMEA. So yes, its an ugly
protocol, but .. it is not really a high-performance device.

I'd suggest modeling this over input subsystem. We could use similar
tag/value/sync protocol (evdev). In similar way /dev/input/mice was
used for running legacy apps during transition, we'd have /dev/...//nmea.

> > > Droid 4: GPS is similar to N900, but different protocol and QMI
> > > encapsulated. This one also has known protocol with userspace
> > > implementation. I did not yet have a detailed look, if its possible
> > > to (un)wrap this in the kernel.
> > 
> > So, this is actually NMEA over QMI. I do have patches libqmi that
> > provides NMEA on stdout.
> 
> Ok. So raw data is NMEA for this one. Should be reasonably easy to
> write a driver exposing this via /dev/gnss device.

If we have qmi implementation somewhere in kernel. Do we?

> > But there seems to be another possibile interface (yes, that modem is
> > crazy, and you can talk to it over few different interfaces), and
> > that's NMEA over GSM07.10. That one should be feasible to decode in
> > kernel and just provide NMEA to userland.
> 
> I think both should be feasible. I suggest to wait a bit more
> until you and Tony figured out some more details. You've got
> the libqmi patch as workaround for now and we want a stable API
> later.

I do have the gsm07.10 one now, too. That one is certainly simple
enough (and should eat less power -- no need to keep USB running).

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Marcel Holtmann]

Hi Pavel,

>>>> (*) I have those in mind:
>>>> 
>>>> Nokia N900: The phone has GPS integrated into the modem and uses
>>>> ISI encapsulated data. The protocol has been reverse engineered
>>>> and it should be possible to write a kernel driver for handling
>>>> the GPS packets and dumping the raw data to /dev/gnss0. I don't
>>>> think this is particularly useful without a non-raw interface,
>>>> though. It would still require a custom userspace implementation.
>>> 
>>> Actually... in this case it would be nice to do the protocol
>>> processing in kernel and just present reasonable interface to
>>> userland... or maybe NMEA.
>> 
>> Converting a binary protocol to NMEA, which needs to be string
>> parsed is not very nice. I think first step would be to write a
>> simple driver, which exposes the binary location protocol without
>> ISI header. Then in a second step we can think about a reasonable
>> interface, that should be supported by all GNSS devices.
> 
> Well, most of the userspace expects NMEA. So yes, its an ugly
> protocol, but .. it is not really a high-performance device.
> 
> I'd suggest modeling this over input subsystem. We could use similar
> tag/value/sync protocol (evdev). In similar way /dev/input/mice was
> used for running legacy apps during transition, we'd have /dev/...//nmea.

I would _not_ model it after the input system. And /dev/input/mice was a really bad design choice. It was there because userspace was too stupid.

>>>> Droid 4: GPS is similar to N900, but different protocol and QMI
>>>> encapsulated. This one also has known protocol with userspace
>>>> implementation. I did not yet have a detailed look, if its possible
>>>> to (un)wrap this in the kernel.
>>> 
>>> So, this is actually NMEA over QMI. I do have patches libqmi that
>>> provides NMEA on stdout.
>> 
>> Ok. So raw data is NMEA for this one. Should be reasonably easy to
>> write a driver exposing this via /dev/gnss device.
> 
> If we have qmi implementation somewhere in kernel. Do we?

Not in a way that it can easily be hooked up, but essentially the QMUX part and everything around it needs to be done in the kernel. Like Phonet and CAIF have been done before that.

>>> But there seems to be another possibile interface (yes, that modem is
>>> crazy, and you can talk to it over few different interfaces), and
>>> that's NMEA over GSM07.10. That one should be feasible to decode in
>>> kernel and just provide NMEA to userland.
>> 
>> I think both should be feasible. I suggest to wait a bit more
>> until you and Tony figured out some more details. You've got
>> the libqmi patch as workaround for now and we want a stable API
>> later.
> 
> I do have the gsm07.10 one now, too. That one is certainly simple
> enough (and should eat less power -- no need to keep USB running).

Someone also needs to make the GSM 07.10 multiplexer code serdev aware. Since currently it is a line discipline and that is not really needed. In case this is a USB serial, it might be better to be hooked up directly into GSM 07.10 without even going through serdev.

Regards

Marcel

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Fri, May 04, 2018 at 03:27:41PM +0200, Sebastian Reichel wrote:
> Hi Johan,
> 
> On Tue, Apr 24, 2018 at 06:34:51PM +0200, Johan Hovold wrote:
> > This series adds a new subsystem for GNSS receivers (e.g. GPS
> > receivers).
> > 
> > While GNSS receivers are typically accessed using a UART interface they
> > often also support other I/O interfaces such as I2C, SPI and USB, while
> > yet other devices use iomem or even some form of remote-processor
> > messaging (rpmsg).
> >     
> > The new GNSS subsystem abstracts the underlying interface and provides a
> > new "gnss" class type, which exposes a character-device interface (e.g.
> > /dev/gnss0) to user space. This allows GNSS receivers to have a
> > representation in the Linux device model, something which is important
> > not least for power management purposes and which also allows for easy
> > detection and (eventually) identification of GNSS devices.
> > 
> > Note that the character-device interface provides raw access to whatever
> > protocol the receiver is (currently) using, such as NMEA 0183, UBX or
> > SiRF Binary. These protocols are expected to be continued to be handled
> > by user space for the time being, even if some hybrid solutions are also
> > conceivable (e.g. to have kernel drivers issue management commands).

> Great work. I like your design decisions. I have quite a few devices
> with have non-serial based GPS peripherals using binary protocols (*).
> As far as I can see it should be possible to add support for those.

Thanks! Yeah, the aim here was to abstract the actual I/O interface
used.

> I have one concern, though. While providing raw data by
> default is fine generally, it is a problem with device
> auto-discovery. I think there should be some IOCTL from
> the start, that can be used to inform userspace about
> the raw protocol being used (i.e. "NMEA"). I fear, that
> userspace may start to just assume raw = NMEA without
> having this (especially since all initial drivers provide
> NMEA).

One problem I see here would be that the driver does not necessarily
know either what protocol is currently being used. Some devices have
boot-pins which can be used to configure the initial protocol used (and
this could perhaps be reflected in DT), but this can often later be
changed (by user space) and even be made persistent using battery-backed
ram or eeproms.

Also note that at least u-blox devices supports having more than one
protocol active on the same port...

Thanks,
Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Marcel Holtmann]

Hi Johan,

>>> This series adds a new subsystem for GNSS receivers (e.g. GPS
>>> receivers).
>>> 
>>> While GNSS receivers are typically accessed using a UART interface they
>>> often also support other I/O interfaces such as I2C, SPI and USB, while
>>> yet other devices use iomem or even some form of remote-processor
>>> messaging (rpmsg).
>>> 
>>> The new GNSS subsystem abstracts the underlying interface and provides a
>>> new "gnss" class type, which exposes a character-device interface (e.g.
>>> /dev/gnss0) to user space. This allows GNSS receivers to have a
>>> representation in the Linux device model, something which is important
>>> not least for power management purposes and which also allows for easy
>>> detection and (eventually) identification of GNSS devices.
>>> 
>>> Note that the character-device interface provides raw access to whatever
>>> protocol the receiver is (currently) using, such as NMEA 0183, UBX or
>>> SiRF Binary. These protocols are expected to be continued to be handled
>>> by user space for the time being, even if some hybrid solutions are also
>>> conceivable (e.g. to have kernel drivers issue management commands).
> 
>> Great work. I like your design decisions. I have quite a few devices
>> with have non-serial based GPS peripherals using binary protocols (*).
>> As far as I can see it should be possible to add support for those.
> 
> Thanks! Yeah, the aim here was to abstract the actual I/O interface
> used.
> 
>> I have one concern, though. While providing raw data by
>> default is fine generally, it is a problem with device
>> auto-discovery. I think there should be some IOCTL from
>> the start, that can be used to inform userspace about
>> the raw protocol being used (i.e. "NMEA"). I fear, that
>> userspace may start to just assume raw = NMEA without
>> having this (especially since all initial drivers provide
>> NMEA).
> 
> One problem I see here would be that the driver does not necessarily
> know either what protocol is currently being used. Some devices have
> boot-pins which can be used to configure the initial protocol used (and
> this could perhaps be reflected in DT), but this can often later be
> changed (by user space) and even be made persistent using battery-backed
> ram or eeproms.
> 
> Also note that at least u-blox devices supports having more than one
> protocol active on the same port...

as long as userspace can determine that it is GNSS hardware and what hardware it is, then you deal with the rest in userspace.

Regards

Marcel

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Mon, May 07, 2018 at 09:06:44PM +0200, Marcel Holtmann wrote:

> >> I have one concern, though. While providing raw data by
> >> default is fine generally, it is a problem with device
> >> auto-discovery. I think there should be some IOCTL from
> >> the start, that can be used to inform userspace about
> >> the raw protocol being used (i.e. "NMEA"). I fear, that
> >> userspace may start to just assume raw = NMEA without
> >> having this (especially since all initial drivers provide
> >> NMEA).
> > 
> > One problem I see here would be that the driver does not necessarily
> > know either what protocol is currently being used. Some devices have
> > boot-pins which can be used to configure the initial protocol used (and
> > this could perhaps be reflected in DT), but this can often later be
> > changed (by user space) and even be made persistent using battery-backed
> > ram or eeproms.
> > 
> > Also note that at least u-blox devices supports having more than one
> > protocol active on the same port...
> 
> as long as userspace can determine that it is GNSS hardware and what
> hardware it is, then you deal with the rest in userspace.

Yeah, I think that will do for now.

Thanks,
Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Marcel Holtmann]

Hi Johan,

>>>> I have one concern, though. While providing raw data by
>>>> default is fine generally, it is a problem with device
>>>> auto-discovery. I think there should be some IOCTL from
>>>> the start, that can be used to inform userspace about
>>>> the raw protocol being used (i.e. "NMEA"). I fear, that
>>>> userspace may start to just assume raw = NMEA without
>>>> having this (especially since all initial drivers provide
>>>> NMEA).
>>> 
>>> One problem I see here would be that the driver does not necessarily
>>> know either what protocol is currently being used. Some devices have
>>> boot-pins which can be used to configure the initial protocol used (and
>>> this could perhaps be reflected in DT), but this can often later be
>>> changed (by user space) and even be made persistent using battery-backed
>>> ram or eeproms.
>>> 
>>> Also note that at least u-blox devices supports having more than one
>>> protocol active on the same port...
>> 
>> as long as userspace can determine that it is GNSS hardware and what
>> hardware it is, then you deal with the rest in userspace.
> 
> Yeah, I think that will do for now.

I forgot to mention that this part should be simple. So providing a DEVTYPE attribute that can be easily associated to the /dev/gnssX nodes is a must have here. Doing complex mapping of USB or DT layouts to figure out this is NMEA vs some vendor vs I need extra code to change the mode etc.

So a proper GNSS daemon will require some hardware abstraction anyway. There will be still a few GNSS devices that are part of the cellular modem, where you have to go via the telephony daemon to get access to it. We have done this within oFono since there would be otherwise no access to it. So only extra pieces that could be done here is to provide a /dev/ugnss (like /dev/uinput, /dev/uhid) so that you can emulate GNSS hardware from userspace as well. In oFono, we could then just hook this up with /dev/ugnss and the GNSS daemon would not have to have to know how to talk to oFono.

Regards

Marcel

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

On Tue, May 08, 2018 at 09:49:36AM +0200, Marcel Holtmann wrote:
> Hi Johan,
> 
> >>>> I have one concern, though. While providing raw data by
> >>>> default is fine generally, it is a problem with device
> >>>> auto-discovery. I think there should be some IOCTL from
> >>>> the start, that can be used to inform userspace about
> >>>> the raw protocol being used (i.e. "NMEA"). I fear, that
> >>>> userspace may start to just assume raw = NMEA without
> >>>> having this (especially since all initial drivers provide
> >>>> NMEA).
> >>> 
> >>> One problem I see here would be that the driver does not necessarily
> >>> know either what protocol is currently being used. Some devices have
> >>> boot-pins which can be used to configure the initial protocol used (and
> >>> this could perhaps be reflected in DT), but this can often later be
> >>> changed (by user space) and even be made persistent using battery-backed
> >>> ram or eeproms.
> >>> 
> >>> Also note that at least u-blox devices supports having more than one
> >>> protocol active on the same port...
> >> 
> >> as long as userspace can determine that it is GNSS hardware and what
> >> hardware it is, then you deal with the rest in userspace.
> > 
> > Yeah, I think that will do for now.
> 
> I forgot to mention that this part should be simple. So providing a
> DEVTYPE attribute that can be easily associated to the /dev/gnssX
> nodes is a must have here. Doing complex mapping of USB or DT layouts
> to figure out this is NMEA vs some vendor vs I need extra code to
> change the mode etc.

Yes, as I mentioned in the cover letter some kind of generic interface
for identifying the device type (and its features) should be defined
eventually.

Note that this is not necessarily something that is needed from the
start however as, for example, gpsd implements protocol detection.

> So a proper GNSS daemon will require some hardware abstraction anyway.
> There will be still a few GNSS devices that are part of the cellular
> modem, where you have to go via the telephony daemon to get access to
> it. We have done this within oFono since there would be otherwise no
> access to it. So only extra pieces that could be done here is to
> provide a /dev/ugnss (like /dev/uinput, /dev/uhid) so that you can
> emulate GNSS hardware from userspace as well. In oFono, we could then
> just hook this up with /dev/ugnss and the GNSS daemon would not have
> to have to know how to talk to oFono.

Interesting idea, could be useful for testing purposes as well. But just
so I understand your use case better, why do you need to go through
oFono rather than implement, say, a serdev driver for the GNSS port of
the modem? To enable the receiver using AT commands on a different port?
Or what kind of interface did you have in mind here?

Thanks,
Johan

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Marcel Holtmann]

Hi Johan,

>>>>>> I have one concern, though. While providing raw data by
>>>>>> default is fine generally, it is a problem with device
>>>>>> auto-discovery. I think there should be some IOCTL from
>>>>>> the start, that can be used to inform userspace about
>>>>>> the raw protocol being used (i.e. "NMEA"). I fear, that
>>>>>> userspace may start to just assume raw = NMEA without
>>>>>> having this (especially since all initial drivers provide
>>>>>> NMEA).
>>>>> 
>>>>> One problem I see here would be that the driver does not necessarily
>>>>> know either what protocol is currently being used. Some devices have
>>>>> boot-pins which can be used to configure the initial protocol used (and
>>>>> this could perhaps be reflected in DT), but this can often later be
>>>>> changed (by user space) and even be made persistent using battery-backed
>>>>> ram or eeproms.
>>>>> 
>>>>> Also note that at least u-blox devices supports having more than one
>>>>> protocol active on the same port...
>>>> 
>>>> as long as userspace can determine that it is GNSS hardware and what
>>>> hardware it is, then you deal with the rest in userspace.
>>> 
>>> Yeah, I think that will do for now.
>> 
>> I forgot to mention that this part should be simple. So providing a
>> DEVTYPE attribute that can be easily associated to the /dev/gnssX
>> nodes is a must have here. Doing complex mapping of USB or DT layouts
>> to figure out this is NMEA vs some vendor vs I need extra code to
>> change the mode etc.
> 
> Yes, as I mentioned in the cover letter some kind of generic interface
> for identifying the device type (and its features) should be defined
> eventually.
> 

I think this needs to be present from the start. Half backed subsystems are dangerous. And I really want to avoid hacking in userspace to figure out details about the hardware.

> Note that this is not necessarily something that is needed from the
> start however as, for example, gpsd implements protocol detection.

That might be, but that is a total hack. Lets get this right from the get-go.

>> So a proper GNSS daemon will require some hardware abstraction anyway.
>> There will be still a few GNSS devices that are part of the cellular
>> modem, where you have to go via the telephony daemon to get access to
>> it. We have done this within oFono since there would be otherwise no
>> access to it. So only extra pieces that could be done here is to
>> provide a /dev/ugnss (like /dev/uinput, /dev/uhid) so that you can
>> emulate GNSS hardware from userspace as well. In oFono, we could then
>> just hook this up with /dev/ugnss and the GNSS daemon would not have
>> to have to know how to talk to oFono.
> 
> Interesting idea, could be useful for testing purposes as well. But just
> so I understand your use case better, why do you need to go through
> oFono rather than implement, say, a serdev driver for the GNSS port of
> the modem? To enable the receiver using AT commands on a different port?
> Or what kind of interface did you have in mind here?

There are bunch of AT command based modems that will require you to send an AT command to enable NMEA reporting. Same as QMI requires you to send a command to enable the reporting. The QMI part might be eventually moved into the kernel if we get QMUX done there and all the USB modems ported to participate in a QMI subsystem so to speak. Until then even for QMI this is needed. Look at oFono and its location reporting drivers. We could easily change oFono to support /dev/ugnss and feed this all into a single GNSS subsystem.

Regards

Marcel

Re: [PATCH 0/7] gnss: add new GNSS subsystem

[Johan Hovold]

Hi Marcel,

Sorry about the late reply. I got side-tracked with other things.

On Tue, May 08, 2018 at 10:03:57PM +0200, Marcel Holtmann wrote:
> Hi Johan,
> 
> >>>>>> I have one concern, though. While providing raw data by
> >>>>>> default is fine generally, it is a problem with device
> >>>>>> auto-discovery. I think there should be some IOCTL from
> >>>>>> the start, that can be used to inform userspace about
> >>>>>> the raw protocol being used (i.e. "NMEA"). I fear, that
> >>>>>> userspace may start to just assume raw = NMEA without
> >>>>>> having this (especially since all initial drivers provide
> >>>>>> NMEA).
> >>>>> 
> >>>>> One problem I see here would be that the driver does not necessarily
> >>>>> know either what protocol is currently being used. Some devices have
> >>>>> boot-pins which can be used to configure the initial protocol used (and
> >>>>> this could perhaps be reflected in DT), but this can often later be
> >>>>> changed (by user space) and even be made persistent using battery-backed
> >>>>> ram or eeproms.
> >>>>> 
> >>>>> Also note that at least u-blox devices supports having more than one
> >>>>> protocol active on the same port...
> >>>> 
> >>>> as long as userspace can determine that it is GNSS hardware and what
> >>>> hardware it is, then you deal with the rest in userspace.
> >>> 
> >>> Yeah, I think that will do for now.
> >> 
> >> I forgot to mention that this part should be simple. So providing a
> >> DEVTYPE attribute that can be easily associated to the /dev/gnssX
> >> nodes is a must have here. Doing complex mapping of USB or DT layouts
> >> to figure out this is NMEA vs some vendor vs I need extra code to
> >> change the mode etc.
> > 
> > Yes, as I mentioned in the cover letter some kind of generic interface
> > for identifying the device type (and its features) should be defined
> > eventually.
> 
> I think this needs to be present from the start. Half backed
> subsystems are dangerous. And I really want to avoid hacking in
> userspace to figure out details about the hardware.

Fair enough. I've now added support for determining the GNSS type, which
reflects the protocol(s) supported by the device and which should allow,
for example, the gpsd protocol probing hack to be dropped.

Currently, there are three types defined:

	"NMEA"  NMEA 0183
        "SiRF"  SiRF Binary
        "UBX"   UBX

Note that both SiRF and UBX type devices typically support a subset of
NMEA 0183 with vendor extensions (e.g. to allow switching to the vendor
protocol).

> > Note that this is not necessarily something that is needed from the
> > start however as, for example, gpsd implements protocol detection.
> 
> That might be, but that is a total hack. Lets get this right from the
> get-go.

I agree; it's horrid.

Thanks,
Johan