Re: [PATCH 1/2] libata: Add three more columns to the ata_timing table.

[David Daney <ddaney@caviumnetworks.com>]

Sergei Shtylyov wrote:
> Hello.
> 
> David Daney wrote:
> 
>> The forthcoming OCTEON SOC Compact Flash driver needs a few more
>> timing values than were available in the ata_timing table.  I add new
>> columns for write_hold, read_hold, and read_holdz times.  The values
>> were obtained from the Compact Flash specification Rev 4.1.
>>
>> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
>>   
> 
>   Not quite correct...
> 
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 4214bfb..b29b7df 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2946,33 +2946,33 @@ int sata_set_spd(struct ata_link *link)
>>   */
>>  
>>  static const struct ata_timing ata_timing[] = {
>>   
> [...]
>> +/*    { XFER_PIO_SLOW, 120, 290, 240, 960, 290, 240, 30, 5, 30, 
>> 960,   0 }, */
>> +    { XFER_PIO_0,     70, 290, 240, 600, 165, 150, 30, 5, 30, 600,   
>> 0 },
>> +    { XFER_PIO_1,     50, 290,  93, 383, 125, 100, 20, 5, 30, 383,   
>> 0 },
>> +    { XFER_PIO_2,     30, 290,  40, 330, 100,  90, 15, 5, 30, 240,   
>> 0 },
>> +    { XFER_PIO_3,     30,  80,  70, 180,  80,  70, 10, 5, 30, 180,   
>> 0 },
>> +    { XFER_PIO_4,     25,  70,  25, 120,  70,  25, 10, 5, 30, 120,   
>> 0 },
>> +    { XFER_PIO_5,     15,  65,  25, 100,  65,  25,  5, 5, 20, 100,   
>> 0 },
>> +    { XFER_PIO_6,     10,  55,  20,  80,  55,  20,  5, 5, 20,  80,   
>> 0 },
>> +
>> +    { XFER_SW_DMA_0, 120,   0,   0,   0, 480, 480, 30, 5, 0,  960,   
>> 0 },
>> +    { XFER_SW_DMA_1,  90,   0,   0,   0, 240, 240, 30, 5, 0,  480,   
>> 0 },
>> +    { XFER_SW_DMA_2,  60,   0,   0,   0, 120, 120, 15, 5, 0,  240,   
>> 0 },
>> +
>> +    { XFER_MW_DMA_0,  60,   0,   0,   0, 215, 215, 20, 20, 0, 480,   
>> 0 },
>>   
> 
>   Wrong, -DIOR hold time is 5 ns for MWDMA0 as well as for all other modes.

Point taken.

> 
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 59b0f1c..7c44e45 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>>   
> [...]
>> @@ -863,6 +868,9 @@ struct ata_timing {
>>      unsigned short cyc8b;        /* t0 for 8-bit I/O */
>>      unsigned short active;        /* t2 or tD */
>>      unsigned short recover;        /* t2i or tK */
>> +    unsigned short write_hold;    /* t4 */
>> +    unsigned short read_hold;    /* t6 */
>>   
> 
>   -DIOR hold time is 5 ns for all PIO and MWDMA modes -- there's no 
> sense in storing it here.
> 

By storing it here, ata_timing_compute() will compute the bus clock 
counts for me.  Makes perfect sense to me.


>> +    unsigned short read_holdz;    /* t6z  or tj */
>>   
> 
>   T6z and Tj are not the same timing. Well, you specify it as 0 for  DMA 
> modes anyway...
> 

I had intended to overload this column for DMA, but somehow screwed it 
up.  For my next patch I plan to populate this column for DMA modes.

David Daney