[PATCH 1/2] ide: Add tx4939ide driver

[PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

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/port_ops routines.

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

 drivers/ide/Kconfig          |    6 +
 drivers/ide/mips/Makefile    |    1 +
 drivers/ide/mips/tx4939ide.c |  762 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 769 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..ba9776d
--- /dev/null
+++ b/drivers/ide/mips/tx4939ide.c
@@ -0,0 +1,762 @@
+/*
+ * 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>
+
+#define MODNAME	"tx4939ide"
+
+/* ATA Shadow Registers (8bit except for DATA which is 16bit) */
+#define TX4939IDE_DATA	0x000
+#define TX4939IDE_Error_Ft	0x001
+#define TX4939IDE_Sec	0x002
+#define TX4939IDE_LBA0	0x003
+#define TX4939IDE_LBA1	0x004
+#define TX4939IDE_LBA2	0x005
+#define TX4939IDE_Device	0x006
+#define TX4939IDE_St_Cmd	0x007
+#define TX4939IDE_Alt_DevCtl	0x402
+/* H/W DMA Registers  */
+#define TX4939IDE_DMA_Cmd	0x800	/* 8bit */
+#define TX4939IDE_DMA_stat	0x802	/* 8bit */
+#define TX4939IDE_PRD_Ptr	0x804	/* 32bit */
+/* ATA100 CORE Registers (16bit) */
+#define TX4939IDE_Sys_Ctl	0xc00
+#define TX4939IDE_Xfer_Cnt_1	0xc08
+#define TX4939IDE_Xfer_Cnt_2	0xc0a
+#define TX4939IDE_Sec_Cnt	0xc10
+#define TX4939IDE_Strt_AddL	0xc18
+#define TX4939IDE_Strt_AddU	0xc20
+#define TX4939IDE_Add_Ctl	0xc28
+#define TX4939IDE_Lo_BCnt	0xc30
+#define TX4939IDE_Up_BCnt	0xc38
+#define TX4939IDE_PIO_Acc	0xc88
+#define TX4939IDE_H_Rst_Tim	0xc90
+#define TX4939IDE_int_ctl	0xc98
+#define TX4939IDE_Pkt_Cmd	0xcb8
+#define TX4939IDE_Bxfer_cntH	0xcc0
+#define TX4939IDE_Bxfer_cntL	0xcc8
+#define TX4939IDE_Dev_TErr	0xcd0
+#define TX4939IDE_Pkt_xfer_ct	0xcd8
+#define TX4939IDE_Strt_AddT	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)
+
+#ifdef __BIG_ENDIAN
+#define TX4939IDE_REG32(reg)	(TX4939IDE_##reg ^ 4)
+#define TX4939IDE_REG16(reg)	(TX4939IDE_##reg ^ 6)
+#define TX4939IDE_REG8(reg)	(TX4939IDE_##reg ^ 7)
+#else
+#define TX4939IDE_REG32(reg)	(TX4939IDE_##reg)
+#define TX4939IDE_REG16(reg)	(TX4939IDE_##reg)
+#define TX4939IDE_REG8(reg)	(TX4939IDE_##reg)
+#endif
+
+#define TX4939IDE_readl(base, reg) \
+	__raw_readl((void __iomem *)((base) + TX4939IDE_REG32(reg)))
+#define TX4939IDE_readw(base, reg) \
+	__raw_readw((void __iomem *)((base) + TX4939IDE_REG16(reg)))
+#define TX4939IDE_readb(base, reg) \
+	__raw_readb((void __iomem *)((base) + TX4939IDE_REG8(reg)))
+#define TX4939IDE_writel(val, base, reg) \
+	__raw_writel(val, (void __iomem *)((base) + TX4939IDE_REG32(reg)))
+#define TX4939IDE_writew(val, base, reg) \
+	__raw_writew(val, (void __iomem *)((base) + TX4939IDE_REG16(reg)))
+#define TX4939IDE_writeb(val, base, reg) \
+	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))
+
+#define TX4939IDE_BASE(hwif)	((hwif)->io_ports.data_addr & ~0xfff)
+
+static void tx4939ide_set_mode(ide_drive_t *drive, const u8 speed)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	unsigned long base = TX4939IDE_BASE(hwif);
+	int is_slave = drive->dn & 1;
+	u16 value;
+	int safe_speed = speed;
+	int i;
+
+	for (i = 0; i < MAX_DRIVES; i++) {
+		if (drive != &hwif->drives[i] &&
+		    (hwif->drives[i].dev_flags & IDE_DFLAG_PRESENT))
+			safe_speed = min(safe_speed,
+					 (int)hwif->drives[i].current_speed);
+	}
+
+	/* Data Transfer Mode Select */
+	switch (speed) {
+	case XFER_UDMA_5:
+		value = 0x00d0;
+		break;
+	case XFER_UDMA_4:
+		value = 0x00c0;
+		break;
+	case XFER_UDMA_3:
+		value = 0x00b0;
+		break;
+	case XFER_UDMA_2:
+		value = 0x00a0;
+		break;
+	case XFER_UDMA_1:
+		value = 0x0090;
+		break;
+	case XFER_UDMA_0:
+		value = 0x0080;
+		break;
+	case XFER_MW_DMA_2:
+		value = 0x0070;
+		break;
+	case XFER_MW_DMA_1:
+		value = 0x0060;
+		break;
+	case XFER_MW_DMA_0:
+		value = 0x0050;
+		break;
+	case XFER_PIO_4:
+		value = 0x0040;
+		break;
+	case XFER_PIO_3:
+		value = 0x0030;
+		break;
+	case XFER_PIO_2:
+		value = 0x0020;
+		break;
+	case XFER_PIO_1:
+		value = 0x0010;
+		break;
+	default:
+	case XFER_PIO_0:
+		value = 0x0000;
+		break;
+	}
+	/* Command Transfer Mode Select */
+	switch (safe_speed) {
+	case XFER_UDMA_5:
+	case XFER_UDMA_4:
+	case XFER_UDMA_3:
+	case XFER_UDMA_2:
+	case XFER_UDMA_1:
+	case XFER_UDMA_0:
+	case XFER_MW_DMA_2:
+	case XFER_MW_DMA_1:
+	case XFER_MW_DMA_0:
+	case XFER_PIO_4:
+		value |= 0x0400;
+		break;
+	case XFER_PIO_3:
+		value |= 0x0300;
+		break;
+	case XFER_PIO_2:
+		value |= 0x0200;
+		break;
+	case XFER_PIO_1:
+		value |= 0x0100;
+		break;
+	default:
+	case XFER_PIO_0:
+		value |= 0x0000;
+		break;
+	}
+
+	if (is_slave)
+		hwif->select_data =
+			(hwif->select_data & ~0xffff0000) | (value << 16);
+	else
+		hwif->select_data = (hwif->select_data & ~0x0000ffff) | value;
+	TX4939IDE_writew(value, base, Sys_Ctl);
+}
+
+static void tx4939ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	tx4939ide_set_mode(drive, XFER_PIO_0 + pio);
+}
+
+static void tx4939ide_check_error_ints(ide_hwif_t *hwif, u16 stat)
+{
+	if (stat & TX4939IDE_INT_BUSERR) {
+		unsigned long base = TX4939IDE_BASE(hwif);
+		/* reset FIFO */
+		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) |
+				 0x4000,
+				 base, Sys_Ctl);
+	}
+	if (stat & (TX4939IDE_INT_ADDRERR | TX4939IDE_INT_REACHMUL |
+		    TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_BUSERR))
+		pr_err("%s: Error interrupt %#x (%s%s%s%s )\n",
+		       hwif->name, stat,
+		       (stat & TX4939IDE_INT_ADDRERR) ?
+		       " Address-Error" : "",
+		       (stat & TX4939IDE_INT_REACHMUL) ?
+		       " Reach-Multiple" : "",
+		       (stat & TX4939IDE_INT_DEVTIMING) ?
+		       " DEV-Timing" : "",
+		       (stat & TX4939IDE_INT_BUSERR) ?
+		       " Bus-Error" : "");
+}
+
+static void tx4939ide_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif;
+	unsigned long base;
+	u16 ctl;
+
+	/*
+	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
+	 * jobs for DMA case.
+	 */
+	if (drive->waiting_for_dma)
+		return;
+	hwif = HWIF(drive);
+	base = TX4939IDE_BASE(hwif);
+	ctl = TX4939IDE_readw(base, int_ctl);
+
+	tx4939ide_check_error_ints(hwif, ctl);
+	TX4939IDE_writew(ctl, base, int_ctl);
+}
+
+static u8 tx4939ide_cable_detect(ide_hwif_t *hwif)
+{
+	unsigned long base = TX4939IDE_BASE(hwif);
+
+	return (TX4939IDE_readw(base, Sys_Ctl) & 0x2000)
+		? ATA_CBL_PATA40 : ATA_CBL_PATA80;
+}
+
+static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
+{
+	ide_hwif_t *hwif	= HWIF(drive);
+	u8 unit			= drive->dn & 1;
+	unsigned long base = TX4939IDE_BASE(hwif);
+	u8 dma_stat = TX4939IDE_readb(base, DMA_stat);
+
+	if (on)
+		dma_stat |= (1 << (5 + unit));
+	else
+		dma_stat &= ~(1 << (5 + unit));
+
+	TX4939IDE_writeb(dma_stat, base, DMA_stat);
+}
+
+static int __tx4939ide_dma_setup(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct request *rq = HWGROUP(drive)->rq;
+	unsigned int reading;
+	u8 dma_stat;
+	unsigned long base = TX4939IDE_BASE(hwif);
+
+	if (rq_data_dir(rq))
+		reading = 0;
+	else
+		reading = 1 << 3;
+
+	/* fall back to pio! */
+	if (!ide_build_dmatable(drive, rq)) {
+		ide_map_sg(drive, rq);
+		return 1;
+	}
+#ifdef __BIG_ENDIAN
+	{
+		unsigned int *table = hwif->dmatable_cpu;
+		while (1) {
+			cpu_to_le64s((u64 *)table);
+			if (*table & 0x80000000)
+				break;
+			table += 2;
+		}
+	}
+#endif
+
+	/* PRD table */
+	TX4939IDE_writel(hwif->dmatable_dma, base, PRD_Ptr);
+
+	/* specify r/w */
+	TX4939IDE_writeb(reading, base, DMA_Cmd);
+
+	/* read DMA status for INTR & ERROR flags */
+	dma_stat = TX4939IDE_readb(base, DMA_stat);
+
+	/* clear INTR & ERROR flags */
+	TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
+	/* recover intmask cleared by writing to bit2 of DMA_stat */
+	TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
+
+	drive->waiting_for_dma = 1;
+	return 0;
+}
+
+static int tx4939ide_dma_setup(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	unsigned long base = TX4939IDE_BASE(hwif);
+	int is_slave = drive->dn & 1;
+	unsigned int nframes;
+	int rc, i;
+	unsigned int sect_size = queue_hardsect_size(drive->queue);
+	u16 select_data;
+
+	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
+	TX4939IDE_writew(select_data, base, Sys_Ctl);
+	if (is_slave)
+		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
+	else
+		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
+
+	rc = __tx4939ide_dma_setup(drive);
+	if (rc == 0) {
+		/* Number of sectors to transfer. */
+		nframes = 0;
+		for (i = 0; i < hwif->sg_nents; i++)
+			nframes += sg_dma_len(&hwif->sg_table[i]);
+		BUG_ON(nframes % sect_size != 0);
+		nframes /= sect_size;
+		BUG_ON(nframes == 0);
+		TX4939IDE_writew(nframes, base, Sec_Cnt);
+	}
+	return rc;
+}
+
+static void tx4939ide_dma_start(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	unsigned long base = TX4939IDE_BASE(hwif);
+	u8 dma_cmd;
+
+	/* Note that this is done *after* the cmd has
+	 * been issued to the drive, as per the BM-IDE spec.
+	 */
+	dma_cmd = TX4939IDE_readb(base, DMA_Cmd);
+	/* start DMA */
+	TX4939IDE_writeb(dma_cmd | 1, base, DMA_Cmd);
+
+	wmb();
+}
+
+static int tx4939ide_dma_end(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	u8 dma_stat = 0, dma_cmd = 0;
+	unsigned long base = TX4939IDE_BASE(hwif);
+	u16 ctl = TX4939IDE_readw(base, int_ctl);
+
+	drive->waiting_for_dma = 0;
+
+	/* get DMA command mode */
+	dma_cmd = TX4939IDE_readb(base, DMA_Cmd);
+	/* stop DMA */
+	TX4939IDE_writeb(dma_cmd & ~1, base, DMA_Cmd);
+
+	/* get DMA status */
+	dma_stat = TX4939IDE_readb(base, DMA_stat);
+
+	/* clear the INTR & ERROR bits */
+	TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
+	/* recover intmask cleared by writing to bit2 of DMA_stat */
+	TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
+
+	/* 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 = HWIF(drive);
+	unsigned long base = TX4939IDE_BASE(hwif);
+	u16 ctl = TX4939IDE_readw(base, int_ctl);
+	u8 dma_stat, stat;
+	u16 ide_int;
+	int found = 0;
+
+	tx4939ide_check_error_ints(hwif, ctl);
+	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, Alt_DevCtl);
+		if ((stat & (ATA_BUSY|ATA_DRQ|ATA_ERR)) == ATA_ERR) {
+			pr_err("%s: detect error %x in %s\n",
+			       drive->name, stat, __func__);
+			found = 1;
+		}
+		/* FALLTHRU */
+	case TX4939IDE_INT_XFEREND:
+		/*
+		 * If only one of XFERINT and HOST was asserted, mask
+		 * this interrupt and wait for an another one.  Note
+		 * that write to bit2 of DMA_stat will clear all
+		 * mask bits.
+		 */
+		ctl |= ide_int << 8;
+		break;
+	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
+		dma_stat = TX4939IDE_readb(base, DMA_stat);
+		if (!(dma_stat & 4))
+			pr_debug("%s: weired 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, int_ctl);
+	return found;
+}
+
+static void tx4939ide_hwif_init(ide_hwif_t *hwif)
+{
+	unsigned long base = TX4939IDE_BASE(hwif);
+	int timeout;
+
+	/* Soft Reset */
+	TX4939IDE_writew(0x8000, base, Sys_Ctl);
+	mmiowb();
+	udelay(1);	/* at least 20 UPSCLK (100ns for 200MHz GBUSCLK) */
+	/* ATA Hard Reset */
+	TX4939IDE_writew(0x0800, base, Sys_Ctl);
+	timeout = 1000;
+	while (TX4939IDE_readw(base, Sys_Ctl) & 0x0800) {
+		if (timeout--)
+			break;
+		udelay(1);
+	}
+	/* mask some interrupts and clear all interrupts */
+	TX4939IDE_writew((TX4939IDE_IGNORE_INTS << 8) | 0xff, base, int_ctl);
+
+#ifdef __BIG_ENDIAN
+	/* This setting does not affect PRD fetch */
+	/* ByteSwap=1, Endian=00 */
+	TX4939IDE_writew(0xc911, base, Add_Ctl);
+#else
+	TX4939IDE_writew(0xc901, base, Add_Ctl);
+#endif
+
+	TX4939IDE_writew(0x0008, base, Lo_BCnt);
+	TX4939IDE_writew(0, base, Up_BCnt);
+}
+
+static int tx4939ide_init_dma(ide_hwif_t *hwif, const struct ide_port_info *d)
+{
+	return ide_allocate_dma_engine(hwif);
+}
+
+#ifdef __BIG_ENDIAN
+/* custom iops (independent from SWAP_IO_SPACE) */
+static u8 mm_inb(unsigned long port)
+{
+	return (u8)readb((void __iomem *)port);
+}
+static void mm_outb(u8 value, unsigned long port)
+{
+	writeb(value, (void __iomem *)port);
+}
+static void mm_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;
+
+		__raw_writew(data, (void __iomem *)io_ports->data_addr);
+	}
+
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_FEATURE)
+		mm_outb(tf->hob_feature, io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_NSECT)
+		mm_outb(tf->hob_nsect, io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAL)
+		mm_outb(tf->hob_lbal, io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAM)
+		mm_outb(tf->hob_lbam, io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAH)
+		mm_outb(tf->hob_lbah, io_ports->lbah_addr);
+
+	if (task->tf_flags & IDE_TFLAG_OUT_FEATURE)
+		mm_outb(tf->feature, io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_NSECT)
+		mm_outb(tf->nsect, io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAL)
+		mm_outb(tf->lbal, io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAM)
+		mm_outb(tf->lbam, io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAH)
+		mm_outb(tf->lbah, io_ports->lbah_addr);
+
+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE) {
+		unsigned long base = TX4939IDE_BASE(hwif);
+		mm_outb((tf->device & HIHI) | drive->select,
+			 io_ports->device_addr);
+		/* Fix ATA100 CORE System Control Register */
+		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) & 0x07f0,
+				 base, Sys_Ctl);
+	}
+}
+static void mm_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;
+
+		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 */
+	mm_outb(ATA_DEVCTL_OBS & ~0x80, io_ports->ctl_addr);
+
+	if (task->tf_flags & IDE_TFLAG_IN_FEATURE)
+		tf->feature = mm_inb(io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_NSECT)
+		tf->nsect  = mm_inb(io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAL)
+		tf->lbal   = mm_inb(io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAM)
+		tf->lbam   = mm_inb(io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAH)
+		tf->lbah   = mm_inb(io_ports->lbah_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_DEVICE)
+		tf->device = mm_inb(io_ports->device_addr);
+
+	if (task->tf_flags & IDE_TFLAG_LBA48) {
+		mm_outb(ATA_DEVCTL_OBS | 0x80, io_ports->ctl_addr);
+
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_FEATURE)
+			tf->hob_feature = mm_inb(io_ports->feature_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_NSECT)
+			tf->hob_nsect   = mm_inb(io_ports->nsect_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAL)
+			tf->hob_lbal    = mm_inb(io_ports->lbal_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAM)
+			tf->hob_lbam    = mm_inb(io_ports->lbam_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAH)
+			tf->hob_lbah    = mm_inb(io_ports->lbah_addr);
+	}
+}
+
+static void mm_insw_swap(unsigned long port, void *addr, u32 count)
+{
+	unsigned short *ptr = addr;
+	unsigned long size = count * 2;
+	port &= ~1;
+	while (count--)
+		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
+	__ide_flush_dcache_range((unsigned long)addr, size);
+}
+static void mm_outsw_swap(unsigned long port, void *addr, u32 count)
+{
+	unsigned short *ptr = addr;
+	unsigned long size = count * 2;
+	port &= ~1;
+	while (count--) {
+		__raw_writew(cpu_to_le16(*ptr), (void __iomem *)port);
+		ptr++;
+	}
+	__ide_flush_dcache_range((unsigned long)addr, size);
+}
+static void mmio_input_data_swap(ide_drive_t *drive, struct request *rq,
+				 void *buf, unsigned int len)
+{
+	mm_insw_swap(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
+}
+static void mmio_output_data_swap(ide_drive_t *drive, struct request *rq,
+				  void *buf, unsigned int len)
+{
+	mm_outsw_swap(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
+}
+
+static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
+{
+	unsigned long base = TX4939IDE_BASE(hwif);
+	return TX4939IDE_readb(base, DMA_stat);
+}
+
+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		= mm_tf_load,
+	.tf_read		= mm_tf_read,
+
+	.input_data		= mmio_input_data_swap,
+	.output_data		= mmio_output_data_swap,
+};
+#endif	/* __BIG_ENDIAN */
+
+static const struct ide_port_ops tx4939ide_port_ops = {
+	.set_pio_mode = tx4939ide_set_pio_mode,
+	.set_dma_mode = tx4939ide_set_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 = tx4939ide_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_hwif_init,
+	.init_dma = tx4939ide_init_dma,
+	.port_ops = &tx4939ide_port_ops,
+	.dma_ops = &tx4939ide_dma_ops,
+#ifdef __BIG_ENDIAN
+	.tp_ops = &tx4939ide_tp_ops,
+#endif
+	.host_flags = IDE_HFLAG_MMIO,
+	.pio_mask = ATA_PIO4,
+	.mwdma_mask = ATA_MWDMA2,
+	.swdma_mask = ATA_SWDMA2,
+	.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_REG8(DATA);
+	hw.io_ports.error_addr = mapbase + TX4939IDE_REG8(Error_Ft);
+	hw.io_ports.nsect_addr = mapbase + TX4939IDE_REG8(Sec);
+	hw.io_ports.lbal_addr = mapbase + TX4939IDE_REG8(LBA0);
+	hw.io_ports.lbam_addr = mapbase + TX4939IDE_REG8(LBA1);
+	hw.io_ports.lbah_addr = mapbase + TX4939IDE_REG8(LBA2);
+	hw.io_ports.device_addr = mapbase + TX4939IDE_REG8(Device);
+	hw.io_ports.command_addr = mapbase + TX4939IDE_REG8(St_Cmd);
+	hw.io_ports.ctl_addr = mapbase + TX4939IDE_REG8(Alt_DevCtl);
+	hw.irq = irq;
+	hw.dev = &pdev->dev;
+
+	pr_info("TX4939 IDE interface (%lx,%d)\n", mapbase, irq);
+	ret = ide_host_add(&tx4939ide_port_info, hws, &host);
+	if (ret)
+		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];
+	unsigned long base = TX4939IDE_BASE(hwif);
+
+	tx4939ide_hwif_init(hwif);
+
+	/* restore Sys_Ctl */
+	TX4939IDE_writew(hwif->select_data & 0xffff, base, Sys_Ctl);
+	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

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Alan Cox]

