[PATCH 01/13] tty_ldisc: add more limits to the @write_wakeup

[PATCH 01/13] tty_ldisc: add more limits to the @write_wakeup

[Felipe Balbi]

From: Huang Shijie <b32955@freescale.com>

In the uart_handle_cts_change(), uart_write_wakeup() is called after
we call @uart_port->ops->start_tx().

The Documentation/serial/driver tells us:
-----------------------------------------------
  start_tx(port)
	Start transmitting characters.

	Locking: port->lock taken.
	Interrupts: locally disabled.
-----------------------------------------------

So when the uart_write_wakeup() is called, the port->lock is taken by
the upper. See the following callstack:

	|_ uart_write_wakeup
	   |_ tty_wakeup
	      |_ ld->ops->write_wakeup

With the port->lock held, we call the @write_wakeup. Some implemetation of
the @write_wakeup does not notice that the port->lock is held, and it still
tries to send data with uart_write() which will try to grab the prot->lock.
A dead lock occurs, see the following log caught in the Bluetooth by uart:

--------------------------------------------------------------------
BUG: spinlock lockup suspected on CPU#0, swapper/0/0
 lock: 0xdc3f4410, .magic: dead4ead, .owner: swapper/0/0, .owner_cpu: 0
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W    3.10.17-16839-ge4a1bef #1320
[<80014cbc>] (unwind_backtrace+0x0/0x138) from [<8001251c>] (show_stack+0x10/0x14)
[<8001251c>] (show_stack+0x10/0x14) from [<802816ac>] (do_raw_spin_lock+0x108/0x184)
[<802816ac>] (do_raw_spin_lock+0x108/0x184) from [<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60)
[<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60) from [<802f5754>] (uart_write+0x38/0xe0)
[<802f5754>] (uart_write+0x38/0xe0) from [<80455270>] (hci_uart_tx_wakeup+0xa4/0x168)
[<80455270>] (hci_uart_tx_wakeup+0xa4/0x168) from [<802dab18>] (tty_wakeup+0x50/0x5c)
[<802dab18>] (tty_wakeup+0x50/0x5c) from [<802f81a4>] (imx_rtsint+0x50/0x80)
[<802f81a4>] (imx_rtsint+0x50/0x80) from [<802f88f4>] (imx_int+0x158/0x17c)
[<802f88f4>] (imx_int+0x158/0x17c) from [<8007abe0>] (handle_irq_event_percpu+0x50/0x194)
[<8007abe0>] (handle_irq_event_percpu+0x50/0x194) from [<8007ad60>] (handle_irq_event+0x3c/0x5c)
--------------------------------------------------------------------

This patch adds more limits to the @write_wakeup, the one who wants to
implemet the @write_wakeup should follow the limits which avoid the deadlock.

Signed-off-by: Huang Shijie <b32955@freescale.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 include/linux/tty_ldisc.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index add26da..00c9d68 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -92,7 +92,10 @@
  *	This function is called by the low-level tty driver to signal
  *	that line discpline should try to send more characters to the
  *	low-level driver for transmission.  If the line discpline does
- *	not have any more data to send, it can just return.
+ *	not have any more data to send, it can just return. If the line
+ *	discipline does have some data to send, please arise a tasklet
+ *	or workqueue to do the real data transfer. Do not send data in
+ *	this hook, it may leads to a deadlock.
  *
  * int (*hangup)(struct tty_struct *)
  *
-- 
1.9.2.459.g68773ac

[PATCH 02/13] bluetooth: hci_ldisc: fix deadlock condition

[Felipe Balbi]

LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().

->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.

Acked-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
Reported-by: Huang Shijie <b32955@freescale.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
 drivers/bluetooth/hci_uart.h  |  1 +
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index f1fbf4f..e00f8f5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
 
 int hci_uart_tx_wakeup(struct hci_uart *hu)
 {
-	struct tty_struct *tty = hu->tty;
-	struct hci_dev *hdev = hu->hdev;
-	struct sk_buff *skb;
-
 	if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
 		set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
 		return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
 
 	BT_DBG("");
 
+	schedule_work(&hu->write_work);
+
+	return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+	struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+	struct tty_struct *tty = hu->tty;
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+
+	/* REVISIT: should we cope with bad skbs or ->write() returning
+	 * and error value ?
+	 */
+
 restart:
 	clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
 
@@ -153,7 +165,6 @@ restart:
 		goto restart;
 
 	clear_bit(HCI_UART_SENDING, &hu->tx_state);
-	return 0;
 }
 
 static void hci_uart_init_work(struct work_struct *work)
@@ -282,6 +293,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	tty->receive_room = 65536;
 
 	INIT_WORK(&hu->init_ready, hci_uart_init_work);
+	INIT_WORK(&hu->write_work, hci_uart_write_work);
 
 	spin_lock_init(&hu->rx_lock);
 
@@ -319,6 +331,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 	if (hdev)
 		hci_uart_close(hdev);
 
+	cancel_work_sync(&hu->write_work);
+
 	if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
 		if (hdev) {
 			if (test_bit(HCI_UART_REGISTERED, &hu->flags))
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index fffa61f..12df101 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -68,6 +68,7 @@ struct hci_uart {
 	unsigned long		hdev_flags;
 
 	struct work_struct	init_ready;
+	struct work_struct	write_work;
 
 	struct hci_uart_proto	*proto;
 	void			*priv;
-- 
1.9.2.459.g68773ac

Re: [PATCH 02/13] bluetooth: hci_ldisc: fix deadlock condition

[Andreas Bießmann]

Dear Felipe Balbi,

On Wed, Apr 23, 2014 at 09:58:26AM -0500, Felipe Balbi wrote:
> LDISCs shouldn't call tty->ops->write() from within
> ->write_wakeup().
> 
> ->write_wakeup() is called with port lock taken and
> IRQs disabled, tty->ops->write() will try to acquire
> the same port lock and we will deadlock.
> 
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
> Reported-by: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>

Tested-by: Andreas Bießmann <andreas@biessmann.de>

on custom TI AM37xx board with 3.4.87. Therefore I vote for adding this to
stable branches (at least 3.4). It applies with a slight line shifting
(init_work is not available there).

I wonder if you have seen my approach [1] to fix this deadlock.  This stuff is
quiet new to me. As I understood the work_queue has a bit more overhead than a
tasklet. Therefore I implemented my fix with a tasklet. Would be great if one
could shed some light on that.

Best regards

Andreas Bießmann

[1] https://lkml.org/lkml/2014/4/16/425

> ---
>  drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
>  drivers/bluetooth/hci_uart.h  |  1 +
>  2 files changed, 20 insertions(+), 5 deletions(-)

[PATCH] bluetooth: hci_ldisc: fix deadlock condition

[Tim Niemeyer]

This Patch was applied to 3.10 and 3.2. It's probably missed on 3.4.

---------------------

From: Felipe Balbi <balbi@ti.com>

LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().

->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.

Acked-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
Reported-by: Huang Shijie <b32955@freescale.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Tested-by: Andreas Bießmann <andreas@biessmann.de>
[tim.niemeyer@corscience.de: rebased on 3.4.103]
Signed-off-by: Tim Niemeyer <tim.niemeyer@corscience.de>
---
 drivers/bluetooth/hci_ldisc.c |   25 ++++++++++++++++++++-----
 drivers/bluetooth/hci_uart.h  |    2 ++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 98a8c05..d4550f9 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
 
 int hci_uart_tx_wakeup(struct hci_uart *hu)
 {
-	struct tty_struct *tty = hu->tty;
-	struct hci_dev *hdev = hu->hdev;
-	struct sk_buff *skb;
-
 	if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
 		set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
 		return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
 
 	BT_DBG("");
 
+	schedule_work(&hu->write_work);
+
+	return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+	struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+	struct tty_struct *tty = hu->tty;
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+
+	/* REVISIT: should we cope with bad skbs or ->write() returning
+	 * and error value ?
+	 */
+
 restart:
 	clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
 
@@ -153,7 +165,6 @@ restart:
 		goto restart;
 
 	clear_bit(HCI_UART_SENDING, &hu->tx_state);
-	return 0;
 }
 
 /* ------- Interface to HCI layer ------ */
@@ -264,6 +275,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	hu->tty = tty;
 	tty->receive_room = 65536;
 
+	INIT_WORK(&hu->write_work, hci_uart_write_work);
+
 	spin_lock_init(&hu->rx_lock);
 
 	/* Flush any pending characters in the driver and line discipline. */
@@ -298,6 +311,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 		if (hdev)
 			hci_uart_close(hdev);
 
+		cancel_work_sync(&hu->write_work);
+
 		if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
 			if (hdev) {
 				hci_unregister_dev(hdev);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 6cf6ab22..af93d83 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -66,6 +66,8 @@ struct hci_uart {
 	unsigned long		flags;
 	unsigned long		hdev_flags;
 
+	struct work_struct	write_work;
+
 	struct hci_uart_proto	*proto;
 	void			*priv;
 
-- 
1.7.10.4

Re: [PATCH] bluetooth: hci_ldisc: fix deadlock condition

[Greg KH]

On Fri, Aug 22, 2014 at 03:45:07PM +0200, Tim Niemeyer wrote:
> This Patch was applied to 3.10 and 3.2. It's probably missed on 3.4.

Does it apply there?

What is the git id of the commit?

thanks,

greg k-h

Re: [PATCH] bluetooth: hci_ldisc: fix deadlock condition

[Tim Niemeyer]

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

Hi

Am Freitag, den 22.08.2014, 10:51 -0500 schrieb Greg KH:
> On Fri, Aug 22, 2014 at 03:45:07PM +0200, Tim Niemeyer wrote:
> > This Patch was applied to 3.10 and 3.2. It's probably missed on 3.4.
> 
> Does it apply there?
The original patch applies not cleanly on 3.4. The init_work is not
available there.

I backported it and it's the same result as the backport from Andreas.
He tested the patch on 3.4.87. The attached patch of my last mail
applies.

> What is the git id of the commit?
Sorry, missed it:

commit da64c27d3c93ee9f89956b9de86c4127eb244494 upstream
commit a22d29e6e5757b1daed7d0b409a815eb33f66e4e stable-3.10
commit 87520218746b8d973670e37237666f174590898c stable-3.2

Tim

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 03/13] Revert "serial: omap: unlock the port lock"

[Felipe Balbi]

This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.

That commit tried to fix a deadlock problem when using
hci_ldisc, but it turns out the bug was in hci_ldsic
all along where it was calling ->write() from within
->write_wakeup() callback.

The problem is that ->write_wakeup() was called with
port lock held and ->write() tried to grab the same
port lock.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 08b6b94..837f6c1 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -398,11 +398,8 @@ static void transmit_chars(struct uart_omap_port *up, unsigned int lsr)
 			break;
 	} while (--count > 0);
 
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
-		spin_unlock(&up->port.lock);
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&up->port);
-		spin_lock(&up->port.lock);
-	}
 
 	if (uart_circ_empty(xmit))
 		serial_omap_stop_tx(&up->port);
