linux-mips
[Top] [All Lists]

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

To: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Subject: Re: [PATCH] ide: Add tx4939ide driver (v4)
From: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Date: Sun, 19 Oct 2008 16:42:15 +0400
Cc: linux-mips@linux-mips.org, linux-ide@vger.kernel.org, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>, ralf@linux-mips.org
In-reply-to: <20081017.230825.95059872.anemo@mba.ocn.ne.jp>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20081017.230825.95059872.anemo@mba.ocn.ne.jp>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Thunderbird 2.0.0.17 (Windows/20080914)
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@mba.ocn.ne.jp>

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



<Prev in Thread] Current Thread [Next in Thread>