Hi, Arnd Thanks for your suggestion. > -----Original Message----- > From: linux-serial-owner@xxxxxxxxxxxxxxx [mailto:linux-serial- > owner@xxxxxxxxxxxxxxx] On Behalf Of Arnd Bergmann > Sent: Wednesday, January 15, 2014 5:26 PM > To: Yuan Yao-B46683 > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; shawn.guo@xxxxxxxxxx; > linux@xxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > serial@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v3 2/2] serial: fsl_lpuart: add DMA support > > On Wednesday 15 January 2014, Yuan Yao wrote: > > Add dma support for lpuart. This function depend on DMA driver. > > You can turn on it by SERIAL_FSL_LPUART_DMA=y. And It works if dts node > has dma properties. > > > > Signed-off-by: Yuan Yao <yao.yuan@xxxxxxxxxxxxx> > > Most of the changes you did look good, but I have to say I'm not a fan of > the #ifdef you added. I don't think you actually need to have the > compile-time selection, and it would be best to just drop that part. > > If you have a good reason to keep it, please change the code to more > readable "if (IS_ENABLED(CONFIG_FSL_LPUART_DMA))" constructs. > If you decide to do this, you will find that you need far fewer such > conditionals because a lot of the code will automatically get dropped by > the compiler. > Thank you! I even not know the IS_ENABLED macro. At the beginning I just think this can save the size of code. But now I think it's not very necessary. I tend to cancel it. > > uart0: serial@40027000 { > > - compatible = "fsl,vf610-lpuart"; > > - reg = <0x40027000 0x1000>; > > - interrupts = <0 61 0x00>; > > - }; > > + compatible = "fsl,vf610-lpuart"; > > + reg = <0x40027000 0x1000>; > > + interrupts = <0 61 0x00>; > > + clocks = <&clks VF610_CLK_UART0>; > > + clock-names = "ipg"; > > + dma-names = "lpuart-tx","lpuart-rx"; > > + dmas = <&edma0 0 VF610_EDMA_MUXID0_UART0_TX>, > > + <&edma0 0 VF610_EDMA_MUXID0_UART0_RX>; > > + }; > > You still haven't addressed my comment about removing the > VF610_EDMA_MUXID0_UART0_TX macros. > Very sorry for forget it. At that time the VF610_EDMA_MUXID0_UART0_TX is because of eDMA driver request. But now it also cancel the macro. So this is my fault. I will fix it next. > > > > +#ifdef CONFIG_SERIAL_FSL_LPUART_DMA > > +#define DMA_MAXBURST 16 > > +#define DMA_MAXBURST_MASK (DMA_MAXBURST - 1) > > +#define FSL_UART_RX_DMA_BUFFER_SIZE 64 > > +#endif > > For this part there was no need for the #ifdef to start with. > > > #define DRIVER_NAME "fsl-lpuart" > > #define DEV_NAME "ttyLP" > > #define UART_NR 6 > > @@ -121,6 +132,26 @@ struct lpuart_port { > > struct clk *clk; > > unsigned int txfifo_size; > > unsigned int rxfifo_size; > > + > > +#ifdef CONFIG_SERIAL_FSL_LPUART_DMA > > + bool lpuart_dma_use; > > + struct dma_chan *dma_tx_chan; > > + struct dma_chan *dma_rx_chan; > > + struct dma_async_tx_descriptor *dma_tx_desc; > > + struct dma_async_tx_descriptor *dma_rx_desc; > > + dma_addr_t dma_tx_buf_bus; > > + dma_addr_t dma_rx_buf_bus; > > + dma_cookie_t dma_tx_cookie; > > + dma_cookie_t dma_rx_cookie; > > + unsigned char *dma_tx_buf_virt; > > + unsigned char *dma_rx_buf_virt; > > + unsigned int dma_tx_bytes; > > + unsigned int dma_rx_bytes; > > + int dma_tx_in_progress; > > + int dma_rx_in_progress; > > + unsigned int dma_rx_timeout; > > + struct timer_list lpuart_timer; > > +#endif > > }; > > This part will result in a slight increase in data size even if dma > support is turned off at compile time. > Yes, I will fix it. > > > > temp = readb(port->membase + UARTCR2); > > writeb(temp | UARTCR2_TIE, port->membase + UARTCR2); > > > > +#ifdef CONFIG_SERIAL_FSL_LPUART_DMA > > + if (sport->lpuart_dma_use) { > > + if (!uart_circ_empty(xmit) && !sport->dma_tx_in_progress) > > + lpuart_prepare_tx(sport); > > + } else { > > + if (readb(port->membase + UARTSR1) & UARTSR1_TDRE) > > + lpuart_transmit_buffer(sport); > > + } > > +#else > > if (readb(port->membase + UARTSR1) & UARTSR1_TDRE) > > lpuart_transmit_buffer(sport); > > +#endif > > } > > So this can simply become > > + if (IS_ENABLED(CONFIG_SERIAL_FSL_LPUART_DMA) && sport- > >lpuart_dma_use) { > + if (!uart_circ_empty(xmit) && !sport->dma_tx_in_progress) > + lpuart_prepare_tx(sport); > + } else { > + if (readb(port->membase + UARTSR1) & UARTSR1_TDRE) > + lpuart_trans mit_buffer(sport); > + } > > and the compiler will silently drop the lpuart_prepare_tx() function and > everything it calls that isn't used elsewhere. > It's wonderful. > > > + > > +#ifdef CONFIG_SERIAL_FSL_LPUART_DMA > > + struct platform_device *pdev = to_platform_device(port->dev); > > + struct device_node *np = pdev->dev.of_node; > > + > > + if (of_get_property(np, "dmas", NULL)) { > > + sport->lpuart_dma_use = true; > > + lpuart_dma_tx_request(port); > > + lpuart_dma_rx_request(port); > > + temp = readb(port->membase + UARTCR5); > > + writeb(temp | UARTCR5_TDMAS, port->membase + UARTCR5); > > + } else > > + sport->lpuart_dma_use = false; > > +#endif > > Same here. > > > @@ -854,6 +1291,10 @@ static int __init lpuart_serial_init(void) > > > > pr_info("serial: Freescale lpuart driver\n"); > > > > +#ifdef CONFIG_SERIAL_FSL_LPUART_DMA > > + pr_info("serial: Freescale lpuart dma support\n"); #endif > > + > > This message is not helpful in the long run, I'd rather see you drop the > other one as well. Note that this function is called on *all* platforms > when the driver is enabled, not just on those that actually have the uart. > > Arnd > -- > 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 > ��.n��������+%������w��{.n�����{��ǫ����{ay� ʇڙ���f���h������_�(�階�ݢj"��������G����?���&��
