[PATCH 1/3] libata: implement dev->acpi_init_gtm

[PATCH 1/3] libata: implement dev->acpi_init_gtm

[Tejun Heo]

Add dev->acpi_init_gtm and store initial GTM values on host
initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
is set.  This is to remember BIOS/firmware programmed initial timing
for later use before reset and mode configuration modify it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |    3 +++
 include/linux/libata.h    |    2 ++
 2 files changed, 5 insertions(+)

Index: work/drivers/ata/libata-acpi.c
===================================================================
--- work.orig/drivers/ata/libata-acpi.c
+++ work/drivers/ata/libata-acpi.c
@@ -94,6 +94,9 @@ static void ata_acpi_associate_ide_port(
 
 		dev->acpi_handle = acpi_get_child(ap->acpi_handle, i);
 	}
+
+	if (ata_acpi_gtm(ap, &ap->acpi_init_gtm) == 0)
+		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct kobject *kobj,
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -209,6 +209,7 @@ enum {
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 	ATA_PFLAG_GTM_VALID	= (1 << 19), /* acpi_gtm data valid */
+	ATA_PFLAG_INIT_GTM_VALID = (1 << 20), /* initial gtm data valid */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
@@ -633,6 +634,7 @@ struct ata_port {
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
 	struct ata_acpi_gtm	acpi_gtm;
+	struct ata_acpi_gtm	acpi_init_gtm;
 #endif
 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
 };

[PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Tejun Heo]

The current ata_acpi_cbl_80wire() is too retricted in its
functionality.  It always reads GTM itself and only returns whether
the cable is 80 wire or not.  Extend it such that...

* It accepts @gtm argument instead of reading iteslf.

* Returns one of ATA_CBL_PATA40, ATA_CBL_PATA80 or ATA_CBL_PATA_UNK.

The function is renamed to ata_acpi_cbl().  Currently there are two
users - cable detection functions of pata_via and pata_amd.  Both are
converted to use the new function with ap->acpi_init_gtm.

Using the initial GTM value is the right thing to do because both are
trying to check firmware setting and by the time ->cable_detect runs
the controller is already forced into PIO0 by reset.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |   68 +++++++++++++++++++++++++---------------------
 drivers/ata/pata_amd.c    |    3 +-
 drivers/ata/pata_via.c    |    3 +-
 include/linux/libata.h    |    2 -
 4 files changed, 43 insertions(+), 33 deletions(-)

Index: work/drivers/ata/libata-acpi.c
===================================================================
--- work.orig/drivers/ata/libata-acpi.c
+++ work/drivers/ata/libata-acpi.c
@@ -390,42 +390,50 @@ static int ata_dev_get_GTF(struct ata_de
 }
 
 /**
- * ata_acpi_cbl_80wire		-	Check for 80 wire cable
- * @ap: Port to check
+ * ata_acpi_cbl - determine cable type from GTM values
+ * @ap: target ATA port
+ * @gtm: GTM to use
+ *
+ * Determine cable type from UDMA configuration in @gtm.  If UDMA is
+ * disabled, it returns ATA_CBL_PATA_UNK.  If UDMA is enabled and
+ * faster than 33, PATA_80; otherwise, PATA_40.
+ *
+ * LOCKING:
+ * None.
  *
- * Return 1 if the ACPI mode data for this port indicates the BIOS selected
- * an 80wire mode.
+ * RETURNS:
+ * Determined cable type (ATA_CBL_*).
  */
-
-int ata_acpi_cbl_80wire(struct ata_port *ap)
+int ata_acpi_cbl(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
 {
-	struct ata_acpi_gtm gtm;
-	int valid = 0;
+	int cbl = ATA_CBL_PATA_UNK;
 
-	/* No _GTM data, no information */
-	if (ata_acpi_gtm(ap, &gtm) < 0)
-		return 0;
+	if (ata_dev_enabled(&ap->link.device[0]) && (gtm->flags & 0x1)) {
+		if (gtm->drive[0].dma < 55)
+			return ATA_CBL_PATA80;
+		else
+			cbl = ATA_CBL_PATA40;
+	}
 
-	/* Split timing, DMA enabled */
-	if ((gtm.flags & 0x11) == 0x11 && gtm.drive[0].dma < 55)
-		valid |= 1;
-	if ((gtm.flags & 0x14) == 0x14 && gtm.drive[1].dma < 55)
-		valid |= 2;
-	/* Shared timing, DMA enabled */
-	if ((gtm.flags & 0x11) == 0x01 && gtm.drive[0].dma < 55)
-		valid |= 1;
-	if ((gtm.flags & 0x14) == 0x04 && gtm.drive[0].dma < 55)
-		valid |= 2;
-
-	/* Drive check */
-	if ((valid & 1) && ata_dev_enabled(&ap->link.device[0]))
-		return 1;
-	if ((valid & 2) && ata_dev_enabled(&ap->link.device[1]))
-		return 1;
-	return 0;
-}
+	if (ata_dev_enabled(&ap->link.device[1]) && (gtm->flags & 0x4)) {
+		u32 acpi_dma;
 
-EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
+		/* if split timing, use drive[0]; otherwise, drive[1] */
+		if (gtm->flags & 0x10)
+			acpi_dma = gtm->drive[1].dma;
+		else
+			acpi_dma = gtm->drive[0].dma;
+
+		if (acpi_dma < 55)
+			return ATA_CBL_PATA80;
+		else
+			cbl = ATA_CBL_PATA40;
+	}
+
+	return cbl;
+
+}
+EXPORT_SYMBOL_GPL(ata_acpi_cbl);
 
 /**
  * taskfile_load_raw - send taskfile registers to host controller
Index: work/drivers/ata/pata_amd.c
===================================================================
--- work.orig/drivers/ata/pata_amd.c
+++ work/drivers/ata/pata_amd.c
@@ -271,7 +271,8 @@ static int nv_cable_detect(struct ata_po
  	if ((udma & 0xC4) == 0xC4 || (udma & 0xC400) == 0xC400)
 		cbl = ATA_CBL_PATA80;
 	/* And a triple check across suspend/resume with ACPI around */
-	if (ata_acpi_cbl_80wire(ap))
+	if ((ap->pflags & ATA_PFLAG_INIT_GTM_VALID) &&
+	    ata_acpi_cbl(ap, &ap->acpi_init_gtm) == ATA_CBL_PATA80)
 		cbl = ATA_CBL_PATA80;
 	return cbl;
 }
