[RFC][PATCH] at91_ide driver

[RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Hi

I'm writting IDE host driver for AT91 Static Memory Controller with Compact Flash
True IDE logic. My company - Kelvatek wants to add driver into the mainline,
we think it can be usesfull for others and including driver will be also beneficial
for us obviously.

Driver is not finished, it is submitted just for review. At least tests
for sam9263ek board should be done and ide_intr() quirk rewritten
more cleanly (see below).

Driver is divided into tree parts: generic IDE, processor specific 
(currently only AT91SAM9263 is supported) and board specific.

Some issues with driver:

* Why not platform driver
Generic pata/ide_platform driver will not work with AT91 as we need to 
do special things before access Task File and Data Register (functions 
set_8bit_mode() and set_16bit_mode()). Also extra things needs to be
done when changing PIO mode.

* Tests
We tested with our custom board which is slightly diffrent than
Atmel Evaluation Kit. Pins settings in the patch are from Kelvatek's board,
to run driver on sam9263ek board settings shall be changed. We did
not tests on Atmel sam9263ek board, because we have no proper connector
for 1,8'' HDD. If someone could do tests for these board, I will be very gracefull. 

* GPIO interrupt
This is the most problem with the driver/hardware. Interrupt from device is
delivered through GPIO. In AT91 GPIO interrupts are triggered on falling
and raising edge of signal (this behaviour is not configured IIRC). Whereas
IDE device request interrupt on high level (raising edge in our case). This
mean we have fake interrupts requests on cpu, which broke IDE layer. 
Problem can be solved in hardware: when device INTRQ line is connected
to CPU through AIC controlled IRQ0 or IRQ1 lines with proper level configured.
We probably do such things in our board, but it's is rather impossible
for Atmel Evaluation Kit. Also there is workaround in software: check interrupt
pin value and exit instantly from ISR when line is on low level. Such thing
is done in the patch (AT91_GPIO_IRQ_HACK in drivers/ide/ide-io.c), but
this is dirty hack and should implemented differently (I hope someone
could give my good idea how).

* Performance
Driver works only in PIO mode, we have no hardware for DMA mode. 
Probably  hardware support could be done using another Chip Select
with Static Memory Controller and extra external logic. Maybe Atmel
people could tell someting more about this issue?  On PIO4 mode 
I achieve 4,5MB/s transfer with 100% CPU usage (CPU is clocked with 200MHz).

* Maintenance
After inclusion, when I finish the driver, I hope someone from Atmel: 
Nicolas or Haavard could take maintenance of it.

Regards
Stanislaw Gruszka

Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>

---
 arch/arm/mach-at91/at91sam9263_devices.c |   96 +++++++
 arch/arm/mach-at91/board-sam9263ek.c     |   11 +
 arch/arm/mach-at91/include/mach/board.h  |    9 +
 drivers/ide/Kconfig                      |    4 +
 drivers/ide/Makefile                     |    1 +
 drivers/ide/at91_ide.c                   |  459 ++++++++++++++++++++++++++++++
 drivers/ide/ide-io.c                     |   22 ++-
 7 files changed, 601 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ide/at91_ide.c

diff --git a/arch/arm/mach-at91/at91sam9263_devices.c b/arch/arm/mach-at91/at91sam9263_devices.c
index 8b88408..976e228 100644
--- a/arch/arm/mach-at91/at91sam9263_devices.c
+++ b/arch/arm/mach-at91/at91sam9263_devices.c
@@ -347,6 +347,102 @@ void __init at91_add_device_mmc(short mmc_id, struct at91_mmc_data *data)
 void __init at91_add_device_mmc(short mmc_id, struct at91_mmc_data *data) {}
 #endif
 
+/* --------------------------------------------------------------------
+ *  IDE
+ * -------------------------------------------------------------------- */
+
+#if defined(CONFIG_BLK_DEV_IDE_AT91) || defined(CONFIG_BLK_DEV_IDE_AT91_MODULE)
+
+/* Proper CS address space will be added */
+#define AT91_IDE_TASK_FILE	0x00c00000
+#define AT91_IDE_CTRL_REG	0x00e00000
+
+static struct resource ide_resources[] = {
+	[0] = {
+		.start	= AT91_IDE_TASK_FILE,
+		.end	= AT91_IDE_TASK_FILE + 8 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= AT91_IDE_CTRL_REG,
+		.end	= AT91_IDE_CTRL_REG, /* 1 byte */
+		.flags	= IORESOURCE_MEM,
+	},
+};
+
+static struct at91_ide_data ide_data;
+
+static struct platform_device at91_ide_device = {
+	.name	= "at91_ide",
+	.id 	= -1,
+	.dev	= {
+		.platform_data = &ide_data,
+	},
+	.resource = ide_resources,
+	.num_resources = ARRAY_SIZE(ide_resources),
+};
+
+void __init at91_add_device_ide(struct at91_ide_data *data)
+{
+	unsigned long ebi0_csa, addr_space;
+	u8 chipselect = data->chipselect;
+
+	/* enable PIO controlled pins, inputs with pull ups */
+	if (data->rst_pin)
+		at91_set_gpio_output(data->rst_pin, 0); /* reset card */
+
+	at91_set_gpio_input(data->irq_pin, 1);
+	at91_set_deglitch(data->irq_pin, 1);
+
+	if (data->det_pin) {
+		at91_set_gpio_input(data->det_pin, 1);
+		at91_set_deglitch(data->det_pin, 1);
+	}
+
+	/* enable EBI SMC controlled pins */
+	at91_set_A_periph(AT91_PIN_PC5, 1);  /* nWAIT */
+	at91_set_A_periph(AT91_PIN_PD6, 0);  /* CFCS0 */
+	at91_set_A_periph(AT91_PIN_PD8, 0);  /* CFCE1 */
+	at91_set_A_periph(AT91_PIN_PD9, 0);  /* CFCE2 */
+	at91_set_A_periph(AT91_PIN_PD14, 0); /* CFNRW */
+
+	/* assign CS4/5 to SMC with Compact Flash logic support
+	 * and fix resources addresses */
+	ebi0_csa = at91_sys_read(AT91_MATRIX_EBI0CSA);
+	switch (chipselect) {
+	case 4:
+		ebi0_csa |= AT91_MATRIX_EBI0_CS4A_SMC_CF1;
+		addr_space = AT91_CHIPSELECT_4;
+		break;
+	case 5:
+		ebi0_csa |= AT91_MATRIX_EBI0_CS5A_SMC_CF2;
+		addr_space = AT91_CHIPSELECT_5;
+		break;
+	default:
+		printk(KERN_ERR "at91_ide: bad chip select %u\n", chipselect);
+		return;
+	}
+	at91_sys_write(AT91_MATRIX_EBI0CSA, ebi0_csa);
+	ide_resources[0].start += addr_space;
+	ide_resources[0].end += addr_space;
+	ide_resources[1].start += addr_space;
+	ide_resources[1].end += addr_space;
+
+	/* turn on the card if reset pin is GPIO */
+	if (data->rst_pin)
+		at91_set_gpio_value(data->rst_pin, 1);
+
+	if (data->det_pin && at91_get_gpio_value(data->det_pin) != 0) {
+		printk(KERN_ERR "at91_ide: no Compact Flash card detected\n");
+		return ;
+	}
+
+	ide_data = *data;
+	platform_device_register(&at91_ide_device);
+}
+#else
+void __init at91_add_device_ide(struct at91_ide_data *data) {}
+#endif
 
 /* --------------------------------------------------------------------
  *  NAND / SmartMedia
diff --git a/arch/arm/mach-at91/board-sam9263ek.c b/arch/arm/mach-at91/board-sam9263ek.c
index 8354015..3884abf 100644
--- a/arch/arm/mach-at91/board-sam9263ek.c
+++ b/arch/arm/mach-at91/board-sam9263ek.c
@@ -366,6 +366,15 @@ static struct gpio_led ek_pwm_led[] = {
 	}
 };
 
+/*
+ * IDE
+ */
+static struct at91_ide_data ek_ide_data = {
+	.irq_pin	= AT91_PIN_PB20,
+	.det_pin	= AT91_PIN_PB22,
+	.rst_pin	= AT91_PIN_PB19,
+	.chipselect	= 4,
+};
 
 static void __init ek_board_init(void)
 {
@@ -397,6 +406,8 @@ static void __init ek_board_init(void)
 	/* LEDs */
 	at91_gpio_leds(ek_leds, ARRAY_SIZE(ek_leds));
 	at91_pwm_leds(ek_pwm_led, ARRAY_SIZE(ek_pwm_led));
+	/* IDE */
+	at91_add_device_ide(&ek_ide_data);
 }
 
 MACHINE_START(AT91SAM9263EK, "Atmel AT91SAM9263-EK")
diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index fb51f0e..a2fbcfa 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h
@@ -59,6 +59,15 @@ 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;
+};
+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 e6857e0..89b990a 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -722,6 +722,10 @@ 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
+
 config IDE_ARM
 	tristate "ARM IDE support"
 	depends on ARM && (ARCH_CLPS7500 || ARCH_RPC || ARCH_SHARK)