> 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/port_ops routines.

It would probably be a lot cleaner using the libata framework, and also
go obsolete less soon.

> +#define TX4939IDE_readl(base, reg) \
> +	__raw_readl((void __iomem *)((base) + TX4939IDE_REG32(reg)))
> +#define TX4939IDE_readw(base, reg) \
> +	__raw_readw((void __iomem *)((base) + TX4939IDE_REG16(reg)))
> +#define TX4939IDE_readb(base, reg) \
> +	__raw_readb((void __iomem *)((base) + TX4939IDE_REG8(reg)))
> +#define TX4939IDE_writel(val, base, reg) \
> +	__raw_writel(val, (void __iomem *)((base) + TX4939IDE_REG32(reg)))
> +#define TX4939IDE_writew(val, base, reg) \
> +	__raw_writew(val, (void __iomem *)((base) + TX4939IDE_REG16(reg)))
> +#define TX4939IDE_writeb(val, base, reg) \
> +	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))

It's generally frowned upon to hide all the detail in macros, it is much
easier to read and understand the code if you don't do this.

> +#define TX4939IDE_BASE(hwif)	((hwif)->io_ports.data_addr & ~0xfff)

Why do you have void __iomem casts all over the write methods not in the
_BASE() method - that would let sparse do its job properly

