<div dir="ltr">Hi Greg,<br> After modifying [ patch v4 1/5 ] , should i submit whole patchset as v5 ?<br> or replying with this single patch modified will be enough?<br> how is this done in linux community ?.<br><div><br></div><div>Regards</div><div>Sudheer</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 30, 2019 at 9:18 PM Greg KH <<a href="mailto:gregkh@linuxfoundation.org">gregkh@linuxfoundation.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Jul 26, 2019 at 06:57:16PM +0530, sudheer.v wrote:<br>
> From: sudheer veliseti <<a href="mailto:sudheer.open@gmail.com" target="_blank">sudheer.open@gmail.com</a>><br>
> <br>
> UART driver for Aspeed's bmc chip AST2500<br>
> <br>
> Design approch:<br>
> AST2500 has dedicated Uart DMA controller which has 12 sets of Tx and RX channels<br>
> connected to UART controller directly.<br>
> Since the DMA controller have dedicated buffers and registers,<br>
> there would be little benifit in adding DMA framework overhead.<br>
> So the software for DMA controller is included within the UART driver itself.<br>
> <br>
> implementation details:<br>
> 'struct ast_uart_port' is populated and registered with uart_core.<br>
> code is organised into two layers UART-layer and DMA-Layer,both of them are<br>
> in the same file.UART-layer requests Rx and Tx dma channels<br>
> and registers callbacks with DMA controller software Layer<br>
> Interrupt service routine for DMA controller is the crucial one for Handling all<br>
> the tx and rx data. ISRs installed for individual uarts are just dummy,and are helpful <br>
> only to report any spurious interrupts in hardware.<br>
> <br>
> <br>
> Signed-off-by: sudheer veliseti <<a href="mailto:sudheer.open@gmail.com" target="_blank">sudheer.open@gmail.com</a>><br>
> ---<br>
> <br>
> Changes from v3->v4:<br>
> - per port uart structures are registerd directly with uart core <br>
> Instead of registering through 8250 Frame work,<br>
> ast_uart_port is registered using uart_add_one_port<br>
> -SDMA_RX_FIX macro replaced with CONFIG_AST_UART_DMA_RX_INTERRUPT<br>
> -ast_uart_sdma_isr : DMA interrupt handler code is improvised<br>
> -replaced pr_debug with ftrace wherever appropriate<br>
> -dev_err is used in all error return cases<br>
> -uart driver structure ast25xx_uart_reg is modified<br>
> -driver name changed to ast2500-uart-dma-drv<br>
> -rx_timer initialisation and callback fn modified<br>
> <br>
> Changes from v2->v3:<br>
> -custom debug replaced by in kerenl dynamic debug: pr_debug <br>
> -change-logs added <br>
> <br>
> <br>
> .../tty/serial/8250/8250_ast2500_uart_dma.c | 1901 +++++++++++++++++<br>
> 1 file changed, 1901 insertions(+)<br>
> create mode 100644 drivers/tty/serial/8250/8250_ast2500_uart_dma.c<br>
> <br>
> diff --git a/drivers/tty/serial/8250/8250_ast2500_uart_dma.c b/drivers/tty/serial/8250/8250_ast2500_uart_dma.c<br>
> new file mode 100644<br>
> index 000000000000..bc830d605372<br>
> --- /dev/null<br>
> +++ b/drivers/tty/serial/8250/8250_ast2500_uart_dma.c<br>
> @@ -0,0 +1,1901 @@<br>
> +// SPDX-License-Identifier: GPL-2.0<br>
> +/*<br>
> + * DMA UART Driver for ASPEED BMC chip: AST2500<br>
> + *<br>
> + * Copyright (C) 2019 sudheer Kumar veliseti, Aspeed technology Inc.<br>
> + * <<a href="mailto:open.sudheer@gmail.com" target="_blank">open.sudheer@gmail.com</a>><br>
<br>
What was the copyright on the file you copied? Please properly<br>
attribute that here.<br>
<br>
<br>
> + *<br>
> + */<br>
> +#include <linux/clk.h><br>
> +#include <linux/dma-mapping.h><br>
> +#include <linux/module.h><br>
> +#include <linux/of_address.h><br>
> +#include <linux/of_irq.h><br>
> +#include <linux/tty.h><br>
> +#include <linux/tty_flip.h><br>
> +#include "8250.h"<br>
> +<br>
> +#define SERIAL8250_CONSOLE NULL<br>
> +#define TTY_AST_MAJOR 204<br>
> +#define TTY_AST_MINOR 68<br>
<br>
Where did you get this minor number from?<br>
<br>
> +<br>
> +#define DMA_BUFF_SIZE 0x1000<br>
> +#define SDMA_RX_BUFF_SIZE 0x10000<br>
> +#define PASS_LIMIT 256<br>
> +#define UART_DMA_NR CONFIG_AST_NR_DMA_UARTS<br>
> +#define AST_UART_SDMA_CH 12<br>
> +<br>
> +/* enum ast_uart_chan_op<br>
> + * operation codes passed to the DMA code by the user, and also used<br>
> + * to inform the current channel owner of any changes to the system state<br>
> + */<br>
> +enum ast_uart_chan_op {<br>
> + AST_UART_DMAOP_TRIGGER,<br>
> + AST_UART_DMAOP_STOP,<br>
> + AST_UART_DMAOP_PAUSE,<br>
> +};<br>
> +<br>
> +/* ast_uart_dma_cbfn: buffer callback routinei type */<br>
> +typedef void (*ast_uart_dma_cbfn)(void *dev_id, u16 len);<br>
> +<br>
> +struct ast_sdma_info {<br>
> + u8 ch_no;<br>
> + u8 direction;<br>
> + u8 enable;<br>
> + void *priv;<br>
> + char *sdma_virt_addr;<br>
> + dma_addr_t dma_phy_addr;<br>
> + /* cdriver callbacks */<br>
> + ast_uart_dma_cbfn callback_fn; /* buffer done callback */<br>
> +};<br>
> +<br>
> +struct ast_sdma_ch {<br>
> + struct ast_sdma_info tx_dma_info[AST_UART_SDMA_CH];<br>
> + struct ast_sdma_info rx_dma_info[AST_UART_SDMA_CH];<br>
> +};<br>
> +<br>
> +struct ast_sdma {<br>
> + void __iomem *reg_base;<br>
> + int dma_irq;<br>
> + struct ast_sdma_ch *dma_ch;<br>
> + struct regmap *map;<br>
> +};<br>
> +<br>
> +#define UART_TX_SDMA_EN 0x00<br>
> +#define UART_RX_SDMA_EN 0x04<br>
> +#define UART_SDMA_CONF 0x08 /* Misc, Buffer size */<br>
> +#define UART_SDMA_TIMER 0x0C<br>
> +#define UART_TX_SDMA_REST 0x20<br>
> +#define UART_RX_SDMA_REST 0x24<br>
> +#define UART_TX_SDMA_IER 0x30<br>
> +#define UART_TX_SDMA_ISR 0x34<br>
> +#define UART_RX_SDMA_IER 0x38<br>
> +#define UART_RX_SDMA_ISR 0x3C<br>
> +#define UART_TX_R_POINT(x) (0x40 + ((x) * 0x20))<br>
> +#define UART_TX_W_POINT(x) (0x44 + ((x) * 0x20))<br>
> +#define UART_TX_SDMA_ADDR(x) (0x48 + ((x) * 0x20))<br>
> +#define UART_RX_R_POINT(x) (0x50 + ((x) * 0x20))<br>
> +#define UART_RX_W_POINT(x) (0x54 + ((x) * 0x20))<br>
> +#define UART_RX_SDMA_ADDR(x) (0x58 + ((x) * 0x20))<br>
> +#define SDMA_CH_EN(x) BIT(x)<br>
> +<br>
> +#define SDMA_TX_BUFF_SIZE_MASK (0x3)<br>
> +#define SDMA_SET_TX_BUFF_SIZE(x)(x)<br>
> +#define SDMA_BUFF_SIZE_1KB (0x0)<br>
> +#define SDMA_BUFF_SIZE_4KB (0x1)<br>
> +#define SDMA_BUFF_SIZE_16KB (0x2)<br>
> +#define SDMA_BUFF_SIZE_64KB (0x3)<br>
> +#define SDMA_RX_BUFF_SIZE_MASK (0x3 << 2)<br>
> +#define SDMA_SET_RX_BUFF_SIZE(x)((x) << 2)<br>
> +#define SDMA_TIMEOUT_DIS BIT(4)<br>
> +<br>
> +#define UART_SDMA11_INT BIT(11)<br>
> +#define UART_SDMA10_INT BIT(10)<br>
> +#define UART_SDMA9_INT BIT(9)<br>
> +#define UART_SDMA8_INT BIT(8)<br>
> +#define UART_SDMA7_INT BIT(7)<br>
> +#define UART_SDMA6_INT BIT(6)<br>
> +#define UART_SDMA5_INT BIT(5)<br>
> +#define UART_SDMA4_INT BIT(4)<br>
> +#define UART_SDMA3_INT BIT(3)<br>
> +#define UART_SDMA2_INT BIT(2)<br>
> +#define UART_SDMA1_INT BIT(1)<br>
> +#define UART_SDMA0_INT BIT(0)<br>
> +<br>
> +/*<br>
> + * Configuration:<br>
> + * share_irqs - whether we pass IRQF_SHARED to request_irq().<br>
> + * This option is unsafe when used on edge-triggered interrupts.<br>
> + */<br>
> +static unsigned int share_irqs = SERIAL8250_SHARE_IRQS;<br>
> +<br>
> +static unsigned int nr_uarts = CONFIG_AST_RUNTIME_DMA_UARTS;<br>
> +<br>
> +struct ast_uart_port {<br>
> + struct uart_port port;<br>
> + unsigned short capabilities; /* port capabilities */<br>
> + unsigned short bugs; /* port bugs */<br>
> + unsigned int tx_loadsz; /* transmit fifo load size */<br>
> + unsigned char acr;<br>
> + unsigned char ier;<br>
> + unsigned char lcr;<br>
> + unsigned char mcr;<br>
> + unsigned char mcr_mask; /* mask of user bits */<br>
> + unsigned char mcr_force; /* mask of forced bits */<br>
> + struct circ_buf rx_dma_buf;<br>
> + struct circ_buf tx_dma_buf;<br>
> + unsigned char dma_channel;<br>
> + dma_addr_t dma_rx_addr; /* Mapped ADMA descr. table */<br>
> + dma_addr_t dma_tx_addr; /* Mapped ADMA descr. table */<br>
> +#ifdef CONFIG_AST_UART_DMA_RX_INTERRUPT<br>
> + struct tasklet_struct rx_tasklet;<br>
> +#else<br>
> + struct timer_list rx_timer;<br>
> + unsigned int workaround;<br>
> +#endif<br>
> + struct tasklet_struct tx_tasklet;<br>
> + spinlock_t lock;<br>
> + int tx_done;<br>
> + int tx_count;<br>
> + struct platform_device *ast_uart_pdev;<br>
> +/*<br>
> + * Some bits in registers are cleared on a read, so they must<br>
> + * be saved whenever the register is read but the bits will not<br>
> + * be immediately processed.<br>
> + */<br>
> +#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS<br>
> + unsigned char lsr_saved_flags;<br>
> +#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA<br>
> + unsigned char msr_saved_flags;<br>
> +<br>
> + /*<br>
> + * We provide a per-port pm hook.<br>
> + */<br>
> + void (*pm)(struct uart_port *port, unsigned int state,<br>
> + unsigned int old);<br>
> +};<br>
> +<br>
> +static struct ast_uart_port ast_uart_ports[UART_DMA_NR];<br>
> +<br>
> +#define GET_DEV(ast_uart_port_priv_ptr)\<br>
> + (ast_uart_port_priv_ptr->ast_uart_pdev->dev)<br>
> +<br>
> +static inline struct ast_uart_port *<br>
> +to_ast_dma_uart_port(struct uart_port *uart) {<br>
> + return container_of(uart, struct ast_uart_port, port);<br>
> +}<br>
> +<br>
> +struct irq_info {<br>
> + spinlock_t lock;<br>
> + struct ast_uart_port *up;<br>
> +};<br>
> +<br>
> +static struct irq_info ast_uart_irq[1];<br>
> +static DEFINE_MUTEX(ast_uart_mutex);<br>
> +<br>
> +/*<br>
> + * Here we define the default xmit fifo size used for each type of UART.<br>
> + */<br>
> +static const struct serial8250_config uart_config[] = {<br>
> + [PORT_UNKNOWN] = {<br>
> + .name = "unknown",<br>
> + .fifo_size = 1,<br>
> + .tx_loadsz = 1,<br>
> + },<br>
> + [PORT_8250] = {<br>
> + .name = "8250",<br>
> + .fifo_size = 1,<br>
> + .tx_loadsz = 1,<br>
> + },<br>
> + [PORT_16450] = {<br>
> + .name = "16450",<br>
> + .fifo_size = 1,<br>
> + .tx_loadsz = 1,<br>
> + },<br>
> + [PORT_16550] = {<br>
> + .name = "16550",<br>
> + .fifo_size = 1,<br>
> + .tx_loadsz = 1,<br>
> + },<br>
> + [PORT_16550A] = {<br>
> + .name = "16550A",<br>
> + .fifo_size = 16,<br>
> + .tx_loadsz = 16,<br>
> + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10<br>
> + | UART_FCR_DMA_SELECT,<br>
> + .flags = UART_CAP_FIFO,<br>
> + },<br>
> +};<br>
<br>
I doubt you need all of these port types, right? You only have one type<br>
of device, please strip out _ALL_ of the unneeded code in here. You did<br>
a wholesale copy of the old driver, to get away with that you then need<br>
to customize it to work properly with your hardware _AND_ take away all<br>
code that is not needed for your hardware.<br>
<br>
Lots of this file can be removed, please do so.<br>
<br>
thanks,<br>
<br>
greg k-h<br>
</blockquote></div>