This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#9500 - Storage rework
Attached to Project:
Rockbox
Opened by Frank Gevaerts (fg) - Sunday, 19 October 2008, 15:28 GMT+2
Last edited by Frank Gevaerts (fg) - Monday, 03 November 2008, 23:16 GMT+2
Opened by Frank Gevaerts (fg) - Sunday, 19 October 2008, 15:28 GMT+2
Last edited by Frank Gevaerts (fg) - Monday, 03 November 2008, 23:16 GMT+2
|
DetailsThis 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 
storage.diff
(116.9 KiB)
b/apps/buffering.c | 6 b/apps/codecs.c | 2 b/apps/debug_menu.c | 30 +-- b/apps/gui/gwps.c | 8 b/apps/main.c | 12 - b/apps/misc.c | 10 - b/apps/mpeg.c | 14 - b/apps/mpeg.h | 2 b/apps/playback.c | 2 b/apps/playlist.c | 2 b/apps/plugin.c | 13 - b/apps/plugin.h | 14 - b/apps/plugins/SOURCES | 2 b/apps/plugins/alpine_cdc.c | 4 b/apps/plugins/battery_bench.c | 4 b/apps/plugins/clock/clock_settings.c | 2 b/apps/plugins/jpeg.c | 18 - b/apps/plugins/mpegplayer/disk_buf.c | 6 b/apps/plugins/mpegplayer/stream_mgr.h | 2 b/apps/plugins/splitedit.c | 4 b/apps/plugins/test_disk.c | 2 b/apps/plugins/video.c | 2 b/apps/plugins/wavplay.c | 6 b/apps/plugins/wavrecord.c | 6 b/apps/recorder/pcm_record.c | 10 - b/apps/recorder/peakmeter.c | 4 b/apps/recorder/recording.c | 8 b/apps/screens.c | 8 b/apps/screens.h | 2 b/apps/scrobbler.c | 2 b/apps/settings.c | 11 - b/apps/settings_list.c | 4 b/apps/tagcache.c | 2 b/apps/tagtree.c | 3 b/apps/talk.c | 10 - b/apps/tree.c | 4 b/bootloader/main-pp.c | 8 b/firmware/SOURCES | 20 +- b/firmware/ata_idle_notify.c | 10 - b/firmware/common/disk.c | 4 b/firmware/drivers/ata.c | 27 ++ b/firmware/drivers/ata_flash.c | 34 ++- b/firmware/drivers/ata_mmc.c | 31 +-- b/firmware/drivers/fat.c | 22 +- b/firmware/drivers/serial.c | 4 b/firmware/export/ata.h | 20 -- b/firmware/export/ata_idle_notify.h | 28 +-- b/firmware/export/config-c100.h | 2 b/firmware/export/config-c200.h | 2 b/firmware/export/config-cowond2.h | 2 b/firmware/export/config-creativezv.h | 2 b/firmware/export/config-creativezvm.h | 2 b/firmware/export/config-creativezvm60gb.h | 2 b/firmware/export/config-e200.h | 2 b/firmware/export/config-fmrecorder.h | 2 b/firmware/export/config-gigabeat-s.h | 2 b/firmware/export/config-gigabeat.h | 2 b/firmware/export/config-h10.h | 2 b/firmware/export/config-h100.h | 2 b/firmware/export/config-h10_5gb.h | 2 b/firmware/export/config-h120.h | 2 b/firmware/export/config-h300.h | 2 b/firmware/export/config-hdd1630.h | 2 b/firmware/export/config-iaudio7.h | 2 b/firmware/export/config-iaudiom3.h | 2 b/firmware/export/config-iaudiom5.h | 2 b/firmware/export/config-iaudiox5.h | 2 b/firmware/export/config-ifp7xx.h | 2 b/firmware/export/config-ipod1g2g.h | 2 b/firmware/export/config-ipod3g.h | 2 b/firmware/export/config-ipod4g.h | 2 b/firmware/export/config-ipodcolor.h | 2 b/firmware/export/config-ipodmini.h | 2 b/firmware/export/config-ipodmini2g.h | 2 b/firmware/export/config-ipodnano.h | 2 b/firmware/export/config-ipodvideo.h | 2 b/firmware/export/config-logikdax.h | 2 b/firmware/export/config-m200.h | 2 b/firmware/export/config-mrobe100.h | 2 b/firmware/export/config-mrobe500.h | 2 b/firmware/export/config-ondavx747.h | 2 b/firmware/export/config-ondavx767.h | 2 b/firmware/export/config-ondiofm.h | 2 b/firmware/export/config-ondiosp.h | 2 b/firmware/export/config-player.h | 2 b/firmware/export/config-recorder.h | 2 b/firmware/export/config-recorderv2.h | 2 b/firmware/export/config-sa9200.h | 2 b/firmware/export/config-tpj1022.h | 2 b/firmware/export/disk.h | 2 b/firmware/export/fat.h | 2 b/firmware/export/flash.h | 48 +++++ b/firmware/export/hotswap.h | 4 b/firmware/export/mmc.h | 48 +++++ b/firmware/export/mv.h | 41 ++++ b/firmware/export/nand.h | 47 +++++ b/firmware/export/nand_id.h | 2 b/firmware/export/powermgmt.h | 6 b/firmware/export/sd.h | 47 +++++ b/firmware/export/storage.h | 49 +++++ b/firmware/hotswap.c | 2 b/firmware/include/dir.h | 2 b/firmware/powermgmt.c | 24 +- b/firmware/storage.c | 266 +++++++++++++++++++++++++++++ b/firmware/target/arm/ata-nand-telechips.c | 29 +-- b/firmware/target/arm/ata-sd-pp.c | 95 +++++----- b/firmware/usb.c | 36 +-- b/firmware/usbstack/usb_core.c | 4 b/firmware/usbstack/usb_storage.c | 20 +- 109 files changed, 942 insertions(+), 365 deletions(-) |
This task depends upon
Closed by Frank Gevaerts (fg)
Monday, 03 November 2008, 23:16 GMT+2
Reason for closing: Accepted
Additional comments about closing: Other work will go to separate tasks
Monday, 03 November 2008, 23:16 GMT+2
Reason for closing: Accepted
Additional comments about closing: Other work will go to separate tasks
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".
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.
- 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.
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.