[PATCH v2] ide: at91_ide.c bugfix for high master clock
Igor Plyatov
plyatov at gmail.com
Wed Apr 20 07:54:31 EDT 2011
Hello Stanislaw!
> On Mon, Dec 13, 2010 at 10:35:49PM +0100, Stanislaw Gruszka wrote:
>> On Sat, Dec 11, 2010 at 11:45:26PM +0300, Igor Plyatov wrote:
>>> The AT91SAM9 microcontrollers with master clock higher then 105 MHz
>>> and PIO0, have overflow of the NCS_RD_PULSE value in the MSB. This
>>> lead to "NCS_RD_PULSE" pulse longer then "NRD_CYCLE" pulse and driver
>>> does not detect IDE device.
>> The overflow happens because MSB (bit 6) is multiplied by 256.
>> NCS pulse length = 256*NCS_RD_PULSE[6] + NCS_RD_PULSE[5:0] clock
>> cycles. So NCS_RD_PULSE 0x41 gives 257 clock cycles not 65 as we
>> expected before. Static memory controller behaviour is undefined
>> when pulse length is bigger than cycle length, so things not work.
>>
>>> + u16 ncs_rd_pulse;
>>> unsigned long mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
>>> AT91_SMC_BAT_SELECT;
>>>
>>> @@ -81,19 +84,29 @@ static void set_smc_timings(const u8 chipselect, const u16 cycle,
>>> if (data_float)
>>> mode |= AT91_SMC_TDF_(data_float);
>>>
>>> + ncs_rd_pulse = cycle;
>>> + if (ncs_rd_pulse> NCS_RD_PULSE_LIMIT) {
>>> + ncs_rd_pulse = NCS_RD_PULSE_LIMIT;
>>> + pr_warn(DRV_NAME ": ncs_rd_pulse limited to maximal value %d\n",
>>> + ncs_rd_pulse);
>>> + }
>> I'm fine with that fix. We can possibly still have problems with higher
>> frequencies, but I'm not sure if someone will use that hardware with
>> faster clocks.
> I looked at patch again and change mind, I'm not ok with it. EBI NCS in
> this case controls compact flash CS0, CS1 signals, so NCS pulse define
> a cycle on IDE bus. IDE cycle length can not be smaller than t0 (from
> PIO Mode Timing Diagram), otherwise we do not conform spec.
>
> IMHO what should be done in case of overflow is increase NCS pulse time
> (IDE cycle) and in consequence overall SMC cycle time. Below draw patch
> how this could be done. I do not even tried to compile it (give up with
> that since I do not have hardware to test anyway), just idea how things
> could be possibly done.
>
> diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> index 000a78e..277224c 100644
> --- a/drivers/ide/at91_ide.c
> +++ b/drivers/ide/at91_ide.c
> @@ -66,9 +66,8 @@
>
> #define leave_16bit(cs, mode) at91_sys_write(AT91_SMC_MODE(cs), mode);
>
> -static void set_smc_timings(const u8 chipselect, const u16 cycle,
> - const u16 setup, const u16 pulse,
> - const u16 data_float, int use_iordy)
> +static void set_smc_timings(const u8 chipselect, u16 cycle, u16 cs_cycle,
> + u16 setup, u16 pulse, u16 data_float, int use_iordy)
> {
> unsigned long mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
> AT91_SMC_BAT_SELECT;
> @@ -89,9 +88,9 @@ static void set_smc_timings(const u8 chipselect, const u16 cycle,
> AT91_SMC_NRDSETUP_(setup) |
> AT91_SMC_NCS_RDSETUP_(0));
> at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
> - AT91_SMC_NCS_WRPULSE_(cycle) |
> + AT91_SMC_NCS_WRPULSE_(cs_cycle) |
> AT91_SMC_NRDPULSE_(pulse) |
> - AT91_SMC_NCS_RDPULSE_(cycle));
> + AT91_SMC_NCS_RDPULSE_(cs_cycle));
> at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
> AT91_SMC_NRDCYCLE_(cycle));
> }
> @@ -106,11 +105,44 @@ static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
> return (unsigned int) tmp;
> }
>
> +/*
> + * In SMC registers values are coded in form:
> + *
> + * mul * ((val& (limit + 1)) ? 1 : 0) + (val& limit)
> + *
> + * where limit can be 0x1f, 0x3f or 0x7f, and mul can be 128 or 256
> + */
> +static int code_smc_values(int *val, const int limit, const int mul)
> +{
> + int tmp;
> +
> + if (*val<= limit)
> + return 0;
> +
> + /* limit< val< mul */
> + tmp = mul - *val;
> + if (tmp> 0) {
> + *val = limit + 1;
> + return tmp;
> + }
> +
> + /* mul + limit< val */
> + if (WARN_ON(*val - mul> limit)) {
> + /* it's not possible to code that value - set max */
> + *val = (limit + 1) | limit;
> + return 0;
> + }
> +
> + /* mul<= val<= mul + limit */
> + *val = (limit + 1) | (*val - mul);
> + return 0;
> +}
> +
> static void apply_timings(const u8 chipselect, const u8 pio,
> const struct ide_timing *timing, int use_iordy)
> {
> unsigned int t0, t1, t2, t6z;
> - unsigned int cycle, setup, pulse, data_float;
> + unsigned int cycle, cs_cycle, setup, pulse, data_float;
> unsigned int mck_hz;
> struct clk *mck;
>
> @@ -133,11 +165,29 @@ static void apply_timings(const u8 chipselect, const u8 pio,
> setup = calc_mck_cycles(t1, mck_hz);
> pulse = calc_mck_cycles(t2, mck_hz);
> data_float = calc_mck_cycles(t6z, mck_hz);
> -
> - pdbg("cycle=%u setup=%u pulse=%u data_float=%u\n",
> - cycle, setup, pulse, data_float);
> -
> - set_smc_timings(chipselect, cycle, setup, pulse, data_float, use_iordy);
> + cs_cycle = cycle;
> +
> + pdbg("cycle=%u cs_cycle=%u setup=%u pulse=%u data_float=%u\n",
> + cycle, cs_cycle, setup, pulse, data_float);
> +
> + /* SMC use special coding scheme, see "Coding and Range of Timing
> + * Parameters" table from AT91SAM926x datasheets.
> + *
> + * SETUP = 128*setup[5] + setup[4:0]
> + * PULSE = 256*pulse[6] + pulse[5:0]
> + * CYCLE = 256*cycle[8:7] + cycle[6:0]
> + */
> + cycle += code_smc_values(&setup, 31, 128);
> + cycle += code_smc_values(&pulse, 63, 256);
> + /* cs_cycle is a full pulse wave, setup and hold times are 0 */
> + cycle += code_smc_values(&cs_cycle, 63, 256);
> + code_cmc_values(&cycle, 127, 256);
> +
> + pdbg("cycle=0x%x cs_cycle=0x%x setup=0x%x pulse=0x%x data_float=0x%x\n",
> + cycle, cs_cycle, setup, pulse, data_float);
> +
> + set_smc_timings(chipselect, cycle, cs_cycle, setup, pulse, data_float,
> + use_iordy);
> }
>
> static void at91_ide_input_data(ide_drive_t *drive, struct ide_cmd *cmd,
Unfortunately, yours patch can't work correctly because it does not
consider all details, but I understand yours idea and fix pata_at91.c
driver.
You can look to email with "[PATCH] ATA: pata_at91.c bugfixes" subject
for details.
Best regards!
--
Igor Plyatov
More information about the linux-arm-kernel
mailing list