Re: EP93xx PIO IDE driver proposal

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

Hello.

João Ramos wrote:

>>> Patch follows.
>>>
>>> diff -urN linux-2.6.29.orig/arch/arm/mach-ep93xx/core.c 
>>> linux-2.6.29/arch/arm/mach-ep93xx/core.c
>>> --- linux-2.6.29.orig/arch/arm/mach-ep93xx/core.c    2009-03-23 
>>> 23:12:14.000000000 +0000
>>> +++ linux-2.6.29/arch/arm/mach-ep93xx/core.c    2009-03-30 
>>> 13:52:04.000000000 +0100
>>> @@ -537,6 +537,51 @@
>>>      platform_device_register(&ep93xx_i2c_device);
>>>  }
>>>  
>>> +static struct resource ep93xx_ide_resources[] = {
>>> +    [0] = {

>>    Your code uses 4-space indentation instead of single-tab -- this 
>> style is not acceptable. I'd advise to run the patch thru 
>> scripts/checkpatch.pl to find and fix the coding style mistakes.

> The problem is mainly my mailer settings: it deforms the patch output. 
> Perhaps next time I should send the patch as an attachment?

    Well, if the IDE maintainer doesn't mind, I don't either. Just make sure 
the attachment will have the MIME type of text/plain.

>>> +/*
>>> + * IDE Interface register map default state
>>> + * (shutdown)
>>> + */
>>> +static void
>>> +ep93xx_ide_pio_clean_regs(unsigned long base)
>>> +{
>>> +    /* disable IDE interface initially, ensuring that no device is 
>>> selected */
>>> +    writel((IDECTRL_CS0N | IDECTRL_CS1N), IDE_REGISTER(IDECTRL));
>>> +
>>> +    /* clear IDE registers */
>>> +    writel(0, IDE_REGISTER(IDECFG));
>>
>>
>>    Writing 0 to this register generates an invalid cycle on IDE bus if 
>> I don't mistake (CS0/1 and DIOR/W all asserted), are you sure you want 
>> this?

> Zero (0) is the default value for the IDE Configuration Register, as 
> stated in EP93xx User's Guide (Section 27, page 11).
> Besides, CS0/1 and DIOR/W lines are controlled through IDE Control 
> Register.

    Sorry, I seem to have mixed them up...

> In that case, perhaps it would be best to have it also set to it's 
> default value, deasserting DIOR/W as well:

> writel((IDECTRL_CS0N | IDECTRL_CS1N | IDECTRL_DIORN | IDECTRL_DIOWN), 
> IDE_REGISTER(IDECTRL));

    Yes, that's what I had in mind...

>>> +/*
>>> + * EP93xx IDE PIO low-level hardware initialization routine
>>> + */
>>> +static void
>>> +ep93xx_ide_pio_init_hwif(ide_hwif_t *hwif)
>>> +{
>>> +    unsigned long base = hwif->config_data;
>>> +
>>> +    /* enforce reset state */
>>> +    ep93xx_ide_pio_clean_regs(base);
>>> +
>>> +    /* set gpio port E, G and H for IDE */
>>> +    ep93xx_ide_gpio_on_ide();
>>> +
>>> +    /*
>>> +     * configure IDE interface:
>>> +     *   - IDE Master Enable
>>> +     *   - Polled IO Operation
>>> +     *   - PIO Mode 4 (16.67 MBps)
>>> +     *   - 1 Wait State (10 ns)
>>> +     */
>>> +    writel(IDECFG_IDEEN | IDECFG_PIO | IDECFG_PIO_MODE_4 |
>>> +           ((1 << 8) & IDECFG_WST), IDE_REGISTER(IDECFG));
>>> +}
>>> +
>>> +/*
>>> + * EP93xx IDE PIO low-level byte-read procedure.
>>> + */
>>> +static u8
>>> +ep93xx_ide_pio_readb(unsigned long base, unsigned long addr)
>>> +{
>>> +    u32    reg;
>>> +
>>> +    /*
>>> +     * initial state: DIORn=1, DIOWn=1
>>> +     * write address out
>>> +     * select CS configuration
>>> +     */
>>> +    reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
>>> +          IDECTRL_DIOWN;
>>> +    writel(reg, IDE_REGISTER(IDECTRL));
>>> +
>>> +    /*
>>> +     * bring DIORn low
>>> +     */
>>> +    reg &= ~IDECTRL_DIORN;
>>> +    writel(reg, IDE_REGISTER(IDECTRL));
>>> +
>>> +    /*
>>> +     * bring DIORn high
>>> +     */
>>> +    reg |= IDECTRL_DIORN;
>>> +    writel(reg, IDE_REGISTER(IDECTRL));
>>> +
>>> +    /*
>>> +     * read IDE Data Input Register
>>> +     */
>>> +    return (readl(IDE_REGISTER(IDEDATAIN)) & 0xFF);

>>    Masking is pointless, it's achieved by truncation to 'u8'.

>>> +}

>>    I don't see what warrants minimum active/recovery time here. And I 
>> don't see how you're supporitng PIO modes 3/4 as you're not checking 
>> -IORDY.

> So, you're saying I should check for the IORDY value at the beginning 
> and end of each transfer?

    Only before deasserting -DIOx.

> What exactly do you mean by 'warrant minimum active/recovery time'? 
> Should I insert manual delays to ensure timing issues?

    Yes, since Cirrus seems to have left you with no option other than 
bit-banging to emulate I/O cycles, you'll have to use delays...

> [...]

>>> +/*
>>> + * EP93xx IDE PIO low-level word buffer-read procedure
>>> + */
>>> +static void
>>> +ep93xx_ide_pio_readsw(unsigned long base, unsigned long addr, void 
>>> *buf,
>>> +                      unsigned int len)
>>> +{
>>> +    u16    *data = (u16 *)buf;
>>> +
>>> +    for (;len;len--)

>>    Put spaces after ; please.

>>> +        *data++ = ep93xx_ide_pio_readw(base, addr);

>>    Is this little-endian only code? If not, it's not valid in the big 
>> endian mode...

> Should I instead use array-indexing code such as:

> for (; len; len--, i++)
>    data[i] = ep93xx_ide_pio_readw(base, addr);

> And the same for 'ep93xx_ide_pio_writesw()' function?

    No, it won't change anything. You should use cpu_to_le16() -- unless 
somehow ep93xx_ide_pio_readw() always returns little-endian data...

MBR, Sergei