Re: EP93xx PIO IDE driver proposal

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

Hello.

João Ramos wrote:

> So here is the revised patch, according to yours comments, to add 
> support for the IDE host controller in the Cirrus Logic's EP93xx CPUs.
> This patch is made against kernel 2.6.30-rc4 (latest release candidate 
> in Linus's tree).

> I've preferred to attach the patch instead of inlining it in this mail, 
> as my mailer seems to deform the patch output.

> Please confirm if the patch is ok.

    It looks OK tab-wise now...

> Sergei: I've added the delays you suggested in the read/write 
> procedures, accordingly to the delays specified in the processor's user 
> manual for PIO Mode 4.

    Why only for PIO mode 4 if you're claiming support for modes 0 thru 4?

> These delays really introduce quite a performance loss. Although the 
> driver is working, these delays make file transfers quite slow; do we 
> really need to enforce manually these delays?

    Unfortunately, yes.

> I mean, on a 200MHz CPU (5ns instruction cycle), can't we assume that 
> instructions and branches that occur between C code instructions will 
> suffice to some of these delays

    You need to accurately measure that, not just assume.

> (the delays are 25ns + 70ns + 25ns), or 


> yet, can't we reduce some of these delays assuming some instruction 
> cycle time is already spent on branches and instructions between reads 
> and writes to the IDE control registers?

    We can in principle, but that time should be accurately measured.

> Best regards,
> João Ramos

> ------------------------------------------------------------------------
> 
> Add IDE host controller support for Cirrus Logic's EP93xx CPUs.
> This driver currently supports only PIO-4 transfer mode.

    It claims support for all modes 0 to 4 nevertheless.

> Signed-off-by: Joao Ramos <joao.ramos@inov.pt>

> ---

> diff -urN linux-2.6.30-rc4.orig/arch/arm/mach-ep93xx/core.c linux-2.6.30-rc4/arch/arm/mach-ep93xx/core.c
> --- linux-2.6.30-rc4.orig/arch/arm/mach-ep93xx/core.c	2009-04-30 05:48:16.000000000 +0100
> +++ linux-2.6.30-rc4/arch/arm/mach-ep93xx/core.c	2009-05-06 13:57:56.000000000 +0100
> @@ -537,6 +537,49 @@

[...]

> diff -urN linux-2.6.30-rc4.orig/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h linux-2.6.30-rc4/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h
> --- linux-2.6.30-rc4.orig/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h	2009-04-30 05:48:16.000000000 +0100
> +++ linux-2.6.30-rc4/arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h	2009-05-06 13:57:56.000000000 +0100
> @@ -78,6 +78,7 @@

[...]

> diff -urN linux-2.6.30-rc4.orig/arch/arm/mach-ep93xx/include/mach/platform.h linux-2.6.30-rc4/arch/arm/mach-ep93xx/include/mach/platform.h
> --- linux-2.6.30-rc4.orig/arch/arm/mach-ep93xx/include/mach/platform.h	2009-04-30 05:48:16.000000000 +0100
> +++ linux-2.6.30-rc4/arch/arm/mach-ep93xx/include/mach/platform.h	2009-05-06 13:57:56.000000000 +0100

[...]

    Please submit the above parts separately to linux-arm-kernel, as the
platform code doesn't belong to the driver.

> diff -urN linux-2.6.30-rc4.orig/drivers/ide/ep93xx-ide.c linux-2.6.30-rc4/drivers/ide/ep93xx-ide.c
> --- linux-2.6.30-rc4.orig/drivers/ide/ep93xx-ide.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.30-rc4/drivers/ide/ep93xx-ide.c	2009-05-06 14:48:17.000000000 +0100
> @@ -0,0 +1,530 @@
> +/*
> + * Support for Cirrus Logic's EP93xx (EP9312, EP9315) CPUs
> + * IDE host controller driver.
> + *
> + * Copyright (c) 2009, Joao Ramos <joao.ramos@inov.pt>
> + *                     INESC Inovacao (INOV)
> + *

[...]

> +
> +/* Macro for checking -IORDY line state */
> +#define	ep93xx_ide_check_iordy() ({	\
> +	u32 _reg = readl(IDE_REGISTER(IDECTRL));	\
> +	(_reg & IDECTRL_IORDY) ? 1 : 0;	\
> +})

    Make this an inline function, please.

