Sdio driver feature by mathias-arm · Pull Request #4 · ARMmbed/sdio-driver

mathias-arm · 2019-08-09T07:35:54Z

Ignore #3. I replayed ARMmbed/mbed-os#10235 (without the mbed-os part that I put here).

With this PR, you can add a sdio-driver.lib in your project (+ a few tweaks to mbed_app.json) and use other for storage_filesystem.blockdevice:

#include "SDIOBlockDevice.h"
static SDIOBlockDevice bd;

BlockDevice *get_other_blockdevice()
{
    return &bd;
}

BlockDevice *BlockDevice::get_default_instance()
{
    return &bd;
}

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

Tested this with the block device test inside features-storage-tests-blockdevice-general_block_device

mathias-arm · 2019-08-12T21:05:40Z

I have created also another repository, to semi-automatically integrate the sdio-driver into MbedOS. It also documents how I have used it to test mbed-bootloader and mbed-cloud-client-example / pelion-example-common with DISCO_F746NG and LPC55S69_NS.

0xc0170 · 2019-08-23T14:16:20Z

Sorry for the delay, I should have a look the upcoming week at this one.

0xc0170

What benefits are there for introducing async? Does it need to be separated (another DEVICE_) ? We had previously async separated due to introducing it to already supported drivers, this could come with it as default.

0xc0170 · 2019-09-02T09:07:18Z

components/storage/blockdevice/COMPONENT_SDIO/SDIOBlockDevice.h

+     *
+     *  @note check physical present switch. Maybe not support by hardware, then function will always return true.
+     */
+    virtual bool isPresent(void);


should be is_present()

0xc0170 · 2019-09-02T09:07:34Z

components/storage/blockdevice/COMPONENT_SDIO/SDIOBlockDevice.h

+    virtual const char *get_type() const;
+
+private:
+    mbed::DigitalIn _cardDetect;


_card_detect and below also for card info member

I've noticed others as well , like SDIO_Cardinfo_t members. We don't use camel case in most cases (only C++ class names)

0xc0170 · 2019-09-02T09:07:55Z

components/storage/blockdevice/COMPONENT_SDIO/mbed_lib.json

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


fix alignment

0xc0170 · 2019-09-02T09:12:01Z

components/storage/blockdevice/COMPONENT_SDIO/SDIOBlockDevice.cpp

