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

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Magnus Damm <magnus.damm@gmail.com>,
	Gareth Williams <gareth.williams.jx@renesas.com>,
	Phil Edworthy <phil.edworthy@renesas.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Milan Stevanovic <milan.stevanovic@se.com>,
	Jimmy Lalande <jimmy.lalande@se.com>,
	Pascal Eberhard <pascal.eberhard@se.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Herve Codina <herve.codina@bootlin.com>,
	Clement Leger <clement.leger@bootlin.com>
Subject: Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA
Date: Fri, 11 Mar 2022 10:39:57 +0100	[thread overview]
Message-ID: <CAMuHMdW0VZsx-zAFr__PPBqKBVyu4v7sAU_yV-ROcTLq10fE6g@mail.gmail.com> (raw)
In-Reply-To: <20220310161650.289387-7-miquel.raynal@bootlin.com>

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