char array cast to unsigned array in sdcard.c assumes big endian CPU

[epsilon537]

In sdcard.c functions sdcard_read(), sdcard_write(), sdcard_read_csd() and sdcard_read_cid(), the passed in char buffer gets cast to unsigned, for convenient access to/from sd_fifo[0]. This will only work as intended on a big endian platform. On little endian platforms an endian byte swap is needed.

I also suspect there will be issues if the passed in char* is not word aligned. I haven't verified that though.

[ZipCPU]

This is probably true, as I've only used this with big-endian support.

If I create little endian support, would you check it for me?

[epsilon537]

Yes, of course.

I also want to mention that board.h (which defines the SDSPI_S struct) is not part of the repo. I found it in the zbasic repo. That's OK I think, but it might be worth mentioning somewhere near the top of sdcard.c.

[ZipCPU]

The board.h file found in ZBasic is a computer generated file specific to ZBasic. It would make more sense to incorporate the AutoFPGA file used to generate the SDSPI components. That I can do.

Also, the problem with the routines in sdcard.c isn't the routines themselves, it's the underlying IP. The IP assumes 32'bit big endian access. If I fix that (and it is fairly easily fixed), everything else in sdcard.c should fall into place.

That is --- everything except the word alignment restriction. I'm not sure the best answer to this, save to add comments in sdcard.h that the pointers must be word aligned. The issue here is that this alignment is assumed, and dealing (properly) with realigning data for CPU's that can't handle unaligned data will have rippling consequences throughout diskio.c. (i.e., I'd have to malloc an aligned sector buffer or otherwise allocate it on the stack, copy data to that buffer, then issue the sdcard.c commands, etc.)

Dan

[ZipCPU]

Please check out and evaluate the changes in the dev branch designed to support little endian CPUs. Note that you'll need to set the OPT_LITTLE_ENDIAN parameter to 1'b1 to get this support.

Dan

[epsilon537]

Making the FIFO big or little endian is actually an optimization, to allow casting its contents into or from a byte array with a single word access, instead of processing byte-by-byte. If you're willing to forego that optimization and handle FIFO access in an endianness independent way (i.e. process the word byte-by-byte), then a config parameter is not required, and as a bonus, the source or destination buffer doesn't have to be word-aligned:

For instance: for(j=0; j<512; j+=4) { unsigned w = _sdcard->sd_fifo[0]; buf[0+j] = (char)((w&0xff000000)>>24); buf[1+j] = (char)((w&0x00ff0000)>>16); buf[2+j] = (char)((w&0x0000ff00)>>8); buf[3+j] = (char)((w&0x000000ff)); }

This should work both on a big and a little-endian CPU (assuming the original big endian IP) and buf doesn't have to be word-aligned.

[ZipCPU]

Yes, that would work, but that would also prevent you from transferring data to or from the SDSPI controller via an external DMA.

[epsilon537]

Good point. I haven't worked out yet how DMA fits into this. I guess you have to consider the DMA engine's endianness and buffer alignment requirements.

I pulled in your fix. However, I'm running into an issue when I have the core configured in little-endian mode. I'm running a slightly modified version of zbasic's sdtest.c on verilator using your sdspisim co-simulator. During the sector-write portion of the test, it gets stuck 'waiting on token':

WRITE: Seek to sector 2 SDSPI: Received a command 0x58 (24) arg 0x2 Going to write to block 00000002 of 03a32000 SDSPI: waiting on token SDSPI: waiting on token SDSPI: waiting on token ...

When I run the same test with the sdspi core configured in big-endian mode, it works fine.

sdtest.zip

[ZipCPU]

I see it.

Check out the dev branch for a fix.

Dan

[ZipCPU]

No ... that's not it ... yet ... give me a bit more.

[ZipCPU]

Should be fixed (now) in 59447ae.

I expanded the TB to check for little endian data, and the ability to read/write little endian data to the SPI. Turns out, there were more issues: I had to make sure the CRC was fed the correct data in the correct order, the RX gearbox was dropping data, etc. These should now be fixed, and the design works in both little and big endian modes locally. Feel free to give it another try.

Dan

[epsilon537]

Great! Now it's working. Thank you!

Regarding the buffer alignment issue, I think you made a good point. It's probably best to require word alignment for those buffers to keep things DMA friendly (and that includes the CPU which in a way is a just very configurable DMA engine...). I put buffer alignment assert() checks in sdcard.c in my fork of sdspi.

/Ruben.

[ZipCPU]

Great! Now it's working. Thank you!

Awesome! Let me close this issue then.