[PATCH 2/3] ide: add at91_ide driver

[PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka]

This is IDE host driver for AT91SAM9 Static Memory Controller with Compact
Flash True IDE Mode logic.

Driver have to switch 8/16 bit bus width when accessing Task Tile or Data
Register. Moreover some extra things need to be done when setting PIO mode.
Only PIO mode is used, hardware have no DMA support. If interrupt line is
connected through GPIO extra quirk is needed to cope with fake interrupts.

Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
---
 arch/arm/mach-at91/include/mach/board.h |   12 +
 drivers/ide/Kconfig                     |    5 +
 drivers/ide/Makefile                    |    1 +
 drivers/ide/at91_ide.c                  |  496 +++++++++++++++++++++++++++++++
 4 files changed, 514 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ide/at91_ide.c

diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index fb51f0e..6674b9b 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h
@@ -59,6 +59,18 @@ struct at91_cf_data {
 };
 extern void __init at91_add_device_cf(struct at91_cf_data *data);
 
+ /* Compact Flash True IDE mode */
+struct at91_ide_data {
+	u8	irq_pin;		/* the same meaning as for CF */
+	u8	det_pin;
+	u8	rst_pin;
+	u8	chipselect;
+	u8	flags;
+#define AT91_IDE_SWAP_A0_A2 0x01
+};
+
+extern void __init at91_add_device_ide(struct at91_ide_data *data);
+
  /* MMC / SD */
 struct at91_mmc_data {
 	u8		det_pin;	/* card detect IRQ */
diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index 3dad229..b11da5b 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -721,6 +721,11 @@ config BLK_DEV_IDE_TX4939
 	depends on SOC_TX4939
 	select BLK_DEV_IDEDMA_SFF
 
+config BLK_DEV_IDE_AT91
+	tristate "Atmel AT91 IDE support"
+	depends on ARM && ARCH_AT91
+	select IDE_TIMINGS
+
 config IDE_ARM
 	tristate "ARM IDE support"
 	depends on ARM && (ARCH_RPC || ARCH_SHARK)
diff --git a/drivers/ide/Makefile b/drivers/ide/Makefile
index d0e3d7d..1c326d9 100644
--- a/drivers/ide/Makefile
+++ b/drivers/ide/Makefile
@@ -116,3 +116,4 @@ obj-$(CONFIG_BLK_DEV_IDE_AU1XXX)	+= au1xxx-ide.o
 
 obj-$(CONFIG_BLK_DEV_IDE_TX4938)	+= tx4938ide.o
 obj-$(CONFIG_BLK_DEV_IDE_TX4939)	+= tx4939ide.o
+obj-$(CONFIG_BLK_DEV_IDE_AT91)		+= at91_ide.o
diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
new file mode 100644
index 0000000..3a1f7e0
--- /dev/null
+++ b/drivers/ide/at91_ide.c
@@ -0,0 +1,496 @@
+/*
+ * IDE host driver for AT91SAM9 Static Memory Controller
+ * with Compact Flash True IDE logic
+ *
+ * Copyright (c) 2008, 2009 Kelvatek Ltd.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/ide.h>
+#include <linux/platform_device.h>
+
+#include <mach/board.h>
+#include <mach/gpio.h>
+#include <mach/at91sam9263.h>
+#include <mach/at91sam9_smc.h>
+#include <mach/at91sam9263_matrix.h>
+
+#define DRV_NAME "at91_ide"
+
+#define perr(fmt, args...) printk(KERN_ERR DRV_NAME ": " fmt, ##args)
+
+#ifdef DEBUG
+#define pdbg(fmt, args...) printk(KERN_DEBUG "%s " fmt, __func__, ##args)
+#else
+#define pdbg(fmt, args...)
+#endif
+
+/*
+ * AT91 Static Memory Controller maps Task File and Data Register
+ * at the same address. To distinguish access between these two
+ * different bus data width is used: 8Bit for Task File, 16Bit for Data I/O
+ *
+ * After initialization we do 8/16 bit flipping (changes in SMC MODE register)
+ * only inside IDE callback routines which are serialized by IDE layer,
+ * so no additional locking needed.
+ */
+
+static void init_smc_mode(const u8 chipselect)
+{
+	at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
+						  AT91_SMC_WRITEMODE |
+						  AT91_SMC_BAT_SELECT |
+						  AT91_SMC_TDF_(0));
+}
+
+static inline void set_8bit_mode(const u8 chipselect)
+{
+	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
+	mode &= ~AT91_SMC_DBW;
+	mode |= AT91_SMC_DBW_8;
+	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	pdbg("%u %08lx\n", chipselect, mode);
+}
+
+static inline void set_16bit_mode(const u8 chipselect)
+{
+	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
+	mode &= ~AT91_SMC_DBW;
+	mode |= AT91_SMC_DBW_16;
+	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	pdbg("%u %08lx\n", chipselect, mode);
+}
+
+static void set_smc_timings(const u8 chipselect, const u16 cycle,
+			    const u16 setup, const u16 pulse,
+			    const u16 data_float, int use_iordy)
+{
+	unsigned long mode;
+
+	/* setup timings in SMC */
+	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
+						   AT91_SMC_NCS_WRSETUP_(0) |
+						   AT91_SMC_NRDSETUP_(setup) |
+						   AT91_SMC_NCS_RDSETUP_(0));
+	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
+						   AT91_SMC_NCS_WRPULSE_(cycle) |
+						   AT91_SMC_NRDPULSE_(pulse) |
+						   AT91_SMC_NCS_RDPULSE_(cycle));
+	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
+						   AT91_SMC_NRDCYCLE_(cycle));
+
+	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
+
+	/* disable or enable waiting for IORDY signal */
+	mode &= ~AT91_SMC_EXNWMODE;
+	if (use_iordy)
+		mode |= AT91_SMC_EXNWMODE_READY;
+
+	/* add data float cycles if needed */
+	mode &= ~AT91_SMC_TDF;
+	if (data_float)
+		mode |= AT91_SMC_TDF_(data_float);
+
+	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+}
+
+static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
+{
+	u64 tmp = ns;
+
+	tmp *= mck_hz;
+	tmp += 1000*1000*1000 - 1; /* round up */
+	do_div(tmp, 1000*1000*1000);
+	return (unsigned int) tmp;
+}
+
+static void apply_timings(const u8 chipselect, const u8 pio,
+			  const struct ide_timing *timing, const u16 *ata_id)
+{
+	unsigned int t0, t1, t2, t6z, th;
+	unsigned int cycle, setup, pulse, data_float;
+	unsigned int mck_hz;
+	struct clk *mck;
+	int use_iordy;
+
+	/* see table 22 of Compact Flash standard 4.1 for the meaning,
+	 * we do not stretch active (t2) time, so setup (t1) + hold time (th)
+	 * assure at least minimal recovery (t2i) time */
+	t0 = timing->cyc8b;
+	t1 = timing->setup;
+	t2 = timing->act8b;
+
+	/* ensure minimal "~IORD data hold time" (t6z) */
+	t6z = (pio < 5) ? 30 : 20;
+	th = t0 - t1 - t2;
+	if (t6z > th)
+		t6z -= th;
+	else
+		t6z = 0; /* already covered in hold time */
+
+	pdbg("t0=%u t1=%u t2=%u th=%u t6z=%u\n", t0, t1, t2, th, t6z);
+
+	mck = clk_get(NULL, "mck");
+	BUG_ON(IS_ERR(mck));
+	mck_hz = clk_get_rate(mck);
+	pdbg("mck_hz=%u\n", mck_hz);
+
+	cycle = calc_mck_cycles(t0, mck_hz);
+	setup = calc_mck_cycles(t1, mck_hz);
+	pulse = calc_mck_cycles(t2, mck_hz);
+	data_float = calc_mck_cycles(t6z, mck_hz);
+
+	pdbg("cycle=%u setup=%u pulse=%u data_float=%u\n",
+	     cycle, setup, pulse, data_float);
+
+	use_iordy = 0;
+	if (ata_id) {
+		if (ata_id_is_cfa(ata_id)) {
+			if (pio == 3 || pio == 4)
+				use_iordy = 1;
+		} else if (pio >= 3 || ata_id_has_iordy(ata_id))
+			use_iordy = 1;
+	}
+
+	set_smc_timings(chipselect, cycle, setup, pulse, data_float, use_iordy);
+}
+
+static u8 ide_mm_inb(unsigned long port)
+{
+	return (u8) readb((void __iomem *) port);
+}
+
+static void ide_mm_outb(u8 value, unsigned long port)
+{
+	writeb(value, (void __iomem *) port);
+}
+
+void at91_ide_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;
+		u8 chipselect = hwif->extra_base;
+
+		set_16bit_mode(chipselect);
+		writew(data, (void __iomem *) io_ports->data_addr);
+		set_8bit_mode(chipselect);
+	}
+
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_FEATURE)
+		ide_mm_outb(tf->hob_feature, io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_NSECT)
+		ide_mm_outb(tf->hob_nsect, io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAL)
+		ide_mm_outb(tf->hob_lbal, io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAM)
+		ide_mm_outb(tf->hob_lbam, io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_HOB_LBAH)
+		ide_mm_outb(tf->hob_lbah, io_ports->lbah_addr);
+
+	if (task->tf_flags & IDE_TFLAG_OUT_FEATURE)
+		ide_mm_outb(tf->feature, io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_NSECT)
+		ide_mm_outb(tf->nsect, io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAL)
+		ide_mm_outb(tf->lbal, io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAM)
+		ide_mm_outb(tf->lbam, io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_OUT_LBAH)
+		ide_mm_outb(tf->lbah, io_ports->lbah_addr);
+
+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE)
+		ide_mm_outb((tf->device & HIHI) | drive->select, io_ports->device_addr);
+}
+
+void at91_ide_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) {
+		u8 chipselect = hwif->extra_base;
+		u16 data;
+
+		set_16bit_mode(chipselect);
+		data = readw((void __iomem *) io_ports->data_addr);
+		set_8bit_mode(chipselect);
+
+		tf->data = data & 0xff;
+		tf->hob_data = (data >> 8) & 0xff;
+	}
+
+	/* be sure we're looking at the low order bits */
+	ide_mm_outb(ATA_DEVCTL_OBS & ~0x80, io_ports->ctl_addr);
+
+	if (task->tf_flags & IDE_TFLAG_IN_FEATURE)
+		tf->feature = ide_mm_inb(io_ports->feature_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_NSECT)
+		tf->nsect  = ide_mm_inb(io_ports->nsect_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAL)
+		tf->lbal   = ide_mm_inb(io_ports->lbal_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAM)
+		tf->lbam   = ide_mm_inb(io_ports->lbam_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_LBAH)
+		tf->lbah   = ide_mm_inb(io_ports->lbah_addr);
+	if (task->tf_flags & IDE_TFLAG_IN_DEVICE)
+		tf->device = ide_mm_inb(io_ports->device_addr);
+
+	if (task->tf_flags & IDE_TFLAG_LBA48) {
+		ide_mm_outb(ATA_DEVCTL_OBS | 0x80, io_ports->ctl_addr);
+
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_FEATURE)
+			tf->hob_feature = ide_mm_inb(io_ports->feature_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_NSECT)
+			tf->hob_nsect   = ide_mm_inb(io_ports->nsect_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAL)
+			tf->hob_lbal    = ide_mm_inb(io_ports->lbal_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAM)
+			tf->hob_lbam    = ide_mm_inb(io_ports->lbam_addr);
+		if (task->tf_flags & IDE_TFLAG_IN_HOB_LBAH)
+			tf->hob_lbah    = ide_mm_inb(io_ports->lbah_addr);
+	}
+}
+
+void at91_ide_input_data(ide_drive_t *drive, struct request *rq,
+			 void *buf, unsigned int len)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct ide_io_ports *io_ports = &hwif->io_ports;
+	u8 chipselect = hwif->extra_base;
+
+	pdbg("cs %u buf %p len %d\n", chipselect, buf, len);
+
+	len++;
+
+	set_16bit_mode(chipselect);
+	__ide_mm_insw((void __iomem *) io_ports->data_addr, buf, len / 2);
+	set_8bit_mode(chipselect);
+}
+
+void at91_ide_output_data(ide_drive_t *drive, struct request *rq,
+			  void *buf, unsigned int len)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct ide_io_ports *io_ports = &hwif->io_ports;
+	u8 chipselect = hwif->extra_base;
+
+	pdbg("cs %u buf %p len %d\n", chipselect,  buf, len);
+
+	set_16bit_mode(chipselect);
+	__ide_mm_outsw((void __iomem *) io_ports->data_addr, buf, len / 2);
+	set_8bit_mode(chipselect);
+}
+
+static void at91_ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	struct ide_timing *timing;
+	u8 chipselect = drive->hwif->extra_base;
+
+	pdbg("chipselect %u pio %u\n", chipselect, pio);
+
+	timing = ide_timing_find_mode(XFER_PIO_0 + pio);
+	BUG_ON(!timing);
+
+	apply_timings(chipselect, pio, timing, drive->id);
+}
+
+static const struct ide_tp_ops at91_ide_tp_ops = {
+	.exec_command	= ide_exec_command,
+	.read_status	= ide_read_status,
+	.read_altstatus	= ide_read_altstatus,
+	.set_irq	= ide_set_irq,
+
+	.tf_load	= at91_ide_tf_load,
+	.tf_read	= at91_ide_tf_read,
+
+	.input_data	= at91_ide_input_data,
+	.output_data	= at91_ide_output_data,
+};
+
+static const struct ide_port_ops at91_ide_port_ops = {
+	.set_pio_mode	= at91_ide_set_pio_mode,
+};
+
+static const struct ide_port_info at91_ide_port_info __initdata = {
+	.port_ops	= &at91_ide_port_ops,
+	.tp_ops		= &at91_ide_tp_ops,
+	.host_flags 	= IDE_HFLAG_MMIO | IDE_HFLAG_NO_DMA | IDE_HFLAG_SINGLE |
+			  IDE_HFLAG_NO_IO_32BIT | IDE_HFLAG_UNMASK_IRQS,
+	.pio_mask 	= ATA_PIO5,
+};
+
+/*
+ * If interrupt is delivered through GPIO, IRQ are triggered on falling
+ * and raising edge of signal. Whereas IDE device request interrupt on high
+ * level (raising edge in our case). This mean we have fake interrupts, so
+ * we need to check interrupt pin and exit instantly from ISR when line
+ * is on low level.
+ */
+
+irqreturn_t at91_irq_handler(int irq, void *dev_id)
+{
+	int ntries = 8;
+	int pin_val1, pin_val2;
+
+	/* additional deglitch, line can be noisy in badly designed PCB */
+	do {
+		pin_val1 = at91_get_gpio_value(irq);
+		pin_val2 = at91_get_gpio_value(irq);
+	} while (pin_val1 != pin_val2 && --ntries > 0);
+
+	if (pin_val1 == 0 || ntries <= 0)
+		return IRQ_HANDLED;
+
+	return ide_intr(irq, dev_id);
+}
+
+static int __init at91_ide_probe(struct platform_device *pdev)
+{
+	int ret;
+	hw_regs_t hw;
+	hw_regs_t *hws[] = { &hw, NULL, NULL, NULL };
+	struct resource *tf_res, *ctl_res;
+	unsigned long tf_base = 0, ctl_base = 0;
+	struct at91_ide_data *board = pdev->dev.platform_data;
+	struct ide_host *host;
+	struct ide_timing *pio0_timing;
+
+	tf_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!ctl_res || !tf_res) {
+		perr("can't get memory resources\n");
+		return -ENODEV;
+	}
+
+	if (!devm_request_mem_region(&pdev->dev, tf_res->start,
+				     tf_res->end - tf_res->start + 1, "ide") ||
+	    !devm_request_mem_region(&pdev->dev, ctl_res->start,
+				     ctl_res->end - ctl_res->start + 1, "alt")) {
+		perr("memory resources in use\n");
+		return -EBUSY;
+	}
+
+	pdbg("chipselect %u irq %u tf %08lx ctl %08lx\n", board->chipselect,
+	     board->irq_pin, tf_res->start, ctl_res->start);
+
+	tf_base = (unsigned long) devm_ioremap(&pdev->dev, tf_res->start,
+					       tf_res->end - tf_res->start + 1);
+	ctl_base = (unsigned long) devm_ioremap(&pdev->dev, ctl_res->start,
+						ctl_res->end - ctl_res->start + 1);
+	if (!tf_base || !ctl_base) {
+		perr("can't map memory regions\n");
+		return -EBUSY;
+	}
+
+	memset(&hw, 0, sizeof(hw));
+
+	if (board->flags & AT91_IDE_SWAP_A0_A2) {
+		/* workaround for stupid hardware bug */
+		hw.io_ports.data_addr	= tf_base + 0;
+		hw.io_ports.error_addr	= tf_base + 4;
+		hw.io_ports.nsect_addr	= tf_base + 2;
+		hw.io_ports.lbal_addr	= tf_base + 6;
+		hw.io_ports.lbam_addr	= tf_base + 1;
+		hw.io_ports.lbah_addr	= tf_base + 5;
+		hw.io_ports.device_addr = tf_base + 3;
+		hw.io_ports.command_addr = tf_base + 7;
+		hw.io_ports.ctl_addr	= ctl_base + 3;
+	} else
+		ide_std_init_ports(&hw, tf_base, ctl_base + 6);
+
+	hw.irq = board->irq_pin;
+	hw.chipset = ide_generic;
+	hw.dev = &pdev->dev;
+
+	host = ide_host_alloc(&at91_ide_port_info, hws);
+	if (!host) {
+		perr("failed to allocate ide host\n");
+		return -ENOMEM;
+	}
+
+	/* setup Static Memory Controller - PIO 0 as default */
+	init_smc_mode(board->chipselect);
+	pio0_timing = ide_timing_find_mode(XFER_PIO_0);
+	apply_timings(board->chipselect, 0, pio0_timing, 0);
+
+	/* with GPIO interrupt we have to do quirks in handler */
+	if (board->irq_pin >= PIN_BASE)
+		host->irq_handler = at91_irq_handler;
+
+	host->ports[0]->extra_base = board->chipselect;
+
+	ret = ide_host_register(host, &at91_ide_port_info, hws);
+	if (ret) {
+		perr("failed to register ide host\n");
+		goto err_free_host;
+	}
+	platform_set_drvdata(pdev, host);
+	return 0;
+
+err_free_host:
+	ide_host_free(host);
+	return ret;
+}
+
+static int __exit at91_ide_remove(struct platform_device *pdev)
+{
+	struct ide_host *host = platform_get_drvdata(pdev);
+
+	ide_host_remove(host);
+	return 0;
+}
+
+static struct platform_driver at91_ide_driver = {
+	.driver	= {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+	.probe	= at91_ide_probe,
+	.remove	= __exit_p(at91_ide_remove),
+};
+
+static int __init at91_ide_init(void)
+{
+	return platform_driver_register(&at91_ide_driver);
+}
+
+static void __exit at91_ide_exit(void)
+{
+	platform_driver_unregister(&at91_ide_driver);
+}
+
+module_init(at91_ide_init);
+module_exit(at91_ide_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Stanislaw Gruszka <stf_xl@wp.pl>");
+
-- 
1.5.2.5

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

> This is IDE host driver for AT91SAM9 Static Memory Controller with Compact
> Flash True IDE Mode logic.
>
> Driver have to switch 8/16 bit bus width when accessing Task Tile or Data
> Register. Moreover some extra things need to be done when setting PIO mode.
> Only PIO mode is used, hardware have no DMA support. If interrupt line is
> connected through GPIO extra quirk is needed to cope with fake interrupts.
>
> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
>   
[...]
> diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
> index fb51f0e..6674b9b 100644
> --- a/arch/arm/mach-at91/include/mach/board.h
> +++ b/arch/arm/mach-at91/include/mach/board.h
> @@ -59,6 +59,18 @@ struct at91_cf_data {
>  };
>  extern void __init at91_add_device_cf(struct at91_cf_data *data);
>  
> + /* Compact Flash True IDE mode */
> +struct at91_ide_data {
> +	u8	irq_pin;		/* the same meaning as for CF */
>   

  I again have to express my dislike about not passing IRQ the usual 
way. Also, see my comments to the platform code.

> +	u8	det_pin;
> +	u8	rst_pin;
> +	u8	chipselect;
> +	u8	flags;
> +#define AT91_IDE_SWAP_A0_A2 0x01
> +};
> +
> +extern void __init at91_add_device_ide(struct at91_ide_data *data);
> +
>   /* MMC / SD */
>  struct at91_mmc_data {
>  	u8		det_pin;	/* card detect IRQ */
> diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> index 3dad229..b11da5b 100644
> --- a/drivers/ide/Kconfig
> +++ b/drivers/ide/Kconfig
> @@ -721,6 +721,11 @@ config BLK_DEV_IDE_TX4939
>  	depends on SOC_TX4939
>  	select BLK_DEV_IDEDMA_SFF
>  
> +config BLK_DEV_IDE_AT91
> +	tristate "Atmel AT91 IDE support"
>   

   Please be more specific -- you can't drive AT91RM9200 SMC.

> +	depends on ARM && ARCH_AT91
>   

   Please add "&& !ARCH_AT91RM9200". And maybe "&& !ARCH_AT91X40" too...

> diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> new file mode 100644
> index 0000000..3a1f7e0
> --- /dev/null
> +++ b/drivers/ide/at91_ide.c
> @@ -0,0 +1,496 @@
> +/*
> + * IDE host driver for AT91SAM9 Static Memory Controller
>   

   Why not call the driver 'at91sam9_ide'?

> +/*
> + * AT91 Static Memory Controller

  AT91SAM9.

> maps Task File and Data Register
>   
> + * at the same address. To distinguish access between these two

   It would have been strange if it did it otherwise...

> + * different bus data width is used: 8Bit for Task File, 16Bit for Data I/O
> + *
> + * After initialization we do 8/16 bit flipping (changes in SMC MODE register)
> + * only inside IDE callback routines which are serialized by IDE layer,
> + * so no additional locking needed.
> + */
> +
> +static void init_smc_mode(const u8 chipselect)
> +{
> +	at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
> +						  AT91_SMC_WRITEMODE |
> +						  AT91_SMC_BAT_SELECT |
> +						  AT91_SMC_TDF_(0));
>   

   I'm not sure why are you fixing the dataflow timing again, this time 
to 0...

> +}
> +
> +static inline void set_8bit_mode(const u8 chipselect)
> +{
> +	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> +	mode &= ~AT91_SMC_DBW;
> +	mode |= AT91_SMC_DBW_8;
> +	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> +	pdbg("%u %08lx\n", chipselect, mode);
> +}
> +
> +static inline void set_16bit_mode(const u8 chipselect)
> +{
> +	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> +	mode &= ~AT91_SMC_DBW;
> +	mode |= AT91_SMC_DBW_16;
> +	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> +	pdbg("%u %08lx\n", chipselect, mode);
> +}
>   

  I'd advice to make this single function because it looks like a code 
duplication too much.

> +static void set_smc_timings(const u8 chipselect, const u16 cycle,
> +			    const u16 setup, const u16 pulse,
> +			    const u16 data_float, int use_iordy)
>   

   Have you considered using already present sam9_smc_configure() 
instead to avoid the code duplication (it needs to be exported though)?

> +{
> +	unsigned long mode;
> +
> +	/* setup timings in SMC */
> +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
> +						   AT91_SMC_NCS_WRSETUP_(0) |
> +						   AT91_SMC_NRDSETUP_(setup) |
> +						   AT91_SMC_NCS_RDSETUP_(0));
> +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
> +						   AT91_SMC_NCS_WRPULSE_(cycle) |
> +						   AT91_SMC_NRDPULSE_(pulse) |
> +						   AT91_SMC_NCS_RDPULSE_(cycle));
> +	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
> +						   AT91_SMC_NRDCYCLE_(cycle));
> +
> +	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
>   

   Frankly speaking, I don't see why you're reading this register back 
