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@xxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
