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
