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@xxxxxxx>
---
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@xxxxxxx>
+ * 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
--
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
