[PATCH v2 00/10] serial: 8250: dw: RZN1 DMA support

[PATCH v2 00/10] serial: 8250: dw: RZN1 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 v5, 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/20220315191255.221473-1-miquel.raynal@bootlin.com/T/#m0ef3323abce3eec961e142bf2fb35e95b9045fc5

Thanks,
Miquèl

Changes in v2:
* Rebased on top of Emil's patches. Added platform data structures
  instead of raw quirk integers in order to provide a CPR value.
* Added includes in dwlib.h even though it's not particularly useful, it
  may help preventing a build error later on if we ever decide to include
  this file from another location.
* Dropped the call to ->tx_dma and instead implemented a callback that
  can be called from serial8250_tx/rx_dma.
* Used the device API instead of the of API.
* Changed the logic about DMA capabilities to avoid breaking existing
  designs.
* Introduced a new quirk related to the flow-control feature of the
  RZ/N1 version of the UART controller when used with DMA.
* Re-arranged the entire series as advised by Andy and Geert.
* Added several preparation patches to ease the review of various
  functional changes.

Miquel Raynal (6):
  serial: 8250: dw: Use the device API
  serial: 8250: dw: Create a more generic platform data structure
  serial: 8250: dw: Check when possible if DMA is effectively supported
  serial: 8250: dma: Allow driver operations before starting DMA
    transfers
  serial: 8250: dw: Introduce an rx_timeout variable in the IRQ path
  ARM: dts: r9a06g032: Fill the UART DMA properties

Phil Edworthy (4):
  serial: 8250: dw: Move the per-device structure
  serial: 8250: dw: Allow to use a fallback CPR value if not synthesized
  serial: 8250: dw: Add support for DMA flow controlling devices
  serial: 8250: dw: Improve RZN1 support

 arch/arm/boot/dts/r9a06g032.dtsi     |  15 ++++
 drivers/tty/serial/8250/8250.h       |  18 +++++
 drivers/tty/serial/8250/8250_dma.c   |   4 +
 drivers/tty/serial/8250/8250_dw.c    | 116 +++++++++++++++++++++------
 drivers/tty/serial/8250/8250_dwlib.c |  11 ++-
 drivers/tty/serial/8250/8250_dwlib.h |  26 ++++++
 6 files changed, 163 insertions(+), 27 deletions(-)

-- 
2.27.0

[PATCH v2 01/10] 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 | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 96a62e95726b..d89731d6c94c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -42,22 +42,6 @@
 #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;
-
-	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..6ffbf502829e 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -2,6 +2,10 @@
 /* Synopsys DesignWare 8250 library header file. */
 
 #include <linux/types.h>
+#include <linux/clk.h>
+#include <linux/notifier.h>
+#include <linux/workqueue.h>
+#include <linux/reset.h>
 
 #include "8250.h"
 
@@ -16,5 +20,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

Re: [PATCH v2 01/10] serial: 8250: dw: Move the per-device structure

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 9:56 PM Miquel Raynal <miquel.raynal@bootlin.com> 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.

...

>  #include <linux/types.h>

> +#include <linux/clk.h>

I have mentioned forward declarations.
So, this can be simply replaced by

struct clk;

> +#include <linux/notifier.h>
> +#include <linux/workqueue.h>

> +#include <linux/reset.h>

Ditto.

struct reset_control;

On top of that, please keep them ordered.

Otherwise it looks good to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] serial: 8250: dw: Move the per-device structure

[Miquel Raynal]

Hi Andy,

andy.shevchenko@gmail.com wrote on Fri, 18 Mar 2022 12:51:29 +0200:

> On Thu, Mar 17, 2022 at 9:56 PM Miquel Raynal <miquel.raynal@bootlin.com> 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.  
> 
> ...
> 
> >  #include <linux/types.h>  
> 
> > +#include <linux/clk.h>  
> 
> I have mentioned forward declarations.

Why do you want forward declarations more than includes?

