[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Santosh Shilimkar]

UART IP idle handling now taken care by runtime pm apis so remove
the hackery from the driver.

Signed-off-by: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/mach-omap2/serial.c     |   31 -------------------------------
 drivers/tty/serial/omap-serial.c |   23 -----------------------
 2 files changed, 54 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 04fdbc4..037e691 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, 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_noidle(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
-
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
-}
-
-static void omap_uart_set_smartidle(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
-	u8 idlemode;
-
-	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
-		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
-	else
-		idlemode = HWMOD_IDLEMODE_SMART;
-
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
-}
-
 #else
 static void omap_uart_enable_wakeup(struct device *dev, bool enable)
 {}
-static void omap_uart_set_noidle(struct device *dev) {}
-static void omap_uart_set_smartidle(struct device *dev) {}
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_OMAP_MUX
@@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
 	omap_up.flags = UPF_BOOT_AUTOCONF;
 	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
-	omap_up.set_forceidle = omap_uart_set_smartidle;
-	omap_up.set_noidle = omap_uart_set_noidle;
 	omap_up.enable_wakeup = omap_uart_enable_wakeup;
 	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
 	omap_up.dma_rx_timeout = info->dma_rx_timeout;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 57d6b29..5722eaf 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
 	return pdata->get_context_loss_count(up->dev);
 }
 
-static void serial_omap_set_forceidle(struct uart_omap_port *up)
-{
-	struct omap_uart_port_info *pdata = up->dev->platform_data;
-
-	if (!pdata || !pdata->set_forceidle)
-		return;
-
-	pdata->set_forceidle(up->dev);
-}
-
-static void serial_omap_set_noidle(struct uart_omap_port *up)
-{
-	struct omap_uart_port_info *pdata = up->dev->platform_data;
-
-	if (!pdata || !pdata->set_noidle)
-		return;
-
-	pdata->set_noidle(up->dev);
-}
-
 static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
 {
 	struct omap_uart_port_info *pdata = up->dev->platform_data;
@@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
 		serial_out(up, UART_IER, up->ier);
 	}
 
-	serial_omap_set_forceidle(up);
-
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 }
@@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
 
 	pm_runtime_get_sync(up->dev);
 	serial_omap_enable_ier_thri(up);
-	serial_omap_set_noidle(up);
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 }
-- 
1.7.9.5

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Santosh Shilimkar]

UART IP idle handling now taken care by runtime pm apis so remove
the hackery from the driver.

Signed-off-by: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/mach-omap2/serial.c     |   31 -------------------------------
 drivers/tty/serial/omap-serial.c |   23 -----------------------
 2 files changed, 54 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 04fdbc4..037e691 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, 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_noidle(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
-
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
-}
-
-static void omap_uart_set_smartidle(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
-	u8 idlemode;
-
-	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
-		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
-	else
-		idlemode = HWMOD_IDLEMODE_SMART;
-
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
-}
-
 #else
 static void omap_uart_enable_wakeup(struct device *dev, bool enable)
 {}
-static void omap_uart_set_noidle(struct device *dev) {}
-static void omap_uart_set_smartidle(struct device *dev) {}
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_OMAP_MUX
@@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
 	omap_up.flags = UPF_BOOT_AUTOCONF;
 	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
-	omap_up.set_forceidle = omap_uart_set_smartidle;
-	omap_up.set_noidle = omap_uart_set_noidle;
 	omap_up.enable_wakeup = omap_uart_enable_wakeup;
 	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
 	omap_up.dma_rx_timeout = info->dma_rx_timeout;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 57d6b29..5722eaf 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
 	return pdata->get_context_loss_count(up->dev);
 }
 
-static void serial_omap_set_forceidle(struct uart_omap_port *up)
-{
-	struct omap_uart_port_info *pdata = up->dev->platform_data;
-
-	if (!pdata || !pdata->set_forceidle)
-		return;
-
-	pdata->set_forceidle(up->dev);
-}
-
-static void serial_omap_set_noidle(struct uart_omap_port *up)
-{
-	struct omap_uart_port_info *pdata = up->dev->platform_data;
-
-	if (!pdata || !pdata->set_noidle)
-		return;
-
-	pdata->set_noidle(up->dev);
-}
-
 static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
 {
 	struct omap_uart_port_info *pdata = up->dev->platform_data;
@@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
 		serial_out(up, UART_IER, up->ier);
 	}
 
-	serial_omap_set_forceidle(up);
-
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 }
@@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
 
 	pm_runtime_get_sync(up->dev);
 	serial_omap_enable_ier_thri(up);
-	serial_omap_set_noidle(up);
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 }
-- 
1.7.9.5

Re: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Felipe Balbi]

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

Hi,

