diff mbox series

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

Message ID 1511540697-27387-18-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series mmc: tmio: various fixes and cleanups | expand

Commit Message

Masahiro Yamada Nov. 24, 2017, 4:24 p.m. UTC
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.

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.

In fact, the code is very strange.

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.

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.

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

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

Changes in v2:
  - Newly added

 drivers/mmc/host/tmio_mmc_core.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

-- 
2.7.4

Comments

Masahiro Yamada Jan. 17, 2018, 4:45 p.m. UTC | #1
Hi Wolfram,


2018-01-16 17:01 GMT+09:00 Wolfram Sang <wsa@the-dreams.de>:
> 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.


I dropped this in v3.



>> 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?


OK, I cleaned up RXRDY | TXRQ in renesas drivers, too.

It is probably possible to cleanup DATAEND,
but I did not touch it for now.

As you may know, the original IP from Panasonic
did not support internal DMAC.

Renesas extended the IP by itself for the internal DMAC.

Panasonic did so in a different way.  So, the DMA engine HW
is completely different, and the current core is not
flexible enough for UniPhier SoCs because
our DMA engines added own interrupt masks.

I will consider how to handle it later,
but I do not see a reason to do it in this series.


>> 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?


I cleaned up force_pio, but in a separate patch
because it is just loosely related.


>> 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.

>


I could not understand this suggestion.
Please let me know if v3 is still a problem.



-- 
Best Regards
Masahiro Yamada
diff mbox series

Patch

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)