[PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Tejun Heo]

 Hello, Jeff.

 This is rewritten sil24 driver against v2.6.13-rc3.  It seems to work
and am currently running stress test on it (random raw read of
concurrency 4, repeatitive mount/copy/checksup/unmount).  I'll keep
running stress test for at least 12 hours and let you know if
something goes wrong.  I've also tested basic error handling and it
seems to work.

 Thanks.

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

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -499,6 +499,14 @@ config SCSI_SATA_SIL
 
 	  If unsure, say N.
 
+config SCSI_SATA_SIL24
+	tristate "Silicon Image 3124/3132 SATA support"
+	depends on SCSI_SATA && PCI && EXPERIMENTAL
+	help
+	  This option enables support for Silicon Image 3124/3132 Serial ATA.
+
+	  If unsure, say N.
+
 config SCSI_SATA_SIS
 	tristate "SiS 964/180 SATA support"
 	depends on SCSI_SATA && PCI && EXPERIMENTAL
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_SCSI_ATA_PIIX)	+= libata.o 
 obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
 obj-$(CONFIG_SCSI_SATA_QSTOR)	+= libata.o sata_qstor.o
 obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
+obj-$(CONFIG_SCSI_SATA_SIL24)	+= libata.o sata_sil24.o
 obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
 obj-$(CONFIG_SCSI_SATA_VITESSE)	+= libata.o sata_vsc.o
 obj-$(CONFIG_SCSI_SATA_SIS)	+= libata.o sata_sis.o
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
new file mode 100644
--- /dev/null
+++ b/drivers/scsi/sata_sil24.c
@@ -0,0 +1,786 @@
+/*
+ * sata_sil24.c - Driver for Silicon Image 3124/3132 SATA-2 controllers
+ *
+ * Copyright 2005  Tejun Heo
+ *
+ * Based on preview driver from Silicon Image.
+ *
+ * NOTE: No NCQ/ATAPI support yet.  The preview driver didn't support
+ * NCQ nor ATAPI, and, unfortunately, I couldn't find out how to make
+ * those work.  Enabling those shouldn't be difficult.  Basic
+ * structure is all there (in libata-dev tree).  If you have any
+ * information about this hardware, please contact me or linux-ide.
+ * Info is needed on...
+ *
+ * - How to issue tagged commands and turn on sactive on issue accordingly.
+ * - Where to put an ATAPI command and how to tell the device to send it.
+ * - How to enable/use 64bit.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2, or (at your option) any
+ * later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <scsi/scsi_host.h>
+#include "scsi.h"
+#include <linux/libata.h>
+#include <asm/io.h>
+
+#define DRV_NAME	"sata_sil24"
+#define DRV_VERSION	"0.20"	/* Silicon Image's preview driver was 0.10 */
+
+#define NR_PORTS	4
+
+/*
+ * Port request block (PRB) 32 bytes
+ */
+struct sil24_prb {
+	u16	ctrl;
+	u16	prot;
+	u32	rx_cnt;
+	u8	fis[6 * 4];
+};
+
+/*
+ * Scatter gather entry (SGE) 16 bytes
+ */
+struct sil24_sge {
+	u64	addr;
+	u32	cnt;
+	u32	flags;
+};
+
+/*
+ * Port multiplier
+ */
+struct sil24_port_multiplier {
+	u32	diag;
+	u32	sactive;
+};
+
+enum {
+	/*
+	 * Global controller registers (128 bytes @ BAR0)
+	 */
+		/* 32 bit regs */
+	HOST_SLOT_STAT		= 0x00, /* 32 bit slot stat * 4 */
+	HOST_CTRL		= 0x40,
+	HOST_IRQ_STAT		= 0x44,
+	HOST_PHY_CFG		= 0x48,
+	HOST_BIST_CTRL		= 0x50,
+	HOST_BIST_PTRN		= 0x54,
+	HOST_BIST_STAT		= 0x58,
+	HOST_MEM_BIST_STAT	= 0x5c,
+	HOST_FLASH_CMD		= 0x70,
+		/* 8 bit regs */
+	HOST_FLASH_DATA		= 0x74,
+	HOST_TRANSITION_DETECT	= 0x75,
+	HOST_GPIO_CTRL		= 0x76,
+	HOST_I2C_ADDR		= 0x78, /* 32 bit */
+	HOST_I2C_DATA		= 0x7c,
+	HOST_I2C_XFER_CNT	= 0x7e,
+	HOST_I2C_CTRL		= 0x7f,
+
+	/* HOST_SLOT_STAT bits */
+	HOST_SSTAT_ATTN		= (1 << 31),
+
+	/*
+	 * Port registers
+	 * (8192 bytes @ +0x0000, +0x2000, +0x4000 and +0x6000 @ BAR2)
+	 */
+	PORT_REGS_SIZE		= 0x2000,
+	PORT_PRB		= 0x0000, /* (32 bytes PRB + 16 bytes SGEs * 6) * 31 (3968 bytes) */
+		/* TF is overlayed w/ PRB regs in the preview driver,
+		 * but it doesn't seem to work. */
+	PORT_TF			= 0x0000,
+
+	PORT_PM			= 0x0f80, /* 8 bytes PM * 16 (128 bytes) */
+		/* 32 bit regs */
+	PORT_CTRL_STAT		= 0x1000, /* write:ctrl, read:stat */
+	PORT_CTRL_CLR		= 0x1004,
+	PORT_IRQ_STAT		= 0x1008,
+	PORT_IRQ_ENABLE_SET	= 0x1010,
+	PORT_IRQ_ENABLE_CLR	= 0x1014,
+	PORT_ACTIVATE_UPPER_ADDR= 0x101c,
+	PORT_EXEC_FIFO		= 0x1020,
+	PORT_CMD_ERR		= 0x1024,
+	PORT_FIS_CFG		= 0x1028,
+	PORT_FIFO_THRES		= 0x102c,
+		/* 16 bit regs */
+	PORT_DECODE_ERR_CNT	= 0x1040,
+	PORT_DECODE_ERR_THRESH	= 0x1042,
+	PORT_CRC_ERR_CNT	= 0x1044,
+	PORT_CRC_ERR_THRESH	= 0x1046,
+	PORT_HSHK_ERR_CNT	= 0x1048,
+	PORT_HSHK_ERR_THRESH	= 0x104a,
+		/* 32 bit regs */
+	PORT_PHY_CFG		= 0x1050,
+	PORT_SLOT_STAT		= 0x1800,
+	PORT_CMD_ACTIVATE	= 0x1c00, /* 64 bit cmd activate * 31 (248 bytes) */
+	PORT_EXEC_DIAG		= 0x1e00, /* 32bit exec diag * 16 (64 bytes, 0-10 used on 3124) */
+	PORT_PSD_DIAG		= 0x1e40, /* 32bit psd diag * 16 (64 bytes, 0-8 used on 3124) */
+	PORT_SCONTROL		= 0x1f00,
+	PORT_SSTATUS		= 0x1f04,
+	PORT_SERROR		= 0x1f08,
+	PORT_SACTIVE		= 0x1f0c,
+
+	/* PORT_CTRL_STAT bits */
+	PORT_CS_PORT_RST	= (1 << 0), /* port reset */
+	PORT_CS_DEV_RST		= (1 << 1), /* device reset */
+	PORT_CS_INIT		= (1 << 2), /* port initialize */
+	PORT_CS_IRQ_WOC		= (1 << 3), /* interrupt write one to clear */
+	PORT_CS_RESUME		= (1 << 4), /* port resume */
+	PORT_CS_32BIT_ACTV	= (1 << 5), /* 32-bit activation */
+	PORT_CS_PM_EN		= (1 << 6), /* port multiplier enable */
+	PORT_CS_RDY		= (1 << 7), /* port ready to accept commands */
+
+	/* PORT_IRQ_STAT/ENABLE_SET/CLR */
+	/* bits[11:0] are masked */
+	PORT_IRQ_COMPLETE	= (1 << 0), /* command(s) completed */
+	PORT_IRQ_ERROR		= (1 << 1), /* command execution error */
+	PORT_IRQ_PORTRDY_CHG	= (1 << 2), /* port ready change */
+	PORT_IRQ_PWR_CHG	= (1 << 3), /* power management change */
+	PORT_IRQ_PHYRDY_CHG	= (1 << 4), /* PHY ready change */
+	PORT_IRQ_COMWAKE	= (1 << 5), /* COMWAKE received */
+	PORT_IRQ_UNK_FIS	= (1 << 6), /* Unknown FIS received */
+	PORT_IRQ_SDB_FIS	= (1 << 11), /* SDB FIS received */
+
+	/* bits[27:16] are unmasked (raw) */
+	PORT_IRQ_RAW_SHIFT	= 16,
+	PORT_IRQ_MASKED_MASK	= 0x7ff,
+	PORT_IRQ_RAW_MASK	= (0x7ff << PORT_IRQ_RAW_SHIFT),
+
+	/* ENABLE_SET/CLR specific, intr steering - 2 bit field */
+	PORT_IRQ_STEER_SHIFT	= 30,
+	PORT_IRQ_STEER_MASK	= (3 << PORT_IRQ_STEER_SHIFT),
+
+	/* PORT_CMD_ERR constants */
+	PORT_CERR_DEV		= 1, /* Error bit in D2H Register FIS */
+	PORT_CERR_SDB		= 2, /* Error bit in SDB FIS */
+	PORT_CERR_DATA		= 3, /* Error in data FIS not detected by dev */
+	PORT_CERR_SEND		= 4, /* Initial cmd FIS transmission failure */
+	PORT_CERR_INCONSISTENT	= 5, /* Protocol mismatch */
+	PORT_CERR_DIRECTION	= 6, /* Data direction mismatch */
+	PORT_CERR_UNDERRUN	= 7, /* Ran out of SGEs while writing */
+	PORT_CERR_OVERRUN	= 8, /* Ran out of SGEs while reading */
+	PORT_CERR_PKT_PROT	= 11, /* DIR invalid in 1st PIO setup of ATAPI */
+	PORT_CERR_SGT_BOUNDARY	= 16, /* PLD ecode 00 - SGT not on qword boundary */
+	PORT_CERR_SGT_TGTABRT	= 17, /* PLD ecode 01 - target abort */
+	PORT_CERR_SGT_MSTABRT	= 18, /* PLD ecode 10 - master abort */
+	PORT_CERR_SGT_PCIPERR	= 19, /* PLD ecode 11 - PCI parity err while fetching SGT */
+	PORT_CERR_CMD_BOUNDARY	= 24, /* ctrl[15:13] 001 - PRB not on qword boundary */
+	PORT_CERR_CMD_TGTABRT	= 25, /* ctrl[15:13] 010 - target abort */
+	PORT_CERR_CMD_MSTABRT	= 26, /* ctrl[15:13] 100 - master abort */
+	PORT_CERR_CMD_PCIPERR	= 27, /* ctrl[15:13] 110 - PCI parity err while fetching PRB */
+	PORT_CERR_XFR_UNDEF	= 32, /* PSD ecode 00 - undefined */
+	PORT_CERR_XFR_TGTABRT	= 33, /* PSD ecode 01 - target abort */
+	PORT_CERR_XFR_MSGABRT	= 34, /* PSD ecode 10 - master abort */
+	PORT_CERR_XFR_PCIPERR	= 35, /* PSD ecode 11 - PCI prity err during transfer */
+	PORT_CERR_SENDSERVICE	= 36, /* FIS received whiel sending service */
+
+	/*
+	 * Other constants
+	 */
+	SGE_TRM			= (1 << 31), /* Last SGE in chain */
+	PRB_SOFT_RST		= (1 << 7),  /* Soft reset request (ign BSY?) */
+
+	/* board id */
+	BID_SIL3124		= 0,
+	BID_SIL3132		= 1,
+
+	IRQ_STAT_4PORTS		= 0xf,
+};
+
+struct sil24_cmd_block {
+	struct sil24_prb prb;
+	struct sil24_sge sge[LIBATA_MAX_PRD];
+};
+
+/*
+ * ap->private_data
+ *
+ * The preview driver always returned 0 for status.  We emulate it
+ * here from the previous interrupt.
+ */
+struct sil24_port_priv {
+	void *port;
+	struct sil24_cmd_block *cmd_block;	/* 32 cmd blocks */
+	dma_addr_t cmd_block_dma;		/* DMA base addr for them */
+};
+
+/* ap->host_set->private_data */
+struct sil24_host_priv {
+	void *host_base;	/* global controller control (128 bytes @BAR0) */
+	void *port_base;	/* port registers (4 * 8192 bytes @BAR2) */
+};
+
+static u8 sil24_check_status(struct ata_port *ap);
+static u8 sil24_check_err(struct ata_port *ap);
+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_phy_reset(struct ata_port *ap);
+static void sil24_qc_prep(struct ata_queued_cmd *qc);
+static int sil24_qc_issue(struct ata_queued_cmd *qc);
+static void sil24_irq_clear(struct ata_port *ap);
+static void sil24_eng_timeout(struct ata_port *ap);
+static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
+static int sil24_port_start(struct ata_port *ap);
+static void sil24_port_stop(struct ata_port *ap);
+static void sil24_host_stop(struct ata_host_set *host_set);
+static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+
+static struct pci_device_id sil24_pci_tbl[] = {
+	{ 0x1095, 0x3124, PCI_ANY_ID, PCI_ANY_ID, 0, 0, BID_SIL3124 },
+	{ 0x1095, 0x3132, PCI_ANY_ID, PCI_ANY_ID, 0, 0, BID_SIL3132 },
+};
+
+static struct pci_driver sil24_pci_driver = {
+	.name			= DRV_NAME,
+	.id_table		= sil24_pci_tbl,
+	.probe			= sil24_init_one,
+	.remove			= ata_pci_remove_one, /* safe? */
+};
+
+static Scsi_Host_Template sil24_sht = {
+	.module			= THIS_MODULE,
+	.name			= DRV_NAME,
+	.ioctl			= ata_scsi_ioctl,
+	.queuecommand		= ata_scsi_queuecmd,
+	.eh_strategy_handler	= ata_scsi_error,
+	.can_queue		= ATA_DEF_QUEUE,
+	.this_id		= ATA_SHT_THIS_ID,
+	.sg_tablesize		= LIBATA_MAX_PRD,
+	.max_sectors		= ATA_MAX_SECTORS,
+	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
+	.emulated		= ATA_SHT_EMULATED,
+	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.proc_name		= DRV_NAME,
+	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.slave_configure	= ata_scsi_slave_config,
+	.bios_param		= ata_std_bios_param,
+	.ordered_flush		= 1, /* NCQ not supported yet */
+};
+
+static struct ata_port_operations sil24_ops = {
+	.port_disable		= ata_port_disable,
+
+	.check_status		= sil24_check_status,
+	.check_altstatus	= sil24_check_status,
+	.check_err		= sil24_check_err,
+	.dev_select		= ata_noop_dev_select,
+
+	.phy_reset		= sil24_phy_reset,
+
+	.qc_prep		= sil24_qc_prep,
+	.qc_issue		= sil24_qc_issue,
+
+	.eng_timeout		= sil24_eng_timeout,
+
+	.irq_handler		= sil24_interrupt,
+	.irq_clear		= sil24_irq_clear,
+
+	.scr_read		= sil24_scr_read,
+	.scr_write		= sil24_scr_write,
+
+	.port_start		= sil24_port_start,
+	.port_stop		= sil24_port_stop,
+	.host_stop		= sil24_host_stop,
+};
+
+static struct ata_port_info sil24_port_info[] = {
+	/* sil_3124 */
+	{
+		.sht		= &sil24_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				  ATA_FLAG_PIO_DMA,
+		.pio_mask	= 0x1f,			/* pio0-4 */
+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
+		.udma_mask	= 0x3f,			/* udma0-5 */
+		.port_ops	= &sil24_ops,
+	},
+	/* sil_3132 */ 
+	{
+		.sht		= &sil24_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				  ATA_FLAG_PIO_DMA,
+		.pio_mask	= 0x1f,			/* pio0-4 */
+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
+		.udma_mask	= 0x3f,			/* udma0-5 */
+		.port_ops	= &sil24_ops,
+	},
+};
+
+static u8 sil24_check_status(struct ata_port *ap)
+{
+	return ATA_DRDY;
+}
+
+static u8 sil24_check_err(struct ata_port *ap)
+{
+	return 0;
+}
+
+static int sil24_scr_map[] = {
+	[SCR_CONTROL]	= 0,
+	[SCR_STATUS]	= 1,
+	[SCR_ERROR]	= 2,
+	[SCR_ACTIVE]	= 3,
+};
+
+static u32 sil24_scr_read(struct ata_port *ap, unsigned sc_reg)
+{
+	void *scr_addr = (void *)ap->ioaddr.scr_addr;
+	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
+		void *addr;
+		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
+		return readl(scr_addr + sil24_scr_map[sc_reg] * 4);
+	}
+	return 0xffffffffU;
+}
+
+static void sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
+{
+	void *scr_addr = (void *)ap->ioaddr.scr_addr;
+	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
+		void *addr;
+		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
+		writel(val, scr_addr + sil24_scr_map[sc_reg] * 4);
+	}
+}
+
+static void sil24_phy_reset(struct ata_port *ap)
+{
+	__sata_phy_reset(ap);
+	/*
+	 * No ATAPI yet.  Just unconditionally indicate ATA device.
+	 * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
+	 * and libata core will ignore the device.
+	 */
+	if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
+		ap->device[0].class = ATA_DEV_ATA;
+}
+
+static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
+				 struct sil24_cmd_block *cb)
+{
+	struct scatterlist *sg = qc->sg;
+	struct sil24_sge *sge = cb->sge;
+	unsigned i;
+
+	for (i = 0; i < qc->n_elem; i++, sg++, sge++) {
+		sge->addr = cpu_to_le64(sg_dma_address(sg));
+		sge->cnt = cpu_to_le32(sg_dma_len(sg));
+		sge->flags = 0;
+		sge->flags = i < qc->n_elem - 1 ? 0 : cpu_to_le32(SGE_TRM);
+	}
+}
+
+static void sil24_qc_prep(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct sil24_port_priv *pp = ap->private_data;
+	struct sil24_cmd_block *cb = pp->cmd_block + qc->tag;
+	struct sil24_prb *prb = &cb->prb;
+
+	switch (qc->tf.protocol) {
+	case ATA_PROT_PIO:
+	case ATA_PROT_DMA:
+	case ATA_PROT_NODATA:
+		break;
+	default:
+		/* ATAPI isn't supported yet */
+		BUG();
+	}
+
+	ata_tf_to_fis(&qc->tf, prb->fis, 0);
+
+	if (qc->flags & ATA_QCFLAG_DMAMAP)
+		sil24_fill_sg(qc, cb);
+}
+
+static int sil24_qc_issue(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct sil24_port_priv *pp = ap->private_data;
+	dma_addr_t paddr = pp->cmd_block_dma + qc->tag * sizeof(*pp->cmd_block);
+
+	writel((u32)paddr, pp->port + PORT_CMD_ACTIVATE);
+	return 0;
+}
+
+static void sil24_irq_clear(struct ata_port *ap)
+{
+	/* unused */
+}
+
+static void sil24_reset_controller(struct ata_port *ap)
+{
+	struct sil24_port_priv *pp = ap->private_data;
+	void *port = pp->port;
+	int cnt;
+	u32 tmp;
+
+	printk(KERN_NOTICE DRV_NAME
+	       " ata%u: resetting controller...\n", ap->id);
+
+	/* Reset controller state.  Is this correct? */
+	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
+	readl(port + PORT_CTRL_STAT);	/* sync */
+
+	/* Max ~100ms */
+	for (cnt = 0; cnt < 1000; cnt++) {
+		udelay(100);
+		tmp = readl(port + PORT_CTRL_STAT);
+		if (!(tmp & PORT_CS_DEV_RST))
+			break;
+	}
+	if (tmp & PORT_CS_DEV_RST)
+		printk(KERN_ERR DRV_NAME
+		       " ata%u: failed to reset controller\n", ap->id);
+}
+
+static void sil24_eng_timeout(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc;
+
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	if (!qc) {
+		printk(KERN_ERR "ata%u: BUG: tiemout without command\n",
+		       ap->id);
+		return;
+	}
+
+	/*
+	 * hack alert!  We cannot use the supplied completion
+	 * function from inside the ->eh_strategy_handler() thread.
+	 * libata is the only user of ->eh_strategy_handler() in
+	 * any kernel, so the default scsi_done() assumes it is
+	 * not being called from the SCSI EH.
+	 */
+	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
+	qc->scsidone = scsi_finish_command;
+	ata_qc_complete(qc, ATA_ERR);
+
+	sil24_reset_controller(ap);
+}
+
+static inline void sil24_host_intr(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+	struct sil24_port_priv *pp = ap->private_data;
+	void *port = pp->port;
+	u32 slot_stat;
+
+	slot_stat = readl(port + PORT_SLOT_STAT);
+	if (!(slot_stat & HOST_SSTAT_ATTN)) {
+		if (qc)
+			ata_qc_complete(qc, 0);
+	} else {
+		u32 irq_stat, cmd_err, sstatus, serror;
+
+		irq_stat = readl(port + PORT_IRQ_STAT);
+		cmd_err = readl(port + PORT_CMD_ERR);
+		sstatus = readl(port + PORT_SSTATUS);
+		serror = readl(port + PORT_SERROR);
+
+		/* Clear IRQ/errors */
+		writel(irq_stat, port + PORT_IRQ_STAT);
+		if (cmd_err)
+			writel(cmd_err, port + PORT_CMD_ERR);
+		if (serror)
+			writel(serror, port + PORT_SERROR);
+
+		printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
+		       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
+		       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
+
+		if (qc)
+			ata_qc_complete(qc, ATA_ERR);
+
+		sil24_reset_controller(ap);
+	}
+}
+
+static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs)
+{
+	struct ata_host_set *host_set = dev_instance;
+	struct sil24_host_priv *hpriv = host_set->private_data;
+	unsigned handled = 0;
+	u32 status;
+	int i;
+
+	status = readl(hpriv->host_base + HOST_IRQ_STAT);
+
+	if (!(status & IRQ_STAT_4PORTS))
+		goto out;
+
+	spin_lock(&host_set->lock);
+
+	for (i = 0; i < host_set->n_ports; i++)
+		if (status & (1 << i)) {
+			struct ata_port *ap = host_set->ports[i];
+			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED))
+				sil24_host_intr(host_set->ports[i]);
+			else {
+				u32 tmp;
+				printk(KERN_WARNING DRV_NAME
+				       ": spurious interrupt from port %d\n", i);
+				tmp = readl(hpriv->host_base + HOST_CTRL);
+				tmp &= ~(1 << i);
+				writel(tmp, hpriv->host_base + HOST_CTRL);
+			}
+			handled++;
+		}
+
+	spin_unlock(&host_set->lock);
+ out:
+	return IRQ_RETVAL(handled);
+}
+
+static int sil24_port_start(struct ata_port *ap)
+{
+	struct device *dev = ap->host_set->dev;
+	struct sil24_host_priv *hpriv = ap->host_set->private_data;
+	struct sil24_port_priv *pp;
+	struct sil24_cmd_block *cb;
+	size_t cb_size = sizeof(*cb);
+	dma_addr_t cb_dma;
+
+	pp = kmalloc(sizeof(*pp), GFP_KERNEL);
+	if (!pp)
+		return -ENOMEM;
+	memset(pp, 0, sizeof(*pp));
+
+	cb = dma_alloc_coherent(dev, cb_size, &cb_dma, GFP_KERNEL);
+	if (!cb) {
+		kfree(pp);
+		return -ENOMEM;
+	}
+	memset(cb, 0, cb_size);
+
+	pp->port = hpriv->port_base + ap->port_no * PORT_REGS_SIZE;
+	pp->cmd_block = cb;
+	pp->cmd_block_dma = cb_dma;
+
+	ap->private_data = pp;
+
+	return 0;
+}
+
+static void sil24_port_stop(struct ata_port *ap)
+{
+	struct device *dev = ap->host_set->dev;
+	struct sil24_port_priv *pp = ap->private_data;
+	size_t cb_size = sizeof(*pp->cmd_block);
+
+	dma_free_coherent(dev, cb_size, pp->cmd_block, pp->cmd_block_dma);
+	kfree(pp);
+}
+
+static void sil24_host_stop(struct ata_host_set *host_set)
+{
+	struct sil24_host_priv *hpriv = host_set->private_data;
+
+	iounmap(hpriv->host_base);
+	iounmap(hpriv->port_base);
+	kfree(hpriv);
+}
+
+static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+	static int printed_version = 0;
+	unsigned int board_id = (unsigned int)ent->driver_data;
+	struct ata_probe_ent *probe_ent = NULL;
+	struct sil24_host_priv *hpriv = NULL;
+	void *host_base = NULL, *port_base = NULL;
+	int i, rc;
+
+	if (!printed_version++)
+		printk(KERN_DEBUG DRV_NAME " version " DRV_VERSION "\n");
+
+	rc = pci_enable_device(pdev);
+	if (rc)
+		return rc;
+
+	rc = pci_request_regions(pdev, DRV_NAME);
+	if (rc)
+		goto out_disable;
+
+	rc = -ENOMEM;
+	/* ioremap mmio registers */
+	host_base = ioremap(pci_resource_start(pdev, 0),
+			    pci_resource_len(pdev, 0));
+	if (!host_base)
+		goto out_free;
+	port_base = ioremap(pci_resource_start(pdev, 2),
+			    pci_resource_len(pdev, 2));
+	if (!port_base)
+		goto out_free;
+
+	/* allocate & init probe_ent and hpriv */
+	probe_ent = kmalloc(sizeof(*probe_ent), GFP_KERNEL);
+	if (!probe_ent)
+		goto out_free;
+
+	hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
+	if (!hpriv)
+		goto out_free;
+
+	memset(probe_ent, 0, sizeof(*probe_ent));
+	probe_ent->dev = pci_dev_to_dev(pdev);
+	INIT_LIST_HEAD(&probe_ent->node);
+
+	probe_ent->sht		= sil24_port_info[board_id].sht;
+	probe_ent->host_flags	= sil24_port_info[board_id].host_flags;
+	probe_ent->pio_mask	= sil24_port_info[board_id].pio_mask;
+	probe_ent->udma_mask	= sil24_port_info[board_id].udma_mask;
+	probe_ent->port_ops	= sil24_port_info[board_id].port_ops;
+	probe_ent->n_ports	= (board_id == BID_SIL3124) ? 4 : 2;
+
+	probe_ent->irq = pdev->irq;
+	probe_ent->irq_flags = SA_SHIRQ;
+	probe_ent->mmio_base = port_base;
+	probe_ent->private_data = hpriv;
+
+	memset(hpriv, 0, sizeof(*hpriv));
+	hpriv->host_base = host_base;
+	hpriv->port_base = port_base;
+
+	/*
+	 * Configure the device
+	 */
+	/*
+	 * FIXME: This device is certainly 64-bit capable.  We just
+	 * don't know how to use it.  After fixing 32bit activation in
+	 * this function, enable 64bit masks here.
+	 */
+	rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+	if (rc) {
+		printk(KERN_ERR DRV_NAME "(%s): 32-bit DMA enable failed\n",
+		       pci_name(pdev));
+		goto out_free;
+	}
+	rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
+	if (rc) {
+		printk(KERN_ERR DRV_NAME "(%s): 32-bit consistent DMA enable failed\n",
+		       pci_name(pdev));
+		goto out_free;
+	}
+
+	/* GPIO off */
+	writel(0, host_base + HOST_FLASH_CMD);
+
+	/* Mask interrupts during initialization */
+	writel(0, host_base + HOST_CTRL);
+
+	for (i = 0; i < probe_ent->n_ports; i++) {
+		void *port = port_base + i * PORT_REGS_SIZE;
+		unsigned long portu = (unsigned long)port;
+		u32 tmp;
+		int cnt;
+
+		probe_ent->port[i].cmd_addr = portu + PORT_TF;
+		probe_ent->port[i].ctl_addr = portu + PORT_TF + 0xa;
+		probe_ent->port[i].altstatus_addr = portu + PORT_TF + 0xa;
+		probe_ent->port[i].scr_addr = portu + PORT_SCONTROL;
+
+		ata_std_ports(&probe_ent->port[i]);
+
+		/* Initial PHY setting */
+		writel(0x20c, port + PORT_PHY_CFG);
+
+		/* Clear port RST */
+		tmp = readl(port + PORT_CTRL_STAT);
+		if (tmp & PORT_CS_PORT_RST) {
+			writel(PORT_CS_PORT_RST, port + PORT_CTRL_CLR);
+			readl(port + PORT_CTRL_STAT);	/* sync */
+			for (cnt = 0; cnt < 10; cnt++) {
+				msleep(10);
+				tmp = readl(port + PORT_CTRL_STAT);
+				if (!(tmp & PORT_CS_PORT_RST))
+					break;
+			}
+			if (tmp & PORT_CS_PORT_RST)
+				printk(KERN_ERR DRV_NAME
+				       "(%s): failed to clear port RST\n",
+				       pci_name(pdev));
+		}
+
+		/* Zero error counters. */
+		writel(0x8000, port + PORT_DECODE_ERR_THRESH);
+		writel(0x8000, port + PORT_CRC_ERR_THRESH);
+		writel(0x8000, port + PORT_HSHK_ERR_THRESH);
+		writel(0x0000, port + PORT_DECODE_ERR_CNT);
+		writel(0x0000, port + PORT_CRC_ERR_CNT);
+		writel(0x0000, port + PORT_HSHK_ERR_CNT);
+
+		/* FIXME: 32bit activation? */
+		writel(0, port + PORT_ACTIVATE_UPPER_ADDR);
+		writel(PORT_CS_32BIT_ACTV, port + PORT_CTRL_STAT);
+
+		/* Configure interrupts */
+		writel(0xffff, port + PORT_IRQ_ENABLE_CLR);
+		writel(PORT_IRQ_COMPLETE | PORT_IRQ_ERROR | PORT_IRQ_SDB_FIS,
+		       port + PORT_IRQ_ENABLE_SET);
+
+		/* Clear interrupts */
+		writel(0x0fff0fff, port + PORT_IRQ_STAT);
+		writel(PORT_CS_IRQ_WOC, port + PORT_CTRL_CLR);
+	}
+
+	/* Turn on interrupts */
+	writel(IRQ_STAT_4PORTS, host_base + HOST_CTRL);
+
+	pci_set_master(pdev);
+
+	ata_device_add(probe_ent);
+
+	kfree(probe_ent);
+	return 0;
+
+ out_free:
+	if (host_base)
+		iounmap(host_base);
+	if (port_base)
+		iounmap(port_base);
+	kfree(probe_ent);
+	kfree(hpriv);
+	pci_release_regions(pdev);
+ out_disable:
+	pci_disable_device(pdev);
+	return rc;
+}
+
+static int __init sil24_init(void)
+{
+	return pci_module_init(&sil24_pci_driver);
+}
+
+static void __exit sil24_exit(void)
+{
+	pci_unregister_driver(&sil24_pci_driver);
+}
+
+MODULE_AUTHOR("Tejun Heo");
+MODULE_DESCRIPTION("Silicon Image 3124/3132 SATA low-level driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, sil24_pci_tbl);
+
+module_init(sil24_init);
+module_exit(sil24_exit);

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Jeff Garzik]