On Fri, Feb 15, 2013 at 05:38:55PM +0530, Santosh Shilimkar wrote:
> UART IP idle handling now taken care by runtime pm apis so remove
> the hackery from the driver.
> 
> Signed-off-by: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  arch/arm/mach-omap2/serial.c     |   31 -------------------------------
>  drivers/tty/serial/omap-serial.c |   23 -----------------------
>  2 files changed, 54 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 04fdbc4..037e691 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, 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_noidle(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct omap_device *od = to_omap_device(pdev);
> -
> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
> -}
> -
> -static void omap_uart_set_smartidle(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct omap_device *od = to_omap_device(pdev);
> -	u8 idlemode;
> -
> -	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
> -		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
> -	else
> -		idlemode = HWMOD_IDLEMODE_SMART;
> -
> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
> -}
> -
>  #else
>  static void omap_uart_enable_wakeup(struct device *dev, bool enable)
>  {}
> -static void omap_uart_set_noidle(struct device *dev) {}
> -static void omap_uart_set_smartidle(struct device *dev) {}
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_OMAP_MUX
> @@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>  	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>  	omap_up.flags = UPF_BOOT_AUTOCONF;
>  	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
> -	omap_up.set_forceidle = omap_uart_set_smartidle;
> -	omap_up.set_noidle = omap_uart_set_noidle;
>  	omap_up.enable_wakeup = omap_uart_enable_wakeup;
>  	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
>  	omap_up.dma_rx_timeout = info->dma_rx_timeout;
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 57d6b29..5722eaf 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
>  	return pdata->get_context_loss_count(up->dev);
>  }
>  
> -static void serial_omap_set_forceidle(struct uart_omap_port *up)
> -{
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> -
> -	if (!pdata || !pdata->set_forceidle)
> -		return;
> -
> -	pdata->set_forceidle(up->dev);
> -}
> -
> -static void serial_omap_set_noidle(struct uart_omap_port *up)
> -{
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> -
> -	if (!pdata || !pdata->set_noidle)
> -		return;
> -
> -	pdata->set_noidle(up->dev);
> -}
> -
>  static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
>  {
>  	struct omap_uart_port_info *pdata = up->dev->platform_data;
> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>  		serial_out(up, UART_IER, up->ier);
>  	}
>  
> -	serial_omap_set_forceidle(up);
> -
>  	pm_runtime_mark_last_busy(up->dev);
>  	pm_runtime_put_autosuspend(up->dev);
>  }
> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>  
>  	pm_runtime_get_sync(up->dev);
>  	serial_omap_enable_ier_thri(up);
> -	serial_omap_set_noidle(up);

this patch is changing behavior. Currently driver has:

start_tx()
get_sync()
set_noidle()
put_autosuspend()

....

stop_tx()
get_sync()
set_force_idle()
put_autosuspend()

with this change, you will have:

start_tx()
get_sync()
set_noidle()
put_autosuspend()
set_force_idle()

this in itself might be enough to go back to corrupted serial if
put_autosuspend is so quick that set_force_idle() is called before all
data has been shifted out of FIFO and through the UART lines.

Before doing this, you should at least test that removing
pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.

Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
there is still the regression of UART never being wakeup capable.

I wonder what are your ideas to sort that part out, I mean, how do you
plan to implement ->set_wake() for the tty port ?

One last comment, to avoid conflicts, it'd be better to split driver
part from removal of platform_data function pointer initialization, so
that we can send driver changes in one merge window and the
platform_data part in another.

-- 
balbi

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

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Felipe Balbi]

Hi,

On Fri, Feb 15, 2013 at 05:38:55PM +0530, Santosh Shilimkar wrote:
> UART IP idle handling now taken care by runtime pm apis so remove
> the hackery from the driver.
> 
> Signed-off-by: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  arch/arm/mach-omap2/serial.c     |   31 -------------------------------
>  drivers/tty/serial/omap-serial.c |   23 -----------------------
>  2 files changed, 54 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 04fdbc4..037e691 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, 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_noidle(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct omap_device *od = to_omap_device(pdev);
> -
> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
> -}
> -
> -static void omap_uart_set_smartidle(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct omap_device *od = to_omap_device(pdev);
> -	u8 idlemode;
> -
> -	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
> -		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
> -	else
> -		idlemode = HWMOD_IDLEMODE_SMART;
> -
> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
> -}
> -
>  #else
>  static void omap_uart_enable_wakeup(struct device *dev, bool enable)
>  {}
> -static void omap_uart_set_noidle(struct device *dev) {}
> -static void omap_uart_set_smartidle(struct device *dev) {}
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_OMAP_MUX
> @@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>  	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>  	omap_up.flags = UPF_BOOT_AUTOCONF;
>  	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
> -	omap_up.set_forceidle = omap_uart_set_smartidle;
> -	omap_up.set_noidle = omap_uart_set_noidle;
>  	omap_up.enable_wakeup = omap_uart_enable_wakeup;
>  	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
>  	omap_up.dma_rx_timeout = info->dma_rx_timeout;
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 57d6b29..5722eaf 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
>  	return pdata->get_context_loss_count(up->dev);
>  }
>  
> -static void serial_omap_set_forceidle(struct uart_omap_port *up)
> -{
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> -
> -	if (!pdata || !pdata->set_forceidle)
> -		return;
> -
> -	pdata->set_forceidle(up->dev);
> -}
> -
> -static void serial_omap_set_noidle(struct uart_omap_port *up)
> -{
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> -
> -	if (!pdata || !pdata->set_noidle)
> -		return;
> -
> -	pdata->set_noidle(up->dev);
> -}
> -
>  static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
>  {
>  	struct omap_uart_port_info *pdata = up->dev->platform_data;
> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>  		serial_out(up, UART_IER, up->ier);
>  	}
>  
> -	serial_omap_set_forceidle(up);
> -
>  	pm_runtime_mark_last_busy(up->dev);
>  	pm_runtime_put_autosuspend(up->dev);
>  }
> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>  
>  	pm_runtime_get_sync(up->dev);
>  	serial_omap_enable_ier_thri(up);
> -	serial_omap_set_noidle(up);

this patch is changing behavior. Currently driver has:

start_tx()
get_sync()
set_noidle()
put_autosuspend()

....

stop_tx()
get_sync()
set_force_idle()
put_autosuspend()

