RFC: LiteSDCard: Add support for DMA/data completion interrupts

[gsomlo]

This is the new/reworked version of #15, which should offer a cleaner sequence of steps getting from the current state to the desired end goal of including dma completion support.

Depends on enjoy-digital/litex#1787 being applied.

The important patches are 5/10 ("fix cmd irq/completion sequencing") and 6/10 ("switch cmd_done irq to edge-triggered"); the latter requires enjoy-digital/litex#1787 to be applied (breaking compatibility with the way LiteSDCard/Litex gateware operated before that point).

[gsomlo]

@paulusmack : thanks for the testing and the feedback! I'm moving the conversation to this PR instead, since it has the latest and clearest patch sequencing (although the end state is the same as in PR #15 ).

I will rework https://github.com/litex-hub/linux/pull/16/commits/14916705b5e4263a3d1f7a36da15fec4f5243ae1to address your observation by writing back the value of pending immediately, before doing any actual irq handling, to avoid races.

In the mean time, I've done some more comprehensive speed tests of my own. I have a slower/older 32GB SanDisk card, and a newer/faster (SanDisk "Ultra") 32-bit card. I ran tests on 4-core Rocket CPUs running at 50MHz (on a nexys video) and at 100MHz (on a stlnv7324 v2, kintex-based board).

I used your time (rather than my original, date) based method:

# for read speed (use *real* value for overall elapsed time):
mount /dev/mmcblk0p1 /mnt; time cp /mnt/testfile ./foo; umount /mnt

# for write speed (add together the *real* values for cp and umount for total elapsed time):
mount /dev/mmcblk0p1 /mnt; time cp foo /mnt/; time umount /mnt

I had a 32.4 MByte (33995776 bytes) file I used for testing.

The transfer speeds are in KBytes per second:

+-------------+---------------------------+---------------------------+
|             |       50MHz Rocket        |       100MHz Rocket       |
| test/commit |  SlowCard   |  FastCard   |  SlowCard   |  FastCard   |
|             |  rd  |  wr  |  rd  |  wr  |  rd  |  wr  |  rd  |  wr  |
+-------------+------+------+------+------+------+------+------+------+
| 1.  f1557fa |  731 |  696 | 1212 |  719 |  964 |  876 | 2278 | 1403 |
| 2.  66074a0 |  678 |  683 | 1182 |  620 |  944 |  765 | 2127 | 1168 |
| 3.  b695cee |  682 |  661 | 1139 |  614 |  949 |  883 | 2272 | 1166 |
| 4. heur.cmd |  718 |  676 | 1169 |  732 |  968 | 1090 | 2434 | 1384 |
| 5. poll 5us | 1126 |  184 |  374 |  --- |  388 |  191 |  388 |  328 |
| 6. poll 0us |  829 |  949 | 1921 |  --- | 1017 | 1435 | 3668 |  --- |  
+-------------+------+------+------+------+------+------+------+------+

The following commits were tested:

