Page MenuHomeFreeBSD

Implement CMD53 support for byte mode and block mode
Needs RevisionPublic

Authored by kibab on Mar 21 2019, 11:19 PM.

Details

Reviewers
bz
manu
Summary

It's now possible to issue CMD53 in block mode.
Currently the chip hangs after running sdiotool twice. This happens on both AllWinner and Rpi3 and can be a chip-specific behavior.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23255
Build 22293: arc lint + arc unit

Event Timeline

kibab created this revision.Mar 21 2019, 11:19 PM
bz added a comment.EditedMar 22 2019, 8:51 PM

I haven't convinced myself yet that these two extra fields are actually needed but meanwhile this is needed (independently). I'll put it into a proper review later tonight.

    Properly zero mmc_data structures in mmc_da.c
    With upcoming MMCCAM changes extra fields will otherwise contain garbage
    and yield unexpected or unhelpful results (hanging controllers and the like).

diff --git a/sys/cam/mmc/mmc_da.c b/sys/cam/mmc/mmc_da.c
index a2dbf949b40..6ec73a46050 100644
--- a/sys/cam/mmc/mmc_da.c
+++ b/sys/cam/mmc/mmc_da.c
@@ -843,6 +843,7 @@ mmc_send_ext_csd(struct cam_periph *periph, union ccb *ccb,
        struct mmc_data d;

        KASSERT(buf_len == 512, ("Buffer for ext csd must be 512 bytes"));
+       memset(&d, 0, sizeof(d));
        d.data = rawextcsd;
        d.len = buf_len;
        d.flags = MMC_DATA_READ;
@@ -988,6 +989,7 @@ mmc_sd_switch(struct cam_periph *periph, union ccb *ccb,
        struct mmc_data mmc_d;
        uint32_t arg;

+       memset(&mmc_d, 0, sizeof(mmc_d));
        memset(res, 0, 64);
        mmc_d.len = 64;
        mmc_d.data = res;
'''
bz added a comment.Mar 22 2019, 9:03 PM

For your information; sdhci_data_irq() needs more love. Currently it runs into the CRC error case which is wrong when I am trying to blk_count = 511 (yes I am avoiding blk_count = 0 for infinite). I haven't looked into it yet, but it'll be next; here's a short debug trace:

DBG: brcmf_sdiod_ramrw:283: write 32768/600487 bytes at offset 0x00000000 in window  0x00198000
sdhci_bcm0-slot0: Got XPT_MMC_IO
sdhci_bcm0-slot0: CMD53 arg 0x9d0001ff flags 0x35 dlen 32704 dflags 0x9 blksz=64 blkcnt=511
sdhci_bcm0-slot0: SDIO Custom block params: blksz:       0x40, blk cnt:      0x1ff
sdhci_bcm0-slot0: Blk size: 0x00000040 | Blk cnt:  0x000001ff
sdhci_bcm0-slot0: Starting command opcode 0x35 flags 0x3a
sdhci_bcm0-slot0: Interrupt 0x11
sdhci_bcm0-slot0: sdhci_finish_command: called, err 0 flags 0x35
Resp: 0x1000 0000 0000 0000
sdhci_bcm0-slot0: Interrupt 0x208010
sdhci_bcm0-slot0: sdhci_data_irq:2174 set error 0x02 intmask 0x200010
                                                     ^^^^^^^^^^^^^^^^^^

sdhci_bcm0-slot0: result: 2
sdhci_bcm0-slot0: sdhci_req_done
sdhci_bcm0-slot0: sdhci_req_done: ccb 0xfffffd0000b90800 status 0x04 mmcio 0xfffffd0000b90800 error 0x02
'''
Printed from here:
if (slot->curcmd->error) {
        /* No need to continue after any error. */
slot_printf(slot, "%s:%d set error %#04x intmask %#04x\n", __func__, __LINE__, slot->curcmd->error, intmask);
        goto done;
}

'''

kibab updated this revision to Diff 55368.Mar 22 2019, 9:53 PM
  • Use SDHCI_MAKE_BLKSZ() for calculating block size
  • Initialize new fields of mmc_data properly
  • Make block mode work
kibab added a comment.Mar 22 2019, 9:54 PM
In D19674#421509, @bz wrote:

I haven't convinced myself yet that these two extra fields are actually needed but meanwhile this is needed (independently). I'll put it into a proper review later tonight.

    Properly zero mmc_data structures in mmc_da.c
    With upcoming MMCCAM changes extra fields will otherwise contain garbage
    and yield unexpected or unhelpful results (hanging controllers and the like).
diff --git a/sys/cam/mmc/mmc_da.c b/sys/cam/mmc/mmc_da.c
index a2dbf949b40..6ec73a46050 100644
--- a/sys/cam/mmc/mmc_da.c
+++ b/sys/cam/mmc/mmc_da.c
@@ -843,6 +843,7 @@ mmc_send_ext_csd(struct cam_periph *periph, union ccb *ccb,
        struct mmc_data d;
        KASSERT(buf_len == 512, ("Buffer for ext csd must be 512 bytes"));
+       memset(&d, 0, sizeof(d));
        d.data = rawextcsd;
        d.len = buf_len;
        d.flags = MMC_DATA_READ;
@@ -988,6 +989,7 @@ mmc_sd_switch(struct cam_periph *periph, union ccb *ccb,
        struct mmc_data mmc_d;
        uint32_t arg;
+       memset(&mmc_d, 0, sizeof(mmc_d));
        memset(res, 0, 64);
        mmc_d.len = 64;
        mmc_d.data = res;
'''

This is correct -- I had to do exact the same change myself.

kibab retitled this revision from Implement CMD53 support for byte-mode only, block mode is WIP. to Implement CMD53 support for byte mode and block mode.Mar 22 2019, 9:56 PM
kibab edited the summary of this revision. (Show Details)
bz added inline comments.Mar 22 2019, 11:48 PM
sys/dev/sdhci/sdhci.c
1627
if (__predict_false(sdhci_debug > 0))
bz requested changes to this revision.Mar 28 2019, 11:46 AM

I've got three proposed changes. The first two hunks remove a timeout interrupt here (unsurprisingly) by using a correct block size.

The third one simplifies the code, shortens it, and makes it more readable.

sys/dev/sdhci/sdhci.c
499
@@ -496,7 +501,13 @@ sdhci_read_block_pio(struct sdhci_slot *slot)
        buffer = slot->curcmd->data->data;
        buffer += slot->offset;
        /* Transfer one block at a time. */
-       left = min(512, slot->curcmd->data->len - slot->offset);
+#ifdef MMCCAM
+       if (slot->curcmd->data->block_count > 0)
+               left = min(slot->curcmd->data->block_size,
+                   slot->curcmd->data->len - slot->offset);
+       else
+#endif
+               left = min(512, slot->curcmd->data->len - slot->offset);
        slot->offset += left;

        /* If we are too fast, broken controllers return zeroes. */
542
@@ -539,7 +550,13 @@ sdhci_write_block_pio(struct sdhci_slot *slot)
        buffer = slot->curcmd->data->data;
        buffer += slot->offset;
        /* Transfer one block at a time. */
-       left = min(512, slot->curcmd->data->len - slot->offset);
+#ifdef MMCCAM
+       if (slot->curcmd->data->block_count > 0) {
+               left = min(slot->curcmd->data->block_size,
+                   slot->curcmd->data->len - slot->offset);
+       } else
+#endif
+               left = min(512, slot->curcmd->data->len - slot->offset);
        slot->offset += left;

        /* Handle unaligned and aligned buffer cases. */
1909

You can simplify this to:

@@ -1888,11 +1907,23 @@ sdhci_start_data(struct sdhci_slot *slot, const struct mmc_data *data)
        }
        /* Current data offset for both PIO and DMA. */
        slot->offset = 0;
-       /* Set block size and request border interrupts on the SDMA boundary. */
-       blksz = SDHCI_MAKE_BLKSZ(slot->sdma_boundary, ulmin(data->len, 512));
+
+#ifdef MMCCAM
+       if (data->block_count > 0) {
+               blksz = SDHCI_MAKE_BLKSZ(slot->sdma_boundary, data->block_size);
+               blkcnt = data->block_count;
+               if (__predict_false(sdhci_debug > 0))
+                       slot_printf(slot, "SDIO Custom block params: blksz: "
+                           "%#10x, blk cnt: %#10x\n", blksz, blkcnt);
+       } else
+#endif
+       {
+               /* Set block size and request border interrupts on the SDMA boundary. */
+               blksz = SDHCI_MAKE_BLKSZ(slot->sdma_boundary, ulmin(data->len, 512));
+               blkcnt = howmany(data->len, 512);
+       }
        WR2(slot, SDHCI_BLOCK_SIZE, blksz);
        /* Set block count. */
-       blkcnt = howmany(data->len, 512);
        WR2(slot, SDHCI_BLOCK_COUNT, blkcnt);
        if (__predict_false(sdhci_debug > 1))
                slot_printf(slot, "Blk size: 0x%08x | Blk cnt:  0x%08x\n",
This revision now requires changes to proceed.Mar 28 2019, 11:46 AM
bz added a comment.Mar 28 2019, 11:47 AM

The interrupt issues I am still seeing on CRC DAT errors seem specific to the broadcom chip on the RPi. I found other references. I have not been able to track down a solution yet but I'd suggest we ignore that for the moment.

bz added a comment.Mar 28 2019, 11:57 AM

Can we break this up into 2-3 changes please?
(1) Adding block_size and block_count to mmcreg.h and fixing the initialiers with memset or = 0; these changes are by themselves and undisputed and could get out of this.
(2) The change to the #defines in mmcreg.h can go in as well; they are not all strictly related to blk count from what I can see, so keeping them on their own would be nice.

Those two I think if cleanly extracted would get an Reviewed by: bz from me.

(3) the aw_mmc.c change need to lose the #define DEBUG line and can probably be folded together into less lines like my previous 3rd hunk suggestion for the sdhci.c. Given this is a driver improvement needed, it can then easily go in on its own.

(4) break out everything else from userspace about sdiotool to have it on its own and deal with it last. It's not connected to the build anyway in HEAD if I remember correctly so can probably just go in after (5)

(5) that should leave us with the clean sdhci.c changes for blk mode after the suggested changes are applied. I think they will be self contained at that point as well and deserve a proper commit message as well.

bz added inline comments.Mar 28 2019, 12:01 PM
sys/arm/allwinner/aw_mmc.c
62

Remove

1123

You can fold this into a smaller block as well by doing:

#ifdef MMCCAM
    if (... > 0) {
      ...
    } else
#endif
    {
      ...
    }

You probably want to have clarity over your comment before committing.

sys/dev/sdhci/sdhci.c
2790

I think this is a long line by now and might just want to be:

slot_printf(slot, "CMD%u arg %#x flags %#x dlen %u dflags %#x "
    "blksz=%zu blkcnt=%zu\n",
2791

I am not exactly sure why so many of the other lines have to change ... I assume it's whitespace cleanup which maybe should just happen on its own or maybe has happened already?

emaste added a subscriber: emaste.Mar 29 2019, 8:40 PM
kibab marked 6 inline comments as done.Apr 1 2019, 9:02 PM
kibab added inline comments.
sys/arm/allwinner/aw_mmc.c
1123

I did -- this is indeed just bytes count. Allwinner is different.

sys/dev/sdhci/sdhci.c
1627

I've actually removed this entirely.

1909

Cool, thanks :-)

2791

I'm a bit unsure on how style(9) would require to format these. Given that now I use an Emacs config that seems to work well for FreeBBSD code and it formats this printf as displayed, I'd rather correct style here.