Wednesday 04 February 2009 13:19:50 Sergei Shtylyov napisał(a):
> > diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
> > index fb51f0e..6674b9b 100644
> > --- a/arch/arm/mach-at91/include/mach/board.h
> > +++ b/arch/arm/mach-at91/include/mach/board.h
> > @@ -59,6 +59,18 @@ struct at91_cf_data {
> > };
> > extern void __init at91_add_device_cf(struct at91_cf_data *data);
> >
> > + /* Compact Flash True IDE mode */
> > +struct at91_ide_data {
> > + u8 irq_pin; /* the same meaning as for CF */
> >
>
> I again have to express my dislike about not passing IRQ the usual
> way. Also, see my comments to the platform code.
Yes, I know, I don't like to argue. Only reasoning to use platform irq resource
seams to be: "because other drivers do". However we have exception - at91_cf
also use board->irq_pin, so maybe this driver could also do ?
> > + u8 det_pin;
> > + u8 rst_pin;
> > + u8 chipselect;
> > + u8 flags;
> > +#define AT91_IDE_SWAP_A0_A2 0x01
> > +};
> > +
> > +extern void __init at91_add_device_ide(struct at91_ide_data *data);
> > +
> > /* MMC / SD */
> > struct at91_mmc_data {
> > u8 det_pin; /* card detect IRQ */
> > diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> > index 3dad229..b11da5b 100644
> > --- a/drivers/ide/Kconfig
> > +++ b/drivers/ide/Kconfig
> > @@ -721,6 +721,11 @@ config BLK_DEV_IDE_TX4939
> > depends on SOC_TX4939
> > select BLK_DEV_IDEDMA_SFF
> >
> > +config BLK_DEV_IDE_AT91
> > + tristate "Atmel AT91 IDE support"
> >
>
> Please be more specific -- you can't drive AT91RM9200 SMC.
Ok.
>
> > + depends on ARM && ARCH_AT91
> >
>
> Please add "&& !ARCH_AT91RM9200". And maybe "&& !ARCH_AT91X40" too...
Ok.
> > diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> > new file mode 100644
> > index 0000000..3a1f7e0
> > --- /dev/null
> > +++ b/drivers/ide/at91_ide.c
> > @@ -0,0 +1,496 @@
> > +/*
> > + * IDE host driver for AT91SAM9 Static Memory Controller
> >
>
> Why not call the driver 'at91sam9_ide'?
>
> > +/*
> > + * AT91 Static Memory Controller
>
> AT91SAM9.
Ok, currently only SAM9 can be used with driver. However I think adding
support to AT91RM9200 to this driver will be not much effort. I don't think
someone will want to write new driver for RM9200 insted using this one.
IMHO, current name is short and sweet and not miss the true so much -
most of the AT91s which can work with True IDE logic are supported.
I would like to stay with current name.
> > maps Task File and Data Register
> >
> > + * at the same address. To distinguish access between these two
>
> It would have been strange if it did it otherwise...
>
> > + * different bus data width is used: 8Bit for Task File, 16Bit for Data I/O
> > + *
> > + * After initialization we do 8/16 bit flipping (changes in SMC MODE register)
> > + * only inside IDE callback routines which are serialized by IDE layer,
> > + * so no additional locking needed.
> > + */
> > +
> > +static void init_smc_mode(const u8 chipselect)
> > +{
> > + at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
> > + AT91_SMC_WRITEMODE |
> > + AT91_SMC_BAT_SELECT |
> > + AT91_SMC_TDF_(0));
> >
>
> I'm not sure why are you fixing the dataflow timing again, this time
> to 0...
Not necessary code, will be removed.
> > +}
> > +
> > +static inline void set_8bit_mode(const u8 chipselect)
> > +{
> > + unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> > + mode &= ~AT91_SMC_DBW;
> > + mode |= AT91_SMC_DBW_8;
> > + at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> > + pdbg("%u %08lx\n", chipselect, mode);
> > +}
> > +
> > +static inline void set_16bit_mode(const u8 chipselect)
> > +{
> > + unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> > + mode &= ~AT91_SMC_DBW;
> > + mode |= AT91_SMC_DBW_16;
> > + at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> > + pdbg("%u %08lx\n", chipselect, mode);
> > +}
> >
>
> I'd advice to make this single function because it looks like a code
> duplication too much.
Uh well.
> > +static void set_smc_timings(const u8 chipselect, const u16 cycle,
> > + const u16 setup, const u16 pulse,
> > + const u16 data_float, int use_iordy)
> >
>
> Have you considered using already present sam9_smc_configure()
> instead to avoid the code duplication (it needs to be exported though)?
I think, it need to be exported, I'll check.
> > +{
> > + unsigned long 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));
> > +
> > + mode = at91_sys_read(AT91_SMC_MODE(chipselect));
> >
>
> Frankly speaking, I don't see why you're reading this register back
> if you already know what needs to be set there -- as you've done it in
> init_smc_mode().
This function is not only used at initialization. It is also used when PIO mode
is changed.
> > +static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
> > +{
> > + u64 tmp = ns;
> > +
> > + tmp *= mck_hz;
> > + tmp += 1000*1000*1000 - 1; /* round up */
> > + do_div(tmp, 1000*1000*1000);
> > + return (unsigned int) tmp;
> > +}
> > +
> > +static void apply_timings(const u8 chipselect, const u8 pio,
> > + const struct ide_timing *timing, const u16 *ata_id)
> > +{
> > + unsigned int t0, t1, t2, t6z, th;
> > + unsigned int cycle, setup, pulse, data_float;
> > + unsigned int mck_hz;
> > + struct clk *mck;
> > + int use_iordy;
> >
> [...]
> > +
> > + use_iordy = 0;
> >
>
> Can be done in initializer...
But here is more readable, I think.
> > + if (ata_id) {
> > + if (ata_id_is_cfa(ata_id)) {
> > + if (pio == 3 || pio == 4)
> > + use_iordy = 1;
> >
> > + } else if (pio >= 3 || ata_id_has_iordy(ata_id))
> > + use_iordy = 1;
> >
>
>
> No, you should check for IORDY regardless of CFA and using IORDY
> shouldn't be limeited to modes 3 and 4:
Hmm, I take as example ata_pio_need_iordy(), it is also wrong or maybe
I don't follow the logic properly.
> if (ata_id_has_iordy(ata_id) && !(ata_id_is_cfa(ata_id) && pio > 4))
> use_iodry = 1;
>
> > +static u8 ide_mm_inb(unsigned long port)
> > +{
> > + return (u8) readb((void __iomem *) port);
> >
>
> Explict cast not needed here.
Ok.
> > +irqreturn_t at91_irq_handler(int irq, void *dev_id)
> > +{
> > + int ntries = 8;
> > + int pin_val1, pin_val2;
> > +
> > + /* additional deglitch, line can be noisy in badly designed PCB */
> > + do {
> > + pin_val1 = at91_get_gpio_value(irq);
> > + pin_val2 = at91_get_gpio_value(irq);
> > + } while (pin_val1 != pin_val2 && --ntries > 0);
> >
> I suggest a shorter code:
>
> do {
> pin_val = at91_get_gpio_value(irq);
> } while (pin_val != at91_get_gpio_value(irq) && --ntries > 0);
For me your form is less readable.
> > +static int __init at91_ide_probe(struct platform_device *pdev)
> > +{
> >
> [...]
> > + host->ports[0]->extra_base = board->chipselect;
> >
>
> BTW, we have 2 fields in the struct hwif_s that fit this case better:
> config_data and select_data. It's a bit stange that you've selected
> extra_base...
Ok.
Regards
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
