Re: [PATCH 10/13] sl82c105: add ->speedproc support

Re: [PATCH 10/13] sl82c105: add ->speedproc support

[Sergei Shtylyov]

Hello, I wrote:

>>> Bartlomiej Zolnierkiewicz wrote:

>>>> [PATCH] sl82c105: add ->speedproc support

>>>> * add sl82c105_tunepio() wrapper for sl82c105_tune_drive()
>>>>  (just to get the error value)

>>>> * add sl82c105_tune_chipset() (->speedproc method) for setting
>>>>  transfer mode

>>>    Thanks for the patch!

>>>  > Index: b/drivers/ide/pci/sl82c105.c

>>>> ===================================================================
>>>> --- a/drivers/ide/pci/sl82c105.c
>>>> +++ b/drivers/ide/pci/sl82c105.c

> [...]

>>>> @@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
>>>>      */
>>>>     drive->io_32bit = 1;
>>>>     drive->unmask    = 1;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
>>>> +{
>>>> +    /*
>>>> +     * TODO: find best PIO mode and set device speed here
>>>> +     *       (requires adding helper function for getting PIO cycle 
>>>> time)
>>>> +     */

>>>   I thought we were doing it by calling ide_get_best_pio_mode() above...

>> We are also using ide_get_best_pio_mode() to get PIO cycle time
>> so we can't move it here ATM.

>   I've found/used quite convenient workaround for that -- return PIO 
> mode actually selected from xxx_tune_pio(), then call 
> ide_config_drive_speed() from the real tuneproc() method.

    Works like charm with ignoring the result, since ide_get_best_pio_mode() 
returns the same explicit mode as was passed to it -- so is good for the 
speedproc() methods also...

>>>> +    (void)sl82c105_tunepio(drive, pio);

>>>    Erm, I thought afterwards that I vainly folded one into another. I 
>>> think it's worth moving those io_32bit and unmask flag assignments 
>>> above back there... May also recast my patch. :-)

>> Moving them to ->init_hwif where they belong would be even better... ;-)

>    Well, I wasn't sure where they belong... :-)
>    So, OK to recast that patch?

   Recasted it and reworked your patch atop of it (adding MWDMA 0/1 support as 
a bonus! :-) -- now need to conduct some testing on a remote target...

>>>> +static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
>>>> +{
>>>> +    mode = ide_rate_filter(drive, mode);
>>>> +
>>>> +    if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
>>>> +        return sl82c105_tunepio(drive, mode - XFER_PIO_0);
>>>> +
>>>> +    /*
>>>> +     * TODO: add MWDMA0/1 support
>>>> +     */
>>>> +    BUG_ON(mode != XFER_MW_DMA_2);

   I wonder is there are some W83C554 users anywhere -- that chipset also 
supports UltraDMA...

MBR, Sergei

Re: [PATCH 10/13] sl82c105: add ->speedproc support

[Woody Suwalski]

Sergei Shtylyov wrote:
> Hello, I wrote:
>
>>>> Bartlomiej Zolnierkiewicz wrote:
>
>>>>> [PATCH] sl82c105: add ->speedproc support
>
>>>>> * add sl82c105_tunepio() wrapper for sl82c105_tune_drive()
>>>>>  (just to get the error value)
>
>>>>> * add sl82c105_tune_chipset() (->speedproc method) for setting
>>>>>  transfer mode
>
>>>>    Thanks for the patch!
>
>>>>  > Index: b/drivers/ide/pci/sl82c105.c
>
>>>>> ===================================================================
>>>>> --- a/drivers/ide/pci/sl82c105.c
>>>>> +++ b/drivers/ide/pci/sl82c105.c
>
>> [...]
>
>>>>> @@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
>>>>>      */
>>>>>     drive->io_32bit = 1;
>>>>>     drive->unmask    = 1;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
>>>>> +{
>>>>> +    /*
>>>>> +     * TODO: find best PIO mode and set device speed here
>>>>> +     *       (requires adding helper function for getting PIO 
>>>>> cycle time)
>>>>> +     */
>
>>>>   I thought we were doing it by calling ide_get_best_pio_mode() 
>>>> above...
>
>>> We are also using ide_get_best_pio_mode() to get PIO cycle time
>>> so we can't move it here ATM.
>
>>   I've found/used quite convenient workaround for that -- return PIO 
>> mode actually selected from xxx_tune_pio(), then call 
>> ide_config_drive_speed() from the real tuneproc() method.
>
>    Works like charm with ignoring the result, since 
> ide_get_best_pio_mode() returns the same explicit mode as was passed 
> to it -- so is good for the speedproc() methods also...
>
>>>>> +    (void)sl82c105_tunepio(drive, pio);
>
>>>>    Erm, I thought afterwards that I vainly folded one into another. 
>>>> I think it's worth moving those io_32bit and unmask flag 
>>>> assignments above back there... May also recast my patch. :-)
>
>>> Moving them to ->init_hwif where they belong would be even better... 
>>> ;-)
>
>>    Well, I wasn't sure where they belong... :-)
>>    So, OK to recast that patch?
>
>   Recasted it and reworked your patch atop of it (adding MWDMA 0/1 
> support as a bonus! :-) -- now need to conduct some testing on a 
> remote target...
>
>>>>> +static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
>>>>> +{
>>>>> +    mode = ide_rate_filter(drive, mode);
>>>>> +
>>>>> +    if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
>>>>> +        return sl82c105_tunepio(drive, mode - XFER_PIO_0);
>>>>> +
>>>>> +    /*
>>>>> +     * TODO: add MWDMA0/1 support
>>>>> +     */
>>>>> +    BUG_ON(mode != XFER_MW_DMA_2);
>
>   I wonder is there are some W83C554 users anywhere -- that chipset 
> also supports UltraDMA...
>
> MBR, Sergei
Sergei,

ARM Netwinder machines are running hard disk IDE on SL82c105.
Could you send me the actual source file to try (or a patch)?

Thanks, Woody

Re: [PATCH 10/13] sl82c105: add ->speedproc support

[Russell King]

On Thu, Mar 15, 2007 at 10:22:44AM -0400, Woody Suwalski wrote:
> Sergei Shtylyov wrote:
> >  I wonder is there are some W83C554 users anywhere -- that chipset 
> >also supports UltraDMA...
> >
> >MBR, Sergei
> Sergei,
> 
> ARM Netwinder machines are running hard disk IDE on SL82c105.
> Could you send me the actual source file to try (or a patch)?

That's actually a W83C553 which does not support UDMA in any shape or
form according to the documents.

I guess it's an enhancement found on W83C554, and in that case any
attempt to add UDMA support to the SL82C105 driver should be done
such that these older chipsets continue to work.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 10/13] sl82c105: add ->speedproc support

[Sergei Shtylyov]

Hello.

Russell King wrote:

>>> I wonder is there are some W83C554 users anywhere -- that chipset 
>>>also supports UltraDMA...

>>Sergei,

>>ARM Netwinder machines are running hard disk IDE on SL82c105.
>>Could you send me the actual source file to try (or a patch)?

> That's actually a W83C553 which does not support UDMA in any shape or
> form according to the documents.

> I guess it's an enhancement found on W83C554, and in that case any
> attempt to add UDMA support to the SL82C105 driver should be done
> such that these older chipsets continue to work.

    I had no plans of adding it so far since there seemed to be no designs 
using that chip but decided to ask if somebody is interested just in case...

MBR, Sergei

Re: [PATCH 10/13] sl82c105: add ->speedproc support

[Bartlomiej Zolnierkiewicz]

On Wednesday 14 March 2007, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>> [PATCH] sl82c105: add ->speedproc support
> 
> >>>> * add sl82c105_tunepio() wrapper for sl82c105_tune_drive()
> >>>>  (just to get the error value)
> 
> >>>> * add sl82c105_tune_chipset() (->speedproc method) for setting
> >>>>  transfer mode
> 
> >>>    Thanks for the patch!
> 
> >>>  > Index: b/drivers/ide/pci/sl82c105.c
> 
> >>>> ===================================================================
> >>>> --- a/drivers/ide/pci/sl82c105.c
> >>>> +++ b/drivers/ide/pci/sl82c105.c
> 
> > [...]
> 
> >>>> @@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
> >>>>      */
> >>>>     drive->io_32bit = 1;
> >>>>     drive->unmask    = 1;
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
> >>>> +{
> >>>> +    /*
> >>>> +     * TODO: find best PIO mode and set device speed here
> >>>> +     *       (requires adding helper function for getting PIO cycle 
> >>>> time)
> >>>> +     */
> 
> >>>   I thought we were doing it by calling ide_get_best_pio_mode() above...
> 
> >> We are also using ide_get_best_pio_mode() to get PIO cycle time
> >> so we can't move it here ATM.
> 
> >   I've found/used quite convenient workaround for that -- return PIO 
> > mode actually selected from xxx_tune_pio(), then call 
> > ide_config_drive_speed() from the real tuneproc() method.
> 
>     Works like charm with ignoring the result, since ide_get_best_pio_mode() 
> returns the same explicit mode as was passed to it -- so is good for the 
> speedproc() methods also...
> 
> >>>> +    (void)sl82c105_tunepio(drive, pio);
> 
> >>>    Erm, I thought afterwards that I vainly folded one into another. I 
> >>> think it's worth moving those io_32bit and unmask flag assignments 
> >>> above back there... May also recast my patch. :-)
> 
> >> Moving them to ->init_hwif where they belong would be even better... ;-)
> 
> >    Well, I wasn't sure where they belong... :-)
> >    So, OK to recast that patch?
> 
>    Recasted it and reworked your patch atop of it (adding MWDMA 0/1 support as 
> a bonus! :-) -- now need to conduct some testing on a remote target...

Great. :)

Bart