linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ide: Add tx4939ide driver (v3)
@ 2008-10-02 15:08 Atsushi Nemoto
  2008-10-08 19:09 ` Bartlomiej Zolnierkiewicz
  2008-10-16 12:52 ` Sergei Shtylyov
  0 siblings, 2 replies; 6+ messages in thread
From: Atsushi Nemoto @ 2008-10-02 15:08 UTC (permalink / raw)
  To: linux-mips; +Cc: linux-ide, Bartlomiej Zolnierkiewicz, ralf, sshtylyov

This is the driver for the Toshiba TX4939 SoC ATA controller.

This controller has standard ATA taskfile registers and DMA
command/status registers, but the register layout is swapped on big
endian.  There are some other endian issue and some special registers
which requires many custom dma_ops/tp_ops routines and build_dmatable.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
This patch is against linux-next 20080919.

Changes since v2:
* more consistent symbol naming
* drop custom selectproc
* more reasonable delay values
* custom ide_build_dmatable for big endian
* cleanup irq handling
* use ide_host_alloc/ide_host_register instead of ide_host_alloc
* drop custom init_iops
* and so on  (Many many thanks to Sergei)

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
 drivers/ide/Kconfig          |    6 +
 drivers/ide/mips/Makefile    |    1 +
 drivers/ide/mips/tx4939ide.c |  775 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 782 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ide/mips/tx4939ide.c

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index 325461c..0ed731a 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -736,6 +736,12 @@ config BLK_DEV_IDE_AU1XXX_SEQTS_PER_RQ
        default "128"
        depends on BLK_DEV_IDE_AU1XXX
 
+config BLK_DEV_IDE_TX4939
+	tristate "TX4939 internal IDE support"
+	depends on SOC_TX4939
+	select BLK_DEV_IDEDMA_SFF
+	select IDE_TIMINGS
+
 config IDE_ARM
 	tristate "ARM IDE support"
 	depends on ARM && (ARCH_CLPS7500 || ARCH_RPC || ARCH_SHARK)
diff --git a/drivers/ide/mips/Makefile b/drivers/ide/mips/Makefile
index 677c7b2..1e0ad98 100644
--- a/drivers/ide/mips/Makefile
+++ b/drivers/ide/mips/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_BLK_DEV_IDE_SWARM)		+= swarm.o
 obj-$(CONFIG_BLK_DEV_IDE_AU1XXX)	+= au1xxx-ide.o
+obj-$(CONFIG_BLK_DEV_IDE_TX4939)	+= tx4939ide.o
 
 EXTRA_CFLAGS    := -Idrivers/ide
diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
new file mode 100644
index 0000000..6671d64
--- /dev/null
+++ b/drivers/ide/mips/tx4939ide.c
@@ -0,0 +1,775 @@
+/*
+ * TX4939 internal IDE driver
+ * Based on RBTX49xx patch from CELF patch archive.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * (C) Copyright TOSHIBA CORPORATION 2005-2007
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/ide.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/scatterlist.h>
+
+#define MODNAME	"tx4939ide"
+
+/* ATA Shadow Registers (8-bit except for DATA which is 16-bit) */
+#define TX4939IDE_DATA			0x000
+#define TX4939IDE_Error_Feature		0x001
+#define TX4939IDE_Sec			0x002
+#define TX4939IDE_LBA0			0x003
+#define TX4939IDE_LBA1			0x004
+#define TX4939IDE_LBA2			0x005
+#define TX4939IDE_DevHead		0x006
+#define TX4939IDE_Stat_Cmd		0x007
+#define TX4939IDE_AltStat_DevCtl	0x402
+/* H/W DMA Registers  */
+#define TX4939IDE_DMA_Cmd	0x800	/* 8-bit */
+#define TX4939IDE_DMA_stat	0x802	/* 8-bit */
+#define TX4939IDE_PRD_Ptr	0x804	/* 32-bit */
+/* ATA100 CORE Registers (16-bit) */
+#define TX4939IDE_Sys_Ctl	0xc00
+#define TX4939IDE_Xfer_Cnt_1	0xc08
+#define TX4939IDE_Xfer_Cnt_2	0xc0a
+#define TX4939IDE_Sec_Cnt	0xc10
+#define TX4939IDE_Start_Lo_Addr	0xc18
+#define TX4939IDE_Start_Up_Addr	0xc20
+#define TX4939IDE_Add_Ctl	0xc28
+#define TX4939IDE_Lo_Burst_Cnt	0xc30
+#define TX4939IDE_Up_Burst_Cnt	0xc38
+#define TX4939IDE_PIO_Addr	0xc88
+#define TX4939IDE_H_Rst_Tim	0xc90
+#define TX4939IDE_Int_Ctl	0xc98
+#define TX4939IDE_Pkt_Cmd	0xcb8
+#define TX4939IDE_Bxfer_Cnt_Hi	0xcc0
+#define TX4939IDE_Bxfer_Cnt_Lo	0xcc8
+#define TX4939IDE_Dev_TErr	0xcd0
+#define TX4939IDE_Pkt_Xfer_Ctl	0xcd8
+#define TX4939IDE_Start_TAddr	0xce0
+
+/* bits for Int_Ctl */
+#define TX4939IDE_INT_ADDRERR	0x80
+#define TX4939IDE_INT_REACHMUL	0x40
+#define TX4939IDE_INT_DEVTIMING	0x20
+#define TX4939IDE_INT_UDMATERM	0x10
+#define TX4939IDE_INT_TIMER	0x08
+#define TX4939IDE_INT_BUSERR	0x04
+#define TX4939IDE_INT_XFEREND	0x02
+#define TX4939IDE_INT_HOST	0x01
+
+#define TX4939IDE_IGNORE_INTS	\
+	(TX4939IDE_INT_ADDRERR | TX4939IDE_INT_REACHMUL | \
+	 TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_UDMATERM | \
+	 TX4939IDE_INT_TIMER | TX4939IDE_INT_XFEREND)
+
+#ifdef __BIG_ENDIAN
+#define tx4939ide_swizzlel(a)	((a) ^ 4)
+#define tx4939ide_swizzlew(a)	((a) ^ 6)
+#define tx4939ide_swizzleb(a)	((a) ^ 7)
+#else
+#define tx4939ide_swizzlel(a)	(a)
+#define tx4939ide_swizzlew(a)	(a)
+#define tx4939ide_swizzleb(a)	(a)
+#endif
+
+static u16 tx4939ide_readw(void __iomem *base, u32 reg)
+{
+	return __raw_readw(base + tx4939ide_swizzlew(reg));
+}
+static u8 tx4939ide_readb(void __iomem *base, u32 reg)
+{
+	return __raw_readb(base + tx4939ide_swizzleb(reg));
+}
+static void tx4939ide_writel(u32 val, void __iomem *base, u32 reg)
+{
+	__raw_writel(val, base + tx4939ide_swizzlel(reg));
+}
+static void tx4939ide_writew(u16 val, void __iomem *base, u32 reg)
+{
+	__raw_writew(val, base + tx4939ide_swizzlew(reg));
+}
+static void tx4939ide_writeb(u8 val, void __iomem *base, u32 reg)
+{
+	__raw_writeb(val, base + tx4939ide_swizzleb(reg));
+}
+
+#define TX4939IDE_BASE(hwif)	((void __iomem *)(hwif)->extra_base)
+
+static void tx4939ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	int is_slave = drive->dn & 1;
+	u32 mask, val;
+	u8 safe = pio;
+	ide_drive_t *pair;
+
+	pair = ide_get_pair_dev(drive);
+	if (pair)
+		safe = min(safe, ide_get_best_pio_mode(pair, 255, 4));
+	/*
+	 * Update Command Transfer Mode for master/slave and Data
+	 * Transfer Mode for this drive.
+	 */
+	mask = is_slave ? 0x07f00000 : 0x000007f0;
+	val = ((safe << 8) | (pio << 4)) << (is_slave ? 16 : 0);
+	hwif->select_data = (hwif->select_data & ~mask) | val;
+	/* tx4939ide_tf_load_fixup() will set the Sys_Ctl register */
+}
+
+static void tx4939ide_set_dma_mode(ide_drive_t *drive, const u8 mode)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	u32 mask, val;
+
+	/* Update Data Transfer Mode for this drive. */
+	if (mode >= XFER_UDMA_0)
+		val = mode - XFER_UDMA_0 + 8;
+	else if (mode >= XFER_MW_DMA_0)
+		val = mode - XFER_MW_DMA_0 + 5;
+	else
+		val = mode - XFER_PIO_0;
+	if (drive->dn & 1) {
+		mask = 0x00f00000;
+		val <<= 20;
+	} else {
+		mask = 0x000000f0;
+		val <<= 4;
+	}
+	hwif->select_data = (hwif->select_data & ~mask) | val;
+	/* tx4939ide_tf_load_fixup() will set the Sys_Ctl register */
+}
+
+static u16 tx4939ide_check_error_ints(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u16 ctl = tx4939ide_readw(base, TX4939IDE_Int_Ctl);
+
+	if (ctl & TX4939IDE_INT_BUSERR) {
+		/* reset FIFO */
+		u16 sysctl = tx4939ide_readw(base, TX4939IDE_Sys_Ctl);
+		tx4939ide_writew(sysctl | 0x4000, base, TX4939IDE_Sys_Ctl);
+		mmiowb();
+		/* wait 12GBUSCLK (typ. 60ns @ GBUS200MHz, max 270ns) */
+		ndelay(270);
+		tx4939ide_writew(sysctl, base, TX4939IDE_Sys_Ctl);
+	}
+	if (ctl & (TX4939IDE_INT_ADDRERR |
+		    TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_BUSERR))
+		pr_err("%s: Error interrupt %#x (%s%s%s )\n",
+		       hwif->name, ctl,
+		       (ctl & TX4939IDE_INT_ADDRERR) ?
+		       " Address-Error" : "",
+		       (ctl & TX4939IDE_INT_DEVTIMING) ?
+		       " DEV-Timing" : "",
+		       (ctl & TX4939IDE_INT_BUSERR) ?
+		       " Bus-Error" : "");
+	return ctl;
+}
+
+static void tx4939ide_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif;
+	void __iomem *base;
+	u16 ctl;
+
+	/*
+	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
+	 * jobs for DMA case.
+	 */
+	if (drive->waiting_for_dma)
+		return;
+	hwif = drive->hwif;
+	base = TX4939IDE_BASE(hwif);
+	ctl = tx4939ide_check_error_ints(hwif);
+	tx4939ide_writew(ctl, base, TX4939IDE_Int_Ctl);
+}
+
+static u8 tx4939ide_cable_detect(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+
+	return (tx4939ide_readw(base, TX4939IDE_Sys_Ctl) & 0x2000) ?
+		ATA_CBL_PATA40 : ATA_CBL_PATA80;
+}
+
+#ifdef __BIG_ENDIAN
+static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
+{
+	ide_hwif_t *hwif	= drive->hwif;
+	u8 unit			= drive->dn & 1;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u8 dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
+
+	if (on)
+		dma_stat |= (1 << (5 + unit));
+	else
+		dma_stat &= ~(1 << (5 + unit));
+
+	tx4939ide_writeb(dma_stat, base, TX4939IDE_DMA_stat);
+}
+#else
+#define tx4939ide_dma_host_set	ide_dma_host_set
+#endif
+
+static u8 tx4939ide_read_and_clear_dma_status(void __iomem *base)
+{
+	u8 dma_stat;
+
+	/* read DMA status for INTR & ERROR flags */
+	dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
+	/* clear INTR & ERROR flags */
+	tx4939ide_writeb(dma_stat | 6, base, TX4939IDE_DMA_stat);
+	/* recover intmask cleared by writing to bit2 of DMA_stat */
+	tx4939ide_writew(TX4939IDE_IGNORE_INTS << 8, base, TX4939IDE_Int_Ctl);
+	return dma_stat;
+}
+
+#ifdef __BIG_ENDIAN
+/* custom ide_build_dmatable to handle swapped layout */
+static int tx4939ide_build_dmatable(ide_drive_t *drive, struct request *rq)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	u32 *table = (u32 *)hwif->dmatable_cpu;
+	unsigned int count = 0;
+	int i;
+	struct scatterlist *sg;
+
+	hwif->sg_nents = ide_build_sglist(drive, rq);
+	if (hwif->sg_nents == 0)
+		return 0;
+
+	for_each_sg(hwif->sg_table, sg, hwif->sg_nents, i) {
+		u32 cur_addr, cur_len, xcount, bcount;
+
+		cur_addr = sg_dma_address(sg);
+		cur_len = sg_dma_len(sg);
+
+		/*
+		 * Fill in the dma table, without crossing any 64kB boundaries.
+		 */
+
+		while (cur_len) {
+			if (count++ >= PRD_ENTRIES)
+				goto use_pio_instead;
+
+			bcount = 0x10000 - (cur_addr & 0xffff);
+			if (bcount > cur_len)
+				bcount = cur_len;
+			xcount = bcount & 0xffff;
+			if (xcount == 0x0000) {
+				if (count++ >= PRD_ENTRIES)
+					goto use_pio_instead;
+				*table++ = 0x8000;
+				*table++ = cur_addr;
+				*table++ = 0x8000;
+				*table++ = cur_addr + 0x8000;
+			} else {
+				*table++ = xcount;
+				*table++ = cur_addr;
+			}
+			cur_addr += bcount;
+			cur_len -= bcount;
+		}
+	}
+
+	if (count) {
+		*(table - 2) |= 0x80000000;
+		return count;
+	}
+
+use_pio_instead:
+	printk(KERN_ERR "%s: %s\n", drive->name,
+		count ? "DMA table too small" : "empty DMA table?");
+
+	ide_destroy_dmatable(drive);
+
+	return 0; /* revert to PIO for this request */
+}
+#else
+#define tx4939ide_build_dmatable	ide_build_dmatable
+#endif
+
+static int tx4939ide_dma_setup(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	struct request *rq = hwif->hwgroup->rq;
+	unsigned int reading;
+	int nent;
+
+	if (rq_data_dir(rq))
+		reading = 0;
+	else
+		reading = 1 << 3;
+
+	/* fall back to pio! */
+	nent = tx4939ide_build_dmatable(drive, rq);
+	if (!nent) {
+		ide_map_sg(drive, rq);
+		return 1;
+	}
+
+	/* PRD table */
+	tx4939ide_writel(hwif->dmatable_dma, base, TX4939IDE_PRD_Ptr);
+
+	/* specify r/w */
+	tx4939ide_writeb(reading, base, TX4939IDE_DMA_Cmd);
+
+	/* clear INTR & ERROR flags */
+	tx4939ide_read_and_clear_dma_status(base);
+
+	drive->waiting_for_dma = 1;
+
+	tx4939ide_writew(SECTOR_SIZE / 2, base, (drive->dn & 1) ?
+			 TX4939IDE_Xfer_Cnt_2 : TX4939IDE_Xfer_Cnt_1);
+	tx4939ide_writew(rq->nr_sectors, base, TX4939IDE_Sec_Cnt);
+	return 0;
+}
+
+static int tx4939ide_dma_end(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	u8 dma_stat, dma_cmd;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u16 ctl = tx4939ide_readw(base, TX4939IDE_Int_Ctl);
+
+	drive->waiting_for_dma = 0;
+
+	/* get DMA command mode */
+	dma_cmd = tx4939ide_readb(base, TX4939IDE_DMA_Cmd);
+	/* stop DMA */
+	tx4939ide_writeb(dma_cmd & ~1, base, TX4939IDE_DMA_Cmd);
+
+	/* read and clear the INTR & ERROR bits */
+	dma_stat = tx4939ide_read_and_clear_dma_status(base);
+
+	/* purge DMA mappings */
+	ide_destroy_dmatable(drive);
+	/* verify good DMA status */
+	wmb();
+
+	if ((dma_stat & 7) == 0 &&
+	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
+	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
+		/* INT_IDE lost... bug? */
+		return 0;
+	return (dma_stat & 7) != 4 ? (0x10 | dma_stat) : 0;
+}
+
+/* returns 1 if dma irq issued, 0 otherwise */
+static int tx4939ide_dma_test_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u16 ctl;
+	u8 dma_stat, stat;
+	u16 ide_int;
+	int found = 0;
+
+	ctl = tx4939ide_check_error_ints(hwif);
+	ide_int = ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST);
+	switch (ide_int) {
+	case TX4939IDE_INT_HOST:
+		/* On error, XFEREND might not be asserted. */
+		stat = tx4939ide_readb(base, TX4939IDE_AltStat_DevCtl);
+		if ((stat & (ATA_BUSY|ATA_DRQ|ATA_ERR)) == ATA_ERR)
+			found = 1;
+		else {
+			/* Wait for XFERINT (Mask HOST and unmask XFERINT) */
+			ctl &= ~TX4939IDE_INT_XFEREND << 8;
+			ctl |= TX4939IDE_INT_HOST << 8;
+		}
+		ctl |= ide_int << 8;
+		break;
+	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
+		dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
+		if (!(dma_stat & 4))
+			pr_warning("%s: weird interrupt status. "
+				   "DMA_stat %#02x int_ctl %#04x\n",
+				   hwif->name, dma_stat, ctl);
+		found = 1;
+		break;
+	}
+	/*
+	 * Do not clear XFERINT, HOST now.  They will be cleared by
+	 * clearing bit2 of DMA_stat.
+	 */
+	ctl &= ~ide_int;
+	tx4939ide_writew(ctl, base, TX4939IDE_Int_Ctl);
+	return found;
+}
+
+static void tx4939ide_init_hwif(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+
+	/* Soft Reset */
+	tx4939ide_writew(0x8000, base, TX4939IDE_Sys_Ctl);
+	mmiowb();
+	/* at least 20 UPSCLK (typ. 100ns @ GBUS200MHz, max 450ns) */
+	ndelay(450);
+	tx4939ide_writew(0x0000, base, TX4939IDE_Sys_Ctl);
+	/* mask some interrupts and clear all interrupts */
+	tx4939ide_writew((TX4939IDE_IGNORE_INTS << 8) | 0xff, base,
+			 TX4939IDE_Int_Ctl);
+
+	tx4939ide_writew(0x0008, base, TX4939IDE_Lo_Burst_Cnt);
+	tx4939ide_writew(0, base, TX4939IDE_Up_Burst_Cnt);
+}
+
+static int tx4939ide_init_dma(ide_hwif_t *hwif, const struct ide_port_info *d)
+{
+	hwif->dma_base = (unsigned long)TX4939IDE_BASE(hwif) +
+		tx4939ide_swizzleb(TX4939IDE_DMA_Cmd);
+	/*
+	 * Note that we cannot use ATA_DMA_TABLE_OFS,ATA_DMA_STATUS
+	 * for big endian.
+	 */
+	return ide_allocate_dma_engine(hwif);
+}
+
+static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	return tx4939ide_readb(base, TX4939IDE_DMA_stat);
+}
+
+static void tx4939ide_tf_load_fixup(ide_drive_t *drive, ide_task_t *task)
+{
+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE) {
+		ide_hwif_t *hwif = drive->hwif;
+		void __iomem *base = TX4939IDE_BASE(hwif);
+		u16 sysctl = hwif->select_data >> ((drive->dn & 1) ? 16 : 0);
+
+		/*
+		 * Fix ATA100 CORE System Control Register. (The write
+		 * to the Device/Head register may write wrong data to
+		 * the System Control Register)
+		 * While Sys_Ctl is written here, selectproc is not needed.
+		 */
+		tx4939ide_writew(sysctl, base, TX4939IDE_Sys_Ctl);
+	}
+}
+
+#ifdef __BIG_ENDIAN
+
+/* custom iops (independent from SWAP_IO_SPACE) */
+static u8 tx4939ide_inb(unsigned long port)
+{
+	return (u8)__raw_readb((void __iomem *)port);
+}
+
+static void tx4939ide_outb(u8 value, unsigned long port)
+{
+	__raw_writeb(value, (void __iomem *)port);
+}
+
+static void tx4939ide_tf_load(ide_drive_t *drive, ide_task_t *task)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct ide_io_ports *io_ports = &hwif->io_ports;
+	struct ide_taskfile *tf = &task->tf;
+	u8 HIHI = (task->tf_flags & IDE_TFLAG_LBA48) ? 0xE0 : 0xEF;
+
+	if (task->tf_flags & IDE_TFLAG_FLAGGED)
+		HIHI = 0xFF;
+
+	if (task->tf_flags & IDE_TFLAG_OUT_DATA) {
+		u16 data = (tf->hob_data << 8) | tf->data;
+
+		/* no endian swap */
+		__raw_writew(data, (void __iomem *)io_ports->data_addr);
+	}
+
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_FEATURE)
+		tx4939ide_outb(tf->hob_feature, io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_NSECT)
+		tx4939ide_outb(tf->hob_nsect, io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAL)
+		tx4939ide_outb(tf->hob_lbal, io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAM)
+		tx4939ide_outb(tf->hob_lbam, io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAH)
+		tx4939ide_outb(tf->hob_lbah, io_ports->lbah_addr);
+
+	if (task->tf_flags & IDE_TFLAG_OUT_FEATURE)
+		tx4939ide_outb(tf->feature, io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_NSECT)
+		tx4939ide_outb(tf->nsect, io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAL)
+		tx4939ide_outb(tf->lbal, io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAM)
+		tx4939ide_outb(tf->lbam, io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAH)
+		tx4939ide_outb(tf->lbah, io_ports->lbah_addr);
+
+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE)
+		tx4939ide_outb((tf->device & HIHI) | drive->select,
+			       io_ports->device_addr);
+
+	tx4939ide_tf_load_fixup(drive, task);
+}
+
+static void tx4939ide_tf_read(ide_drive_t *drive, ide_task_t *task)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct ide_io_ports *io_ports = &hwif->io_ports;
+	struct ide_taskfile *tf = &task->tf;
+
+	if (task->tf_flags & IDE_TFLAG_IN_DATA) {
+		u16 data;
+
+		/* no endian swap */
+		data = __raw_readw((void __iomem *)io_ports->data_addr);
+		tf->data = data & 0xff;
+		tf->hob_data = (data >> 8) & 0xff;
+	}
+
+	/* be sure we're looking at the low order bits */
+	tx4939ide_outb(ATA_DEVCTL_OBS & ~0x80, io_ports->ctl_addr);
+
+	if (task->tf_flags & IDE_TFLAG_IN_FEATURE)
+		tf->feature = tx4939ide_inb(io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_NSECT)
+		tf->nsect  = tx4939ide_inb(io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAL)
+		tf->lbal   = tx4939ide_inb(io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAM)
+		tf->lbam   = tx4939ide_inb(io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAH)
+		tf->lbah   = tx4939ide_inb(io_ports->lbah_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_DEVICE)
+		tf->device = tx4939ide_inb(io_ports->device_addr);
+
+	if (task->tf_flags & IDE_TFLAG_LBA48) {
+		tx4939ide_outb(ATA_DEVCTL_OBS | 0x80, io_ports->ctl_addr);
+
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_FEATURE)
+			tf->hob_feature =
+				tx4939ide_inb(io_ports->feature_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_NSECT)
+			tf->hob_nsect   = tx4939ide_inb(io_ports->nsect_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAL)
+			tf->hob_lbal    = tx4939ide_inb(io_ports->lbal_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAM)
+			tf->hob_lbam    = tx4939ide_inb(io_ports->lbam_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAH)
+			tf->hob_lbah    = tx4939ide_inb(io_ports->lbah_addr);
+	}
+}
+
+static void tx4939ide_insw_swap(unsigned long port, void *addr, u32 count)
+{
+	unsigned short *ptr = addr;
+	unsigned long size = count * 2;
+	while (count--)
+		*ptr++ = cpu_to_le16(__raw_readw((void __iomem *)port));
+	__ide_flush_dcache_range((unsigned long)addr, size);
+}
+
+static void tx4939ide_outsw_swap(unsigned long port, void *addr, u32 count)
+{
+	unsigned short *ptr = addr;
+	unsigned long size = count * 2;
+	while (count--) {
+		__raw_writew(le16_to_cpu(*ptr), (void __iomem *)port);
+		ptr++;
+	}
+	__ide_flush_dcache_range((unsigned long)addr, size);
+}
+
+static void tx4939ide_input_data_swap(ide_drive_t *drive, struct request *rq,
+				 void *buf, unsigned int len)
+{
+	tx4939ide_insw_swap(drive->hwif->io_ports.data_addr,
+			    buf, (len + 1) / 2);
+}
+
+static void tx4939ide_output_data_swap(ide_drive_t *drive, struct request *rq,
+				  void *buf, unsigned int len)
+{
+	tx4939ide_outsw_swap(drive->hwif->io_ports.data_addr,
+			     buf, (len + 1) / 2);
+}
+
+static const struct ide_tp_ops tx4939ide_tp_ops = {
+	.exec_command		= ide_exec_command,
+	.read_status		= ide_read_status,
+	.read_altstatus		= ide_read_altstatus,
+	.read_sff_dma_status	= tx4939ide_read_sff_dma_status,
+
+	.set_irq		= ide_set_irq,
+
+	.tf_load		= tx4939ide_tf_load,
+	.tf_read		= tx4939ide_tf_read,
+
+	.input_data		= tx4939ide_input_data_swap,
+	.output_data		= tx4939ide_output_data_swap,
+};
+
+#else	/* __LITTLE_ENDIAN */
+
+static void tx4939ide_tf_load(ide_drive_t *drive, ide_task_t *task)
+{
+	ide_tf_load(drive, task);
+	tx4939ide_tf_load_fixup(drive, task);
+}
+
+static const struct ide_tp_ops tx4939ide_tp_ops = {
+	.exec_command		= ide_exec_command,
+	.read_status		= ide_read_status,
+	.read_altstatus		= ide_read_altstatus,
+	.read_sff_dma_status	= tx4939ide_read_sff_dma_status,
+
+	.set_irq		= ide_set_irq,
+
+	.tf_load		= tx4939ide_tf_load,
+	.tf_read		= ide_tf_read,
+
+	.input_data		= ide_input_data,
+	.output_data		= ide_output_data,
+};
+
+#endif	/* __LITTLE_ENDIAN */
+
+static const struct ide_port_ops tx4939ide_port_ops = {
+	.set_pio_mode = tx4939ide_set_pio_mode,
+	.set_dma_mode = tx4939ide_set_dma_mode,
+	.clear_irq = tx4939ide_clear_irq,
+	.cable_detect = tx4939ide_cable_detect,
+};
+
+static const struct ide_dma_ops tx4939ide_dma_ops = {
+	.dma_host_set = tx4939ide_dma_host_set,
+	.dma_setup = tx4939ide_dma_setup,
+	.dma_exec_cmd = ide_dma_exec_cmd,
+	.dma_start = ide_dma_start,
+	.dma_end = tx4939ide_dma_end,
+	.dma_test_irq = tx4939ide_dma_test_irq,
+	.dma_lost_irq = ide_dma_lost_irq,
+	.dma_timeout = ide_dma_timeout,
+};
+
+static const struct ide_port_info tx4939ide_port_info __initdata = {
+	.init_hwif = tx4939ide_init_hwif,
+	.init_dma = tx4939ide_init_dma,
+	.port_ops = &tx4939ide_port_ops,
+	.dma_ops = &tx4939ide_dma_ops,
+	.tp_ops = &tx4939ide_tp_ops,
+	.host_flags = IDE_HFLAG_MMIO,
+	.pio_mask = ATA_PIO4,
+	.mwdma_mask = ATA_MWDMA2,
+	.udma_mask = ATA_UDMA5,
+};
+
+static int __init tx4939ide_probe(struct platform_device *pdev)
+{
+	hw_regs_t hw;
+	hw_regs_t *hws[] = { &hw, NULL, NULL, NULL };
+	struct ide_host *host;
+	struct resource *res;
+	int irq;
+	unsigned long mapbase;
+	int ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return -ENODEV;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	mapbase = (unsigned long)devm_ioremap(&pdev->dev, res->start,
+					      res->end - res->start + 1);
+	if (!mapbase)
+		return -EBUSY;
+	memset(&hw, 0, sizeof(hw));
+	hw.io_ports.data_addr =
+		mapbase + tx4939ide_swizzlew(TX4939IDE_DATA);
+	hw.io_ports.error_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_Error_Feature);
+	hw.io_ports.nsect_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_Sec);
+	hw.io_ports.lbal_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_LBA0);
+	hw.io_ports.lbam_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_LBA1);
+	hw.io_ports.lbah_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_LBA2);
+	hw.io_ports.device_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_DevHead);
+	hw.io_ports.command_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_Stat_Cmd);
+	hw.io_ports.ctl_addr =
+		mapbase + tx4939ide_swizzleb(TX4939IDE_AltStat_DevCtl);
+	hw.irq = irq;
+	hw.dev = &pdev->dev;
+
+	pr_info("TX4939 IDE interface (%lx,%d)\n", mapbase, irq);
+	host = ide_host_alloc(&tx4939ide_port_info, hws);
+	if (!host)
+		return -ENOMEM;
+	/* use extra_base for base address of the all registers */
+	host->ports[0]->extra_base = mapbase;
+	ret = ide_host_register(host, &tx4939ide_port_info, hws);
+	if (ret) {
+		ide_host_free(host);
+		return ret;
+	}
+	platform_set_drvdata(pdev, host);
+	return 0;
+}
+
+static int __exit tx4939ide_remove(struct platform_device *pdev)
+{
+	struct ide_host *host = platform_get_drvdata(pdev);
+
+	ide_host_remove(host);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tx4939ide_resume(struct platform_device *dev)
+{
+	struct ide_host *host = platform_get_drvdata(dev);
+	ide_hwif_t *hwif = host->ports[0];
+
+	tx4939ide_init_hwif(hwif);
+	return 0;
+}
+#else
+#define tx4939ide_resume	NULL
+#endif
+
+static struct platform_driver tx4939ide_driver = {
+	.driver = {
+		.name = MODNAME,
+		.owner = THIS_MODULE,
+	},
+	.remove = __exit_p(tx4939ide_remove),
+	.resume = tx4939ide_resume,
+};
+
+static int __init tx4939ide_init(void)
+{
+	return platform_driver_probe(&tx4939ide_driver, tx4939ide_probe);
+}
+
+static void __exit tx4939ide_exit(void)
+{
+	platform_driver_unregister(&tx4939ide_driver);
+}
+
+module_init(tx4939ide_init);
+module_exit(tx4939ide_exit);
+
+MODULE_DESCRIPTION("TX4939 internal IDE driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:tx4939ide");
-- 
1.5.6.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ide: Add tx4939ide driver (v3)
  2008-10-02 15:08 [PATCH] ide: Add tx4939ide driver (v3) Atsushi Nemoto
@ 2008-10-08 19:09 ` Bartlomiej Zolnierkiewicz
  2008-10-16 12:52 ` Sergei Shtylyov
  1 sibling, 0 replies; 6+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2008-10-08 19:09 UTC (permalink / raw)
  To: Atsushi Nemoto; +Cc: linux-mips, linux-ide, ralf, sshtylyov

On Thursday 02 October 2008, Atsushi Nemoto wrote:
> This is the driver for the Toshiba TX4939 SoC ATA controller.
> 
> This controller has standard ATA taskfile registers and DMA
> command/status registers, but the register layout is swapped on big
> endian.  There are some other endian issue and some special registers
> which requires many custom dma_ops/tp_ops routines and build_dmatable.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
> This patch is against linux-next 20080919.
> 
> Changes since v2:
> * more consistent symbol naming
> * drop custom selectproc
> * more reasonable delay values
> * custom ide_build_dmatable for big endian
> * cleanup irq handling
> * use ide_host_alloc/ide_host_register instead of ide_host_alloc
> * drop custom init_iops
> * and so on  (Many many thanks to Sergei)

I think that Sergei is still on vacation so it may take a while to
get his feedback.

[ in the meantime I replaced v2 by v3 in pata tree ]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ide: Add tx4939ide driver (v3)
  2008-10-02 15:08 [PATCH] ide: Add tx4939ide driver (v3) Atsushi Nemoto
  2008-10-08 19:09 ` Bartlomiej Zolnierkiewicz
@ 2008-10-16 12:52 ` Sergei Shtylyov
  2008-10-16 16:31   ` Atsushi Nemoto
  1 sibling, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2008-10-16 12:52 UTC (permalink / raw)
  To: Atsushi Nemoto; +Cc: linux-mips, linux-ide, Bartlomiej Zolnierkiewicz, ralf

Hello.

Atsushi Nemoto wrote:

> This is the driver for the Toshiba TX4939 SoC ATA controller.
>
> This controller has standard ATA taskfile registers and DMA
> command/status registers, but the register layout is swapped on big
> endian.  There are some other endian issue and some special registers
> which requires many custom dma_ops/tp_ops routines and build_dmatable.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

   Mostly ACK but there's still a few minor nits...

> diff --git a/drivers/ide/mips/Makefile b/drivers/ide/mips/Makefile
> index 677c7b2..1e0ad98 100644
> --- a/drivers/ide/mips/Makefile
> +++ b/drivers/ide/mips/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_BLK_DEV_IDE_SWARM)		+= swarm.o
>   

  The context have changed here but I guess Bart handled that...

> diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
> new file mode 100644
> index 0000000..6671d64
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,775 @@
>   
[...]
> +/* ATA Shadow Registers (8-bit except for DATA which is 16-bit) */
> +#define TX4939IDE_DATA			0x000
>   

   Not sure whether the data register deserves more respect than the 
others. :-)

> +/* H/W DMA Registers  */
> +#define TX4939IDE_DMA_Cmd	0x800	/* 8-bit */
> +#define TX4939IDE_DMA_stat	0x802	/* 8-bit */
>   

   Symbol case still inconsistent...

> +static void tx4939ide_set_dma_mode(ide_drive_t *drive, const u8 mode)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	u32 mask, val;
> +
> +	/* Update Data Transfer Mode for this drive. */
> +	if (mode >= XFER_UDMA_0)
> +		val = mode - XFER_UDMA_0 + 8;
> +	else if (mode >= XFER_MW_DMA_0)
> +		val = mode - XFER_MW_DMA_0 + 5;
> +	else
> +		val = mode - XFER_PIO_0;
>   

   I must've missed that in the previous review but you don't need to 
handle PIO modes in this method.

> +static u16 tx4939ide_check_error_ints(ide_hwif_t *hwif)
> +{
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	u16 ctl = tx4939ide_readw(base, TX4939IDE_Int_Ctl);
> +
> +	if (ctl & TX4939IDE_INT_BUSERR) {
> +		/* reset FIFO */
> +		u16 sysctl = tx4939ide_readw(base, TX4939IDE_Sys_Ctl);
> +		tx4939ide_writew(sysctl | 0x4000, base, TX4939IDE_Sys_Ctl);
> +		mmiowb();
> +		/* wait 12GBUSCLK (typ. 60ns @ GBUS200MHz, max 270ns) */
> +		ndelay(270);
> +		tx4939ide_writew(sysctl, base, TX4939IDE_Sys_Ctl);
> +	}
> +	if (ctl & (TX4939IDE_INT_ADDRERR |
> +		    TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_BUSERR))
>   

   Hm, why not line up TX4939IDE_INT_DEVTIMING with TX4939IDE_INT_ADDRERR?

> +static void tx4939ide_clear_irq(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif;
> +	void __iomem *base;
> +	u16 ctl;
> +
> +	/*
> +	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
> +	 * jobs for DMA case.
>   

   Shouldn't it be "job", singular?

> +#ifdef __BIG_ENDIAN
> +static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
> +{
> +	ide_hwif_t *hwif	= drive->hwif;
> +	u8 unit			= drive->dn & 1;
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	u8 dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
>   

   Hm, why not line up all the initializers? Either do line up all or do 
not line up any...

> +static u8 tx4939ide_read_and_clear_dma_status(void __iomem *base)
>   

   Hum, that's a long name... :-)

> +#ifdef __BIG_ENDIAN
> +/* custom ide_build_dmatable to handle swapped layout */
> +static int tx4939ide_build_dmatable(ide_drive_t *drive, struct request *rq)
> +{
>   
[...]
> +			xcount = bcount & 0xffff;
> +			if (xcount == 0x0000) {
>   

   Hm, I'm not sure this is necessary here... although I didn't see an 
explicit mention that zero count means 64 KB in the datasheet -- which 
it must mean if the BMIDE spec. was followed).
In ide-dma.c this check was added because of CS5530's brain damage...

> +static int tx4939ide_dma_end(ide_drive_t *drive)
> +{
>   
[...]
> +	if ((dma_stat & 7) == 0 &&
> +	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
> +	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
>   

   Parens around & and | are hardly needed...

> +/* returns 1 if dma irq issued, 0 otherwise */
> +static int tx4939ide_dma_test_irq(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	u16 ctl;
> +	u8 dma_stat, stat;
> +	u16 ide_int;
> +	int found = 0;
> +
> +	ctl = tx4939ide_check_error_ints(hwif);
> +	ide_int = ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST);
> +	switch (ide_int) {
> +	case TX4939IDE_INT_HOST:
> +		/* On error, XFEREND might not be asserted. */
> +		stat = tx4939ide_readb(base, TX4939IDE_AltStat_DevCtl);
> +		if ((stat & (ATA_BUSY|ATA_DRQ|ATA_ERR)) == ATA_ERR)
> +			found = 1;
> +		else {
> +			/* Wait for XFERINT (Mask HOST and unmask XFERINT) */
>   

   s/XFERINT/XFEREND/

> +			ctl &= ~TX4939IDE_INT_XFEREND << 8;
> +			ctl |= TX4939IDE_INT_HOST << 8;
>   

  The last statement seems superfluous given that the same is achieved 
by the following statement.

> +		}
> +		ctl |= ide_int << 8;
> +		break;
> +	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
> +		dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
> +		if (!(dma_stat & 4))
> +			pr_warning("%s: weird interrupt status. "
> +				   "DMA_stat %#02x int_ctl %#04x\n",
> +				   hwif->name, dma_stat, ctl);
> +		found = 1;
> +		break;
> +	}
> +	/*
> +	 * Do not clear XFERINT, HOST now.  They will be cleared by
>   

   s/XFERINT/XFEREND/

> +static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
> +{
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	return tx4939ide_readb(base, TX4939IDE_DMA_stat);
> +}
> +
>   

   Can't ide_read_sff_dma_status() be used in LE mode now that you set 
hwif->dma_base?

> +static void tx4939ide_insw_swap(unsigned long port, void *addr, u32 count)
>   
[...]
> +static void tx4939ide_outsw_swap(unsigned long port, void *addr, u32 count)
>   

   Shouldn't these be inline (if you really need them)?

> +static int __init tx4939ide_probe(struct platform_device *pdev)
> +{
>   
[...]
> +	pr_info("TX4939 IDE interface (%lx,%d)\n", mapbase, irq);
>   

   Hm, the bare numbers in the log won't be informative, could you add 
"base" and "IRQ"?

MBR, Sergei



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ide: Add tx4939ide driver (v3)
  2008-10-16 12:52 ` Sergei Shtylyov
@ 2008-10-16 16:31   ` Atsushi Nemoto
  2008-10-16 17:23     ` Sergei Shtylyov
  0 siblings, 1 reply; 6+ messages in thread
From: Atsushi Nemoto @ 2008-10-16 16:31 UTC (permalink / raw)
  To: sshtylyov; +Cc: linux-mips, linux-ide, bzolnier, ralf

On Thu, 16 Oct 2008 16:52:45 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > This is the driver for the Toshiba TX4939 SoC ATA controller.
> 
>    Mostly ACK but there's still a few minor nits...

Welcome back!  I will address all of your points except for followings.

> > +			xcount = bcount & 0xffff;
> > +			if (xcount == 0x0000) {
> >   
> 
>    Hm, I'm not sure this is necessary here... although I didn't see an 
> explicit mention that zero count means 64 KB in the datasheet -- which 
> it must mean if the BMIDE spec. was followed).
> In ide-dma.c this check was added because of CS5530's brain damage...

Hmm, if I could test this case easily I will drop this.  Otherwise I
will keep it as is for future investigation.

> > +	if ((dma_stat & 7) == 0 &&
> > +	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
> > +	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
> 
>    Parens around & and | are hardly needed...

You mean more parens are needed?

---
Atsushi Nemoto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ide: Add tx4939ide driver (v3)
  2008-10-16 16:31   ` Atsushi Nemoto
@ 2008-10-16 17:23     ` Sergei Shtylyov
  2008-10-16 17:29       ` Atsushi Nemoto
  0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2008-10-16 17:23 UTC (permalink / raw)
  To: Atsushi Nemoto; +Cc: linux-mips, linux-ide, bzolnier, ralf

Hello.

Atsushi Nemoto wrote:

>>>This is the driver for the Toshiba TX4939 SoC ATA controller.

>>   Mostly ACK but there's still a few minor nits...

> Welcome back!

    Indeed, it turned out to be hard to drown in the Dead see. :-)

>  I will address all of your points except for followings.

>>>+			xcount = bcount & 0xffff;
>>>+			if (xcount == 0x0000) {
>>>  

>>   Hm, I'm not sure this is necessary here... although I didn't see an 
>>explicit mention that zero count means 64 KB in the datasheet -- which 
>>it must mean if the BMIDE spec. was followed).
>>In ide-dma.c this check was added because of CS5530's brain damage...

> Hmm, if I could test this case easily I will drop this.  Otherwise I
> will keep it as is for future investigation.

    OK.

>>>+	if ((dma_stat & 7) == 0 &&
>>>+	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
>>>+	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
>>
>>   Parens around & and | are hardly needed...

> You mean more parens are needed?

    I mean less. :-)

> ---
> Atsushi Nemoto

MBR, Sergei

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ide: Add tx4939ide driver (v3)
  2008-10-16 17:23     ` Sergei Shtylyov
@ 2008-10-16 17:29       ` Atsushi Nemoto
  0 siblings, 0 replies; 6+ messages in thread
From: Atsushi Nemoto @ 2008-10-16 17:29 UTC (permalink / raw)
  To: sshtylyov; +Cc: linux-mips, linux-ide, bzolnier, ralf

On Thu, 16 Oct 2008 21:23:07 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> >>>+	if ((dma_stat & 7) == 0 &&
> >>>+	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
> >>>+	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
> >>
> >>   Parens around & and | are hardly needed...
> 
> > You mean more parens are needed?
> 
>     I mean less. :-)

Well, I think all above parens are required.  '&' and '|' are weaker
than '==', no?

---
Atsushi Nemoto

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-10-16 17:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-02 15:08 [PATCH] ide: Add tx4939ide driver (v3) Atsushi Nemoto
2008-10-08 19:09 ` Bartlomiej Zolnierkiewicz
2008-10-16 12:52 ` Sergei Shtylyov
2008-10-16 16:31   ` Atsushi Nemoto
2008-10-16 17:23     ` Sergei Shtylyov
2008-10-16 17:29       ` Atsushi Nemoto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).