于 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
