[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[Govindraj.R]

Modify the omap_uart_can_sleep function to check uart is active
or not to be used by pm code to enter low power states.

Removing this check can cause console response little sluggish.
However no characters will be lost until uart clocks are gated
and woken up using rx-pad. UART interface clocks can be auto gated
this can make response on uart slower. This behaviour was observed
only on some of OMAP3 boards(beagleboard xm rev c).

Reported-by: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 6725caf..ccf3550 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -156,23 +156,20 @@ static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
 
 int omap_uart_can_sleep(void)
 {
-	struct omap_uart_state *uart;
-	int can_sleep = 1;
-
-	list_for_each_entry(uart, &uart_list, node) {
-		if (!uart->clocked)
-			continue;
+	struct omap_hwmod *oh;
+	u8 i, ret = true;
 
-		if (!uart->can_sleep) {
-			can_sleep = 0;
+	for (i = 0; i < num_uarts; i++) {
+		oh = omap_uart_hwmod_lookup(i);
+		if (!oh)
 			continue;
-		}
 
-		/* This UART can now safely sleep. */
-		omap_uart_allow_sleep(uart);
+		if (oh->od && oh->od->pdev &&
+				!pm_runtime_suspended(&oh->od->pdev->dev))
+			return false;
 	}
 
-	return can_sleep;
+	return ret;
 }
 
 #ifdef CONFIG_OMAP_MUX
-- 
1.7.4.1

[PATCH v6 11/16] OMAP2+: UART: Move errata handling from serial.c to omap-serial

[Govindraj.R]

Move the errata handling mechanism from serial.c to omap-serial file
and utilise the same func in driver file.

Errata i202, i291 are moved to be handled with omap-serial
Moving the errata macro from serial.c file to driver header file
as from on errata will be handled in driver file itself.

Corrected errata id from chapter reference 2.15 to errata id i291.

Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c                  |   78 +++++--------------------
 arch/arm/plat-omap/include/plat/omap-serial.h |    5 ++
 drivers/tty/serial/omap-serial.c              |   63 ++++++++++++++++++--
 3 files changed, 77 insertions(+), 69 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index ccf3550..0731575 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -42,8 +42,6 @@
 #include "control.h"
 #include "mux.h"
 
-#define UART_ERRATA_i202_MDR1_ACCESS	(0x1 << 1)
-
 /*
  * NOTE: By default the serial timeout is disabled as it causes lost characters
  * over the serial ports. This means that the UART clocks will stay on until
@@ -54,11 +52,6 @@
 
 #define MAX_UART_HWMOD_NAME_LEN		16
 
-struct omap_uart_state {
-	int num;
-	u32 dma_enabled;
-};
-
 static u8 num_uarts;
 
 static int uart_idle_hwmod(struct omap_device *od)
@@ -97,63 +90,6 @@ struct omap_hwmod *omap_uart_hwmod_lookup(int num)
 
 #if defined(CONFIG_PM)
 
-/*
- * Work Around for Errata i202 (3430 - 1.12, 3630 - 1.6)
- * The access to uart register after MDR1 Access
- * causes UART to corrupt data.
- *
- * Need a delay =
- * 5 L4 clock cycles + 5 UART functional clock cycle (@48MHz = ~0.2uS)
- * give 10 times as much
- */
-static void omap_uart_mdr1_errataset(struct omap_uart_state *uart, u8 mdr1_val,
-		u8 fcr_val)
-{
-	u8 timeout = 255;
-
-	serial_write_reg(uart, UART_OMAP_MDR1, mdr1_val);
-	udelay(2);
-	serial_write_reg(uart, UART_FCR, fcr_val | UART_FCR_CLEAR_XMIT |
-			UART_FCR_CLEAR_RCVR);
-	/*
-	 * Wait for FIFO to empty: when empty, RX_FIFO_E bit is 0 and
-	 * TX_FIFO_E bit is 1.
-	 */
-	while (UART_LSR_THRE != (serial_read_reg(uart, UART_LSR) &
-				(UART_LSR_THRE | UART_LSR_DR))) {
-		timeout--;
-		if (!timeout) {
-			/* Should *never* happen. we warn and carry on */
-			dev_crit(&uart->pdev->dev, "Errata i202: timedout %x\n",
-			serial_read_reg(uart, UART_LSR));
-			break;
-		}
-		udelay(1);
-	}
-}
-#endif /* CONFIG_PM && CONFIG_ARCH_OMAP3 */
-
-static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
-					       int enable)
-{
-	u8 idlemode;
-
-	if (enable) {
-		/**
-		 * Errata 2.15: [UART]:Cannot Acknowledge Idle Requests
-		 * in Smartidle Mode When Configured for DMA Operations.
-		 */
-		if (uart->dma_enabled)
-			idlemode = HWMOD_IDLEMODE_FORCE;
-		else
-			idlemode = HWMOD_IDLEMODE_SMART;
-	} else {
-		idlemode = HWMOD_IDLEMODE_NO;
-	}
-
-	omap_hwmod_set_slave_idlemode(uart->oh, idlemode);
-}
-
 int omap_uart_can_sleep(void)
 {
 	struct omap_hwmod *oh;
@@ -313,7 +249,20 @@ static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
 		omap_hwmod_disable_wakeup(od->hwmods[0]);
 }
 
+/*
+ * Errata i291: [UART]:Cannot Acknowledge Idle Requests
+ * in Smartidle Mode When Configured for DMA Operations.
+ * WA: configure uart in force idle mode.
+ */
+static void omap_uart_set_forceidle(struct platform_device *pdev)
+{
+	struct omap_device *od = to_omap_device(pdev);
+
+	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_FORCE);
+}
+
 #else
+static void omap_uart_set_forceidle(struct platform_device *pdev) {}
 static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
 {}
 #endif /* CONFIG_PM */
@@ -388,6 +337,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
 	pdata->flags = UPF_BOOT_AUTOCONF;
 	pdata->enable_wakeup = omap_uart_enable_wakeup;
 	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
+	pdata->set_force_idle = omap_uart_set_forceidle;
 
 	/* Enable the MDR1 errata for OMAP3 */
 	if (cpu_is_omap34xx() && !cpu_is_ti816x())
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 8ef81ce..12f5262 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -56,6 +56,8 @@
 
 #define MSR_SAVE_FLAGS		UART_MSR_ANY_DELTA
 
+#define UART_ERRATA_i202_MDR1_ACCESS	(0x1 << 1)
+
 struct omap_uart_port_info {
 	bool			dma_enabled;	/* To specify DMA Mode */
 	unsigned int		uartclk;	/* UART clock rate */
@@ -65,6 +67,7 @@ struct omap_uart_port_info {
 
 	void (*enable_wakeup)(struct platform_device *, bool);
 	u32 (*get_context_loss_count)(struct device *);
+	void (*set_force_idle)(struct platform_device *);
 };
 
 struct uart_omap_dma {
@@ -118,6 +121,8 @@ struct uart_omap_port {
 	unsigned long		port_activity;
 	u32			context_loss_cnt;
 	u8			wakeups_enabled;
+	u32			errata;
+
 };
 
 #endif /* __OMAP_SERIAL_H__ */
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 43c33da..817443e 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -51,6 +51,7 @@ static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 static void uart_tx_dma_callback(int lch, u16 ch_status, void *data);
 static void serial_omap_rx_timeout(unsigned long uart_no);
 static int serial_omap_start_rxdma(struct uart_omap_port *up);
+static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1);
 
 static inline unsigned int serial_in(struct uart_omap_port *up, int offset)
 {
@@ -805,7 +806,11 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* Protocol, Baud Rate, and Interrupt Settings */
 
-	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+	if (up->errata & UART_ERRATA_i202_MDR1_ACCESS)
+		serial_omap_mdr1_errataset(up, up->mdr1);
+	else
+		serial_out(up, UART_OMAP_MDR1, up->mdr1);
+
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
 	up->efr = serial_in(up, UART_EFR);
@@ -830,7 +835,10 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	else
 		up->mdr1 = UART_OMAP_MDR1_16X_MODE;
 
-	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+	if (up->errata & UART_ERRATA_i202_MDR1_ACCESS)
+		serial_omap_mdr1_errataset(up, up->mdr1);
+	else
+		serial_out(up, UART_OMAP_MDR1, up->mdr1);
 
 	/* Hardware Flow Control Configuration */
 
@@ -1350,6 +1358,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 	up->port.flags = omap_up_info->flags;
 	up->port.uartclk = omap_up_info->uartclk;
 	up->uart_dma.uart_base = mem->start;
+	up->errata = omap_up_info->errata;
 
 	if (omap_up_info->dma_enabled) {
 		up->uart_dma.uart_dma_tx = dma_tx->start;
@@ -1405,9 +1414,47 @@ static int serial_omap_remove(struct platform_device *dev)
 	return 0;
 }
 
+/*
+ * Work Around for Errata i202 (3430 - 1.12, 3630 - 1.6)
+ * The access to uart register after MDR1 Access
+ * causes UART to corrupt data.
+ *
+ * Need a delay =
+ * 5 L4 clock cycles + 5 UART functional clock cycle (@48MHz = ~0.2uS)
+ * give 10 times as much
+ */
+static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1)
+{
+	u8 timeout = 255;
+
+	serial_out(up, UART_OMAP_MDR1, mdr1);
+	udelay(2);
+	serial_out(up, UART_FCR, up->fcr | UART_FCR_CLEAR_XMIT |
+			UART_FCR_CLEAR_RCVR);
+	/*
+	 * Wait for FIFO to empty: when empty, RX_FIFO_E bit is 0 and
+	 * TX_FIFO_E bit is 1.
+	 */
+	while (UART_LSR_THRE != (serial_in(up, UART_LSR) &
+				(UART_LSR_THRE | UART_LSR_DR))) {
+		timeout--;
+		if (!timeout) {
+			/* Should *never* happen. we warn and carry on */
+			dev_crit(&up->pdev->dev, "Errata i202: timedout %x\n",
+						serial_in(up, UART_LSR));
+			break;
+		}
+		udelay(1);
+	}
+}
+
 static void serial_omap_restore_context(struct uart_omap_port *up)
 {
-	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+	if (up->errata & UART_ERRATA_i202_MDR1_ACCESS)
+		serial_omap_mdr1_errataset(up, UART_OMAP_MDR1_DISABLE);
+	else
+		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
+
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
 	serial_out(up, UART_EFR, UART_EFR_ECB);
 	serial_out(up, UART_LCR, 0x0); /* Operational mode */
@@ -1423,8 +1470,10 @@ static void serial_omap_restore_context(struct uart_omap_port *up)
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
 	serial_out(up, UART_EFR, up->efr);
 	serial_out(up, UART_LCR, up->lcr);
-	/* UART 16x mode */
-	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+	if (up->errata & UART_ERRATA_i202_MDR1_ACCESS)
+		serial_omap_mdr1_errataset(up, up->mdr1);
+	else
+		serial_out(up, UART_OMAP_MDR1, up->mdr1);
 }
 
 static int serial_omap_runtime_suspend(struct device *dev)
@@ -1453,6 +1502,10 @@ static int serial_omap_runtime_suspend(struct device *dev)
 		}
 	}
 
+	/* Errata i291 */
+	if (up->use_dma && pdata->set_force_idle)
+		pdata->set_force_idle(up->pdev);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH v6 12/16] OMAP2+: UART: Allow UART parameters to be configured from board file.

[Govindraj.R]

From: Deepak K <deepak.k@ti.com>

The following UART parameters are defined within the UART driver:

1). Whether the UART uses DMA (dma_enabled), by default set to 0
2). The size of dma buffer (set to 4096 bytes)
3). The time after which the dma should stop if no more data is received.
4). The auto suspend delay that will be passed for pm_runtime_autosuspend
    where uart will be disabled after timeout

Different UARTs may be used for different purpose such as the console,
for interfacing bluetooth chip, for interfacing to a modem chip, etc.
Therefore, it is necessary to be able to customize the above settings
for a given board on a per UART basis.

This change allows these parameters to be configured from the board file
and allows the parameters to be configured for each UART independently.

If a board does not define its own custom parameters for the UARTs, then
use the default parameters in the structure "omap_serial_default_info".
The default parameters are defined to be the same as the current settings
in the UART driver to avoid breaking the UART for any cuurnelty supported
boards. By default, make all boards use the default UART parameters.

Signed-off-by: Deepak K <deepak.k@ti.com>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
This patch is derived and reworked from a custom 2.6.35 kernel
Available here:
http://git.omapzoom.org/?p=kernel/omap.git;
a=commitdiff;h=584ef316542f77312be7ba96a0f3013c8f64852b;
hp=7233a76cb362c0fc603f773274159adff91d3513

 arch/arm/mach-omap2/board-n8x0.c              |    6 +-
 arch/arm/mach-omap2/serial.c                  |   56 ++++++++++++++++++++----
 arch/arm/plat-omap/include/plat/omap-serial.h |    7 ++-
 arch/arm/plat-omap/include/plat/serial.h      |    5 ++-
 drivers/tty/serial/omap-serial.c              |    8 +--
 5 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c
index e11f0c5..3408726 100644
--- a/arch/arm/mach-omap2/board-n8x0.c
+++ b/arch/arm/mach-omap2/board-n8x0.c
@@ -656,15 +656,15 @@ static inline void board_serial_init(void)
 	bdata.pads_cnt = 0;
 
 	bdata.id = 0;
-	omap_serial_init_port(&bdata);
+	omap_serial_init_port(&bdata, NULL);
 
 	bdata.id = 1;
-	omap_serial_init_port(&bdata);
+	omap_serial_init_port(&bdata, NULL);
 
 	bdata.id = 2;
 	bdata.pads = serial2_pads;
 	bdata.pads_cnt = ARRAY_SIZE(serial2_pads);
-	omap_serial_init_port(&bdata);
+	omap_serial_init_port(&bdata, NULL);
 }
 
 #else
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 0731575..78f7051 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -43,17 +43,29 @@
 #include "mux.h"
 
 /*
- * NOTE: By default the serial timeout is disabled as it causes lost characters
- * over the serial ports. This means that the UART clocks will stay on until
- * disabled via sysfs. This also causes that any deeper omap sleep states are
- * blocked. 
+ * NOTE: By default the serial auto_suspend timeout is disabled as it causes
+ * lost characters over the serial ports. This means that the UART clocks will
+ * stay on until power/autosuspend_delay is set for the uart from sysfs.
+ * This also causes that any deeper omap sleep states are blocked.
  */
-#define DEFAULT_TIMEOUT 0
+#define DEFAULT_AUTOSUSPEND_DELAY	-1
 
 #define MAX_UART_HWMOD_NAME_LEN		16
 
 static u8 num_uarts;
 
+#define DEFAULT_RXDMA_TIMEOUT		1	/* RX DMA polling rate (us) */
+#define DEFAULT_RXDMA_BUFSIZE		4096	/* RX DMA buffer size */
+
+static struct omap_uart_port_info omap_serial_default_info[] = {
+	{
+		.dma_enabled	= 0,
+		.dma_rx_buf_size = DEFAULT_RXDMA_BUFSIZE,
+		.dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
+		.autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
+	},
+};
+
 static int uart_idle_hwmod(struct omap_device *od)
 {
 	omap_hwmod_idle(od->hwmods[0]);
@@ -298,6 +310,7 @@ core_initcall(omap_serial_early_init);
 /**
  * omap_serial_init_port() - initialize single serial port
  * @bdata: port specific board data pointer
+ * @info: platform specific data pointer
  *
  * This function initialies serial driver for given port only.
  * Platforms can call this function instead of omap_serial_init()
@@ -306,7 +319,8 @@ core_initcall(omap_serial_early_init);
  * Don't mix calls to omap_serial_init_port() and omap_serial_init(),
  * use only one of the two.
  */
-void __init omap_serial_init_port(struct omap_board_data *bdata)
+void __init omap_serial_init_port(struct omap_board_data *bdata,
+			struct omap_uart_port_info *info)
 {
 	struct omap_hwmod *oh;
 	struct platform_device *pdev;
@@ -325,6 +339,9 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
 	if (!oh)
 		return;
 
+	if (info == NULL)
+		info = omap_serial_default_info;
+
 	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
 	if (!pdata) {
 		pr_err("Memory allocation for UART pdata failed\n");
@@ -338,6 +355,10 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
 	pdata->enable_wakeup = omap_uart_enable_wakeup;
 	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
 	pdata->set_force_idle = omap_uart_set_forceidle;
+	pdata->dma_enabled = info->dma_enabled;
+	pdata->dma_rx_buf_size = info->dma_rx_buf_size;
+	pdata->dma_rx_timeout = info->dma_rx_timeout;
+	pdata->autosuspend_timeout = info->autosuspend_timeout;
 
 	/* Enable the MDR1 errata for OMAP3 */
 	if (cpu_is_omap34xx() && !cpu_is_ti816x())
@@ -371,13 +392,14 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
 }
 
 /**
- * omap_serial_init() - initialize all supported serial ports
+ * omap_serial_board_init() - initialize all supported serial ports
+ * @info: platform specific data pointer
  *
  * Initializes all available UARTs as serial ports. Platforms
  * can call this function when they want to have default behaviour
  * for serial ports (e.g initialize them all as serial ports).
  */
-void __init omap_serial_init(void)
+void __init omap_serial_board_init(struct omap_uart_port_info *info)
 {
 	struct omap_board_data bdata;
 	u8 i;
@@ -391,7 +413,21 @@ void __init omap_serial_init(void)
 		if (cpu_is_omap44xx() || cpu_is_omap34xx())
 			omap_serial_fill_default_pads(&bdata);
 
-		omap_serial_init_port(&bdata);
-
+		if (info == NULL)
+			omap_serial_init_port(&bdata, NULL);
+		else
+			omap_serial_init_port(&bdata, &info[i]);
 	}
 }
+
+/**
+ * omap_serial_init() - initialize all supported serial ports
+ *
+ * Initializes all available UARTs.
+ * Platforms can call this function when they want to have default behaviour
+ * for serial ports (e.g initialize them all as serial ports).
+ */
+void __init omap_serial_init(void)
+{
+	omap_serial_board_init(NULL);
+}
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 12f5262..a40a41a 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -64,6 +64,9 @@ struct omap_uart_port_info {
 	upf_t			flags;		/* UPF_* flags */
 
 	u32			errata;
+	unsigned int		dma_rx_buf_size;
+	unsigned int		dma_rx_timeout;
+	unsigned int		autosuspend_timeout;
 
 	void (*enable_wakeup)(struct platform_device *, bool);
 	u32 (*get_context_loss_count)(struct device *);
@@ -91,8 +94,8 @@ struct uart_omap_dma {
 	spinlock_t		rx_lock;
 	/* timer to poll activity on rx dma */
 	struct timer_list	rx_timer;
-	int			rx_buf_size;
-	int			rx_timeout;
+	unsigned int		rx_buf_size;
+	unsigned int		rx_timeout;
 };
 
 struct uart_omap_port {
diff --git a/arch/arm/plat-omap/include/plat/serial.h b/arch/arm/plat-omap/include/plat/serial.h
index a8bc1f1..d71e2c7 100644
--- a/arch/arm/plat-omap/include/plat/serial.h
+++ b/arch/arm/plat-omap/include/plat/serial.h
@@ -106,10 +106,13 @@
 #ifndef __ASSEMBLER__
 
 struct omap_board_data;
+struct omap_uart_port_info;
 
 extern void omap_serial_init(void);
-extern void omap_serial_init_port(struct omap_board_data *bdata);
 extern int omap_uart_can_sleep(void);
+extern void omap_serial_board_init(struct omap_uart_port_info *platform_data);
+extern void omap_serial_init_port(struct omap_board_data *bdata,
+		struct omap_uart_port_info *platform_data);
 #endif
 
 #endif
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 817443e..15e0655 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -43,8 +43,6 @@
 #include <plat/dmtimer.h>
 #include <plat/omap-serial.h>
 
-#define OMAP_UART_AUTOSUSPEND_DELAY -1
-
 static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 
 /* Forward declaration of functions */
@@ -1364,8 +1362,8 @@ static int serial_omap_probe(struct platform_device *pdev)
 		up->uart_dma.uart_dma_tx = dma_tx->start;
 		up->uart_dma.uart_dma_rx = dma_rx->start;
 		up->use_dma = 1;
-		up->uart_dma.rx_buf_size = 4096;
-		up->uart_dma.rx_timeout = 2;
+		up->uart_dma.rx_buf_size = omap_up_info->dma_rx_buf_size;
+		up->uart_dma.rx_timeout = omap_up_info->dma_rx_timeout;
 		spin_lock_init(&(up->uart_dma.tx_lock));
 		spin_lock_init(&(up->uart_dma.rx_lock));
 		up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
@@ -1374,7 +1372,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev,
-			OMAP_UART_AUTOSUSPEND_DELAY);
+			omap_up_info->autosuspend_timeout);
 
 	pm_runtime_irq_safe(&pdev->dev);
 	if (device_may_wakeup(&pdev->dev)) {
-- 
1.7.4.1

[PATCH v6 13/16] OMAP2+: UART: Make the RX_TIMEOUT for DMA configurable for each UART

[Govindraj.R]

From: Jon Hunter <jon-hunter@ti.com>

When using DMA there are two timeouts defined. The first timeout,
rx_timeout, is really a polling rate in which software polls the
DMA status to see if the DMA has finished. This is necessary for
the RX side because we do not know how much data we will receive.
The secound timeout, RX_TIMEOUT, is a timeout after which the
DMA will be stopped if no more data is received. To make this
clearer, rename rx_timeout as rx_poll_rate and rename the
function serial_omap_rx_timeout() to serial_omap_rxdma_poll().

The OMAP-Serial driver defines an RX_TIMEOUT of 3 seconds that is
used to indicate when the DMA for UART can be stopped if no more
data is received. The value is a global definition that is applied
to all instances of the UART.

Each UART may be used for a different purpose and so the timeout
required may differ. Make this value configurable for each UART so
that this value can be optimised for power savings.

Acked-by: Kevin Hilman <khilman@ti.com>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
This patch is derived from a custom 2.6.35 kernel
Available here:
http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;
h=b47839fb84e68333ea29a327cf5aa694b71be7d6;
hp=f62f570e8580507be48ad004defc2ec714b6e05d

 arch/arm/mach-omap2/serial.c                  |    5 ++++-
 arch/arm/plat-omap/include/plat/omap-serial.h |    3 ++-
 drivers/tty/serial/omap-serial.c              |   15 ++++++++-------
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 78f7051..24b5809 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -54,13 +54,15 @@
 
 static u8 num_uarts;
 
-#define DEFAULT_RXDMA_TIMEOUT		1	/* RX DMA polling rate (us) */
+#define DEFAULT_RXDMA_POLLRATE		1	/* RX DMA polling rate (us) */
 #define DEFAULT_RXDMA_BUFSIZE		4096	/* RX DMA buffer size */
+#define DEFAULT_RXDMA_TIMEOUT		(3 * HZ)/* RX DMA timeout (jiffies) */
 
 static struct omap_uart_port_info omap_serial_default_info[] = {
 	{
 		.dma_enabled	= 0,
 		.dma_rx_buf_size = DEFAULT_RXDMA_BUFSIZE,
+		.dma_rx_poll_rate = DEFAULT_RXDMA_POLLRATE,
 		.dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
 		.autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
 	},
@@ -357,6 +359,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 	pdata->set_force_idle = omap_uart_set_forceidle;
 	pdata->dma_enabled = info->dma_enabled;
 	pdata->dma_rx_buf_size = info->dma_rx_buf_size;
+	pdata->dma_rx_poll_rate = info->dma_rx_poll_rate;
 	pdata->dma_rx_timeout = info->dma_rx_timeout;
 	pdata->autosuspend_timeout = info->autosuspend_timeout;
 
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index a40a41a..28abc6b 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -51,7 +51,6 @@
 
 #define OMAP_UART_DMA_CH_FREE	-1
 
-#define RX_TIMEOUT		(3 * HZ)
 #define OMAP_MAX_HSUART_PORTS	4
 
 #define MSR_SAVE_FLAGS		UART_MSR_ANY_DELTA
@@ -67,6 +66,7 @@ struct omap_uart_port_info {
 	unsigned int		dma_rx_buf_size;
 	unsigned int		dma_rx_timeout;
 	unsigned int		autosuspend_timeout;
+	unsigned int		dma_rx_poll_rate;
 
 	void (*enable_wakeup)(struct platform_device *, bool);
 	u32 (*get_context_loss_count)(struct device *);
@@ -95,6 +95,7 @@ struct uart_omap_dma {
 	/* timer to poll activity on rx dma */
 	struct timer_list	rx_timer;
 	unsigned int		rx_buf_size;
+	unsigned int		rx_poll_rate;
 	unsigned int		rx_timeout;
 };
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 15e0655..64e4ab5 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -47,7 +47,7 @@ static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 
 /* Forward declaration of functions */
 static void uart_tx_dma_callback(int lch, u16 ch_status, void *data);
-static void serial_omap_rx_timeout(unsigned long uart_no);
+static void serial_omap_rxdma_poll(unsigned long uart_no);
 static int serial_omap_start_rxdma(struct uart_omap_port *up);
 static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1);
 
@@ -542,7 +542,7 @@ static int serial_omap_startup(struct uart_port *port)
 			(dma_addr_t *)&(up->uart_dma.tx_buf_dma_phys),
 			0);
 		init_timer(&(up->uart_dma.rx_timer));
-		up->uart_dma.rx_timer.function = serial_omap_rx_timeout;
+		up->uart_dma.rx_timer.function = serial_omap_rxdma_poll;
 		up->uart_dma.rx_timer.data = up->pdev->id;
 		/* Currently the buffer size is 4KB. Can increase it */
 		up->uart_dma.rx_buf = dma_alloc_coherent(NULL,
@@ -1147,7 +1147,7 @@ static int serial_omap_resume(struct device *dev)
 	return 0;
 }
 
-static void serial_omap_rx_timeout(unsigned long uart_no)
+static void serial_omap_rxdma_poll(unsigned long uart_no)
 {
 	struct uart_omap_port *up = ui[uart_no];
 	unsigned int curr_dma_pos, curr_transmitted_size;
@@ -1157,9 +1157,9 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
 	if ((curr_dma_pos == up->uart_dma.prev_rx_dma_pos) ||
 			     (curr_dma_pos == 0)) {
 		if (jiffies_to_msecs(jiffies - up->port_activity) <
-							RX_TIMEOUT) {
+						up->uart_dma.rx_timeout) {
 			mod_timer(&up->uart_dma.rx_timer, jiffies +
-				usecs_to_jiffies(up->uart_dma.rx_timeout));
+				usecs_to_jiffies(up->uart_dma.rx_poll_rate));
 		} else {
 			serial_omap_stop_rxdma(up);
 			up->ier |= (UART_IER_RDI | UART_IER_RLSI);
@@ -1188,7 +1188,7 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
 		}
 	} else  {
 		mod_timer(&up->uart_dma.rx_timer, jiffies +
-			usecs_to_jiffies(up->uart_dma.rx_timeout));
+			usecs_to_jiffies(up->uart_dma.rx_poll_rate));
 	}
 	up->port_activity = jiffies;
 }
