[PATCH 0/7] RZN1 UART DMA support

[PATCH 0/7] RZN1 UART DMA support

[Miquel Raynal]

Hello,

Support for the RZN1 DMA engine allows us adapt a little bit the 8250 DW
UART driver with to bring DMA support for this SoC.

This short series applies on top of the series bringing RZN1 DMA
support, currently on its v4, see [1]. Technically speaking, only the DT
patch needs to be applied after [1]. The other patches can come in at
any moment, because if no "dmas" property is provided in the DT, DMA
support will simply be ignored.

[1] https://lore.kernel.org/dmaengine/20220310155755.287294-1-miquel.raynal@bootlin.com/T/#mce6fec36e16dca560ab18935c273fcaf794a1cc4

Thanks,
Miquèl

Miquel Raynal (2):
  serial: 8250_dw: Provide the RZN1 CPR register value
  ARM: dts: r9a06g032: Fill the UART DMA properties

Phil Edworthy (5):
  serial: 8250_dma: Use ->tx_dma function pointer to start next DMA
  serial: 8250_dw: Move the per-device structure
  serial: 8250_dw: Use a fallback CPR value if not synthesized
  serial: 8250_dw: Add a dma_capable bit to the platform data
  serial: 8250_dw: Add support for RZ/N1 DMA

 arch/arm/boot/dts/r9a06g032.dtsi     |  15 ++++
 drivers/tty/serial/8250/8250_dma.c   |   4 +-
 drivers/tty/serial/8250/8250_dw.c    | 119 +++++++++++++++++++++++----
 drivers/tty/serial/8250/8250_dwlib.c |  17 +++-
 drivers/tty/serial/8250/8250_dwlib.h |  22 +++++
 5 files changed, 155 insertions(+), 22 deletions(-)

-- 
2.27.0

[PATCH 1/7] serial: 8250_dma: Use ->tx_dma function pointer to start next DMA

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

The 8250 driver is quite flexible. Regarding DMA handling, there is the
possibility to either use the default helper (serial8250_tx_dma()) or
call a specific function. Only the omap and brcm implementation do
that. In both cases, they don't use the serial8250_tx_dma() helper at
all.

As we are going to write a new DMA handling function for the RZ/N1 SoCs
which will use the serial8250_tx_dma() implementation (preceded by a
couple of register writes), we need the ->tx_dma() pointer to link to
our own function, but within the __dma_tx_complete() helper we also need
to call our own implementation instead of the default one directly.

In order to do that, let's call ->tx_dma() instead of
serial8250_tx_dma() from __dma_tx_complete().

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
[miquel.raynal@bootlin.com: Re-write commit message]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 890fa7ddaa7f..a0563f2341ac 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -33,7 +33,7 @@ static void __dma_tx_complete(void *param)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
 
-	ret = serial8250_tx_dma(p);
+	ret = dma->tx_dma(p);
 	if (ret)
 		serial8250_set_THRI(p);
 
-- 
2.27.0

[PATCH 2/7] serial: 8250_dw: Move the per-device structure

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

This structure needs to be reused from dwlib, so let's move it into a
shared header. There is no functional change.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
[miquel.raynal@bootlin.com: Extracted from a bigger change]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dw.c    | 16 ----------------
 drivers/tty/serial/8250/8250_dwlib.h | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1769808031c5..ee7562a9ec76 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -37,22 +37,6 @@
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
 
-struct dw8250_data {
-	struct dw8250_port_data	data;
-
-	u8			usr_reg;
-	int			msr_mask_on;
-	int			msr_mask_off;
-	struct clk		*clk;
-	struct clk		*pclk;
-	struct notifier_block	clk_notifier;
-	struct work_struct	clk_work;
-	struct reset_control	*rst;
-
-	unsigned int		skip_autocfg:1;
-	unsigned int		uart_16550_compatible:1;
-};
-
 static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
 {
 	return container_of(data, struct dw8250_data, data);
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index 83d528e5cc21..ef63eaf7e598 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -16,5 +16,21 @@ struct dw8250_port_data {
 	u8			dlf_size;
 };
 
+struct dw8250_data {
+	struct dw8250_port_data	data;
+
+	u8			usr_reg;
+	int			msr_mask_on;
+	int			msr_mask_off;
+	struct clk		*clk;
+	struct clk		*pclk;
+	struct notifier_block	clk_notifier;
+	struct work_struct	clk_work;
+	struct reset_control	*rst;
+
+	unsigned int		skip_autocfg:1;
+	unsigned int		uart_16550_compatible:1;
+};
+
 void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct ktermios *old);
 void dw8250_setup_port(struct uart_port *p);
-- 
2.27.0

[PATCH 3/7] serial: 8250_dw: Use a fallback CPR value if not synthesized

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

This UART controller can be synthesized without the CPR register.
In that case, let's use the platform information to provide a CPR value.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dwlib.c | 10 ++++++++--
 drivers/tty/serial/8250/8250_dwlib.h |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 622d3b0d89e7..5cf298c5a0f9 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -3,6 +3,7 @@
 
 #include <linux/bitops.h>
 #include <linux/device.h>
