Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9749 - PP502x: Don't reset IDE0 on startup

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Friday, 02 January 2009, 21:59 GMT+2
Last edited by Andree Buschmann (Buschel) - Saturday, 21 February 2009, 10:44 GMT+2
Task Type Patches
Category Drivers
Status Closed
Assigned To No-one
Player Type PortalPlayer-based
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

I have a 30GB 5th generation iPod with a stock MK3008GAL hard drive. When starting Rockbox, I hear two clicks while the logo is displayed. It seems like the heads are unloaded and then loaded again. This delays startup a bit.

The unload is triggered when DEV_IDE0 is reset in system_init() in firmware/target/arm/system-pp502x.c. The load happens later when needed. From an experiment I found that resetting DEV_IDE0 unfreezes the lock, and the ATA standard says that can only happen if the device is powered down or hard reset. Because of this I think resetting DEV_IDE0 hard resets the ATA device.

DEV_IDE0 resets also seem to reset some host settings because if done after ata_device_init() (which initializes the host, not the device) they break DMA ( FS#9708 ). The following implementation of ata_reset() in firmware/target/arm/ata-pp5020.c works:
DEV_RS = DEV_IDE0;
DEV_RS = 0;
ata_device_init();
However, since everything currently works with an ata_reset() which does nothing, I am not submitting a patch for that.

I am submitting two patches to disable DEV_IDE0 resetting. One disables it only on the Video iPod and one disables it on all HD based pp502x targets. I have only tested this on my Video iPod, so it's up to others to see if this works and speeds up startup on other targets.

This was started at: http://forums.rockbox.org/index.php?topic=19788.0
   no_ide0_reset_ipodvideo.patch (0.5 KiB) 
 rockbox-19642-dma/firmware/target/arm/system-pp502x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

   no_ide0_reset_allhdbased.patch (1 KiB) 
 rockbox-19642-dma/firmware/target/arm/system-pp502x.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

This task depends upon

Closed by  Andree Buschmann (Buschel)
Saturday, 21 February 2009, 10:44 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Submitted for missing targets (H10, iPod color) with r20075.
Comment by Andree Buschmann (Buschel) - Saturday, 03 January 2009, 11:45 GMT+2
Tested with several startups and works fine on my 5.5G 30GB. The startup time is a bit faster with this patch (before: ~8.5s, after: ~8.0s).

Can anybody else please test on other PP-HDD-targets?
Comment by Nils Wallménius (nls) - Saturday, 03 January 2009, 17:45 GMT+2
Have you tested if/how this affects the bootloader too?
Comment by Seheon Ryu (cpu98) - Saturday, 03 January 2009, 17:52 GMT+2
23.7s -> 23.4s
By the way mine takes half a minute :(
Comment by Boris Gjenero (dreamlayers) - Sunday, 04 January 2009, 01:30 GMT+2
This is irrelevant in the boot loader. All changes are in an #ifndef BOOTLOADER block. BTW I have tested the changes if Rockbox replaces the original OS in the firmware partition (with no bootloader), and everything works properly even then.

Do others have any thoughts on whether to implement ata_reset()? It seems wasteful to start hard-resetting on every IDE power-on (causing the drive to restart its startup sequence) if everything works without that. (That's what ata.c tries to do, but can't do on pp502x because ata_reset() does nothing.)
Comment by Boris Gjenero (dreamlayers) - Sunday, 04 January 2009, 05:04 GMT+2
Another reason why this patch may be a good idea: The ATA standard defines proper timings for hardware reset (assert, wait >25 microseconds, negate, wait >2 milliseconds, and only then check status). This is totally disregarded in system_init(). An overly short asserted time can incompletely reset a device and cause it to malfunction (eg. lock up). Checking for status too soon can give false status. I think this is a case of overly short asserted time causing problems: http://forums.rockbox.org/index.php?topic=19953.msg141860#msg141860 For an example of a proper reset, see ata_reset() in http://svn.rockbox.org/viewvc.cgi/trunk/firmware/target/sh/archos/ata-archos.c?view=markup
Comment by Debra Capodice (LurkAzusa) - Sunday, 04 January 2009, 21:05 GMT+2
This patch also fixes the ATA -1 error when using a A-DATA 16 or 32 Gb CF in the ipod series. Tested on ipod4G.
Comment by Boris Gjenero (dreamlayers) - Friday, 09 January 2009, 07:41 GMT+2
On my 5G 30GB with a stock MK3008GAL, without this patch the first init_and_check() in ata_init() fails with error -31. This means check_registers() found STATUS_BSY set, presumably because the drive is still busy after the hard reset caused by the DEV_RS line. It works the second time because the code waits for STATUS_BSY to clear if it thinks it is hard-resetting the ATA device. (It's not actually hard resetting it because ata_reset() does nothing.) With this patch, init_and_check() works the first time.

Comment by Andree Buschmann (Buschel) - Friday, 09 January 2009, 09:32 GMT+2
In my opinion this patch should be submitted to svn -- as a first step for iPod Video only. From all tests it doesn't harm anything, but solves CF-mod issues, speeds up booting and seems to be the proper solution. I am not capable of submitting in the next days (due to vacation:-). So, either somebody else does it or I will do after the vacation.
Comment by Andree Buschmann (Buschel) - Sunday, 18 January 2009, 13:35 GMT+2
Submitted with r19789 for iPod Video and iPod 4G.
Comment by Boris Gjenero (dreamlayers) - Monday, 19 January 2009, 19:07 GMT+2
Thanks Andree

According to http://www.ipodlinux.org/wiki/PP5020 and my own observations, IDE0_CFG & 0x80000000 can be used to hard-reset the ATA device without resetting the controller. Set the bit, wait >25 microseconds, negate, and wait >2 milliseconds. I am not submitting a patch to implement ata_reset() because (as I said earlier) I feel it is unnecessary.

Loading...