> +	for (i = 0; i < MAX_DRIVES; i++) {
> +		if (drive != &hwif->drives[i] &&

You don't actually need the first test. This also appears wrong. In your
tests MW_DMA_0 is 'faster' than PIO4 but in fact MW_DMA_0 PIO timings are
*slower* than PIO4 so the mode is not in fact slower.

> +	case XFER_MW_DMA_2:
> +	case XFER_MW_DMA_1:
> +	case XFER_MW_DMA_0:
> +	case XFER_PIO_4:
> +		value |= 0x0400;
> +		break;

This looks odd according to the speed tables. Can you clarify what is
going on ?

> +#ifdef __BIG_ENDIAN
> +	{
> +		unsigned int *table = hwif->dmatable_cpu;
> +		while (1) {
> +			cpu_to_le64s((u64 *)table);
> +			if (*table & 0x80000000)
> +				break;

You modify the table but you never ensure the data is not still in
temporary variables from the compiler or flushed from cache

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>+#define TX4939IDE_readl(base, reg) \
>>+	__raw_readl((void __iomem *)((base) + TX4939IDE_REG32(reg)))
>>+#define TX4939IDE_readw(base, reg) \
>>+	__raw_readw((void __iomem *)((base) + TX4939IDE_REG16(reg)))
>>+#define TX4939IDE_readb(base, reg) \
>>+	__raw_readb((void __iomem *)((base) + TX4939IDE_REG8(reg)))
>>+#define TX4939IDE_writel(val, base, reg) \
>>+	__raw_writel(val, (void __iomem *)((base) + TX4939IDE_REG32(reg)))
>>+#define TX4939IDE_writew(val, base, reg) \
>>+	__raw_writew(val, (void __iomem *)((base) + TX4939IDE_REG16(reg)))
>>+#define TX4939IDE_writeb(val, base, reg) \
>>+	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))

> It's generally frowned upon to hide all the detail in macros, it is much
> easier to read and understand the code if you don't do this.

>>+#define TX4939IDE_BASE(hwif)	((hwif)->io_ports.data_addr & ~0xfff)

> Why do you have void __iomem casts all over the write methods not in the
> _BASE() method - that would let sparse do its job properly

    I don't get why there's need for & at all -- isn't IDE data register 
address always on 4K boundary?

>>+	for (i = 0; i < MAX_DRIVES; i++) {
>>+		if (drive != &hwif->drives[i] &&

> You don't actually need the first test.

    No, he does need it -- in order not to clamp the new PIO mode based on the 
previosly selected one. Although, one should call ide_get_paired_drive() ISO 
this loop.

> This also appears wrong. In your
> tests MW_DMA_0 is 'faster' than PIO4 but in fact MW_DMA_0 PIO timings are
> *slower* than PIO4 so the mode is not in fact slower.

    I don't think it's about the DMA timings at all. Though indeed, MWDMA0/1 
do (iff it's drive's max) implies slower max PIO mode than PIO4.

>>+	case XFER_MW_DMA_2:
>>+	case XFER_MW_DMA_1:
>>+	case XFER_MW_DMA_0:
>>+	case XFER_PIO_4:
>>+		value |= 0x0400;
>>+		break;

> This looks odd according to the speed tables. Can you clarify what is
> going on ?

    This apparently selects the command PIO timing safest for both drives but 
does this incorrectly -- the current DMA (or even PIO) mode shouldn't be a 
part of the equation.  There are several examples how to do this including 
siimage.c and cs5535.c...

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Tue, 09 Sep 2008 21:08:14 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> >>+#define TX4939IDE_BASE(hwif)	((hwif)->io_ports.data_addr & ~0xfff)
> 
> > Why do you have void __iomem casts all over the write methods not in the
> > _BASE() method - that would let sparse do its job properly
> 
>     I don't get why there's need for & at all -- isn't IDE data register 
> address always on 4K boundary?

On little endian, yes.  On big endian, this controller flips addr[2:0].

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Tue, 9 Sep 2008 17:44:59 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > 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/port_ops routines.
> 
> It would probably be a lot cleaner using the libata framework, and also
> go obsolete less soon.

Yes, that's future plan.

> > +#define TX4939IDE_writeb(val, base, reg) \
> > +	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))
> 
> It's generally frowned upon to hide all the detail in macros, it is much
> easier to read and understand the code if you don't do this.

OK, I'll try to make much readble.

> > +#define TX4939IDE_BASE(hwif)	((hwif)->io_ports.data_addr & ~0xfff)
> 
> Why do you have void __iomem casts all over the write methods not in the
> _BASE() method - that would let sparse do its job properly

Indeed. I'll do it.

> > +	for (i = 0; i < MAX_DRIVES; i++) {
> > +		if (drive != &hwif->drives[i] &&
> 
> You don't actually need the first test. This also appears wrong. In your
> tests MW_DMA_0 is 'faster' than PIO4 but in fact MW_DMA_0 PIO timings are
> *slower* than PIO4 so the mode is not in fact slower.
> 
> > +	case XFER_MW_DMA_2:
> > +	case XFER_MW_DMA_1:
> > +	case XFER_MW_DMA_0:
> > +	case XFER_PIO_4:
> > +		value |= 0x0400;
> > +		break;
> 
> This looks odd according to the speed tables. Can you clarify what is
> going on ?

As you and Sergei pointed out, the code seems somewhat broken.  I'll
rework on it.

> > +#ifdef __BIG_ENDIAN
> > +	{
> > +		unsigned int *table = hwif->dmatable_cpu;
> > +		while (1) {
> > +			cpu_to_le64s((u64 *)table);
> > +			if (*table & 0x80000000)
> > +				break;
> 
> You modify the table but you never ensure the data is not still in
> temporary variables from the compiler or flushed from cache

The dmatable_cpu is allocated by pci_alloc_consistent so that flush is
not needed.  But... this is not PCI device.  I should not use
ide_allocate_dma_engine().  I'll fix it.

Thank you for review.
---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Thu, 11 Sep 2008 00:06:49 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> The dmatable_cpu is allocated by pci_alloc_consistent so that flush is
> not needed.  But... this is not PCI device.  I should not use
> ide_allocate_dma_engine().  I'll fix it.

Fortunately such a fix will not be needed.  A patch making
ide_allocate_dma_engine() independent from PCI already posted (
"[PATCH 4/8] ide: switch to DMA-mapping API part 2").  I can use
ide_allocate_dma_engine() with non-PCI device safely.  Thanks.

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

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/port_ops routines.

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

>  drivers/ide/Kconfig          |    6 +
>  drivers/ide/mips/Makefile    |    1 +
>  drivers/ide/mips/tx4939ide.c |  762 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 769 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/ide/mips/tx4939ide.c

> diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
> new file mode 100644
> index 0000000..ba9776d
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,762 @@

[...]

> +static void tx4939ide_set_mode(ide_drive_t *drive, const u8 speed)
> +{
> +	ide_hwif_t *hwif = HWIF(drive);
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	int is_slave = drive->dn & 1;
> +	u16 value;
> +	int safe_speed = speed;
> +	int i;
> +
> +	for (i = 0; i < MAX_DRIVES; i++) {

    Use ide_get_paired_drive() ISO this loop.

> +		if (drive != &hwif->drives[i] &&
> +		    (hwif->drives[i].dev_flags & IDE_DFLAG_PRESENT))
> +			safe_speed = min(safe_speed,
> +					 (int)hwif->drives[i].current_speed);

    You shouldn't clamp the command PIO mode timings like this, and shouldn't 
do it at all when DMA mode is set. Call ide_get_best_pio_mode(255, 4) to get 
the mate drive's fastest PIO mode which should be a clamping value.

> +	/* Command Transfer Mode Select */
> +	switch (safe_speed) {
> +	case XFER_UDMA_5:
> +	case XFER_UDMA_4:
> +	case XFER_UDMA_3:
> +	case XFER_UDMA_2:
> +	case XFER_UDMA_1:
> +	case XFER_UDMA_0:
> +	case XFER_MW_DMA_2:

    You shouldn't change the command PIO mode when DMA mode is selected.

> +	case XFER_MW_DMA_1:
> +	case XFER_MW_DMA_0:
> +	case XFER_PIO_4:

    MWDMA0/1 timings don't match PIO4, they are [much] slower.

> +		value |= 0x0400;
> +		break;
> +	case XFER_PIO_3:
> +		value |= 0x0300;
> +		break;
> +	case XFER_PIO_2:
> +		value |= 0x0200;
> +		break;
> +	case XFER_PIO_1:
> +		value |= 0x0100;
> +		break;
> +	default:
> +	case XFER_PIO_0:
> +		value |= 0x0000;
> +		break;
> +	}
 > +
> +	if (is_slave)
> +		hwif->select_data =
> +			(hwif->select_data & ~0xffff0000) | (value << 16);

    Why not just 0x0000ffff?

> +	else
> +		hwif->select_data = (hwif->select_data & ~0x0000ffff) | value;

    Why not just 0xffff0000?

> +	TX4939IDE_writew(value, base, Sys_Ctl);
> +}
> +
> +static void tx4939ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
> +{
> +	tx4939ide_set_mode(drive, XFER_PIO_0 + pio);
> +}

    I suggest that you implement tx4939ide_set_{dma|pio}_mode() as seperate 
functions, possibly using a common function to do a final part. These 2 
methods are quite different functionally.

> +
> +static void tx4939ide_check_error_ints(ide_hwif_t *hwif, u16 stat)
> +{
> +	if (stat & TX4939IDE_INT_BUSERR) {
> +		unsigned long base = TX4939IDE_BASE(hwif);
> +		/* reset FIFO */
> +		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) |
> +				 0x4000,
> +				 base, Sys_Ctl);
> +	}
> +	if (stat & (TX4939IDE_INT_ADDRERR | TX4939IDE_INT_REACHMUL |
> +		    TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_BUSERR))
> +		pr_err("%s: Error interrupt %#x (%s%s%s%s )\n",
> +		       hwif->name, stat,
> +		       (stat & TX4939IDE_INT_ADDRERR) ?
> +		       " Address-Error" : "",
> +		       (stat & TX4939IDE_INT_REACHMUL) ?
> +		       " Reach-Multiple" : "",
> +		       (stat & TX4939IDE_INT_DEVTIMING) ?
> +		       " DEV-Timing" : "",
> +		       (stat & TX4939IDE_INT_BUSERR) ?
> +		       " Bus-Error" : "");
> +}
> +
> +static void tx4939ide_clear_irq(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif;
> +	unsigned long base;
> +	u16 ctl;
> +
> +	/*
> +	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
> +	 * jobs for DMA case.
> +	 */
> +	if (drive->waiting_for_dma)
> +		return;
> +	hwif = HWIF(drive);
> +	base = TX4939IDE_BASE(hwif);
> +	ctl = TX4939IDE_readw(base, int_ctl);
> +
> +	tx4939ide_check_error_ints(hwif, ctl);
> +	TX4939IDE_writew(ctl, base, int_ctl);
> +}
> +
> +static int tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = HWIF(drive);
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	int is_slave = drive->dn & 1;
> +	unsigned int nframes;
> +	int rc, i;
> +	unsigned int sect_size = queue_hardsect_size(drive->queue);
> +	u16 select_data;
> +
> +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> +	TX4939IDE_writew(select_data, base, Sys_Ctl);

    Unfortunately, programming the timings from the dma_setup() method isn't 
enough since it won't be called for PIO transfers.  You'll have to use the 
selectproc() method.

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Tue, 09 Sep 2008 21:50:32 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > +static void tx4939ide_set_mode(ide_drive_t *drive, const u8 speed)
> > +{
> > +	ide_hwif_t *hwif = HWIF(drive);
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	int is_slave = drive->dn & 1;
> > +	u16 value;
> > +	int safe_speed = speed;
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_DRIVES; i++) {
> 
>     Use ide_get_paired_drive() ISO this loop.

Thanks.  That's what I needed.

> > +		if (drive != &hwif->drives[i] &&
> > +		    (hwif->drives[i].dev_flags & IDE_DFLAG_PRESENT))
> > +			safe_speed = min(safe_speed,
> > +					 (int)hwif->drives[i].current_speed);
> 
>     You shouldn't clamp the command PIO mode timings like this, and shouldn't 
> do it at all when DMA mode is set. Call ide_get_best_pio_mode(255, 4) to get 
> the mate drive's fastest PIO mode which should be a clamping value.
> 
> > +	/* Command Transfer Mode Select */
> > +	switch (safe_speed) {
> > +	case XFER_UDMA_5:
> > +	case XFER_UDMA_4:
> > +	case XFER_UDMA_3:
> > +	case XFER_UDMA_2:
> > +	case XFER_UDMA_1:
> > +	case XFER_UDMA_0:
> > +	case XFER_MW_DMA_2:
> 
>     You shouldn't change the command PIO mode when DMA mode is selected.

But the "Command Transfer Mode Select" bits affects access timings on
setting task registers for DMA command.  Hmm... do you mean I should
not do it _here_?

> > +	case XFER_MW_DMA_1:
> > +	case XFER_MW_DMA_0:
> > +	case XFER_PIO_4:
> 
>     MWDMA0/1 timings don't match PIO4, they are [much] slower.

Oh thanks.  I will fix it.

> > +		hwif->select_data =
> > +			(hwif->select_data & ~0xffff0000) | (value << 16);
> 
>     Why not just 0x0000ffff?
> 
> > +	else
> > +		hwif->select_data = (hwif->select_data & ~0x0000ffff) | value;
> 
>     Why not just 0xffff0000?

Indeed.

> > +static void tx4939ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
> > +{
> > +	tx4939ide_set_mode(drive, XFER_PIO_0 + pio);
> > +}
> 
>     I suggest that you implement tx4939ide_set_{dma|pio}_mode() as seperate 
> functions, possibly using a common function to do a final part. These 2 
> methods are quite different functionally.
> 
> > +static int tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = HWIF(drive);
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	int is_slave = drive->dn & 1;
> > +	unsigned int nframes;
> > +	int rc, i;
> > +	unsigned int sect_size = queue_hardsect_size(drive->queue);
> > +	u16 select_data;
> > +
> > +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> > +	TX4939IDE_writew(select_data, base, Sys_Ctl);
> 
>     Unfortunately, programming the timings from the dma_setup() method isn't 
> enough since it won't be called for PIO transfers.  You'll have to use the 
> selectproc() method.

Thanks.  I should rework whole timing setup code.

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Atsushi Nemoto wrote:

>>>+		if (drive != &hwif->drives[i] &&
>>>+		    (hwif->drives[i].dev_flags & IDE_DFLAG_PRESENT))
>>>+			safe_speed = min(safe_speed,
>>>+					 (int)hwif->drives[i].current_speed);

>>    You shouldn't clamp the command PIO mode timings like this, and shouldn't 
>>do it at all when DMA mode is set. Call ide_get_best_pio_mode(255, 4) to get 
>>the mate drive's fastest PIO mode which should be a clamping value.

>>>+	/* Command Transfer Mode Select */
>>>+	switch (safe_speed) {
>>>+	case XFER_UDMA_5:
>>>+	case XFER_UDMA_4:
>>>+	case XFER_UDMA_3:
>>>+	case XFER_UDMA_2:
>>>+	case XFER_UDMA_1:
>>>+	case XFER_UDMA_0:
>>>+	case XFER_MW_DMA_2:

>>    You shouldn't change the command PIO mode when DMA mode is selected.

> But the "Command Transfer Mode Select" bits affects access timings on
> setting task registers for DMA command.

    So what? PIO and DMA are different protocols on IDE bus, so they shouldn't 
affect each other. The IDE core will always tune the best PIO mode for you, so 
the optimal command timings will be set.

>  Hmm... do you mean I should not do it _here_?

    You should only change command PIO timings only when PIO mode is changed.

>>>+	case XFER_MW_DMA_1:
>>>+	case XFER_MW_DMA_0:
>>>+	case XFER_PIO_4:

>>    MWDMA0/1 timings don't match PIO4, they are [much] slower.

> Oh thanks.  I will fix it.

    Just do not change PIO mode when selecitng DMA mode at all.

>>>+		hwif->select_data =
>>>+			(hwif->select_data & ~0xffff0000) | (value << 16);

>>    Why not just 0x0000ffff?

>>>+	else
>>>+		hwif->select_data = (hwif->select_data & ~0x0000ffff) | value;

>>    Why not just 0xffff0000?

> Indeed.

    Acltually, this is somewhat wrong WRT the programming the command PIO 
timings in the bits 8..10: they should be set to the same value (matching to 
the last "safest" PIO mode set) for both drives, so you should only "switch" 
bits 4 thru 7 of this register.

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello, I just wrote:

>    Acltually, this is somewhat wrong WRT the programming the command PIO 
> timings in the bits 8..10: they should be set to the same value 

    Hmm, probably isn't actually wrong, though usually IDE controllers have a 
single command timings register per channel.

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Wed, 10 Sep 2008 19:55:16 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > But the "Command Transfer Mode Select" bits affects access timings on
> > setting task registers for DMA command.
> 
>     So what? PIO and DMA are different protocols on IDE bus, so they shouldn't 
> affect each other. The IDE core will always tune the best PIO mode for you, so 
> the optimal command timings will be set.

Hmm, that would be a thing I had misunderstood.  I thought
set_pio_mode is not called when the drive was DMA capable.

Thank you for detailed review!

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Atsushi Nemoto wrote:

>>>But the "Command Transfer Mode Select" bits affects access timings on
>>>setting task registers for DMA command.

>>    So what? PIO and DMA are different protocols on IDE bus, so they shouldn't 
>>affect each other. The IDE core will always tune the best PIO mode for you, so 
>>the optimal command timings will be set.

> Hmm, that would be a thing I had misunderstood.  I thought
> set_pio_mode is not called when the drive was DMA capable.

    PIO autotuning was optional before (done only if the driver requested it 
via setting drive->autotune), but now done always.

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

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/port_ops routines.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>  

[...]

> 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..ba9776d
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,762 @@
> +#ifdef __BIG_ENDIAN
> +#define TX4939IDE_REG32(reg)	(TX4939IDE_##reg ^ 4)
> +#define TX4939IDE_REG16(reg)	(TX4939IDE_##reg ^ 6)
> +#define TX4939IDE_REG8(reg)	(TX4939IDE_##reg ^ 7)
> +#else
> +#define TX4939IDE_REG32(reg)	(TX4939IDE_##reg)
> +#define TX4939IDE_REG16(reg)	(TX4939IDE_##reg)
> +#define TX4939IDE_REG8(reg)	(TX4939IDE_##reg)
> +#endif
> +
> +#define TX4939IDE_readl(base, reg) \
> +	__raw_readl((void __iomem *)((base) + TX4939IDE_REG32(reg)))
> +#define TX4939IDE_readw(base, reg) \
> +	__raw_readw((void __iomem *)((base) + TX4939IDE_REG16(reg)))
> +#define TX4939IDE_readb(base, reg) \
> +	__raw_readb((void __iomem *)((base) + TX4939IDE_REG8(reg)))
> +#define TX4939IDE_writel(val, base, reg) \
> +	__raw_writel(val, (void __iomem *)((base) + TX4939IDE_REG32(reg)))
> +#define TX4939IDE_writew(val, base, reg) \
> +	__raw_writew(val, (void __iomem *)((base) + TX4939IDE_REG16(reg)))
> +#define TX4939IDE_writeb(val, base, reg) \
> +	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))
>   

   Why dont you #define __swizzle_addr_[bwlq]() in 
include/asm/mach/magle-port.h?
Or you never use read[bwlq]() accessorts for the SoC registers?

> +static void tx4939ide_check_error_ints(ide_hwif_t *hwif, u16 stat)
> +{
> +	if (stat & TX4939IDE_INT_BUSERR) {
> +		unsigned long base = TX4939IDE_BASE(hwif);
> +		/* reset FIFO */
> +		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) |
> +				 0x4000,
> +				 base, Sys_Ctl);
>   

   Are you sure bit 14 is self-clearing? The datashhet doesn't seem to 
say that...

> +static void tx4939ide_clear_irq(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif;
> +	unsigned long base;
> +	u16 ctl;
> +
> +	/*
> +	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
> +	 * jobs for DMA case.
> +	 */
> +	if (drive->waiting_for_dma)
> +		return;
> +	hwif = HWIF(drive);
> +	base = TX4939IDE_BASE(hwif);
>   

   I think you might cache the base address in hwif->extra_base to avoid 
masking with ~0xfff every time...

> +static u8 tx4939ide_cable_detect(ide_hwif_t *hwif)
> +{
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +
> +	return (TX4939IDE_readw(base, Sys_Ctl) & 0x2000)
> +		? ATA_CBL_PATA40 : ATA_CBL_PATA80;
>   

   Could you keep ? on the same line as the 1st operand?

> +static int __tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	struct request *rq = HWGROUP(drive)->rq;
> +	unsigned int reading;
> +	u8 dma_stat;
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +
> +	if (rq_data_dir(rq))
> +		reading = 0;
> +	else
> +		reading = 1 << 3;
> +
> +	/* fall back to pio! */
> +	if (!ide_build_dmatable(drive, rq)) {
> +		ide_map_sg(drive, rq);
> +		return 1;
> +	}
> +#ifdef __BIG_ENDIAN
> +	{
> +		unsigned int *table = hwif->dmatable_cpu;
> +		while (1) {
> +			cpu_to_le64s((u64 *)table);
> +			if (*table & 0x80000000)
> +				break;
> +			table += 2;
> +		}
> +	}
> +#endif
>   

   Ugh...

> +static int tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = HWIF(drive);
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	int is_slave = drive->dn & 1;
> +	unsigned int nframes;
> +	int rc, i;
> +	unsigned int sect_size = queue_hardsect_size(drive->queue);
> +	u16 select_data;
> +
> +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> +	TX4939IDE_writew(select_data, base, Sys_Ctl);
> +	if (is_slave)
> +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
> +	else
> +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
>   

	TX4939IDE_writew(sect_size / 2, base, is_slave ? Xfer_Cnt_2 : Xfer_Cnt_1);

> +	rc = __tx4939ide_dma_setup(drive);
> +	if (rc == 0) {
> +		/* Number of sectors to transfer. */
> +		nframes = 0;
> +		for (i = 0; i < hwif->sg_nents; i++)
> +			nframes += sg_dma_len(&hwif->sg_table[i]);
> +		BUG_ON(nframes % sect_size != 0);
> +		nframes /= sect_size;
> +		BUG_ON(nframes == 0);
> +		TX4939IDE_writew(nframes, base, Sec_Cnt);
>   

   Ugh, it looks much easier in my TC86C001 driver... doesn't 
hwgroup->rq->nr_sectors give you a number of 512 sectors?
Why bother with other (multiple of 512) sizes when you can always 
program transfer in 512-byte sectors? Or was I wrong there?

> +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))
> +		/* INT_IDE lost... bug? */
> +		return 0;
>   

   You shouldn't fake the BMDMA interrupt. If it's not there, it's not 
there. Or does this actually happen?

> +	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 = HWIF(drive);
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	u16 ctl = TX4939IDE_readw(base, int_ctl);
> +	u8 dma_stat, stat;
> +	u16 ide_int;
> +	int found = 0;
> +
> +	tx4939ide_check_error_ints(hwif, ctl);
> +	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, Alt_DevCtl);
> +		if ((stat & (ATA_BUSY|ATA_DRQ|ATA_ERR)) == ATA_ERR) {
> +			pr_err("%s: detect error %x in %s\n",
> +			       drive->name, stat, __func__);
> +			found = 1;
> +		}

   Again, you shouldn't fake the BMDMA interrupt... this is not needed.

> +		/* FALLTHRU */
> +	case TX4939IDE_INT_XFEREND:
> +		/*
> +		 * If only one of XFERINT and HOST was asserted, mask
> +		 * this interrupt and wait for an another one.  Note
>   

   This comment somewhat contradicts the code which returns 1 if only 
HOST interupt is asserted if ERR is set.

> +		 * that write to bit2 of DMA_stat will clear all
> +		 * mask bits.
> +		 */
> +		ctl |= ide_int << 8;
> +		break;
> +	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
> +		dma_stat = TX4939IDE_readb(base, DMA_stat);
> +		if (!(dma_stat & 4))
> +			pr_debug("%s: weired interrupt status. "
>   

   Weird.

> +				 "DMA_stat %#02x int_ctl %#04x\n",
> +				 hwif->name, dma_stat, ctl);
> +		found = 1;
>   

   No fakes -- unless that really happens. :-)

> +static void tx4939ide_hwif_init(ide_hwif_t *hwif)
> +{
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	int timeout;
> +
> +	/* Soft Reset */
> +	TX4939IDE_writew(0x8000, base, Sys_Ctl);
> +	mmiowb();
> +	udelay(1);	/* at least 20 UPSCLK (100ns for 200MHz GBUSCLK) */
> +	/* ATA Hard Reset */
> +	TX4939IDE_writew(0x0800, base, Sys_Ctl);
> +	timeout = 1000;
> +	while (TX4939IDE_readw(base, Sys_Ctl) & 0x0800) {
> +		if (timeout--)
> +			break;
> +		udelay(1);
> +	}
>   

   Don't do this -- there's nothing gained from the ATA hard reset but 
an extra delay; I removed such stuff from the TC86C001 driver. The IDE 
core will soft-reset the bus if needed...

> #ifdef __BIG_ENDIAN
> +/* custom iops (independent from SWAP_IO_SPACE) */
>   
> +static u8 mm_inb(unsigned long port)
> +{
> +	return (u8)readb((void __iomem *)port);
> +}
> +static void mm_outb(u8 value, unsigned long port)
> +{
> +	writeb(value, (void __iomem *)port);
> +}
> +static void mm_tf_load(ide_drive_t *drive, ide_task_t *task)
> +{
>   
[...]
> +	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE) {
> +		unsigned long base = TX4939IDE_BASE(hwif);
> +		mm_outb((tf->device & HIHI) | drive->select,
> +			 io_ports->device_addr);
>   

   I'm seeing no sense in re-defining so far...

> +		/* Fix ATA100 CORE System Control Register */
> +		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) & 0x07f0,
> +				 base, Sys_Ctl);
>   

   Ah... you're doing it here (but not in LE mode?). I think to avoid 
duplicating ide_tf_load() you need ot use selectproc().

> +	}
> +}
> +static void mm_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) {
>   

   I wish there was no such flag...

> +		u16 data;
> +
> +		data = __raw_readw((void __iomem *)io_ports->data_addr);
>   

    Ugh...

> +static void mm_insw_swap(unsigned long port, void *addr, u32 count)
> +{
> +	unsigned short *ptr = addr;
> +	unsigned long size = count * 2;
> +	port &= ~1;
> +	while (count--)
> +		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
> +	__ide_flush_dcache_range((unsigned long)addr, size);
>   

   Why is this needed BTW?

> +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,
>   

   Hum, it should be re-defined in both LE and BE mode (but actually not 
called anyway).

> +
> +	.set_irq		= ide_set_irq,
> +
> +	.tf_load		= mm_tf_load,
> +	.tf_read		= mm_tf_read,
> +
> +	.input_data		= mmio_input_data_swap,
> +	.output_data		= mmio_output_data_swap,
> +};
> +#endif	/* __BIG_ENDIAN */
> +
> +static const struct ide_port_info tx4939ide_port_info __initdata = {
> +	.init_hwif = tx4939ide_hwif_init,
> +	.init_dma = tx4939ide_init_dma,
> +	.port_ops = &tx4939ide_port_ops,
> +	.dma_ops = &tx4939ide_dma_ops,
> +#ifdef __BIG_ENDIAN
> +	.tp_ops = &tx4939ide_tp_ops,
> +#endif
> +	.host_flags = IDE_HFLAG_MMIO,
> +	.pio_mask = ATA_PIO4,
> +	.mwdma_mask = ATA_MWDMA2,
> +	.swdma_mask = ATA_SWDMA2,
>   

   No, SWDMA isn't supported.

> +	.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_REG8(DATA);
>   

   Wrong, should be TX4939IDE_REG16(). I wonder how it manages to work 
in BE mode with this...

> +#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];
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +
> +	tx4939ide_hwif_init(hwif);
>   

   ATA hard reset when coming out of suspend? Nice... :-)

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Thu, 11 Sep 2008 03:02:05 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > +#define TX4939IDE_readl(base, reg) \
> > +	__raw_readl((void __iomem *)((base) + TX4939IDE_REG32(reg)))
> > +#define TX4939IDE_readw(base, reg) \
> > +	__raw_readw((void __iomem *)((base) + TX4939IDE_REG16(reg)))
> > +#define TX4939IDE_readb(base, reg) \
> > +	__raw_readb((void __iomem *)((base) + TX4939IDE_REG8(reg)))
> > +#define TX4939IDE_writel(val, base, reg) \
> > +	__raw_writel(val, (void __iomem *)((base) + TX4939IDE_REG32(reg)))
> > +#define TX4939IDE_writew(val, base, reg) \
> > +	__raw_writew(val, (void __iomem *)((base) + TX4939IDE_REG16(reg)))
> > +#define TX4939IDE_writeb(val, base, reg) \
> > +	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))
> >   
> 
>    Why dont you #define __swizzle_addr_[bwlq]() in 
> include/asm/mach/magle-port.h?
> Or you never use read[bwlq]() accessorts for the SoC registers?

Because __swizzle_addr_[bwlq]() affects _all_ device including PCI
devices.  I hope all PCI driver works as is, so put all dirty things
in platform specific driver ;-)

