Tuesday 20 January 2009 12:07:17 Sergei Shtylyov napisał(a):
> > arch/arm/mach-at91/at91sam9263_devices.c | 96 +++++++
> > arch/arm/mach-at91/board-sam9263ek.c | 11 +
> > arch/arm/mach-at91/include/mach/board.h | 9 +
> >
>
> This should probably go thru the ARM tree... though the maintainer
> will decide.
I will submit patches to linux-ide and ARM things separately (ARM when I
finally test with Atmel Evaluation Kit) . Against which tree IDE patches
should be submitted, against Bart's kernel?
> > +/* Proper CS address space will be added */
> > +#define AT91_IDE_TASK_FILE 0x00c00000
> > +#define AT91_IDE_CTRL_REG 0x00e00000
> >
>
> Er, are you sure? Address lines should be 110 to address the device
> control and alternate status registers, do they get asserted properly?
Hmm, you may have right, because I can see warning
hda: probing with STATUS(0x50) instead of ALTSTATUS(0x00)
but I don't understand this issue, I'm going to investigate.
> > + * IDE host driver for AT91 Static Memory Controler
> >
>
> I'm afraid you're being too generic here: e.g. AT91RM9200 has
> incompatible SMC.
Well, we could add #ifdef with diffrent implementation of init_smc_mode(),
set_8bit_mode(), etc...
> > + at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
> > + AT91_SMC_WRITEMODE |
> > + AT91_SMC_BAT_SELECT |
> > + AT91_SMC_TDF_(2));
>
> I'm not sure why you're fixing the data float timing to 2 cycles --
> it correponds to the t6z timing which is 30 ns for ATA modes and 20 ns
> for CF specific ones...
Right, TDF it's not needed as long as recover time is longer than t6z and such
is in our case.
> > + tmp *= mck_hz;
> > + tmp += 1000*1000*1000 + 1; /* round up */
>
> You probably mean -1 if you intend to just round up (so tmp +=
> 999999999).
Opps.
> > + if (t9 < t2i - t1)
> > + t9 = t2i - t1;
> >
>
> It more sense to calculate such things *after* quantizing the
> timings with calc_mck_cycles()...
Why?
> > + mck = clk_get(NULL, "mck");
> > + BUG_ON(IS_ERR(mck));
> > + mck_hz = clk_get_rate(mck);
> >
>
> Why not call clk_get[_rate]() only once, at startup? It can change
> dynamically?
No. But it's like a global variable with some fancy interface. I don't like to
have own copy global copy of mck_hz.
> > + t2i = calc_mck_cycles(t2i, mck_hz);
> >
>
> If you're not using t2i, why quantize it?
Just for debug, will be removed.
> > + t9 = calc_mck_cycles(t9, mck_hz);
> > + pdbg("t0=%u t1=%u t2=%u t2i=%u t9=%u\n", t0, t1, t2, t2i, t9);
> >
>
> Besides, we have ide_timing_compute() doing the same thing.
I don't like results of ide_timing_compute(), I will use ide_timing_find_mode() and quantize
by my own. This also is needed at startup when we need to program SMC and have no
drive structure to pass to ide_timing_compute().
>
> > + /* values are rounded up so we need to assure cycle is larger than pulse */
> > + if (t0 < t1 + t2 + t9)
> > + t0 = t1 + t2 + t9;
> > +
> > + /* setup calculated timings */
> > + at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
> > + AT91_SMC_NCS_WRSETUP_(0) |
> > + AT91_SMC_NRDSETUP_(t1) |
> > + AT91_SMC_NCS_RDSETUP_(0));
> > + at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
> > + AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
> >
>
> With zero address to CS setup time it's the same as t0.
Not true for slower PIO modes. But CS pulse can be t0. Write data is driver on
NWR signal (WRITE_MODE = 1) in at91_ide, in opposite to pata_at32 where
there is need to non zero CS setup or recovery time.
> > + /* disable or enable waiting for IORDY signal */
> > + mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> > + mode &= ~AT91_SMC_EXNWMODE;
> > + if (pio <= 4)
> >
>
> The IORDY loigic is not as simple -- you'd better use
> ata_id_has_iordy() for PIO modes < 5.
Hmm. Really IDE host should behave like this? Must IDENTYFY
DEVICE and then disable IORDY if device not use it. Maybe
this is simpler, host have to react on IORDY signal but device
just not assert it.
I would like to remove this code and have allways NWAIT
READY mode to make flipping 8/16 simpler. But I don't think we
have guarantees that PIO5 and 6 devices not assert IORDY.
> > + else
> > + mode |= AT91_SMC_EXNWMODE_DISABLE;
> >
>
> This constant is 0, so else branch can be skipped.
Do you think this make code more readable? Compiler optimize this.
> Why not pass it normally, via the platform device's resource?
Irq pin is board specific. When use resource I will need in processor
code modify resource. I think this is not necessary code. Anyway
I need to have access to board data to get chipselect number.
> > +#ifdef AT91_GPIO_IRQ_HACK
> > +#define NR_TRIES 10
> > + int ntries = 0;
> > + int pin_val1, pin_val2;
> > + do {
> > + pin_val1 = at91_get_gpio_value(AT91_PIN_PB20);
> > + pin_val2 = at91_get_gpio_value(AT91_PIN_PB20);
> > + } while (pin_val1 != pin_val2 && ntries++ < NR_TRIES);
> >
>
> You need manual pin debouncing even after calling
> at91_set_deglitch()? :-O
I'll check. Maybe the delay is enough for us.
> MBR, Sergei
Thanks very much
Stanislaw Gruszka
--
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