1. f1557fa : "heuristically" selected commands, no data completion irq handling (essentially the current status-quo, only using edge-triggered gateware)
2. 66074a0 : all commands subject to irq handling (remove "heuristic" selection), still no data completion irq handling
3. b695cee : full patch series applied, all commands and data completion handled via irq
4. patch 66074a0 reverted after applying full series (we're back to "heuristically" picking commands, but are also handling data completion via irq)
5. polling with default #define SD_SLEEP_US 5 interval (comment out irq property in DT node)
6. polling with aggressive #define SD_SLEEP_US 0 interval (edited in driver source)

It appears that keeping the heuristics for comand completion and adding data completion IRQ handling is competitive with (sometimes faster than) the status quo, particularly as we increase the cpu clock frequency.

@Dolu1990 - I removed the "heuristics" (patch 66074a0) since it apparently made a positive difference in your case (naxriscv 64bit). Any chance you could run a similar set of tests to what I did here (and what @paulusmack did in #15 (comment)) and report back?

I'm guessing there's going to be variability since we're all using different SDCard models. Also, I'm 10-20 KB/s faster or slower results if I repeat the same test multiple times, so this is definitely not super precise :)

It would be interesting if any one of us can figure out a way to dial up the cpu frequency past 100MHz to see what that trend would look like...

EDIT: added polling tests for 50MHz rocket: "fast" card writes errored out hard, and could not complete; "slow" card got really slow for writes with 5us polling interval, did fine in "aggressive" mode. It's late and I'm getting sleepy, so I'll hook up the 100MHz capable dev board tomorrow and see what I can get away with on polling there as well.

EDIT2: added polling tests for 100MHz rocket: "slow card" worked well, "fast card" had spectacularly fast reads, but face planted when trying to write.

Editorial opinion: While throwing around all these numbers, we should remember that raw sdcard transfer speeds are optimized at the expense of keeping a cpu core spinning in a polling loop, unless we use IRQs to free it up. Yes, there's an overhead of dealing with an interrupt, but it's fixed in terms of opcodes, which makes it a delay that becomes a smaller fraction of the transfer speed as cpu frequencies scale up...

[paulusmack]

Hmmm, for row 4 in your table (and also for row 1), we are still taking an interrupt for every command, even if litex_mmc_send_cmd doesn't wait for it, which seems like wasted CPU cycles. Surely it takes fewer CPU cycles to do two MMIO writes to disable/enable the interrupt than it does to take an interrupt and handle it.

[paulusmack]

Also, I wonder why we need a data completion interrupt for writes. I don't see how the command can complete before all the data has been transferred, for a write to the card. So the command completion interrupt should be sufficient, for writes. In fact, for writes, we need neither the interrupt nor the polling of the completion status. That should simplify the code in litex_mmc_request() a bit.

[gsomlo]

Hmmm, for row 4 in your table (and also for row 1), we are still taking an interrupt for every command, even if litex_mmc_send_cmd doesn't wait for it, which seems like wasted CPU cycles. Surely it takes fewer CPU cycles to do two MMIO writes to disable/enable the interrupt than it does to take an interrupt and handle it.

You're right, I guess if we're going to keep the "heuristics", we might indeed want to toggle the command completion interrupt. @Dolu1990 reported his transfer speeds improving when we got rid of them, so I just went for it, and "putting them back" for this specific test was an incomplete operation on my part -- good catch! :)

Also, I wonder why we need a data completion interrupt for writes. I don't see how the command can complete before all the data has been transferred, for a write to the card. So the command completion interrupt should be sufficient

I'll add some pr_info() statements and run some more tests that way, see if anything spins where we think it shouldn't. @Dolu1990 observed that the original upstream driver (with selective/heuristic commands-only irq handling) got "faster" when dialing SD_SLEEP_US down to 0, which to me indicated we're spinning a significant/measurable amount in places where we should be using IRQs instead.

I'm not exactly sure how the apparent "concurrency" between commands and data DMA transfers overlaps. But the driver (in its current, unmodified-upstream state) does things in the following order:

1. set up data DMA (if applicable)
2. set up the command (with or without irq handling, as per the heuristic, which says "yes" to irq handling if data is transfered)
3. wait for completion of command (completion queue or spinning/polling)
4. if data, wait for its completion (spinning, no irq, hence the SD_SLEEP_US thing, which obviously comes into play at least for one, if not both xfer directions, as per @Dolu1990)

So I didn't want to (or think I needed to) worry about which (command or data xfer) finishes first: their respective completion queues would get triggered by the respective data/command IRQ, and when I got there I would call wait_for_completion(), which would either yield the cpu or keep going right away, depending on whether the IRQ I'm waiting for did or didn't yet fire.

@enjoy-digital might be able to shed some light on whether we're guaranteed for commands to always finish before write DMA transfers (but not reads). I'll run some empirical tests on my end, but if I don't find any evidence of writes spinning past command completion, I don't have enough insight to equate that with "evidence of absence" :)

