Re: [PATCH v2 17/22] mmc: tmio: move TMIO_MASK_{READOP,WRITEOP} handling to correct place

[Wolfram Sang]

On Sat, Nov 25, 2017 at 01:24:52AM +0900, Masahiro Yamada wrote:
> This driver was largely extended by Renesas, but actually used by
> several SoC vendors.  The current code does not work for UniPhier
> SoCs at least.

Do you insist on this paragraph? All people working on the code so far
tried hard to make it work on all devices they had access to. So far, I
can't recall one of the several SoC vendors contacting upstream to get
their support merged (until now, of course) or even to file bugs.
Because I can't guess unknown stuff, I'd prefer to skip this paragraph.

> The DMA mode for UniPhier SoCs failed with the following error message:
>   PIO IRQ in DMA mode!
> 
> For UniPhier SoCs, the TMIO_MASK_{READOP,WRITEOP} are asserted in the
> DMA mode as well.

Sure, that we need to fix! Note that both Renesas DMA drivers also
enable DATAEND and disable RXRDY | TXRQ on their own, too. So, probably
the same issue? And IIUC we can decide now to prepare the irqs like this
in tmio_core because all known DMA engines need it. Or we keep it that
DMA engines set up irqs themselves. More flexible but maybe
over-engineered?

> In fact, the code is very strange.

Yes.

> The TMIO_MASK_{READOP,WRITEOP} IRQs are set as follows:
> 
>     /* Unmask the IRQs we want to know about */
>     if (!_host->chan_rx)
>             irq_mask |= TMIO_MASK_READOP;
>     if (!_host->chan_tx)
>             irq_mask |= TMIO_MASK_WRITEOP;
> 
> At this point, _host->{chan_rx,chan_tx} are _always_ NULL because
> tmio_mmc_request_dma() is called after this code.  Consequently,
> TMIO_MASK_{READOP,WRITEOP} are set whether DMA is used or not.

Yes :( Bummer.

> tmio_mmc_cmd_irq() enables TMIO_MASK_{READOP,WRITEOP}, but never
> disables them.  This does not take care of a case where ->force_pio
> is set, but unset later.

force_pio gets disabled within the same request? I may be overlooking
something but I only see code paths where force_pio gets cleared and a
little later the request gets finished. Can you give me a pointer?

> After all, the correct place to handle those flags is just before
> starting the data transfer.

While I totally agree the code below can be improved for sure, I'd
prefer to keep the design pattern that irqs get disabled once they are
not actively used. Generally spoken, I think it makes sense to keep
regressions on old platforms low. Any chance this can be achieved?
Other than that, fixing/removing this irq_mask handling from probe() is
really good.

> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> ---
> 
> Changes in v2:
>   - Newly added
> 
>  drivers/mmc/host/tmio_mmc_core.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 7d169ed..345e379 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -621,15 +621,19 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host, unsigned int stat)
>  	 */
>  	if (host->data && (!cmd->error || cmd->error == -EILSEQ)) {
>  		if (host->data->flags & MMC_DATA_READ) {
> -			if (host->force_pio || !host->chan_rx)
> +			if (host->force_pio || !host->chan_rx) {
>  				tmio_mmc_enable_mmc_irqs(host, TMIO_MASK_READOP);
> -			else
> +			} else {
> +				tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_READOP);
>  				tasklet_schedule(&host->dma_issue);
> +			}
>  		} else {
> -			if (host->force_pio || !host->chan_tx)
> +			if (host->force_pio || !host->chan_tx) {
>  				tmio_mmc_enable_mmc_irqs(host, TMIO_MASK_WRITEOP);
> -			else
> +			} else {
> +				tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_WRITEOP);
>  				tasklet_schedule(&host->dma_issue);
> +			}
>  		}
>  	} else {
>  		schedule_work(&host->done);
> @@ -1285,12 +1289,6 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
>  	_host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK);
>  	tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
>  
> -	/* Unmask the IRQs we want to know about */
> -	if (!_host->chan_rx)
> -		irq_mask |= TMIO_MASK_READOP;
> -	if (!_host->chan_tx)
> -		irq_mask |= TMIO_MASK_WRITEOP;
> -
>  	_host->sdcard_irq_mask &= ~irq_mask;
>  
>  	if (_host->native_hotplug)
> -- 
> 2.7.4
>

Attachment: signature.asc
Description: PGP signature