Rockbox

Tasklist

FS#10633 - iPodNano2G R/W FTL (plus changes needed to make it run)

Attached to Project: Rockbox
Opened by Michael Sparmann (TheSeven) - Friday, 02 October 2009, 03:30 GMT
Last edited by Dave Chapman (linuxstb) - Sunday, 04 October 2009, 20:14 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Here is a patch based on current SVN to implement a R/W FTL on iPodNano2G. It's still very preliminary code, high risk of data corruption, as it hasn't been tested extensively yet. Read-only support seems to be stable now, writing works in most cases, even though I've already seen it fail once, but couldn't reproduce it. So let's test that thing and track those bugs down.
There is quite a lot of unrelated changes, which are either needed to make it work, or just stuff linuxstb has already patched in his working copy but not committed yet.
This task depends upon

Closed by  Dave Chapman (linuxstb)
Sunday, 04 October 2009, 20:14 GMT
Reason for closing:  Accepted
Additional comments about closing:  All relevant code from these patches has now been committed.
Comment by Dave Chapman (linuxstb) - Friday, 02 October 2009, 07:37 GMT
Thanks - that's great!

I'll try and look at it in the next few days and commit it (plus my own outstanding changes), so we're all working from the same code.

Rockbox has coding guidelines in docs/CONTRIBUTING - I see your code doesn't conform in at least a couple of ways (use of TABs and "//" comments), but that's no problem - I'll do whatever is needed to clean it up before committing. But it would be easier if you could follow those for future patches.

Comment by Michael Sparmann (TheSeven) - Friday, 02 October 2009, 10:25 GMT
I'll do some more cleanup today myself, and split that thing into several parts, to make the changes more clear. I'll also try to fix most of the warnings, just didn't want to do this tonight as it was just to late.
/me is wondering how the tabs went in there, as I hate these things myself quite a lot.
Comment by Michael Sparmann (TheSeven) - Friday, 02 October 2009, 15:19 GMT
1_fat-configurable-sectorsize.patch:
Allows targets to set a different sector size than 512 for the storage system. Should be harmless.

2_s5l8701-hw-corrections.patch:
Some hardware define corrections for the S5L8701, needed for the FTL.

3_ibugger-support_startup-fixes.patch:
Some fixes for crt0.S and boot.lds to make it work properly. Also added some things to make DEBUG builds iBugger-compatible.

4_make-app-build-work-properly.patch:
Various fixes to make app builds work, mainly written by linuxstb.

5_FTL.patch:
The actual FTL.
Comment by Michael Sparmann (TheSeven) - Friday, 02 October 2009, 16:47 GMT
I hope kugel is happy now...
Comment by Rob Purchase (shotofadds) - Friday, 02 October 2009, 20:29 GMT
I don't mean to be a party pooper here, but what's the origin of this code? With labels like field_A0, sub_220061E8, etc I'm assuming it has very strong similarities to the OF disassembly. If that's the case I'd be very wary of claiming any copyright over this code, and you may not have the right to GPL license it.
Comment by Dave Chapman (linuxstb) - Friday, 02 October 2009, 23:26 GMT
I've committed patches #1 and #2 (as r22874 and r22875 respectively).

I've also committed my own version of patch #4 (it was originally from my patch) but didn't include the unrelated changes in firmware/SOURCES, just the change of filename for the Nano 2G's button driver.

Regarding patch #3, this appears to be doing too much - changing the debugging info displayed in the bootloader, adding support for iBugger, and changing crt0.S to support an apps build. IIUC, this also breaks the Meizu builds. I'll possibly commit my own changes to crt0.S, and we can then take it from there.

Regarding #5, I share Rob's concerns, and hence am not keen on committing it...
Comment by Dave Chapman (linuxstb) - Friday, 02 October 2009, 23:52 GMT
I've now committed my own pending changes to crt0.S as r22879, most (if not all) are also included in your patch #3.

Are you able to isolate just the bug-fixes to crt0.S in your patch and post a patch just with those?

Thanks.
Comment by MichaelGiacomelli (saratoga) - Saturday, 03 October 2009, 04:14 GMT
>With labels like field_A0, sub_220061E8, etc I'm assuming it has very strong similarities to the OF disassembly. If that's the case I'd be very wary of claiming any copyright over this code, and you may not have the right to GPL license it.