EDIT: I ran some tests on 50MHz Rocket (nexys video), by unwinding the readx_poll_timeout() loops used by the command-completion portion (https://github.com/litex-hub/linux/blob/litex-rebase/drivers/mmc/host/litex_mmc.c#L108)
and by the data-completion polling (https://github.com/litex-hub/linux/blob/litex-rebase/drivers/mmc/host/litex_mmc.c#L183), and adding counters for how many times each of them spins.

With (heuristic command-only) IRQs, nothing spins, for me, ever. With polling (no irq support), only command completion spins, data completion never does (neither on reads, nor on writes).

@Dolu1990 -- can you try the original unmodified gateware and upstream driver with the spin counter patch applied and let us know what you see?

@enjoy-digital -- off the top of your head, is it possible for a dma transfer to outlast a command that's started concurrently? I can probably dig into the gateware HDL sources and try to figure this out, but maybe you already know the answer :)

[paulusmack]

Regarding DMA completion on writes, it doesn't seem logically possible to me for the card to claim that a write command has been completed when it hasn't yet received the data to write. I suppose ultimately it depends on what the SD card specification says about the precise meaning of the command completion status.

[gsomlo]

On Thu, Oct 19, 2023 at 06:23:54PM -0700, Paul Mackerras wrote: it doesn't seem logically possible to me for the card to claim that a write command has been completed when it hasn't yet received the data to write.

I (hopefully) never claimed that :) First there's the DMA transfer setup (write base address, count, dma start register). Then there's the actual command setup (and I presume somehow the DMA FSM waits for some signal issued by the command FSM before any actual data starts being transfered over DMA, but not sure about this part). Next we wait for the command to complete (polling its completion register, or sleeping on the command_done completion queue until an interrupt wakes us). *Finally*, we "wait" for the DMA data xfer completion, which in the case of RocketChip based Litex is *always already complete* by the time the command completes, i.e. no further spinning on completing the DMA data xfer happens in my case, whether it's a read or write. Apparently there's a way for the DMA xfer to be still on-going when the command portion of the entire transaction is completed, I just can't seem to reproduce it on my setup.

[paulusmack]

Apparently there's a way for the DMA xfer to be still on-going when the command portion of the entire transaction is completed, I just can't seem to reproduce it on my setup.

I could understand that for a read, but for a write that would be bizarre.

I have never actually seen it either. Using interrupts and completions means that the time from when the command is complete and the interrupt is raised, to when we are actually checking for DMA completion, is at least 120 microseconds on my setup, more than enough time for transferring 512 bytes via DMA.

[paulusmack]

Looking at the driver, I see there are four call sites where we call litex_mmc_send_cmd() with transfer == SD_CTL_DATA_XFER_NONE and only one call site where transfer can possibly be something else.

That suggests to me that we could move the parts of litex_mmc_send_cmd() that have to do with data handling out to that one call site. Then we could split litex_mmc_send_cmd() into two versions; one for the four call sites that where we are never transferring data, and a separate copy for the other call site that can transfer data. Then we make the first copy always do polling and the second copy always use an interrupt for command completion. That way we get good performance but we don't need any explicit heuristic in litex_mmc_send_cmd().

I'll do a patch to show what I'm thinking of.

[gsomlo]

Looking at the driver, I see there are four call sites where we call litex_mmc_send_cmd() with transfer == SD_CTL_DATA_XFER_NONE and only one call site where transfer can possibly be something else.

That suggests to me that we could move the parts of litex_mmc_send_cmd() that have to do with data handling out to that one call site. [...]

I'll do a patch to show what I'm thinking of.

How about starting on top of commit 1f7136d and then modifying the litex_mmc_send_cmd() signature to add a Boolean use_irq argument? Then you can decide on the heuristic right at the call site itself. Is that along the lines of what you had in mind?

Either way, I'd like to hear from @Dolu1990 before we really decide on any changes, since his NaxRiscV experiments apparently did run into scenarios where data completion occurred after command completion, and I'd like to get a better feeling for why/how that happened...

[Dolu1990]

Hi,

So i just did some tests using dual core Nax 16 KB i$ 16KB d$ each, 100mhz + 256KB L2
Using time cp test of 32 MB :

- Vanilla 
  - W 468  R 936
- Vanilla + sleep 0 
  - W 2100 R 4551
- this patch (b695ceef)
  - W 1733 R 3343

I did checked again with the logic analyser. For read test all seems good, i don't see the CPU waiting on timer tick anymore.
For write test i observed the following behaviour which is suspect ? Or maybe it is wanted :)

