Bartlomiej Zolnierkiewicz escreveu:
On Wednesday 13 May 2009 16:18:39 João Ramos wrote:
Hi,
In attachment I send the revised patch for the EP93xx CPU IDE controller
(well, not entirely, only the IDE bit, I letf out the platform code as
suggested), changed and fixed according to all the comments you made before.
This driver depends on the previous patches I sent (ide_drive_data patch
and set_pio_mode patch).
Please make your suggestions.
I still need to go over previous patches but here are some comments against
ep93xx-ide.c itself...
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-13 14:47:02.000000000 +0100
@@ -0,0 +1,528 @@
+/*
+ * Support for Cirrus Logic's EP93xx (EP9312, EP9315) CPUs
+ * IDE host controller driver.
+ *
+ * Copyright (c) 2009, Joao Ramos <joao.ramos@xxxxxxx>
+ * INESC Inovacao (INOV)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/ioport.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/ide.h>
+#include <linux/delay.h>
+
+#define MODULE_NAME "ep93xx-ide"
+
+/*
+ * Configuration and usage of the IDE device is made through the
+ * IDE Interface Register Map (EP93xx User's Guide, Section 27).
+ *
+ * This section holds common registers and flag definitions for
+ * that interface.
+ */
+
+/*
+ * IDE Register offset
+ */
+#define IDECTRL 0x00
+#define IDECFG 0x04
+#define IDEMDMAOP 0x08
+#define IDEUDMAOP 0x0C
+#define IDEDATAOUT 0x10
+#define IDEDATAIN 0x14
+#define IDEMDMADATAOUT 0x18
+#define IDEMDMADATAIN 0x1C
+#define IDEUDMADATAOUT 0x20
+#define IDEUDMADATAIN 0x24
+#define IDEUDMASTS 0x28
+#define IDEUDMADEBUG 0x2C
+#define IDEUDMAWRBUFSTS 0x30
+#define IDEUDMARDBUFSTS 0x34
+
+/*
+ * 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
+
+/*
+ * readl/writel helpers to access internal registers using
+ * an ioremapped cookie and the specified IDE register offset
+ */
+
+static inline u32 ep93xx_readl(unsigned long base, u8 offset)
+{
+ return readl((void __iomem *)(base + offset));
+}
+
+static inline void ep93xx_writel(u32 value, unsigned long base, u8 offset)
+{
+ writel(value, (void __iomem *)(base + offset));
+}
Hmm, what do we need these wrappers for exactly?
Please remove them.
I just added those to increase code readability, so that I wouldn't have
to do a '(void __iomem *)(base + offset)' in every readl/writel call.
But I can remove it, no problem.
+/*
+ * Check whether IORDY is asserted or not.
+ */
+static inline int ep93xx_ide_check_iordy(unsigned long base)
+{
+ u32 reg = ep93xx_readl(base, IDECTRL);
+
+ return (reg & IDECTRL_IORDY) ? 1 : 0;
+}
+
+/*
+ * IDE Interface register map default state
+ * (shutdown)
+ */
+static void ep93xx_ide_clean_regs(unsigned long base)
+{
+ /* disable IDE interface initially */
+ ep93xx_writel(IDECTRL_CS0N | IDECTRL_CS1N | IDECTRL_DIORN |
+ IDECTRL_DIOWN, base, IDECTRL);
+
+ /* clear IDE registers */
+ ep93xx_writel(0, base, IDECFG);
+ ep93xx_writel(0, base, IDEMDMAOP);
+ ep93xx_writel(0, base, IDEUDMAOP);
+ ep93xx_writel(0, base, IDEDATAOUT);
+ ep93xx_writel(0, base, IDEDATAIN);
+ ep93xx_writel(0, base, IDEMDMADATAOUT);
+ ep93xx_writel(0, base, IDEMDMADATAIN);
+ ep93xx_writel(0, base, IDEUDMADATAOUT);
+ ep93xx_writel(0, base, IDEUDMADATAIN);
+ ep93xx_writel(0, base, IDEUDMADEBUG);
+}
+
+static u16 ep93xx_ide_read(unsigned long base, unsigned long addr,
+ struct ide_timing *timing)
+{
+ u32 reg;
+
+ reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
+ IDECTRL_DIOWN;
+ ep93xx_writel(reg, base, IDECTRL);
+ ndelay(timing->setup);
+
+ reg &= ~IDECTRL_DIORN;
+ ep93xx_writel(reg, base, IDECTRL);
+ ndelay(timing->active);
+
+ while (!ep93xx_ide_check_iordy(base))
+ cpu_relax();
+
+ reg |= IDECTRL_DIORN;
+ ep93xx_writel(reg, base, IDECTRL);
+ ndelay(timing->recover);
+
+ return ep93xx_readl(base, IDEDATAIN);
+}
+
+static void ep93xx_ide_write(unsigned long base, u16 value,
+ unsigned long addr, struct ide_timing *timing)
+{
+ u32 reg;
+
+ reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
+ IDECTRL_DIOWN;
+ ep93xx_writel(reg, base, IDECTRL);
+ ndelay(timing->setup);
+
+ ep93xx_writel(value, base, IDEDATAOUT);
+
+ reg &= ~IDECTRL_DIOWN;
+ ep93xx_writel(reg, base, IDECTRL);
+ ndelay(timing->active);
+
+ while (!ep93xx_ide_check_iordy(base))
+ cpu_relax();
+
+ reg |= IDECTRL_DIOWN;
+ ep93xx_writel(reg, base, IDECTRL);
+ ndelay(timing->recover);
+}
+
+static void ep93xx_ide_readsw(unsigned long base, unsigned long addr,
+ void *buf, unsigned int len, struct ide_timing *timing)
+{
+ u16 *data = (u16 *) buf;
+
+ while (len--)
+ *data++ = cpu_to_le16(ep93xx_ide_read(base, addr, timing));
+}
+
+static void ep93xx_ide_writesw(unsigned long base, unsigned long addr,
+ void *buf, unsigned int len, struct ide_timing *timing)
+{
+ u16 *data = (u16 *) buf;
+
+ while (len--)
+ ep93xx_ide_write(base, le16_to_cpu(*data++), addr, timing);
+}
+
+/*
+ * EP93xx IDE PIO Transport Operations
+ */
+
+static void ep93xx_ide_exec_command(struct hwif_s *hwif, u8 cmd)
+{
+ ide_drive_t *drive = hwif->devices[0];
Actually I was suggesting something else:
in ->set_pio_mode do something like:
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;
if (pair)
struct ide_timing *pair_t = ide_get_drivedata(pair);
if (pair_t && pair_t->mode < t->mode)
cmd_t = pair_t;
}
ide_set_drivedata(drive, (void *)t);
ide_set_hwifdata(hwif, (void *)cmd_t);
so you can later use ide_get_hwifdata() in everything except
ep93xx_ide_readsw() and ep93xx_ide_write().
Ok.
It allows proper support of two devices on the port and doesn't access
hwif->devices directly.
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ ep93xx_ide_write(hwif->config_data, cmd, hwif->io_ports.command_addr, t);
+}
+
+static u8 ep93xx_ide_read_status(ide_hwif_t *hwif)
+{
+ ide_drive_t *drive = hwif->devices[0];
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ return ep93xx_ide_read(hwif->config_data, hwif->io_ports.status_addr, t);
+}
+
+static u8 ep93xx_ide_read_altstatus(ide_hwif_t *hwif)
+{
+ ide_drive_t *drive = hwif->devices[0];
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ return ep93xx_ide_read(hwif->config_data, hwif->io_ports.ctl_addr, t);
+}
+
+static void ep93xx_ide_write_devctl(ide_hwif_t *hwif, u8 ctl)
+{
+ ide_drive_t *drive = hwif->devices[0];
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ ep93xx_ide_write(hwif->config_data, ctl, hwif->io_ports.ctl_addr, t);
+}
+
+static void ep93xx_ide_dev_select(ide_drive_t *drive)
+{
+ ide_hwif_t *hwif = drive->hwif;
+ u8 select = drive->select | ATA_DEVICE_OBS;
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ ep93xx_ide_write(hwif->config_data, select, hwif->io_ports.device_addr, t);
+}
+
+static void
+ep93xx_ide_tf_load(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;
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ if (valid & IDE_VALID_FEATURE)
+ ep93xx_ide_write(hwif->config_data, tf->feature,
+ io_ports->feature_addr, t);
+ if (valid & IDE_VALID_NSECT)
+ ep93xx_ide_write(hwif->config_data, tf->nsect,
+ io_ports->nsect_addr, t);
+ if (valid & IDE_VALID_LBAL)
+ ep93xx_ide_write(hwif->config_data, tf->lbal,
+ io_ports->lbal_addr, t);
+ if (valid & IDE_VALID_LBAM)
+ ep93xx_ide_write(hwif->config_data, tf->lbam,
+ io_ports->lbam_addr, t);
+ if (valid & IDE_VALID_LBAH)
+ ep93xx_ide_write(hwif->config_data, tf->lbah,
+ io_ports->lbah_addr, t);
+ if (valid & IDE_VALID_DEVICE)
+ ep93xx_ide_write(hwif->config_data, tf->device,
+ io_ports->device_addr, t);
+}
+
+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;
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ if (valid & IDE_VALID_ERROR)
+ tf->error = ep93xx_ide_read(hwif->config_data,
+ io_ports->feature_addr, t);
+ if (valid & IDE_VALID_NSECT)
+ tf->nsect = ep93xx_ide_read(hwif->config_data,
+ io_ports->nsect_addr, t);
+ if (valid & IDE_VALID_LBAL)
+ tf->lbal = ep93xx_ide_read(hwif->config_data,
+ io_ports->lbal_addr, t);
+ if (valid & IDE_VALID_LBAM)
+ tf->lbam = ep93xx_ide_read(hwif->config_data,
+ io_ports->lbam_addr, t);
+ if (valid & IDE_VALID_LBAH)
+ tf->lbah = ep93xx_ide_read(hwif->config_data,
+ io_ports->lbah_addr, t);
+ if (valid & IDE_VALID_DEVICE)
+ tf->device = ep93xx_ide_read(hwif->config_data,
+ io_ports->device_addr, t);
+}
+
+static void ep93xx_ide_input_data(ide_drive_t *drive, struct ide_cmd *cmd,
+ void *buf, unsigned int len)
+{
+ ide_hwif_t *hwif = drive->hwif;
+ struct ide_io_ports *io_ports = &hwif->io_ports;
+ unsigned int words = (len + 1) >> 1;
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ ep93xx_ide_readsw(hwif->config_data, io_ports->data_addr, buf, words, t);
ep93xx_ide_readsw() is used only here so it makes sense to inline its
content here
Ok.
+}
+
+static void ep93xx_ide_output_data(ide_drive_t *drive, struct ide_cmd *cmd,
+ void *buf, unsigned int len)
+{
+ ide_hwif_t *hwif = drive->hwif;
+ struct ide_io_ports *io_ports = &hwif->io_ports;
+ unsigned int words = (len + 1) >> 1;
+ struct ide_timing *t = (struct ide_timing *) ide_get_drivedata(drive);
+
+ ep93xx_ide_writesw(hwif->config_data, io_ports->data_addr, buf, words, t);
ditto
+}
+
+static struct ide_tp_ops ep93xx_ide_tp_ops = {
+ .exec_command = ep93xx_ide_exec_command,
+ .read_status = ep93xx_ide_read_status,
+ .read_altstatus = ep93xx_ide_read_altstatus,
+ .write_devctl = ep93xx_ide_write_devctl,
+ .dev_select = ep93xx_ide_dev_select,
+ .tf_load = ep93xx_ide_tf_load,
+ .tf_read = ep93xx_ide_tf_read,
+ .input_data = ep93xx_ide_input_data,
+ .output_data = ep93xx_ide_output_data,
+};
Indentation:
.exec_command = ep93xx_ide_exec_command,
.read_status = ep93xx_ide_read_status,
...
+static void ep93xx_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+ struct ide_timing *timing;
+ u32 reg = IDECFG_IDEEN | IDECFG_PIO;
+
+ timing = ide_timing_find_mode(XFER_PIO_0 + pio);
+ /*
+ * store the adequate PIO mode timings in the ide_drive_t
+ * 'drive_data' field, to be used later by ep93xx_ide_{read,write}
+ */
+ ide_set_drivedata(drive, (void *)timing);
+
+ /* reconfigure IDE controller according to PIO mode */
+ switch (pio) {
+ case 4:
+ reg |= IDECFG_PIO_MODE_4 | ((1 << 8) & IDECFG_WST);
+ break;
+ case 3:
+ reg |= IDECFG_PIO_MODE_3 | ((1 << 8) & IDECFG_WST);
+ break;
+ case 2:
+ reg |= IDECFG_PIO_MODE_2 | ((2 << 8) & IDECFG_WST);
+ break;
+ case 1:
+ reg |= IDECFG_PIO_MODE_1 | ((2 << 8) & IDECFG_WST);
+ break;
+ case 0:
+ default:
+ reg |= IDECFG_PIO_MODE_0 | ((3 << 8) & IDECFG_WST);
+ break;
+ }
+ ep93xx_writel(reg, drive->hwif->config_data, IDECFG);
+}
+
+static struct ide_port_ops ep93xx_ide_port_ops = {
+ .set_pio_mode = ep93xx_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 |
+ IDE_HFLAG_NO_DMA | IDE_HFLAG_MMIO | IDE_HFLAG_NO_IO_32BIT |
+ IDE_HFLAG_NO_UNMASK_IRQS,
+ .pio_mask = ATA_PIO4,
+};
Indentation.
+static int __init ep93xx_ide_probe(struct platform_device *pdev)
+{
+ int retval;
+ int irq;
+ void __iomem *ide_base;
+ struct resource *mem_res;
+ hw_regs_t hw;
+ hw_regs_t *hws[] = { &hw, NULL, NULL, NULL };
+ struct ide_host *host;
+
+ mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem_res) {
+ dev_err(&pdev->dev,
+ "could not retrieve device memory resources!\n");
+ return -ENXIO;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev,
+ "could not retrieve device IRQ resource!\n");
+ return -ENXIO;
+ }
+
+ if (!request_mem_region
+ (mem_res->start, mem_res->end - mem_res->start + 1, MODULE_NAME)) {
+ dev_err(&pdev->dev, "could not request memory resources!\n");
+ return -EBUSY;
+ }
+
+ ide_base = ioremap(mem_res->start, mem_res->end - mem_res->start + 1);
+ if (!ide_base) {
+ dev_err(&pdev->dev, "could not map memory resources!\n");
+ retval = -ENOMEM;
+ goto fail_release_mem;
+ }
+
+ memset(&hw, 0, sizeof(hw));
+
+ /*
+ * fill in ide_io_ports structure.
+ * we use magic numbers 0x10 for io_addr and 0x0E for ctl_addr,
+ * hence the lowest 3 bits will give us the real address (ranging from
+ * 0 to 7) and the subsequent 2 bits will give us the CS1n and CS0n
+ * values:
+ * CS1n CS0n A2 A1 A0
+ * 1 0 x x x -> IO_ADDR (base 0x10)
+ * 0 1 x x x -> CTL_ADDR (base 0x0E)
+ */
+ ide_std_init_ports(&hw, 0x10, 0x0E);
Why not just setup the real addresses here instead of using
ide_std_init_ports()?
The IDE register's address are specified through bitfields in the EP93xx
IDECTRL register, as described in the above comment.
The 'workaround' in the addresses is just to ease manipulation of those
bitfields while writing the register.
I suppose I could write the register's address in the hw->io_ports so
that the value would be directly ORed to the IDECTRL register, if that
is what you suggest.
It seems that it would make driver simpler and get rid of >config_data use.
No. The config_data is used to store the ioremapped cookie of the IDE
registers base address. I need that address to access the CPUs IDE
registers.
There's no connection between this address (which maps to CPU's internal
memory, allowing to acess the IDE registers) and the actual IDE
addresses, which are defined through bitfields in the IDE control register.
Yeah, I know, weird IDE host controller... :-)
+ hw.irq = irq;
+ hw.chipset = ide_generic;
+ hw.dev = &pdev->dev;
+ hw.config = (unsigned long)ide_base;
+
+ /* enforce EP93xx IDE controller reset state */
+ ep93xx_ide_clean_regs(hw.config);
+
+ /* set gpio port E, G and H for IDE */
+ ep93xx_ide_aquire_gpios();
+
+ /*
+ * configure IDE interface:
+ * - IDE Master Enable
+ * - Polled IO Operation
+ * - PIO Mode 0 -> 3.33 MBps
+ * - 3 Wait States -> 30 ns
+ */
+ ep93xx_writel(IDECFG_IDEEN | IDECFG_PIO | IDECFG_PIO_MODE_0 |
+ ((3 << 8) & IDECFG_WST), hw.config, IDECFG);
+
+ retval = ide_host_add(&ep93xx_ide_port_info, hws, &host);
+ if (retval) {
+ dev_err(&pdev->dev,
+ "unable to register EP93xx IDE driver!\n");
+ 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;
+
+ /* reset state */
+ ep93xx_ide_clean_regs(host->ports[0]->config_data);
+
+ /* restore back GPIO port E, G and H for GPIO use */
+ ep93xx_ide_release_gpios();
+
+ ide_host_remove(host);
+
+ iounmap((void __iomem *)(host->ports[0]->config_data));
'host' access but 'host' is already gone...
IOW host->ports[0]->config_data needs to be cached in local variable
before ide_host_remove() is called...
Ups... You're right on this; I didn't spot this since my test platform
doesn't use this driver as a loadable/unloadable module.
I will fix that also.
Thanks for your comments.
Best regards,
João
Thanks,
Bart
--
************************************************************************
João Ramos <joao.ramos@xxxxxxx>
INOV INESC Inovação - ESTG Leiria
Escola Superior de Tecnologia e Gestão de Leiria
Edíficio C1, Campus 2
Morro do Lena, Alto do Vieiro
Leiria
2411-901 Leiria
Portugal
Tel: +351244843424
Fax: +351244843424
************************************************************************
--
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