Index: work/drivers/ata/pata_via.c
===================================================================
--- work.orig/drivers/ata/pata_via.c
+++ work/drivers/ata/pata_via.c
@@ -185,7 +185,8 @@ static int via_cable_detect(struct ata_p
 	if (ata66 & (0x10100000 >> (16 * ap->port_no)))
 		return ATA_CBL_PATA80;
 	/* Check with ACPI so we can spot BIOS reported SATA bridges */
-	if (ata_acpi_cbl_80wire(ap))
+	if ((ap->pflags & ATA_PFLAG_INIT_GTM_VALID) &&
+	    ata_acpi_cbl(ap, &ap->acpi_init_gtm) == ATA_CBL_PATA80)
 		return ATA_CBL_PATA80;
 	return ATA_CBL_PATA40;
 }
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -921,7 +921,7 @@ enum {
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
-extern int ata_acpi_cbl_80wire(struct ata_port *ap);
+int ata_acpi_cbl(struct ata_port *ap, const struct ata_acpi_gtm *gtm);
 int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 #else

[PATCH 3/3] pata_amd: fix and improve cable detection

[Tejun Heo]

Cable detection on Nvidia PATA hosts is pathetic.  The current
nv_cable_detect() assumes that the native cable detection only
mistakes 80c as 40c but this isn't true.  On ASUS A8N-E, cable
register almost always says 80c is attached and it also manages to
trick the drive that the cable is 80c.

Also, BIOS checking, and before the get_acpi_cbl() update ACPI
checking too, in the current implementation were useless because they
read the setting _after_ reset is complete which forces the host into
PIO0, so they never triggered.

Reimplement nv_cable_detect() such that...

* BIOS setting is cached during controller initialization and restored
  on driver detach.

* BIOS and ACPI cable results are considered more important.  Native
  cable bits are used iff both BIOS and ACPI results are PATA_UNK.  If
  BIOS or ACPI indicates 80c, it's 80c.  If none indicates 80c, it's
  40c.

This change makes cable detection work correctly all the time on ASUS
A8N-E and should also work well on other NV PATA configurations.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/pata_amd.c |  112 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 90 insertions(+), 22 deletions(-)

Index: work/drivers/ata/pata_amd.c
===================================================================
--- work.orig/drivers/ata/pata_amd.c
+++ work/drivers/ata/pata_amd.c
@@ -255,26 +255,69 @@ static int nv_cable_detect(struct ata_po
 {
 	static const u8 bitmask[2] = {0x03, 0x0C};
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	struct ata_device *dev;
+	char acpi_str[32] = "";
+	int native_cbl, bios_cbl, acpi_cbl, verdict;
+	u32 saved_udma, udma;
 	u8 ata66;
-	u16 udma;
-	int cbl;
 
+	/* native method, this is usually broken */
 	pci_read_config_byte(pdev, 0x52, &ata66);
 	if (ata66 & bitmask[ap->port_no])
-		cbl = ATA_CBL_PATA80;
+		native_cbl = ATA_CBL_PATA80;
 	else
-		cbl = ATA_CBL_PATA40;
+		native_cbl = ATA_CBL_PATA40;
 
- 	/* We now have to double check because the Nvidia boxes BIOS
- 	   doesn't always set the cable bits but does set mode bits */
- 	pci_read_config_word(pdev, 0x62 - 2 * ap->port_no, &udma);
- 	if ((udma & 0xC4) == 0xC4 || (udma & 0xC400) == 0xC400)
-		cbl = ATA_CBL_PATA80;
-	/* And a triple check across suspend/resume with ACPI around */
-	if ((ap->pflags & ATA_PFLAG_INIT_GTM_VALID) &&
-	    ata_acpi_cbl(ap, &ap->acpi_init_gtm) == ATA_CBL_PATA80)
-		cbl = ATA_CBL_PATA80;
-	return cbl;
+	/* peek BIOS configuration */
+	bios_cbl = ATA_CBL_PATA_UNK;
+
+	saved_udma = udma = (unsigned long)ap->host->private_data;
+	if (ap->port_no == 0)
+		udma >>= 16;
+
+	ata_link_for_each_dev(dev, &ap->link) {
+		u32 tmp = udma;
+
+		if (dev->devno == 0)
+			tmp >>= 8;
+
+		if (!ata_dev_enabled(dev) || (tmp & 0xC0) != 0xC0)
+			continue;
+
+		if (tmp & 0x4)
+			bios_cbl = ATA_CBL_PATA80;
+		else
+			bios_cbl = ATA_CBL_PATA40;
+	}
+
+	/* and ACPI configuration */
+	acpi_cbl = ATA_CBL_PATA_UNK;
+	if (ap->pflags & ATA_PFLAG_INIT_GTM_VALID)
+		acpi_cbl = ata_acpi_cbl(ap, &ap->acpi_init_gtm);
+
+	/* Three values collected.  Native value is usually
+	 * untrustworthy.  Consider it iff both BIOS and ACPI values
+	 * are unknown; otherwise, choose higher of the two.
+	 */
+	if (bios_cbl == ATA_CBL_PATA_UNK && acpi_cbl == ATA_CBL_PATA_UNK)
+		verdict = native_cbl;
+	else if (bios_cbl == ATA_CBL_PATA80 || acpi_cbl == ATA_CBL_PATA80)
+		verdict = ATA_CBL_PATA80;
+	else
+		verdict = ATA_CBL_PATA40;
+
+	if (ap->pflags & ATA_PFLAG_INIT_GTM_VALID)
+		snprintf(acpi_str, sizeof(acpi_str), " (%u:%u:0x%x)",
+			 ap->acpi_init_gtm.drive[0].dma,
+			 ap->acpi_init_gtm.drive[1].dma,
+			 ap->acpi_init_gtm.flags);
+
+	ata_port_printk(ap, KERN_DEBUG, "nv_cable_detect: native=%d (0x%x) "
+			"BIOS=%d (0x%x) ACPI=%d%s verdict=%d\n",
+			native_cbl, ata66, bios_cbl, saved_udma, acpi_cbl,
+			acpi_str, verdict);
+
+	return verdict;
 }
 
 /**
@@ -314,6 +357,14 @@ static void nv133_set_dmamode(struct ata
 	timing_setup(ap, adev, 0x50, adev->dma_mode, 4);
 }
 
+static void nv_host_stop(struct ata_host *host)
+{
+	u32 udma = (unsigned long)host->private_data;
+
+	/* restore PCI config register 0x60 */
+	pci_write_config_dword(to_pci_dev(host->dev), 0x60, udma);
+}
+
 static struct scsi_host_template amd_sht = {
 	.module			= THIS_MODULE,
 	.name			= DRV_NAME,
@@ -495,6 +546,7 @@ static struct ata_port_operations nv100_
 	.irq_on		= ata_irq_on,
 
 	.port_start	= ata_sff_port_start,
+	.host_stop	= nv_host_stop,
 };
 
 static struct ata_port_operations nv133_port_ops = {
@@ -528,6 +580,7 @@ static struct ata_port_operations nv133_
 	.irq_on		= ata_irq_on,
 
 	.port_start	= ata_sff_port_start,
+	.host_stop	= nv_host_stop,
 };
 
 static int amd_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -614,7 +667,8 @@ static int amd_init_one(struct pci_dev *
 			.port_ops = &amd100_port_ops
 		}
 	};
