Re: EP93xx PIO IDE driver proposal

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Friday 08 May 2009 19:28:22 João Ramos wrote:
> 
> >
> > Yes! :)
> >
> > There is still a room for improvement though -- it would be better to fix
> > IDE core to set PIO0 before probing devices for all host controllers.
> >
> > Moreover it seems that doing it this way would allow us to remove ->init_hwif
> > method from this driver and do all necessary setup in ep93xx_ide_probe()
> > (this controller is a single port one so theoretically there shouldn't be
> > a need for having per-port ->init_hwif implementation).
> >   
> 
> So after all this discussion ;-) , my driver will have no 'init_hwif' 
> method, and the setup code will be on 'ep93xx_ide_probe', which will 
> configure entirely the IDE host controller.
> Moreover, this initial configuration will setup the controller to work 
> at PIO Mode 0. Later on, the 'set_pio_mode' method will be called and 
> the controller will configure itself according to the PIO mode reported 
> by the IDE core.
> 
> Can I proceed this way?

Well, yes.  Though I hoped that you would at least give a try to fixing
IDE core to program PIO0 initially for all host drivers that implement
->set_pio_mode method...

> >   
> >> There's just only one issue; normally, I would setup the specific 
> >> timings (t0, t1, t2, t2i, etc) in the 'pio_set_mode' hook. However, if 
> >> you look further in the driver, those timings aren't defined through a 
> >> memory controller but instead manually enforced by 'ndelay' calls (arghhh).
> >> This means that in my low-level procedures for reading and writing, I 
> >> need to have access to the timings (or the struct ide_timing) 
> >> corresponding to the PIO mode selected, in order to use the correct delays.
> >>
> >> My question is: which is the best way to accomplish this? Declaring a 
> >> global struct ide_timing variable pointer that always holds the correct 
> >> ide_timing struct to the selected PIO mode? Or should I always check (in 
> >> some manner) what is the current PIO mode and then select the adequate 
> >> delays?
> >>     
> >
> > I think that the setting variable pointer in ->set_pio_mode method would
> > work best.  Seems like the existing drive_data field of ide_drive_t is well
> > suited for this purpose (however it may be worth to convert it to 'void *'
> > type while we are it).
> >   
> 
> Did you mean 'drive_data' field, or 'driver_data' field?
> 'drive_data' field is an unsigned int value; I guess you meant 
> 'driver_data' field as it is a (void *) field, so I can define it as a 
> pointer to the correct 'struct ide_timing'.

That is why I hinted that you may need to convert 'drive_data' to
'void *' type first.  You may also try to use 'driver_data' instead
but you will discover rather quickly that you shouldn't do this... ;)

'driver_data' is for use by IDE core and IDE device drivers.

'drive_data' is for use by IDE host drivers.

Thanks,
Bart