Tejun Heo wrote:
>  Hello, Jeff.
> 
>  This is rewritten sil24 driver against v2.6.13-rc3.  It seems to work
> and am currently running stress test on it (random raw read of
> concurrency 4, repeatitive mount/copy/checksup/unmount).  I'll keep
> running stress test for at least 12 hours and let you know if
> something goes wrong.  I've also tested basic error handling and it
> seems to work.

I've merged this into the 'sil24' branch of libata-dev.git, and moved 
the original driver from Silicon Image into the 'sil24-original' branch.

So, please submit an incremental patch for any future changes to this 
driver.

Comments below, need a few fixups before pushing upstream, most likely.

Also, a question:  do you have hardware docs?


> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -499,6 +499,14 @@ config SCSI_SATA_SIL
>  
>  	  If unsure, say N.
>  
> +config SCSI_SATA_SIL24
> +	tristate "Silicon Image 3124/3132 SATA support"
> +	depends on SCSI_SATA && PCI && EXPERIMENTAL
> +	help
> +	  This option enables support for Silicon Image 3124/3132 Serial ATA.
> +
> +	  If unsure, say N.
> +
>  config SCSI_SATA_SIS
>  	tristate "SiS 964/180 SATA support"
>  	depends on SCSI_SATA && PCI && EXPERIMENTAL
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -126,6 +126,7 @@ obj-$(CONFIG_SCSI_ATA_PIIX)	+= libata.o 
>  obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
>  obj-$(CONFIG_SCSI_SATA_QSTOR)	+= libata.o sata_qstor.o
>  obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
> +obj-$(CONFIG_SCSI_SATA_SIL24)	+= libata.o sata_sil24.o
>  obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
>  obj-$(CONFIG_SCSI_SATA_VITESSE)	+= libata.o sata_vsc.o
>  obj-$(CONFIG_SCSI_SATA_SIS)	+= libata.o sata_sis.o
> diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/scsi/sata_sil24.c
> @@ -0,0 +1,786 @@
> +/*
> + * sata_sil24.c - Driver for Silicon Image 3124/3132 SATA-2 controllers
> + *
> + * Copyright 2005  Tejun Heo
> + *
> + * Based on preview driver from Silicon Image.
> + *
> + * NOTE: No NCQ/ATAPI support yet.  The preview driver didn't support
> + * NCQ nor ATAPI, and, unfortunately, I couldn't find out how to make
> + * those work.  Enabling those shouldn't be difficult.  Basic
> + * structure is all there (in libata-dev tree).  If you have any
> + * information about this hardware, please contact me or linux-ide.
> + * Info is needed on...
> + *
> + * - How to issue tagged commands and turn on sactive on issue accordingly.
> + * - Where to put an ATAPI command and how to tell the device to send it.
> + * - How to enable/use 64bit.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2, or (at your option) any
> + * later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/blkdev.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <scsi/scsi_host.h>
> +#include "scsi.h"
> +#include <linux/libata.h>
> +#include <asm/io.h>
> +
> +#define DRV_NAME	"sata_sil24"
> +#define DRV_VERSION	"0.20"	/* Silicon Image's preview driver was 0.10 */
> +
> +#define NR_PORTS	4
> +
> +/*
> + * Port request block (PRB) 32 bytes
> + */
> +struct sil24_prb {
> +	u16	ctrl;
> +	u16	prot;
> +	u32	rx_cnt;
> +	u8	fis[6 * 4];
> +};
> +
> +/*
> + * Scatter gather entry (SGE) 16 bytes
> + */
> +struct sil24_sge {
> +	u64	addr;
> +	u32	cnt;
> +	u32	flags;
> +};
> +
> +/*
> + * Port multiplier
> + */
> +struct sil24_port_multiplier {
> +	u32	diag;
> +	u32	sactive;
> +};

