Re: EP93xx PIO IDE driver proposal

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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