Re: EP93xx PIO IDE driver proposal

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);
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?

Best regards,
João Ramos

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

   João Ramos        <joao.ramos@xxxxxxx>

   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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux