Timing computation, values missing from table (in spec though)

Timing computation, values missing from table (in spec though)

[Matt Sealey]

A question so I can see if I can use ata_timing_compute or possibly improve it..

The current mode timings list seems to be missing a few timings we
need that cannot possibly (and don't seem to be attempted) to be
resolved from the ata_timings table in libata-core.c.

We have a bank of registers in our ATA controller where these values
need setting; is it that the timings table is missing the values (and
the same true of the one in ide-timings.c) no
other drive controller actually needed such fine timings and would
implement them as per the spec minimums by itself? We need to actually
set the specific time in ATA clocks in those
registers, and there is a register for every timing reference in the
ATA spec tables and diagrams.. specifically t4 and t9 are missing for
PIO (and tA would be derived for iordy by the current code, right? But
we also need to set tRD which seems not to be) and for DMA, several
others (tM for MWDMA) and I am really confused where the values for
UDMA are meant to be derived for the controller since none of them are
listed in the comments for struct ata_timing.

Is it a good or bad idea to expand the table to include the extra
timings, or should we just include a custom table of timings and
manage the situation ourselves? Part of my reason for wanting to use
the libata core timings is because of all the safety checks which I
don't get with a custom blob of numbers pulled from some older source
code that I have no idea how they were derived..

-- 
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

Re: Timing computation, values missing from table (in spec though)

[Sergei Shtylyov]

Hello.

On 02/09/2012 11:19 PM, Matt Sealey wrote:

> A question so I can see if I can use ata_timing_compute or possibly improve it..

> The current mode timings list seems to be missing a few timings we
> need that cannot possibly (and don't seem to be attempted) to be
> resolved from the ata_timings table in libata-core.c.

> We have a bank of registers in our ATA controller where these values
> need setting; is it that the timings table is missing the values (and
> the same true of the one in ide-timings.c) no
> other drive controller actually needed such fine timings and would
> implement them as per the spec minimums by itself?

    Yes.

> We need to actually set the specific time in ATA clocks in those
> registers, and there is a register for every timing reference in the
> ATA spec tables and diagrams.. specifically t4 and t9 are missing for

    I have controller with t9 programmable too (and t2i NOT programmable at the 
same time), so I had to use custom formula to calculate it, in order to stretch 
the t2i to meet t0 timing. :-)

> PIO (and tA would be derived for iordy by the current code, right? But
> we also need to set tRD which seems not to be)

    tRD has minimum value of 0 for all modes, hence I see no sense adding it to 
the ata_timing[]. The same about tA which is always 35 ns min (or max?). And 
wait, is tRD aven controllable by host?!

> and for DMA, several others (tM for MWDMA)

    I also needed this timing in my yet unpublished driver but decided not to 
clutter ata_timing[] -- unlike we did with tJ ('dmack_hold') used by only one 
driver and moreover, simply computable from the driver mode.

> and I am really confused where the values for
> UDMA are meant to be derived for the controller since none of them are
> listed in the comments for struct ata_timing.

    Common x86 controllers only need one timing, hence the table only has 
t2CYCTYP/2.

> Is it a good or bad idea to expand the table to include the extra
> timings, or should we just include a custom table of timings and
> manage the situation ourselves? Part of my reason for wanting to use
> the libata core timings is because of all the safety checks which I
> don't get with a custom blob of numbers pulled from some older source
> code that I have no idea how they were derived..

    Well, that depends on the demand on the new timings -- if more than one 
driver needs them, we should add them to the table. Although with more UDMA 
timings added, the table is going to become quite ugly (perhaps we should add a 
new table for just UDMA timings instead of mechanically extending 
ata_timing[]?). I myself decided not to add anything to ata_timing[] as my 
driver seemed one-off example of needing so many timings...

MBR, Sergei

Re: Timing computation, values missing from table (in spec though)

[Matt Sealey]

On Fri, Feb 10, 2012 at 10:34 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
>
> On 02/09/2012 11:19 PM, Matt Sealey wrote:
>
>> We need to actually set the specific time in ATA clocks in those
>> registers, and there is a register for every timing reference in the
>> ATA spec tables and diagrams.. specifically t4 and t9 are missing for
>
>   I have controller with t9 programmable too (and t2i NOT programmable at
> the same time), so I had to use custom formula to calculate it, in order to
> stretch the t2i to meet t0 timing. :-)

