[PATCH 0/3] serdev support for n_gsm

[PATCH 0/3] serdev support for n_gsm

[Tony Lindgren]

Hi all,

Here's a series of patches to add initial serdev support to n_gsm
TS 27.010 line discipline.

This allows handling vendor specific protocols on top of TS 27.010 and
allows creating simple serdev drivers where it makes sense. So far I've
tested it with droid 4 for it's modem to provide char devices for AT
ports, modem PM support, and serdev drivers for GNSS and Alsa ASoC.

I'll be posting the related MFD, GNSS and Alsa ASoC drivers separately.
For reference, the MFD driver is at [0], the GNSS driver at [1], and
the Alsa ASoC driver at [2] below.

Regards,

Tony


[0] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/drivers/mfd/motorola-mdm.c?h=droid4-pending-mdm-v4.20
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/drivers/gnss/motmdm.c?h=droid4-pending-mdm-v4.20
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/sound/soc/codecs/motmdm.c?h=droid4-pending-mdm-v4.20


Tony Lindgren (3):
  tty: n_gsm: Add copy_config() and gsm_config() to prepare for serdev
  n_gsm: Constify u8 and unsigned char usage
  tty: n_gsm: Add support for serdev drivers

 drivers/tty/n_gsm.c        | 548 +++++++++++++++++++++++++++++--------
 include/linux/serdev-gsm.h | 227 +++++++++++++++
 2 files changed, 663 insertions(+), 112 deletions(-)
 create mode 100644 include/linux/serdev-gsm.h

-- 
2.20.1

[PATCH 1/3] tty: n_gsm: Add copy_config() and gsm_config() to prepare for serdev

[Tony Lindgren]

For supporting serdev drivers, we need to be able to configure n_gsm
from drivers. Let's prepare for that by adding copy_config() and
gsm_config() helper functions by moving the code around a bit.

Let's also unify the comments to keep checkpatch happy while at it.

Cc: linux-serial@vger.kernel.org
Cc: Alan Cox <alan@llwyncelyn.cymru>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Johan Hovold <johan@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_gsm.c | 207 +++++++++++++++++++++++---------------------
 1 file changed, 107 insertions(+), 100 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2214,6 +2214,111 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	return gsm;
 }
 
