Re: [PATCH 2/3] ide: add at91_ide driver

[Stanislaw Gruszka <stf_xl@wp.pl>]

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