Re: EP93xx PIO IDE driver proposal

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi João,

I merged all preparatory patches into Linus tree but there has been IDE
Maintainer and policy change so I would suggest hurrying up with the final
version of the patch (I think that it will be still fine for 2.6.31 but
the final decision is up to David now).

On Saturday 06 June 2009 17:26:31 Sergei Shtylyov wrote:
> Hello.
> 
> João Ramos wrote:
> 
> > Ok, I've done the fixes you suggested and tested the patch.
> > It is working fine, and I'm sending you (once again) the patch 
> > attached to this e-mail.
> >
> > In the meanwhile, I will post the entire patch to the 'arm-linux' 
> > mailing list, iterating untill I get a final patch for the platform 
> > dependent part.
> > Then, I will send the full patch back at linux-ide for submission.
> 
>    I'm not sure why you again joined the driver patch and the platfrorm 
> code patch together...
> 
> >>>> Ok, I will apply the changes, test them and then resubmit the final 
> >>>> patch.
> >>>> By the way, I will submit the final patch through 'arm-linux' 
> >>>> mailing list, if that is OK to you, since the full patch has 
> >>>> platform-specific dependencies.
> >>>> I mean, as long as we iterate with the IDE part of it untill it is 
> >>>> good for submission from you, I can submit it in 'arm-linux' with 
> >>>> your acknowledge, right?     
> >>>
> >>> It is good to post it also to linux-arm for additional review & 
> >>> better merge
> >>> coordination but the patch itself should go through IDE tree and 
> >>> linux-ide.
> >>>
> >>> [ It is IDE host driver, has also IDE dependencies 
> >>> (ide_{set,get}_drivedata()
> >>>   patch) and there are some other pending IDE changes that will 
> >>> require minor
> >>>   updates to the patch. ]
> >>>
> >>> Just ping me after platform-specific part is in and I'll push the 
> >>> driver
> >>> to Linus (of course given that everybody is happy with the final 
> >>> version).
> 
>    I'm not happy yet. :-)
> 
> [...]
> 
> > Add IDE host controller support for Cirrus Logic's EP93xx CPUs.
> >
> > Signed-off-by: Joao Ramos <joao.ramos@inov.pt>
> > Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> > Cc: Ryan Mallon <ryan@bluewatersys.com>
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Cc: Sergei Shtylyov <sshtylyov@ru.montavista.com>
> >
> > ---
> >
> > diff -urN linux-2.6.30-rc6.orig/arch/arm/mach-ep93xx/core.c linux-2.6.30-rc6/arch/arm/mach-ep93xx/core.c
> > --- linux-2.6.30-rc6.orig/arch/arm/mach-ep93xx/core.c	2009-05-16 05:12:57.000000000 +0100
> > +++ linux-2.6.30-rc6/arch/arm/mach-ep93xx/core.c	2009-05-19 16:03:36.000000000 +0100
> > @@ -537,6 +537,31 @@
> >  	platform_device_register(&ep93xx_i2c_device);
> >  }
> >  
> > +static struct resource ep93xx_ide_resources[] = {
> > +	{
> > +		.start	= EP93XX_IDE_PHYS_BASE,
> > +		.end	= EP93XX_IDE_PHYS_BASE + 0x37,
> > +		.flags	= IORESOURCE_MEM,
> > +	},
> > +	{
> > +		.start	= IRQ_EP93XX_EXT3,
> > +		.end	= IRQ_EP93XX_EXT3,
> > +		.flags	= IORESOURCE_IRQ,
> > +	},
> > +};
> > +
> > +static struct platform_device ep93xx_ide_device = {
> > +	.name		= "ep93xx-ide",
> > +	.id		= -1,
> > +	.num_resources	= ARRAY_SIZE(ep93xx_ide_resources),
> > +	.resource	= ep93xx_ide_resources,
> > +};
> > +
> > +void __init ep93xx_register_ide(void)
> > +{
> > +	platform_device_register(&ep93xx_ide_device);
> > +}
> > +
> >  extern void ep93xx_gpio_init(void);
> >  
> >  void __init ep93xx_init_devices(void)
> > diff -urN linux-2.6.30-rc6.orig/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h linux-2.6.30-rc6/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> > --- linux-2.6.30-rc6.orig/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h	2009-05-16 05:12:57.000000000 +0100
> > +++ linux-2.6.30-rc6/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h	2009-05-19 16:03:36.000000000 +0100
> > @@ -78,6 +78,7 @@
> >  #define EP93XX_BOOT_ROM_BASE		(EP93XX_AHB_VIRT_BASE + 0x00090000)
> >  
> >  #define EP93XX_IDE_BASE			(EP93XX_AHB_VIRT_BASE + 0x000a0000)
> > +#define EP93XX_IDE_PHYS_BASE		(EP93XX_AHB_PHYS_BASE + 0x000a0000)
> >  
> >  #define EP93XX_VIC1_BASE		(EP93XX_AHB_VIRT_BASE + 0x000b0000)
> >  
> > diff -urN linux-2.6.30-rc6.orig/arch/arm/mach-ep93xx/include/mach/platform.h linux-2.6.30-rc6/arch/arm/mach-ep93xx/include/mach/platform.h
> > --- linux-2.6.30-rc6.orig/arch/arm/mach-ep93xx/include/mach/platform.h	2009-05-16 05:12:57.000000000 +0100
> > +++ linux-2.6.30-rc6/arch/arm/mach-ep93xx/include/mach/platform.h	2009-05-19 16:03:36.000000000 +0100
> > @@ -15,6 +15,7 @@
> >  void ep93xx_map_io(void);
> >  void ep93xx_init_irq(void);
> >  void ep93xx_init_time(unsigned long);
> > +void ep93xx_register_ide(void);
> >  void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
> >  void ep93xx_register_i2c(struct i2c_board_info *devices, int num);
> >  void ep93xx_init_devices(void);
> > diff -urN linux-2.6.30-rc6.orig/drivers/ide/ep93xx-ide.c linux-2.6.30-rc6/drivers/ide/ep93xx-ide.c
> > --- linux-2.6.30-rc6.orig/drivers/ide/ep93xx-ide.c	1970-01-01 01:00:00.000000000 +0100
> > +++ linux-2.6.30-rc6/drivers/ide/ep93xx-ide.c	2009-05-19 16:06:33.000000000 +0100
> > @@ -0,0 +1,521 @@
> >   
> [...]
> > +static u16 __ep93xx_ide_read(void __iomem *base, unsigned long addr,
> > +		struct ide_timing *t)
> > +{
> > +	u32 reg;
> > +
> > +	reg = addr | IDECTRL_DIORN | IDECTRL_DIOWN;
> > +	writel(reg, base + IDECTRL);
> > +	ndelay(t->setup);
> > +
> > +	reg &= ~IDECTRL_DIORN;
> > +	writel(reg, base + IDECTRL);
> > +	ndelay(t->active);
> > +
> > +	while (!ep93xx_ide_check_iordy(base))
> > +		cpu_relax();
> > +
> > +	reg |= IDECTRL_DIORN;
> > +	writel(reg, base + IDECTRL);
> > +	ndelay(t->recover);
> >   
> 
>   No, actually it should be t->cycle - t->active - t->setup. t->recover 
> is a only a minimum recovery time, and you need to respect t->cycle minimum.
> 
> > +static void __ep93xx_ide_write(void __iomem *base, u16 value,
> > +		unsigned long addr, struct ide_timing *t)
> > +{
> > +	u32 reg;
> > +
> > +	reg = addr | IDECTRL_DIORN | IDECTRL_DIOWN;
> > +	writel(reg, base + IDECTRL);
> > +	ndelay(t->setup);
> > +
> > +	writel(value, base + IDEDATAOUT);
> > +
> > +	reg &= ~IDECTRL_DIOWN;
> > +	writel(reg, base + IDECTRL);
> > +	ndelay(t->active);
> > +
> > +	while (!ep93xx_ide_check_iordy(base))
> > +		cpu_relax();
> > +
> > +	reg |= IDECTRL_DIOWN;
> > +	writel(reg, base + IDECTRL);
> > +	ndelay(t->recover);
> >   
> 
>    Same here...
> 
> > +static void ep93xx_ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
> > +{
> > +	ide_drive_t *pair = ide_get_pair_dev(drive);
> > +	struct ide_timing *t = ide_timing_find_mode(XFER_PIO_0 + pio);
> > +	struct ide_timing *cmd_t = t;
> > +	u32 reg = IDECFG_IDEEN | IDECFG_PIO;
> > +	void __iomem *base = drive->hwif->host->host_priv;
> > +
> > +	if (pair) {
> > +		struct ide_timing *pair_t = ide_get_drivedata(pair);
> > +
> > +		if (pair_t && pair_t->mode < t->mode)
> > +			cmd_t = pair_t;
> >   
> 
>    No, it should be like in other drivers:
> 
>         if (pair) {
> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/ide/palm_bk3710.c;h=09d813d313f4a44986a2f358d8ddc08c619533f1;hb=HEAD#l169>                 
> u8 mode = ide_get_best_pio_mode(pair, 255, 4);
> 
> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/ide/palm_bk3710.c;h=09d813d313f4a44986a2f358d8ddc08c619533f1;hb=HEAD#l170> 
> 
> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/ide/palm_bk3710.c;h=09d813d313f4a44986a2f358d8ddc08c619533f1;hb=HEAD#l171>                 
> if (mode < pio)
> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/ide/palm_bk3710.c;h=09d813d313f4a44986a2f358d8ddc08c619533f1;hb=HEAD#l172>                         
> cmd_t = ide_timing_find_mode(XFER_PIO_0 + mode) ;
> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/ide/palm_bk3710.c;h=09d813d313f4a44986a2f358d8ddc08c619533f1;hb=HEAD#l173>         
> }
> 
> 
> > +	}
> > +
> > +	/*
> > +	 * store the adequate PIO mode timings, to be used later
> > +	 * by ep93xx_ide_{read,write}
> > +	 */
> > +	ide_set_drivedata(drive, (void *)t);
> > +	ide_set_hwifdata(drive->hwif, (void *)cmd_t);
> > +
> > +	/* reconfigure IDE controller according to PIO mode */
> > +	switch (pio) {
> > +	case 4:
> > +		reg |= IDECFG_PIO_MODE_4 | ((1 << 8) & IDECFG_WST);
> >   
> 
>    What does WST field mean? I also don't understand what purpose could 
> serve setting the PIO mode here if you ensure to apply all the necessary 
> manually...
> 
> > +static struct ide_port_ops ep93xx_ide_port_ops = {
> > +	.set_pio_mode = ep93xx_ide_set_pio_mode,
> > +};
> > +
> > +static struct ide_port_info ep93xx_ide_port_info = {
> > +	.name		= MODULE_NAME,
> > +	.tp_ops 	= &ep93xx_ide_tp_ops,
> > +	.port_ops 	= &ep93xx_ide_port_ops,
> > +	.host_flags 	= IDE_HFLAG_SINGLE | IDE_HFLAG_POST_SET_MODE |
> >   
> 
>   Is the latter really necessary?
> 
> > +	/*
> > +	 * fill in ide_io_ports structure.
> > +	 * the device IDE register to be accessed is selected through
> > +	 * IDECTRL register's specific bitfields 'DA', 'CS1n' and 'CS0n':
> > +	 *   b4   b3   b2    b1     b0
> > +	 *   A2   A1   A0   CS1n   CS0n
> > +	 * the values filled in this structure allows the value to be directly
> > +	 * ORed to the IDECTRL register, hence giving directly the A[2:0] and
> > +	 * CS1n/CS0n values for each IDE register.
> > +	 * The values correspond to the transformation:
> > +	 *   ((real IDE address) << 2) | CS1n value << 1 | CS0n value
> > +	 */
> > +	hw.io_ports.data_addr = 0x02;
> > +	hw.io_ports.error_addr = 0x06;
> > +	hw.io_ports.nsect_addr = 0x0A;
> > +	hw.io_ports.lbal_addr = 0x0E;
> > +	hw.io_ports.lbam_addr = 0x12;
> > +	hw.io_ports.lbah_addr = 0x16;
> >
> >   
> 
>    Hm, where's this empty line from? :-)
> 
> > +	hw.io_ports.device_addr = 0x1A;
> > +	hw.io_ports.command_addr = 0x1E;
> > +	hw.io_ports.ctl_addr = 0x01;
> 
> MBR, Sergei