-	const struct ata_port_info *ppi[] = { NULL, NULL };
+	struct ata_port_info pi;
+	const struct ata_port_info *ppi[] = { &pi, NULL };
 	static int printed_version;
 	int type = id->driver_data;
 	u8 fifo;
@@ -628,6 +682,19 @@ static int amd_init_one(struct pci_dev *
 	if (type == 1 && pdev->revision > 0x7)
 		type = 2;
 
+	/* Serenade ? */
+	if (type == 5 && pdev->subsystem_vendor == PCI_VENDOR_ID_AMD &&
+			 pdev->subsystem_device == PCI_DEVICE_ID_AMD_SERENADE)
+		type = 6;	/* UDMA 100 only */
+
+	/*
+	 * Okay, type is determined now.  Apply type-specific workarounds.
+	 */
+	pi = info[type];
+
+	if (type < 3)
+		ata_pci_clear_simplex(pdev);
+
 	/* Check for AMD7411 */
 	if (type == 3)
 		/* FIFO is broken */
@@ -635,16 +702,17 @@ static int amd_init_one(struct pci_dev *
 	else
 		pci_write_config_byte(pdev, 0x41, fifo | 0xF0);
 
-	/* Serenade ? */
-	if (type == 5 && pdev->subsystem_vendor == PCI_VENDOR_ID_AMD &&
-			 pdev->subsystem_device == PCI_DEVICE_ID_AMD_SERENADE)
-		type = 6;	/* UDMA 100 only */
+	/* Cable detection on Nvidia chips doesn't work too well,
+	 * cache BIOS programmed UDMA mode.
+	 */
+	if (type == 7 || type == 8) {
+		u32 udma;
 
-	if (type < 3)
-		ata_pci_clear_simplex(pdev);
+		pci_read_config_dword(pdev, 0x60, &udma);
+		pi.private_data = (void *)(unsigned long)udma;
+	}
 
 	/* And fire it up */
-	ppi[0] = &info[type];
 	return ata_pci_init_one(pdev, ppi);
 }
 

Re: [PATCH 1/3] libata: implement dev->acpi_init_gtm

[Jeff Garzik]

Tejun Heo wrote:
> Add dev->acpi_init_gtm and store initial GTM values on host
> initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
> is set.  This is to remember BIOS/firmware programmed initial timing
> for later use before reset and mode configuration modify it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

It sounds like pata_via and pata_amd need a foo_save_initial_config() 
much like AHCI, during which they would fill ppriv->init_gtm rather than 
ap->init_gtm.  Thoughts?  Both drivers are calling ata_acpi_cbl() 
themselves, permitting the possibility of ppriv->init_gtm.

This avoids forcing everyone else to bear the memory cost in ata_port 
for just these two drivers.

	Jeff

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Alan Cox]