+#include <linux/of_device.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/serial_8250.h>
@@ -90,6 +91,7 @@ EXPORT_SYMBOL_GPL(dw8250_do_set_termios);
 void dw8250_setup_port(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
+	const struct dw8250_platform_data *plat = of_device_get_match_data(up->port.dev);
 	u32 reg;
 
 	/*
@@ -116,8 +118,12 @@ void dw8250_setup_port(struct uart_port *p)
 	}
 
 	reg = dw8250_readl_ext(p, DW_UART_CPR);
-	if (!reg)
-		return;
+	if (!reg) {
+		if (!plat)
+			return;
+
+		reg = plat->cpr;
+	}
 
 	/* Select the type based on FIFO */
 	if (reg & DW_UART_CPR_FIFO_MODE) {
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index ef63eaf7e598..ffce2744a28e 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -16,6 +16,10 @@ struct dw8250_port_data {
 	u8			dlf_size;
 };
 
+struct dw8250_platform_data {
+	u32 cpr;
+};
+
 struct dw8250_data {
 	struct dw8250_port_data	data;
 
-- 
2.27.0

[PATCH 4/7] serial: 8250_dw: Provide the RZN1 CPR register value

[Miquel Raynal]

On the RZN1 SoC, the DW serial IP has been synthesized without CPR
support. In order to continue being able to parse the hardware
capabilities, provide the equivalent register value as platform data.

Suggested-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index ee7562a9ec76..1f7a423d6ef2 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -677,11 +677,15 @@ static const struct dev_pm_ops dw8250_pm_ops = {
 	SET_RUNTIME_PM_OPS(dw8250_runtime_suspend, dw8250_runtime_resume, NULL)
 };
 
+static const struct dw8250_platform_data rzn1_pdata = {
+	.cpr = 0x00012f32,
+};
+
 static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-uart" },
 	{ .compatible = "cavium,octeon-3860-uart" },
 	{ .compatible = "marvell,armada-38x-uart" },
-	{ .compatible = "renesas,rzn1-uart" },
+	{ .compatible = "renesas,rzn1-uart", .data = &rzn1_pdata },
 	{ .compatible = "starfive,jh7100-uart" },
 	{ /* Sentinel */ }
 };
-- 
2.27.0

[PATCH 5/7] serial: 8250_dw: Add a dma_capable bit to the platform data

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

The CPR register can give the information whether the IP is DMA capable
or not. Let's extract this information and use it to discriminate when
the DMA can be hooked up or not.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dw.c    | 4 ++--
 drivers/tty/serial/8250/8250_dwlib.c | 7 +++++--
 drivers/tty/serial/8250/8250_dwlib.h | 1 +
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1f7a423d6ef2..c0f54284bc70 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -556,8 +556,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (!data->skip_autocfg)
 		dw8250_setup_port(p);
 
-	/* If we have a valid fifosize, try hooking up DMA */
-	if (p->fifosize) {
+	/* If we have a valid fifosize and DMA support, try hooking up DMA */
+	if (p->fifosize && data->dma_capable) {
 		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
 		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
 		up->dma = &data->data.dma;
diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 5cf298c5a0f9..5ec7c12ed117 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -92,6 +92,8 @@ void dw8250_setup_port(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
 	const struct dw8250_platform_data *plat = of_device_get_match_data(up->port.dev);
+	struct dw8250_port_data *d = p->private_data;
+	struct dw8250_data *data = container_of(d, struct dw8250_data, data);
 	u32 reg;
 
 	/*
@@ -110,8 +112,6 @@ void dw8250_setup_port(struct uart_port *p)
 	dw8250_writel_ext(p, DW_UART_DLF, 0);
 
 	if (reg) {
-		struct dw8250_port_data *d = p->private_data;
-
 		d->dlf_size = fls(reg);
 		p->get_divisor = dw8250_get_divisor;
 		p->set_divisor = dw8250_set_divisor;
@@ -138,5 +138,8 @@ void dw8250_setup_port(struct uart_port *p)
 
 	if (reg & DW_UART_CPR_SIR_MODE)
 		up->capabilities |= UART_CAP_IRDA;
+
+	if (reg & DW_UART_CPR_DMA_EXTRA)
+		data->dma_capable = 1;
 }
 EXPORT_SYMBOL_GPL(dw8250_setup_port);
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index ffce2744a28e..900b2321aff0 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -34,6 +34,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		dma_capable:1;
 };
 
 void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct ktermios *old);
-- 
2.27.0

[PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

The Renesas RZ/N1 devices have a modified Synopsys DW UART. The
modifications are mostly related to the DMA handlnig, and so this patch
adds support for DMA.

The RZ/N1 UART must be used with the peripheral as the flow
controller. This means the DMA length should also be programmed into
UART registers.

Aside from this, there are some points to note about DMA burst sizes.
First, DMA must not remove all of the data from the rx FIFO. Otherwise,
we do not get a 'character timeout' interrupt, and so do not know that
we should push data up the serial stack. Therefore, we have the rx
threshold for generating an interrupt set to half the FIFO depth (this
is the default for 16550A), and set the DMA burst size when reading the
FIFO to a quarter of the FIFO depth.

Second, when transmitting data using DMA, the burst size must be limited
to 1 byte to handle then case when transmitting just 1 byte. Otherwise
the DMA doesn't complete the burst, and nothing happens.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dma.c   |  2 +
 drivers/tty/serial/8250/8250_dw.c    | 97 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_dwlib.h |  1 +
 3 files changed, 100 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index a0563f2341ac..0858c0b988a2 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -105,6 +105,7 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 	dma->tx_err = 1;
 	return ret;
 }
+EXPORT_SYMBOL_GPL(serial8250_tx_dma);
 
 int serial8250_rx_dma(struct uart_8250_port *p)
 {
@@ -130,6 +131,7 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(serial8250_rx_dma);
 
 void serial8250_rx_dma_flush(struct uart_8250_port *p)
 {
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index c0f54284bc70..04e05fc939df 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -37,6 +37,20 @@
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
 
+/* Offsets for the Renesas RZ/N1 DesignWare specific registers */
+/* DMA Software Ack */
+#define RZN1_UART_DMASA			0xa8
+/* DMA Control Register Transmit Mode */
+#define RZN1_UART_TDMACR		0x10c
+/* DMA Control Register Receive Mode */
+#define RZN1_UART_RDMACR		0x110
+
+#define RZN1_UART_xDMACR_DMA_EN		BIT(0)
+#define RZN1_UART_xDMACR_1_WORD_BURST	0
+#define RZN1_UART_xDMACR_4_WORD_BURST	BIT(1)
+#define RZN1_UART_xDMACR_8_WORD_BURST	(BIT(1) | BIT(2))
+#define RZN1_UART_xDMACR_BLK_SZ_OFFSET	3
+
 static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
 {
 	return container_of(data, struct dw8250_data, data);
@@ -217,6 +231,22 @@ static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
 }
 
 
+static void rzn1_8250_handle_irq(struct uart_port *port, unsigned int iir)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct uart_8250_dma *dma = up->dma;
+	unsigned char status;
+
+	if (up->dma && dma->rx_running) {
+		status = port->serial_in(port, UART_LSR);
+		if (status & (UART_LSR_DR | UART_LSR_BI)) {
+			/* Stop the DMA transfer */
+			writel(0, port->membase + RZN1_UART_RDMACR);
+			writel(1, port->membase + RZN1_UART_DMASA);
+		}
+	}
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
@@ -245,6 +275,9 @@ static int dw8250_handle_irq(struct uart_port *p)
 		spin_unlock_irqrestore(&p->lock, flags);
 	}
 
+	if (d->is_rzn1 && ((iir & 0x3f) == UART_IIR_RX_TIMEOUT))
+		rzn1_8250_handle_irq(p, iir);
+
 	if (serial8250_handle_irq(p, iir))
 		return 1;
 
@@ -368,6 +401,61 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
 	return param == chan->device->dev;
 }
 
+static u32 rzn1_get_dmacr_burst(int max_burst)
+{
+	u32 val = 0;
+
+	if (max_burst >= 8)
+		val = RZN1_UART_xDMACR_8_WORD_BURST;
+	else if (max_burst >= 4)
+		val = RZN1_UART_xDMACR_4_WORD_BURST;
+	else
+		val = RZN1_UART_xDMACR_1_WORD_BURST;
+
+	return val;
+}
+
+static int rzn1_dw8250_tx_dma(struct uart_8250_port *p)
+{
+	struct uart_port		*up = &p->port;
+	struct uart_8250_dma		*dma = p->dma;
+	struct circ_buf			*xmit = &p->port.state->xmit;
+	int tx_size;
+	u32 val;
+
+	if (uart_tx_stopped(&p->port) || dma->tx_running ||
+	    uart_circ_empty(xmit))
+		return 0;
+
+	tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
+
+	writel(0, up->membase + RZN1_UART_TDMACR);
+	val = rzn1_get_dmacr_burst(dma->txconf.dst_maxburst);
+	val |= tx_size << RZN1_UART_xDMACR_BLK_SZ_OFFSET;
+	val |= RZN1_UART_xDMACR_DMA_EN;
+	writel(val, up->membase + RZN1_UART_TDMACR);
+
+	return serial8250_tx_dma(p);
+}
+
+static int rzn1_dw8250_rx_dma(struct uart_8250_port *p)
+{
+	struct uart_port		*up = &p->port;
+	struct uart_8250_dma		*dma = p->dma;
+	u32 val;
+
+	if (dma->rx_running)
+		return 0;
+
+	writel(0, up->membase + RZN1_UART_RDMACR);
+	val = rzn1_get_dmacr_burst(dma->rxconf.src_maxburst);
+	val |= dma->rx_size << RZN1_UART_xDMACR_BLK_SZ_OFFSET;
+	val |= RZN1_UART_xDMACR_DMA_EN;
+	writel(val, up->membase + RZN1_UART_RDMACR);
+
+	return serial8250_rx_dma(p);
+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	struct device_node *np = p->dev->of_node;
@@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *pdev)
 		data->msr_mask_off |= UART_MSR_TERI;
 	}
 
+	data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");
+
 	/* Always ask for fixed clock rate from a property. */
 	device_property_read_u32(dev, "clock-frequency", &p->uartclk);
 
@@ -561,6 +651,13 @@ static int dw8250_probe(struct platform_device *pdev)
 		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
 		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
 		up->dma = &data->data.dma;
+
+		if (data->is_rzn1) {
+			data->data.dma.txconf.device_fc = 1;
+			data->data.dma.rxconf.device_fc = 1;
+			uart.dma->tx_dma = rzn1_dw8250_tx_dma;
+			uart.dma->rx_dma = rzn1_dw8250_rx_dma;
+		}
 	}
 
 	data->data.line = serial8250_register_8250_port(up);
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index 900b2321aff0..014005c170e4 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -35,6 +35,7 @@ struct dw8250_data {
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
 	unsigned int		dma_capable:1;
+	unsigned int		is_rzn1:1;
 };
 
 void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct ktermios *old);