+static void gsm_copy_config_values(struct gsm_mux *gsm,
+				   struct gsm_config *c)
+{
+	memset(c, 0, sizeof(*c));
+	c->adaption = gsm->adaption;
+	c->encapsulation = gsm->encoding;
+	c->initiator = gsm->initiator;
+	c->t1 = gsm->t1;
+	c->t2 = gsm->t2;
+	c->t3 = 0;	/* Not supported */
+	c->n2 = gsm->n2;
+	if (gsm->ftype == UIH)
+		c->i = 1;
+	else
+		c->i = 2;
+	pr_debug("Ftype %d i %d\n", gsm->ftype, c->i);
+	c->mru = gsm->mru;
+	c->mtu = gsm->mtu;
+	c->k = 0;
+}
+
+static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
+{
+	int need_close = 0;
+	int need_restart = 0;
+
+	/* Stuff we don't support yet - UI or I frame transport, windowing */
+	if ((c->adaption != 1 && c->adaption != 2) || c->k)
+		return -EOPNOTSUPP;
+	/* Check the MRU/MTU range looks sane */
+	if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
+		return -EINVAL;
+	if (c->n2 < 3)
+		return -EINVAL;
+	if (c->encapsulation > 1)	/* Basic, advanced, no I */
+		return -EINVAL;
+	if (c->initiator > 1)
+		return -EINVAL;
+	if (c->i == 0 || c->i > 2)	/* UIH and UI only */
+		return -EINVAL;
+	/*
+	 * See what is needed for reconfiguration
+	 */
+
+	/* Timing fields */
+	if (c->t1 != 0 && c->t1 != gsm->t1)
+		need_restart = 1;
+	if (c->t2 != 0 && c->t2 != gsm->t2)
+		need_restart = 1;
+	if (c->encapsulation != gsm->encoding)
+		need_restart = 1;
+	if (c->adaption != gsm->adaption)
+		need_restart = 1;
+	/* Requires care */
+	if (c->initiator != gsm->initiator)
+		need_close = 1;
+	if (c->mru != gsm->mru)
+		need_restart = 1;
+	if (c->mtu != gsm->mtu)
+		need_restart = 1;
+
+	/*
+	 * Close down what is needed, restart and initiate the new
+	 * configuration
+	 */
+
+	if (need_close || need_restart) {
+		int ret;
+
+		ret = gsm_disconnect(gsm);
+
+		if (ret)
+			return ret;
+	}
+	if (need_restart)
+		gsm_cleanup_mux(gsm);
+
+	gsm->initiator = c->initiator;
+	gsm->mru = c->mru;
+	gsm->mtu = c->mtu;
+	gsm->encoding = c->encapsulation;
+	gsm->adaption = c->adaption;
+	gsm->n2 = c->n2;
+
+	if (c->i == 1)
+		gsm->ftype = UIH;
+	else if (c->i == 2)
+		gsm->ftype = UI;
+
+	if (c->t1)
+		gsm->t1 = c->t1;
+	if (c->t2)
+		gsm->t2 = c->t2;
+
+	/*
+	 * FIXME: We need to separate activation/deactivation from adding
+	 * and removing from the mux array
+	 */
+	if (need_restart)
+		gsm_activate_mux(gsm);
+	if (gsm->initiator && need_close)
+		gsm_dlci_begin_open(gsm->dlci[0]);
+	return 0;
+}
+
 /**
  *	gsmld_output		-	write to link
  *	@gsm: our mux
@@ -2495,89 +2600,6 @@ static __poll_t gsmld_poll(struct tty_struct *tty, struct file *file,
 	return mask;
 }
 
-static int gsmld_config(struct tty_struct *tty, struct gsm_mux *gsm,
-							struct gsm_config *c)
-{
-	int need_close = 0;
-	int need_restart = 0;
-
-	/* Stuff we don't support yet - UI or I frame transport, windowing */
-	if ((c->adaption != 1 && c->adaption != 2) || c->k)
-		return -EOPNOTSUPP;
-	/* Check the MRU/MTU range looks sane */
-	if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
-		return -EINVAL;
-	if (c->n2 < 3)
-		return -EINVAL;
-	if (c->encapsulation > 1)	/* Basic, advanced, no I */
-		return -EINVAL;
-	if (c->initiator > 1)
-		return -EINVAL;
-	if (c->i == 0 || c->i > 2)	/* UIH and UI only */
-		return -EINVAL;
-	/*
-	 *	See what is needed for reconfiguration
-	 */
-
-	/* Timing fields */
-	if (c->t1 != 0 && c->t1 != gsm->t1)
-		need_restart = 1;
-	if (c->t2 != 0 && c->t2 != gsm->t2)
-		need_restart = 1;
-	if (c->encapsulation != gsm->encoding)
-		need_restart = 1;
-	if (c->adaption != gsm->adaption)
-		need_restart = 1;
-	/* Requires care */
-	if (c->initiator != gsm->initiator)
-		need_close = 1;
-	if (c->mru != gsm->mru)
-		need_restart = 1;
-	if (c->mtu != gsm->mtu)
-		need_restart = 1;
-
-	/*
-	 *	Close down what is needed, restart and initiate the new
-	 *	configuration
-	 */
-
-	if (need_close || need_restart) {
-		int ret;
-
-		ret = gsm_disconnect(gsm);
-
-		if (ret)
-			return ret;
-	}
-	if (need_restart)
-		gsm_cleanup_mux(gsm);
-
-	gsm->initiator = c->initiator;
-	gsm->mru = c->mru;
-	gsm->mtu = c->mtu;
-	gsm->encoding = c->encapsulation;
-	gsm->adaption = c->adaption;
-	gsm->n2 = c->n2;
-
-	if (c->i == 1)
-		gsm->ftype = UIH;
-	else if (c->i == 2)
-		gsm->ftype = UI;
-
-	if (c->t1)
-		gsm->t1 = c->t1;
-	if (c->t2)
-		gsm->t2 = c->t2;
-
-	/* FIXME: We need to separate activation/deactivation from adding
-	   and removing from the mux array */
-	if (need_restart)
-		gsm_activate_mux(gsm);
-	if (gsm->initiator && need_close)
-		gsm_dlci_begin_open(gsm->dlci[0]);
-	return 0;
-}
-
 static int gsmld_ioctl(struct tty_struct *tty, struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
@@ -2586,29 +2608,14 @@ static int gsmld_ioctl(struct tty_struct *tty, struct file *file,
 
 	switch (cmd) {
 	case GSMIOC_GETCONF:
-		memset(&c, 0, sizeof(c));
-		c.adaption = gsm->adaption;
-		c.encapsulation = gsm->encoding;
-		c.initiator = gsm->initiator;
-		c.t1 = gsm->t1;
-		c.t2 = gsm->t2;
-		c.t3 = 0;	/* Not supported */
-		c.n2 = gsm->n2;
-		if (gsm->ftype == UIH)
-			c.i = 1;
-		else
-			c.i = 2;
-		pr_debug("Ftype %d i %d\n", gsm->ftype, c.i);
-		c.mru = gsm->mru;
-		c.mtu = gsm->mtu;
-		c.k = 0;
+		gsm_copy_config_values(gsm, &c);
 		if (copy_to_user((void *)arg, &c, sizeof(c)))
 			return -EFAULT;
 		return 0;
 	case GSMIOC_SETCONF:
 		if (copy_from_user(&c, (void *)arg, sizeof(c)))
 			return -EFAULT;
-		return gsmld_config(tty, gsm, &c);
+		return gsm_config(gsm, &c);
 	default:
 		return n_tty_ioctl_helper(tty, file, cmd, arg);
 	}
-- 
2.20.1

[PATCH 2/3] n_gsm: Constify u8 and unsigned char usage

[Tony Lindgren]

In order to prepare for adding serdev driver support, let's constify
the use of u8 and unsigned char for n_gsm.

Note that gsm_control_modem() gsm_control_rls() read the data for tty
control characters and then call gsm_control_reply() that allocates a
new reply and copies the data.

Cc: linux-serial@vger.kernel.org
Cc: Alan Cox <alan@llwyncelyn.cymru>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Johan Hovold <johan@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_gsm.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -143,8 +143,8 @@ struct gsm_dlci {
 	struct sk_buff *skb;	/* Frame being sent */
 	struct sk_buff_head skb_list;	/* Queued frames */
 	/* Data handling callback */
-	void (*data)(struct gsm_dlci *dlci, u8 *data, int len);
-	void (*prev_data)(struct gsm_dlci *dlci, u8 *data, int len);
+	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
+	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
 	struct net_device *net; /* network interface, if created */
 };
 
@@ -988,7 +988,7 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
  *	Encode up and queue a UI/UIH frame containing our response.
  */
 
-static void gsm_control_reply(struct gsm_mux *gsm, int cmd, u8 *data,
+static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
 					int dlen)
 {
 	struct gsm_msg *msg;
@@ -1073,14 +1073,14 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
  *	and if need be stuff a break message down the tty.
  */
 
-static void gsm_control_modem(struct gsm_mux *gsm, u8 *data, int clen)
+static void gsm_control_modem(struct gsm_mux *gsm, const u8 *data, int clen)
 {
 	unsigned int addr = 0;
 	unsigned int modem = 0;
 	unsigned int brk = 0;
 	struct gsm_dlci *dlci;
 	int len = clen;
-	u8 *dp = data;
+	const u8 *dp = data;
 	struct tty_struct *tty;
 
 	while (gsm_read_ea(&addr, *dp++) == 0) {
@@ -1134,13 +1134,13 @@ static void gsm_control_modem(struct gsm_mux *gsm, u8 *data, int clen)
  *	this into the uplink tty if present
  */
 
-static void gsm_control_rls(struct gsm_mux *gsm, u8 *data, int clen)
+static void gsm_control_rls(struct gsm_mux *gsm, const u8 *data, int clen)
 {
 	struct tty_port *port;
 	unsigned int addr = 0;
 	u8 bits;
 	int len = clen;
-	u8 *dp = data;
+	const u8 *dp = data;
 
 	while (gsm_read_ea(&addr, *dp++) == 0) {
 		len--;
@@ -1189,7 +1189,7 @@ static void gsm_dlci_begin_close(struct gsm_dlci *dlci);
  */
 
 static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
-							u8 *data, int clen)
+						const u8 *data, int clen)
 {
 	u8 buf[1];
 	unsigned long flags;
@@ -1261,7 +1261,7 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
  */
 
 static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
-							u8 *data, int clen)
+						const u8 *data, int clen)
 {
 	struct gsm_control *ctrl;
 	unsigned long flags;
@@ -1553,7 +1553,7 @@ static void gsm_dlci_begin_close(struct gsm_dlci *dlci)
  *	open we shovel the bits down it, if not we drop them.
  */
 
-static void gsm_dlci_data(struct gsm_dlci *dlci, u8 *data, int clen)
+static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
 {
 	/* krefs .. */
 	struct tty_port *port = &dlci->port;
@@ -1603,7 +1603,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, u8 *data, int clen)
  *	and we divide up the work accordingly.
  */
 
-static void gsm_dlci_command(struct gsm_dlci *dlci, u8 *data, int len)
+static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len)
 {
 	/* See what command is involved */
 	unsigned int command = 0;
@@ -2702,7 +2702,7 @@ static void gsm_mux_net_tx_timeout(struct net_device *net)
 }
 
 static void gsm_mux_rx_netchar(struct gsm_dlci *dlci,
-				   unsigned char *in_buf, int size)
+				const unsigned char *in_buf, int size)
 {
 	struct net_device *net = dlci->net;
 	struct sk_buff *skb;
-- 
2.20.1

[PATCH 3/3] tty: n_gsm: Add support for serdev drivers

[Tony Lindgren]

We can make use of serdev drivers to do simple device drivers for
TS 27.010 chanels, and we can handle vendor specific protocols on top
of TS 27.010 with serdev drivers.

Serdev device drivers can be done for channel specific features, such
as GNSS framework and Alsa ASoC voice call audio mixer found on some
modems. And serdev drivers can be used to provide character devices
for things like AT modems on TS 27.010 channels.

So far this has been tested with Motorola droid4 where there is a custom
packet numbering protocol on top of TS 27.010 for the MDM6600 modem. I'll
be posting the motorola-mdm MFD driver separately followed by channel
specific drivers for Alsa ASoC and GNSS frameworks.

I initially though about adding the serdev support into a separate file,
but that will take some refactoring of n_gsm.c. And I'd like to have
things working first. Then later on we might want to consider splitting
n_gsm.c into three pieces for core, tty and serdev parts.

Cc: linux-serial@vger.kernel.org
Cc: Alan Cox <alan@llwyncelyn.cymru>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Johan Hovold <johan@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/n_gsm.c        | 317 +++++++++++++++++++++++++++++++++++++
 include/linux/serdev-gsm.h | 227 ++++++++++++++++++++++++++
 2 files changed, 544 insertions(+)
 create mode 100644 include/linux/serdev-gsm.h

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -39,6 +39,7 @@
 #include <linux/file.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/serdev.h>
 #include <linux/timer.h>
 #include <linux/tty_flip.h>
 #include <linux/tty_driver.h>
@@ -50,6 +51,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/gsmmux.h>
+#include <linux/serdev-gsm.h>
 
 static int debug;
 module_param(debug, int, 0600);
@@ -145,6 +147,7 @@ struct gsm_dlci {
 	/* Data handling callback */
 	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
 	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
+	struct gsm_serdev_dlci *ops; /* serdev dlci ops, if used */
 	struct net_device *net; /* network interface, if created */
 };
 
@@ -179,6 +182,7 @@ struct gsm_control {
  */
 
 struct gsm_mux {
+	struct gsm_serdev *gsd;		/* Serial device bus data */
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	struct mutex mutex;
@@ -2319,6 +2323,319 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	return 0;
 }
 
+#ifdef CONFIG_SERIAL_DEV_BUS
+static int gsd_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	struct gsm_mux *gsm;
+
+	if (!gsd || !gsd->gsm)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	if (!c)
+		return -EINVAL;
+
+	gsm_copy_config_values(gsm, c);
+
+	return 0;
+}
+
+static int gsd_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	struct gsm_mux *gsm;
+
+	if (!gsd || !gsd->serdev || !gsd->gsm)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	if (!c)
+		return -EINVAL;
+
+	return gsm_config(gsm, c);
+}
+
+static struct gsm_dlci *gsd_dlci_get(struct gsm_serdev *gsd, int line,
+				     bool allocate)
+{
+	struct gsm_mux *gsm;
+	struct gsm_dlci *dlci;
+
+	if (!gsd || !gsd->gsm)
+		return ERR_PTR(-ENODEV);
+
+	gsm = gsd->gsm;
+
+	if (line < 1 || line >= 63)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&gsm->mutex);
+
+	if (gsm->dlci[line]) {
+		dlci = gsm->dlci[line];
+		goto unlock;
+	} else if (!allocate) {
+		dlci = ERR_PTR(-ENODEV);
+		goto unlock;
+	}
+
+	dlci = gsm_dlci_alloc(gsm, line);
+	if (!dlci) {
+		gsm = ERR_PTR(-ENOMEM);
+		goto unlock;
+	}
+
+	gsm->dlci[line] = dlci;
+
+unlock:
+	mutex_unlock(&gsm->mutex);
+
+	return dlci;
+}
+
+static void gsd_dlci_data(struct gsm_dlci *dlci, const u8 *buf, int len)
+{
+	struct gsm_mux *gsm = dlci->gsm;
+	struct gsm_serdev *gsd = gsm->gsd;
+
+	if (!gsd || !dlci->ops)
+		return;
+
+	switch (dlci->adaption) {
+	case 0:
+	case 1:
+		if (dlci->ops->receive_buf)
+			dlci->ops->receive_buf(dlci->ops, buf, len);
+		break;
+	default:
+		pr_warn("dlci%i adaption %i not yet implemented\n",
+			dlci->addr, dlci->adaption);
+		break;
+	}
+}
+
+static int gsd_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *sd,
+		     const u8 *buf, int len)
+{
+	struct gsm_mux *gsm;
+	struct gsm_dlci *dlci;
+	struct gsm_msg *msg;
+	int h, size, total_size = 0;
+	u8 *dp;
+
+	if (!gsd || !gsd->gsm)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	dlci = gsd_dlci_get(gsd, sd->line, false);
+	if (IS_ERR(dlci))
+		return PTR_ERR(dlci);
+
+	h = dlci->adaption - 1;
+
+	if (len > gsm->mtu)
+		len = gsm->mtu;
+
+	size = len + h;
+
+	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
+	if (!msg)
+		return -ENOMEM;
+
+	dp = msg->data;
+	switch (dlci->adaption) {
+	case 1:
+		break;
+	case 2:
+		*dp++ = gsm_encode_modem(dlci);
+		break;
+	}
+	memcpy(dp, buf, len);
+	__gsm_data_queue(dlci, msg);
+	total_size += size;
+
+	return total_size;
+}
+
+static void gsd_data_kick(struct gsm_serdev *gsd)
+{
+	struct gsm_mux *gsm;
+	unsigned long flags;
+
+	if (!gsd || !gsd->gsm)
+		return;
+
+	gsm = gsd->gsm;
+
+	spin_lock_irqsave(&gsm->tx_lock, flags);
+	gsm_data_kick(gsm);
+	spin_unlock_irqrestore(&gsm->tx_lock, flags);
+}
+
+static int gsd_register_dlci(struct gsm_serdev *gsd,
+			     struct gsm_serdev_dlci *ops)
+{
+	struct gsm_dlci *dlci;
+	struct gsm_mux *gsm;
+
+	if (!gsd || !gsd->gsm || !gsd->serdev)
+		return -ENODEV;
+
+	gsm = gsd->gsm;
+
+	if (!ops || !ops->line || !ops->receive_buf)
+		return -EINVAL;
+
+	dlci = gsd_dlci_get(gsd, ops->line, true);
+	if (IS_ERR(dlci))
+		return PTR_ERR(dlci);
+
+	if (dlci->state == DLCI_OPENING || dlci->state == DLCI_OPEN ||
+	    dlci->state == DLCI_CLOSING)
+		return -EBUSY;
+
+	mutex_lock(&dlci->mutex);
+	dlci->ops = ops;
+	dlci->modem_rx = 0;
+	dlci->prev_data = dlci->data;
+	dlci->data = gsd_dlci_data;
+	mutex_unlock(&dlci->mutex);
+
+	gsm_dlci_begin_open(dlci);
+
+	return 0;
+}
+
+static void gsd_unregister_dlci(struct gsm_serdev *gsd,
+				struct gsm_serdev_dlci *ops)
+{
+	struct gsm_mux *gsm;
+	struct gsm_dlci *dlci;
+
+	if (!gsd || !gsd->gsm || !gsd->serdev || !gsd->unregister_dlci)
+		return;
+
+	gsm = gsd->gsm;
+
+	if (!ops || !ops->line)
+		return;
+
+	dlci = gsd_dlci_get(gsd, ops->line, false);
+	if (IS_ERR(dlci))
+		return;
+
+	mutex_lock(&dlci->mutex);
+	gsm_destroy_network(dlci);
+	dlci->data = dlci->prev_data;
+	dlci->ops = NULL;
+	mutex_unlock(&dlci->mutex);
+
+	gsm_dlci_begin_close(dlci);
+}
+
+static int gsm_serdev_output(struct gsm_mux *gsm, u8 *data, int len)
+{
+	struct gsm_serdev *gsd = gsm->gsd;
+	struct serdev_device *serdev = gsm->gsd->serdev;
+	bool asleep;
+
+	asleep = atomic_read(&gsd->asleep);
+	if (asleep)
+		return -ENOSPC;
+
+	if (gsm->gsd->output)
+		return gsm->gsd->output(gsm->gsd, data, len);
+	else
+		return serdev_device_write_buf(serdev, data, len);
+}
+
+static int gsd_receive_buf(struct serdev_device *serdev, const u8 *data,
+			   size_t count)
+{
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+	struct gsm_mux *gsm;
+	const unsigned char *dp;
+	int i;
+
+	if (WARN_ON(!gsd))
+		return 0;
+
+	gsm = gsd->gsm;
+
+	if (debug & 4)
+		print_hex_dump_bytes("gsd_receive_buf: ",
+				     DUMP_PREFIX_OFFSET,
+				     data, count);
+
+	for (i = count, dp = data; i; i--, dp++)
+		gsm->receive(gsm, *dp);
+
+	return count;
+}
+
+static void gsd_write_wakeup(struct serdev_device *serdev)
+{
+	serdev_device_write_wakeup(serdev);
+}
+
+static struct serdev_device_ops gsd_client_ops = {
+	.receive_buf = gsd_receive_buf,
+	.write_wakeup = gsd_write_wakeup,
+};
+
+int gsm_serdev_register_device(struct gsm_serdev *gsd)
+{
+	struct gsm_mux *gsm;
+	int error;
+
+	if (WARN(!gsd || !gsd->serdev || !gsd->output,
+		 "serdev and output must be initialized\n"))
+		return -EINVAL;
+
+	serdev_device_set_client_ops(gsd->serdev, &gsd_client_ops);
+
+	gsm = gsm_alloc_mux();
+	if (!gsm)
+		return -ENOMEM;
+
+	gsm->encoding = 1;
+	gsm->tty = NULL;
+	gsm->gsd = gsd;
+	atomic_set(&gsd->asleep, 0);
+	gsd->gsm = gsm;
+	gsd->get_config = gsd_get_config;
+	gsd->set_config = gsd_set_config;
+	gsd->register_dlci = gsd_register_dlci;
+	gsd->unregister_dlci = gsd_unregister_dlci;
+	gsm->output = gsm_serdev_output;
+	gsd->write = gsd_write;
+	gsd->kick = gsd_data_kick;
+	mux_get(gsd->gsm);
+
+	error = gsm_activate_mux(gsd->gsm);
+	if (error) {
+		gsm_cleanup_mux(gsd->gsm);
+		mux_put(gsd->gsm);
+
+		return error;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_register_device);
+
+void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
+{
+	gsm_cleanup_mux(gsd->gsm);
+	mux_put(gsd->gsm);
+	gsd->gsm = NULL;
+}
+EXPORT_SYMBOL_GPL(gsm_serdev_unregister_device);
+
+#endif	/* CONFIG_SERIAL_DEV_BUS */
+
 /**
  *	gsmld_output		-	write to link
  *	@gsm: our mux
diff --git a/include/linux/serdev-gsm.h b/include/linux/serdev-gsm.h
new file mode 100644
--- /dev/null
+++ b/include/linux/serdev-gsm.h
@@ -0,0 +1,227 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+struct serdev_device;
+struct gsm_mux;
+struct gsm_config;
+struct gsm_dlci;
+struct gsm_serdev_dlci;
+
+/**
+ * struct gsm_serdev - serdev-gsm instance
+ * @serdev:		serdev instance
+ * @gsm:		ts 27.010 n_gsm instance
+ * @asleep:		device is in idle state
+ * @drvdata:		serdev-gsm consumer driver data
+ * @get_config:		get ts 27.010 config
+ * @set_config:		set ts 27.010 config
+ * @register_dlci:	register ts 27.010 channel
+ * @unregister_dlci:	unregister ts 27.010 channel
+ * @output:		read data from ts 27.010 channel
+ * @write:		write data to a ts 27.010 channel
+ * @kick:		indicate more data is ready
+ *
+ * Currently only serdev and output must be initialized, the rest are
+ * are initialized by gsm_serdev_register_dlci().
+ */
+struct gsm_serdev {
+	struct serdev_device *serdev;
+	struct gsm_mux *gsm;
+	atomic_t asleep;
+	void *drvdata;
+	int (*get_config)(struct gsm_serdev *gsd, struct gsm_config *c);
+	int (*set_config)(struct gsm_serdev *gsd, struct gsm_config *c);
+	int (*register_dlci)(struct gsm_serdev *gsd,
+			     struct gsm_serdev_dlci *ops);
+	void (*unregister_dlci)(struct gsm_serdev *gsd,
+				struct gsm_serdev_dlci *ops);
+	int (*output)(struct gsm_serdev *gsd, u8 *data, int len);
+	int (*write)(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+		     const u8 *buf, int len);
+	void (*kick)(struct gsm_serdev *gsd);
+};
+
+/**
+ * struct gsm_serdev_dlci - serdev-gsm ts 27.010 channel data
+ * @line:		ts 27.010 channel, control channel 0 is not available
+ * @receive_buf:	function to handle data received for the channel
+ */
+struct gsm_serdev_dlci {
+	int line;
+	int (*receive_buf)(struct gsm_serdev_dlci *ops,
+			   const unsigned char *buf,
+			   size_t len);
+};
+
+#ifdef CONFIG_SERIAL_DEV_BUS
+
+int gsm_serdev_register_device(struct gsm_serdev *gsd);
+void gsm_serdev_unregister_device(struct gsm_serdev *gsd);
+
+static inline void *gsm_serdev_get_drvdata(struct device *dev)
+{
+	struct serdev_device *serdev = to_serdev_device(dev);
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+
+	if (gsd)
+		return gsd->drvdata;
+
+	return NULL;
+}
+
+static inline void gsm_serdev_set_drvdata(struct device *dev, void *drvdata)
+{
+	struct serdev_device *serdev = to_serdev_device(dev);
+	struct gsm_serdev *gsd = serdev_device_get_drvdata(serdev);
+
+	if (gsd)
+		gsd->drvdata = drvdata;
+}
+
+/**
+ * gsm_serdev_get_config - read ts 27.010 config
+ * @gsd:	serdev-gsm instance
+ * @c:		ts 27.010 config data
+ *
+ * See gsm_copy_config_values() for more information.
+ */
+static inline
+int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	return gsd->get_config(gsd, c);
+}
+
+/**
+ * gsm_serdev_set_config - set ts 27.010 config
+ * @gsd:	serdev-gsm instance
+ * @c:		ts 27.010 config data
+ *
+ * See gsm_config() for more information.
+ */
+static inline
+int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	if (gsd && gsd->set_config)
+		return gsd->set_config(gsd, c);
+
+	return -ENODEV;
+}
+
+/**
+ * gsm_serdev_register_dlci - register a ts 27.010 channel
+ * @gsd:	serdev-gsm instance
+ * @ops:	channel ops
+ */
+static inline
+int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
+			     struct gsm_serdev_dlci *ops)
+{
+	if (gsd && gsd->register_dlci)
+		return gsd->register_dlci(gsd, ops);
+
+	return -ENODEV;
+}
+
+/**
+ * gsm_serdev_unregister_dlci - unregister a ts 27.010 channel
+ * @gsd:	serdev-gsm instance
+ * @ops:	channel ops
+ */
+static inline
+void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
+				struct gsm_serdev_dlci *ops)
+{
+	if (gsd && gsd->unregister_dlci)
+		gsd->unregister_dlci(gsd, ops);
+}
+
+/**
+ * gsm_serdev_write - write data to a ts 27.010 channel
+ * @gsd:	serdev-gsm instance
+ * @ops:	channel ops
+ * @buf:	write buffer
+ * @len:	buffer length
+ */
+static inline
+int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+		     const u8 *buf, int len)
+{
+	if (gsd && gsd->write)
+		return gsd->write(gsd, ops, buf, len);
+
+	return -ENODEV;
+}
+
+/**
+ * gsm_serdev_data_kick - indicate more data can be trasmitted
+ * @gsd:	serdev-gsm instance
+ *
+ * See gsm_data_kick() for more information.
+ */
+static inline
+void gsm_serdev_data_kick(struct gsm_serdev *gsd)
+{
+	if (gsd && gsd->kick)
+		gsd->kick(gsd);
+}
+
+#else	/* CONFIG_SERIAL_DEV_BUS */
+
+static inline
+int gsm_serdev_register_device(struct gsm_serdev *gsd)
+{
+	return -ENODEV;
+}
+
+static inline
+void gsm_serdev_unregister_device(struct gsm_serdev *gsd)
+{
+}
+
+static inline void *gsm_serdev_get_drvdata(struct device *dev)
+{
+	return NULL;
+}
+
+static inline
+void gsm_serdev_set_drvdata(struct device *dev, void *drvdata)
+{
+}
+
+static inline
+int gsm_serdev_get_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	return -ENODEV;
+}
+
+static inline
+int gsm_serdev_set_config(struct gsm_serdev *gsd, struct gsm_config *c)
+{
+	return -ENODEV;
+}
+
+static inline
+int gsm_serdev_register_dlci(struct gsm_serdev *gsd,
+			     struct gsm_serdev_dlci *ops)
+{
+	return -ENODEV;
+}
+
+static inline
+void gsm_serdev_unregister_dlci(struct gsm_serdev *gsd,
+				struct gsm_serdev_dlci *ops)
+{
+}
+
+static inline
+int gsm_serdev_write(struct gsm_serdev *gsd, struct gsm_serdev_dlci *ops,
+		     const u8 *buf, int len)
+{
+	return -ENODEV;
+}
+
+static inline
+void gsm_serdev_data_kick(struct gsm_serdev *gsd)
+{
+}
+
+#endif	/* CONFIG_SERIAL_DEV_BUS */
-- 
2.20.1

