Re: [RFC][PATCH] at91_ide driver

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi,

On Monday 19 January 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>>>> +#ifdef AT91_GPIO_IRQ_HACK
> >>>>> +#define NR_TRIES 10
> >>>>> +    int ntries = 0;
> >>>>> +    int pin_val1, pin_val2;   
> >>>>> +    do {
> >>>>> +        pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> >>>>> +        pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> >>>>> +    } while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
> >>>>>             
> >>>> You really don't want to put special board specific code in generic
> >>>> locations. In the libata case you don't need to and I think in the ide
> >>>> case you can avoid it too by wrapping the IRQ handler.
> >>>>       
> >>>    Unfortunately, it seems you can't wrap ide_intr(), at least with 
> >>> the current code.
> >>>     
> >>
> >> Well... there shouldn't be much problem with:
> >>
> >> * adding ->irq_handler method to struct ide_port_info and struct 
> >> ide_host
> >>
> >>   [ which reminds me that struct ide_port_info would be better named 
> >> struct
> >>     ide_host_info and IIRC somebody has already noticed it in the 
> >> past ;-) ]
> >>
> >> * exporting ide_intr()
> >>
> >> * adding ide_interrupt() wrapper around ide_intr() which will do sth 
> >> like:
> >>
> >>     if (host->irq_handler)
> >>         return host->irq_handler()
> >>     else
> >>         return ide_intr()
> >>
> >>   and then passing &ide_interrupt instead of &ide_intr to request_irq()
> >>
> >> * implementing at91_irq_handler()
> >>
> >> * Et Voila!
> >>
> >> In the longer term it would also be useful for other purposes
> >> (like adding ATA-like flash devices support to IDE).
> >>   
> >
> >   Oh, you must be meaning that brain damaged Disk-On-Chip H3... but I 
> > don't think it would need to wrap ide_intr() as it should have its own 
> > "class driver" (like ide-disk).
> >
> >>>> Other comments:
> >>>>     - The old and new ATA layers both have timing tables and timing
> >>>> functions so you don't need all the duplicated timing table logic.
> >>>>         
> >>>    Stanislaw's patch is adding the DIOx- to address hold time (t9) 
> >>> to the existing ones. While there's has been already a patch by 
> >>> David Daney adding this timing to libata (however, the author have 
> >>> ditched this idea finally), the table in ide-timings.c still misses 
> >>> it, as well as the PIO mode 6 timings...
> >>>     
> >>
> >> Indeed... should be easy and quick to fix though.
> >>   
> >
> >   We need to add support for CFA's MWDMA modes 3 and 4 then as well...
> >
> >>>    Hm, besides the address setup and active/recovery times there 
> >>> seem wrong for the PIO mode 5: they should be 15 and 65/25, not 20 
> >>> and 50/30. Bart, are you reading this? :-)
> >>>     
> >>
> >> Yeah.  Where's the patch? :-)
> >>   
> >
> >   I was not feeling confident because the address setup and recovery 
> > times defined by CF spec. are less than those we have (that were 
> > spec'ed by Quantum I guess?). However, Wikipedia's article about PIO 
> > tells me that no PIO5 capable hard disks were manufactured...

FWIW I've never seen disk or disk's ID data dump with PIO5 support
but this of course doesn't prove that they don't exist... ;)

>    I've already started the patch adding support of the CFA modes... I'm 
> still not sure how/whether to keep the non-standard PIO5 mode support. 
> Probalbly I'll use the maximum timings: 20/65/30 (although with typical 
> 30 ns cycle time of the PCI chips, there shouldn't be much difference).

CFA devices supporting PIO5 most likely also support PIO6 which IDE core
code should prefer to use first so lower (safer) PIO5 timings shouldn't
matter in reality...

Thanks,
Bart