diff --git a/drivers/ide/Makefile b/drivers/ide/Makefile
index 7818d40..8d60b34 100644
--- a/drivers/ide/Makefile
+++ b/drivers/ide/Makefile
@@ -113,3 +113,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..f82ecb9
--- /dev/null
+++ b/drivers/ide/at91_ide.c
@@ -0,0 +1,459 @@
+/*
+ * IDE host driver for AT91 Static Memory Controler
+ * 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 DEBUG 1
+#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
+ */
+
+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_(2));
+}
+
+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 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 set_ebi_timings(const u8 chipselect, const u8 pio)
+{
+	/* Compact Flash True IDE mode timings, all values in nano seconds,
+	 * see table 22 of standard 4.1 */
+	const struct { unsigned int t0, t1, t2, t2i, t9; } timings[7] = {
+		{ .t0 = 600, .t1 = 70, .t2 = 290, .t2i =  0, .t9 = 20 }, /* PIO 0 */
+		{ .t0 = 383, .t1 = 50, .t2 = 290, .t2i =  0, .t9 = 15 }, /* PIO 1 */
+		{ .t0 = 240, .t1 = 30, .t2 = 290, .t2i =  0, .t9 = 10 }, /* PIO 2 */
+		{ .t0 = 180, .t1 = 30, .t2 =  80, .t2i = 70, .t9 = 10 }, /* PIO 3 */
+		{ .t0 = 120, .t1 = 25, .t2 =  70, .t2i = 25, .t9 = 10 }, /* PIO 4 */
+		{ .t0 = 100, .t1 = 15, .t2 =  65, .t2i = 25, .t9 = 10 }, /* PIO 5 */
+		{ .t0 =  80, .t1 = 10, .t2 =  55, .t2i = 20, .t9 = 10 }, /* PIO 6 */
+	};
+	unsigned int t0, t1, t2, t2i, t9;
+	unsigned int mck_hz;
+	struct clk *mck;
+	unsigned long mode;
+
+	BUG_ON(pio > 6);
+
+	t0 = timings[pio].t0;
+	t1 = timings[pio].t1;
+	t2 = timings[pio].t2;
+	t2i = timings[pio].t2i;
+	t9 = timings[pio].t9;
+
+	if (t2i > 0) {
+		/* t1 is the part of t2i, let's t9 be the rest of t2i */
+		WARN_ON(t2i < t1);
+		if (t9 < t2i - t1)
+			t9 = t2i - t1;
+	}
+
+	mck = clk_get(NULL, "mck");
+	BUG_ON(IS_ERR(mck));
+	mck_hz = clk_get_rate(mck);
+	pdbg("mck_hz=%u\n", mck_hz);
+
+	t0 = calc_mck_cycles(t0, mck_hz);
+	t1 = calc_mck_cycles(t1, mck_hz);
+	t2 = calc_mck_cycles(t2, mck_hz);
+	t2i = calc_mck_cycles(t2i, mck_hz);
+	t9 = calc_mck_cycles(t9, mck_hz);
+	pdbg("t0=%u t1=%u t2=%u t2i=%u t9=%u\n", t0, t1, t2, t2i, t9);
+
+	clk_put(mck);
+
+	/* values are rounded up so we need to assure cycle is larger than pulse */
+	if (t0 < t1 + t2 + t9)
+		t0 = t1 + t2 + t9;
+
+	/* setup calculated timings */
+	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
+						   AT91_SMC_NCS_WRSETUP_(0) |
+						   AT91_SMC_NRDSETUP_(t1) |
+						   AT91_SMC_NCS_RDSETUP_(0));
+	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
+						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
+						   AT91_SMC_NRDPULSE_(t2) |
+						   AT91_SMC_NCS_RDPULSE_(t1 + t2 + t9));
+	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(t0) |
+						   AT91_SMC_NRDCYCLE_(t0));
+
+	/* disable or enable waiting for IORDY signal */
+	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
+	mode &= ~AT91_SMC_EXNWMODE;
+	if (pio <= 4)
+		mode |= AT91_SMC_EXNWMODE_FROZEN;
+	else 
+		mode |= AT91_SMC_EXNWMODE_DISABLE;
+	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+}
+
+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 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 void at91_ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	u8 chipselect = drive->hwif->extra_base;
+
+	pdbg("pio %u\n", pio);
+
+	if (pio > 6) {
+		perr("can't set PIO %d mode\n", pio);
+		return;
+	}
+	set_ebi_timings(chipselect, pio);
+}
+
+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,
+	.pio_mask = ATA_PIO6,
+};
+
+static int __init at91_ide_probe(struct platform_device *pdev)
+{
+	int ret = -ENOMEM;
+	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;
+
+	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");
+		goto err;
+	}
+
+	pdbg("chipselect %u irq %u tf %lu ctl %lu\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");
+		goto err_unmap;
+	}
+
+	memset(&hw, 0, sizeof(hw));
+#if 0
+	/* In 12-0275-01 version of the PCB address
+	 * lines CF_A0 and CF_A2 are swapped */
+	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;
+#else
+	/* Proper lines addresses */
+	hw.io_ports.data_addr = tf_base + 0;
+	hw.io_ports.error_addr = tf_base + 1;
+	hw.io_ports.nsect_addr = tf_base + 2;
+	hw.io_ports.lbal_addr = tf_base + 3;
+	hw.io_ports.lbam_addr = tf_base + 4;
+	hw.io_ports.lbah_addr =	tf_base + 5;
+	hw.io_ports.device_addr = tf_base + 6;
+	hw.io_ports.command_addr = tf_base + 7;
+#endif
+	hw.io_ports.ctl_addr = ctl_base;
+	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");
+		goto err_unmap;
+	}
+
+	/* setup Static Memory Controller - PIO 0 as default */
+	init_smc_mode(board->chipselect);
+	set_ebi_timings(board->chipselect, 0);
+
+	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);
+err_unmap:
+	if (ctl_base)
+		devm_iounmap(&pdev->dev, (void __iomem *) ctl_base);
+	if (tf_base)
+		devm_iounmap(&pdev->dev, (void __iomem *) tf_base);
+err:
+	return ret;
+}
+
+static int __exit at91_ide_remove(struct platform_device *pdev)
+{
+	struct ide_host *host = platform_get_drvdata(pdev);
+	ide_hwif_t *hwif = host->ports[0];
+	unsigned long data_addr = hwif->io_ports.data_addr;
+	unsigned long ctl_addr = hwif->io_ports.ctl_addr;
+
+	ide_host_remove(host);
+	devm_iounmap(&pdev->dev, (void __iomem *) data_addr);
+	devm_iounmap(&pdev->dev, (void __iomem *) ctl_addr);
+
+	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>");
+
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index cc35d6d..6dba1fc 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1354,7 +1354,12 @@ static void unexpected_intr (int irq, ide_hwgroup_t *hwgroup)
  *	request completed. At this point we issue the next request
  *	on the hwgroup and the process begins again.
  */
- 
+#define AT91_GPIO_IRQ_HACK
+
+#ifdef AT91_GPIO_IRQ_HACK
+#include <mach/gpio.h>
+#endif 
+
 irqreturn_t ide_intr (int irq, void *dev_id)
 {
 	unsigned long flags;
@@ -1364,6 +1369,21 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 	ide_handler_t *handler;
 	ide_startstop_t startstop;
 
+#ifdef AT91_GPIO_IRQ_HACK
+#define NR_TRIES 10
+	int ntries = 0;
+	int pin_val1, pin_val2;	
+	do {
+		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
+		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
+	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
+
+	udelay(20); // XXX: this need to be here otherwise IDE layer losts interrups, don't know why !!!
+	if (pin_val1 == 0 || ntries > NR_TRIES)
+		return IRQ_HANDLED;
+#undef NR_TIRES
+#endif
+ 
 	spin_lock_irqsave(&ide_lock, flags);
 	hwif = hwgroup->hwif;
 
-- 
1.5.2.5

Re: [RFC][PATCH] at91_ide driver

[Haavard Skinnemoen]

Stanislaw Gruszka wrote:
> Some issues with driver:
> 
> * Why not platform driver
> Generic pata/ide_platform driver will not work with AT91 as we need to 
> do special things before access Task File and Data Register (functions 
> set_8bit_mode() and set_16bit_mode()). Also extra things needs to be
> done when changing PIO mode.

Why not use libata? There's already a pata_at32 driver there which
should be usable on at91 with perhaps a few minor changes.

> * Maintenance
> After inclusion, when I finish the driver, I hope someone from Atmel: 
> Nicolas or Haavard could take maintenance of it.

I'm not really involved with AT91 in any way except that I want to
share as many drivers as possible between AT91 and AVR32. So I don't
think I can take responsibility for an AT91-only driver.

Haavard

Re: [RFC][PATCH] at91_ide driver

[Alan Cox]

> +#ifdef AT91_GPIO_IRQ_HACK
> +#define NR_TRIES 10
> +	int ntries = 0;
> +	int pin_val1, pin_val2;	
> +	do {
> +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);

You really don't want to put special board specific code in generic
locations. In the libata case you don't need to and I think in the ide
case you can avoid it too by wrapping the IRQ handler. Libata also
supports polled mode.

Now one other question - you use the 8/16bit set to flip between register
write sizes. Can you explain exactly what guarantees you don't take an
IRQ between setting the size and a write and having the IRQ handler or a
timer run in the middle, call other IDE routines and mess up because of
this.

I see nothing guaranteeing atomicity here and I suspect you either need
locks or (for single processor only) some carefully arranged scheme to
'stack' the 8/16bit select status.

Other comments:
	- The old and new ATA layers both have timing tables and timing
functions so you don't need all the duplicated timing table logic. 

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Wednesday 14 January 2009 13:58:32 Haavard Skinnemoen napisał(a):
> Stanislaw Gruszka wrote:
> > Some issues with driver:
> > 
> > * Why not platform driver
> > Generic pata/ide_platform driver will not work with AT91 as we need to 
> > do special things before access Task File and Data Register (functions 
> > set_8bit_mode() and set_16bit_mode()). Also extra things needs to be
> > done when changing PIO mode.
> 
> Why not use libata? There's already a pata_at32 driver there which
> should be usable on at91 with perhaps a few minor changes.
Because I didn't know about this driver. I searched someting for AT91
and missed that one. Surly in mainline AT91 support should be added
for pata_at32 driver. In my company case we want to go into production
with 2.6.27 kernel, PATA is marked as experimental even in 2.6.28. 
I don't know if better to use PATA or IDE stack for our case.

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Wednesday 14 January 2009 14:17:27 Alan Cox napisał(a):
> > +#ifdef AT91_GPIO_IRQ_HACK
> > +#define NR_TRIES 10
> > +	int ntries = 0;
> > +	int pin_val1, pin_val2;	
> > +	do {
> > +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> > +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> > +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
> 
> You really don't want to put special board specific code in generic
> locations. In the libata case you don't need to and I think in the ide
> case you can avoid it too by wrapping the IRQ handler. Libata also
> supports polled mode.
I was thinking about that and don't know why I refused idea of 
wrapping irq handler.

> Now one other question - you use the 8/16bit set to flip between register
> write sizes. Can you explain exactly what guarantees you don't take an
> IRQ between setting the size and a write and having the IRQ handler or a
> timer run in the middle, call other IDE routines and mess up because of
> this.
> I see nothing guaranteeing atomicity here and I suspect you either need
> locks or (for single processor only) some carefully arranged scheme to
> 'stack' the 8/16bit select status.
I thought ide_port callbacks are serialized by IDE layer. I will use locks, I think.

Thanks
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Alan Cox]

> > I see nothing guaranteeing atomicity here and I suspect you either need
> > locks or (for single processor only) some carefully arranged scheme to
> > 'stack' the 8/16bit select status.
> I thought ide_port callbacks are serialized by IDE layer. I will use locks, I think.

So all your accesses are within the various IDE locks so you never
taken an interrupt or timer at the wrong moment. Ok - that should be
documented though.

Alan

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Haavard Skinnemoen wrote:

>>* Why not platform driver
>>Generic pata/ide_platform driver will not work with AT91 as we need to 
>>do special things before access Task File and Data Register (functions 
>>set_8bit_mode() and set_16bit_mode()). Also extra things needs to be
>>done when changing PIO mode.

> Why not use libata?

    Why libata? You absolutely need the larger code footprint with the added 
"bonus" of SCSI disk emulation? :-)

> There's already a pata_at32 driver there which
> should be usable on at91 with perhaps a few minor changes.

    Well, if there's already close driver, it changes the matter...

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>> +#ifdef AT91_GPIO_IRQ_HACK
>> +#define NR_TRIES 10
>> +	int ntries = 0;
>> +	int pin_val1, pin_val2;	
>> +	do {
>> +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
>> +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
>> +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
>>     
>
> You really don't want to put special board specific code in generic
> locations. In the libata case you don't need to and I think in the ide
> case you can avoid it too by wrapping the IRQ handler.

   Unfortunately, it seems you can't wrap ide_intr(), at least with the 
current code.

> Libata also supports polled mode.
>   

   Yeah. Stanslaw, I'd (have to) advise going the libata way in this 
case -- in case you want to avoid the additional trouble of porting this 
broken-minded IRQ implementation to the IDE core... although the real 
issue seems to olny be with the development board.

> Other comments:
> 	- The old and new ATA layers both have timing tables and timing
> functions so you don't need all the duplicated timing table logic.
>   

   Stanislaw's patch is adding the DIOx- to address hold time (t9) to 
the existing ones. While there's has been already a patch by David Daney 
adding this timing to libata (however, the author have ditched this idea 
finally), the table in ide-timings.c still misses it, as well as the PIO 
mode 6 timings...
   Hm, besides the address setup and active/recovery times there seem 
wrong for the PIO mode 5: they should be 15 and 65/25, not 20 and 50/30. 
Bart, are you reading this? :-)

WBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Friday 16 January 2009 14:32:05 Sergei Shtylyov napisał(a):
> > You really don't want to put special board specific code in generic
> > locations. In the libata case you don't need to and I think in the ide
> > case you can avoid it too by wrapping the IRQ handler.
> 
>    Unfortunately, it seems you can't wrap ide_intr(), at least with the 
> current code.
Perhaps only EXPORT_SYMBOL is missed. There is some ide_intr lock function,
but it is only used for m68k.
 
> > Libata also supports polled mode.
> >   
> 
>    Yeah. Stanslaw, I'd (have to) advise going the libata way in this 
> case -- in case you want to avoid the additional trouble of porting this 
> broken-minded IRQ implementation to the IDE core... 
I think, I will try to add AT91 stuff for pata_at91 to have this hardware
support in mineline. I will also finish at91_ide for my company usage with
older kernel.
 
>    Stanislaw's patch is adding the DIOx- to address hold time (t9) to 
> the existing ones. While there's has been already a patch by David Daney 
> adding this timing to libata (however, the author have ditched this idea 
> finally), the table in ide-timings.c still misses it, as well as the PIO 
> mode 6 timings...
I use t9 for calculating memory controller settings, but maybe it is unneeded,
pata_at91 don't use this value.

Cheers
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

>>>You really don't want to put special board specific code in generic
>>>locations. In the libata case you don't need to and I think in the ide
>>>case you can avoid it too by wrapping the IRQ handler.

>>   Unfortunately, it seems you can't wrap ide_intr(), at least with the 
>>current code.

> Perhaps only EXPORT_SYMBOL is missed.

    No, init_irq() always calls request_irq() with ide_intr as argument.

>>>Libata also supports polled mode.

>>   Yeah. Stanslaw, I'd (have to) advise going the libata way in this 
>>case -- in case you want to avoid the additional trouble of porting this 
>>broken-minded IRQ implementation to the IDE core... 

> I think, I will try to add AT91 stuff for pata_at91 to have this hardware
> support in mineline. I will also finish at91_ide for my company usage with
> older kernel.

    Well, it can also be accepted into the IDE subsystem (minus the IRQ hack) 
-- it would fit the embedded use case better due to a lower memory  footprint 
(and not having to emulate SCSI). Libata has been known to lose to IDE core in 
the PIO transfer speeds (very significantly) though I'm not sure of the 
current state of affairs...

>>   Stanislaw's patch is adding the DIOx- to address hold time (t9) to 
>>the existing ones. While there's has been already a patch by David Daney 
>>adding this timing to libata (however, the author have ditched this idea 
>>finally), the table in ide-timings.c still misses it, as well as the PIO 
>>mode 6 timings...

> I use t9 for calculating memory controller settings, but maybe it is unneeded,
> pata_at91 don't use this value.

    You mean pata_at32? It might have been that this detail was missed...

> Cheers
> Stanislaw Gruszka

WBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Alan Cox]

> the PIO transfer speeds (very significantly) though I'm not sure of the 
> current state of affairs...

16bit only performance is identical (as you'd expect) 32bit is now
identical with the 32bit pio patches applied

Re: [RFC][PATCH] at91_ide driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday 16 January 2009, Sergei Shtylyov wrote:
> Hello.
> 
> Alan Cox wrote:
> 
> >> +#ifdef AT91_GPIO_IRQ_HACK
> >> +#define NR_TRIES 10
> >> +	int ntries = 0;
> >> +	int pin_val1, pin_val2;	
> >> +	do {
> >> +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> >> +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> >> +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
> >>     
> >
> > You really don't want to put special board specific code in generic
> > locations. In the libata case you don't need to and I think in the ide
> > case you can avoid it too by wrapping the IRQ handler.
> 
>    Unfortunately, it seems you can't wrap ide_intr(), at least with the 
> current code.

Well... there shouldn't be much problem with:

* adding ->irq_handler method to struct ide_port_info and struct ide_host

  [ which reminds me that struct ide_port_info would be better named struct
    ide_host_info and IIRC somebody has already noticed it in the past ;-) ]

* exporting ide_intr()

* adding ide_interrupt() wrapper around ide_intr() which will do sth like:

	if (host->irq_handler)
		return host->irq_handler()
	else
		return ide_intr()

  and then passing &ide_interrupt instead of &ide_intr to request_irq()

* implementing at91_irq_handler()

* Et Voila!

In the longer term it would also be useful for other purposes
(like adding ATA-like flash devices support to IDE).

> > Libata also supports polled mode.
> >   
> 
>    Yeah. Stanslaw, I'd (have to) advise going the libata way in this 
> case -- in case you want to avoid the additional trouble of porting this 
> broken-minded IRQ implementation to the IDE core... although the real 
> issue seems to olny be with the development board.

Enhancing pata_at32 seems to be also a good idea but at91_ide
looks almost ready for merge, is clean and relatively simple.

I think that the only things needing fixing before merge are:

- IRQ hack

- not using IDE_TIMINGS

and both should be easy to address IMHO.

> > Other comments:
> > 	- The old and new ATA layers both have timing tables and timing
> > functions so you don't need all the duplicated timing table logic.
> >   
> 
>    Stanislaw's patch is adding the DIOx- to address hold time (t9) to 
> the existing ones. While there's has been already a patch by David Daney 
> adding this timing to libata (however, the author have ditched this idea 
> finally), the table in ide-timings.c still misses it, as well as the PIO 
> mode 6 timings...

Indeed... should be easy and quick to fix though.

>    Hm, besides the address setup and active/recovery times there seem 
> wrong for the PIO mode 5: they should be 15 and 65/25, not 20 and 50/30. 
> Bart, are you reading this? :-)

Yeah.  Where's the patch? :-)

Thanks,
Bart

Re: [RFC][PATCH] at91_ide driver

[Bartlomiej Zolnierkiewicz]

Czesc,

Just some minor nitpicks:

On Wednesday 14 January 2009, Stanislaw Gruszka wrote:

[...]

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

no strong feelings about CodingStyle but we usually do:

...
	.exec_command	= ide_exec_command,
	.read_status	= ide_read_status,
...

which helps code readability and consistency

> +static void at91_ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
> +{
> +	u8 chipselect = drive->hwif->extra_base;
> +
> +	pdbg("pio %u\n", pio);
> +
> +	if (pio > 6) {
> +		perr("can't set PIO %d mode\n", pio);
> +		return;
> +	}

no need for this check (core code guarantees that it won't happen)

> +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,
> +	.pio_mask = ATA_PIO6,
> +};

AFAICS IDE_HFLAG_NO_IO_32BIT should also be set here,
you may also want to set IDE_HFLAG_UNMASK_IRQS while at it

and CodingStyle fixup would be a nice bonus

> +#if 0
> +	/* In 12-0275-01 version of the PCB address
> +	 * lines CF_A0 and CF_A2 are swapped */
> +	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;
> +#else
> +	/* Proper lines addresses */
> +	hw.io_ports.data_addr = tf_base + 0;
> +	hw.io_ports.error_addr = tf_base + 1;
> +	hw.io_ports.nsect_addr = tf_base + 2;
> +	hw.io_ports.lbal_addr = tf_base + 3;
> +	hw.io_ports.lbam_addr = tf_base + 4;
> +	hw.io_ports.lbah_addr =	tf_base + 5;
> +	hw.io_ports.device_addr = tf_base + 6;
> +	hw.io_ports.command_addr = tf_base + 7;
> +#endif

How's about using enums for offsets, i.e.

enum {
#if 0
...
	AT91_IDE_ERROR_ADDR_OFFSET = 4,
...
#else
...
	AT91_IDE_ERROR_ADDR_OFFSET = 1,
...
#endif
};

?

otherwise (except issues already discussed in the other mail) it looks fine

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>> +#ifdef AT91_GPIO_IRQ_HACK
>>>> +#define NR_TRIES 10
>>>> +	int ntries = 0;
>>>> +	int pin_val1, pin_val2;	
>>>> +	do {
>>>> +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
>>>> +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
>>>> +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
>>>>     
>>>>         
>>> You really don't want to put special board specific code in generic
>>> locations. In the libata case you don't need to and I think in the ide
>>> case you can avoid it too by wrapping the IRQ handler.
>>>       
>>    Unfortunately, it seems you can't wrap ide_intr(), at least with the 
>> current code.
>>     
>
> Well... there shouldn't be much problem with:
>
> * adding ->irq_handler method to struct ide_port_info and struct ide_host
>
>   [ which reminds me that struct ide_port_info would be better named struct
>     ide_host_info and IIRC somebody has already noticed it in the past ;-) ]
>
> * exporting ide_intr()
>
> * adding ide_interrupt() wrapper around ide_intr() which will do sth like:
>
> 	if (host->irq_handler)
> 		return host->irq_handler()
> 	else
> 		return ide_intr()
>
>   and then passing &ide_interrupt instead of &ide_intr to request_irq()
>
> * implementing at91_irq_handler()
>
> * Et Voila!
>
> In the longer term it would also be useful for other purposes
> (like adding ATA-like flash devices support to IDE).
>   

   Oh, you must be meaning that brain damaged Disk-On-Chip H3... but I 
don't think it would need to wrap ide_intr() as it should have its own 
"class driver" (like ide-disk).

>>> Other comments:
>>> 	- The old and new ATA layers both have timing tables and timing
>>> functions so you don't need all the duplicated timing table logic.
>>>   
>>>       
>>    Stanislaw's patch is adding the DIOx- to address hold time (t9) to 
>> the existing ones. While there's has been already a patch by David Daney 
>> adding this timing to libata (however, the author have ditched this idea 
>> finally), the table in ide-timings.c still misses it, as well as the PIO 
>> mode 6 timings...
>>     
>
> Indeed... should be easy and quick to fix though.
>   

   We need to add support for CFA's MWDMA modes 3 and 4 then as well...

>>    Hm, besides the address setup and active/recovery times there seem 
>> wrong for the PIO mode 5: they should be 15 and 65/25, not 20 and 50/30. 
>> Bart, are you reading this? :-)
>>     
>
> Yeah.  Where's the patch? :-)
>   

   I was not feeling confident because the address setup and recovery 
times defined by CF spec. are less than those we have (that were spec'ed 
by Quantum I guess?). However, Wikipedia's article about PIO tells me 
that no PIO5 capable hard disks were manufactured...

> Thanks,
> Bart

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>> the PIO transfer speeds (very significantly) though I'm not sure of the 
>> current state of affairs...
>>     
>
> 16bit only performance is identical (as you'd expect)

   Well, I don't know what to expect, having seen figures like 0.5 (or 
1.5) MB/s cited on this list...

> 32bit is now
> identical with the 32bit pio patches applied
>   

   Hm, I didn't realize 32-bit I/O really can really boost the IDE 
speed. I vaguely remember (or misremember) benchmarking drives under DOS 
(many years ago) with and without 32-bit access enabled in the BIOS 
Setup and not seeing any significant differences. Perhpas it's worth to 
re-benchmark this in Linux if I find the time...

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Alan Cox]