Re: [PATCH 0/3] serdev support for n_gsm

[Pavel Machek]

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

On Sun 2019-01-13 17:25:25, Tony Lindgren wrote:
> Hi all,
> 
> Here's a series of patches to add initial serdev support to n_gsm
> TS 27.010 line discipline.
> 
> This allows handling vendor specific protocols on top of TS 27.010 and
> allows creating simple serdev drivers where it makes sense. So far I've
> tested it with droid 4 for it's modem to provide char devices for AT
> ports, modem PM support, and serdev drivers for GNSS and Alsa ASoC.
> 
> I'll be posting the related MFD, GNSS and Alsa ASoC drivers separately.
> For reference, the MFD driver is at [0], the GNSS driver at [1], and
> the Alsa ASoC driver at [2] below.

For the series:

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

Thanks for doing this!
								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/3] serdev support for n_gsm

[Greg Kroah-Hartman]

On Sun, Jan 13, 2019 at 05:25:25PM -0800, Tony Lindgren wrote:
> Hi all,
> 
> Here's a series of patches to add initial serdev support to n_gsm
> TS 27.010 line discipline.
> 
> This allows handling vendor specific protocols on top of TS 27.010 and
> allows creating simple serdev drivers where it makes sense. So far I've
> tested it with droid 4 for it's modem to provide char devices for AT
> ports, modem PM support, and serdev drivers for GNSS and Alsa ASoC.
> 
> I'll be posting the related MFD, GNSS and Alsa ASoC drivers separately.
> For reference, the MFD driver is at [0], the GNSS driver at [1], and
> the Alsa ASoC driver at [2] below.