if you already know what needs to be set there -- as you've done it in 
init_smc_mode().

> +static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
> +{
> +	u64 tmp = ns;
> +
> +	tmp *= mck_hz;
> +	tmp += 1000*1000*1000 - 1; /* round up */
> +	do_div(tmp, 1000*1000*1000);
> +	return (unsigned int) tmp;
> +}
> +
> +static void apply_timings(const u8 chipselect, const u8 pio,
> +			  const struct ide_timing *timing, const u16 *ata_id)
> +{
> +	unsigned int t0, t1, t2, t6z, th;
> +	unsigned int cycle, setup, pulse, data_float;
> +	unsigned int mck_hz;
> +	struct clk *mck;
> +	int use_iordy;
>   
[...]
> +
> +	use_iordy = 0;
>   

   Can be done in initializer...

> +	if (ata_id) {
> +		if (ata_id_is_cfa(ata_id)) {
> +			if (pio == 3 || pio == 4)
> +				use_iordy = 1;
>   
> +		} else if (pio >= 3 || ata_id_has_iordy(ata_id))
> +			use_iordy = 1;
>   


   No, you should check for IORDY regardless of CFA and using IORDY 
shouldn't be limeited to modes 3 and 4:

        if (ata_id_has_iordy(ata_id) && !(ata_id_is_cfa(ata_id) && pio > 4))
           use_iodry = 1;

> +static u8 ide_mm_inb(unsigned long port)
> +{
> +	return (u8) readb((void __iomem *) port);
>   

  Explict cast not needed here.

> +void at91_ide_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) {
>   

   Sigh. Bart, couldn't we drop that stupid flag? I bet nobody ever used it.

> +void at91_ide_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) {
>   

   ... and this one too?

> +/*
> + * If interrupt is delivered through GPIO, IRQ are triggered on falling
> + * and raising edge of signal. Whereas IDE device request interrupt on high
> + * level (raising edge in our case). This mean we have fake interrupts, so
> + * we need to check interrupt pin and exit instantly from ISR when line
> + * is on low level.
> + */
> +
> +irqreturn_t at91_irq_handler(int irq, void *dev_id)
> +{
> +	int ntries = 8;
> +	int pin_val1, pin_val2;
> +
> +	/* additional deglitch, line can be noisy in badly designed PCB */
> +	do {
> +		pin_val1 = at91_get_gpio_value(irq);
> +		pin_val2 = at91_get_gpio_value(irq);
> +	} while (pin_val1 != pin_val2 && --ntries > 0);
>   

  I suggest a shorter code:

	do {
		pin_val = at91_get_gpio_value(irq);
	} while (pin_val != at91_get_gpio_value(irq) && --ntries > 0);


> +static int __init at91_ide_probe(struct platform_device *pdev)
> +{
>   
[...]
> +	host->ports[0]->extra_base = board->chipselect;
>   

   BTW, we have 2 fields in the struct hwif_s that fit this case better: 
config_data and select_data. It's a bit stange that you've selected 
extra_base...

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka]

Wednesday 04 February 2009 13:19:50 Sergei Shtylyov napisał(a):
> > diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
> > index fb51f0e..6674b9b 100644
> > --- a/arch/arm/mach-at91/include/mach/board.h
> > +++ b/arch/arm/mach-at91/include/mach/board.h
> > @@ -59,6 +59,18 @@ struct at91_cf_data {
> >  };
> >  extern void __init at91_add_device_cf(struct at91_cf_data *data);
> >  
> > + /* Compact Flash True IDE mode */
> > +struct at91_ide_data {
> > +	u8	irq_pin;		/* the same meaning as for CF */
> >   
> 
>   I again have to express my dislike about not passing IRQ the usual 
> way. Also, see my comments to the platform code.

Yes, I know, I don't like to argue. Only reasoning to use platform irq resource
seams to be: "because other drivers do". However we have exception - at91_cf
also use board->irq_pin, so maybe this driver could also do ?

> > +	u8	det_pin;
> > +	u8	rst_pin;
> > +	u8	chipselect;
> > +	u8	flags;
> > +#define AT91_IDE_SWAP_A0_A2 0x01
> > +};
> > +
> > +extern void __init at91_add_device_ide(struct at91_ide_data *data);
> > +
> >   /* MMC / SD */
> >  struct at91_mmc_data {
> >  	u8		det_pin;	/* card detect IRQ */
> > diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> > index 3dad229..b11da5b 100644
> > --- a/drivers/ide/Kconfig
> > +++ b/drivers/ide/Kconfig
> > @@ -721,6 +721,11 @@ config BLK_DEV_IDE_TX4939
> >  	depends on SOC_TX4939
> >  	select BLK_DEV_IDEDMA_SFF
> >  
> > +config BLK_DEV_IDE_AT91
> > +	tristate "Atmel AT91 IDE support"
> >   
> 
>    Please be more specific -- you can't drive AT91RM9200 SMC.

Ok.

> 
> > +	depends on ARM && ARCH_AT91
> >   
> 
>    Please add "&& !ARCH_AT91RM9200". And maybe "&& !ARCH_AT91X40" too...

Ok.
 
> > diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> > new file mode 100644
> > index 0000000..3a1f7e0
> > --- /dev/null
> > +++ b/drivers/ide/at91_ide.c
> > @@ -0,0 +1,496 @@
> > +/*
> > + * IDE host driver for AT91SAM9 Static Memory Controller
> >   
> 
>    Why not call the driver 'at91sam9_ide'?
> 
> > +/*
> > + * AT91 Static Memory Controller
> 
>   AT91SAM9.

Ok, currently only SAM9 can be used with driver. However I think adding
support to AT91RM9200 to this driver will be not much effort. I don't think
someone will want to write new driver for RM9200 insted using this one.

IMHO, current name is short and sweet and not miss the true so much -
most of the AT91s which can work with True IDE logic are supported. 
I would like to stay with current name.

> > maps Task File and Data Register
> >   
> > + * at the same address. To distinguish access between these two
> 
>    It would have been strange if it did it otherwise...
> 
> > + * different bus data width is used: 8Bit for Task File, 16Bit for Data I/O
> > + *
> > + * After initialization we do 8/16 bit flipping (changes in SMC MODE register)
> > + * only inside IDE callback routines which are serialized by IDE layer,
> > + * so no additional locking needed.
> > + */
> > +
> > +static void init_smc_mode(const u8 chipselect)
> > +{
> > +	at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
> > +						  AT91_SMC_WRITEMODE |
> > +						  AT91_SMC_BAT_SELECT |
> > +						  AT91_SMC_TDF_(0));
> >   
> 
>    I'm not sure why are you fixing the dataflow timing again, this time 
> to 0...

Not necessary code, will be removed.

> > +}
> > +
> > +static inline void set_8bit_mode(const u8 chipselect)
> > +{
> > +	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> > +	mode &= ~AT91_SMC_DBW;
> > +	mode |= AT91_SMC_DBW_8;
> > +	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> > +	pdbg("%u %08lx\n", chipselect, mode);
> > +}
> > +
> > +static inline void set_16bit_mode(const u8 chipselect)
> > +{
> > +	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> > +	mode &= ~AT91_SMC_DBW;
> > +	mode |= AT91_SMC_DBW_16;
> > +	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> > +	pdbg("%u %08lx\n", chipselect, mode);
> > +}
> >   
> 
>   I'd advice to make this single function because it looks like a code 
> duplication too much.

Uh well.

> > +static void set_smc_timings(const u8 chipselect, const u16 cycle,
> > +			    const u16 setup, const u16 pulse,
> > +			    const u16 data_float, int use_iordy)
> >   
> 
>    Have you considered using already present sam9_smc_configure() 
> instead to avoid the code duplication (it needs to be exported though)?

I think, it need to be exported, I'll check.

> > +{
> > +	unsigned long mode;
> > +
> > +	/* setup timings in SMC */
> > +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
> > +						   AT91_SMC_NCS_WRSETUP_(0) |
> > +						   AT91_SMC_NRDSETUP_(setup) |
> > +						   AT91_SMC_NCS_RDSETUP_(0));
> > +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
> > +						   AT91_SMC_NCS_WRPULSE_(cycle) |
> > +						   AT91_SMC_NRDPULSE_(pulse) |
> > +						   AT91_SMC_NCS_RDPULSE_(cycle));
> > +	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
> > +						   AT91_SMC_NRDCYCLE_(cycle));
> > +
> > +	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> >   
> 
>    Frankly speaking, I don't see why you're reading this register back 
> if you already know what needs to be set there -- as you've done it in 
> init_smc_mode().

This function is not only used at initialization. It is also used when PIO mode
is changed.

> > +static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
> > +{
> > +	u64 tmp = ns;
> > +
> > +	tmp *= mck_hz;
> > +	tmp += 1000*1000*1000 - 1; /* round up */
> > +	do_div(tmp, 1000*1000*1000);
> > +	return (unsigned int) tmp;
> > +}
> > +
> > +static void apply_timings(const u8 chipselect, const u8 pio,
> > +			  const struct ide_timing *timing, const u16 *ata_id)
> > +{
> > +	unsigned int t0, t1, t2, t6z, th;
> > +	unsigned int cycle, setup, pulse, data_float;
> > +	unsigned int mck_hz;
> > +	struct clk *mck;
> > +	int use_iordy;
> >   
> [...]
> > +
> > +	use_iordy = 0;
> >   
> 
>    Can be done in initializer...

But here is more readable, I think.

> > +	if (ata_id) {
> > +		if (ata_id_is_cfa(ata_id)) {
> > +			if (pio == 3 || pio == 4)
> > +				use_iordy = 1;
> >   
> > +		} else if (pio >= 3 || ata_id_has_iordy(ata_id))
> > +			use_iordy = 1;
> >   
> 
> 
>    No, you should check for IORDY regardless of CFA and using IORDY 
> shouldn't be limeited to modes 3 and 4:

Hmm, I take as example ata_pio_need_iordy(), it is also wrong or maybe
I don't follow the logic properly.

 
>         if (ata_id_has_iordy(ata_id) && !(ata_id_is_cfa(ata_id) && pio > 4))
>            use_iodry = 1;
> 
> > +static u8 ide_mm_inb(unsigned long port)
> > +{
> > +	return (u8) readb((void __iomem *) port);
> >   
> 
>   Explict cast not needed here.

Ok.

> > +irqreturn_t at91_irq_handler(int irq, void *dev_id)
> > +{
> > +	int ntries = 8;
> > +	int pin_val1, pin_val2;
> > +
> > +	/* additional deglitch, line can be noisy in badly designed PCB */
> > +	do {
> > +		pin_val1 = at91_get_gpio_value(irq);
> > +		pin_val2 = at91_get_gpio_value(irq);
> > +	} while (pin_val1 != pin_val2 && --ntries > 0);
> >   
>   I suggest a shorter code:
> 
> 	do {
> 		pin_val = at91_get_gpio_value(irq);
> 	} while (pin_val != at91_get_gpio_value(irq) && --ntries > 0);

For me your form is less readable.

> > +static int __init at91_ide_probe(struct platform_device *pdev)
> > +{
> >   
> [...]
> > +	host->ports[0]->extra_base = board->chipselect;
> >   
> 
>    BTW, we have 2 fields in the struct hwif_s that fit this case better: 
> config_data and select_data. It's a bit stange that you've selected 
> extra_base...

Ok.

Regards
Stanislaw Gruszka

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Stanislaw Gruszka wrote:

>>>diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
>>>index fb51f0e..6674b9b 100644
>>>--- a/arch/arm/mach-at91/include/mach/board.h
>>>+++ b/arch/arm/mach-at91/include/mach/board.h
>>>@@ -59,6 +59,18 @@ struct at91_cf_data {
>>> };
>>> extern void __init at91_add_device_cf(struct at91_cf_data *data);
>>> 
>>>+ /* Compact Flash True IDE mode */
>>>+struct at91_ide_data {
>>>+	u8	irq_pin;		/* the same meaning as for CF */

>>  I again have to express my dislike about not passing IRQ the usual 
>>way. Also, see my comments to the platform code.

> Yes, I know, I don't like to argue. Only reasoning to use platform irq resource
> seams to be: "because other drivers do". However we have exception - at91_cf
> also use board->irq_pin, so maybe this driver could also do ?

    Then why have the memory resource when we can calculate it from the chip 
select? (I'm not asking you to do that, since the platfrom device resources 
are user-visible thru /proc/iomem -- even if the driver is not enabled.)

>>>diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
>>>new file mode 100644
>>>index 0000000..3a1f7e0
>>>--- /dev/null
>>>+++ b/drivers/ide/at91_ide.c
>>>@@ -0,0 +1,496 @@
>>>+/*
>>>+ * IDE host driver for AT91SAM9 Static Memory Controller

>>   Why not call the driver 'at91sam9_ide'?

>>>+/*
>>>+ * AT91 Static Memory Controller

>>  AT91SAM9.

> Ok, currently only SAM9 can be used with driver. However I think adding
> support to AT91RM9200 to this driver will be not much effort.

    Can you answer the simple question: why we should try to support two 
incompatible chips with a single driver? Because the driver name will be 
shorter? :-)

> I don't think
> someone will want to write new driver for RM9200 insted using this one.

    You're right, nobody will want that... because AT91RM9200 as is has *no 
support for True IDE mode*. ;-)

> IMHO, current name is short and sweet and not miss the true so much -
> most of the AT91s which can work with True IDE logic are supported. 
> I would like to stay with current name.

    Oh, do what you want... just don't claim it will work on RM9200. :-/

>>>+ * different bus data width is used: 8Bit for Task File, 16Bit for Data I/O
>>>+ *
>>>+ * After initialization we do 8/16 bit flipping (changes in SMC MODE register)
>>>+ * only inside IDE callback routines which are serialized by IDE layer,
>>>+ * so no additional locking needed.
>>>+ */
>>>+
>>>+static void init_smc_mode(const u8 chipselect)
>>>+{
>>>+	at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
>>>+						  AT91_SMC_WRITEMODE |
>>>+						  AT91_SMC_BAT_SELECT |
>>>+						  AT91_SMC_TDF_(0));
>>>  

>>   I'm not sure why are you fixing the dataflow timing again, this time 
>>to 0...

> Not necessary code, will be removed.

    All this funciton seems pretty useless.

>>>+{
>>>+	unsigned long mode;
>>>+
>>>+	/* setup timings in SMC */
>>>+	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
>>>+						   AT91_SMC_NCS_WRSETUP_(0) |
>>>+						   AT91_SMC_NRDSETUP_(setup) |
>>>+						   AT91_SMC_NCS_RDSETUP_(0));
>>>+	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
>>>+						   AT91_SMC_NCS_WRPULSE_(cycle) |
>>>+						   AT91_SMC_NRDPULSE_(pulse) |
>>>+						   AT91_SMC_NCS_RDPULSE_(cycle));
>>>+	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
>>>+						   AT91_SMC_NRDCYCLE_(cycle));
>>>+
>>>+	mode = at91_sys_read(AT91_SMC_MODE(chipselect));

>>   Frankly speaking, I don't see why you're reading this register back 
>>if you already know what needs to be set there -- as you've done it in 
>>init_smc_mode().

> This function is not only used at initialization. It is also used when PIO mode
> is changed.

    So what? You can just write the fixed mode bits into the register every 
time without readback.

>>>+static void apply_timings(const u8 chipselect, const u8 pio,
>>>+			  const struct ide_timing *timing, const u16 *ata_id)
>>>+{

[...]

>>>+	if (ata_id) {
>>>+		if (ata_id_is_cfa(ata_id)) {
>>>+			if (pio == 3 || pio == 4)
>>>+				use_iordy = 1;
>>>  
>>>+		} else if (pio >= 3 || ata_id_has_iordy(ata_id))
>>>+			use_iordy = 1;

>>   No, you should check for IORDY regardless of CFA and using IORDY 
>>shouldn't be limeited to modes 3 and 4:

> Hmm, I take as example ata_pio_need_iordy(), it is also wrong or maybe
> I don't follow the logic properly.

    Let me look... yes, you don't follow it properly. Besides, it could have 
omitted checking for PIO mode > 2.

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>> diff --git a/arch/arm/mach-at91/include/mach/board.h 
>>>> b/arch/arm/mach-at91/include/mach/board.h
>>>> index fb51f0e..6674b9b 100644
>>>> --- a/arch/arm/mach-at91/include/mach/board.h
>>>> +++ b/arch/arm/mach-at91/include/mach/board.h
>>>> @@ -59,6 +59,18 @@ struct at91_cf_data {
>>>> };
>>>> extern void __init at91_add_device_cf(struct at91_cf_data *data);
>>>>
>>>> + /* Compact Flash True IDE mode */
>>>> +struct at91_ide_data {
>>>> +    u8    irq_pin;        /* the same meaning as for CF */

>>>  I again have to express my dislike about not passing IRQ the usual 
>>> way. Also, see my comments to the platform code.

>> Yes, I know, I don't like to argue. Only reasoning to use platform irq 
>> resource
>> seams to be: "because other drivers do". However we have exception - 
>> at91_cf
>> also use board->irq_pin, so maybe this driver could also do ?

>    Then why have the memory resource when we can calculate it from the 
> chip select? (I'm not asking you to do that, since the platfrom device 
> resources are user-visible thru /proc/iomem -- even if the driver is not 
> enabled.)

    Oh, I forgot that it's ARM with its #ifdef hell. :-D
Then they're only visible when the driver is enabled.

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka]

Wednesday 04 February 2009 17:04:27 Sergei Shtylyov napisał(a):
> >>> extern void __init at91_add_device_cf(struct at91_cf_data *data);
> >>> 
> >>>+ /* Compact Flash True IDE mode */
> >>>+struct at91_ide_data {
> >>>+	u8	irq_pin;		/* the same meaning as for CF */
> 
> >>  I again have to express my dislike about not passing IRQ the usual 
> >>way. Also, see my comments to the platform code.
> 
> > Yes, I know, I don't like to argue. Only reasoning to use platform irq resource
> > seams to be: "because other drivers do". However we have exception - at91_cf
> > also use board->irq_pin, so maybe this driver could also do ?
> 
>     Then why have the memory resource when we can calculate it from the chip 
> select?  
Great idea, I very like it :) But memory is a platform (cpu) resource, however
board dependend.

> (I'm not asking you to do that, since the platfrom device resources   
> are user-visible thru /proc/iomem -- even if the driver is not enabled.)
Let's distinguish platform (cpu) resources and board resources.

If you take a look at arch/arm/mach-at91/*_devices.c files, 
IORESOURCE_IRQ are used for interrupts from devices that are integrated
on the chip. Board specific irq pins (like in at91_cf, at91_ether) are not
passed to platform driver via platform_resource but via board data.

> >>>diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> >>>new file mode 100644
> >>>index 0000000..3a1f7e0
> >>>--- /dev/null
> >>>+++ b/drivers/ide/at91_ide.c
> >>>@@ -0,0 +1,496 @@
> >>>+/*
> >>>+ * IDE host driver for AT91SAM9 Static Memory Controller
> 
> >>   Why not call the driver 'at91sam9_ide'?
> 
> >>>+/*
> >>>+ * AT91 Static Memory Controller
> 
> >>  AT91SAM9.
> 
> > Ok, currently only SAM9 can be used with driver. However I think adding
> > support to AT91RM9200 to this driver will be not much effort.
> 
>     Can you answer the simple question: why we should try to support two 
> incompatible chips with a single driver? Because the driver name will be 
> shorter? :-)
Very funny. I think patch adding RM9200 support to this driver will have less
than 50 lines changeset, whereas writing new driver would be about 500
lines.

> > I don't think
> > someone will want to write new driver for RM9200 insted using this one.
> 
>     You're right, nobody will want that... because AT91RM9200 as is has *no 
> support for True IDE mode*. ;-)
Atmel documents are confusing. AT91RM9200 datasheet tells there is no
True IDE support, but RM9200 hard drive application note (which I send you
a link before) tell it is.

> >>   Frankly speaking, I don't see why you're reading this register back 
> >>if you already know what needs to be set there -- as you've done it in 
> >>init_smc_mode().
> 
> > This function is not only used at initialization. It is also used when PIO mode
> > is changed.
> 
>     So what? You can just write the fixed mode bits into the register every 
> time without readback.

Ok, I see now.

Cheers
Stanislaw Gruszka

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Stanislaw Gruszka wrote:

>>>>>extern void __init at91_add_device_cf(struct at91_cf_data *data);
>>>>>
>>>>>+ /* Compact Flash True IDE mode */
>>>>>+struct at91_ide_data {
>>>>>+	u8	irq_pin;		/* the same meaning as for CF */

>>>> I again have to express my dislike about not passing IRQ the usual 
>>>>way. Also, see my comments to the platform code.

>>>Yes, I know, I don't like to argue. Only reasoning to use platform irq resource
>>>seams to be: "because other drivers do". However we have exception - at91_cf
>>>also use board->irq_pin, so maybe this driver could also do ?

>>    Then why have the memory resource when we can calculate it from the chip 
>>select?  

> Great idea, I very like it :)

    It should have arisen from trying to follow your approach consistently.
    You can then drop the whole idea of using the platform device -- 'struct 
device' already has 'platform_data' member (although... there is a prominent 
example of a platfrom driver using only the platform data to pass all the 
resource information: drivers/serial/8250.c).

> But memory is a platform (cpu) resource,

    I don't get what you're trying to say. AFAIK, there has never been an 
*implication* that the platfrom device resources should be CPU-, and not 
board-specific.

> however board dependend.

    Haven't you just told us that it's not board, but CPU dependent?

>>(I'm not asking you to do that, since the platfrom device resources   
>>are user-visible thru /proc/iomem -- even if the driver is not enabled.)

> Let's distinguish platform (cpu) resources and board resources.

    I'm seeing no point in such distinction. The platfrom device framework was 
created exactly with the assorted on-board devices in mind.

> If you take a look at arch/arm/mach-at91/*_devices.c files, 
> IORESOURCE_IRQ are used for interrupts from devices that are integrated
> on the chip. Board specific irq pins (like in at91_cf, at91_ether) are not
> passed to platform driver via platform_resource but via board data.

    I don't care what the particular broken ARM platfrom code does. If it 
can't pass resources in a normal way, it should be fixed, and not followed as 
a good example.

>>>>>diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
>>>>>new file mode 100644
>>>>>index 0000000..3a1f7e0
>>>>>--- /dev/null
>>>>>+++ b/drivers/ide/at91_ide.c
>>>>>@@ -0,0 +1,496 @@
>>>>>+/*
>>>>>+ * IDE host driver for AT91SAM9 Static Memory Controller

>>>>  Why not call the driver 'at91sam9_ide'?

>>>>>+/*
>>>>>+ * AT91 Static Memory Controller

>>>> AT91SAM9.

>>>Ok, currently only SAM9 can be used with driver. However I think adding
>>>support to AT91RM9200 to this driver will be not much effort.

>>    Can you answer the simple question: why we should try to support two 
>>incompatible chips with a single driver? Because the driver name will be 
>>shorter? :-)

> Very funny. I think patch adding RM9200 support to this driver will have less
> than 50 lines changeset,  whereas writing new driver would be about 500 lines.

    This approach is so broken-minded that I'm just out words to argue any more.
    Let's then support say all the PCI IDE chipsets with the single driver 
(actully, there was a driver that tried to support 2 incompatible Promise chip 
families but it got split finally). Moreover, with AT91RM9200 lacking True IDE 
mode support, the code (if anybody in their right mind would undertake the 
task of adding IDE support) will be different enough from what is there 
already, that I doubt that that imaginary porgrammer would keep clinging to 
the doubtful idea to the very end... anyway, I'm now considering the very 
possibilty of anybody trying to do that minimal, exactly due to AT91RM9200 
lacking True IDE support.

>>>I don't think
>>>someone will want to write new driver for RM9200 insted using this one.

>>    You're right, nobody will want that... because AT91RM9200 as is has *no 
>>support for True IDE mode*. ;-)

> Atmel documents are confusing. AT91RM9200 datasheet tells there is no
> True IDE support,

    There is no confusion there -- at least for me. Yes, there is no True IDE 
support.

> but RM9200 hard drive application note (which I send you a link before) tell it is.

    That is just a total hack. I don't think anyone will really want to use 
the GPIO controlled chip selects that this application note describes... well, 
if only out of total desperation. :-)

> Cheers
> Stanislaw Gruszka

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Andrew Victor]

hi,

>>>>> AT91SAM9.
>
>>>> Ok, currently only SAM9 can be used with driver. However I think adding
>>>> support to AT91RM9200 to this driver will be not much effort.
>
>>>   Can you answer the simple question: why we should try to support two
>>> incompatible chips with a single driver? Because the driver name will be
>>> shorter? :-)
>
>> Very funny. I think patch adding RM9200 support to this driver will have
>> less
>> than 50 lines changeset,  whereas writing new driver would be about 500
>> lines.
>
>   This approach is so broken-minded that I'm just out words to argue any
> more.

This driver should also work on the Atmel AT91CAP9 and AT572D940HF processors.
So I see no valid reason why the driver cannot be called at91_ide.


Regards,
  Andrew Victor

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Andrew Victor wrote:

>>>>>>AT91SAM9.

>>>>>Ok, currently only SAM9 can be used with driver. However I think adding
>>>>>support to AT91RM9200 to this driver will be not much effort.

>>>>  Can you answer the simple question: why we should try to support two
>>>>incompatible chips with a single driver? Because the driver name will be
>>>>shorter? :-)

>>>Very funny. I think patch adding RM9200 support to this driver will have
>>>less
>>>than 50 lines changeset,  whereas writing new driver would be about 500
>>>lines.

>>  This approach is so broken-minded that I'm just out words to argue any
>>more.

> This driver should also work on the Atmel AT91CAP9 and AT572D940HF processors.
> So I see no valid reason why the driver cannot be called at91_ide.

    I've already said: call it whatever you want. Just don't try to add 
support for an incompatible AT91 SMC.

> Regards,
>   Andrew Victor

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka]

Thursday 05 February 2009 17:09:06 Sergei Shtylyov napisał(a):
> > If you take a look at arch/arm/mach-at91/*_devices.c files, 
> > IORESOURCE_IRQ are used for interrupts from devices that are integrated
> > on the chip. Board specific irq pins (like in at91_cf, at91_ether) are not
> > passed to platform driver via platform_resource but via board data.
> 
>     I don't care what the particular broken ARM platfrom code does. If it 
> can't pass resources in a normal way, it should be fixed, and not followed as 
> a good example.
So fix these divers, then I will have no bad examples. I'll stay with board->irq_pin
for now, moving to platform_resource could be done in separate patch (maybe
with at91_cf and at91_ether).

Cheers
Stanislaw Gruszka

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>    Can you answer the simple question: why we should try to support 
>> two incompatible chips with a single driver? Because the driver name 
>> will be shorter? :-)
>
>> Very funny. I think patch adding RM9200 support to this driver will 
>> have less
>> than 50 lines changeset,  whereas writing new driver would be about 
>> 500 lines.
>
>    This approach is so broken-minded that I'm just out words to argue 
> any more.
>    Let's then support say all the PCI IDE chipsets with the single 
> driver (actully, there was a driver that tried to support 2 
> incompatible Promise chip families but it got split finally).

   To say the truth, there are stil at least 2 examples of such drivers: 
hpt366 and aec6210. While the former is justified by the bogus chip 
identification poilicy used by the vendor, the latter has no 
justification at all.

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Bartlomiej Zolnierkiewicz]

On Friday 06 February 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>    Can you answer the simple question: why we should try to support 
> >> two incompatible chips with a single driver? Because the driver name 
> >> will be shorter? :-)
> >
> >> Very funny. I think patch adding RM9200 support to this driver will 
> >> have less
> >> than 50 lines changeset,  whereas writing new driver would be about 
> >> 500 lines.
> >
> >    This approach is so broken-minded that I'm just out words to argue 
> > any more.
> > 
> >    Let's then support say all the PCI IDE chipsets with the single 
> > driver (actully, there was a driver that tried to support 2 
> > incompatible Promise chip families but it got split finally).

Actually it was the case for Linux during early 2.4.x days. :)

[ Probably for historical reasons. ]

>    To say the truth, there are stil at least 2 examples of such drivers: 
> hpt366 and aec6210. While the former is justified by the bogus chip 

The former can be probably still improved with hpt3xx_main.c and chipset
family specific code separated into hpt36x.c etc.

> identification poilicy used by the vendor, the latter has no 
> justification at all.

Patches are always warmly welcomed.

Thanks,
Bart

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>   Can you answer the simple question: why we should try to support 
>>>>two incompatible chips with a single driver? Because the driver name 
>>>>will be shorter? :-)

>>>>Very funny. I think patch adding RM9200 support to this driver will 
>>>>have less
>>>>than 50 lines changeset,  whereas writing new driver would be about 
>>>>500 lines.

>>>   This approach is so broken-minded that I'm just out words to argue 
>>>any more.

>>>   Let's then support say all the PCI IDE chipsets with the single 
>>>driver (actully, there was a driver that tried to support 2 
>>>incompatible Promise chip families but it got split finally).

> Actually it was the case for Linux during early 2.4.x days. :)

    And I've spent considerable time fixing this driver in 2.4.20. :-)

> [ Probably for historical reasons. ]

>>   To say the truth, there are stil at least 2 examples of such drivers: 
>>hpt366 and aec6210. While the former is justified by the bogus chip 

> The former can be probably still improved with hpt3xx_main.c and chipset
> family specific code separated into hpt36x.c etc.

    Don't think that there's even enough common code for hpt3xx_main.c.

>>identification poilicy used by the vendor, the latter has no 
>>justification at all.

> Patches are always warmly welcomed.

    There is just to much I'd like to be done with this driver and too little 
time to do that. :-)

> Thanks,
> Bart

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Bartlomiej Zolnierkiewicz]

On Wednesday 04 February 2009, Sergei Shtylyov wrote:

[...]

> > +void at91_ide_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) {
> >   
> 
>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody ever used it.

It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to break it.

Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA to struct
ide_tp_ops -- it should also help some other host drivers like tx493*.

Thanks,
Bart

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>> +void at91_ide_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) {
>>>   
>>>       
>>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody ever used it.
>>     
>
> It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to break it.
>
> Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA to struct
> ide_tp_ops -- it should also help some other host drivers like tx493*.
>   

  That would be extremely senseless activity sicne I believe this flag 
is totally useless. I have better thing to do. :-)

> Thanks,
> Bart

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Bartlomiej Zolnierkiewicz]

On Friday 06 February 2009, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>> +void at91_ide_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) {
> >>>   
> >>>       
> >>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody ever used it.
> >>     
> >
> > It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to break it.
> >
> > Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA to struct
> > ide_tp_ops -- it should also help some other host drivers like tx493*.
> >   
> 
>   That would be extremely senseless activity sicne I believe this flag 
> is totally useless. I have better thing to do. :-)

I would love to remove this flag but it is used by user-space exposed
interface (which was used by few drive vendors for their diagnostics
tools -- doesn't matter whether internal or external) so you should put
some technical arguments behind its removal (you know many of low-level
technical details better than me so I may be missing something which is
obvious to you).

OTOH while ->{read,write}_data approach would result in something like
~50 extra LOC (or even less with be_tp_ops) compared to removal it is
completely safe and we don't need to spend a single second wondering
about potential breakage (one day this ioctl will go away together with
the flag and issue will solve itself).

Thanks,
Bart

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>> +void at91_ide_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) {
>>>>>   
>>>>>       
>>>>>           
>>>>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody ever used it.
>>>>     
>>>>         
>>> It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to break it.
>>>
>>> Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA to struct
>>> ide_tp_ops -- it should also help some other host drivers like tx493*.
>>>   
>>>       
>>   That would be extremely senseless activity sicne I believe this flag 
>> is totally useless. I have better thing to do. :-)
>>     
>
> I would love to remove this flag but it is used by user-space exposed
> interface 

   I know that...

> (which was used by few drive vendors for their diagnostics
> tools -- doesn't matter whether internal or external) so you should put
> some technical arguments behind its removal (you know many of low-level
> technical details better than me so I may be missing something which is
> obvious to you).
>   

   Well, the vendors can do strange things, of course...
   However, accessing the data register is certainly not a part of any 
ATA/PI defined command's inputs/outputs (the corresponding tables just 
don't have this register). I suspect that this flag was added just "for 
completeness".

> OTOH while ->{read,write}_data approach would result in something like
> ~50 extra LOC (or even less with be_tp_ops) compared to removal it is
> completely safe and we don't need to spend a single second wondering
> about potential breakage 

   Go for it. ;-)

> (one day this ioctl will go away together with
> the flag and issue will solve itself).
>   

  I certainly can live with this feature in the meantime... :-)
  BTW, looks like libata has already dropped HDIO_DRIVE_TASKFILE ioctl.

> Thanks,
> Bart
>   

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>>>> +void at91_ide_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) {
>>>>>>                   
>>>>>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody 
>>>>> ever used it.
>>>>>             
>>>> It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to break 
>>>> it.
>>>>
>>>> Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA to 
>>>> struct
>>>> ide_tp_ops -- it should also help some other host drivers like tx493*.
>>>>         
>>>   That would be extremely senseless activity sicne I believe this 
>>> flag is totally useless. I have better thing to do. :-)
>>>     
>>
>> I would love to remove this flag but it is used by user-space exposed
>> interface 
>
>   I know that...
>
>> (which was used by few drive vendors for their diagnostics
>> tools -- doesn't matter whether internal or external) so you should put
>> some technical arguments behind its removal (you know many of low-level
>> technical details better than me so I may be missing something which is
>> obvious to you).
>>   
>
>   Well, the vendors can do strange things, of course...
>   However, accessing the data register is certainly not a part of any 
> ATA/PI defined command's inputs/outputs (the corresponding tables just 
> don't have this register). I suspect that this flag was added just 
> "for completeness".
>
>> OTOH while ->{read,write}_data approach would result in something like
>> ~50 extra LOC (or even less with be_tp_ops) compared to removal it is
>> completely safe and we don't need to spend a single second wondering
>> about potential breakage 
>
>   Go for it. ;-)

   I think I have a better idea than creating another (useless) couple 
of "transport" methods. Why not (ab)use the exisitng {in|out}put_data() 
methods instead? :-)

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>>>>> +void at91_ide_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) {
>>>>>>>                   
>>>>>>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody 
>>>>>> ever used it.
>>>>>>             
>>>>> It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to 
>>>>> break it.
>>>>>
>>>>> Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA 
>>>>> to struct
>>>>> ide_tp_ops -- it should also help some other host drivers like 
>>>>> tx493*.
>>>>>         
>>>>   That would be extremely senseless activity sicne I believe this 
>>>> flag is totally useless. I have better thing to do. :-)
>>>>     
>>>
>>> I would love to remove this flag but it is used by user-space exposed
>>> interface 
>>
>>   I know that...
>>
>>> (which was used by few drive vendors for their diagnostics
>>> tools -- doesn't matter whether internal or external) so you should put
>>> some technical arguments behind its removal (you know many of low-level
>>> technical details better than me so I may be missing something which is
>>> obvious to you).
>>>   
>>
>>   Well, the vendors can do strange things, of course...
>>   However, accessing the data register is certainly not a part of any 
>> ATA/PI defined command's inputs/outputs (the corresponding tables 
>> just don't have this register). I suspect that this flag was added 
>> just "for completeness".
>>
>>> OTOH while ->{read,write}_data approach would result in something like
>>> ~50 extra LOC (or even less with be_tp_ops) compared to removal it is
>>> completely safe and we don't need to spend a single second wondering
>>> about potential breakage 
>>
>>   Go for it. ;-)
>
>   I think I have a better idea than creating another (useless) couple 
> of "transport" methods. Why not (ab)use the exisitng 
> {in|out}put_data() methods instead? :-)

   Besides, those methods are clearly subject to some size 
optimization... I'll cook up a patch while I'm at it. :-)

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Bartlomiej Zolnierkiewicz]

On Sunday 08 February 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>>>>>> +void at91_ide_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) {
> >>>>>>>                   
> >>>>>>    Sigh. Bart, couldn't we drop that stupid flag? I bet nobody 
> >>>>>> ever used it.
> >>>>>>             
> >>>>> It is there for HDIO_DRIVE_TASKFILE ioctl and I prefer not to 
> >>>>> break it.
> >>>>>
> >>>>> Just add ->{read,write}_data methods for IDE_TFLAG_{IN,OUT}_DATA 
> >>>>> to struct
> >>>>> ide_tp_ops -- it should also help some other host drivers like 
> >>>>> tx493*.
> >>>>>         
> >>>>   That would be extremely senseless activity sicne I believe this 
> >>>> flag is totally useless. I have better thing to do. :-)
> >>>>     
> >>>
> >>> I would love to remove this flag but it is used by user-space exposed
> >>> interface 
> >>
> >>   I know that...
> >>
> >>> (which was used by few drive vendors for their diagnostics
> >>> tools -- doesn't matter whether internal or external) so you should put
> >>> some technical arguments behind its removal (you know many of low-level
> >>> technical details better than me so I may be missing something which is
> >>> obvious to you).
> >>>   
> >>
> >>   Well, the vendors can do strange things, of course...
> >>   However, accessing the data register is certainly not a part of any 
> >> ATA/PI defined command's inputs/outputs (the corresponding tables 
> >> just don't have this register). I suspect that this flag was added 
> >> just "for completeness".
> >>
> >>> OTOH while ->{read,write}_data approach would result in something like
> >>> ~50 extra LOC (or even less with be_tp_ops) compared to removal it is
> >>> completely safe and we don't need to spend a single second wondering
> >>> about potential breakage 
> >>
> >>   Go for it. ;-)
> >
> >   I think I have a better idea than creating another (useless) couple 
> > of "transport" methods. Why not (ab)use the exisitng 
> > {in|out}put_data() methods instead? :-)

Good idea.

>    Besides, those methods are clearly subject to some size 
> optimization... I'll cook up a patch while I'm at it. :-)

:)

Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka]

Wednesday 04 February 2009 13:19:50 Sergei Shtylyov napisał(a):
> [...]
> > +	host->ports[0]->extra_base = board->chipselect;
> >   
> 
>    BTW, we have 2 fields in the struct hwif_s that fit this case better: 
> config_data and select_data. It's a bit stange that you've selected 
> extra_base...
config_data is in use by IDE core, I'll switch to select_data.

Stanislaw Gruszka

Re: [PATCH 2/3] ide: add at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

>>> +	host->ports[0]->extra_base = board->chipselect;
>>>   
>>>       
>>    BTW, we have 2 fields in the struct hwif_s that fit this case better: 
>> config_data and select_data. It's a bit stange that you've selected 
>> extra_base...
>>     
> config_data is in use by IDE core,

   Hm, it shouldn't be. These fields are exclusively for drivers' use.

MBR, Sergei

Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka]

Friday 06 February 2009 11:36:44 Sergei Shtylyov napisał(a):
> >>> +	host->ports[0]->extra_base = board->chipselect;
> >>>   
> >>>       
> >>    BTW, we have 2 fields in the struct hwif_s that fit this case better: 
> >> config_data and select_data. It's a bit stange that you've selected 
> >> extra_base...
> >>     
> > config_data is in use by IDE core,
> 
>    Hm, it shouldn't be. These fields are exclusively for drivers' use.
Right, it is just overwritten in ide_init_port_hw() and should be
setup by hw->config.

Stanislaw Gruszka