-- 
2.27.0

[PATCH 7/7] ARM: dts: r9a06g032: Fill the UART DMA properties

[Miquel Raynal]

UART 0 to 2 do not have DMA support, while UART 3 to 7 do.

Fill the "dmas" and "dma-names" properties for each of these nodes.

Please mind that these nodes go through the dmamux node which will
redirect the requests to the right DMA controller. The first 4 cells of
the "dmas" properties will be transferred as-is to the DMA
controllers. The last 2 cells are consumed by the dmamux. Which means
cell 0 and 4 are almost redundant, one giving the controller request ID
and the other the dmamux channel which is a 1:1 translation of the
request IDs, shifted by 16 when pointing to the second DMA controller.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 804f2d6f416f..aa447e2622e0 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -128,6 +128,9 @@ uart3: serial@50000000 {
 			reg-io-width = <4>;
 			clocks = <&sysctrl R9A06G032_CLK_UART3>, <&sysctrl R9A06G032_HCLK_UART3>;
 			clock-names = "baudclk", "apb_pclk";
+			dmas =  <&dmamux 0 0 0 0 0 1>,
+				<&dmamux 1 0 0 0 1 1>;
+			dma-names = "rx", "tx";
 			status = "disabled";
 		};
 
@@ -139,6 +142,9 @@ uart4: serial@50001000 {
 			reg-io-width = <4>;
 			clocks = <&sysctrl R9A06G032_CLK_UART4>, <&sysctrl R9A06G032_HCLK_UART4>;
 			clock-names = "baudclk", "apb_pclk";
+			dmas =  <&dmamux 2 0 0 0 2 1>,
+				<&dmamux 3 0 0 0 3 1>;
+			dma-names = "rx", "tx";
 			status = "disabled";
 		};
 
@@ -150,6 +156,9 @@ uart5: serial@50002000 {
 			reg-io-width = <4>;
 			clocks = <&sysctrl R9A06G032_CLK_UART5>, <&sysctrl R9A06G032_HCLK_UART5>;
 			clock-names = "baudclk", "apb_pclk";
+			dmas =  <&dmamux 4 0 0 0 4 1>,
+				<&dmamux 5 0 0 0 5 1>;
+			dma-names = "rx", "tx";
 			status = "disabled";
 		};
 
@@ -161,6 +170,9 @@ uart6: serial@50003000 {
 			reg-io-width = <4>;
 			clocks = <&sysctrl R9A06G032_CLK_UART6>, <&sysctrl R9A06G032_HCLK_UART6>;
 			clock-names = "baudclk", "apb_pclk";
+			dmas =  <&dmamux 6 0 0 0 6 1>,
+				<&dmamux 7 0 0 0 7 1>;
+			dma-names = "rx", "tx";
 			status = "disabled";
 		};
 
@@ -172,6 +184,9 @@ uart7: serial@50004000 {
 			reg-io-width = <4>;
 			clocks = <&sysctrl R9A06G032_CLK_UART7>, <&sysctrl R9A06G032_HCLK_UART7>;
 			clock-names = "baudclk", "apb_pclk";
+			dmas =  <&dmamux 4 0 0 0 20 1>,
+				<&dmamux 5 0 0 0 21 1>;
+			dma-names = "rx", "tx";
 			status = "disabled";
 		};
 
-- 
2.27.0

Re: [PATCH 1/7] serial: 8250_dma: Use ->tx_dma function pointer to start next DMA

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 05:16:44PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> The 8250 driver is quite flexible. Regarding DMA handling, there is the
> possibility to either use the default helper (serial8250_tx_dma()) or
> call a specific function. Only the omap and brcm implementation do
> that. In both cases, they don't use the serial8250_tx_dma() helper at
> all.
> 
> As we are going to write a new DMA handling function for the RZ/N1 SoCs
> which will use the serial8250_tx_dma() implementation (preceded by a
> couple of register writes), we need the ->tx_dma() pointer to link to
> our own function, but within the __dma_tx_complete() helper we also need
> to call our own implementation instead of the default one directly.
> 
> In order to do that, let's call ->tx_dma() instead of
> serial8250_tx_dma() from __dma_tx_complete().

In 8250 driver the pattern is to give the generic function "do" name and
then call it in a wrapper:

	if (->foo())
		return ->foo();

	return serial8250_do_foo();

There are plenty of examples in that driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] serial: 8250_dw: Move the per-device structure

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 05:16:45PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> This structure needs to be reused from dwlib, so let's move it into a
> shared header. There is no functional change.

...