I have applied the first two patches to my tree, as those are nice
cleanups.

The last one I want some feedback from the serdev developers to verify
all is set up properly, and Johan, to see if this ends up conflicting
with the gnss code, as that would not be good.

thanks,

greg k-h

Re: [PATCH 0/3] serdev support for n_gsm

[Johan Hovold]

Adding Marcel on CC.

On Fri, Jan 18, 2019 at 12:59:58PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Jan 13, 2019 at 05:25:25PM -0800, Tony Lindgren wrote:
> > Hi all,
> > 
> > Here's a series of patches to add initial serdev support to n_gsm
> > TS 27.010 line discipline.
> > 
> > This allows handling vendor specific protocols on top of TS 27.010 and
> > allows creating simple serdev drivers where it makes sense. So far I've
> > tested it with droid 4 for it's modem to provide char devices for AT
> > ports, modem PM support, and serdev drivers for GNSS and Alsa ASoC.
> > 
> > I'll be posting the related MFD, GNSS and Alsa ASoC drivers separately.
> > For reference, the MFD driver is at [0], the GNSS driver at [1], and
> > the Alsa ASoC driver at [2] below.
> 
> I have applied the first two patches to my tree, as those are nice
> cleanups.
> 
> The last one I want some feedback from the serdev developers to verify
> all is set up properly, and Johan, to see if this ends up conflicting
> with the gnss code, as that would not be good.