-- 
1.9.2.459.g68773ac

[PATCH 04/13] serial: fix UART_IIR_ID

[Felipe Balbi]

UART IRQ Identification bitfield is 3
bits long (bits 3:1) but current mask only
masks 2 bits. Fix it.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 include/uapi/linux/serial_reg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index e632260..99b4705 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -32,7 +32,7 @@
 
 #define UART_IIR	2	/* In:  Interrupt ID Register */
 #define UART_IIR_NO_INT		0x01 /* No interrupts pending */
-#define UART_IIR_ID		0x06 /* Mask for the interrupt ID */
+#define UART_IIR_ID		0x0e /* Mask for the interrupt ID */
 #define UART_IIR_MSI		0x00 /* Modem status interrupt */
 #define UART_IIR_THRI		0x02 /* Transmitter holding register empty */
 #define UART_IIR_RDI		0x04 /* Receiver data interrupt */
-- 
1.9.2.459.g68773ac

[PATCH 05/13] tty: serial: add missing braces

[Felipe Balbi]

per CodingStyle we should have those braces, no
functional changes.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 837f6c1..f456f46 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1694,8 +1694,10 @@ static int serial_omap_probe(struct platform_device *pdev)
 	    omap_up_info->DTR_present) {
 		up->DTR_gpio = omap_up_info->DTR_gpio;
 		up->DTR_inverted = omap_up_info->DTR_inverted;
-	} else
+	} else {
 		up->DTR_gpio = -EINVAL;
+	}
+
 	up->DTR_active = 0;
 
 	up->dev = &pdev->dev;
@@ -1757,6 +1759,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, up);
 	if (omap_up_info->autosuspend_timeout == 0)
 		omap_up_info->autosuspend_timeout = -1;
+
 	device_init_wakeup(up->dev, true);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev,
-- 
1.9.2.459.g68773ac

[PATCH 06/13] tty: serial: omap: switch over to devm_request_gpio

[Felipe Balbi]

