Re: EP93xx PIO IDE driver proposal

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Tuesday 12 May 2009 18:01:53 João Ramos wrote:
> João Ramos escreveu:
> >
> >>
> >> 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?
> >
> >>  
> >>> 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).
> >>  
> 
> Are you sure I can do this safely?
> 
> Using the patch i've sent earlier, I am using the 'drive_data' field  
> (now converted to void * type) to store the struct ide_timing pointer 
> that holds the adequate timings for the selected PIO mode.
> This is working, and the fix you suggested works, but sometimes I get a 
> null pointer dereference I can't seem to figure why.
> As I needed to define low-level read/write procedures, I've defined the 
> entire ide_tp_ops structure with my own provided methods.
> For the tf_load, tf_read, input_data and output_data methods, the fix is 
> easy since I have an ide_drive_t structure pointer as a parameter, so I 
> access the timing structure using:
> 
> struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
> 
> However, for the remaining methods (exec_command, read_status, 
> read_altstatus, write_devctl and dev_select), I only have access to an 
> ide_hwif_t pointer, so in order to get access to the containing 
> ide_drive_t and then to the struct ide_timing pointer stored before, I do:
> 
> ide_drive_t *drive = (ide_drive_t *) container_of(&hwif, ide_drive_t, hwif);

This doesn't look correct.

> struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
> 
> And this seems to work, however at some point, after a while I get a 
> kernel Oops pointing out a null pointer dereference.
> 
> Can someone help me here?
> Is there a better way to retrieve the ide_drive_t pointer from the 
> ide_hwif_t structure?

hwif->devices[0] / hwif->devices[1]

However, I see the problem -- we need timing data also for command PIO.

Sergei, seems like we should just stuff pointer to command PIO timings
(which would be maximum PIO supported by both devices on the port) into
hwif->hwif_data and use it everywhere except ->*put_data methods?

Or maybe there is some better way to do it?

Thanks,
Bart