> > +static void tx4939ide_check_error_ints(ide_hwif_t *hwif, u16 stat)
> > +{
> > +	if (stat & TX4939IDE_INT_BUSERR) {
> > +		unsigned long base = TX4939IDE_BASE(hwif);
> > +		/* reset FIFO */
> > +		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) |
> > +				 0x4000,
> > +				 base, Sys_Ctl);
> >   
> 
>    Are you sure bit 14 is self-clearing? The datashhet doesn't seem to 
> say that...

Well, I cannot remember...  I thought I checked that bit cleard by
reading it, but actually the bit is write-only.  Maybe clearing
explicitly would be a safe bet.

> > +	hwif = HWIF(drive);
> > +	base = TX4939IDE_BASE(hwif);
> >   
> 
>    I think you might cache the base address in hwif->extra_base to avoid 
> masking with ~0xfff every time...

OK, I will try it.

> > +static u8 tx4939ide_cable_detect(ide_hwif_t *hwif)
> > +{
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +
> > +	return (TX4939IDE_readw(base, Sys_Ctl) & 0x2000)
> > +		? ATA_CBL_PATA40 : ATA_CBL_PATA80;
> >   
> 
>    Could you keep ? on the same line as the 1st operand?

OK.

> > +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> > +	TX4939IDE_writew(select_data, base, Sys_Ctl);
> > +	if (is_slave)
> > +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
> > +	else
> > +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
> >   
> 
> 	TX4939IDE_writew(sect_size / 2, base, is_slave ? Xfer_Cnt_2 : Xfer_Cnt_1);

OK.

> > +	rc = __tx4939ide_dma_setup(drive);
> > +	if (rc == 0) {
> > +		/* Number of sectors to transfer. */
> > +		nframes = 0;
> > +		for (i = 0; i < hwif->sg_nents; i++)
> > +			nframes += sg_dma_len(&hwif->sg_table[i]);
> > +		BUG_ON(nframes % sect_size != 0);
> > +		nframes /= sect_size;
> > +		BUG_ON(nframes == 0);
> > +		TX4939IDE_writew(nframes, base, Sec_Cnt);
> >   
> 
>    Ugh, it looks much easier in my TC86C001 driver... doesn't 
> hwgroup->rq->nr_sectors give you a number of 512 sectors?
> Why bother with other (multiple of 512) sizes when you can always 
> program transfer in 512-byte sectors? Or was I wrong there?

Hmm.  Good idea.  I will try it.

> > +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))
> > +		/* INT_IDE lost... bug? */
> > +		return 0;
> >   
> 
>    You shouldn't fake the BMDMA interrupt. If it's not there, it's not 
> there. Or does this actually happen?