with this change, you will have:

start_tx()
get_sync()
set_noidle()
put_autosuspend()
set_force_idle()

this in itself might be enough to go back to corrupted serial if
put_autosuspend is so quick that set_force_idle() is called before all
data has been shifted out of FIFO and through the UART lines.

Before doing this, you should at least test that removing
pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.

Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
there is still the regression of UART never being wakeup capable.

I wonder what are your ideas to sort that part out, I mean, how do you
plan to implement ->set_wake() for the tty port ?

One last comment, to avoid conflicts, it'd be better to split driver
part from removal of platform_data function pointer initialization, so
that we can send driver changes in one merge window and the
platform_data part in another.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/f3c03cbc/attachment-0001.sig>

Re: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Santosh Shilimkar]

On Friday 15 February 2013 06:27 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 15, 2013 at 05:38:55PM +0530, Santosh Shilimkar wrote:
>> UART IP idle handling now taken care by runtime pm apis so remove
>> the hackery from the driver.
>>
>> Signed-off-by: Rajendra nayak <rnayak@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>   arch/arm/mach-omap2/serial.c     |   31 -------------------------------
>>   drivers/tty/serial/omap-serial.c |   23 -----------------------
>>   2 files changed, 54 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 04fdbc4..037e691 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, 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_noidle(struct device *dev)
>> -{
>> -	struct platform_device *pdev = to_platform_device(dev);
>> -	struct omap_device *od = to_omap_device(pdev);
>> -
>> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
>> -}
>> -
>> -static void omap_uart_set_smartidle(struct device *dev)
>> -{
>> -	struct platform_device *pdev = to_platform_device(dev);
>> -	struct omap_device *od = to_omap_device(pdev);
>> -	u8 idlemode;
>> -
>> -	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
>> -		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
>> -	else
>> -		idlemode = HWMOD_IDLEMODE_SMART;
>> -
>> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
>> -}
>> -
>>   #else
>>   static void omap_uart_enable_wakeup(struct device *dev, bool enable)
>>   {}
>> -static void omap_uart_set_noidle(struct device *dev) {}
>> -static void omap_uart_set_smartidle(struct device *dev) {}
>>   #endif /* CONFIG_PM */
>>
>>   #ifdef CONFIG_OMAP_MUX
>> @@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>>   	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>>   	omap_up.flags = UPF_BOOT_AUTOCONF;
>>   	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
>> -	omap_up.set_forceidle = omap_uart_set_smartidle;
>> -	omap_up.set_noidle = omap_uart_set_noidle;
>>   	omap_up.enable_wakeup = omap_uart_enable_wakeup;
>>   	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
>>   	omap_up.dma_rx_timeout = info->dma_rx_timeout;
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 57d6b29..5722eaf 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
>>   	return pdata->get_context_loss_count(up->dev);
>>   }
>>
>> -static void serial_omap_set_forceidle(struct uart_omap_port *up)
>> -{
>> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
>> -
>> -	if (!pdata || !pdata->set_forceidle)
>> -		return;
>> -
>> -	pdata->set_forceidle(up->dev);
>> -}
>> -
>> -static void serial_omap_set_noidle(struct uart_omap_port *up)
>> -{
>> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
>> -
>> -	if (!pdata || !pdata->set_noidle)
>> -		return;
>> -
>> -	pdata->set_noidle(up->dev);
>> -}
>> -
>>   static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
>>   {
>>   	struct omap_uart_port_info *pdata = up->dev->platform_data;
>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>>   		serial_out(up, UART_IER, up->ier);
>>   	}
>>
>> -	serial_omap_set_forceidle(up);
>> -
>>   	pm_runtime_mark_last_busy(up->dev);
>>   	pm_runtime_put_autosuspend(up->dev);
>>   }
>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>>
>>   	pm_runtime_get_sync(up->dev);
>>   	serial_omap_enable_ier_thri(up);
>> -	serial_omap_set_noidle(up);
>
> this patch is changing behavior. Currently driver has:
>
> start_tx()
> get_sync()
> set_noidle()
> put_autosuspend()
>
> ....
>
> stop_tx()
> get_sync()
> set_force_idle()
> put_autosuspend()
>
> with this change, you will have:
>
> start_tx()
> get_sync()
> set_noidle()
> put_autosuspend()
> set_force_idle()
>
> this in itself might be enough to go back to corrupted serial if
> put_autosuspend is so quick that set_force_idle() is called before all
> data has been shifted out of FIFO and through the UART lines.
>
Really. Infact the old sequence was buggy because you are setting 
force_idle even before suspending the device. And that force idle
isn't really force idle but setting ip to smart idle. Just look at
what serial.c

> Before doing this, you should at least test that removing
> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
>
Seems to work for me with above changes as well. Can you please
try out and see if you see any issue. I doubt you will...

> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> there is still the regression of UART never being wakeup capable.
>
You are mixing the stuff here. The subject is correct.

> I wonder what are your ideas to sort that part out, I mean, how do you
> plan to implement ->set_wake() for the tty port ?
>
That is not related to module idle modes. It is for ioring and that
needs pin control support. As you can see the patch doesn't touch
omap_uart_enable_wakeup(). Thats needs pincontrol support so that
it can be made work with DT. Today hwmod handles that for you with
pin information provided by platform code.

> One last comment, to avoid conflicts, it'd be better to split driver
> part from removal of platform_data function pointer initialization, so
> that we can send driver changes in one merge window and the
> platform_data part in another.
>
Agree. We can split the patch once everybody is ok with it.