> Using the initial GTM value is the right thing to do because both are
> trying to check firmware setting and by the time ->cable_detect runs
> the controller is already forced into PIO0 by reset.

As we only look at the DMA bits that doesn't matter but I agree it would
be cleaner if we were more careful. Also our mode setting outside
pata_acpi doesn't touch the ACPI settings so its ok for that reason too.

> + * Determine cable type from UDMA configuration in @gtm.  If UDMA is
> + * disabled, it returns ATA_CBL_PATA_UNK.  If UDMA is enabled and
> + * faster than 33, PATA_80; otherwise, PATA_40.

NAK

Consider the case of a UDMA 33 capable device on an 80 wire cable. We
simply can't use the cable detect methods to prove it is 40 wire this way.

What we actually know is

	If UDMA > 33 selected		-> UDMA > 33 works
					80 wire
					Short
					or SATA

	If UDMA > 33 not selected	Unknown
					40 wire
					80 + Drive limit
					80 + Controller limit
					80 + BIOS is set to PIO
					etc

The only case I can see you could reasonably suggest 40 wire would be
IFF the drive is capable of UDMA > 33 _and_ the ACPI mode is UDMA 33.



Alan

Re: [PATCH 3/3] pata_amd: fix and improve cable detection

[Alan Cox]

On Sat, 03 Nov 2007 00:22:24 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Cable detection on Nvidia PATA hosts is pathetic.  The current
> nv_cable_detect() assumes that the native cable detection only
> mistakes 80c as 40c but this isn't true.  On ASUS A8N-E, cable
> register almost always says 80c is attached and it also manages to
> trick the drive that the cable is 80c.

According to the last Nvidia comments on Nvidia there is no cable detect
bit. I've just never gotten around to implementing it because due to lack
of documentation Nvidia PATA is very very low priority compared to
problems with documented chipsets from helpful vendors.

Alan

Re: [PATCH 1/3] libata: implement dev->acpi_init_gtm

[Alan Cox]

On Sat, 03 Nov 2007 00:20:10 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Add dev->acpi_init_gtm and store initial GTM values on host
> initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
> is set.  This is to remember BIOS/firmware programmed initial timing
> for later use before reset and mode configuration modify it.

Don't you need to clear this on a hotplug event ?

Alan

Re: [PATCH 1/3] libata: implement dev->acpi_init_gtm

[Tejun Heo]