IIRC, Yes.

> > +		/*
> > +		 * If only one of XFERINT and HOST was asserted, mask
> > +		 * this interrupt and wait for an another one.  Note
> >   
> 
>    This comment somewhat contradicts the code which returns 1 if only 
> HOST interupt is asserted if ERR is set.

Indeed.  I will make the comment more precise.

> > +	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
> > +		dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +		if (!(dma_stat & 4))
> > +			pr_debug("%s: weired interrupt status. "
> >   
> 
>    Weird.

Sure.  But it can happen IIRC...

> > +static void tx4939ide_hwif_init(ide_hwif_t *hwif)
> > +{
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	int timeout;
> > +
> > +	/* Soft Reset */
> > +	TX4939IDE_writew(0x8000, base, Sys_Ctl);
> > +	mmiowb();
> > +	udelay(1);	/* at least 20 UPSCLK (100ns for 200MHz GBUSCLK) */
> > +	/* ATA Hard Reset */
> > +	TX4939IDE_writew(0x0800, base, Sys_Ctl);
> > +	timeout = 1000;
> > +	while (TX4939IDE_readw(base, Sys_Ctl) & 0x0800) {
> > +		if (timeout--)
> > +			break;
> > +		udelay(1);
> > +	}
> 
>    Don't do this -- there's nothing gained from the ATA hard reset but 
> an extra delay; I removed such stuff from the TC86C001 driver. The IDE 
> core will soft-reset the bus if needed...

OK.

> > #ifdef __BIG_ENDIAN
> > +/* custom iops (independent from SWAP_IO_SPACE) */
> >   
> > +static u8 mm_inb(unsigned long port)
> > +{
> > +	return (u8)readb((void __iomem *)port);
> > +}
> > +static void mm_outb(u8 value, unsigned long port)
> > +{
> > +	writeb(value, (void __iomem *)port);
> > +}
> > +static void mm_tf_load(ide_drive_t *drive, ide_task_t *task)
> > +{
> >   
> [...]
> > +	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE) {
> > +		unsigned long base = TX4939IDE_BASE(hwif);
> > +		mm_outb((tf->device & HIHI) | drive->select,
> > +			 io_ports->device_addr);
> >   
> 
>    I'm seeing no sense in re-defining so far...
> 
> > +		/* Fix ATA100 CORE System Control Register */
> > +		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) & 0x07f0,
> > +				 base, Sys_Ctl);
> >   
> 
>    Ah... you're doing it here (but not in LE mode?). I think to avoid 
> duplicating ide_tf_load() you need ot use selectproc().

Oh, my fault.  LE mode also needs this fix.  I still need ide_tf_load
on BE mode to support IDE_TFLAG_OUT_DATA.

> > +static void mm_insw_swap(unsigned long port, void *addr, u32 count)
> > +{
> > +	unsigned short *ptr = addr;
> > +	unsigned long size = count * 2;
> > +	port &= ~1;
> > +	while (count--)
> > +		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
> > +	__ide_flush_dcache_range((unsigned long)addr, size);
> >   
> 
>    Why is this needed BTW?

Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
inconsistency on PIO drive.  PIO transfer only writes to cache but
upper layers expects the data is in main memory.

> > +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,
> >   
> 
>    Hum, it should be re-defined in both LE and BE mode (but actually not 
> called anyway).

What do you mean?  Please elaborate?

> > +	.host_flags = IDE_HFLAG_MMIO,
> > +	.pio_mask = ATA_PIO4,
> > +	.mwdma_mask = ATA_MWDMA2,
> > +	.swdma_mask = ATA_SWDMA2,
> >   
> 
>    No, SWDMA isn't supported.

Oh, indeed.

> > +	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_REG8(DATA);
> >   
> 
>    Wrong, should be TX4939IDE_REG16(). I wonder how it manages to work 
> in BE mode with this...

Well, "port &= ~1" in mm_insw_swap() and mm_outsw_swap do the trick.

> > +#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];
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +
> > +	tx4939ide_hwif_init(hwif);
> >   
> 
>    ATA hard reset when coming out of suspend? Nice... :-)

Will be fixed in tx4939ide_hwif_init().


Thanks!

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Atsushi Nemoto wrote:

>>>+static void tx4939ide_check_error_ints(ide_hwif_t *hwif, u16 stat)
>>>+{
>>>+	if (stat & TX4939IDE_INT_BUSERR) {
>>>+		unsigned long base = TX4939IDE_BASE(hwif);
>>>+		/* reset FIFO */
>>>+		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) |
>>>+				 0x4000,
>>>+				 base, Sys_Ctl);

>>   Are you sure bit 14 is self-clearing? The datashhet doesn't seem to 
>>say that...

> Well, I cannot remember...  I thought I checked that bit cleard by
> reading it, but actually the bit is write-only.  Maybe clearing
> explicitly would be a safe bet.

    It's also write-only on TC86C001, and the original driver (as well as 
mine) cleared it explicitly.

>>>+	rc = __tx4939ide_dma_setup(drive);
>>>+	if (rc == 0) {
>>>+		/* Number of sectors to transfer. */
>>>+		nframes = 0;
>>>+		for (i = 0; i < hwif->sg_nents; i++)
>>>+			nframes += sg_dma_len(&hwif->sg_table[i]);
>>>+		BUG_ON(nframes % sect_size != 0);
>>>+		nframes /= sect_size;
>>>+		BUG_ON(nframes == 0);
>>>+		TX4939IDE_writew(nframes, base, Sec_Cnt);

>>   Ugh, it looks much easier in my TC86C001 driver... doesn't 
>>hwgroup->rq->nr_sectors give you a number of 512 sectors?
>>Why bother with other (multiple of 512) sizes when you can always 
>>program transfer in 512-byte sectors? Or was I wrong there?

    Anyway, the TX3939 datasheet says that sector size must be a multiple of 
256 words when transferring more than 1 sector.

> Hmm.  Good idea.  I will try it.

    At least it worked with a CD-ROM for me. :-)

>>>+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))
>>>+		/* INT_IDE lost... bug? */
>>>+		return 0;

>>   You shouldn't fake the BMDMA interrupt. If it's not there, it's not 
>>there. Or does this actually happen?

> IIRC, Yes.

    Hum, let me think... worth printing a message if this happens.

>>>+		/*
>>>+		 * If only one of XFERINT and HOST was asserted, mask
>>>+		 * this interrupt and wait for an another one.  Note

>>   This comment somewhat contradicts the code which returns 1 if only 
>>HOST interupt is asserted if ERR is set.

    Which is not its business to test. I think you should remove that above 
check -- if there's INTRQ asserted, then it's asserted. I wonder if BMIDE 
interrupt bit gets set in that case (suspecting it's not)...

> Indeed.  I will make the comment more precise.

>>>+	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
>>>+		dma_stat = TX4939IDE_readb(base, DMA_stat);
>>>+		if (!(dma_stat & 4))
>>>+			pr_debug("%s: weired interrupt status. "
>>>  

>>   Weird.

> Sure.  But it can happen IIRC...

    I meant the typo. :-)

>>>#ifdef __BIG_ENDIAN
>>>+/* custom iops (independent from SWAP_IO_SPACE) */
>>>  
>>>+static u8 mm_inb(unsigned long port)
>>>+{
>>>+	return (u8)readb((void __iomem *)port);
>>>+}
>>>+static void mm_outb(u8 value, unsigned long port)
>>>+{
>>>+	writeb(value, (void __iomem *)port);
>>>+}
>>>+static void mm_tf_load(ide_drive_t *drive, ide_task_t *task)
>>>+{
>>>  

>>[...]

>>>+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE) {
>>>+		unsigned long base = TX4939IDE_BASE(hwif);
>>>+		mm_outb((tf->device & HIHI) | drive->select,
>>>+			 io_ports->device_addr);

>>   I'm seeing no sense in re-defining so far...

>>>+		/* Fix ATA100 CORE System Control Register */
>>>+		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) & 0x07f0,
>>>+				 base, Sys_Ctl);

>>   Ah... you're doing it here (but not in LE mode?). I think to avoid 
>>duplicating ide_tf_load() you need ot use selectproc().

> Oh, my fault.  LE mode also needs this fix.  I still need ide_tf_load
> on BE mode to support IDE_TFLAG_OUT_DATA.

   Yeah, that totally useless flag...

