João Ramos wrote:
Sergei: I've added the delays you suggested in the read/write
procedures, accordingly to the delays specified in the processor's
user manual for PIO Mode 4.
Why only for PIO mode 4 if you're claiming support for modes 0 thru 4?
The patch currently supports only PIO Mode 4, as it is hardcoded into
the CPU's IDE configuration register:
writel(IDECFG_IDEEN | IDECFG_PIO | IDECFG_PIO_MODE_4 |
((1 << 8) & IDECFG_WST), IDE_REGISTER(IDECFG));
And also in the ide_port_info struct:
static struct ide_port_info ep93xx_ide_port_info = {
.name = MODULE_NAME,
.init_hwif = ep93xx_ide_init_hwif,
.tp_ops = &ep93xx_ide_tp_ops,
.host_flags = IDE_HFLAG_SINGLE | IDE_HFLAG_NO_DMA | IDE_HFLAG_MMIO |
IDE_HFLAG_NO_IO_32BIT | IDE_HFLAG_NO_UNMASK_IRQS,
.pio_mask = ATA_PIO4,
Note that the ATA_PIO4 mask means support for PIO modes 0 thru 4, not
just PIO mode 4 (although in the absense of the set_pio_mode() method this
mask hardly matters at all)...
};
So you're saying I should support all PIO modes? If so, I would have to
For the safe functining of your IDE driver, yes; you should support at
least PIO0 as a safe bet. Think about CompactFlash -- the older cards don't
even support PIO4, only PIO2 maximum.
make conditional code, checking perhaps a module param to sort which PIO
mode to use.
Also, the manual delays should also depend on the PIO mode selected...
Sure.
It would be done either by a module param (defaulting to PIO Mode 4), or
through a kernel configuration variable...
Why? We have set_pio_mode() method in 'struct ide_port_ops' for that.
The IDE core will select the best PIO mode for you based on the drive's
capabilities -- you just need to implement this method.
+
+/*
+ * EP93xx IDE PIO low-level hardware initialization routine
+ */
+static void ep93xx_ide_init_hwif(ide_hwif_t *hwif)
+{
+ unsigned long base = hwif->config_data;
+
+ /* enforce reset state */
+ ep93xx_ide_clean_regs(base);
+
+ /* set gpio port E, G and H for IDE */
+ ep93xx_ide_on_gpio(1);
Shouldn't this be done in the platform code instead?
The idea is to make this driver loadable, as suggested earlier by Ryan
and Hartley.
The IDE pins are initially (and by default) set to GPIO function. If the
IDE driver is registered, through specific platform code or by loading
the module at runtime, then the IDE driver cares to configure the IDE
pins for IDE function, returning them to GPIO function once the driver
is unloaded.
I'm not sure you can just "return the pins to GPIO function" since they
will remain connected to the IDE port and will affect its state even being
in GPIO mode... I think you don't have a choice here: they are either always
belong to GPIO (if there's no IDE port) or always belong to IDE (if the IDE
port is present).
[...]
+static u8 ep93xx_ide_readb(unsigned long base, unsigned long addr)
+{
+ u32 reg;
+
+ reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
+ IDECTRL_DIOWN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+ ndelay(25);
+
+ reg &= ~IDECTRL_DIORN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+ ndelay(70);
+
+ while (!ep93xx_ide_check_iordy())
+ cpu_relax();
+
+ reg |= IDECTRL_DIORN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+ ndelay(25);
+
+ return readl(IDE_REGISTER(IDEDATAIN));
Hey, how this even works (if the data doesn't get latched
somehow)?! You
should read the register right *before* the deassertion of -DIORx! The
minimum data hold time is only 5 ns and the data lines will be tristated
within 30 ns maximum...
Will be fixed.
Again, I don't know, maybe the data register is indeed latched by the
controller at the rising edge of -DIOR... because this code most probably
wouldn't work otherwise. Please check the documentation, maybe it's illegal
to read the data before the deassertion of -DIOR. But at least doing it
after 25 ns delay looked too much fishy...
+
+ reg &= ~IDECTRL_DIOWN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+ ndelay(70);
+
+ while (!ep93xx_ide_check_iordy())
+ cpu_relax();
+
+ reg |= IDECTRL_DIOWN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+ ndelay(25);
+}
+
[...]
The same question: why we need both ep93xx_ide_write[bw]()?
Same answer as before. Perhaps there's no need for those.
Since this is not a hotplug driver, you can save some memory on
making ep93xx_ide_probe() __init -- using platform_driver_probe() here
instead of platform_driver_register() and *not* initializing the
'probe' field of the 'struct platform_driver'.
I think Ryan and Hartley would like this driver to be
loadable/unloadable at runtime, as I pointed out earlier in this mail.
So what? It'll remain [un]loadable...
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