> (many years ago) with and without 32-bit access enabled in the BIOS 
> Setup and not seeing any significant differences. Perhpas it's worth to 
> re-benchmark this in Linux if I find the time...

On the intel chipsets I have it nearly doubles PIO speed, ditto on the
Delkin cardbus.

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>> the PIO transfer speeds (very significantly) though I'm not sure of 
>>> the current state of affairs...
>>>     
>>
>> 16bit only performance is identical (as you'd expect)
>
>   Well, I don't know what to expect, having seen figures like 0.5 (or 
> 1.5) MB/s cited on this list...

   Instead of going to bed, I tried booting FC8 (2.6.23.y) off DVD in 
rescue mode and suppressing libata's DMA use: hdparm -t topped at around 
1 MB/s from both SATA hard drive and PATA DVD drive with sata_sil driven 
the former and pata_atiixp the latter... I wasn't able to compare 
results with IDE core of course.

>> identical with the 32bit pio patches applied
>>   
> 32bit is now
>
>   Hm, I didn't realize 32-bit I/O really can really boost the IDE 
> speed. I vaguely remember (or misremember) benchmarking drives under 
> DOS (many years ago) with and without 32-bit access enabled in the 
> BIOS Setup and not seeing any significant differences. Perhpas it's 
> worth to re-benchmark this in Linux if I find the time...

   With hdparm being able to control that via its -c option when using 
IDE driver, it shouldn't take much time...

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>> the PIO transfer speeds (very significantly) though I'm not sure of 
>>>> the current state of affairs...

>>> 16bit only performance is identical (as you'd expect)

>>   Well, I don't know what to expect, having seen figures like 0.5 (or 
>> 1.5) MB/s cited on this list...

>   Instead of going to bed, I tried booting FC8 (2.6.23.y) off DVD in 
> rescue mode and suppressing libata's DMA use: hdparm -t topped at around 
> 1 MB/s from both SATA hard drive and PATA DVD drive with sata_sil driven 
> the former and pata_atiixp the latter... I wasn't able to compare 
> results with IDE core of course.

>>> identical with the 32bit pio patches applied

>> 32bit is now

>>   Hm, I didn't realize 32-bit I/O really can really boost the IDE 
>> speed. I vaguely remember (or misremember) benchmarking drives under 
>> DOS (many years ago) with and without 32-bit access enabled in the 
>> BIOS Setup and not seeing any significant differences. Perhpas it's 
>> worth to re-benchmark this in Linux if I find the time...

>   With hdparm being able to control that via its -c option when using 
> IDE driver, it shouldn't take much time...

    Here's the results I got on my work PC having 2 hard disks on 82801DB, 
still running the ancient RH9 (which included hdparm 5.5):

[root@wasted etc]# /sbin/hdparm -i /dev/hda

/dev/hda:

  Model=ST380011A, FwRev=3.06, SerialNo=5JV4FZNT
  Config={ HardSect NotMFM HdSw>15uSec Fixed DTR>10Mbs RotSpdTol>.5% }
  RawCHS=16383/16/63, TrkSize=0, SectSize=0, ECCbytes=4
  BuffType=unknown, BuffSize=2048kB, MaxMultSect=16, MultSect=16
  CurCHS=16383/16/63, CurSects=16514064, LBA=yes, LBAsects=156301488
  IORDY=on/off, tPIO={min:240,w/IORDY:120}, tDMA={min:120,rec:120}
  PIO modes:  pio0 pio1 pio2 pio3 pio4
  DMA modes:  mdma0 mdma1 mdma2
  UDMA modes: udma0 udma1 udma2 udma3 udma4 *udma5
  AdvancedPM=no WriteCache=enabled
  Drive conforms to: ATA/ATAPI-6 T13 1410D revision 2:  1 2 3 4 5 6

[root@wasted etc]# /sbin/hdparm -c /dev/hda

/dev/hda:
  IO_support   =  0 (default 16-bit)

[root@wasted etc]# /sbin/hdparm -d0 -p4 /dev/hda

/dev/hda:
  setting using_dma to 0 (off)
  using_dma    =  0 (off)

/dev/hda:
  attempting to set PIO mode to 4

[root@wasted etc]# /sbin/hdparm -t /dev/hda

/dev/hda:
  Timing buffered disk reads:  64 MB in 22.65 seconds =  2.83 MB/sec

[root@wasted etc]# /sbin/hdparm -c1 /dev/hda

/dev/hda:
  setting 32-bit IO_support flag to 1
  IO_support   =  1 (default 32-bit)

[root@wasted etc]# /sbin/hdparm -t /dev/hda

  Timing buffered disk reads:  64 MB in 18.19 seconds =  3.52 MB/sec

[root@wasted etc]# /sbin/hdparm -i /dev/hdb