>>>+static void mm_insw_swap(unsigned long port, void *addr, u32 count)
>>>+{
>>>+	unsigned short *ptr = addr;
>>>+	unsigned long size = count * 2;
>>>+	port &= ~1;
>>>+	while (count--)
>>>+		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
>>>+	__ide_flush_dcache_range((unsigned long)addr, size);

>>   Why is this needed BTW?

> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> inconsistency on PIO drive.  PIO transfer only writes to cache but
> upper layers expects the data is in main memory.

    Hum, then I wonder why it's MIPS specific...

>>>+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,

>>   Hum, it should be re-defined in both LE and BE mode (but actually not 
>>called anyway).

> What do you mean?  Please elaborate?

    I mean that in LE mode you're using ide_read_sff_dma_status() but not 
setting hwif->dma_base, so it won't work. But since it shouldn't be called in 
this driver's case, this doesn't hurt.

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> >>   You shouldn't fake the BMDMA interrupt. If it's not there, it's not 
> >>there. Or does this actually happen?
> 
> > IIRC, Yes.
> 
>     Hum, let me think... worth printing a message if this happens.

It might be.  I'll try it.

> >>>+		/*
> >>>+		 * If only one of XFERINT and HOST was asserted, mask
> >>>+		 * this interrupt and wait for an another one.  Note
> 
> >>   This comment somewhat contradicts the code which returns 1 if only 
> >>HOST interupt is asserted if ERR is set.
> 
>     Which is not its business to test. I think you should remove that above 
> check -- if there's INTRQ asserted, then it's asserted. I wonder if BMIDE 
> interrupt bit gets set in that case (suspecting it's not)...

Well, let me explain a bit.  The datasheed say I should wait _both_
XFERINT and HOST interrupt.  So, if only one of them was asserted, I
mask it and wait another one.  But on the error case, only HOST was
asserted and XFERINT was never asserted.  Then I could not exit from
"waiting another one" state, until timeout.

> >>>+			pr_debug("%s: weired interrupt status. "
> >>>  
> 
> >>   Weird.
> 
> > Sure.  But it can happen IIRC...
> 
>     I meant the typo. :-)

Oh, thanks!

> >>>+	__ide_flush_dcache_range((unsigned long)addr, size);
> 
> >>   Why is this needed BTW?
> 
> > Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> > inconsistency on PIO drive.  PIO transfer only writes to cache but
> > upper layers expects the data is in main memory.
> 
>     Hum, then I wonder why it's MIPS specific...

SPARC also have it.  And there were some discussions for ARM IIRC.

> >>>+	.read_sff_dma_status	= tx4939ide_read_sff_dma_status,
> 
> >>   Hum, it should be re-defined in both LE and BE mode (but actually not 
> >>called anyway).
> 
> > What do you mean?  Please elaborate?
> 
>     I mean that in LE mode you're using ide_read_sff_dma_status() but not 
> setting hwif->dma_base, so it won't work. But since it shouldn't be called in 
> this driver's case, this doesn't hurt.

Yes, I see, thanks.

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Atsushi Nemoto wrote:

>>>>>+		/*
>>>>>+		 * If only one of XFERINT and HOST was asserted, mask
>>>>>+		 * this interrupt and wait for an another one.  Note

>>>>  This comment somewhat contradicts the code which returns 1 if only 
>>>>HOST interupt is asserted if ERR is set.

>>    Which is not its business to test. I think you should remove that above 
>>check -- if there's INTRQ asserted, then it's asserted. I wonder if BMIDE 
>>interrupt bit gets set in that case (suspecting it's not)...

> Well, let me explain a bit.  The datasheed say I should wait _both_
> XFERINT and HOST interrupt.  So, if only one of them was asserted, I

    Since this is SFF-8038i compatible, you should first check if the spec'ed
interrupt status bit (called IDE_INT here) is set. If it isn't [always] get
set when it should, you may check for other interrupt bits (XFEREND/HOST/etc.)
and take the necessary measures if you detect interrupt on them.

> mask it and wait another one.  But on the error case, only HOST was
> asserted and XFERINT was never asserted.

    And I suspect that IDE_INT also doesn't get set, right? The check for 
ERR=1 however is not needed. If the drive does assert its interrupt signal 
(INTRQ), it knows better why.

> Then I could not exit from "waiting another one" state, until timeout.

    Well, at least most of the other [PCI] drivers will wait for timeout in 
the case of the DMA status register bit 2 (IDE_INT here) not set -- despite 
some IDE chips do have bits indicating the INTRQ status from IDE bus. But 
usually, INTRQ still comes thru to this bit even if a command ends in error... 
This isn't the case here?

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Atsushi Nemoto wrote:

>>>>>+		/*
>>>>>+		 * If only one of XFERINT and HOST was asserted, mask
>>>>>+		 * this interrupt and wait for an another one.  Note

>>>>  This comment somewhat contradicts the code which returns 1 if only 
>>>>HOST interupt is asserted if ERR is set.

>>    Which is not its business to test. I think you should remove that above 
>>check -- if there's INTRQ asserted, then it's asserted. I wonder if BMIDE 
>>interrupt bit gets set in that case (suspecting it's not)...

> Well, let me explain a bit.  The datasheed say I should wait _both_
> XFERINT and HOST interrupt.  So, if only one of them was asserted, I
> mask it and wait another one.  But on the error case, only HOST was
> asserted and XFERINT was never asserted.  Then I could not exit from
> "waiting another one" state, until timeout.

    Hmm, I got it: you decide whether it's worth waiting more for XFEREND 
interrupt based on whether ERR is set or not. I suppose IDE_INT doesn't get 
set in case the command gets endede with ERR set?

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Fri, 12 Sep 2008 21:19:20 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > Well, let me explain a bit.  The datasheed say I should wait _both_
> > XFERINT and HOST interrupt.  So, if only one of them was asserted, I
> > mask it and wait another one.  But on the error case, only HOST was
> > asserted and XFERINT was never asserted.  Then I could not exit from
> > "waiting another one" state, until timeout.
> 
>     Hmm, I got it: you decide whether it's worth waiting more for XFEREND 
> interrupt based on whether ERR is set or not. I suppose IDE_INT doesn't get 
> set in case the command gets endede with ERR set?

IIRC, yes.  And anyway, the interrupt signal from this controller to
CPU is not asserted because HOSTINT was masked by int_ctl register to
wait for XFERINT interrupt.

So, regardless of IDE_INT was set or not, no more interrupt raised to
CPU.

Many of strangeness of interrupt handling in this driver is based on
the fact that the IDE_INT bit in DMA status register does not refrect
the controllers interrupt status directly.  And the implementation of
the IDE_INT bit is actually broken.  Claring the IDE_INT bit also
clears all mask bits in int_ctl registers.  Usually this sort of
behaviour is called "bug". ;)

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Atsushi Nemoto wrote:

>>> Well, let me explain a bit.  The datasheed say I should wait _both_
>>> XFERINT and HOST interrupt.  So, if only one of them was asserted, I
>>> mask it and wait another one.  But on the error case, only HOST was
>>> asserted and XFERINT was never asserted.  Then I could not exit from
>>> "waiting another one" state, until timeout.
>>>       
>>     Hmm, I got it: you decide whether it's worth waiting more for XFEREND 
>> interrupt based on whether ERR is set or not. I suppose IDE_INT doesn't get 
>> set in case the command gets endede with ERR set?
>>     
>
> IIRC, yes.  And anyway, the interrupt signal from this controller to
>   

Thats wrong -- According t the spec. the bit should be set following any 
assertion of INTRQ on IDE bus (possibly not at once though -- after 
flushing FIFO). Well, no wonder with such description of the bits as:


INT_IDE (RWC) [Interrupt]
Is “1” when data transfer completes. This bit is cleared by writing “1” 
to it.
When this bit is set to ‘1’, the following bits of the ATA Interrupt 
Controller Register will be
reset: bits [15:8] (Mask Address Error INT, Mask Reach Multiple INT, 
Mask DEV
Timing Error, Mask Ultra DMA DEV Terminate, Mask Timer INT, Mask Bus 
Error, Mask
Data Transfer End, Mask Host INT), and bits [1:0] (Data Transfer End, 
Host INT).

> CPU is not asserted because HOSTINT was masked by int_ctl register to
> wait for XFERINT interrupt.
>
> So, regardless of IDE_INT was set or not, no more interrupt raised to
> CPU.
>   

Ah, it gets purposedly masked out...

> Many of strangeness of interrupt handling in this driver is based on
> the fact that the IDE_INT bit in DMA status register does not refrect
> the controllers interrupt status directly.

It also seems to reflect the wrong status, i.e. that of the XFEREND 
interrupt...

> And the implementation of
> the IDE_INT bit is actually broken.  Claring the IDE_INT bit also
> clears all mask bits in int_ctl registers.  Usually this sort of
> behaviour is called "bug". ;)
>   

Hm, I thought that was done on purpose to "accelerate" interrupt 
handling or something... it can help indeed if you're not masking those 
interrupts.

> ---
> Atsushi Nemoto
>   

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello, I wrote:

> Thats wrong -- According t the spec. the bit should be set following 
> any assertion of INTRQ on IDE bus (possibly not at once though -- 
> after flushing FIFO). Well, no wonder with such description of the 
> bits as:
>
>
> INT_IDE (RWC) [Interrupt]
> Is “1” when data transfer completes. This bit is cleared by writing 
> “1” to it.
> When this bit is set to ‘1’, the following bits of the ATA Interrupt 
> Controller Register will be
> reset: bits [15:8] (Mask Address Error INT, Mask Reach Multiple INT, 
> Mask DEV
> Timing Error, Mask Ultra DMA DEV Terminate, Mask Timer INT, Mask Bus 
> Error, Mask
> Data Transfer End, Mask Host INT), and bits [1:0] (Data Transfer End, 
> Host INT).

   Forgot to mentiom that from this description it's not even clear if 
the int_ctl register bits are cleared when 1 is written to this bit or 
when the controller sets it. :-)

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Bartlomiej Zolnierkiewicz]

On Friday 12 September 2008, Atsushi Nemoto wrote:
> On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

[...]

> > >>>+	__ide_flush_dcache_range((unsigned long)addr, size);
> > 
> > >>   Why is this needed BTW?
> > 
> > > Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> > > inconsistency on PIO drive.  PIO transfer only writes to cache but
> > > upper layers expects the data is in main memory.
> > 
> >     Hum, then I wonder why it's MIPS specific...
> 
> SPARC also have it.  And there were some discussions for ARM IIRC.

I was under the impression that it has been addressed by Tejun at
the higher-layer level (for both ide/libata) long time ago and that
MIPS/SPARC code are just a left-overs which could be removed now?

Thanks,
Bart

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> On Friday 12 September 2008, Atsushi Nemoto wrote:
>> On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> 
> [...]
> 
>>>>>> +	__ide_flush_dcache_range((unsigned long)addr, size);
>>>>>   Why is this needed BTW?
>>>> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
>>>> inconsistency on PIO drive.  PIO transfer only writes to cache but
>>>> upper layers expects the data is in main memory.
>>>     Hum, then I wonder why it's MIPS specific...
>> SPARC also have it.  And there were some discussions for ARM IIRC.
> 
> I was under the impression that it has been addressed by Tejun at
> the higher-layer level (for both ide/libata) long time ago and that
> MIPS/SPARC code are just a left-overs which could be removed now?

cc'ing Jens and James.  IIRC, I posted several patches but they never
went in.  I don't remember what the objections were or whether any
alternative fix went in.

Thanks.

-- 
tejun

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Sun, 28 Sep 2008 07:09:35 +0900, Tejun Heo <htejun@gmail.com> wrote:
> >>>> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> >>>> inconsistency on PIO drive.  PIO transfer only writes to cache but
> >>>> upper layers expects the data is in main memory.
> >>>     Hum, then I wonder why it's MIPS specific...
> >> SPARC also have it.  And there were some discussions for ARM IIRC.
> > 
> > I was under the impression that it has been addressed by Tejun at
> > the higher-layer level (for both ide/libata) long time ago and that
> > MIPS/SPARC code are just a left-overs which could be removed now?
> 
> cc'ing Jens and James.  IIRC, I posted several patches but they never
> went in.  I don't remember what the objections were or whether any
> alternative fix went in.

I suppose you mean thread http://lkml.org/lkml/2006/1/13/156.

IIUC flushing in ide string ops is still needed for MIPS.

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[James Bottomley]

On Sun, 2008-09-28 at 07:09 +0900, Tejun Heo wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Friday 12 September 2008, Atsushi Nemoto wrote:
> >> On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > 
> > [...]
> > 
> >>>>>> +	__ide_flush_dcache_range((unsigned long)addr, size);
> >>>>>   Why is this needed BTW?
> >>>> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> >>>> inconsistency on PIO drive.  PIO transfer only writes to cache but
> >>>> upper layers expects the data is in main memory.
> >>>     Hum, then I wonder why it's MIPS specific...
> >> SPARC also have it.  And there were some discussions for ARM IIRC.
> > 
> > I was under the impression that it has been addressed by Tejun at
> > the higher-layer level (for both ide/libata) long time ago and that
> > MIPS/SPARC code are just a left-overs which could be removed now?
> 
> cc'ing Jens and James.  IIRC, I posted several patches but they never
> went in.  I don't remember what the objections were or whether any
> alternative fix went in.

Which patches were these?  We have several methods of doing PIO
fallback, the most common one being
scatterlist.c:sg_copy_from/to_buffer() which does the cache coherency.

James


> Thanks.
> 

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Tejun Heo]

James Bottomley wrote:
> On Sun, 2008-09-28 at 07:09 +0900, Tejun Heo wrote:
>> Bartlomiej Zolnierkiewicz wrote:
>>> On Friday 12 September 2008, Atsushi Nemoto wrote:
>>>> On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
>>> [...]
>>>
>>>>>>>> +	__ide_flush_dcache_range((unsigned long)addr, size);
>>>>>>>   Why is this needed BTW?
>>>>>> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
>>>>>> inconsistency on PIO drive.  PIO transfer only writes to cache but
>>>>>> upper layers expects the data is in main memory.
>>>>>     Hum, then I wonder why it's MIPS specific...
>>>> SPARC also have it.  And there were some discussions for ARM IIRC.
>>> I was under the impression that it has been addressed by Tejun at
>>> the higher-layer level (for both ide/libata) long time ago and that
>>> MIPS/SPARC code are just a left-overs which could be removed now?
>> cc'ing Jens and James.  IIRC, I posted several patches but they never
>> went in.  I don't remember what the objections were or whether any
>> alternative fix went in.
> 
> Which patches were these?  We have several methods of doing PIO
> fallback, the most common one being
> scatterlist.c:sg_copy_from/to_buffer() which does the cache coherency.

The thread Atsushi found seems to be the correct one.

  http://lkml.org/lkml/2006/1/13/156

Thanks.

-- 
tejun

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Jens Axboe]

On Sat, Oct 04 2008, Tejun Heo wrote:
> James Bottomley wrote:
> > On Sun, 2008-09-28 at 07:09 +0900, Tejun Heo wrote:
> >> Bartlomiej Zolnierkiewicz wrote:
> >>> On Friday 12 September 2008, Atsushi Nemoto wrote:
> >>>> On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> >>> [...]
> >>>
> >>>>>>>> +	__ide_flush_dcache_range((unsigned long)addr, size);
> >>>>>>>   Why is this needed BTW?
> >>>>>> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> >>>>>> inconsistency on PIO drive.  PIO transfer only writes to cache but
> >>>>>> upper layers expects the data is in main memory.
> >>>>>     Hum, then I wonder why it's MIPS specific...
> >>>> SPARC also have it.  And there were some discussions for ARM IIRC.
> >>> I was under the impression that it has been addressed by Tejun at
> >>> the higher-layer level (for both ide/libata) long time ago and that
> >>> MIPS/SPARC code are just a left-overs which could be removed now?
> >> cc'ing Jens and James.  IIRC, I posted several patches but they never
> >> went in.  I don't remember what the objections were or whether any
> >> alternative fix went in.
> > 
> > Which patches were these?  We have several methods of doing PIO
> > fallback, the most common one being
> > scatterlist.c:sg_copy_from/to_buffer() which does the cache coherency.
> 
> The thread Atsushi found seems to be the correct one.
> 
>   http://lkml.org/lkml/2006/1/13/156
> 
> Thanks.

I agreed to them last time... Shall we get this merged up?

-- 
Jens Axboe

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Ralf Baechle]

On Sat, Sep 27, 2008 at 06:19:19PM +0200, Bartlomiej Zolnierkiewicz wrote:

> > > >>>+	__ide_flush_dcache_range((unsigned long)addr, size);
> > > 
> > > >>   Why is this needed BTW?
> > > 
> > > > Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> > > > inconsistency on PIO drive.  PIO transfer only writes to cache but
> > > > upper layers expects the data is in main memory.
> > > 
> > >     Hum, then I wonder why it's MIPS specific...
> > 
> > SPARC also have it.  And there were some discussions for ARM IIRC.

It should affect any architecture that has virtually indexed data caches
with aliases.

> I was under the impression that it has been addressed by Tejun at
> the higher-layer level (for both ide/libata) long time ago and that
> MIPS/SPARC code are just a left-overs which could be removed now?

I'd highly appreciate that.  The __ide_ins? / __ide_outs? ops don't know
if a page is mapped to userspace so will have to do unnecessary flushes.
A mechanism that allows flush_dcache_page to be used would be far
preferable.

  Ralf

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

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/port_ops routines.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>   
[...]
> diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
> new file mode 100644
> index 0000000..ba9776d
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,762 @@
>   
[...]
> +static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
> +{
> +	ide_hwif_t *hwif	= HWIF(drive);
> +	u8 unit			= drive->dn & 1;
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	u8 dma_stat = TX4939IDE_readb(base, DMA_stat);
> +
> +	if (on)
> +		dma_stat |= (1 << (5 + unit));
> +	else
> +		dma_stat &= ~(1 << (5 + unit));
> +
> +	TX4939IDE_writeb(dma_stat, base, DMA_stat);
> +}
>   

   BTW, you could save on using ide_dma_host_set() in LE mode if you'd 
set hwif->dma_base properly...

> +static int __tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	struct request *rq = HWGROUP(drive)->rq;
> +	unsigned int reading;
> +	u8 dma_stat;
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +
[...]
> +
> +	/* read DMA status for INTR & ERROR flags */
> +	dma_stat = TX4939IDE_readb(base, DMA_stat);
> +
> +	/* clear INTR & ERROR flags */
> +	TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
> +	/* recover intmask cleared by writing to bit2 of DMA_stat */
> +	TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
>   

   I think it might be worth factoring the BMDMA status clearing code 
into a separate function...

> +
> +	drive->waiting_for_dma = 1;
> +	return 0;
> +}
> +
> +static int tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = HWIF(drive);
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	int is_slave = drive->dn & 1;
> +	unsigned int nframes;
> +	int rc, i;
> +	unsigned int sect_size = queue_hardsect_size(drive->queue);
> +	u16 select_data;
> +
> +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> +	TX4939IDE_writew(select_data, base, Sys_Ctl);
> +	if (is_slave)
> +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
> +	else
> +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
> +
> +	rc = __tx4939ide_dma_setup(drive);
>   

   Hm, I think you need to call this earlier and do an early exit if it 