> +struct dw8250_data {
> +	struct dw8250_port_data	data;

> +	u8			usr_reg;

I believe types.h is already in the inclusion block.

> +	int			msr_mask_on;
> +	int			msr_mask_off;

> +	struct clk		*clk;
> +	struct clk		*pclk;

Do you need forward declarations?

> +	struct notifier_block	clk_notifier;
> +	struct work_struct	clk_work;

I haven't seen the change in the inclusion block.
Don't you miss necessary headers?

> +	struct reset_control	*rst;

Do you need forward declaration?

> +	unsigned int		skip_autocfg:1;
> +	unsigned int		uart_16550_compatible:1;
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/7] serial: 8250_dw: Use a fallback CPR value if not synthesized

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 05:16:46PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> This UART controller can be synthesized without the CPR register.
> In that case, let's use the platform information to provide a CPR value.

...

> +#include <linux/of_device.h>

> +	const struct dw8250_platform_data *plat = of_device_get_match_data(up->port.dev);

No. Please use device property APIs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] serial: 8250_dw: Add a dma_capable bit to the platform data

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 05:16:48PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> The CPR register can give the information whether the IP is DMA capable
> or not. Let's extract this information and use it to discriminate when
> the DMA can be hooked up or not.

...

> +	/* If we have a valid fifosize and DMA support, try hooking up DMA */
> +	if (p->fifosize && data->dma_capable) {

> +	if (reg & DW_UART_CPR_DMA_EXTRA)
> +		data->dma_capable = 1;

How many designs will be broken by this change?

...

> +	unsigned int		dma_capable:1;

Note, we use up->dma == NULL for no-DMA, no additional flag is needed.
Just make sure that for your platform you enable DMA by filling that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 05:16:49PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> The Renesas RZ/N1 devices have a modified Synopsys DW UART. The
> modifications are mostly related to the DMA handlnig, and so this patch
> adds support for DMA.

> The RZ/N1 UART must be used with the peripheral as the flow
> controller.

(1)

> This means the DMA length should also be programmed into
> UART registers.

(2)

Hmm... DMA controller vs. Peripheral flow control is about signalling on the HW
level on who starts the transaction. This is programmed in the DMA controller
device driver. Is it what you do in DesignWare DMA patch series?

Ah, I see now, you set fc here.

But still it's not clear how (2) and (1) are related.

> Aside from this, there are some points to note about DMA burst sizes.
> First, DMA must not remove all of the data from the rx FIFO. Otherwise,
> we do not get a 'character timeout' interrupt, and so do not know that
> we should push data up the serial stack. Therefore, we have the rx
> threshold for generating an interrupt set to half the FIFO depth (this
> is the default for 16550A), and set the DMA burst size when reading the
> FIFO to a quarter of the FIFO depth.
> 
> Second, when transmitting data using DMA, the burst size must be limited
> to 1 byte to handle then case when transmitting just 1 byte. Otherwise
> the DMA doesn't complete the burst, and nothing happens.

...

> +/* Offsets for the Renesas RZ/N1 DesignWare specific registers */
> +/* DMA Software Ack */
> +#define RZN1_UART_DMASA			0xa8

Is it specific to Renesas? IIRC it's Synopsys DesignWare register, makes
sense to use appropriate prefix or no prefix.

...

> +#define RZN1_UART_xDMACR_1_WORD_BURST	0
> +#define RZN1_UART_xDMACR_4_WORD_BURST	BIT(1)
> +#define RZN1_UART_xDMACR_8_WORD_BURST	(BIT(1) | BIT(2))

This looks like incorrect use of BIT() macro.
Please, use plain decimal integers. Something like

	1	(0 << 1)
	4	(1 << 1)
	8	(3 << 1)

If I'm mistaken, describe the meaning of each bit there.

...

> +static void rzn1_8250_handle_irq(struct uart_port *port, unsigned int iir)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	struct uart_8250_dma *dma = up->dma;
> +	unsigned char status;

> +	if (up->dma && dma->rx_running) {

With

	if (!)up->dma && dma->rx_running))
		return;

maybe easier to read the rest.

> +		status = port->serial_in(port, UART_LSR);
> +		if (status & (UART_LSR_DR | UART_LSR_BI)) {
> +			/* Stop the DMA transfer */
> +			writel(0, port->membase + RZN1_UART_RDMACR);
> +			writel(1, port->membase + RZN1_UART_DMASA);
> +		}
> +	}
> +}

...

> +	if (d->is_rzn1 && ((iir & 0x3f) == UART_IIR_RX_TIMEOUT))
> +		rzn1_8250_handle_irq(p, iir);

A few years ago it was a discussion about broken timeout on some platforms
with Synopsys DesignWare UART + DMA. Can it be that this is actually required
for all of them that uses same combination of IPs?

...

> +static u32 rzn1_get_dmacr_burst(int max_burst)
> +{

> +	u32 val = 0;

Redundant assignment and variable itself. Use return statements directly.

> +	if (max_burst >= 8)
> +		val = RZN1_UART_xDMACR_8_WORD_BURST;
> +	else if (max_burst >= 4)
> +		val = RZN1_UART_xDMACR_4_WORD_BURST;
> +	else
> +		val = RZN1_UART_xDMACR_1_WORD_BURST;
> +
> +	return val;
> +}

...

> +static int rzn1_dw8250_tx_dma(struct uart_8250_port *p)
> +{
> +	struct uart_port		*up = &p->port;
> +	struct uart_8250_dma		*dma = p->dma;
> +	struct circ_buf			*xmit = &p->port.state->xmit;
> +	int tx_size;
> +	u32 val;
> +
> +	if (uart_tx_stopped(&p->port) || dma->tx_running ||
> +	    uart_circ_empty(xmit))
> +		return 0;
> +
> +	tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);

> +	writel(0, up->membase + RZN1_UART_TDMACR);
> +	val = rzn1_get_dmacr_burst(dma->txconf.dst_maxburst);
> +	val |= tx_size << RZN1_UART_xDMACR_BLK_SZ_OFFSET;
> +	val |= RZN1_UART_xDMACR_DMA_EN;
> +	writel(val, up->membase + RZN1_UART_TDMACR);

Can this be added as a callback to the serial8250_tx_dma()?
Ditto for Rx counterpart.

> +	return serial8250_tx_dma(p);
> +}

...

> +	data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");

Device property API.

>  	/* Always ask for fixed clock rate from a property. */
>  	device_property_read_u32(dev, "clock-frequency", &p->uartclk);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/7] serial: 8250_dw: Use a fallback CPR value if not synthesized

[Miquel Raynal]

Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:02:41
+0200:

> On Thu, Mar 10, 2022 at 05:16:46PM +0100, Miquel Raynal wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> > 
> > This UART controller can be synthesized without the CPR register.
> > In that case, let's use the platform information to provide a CPR value.  
> 
> ...
> 
> > +#include <linux/of_device.h>  
> 
> > +	const struct dw8250_platform_data *plat = of_device_get_match_data(up->port.dev);  
> 
> No. Please use device property APIs.