/dev/hdb:

  Model=Maxtor 2F030J0, FwRev=VAM51JJ0, SerialNo=F11HZPNE
  Config={ Fixed }
  RawCHS=16383/16/63, TrkSize=0, SectSize=0, ECCbytes=57
  BuffType=DualPortCache, BuffSize=2048kB, MaxMultSect=16, MultSect=16
  CurCHS=16383/16/63, CurSects=16514064, LBA=yes, LBAsects=60056543
  IORDY=on/off, tPIO={min:120,w/IORDY:120}, tDMA={min:120,rec:120}
  PIO modes:  pio0 pio1 pio2 pio3 pio4
  DMA modes:  mdma0 mdma1 mdma2
  UDMA modes: udma0 udma1 udma2 udma3 udma4 *udma5 udma6
  AdvancedPM=yes: disabled (255) WriteCache=enabled
  Drive conforms to: (null):  1 2 3 4 5 6 7

[root@wasted etc]# /sbin/hdparm -c /dev/hdb

/dev/hdb:
  IO_support   =  0 (default 16-bit)

[root@wasted etc]# /sbin/hdparm -d0 -p4 /dev/hdb

/dev/hdb:
  setting using_dma to 0 (off)
  using_dma    =  0 (off)

/dev/hdb:
  attempting to set PIO mode to 4

[root@wasted etc]# /sbin/hdparm -t /dev/hdb

/dev/hdb:
  Timing buffered disk reads:  64 MB in 20.80 seconds =  3.08 MB/sec

[root@wasted etc]# /sbin/hdparm -c1 /dev/hdb

/dev/hdb:
  setting 32-bit IO_support flag to 1
  IO_support   =  1 (default 32-bit)

[root@wasted etc]# /sbin/hdparm -t /dev/hdb

  Timing buffered disk reads:  64 MB in 12.69 seconds =  5.04 MB/sec

    As you can see, although the speed increase can certainly be seen, it's 
not really that big for the (cheapo :-) Seagate drive while Maxtor 
demonstrated 1.6x increase. I need to try somewhat newer kernel though (don't 
want to reboot to CentOS)...

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Witam :)

On Friday 16 January 2009 17:58, Bartlomiej Zolnierkiewicz wrote:
> Well... there shouldn't be much problem with:
>
> * adding ->irq_handler method to struct ide_port_info and struct ide_host
>
>   [ which reminds me that struct ide_port_info would be better named struct
>     ide_host_info and IIRC somebody has already noticed it in the past ;-)
> ]
>
> * exporting ide_intr()
>
> * adding ide_interrupt() wrapper around ide_intr() which will do sth like:
>
> 	if (host->irq_handler)
> 		return host->irq_handler()
> 	else
> 		return ide_intr()
>
IMHO (slightly) better way is requesting irq handler directly
in init_irq() e.g: 

irq_func = host->irq_handler;
if (!irq_func)
	irq_func =  &ide_intr;
if (request_irq(hwif->irq, irq_func, sa,hwif->name,hwgroup))
                        goto out_unlink;

Cheers
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

On Friday 16 January 2009 18:43, Bartlomiej Zolnierkiewicz wrote:
> > +#if 0
> > +	/* In 12-0275-01 version of the PCB address
> > +	 * lines CF_A0 and CF_A2 are swapped */
> > +	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;
> > +#else
> > +	/* Proper lines addresses */
> > +	hw.io_ports.data_addr = tf_base + 0;
> > +	hw.io_ports.error_addr = tf_base + 1;
> > +	hw.io_ports.nsect_addr = tf_base + 2;
> > +	hw.io_ports.lbal_addr = tf_base + 3;
> > +	hw.io_ports.lbam_addr = tf_base + 4;
> > +	hw.io_ports.lbah_addr =	tf_base + 5;
> > +	hw.io_ports.device_addr = tf_base + 6;
> > +	hw.io_ports.command_addr = tf_base + 7;
> > +#endif
>
> How's about using enums for offsets, i.e.
>
> enum {
> #if 0
> ...
> 	AT91_IDE_ERROR_ADDR_OFFSET = 4,
> ...
> #else
> ...
> 	AT91_IDE_ERROR_ADDR_OFFSET = 1,
> ...
> #endif
> };
Oh, I remove this code. It is just workaround for stupid
hardware bug.
 

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

On Friday 16 January 2009 16:34, Sergei Shtylyov wrote:
> > I use t9 for calculating memory controller settings, but maybe it is
> > unneeded, pata_at91 don't use this value.
>
>     You mean pata_at32? It might have been that this detail was missed...
Yes at32 of course. Now I'm trying to figure out difference between settings
in at32 and my code. They differ, so one driver (I suspect my) is probably 
wrong.

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Bartlomiej Zolnierkiewicz]

On Monday 19 January 2009, Stanislaw Gruszka wrote:
> Witam :)
> 
> On Friday 16 January 2009 17:58, Bartlomiej Zolnierkiewicz wrote:
> > Well... there shouldn't be much problem with:
> >
> > * adding ->irq_handler method to struct ide_port_info and struct ide_host
> >
> >   [ which reminds me that struct ide_port_info would be better named struct
> >     ide_host_info and IIRC somebody has already noticed it in the past ;-)
> > ]
> >
> > * exporting ide_intr()
> >
> > * adding ide_interrupt() wrapper around ide_intr() which will do sth like:
> >
> > 	if (host->irq_handler)
> > 		return host->irq_handler()
> > 	else
> > 		return ide_intr()
> >
> IMHO (slightly) better way is requesting irq handler directly
> in init_irq() e.g: 
> 
> irq_func = host->irq_handler;
> if (!irq_func)
> 	irq_func =  &ide_intr;
> if (request_irq(hwif->irq, irq_func, sa,hwif->name,hwgroup))
>                         goto out_unlink;

Agreed.

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

>>>I use t9 for calculating memory controller settings, but maybe it is
>>>unneeded, pata_at91 don't use this value.

>>    You mean pata_at32? It might have been that this detail was missed...

> Yes at32 of course. Now I'm trying to figure out difference between settings
> in at32 and my code. They differ, so one driver (I suspect my) is probably 
> wrong.

    I'm going to post the deatiled review af your patch RSN. I've even got so 
far as to reading Atmel's dataheets (well, it wasn't for the first time as I 
had some experience with their AVR8 mictocotrollers :-).

> Stanislaw Gruszka

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>>> +#ifdef AT91_GPIO_IRQ_HACK
>>>>> +#define NR_TRIES 10
>>>>> +    int ntries = 0;
>>>>> +    int pin_val1, pin_val2;   
>>>>> +    do {
>>>>> +        pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
>>>>> +        pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
>>>>> +    } while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
>>>>>             
>>>> You really don't want to put special board specific code in generic
>>>> locations. In the libata case you don't need to and I think in the ide
>>>> case you can avoid it too by wrapping the IRQ handler.
>>>>       
>>>    Unfortunately, it seems you can't wrap ide_intr(), at least with 
>>> the current code.
>>>     
>>
>> Well... there shouldn't be much problem with:
>>
>> * adding ->irq_handler method to struct ide_port_info and struct 
>> ide_host
>>
>>   [ which reminds me that struct ide_port_info would be better named 
>> struct
>>     ide_host_info and IIRC somebody has already noticed it in the 
>> past ;-) ]
>>
>> * exporting ide_intr()
>>
>> * adding ide_interrupt() wrapper around ide_intr() which will do sth 
>> like:
>>
>>     if (host->irq_handler)
>>         return host->irq_handler()
>>     else
>>         return ide_intr()
>>
>>   and then passing &ide_interrupt instead of &ide_intr to request_irq()
>>
>> * implementing at91_irq_handler()
>>
>> * Et Voila!
>>
>> In the longer term it would also be useful for other purposes
>> (like adding ATA-like flash devices support to IDE).
>>   
>
>   Oh, you must be meaning that brain damaged Disk-On-Chip H3... but I 
> don't think it would need to wrap ide_intr() as it should have its own 
> "class driver" (like ide-disk).
>
>>>> Other comments:
>>>>     - The old and new ATA layers both have timing tables and timing
>>>> functions so you don't need all the duplicated timing table logic.
>>>>         
>>>    Stanislaw's patch is adding the DIOx- to address hold time (t9) 
>>> to the existing ones. While there's has been already a patch by 
>>> David Daney adding this timing to libata (however, the author have 
>>> ditched this idea finally), the table in ide-timings.c still misses 
>>> it, as well as the PIO mode 6 timings...
>>>     
>>
>> Indeed... should be easy and quick to fix though.
>>   
>
>   We need to add support for CFA's MWDMA modes 3 and 4 then as well...
>
>>>    Hm, besides the address setup and active/recovery times there 
>>> seem wrong for the PIO mode 5: they should be 15 and 65/25, not 20 
>>> and 50/30. Bart, are you reading this? :-)
>>>     
>>
>> Yeah.  Where's the patch? :-)
>>   
>
>   I was not feeling confident because the address setup and recovery 
> times defined by CF spec. are less than those we have (that were 
> spec'ed by Quantum I guess?). However, Wikipedia's article about PIO 
> tells me that no PIO5 capable hard disks were manufactured...

   I've already started the patch adding support of the CFA modes... I'm 
still not sure how/whether to keep the non-standard PIO5 mode support. 
Probalbly I'll use the maximum timings: 20/65/30 (although with typical 
30 ns cycle time of the PCI chips, there shouldn't be much difference).

>> Bart
> Thanks,

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

> I'm writting IDE host driver for AT91 Static Memory Controller with Compact Flash
> True IDE logic. My company - Kelvatek wants to add driver into the mainline,
> we think it can be usesfull for others and including driver will be also beneficial
> for us obviously.  

   If everybody looked at it that way...

> Some issues with driver:
>
> * Why not platform driver  

   But it is a platform driver. :-)

> Generic pata/ide_platform driver will not work with AT91 as we need to 
>   

  Ah, why not generic...

> do special things before access Task File and Data Register (functions 
> set_8bit_mode() and set_16bit_mode()).

   That part really doesn't look well -- Atmel should have put more 
though into it and make the register stride 2 bytes, so that 16-bit 
access could alwys be used...

> * Tests
> We tested with our custom board which is slightly diffrent than
> Atmel Evaluation Kit. Pins settings in the patch are from Kelvatek's board,
> to run driver on sam9263ek board settings shall be changed.

  I then don't understand how are you submitting this as a part of 
SAM9623EK board code...

> We did not tests on Atmel sam9263ek board, because we have no proper connector
> for 1,8'' HDD.

   Not sure I've understood that. You have 1,8'' drive and the board has 
stndard IDE connector?

> * GPIO interrupt
> This is the most problem with the driver/hardware. Interrupt from device is
> delivered through GPIO. In AT91 GPIO interrupts are triggered on falling
> and raising edge of signal (this behaviour is not configured IIRC). Whereas
>   

   Ugh... what were they thinking?

> IDE device request interrupt on high level (raising edge in our case). This
>   

   The IDE interrupts historically trigger on rising edge.

> mean we have fake interrupts requests on cpu, which broke IDE layer. 
> Problem can be solved in hardware: when device INTRQ line is connected
> to CPU through AIC controlled IRQ0 or IRQ1 lines with proper level configured.
> We probably do such things in our board, but it's is rather impossible
> for Atmel Evaluation Kit. Also there is workaround in software: check interrupt
> pin value and exit instantly from ISR when line is on low level. Such thing
> is done in the patch (AT91_GPIO_IRQ_HACK in drivers/ide/ide-io.c), but
> this is dirty hack and should implemented differently (I hope someone
> could give my good idea how).
>   

   We need to teach IDE core that IRQ handler can be different from the 
default one.

> * Performance
> Driver works only in PIO mode, we have no hardware for DMA mode. 
> Probably  hardware support could be done using another Chip Select
> with Static Memory Controller and extra external logic. Maybe Atmel
> people could tell someting more about this issue?

   I wouldn't hope on that... :-)

> On PIO4 mode I achieve 4,5MB/s

   Seems a good result actually. How it was measured?

> Regards
> Stanislaw Gruszka
>
> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
>
> ---
>  arch/arm/mach-at91/at91sam9263_devices.c |   96 +++++++
>  arch/arm/mach-at91/board-sam9263ek.c     |   11 +
>  arch/arm/mach-at91/include/mach/board.h  |    9 +
>   

   This should probably go thru the ARM tree... though the maintainer 
will decide.

> diff --git a/arch/arm/mach-at91/at91sam9263_devices.c b/arch/arm/mach-at91/at91sam9263_devices.c
> index 8b88408..976e228 100644
> --- a/arch/arm/mach-at91/at91sam9263_devices.c
> +++ b/arch/arm/mach-at91/at91sam9263_devices.c
> @@ -347,6 +347,102 @@ void __init at91_add_device_mmc(short mmc_id, struct at91_mmc_data *data)
>  void __init at91_add_device_mmc(short mmc_id, struct at91_mmc_data *data) {}
>  #endif
>  
> +/* --------------------------------------------------------------------
> + *  IDE
> + * -------------------------------------------------------------------- */
> +
> +#if defined(CONFIG_BLK_DEV_IDE_AT91) || defined(CONFIG_BLK_DEV_IDE_AT91_MODULE)
>   

   Welcome to the ARM #ifdef hell. :-)

