Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Drivers
  • Assigned To No-one
  • Operating System PortalPlayer-based
  • 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 dreamlayers - 2009-01-02
Last edited by Buschel - 2009-02-21

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

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)
   no_ide0_reset_allhdbased.patch (1 KiB)
Closed by  Buschel
2009-02-21 09:44
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

Submitted for missing targets (H10, iPod color) with r20075.

Buschel commented on 2009-01-03 10:45

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?

nls commented on 2009-01-03 16:45

Have you tested if/how this affects the bootloader too?

cpu98 commented on 2009-01-03 16:52

23.7s → 23.4s
By the way mine takes half a minute :(

dreamlayers commented on 2009-01-04 00:30

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

dreamlayers commented on 2009-01-04 04:04

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

LurkAzusa commented on 2009-01-04 20:05

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.

dreamlayers commented on 2009-01-09 06:41

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.

Buschel commented on 2009-01-09 08:32

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.

Buschel commented on 2009-01-18 12:35

Submitted with r19789 for iPod Video and iPod 4G.

dreamlayers commented on 2009-01-19 18:07

Thanks Andree

According to http://www.ipodlinux.org/wiki/PP5020 and my own observations, IDE0_CFG & 0×80000000 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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing