Initial revision of the SDIO Design document

[mmahadevan108]

Description (required)

Initial revision of the SDIO design document

Pull request type (required)

[X Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results (required)

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

[mmahadevan108]

cc @bulislaw @maclobdell

[ciarmcom]

@mmahadevan108, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

[pea-pod]

I lightly compared this design document to the QSPI one in the main branch. I figured this would be comparable as the use cases and hardware features would be similar.

The first issue I noticed is that there is no provision for a implementation defined SDIO structure that would hold data like hardware instance base address. Some boards have multiple SD controllers. If a structure is not used, you would be only able to use one of them.

The second issue I noticed in the design document is that unlike the QSPI init function (or most of the HAL functions for the peripherals) there is no possibility of specifying the pins. Again, I’m ignorant here, but it seems like it is possible that some vendors might mux these pins out to potentially multiple places. I have found at least one board which has this capability. Alternatively, you could define a structure for the pins and other features that are important instead of calling several different functions and pass that to the init function.

This may be for a future commit, but it seems like there also should be a few more public functions, such as a frequency selection, a mode selection (1-bit/4-bit SD or MMC, 8-bit MMC), and possibly PinMap functions.

Finally, there are a few manufacturers that make SDIO based WiFi modules (such as Murata and Laird). Would this api be usable for both storage and these modules using SDIO?

[LMESTM]

above comments from @pea-pod make sense. In case we only target SDcard and not SDIO connected WIFI modules, then we should make it clear or maybe name the driver accordingly (SDcard rater than SDIO).

[adbridge]

@mmahadevan108 it looks like you have removed sections from the template header or are using an out of date one. Could you please update to the latest one going forward and fill in all relevant sections. This really helps us to better understand the changes and their impact.

[adbridge]

@bulislaw could you review please.

[bulislaw]

@LMESTM @jeromecoutant @ARMmbed/team-cypress do you guys have any comments? Or are you ok with this first draft being merged to the feature branch?

[morser499]

I have similar concerns as expressed above. Additionally, there should be a sdio_free() function added to de-initialize the driver.

[0xc0170]

Thanks @pea-pod for the review.

@mmahadevan108 Can you review? As this is for the future branch, there can be made subsequent updatess to address the reviews (if not simple to do right away here) ?

[0xc0170]

How can we progress here? I'll talk to @maclobdell

[pea-pod]

This may not be what you are specifically asking for, but in any case, here are my two cents on the matter.

If I had my druthers, I would prefer something similar to this:

hal/sdmmc/
    sdmmc_hal.h
    sdmmc_types.h
    sdmmc_card_types.h
    sdio_types.h
    sdmmc_card.h
    sdmmc_card.c
features/storage
    SDMMCBlockDevice.h
    SDMMCBlockDevice.cpp
drivers/
    SDIO.h
targets/TARGET_i_CAN_HAZ_SILLYCON?
    sdmmc_hal.c

• The sdmmc_hal.h would handle the setup of the hardware peripheral.
• The sdmmc_types.h would contain all values common to the sdmmc libraries, including return values, cmds that are in common, etc.
• The sdmmc_card_types.h would contain the typedefs, values, etc. specific to the card itself.
• The sdio_types.h would contain any of the values specific to the IO side (i.e. various WiFi or WiFi/BT devices).
• The sdmmc_card.h/c would contain a C implementation of all of the states necessary to use a memory card over this bus. This is an optional location to implement this. It could be done completely in the SDMMCBlockDevice.h device. Alternately, it could handle the intermediate state machine, and leave the block handling up to the
• The features/storage folder would contain everything necessary to implement a memory card interface over the SDMMC peripheral in the SDMMCBlockDevice.h.
• The SDIO.h would implement the SDIO interface, which seems to seems to have a fairly well defined interface (as far as I can tell), even if the commands are specific to devices.
• The sdmmc_hal.c would be implemented by the appropriate vendors.

Since whomever consumes this (i.e. not the programmer, but the actual purchaser) would not really care if an SD or MMC device was installed, I think implementing the One Ring to Rule Them All Ash nazg durbatulûk one interface to handle all SD/MMC cards (limited to the devices the interface is capable of implementing) would be ideal.

Furthermore, if the initial requirement for the SDMMC interface is memory cards (which I do believe is reasonable), I would prefer that it be designed and specified in such a way as to do as much to implement the SDIO specific functionality, or at least not preclude it.

The hal itself would contain the pin locations etc., handle bring up and teardown, etc.

Finally, whatever does get implemented should have the DMA capabilities implemented out of the box (if there is any other way, for that matter). There's barely a benefit of speed going from SPI to SDMMC without including the DMA functionality.

[0xc0170]

@maclobdell What shall we do with this one ? shall we park it (close it) and get back to it with an update?

[ciarmcom]

This pull request has automatically been marked as stale because it has had no recent activity. @mmahadevan108, please carry out any necessary work to get the changes merged. Thank you for your contributions.

[0xc0170]

I'll close this PR. It is marked with feature: hal to track this PR separately.