+        return SD_BLOCK_DEVICE_ERROR_ERASEBLOCKS;
+#if DEVICE_SDIO_ASYNC
+    } else {
+        uint32_t tickstart = us_ticker_read();


why is this using _us_ticker - not wait_us() rather or Ticker object? Shouldn't it sleep_for rather?

mathias-arm · 2019-09-04T15:58:55Z

@0xc0170: I think the idea with having DEVICE_SDIO_ASYNC is that the async drivers would not have to be duplicating some of the logic and putting the wait logic in control of SDIOBlockDevice. I think @orenc17 did that part. In my #3 version, I did add the timeout value to the HAL call and left the wait in the low-level part. Your call.

I will look into the other changes.

0xc0170 · 2019-09-17T10:46:03Z

@ARMmbed/mbed-os-storage Could you please review?

VeijoPesonen · 2019-09-18T06:46:42Z

hal/sdio_api.h

+ *            @arg  SD_TRANSFER_OK: No data transfer is acting
+ *            @arg  SD_TRANSFER_BUSY: Data transfer is acting
+ */
+int sdio_get_card_state(void);


In my opinion the documentation tells that this would be the same as checking sdio_read_pending and sdio_write_pending in a row. Implementation seems to be something different. Would you please update this comment section.

There is a little bit of confusion because the sdio_read_pending() and sdio_write_pending() are only defined if SDIO_ASYNC is defined.

VeijoPesonen · 2019-09-18T06:50:14Z

targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_LPC55S69/TARGET_M33_NS/sdio_api.c

+{
+    int sd_state = MSD_OK;
+
+    if (!SD_CheckReadOnly(&g_sd)) {


This functionality is something different I was expecting based on the comment section.

We don't do camelCase, I suppose.

The NXP driver does not have SDIO_ASYNC defined so it does not seem to have a notion of the device being "busy". Actually it should probably return SD_TRANSFER_OK and a SD_TRANFER_ERROR should probably be added.

Regarding camelCase, my goal was to keep my changes to a minimum on the target-specific code. The symbols beyond the HAL seem to me much less important.

We should not touch 3rd party drivers so if this CheckReadOnly comes from there, all good !

VeijoPesonen · 2019-09-18T07:05:26Z

hal/sdio_api.h

@@ -0,0 +1,163 @@
+/** \addtogroup hal */


What is the reason for having a new API instead of providing an implementation for the already existing BlockDevice interface and extend it when necessary? Maybe do it in a similar manner as SDBlockDevice.(My bad, this is HAL API, not Mbed OS API...)

Or are there any plans to get this driver to Mbed OS one day?

One day there will be plans to add this to MbedOS. The original code comes from 2 different implementations. @orenc17 did some work to align them. I understand the reason of the existence of this repository as a place to work out the APIs and the generic part. Having the different implementations in the same tree will allow easier changes and testing.

0xc0170

Just some styling remarks, otherwise looks fine to me to land and continue progressing this

0xc0170 · 2019-09-20T08:34:37Z

components/storage/blockdevice/COMPONENT_SDIO/SDIOBlockDevice.cpp

+#define SDIO_DBG 0       /*!< 1 - Enable debugging */
+#define SDIO_CMD_TRACE 0 /*!< 1 - Enable SD command tracing */
+
+#define SDIO_BLOCK_DEVICE_ERROR_WOULD_BLOCK -5001           /*!< operation would block */


I would make this enum rather than macros. SImilar to types like qspif_bd_error (an example how error code look like).

0xc0170 · 2019-09-20T08:35:38Z

components/storage/blockdevice/COMPONENT_SDIO/SDIOBlockDevice.cpp

+#define BLOCK_SIZE_HC 512 /*!< Block size supported for SD card is 512 bytes  */
+
+// Types
+#define SDCARD_NONE 0  /**< No card is present */


same here as above, lets make them more debug friendly. I would even do enum class as we are now C++1x

0xc0170 · 2019-09-20T08:38:29Z

hal/sdio_api.h

+ * @brief  DeInitializes the SD card device.
+ * @retval SD status
+ */
+int sdio_deinit(void);


hal usually names this _free

0xc0170 · 2019-09-20T08:40:00Z

hal/sdio_api.h

+ * @param  block_count: Number of SD blocks to write
+ * @retval SD status
+ */
+int sdio_write_blocks(uint8_t *data, uint32_t address, uint32_t block_count);


return types for HAL: wouldn't it be better to have a status return type, see qspi HAL for instance. qspi_status_t to describe all return types

AGlass0fMilk · 2020-04-27T02:51:11Z

What's the status of the sdio blockdevice feature? I have a need for one of my custom boards. I'd be interested in helping integrate this feature to mbed-os master!

@maclobdell

maclobdell · 2020-04-28T12:41:13Z

@AGlass0fMilk @mathias-arm as part of the silicon partner governance, a design document was proposed for official SDIO ARMmbed/mbed-os#11847. There was some work initially but it seems to have stalled a bit. It would be great if you can review that document and provide your feedback and we can push it a bit closer to completion.

JojoS62 and others added 20 commits August 9, 2019 00:39

initial commit

33c0600

add SDIOBlockdevice implementation for STM32F4

add implementation for STM32F7

4773505

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

8d1f833

changed copyright to mBed standard

088679c

code cleanup

124d0bd

- removed comments - renamed short variable names

fiixed timeout checking

26a4b7c

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

Restructure across the tree

c0bd5f9

Rename sdio_device to sdio_api

eb4cb06

Remove tests

bb0d00a

API renaming

62eb597

Astyle fixes

5d1d09b

Change error type to int and use us_ticker_read

e742718

Remove timeout parameter

4295558

Move documentation to sdio_api.h

d5b6a08

Compilation fix

7ae99b1

Minor fixes

b840f32

Check if initialized before calling is_valid

11daa9f

Add SDIO support for NXP LPC55S69

f27d137

Tested this with the block device test inside features-storage-tests-blockdevice-general_block_device

Restructure NXP implementation

b892505

Update to "new" API

744451e

mathias-arm mentioned this pull request Aug 9, 2019

Unified implementation for TARGET_STM and TARGET_NXP #3

Closed

mathias-arm added 2 commits August 9, 2019 02:44

Fix some warnings (tested on GCC 8)

a068132

Only wait (and require USTICKER) when DEVICE_SDIO_ASYNC

c18f614

mathias-arm added a commit to mathias-arm/mbed-os that referenced this pull request Aug 12, 2019

Add in-tree support for out-of-tree SDIO driver (ARMmbed/sdio-driver#4)

8f1f102

0xc0170 reviewed Sep 2, 2019

View reviewed changes

Remove CamelCase symbols

19bca26

mathias-arm mentioned this pull request Sep 12, 2019

Remove some warnings nxp-archive/nxpmicro_sdio-driver#1

Merged

VeijoPesonen reviewed Sep 18, 2019

View reviewed changes

0xc0170 requested changes Sep 20, 2019

View reviewed changes

Conversation

mathias-arm commented Aug 9, 2019

Uh oh!

mathias-arm commented Aug 12, 2019

Uh oh!

0xc0170 commented Aug 23, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathias-arm commented Sep 4, 2019

Uh oh!

0xc0170 commented Sep 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AGlass0fMilk commented Apr 27, 2020

Uh oh!

maclobdell commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

VeijoPesonen Sep 18, 2019 •

edited

Loading