Re: EP93xx PIO IDE driver proposal

["João Ramos" <joao.ramos@inov.pt>]

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@inov.pt>

    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

************************************************************************