returns 1.

> +	if (rc == 0) {
>   

   Better check for !=0, return early and avoid unnecessary 
indentatiuon, isn't it?

> +		/* Number of sectors to transfer. */
> +		nframes = 0;
> +		for (i = 0; i < hwif->sg_nents; i++)
> +			nframes += sg_dma_len(&hwif->sg_table[i]);
> +		BUG_ON(nframes % sect_size != 0);
> +		nframes /= sect_size;
> +		BUG_ON(nframes == 0);
> +		TX4939IDE_writew(nframes, base, Sec_Cnt);
> +	}
> +	return rc;
> +}
> +
> +static void tx4939ide_dma_start(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +	u8 dma_cmd;
> +
> +	/* Note that this is done *after* the cmd has
> +	 * been issued to the drive, as per the BM-IDE spec.
> +	 */
> +	dma_cmd = TX4939IDE_readb(base, DMA_Cmd);
> +	/* start DMA */
> +	TX4939IDE_writeb(dma_cmd | 1, base, DMA_Cmd);
> +
> +	wmb();
> +}
>   

   You could save by using ide_dma_start() in LE mode too...

> +#ifdef __BIG_ENDIAN
> +/* custom iops (independent from SWAP_IO_SPACE) */
> +static u8 mm_inb(unsigned long port)
> +{
> +	return (u8)readb((void __iomem *)port);
> +}
> +static void mm_outb(u8 value, unsigned long port)
> +{
> +	writeb(value, (void __iomem *)port);
> +}
>   

   I'm not sure readb()/writeb() are good substitute for 
__raw_readb()/__raw_writeb() as __swizzle_addr_b() might be actually 
changing the address...

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Fri, 12 Sep 2008 02:33:12 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > +static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
> > +{
> > +	ide_hwif_t *hwif	= HWIF(drive);
> > +	u8 unit			= drive->dn & 1;
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	u8 dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +
> > +	if (on)
> > +		dma_stat |= (1 << (5 + unit));
> > +	else
> > +		dma_stat &= ~(1 << (5 + unit));
> > +
> > +	TX4939IDE_writeb(dma_stat, base, DMA_stat);
> > +}
> >   
> 
>    BTW, you could save on using ide_dma_host_set() in LE mode if you'd 
> set hwif->dma_base properly...

Yes.  I like endian-free approach in general, but there is already
some ifdefs in this driver.  I have no strong opinion here.

> > +static int __tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = drive->hwif;
> > +	struct request *rq = HWGROUP(drive)->rq;
> > +	unsigned int reading;
> > +	u8 dma_stat;
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +
> [...]
> > +
> > +	/* read DMA status for INTR & ERROR flags */
> > +	dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +
> > +	/* clear INTR & ERROR flags */
> > +	TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
> > +	/* recover intmask cleared by writing to bit2 of DMA_stat */
> > +	TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
> >   
> 
>    I think it might be worth factoring the BMDMA status clearing code 
> into a separate function...

OK.  Good idea.

> > +static int tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = HWIF(drive);
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	int is_slave = drive->dn & 1;
> > +	unsigned int nframes;
> > +	int rc, i;
> > +	unsigned int sect_size = queue_hardsect_size(drive->queue);
> > +	u16 select_data;
> > +
> > +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> > +	TX4939IDE_writew(select_data, base, Sys_Ctl);
> > +	if (is_slave)
> > +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
> > +	else
> > +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
> > +
> > +	rc = __tx4939ide_dma_setup(drive);
> 
>    Hm, I think you need to call this earlier and do an early exit if it 
> returns 1.
> 
> > +	if (rc == 0) {
> >   
> 
>    Better check for !=0, return early and avoid unnecessary 
> indentatiuon, isn't it?

It mighet be better.  I'll try it.

> > +static void tx4939ide_dma_start(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = drive->hwif;
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	u8 dma_cmd;
> > +
> > +	/* Note that this is done *after* the cmd has
> > +	 * been issued to the drive, as per the BM-IDE spec.
> > +	 */
> > +	dma_cmd = TX4939IDE_readb(base, DMA_Cmd);
> > +	/* start DMA */
> > +	TX4939IDE_writeb(dma_cmd | 1, base, DMA_Cmd);
> > +
> > +	wmb();
> > +}
> >   
> 
>    You could save by using ide_dma_start() in LE mode too...

Ditto.

> > +#ifdef __BIG_ENDIAN
> > +/* custom iops (independent from SWAP_IO_SPACE) */
> > +static u8 mm_inb(unsigned long port)
> > +{
> > +	return (u8)readb((void __iomem *)port);
> > +}
> > +static void mm_outb(u8 value, unsigned long port)
> > +{
> > +	writeb(value, (void __iomem *)port);
> > +}
> >   
> 
>    I'm not sure readb()/writeb() are good substitute for 
> __raw_readb()/__raw_writeb() as __swizzle_addr_b() might be actually 
> changing the address...

__swizzle_addr_b() used for both readb() and __raw_readb().  ioswabb()
is used for readb() and __raw_ioswabb() is used for __raw_readb().
Hm, __raw_readb() might be better for consistency, though I cannot
imagine any custom ioswabb() ;-)

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Atsushi Nemoto wrote:

>>>+static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
>>>+{
>>>+	ide_hwif_t *hwif	= HWIF(drive);
>>>+	u8 unit			= drive->dn & 1;
>>>+	unsigned long base = TX4939IDE_BASE(hwif);
>>>+	u8 dma_stat = TX4939IDE_readb(base, DMA_stat);
>>>+
>>>+	if (on)
>>>+		dma_stat |= (1 << (5 + unit));
>>>+	else
>>>+		dma_stat &= ~(1 << (5 + unit));
>>>+
>>>+	TX4939IDE_writeb(dma_stat, base, DMA_stat);
>>>+}

>>   BTW, you could save on using ide_dma_host_set() in LE mode if you'd 
>>set hwif->dma_base properly...

> Yes.  I like endian-free approach in general, but there is already
> some ifdefs in this driver.  I have no strong opinion here.

    Unfortunately, the way SFF-8038i registers were implemented in TX4939 
necessiates BE specific #ifdef'ery.  Or at least the run-time endianness 
detection and passing the right struct *_ops to the IDE core -- but that would 
burden the driver with unused and/or unneeded code for the opposite endiannes. 
  It could've been somewhat easied by the use of hwif->dma_{command|status}, 
etc. but those were recently removed (then again, if the DMA engine is not 
SFF-8038i compatible, those fields make little sense)...