Regards
Santosh

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Santosh Shilimkar]

On Friday 15 February 2013 06:27 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 15, 2013 at 05:38:55PM +0530, Santosh Shilimkar wrote:
>> UART IP idle handling now taken care by runtime pm apis so remove
>> the hackery from the driver.
>>
>> Signed-off-by: Rajendra nayak <rnayak@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>   arch/arm/mach-omap2/serial.c     |   31 -------------------------------
>>   drivers/tty/serial/omap-serial.c |   23 -----------------------
>>   2 files changed, 54 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 04fdbc4..037e691 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, 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_noidle(struct device *dev)
>> -{
>> -	struct platform_device *pdev = to_platform_device(dev);
>> -	struct omap_device *od = to_omap_device(pdev);
>> -
>> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
>> -}
>> -
>> -static void omap_uart_set_smartidle(struct device *dev)
>> -{
>> -	struct platform_device *pdev = to_platform_device(dev);
>> -	struct omap_device *od = to_omap_device(pdev);
>> -	u8 idlemode;
>> -
>> -	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
>> -		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
>> -	else
>> -		idlemode = HWMOD_IDLEMODE_SMART;
>> -
>> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
>> -}
>> -
>>   #else
>>   static void omap_uart_enable_wakeup(struct device *dev, bool enable)
>>   {}
>> -static void omap_uart_set_noidle(struct device *dev) {}
>> -static void omap_uart_set_smartidle(struct device *dev) {}
>>   #endif /* CONFIG_PM */
>>
>>   #ifdef CONFIG_OMAP_MUX
>> @@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>>   	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>>   	omap_up.flags = UPF_BOOT_AUTOCONF;
>>   	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
>> -	omap_up.set_forceidle = omap_uart_set_smartidle;
>> -	omap_up.set_noidle = omap_uart_set_noidle;
>>   	omap_up.enable_wakeup = omap_uart_enable_wakeup;
>>   	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
>>   	omap_up.dma_rx_timeout = info->dma_rx_timeout;
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 57d6b29..5722eaf 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
>>   	return pdata->get_context_loss_count(up->dev);
>>   }
>>
>> -static void serial_omap_set_forceidle(struct uart_omap_port *up)
>> -{
>> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
>> -
>> -	if (!pdata || !pdata->set_forceidle)
>> -		return;
>> -
>> -	pdata->set_forceidle(up->dev);
>> -}
>> -
>> -static void serial_omap_set_noidle(struct uart_omap_port *up)
>> -{
>> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
>> -
>> -	if (!pdata || !pdata->set_noidle)
>> -		return;
>> -
>> -	pdata->set_noidle(up->dev);
>> -}
>> -
>>   static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
>>   {
>>   	struct omap_uart_port_info *pdata = up->dev->platform_data;
>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>>   		serial_out(up, UART_IER, up->ier);
>>   	}
>>
>> -	serial_omap_set_forceidle(up);
>> -
>>   	pm_runtime_mark_last_busy(up->dev);
>>   	pm_runtime_put_autosuspend(up->dev);
>>   }
>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>>
>>   	pm_runtime_get_sync(up->dev);
>>   	serial_omap_enable_ier_thri(up);
>> -	serial_omap_set_noidle(up);
>
> this patch is changing behavior. Currently driver has:
>
> start_tx()
> get_sync()
> set_noidle()
> put_autosuspend()
>
> ....
>
> stop_tx()
> get_sync()
> set_force_idle()
> put_autosuspend()
>
> with this change, you will have:
>
> start_tx()
> get_sync()
> set_noidle()
> put_autosuspend()
> set_force_idle()
>
> this in itself might be enough to go back to corrupted serial if
> put_autosuspend is so quick that set_force_idle() is called before all
> data has been shifted out of FIFO and through the UART lines.
>
Really. Infact the old sequence was buggy because you are setting 
force_idle even before suspending the device. And that force idle
isn't really force idle but setting ip to smart idle. Just look at
what serial.c

> Before doing this, you should at least test that removing
> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
>
Seems to work for me with above changes as well. Can you please
try out and see if you see any issue. I doubt you will...

> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> there is still the regression of UART never being wakeup capable.
>
You are mixing the stuff here. The subject is correct.

> I wonder what are your ideas to sort that part out, I mean, how do you
> plan to implement ->set_wake() for the tty port ?
>
That is not related to module idle modes. It is for ioring and that
needs pin control support. As you can see the patch doesn't touch
omap_uart_enable_wakeup(). Thats needs pincontrol support so that
it can be made work with DT. Today hwmod handles that for you with
pin information provided by platform code.

> One last comment, to avoid conflicts, it'd be better to split driver
> part from removal of platform_data function pointer initialization, so
> that we can send driver changes in one merge window and the
> platform_data part in another.
>
Agree. We can split the patch once everybody is ok with it.

Regards
Santosh

Re: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Felipe Balbi]

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

Hi,

On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
> >>@@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
> >>  		serial_out(up, UART_IER, up->ier);
> >>  	}
> >>
> >>-	serial_omap_set_forceidle(up);
> >>-
> >>  	pm_runtime_mark_last_busy(up->dev);
> >>  	pm_runtime_put_autosuspend(up->dev);
> >>  }
> >>@@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
> >>
> >>  	pm_runtime_get_sync(up->dev);
> >>  	serial_omap_enable_ier_thri(up);
> >>-	serial_omap_set_noidle(up);
> >
> >this patch is changing behavior. Currently driver has:
> >
> >start_tx()
> >get_sync()
> >set_noidle()
> >put_autosuspend()
> >
> >....
> >
> >stop_tx()
> >get_sync()
> >set_force_idle()
> >put_autosuspend()
> >
> >with this change, you will have:
> >
> >start_tx()
> >get_sync()
> >set_noidle()
> >put_autosuspend()
> >set_force_idle()
> >
> >this in itself might be enough to go back to corrupted serial if
> >put_autosuspend is so quick that set_force_idle() is called before all
> >data has been shifted out of FIFO and through the UART lines.
> >
> Really. Infact the old sequence was buggy because you are setting
> force_idle even before suspending the device. And that force idle

