Re: [PATCH v3 10/13] spi: cadence-quadspi: enable PHY for direct reads and indirect writes

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

>>> +static bool cqspi_use_phy(struct cqspi_flash_pdata *f_pdata,
>>> +			  const struct spi_mem_op *op)
>>> +{
>>> +	return f_pdata->use_phy && op->data.nbytes > 16 &&
>> Why is the check looking for 16 here, and 1kiB above?
>
> Direct reads have very little per-op overhead, so enabling PHY is
> beneficial even for relatively small transfers. (> 16)
>
> Indirect writes, on the other hand, incur significantly higher setup
> cost, resulting in much larger break point. (> 1kiB)

Ok, this probably deserves to be defined, eg.

#define MIN_DIR_READ/INDIR_WRITE_LEN_FOR_PHY

And a comment would be very welcome to explain these arbitrary choices.

They can probably be improved further later.

>>>   	ddev = cqspi->rx_chan->device->dev;
>>>   	dma_dst = dma_map_single(ddev, buf, len, DMA_FROM_DEVICE);
>>>   	if (dma_mapping_error(ddev, dma_dst)) {
>>>   		dev_err(dev, "dma mapping failed\n");
>>>   		return -ENOMEM;
>>>   	}
>>> -	tx = dmaengine_prep_dma_memcpy(cqspi->rx_chan, dma_dst, dma_src,
>>> -				       len, flags);
>>> +	tx = dmaengine_prep_dma_memcpy(cqspi->rx_chan, dma_dst, dma_src, len,
>>> +				       flags);
>> Not related to the change, isn't it?
>
> Yeah, not related I'll leave this untouched. However, the changes above
> and below are related and belong together in the same patch.

They should likely be done in a preparation patch.

Thanks,
Miquèl