@@ -1227,7 +1227,7 @@ static int serial_omap_start_rxdma(struct uart_omap_port *up)
 	/* FIXME: Cache maintenance needed here? */
 	omap_start_dma(up->uart_dma.rx_dma_channel);
 	mod_timer(&up->uart_dma.rx_timer, jiffies +
-				usecs_to_jiffies(up->uart_dma.rx_timeout));
+				usecs_to_jiffies(up->uart_dma.rx_poll_rate));
 	up->uart_dma.rx_dma_used = true;
 	return ret;
 }
@@ -1364,6 +1364,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 		up->use_dma = 1;
 		up->uart_dma.rx_buf_size = omap_up_info->dma_rx_buf_size;
 		up->uart_dma.rx_timeout = omap_up_info->dma_rx_timeout;
+		up->uart_dma.rx_poll_rate = omap_up_info->dma_rx_poll_rate;
 		spin_lock_init(&(up->uart_dma.tx_lock));
 		spin_lock_init(&(up->uart_dma.rx_lock));
 		up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
-- 
1.7.4.1

[PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken

[Govindraj.R]

In suspend path the console_lock is taken by uart_port_suspend
however when no_console_suspend is used console_lock is not taken.

During system wide suspend omap_pwr_domain hooks cut all
clocks that are left enabled. So its unsafe to proceed printing after
clocks are cut by pwr_domain hooks. Also pm_runtime will be disabled after
dpm_suspend devices happens. So buffer all prints in suspend path by taking
console_lock and print them back safely after power domain hooks re-enable
clocks back.

Use CONFIG_SERIAL_OMAP_CONSOLE macro check to take console_lock since
console ops are available only if omap console is defined.
omap-serial can be built as module without console support.

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/plat-omap/include/plat/omap-serial.h |    1 +
 drivers/tty/serial/omap-serial.c              |   20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 28abc6b..de8de87 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -126,6 +126,7 @@ struct uart_omap_port {
 	u32			context_loss_cnt;
 	u8			wakeups_enabled;
 	u32			errata;
+	u8			console_locked;
 
 };
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 64e4ab5..92a1f10 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1133,8 +1133,15 @@ static int serial_omap_suspend(struct device *dev)
 {
 	struct uart_omap_port *up = dev_get_drvdata(dev);
 
-	if (up)
+	if (up) {
 		uart_suspend_port(&serial_omap_reg, &up->port);
+#ifdef CONFIG_SERIAL_OMAP_CONSOLE
+		if (up->port.line == up->port.cons->index &&
+				!is_console_locked())
+			up->console_locked = console_trylock();
+#endif
+	}
+
 	return 0;
 }
 
@@ -1142,8 +1149,17 @@ static int serial_omap_resume(struct device *dev)
 {
 	struct uart_omap_port *up = dev_get_drvdata(dev);
 
-	if (up)
+	if (up) {
 		uart_resume_port(&serial_omap_reg, &up->port);
+#ifdef CONFIG_SERIAL_OMAP_CONSOLE
+		if (up->port.line == up->port.cons->index &&
+					up->console_locked) {
+			console_unlock();
+			up->console_locked = 0;
+		}
+#endif
+	}
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Govindraj.R]

For the early console probing we had avoided hwmod reset and idling
and uart was idled using hwmod API and enabled back using omap_device API
after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and
use runtime API to enable back UART.

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c                  |   26 ++++++++----------------
 arch/arm/plat-omap/include/plat/omap-serial.h |    1 +
 drivers/tty/serial/omap-serial.c              |    1 +
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 24b5809..15d6c51 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -25,13 +25,11 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
-#include <linux/console.h>
 
 #include <plat/omap-serial.h>
 #include <plat/common.h>
 #include <plat/board.h>
 #include <plat/dma.h>
-#include <plat/omap_hwmod.h>
 #include <plat/omap_device.h>
 #include <plat/omap-pm.h>
 
@@ -275,10 +273,16 @@ static void omap_uart_set_forceidle(struct platform_device *pdev)
 	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_FORCE);
 }
 
+static void omap_uart_hwmod_idle(struct platform_device *pdev)
+{
+	struct omap_device *od = to_omap_device(pdev);
+
+	omap_hwmod_idle(od->hwmods[0]);
+}
+
 #else
 static void omap_uart_set_forceidle(struct platform_device *pdev) {}
-static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
-{}
+static void omap_uart_hwmod_idle(struct platform_device *pdev) {}
 #endif /* CONFIG_PM */
 
 static int __init omap_serial_early_init(void)
@@ -357,6 +361,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 	pdata->enable_wakeup = omap_uart_enable_wakeup;
 	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
 	pdata->set_force_idle = omap_uart_set_forceidle;
+	pdata->hwmod_idle = omap_uart_hwmod_idle;
 	pdata->dma_enabled = info->dma_enabled;
 	pdata->dma_rx_buf_size = info->dma_rx_buf_size;
 	pdata->dma_rx_poll_rate = info->dma_rx_poll_rate;
@@ -375,19 +380,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 
 	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
 