I think we need to have a discussion about how to model modems generally
before getting into implementation details.

Modems are currently managed by user space (e.g. through ofono) and
I'm not sure that moving only parts of its responsibilities into the
kernel is necessarily the right thing to do. You still need coordination
between the various components for things like power management.

TS 27.010 may make it seem like we can move everything into the kernel,
but Tony's to-be-posted Motorola MFD driver is still exposing character
devices for most of the muxed ports. If I understand things correctly,
there also still needs to be some coordination with USB over which some
channels are handled (e.g. IP over USB, gnss over muxed UART).

Instead of adding these extra layers, only to export most ports to user
space again, it may be better to hook into the various kernel subsystems
through dedicated user-space-implementation interfaces such as the
suggested ugnss interface, which means that user space feeds gnss data
into the kernel which in turn makes it available through a standard
interface.

This would also allow things to work for other transports such as USB
CDC for which the mux protocol isn't used.

Johan

Re: [PATCH 0/3] serdev support for n_gsm

[Tony Lindgren]

Hi,

* Johan Hovold <johan@kernel.org> [190121 10:57]:
> Adding Marcel on CC.
> 
> On Fri, Jan 18, 2019 at 12:59:58PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Jan 13, 2019 at 05:25:25PM -0800, Tony Lindgren wrote:
> > > Hi all,
> > > 
> > > Here's a series of patches to add initial serdev support to n_gsm
> > > TS 27.010 line discipline.
> > > 
> > > This allows handling vendor specific protocols on top of TS 27.010 and
> > > allows creating simple serdev drivers where it makes sense. So far I've
> > > tested it with droid 4 for it's modem to provide char devices for AT
> > > ports, modem PM support, and serdev drivers for GNSS and Alsa ASoC.
> > > 
> > > I'll be posting the related MFD, GNSS and Alsa ASoC drivers separately.
> > > For reference, the MFD driver is at [0], the GNSS driver at [1], and
> > > the Alsa ASoC driver at [2] below.
> > 
> > I have applied the first two patches to my tree, as those are nice
> > cleanups.
> > 
> > The last one I want some feedback from the serdev developers to verify
> > all is set up properly, and Johan, to see if this ends up conflicting
> > with the gnss code, as that would not be good.
> 
> I think we need to have a discussion about how to model modems generally
> before getting into implementation details.
> 
> Modems are currently managed by user space (e.g. through ofono) and
> I'm not sure that moving only parts of its responsibilities into the
> kernel is necessarily the right thing to do. You still need coordination
> between the various components for things like power management.

At least now we do have the option of doing kernel drivers or user
space apps whichever way we want to :)

And we can now do the user space apps without having to implement
any of the Motorola custom packet numbering layer on top of TS 27.010
for each app.

For some user space examples, I have posted scripts to send and receive
SMS at [3], and Pavel has ofono patches [4] below. Seems like we can
also add support to ModemManager along the similar lines. And for the
serdev drivers, those implement standard Linux interfaces for apps
to use.

For PM, about a year ago I tried making things work with a user space
solution and it sucked big time[5]. The power management makes sense
to do in the kernel driver at least in this case as there are shared
GPIO pins between the USB PHY and TS 27.010 UART. The shared GPIOs
are handled by the phy-mapphone-mdm6600 driver.

With the serdev n_gsm MFD driver, the only thing that needs to be done
to idle the modem is to enable autosuspend for the OHCI interface. So
no spefific coordination between various components is needed for PM
beyond that. Things idle just fine using PM runtime.

> TS 27.010 may make it seem like we can move everything into the kernel,
> but Tony's to-be-posted Motorola MFD driver is still exposing character
> devices for most of the muxed ports. If I understand things correctly,
> there also still needs to be some coordination with USB over which some
> channels are handled (e.g. IP over USB, gnss over muxed UART).

Hmm yes now we can do either user space daemons or kernel serdev
drivers.

For USB, the modem data connection already works with USB OHCI over
QMI. So it's is already handled and separated out of this. The USB
PHY and TS 27.010 UART have shared GPIO pins handled by the USB PHY
driver. The USB PHY is integrated into the modem with the shared
GPIO pins controlling the PHY and the TS 27.010 UART PM..
But it's working for PM and like I mentioned modem PM works as long
as OHCI is set to autosuspend. Well the modem also wants to see
TS 27.010 connected before idling.