then that bug has to be fixed first and patch needs to be sent to stable
before we change that part of the code, wouldn't you agree ?

> isn't really force idle but setting ip to smart idle. Just look at
> what serial.c

indeed.

> >Before doing this, you should at least test that removing
> >pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> >start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
> >
> Seems to work for me with above changes as well. Can you please
> try out and see if you see any issue. I doubt you will...

what I'm saying is that current code, as you put it yourself, is buggy,
so we ought to fix the bugs first before changing behavior. If not for
anything else, at least to have a clean tree which we can bisect.

> >Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> >there is still the regression of UART never being wakeup capable.
> >
> You are mixing the stuff here. The subject is correct.

->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
all.

> >I wonder what are your ideas to sort that part out, I mean, how do you
> >plan to implement ->set_wake() for the tty port ?
> >
> That is not related to module idle modes. It is for ioring and that
> needs pin control support. As you can see the patch doesn't touch
> omap_uart_enable_wakeup(). Thats needs pincontrol support so that

see above.

-- 
balbi

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

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Felipe Balbi]

Hi,

On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
> >>@@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
> >>  		serial_out(up, UART_IER, up->ier);
> >>  	}
> >>
> >>-	serial_omap_set_forceidle(up);
> >>-
> >>  	pm_runtime_mark_last_busy(up->dev);
> >>  	pm_runtime_put_autosuspend(up->dev);
> >>  }
> >>@@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
> >>
> >>  	pm_runtime_get_sync(up->dev);
> >>  	serial_omap_enable_ier_thri(up);
> >>-	serial_omap_set_noidle(up);
> >
> >this patch is changing behavior. Currently driver has:
> >
> >start_tx()
> >get_sync()
> >set_noidle()
> >put_autosuspend()
> >
> >....
> >
> >stop_tx()
> >get_sync()
> >set_force_idle()
> >put_autosuspend()
> >
> >with this change, you will have:
> >
> >start_tx()
> >get_sync()
> >set_noidle()
> >put_autosuspend()
> >set_force_idle()
> >
> >this in itself might be enough to go back to corrupted serial if
> >put_autosuspend is so quick that set_force_idle() is called before all
> >data has been shifted out of FIFO and through the UART lines.
> >
> Really. Infact the old sequence was buggy because you are setting
> force_idle even before suspending the device. And that force idle

then that bug has to be fixed first and patch needs to be sent to stable
before we change that part of the code, wouldn't you agree ?

> isn't really force idle but setting ip to smart idle. Just look at
> what serial.c

indeed.

> >Before doing this, you should at least test that removing
> >pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> >start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
> >
> Seems to work for me with above changes as well. Can you please
> try out and see if you see any issue. I doubt you will...

what I'm saying is that current code, as you put it yourself, is buggy,
so we ought to fix the bugs first before changing behavior. If not for
anything else, at least to have a clean tree which we can bisect.

> >Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> >there is still the regression of UART never being wakeup capable.
> >
> You are mixing the stuff here. The subject is correct.

->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
all.

> >I wonder what are your ideas to sort that part out, I mean, how do you
> >plan to implement ->set_wake() for the tty port ?
> >
> That is not related to module idle modes. It is for ioring and that
> needs pin control support. As you can see the patch doesn't touch
> omap_uart_enable_wakeup(). Thats needs pincontrol support so that

see above.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/889f9422/attachment-0001.sig>

Re: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Santosh Shilimkar]

On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
>>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>>>>   		serial_out(up, UART_IER, up->ier);
>>>>   	}
>>>>
>>>> -	serial_omap_set_forceidle(up);
>>>> -
>>>>   	pm_runtime_mark_last_busy(up->dev);
>>>>   	pm_runtime_put_autosuspend(up->dev);
>>>>   }
>>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>>>>
>>>>   	pm_runtime_get_sync(up->dev);
>>>>   	serial_omap_enable_ier_thri(up);
>>>> -	serial_omap_set_noidle(up);
>>>
>>> this patch is changing behavior. Currently driver has:
>>>
>>> start_tx()
>>> get_sync()
>>> set_noidle()
>>> put_autosuspend()
>>>
>>> ....
>>>
>>> stop_tx()
>>> get_sync()
>>> set_force_idle()
>>> put_autosuspend()
>>>
>>> with this change, you will have:
>>>
>>> start_tx()
>>> get_sync()
>>> set_noidle()
>>> put_autosuspend()
>>> set_force_idle()
>>>
>>> this in itself might be enough to go back to corrupted serial if
>>> put_autosuspend is so quick that set_force_idle() is called before all
>>> data has been shifted out of FIFO and through the UART lines.
>>>
>> Really. Infact the old sequence was buggy because you are setting
>> force_idle even before suspending the device. And that force idle
>
> then that bug has to be fixed first and patch needs to be sent to stable
> before we change that part of the code, wouldn't you agree ?
>
Agree. Will be good to get that fixed for stable. Probably Sourabh
can fix that one.