>>>+static int __tx4939ide_dma_setup(ide_drive_t *drive)
>>>+{
>>>+	ide_hwif_t *hwif = drive->hwif;
>>>+	struct request *rq = HWGROUP(drive)->rq;
>>>+	unsigned int reading;
>>>+	u8 dma_stat;
>>>+	unsigned long base = TX4939IDE_BASE(hwif);
>>>+

>>[...]

>>>+
>>>+	/* read DMA status for INTR & ERROR flags */
>>>+	dma_stat = TX4939IDE_readb(base, DMA_stat);
>>>+
>>>+	/* clear INTR & ERROR flags */
>>>+	TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
>>>+	/* recover intmask cleared by writing to bit2 of DMA_stat */
>>>+	TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
>>>  
>>
>>   I think it might be worth factoring the BMDMA status clearing code 
>>into a separate function...

    ...along with the int_ctl write, of course.

> OK.  Good idea.

>>>+#ifdef __BIG_ENDIAN
>>>+/* custom iops (independent from SWAP_IO_SPACE) */
>>>+static u8 mm_inb(unsigned long port)
>>>+{
>>>+	return (u8)readb((void __iomem *)port);
>>>+}
>>>+static void mm_outb(u8 value, unsigned long port)
>>>+{
>>>+	writeb(value, (void __iomem *)port);
>>>+}

>>   I'm not sure readb()/writeb() are good substitute for 
>>__raw_readb()/__raw_writeb() as __swizzle_addr_b() might be actually 
>>changing the address...

> __swizzle_addr_b() used for both readb() and __raw_readb().  ioswabb()

    Ah, I missed that. :-/
    More's the reason to rely on the default methods where possible.

> ---
> Atsushi Nemoto

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

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/port_ops routines.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>   
[...]
> diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
> new file mode 100644
> index 0000000..ba9776d
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,762 @@
>   
[...]
> +static void tx4939ide_hwif_init(ide_hwif_t *hwif)
> +{
>   
[...]
> +
> +#ifdef __BIG_ENDIAN
> +	/* This setting does not affect PRD fetch */
> +	/* ByteSwap=1, Endian=00 */
> +	TX4939IDE_writew(0xc911, base, Add_Ctl);
> +#else
> +	TX4939IDE_writew(0xc901, base, Add_Ctl);
> +#endif
>   

   Aren't these the default register values? Is there a sense in writing 
them?

> +#ifdef __BIG_ENDIAN
> +/* custom iops (independent from SWAP_IO_SPACE) */
> +static u8 mm_inb(unsigned long port)
> +{
> +	return (u8)readb((void __iomem *)port);
> +}
> +static void mm_outb(u8 value, unsigned long port)
> +{
> +	writeb(value, (void __iomem *)port);
> +}
> +static void mm_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;
> +
> +		__raw_writew(data, (void __iomem *)io_ports->data_addr);
>   

   This doesn't look consistent (aside from the TX4939IDE_REG8/16 issue) 
-- mm_outsw_swap() calls cpu_to_le16() before writing 16-bit data but 
this code doesn't. So, either one of those should be wrong...

> +static void mm_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;
> +
> +		data = __raw_readw((void __iomem *)io_ports->data_addr);
> +		tf->data = data & 0xff;
> +		tf->hob_data = (data >> 8) & 0xff;
>   

    Same here...

> +static void mm_insw_swap(unsigned long port, void *addr, u32 count)
> +{
> +	unsigned short *ptr = addr;
> +	unsigned long size = count * 2;
> +	port &= ~1;
> +	while (count--)
> +		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
> +	__ide_flush_dcache_range((unsigned long)addr, size);
> +}
> +static void mm_outsw_swap(unsigned long port, void *addr, u32 count)
> +{
> +	unsigned short *ptr = addr;
> +	unsigned long size = count * 2;
> +	port &= ~1;
> +	while (count--) {
> +		__raw_writew(cpu_to_le16(*ptr), (void __iomem *)port);
> +		ptr++;
> +	}
> +	__ide_flush_dcache_range((unsigned long)addr, size);
> +}
>   

    Hum... but is it really correct to convert from/to LE order above? 
I'm prett sure that data is expected in LE order -- look ar 
ide_fix_driveid() for example...

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Sun, 14 Sep 2008 01:48:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > +#ifdef __BIG_ENDIAN
> > +	/* This setting does not affect PRD fetch */
> > +	/* ByteSwap=1, Endian=00 */
> > +	TX4939IDE_writew(0xc911, base, Add_Ctl);
> > +#else
> > +	TX4939IDE_writew(0xc901, base, Add_Ctl);
> > +#endif
> >   
> 
>    Aren't these the default register values? Is there a sense in writing 
> them?

Indeed.  It seems redundant.

> > +#ifdef __BIG_ENDIAN
> > +/* custom iops (independent from SWAP_IO_SPACE) */
> > +static u8 mm_inb(unsigned long port)
> > +{
> > +	return (u8)readb((void __iomem *)port);
> > +}
> > +static void mm_outb(u8 value, unsigned long port)
> > +{
> > +	writeb(value, (void __iomem *)port);
> > +}
> > +static void mm_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;
> > +
> > +		__raw_writew(data, (void __iomem *)io_ports->data_addr);
> >   
> 
>    This doesn't look consistent (aside from the TX4939IDE_REG8/16 issue) 
> -- mm_outsw_swap() calls cpu_to_le16() before writing 16-bit data but 
> this code doesn't. So, either one of those should be wrong...

Thanks, this code should be wrong.  IDE_TFLAG_OUT_DATA is totally
untested...

> > +static void mm_insw_swap(unsigned long port, void *addr, u32 count)
> > +{
> > +	unsigned short *ptr = addr;
> > +	unsigned long size = count * 2;
> > +	port &= ~1;
> > +	while (count--)
> > +		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
> > +	__ide_flush_dcache_range((unsigned long)addr, size);
> > +}
> > +static void mm_outsw_swap(unsigned long port, void *addr, u32 count)
> > +{
> > +	unsigned short *ptr = addr;
> > +	unsigned long size = count * 2;
> > +	port &= ~1;
> > +	while (count--) {
> > +		__raw_writew(cpu_to_le16(*ptr), (void __iomem *)port);
> > +		ptr++;
> > +	}
> > +	__ide_flush_dcache_range((unsigned long)addr, size);
> > +}
> >   
> 
>     Hum... but is it really correct to convert from/to LE order above? 
> I'm prett sure that data is expected in LE order -- look ar 
> ide_fix_driveid() for example...

Well, do you mean I should use cpu_to_le16 in mm_insw_swap and
le16_to_cpu in mm_outsw_swap?  Or can I avoid these swapping entirely
in some way?

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Atsushi Nemoto wrote:

>>> +#ifdef __BIG_ENDIAN
>>> +/* custom iops (independent from SWAP_IO_SPACE) */
>>> +static u8 mm_inb(unsigned long port)
>>> +{
>>> +	return (u8)readb((void __iomem *)port);
>>> +}
>>> +static void mm_outb(u8 value, unsigned long port)
>>> +{
>>> +	writeb(value, (void __iomem *)port);
>>> +}
>>> +static void mm_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;
>>> +
>>> +		__raw_writew(data, (void __iomem *)io_ports->data_addr);
>>>   
>>>       
>>    This doesn't look consistent (aside from the TX4939IDE_REG8/16 issue) 
>> -- mm_outsw_swap() calls cpu_to_le16() before writing 16-bit data but 
>> this code doesn't. So, either one of those should be wrong...
>>     
>
> Thanks, this code should be wrong.  IDE_TFLAG_OUT_DATA is totally
> untested...
>   

   Hum, not necessarily...
   If the data register is BE, this should work correctly, if I don't 
mistake (once you fix the data register's address).

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Tue, 16 Sep 2008 14:29:27 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> >>    This doesn't look consistent (aside from the TX4939IDE_REG8/16 issue) 
> >> -- mm_outsw_swap() calls cpu_to_le16() before writing 16-bit data but 
> >> this code doesn't. So, either one of those should be wrong...
> >
> > Thanks, this code should be wrong.  IDE_TFLAG_OUT_DATA is totally
> > untested...
> 
>    Hum, not necessarily...
>    If the data register is BE, this should work correctly, if I don't 
> mistake (once you fix the data register's address).

Hmm... or ide_tf_load()/ide_tf_read() is broken for big endian MIPS ?
(and possibly SPARC etc.)

__ide_mm_writesw(port, &data, 1) should be used instead of writew()
for IDE_TFLAG_OUT_DATA?

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Atsushi Nemoto wrote:

>>>>   This doesn't look consistent (aside from the TX4939IDE_REG8/16 issue) 
>>>>-- mm_outsw_swap() calls cpu_to_le16() before writing 16-bit data but 
>>>>this code doesn't. So, either one of those should be wrong...

>>>Thanks, this code should be wrong.  IDE_TFLAG_OUT_DATA is totally
>>>untested...

>>   Hum, not necessarily...
>>   If the data register is BE, this should work correctly, if I don't 
>>mistake (once you fix the data register's address).

> Hmm... or ide_tf_load()/ide_tf_read() is broken for big endian MIPS ?
> (and possibly SPARC etc.)

    Probably it is. But hardly anybody cares -- as I said, that flag seems 
totally useless.

> __ide_mm_writesw(port, &data, 1) should be used instead of writew()
> for IDE_TFLAG_OUT_DATA?

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello.

Atsushi Nemoto wrote:

>>>>   This doesn't look consistent (aside from the TX4939IDE_REG8/16 issue) 
>>>>-- mm_outsw_swap() calls cpu_to_le16() before writing 16-bit data but 
>>>>this code doesn't. So, either one of those should be wrong...
>>>
>>>Thanks, this code should be wrong.  IDE_TFLAG_OUT_DATA is totally
>>>untested...
>>
>>   Hum, not necessarily...
>>   If the data register is BE, this should work correctly, if I don't 
>>mistake (once you fix the data register's address).

> Hmm... or ide_tf_load()/ide_tf_read() is broken for big endian MIPS ?
> (and possibly SPARC etc.)

> __ide_mm_writesw(port, &data, 1) should be used instead of writew()
> for IDE_TFLAG_OUT_DATA?

    Probably the code there relies on the writew() doing the necessary byte 
swapping -- the same as ata_{in|out}_data() must be reying on ins[wl]() and 
outs[wl]() to do that, as well as on __ide_mm_reads[wl]() and 
__ide_mm_writes[wl]() -- which boil down to reads[wl]() and writes[wl]() on MIPS.
    What's not clear to me is why in MIPS read[wlq]() vs reads[wlq](), 
write[wlq]() vs writes[wl], in[wlq]() vs ins[wlq](), and out[wlq]() vs 
outs[wlq]() are using the different byte swapping: the single form uses 
ioswab[wlq]() while the sting form uses __mem_ioswab[wlq]() -- and those are 
defined as one swapping bytes and the other not in 
incluse/asm-mips/mach-generic/mangle-port.h. Cananybody shed some light on this?

> ---
> Atsushi Nemoto

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>>>   This doesn't look consistent (aside from the TX4939IDE_REG8/16 
>>>>> issue) -- mm_outsw_swap() calls cpu_to_le16() before writing 
>>>>> 16-bit data but this code doesn't. So, either one of those should 
>>>>> be wrong...
>>>>
>>>> Thanks, this code should be wrong.  IDE_TFLAG_OUT_DATA is totally
>>>> untested...
>>>
>>>   Hum, not necessarily...
>>>   If the data register is BE, this should work correctly, if I don't 
>>> mistake (once you fix the data register's address).
>
>> Hmm... or ide_tf_load()/ide_tf_read() is broken for big endian MIPS ?
>> (and possibly SPARC etc.)
>
>> __ide_mm_writesw(port, &data, 1) should be used instead of writew()
>> for IDE_TFLAG_OUT_DATA?
>
>    Probably the code there relies on the writew() doing the necessary 
> byte swapping -- the same as ata_{in|out}_data() must be reying on 
> ins[wl]() and outs[wl]() to do that, as well as on 
> __ide_mm_reads[wl]() and __ide_mm_writes[wl]() -- which boil down to 
> reads[wl]() and writes[wl]() on MIPS.
>    What's not clear to me is why in MIPS read[wlq]() vs reads[wlq](), 
> write[wlq]() vs writes[wl], in[wlq]() vs ins[wlq](), and out[wlq]() vs 
> outs[wlq]() are using the different byte swapping: the single form 
> uses ioswab[wlq]() while the sting form uses __mem_ioswab[wlq]() -- 
> and those are defined as one swapping bytes and the other not in 
> incluse/asm-mips/mach-generic/mangle-port.h. Cananybody shed some 
> light on this?

   Oh, I think I understand: the "single" versions take/return the value 
in host endian but the "string" version must treat the data as a stream 
of bytes.

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

Hello, I wrote:

>> +static void mm_insw_swap(unsigned long port, void *addr, u32 count)
>> +{
>> +    unsigned short *ptr = addr;
>> +    unsigned long size = count * 2;
>> +    port &= ~1;
>> +    while (count--)
>> +        *ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
>> +    __ide_flush_dcache_range((unsigned long)addr, size);
>> +}
>> +static void mm_outsw_swap(unsigned long port, void *addr, u32 count)
>> +{
>> +    unsigned short *ptr = addr;
>> +    unsigned long size = count * 2;
>> +    port &= ~1;
>> +    while (count--) {
>> +        __raw_writew(cpu_to_le16(*ptr), (void __iomem *)port);
>> +        ptr++;
>> +    }
>> +    __ide_flush_dcache_range((unsigned long)addr, size);
>> +}
>>   
>
>    Hum... but is it really correct to convert from/to LE order above? 
> I'm prett sure that data is expected in LE order -- look ar 
> ide_fix_driveid() for example...
>

   Assuming that the IDE data words' MSB appears at offset 6 and LSB at 
offset 7 (which would seem logical), the data is actually in BE (CPU) 
orger, so
mm_insw_swap() should use cpu_to_le16() and mm_outsw_swap() le16_to_cpu()...

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Mon, 15 Sep 2008 00:55:11 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> >    Hum... but is it really correct to convert from/to LE order above? 
> > I'm prett sure that data is expected in LE order -- look ar 
> > ide_fix_driveid() for example...
> >
> 
>    Assuming that the IDE data words' MSB appears at offset 6 and LSB at 
> offset 7 (which would seem logical), the data is actually in BE (CPU) 
> orger, so
> mm_insw_swap() should use cpu_to_le16() and mm_outsw_swap() le16_to_cpu()...

Thanks, I see.

---
Atsushi Nemoto

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov]

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/port_ops routines.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>   
[...]
> diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
> new file mode 100644
> index 0000000..ba9776d
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,762 @@
>   
[...]
> +static int __tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	struct request *rq = HWGROUP(drive)->rq;
> +	unsigned int reading;
> +	u8 dma_stat;
> +	unsigned long base = TX4939IDE_BASE(hwif);
> +
> +	if (rq_data_dir(rq))
> +		reading = 0;
> +	else
> +		reading = 1 << 3;
> +
> +	/* fall back to pio! */
> +	if (!ide_build_dmatable(drive, rq)) {
> +		ide_map_sg(drive, rq);
> +		return 1;
> +	}
> +#ifdef __BIG_ENDIAN
> +	{
> +		unsigned int *table = hwif->dmatable_cpu;
>   

   s/unsigned int/__le32/ perhaps?

> +		while (1) {
> +			cpu_to_le64s((u64 *)table);
>   

   Wait, PRD is already already in LE format, so this should be 
le64_to_cpus().

> +			if (*table & 0x80000000)
>   

   Hum... you don't have to check that with ide_build_dmatable() 
returning the PRD count...

> +				break;
> +			table += 2;
> +		}
> +	}
> +#endif
>   

MBR, Sergei

Re: [PATCH 1/2] ide: Add tx4939ide driver

[Atsushi Nemoto]

On Wed, 17 Sep 2008 01:59:24 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> > +		unsigned int *table = hwif->dmatable_cpu;
> >   
> 
>    s/unsigned int/__le32/ perhaps?

Yes.  And __le64 would be much better.

> > +		while (1) {
> > +			cpu_to_le64s((u64 *)table);
> >   
> 
>    Wait, PRD is already already in LE format, so this should be 
> le64_to_cpus().

OK.

> > +			if (*table & 0x80000000)
> >   
> 
>    Hum... you don't have to check that with ide_build_dmatable() 
> returning the PRD count...

Indeed.  I'll do it.  Thanks.