static int litex_mmc_send_cmd(struct litex_mmc_host *host,
			      u8 cmd, u32 arg, u8 response_len, u8 transfer)
	...		      
	/* Send command */
	litex_write32(host->sdcore + LITEX_CORE_CMDARG, arg);
	litex_write32(host->sdcore + LITEX_CORE_CMDCMD,
		      cmd << 8 | transfer << 5 | response_len);
	litex_write8(host->sdcore + LITEX_CORE_CMDSND, 1);

    trace(1);
	if (host->irq > 0)
		wait_for_completion(&host->cmd_done);
    trace(2);
    
    
static irqreturn_t litex_mmc_interrupt(int irq, void *arg)
{
	struct mmc_host *mmc = arg;
	struct litex_mmc_host *host = mmc_priv(mmc);
	u32 pending = litex_read32(host->sdirq + LITEX_IRQ_PENDING);
	u32 handled = 0;
    trace(7);    
   ...

You can see bellow that trace(1) is followed by two interrupt trace(7) before reaching trace(2) :

Is it wanted ?

[gsomlo]

@Dolu1990, I think your two interrupts might be the data-dma completion irq happening before your command completion irq which then wakes up the cmd_done completion wait. I think that's normal But the problem I thought we're trying to solve is spinning while waiting for data completion after waking up from the cmd_done completion (the only place I'd expect the SD_SLEEP_US value to make a difference, here (in the vanilla, pre-modification kernel).

Can you try testing for reads and writes with this patch applied to the original unmodified litex-rebase tree on the unmodified (level-triggered) irq gateware? (feel free to modify the pr_warn() calls with your own trace() if that helps visualize the results better)

I'd like to know if gls2_readx_poll_timeout() spins at all for you or not. gls1_readx_poll_timeout() should never spin if you're using IRQs for command completion.

If gls2_readx... does spin, you're completing a command before the data DMA operation is finished, and in that case it would make sense to

1. figure out why
2. decide if 1 is a problem and fix it, or add support for data completion IRQs (the whole point of this patch series) :)

[Dolu1990]

So tested your https://github.com/litex-hub/linux/files/13056696/foo.patch.txt with non of the modifications from this PR, with SLEEP = 5 (default)

gls1_readx_poll_timeout() should never spin if you're using IRQs for command completion.

From what i can mesure, that's right. it doesn't spin when irq are used in litex_mmc_send_cmd

if (host->irq > 0 &&
	    (transfer != SD_CTL_DATA_XFER_NONE ||
	     response_len == SD_CTL_RESP_SHORT_BUSY)) {
		reinit_completion(&host->cmd_done);
		litex_write32(host->sdirq + LITEX_IRQ_ENABLE,
			      SDIRQ_CMD_DONE | SDIRQ_CARD_DETECT);
		wait_for_completion(&host->cmd_done);
        trace(0);
        ret = litex_mmc_sdcard_wait_done2(host->sdcore + LITEX_CORE_CMDEVT, dev); //This guy never spin
        trace(1);
} else {
          ret = litex_mmc_sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT, dev); // There it spin, seems always 1 time
 }

I'd like to know if gls2_readx_poll_timeout() spins at all for you or not.

never spin.

I thought we're trying to solve is spinning while waiting for data completion after waking up from the cmd_done completion

When did you observed that behaviour ? From my observation long ago, that the driver was waiting on kernel ticks before doing stuff ? (i'm just asking to be sure there is no missunderstanding)

[gsomlo]

if (host->irq > 0 &&
	    (transfer != SD_CTL_DATA_XFER_NONE ||
	     response_len == SD_CTL_RESP_SHORT_BUSY)) {
		[...]
        trace(0);
        ret = litex_mmc_sdcard_wait_done2(host->sdcore + LITEX_CORE_CMDEVT, dev); //This guy never spin
        trace(1);
} else {
          ret = litex_mmc_sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT, dev); // There it spin, seems always 1 time
 }

Oh, it makes sense now -- you're spinning while waiting for command completion during commands that weren't heuristically selected to use IRQs :)