I know this is unlikely, but just asking...  you didn't test this with a 
port multiplier, did you?


> +enum {
> +	/*
> +	 * Global controller registers (128 bytes @ BAR0)
> +	 */
> +		/* 32 bit regs */
> +	HOST_SLOT_STAT		= 0x00, /* 32 bit slot stat * 4 */
> +	HOST_CTRL		= 0x40,
> +	HOST_IRQ_STAT		= 0x44,
> +	HOST_PHY_CFG		= 0x48,
> +	HOST_BIST_CTRL		= 0x50,
> +	HOST_BIST_PTRN		= 0x54,
> +	HOST_BIST_STAT		= 0x58,
> +	HOST_MEM_BIST_STAT	= 0x5c,
> +	HOST_FLASH_CMD		= 0x70,
> +		/* 8 bit regs */
> +	HOST_FLASH_DATA		= 0x74,
> +	HOST_TRANSITION_DETECT	= 0x75,
> +	HOST_GPIO_CTRL		= 0x76,
> +	HOST_I2C_ADDR		= 0x78, /* 32 bit */
> +	HOST_I2C_DATA		= 0x7c,
> +	HOST_I2C_XFER_CNT	= 0x7e,
> +	HOST_I2C_CTRL		= 0x7f,
> +
> +	/* HOST_SLOT_STAT bits */
> +	HOST_SSTAT_ATTN		= (1 << 31),
> +
> +	/*
> +	 * Port registers
> +	 * (8192 bytes @ +0x0000, +0x2000, +0x4000 and +0x6000 @ BAR2)
> +	 */
> +	PORT_REGS_SIZE		= 0x2000,
> +	PORT_PRB		= 0x0000, /* (32 bytes PRB + 16 bytes SGEs * 6) * 31 (3968 bytes) */
> +		/* TF is overlayed w/ PRB regs in the preview driver,
> +		 * but it doesn't seem to work. */
> +	PORT_TF			= 0x0000,
> +
> +	PORT_PM			= 0x0f80, /* 8 bytes PM * 16 (128 bytes) */
> +		/* 32 bit regs */
> +	PORT_CTRL_STAT		= 0x1000, /* write:ctrl, read:stat */
> +	PORT_CTRL_CLR		= 0x1004,
> +	PORT_IRQ_STAT		= 0x1008,
> +	PORT_IRQ_ENABLE_SET	= 0x1010,
> +	PORT_IRQ_ENABLE_CLR	= 0x1014,
> +	PORT_ACTIVATE_UPPER_ADDR= 0x101c,
> +	PORT_EXEC_FIFO		= 0x1020,
> +	PORT_CMD_ERR		= 0x1024,
> +	PORT_FIS_CFG		= 0x1028,
> +	PORT_FIFO_THRES		= 0x102c,
> +		/* 16 bit regs */
> +	PORT_DECODE_ERR_CNT	= 0x1040,
> +	PORT_DECODE_ERR_THRESH	= 0x1042,
> +	PORT_CRC_ERR_CNT	= 0x1044,
> +	PORT_CRC_ERR_THRESH	= 0x1046,
> +	PORT_HSHK_ERR_CNT	= 0x1048,
> +	PORT_HSHK_ERR_THRESH	= 0x104a,
> +		/* 32 bit regs */
> +	PORT_PHY_CFG		= 0x1050,
> +	PORT_SLOT_STAT		= 0x1800,
> +	PORT_CMD_ACTIVATE	= 0x1c00, /* 64 bit cmd activate * 31 (248 bytes) */
> +	PORT_EXEC_DIAG		= 0x1e00, /* 32bit exec diag * 16 (64 bytes, 0-10 used on 3124) */
> +	PORT_PSD_DIAG		= 0x1e40, /* 32bit psd diag * 16 (64 bytes, 0-8 used on 3124) */
> +	PORT_SCONTROL		= 0x1f00,
> +	PORT_SSTATUS		= 0x1f04,
> +	PORT_SERROR		= 0x1f08,
> +	PORT_SACTIVE		= 0x1f0c,
> +
> +	/* PORT_CTRL_STAT bits */
> +	PORT_CS_PORT_RST	= (1 << 0), /* port reset */
> +	PORT_CS_DEV_RST		= (1 << 1), /* device reset */
> +	PORT_CS_INIT		= (1 << 2), /* port initialize */
> +	PORT_CS_IRQ_WOC		= (1 << 3), /* interrupt write one to clear */
> +	PORT_CS_RESUME		= (1 << 4), /* port resume */
> +	PORT_CS_32BIT_ACTV	= (1 << 5), /* 32-bit activation */
> +	PORT_CS_PM_EN		= (1 << 6), /* port multiplier enable */
> +	PORT_CS_RDY		= (1 << 7), /* port ready to accept commands */
> +
> +	/* PORT_IRQ_STAT/ENABLE_SET/CLR */
> +	/* bits[11:0] are masked */
> +	PORT_IRQ_COMPLETE	= (1 << 0), /* command(s) completed */
> +	PORT_IRQ_ERROR		= (1 << 1), /* command execution error */
> +	PORT_IRQ_PORTRDY_CHG	= (1 << 2), /* port ready change */
> +	PORT_IRQ_PWR_CHG	= (1 << 3), /* power management change */
> +	PORT_IRQ_PHYRDY_CHG	= (1 << 4), /* PHY ready change */
> +	PORT_IRQ_COMWAKE	= (1 << 5), /* COMWAKE received */
> +	PORT_IRQ_UNK_FIS	= (1 << 6), /* Unknown FIS received */
> +	PORT_IRQ_SDB_FIS	= (1 << 11), /* SDB FIS received */
> +
> +	/* bits[27:16] are unmasked (raw) */
> +	PORT_IRQ_RAW_SHIFT	= 16,
> +	PORT_IRQ_MASKED_MASK	= 0x7ff,
> +	PORT_IRQ_RAW_MASK	= (0x7ff << PORT_IRQ_RAW_SHIFT),
> +
> +	/* ENABLE_SET/CLR specific, intr steering - 2 bit field */
> +	PORT_IRQ_STEER_SHIFT	= 30,
> +	PORT_IRQ_STEER_MASK	= (3 << PORT_IRQ_STEER_SHIFT),
> +
> +	/* PORT_CMD_ERR constants */
> +	PORT_CERR_DEV		= 1, /* Error bit in D2H Register FIS */
> +	PORT_CERR_SDB		= 2, /* Error bit in SDB FIS */
> +	PORT_CERR_DATA		= 3, /* Error in data FIS not detected by dev */
> +	PORT_CERR_SEND		= 4, /* Initial cmd FIS transmission failure */
> +	PORT_CERR_INCONSISTENT	= 5, /* Protocol mismatch */
> +	PORT_CERR_DIRECTION	= 6, /* Data direction mismatch */
> +	PORT_CERR_UNDERRUN	= 7, /* Ran out of SGEs while writing */
> +	PORT_CERR_OVERRUN	= 8, /* Ran out of SGEs while reading */
> +	PORT_CERR_PKT_PROT	= 11, /* DIR invalid in 1st PIO setup of ATAPI */
> +	PORT_CERR_SGT_BOUNDARY	= 16, /* PLD ecode 00 - SGT not on qword boundary */
> +	PORT_CERR_SGT_TGTABRT	= 17, /* PLD ecode 01 - target abort */
> +	PORT_CERR_SGT_MSTABRT	= 18, /* PLD ecode 10 - master abort */
> +	PORT_CERR_SGT_PCIPERR	= 19, /* PLD ecode 11 - PCI parity err while fetching SGT */
> +	PORT_CERR_CMD_BOUNDARY	= 24, /* ctrl[15:13] 001 - PRB not on qword boundary */
> +	PORT_CERR_CMD_TGTABRT	= 25, /* ctrl[15:13] 010 - target abort */
> +	PORT_CERR_CMD_MSTABRT	= 26, /* ctrl[15:13] 100 - master abort */
> +	PORT_CERR_CMD_PCIPERR	= 27, /* ctrl[15:13] 110 - PCI parity err while fetching PRB */
> +	PORT_CERR_XFR_UNDEF	= 32, /* PSD ecode 00 - undefined */
> +	PORT_CERR_XFR_TGTABRT	= 33, /* PSD ecode 01 - target abort */
> +	PORT_CERR_XFR_MSGABRT	= 34, /* PSD ecode 10 - master abort */
> +	PORT_CERR_XFR_PCIPERR	= 35, /* PSD ecode 11 - PCI prity err during transfer */
> +	PORT_CERR_SENDSERVICE	= 36, /* FIS received whiel sending service */
> +
> +	/*
> +	 * Other constants
> +	 */
> +	SGE_TRM			= (1 << 31), /* Last SGE in chain */
> +	PRB_SOFT_RST		= (1 << 7),  /* Soft reset request (ign BSY?) */
> +
> +	/* board id */
> +	BID_SIL3124		= 0,
> +	BID_SIL3132		= 1,
> +
> +	IRQ_STAT_4PORTS		= 0xf,
> +};
> +
> +struct sil24_cmd_block {
> +	struct sil24_prb prb;
> +	struct sil24_sge sge[LIBATA_MAX_PRD];
> +};
> +
> +/*
> + * ap->private_data
> + *
> + * The preview driver always returned 0 for status.  We emulate it
> + * here from the previous interrupt.
> + */
> +struct sil24_port_priv {
> +	void *port;
> +	struct sil24_cmd_block *cmd_block;	/* 32 cmd blocks */
> +	dma_addr_t cmd_block_dma;		/* DMA base addr for them */
> +};
> +
> +/* ap->host_set->private_data */
> +struct sil24_host_priv {
> +	void *host_base;	/* global controller control (128 bytes @BAR0) */
> +	void *port_base;	/* port registers (4 * 8192 bytes @BAR2) */
> +};
> +
> +static u8 sil24_check_status(struct ata_port *ap);
> +static u8 sil24_check_err(struct ata_port *ap);
> +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_phy_reset(struct ata_port *ap);
> +static void sil24_qc_prep(struct ata_queued_cmd *qc);
> +static int sil24_qc_issue(struct ata_queued_cmd *qc);
> +static void sil24_irq_clear(struct ata_port *ap);
> +static void sil24_eng_timeout(struct ata_port *ap);
> +static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
> +static int sil24_port_start(struct ata_port *ap);
> +static void sil24_port_stop(struct ata_port *ap);
> +static void sil24_host_stop(struct ata_host_set *host_set);
> +static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
> +
> +static struct pci_device_id sil24_pci_tbl[] = {
> +	{ 0x1095, 0x3124, PCI_ANY_ID, PCI_ANY_ID, 0, 0, BID_SIL3124 },
> +	{ 0x1095, 0x3132, PCI_ANY_ID, PCI_ANY_ID, 0, 0, BID_SIL3132 },
> +};
> +
> +static struct pci_driver sil24_pci_driver = {
> +	.name			= DRV_NAME,
> +	.id_table		= sil24_pci_tbl,
> +	.probe			= sil24_init_one,
> +	.remove			= ata_pci_remove_one, /* safe? */
> +};
> +
> +static Scsi_Host_Template sil24_sht = {
> +	.module			= THIS_MODULE,
> +	.name			= DRV_NAME,
> +	.ioctl			= ata_scsi_ioctl,
> +	.queuecommand		= ata_scsi_queuecmd,
> +	.eh_strategy_handler	= ata_scsi_error,
> +	.can_queue		= ATA_DEF_QUEUE,
> +	.this_id		= ATA_SHT_THIS_ID,
> +	.sg_tablesize		= LIBATA_MAX_PRD,
> +	.max_sectors		= ATA_MAX_SECTORS,
> +	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
> +	.emulated		= ATA_SHT_EMULATED,
> +	.use_clustering		= ATA_SHT_USE_CLUSTERING,
> +	.proc_name		= DRV_NAME,
> +	.dma_boundary		= ATA_DMA_BOUNDARY,
> +	.slave_configure	= ata_scsi_slave_config,
> +	.bios_param		= ata_std_bios_param,
> +	.ordered_flush		= 1, /* NCQ not supported yet */
> +};
> +
> +static struct ata_port_operations sil24_ops = {
> +	.port_disable		= ata_port_disable,
> +
> +	.check_status		= sil24_check_status,
> +	.check_altstatus	= sil24_check_status,
> +	.check_err		= sil24_check_err,
> +	.dev_select		= ata_noop_dev_select,
> +
> +	.phy_reset		= sil24_phy_reset,
> +
> +	.qc_prep		= sil24_qc_prep,
> +	.qc_issue		= sil24_qc_issue,
> +
> +	.eng_timeout		= sil24_eng_timeout,
> +
> +	.irq_handler		= sil24_interrupt,
> +	.irq_clear		= sil24_irq_clear,
> +
> +	.scr_read		= sil24_scr_read,
> +	.scr_write		= sil24_scr_write,
> +
> +	.port_start		= sil24_port_start,
> +	.port_stop		= sil24_port_stop,
> +	.host_stop		= sil24_host_stop,
> +};
> +
> +static struct ata_port_info sil24_port_info[] = {
> +	/* sil_3124 */
> +	{
> +		.sht		= &sil24_sht,
> +		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> +				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> +				  ATA_FLAG_PIO_DMA,
> +		.pio_mask	= 0x1f,			/* pio0-4 */
> +		.mwdma_mask	= 0x07,			/* mwdma0-2 */
> +		.udma_mask	= 0x3f,			/* udma0-5 */
> +		.port_ops	= &sil24_ops,
> +	},
> +	/* sil_3132 */ 
> +	{
> +		.sht		= &sil24_sht,
> +		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> +				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> +				  ATA_FLAG_PIO_DMA,
> +		.pio_mask	= 0x1f,			/* pio0-4 */
> +		.mwdma_mask	= 0x07,			/* mwdma0-2 */
> +		.udma_mask	= 0x3f,			/* udma0-5 */
> +		.port_ops	= &sil24_ops,
> +	},
> +};
> +
> +static u8 sil24_check_status(struct ata_port *ap)
> +{
> +	return ATA_DRDY;
> +}
> +
> +static u8 sil24_check_err(struct ata_port *ap)
> +{
> +	return 0;
> +}

For these two functions, we need to grab the values for these registers 
from the D2H Register FIS.  These should not be constant values, they 
are used in probing.


> +static int sil24_scr_map[] = {
> +	[SCR_CONTROL]	= 0,
> +	[SCR_STATUS]	= 1,
> +	[SCR_ERROR]	= 2,
> +	[SCR_ACTIVE]	= 3,
> +};
> +
> +static u32 sil24_scr_read(struct ata_port *ap, unsigned sc_reg)
> +{
> +	void *scr_addr = (void *)ap->ioaddr.scr_addr;
> +	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
> +		void *addr;
> +		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
> +		return readl(scr_addr + sil24_scr_map[sc_reg] * 4);
> +	}
> +	return 0xffffffffU;
> +}
> +
> +static void sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
> +{
> +	void *scr_addr = (void *)ap->ioaddr.scr_addr;
> +	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
> +		void *addr;
> +		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
> +		writel(val, scr_addr + sil24_scr_map[sc_reg] * 4);
> +	}
> +}
> +
> +static void sil24_phy_reset(struct ata_port *ap)
> +{
> +	__sata_phy_reset(ap);
> +	/*
> +	 * No ATAPI yet.  Just unconditionally indicate ATA device.
> +	 * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
> +	 * and libata core will ignore the device.
> +	 */
> +	if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
> +		ap->device[0].class = ATA_DEV_ATA;
> +}

We need to use the standard probing code to figure this out. 
ata_dev_classify(), etc.


> +static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
> +				 struct sil24_cmd_block *cb)
> +{
> +	struct scatterlist *sg = qc->sg;
> +	struct sil24_sge *sge = cb->sge;
> +	unsigned i;
> +
> +	for (i = 0; i < qc->n_elem; i++, sg++, sge++) {
> +		sge->addr = cpu_to_le64(sg_dma_address(sg));
> +		sge->cnt = cpu_to_le32(sg_dma_len(sg));
> +		sge->flags = 0;
> +		sge->flags = i < qc->n_elem - 1 ? 0 : cpu_to_le32(SGE_TRM);
> +	}
> +}
> +
> +static void sil24_qc_prep(struct ata_queued_cmd *qc)
> +{
> +	struct ata_port *ap = qc->ap;
> +	struct sil24_port_priv *pp = ap->private_data;
> +	struct sil24_cmd_block *cb = pp->cmd_block + qc->tag;
> +	struct sil24_prb *prb = &cb->prb;
> +
> +	switch (qc->tf.protocol) {
> +	case ATA_PROT_PIO:
> +	case ATA_PROT_DMA:
> +	case ATA_PROT_NODATA:
> +		break;
> +	default:
> +		/* ATAPI isn't supported yet */
> +		BUG();
> +	}
> +
> +	ata_tf_to_fis(&qc->tf, prb->fis, 0);
> +
> +	if (qc->flags & ATA_QCFLAG_DMAMAP)
> +		sil24_fill_sg(qc, cb);
> +}
> +
> +static int sil24_qc_issue(struct ata_queued_cmd *qc)
> +{
> +	struct ata_port *ap = qc->ap;
> +	struct sil24_port_priv *pp = ap->private_data;
> +	dma_addr_t paddr = pp->cmd_block_dma + qc->tag * sizeof(*pp->cmd_block);
> +
> +	writel((u32)paddr, pp->port + PORT_CMD_ACTIVATE);
> +	return 0;
> +}
> +
> +static void sil24_irq_clear(struct ata_port *ap)
> +{
> +	/* unused */
> +}