> +
> +/*
> + * IDE Control Register bit fields
> + */
> +#define	IDECTRL_CS0N			0x00000001
> +#define	IDECTRL_CS1N			0x00000002
> +#define	IDECTRL_DA				0x0000001C
> +#define	IDECTRL_DIORN			0x00000020
> +#define	IDECTRL_DIOWN			0x00000040
> +#define	IDECTRL_DASPN			0x00000080
> +#define	IDECTRL_DMARQ			0x00000100
> +#define	IDECTRL_INTRQ			0x00000200
> +#define	IDECTRL_IORDY			0x00000400
> +
> +/*
> + * IDE Configuration Register bit fields
> + */
> +#define	IDECFG_IDEEN			0x00000001
> +#define	IDECFG_PIO				0x00000002
> +#define	IDECFG_MDMA				0x00000004
> +#define	IDECFG_UDMA				0x00000008
> +#define	IDECFG_PIO_MODE_0		0x00000000
> +#define	IDECFG_PIO_MODE_1		0x00000010
> +#define	IDECFG_PIO_MODE_2		0x00000020
> +#define	IDECFG_PIO_MODE_3		0x00000030
> +#define	IDECFG_PIO_MODE_4		0x00000040
> +#define	IDECFG_MDMA_MODE_0		0x00000000
> +#define	IDECFG_MDMA_MODE_1		0x00000010
> +#define	IDECFG_MDMA_MODE_2		0x00000020
> +#define	IDECFG_UDMA_MODE_0		0x00000000
> +#define	IDECFG_UDMA_MODE_1		0x00000010
> +#define	IDECFG_UDMA_MODE_2		0x00000020
> +#define	IDECFG_UDMA_MODE_3		0x00000030
> +#define	IDECFG_UDMA_MODE_4		0x00000040
> +#define	IDECFG_WST				0x00000300
> +
> +/*
> + * IDE Interface register map default state
> + * (shutdown)
> + */
> +static void ep93xx_ide_clean_regs(unsigned long base)
> +{
> +	/* disable IDE interface initially */
> +	writel((IDECTRL_CS0N | IDECTRL_CS1N | IDECTRL_DIORN |
> +			IDECTRL_DIOWN), IDE_REGISTER(IDECTRL));
> +
> +	/* clear IDE registers */
> +	writel(0, IDE_REGISTER(IDECFG));
> +	writel(0, IDE_REGISTER(IDEMDMAOP));
> +	writel(0, IDE_REGISTER(IDEUDMAOP));
> +	writel(0, IDE_REGISTER(IDEDATAOUT));
> +	writel(0, IDE_REGISTER(IDEDATAIN));
> +	writel(0, IDE_REGISTER(IDEMDMADATAOUT));
> +	writel(0, IDE_REGISTER(IDEMDMADATAIN));
> +	writel(0, IDE_REGISTER(IDEUDMADATAOUT));
> +	writel(0, IDE_REGISTER(IDEUDMADATAIN));
> +	writel(0, IDE_REGISTER(IDEUDMADEBUG));
> +}
> +
> +/*
> + * EP93xx IDE PIO low-level hardware initialization routine
> + */
> +static void ep93xx_ide_init_hwif(ide_hwif_t *hwif)
> +{
> +	unsigned long base = hwif->config_data;
> +
> +	/* enforce reset state */
> +	ep93xx_ide_clean_regs(base);
> +
> +	/* set gpio port E, G and H for IDE */
> +	ep93xx_ide_on_gpio(1);

    Shouldn't this be done in the platform code instead?

> +
> +	/*
> +	 * configure IDE interface:
> +	 *   - IDE Master Enable
> +	 *   - Polled IO Operation
> +	 *   - PIO Mode 4 (16.67 MBps)
> +	 *   - 1 Wait State (10 ns)
> +	 */
> +	writel(IDECFG_IDEEN | IDECFG_PIO | IDECFG_PIO_MODE_4 |
> +	       ((1 << 8) & IDECFG_WST), IDE_REGISTER(IDECFG));
> +}
> +
> +static u8 ep93xx_ide_readb(unsigned long base, unsigned long addr)
> +{
> +	u32 reg;
> +
> +	reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
> +	    IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +
> +	reg &= ~IDECTRL_DIORN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(70);
> +
> +	while (!ep93xx_ide_check_iordy())
> +		cpu_relax();
> +
> +	reg |= IDECTRL_DIORN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +
> +	return readl(IDE_REGISTER(IDEDATAIN));

    Hey, how this even works (if the data doesn't get latched somehow)?! You
should read the register right *before* the deassertion of -DIORx! The
minimum data hold time is only 5 ns and the data lines will be tristated
within 30 ns maximum...

[...]

> +static u16 ep93xx_ide_readw(unsigned long base, unsigned long addr)
> +{
> +	u32 reg;
> +
> +	reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
> +	    IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +
> +	reg &= ~IDECTRL_DIORN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(70);
> +
> +	while (!ep93xx_ide_check_iordy())
> +		cpu_relax();
> +
> +	reg |= IDECTRL_DIORN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +
> +	return readl(IDE_REGISTER(IDEDATAIN));
> +}

    I don't see any difference between ep93xx_ide_read[bw](), so why don't
you use a single function?

> +static void
> +ep93xx_ide_writeb(unsigned long base, u8 value, unsigned long addr)
> +{
> +	u32 reg;
> +
> +	reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
> +	    IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +
> +	writel(value, IDE_REGISTER(IDEDATAOUT));

    Hum, do you know at which moments this controller starts/stops driving
data lines on the IDE bus? After DIOWx- assertion/deassertion?

> +
> +	reg &= ~IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(70);
> +
> +	while (!ep93xx_ide_check_iordy())
> +		cpu_relax();
> +
> +	reg |= IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +}
> +

[...]

> +
> +static void
> +ep93xx_ide_writew(unsigned long base, u16 value, unsigned long addr)
> +{
> +	u32 reg;
> +
> +	reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
> +	    IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +
> +	writel(value, IDE_REGISTER(IDEDATAOUT));
> +
> +	reg &= ~IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(70);
> +
> +	while (!ep93xx_ide_check_iordy())
> +		cpu_relax();
> +
> +	reg |= IDECTRL_DIOWN;
> +	writel(reg, IDE_REGISTER(IDECTRL));
> +	ndelay(25);
> +}

    The same question: why we need both ep93xx_ide_write[bw]()?

> +
> +static void
> +ep93xx_ide_readsw(unsigned long base, unsigned long addr, void *buf,
> +		      unsigned int len)
> +{
> +	u16 *data = (u16 *) buf;
> +
> +	for (; len; len--)

    IMHO, while (len--) fits better...

> +		*data++ = cpu_to_le16(ep93xx_ide_readw(base, addr));
> +}
> +

> +
> +/*
> + * EP93xx IDE PIO Transport Operations
> + */
> +
> +static void ep93xx_ide_exec_command(struct hwif_s *hwif, u8 cmd)
> +{
> +	ep93xx_ide_writeb(hwif->config_data, cmd,
> +			      hwif->io_ports.command_addr);

    It's preferrable if you do not insert unnecessary spaces in the
multiline statements:

	ep93xx_ide_writeb(hwif->config_data, cmd,
			  hwif->io_ports.command_addr);

    This also looks prettier. :-)

> +static void
> +ep93xx_ide_tf_read(ide_drive_t *drive, struct ide_taskfile *tf, u8 valid)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	struct ide_io_ports *io_ports = &hwif->io_ports;
> +
> +	if (valid & IDE_VALID_ERROR)
> +		tf->error =
> +		    ep93xx_ide_readb(hwif->config_data,

> +					 io_ports->feature_addr);

    Again, tabs are strongly preferred over spaces (and carrying the
statement over to the next line where it's not necessary):

		tf->error = ep93xx_ide_readb(hwif->config_data,
					     io_ports->feature_addr);

> +static int __devinit ep93xx_ide_probe(struct platform_device *pdev)
> +{
> +	int retval;
> +	int	irq;

    Stray tab?

[...]

> +	retval = ide_host_add(&ep93xx_ide_port_info, hws, &host);
> +	if (retval) {
> +		dev_err(&pdev->dev,
> +			"unable to add EP93xx IDE host controller!\n");

    s/add/register/, s/host controller/driver/

> +		goto fail_unmap;
> +	}
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	dev_info(&pdev->dev, "EP93xx IDE host controller driver initialized\n");
> +
> +	return 0;
> +
> +fail_unmap:
> +	iounmap(ide_base);
> +fail_release_mem:
> +	release_mem_region(mem_res->start, mem_res->end - mem_res->start + 1);
> +	return retval;
> +}
> +
> +static int __devexit ep93xx_ide_remove(struct platform_device *pdev)
> +{
> +	struct resource *mem_res;
> +	struct ide_host *host = pdev->dev.driver_data;
> +
> +	/* IDE interface reset state */

    Punctuation missing...

> +	ep93xx_ide_clean_regs(host->ports[0]->config_data);
> +
> +	/* restore back GPIO port E, G and H for GPIO use */
> +	ep93xx_ide_on_gpio(0);
> +
> +	ide_host_remove(host);
> +
> +	iounmap((void __iomem *)(host->ports[0]->config_data));
> +
> +	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(mem_res->start, mem_res->end - mem_res->start + 1);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ep93xx_ide_driver = {
> +	.probe = ep93xx_ide_probe,
> +	.remove = __exit_p(ep93xx_ide_remove),
> +	.driver = {
> +		   .name = MODULE_NAME,
> +		   .owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init ep93xx_ide_init(void)
> +{
> +	return platform_driver_register(&ep93xx_ide_driver);

    Since this is not a hotplug driver, you can save some memory on making 
ep93xx_ide_probe() __init -- using platform_driver_probe() here instead of 
platform_driver_register() and *not* initializing the 'probe' field of the 
'struct platform_driver'.

MBR, Sergei