Alan Cox wrote:
> On Sat, 03 Nov 2007 00:20:10 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> Add dev->acpi_init_gtm and store initial GTM values on host
>> initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
>> is set.  This is to remember BIOS/firmware programmed initial timing
>> for later use before reset and mode configuration modify it.
> 
> Don't you need to clear this on a hotplug event ?

It just says the GTM value loaded on driver initialization is valid.
The rest is upto the user.

-- 
tejun

Re: [PATCH 1/3] libata: implement dev->acpi_init_gtm

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Add dev->acpi_init_gtm and store initial GTM values on host
>> initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
>> is set.  This is to remember BIOS/firmware programmed initial timing
>> for later use before reset and mode configuration modify it.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> It sounds like pata_via and pata_amd need a foo_save_initial_config()
> much like AHCI, during which they would fill ppriv->init_gtm rather than
> ap->init_gtm.  Thoughts?  Both drivers are calling ata_acpi_cbl()
> themselves, permitting the possibility of ppriv->init_gtm.
> 
> This avoids forcing everyone else to bear the memory cost in ata_port
> for just these two drivers.

The memory overhead is pretty small and more importantly there isn't a
good place to load initial GTM.  ACPI is associated with the host during
host registration after all private host initialization is complete.
When the EH gets invoked, the first thing which is done is forcing PIO0
before initial reset.

Unless we add another hook, the only place to put this is in private
->error_handler().  A driver can check whether it's being called for the
first time and record it in private structure, which isn't too pretty,
so I thought doing it this way was fair tradeoff.

Thanks.

-- 
tejun

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Tejun Heo]

Alan Cox wrote:
>> Using the initial GTM value is the right thing to do because both are
>> trying to check firmware setting and by the time ->cable_detect runs
>> the controller is already forced into PIO0 by reset.
> 
> As we only look at the DMA bits that doesn't matter but I agree it would
> be cleaner if we were more careful. Also our mode setting outside
> pata_acpi doesn't touch the ACPI settings so its ok for that reason too.

GTM results do change whether you call STM or not.  GTM in many cases is
implemented as reading the current configuration value from the
controller so if you configure PIO0, it will return PIO0 timings.  And,
yeah, on ASUS A8N-E, if you configure PIO0, GTM answer says DMA is off.

>> + * Determine cable type from UDMA configuration in @gtm.  If UDMA is
>> + * disabled, it returns ATA_CBL_PATA_UNK.  If UDMA is enabled and
>> + * faster than 33, PATA_80; otherwise, PATA_40.
> 
> NAK
> 
> Consider the case of a UDMA 33 capable device on an 80 wire cable. We
> simply can't use the cable detect methods to prove it is 40 wire this way.
> 
> What we actually know is
> 
> 	If UDMA > 33 selected		-> UDMA > 33 works
> 					80 wire
> 					Short
> 					or SATA
> 
> 	If UDMA > 33 not selected	Unknown
> 					40 wire
> 					80 + Drive limit
> 					80 + Controller limit
> 					80 + BIOS is set to PIO
> 					etc
> 
> The only case I can see you could reasonably suggest 40 wire would be
> IFF the drive is capable of UDMA > 33 _and_ the ACPI mode is UDMA 33.

What the function answers is not actually cable type but "according to
the current configuration, using this cable type won't violate BIOS
configuration" kind of answer.  How the caller is to use that is the
caller's responsibility.

-- 
tejun

Re: [PATCH 3/3] pata_amd: fix and improve cable detection

[Tejun Heo]

Alan Cox wrote:
>> Cable detection on Nvidia PATA hosts is pathetic.  The current
>> nv_cable_detect() assumes that the native cable detection only
>> mistakes 80c as 40c but this isn't true.  On ASUS A8N-E, cable
>> register almost always says 80c is attached and it also manages to
>> trick the drive that the cable is 80c.
> 
> According to the last Nvidia comments on Nvidia there is no cable detect
> bit. I've just never gotten around to implementing it because due to lack
> of documentation Nvidia PATA is very very low priority compared to
> problems with documented chipsets from helpful vendors.

Yeah, it seems some boards just say 80C while others just say 40C, so
all we can use is how the BIOS configured it.  I suppose BIOS does it by
issuing trial commands which I don't think adding to libata is a good idea.

Can you please lemme know what you don't like about the current
implementation or what other approach you have in mind?  I don't like
Nvidia PATA either but there are a lot of people using it out there.

Thanks.

-- 
tejun

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Alan Cox]

> GTM results do change whether you call STM or not.  GTM in many cases is
> implemented as reading the current configuration value from the
> controller so if you configure PIO0, it will return PIO0 timings.  And,
> yeah, on ASUS A8N-E, if you configure PIO0, GTM answer says DMA is off.

Oh fun - ok then we do need to cache it.

> What the function answers is not actually cable type but "according to
> the current configuration, using this cable type won't violate BIOS
> configuration" kind of answer.  How the caller is to use that is the
> caller's responsibility.