> So, this can be simply replaced by
> 
> struct clk;
> 
> > +#include <linux/notifier.h>
> > +#include <linux/workqueue.h>  

And why these two should remain but reset and clk be replaced?

> 
> > +#include <linux/reset.h>  
> 
> Ditto.
> 
> struct reset_control;
> 
> On top of that, please keep them ordered.
> 
> Otherwise it looks good to me.
> 


Thanks,
Miquèl

Re: [PATCH v2 01/10] serial: 8250: dw: Move the per-device structure

[Andy Shevchenko]

On Tue, Mar 29, 2022 at 10:10:49AM +0200, Miquel Raynal wrote:
> andy.shevchenko@gmail.com wrote on Fri, 18 Mar 2022 12:51:29 +0200:
> > On Thu, Mar 17, 2022 at 9:56 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:

...

> > > +#include <linux/clk.h>  
> > 
> > I have mentioned forward declarations.
> 
> Why do you want forward declarations more than includes?

Because they will speed up the kernel build and avoid dirtifying the namespace
(less possible collisions).

> > So, this can be simply replaced by
> > 
> > struct clk;
> > 
> > > +#include <linux/notifier.h>
> > > +#include <linux/workqueue.h>  
> 
> And why these two should remain but reset and clk be replaced?

Because these one are being used, clk and reset are not (the pointers
are opaque from the point of view of this header).

> > > +#include <linux/reset.h>  
> > 
> > Ditto.
> > 
> > struct reset_control;
> > 
> > On top of that, please keep them ordered.
> > 
> > Otherwise it looks good to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] serial: 8250: dw: Move the per-device structure

[Miquel Raynal]

Hi Andy,

andy.shevchenko@gmail.com wrote on Tue, 29 Mar 2022 14:11:21 +0300:

> On Tue, Mar 29, 2022 at 10:10:49AM +0200, Miquel Raynal wrote:
> > andy.shevchenko@gmail.com wrote on Fri, 18 Mar 2022 12:51:29 +0200:  
> > > On Thu, Mar 17, 2022 at 9:56 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> 
> ...
> 
> > > > +#include <linux/clk.h>    
> > > 
> > > I have mentioned forward declarations.  
> > 
> > Why do you want forward declarations more than includes?  
> 
> Because they will speed up the kernel build and avoid dirtifying the namespace
> (less possible collisions).
> 
> > > So, this can be simply replaced by
> > > 
> > > struct clk;
> > >   
> > > > +#include <linux/notifier.h>
> > > > +#include <linux/workqueue.h>    
> > 
> > And why these two should remain but reset and clk be replaced?  
> 
> Because these one are being used, clk and reset are not (the pointers
> are opaque from the point of view of this header).

Oh yeah, I forgot that point, thanks for the clarification.

Thanks,
Miquèl

[PATCH v2 02/10] serial: 8250: dw: Use the device API

[Miquel Raynal]

Use the device API instead of the of_* API.
While at it move this operation outside of the if block to reduce the
indentation level.

Cc: Emil Renner Berthing <kernel@esmil.dk>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index d89731d6c94c..28f0dea2ed88 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -376,9 +376,9 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	struct device_node *np = p->dev->of_node;
+	unsigned long quirks = (unsigned long)device_get_match_data(p->dev);
 
 	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 */
-- 
2.27.0

Re: [PATCH v2 02/10] serial: 8250: dw: Use the device API

[Geert Uytterhoeven]

On Thu, Mar 17, 2022 at 6:46 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Use the device API instead of the of_* API.
> While at it move this operation outside of the if block to reduce the
> indentation level.
>
> Cc: Emil Renner Berthing <kernel@esmil.dk>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v2 03/10] serial: 8250: dw: Create a more generic platform data structure

[Miquel Raynal]

Before adding more platform data information, let's turn the quirks
information as being a member of a wider structure. More fields will be
added later.

Cc: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dw.c    | 24 +++++++++++++++++++-----
 drivers/tty/serial/8250/8250_dwlib.h |  4 ++++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 28f0dea2ed88..88fa17882df5 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -19,6 +19,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/workqueue.h>
 #include <linux/notifier.h>
 #include <linux/slab.h>