This heuristic selectivity threw me off, and I (WRONGLY!) thought you must be spinning while waiting for DMA...

Given this new knowledge, I think we should

1. forget about integration/soc/add_sdcard: switch to edge-triggered cmd-done irq enjoy-digital/litex#1787
2. go with @paulusmack's idea of making the heuristic a Boolean argument to litex_mmc_send_cmd().
3. finally, we should then select which call sites should receive a true vs. false value for that argument

Can you print out the command number that's causing you to spin (i.e., where the heuristic to use IRQ doesn't apply and perhaps should)?

In conclusion, I think we can also ditch all commits beyond 1f7136d and leave the IRQ setup and handling as-is, and just "tune" the heuristic instead... :)

Apologies to everyone for the misunderstanding!

[Dolu1990]

The comand which make it spin while not using IRQ is 23 (decimal) => SET_BLOCK_COUNT =>

"Defines the number of blocks which are
going to be transferred in the immediatedly
succeeding multiple block read or write
command. "

So, to me it seems that, if we don't use IRQ for a given command, for performance reasons we shouldn't use readx_poll_timeout , but instead something which does some active pulling with eventualy a "sleep(0)" to allow rescheduling to other tasks.

[gsomlo]

On Fri, Oct 20, 2023 at 12:54:41PM -0700, Dolu1990 wrote: The comand which make it spin while not using IRQ is 23 (decimal) => SET_BLOCK_COUNT => [...] So, to me it seems that, if we don't use IRQ for a given command, for performance reasons we shouldn't use readx_poll_timeout , but instead something which does some active pulling with eventualy a "sleep(0)" to allow rescheduling to other tasks.

That or we should add `... || cmd == MMC_SET_BLOCK_COUNT` to the irq selection heuristic :)

[Dolu1990]

I did a test with the following :

	if (host->irq > 0 &&
	    (transfer != SD_CTL_DATA_XFER_NONE ||
	     response_len == SD_CTL_RESP_SHORT_BUSY)) {
		reinit_completion(&host->cmd_done);
		litex_write32(host->sdirq + LITEX_IRQ_ENABLE,
			      SDIRQ_CMD_DONE | SDIRQ_CARD_DETECT);
		wait_for_completion(&host->cmd_done);
        trace(0);
        ret = litex_mmc_sdcard_wait_done_sleep5(host->sdcore + LITEX_CORE_CMDEVT, dev);
        trace(1);
	} else {
        ret = litex_mmc_sdcard_wait_done_sleep0(host->sdcore + LITEX_CORE_CMDEVT, dev, cmd);
    }

And that solve the performance issue with everything else vanilla

That or we should add ... || cmd == SET_BLOCK_COUNT to the irq selection heuristic :)

I don't agree, it isn't a heuristic issue, i think it is more that sending a command and waiting on the next timer IRQ before doing forward progress seems weird to me, unless you expect the command to take ages (like 100ms+)

[gsomlo]

On Fri, Oct 20, 2023 at 01:30:28PM -0700, Dolu1990 wrote: That or we should add ... || cmd == SET_BLOCK_COUNT to the irq selection heuristic :) I don't agree, it isn't a heuristic issue, i think it is more that sending a command and waiting on the next timer IRQ before doing forward progress seems weird to me, unless you expect the command to take ages (like 100ms+)

I'm instinctively paranoid about things that spin in a tight loop (that are *not* spinlocks), waiting for "slow" hardwre to do stuff when IRQ support is available... :) Looks like maybe my instincts are wrong for CPUs that run at such low frequencies that things like an SDCard command would run within timeframes comparable to CPU opcodes, and wehere the idea of selectively *not* using IRQs for that type of hardware is a bona fide optimization of overall system performance... :)

[Dolu1990]

I think here is maybe a good alternative to fully blocking busy loop :
https://github.com/torvalds/linux/blob/9c5d00cb7b6bbc5a7965d9ab7d223b5402d1f02c/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L307