How does the caller make use of it ? The only way I can think of to use
it is that if either drive says UDMA > 33 then we know its 80 wire.
Otherwise we can't really prove anything. And if neither drive says > 80
wire we have to go on what other info we have, which for NVidia appears
to be "guess 40".

I'm not sure therefore we gain anything ?

Re: [PATCH 3/3] pata_amd: fix and improve cable detection

[Alan Cox]

> all we can use is how the BIOS configured it.  I suppose BIOS does it by
> issuing trial commands which I don't think adding to libata is a good idea.

The BIOS does it by asking the hardware somehow. I traced one or two
BIOSes that far. The info is there but its not documented in the
slightest so only ACPI makes it visible via the BIOS.

> 
> Can you please lemme know what you don't like about the current
> implementation or what other approach you have in mind?  I don't like
> Nvidia PATA either but there are a lot of people using it out there.

We seem to be able to trust the drives and BIOS ACPI data for Nvidia (at
least what I have seen), so I guess we should simply declare the cable
type unknown, 80 wire if ACPI says it is and then do the drive detect
side ?

Alan

Re: [PATCH 3/3] pata_amd: fix and improve cable detection

[Tejun Heo]

Alan Cox wrote:
>> all we can use is how the BIOS configured it.  I suppose BIOS does it by
>> issuing trial commands which I don't think adding to libata is a good idea.
> 
> The BIOS does it by asking the hardware somehow. I traced one or two
> BIOSes that far. The info is there but its not documented in the
> slightest so only ACPI makes it visible via the BIOS.

OIC, gawdddddddd... What was so long with just following what those
AMD chips did?

>> Can you please lemme know what you don't like about the current
>> implementation or what other approach you have in mind?  I don't like
>> Nvidia PATA either but there are a lot of people using it out there.
> 
> We seem to be able to trust the drives and BIOS ACPI data for Nvidia (at
> least what I have seen), so I guess we should simply declare the cable
> type unknown, 80 wire if ACPI says it is and then do the drive detect
> side ?

There are machines with broken BIOSen so actually the most reliable
source is the UDMA timing register.  On ASUS A8N-E, ACPI and UDMA
timing register always concur.  I think it's best to look at both not
so much for accuracy but for debugging, so the following printk in the
patch.

+	if (ap->pflags & ATA_PFLAG_INIT_GTM_VALID)
+		snprintf(acpi_str, sizeof(acpi_str), " (%u:%u:0x%x)",
+			 ap->acpi_init_gtm.drive[0].dma,
+			 ap->acpi_init_gtm.drive[1].dma,
+			 ap->acpi_init_gtm.flags);
+
+	ata_port_printk(ap, KERN_DEBUG, "nv_cable_detect: native=%d (0x%x) "
+			"BIOS=%d (0x%x) ACPI=%d%s verdict=%d\n",
+			native_cbl, ata66, bios_cbl, saved_udma, acpi_cbl,
+			acpi_str, verdict);

When cable detection goes wrong, we'll at least know what all those
different sources are telling.

Thanks.

-- 
tejun

Re: [PATCH 3/3] pata_amd: fix and improve cable detection

[Tejun Heo]

Hello, again.  Forgot one thing.

Alan Cox wrote:
>> all we can use is how the BIOS configured it.  I suppose BIOS does it by
>> issuing trial commands which I don't think adding to libata is a good idea.
> 
> The BIOS does it by asking the hardware somehow. I traced one or two
> BIOSes that far. The info is there but its not documented in the
> slightest so only ACPI makes it visible via the BIOS.
> 
>> Can you please lemme know what you don't like about the current
>> implementation or what other approach you have in mind?  I don't like
>> Nvidia PATA either but there are a lot of people using it out there.
> 
> We seem to be able to trust the drives and BIOS ACPI data for Nvidia (at
> least what I have seen), so I guess we should simply declare the cable
> type unknown, 80 wire if ACPI says it is and then do the drive detect
> side ?

The drive side can't be trusted.  For the drive side detection work
properly, there should be a capacitor attached to PDIAG-:CBLID- line so
that the drive can tell 40C cable which connects CBLID- to the host
connector by sampling while the capacitor is still discharging.  If
there's no capacitor on the line, 40C and 80C cables behave identically.
 Voltage rises as soon as CBLID- is disasserted and drive always reports
80C and that's what happens on my A8N-E.  Hell of a way to save cost for
one capacitor, I guess.  :-(

-- 
tejun

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Tejun Heo]

Alan Cox wrote:
>> What the function answers is not actually cable type but "according to
>> the current configuration, using this cable type won't violate BIOS
>> configuration" kind of answer.  How the caller is to use that is the
>> caller's responsibility.
> 
> How does the caller make use of it ? The only way I can think of to use
> it is that if either drive says UDMA > 33 then we know its 80 wire.
> Otherwise we can't really prove anything. And if neither drive says > 80
> wire we have to go on what other info we have, which for NVidia appears
> to be "guess 40".
> 
> I'm not sure therefore we gain anything ?

