ata: Rework power management behavior a bit

After continued reports of corruption using iFlash adapters, I went
digging for more clues, and this combination of changes seemed to
solve data corruption with the iFlash adapters on the ipod video:

 1) Instead of SLEEP, use STANDBY_IMMEDIATE when we detect drive
    as an SSD or CFA-compliant device.  The latter is technically higher
    power than the former, but what this means in practice is unclear.
 2) Don't check ATA powermanagement flag prior to issuing powermgmt
    commands. This reverts the previous "workaround" for the FC1307A --
    and PM is a mandatory part of the ATA spec for any CFA device.
 3) Prior to issuing SLEEP/STANDBY_IMMEDIATE, issue FLUSH CACHE.  The
    ATA spec says this is redundant for the latter, but says nothing
    about the former.  Either way it is always safe to call first.
 4) Delete all other FC1307A_WORKAROUND code related to powermgmt flags.

Change-Id: I492d06664c097d9bbd5cccfb9f5b516da165b1ee

1 files changed, 65 insertions, 42 deletions

diff --git a/firmware/drivers/ata.c b/firmware/drivers/ata.c
index 3514511270..a02539c896 100644
--- a/firmware/drivers/ata.c
+++ b/firmware/drivers/ata.c
@@ -36,14 +36,6 @@
 #include "storage.h"
 #include "logf.h"
 
-/* The FC1307A ATA->SD chipset (used by the common iFlash adapters)
-   doesn't support mandatory ATA power management commands.  Unfortunately
-   simply gating off the SLEEP command isn't sufficient; we need to
-   disable advanced powersaving entirely because otherwise we might
-   kill power before the device has finished flusing writes.
-*/
-//#define FC1307A_WORKAROUND
-
 #define SELECT_DEVICE1  0x10
 #define SELECT_LBA      0x40
 
@@ -62,6 +54,7 @@
 #define CMD_STANDBY                0xE2
 #define CMD_IDENTIFY               0xEC
 #define CMD_SLEEP                  0xE6
+#define CMD_FLUSH_CACHE            0xE7
 #define CMD_SET_FEATURES           0xEF
 #define CMD_SECURITY_FREEZE_LOCK   0xF5
 #ifdef HAVE_ATA_DMA
@@ -75,7 +68,6 @@
 
 #ifdef HAVE_ATA_POWER_OFF
 #define ATA_POWER_OFF_TIMEOUT 2*HZ
-#define ATA_POWER_OFF_TIMEOUT_NOPM 5*HZ
 #endif
 
 #if defined(HAVE_USBSTACK)
@@ -160,12 +152,7 @@ static inline bool ata_sleep_timed_out(void)
 static inline void schedule_ata_power_off(void)
 {
 #ifdef HAVE_ATA_POWER_OFF
-    power_off_tick = current_tick;
-    /* If our device doesn't support SLEEP give a bit more time to flush */
-    if (!(identify_info[82] & (1 << 3)))
-        power_off_tick += ATA_POWER_OFF_TIMEOUT_NOPM;
-    else
-        power_off_tick += ATA_POWER_OFF_TIMEOUT;
+    power_off_tick = current_tick + ATA_POWER_OFF_TIMEOUT;
 #endif
 }
 
@@ -239,12 +226,6 @@ static int ata_perform_wakeup(int state)
 
 static int ata_perform_sleep(void)
 {
-    /* Don't issue the sleep command if the device
-       doesn't support (mandatory!) ATA power management commands!
-    */
-    if (!(identify_info[82] & (1 << 3)))
-       return 0;
-
     logf("ata SLEEP %ld", current_tick);
 
     ATA_OUT8(ATA_SELECT, ata_device);
@@ -254,10 +235,25 @@ static int ata_perform_sleep(void)
         return -1;
     }
 
-    ATA_OUT8(ATA_COMMAND, CMD_SLEEP);
+    /* STANDBY IMMEDIATE
+        - writes all cached data
+        - transitions to PM2:Standby
+        - enters Standby_z power condition
 
-    if (!wait_for_rdy())
-    {
+      This places the device into a state where power-off is safe, but
+      it is not the lowest-theoretical power state -- that is SLEEP, but
+      that is bugged on some SSDs (FC1307A-based).
+
+      TODO: Is there a practical downside to using STANDBY_IMMEDIATE instead
+      of SLEEP, assuming the former spins down the drive?
+    */
+    if (ata_disk_isssd()) {
+        ATA_OUT8(ATA_COMMAND, CMD_STANDBY_IMMEDIATE);
+    } else {
+        ATA_OUT8(ATA_COMMAND, CMD_SLEEP);
+    }
+
+    if (!wait_for_rdy()) {
         DEBUGF("ata_perform_sleep() - CMD failed\n");
         return -2;
     }