this will make sure gpio gets freed automatically
when this device is destroyed.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f456f46..07d4273 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1611,7 +1611,7 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
 	/* check for tx enable gpio */
 	up->rts_gpio = of_get_named_gpio_flags(np, "rts-gpio", 0, &flags);
 	if (gpio_is_valid(up->rts_gpio)) {
-		ret = gpio_request(up->rts_gpio, "omap-serial");
+		ret = devm_gpio_request(up->dev, up->rts_gpio, "omap-serial");
 		if (ret < 0)
 			return ret;
 		ret = gpio_direction_output(up->rts_gpio,
@@ -1677,7 +1677,8 @@ static int serial_omap_probe(struct platform_device *pdev)
 
 	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
 	    omap_up_info->DTR_present) {
-		ret = gpio_request(omap_up_info->DTR_gpio, "omap-serial");
+		ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
+				"omap-serial");
 		if (ret < 0)
 			return ret;
 		ret = gpio_direction_output(omap_up_info->DTR_gpio,
-- 
1.9.2.459.g68773ac

[PATCH 07/13] tty: serial: omap: cleanup variable declarations

[Felipe Balbi]

cleanup only, no functional changes.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 07d4273..3813740 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1641,10 +1641,13 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
 
 static int serial_omap_probe(struct platform_device *pdev)
 {
-	struct uart_omap_port	*up;
-	struct resource		*mem, *irq;
 	struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
-	int ret, uartirq = 0, wakeirq = 0;
+	struct uart_omap_port *up;
+	struct resource *mem;
+	struct resource *irq;
+	int uartirq = 0;
+	int wakeirq = 0;
+	int ret;
 
 	/* The optional wakeirq may be specified in the board dts file */
 	if (pdev->dev.of_node) {
-- 
1.9.2.459.g68773ac

[PATCH 08/13] tty: serial: omap: switch over to platform_get_resource

[Felipe Balbi]

this way we can remove one pointer declaration.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 3813740..cb45e88 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1644,7 +1644,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 	struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
 	struct uart_omap_port *up;
 	struct resource *mem;
-	struct resource *irq;
 	int uartirq = 0;
 	int wakeirq = 0;
 	int ret;
@@ -1658,12 +1657,9 @@ static int serial_omap_probe(struct platform_device *pdev)
 		omap_up_info = of_get_uart_port_info(&pdev->dev);
 		pdev->dev.platform_data = omap_up_info;
 	} else {
-		irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-		if (!irq) {
-			dev_err(&pdev->dev, "no irq resource?\n");
-			return -ENODEV;
-		}
-		uartirq = irq->start;
+		uartirq = platform_get_irq(pdev, 0);
+		if (uartirq < 0)
+			return -EPROBE_DEFER;
 	}
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.9.2.459.g68773ac

Re: [PATCH 08/13] tty: serial: omap: switch over to platform_get_resource

[Fabio Estevam]

On Wed, Apr 23, 2014 at 11:58 AM, Felipe Balbi <balbi@ti.com> wrote:

> @@ -1658,12 +1657,9 @@ static int serial_omap_probe(struct platform_device *pdev)
>                 omap_up_info = of_get_uart_port_info(&pdev->dev);
>                 pdev->dev.platform_data = omap_up_info;
>         } else {
> -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> -               if (!irq) {
> -                       dev_err(&pdev->dev, "no irq resource?\n");
> -                       return -ENODEV;
> -               }
> -               uartirq = irq->start;
> +               uartirq = platform_get_irq(pdev, 0);
> +               if (uartirq < 0)
> +                       return -EPROBE_DEFER;


Maybe you could just do a 'return uartirq' here instead.

Re: [PATCH 08/13] tty: serial: omap: switch over to platform_get_resource

[Felipe Balbi]

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

On Wed, Apr 23, 2014 at 12:27:59PM -0300, Fabio Estevam wrote:
> On Wed, Apr 23, 2014 at 11:58 AM, Felipe Balbi <balbi@ti.com> wrote:
> 
> > @@ -1658,12 +1657,9 @@ static int serial_omap_probe(struct platform_device *pdev)
> >                 omap_up_info = of_get_uart_port_info(&pdev->dev);
> >                 pdev->dev.platform_data = omap_up_info;
> >         } else {
> > -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > -               if (!irq) {
> > -                       dev_err(&pdev->dev, "no irq resource?\n");
> > -                       return -ENODEV;
> > -               }
> > -               uartirq = irq->start;
> > +               uartirq = platform_get_irq(pdev, 0);
> > +               if (uartirq < 0)
> > +                       return -EPROBE_DEFER;
> 
> 
> Maybe you could just do a 'return uartirq' here instead.

I don't mind either way, I'm only returning -EPROBE_DEFER because that's
what the other branch of this conditional returns. Tony, what do you
prefer ?

-- 
balbi

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

[PATCH 09/13] tty: serial: omap: switch over to devm_ioremap_resource

[Felipe Balbi]

just using helper function to remove some duplicated
code a bit. While at that, also move allocation of
struct uart_omap_port higher in the code so that
we return much earlier in case of no memory.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index cb45e88..b46aaf3 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1644,6 +1644,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 	struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
 	struct uart_omap_port *up;
 	struct resource *mem;
+	void __iomem *base;
 	int uartirq = 0;
 	int wakeirq = 0;
 	int ret;
@@ -1662,17 +1663,14 @@ static int serial_omap_probe(struct platform_device *pdev)
 			return -EPROBE_DEFER;
 	}
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!mem) {
-		dev_err(&pdev->dev, "no mem resource?\n");
-		return -ENODEV;
-	}
+	up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
+	if (!up)
+		return -ENOMEM;
 
-	if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
-				pdev->dev.driver->name)) {
-		dev_err(&pdev->dev, "memory region already claimed\n");
-		return -EBUSY;
-	}
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
 
 	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
 	    omap_up_info->DTR_present) {
@@ -1686,10 +1684,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
-	if (!up)
-		return -ENOMEM;
-
 	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
 	    omap_up_info->DTR_present) {
 		up->DTR_gpio = omap_up_info->DTR_gpio;
@@ -1732,14 +1726,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 
 	sprintf(up->name, "OMAP UART%d", up->port.line);
 	up->port.mapbase = mem->start;
-	up->port.membase = devm_ioremap(&pdev->dev, mem->start,
-						resource_size(mem));
-	if (!up->port.membase) {
-		dev_err(&pdev->dev, "can't ioremap UART\n");
-		ret = -ENOMEM;
-		goto err_ioremap;
-	}
-
+	up->port.membase = base;
 	up->port.flags = omap_up_info->flags;
 	up->port.uartclk = omap_up_info->uartclk;
 	if (!up->port.uartclk) {
@@ -1786,7 +1773,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 err_add_port:
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-err_ioremap:
 err_rs485:
 err_port_line:
 	dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
-- 
1.9.2.459.g68773ac

[PATCH 10/13] tty: serial: omap: remove some dead code

[Felipe Balbi]

nobody passes a DTR_gpio to this driver, so
this code is not necessary.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
 1 file changed, 39 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index b46aaf3..6654682 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -163,10 +163,6 @@ struct uart_omap_port {
 	u8			wakeups_enabled;
 	u32			features;
 
-	int			DTR_gpio;
-	int			DTR_inverted;
-	int			DTR_active;
-
 	struct serial_rs485	rs485;
 	int			rts_gpio;
 
@@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	serial_out(up, UART_MCR, up->mcr);
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
-
-	if (gpio_is_valid(up->DTR_gpio) &&
-	    !!(mctrl & TIOCM_DTR) != up->DTR_active) {
-		up->DTR_active = !up->DTR_active;
-		if (gpio_cansleep(up->DTR_gpio))
-			schedule_work(&up->qos_work);
-		else
-			gpio_set_value(up->DTR_gpio,
-				       up->DTR_active != up->DTR_inverted);
-	}
 }
 
 static void serial_omap_break_ctl(struct uart_port *port, int break_state)
@@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
 						qos_work);
 
 	pm_qos_update_request(&up->pm_qos_request, up->latency);
-	if (gpio_is_valid(up->DTR_gpio))
-		gpio_set_value_cansleep(up->DTR_gpio,
-					up->DTR_active != up->DTR_inverted);
 }
 
 static void
@@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
-	    omap_up_info->DTR_present) {
-		ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
-				"omap-serial");
-		if (ret < 0)
-			return ret;
-		ret = gpio_direction_output(omap_up_info->DTR_gpio,
-					    omap_up_info->DTR_inverted);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
-	    omap_up_info->DTR_present) {
-		up->DTR_gpio = omap_up_info->DTR_gpio;
-		up->DTR_inverted = omap_up_info->DTR_inverted;
-	} else {
-		up->DTR_gpio = -EINVAL;
-	}
-
-	up->DTR_active = 0;
-
 	up->dev = &pdev->dev;
 	up->port.dev = &pdev->dev;
 	up->port.type = PORT_OMAP;
-- 
1.9.2.459.g68773ac

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[Felipe Balbi]

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

+Neil

On Wed, Apr 23, 2014 at 09:58:34AM -0500, Felipe Balbi wrote:
> nobody passes a DTR_gpio to this driver, so
> this code is not necessary.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
>  1 file changed, 39 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index b46aaf3..6654682 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -163,10 +163,6 @@ struct uart_omap_port {
>  	u8			wakeups_enabled;
>  	u32			features;
>  
> -	int			DTR_gpio;
> -	int			DTR_inverted;
> -	int			DTR_active;
> -
>  	struct serial_rs485	rs485;
>  	int			rts_gpio;
>  
> @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	serial_out(up, UART_MCR, up->mcr);
>  	pm_runtime_mark_last_busy(up->dev);
>  	pm_runtime_put_autosuspend(up->dev);
> -
> -	if (gpio_is_valid(up->DTR_gpio) &&
> -	    !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> -		up->DTR_active = !up->DTR_active;
> -		if (gpio_cansleep(up->DTR_gpio))
> -			schedule_work(&up->qos_work);
> -		else
> -			gpio_set_value(up->DTR_gpio,
> -				       up->DTR_active != up->DTR_inverted);
> -	}
>  }
>  
>  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
>  						qos_work);
>  
>  	pm_qos_update_request(&up->pm_qos_request, up->latency);
> -	if (gpio_is_valid(up->DTR_gpio))
> -		gpio_set_value_cansleep(up->DTR_gpio,
> -					up->DTR_active != up->DTR_inverted);
>  }
>  
>  static void
> @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> -	    omap_up_info->DTR_present) {
> -		ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> -				"omap-serial");
> -		if (ret < 0)
> -			return ret;
> -		ret = gpio_direction_output(omap_up_info->DTR_gpio,
> -					    omap_up_info->DTR_inverted);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> -	    omap_up_info->DTR_present) {
> -		up->DTR_gpio = omap_up_info->DTR_gpio;
> -		up->DTR_inverted = omap_up_info->DTR_inverted;
> -	} else {
> -		up->DTR_gpio = -EINVAL;
> -	}
> -
> -	up->DTR_active = 0;
> -
>  	up->dev = &pdev->dev;
>  	up->port.dev = &pdev->dev;
>  	up->port.type = PORT_OMAP;
> -- 
> 1.9.2.459.g68773ac
> 

-- 
balbi

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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[Nishanth Menon]

On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> nobody passes a DTR_gpio to this driver, so
> this code is not necessary.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---

Niel,
this seems to revert the functionality introduced in
commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
(OMAP/serial: Add support for driving a GPIO as DTR.)

would you like to Ack this change?