I was trying to discern between the following three.

1. UDMA mode isn't configured at all.  Either BIOS forgot it, isn't
there or some other entity cleared it before the driver is loaded.  I
can't tell you anything.  If you have another source for cable
detection, use it instead.

2. UDMA mode is configured but equal to or under UDMA33.  Dunno whether
it's cable or device limit but anyways you better limit it to UDMA33 too.

3. UDMA mode is configured and above UDMA33.  Do whatever you wanna do.

Return values are ATA_CBL_PATA_UNK, ATA_CBL_PATA_UDMA33 and
ATA_CBL_PATA_UDMA66 respectively.

Thanks.

-- 
tejun

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Bartlomiej Zolnierkiewicz]

On Friday 02 November 2007, Alan Cox wrote:
> > Using the initial GTM value is the right thing to do because both are
> > trying to check firmware setting and by the time ->cable_detect runs
> > the controller is already forced into PIO0 by reset.
> 
> As we only look at the DMA bits that doesn't matter but I agree it would

It does matter,

http://lkml.org/lkml/2007/10/12/537

"
>       libata: correct handling of SRST reset sequences

This potentially adds a regression for nVidia boxes (but ACPI cable
detection should compensate for it):

pata_amd's ->cable_detect checks UDMA timings to get the cable type
(cable detection is done before the SRST) but pata_amd's ->set_piomode
messes with UDMA timings.  It seems that both methods need fixing.
"

[ Now we see that since ACPI can just dump the current settings
  ACPI cable detection won't help a tiny bit. ]

AFAICS pata_via.c has the same problem w.r.t. ->set_piomode

> be cleaner if we were more careful. Also our mode setting outside
> pata_acpi doesn't touch the ACPI settings so its ok for that reason too.

Thanks,
Bart

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Bartlomiej Zolnierkiewicz]

On Saturday 03 November 2007, Bartlomiej Zolnierkiewicz wrote:
> On Friday 02 November 2007, Alan Cox wrote:
> > > Using the initial GTM value is the right thing to do because both are
> > > trying to check firmware setting and by the time ->cable_detect runs
> > > the controller is already forced into PIO0 by reset.
> > 
> > As we only look at the DMA bits that doesn't matter but I agree it would
> 
> It does matter,
> 
> http://lkml.org/lkml/2007/10/12/537
> 
> "
> >       libata: correct handling of SRST reset sequences
> 
> This potentially adds a regression for nVidia boxes (but ACPI cable
> detection should compensate for it):
> 
> pata_amd's ->cable_detect checks UDMA timings to get the cable type
> (cable detection is done before the SRST) but pata_amd's ->set_piomode

s/after/before/ of course

> messes with UDMA timings.  It seems that both methods need fixing.
> "
> 
> [ Now we see that since ACPI can just dump the current settings
>   ACPI cable detection won't help a tiny bit. ]
> 
> AFAICS pata_via.c has the same problem w.r.t. ->set_piomode
> 
> > be cleaner if we were more careful. Also our mode setting outside
> > pata_acpi doesn't touch the ACPI settings so its ok for that reason too.
> 
> Thanks,
> Bart

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Alan Cox]

> 2. UDMA mode is configured but equal to or under UDMA33.  Dunno whether
> it's cable or device limit but anyways you better limit it to UDMA33 too.

If it is under UDMA33 you know it isn't a cable limit

If it is UDMA 33 and the device top is > UDMA 33 it might be

This matters as you've got to account for both master and slave, as well
as hotplug in theory.

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Tejun Heo]

Alan Cox wrote:
>> 2. UDMA mode is configured but equal to or under UDMA33.  Dunno whether
>> it's cable or device limit but anyways you better limit it to UDMA33 too.
> 
> If it is under UDMA33 you know it isn't a cable limit
> 
> If it is UDMA 33 and the device top is > UDMA 33 it might be
> 
> This matters as you've got to account for both master and slave, as well
> as hotplug in theory.

I'm not sure whether that level of sophistication is necessary here.
This is more like a transfer rate filter thing rather than actual cable
detection.  We're basically just trying to follow what BIOS did.  Maybe
we should return ATA_CBL_PATA_MAYBE_80 unconditionally and implement
ata_acpi_more_filter()?

-- 
tejun

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Alan Cox]

> detection.  We're basically just trying to follow what BIOS did.  Maybe
> we should return ATA_CBL_PATA_MAYBE_80 unconditionally and implement

MAYBE_80 = UNK.

> ata_acpi_more_filter()?

We can also use UNK intelligently in the eh code to go straight from
UDMA133/100/66 to 33 (if you don't already I've not read that logic
recently)

