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

[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>

Again, mostly ACK but there are some things that I haven't noticed before...

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index 6c6dd2f..c23ff28 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -746,6 +746,12 @@ config BLK_DEV_IDE_AU1XXX_SEQTS_PER_RQ
        default "128"
        depends on BLK_DEV_IDE_AU1XXX
+config BLK_DEV_IDE_TX4939
+	tristate "TX4939 internal IDE support"
+	depends on SOC_TX4939
+	select BLK_DEV_IDEDMA_SFF
+	select IDE_TIMINGS

  BTW, are you really using anything from ide-timings.c?

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

  Speaking about cnsistency, "data" is not an acronym. :-)

+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 {
+		BUG_ON(mode < XFER_MW_DMA_0);

  Should be no need for that as it's filtered out at the higher level...

+static int tx4939ide_dma_setup(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	void __iomem *base = TX4939IDE_BASE(hwif);
+	struct request *rq = hwif->hwgroup->rq;
+	unsigned int reading;

  BTW, shoudn't it be of type 'u8'?

+	int nent;
+
+	if (rq_data_dir(rq))
+		reading = 0;
+	else
+		reading = 1 << 3;

I think it's time to start using ATA_DMA_WR instead of the bare number; maybe I'll submit a patch to do this for ide-dma-sff.c...

+static int tx4939ide_dma_end(ide_drive_t *drive)
+{
[...]
+	tx4939ide_writeb(dma_cmd & ~1, base, TX4939IDE_DMA_Cmd);

  Suggesting s/1/ATA_DMA_START/...
OTOH, might be addressed by a followup patch converting every SFF-8038i compatible driver, if I (or Bart) ever get to it...

+/* returns 1 if dma irq issued, 0 otherwise */

  OTOH, DMA and IRQ are acronyms... :-)

+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)

  Er, need spaces around | for consistency...

+			found = 1;
+		else
+			/* Wait for XFEREND (Mask HOST and unmask XFEREND) */
+			ctl &= ~TX4939IDE_INT_XFEREND << 8;
+		ctl |= ide_int << 8;
+		break;
+	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
+		dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_Stat);
+		if (!(dma_stat & 4))

  s/4/ATA_DMA_INTR/?

+static void tx4939ide_init_hwif(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);
+
+	/* Soft Reset */
+	tx4939ide_writew(0x8000, base, TX4939IDE_Sys_Ctl);
+	mmiowb();
+	/* at least 20 UPSCLK (typ. 100ns @ GBUS200MHz, max 450ns) */

  Not 20 GBUSCLK?

+static int tx4939ide_init_dma(ide_hwif_t *hwif, const struct ide_port_info *d)
+{
+	hwif->dma_base = (unsigned long)TX4939IDE_BASE(hwif) +

Hum, casting 'hwif->extra_base' to 'void __iomem *' and then back to 'unsigned long' is too much, don't you think?

+#ifdef __BIG_ENDIAN
+
+static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
+{
+	void __iomem *base = TX4939IDE_BASE(hwif);

  No new line after declaration...

+	return tx4939ide_readb(base, TX4939IDE_DMA_Stat);
+}
+
+/* custom iops (independent from SWAP_IO_SPACE) */
+static u8 tx4939ide_inb(unsigned long port)
+{
+	return (u8)__raw_readb((void __iomem *)port);

  Doesn't __raw_readb() return 'u8'?

+static void tx4939ide_tf_load(ide_drive_t *drive, ide_task_t *task)
+{
[...]
+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE)
+		tx4939ide_outb((tf->device & HIHI) | drive->select,
+			       io_ports->device_addr);
+
+	tx4939ide_tf_load_fixup(drive, task);

Might be worth calling tx4939ide_tf_load_fixup() under the preceding *if* and removing the corresponding *if* from that function...

+static void tx4939ide_tf_load(ide_drive_t *drive, ide_task_t *task)
+{
+	ide_tf_load(drive, task);
+	tx4939ide_tf_load_fixup(drive, task);

  ... and adding *if* here.

+static int __init tx4939ide_probe(struct platform_device *pdev)
+{
+	hw_regs_t hw;
+	hw_regs_t *hws[] = { &hw, NULL, NULL, NULL };
+	struct ide_host *host;
+	struct resource *res;
+	int irq;
+	unsigned long mapbase;
+	int ret;

  Variables 'írq' and 'ret' could be on the same line...

+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return -ENODEV;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	mapbase = (unsigned long)devm_ioremap(&pdev->dev, res->start,
+					      res->end - res->start + 1);

  Looks like you've forgotten to call request_mem_region()...

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