>  drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
>  1 file changed, 39 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index b46aaf3..6654682 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -163,10 +163,6 @@ struct uart_omap_port {
>  	u8			wakeups_enabled;
>  	u32			features;
>  
> -	int			DTR_gpio;
> -	int			DTR_inverted;
> -	int			DTR_active;
> -
>  	struct serial_rs485	rs485;
>  	int			rts_gpio;
>  
> @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	serial_out(up, UART_MCR, up->mcr);
>  	pm_runtime_mark_last_busy(up->dev);
>  	pm_runtime_put_autosuspend(up->dev);
> -
> -	if (gpio_is_valid(up->DTR_gpio) &&
> -	    !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> -		up->DTR_active = !up->DTR_active;
> -		if (gpio_cansleep(up->DTR_gpio))
> -			schedule_work(&up->qos_work);
> -		else
> -			gpio_set_value(up->DTR_gpio,
> -				       up->DTR_active != up->DTR_inverted);
> -	}
>  }
>  
>  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
>  						qos_work);
>  
>  	pm_qos_update_request(&up->pm_qos_request, up->latency);
> -	if (gpio_is_valid(up->DTR_gpio))
> -		gpio_set_value_cansleep(up->DTR_gpio,
> -					up->DTR_active != up->DTR_inverted);
>  }
>  
>  static void
> @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> -	    omap_up_info->DTR_present) {
> -		ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> -				"omap-serial");
> -		if (ret < 0)
> -			return ret;
> -		ret = gpio_direction_output(omap_up_info->DTR_gpio,
> -					    omap_up_info->DTR_inverted);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> -	    omap_up_info->DTR_present) {
> -		up->DTR_gpio = omap_up_info->DTR_gpio;
> -		up->DTR_inverted = omap_up_info->DTR_inverted;
> -	} else {
> -		up->DTR_gpio = -EINVAL;
> -	}
> -
> -	up->DTR_active = 0;
> -
>  	up->dev = &pdev->dev;
>  	up->port.dev = &pdev->dev;
>  	up->port.type = PORT_OMAP;
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[NeilBrown]

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

On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote:

> On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > nobody passes a DTR_gpio to this driver, so
> > this code is not necessary.
> > 
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> 
> Niel,
> this seems to revert the functionality introduced in
> commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> (OMAP/serial: Add support for driving a GPIO as DTR.)
> 
> would you like to Ack this change?

I have a couple of out-of-tree drivers that use this support.

I hope to get back to working on that code one day and even get those drivers
upstream.  So I would really prefer this code to remain if it isn't causing
any actual problems.

Of course, I can always re-submit it when I need it again, but that it just
extra work all around.

Sorry that I have pushed those drivers already, but sometimes life gets in
the way :-)

Thanks,
NeilBrown


> 
> >  drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
> >  1 file changed, 39 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > index b46aaf3..6654682 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -163,10 +163,6 @@ struct uart_omap_port {
> >  	u8			wakeups_enabled;
> >  	u32			features;
> >  
> > -	int			DTR_gpio;
> > -	int			DTR_inverted;
> > -	int			DTR_active;
> > -
> >  	struct serial_rs485	rs485;
> >  	int			rts_gpio;
> >  
> > @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
> >  	serial_out(up, UART_MCR, up->mcr);
> >  	pm_runtime_mark_last_busy(up->dev);
> >  	pm_runtime_put_autosuspend(up->dev);
> > -
> > -	if (gpio_is_valid(up->DTR_gpio) &&
> > -	    !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> > -		up->DTR_active = !up->DTR_active;
> > -		if (gpio_cansleep(up->DTR_gpio))
> > -			schedule_work(&up->qos_work);
> > -		else
> > -			gpio_set_value(up->DTR_gpio,
> > -				       up->DTR_active != up->DTR_inverted);
> > -	}
> >  }
> >  
> >  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> > @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
> >  						qos_work);
> >  
> >  	pm_qos_update_request(&up->pm_qos_request, up->latency);
> > -	if (gpio_is_valid(up->DTR_gpio))
> > -		gpio_set_value_cansleep(up->DTR_gpio,
> > -					up->DTR_active != up->DTR_inverted);
> >  }
> >  
> >  static void
> > @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
> >  	if (IS_ERR(base))
> >  		return PTR_ERR(base);
> >  
> > -	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> > -	    omap_up_info->DTR_present) {
> > -		ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> > -				"omap-serial");
> > -		if (ret < 0)
> > -			return ret;
> > -		ret = gpio_direction_output(omap_up_info->DTR_gpio,
> > -					    omap_up_info->DTR_inverted);
> > -		if (ret < 0)
> > -			return ret;
> > -	}
> > -
> > -	if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> > -	    omap_up_info->DTR_present) {
> > -		up->DTR_gpio = omap_up_info->DTR_gpio;
> > -		up->DTR_inverted = omap_up_info->DTR_inverted;
> > -	} else {
> > -		up->DTR_gpio = -EINVAL;
> > -	}
> > -
> > -	up->DTR_active = 0;
> > -
> >  	up->dev = &pdev->dev;
> >  	up->port.dev = &pdev->dev;
> >  	up->port.type = PORT_OMAP;
> > 
> 
> 


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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[Felipe Balbi]

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

Hi,

On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote:
> On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote:
> 
> > On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > > nobody passes a DTR_gpio to this driver, so
> > > this code is not necessary.
> > > 
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > 
> > Niel,
> > this seems to revert the functionality introduced in
> > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> > (OMAP/serial: Add support for driving a GPIO as DTR.)
> > 
> > would you like to Ack this change?
> 
> I have a couple of out-of-tree drivers that use this support.
> 
> I hope to get back to working on that code one day and even get those drivers
> upstream.  So I would really prefer this code to remain if it isn't causing
> any actual problems.

it causes problem with DT (not really). That suport is only available on
legacy platform_data-based boot, it's not available on DT. I hear Tony
is pretty close to turning OMAP3 DT-only.

> Of course, I can always re-submit it when I need it again, but that it just
> extra work all around.

I wonder how you will pass those attributes through DT considering they
are *really* SW configuration. Why can't you use the real DTR pin, btw ?

-- 
balbi

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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[NeilBrown]

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

On Wed, 23 Apr 2014 18:01:21 -0500 Felipe Balbi <balbi@ti.com> wrote:

> Hi,
> 
> On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote:
> > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote:
> > 
> > > On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > > > nobody passes a DTR_gpio to this driver, so
> > > > this code is not necessary.
> > > > 
> > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > ---
> > > 
> > > Niel,
> > > this seems to revert the functionality introduced in
> > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> > > (OMAP/serial: Add support for driving a GPIO as DTR.)
> > > 
> > > would you like to Ack this change?
> > 
> > I have a couple of out-of-tree drivers that use this support.
> > 
> > I hope to get back to working on that code one day and even get those drivers
> > upstream.  So I would really prefer this code to remain if it isn't causing
> > any actual problems.
> 
> it causes problem with DT (not really). That suport is only available on
> legacy platform_data-based boot, it's not available on DT. I hear Tony
> is pretty close to turning OMAP3 DT-only.
> 
> > Of course, I can always re-submit it when I need it again, but that it just
> > extra work all around.
> 
> I wonder how you will pass those attributes through DT considering they
> are *really* SW configuration. Why can't you use the real DTR pin, btw ?
> 

This myth that DT is only about hardware is probably one of the reasons that I
haven't got these out-of-tree drivers upstream yet.

There is no "real" DTR pin.

When I open /dev/ttyWHATEVER, I need the driver for the device that is
permanently connected to that serial port to be told that the serial-device
has been opened so that it can "power on" or "wake up" the device.

I don't much care how that happens, or how I tell DT that it has to happen.
I just need it to happen.

In discrete hardware devices, the DTR line is what you would use.  The RS232
port would raise (or lower or whatever) DTR when /dev/ttyWHATEVER was open,
and  the device that was plugged in would detect the level change and do
stuff.

But I don't have discrete hardware.  I have a bunch of stuff soldered onto a
board with ad-hoc connections chosen to make the life of the hardware builder
easy rather than chosen to make the life of the software developer easy
(which I think is the correct choice).

So I need to tell DT "This device is plugged into this UART, and there is no
DTR line, but when the UARTs DTR line would be asserted (if it had one), then
I need that regulator of there turned on". or maybe "I need to toggle this
GPIO  with exactly this pattern, while watching that GPIO to see if it is
working".

So I thought:

 1/ give the UART a "virtual" DTR so it could drive any GPIO
 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
    and responded to state changes on that GPIO by turning on or off the
    regulator
 3/ create a dedicated driver which knows how to toggle the magic GPIO while
    watching the other GPIO to convince the silly device to wakeup, or go to
    sleep, as required, and have this appear as a (virtual) GPIO.

Then I can just tell DT that these (virtual) GPIOs are connected together,
and it will all just work.  And it does.

But given the whole "no no, DT is for describing hardware, and you are
describing software" attitude,  it seems that I lost motivation for a while
(that wasn't the only reason, but it didn't help).

I have a patch which converts the OMAP serial driver to use DT to configure
these virtual DTR lines.  I'm very happy to submit that if there is some
chance it might be accepted and will keep the current DTR code in place.

On the other hand, if you can point out to me what I'm missing, and how I can
solve my problem with any virtual GPIOs, I'm all ears.

To make my problem simple and explicit:  I have a device attached to a UART
which has a separate regulator.  The regulator should be powered on if and
only if the /dev/ttyXX interface to the UART is open.  The device is a
bluetooth transceiver.
(I have another device which is a GPS receiver which has similar
but more complicated requirements)

Thanks,
NeilBrown


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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[Felipe Balbi]

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

Hi,

On Thu, Apr 24, 2014 at 10:13:29AM +1000, NeilBrown wrote:
> On Wed, 23 Apr 2014 18:01:21 -0500 Felipe Balbi <balbi@ti.com> wrote:
> 
> > Hi,
> > 
> > On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote:
> > > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote:
> > > 
> > > > On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > > > > nobody passes a DTR_gpio to this driver, so
> > > > > this code is not necessary.
> > > > > 
> > > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > > ---
> > > > 
> > > > Niel,
> > > > this seems to revert the functionality introduced in
> > > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> > > > (OMAP/serial: Add support for driving a GPIO as DTR.)
> > > > 
> > > > would you like to Ack this change?
> > > 
> > > I have a couple of out-of-tree drivers that use this support.
> > > 
> > > I hope to get back to working on that code one day and even get those drivers
> > > upstream.  So I would really prefer this code to remain if it isn't causing
> > > any actual problems.
> > 
> > it causes problem with DT (not really). That suport is only available on
> > legacy platform_data-based boot, it's not available on DT. I hear Tony
> > is pretty close to turning OMAP3 DT-only.
> > 
> > > Of course, I can always re-submit it when I need it again, but that it just
> > > extra work all around.
> > 
> > I wonder how you will pass those attributes through DT considering they
> > are *really* SW configuration. Why can't you use the real DTR pin, btw ?
> > 
> 
> This myth that DT is only about hardware is probably one of the
> reasons that I haven't got these out-of-tree drivers upstream yet.

take that up with DT maintainers

> There is no "real" DTR pin.

heh, my bad... confused with RTS which is supported in this HW.

> When I open /dev/ttyWHATEVER, I need the driver for the device that is
> permanently connected to that serial port to be told that the serial-device
> has been opened so that it can "power on" or "wake up" the device.
> 
> I don't much care how that happens, or how I tell DT that it has to happen.
> I just need it to happen.
> 
> In discrete hardware devices, the DTR line is what you would use.  The RS232
> port would raise (or lower or whatever) DTR when /dev/ttyWHATEVER was open,
> and  the device that was plugged in would detect the level change and do
> stuff.
> 
> But I don't have discrete hardware.  I have a bunch of stuff soldered onto a
> board with ad-hoc connections chosen to make the life of the hardware builder
> easy rather than chosen to make the life of the software developer easy
> (which I think is the correct choice).
> 
> So I need to tell DT "This device is plugged into this UART, and there is no
> DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> I need that regulator of there turned on". or maybe "I need to toggle this
> GPIO  with exactly this pattern, while watching that GPIO to see if it is
> working".
> 
> So I thought:
> 
>  1/ give the UART a "virtual" DTR so it could drive any GPIO
>  2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
>     and responded to state changes on that GPIO by turning on or off the
>     regulator
>  3/ create a dedicated driver which knows how to toggle the magic GPIO while
>     watching the other GPIO to convince the silly device to wakeup, or go to
>     sleep, as required, and have this appear as a (virtual) GPIO.
> 
> Then I can just tell DT that these (virtual) GPIOs are connected together,
> and it will all just work.  And it does.
> 
> But given the whole "no no, DT is for describing hardware, and you are
> describing software" attitude,  it seems that I lost motivation for a while
> (that wasn't the only reason, but it didn't help).
> 
> I have a patch which converts the OMAP serial driver to use DT to configure
> these virtual DTR lines.  I'm very happy to submit that if there is some
> chance it might be accepted and will keep the current DTR code in place.

I have no problem either way, just that unused code doesn't have to be
sitting in the tree and I'm not entirely sure this GPIO should be
handled by omap-serial.c, perhaps something more generic inside
serial-core so other UART drivers can benefit from it.

> On the other hand, if you can point out to me what I'm missing, and how I can
> solve my problem with any virtual GPIOs, I'm all ears.
> 
> To make my problem simple and explicit:  I have a device attached to a UART
> which has a separate regulator.  The regulator should be powered on if and

So you're using DTR to power the GPIO and hoping that the regulator
stabilizes quickly enough so that by the time your open() finishes you
don't have to add nonsensical msleep() calls before writing to the
device. Sounds a bit fragile to me.

> only if the /dev/ttyXX interface to the UART is open.  The device is a
> bluetooth transceiver.

considering this is a BTUART device, why didn't you do this at the ldisc
level ? hci_uart_open() sounds like a good choice from a quick thinking.

-- 
balbi

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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[NeilBrown]

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

On Wed, 23 Apr 2014 20:21:00 -0500 Felipe Balbi <balbi@ti.com> wrote:

> I have no problem either way, just that unused code doesn't have to be
> sitting in the tree and I'm not entirely sure this GPIO should be
> handled by omap-serial.c, perhaps something more generic inside
> serial-core so other UART drivers can benefit from it.

Perhaps.  But there there are more people I need to convince :-)

> 
> > On the other hand, if you can point out to me what I'm missing, and how I can
> > solve my problem with any virtual GPIOs, I'm all ears.
> > 
> > To make my problem simple and explicit:  I have a device attached to a UART
> > which has a separate regulator.  The regulator should be powered on if and
> 
> So you're using DTR to power the GPIO and hoping that the regulator
> stabilizes quickly enough so that by the time your open() finishes you
> don't have to add nonsensical msleep() calls before writing to the
> device. Sounds a bit fragile to me.

The gpio_set call is synchronous, and the gpio-regulator driver could add a
delay (I think).


> 
> > only if the /dev/ttyXX interface to the UART is open.  The device is a
> > bluetooth transceiver.
> 
> considering this is a BTUART device, why didn't you do this at the ldisc
> level ? hci_uart_open() sounds like a good choice from a quick thinking.
> 

I'll have a look into that, thanks.

NeilBrown


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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[Felipe Balbi]

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

Hi,

On Thu, Apr 24, 2014 at 11:41:15AM +1000, NeilBrown wrote:
> On Wed, 23 Apr 2014 20:21:00 -0500 Felipe Balbi <balbi@ti.com> wrote:
> 
> > I have no problem either way, just that unused code doesn't have to be
> > sitting in the tree and I'm not entirely sure this GPIO should be
> > handled by omap-serial.c, perhaps something more generic inside
> > serial-core so other UART drivers can benefit from it.
> 
> Perhaps.  But there there are more people I need to convince :-)

heh, Greg is in Cc, that'd be a good start.

> > > On the other hand, if you can point out to me what I'm missing, and how I can
> > > solve my problem with any virtual GPIOs, I'm all ears.
> > > 
> > > To make my problem simple and explicit:  I have a device attached to a UART
> > > which has a separate regulator.  The regulator should be powered on if and
> > 
> > So you're using DTR to power the GPIO and hoping that the regulator
> > stabilizes quickly enough so that by the time your open() finishes you
> > don't have to add nonsensical msleep() calls before writing to the
> > device. Sounds a bit fragile to me.
> 
> The gpio_set call is synchronous, and the gpio-regulator driver could add a

sure, but it's synchronous towards toggling the GPIO, pulling it high.
It doesn't guarantee that the far-end regulator's output will be fully
changed.

> delay (I think).

yeah, that'd be part of the regulator-gpio with the startup-delay-ns
property (IIRC)

> > > only if the /dev/ttyXX interface to the UART is open.  The device is a
> > > bluetooth transceiver.
> > 
> > considering this is a BTUART device, why didn't you do this at the ldisc
> > level ? hci_uart_open() sounds like a good choice from a quick thinking.
> > 
> 
> I'll have a look into that, thanks.