@@ -265,6 +261,34 @@ static int ata_perform_sleep(void)
     return 0;
 }
 
+static int ata_perform_flush_cache(void)
+{
+    /* Don't issue the flush cache command if the device
+       doesn't support it, even though it's mandatory.
+    */
+    if (!(identify_info[83] & (1 << 12)))
+       return 0;
+
+    logf("ata FLUSH CACHE %ld", current_tick);
+
+    ATA_OUT8(ATA_SELECT, ata_device);
+
+    if(!wait_for_rdy()) {
+        DEBUGF("ata_perform_flush_cache() - not RDY\n");
+        return -1;
+    }
+
+    ATA_OUT8(ATA_COMMAND, CMD_FLUSH_CACHE);
+
+    if (!wait_for_rdy()) {
+        DEBUGF("ata_perform_flush_cache() - CMD failed\n");
+        return -2;
+    }
+
+    return 0;
+}
+
+
 static ICODE_ATTR int wait_for_start_of_transfer(void)
 {
     if (!wait_for_bsy())
@@ -361,15 +385,27 @@ static ICODE_ATTR void copy_write_sectors(const unsigned char* buf,
 
 int ata_disk_isssd(void)
 {
-    /* offset 217 is "Nominal Rotation rate"
+    /*
+       offset 217 is "Nominal Rotation rate"
        0x0000 == Not reported
        0x0001 == Solid State
        0x0401 -> 0xffe == RPM
        All others reserved
 
-       Some CF cards return 0x0100 (ie byteswapped 0x0001) so accept either
+       Some CF cards return 0x0100 (ie byteswapped 0x0001) so accept either.
+
+       However, this is a very recent change, and we can't rely on it,
+       especially for the FC1307A CF->SD adapters.
+
+       So we have to resort to other heuristics.
+
+       offset 83 b2 is set to show device implementes CFA commands
+       offset 0 is 0x848a for CF, but that's not guaranteed, because reasons.
+
+       These don't guarantee this is an SSD but it's better than nothing.
      */
-    return (identify_info[217] == 0x0001 || identify_info[217] == 0x0100);
+    return (identify_info[83] & (1<<2) ||
+            identify_info[217] == 0x0001 || identify_info[217] == 0x0100);
 }
 
 static int ata_transfer_sectors(unsigned long start,
@@ -836,29 +872,16 @@ void ata_spindown(int seconds)
 
 bool ata_disk_is_active(void)
 {
-#ifdef FC1307A_WORKAROUND
-    /* "active" == "spinning" in this context.
-       without power management this becomes moot */
-    if (!(identify_info[82] & (1 << 3)))
-        return false;
-#endif
-
     return ata_state >= ATA_SPINUP;
 }
 
 void ata_sleepnow(void)
 {
-#ifdef FC1307A_WORKAROUND
-    /* Completely disable all power management */
-    if (!(identify_info[82] & (1 << 3)))
-       return;
-#endif
-
     if (ata_state >= ATA_SPINUP) {
         logf("ata SLEEPNOW %ld", current_tick);
         mutex_lock(&ata_mtx);
         if (ata_state == ATA_ON) {
-            if (!ata_perform_sleep()) {
+            if (!ata_perform_flush_cache() && !ata_perform_sleep()) {
                 ata_state = ATA_SLEEPING;
                 schedule_ata_power_off();
             }
@@ -1093,7 +1116,7 @@ static int set_features(void)
         unsigned char parameter;
     } features[] = {
         { 83, 14, 0x03, 0 },   /* force PIO mode */
-        { 83, 3, 0x05, 0x80 }, /* adv. power management: lowest w/o standby */
+        { 83, 3, 0x05, 0x80 }, /* adv. power management: lowest w/o standby */  // TODO: What about FC1307A that doesn't advertise this properly?
         { 83, 9, 0x42, 0x80 }, /* acoustic management: lowest noise */
         { 82, 6, 0xaa, 0 },    /* enable read look-ahead */
 #ifdef HAVE_ATA_DMA