we need to fill this in.


> +static void sil24_reset_controller(struct ata_port *ap)
> +{
> +	struct sil24_port_priv *pp = ap->private_data;
> +	void *port = pp->port;
> +	int cnt;
> +	u32 tmp;
> +
> +	printk(KERN_NOTICE DRV_NAME
> +	       " ata%u: resetting controller...\n", ap->id);
> +
> +	/* Reset controller state.  Is this correct? */
> +	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
> +	readl(port + PORT_CTRL_STAT);	/* sync */
> +
> +	/* Max ~100ms */
> +	for (cnt = 0; cnt < 1000; cnt++) {
> +		udelay(100);
> +		tmp = readl(port + PORT_CTRL_STAT);
> +		if (!(tmp & PORT_CS_DEV_RST))
> +			break;
> +	}

Use mdelay.  We should be able to sleep here?

Either way, we want to avoid long delays like this.


> +	if (tmp & PORT_CS_DEV_RST)
> +		printk(KERN_ERR DRV_NAME
> +		       " ata%u: failed to reset controller\n", ap->id);
> +}
> +
> +static void sil24_eng_timeout(struct ata_port *ap)
> +{
> +	struct ata_queued_cmd *qc;
> +
> +	qc = ata_qc_from_tag(ap, ap->active_tag);
> +	if (!qc) {
> +		printk(KERN_ERR "ata%u: BUG: tiemout without command\n",
> +		       ap->id);
> +		return;
> +	}
> +
> +	/*
> +	 * hack alert!  We cannot use the supplied completion
> +	 * function from inside the ->eh_strategy_handler() thread.
> +	 * libata is the only user of ->eh_strategy_handler() in
> +	 * any kernel, so the default scsi_done() assumes it is
> +	 * not being called from the SCSI EH.
> +	 */
> +	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
> +	qc->scsidone = scsi_finish_command;
> +	ata_qc_complete(qc, ATA_ERR);
> +
> +	sil24_reset_controller(ap);
> +}
> +
> +static inline void sil24_host_intr(struct ata_port *ap)
> +{
> +	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
> +	struct sil24_port_priv *pp = ap->private_data;
> +	void *port = pp->port;
> +	u32 slot_stat;
> +
> +	slot_stat = readl(port + PORT_SLOT_STAT);
> +	if (!(slot_stat & HOST_SSTAT_ATTN)) {
> +		if (qc)
> +			ata_qc_complete(qc, 0);
> +	} else {
> +		u32 irq_stat, cmd_err, sstatus, serror;
> +
> +		irq_stat = readl(port + PORT_IRQ_STAT);
> +		cmd_err = readl(port + PORT_CMD_ERR);
> +		sstatus = readl(port + PORT_SSTATUS);
> +		serror = readl(port + PORT_SERROR);
> +
> +		/* Clear IRQ/errors */
> +		writel(irq_stat, port + PORT_IRQ_STAT);
> +		if (cmd_err)
> +			writel(cmd_err, port + PORT_CMD_ERR);
> +		if (serror)
> +			writel(serror, port + PORT_SERROR);
> +
> +		printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
> +		       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
> +		       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
> +
> +		if (qc)
> +			ata_qc_complete(qc, ATA_ERR);
> +
> +		sil24_reset_controller(ap);
> +	}
> +}

Two comments:
1) The "OK" test for command completion seems quite fragile.  I think 
the code may be -assuming- a command is completed.  I'll have to check 
the docs to see if HOST_IRQ_STAT + no-other-events truly guarantees an 
indication of command completion.

2) Error handling should be moved out-of-line, in a separate function.


> +static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs)
> +{
> +	struct ata_host_set *host_set = dev_instance;
> +	struct sil24_host_priv *hpriv = host_set->private_data;
> +	unsigned handled = 0;
> +	u32 status;
> +	int i;
> +
> +	status = readl(hpriv->host_base + HOST_IRQ_STAT);
> +
> +	if (!(status & IRQ_STAT_4PORTS))
> +		goto out;

also check for status==0xffffffff to indicate PCI bus fault, or hardware 
unplug.


> +	spin_lock(&host_set->lock);
> +
> +	for (i = 0; i < host_set->n_ports; i++)
> +		if (status & (1 << i)) {
> +			struct ata_port *ap = host_set->ports[i];
> +			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED))
> +				sil24_host_intr(host_set->ports[i]);
> +			else {
> +				u32 tmp;
> +				printk(KERN_WARNING DRV_NAME
> +				       ": spurious interrupt from port %d\n", i);
> +				tmp = readl(hpriv->host_base + HOST_CTRL);
> +				tmp &= ~(1 << i);
> +				writel(tmp, hpriv->host_base + HOST_CTRL);

add a comment describing what these three lines of code do


> +			}
> +			handled++;
> +		}
> +
> +	spin_unlock(&host_set->lock);
> + out:
> +	return IRQ_RETVAL(handled);
> +}
> +
> +static int sil24_port_start(struct ata_port *ap)
> +{
> +	struct device *dev = ap->host_set->dev;
> +	struct sil24_host_priv *hpriv = ap->host_set->private_data;
> +	struct sil24_port_priv *pp;
> +	struct sil24_cmd_block *cb;
> +	size_t cb_size = sizeof(*cb);
> +	dma_addr_t cb_dma;
> +
> +	pp = kmalloc(sizeof(*pp), GFP_KERNEL);
> +	if (!pp)
> +		return -ENOMEM;
> +	memset(pp, 0, sizeof(*pp));
> +
> +	cb = dma_alloc_coherent(dev, cb_size, &cb_dma, GFP_KERNEL);
> +	if (!cb) {
> +		kfree(pp);
> +		return -ENOMEM;
> +	}
> +	memset(cb, 0, cb_size);
> +
> +	pp->port = hpriv->port_base + ap->port_no * PORT_REGS_SIZE;
> +	pp->cmd_block = cb;
> +	pp->cmd_block_dma = cb_dma;
> +
> +	ap->private_data = pp;
> +
> +	return 0;
> +}
> +
> +static void sil24_port_stop(struct ata_port *ap)
> +{
> +	struct device *dev = ap->host_set->dev;
> +	struct sil24_port_priv *pp = ap->private_data;
> +	size_t cb_size = sizeof(*pp->cmd_block);
> +
> +	dma_free_coherent(dev, cb_size, pp->cmd_block, pp->cmd_block_dma);
> +	kfree(pp);
> +}
> +
> +static void sil24_host_stop(struct ata_host_set *host_set)
> +{
> +	struct sil24_host_priv *hpriv = host_set->private_data;
> +
> +	iounmap(hpriv->host_base);
> +	iounmap(hpriv->port_base);
> +	kfree(hpriv);
> +}
> +
> +static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	static int printed_version = 0;
> +	unsigned int board_id = (unsigned int)ent->driver_data;
> +	struct ata_probe_ent *probe_ent = NULL;
> +	struct sil24_host_priv *hpriv = NULL;
> +	void *host_base = NULL, *port_base = NULL;
> +	int i, rc;
> +
> +	if (!printed_version++)
> +		printk(KERN_DEBUG DRV_NAME " version " DRV_VERSION "\n");
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc)
> +		return rc;
> +
> +	rc = pci_request_regions(pdev, DRV_NAME);
> +	if (rc)
> +		goto out_disable;
> +
> +	rc = -ENOMEM;
> +	/* ioremap mmio registers */
> +	host_base = ioremap(pci_resource_start(pdev, 0),
> +			    pci_resource_len(pdev, 0));
> +	if (!host_base)
> +		goto out_free;
> +	port_base = ioremap(pci_resource_start(pdev, 2),
> +			    pci_resource_len(pdev, 2));
> +	if (!port_base)
> +		goto out_free;
> +	/* allocate & init probe_ent and hpriv */
> +	probe_ent = kmalloc(sizeof(*probe_ent), GFP_KERNEL);
> +	if (!probe_ent)
> +		goto out_free;
> +
> +	hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
> +	if (!hpriv)
> +		goto out_free;
> +
> +	memset(probe_ent, 0, sizeof(*probe_ent));
> +	probe_ent->dev = pci_dev_to_dev(pdev);
> +	INIT_LIST_HEAD(&probe_ent->node);
> +
> +	probe_ent->sht		= sil24_port_info[board_id].sht;
> +	probe_ent->host_flags	= sil24_port_info[board_id].host_flags;
> +	probe_ent->pio_mask	= sil24_port_info[board_id].pio_mask;
> +	probe_ent->udma_mask	= sil24_port_info[board_id].udma_mask;
> +	probe_ent->port_ops	= sil24_port_info[board_id].port_ops;
> +	probe_ent->n_ports	= (board_id == BID_SIL3124) ? 4 : 2;
> +
> +	probe_ent->irq = pdev->irq;
> +	probe_ent->irq_flags = SA_SHIRQ;
> +	probe_ent->mmio_base = port_base;
> +	probe_ent->private_data = hpriv;
> +
> +	memset(hpriv, 0, sizeof(*hpriv));
> +	hpriv->host_base = host_base;
> +	hpriv->port_base = port_base;
> +
> +	/*
> +	 * Configure the device
> +	 */
> +	/*
> +	 * FIXME: This device is certainly 64-bit capable.  We just
> +	 * don't know how to use it.  After fixing 32bit activation in
> +	 * this function, enable 64bit masks here.
> +	 */

elaboration?


> +	rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
> +	if (rc) {
> +		printk(KERN_ERR DRV_NAME "(%s): 32-bit DMA enable failed\n",
> +		       pci_name(pdev));
> +		goto out_free;
> +	}
> +	rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
> +	if (rc) {
> +		printk(KERN_ERR DRV_NAME "(%s): 32-bit consistent DMA enable failed\n",
> +		       pci_name(pdev));
> +		goto out_free;
> +	}
> +
> +	/* GPIO off */
> +	writel(0, host_base + HOST_FLASH_CMD);
> +
> +	/* Mask interrupts during initialization */
> +	writel(0, host_base + HOST_CTRL);

add a readl() to flush this write out to the PCI bus ("PCI posting")


> +	for (i = 0; i < probe_ent->n_ports; i++) {
> +		void *port = port_base + i * PORT_REGS_SIZE;
> +		unsigned long portu = (unsigned long)port;
> +		u32 tmp;
> +		int cnt;
> +
> +		probe_ent->port[i].cmd_addr = portu + PORT_TF;
> +		probe_ent->port[i].ctl_addr = portu + PORT_TF + 0xa;
> +		probe_ent->port[i].altstatus_addr = portu + PORT_TF + 0xa;
> +		probe_ent->port[i].scr_addr = portu + PORT_SCONTROL;
> +
> +		ata_std_ports(&probe_ent->port[i]);
> +
> +		/* Initial PHY setting */
> +		writel(0x20c, port + PORT_PHY_CFG);
> +
> +		/* Clear port RST */
> +		tmp = readl(port + PORT_CTRL_STAT);
> +		if (tmp & PORT_CS_PORT_RST) {
> +			writel(PORT_CS_PORT_RST, port + PORT_CTRL_CLR);
> +			readl(port + PORT_CTRL_STAT);	/* sync */
> +			for (cnt = 0; cnt < 10; cnt++) {
> +				msleep(10);
> +				tmp = readl(port + PORT_CTRL_STAT);
> +				if (!(tmp & PORT_CS_PORT_RST))
> +					break;
> +			}
> +			if (tmp & PORT_CS_PORT_RST)
> +				printk(KERN_ERR DRV_NAME
> +				       "(%s): failed to clear port RST\n",
> +				       pci_name(pdev));
> +		}

this is already done in sata_phy_reset()?


> +		/* Zero error counters. */
> +		writel(0x8000, port + PORT_DECODE_ERR_THRESH);
> +		writel(0x8000, port + PORT_CRC_ERR_THRESH);
> +		writel(0x8000, port + PORT_HSHK_ERR_THRESH);
> +		writel(0x0000, port + PORT_DECODE_ERR_CNT);
> +		writel(0x0000, port + PORT_CRC_ERR_CNT);
> +		writel(0x0000, port + PORT_HSHK_ERR_CNT);
> +
> +		/* FIXME: 32bit activation? */
> +		writel(0, port + PORT_ACTIVATE_UPPER_ADDR);
> +		writel(PORT_CS_32BIT_ACTV, port + PORT_CTRL_STAT);
> +
> +		/* Configure interrupts */
> +		writel(0xffff, port + PORT_IRQ_ENABLE_CLR);
> +		writel(PORT_IRQ_COMPLETE | PORT_IRQ_ERROR | PORT_IRQ_SDB_FIS,
> +		       port + PORT_IRQ_ENABLE_SET);
> +
> +		/* Clear interrupts */
> +		writel(0x0fff0fff, port + PORT_IRQ_STAT);
> +		writel(PORT_CS_IRQ_WOC, port + PORT_CTRL_CLR);
> +	}
> +
> +	/* Turn on interrupts */
> +	writel(IRQ_STAT_4PORTS, host_base + HOST_CTRL);
> +
> +	pci_set_master(pdev);
> +
> +	ata_device_add(probe_ent);

as with other drivers, add FIXME comment, indicating that the return 
value should be checked, and if ata_device_add() failed.


> +	kfree(probe_ent);
> +	return 0;
> +
> + out_free:
> +	if (host_base)
> +		iounmap(host_base);
> +	if (port_base)
> +		iounmap(port_base);
> +	kfree(probe_ent);
> +	kfree(hpriv);
> +	pci_release_regions(pdev);
> + out_disable:
> +	pci_disable_device(pdev);
> +	return rc;
> +}
> +
> +static int __init sil24_init(void)
> +{
> +	return pci_module_init(&sil24_pci_driver);
> +}
> +
> +static void __exit sil24_exit(void)
> +{
> +	pci_unregister_driver(&sil24_pci_driver);
> +}
> +
> +MODULE_AUTHOR("Tejun Heo");
> +MODULE_DESCRIPTION("Silicon Image 3124/3132 SATA low-level driver");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(pci, sil24_pci_tbl);
> +
> +module_init(sil24_init);
> +module_exit(sil24_exit);
> 

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Tejun Heo]

 Hello, Jeff.

 I'll answer to your comments in this mail (and several questions,
too) and will soon post patches fixing things in separate mails.

On Thu, Jul 28, 2005 at 04:27:37PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Hello, Jeff.
> >
> > This is rewritten sil24 driver against v2.6.13-rc3.  It seems to work
> >and am currently running stress test on it (random raw read of
> >concurrency 4, repeatitive mount/copy/checksup/unmount).  I'll keep
> >running stress test for at least 12 hours and let you know if
> >something goes wrong.  I've also tested basic error handling and it
> >seems to work.
> 
> I've merged this into the 'sil24' branch of libata-dev.git, and moved 
> the original driver from Silicon Image into the 'sil24-original' branch.
> 
> So, please submit an incremental patch for any future changes to this 
> driver.
> 
> Comments below, need a few fixups before pushing upstream, most likely.
> 
> Also, a question:  do you have hardware docs?
> 

 Unfortunately, no.  I've written this driver soley based on the SII's
preview driver.  It would be great if I can obtain the hardware docs,
so that I can do things more properly and try NCQ and ATAPI.