so, Ack for $subject or not ?

-- 
balbi

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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[NeilBrown]

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

On Wed, 23 Apr 2014 20:43:34 -0500 Felipe Balbi <balbi@ti.com> wrote:

> so, Ack for $subject or not ?
> 

Just at the moment I'm finding it hard to care.
So
 Acked-by: NeilBrown <neilb@suse.de>

Whatever....

NeilBrown

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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[One Thousand Gnomes]

> > But I don't have discrete hardware.  I have a bunch of stuff soldered onto a
> > board with ad-hoc connections chosen to make the life of the hardware builder
> > easy rather than chosen to make the life of the software developer easy
> > (which I think is the correct choice).
> > 
> > So I need to tell DT "This device is plugged into this UART, and there is no
> > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> > I need that regulator of there turned on". or maybe "I need to toggle this
> > GPIO  with exactly this pattern, while watching that GPIO to see if it is
> > working".
> > 
> > So I thought:
> > 
> >  1/ give the UART a "virtual" DTR so it could drive any GPIO
> >  2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
> >     and responded to state changes on that GPIO by turning on or off the
> >     regulator
> >  3/ create a dedicated driver which knows how to toggle the magic GPIO while
> >     watching the other GPIO to convince the silly device to wakeup, or go to
> >     sleep, as required, and have this appear as a (virtual) GPIO.

Unless you are using it as a "real' DTR line then I think this is the
wrong approach. This problem actually is turning up in both PC class and
ARM boxes now all over the place for three reasons I am seeing.

1. We are getting a lot of hardware moving to serial attached
bluetooth/gps/etc because of the power win. In addition ACPI can describe
power relationships and what is on the other end of a UART embedded in
the device

2. We've got cheap hardware with modem lines being "retrofitted" via gpio

3. There are a subset of devices that have extra control lines beyond the
usual serial port ones. These range from additional control lines for
things like smartcard programmers to port muxing.

> I have no problem either way, just that unused code doesn't have to be
> sitting in the tree and I'm not entirely sure this GPIO should be
> handled by omap-serial.c, perhaps something more generic inside
> serial-core so other UART drivers can benefit from it.

serial-core provides power hooks. If the goal is that this comes on when
you power up the uart and goes away on the last close then the hooks are
there already. If its ldisc specific then the tty layer also calls the
device on ldisc changes precisely so it can make hardware specific
twiddles in such cases.

A set of gpios on the tty_port object would not go amiss and would also
address the PC case.

> considering this is a BTUART device, why didn't you do this at the ldisc
> level ? hci_uart_open() sounds like a good choice from a quick thinking.

ldiscs are hardware independent. Nothing about hardware belongs in an
ldisc. Any ldisc should within reason work on any port.

What I propsed when it came up ages ago was to expose some gpio settings
in the tty, to provide ways for them to be set by default and also ioctls
to configure them. I still think this (but moved into the tty_port as its
a persistent hardware property) is a good idea now that we are starting
to see more use cases than weird dongles and muxes on pre-production
reference boards.

With my tty and serial hat on I think a power gpio is a no-brainer even
without doing the other work and therefore it should probably be described
generically for a serial port in the DT as well as in the ACPI data. If
you should also be able to give it a regulator to use as well that also
seems to make complete sense.

Alan

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[NeilBrown]

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

On Thu, 24 Apr 2014 15:19:14 +0100 One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:

> > > But I don't have discrete hardware.  I have a bunch of stuff soldered onto a
> > > board with ad-hoc connections chosen to make the life of the hardware builder
> > > easy rather than chosen to make the life of the software developer easy
> > > (which I think is the correct choice).
> > > 
> > > So I need to tell DT "This device is plugged into this UART, and there is no
> > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> > > I need that regulator of there turned on". or maybe "I need to toggle this
> > > GPIO  with exactly this pattern, while watching that GPIO to see if it is
> > > working".
> > > 
> > > So I thought:
> > > 
> > >  1/ give the UART a "virtual" DTR so it could drive any GPIO
> > >  2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
> > >     and responded to state changes on that GPIO by turning on or off the
> > >     regulator
> > >  3/ create a dedicated driver which knows how to toggle the magic GPIO while
> > >     watching the other GPIO to convince the silly device to wakeup, or go to
> > >     sleep, as required, and have this appear as a (virtual) GPIO.
> 
> Unless you are using it as a "real' DTR line then I think this is the
> wrong approach. This problem actually is turning up in both PC class and
> ARM boxes now all over the place for three reasons I am seeing.
> 
> 1. We are getting a lot of hardware moving to serial attached
> bluetooth/gps/etc because of the power win. In addition ACPI can describe
> power relationships and what is on the other end of a UART embedded in
> the device
> 
> 2. We've got cheap hardware with modem lines being "retrofitted" via gpio
> 
> 3. There are a subset of devices that have extra control lines beyond the
> usual serial port ones. These range from additional control lines for
> things like smartcard programmers to port muxing.
> 
> > I have no problem either way, just that unused code doesn't have to be
> > sitting in the tree and I'm not entirely sure this GPIO should be
> > handled by omap-serial.c, perhaps something more generic inside
> > serial-core so other UART drivers can benefit from it.
> 
> serial-core provides power hooks. If the goal is that this comes on when
> you power up the uart and goes away on the last close then the hooks are
> there already.

Could you be a bit more explicit, or point to an example user of these hooks?

I had a quick look and I guess that uart_change_pm() is the most likely
candidate for what you are referring to.
I can see how that interfaces to the specific piece of uart hardware, such as
omap-serial.c
But I cannot see how you would plumb that though to the device that was
plugged in to the serial port.  I guess if that device could be registered as
a child of the omap_serial device, then power management inheritance might
come in to play, but I cannot see any way to tell a serial port that it has
some arbitrary child device.

So maybe I'm missing something.

>                  If its ldisc specific then the tty layer also calls the
> device on ldisc changes precisely so it can make hardware specific
> twiddles in such cases.
> 
> A set of gpios on the tty_port object would not go amiss and would also
> address the PC case.
> 
> > considering this is a BTUART device, why didn't you do this at the ldisc
> > level ? hci_uart_open() sounds like a good choice from a quick thinking.
> 
> ldiscs are hardware independent. Nothing about hardware belongs in an
> ldisc. Any ldisc should within reason work on any port.
> 
> What I propsed when it came up ages ago was to expose some gpio settings
> in the tty, to provide ways for them to be set by default and also ioctls
> to configure them. I still think this (but moved into the tty_port as its
> a persistent hardware property) is a good idea now that we are starting
> to see more use cases than weird dongles and muxes on pre-production
> reference boards.
> 
> With my tty and serial hat on I think a power gpio is a no-brainer even
> without doing the other work and therefore it should probably be described
> generically for a serial port in the DT as well as in the ACPI data. If
> you should also be able to give it a regulator to use as well that also
> seems to make complete sense.

In one case the "power on" sequence is substantially more complex that just a
gpio or regulator.  I would need to write a device driver for the (GPS) chip
which could receive a poweron/poweroff signal from the uart and do the
required magic.

Having serial-core know about gpios and regulators and random other stuff
seems wrong.
I would really like to see the "runtime interpreted power sequences" become a
real thing.  Then serial-core could activate a "RIPS", and that would have
the flexibility to do whatever is needed without adding complexity to
serial-core.
Using a virtual GPIO was my poor-mans RIPS.  Using gpiolib, and driver can
pretend to be a gpio so it is a simple "pipe" to send a power-on/power-off
signal over.

So ... with your "serial" hat on, if I were to write/test a patch to allow
any serial port to have a "power-gpio" described in DT and the gpio would be
driven in uart_change_pm(), would you consider accepting that patch, or did I
misunderstand?

Thanks,
NeilBrown

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

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[Yegor Yefremov]