Are you suggesting the use of CPR DT property? If yes, what is the
point if there is one CPR per SoC? I would argue that DT description is
already quite complex and supporting one or another register is up to
the OS as long as we can map CPR registers to SoCs?

TBH Phil initially introduced a DT property which I turned into a pdata
entry because when I can avoid playing with the bindings, I generally
do so.

Thanks,
Miquèl

Re: [PATCH 5/7] serial: 8250_dw: Add a dma_capable bit to the platform data

[Miquel Raynal]

Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:06:25
+0200:

> On Thu, Mar 10, 2022 at 05:16:48PM +0100, Miquel Raynal wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> > 
> > The CPR register can give the information whether the IP is DMA capable
> > or not. Let's extract this information and use it to discriminate when
> > the DMA can be hooked up or not.  
> 
> ...
> 
> > +	/* If we have a valid fifosize and DMA support, try hooking up DMA */
> > +	if (p->fifosize && data->dma_capable) {  
> 
> > +	if (reg & DW_UART_CPR_DMA_EXTRA)
> > +		data->dma_capable = 1;  
> 
> How many designs will be broken by this change?

My understanding was that CPR registers where always synthesized until
now even though it was not mandatory and that the RZN1 SoC was the
first one to not embed it. My hope was that people using this driver
would have brought "external" CPR support earlier if they needed it,
but I understand this assumption might be wrong.

Anyway, I also hesitated to do something more custom for the RZN1 I'll
try something else.

> 
> ...
> 
> > +	unsigned int		dma_capable:1;  
> 
> Note, we use up->dma == NULL for no-DMA, no additional flag is needed.
> Just make sure that for your platform you enable DMA by filling that.

dma_capable is just a capability the SoC has. It was discovered at
probe time and should be saved to know, later, if DMA can be hooked up
or not. At the time we look at the CPR register we don't yet have DMA
fields populated so its too early to set up->dma to NULL.

Thanks,
Miquèl

Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Miquel Raynal]

Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:25:52
+0200:

> On Thu, Mar 10, 2022 at 05:16:49PM +0100, Miquel Raynal wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> > 
> > The Renesas RZ/N1 devices have a modified Synopsys DW UART. The
> > modifications are mostly related to the DMA handlnig, and so this patch
> > adds support for DMA.  
> 
> > The RZ/N1 UART must be used with the peripheral as the flow
> > controller.  
> 
> (1)
> 
> > This means the DMA length should also be programmed into
> > UART registers.  
> 
> (2)
> 
> Hmm... DMA controller vs. Peripheral flow control is about signalling on the HW
> level on who starts the transaction. This is programmed in the DMA controller
> device driver. Is it what you do in DesignWare DMA patch series?
> 
> Ah, I see now, you set fc here.
> 
> But still it's not clear how (2) and (1) are related.

Both come from the system manual:
(1) table 11.45 "Flow Control Combinations" states that using UART with
DMA requires setting the DMA in the peripheral flow controller mode
regardless of the direction.
(2) chapter 11.6.1.3 "Basic Interface Definitions" explains that the
burst size in the above case must be configured in the peripheral's
register DEST/SRC_BURST_SIZE.

> > Aside from this, there are some points to note about DMA burst sizes.
> > First, DMA must not remove all of the data from the rx FIFO. Otherwise,
> > we do not get a 'character timeout' interrupt, and so do not know that
> > we should push data up the serial stack. Therefore, we have the rx
> > threshold for generating an interrupt set to half the FIFO depth (this
> > is the default for 16550A), and set the DMA burst size when reading the
> > FIFO to a quarter of the FIFO depth.
> > 
> > Second, when transmitting data using DMA, the burst size must be limited
> > to 1 byte to handle then case when transmitting just 1 byte. Otherwise
> > the DMA doesn't complete the burst, and nothing happens.  
> 
> ...
> 
> > +/* Offsets for the Renesas RZ/N1 DesignWare specific registers */
> > +/* DMA Software Ack */
> > +#define RZN1_UART_DMASA			0xa8  
> 
> Is it specific to Renesas? IIRC it's Synopsys DesignWare register, makes
> sense to use appropriate prefix or no prefix.

I have no idea, I can use a more common prefix.

> 
> ...
> 
> > +#define RZN1_UART_xDMACR_1_WORD_BURST	0
> > +#define RZN1_UART_xDMACR_4_WORD_BURST	BIT(1)
> > +#define RZN1_UART_xDMACR_8_WORD_BURST	(BIT(1) | BIT(2))  
> 
> This looks like incorrect use of BIT() macro.
> Please, use plain decimal integers. Something like
> 
> 	1	(0 << 1)
> 	4	(1 << 1)
> 	8	(3 << 1)
> 
> If I'm mistaken, describe the meaning of each bit there.

Matter of taste, I believe, whatever.

> 
> ...
> 
> > +static void rzn1_8250_handle_irq(struct uart_port *port, unsigned int iir)
> > +{
> > +	struct uart_8250_port *up = up_to_u8250p(port);
> > +	struct uart_8250_dma *dma = up->dma;
> > +	unsigned char status;  
> 
> > +	if (up->dma && dma->rx_running) {  
> 
> With
> 
> 	if (!)up->dma && dma->rx_running))
> 		return;
> 
> maybe easier to read the rest.
> 
> > +		status = port->serial_in(port, UART_LSR);
> > +		if (status & (UART_LSR_DR | UART_LSR_BI)) {
> > +			/* Stop the DMA transfer */
> > +			writel(0, port->membase + RZN1_UART_RDMACR);
> > +			writel(1, port->membase + RZN1_UART_DMASA);
> > +		}
> > +	}
> > +}  
> 
> ...
> 
> > +	if (d->is_rzn1 && ((iir & 0x3f) == UART_IIR_RX_TIMEOUT))
> > +		rzn1_8250_handle_irq(p, iir);  
> 
> A few years ago it was a discussion about broken timeout on some platforms
> with Synopsys DesignWare UART + DMA. Can it be that this is actually required
> for all of them that uses same combination of IPs?

I am not sure because I went through the fix for this issue and for me
there are two different things

