Re: EP93xx PIO IDE driver proposal

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

Bartlomiej Zolnierkiewicz escreveu:
> 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]
>   

Well, it was a poorly attempt to make it work after trying with 
'hwif->cur_dev' :-) .

I thought of using the way you recommended; I didn't use it because: 1 - 
didn't know which index to use; 2 - looked like an 'ugly' way to do it...

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


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

    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

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