[PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()

[PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()

[Tejun Heo]

This patch adds probeinit component operation to
ata_drive_probe_reset().  If present, this new operation is called
before performing any reset.  The operations's roll is to prepare @ap
for following probe-reset operations.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    9 +++++++--
 include/linux/libata.h     |    2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

e854fb48e07ab9ae790280843fdd6d7f2bee9be3
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index e302545..8bd197d 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2485,7 +2485,8 @@ int ata_std_probe_reset(struct ata_port 
 	if (ap->flags & ATA_FLAG_SATA && ap->ops->scr_read)
 		hardreset = sata_std_hardreset;
 
-	return ata_drive_probe_reset(ap, ata_std_softreset, hardreset,
+	return ata_drive_probe_reset(ap, NULL,
+				     ata_std_softreset, hardreset,
 				     ata_std_postreset, classes);
 }
 
@@ -2524,6 +2525,7 @@ static int do_probe_reset(struct ata_por
 /**
  *	ata_drive_probe_reset - Perform probe reset with given methods
  *	@ap: port to reset
+ *	@probeinit: probeinit method (can be NULL)
  *	@softreset: softreset method (can be NULL)
  *	@hardreset: hardreset method (can be NULL)
  *	@postreset: postreset method (can be NULL)
@@ -2552,12 +2554,15 @@ static int do_probe_reset(struct ata_por
  *	if classification fails, and any error code from reset
  *	methods.
  */
-int ata_drive_probe_reset(struct ata_port *ap,
+int ata_drive_probe_reset(struct ata_port *ap, ata_probeinit_fn_t probeinit,
 			  ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			  ata_postreset_fn_t postreset, unsigned int *classes)
 {
 	int rc = -EINVAL;
 
+	if (probeinit)
+		probeinit(ap);
+
 	if (softreset) {
 		rc = do_probe_reset(ap, softreset, postreset, classes);
 		if (rc == 0)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f4cd1eb..e8f29ce 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -244,6 +244,7 @@ struct ata_queued_cmd;
 
 /* typedefs */
 typedef void (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
+typedef void (*ata_probeinit_fn_t)(struct ata_port *);
 typedef int (*ata_reset_fn_t)(struct ata_port *, int, unsigned int *);
 typedef void (*ata_postreset_fn_t)(struct ata_port *ap, unsigned int *);
 
@@ -484,6 +485,7 @@ extern void __sata_phy_reset(struct ata_
 extern void sata_phy_reset(struct ata_port *ap);
 extern void ata_bus_reset(struct ata_port *ap);
 extern int ata_drive_probe_reset(struct ata_port *ap,
+			ata_probeinit_fn_t probeinit,
 			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			ata_postreset_fn_t postreset, unsigned int *classes);
 extern int ata_std_softreset(struct ata_port *ap, int verbose,
-- 
1.1.3

[PATCH 01/11] libata: fix ata_std_probe_reset() SATA detection

[Tejun Heo]

ap->cbl is initialized during postreset and thus unknown on entry to
ata_std_probe_reset().  This patch makes ata_std_probe_reset() use
ATA_FLAG_SATA flag instead of ap->cbl to detect SATA port.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

f820ad6fcf9839d76cfbaa6553c3875b0d7c0657
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 32ac63c..1d106b4 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2472,7 +2472,7 @@ int ata_std_probe_reset(struct ata_port 
 	ata_reset_fn_t hardreset;
 
 	hardreset = NULL;
-	if (ap->cbl == ATA_CBL_SATA && ap->ops->scr_read)
+	if (ap->flags & ATA_FLAG_SATA && ap->ops->scr_read)
 		hardreset = sata_std_hardreset;
 
 	return ata_drive_probe_reset(ap, ata_std_softreset, hardreset,
-- 
1.1.3

[PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3

[Tejun Heo]

Hello, all.

This is the third take of new reset mechanism.  5 of 12 patches made
into upstream in the last iteration [1].  Patches from #06 are dropped
due to incorrect probe_reset implementation.

This patchset is against...

  upstream (e4e7b89280d1d666e2c09e5ad36cf071796c4c7e)
  + [PATCHSET] libata: various fixes related to EH, take #3 [2]
  + [PATCH] ahci: separate out ahci_fill_cmd_slot() [3]

Changes from the last itertion are...

* #01 fixes SATA detection bug in ata_std_probe_reset()

* #02-#04 implements probeinit component operation to properly wake
  sata phy before softreset

* #05-#11 are #06-#12 from the last iteration adapted to above changes

Jeff, after patches upto #04 are applied, the following differences
remain between the new reset mechanism and the original one.

1. For softreset

In the original code, ata_busy_sleep() is done after waking up the PHY
whether SATA_RESET is used or not.  For SRST case, it looks like the
following.

	Original			New
-----------------------------------------------------------------
a1. Wake up PHY using SControl	b1. Wake up PHY using SControl

a2. ata_busy_sleep(),
    if sata_dev_present()

a3. dev_select(ap, 0)		b2. dev_select(ap, 0)

a4. do softreset		b3. do softreset

a5. ata_busy_sleep() for	b4. ata_busy_sleep() for
    present devices in		    present devices in
    ata_bus_post_reset()	    ata_bus_post_reset()


I thought the ata_busy_sleep() in #a2 was for SATA_RESET case and left
it out.  Do you think it's necessary for SRST too?


2. For hardreset

Unlike in the original code, the new reset mechanism first wake up the
PHY before invoking sata_std_hardreset() even when
sata_std_hardreset() is the only reset method.  And dev_select(ap, 0)
is not performed for hardreset.

So, the diffrence looks like the following.

	Original			New
-----------------------------------------------------------------
				b1. Wake up PHY using SControl

a1. Reset PHY using SControl	b2. Reset PHY using SControl

a2. Wake up PHY using SControl	b3. Wakeup PHY using SControl

a3. ata_busy_sleep(),		b4. ata_busy_sleep(),
    if sata_dev_present()	    if sata_dev_present()

a4. dev_select(ap, 0)


#b1 is there as probeinit operation and as most controllers implement
softreset, there will be softreset operations between #b1 and #b2 in
most cases.  The dev_select(ap, 0) in #a4 is actually for SRST and
EDD.  For SATA_RESET, double dev_select()'s are soon done later in
ata_bus_reset() with only ata_irq_on() inbetween.


3. During postreset

The original code does ata_irq_on() if ctl_addr is non-NULL, double
select and ctl setup.  The new code does double select and
ata_irq_on() if ctl_addr is non-NULL.

	Original			New
-----------------------------------------------------------------
a1. ata_irq_on() if ctl_addr

a2. double select		b1. double select

a3. ctl setup
				b2. ata_irq_on() if ctl_addr


ata_irq_on() actually implies ctl setup.  Also, in the original code,
'if ctl_addr' test in #a1 is bogus because in #a3, ctl_addr is used
unchecked.  New code just does ata_irq_on() at the end.


IMHO, the remaining differences seem harmless and mainly due to the
fact that new code splits soft and hard resets explicitly whereas the
original code shared a lot of code path.  If any of above changes
seems suspicious, please let me know.

Thanks.

--
tejun

[1] http://marc.theaimsgroup.com/?l=linux-ide&m=113809002924734&w=2
[2] http://marc.theaimsgroup.com/?l=linux-ide&m=113880953326268&w=2
[3] http://marc.theaimsgroup.com/?l=linux-ide&m=113881369505598&w=2

[PATCH 10/11] ahci: convert to new reset mechanism

[Tejun Heo]

Convert ahci ->phy_reset to new reset mechanism.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

bc104d8fbf1cc23220be6ae5e7a02881fe4fbfcd
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 1f81333..46dc3ae 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -190,7 +190,7 @@ static void ahci_scr_write (struct ata_p
 static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
 static irqreturn_t ahci_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
-static void ahci_phy_reset(struct ata_port *ap);
+static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void ahci_irq_clear(struct ata_port *ap);
 static void ahci_eng_timeout(struct ata_port *ap);
 static int ahci_port_start(struct ata_port *ap);
@@ -229,7 +229,7 @@ static const struct ata_port_operations 
 
 	.tf_read		= ahci_tf_read,
 
-	.phy_reset		= ahci_phy_reset,
+	.probe_reset		= ahci_probe_reset,
 
 	.qc_prep		= ahci_qc_prep,
 	.qc_issue		= ahci_qc_issue,
@@ -251,8 +251,7 @@ static const struct ata_port_info ahci_p
 	{
 		.sht		= &ahci_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
-				  ATA_FLAG_PIO_DMA,
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
@@ -515,28 +514,35 @@ static inline void ahci_fill_cmd_slot(st
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
-static void ahci_phy_reset(struct ata_port *ap)
+static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
-	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
-	struct ata_device *dev = &ap->device[0];
-	u32 new_tmp, tmp;
+	int rc;
+
+	DPRINTK("ENTER\n");
 
 	ahci_stop_engine(ap);
-	__sata_phy_reset(ap);
+	rc = sata_std_hardreset(ap, verbose, class);
 	ahci_start_engine(ap);
 
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		return;
+	if (rc == 0)
+		*class = ahci_dev_classify(ap);
+	if (*class == ATA_DEV_UNKNOWN)
+		*class = ATA_DEV_NONE;
 
-	dev->class = ahci_dev_classify(ap);
-	if (!ata_dev_present(dev)) {
-		ata_port_disable(ap);
-		return;
-	}
+	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
+	return rc;
+}
+
+static void ahci_postreset(struct ata_port *ap, unsigned int *class)
+{
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	u32 new_tmp, tmp;
+
+	ata_std_postreset(ap, class);
 
 	/* Make sure port's ATAPI bit is set appropriately */
 	new_tmp = tmp = readl(port_mmio + PORT_CMD);
-	if (dev->class == ATA_DEV_ATAPI)
+	if (*class == ATA_DEV_ATAPI)
 		new_tmp |= PORT_CMD_ATAPI;
 	else
 		new_tmp &= ~PORT_CMD_ATAPI;
@@ -546,6 +552,13 @@ static void ahci_phy_reset(struct ata_po
 	}
 }
 
+static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
+{
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     NULL, ahci_hardreset,
+				     ahci_postreset, classes);
+}
+
 static u8 ahci_check_status(struct ata_port *ap)
 {
 	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
-- 
1.1.3

[PATCH 02/11] libata: separate out sata_phy_resume() from sata_std_hardreset()

[Tejun Heo]

This patch separates out sata_phy_resume() from sata_std_hardreset().
The function will later be used by probeinit callback.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   32 +++++++++++++++++++++-----------
 1 files changed, 21 insertions(+), 11 deletions(-)

11494a4da9ba499fd04996d85322ddc462fe47dc
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 1d106b4..e302545 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2277,6 +2277,24 @@ err_out:
 	DPRINTK("EXIT\n");
 }
 
+static int sata_phy_resume(struct ata_port *ap)
+{
+	unsigned long timeout = jiffies + (HZ * 5);
+	u32 sstatus;
+
+	scr_write_flush(ap, SCR_CONTROL, 0x300);
+
+	/* Wait for phy to become ready, if necessary. */
+	do {
+		msleep(200);
+		sstatus = scr_read(ap, SCR_STATUS);
+		if ((sstatus & 0xf) != 1)
+			return 0;
+	} while (time_before(jiffies, timeout));
+
+	return -1;
+}
+
 /**
  *	ata_std_softreset - reset host port via ATA SRST
  *	@ap: port to reset
@@ -2357,8 +2375,7 @@ int ata_std_softreset(struct ata_port *a
  */
 int sata_std_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
-	u32 sstatus, serror;
-	unsigned long timeout = jiffies + (HZ * 5);
+	u32 serror;
 
 	DPRINTK("ENTER\n");
 
@@ -2371,15 +2388,8 @@ int sata_std_hardreset(struct ata_port *
 	 */
 	msleep(1);
 
-	scr_write_flush(ap, SCR_CONTROL, 0x300);
-
-	/* Wait for phy to become ready, if necessary. */
-	do {
-		msleep(200);
-		sstatus = scr_read(ap, SCR_STATUS);
-		if ((sstatus & 0xf) != 1)
-			break;
-	} while (time_before(jiffies, timeout));
+	/* Bring phy back */
+	sata_phy_resume(ap);
 
 	/* Clear SError */
 	serror = scr_read(ap, SCR_ERROR);
-- 
1.1.3

[PATCH 09/11] ata_piix: convert sata to new reset mechanism

[Tejun Heo]

Convert ata_piix sata ->phy_reset to new reset mechanism.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)

bd10b2138fbcf1c8c7741f92f3f10bf65b6899c3
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index f4215c9..c2561f7 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -132,7 +132,7 @@ static int piix_init_one (struct pci_dev
 				    const struct pci_device_id *ent);
 
 static int piix_pata_probe_reset(struct ata_port *ap, unsigned int *classes);
-static void piix_sata_phy_reset(struct ata_port *ap);
+static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
 
@@ -235,7 +235,7 @@ static const struct ata_port_operations 
 	.exec_command		= ata_exec_command,
 	.dev_select		= ata_std_dev_select,
 
-	.phy_reset		= piix_sata_phy_reset,
+	.probe_reset		= piix_sata_probe_reset,
 
 	.bmdma_setup		= ata_bmdma_setup,
 	.bmdma_start		= ata_bmdma_start,
@@ -272,8 +272,8 @@ static struct ata_port_info piix_port_in
 	/* ich5_sata */
 	{
 		.sht		= &piix_sht,
-		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_SRST |
-				  PIIX_FLAG_COMBINED | PIIX_FLAG_CHECKINTR,
+		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED |
+				  PIIX_FLAG_CHECKINTR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
@@ -297,8 +297,7 @@ static struct ata_port_info piix_port_in
 	/* ich6_sata */
 	{
 		.sht		= &piix_sht,
-		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_SRST |
-				  PIIX_FLAG_COMBINED_ICH6 |
+		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
 				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
@@ -309,8 +308,7 @@ static struct ata_port_info piix_port_in
 	/* ich6_sata_ahci */
 	{
 		.sht		= &piix_sht,
-		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_SRST |
-				  PIIX_FLAG_COMBINED_ICH6 |
+		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
 				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
 				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
@@ -455,28 +453,29 @@ static int piix_sata_probe (struct ata_p
 }
 
 /**
- *	piix_sata_phy_reset - Probe specified port on SATA host controller
- *	@ap: Port to probe
+ *	piix_sata_probe_reset - Perform reset on SATA port and classify
+ *	@ap: Port to reset
+ *	@classes: Resulting classes of attached devices
  *
- *	Probe SATA phy.
+ *	Reset SATA phy and classify attached devices.
  *
  *	LOCKING:
  *	None (inherited from caller).
  */
-
-static void piix_sata_phy_reset(struct ata_port *ap)
+static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	int i;
+
 	if (!piix_sata_probe(ap)) {
-		ata_port_disable(ap);
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			classes[i] = ATA_DEV_NONE;
 		printk(KERN_INFO "ata%u: SATA port has no device.\n", ap->id);
-		return;
+		return 0;
 	}
 
-	ap->cbl = ATA_CBL_SATA;
-
-	ata_port_probe(ap);
-
-	ata_bus_reset(ap);
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     ata_std_softreset, NULL,
+				     ata_std_postreset, classes);
 }
 
 /**
-- 
1.1.3

[PATCH 04/11] libata: implement ata_std_probeinit()

[Tejun Heo]

This patch implements the off-the-shelf probeinit component operation.
Currently, all it does is waking up the PHY if it's a SATA port.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   16 +++++++++++++++-
 include/linux/libata.h     |    1 +
 2 files changed, 16 insertions(+), 1 deletions(-)

9be7440acf08c8100e70ab3406a956c5d8086c0a
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8bd197d..fac9677 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2296,6 +2296,19 @@ static int sata_phy_resume(struct ata_po
 }
 
 /**
+ *	ata_std_probeinit - initialize probing
+ *	@ap: port to be probed
+ *
+ *	@ap is about to be probed.  Initialize it.  This function is
+ *	to be used as standard callback for ata_drive_probe_reset().
+ */
+extern void ata_std_probeinit(struct ata_port *ap)
+{
+	if (ap->flags & ATA_FLAG_SATA && ap->ops->scr_read)
+		sata_phy_resume(ap);
+}
+
+/**
  *	ata_std_softreset - reset host port via ATA SRST
  *	@ap: port to reset
  *	@verbose: fail verbosely
@@ -2485,7 +2498,7 @@ int ata_std_probe_reset(struct ata_port 
 	if (ap->flags & ATA_FLAG_SATA && ap->ops->scr_read)
 		hardreset = sata_std_hardreset;
 
-	return ata_drive_probe_reset(ap, NULL,
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ata_std_softreset, hardreset,
 				     ata_std_postreset, classes);
 }
@@ -5525,6 +5538,7 @@ EXPORT_SYMBOL_GPL(ata_port_probe);
 EXPORT_SYMBOL_GPL(sata_phy_reset);
 EXPORT_SYMBOL_GPL(__sata_phy_reset);
 EXPORT_SYMBOL_GPL(ata_bus_reset);
+EXPORT_SYMBOL_GPL(ata_std_probeinit);
 EXPORT_SYMBOL_GPL(ata_std_softreset);
 EXPORT_SYMBOL_GPL(sata_std_hardreset);
 EXPORT_SYMBOL_GPL(ata_std_postreset);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e8f29ce..68b3fe6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -488,6 +488,7 @@ extern int ata_drive_probe_reset(struct 
 			ata_probeinit_fn_t probeinit,
 			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			ata_postreset_fn_t postreset, unsigned int *classes);
+extern void ata_std_probeinit(struct ata_port *ap);
 extern int ata_std_softreset(struct ata_port *ap, int verbose,
 			     unsigned int *classes);
 extern int sata_std_hardreset(struct ata_port *ap, int verbose,
-- 
1.1.3

[PATCH 05/11] sata_sil: convert to new reset mechanism

[Tejun Heo]

Convert sata_sil to use new reset mechanism.  sata_sil is fairly
generic and can directly use std routine.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/sata_sil.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

8d307a344239cb9d6ab516e36e91c69154603f03
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index b017f85..6c8becc 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -157,7 +157,7 @@ static const struct ata_port_operations 
 	.check_status		= ata_check_status,
 	.exec_command		= ata_exec_command,
 	.dev_select		= ata_std_dev_select,
-	.phy_reset		= sata_phy_reset,
+	.probe_reset		= ata_std_probe_reset,
 	.post_set_mode		= sil_post_set_mode,
 	.bmdma_setup            = ata_bmdma_setup,
 	.bmdma_start            = ata_bmdma_start,
@@ -180,7 +180,7 @@ static const struct ata_port_info sil_po
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_MMIO,
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -189,8 +189,7 @@ static const struct ata_port_info sil_po
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
-				  SIL_FLAG_MOD15WRITE,
+				  ATA_FLAG_MMIO | SIL_FLAG_MOD15WRITE,
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -199,7 +198,7 @@ static const struct ata_port_info sil_po
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_MMIO,
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
-- 
1.1.3

[PATCH 11/11] ahci: add softreset

[Tejun Heo]

Now that libata is smart enought to handle both soft and hard resets,
add softreset method.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 133 insertions(+), 1 deletions(-)

ab703283fe99d9edebf40b84f7cdc53d398eb520
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 46dc3ae..76f89f9 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -514,6 +514,138 @@ static inline void ahci_fill_cmd_slot(st
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
+static int ahci_wait_for_bit(void __iomem *reg, u32 mask, u32 val,
+			     unsigned long interval_msec,
+			     unsigned long timeout_msec)
+{
+	unsigned long timeout;
+	u32 tmp;
+
+	timeout = jiffies + (timeout_msec * HZ) / 1000;
+	do {
+		tmp = readl(reg);
+		if ((tmp & mask) != val)
+			return 0;
+		msleep(interval_msec);
+	} while (time_before(jiffies, timeout));
+
+	return -1;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
+	const u32 cmd_fis_len = 5; /* five dwords */
+	const char *reason = NULL;
+	struct ata_taskfile tf;
+	u8 *fis;
+	int rc;
+
+	DPRINTK("ENTER\n");
+
+	/* prepare for SRST (AHCI-1.1 10.4.1) */
+	rc = ahci_stop_engine(ap);
+	if (rc) {
+		reason = "failed to stop engine";
+		goto fail_restart;
+	}
+
+	/* check BUSY/DRQ, perform Command List Override if necessary */
+	ahci_tf_read(ap, &tf);
+	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
+		u32 tmp;
+
+		if (!(hpriv->cap & HOST_CAP_CLO)) {
+			rc = -EIO;
+			reason = "port busy but no CLO";
+			goto fail_restart;
+		}
+
+		tmp = readl(port_mmio + PORT_CMD);
+		tmp |= PORT_CMD_CLO;
+		writel(tmp, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD); /* flush */
+
+		if (ahci_wait_for_bit(port_mmio + PORT_CMD,
+				      PORT_CMD_CLO, PORT_CMD_CLO, 1, 500)) {
+			rc = -EIO;
+			reason = "CLO failed";
+			goto fail_restart;
+		}
+	}
+
+	/* restart engine */
+	ahci_start_engine(ap);
+
+	ata_tf_init(ap, &tf, 0);
+	fis = pp->cmd_tbl;
+
+	/* issue the first D2H Register FIS */
+	ahci_fill_cmd_slot(ap, cmd_fis_len | AHCI_CMD_RESET | AHCI_CMD_CLR_BUSY);
+
+	tf.ctl |= ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	if (ahci_wait_for_bit(port_mmio + PORT_CMD_ISSUE, 0x1, 0x1, 1, 500)) {
+		rc = -EIO;
+		reason = "1st FIS failed";
+		goto fail;
+	}
+
+	/* spec says at least 5us, but be generous and sleep for 1ms */
+	msleep(1);
+
+	/* issue the second D2H Register FIS */
+	ahci_fill_cmd_slot(ap, cmd_fis_len);
+
+	tf.ctl &= ~ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	/* spec mandates ">= 2ms" before checking status.
+	 * We wait 150ms, because that was the magic delay used for
+	 * ATAPI devices in Hale Landis's ATADRVR, for the period of time
+	 * between when the ATA command register is written, and then
+	 * status is checked.  Because waiting for "a while" before
+	 * checking status is fine, post SRST, we perform this magic
+	 * delay here as well.
+	 */
+	msleep(150);
+
+	*class = ATA_DEV_NONE;
+	if (sata_dev_present(ap)) {
+		if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
+			rc = -EIO;
+			reason = "device not ready";
+			goto fail;
+		}
+		*class = ahci_dev_classify(ap);
+	}
+
+	DPRINTK("EXIT, class=%u\n", *class);
+	return 0;
+
+ fail_restart:
+	ahci_start_engine(ap);
+ fail:
+	if (verbose)
+		printk(KERN_ERR "ata%u: softreset failed (%s)\n",
+		       ap->id, reason);
+	else
+		DPRINTK("EXIT, rc=%d reason=\"%s\"\n", rc, reason);
+	return rc;
+}
+
 static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
 	int rc;
@@ -555,7 +687,7 @@ static void ahci_postreset(struct ata_po
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
-				     NULL, ahci_hardreset,
+				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);
 }
 
-- 
1.1.3

[PATCH 08/11] ata_piix: convert pata to new reset mechanism

[Tejun Heo]

Convert ata_piix pata ->phy_reset to new reset mechanism.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |   47 +++++++++++++++++++++++++++++++----------------
 1 files changed, 31 insertions(+), 16 deletions(-)

4c83a0664401bd8a04813e810eb1a828f0aeafb2
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 49cc420..f4215c9 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -131,7 +131,7 @@ enum {
 static int piix_init_one (struct pci_dev *pdev,
 				    const struct pci_device_id *ent);
 
-static void piix_pata_phy_reset(struct ata_port *ap);
+static int piix_pata_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void piix_sata_phy_reset(struct ata_port *ap);
 static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
@@ -207,7 +207,7 @@ static const struct ata_port_operations 
 	.exec_command		= ata_exec_command,
 	.dev_select		= ata_std_dev_select,
 
-	.phy_reset		= piix_pata_phy_reset,
+	.probe_reset		= piix_pata_probe_reset,
 
 	.bmdma_setup		= ata_bmdma_setup,
 	.bmdma_start		= ata_bmdma_start,
@@ -258,8 +258,7 @@ static struct ata_port_info piix_port_in
 	/* ich5_pata */
 	{
 		.sht		= &piix_sht,
-		.host_flags	= ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST |
-				  PIIX_FLAG_CHECKINTR,
+		.host_flags	= ATA_FLAG_SLAVE_POSS | PIIX_FLAG_CHECKINTR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 #if 0
 		.mwdma_mask	= 0x06, /* mwdma1-2 */
@@ -284,7 +283,7 @@ static struct ata_port_info piix_port_in
 	/* piix4_pata */
 	{
 		.sht		= &piix_sht,
-		.host_flags	= ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST,
+		.host_flags	= ATA_FLAG_SLAVE_POSS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 #if 0
 		.mwdma_mask	= 0x06, /* mwdma1-2 */
@@ -366,30 +365,46 @@ cbl40:
 }
 
 /**
- *	piix_pata_phy_reset - Probe specified port on PATA host controller
- *	@ap: Port to probe
+ *	piix_pata_postreset - Postreset stuff on PATA host controller
+ *	@ap: Target port
+ *	@classes: Classes of attached devices
  *
- *	Probe PATA phy.
+ *	Postreset processing including cable detection.
  *
  *	LOCKING:
  *	None (inherited from caller).
  */
+static void piix_pata_postreset(struct ata_port *ap, unsigned int *classes)
+{
+	piix_pata_cbl_detect(ap);
+	ata_std_postreset(ap, classes);
+}
 
-static void piix_pata_phy_reset(struct ata_port *ap)
+/**
+ *	piix_pata_probe_reset - Perform reset on PATA port and classify
+ *	@ap: Port to reset
+ *	@classes: Resulting classes of attached devices
+ *
+ *	Reset PATA phy and classify attached devices.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+static int piix_pata_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
+	int i;
 
 	if (!pci_test_config_bits(pdev, &piix_enable_bits[ap->hard_port_no])) {
-		ata_port_disable(ap);
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			classes[i] = ATA_DEV_NONE;
 		printk(KERN_INFO "ata%u: port disabled. ignoring.\n", ap->id);
-		return;
+		return 0;
 	}
 
-	piix_pata_cbl_detect(ap);
-
-	ata_port_probe(ap);
-
-	ata_bus_reset(ap);
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     ata_std_softreset, NULL,
+				     piix_pata_postreset, classes);
 }
 
 /**
-- 
1.1.3

[PATCH 06/11] sata_sil24: convert to new reset mechanism

[Tejun Heo]

Convert sata_sil24 ->phy_reset to new reset mechanism.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/sata_sil24.c |   56 +++++++++++++++++++++++----------------------
 1 files changed, 28 insertions(+), 28 deletions(-)

f71741b6ef0240a346963e05ac6808fb9478a45e
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 1160fda..a529628 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -249,7 +249,7 @@ static u8 sil24_check_status(struct ata_
 static u32 sil24_scr_read(struct ata_port *ap, unsigned sc_reg);
 static void sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val);
 static void sil24_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
-static void sil24_phy_reset(struct ata_port *ap);
+static int sil24_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void sil24_qc_prep(struct ata_queued_cmd *qc);
 static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc);
 static void sil24_irq_clear(struct ata_port *ap);
@@ -305,7 +305,7 @@ static const struct ata_port_operations 
 
 	.tf_read		= sil24_tf_read,
 
-	.phy_reset		= sil24_phy_reset,
+	.probe_reset		= sil24_probe_reset,
 
 	.qc_prep		= sil24_qc_prep,
 	.qc_issue		= sil24_qc_issue,
@@ -335,8 +335,8 @@ static struct ata_port_info sil24_port_i
 	{
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
-				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(4),
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  SIL24_NPORTS2FLAG(4),
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -346,8 +346,8 @@ static struct ata_port_info sil24_port_i
 	{
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
-				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(2),
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  SIL24_NPORTS2FLAG(2),
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -357,8 +357,8 @@ static struct ata_port_info sil24_port_i
 	{
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
-				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(1),
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  SIL24_NPORTS2FLAG(1),
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -427,7 +427,8 @@ static void sil24_tf_read(struct ata_por
 	*tf = pp->tf;
 }
 
-static int sil24_issue_SRST(struct ata_port *ap)
+static int sil24_softreset(struct ata_port *ap, int verbose,
+			   unsigned int *class)
 {
 	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
 	struct sil24_port_priv *pp = ap->private_data;
@@ -436,6 +437,8 @@ static int sil24_issue_SRST(struct ata_p
 	u32 irq_enable, irq_stat;
 	int cnt;
 
+	DPRINTK("ENTER\n");
+
 	/* temporarily turn off IRQs during SRST */
 	irq_enable = readl(port + PORT_IRQ_ENABLE_SET);
 	writel(irq_enable, port + PORT_IRQ_ENABLE_CLR);
@@ -465,30 +468,27 @@ static int sil24_issue_SRST(struct ata_p
 	/* restore IRQs */
 	writel(irq_enable, port + PORT_IRQ_ENABLE_SET);
 
-	if (!(irq_stat & PORT_IRQ_COMPLETE))
-		return -1;
+	if (sata_dev_present(ap)) {
+		if (!(irq_stat & PORT_IRQ_COMPLETE)) {
+			DPRINTK("EXIT, srst failed\n");
+			return -EIO;
+		}
 
-	/* update TF */
-	sil24_update_tf(ap);
+		sil24_update_tf(ap);
+		*class = ata_dev_classify(&pp->tf);
+	}
+	if (*class == ATA_DEV_UNKNOWN)
+		*class = ATA_DEV_NONE;
+
+	DPRINTK("EXIT, class=%u\n", *class);
 	return 0;
 }
 
-static void sil24_phy_reset(struct ata_port *ap)
+static int sil24_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
-	struct sil24_port_priv *pp = ap->private_data;
-
-	__sata_phy_reset(ap);
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		return;
-
-	if (sil24_issue_SRST(ap) < 0) {
-		printk(KERN_ERR DRV_NAME
-		       " ata%u: SRST failed, disabling port\n", ap->id);
-		ap->ops->port_disable(ap);
-		return;
-	}
-
-	ap->device->class = ata_dev_classify(&pp->tf);
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     sil24_softreset, NULL,
+				     ata_std_postreset, classes);
 }
 
 static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
-- 
1.1.3

[PATCH 07/11] sata_sil24: add hardreset

[Tejun Heo]

Now that libata is smart enough to handle both soft and hard resets,
add hardreset method.  Note that sil24 hardreset doesn't supply
signature; still, the new reset mechanism can make good use of it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/sata_sil24.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

8ef9da7d26ee574c1d719609c86b6c86543ab496
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index a529628..c2df4b6 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -484,10 +484,19 @@ static int sil24_softreset(struct ata_po
 	return 0;
 }
 
+static int sil24_hardreset(struct ata_port *ap, int verbose,
+			   unsigned int *class)
+{
+	unsigned int dummy_class;
+
+	/* sil24 doesn't report device signature after hard reset */
+	return sata_std_hardreset(ap, verbose, &dummy_class);
+}
+
 static int sil24_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
-				     sil24_softreset, NULL,
+				     sil24_softreset, sil24_hardreset,
 				     ata_std_postreset, classes);
 }
 
-- 
1.1.3

Re: [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3

[Jeff Garzik]

Tejun Heo wrote:
> Jeff, after patches upto #04 are applied, the following differences
> remain between the new reset mechanism and the original one.
> 
> 1. For softreset
> 
> In the original code, ata_busy_sleep() is done after waking up the PHY
> whether SATA_RESET is used or not.  For SRST case, it looks like the
> following.
> 
> 	Original			New
> -----------------------------------------------------------------
> a1. Wake up PHY using SControl	b1. Wake up PHY using SControl
> 
> a2. ata_busy_sleep(),
>     if sata_dev_present()
> 
> a3. dev_select(ap, 0)		b2. dev_select(ap, 0)
> 
> a4. do softreset		b3. do softreset
> 
> a5. ata_busy_sleep() for	b4. ata_busy_sleep() for
>     present devices in		    present devices in
>     ata_bus_post_reset()	    ata_bus_post_reset()
> 
> 
> I thought the ata_busy_sleep() in #a2 was for SATA_RESET case and left
> it out.  Do you think it's necessary for SRST too?

Yes.  You want to check status before dev_select() to ensure that we are 
at bus-idle in the HSM.

Follow the register I/O operations in Hale Landis's ATADRDR 
(http://www.ata-atapi.com/atadrvr.htm).  For PATA probing, I consider 
his code darn near canonical.


> 2. For hardreset
> 
> Unlike in the original code, the new reset mechanism first wake up the
> PHY before invoking sata_std_hardreset() even when
> sata_std_hardreset() is the only reset method.  And dev_select(ap, 0)
> is not performed for hardreset.
> 
> So, the diffrence looks like the following.
> 
> 	Original			New
> -----------------------------------------------------------------
> 				b1. Wake up PHY using SControl
> 
> a1. Reset PHY using SControl	b2. Reset PHY using SControl
> 
> a2. Wake up PHY using SControl	b3. Wakeup PHY using SControl
> 
> a3. ata_busy_sleep(),		b4. ata_busy_sleep(),
>     if sata_dev_present()	    if sata_dev_present()
> 
> a4. dev_select(ap, 0)
> 
> 
> #b1 is there as probeinit operation and as most controllers implement
> softreset, there will be softreset operations between #b1 and #b2 in
> most cases.  The dev_select(ap, 0) in #a4 is actually for SRST and
> EDD.  For SATA_RESET, double dev_select()'s are soon done later in
> ata_bus_reset() with only ata_irq_on() inbetween.

I agree RE double select.

However, I don't want to mess with the reset/wake phy sequence at all. 
It works now, and changing it is really unnecessary churn.


> 3. During postreset
> 
> The original code does ata_irq_on() if ctl_addr is non-NULL, double
> select and ctl setup.  The new code does double select and
> ata_irq_on() if ctl_addr is non-NULL.
> 
> 	Original			New
> -----------------------------------------------------------------
> a1. ata_irq_on() if ctl_addr
> 
> a2. double select		b1. double select
> 
> a3. ctl setup
> 				b2. ata_irq_on() if ctl_addr
> 
> 
> ata_irq_on() actually implies ctl setup.  Also, in the original code,
> 'if ctl_addr' test in #a1 is bogus because in #a3, ctl_addr is used
> unchecked.  New code just does ata_irq_on() at the end.

This (though probably not double select) was largely based on ATADRVR, 
so I would rather not change the ata_irq_on() order without a good reason.


> IMHO, the remaining differences seem harmless and mainly due to the
> fact that new code splits soft and hard resets explicitly whereas the
> original code shared a lot of code path.  If any of above changes
> seems suspicious, please let me know.

Life is FAR easier in the long run if you don't change the hardware 
register read/write order at all, when switching libata to your new 
reset mechanism.  You are welcome to experiment with that -- but in 
separate patches, at the end of the patch series.  The ATA host state 
machine, with added SATA complexities, is way too fragile to mess with 
without lots of testing.

Your software will be more robust if you don't change the register I/O 
order at all.  Doing so means you leverage all the existing field 
testing done with the original probe/reset code.

Thanks for your continued work and patience on this...  we're getting there.

	Jeff

Re: [PATCH 01/11] libata: fix ata_std_probe_reset() SATA detection

[Jeff Garzik]

Tejun Heo wrote:
> ap->cbl is initialized during postreset and thus unknown on entry to
> ata_std_probe_reset().  This patch makes ata_std_probe_reset() use
> ATA_FLAG_SATA flag instead of ap->cbl to detect SATA port.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-2

Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()

[Jeff Garzik]

Tejun Heo wrote:
> This patch adds probeinit component operation to
> ata_drive_probe_reset().  If present, this new operation is called
> before performing any reset.  The operations's roll is to prepare @ap
> for following probe-reset operations.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
[...]
>  extern int ata_drive_probe_reset(struct ata_port *ap,
> +			ata_probeinit_fn_t probeinit,
>  			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
>  			ata_postreset_fn_t postreset, unsigned int *classes);

Applied patches 3-4, although I dislike that ata_drive_probe_reset() is 
growing a ton of function pointer arguments.  Please consider a better 
approach when you have some free time.  Perhaps these need to be added 
to ata_port_operations?  Perhaps another ata_reset_operations struct? 
What do you think?

	Jeff

Re: [PATCH 05/11] sata_sil: convert to new reset mechanism

[Jeff Garzik]

Tejun Heo wrote:
> Convert sata_sil to use new reset mechanism.  sata_sil is fairly
> generic and can directly use std routine.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK 5-6, pending addressing of comments in email
<43EAE67D.6010304@pobox.com>, the reply to patch #0 of this series.

Will wait for resend to apply patches 5+ of this series.

Re: [PATCH 07/11] sata_sil24: add hardreset

[Jeff Garzik]

Tejun Heo wrote:
> Now that libata is smart enough to handle both soft and hard resets,
> add hardreset method.  Note that sil24 hardreset doesn't supply
> signature; still, the new reset mechanism can make good use of it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/sata_sil24.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> 8ef9da7d26ee574c1d719609c86b6c86543ab496
> diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> index a529628..c2df4b6 100644
> --- a/drivers/scsi/sata_sil24.c
> +++ b/drivers/scsi/sata_sil24.c
> @@ -484,10 +484,19 @@ static int sil24_softreset(struct ata_po
>  	return 0;
>  }
>  
> +static int sil24_hardreset(struct ata_port *ap, int verbose,
> +			   unsigned int *class)
> +{
> +	unsigned int dummy_class;
> +
> +	/* sil24 doesn't report device signature after hard reset */
> +	return sata_std_hardreset(ap, verbose, &dummy_class);
> +}

ACK for sil24, though as a comment, we should move away from the 
practice of assuming we'll get a device signature from hard reset.  It's 
IMO easier to presume that SRST will always follow a hard reset, and 
you'll get the signature that way.  But that's an additional step from 
the current libata reset, so that's a separate patch to explore that 
train of thought...

	Jeff

Re: [PATCH 08/11] ata_piix: convert pata to new reset mechanism

[Jeff Garzik]

Tejun Heo wrote:
> Convert ata_piix pata ->phy_reset to new reset mechanism.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 8-11

Re: [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3

[Tejun]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> 1. For softreset
>>
[--snip--]
>> I thought the ata_busy_sleep() in #a2 was for SATA_RESET case and left
>> it out.  Do you think it's necessary for SRST too?
> 
> Yes.  You want to check status before dev_select() to ensure that we are 
> at bus-idle in the HSM.

Yes, I'll add that back.

> Follow the register I/O operations in Hale Landis's ATADRDR 
> (http://www.ata-atapi.com/atadrvr.htm).  For PATA probing, I consider 
> his code darn near canonical.

Will do.

> 
>> 2. For hardreset
[--snip--]
>> #b1 is there as probeinit operation and as most controllers implement
>> softreset, there will be softreset operations between #b1 and #b2 in
>> most cases.  The dev_select(ap, 0) in #a4 is actually for SRST and
>> EDD.  For SATA_RESET, double dev_select()'s are soon done later in
>> ata_bus_reset() with only ata_irq_on() inbetween.
> 
> I agree RE double select.
> 
> However, I don't want to mess with the reset/wake phy sequence at all. 
> It works now, and changing it is really unnecessary churn.
> 

I'll add double select back.  But removing additional PHY wakup before 
hardreset would require changes to reset_drive function.  Currently 
->probe_init is called to prep ports for probing (in this case, waking 
up PHY) for both hard and soft reset cases and that's why there's PHY 
wake up before hardreset.  I'd really like to waking up PHY out of reset 
component operations.  I'll figure something out.

> 
>> 3. During postreset
>>
>> The original code does ata_irq_on() if ctl_addr is non-NULL, double
>> select and ctl setup.  The new code does double select and
>> ata_irq_on() if ctl_addr is non-NULL.
>>
>>     Original            New
>> -----------------------------------------------------------------
>> a1. ata_irq_on() if ctl_addr
>>
>> a2. double select        b1. double select
>>
>> a3. ctl setup
>>                 b2. ata_irq_on() if ctl_addr
>>
>>
>> ata_irq_on() actually implies ctl setup.  Also, in the original code,
>> 'if ctl_addr' test in #a1 is bogus because in #a3, ctl_addr is used
>> unchecked.  New code just does ata_irq_on() at the end.
> 
> 
> This (though probably not double select) was largely based on ATADRVR, 
> so I would rather not change the ata_irq_on() order without a good reason.
> 
> 
>> IMHO, the remaining differences seem harmless and mainly due to the
>> fact that new code splits soft and hard resets explicitly whereas the
>> original code shared a lot of code path.  If any of above changes
>> seems suspicious, please let me know.
> 
> 
> Life is FAR easier in the long run if you don't change the hardware 
> register read/write order at all, when switching libata to your new 
> reset mechanism.  You are welcome to experiment with that -- but in 
> separate patches, at the end of the patch series.  The ATA host state 
> machine, with added SATA complexities, is way too fragile to mess with 
> without lots of testing.
 >
> Your software will be more robust if you don't change the register I/O 
> order at all.  Doing so means you leverage all the existing field 
> testing done with the original probe/reset code.

I see your point.  I'll submit another patch to make probing sequence 
register-wise identical to before.

> Thanks for your continued work and patience on this...  we're getting 
> there.

Thanks for reviewing.

-- 
tejun

Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()

[Tejun]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch adds probeinit component operation to
>> ata_drive_probe_reset().  If present, this new operation is called
>> before performing any reset.  The operations's roll is to prepare @ap
>> for following probe-reset operations.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> [...]
> 
>>  extern int ata_drive_probe_reset(struct ata_port *ap,
>> +            ata_probeinit_fn_t probeinit,
>>              ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
>>              ata_postreset_fn_t postreset, unsigned int *classes);
> 
> 
> Applied patches 3-4, although I dislike that ata_drive_probe_reset() is 
> growing a ton of function pointer arguments.  Please consider a better 
> approach when you have some free time.  Perhaps these need to be added 
> to ata_port_operations?  Perhaps another ata_reset_operations struct? 
> What do you think?
> 

I thought about adding the component operations to ata_port_operations, 
but those callbacks would only be used if ->probe_reset uses 
ata_drive_probe_reset() and layering ends up weird.  BTW, the same thing 
is true for ->bmdma_* callbacks and a few more, I think.

So, I'm a little bit unconformatble with clamming multi levels of 
operations into ata_port_operations or adding ata_reset_operations to 
ata_port.  So, the wrap-with-drive-function thing was my compromise, 
which isn't very pretty but keeps the functionality.

A problem with clamming multi-level callbacks into one structure is that 
it's not clear which callbacks should be implemented and with core code 
constantly changing, the requirements also changes along.  Changing API 
is actually good thing but in this case it's difficult to know what end 
effects changes have on low-level drivers.  (remember ->dev_select 
breakage last year?)

So, IMHO we should not add more layered operations to top-level.  It 
would be nice if we can come up with some simple way to separate out 
layered callbacks.  Do you agree with this line of thought?

-- 
tejun

Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()

[Jeff Garzik]

Tejun wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>> This patch adds probeinit component operation to
>>> ata_drive_probe_reset().  If present, this new operation is called
>>> before performing any reset.  The operations's roll is to prepare @ap
>>> for following probe-reset operations.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>>
>> [...]
>>
>>>  extern int ata_drive_probe_reset(struct ata_port *ap,
>>> +            ata_probeinit_fn_t probeinit,
>>>              ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
>>>              ata_postreset_fn_t postreset, unsigned int *classes);
>>
>>
>>
>> Applied patches 3-4, although I dislike that ata_drive_probe_reset() 
>> is growing a ton of function pointer arguments.  Please consider a 
>> better approach when you have some free time.  Perhaps these need to 
>> be added to ata_port_operations?  Perhaps another ata_reset_operations 
>> struct? What do you think?
>>
> 
> I thought about adding the component operations to ata_port_operations, 
> but those callbacks would only be used if ->probe_reset uses 
> ata_drive_probe_reset() and layering ends up weird.  BTW, the same thing 
> is true for ->bmdma_* callbacks and a few more, I think.
> 
> So, I'm a little bit unconformatble with clamming multi levels of 
> operations into ata_port_operations or adding ata_reset_operations to 
> ata_port.  So, the wrap-with-drive-function thing was my compromise, 
> which isn't very pretty but keeps the functionality.

Fair enough.


> A problem with clamming multi-level callbacks into one structure is that 
> it's not clear which callbacks should be implemented and with core code 
> constantly changing, the requirements also changes along.  Changing API 
> is actually good thing but in this case it's difficult to know what end 
> effects changes have on low-level drivers.  (remember ->dev_select 
> breakage last year?)
> 
> So, IMHO we should not add more layered operations to top-level.  It 
> would be nice if we can come up with some simple way to separate out 
> layered callbacks.  Do you agree with this line of thought?

Indeed, one thought I had was having a cleaner upper-level qc_issue API, 
and separating the BMDMA hooks out somehow into a separate layer, 
outside ata_port_operations.

	Jeff