> +/* Proper CS address space will be added */
> +#define AT91_IDE_TASK_FILE	0x00c00000
> +#define AT91_IDE_CTRL_REG	0x00e00000
>   

   Er, are you sure? Address lines should be 110 to address the device 
control and alternate status registers, do they get asserted properly?

> +static struct resource ide_resources[] = {
> +	[0] = {
> +		.start	= AT91_IDE_TASK_FILE,
> +		.end	= AT91_IDE_TASK_FILE + 8 - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start	= AT91_IDE_CTRL_REG,
> +		.end	= AT91_IDE_CTRL_REG, /* 1 byte */
> +		.flags	= IORESOURCE_MEM,
> +	},
>   

   Er, why no IRQ resource?

> diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> new file mode 100644
> index 0000000..f82ecb9
> --- /dev/null
> +++ b/drivers/ide/at91_ide.c
> @@ -0,0 +1,459 @@
> +/*
> + * IDE host driver for AT91 Static Memory Controler
>   

  I'm afraid you're being too generic here: e.g. AT91RM9200 has 
incompatible SMC.

> +#define DEBUG 1
> +#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
>   

   Consider using dev_err() and dev_dbg() instead...

> +/* 
> + * 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
> + */
> +
> +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_(2)); 

   I'm not sure why you're fixing the data float timing to 2 cycles -- 
it correponds to the t6z timing which is 30 ns for ATA modes and 20 ns 
for CF specific ones...
   Why are you not using optimized data float mode?

