| Message ID | 1303299877-12088-1-git-send-email-plyatov@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/20/2011 07:44 AM, 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 > CS_PULSE = PULSE + SETUP/2 + HOLD/2 > * This fixes allows to correctly operate with any master clock frequency. > > Signed-off-by: Igor Plyatov<plyatov@gmail.com> > --- > drivers/ata/pata_at91.c | 223 ++++++++++++++++++++++++++++++++--------------- > 1 files changed, 154 insertions(+), 69 deletions(-) FYI, the two previous "v4" patches are now applied. Based on the comments from Stanislaw, this patch would need to be updated anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
On Tue, Apr 26, 2011 at 10:38:04AM +0400, Igor Plyatov wrote: > >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 err, setup time is t1 > >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? Full "External Bus Interface" section from AT91 spec is important, because is describe how SMC works internally in CF True IDE mode. > 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 Correct. Also t0 = t1 + t2 + t9 relation must be true. > Chip Select (NCS) signal parameters: > * cs_setup = NCS_RD_SETUP > * cs_pulse = NCS_RD_PULSE > * cs_cycle = cycle You can setup that, but they need to have proper relations with NRD. > Notes: > * The NCS_RD_CYCLE is equal to the NRD_CYCLE for AT91SAM9, because they > start/finish simultaneously (HW related). No, NCS and NRD are different signals, waveforms are different (if configured differently in SMC controller). > * The NCS signal is not the same as CS1, CS2 ATA signals! It used only to > enable data bus transceiver U2. Well, they are different signals but connected together. All of these are controlled by NCS: CFCE1 (CS1), CFCE2 (CS2), CFRNW (DIR), CFCS0 (OE) if SMC is configured in True IDE mode. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > * The NCS_RD_CYCLE is equal to the NRD_CYCLE for AT91SAM9, because they > > start/finish simultaneously (HW related). > No, NCS and NRD are different signals, waveforms are different (if > configured differently in SMC controller). Your sentence is true. NRD and NCS cycle time must be the same (otherwise SMC behaviour is undefined), hence both signals start/finish simultaneously. Anyway waveforms (setup, and pulse time) can be different. All of thise are kinda obvious, no? :-) Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(Cc again linux-ide) > >>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? > >Full "External Bus Interface" section from AT91 spec is important, > >because is describe how SMC works internally in CF True IDE mode. > > You can look at it here: > http://www.atmel.com/dyn/resources/prod_documents/doc6384.pdf > 12 MB, Pages 159-200. I know where AT91 documentation can be downloaded from. > >>* The NCS signal is not the same as CS1, CS2 ATA signals! It used only to > >> enable data bus transceiver U2. > >Well, they are different signals but connected together. All of these are > >controlled by NCS: CFCE1 (CS1), CFCE2 (CS2), CFRNW (DIR), CFCS0 (OE) > >if SMC is configured in True IDE mode. > > I must repeat again - NCS ("CFCS1" from schematic) are completely > different signal for different purpose, then A0, A1, A2, CS0, CS1. > It used only to enable data bus transceiver U2. So, how in your opinion CS0 and CS1 are controlled? IMO there are delivered from CFCE1 and CFCE2, which itself are delivered from NCS. Here is corresponding quote from atmel documentation you cited above: "The CFCE1 and CFCE2 waveforms are identical to the corresponding NCSx waveform" > I think it is much better do not simply keep > > cs_setup = 0; > cs_pulse = cycle; /* cs_hold = 0 */ > > as you propose, but better to keep good balance and assume: > > cs_setup = setup / 2; Again. If you do this, real t1 time - ADDR "A02, A01, A00, -CS0, -CS1" valid to "-IORD / -IOWR" edge, on ATA bus will be equal "setup / 2" not "setup" as it should. Actually only time between -CS0, -CS1 and -IORD/-IOWR will not be correct, address lines are established at the beginning of SMC cycle, as you pointed. > This is because at high master clock frequency, "cs_pulse" value > physically can not be as large as "cycle" (SMC HW limitations). There is no point to consider times of rising and falling edges of electric signal in discussion here, they are small enough and can be ignored. AFAIK NCS pulse time equal to cycle time is pretty valid SMC setting, there is no reason to not utilize that. > So, I still don't understand why you think that my equations is not right. > If you think another way, then please explain your idea better then > a few words. I'm sorry, I'm not able to explain things different than I did in this and my previous emails. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stanislaw! Thank you for patience and detailed explanations! Sorry for long delays between answers, but I must work with another projects. > >>>> * The NCS signal is not the same as CS1, CS2 ATA signals! It used only to >>>> enable data bus transceiver U2. >>> Well, they are different signals but connected together. All of these are >>> controlled by NCS: CFCE1 (CS1), CFCE2 (CS2), CFRNW (DIR), CFCS0 (OE) >>> if SMC is configured in True IDE mode. >> I must repeat again - NCS ("CFCS1" from schematic) are completely >> different signal for different purpose, then A0, A1, A2, CS0, CS1. >> It used only to enable data bus transceiver U2. > So, how in your opinion CS0 and CS1 are controlled? IMO there > are delivered from CFCE1 and CFCE2, which itself are delivered > from NCS. Here is corresponding quote from atmel documentation > you cited above: > > "The CFCE1 and CFCE2 waveforms are identical to the corresponding NCSx waveform" This makes picture completely clean for me now. I will publish new version patch for pata_at91.c. Best regards!
Hi Jeff! > On 04/20/2011 07:44 AM, 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 >> CS_PULSE = PULSE + SETUP/2 + HOLD/2 >> * This fixes allows to correctly operate with any master clock >> frequency. >> >> Signed-off-by: Igor Plyatov<plyatov@gmail.com> >> --- >> drivers/ata/pata_at91.c | 223 >> ++++++++++++++++++++++++++++++++--------------- >> 1 files changed, 154 insertions(+), 69 deletions(-) > > FYI, the two previous "v4" patches are now applied. > > Based on the comments from Stanislaw, this patch would need to be > updated anyway. Thank you for info! I will update this patch. Best regards!
diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c index 0da0dcc..b18249e 100644 --- a/drivers/ata/pata_at91.c +++ b/drivers/ata/pata_at91.c @@ -3,6 +3,7 @@ * with CompactFlash interface in True IDE mode * * Copyright (C) 2009 Matyukevich Sergey + * 2011 Igor Plyatov * * Based on: * * generic platform driver by Paul Mundt: drivers/ata/pata_platform.c @@ -31,26 +32,140 @@ #include <mach/board.h> #include <mach/gpio.h> +#define DRV_NAME "pata_at91" +#define DRV_VERSION "0.2" -#define DRV_NAME "pata_at91" -#define DRV_VERSION "0.1" - -#define CF_IDE_OFFSET 0x00c00000 -#define CF_ALT_IDE_OFFSET 0x00e00000 -#define CF_IDE_RES_SIZE 0x08 +#define CF_IDE_OFFSET 0x00c00000 +#define CF_ALT_IDE_OFFSET 0x00e00000 +#define CF_IDE_RES_SIZE 0x08 struct at91_ide_info { unsigned long mode; unsigned int cs; - struct clk *mck; - void __iomem *ide_addr; void __iomem *alt_addr; }; -static const struct ata_timing initial_timing = - {XFER_PIO_0, 70, 290, 240, 600, 165, 150, 600, 0}; +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 +}; + +/* + * Adjust value for one of SMC_SETUP, SMC_PULSE or SMC_CYCLE registers. + * Returns: + * difference between input and output value or + * negative value in case of invalid input value. + */ +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 */ +} + +/* + * Calculate SMC_SETUP, SMC_PULSE, SMC_CYCLE register values. + * + * 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] + * + * cs_setup = setup/2 + * cs_pulse = pulse + setup/2 + hold/2 + */ +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 */ + }; + + ret_val = adjust_smc_value(setup, range_setup, sizeof(range_setup)); + if (ret_val < 0) + dev_warn(dev, "maximal setup value\n"); + else + *cycle += ret_val; + + ret_val = adjust_smc_value(pulse, range_pulse, sizeof(range_pulse)); + if (ret_val < 0) + dev_warn(dev, "maximal pulse value\n"); + else + *cycle += ret_val; + + ret_val = adjust_smc_value(cycle, range_cycle, sizeof(range_cycle)); + if (ret_val < 0) + dev_warn(dev, "maximal cycle value\n"); + + *cs_setup = *setup / 2; + ret_val = adjust_smc_value(cs_setup, range_setup, sizeof(range_setup)); + if (ret_val < 0) + dev_warn(dev, "maximal cs_setup value\n"); + + if (*cs_setup >= *setup) { + dev_warn(dev, "cs_setup value >= setup\n"); + *cs_pulse = *pulse; + } else { + if ((*cs_setup == 0) && (*setup == 1)) + *cs_setup = 1; + + /* transformed pulse + setup/2 + hold/2 */ + *cs_pulse = (*pulse + *cycle)/2; + ret_val = adjust_smc_value(cs_pulse, range_pulse, + sizeof(range_pulse)); + if (ret_val < 0) + dev_warn(dev, "maximal cs_pulse value\n"); + else if (ret_val > 0) { + cs_hold = *cycle - *cs_setup - *cs_pulse; + if (cs_hold < ret_val) + dev_warn(dev, "unable setup correct cs_pulse\n"); + } + } + + dev_dbg(dev, "setup=%u, cs_setup=%u, pulse=%u, cs_pulse=%u, cycle=%u\n", + *setup, *cs_setup, *pulse, *cs_pulse, *cycle); +} static unsigned long calc_mck_cycles(unsigned long ns, unsigned long mck_hz) { @@ -78,71 +193,43 @@ static void set_smc_mode(struct at91_ide_info *info) static void set_smc_timing(struct device *dev, struct at91_ide_info *info, const struct ata_timing *ata) { - unsigned long read_cycle, write_cycle, active, recover; - unsigned long nrd_setup, nrd_pulse, nrd_recover; - unsigned long nwe_setup, nwe_pulse; - - unsigned long ncs_write_setup, ncs_write_pulse; - unsigned long ncs_read_setup, ncs_read_pulse; - - unsigned long mck_hz; - - read_cycle = ata->cyc8b; - nrd_setup = ata->setup; - nrd_pulse = ata->act8b; - nrd_recover = ata->rec8b; + unsigned int cycle; /* ATA cycle width in MCK ticks */ + unsigned int setup; /* setup width in MCK ticks */ + unsigned int pulse; /* CFIOR and CFIOW pulse width in MCK ticks */ + unsigned int cs_setup = 0;/* CS4 or CS5 setup width in MCK ticks */ + unsigned int cs_pulse = 0;/* CS4 or CS5 pulse width in MCK ticks*/ + unsigned long mck_hz; /* MCK frequency in Hz */ mck_hz = clk_get_rate(info->mck); + cycle = calc_mck_cycles(ata->cyc8b, mck_hz); + setup = calc_mck_cycles(ata->setup, mck_hz); + pulse = calc_mck_cycles(ata->act8b, mck_hz); - read_cycle = calc_mck_cycles(read_cycle, mck_hz); - nrd_setup = calc_mck_cycles(nrd_setup, mck_hz); - nrd_pulse = calc_mck_cycles(nrd_pulse, mck_hz); - nrd_recover = calc_mck_cycles(nrd_recover, mck_hz); - - active = nrd_setup + nrd_pulse; - recover = read_cycle - active; - - /* Need at least two cycles recovery */ - if (recover < 2) - read_cycle = active + 2; - - /* (CS0, CS1, DIR, OE) <= (CFCE1, CFCE2, CFRNW, NCSX) timings */ - ncs_read_setup = 1; - ncs_read_pulse = read_cycle - 2; - - /* Write timings same as read timings */ - write_cycle = read_cycle; - nwe_setup = nrd_setup; - nwe_pulse = nrd_pulse; - ncs_write_setup = ncs_read_setup; - ncs_write_pulse = ncs_read_pulse; - - 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); + calc_smc_vals(dev, &setup, &cs_setup, &pulse, &cs_pulse, &cycle); + /* Convert values into SMC format */ + setup = (setup & 0x1f) | ((setup & 0x80) >> 2); + pulse = (pulse & 0x3f) | ((pulse & 0x100) >> 2); + cycle = (cycle & 0x7f) | ((cycle & 0x300) >> 1); + cs_setup = (cs_setup & 0x1f) | ((cs_setup & 0x80) >> 2); + cs_pulse = (cs_pulse & 0x3f) | ((cs_pulse & 0x100) >> 2); + + /* SMC setup. Write timings are same as read timings */ at91_sys_write(AT91_SMC_SETUP(info->cs), - AT91_SMC_NWESETUP_(nwe_setup) | - AT91_SMC_NRDSETUP_(nrd_setup) | - AT91_SMC_NCS_WRSETUP_(ncs_write_setup) | - AT91_SMC_NCS_RDSETUP_(ncs_read_setup)); + AT91_SMC_NWESETUP_(setup) | + AT91_SMC_NRDSETUP_(setup) | + AT91_SMC_NCS_WRSETUP_(cs_setup) | + AT91_SMC_NCS_RDSETUP_(cs_setup)); at91_sys_write(AT91_SMC_PULSE(info->cs), - AT91_SMC_NWEPULSE_(nwe_pulse) | - AT91_SMC_NRDPULSE_(nrd_pulse) | - AT91_SMC_NCS_WRPULSE_(ncs_write_pulse) | - AT91_SMC_NCS_RDPULSE_(ncs_read_pulse)); + AT91_SMC_NWEPULSE_(pulse) | + AT91_SMC_NRDPULSE_(pulse) | + AT91_SMC_NCS_WRPULSE_(cs_pulse) | + AT91_SMC_NCS_RDPULSE_(cs_pulse)); at91_sys_write(AT91_SMC_CYCLE(info->cs), - AT91_SMC_NWECYCLE_(write_cycle) | - AT91_SMC_NRDCYCLE_(read_cycle)); - - return; + AT91_SMC_NWECYCLE_(cycle) | + AT91_SMC_NRDCYCLE_(cycle)); } static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev) @@ -163,8 +250,6 @@ static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev) /* Setup SMC mode */ set_smc_mode(info); - - return; } static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev, @@ -330,7 +415,7 @@ static int __devexit pata_at91_remove(struct platform_device *pdev) static struct platform_driver pata_at91_driver = { .probe = pata_at91_probe, .remove = __devexit_p(pata_at91_remove), - .driver = { + .driver = { .name = DRV_NAME, .owner = THIS_MODULE, },
* 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 CS_PULSE = PULSE + SETUP/2 + HOLD/2 * This fixes allows to correctly operate with any master clock frequency. Signed-off-by: Igor Plyatov <plyatov@gmail.com> --- drivers/ata/pata_at91.c | 223 ++++++++++++++++++++++++++++++++--------------- 1 files changed, 154 insertions(+), 69 deletions(-)