> 
> >diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> >--- a/drivers/scsi/Kconfig
> >+++ b/drivers/scsi/Kconfig
> >@@ -499,6 +499,14 @@ config SCSI_SATA_SIL
> > 
> > 	  If unsure, say N.
> > 
> >+config SCSI_SATA_SIL24
> >+	tristate "Silicon Image 3124/3132 SATA support"
> >+	depends on SCSI_SATA && PCI && EXPERIMENTAL
> >+	help
> >+	  This option enables support for Silicon Image 3124/3132 Serial ATA.
> >+
> >+	  If unsure, say N.
> >+
> > config SCSI_SATA_SIS
> > 	tristate "SiS 964/180 SATA support"
> > 	depends on SCSI_SATA && PCI && EXPERIMENTAL
> >diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> >--- a/drivers/scsi/Makefile
> >+++ b/drivers/scsi/Makefile
> >@@ -126,6 +126,7 @@ obj-$(CONFIG_SCSI_ATA_PIIX)	+= libata.o 
> > obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
> > obj-$(CONFIG_SCSI_SATA_QSTOR)	+= libata.o sata_qstor.o
> > obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
> >+obj-$(CONFIG_SCSI_SATA_SIL24)	+= libata.o sata_sil24.o
> > obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
> > obj-$(CONFIG_SCSI_SATA_VITESSE)	+= libata.o sata_vsc.o
> > obj-$(CONFIG_SCSI_SATA_SIS)	+= libata.o sata_sis.o
> >diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> >new file mode 100644
> >--- /dev/null
> >+++ b/drivers/scsi/sata_sil24.c
> >@@ -0,0 +1,786 @@
> >+/*
> >+ * sata_sil24.c - Driver for Silicon Image 3124/3132 SATA-2 controllers
> >+ *
> >+ * Copyright 2005  Tejun Heo
> >+ *
> >+ * Based on preview driver from Silicon Image.
> >+ *
> >+ * NOTE: No NCQ/ATAPI support yet.  The preview driver didn't support
> >+ * NCQ nor ATAPI, and, unfortunately, I couldn't find out how to make
> >+ * those work.  Enabling those shouldn't be difficult.  Basic
> >+ * structure is all there (in libata-dev tree).  If you have any
> >+ * information about this hardware, please contact me or linux-ide.
> >+ * Info is needed on...
> >+ *
> >+ * - How to issue tagged commands and turn on sactive on issue 
> >accordingly.
> >+ * - Where to put an ATAPI command and how to tell the device to send it.
> >+ * - How to enable/use 64bit.
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify it
> >+ * under the terms of the GNU General Public License as published by the
> >+ * Free Software Foundation; either version 2, or (at your option) any
> >+ * later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful, but
> >+ * WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >+ * General Public License for more details.
> >+ *
> >+ */
> >+
> >+#include <linux/kernel.h>
> >+#include <linux/module.h>
> >+#include <linux/pci.h>
> >+#include <linux/blkdev.h>
> >+#include <linux/delay.h>
> >+#include <linux/interrupt.h>
> >+#include <linux/dma-mapping.h>
> >+#include <scsi/scsi_host.h>
> >+#include "scsi.h"
> >+#include <linux/libata.h>
> >+#include <asm/io.h>
> >+
> >+#define DRV_NAME	"sata_sil24"
> >+#define DRV_VERSION	"0.20"	/* Silicon Image's preview driver was 0.10 */
> >+
> >+#define NR_PORTS	4
> >+
> >+/*
> >+ * Port request block (PRB) 32 bytes
> >+ */
> >+struct sil24_prb {
> >+	u16	ctrl;
> >+	u16	prot;
> >+	u32	rx_cnt;
> >+	u8	fis[6 * 4];
> >+};
> >+
> >+/*
> >+ * Scatter gather entry (SGE) 16 bytes
> >+ */
> >+struct sil24_sge {
> >+	u64	addr;
> >+	u32	cnt;
> >+	u32	flags;
> >+};
> >+
> >+/*
> >+ * Port multiplier
> >+ */
> >+struct sil24_port_multiplier {
> >+	u32	diag;
> >+	u32	sactive;
> >+};
> 
> I know this is unlikely, but just asking...  you didn't test this with a 
> port multiplier, did you?
> 

 No, the structure is there just for completness (the preview driver
had it).  If I can get the docs and access to a port multiplier, I'm
willing to implement PM support though.  Is any multiplier on sale?  I
cannot find any on online shops over here.

> 
> >+enum {
> >+	/*
> >+	 * Global controller registers (128 bytes @ BAR0)
> >+	 */
> >+		/* 32 bit regs */
> >+	HOST_SLOT_STAT		= 0x00, /* 32 bit slot stat * 4 */
> >+	HOST_CTRL		= 0x40,
> >+	HOST_IRQ_STAT		= 0x44,
> >+	HOST_PHY_CFG		= 0x48,
> >+	HOST_BIST_CTRL		= 0x50,
> >+	HOST_BIST_PTRN		= 0x54,
> >+	HOST_BIST_STAT		= 0x58,
> >+	HOST_MEM_BIST_STAT	= 0x5c,
> >+	HOST_FLASH_CMD		= 0x70,
> >+		/* 8 bit regs */
> >+	HOST_FLASH_DATA		= 0x74,
> >+	HOST_TRANSITION_DETECT	= 0x75,
> >+	HOST_GPIO_CTRL		= 0x76,
> >+	HOST_I2C_ADDR		= 0x78, /* 32 bit */
> >+	HOST_I2C_DATA		= 0x7c,
> >+	HOST_I2C_XFER_CNT	= 0x7e,
> >+	HOST_I2C_CTRL		= 0x7f,
> >+
> >+	/* HOST_SLOT_STAT bits */
> >+	HOST_SSTAT_ATTN		= (1 << 31),
> >+
> >+	/*
> >+	 * Port registers
> >+	 * (8192 bytes @ +0x0000, +0x2000, +0x4000 and +0x6000 @ BAR2)
> >+	 */
> >+	PORT_REGS_SIZE		= 0x2000,
> >+	PORT_PRB		= 0x0000, /* (32 bytes PRB + 16 bytes SGEs * 
> >6) * 31 (3968 bytes) */
> >+		/* TF is overlayed w/ PRB regs in the preview driver,
> >+		 * but it doesn't seem to work. */
> >+	PORT_TF			= 0x0000,
> >+
> >+	PORT_PM			= 0x0f80, /* 8 bytes PM * 16 (128 bytes) */
> >+		/* 32 bit regs */
> >+	PORT_CTRL_STAT		= 0x1000, /* write:ctrl, read:stat */
> >+	PORT_CTRL_CLR		= 0x1004,
> >+	PORT_IRQ_STAT		= 0x1008,
> >+	PORT_IRQ_ENABLE_SET	= 0x1010,
> >+	PORT_IRQ_ENABLE_CLR	= 0x1014,
> >+	PORT_ACTIVATE_UPPER_ADDR= 0x101c,
> >+	PORT_EXEC_FIFO		= 0x1020,
> >+	PORT_CMD_ERR		= 0x1024,
> >+	PORT_FIS_CFG		= 0x1028,
> >+	PORT_FIFO_THRES		= 0x102c,
> >+		/* 16 bit regs */
> >+	PORT_DECODE_ERR_CNT	= 0x1040,
> >+	PORT_DECODE_ERR_THRESH	= 0x1042,
> >+	PORT_CRC_ERR_CNT	= 0x1044,
> >+	PORT_CRC_ERR_THRESH	= 0x1046,
> >+	PORT_HSHK_ERR_CNT	= 0x1048,
> >+	PORT_HSHK_ERR_THRESH	= 0x104a,
> >+		/* 32 bit regs */
> >+	PORT_PHY_CFG		= 0x1050,
> >+	PORT_SLOT_STAT		= 0x1800,
> >+	PORT_CMD_ACTIVATE	= 0x1c00, /* 64 bit cmd activate * 31 (248 
> >bytes) */
> >+	PORT_EXEC_DIAG		= 0x1e00, /* 32bit exec diag * 16 (64 bytes, 
> >0-10 used on 3124) */
> >+	PORT_PSD_DIAG		= 0x1e40, /* 32bit psd diag * 16 (64 bytes, 
> >0-8 used on 3124) */
> >+	PORT_SCONTROL		= 0x1f00,
> >+	PORT_SSTATUS		= 0x1f04,
> >+	PORT_SERROR		= 0x1f08,
> >+	PORT_SACTIVE		= 0x1f0c,
> >+
> >+	/* PORT_CTRL_STAT bits */
> >+	PORT_CS_PORT_RST	= (1 << 0), /* port reset */
> >+	PORT_CS_DEV_RST		= (1 << 1), /* device reset */
> >+	PORT_CS_INIT		= (1 << 2), /* port initialize */
> >+	PORT_CS_IRQ_WOC		= (1 << 3), /* interrupt write one to clear 
> >*/
> >+	PORT_CS_RESUME		= (1 << 4), /* port resume */
> >+	PORT_CS_32BIT_ACTV	= (1 << 5), /* 32-bit activation */
> >+	PORT_CS_PM_EN		= (1 << 6), /* port multiplier enable */
> >+	PORT_CS_RDY		= (1 << 7), /* port ready to accept commands 
> >*/
> >+
> >+	/* PORT_IRQ_STAT/ENABLE_SET/CLR */
> >+	/* bits[11:0] are masked */
> >+	PORT_IRQ_COMPLETE	= (1 << 0), /* command(s) completed */
> >+	PORT_IRQ_ERROR		= (1 << 1), /* command execution error */
> >+	PORT_IRQ_PORTRDY_CHG	= (1 << 2), /* port ready change */
> >+	PORT_IRQ_PWR_CHG	= (1 << 3), /* power management change */
> >+	PORT_IRQ_PHYRDY_CHG	= (1 << 4), /* PHY ready change */
> >+	PORT_IRQ_COMWAKE	= (1 << 5), /* COMWAKE received */
> >+	PORT_IRQ_UNK_FIS	= (1 << 6), /* Unknown FIS received */
> >+	PORT_IRQ_SDB_FIS	= (1 << 11), /* SDB FIS received */
> >+
> >+	/* bits[27:16] are unmasked (raw) */
> >+	PORT_IRQ_RAW_SHIFT	= 16,
> >+	PORT_IRQ_MASKED_MASK	= 0x7ff,
> >+	PORT_IRQ_RAW_MASK	= (0x7ff << PORT_IRQ_RAW_SHIFT),
> >+
> >+	/* ENABLE_SET/CLR specific, intr steering - 2 bit field */
> >+	PORT_IRQ_STEER_SHIFT	= 30,
> >+	PORT_IRQ_STEER_MASK	= (3 << PORT_IRQ_STEER_SHIFT),
> >+
> >+	/* PORT_CMD_ERR constants */
> >+	PORT_CERR_DEV		= 1, /* Error bit in D2H Register FIS */
> >+	PORT_CERR_SDB		= 2, /* Error bit in SDB FIS */
> >+	PORT_CERR_DATA		= 3, /* Error in data FIS not detected by 
> >dev */
> >+	PORT_CERR_SEND		= 4, /* Initial cmd FIS transmission failure 
> >*/
> >+	PORT_CERR_INCONSISTENT	= 5, /* Protocol mismatch */
> >+	PORT_CERR_DIRECTION	= 6, /* Data direction mismatch */
> >+	PORT_CERR_UNDERRUN	= 7, /* Ran out of SGEs while writing */
> >+	PORT_CERR_OVERRUN	= 8, /* Ran out of SGEs while reading */
> >+	PORT_CERR_PKT_PROT	= 11, /* DIR invalid in 1st PIO setup of 
> >ATAPI */
> >+	PORT_CERR_SGT_BOUNDARY	= 16, /* PLD ecode 00 - SGT not on qword 
> >boundary */
> >+	PORT_CERR_SGT_TGTABRT	= 17, /* PLD ecode 01 - target abort */
> >+	PORT_CERR_SGT_MSTABRT	= 18, /* PLD ecode 10 - master abort */
> >+	PORT_CERR_SGT_PCIPERR	= 19, /* PLD ecode 11 - PCI parity err while 
> >fetching SGT */
> >+	PORT_CERR_CMD_BOUNDARY	= 24, /* ctrl[15:13] 001 - PRB not on qword 
> >boundary */
> >+	PORT_CERR_CMD_TGTABRT	= 25, /* ctrl[15:13] 010 - target abort */
> >+	PORT_CERR_CMD_MSTABRT	= 26, /* ctrl[15:13] 100 - master abort */
> >+	PORT_CERR_CMD_PCIPERR	= 27, /* ctrl[15:13] 110 - PCI parity err 
> >while fetching PRB */
> >+	PORT_CERR_XFR_UNDEF	= 32, /* PSD ecode 00 - undefined */
> >+	PORT_CERR_XFR_TGTABRT	= 33, /* PSD ecode 01 - target abort */
> >+	PORT_CERR_XFR_MSGABRT	= 34, /* PSD ecode 10 - master abort */
> >+	PORT_CERR_XFR_PCIPERR	= 35, /* PSD ecode 11 - PCI prity err during 
> >transfer */
> >+	PORT_CERR_SENDSERVICE	= 36, /* FIS received whiel sending service 
> >*/
> >+
> >+	/*
> >+	 * Other constants
> >+	 */
> >+	SGE_TRM			= (1 << 31), /* Last SGE in chain */
> >+	PRB_SOFT_RST		= (1 << 7),  /* Soft reset request (ign 
> >BSY?) */
> >+
> >+	/* board id */
> >+	BID_SIL3124		= 0,
> >+	BID_SIL3132		= 1,
> >+
> >+	IRQ_STAT_4PORTS		= 0xf,
> >+};
> >+
> >+struct sil24_cmd_block {
> >+	struct sil24_prb prb;
> >+	struct sil24_sge sge[LIBATA_MAX_PRD];
> >+};
> >+
> >+/*
> >+ * ap->private_data
> >+ *
> >+ * The preview driver always returned 0 for status.  We emulate it
> >+ * here from the previous interrupt.
> >+ */
> >+struct sil24_port_priv {
> >+	void *port;
> >+	struct sil24_cmd_block *cmd_block;	/* 32 cmd blocks */
> >+	dma_addr_t cmd_block_dma;		/* DMA base addr for them */
> >+};
> >+
> >+/* ap->host_set->private_data */
> >+struct sil24_host_priv {
> >+	void *host_base;	/* global controller control (128 bytes 
> >@BAR0) */
> >+	void *port_base;	/* port registers (4 * 8192 bytes @BAR2) */
> >+};
> >+
> >+static u8 sil24_check_status(struct ata_port *ap);
> >+static u8 sil24_check_err(struct ata_port *ap);
> >+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_phy_reset(struct ata_port *ap);
> >+static void sil24_qc_prep(struct ata_queued_cmd *qc);
> >+static int sil24_qc_issue(struct ata_queued_cmd *qc);
> >+static void sil24_irq_clear(struct ata_port *ap);
> >+static void sil24_eng_timeout(struct ata_port *ap);
> >+static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct 
> >pt_regs *regs);
> >+static int sil24_port_start(struct ata_port *ap);
> >+static void sil24_port_stop(struct ata_port *ap);
> >+static void sil24_host_stop(struct ata_host_set *host_set);
> >+static int sil24_init_one(struct pci_dev *pdev, const struct 
> >pci_device_id *ent);
> >+
> >+static struct pci_device_id sil24_pci_tbl[] = {
> >+	{ 0x1095, 0x3124, PCI_ANY_ID, PCI_ANY_ID, 0, 0, BID_SIL3124 },
> >+	{ 0x1095, 0x3132, PCI_ANY_ID, PCI_ANY_ID, 0, 0, BID_SIL3132 },
> >+};
> >+
> >+static struct pci_driver sil24_pci_driver = {
> >+	.name			= DRV_NAME,
> >+	.id_table		= sil24_pci_tbl,
> >+	.probe			= sil24_init_one,
> >+	.remove			= ata_pci_remove_one, /* safe? */
> >+};
> >+
> >+static Scsi_Host_Template sil24_sht = {
> >+	.module			= THIS_MODULE,
> >+	.name			= DRV_NAME,
> >+	.ioctl			= ata_scsi_ioctl,
> >+	.queuecommand		= ata_scsi_queuecmd,
> >+	.eh_strategy_handler	= ata_scsi_error,
> >+	.can_queue		= ATA_DEF_QUEUE,
> >+	.this_id		= ATA_SHT_THIS_ID,
> >+	.sg_tablesize		= LIBATA_MAX_PRD,
> >+	.max_sectors		= ATA_MAX_SECTORS,
> >+	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
> >+	.emulated		= ATA_SHT_EMULATED,
> >+	.use_clustering		= ATA_SHT_USE_CLUSTERING,
> >+	.proc_name		= DRV_NAME,
> >+	.dma_boundary		= ATA_DMA_BOUNDARY,
> >+	.slave_configure	= ata_scsi_slave_config,
> >+	.bios_param		= ata_std_bios_param,
> >+	.ordered_flush		= 1, /* NCQ not supported yet */
> >+};
> >+
> >+static struct ata_port_operations sil24_ops = {
> >+	.port_disable		= ata_port_disable,
> >+
> >+	.check_status		= sil24_check_status,
> >+	.check_altstatus	= sil24_check_status,
> >+	.check_err		= sil24_check_err,
> >+	.dev_select		= ata_noop_dev_select,
> >+
> >+	.phy_reset		= sil24_phy_reset,
> >+
> >+	.qc_prep		= sil24_qc_prep,
> >+	.qc_issue		= sil24_qc_issue,
> >+
> >+	.eng_timeout		= sil24_eng_timeout,
> >+
> >+	.irq_handler		= sil24_interrupt,
> >+	.irq_clear		= sil24_irq_clear,
> >+
> >+	.scr_read		= sil24_scr_read,
> >+	.scr_write		= sil24_scr_write,
> >+
> >+	.port_start		= sil24_port_start,
> >+	.port_stop		= sil24_port_stop,
> >+	.host_stop		= sil24_host_stop,
> >+};
> >+
> >+static struct ata_port_info sil24_port_info[] = {
> >+	/* sil_3124 */
> >+	{
> >+		.sht		= &sil24_sht,
> >+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> >+				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> >+				  ATA_FLAG_PIO_DMA,
> >+		.pio_mask	= 0x1f,			/* pio0-4 */
> >+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
> >+		.udma_mask	= 0x3f,			/* udma0-5 */
> >+		.port_ops	= &sil24_ops,
> >+	},
> >+	/* sil_3132 */ 
> >+	{
> >+		.sht		= &sil24_sht,
> >+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> >+				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> >+				  ATA_FLAG_PIO_DMA,
> >+		.pio_mask	= 0x1f,			/* pio0-4 */
> >+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
> >+		.udma_mask	= 0x3f,			/* udma0-5 */
> >+		.port_ops	= &sil24_ops,
> >+	},
> >+};
> >+
> >+static u8 sil24_check_status(struct ata_port *ap)
> >+{
> >+	return ATA_DRDY;
> >+}
> >+
> >+static u8 sil24_check_err(struct ata_port *ap)
> >+{
> >+	return 0;
> >+}
> 
> For these two functions, we need to grab the values for these registers 
> from the D2H Register FIS.  These should not be constant values, they 
> are used in probing.
> 

 Sadly I don't know where the values are.  The original driver seems