> Instead of adding these extra layers, only to export most ports to user
> space again, it may be better to hook into the various kernel subsystems
> through dedicated user-space-implementation interfaces such as the
> suggested ugnss interface, which means that user space feeds gnss data
> into the kernel which in turn makes it available through a standard
> interface.

Sure that's doable. But notice that we actually need to kick the
serdev GNSS interface to get more data. It's not a passive GNSS
data feed in this case. So it's not going to be just a case of
cat /dev/motmdm4 > /dev/ugnss. Without the serdev GNSS driver,
it would be some-custom-app -i /dev/motmdm4 -o /dev/ugnss.

And without the n_gsm serdev support, it's a mess of some app
similar to [5] initializing n_gsm, trying to deal with the USB
PHY PM, dealing with Motorola custom packet numbering, kicking
GNSS device, feeding data to /dev/ugnss. Hmm I think I've already
been there just to be able to type AT commands to the modem and
it did not work :)

Anyways, for the serdev kernel drivers, the criteria I've tried
to follow is: "Can this serdev device driver make user space
apps use standard Linux interfaces for the hardware?"

So for the serdev Alsa ASoC driver, user space can use the standard
Alsa interface for setting voice call volume. And for the serdev
GNSS driver, user space can use /dev/gnss0.

I don't think it makes sense to do the GNSS driver in user space
in this case because of the kicking needed. But at least it
certainly is doable with /dev/ugnss.

> This would also allow things to work for other transports such as USB
> CDC for which the mux protocol isn't used.

Yeah I agree it would be nice to have GNSS framework have ugnss
device similar to drivers/input/misc/uinput.c. Anyways, having
a serdev GNSS driver in this case does not prevent a USB driver
from feeding data to the /dev/ugnss interface. In this case
doing GNSS driver over UART is preferred for smaller power
consumption.

Regards,

Tony


[0] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/drivers/mfd/motorola-mdm.c?h=droid4-pending-mdm-v4.20
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/drivers/gnss/motmdm.c?h=droid4-pending-mdm-v4.20
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/sound/soc/codecs/motmdm.c?h=droid4-pending-mdm-v4.20
[3] https://github.com/tmlind/droid4-sms-tools
[4] https://github.com/pavelmachek/ofono
[5] https://github.com/tmlind/droid4-ngsm

Re: [PATCH 0/3] serdev support for n_gsm

[Johan Hovold]

Hi Tony,

and sorry about the reply latency. This is quite a lot to think about.

I'm also adding Kishon and Mark on CC (e.g. for the phy and ASoC bits).

On Mon, Jan 21, 2019 at 09:01:16AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Johan Hovold <johan@kernel.org> [190121 10:57]:
> > Adding Marcel on CC.
> > 
> > On Fri, Jan 18, 2019 at 12:59:58PM +0100, Greg Kroah-Hartman wrote:
> > > On Sun, Jan 13, 2019 at 05:25:25PM -0800, Tony Lindgren wrote:
> > > > Hi all,
> > > > 
> > > > Here's a series of patches to add initial serdev support to n_gsm
> > > > TS 27.010 line discipline.
> > > > 
> > > > This allows handling vendor specific protocols on top of TS 27.010 and
> > > > allows creating simple serdev drivers where it makes sense. So far I've
> > > > tested it with droid 4 for it's modem to provide char devices for AT
> > > > ports, modem PM support, and serdev drivers for GNSS and Alsa ASoC.
> > > > 
> > > > I'll be posting the related MFD, GNSS and Alsa ASoC drivers separately.
> > > > For reference, the MFD driver is at [0], the GNSS driver at [1], and
> > > > the Alsa ASoC driver at [2] below.
> > > 
> > > I have applied the first two patches to my tree, as those are nice
> > > cleanups.
> > > 
> > > The last one I want some feedback from the serdev developers to verify
> > > all is set up properly, and Johan, to see if this ends up conflicting
> > > with the gnss code, as that would not be good.
> > 
> > I think we need to have a discussion about how to model modems generally
> > before getting into implementation details.
> > 
> > Modems are currently managed by user space (e.g. through ofono) and
> > I'm not sure that moving only parts of its responsibilities into the
> > kernel is necessarily the right thing to do. You still need coordination
> > between the various components for things like power management.
> 
> At least now we do have the option of doing kernel drivers or user
> space apps whichever way we want to :)
> 
> And we can now do the user space apps without having to implement
> any of the Motorola custom packet numbering layer on top of TS 27.010
> for each app.
> 
> For some user space examples, I have posted scripts to send and receive
> SMS at [3], and Pavel has ofono patches [4] below. Seems like we can
> also add support to ModemManager along the similar lines. And for the
> serdev drivers, those implement standard Linux interfaces for apps
> to use.
> 
> For PM, about a year ago I tried making things work with a user space
> solution and it sucked big time[5]. The power management makes sense
> to do in the kernel driver at least in this case as there are shared
> GPIO pins between the USB PHY and TS 27.010 UART. The shared GPIOs
> are handled by the phy-mapphone-mdm6600 driver.
> 
> With the serdev n_gsm MFD driver, the only thing that needs to be done
> to idle the modem is to enable autosuspend for the OHCI interface. So
> no spefific coordination between various components is needed for PM
> beyond that. Things idle just fine using PM runtime.

Yeah, I don't envy you trying to get this to work (and now I'm getting
dragged into it ;) ).

It would really help with a high-level outline of the modem and its
components. I've done my best to derive it from these patches and the
code you link to, but that info needs to go in the patch descriptions
(or cover letter).

This series adds a new interface (gsm_serdev) but no users, which is not
something we normally accept. I think you need to post the lot, at least
as an RFC in order for the full picture to be visible.

Similarly, including an example device tree would also help with the
overall picture. I've been able to derive the following (from your code
and already merged phy driver binding doc):

   	mdm6600_phy: usb-phy {
		compatible = "motorola,mapphone-mdm6600";
		enable-gpios = <&gpio3 31 GPIO_ACTIVE_LOW>;
		power-gpios = <&gpio2 22 GPIO_ACTIVE_HIGH>;
		reset-gpios = <&gpio2 17 GPIO_ACTIVE_HIGH>;
		motorola,mode-gpios = <&gpio5 20 GPIO_ACTIVE_HIGH>,
				      <&gpio5 21 GPIO_ACTIVE_HIGH>;
		motorola,cmd-gpios = <&gpio4 7 GPIO_ACTIVE_HIGH>,
				     <&gpio4 8 GPIO_ACTIVE_HIGH>,
				     <&gpio5 14 GPIO_ACTIVE_HIGH>;
		motorola,status-gpios = <&gpio2 20 GPIO_ACTIVE_HIGH>,
					<&gpio2 21 GPIO_ACTIVE_HIGH>,
					<&gpio2 23 GPIO_ACTIVE_HIGH>;
		#phy-cells = <0>;
	};

	&uart {
		gsm-mux? {
			compatible = "motorola,mapphone-mdm6600-serdev";
			phy = <&mdm6600_phy>;

			gnss {
				compatible = "motorola,mapphone-mdm6600-gnss";
			};

			audio-codec {

			};
		};
	};

	&ohci {
		phys = <&mdm6600_phy>;
	};

Which brings us back to the question of how to model modems. You've
already added a phy-driver for something which really is some sort of
modem representation.

I'm not saying I have a solution for this, but again, I think this needs
to be discussed before merging more code.

