Re: [PATCH 2/3] ide: add at91_ide driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

Stanislaw Gruszka wrote:

This is IDE host driver for AT91SAM9 Static Memory Controller with Compact
Flash True IDE Mode logic.

Driver have to switch 8/16 bit bus width when accessing Task Tile or Data
Register. Moreover some extra things need to be done when setting PIO mode.
Only PIO mode is used, hardware have no DMA support. If interrupt line is
connected through GPIO extra quirk is needed to cope with fake interrupts.

Signed-off-by: Stanislaw Gruszka <stf_xl@xxxxx>
[...]
diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index fb51f0e..6674b9b 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h
@@ -59,6 +59,18 @@ struct at91_cf_data {
 };
 extern void __init at91_add_device_cf(struct at91_cf_data *data);
+ /* Compact Flash True IDE mode */
+struct at91_ide_data {
+	u8	irq_pin;		/* the same meaning as for CF */

I again have to express my dislike about not passing IRQ the usual way. Also, see my comments to the platform code.

+	u8	det_pin;
+	u8	rst_pin;
+	u8	chipselect;
+	u8	flags;
+#define AT91_IDE_SWAP_A0_A2 0x01
+};
+
+extern void __init at91_add_device_ide(struct at91_ide_data *data);
+
  /* MMC / SD */
 struct at91_mmc_data {
 	u8		det_pin;	/* card detect IRQ */
diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index 3dad229..b11da5b 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -721,6 +721,11 @@ config BLK_DEV_IDE_TX4939
 	depends on SOC_TX4939
 	select BLK_DEV_IDEDMA_SFF
+config BLK_DEV_IDE_AT91
+	tristate "Atmel AT91 IDE support"

  Please be more specific -- you can't drive AT91RM9200 SMC.

+	depends on ARM && ARCH_AT91

  Please add "&& !ARCH_AT91RM9200". And maybe "&& !ARCH_AT91X40" too...

diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
new file mode 100644
index 0000000..3a1f7e0
--- /dev/null
+++ b/drivers/ide/at91_ide.c
@@ -0,0 +1,496 @@
+/*
+ * IDE host driver for AT91SAM9 Static Memory Controller

  Why not call the driver 'at91sam9_ide'?

+/*
+ * AT91 Static Memory Controller

 AT91SAM9.

maps Task File and Data Register
+ * at the same address. To distinguish access between these two

  It would have been strange if it did it otherwise...

+ * different bus data width is used: 8Bit for Task File, 16Bit for Data I/O
+ *
+ * After initialization we do 8/16 bit flipping (changes in SMC MODE register)
+ * only inside IDE callback routines which are serialized by IDE layer,
+ * so no additional locking needed.
+ */
+
+static void init_smc_mode(const u8 chipselect)
+{
+	at91_sys_write(AT91_SMC_MODE(chipselect), AT91_SMC_READMODE |
+						  AT91_SMC_WRITEMODE |
+						  AT91_SMC_BAT_SELECT |
+						  AT91_SMC_TDF_(0));

I'm not sure why are you fixing the dataflow timing again, this time to 0...

+}
+
+static inline void set_8bit_mode(const u8 chipselect)
+{
+	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
+	mode &= ~AT91_SMC_DBW;
+	mode |= AT91_SMC_DBW_8;
+	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	pdbg("%u %08lx\n", chipselect, mode);
+}
+
+static inline void set_16bit_mode(const u8 chipselect)
+{
+	unsigned long mode = at91_sys_read(AT91_SMC_MODE(chipselect));
+	mode &= ~AT91_SMC_DBW;
+	mode |= AT91_SMC_DBW_16;
+	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	pdbg("%u %08lx\n", chipselect, mode);
+}

I'd advice to make this single function because it looks like a code duplication too much.

+static void set_smc_timings(const u8 chipselect, const u16 cycle,
+			    const u16 setup, const u16 pulse,
+			    const u16 data_float, int use_iordy)

Have you considered using already present sam9_smc_configure() instead to avoid the code duplication (it needs to be exported though)?

+{
+	unsigned long mode;
+
+	/* setup timings in SMC */
+	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
+						   AT91_SMC_NCS_WRSETUP_(0) |
+						   AT91_SMC_NRDSETUP_(setup) |
+						   AT91_SMC_NCS_RDSETUP_(0));
+	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
+						   AT91_SMC_NCS_WRPULSE_(cycle) |
+						   AT91_SMC_NRDPULSE_(pulse) |
+						   AT91_SMC_NCS_RDPULSE_(cycle));
+	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
+						   AT91_SMC_NRDCYCLE_(cycle));
+
+	mode = at91_sys_read(AT91_SMC_MODE(chipselect));

