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)
> +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)?
> +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)
> - 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.
Thanks
Stanislaw
--
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