> > TS 27.010 may make it seem like we can move everything into the kernel,
> > but Tony's to-be-posted Motorola MFD driver is still exposing character
> > devices for most of the muxed ports. If I understand things correctly,
> > there also still needs to be some coordination with USB over which some
> > channels are handled (e.g. IP over USB, gnss over muxed UART).
> 
> Hmm yes now we can do either user space daemons or kernel serdev
> drivers.
> 
> For USB, the modem data connection already works with USB OHCI over
> QMI. So it's is already handled and separated out of this. The USB
> PHY and TS 27.010 UART have shared GPIO pins handled by the USB PHY
> driver. The USB PHY is integrated into the modem with the shared
> GPIO pins controlling the PHY and the TS 27.010 UART PM..
> But it's working for PM and like I mentioned modem PM works as long
> as OHCI is set to autosuspend. Well the modem also wants to see
> TS 27.010 connected before idling.
> 
> > Instead of adding these extra layers, only to export most ports to user
> > space again, it may be better to hook into the various kernel subsystems
> > through dedicated user-space-implementation interfaces such as the
> > suggested ugnss interface, which means that user space feeds gnss data
> > into the kernel which in turn makes it available through a standard
> > interface.
> 
> Sure that's doable. But notice that we actually need to kick the
> serdev GNSS interface to get more data. It's not a passive GNSS
> data feed in this case. So it's not going to be just a case of
> cat /dev/motmdm4 > /dev/ugnss. Without the serdev GNSS driver,
> it would be some-custom-app -i /dev/motmdm4 -o /dev/ugnss.

Yeah, I remember us discussing that briefly off list.
 
> And without the n_gsm serdev support, it's a mess of some app
> similar to [5] initializing n_gsm, trying to deal with the USB
> PHY PM, dealing with Motorola custom packet numbering, kicking
> GNSS device, feeding data to /dev/ugnss. Hmm I think I've already
> been there just to be able to type AT commands to the modem and
> it did not work :)

It's a mess indeed, but I'd rather see user-space dealing with until we
figure out how best to do it in the kernel. ;)

> Anyways, for the serdev kernel drivers, the criteria I've tried
> to follow is: "Can this serdev device driver make user space
> apps use standard Linux interfaces for the hardware?"
> 
> So for the serdev Alsa ASoC driver, user space can use the standard
> Alsa interface for setting voice call volume. And for the serdev
> GNSS driver, user space can use /dev/gnss0.

I understand. Both drivers appears to be using AT commands for control.
It would be interesting to hear what Mark has to say about the codec
driver too. Moving AT handling into the kernel scares me a bit. If we
already have a telephony stack to deal with it in user-space, my
inclination is to let it continue to handle it.

Modem-managed GNSS is also different from receivers connected directly
to the host. It's really the modem that drives the GNSS receiver, and
offers a higher-level interface to the host, for example, by buffering
output which the host can later request. It may or may not be the
kernel's job to periodically poll the modem to recreate an NMEA feed so
to speak.

But the end-result of having it accessible through a standard interface
is of course appealing.


I have some more comments on the gsm_serdev interface and motorola mfd,
but let's start with the above.

Johan


> [0] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/drivers/mfd/motorola-mdm.c?h=droid4-pending-mdm-v4.20
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/drivers/gnss/motmdm.c?h=droid4-pending-mdm-v4.20
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/tree/sound/soc/codecs/motmdm.c?h=droid4-pending-mdm-v4.20
> [3] https://github.com/tmlind/droid4-sms-tools
> [4] https://github.com/pavelmachek/ofono
> [5] https://github.com/tmlind/droid4-ngsm

Re: [PATCH 0/3] serdev support for n_gsm

[Tony Lindgren]

Hi,

* Johan Hovold <johan@kernel.org> [190124 16:39]:
> This series adds a new interface (gsm_serdev) but no users, which is not
> something we normally accept. I think you need to post the lot, at least
> as an RFC in order for the full picture to be visible.

Sure I can post them all as RFC series maybe this coming
weekend when I get a chance.

> Similarly, including an example device tree would also help with the
> overall picture. I've been able to derive the following (from your code
> and already merged phy driver binding doc):
> 
>    	mdm6600_phy: usb-phy {
> 		compatible = "motorola,mapphone-mdm6600";
> 		enable-gpios = <&gpio3 31 GPIO_ACTIVE_LOW>;
> 		power-gpios = <&gpio2 22 GPIO_ACTIVE_HIGH>;
> 		reset-gpios = <&gpio2 17 GPIO_ACTIVE_HIGH>;
> 		motorola,mode-gpios = <&gpio5 20 GPIO_ACTIVE_HIGH>,
> 				      <&gpio5 21 GPIO_ACTIVE_HIGH>;
> 		motorola,cmd-gpios = <&gpio4 7 GPIO_ACTIVE_HIGH>,
> 				     <&gpio4 8 GPIO_ACTIVE_HIGH>,
> 				     <&gpio5 14 GPIO_ACTIVE_HIGH>;
> 		motorola,status-gpios = <&gpio2 20 GPIO_ACTIVE_HIGH>,
> 					<&gpio2 21 GPIO_ACTIVE_HIGH>,
> 					<&gpio2 23 GPIO_ACTIVE_HIGH>;
> 		#phy-cells = <0>;
> 	};
> 
> 	&uart {
> 		gsm-mux? {
> 			compatible = "motorola,mapphone-mdm6600-serdev";
> 			phy = <&mdm6600_phy>;
> 
> 			gnss {
> 				compatible = "motorola,mapphone-mdm6600-gnss";
> 			};
> 
> 			audio-codec {
> 
> 			};
> 		};
> 	};
> 
> 	&ohci {
> 		phys = <&mdm6600_phy>;
> 	};
> 
> Which brings us back to the question of how to model modems. You've
> already added a phy-driver for something which really is some sort of
> modem representation.

That is not a "modem representation".

It's just doing what's typically done for drivers with firmware
like let's say WLAN pwrseq driver on SDIO bus. So start device,
enable clocks for the PHY so Linux won't oops when loading
ohci-platform device driver. It does not have much anything to
do beyond enabling communication over Linux standard buses like
TS 27.010 and USB. For more info, please see commit 5d1ebbda0318
("phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on
Droid 4").

It's too bad the USB PHY for the Linux SoC OHCI is on the modem
and clocked from the modem instead of what is usually done
where it would be clocked by the Linux running SoC. And too bad
some of the GPIO pins are shared with the TS 27.010 port. But hey,
it's just bad hardware design and not much we can do about it.
And it can be all isolated in the USB PHY driver.

> > And without the n_gsm serdev support, it's a mess of some app
> > similar to [5] initializing n_gsm, trying to deal with the USB
> > PHY PM, dealing with Motorola custom packet numbering, kicking
> > GNSS device, feeding data to /dev/ugnss. Hmm I think I've already
> > been there just to be able to type AT commands to the modem and
> > it did not work :)
> 
> It's a mess indeed, but I'd rather see user-space dealing with until we
> figure out how best to do it in the kernel. ;)

Well we really do not have user space dealing with it. And I'm
not planning on working on the user space approach any longer
for n_gsm, Motorola custom packet layer, and shared GPIO pins
for PM. That was proven to be a dead end alread and not worth
continuing.

For kicking the GNSS, that can be certainly done with a kernel
serdev driver or some custom user space app. With the $subject
patch we now have both options available unlike earlier.

> > Anyways, for the serdev kernel drivers, the criteria I've tried
> > to follow is: "Can this serdev device driver make user space
> > apps use standard Linux interfaces for the hardware?"
> > 
> > So for the serdev Alsa ASoC driver, user space can use the standard
> > Alsa interface for setting voice call volume. And for the serdev
> > GNSS driver, user space can use /dev/gnss0.
> 
> I understand. Both drivers appears to be using AT commands for control.
> It would be interesting to hear what Mark has to say about the codec
> driver too. Moving AT handling into the kernel scares me a bit. If we
> already have a telephony stack to deal with it in user-space, my
> inclination is to let it continue to handle it.

Right, too bad it's AT commands instead of let's say just
register offsets to a mem region on the modem. Or even some
enumerated commands. And yes it's the firmware doing all
kinds of things, just like with a let's say a WLAN driver.

And no, we don't really have a telephony stack to deal with it
in the user-space. But with the $subject patch it becomes doable
for the parts it makes sense to do, for ofono, ModemManager and
my phone scripts too.

