* [PATCH 05/11] sata_sil: convert to new reset mechanism
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-09 7:05 ` Jeff Garzik
2006-02-02 9:20 ` [PATCH 07/11] sata_sil24: add hardreset Tejun Heo
` (10 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 07/11] sata_sil24: add hardreset
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
2006-02-02 9:20 ` [PATCH 05/11] sata_sil: convert to new reset mechanism Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-09 7:08 ` Jeff Garzik
2006-02-02 9:20 ` [PATCH 06/11] sata_sil24: convert to new reset mechanism Tejun Heo
` (9 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 07/11] sata_sil24: add hardreset
2006-02-02 9:20 ` [PATCH 07/11] sata_sil24: add hardreset Tejun Heo
@ 2006-02-09 7:08 ` Jeff Garzik
0 siblings, 0 replies; 21+ messages in thread
From: Jeff Garzik @ 2006-02-09 7:08 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, linux-ide
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 06/11] sata_sil24: convert to new reset mechanism
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
2006-02-02 9:20 ` [PATCH 05/11] sata_sil: convert to new reset mechanism Tejun Heo
2006-02-02 9:20 ` [PATCH 07/11] sata_sil24: add hardreset Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-02 9:20 ` [PATCH 11/11] ahci: add softreset Tejun Heo
` (8 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 11/11] ahci: add softreset
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (2 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 06/11] sata_sil24: convert to new reset mechanism Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-02 9:20 ` [PATCH 02/11] libata: separate out sata_phy_resume() from sata_std_hardreset() Tejun Heo
` (7 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 02/11] libata: separate out sata_phy_resume() from sata_std_hardreset()
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (3 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 11/11] ahci: add softreset Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-02 9:20 ` [PATCH 08/11] ata_piix: convert pata to new reset mechanism Tejun Heo
` (6 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 08/11] ata_piix: convert pata to new reset mechanism
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (4 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 02/11] libata: separate out sata_phy_resume() from sata_std_hardreset() Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-09 7:10 ` Jeff Garzik
2006-02-02 9:20 ` [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset() Tejun Heo
` (5 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (5 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 08/11] ata_piix: convert pata to new reset mechanism Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-09 7:02 ` Jeff Garzik
2006-02-02 9:20 ` [PATCH 04/11] libata: implement ata_std_probeinit() Tejun Heo
` (4 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()
2006-02-02 9:20 ` [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset() Tejun Heo
@ 2006-02-09 7:02 ` Jeff Garzik
2006-02-09 9:39 ` Tejun
0 siblings, 1 reply; 21+ messages in thread
From: Jeff Garzik @ 2006-02-09 7:02 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, linux-ide
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()
2006-02-09 7:02 ` Jeff Garzik
@ 2006-02-09 9:39 ` Tejun
2006-02-09 9:42 ` Jeff Garzik
0 siblings, 1 reply; 21+ messages in thread
From: Tejun @ 2006-02-09 9:39 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, linux-ide
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()
2006-02-09 9:39 ` Tejun
@ 2006-02-09 9:42 ` Jeff Garzik
0 siblings, 0 replies; 21+ messages in thread
From: Jeff Garzik @ 2006-02-09 9:42 UTC (permalink / raw)
To: Tejun; +Cc: albertcc, linux-ide
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 04/11] libata: implement ata_std_probeinit()
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (6 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset() Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-02 9:20 ` [PATCH 09/11] ata_piix: convert sata to new reset mechanism Tejun Heo
` (3 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 09/11] ata_piix: convert sata to new reset mechanism
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (7 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 04/11] libata: implement ata_std_probeinit() Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-02 9:20 ` [PATCH 01/11] libata: fix ata_std_probe_reset() SATA detection Tejun Heo
` (2 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 01/11] libata: fix ata_std_probe_reset() SATA detection
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (8 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 09/11] ata_piix: convert sata to new reset mechanism Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-09 6:54 ` Jeff Garzik
2006-02-02 9:20 ` [PATCH 10/11] ahci: convert to new reset mechanism Tejun Heo
2006-02-09 6:51 ` [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Jeff Garzik
11 siblings, 1 reply; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 10/11] ahci: convert to new reset mechanism
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (9 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 01/11] libata: fix ata_std_probe_reset() SATA detection Tejun Heo
@ 2006-02-02 9:20 ` Tejun Heo
2006-02-09 6:51 ` [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Jeff Garzik
11 siblings, 0 replies; 21+ messages in thread
From: Tejun Heo @ 2006-02-02 9:20 UTC (permalink / raw)
To: jgarzik, albertcc, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3
2006-02-02 9:20 [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Tejun Heo
` (10 preceding siblings ...)
2006-02-02 9:20 ` [PATCH 10/11] ahci: convert to new reset mechanism Tejun Heo
@ 2006-02-09 6:51 ` Jeff Garzik
2006-02-09 9:20 ` Tejun
11 siblings, 1 reply; 21+ messages in thread
From: Jeff Garzik @ 2006-02-09 6:51 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, linux-ide, Alan Cox
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3
2006-02-09 6:51 ` [PATCHSET] libata: [PATCHSET] libata: new reset mechanism, take#3 Jeff Garzik
@ 2006-02-09 9:20 ` Tejun
0 siblings, 0 replies; 21+ messages in thread
From: Tejun @ 2006-02-09 9:20 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, linux-ide, Alan Cox
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
^ permalink raw reply [flat|nested] 21+ messages in thread