RE: [RFC][PATCH]: Adding support for omap-serail driver

[HU TAO-TGHK48]

I don't  see much added value to use omap_uart_idle_timer() in serial.c since it will be called anyway after timeout.

Is it more simple to do it in omap_uart_prepare_idle()?
Hence omap_uart_prepare_idle() can be move into driver/serial/omap_serial.c eventually.

>  
> +extern int are_driveromap_uarts_active(int *);
> +
>  static void omap_uart_idle_timer(unsigned long data)
>  {
>  	struct omap_uart_state *uart = (struct omap_uart_state *)data;
> +	int dummy;
> +
> +	if (are_driveromap_uarts_active(&dummy)){
> +		/* Keep UART on NoIdle when DMA channel is 
> enabled in omap
> +		 * serial: do not allow UART to goto Smart-idle 
> till its done
> +		 * dma'ing
> +		 */
> +		omap_uart_block_sleep(uart);
> +		return;
> +	}
>  
>  	omap_uart_allow_sleep(uart);
>  }
> 

 

> -----Original Message-----
> From: Pandita, Vikram [mailto:vikram.pandita@xxxxxx] 
> Sent: Tuesday, September 01, 2009 10:58 PM
> To: Govindraj; HU TAO-TGHK48
> Cc: vimal singh; linux-omap@xxxxxxxxxxxxxxx; LKML; 
> linux-serial@xxxxxxxxxxxxxxx; Ye Yuan.Bo-A22116; Chen Xiaolong-A21785
> Subject: RE: [RFC][PATCH]: Adding support for omap-serail driver
> 
> govindraj
> 
> >-----Original Message-----
> >From: linux-omap-owner@xxxxxxxxxxxxxxx 
> >[mailto:linux-omap-owner@xxxxxxxxxxxxxxx] On Behalf Of Govindraj
> >Sent: Tuesday, September 01, 2009 2:14 AM
> >To: HU TAO-TGHK48
> >Cc: vimal singh; linux-omap@xxxxxxxxxxxxxxx; LKML; 
> >linux-serial@xxxxxxxxxxxxxxx; Ye Yuan.Bo-A22116; Chen Xiaolong-A21785
> >Subject: Re: [RFC][PATCH]: Adding support for omap-serail driver
> >
> >On Mon, Aug 31, 2009 at 5:20 PM, HU 
> TAO-TGHK48<taohu@xxxxxxxxxxxx> wrote:
> >>
> >> 1. Shall we cleanup PM related stuff in 
> arch/arm/mach-omap2/serial.c 
> >> as well?
> >>    Originally serail.c register UART IRQ  to decide if 
> UART idle for 
> >> a while and is able to enter low power mode (e.g. retention).
> >>    To work with original 8250 driver, it is probably the only way 
> >> since 8250 is not aware of OMAP PM.
> >>
> >>    However  it would be more reasonable to merge PM stuff to 
> >> omap-serial.c. since the new driver is already OMAP specific
> >>
> >> 2. There is an issue for DMA  with current implementation 
> in serial.c
> >>    When Rx DMA is active NO Rx IRQ will be generated.
> >>    So serial.c will easily set uart->can_sleep with "1" 
> even there is 
> >> Rx DMA ongoing
> >>    +   if ((iir & 0x4) && up->use_dma) {
> >>    +           up->ier &= ~UART_IER_RDI;
> >>    +           serial_out(up, UART_IER, up->ier
> >>
> >>   In my view, the best way is to do the idle detection in 
> >> omap_serial.c.
> >
> >Yes I understand that we cannot adapt 8250 PM model for omap-serial 
> >driver in DMA mode I am currently working on that adaption with dma 
> >mode and will be posting a separate patch for changes on serial.c.
> >
> >Wouldn't it be cleaner to inherit and adapt the Serial-PM framework 
> >from serial.c rather than redefining the PM changes in the driver.
> >
> >As Serial-PM framework already has support for interrupt mode and we 
> >have to adapt the same for DMA  mode.
> >
> >Also defining PM changes in omap-serial would need changes 
> in PM framework.
> >As PM framework calls functions from serail.c file if we are 
> defining 
> >PM framework in our driver then we may need to redefine the 
> functions 
> >as in serial.c or modify the PM-framework for uart-activity check.
> >I feel adapting the existing serial-PM framework for DMA 
> mode would be 
> >better rather than doing these changes.
> 
> You can take reference of how to integrate omap-serial driver 
> PM with mach-omap2/serial.c as follows, for reference --- 
> 
> 
> ---
> From 69112426bd6009cb11e104b9aafcc65429d662f0 Mon Sep 17 00:00:00 2001
> From: Vikram Pandita <vikram.pandita@xxxxxx>
> Date: Fri, 21 Aug 2009 11:15:58 -0500
> Subject: [RFC PATCH] OMAP: Serial: Keep uart in no idle mode 
> when DMA ongoing
> 
> Keep UART in NoIdle mode when DMA is going on.
> 
> Only once UART transfers are complete, switch to smart idle and
> allow OsIdle to kick in
> 
> Signed-off-by: Vikram Pandita <vikram.pandita@xxxxxx>
> ---
>  arch/arm/mach-omap2/serial.c |   12 ++++++++++++
>  drivers/serial/omap-serial.c |    2 +-
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c 
> b/arch/arm/mach-omap2/serial.c
> index ff9beb7..a6ee6ab 100755
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -359,9 +359,21 @@ static void omap_uart_allow_sleep(struct 
> omap_uart_state *uart)
>  	del_timer(&uart->timer);
>  }
>  
> +extern int are_driveromap_uarts_active(int *);
> +
>  static void omap_uart_idle_timer(unsigned long data)
>  {
>  	struct omap_uart_state *uart = (struct omap_uart_state *)data;
> +	int dummy;
> +
> +	if (are_driveromap_uarts_active(&dummy)){
> +		/* Keep UART on NoIdle when DMA channel is 
> enabled in omap
> +		 * serial: do not allow UART to goto Smart-idle 
> till its done
> +		 * dma'ing
> +		 */
> +		omap_uart_block_sleep(uart);
> +		return;
> +	}
>  
>  	omap_uart_allow_sleep(uart);
>  }
> diff --git a/drivers/serial/omap-serial.c 
> b/drivers/serial/omap-serial.c
> index 938f29f..f105e24 100644
> --- a/drivers/serial/omap-serial.c
> +++ b/drivers/serial/omap-serial.c
> @@ -1641,6 +1641,7 @@ int omap24xx_uart_cts_wakeup(int 
> uart_no, int state)
>  	return 0;
>  }
>  EXPORT_SYMBOL(omap24xx_uart_cts_wakeup);
> +#endif
>  /**
>   * are_driver8250_uarts_active() - Check if any ports managed by this
>   * driver are currently busy.  This should be called with interrupts
> @@ -1709,7 +1710,6 @@ int are_driveromap_uarts_active(int 
> *driver8250_managed)
>  }
>  EXPORT_SYMBOL(are_driveromap_uarts_active);
>  
> -#endif
>  
>  static void serial_omap_display_reg(struct uart_port *port)
>  {
> -- 
> 1.6.3.3.334.g916e1
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/