to assume that taskfile registers are overlayed with PORT_PRB, but
they are not.  Values are bogus there.  Again, in need of hardware
docs here.

 The original rewritten sil24 driver against NCQ helpers had simple
status register emulation using normal/error completion interrupt.  I
don't know why I stripped that while converting the driver over
vanilla libata (sorry).  I'll restore it.  It's not good, but it
correctly indicates error on error.

> 
> >+static int sil24_scr_map[] = {
> >+	[SCR_CONTROL]	= 0,
> >+	[SCR_STATUS]	= 1,
> >+	[SCR_ERROR]	= 2,
> >+	[SCR_ACTIVE]	= 3,
> >+};
> >+
> >+static u32 sil24_scr_read(struct ata_port *ap, unsigned sc_reg)
> >+{
> >+	void *scr_addr = (void *)ap->ioaddr.scr_addr;
> >+	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
> >+		void *addr;
> >+		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
> >+		return readl(scr_addr + sil24_scr_map[sc_reg] * 4);
> >+	}
> >+	return 0xffffffffU;
> >+}
> >+
> >+static void sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
> >+{
> >+	void *scr_addr = (void *)ap->ioaddr.scr_addr;
> >+	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
> >+		void *addr;
> >+		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
> >+		writel(val, scr_addr + sil24_scr_map[sc_reg] * 4);
> >+	}
> >+}
> >+
> >+static void sil24_phy_reset(struct ata_port *ap)
> >+{
> >+	__sata_phy_reset(ap);
> >+	/*
> >+	 * No ATAPI yet.  Just unconditionally indicate ATA device.
> >+	 * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
> >+	 * and libata core will ignore the device.
> >+	 */
> >+	if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
> >+		ap->device[0].class = ATA_DEV_ATA;
> >+}
> 
> We need to use the standard probing code to figure this out. 
> ata_dev_classify(), etc.
> 

 Again, the problem here is that I don't know how to access the
signature values after reset.  The preview driver didn't do that and
maybe that's why they duplicated the whole reset-probe procedure.  The
above code always forces ATA_DEV_ATA signature after reset which makes
ata_dev_identify() always use ATA_CMD_ID_ATA which will be failed by
the drive (with soon-to-be-posted status fix... :-).  So, it's a bit
hacky but achieves ATA-only support with very few lines.

 Maybe the proper thing here would be...

* get information on how to access signature information and set class
  to ATA_DEV_NONE if ATAPI.  (Or, even better, add proper ATAPI support)
* if signature info is inaccessible, we can implement ATA_FLAG_NOSIG
  flag and let libata layer figure out what's attached using both
  ATA_CMD_ID_ATA and ATA_CMD_ID_ATAPI.

> 
> >+static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
> >+				 struct sil24_cmd_block *cb)
> >+{
> >+	struct scatterlist *sg = qc->sg;
> >+	struct sil24_sge *sge = cb->sge;
> >+	unsigned i;
> >+
> >+	for (i = 0; i < qc->n_elem; i++, sg++, sge++) {
> >+		sge->addr = cpu_to_le64(sg_dma_address(sg));
> >+		sge->cnt = cpu_to_le32(sg_dma_len(sg));
> >+		sge->flags = 0;
> >+		sge->flags = i < qc->n_elem - 1 ? 0 : cpu_to_le32(SGE_TRM);
> >+	}
> >+}
> >+
> >+static void sil24_qc_prep(struct ata_queued_cmd *qc)
> >+{
> >+	struct ata_port *ap = qc->ap;
> >+	struct sil24_port_priv *pp = ap->private_data;
> >+	struct sil24_cmd_block *cb = pp->cmd_block + qc->tag;
> >+	struct sil24_prb *prb = &cb->prb;
> >+
> >+	switch (qc->tf.protocol) {
> >+	case ATA_PROT_PIO:
> >+	case ATA_PROT_DMA:
> >+	case ATA_PROT_NODATA:
> >+		break;
> >+	default:
> >+		/* ATAPI isn't supported yet */
> >+		BUG();
> >+	}
> >+
> >+	ata_tf_to_fis(&qc->tf, prb->fis, 0);
> >+
> >+	if (qc->flags & ATA_QCFLAG_DMAMAP)
> >+		sil24_fill_sg(qc, cb);
> >+}
> >+
> >+static int sil24_qc_issue(struct ata_queued_cmd *qc)
> >+{
> >+	struct ata_port *ap = qc->ap;
> >+	struct sil24_port_priv *pp = ap->private_data;
> >+	dma_addr_t paddr = pp->cmd_block_dma + qc->tag * 
> >sizeof(*pp->cmd_block);
> >+
> >+	writel((u32)paddr, pp->port + PORT_CMD_ACTIVATE);
> >+	return 0;
> >+}
> >+
> >+static void sil24_irq_clear(struct ata_port *ap)
> >+{
> >+	/* unused */
> >+}
> 
> we need to fill this in.
> 

 AFAIK, this function will be called only from ata_device_add().  As
we call that function after resetting all status (including
interrupts), this callback isn't really useful in this case, I think.

> 
> >+static void sil24_reset_controller(struct ata_port *ap)
> >+{
> >+	struct sil24_port_priv *pp = ap->private_data;
> >+	void *port = pp->port;
> >+	int cnt;
> >+	u32 tmp;
> >+
> >+	printk(KERN_NOTICE DRV_NAME
> >+	       " ata%u: resetting controller...\n", ap->id);
> >+
> >+	/* Reset controller state.  Is this correct? */
> >+	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
> >+	readl(port + PORT_CTRL_STAT);	/* sync */
> >+
> >+	/* Max ~100ms */
> >+	for (cnt = 0; cnt < 1000; cnt++) {
> >+		udelay(100);
> >+		tmp = readl(port + PORT_CTRL_STAT);
> >+		if (!(tmp & PORT_CS_DEV_RST))
> >+			break;
> >+	}
> 
> Use mdelay.  We should be able to sleep here?
> 
> Either way, we want to avoid long delays like this.
> 

With NCQ-helpers changes, all errors are handled in EH thread (NCQ
device or not), so original rewritten driver uses msleep, but here we
cannot as sil24_host_intr calls this function on error (I know it's
bad... but AHCI does the same way currently).  And the reset procedure
is what I've found by trial & error, so I don't know what the upper
time limit is.  I can change it to cnt < 100, msleep(1) or whatever
else, as it's an arbitrary value anyway.  Again, more information
needed.

> 
> >+	if (tmp & PORT_CS_DEV_RST)
> >+		printk(KERN_ERR DRV_NAME
> >+		       " ata%u: failed to reset controller\n", ap->id);
> >+}
> >+
> >+static void sil24_eng_timeout(struct ata_port *ap)
> >+{
> >+	struct ata_queued_cmd *qc;
> >+
> >+	qc = ata_qc_from_tag(ap, ap->active_tag);
> >+	if (!qc) {
> >+		printk(KERN_ERR "ata%u: BUG: tiemout without command\n",
> >+		       ap->id);
> >+		return;
> >+	}
> >+
> >+	/*
> >+	 * hack alert!  We cannot use the supplied completion
> >+	 * function from inside the ->eh_strategy_handler() thread.
> >+	 * libata is the only user of ->eh_strategy_handler() in
> >+	 * any kernel, so the default scsi_done() assumes it is
> >+	 * not being called from the SCSI EH.
> >+	 */
> >+	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
> >+	qc->scsidone = scsi_finish_command;
> >+	ata_qc_complete(qc, ATA_ERR);
> >+
> >+	sil24_reset_controller(ap);
> >+}
> >+
> >+static inline void sil24_host_intr(struct ata_port *ap)
> >+{
> >+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
> >+	struct sil24_port_priv *pp = ap->private_data;
> >+	void *port = pp->port;
> >+	u32 slot_stat;
> >+
> >+	slot_stat = readl(port + PORT_SLOT_STAT);
> >+	if (!(slot_stat & HOST_SSTAT_ATTN)) {
> >+		if (qc)
> >+			ata_qc_complete(qc, 0);
> >+	} else {
> >+		u32 irq_stat, cmd_err, sstatus, serror;
> >+
> >+		irq_stat = readl(port + PORT_IRQ_STAT);
> >+		cmd_err = readl(port + PORT_CMD_ERR);
> >+		sstatus = readl(port + PORT_SSTATUS);
> >+		serror = readl(port + PORT_SERROR);
> >+
> >+		/* Clear IRQ/errors */
> >+		writel(irq_stat, port + PORT_IRQ_STAT);
> >+		if (cmd_err)
> >+			writel(cmd_err, port + PORT_CMD_ERR);
> >+		if (serror)
> >+			writel(serror, port + PORT_SERROR);
> >+
> >+		printk(KERN_ERR DRV_NAME " ata%u: error interrupt on 
> >port%d\n"
> >+		       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x 
> >serror=0x%x\n",
> >+		       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, 
> >sstatus, serror);
> >+
> >+		if (qc)
> >+			ata_qc_complete(qc, ATA_ERR);
> >+
> >+		sil24_reset_controller(ap);
> >+	}
> >+}
> 
> Two comments:
> 1) The "OK" test for command completion seems quite fragile.  I think 
> the code may be -assuming- a command is completed.  I'll have to check 
> the docs to see if HOST_IRQ_STAT + no-other-events truly guarantees an 
> indication of command completion.
> 

 It's from the preview driver.  DOCS, PLEASE. :-)

> 2) Error handling should be moved out-of-line, in a separate function.

 Sure.

> 
> >+static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct 
> >pt_regs *regs)
> >+{
> >+	struct ata_host_set *host_set = dev_instance;
> >+	struct sil24_host_priv *hpriv = host_set->private_data;
> >+	unsigned handled = 0;
> >+	u32 status;
> >+	int i;
> >+
> >+	status = readl(hpriv->host_base + HOST_IRQ_STAT);
> >+
> >+	if (!(status & IRQ_STAT_4PORTS))
> >+		goto out;
> 
> also check for status==0xffffffff to indicate PCI bus fault, or hardware 
> unplug.
> 

 Okay.  Hmmm... Out of curiosity, are we supposed to do this kind of
test in all drivers?  Can you please point me to information regarding
this?

> 
> >+	spin_lock(&host_set->lock);
> >+
> >+	for (i = 0; i < host_set->n_ports; i++)
> >+		if (status & (1 << i)) {
> >+			struct ata_port *ap = host_set->ports[i];
> >+			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED))
> >+				sil24_host_intr(host_set->ports[i]);
> >+			else {
> >+				u32 tmp;
> >+				printk(KERN_WARNING DRV_NAME
> >+				       ": spurious interrupt from port 
> >%d\n", i);
> >+				tmp = readl(hpriv->host_base + HOST_CTRL);
> >+				tmp &= ~(1 << i);
> >+				writel(tmp, hpriv->host_base + HOST_CTRL);
> 
> add a comment describing what these three lines of code do
> 

 The code disables interrupt for the port which has generated spurious
interrupt.  The original code just printed a message and told IRQ
subsystem that it isn't ours, possibly causing "nobody cared" error.
So, this piece of code is what I've added.  I think I'll enable IRQs
just for enabled ports and remove the disabling code here.

> 
> >+			}
> >+			handled++;
> >+		}
> >+
> >+	spin_unlock(&host_set->lock);
> >+ out:
> >+	return IRQ_RETVAL(handled);
> >+}
> >+
> >+static int sil24_port_start(struct ata_port *ap)
> >+{
> >+	struct device *dev = ap->host_set->dev;
> >+	struct sil24_host_priv *hpriv = ap->host_set->private_data;
> >+	struct sil24_port_priv *pp;
> >+	struct sil24_cmd_block *cb;
> >+	size_t cb_size = sizeof(*cb);
> >+	dma_addr_t cb_dma;
> >+
> >+	pp = kmalloc(sizeof(*pp), GFP_KERNEL);
> >+	if (!pp)
> >+		return -ENOMEM;
> >+	memset(pp, 0, sizeof(*pp));
> >+
> >+	cb = dma_alloc_coherent(dev, cb_size, &cb_dma, GFP_KERNEL);
> >+	if (!cb) {
> >+		kfree(pp);
> >+		return -ENOMEM;
> >+	}
> >+	memset(cb, 0, cb_size);
> >+
> >+	pp->port = hpriv->port_base + ap->port_no * PORT_REGS_SIZE;
> >+	pp->cmd_block = cb;
> >+	pp->cmd_block_dma = cb_dma;
> >+
> >+	ap->private_data = pp;
> >+
> >+	return 0;
> >+}
> >+
> >+static void sil24_port_stop(struct ata_port *ap)
> >+{
> >+	struct device *dev = ap->host_set->dev;
> >+	struct sil24_port_priv *pp = ap->private_data;
> >+	size_t cb_size = sizeof(*pp->cmd_block);
> >+
> >+	dma_free_coherent(dev, cb_size, pp->cmd_block, pp->cmd_block_dma);
> >+	kfree(pp);
> >+}
> >+
> >+static void sil24_host_stop(struct ata_host_set *host_set)
> >+{
> >+	struct sil24_host_priv *hpriv = host_set->private_data;
> >+
> >+	iounmap(hpriv->host_base);
> >+	iounmap(hpriv->port_base);
> >+	kfree(hpriv);
> >+}
> >+
> >+static int sil24_init_one(struct pci_dev *pdev, const struct 
> >pci_device_id *ent)
> >+{
> >+	static int printed_version = 0;
> >+	unsigned int board_id = (unsigned int)ent->driver_data;
> >+	struct ata_probe_ent *probe_ent = NULL;
> >+	struct sil24_host_priv *hpriv = NULL;
> >+	void *host_base = NULL, *port_base = NULL;
> >+	int i, rc;
> >+
> >+	if (!printed_version++)
> >+		printk(KERN_DEBUG DRV_NAME " version " DRV_VERSION "\n");
> >+
> >+	rc = pci_enable_device(pdev);
> >+	if (rc)
> >+		return rc;
> >+
> >+	rc = pci_request_regions(pdev, DRV_NAME);
> >+	if (rc)
> >+		goto out_disable;
> >+
> >+	rc = -ENOMEM;
> >+	/* ioremap mmio registers */
> >+	host_base = ioremap(pci_resource_start(pdev, 0),
> >+			    pci_resource_len(pdev, 0));
> >+	if (!host_base)
> >+		goto out_free;
> >+	port_base = ioremap(pci_resource_start(pdev, 2),
> >+			    pci_resource_len(pdev, 2));
> >+	if (!port_base)
> >+		goto out_free;
> >+	/* allocate & init probe_ent and hpriv */
> >+	probe_ent = kmalloc(sizeof(*probe_ent), GFP_KERNEL);
> >+	if (!probe_ent)
> >+		goto out_free;
> >+
> >+	hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
> >+	if (!hpriv)
> >+		goto out_free;
> >+
> >+	memset(probe_ent, 0, sizeof(*probe_ent));
> >+	probe_ent->dev = pci_dev_to_dev(pdev);
> >+	INIT_LIST_HEAD(&probe_ent->node);
> >+
> >+	probe_ent->sht		= sil24_port_info[board_id].sht;
> >+	probe_ent->host_flags	= sil24_port_info[board_id].host_flags;
> >+	probe_ent->pio_mask	= sil24_port_info[board_id].pio_mask;
> >+	probe_ent->udma_mask	= sil24_port_info[board_id].udma_mask;
> >+	probe_ent->port_ops	= sil24_port_info[board_id].port_ops;
> >+	probe_ent->n_ports	= (board_id == BID_SIL3124) ? 4 : 2;
> >+
> >+	probe_ent->irq = pdev->irq;
> >+	probe_ent->irq_flags = SA_SHIRQ;
> >+	probe_ent->mmio_base = port_base;
> >+	probe_ent->private_data = hpriv;
> >+
> >+	memset(hpriv, 0, sizeof(*hpriv));
> >+	hpriv->host_base = host_base;
> >+	hpriv->port_base = port_base;
> >+
> >+	/*
> >+	 * Configure the device
> >+	 */
> >+	/*
> >+	 * FIXME: This device is certainly 64-bit capable.  We just
> >+	 * don't know how to use it.  After fixing 32bit activation in
> >+	 * this function, enable 64bit masks here.
> >+	 */
> 
> elaboration?
> 

 In the preview driver, while initializing port, PORT_CSR_M_32BIT_ACTV
is written to CtrlSetStatus.  I don't know what it really means and
have no means of testing it as I don't have any machine with more than
4gigs of memory.  So, I wasn't really sure if that 32 bit applies only
to consistent memory (maybe it's 32bit activation address?) or also to
DMA buffers.  That's why I just used 32bit masks for both of them.
Again, with docs, this should be easy to fix.

> 
> >+	rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
> >+	if (rc) {
> >+		printk(KERN_ERR DRV_NAME "(%s): 32-bit DMA enable failed\n",
> >+		       pci_name(pdev));
> >+		goto out_free;
> >+	}
> >+	rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
> >+	if (rc) {
> >+		printk(KERN_ERR DRV_NAME "(%s): 32-bit consistent DMA enable 
> >failed\n",
> >+		       pci_name(pdev));
> >+		goto out_free;
> >+	}
> >+
> >+	/* GPIO off */
> >+	writel(0, host_base + HOST_FLASH_CMD);
> >+
> >+	/* Mask interrupts during initialization */
> >+	writel(0, host_base + HOST_CTRL);
> 
> add a readl() to flush this write out to the PCI bus ("PCI posting")
> 

 Sure.  And, out of curiosity, isn't sync unnecessary unless we're
gonna perform some kind of timed waiting following it?  We don't have
any timing requirement after above interrupt masking, do we?

> 
> >+	for (i = 0; i < probe_ent->n_ports; i++) {
> >+		void *port = port_base + i * PORT_REGS_SIZE;
> >+		unsigned long portu = (unsigned long)port;
> >+		u32 tmp;
> >+		int cnt;
> >+
> >+		probe_ent->port[i].cmd_addr = portu + PORT_TF;
> >+		probe_ent->port[i].ctl_addr = portu + PORT_TF + 0xa;
> >+		probe_ent->port[i].altstatus_addr = portu + PORT_TF + 0xa;
> >+		probe_ent->port[i].scr_addr = portu + PORT_SCONTROL;
> >+
> >+		ata_std_ports(&probe_ent->port[i]);
> >+
> >+		/* Initial PHY setting */
> >+		writel(0x20c, port + PORT_PHY_CFG);
> >+
> >+		/* Clear port RST */
> >+		tmp = readl(port + PORT_CTRL_STAT);
> >+		if (tmp & PORT_CS_PORT_RST) {
> >+			writel(PORT_CS_PORT_RST, port + PORT_CTRL_CLR);
> >+			readl(port + PORT_CTRL_STAT);	/* sync */
> >+			for (cnt = 0; cnt < 10; cnt++) {
> >+				msleep(10);
> >+				tmp = readl(port + PORT_CTRL_STAT);
> >+				if (!(tmp & PORT_CS_PORT_RST))
> >+					break;
> >+			}
> >+			if (tmp & PORT_CS_PORT_RST)
> >+				printk(KERN_ERR DRV_NAME
> >+				       "(%s): failed to clear port RST\n",
> >+				       pci_name(pdev));
> >+		}
> 
> this is already done in sata_phy_reset()?
> 

 I don't know what PORT_RST does here.  This is what the preview
driver does, so...  If it's the same thing as phy reset using SATA
SCR, we can remove it.  Again, docs needed. 

> 
> >+		/* Zero error counters. */
> >+		writel(0x8000, port + PORT_DECODE_ERR_THRESH);
> >+		writel(0x8000, port + PORT_CRC_ERR_THRESH);
> >+		writel(0x8000, port + PORT_HSHK_ERR_THRESH);
> >+		writel(0x0000, port + PORT_DECODE_ERR_CNT);
> >+		writel(0x0000, port + PORT_CRC_ERR_CNT);
> >+		writel(0x0000, port + PORT_HSHK_ERR_CNT);
> >+
> >+		/* FIXME: 32bit activation? */
> >+		writel(0, port + PORT_ACTIVATE_UPPER_ADDR);
> >+		writel(PORT_CS_32BIT_ACTV, port + PORT_CTRL_STAT);
> >+
> >+		/* Configure interrupts */
> >+		writel(0xffff, port + PORT_IRQ_ENABLE_CLR);
> >+		writel(PORT_IRQ_COMPLETE | PORT_IRQ_ERROR | PORT_IRQ_SDB_FIS,
> >+		       port + PORT_IRQ_ENABLE_SET);
> >+
> >+		/* Clear interrupts */
> >+		writel(0x0fff0fff, port + PORT_IRQ_STAT);
> >+		writel(PORT_CS_IRQ_WOC, port + PORT_CTRL_CLR);
> >+	}
> >+
> >+	/* Turn on interrupts */
> >+	writel(IRQ_STAT_4PORTS, host_base + HOST_CTRL);
> >+
> >+	pci_set_master(pdev);
> >+
> >+	ata_device_add(probe_ent);
> 
> as with other drivers, add FIXME comment, indicating that the return 
> value should be checked, and if ata_device_add() failed.
> 

 Sure.

> 
> >+	kfree(probe_ent);
> >+	return 0;
> >+
> >+ out_free:
> >+	if (host_base)
> >+		iounmap(host_base);
> >+	if (port_base)
> >+		iounmap(port_base);
> >+	kfree(probe_ent);
> >+	kfree(hpriv);
> >+	pci_release_regions(pdev);
> >+ out_disable:
> >+	pci_disable_device(pdev);
> >+	return rc;
> >+}
> >+
> >+static int __init sil24_init(void)
> >+{
> >+	return pci_module_init(&sil24_pci_driver);
> >+}
> >+
> >+static void __exit sil24_exit(void)
> >+{
> >+	pci_unregister_driver(&sil24_pci_driver);
> >+}
> >+
> >+MODULE_AUTHOR("Tejun Heo");
> >+MODULE_DESCRIPTION("Silicon Image 3124/3132 SATA low-level driver");
> >+MODULE_LICENSE("GPL");
> >+MODULE_DEVICE_TABLE(pci, sil24_pci_tbl);
> >+
> >+module_init(sil24_init);
> >+module_exit(sil24_exit);
> >

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Edward Falk]

