MMC (eMMC) support

[barthess]

Hi Giovanni
My current project need support for memory devices like KE4CN2H5C
It is just MMC in BGA package. After some digging in Chibios HAL code
and MMC documentation I have it successfully working. Is Chibios needs
MMC support in its code? I have seen some TODOs in source files.

P.S.
I performed some benchmarks.
Conditions: 48MHz, 8-bit bus, 64kiB blocks.
Results: read - 33MiB/s; write - 14MiB/s.

[Giovanni]

hi,

Are you using SDIO or MMC_SPI?

What kind of changes are required?

Giovanni

[barthess]

I am using SDIO. Driver needs:
1) slight different start procedure
2) slight different CSD slices defines
3) special procedure for acquire "extended CSD" register (for MMC v4.0 or newer)

not mandatory changes:
a) automatic detections of bus width (or let user to define it using dedicated field in config structure)
b) detection of 50MHz cards (mostly for SD)

[Giovanni]

Put the code on trunk.

Giovanni

[barthess]

Put the code on trunk.

Will do that when code finished.

I observe something strange with capacitance detection. Both CSD and EXT_CSD registers
reports capacitance exactly 2 times lower than real. Real capacitance was "measured" using
sequential reads until card returns overflow error. Calculation was performed
using JESD84-A441 standard.

Any ideas? Should it be bug in MMC?

[Giovanni]

No idea, that driver is still "strange", there are reports of random problems but I am unable to reproduce or nail them.

Giovanni

[barthess]

I am tired from all that semi proprietary crap and thousands versions and subversions of standards.
I am more and more thinking about hardcoding of bus width and max supported clock in the SDCConfig structure.

About strange things with our KE4CN2H5C. I think it is from stolen defective batch.

[barthess]

Hi there.
I commited new code. Some notes about it
1) Added new capacity calculator for MMC (it is mandatory for cards more than 2GB)
2) Added temporarily static buffer for EXT_CSD register. I think about merging it with buffer for unalignment access
3) Added functions for unpack CSD (and other registers) in human readable c-structures (enabled when CH_DBG_ENABLE_CHECKS)
4) In lld part added function to acquire registers using data bus (need for EXT_CSD for example)
5) Added detection routine for maximum supported clock (I really hate this procedure for SDC!)

Not realized yet:
Bus width detection. Currently it is just hardcodes in driver config structure. For SDC it can
be read from card registers. For MMC it need special trial and error procedure, but
SDIO cell in STM32 has hardware detector returning error code when not all data lines
put start bit on bus.

[Giovanni]

Hi,

I see some problems.

You added a lot of features directly in the SDC driver and not in the superclass mmcsd, this means that the MMC_SPI drivers does not see those.

I would put those unpack routines and structures as part of mmcsd so both drivers can use them. Do not put the unpacked structures inside the SDCDriver structure, the user can declare them if the functionality is needed, why enforce those?

For example:

void mmcsdUnpackCID(const MMCSDBlockDevice *mmcsdp, unpacked_mmc_cid_t *umcp)

and so on.

2) Added temporarily static buffer for EXT_CSD register. I think about merging it with buffer for unalignment access

Temporary stuff makes the 3.0 release not possible.

3) Added functions for unpack CSD (and other registers) in human readable c-structures (enabled when CH_DBG_ENABLE_CHECKS)

This is forbidden, the HAL in 3.0 MUST NOT have dependencies on ChibiOS/RT. It is supposed to work even with other RTOSes. Anyway see my previous comments, this should not be in the driver at all. I would leave to the user to call those functions if he needs to do debug.

Code: Select all

bool sdc_lld_read_special(SDCDriver *sdcp, uint8_t *buf, size_t bytes,
                          uint8_t cmd, uint32_t arg) {
...


Why in the low level driver? it does nothing STM32-specific I think.

Code: Select all

/**
 * @brief     Temporal storage for different purposes (extended CSD, etc.).
 */
static uint8_t scratchpad[512];


This is simply a no-go in a high level driver, the driver is supposed to support multiple instances and static buffers prevents this. It this read of the extended CSD unavoidable? I am not that particularly happy with 512 bytes wasted for that. The unaligned buffer is in the low level so it is not usable for that.
A possible solution could be to make the user pass a pointer to a buffer in the configuration structure, if passed as NULL then that specific detection case will fail. Also, we can make MMC support configurable if it is so onerous.

Giovanni

[barthess]

Thanks for code review.

EXT_CSD is mandatory for MMC more than 2GB because capacity value can not be stored in CSD.
Also it stores card clock capability. Idea about placing pointer into SDConfig looks reasonable, driver
will reject big cards if detects NULL. This memory needs only during init procedure so
it may be freed by user after sdConnect(). But better to pass pinter as additional
argument to sdConnect() function I think.

I will move all unpacking functions to superclass and introduce public API.

This is forbidden, the HAL in 3.0 MUST NOT have dependencies on ChibiOS/RT.

What about HAL_DBG_ENABLE flag in halconf?

bool sdc_lld_read_special(SDCDriver *sdcp, uint8_t *buf, size_t bytes,
uint8_t cmd, uint32_t arg) {
...
Why in the low level driver? it does nothing STM32-specific I think.

It needs because all this extended registers available only via data (not CMD) bus
and must be read in byte (not block) oriented mode. We can move some part
of this function in high level but byte read routine must be introduced in low level.