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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Mar 29, 2022 at 05:24:28PM +0200, 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_USR	0x1f /* UART Status Register */
> +#define DW_UART_DMASA	0xa8 /* DMA Software Ack */
> +#define RZN1_UART_TDMACR 0x10c /* DMA Control Register Transmit Mode */
> +#define RZN1_UART_RDMACR 0x110 /* DMA Control Register Receive Mode */

Yep!

Since it seems we may have a v4, I would add a blank line between Synopsys
and custom register groups.

...

>  #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)

Seems the last has no TAB in it. It may require to indent the above as well.
Alternatively you may rename FLOW_CONTROLLER --> FC and add a comment on top
to explain this.

...

> +	const struct dw8250_platform_data *pdata = device_get_match_data(p->dev);

Third one!

> +	/* 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) {

For example,

	unsigned int quirks = data->pdata.quirks;

	if (up->dma && up->dma->rx_running && rx_timeout && quirks & DW_UART_QUIRK_IS_DMA_FC) {

(It's also fine to have that on one line)

> +		status = p->serial_in(p, UART_LSR);
> +		if (status & (UART_LSR_DR | UART_LSR_BI)) {
> +			writel(0, p->membase + RZN1_UART_RDMACR);
> +			writel(1, p->membase + DW_UART_DMASA);
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko