Quoting Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>:
On Friday 08 May 2009 20:16:56 João Ramos wrote:
Bartlomiej Zolnierkiewicz escreveu:
> 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...
>
Sorry, I didn't noticed your hint... Sure, I can give it a try ;-)
Maybe with a little help, but I can try. You mean, when the host driver
is registered (ide_host_register, or ide_host_add that later calls
ide_host_register), maybe in the 'ide_init_port' method (sorry, I need
some guidance here...) check if the 'set_pio_mode' method is
implemented, and after initializing each port (d->init_hwif(hwif))
default it to PIO Mode 0, calling set_pio_mode method.
ide_init_port() seems OK but I think that ide_port_init_devices()
[it is called after ide_init_port()] would be a bit safer and more
flexible (some host drivers may also require special ->init_dev
setup first) and the check for ->set_pio_mode method presence can
be done just before actually using the method, i.e.
const struct ide_port_ops *port_ops = hwif->port_ops;
if (port_ops && port_ops->set_pio_mode)
port_ops->set_pio_mode(...)
Ok. I'll will give that a try and I'll report back with a proposed
patch for it.
Is this correct? Sorry, has I stated earlier, I'm wasn't familiar with
the IDE susbsystem untill I wrote this patch; but I'm willing to
contribute in any way I can, so please, bear with me on this :-) .
Sure, nobody starts from the expert level and not all maintainers are
into "prove the maintainer wrong" elitist's idiocy. ;)
>
>>>
>>>
>>>> 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.
>
And this conversion is made by my driver code, or should I fix directly
in the ide_drive_t structure?
The latter -- ide_drive_t is the place needing fixing.
Same: I will fix that and propose a patch.
Regards,
João Ramos
Thanks,
Bart
--
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