> 
> ...
> 
> > +static u32 rzn1_get_dmacr_burst(int max_burst)
> > +{  
> 
> > +	u32 val = 0;  
> 
> Redundant assignment and variable itself. Use return statements directly.
> 
> > +	if (max_burst >= 8)
> > +		val = RZN1_UART_xDMACR_8_WORD_BURST;
> > +	else if (max_burst >= 4)
> > +		val = RZN1_UART_xDMACR_4_WORD_BURST;
> > +	else
> > +		val = RZN1_UART_xDMACR_1_WORD_BURST;
> > +
> > +	return val;
> > +}  
> 
> ...
> 
> > +static int rzn1_dw8250_tx_dma(struct uart_8250_port *p)
> > +{
> > +	struct uart_port		*up = &p->port;
> > +	struct uart_8250_dma		*dma = p->dma;
> > +	struct circ_buf			*xmit = &p->port.state->xmit;
> > +	int tx_size;
> > +	u32 val;
> > +
> > +	if (uart_tx_stopped(&p->port) || dma->tx_running ||
> > +	    uart_circ_empty(xmit))
> > +		return 0;
> > +
> > +	tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);  
> 
> > +	writel(0, up->membase + RZN1_UART_TDMACR);
> > +	val = rzn1_get_dmacr_burst(dma->txconf.dst_maxburst);
> > +	val |= tx_size << RZN1_UART_xDMACR_BLK_SZ_OFFSET;
> > +	val |= RZN1_UART_xDMACR_DMA_EN;
> > +	writel(val, up->membase + RZN1_UART_TDMACR);  
> 
> Can this be added as a callback to the serial8250_tx_dma()?
> Ditto for Rx counterpart.

Fair enough.

> 
> > +	return serial8250_tx_dma(p);
> > +}  
> 
> ...
> 
> > +	data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");  
> 
> Device property API.

I'm not sure to get what you mean here again. The Information is in the
device tree, the compatible string already gives us what we need, why
would we need a device property? (or perhaps I misunderstand what
"device property API" means)
> 
> >  	/* Always ask for fixed clock rate from a property. */
> >  	device_property_read_u32(dev, "clock-frequency", &p->uartclk);  
> 


Thanks,
Miquèl

Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Geert Uytterhoeven]

Hi Miquel,

On Thu, Mar 10, 2022 at 5:17 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
>
> The Renesas RZ/N1 devices have a modified Synopsys DW UART. The
> modifications are mostly related to the DMA handlnig, and so this patch
> adds support for DMA.
>
> The RZ/N1 UART must be used with the peripheral as the flow
> controller. This means the DMA length should also be programmed into
> UART registers.
>
> Aside from this, there are some points to note about DMA burst sizes.
> First, DMA must not remove all of the data from the rx FIFO. Otherwise,
> we do not get a 'character timeout' interrupt, and so do not know that
> we should push data up the serial stack. Therefore, we have the rx
> threshold for generating an interrupt set to half the FIFO depth (this
> is the default for 16550A), and set the DMA burst size when reading the
> FIFO to a quarter of the FIFO depth.
>
> Second, when transmitting data using DMA, the burst size must be limited
> to 1 byte to handle then case when transmitting just 1 byte. Otherwise
> the DMA doesn't complete the burst, and nothing happens.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c

> @@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *pdev)
>                 data->msr_mask_off |= UART_MSR_TERI;
>         }
>
> +       data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");

Explicit checks for compatible values are frowned upon if you have
a match table.
Please handle this through of_device.data, cfr. the various quirks.
Please rename "is_rzn1" to something that describes the feature.

> +
>         /* Always ask for fixed clock rate from a property. */
>         device_property_read_u32(dev, "clock-frequency", &p->uartclk);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Geert Uytterhoeven]

Hi Miquel,

CC esmil

On Fri, Mar 11, 2022 at 10:39 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Miquel,
>
> On Thu, Mar 10, 2022 at 5:17 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > The Renesas RZ/N1 devices have a modified Synopsys DW UART. The
> > modifications are mostly related to the DMA handlnig, and so this patch
> > adds support for DMA.
> >
> > The RZ/N1 UART must be used with the peripheral as the flow
> > controller. This means the DMA length should also be programmed into
> > UART registers.
> >
> > Aside from this, there are some points to note about DMA burst sizes.
> > First, DMA must not remove all of the data from the rx FIFO. Otherwise,
> > we do not get a 'character timeout' interrupt, and so do not know that
> > we should push data up the serial stack. Therefore, we have the rx
> > threshold for generating an interrupt set to half the FIFO depth (this
> > is the default for 16550A), and set the DMA burst size when reading the
> > FIFO to a quarter of the FIFO depth.
> >
> > Second, when transmitting data using DMA, the burst size must be limited
> > to 1 byte to handle then case when transmitting just 1 byte. Otherwise
> > the DMA doesn't complete the burst, and nothing happens.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Thanks for your patch!
>
> > --- a/drivers/tty/serial/8250/8250_dma.c
> > +++ b/drivers/tty/serial/8250/8250_dma.c
>
> > @@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *pdev)
> >                 data->msr_mask_off |= UART_MSR_TERI;
> >         }
> >
> > +       data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");
>
> Explicit checks for compatible values are frowned upon if you have
> a match table.
> Please handle this through of_device.data, cfr. the various quirks.

Oops, these are not yet upstream, but present in my tree due to including
support for StarLight, cfr.
https://github.com/esmil/linux/commits/visionfive/drivers/tty/serial/8250/8250_dw.c

But you do already have:

+       { .compatible = "renesas,rzn1-uart", .data = &rzn1_pdata },

since "[PATCH 4/7] serial: 8250_dw: Provide the RZN1 CPR register value".

> Please rename "is_rzn1" to something that describes the feature.
>
> > +
> >         /* Always ask for fixed clock rate from a property. */
> >         device_property_read_u32(dev, "clock-frequency", &p->uartclk);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Fri, 11 Mar 2022 10:51:53 +0100:

> Hi Miquel,
> 
> CC esmil
> 

> > > --- a/drivers/tty/serial/8250/8250_dma.c
> > > +++ b/drivers/tty/serial/8250/8250_dma.c  
> >  
> > > @@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *pdev)
> > >                 data->msr_mask_off |= UART_MSR_TERI;
> > >         }
> > >
> > > +       data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");  
> >
> > Explicit checks for compatible values are frowned upon if you have
> > a match table.
> > Please handle this through of_device.data, cfr. the various quirks.  
> 
> Oops, these are not yet upstream, but present in my tree due to including
> support for StarLight, cfr.
> https://github.com/esmil/linux/commits/visionfive/drivers/tty/serial/8250/8250_dw.c

Oh thanks for pointing it! Too bad that these quirks were not
introduced inside a wider structure, I think it's always a must even if
there is only one parameter there. Anyway, I'll introduce a wider
specific structure and use it.

> But you do already have:
> 
> +       { .compatible = "renesas,rzn1-uart", .data = &rzn1_pdata },
> 
> since "[PATCH 4/7] serial: 8250_dw: Provide the RZN1 CPR register value".
> 
> > Please rename "is_rzn1" to something that describes the feature.
> >  
> > > +
> > >         /* Always ask for fixed clock rate from a property. */
> > >         device_property_read_u32(dev, "clock-frequency", &p->uartclk);  
> 
> Gr{oetje,eeting}s,
> 
>                         Geert


Thanks,
Miquèl

[PATCH] serial: 8250_dw: Use device tree match data

[Emil Renner Berthing]

..rather than multiple calls to of_device_is_compatible().

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
---