Hi Tejun; I'm the guy at Google working on SATA drivers (port 
multipliers right now).  As soon as I can (next week perhaps, I'll start 
looking at the driver you wrote.  From what I can see, it looks quite good.



>>>+
>>>+static u8 sil24_check_status(struct ata_port *ap)
>>>+{
>>>+	return ATA_DRDY;
>>>+}
>>>+
>>>+static u8 sil24_check_err(struct ata_port *ap)
>>>+{
>>>+	return 0;
>>>+}
>>
>>For these two functions, we need to grab the values for these registers 
>>from the D2H Register FIS.  These should not be constant values, they 
>>are used in probing.
>>
> 
>  Sadly I don't know where the values are.  The original driver seems
> to assume that taskfile registers are overlayed with PORT_PRB, but
> they are not.  Values are bogus there.  Again, in need of hardware
> docs here.

The original driver is broken.  Taskfile registers have to be read back 
from the returned FIS block after a command completion.

Hmmmm, perhaps libata should provide an ata_fis_to_tf() function that 
examines a FIS block, confirms that it's a device-to-host type FIS, and 
read the taskfile registers back out.



> 
>  The original rewritten sil24 driver against NCQ helpers had simple
> status register emulation using normal/error completion interrupt.  I
> don't know why I stripped that while converting the driver over
> vanilla libata (sorry).  I'll restore it.  It's not good, but it
> correctly indicates error on error.

It's still better than what we have.



>>>+static void sil24_phy_reset(struct ata_port *ap)
>>>+{
>>>+	__sata_phy_reset(ap);
>>>+	/*
>>>+	 * No ATAPI yet.  Just unconditionally indicate ATA device.
>>>+	 * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
>>>+	 * and libata core will ignore the device.
>>>+	 */
>>>+	if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
>>>+		ap->device[0].class = ATA_DEV_ATA;
>>>+}
>>
>>We need to use the standard probing code to figure this out. 
>>ata_dev_classify(), etc.
>>
> 
> 
>  Again, the problem here is that I don't know how to access the
> signature values after reset.

Again, you would need to fetch them from the returned FIS structure. 
Here's a code fragment derived from SiI's issue_soft_reset() function:

	struct Port_Registers *port_base = yadayada;
	struct sil_port_priv *pp = ap->private_data;
	struct Port_Registers *PR;	/* in memory */
	dma_addr_t paddr = pp->pc_dma; /* physical address base */

	PR = (struct Port_Registers *) (&pp->pc->mregs);
	port_base = yadayada;
	slot = 0;
	slotp = &PR->Slot[slot];
	memset(&slotp->Prb, 0, sizeof(slotp->Prb));
	slotp->Prb.Control = 0x80;		/* soft reset */
	slotp->Prb.FisType == 0;
	writel(paddr, &port_base->CmdActivate[slot].s.ActiveLow);
	if (!sil_wait_for_completion(port_base)) {
		/* timeout or error */
		return ATA_DEV_NONE;
	} else {
		/* Examine slot for taskfile registers */
		slotp = port_base->Slot[slot];
		if (slotp->Prb.FisType != 0x34 &&
		    slotp->Prb.FisType != 0x5F) {
			/* WTF?  Wrong FIS Type */
			return ATA_DEV_NONE;
		}
		if (slotp->Prb.CylLow == 0 &&
		    slotp->Prb.CylHigh == 0)
			return ATA_DEV_ATA;
		if (slotp->Prb.CylLow == 0x14 &&
		    slotp->Prb.CylHigh == 0xEB)
			return ATA_DEV_ATAPI;
		if (slotp->Prb.CylLow == 0x69 &&
		    slotp->Prb.CylHigh == 0x96)
			return ATA_DEV_PORT_MULTIPLIER;
		printk(KERN_WARN "unknown ATA device signature %x.%x\n",
			slotp->Prb.CylLow, slotp->Prb.CylHigh);
		return ATA_DEV_NONE;
	}
	


>>>+static void sil24_irq_clear(struct ata_port *ap)
>>>+{
>>>+	/* unused */
>>>+}
>>
>>we need to fill this in.

I think this will work (adapted from sil_interrupt():

static void sil_irq_clear(struct ata_port *ap)
{
         struct sil_port_priv *pp = ap->private_data;
         struct Port_Registers *port_base = pp->pregs;
	unsigned long port_int;

	port_int  = readl((void *)&port_base->IntStatus);
	writel(port_int, &port_base->IntStatus);
}

I'm assuming that this entry point is expected to clear all interrupts, no?



>>>+	/* Max ~100ms */
>>>+	for (cnt = 0; cnt < 1000; cnt++) {
>>>+		udelay(100);
>>>+		tmp = readl(port + PORT_CTRL_STAT);
>>>+		if (!(tmp & PORT_CS_DEV_RST))
>>>+			break;
>>>+	}
>>
>>Use mdelay.  We should be able to sleep here?
>>
>>Either way, we want to avoid long delays like this.

Does mdelay() sleep?  I thought it just called udelay().

At any rate, I think 100ms is only the worst-case delay.

Is it safe to call msleep() at boot time?




>>>+	/* GPIO off */
>>>+	writel(0, host_base + HOST_FLASH_CMD);
>>>+
>>>+	/* Mask interrupts during initialization */
>>>+	writel(0, host_base + HOST_CTRL);
>>
>>add a readl() to flush this write out to the PCI bus ("PCI posting")
>>
> 
> 
>  Sure.  And, out of curiosity, isn't sync unnecessary unless we're
> gonna perform some kind of timed waiting following it?  We don't have
> any timing requirement after above interrupt masking, do we?


I think we're ok here; the code reads PORT_CTRL_STAT a few lines down; 
that will flush the write.  I don't think there's a race condition involved.





OK, a couple of questions of my own:

Any documentation on NCQ helpers or other related kernel code?

What's the proper way to implement very long delays.  I want to 
implement staggered disk spinup in my port multiplier code.  Would 
mdelay(5000) be terribly anti-social?  I'm guessing yes, but then, how 
do I ensure that no process tries to access the disk until I've spun it up?

Port multiplier spec says that the GSCR[2] register indicates how many 
ports the port multiplier supports.  On my 5-port device, this register 
reads back as six.  Are they counting the control port, or is this 
device defective?

What causes a disk to spin up?  Is it the COMRESET?  Soft reset?  In 
either case, it sounds like I need to spin up a drive just to detect it. 
  Not optimal.

	-ed falk

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Jeff Garzik]

Edward Falk wrote:
> Hi Tejun; I'm the guy at Google working on SATA drivers (port 
> multipliers right now).  As soon as I can (next week perhaps, I'll start 
> looking at the driver you wrote.  From what I can see, it looks quite good.
> 
> 
> 
>>>> +
>>>> +static u8 sil24_check_status(struct ata_port *ap)
>>>> +{
>>>> +    return ATA_DRDY;
>>>> +}
>>>> +
>>>> +static u8 sil24_check_err(struct ata_port *ap)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>
>>>
>>> For these two functions, we need to grab the values for these 
>>> registers from the D2H Register FIS.  These should not be constant 
>>> values, they are used in probing.
>>>
>>
>>  Sadly I don't know where the values are.  The original driver seems
>> to assume that taskfile registers are overlayed with PORT_PRB, but
>> they are not.  Values are bogus there.  Again, in need of hardware
>> docs here.
> 
> 
> The original driver is broken.  Taskfile registers have to be read back 
> from the returned FIS block after a command completion.

Correct.


> Hmmmm, perhaps libata should provide an ata_fis_to_tf() function that 
> examines a FIS block, confirms that it's a device-to-host type FIS, and 
> read the taskfile registers back out.

Such as ata_tf_from_fis() in libata-core.c? :)


>>  The original rewritten sil24 driver against NCQ helpers had simple
>> status register emulation using normal/error completion interrupt.  I
>> don't know why I stripped that while converting the driver over
>> vanilla libata (sorry).  I'll restore it.  It's not good, but it
>> correctly indicates error on error.
> 
> 
> It's still better than what we have.
> 
> 
> 
>>>> +static void sil24_phy_reset(struct ata_port *ap)
>>>> +{
>>>> +    __sata_phy_reset(ap);
>>>> +    /*
>>>> +     * No ATAPI yet.  Just unconditionally indicate ATA device.
>>>> +     * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
>>>> +     * and libata core will ignore the device.
>>>> +     */
>>>> +    if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
>>>> +        ap->device[0].class = ATA_DEV_ATA;
>>>> +}
>>>
>>>
>>> We need to use the standard probing code to figure this out. 
>>> ata_dev_classify(), etc.
>>>
>>
>>
>>  Again, the problem here is that I don't know how to access the
>> signature values after reset.
> 
> 
> Again, you would need to fetch them from the returned FIS structure. 

Correct.


>>>> +static void sil24_irq_clear(struct ata_port *ap)
>>>> +{
>>>> +    /* unused */
>>>> +}
>>>
>>>
>>> we need to fill this in.
> 
> 
> I think this will work (adapted from sil_interrupt():
> 
> static void sil_irq_clear(struct ata_port *ap)
> {
>         struct sil_port_priv *pp = ap->private_data;
>         struct Port_Registers *port_base = pp->pregs;
>     unsigned long port_int;
> 
>     port_int  = readl((void *)&port_base->IntStatus);
>     writel(port_int, &port_base->IntStatus);
> }
> 
> I'm assuming that this entry point is expected to clear all interrupts, no?

Correct.


>>>> +    /* Max ~100ms */
>>>> +    for (cnt = 0; cnt < 1000; cnt++) {
>>>> +        udelay(100);
>>>> +        tmp = readl(port + PORT_CTRL_STAT);
>>>> +        if (!(tmp & PORT_CS_DEV_RST))
>>>> +            break;
>>>> +    }
>>>
>>>
>>> Use mdelay.  We should be able to sleep here?
>>>
>>> Either way, we want to avoid long delays like this.
> 
> 
> Does mdelay() sleep?  I thought it just called udelay().

mdelay() does not sleep.


> At any rate, I think 100ms is only the worst-case delay.
> 
> Is it safe to call msleep() at boot time?

Yes.


>>>> +    /* GPIO off */
>>>> +    writel(0, host_base + HOST_FLASH_CMD);
>>>> +
>>>> +    /* Mask interrupts during initialization */
>>>> +    writel(0, host_base + HOST_CTRL);
>>>
>>>
>>> add a readl() to flush this write out to the PCI bus ("PCI posting")
>>>
>>
>>
>>  Sure.  And, out of curiosity, isn't sync unnecessary unless we're
>> gonna perform some kind of timed waiting following it?  We don't have
>> any timing requirement after above interrupt masking, do we?
> 
> 
> 
> I think we're ok here; the code reads PORT_CTRL_STAT a few lines down; 
> that will flush the write.  I don't think there's a race condition 
> involved.

No race condition.  Typically there is often a mistaken assumption that

	writel(...)
	udelay(...)

will accomplish the desired effect.  Due to posted writes, the write may 
be posted to the PCI device after some delay, such that, the udelay() 
and the posted write execute concurrently, skewing the desired timing 
effect.


> OK, a couple of questions of my own:
> 
> Any documentation on NCQ helpers or other related kernel code?
> 
> What's the proper way to implement very long delays.  I want to 
> implement staggered disk spinup in my port multiplier code.  Would 
> mdelay(5000) be terribly anti-social?  I'm guessing yes, but then, how 

Yes :)  msleep(5000) is far less anti-social, but still yucky...


> do I ensure that no process tries to access the disk until I've spun it up?

The probe code is typically serialized, to prevent problems like this. 
Specifically, you'll have to be aware of when libata adds devices to the 
SCSI layer.


> Port multiplier spec says that the GSCR[2] register indicates how many 
> ports the port multiplier supports.  On my 5-port device, this register 
> reads back as six.  Are they counting the control port, or is this 
> device defective?

Good question!  ;-)


> What causes a disk to spin up?  Is it the COMRESET?  Soft reset?  In 
> either case, it sounds like I need to spin up a drive just to detect it. 
>  Not optimal.

It depends.  COMRESET or power management can do it.  This stuff is 
documented in the SATA docs.  This is more on the device end than the 
host end, but in general you must be aware of
* host controller power state(s)
* SATA bus power state(s)
* SATA device power state(s)

	Jeff

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Tejun Heo]

 Greetings, Edward and Jeff.

 Jeff, as my previous patchset against sil24 driver hasn't been
accepted yet, and it seems that most of them can be done better with
info from Edward, please ignore the patchset.  I'll post a new
patchset tomorrow (hopefully).  Sorry about the hassle.

On Wed, Aug 03, 2005 at 12:09:05AM -0400, Jeff Garzik wrote:
> Edward Falk wrote:
> >Hi Tejun; I'm the guy at Google working on SATA drivers (port 
> >multipliers right now). 

 Great.