>> isn't really force idle but setting ip to smart idle. Just look at
>> what serial.c
>
> indeed.
>
>>> Before doing this, you should at least test that removing
>>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
>>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
>>>
>> Seems to work for me with above changes as well. Can you please
>> try out and see if you see any issue. I doubt you will...
>
> what I'm saying is that current code, as you put it yourself, is buggy,
> so we ought to fix the bugs first before changing behavior. If not for
> anything else, at least to have a clean tree which we can bisect.
>
>>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
>>> there is still the regression of UART never being wakeup capable.
>>>
>> You are mixing the stuff here. The subject is correct.
>
> ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
> SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
> all.
>
But SYSC is already in smart_idle_wakeup() mode. I get your point
though. The main purpose of that wakeup hook is to trigger io_ring
and pad wakeup.

BTW, Rajendra is looking at how to get rid of wakeup function pointer
as well since that also comes in way for DT.

Regards
Santosh

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Santosh Shilimkar]

On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
>>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>>>>   		serial_out(up, UART_IER, up->ier);
>>>>   	}
>>>>
>>>> -	serial_omap_set_forceidle(up);
>>>> -
>>>>   	pm_runtime_mark_last_busy(up->dev);
>>>>   	pm_runtime_put_autosuspend(up->dev);
>>>>   }
>>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>>>>
>>>>   	pm_runtime_get_sync(up->dev);
>>>>   	serial_omap_enable_ier_thri(up);
>>>> -	serial_omap_set_noidle(up);
>>>
>>> this patch is changing behavior. Currently driver has:
>>>
>>> start_tx()
>>> get_sync()
>>> set_noidle()
>>> put_autosuspend()
>>>
>>> ....
>>>
>>> stop_tx()
>>> get_sync()
>>> set_force_idle()
>>> put_autosuspend()
>>>
>>> with this change, you will have:
>>>
>>> start_tx()
>>> get_sync()
>>> set_noidle()
>>> put_autosuspend()
>>> set_force_idle()
>>>
>>> this in itself might be enough to go back to corrupted serial if
>>> put_autosuspend is so quick that set_force_idle() is called before all
>>> data has been shifted out of FIFO and through the UART lines.
>>>
>> Really. Infact the old sequence was buggy because you are setting
>> force_idle even before suspending the device. And that force idle
>
> then that bug has to be fixed first and patch needs to be sent to stable
> before we change that part of the code, wouldn't you agree ?
>
Agree. Will be good to get that fixed for stable. Probably Sourabh
can fix that one.

>> isn't really force idle but setting ip to smart idle. Just look at
>> what serial.c
>
> indeed.
>
>>> Before doing this, you should at least test that removing
>>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
>>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
>>>
>> Seems to work for me with above changes as well. Can you please
>> try out and see if you see any issue. I doubt you will...
>
> what I'm saying is that current code, as you put it yourself, is buggy,
> so we ought to fix the bugs first before changing behavior. If not for
> anything else, at least to have a clean tree which we can bisect.
>
>>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
>>> there is still the regression of UART never being wakeup capable.
>>>
>> You are mixing the stuff here. The subject is correct.
>
> ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
> SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
> all.
>
But SYSC is already in smart_idle_wakeup() mode. I get your point
though. The main purpose of that wakeup hook is to trigger io_ring
and pad wakeup.

BTW, Rajendra is looking at how to get rid of wakeup function pointer
as well since that also comes in way for DT.

Regards
Santosh

Re: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[a0131647]

Hi,
On Friday 15 February 2013 07:13 PM, Santosh Shilimkar wrote:
> On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
>> Hi,
>>
>> On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
>>>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct 
>>>>> uart_port *port)
>>>>>           serial_out(up, UART_IER, up->ier);
>>>>>       }
>>>>>
>>>>> -    serial_omap_set_forceidle(up);
>>>>> -
>>>>>       pm_runtime_mark_last_busy(up->dev);
>>>>>       pm_runtime_put_autosuspend(up->dev);
>>>>>   }
>>>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct 
>>>>> uart_port *port)
>>>>>
>>>>>       pm_runtime_get_sync(up->dev);
>>>>>       serial_omap_enable_ier_thri(up);
>>>>> -    serial_omap_set_noidle(up);
>>>>
>>>> this patch is changing behavior. Currently driver has:
>>>>
>>>> start_tx()
>>>> get_sync()
>>>> set_noidle()
>>>> put_autosuspend()
>>>>
>>>> ....
>>>>
>>>> stop_tx()
>>>> get_sync()
>>>> set_force_idle()
>>>> put_autosuspend()
>>>>
>>>> with this change, you will have:
>>>>
>>>> start_tx()
>>>> get_sync()
>>>> set_noidle()
>>>> put_autosuspend()
>>>> set_force_idle()
>>>>
>>>> this in itself might be enough to go back to corrupted serial if
>>>> put_autosuspend is so quick that set_force_idle() is called before all
>>>> data has been shifted out of FIFO and through the UART lines.
>>>>
>>> Really. Infact the old sequence was buggy because you are setting
>>> force_idle even before suspending the device. And that force idle
>>
>> then that bug has to be fixed first and patch needs to be sent to stable
>> before we change that part of the code, wouldn't you agree ?
>>
> Agree. Will be good to get that fixed for stable. Probably Sourabh
> can fix that one.
>
Yes, will send a patch fix  for this.
>>> isn't really force idle but setting ip to smart idle. Just look at
>>> what serial.c
>>
>> indeed.
>>
>>>> Before doing this, you should at least test that removing
>>>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
>>>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works 
>>>> fine.
>>>>
>>> Seems to work for me with above changes as well. Can you please
>>> try out and see if you see any issue. I doubt you will...
>>
>> what I'm saying is that current code, as you put it yourself, is buggy,
>> so we ought to fix the bugs first before changing behavior. If not for
>> anything else, at least to have a clean tree which we can bisect.
>>
>>>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
>>>> there is still the regression of UART never being wakeup capable.
>>>>
>>> You are mixing the stuff here. The subject is correct.
>>
>> ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
>> SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
>> all.
>>
> But SYSC is already in smart_idle_wakeup() mode. I get your point
> though. The main purpose of that wakeup hook is to trigger io_ring
> and pad wakeup.
>
> BTW, Rajendra is looking at how to get rid of wakeup function pointer
> as well since that also comes in way for DT.
>
> Regards
> Santosh
>
>
~Sourav

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[a0131647]

