Hi again, Marc.

On Wed, Sep 07, 2005 at 11:37:12AM +0200, Marc Bevand wrote:

Hi Tejun.

Tejun Heo wrote:
| 
|  Here's a big patch against sil24-original branch.  It contains the
| followings.
| 
|  a. build fix & oops fix for sil24-original branch
|  b. backported sata_sil24_new driver
|  c. debug messages (you don't need to turn on ATA_DEBUG)
|  d. 50ms sleep after completion of IDENTIFY before actually reading data
| 
|  Building both sata_sil24 and sata_sil24_new as modules and
| loading/unloading them work on my machine.  Can you please try this
| and let me know how it goes?

Real quick update on this (it's 2:40 am here): sata_sil24_new still
does not detects my disks while sata_sil24 works perfectly. I will post
the full dmesg's + the various things I tried tomorrow.

 I compared original and new sil24 and here are things that are done
differently upto the point where identify command is issued.

 a. Stuff done inside sil_init_pm() in the original driver is not done
    in the new driver.  This includes clearing PM_EN and RESUME bits
    in the ctrl/stat register and DVR_RST'ing the port.

 b. Original driver performs both SATA phy reset and soft reset.  New
    driver performs only SATA phy reset.

 c. New driver uses different order when initializing irq mask and
    doesn't turn on phy status change interrupt.

 d. New driver doesn't turn off SATA link power management.

 We've already tried #d, and I don't think #c has anything to do with
it as inserting 50ms sleep before reading data doesn't make any
difference.  So, it leaves us with #a and #b.  Here's a patch for #a.
If this doesn't work, I'll make a patch for #b.

 This patch is on top of previous test patch which backported
sil24_new.  Please try this one out and let me know the result.

 Thanks.

diff --git a/drivers/scsi/sata_sil24_new.c b/drivers/scsi/sata_sil24_new.c
--- a/drivers/scsi/sata_sil24_new.c
+++ b/drivers/scsi/sata_sil24_new.c

@@ -431,15 +431,11 @@ static void sil24_irq_clear(struct ata_p
 	/* unused */
 }
 
-static void sil24_reset_controller(struct ata_port *ap)
+static int __sil24_reset_controller(void *port)
 {
-	void *port = (void *)ap->ioaddr.cmd_addr;
 	int cnt;
 	u32 tmp;
 
-	printk(KERN_NOTICE DRV_NAME
-	       " ata%u: resetting controller...\n", ap->id);
-
 	/* Reset controller state.  Is this correct? */
 	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
 	readl(port + PORT_CTRL_STAT);	/* sync */

@@ -451,7 +447,17 @@ static void sil24_reset_controller(struc
 		if (!(tmp & PORT_CS_DEV_RST))
 			break;
 	}
+
 	if (tmp & PORT_CS_DEV_RST)
+		return -1;
+	return 0;
+}
+
+static void sil24_reset_controller(struct ata_port *ap)
+{
+	printk(KERN_NOTICE DRV_NAME
+	       " ata%u: resetting controller...\n", ap->id);
+	if (__sil24_reset_controller((void *)ap->ioaddr.cmd_addr))
 		printk(KERN_ERR DRV_NAME
 		       " ata%u: failed to reset controller\n", ap->id);
 }

@@ -745,6 +751,15 @@ static int sil24_init_one(struct pci_dev
 		/* Clear interrupts */
 		writel(0x0fff0fff, port + PORT_IRQ_STAT);
 		writel(PORT_CS_IRQ_WOC, port + PORT_CTRL_CLR);
+
+		/* Clear port enable and resume bits */
+		writel(PORT_CS_PM_EN | PORT_CS_RESUME, port + PORT_CTRL_CLR);
+
+		/* Reset itself */
+		if (__sil24_reset_controller(port))
+			printk(KERN_ERR DRV_NAME
+			       "(%s): failed to reset controller\n",
+			       pci_name(pdev));
 	}
 
 	/* Turn on interrupts */