static int mtk_mdio_busy_wait(struct mtk_eth *eth)
{
	unsigned long t_start = jiffies;

	while (1) {
		if (!(mtk_r32(eth, MTK_PHY_IAC) & PHY_IAC_ACCESS))
			return 0;
		if (time_after(jiffies, t_start + PHY_IAC_TIMEOUT))
			break;
		cond_resched();
	}

	dev_err(eth->dev, "mdio: MDIO timeout\n");
	return -ETIMEDOUT;
}

• It yield the CPU to other tasks using cond_resched()
• It implement a timeout via time_after
• loop until condition is meet

Would that be acceptable as a "non IRQ wait path" ?

[Dolu1990]

Note, i never used cond_resched / know next to nothing about cond_resched XD

[paulusmack]

Certainly we shouldn't spin indefinitely without a timeout. Regarding the use of cond_resched(), it's probably reasonably quick in the case where it doesn't reschedule - I would expect a few microseconds on the class of CPU that we're talking about here. If it does reschedule, it could be ages (seconds) before it returns.

It seems reasonable to me to spin without calling cond_resched() for a length of time that is comparable to the context switch latency. On FPGA-based CPUs it's probably reasonable to spin without calling cond_resched for up to 100 - 200 microseconds or so.

On my 100MHz Microwatt CPU (FPGA-based) I measured the latency to take and dispatch an interrupt and then resume a task that had called wait_for_completion() to be at least 120 microseconds. So for commands that are going to take less than that amount of time it is more efficient in terms of total CPU cycles consumed to spin rather than use an interrupt.

If we know that a command is likely to take a long time (a millisecond or more) then the best thing is to use an interrupt and call wait_for_completion() without any polling. But if we think it is probably quick, or we don't really know whether it will be quick, then what I would do is poll for about 100 to 150 microseconds. If that times out, then enable the interrupt and call wait_for_completion().

By the way, I notice we are using readx_poll_timeout() rather than readx_poll_timeout_atomic(). The difference is that readx_poll_timeout() uses usleep_range() which involves setting up an hrtimer and sleeping (meaning the scheduler gets involved). The guidance in Documentation/timers/timers-howto.rst is that for sleeping for less than 10 us it is better to use udelay() rather than usleep_range() (meaning it is better to use readx_poll_timeout_atomic() rather than readx_poll_timeout()). (Note that that guidance is probably written for GHz-class CPUs, so for slow FPGA-based CPUs it is probably best to use udelay() for anything under 100 us.)

My suggestions would be: for commands that may be quick, poll for completion using readx_poll_timeout_atomic() with 1 us delay between polls and a 150 us timeout, and if the poll times out, then enable the interrupt and use wait_for_completion() to wait until it is done. (If we do that then I don't think we need cond_resched().) If we call wait_for_completion(), then when it returns we should check the completion status, like what litex_mmc_sdcard_wait_done does, but without any polling, just a single read of the command-event register.

[Dolu1990]

Sound good to me :)

[gsomlo]

On Tue, Oct 24, 2023 at 02:49:12AM -0700, Paul Mackerras wrote: My suggestions would be: for commands that may be quick, poll for completion using readx_poll_timeout_atomic() with 1 us delay between polls and a 150 us timeout, and if the poll times out, then enable the interrupt and use wait_for_completion() to wait until it is done. (If we do that then I don't think we need cond_resched().)

Do you mean we should do this for all commands and give up on the current "heuristic" that pre-determines which ones will use IRQ? Just start polling then switch to IRQ if the command takes too long, for all commands? Or should we only do this with commands not already covered by the heuristic?

If we call wait_for_completion(), then when it returns we should check the completion status, like what litex_mmc_sdcard_wait_done does, but without any polling, just a single read of the command-event register.

The `delay_before_read` argument to `readx_poll_timeout[_atomic]()` is (and should continue to be) set to false, therefore no polling/spinning occurs before the first completion status check -- that's already the current behavior in the existing driver. EDIT: I presume we'd call `readx_poll_timeout_atomic()` with `delay_before_read` set to true, if that's the *first* thing we do, so presumably after timing out, enabling IRQ, and waiting for completion, we *could* just do a read of the register rather than calling readx_poll_timeout() again -- so I guess in that context it makes sense. The only remaining question is whether to apply this treatment to all commands uniformly...