Hi,
On Friday 15 February 2013 07:13 PM, Santosh Shilimkar wrote:
> On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
>> Hi,
>>
>> On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
>>>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct 
>>>>> uart_port *port)
>>>>>           serial_out(up, UART_IER, up->ier);
>>>>>       }
>>>>>
>>>>> -    serial_omap_set_forceidle(up);
>>>>> -
>>>>>       pm_runtime_mark_last_busy(up->dev);
>>>>>       pm_runtime_put_autosuspend(up->dev);
>>>>>   }
>>>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct 
>>>>> uart_port *port)
>>>>>
>>>>>       pm_runtime_get_sync(up->dev);
>>>>>       serial_omap_enable_ier_thri(up);
>>>>> -    serial_omap_set_noidle(up);
>>>>
>>>> this patch is changing behavior. Currently driver has:
>>>>
>>>> start_tx()
>>>> get_sync()
>>>> set_noidle()
>>>> put_autosuspend()
>>>>
>>>> ....
>>>>
>>>> stop_tx()
>>>> get_sync()
>>>> set_force_idle()
>>>> put_autosuspend()
>>>>
>>>> with this change, you will have:
>>>>
>>>> start_tx()
>>>> get_sync()
>>>> set_noidle()
>>>> put_autosuspend()
>>>> set_force_idle()
>>>>
>>>> this in itself might be enough to go back to corrupted serial if
>>>> put_autosuspend is so quick that set_force_idle() is called before all
>>>> data has been shifted out of FIFO and through the UART lines.
>>>>
>>> Really. Infact the old sequence was buggy because you are setting
>>> force_idle even before suspending the device. And that force idle
>>
>> then that bug has to be fixed first and patch needs to be sent to stable
>> before we change that part of the code, wouldn't you agree ?
>>
> Agree. Will be good to get that fixed for stable. Probably Sourabh
> can fix that one.
>
Yes, will send a patch fix  for this.
>>> isn't really force idle but setting ip to smart idle. Just look at
>>> what serial.c
>>
>> indeed.
>>
>>>> Before doing this, you should at least test that removing
>>>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
>>>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works 
>>>> fine.
>>>>
>>> Seems to work for me with above changes as well. Can you please
>>> try out and see if you see any issue. I doubt you will...
>>
>> what I'm saying is that current code, as you put it yourself, is buggy,
>> so we ought to fix the bugs first before changing behavior. If not for
>> anything else, at least to have a clean tree which we can bisect.
>>
>>>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
>>>> there is still the regression of UART never being wakeup capable.
>>>>
>>> You are mixing the stuff here. The subject is correct.
>>
>> ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
>> SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
>> all.
>>
> But SYSC is already in smart_idle_wakeup() mode. I get your point
> though. The main purpose of that wakeup hook is to trigger io_ring
> and pad wakeup.
>
> BTW, Rajendra is looking at how to get rid of wakeup function pointer
> as well since that also comes in way for DT.
>
> Regards
> Santosh
>
>
~Sourav

RE: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Bedia, Vaibhav]

Hi,

On Fri, Feb 15, 2013 at 19:13:42, Shilimkar, Santosh wrote:
> On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
> >>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
> >>>>   		serial_out(up, UART_IER, up->ier);
> >>>>   	}
> >>>>
> >>>> -	serial_omap_set_forceidle(up);
> >>>> -
> >>>>   	pm_runtime_mark_last_busy(up->dev);
> >>>>   	pm_runtime_put_autosuspend(up->dev);
> >>>>   }
> >>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
> >>>>
> >>>>   	pm_runtime_get_sync(up->dev);
> >>>>   	serial_omap_enable_ier_thri(up);
> >>>> -	serial_omap_set_noidle(up);
> >>>
> >>> this patch is changing behavior. Currently driver has:
> >>>
> >>> start_tx()
> >>> get_sync()
> >>> set_noidle()
> >>> put_autosuspend()
> >>>
> >>> ....
> >>>
> >>> stop_tx()
> >>> get_sync()
> >>> set_force_idle()
> >>> put_autosuspend()
> >>>
> >>> with this change, you will have:
> >>>
> >>> start_tx()
> >>> get_sync()
> >>> set_noidle()
> >>> put_autosuspend()
> >>> set_force_idle()
> >>>
> >>> this in itself might be enough to go back to corrupted serial if
> >>> put_autosuspend is so quick that set_force_idle() is called before all
> >>> data has been shifted out of FIFO and through the UART lines.
> >>>
> >> Really. Infact the old sequence was buggy because you are setting
> >> force_idle even before suspending the device. And that force idle
> >
> > then that bug has to be fixed first and patch needs to be sent to stable
> > before we change that part of the code, wouldn't you agree ?
> >
> Agree. Will be good to get that fixed for stable. Probably Sourabh
> can fix that one.
> 
> >> isn't really force idle but setting ip to smart idle. Just look at
> >> what serial.c
> >
> > indeed.
> >
> >>> Before doing this, you should at least test that removing
> >>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> >>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
> >>>
> >> Seems to work for me with above changes as well. Can you please
> >> try out and see if you see any issue. I doubt you will...
> >
> > what I'm saying is that current code, as you put it yourself, is buggy,
> > so we ought to fix the bugs first before changing behavior. If not for
> > anything else, at least to have a clean tree which we can bisect.
> >
> >>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> >>> there is still the regression of UART never being wakeup capable.
> >>>
> >> You are mixing the stuff here. The subject is correct.
> >
> > ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
> > SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
> > all.
> >
> But SYSC is already in smart_idle_wakeup() mode. I get your point
> though. The main purpose of that wakeup hook is to trigger io_ring
> and pad wakeup.
> 
> BTW, Rajendra is looking at how to get rid of wakeup function pointer
> as well since that also comes in way for DT.
> 

