Re: EP93xx PIO IDE driver proposal

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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