@@ -376,7 +377,8 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	struct device_node *np = p->dev->of_node;
-	unsigned long quirks = (unsigned long)device_get_match_data(p->dev);
+	const struct dw8250_platform_data *pdata = device_get_match_data(p->dev);
+	unsigned long quirks = pdata ? pdata->quirks : 0;
 
 	if (np) {
 		int id;
@@ -683,13 +685,25 @@ 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 dw8250_octeon_3860_data = {
+	.quirks = DW_UART_QUIRK_OCTEON,
+};
+
+static const struct dw8250_platform_data dw8250_armada_38x_data = {
+	.quirks = DW_UART_QUIRK_ARMADA_38X,
+};
+
+static const struct dw8250_platform_data dw8250_starfive_jh7100_data = {
+	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
+};
+
 static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-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 = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
+	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
 	{ .compatible = "renesas,rzn1-uart" },
-	{ .compatible = "starfive,jh7100-hsuart",  .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
-	{ .compatible = "starfive,jh7100-uart",    .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
+	{ .compatible = "starfive,jh7100-hsuart", .data = &dw8250_starfive_jh7100_data },
+	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dw8250_of_match);
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index 6ffbf502829e..766f80799d13 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -20,6 +20,10 @@ struct dw8250_port_data {
 	u8			dlf_size;
 };
 
+struct dw8250_platform_data {
+	unsigned long quirks;
+};
+
 struct dw8250_data {
 	struct dw8250_port_data	data;
 
-- 
2.27.0

Re: [PATCH v2 03/10] serial: 8250: dw: Create a more generic platform data structure

[Geert Uytterhoeven]

On Thu, Mar 17, 2022 at 6:46 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Before adding more platform data information, let's turn the quirks
> information as being a member of a wider structure. More fields will be
> added later.
>
> Cc: Emil Renner Berthing <kernel@esmil.dk>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v2 03/10] serial: 8250: dw: Create a more generic platform data structure

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 06:46:20PM +0100, Miquel Raynal wrote:
> Before adding more platform data information, let's turn the quirks
> information as being a member of a wider structure. More fields will be
> added later.

...

> +struct dw8250_platform_data {
> +	unsigned long quirks;

unsigned int would be better since it won't change the sizeof() depending on
the hardware / compilation options.

> +};

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 04/10] serial: 8250: dw: Allow to use a fallback CPR value if not synthesized

[Miquel Raynal]

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

DW UART controllers can be synthesized without the CPR register.
In this case, allow to the platform information to provide a CPR value.

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

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 622d3b0d89e7..cef20ca3ad61 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -5,6 +5,7 @@
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/property.h>
 #include <linux/serial_8250.h>
 #include <linux/serial_core.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 *pdata = device_get_match_data(up->port.dev);
 	u32 reg;
 
 	/*
@@ -116,6 +118,8 @@ void dw8250_setup_port(struct uart_port *p)
 	}
 
 	reg = dw8250_readl_ext(p, DW_UART_CPR);
+	if (!reg && pdata)
+		reg = pdata->cpr;
 	if (!reg)
 		return;
 
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index 766f80799d13..704ba91ab09f 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -22,6 +22,7 @@ struct dw8250_port_data {
 
 struct dw8250_platform_data {
 	unsigned long quirks;
+	u32 cpr;
 };
 
 struct dw8250_data {
-- 
2.27.0

Re: [PATCH v2 04/10] serial: 8250: dw: Allow to use a fallback CPR value if not synthesized

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 06:46:21PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> DW UART controllers can be synthesized without the CPR register.
> In this case, allow to the platform information to provide a CPR value.

...

>  void dw8250_setup_port(struct uart_port *p)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(p);
> +	const struct dw8250_platform_data *pdata = device_get_match_data(up->port.dev);

Why not p->dev?

...

>  	reg = dw8250_readl_ext(p, DW_UART_CPR);
> +	if (!reg && pdata)
> +		reg = pdata->cpr;

Perhaps

	if (!reg) {
		if (pdata)
			reg = pdata->cpr;
		dev_dbg("CPR is not available, using %x instead\n", reg);
	}

?

>  	if (!reg)
>  		return;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 05/10] serial: 8250: dw: Check when possible if DMA is effectively supported

[Miquel Raynal]

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

We assume existing designs either provide a valid CPR register or do not
provide any.

Co-developed-by: Phil Edworthy <phil.edworthy@renesas.com>
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 88fa17882df5..2f4a818f787c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -564,8 +564,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->no_dma) {
 		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 cef20ca3ad61..b3ff67401670 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 *pdata = 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;
@@ -136,5 +136,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->no_dma = 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 704ba91ab09f..ff965345fc14 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -39,6 +39,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		no_dma:1;
 };
 
 void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct ktermios *old);
-- 
2.27.0

Re: [PATCH v2 05/10] serial: 8250: dw: Check when possible if DMA is effectively supported

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 06:46:22PM +0100, Miquel Raynal wrote:
> The CPR register can give the information whether the IP is DMA capable
> or not. Let's extract this information when the CPR register is valid
> and use it to discriminate when the DMA cannot be hooked up.
> 
> We assume existing designs either provide a valid CPR register or do not
> provide any.

...

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

My question still remains: Does this bit is _guaranteed_ to be set when this IP
is integrated on all possible DMAs?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/10] serial: 8250: dw: Check when possible if DMA is effectively supported

[Miquel Raynal]

Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Fri, 18 Mar 2022 15:50:12
+0200:

> On Thu, Mar 17, 2022 at 06:46:22PM +0100, Miquel Raynal wrote:
> > The CPR register can give the information whether the IP is DMA capable
> > or not. Let's extract this information when the CPR register is valid
> > and use it to discriminate when the DMA cannot be hooked up.
> > 
> > We assume existing designs either provide a valid CPR register or do not
> > provide any.  
> 
> ...
> 
> > +	if (!(reg & DW_UART_CPR_DMA_EXTRA))
> > +		data->no_dma = 1;  
> 
> My question still remains: Does this bit is _guaranteed_ to be set when this IP
> is integrated on all possible DMAs?

I'll get rid of that entirely, let's just hope there is always DMA
support.

Thanks,
Miquèl

[PATCH v2 06/10] serial: 8250: dma: Allow driver operations before starting DMA transfers

[Miquel Raynal]

One situation where this could be used is when configuring the UART
controller to be the DMA flow controller. This is a typical case where
the driver might need to program a few more registers before starting a
DMA transfer. Provide the necessary infrastructure to support this
case.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250.h     | 18 ++++++++++++++++++
 drivers/tty/serial/8250/8250_dma.c |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index db784ace25d8..d19f24e4d13e 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -17,6 +17,8 @@
 struct uart_8250_dma {
 	int (*tx_dma)(struct uart_8250_port *p);
 	int (*rx_dma)(struct uart_8250_port *p);
+	void (*prepare_tx_dma)(struct uart_8250_port *p);
+	void (*prepare_rx_dma)(struct uart_8250_port *p);
 
 	/* Filter function */
 	dma_filter_fn		fn;
@@ -301,6 +303,22 @@ extern int serial8250_rx_dma(struct uart_8250_port *);
 extern void serial8250_rx_dma_flush(struct uart_8250_port *);
 extern int serial8250_request_dma(struct uart_8250_port *);
 extern void serial8250_release_dma(struct uart_8250_port *);
+
+static inline void serial8250_do_prepare_tx_dma(struct uart_8250_port *p)
+{
+	struct uart_8250_dma *dma = p->dma;
+
+	if (dma->prepare_tx_dma)
+		dma->prepare_tx_dma(p);
+}
+
+static inline void serial8250_do_prepare_rx_dma(struct uart_8250_port *p)
+{
+	struct uart_8250_dma *dma = p->dma;
+
+	if (dma->prepare_rx_dma)
+		dma->prepare_rx_dma(p);
+}
 #else
 static inline int serial8250_tx_dma(struct uart_8250_port *p)
 {
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 890fa7ddaa7f..558d3a2ac65b 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -77,6 +77,8 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 
 	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
 
+	serial8250_do_prepare_tx_dma(p);
+
 	desc = dmaengine_prep_slave_single(dma->txchan,
 					   dma->tx_addr + xmit->tail,
 					   dma->tx_size, DMA_MEM_TO_DEV,
@@ -114,6 +116,8 @@ int serial8250_rx_dma(struct uart_8250_port *p)
 	if (dma->rx_running)
 		return 0;
 
+	serial8250_do_prepare_rx_dma(p);
+
 	desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
 					   dma->rx_size, DMA_DEV_TO_MEM,
 					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
-- 
2.27.0

[PATCH v2 07/10] serial: 8250: dw: Introduce an rx_timeout variable in the IRQ path

[Miquel Raynal]

In a next change we are going to need the same Rx timeout condition as
we already have in the IRQ handling code. Let's just create a boolean to
clarify what this operation does before reusing it.

There is no functional change.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 2f4a818f787c..c27f32f67680 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -228,6 +228,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	struct uart_8250_port *up = up_to_u8250p(p);
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	unsigned int iir = p->serial_in(p, UART_IIR);
+	bool rx_timeout = (iir & 0x3f) == UART_IIR_RX_TIMEOUT;
 	unsigned int status;
 	unsigned long flags;
 
@@ -241,7 +242,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	 * This problem has only been observed so far when not in DMA mode
 	 * so we limit the workaround only to non-DMA mode.
 	 */
-	if (!up->dma && ((iir & 0x3f) == UART_IIR_RX_TIMEOUT)) {
+	if (!up->dma && rx_timeout) {
 		spin_lock_irqsave(&p->lock, flags);
 		status = p->serial_in(p, UART_LSR);
 
-- 
2.27.0

[PATCH v2 08/10] serial: 8250: dw: Add support for DMA flow controlling devices

[Miquel Raynal]

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

DW based controllers like the one on Renesas RZ/N1 must be programmed as
flow controllers when using DMA.

* Table 11.45 of the system manual, "Flow Control Combinations", states
  that using UART with DMA requires setting the DMA in the peripheral
  flow controller mode regardless of the direction.

* Chapter 11.6.1.3 of the system manual, "Basic Interface Definitions",
  explains that the burst size in the above case must be configured in
  the peripheral's register DEST/SRC_BURST_SIZE.

Experiments shown that upon Rx timeout, the DMA transaction needed to be
manually cleared as well.

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index c27f32f67680..edb3f347be8e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -34,14 +34,23 @@
 
 /* Offsets for the DesignWare specific registers */
 #define DW_UART_USR	0x1f /* UART Status Register */
+#define DW_UART_DMASA	0xa8 /* DMA Software Ack */
+#define DW_UART_TDMACR	0x10c /* DMA Control Register Transmit Mode */
+#define DW_UART_RDMACR	0x110 /* DMA Control Register Receive Mode */
 
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
+#define DW_UART_xDMACR_DMA_EN		BIT(0)
+#define DW_UART_xDMACR_1_WORD_BURST	(0 << 1)
+#define DW_UART_xDMACR_4_WORD_BURST	(1 << 1)
+#define DW_UART_xDMACR_8_WORD_BURST	(3 << 1)
+#define DW_UART_xDMACR_BLK_SZ(x)	((x) << 3)
 
 /* 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)
+#define DW_UART_QUIRK_IS_DMA_FLOW_CONTROLLER BIT(3)
 
 static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
 {
@@ -225,6 +234,7 @@ static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
 
 static int dw8250_handle_irq(struct uart_port *p)
 {
+	const struct dw8250_platform_data *pdata = device_get_match_data(p->dev);
 	struct uart_8250_port *up = up_to_u8250p(p);
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	unsigned int iir = p->serial_in(p, UART_IIR);
@@ -252,6 +262,16 @@ static int dw8250_handle_irq(struct uart_port *p)
 		spin_unlock_irqrestore(&p->lock, flags);
 	}
 
+	/* Manually stop the Rx DMA transfer when acting as flow controller */
+	if (up->dma && up->dma->rx_running && rx_timeout && pdata &&
+	    pdata->quirks & DW_UART_QUIRK_IS_DMA_FLOW_CONTROLLER) {
+		status = p->serial_in(p, UART_LSR);
+		if (status & (UART_LSR_DR | UART_LSR_BI)) {
+			writel(0, p->membase + DW_UART_RDMACR);
+			writel(1, p->membase + DW_UART_DMASA);
+		}
+	}
+
 	if (serial8250_handle_irq(p, iir))
 		return 1;
 
@@ -375,6 +395,42 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
 	return param == chan->device->dev;
 }
 
+static u32 dw8250_rzn1_get_dmacr_burst(int max_burst)
+{
+	if (max_burst >= 8)
+		return DW_UART_xDMACR_8_WORD_BURST;
+	else if (max_burst >= 4)
+		return DW_UART_xDMACR_4_WORD_BURST;
+	else
+		return DW_UART_xDMACR_1_WORD_BURST;
+}
+
+static void dw8250_prepare_tx_dma(struct uart_8250_port *p)
+{
+	struct uart_port *up = &p->port;
+	struct uart_8250_dma *dma = p->dma;
+	u32 val;
+
+	writel(0, up->membase + DW_UART_TDMACR);
+	val = dw8250_rzn1_get_dmacr_burst(dma->txconf.dst_maxburst) |
+	      DW_UART_xDMACR_BLK_SZ(dma->tx_size) |
+	      DW_UART_xDMACR_DMA_EN;
+	writel(val, up->membase + DW_UART_TDMACR);
+}
+
+static void dw8250_prepare_rx_dma(struct uart_8250_port *p)
+{
+	struct uart_port *up = &p->port;
+	struct uart_8250_dma *dma = p->dma;
+	u32 val;
+
+	writel(0, up->membase + DW_UART_RDMACR);
+	val = dw8250_rzn1_get_dmacr_burst(dma->rxconf.src_maxburst) |
+	      DW_UART_xDMACR_BLK_SZ(dma->rx_size) |
+	      DW_UART_xDMACR_DMA_EN;
+	writel(val, up->membase + DW_UART_RDMACR);
+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	struct device_node *np = p->dev->of_node;
@@ -409,6 +465,12 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 			p->serial_out = dw8250_serial_out38x;
 		if (quirks & DW_UART_QUIRK_SKIP_SET_RATE)
 			p->set_termios = dw8250_do_set_termios;
+		if (quirks & DW_UART_QUIRK_IS_DMA_FLOW_CONTROLLER) {
+			data->data.dma.txconf.device_fc = 1;
+			data->data.dma.rxconf.device_fc = 1;
+			data->data.dma.prepare_tx_dma = dw8250_prepare_tx_dma;
+			data->data.dma.prepare_rx_dma = dw8250_prepare_rx_dma;
+		}
 
 	} else if (acpi_dev_present("APMC0D08", NULL, -1)) {
 		p->iotype = UPIO_MEM32;
-- 
2.27.0

Re: [PATCH v2 08/10] serial: 8250: dw: Add support for DMA flow controlling devices

[Andy Shevchenko]

On Thu, Mar 17, 2022 at 06:46:25PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> DW based controllers like the one on Renesas RZ/N1 must be programmed as
> flow controllers when using DMA.
> 
> * Table 11.45 of the system manual, "Flow Control Combinations", states
>   that using UART with DMA requires setting the DMA in the peripheral
>   flow controller mode regardless of the direction.
> 
> * Chapter 11.6.1.3 of the system manual, "Basic Interface Definitions",
>   explains that the burst size in the above case must be configured in
>   the peripheral's register DEST/SRC_BURST_SIZE.
> 
> Experiments shown that upon Rx timeout, the DMA transaction needed to be
> manually cleared as well.

...

> +#define DW_UART_TDMACR	0x10c /* DMA Control Register Transmit Mode */
> +#define DW_UART_RDMACR	0x110 /* DMA Control Register Receive Mode */

These are not Synposys ones.

...

> +static u32 dw8250_rzn1_get_dmacr_burst(int max_burst)
> +{
> +	if (max_burst >= 8)
> +		return DW_UART_xDMACR_8_WORD_BURST;
> +	else if (max_burst >= 4)
> +		return DW_UART_xDMACR_4_WORD_BURST;
> +	else
> +		return DW_UART_xDMACR_1_WORD_BURST;
> +}

Redundant 'else' in all cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/10] serial: 8250: dw: Add support for DMA flow controlling devices

[Miquel Raynal]

Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Fri, 18 Mar 2022 16:24:51
+0200:

> On Thu, Mar 17, 2022 at 06:46:25PM +0100, Miquel Raynal wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> > 
> > DW based controllers like the one on Renesas RZ/N1 must be programmed as
> > flow controllers when using DMA.
> > 
> > * Table 11.45 of the system manual, "Flow Control Combinations", states
> >   that using UART with DMA requires setting the DMA in the peripheral
> >   flow controller mode regardless of the direction.
> > 
> > * Chapter 11.6.1.3 of the system manual, "Basic Interface Definitions",
> >   explains that the burst size in the above case must be configured in
> >   the peripheral's register DEST/SRC_BURST_SIZE.
> > 
> > Experiments shown that upon Rx timeout, the DMA transaction needed to be
> > manually cleared as well.  
> 
> ...
> 
> > +#define DW_UART_TDMACR	0x10c /* DMA Control Register Transmit Mode */
> > +#define DW_UART_RDMACR	0x110 /* DMA Control Register Receive Mode */  
> 
> These are not Synposys ones.

Ok

> ...
> 
> > +static u32 dw8250_rzn1_get_dmacr_burst(int max_burst)
> > +{
> > +	if (max_burst >= 8)
> > +		return DW_UART_xDMACR_8_WORD_BURST;
> > +	else if (max_burst >= 4)
> > +		return DW_UART_xDMACR_4_WORD_BURST;
> > +	else
> > +		return DW_UART_xDMACR_1_WORD_BURST;
> > +}  
> 
> Redundant 'else' in all cases.

I'm sorry but dropping the else statement here does not make any
sense. I find it much easier to the eyes the current way.

Thanks,
Miquèl

[PATCH v2 09/10] serial: 8250: dw: Improve RZN1 support

[Miquel Raynal]

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

Renesas RZ/N1 SoC features a slightly modified DW UART.

On this SoC, the CPR register value is known but not synthetized in
hardware. We hence need to provide a CPR value in the platform
data. This version of the controller also relies on acting as flow
controller when using DMA, so we need to provide the
'IS_DMA_FLOW_CONTROLLER' quirk.

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index edb3f347be8e..9b393770d40e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -756,6 +756,11 @@ static const struct dw8250_platform_data dw8250_armada_38x_data = {
 	.quirks = DW_UART_QUIRK_ARMADA_38X,
 };
 
+static const struct dw8250_platform_data dw8250_renesas_rzn1_data = {
+	.quirks = DW_UART_QUIRK_IS_DMA_FLOW_CONTROLLER,
+	.cpr = 0x00012f32,
+};
+
 static const struct dw8250_platform_data dw8250_starfive_jh7100_data = {
 	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
 };
@@ -764,7 +769,7 @@ static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-uart" },
 	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
 	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
-	{ .compatible = "renesas,rzn1-uart" },
+	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
 	{ .compatible = "starfive,jh7100-hsuart", .data = &dw8250_starfive_jh7100_data },
 	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
 	{ /* Sentinel */ }
-- 
2.27.0

[PATCH v2 10/10] 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