Hi,
[...]
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?
The patch currently supports only PIO Mode 4, as it is hardcoded into
the CPU's IDE configuration register:
writel(IDECFG_IDEEN | IDECFG_PIO | IDECFG_PIO_MODE_4 |
((1 << 8) & IDECFG_WST), IDE_REGISTER(IDECFG));
And also in the ide_port_info struct:
static struct ide_port_info ep93xx_ide_port_info = {
.name = MODULE_NAME,
.init_hwif = ep93xx_ide_init_hwif,
.tp_ops = &ep93xx_ide_tp_ops,
.host_flags = IDE_HFLAG_SINGLE | IDE_HFLAG_NO_DMA | IDE_HFLAG_MMIO |
IDE_HFLAG_NO_IO_32BIT | IDE_HFLAG_NO_UNMASK_IRQS,
.pio_mask = ATA_PIO4,
};
So you're saying I should support all PIO modes? If so, I would have to
make conditional code, checking perhaps a module param to sort which PIO
mode to use.
Also, the manual delays should also depend on the PIO mode selected...
It would be done either by a module param (defaulting to PIO Mode 4), or
through a kernel configuration variable...
[...]
Please submit the above parts separately to linux-arm-kernel, as the
platform code doesn't belong to the driver.
Ok.
+
+/* 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.
Ok.
[...]
+
+/*
+ * 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?
The idea is to make this driver loadable, as suggested earlier by Ryan
and Hartley.
The IDE pins are initially (and by default) set to GPIO function. If the
IDE driver is registered, through specific platform code or by loading
the module at runtime, then the IDE driver cares to configure the IDE
pins for IDE function, returning them to GPIO function once the driver
is unloaded.
I think this is the approach desired by the EP93xx maintainers, correct?
(Ryan? Hartley?)
[...]
+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...
Will be fixed.
[...]
+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?
The difference is only the return value, which casts to either u8 or u16
type.
Perhaps there's no need for two different functions, assuming the
high-level code will always cast down the variables to their correct
type (u8, u16).
+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?
I will look into that. I based this source code in the CPU's user guide,
which tips a correct procedure for reading/writing in PIO mode.
But I will check that, as I already had some trouble with the user's
guide...
+
+ 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]()?
Same answer as before. Perhaps there's no need for those.
+
+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...
Ok.
+ *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. :-)
Ok.
+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);
Ok.
+static int __devinit ep93xx_ide_probe(struct platform_device *pdev)
+{
+ int retval;
+ int irq;
Stray tab?
[...]
Will be fixed.
+ 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/
Ok.
[...]
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'.
I think Ryan and Hartley would like this driver to be
loadable/unloadable at runtime, as I pointed out earlier in this mail.
I can make the fixes about this, ensuring Ryan and Hartley will both
agree to them.
MBR, Sergei
Regards,
João
--
************************************************************************
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