On Fri, Apr 25, 2014 at 11:34 AM, NeilBrown <neilb@suse.de> wrote:
> On Thu, 24 Apr 2014 15:19:14 +0100 One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
>
>> > > But I don't have discrete hardware.  I have a bunch of stuff soldered onto a
>> > > board with ad-hoc connections chosen to make the life of the hardware builder
>> > > easy rather than chosen to make the life of the software developer easy
>> > > (which I think is the correct choice).
>> > >
>> > > So I need to tell DT "This device is plugged into this UART, and there is no
>> > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
>> > > I need that regulator of there turned on". or maybe "I need to toggle this
>> > > GPIO  with exactly this pattern, while watching that GPIO to see if it is
>> > > working".
>> > >
>> > > So I thought:
>> > >
>> > >  1/ give the UART a "virtual" DTR so it could drive any GPIO
>> > >  2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
>> > >     and responded to state changes on that GPIO by turning on or off the
>> > >     regulator
>> > >  3/ create a dedicated driver which knows how to toggle the magic GPIO while
>> > >     watching the other GPIO to convince the silly device to wakeup, or go to
>> > >     sleep, as required, and have this appear as a (virtual) GPIO.
>>
>> Unless you are using it as a "real' DTR line then I think this is the
>> wrong approach. This problem actually is turning up in both PC class and
>> ARM boxes now all over the place for three reasons I am seeing.
>>
>> 1. We are getting a lot of hardware moving to serial attached
>> bluetooth/gps/etc because of the power win. In addition ACPI can describe
>> power relationships and what is on the other end of a UART embedded in
>> the device
>>
>> 2. We've got cheap hardware with modem lines being "retrofitted" via gpio
>>
>> 3. There are a subset of devices that have extra control lines beyond the
>> usual serial port ones. These range from additional control lines for
>> things like smartcard programmers to port muxing.
>>
>> > I have no problem either way, just that unused code doesn't have to be
>> > sitting in the tree and I'm not entirely sure this GPIO should be
>> > handled by omap-serial.c, perhaps something more generic inside
>> > serial-core so other UART drivers can benefit from it.
>>
>> serial-core provides power hooks. If the goal is that this comes on when
>> you power up the uart and goes away on the last close then the hooks are
>> there already.
>
> Could you be a bit more explicit, or point to an example user of these hooks?
>
> I had a quick look and I guess that uart_change_pm() is the most likely
> candidate for what you are referring to.
> I can see how that interfaces to the specific piece of uart hardware, such as
> omap-serial.c
> But I cannot see how you would plumb that though to the device that was
> plugged in to the serial port.  I guess if that device could be registered as
> a child of the omap_serial device, then power management inheritance might
> come in to play, but I cannot see any way to tell a serial port that it has
> some arbitrary child device.
>
> So maybe I'm missing something.
>
>>                  If its ldisc specific then the tty layer also calls the
>> device on ldisc changes precisely so it can make hardware specific
>> twiddles in such cases.
>>
>> A set of gpios on the tty_port object would not go amiss and would also
>> address the PC case.
>>
>> > considering this is a BTUART device, why didn't you do this at the ldisc
>> > level ? hci_uart_open() sounds like a good choice from a quick thinking.
>>
>> ldiscs are hardware independent. Nothing about hardware belongs in an
>> ldisc. Any ldisc should within reason work on any port.
>>
>> What I propsed when it came up ages ago was to expose some gpio settings
>> in the tty, to provide ways for them to be set by default and also ioctls
>> to configure them. I still think this (but moved into the tty_port as its
>> a persistent hardware property) is a good idea now that we are starting
>> to see more use cases than weird dongles and muxes on pre-production
>> reference boards.
>>
>> With my tty and serial hat on I think a power gpio is a no-brainer even
>> without doing the other work and therefore it should probably be described
>> generically for a serial port in the DT as well as in the ACPI data. If
>> you should also be able to give it a regulator to use as well that also
>> seems to make complete sense.
>
> In one case the "power on" sequence is substantially more complex that just a
> gpio or regulator.  I would need to write a device driver for the (GPS) chip
> which could receive a poweron/poweroff signal from the uart and do the
> required magic.
>
> Having serial-core know about gpios and regulators and random other stuff
> seems wrong.
> I would really like to see the "runtime interpreted power sequences" become a
> real thing.  Then serial-core could activate a "RIPS", and that would have
> the flexibility to do whatever is needed without adding complexity to
> serial-core.
> Using a virtual GPIO was my poor-mans RIPS.  Using gpiolib, and driver can
> pretend to be a gpio so it is a simple "pipe" to send a power-on/power-off
> signal over.
>
> So ... with your "serial" hat on, if I were to write/test a patch to allow
> any serial port to have a "power-gpio" described in DT and the gpio would be
> driven in uart_change_pm(), would you consider accepting that patch, or did I
> misunderstand?

