Implement CMD53 support for byte mode and block mode

[kibab]

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.

[bz]

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]

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]

• Use SDHCI_MAKE_BLKSZ() for calculating block size
• Initialize new fields of mmc_data properly
• Make block mode work

[kibab]

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.

[bz]

sys/dev/sdhci/sdhci.c

1627

if (__predict_false(sdhci_debug > 0))

[bz]

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",

[bz]

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]

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]

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?

[kibab]

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.