Re: [PATCH 2/3] libata: extend ata_acpi_cbl_80wire() and fix cable detection in pata_via and pata_amd

[Tejun Heo]

Alan Cox wrote:
>> detection.  We're basically just trying to follow what BIOS did.  Maybe
>> we should return ATA_CBL_PATA_MAYBE_80 unconditionally and implement
> 
> MAYBE_80 = UNK.

Yeah, right.  I somehow thought it was close to "don't really know but
treat as 40c" but it's more like "don't really know maybe 80c".

I think implementing this as mode filter is the better approach and
reflects the situation better.  We can't really tell the cable type.
All we know is how BIOS/firmware configured it and we're just trying to
use the same transfer mode.

Do you agree?  Then, I'll go ahead and redo the patch and implement this
mess as mode filter.

>> ata_acpi_more_filter()?
> 
> We can also use UNK intelligently in the eh code to go straight from
> UDMA133/100/66 to 33 (if you don't already I've not read that logic
> recently)

The default behavior of the current EH code should be enough.  It first
decelerate one step (e.g. UDMA/133->UDMA/100) and then right to UDMA/33.
 With the pending speed down update patch applied, it will take at most
three consecutive errors to reach UDMA/33 from any speed above that.

Thanks.

-- 
tejun

Re: [PATCH 1/3] libata: implement dev->acpi_init_gtm

[Tejun Heo]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Add dev->acpi_init_gtm and store initial GTM values on host
>>> initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
>>> is set.  This is to remember BIOS/firmware programmed initial timing
>>> for later use before reset and mode configuration modify it.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> It sounds like pata_via and pata_amd need a foo_save_initial_config()
>> much like AHCI, during which they would fill ppriv->init_gtm rather than
>> ap->init_gtm.  Thoughts?  Both drivers are calling ata_acpi_cbl()
>> themselves, permitting the possibility of ppriv->init_gtm.
>>
>> This avoids forcing everyone else to bear the memory cost in ata_port
>> for just these two drivers.
> 
> The memory overhead is pretty small and more importantly there isn't a
> good place to load initial GTM.  ACPI is associated with the host during
> host registration after all private host initialization is complete.
> When the EH gets invoked, the first thing which is done is forcing PIO0
> before initial reset.
> 
> Unless we add another hook, the only place to put this is in private
> ->error_handler().  A driver can check whether it's being called for the
> first time and record it in private structure, which isn't too pretty,
> so I thought doing it this way was fair tradeoff.

Jeff, do you agree or still think it's better done in LLDs?  I'm okay
either way.

Thanks.

-- 
tejun

Re: [PATCH 1/3] libata: implement dev->acpi_init_gtm

[Jeff Garzik]

Tejun Heo wrote:
> Tejun Heo wrote:
>> Jeff Garzik wrote:
>>> Tejun Heo wrote:
>>>> Add dev->acpi_init_gtm and store initial GTM values on host
>>>> initialization.  If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag
>>>> is set.  This is to remember BIOS/firmware programmed initial timing
>>>> for later use before reset and mode configuration modify it.
>>>>
>>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>> It sounds like pata_via and pata_amd need a foo_save_initial_config()
>>> much like AHCI, during which they would fill ppriv->init_gtm rather than
>>> ap->init_gtm.  Thoughts?  Both drivers are calling ata_acpi_cbl()
>>> themselves, permitting the possibility of ppriv->init_gtm.
>>>
>>> This avoids forcing everyone else to bear the memory cost in ata_port
>>> for just these two drivers.
>> The memory overhead is pretty small and more importantly there isn't a
>> good place to load initial GTM.  ACPI is associated with the host during
>> host registration after all private host initialization is complete.
>> When the EH gets invoked, the first thing which is done is forcing PIO0
>> before initial reset.
>>
>> Unless we add another hook, the only place to put this is in private
>> ->error_handler().  A driver can check whether it's being called for the
>> first time and record it in private structure, which isn't too pretty,
>> so I thought doing it this way was fair tradeoff.
> 
> Jeff, do you agree or still think it's better done in LLDs?  I'm okay
> either way.

I trust your judgement...  My reply was more of a "gut feeling" sort of 
thing.

In general, I want to keep libata as close as possible to the "LLD 
call-out model" as possible:  LLD registers itself with system services 
[perhaps using generic helpers to do so].  LLD fills in its own scsi 
host template and ata_port_operations [perhaps using generic helpers to 
do so].  LLD implements specific helpers that control LLD-specific 
behavior, calling out to libata helpers as need arises.

So I tend to prefer adding direct LLD control points to ATA_FLAG_xxx 
control points or other solutions, if the situation permits me to choose.

If you have to ask...  I will always prefer "it" (for some value of...) 
to be done in the LLD.  ;-)

	Jeff