> > As soon as I can (next week perhaps, I'll start 
> >looking at the driver you wrote.  From what I can see, it looks quite good.
> >

 Thanks a lot.  I'll try to make my driver saner by then.

> >
> >
> >>>>+
> >>>>+static u8 sil24_check_status(struct ata_port *ap)
> >>>>+{
> >>>>+    return ATA_DRDY;
> >>>>+}
> >>>>+
> >>>>+static u8 sil24_check_err(struct ata_port *ap)
> >>>>+{
> >>>>+    return 0;
> >>>>+}
> >>>
> >>>
> >>>For these two functions, we need to grab the values for these 
> >>>registers from the D2H Register FIS.  These should not be constant 
> >>>values, they are used in probing.
> >>>
> >>
> >> Sadly I don't know where the values are.  The original driver seems
> >>to assume that taskfile registers are overlayed with PORT_PRB, but
> >>they are not.  Values are bogus there.  Again, in need of hardware
> >>docs here.
> >
> >
> >The original driver is broken.  Taskfile registers have to be read back 
> >from the returned FIS block after a command completion.
> 
> Correct.
> 

 Ahh.. Thanks a lot.  I thought PRB was used just for command issues.
I'll fix the driver ASAP.

> 
> >Hmmmm, perhaps libata should provide an ata_fis_to_tf() function that 
> >examines a FIS block, confirms that it's a device-to-host type FIS, and 
> >read the taskfile registers back out.
> 
> Such as ata_tf_from_fis() in libata-core.c? :)
> 
> 
> >> The original rewritten sil24 driver against NCQ helpers had simple
> >>status register emulation using normal/error completion interrupt.  I
> >>don't know why I stripped that while converting the driver over
> >>vanilla libata (sorry).  I'll restore it.  It's not good, but it
> >>correctly indicates error on error.
> >
> >
> >It's still better than what we have.
> >

 Hmmm.. Now that I know how to access the D2H FIS, I can properly
emulate status and error registers from interrupt handler.  I'll
submit a patch soon.

> >
> >
> >>>>+static void sil24_phy_reset(struct ata_port *ap)
> >>>>+{
> >>>>+    __sata_phy_reset(ap);
> >>>>+    /*
> >>>>+     * No ATAPI yet.  Just unconditionally indicate ATA device.
> >>>>+     * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
> >>>>+     * and libata core will ignore the device.
> >>>>+     */
> >>>>+    if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
> >>>>+        ap->device[0].class = ATA_DEV_ATA;
> >>>>+}
> >>>
> >>>
> >>>We need to use the standard probing code to figure this out. 
> >>>ata_dev_classify(), etc.
> >>>
> >>
> >>
> >> Again, the problem here is that I don't know how to access the
> >>signature values after reset.
> >
> >
> >Again, you would need to fetch them from the returned FIS structure. 
> 
> Correct.
> 

 Again, thanks.  ;-)

> 
> >>>>+static void sil24_irq_clear(struct ata_port *ap)
> >>>>+{
> >>>>+    /* unused */
> >>>>+}
> >>>
> >>>
> >>>we need to fill this in.
> >
> >
> >I think this will work (adapted from sil_interrupt():
> >
> >static void sil_irq_clear(struct ata_port *ap)
> >{
> >        struct sil_port_priv *pp = ap->private_data;
> >        struct Port_Registers *port_base = pp->pregs;
> >    unsigned long port_int;
> >
> >    port_int  = readl((void *)&port_base->IntStatus);
> >    writel(port_int, &port_base->IntStatus);
> >}
> >
> >I'm assuming that this entry point is expected to clear all interrupts, no?
> 
> Correct.
> 

 I'll verify with the error register clearing part of the original
driver and submit a patch.

> 
> >>>>+    /* Max ~100ms */
> >>>>+    for (cnt = 0; cnt < 1000; cnt++) {
> >>>>+        udelay(100);
> >>>>+        tmp = readl(port + PORT_CTRL_STAT);
> >>>>+        if (!(tmp & PORT_CS_DEV_RST))
> >>>>+            break;
> >>>>+    }
> >>>
> >>>
> >>>Use mdelay.  We should be able to sleep here?
> >>>
> >>>Either way, we want to avoid long delays like this.
> >
> >
> >Does mdelay() sleep?  I thought it just called udelay().
> 
> mdelay() does not sleep.
> 
> 
> >At any rate, I think 100ms is only the worst-case delay.
> >
> >Is it safe to call msleep() at boot time?
> 
> Yes.
> 
> 
> >>>>+    /* GPIO off */
> >>>>+    writel(0, host_base + HOST_FLASH_CMD);
> >>>>+
> >>>>+    /* Mask interrupts during initialization */
> >>>>+    writel(0, host_base + HOST_CTRL);
> >>>
> >>>
> >>>add a readl() to flush this write out to the PCI bus ("PCI posting")
> >>>
> >>
> >>
> >> Sure.  And, out of curiosity, isn't sync unnecessary unless we're
> >>gonna perform some kind of timed waiting following it?  We don't have
> >>any timing requirement after above interrupt masking, do we?
> >
> >
> >
> >I think we're ok here; the code reads PORT_CTRL_STAT a few lines down; 
> >that will flush the write.  I don't think there's a race condition 
> >involved.
> 
> No race condition.  Typically there is often a mistaken assumption that
> 
> 	writel(...)
> 	udelay(...)
> 
> will accomplish the desired effect.  Due to posted writes, the write may 
> be posted to the PCI device after some delay, such that, the udelay() 
> and the posted write execute concurrently, skewing the desired timing 
> effect.
> 

 So... we don't really need flushing here, right?  No delay
requirement here.

> 
> >OK, a couple of questions of my own:
> >
> >Any documentation on NCQ helpers or other related kernel code?
> >

 Not yet.  I'm waiting for Jeff's comment before converting all
drivers and possibly writing up a document.  If Jeff doesn't like it,
I'll have to tear it down and do it some other way, so you better not
invest too much into the helpers yet.  However, the usage is really
simple, just take a look at interrupt and error hanndlers of ahci.c
driver.  Other NCQ enabled drivers shouldn't be very different.

> >What's the proper way to implement very long delays.  I want to 
> >implement staggered disk spinup in my port multiplier code.  Would 
> >mdelay(5000) be terribly anti-social?  I'm guessing yes, but then, how 
> 
> Yes :)  msleep(5000) is far less anti-social, but still yucky...
> 

 Heh... but w/ staggered spin-up, I don't think we have many other
choices.  Maybe it would be nice if we can do multi-threaded device
driver initialization with checkpoints to order necessary stuff...
BTW, doesn't the BIOS spin up drives as in SCSI HBAs?

> 
> >do I ensure that no process tries to access the disk until I've spun it up?
> 
> The probe code is typically serialized, to prevent problems like this. 
> Specifically, you'll have to be aware of when libata adds devices to the 
> SCSI layer.
> 
> 
> >Port multiplier spec says that the GSCR[2] register indicates how many 
> >ports the port multiplier supports.  On my 5-port device, this register 
> >reads back as six.  Are they counting the control port, or is this 
> >device defective?
> 
> Good question!  ;-)
> 

 Hmmm... that field is 4-bit wide, so, if the count includes the
control port, the actual maximum number of supported ports would be 14
instead of 15 as specified in the spec.  I guess the device is faulty.
And my spec (rev 1.1) says GSCR[2] 3-0: Number of exposed device
fan-out ports.  That's pretty clear, I think.

> 
> >What causes a disk to spin up?  Is it the COMRESET?  Soft reset?  In 
> >either case, it sounds like I need to spin up a drive just to detect it. 
> > Not optimal.
> 
> It depends.  COMRESET or power management can do it.  This stuff is 
> documented in the SATA docs.  This is more on the device end than the 
> host end, but in general you must be aware of
> * host controller power state(s)
> * SATA bus power state(s)
> * SATA device power state(s)
> 
> 	Jeff
> 

 Thanks, Edward & Jeff.

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Tejun Heo]

 Hello, Edward.

 One more question.

> > >
> > >I think this will work (adapted from sil_interrupt():
> > >
> > >static void sil_irq_clear(struct ata_port *ap)
> > >{
> > >        struct sil_port_priv *pp = ap->private_data;
> > >        struct Port_Registers *port_base = pp->pregs;
> > >    unsigned long port_int;
> > >
> > >    port_int  = readl((void *)&port_base->IntStatus);
> > >    writel(port_int, &port_base->IntStatus);
> > >}
> > >
> > >I'm assuming that this entry point is expected to clear all interrupts, no?
> > 
> > Correct.
> > 
> 
>  I'll verify with the error register clearing part of the original
> driver and submit a patch.
> 

 Command completion interrupt is automatcally cleared by reading
PORT_SLOT_STAT register (SlotStatus in the original driver), and error
registers should be manually cleared by writing to PORT_IRQ_STAT
(IntStatus).

 I agree that above code should clear both.  Just wanna verify.  Have
you tested it and/or do you have any information confirming this?  If
we don't have any further info, I think we should read PORT_SLOT_STAT
before clearing PORT_IRQ_STAT to be on the safe side.

 Thanks.

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Edward Falk]

>  I agree that above code should clear both.  Just wanna verify.  Have
> you tested it and/or do you have any information confirming this?  If
> we don't have any further info, I think we should read PORT_SLOT_STAT
> before clearing PORT_IRQ_STAT to be on the safe side.

I've implemented the clear_irq() function to clear all interrupts as you 
said, but haven't had time to test this thoroughly.  Ultimately, we 
don't know if it works until we experience real errors and see how the 
system responds.

I don't think it matters in which order you do things, as long as you 
allow for the fact that reading PORT_SLOT_STAT will clear the command 
completion interrupt flag.

	-ed

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Tejun Heo]

 Hi, again, Edward.

 Bad news...

On Tue, Aug 02, 2005 at 03:56:05PM -0700, Edward Falk wrote:
> 
> Again, you would need to fetch them from the returned FIS structure. 
> Here's a code fragment derived from SiI's issue_soft_reset() function:
> 
> 	struct Port_Registers *port_base = yadayada;
> 	struct sil_port_priv *pp = ap->private_data;
> 	struct Port_Registers *PR;	/* in memory */
> 	dma_addr_t paddr = pp->pc_dma; /* physical address base */
> 
> 	PR = (struct Port_Registers *) (&pp->pc->mregs);
> 	port_base = yadayada;
> 	slot = 0;
> 	slotp = &PR->Slot[slot];
> 	memset(&slotp->Prb, 0, sizeof(slotp->Prb));
> 	slotp->Prb.Control = 0x80;		/* soft reset */
> 	slotp->Prb.FisType == 0;
> 	writel(paddr, &port_base->CmdActivate[slot].s.ActiveLow);
> 	if (!sil_wait_for_completion(port_base)) {
> 		/* timeout or error */
> 		return ATA_DEV_NONE;
> 	} else {
> 		/* Examine slot for taskfile registers */
> 		slotp = port_base->Slot[slot];
> 		if (slotp->Prb.FisType != 0x34 &&
> 		    slotp->Prb.FisType != 0x5F) {
> 			/* WTF?  Wrong FIS Type */
> 			return ATA_DEV_NONE;
> 		}
> 		if (slotp->Prb.CylLow == 0 &&
> 		    slotp->Prb.CylHigh == 0)
> 			return ATA_DEV_ATA;
> 		if (slotp->Prb.CylLow == 0x14 &&
> 		    slotp->Prb.CylHigh == 0xEB)
> 			return ATA_DEV_ATAPI;
> 		if (slotp->Prb.CylLow == 0x69 &&
> 		    slotp->Prb.CylHigh == 0x96)
> 			return ATA_DEV_PORT_MULTIPLIER;
> 		printk(KERN_WARN "unknown ATA device signature %x.%x\n",
> 			slotp->Prb.CylLow, slotp->Prb.CylHigh);
> 		return ATA_DEV_NONE;
> 	}

 Reading FIS off the cotroller's PRB doesn't seem to work.  I've added
the following code to the preview sil24 driver after completing soft
reset.

	writel((u32) paddr, &port_base->CmdActivate[0].s.ActiveLow);
	if (!sil_wait_for_completion(port_base)) {
		struct Port_Request_Block *prb = &port_base->Slot[0].Prb;
		printk("POSTRESET c:p=%04x:%04x rc=%08x\n"
		       "fis=%08x:%08x:%08x:%08x\n",
		       prb->Control, prb->Protocol, prb->ReceiveCount,
		       prb->AsDword_1, prb->AsDword_2,
		       prb->AsDword_3, prb->AsDword_4);
		return ATA_DEV_ATA; 
	} else	{
		 printk("sata_sil24: Port Not Ready status=%x\n",
				port_base->CtrlSetStatus);
		return ATA_DEV_NONE;
	}

 And, this is what I get.

sata_sil24: Issuing soft reset to phys=1f4f2000
POSTRESET c:p=0080:0000 rc=00000000
fis=01500000:00000001:00000000:00000001

 Which, apparantly, isn't a valid D2H FIS.

 Here are some more dumps of contoller's PRB from my sil24 driver.
PRE ISSUE is the values of PRB regs before issueing a command.  CMD is
the command to issue in the consistent memory.  POST ISSUE and POST
INTR are respectively after issueing a command and processing a
interrupt.

** POST RESET
c:p=ffff:ffff rc=00000200
fis=0050fc90:e0000000:00000000:00000000
** PRE ISSUE
c:p=ffff:ffff rc=00000200
fis=0050fc90:e0000000:00000000:00000000
** CMD
c:p=0000:0000 rc=00000000
fis=00ec8027:a0000000:00000000:08000000
** POST ISSUE
c:p=ffff:ffff rc=00000000
fis=1f57d000:00000000:1f57d000:00000000
** POST INTERRUPT
c:p=0200:0000 rc=00000200
fis=0050d000:a0000000:1f000000:000000ff
ata6: dev 0 ATA, max UDMA7, 312581808 sectors: lba48

 Is there something more to do to get received FIS from PRB?

 Thanks.

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Edward Falk]

Tejun Heo wrote:
>  Hi, again, Edward.
> 
>  Bad news...
> 
> On Tue, Aug 02, 2005 at 03:56:05PM -0700, Edward Falk wrote:
> 
>>Again, you would need to fetch them from the returned FIS structure. 
>>Here's a code fragment derived from SiI's issue_soft_reset() function:

Yeah, well, that's what I get for sending code fragments I haven't 
tested.  Stand by, I'll tinker with my own version of the driver and 
send you an improved fragment.

	-ed

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Tejun Heo]

Edward Falk wrote:
> Tejun Heo wrote:
> 
>>  Hi, again, Edward.
>>
>>  Bad news...
>>
>> On Tue, Aug 02, 2005 at 03:56:05PM -0700, Edward Falk wrote:
>>
>>> Again, you would need to fetch them from the returned FIS structure. 
>>> Here's a code fragment derived from SiI's issue_soft_reset() function:
> 
> 
> Yeah, well, that's what I get for sending code fragments I haven't 
> tested.  Stand by, I'll tinker with my own version of the driver and 
> send you an improved fragment.
> 
>     -ed

  One more thing I've found out.  They actually look like valid FIS 
values except for the FIS type.  I missed that as I was looking too hard 
at the FIS type value.

POSTRESET c:p=0080:0000 rc=00000000
fis=01500000:00000001:00000000:00000001
       ^    ^        ^                 ^
      DRDY  |      sect num        sect cnt
        FIS type 0?

  The following is from new driver w/ ATAPI device after SATA phy reset. 
(not softreset)

** POST RESET
c:p=0080:0000 rc=00000000
fis=01000027:00eb1401:00000000:00000001
            ^   ^^^^^^                 ^
       type 27? |----- valid sig ------|

  Also, after command completion, the status/error registers seem to 
carry the correct values.  The following is after issuing 
ATA_DEV_IDENTIFY to a ATAPI device.

** POST INTERRUPT
c:p=ffff:ffff rc=00000000
fis=0451e000:a0eb1400:1f000000:00000003
      ^ ^   ^
   ABRT |  FIS type 00 -_-;;
     DRDY/ERR

  So, it almost seems like I can just ignore the FIS type value. 
However, after a SATA phy reset, a hard disk returns the following.

** POST RESET
c:p=ffff:ffff rc=00000200
fis=0050fc90:e0000000:00000000:00000000

  This doesn't contain valid signature....

  I'll wait for further comment from you.  Thanks a lot.  ;-)

-- 
tejun

RE: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Carlos Pardo]

Jeff, et al -

One question. Do the open source driver support pass-through commands ? if so, how ? If you do not support it, are you planning to implement pass-through commands sometime in the future ?

Carlos

Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Jeff Garzik]

On Mon, Aug 08, 2005 at 10:31:45AM -0700, Carlos Pardo wrote:
> One question. Do the open source driver support pass-through commands ? if so, how ? If you do not support it, are you planning to implement pass-through commands sometime in the future ?

I don't know about sata_sil24 in particular.  In general, there is a
patch in the "libata-dev.git" repository that supports ATA passthru, and
this is scheduled to be merged into the main 2.6.x kernel as soon as
some bugs are fixed.

sata_sil24 definitely needs to support this ATA passthru feature.

As implemented, each SATA driver is simply a generic ATA command engine,
for any command.  struct ata_taskfile (linux/ata.h) has a field that
indicates the command protocol used for command delivery (pio, dma,
no-data, etc.).  Each driver should execute ATA commands based on the
specified ATA protocol.

	Jeff