• Status Closed
  • Percent Complete
  • Task Type Patches
  • Category Drivers
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by fg - 2008-10-19
Last edited by fg - 2008-11-03

FS#9500 - Storage rework

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

Closed by  fg
2008-11-03 22:16
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Other work will go to separate tasks

fg commented on 2008-10-19 16:22

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)

fg commented on 2008-10-29 21:49

This one gets rid of *_get_identify() for non-ata, and introduces *_get_info() instead. None of the mentioned problems have been solved yet.

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”.

As an aside, I would have thought this is a prime candiate for discussion on the dev mailing list.

fg commented on 2008-10-30 12:41

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.

fg commented on 2008-10-30 21:02

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.

fg commented on 2008-10-31 23:46

Sync to r18950

fg commented on 2008-11-01 15:06

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.


Available keyboard shortcuts


Task Details

Task Editing