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

PATCH In review Tejun Heo 2 participants 21 messages Feb 2, 2006 – Feb 9, 2006

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Tejun Heo

author

2006-02-02 09:20 UTC #0 lore raw

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

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

Tejun Heo

author

2006-02-02 09:20 UTC #1 lore raw

PATCH 1/11 libata: fix ata_std_probe_reset() SATA detection

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 <[email protected]>

---

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

f820ad6fcf9839d76cfbaa6553c3875b0d7c0657

drivers/scsi/libata-core.c +1 -1

index 32ac63c..1d106b4 100644
@@ -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,

Signature · 1 line

1.1.3

Jeff Garzik

2006-02-09 06:54 UTC #2 lore raw

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

Quoted 5 lines · 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 <[email protected]>

applied 1-2

Tejun Heo

author

2006-02-02 09:20 UTC #3 lore raw

PATCH 2/11 libata: separate out sata_phy_resume() from sata_std_hardreset()

In reply to 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 <[email protected]>

---

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

11494a4da9ba499fd04996d85322ddc462fe47dc

drivers/scsi/libata-core.c +21 ~~-11~~

index 1d106b4..e302545 100644
@@ -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);

Signature · 1 line

1.1.3

Tejun Heo

author

2006-02-02 09:20 UTC #4 lore raw

PATCH 3/11 libata: add probeinit component operation to ata_drive_probe_reset()

In reply to 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 <[email protected]>

---

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

e854fb48e07ab9ae790280843fdd6d7f2bee9be3

drivers/scsi/libata-core.c +7 -2

index e302545..8bd197d 100644
@@ -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)

include/linux/libata.h +2 -0

index f4cd1eb..e8f29ce 100644
@@ -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,

Signature · 1 line

1.1.3

Jeff Garzik

2006-02-09 07:02 UTC #5 lore raw

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

Quoted 6 lines · 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 <[email protected]>

[...]

include/linux/libata.h extern void __sata_phy_reset(struct ata_ · line 490

Quoted 4 lines

extern int ata_drive_probe_reset(struct ata_port *ap,

include/linux/libata.h +1 -0

+			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

Tejun

author

2006-02-09 09:40 UTC #6 lore raw

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

Quoted 11 lines · Jeff Garzik

Quoted 6 lines · 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 <[email protected]>
[...]

Quoted 8 lines

Quoted 4 lines
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?

Signature · 1 line

tejun

Jeff Garzik

2006-02-09 09:42 UTC #7 lore raw

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

Quoted 21 lines · Tejun

Quoted 19 lines · Jeff Garzik
Quoted 6 lines · 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 <[email protected]>
[...]
Quoted 4 lines
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);

Quoted 6 lines

Quoted 2 lines
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.

Quoted 3 lines

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

Tejun Heo

author

2006-02-02 09:20 UTC #8 lore raw

PATCH 4/11 libata: implement ata_std_probeinit()

In reply to 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 <[email protected]>

---

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

9be7440acf08c8100e70ab3406a956c5d8086c0a

drivers/scsi/libata-core.c +15 -1

index 8bd197d..fac9677 100644
@@ -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);

include/linux/libata.h +1 -0

index e8f29ce..68b3fe6 100644
@@ -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,

Signature · 1 line

1.1.3

Tejun Heo

author

2006-02-02 09:20 UTC #9 lore raw

PATCH 5/11 sata_sil: convert to new reset mechanism

In reply to 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 <[email protected]>

---

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

8d307a344239cb9d6ab516e36e91c69154603f03

drivers/scsi/sata_sil.c +4 -5

index b017f85..6c8becc 100644
@@ -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 */

Signature · 1 line

1.1.3

Jeff Garzik

2006-02-09 07:05 UTC #10 lore raw

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

Quoted 4 lines · 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 <[email protected]>

ACK 5-6, pending addressing of comments in email <[email protected]>, the reply to patch #0 of this series.

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

Tejun Heo

author

2006-02-02 09:20 UTC #11 lore raw

PATCH 6/11 sata_sil24: convert to new reset mechanism

In reply to Tejun Heo

Convert sata_sil24 ->phy_reset to new reset mechanism.

Signed-off-by Tejun Heo <[email protected]>

---

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

f71741b6ef0240a346963e05ac6808fb9478a45e

drivers/scsi/sata_sil24.c +28 ~~-28~~

index 1160fda..a529628 100644
@@ -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,