I don't think disassembler offsets can be copyrighted. They're just random computer generated numbers. I'm not sure where field_A0 came from, but unless its actually Apple's variable name (which seems unlikely) I think it's also fine, at least until a better name is available.
Comment by Michael Sparmann (TheSeven) - Saturday, 03 October 2009, 05:29 GMT
field_A0 just means that it is offset 0xA0 in that struct and I don't know what it's good for, as it seems unused. So I don't think this is an issue.
What could be more of an issue are those sub_XXXXXXXXs, which are the addresses of the corresponding norboot functions, more or less as a note where to find these once you're actually gonna implement them. Right now, all of these are just stubs, and they could easily be removed.
Yes, the whole code is quite similar to the OFW's FTL implementation, but this is the only way to stay compatible, and thus able to boot our code in the first place. This also isn't a decompilation, it was written 100% by me, along with the disassembly, of course, but I didn't just try to decompile it manually but rather make sense of it and implement some things in a way that looks more appropriate than what apple's compiler had generated.
Is that kind of code source OK for rockbox? If not, we probably won't ever have a working FTL with write support for this platform, as compatibility without code similarities (or even working out how it works only from disk structures, not from the disassembly) is hardly possible.
I'll probably rework things again in the evening, as I also spotted a few more bugs during testing tonight.
Comment by Rob Purchase (shotofadds) - Saturday, 03 October 2009, 14:08 GMT
> This also isn't a decompilation, it was written 100% by me, along with the disassembly, of course, but I didn't just try to decompile it manually but rather make sense of it and implement some things in a way that looks more appropriate than what apple's compiler had generated.

That's all I wanted to make sure of, that it's your own implementation rather than a decompilation (the numerous sub_* calls in ftl_init do look a little suspicious...) Things like field_A0 are perfectly understandable if these structures need to be written to disk.

If it's all code you've written yourself (using the OF disassembly as a learning resource only) I can't see a problem - after all that's how I wrote the read-only Telechips FTL implementation...
Comment by Michael Sparmann (TheSeven) - Saturday, 03 October 2009, 23:37 GMT
Well, I wrote it while having IDA open on the second screen, so I pretty much followed what apple did, (and thus splitted it into roughly the same subfunctions), but there is no copy and paste or automated transformation at least. One could possibly still interpret that as a derived work, as that clean room probably isn't clean enough. If anybody wants to, feel free to completely rewrite it, I'll happily supply more information to you and ask your questions.

I would actually *want* to remove these sub_XXXXXXXX stubs, as they don't serve any use (besides as a reminder), and one could clean up ftl_init() a lot by removing them. I won't implement these anyways, as there is just no need to. If there is something wrong with the FTL, norboot will have already fixed that when we are called, so this should work perfectly.
Comment by Dave Chapman (linuxstb) - Sunday, 04 October 2009, 10:42 GMT
I've been making a few more commits, so now all plugins and codecs compile with SVN Rockbox.

The attached patch contains (I hope) the remaining changes from Michael's set of patches required for R/W FTL support. I have not included the changes to make Rockbox work with iBugger - I would prefer to deal with those independently.

The next steps before this can be committed are to:

a) Ensure that the changes to crt0.S don't break the Meizu port
b) Clean up the FTL code to remove the sub_XXXXX functions

So for anyone wishing to test Rockbox on the Nano2G, you need to take a clean copy of the current SVN (r22914 or later) and just apply this single patch. Then type "make" followed by "make fullzip" to get the required rockbox.zip to install.

I've also uploaded a test binary (r22914 patched with this patch) here - http://linuxstb.cream.org/rockbox/rockbox-nano2g-r22914-patched.zip (unzip that to the root of your ipod).
Comment by Michael Sparmann (TheSeven) - Sunday, 04 October 2009, 13:11 GMT
OK, i removed the stubs and did some further cleanup, and tried to remove the impact on meizu.
I'm not 100% sure if the crt0.S patch is really needed to make the FTL work, but this one is just *correct* (and tested) for the Nano2G, borrowing the meizu code may have some issues.
The stack size increase doesn't seem neccessary for the core itself, but for some plugins (pictureflow seems to be worst). It is needed because of the increased sector size, as some sector buffers are on the stack.
Comment by Dave Chapman (linuxstb) - Sunday, 04 October 2009, 15:04 GMT
FTL-20091004.patch is now committed as r22918, apart from the changes to power-nano2g.c and system-s5l8700.c
Comment by Michael Sparmann (TheSeven) - Sunday, 04 October 2009, 15:14 GMT
Now that it was committed, here is another little patch to make it unmount the FTL on shutdown.
It also enables system_reboot() to actually reboot.
Comment by Dave Chapman (linuxstb) - Sunday, 04 October 2009, 17:53 GMT
clean-unmount.patch is now committed (r22920), as well as the crt0.S part of crt0.S_and_stack.patch (r22919).

So I think the only outstanding changes from this task are the change to stack size (I want to investigate that further before committing), and the changes to make Rockbox work with iBugger.

Loading...