Sergei Shtylyov escreveu:
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)...
I've already seen that, as stated in my earlier mails. Now I've got the
full picture about the IDE subsystem ;-) .
};
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.
I've already done that, implementing the 'pio_set_mode' method.
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.
As discussed with Bartlomiej Zolnierkiewicz, I will use struct
ide_timing to figure out the correct timings for each PIO mode, and use
the adequate delays in my read/write functions.
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.
Now I got it ;-) . Sorry, I'm quite inexperient in the IDE subsystem
(and in kernel programming) so it took me a while to figure that out.
But I've done that and I've already tested it, and it works.
I am just finishing some other fixes and I will submit a new patch soon.
+
+/*
+ * 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).
The point is that user's may select (or not) the IDE driver.
Some patches are already in the arm-linux mailing list for this; by
default all pins are configured for GPIO function, and they remain that
way if no IDE option is selected.
If the IDE driver is selected and compiled (and running), the driver
will then care to claim the IDE pins to the IDE function, through a
machine-specific core call (such as ep93xx_ide_on_gpio).
This approach works for both systems with and without IDE driver selected.
[...]
+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...
Please check my earliest emails; i've described the behaviour of the IDE
controller.
+
+ 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...
I've also came to that conclusion (now).
Gosh, I really need to study harder my "Linux Device Drivers" book ;-) .
Best regards,
João Ramos
--
************************************************************************
João Ramos <joao.ramos@xxxxxxx>
INOV INESC Inovação - ESTG Leiria
Escola Superior de Tecnologia e Gestão de Leiria
Edíficio C1, Campus 2
Morro do Lena, Alto do Vieiro
Leiria
2411-901 Leiria
Portugal
Tel: +351244843424
Fax: +351244843424
************************************************************************
--
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
