Re: [PATCH 5/5] dmaengine: dw: Add DMA-channels mask cell support

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

On Thu, Jul 30, 2020 at 06:45:45PM +0300, Serge Semin wrote:
> DW DMA IP-core provides a way to synthesize the DMA controller with
> channels having different parameters like maximum burst-length,
> multi-block support, maximum data width, etc. Those parameters both
> explicitly and implicitly affect the channels performance. Since DMA slave
> devices might be very demanding to the DMA performance, let's provide a
> functionality for the slaves to be assigned with DW DMA channels, which
> performance according to the platform engineer fulfill their requirements.
> After this patch is applied it can be done by passing the mask of suitable
> DMA-channels either directly in the dw_dma_slave structure instance or as
> a fifth cell of the DMA DT-property. If mask is zero or not provided, then
> there is no limitation on the channels allocation.
> 
> For instance Baikal-T1 SoC is equipped with a DW DMAC engine, which first
> two channels are synthesized with max burst length of 16, while the rest
> of the channels have been created with max-burst-len=4. It would seem that
> the first two channels must be faster than the others and should be more
> preferable for the time-critical DMA slave devices. In practice it turned
> out that the situation is quite the opposite. The channels with
> max-burst-len=4 demonstrated a better performance than the channels with
> max-burst-len=16 even when they both had been initialized with the same
> settings. The performance drop of the first two DMA-channels made them
> unsuitable for the DW APB SSI slave device. No matter what settings they
> are configured with, full-duplex SPI transfers occasionally experience the
> Rx FIFO overflow. It means that the DMA-engine doesn't keep up with
> incoming data pace even though the SPI-bus is enabled with speed of 25MHz
> while the DW DMA controller is clocked with 50MHz signal. There is no such
> problem has been noticed for the channels synthesized with
> max-burst-len=4.

...

> +	if (dws->channels && !(dws->channels & dwc->mask))

You can drop the first check if...

> +		return false;

...

> +	if (dma_spec->args_count >= 4)
> +		slave.channels = dma_spec->args[3];

...you apply sane default here or somewhere else.

...

> +		    fls(slave.channels) > dw->pdata->nr_channels))

Does it really make sense?

I think it can also be simplified to faster op, i.e.
	BIT(nr_channels) < slave.channels
(but check for off-by-one errors)

...

> + * @channels:	mask of the channels permitted for allocation (zero
> + *		value means any)

Perhaps on one line?

-- 
With Best Regards,
Andy Shevchenko