Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On Sat, Oct 20, 2018 at 09:56:24PM +0530, Vinod wrote: > On 19-10-18, 12:41, sudheer.v wrote: > > On Fri, Oct 19, 2018 at 10:32:24AM +1100, Benjamin Herrenschmidt wrote: > > > On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote: > > > > > > > > > It's not a dmaengine driver. It's a serial UART driver that happens to > > > > > use a dedicated DMA engine. > > > > > > > > Then I see no reason for it to use dmaengine APIs. The framework allows > > > > people to share a controller for many clients, but if you have dedicated > > > > one then you may use it directly > > > > > > Well... the engine is shared by a few UARTs, they have dedicated rings > > > but there's a common set of regs for interrupt handling etc. > > > > > > That said, I still think it could be contained within a UART driver, > > > there's little benefit in adding the framework overhead, esp since > > > these are really weak cores, any overhead will be felt. > > > > > > Ben. > > > > > > > > It's unclear whether it should be split into two drivers, or just have > > > > > the serial driver directly use the dma engine since that engine is > > > > > dedicated in HW to only work on those UARTs and nothing else... > > > > > > > > > > Cheers, > > > > > Ben. > > > > Initially we wanted to have a single driver, > > however we had an informal discussion with one of the maintainer > > and based on the feedback, followed the Linux DMA and UART architecture. > > > > If this seperate DMA-engine driver adds more overhead than benifit, > > we will merge them into a single UART driver and resubmitt the patches. > > Vinod, > > can this dma-controller driver sit under dma subsystem?. > > or better to move it under UART framework. > > > My advise would be to see what you can do with the DMA IP block. If this > can/would be used in different places then it would make sense to do a > dmaengine driver and solve the problem for everyone. > > If this is always going to be hidden behind serial then maybe it makes > sense to be inside serial driver and not use dmaengine APIs > > If you decide to prefer the former case, please move it to dmaengine and > resubmit :) > > HTH > -- > ~Vinod Hi All, As the DMA engine is dedicated only to UART,we have decided to rewrite the driver so that no code will come under drivers/dma. I will resubmitt the patches after merging dma controller code and uart driver code. Regards -sudheer
Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On 19-10-18, 12:41, sudheer.v wrote: > On Fri, Oct 19, 2018 at 10:32:24AM +1100, Benjamin Herrenschmidt wrote: > > On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote: > > > > > > > It's not a dmaengine driver. It's a serial UART driver that happens to > > > > use a dedicated DMA engine. > > > > > > Then I see no reason for it to use dmaengine APIs. The framework allows > > > people to share a controller for many clients, but if you have dedicated > > > one then you may use it directly > > > > Well... the engine is shared by a few UARTs, they have dedicated rings > > but there's a common set of regs for interrupt handling etc. > > > > That said, I still think it could be contained within a UART driver, > > there's little benefit in adding the framework overhead, esp since > > these are really weak cores, any overhead will be felt. > > > > Ben. > > > > > > It's unclear whether it should be split into two drivers, or just have > > > > the serial driver directly use the dma engine since that engine is > > > > dedicated in HW to only work on those UARTs and nothing else... > > > > > > > > Cheers, > > > > Ben. > > Initially we wanted to have a single driver, > however we had an informal discussion with one of the maintainer > and based on the feedback, followed the Linux DMA and UART architecture. > > If this seperate DMA-engine driver adds more overhead than benifit, > we will merge them into a single UART driver and resubmitt the patches. > Vinod, > can this dma-controller driver sit under dma subsystem?. > or better to move it under UART framework. My advise would be to see what you can do with the DMA IP block. If this can/would be used in different places then it would make sense to do a dmaengine driver and solve the problem for everyone. If this is always going to be hidden behind serial then maybe it makes sense to be inside serial driver and not use dmaengine APIs If you decide to prefer the former case, please move it to dmaengine and resubmit :) HTH -- ~Vinod
Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On Fri, Oct 19, 2018 at 10:32:24AM +1100, Benjamin Herrenschmidt wrote: > On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote: > > > > > It's not a dmaengine driver. It's a serial UART driver that happens to > > > use a dedicated DMA engine. > > > > Then I see no reason for it to use dmaengine APIs. The framework allows > > people to share a controller for many clients, but if you have dedicated > > one then you may use it directly > > Well... the engine is shared by a few UARTs, they have dedicated rings > but there's a common set of regs for interrupt handling etc. > > That said, I still think it could be contained within a UART driver, > there's little benefit in adding the framework overhead, esp since > these are really weak cores, any overhead will be felt. > > Ben. > > > > It's unclear whether it should be split into two drivers, or just have > > > the serial driver directly use the dma engine since that engine is > > > dedicated in HW to only work on those UARTs and nothing else... > > > > > > Cheers, > > > Ben. Initially we wanted to have a single driver, however we had an informal discussion with one of the maintainer and based on the feedback, followed the Linux DMA and UART architecture. If this seperate DMA-engine driver adds more overhead than benifit, we will merge them into a single UART driver and resubmitt the patches. Vinod, can this dma-controller driver sit under dma subsystem?. or better to move it under UART framework. Thank you. -- Sudheer
Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote: > > > It's not a dmaengine driver. It's a serial UART driver that happens to > > use a dedicated DMA engine. > > Then I see no reason for it to use dmaengine APIs. The framework allows > people to share a controller for many clients, but if you have dedicated > one then you may use it directly Well... the engine is shared by a few UARTs, they have dedicated rings but there's a common set of regs for interrupt handling etc. That said, I still think it could be contained within a UART driver, there's little benefit in adding the framework overhead, esp since these are really weak cores, any overhead will be felt. Ben. > > It's unclear whether it should be split into two drivers, or just have > > the serial driver directly use the dma engine since that engine is > > dedicated in HW to only work on those UARTs and nothing else... > > > > Cheers, > > Ben. > > > > > > > While doing resubmission please take some time to understand subsystem > > > tags to use. (hint git log will tell you) > > > > > > Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please > > > let git generate that for you (hint git format-patch start..end does a > > > good job) > > > > > > > @@ -0,0 +1,1594 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > > + *1. 2018/07/01 Shivah Shankar created > > > > + *2. 2018/08/25 sudheer.veliseti modified > > > > > > we dont use this log in kernel. I do not see s-o-b by Shivah, that > > > should be added. I think he should be author and you need to list > > > changes you did.. > > > > >
Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On 17-10-18, 19:56, Benjamin Herrenschmidt wrote: > On Wed, 2018-10-17 at 11:35 +0530, Vinod wrote: > > On 17-10-18, 09:41, sudheer.v wrote: > > > > Please add the change log describing the driver and its features > > > > > Signed-off-by: sudheer.v > > > --- > > > drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 > > > > > > 1 file changed, 1594 insertions(+) > > > create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > > > > diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > new file mode 100644 > > > index 000..e1019a8 > > > --- /dev/null > > > +++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > > why is this in serial. It is dmaengine driver so belongs to drivers/dma/ > > like other controllers. Please move it out and resubmit. > > It's not a dmaengine driver. It's a serial UART driver that happens to > use a dedicated DMA engine. Then I see no reason for it to use dmaengine APIs. The framework allows people to share a controller for many clients, but if you have dedicated one then you may use it directly > It's unclear whether it should be split into two drivers, or just have > the serial driver directly use the dma engine since that engine is > dedicated in HW to only work on those UARTs and nothing else... > > Cheers, > Ben. > > > > While doing resubmission please take some time to understand subsystem > > tags to use. (hint git log will tell you) > > > > Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please > > let git generate that for you (hint git format-patch start..end does a > > good job) > > > > > @@ -0,0 +1,1594 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > + *1. 2018/07/01 Shivah Shankar created > > > + *2. 2018/08/25 sudheer.veliseti modified > > > > we dont use this log in kernel. I do not see s-o-b by Shivah, that > > should be added. I think he should be author and you need to list > > changes you did.. > > -- ~Vinod
Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On Wed, 2018-10-17 at 11:35 +0530, Vinod wrote: > On 17-10-18, 09:41, sudheer.v wrote: > > Please add the change log describing the driver and its features > > > Signed-off-by: sudheer.v > > > > --- > > drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 > > > > 1 file changed, 1594 insertions(+) > > create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > > > diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > new file mode 100644 > > index 000..e1019a8 > > --- /dev/null > > +++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > why is this in serial. It is dmaengine driver so belongs to drivers/dma/ > like other controllers. Please move it out and resubmit. It's not a dmaengine driver. It's a serial UART driver that happens to use a dedicated DMA engine. It's unclear whether it should be split into two drivers, or just have the serial driver directly use the dma engine since that engine is dedicated in HW to only work on those UARTs and nothing else... Cheers, Ben. > While doing resubmission please take some time to understand subsystem > tags to use. (hint git log will tell you) > > Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please > let git generate that for you (hint git format-patch start..end does a > good job) > > > @@ -0,0 +1,1594 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > + *1. 2018/07/01 Shivah Shankar created > > + *2. 2018/08/25 sudheer.veliseti modified > > we dont use this log in kernel. I do not see s-o-b by Shivah, that > should be added. I think he should be author and you need to list > changes you did.. >
Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
On 17-10-18, 09:41, sudheer.v wrote: Please add the change log describing the driver and its features > Signed-off-by: sudheer.v > --- > drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 > > 1 file changed, 1594 insertions(+) > create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c > > diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c > new file mode 100644 > index 000..e1019a8 > --- /dev/null > +++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c why is this in serial. It is dmaengine driver so belongs to drivers/dma/ like other controllers. Please move it out and resubmit. While doing resubmission please take some time to understand subsystem tags to use. (hint git log will tell you) Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please let git generate that for you (hint git format-patch start..end does a good job) > @@ -0,0 +1,1594 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c > + *1. 2018/07/01 Shivah Shankar created > + *2. 2018/08/25 sudheer.veliseti modified we dont use this log in kernel. I do not see s-o-b by Shivah, that should be added. I think he should be author and you need to list changes you did.. -- ~Vinod
[[PATCH] 8/9] DMA-UART-Driver-for-AST2500
Signed-off-by: sudheer.v
---
drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594
1 file changed, 1594 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c
diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
new file mode 100644
index 000..e1019a8
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
@@ -0,0 +1,1594 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * drivers/tty/serial/8250/8250_aspeed_uart_dma.c
+ *1. 2018/07/01 Shivah Shankar created
+ *2. 2018/08/25 sudheer.veliseti modified
+ *
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "8250.h"
+#include
+#define SDMA_RX_BUFF_SIZE 0x1 //65536
+#define DMA_BUFF_SIZE 0x1000 //4096
+
+
+
+
+#undef UART_XMIT_SIZE
+#define UART_XMIT_SIZE 0x1000
+#define UART_RX_SIZE 0x1
+
+#ifdef UART_DMA_DEBUG
+ #define UART_DBG(fmt, args...) pr_debug("%s() " fmt, __func__, ## args)
+#else
+ #define UART_DBG(fmt, args...)
+#endif
+
+#ifdef CONFIG_UART_TX_DMA_DEBUG
+ #define UART_TX_DBG(fmt, args...) pr_debug("%s()"fmt, __func__, ## args)
+#else
+ #define UART_TX_DBG(fmt, args...)
+#endif
+
+/*
+ * Configuration:
+ * share_irqs - whether we pass IRQF_SHARED to request_irq(). This option
+ *is unsafe when used on edge-triggered interrupts.
+ */
+static unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
+
+static unsigned int nr_uarts = CONFIG_AST_RUNTIME_DMA_UARTS;
+
+/*
+ * Debugging.
+ */
+#if 0
+#define DEBUG_AUTOCONF(fmt...) UART_DBG(fmt)
+#else
+#define DEBUG_AUTOCONF(fmt...) do { } while (0)
+#endif
+
+#if 0
+#define DEBUG_INTR(fmt...) UART_DBG(fmt)
+#else
+#define DEBUG_INTR(fmt...) do { } while (0)
+#endif
+
+#define PASS_LIMIT 256
+
+#include
+
+
+#define UART_DMA_NRCONFIG_AST_NR_DMA_UARTS
+
+struct ast_uart_port {
+ struct uart_portport;
+ struct platform_device *pdev;
+ unsigned short capabilities; /* port capabilities */
+ unsigned short bugs; /* port bugs */
+ unsigned inttx_loadsz; /* transmit fifo load size */
+ unsigned char acr;
+ unsigned char ier;
+ unsigned char lcr;
+ unsigned char mcr;
+ unsigned char mcr_mask; /* mask of user bits */
+ unsigned char mcr_force; /* mask of forced bits */
+ unsigned intchannel_no;
+ struct scatterlist rx_sgl;
+ struct dma_chan *rx_dma_chan;
+ struct circ_buf rx_dma_buf;
+ dma_addr_t dma_rx_addr;
+ u8 rx_in_progress;
+ struct dma_async_tx_descriptor *rx_dma_desc;
+ dma_cookie_trx_cookie;
+ unsigned intrx_bytes_requested;
+ unsigned intrx_bytes_transferred;
+ struct tasklet_struct rx_tasklet;
+ struct scatterlist tx_sgl;
+ struct dma_chan *tx_dma_chan;
+ struct circ_buf tx_dma_buf;
+ dma_addr_t dma_tx_addr;
+ u8 tx_in_progress;
+ struct dma_async_tx_descriptor *tx_dma_desc;
+ dma_cookie_ttx_cookie;
+ unsigned inttx_bytes_requested;
+ unsigned inttx_bytes_transferred;
+ struct tasklet_struct tx_tasklet;
+ spinlock_t lock;
+ int tx_done;
+ int tx_count;
+ /*
+* Some bits in registers are cleared on a read, so they must
+* be saved whenever the register is read but the bits will not
+* be immediately processed.
+*/
+#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
+ unsigned char lsr_saved_flags;
+#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
+ unsigned char msr_saved_flags;
+
+ /*
+* We provide a per-port pm hook.
+*/
+ void(*pm)(struct uart_port *port,
+ unsigned int state, unsigned int old);
+};
+
+static struct ast_uart_port ast_uart_ports[UART_DMA_NR];
+
+static int ast_dma_channel_setup(struct ast_uart_port *up);
+static inline struct ast_uart_port *
+to_ast_dma_uart_port(struct uart_port *uart)
+{
+ return container_of(uart, struct ast_uart_port, port);
+}
+
+struct irq_info {
+ spinlock_t lock;
+ struct ast_uart_port *up;
+};
+
+static void ast_dma_channel_teardown(struct ast_uart_port *s);
+static struct irq_info ast_uart_irq[1];
+static DEFINE_MUTEX(ast_uart_mutex);
+
+/*
+ * Here we define
