search

enjoy-digital / litesdcard

Small footprint and configurable SDCard core
Other
111 stars 35 forks source link

core/phy: Better spec compliance and interrupt support #25

Closed paulusmack closed 3 years ago

paulusmack commented 3 years ago

With this it's working well under Linux - I can do mkfs.ext4 and use it as a root file system and seems quite robust and not too slow.

paulusmack commented 3 years ago

I took out the "phy: Sample received data one clock cycle later" since it isn't right for 50MHz sysclock and 25MHz SD clock.

paulusmack commented 3 years ago

I added "phy: Adjust receive data sample point" since Gabriel Somlo reported that it worked for him (50MHz sysclock, 25MHz SD clock), and it works for me at all the SD card clock rates I tried (6.25, 12.5, 25 and 50 MHz) with 100MHz system clock.

gsomlo commented 3 years ago

tested this along with https://github.com/enjoy-digital/litex/pull/885 with linux-on-litex-rocket after removing the additional halving of the sdclock here: https://github.com/litex-hub/linux/commit/5e1d2d8782beaf1ac2d0f5b3af12d5e83679b67e

The test:

  1. loading boot.bin from the sdcard

  2. running the following test:

    mount /dev/mmcblk0p1 /mnt
cp /mnt/boot.bin /root/foo
cp /root/foo /mnt/   # this used to hang at full (25MHz sdclock speed, and worked when halving it)
umount /mnt

mount /dev/mmcblk0p1 /mnt
md5sum /mnt/*

Results:

  1. on the nexys4ddr: both tests pass with flying colors, repeatable each time a new bitstream is built (with Vivado). Clear improvement.
  2. on the trellisboard: it is possible to get "lucky" with nextpnr and obtain a bitstream that passes both tests!
    • for context: nextpnr typically reports, pessimistically, that the safe Fmax is in the 19MHz - 21MHz range (when 50MHz is requested); typically, ignoring that and running the resulting design at 50MHz works fine most of the time. It appears this series somewhat reduces the probability of "getting lucky" from "most of the time" to "some of the time".
enjoy-digital commented 3 years ago

Thanks a lot @paulusmack for your various improvements/fixes with your fresh eyes on LiteSDCard! As discussed by mail, this is really appreciated to have you help on this and all the changes you are suggesting seem to make sense. I started reviewing/merging them (one by one by applying the patches from the PR manually since I find it easier for these changes):

The busy capability has been applied in https://github.com/enjoy-digital/litesdcard/commit/d99a1c58e96dd58b433b22f08a2f895aa977dd6c but I did some changes to it to simplify it a bit in https://github.com/enjoy-digital/litesdcard/commit/755302dace708b509606cf81e6b353e4f9551718. https://github.com/enjoy-digital/litex/pull/885 has also been merged with a small change applied to reflect the LiteSDCard changes I did: https://github.com/enjoy-digital/litex/commit/2ac7e0b97825c2ecb45c9cadf7d5abb299672e81. Can you look at the changes I tell me if it makes sense for you? The idea is mostly to have the busy handling self-contained in the PHY and simplified a bit and avoid a specific register flag for the busy capability by using a specific command.

I'm now going to review/apply the other changes.

enjoy-digital commented 3 years ago
enjoy-digital commented 3 years ago

https://github.com/enjoy-digital/litesdcard/pull/25/commits/cb7b11776b23df283f8744cedb94d21937025c35 has also been integrated. I just did minor cosmetic changes on top of it with https://github.com/enjoy-digital/litesdcard/commit/c4e963c313317d1bea6926d89231b9260ae6f121.

enjoy-digital commented 3 years ago

https://github.com/enjoy-digital/litesdcard/pull/25/commits/963c8eddad35566120e58d290247dd77fde32b7a has also been integrated. I moved the logic to the PHY with https://github.com/enjoy-digital/litesdcard/commit/194a58c7fbeb5437ce54e363427ed873c1bc257a since this delay will be related to the round-trip latency (Clk output + Read input) that maybe vary in the future if we provide specific implementation for some devices. I've also been able to SDCard boot on Arty with a 100MHz sys_clk and 50MHz sd_clk with this.

@paulusmack: So all your changes are now integrated, can you have a look at the changes and tell me if it is fine for you? Could you also verify it's still working with Microwatt? (with the Linux driver that will have to be adapted due to the change I did for the busy command).

paulusmack commented 3 years ago 
* [30d3d83](https://github.com/enjoy-digital/litesdcard/commit/30d3d839d8ff27671b173893c5788c9d76349009) has been applied.

* For [c0bf3ca](https://github.com/enjoy-digital/litesdcard/commit/c0bf3caa6588d02b4d682a9f4a9f12651e00483f), I don't see `self.cmd_done` used in your code, were you using this for debug? I'm not sure we need to apply it since it's already exposed as `self.cmd_event.fields.done`.

I used it in https://github.com/paulusmack/litex/tree/cmdintr where I add a level-triggered interrupt on command completion to the EventManager that add_sdcard creates. I want a command-complete interrupt so the Linux driver doesn't have to poll during commands such as erase, which can take many milliseconds. Also, for a write, the Mem2Block DMA completion interrupt wouldn't imply that the write command was completed, just that the data had been transferred to the card, so again we could be polling for several milliseconds until the write command completed.

Your comment about using self.cmd_event.fields.done is a good point. I'll rework my cmdintr branch to use that instead.

paulusmack commented 3 years ago

@enjoy-digital: The code looks fine in your master branch and it is all working for me at 50MHz SD card clock, so I'm closing this PR. Thanks.