Am Montag, den 10.08.2015, 19:07 +0900 schrieb Jiada Wang:
> Hi Lucas
>
>
> On 08/06/2015 01:03 AM, Lucas Stach wrote:
> > Triggering the DMA engine for every byte is horribly inefficient.
> > Also it doesn't allow to use the aging timer for the RX FIFO as this
> > requires the DMA engine to leave one byte remaining in the FIFO when
> > doing a normal burst transfer.
> >
> > Adjust watermark levels so that the DMA engine can do at least 8 byte
> > burst transfers. This is a conservative value, as the both TX and RX
> > FIFOs are able to contain 32 bytes.
> >
> > Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > ---
> > drivers/tty/serial/imx.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 7bb683264013..363c82dcfbb7 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -971,6 +971,8 @@ static int start_rx_dma(struct imx_port *sport)
> >
> > #define TXTL_DEFAULT 2 /* reset default */
> > #define RXTL_DEFAULT 1 /* reset default */
> > +#define TXTL_DMA 8 /* DMA burst setting */
> > +#define RXTL_DMA 9 /* DMA burst setting */
> >
> > static void imx_setup_ufcr(struct imx_port *sport,
> > unsigned char txwl, unsigned char rxwl)
> > @@ -1018,7 +1020,8 @@ static int imx_uart_dma_init(struct imx_port *sport)
> > slave_config.direction = DMA_DEV_TO_MEM;
> > slave_config.src_addr = sport->port.mapbase + URXD0;
> > slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> > - slave_config.src_maxburst = RXTL_DEFAULT;
> > + /* one byte less than the watermark level to enable the aging timer */
> > + slave_config.src_maxburst = RXTL_DMA - 1;
> > ret = dmaengine_slave_config(sport->dma_chan_rx, &slave_config);
> > if (ret) {
> > dev_err(dev, "error in RX dma configuration.\n");
> > @@ -1042,7 +1045,7 @@ static int imx_uart_dma_init(struct imx_port *sport)
> > slave_config.direction = DMA_MEM_TO_DEV;
> > slave_config.dst_addr = sport->port.mapbase + URTX0;
> > slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> > - slave_config.dst_maxburst = TXTL_DEFAULT;
> > + slave_config.dst_maxburst = TXTL_DMA;
> > ret = dmaengine_slave_config(sport->dma_chan_tx, &slave_config);
> > if (ret) {
> > dev_err(dev, "error in TX dma configuration.");
> > @@ -1079,6 +1082,8 @@ static void imx_enable_dma(struct imx_port *sport)
> > temp |= UCR4_IDDMAEN;
> > writel(temp, sport->port.membase + UCR4);
> >
> > + imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
> > +
> > sport->dma_is_enabled = 1;
> > }
> >
> > @@ -1101,6 +1106,8 @@ static void imx_disable_dma(struct imx_port *sport)
> > temp &= ~UCR4_IDDMAEN;
> > writel(temp, sport->port.membase + UCR4);
> >
> > + imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
> > +
> > sport->dma_is_enabled = 0;
> > }
> >
> >
> How about make watermark value configurable via DTS?
>
> For some specific user case & HW, user may want to have higer rx
> watermark value to improve performance more,
> for example in case HW flow control is available, then it's safe to have
> higher rx watermark value (by set CTS trigger level properly)
> if there is no specific setting in DTS, then we can use default value
> RXTL_DMA/TXTL_DMA
>
> What do you think?
I agree that this is something that we might want to do. But it can
easily be done in a follow on patch after this series. I don't want to
delay this series (that fixes real bugs in the DMA implementation) by
the discussions needed when changing the DT bindings.
BTW: Did you have a chance to do some more testing? If yes I would
appreciate a formal tested-by for the series. I'll repost this series in
the coming days with your last comments fixed.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
