lkml.org 
[lkml]   [2011]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ATA: pata_at91.c bugfixes
    Hi Stanislaw!

    Thank you for patch review!

    > Hi Igor
    >
    > On Wed, Apr 20, 2011 at 03:44:37PM +0400, Igor Plyatov wrote:
    >> * Fix "initial_timing" structure initialisation. The "struct ata_timing" must
    >> contain 10 members, but ".dmack_hold" member was not initialised.
    >> * The AT91SAM9 microcontrollers use special coding scheme for SMC_SETUP,
    >> SMC_PULSE, SMC_CYCLE registers.
    >> Old driver operates correctly only with low master clock values, but
    >> with high master clock it incorrectly calculates SMC values and stops
    >> to operate, because SMC can be setup only to admissible ranges in special
    >> format.
    >> New code correctly calculates SMC registers values, adjusts calculated
    >> to admissible ranges, enlarges cycles if required and converts values
    >> into SMC's format.
    >> * Old driver calculates CS_SETUP and CS_PULSE cycles incorrectly
    >> because of wrong assumptions.
    >> New code calculates:
    >> CS_SETUP = SETUP/2
    > If you to this, then {RD,WR}_SETUP have to be equal SETUP + SETUP/2
    > to generate proper setup (t0) time on IDE bus. I think the best way is
    > set CS_SETUP and CS_HOLD to 0 (what your code do if SETUP and HOLD<=1),
    > but to do this you need to take care of data float (t6z)

    Why so?

    This is not clear for me. Maybe we talk about different things or I have
    wrong
    understanding of ATA timings.
    Can you please look at "Standard Read Cycle" for AT91SAM9 MCU
    http://www.kicad.ru/files/AT91SAM9G20%20bus%20timing.pdf
    , CompactFlash connection schematic
    http://www.kicad.ru/files/CF%20for%20AT91SAM9%20(True%20IDE%20mode).pdf
    and comment my thoughts?

    Here is a legend for "Standard Read Cycle" timing diagram.
    ------------------------------------------------------------------------------
    Read (NRD) signal parameters:
    * t0 = cycle = NRD_CYCLE
    * t1 = setup = NRD_SETUP
    * t2 = pulse = NRD_PULSE
    * t9 = hold = NRD_HOLD

    Chip Select (NCS) signal parameters:
    * cs_setup = NCS_RD_SETUP
    * cs_pulse = NCS_RD_PULSE
    * cs_cycle = cycle

    Notes:
    * The NCS_RD_CYCLE is equal to the NRD_CYCLE for AT91SAM9, because they
    start/finish simultaneously (HW related).
    * The NCS signal is not the same as CS1, CS2 ATA signals! It used only to
    enable data bus transceiver U2.

    So NCS parameters calculated as
    cs_setup = setup/2
    cs_pulse = pulse + setup/2 + hold/2 = (pulse + cycle)/2
    >> +static const struct ata_timing initial_timing = {
    >> + .mode = XFER_PIO_0,
    >> + .setup = 70,
    >> + .act8b = 290,
    >> + .rec8b = 240,
    >> + .cyc8b = 600,
    >> + .active = 165,
    >> + .recover = 150,
    >> + .dmack_hold = 0,
    >> + .cycle = 600,
    >> + .udma = 0
    >> +};
    > Is this really needed, why not use ata_timing_find_mode(XFER_PIO_0)?

    OK. Fixed.

    >> +static int adjust_smc_value(unsigned int *value, int *prange,
    >> + unsigned int size)
    >> +{
    >> + unsigned char i;
    >> + int remainder;
    >> +
    >> + for (i = 0; i< size; i = i + 2) {
    >> + if (*value< prange[i]) {
    >> + remainder = prange[i] - *value;
    >> + *value = prange[i]; /* nearest valid value */
    >> + return remainder;
    >> + }
    >> + else if ((prange[i]<= *value)&& (*value<= prange[i+1])) {
    >> + return 0;
    >> + }
    >> + }
    >> + *value = prange[size - 1]; /* assign maximal possible value */
    >> +
    >> + return -1; /* invalid value */
    >> +}
    > [snip]
    >> +static void calc_smc_vals(struct device *dev,
    >> + unsigned int *setup, unsigned int *cs_setup,
    >> + unsigned int *pulse, unsigned int *cs_pulse,
    >> + unsigned int *cycle)
    >> +{
    >> + int ret_val;
    >> + int cs_hold;
    >> + int range_setup[] = { /* SMC_SETUP valid ranges */
    >> + 0, 31, /* first range */
    >> + 128, 159, /* second range */
    >> + };
    >> + int range_pulse[] = { /* SMC_PULSE valid ranges */
    >> + 0, 63, /* first range */
    >> + 256, 319, /* second range */
    >> + };
    >> + int range_cycle[] = { /* SMC_CYCLE valid ranges */
    >> + 0, 127, /* first range */
    >> + 256, 383, /* second range */
    >> + 512, 639, /* third range */
    >> + 768, 895, /* fourth range */
    >> + };
    > Introducing helper structure like
    >
    > struct smc_range {
    > u16 min, max;
    > };
    >
    > would be a bit cleaner way of coding that (hint: ARRAY_SIZE could be used
    > instead of sizeof then)
    OK. Fixed.
    >> - dev_dbg(dev, "ATA timings: nrd_setup = %lu nrd_pulse = %lu nrd_cycle = %lu\n",
    >> - nrd_setup, nrd_pulse, read_cycle);
    >> - dev_dbg(dev, "ATA timings: nwe_setup = %lu nwe_pulse = %lu nwe_cycle = %lu\n",
    >> - nwe_setup, nwe_pulse, write_cycle);
    >> - dev_dbg(dev, "ATA timings: ncs_read_setup = %lu ncs_read_pulse = %lu\n",
    >> - ncs_read_setup, ncs_read_pulse);
    >> - dev_dbg(dev, "ATA timings: ncs_write_setup = %lu ncs_write_pulse = %lu\n",
    >> - ncs_write_setup, ncs_write_pulse);
    > It's worth to have some debugging prints to check if values are calculated
    > properly.

    They exists already in the calc_smc_vals() function, but now I move them
    to set_smc_timing().

    > Thanks
    > Stanislaw
    Best regards!
    --
    Igor Plyatov



    \
     
     \ /
      Last update: 2011-04-26 08:41    [W:0.031 / U:0.220 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site