Okay.. good to know someone else looked at it.

>> PIO (and tA would be derived for iordy by the current code, right? But
>> we also need to set tRD which seems not to be)
>
>   tRD has minimum value of 0 for all modes, hence I see no sense adding it
> to the ata_timing[]. The same about tA which is always 35 ns min (or max?).
> And wait, is tRD aven controllable by host?!

Sure. It's the time between negating iordy and sampling data (t5) and depending
on perhaps some freaky cabling, buffers on the PCB between controller and
cable and drive or just lazy manual tracing out of the ATA bus lines
from the SoC
(figuring exact trace lengths on a bus meant for a bendy cable seems like an
egregious waste of time) it needs to be set just in case there IS some
inordinate
amount of time for the data to settle.

0xBASE+0x00F   TIME_PIO_RDX     "PIO timing parameter. Controls trd"

The formula in the manual is (and trd1 is the ATA controller, trd is
the ATA we meet it by setting time_pio_rdx);

trd1(max) = (–trd) + (tskew3 + tskew4)
trd1(min) = (time_pio_rdx - 0.5) × T – (tsu + thi)
(time_pio_rdx - 0.5) × T > tsu + thi + tskew3 + tskew4

tskew3 is the maximum propagation delay for iordy and data lines, and
tskew4 is the
maximum difference in propagation delay between iody and the data..
the math makes
sense and I can see why they would implement their controller this way
from a logic
design point of view.

Makes the software a pain though. The current BSP driver hardcodes it
to 1 clock (to account for
skew I assume which doesn't really exist on a system where the ATA
controller is connected
to an ATA device about an inch away on the PCB compared to a foot of
bendy cable). In the event
we need to use a bendy cable I guess this would need fixing.. my
current plan is to hardcode
it but I would definitely like it to be in the table for completeness
(because then the code is
TIME_PIO_RDX = tRDmin + skew and not TIME_PIO_RDX = skew)

>> and for DMA, several others (tM for MWDMA)
>
>   I also needed this timing in my yet unpublished driver but decided not to
> clutter ata_timing[] -- unlike we did with tJ ('dmack_hold') used by only
> one driver and moreover, simply computable from the driver mode.

Well if pata_imx gets UDMA support it needs to set TIME_JN to control
tJ and tN (theory being that it's the time between DIORW and DMACK
where the chipselect changes, to allow the bus to wait to sample DMACK
with when the correct chipselect is in place and not before or it might be
sampling dmack driven from the wrong device?)... so that's a second user..
oddly the BSP driver puts tN, tJ and tJNH in the table (which is just tN+tJ
in every instance) and uses tJNH. Wasting about 10 bytes there..

This is why I don't want to use their data, it's kind of.. overzealous in a way.

>> and I am really confused where the values for
>> UDMA are meant to be derived for the controller since none of them are
>> listed in the comments for struct ata_timing.
>
>   Common x86 controllers only need one timing, hence the table only has
> t2CYCTYP/2.

Okay..

>> Is it a good or bad idea to expand the table to include the extra
>> timings, or should we just include a custom table of timings and
>> manage the situation ourselves? Part of my reason for wanting to use
>> the libata core timings is because of all the safety checks which I
>> don't get with a custom blob of numbers pulled from some older source
>> code that I have no idea how they were derived..
>
>   Well, that depends on the demand on the new timings -- if more than one
> driver needs them, we should add them to the table. Although with more UDMA
> timings added, the table is going to become quite ugly (perhaps we should
> add a new table for just UDMA timings instead of mechanically extending
> ata_timing[]?). I myself decided not to add anything to ata_timing[] as my
> driver seemed one-off example of needing so many timings...

Well, I think then the solution is I put the timings in the driver,
and when it gets pushed
out, and when your driver gets pushed out, that's two in-kernel users
that need more
than the current solution and as such we can think about it later.

Thanks :)

-- 
Matt