For the Alsa driver, we need to configure the i2s channel
on the PMIC for time division multiplexing with set_tdm_slot()
for voice calls as the PMIC is the clock master.

> Modem-managed GNSS is also different from receivers connected directly
> to the host. It's really the modem that drives the GNSS receiver, and
> offers a higher-level interface to the host, for example, by buffering
> output which the host can later request. It may or may not be the
> kernel's job to periodically poll the modem to recreate an NMEA feed so
> to speak.

This is very similar to what we're doing with any driver that needs
firmware loaded, let's say again WLAN on SDIO. Or Bluetooth on SDIO.
Or a USB device with firmware :)

> But the end-result of having it accessible through a standard interface
> is of course appealing.

Yes I agree. That's the best way to make things work in a
standard way for Linux distros.

Based on my experience with trying to make embedded devices usable
with standard Linux distros, the irattach/ldattach/hciattach
route never worked well from user point of view.

If we can make things usable out of the box with simple channel
specific device drivers or user space apps it's a big improvment
for users. If we can also get rid of some duplicate copies of the
same code implementing the same thing in favor of using Linux
generic interfaces then even better :)

Then maybe later on if it turns out we have three or more
modems from various vendors needing similar feature it might
make sense to attempt to move some of the handling to a generic
framework like you did for GNSS. But currently I do not see
a need for that and for QMI based modems things are handled
in a different way anyways.

> I have some more comments on the gsm_serdev interface and motorola mfd,
> but let's start with the above.

Sure thanks, I'll post the RFC series of the pending patches.

Regards,

Tony

Re: [PATCH 0/3] serdev support for n_gsm

[Pavel Machek]

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

Hi!

> > And without the n_gsm serdev support, it's a mess of some app
> > similar to [5] initializing n_gsm, trying to deal with the USB
> > PHY PM, dealing with Motorola custom packet numbering, kicking
> > GNSS device, feeding data to /dev/ugnss. Hmm I think I've already
> > been there just to be able to type AT commands to the modem and
> > it did not work :)
> 
> It's a mess indeed, but I'd rather see user-space dealing with until we
> figure out how best to do it in the kernel. ;)
> 
> > Anyways, for the serdev kernel drivers, the criteria I've tried
> > to follow is: "Can this serdev device driver make user space
> > apps use standard Linux interfaces for the hardware?"
> > 
> > So for the serdev Alsa ASoC driver, user space can use the standard
> > Alsa interface for setting voice call volume. And for the serdev
> > GNSS driver, user space can use /dev/gnss0.
> 
> I understand. Both drivers appears to be using AT commands for control.
> It would be interesting to hear what Mark has to say about the codec
> driver too. Moving AT handling into the kernel scares me a bit. If we
> already have a telephony stack to deal with it in user-space, my
> inclination is to let it continue to handle it.

The userspace part of the telephony stack uses ALSA mixers to
do... well... audio mixer configuration for phone calls, and it would
be good if Droid 4 could keep the same interface.

> Modem-managed GNSS is also different from receivers connected directly
> to the host. It's really the modem that drives the GNSS receiver, and
> offers a higher-level interface to the host, for example, by buffering
> output which the host can later request. It may or may not be the
> kernel's job to periodically poll the modem to recreate an NMEA feed so
> to speak.
> 
> But the end-result of having it accessible through a standard interface
> is of course appealing.

Yes please. On N900, there's special (socket-based) interface to the
GPS... and it si painful.

Plus, it will take some time before modem support on Droid 4
stabilizes... and it would be very welcome to be able to use the GPS
in the meantime.

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/3] serdev support for n_gsm

[Pavel Machek]

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

Hi!

> and sorry about the reply latency. This is quite a lot to think about.
> 
> I'm also adding Kishon and Mark on CC (e.g. for the phy and ASoC
> bits).

I just wanted to ask... any news here?

> > For some user space examples, I have posted scripts to send and receive
> > SMS at [3], and Pavel has ofono patches [4] below. Seems like we can
> > also add support to ModemManager along the similar lines. And for the
> > serdev drivers, those implement standard Linux interfaces for apps
> > to use.
> > 
> > For PM, about a year ago I tried making things work with a user space
> > solution and it sucked big time[5]. The power management makes sense
> > to do in the kernel driver at least in this case as there are shared
> > GPIO pins between the USB PHY and TS 27.010 UART. The shared GPIOs
> > are handled by the phy-mapphone-mdm6600 driver.
> > 
> > With the serdev n_gsm MFD driver, the only thing that needs to be done
> > to idle the modem is to enable autosuspend for the OHCI interface. So
> > no spefific coordination between various components is needed for PM
> > beyond that. Things idle just fine using PM runtime.
> 
> Yeah, I don't envy you trying to get this to work (and now I'm getting
> dragged into it ;) ).

Yeah, and now I'm in, too. I'd really like to have an useful
phone. Droid4 seems like best candidate.

> It would really help with a high-level outline of the modem and its
> components. I've done my best to derive it from these patches and the
> code you link to, but that info needs to go in the patch descriptions
> (or cover letter).

Are you ready for the crazyness?

There are two modems in droid 4. I don't care about the LTE one. The
GSM one is connected with few USB channels, and few multiplexed
channels over UART.

One of USB channels is standard AT commands.
One of USB channels is QMI.
But using USB means big power consumption, so it is better to use
multiplexed channels over UART.

Few of those look vaguely like AT commands, but voice and sms and
... are going over separate channels. One of those contains NMEA data
(packet in AT lookalike).

> > Sure that's doable. But notice that we actually need to kick the
> > serdev GNSS interface to get more data. It's not a passive GNSS
> > data feed in this case. So it's not going to be just a case of
> > cat /dev/motmdm4 > /dev/ugnss. Without the serdev GNSS driver,
> > it would be some-custom-app -i /dev/motmdm4 -o /dev/ugnss.
> 
> Yeah, I remember us discussing that briefly off list.
>  
> > And without the n_gsm serdev support, it's a mess of some app
> > similar to [5] initializing n_gsm, trying to deal with the USB
> > PHY PM, dealing with Motorola custom packet numbering, kicking
> > GNSS device, feeding data to /dev/ugnss. Hmm I think I've already
> > been there just to be able to type AT commands to the modem and
> > it did not work :)
> 
> It's a mess indeed, but I'd rather see user-space dealing with until we
> figure out how best to do it in the kernel. ;)

Userspace should be shielded from hardware-specific mess :-(.

> > Anyways, for the serdev kernel drivers, the criteria I've tried
> > to follow is: "Can this serdev device driver make user space
> > apps use standard Linux interfaces for the hardware?"
> > 
> > So for the serdev Alsa ASoC driver, user space can use the standard
> > Alsa interface for setting voice call volume. And for the serdev
> > GNSS driver, user space can use /dev/gnss0.
> 
> I understand. Both drivers appears to be using AT commands for control.
> It would be interesting to hear what Mark has to say about the codec
> driver too. Moving AT handling into the kernel scares me a bit. If we
> already have a telephony stack to deal with it in user-space, my
> inclination is to let it continue to handle it.

These "Motorola AT" commands are really a bit different from standard
AT commands. I was working on userspace, and got something... but
could not get SMSes to work.

> Modem-managed GNSS is also different from receivers connected directly
> to the host. It's really the modem that drives the GNSS receiver, and
> offers a higher-level interface to the host, for example, by buffering
> output which the host can later request. It may or may not be the
> kernel's job to periodically poll the modem to recreate an NMEA feed so
> to speak.
> 
> But the end-result of having it accessible through a standard interface
> is of course appealing.

We'd really like unified interface for the GPS receivers, please. In
the Droid4 case, there's separate channel on the UART that has just
the GPS... so it is really quite similar to normal GPS.

We won't have proper driver for the modem anytime soon, but it would
be good to be able to use the GPS in the meantime.

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 --]