[PATCH v2] ide: at91_ide.c bugfix for high master clock

[PATCH v2] ide: at91_ide.c bugfix for high master clock

[Igor Plyatov]

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.

Signed-off-by: Igor Plyatov <plyatov@gmail.com>
---
 drivers/ide/at91_ide.c |   33 +++++++++++++++++++++++----------
 1 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
index 000a78e..5f97710 100644
--- a/drivers/ide/at91_ide.c
+++ b/drivers/ide/at91_ide.c
@@ -36,6 +36,8 @@
 #define perr(fmt, args...) pr_err(DRV_NAME ": " fmt, ##args)
 #define pdbg(fmt, args...) pr_debug("%s " fmt, __func__, ##args)
 
+#define NCS_RD_PULSE_LIMIT  0x3f /* maximal value for pulse bitfields */
+
 /*
  * Access to IDE device is possible through EBI Static Memory Controller
  * with Compact Flash logic. For details see EBI and SMC datasheet sections
@@ -70,6 +72,7 @@ static void set_smc_timings(const u8 chipselect, const u16 cycle,
 			    const u16 setup, const u16 pulse,
 			    const u16 data_float, int use_iordy)
 {
+	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);
+	}
+
 	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
 
 	/* setup timings in SMC */
-	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
-						   AT91_SMC_NCS_WRSETUP_(0) |
-						   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_NRDPULSE_(pulse) |
-						   AT91_SMC_NCS_RDPULSE_(cycle));
-	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
-						   AT91_SMC_NRDCYCLE_(cycle));
+	at91_sys_write(AT91_SMC_SETUP(chipselect),
+			AT91_SMC_NWESETUP_(setup) |
+			AT91_SMC_NCS_WRSETUP_(0) |
+			AT91_SMC_NRDSETUP_(setup) |
+			AT91_SMC_NCS_RDSETUP_(0));
+	at91_sys_write(AT91_SMC_PULSE(chipselect),
+			AT91_SMC_NWEPULSE_(pulse) |
+			AT91_SMC_NCS_WRPULSE_(ncs_rd_pulse) |
+			AT91_SMC_NRDPULSE_(pulse) |
+			AT91_SMC_NCS_RDPULSE_(ncs_rd_pulse));
+	at91_sys_write(AT91_SMC_CYCLE(chipselect),
+			AT91_SMC_NWECYCLE_(cycle) |
+			AT91_SMC_NRDCYCLE_(cycle));
 }
 
 static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
-- 
1.7.0.4

[PATCH v2] ide: at91_ide.c bugfix for high master clock

[Stanislaw Gruszka]

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. 

Thanks
Stanislaw

[PATCH v2] ide: at91_ide.c bugfix for high master clock

[Stanislaw Gruszka]

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,

[PATCH v2] ide: at91_ide.c bugfix for high master clock

[Igor Plyatov]

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