Hello.
João Ramos wrote:
Patch follows.
diff -urN linux-2.6.29.orig/arch/arm/mach-ep93xx/core.c
linux-2.6.29/arch/arm/mach-ep93xx/core.c
--- linux-2.6.29.orig/arch/arm/mach-ep93xx/core.c 2009-03-23
23:12:14.000000000 +0000
+++ linux-2.6.29/arch/arm/mach-ep93xx/core.c 2009-03-30
13:52:04.000000000 +0100
@@ -537,6 +537,51 @@
platform_device_register(&ep93xx_i2c_device);
}
+static struct resource ep93xx_ide_resources[] = {
+ [0] = {
Your code uses 4-space indentation instead of single-tab -- this
style is not acceptable. I'd advise to run the patch thru
scripts/checkpatch.pl to find and fix the coding style mistakes.
The problem is mainly my mailer settings: it deforms the patch output.
Perhaps next time I should send the patch as an attachment?
Well, if the IDE maintainer doesn't mind, I don't either. Just make sure
the attachment will have the MIME type of text/plain.
+/*
+ * IDE Interface register map default state
+ * (shutdown)
+ */
+static void
+ep93xx_ide_pio_clean_regs(unsigned long base)
+{
+ /* disable IDE interface initially, ensuring that no device is
selected */
+ writel((IDECTRL_CS0N | IDECTRL_CS1N), IDE_REGISTER(IDECTRL));
+
+ /* clear IDE registers */
+ writel(0, IDE_REGISTER(IDECFG));
Writing 0 to this register generates an invalid cycle on IDE bus if
I don't mistake (CS0/1 and DIOR/W all asserted), are you sure you want
this?
Zero (0) is the default value for the IDE Configuration Register, as
stated in EP93xx User's Guide (Section 27, page 11).
Besides, CS0/1 and DIOR/W lines are controlled through IDE Control
Register.
Sorry, I seem to have mixed them up...
In that case, perhaps it would be best to have it also set to it's
default value, deasserting DIOR/W as well:
writel((IDECTRL_CS0N | IDECTRL_CS1N | IDECTRL_DIORN | IDECTRL_DIOWN),
IDE_REGISTER(IDECTRL));
Yes, that's what I had in mind...
+/*
+ * EP93xx IDE PIO low-level hardware initialization routine
+ */
+static void
+ep93xx_ide_pio_init_hwif(ide_hwif_t *hwif)
+{
+ unsigned long base = hwif->config_data;
+
+ /* enforce reset state */
+ ep93xx_ide_pio_clean_regs(base);
+
+ /* set gpio port E, G and H for IDE */
+ ep93xx_ide_gpio_on_ide();
+
+ /*
+ * 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));
+}
+
+/*
+ * EP93xx IDE PIO low-level byte-read procedure.
+ */
+static u8
+ep93xx_ide_pio_readb(unsigned long base, unsigned long addr)
+{
+ u32 reg;
+
+ /*
+ * initial state: DIORn=1, DIOWn=1
+ * write address out
+ * select CS configuration
+ */
+ reg = ((addr & 0x07) << 2) | ((addr >> 3) & 0x03) | IDECTRL_DIORN |
+ IDECTRL_DIOWN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+
+ /*
+ * bring DIORn low
+ */
+ reg &= ~IDECTRL_DIORN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+
+ /*
+ * bring DIORn high
+ */
+ reg |= IDECTRL_DIORN;
+ writel(reg, IDE_REGISTER(IDECTRL));
+
+ /*
+ * read IDE Data Input Register
+ */
+ return (readl(IDE_REGISTER(IDEDATAIN)) & 0xFF);
Masking is pointless, it's achieved by truncation to 'u8'.
+}
I don't see what warrants minimum active/recovery time here. And I
don't see how you're supporitng PIO modes 3/4 as you're not checking
-IORDY.
So, you're saying I should check for the IORDY value at the beginning
and end of each transfer?
Only before deasserting -DIOx.
What exactly do you mean by 'warrant minimum active/recovery time'?
Should I insert manual delays to ensure timing issues?
Yes, since Cirrus seems to have left you with no option other than
bit-banging to emulate I/O cycles, you'll have to use delays...
[...]
+/*
+ * EP93xx IDE PIO low-level word buffer-read procedure
+ */
+static void
+ep93xx_ide_pio_readsw(unsigned long base, unsigned long addr, void
*buf,
+ unsigned int len)
+{
+ u16 *data = (u16 *)buf;
+
+ for (;len;len--)
Put spaces after ; please.
+ *data++ = ep93xx_ide_pio_readw(base, addr);
Is this little-endian only code? If not, it's not valid in the big
endian mode...
Should I instead use array-indexing code such as:
for (; len; len--, i++)
data[i] = ep93xx_ide_pio_readw(base, addr);
And the same for 'ep93xx_ide_pio_writesw()' function?
No, it won't change anything. You should use cpu_to_le16() -- unless
somehow ep93xx_ide_pio_readw() always returns little-endian data...
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