Signature · 1 line

1.1.3

Tejun Heo

author

2006-02-02 09:20 UTC #12 lore raw

PATCH 7/11 sata_sil24: add hardreset

In reply to 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 <[email protected]>

---

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

8ef9da7d26ee574c1d719609c86b6c86543ab496

drivers/scsi/sata_sil24.c +10 -1

index a529628..c2df4b6 100644
@@ -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);
 }

Signature · 1 line

1.1.3

Jeff Garzik

2006-02-09 07:08 UTC #13 lore raw

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

Quoted 18 lines · 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 <[email protected]>
---
 drivers/scsi/sata_sil24.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)
8ef9da7d26ee574c1d719609c86b6c86543ab496
drivers/scsi/sata_sil24.c +0 -0
index a529628..c2df4b6 100644
@@ -484,10 +484,19 @@ static int sil24_softreset(struct ata_po
 	return 0;

drivers/scsi/sata_sil24.c static int sil24_softreset(struct ata_po · line 494

Quoted 10 lines

drivers/scsi/sata_sil24.c +8 -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

Tejun Heo

author

2006-02-02 09:20 UTC #14 lore raw

PATCH 8/11 ata_piix: convert pata to new reset mechanism

In reply to Tejun Heo

Convert ata_piix pata ->phy_reset to new reset mechanism.

Signed-off-by Tejun Heo <[email protected]>

---

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

4c83a0664401bd8a04813e810eb1a828f0aeafb2

drivers/scsi/ata_piix.c +31 ~~-16~~

index 49cc420..f4215c9 100644
@@ -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);
 }
 
 /**

Signature · 1 line

1.1.3

Jeff Garzik

2006-02-09 07:10 UTC #15 lore raw

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

Quoted 3 lines · Tejun Heo

Convert ata_piix pata ->phy_reset to new reset mechanism.
Signed-off-by Tejun Heo <[email protected]>

ACK patches 8-11

Tejun Heo

author

2006-02-02 09:20 UTC #16 lore raw

PATCH 9/11 ata_piix: convert sata to new reset mechanism

In reply to Tejun Heo

Convert ata_piix sata ->phy_reset to new reset mechanism.

Signed-off-by Tejun Heo <[email protected]>

---

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

bd10b2138fbcf1c8c7741f92f3f10bf65b6899c3

drivers/scsi/ata_piix.c +19 ~~-20~~

index f4215c9..c2561f7 100644
@@ -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);
 }
 
 /**

Signature · 1 line

1.1.3

Tejun Heo

author

2006-02-02 09:20 UTC #17 lore raw

PATCH 10/11 ahci: convert to new reset mechanism

In reply to Tejun Heo

Convert ahci ->phy_reset to new reset mechanism.

Signed-off-by Tejun Heo <[email protected]>

---

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

bc104d8fbf1cc23220be6ae5e7a02881fe4fbfcd

drivers/scsi/ahci.c +30 ~~-17~~

index 1f81333..46dc3ae 100644
@@ -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;

Signature · 1 line

1.1.3

Tejun Heo

author

2006-02-02 09:20 UTC #18 lore raw

PATCH 11/11 ahci: add softreset

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

---

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

ab703283fe99d9edebf40b84f7cdc53d398eb520

drivers/scsi/ahci.c +133 -1

index 46dc3ae..76f89f9 100644
@@ -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);
 }

Signature · 1 line

1.1.3

Jeff Garzik

2006-02-09 06:51 UTC #19 lore raw

In reply to Tejun Heo

Quoted 20 lines · Tejun Heo

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

Quoted 7 lines

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.

Quoted 20 lines

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

Quoted 8 lines

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.

Quoted 12 lines

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

Quoted 7 lines

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.

Quoted 4 lines

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

Tejun

author

2006-02-09 09:20 UTC #20 lore raw

Quoted 4 lines · Jeff Garzik

Quoted 2 lines · Tejun Heo
1. For softreset

[--snip--]

Quoted 4 lines

Quoted 2 lines
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.

Quoted 1 line

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.

Quoted 2 lines

Quoted 2 lines
2. For hardreset

[--snip--]

Quoted 10 lines

Quoted 5 lines
#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.

Quoted 31 lines

Quoted 13 lines
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
Quoted 7 lines
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.
Quoted 4 lines
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.

Quoted 3 lines

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.

Quoted 1 line

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

Thanks for reviewing.

Signature · 1 line

tejun