Re: [PATCH 2/5] serial: imx: add DMA support for imx6

[Huang Shijie <b32955@freescale.com>]

于 2013年07月03日 11:38, Shawn Guo 写道:
>> static void imx_enable_dma(struct imx_port *sport)
>> >  +{
>> >  +	unsigned long temp;
>> >  +	struct tty_port *port =&sport->port.state->port;
>> >  +
>> >  +	port->low_latency = 1;
>> >  +	INIT_WORK(&sport->tsk_dma_tx, dma_tx_work);
>> >  +	INIT_WORK(&sport->tsk_dma_rx, dma_rx_work);
>> >  +	init_waitqueue_head(&sport->dma_wait);
> I had a hard time to understand why these work and wait queue are
> necessarily needed here.
Blame it on the sdma driver.

For example, we receive some data in the RXFIFO, normally , the 
interrupt handler will
arise a DMA to fetch the data. We will call the 
dmaengine_prep_slave_sg() to prepare for the DMA,
but sdma will call:
->sdma_prep_slave_sg() -->sdma_load_context() --> sdma_run_channel0()

the sdma_run_channel0() will poll for 500us(we have found the 500us is 
not long enough in some case),
that's why i use the work queue:
We should not delay such long in interrupt context.

The same reason for TX.
> I haven't totally understand it.  But it looks like to me that the
> implementation might be complexer than it needs to be.  Can you please
> take a look at serial-tegra.c to see how the DMA is supported there?
serial-tegra.c arises the dma in the interrupt context. maybe its 
dmaengine is better then the imx-sdma.
> It looks much neater than the changes we have here.
>
>> >  +
>> >  +	/* set UCR1 */
>> >  +	temp = readl(sport->port.membase + UCR1);
>> >  +	temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN |
>> >  +		/* wait for 4 idle frames and enable AGING Timer */
>> >  +		UCR1_ICD_REG(0);
>> >  +	writel(temp, sport->port.membase + UCR1);
>> >  +
>> >  +	/* set UCR4 */
>> >  +	temp = readl(sport->port.membase + UCR4);
>> >  +	temp |= UCR4_IDDMAEN;
>> >  +	writel(temp, sport->port.membase + UCR4);
>> >  +}
>> >  +
>> >  +static void imx_disable_dma(struct imx_port *sport)
>> >  +{
>> >  +	unsigned long temp;
>> >  +	struct tty_port *port =&sport->port.state->port;
>> >  +
>> >  +	/* clear UCR1 */
>> >  +	temp = readl(sport->port.membase + UCR1);
>> >  +	temp&= ~(UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN);
>> >  +	writel(temp, sport->port.membase + UCR1);
>> >  +
>> >  +	/* clear UCR2 */
>> >  +	temp = readl(sport->port.membase + UCR2);
>> >  +	temp&= ~(UCR2_CTSC | UCR2_CTS);
>> >  +	writel(temp, sport->port.membase + UCR2);
>> >  +
>> >  +	/* clear UCR4 */
>> >  +	temp = readl(sport->port.membase + UCR4);
>> >  +	temp&= ~UCR4_IDDMAEN;
>> >  +	writel(temp, sport->port.membase + UCR4);
>> >  +
>> >  +	sport->dma_is_enabled = 0;
>> >  +	sport->dma_is_inited = 0;
> Shouldn't dma_is_inited be reset in imx_uart_dma_exit() for better?
yes. it's ok to set there.
> (I haven't looked at the necessity of these flags.  But please save
> the use of them if we can.)
>
>> >  +
>> >  +	port->low_latency = 0;
>> >  +}
>> >  +
>> >    /* half the RX buffer size */
>> >    #define CTSTL 16
>> >  
>> >  @@ -870,6 +1250,14 @@ static void imx_shutdown(struct uart_port *port)
>> >    	unsigned long temp;
>> >    	unsigned long flags;
>> >  
>> >  +	if (sport->dma_is_enabled) {
>> >  +		/* We have to wait for the DMA to finish. */
>> >  +		wait_event(sport->dma_wait,
>> >  +			!sport->dma_is_rxing&&  !sport->dma_is_txing);
>> >  +		imx_stop_rx(port);
>> >  +		imx_uart_dma_exit(sport);
>> >  +	}
>> >  +
>> >    	spin_lock_irqsave(&sport->port.lock, flags);
>> >    	temp = readl(sport->port.membase + UCR2);
>> >    	temp&= ~(UCR2_TXEN);
>> >  @@ -910,6 +1298,10 @@ static void imx_shutdown(struct uart_port *port)
>> >    		temp&= ~(UCR1_IREN);
>> >  
>> >    	writel(temp, sport->port.membase + UCR1);
>> >  +
>> >  +	if (sport->dma_is_enabled)
>> >  +		imx_disable_dma(sport);
>> >  +
> Shouldn't imx_disable_dma() be called before imx_uart_dma_exit()
> logically?
>
i think it's ok.
I have forgotten why i puted it there. The dma support patch is kept in 
my hand for a long time....

I will try to put the imx_disable_dma() in the imx_uart_dma_exit().
>> >    	spin_unlock_irqrestore(&sport->port.lock, flags);
>> >  
>> >    	clk_disable_unprepare(sport->clk_per);
>> >  @@ -956,6 +1348,13 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
>> >    		if (sport->have_rtscts) {
>> >    			ucr2&= ~UCR2_IRTS;
>> >    			ucr2 |= UCR2_CTSC;
>> >  +
>> >  +			/* Can we enable the DMA support? */
>> >  +			if (is_imx6_uart(sport)&&  !uart_console(port)
>> >  +				&&  !sport->dma_is_inited) {
>> >  +				if (!imx_uart_dma_init(sport))
>> >  +					sport->dma_is_inited = 1;
> I think the setting of the flag can just be handled inside
> imx_uart_dma_init().
>
ok.
>> >  +			}
>> >    		} else {
>> >    			termios->c_cflag&= ~CRTSCTS;
>> >    		}
>> >  @@ -1069,6 +1468,10 @@ imx_set_termios(struct uart_port *port, struct ktermios *termios,
>> >    	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
>> >    		imx_enable_ms(&sport->port);
>> >  
>> >  +	if (sport->dma_is_inited&&  !sport->dma_is_enabled) {
>> >  +		imx_enable_dma(sport);
>> >  +		sport->dma_is_enabled = 1;
> Ditto
>
>> >  +	}
> I see imx_disable_dma() and imx_uart_dma_exit() are called in
> imx_shutdown().  Why can not imx_uart_dma_init() and imx_enable_dma()
> be called in imx_startup()?
i intend to binding the DMA with the RTS/CTS together.

The setting of rts/cts is not in imx_startup(), but in the 
imx_set_termios().




thanks
Huang Shijie



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