[PATCHSET] sata_nv: convert to new EH and add hotplug support

[PATCHSET] sata_nv: convert to new EH and add hotplug support

[Tejun Heo]

Hello, all.

This patchset converts sata_nv to new EH and adds hotplug support.
I've tested it on ASUS A8N-E (nForce4, CK804, 10de:0054), and
everything works fine.  However, the controller doesn't have
protection against data transfer hang (IORDY hang) and the machine
completely locks up if a device is removed during active data
transfer.  I don't think it can be worked around without using the
better interface (ADMA).

This patchset contains 7 patches.

#01-03: prep sata_nv
#04   : implement irq manipulation methods
#05   : improve irq handler
#06   : implement new EH
#07   : add hotplug support

This patchset is against

     upstream (aeb2ecd6096182cc080d37679080c0f088dcd4a4)

Thanks.

--
tejun

[PATCH 1/7] sata_nv: kill not-working hotplug code

[Tejun Heo]

sata_nv contained hotplug code which is mainly for demonstrating how
hotplug event is handled.  This patch kills the demo code in
prepration for real hotplug implementation.

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

---

 drivers/scsi/sata_nv.c |  172 +-----------------------------------------------
 1 files changed, 2 insertions(+), 170 deletions(-)

8af8345ccc9e9a5aaad6f02a835e0e187b57de94
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index 9055124..716581e 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -64,12 +64,6 @@ enum {
 	NV_INT_STATUS_SDEV_PM		= 0x20,
 	NV_INT_STATUS_SDEV_ADDED	= 0x40,
 	NV_INT_STATUS_SDEV_REMOVED	= 0x80,
-	NV_INT_STATUS_PDEV_HOTPLUG	= (NV_INT_STATUS_PDEV_ADDED |
-					   NV_INT_STATUS_PDEV_REMOVED),
-	NV_INT_STATUS_SDEV_HOTPLUG	= (NV_INT_STATUS_SDEV_ADDED |
-					   NV_INT_STATUS_SDEV_REMOVED),
-	NV_INT_STATUS_HOTPLUG		= (NV_INT_STATUS_PDEV_HOTPLUG |
-					   NV_INT_STATUS_SDEV_HOTPLUG),
 
 	NV_INT_ENABLE			= 0x11,
 	NV_INT_ENABLE_CK804		= 0x441,
@@ -81,12 +75,6 @@ enum {
 	NV_INT_ENABLE_SDEV_PM		= 0x20,
 	NV_INT_ENABLE_SDEV_ADDED	= 0x40,
 	NV_INT_ENABLE_SDEV_REMOVED	= 0x80,
-	NV_INT_ENABLE_PDEV_HOTPLUG	= (NV_INT_ENABLE_PDEV_ADDED |
-					   NV_INT_ENABLE_PDEV_REMOVED),
-	NV_INT_ENABLE_SDEV_HOTPLUG	= (NV_INT_ENABLE_SDEV_ADDED |
-					   NV_INT_ENABLE_SDEV_REMOVED),
-	NV_INT_ENABLE_HOTPLUG		= (NV_INT_ENABLE_PDEV_HOTPLUG |
-					   NV_INT_ENABLE_SDEV_HOTPLUG),
 
 	NV_INT_CONFIG			= 0x12,
 	NV_INT_CONFIG_METHD		= 0x01, // 0 = INT, 1 = SMI
@@ -102,12 +90,6 @@ static irqreturn_t nv_interrupt (int irq
 static u32 nv_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void nv_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static void nv_host_stop (struct ata_host_set *host_set);
-static void nv_enable_hotplug(struct ata_probe_ent *probe_ent);
-static void nv_disable_hotplug(struct ata_host_set *host_set);
-static int nv_check_hotplug(struct ata_host_set *host_set);
-static void nv_enable_hotplug_ck804(struct ata_probe_ent *probe_ent);
-static void nv_disable_hotplug_ck804(struct ata_host_set *host_set);
-static int nv_check_hotplug_ck804(struct ata_host_set *host_set);
 
 enum nv_host_type
 {
@@ -158,34 +140,19 @@ static const struct pci_device_id nv_pci
 struct nv_host_desc
 {
 	enum nv_host_type	host_type;
-	void			(*enable_hotplug)(struct ata_probe_ent *probe_ent);
-	void			(*disable_hotplug)(struct ata_host_set *host_set);
-	int			(*check_hotplug)(struct ata_host_set *host_set);
-
 };
 static struct nv_host_desc nv_device_tbl[] = {
 	{
 		.host_type	= GENERIC,
-		.enable_hotplug	= NULL,
-		.disable_hotplug= NULL,
-		.check_hotplug	= NULL,
 	},
 	{
 		.host_type	= NFORCE2,
-		.enable_hotplug	= nv_enable_hotplug,
-		.disable_hotplug= nv_disable_hotplug,
-		.check_hotplug	= nv_check_hotplug,
 	},
 	{
 		.host_type	= NFORCE3,
-		.enable_hotplug	= nv_enable_hotplug,
-		.disable_hotplug= nv_disable_hotplug,
-		.check_hotplug	= nv_check_hotplug,
 	},
-	{	.host_type	= CK804,
-		.enable_hotplug	= nv_enable_hotplug_ck804,
-		.disable_hotplug= nv_disable_hotplug_ck804,
-		.check_hotplug	= nv_check_hotplug_ck804,
+	{
+		.host_type	= CK804,
 	},
 };
 
@@ -275,7 +242,6 @@ static irqreturn_t nv_interrupt (int irq
 				 struct pt_regs *regs)
 {
 	struct ata_host_set *host_set = dev_instance;
-	struct nv_host *host = host_set->private_data;
 	unsigned int i;
 	unsigned int handled = 0;
 	unsigned long flags;
@@ -301,9 +267,6 @@ static irqreturn_t nv_interrupt (int irq
 
 	}
 
-	if (host->host_desc->check_hotplug)
-		handled += host->host_desc->check_hotplug(host_set);
-
 	spin_unlock_irqrestore(&host_set->lock, flags);
 
 	return IRQ_RETVAL(handled);
@@ -329,10 +292,6 @@ static void nv_host_stop (struct ata_hos
 {
 	struct nv_host *host = host_set->private_data;
 
-	// Disable hotplug event interrupts.
-	if (host->host_desc->disable_hotplug)
-		host->host_desc->disable_hotplug(host_set);
-
 	kfree(host);
 
 	ata_pci_host_stop(host_set);
@@ -409,10 +368,6 @@ static int nv_init_one (struct pci_dev *
 	if (rc != NV_PORTS)
 		goto err_out_iounmap;
 
-	// Enable hotplug event interrupts.
-	if (host->host_desc->enable_hotplug)
-		host->host_desc->enable_hotplug(probe_ent);
-
 	kfree(probe_ent);
 
 	return 0;
@@ -432,129 +387,6 @@ err_out:
 	return rc;
 }
 
-static void nv_enable_hotplug(struct ata_probe_ent *probe_ent)
-{
-	u8 intr_mask;
-
-	outb(NV_INT_STATUS_HOTPLUG,
-		probe_ent->port[0].scr_addr + NV_INT_STATUS);
-
-	intr_mask = inb(probe_ent->port[0].scr_addr + NV_INT_ENABLE);
-	intr_mask |= NV_INT_ENABLE_HOTPLUG;
-
-	outb(intr_mask, probe_ent->port[0].scr_addr + NV_INT_ENABLE);
-}
-
-static void nv_disable_hotplug(struct ata_host_set *host_set)
-{
-	u8 intr_mask;
-
-	intr_mask = inb(host_set->ports[0]->ioaddr.scr_addr + NV_INT_ENABLE);
-
-	intr_mask &= ~(NV_INT_ENABLE_HOTPLUG);
-
-	outb(intr_mask, host_set->ports[0]->ioaddr.scr_addr + NV_INT_ENABLE);
-}
-
-static int nv_check_hotplug(struct ata_host_set *host_set)
-{
-	u8 intr_status;
-
-	intr_status = inb(host_set->ports[0]->ioaddr.scr_addr + NV_INT_STATUS);
-
-	// Clear interrupt status.
-	outb(0xff, host_set->ports[0]->ioaddr.scr_addr + NV_INT_STATUS);
-
-	if (intr_status & NV_INT_STATUS_HOTPLUG) {
-		if (intr_status & NV_INT_STATUS_PDEV_ADDED)
-			printk(KERN_WARNING "nv_sata: "
-				"Primary device added\n");
-
-		if (intr_status & NV_INT_STATUS_PDEV_REMOVED)
-			printk(KERN_WARNING "nv_sata: "
-				"Primary device removed\n");
-
-		if (intr_status & NV_INT_STATUS_SDEV_ADDED)
-			printk(KERN_WARNING "nv_sata: "
-				"Secondary device added\n");
-
-		if (intr_status & NV_INT_STATUS_SDEV_REMOVED)
-			printk(KERN_WARNING "nv_sata: "
-				"Secondary device removed\n");
-
-		return 1;
-	}
-
-	return 0;
-}
-
-static void nv_enable_hotplug_ck804(struct ata_probe_ent *probe_ent)
-{
-	struct pci_dev *pdev = to_pci_dev(probe_ent->dev);
-	u8 intr_mask;
-	u8 regval;
-
-	pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
-	regval |= NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
-	pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
-
-	writeb(NV_INT_STATUS_HOTPLUG, probe_ent->mmio_base + NV_INT_STATUS_CK804);
-
-	intr_mask = readb(probe_ent->mmio_base + NV_INT_ENABLE_CK804);
-	intr_mask |= NV_INT_ENABLE_HOTPLUG;
-
-	writeb(intr_mask, probe_ent->mmio_base + NV_INT_ENABLE_CK804);
-}
-
-static void nv_disable_hotplug_ck804(struct ata_host_set *host_set)
-{
-	struct pci_dev *pdev = to_pci_dev(host_set->dev);
-	u8 intr_mask;
-	u8 regval;
-
-	intr_mask = readb(host_set->mmio_base + NV_INT_ENABLE_CK804);
-
-	intr_mask &= ~(NV_INT_ENABLE_HOTPLUG);
-
-	writeb(intr_mask, host_set->mmio_base + NV_INT_ENABLE_CK804);
-
-	pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
-	regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
-	pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
-}
-
-static int nv_check_hotplug_ck804(struct ata_host_set *host_set)
-{
-	u8 intr_status;
-
-	intr_status = readb(host_set->mmio_base + NV_INT_STATUS_CK804);
-
-	// Clear interrupt status.
-	writeb(0xff, host_set->mmio_base + NV_INT_STATUS_CK804);
-
-	if (intr_status & NV_INT_STATUS_HOTPLUG) {
-		if (intr_status & NV_INT_STATUS_PDEV_ADDED)
-			printk(KERN_WARNING "nv_sata: "
-				"Primary device added\n");
-
-		if (intr_status & NV_INT_STATUS_PDEV_REMOVED)
-			printk(KERN_WARNING "nv_sata: "
-				"Primary device removed\n");
-
-		if (intr_status & NV_INT_STATUS_SDEV_ADDED)
-			printk(KERN_WARNING "nv_sata: "
-				"Secondary device added\n");
-
-		if (intr_status & NV_INT_STATUS_SDEV_REMOVED)
-			printk(KERN_WARNING "nv_sata: "
-				"Secondary device removed\n");
-
-		return 1;
-	}
-
-	return 0;
-}
-
 static int __init nv_init(void)
 {
 	return pci_module_init(&nv_pci_driver);
-- 
1.3.2

[PATCH 2/7] sata_nv: kill struct nv_host

[Tejun Heo]

struct nv_host is sata_nv's host_set->private_data.  It contains two
fields nv_host_desc which points to a static desc entry and unused
host_flags.  Kill nv_host and make host_set->private_data directly
point to the nv_host_desc entry.

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

---

 drivers/scsi/sata_nv.c |   24 ++----------------------
 1 files changed, 2 insertions(+), 22 deletions(-)

43cafdaf0396f38f989c6538777f69ac5397cbc4
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index 716581e..ee674dd 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -156,12 +156,6 @@ static struct nv_host_desc nv_device_tbl
 	},
 };
 
-struct nv_host
-{
-	struct nv_host_desc	*host_desc;
-	unsigned long		host_flags;
-};
-
 static struct pci_driver nv_pci_driver = {
 	.name			= DRV_NAME,
 	.id_table		= nv_pci_tbl,
@@ -290,17 +284,12 @@ static void nv_scr_write (struct ata_por
 
 static void nv_host_stop (struct ata_host_set *host_set)
 {
-	struct nv_host *host = host_set->private_data;
-
-	kfree(host);
-
 	ata_pci_host_stop(host_set);
 }
 
 static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version = 0;
-	struct nv_host *host;
 	struct ata_port_info *ppi;
 	struct ata_probe_ent *probe_ent;
 	int pci_dev_busy = 0;
@@ -342,19 +331,12 @@ static int nv_init_one (struct pci_dev *
 	if (!probe_ent)
 		goto err_out_regions;
 
-	host = kmalloc(sizeof(struct nv_host), GFP_KERNEL);
-	if (!host)
-		goto err_out_free_ent;
-
-	memset(host, 0, sizeof(struct nv_host));
-	host->host_desc = &nv_device_tbl[ent->driver_data];
-
-	probe_ent->private_data = host;
+	probe_ent->private_data = &nv_device_tbl[ent->driver_data];
 
 	probe_ent->mmio_base = pci_iomap(pdev, 5, 0);
 	if (!probe_ent->mmio_base) {
 		rc = -EIO;
-		goto err_out_free_host;
+		goto err_out_free_ent;
 	}
 
 	base = (unsigned long)probe_ent->mmio_base;
@@ -374,8 +356,6 @@ static int nv_init_one (struct pci_dev *
 
 err_out_iounmap:
 	pci_iounmap(pdev, probe_ent->mmio_base);
-err_out_free_host:
-	kfree(host);
 err_out_free_ent:
 	kfree(probe_ent);
 err_out_regions:
-- 
1.3.2

[PATCH 3/7] sata_nv: simplify interrupt constants

[Tejun Heo]

Simplify interrupt constants.

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

---

 drivers/scsi/sata_nv.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

fde0e7bc19e463c9c1eb2f331ea9744388752c59
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index ee674dd..f370044 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -54,28 +54,21 @@ enum {
 	NV_PORT0_SCR_REG_OFFSET		= 0x00,
 	NV_PORT1_SCR_REG_OFFSET		= 0x40,
 
+	/* INT_STATUS/ENABLE */
 	NV_INT_STATUS			= 0x10,
-	NV_INT_STATUS_CK804		= 0x440,
-	NV_INT_STATUS_PDEV_INT		= 0x01,
-	NV_INT_STATUS_PDEV_PM		= 0x02,
-	NV_INT_STATUS_PDEV_ADDED	= 0x04,
-	NV_INT_STATUS_PDEV_REMOVED	= 0x08,
-	NV_INT_STATUS_SDEV_INT		= 0x10,
-	NV_INT_STATUS_SDEV_PM		= 0x20,
-	NV_INT_STATUS_SDEV_ADDED	= 0x40,
-	NV_INT_STATUS_SDEV_REMOVED	= 0x80,
-
 	NV_INT_ENABLE			= 0x11,
+	NV_INT_STATUS_CK804		= 0x440,
 	NV_INT_ENABLE_CK804		= 0x441,
-	NV_INT_ENABLE_PDEV_MASK		= 0x01,
-	NV_INT_ENABLE_PDEV_PM		= 0x02,
-	NV_INT_ENABLE_PDEV_ADDED	= 0x04,
-	NV_INT_ENABLE_PDEV_REMOVED	= 0x08,
-	NV_INT_ENABLE_SDEV_MASK		= 0x10,
-	NV_INT_ENABLE_SDEV_PM		= 0x20,
-	NV_INT_ENABLE_SDEV_ADDED	= 0x40,
-	NV_INT_ENABLE_SDEV_REMOVED	= 0x80,
 
+	/* INT_STATUS/ENABLE bits */
+	NV_INT_DEV			= 0x01,
+	NV_INT_PM			= 0x02,
+	NV_INT_ADDED			= 0x04,
+	NV_INT_REMOVED			= 0x08,
+
+	NV_INT_PORT_SHIFT		= 4,	/* each port occupies 4 bits */
+
+	/* INT_CONFIG */
 	NV_INT_CONFIG			= 0x12,
 	NV_INT_CONFIG_METHD		= 0x01, // 0 = INT, 1 = SMI
 
-- 
1.3.2

[PATCH 5/7] sata_nv: improve irq handler

[Tejun Heo]

Improve irq handler such that it considers irq_stat if available.
Note that this patch introduces the following two behavior changes.

* if irq_stat is available and device interrupt is not reported,
  BMDMA/TF registers are never quiried.

* if irq_stat is available and device interrupt is reported, irq
  handler will return IRQ_HANDLED regardless of BMDMA/TF status.

This change also makes later hotplug change easier to integrate.

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

---

 drivers/scsi/sata_nv.c |   55 +++++++++++++++++++++++++++++++++++-------------
 1 files changed, 40 insertions(+), 15 deletions(-)

77fddbda2987defb2491ba370746daa380bb5c11
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index 93e74aa..f1218e5 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -291,33 +291,58 @@ static void ck804_clr_irq_status(struct 
 	writeb(0xff, host_set->mmio_base + NV_INT_STATUS_CK804);
 }
 
+static int nv_host_intr(struct ata_port *ap, int irq_stat_valid, u8 irq_stat)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+	int handled;
+
+	/* bail out if not our interrupt */
+	if (irq_stat_valid && !(irq_stat & NV_INT_DEV))
+		return 0;
+
+	/* No request pending?  Clear interrupt status anyway, in case
+	 * there's one pending.
+	 */
+	if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
+		ata_check_status(ap);
+		return irq_stat_valid;
+	}
+
+	handled = ata_host_intr(ap, qc);
+	if (unlikely(!handled) && irq_stat_valid) {
+		/* spurious interrupt, clear it */
+		ata_check_status(ap);
+		handled++;
+	}
+
+	return handled;
+}
+
 static irqreturn_t nv_interrupt (int irq, void *dev_instance,
 				 struct pt_regs *regs)
 {
 	struct ata_host_set *host_set = dev_instance;
+	struct nv_host_desc *host_desc = host_set->private_data;
+	int irq_stat_valid = 0;
+	u8 irq_stat = 0;
 	unsigned int i;
 	unsigned int handled = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&host_set->lock, flags);
 
+	if (host_desc->get_irq_status) {
+		irq_stat_valid = 1;
+		irq_stat = host_desc->get_irq_status(host_set);
+	}
+
 	for (i = 0; i < host_set->n_ports; i++) {
-		struct ata_port *ap;
-
-		ap = host_set->ports[i];
-		if (ap &&
-		    !(ap->flags & ATA_FLAG_DISABLED)) {
-			struct ata_queued_cmd *qc;
-
-			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
-				handled += ata_host_intr(ap, qc);
-			else
-				// No request pending?  Clear interrupt status
-				// anyway, in case there's one pending.
-				ap->ops->check_status(ap);
-		}
+		struct ata_port *ap = host_set->ports[i];
+
+		if (ap && !(ap->flags & ATA_FLAG_DISABLED))
+			handled += nv_host_intr(ap, irq_stat_valid, irq_stat);
 
+		irq_stat >>= NV_INT_PORT_SHIFT;
 	}
 
 	spin_unlock_irqrestore(&host_set->lock, flags);
-- 
1.3.2

[PATCH 6/7] sata_nv: implement new EH

[Tejun Heo]

Implement new EH.  freeze/thaw are performed by manipulating irq
enable on supported controllers.  On controllers which don't have irq
registers, stock BMDMA routines are used.

As SATA phy reset on some controllers fails to report proper device
signature, this patch defines nv_hardreset() which performs standard
SATA phy reset but doesn't perform classification.

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

---

 drivers/scsi/sata_nv.c |   78 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 64 insertions(+), 14 deletions(-)

da96be7f2eec90ebd3944b8106e4b351a7f311d8
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index f1218e5..b041c13 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -68,6 +68,9 @@ enum {
 
 	NV_INT_PORT_SHIFT		= 4,	/* each port occupies 4 bits */
 
+	NV_INT_ALL			= 0x0f,
+	NV_INT_MASK			= NV_INT_DEV,
+
 	/* INT_CONFIG */
 	NV_INT_CONFIG			= 0x12,
 	NV_INT_CONFIG_METHD		= 0x01, // 0 = INT, 1 = SMI
@@ -90,6 +93,9 @@ static void ck804_clr_irq_status(struct 
 static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static irqreturn_t nv_interrupt (int irq, void *dev_instance,
 				 struct pt_regs *regs);
+static void nv_freeze (struct ata_port *ap);
+static void nv_thaw (struct ata_port *ap);
+static void nv_error_handler (struct ata_port *ap);
 static u32 nv_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void nv_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static void nv_host_stop (struct ata_host_set *host_set);
@@ -207,14 +213,16 @@ static const struct ata_port_operations 
 	.exec_command		= ata_exec_command,
 	.check_status		= ata_check_status,
 	.dev_select		= ata_std_dev_select,
-	.phy_reset		= sata_phy_reset,
 	.bmdma_setup		= ata_bmdma_setup,
 	.bmdma_start		= ata_bmdma_start,
 	.bmdma_stop		= ata_bmdma_stop,
 	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
-	.eng_timeout		= ata_eng_timeout,
+	.freeze			= nv_freeze,
+	.thaw			= nv_thaw,
+	.error_handler		= nv_error_handler,
+	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
 	.data_xfer		= ata_pio_data_xfer,
 	.irq_handler		= nv_interrupt,
 	.irq_clear		= ata_bmdma_irq_clear,
@@ -225,20 +233,9 @@ static const struct ata_port_operations 
 	.host_stop		= nv_host_stop,
 };
 
-/* FIXME: The hardware provides the necessary SATA PHY controls
- * to support ATA_FLAG_SATA_RESET.  However, it is currently
- * necessary to disable that flag, to solve misdetection problems.
- * See http://bugme.osdl.org/show_bug.cgi?id=3352 for more info.
- *
- * This problem really needs to be investigated further.  But in the
- * meantime, we avoid ATA_FLAG_SATA_RESET to get people working.
- */
 static struct ata_port_info nv_port_info = {
 	.sht		= &nv_sht,
-	.host_flags	= ATA_FLAG_SATA |
-			  /* ATA_FLAG_SATA_RESET | */
-			  ATA_FLAG_SRST |
-			  ATA_FLAG_NO_LEGACY,
+	.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
 	.pio_mask	= NV_PIO_MASK,
 	.mwdma_mask	= NV_MWDMA_MASK,
 	.udma_mask	= NV_UDMA_MASK,
@@ -350,6 +347,59 @@ static irqreturn_t nv_interrupt (int irq
 	return IRQ_RETVAL(handled);
 }
 
+static void nv_freeze (struct ata_port *ap)
+{
+	struct ata_host_set *host_set = ap->host_set;
+	struct nv_host_desc *host_desc = host_set->private_data;
+	int shift = ap->port_no * NV_INT_PORT_SHIFT;
+	u8 mask;
+
+	if (!host_desc->get_irq_mask) {
+		ata_bmdma_freeze(ap);
+		return;
+	}
+
+	mask = host_desc->get_irq_mask(host_set);
+	mask &= ~(NV_INT_ALL << shift);
+	host_desc->set_irq_mask(host_set, mask);
+}
+
+static void nv_thaw (struct ata_port *ap)
+{
+	struct ata_host_set *host_set = ap->host_set;
+	struct nv_host_desc *host_desc = host_set->private_data;
+	int shift = ap->port_no * NV_INT_PORT_SHIFT;
+	u8 mask;
+
+	if (!host_desc->get_irq_mask) {
+		ata_bmdma_thaw(ap);
+		return;
+	}
+
+	host_desc->clr_irq_status(host_set);
+
+	mask = host_desc->get_irq_mask(host_set);
+	mask |= (NV_INT_MASK << shift);
+	host_desc->set_irq_mask(host_set, mask);
+}
+
+static int nv_hardreset (struct ata_port *ap, unsigned int *class)
+{
+	unsigned int dummy;
+
+	/* SATA hardreset fails to retrieve proper device signature on
+	 * some controllers.  Don't classify on hardreset.  For more
+	 * info, see http://bugme.osdl.org/show_bug.cgi?id=3352
+	 */
+	return sata_std_hardreset(ap, &dummy);
+}
+
+static void nv_error_handler (struct ata_port *ap)
+{
+	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset,
+			   nv_hardreset, ata_std_postreset);
+}
+
 static u32 nv_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {
 	if (sc_reg > SCR_CONTROL)
-- 
1.3.2

[PATCH 7/7] sata_nv: add hotplug support

[Tejun Heo]

Add hotplug support.

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

---

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

272a9ef56e7ac2fa38ddde41af45b59f8cfe61ff
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index b041c13..d88eeed 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -69,7 +69,8 @@ enum {
 	NV_INT_PORT_SHIFT		= 4,	/* each port occupies 4 bits */
 
 	NV_INT_ALL			= 0x0f,
-	NV_INT_MASK			= NV_INT_DEV,
+	NV_INT_MASK			= NV_INT_DEV |
+					  NV_INT_ADDED | NV_INT_REMOVED,
 
 	/* INT_CONFIG */
 	NV_INT_CONFIG			= 0x12,
@@ -293,6 +294,13 @@ static int nv_host_intr(struct ata_port 
 	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
 	int handled;
 
+	/* freeze if hotplugged */
+	if (irq_stat_valid &&
+	    unlikely(irq_stat & (NV_INT_ADDED | NV_INT_REMOVED))) {
+		ata_port_freeze(ap);
+		return 1;
+	}
+
 	/* bail out if not our interrupt */
 	if (irq_stat_valid && !(irq_stat & NV_INT_DEV))
 		return 0;
-- 
1.3.2

[PATCH 4/7] sata_nv: implement irq manipulation methods

[Tejun Heo]

Add four irq manipulation callbacks to nv_host_desc and implement
those methods for nf2/3 and ck804.  These methods will be used by new
irq_handler, EH and hotplug support.

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

---

 drivers/scsi/sata_nv.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

d47f98a5ada7fb993f659f4ba1eb496457122455
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index f370044..93e74aa 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -77,6 +77,16 @@ enum {
 	NV_MCP_SATA_CFG_20_SATA_SPACE_EN = 0x04,
 };
 
+static u8 nf2_get_irq_mask(struct ata_host_set *host_set);
+static void nf2_set_irq_mask(struct ata_host_set *host_set, u8 mask);
+static u8 nf2_get_irq_status(struct ata_host_set *host_set);
+static void nf2_clr_irq_status(struct ata_host_set *host_set);
+
+static u8 ck804_get_irq_mask(struct ata_host_set *host_set);
+static void ck804_set_irq_mask(struct ata_host_set *host_set, u8 mask);
+static u8 ck804_get_irq_status(struct ata_host_set *host_set);
+static void ck804_clr_irq_status(struct ata_host_set *host_set);
+
 static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static irqreturn_t nv_interrupt (int irq, void *dev_instance,
 				 struct pt_regs *regs);
@@ -133,6 +143,10 @@ static const struct pci_device_id nv_pci
 struct nv_host_desc
 {
 	enum nv_host_type	host_type;
+	u8   (*get_irq_mask) (struct ata_host_set *host_set);
+	void (*set_irq_mask) (struct ata_host_set *host_set, u8 mask);
+	u8   (*get_irq_status) (struct ata_host_set *host_set);
+	void (*clr_irq_status) (struct ata_host_set *host_set);
 };
 static struct nv_host_desc nv_device_tbl[] = {
 	{
@@ -140,12 +154,24 @@ static struct nv_host_desc nv_device_tbl
 	},
 	{
 		.host_type	= NFORCE2,
+		.get_irq_mask	= nf2_get_irq_mask,
+		.set_irq_mask	= nf2_set_irq_mask,
+		.get_irq_status	= nf2_get_irq_status,
+		.clr_irq_status	= nf2_clr_irq_status,
 	},
 	{
 		.host_type	= NFORCE3,
+		.get_irq_mask	= nf2_get_irq_mask,
+		.set_irq_mask	= nf2_set_irq_mask,
+		.get_irq_status	= nf2_get_irq_status,
+		.clr_irq_status	= nf2_clr_irq_status,
 	},
 	{
 		.host_type	= CK804,
+		.get_irq_mask	= ck804_get_irq_mask,
+		.set_irq_mask	= ck804_set_irq_mask,
+		.get_irq_status	= ck804_get_irq_status,
+		.clr_irq_status	= ck804_clr_irq_status,
 	},
 };
 
@@ -225,6 +251,46 @@ MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(pci, nv_pci_tbl);
 MODULE_VERSION(DRV_VERSION);
 
+static u8 nf2_get_irq_mask(struct ata_host_set *host_set)
+{
+	return inb(host_set->ports[0]->ioaddr.scr_addr + NV_INT_ENABLE);
+}
+
+static void nf2_set_irq_mask(struct ata_host_set *host_set, u8 mask)
+{
+	outb(mask, host_set->ports[0]->ioaddr.scr_addr + NV_INT_ENABLE);
+}
+
+static u8 nf2_get_irq_status(struct ata_host_set *host_set)
+{
+	return inb(host_set->ports[0]->ioaddr.scr_addr + NV_INT_STATUS);
+}
+
+static void nf2_clr_irq_status(struct ata_host_set *host_set)
+{
+	outb(0xff, host_set->ports[0]->ioaddr.scr_addr + NV_INT_STATUS);
+}
+
+static u8 ck804_get_irq_mask(struct ata_host_set *host_set)
+{
+	return readb(host_set->mmio_base + NV_INT_ENABLE_CK804);
+}
+
+static void ck804_set_irq_mask(struct ata_host_set *host_set, u8 mask)
+{
+	writeb(mask, host_set->mmio_base + NV_INT_ENABLE_CK804);
+}
+
+static u8 ck804_get_irq_status(struct ata_host_set *host_set)
+{
+	return readb(host_set->mmio_base + NV_INT_STATUS_CK804);
+}
+
+static void ck804_clr_irq_status(struct ata_host_set *host_set)
+{
+	writeb(0xff, host_set->mmio_base + NV_INT_STATUS_CK804);
+}
+
 static irqreturn_t nv_interrupt (int irq, void *dev_instance,
 				 struct pt_regs *regs)
 {
@@ -277,6 +343,18 @@ static void nv_scr_write (struct ata_por
 
 static void nv_host_stop (struct ata_host_set *host_set)
 {
+	struct nv_host_desc *host_desc = host_set->private_data;
+
+	/* disable SATA space for CK804 */
+	if (host_desc->host_type == CK804) {
+		struct pci_dev *pdev = to_pci_dev(host_set->dev);
+		u8 regval;
+
+		pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
+		regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
+		pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
+	}
+
 	ata_pci_host_stop(host_set);
 }
 
@@ -337,6 +415,15 @@ static int nv_init_one (struct pci_dev *
 	probe_ent->port[0].scr_addr = base + NV_PORT0_SCR_REG_OFFSET;
 	probe_ent->port[1].scr_addr = base + NV_PORT1_SCR_REG_OFFSET;
 
+	/* enable SATA space for CK804 */
+	if (ent->driver_data == CK804) {
+		u8 regval;
+
+		pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
+		regval |= NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
+		pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
+	}
+
 	pci_set_master(pdev);
 
 	rc = ata_device_add(probe_ent);
-- 
1.3.2

Re: [PATCH 4/7] sata_nv: implement irq manipulation methods

[Jeff Garzik]

Tejun Heo wrote:
> Add four irq manipulation callbacks to nv_host_desc and implement
> those methods for nf2/3 and ck804.  These methods will be used by new
> irq_handler, EH and hotplug support.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/sata_nv.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 87 insertions(+), 0 deletions(-)
> 
> d47f98a5ada7fb993f659f4ba1eb496457122455
> diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
> index f370044..93e74aa 100644
> --- a/drivers/scsi/sata_nv.c
> +++ b/drivers/scsi/sata_nv.c
> @@ -77,6 +77,16 @@ enum {
>  	NV_MCP_SATA_CFG_20_SATA_SPACE_EN = 0x04,
>  };
>  
> +static u8 nf2_get_irq_mask(struct ata_host_set *host_set);
> +static void nf2_set_irq_mask(struct ata_host_set *host_set, u8 mask);
> +static u8 nf2_get_irq_status(struct ata_host_set *host_set);
> +static void nf2_clr_irq_status(struct ata_host_set *host_set);
> +
> +static u8 ck804_get_irq_mask(struct ata_host_set *host_set);
> +static void ck804_set_irq_mask(struct ata_host_set *host_set, u8 mask);
> +static u8 ck804_get_irq_status(struct ata_host_set *host_set);
> +static void ck804_clr_irq_status(struct ata_host_set *host_set);
> +
>  static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
>  static irqreturn_t nv_interrupt (int irq, void *dev_instance,
>  				 struct pt_regs *regs);
> @@ -133,6 +143,10 @@ static const struct pci_device_id nv_pci
>  struct nv_host_desc
>  {
>  	enum nv_host_type	host_type;
> +	u8   (*get_irq_mask) (struct ata_host_set *host_set);
> +	void (*set_irq_mask) (struct ata_host_set *host_set, u8 mask);
> +	u8   (*get_irq_status) (struct ata_host_set *host_set);
> +	void (*clr_irq_status) (struct ata_host_set *host_set);

NAK this train of thought.  We don't need new mini-API hooks like this, 
when you could just implement nf2_xxx() and ck804_xxx() hooks at a 
higher level.  Rather than growing "stealth nf2 checks" like this:

+static void nv_freeze (struct ata_port *ap)
+{
+	struct ata_host_set *host_set = ap->host_set;
+	struct nv_host_desc *host_desc = host_set->private_data;
+	int shift = ap->port_no * NV_INT_PORT_SHIFT;
+	u8 mask;
+
+	if (!host_desc->get_irq_mask) {
+		ata_bmdma_freeze(ap);
+		return;
+	}

you should implement nf2_freeze() and nv_freeze() separately (or 
nf2_freeze and ck804_freeze, whatever).

Another example of adding a test, where adding a hook would be better:

  static void nv_host_stop (struct ata_host_set *host_set)
  {
+	struct nv_host_desc *host_desc = host_set->private_data;
+
+	/* disable SATA space for CK804 */
+	if (host_desc->host_type == CK804) {
+		struct pci_dev *pdev = to_pci_dev(host_set->dev);
+		u8 regval;
+
+		pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
+		regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
+		pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
+	}

Thoughout _every_ single patch, one sees old-vs-ck804 differences.  Your 
patches highlight these differences with every single irq_stat_valid 
test, for example.

I think it is better for testing a long term maintainability if your 
patches create separate hooks for old and ck804 hardware.  I think 
you'll find that it makes implementing EH and hotplug stuff much easier, 
and it will aid the integration of ADMA support down the road.  A first 
step is probably splitting the interrupt handler into nv_xxx and 
ck804_xxx versions, then proceeding your 7-patch set, keeping in mind 
the "hook preferred over 'if'" model.

	Jeff

Re: [PATCH 4/7] sata_nv: implement irq manipulation methods

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Add four irq manipulation callbacks to nv_host_desc and implement
>> those methods for nf2/3 and ck804.  These methods will be used by new
>> irq_handler, EH and hotplug support.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>>  drivers/scsi/sata_nv.c |   87 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 87 insertions(+), 0 deletions(-)
>>
>> d47f98a5ada7fb993f659f4ba1eb496457122455
>> diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
>> index f370044..93e74aa 100644
>> --- a/drivers/scsi/sata_nv.c
>> +++ b/drivers/scsi/sata_nv.c
>> @@ -77,6 +77,16 @@ enum {
>>      NV_MCP_SATA_CFG_20_SATA_SPACE_EN = 0x04,
>>  };
>>  
>> +static u8 nf2_get_irq_mask(struct ata_host_set *host_set);
>> +static void nf2_set_irq_mask(struct ata_host_set *host_set, u8 mask);
>> +static u8 nf2_get_irq_status(struct ata_host_set *host_set);
>> +static void nf2_clr_irq_status(struct ata_host_set *host_set);
>> +
>> +static u8 ck804_get_irq_mask(struct ata_host_set *host_set);
>> +static void ck804_set_irq_mask(struct ata_host_set *host_set, u8 mask);
>> +static u8 ck804_get_irq_status(struct ata_host_set *host_set);
>> +static void ck804_clr_irq_status(struct ata_host_set *host_set);
>> +
>>  static int nv_init_one (struct pci_dev *pdev, const struct 
>> pci_device_id *ent);
>>  static irqreturn_t nv_interrupt (int irq, void *dev_instance,
>>                   struct pt_regs *regs);
>> @@ -133,6 +143,10 @@ static const struct pci_device_id nv_pci
>>  struct nv_host_desc
>>  {
>>      enum nv_host_type    host_type;
>> +    u8   (*get_irq_mask) (struct ata_host_set *host_set);
>> +    void (*set_irq_mask) (struct ata_host_set *host_set, u8 mask);
>> +    u8   (*get_irq_status) (struct ata_host_set *host_set);
>> +    void (*clr_irq_status) (struct ata_host_set *host_set);
> 
> NAK this train of thought.  We don't need new mini-API hooks like this, 
> when you could just implement nf2_xxx() and ck804_xxx() hooks at a 
> higher level.  Rather than growing "stealth nf2 checks" like this:
> 
> +static void nv_freeze (struct ata_port *ap)
> +{
> +    struct ata_host_set *host_set = ap->host_set;
> +    struct nv_host_desc *host_desc = host_set->private_data;
> +    int shift = ap->port_no * NV_INT_PORT_SHIFT;
> +    u8 mask;
> +
> +    if (!host_desc->get_irq_mask) {
> +        ata_bmdma_freeze(ap);
> +        return;
> +    }
> 
> you should implement nf2_freeze() and nv_freeze() separately (or 
> nf2_freeze and ck804_freeze, whatever).
> 
> Another example of adding a test, where adding a hook would be better:
> 
>  static void nv_host_stop (struct ata_host_set *host_set)
>  {
> +    struct nv_host_desc *host_desc = host_set->private_data;
> +
> +    /* disable SATA space for CK804 */
> +    if (host_desc->host_type == CK804) {
> +        struct pci_dev *pdev = to_pci_dev(host_set->dev);
> +        u8 regval;
> +
> +        pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
> +        regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
> +        pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
> +    }
> 
> Thoughout _every_ single patch, one sees old-vs-ck804 differences.  Your 
> patches highlight these differences with every single irq_stat_valid 
> test, for example.
> 
> I think it is better for testing a long term maintainability if your 
> patches create separate hooks for old and ck804 hardware.  I think 
> you'll find that it makes implementing EH and hotplug stuff much easier, 
> and it will aid the integration of ADMA support down the road.  A first 
> step is probably splitting the interrupt handler into nv_xxx and 
> ck804_xxx versions, then proceeding your 7-patch set, keeping in mind 
> the "hook preferred over 'if'" model.

Yeap, implemented that way at first but code was much more compact this 
way, so...  I'll revert to original implementation.

Thanks.

-- 
tejun