As soon as this patch
(http://www.spinics.net/lists/arm-kernel/msg325197.html) will be
applied, we don't really need this DTR GPIO any more.

DTR_active is the only stuff, that is missing.

Yegor

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[One Thousand Gnomes]

> As soon as this patch
> (http://www.spinics.net/lists/arm-kernel/msg325197.html) will be
> applied, we don't really need this DTR GPIO any more.

For the omap specific case, not for the general case of sorting out power
hierarchies.

Alan

Re: [PATCH 10/13] tty: serial: omap: remove some dead code

[One Thousand Gnomes]

> I had a quick look and I guess that uart_change_pm() is the most likely
> candidate for what you are referring to.
> I can see how that interfaces to the specific piece of uart hardware, such as
> omap-serial.c
> But I cannot see how you would plumb that though to the device that was
> plugged in to the serial port.  I guess if that device could be registered as
> a child of the omap_serial device, then power management inheritance might
> come in to play, but I cannot see any way to tell a serial port that it has
> some arbitrary child device.

If your device is powered by something like a regulator then you can
drive the regulator from the uart_pm helpers.

> In one case the "power on" sequence is substantially more complex that just a
> gpio or regulator.  I would need to write a device driver for the (GPS) chip
> which could receive a poweron/poweroff signal from the uart and do the
> required magic.

In which case giving the tty a child device (or devices) would probably
be more sensible yes. No problem with that either.

> I would really like to see the "runtime interpreted power sequences" become a
> real thing.  Then serial-core could activate a "RIPS", and that would have
> the flexibility to do whatever is needed without adding complexity to
> serial-core.

Something like ACPI has you mean ? When we put the device into D0 the
ACPI methods can do stuff.

> So ... with your "serial" hat on, if I were to write/test a patch to allow
> any serial port to have a "power-gpio" described in DT and the gpio would be
> driven in uart_change_pm(), would you consider accepting that patch, or did I
> misunderstand?

We are going to need it anyway for other devices fairly soon. It's common
for new PC class hardware to have gpio management for the bluetooth,
gps and and sometimes sensor devices attached to the tty. That's true
irrespective of whether you happen to choose to use it for virtual gpio
hacks.

Alan

[PATCH 11/13] tty: serial: omap: remove unneeded singlethread workqueue

[Felipe Balbi]

it wasn't used by anything, just remove it.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 6654682..ab22dab 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -180,8 +180,6 @@ static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 /* Forward declaration of functions */
 static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1);
 
-static struct workqueue_struct *serial_omap_uart_wq;
-
 static inline unsigned int serial_in(struct uart_omap_port *up, int offset)
 {
 	offset <<= up->port.regshift;
@@ -1701,7 +1699,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 	up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
 	pm_qos_add_request(&up->pm_qos_request,
 		PM_QOS_CPU_DMA_LATENCY, up->latency);
-	serial_omap_uart_wq = create_singlethread_workqueue(up->name);
 	INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
 
 	platform_set_drvdata(pdev, up);
-- 
1.9.2.459.g68773ac

[PATCH 12/13] tty: serial: omap: fix Sparse warnings

[Felipe Balbi]

Fix the following Sparse warnings:

drivers/tty/serial/omap-serial.c:1418:49: warning: incorrect \
	type in argument 2 (different address spaces)
drivers/tty/serial/omap-serial.c:1418:49:    expected void const \
	[noderef] <asn:1>*from
drivers/tty/serial/omap-serial.c:1418:49:    got struct serial_rs485 \
	*<noident>
drivers/tty/serial/omap-serial.c:1426:35: warning: incorrect \
	type in argument 1 (different address spaces)
drivers/tty/serial/omap-serial.c:1426:35:    expected void [noderef] \
	<asn:1>*to
drivers/tty/serial/omap-serial.c:1426:35:    got struct serial_rs485 \
	*<noident>

Reported-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/omap-serial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index ab22dab..d017cec 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1398,7 +1398,7 @@ serial_omap_ioctl(struct uart_port *port, unsigned int cmd, unsigned long arg)
 
 	switch (cmd) {
 	case TIOCSRS485:
-		if (copy_from_user(&rs485conf, (struct serial_rs485 *) arg,
+		if (copy_from_user(&rs485conf, (void __user *) arg,
 					sizeof(rs485conf)))
 			return -EFAULT;
 
@@ -1406,7 +1406,7 @@ serial_omap_ioctl(struct uart_port *port, unsigned int cmd, unsigned long arg)
 		break;
 
 	case TIOCGRS485:
-		if (copy_to_user((struct serial_rs485 *) arg,
+		if (copy_to_user((void __user *) arg,
 					&(to_uart_omap_port(port)->rs485),
 					sizeof(rs485conf)))
 			return -EFAULT;
-- 
1.9.2.459.g68773ac

[PATCH 13/13] serial: 8250: add OMAP glue

[Felipe Balbi]

NOT COMPLETE

NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/tty/serial/8250/8250_omap.c | 233 ++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |   7 ++
 drivers/tty/serial/8250/Makefile    |   1 +
 3 files changed, 241 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_omap.c

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
new file mode 100644
index 0000000..e8ae479
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -0,0 +1,233 @@
+/*
+ * 8250-omap.c - OMAP adaptation for 8250 driver
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Felipe Balbi <balbi@ti.com>
+ *
+ * Based on omap-serial.c:
+ *
+ * Copyright (C) 2010 Texas Instruments
+ *
+ * Authors:
+ *	Govindraj R	<govindraj.raja@ti.com>
+ *	Thara Gopinath	<thara@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Note: This driver is made separate from 8250 driver as we cannot
+ * over load 8250 driver with omap platform specific configuration for
+ * features like DMA, it makes easier to implement features like DMA and
+ * hardware flow control and software flow control configuration with
+ * this driver as required for the omap-platform.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/serial_8250.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_data/serial-omap.h>
+
+#include <dt-bindings/gpio/gpio.h>
+
+#define OMAP_MAX_HSUART_PORTS	6
+
+#define UART_BUILD_REVISION(x, y)	(((x) << 8) | (y))
+
+#define OMAP_UART_REV_42 0x0402
+#define OMAP_UART_REV_46 0x0406
+#define OMAP_UART_REV_52 0x0502
+#define OMAP_UART_REV_63 0x0603
+
+#define OMAP_UART_TX_WAKEUP_EN		BIT(7)
+
+/* Feature flags */
+#define OMAP_UART_WER_HAS_TX_WAKEUP	BIT(0)
+
+#define UART_ERRATA_i202_MDR1_ACCESS	BIT(0)
+#define UART_ERRATA_i291_DMA_FORCEIDLE	BIT(1)
+
+#define DEFAULT_CLK_SPEED 48000000 /* 48Mhz*/
+
+/* SCR register bitmasks */
+#define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK		(1 << 7)
+#define OMAP_UART_SCR_TX_TRIG_GRANU1_MASK		(1 << 6)
+#define OMAP_UART_SCR_TX_EMPTY			(1 << 3)
+
+/* FCR register bitmasks */
+#define OMAP_UART_FCR_RX_FIFO_TRIG_MASK			(0x3 << 6)
+#define OMAP_UART_FCR_TX_FIFO_TRIG_MASK			(0x3 << 4)
+
+/* MVR register bitmasks */
+#define OMAP_UART_MVR_SCHEME_SHIFT	30
+
+#define OMAP_UART_LEGACY_MVR_MAJ_MASK	0xf0
+#define OMAP_UART_LEGACY_MVR_MAJ_SHIFT	4
+#define OMAP_UART_LEGACY_MVR_MIN_MASK	0x0f
+
+#define OMAP_UART_MVR_MAJ_MASK		0x700
+#define OMAP_UART_MVR_MAJ_SHIFT		8
+#define OMAP_UART_MVR_MIN_MASK		0x3f
+
+#define OMAP_UART_DMA_CH_FREE	-1
+
+#define MSR_SAVE_FLAGS		UART_MSR_ANY_DELTA
+#define OMAP_MODE13X_SPEED	230400
+
+/* WER = 0x7F
+ * Enable module level wakeup in WER reg
+ */
+#define OMAP_UART_WER_MOD_WKUP	0X7F
+
+/* Enable XON/XOFF flow control on output */
+#define OMAP_UART_SW_TX		0x08
+
+/* Enable XON/XOFF flow control on input */
+#define OMAP_UART_SW_RX		0x02
+
+#define OMAP_UART_SW_CLR	0xF0
+
+#define OMAP_UART_TCR_TRIG	0x0F
+
+struct uart_omap_port {
+	struct uart_8250_port	uart;
+	struct device		*dev;
+	int			wakeirq;
+
+	unsigned char		ier;
+	unsigned char		lcr;
+	unsigned char		mcr;
+	unsigned char		fcr;
+	unsigned char		efr;
+	unsigned char		dll;
+	unsigned char		dlh;
+	unsigned char		mdr1;
+	unsigned char		scr;
+	unsigned char		wer;
+
+	int			line;
+
+	/*
+	 * Some bits in registers are cleared on a read, so they must
+	 * be saved whenever the register is read but the bits will not
+	 * be immediately processed.
+	 */
+	unsigned int		lsr_break_flag;
+	unsigned char		msr_saved_flags;
+	char			name[20];
+	unsigned long		port_activity;
+	int			context_loss_cnt;
+	u32			errata;
+	u8			wakeups_enabled;
+	u32			features;
+};
+
+#define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port)))
+
+static int serial_omap_probe(struct platform_device *pdev)
+{
+	struct uart_omap_port *up;
+	struct resource *mem;
+	void __iomem *base;
+	int uartirq = 0;
+	int ret;
+
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	uartirq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (!uartirq)
+		return -EPROBE_DEFER;
+
+	up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
+	if (!up)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	up->dev = &pdev->dev;
+	up->uart.port.mapbase = mem->start;
+	up->uart.port.membase = base;
+	up->uart.port.irq = uartirq;
+	up->uart.port.dev = &pdev->dev;
+	up->uart.port.type = PORT_16750;
+	up->uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT;
+	up->uart.port.iotype = UPIO_MEM;
+
+	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				&up->uart.port.uartclk)) {
+		up->uart.port.uartclk = DEFAULT_CLK_SPEED;
+		dev_warn(&pdev->dev,
+			 "No clock speed specified: using default: %d\n",
+			 DEFAULT_CLK_SPEED);
+	}
+
+	platform_set_drvdata(pdev, up);
+
+	device_init_wakeup(up->dev, true);
+	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
+	ret = serial8250_register_8250_port(&up->uart);
+	if (ret < 0)
+		goto err_add_port;
+
+	up->line = ret;
+
+	return 0;
+
+err_add_port:
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
+				pdev->id, __func__, ret);
+	return ret;
+}
+
+static int serial_omap_remove(struct platform_device *dev)
+{
+	struct uart_omap_port *up = platform_get_drvdata(dev);
+
+	serial8250_unregister_port(up->line);
+
+	pm_runtime_put_sync(up->dev);
+	pm_runtime_disable(up->dev);
+
+	return 0;
+}
+
+static const struct of_device_id omap_serial_of_match[] = {
+	{ .compatible = "ti,am335x-uart" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, omap_serial_of_match);
+
+static struct platform_driver serial_omap_driver = {
+	.probe          = serial_omap_probe,
+	.remove         = serial_omap_remove,
+	.driver		= {
+		.name	= DRIVER_NAME,
+		.of_match_table = of_match_ptr(omap_serial_of_match),
+	},
+};
+
+module_platform_driver(serial_omap_driver);
+
+MODULE_DESCRIPTION("OMAP High Speed UART driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Texas Instruments Incorporated");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 2332991..ad092d5 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -287,6 +287,13 @@ config SERIAL_8250_DW
 	  Selecting this option will enable handling of the extra features
 	  present in the Synopsys DesignWare APB UART.
 
+config SERIAL_8250_OMAP
+	tristate "Support for OMAP 8250 quirks"
+	depends on SERIAL_8250
+	help
+	  Selecting this option will enable handling of the extra features
+	  present in the TI OMAP UART.
+
 config SERIAL_8250_EM
 	tristate "Support for Emma Mobile integrated serial port"
 	depends on SERIAL_8250 && ARM && HAVE_CLK
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 36d68d0..0119a11 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -19,4 +19,5 @@ obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
+obj-$(CONFIG_SERIAL_8250_OMAP)		+= 8250_omap.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
-- 
1.9.2.459.g68773ac