Re: [PATCH] ide: Add tx4939ide driver (v3)

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

 



Hello.

Atsushi Nemoto wrote:

This is the driver for the Toshiba TX4939 SoC ATA controller.

This controller has standard ATA taskfile registers and DMA
command/status registers, but the register layout is swapped on big
endian.  There are some other endian issue and some special registers
which requires many custom dma_ops/tp_ops routines and build_dmatable.

Signed-off-by: Atsushi Nemoto <anemo@xxxxxxxxxxxxx>

  Mostly ACK but there's still a few minor nits...

diff --git a/drivers/ide/mips/Makefile b/drivers/ide/mips/Makefile
index 677c7b2..1e0ad98 100644
--- a/drivers/ide/mips/Makefile
+++ b/drivers/ide/mips/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_BLK_DEV_IDE_SWARM)		+= swarm.o

 The context have changed here but I guess Bart handled that...

diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
new file mode 100644
index 0000000..6671d64
--- /dev/null
+++ b/drivers/ide/mips/tx4939ide.c
@@ -0,0 +1,775 @@
[...]
+/* ATA Shadow Registers (8-bit except for DATA which is 16-bit) */
+#define TX4939IDE_DATA			0x000

Not sure whether the data register deserves more respect than the others. :-)

+/* H/W DMA Registers  */
+#define TX4939IDE_DMA_Cmd	0x800	/* 8-bit */
+#define TX4939IDE_DMA_stat	0x802	/* 8-bit */

  Symbol case still inconsistent...

+static void tx4939ide_set_dma_mode(ide_drive_t *drive, const u8 mode)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	u32 mask, val;
+
+	/* Update Data Transfer Mode for this drive. */
+	if (mode >= XFER_UDMA_0)
+		val = mode - XFER_UDMA_0 + 8;
+	else if (mode >= XFER_MW_DMA_0)
+		val = mode - XFER_MW_DMA_0 + 5;
+	else
+		val = mode - XFER_PIO_0;

I must've missed that in the previous review but you don't need to handle PIO modes in this method.

+static u16 tx4939ide_check_error_ints(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u16 ctl = tx4939ide_readw(base, TX4939IDE_Int_Ctl);
+
+	if (ctl & TX4939IDE_INT_BUSERR) {
+		/* reset FIFO */
+		u16 sysctl = tx4939ide_readw(base, TX4939IDE_Sys_Ctl);
+		tx4939ide_writew(sysctl | 0x4000, base, TX4939IDE_Sys_Ctl);
+		mmiowb();
+		/* wait 12GBUSCLK (typ. 60ns @ GBUS200MHz, max 270ns) */
+		ndelay(270);
+		tx4939ide_writew(sysctl, base, TX4939IDE_Sys_Ctl);
+	}
+	if (ctl & (TX4939IDE_INT_ADDRERR |
+		    TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_BUSERR))

  Hm, why not line up TX4939IDE_INT_DEVTIMING with TX4939IDE_INT_ADDRERR?

+static void tx4939ide_clear_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif;
+	void __iomem *base;
+	u16 ctl;
+
+	/*
+	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
+	 * jobs for DMA case.

  Shouldn't it be "job", singular?

+#ifdef __BIG_ENDIAN
+static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
+{
+	ide_hwif_t *hwif	= drive->hwif;
+	u8 unit			= drive->dn & 1;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u8 dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);

Hm, why not line up all the initializers? Either do line up all or do not line up any...

+static u8 tx4939ide_read_and_clear_dma_status(void __iomem *base)

  Hum, that's a long name... :-)

+#ifdef __BIG_ENDIAN
+/* custom ide_build_dmatable to handle swapped layout */
+static int tx4939ide_build_dmatable(ide_drive_t *drive, struct request *rq)
+{
[...]
+			xcount = bcount & 0xffff;
+			if (xcount == 0x0000) {

Hm, I'm not sure this is necessary here... although I didn't see an explicit mention that zero count means 64 KB in the datasheet -- which it must mean if the BMIDE spec. was followed).
In ide-dma.c this check was added because of CS5530's brain damage...

+static int tx4939ide_dma_end(ide_drive_t *drive)
+{
[...]
+	if ((dma_stat & 7) == 0 &&
+	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
+	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))

  Parens around & and | are hardly needed...

+/* returns 1 if dma irq issued, 0 otherwise */
+static int tx4939ide_dma_test_irq(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	u16 ctl;
+	u8 dma_stat, stat;
+	u16 ide_int;
+	int found = 0;
+
+	ctl = tx4939ide_check_error_ints(hwif);
+	ide_int = ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST);
+	switch (ide_int) {
+	case TX4939IDE_INT_HOST:
+		/* On error, XFEREND might not be asserted. */
+		stat = tx4939ide_readb(base, TX4939IDE_AltStat_DevCtl);
+		if ((stat & (ATA_BUSY|ATA_DRQ|ATA_ERR)) == ATA_ERR)
+			found = 1;
+		else {
+			/* Wait for XFERINT (Mask HOST and unmask XFERINT) */

  s/XFERINT/XFEREND/

+			ctl &= ~TX4939IDE_INT_XFEREND << 8;
+			ctl |= TX4939IDE_INT_HOST << 8;

The last statement seems superfluous given that the same is achieved by the following statement.

+		}
+		ctl |= ide_int << 8;
+		break;
+	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
+		dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
+		if (!(dma_stat & 4))
+			pr_warning("%s: weird interrupt status. "
+				   "DMA_stat %#02x int_ctl %#04x\n",
+				   hwif->name, dma_stat, ctl);
+		found = 1;
+		break;
+	}
+	/*
+	 * Do not clear XFERINT, HOST now.  They will be cleared by

  s/XFERINT/XFEREND/

+static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	return tx4939ide_readb(base, TX4939IDE_DMA_stat);
+}
+

Can't ide_read_sff_dma_status() be used in LE mode now that you set hwif->dma_base?

+static void tx4939ide_insw_swap(unsigned long port, void *addr, u32 count)
[...]
+static void tx4939ide_outsw_swap(unsigned long port, void *addr, u32 count)

  Shouldn't these be inline (if you really need them)?

+static int __init tx4939ide_probe(struct platform_device *pdev)
+{
[...]
+	pr_info("TX4939 IDE interface (%lx,%d)\n", mapbase, irq);

Hm, the bare numbers in the log won't be informative, could you add "base" and "IRQ"?

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