Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

Thank you for the patch.

On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote:
> As typically a shmobile SoC has less DMA channels than devices that can use
> DMA, we may want to prioritize access to the DMA channels in the future.
> This means that dmaengine_prep_slave_sg() may start failing arbitrarily.
> 
> Handle dmaengine_prep_slave_sg() failures gracefully by falling back to
> PIO.

Just a random thought, do you think there would be performance-sensitive cases 
where failing the transfer completely would be better than using PIO ?

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

-EAGAIN might not be the best error code to return in this case, as it would 
imply that the caller should just call rspi_dma_transfer() again. On the other 
hand I don't see another error code that would be a perfect match, so

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 38fd938d6360..c850dfdfa9e3 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_tx)
> -			return -EIO;
> +			goto no_dma;
> 
>  		irq_mask |= SPCR_SPTIE;
>  	}
> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_rx)
> -			return -EIO;
> +			goto no_dma;
> 
>  		irq_mask |= SPCR_SPRIE;
>  	}
> @@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi,
> struct sg_table *tx, enable_irq(rspi->rx_irq);
> 
>  	return ret;
> +
> +no_dma:
> +	pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
> +		     dev_driver_string(&rspi->master->dev),
> +		     dev_name(&rspi->master->dev));
> +	return -EAGAIN;
>  }
> 
>  static void rspi_receive_init(const struct rspi_data *rspi)
> @@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi,
> 
>  	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
>  		/* rx_buf can be NULL on RSPI on SH in TX-only Mode */
> -		return rspi_dma_transfer(rspi, &xfer->tx_sg,
> -					 xfer->rx_buf ? &xfer->rx_sg : NULL);
> +		ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
> +					xfer->rx_buf ? &xfer->rx_sg : NULL);
> +		if (ret != -EAGAIN)
> +			return ret;
>  	}
> 
>  	ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len);
> @@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer) {
>  	int ret;
> 
> -	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
> -		return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
> +	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> +		ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL);
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> 
>  	ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len);
>  	if (ret < 0)
> @@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer)
> 
>  static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer
> *xfer) {
> -	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer))
> -		return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
> +	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> +		int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> 
>  	return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len);
>  }

-- 
Regards,

Laurent Pinchart