FS#9500 - Storage rework

Attached to Project: Rockbox
Opened by Frank Gevaerts (fg) - Sunday, 19 October 2008, 13:28 GMT
Last edited by Frank Gevaerts (fg) - Monday, 03 November 2008, 22:16 GMT
Task Type Patches
Category Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch adds a storage_*() abstraction to replace ata_*(). To do that, it also introduces sd_*, nand_*, mmc_* and flash_*.
This should be a good first step to allow multi-driver targets, like the Elio (ATA/SD), or the D2 (NAND/SD).

This is far from done. Some of the missing things are:
- It doesn't compile for the sim yet
- The functions in storage.c should be macros on single-driver targets
- I haven't actually tested this on target
- There is still a lot of confusion between HAVE_HOTSWAP and HAVE_MULTIVOLUME.
- MULTIVOLUME needs to be split in two concepts : drives and partitions
This task depends upon

Closed by  Frank Gevaerts (fg)
Monday, 03 November 2008, 22:16 GMT
Reason for closing:  Accepted
Additional comments about closing:  Other work will go to separate tasks
Comment by Frank Gevaerts (fg) - Sunday, 19 October 2008, 16:22 GMT
I'd also like to get rid of direct access to storage_sleep() and storage_spindown() in the plugin api, and replace them with something that gives hints to the storage layer based on path (or file descriptor)
Comment by Frank Gevaerts (fg) - Wednesday, 29 October 2008, 21:49 GMT
This one gets rid of *_get_identify() for non-ata, and introduces *_get_info() instead. None of the mentioned problems have been solved yet.
Comment by Rob Purchase (shotofadds) - Thursday, 30 October 2008, 11:39 GMT
Hi Frank,

My few comments, based on skimming through the diff over lunch (I haven't tested on any target yet):

1. What you have here is a candidate for "a good first step" already, assuming the sim can be made to compile without too much hassle. It's probably a good idea to split big changes like this into smaller parts, and maybe it's a good idea to keep things that might intentionally change behaviour (eg. disk/partition handling, fixing HAVE_HOTSWAP/HAVE_MULTIVOLUME confusion) to a separate, later patch.

2. HAVE_DISK_STORAGE should probably be HAVE_STORAGE_DISK, to be consistent with the other HAVE_STORAGE_* macros

3. Is it necessary to have macros for storage_* on single-driver targets? Would inlining those functions be sufficient?

4. Isn't flash_* redundant, since the iFP flash driver uses NAND anyway?

5. Does the storage_* layer really need to pass *_spin (etc) down to the sd/nand drivers? Could the underlying null implementations be removed and handled like storage_sleepnow()?

6. If we need to make up "could be better" vendor/model/firmware strings on non-ata, wouldn't it be better not to have them at all?

7. There's a leftover USING_ATA_CALLBACK in plugin.c (ok, it's only in a comment..)

8. firmware/export/flash.h has #define __MMC_H__

9. Are the 'extern' function modifiers in the new headers necessary? The fact that they're in a header at all says they are "external".
Comment by Rob Purchase (shotofadds) - Thursday, 30 October 2008, 11:41 GMT
As an aside, I would have thought this is a prime candiate for discussion on the dev mailing list.
Comment by Frank Gevaerts (fg) - Thursday, 30 October 2008, 12:41 GMT
Thanks for looking at it!

1. I won't commit this in one step. Once I'm reasonably happy with it, my plan is to split it into a set of logically consecutive patches and commit those. I also won't do the disk/partition deconfusion right away, as it is reasonably separated from this.
2. Actually, it means something else. HAVE_DISK_STORAGE and HAVE_FLASH_STORAGE indicate certain features, like spin up times and things like that. HAVE_STORAGE_* basically is a namespace indicator. Maybe this needs to be thought through a bit more.
3. I'd like inline functions as well, but some of these functions get used as callbacks (I think mainly the spinup related ones). Needs more thought.
4. probably. iFP is annoying actually, as it doesn't compile at all.
5. They should probably go, yes.
6. USB storage needs names. I don't think anything else does though.
7. I'll fix that
8. Oops. I'll fix that too (unless I drop the entire file).

As soon as I've tested it a bit, I'll bring it up on the ML. I think it's a bit too invasive to commit without a proper warning.
Comment by Frank Gevaerts (fg) - Thursday, 30 October 2008, 21:02 GMT
Changes since v2:
- now has inline functions
- no flash_* anymore ata_flash.c now also uses nand_*
- no passing down of unneeded functions any more.
- now also has changes to the bootloaders
- the simulators compile and run again

Still totally untested (except for compilation)
I think that this is feature-complete enough for a first phase, but more review (and possibly splitting) is needed.
Most targets need between 100 and 500 bytes more (I think because the new *_get_info() function), but the tcc ones actually need about 2kb less.
Comment by Frank Gevaerts (fg) - Friday, 31 October 2008, 23:46 GMT
Sync to r18950
Comment by Frank Gevaerts (fg) - Saturday, 01 November 2008, 15:06 GMT
Now tested on sansa c200, ipod mini and video (with and without rockbox usb), iriver h10, h120 and h320, gigabeat f,gigabeat s mrobe 100, iaudio x5l, cowon d2. Test planned on Archos Player (waiting for batteries to charge).

Current deltas (RAM usage) are : cowond2 -2064, gigabeatf +352, gigabeats +400, h10_5gb +464, h120 +264, h300 +272, iaudiox5 +288, ipodmini2g +448, ipodvideo +480, mrobe100 +448, player +264, sansac200 -128

*_get_info() is only used by usb_storage.c and debug_menu.c (as a fallback, currently only for tcc players until they have a proper disk_callback() function). Not including that reduces the delta a bit for hardware USB targets. The deltas then go down by 140 bytes for sh, 190 for coldfire and 220 for arm. Is this worth the extra #ifdef in all storage drivers, and what should that #ifdef actually depend on?

Other than possible delta reductions, and assuming that the player test won't give any surprises, I think this is ready for commit.