Re: [PATCH 2/2] [RFC] spi: sh-msiof: Handle dmaengine_prep_slave_single() failures gracefully

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

Hi Geert,

Thank you for the patch.

On Wednesday 09 July 2014 12:26:23 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_single() may start failing
> arbitrarily.
> 
> Handle dmaengine_prep_slave_single() failures gracefully by falling back to
> PIO. This requires moving DMA-specific configuration of the MSIOF device
> after the call(s) to dmaengine_prep_slave_single().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Please see below for a small comment.

> ---
>  drivers/spi/spi-sh-msiof.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 4f0f1cbc92ef..2d7c45bbb6f3 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -636,12 +636,6 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv
> *p, const void *tx, dma_cookie_t cookie;
>  	int ret;
> 
> -	/* 1 stage FIFO watermarks for DMA */
> -	sh_msiof_write(p, FCTR, FCTR_TFWM_1 | FCTR_RFWM_1);
> -
> -	/* setup msiof transfer mode registers (32-bit words) */
> -	sh_msiof_spi_set_mode_regs(p, tx, rx, 32, len / 4);
> -
>  	if (tx) {
>  		ier_bits |= IER_TDREQE | IER_TDMAE;
>  		dma_sync_single_for_device(&p->pdev->dev, p->tx_dma_addr, len,
> @@ -650,7 +644,7 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv
> *p, const void *tx, p->tx_dma_addr, len, DMA_TO_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_tx)
> -			return -EIO;
> +			return -EAGAIN;
>  	}
> 
>  	if (rx) {
> @@ -659,8 +653,15 @@ static int sh_msiof_dma_once(struct sh_msiof_spi_priv
> *p, const void *tx, p->rx_dma_addr, len, DMA_FROM_DEVICE,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>  		if (!desc_rx)
> -			return -EIO;
> +			return -EAGAIN;

This isn't a new issue introduced by this patch, but aren't you leaking 
desc_tx here ?

>  	}
> +
> +	/* 1 stage FIFO watermarks for DMA */
> +	sh_msiof_write(p, FCTR, FCTR_TFWM_1 | FCTR_RFWM_1);
> +
> +	/* setup msiof transfer mode registers (32-bit words) */
> +	sh_msiof_spi_set_mode_regs(p, tx, rx, 32, len / 4);
> +
>  	sh_msiof_write(p, IER, ier_bits);
> 
>  	reinit_completion(&p->done);
> @@ -822,6 +823,12 @@ static int sh_msiof_transfer_one(struct spi_master
> *master, copy32(p->tx_dma_page, tx_buf, l / 4);
> 
>  		ret = sh_msiof_dma_once(p, tx_buf, rx_buf, l);
> +		if (ret = -EAGAIN) {
> +			pr_warn_once("%s %s: DMA not available, falling back to PIO\n",
> +				     dev_driver_string(&p->pdev->dev),
> +				     dev_name(&p->pdev->dev));
> +			break;
> +		}
>  		if (ret)
>  			return ret;

-- 
Regards,

Laurent Pinchart