> +static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
> +{
> +	u64 tmp = ns;  

   No empty line after declaration.

> +	tmp *= mck_hz;
> +	tmp += 1000*1000*1000 + 1; /* round up */

    You probably mean -1 if you intend to just round up (so tmp += 
999999999).

> +static void set_ebi_timings(const u8 chipselect, const u8 pio)
> +{
> +	/* Compact Flash True IDE mode timings, all values in nano seconds,
> +	 * see table 22 of standard 4.1 */
> +	const struct { unsigned int t0, t1, t2, t2i, t9; } timings[7] = {
> +		{ .t0 = 600, .t1 = 70, .t2 = 290, .t2i =  0, .t9 = 20 }, /* PIO 0 */
> +		{ .t0 = 383, .t1 = 50, .t2 = 290, .t2i =  0, .t9 = 15 }, /* PIO 1 */
> +		{ .t0 = 240, .t1 = 30, .t2 = 290, .t2i =  0, .t9 = 10 }, /* PIO 2 */
> +		{ .t0 = 180, .t1 = 30, .t2 =  80, .t2i = 70, .t9 = 10 }, /* PIO 3 */
> +		{ .t0 = 120, .t1 = 25, .t2 =  70, .t2i = 25, .t9 = 10 }, /* PIO 4 */
> +		{ .t0 = 100, .t1 = 15, .t2 =  65, .t2i = 25, .t9 = 10 }, /* PIO 5 */
> +		{ .t0 =  80, .t1 = 10, .t2 =  55, .t2i = 20, .t9 = 10 }, /* PIO 6 */
> +	};

   Well, you've already received comments on this one...
   I've already started the patch adding support of the CFA modes.

> +	unsigned int t0, t1, t2, t2i, t9;
> +	unsigned int mck_hz;
> +	struct clk *mck;
> +	unsigned long mode;
> +
> +	BUG_ON(pio > 6);
> +
> +	t0 = timings[pio].t0;
> +	t1 = timings[pio].t1;
> +	t2 = timings[pio].t2;
> +	t2i = timings[pio].t2i;
> +	t9 = timings[pio].t9;
> +
> +	if (t2i > 0) {
> +		/* t1 is the part of t2i, let's t9 be the rest of t2i */
> +		WARN_ON(t2i < t1); 

   Doesn't happen.

> +		if (t9 < t2i - t1)
> +			t9 = t2i - t1;
>   

   It  more sense to calculate such things *after* quantizing the 
timings with calc_mck_cycles()...

> +	mck = clk_get(NULL, "mck");
> +	BUG_ON(IS_ERR(mck));
> +	mck_hz = clk_get_rate(mck);
>   

   Why not call clk_get[_rate]() only once, at startup? It can change 
dynamically?

> +	t0 = calc_mck_cycles(t0, mck_hz);
> +	t1 = calc_mck_cycles(t1, mck_hz);
> +	t2 = calc_mck_cycles(t2, mck_hz);
> +	t2i = calc_mck_cycles(t2i, mck_hz);
>   

   If you're not using t2i, why quantize it?

> +	t9 = calc_mck_cycles(t9, mck_hz);
> +	pdbg("t0=%u t1=%u t2=%u t2i=%u t9=%u\n", t0, t1, t2, t2i, t9);
>   

   Besides, we have ide_timing_compute() doing the same thing.

> +	/* values are rounded up so we need to assure cycle is larger than pulse */
> +	if (t0 < t1 + t2 + t9)
> +		t0 = t1 + t2 + t9;
> +
> +	/* setup calculated timings */
> +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
> +						   AT91_SMC_NCS_WRSETUP_(0) |
> +						   AT91_SMC_NRDSETUP_(t1) |
> +						   AT91_SMC_NCS_RDSETUP_(0));
> +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
> +						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
>   

  With zero address to CS setup time it's the same as t0.

> +						   AT91_SMC_NRDPULSE_(t2) |
> +						   AT91_SMC_NCS_RDPULSE_(t1 + t2 + t9));
>   

   Same here.

> +	/* disable or enable waiting for IORDY signal */
> +	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> +	mode &= ~AT91_SMC_EXNWMODE;
> +	if (pio <= 4)
>   

   The IORDY loigic is not as simple -- you'd better use 
ata_id_has_iordy() for PIO modes < 5.

> +		mode |= AT91_SMC_EXNWMODE_FROZEN;
>   

   No, it should be READY mode.

> +	else 
> +		mode |= AT91_SMC_EXNWMODE_DISABLE;
>   

   This constant is 0, so else branch can be skipped.

> +static void at91_ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
> +{
> +	u8 chipselect = drive->hwif->extra_base;
> +
> +	pdbg("pio %u\n", pio);
> +
> +	if (pio > 6) {
>   

   Can't happen.

> +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,
> +	.pio_mask = ATA_PIO6,
>   

   No support for mode 6 yet, working on it.

> +static int __init at91_ide_probe(struct platform_device *pdev)
> +{
> +	int ret = -ENOMEM;
> +	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;
> +
> +	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");
> +		goto err;
>   

   -ENODEV fits here better than -ENOMEM.
   You also need to call request_mem_region()

> +#if 0
>   

   This is ghenerall frowned on...

> +	/* In 12-0275-01 version of the PCB address
> +	 * lines CF_A0 and CF_A2 are swapped */
>   

   You should try to communicate such info to the driver via the 
platfrom data...

> +#else
> +	/* Proper lines addresses */
> +	hw.io_ports.data_addr = tf_base + 0;
> +	hw.io_ports.error_addr = tf_base + 1;
> +	hw.io_ports.nsect_addr = tf_base + 2;
> +	hw.io_ports.lbal_addr = tf_base + 3;
> +	hw.io_ports.lbam_addr = tf_base + 4;
> +	hw.io_ports.lbah_addr =	tf_base + 5;
> +	hw.io_ports.device_addr = tf_base + 6;
> +	hw.io_ports.command_addr = tf_base + 7;

   I suggest using hw.io_ports_array instead.

> +	hw.irq = board->irq_pin;
>   

   Why not pass it normally, via the platform device's resource?

> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index cc35d6d..6dba1fc 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -1354,7 +1354,12 @@ static void unexpected_intr (int irq, ide_hwgroup_t *hwgroup)
>   *	request completed. At this point we issue the next request
>   *	on the hwgroup and the process begins again.
>   */
> - 
> +#define AT91_GPIO_IRQ_HACK
> +
> +#ifdef AT91_GPIO_IRQ_HACK
> +#include <mach/gpio.h>
> +#endif 
> +
>  irqreturn_t ide_intr (int irq, void *dev_id)
>  {
>  	unsigned long flags;
> @@ -1364,6 +1369,21 @@ irqreturn_t ide_intr (int irq, void *dev_id)
>  	ide_handler_t *handler;
>  	ide_startstop_t startstop;
>  
> +#ifdef AT91_GPIO_IRQ_HACK
> +#define NR_TRIES 10
> +	int ntries = 0;
> +	int pin_val1, pin_val2;	
> +	do {
> +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
>   

   You need manual pin debouncing even after calling 
at91_set_deglitch()? :-O

> +	udelay(20); // XXX: this need to be here otherwise IDE layer losts interrups, don't know why !!!
>   

   Ugh...

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Tuesday 20 January 2009 12:07:17 Sergei Shtylyov napisał(a):
> > do special things before access Task File and Data Register (functions 
> > set_8bit_mode() and set_16bit_mode()).
> 
>    That part really doesn't look well -- Atmel should have put more 
> though into it and make the register stride 2 bytes, so that 16-bit 
> access could alwys be used...
Actually it is possible (I realize when seeing pata_at32) when address
lines are connected in diffrent way, h/w trick is described in AVR32 ATA
application note. Only register addresses are multiplied by 2 then.
However Atmel reference documents for AT91 don't describe such
connection. I suppose AT91 evaluation boards are designed without the trick.
 
>   I then don't understand how are you submitting this as a part of 
> SAM9623EK board code...
I asked for comment not for submition (yet) :)

> > We did not tests on Atmel sam9263ek board, because we have no proper connector
> > for 1,8'' HDD.
> 
>    Not sure I've understood that. You have 1,8'' drive and the board has 
> stndard IDE connector?
Board has 44-pin male IDE connector (rather non standard one), we have no cable
or any device to connect. Kelvatek's board has Compact Flash connector, we use CF
card for testing. We currently ordered proper cable and IDE->CF adapter, so
I'll do tests on sam9263ek.

> > On PIO4 mode I achieve 4,5MB/s
> 
>    Seems a good result actually. How it was measured?
hdparm -t /dev/hda

I will comment other things soon.

Cheers
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Stanislaw Gruszka wrote:

>>>do special things before access Task File and Data Register (functions 
>>>set_8bit_mode() and set_16bit_mode()).

>>   That part really doesn't look well -- Atmel should have put more 
>>though into it and make the register stride 2 bytes, so that 16-bit 
>>access could alwys be used...

> Actually it is possible (I realize when seeing pata_at32) when address
> lines are connected in diffrent way, h/w trick is described in AVR32 ATA
> application note. Only register addresses are multiplied by 2 then.
> However Atmel reference documents for AT91 don't describe such
> connection. I suppose AT91 evaluation boards are designed without the trick.

    It's not much of a trick, just a usual way of connection a 16-bit 
device... I'd advise that you pass the register stride info via the platform 
data, alike to what pata_platform devices do.

>>>We did not tests on Atmel sam9263ek board, because we have no proper connector
>>>for 1,8'' HDD.

>>   Not sure I've understood that. You have 1,8'' drive and the board has 
>>stndard IDE connector?
> 
> Board has 44-pin male IDE connector (rather non standard one), we have no cable

    I think it's pretty standard laptop connector.

> or any device to connect. Kelvatek's board has Compact Flash connector, we use CF
> card for testing. We currently ordered proper cable and IDE->CF adapter, so
> I'll do tests on sam9263ek.

    Besides, not all CF connectors are DMA compatible (don't have pin 43/44 
wired for DMA).

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Tuesday 20 January 2009 12:07:17 Sergei Shtylyov napisał(a):
>    Besides, we have ide_timing_compute() doing the same thing.
I'm trying use it, but have too bigger results - 1 or 2 cycles is added.
I'm doing something wrong.

        T = 1000000000 / (mck_hz / 1000);
        pdbg("pio %u T %u\n", pio, T);
        ret = ide_timing_compute(drive, pio + XFER_PIO_0, &timing, T, 1);

        t0 = timing->cyc8b;
        t1 = timing->setup;
        t2 = timing->act8b;
        t2i = timing->recover;
        t9 = 0;
        pdbg("1: t0=%02u t1=%02u t2=%02u t2i=%02u t9=%02u\n", t0, t1, t2, t2i, t9);

Clock is approx 100MHz , results should be the same as in standard 
divided by 10.

hda: host max PIO5 wanted PIO0 selected PIO0
at91_ide_set_pio_mode pio 0 T 10004
set_ebi_timings 1: t0=60 t1=07 t2=32 t2i=29 t9=00
set_ebi_timings 2: t0=60 t1=07 t2=29 t2i=00 t9=02

hda: host max PIO5 wanted PIO1 selected PIO1
at91_ide_set_pio_mode pio 1 T 10004
set_ebi_timings 1: t0=39 t1=05 t2=29 t2i=18 t9=00
set_ebi_timings 2: t0=39 t1=05 t2=29 t2i=00 t9=02

hda: host max PIO5 wanted PIO2 selected PIO2
at91_ide_set_pio_mode pio 2 T 10004
set_ebi_timings 1: t0=33 t1=03 t2=29 t2i=12 t9=00
set_ebi_timings 2: t0=24 t1=03 t2=29 t2i=00 t9=01

hda: host max PIO5 wanted PIO3 selected PIO3
at91_ide_set_pio_mode pio 3 T 10004
set_ebi_timings 1: t0=18 t1=03 t2=09 t2i=09 t9=00
set_ebi_timings 2: t0=18 t1=03 t2=08 t2i=07 t9=04

hda: host max PIO5 wanted PIO4 selected PIO4
at91_ide_set_pio_mode pio 4 T 10004
set_ebi_timings 1: t0=12 t1=03 t2=08 t2i=04 t9=00
set_ebi_timings 2: t0=12 t1=03 t2=07 t2i=03 t9=01

hda: host max PIO5 wanted PIO5 selected PIO5
at91_ide_set_pio_mode pio 5 T 10004
set_ebi_timings 1: t0=12 t1=02 t2=07 t2i=05 t9=00
set_ebi_timings 2: t0=10 t1=02 t2=07 t2i=03 t9=01
 
Cheers
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

>>    Besides, we have ide_timing_compute() doing the same thing.
>>     
> I'm trying use it, but have too bigger results - 1 or 2 cycles is added.
>   

   That's most probably because ide_timing_compute() assumes non-zero 
minimum recovery time for PIO modes 0 to 2 (libata does the same) -- 
that actually smells of over-caution. It then tries to stretch the 
active time if the sum of active and recovery times is less than cycle 
time (all quantized already).

> I'm doing something wrong.
>   

   Well, you don't compute t2i. And you use recover field returend by 
ide_timing_compute() instead of rev8b.

>         T = 1000000000 / (mck_hz / 1000);
>         pdbg("pio %u T %u\n", pio, T);
>         ret = ide_timing_compute(drive, pio + XFER_PIO_0, &timing, T, 1);
>
>         t0 = timing->cyc8b;
>         t1 = timing->setup;
>         t2 = timing->act8b;
>         t2i = timing->recover;
>   

   No, you should use rec8b.

>         t9 = 0;
>         pdbg("1: t0=%02u t1=%02u t2=%02u t2i=%02u t9=%02u\n", t0, t1, t2, t2i, t9);
>
> Clock is approx 100MHz , results should be the same as in standard 
> divided by 10.
>
> hda: host max PIO5 wanted PIO0 selected PIO0
> at91_ide_set_pio_mode pio 0 T 10004
> set_ebi_timings 1: t0=60 t1=07 t2=32 t2i=29 t9=00
> set_ebi_timings 2: t0=60 t1=07 t2=29 t2i=00 t9=02
>   

   Yeah, with the PIO0 active time 290 ns + revovery time 240 ns giving 
530 ns, the function should stretch the active time.

> hda: host max PIO5 wanted PIO1 selected PIO1
> at91_ide_set_pio_mode pio 1 T 10004
> set_ebi_timings 1: t0=39 t1=05 t2=29 t2i=18 t9=00
> set_ebi_timings 2: t0=39 t1=05 t2=29 t2i=00 t9=02
>   

   Here 290 + 100 gives 390, so no stretching occurs.

> hda: host max PIO5 wanted PIO2 selected PIO2
> at91_ide_set_pio_mode pio 2 T 10004
> set_ebi_timings 1: t0=33 t1=03 t2=29 t2i=12 t9=00
> set_ebi_timings 2: t0=24 t1=03 t2=29 t2i=00 t9=01
>   

   Here 290 + 40 gives 330, so no stretching occurs.

> hda: host max PIO5 wanted PIO3 selected PIO3
> at91_ide_set_pio_mode pio 3 T 10004
> set_ebi_timings 1: t0=18 t1=03 t2=09 t2i=09 t9=00
> set_ebi_timings 2: t0=18 t1=03 t2=08 t2i=07 t9=04
>   

   Here 80 + 70 gives 150, so the active time is stretched.

> hda: host max PIO5 wanted PIO4 selected PIO4
> at91_ide_set_pio_mode pio 4 T 10004
> set_ebi_timings 1: t0=12 t1=03 t2=08 t2i=04 t9=00
> set_ebi_timings 2: t0=12 t1=03 t2=07 t2i=03 t9=01
>   

   Here 70 + 30 gives 100, so the active time is stretched.

> hda: host max PIO5 wanted PIO5 selected PIO5
> at91_ide_set_pio_mode pio 5 T 10004
> set_ebi_timings 1: t0=12 t1=02 t2=07 t2i=05 t9=00
> set_ebi_timings 2: t0=10 t1=02 t2=07 t2i=03 t9=01
>   

   Here 50 + 30 gives 80 (the IDE core doesn't know about CF extended 
modes, so uses non-standard PIO5 timings), so the active time is 
stretched again.

> Cheers
> Stanislaw Gruszka
>   

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Thursday 22 January 2009 10:44:10 Sergei Shtylyov napisał(a):
> Hello.
> 
> Stanislaw Gruszka wrote:
> 
> >>    Besides, we have ide_timing_compute() doing the same thing.
> >>     
> > I'm trying use it, but have too bigger results - 1 or 2 cycles is added.
> >   
> 
>    That's most probably because ide_timing_compute() assumes non-zero 
> minimum recovery time for PIO modes 0 to 2 (libata does the same) -- 
> that actually smells of over-caution. It then tries to stretch the 
> active time if the sum of active and recovery times is less than cycle 
> time (all quantized already).
Hmm, why active time is stretched. IMHO it is better to stretch inactive
time (t2i), such it can be divided into setup (t1) and recovery time (t9 with t6z).
Otherwise sum of t1 + t2 + max(t9, t2i -  t1)  will be longer than cycle time t0.

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Tuesday 20 January 2009 12:07:17 Sergei Shtylyov napisał(a):
> >  arch/arm/mach-at91/at91sam9263_devices.c |   96 +++++++
> >  arch/arm/mach-at91/board-sam9263ek.c     |   11 +
> >  arch/arm/mach-at91/include/mach/board.h  |    9 +
> >   
> 
>    This should probably go thru the ARM tree... though the maintainer 
> will decide.
I will submit patches to linux-ide and ARM things separately (ARM when I 
finally test with Atmel Evaluation Kit) . Against which tree IDE patches
should be submitted, against Bart's kernel?

> > +/* Proper CS address space will be added */
> > +#define AT91_IDE_TASK_FILE	0x00c00000
> > +#define AT91_IDE_CTRL_REG	0x00e00000
> >   
> 
>    Er, are you sure? Address lines should be 110 to address the device 
> control and alternate status registers, do they get asserted properly?
Hmm, you may have right, because I can see warning 

hda: probing with STATUS(0x50) instead of ALTSTATUS(0x00)

but I don't understand this issue, I'm going to investigate.

> > + * IDE host driver for AT91 Static Memory Controler
> >   
> 
>   I'm afraid you're being too generic here: e.g. AT91RM9200 has 
> incompatible SMC.
Well, we could add #ifdef with diffrent implementation of init_smc_mode(), 
set_8bit_mode(), etc... 

> > +	at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
> > +						  AT91_SMC_WRITEMODE |
> > +						  AT91_SMC_BAT_SELECT |
> > +						  AT91_SMC_TDF_(2)); 
> 
>    I'm not sure why you're fixing the data float timing to 2 cycles -- 
> it correponds to the t6z timing which is 30 ns for ATA modes and 20 ns 
> for CF specific ones...
Right, TDF it's not needed as long as recover time is longer than t6z and such
is in our case.

> > +	tmp *= mck_hz;
> > +	tmp += 1000*1000*1000 + 1; /* round up */
> 
>     You probably mean -1 if you intend to just round up (so tmp += 
> 999999999).
Opps.
 
> > +		if (t9 < t2i - t1)
> > +			t9 = t2i - t1;
> >   
> 
>    It  more sense to calculate such things *after* quantizing the 
> timings with calc_mck_cycles()...
Why? 

> > +	mck = clk_get(NULL, "mck");
> > +	BUG_ON(IS_ERR(mck));
> > +	mck_hz = clk_get_rate(mck);
> >   
> 
>    Why not call clk_get[_rate]() only once, at startup? It can change 
> dynamically?
No. But it's like a global variable with some fancy interface. I don't like to 
have own copy global copy of mck_hz.

> > +	t2i = calc_mck_cycles(t2i, mck_hz);
> >   
> 
>    If you're not using t2i, why quantize it?
Just for debug, will be removed.
 
> > +	t9 = calc_mck_cycles(t9, mck_hz);
> > +	pdbg("t0=%u t1=%u t2=%u t2i=%u t9=%u\n", t0, t1, t2, t2i, t9);
> >   
> 
>    Besides, we have ide_timing_compute() doing the same thing.
I don't like results of ide_timing_compute(), I will use ide_timing_find_mode() and quantize
by my own. This also is needed at startup when we need to program SMC and have no
drive structure to pass to ide_timing_compute().

> 
> > +	/* values are rounded up so we need to assure cycle is larger than pulse */
> > +	if (t0 < t1 + t2 + t9)
> > +		t0 = t1 + t2 + t9;
> > +
> > +	/* setup calculated timings */
> > +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
> > +						   AT91_SMC_NCS_WRSETUP_(0) |
> > +						   AT91_SMC_NRDSETUP_(t1) |
> > +						   AT91_SMC_NCS_RDSETUP_(0));
> > +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
> > +						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
> >   
> 
>   With zero address to CS setup time it's the same as t0.
Not true for slower PIO modes. But CS pulse can be t0. Write data is driver on
NWR signal (WRITE_MODE = 1) in at91_ide,  in opposite to pata_at32 where 
there is need to non zero CS setup or recovery time.
 
> > +	/* disable or enable waiting for IORDY signal */
> > +	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> > +	mode &= ~AT91_SMC_EXNWMODE;
> > +	if (pio <= 4)
> >   
> 
>    The IORDY loigic is not as simple -- you'd better use 
> ata_id_has_iordy() for PIO modes < 5.
Hmm. Really IDE host should behave like this? Must IDENTYFY
DEVICE and then disable IORDY if device not use it. Maybe
this is simpler, host have to react on IORDY signal but device
just not assert it. 

I would like to remove this code and have allways NWAIT
READY mode to make flipping 8/16 simpler. But I don't think we 
have guarantees that PIO5 and 6 devices not assert IORDY.

> > +	else 
> > +		mode |= AT91_SMC_EXNWMODE_DISABLE;
> >   
> 
>    This constant is 0, so else branch can be skipped.
Do you think this make code more readable? Compiler optimize this.

>    Why not pass it normally, via the platform device's resource?
Irq pin is board specific. When use resource I will need in processor 
code modify resource. I think this is not necessary code. Anyway
I need to have access to board data to get chipselect number.

> > +#ifdef AT91_GPIO_IRQ_HACK
> > +#define NR_TRIES 10
> > +	int ntries = 0;
> > +	int pin_val1, pin_val2;	
> > +	do {
> > +		pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> > +		pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> > +	} while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
> >   
> 
>    You need manual pin debouncing even after calling 
> at91_set_deglitch()? :-O
I'll check. Maybe the delay is enough for us.

> MBR, Sergei

Thanks very much
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Wednesday 14 January 2009 13:58:32 Haavard Skinnemoen napisał(a):
> Stanislaw Gruszka wrote:
> > Some issues with driver:
> > 
> > * Why not platform driver
> > Generic pata/ide_platform driver will not work with AT91 as we need to 
> > do special things before access Task File and Data Register (functions 
> > set_8bit_mode() and set_16bit_mode()). Also extra things needs to be
> > done when changing PIO mode.
> 
> Why not use libata? There's already a pata_at32 driver there which
> should be usable on at91 with perhaps a few minor changes.

I will submit at91_ide for the mainline and not touch pata_at32  for reason
listed below (some of them was already mentioned in this thread):

* there are difference in h/w attend 
  - AT91 and AT32 static memory controller h/w is the same, but linux software 
    interface to programing SMC is diffrent (in this driver we need to program SMC)
  - AT91 have to switch between 8/16 bit data bus width, AT32 not
  - AT32 not need to have GPIO IRQ quirk
* libata have more overhead due to SCSI emulation
* ide maintainer is likely to accept at91_ide
* my company want to use it in production with older kernel

Cheers
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

>>>  arch/arm/mach-at91/at91sam9263_devices.c |   96 +++++++
>>>  arch/arm/mach-at91/board-sam9263ek.c     |   11 +
>>>  arch/arm/mach-at91/include/mach/board.h  |    9 +
>>>   
>>>       
>>    This should probably go thru the ARM tree... though the maintainer 
>> will decide.
>>     
> I will submit patches to linux-ide and ARM things separately (ARM when I 
> finally test with Atmel Evaluation Kit) . Against which tree IDE patches
> should be submitted, against Bart's kernel?
>
>   
>>> +/* Proper CS address space will be added */
>>> +#define AT91_IDE_TASK_FILE	0x00c00000
>>> +#define AT91_IDE_CTRL_REG	0x00e00000

   Besides, I'm not sure: these 2 address range are decoded via 1 SMC 
chip select or 2?

>>    Er, are you sure? Address lines should be 110 to address the device 
>> control and alternate status registers, do they get asserted properly?
>>     
> Hmm, you may have right, because I can see warning 
>
> hda: probing with STATUS(0x50) instead of ALTSTATUS(0x00)
>
> but I don't understand this issue, I'm going to investigate.
>   

   I think it's exactly due to the driver reading alternate status from 
0x00e00000 ISO 0x00e00006. Soft reset also won't work because of the 
wrong address.

>>> + * IDE host driver for AT91 Static Memory Controler  
>>>       
>>   I'm afraid you're being too generic here: e.g. AT91RM9200 has 
>> incompatible SMC.
>>     
> Well, we could add #ifdef with diffrent implementation of init_smc_mode(), 
> set_8bit_mode(), etc... 
>   

   No, #ifdef'ery is certainly not an option.

>>> +		if (t9 < t2i - t1)
>>> +			t9 = t2i - t1;
>>>   
>>>       
>>    It  more sense to calculate such things *after* quantizing the 
>> timings with calc_mck_cycles()...
>>     
> Why?
>   

   More precise result -- matching the clock being used.

>>> +	t9 = calc_mck_cycles(t9, mck_hz);
>>> +	pdbg("t0=%u t1=%u t2=%u t2i=%u t9=%u\n", t0, t1, t2, t2i, t9);
>>>   
>>>       
>>    Besides, we have ide_timing_compute() doing the same thing.
>>     
> I don't like results of ide_timing_compute(),

   Well, it seems worth fixing...

> I will use ide_timing_find_mode() and quantize
> by my own. This also is needed at startup when we need to program SMC and have no
> drive structure to pass to ide_timing_compute().
>   

   Looks like we need to export ide_timing_quantize() too...

>>> +	/* values are rounded up so we need to assure cycle is larger than pulse */
>>> +	if (t0 < t1 + t2 + t9)
>>> +		t0 = t1 + t2 + t9;
>>> +
>>> +	/* setup calculated timings */
>>> +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
>>> +						   AT91_SMC_NCS_WRSETUP_(0) |
>>> +						   AT91_SMC_NRDSETUP_(t1) |
>>> +						   AT91_SMC_NCS_RDSETUP_(0));
>>> +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
>>> +						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
>>>   
>>>       
>>   With zero address to CS setup time it's the same as t0.
>>     
> Not true for slower PIO modes.

   Well, you're right probably...

> But CS pulse can be t0.

   Yes, it must be no less than t0.

> Write data is driver on
> NWR signal (WRITE_MODE = 1) in at91_ide,  in opposite to pata_at32 where 
> there is need to non zero CS setup or recovery time.
>   

   You're always setting CS setup to 0. Recovery time can't be 0, you 
probably mean nCS hold time?

>>> +	/* disable or enable waiting for IORDY signal */
>>> +	mode = at91_sys_read(AT91_SMC_MODE(chipselect));
>>> +	mode &= ~AT91_SMC_EXNWMODE;
>>> +	if (pio <= 4)
>>>   
>>>       
>>    The IORDY loigic is not as simple -- you'd better use 
>> ata_id_has_iordy() for PIO modes < 5.
>>     
> Hmm. Really IDE host should behave like this?
>   

   PIO modes 0 to 2 don't require IORDY and low end CF drives don't 
support IORDY modes.

> Must IDENTYFY DEVICE and then disable IORDY if device not use it.

   Not, the host must only disable drive's IORDY if it does't have this 
signal.

> Maybe this is simpler, host have to react on IORDY signal but device
> just not assert it. 
>
> I would like to remove this code and have allways NWAIT
> READY mode to make flipping 8/16 simpler.

   I don't understand how these are connected.

> But I don't think we 
> have guarantees that PIO5 and 6 devices not assert IORDY.
>   

   They must not assert IORDY according to the spec (unless it's 
improbable case of a hard drive with PIO5).

>>> +	else 
>>> +		mode |= AT91_SMC_EXNWMODE_DISABLE;
>>>   
>>>       
>>    This constant is 0, so else branch can be skipped.
>>     
> Do you think this make code more readable? Compiler optimize this.
>   

   Up to you.

>>    Why not pass it normally, via the platform device's resource?
>>     
> Irq pin is board specific. When use resource I will need in processor 
> code modify resource.

   So what? That's pretty normal and won't take much code.

BR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>> +            t9 = t2i - t1;
>>>>         
>>>    It  more sense to calculate such things *after* quantizing the 
>>> timings with calc_mck_cycles()...
>>>     
>> Why?
>>   
>
>   More precise result -- matching the clock being used.

   However, I don't understand now why you need to stretch t9...

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>>> +            t9 = t2i - t1;
>>>>>         
>>>>    It  more sense to calculate such things *after* quantizing the 
>>>> timings with calc_mck_cycles()...
>>>>     
>>> Why?
>>>   
>>
>>   More precise result -- matching the clock being used.
>
>   However, I don't understand now why you need to stretch t9...

  Ah, I do again. t0 *should be equal* to t1 + t2 + t9 for the better 
speeds.

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Thursday 22 January 2009 13:24:18 Sergei Shtylyov napisał(a):
> Hello, I wrote:
> 
> >>>>> +            t9 = t2i - t1;
> >>>>>         
> >>>>    It  more sense to calculate such things *after* quantizing the 
> >>>> timings with calc_mck_cycles()...
> >>>>     
> >>> Why?
> >>>   
> >>
> >>   More precise result -- matching the clock being used.
> >
> >   However, I don't understand now why you need to stretch t9...
> 
>   Ah, I do again. t0 *should be equal* to t1 + t2 + t9 for the better 
> speeds.
And hold time should meet min t2i requirement, so t9 = max(t9_orig, t2i - t1).

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Thursday 22 January 2009 13:06:16 Sergei Shtylyov napisał(a):
> >>> +/* Proper CS address space will be added */
> >>> +#define AT91_IDE_TASK_FILE	0x00c00000
> >>> +#define AT91_IDE_CTRL_REG	0x00e00000
> 
>    Besides, I'm not sure: these 2 address range are decoded via 1 SMC 
> chip select or 2?

These are in one chip select address space.

> > Well, we could add #ifdef with diffrent implementation of init_smc_mode(), 
> > set_8bit_mode(), etc... 
> >   
> 
>    No, #ifdef'ery is certainly not an option.

Why? Other option is create some header files and implement and exporting these
functions from processor specific code. This add files dependencies and spread
things across sources, FWIW.

> >>> +	/* values are rounded up so we need to assure cycle is larger than pulse */
> >>> +	if (t0 < t1 + t2 + t9)
> >>> +		t0 = t1 + t2 + t9;
> >>> +
> >>> +	/* setup calculated timings */
> >>> +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
> >>> +						   AT91_SMC_NCS_WRSETUP_(0) |
> >>> +						   AT91_SMC_NRDSETUP_(t1) |
> >>> +						   AT91_SMC_NCS_RDSETUP_(0));
> >>> +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
> >>> +						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
> >>>   
> >>>       
> >>   With zero address to CS setup time it's the same as t0.
> >>     
> > Not true for slower PIO modes.
> 
>    Well, you're right probably...
> 
> > But CS pulse can be t0.
> 
>    Yes, it must be no less than t0.

Pulse time can be less than t0, cycle time no.
 
> > Write data is driver on
> > NWR signal (WRITE_MODE = 1) in at91_ide,  in opposite to pata_at32 where 
> > there is need to non zero CS setup or recovery time.
> >   
> 
>    You're always setting CS setup to 0. Recovery time can't be 0, you 
> probably mean nCS hold time?

Yes, NCS hold time of course.

 
> > Maybe this is simpler, host have to react on IORDY signal but device
> > just not assert it. 
> >
> > I would like to remove this code and have allways NWAIT
> > READY mode to make flipping 8/16 simpler.
>
>    I don't understand how these are connected.

Both set SMC MODE register. If I will not change NWAIT in MODE register,
I can write static values in set_8/16bit_mode and stop doing logical operation on it.

> >>    Why not pass it normally, via the platform device's resource?
> >>     
> > Irq pin is board specific. When use resource I will need in processor 
> > code modify resource.
> 
>    So what? That's pretty normal and won't take much code.

I see only more memory and code usage. Let's reverse question:
Why use RESOURCE_IRQ when we have irq in board data?


Cheers
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Stanislaw Gruszka wrote:

>>>>>>>+            t9 = t2i - t1;

>>>>>>   It  more sense to calculate such things *after* quantizing the 
>>>>>>timings with calc_mck_cycles()...

>>>>>Why?

>>>>  More precise result -- matching the clock being used.

>>>  However, I don't understand now why you need to stretch t9...

>>  Ah, I do again. t0 *should be equal* to t1 + t2 + t9 for the better 
>>speeds.

    Moreover, t1 + t2 + t9 must be no less that t0 minimum, otherwise it would 
be a timing violation.

> And hold time should meet min t2i requirement, so t9 = max(t9_orig, t2i - t1).

    There's no direct connection between these in ATA/CF specs.

> Stanislaw Gruszka

WBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Stanislaw Gruszka wrote:

>>>>>+/* Proper CS address space will be added */
>>>>>+#define AT91_IDE_TASK_FILE	0x00c00000
>>>>>+#define AT91_IDE_CTRL_REG	0x00e00000

>>   Besides, I'm not sure: these 2 address range are decoded via 1 SMC 
>>chip select or 2?

> These are in one chip select address space.

    Then some external circutry converts the single nCS into -CS0 and -CS1?

>>>Well, we could add #ifdef with diffrent implementation of init_smc_mode(), 
>>>set_8bit_mode(), etc... 

>>   No, #ifdef'ery is certainly not an option.

> Why?

    It's totally ugly and unacceptable way of doing things. It seems also 
totally wrong to add support for totally incompatible SMC to this driver 
(especially with #ifdef's). Another driver should be written if CF support is 
required.

> Other option is create some header files and implement and exporting these
> functions from processor specific code. This add files dependencies and spread
> things across sources, FWIW.

    I don't think it's feasible as that SMC is just too different.

>>>>>+	/* values are rounded up so we need to assure cycle is larger than pulse */
>>>>>+	if (t0 < t1 + t2 + t9)
>>>>>+		t0 = t1 + t2 + t9;
>>>>>+
>>>>>+	/* setup calculated timings */
>>>>>+	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
>>>>>+						   AT91_SMC_NCS_WRSETUP_(0) |
>>>>>+						   AT91_SMC_NRDSETUP_(t1) |
>>>>>+						   AT91_SMC_NCS_RDSETUP_(0));
>>>>>+	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
>>>>>+						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |

>>>>  With zero address to CS setup time it's the same as t0.

>>>Not true for slower PIO modes.

>>   Well, you're right probably...

     They should better be the same and in any case t1 + t2 + t9 must be no 
less than t0min.

>>>But CS pulse can be t0.

>>   Yes, it must be no less than t0.

> Pulse time can be less than t0, cycle time no.

    nCS pulse time cannot be less than t0min as by address valid ATA spec 
means both address and -CSx valid, not just address.

>>>Maybe this is simpler, host have to react on IORDY signal but device
>>>just not assert it. 

>>>I would like to remove this code and have allways NWAIT
>>>READY mode to make flipping 8/16 simpler.

>>   I don't understand how these are connected.

> Both set SMC MODE register. If I will not change NWAIT in MODE register,
> I can write static values in set_8/16bit_mode and stop doing logical operation on it.

    No, that's undesirable.

>>>>   Why not pass it normally, via the platform device's resource?

>>>Irq pin is board specific. When use resource I will need in processor 
>>>code modify resource.

>>   So what? That's pretty normal and won't take much code.

> I see only more memory and code usage. Let's reverse question:
> Why use RESOURCE_IRQ when we have irq in board data?

    Len't reverse it again: why have it in board data when the normal location 
is the resource? :-)

