New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add driver for sd-cards (using spi) #6031
add driver for sd-cards (using spi) #6031
Conversation
I guess you tested with a Samr21-xpro using the I/O Xplained extension ? |
Actually not, never heard of that before. I used a thing you could call "DIY micro-sd to breadboard adapter" which is basically one of those micro-sd to sd adapters with some pins soldered onto it :P |
nice to see this new driver. I look forward to testing this soonish (: @MichelRottleuthner did you try to use the remote with an onboard sd slot? |
@cgundogan no - but i will try that tomorrow if you hand me one of the remote boards ;) Apart from that, do we have one of those extension boards mentioned by @aabadie lying around? |
I have some in my office ;) Otherwise you can access those extension on the IoT-LAB Saclay site on nodes called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a full review, just some stuff I picked up while looking at the code.
|
||
/* see sd spec. 5.3.2 CSD Register (CSD Version 1.0) */ | ||
struct { | ||
uint8_t CSD_STRUCTURE : 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we just discuss on the mailing list that bitfields are bad ROM-wise? Also: the distribution of the bits makes no sense (2 byte of uint8_t
, then directly after 8 byte of uint8_t
=> 6 bytes wasted) so this will most likely waste a lot of RAM, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(okay sorry, the spec says the 6 bytes are reserved)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely agree on the point that the ordering doesn't make sence. This just represents the ordering of the values in the sd-spec and comes from monkey-like typing them to the header file ;) I'm not quite sure about the general desicion whether the use of bitfields is bad or good in this case. I have read the discussion (thanks for the hint) and will look at the register stuff again with this in mind. Nonetheless imho trading a few bytes of ROM for better usability may be worth it, especially if the effect causes both more, or less usage of ROM depending on the platform.
struct { | ||
uint8_t MID; /* Manufacturer ID */ | ||
char OID[2]; /* OEM/Application ID*/ | ||
char PNM[5]; /* Product name */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adhere to coding conventions
@@ -0,0 +1,359 @@ | |||
/* | |||
* Copyright (C) 2016 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyrighted by whom?
#define R1_HAS_CMD_CRC_ERR(X) ((((X) &SD_R1_RESPONSE_CMD_CRC_ERROR) != 0)) | ||
#define R1_HAS_ILL_CMD_ERR(X) ((((X) &SD_R1_RESPONSE_ILLEGAL_CMD_ERROR) != 0)) | ||
#define IS_R1_IDLE_BIT_SET(X) (((X) &SD_R1_RESPONSE_IN_IDLE_STATE) != 0) | ||
#define R1_HAS_ERROR(X) (R1_HAS_PARAM_ERR(X) || R1_HAS_ADDR_ERR(X) || R1_HAS_ERASE_ERR(X) || R1_HAS_CMD_CRC_ERR(X) || R1_HAS_ILL_CMD_ERR(X)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the line length to max 80 characters (100 if it absolutely doesn't go any other way)
@MichelRottleuthner: I'm volunteer for helping you getting this work on a |
@smlng I will update the RE-Mote wiki shortly with instruction on how to enable the Micro-SD for the current revision B batch, as resistors enabling the microSD holder were not mounted at the production line (fiex for the next batch of RE-Motes) |
Instruction on enabling the Micro-SD on RE-Motes revision B Here (for the ones that left without the resistors that should been mounted as default) |
Can confirm this driver as working on the RE-Mote revB.
|
Nice! I will test ASAP. To avoid the 1V you need to configure the SPI lines as output/low, we will test and report if this works as expected as well. Thanks! |
What compiler version or toolchain are you using? I had to change the |
It is working on the RE-Mote revision B:
|
I'm using 4.9.2 but the missing bool is because I forgot to include stdbool.h. I didn't see that before because compiler didn't complain on samr21x-pro. Will provide a fix for that! thanks for testing!
That's what I wanted to explain by the edit above, driving it low works indeed ;) |
d8bec06
to
7df9f42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. However some coding style and code improvements possible 😄
|
||
for (int i = 0; i < chunk_blocks * SD_HC_BLOCK_SIZE; i++) { | ||
|
||
if (((i % SD_HC_BLOCK_SIZE) == 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove outer pair of parentheses, one should do
|
||
puts("insert SD-card and use 'init' command to set card to spi mode"); | ||
puts("WARNING: using 'write' or 'copy' commands WILL overwrite data on your sd-card and\n\ | ||
almost for sure corrupt existing filesystems, partitions and contained data!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add separate puts
and indent
printf("read error %d (block %d)\n", rd_state, src_block); | ||
return -1; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else
not needed, because return -1
above
printf("write error %d (block %d)\n", wr_state, dst_block); | ||
return -1; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again else
not needed here
int src_block; | ||
int dst_block; | ||
|
||
if (argc == 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert statement into:
if (argc != 3) {
printf("usage: %s src_block dst_block\n", argv[0]);
return -1;
}
src_block = (uint32_t)atoi(argv[1]);
dst_block = (uint32_t)atoi(argv[2]);
xtimer_init(); | ||
return SD_INIT_SPI_POWER_SEQ; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else not needed due to return above
gpio_set(card->cs_pin); /* unselect sdcard for power up sequence */ | ||
|
||
/* powersequence: perform at least 74 clockcycles with mosi_pin being high | ||
(same as sending dummy bytes with 0xFF) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
second line of comment should start with aligned *
, too
_dyn_spi_transfer_byte = &_hw_spi_transfer_byte; | ||
return SD_INIT_ENABLE_CRC; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else not needed, again
|
||
char sdcard_spi_send_cmd(sd_card_t *card, char sd_cmd_idx, uint32_t argument, int max_retry) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
|
||
} | ||
else { | ||
DEBUG("_read_data_packet: _transfer_bytes [RX_TX_ERROR] (while transmitting crc)\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same error message as below, remove here completely and remove else
below, so it will hit the message in both cases?
@MichelRottleuthner nice work! IMHO for the clarity of the interface, I think it would make much sense to split the header file, one (i.e. |
Just tried to run this using the
before getting back to the @MichelRottleuthner: Have you any hint, what this could be about? |
Connected the same shield to an
|
@haukepetersen The second log looks suspicious to me. The lower 12 Bits of R7 response should match the argument that is sent with CMD8, which clearly isn't the case here ( Can you give me more details about the card used? |
@haukepetersen I just tested init on pba-d-01-kw2x with a few cards I have lying around here (one of them is a 32GB microSDHC Samsung EVO MB-MP32D) but I could not reproduce this behaviour. Look at the newly added SDCARD_SPI_CONFIGS in the tests Makefile to see which pins I used. |
Thanks for the replies. My card here is a |
@haukepetersen I finally got my hands on one of the pba-d-01-kw2x shields with msd slot and can confirm that it doesn't work. It looks like the MISO and MOSI pins are swapped (compared to the boards periph_conf.h). I double checked it and the shield works fine when connected with jumper wires that swap both pins. |
now finally: using an
|
now I don't know what the problems with my other setup's were, I would say let's get this PR in a mergable condition and get it into master. So my main issue is still the separation of user interface and internal functionality: Could you give it a try to improve this?! I will give some suggestions as comments in the code below. |
|
||
#ifndef NUM_OF_SD_CARDS | ||
#define NUM_OF_SD_CARDS 1 | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have this driver following the same paradigm as our other device drivers for defining parameters and deducting the the number of defined devices:
- have a default parameter configuration in
drivers/sdcard_spi/include/sdcard_spi_params.h
- use auto_init (as e.g.
sys/auto_init/netif/auto_init_w5100.c
, of course ommiting the gnrc specific parts) - define the device descriptor
sdcard_spi_t
and a params structsdcard_spi_params_t
in this header file - have an init function, that takes exactly those two prameters:
int sdcard_init(sdcard_spi_t *dev, sdcard_spi_params_t *params);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also see https://github.com/RIOT-OS/RIOT/wiki/Guide:-Writing-a-device-driver-in-RIOT for further reference
Oh, and forget about the comment above about the interface saparation -> didn't see that you already addressed that... sorry. |
#define ASCII_UNPRINTABLE_REPLACEMENT "." | ||
|
||
/* this is provided by the sdcard_spi driver | ||
* see /home/michel/Projekte/fatfs/RIOT/sys/auto_init/storage/auto_init_sdcard_spi.c */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this path might be hard to follow for people that don't have access to your computer :-)
int csd_structure; | ||
cid_t cid; | ||
csd_t csd; | ||
} typedef sdcard_spi_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and above: you need to comment every struct member, otherwise Murdock will complain...
* @name Onboard micro-sd slot pin definitions | ||
* @{ | ||
*/ | ||
#define SDCARD_SPI_PARAM_SPI SPI_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be SPI_DEV(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ups, never mind, SPI_1
is just fine here, the SPI_DEV() define is not yet merged...
* @{ | ||
*/ | ||
#define SDCARD_SPI_PARAM_SPI SPI_1 | ||
#define SDCARD_SPI_PARAM_CS GPIO_PA7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and below: better use the RIOT global GPIO defines -> GPIO_PIN(0, 7)
#define SDCARD_SPI_PARAM_MISO (GPIO_PIN(2,7)) | ||
#endif | ||
#ifndef SDCARD_SPI_PARAM_POWER | ||
#define SDCARD_SPI_PARAM_POWER (GPIO_UNDEF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pickey, but the indention is off. Its somehow unaligned to 4-space boundaries..
re-confirmed working on the |
Oh, and feel free to squash! |
188fcc0
to
87ae9e2
Compare
rebased and squashed |
perfect, then let's see what Murdock has to say about this. |
Murdock complains about missing doc in the file stated above, and there is some trouble with printf formatting on msp and avr8 platforms... |
requires another rebase |
#define SD_CARD_WAIT_AFTER_POWER_UP_US 1000 | ||
|
||
/* R1 response bits (see sd spec. 7.3.2.1 Format R1) */ | ||
#define SD_R1_RESPONSE_PARAM_ERROR 0b01000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to be nitty (: but binary literals are GCC Extensions. I would vote for replacing them with hex numbers or with shifts+ors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@MichelRottleuthner you think you can get the rebase and Murdock fixes done today? Would be nice to merge this before feature freeze... |
Should be possible :) |
87ae9e2
to
7efb117
Compare
@MichelRottleuthner please squash so everything is ready for merging (once CI has passed) |
bda0331
to
ac2ae7c
Compare
squashed |
@smlng, @haukepetersen Murdock and Jenkins are finally successful. Please remove your dismisses and approve this PR ASAP! |
ACK and go |
@aabadie it would be possible to include this to the Xplained extension for samr21? |
This PR adds a driver to read/write sd-cards over spi. Also included is an interactive test application (tests/sdcard_spi) to perform some simple checks with various cards (init card, read/write/copy blocks, show CID and CSR card information).
This driver has been successfully tested with multiple micro-sd cards:
1GB SDSC Kingston
2GB SDSC SanDisk
16GB SDHC SanDisk Ultra
32GB SDHC SanDisk Ultra
32GB SDHC Samsung Evo
128GB SDXC SanDisk Ultra
For now i have only tried this on the samr21-xpro board, but since no board specific features are used it should also work with other platforms.