Re: [PATCH] Palmchip BK3710 IDE driver
On Tuesday 05 February 2008, Anton Salnikov wrote:
I've tested the driver on 2.6.24.
I've made the changes you asked for. But when I tested them on
arm/mach-davinci
Thanks!
architecture on the current Linus' git there was link error undefined
reference
to symbol ide_hwif_setup_dma().
However on x86_64 architecture it compiles well without any warnings with
respect to link error of undefined clk_get() clk_enable() clk_get_rate(),
which
are not present in this architecture.
ide_hwif_setup_dma() comes from setup-pci.c which depends on BLK_DEV_IDEPCI.
palm_bk3710 host driver selects BLK_DEV_IDEDMA_PCI which should also select
BLK_DEV_IDEPCI but PCI doesn't even seem to be supported by this ARM platform.
I (hopefully) workarounded the issue by adding an additional #ifdef to
linux/ide.h but it is a unmaintanable and ugly hack:
Index: b/include/linux/ide.h
===
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1014,7 +1014,8 @@ extern int __ide_pci_register_driver(str
void ide_pci_setup_ports(struct pci_dev *, const struct ide_port_info *, int,
u8 *);
void ide_setup_pci_noise(struct pci_dev *, const struct ide_port_info *);
-#ifdef CONFIG_BLK_DEV_IDEDMA_PCI
+/* FIXME: palm_bk3710 uses BLK_DEV_IDEDMA_PCI without BLK_DEV_IDEPCI! */
+#if defined(CONFIG_BLK_DEV_IDEPCI) defined(CONFIG_BLK_DEV_IDEDMA_PCI)
void ide_hwif_setup_dma(ide_hwif_t *, const struct ide_port_info *);
#else
static inline void ide_hwif_setup_dma(ide_hwif_t *hwif,
Anton/Sergei: please look into fixing it properly - we probably just need to
to have a new config option (CONFIG_IDE_SFF_DMA sounds neat) to be selected by
both BLK_DEV_{IDEDMA_PCI,PALMCHIP_BK3710} and to replace BLK_DEV_IDEDMA_PCI
#ifdefs in ide-dma.c.
I've just noticed that there is no cable detection for UDMA modes UDMA2?
For the present controller in documentation: Cable Select (CSEL) -
unsupported
IDE controller signal. This signal is not necessary because an 80-conductor
cable must be used with the DM644x.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
Minor issue: the original patch description got lost somehow so I just used
the one from the previous version.
---
drivers/ide/Kconfig |9
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 420
++
drivers/ide/ide-proc.c|1
include/linux/ide.h |2
5 files changed, 432 insertions(+), 1 deletion(-)
Index: 2.6.25.ide/drivers/ide/Kconfig
===
--- 2.6.25.ide.orig/drivers/ide/Kconfig
+++ 2.6.25.ide/drivers/ide/Kconfig
@@ -1009,6 +1009,15 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ tristate Palmchip bk3710 IDE controller support
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
+
+
config BLK_DEV_MPC8xx_IDE
tristate MPC8xx IDE support
depends on 8xx (LWMON || IVMS8 || IVML24 || TQM8xxL) IDE=y
BLK_DEV_IDE=y !PPC_MERGE
Index: 2.6.25.ide/drivers/ide/arm/Makefile
===
--- 2.6.25.ide.orig/drivers/ide/arm/Makefile
+++ 2.6.25.ide/drivers/ide/arm/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_BLK_DEV_IDE_ICSIDE) += icside.o
obj-$(CONFIG_BLK_DEV_IDE_RAPIDE) += rapide.o
obj-$(CONFIG_BLK_DEV_IDE_BAST) += bast-ide.o
+obj-$(CONFIG_BLK_DEV_PALMCHIP_BK3710)+= palm_bk3710.o
ifeq ($(CONFIG_IDE_ARM), m)
obj-m += ide_arm.o
Index: 2.6.25.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.25.ide/drivers/ide/arm/palm_bk3710.c
[...]
+/*static inline u8 hwif_read8(u32 base, u32 reg)
+{
+ return readb(base + reg);
+}
+
+static inline u16 hwif_read16(u32 base, u32 reg)
+{
+ return readw(base + reg);
+}
+
+static inline void hwif_write16(u32 base, u16 val, u32 reg)
+{
+ writew(val, base + reg);
+}
+
+static inline u32 hwif_read32(u32 base, u32 reg)
+{
+ return readl(base + reg);
+}
+
+static inline void hwif_write32(u32 base, u32 val, u32 reg)
+{
+ writel(val, base + reg);
+}*/
I removed this chunk while merging the patch (no need for a dead code)
also added Reviewed-by: tags crediting Alan Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Hi,
On Friday 25 January 2008, Anton Salnikov wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Unfortunately some changes merged after 2.6.24 broke the build:
drivers/ide/arm/palm_bk3710.c: In function ‘palm_bk3710_set_dma_mode’:
drivers/ide/arm/palm_bk3710.c:243: error: ‘struct hwif_s’ has no member named
‘dma_master’
drivers/ide/arm/palm_bk3710.c: In function ‘palm_bk3710_set_pio_mode’:
drivers/ide/arm/palm_bk3710.c:259: error: ‘struct hwif_s’ has no member named
‘dma_master’
[ -dma_master is gone, -dma_base should be used instead ]
drivers/ide/arm/palm_bk3710.c: In function ‘palm_bk3710_probe’:
drivers/ide/arm/palm_bk3710.c:389: error: too many arguments to function
‘ide_register_hw’
[ 'initializing' argument is gone, just removing it is OK ]
drivers/ide/arm/palm_bk3710.c:404: error: too many arguments to function
‘ide_setup_dma’
[ 'num_ports' argument is gone, just removing it is OK ]
Please re-sync the patch with the current Linus' git tree.
There are also few less critical issues left...
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
drivers/ide/Kconfig |8
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 424
++
drivers/ide/ide-proc.c|1
include/linux/ide.h |2
5 files changed, 435 insertions(+), 1 deletion(-)
Index: 2.6.24.ide/drivers/ide/Kconfig
===
--- 2.6.24.ide.orig/drivers/ide/Kconfig
+++ 2.6.24.ide/drivers/ide/Kconfig
@@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ bool Palmchip bk3710 IDE controller support
'tristate'
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
+
config BLK_DEV_MPC8xx_IDE
bool MPC8xx IDE support
depends on 8xx (LWMON || IVMS8 || IVML24 || TQM8xxL) IDE=y
BLK_DEV_IDE=y !PPC_MERGE
Index: 2.6.24.ide/drivers/ide/arm/Makefile
===
--- 2.6.24.ide.orig/drivers/ide/arm/Makefile
+++ 2.6.24.ide/drivers/ide/arm/Makefile
[...]
+static inline u8 hwif_read8(u32 base, u32 reg)
+{
+ return readb(base + reg);
+}
+
+static inline void hwif_write8(u32 base, u8 val, u32 reg)
+{
+ writeb(val, base + reg);
+}
+
+static inline u16 hwif_read16(u32 base, u32 reg)
+{
+ return readw(base + reg);
+}
+
+static inline void hwif_write16(u32 base, u16 val, u32 reg)
+{
+ writew(val, base + reg);
+}
+
+static inline u32 hwif_read32(u32 base, u32 reg)
+{
+ return readl(base + reg);
+}
+
+static inline void hwif_write32(u32 base, u32 val, u32 reg)
+{
+ writel(val, base + reg);
+}
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_read8’:
drivers/ide/arm/palm_bk3710.c:93: warning: passing argument 1 of ‘readb’ makes
pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_write8’:
drivers/ide/arm/palm_bk3710.c:98: warning: passing argument 2 of ‘writeb’ makes
pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_read16’:
drivers/ide/arm/palm_bk3710.c:103: warning: passing argument 1 of ‘readw’ makes
pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_write16’:
drivers/ide/arm/palm_bk3710.c:108: warning: passing argument 2 of ‘writew’
makes pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_read32’:
drivers/ide/arm/palm_bk3710.c:113: warning: passing argument 1 of ‘readl’ makes
pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_write32’:
drivers/ide/arm/palm_bk3710.c:118: warning: passing argument 2 of ‘writel’
makes pointer from integer without a cast
'base' should be of 'void __iomem *' type for read*/write* ops
Besides: because the driver supports multiple controllers now these wrappers
doesn't provide any value and just obfuscate the code (as noticed by Alan).
Please remove them.
+static void palm_bk3710_setudmamode(u32 base, unsigned int dev,
'base' should be of 'void __iomem *' type for read*/*write ops
[ casting from u32 can be done in the caller ]
+ unsigned int mode)
+{
+ u8 tenv, trp, t0;
+ u32 val32;
+ u16 val16;
+
+ /* DMA Data Setup */
+ t0 = (palm_bk3710_udmatimings[mode].cycletime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+ tenv = (20 + ide_palm_clk - 1) / ide_palm_clk - 1;
+ trp =
Re: [PATCH] Palmchip BK3710 IDE driver
Needless parens all over the code like this... Also, you could have made it either 2 (getting rid of |= operator) or 4 lines (read/write, =, and |=). mode |= dev 4; - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Hello.
Anton Salnikov wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
Acked-by: Sergei Shtylyov [EMAIL PROTECTED]
Index: 2.6.24.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24.ide/drivers/ide/arm/palm_bk3710.c
+static void palm_bk3710_setudmamode(u32 base, unsigned int dev,
+ unsigned int mode)
+{
+ u8 tenv, trp, t0;
+ u32 val32;
+ u16 val16;
+
+ /* DMA Data Setup */
+ t0 = (palm_bk3710_udmatimings[mode].cycletime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+ tenv = (20 + ide_palm_clk - 1) / ide_palm_clk - 1;
+ trp = (palm_bk3710_udmatimings[mode].rptime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+
+ /* udmatim Register */
+ val16 = hwif_read16(base, BK3710_UDMATIM) (dev ? 0xFF0F : 0xFFF0);
+ val16 |= (mode (dev ? 4 : 0));
Needless parens all over the code like this...
Also, you could have made it either 2 (getting rid of |= operator) or 4
lines (read/write, =, and |=).
MBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Palmchip BK3710 IDE driver
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
drivers/ide/Kconfig |8
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 424 ++
drivers/ide/ide-proc.c|1
include/linux/ide.h |2
5 files changed, 435 insertions(+), 1 deletion(-)
Index: 2.6.24.ide/drivers/ide/Kconfig
===
--- 2.6.24.ide.orig/drivers/ide/Kconfig
+++ 2.6.24.ide/drivers/ide/Kconfig
@@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ bool Palmchip bk3710 IDE controller support
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
+
config BLK_DEV_MPC8xx_IDE
bool MPC8xx IDE support
depends on 8xx (LWMON || IVMS8 || IVML24 || TQM8xxL) IDE=y
BLK_DEV_IDE=y !PPC_MERGE
Index: 2.6.24.ide/drivers/ide/arm/Makefile
===
--- 2.6.24.ide.orig/drivers/ide/arm/Makefile
+++ 2.6.24.ide/drivers/ide/arm/Makefile
@@ -2,5 +2,6 @@
obj-$(CONFIG_BLK_DEV_IDE_ICSIDE) += icside.o
obj-$(CONFIG_BLK_DEV_IDE_RAPIDE) += rapide.o
obj-$(CONFIG_BLK_DEV_IDE_BAST) += bast-ide.o
+obj-$(CONFIG_BLK_DEV_PALMCHIP_BK3710) += palm_bk3710.o
EXTRA_CFLAGS := -Idrivers/ide
Index: 2.6.24.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,424 @@
+/*
+ * Palmchip bk3710 IDE controller
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software, Inc., [EMAIL PROTECTED]
+ *
+ *
+ *
+ * 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/types.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/ioport.h
+#include linux/hdreg.h
+#include linux/ide.h
+#include linux/delay.h
+#include linux/init.h
+#include linux/clk.h
+#include linux/platform_device.h
+
+/* Offset of the primary interface registers */
+#define IDE_PALM_ATA_PRI_REG_OFFSET 0x1F0
+
+/* Primary Control Offset */
+#define IDE_PALM_ATA_PRI_CTL_OFFSET 0x3F6
+
+/*
+ * PalmChip 3710 IDE Controller UDMA timing structure Definition
+ */
+struct palm_bk3710_udmatiming {
+ unsigned int rptime;/* Ready to pause time */
+ unsigned int cycletime; /* Cycle Time */
+};
+
+#define BK3710_BMICP 0x00
+#define BK3710_BMISP 0x02
+#define BK3710_BMIDTP 0x04
+#define BK3710_BMICS 0x08
+#define BK3710_BMISS 0x0A
+#define BK3710_BMIDTS 0x0C
+#define BK3710_IDETIMP 0x40
+#define BK3710_IDETIMS 0x42
+#define BK3710_SIDETIM 0x44
+#define BK3710_SLEWCTL 0x45
+#define BK3710_IDESTATUS 0x47
+#define BK3710_UDMACTL 0x48
+#define BK3710_UDMATIM 0x4A
+#define BK3710_MISCCTL 0x50
+#define BK3710_REGSTB 0x54
+#define BK3710_REGRCVR 0x58
+#define BK3710_DATSTB 0x5C
+#define BK3710_DATRCVR 0x60
+#define BK3710_DMASTB 0x64
+#define BK3710_DMARCVR 0x68
+#define BK3710_UDMASTB 0x6C
+#define BK3710_UDMATRP 0x70
+#define BK3710_UDMAENV 0x74
+#define BK3710_IORDYTMP0x78
+#define BK3710_IORDYTMS0x7C
+
+#include ../ide-timing.h
+
+static long ide_palm_clk;
+
+static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
+ {160, 240}, /* UDMA Mode 0 */
+ {125, 160}, /* UDMA Mode 1 */
+ {100, 120}, /* UDMA Mode 2 */
+ {100, 90}, /* UDMA Mode 3 */
+ {85, 60},
[PATCH] Palmchip BK3710 IDE driver
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
drivers/ide/Kconfig |8
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 428 ++
drivers/ide/ide-proc.c|1
include/linux/ide.h |2
5 files changed, 439 insertions(+), 1 deletion(-)
Index: 2.6.24-rc8.ide/drivers/ide/Kconfig
===
--- 2.6.24-rc8.ide.orig/drivers/ide/Kconfig
+++ 2.6.24-rc8.ide/drivers/ide/Kconfig
@@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ bool Palmchip bk3710 IDE controller support
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
+
config BLK_DEV_MPC8xx_IDE
bool MPC8xx IDE support
depends on 8xx (LWMON || IVMS8 || IVML24 || TQM8xxL) IDE=y
BLK_DEV_IDE=y !PPC_MERGE
Index: 2.6.24-rc8.ide/drivers/ide/arm/Makefile
===
--- 2.6.24-rc8.ide.orig/drivers/ide/arm/Makefile
+++ 2.6.24-rc8.ide/drivers/ide/arm/Makefile
@@ -2,5 +2,6 @@
obj-$(CONFIG_BLK_DEV_IDE_ICSIDE) += icside.o
obj-$(CONFIG_BLK_DEV_IDE_RAPIDE) += rapide.o
obj-$(CONFIG_BLK_DEV_IDE_BAST) += bast-ide.o
+obj-$(CONFIG_BLK_DEV_PALMCHIP_BK3710) += palm_bk3710.o
EXTRA_CFLAGS := -Idrivers/ide
Index: 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,428 @@
+/*
+ * Palmchip bk3710 IDE controller
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software, Inc., [EMAIL PROTECTED]
+ *
+ *
+ *
+ * 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/types.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/ioport.h
+#include linux/hdreg.h
+#include linux/ide.h
+#include linux/delay.h
+#include linux/init.h
+#include linux/clk.h
+#include linux/platform_device.h
+
+/* Offset of the primary interface registers */
+#define IDE_PALM_ATA_PRI_REG_OFFSET 0x1F0
+
+/* Primary Control Offset */
+#define IDE_PALM_ATA_PRI_CTL_OFFSET 0x3F6
+
+/*
+ * PalmChip 3710 IDE Controller UDMA timing structure Definition
+ */
+struct palm_bk3710_udmatiming {
+ unsigned int rptime;/* Ready to pause time */
+ unsigned int cycletime; /* Cycle Time */
+};
+
+#define BK3710_BMICP 0x00
+#define BK3710_BMISP 0x02
+#define BK3710_BMIDTP 0x04
+#define BK3710_BMICS 0x08
+#define BK3710_BMISS 0x0A
+#define BK3710_BMIDTS 0x0C
+#define BK3710_IDETIMP 0x40
+#define BK3710_IDETIMS 0x42
+#define BK3710_SIDETIM 0x44
+#define BK3710_SLEWCTL 0x45
+#define BK3710_IDESTATUS 0x47
+#define BK3710_UDMACTL 0x48
+#define BK3710_UDMATIM 0x4A
+#define BK3710_MISCCTL 0x50
+#define BK3710_REGSTB 0x54
+#define BK3710_REGRCVR 0x58
+#define BK3710_DATSTB 0x5C
+#define BK3710_DATRCVR 0x60
+#define BK3710_DMASTB 0x64
+#define BK3710_DMARCVR 0x68
+#define BK3710_UDMASTB 0x6C
+#define BK3710_UDMATRP 0x70
+#define BK3710_UDMAENV 0x74
+#define BK3710_IORDYTMP0x78
+#define BK3710_IORDYTMS0x7C
+
+#include ../ide-timing.h
+
+static long ide_palm_clk;
+
+static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
+ {160, 240}, /* UDMA Mode 0 */
+ {125, 160}, /* UDMA Mode 1 */
+ {100, 120}, /* UDMA Mode 2 */
+ {100, 90}, /* UDMA Mode
Re: [PATCH] Palmchip BK3710 IDE driver
Anton Salnikov wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
Index: 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,428 @@
+static inline u8 hwif_read8(u32 base, u32 reg)
+{
+ return readb(base + reg);
+}
+
+static inline void hwif_write8(u32 base, u8 val, u32 reg)
+{
+ writeb(val, base + reg);
+}
+
+static inline u16 hwif_read16(u32 base, u32 reg)
+{
+ return readw(base + reg);
+}
+
+static inline void hwif_write16(u32 base, u16 val, u32 reg)
+{
+ writew(val, base + reg);
+}
+
+static inline u32 hwif_read32(u32 base, u32 reg)
+{
+ return readl(base + reg);
+}
+
+static inline void hwif_write32(u32 base, u32 val, u32 reg)
+{
+ writel(val, base + reg);
+}
Hm, I don't see why you need those accessors but well...
+static void palm_bk3710_setudmamode(u32 base, unsigned int dev,
+ unsigned int mode)
+{
+ u8 tenv, trp, t0;
+ u32 val32;
+ u16 val16;
+
+ /* DMA Data Setup */
+ t0 = (palm_bk3710_udmatimings[mode].cycletime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+ tenv = (20 + ide_palm_clk - 1) / ide_palm_clk - 1;
+ trp = (palm_bk3710_udmatimings[mode].rptime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+
+ /* udmatim Register */
+ val16 = hwif_read16(base, BK3710_UDMATIM) (dev ? 0xFF0F : 0xFFF0);
+ val16 |= (mode ((!dev) ? 0 : 4));
Why not (dev ? 4 : 0) like the rest?
+ hwif_write16(base, val16, BK3710_UDMATIM);
+
+ /* udmastb Ultra DMA Access Strobe Width */
+ val32 = hwif_read32(base, BK3710_UDMASTB) (0xFF (dev ? 0 : 8));
+ val32 |= (t0 (dev ? 8 : 0));
+ hwif_write32(base, val32, BK3710_UDMASTB);
+
+ /* udmatrp Ultra DMA Ready to Pause Time */
+ val32 = hwif_read32(base, BK3710_UDMATRP) (0xFF (dev ? 0 : 8));
+ val32 |= (trp (dev ? 8 : 0));
+ hwif_write32(base, val32, BK3710_UDMATRP);
+
+ /* udmaenv Ultra DMA envelop Time */
+ val32 = hwif_read32(base, BK3710_UDMAENV) (0xFF (dev ? 0 : 8));
+ val32 |= (tenv (dev ? 8 : 0));
+ hwif_write32(base, val32, BK3710_UDMAENV);
+
+ /* Enable UDMA for Device */
+ val16 = hwif_read16(base, BK3710_UDMACTL) | (1 dev);
+ hwif_write16(base, val16, BK3710_UDMACTL);
+}
+
+static void palm_bk3710_setdmamode(u32 base, unsigned int dev,
+ unsigned short min_cycle,
+ unsigned int mode)
+{
+ u8 td, tkw, t0;
+ u32 val32;
+ u16 val16;
+
+ unsigned cycletime = max_t(int, ide_timing_find_mode(mode)-cycle,
+ min_cycle);
Hm, why integer comparison and then unsigned maximum?
+
+ /* DMA Data Setup */
+ t0 = (cycletime + ide_palm_clk - 1) / ide_palm_clk;
+ td = (ide_timing_find_mode(mode)-active + ide_palm_clk - 1)
+ / ide_palm_clk;
Couldn't you call ide_timing_find_mode() only once?
[...]
+static int __devinit palm_bk3710_probe(struct platform_device *pdev)
+{
+ hw_regs_t ide_ctlr_info;
+ int index = 0;
+ int pribase;
+ struct clk *clkp;
+ struct resource *mem, *irq;
+ ide_hwif_t *hwif;
+ u32 base;
+ int ret;
+
+ clkp = clk_get(NULL, IDECLK);
+ if (IS_ERR(clkp))
+ return -ENODEV;
+
+ ideclkp = clkp;
+ clk_enable(ideclkp);
+ ide_palm_clk = clk_get_rate(ideclkp)/10;
+ ide_palm_clk = (1/ide_palm_clk) + 1;
+ /* Register the IDE interface with Linux ATA Interface */
+ memset(ide_ctlr_info, 0, sizeof(ide_ctlr_info));
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (mem == NULL) {
+ printk(KERN_INFO failed to get memory region resource\n);
This deserves KERN_ERR, not KERN_INFO.
+ return -ENODEV;
+ }
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (irq == NULL) {
+ printk(KERN_INFO failed to get IRQ resource\n);
This too...
+ return -ENODEV;
+ }
+
+ base = mem-start;
+
+ /* Configure the Palm Chip controller */
+ palm_bk3710_chipinit(base);
+
+ pribase = mem-start + IDE_PALM_ATA_PRI_REG_OFFSET;
+ for (index = 0; index IDE_NR_PORTS - 2; index++)
+ ide_ctlr_info.io_ports[index] = pribase + index;
+ ide_ctlr_info.io_ports[IDE_CONTROL_OFFSET] = mem-start +
+ IDE_PALM_ATA_PRI_CTL_OFFSET;
+ ide_ctlr_info.irq = irq-start;
+
Re: [PATCH] Palmchip BK3710 IDE driver
Hello. Alan Cox wrote: Why you chose to use ioread32() and iowrite32() if your device is strictly memory mapped? Those functions add some overhead, and boil down to readl() and There are distinct portability advantages but you shouldn't mix ioread32/iowrite32 with ioremap as that isn't guaranteed to work. Hm, pci_iomap() boils down to ioremap(), so readl/writel does fine and fixes up the driver. Which brings up another issue with the latest patch: Anton, where do you call ioremap() or something alike to get the virtual address of the registers? MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
On Wed, 23 Jan 2008 16:38:06 +0300 Sergei Shtylyov [EMAIL PROTECTED] wrote: Hello. Alan Cox wrote: Why you chose to use ioread32() and iowrite32() if your device is strictly memory mapped? Those functions add some overhead, and boil down to readl() and There are distinct portability advantages but you shouldn't mix ioread32/iowrite32 with ioremap as that isn't guaranteed to work. Hm, pci_iomap() boils down to ioremap(), so For most cases on existing hardware. Do not rely on that in future. Alan - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
+static inline u32 ioread(u32 reg)
+{
+ return ioread32(base + reg);
+}
+
+static inline void iowrite(u32 val, u32 reg)
+{
+ iowrite32(val, base + reg);
+}
Why not just use ioread32/iowrite32 directly ?
Because this increases readability and does not affect functionality at all
Otherwise this looks way way better.
Alan
Anton
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
On Tue, 22 Jan 2008 15:11:37 +0300
Anton Salnikov [EMAIL PROTECTED] wrote:
+static inline u32 ioread(u32 reg)
+{
+ return ioread32(base + reg);
+}
+
+static inline void iowrite(u32 val, u32 reg)
+{
+ iowrite32(val, base + reg);
+}
Why not just use ioread32/iowrite32 directly ?
Because this increases readability and does not affect functionality at all
There I would disagree. It means that if you ever have a system with two
of these controllers in future it will break as base is a static for the
entire driver.
Easy enough to fix with a simple
static inline u32 hwif_ioread32(hwif, u32 reg) { ...
hwif-dma_base ...}
version ?
Alan
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Anton Salnikov wrote:
+static inline u32 ioread(u32 reg)
+{
+ return ioread32(base + reg);
+}
+
+static inline void iowrite(u32 val, u32 reg)
+{
+ iowrite32(val, base + reg);
+}
Why not just use ioread32/iowrite32 directly ?
Because this increases readability and does not affect functionality at all
Disagree.
It confuses the namespace, /decreasing/ readability. Anyone _but_ you
reading this code will be confused by a non-standard API named similarly
to an existing kernel API.
Jeff
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Hello.
Anton Salnikov wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Use ide_setup_dma() since BLK_DEV_PALMCHIP_BK3710 selects BLK_DEV_IDEDMA_PCI
So I deleted exports from ide-dma.c
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
Index: 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,434 @@
+static long ide_palm_clk;
+static u32 base;
This base will be in hwif-dma_master, so you can aslo use it this way if
you make your 'hwif' pointer static instead. Although as this
+
+static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
+ {160, 240}, /* UDMA Mode 0 */
+ {125, 160}, /* UDMA Mode 1 */
+ {100, 120}, /* UDMA Mode 2 */
+ {100, 90}, /* UDMA Mode 3 */
+ {85, 60}, /* UDMA Mode 4 */
+};
+
+static struct clk *ideclkp;
+
+static inline u32 ioread(u32 reg)
+{
+ return ioread32(base + reg);
+}
+
+static inline void iowrite(u32 val, u32 reg)
+{
+ iowrite32(val, base + reg);
+}
Why you chose to use ioread32() and iowrite32() if your device is strictly
memory mapped? Those functions add some overhead, and boil down to readl() and
writel() anyway. IIRC, they should only be used if you have a hardware
registers accessible from both I/O and memory space and driver supports both
types of mapping (e.g., I/O mapped for the older hardware, memory mapped for
the newer one)...
+static void palm_bk3710_setudmamode(unsigned int dev, unsigned int mode)
+{
+ u8 tenv, trp, t0;
+ u32 val;
+
+ /* DMA Data Setup */
+ t0 = (palm_bk3710_udmatimings[mode].cycletime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+ tenv = (20 + ide_palm_clk - 1) / ide_palm_clk - 1;
+ trp = (palm_bk3710_udmatimings[mode].rptime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+
+ /* udmatim Register */
+ val = ioread(BK3710_UDMATIM) ((!dev) ? 0xFFF0 : 0xFF0F);
Hm, isn't it shorter:
val = ioread(BK3710_UDMATIM) (dev ? 0xFF0F : 0xFFF0);
+ iowrite(val, BK3710_UDMATIM);
Why not skip intermediate write and re-read? Does hardware require
clearing the fields before setting new values by some weird chance?
+ val = ioread(BK3710_UDMATIM) | (mode ((!dev) ? 0 : 4));
You actually need to shift left by 0 or 8, so shift count should be 3, not
4 since 1 4 == 16! I'm suggesting:
val = ioread(BK3710_UDMATIM) | (mode (dev ? 3 : 0));
+ iowrite(val, BK3710_UDMATIM);
+
+ /* udmastb Ultra DMA Access Strobe Width */
+ val = ioread(BK3710_UDMASTB) (0xFF ((!dev) 4));
I'm suggesting:
val = ioread(BK3710_UDMASTB) (0xFF00 (dev 3));
+ iowrite(val, BK3710_UDMASTB);
+ val = ioread(BK3710_UDMASTB) | (t0 (dev 4));
+ iowrite(val, BK3710_UDMASTB);
+
+ /* udmatrp Ultra DMA Ready to Pause Time */
+ val = ioread(BK3710_UDMATRP) (0xFF ((!dev) 4));
+ iowrite(val, BK3710_UDMATRP);
+ val = ioread(BK3710_UDMATRP) | (trp (dev 4));
+ iowrite(val, BK3710_UDMATRP);
+
+ /* udmaenv Ultra DMA envelop Time */
+ val = ioread(BK3710_UDMAENV) (0xFF ((!dev) 4));
+ iowrite(val, BK3710_UDMAENV);
+ val = ioread(BK3710_UDMAENV) | (tenv (dev 4));
+ iowrite(val, BK3710_UDMAENV);
3 ISO 4 everywhere...
+
+ /* Enable UDMA for Device */
+ val = ioread(BK3710_UDMACTL) | (1 dev);
+ iowrite(val, BK3710_UDMACTL);
+}
+
+static void palm_bk3710_setdmamode(unsigned int dev, unsigned int cycletime,
+ unsigned int mode)
+{
+ u8 td, tkw, t0;
+ u32 val;
+
+ if (cycletime ide_timing[mode].cycle)
+ cycletime = ide_timing[mode].cycle;
I don't think this check is necessary as you're passing either
ide_timing[mode].cycle or drive-id-eide_dma_min -- whichever is greater...
+
+ /* DMA Data Setup */
+ t0 = (cycletime + ide_palm_clk - 1) / ide_palm_clk;
+ td = (ide_timing_find_mode(mode)-active + ide_palm_clk - 1)
You're not passing a mode suitable to ide_timing_find_mode() here, but
index into ide_timing[] table -- almost the same thing that this call is
supposed to yield... :-/
+ / ide_palm_clk;
+ tkw = t0 - td - 1;
+ td -= 1;
+
+ val = ioread(BK3710_DMASTB) (0xFF ((!dev) 4));
+ iowrite(val, BK3710_DMASTB);
+ val = ioread(BK3710_DMASTB) | (td (dev 4));
+ iowrite(val, BK3710_DMASTB);
+
+ val = ioread(BK3710_DMARCVR) (0xFF ((!dev) 4));
+ iowrite(val, BK3710_DMARCVR);
+ val =
Re: [PATCH] Palmchip BK3710 IDE driver
Why you chose to use ioread32() and iowrite32() if your device is strictly memory mapped? Those functions add some overhead, and boil down to readl() and There are distinct portability advantages but you shouldn't mix ioread32/iowrite32 with ioremap as that isn't guaranteed to work. readl/writel does fine and fixes up the driver. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Palmchip BK3710 IDE driver
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Use ide_setup_dma() since BLK_DEV_PALMCHIP_BK3710 selects BLK_DEV_IDEDMA_PCI
So I deleted exports from ide-dma.c
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
drivers/ide/Kconfig |8
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 434 ++
drivers/ide/ide-proc.c|1
include/linux/ide.h |2
5 files changed, 445 insertions(+), 1 deletion(-)
Index: 2.6.24-rc8.ide/drivers/ide/Kconfig
===
--- 2.6.24-rc8.ide.orig/drivers/ide/Kconfig
+++ 2.6.24-rc8.ide/drivers/ide/Kconfig
@@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ bool Palmchip bk3710 IDE controller support
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
+
config BLK_DEV_MPC8xx_IDE
bool MPC8xx IDE support
depends on 8xx (LWMON || IVMS8 || IVML24 || TQM8xxL) IDE=y
BLK_DEV_IDE=y !PPC_MERGE
Index: 2.6.24-rc8.ide/drivers/ide/arm/Makefile
===
--- 2.6.24-rc8.ide.orig/drivers/ide/arm/Makefile
+++ 2.6.24-rc8.ide/drivers/ide/arm/Makefile
@@ -2,5 +2,6 @@
obj-$(CONFIG_BLK_DEV_IDE_ICSIDE) += icside.o
obj-$(CONFIG_BLK_DEV_IDE_RAPIDE) += rapide.o
obj-$(CONFIG_BLK_DEV_IDE_BAST) += bast-ide.o
+obj-$(CONFIG_BLK_DEV_PALMCHIP_BK3710) += palm_bk3710.o
EXTRA_CFLAGS := -Idrivers/ide
Index: 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,434 @@
+/*
+ * Palmchip bk3710 IDE controller
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software, Inc., [EMAIL PROTECTED]
+ *
+ *
+ *
+ * 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/types.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/ioport.h
+#include linux/hdreg.h
+#include linux/ide.h
+#include linux/delay.h
+#include linux/init.h
+#include linux/clk.h
+#include linux/platform_device.h
+
+/* Offset of the primary interface registers */
+#define IDE_PALM_ATA_PRI_REG_OFFSET 0x1F0
+
+/* Primary Control Offset */
+#define IDE_PALM_ATA_PRI_CTL_OFFSET 0x3F6
+
+/*
+ * PalmChip 3710 IDE Controller UDMA timing structure Definition
+ */
+struct palm_bk3710_udmatiming {
+ unsigned int rptime;/* Ready to pause time */
+ unsigned int cycletime; /* Cycle Time */
+};
+
+#define BK3710_BMICP 0x00
+#define BK3710_BMISP 0x02
+#define BK3710_BMIDTP 0x04
+#define BK3710_BMICS 0x08
+#define BK3710_BMISS 0x0A
+#define BK3710_BMIDTS 0x0C
+#define BK3710_IDETIMP 0x40
+#define BK3710_IDETIMS 0x42
+#define BK3710_SIDETIM 0x44
+#define BK3710_SLEWCTL 0x45
+#define BK3710_IDESTATUS 0x47
+#define BK3710_UDMACTL 0x48
+#define BK3710_UDMATIM 0x4A
+#define BK3710_MISCCTL 0x50
+#define BK3710_REGSTB 0x54
+#define BK3710_REGRCVR 0x58
+#define BK3710_DATSTB 0x5C
+#define BK3710_DATRCVR 0x60
+#define BK3710_DMASTB 0x64
+#define BK3710_DMARCVR 0x68
+#define BK3710_UDMASTB 0x6C
+#define BK3710_UDMATRP 0x70
+#define BK3710_UDMAENV 0x74
+#define BK3710_IORDYTMP0x78
+#define BK3710_IORDYTMS0x7C
+
+#include ../ide-timing.h
+
+static long ide_palm_clk;
+static u32 base;
+
+static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
+ {160, 240}, /* UDMA Mode 0 */
+
Re: [PATCH] Palmchip BK3710 IDE driver
+static inline u32 ioread(u32 reg)
+{
+ return ioread32(base + reg);
+}
+
+static inline void iowrite(u32 val, u32 reg)
+{
+ iowrite32(val, base + reg);
+}
Why not just use ioread32/iowrite32 directly ?
Otherwise this looks way way better.
Alan
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
+ /* udmatim Register */ + palm_bk3710_base-config.udmatim = 0xFFF0; + palm_bk3710_base-config.udmatim |= level; Direct memory access to I/O space - should be using read/write functions Sigh, I was anticipationg that somebody would say that... :-) Well yes - its not very nice code and it isn't really upstream to scratch. Why? ATA/PI says in the note to the table Register transfer to/from device: 5 Mode shall be selected no higher than the *highest* mode supported by the *slowest* device. You are correct, ignore that. You don't touch the timing for the other device. It won't -- we can *not* call ide_setup_dma() which fills them out as this is not a PCI chip. Gak. Alan - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Alan Cox wrote: It won't -- we can *not* call ide_setup_dma() which fills them out as this is not a PCI chip. Gak. Or maybe we still can -- with hwif-mmio set? AFAIR (from the 2006 discussion) Bart said it's *only* for PCI devices... Alan MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Hello.
Alan Cox wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
New drivers should really be going at least parallel into drivers/ata and
legacy IDE.
Alas, it's unlikely that we can spend more time on developing libata
equivalent which we don't need at this point.
+struct palm_bk3710_ideconfigregs {
+ unsigned short idetimp __attribute__((packed));
+ unsigned short idetims __attribute__((packed));
+ unsigned char sidetim;
+ unsigned short slewctl __attribute__((packed));
+ unsigned char idestatus;
+ unsigned short udmactl __attribute__((packed));
+ unsigned short udmatim __attribute__((packed));
+ unsigned char rsvd0[4];
+ unsigned int miscctl __attribute__((packed));
+ unsigned int regstb __attribute__((packed));
NAK - the size of an unsigned int in the kernel isn't defined. All the
packed stuff is also horribly platform dependant. True the driver is ARM
only currently but that doesn't make it a good idea.
Can't you use defined offsets ?
+struct palm_bk3710_ideregs {
+ struct palm_bk3710_dmaengineregs dmaengine;
So why are only some packed ?
+ /* udmatim Register */
+ palm_bk3710_base-config.udmatim = 0xFFF0;
+ palm_bk3710_base-config.udmatim |= level;
Direct memory access to I/O space - should be using read/write functions
Sigh, I was anticipationg that somebody would say that... :-)
+ if (mate mate-present) {
+ u8 mode2 = ide_get_best_pio_mode(mate, 255, 4);
If the pair device is present but not in the best mode it can do this
will be wrong surely ?
Why? ATA/PI says in the note to the table Register transfer to/from
device:
5 Mode shall be selected no higher than the *highest* mode supported by the
*slowest* device.
Some other drivers are already using the same code (I'm not even sure it's
needed here -- never saw the chip documentation).
+ if (!request_region(mem-start, 8, palm_bk3710_hwif-name)) {
+ printk(KERN_ERR Error, ports in use.\n);
+ return -EBUSY;
+ }
+
+ palm_bk3710_hwif-dmatable_cpu = dma_alloc_coherent(
+ NULL,
+ PRD_ENTRIES * PRD_BYTES,
+ palm_bk3710_hwif-dmatable_dma,
+ GFP_ATOMIC);
+
+ if (!palm_bk3710_hwif-dmatable_cpu) {
Leaks the reserved region
My bad -- I've managed to overlook this while reviewing.
+ printk(KERN_ERR Error, unable to allocate DMA table.\n);
+ return -ENOMEM;
+ }
-static void ide_dma_exec_cmd(ide_drive_t *drive, u8 command)
+void ide_dma_exec_cmd(ide_drive_t *drive, u8 command)
{
/* issue cmd to drive */
ide_execute_command(drive, command, ide_dma_intr, 2*WAIT_CMD,
dma_timer_expiry);
}
+EXPORT_SYMBOL(ide_dma_exec_cmd);
These are not needed the NULL default request will fill them in for you
It won't -- we can *not* call ide_setup_dma() which fills them out as this
is not a PCI chip.
MBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
On Friday 18 January 2008, Sergei Shtylyov wrote: Alan Cox wrote: It won't -- we can *not* call ide_setup_dma() which fills them out as this is not a PCI chip. Gak. Or maybe we still can -- with hwif-mmio set? AFAIR (from the 2006 discussion) Bart said it's *only* for PCI devices... Well, since BLK_DEV_PALMCHIP_BK3710 selects BLK_DEV_IDEDMA_PCI it should be possible to call ide_setup_dma() in this driver... Bart - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Palmchip BK3710 IDE driver
Hi,
On Thursday 17 January 2008, Anton Salnikov wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
Thanks, overall it looks good but the way in which controller registers are
accessed needs to be reworked according to Alan's comments before this driver
can be accepted in the mainline.
I had to export two functions (ide_dma_exec_cmd and __ide_dma_test_irq) from
driver/ide/ide-dma.c to get rid of copying them.
Please make the exports _GPL.
Also some minor comments below.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
drivers/ide/Kconfig |8
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 486
++
drivers/ide/ide-dma.c |6
drivers/ide/ide-proc.c|1
include/linux/ide.h |4
6 files changed, 503 insertions(+), 3 deletions(-)
Index: 2.6.24-rc7.ide/drivers/ide/Kconfig
===
--- 2.6.24-rc7.ide.orig/drivers/ide/Kconfig
+++ 2.6.24-rc7.ide/drivers/ide/Kconfig
@@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ bool Palmchip bk3710 IDE controller support
tristate?
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
[...]
Index: 2.6.24-rc7.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24-rc7.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,486 @@
+/*
+ * Palmchip bk3710 IDE controller
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software, Inc., [EMAIL PROTECTED]
[...]
+ Modifications:
+ ver. 1.0: Oct 2005, Swaminathan S
+ -
I would prefer revision history to be removed if possible (we keep changelogs
in git tree, also the date doesn't match with Copyrights notice).
[...]
+static ide_hwif_t *palm_bk3710_hwif;
palm_bk3710_hwif is only accessed in palm_bk3710_probe() so may be as well
moved there (+ renamed to hwif to match usual naming used by other drivers)
+static struct palm_bk3710_ideregs __iomem *palm_bk3710_base;
+static long ide_palm_clk;
+
+static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
+ {160, 240}, /* UDMA Mode 0 */
+ {125, 160}, /* UDMA Mode 1 */
+ {100, 120}, /* UDMA Mode 2 */
+ {100, 90}, /* UDMA Mode 3 */
+ {85, 60}, /* UDMA Mode 4 */
+ {85, 40} /* UDMA Mode 5 */
+};
Hmmm, but palm_bk3710_probe() limits max UDMA to UDMA4...?
+static struct clk *ideclkp;
+
+static void palm_bk3710_setudmamode(unsigned int dev, unsigned int level)
'level' is a bit confusing name for UDMA mode number
+{
+ char ide_tenv, ide_trp, ide_t0;
- char - u8
- ide_ prefix may as well be dropped
+ /* DMA Data Setup */
+ ide_t0 = (palm_bk3710_udmatimings[level].cycletime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+ ide_tenv = (20 + ide_palm_clk - 1) / ide_palm_clk - 1;
+ ide_trp = (palm_bk3710_udmatimings[level].rptime + ide_palm_clk - 1)
+ / ide_palm_clk - 1;
+
+
+ if (!dev) {
Since this code needs to be recasted anyway it would be a nice cleanup
to merge '!dev' and 'dev' cases.
+ /* setup master device parameters */
+
+ /* udmatim Register */
+ palm_bk3710_base-config.udmatim = 0xFFF0;
+ palm_bk3710_base-config.udmatim |= level;
+ /* udmastb Ultra DMA Access Strobe Width */
+ palm_bk3710_base-config.udmastb = 0xFF00;
+ palm_bk3710_base-config.udmastb |= ide_t0;
+ /* udmatrp Ultra DMA Ready to Pause Time */
+ palm_bk3710_base-config.udmatrp = 0xFF00;
+ palm_bk3710_base-config.udmatrp |= ide_trp;
+ /* udmaenv Ultra DMA envelop Time */
+ palm_bk3710_base-config.udmaenv = 0xFF00;
+ palm_bk3710_base-config.udmaenv |= ide_tenv;
+ /* Enable UDMA for Device 0 */
+ palm_bk3710_base-config.udmactl |= 1;
+ } else {
+ /* setup slave device parameters */
+
+ /* udmatim Register */
+ palm_bk3710_base-config.udmatim = 0xFF0F;
+ palm_bk3710_base-config.udmatim |= (level 4);
+ /* udmastb Ultra DMA Access Strobe Width */
+ palm_bk3710_base-config.udmastb = 0xFF;
+ palm_bk3710_base-config.udmastb |= (ide_t0 8);
+ /* udmatrp Ultra
Re: [PATCH] Palmchip BK3710 IDE driver
Hello. Anton Salnikov wrote: This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8. The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes. Supports interface to compact Flash (CF) configured in True-IDE mode. I had to export two functions (ide_dma_exec_cmd and __ide_dma_test_irq) from driver/ide/ide-dma.c to get rid of copying them. Signed-off-by: Anton Salnikov [EMAIL PROTECTED] Acked-by: Sergei Shtylyov [EMAIL PROTECTED] MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Palmchip BK3710 IDE driver
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
I had to export two functions (ide_dma_exec_cmd and __ide_dma_test_irq) from
driver/ide/ide-dma.c to get rid of copying them.
Signed-off-by: Anton Salnikov [EMAIL PROTECTED]
---
drivers/ide/Kconfig |8
drivers/ide/arm/Makefile |1
drivers/ide/arm/palm_bk3710.c | 486 ++
drivers/ide/ide-dma.c |6
drivers/ide/ide-proc.c|1
include/linux/ide.h |4
6 files changed, 503 insertions(+), 3 deletions(-)
Index: 2.6.24-rc7.ide/drivers/ide/Kconfig
===
--- 2.6.24-rc7.ide.orig/drivers/ide/Kconfig
+++ 2.6.24-rc7.ide/drivers/ide/Kconfig
@@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
normally be on; disable it only if you are running a custom hard
drive subsystem through an expansion card.
+config BLK_DEV_PALMCHIP_BK3710
+ bool Palmchip bk3710 IDE controller support
+ depends on ARCH_DAVINCI
+ select BLK_DEV_IDEDMA_PCI
+ help
+ Say Y here if you want to support the onchip IDE controller on the
+ TI DaVinci SoC
+
config BLK_DEV_MPC8xx_IDE
bool MPC8xx IDE support
depends on 8xx (LWMON || IVMS8 || IVML24 || TQM8xxL) IDE=y
BLK_DEV_IDE=y !PPC_MERGE
Index: 2.6.24-rc7.ide/drivers/ide/arm/Makefile
===
--- 2.6.24-rc7.ide.orig/drivers/ide/arm/Makefile
+++ 2.6.24-rc7.ide/drivers/ide/arm/Makefile
@@ -2,5 +2,6 @@
obj-$(CONFIG_BLK_DEV_IDE_ICSIDE) += icside.o
obj-$(CONFIG_BLK_DEV_IDE_RAPIDE) += rapide.o
obj-$(CONFIG_BLK_DEV_IDE_BAST) += bast-ide.o
+obj-$(CONFIG_BLK_DEV_PALMCHIP_BK3710) += palm_bk3710.o
EXTRA_CFLAGS := -Idrivers/ide
Index: 2.6.24-rc7.ide/drivers/ide/arm/palm_bk3710.c
===
--- /dev/null
+++ 2.6.24-rc7.ide/drivers/ide/arm/palm_bk3710.c
@@ -0,0 +1,486 @@
+/*
+ * Palmchip bk3710 IDE controller
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software, Inc., [EMAIL PROTECTED]
+ *
+ *
+ *
+ * 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.
+ *
+ Modifications:
+ ver. 1.0: Oct 2005, Swaminathan S
+ -
+ *
+ */
+
+#include linux/types.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/ioport.h
+#include linux/hdreg.h
+#include linux/ide.h
+#include linux/delay.h
+#include linux/init.h
+#include linux/clk.h
+#include linux/platform_device.h
+
+/* Offset of the primary interface registers */
+#define IDE_PALM_ATA_PRI_REG_OFFSET 0x1F0
+
+/* Primary Control Offset */
+#define IDE_PALM_ATA_PRI_CTL_OFFSET 0x3F6
+
+/*
+ * PalmChip 3710 IDE Controller UDMA timing structure Definition
+ */
+struct palm_bk3710_udmatiming {
+ unsigned int rptime;/* Ready to pause time */
+ unsigned int cycletime; /* Cycle Time */
+};
+
+/*
+ * Register Layout Structure for DmaEngine
+ */
+struct palm_bk3710_dmaengineregs {
+ unsigned short bmicp;
+ unsigned short bmisp;
+ unsigned int bmidtp;
+ unsigned short bmics;
+ unsigned short bmiss;
+ unsigned int bmidts;
+};
+
+/*
+ * Register Layout Structure for Config
+ */
+struct palm_bk3710_ideconfigregs {
+ unsigned short idetimp __attribute__((packed));
+ unsigned short idetims __attribute__((packed));
+ unsigned char sidetim;
+ unsigned short slewctl __attribute__((packed));
+ unsigned char idestatus;
+ unsigned short udmactl __attribute__((packed));
+ unsigned short udmatim __attribute__((packed));
+ unsigned char rsvd0[4];
+ unsigned int miscctl __attribute__((packed));
+ unsigned int regstb __attribute__((packed));
+ unsigned int regrcvr __attribute__((packed));
+ unsigned int datstb __attribute__((packed));
+ unsigned int datrcvr __attribute__((packed));
+ unsigned int dmastb
Re: [PATCH] Palmchip BK3710 IDE driver
On Thu, 17 Jan 2008 21:50:56 +0300
Anton Salnikov [EMAIL PROTECTED] wrote:
This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
Supports interface to compact Flash (CF) configured in True-IDE mode.
New drivers should really be going at least parallel into drivers/ata and
legacy IDE.
+struct palm_bk3710_ideconfigregs {
+ unsigned short idetimp __attribute__((packed));
+ unsigned short idetims __attribute__((packed));
+ unsigned char sidetim;
+ unsigned short slewctl __attribute__((packed));
+ unsigned char idestatus;
+ unsigned short udmactl __attribute__((packed));
+ unsigned short udmatim __attribute__((packed));
+ unsigned char rsvd0[4];
+ unsigned int miscctl __attribute__((packed));
+ unsigned int regstb __attribute__((packed));
NAK - the size of an unsigned int in the kernel isn't defined. All the
packed stuff is also horribly platform dependant. True the driver is ARM
only currently but that doesn't make it a good idea.
Can't you use defined offsets ?
+struct palm_bk3710_ideregs {
+ struct palm_bk3710_dmaengineregs dmaengine;
So why are only some packed ?
+ /* udmatim Register */
+ palm_bk3710_base-config.udmatim = 0xFFF0;
+ palm_bk3710_base-config.udmatim |= level;
Direct memory access to I/O space - should be using read/write functions
+ if (mate mate-present) {
+ u8 mode2 = ide_get_best_pio_mode(mate, 255, 4);
If the pair device is present but not in the best mode it can do this
will be wrong surely ?
+ if (!request_region(mem-start, 8, palm_bk3710_hwif-name)) {
+ printk(KERN_ERR Error, ports in use.\n);
+ return -EBUSY;
+ }
+
+ palm_bk3710_hwif-dmatable_cpu = dma_alloc_coherent(
+ NULL,
+ PRD_ENTRIES * PRD_BYTES,
+ palm_bk3710_hwif-dmatable_dma,
+ GFP_ATOMIC);
+
+ if (!palm_bk3710_hwif-dmatable_cpu) {
Leaks the reserved region
+ printk(KERN_ERR Error, unable to allocate DMA table.\n);
+ return -ENOMEM;
+ }
-static void ide_dma_exec_cmd(ide_drive_t *drive, u8 command)
+void ide_dma_exec_cmd(ide_drive_t *drive, u8 command)
{
/* issue cmd to drive */
ide_execute_command(drive, command, ide_dma_intr, 2*WAIT_CMD,
dma_timer_expiry);
}
+EXPORT_SYMBOL(ide_dma_exec_cmd);
These are not needed the NULL default request will fill them in for you
Alan
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