Frankly speaking, I don't see why you're reading this register back if you already know what needs to be set there -- as you've done it in init_smc_mode().

+static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)
+{
+	u64 tmp = ns;
+
+	tmp *= mck_hz;
+	tmp += 1000*1000*1000 - 1; /* round up */
+	do_div(tmp, 1000*1000*1000);
+	return (unsigned int) tmp;
+}
+
+static void apply_timings(const u8 chipselect, const u8 pio,
+			  const struct ide_timing *timing, const u16 *ata_id)
+{
+	unsigned int t0, t1, t2, t6z, th;
+	unsigned int cycle, setup, pulse, data_float;
+	unsigned int mck_hz;
+	struct clk *mck;
+	int use_iordy;
[...]
+
+	use_iordy = 0;

  Can be done in initializer...

+	if (ata_id) {
+		if (ata_id_is_cfa(ata_id)) {
+			if (pio == 3 || pio == 4)
+				use_iordy = 1;
+ } else if (pio >= 3 || ata_id_has_iordy(ata_id))
+			use_iordy = 1;


No, you should check for IORDY regardless of CFA and using IORDY shouldn't be limeited to modes 3 and 4:

       if (ata_id_has_iordy(ata_id) && !(ata_id_is_cfa(ata_id) && pio > 4))
          use_iodry = 1;

+static u8 ide_mm_inb(unsigned long port)
+{
+	return (u8) readb((void __iomem *) port);

 Explict cast not needed here.

+void at91_ide_tf_load(ide_drive_t *drive, ide_task_t *task)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct ide_io_ports *io_ports = &hwif->io_ports;
+	struct ide_taskfile *tf = &task->tf;
+	u8 HIHI = (task->tf_flags & IDE_TFLAG_LBA48) ? 0xE0 : 0xEF;
+
+	if (task->tf_flags & IDE_TFLAG_FLAGGED)
+		HIHI = 0xFF;
+
+	if (task->tf_flags & IDE_TFLAG_OUT_DATA) {

  Sigh. Bart, couldn't we drop that stupid flag? I bet nobody ever used it.

+void at91_ide_tf_read(ide_drive_t *drive, ide_task_t *task)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct ide_io_ports *io_ports = &hwif->io_ports;
+	struct ide_taskfile *tf = &task->tf;
+
+	if (task->tf_flags & IDE_TFLAG_IN_DATA) {

  ... and this one too?

+/*
+ * If interrupt is delivered through GPIO, IRQ are triggered on falling
+ * and raising edge of signal. Whereas IDE device request interrupt on high
+ * level (raising edge in our case). This mean we have fake interrupts, so
+ * we need to check interrupt pin and exit instantly from ISR when line
+ * is on low level.
+ */
+
+irqreturn_t at91_irq_handler(int irq, void *dev_id)
+{
+	int ntries = 8;
+	int pin_val1, pin_val2;
+
+	/* additional deglitch, line can be noisy in badly designed PCB */
+	do {
+		pin_val1 = at91_get_gpio_value(irq);
+		pin_val2 = at91_get_gpio_value(irq);
+	} while (pin_val1 != pin_val2 && --ntries > 0);

 I suggest a shorter code:

	do {
		pin_val = at91_get_gpio_value(irq);
	} while (pin_val != at91_get_gpio_value(irq) && --ntries > 0);


+static int __init at91_ide_probe(struct platform_device *pdev)
+{
[...]
+	host->ports[0]->extra_base = board->chipselect;

BTW, we have 2 fields in the struct hwif_s that fit this case better: config_data and select_data. It's a bit stange that you've selected extra_base...

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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux