SDIO support - WIP #10235

orenc17 · 2019-03-26T18:12:37Z

Description

Importing most of the change from initial PR for SDIOBlockdevice implementation sdio-driver#1
Renamed most of the original API's to something more similar to SPI API.

@JojoS62 thanks for the hard work
@ARMmbed/mbed-os-maintainers please set this to a new feature branch

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-storage @JojoS62 @ARMmbed/team-st-mcd

Release Notes

orenc17 · 2019-03-26T18:13:24Z

@mmahadevan108 FYI

cmonr · 2019-03-26T18:35:38Z

@orenc17 Please take a look at the astyle issues (yesterday's issues have been fixed and merged. Results in job appear to be correctly failing).

jeromecoutant · 2019-03-27T07:54:17Z

Hi
Are there any tests ?

orenc17 · 2019-03-27T08:57:50Z

Hi
Are there any tests ?

@jeromecoutant i'm working on integrating the new Blockdevice to the current Blockdevice tests

add SDIOBlockdevice implementation for STM32F4

almost the same as for STM32F4, but in F7 SDIO device is renamed to SDMMC SDMMC1 is used for DISCO_F746NG updated readme.md for new target # Conflicts: # README.md

- removed comments - renamed short variable names

tickstart was inside checking loop removed wait_ms() calls added timeout checks for all while loops

offirko · 2019-03-27T10:12:30Z

components/storage/blockdevice/COMPONENT_SDIO/SDIOBlockDevice.cpp

+        return SD_BLOCK_DEVICE_ERROR_NO_DEVICE;
+    }
+
+    if (!is_valid_read(addr, size)) {


when not initialized 'this->size()" is 0 and is_valid_read/program/trim will fail..
You will receive SD_BLOCK_DEVICE_ERROR_PARAMETER instead of SD_BLOCK_DEVICE_ERROR_NO_INIT, which is more appropriate.

orenc17 · 2019-03-27T13:26:55Z

@ARMmbed/mbed-os-hal can you please review

Tested this with the block device test inside features-storage-tests-blockdevice-general_block_device Below was the change made to add SDIO test --- a/features/storage/TESTS/blockdevice/general_block_device/main.cpp +++ b/features/storage/TESTS/blockdevice/general_block_device/main.cpp @@ -47,6 +47,8 @@ #include "FlashIAPBlockDevice.h" #endif +#include "SDIOBlockDevice.h" + using namespace utest::v1; #define TEST_BLOCK_COUNT 10 @@ -69,6 +71,7 @@ enum bd_type { dataflash, sd, flashiap, + sdio, default_bd }; @@ -90,6 +93,11 @@ static inline uint32_t align_up(uint32_t val, uint32_t size) static BlockDevice *get_bd_instance(uint8_t bd_type) { switch (bd_arr[bd_type]) { + case sdio: { + static SDIOBlockDevice default_bd(P0_17); + return &default_bd; + break; + } case spif: { #if COMPONENT_SPIF static SPIFBlockDevice default_bd( @@ -632,6 +640,8 @@ void test_get_type_functionality() const char *bd_type = block_device->get_type(); TEST_ASSERT_NOT_EQUAL(0, bd_type); + TEST_ASSERT_EQUAL(0, strcmp(bd_type, "SDIO")); + #if COMPONENT_QSPIF TEST_ASSERT_EQUAL(0, strcmp(bd_type, "QSPIF")); #elif COMPONENT_SPIF @@ -708,10 +718,12 @@ int get_bd_count() bd_arr[count++] = flashiap; //4 #endif + bd_arr[count++] = sdio; //5 + return count; } -static const char *prefix[] = {"SPIF ", "QSPIF ", "DATAFLASH ", "SD ", "FLASHIAP ", "DEFAULT "}; +static const char *prefix[] = {"SPIF ", "QSPIF ", "DATAFLASH ", "SD ", "FLASHIAP ", "SDIO ", "DEFAULT "}; int main() { Signed-off-by: Mahesh Mahadevan <[email protected]>

orenc17 · 2019-03-27T16:21:27Z

@mmahadevan108
I've added your implementation from ARMmbed/sdio-driver#2
modified a bit, could you please review?

it seems to pass the general-block-device test

mmahadevan108 · 2019-03-27T18:32:32Z

Should we pass in PinNames to sdio_init() for DAT0-3, CLK, CMD?

orenc17 · 2019-03-27T18:37:39Z

Should we pass in PinNames to sdio_init() for DAT0-3, CLK, CMD?

@mmahadevan108 i think so, this is an initial step, it will evolve in time
that is also why this PR is targeted to a feature branch

dannybenor · 2019-03-28T10:07:23Z

hal/sdio_api.h

+ * @param  ReadAddr: Address from where data is to be read
+ * @param  NumOfBlocks: Number of SD blocks to read
+ * @retval SD status
+ */


Please specify the ReadAddress units: Bytes or blocks?

dannybenor · 2019-03-28T10:14:52Z

hal/sdio_api.h

+    uint32_t RelCardAdd;   /* Specifies the Relative Card Address             */
+    uint32_t BlockNbr;     /* Specifies the Card Capacity in blocks           */
+    uint32_t BlockSize;    /* Specifies one block size in bytes               */
+    uint32_t LogBlockNbr;  /* Specifies the Card logical Capacity in blocks   */


Will be good to add some comments about the difference between BlockNbr and LogBlockNbr

this part is a direct copy from sdio-driver.
@JojoS62 is there a difference?

screamerbg · 2019-03-28T10:46:06Z

@orenc17 @0xc0170 @bulislaw Is this a feature or a new component? I'm confused as this looks more like a new component implementation, rather than fundametal Mbed OS feature. I.e. it's an addition to the storage feature.

orenc17 · 2019-03-28T11:53:29Z

@screamerbg this is a new HAL layer for mbed-os plus a new BlockDevice using that hal
Blockdevices in mbed-os are components

you wont find any FEATURE_ folders in this PR

jeromecoutant · 2019-04-12T07:18:01Z

features/storage/TESTS/blockdevice/general_block_device/main.cpp

@@ -48,6 +48,10 @@
 #include "FlashIAPBlockDevice.h"
 #endif

+#if COMPONENT_SDIO
+#include "SDIOBlockDevice.h"


What happened if a targets adds component SDIO and SD and QSPIF ?

All three blockdevices will be tested

jeromecoutant · 2019-04-12T07:18:45Z

i'm working on integrating the new Blockdevice to the current Blockdevice tests

Could you attach your test results ?

Thx

jeromecoutant · 2019-04-12T07:20:12Z

components/storage/blockdevice/COMPONENT_SDIO/mbed_lib.json

+    },
+    "target_overrides": {
+        "DISCO_F469NI": {
+			"CMD_TIMEOUT": 30000


Why same value is overridden ?

jeromecoutant · 2019-04-12T07:29:23Z

targets/TARGET_STM/TARGET_STM32F4/sdio_api.c

+         PC9     ------> SDIO_D1
+         PC8     ------> SDIO_D0
+         */
+        GPIO_InitStruct.Pin = GPIO_PIN_12 | GPIO_PIN_11 | GPIO_PIN_10 | GPIO_PIN_9 | GPIO_PIN_8;


This pin configuration can't be defined for all STM32 like this.
We should use some MACRO which can be define at each MCU level?
Or we should create a new structure in PeripheralPins.c ?

But OK to keep it like this in a first step, it can be optimized later while adding more SDIO targets.

jeromecoutant · 2019-04-12T07:32:40Z

targets/targets.json

-            "USBDEVICE"
+            "USBDEVICE",
+            "SDIO",
+            "SDIO_ASYNC"


We need to add COMPONENT_SDIO AND DEVICE_SDIO AND DEVICE_SDIO_ASYNC ?

jeromecoutant · 2019-04-30T16:01:36Z

Hi
Any review update and test result ?

adbridge · 2019-05-01T11:19:19Z

@orenc17 Could you address the review comments please ?

0xc0170 · 2019-05-03T12:34:38Z

@orenc17 What is missing here to land this on the feature branch? Besides tests.

jeromecoutant · 2019-05-28T14:58:13Z

Hi
Initial PR from @JojoS62 was in November 2018...
Can you assign someone to review, update and merge this one ?
Thx

@MarceloSalazar

0xc0170 · 2019-06-04T14:01:33Z

@ARMmbed/mbed-os-storage ^^

0Grit · 2019-06-04T14:04:54Z

@davisga @trowbridgec @farrenv @AGlass0fMilk any feedback?
I know next to nothing about SDIO.

cmonr changed the base branch from master to feature-sdio March 26, 2019 18:34

cmonr added needs: review release-version: feature branch labels Mar 26, 2019

cmonr requested review from a team March 26, 2019 18:39

JojoS62 and others added 12 commits March 27, 2019 11:02

initial commit

6af45dd

add SDIOBlockdevice implementation for STM32F4

add implementation for STM32F7

114aa12

almost the same as for STM32F4, but in F7 SDIO device is renamed to SDMMC SDMMC1 is used for DISCO_F746NG updated readme.md for new target # Conflicts: # README.md

changed copyright notice to standard mBed

3688197

changed copyright to mBed standard

f4fee5b

code cleanup

2245fed

- removed comments - renamed short variable names

fiixed timeout checking

ba6ba78

tickstart was inside checking loop removed wait_ms() calls added timeout checks for all while loops

Restructure across the tree

917a780

Rename sdio_device to sdio_api

d72c324

Remove tests

2a7f9e9

API renaming

995856a

Astyle fixes

3d2cb80

Integrate SDIOBlockDevice into kvstore and system storage

88e446c

offirko reviewed Mar 27, 2019

View reviewed changes

Oren Cohen added 3 commits March 27, 2019 14:46

Change error type to int and use us_ticker_read

8121862

Remove timeout parameter

b0c5746

Move documentation to sdio_api.h

67eba9b

orenc17 changed the base branch from feature-sdio to master March 27, 2019 13:18

orenc17 changed the base branch from master to feature-sdio March 27, 2019 13:18

Enable SDIO Blockdevice on ST boards

9d79e7c

mmahadevan108 and others added 3 commits March 27, 2019 17:55

Restructure NXP implementation

506626b

Update to "new" API

e121185

orenc17 closed this Mar 27, 2019

orenc17 reopened this Mar 27, 2019

adbridge removed the needs: review label Mar 27, 2019

mmahadevan108 approved these changes Mar 27, 2019

View reviewed changes

cmonr added the needs: review label Mar 27, 2019

dannybenor reviewed Mar 28, 2019

View reviewed changes

orenc17 changed the title ~~New feature - SDIO~~ SDIO support - WIP Mar 28, 2019

jeromecoutant reviewed Apr 12, 2019

View reviewed changes

adbridge added needs: work and removed needs: review labels May 1, 2019

orenc17 closed this Jun 4, 2019

This was referenced Aug 9, 2019

Unified implementation for TARGET_STM and TARGET_NXP ARMmbed/sdio-driver#3

Closed

Sdio driver feature ARMmbed/sdio-driver#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDIO support - WIP #10235

SDIO support - WIP #10235

orenc17 commented Mar 26, 2019

orenc17 commented Mar 26, 2019

cmonr commented Mar 26, 2019

jeromecoutant commented Mar 27, 2019 •

edited

Loading

orenc17 commented Mar 27, 2019

offirko Mar 27, 2019

orenc17 commented Mar 27, 2019

orenc17 commented Mar 27, 2019 •

edited

Loading

mmahadevan108 commented Mar 27, 2019 •

edited

Loading

orenc17 commented Mar 27, 2019

dannybenor Mar 28, 2019

dannybenor Mar 28, 2019

orenc17 Mar 28, 2019

screamerbg commented Mar 28, 2019 •

edited

Loading

orenc17 commented Mar 28, 2019

jeromecoutant Apr 12, 2019

orenc17 Apr 12, 2019

jeromecoutant commented Apr 12, 2019

jeromecoutant Apr 12, 2019

jeromecoutant Apr 12, 2019

jeromecoutant Apr 12, 2019

orenc17 Apr 12, 2019

jeromecoutant commented Apr 30, 2019

adbridge commented May 1, 2019

0xc0170 commented May 3, 2019

jeromecoutant commented May 28, 2019

0xc0170 commented Jun 4, 2019

0Grit commented Jun 4, 2019

SDIO support - WIP #10235

SDIO support - WIP #10235

Conversation

orenc17 commented Mar 26, 2019

Description

Pull request type

Reviewers

Release Notes

orenc17 commented Mar 26, 2019

cmonr commented Mar 26, 2019

jeromecoutant commented Mar 27, 2019 • edited Loading

orenc17 commented Mar 27, 2019

Choose a reason for hiding this comment

orenc17 commented Mar 27, 2019

orenc17 commented Mar 27, 2019 • edited Loading

mmahadevan108 commented Mar 27, 2019 • edited Loading

orenc17 commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

screamerbg commented Mar 28, 2019 • edited Loading

orenc17 commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromecoutant commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromecoutant commented Apr 30, 2019

adbridge commented May 1, 2019

0xc0170 commented May 3, 2019

jeromecoutant commented May 28, 2019

0xc0170 commented Jun 4, 2019

0Grit commented Jun 4, 2019

jeromecoutant commented Mar 27, 2019 •

edited

Loading

orenc17 commented Mar 27, 2019 •

edited

Loading

mmahadevan108 commented Mar 27, 2019 •

edited

Loading

screamerbg commented Mar 28, 2019 •

edited

Loading