> Cheers
> Stanislaw Gruszka

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Thursday 22 January 2009 14:48:25 Sergei Shtylyov napisał(a):
> Stanislaw Gruszka wrote:
> 
> >>>>>+/* Proper CS address space will be added */
> >>>>>+#define AT91_IDE_TASK_FILE	0x00c00000
> >>>>>+#define AT91_IDE_CTRL_REG	0x00e00000
> 
> >>   Besides, I'm not sure: these 2 address range are decoded via 1 SMC 
> >>chip select or 2?
> 
> > These are in one chip select address space.
> 
>     Then some external circutry converts the single nCS into -CS0 and -CS1?

-CS0 -CS1 are controlled by address lines A[23:21]
0x00c00000 give -CS1=1 -CS1=0 
0x00e00000 give -CS0=0 -CS1=1 

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Thursday 22 January 2009 13:06:16 Sergei Shtylyov napisał(a):
> > Hmm, you may have right, because I can see warning 
> >
> > hda: probing with STATUS(0x50) instead of ALTSTATUS(0x00)
> >
> > but I don't understand this issue, I'm going to investigate.
> >   
> 
>    I think it's exactly due to the driver reading alternate status from 
> 0x00e00000 ISO 0x00e00006. Soft reset also won't work because of the 
> wrong address.
Right, I change the address and warning disappear.