Hi Miquel,

> > > > --- a/drivers/tty/serial/8250/8250_dma.c
> > > > +++ b/drivers/tty/serial/8250/8250_dma.c =20
> > > =20
> > > > @@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *p=
> dev)
> > > >                 data->msr_mask_off |=3D UART_MSR_TERI;
> > > >         }
> > > >
> > > > +       data->is_rzn1 =3D of_device_is_compatible(dev->of_node, "rene=
> sas,rzn1-uart"); =20
> > >
> > > Explicit checks for compatible values are frowned upon if you have
> > > a match table.
> > > Please handle this through of_device.data, cfr. the various quirks. =20
> >=20
> > Oops, these are not yet upstream, but present in my tree due to including
> > support for StarLight, cfr.
> > https://github.com/esmil/linux/commits/visionfive/drivers/tty/serial/8250=
> /8250_dw.c
> 
> Oh thanks for pointing it! Too bad that these quirks were not
> introduced inside a wider structure, I think it's always a must even if
> there is only one parameter there. Anyway, I'll introduce a wider
> specific structure and use it.

For reference this is the patch I wrote for the StarFive JH7100 tree.
Feel free to use it or do something better as you see fit.

/Emil

 drivers/tty/serial/8250/8250_dw.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1769808031c5..f564a019a7be 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -37,6 +37,11 @@
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
 
+/* Quirks */
+#define DW_UART_QUIRK_OCTEON		BIT(0)
+#define DW_UART_QUIRK_ARMADA_38X	BIT(1)
+#define DW_UART_QUIRK_SKIP_SET_RATE	BIT(2)
+
 struct dw8250_data {
 	struct dw8250_port_data	data;
 
@@ -389,6 +394,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 	struct device_node *np = p->dev->of_node;
 
 	if (np) {
+		unsigned long quirks = (unsigned long)of_device_get_match_data(p->dev);
 		int id;
 
 		/* get index of serial line, if found in DT aliases */
@@ -396,7 +402,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 		if (id >= 0)
 			p->line = id;
 #ifdef CONFIG_64BIT
-		if (of_device_is_compatible(np, "cavium,octeon-3860-uart")) {
+		if (quirks & DW_UART_QUIRK_OCTEON) {
 			p->serial_in = dw8250_serial_inq;
 			p->serial_out = dw8250_serial_outq;
 			p->flags = UPF_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
@@ -412,9 +418,9 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 			p->serial_out = dw8250_serial_out32be;
 		}
 
-		if (of_device_is_compatible(np, "marvell,armada-38x-uart"))
+		if (quirks & DW_UART_QUIRK_ARMADA_38X)
 			p->serial_out = dw8250_serial_out38x;
-		if (of_device_is_compatible(np, "starfive,jh7100-uart"))
+		if (quirks & DW_UART_QUIRK_SKIP_SET_RATE)
 			p->set_termios = dw8250_do_set_termios;
 
 	} else if (acpi_dev_present("APMC0D08", NULL, -1)) {
@@ -695,10 +701,10 @@ static const struct dev_pm_ops dw8250_pm_ops = {
 
 static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-uart" },
-	{ .compatible = "cavium,octeon-3860-uart" },
-	{ .compatible = "marvell,armada-38x-uart" },
+	{ .compatible = "cavium,octeon-3860-uart", .data = (void *)DW_UART_QUIRK_OCTEON },
+	{ .compatible = "marvell,armada-38x-uart", .data = (void *)DW_UART_QUIRK_ARMADA_38X },
 	{ .compatible = "renesas,rzn1-uart" },
-	{ .compatible = "starfive,jh7100-uart" },
+	{ .compatible = "starfive,jh7100-uart",    .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dw8250_of_match);
-- 
2.35.1

Re: [PATCH 3/7] serial: 8250_dw: Use a fallback CPR value if not synthesized

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 08:01:01PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:02:41
> +0200:
> > On Thu, Mar 10, 2022 at 05:16:46PM +0100, Miquel Raynal wrote:
> > > From: Phil Edworthy <phil.edworthy@renesas.com>

...

> > > +#include <linux/of_device.h>  
> > 
> > > +	const struct dw8250_platform_data *plat = of_device_get_match_data(up->port.dev);  
> > 
> > No. Please use device property APIs.
> 
> Are you suggesting the use of CPR DT property? If yes, what is the
> point if there is one CPR per SoC? I would argue that DT description is
> already quite complex and supporting one or another register is up to
> the OS as long as we can map CPR registers to SoCs?

I'm suggesting to use device property APIs, I'm not talking about ABI.
In this case instead of above you may simply do

#include <linux/property.h>

	const struct dw8250_platform_data *plat = device_get_match_data(up->port.dev);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] serial: 8250_dw: Add a dma_capable bit to the platform data

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 08:13:51PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:06:25
> +0200:
> > On Thu, Mar 10, 2022 at 05:16:48PM +0100, Miquel Raynal wrote:
> > > From: Phil Edworthy <phil.edworthy@renesas.com>

...

> > > +	/* If we have a valid fifosize and DMA support, try hooking up DMA */
> > > +	if (p->fifosize && data->dma_capable) {  
> > 
> > > +	if (reg & DW_UART_CPR_DMA_EXTRA)
> > > +		data->dma_capable = 1;  
> > 
> > How many designs will be broken by this change?
> 
> My understanding was that CPR registers where always synthesized until
> now even though it was not mandatory and that the RZN1 SoC was the
> first one to not embed it. My hope was that people using this driver
> would have brought "external" CPR support earlier if they needed it,
> but I understand this assumption might be wrong.
> 
> Anyway, I also hesitated to do something more custom for the RZN1 I'll
> try something else.

My point is that you have put this requirement to the above conditionals.
Have you checked all supported platforms that announce CPR that that bit
is set when DMA is indeed in use?

AFAIK on Intel SoCs the native UART DMA signalling is not used, the
integration between DMA and UART is done differently because it requires
more signals to connect. It might be that I misread the documentation
and this is not the case and we indeed set that bit as well.

Also, what to do with the platforms that have no CPR but support DMA currently?

...

> > > +	unsigned int		dma_capable:1;  
> > 
> > Note, we use up->dma == NULL for no-DMA, no additional flag is needed.
> > Just make sure that for your platform you enable DMA by filling that.
> 
> dma_capable is just a capability the SoC has. It was discovered at
> probe time and should be saved to know, later, if DMA can be hooked up
> or not. At the time we look at the CPR register we don't yet have DMA
> fields populated so its too early to set up->dma to NULL.

'up->dma == NULL' check will tell you that, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Andy Shevchenko]

On Thu, Mar 10, 2022 at 08:27:43PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:25:52
> +0200:
> > On Thu, Mar 10, 2022 at 05:16:49PM +0100, Miquel Raynal wrote:

...

> > > +/* Offsets for the Renesas RZ/N1 DesignWare specific registers */
> > > +/* DMA Software Ack */
> > > +#define RZN1_UART_DMASA			0xa8  
> > 
> > Is it specific to Renesas? IIRC it's Synopsys DesignWare register, makes
> > sense to use appropriate prefix or no prefix.
> 
> I have no idea, I can use a more common prefix.

It's a register described in Synopsys DesignWare specification. It's not
related to Renesas IP integration.

...

> > > +#define RZN1_UART_xDMACR_1_WORD_BURST	0
> > > +#define RZN1_UART_xDMACR_4_WORD_BURST	BIT(1)
> > > +#define RZN1_UART_xDMACR_8_WORD_BURST	(BIT(1) | BIT(2))  
> > 
> > This looks like incorrect use of BIT() macro.
> > Please, use plain decimal integers. Something like
> > 
> > 	1	(0 << 1)
> > 	4	(1 << 1)
> > 	8	(3 << 1)
> > 
> > If I'm mistaken, describe the meaning of each bit there.
> 
> Matter of taste, I believe, whatever.

Actually no, when one uses BIT() masks it implies that in the datasheet each
bit is meaningful. So, please clarify this and update accordingly.

...

> > > +	data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");
> > 
> > Device property API.
> 
> I'm not sure to get what you mean here again. The Information is in the
> device tree, the compatible string already gives us what we need, why
> would we need a device property? (or perhaps I misunderstand what
> "device property API" means)

Use non-OF APIs, which usually starts with device_property_ or fwnode_.
But Geert already suggested something better.

> > >  	/* Always ask for fixed clock rate from a property. */
> > >  	device_property_read_u32(dev, "clock-frequency", &p->uartclk);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_dw: Use device tree match data

[Andy Shevchenko]

On Fri, Mar 11, 2022 at 03:48:14PM +0100, Emil Renner Berthing wrote:
> ..rather than multiple calls to of_device_is_compatible().

> For reference this is the patch I wrote for the StarFive JH7100 tree.
> Feel free to use it or do something better as you see fit.

>  	if (np) {
> +		unsigned long quirks = (unsigned long)of_device_get_match_data(p->dev);

It can be done outside of the np check with device property APIs in use.
Also it needs to use (uintptr_t) for better coverage.

		unsigned long quirks = (uintptr_t)device_get_match_data(p->dev);

Or use data structure as driver_data.

		const struct ... *data = device_get_match_data(p->dev);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_dw: Use device tree match data

[Miquel Raynal]

Hi Emil,

kernel@esmil.dk wrote on Fri, 11 Mar 2022 15:48:14 +0100:

> ..rather than multiple calls to of_device_is_compatible().
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> ---
> 
> Hi Miquel,
> 
> > > > > --- a/drivers/tty/serial/8250/8250_dma.c
> > > > > +++ b/drivers/tty/serial/8250/8250_dma.c =20  
> > > > =20  
> > > > > @@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *p=  
> > dev)  
> > > > >                 data->msr_mask_off |=3D UART_MSR_TERI;
> > > > >         }
> > > > >
> > > > > +       data->is_rzn1 =3D of_device_is_compatible(dev->of_node, "rene=  
> > sas,rzn1-uart"); =20  
> > > >
> > > > Explicit checks for compatible values are frowned upon if you have
> > > > a match table.
> > > > Please handle this through of_device.data, cfr. the various quirks. =20  
> > >=20
> > > Oops, these are not yet upstream, but present in my tree due to including
> > > support for StarLight, cfr.
> > > https://github.com/esmil/linux/commits/visionfive/drivers/tty/serial/8250=  
> > /8250_dw.c
> > 
> > Oh thanks for pointing it! Too bad that these quirks were not
> > introduced inside a wider structure, I think it's always a must even if
> > there is only one parameter there. Anyway, I'll introduce a wider
> > specific structure and use it.  
> 
> For reference this is the patch I wrote for the StarFive JH7100 tree.
> Feel free to use it or do something better as you see fit.

Thanks for the pointers, I've fetched the three 8250_dw patches from
your tree directly, and will build on top of them!

Cheers,
Miquèl

> 
> /Emil
> 
>  drivers/tty/serial/8250/8250_dw.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 1769808031c5..f564a019a7be 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -37,6 +37,11 @@
>  /* DesignWare specific register fields */
>  #define DW_UART_MCR_SIRE		BIT(6)
>  
> +/* Quirks */
> +#define DW_UART_QUIRK_OCTEON		BIT(0)
> +#define DW_UART_QUIRK_ARMADA_38X	BIT(1)
> +#define DW_UART_QUIRK_SKIP_SET_RATE	BIT(2)
> +
>  struct dw8250_data {
>  	struct dw8250_port_data	data;
>  
> @@ -389,6 +394,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  	struct device_node *np = p->dev->of_node;
>  
>  	if (np) {
> +		unsigned long quirks = (unsigned long)of_device_get_match_data(p->dev);
>  		int id;
>  
>  		/* get index of serial line, if found in DT aliases */
> @@ -396,7 +402,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  		if (id >= 0)
>  			p->line = id;
>  #ifdef CONFIG_64BIT
> -		if (of_device_is_compatible(np, "cavium,octeon-3860-uart")) {
> +		if (quirks & DW_UART_QUIRK_OCTEON) {
>  			p->serial_in = dw8250_serial_inq;
>  			p->serial_out = dw8250_serial_outq;
>  			p->flags = UPF_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
> @@ -412,9 +418,9 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  			p->serial_out = dw8250_serial_out32be;
>  		}
>  
> -		if (of_device_is_compatible(np, "marvell,armada-38x-uart"))
> +		if (quirks & DW_UART_QUIRK_ARMADA_38X)
>  			p->serial_out = dw8250_serial_out38x;
> -		if (of_device_is_compatible(np, "starfive,jh7100-uart"))
> +		if (quirks & DW_UART_QUIRK_SKIP_SET_RATE)
>  			p->set_termios = dw8250_do_set_termios;
>  
>  	} else if (acpi_dev_present("APMC0D08", NULL, -1)) {
> @@ -695,10 +701,10 @@ static const struct dev_pm_ops dw8250_pm_ops = {
>  
>  static const struct of_device_id dw8250_of_match[] = {
>  	{ .compatible = "snps,dw-apb-uart" },
> -	{ .compatible = "cavium,octeon-3860-uart" },
> -	{ .compatible = "marvell,armada-38x-uart" },
> +	{ .compatible = "cavium,octeon-3860-uart", .data = (void *)DW_UART_QUIRK_OCTEON },
> +	{ .compatible = "marvell,armada-38x-uart", .data = (void *)DW_UART_QUIRK_ARMADA_38X },
>  	{ .compatible = "renesas,rzn1-uart" },
> -	{ .compatible = "starfive,jh7100-uart" },
> +	{ .compatible = "starfive,jh7100-uart",    .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
>  	{ /* Sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, dw8250_of_match);


Thanks,
Miquèl