Re: EP93xx PIO IDE driver proposal

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

Hi,

[...]
>
>
>> 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,
};

So you're saying I should support all PIO modes? If so, I would have to 
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...
It would be done either by a module param (defaulting to PIO Mode 4), or 
through a kernel configuration variable...

>
> [...]
>
>    Please submit the above parts separately to linux-arm-kernel, as the
> platform code doesn't belong to the driver.
>

Ok.

>
>
>> +
>> +/* Macro for checking -IORDY line state */
>> +#define    ep93xx_ide_check_iordy() ({    \
>> +    u32 _reg = readl(IDE_REGISTER(IDECTRL));    \
>> +    (_reg & IDECTRL_IORDY) ? 1 : 0;    \
>> +})
>
>    Make this an inline function, please.

Ok.

[...]

>>
>> +
>> +/*
>> + * 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 think this is the approach desired by the EP93xx maintainers, correct? 
(Ryan? Hartley?)

[...]
>
>>
>> +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.

>
> [...]
>
>> +static u16 ep93xx_ide_readw(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));
>> +}
>
>    I don't see any difference between ep93xx_ide_read[bw](), so why don't
> you use a single function?

The difference is only the return value, which casts to either u8 or u16 
type.
Perhaps there's no need for two different functions, assuming the 
high-level code will always cast down the variables to their correct 
type (u8, u16).

>
>> +static void
>> +ep93xx_ide_writeb(unsigned long base, u8 value, unsigned long addr)
>> +{
>> +    u32 reg;
>> +
>> +    reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
>> +        IDECTRL_DIOWN;
>> +    writel(reg, IDE_REGISTER(IDECTRL));
>> +    ndelay(25);
>> +
>> +    writel(value, IDE_REGISTER(IDEDATAOUT));
>
>    Hum, do you know at which moments this controller starts/stops driving
> data lines on the IDE bus? After DIOWx- assertion/deassertion?

I will look into that. I based this source code in the CPU's user guide, 
which tips a correct procedure for reading/writing in PIO mode.
But I will check that, as I already had some trouble with the user's 
guide...

>
>> +
>> +    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.

>
>> +
>> +static void
>> +ep93xx_ide_readsw(unsigned long base, unsigned long addr, void *buf,
>> +              unsigned int len)
>> +{
>> +    u16 *data = (u16 *) buf;
>> +
>> +    for (; len; len--)
>
>    IMHO, while (len--) fits better...

Ok.

>
>> +        *data++ = cpu_to_le16(ep93xx_ide_readw(base, addr));
>> +}
>> +
>
>> +
>> +/*
>> + * EP93xx IDE PIO Transport Operations
>> + */
>> +
>> +static void ep93xx_ide_exec_command(struct hwif_s *hwif, u8 cmd)
>> +{
>> +    ep93xx_ide_writeb(hwif->config_data, cmd,
>> +                  hwif->io_ports.command_addr);
>
>    It's preferrable if you do not insert unnecessary spaces in the
> multiline statements:
>
>     ep93xx_ide_writeb(hwif->config_data, cmd,
>               hwif->io_ports.command_addr);
>
>    This also looks prettier. :-)

Ok.

>
>> +static void
>> +ep93xx_ide_tf_read(ide_drive_t *drive, struct ide_taskfile *tf, u8 
>> valid)
>> +{
>> +    ide_hwif_t *hwif = drive->hwif;
>> +    struct ide_io_ports *io_ports = &hwif->io_ports;
>> +
>> +    if (valid & IDE_VALID_ERROR)
>> +        tf->error =
>> +            ep93xx_ide_readb(hwif->config_data,
>
>> +                     io_ports->feature_addr);
>
>    Again, tabs are strongly preferred over spaces (and carrying the
> statement over to the next line where it's not necessary):
>
>         tf->error = ep93xx_ide_readb(hwif->config_data,
>                          io_ports->feature_addr);

Ok.

>
>> +static int __devinit ep93xx_ide_probe(struct platform_device *pdev)
>> +{
>> +    int retval;
>> +    int    irq;
>
>    Stray tab?
>
> [...]

Will be fixed.

>
>> +    retval = ide_host_add(&ep93xx_ide_port_info, hws, &host);
>> +    if (retval) {
>> +        dev_err(&pdev->dev,
>> +            "unable to add EP93xx IDE host controller!\n");
>
>    s/add/register/, s/host controller/driver/

Ok.

[...]


>    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.
I can make the fixes about this, ensuring Ryan and Hartley will both 
agree to them.


> MBR, Sergei
>

Regards,
João


-- 
************************************************************************

    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

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