-	console_lock(); /* in case the earlycon is on the UART */
-
-	/*
-	 * Because of early UART probing, UART did not get idled
-	 * on init.  Now that omap_device is ready, ensure full idle
-	 * before doing omap_device_enable().
-	 */
-	omap_hwmod_idle(oh);
-
-	omap_device_enable(pdev);
-
-	console_unlock();
-
 	if ((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
 		device_init_wakeup(&pdev->dev, true);
 
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index de8de87..800f215 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -71,6 +71,7 @@ struct omap_uart_port_info {
 	void (*enable_wakeup)(struct platform_device *, bool);
 	u32 (*get_context_loss_count)(struct device *);
 	void (*set_force_idle)(struct platform_device *);
+	void (*hwmod_idle)(struct platform_device *);
 };
 
 struct uart_omap_dma {
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 92a1f10..8125a89 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1394,6 +1394,7 @@ static int serial_omap_probe(struct platform_device *pdev)
 	pm_runtime_irq_safe(&pdev->dev);
 	if (device_may_wakeup(&pdev->dev)) {
 		pm_runtime_enable(&pdev->dev);
+		omap_up_info->hwmod_idle(pdev);
 		pm_runtime_get_sync(&pdev->dev);
 	}
 
-- 
1.7.4.1

[PATCH v6 16/16] OMAP2+: UART: Do not gate uart clocks if used for debug_prints

[Govindraj.R]

If OMAP UART is used as console uart and debug is enabled,
avoid gating of uart clocks to print all debug prints.

If uart clocks are gated then the debug prints from omap_device
framework or hwmod framework can cause uart to enter recursive pm_runtime calls,
which can cause a deadlock over power lock usage.

For example: Say, uart clocks are cut and we get a print from omap_device_disable
stating disabling uart clocks. This print calls omap_uart driver console_write
which will call runtime API get_sync which means we enter from runtime API put
context to runtime API get context.

--> runtime put (take power lock)
    --> print disabling uart clocks
        --> call uart console write
            --> call get_sync (try to take power lock)

Also any clock enable API call from uart driver should not call any uart
operation until clocks are enabled back. Like get_sync having debug print
calling uart console write even before clocks are enabled.

So to avoid these scenarios, identify from bootargs  if OMAP_UART(ttyO) is used
in debug mode. If so, do not set device_may_wakeup. This will prevent
pm_runtime_enable in uart driver probe and will avoid uart clock gating.

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
More details on this topic and experiments done listed here:
http://www.spinics.net/lists/linux-serial/msg04128.html

 arch/arm/mach-omap2/serial.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 15d6c51..777f81c 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -285,8 +285,24 @@ static void omap_uart_set_forceidle(struct platform_device *pdev) {}
 static void omap_uart_hwmod_idle(struct platform_device *pdev) {}
 #endif /* CONFIG_PM */
 
+static int uart_debug;
+
+char *cmdline_find_option(char *str)
+{
+	extern char *saved_command_line;
+
+	return strstr(saved_command_line, str);
+}
+
 static int __init omap_serial_early_init(void)
 {
+	if (cmdline_find_option("debug") &&
+			cmdline_find_option(OMAP_SERIAL_NAME)) {
+		uart_debug = true;
+		pr_info("OMAP UART used as console in debug mode"
+			" uart clocks will not be gated");
+	}
+
 	do {
 		struct omap_hwmod *oh;
 
@@ -380,7 +396,8 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 
 	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
 
-	if ((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
+	if (((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
+			&& !uart_debug)
 		device_init_wakeup(&pdev->dev, true);
 
 	kfree(pdata);
-- 
1.7.4.1

[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> Modify the omap_uart_can_sleep function to check uart is active
> or not to be used by pm code to enter low power states.

Doesn't the driver now control when the UART clocks are gated (using
runtime PM autosuspend)?

IMO, this check should be completely removed and the driver should
be managing this with the autosuspend timeout.

> Removing this check can cause console response little sluggish.

Sluggish in what way?

If the driver is runtime suspended, it should only be sluggish for the
first character.  After that, the autosuspend timeout should prevent it
from feeling sluggish.

> However no characters will be lost until uart clocks are gated
> and woken up using rx-pad. UART interface clocks can be auto gated
> this can make response on uart slower. This behaviour was observed
> only on some of OMAP3 boards(beagleboard xm rev c).
>
> Reported-by: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
>  arch/arm/mach-omap2/serial.c |   21 +++++++++------------
>  1 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 6725caf..ccf3550 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -156,23 +156,20 @@ static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
>  
>  int omap_uart_can_sleep(void)
>  {
> -	struct omap_uart_state *uart;
> -	int can_sleep = 1;
> -
> -	list_for_each_entry(uart, &uart_list, node) {
> -		if (!uart->clocked)
> -			continue;
> +	struct omap_hwmod *oh;
> +	u8 i, ret = true;
>  
> -		if (!uart->can_sleep) {
> -			can_sleep = 0;
> +	for (i = 0; i < num_uarts; i++) {
> +		oh = omap_uart_hwmod_lookup(i);

This is a heavy operation to add for *every* entry into idle.

> +		if (!oh)
>  			continue;
> -		}
>  
> -		/* This UART can now safely sleep. */
> -		omap_uart_allow_sleep(uart);
> +		if (oh->od && oh->od->pdev &&
> +				!pm_runtime_suspended(&oh->od->pdev->dev))
> +			return false;
>  	}
>  
> -	return can_sleep;
> +	return ret;
>  }
>  
>  #ifdef CONFIG_OMAP_MUX

Kevin

[PATCH v6 12/16] OMAP2+: UART: Allow UART parameters to be configured from board file.

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> From: Deepak K <deepak.k@ti.com>
>
> The following UART parameters are defined within the UART driver:
>
> 1). Whether the UART uses DMA (dma_enabled), by default set to 0
> 2). The size of dma buffer (set to 4096 bytes)
> 3). The time after which the dma should stop if no more data is received.
> 4). The auto suspend delay that will be passed for pm_runtime_autosuspend
>     where uart will be disabled after timeout
>
> Different UARTs may be used for different purpose such as the console,
> for interfacing bluetooth chip, for interfacing to a modem chip, etc.
> Therefore, it is necessary to be able to customize the above settings
> for a given board on a per UART basis.
>
> This change allows these parameters to be configured from the board file
> and allows the parameters to be configured for each UART independently.
>
> If a board does not define its own custom parameters for the UARTs, then
> use the default parameters in the structure "omap_serial_default_info".
> The default parameters are defined to be the same as the current settings
> in the UART driver to avoid breaking the UART for any cuurnelty supported
> boards. By default, make all boards use the default UART parameters.
>
> Signed-off-by: Deepak K <deepak.k@ti.com>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>

A couple minor comments below...

> ---
> This patch is derived and reworked from a custom 2.6.35 kernel
> Available here:
> http://git.omapzoom.org/?p=kernel/omap.git;
> a=commitdiff;h=584ef316542f77312be7ba96a0f3013c8f64852b;
> hp=7233a76cb362c0fc603f773274159adff91d3513
>
>  arch/arm/mach-omap2/board-n8x0.c              |    6 +-
>  arch/arm/mach-omap2/serial.c                  |   56 ++++++++++++++++++++----
>  arch/arm/plat-omap/include/plat/omap-serial.h |    7 ++-
>  arch/arm/plat-omap/include/plat/serial.h      |    5 ++-
>  drivers/tty/serial/omap-serial.c              |    8 +--
>  5 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c
> index e11f0c5..3408726 100644
> --- a/arch/arm/mach-omap2/board-n8x0.c
> +++ b/arch/arm/mach-omap2/board-n8x0.c
> @@ -656,15 +656,15 @@ static inline void board_serial_init(void)
>  	bdata.pads_cnt = 0;
>  
>  	bdata.id = 0;
> -	omap_serial_init_port(&bdata);
> +	omap_serial_init_port(&bdata, NULL);
>  
>  	bdata.id = 1;
> -	omap_serial_init_port(&bdata);
> +	omap_serial_init_port(&bdata, NULL);
>  
>  	bdata.id = 2;
>  	bdata.pads = serial2_pads;
>  	bdata.pads_cnt = ARRAY_SIZE(serial2_pads);
> -	omap_serial_init_port(&bdata);
> +	omap_serial_init_port(&bdata, NULL);
>  }
>  
>  #else
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 0731575..78f7051 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -43,17 +43,29 @@
>  #include "mux.h"
>  
>  /*
> - * NOTE: By default the serial timeout is disabled as it causes lost characters
> - * over the serial ports. This means that the UART clocks will stay on until
> - * disabled via sysfs. This also causes that any deeper omap sleep states are
> - * blocked. 
> + * NOTE: By default the serial auto_suspend timeout is disabled as it causes
> + * lost characters over the serial ports. This means that the UART clocks will
> + * stay on until power/autosuspend_delay is set for the uart from sysfs.
> + * This also causes that any deeper omap sleep states are blocked.
>   */
> -#define DEFAULT_TIMEOUT 0
> +#define DEFAULT_AUTOSUSPEND_DELAY	-1
>  
>  #define MAX_UART_HWMOD_NAME_LEN		16
>  
>  static u8 num_uarts;
>  
> +#define DEFAULT_RXDMA_TIMEOUT		1	/* RX DMA polling rate (us) */
> +#define DEFAULT_RXDMA_BUFSIZE		4096	/* RX DMA buffer size */
> +
> +static struct omap_uart_port_info omap_serial_default_info[] = {

This could be __initdata

> +	{
> +		.dma_enabled	= 0,

This field is a bool, use 'false' instead of 0.

> +		.dma_rx_buf_size = DEFAULT_RXDMA_BUFSIZE,
> +		.dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
> +		.autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
> +	},
> +};
> +
>  static int uart_idle_hwmod(struct omap_device *od)
>  {
>  	omap_hwmod_idle(od->hwmods[0]);
> @@ -298,6 +310,7 @@ core_initcall(omap_serial_early_init);
>  /**
>   * omap_serial_init_port() - initialize single serial port
>   * @bdata: port specific board data pointer
> + * @info: platform specific data pointer
>   *
>   * This function initialies serial driver for given port only.
>   * Platforms can call this function instead of omap_serial_init()
> @@ -306,7 +319,8 @@ core_initcall(omap_serial_early_init);
>   * Don't mix calls to omap_serial_init_port() and omap_serial_init(),
>   * use only one of the two.
>   */
> -void __init omap_serial_init_port(struct omap_board_data *bdata)
> +void __init omap_serial_init_port(struct omap_board_data *bdata,
> +			struct omap_uart_port_info *info)

alignment.  2nd argument should align with 1st

>  {
>  	struct omap_hwmod *oh;
>  	struct platform_device *pdev;
> @@ -325,6 +339,9 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>  	if (!oh)
>  		return;
>  
> +	if (info == NULL)

if (!info)

there's another one of these elsewhere in the patch too.

[...]

Kevin

[PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> In suspend path the console_lock is taken by uart_port_suspend
> however when no_console_suspend is used console_lock is not taken.
>
> During system wide suspend omap_pwr_domain hooks cut all
> clocks that are left enabled. So its unsafe to proceed printing after
> clocks are cut by pwr_domain hooks. 

As I've mentioned in previous reviews, when no_console_suspend is
enabled, the user has explicitly requested console output during
suspend.  In order to support that, we should not be cutting clocks at
all in that mode.

One way to address this would be to just disable runtime PM in the
->prepare method of the driver if no_console_suspend is enabled.

> Also pm_runtime will be disabled after dpm_suspend devices happens.

Not sure why this is relevant.

> So buffer all prints in suspend path by taking console_lock and print
> them back safely after power domain hooks re-enable clocks back.
>
> Use CONFIG_SERIAL_OMAP_CONSOLE macro check to take console_lock since
> console ops are available only if omap console is defined.
> omap-serial can be built as module without console support.

Please revisit use of #ifdefs.  (c.f. #ifdefs are ugly in
Documentation/SubmittingPatches)

Kevin

[PATCH v6 11/16] OMAP2+: UART: Move errata handling from serial.c to omap-serial

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> Move the errata handling mechanism from serial.c to omap-serial file
> and utilise the same func in driver file.
>
> Errata i202, i291 are moved to be handled with omap-serial
> Moving the errata macro from serial.c file to driver header file
> as from on errata will be handled in driver file itself.
>
> Corrected errata id from chapter reference 2.15 to errata id i291.
>
> Acked-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>

The errata handling for i291 is moved from device code to driver code
but it's functionality is also changed (but not described.)

In the current mainline code, The workaround for i291 is done whenever
UART clocks are about to be cut.  When the clocks are (re)enabled, the
device is put back into no-idle.

This patch puts the device into force-idle just before the idle/suspend
transistion, but never puts it back into no-idle.  So after the first
idle/suspend transition, the IP remains in force-idle forever.  I don't
think that's what we want.

Kevin

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> For the early console probing we had avoided hwmod reset and idling
> and uart was idled using hwmod API and enabled back using omap_device API
> after omap_device registration.
>
> Now since we are using runtime API's to enable back uart, move hwmod
> idling and use runtime API to enable back UART.
>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>

Now that the driver is using runtime PM.  Why do we still need
HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?

The comment in the code says:

		/*
		 * During UART early init, device need to be probed
		 * to determine SoC specific init before omap_device
		 * is ready.  Therefore, don't allow idle here
		 */

This was true when using the 8250 driver because it was not using
runtime PM so could not know how to (re)enable the device.

However, since the driver is now runtime PM adapted, any device access
should be contained within a runtime PM get/put block, so there should
no longer be a reason not allow the IP blocks to be reset during boot.

Kevin

[PATCH v6 11/16] OMAP2+: UART: Move errata handling from serial.c to omap-serial

[Govindraj]

On Wed, Oct 12, 2011 at 2:31 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> Move the errata handling mechanism from serial.c to omap-serial file
>> and utilise the same func in driver file.
>>
>> Errata i202, i291 are moved to be handled with omap-serial
>> Moving the errata macro from serial.c file to driver header file
>> as from on errata will be handled in driver file itself.
>>
>> Corrected errata id from chapter reference 2.15 to errata id i291.
>>
>> Acked-by: Alan Cox <alan@linux.intel.com>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>
> The errata handling for i291 is moved from device code to driver code
> but it's functionality is also changed (but not described.)
>
> In the current mainline code, The workaround for i291 is done whenever
> UART clocks are about to be cut. ?When the clocks are (re)enabled, the
> device is put back into no-idle.
>
> This patch puts the device into force-idle just before the idle/suspend
> transistion, but never puts it back into no-idle. ?So after the first
> idle/suspend transition, the IP remains in force-idle forever. ?I don't
> think that's what we want.
>

I was thinking no_idle will be done once we re-enable uart clocks
from omap_device_enable but that's not the case looking into the code.

Will correct it will add no_idle call from .runtime_resume as done early
where we put into no_idle(sysc) while enabling the clocks.

--
Thanks,
Govindraj.R

[PATCH v6 12/16] OMAP2+: UART: Allow UART parameters to be configured from board file.

[Govindraj]

On Wed, Oct 12, 2011 at 12:23 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> From: Deepak K <deepak.k@ti.com>
>>
>> The following UART parameters are defined within the UART driver:
>>
>> 1). Whether the UART uses DMA (dma_enabled), by default set to 0
>> 2). The size of dma buffer (set to 4096 bytes)
>> 3). The time after which the dma should stop if no more data is received.
>> 4). The auto suspend delay that will be passed for pm_runtime_autosuspend
>> ? ? where uart will be disabled after timeout
>>
>> Different UARTs may be used for different purpose such as the console,
>> for interfacing bluetooth chip, for interfacing to a modem chip, etc.
>> Therefore, it is necessary to be able to customize the above settings
>> for a given board on a per UART basis.
>>
>> This change allows these parameters to be configured from the board file
>> and allows the parameters to be configured for each UART independently.
>>
>> If a board does not define its own custom parameters for the UARTs, then
>> use the default parameters in the structure "omap_serial_default_info".
>> The default parameters are defined to be the same as the current settings
>> in the UART driver to avoid breaking the UART for any cuurnelty supported
>> boards. By default, make all boards use the default UART parameters.
>>
>> Signed-off-by: Deepak K <deepak.k@ti.com>
>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>
> A couple minor comments below...
>
>> ---
>> This patch is derived and reworked from a custom 2.6.35 kernel
>> Available here:
>> http://git.omapzoom.org/?p=kernel/omap.git;
>> a=commitdiff;h=584ef316542f77312be7ba96a0f3013c8f64852b;
>> hp=7233a76cb362c0fc603f773274159adff91d3513
>>
>> ?arch/arm/mach-omap2/board-n8x0.c ? ? ? ? ? ? ?| ? ?6 +-
>> ?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 56 ++++++++++++++++++++----
>> ?arch/arm/plat-omap/include/plat/omap-serial.h | ? ?7 ++-
>> ?arch/arm/plat-omap/include/plat/serial.h ? ? ?| ? ?5 ++-
>> ?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ? ?8 +--
>> ?5 files changed, 61 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c
>> index e11f0c5..3408726 100644
>> --- a/arch/arm/mach-omap2/board-n8x0.c
>> +++ b/arch/arm/mach-omap2/board-n8x0.c
>> @@ -656,15 +656,15 @@ static inline void board_serial_init(void)
>> ? ? ? bdata.pads_cnt = 0;
>>
>> ? ? ? bdata.id = 0;
>> - ? ? omap_serial_init_port(&bdata);
>> + ? ? omap_serial_init_port(&bdata, NULL);
>>
>> ? ? ? bdata.id = 1;
>> - ? ? omap_serial_init_port(&bdata);
>> + ? ? omap_serial_init_port(&bdata, NULL);
>>
>> ? ? ? bdata.id = 2;
>> ? ? ? bdata.pads = serial2_pads;
>> ? ? ? bdata.pads_cnt = ARRAY_SIZE(serial2_pads);
>> - ? ? omap_serial_init_port(&bdata);
>> + ? ? omap_serial_init_port(&bdata, NULL);
>> ?}
>>
>> ?#else
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 0731575..78f7051 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -43,17 +43,29 @@
>> ?#include "mux.h"
>>
>> ?/*
>> - * NOTE: By default the serial timeout is disabled as it causes lost characters
>> - * over the serial ports. This means that the UART clocks will stay on until
>> - * disabled via sysfs. This also causes that any deeper omap sleep states are
>> - * blocked.
>> + * NOTE: By default the serial auto_suspend timeout is disabled as it causes
>> + * lost characters over the serial ports. This means that the UART clocks will
>> + * stay on until power/autosuspend_delay is set for the uart from sysfs.
>> + * This also causes that any deeper omap sleep states are blocked.
>> ? */
>> -#define DEFAULT_TIMEOUT 0
>> +#define DEFAULT_AUTOSUSPEND_DELAY ? ?-1
>>
>> ?#define MAX_UART_HWMOD_NAME_LEN ? ? ? ? ? ? ?16
>>
>> ?static u8 num_uarts;
>>
>> +#define DEFAULT_RXDMA_TIMEOUT ? ? ? ? ? ? ? ?1 ? ? ? /* RX DMA polling rate (us) */
>> +#define DEFAULT_RXDMA_BUFSIZE ? ? ? ? ? ? ? ?4096 ? ?/* RX DMA buffer size */
>> +
>> +static struct omap_uart_port_info omap_serial_default_info[] = {
>
> This could be __initdata
>

yes correct, will change this.


>> + ? ? {
>> + ? ? ? ? ? ? .dma_enabled ? ?= 0,
>
> This field is a bool, use 'false' instead of 0.

yes fine.

>
>> + ? ? ? ? ? ? .dma_rx_buf_size = DEFAULT_RXDMA_BUFSIZE,
>> + ? ? ? ? ? ? .dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
>> + ? ? ? ? ? ? .autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
>> + ? ? },
>> +};
>> +
>> ?static int uart_idle_hwmod(struct omap_device *od)
>> ?{
>> ? ? ? omap_hwmod_idle(od->hwmods[0]);
>> @@ -298,6 +310,7 @@ core_initcall(omap_serial_early_init);
>> ?/**
>> ? * omap_serial_init_port() - initialize single serial port
>> ? * @bdata: port specific board data pointer
>> + * @info: platform specific data pointer
>> ? *
>> ? * This function initialies serial driver for given port only.
>> ? * Platforms can call this function instead of omap_serial_init()
>> @@ -306,7 +319,8 @@ core_initcall(omap_serial_early_init);
>> ? * Don't mix calls to omap_serial_init_port() and omap_serial_init(),
>> ? * use only one of the two.
>> ? */
>> -void __init omap_serial_init_port(struct omap_board_data *bdata)
>> +void __init omap_serial_init_port(struct omap_board_data *bdata,
>> + ? ? ? ? ? ? ? ? ? ? struct omap_uart_port_info *info)
>
> alignment. ?2nd argument should align with 1st
>
>> ?{
>> ? ? ? struct omap_hwmod *oh;
>> ? ? ? struct platform_device *pdev;
>> @@ -325,6 +339,9 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>> ? ? ? if (!oh)
>> ? ? ? ? ? ? ? return;
>>
>> + ? ? if (info == NULL)
>
> if (!info)
>
> there's another one of these elsewhere in the patch too.
>

will check and correct.

--
Thanks,
Govindraj.R

[PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken

[Govindraj]

On Wed, Oct 12, 2011 at 12:31 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> In suspend path the console_lock is taken by uart_port_suspend
>> however when no_console_suspend is used console_lock is not taken.
>>
>> During system wide suspend omap_pwr_domain hooks cut all
>> clocks that are left enabled. So its unsafe to proceed printing after
>> clocks are cut by pwr_domain hooks.
>
> As I've mentioned in previous reviews, when no_console_suspend is
> enabled, the user has explicitly requested console output during
> suspend. ?In order to support that, we should not be cutting clocks at
> all in that mode.
>
> One way to address this would be to just disable runtime PM in the
> ->prepare method of the driver if no_console_suspend is enabled.
>

Okay fine exploring this option, right API's would be to use
pm_runtime_forbid/allow.

<<SNIP>>

+static int serial_omap_runtime_prepare(struct device *dev)
+{
+       if (!console_suspend_enabled)
+               pm_runtime_forbid(dev);
+
+       return 0;
+}
+
+static void serial_omap_runtime_complete(struct device *dev)
+{
+       if (!console_suspend_enabled)
+               pm_runtime_allow(dev);
+}
+
 static const struct dev_pm_ops serial_omap_dev_pm_ops = {
        SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume)
        SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend,
                                serial_omap_runtime_resume, NULL)
+       .prepare = serial_omap_runtime_prepare,
+       .complete =  serial_omap_runtime_complete,
 };

<<SNIP>>

But to either use runtime forbid or disable we have ensure that
power_domain hooks don't go ahead and disable
the clocks with omap_device_idle as *runtime forbid or disable will
not set runtime_status to RPM_SUSPENDED*
and will stay in RPM_ACTIVE if we call runtime disable or forbid from
active state.

in power_domain hooks we just check the pm_runtime_status_suspended
this will be false even if
we do runtime disable/forbid and it will cut uart clocks always.

So we may need below check also:

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index 26aee5c..286a534 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -592,7 +592,8 @@ static int _od_suspend_noirq(struct device *dev)

        ret = pm_generic_suspend_noirq(dev);

-       if (!ret && !pm_runtime_status_suspended(dev)) {
+       if (!ret && pm_runtime_enabled(dev) &&
+                       !pm_runtime_status_suspended(dev)) {
                if (pm_generic_runtime_suspend(dev) == 0) {
                        omap_device_idle(pdev);
                        od->flags |= OMAP_DEVICE_SUSPENDED;


--
Thanks,
Govindraj.R

[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[Govindraj]

On Tue, Oct 11, 2011 at 11:54 PM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> Modify the omap_uart_can_sleep function to check uart is active
>> or not to be used by pm code to enter low power states.
>
> Doesn't the driver now control when the UART clocks are gated (using
> runtime PM autosuspend)?
>
> IMO, this check should be completely removed and the driver should
> be managing this with the autosuspend timeout.
>
>> Removing this check can cause console response little sluggish.
>
> Sluggish in what way?
>

response is slower like when we type something or cat debugfs/pm_count
see things little slower on console, there is no character loss.

Happens even though we have not set the autosuspend timeout and uart
clocks are active,
which basically means allowing mpu to enter retention keeping uart active.

this delay in response or sluggishness is not there on my 3430SDP or
3630zoom board but I was able to see this behavior on a beagle board(xm rev c).

retaining this uart_can_sleep check in omap3_can_sleep ensures a better
console user experience. (not allowing mpu to enter retention
until uart clocks are cut)


> If the driver is runtime suspended, it should only be sluggish for the
> first character. ?After that, the autosuspend timeout should prevent it
> from feeling sluggish.
>
>> However no characters will be lost until uart clocks are gated
>> and woken up using rx-pad. UART interface clocks can be auto gated
>> this can make response on uart slower. This behaviour was observed
>> only on some of OMAP3 boards(beagleboard xm rev c).
>>
>> Reported-by: Tero Kristo <t-kristo@ti.com>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>> ---
>> ?arch/arm/mach-omap2/serial.c | ? 21 +++++++++------------
>> ?1 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 6725caf..ccf3550 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -156,23 +156,20 @@ static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
>>
>> ?int omap_uart_can_sleep(void)
>> ?{
>> - ? ? struct omap_uart_state *uart;
>> - ? ? int can_sleep = 1;
>> -
>> - ? ? list_for_each_entry(uart, &uart_list, node) {
>> - ? ? ? ? ? ? if (!uart->clocked)
>> - ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? struct omap_hwmod *oh;
>> + ? ? u8 i, ret = true;
>>
>> - ? ? ? ? ? ? if (!uart->can_sleep) {
>> - ? ? ? ? ? ? ? ? ? ? can_sleep = 0;
>> + ? ? for (i = 0; i < num_uarts; i++) {
>> + ? ? ? ? ? ? oh = omap_uart_hwmod_lookup(i);
>
> This is a heavy operation to add for *every* entry into idle.
>

removing uart_list caused this ops, retaining uart_list
will ensure this will be replaced with list_for_each_entry
as done in existing code.

--
Thanks,
Govindraj.R

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Govindraj]

On Wed, Oct 12, 2011 at 2:36 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> For the early console probing we had avoided hwmod reset and idling
>> and uart was idled using hwmod API and enabled back using omap_device API
>> after omap_device registration.
>>
>> Now since we are using runtime API's to enable back uart, move hwmod
>> idling and use runtime API to enable back UART.
>>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>
> Now that the driver is using runtime PM. ?Why do we still need
> HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?
>
> The comment in the code says:
>
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * During UART early init, device need to be probed
> ? ? ? ? ? ? ? ? * to determine SoC specific init before omap_device
> ? ? ? ? ? ? ? ? * is ready. ?Therefore, don't allow idle here
> ? ? ? ? ? ? ? ? */
>
> This was true when using the 8250 driver because it was not using
> runtime PM so could not know how to (re)enable the device.
>
> However, since the driver is now runtime PM adapted, any device access
> should be contained within a runtime PM get/put block, so there should
> no longer be a reason not allow the IP blocks to be reset during boot.
>

Forgot to add, this is still needed for
earlyprintk(CONFIG_EARLY_PRINTK) use case,

The initial boot prints until a console driver is available is from
"arch/arm/kernel/early_printk.c" which does a tx on uart console
and relies on configuration from bootloader.

during bootup earlyprink does a tx on uart console and if  uart driver
is not available yet
uart reset or idle done by hwmod layer can cause boot failures.

--> put_char from earlyprintk.c
     --> reset/idle from hwmod layer
          --> put_char from earlyprintk.c


So console_uart reset or clock gating must be done only after uart
driver is available or be prevented
using these available hwmod_flags.

omap_serial_early_init should be now be binded with CONFIG_SERIAL_OMAP macro ?

--
Thanks,
Govindraj.R

[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[Kevin Hilman]

Govindraj <govindraj.ti@gmail.com> writes:

> On Tue, Oct 11, 2011 at 11:54 PM, Kevin Hilman <khilman@ti.com> wrote:
>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>
>>> Modify the omap_uart_can_sleep function to check uart is active
>>> or not to be used by pm code to enter low power states.
>>
>> Doesn't the driver now control when the UART clocks are gated (using
>> runtime PM autosuspend)?
>>
>> IMO, this check should be completely removed and the driver should
>> be managing this with the autosuspend timeout.
>>
>>> Removing this check can cause console response little sluggish.
>>
>> Sluggish in what way?
>>
>
> response is slower like when we type something or cat debugfs/pm_count
> see things little slower on console, there is no character loss.
>
> Happens even though we have not set the autosuspend timeout and uart
> clocks are active, which basically means allowing mpu to enter
> retention keeping uart active.

OK, I see now.

> this delay in response or sluggishness is not there on my 3430SDP or
> 3630zoom board but I was able to see this behavior on a beagle
> board(xm rev c).

Here's why:

The difference is the powerdomain that the console UART is on for these
boards.  UART1,2 are in CORE, UART2/3 are in PER.   SDP uses UART1 (CORE),
Zoom3 doesn't use OMAP UARTs at all, and Beagle uses UART3 (PER).

Due to a HW sleepdep between MPU and CORE, MPU will not transition until
CORE does, which means MPU will not transition until UART 1 & 2 are
idle.

On Beagle, the console is ttyO2 (UART3) which is in PER, and since the
MPU is free to transition independently of PER, that is what is
happening, resulting in slower response time on for any boards that have
PER-UART consoles.

> retaining this uart_can_sleep check in omap3_can_sleep ensures a better
> console user experience. (not allowing mpu to enter retention
> until uart clocks are cut)

Yes, but obviously comes at the expense of power savings.  IOW, This is
a hard-coded power vs. performance trade off that we are trying to get
away from.

So, the root of the problem is that the MPU wakeup latency is causing a
"sluggish" console.  The solution?  request an MPU wakeup latency
constraint.

This is a classic use-case for such a constraint, and the serial driver
should have the option of requesting a constraint to prevent the sluggish
console.  The constraint only needs to be held until the auto-suspend
delay expires, so should be relased in the ->runtime_suspend() method of
the driver.

This constraint needs to be configurable, probably from the board file,
so that it is optional, and so users who don't care about sluggish
consoles (or non-console UART users who don't care about response time)
have the option of preferring power savings over UART responsiveness.

As a reference, the i2c driver is currently doing something similar 
in that it request an MPU constraint to prevent the MPU from going into
retention/off while waiting for an i2c interrupt to arrive.

Kevin

[PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken

[Kevin Hilman]

Govindraj <govindraj.ti@gmail.com> writes:

> On Wed, Oct 12, 2011 at 12:31 AM, Kevin Hilman <khilman@ti.com> wrote:
>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>
>>> In suspend path the console_lock is taken by uart_port_suspend
>>> however when no_console_suspend is used console_lock is not taken.
>>>
>>> During system wide suspend omap_pwr_domain hooks cut all
>>> clocks that are left enabled. So its unsafe to proceed printing after
>>> clocks are cut by pwr_domain hooks.
>>
>> As I've mentioned in previous reviews, when no_console_suspend is
>> enabled, the user has explicitly requested console output during
>> suspend. ?In order to support that, we should not be cutting clocks at
>> all in that mode.
>>
>> One way to address this would be to just disable runtime PM in the
>> ->prepare method of the driver if no_console_suspend is enabled.
>>
>
> Okay fine exploring this option, right API's would be to use
> pm_runtime_forbid/allow.

Yes.

> <<SNIP>>
>
> +static int serial_omap_runtime_prepare(struct device *dev)
> +{
> +       if (!console_suspend_enabled)
> +               pm_runtime_forbid(dev);
> +
> +       return 0;
> +}
> +
> +static void serial_omap_runtime_complete(struct device *dev)
> +{
> +       if (!console_suspend_enabled)
> +               pm_runtime_allow(dev);
> +}
> +
>  static const struct dev_pm_ops serial_omap_dev_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume)
>         SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend,
>                                 serial_omap_runtime_resume, NULL)
> +       .prepare = serial_omap_runtime_prepare,
> +       .complete =  serial_omap_runtime_complete,
>  };

OK, but please add comments to these functions about exactly why they
are needed.

>
> But to either use runtime forbid or disable we have ensure that
> power_domain hooks don't go ahead and disable
> the clocks with omap_device_idle as *runtime forbid or disable will
> not set runtime_status to RPM_SUSPENDED*
> and will stay in RPM_ACTIVE if we call runtime disable or forbid from
> active state.

Correct.

> in power_domain hooks we just check the pm_runtime_status_suspended
> this will be false even if
> we do runtime disable/forbid and it will cut uart clocks always.
>
> So we may need below check also:
>
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index 26aee5c..286a534 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -592,7 +592,8 @@ static int _od_suspend_noirq(struct device *dev)
>
>         ret = pm_generic_suspend_noirq(dev);
>
> -       if (!ret && !pm_runtime_status_suspended(dev)) {
> +       if (!ret && pm_runtime_enabled(dev) &&
> +                       !pm_runtime_status_suspended(dev)) {
>                 if (pm_generic_runtime_suspend(dev) == 0) {
>                         omap_device_idle(pdev);
>                         od->flags |= OMAP_DEVICE_SUSPENDED;

This isn't right either because devices that may not yet be initialized
(or loaded) may not have runtime PM enabled, so those devices may not
be properly idled.

We have an omap_device API to disable this feature at the PM domain level:
omap_device_disable_idle_on_suspend().  All you should have to do is to
use this API in the device init code on the console UART if
no_console_suspend has been enabled.

Kevin

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Kevin Hilman]

Govindraj <govindraj.ti@gmail.com> writes:

> On Wed, Oct 12, 2011 at 2:36 AM, Kevin Hilman <khilman@ti.com> wrote:
>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>
>>> For the early console probing we had avoided hwmod reset and idling
>>> and uart was idled using hwmod API and enabled back using omap_device API
>>> after omap_device registration.
>>>
>>> Now since we are using runtime API's to enable back uart, move hwmod
>>> idling and use runtime API to enable back UART.
>>>
>>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>>
>> Now that the driver is using runtime PM. ?Why do we still need
>> HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?
>>
>> The comment in the code says:
>>
>> ? ? ? ? ? ? ? ?/*
>> ? ? ? ? ? ? ? ? * During UART early init, device need to be probed
>> ? ? ? ? ? ? ? ? * to determine SoC specific init before omap_device
>> ? ? ? ? ? ? ? ? * is ready. ?Therefore, don't allow idle here
>> ? ? ? ? ? ? ? ? */
>>
>> This was true when using the 8250 driver because it was not using
>> runtime PM so could not know how to (re)enable the device.
>>
>> However, since the driver is now runtime PM adapted, any device access
>> should be contained within a runtime PM get/put block, so there should
>> no longer be a reason not allow the IP blocks to be reset during boot.
>>
>
> Forgot to add, this is still needed for
> earlyprintk(CONFIG_EARLY_PRINTK) use case,

Ah, right.  I forgot about that.  Please update the changelog (and
comment in the code) to reflect that.

> The initial boot prints until a console driver is available is from
> "arch/arm/kernel/early_printk.c" which does a tx on uart console
> and relies on configuration from bootloader.
>
> during bootup earlyprink does a tx on uart console and if  uart driver
> is not available yet
> uart reset or idle done by hwmod layer can cause boot failures.
>
> --> put_char from earlyprintk.c
>      --> reset/idle from hwmod layer
>           --> put_char from earlyprintk.c
>
>
> So console_uart reset or clock gating must be done only after uart
> driver is available or be prevented using these available hwmod_flags.

So why not leave the driver out of it and solve it like the current code
does?

The current codes use the hwmod flags, then waits until the UART driver
is available (after omap_device_build) and uses omap_hwmod_idle() to do
an clean idle of the device. 

Notably this is inside a console_lock/console_unlock block so that
prints are buffered.

The current code then does an omap_device_enable() to re-enable the
device, but you shouldn't need that after the driver is converted to
runtime PM.

Kevin

[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[Govindraj]

On Thu, Oct 13, 2011 at 1:11 AM, Kevin Hilman <khilman@ti.com> wrote:
> Govindraj <govindraj.ti@gmail.com> writes:
>
>> On Tue, Oct 11, 2011 at 11:54 PM, Kevin Hilman <khilman@ti.com> wrote:
>>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>>
>>>> Modify the omap_uart_can_sleep function to check uart is active
>>>> or not to be used by pm code to enter low power states.
>>>
>>> Doesn't the driver now control when the UART clocks are gated (using
>>> runtime PM autosuspend)?
>>>
>>> IMO, this check should be completely removed and the driver should
>>> be managing this with the autosuspend timeout.
>>>
>>>> Removing this check can cause console response little sluggish.
>>>
>>> Sluggish in what way?
>>>
>>
>> response is slower like when we type something or cat debugfs/pm_count
>> see things little slower on console, there is no character loss.
>>
>> Happens even though we have not set the autosuspend timeout and uart
>> clocks are active, which basically means allowing mpu to enter
>> retention keeping uart active.
>
> OK, I see now.
>
>> this delay in response or sluggishness is not there on my 3430SDP or
>> 3630zoom board but I was able to see this behavior on a beagle
>> board(xm rev c).
>
> Here's why:
>
> The difference is the powerdomain that the console UART is on for these
> boards. ?UART1,2 are in CORE, UART2/3 are in PER. ? SDP uses UART1 (CORE),
> Zoom3 doesn't use OMAP UARTs at all, and Beagle uses UART3 (PER).
>
> Due to a HW sleepdep between MPU and CORE, MPU will not transition until
> CORE does, which means MPU will not transition until UART 1 & 2 are
> idle.
>
> On Beagle, the console is ttyO2 (UART3) which is in PER, and since the
> MPU is free to transition independently of PER, that is what is
> happening, resulting in slower response time on for any boards that have
> PER-UART consoles.
>
>> retaining this uart_can_sleep check in omap3_can_sleep ensures a better
>> console user experience. (not allowing mpu to enter retention
>> until uart clocks are cut)
>
> Yes, but obviously comes at the expense of power savings. ?IOW, This is
> a hard-coded power vs. performance trade off that we are trying to get
> away from.
>
> So, the root of the problem is that the MPU wakeup latency is causing a
> "sluggish" console. ?The solution? ?request an MPU wakeup latency
> constraint.
>

Okay, Will explore this.

> This is a classic use-case for such a constraint, and the serial driver
> should have the option of requesting a constraint to prevent the sluggish
> console. ?The constraint only needs to be held until the auto-suspend
> delay expires, so should be relased in the ->runtime_suspend() method of
> the driver.
>
> This constraint needs to be configurable, probably from the board file,
> so that it is optional, and so users who don't care about sluggish
> consoles (or non-console UART users who don't care about response time)
> have the option of preferring power savings over UART responsiveness.
>
> As a reference, the i2c driver is currently doing something similar
> in that it request an MPU constraint to prevent the MPU from going into
> retention/off while waiting for an i2c interrupt to arrive.
>

Thanks, will check and try to use the mpu constraints

--
Govindraj.R.

[PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken

[Govindraj]

On Thu, Oct 13, 2011 at 5:17 AM, Kevin Hilman <khilman@ti.com> wrote:
> Govindraj <govindraj.ti@gmail.com> writes:
>
>> On Wed, Oct 12, 2011 at 12:31 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>>
>>>> In suspend path the console_lock is taken by uart_port_suspend
>>>> however when no_console_suspend is used console_lock is not taken.
>>>>
>>>> During system wide suspend omap_pwr_domain hooks cut all
>>>> clocks that are left enabled. So its unsafe to proceed printing after
>>>> clocks are cut by pwr_domain hooks.
>>>
>>> As I've mentioned in previous reviews, when no_console_suspend is
>>> enabled, the user has explicitly requested console output during
>>> suspend. ?In order to support that, we should not be cutting clocks at
>>> all in that mode.
>>>
>>> One way to address this would be to just disable runtime PM in the
>>> ->prepare method of the driver if no_console_suspend is enabled.
>>>
>>
>> Okay fine exploring this option, right API's would be to use
>> pm_runtime_forbid/allow.
>
> Yes.
>
>> <<SNIP>>
>>
>> +static int serial_omap_runtime_prepare(struct device *dev)
>> +{
>> + ? ? ? if (!console_suspend_enabled)
>> + ? ? ? ? ? ? ? pm_runtime_forbid(dev);
>> +
>> + ? ? ? return 0;
>> +}
>> +
>> +static void serial_omap_runtime_complete(struct device *dev)
>> +{
>> + ? ? ? if (!console_suspend_enabled)
>> + ? ? ? ? ? ? ? pm_runtime_allow(dev);
>> +}
>> +
>> ?static const struct dev_pm_ops serial_omap_dev_pm_ops = {
>> ? ? ? ? SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume)
>> ? ? ? ? SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? serial_omap_runtime_resume, NULL)
>> + ? ? ? .prepare = serial_omap_runtime_prepare,
>> + ? ? ? .complete = ?serial_omap_runtime_complete,
>> ?};
>
> OK, but please add comments to these functions about exactly why they
> are needed.
>
>>
>> But to either use runtime forbid or disable we have ensure that
>> power_domain hooks don't go ahead and disable
>> the clocks with omap_device_idle as *runtime forbid or disable will
>> not set runtime_status to RPM_SUSPENDED*
>> and will stay in RPM_ACTIVE if we call runtime disable or forbid from
>> active state.
>
> Correct.
>
>> in power_domain hooks we just check the pm_runtime_status_suspended
>> this will be false even if
>> we do runtime disable/forbid and it will cut uart clocks always.
>>
>> So we may need below check also:
>>
>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> index 26aee5c..286a534 100644
>> --- a/arch/arm/plat-omap/omap_device.c
>> +++ b/arch/arm/plat-omap/omap_device.c
>> @@ -592,7 +592,8 @@ static int _od_suspend_noirq(struct device *dev)
>>
>> ? ? ? ? ret = pm_generic_suspend_noirq(dev);
>>
>> - ? ? ? if (!ret && !pm_runtime_status_suspended(dev)) {
>> + ? ? ? if (!ret && pm_runtime_enabled(dev) &&
>> + ? ? ? ? ? ? ? ? ? ? ? !pm_runtime_status_suspended(dev)) {
>> ? ? ? ? ? ? ? ? if (pm_generic_runtime_suspend(dev) == 0) {
>> ? ? ? ? ? ? ? ? ? ? ? ? omap_device_idle(pdev);
>> ? ? ? ? ? ? ? ? ? ? ? ? od->flags |= OMAP_DEVICE_SUSPENDED;
>
> This isn't right either because devices that may not yet be initialized
> (or loaded) may not have runtime PM enabled, so those devices may not
> be properly idled.
>
> We have an omap_device API to disable this feature at the PM domain level:
> omap_device_disable_idle_on_suspend(). ?All you should have to do is to
> use this API in the device init code on the console UART if
> no_console_suspend has been enabled.


Yes seems okay to me,

Will check if no_console_suspend is used then set
omap_device_disable_idle_on_suspend
in serial.c.

--
Thanks,
Govindraj.R

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Govindraj]

On Thu, Oct 13, 2011 at 5:30 AM, Kevin Hilman <khilman@ti.com> wrote:
> Govindraj <govindraj.ti@gmail.com> writes:
>
>> On Wed, Oct 12, 2011 at 2:36 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>>
>>>> For the early console probing we had avoided hwmod reset and idling
>>>> and uart was idled using hwmod API and enabled back using omap_device API
>>>> after omap_device registration.
>>>>
>>>> Now since we are using runtime API's to enable back uart, move hwmod
>>>> idling and use runtime API to enable back UART.
>>>>
>>>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>>>
>>> Now that the driver is using runtime PM. ?Why do we still need
>>> HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?
>>>
>>> The comment in the code says:
>>>
>>> ? ? ? ? ? ? ? ?/*
>>> ? ? ? ? ? ? ? ? * During UART early init, device need to be probed
>>> ? ? ? ? ? ? ? ? * to determine SoC specific init before omap_device
>>> ? ? ? ? ? ? ? ? * is ready. ?Therefore, don't allow idle here
>>> ? ? ? ? ? ? ? ? */
>>>
>>> This was true when using the 8250 driver because it was not using
>>> runtime PM so could not know how to (re)enable the device.
>>>
>>> However, since the driver is now runtime PM adapted, any device access
>>> should be contained within a runtime PM get/put block, so there should
>>> no longer be a reason not allow the IP blocks to be reset during boot.
>>>
>>
>> Forgot to add, this is still needed for
>> earlyprintk(CONFIG_EARLY_PRINTK) use case,
>
> Ah, right. ?I forgot about that. ?Please update the changelog (and
> comment in the code) to reflect that.
>
>> The initial boot prints until a console driver is available is from
>> "arch/arm/kernel/early_printk.c" which does a tx on uart console
>> and relies on configuration from bootloader.
>>
>> during bootup earlyprink does a tx on uart console and if ?uart driver
>> is not available yet
>> uart reset or idle done by hwmod layer can cause boot failures.
>>
>> --> put_char from earlyprintk.c
>> ? ? ?--> reset/idle from hwmod layer
>> ? ? ? ? ? --> put_char from earlyprintk.c
>>
>>
>> So console_uart reset or clock gating must be done only after uart
>> driver is available or be prevented using these available hwmod_flags.
>
> So why not leave the driver out of it and solve it like the current code
> does?
>
> The current codes use the hwmod flags, then waits until the UART driver
> is available (after omap_device_build) and uses omap_hwmod_idle() to do
> an clean idle of the device.
>
> Notably this is inside a console_lock/console_unlock block so that
> prints are buffered.
>
> The current code then does an omap_device_enable() to re-enable the
> device, but you shouldn't need that after the driver is converted to
> runtime PM.

Yes similar approach here, We are not doing hwmod idle
until console driver is available, once omap-serial is available
from probe doing hwmod_idle* and then get_sync.

hwmod idle in serial.c will still cause problems if ealryprintk tries to print
until omap-uart console driver is not available, as now with rumtime adaptation
only driver can enable back clocks. So have added a function pointer
to pdata which
calls hwmod_idle implemented in serial.c calling omap_hwmod_idle.

--
Thanks,
Govindraj.R

*function pointer implemented in serial.c not directly calling
omap_hmwod_idle API.

[PATCH v6 10/16] OMAP2+: UART: Modify omap_uart_can_sleep function

[Jean Pihet]

Hi Govindraj,

On Thu, Oct 13, 2011 at 3:09 AM, Govindraj <govindraj.ti@gmail.com> wrote:
...
>>
>> Yes, but obviously comes at the expense of power savings. ?IOW, This is
>> a hard-coded power vs. performance trade off that we are trying to get
>> away from.
>>
>> So, the root of the problem is that the MPU wakeup latency is causing a
>> "sluggish" console. ?The solution? ?request an MPU wakeup latency
>> constraint.
>>
>
> Okay, Will explore this.
>
>> This is a classic use-case for such a constraint, and the serial driver
>> should have the option of requesting a constraint to prevent the sluggish
>> console. ?The constraint only needs to be held until the auto-suspend
>> delay expires, so should be relased in the ->runtime_suspend() method of
>> the driver.
>>
>> This constraint needs to be configurable, probably from the board file,
>> so that it is optional, and so users who don't care about sluggish
>> consoles (or non-console UART users who don't care about response time)
>> have the option of preferring power savings over UART responsiveness.
>>
>> As a reference, the i2c driver is currently doing something similar
>> in that it request an MPU constraint to prevent the MPU from going into
>> retention/off while waiting for an i2c interrupt to arrive.
>>
>
> Thanks, will check and try to use the mpu constraints
As a reference the pm-qos branch of
https://gitorious.org/jpihet/omap-pm has the latest code for the
per-device constraints framework.

Please refer to the documentation at Documentation/power/pm_qos_interface.txt.
The commit dbec9ed1 [1] shows how I2C is using the framework.

[1] https://gitorious.org/jpihet/omap-pm/commit/dbec9ed1a6d6341d2ad2352a9578d66d15d198f4

Regards,
Jean

>
> --
> Govindraj.R.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Kevin Hilman]

Govindraj <govindraj.ti@gmail.com> writes:

> On Thu, Oct 13, 2011 at 5:30 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Govindraj <govindraj.ti@gmail.com> writes:
>>
>>> On Wed, Oct 12, 2011 at 2:36 AM, Kevin Hilman <khilman@ti.com> wrote:
>>>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>>>
>>>>> For the early console probing we had avoided hwmod reset and idling
>>>>> and uart was idled using hwmod API and enabled back using omap_device API
>>>>> after omap_device registration.
>>>>>
>>>>> Now since we are using runtime API's to enable back uart, move hwmod
>>>>> idling and use runtime API to enable back UART.
>>>>>
>>>>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>>>>
>>>> Now that the driver is using runtime PM. ?Why do we still need
>>>> HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?
>>>>
>>>> The comment in the code says:
>>>>
>>>> ? ? ? ? ? ? ? ?/*
>>>> ? ? ? ? ? ? ? ? * During UART early init, device need to be probed
>>>> ? ? ? ? ? ? ? ? * to determine SoC specific init before omap_device
>>>> ? ? ? ? ? ? ? ? * is ready. ?Therefore, don't allow idle here
>>>> ? ? ? ? ? ? ? ? */
>>>>
>>>> This was true when using the 8250 driver because it was not using
>>>> runtime PM so could not know how to (re)enable the device.
>>>>
>>>> However, since the driver is now runtime PM adapted, any device access
>>>> should be contained within a runtime PM get/put block, so there should
>>>> no longer be a reason not allow the IP blocks to be reset during boot.
>>>>
>>>
>>> Forgot to add, this is still needed for
>>> earlyprintk(CONFIG_EARLY_PRINTK) use case,
>>
>> Ah, right. ?I forgot about that. ?Please update the changelog (and
>> comment in the code) to reflect that.
>>
>>> The initial boot prints until a console driver is available is from
>>> "arch/arm/kernel/early_printk.c" which does a tx on uart console
>>> and relies on configuration from bootloader.
>>>
>>> during bootup earlyprink does a tx on uart console and if ?uart driver
>>> is not available yet
>>> uart reset or idle done by hwmod layer can cause boot failures.
>>>
>>> --> put_char from earlyprintk.c
>>> ? ? ?--> reset/idle from hwmod layer
>>> ? ? ? ? ? --> put_char from earlyprintk.c
>>>
>>>
>>> So console_uart reset or clock gating must be done only after uart
>>> driver is available or be prevented using these available hwmod_flags.
>>
>> So why not leave the driver out of it and solve it like the current code
>> does?
>>
>> The current codes use the hwmod flags, then waits until the UART driver
>> is available (after omap_device_build) and uses omap_hwmod_idle() to do
>> an clean idle of the device.
>>
>> Notably this is inside a console_lock/console_unlock block so that
>> prints are buffered.
>>
>> The current code then does an omap_device_enable() to re-enable the
>> device, but you shouldn't need that after the driver is converted to
>> runtime PM.
>
> Yes similar approach here, We are not doing hwmod idle
> until console driver is available, once omap-serial is available
> from probe doing hwmod_idle* and then get_sync.

> hwmod idle in serial.c will still cause problems if ealryprintk tries
> to print until omap-uart console driver is not available, 

It will try, but note that current code takes the console_lock() during
that time, so those prints will be buffered.

> as now with rumtime adaptation only driver can enable back clocks.

I'm not sure why you're calling this "enable back clocks."  This patch
is just trying to decide where to idle the hwmod (that is, disable the
clocks.)

Re: only the driver can do it, I can think of at least 2 ways to keep
this out of the driver.

1) Use the a custom activate_func in the omap_device pm_lats struct
   to idle the first time.

2) Use a bus notifier so the device init can be notified when the
   real driver is available.  I think you're probably wanting
   the BUS_NOTIFY_BIND_DRIVER event, which would happen right
   before probe.  There's also BUS_NOTIFY_BOUND_DRIVER which
   happens right after probe.   You might actually want to use
   both.  e.g.   console_lock(); omap_hwmod_idle() in BIND
   and console_unlock() in 'BOUND'.

> So have added a function pointer to pdata which calls hwmod_idle
> implemented in serial.c calling omap_hwmod_idle.

Yes, I saw that in the patch, and that's what I don't like.

Fixing up after earlyprintk is not the responsiblity of the driver, and
the driver should not know or care whether earlyprintk was or wasn't
used before it was loaded.  This needs to be handled in device init code
as it is today.

Kevin

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Govindraj]

On Fri, Oct 14, 2011 at 2:31 AM, Kevin Hilman <khilman@ti.com> wrote:
> Govindraj <govindraj.ti@gmail.com> writes:
>
>> On Thu, Oct 13, 2011 at 5:30 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> Govindraj <govindraj.ti@gmail.com> writes:
>>>
>>>> On Wed, Oct 12, 2011 at 2:36 AM, Kevin Hilman <khilman@ti.com> wrote:
>>>>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>>>>
>>>>>> For the early console probing we had avoided hwmod reset and idling
>>>>>> and uart was idled using hwmod API and enabled back using omap_device API
>>>>>> after omap_device registration.
>>>>>>
>>>>>> Now since we are using runtime API's to enable back uart, move hwmod
>>>>>> idling and use runtime API to enable back UART.
>>>>>>
>>>>>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>>>>>
>>>>> Now that the driver is using runtime PM. ?Why do we still need
>>>>> HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?
>>>>>
>>>>> The comment in the code says:
>>>>>
>>>>> ? ? ? ? ? ? ? ?/*
>>>>> ? ? ? ? ? ? ? ? * During UART early init, device need to be probed
>>>>> ? ? ? ? ? ? ? ? * to determine SoC specific init before omap_device
>>>>> ? ? ? ? ? ? ? ? * is ready. ?Therefore, don't allow idle here
>>>>> ? ? ? ? ? ? ? ? */
>>>>>
>>>>> This was true when using the 8250 driver because it was not using
>>>>> runtime PM so could not know how to (re)enable the device.
>>>>>
>>>>> However, since the driver is now runtime PM adapted, any device access
>>>>> should be contained within a runtime PM get/put block, so there should
>>>>> no longer be a reason not allow the IP blocks to be reset during boot.
>>>>>
>>>>
>>>> Forgot to add, this is still needed for
>>>> earlyprintk(CONFIG_EARLY_PRINTK) use case,
>>>
>>> Ah, right. ?I forgot about that. ?Please update the changelog (and
>>> comment in the code) to reflect that.
>>>
>>>> The initial boot prints until a console driver is available is from
>>>> "arch/arm/kernel/early_printk.c" which does a tx on uart console
>>>> and relies on configuration from bootloader.
>>>>
>>>> during bootup earlyprink does a tx on uart console and if ?uart driver
>>>> is not available yet
>>>> uart reset or idle done by hwmod layer can cause boot failures.
>>>>
>>>> --> put_char from earlyprintk.c
>>>> ? ? ?--> reset/idle from hwmod layer
>>>> ? ? ? ? ? --> put_char from earlyprintk.c
>>>>
>>>>
>>>> So console_uart reset or clock gating must be done only after uart
>>>> driver is available or be prevented using these available hwmod_flags.
>>>
>>> So why not leave the driver out of it and solve it like the current code
>>> does?
>>>
>>> The current codes use the hwmod flags, then waits until the UART driver
>>> is available (after omap_device_build) and uses omap_hwmod_idle() to do
>>> an clean idle of the device.
>>>
>>> Notably this is inside a console_lock/console_unlock block so that
>>> prints are buffered.
>>>
>>> The current code then does an omap_device_enable() to re-enable the
>>> device, but you shouldn't need that after the driver is converted to
>>> runtime PM.
>>
>> Yes similar approach here, We are not doing hwmod idle
>> until console driver is available, once omap-serial is available
>> from probe doing hwmod_idle* and then get_sync.
>
>> hwmod idle in serial.c will still cause problems if ealryprintk tries
>> to print until omap-uart console driver is not available,
>
> It will try, but note that current code takes the console_lock() during
> that time, so those prints will be buffered.
>
>> as now with rumtime adaptation only driver can enable back clocks.
>
> I'm not sure why you're calling this "enable back clocks." ?This patch
> is just trying to decide where to idle the hwmod (that is, disable the
> clocks.)
>

Yes correct, its about where exactly to do hwmod_idle for
avoiding hmwod_reset/idle from boot up.


> Re: only the driver can do it, I can think of at least 2 ways to keep
> this out of the driver.
>

I did some study into the approaches you suggested,
Below are my observation, Please correct me if I missed out
any thing in my below observation,


> 1) Use the a custom activate_func in the omap_device pm_lats struct
> ? to idle the first time.
>

same activate func gets called for all uarts, how to determine the
activate func. was called for first time for the given uart.

(use od->pdev->dev.name strcmp with uart name for first name
and maintain a state machine ? confused)

Then should we have different activate funcs?


> 2) Use a bus notifier so the device init can be notified when the
> ? real driver is available. ?I think you're probably wanting
> ? the BUS_NOTIFY_BIND_DRIVER event, which would happen right
> ? before probe. ?There's also BUS_NOTIFY_BOUND_DRIVER which
> ? happens right after probe. ? You might actually want to use
> ? both. ?e.g. ? console_lock(); omap_hwmod_idle() in BIND
> ? and console_unlock() in 'BOUND'.
>

bus_register_notifier is for all drivers within the bus,
omap-uart is registered as platform bus and there a lot more
devices that register under platform bus,

So registering the notifier for platform bus will call notifier
for all probes withing platform bus,

Is there a way to have notifier per device or given device?

Since bus notifier seems to be notify for all devices
registered within the given bus.

--
Thanks,
Govindraj.R


>> So have added a function pointer to pdata which calls hwmod_idle
>> implemented in serial.c calling omap_hwmod_idle.
>
> Yes, I saw that in the patch, and that's what I don't like.
>
> Fixing up after earlyprintk is not the responsiblity of the driver, and
> the driver should not know or care whether earlyprintk was or wasn't
> used before it was loaded. ?This needs to be handled in device init code
> as it is today.
>
> Kevin
>

[PATCH v6 15/16] OMAP2+: UART: Enable back uart clocks with runtime API for early console

[Kevin Hilman]

Govindraj <govindraj.ti@gmail.com> writes:

[...]

>
> I did some study into the approaches you suggested,
> Below are my observation, Please correct me if I missed out
> any thing in my below observation,
>
>
>> 1) Use the a custom activate_func in the omap_device pm_lats struct
>> ? to idle the first time.
>>
>
> same activate func gets called for all uarts, how to determine the
> activate func. was called for first time for the given uart.
>
> (use od->pdev->dev.name strcmp with uart name for first name
> and maintain a state machine ? confused)
>
> Then should we have different activate funcs?

You would only need to install a custom activate_func for the console
UART device.  After the first call, you could set the activate_func back
to the default.

>> 2) Use a bus notifier so the device init can be notified when the
>> ? real driver is available. ?I think you're probably wanting
>> ? the BUS_NOTIFY_BIND_DRIVER event, which would happen right
>> ? before probe. ?There's also BUS_NOTIFY_BOUND_DRIVER which
>> ? happens right after probe. ? You might actually want to use
>> ? both. ?e.g. ? console_lock(); omap_hwmod_idle() in BIND
>> ? and console_unlock() in 'BOUND'.
>
> bus_register_notifier is for all drivers within the bus,
> omap-uart is registered as platform bus and there a lot more
> devices that register under platform bus,
>
> So registering the notifier for platform bus will call notifier
> for all probes withing platform bus,
>
> Is there a way to have notifier per device or given device?

The notifier callback passes the device pointer.  You just have to take
action for the devices you care about, and return for the others.

Kevin