With these changes the async wakeup mechanism for UART which depends on
SIDLE bits being set to 0x3 and ENWAKEUP being set to 0x1 breaks. I noticed
this while testing these changes on top of the AM335x suspend-resume support.

How about leveraging the generic wakeup flag for all devices to get the
required functionality for wakeup? A call to device_init_wakeup() in probe,
followed by a check for device_may_wakeup() in the driver's suspend routine
can be used to have omap_device_idle_hwmods() configure the SIDLE bits
appropriately. This should help configure SIDLE to FORCE/NO_IDLE during active
mode along with the appropriate SYSC configuration during suspend. The
device_may_wakeup() check could even be used to enable IO Daisy chaining
feature for the IOs.

Regards,
Vaibhav

[RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[Bedia, Vaibhav]

Hi,

On Fri, Feb 15, 2013 at 19:13:42, Shilimkar, Santosh wrote:
> On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
> >>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
> >>>>   		serial_out(up, UART_IER, up->ier);
> >>>>   	}
> >>>>
> >>>> -	serial_omap_set_forceidle(up);
> >>>> -
> >>>>   	pm_runtime_mark_last_busy(up->dev);
> >>>>   	pm_runtime_put_autosuspend(up->dev);
> >>>>   }
> >>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
> >>>>
> >>>>   	pm_runtime_get_sync(up->dev);
> >>>>   	serial_omap_enable_ier_thri(up);
> >>>> -	serial_omap_set_noidle(up);
> >>>
> >>> this patch is changing behavior. Currently driver has:
> >>>
> >>> start_tx()
> >>> get_sync()
> >>> set_noidle()
> >>> put_autosuspend()
> >>>
> >>> ....
> >>>
> >>> stop_tx()
> >>> get_sync()
> >>> set_force_idle()
> >>> put_autosuspend()
> >>>
> >>> with this change, you will have:
> >>>
> >>> start_tx()
> >>> get_sync()
> >>> set_noidle()
> >>> put_autosuspend()
> >>> set_force_idle()
> >>>
> >>> this in itself might be enough to go back to corrupted serial if
> >>> put_autosuspend is so quick that set_force_idle() is called before all
> >>> data has been shifted out of FIFO and through the UART lines.
> >>>
> >> Really. Infact the old sequence was buggy because you are setting
> >> force_idle even before suspending the device. And that force idle
> >
> > then that bug has to be fixed first and patch needs to be sent to stable
> > before we change that part of the code, wouldn't you agree ?
> >
> Agree. Will be good to get that fixed for stable. Probably Sourabh
> can fix that one.
> 
> >> isn't really force idle but setting ip to smart idle. Just look at
> >> what serial.c
> >
> > indeed.
> >
> >>> Before doing this, you should at least test that removing
> >>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> >>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
> >>>
> >> Seems to work for me with above changes as well. Can you please
> >> try out and see if you see any issue. I doubt you will...
> >
> > what I'm saying is that current code, as you put it yourself, is buggy,
> > so we ought to fix the bugs first before changing behavior. If not for
> > anything else, at least to have a clean tree which we can bisect.
> >
> >>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> >>> there is still the regression of UART never being wakeup capable.
> >>>
> >> You are mixing the stuff here. The subject is correct.
> >
> > ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
> > SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
> > all.
> >
> But SYSC is already in smart_idle_wakeup() mode. I get your point
> though. The main purpose of that wakeup hook is to trigger io_ring
> and pad wakeup.
> 
> BTW, Rajendra is looking at how to get rid of wakeup function pointer
> as well since that also comes in way for DT.
> 

With these changes the async wakeup mechanism for UART which depends on
SIDLE bits being set to 0x3 and ENWAKEUP being set to 0x1 breaks. I noticed
this while testing these changes on top of the AM335x suspend-resume support.

How about leveraging the generic wakeup flag for all devices to get the
required functionality for wakeup? A call to device_init_wakeup() in probe,
followed by a check for device_may_wakeup() in the driver's suspend routine
can be used to have omap_device_idle_hwmods() configure the SIDLE bits
appropriately. This should help configure SIDLE to FORCE/NO_IDLE during active
mode along with the appropriate SYSC configuration during suspend. The
device_may_wakeup() check could even be used to enable IO Daisy chaining
feature for the IOs.

Regards,
Vaibhav