Thanks
Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 19 January 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>>>> +#ifdef AT91_GPIO_IRQ_HACK
> >>>>> +#define NR_TRIES 10
> >>>>> +    int ntries = 0;
> >>>>> +    int pin_val1, pin_val2;   
> >>>>> +    do {
> >>>>> +        pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> >>>>> +        pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> >>>>> +    } while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
> >>>>>             
> >>>> You really don't want to put special board specific code in generic
> >>>> locations. In the libata case you don't need to and I think in the ide
> >>>> case you can avoid it too by wrapping the IRQ handler.
> >>>>       
> >>>    Unfortunately, it seems you can't wrap ide_intr(), at least with 
> >>> the current code.
> >>>     
> >>
> >> Well... there shouldn't be much problem with:
> >>
> >> * adding ->irq_handler method to struct ide_port_info and struct 
> >> ide_host
> >>
> >>   [ which reminds me that struct ide_port_info would be better named 
> >> struct
> >>     ide_host_info and IIRC somebody has already noticed it in the 
> >> past ;-) ]
> >>
> >> * exporting ide_intr()
> >>
> >> * adding ide_interrupt() wrapper around ide_intr() which will do sth 
> >> like:
> >>
> >>     if (host->irq_handler)
> >>         return host->irq_handler()
> >>     else
> >>         return ide_intr()
> >>
> >>   and then passing &ide_interrupt instead of &ide_intr to request_irq()
> >>
> >> * implementing at91_irq_handler()
> >>
> >> * Et Voila!
> >>
> >> In the longer term it would also be useful for other purposes
> >> (like adding ATA-like flash devices support to IDE).
> >>   
> >
> >   Oh, you must be meaning that brain damaged Disk-On-Chip H3... but I 
> > don't think it would need to wrap ide_intr() as it should have its own 
> > "class driver" (like ide-disk).
> >
> >>>> Other comments:
> >>>>     - The old and new ATA layers both have timing tables and timing
> >>>> functions so you don't need all the duplicated timing table logic.
> >>>>         
> >>>    Stanislaw's patch is adding the DIOx- to address hold time (t9) 
> >>> to the existing ones. While there's has been already a patch by 
> >>> David Daney adding this timing to libata (however, the author have 
> >>> ditched this idea finally), the table in ide-timings.c still misses 
> >>> it, as well as the PIO mode 6 timings...
> >>>     
> >>
> >> Indeed... should be easy and quick to fix though.
> >>   
> >
> >   We need to add support for CFA's MWDMA modes 3 and 4 then as well...
> >
> >>>    Hm, besides the address setup and active/recovery times there 
> >>> seem wrong for the PIO mode 5: they should be 15 and 65/25, not 20 
> >>> and 50/30. Bart, are you reading this? :-)
> >>>     
> >>
> >> Yeah.  Where's the patch? :-)
> >>   
> >
> >   I was not feeling confident because the address setup and recovery 
> > times defined by CF spec. are less than those we have (that were 
> > spec'ed by Quantum I guess?). However, Wikipedia's article about PIO 
> > tells me that no PIO5 capable hard disks were manufactured...

FWIW I've never seen disk or disk's ID data dump with PIO5 support
but this of course doesn't prove that they don't exist... ;)

>    I've already started the patch adding support of the CFA modes... I'm 
> still not sure how/whether to keep the non-standard PIO5 mode support. 
> Probalbly I'll use the maximum timings: 20/65/30 (although with typical 
> 30 ns cycle time of the PCI chips, there shouldn't be much difference).

CFA devices supporting PIO5 most likely also support PIO6 which IDE core
code should prefer to use first so lower (safer) PIO5 timings shouldn't
matter in reality...

Thanks,
Bart

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello, I wrote:

>>>> Well, we could add #ifdef with diffrent implementation of 
>>>> init_smc_mode(), set_8bit_mode(), etc... 

>>>   No, #ifdef'ery is certainly not an option.

>> Why?

>    It's totally ugly and unacceptable way of doing things. It seems also 
> totally wrong to add support for totally incompatible SMC to this driver 
> (especially with #ifdef's). Another driver should be written if CF 
> support is required.

>> Other option is create some header files and implement and exporting 
>> these
>> functions from processor specific code. This add files dependencies 
>> and spread
>> things across sources, FWIW.

>    I don't think it's feasible as that SMC is just too different.

    Besides, AT91RM9200 code turned out to already be registering a platform 
device called at91_cf -- driver for which I have located in drivers/pcmcia/, 
so apparently they're not interested in True IDE mode support (and have 
overgeneralized the name as well :-).

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Tuesday 27 January 2009 16:46:38 Sergei Shtylyov napisał(a):
> Hello, I wrote:
> 
> >>>> Well, we could add #ifdef with diffrent implementation of 
> >>>> init_smc_mode(), set_8bit_mode(), etc... 
> 
> >>>   No, #ifdef'ery is certainly not an option.
> 
> >> Why?
> 
> >    It's totally ugly and unacceptable way of doing things. It seems also 
> > totally wrong to add support for totally incompatible SMC to this driver 
> > (especially with #ifdef's). Another driver should be written if CF 
> > support is required.
> 
> >> Other option is create some header files and implement and exporting 
> >> these
> >> functions from processor specific code. This add files dependencies 
> >> and spread
> >> things across sources, FWIW.
> 
> >    I don't think it's feasible as that SMC is just too different.
> 
>     Besides, AT91RM9200 code turned out to already be registering a platform 
> device called at91_cf -- driver for which I have located in drivers/pcmcia/, 
> so apparently they're not interested in True IDE mode support (and have 
> overgeneralized the name as well :-).
There can be AT91RM9200 boards where is only IDE hardware instead
"full" Compact Flash support.

Atmel publish application note with description how connect HDD to AT91RM9200.
Connection is very similar as for SAM9, the major difference is using GPIO to control 
CF ~CSx signals. Here is document:

http://www.atmel.com/dyn/resources/prod_documents/doc6023.pdf

Currently people at at91.com forum did board with HDD. As driver
they use pata_platform with some hacks, which look very similar
as things in this driver, except they use polling and not need irq quirk:

http://www.at91.com/samphpbb/viewtopic.php?f=9&t=4528&p=15739

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Sergei Shtylyov]

Hello.

Stanislaw Gruszka wrote:

>>>>>>Well, we could add #ifdef with diffrent implementation of 
>>>>>>init_smc_mode(), set_8bit_mode(), etc... 

>>>>>  No, #ifdef'ery is certainly not an option.

>>>>Why?

>>>   It's totally ugly and unacceptable way of doing things. It seems also 
>>>totally wrong to add support for totally incompatible SMC to this driver 
>>>(especially with #ifdef's). Another driver should be written if CF 
>>>support is required.

>>>>Other option is create some header files and implement and exporting 
>>>>these
>>>>functions from processor specific code. This add files dependencies 
>>>>and spread
>>>>things across sources, FWIW.

>>>   I don't think it's feasible as that SMC is just too different.

>>    Besides, AT91RM9200 code turned out to already be registering a platform 
>>device called at91_cf -- driver for which I have located in drivers/pcmcia/, 
>>so apparently they're not interested in True IDE mode support (and have 
>>overgeneralized the name as well :-).

> There can be AT91RM9200 boards where is only IDE hardware instead
> "full" Compact Flash support.

> Atmel publish application note with description how connect HDD to AT91RM9200.
> Connection is very similar as for SAM9, the major difference is using GPIO to control 
> CF ~CSx signals. Here is document:

    Horror... Frankly speaking, I don't see why they had to use GPIO and not 
the same decoding scheme as on AT91SAM9xxx.

> http://www.atmel.com/dyn/resources/prod_documents/doc6023.pdf

> Currently people at at91.com forum did board with HDD. As driver
> they use pata_platform with some hacks, which look very similar
> as things in this driver, except they use polling and not need irq quirk:

> http://www.at91.com/samphpbb/viewtopic.php?f=9&t=4528&p=15739

    It's not clear why they define IRQ resource not having IRQ.

> Stanislaw Gruszka

MBR, Sergei

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Czesc

On Friday 16 January 2009 18:43, Bartlomiej Zolnierkiewicz wrote:
> > +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,
> > +	.pio_mask = ATA_PIO6,
> > +};
>
> AFAICS IDE_HFLAG_NO_IO_32BIT should also be set here,
> you may also want to set IDE_HFLAG_UNMASK_IRQS while at it
I have some doubts about things pointed out by Alan  - atomic access.  
Due to flipping 8/16 bit in this driver, all functions accessing task file 
and data register should do things atomically. If for example
tp_ops->input_data() will be braked by interrupt and any other function
accessing task file will be called very bad things can happen.
If I use IDE_HFLAG_UNMASK_IRQS IDE layer will assure atomic
access to ATA registers ?

Stanislaw Gruszka

Re: [RFC][PATCH] at91_ide driver

[Bartlomiej Zolnierkiewicz]

On Friday 30 January 2009, Stanislaw Gruszka wrote:
> Czesc
> 
> On Friday 16 January 2009 18:43, Bartlomiej Zolnierkiewicz wrote:
> > > +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,
> > > +	.pio_mask = ATA_PIO6,
> > > +};
> >
> > AFAICS IDE_HFLAG_NO_IO_32BIT should also be set here,
> > you may also want to set IDE_HFLAG_UNMASK_IRQS while at it
> I have some doubts about things pointed out by Alan  - atomic access.  
> Due to flipping 8/16 bit in this driver, all functions accessing task file 
> and data register should do things atomically. If for example
> tp_ops->input_data() will be braked by interrupt and any other function
> accessing task file will be called very bad things can happen.

All taskfile / data access is atomic, the only exception is [Alt]Status
register read if shared IRQs are used (which doesn't seem to be the case
with AT91) or if the hardware is flaky and unexpected IRQs can happen...

If 8/16-bit flipping is really an issue then AT91 IRQ handler may need
to save info about current mode on entry, switch to 8-bit (if in 16-bit
mode), restore the saved mode on exit (if it was 16-bit mode).

> If I use IDE_HFLAG_UNMASK_IRQS IDE layer will assure atomic
> access to ATA registers ?

IDE_HFLAG_UNMASK_IRQS allows other IRQs to be serviced while IDE IRQ
is being serviced so really no problem here (unless some other drivers
also mess with 8/16-bit flipping but if so then the current code needs
fixing too).

Thanks,
Bart

Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka]

Sunday 01 February 2009 18:13:17 Bartlomiej Zolnierkiewicz napisał(a):
> > I have some doubts about things pointed out by Alan  - atomic access.  
> > Due to flipping 8/16 bit in this driver, all functions accessing task file 
> > and data register should do things atomically. If for example
> > tp_ops->input_data() will be braked by interrupt and any other function
> > accessing task file will be called very bad things can happen.
> 
> All taskfile / data access is atomic, the only exception is [Alt]Status
> register read if shared IRQs are used (which doesn't seem to be the case
> with AT91) or if the hardware is flaky and unexpected IRQs can happen...
> 
> If 8/16-bit flipping is really an issue then AT91 IRQ handler may need
> to save info about current mode on entry, switch to 8-bit (if in 16-bit
> mode), restore the saved mode on exit (if it was 16-bit mode).
I think we don't need it because we don't use shared interrupts.
Moreover hardware allow to Altarnate Status register be accessed
in both 8 and 16 bit modes.

> > If I use IDE_HFLAG_UNMASK_IRQS IDE layer will assure atomic
> > access to ATA registers ?
> 
> IDE_HFLAG_UNMASK_IRQS allows other IRQs to be serviced while IDE IRQ
> is being serviced so really no problem here (unless some other drivers
> also mess with 8/16-bit flipping but if so then the current code needs
> fixing too).
We have exclusive access to Static Memory Controller registers as long
as there are no bug in board specific code. So no problem here either.

Stanislaw Gruszka