* [PATCH]: [MPC5200] (v2) Add ATA DMA support
@ 2008-06-27 12:44 Tim Yamin
2008-06-30 15:40 ` Daniel Schnell
2008-07-01 23:49 ` Grant Likely
0 siblings, 2 replies; 9+ messages in thread
From: Tim Yamin @ 2008-06-27 12:44 UTC (permalink / raw)
To: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 228 bytes --]
Changes from previous version:
- Add FIFO status error checking code before a DMA transaction starts
and after it is completed.
- Fix an incorrect check in the previous patch causing spurious "dma
table too small" errors.
Tim
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 1050-mpc5200-add-ATA-DMA.patch --]
[-- Type: text/x-patch; name=1050-mpc5200-add-ATA-DMA.patch, Size: 24397 bytes --]
This patch adds MDMA/UDMA support (using BestComm for DMA) on the MPC5200
platform.
Based heavily on previous work by Freescale (Bernard Kuhn, John Rigby)
and Domen Puncer.
Using a SanDisk Extreme IV CF card I get read speeds of approximately
26.70 MB/sec.
The BestComm ATA task priority was changed to maximum in bestcomm_priv.h;
this fixes a deadlock issue I was experiencing when heavy DMA was
occuring on both the ATA and Ethernet BestComm tasks, e.g. when
downloading a large file over a LAN to disk.
There's also what I believe to be a hardware bug if you have high levels
of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
the address bus seems to sometimes get corrupted with ATA commands while
the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
I've asked Freescale about this but have not received a reply yet -- if
anybody from Freescale has any ideas please contact me; I can supply some
analyzer traces if needed. Therefore, for now, do not enable DMA if you
need reliable LocalPlus Bus unless you do a fixup in your driver as
follows:
Locking example:
while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
{
struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
if(bcom_buffer_done_2(tsk))
return 1;
}
return 0;
(Save the return value to `flags`)
Unlocking example:
if(flags == 0)
clear_bit(0, &pata_mpc52xx_ata_dma_lock);
Comments and testing would of course be very welcome.
Thanks,
Signed-off-by: Tim Yamin <plasm@roo.me.uk>
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/ata.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h 2008-03-18 15:49:53.000000000 +0000
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/ata.h 2008-04-15 10:42:38.000000000 +0100
@@ -16,8 +16,8 @@
struct bcom_ata_bd {
u32 status;
- u32 dst_pa;
u32 src_pa;
+ u32 dst_pa;
};
extern struct bcom_task *
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.c
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-03-18 15:49:53.000000000 +0000
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-04-15 10:42:38.000000000 +0100
@@ -330,11 +330,10 @@
/* Init 'always' initiator */
out_8(&bcom_eng->regs->ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
- /* Disable COMM Bus Prefetch on the original 5200; it's broken */
- if ((mfspr(SPRN_SVR) & MPC5200_SVR_MASK) == MPC5200_SVR) {
- regval = in_be16(&bcom_eng->regs->PtdCntrl);
- out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
- }
+ /* Disable COMM Bus Prefetch; ATA DMA does not work properly with it
+ enabled. */
+ regval = in_be16(&bcom_eng->regs->PtdCntrl);
+ out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
/* Init lock */
spin_lock_init(&bcom_eng->lock);
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-03-18 15:49:53.000000000 +0000
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-15 10:42:38.000000000 +0100
@@ -17,6 +17,7 @@
#define __BESTCOMM_H__
struct bcom_bd; /* defined later on ... */
+struct bcom_bd_2;
/* ======================================================================== */
@@ -49,6 +50,22 @@
void* priv;
};
+struct bcom_task_2 {
+ unsigned int tasknum;
+ unsigned int flags;
+ int irq;
+
+ struct bcom_bd_2 *bd;
+ phys_addr_t bd_pa;
+ void **cookie;
+ unsigned short index;
+ unsigned short outdex;
+ unsigned int num_bd;
+ unsigned int bd_size;
+
+ void* priv;
+};
+
#define BCOM_FLAGS_NONE 0x00000000ul
#define BCOM_FLAGS_ENABLE_TASK (1ul << 0)
@@ -95,6 +112,11 @@
u32 data[1]; /* variable, but at least 1 */
};
+struct bcom_bd_2 {
+ u32 status;
+ u32 data[2]; /* variable, but at least 2 */
+};
+
#define BCOM_BD_READY 0x40000000ul
/** _bcom_next_index - Get next input index.
@@ -108,6 +130,12 @@
return ((tsk->index + 1) == tsk->num_bd) ? 0 : tsk->index + 1;
}
+static inline int
+_bcom_next_index_2(struct bcom_task_2 *tsk)
+{
+ return ((tsk->index + 1) == tsk->num_bd) ? 0 : tsk->index + 1;
+}
+
/** _bcom_next_outdex - Get next output index.
* @tsk: pointer to task structure
*
@@ -129,6 +157,12 @@
return tsk->index == tsk->outdex;
}
+static inline int
+bcom_queue_empty_2(struct bcom_task_2 *tsk)
+{
+ return tsk->index == tsk->outdex;
+}
+
/**
* bcom_queue_full - Checks if a BestComm task BD queue is full
* @tsk: The BestComm task structure
@@ -151,6 +185,14 @@
return !(tsk->bd[tsk->outdex].status & BCOM_BD_READY);
}
+static inline int
+bcom_buffer_done_2(struct bcom_task_2 *tsk)
+{
+ if (bcom_queue_empty_2(tsk))
+ return 0;
+ return !(tsk->bd[tsk->outdex].status & BCOM_BD_READY);
+}
+
/**
* bcom_prepare_next_buffer - clear status of next available buffer.
* @tsk: The BestComm task structure
@@ -175,6 +217,22 @@
bcom_enable(tsk);
}
+static inline struct bcom_bd_2 *
+bcom_prepare_next_buffer_2(struct bcom_task_2 *tsk)
+{
+ tsk->bd[tsk->index].status = 0; /* cleanup last status */
+ return &tsk->bd[tsk->index];
+}
+
+static inline void
+bcom_submit_next_buffer_2(struct bcom_task_2 *tsk, void *cookie)
+{
+ tsk->cookie[tsk->index] = cookie;
+ mb(); /* ensure the bd is really up-to-date */
+ tsk->bd[tsk->index].status |= BCOM_BD_READY;
+ tsk->index = _bcom_next_index_2(tsk);
+}
+
static inline void *
bcom_retrieve_buffer(struct bcom_task *tsk, u32 *p_status, struct bcom_bd **p_bd)
{
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-03-18 15:49:53.000000000 +0000
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-04-15 10:42:38.000000000 +0100
@@ -198,8 +198,8 @@ struct bcom_task_header {
#define BCOM_IPR_SCTMR_1 2
#define BCOM_IPR_FEC_RX 6
#define BCOM_IPR_FEC_TX 5
-#define BCOM_IPR_ATA_RX 4
-#define BCOM_IPR_ATA_TX 3
+#define BCOM_IPR_ATA_RX 7
+#define BCOM_IPR_ATA_TX 7
#define BCOM_IPR_SCPCI_RX 2
#define BCOM_IPR_SCPCI_TX 2
#define BCOM_IPR_PSC3_RX 2
diff -Nurp linux-2.6.26-rc6/drivers/ata/Kconfig linux-2.6.26-rc6.new/drivers/ata/Kconfig
--- linux-2.6.26-rc6/drivers/ata/Kconfig 2008-03-18 15:49:33.000000000 +0000
+++ linux-2.6.26-rc6.new/drivers/ata/Kconfig 2008-04-15 10:41:51.000000000 +0100
@@ -462,6 +462,15 @@
If unsure, say N.
+config PATA_MPC52xx_DMA
+ tristate "Freescale MPC52xx SoC internal IDE DMA"
+ depends on PATA_MPC52xx
+ help
+ This option enables support for DMA on the MPC52xx SoC PATA
+ controller.
+
+ If unsure, say N.
+
config PATA_MPIIX
tristate "Intel PATA MPIIX support"
depends on PCI
diff -Nurp linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c linux-2.6.26-rc6.new/drivers/ata/pata_mpc52xx.c
--- linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c 2008-03-18 15:49:33.000000000 +0000
+++ linux-2.6.26-rc6.new/drivers/ata/pata_mpc52xx.c 2008-04-15 10:41:49.000000000 +0100
@@ -6,6 +6,9 @@
* Copyright (C) 2006 Sylvain Munaut <tnt@246tNt.com>
* Copyright (C) 2003 Mipsys - Benjamin Herrenschmidt
*
+ * UDMA support based on patches by Freescale (Bernard Kuhn, John Rigby),
+ * Domen Puncer and Tim Yamin.
+ *
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed "as is" without any warranty of any
* kind, whether express or implied.
@@ -17,28 +20,47 @@
#include <linux/delay.h>
#include <linux/libata.h>
+#include <asm/cacheflush.h>
#include <asm/types.h>
#include <asm/prom.h>
#include <asm/of_platform.h>
#include <asm/mpc52xx.h>
+#include <sysdev/bestcomm/bestcomm.h>
+#include <sysdev/bestcomm/bestcomm_priv.h>
+#include <sysdev/bestcomm/ata.h>
#define DRV_NAME "mpc52xx_ata"
#define DRV_VERSION "0.1.2"
-
/* Private structures used by the driver */
struct mpc52xx_ata_timings {
u32 pio1;
u32 pio2;
+ u32 mdma1;
+ u32 mdma2;
+ u32 udma1;
+ u32 udma2;
+ u32 udma3;
+ u32 udma4;
+ u32 udma5;
+ int using_udma;
};
struct mpc52xx_ata_priv {
unsigned int ipb_period;
struct mpc52xx_ata __iomem * ata_regs;
+ struct mpc52xx_ata __iomem * ata_regs_pa;
int ata_irq;
struct mpc52xx_ata_timings timings[2];
int csel;
+
+ /* DMA */
+ struct bcom_task * dmatsk;
+ const struct udmaspec * udmaspec;
+ const struct mdmaspec * mdmaspec;
+ int mpc52xx_ata_dma_last_write;
+ int waiting_for_dma;
};
@@ -53,6 +75,102 @@
#define CALC_CLKCYC(c,v) ((((v)+(c)-1)/(c)))
+/* ATAPI-4 MDMA specs (in clocks) */
+struct mdmaspec {
+ u32 t0M[3];
+ u32 td[3];
+ u32 th[3];
+ u32 tj[3];
+ u32 tkw[3];
+ u32 tm[3];
+ u32 tn[3];
+};
+
+// -----------------------------------------------------------------------------------------------
+
+static const struct mdmaspec mdmaspec66 = {
+ {32, 10, 8},
+ {15, 6, 5},
+ {2, 1, 1},
+ {2, 1, 1},
+ {15, 4, 2},
+ {4, 2, 2},
+ {1, 1, 1}
+};
+
+static const struct mdmaspec mdmaspec132 = {
+ {64, 20, 16},
+ {29, 11, 10},
+ {3, 2, 2},
+ {3, 1, 1},
+ {29, 7, 4},
+ {7, 4, 4},
+ {2, 1, 1}
+};
+
+
+/* ATAPI-4 UDMA specs (in clocks) */
+struct udmaspec {
+ u32 tcyc[6];
+ u32 t2cyc[6];
+ u32 tds[6];
+ u32 tdh[6];
+ u32 tdvs[6];
+ u32 tdvh[6];
+ u32 tfs_min[6];
+ u32 tli_max[6];
+ u32 tmli[6];
+ u32 taz[6];
+ u32 tzah[6];
+ u32 tenv_min[6];
+ u32 tsr[6];
+ u32 trfs[6];
+ u32 trp[6];
+ u32 tack[6];
+ u32 tss[6];
+};
+
+static const struct udmaspec udmaspec66 = {
+ { 8, 5, 4, 3, 2, 2},
+ {16, 11, 8, 6, 4 , 2},
+ { 1, 1, 1, 1, 1, 1},
+ { 1, 1, 1, 1, 1, 1},
+ { 5, 4, 3, 2, 1, 1},
+ { 1, 1, 1, 1, 1, 1},
+ {16, 14, 12, 9, 8, 6},
+ {10, 10, 10, 7, 8, 5},
+ { 2, 2, 2, 2, 2, 2},
+ { 1, 1, 1, 1, 1, 1},
+ { 2, 2, 2, 2, 2, 2},
+ { 2, 2, 2, 2, 2, 2},
+ { 3, 2, 2, 2, 2, 2},
+ { 5, 5, 4, 4, 4, 4},
+ {11, 9, 7, 7, 7, 6},
+ { 2, 2, 2, 2, 2, 2},
+ { 4, 4, 4, 4, 4, 4}
+};
+
+static const struct udmaspec udmaspec132 = {
+ {15, 10, 6, 7, 2, 3},
+ {31, 21, 12, 12, 5, 6},
+ { 2, 2, 1, 1, 0, 1},
+ { 1, 1, 1, 1, 0, 1},
+ {10, 7, 5, 3, 1, 1},
+ { 1, 1, 1, 1, 1, 1},
+ {30, 27, 23, 15, 16, 12},
+ {20, 20, 20, 13, 14, 10},
+ { 3, 3, 3, 3, 2, 3},
+ { 2, 2, 2, 2, 1, 2},
+ { 3, 3, 3, 3, 2, 3},
+ { 3, 3, 3, 3, 2, 3},
+ { 7, 4, 3, 3, 2, 3},
+ {10, 10, 8, 8, 7, 7},
+ {22, 17, 14, 14, 13, 12},
+ { 3, 3, 3, 3, 2, 3},
+ { 7, 7, 7, 7, 6, 7},
+};
+
+// -----------------------------------------------------------------------------------------------
/* Bit definitions inside the registers */
#define MPC52xx_ATA_HOSTCONF_SMR 0x80000000UL /* State machine reset */
@@ -66,6 +184,7 @@
#define MPC52xx_ATA_HOSTSTAT_WERR 0x01000000UL /* Write Error */
#define MPC52xx_ATA_FIFOSTAT_EMPTY 0x01 /* FIFO Empty */
+#define MPC52xx_ATA_FIFOSTAT_ERROR 0x40 /* FIFO Error */
#define MPC52xx_ATA_DMAMODE_WRITE 0x01 /* Write DMA */
#define MPC52xx_ATA_DMAMODE_READ 0x02 /* Read DMA */
@@ -75,6 +194,8 @@
#define MPC52xx_ATA_DMAMODE_FR 0x20 /* FIFO Reset */
#define MPC52xx_ATA_DMAMODE_HUT 0x40 /* Host UDMA burst terminate */
+#define MAX_DMA_BUFFERS 128
+#define MAX_DMA_BUFFER_SIZE 0x20000u
/* Structure of the hardware registers */
struct mpc52xx_ata {
@@ -133,6 +254,9 @@
u8 reserved21[2];
};
+/* BestComm locking */
+unsigned long pata_mpc52xx_ata_dma_lock = 0;
+struct bcom_task_2 *pata_mpc52xx_ata_dma_task;
/* ======================================================================== */
/* Aux fns */
@@ -141,6 +265,19 @@
/* MPC52xx low level hw control */
+static inline void
+mpc52xx_ata_wait_tip_bit_clear(struct mpc52xx_ata __iomem *regs)
+{
+ int timeout = 1000;
+
+ while (in_be32(®s->host_status) & MPC52xx_ATA_HOSTSTAT_TIP)
+ if (timeout-- == 0) {
+ printk(KERN_ERR "mpc52xx-ide: Timeout waiting for TIP clear\n");
+ break;
+ }
+ udelay(10); /* FIXME: Necessary ??? */
+}
+
static int
mpc52xx_ata_compute_pio_timings(struct mpc52xx_ata_priv *priv, int dev, int pio)
{
@@ -165,6 +302,95 @@
return 0;
}
+static int
+mpc52xx_ata_compute_mdma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
+{
+ struct mpc52xx_ata_timings *timing = &priv->timings[dev];
+ u32 t0M, td, tkw, tm, th, tj, tn;
+
+ if (speed < 0 || speed > 2)
+ return -EINVAL;
+
+ t0M = priv->mdmaspec->t0M[speed];
+ td = priv->mdmaspec->td[speed];
+ tkw = priv->mdmaspec->tkw[speed];
+ tm = priv->mdmaspec->tm[speed];
+ th = priv->mdmaspec->th[speed];
+ tj = priv->mdmaspec->tj[speed];
+ tn = priv->mdmaspec->tn[speed];
+
+ DPRINTK ("t0M = %d\n", t0M);
+ DPRINTK ("td = %d\n", td);
+ DPRINTK ("tkw = %d\n", tkw);
+ DPRINTK ("tm = %d\n", tm);
+ DPRINTK ("th = %d\n", th);
+ DPRINTK ("tj = %d\n", tj);
+ DPRINTK ("tn = %d\n", tn);
+
+ timing->mdma1 = (t0M << 24) | (td << 16) | (tkw << 8) | (tm);
+ timing->mdma2 = (th << 24) | (tj << 16) | (tn << 8);
+
+ timing->using_udma = 0;
+
+ return 0;
+}
+
+static int
+mpc52xx_ata_compute_udma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
+{
+ struct mpc52xx_ata_timings *timing = &priv->timings[dev];
+ u32 t2cyc, tcyc, tds, tdh, tdvs, tdvh, tfs, tli, tmli, taz, tenv, tsr, tss, trfs, trp, tack, tzah;
+
+ if (speed < 0 || speed > 2)
+ return -EINVAL;
+
+ t2cyc = priv->udmaspec->t2cyc[speed];
+ tcyc = priv->udmaspec->tcyc[speed];
+ tds = priv->udmaspec->tds[speed];
+ tdh = priv->udmaspec->tdh[speed];
+ tdvs = priv->udmaspec->tdvs[speed];
+ tdvh = priv->udmaspec->tdvh[speed];
+ tfs = priv->udmaspec->tfs_min[speed];
+ tmli = priv->udmaspec->tmli[speed];
+ tenv = priv->udmaspec->tenv_min[speed];
+ tss = priv->udmaspec->tss[speed];
+ trp = priv->udmaspec->trp[speed];
+ tack = priv->udmaspec->tack[speed];
+ tzah = priv->udmaspec->tzah[speed];
+ taz = priv->udmaspec->taz[speed];
+ trfs = priv->udmaspec->trfs[speed];
+ tsr = priv->udmaspec->tsr[speed];
+ tli = priv->udmaspec->tli_max[speed];
+
+ DPRINTK ("UDMA t2cyc = %d\n", t2cyc);
+ DPRINTK ("UDMA tcyc = %d\n", tcyc);
+ DPRINTK ("UDMA tds = %d\n", tds);
+ DPRINTK ("UDMA tdh = %d\n", tdh);
+ DPRINTK ("UDMA tdvs = %d\n", tdvs);
+ DPRINTK ("UDMA tdvh = %d\n", tdvh);
+ DPRINTK ("UDMA tfs = %d\n", tfs);
+ DPRINTK ("UDMA tli = %d\n", tli);
+ DPRINTK ("UDMA tmli = %d\n", tmli);
+ DPRINTK ("UDMA taz = %d\n", taz);
+ DPRINTK ("UDMA tenv = %d\n", tenv);
+ DPRINTK ("UDMA tsr = %d\n", tsr);
+ DPRINTK ("UDMA tss = %d\n", tss);
+ DPRINTK ("UDMA trfs = %d\n", trfs);
+ DPRINTK ("UDMA trp = %d\n", trp);
+ DPRINTK ("UDMA tack = %d\n", tack);
+ DPRINTK ("UDMA tzah = %d\n", tzah);
+
+ timing->udma1 = (t2cyc << 24) | (tcyc << 16) | (tds << 8) | (tdh);
+ timing->udma2 = (tdvs << 24) | (tdvh << 16) | (tfs << 8) | (tli);
+ timing->udma3 = (tmli << 24) | (taz << 16) | (tenv << 8) | (tsr);
+ timing->udma4 = (tss << 24) | (trfs << 16) | (trp << 8) | (tack);
+ timing->udma5 = (tzah << 24);
+
+ timing->using_udma = 1;
+
+ return 0;
+}
+
static void
mpc52xx_ata_apply_timings(struct mpc52xx_ata_priv *priv, int device)
{
@@ -173,14 +399,13 @@
out_be32(®s->pio1, timing->pio1);
out_be32(®s->pio2, timing->pio2);
- out_be32(®s->mdma1, 0);
- out_be32(®s->mdma2, 0);
- out_be32(®s->udma1, 0);
- out_be32(®s->udma2, 0);
- out_be32(®s->udma3, 0);
- out_be32(®s->udma4, 0);
- out_be32(®s->udma5, 0);
-
+ out_be32(®s->mdma1, timing->mdma1);
+ out_be32(®s->mdma2, timing->mdma2);
+ out_be32(®s->udma1, timing->udma1);
+ out_be32(®s->udma2, timing->udma2);
+ out_be32(®s->udma3, timing->udma3);
+ out_be32(®s->udma4, timing->udma4);
+ out_be32(®s->udma5, timing->udma5);
priv->csel = device;
}
@@ -245,6 +470,28 @@
mpc52xx_ata_apply_timings(priv, adev->devno);
}
static void
+mpc52xx_ata_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ int rv;
+
+ if (adev->dma_mode >= XFER_UDMA_0) {
+ int dma = adev->dma_mode - XFER_UDMA_0;
+ rv = mpc52xx_ata_compute_udma_timings(priv, adev->devno, dma);
+ } else {
+ int dma = adev->dma_mode - XFER_MW_DMA_0;
+ rv = mpc52xx_ata_compute_mdma_timings(priv, adev->devno, dma);
+ }
+
+ if (rv) {
+ printk(KERN_ERR DRV_NAME
+ ": Trying to select invalid DMA mode %d\n", adev->dma_mode);
+ return;
+ }
+
+ mpc52xx_ata_apply_timings(priv, adev->devno);
+}
+static void
mpc52xx_ata_dev_select(struct ata_port *ap, unsigned int device)
{
struct mpc52xx_ata_priv *priv = ap->host->private_data;
@@ -255,16 +502,208 @@
ata_sff_dev_select(ap,device);
}
+static void
+mpc52xx_sff_exec_command(struct ata_port *ap, const struct ata_taskfile *tf)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ mpc52xx_ata_wait_tip_bit_clear(priv->ata_regs);
+ ata_sff_exec_command(ap, tf);
+}
+
+static int
+mpc52xx_ata_build_dmatable(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ struct mpc52xx_ata __iomem *regs_pa = priv->ata_regs_pa;
+ unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE), si;
+ struct scatterlist *sg;
+ int count = 0;
+
+ if (read)
+ bcom_ata_rx_prepare(priv->dmatsk);
+ else
+ bcom_ata_tx_prepare(priv->dmatsk);
+
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
+ u32 cur_addr = sg_dma_address(sg);
+ u32 cur_len = sg_dma_len(sg);
+
+ while (cur_len) {
+ unsigned int tc = min(cur_len, MAX_DMA_BUFFER_SIZE);
+ struct bcom_ata_bd *bd = (struct bcom_ata_bd *) bcom_prepare_next_buffer_2((struct bcom_task_2 *) priv->dmatsk);
+
+ if (read) {
+ bd->status = tc;
+ bd->src_pa = (__force u32) ®s_pa->fifo_data;
+ bd->dst_pa = cur_addr;
+
+ invalidate_dcache_range((__force u32) phys_to_virt(cur_addr), (__force u32) phys_to_virt(cur_addr) + (__force u32) cur_len);
+ } else {
+ bd->status = tc;
+ bd->src_pa = cur_addr;
+ bd->dst_pa = (__force u32) ®s_pa->fifo_data;
+
+ flush_dcache_range((__force u32) phys_to_virt(cur_addr), (__force u32) phys_to_virt(cur_addr) + (__force u32) cur_len);
+ }
+
+ bcom_submit_next_buffer_2((struct bcom_task_2 *) priv->dmatsk, NULL);
+
+ cur_addr += tc;
+ cur_len -= tc;
+ count++;
+
+ if (count > MAX_DMA_BUFFERS) {
+ printk(KERN_ALERT "%s: %i dma table too small\n", __func__, __LINE__);
+ goto use_pio_instead;
+ }
+ }
+ }
+ return 1;
+
+use_pio_instead:
+ bcom_ata_reset_bd(priv->dmatsk);
+
+ return 0;
+}
+
+static void
+mpc52xx_bmdma_setup(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ struct mpc52xx_ata __iomem *regs = priv->ata_regs;
+
+ unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE);
+ u8 dma_mode;
+
+ if (!mpc52xx_ata_build_dmatable(qc)) {
+ printk(KERN_ALERT "%s: %i, return 1?\n", __func__, __LINE__);
+ }
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ printk("WARNING: pata_mpc52xx FIFO error detected: 0x%02x!\n", in_8(&priv->ata_regs->fifo_status));
+
+ if (read) {
+ dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_READ |
+ MPC52xx_ATA_DMAMODE_FE;
+
+ /* Setup FIFO if direction changed */
+ if (priv->mpc52xx_ata_dma_last_write != 0) {
+ priv->mpc52xx_ata_dma_last_write = 0;
+ mpc52xx_ata_wait_tip_bit_clear(regs);
+
+ /* Configure FIFO with granularity to 7 like sample code */
+ out_8(®s->fifo_control, 7);
+ out_be16(®s->fifo_alarm, 128);
+
+ /* Set FIFO Reset bit (FR) */
+ out_8(®s->dma_mode, MPC52xx_ATA_DMAMODE_FR);
+ }
+ } else {
+ dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_WRITE;
+
+ /* Setup FIFO if direction changed */
+ if (priv->mpc52xx_ata_dma_last_write != 1) {
+ priv->mpc52xx_ata_dma_last_write = 1;
+ mpc52xx_ata_wait_tip_bit_clear(regs);
+
+ /* Configure FIFO with granularity to 4 like sample code */
+ out_8(®s->fifo_control, 4);
+ out_be16(®s->fifo_alarm, 128);
+ }
+ }
+
+ if (priv->timings[qc->dev->devno].using_udma)
+ dma_mode |= MPC52xx_ATA_DMAMODE_UDMA;
+
+ mpc52xx_ata_wait_tip_bit_clear(regs);
+ out_8(®s->dma_mode, dma_mode);
+
+ priv->waiting_for_dma = ATA_DMA_ACTIVE;
+
+ ata_wait_idle(ap);
+ ap->ops->sff_exec_command(ap, &qc->tf);
+}
+
+static void
+mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ /* LocalBus lock */
+ while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
+ ;
+
+ bcom_set_task_auto_start(priv->dmatsk->tasknum, priv->dmatsk->tasknum | 0x40);
+ bcom_enable(priv->dmatsk);
+}
+
+static void
+mpc52xx_bmdma_stop(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ bcom_disable(priv->dmatsk);
+ bcom_ata_reset_bd(priv->dmatsk);
+
+ /* LocalBus unlock*/
+ clear_bit(0, &pata_mpc52xx_ata_dma_lock);
+
+ priv->waiting_for_dma = 0;
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ printk("WARNING: pata_mpc52xx FIFO error detected: 0x%02x!\n", in_8(&priv->ata_regs->fifo_status));
+}
+
+static u8
+mpc52xx_bmdma_status(struct ata_port *ap)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ {
+ printk("WARNING: pata_mpc52xx FIFO error detected: 0x%02x!\n", in_8(&priv->ata_regs->fifo_status));
+ return priv->waiting_for_dma | ATA_DMA_ERR;
+ }
+
+ return priv->waiting_for_dma;
+}
+
+static irqreturn_t
+mpc52xx_ata_task_irq(int irq, void *vpriv)
+{
+ struct mpc52xx_ata_priv *priv = vpriv;
+ priv->waiting_for_dma |= ATA_DMA_INTR;
+
+ return IRQ_HANDLED;
+}
+
static struct scsi_host_template mpc52xx_ata_sht = {
ATA_PIO_SHT(DRV_NAME),
};
static struct ata_port_operations mpc52xx_ata_port_ops = {
.inherits = &ata_sff_port_ops,
- .sff_dev_select = mpc52xx_ata_dev_select,
- .cable_detect = ata_cable_40wire,
+
.set_piomode = mpc52xx_ata_set_piomode,
- .post_internal_cmd = ATA_OP_NULL,
+ .set_dmamode = mpc52xx_ata_set_dmamode,
+
+ .sff_exec_command = mpc52xx_sff_exec_command,
+ .sff_dev_select = mpc52xx_ata_dev_select,
+
+ .bmdma_setup = mpc52xx_bmdma_setup,
+ .bmdma_start = mpc52xx_bmdma_start,
+ .bmdma_stop = mpc52xx_bmdma_stop,
+ .bmdma_status = mpc52xx_bmdma_status,
+
+ .qc_prep = ata_noop_qc_prep,
};
static int __devinit
@@ -281,9 +720,14 @@
ap = host->ports[0];
ap->flags |= ATA_FLAG_SLAVE_POSS;
- ap->pio_mask = 0x1f; /* Up to PIO4 */
- ap->mwdma_mask = 0x00; /* No MWDMA */
- ap->udma_mask = 0x00; /* No UDMA */
+ ap->pio_mask = ATA_PIO4; /* Up to PIO4 */
+#ifdef CONFIG_PATA_MPC52xx_DMA
+ ap->mwdma_mask = 0x07; /* Up to MWDMA2 */
+ ap->udma_mask = ATA_UDMA2; /* Up to UDMA2 */
+#else
+ ap->mwdma_mask = 0x00; /* No MWDMA */
+ ap->udma_mask = 0x00; /* No UDMA */
+#endif
ap->ops = &mpc52xx_ata_port_ops;
host->private_data = priv;
@@ -333,7 +777,7 @@
int ata_irq;
struct mpc52xx_ata __iomem *ata_regs;
struct mpc52xx_ata_priv *priv;
- int rv;
+ int rv, ret, task_irq;
/* Get ipb frequency */
ipb_freq = mpc52xx_find_ipb_freq(op->node);
@@ -389,8 +833,31 @@
priv->ipb_period = 1000000000 / (ipb_freq / 1000);
priv->ata_regs = ata_regs;
+ priv->ata_regs_pa = (struct mpc52xx_ata __iomem *) res_mem.start;
priv->ata_irq = ata_irq;
priv->csel = -1;
+ priv->mpc52xx_ata_dma_last_write = -1;
+
+ if (ipb_freq/1000000 == 66) {
+ priv->mdmaspec = &mdmaspec66;
+ priv->udmaspec = &udmaspec66;
+ } else {
+ priv->mdmaspec = &mdmaspec132;
+ priv->udmaspec = &udmaspec132;
+ }
+
+ priv->dmatsk = bcom_ata_init(MAX_DMA_BUFFERS, MAX_DMA_BUFFER_SIZE);
+ pata_mpc52xx_ata_dma_task = (struct bcom_task_2 *) priv->dmatsk;
+ if (!priv->dmatsk) {
+ printk(KERN_ALERT "%s: %i\n", __func__, __LINE__);
+ rv = -ENOMEM;
+ goto err;
+ }
+
+ task_irq = bcom_get_task_irq(priv->dmatsk);
+ ret = request_irq(task_irq, &mpc52xx_ata_task_irq, IRQF_DISABLED, "ATA task", priv);
+ if (ret)
+ printk(KERN_ALERT "%s: request_irq failed with: %i\n", __func__, ret);
/* Init the hw */
rv = mpc52xx_ata_hw_init(priv);
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-06-27 12:44 [PATCH]: [MPC5200] (v2) Add ATA DMA support Tim Yamin
@ 2008-06-30 15:40 ` Daniel Schnell
2008-07-01 23:49 ` Grant Likely
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Schnell @ 2008-06-30 15:40 UTC (permalink / raw)
To: Tim Yamin, linuxppc-dev
Hi,
Against which kernel is this patch against ?
Regards,
Daniel.
Tim Yamin wrote:
> Changes from previous version:
>=20
> - Add FIFO status error checking code before a DMA transaction starts
> and after it is completed.=20
> - Fix an incorrect check in the previous patch causing spurious "dma
> table too small" errors.=20
>=20
> Tim
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-06-27 12:44 [PATCH]: [MPC5200] (v2) Add ATA DMA support Tim Yamin
2008-06-30 15:40 ` Daniel Schnell
@ 2008-07-01 23:49 ` Grant Likely
2008-07-02 12:48 ` Tim Yamin
1 sibling, 1 reply; 9+ messages in thread
From: Grant Likely @ 2008-07-01 23:49 UTC (permalink / raw)
To: Tim Yamin; +Cc: linuxppc-dev
On Fri, Jun 27, 2008 at 01:44:08PM +0100, Tim Yamin wrote:
> diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.c
> --- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-03-18 15:49:53.000000000 +0000
> +++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-04-15 10:42:38.000000000 +0100
> @@ -330,11 +330,10 @@
> /* Init 'always' initiator */
> out_8(&bcom_eng->regs->ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
>
> - /* Disable COMM Bus Prefetch on the original 5200; it's broken */
> - if ((mfspr(SPRN_SVR) & MPC5200_SVR_MASK) == MPC5200_SVR) {
> - regval = in_be16(&bcom_eng->regs->PtdCntrl);
> - out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
> - }
> + /* Disable COMM Bus Prefetch; ATA DMA does not work properly with it
> + enabled. */
> + regval = in_be16(&bcom_eng->regs->PtdCntrl);
> + out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
This could have a performance impact on existing systems that don't
use/need ATA DMA.
Please run next version through checkpatch. There's lots of minor
cosmetic stuff, but there are also a few important errors.
> diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.h
> --- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-03-18 15:49:53.000000000 +0000
> +++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-15 10:42:38.000000000 +0100
> @@ -17,6 +17,7 @@
> #define __BESTCOMM_H__
>
> struct bcom_bd; /* defined later on ... */
> +struct bcom_bd_2;
>
>
> /* ======================================================================== */
> @@ -49,6 +50,22 @@
> void* priv;
> };
>
> +struct bcom_task_2 {
> + unsigned int tasknum;
> + unsigned int flags;
> + int irq;
> +
> + struct bcom_bd_2 *bd;
> + phys_addr_t bd_pa;
> + void **cookie;
> + unsigned short index;
> + unsigned short outdex;
> + unsigned int num_bd;
> + unsigned int bd_size;
> +
> + void* priv;
> +};
> +
Oh, ugly. The only difference seems to be that bcom_bd_2 is 1 word
larger than bcom_bd. There must be a better way to do this rather
than just duplicating all the functions and structures.. It would
probably be better to use bd_size to determine how to dereference an
index into the *bd list. It will require a bit of refactoring, but
it will be much cleaner and more maintainable that way.
> diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
> --- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-03-18 15:49:53.000000000 +0000
> +++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-04-15 10:42:38.000000000 +0100
> @@ -198,8 +198,8 @@ struct bcom_task_header {
> #define BCOM_IPR_SCTMR_1 2
> #define BCOM_IPR_FEC_RX 6
> #define BCOM_IPR_FEC_TX 5
> -#define BCOM_IPR_ATA_RX 4
> -#define BCOM_IPR_ATA_TX 3
> +#define BCOM_IPR_ATA_RX 7
> +#define BCOM_IPR_ATA_TX 7
> #define BCOM_IPR_SCPCI_RX 2
> #define BCOM_IPR_SCPCI_TX 2
> #define BCOM_IPR_PSC3_RX 2
Is this a bug fix? If so, please put it into a separate patch.
> diff -Nurp linux-2.6.26-rc6/drivers/ata/Kconfig linux-2.6.26-rc6.new/drivers/ata/Kconfig
> --- linux-2.6.26-rc6/drivers/ata/Kconfig 2008-03-18 15:49:33.000000000 +0000
> +++ linux-2.6.26-rc6.new/drivers/ata/Kconfig 2008-04-15 10:41:51.000000000 +0100
> @@ -462,6 +462,15 @@
>
> If unsure, say N.
>
> +config PATA_MPC52xx_DMA
> + tristate "Freescale MPC52xx SoC internal IDE DMA"
> + depends on PATA_MPC52xx
> + help
> + This option enables support for DMA on the MPC52xx SoC PATA
> + controller.
> +
> + If unsure, say N.
> +
Good, it can be turned off. Do you think there is any risk to existing
ATA users with this patch applied if this is turned off?
> config PATA_MPIIX
> tristate "Intel PATA MPIIX support"
> depends on PCI
> diff -Nurp linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c linux-2.6.26-rc6.new/drivers/ata/pata_mpc52xx.c
> --- linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c 2008-03-18 15:49:33.000000000 +0000
> +++ linux-2.6.26-rc6.new/drivers/ata/pata_mpc52xx.c 2008-04-15 10:41:49.000000000 +0100
> @@ -6,6 +6,9 @@
> * Copyright (C) 2006 Sylvain Munaut <tnt@246tNt.com>
> * Copyright (C) 2003 Mipsys - Benjamin Herrenschmidt
> *
> + * UDMA support based on patches by Freescale (Bernard Kuhn, John Rigby),
> + * Domen Puncer and Tim Yamin.
> + *
> * This file is licensed under the terms of the GNU General Public License
> * version 2. This program is licensed "as is" without any warranty of any
> * kind, whether express or implied.
> @@ -17,28 +20,47 @@
> #include <linux/delay.h>
> #include <linux/libata.h>
>
> +#include <asm/cacheflush.h>
> #include <asm/types.h>
> #include <asm/prom.h>
> #include <asm/of_platform.h>
> #include <asm/mpc52xx.h>
>
> +#include <sysdev/bestcomm/bestcomm.h>
> +#include <sysdev/bestcomm/bestcomm_priv.h>
> +#include <sysdev/bestcomm/ata.h>
>
> #define DRV_NAME "mpc52xx_ata"
> #define DRV_VERSION "0.1.2"
>
> -
> /* Private structures used by the driver */
> struct mpc52xx_ata_timings {
> u32 pio1;
> u32 pio2;
> + u32 mdma1;
> + u32 mdma2;
> + u32 udma1;
> + u32 udma2;
> + u32 udma3;
> + u32 udma4;
> + u32 udma5;
> + int using_udma;
> };
>
> struct mpc52xx_ata_priv {
> unsigned int ipb_period;
> struct mpc52xx_ata __iomem * ata_regs;
> + struct mpc52xx_ata __iomem * ata_regs_pa;
Why two copies of the ata_regs? (this should be documented)
> int ata_irq;
> struct mpc52xx_ata_timings timings[2];
> int csel;
> +
> + /* DMA */
> + struct bcom_task * dmatsk;
> + const struct udmaspec * udmaspec;
> + const struct mdmaspec * mdmaspec;
> + int mpc52xx_ata_dma_last_write;
> + int waiting_for_dma;
> };
>
>
> @@ -53,6 +75,102 @@
>
> #define CALC_CLKCYC(c,v) ((((v)+(c)-1)/(c)))
>
> +/* ATAPI-4 MDMA specs (in clocks) */
> +struct mdmaspec {
> + u32 t0M[3];
> + u32 td[3];
> + u32 th[3];
> + u32 tj[3];
> + u32 tkw[3];
> + u32 tm[3];
> + u32 tn[3];
> +};
This gets things to be quite verbose later on and it seems to separate
related data into 7 separate arrays. Would it make more sense to
structure it like this:
struct mdmaspec {
u32 t0M;
u32 td;
u32 th;
u32 tj;
u32 tkw;
u32 tm;
u32 tn;
};
static const struct mdmaspec mdmaspec66[3] = {
{.t0M=32, .td=15, .th=2, .tj=2, .tkw=15, .tm=4, .tn=1,},
{.t0M=10, .td=6, .th=1, .tj=1, .tkw=4, .tm=2, .tn=1,},
{.t0M=8, .td=5, .th=1, .tj=1, .tkw=2, .tm=2, .tn=1,},
};
It would allow some of the calculate code lower down dereference the
array once and should be much less verbose.
> +// -----------------------------------------------------------------------------------------------
Please use /* ----- */ instead
> +static const struct mdmaspec mdmaspec66 = {
> + {32, 10, 8},
> + {15, 6, 5},
> + {2, 1, 1},
> + {2, 1, 1},
> + {15, 4, 2},
> + {4, 2, 2},
> + {1, 1, 1}
> +};
This is a little fragile, please use the following form to make it more
robust against future structure changes. (and ditto through the rest of
the file)
+static const struct mdmaspec mdmaspec66 = {
+ .t0M = {32, 10, 8},
+ .td = {15, 6, 5},
+ .th = {2, 1, 1},
+ .tj = {2, 1, 1},
+ .tkw = {15, 4, 2},
+ .tm = {4, 2, 2},
+ .tn = {1, 1, 1}
+};
> /* ======================================================================== */
> /* Aux fns */
> @@ -141,6 +265,19 @@
>
> /* MPC52xx low level hw control */
>
> +static inline void
Drop 'inline' and trust GCC
> +mpc52xx_ata_wait_tip_bit_clear(struct mpc52xx_ata __iomem *regs)
> +{
> + int timeout = 1000;
> +
> + while (in_be32(®s->host_status) & MPC52xx_ATA_HOSTSTAT_TIP)
> + if (timeout-- == 0) {
> + printk(KERN_ERR "mpc52xx-ide: Timeout waiting for TIP clear\n");
> + break;
> + }
> + udelay(10); /* FIXME: Necessary ??? */
Can you find any way to avoid this? This could be a performance drain.
> +static int
> +mpc52xx_ata_compute_mdma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
> +{
> + struct mpc52xx_ata_timings *timing = &priv->timings[dev];
> + u32 t0M, td, tkw, tm, th, tj, tn;
> +
> + if (speed < 0 || speed > 2)
> + return -EINVAL;
> +
> + t0M = priv->mdmaspec->t0M[speed];
> + td = priv->mdmaspec->td[speed];
> + tkw = priv->mdmaspec->tkw[speed];
> + tm = priv->mdmaspec->tm[speed];
> + th = priv->mdmaspec->th[speed];
> + tj = priv->mdmaspec->tj[speed];
> + tn = priv->mdmaspec->tn[speed];
> +
> + DPRINTK ("t0M = %d\n", t0M);
> + DPRINTK ("td = %d\n", td);
> + DPRINTK ("tkw = %d\n", tkw);
> + DPRINTK ("tm = %d\n", tm);
> + DPRINTK ("th = %d\n", th);
> + DPRINTK ("tj = %d\n", tj);
> + DPRINTK ("tn = %d\n", tn);
> +
> + timing->mdma1 = (t0M << 24) | (td << 16) | (tkw << 8) | (tm);
> + timing->mdma2 = (th << 24) | (tj << 16) | (tn << 8);
This is what I'm referring to. You could do:
struct mdmaspec *s = &priv->mdmaspec[speed];
and then access all the data as s->(blah) directly.
> +static int
> +mpc52xx_ata_build_dmatable(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> + struct mpc52xx_ata __iomem *regs_pa = priv->ata_regs_pa;
> + unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE), si;
> + struct scatterlist *sg;
> + int count = 0;
> +
> + if (read)
> + bcom_ata_rx_prepare(priv->dmatsk);
> + else
> + bcom_ata_tx_prepare(priv->dmatsk);
> +
> + for_each_sg(qc->sg, sg, qc->n_elem, si) {
> + u32 cur_addr = sg_dma_address(sg);
> + u32 cur_len = sg_dma_len(sg);
> +
> + while (cur_len) {
> + unsigned int tc = min(cur_len, MAX_DMA_BUFFER_SIZE);
> + struct bcom_ata_bd *bd = (struct bcom_ata_bd *) bcom_prepare_next_buffer_2((struct bcom_task_2 *) priv->dmatsk);
Ugly casting. Reworking the bestcomm stuff should eliminate it.
> +
> + if (read) {
> + bd->status = tc;
> + bd->src_pa = (__force u32) ®s_pa->fifo_data;
> + bd->dst_pa = cur_addr;
> +
> + invalidate_dcache_range((__force u32) phys_to_virt(cur_addr), (__force u32) phys_to_virt(cur_addr) + (__force u32) cur_len);
Even uglier casting. Is this the right approach?
> +static void
> +mpc52xx_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> + struct mpc52xx_ata __iomem *regs = priv->ata_regs;
> +
> + unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE);
> + u8 dma_mode;
> +
> + if (!mpc52xx_ata_build_dmatable(qc)) {
> + printk(KERN_ALERT "%s: %i, return 1?\n", __func__, __LINE__);
> + }
> +
> + /* Check FIFO is OK... */
> + if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
> + printk("WARNING: pata_mpc52xx FIFO error detected: 0x%02x!\n", in_8(&priv->ata_regs->fifo_status));
Line too long, please split (goes for the whole file)
> static int __devinit
> @@ -281,9 +720,14 @@
>
> ap = host->ports[0];
> ap->flags |= ATA_FLAG_SLAVE_POSS;
> - ap->pio_mask = 0x1f; /* Up to PIO4 */
> - ap->mwdma_mask = 0x00; /* No MWDMA */
> - ap->udma_mask = 0x00; /* No UDMA */
> + ap->pio_mask = ATA_PIO4; /* Up to PIO4 */
> +#ifdef CONFIG_PATA_MPC52xx_DMA
> + ap->mwdma_mask = 0x07; /* Up to MWDMA2 */
> + ap->udma_mask = ATA_UDMA2; /* Up to UDMA2 */
> +#else
> + ap->mwdma_mask = 0x00; /* No MWDMA */
> + ap->udma_mask = 0x00; /* No UDMA */
> +#endif
Is there any way to turn on/off DMA at runtime instead of CONFIG time?
>
> priv->ipb_period = 1000000000 / (ipb_freq / 1000);
> priv->ata_regs = ata_regs;
> + priv->ata_regs_pa = (struct mpc52xx_ata __iomem *) res_mem.start;
I'm not fond of this. First off, it is *not* __iomem. It is physical
address. It would be better to use the offset_of macro to add an offset
to the physical base address. Doing it this way forces you to cast and
sidestep the compile time checks for incorrect dereferences.
Some big comments, but it looks like a good driver; thanks!
Cheers,
g.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-07-01 23:49 ` Grant Likely
@ 2008-07-02 12:48 ` Tim Yamin
2008-07-02 17:30 ` Grant Likely
0 siblings, 1 reply; 9+ messages in thread
From: Tim Yamin @ 2008-07-02 12:48 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]
Hi Grant,
Thanks for the feedback. New version is attached.
> Is this a bug fix? If so, please put it into a separate patch.
I suppose so, yes. If Ethernet has higher priority than ATA, you can
get a deadlock if you try and download a large file over a LAN to
disk, for example. But given that nothing other than this patch uses
BestComm for ATA do you have any specific reason to split it out into
another patch?
> Good, it can be turned off. Do you think there is any risk to existing
> ATA users with this patch applied if this is turned off?
With the new version, the only risk if this is turned off is the
change I've made to bestcomm.h. Other than that, you should get no
risk because none of the new
code is executed (mwdma_mask and udma_mask are set to 0 if the option
is turned off).
> Can you find any way to avoid this? This could be a performance drain.
Previous code had this, so I kept it. Things do seem to work OK
without it, so I've removed it...
> Is there any way to turn on/off DMA at runtime instead of CONFIG time?
You could use libata.dma=0 to force DMA off even if it's enabled at CONFIG time.
>> priv->ipb_period = 1000000000 / (ipb_freq / 1000);
>> priv->ata_regs = ata_regs;
>> + priv->ata_regs_pa = (struct mpc52xx_ata __iomem *) res_mem.start;
>
> I'm not fond of this. First off, it is *not* __iomem. It is physical
> address. It would be better to use the offset_of macro to add an offset
> to the physical base address. Doing it this way forces you to cast and
> sidestep the compile time checks for incorrect dereferences.
I'm afraid I'm not quite sure what you have in mind here, could you
please provide a pointer?
Thanks,
Tim
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 1050-mpc5200-add-ATA-DMA.patch --]
[-- Type: text/x-patch; name=1050-mpc5200-add-ATA-DMA.patch, Size: 22904 bytes --]
This patch adds MDMA/UDMA support (using BestComm for DMA) on the MPC5200
platform.
Based heavily on previous work by Freescale (Bernard Kuhn, John Rigby)
and Domen Puncer.
Using a SanDisk Extreme IV CF card I get read speeds of approximately
26.70 MB/sec.
The BestComm ATA task priority was changed to maximum in bestcomm_priv.h;
this fixes a deadlock issue I was experiencing when heavy DMA was
occuring on both the ATA and Ethernet BestComm tasks, e.g. when
downloading a large file over a LAN to disk.
There's also what I believe to be a hardware bug if you have high levels
of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
the address bus seems to sometimes get corrupted with ATA commands while
the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
I've asked Freescale about this but have not received a reply yet -- if
anybody from Freescale has any ideas please contact me; I can supply some
analyzer traces if needed. Therefore, for now, do not enable DMA if you
need reliable LocalPlus Bus unless you do a fixup in your driver as
follows:
Locking example:
while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
{
struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
if(bcom_buffer_done_2(tsk))
return 1;
}
return 0;
(Save the return value to `flags`)
Unlocking example:
if(flags == 0)
clear_bit(0, &pata_mpc52xx_ata_dma_lock);
Comments and testing would of course be very welcome.
Thanks,
Signed-off-by: Tim Yamin <plasm@roo.me.uk>
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h 2008-07-02 12:48:14.000000000 +0100
@@ -16,8 +16,8 @@
struct bcom_ata_bd {
u32 status;
- u32 dst_pa;
u32 src_pa;
+ u32 dst_pa;
};
extern struct bcom_task *
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-07-02 12:48:14.000000000 +0100
@@ -330,11 +330,16 @@ bcom_engine_init(void)
/* Init 'always' initiator */
out_8(&bcom_eng->regs->ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
+ /* If ATA DMA is enabled, always turn prefetch off (it breaks things) */
+#ifndef CONFIG_PATA_MPC52xx_DMA
/* Disable COMM Bus Prefetch on the original 5200; it's broken */
if ((mfspr(SPRN_SVR) & MPC5200_SVR_MASK) == MPC5200_SVR) {
+#endif
regval = in_be16(&bcom_eng->regs->PtdCntrl);
out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
+#ifndef CONFIG_PATA_MPC52xx_DMA
}
+#endif
/* Init lock */
spin_lock_init(&bcom_eng->lock);
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-07-02 12:48:14.000000000 +0100
@@ -140,15 +140,29 @@ bcom_queue_full(struct bcom_task *tsk)
}
/**
+ * bcom_get_bd - Get a BD from the queue
+ * @tsk: The BestComm task structure
+ * index: Index of the BD to fetch
+ */
+static inline struct bcom_bd
+*bcom_get_bd(struct bcom_task *tsk, unsigned int index)
+{
+ return ((void *) tsk->bd) + (index * tsk->bd_size);
+}
+
+/**
* bcom_buffer_done - Checks if a BestComm
* @tsk: The BestComm task structure
*/
static inline int
bcom_buffer_done(struct bcom_task *tsk)
{
+ struct bcom_bd *bd;
if (bcom_queue_empty(tsk))
return 0;
- return !(tsk->bd[tsk->outdex].status & BCOM_BD_READY);
+
+ bd = bcom_get_bd(tsk, tsk->outdex);
+ return !(bd->status & BCOM_BD_READY);
}
/**
@@ -160,16 +174,21 @@ bcom_buffer_done(struct bcom_task *tsk)
static inline struct bcom_bd *
bcom_prepare_next_buffer(struct bcom_task *tsk)
{
- tsk->bd[tsk->index].status = 0; /* cleanup last status */
- return &tsk->bd[tsk->index];
+ struct bcom_bd *bd;
+
+ bd = bcom_get_bd(tsk, tsk->index);
+ bd->status = 0; /* cleanup last status */
+ return bd;
}
static inline void
bcom_submit_next_buffer(struct bcom_task *tsk, void *cookie)
{
+ struct bcom_bd *bd = bcom_get_bd(tsk, tsk->index);
+
tsk->cookie[tsk->index] = cookie;
mb(); /* ensure the bd is really up-to-date */
- tsk->bd[tsk->index].status |= BCOM_BD_READY;
+ bd->status |= BCOM_BD_READY;
tsk->index = _bcom_next_index(tsk);
if (tsk->flags & BCOM_FLAGS_ENABLE_TASK)
bcom_enable(tsk);
@@ -179,10 +198,12 @@ static inline void *
bcom_retrieve_buffer(struct bcom_task *tsk, u32 *p_status, struct bcom_bd **p_bd)
{
void *cookie = tsk->cookie[tsk->outdex];
+ struct bcom_bd *bd = bcom_get_bd(tsk, tsk->outdex);
+
if (p_status)
- *p_status = tsk->bd[tsk->outdex].status;
+ *p_status = bd->status;
if (p_bd)
- *p_bd = &tsk->bd[tsk->outdex];
+ *p_bd = bd;
tsk->outdex = _bcom_next_outdex(tsk);
return cookie;
}
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-07-02 12:48:14.000000000 +0100
@@ -198,8 +198,8 @@ struct bcom_task_header {
#define BCOM_IPR_SCTMR_1 2
#define BCOM_IPR_FEC_RX 6
#define BCOM_IPR_FEC_TX 5
-#define BCOM_IPR_ATA_RX 4
-#define BCOM_IPR_ATA_TX 3
+#define BCOM_IPR_ATA_RX 7
+#define BCOM_IPR_ATA_TX 7
#define BCOM_IPR_SCPCI_RX 2
#define BCOM_IPR_SCPCI_TX 2
#define BCOM_IPR_PSC3_RX 2
diff -urp linux-2.6.26-rc6/drivers/ata/Kconfig linux-2.6.26-rc6-ata/drivers/ata/Kconfig
--- linux-2.6.26-rc6/drivers/ata/Kconfig 2008-07-02 12:51:27.000000000 +0100
+++ linux-2.6.26-rc6-ata/drivers/ata/Kconfig 2008-07-02 12:47:15.000000000 +0100
@@ -462,6 +462,15 @@ config PATA_MPC52xx
If unsure, say N.
+config PATA_MPC52xx_DMA
+ tristate "Freescale MPC52xx SoC internal IDE DMA"
+ depends on PATA_MPC52xx
+ help
+ This option enables support for DMA on the MPC52xx SoC PATA
+ controller.
+
+ If unsure, say N.
+
config PATA_MPIIX
tristate "Intel PATA MPIIX support"
depends on PCI
diff -urp linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c linux-2.6.26-rc6-ata/drivers/ata/pata_mpc52xx.c
--- linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c 2008-07-02 12:51:27.000000000 +0100
+++ linux-2.6.26-rc6-ata/drivers/ata/pata_mpc52xx.c 2008-07-02 12:47:14.000000000 +0100
@@ -6,6 +6,9 @@
* Copyright (C) 2006 Sylvain Munaut <tnt@246tNt.com>
* Copyright (C) 2003 Mipsys - Benjamin Herrenschmidt
*
+ * UDMA support based on patches by Freescale (Bernard Kuhn, John Rigby),
+ * Domen Puncer and Tim Yamin.
+ *
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed "as is" without any warranty of any
* kind, whether express or implied.
@@ -17,28 +20,47 @@
#include <linux/delay.h>
#include <linux/libata.h>
+#include <asm/cacheflush.h>
#include <asm/types.h>
#include <asm/prom.h>
#include <asm/of_platform.h>
#include <asm/mpc52xx.h>
+#include <sysdev/bestcomm/bestcomm.h>
+#include <sysdev/bestcomm/bestcomm_priv.h>
+#include <sysdev/bestcomm/ata.h>
#define DRV_NAME "mpc52xx_ata"
#define DRV_VERSION "0.1.2"
-
/* Private structures used by the driver */
struct mpc52xx_ata_timings {
u32 pio1;
u32 pio2;
+ u32 mdma1;
+ u32 mdma2;
+ u32 udma1;
+ u32 udma2;
+ u32 udma3;
+ u32 udma4;
+ u32 udma5;
+ int using_udma;
};
struct mpc52xx_ata_priv {
unsigned int ipb_period;
struct mpc52xx_ata __iomem * ata_regs;
+ struct mpc52xx_ata *ata_regs_pa;
int ata_irq;
struct mpc52xx_ata_timings timings[2];
int csel;
+
+ /* DMA */
+ struct bcom_task *dmatsk;
+ const struct udmaspec *udmaspec;
+ const struct mdmaspec *mdmaspec;
+ int mpc52xx_ata_dma_last_write;
+ int waiting_for_dma;
};
@@ -53,6 +75,95 @@ static const int ataspec_ta[5] = { 35
#define CALC_CLKCYC(c,v) ((((v)+(c)-1)/(c)))
+/* ======================================================================== */
+
+/* ATAPI-4 MDMA specs (in clocks) */
+struct mdmaspec {
+ u32 t0M;
+ u32 td;
+ u32 th;
+ u32 tj;
+ u32 tkw;
+ u32 tm;
+ u32 tn;
+};
+
+static const struct mdmaspec mdmaspec66[3] = {
+ { .t0M = 32, .td = 15, .th = 2, .tj = 2, .tkw = 15, .tm = 4, .tn = 1 },
+ { .t0M = 10, .td = 6, .th = 1, .tj = 1, .tkw = 4, .tm = 2, .tn = 1 },
+ { .t0M = 8, .td = 5, .th = 1, .tj = 1, .tkw = 2, .tm = 2, .tn = 1 },
+};
+
+static const struct mdmaspec mdmaspec132[3] = {
+ { .t0M = 64, .td = 29, .th = 3, .tj = 3, .tkw = 29, .tm = 7, .tn = 2 },
+ { .t0M = 20, .td = 11, .th = 2, .tj = 1, .tkw = 7, .tm = 4, .tn = 1 },
+ { .t0M = 16, .td = 10, .th = 2, .tj = 1, .tkw = 4, .tm = 4, .tn = 1 },
+};
+
+/* ATAPI-4 UDMA specs (in clocks) */
+struct udmaspec {
+ u32 tcyc;
+ u32 t2cyc;
+ u32 tds;
+ u32 tdh;
+ u32 tdvs;
+ u32 tdvh;
+ u32 tfs;
+ u32 tli;
+ u32 tmli;
+ u32 taz;
+ u32 tzah;
+ u32 tenv;
+ u32 tsr;
+ u32 trfs;
+ u32 trp;
+ u32 tack;
+ u32 tss;
+};
+
+static const struct udmaspec udmaspec66[6] = {
+ { .tcyc = 8, .t2cyc = 16, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
+ .tfs = 16, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 3, .trfs = 5, .trp = 11, .tack = 2, .tss = 4 },
+ { .tcyc = 5, .t2cyc = 11, .tds = 1, .tdh = 1, .tdvs = 4, .tdvh = 1,
+ .tfs = 14, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 5, .trp = 9, .tack = 2, .tss = 4 },
+ { .tcyc = 4, .t2cyc = 8, .tds = 1, .tdh = 1, .tdvs = 3, .tdvh = 1,
+ .tfs = 12, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4 },
+ { .tcyc = 3, .t2cyc = 6, .tds = 1, .tdh = 1, .tdvs = 2, .tdvh = 1,
+ .tfs = 9, .tli = 7, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4 },
+ { .tcyc = 2, .t2cyc = 4, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
+ .tfs = 8, .tli = 8, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4 },
+ { .tcyc = 2, .t2cyc = 2, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
+ .tfs = 6, .tli = 5, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 6, .tack = 2, .tss = 4 },
+};
+
+static const struct udmaspec udmaspec132[6] = {
+ { .tcyc = 15, .t2cyc = 31, .tds = 2, .tdh = 1, .tdvs = 10, .tdvh = 1,
+ .tfs = 30, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 7, .trfs = 10, .trp = 22, .tack = 3, .tss = 7 },
+ { .tcyc = 10, .t2cyc = 21, .tds = 2, .tdh = 1, .tdvs = 7, .tdvh = 1,
+ .tfs = 27, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 4, .trfs = 10, .trp = 17, .tack = 3, .tss = 7 },
+ { .tcyc = 6, .t2cyc = 12, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
+ .tfs = 23, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 3, .trfs = 8, .trp = 14, .tack = 3, .tss = 7 },
+ { .tcyc = 7, .t2cyc = 12, .tds = 1, .tdh = 1, .tdvs = 3, .tdvh = 1,
+ .tfs = 15, .tli = 13, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 3, .trfs = 8, .trp = 14, .tack = 3, .tss = 7 },
+ { .tcyc = 2, .t2cyc = 5, .tds = 0, .tdh = 0, .tdvs = 1, .tdvh = 1,
+ .tfs = 16, .tli = 14, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 7, .trp = 13, .tack = 2, .tss = 6 },
+ { .tcyc = 3, .t2cyc = 6, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
+ .tfs = 12, .tli = 10, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 3, .trfs = 7, .trp = 12, .tack = 3, .tss = 7 },
+};
+
+/* ======================================================================== */
/* Bit definitions inside the registers */
#define MPC52xx_ATA_HOSTCONF_SMR 0x80000000UL /* State machine reset */
@@ -66,6 +165,7 @@ static const int ataspec_ta[5] = { 35
#define MPC52xx_ATA_HOSTSTAT_WERR 0x01000000UL /* Write Error */
#define MPC52xx_ATA_FIFOSTAT_EMPTY 0x01 /* FIFO Empty */
+#define MPC52xx_ATA_FIFOSTAT_ERROR 0x40 /* FIFO Error */
#define MPC52xx_ATA_DMAMODE_WRITE 0x01 /* Write DMA */
#define MPC52xx_ATA_DMAMODE_READ 0x02 /* Read DMA */
@@ -75,6 +175,8 @@ static const int ataspec_ta[5] = { 35
#define MPC52xx_ATA_DMAMODE_FR 0x20 /* FIFO Reset */
#define MPC52xx_ATA_DMAMODE_HUT 0x40 /* Host UDMA burst terminate */
+#define MAX_DMA_BUFFERS 128
+#define MAX_DMA_BUFFER_SIZE 0x20000u
/* Structure of the hardware registers */
struct mpc52xx_ata {
@@ -133,6 +235,9 @@ struct mpc52xx_ata {
u8 reserved21[2];
};
+/* BestComm locking */
+unsigned long pata_mpc52xx_ata_dma_lock;
+struct bcom_task *pata_mpc52xx_ata_dma_task;
/* ======================================================================== */
/* Aux fns */
@@ -165,6 +270,41 @@ mpc52xx_ata_compute_pio_timings(struct m
return 0;
}
+static int
+mpc52xx_ata_compute_mdma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
+{
+ struct mpc52xx_ata_timings *timing = &priv->timings[dev];
+ const struct mdmaspec *s = &priv->mdmaspec[speed];
+
+ if (speed < 0 || speed > 2)
+ return -EINVAL;
+
+ timing->mdma1 = (s->t0M << 24) | (s->td << 16) | (s->tkw << 8) | (s->tm);
+ timing->mdma2 = (s->th << 24) | (s->tj << 16) | (s->tn << 8);
+ timing->using_udma = 0;
+
+ return 0;
+}
+
+static int
+mpc52xx_ata_compute_udma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
+{
+ struct mpc52xx_ata_timings *timing = &priv->timings[dev];
+ const struct udmaspec *s = &priv->udmaspec[speed];
+
+ if (speed < 0 || speed > 2)
+ return -EINVAL;
+
+ timing->udma1 = (s->t2cyc << 24) | (s->tcyc << 16) | (s->tds << 8) | (s->tdh);
+ timing->udma2 = (s->tdvs << 24) | (s->tdvh << 16) | (s->tfs << 8) | (s->tli);
+ timing->udma3 = (s->tmli << 24) | (s->taz << 16) | (s->tenv << 8) | (s->tsr);
+ timing->udma4 = (s->tss << 24) | (s->trfs << 16) | (s->trp << 8) | (s->tack);
+ timing->udma5 = (s->tzah << 24);
+ timing->using_udma = 1;
+
+ return 0;
+}
+
static void
mpc52xx_ata_apply_timings(struct mpc52xx_ata_priv *priv, int device)
{
@@ -173,14 +313,13 @@ mpc52xx_ata_apply_timings(struct mpc52xx
out_be32(®s->pio1, timing->pio1);
out_be32(®s->pio2, timing->pio2);
- out_be32(®s->mdma1, 0);
- out_be32(®s->mdma2, 0);
- out_be32(®s->udma1, 0);
- out_be32(®s->udma2, 0);
- out_be32(®s->udma3, 0);
- out_be32(®s->udma4, 0);
- out_be32(®s->udma5, 0);
-
+ out_be32(®s->mdma1, timing->mdma1);
+ out_be32(®s->mdma2, timing->mdma2);
+ out_be32(®s->udma1, timing->udma1);
+ out_be32(®s->udma2, timing->udma2);
+ out_be32(®s->udma3, timing->udma3);
+ out_be32(®s->udma4, timing->udma4);
+ out_be32(®s->udma5, timing->udma5);
priv->csel = device;
}
@@ -245,6 +384,29 @@ mpc52xx_ata_set_piomode(struct ata_port
mpc52xx_ata_apply_timings(priv, adev->devno);
}
static void
+mpc52xx_ata_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ int rv;
+
+ if (adev->dma_mode >= XFER_UDMA_0) {
+ int dma = adev->dma_mode - XFER_UDMA_0;
+ rv = mpc52xx_ata_compute_udma_timings(priv, adev->devno, dma);
+ } else {
+ int dma = adev->dma_mode - XFER_MW_DMA_0;
+ rv = mpc52xx_ata_compute_mdma_timings(priv, adev->devno, dma);
+ }
+
+ if (rv) {
+ printk(KERN_ERR DRV_NAME
+ ": Trying to select invalid DMA mode %d\n",
+ adev->dma_mode);
+ return;
+ }
+
+ mpc52xx_ata_apply_timings(priv, adev->devno);
+}
+static void
mpc52xx_ata_dev_select(struct ata_port *ap, unsigned int device)
{
struct mpc52xx_ata_priv *priv = ap->host->private_data;
@@ -255,16 +416,190 @@ mpc52xx_ata_dev_select(struct ata_port *
ata_sff_dev_select(ap,device);
}
+static int
+mpc52xx_ata_build_dmatable(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ struct mpc52xx_ata *regs_pa = priv->ata_regs_pa;
+ unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE), si;
+ struct scatterlist *sg;
+ int count = 0;
+
+ if (read)
+ bcom_ata_rx_prepare(priv->dmatsk);
+ else
+ bcom_ata_tx_prepare(priv->dmatsk);
+
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
+ dma_addr_t cur_addr = sg_dma_address(sg);
+ u32 cur_len = sg_dma_len(sg);
+
+ while (cur_len) {
+ unsigned int tc = min(cur_len, MAX_DMA_BUFFER_SIZE);
+ struct bcom_ata_bd *bd = (struct bcom_ata_bd *) bcom_prepare_next_buffer(priv->dmatsk);
+
+ if (read) {
+ bd->status = tc;
+ bd->src_pa = (__force u32) ®s_pa->fifo_data;
+ bd->dst_pa = (__force u32) cur_addr;
+ } else {
+ bd->status = tc;
+ bd->src_pa = (__force u32) cur_addr;
+ bd->dst_pa = (__force u32) ®s_pa->fifo_data;
+ }
+
+ bcom_submit_next_buffer(priv->dmatsk, NULL);
+
+ cur_addr += tc;
+ cur_len -= tc;
+ count++;
+
+ if (count > MAX_DMA_BUFFERS) {
+ printk(KERN_ALERT "%s: %i dma table"
+ "too small\n", __func__, __LINE__);
+ goto use_pio_instead;
+ }
+ }
+ }
+ return 1;
+
+use_pio_instead:
+ bcom_ata_reset_bd(priv->dmatsk);
+ return 0;
+}
+
+static void
+mpc52xx_bmdma_setup(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ struct mpc52xx_ata __iomem *regs = priv->ata_regs;
+
+ unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE);
+ u8 dma_mode;
+
+ if (!mpc52xx_ata_build_dmatable(qc))
+ printk(KERN_ALERT "%s: %i, return 1?\n", __func__, __LINE__);
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ printk(KERN_ALERT "%s: FIFO error detected: 0x%02x!\n",
+ __func__, in_8(&priv->ata_regs->fifo_status));
+
+ if (read) {
+ dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_READ |
+ MPC52xx_ATA_DMAMODE_FE;
+
+ /* Setup FIFO if direction changed */
+ if (priv->mpc52xx_ata_dma_last_write != 0) {
+ priv->mpc52xx_ata_dma_last_write = 0;
+
+ /* Configure FIFO with granularity to 7 */
+ out_8(®s->fifo_control, 7);
+ out_be16(®s->fifo_alarm, 128);
+
+ /* Set FIFO Reset bit (FR) */
+ out_8(®s->dma_mode, MPC52xx_ATA_DMAMODE_FR);
+ }
+ } else {
+ dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_WRITE;
+
+ /* Setup FIFO if direction changed */
+ if (priv->mpc52xx_ata_dma_last_write != 1) {
+ priv->mpc52xx_ata_dma_last_write = 1;
+
+ /* Configure FIFO with granularity to 4 */
+ out_8(®s->fifo_control, 4);
+ out_be16(®s->fifo_alarm, 128);
+ }
+ }
+
+ if (priv->timings[qc->dev->devno].using_udma)
+ dma_mode |= MPC52xx_ATA_DMAMODE_UDMA;
+
+ out_8(®s->dma_mode, dma_mode);
+ priv->waiting_for_dma = ATA_DMA_ACTIVE;
+
+ ata_wait_idle(ap);
+ ap->ops->sff_exec_command(ap, &qc->tf);
+}
+
+static void
+mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ /* LocalBus lock */
+ while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
+ ;
+
+ bcom_set_task_auto_start(priv->dmatsk->tasknum, priv->dmatsk->tasknum);
+ bcom_enable(priv->dmatsk);
+}
+
+static void
+mpc52xx_bmdma_stop(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ bcom_disable(priv->dmatsk);
+ bcom_ata_reset_bd(priv->dmatsk);
+
+ /* LocalBus unlock*/
+ clear_bit(0, &pata_mpc52xx_ata_dma_lock);
+
+ priv->waiting_for_dma = 0;
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ printk(KERN_ALERT "%s: FIFO error detected: 0x%02x!\n",
+ __func__, in_8(&priv->ata_regs->fifo_status));
+}
+
+static u8
+mpc52xx_bmdma_status(struct ata_port *ap)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR) {
+ printk(KERN_ALERT "%s: FIFO error detected: 0x%02x!\n",
+ __func__, in_8(&priv->ata_regs->fifo_status));
+ return priv->waiting_for_dma | ATA_DMA_ERR;
+ }
+
+ return priv->waiting_for_dma;
+}
+
+static irqreturn_t
+mpc52xx_ata_task_irq(int irq, void *vpriv)
+{
+ struct mpc52xx_ata_priv *priv = vpriv;
+ priv->waiting_for_dma |= ATA_DMA_INTR;
+
+ return IRQ_HANDLED;
+}
+
static struct scsi_host_template mpc52xx_ata_sht = {
ATA_PIO_SHT(DRV_NAME),
};
static struct ata_port_operations mpc52xx_ata_port_ops = {
.inherits = &ata_sff_port_ops,
- .sff_dev_select = mpc52xx_ata_dev_select,
- .cable_detect = ata_cable_40wire,
+
.set_piomode = mpc52xx_ata_set_piomode,
- .post_internal_cmd = ATA_OP_NULL,
+ .set_dmamode = mpc52xx_ata_set_dmamode,
+ .sff_dev_select = mpc52xx_ata_dev_select,
+
+ .bmdma_setup = mpc52xx_bmdma_setup,
+ .bmdma_start = mpc52xx_bmdma_start,
+ .bmdma_stop = mpc52xx_bmdma_stop,
+ .bmdma_status = mpc52xx_bmdma_status,
+
+ .qc_prep = ata_noop_qc_prep,
};
static int __devinit
@@ -281,9 +615,14 @@ mpc52xx_ata_init_one(struct device *dev,
ap = host->ports[0];
ap->flags |= ATA_FLAG_SLAVE_POSS;
- ap->pio_mask = 0x1f; /* Up to PIO4 */
- ap->mwdma_mask = 0x00; /* No MWDMA */
- ap->udma_mask = 0x00; /* No UDMA */
+ ap->pio_mask = ATA_PIO4; /* Up to PIO4 */
+#ifdef CONFIG_PATA_MPC52xx_DMA
+ ap->mwdma_mask = ATA_MWDMA2; /* Up to MWDMA2 */
+ ap->udma_mask = ATA_UDMA2; /* Up to UDMA2 */
+#else
+ ap->mwdma_mask = 0x00; /* No MWDMA */
+ ap->udma_mask = 0x00; /* No UDMA */
+#endif
ap->ops = &mpc52xx_ata_port_ops;
host->private_data = priv;
@@ -333,7 +672,7 @@ mpc52xx_ata_probe(struct of_device *op,
int ata_irq;
struct mpc52xx_ata __iomem *ata_regs;
struct mpc52xx_ata_priv *priv;
- int rv;
+ int rv, ret, task_irq;
/* Get ipb frequency */
ipb_freq = mpc52xx_find_ipb_freq(op->node);
@@ -389,8 +728,34 @@ mpc52xx_ata_probe(struct of_device *op,
priv->ipb_period = 1000000000 / (ipb_freq / 1000);
priv->ata_regs = ata_regs;
+ priv->ata_regs_pa = (struct mpc52xx_ata *) res_mem.start;
priv->ata_irq = ata_irq;
priv->csel = -1;
+ priv->mpc52xx_ata_dma_last_write = -1;
+
+ if (ipb_freq/1000000 == 66) {
+ priv->mdmaspec = mdmaspec66;
+ priv->udmaspec = udmaspec66;
+ } else {
+ priv->mdmaspec = mdmaspec132;
+ priv->udmaspec = udmaspec132;
+ }
+
+ pata_mpc52xx_ata_dma_lock = 0;
+ priv->dmatsk = bcom_ata_init(MAX_DMA_BUFFERS, MAX_DMA_BUFFER_SIZE);
+ pata_mpc52xx_ata_dma_task = priv->dmatsk;
+ if (!priv->dmatsk) {
+ printk(KERN_ALERT "%s: %i\n", __func__, __LINE__);
+ rv = -ENOMEM;
+ goto err;
+ }
+
+ task_irq = bcom_get_task_irq(priv->dmatsk);
+ ret = request_irq(task_irq, &mpc52xx_ata_task_irq, IRQF_DISABLED,
+ "ATA task", priv);
+ if (ret)
+ printk(KERN_ALERT "%s: request_irq failed with: "
+ "%i\n", __func__, ret);
/* Init the hw */
rv = mpc52xx_ata_hw_init(priv);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-07-02 12:48 ` Tim Yamin
@ 2008-07-02 17:30 ` Grant Likely
2008-07-03 15:35 ` Tim Yamin
0 siblings, 1 reply; 9+ messages in thread
From: Grant Likely @ 2008-07-02 17:30 UTC (permalink / raw)
To: Tim Yamin; +Cc: linuxppc-dev
On Wed, Jul 02, 2008 at 01:48:39PM +0100, Tim Yamin wrote:
> Hi Grant,
>
> Thanks for the feedback. New version is attached.
Your welcome. The patch is looking pretty good. Some more comments
below.
> > > -#define BCOM_IPR_ATA_RX 4
> > > -#define BCOM_IPR_ATA_TX 3
> > > +#define BCOM_IPR_ATA_RX 7
> > > +#define BCOM_IPR_ATA_TX 7
> >
> > Is this a bug fix? If so, please put it into a separate patch.
>
> I suppose so, yes. If Ethernet has higher priority than ATA, you can
> get a deadlock if you try and download a large file over a LAN to
> disk, for example. But given that nothing other than this patch uses
> BestComm for ATA do you have any specific reason to split it out into
> another patch?
I know that only ATA uses this; but it is nice to have fixes to things
that are obviously wrong in existing code to be split into their own
patches. That way, even if the ATA patch gets backed out, the bug fix
will remain.
> >> priv->ipb_period = 1000000000 / (ipb_freq / 1000);
> >> priv->ata_regs = ata_regs;
> >> + priv->ata_regs_pa = (struct mpc52xx_ata __iomem *) res_mem.start;
> >
> > I'm not fond of this. First off, it is *not* __iomem. It is physical
> > address. It would be better to use the offset_of macro to add an offset
> > to the physical base address. Doing it this way forces you to cast and
> > sidestep the compile time checks for incorrect dereferences.
>
> I'm afraid I'm not quite sure what you have in mind here, could you
> please provide a pointer?
See below comments
>
> Thanks,
>
> Tim
> This patch adds MDMA/UDMA support (using BestComm for DMA) on the MPC5200
> platform.
>
> Based heavily on previous work by Freescale (Bernard Kuhn, John Rigby)
> and Domen Puncer.
>
> Using a SanDisk Extreme IV CF card I get read speeds of approximately
> 26.70 MB/sec.
>
> The BestComm ATA task priority was changed to maximum in bestcomm_priv.h;
> this fixes a deadlock issue I was experiencing when heavy DMA was
> occuring on both the ATA and Ethernet BestComm tasks, e.g. when
> downloading a large file over a LAN to disk.
>
> There's also what I believe to be a hardware bug if you have high levels
> of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
> the address bus seems to sometimes get corrupted with ATA commands while
> the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
>
> I've asked Freescale about this but have not received a reply yet -- if
> anybody from Freescale has any ideas please contact me; I can supply some
> analyzer traces if needed. Therefore, for now, do not enable DMA if you
> need reliable LocalPlus Bus unless you do a fixup in your driver as
> follows:
>
> Locking example:
>
> while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
> {
> struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
>
> if(bcom_buffer_done_2(tsk))
> return 1;
> }
>
> return 0;
>
> (Save the return value to `flags`)
>
> Unlocking example:
>
> if(flags == 0)
> clear_bit(0, &pata_mpc52xx_ata_dma_lock);
>
> Comments and testing would of course be very welcome.
>
> Thanks,
>
> Signed-off-by: Tim Yamin <plasm@roo.me.uk>
>
> diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h
> --- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h 2008-04-17 03:49:44.000000000 +0100
> +++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h 2008-07-02 12:48:14.000000000 +0100
> @@ -16,8 +16,8 @@
>
> struct bcom_ata_bd {
> u32 status;
> - u32 dst_pa;
> u32 src_pa;
> + u32 dst_pa;
> };
This also looks like an obvious bug fix. A separate patch would be nice
here.
>
> extern struct bcom_task *
> diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c
> --- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-04-17 03:49:44.000000000 +0100
> +++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-07-02 12:48:14.000000000 +0100
> @@ -330,11 +330,16 @@ bcom_engine_init(void)
> /* Init 'always' initiator */
> out_8(&bcom_eng->regs->ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
>
> + /* If ATA DMA is enabled, always turn prefetch off (it breaks things) */
> +#ifndef CONFIG_PATA_MPC52xx_DMA
> /* Disable COMM Bus Prefetch on the original 5200; it's broken */
> if ((mfspr(SPRN_SVR) & MPC5200_SVR_MASK) == MPC5200_SVR) {
> +#endif
> regval = in_be16(&bcom_eng->regs->PtdCntrl);
> out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
> +#ifndef CONFIG_PATA_MPC52xx_DMA
> }
> +#endif
>
> /* Init lock */
> spin_lock_init(&bcom_eng->lock);
> diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h
> --- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-17 03:49:44.000000000 +0100
> +++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-07-02 12:48:14.000000000 +0100
> @@ -140,15 +140,29 @@ bcom_queue_full(struct bcom_task *tsk)
> }
>
> /**
> + * bcom_get_bd - Get a BD from the queue
> + * @tsk: The BestComm task structure
> + * index: Index of the BD to fetch
> + */
> +static inline struct bcom_bd
> +*bcom_get_bd(struct bcom_task *tsk, unsigned int index)
> +{
> + return ((void *) tsk->bd) + (index * tsk->bd_size);
Since the size of a bd is not a given, change the type of bcom_task.bd
from 'struct bcom_bd *' to simply 'void *'. That will eliminate the
need for this cast.
> +}
> +
> +/**
> * bcom_buffer_done - Checks if a BestComm
> * @tsk: The BestComm task structure
> */
> static inline int
> bcom_buffer_done(struct bcom_task *tsk)
> {
> + struct bcom_bd *bd;
> if (bcom_queue_empty(tsk))
> return 0;
> - return !(tsk->bd[tsk->outdex].status & BCOM_BD_READY);
> +
> + bd = bcom_get_bd(tsk, tsk->outdex);
> + return !(bd->status & BCOM_BD_READY);
> }
>
> /**
> @@ -160,16 +174,21 @@ bcom_buffer_done(struct bcom_task *tsk)
> static inline struct bcom_bd *
> bcom_prepare_next_buffer(struct bcom_task *tsk)
> {
> - tsk->bd[tsk->index].status = 0; /* cleanup last status */
> - return &tsk->bd[tsk->index];
> + struct bcom_bd *bd;
> +
> + bd = bcom_get_bd(tsk, tsk->index);
> + bd->status = 0; /* cleanup last status */
> + return bd;
> }
>
> static inline void
> bcom_submit_next_buffer(struct bcom_task *tsk, void *cookie)
> {
> + struct bcom_bd *bd = bcom_get_bd(tsk, tsk->index);
> +
> tsk->cookie[tsk->index] = cookie;
> mb(); /* ensure the bd is really up-to-date */
> - tsk->bd[tsk->index].status |= BCOM_BD_READY;
> + bd->status |= BCOM_BD_READY;
> tsk->index = _bcom_next_index(tsk);
> if (tsk->flags & BCOM_FLAGS_ENABLE_TASK)
> bcom_enable(tsk);
> @@ -179,10 +198,12 @@ static inline void *
> bcom_retrieve_buffer(struct bcom_task *tsk, u32 *p_status, struct bcom_bd **p_bd)
> {
> void *cookie = tsk->cookie[tsk->outdex];
> + struct bcom_bd *bd = bcom_get_bd(tsk, tsk->outdex);
> +
> if (p_status)
> - *p_status = tsk->bd[tsk->outdex].status;
> + *p_status = bd->status;
> if (p_bd)
> - *p_bd = &tsk->bd[tsk->outdex];
> + *p_bd = bd;
> tsk->outdex = _bcom_next_outdex(tsk);
> return cookie;
> }
The bestcomm changes are much better. Can you please split them
into a separate patch? The common bestcomm stuff is logically separate
from the ATA driver changes.
> diff -urp linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c linux-2.6.26-rc6-ata/drivers/ata/pata_mpc52xx.c
> --- linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c 2008-07-02 12:51:27.000000000 +0100
> +++ linux-2.6.26-rc6-ata/drivers/ata/pata_mpc52xx.c 2008-07-02 12:47:14.000000000 +0100
> @@ -6,6 +6,9 @@
> * Copyright (C) 2006 Sylvain Munaut <tnt@246tNt.com>
> * Copyright (C) 2003 Mipsys - Benjamin Herrenschmidt
> *
> + * UDMA support based on patches by Freescale (Bernard Kuhn, John Rigby),
> + * Domen Puncer and Tim Yamin.
> + *
> * This file is licensed under the terms of the GNU General Public License
> * version 2. This program is licensed "as is" without any warranty of any
> * kind, whether express or implied.
> @@ -17,28 +20,47 @@
> #include <linux/delay.h>
> #include <linux/libata.h>
>
> +#include <asm/cacheflush.h>
> #include <asm/types.h>
> #include <asm/prom.h>
> #include <asm/of_platform.h>
> #include <asm/mpc52xx.h>
>
> +#include <sysdev/bestcomm/bestcomm.h>
> +#include <sysdev/bestcomm/bestcomm_priv.h>
> +#include <sysdev/bestcomm/ata.h>
>
> #define DRV_NAME "mpc52xx_ata"
> #define DRV_VERSION "0.1.2"
>
> -
> /* Private structures used by the driver */
> struct mpc52xx_ata_timings {
> u32 pio1;
> u32 pio2;
> + u32 mdma1;
> + u32 mdma2;
> + u32 udma1;
> + u32 udma2;
> + u32 udma3;
> + u32 udma4;
> + u32 udma5;
> + int using_udma;
> };
>
> struct mpc52xx_ata_priv {
> unsigned int ipb_period;
> struct mpc52xx_ata __iomem * ata_regs;
> + struct mpc52xx_ata *ata_regs_pa;
Change this to: 'phys_addr_t ata_regs_pa;'
> int ata_irq;
> struct mpc52xx_ata_timings timings[2];
> int csel;
> +
> + /* DMA */
> + struct bcom_task *dmatsk;
> + const struct udmaspec *udmaspec;
> + const struct mdmaspec *mdmaspec;
> + int mpc52xx_ata_dma_last_write;
> + int waiting_for_dma;
> };
>
>
> @@ -53,6 +75,95 @@ static const int ataspec_ta[5] = { 35
>
> #define CALC_CLKCYC(c,v) ((((v)+(c)-1)/(c)))
>
> +/* ======================================================================== */
> +
> +/* ATAPI-4 MDMA specs (in clocks) */
> +struct mdmaspec {
> + u32 t0M;
> + u32 td;
> + u32 th;
> + u32 tj;
> + u32 tkw;
> + u32 tm;
> + u32 tn;
> +};
> +
> +static const struct mdmaspec mdmaspec66[3] = {
> + { .t0M = 32, .td = 15, .th = 2, .tj = 2, .tkw = 15, .tm = 4, .tn = 1 },
> + { .t0M = 10, .td = 6, .th = 1, .tj = 1, .tkw = 4, .tm = 2, .tn = 1 },
> + { .t0M = 8, .td = 5, .th = 1, .tj = 1, .tkw = 2, .tm = 2, .tn = 1 },
> +};
> +
> +static const struct mdmaspec mdmaspec132[3] = {
> + { .t0M = 64, .td = 29, .th = 3, .tj = 3, .tkw = 29, .tm = 7, .tn = 2 },
> + { .t0M = 20, .td = 11, .th = 2, .tj = 1, .tkw = 7, .tm = 4, .tn = 1 },
> + { .t0M = 16, .td = 10, .th = 2, .tj = 1, .tkw = 4, .tm = 4, .tn = 1 },
> +};
> +
> +/* ATAPI-4 UDMA specs (in clocks) */
> +struct udmaspec {
> + u32 tcyc;
> + u32 t2cyc;
> + u32 tds;
> + u32 tdh;
> + u32 tdvs;
> + u32 tdvh;
> + u32 tfs;
> + u32 tli;
> + u32 tmli;
> + u32 taz;
> + u32 tzah;
> + u32 tenv;
> + u32 tsr;
> + u32 trfs;
> + u32 trp;
> + u32 tack;
> + u32 tss;
> +};
> +
> +static const struct udmaspec udmaspec66[6] = {
> + { .tcyc = 8, .t2cyc = 16, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
> + .tfs = 16, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 3, .trfs = 5, .trp = 11, .tack = 2, .tss = 4 },
> + { .tcyc = 5, .t2cyc = 11, .tds = 1, .tdh = 1, .tdvs = 4, .tdvh = 1,
> + .tfs = 14, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 2, .trfs = 5, .trp = 9, .tack = 2, .tss = 4 },
> + { .tcyc = 4, .t2cyc = 8, .tds = 1, .tdh = 1, .tdvs = 3, .tdvh = 1,
> + .tfs = 12, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4 },
> + { .tcyc = 3, .t2cyc = 6, .tds = 1, .tdh = 1, .tdvs = 2, .tdvh = 1,
> + .tfs = 9, .tli = 7, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4 },
> + { .tcyc = 2, .t2cyc = 4, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
> + .tfs = 8, .tli = 8, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4 },
> + { .tcyc = 2, .t2cyc = 2, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
> + .tfs = 6, .tli = 5, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 2, .trfs = 4, .trp = 6, .tack = 2, .tss = 4 },
> +};
Yes, I think this is more robust. One (very minor) nit. Can you line up
the elements and put the closing brackets on the same line? Makes it a
look less like noise. for example:
+ { .tcyc = 8, .t2cyc = 16, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
+ .tfs = 16, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 3, .trfs = 5, .trp = 11, .tack = 2, .tss = 4,
+ },
+ { .tcyc = 5, .t2cyc = 11, .tds = 1, .tdh = 1, .tdvs = 4, .tdvh = 1,
+ .tfs = 14, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 5, .trp = 9, .tack = 2, .tss = 4,
+ },
> +
> +static const struct udmaspec udmaspec132[6] = {
> + { .tcyc = 15, .t2cyc = 31, .tds = 2, .tdh = 1, .tdvs = 10, .tdvh = 1,
> + .tfs = 30, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
> + .tsr = 7, .trfs = 10, .trp = 22, .tack = 3, .tss = 7 },
> + { .tcyc = 10, .t2cyc = 21, .tds = 2, .tdh = 1, .tdvs = 7, .tdvh = 1,
> + .tfs = 27, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
> + .tsr = 4, .trfs = 10, .trp = 17, .tack = 3, .tss = 7 },
> + { .tcyc = 6, .t2cyc = 12, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
> + .tfs = 23, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
> + .tsr = 3, .trfs = 8, .trp = 14, .tack = 3, .tss = 7 },
> + { .tcyc = 7, .t2cyc = 12, .tds = 1, .tdh = 1, .tdvs = 3, .tdvh = 1,
> + .tfs = 15, .tli = 13, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
> + .tsr = 3, .trfs = 8, .trp = 14, .tack = 3, .tss = 7 },
> + { .tcyc = 2, .t2cyc = 5, .tds = 0, .tdh = 0, .tdvs = 1, .tdvh = 1,
> + .tfs = 16, .tli = 14, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
> + .tsr = 2, .trfs = 7, .trp = 13, .tack = 2, .tss = 6 },
> + { .tcyc = 3, .t2cyc = 6, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
> + .tfs = 12, .tli = 10, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
> + .tsr = 3, .trfs = 7, .trp = 12, .tack = 3, .tss = 7 },
> +};
> +
> +static int
> +mpc52xx_ata_compute_udma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
> +{
> + struct mpc52xx_ata_timings *timing = &priv->timings[dev];
> + const struct udmaspec *s = &priv->udmaspec[speed];
> +
> + if (speed < 0 || speed > 2)
> + return -EINVAL;
> +
> + timing->udma1 = (s->t2cyc << 24) | (s->tcyc << 16) | (s->tds << 8) | (s->tdh);
> + timing->udma2 = (s->tdvs << 24) | (s->tdvh << 16) | (s->tfs << 8) | (s->tli);
> + timing->udma3 = (s->tmli << 24) | (s->taz << 16) | (s->tenv << 8) | (s->tsr);
> + timing->udma4 = (s->tss << 24) | (s->trfs << 16) | (s->trp << 8) | (s->tack);
> + timing->udma5 = (s->tzah << 24);
Nit; lines over 80 characters, but it's not a big deal. Do whatever you
think looks the nicest.
> + timing->using_udma = 1;
> +
> + return 0;
> +}
> +
> static void
> mpc52xx_ata_apply_timings(struct mpc52xx_ata_priv *priv, int device)
> {
> @@ -173,14 +313,13 @@ mpc52xx_ata_apply_timings(struct mpc52xx
>
> out_be32(®s->pio1, timing->pio1);
> out_be32(®s->pio2, timing->pio2);
> - out_be32(®s->mdma1, 0);
> - out_be32(®s->mdma2, 0);
> - out_be32(®s->udma1, 0);
> - out_be32(®s->udma2, 0);
> - out_be32(®s->udma3, 0);
> - out_be32(®s->udma4, 0);
> - out_be32(®s->udma5, 0);
> -
> + out_be32(®s->mdma1, timing->mdma1);
> + out_be32(®s->mdma2, timing->mdma2);
> + out_be32(®s->udma1, timing->udma1);
> + out_be32(®s->udma2, timing->udma2);
> + out_be32(®s->udma3, timing->udma3);
> + out_be32(®s->udma4, timing->udma4);
> + out_be32(®s->udma5, timing->udma5);
> priv->csel = device;
> }
>
> @@ -245,6 +384,29 @@ mpc52xx_ata_set_piomode(struct ata_port
> mpc52xx_ata_apply_timings(priv, adev->devno);
> }
> static void
> +mpc52xx_ata_set_dmamode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> + int rv;
> +
> + if (adev->dma_mode >= XFER_UDMA_0) {
> + int dma = adev->dma_mode - XFER_UDMA_0;
> + rv = mpc52xx_ata_compute_udma_timings(priv, adev->devno, dma);
> + } else {
> + int dma = adev->dma_mode - XFER_MW_DMA_0;
> + rv = mpc52xx_ata_compute_mdma_timings(priv, adev->devno, dma);
> + }
> +
> + if (rv) {
> + printk(KERN_ERR DRV_NAME
> + ": Trying to select invalid DMA mode %d\n",
> + adev->dma_mode);
> + return;
> + }
> +
> + mpc52xx_ata_apply_timings(priv, adev->devno);
> +}
> +static void
> mpc52xx_ata_dev_select(struct ata_port *ap, unsigned int device)
> {
> struct mpc52xx_ata_priv *priv = ap->host->private_data;
> @@ -255,16 +416,190 @@ mpc52xx_ata_dev_select(struct ata_port *
> ata_sff_dev_select(ap,device);
> }
>
> +static int
> +mpc52xx_ata_build_dmatable(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> + struct mpc52xx_ata *regs_pa = priv->ata_regs_pa;
> + unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE), si;
> + struct scatterlist *sg;
> + int count = 0;
> +
> + if (read)
> + bcom_ata_rx_prepare(priv->dmatsk);
> + else
> + bcom_ata_tx_prepare(priv->dmatsk);
> +
> + for_each_sg(qc->sg, sg, qc->n_elem, si) {
> + dma_addr_t cur_addr = sg_dma_address(sg);
> + u32 cur_len = sg_dma_len(sg);
> +
> + while (cur_len) {
> + unsigned int tc = min(cur_len, MAX_DMA_BUFFER_SIZE);
> + struct bcom_ata_bd *bd = (struct bcom_ata_bd *) bcom_prepare_next_buffer(priv->dmatsk);
> +
> + if (read) {
> + bd->status = tc;
> + bd->src_pa = (__force u32) ®s_pa->fifo_data;
> + bd->dst_pa = (__force u32) cur_addr;
> + } else {
> + bd->status = tc;
> + bd->src_pa = (__force u32) cur_addr;
> + bd->dst_pa = (__force u32) ®s_pa->fifo_data;
These are the ugly casts of physical addresses stored in __iomem
pointers and then cast back into u32. ata_regs_pa should be phys_addr_t
and ®s_pa->fifo_data should be changed to:
ata_regs_pa + offsetof(struct mpc52xx_ata, fifo_data);
Truth be told; I'm not at all fond of using a structure to define
register offsets, but that is not a comment on this patch. All the
5200 code is already written that way, so it is better to be consistent.
> + }
> +
> + bcom_submit_next_buffer(priv->dmatsk, NULL);
> +
> + cur_addr += tc;
> + cur_len -= tc;
> + count++;
> +
> + if (count > MAX_DMA_BUFFERS) {
> + printk(KERN_ALERT "%s: %i dma table"
> + "too small\n", __func__, __LINE__);
> + goto use_pio_instead;
> + }
> + }
> + }
> + return 1;
> +
> +use_pio_instead:
It is good practise to indent goto labels with 1 space (not tab). Doing
so means that diff will do the right thing on displaying the c function
name when a line changes after the label.
> + bcom_ata_reset_bd(priv->dmatsk);
> + return 0;
> +}
> +
> +static void
> +mpc52xx_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> + struct mpc52xx_ata __iomem *regs = priv->ata_regs;
> +
> + unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE);
> + u8 dma_mode;
> +
> + if (!mpc52xx_ata_build_dmatable(qc))
> + printk(KERN_ALERT "%s: %i, return 1?\n", __func__, __LINE__);
You should use the dev_alert() macros instead of printk(). Ditto through
the rest of the file. Look in include/linux/device.h for the full list.
> +
> + /* Check FIFO is OK... */
> + if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
> + printk(KERN_ALERT "%s: FIFO error detected: 0x%02x!\n",
> + __func__, in_8(&priv->ata_regs->fifo_status));
> +
> + if (read) {
> + dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_READ |
> + MPC52xx_ATA_DMAMODE_FE;
> +
> + /* Setup FIFO if direction changed */
> + if (priv->mpc52xx_ata_dma_last_write != 0) {
> + priv->mpc52xx_ata_dma_last_write = 0;
> +
> + /* Configure FIFO with granularity to 7 */
> + out_8(®s->fifo_control, 7);
> + out_be16(®s->fifo_alarm, 128);
> +
> + /* Set FIFO Reset bit (FR) */
> + out_8(®s->dma_mode, MPC52xx_ATA_DMAMODE_FR);
> + }
> + } else {
> + dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_WRITE;
> +
> + /* Setup FIFO if direction changed */
> + if (priv->mpc52xx_ata_dma_last_write != 1) {
> + priv->mpc52xx_ata_dma_last_write = 1;
> +
> + /* Configure FIFO with granularity to 4 */
> + out_8(®s->fifo_control, 4);
> + out_be16(®s->fifo_alarm, 128);
> + }
> + }
> +
> + if (priv->timings[qc->dev->devno].using_udma)
> + dma_mode |= MPC52xx_ATA_DMAMODE_UDMA;
> +
> + out_8(®s->dma_mode, dma_mode);
> + priv->waiting_for_dma = ATA_DMA_ACTIVE;
> +
> + ata_wait_idle(ap);
> + ap->ops->sff_exec_command(ap, &qc->tf);
> +}
> +
> +static void
> +mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> +
> + /* LocalBus lock */
> + while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
> + ;
Need to be able to bail on timeout.
> +
> + bcom_set_task_auto_start(priv->dmatsk->tasknum, priv->dmatsk->tasknum);
> + bcom_enable(priv->dmatsk);
> +}
> +
> +static void
> +mpc52xx_bmdma_stop(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> +
> + bcom_disable(priv->dmatsk);
> + bcom_ata_reset_bd(priv->dmatsk);
> +
> + /* LocalBus unlock*/
> + clear_bit(0, &pata_mpc52xx_ata_dma_lock);
> +
> + priv->waiting_for_dma = 0;
> +
> + /* Check FIFO is OK... */
> + if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
> + printk(KERN_ALERT "%s: FIFO error detected: 0x%02x!\n",
> + __func__, in_8(&priv->ata_regs->fifo_status));
> +}
> +
> +static u8
> +mpc52xx_bmdma_status(struct ata_port *ap)
> +{
> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> +
> + /* Check FIFO is OK... */
> + if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR) {
> + printk(KERN_ALERT "%s: FIFO error detected: 0x%02x!\n",
> + __func__, in_8(&priv->ata_regs->fifo_status));
> + return priv->waiting_for_dma | ATA_DMA_ERR;
> + }
> +
> + return priv->waiting_for_dma;
> +}
> +
> +static irqreturn_t
> +mpc52xx_ata_task_irq(int irq, void *vpriv)
> +{
> + struct mpc52xx_ata_priv *priv = vpriv;
> + priv->waiting_for_dma |= ATA_DMA_INTR;
> +
> + return IRQ_HANDLED;
> +}
> +
> static struct scsi_host_template mpc52xx_ata_sht = {
> ATA_PIO_SHT(DRV_NAME),
> };
>
> static struct ata_port_operations mpc52xx_ata_port_ops = {
> .inherits = &ata_sff_port_ops,
> - .sff_dev_select = mpc52xx_ata_dev_select,
> - .cable_detect = ata_cable_40wire,
> +
> .set_piomode = mpc52xx_ata_set_piomode,
> - .post_internal_cmd = ATA_OP_NULL,
> + .set_dmamode = mpc52xx_ata_set_dmamode,
> + .sff_dev_select = mpc52xx_ata_dev_select,
> +
> + .bmdma_setup = mpc52xx_bmdma_setup,
> + .bmdma_start = mpc52xx_bmdma_start,
> + .bmdma_stop = mpc52xx_bmdma_stop,
> + .bmdma_status = mpc52xx_bmdma_status,
> +
> + .qc_prep = ata_noop_qc_prep,
> };
>
> static int __devinit
> @@ -281,9 +615,14 @@ mpc52xx_ata_init_one(struct device *dev,
>
> ap = host->ports[0];
> ap->flags |= ATA_FLAG_SLAVE_POSS;
> - ap->pio_mask = 0x1f; /* Up to PIO4 */
> - ap->mwdma_mask = 0x00; /* No MWDMA */
> - ap->udma_mask = 0x00; /* No UDMA */
> + ap->pio_mask = ATA_PIO4; /* Up to PIO4 */
> +#ifdef CONFIG_PATA_MPC52xx_DMA
> + ap->mwdma_mask = ATA_MWDMA2; /* Up to MWDMA2 */
> + ap->udma_mask = ATA_UDMA2; /* Up to UDMA2 */
> +#else
> + ap->mwdma_mask = 0x00; /* No MWDMA */
> + ap->udma_mask = 0x00; /* No UDMA */
> +#endif
ap->* values are already zeroed. You can drop the whole of the #else
side of this clause. (the old code didn't need to set them to zero).
> ap->ops = &mpc52xx_ata_port_ops;
> host->private_data = priv;
>
> @@ -333,7 +672,7 @@ mpc52xx_ata_probe(struct of_device *op,
> int ata_irq;
> struct mpc52xx_ata __iomem *ata_regs;
> struct mpc52xx_ata_priv *priv;
> - int rv;
> + int rv, ret, task_irq;
>
> /* Get ipb frequency */
> ipb_freq = mpc52xx_find_ipb_freq(op->node);
> @@ -389,8 +728,34 @@ mpc52xx_ata_probe(struct of_device *op,
>
> priv->ipb_period = 1000000000 / (ipb_freq / 1000);
> priv->ata_regs = ata_regs;
> + priv->ata_regs_pa = (struct mpc52xx_ata *) res_mem.start;
And this should simply be: priv->ata_regs_pa = res_mem.start;
> priv->ata_irq = ata_irq;
> priv->csel = -1;
> + priv->mpc52xx_ata_dma_last_write = -1;
> +
> + if (ipb_freq/1000000 == 66) {
> + priv->mdmaspec = mdmaspec66;
> + priv->udmaspec = udmaspec66;
> + } else {
> + priv->mdmaspec = mdmaspec132;
> + priv->udmaspec = udmaspec132;
> + }
> +
> + pata_mpc52xx_ata_dma_lock = 0;
> + priv->dmatsk = bcom_ata_init(MAX_DMA_BUFFERS, MAX_DMA_BUFFER_SIZE);
> + pata_mpc52xx_ata_dma_task = priv->dmatsk;
> + if (!priv->dmatsk) {
> + printk(KERN_ALERT "%s: %i\n", __func__, __LINE__);
> + rv = -ENOMEM;
> + goto err;
> + }
> +
> + task_irq = bcom_get_task_irq(priv->dmatsk);
> + ret = request_irq(task_irq, &mpc52xx_ata_task_irq, IRQF_DISABLED,
> + "ATA task", priv);
> + if (ret)
> + printk(KERN_ALERT "%s: request_irq failed with: "
> + "%i\n", __func__, ret);
>
> /* Init the hw */
> rv = mpc52xx_ata_hw_init(priv);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-07-02 17:30 ` Grant Likely
@ 2008-07-03 15:35 ` Tim Yamin
2008-07-03 23:47 ` Benjamin Herrenschmidt
2008-07-04 3:26 ` Grant Likely
0 siblings, 2 replies; 9+ messages in thread
From: Tim Yamin @ 2008-07-03 15:35 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]
On Wed, Jul 2, 2008 at 6:30 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> I know that only ATA uses this; but it is nice to have fixes to things
> that are obviously wrong in existing code to be split into their own
> patches. That way, even if the ATA patch gets backed out, the bug fix
> will remain.
Ok, so I've split the patch up into two pieces now...
>> +static void
>> +mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
>> +{
>> + struct ata_port *ap = qc->ap;
>> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
>> +
>> + /* LocalBus lock */
>> + while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
>> + ;
>
> Need to be able to bail on timeout.
A deadlock can't occur within the PATA driver because you won't have
two DMA requests happening at once, so there is no point in adding a
timeout. And even if you do have a timeout, you'd have to drop the I/O
request somehow, so it's not really a good idea. If anything else
needs to touch the DMA lock, it should do so in a sensible fashion...
Thanks,
Tim
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bestcomm-ata-fixes.patch --]
[-- Type: text/x-patch; name=bestcomm-ata-fixes.patch, Size: 4212 bytes --]
1) ata.h has dst_pa in the wrong place (needs to match what the BestComm
task microcode in bcom_ata_task.c expects); fix it.
2) The BestComm ATA task priority was changed to maximum in bestcomm_priv.h;
this fixes a deadlock issue I was experiencing when heavy DMA was
occuring on both the ATA and Ethernet BestComm tasks, e.g. when
downloading a large file over a LAN to disk.
3) The ATA BestComm driver uses bcom_ata_bd which is bigger than bcom_bd
and this causes problems because the various bcom_... functions do not
dereference the correct location. I've introduced bcom_get_bd which
uses bcom_task.bd_size and this fixes the problem.
Signed-off-by: Tim Yamin <plasm@roo.me.uk>
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h 2008-07-03 16:17:05.000000000 +0100
@@ -16,8 +16,8 @@
struct bcom_ata_bd {
u32 status;
- u32 dst_pa;
u32 src_pa;
+ u32 dst_pa;
};
extern struct bcom_task *
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-07-03 16:17:05.000000000 +0100
@@ -38,7 +38,7 @@ struct bcom_task {
unsigned int flags;
int irq;
- struct bcom_bd *bd;
+ void *bd;
phys_addr_t bd_pa;
void **cookie;
unsigned short index;
@@ -140,15 +140,29 @@ bcom_queue_full(struct bcom_task *tsk)
}
/**
+ * bcom_get_bd - Get a BD from the queue
+ * @tsk: The BestComm task structure
+ * index: Index of the BD to fetch
+ */
+static inline struct bcom_bd
+*bcom_get_bd(struct bcom_task *tsk, unsigned int index)
+{
+ return tsk->bd + index * tsk->bd_size;
+}
+
+/**
* bcom_buffer_done - Checks if a BestComm
* @tsk: The BestComm task structure
*/
static inline int
bcom_buffer_done(struct bcom_task *tsk)
{
+ struct bcom_bd *bd;
if (bcom_queue_empty(tsk))
return 0;
- return !(tsk->bd[tsk->outdex].status & BCOM_BD_READY);
+
+ bd = bcom_get_bd(tsk, tsk->outdex);
+ return !(bd->status & BCOM_BD_READY);
}
/**
@@ -160,16 +174,21 @@ bcom_buffer_done(struct bcom_task *tsk)
static inline struct bcom_bd *
bcom_prepare_next_buffer(struct bcom_task *tsk)
{
- tsk->bd[tsk->index].status = 0; /* cleanup last status */
- return &tsk->bd[tsk->index];
+ struct bcom_bd *bd;
+
+ bd = bcom_get_bd(tsk, tsk->index);
+ bd->status = 0; /* cleanup last status */
+ return bd;
}
static inline void
bcom_submit_next_buffer(struct bcom_task *tsk, void *cookie)
{
+ struct bcom_bd *bd = bcom_get_bd(tsk, tsk->index);
+
tsk->cookie[tsk->index] = cookie;
mb(); /* ensure the bd is really up-to-date */
- tsk->bd[tsk->index].status |= BCOM_BD_READY;
+ bd->status |= BCOM_BD_READY;
tsk->index = _bcom_next_index(tsk);
if (tsk->flags & BCOM_FLAGS_ENABLE_TASK)
bcom_enable(tsk);
@@ -179,10 +198,12 @@ static inline void *
bcom_retrieve_buffer(struct bcom_task *tsk, u32 *p_status, struct bcom_bd **p_bd)
{
void *cookie = tsk->cookie[tsk->outdex];
+ struct bcom_bd *bd = bcom_get_bd(tsk, tsk->outdex);
+
if (p_status)
- *p_status = tsk->bd[tsk->outdex].status;
+ *p_status = bd->status;
if (p_bd)
- *p_bd = &tsk->bd[tsk->outdex];
+ *p_bd = bd;
tsk->outdex = _bcom_next_outdex(tsk);
return cookie;
}
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-07-03 16:17:05.000000000 +0100
@@ -198,8 +198,8 @@ struct bcom_task_header {
#define BCOM_IPR_SCTMR_1 2
#define BCOM_IPR_FEC_RX 6
#define BCOM_IPR_FEC_TX 5
-#define BCOM_IPR_ATA_RX 4
-#define BCOM_IPR_ATA_TX 3
+#define BCOM_IPR_ATA_RX 7
+#define BCOM_IPR_ATA_TX 7
#define BCOM_IPR_SCPCI_RX 2
#define BCOM_IPR_SCPCI_TX 2
#define BCOM_IPR_PSC3_RX 2
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: pata_mpc52xx-add-dma.patch --]
[-- Type: text/x-patch; name=pata_mpc52xx-add-dma.patch, Size: 19524 bytes --]
This patch adds MDMA/UDMA support (using BestComm for DMA) on the MPC5200
platform. Patch requires the ATA BestComm fixes to function properly.
Based heavily on previous work by Freescale (Bernard Kuhn, John Rigby)
and Domen Puncer.
Using a SanDisk Extreme IV CF card I get read speeds of approximately
26.70 MB/sec.
There's also what I believe to be a hardware bug if you have high levels
of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
the address bus seems to sometimes get corrupted with ATA commands while
the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
I've asked Freescale about this but have not received a reply yet -- if
anybody from Freescale has any ideas please contact me; I can supply some
analyzer traces if needed. Therefore, for now, do not enable DMA if you
need reliable LocalPlus Bus unless you do a fixup in your driver as
follows:
Locking example:
while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
{
struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
if(bcom_buffer_done_2(tsk))
return 1;
}
return 0;
(Save the return value to `flags`)
Unlocking example:
if(flags == 0)
clear_bit(0, &pata_mpc52xx_ata_dma_lock);
Comments and testing would of course be very welcome.
Thanks,
Signed-off-by: Tim Yamin <plasm@roo.me.uk>
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-04-17 03:49:44.000000000 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-07-03 16:17:05.000000000 +0100
@@ -330,11 +330,16 @@ bcom_engine_init(void)
/* Init 'always' initiator */
out_8(&bcom_eng->regs->ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
+ /* If ATA DMA is enabled, always turn prefetch off (it breaks things) */
+#ifndef CONFIG_PATA_MPC52xx_DMA
/* Disable COMM Bus Prefetch on the original 5200; it's broken */
if ((mfspr(SPRN_SVR) & MPC5200_SVR_MASK) == MPC5200_SVR) {
+#endif
regval = in_be16(&bcom_eng->regs->PtdCntrl);
out_be16(&bcom_eng->regs->PtdCntrl, regval | 1);
+#ifndef CONFIG_PATA_MPC52xx_DMA
}
+#endif
/* Init lock */
spin_lock_init(&bcom_eng->lock);
diff -urp linux-2.6.26-rc6/drivers/ata/Kconfig linux-2.6.26-rc6-ata/drivers/ata/Kconfig
--- linux-2.6.26-rc6/drivers/ata/Kconfig 2008-07-03 13:06:35.000000000 +0100
+++ linux-2.6.26-rc6-ata/drivers/ata/Kconfig 2008-07-03 16:16:32.000000000 +0100
@@ -462,6 +462,15 @@ config PATA_MPC52xx
If unsure, say N.
+config PATA_MPC52xx_DMA
+ tristate "Freescale MPC52xx SoC internal IDE DMA"
+ depends on PATA_MPC52xx
+ help
+ This option enables support for DMA on the MPC52xx SoC PATA
+ controller.
+
+ If unsure, say N.
+
config PATA_MPIIX
tristate "Intel PATA MPIIX support"
depends on PCI
diff -urp linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c linux-2.6.26-rc6-ata/drivers/ata/pata_mpc52xx.c
--- linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c 2008-07-03 13:06:35.000000000 +0100
+++ linux-2.6.26-rc6-ata/drivers/ata/pata_mpc52xx.c 2008-07-03 16:16:32.000000000 +0100
@@ -6,6 +6,9 @@
* Copyright (C) 2006 Sylvain Munaut <tnt@246tNt.com>
* Copyright (C) 2003 Mipsys - Benjamin Herrenschmidt
*
+ * UDMA support based on patches by Freescale (Bernard Kuhn, John Rigby),
+ * Domen Puncer and Tim Yamin.
+ *
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed "as is" without any warranty of any
* kind, whether express or implied.
@@ -17,28 +20,47 @@
#include <linux/delay.h>
#include <linux/libata.h>
+#include <asm/cacheflush.h>
#include <asm/types.h>
#include <asm/prom.h>
#include <asm/of_platform.h>
#include <asm/mpc52xx.h>
+#include <sysdev/bestcomm/bestcomm.h>
+#include <sysdev/bestcomm/bestcomm_priv.h>
+#include <sysdev/bestcomm/ata.h>
#define DRV_NAME "mpc52xx_ata"
#define DRV_VERSION "0.1.2"
-
/* Private structures used by the driver */
struct mpc52xx_ata_timings {
u32 pio1;
u32 pio2;
+ u32 mdma1;
+ u32 mdma2;
+ u32 udma1;
+ u32 udma2;
+ u32 udma3;
+ u32 udma4;
+ u32 udma5;
+ int using_udma;
};
struct mpc52xx_ata_priv {
unsigned int ipb_period;
struct mpc52xx_ata __iomem * ata_regs;
+ phys_addr_t ata_regs_pa;
int ata_irq;
struct mpc52xx_ata_timings timings[2];
int csel;
+
+ /* DMA */
+ struct bcom_task *dmatsk;
+ const struct udmaspec *udmaspec;
+ const struct mdmaspec *mdmaspec;
+ int mpc52xx_ata_dma_last_write;
+ int waiting_for_dma;
};
@@ -53,6 +75,107 @@ static const int ataspec_ta[5] = { 35
#define CALC_CLKCYC(c,v) ((((v)+(c)-1)/(c)))
+/* ======================================================================== */
+
+/* ATAPI-4 MDMA specs (in clocks) */
+struct mdmaspec {
+ u32 t0M;
+ u32 td;
+ u32 th;
+ u32 tj;
+ u32 tkw;
+ u32 tm;
+ u32 tn;
+};
+
+static const struct mdmaspec mdmaspec66[3] = {
+ { .t0M = 32, .td = 15, .th = 2, .tj = 2, .tkw = 15, .tm = 4, .tn = 1 },
+ { .t0M = 10, .td = 6, .th = 1, .tj = 1, .tkw = 4, .tm = 2, .tn = 1 },
+ { .t0M = 8, .td = 5, .th = 1, .tj = 1, .tkw = 2, .tm = 2, .tn = 1 },
+};
+
+static const struct mdmaspec mdmaspec132[3] = {
+ { .t0M = 64, .td = 29, .th = 3, .tj = 3, .tkw = 29, .tm = 7, .tn = 2 },
+ { .t0M = 20, .td = 11, .th = 2, .tj = 1, .tkw = 7, .tm = 4, .tn = 1 },
+ { .t0M = 16, .td = 10, .th = 2, .tj = 1, .tkw = 4, .tm = 4, .tn = 1 },
+};
+
+/* ATAPI-4 UDMA specs (in clocks) */
+struct udmaspec {
+ u32 tcyc;
+ u32 t2cyc;
+ u32 tds;
+ u32 tdh;
+ u32 tdvs;
+ u32 tdvh;
+ u32 tfs;
+ u32 tli;
+ u32 tmli;
+ u32 taz;
+ u32 tzah;
+ u32 tenv;
+ u32 tsr;
+ u32 trfs;
+ u32 trp;
+ u32 tack;
+ u32 tss;
+};
+
+static const struct udmaspec udmaspec66[6] = {
+ { .tcyc = 8, .t2cyc = 16, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
+ .tfs = 16, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 3, .trfs = 5, .trp = 11, .tack = 2, .tss = 4,
+ },
+ { .tcyc = 5, .t2cyc = 11, .tds = 1, .tdh = 1, .tdvs = 4, .tdvh = 1,
+ .tfs = 14, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 5, .trp = 9, .tack = 2, .tss = 4,
+ },
+ { .tcyc = 4, .t2cyc = 8, .tds = 1, .tdh = 1, .tdvs = 3, .tdvh = 1,
+ .tfs = 12, .tli = 10, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4,
+ },
+ { .tcyc = 3, .t2cyc = 6, .tds = 1, .tdh = 1, .tdvs = 2, .tdvh = 1,
+ .tfs = 9, .tli = 7, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4,
+ },
+ { .tcyc = 2, .t2cyc = 4, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
+ .tfs = 8, .tli = 8, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 7, .tack = 2, .tss = 4,
+ },
+ { .tcyc = 2, .t2cyc = 2, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
+ .tfs = 6, .tli = 5, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 4, .trp = 6, .tack = 2, .tss = 4,
+ },
+};
+
+static const struct udmaspec udmaspec132[6] = {
+ { .tcyc = 15, .t2cyc = 31, .tds = 2, .tdh = 1, .tdvs = 10, .tdvh = 1,
+ .tfs = 30, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 7, .trfs = 10, .trp = 22, .tack = 3, .tss = 7,
+ },
+ { .tcyc = 10, .t2cyc = 21, .tds = 2, .tdh = 1, .tdvs = 7, .tdvh = 1,
+ .tfs = 27, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 4, .trfs = 10, .trp = 17, .tack = 3, .tss = 7,
+ },
+ { .tcyc = 6, .t2cyc = 12, .tds = 1, .tdh = 1, .tdvs = 5, .tdvh = 1,
+ .tfs = 23, .tli = 20, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 3, .trfs = 8, .trp = 14, .tack = 3, .tss = 7,
+ },
+ { .tcyc = 7, .t2cyc = 12, .tds = 1, .tdh = 1, .tdvs = 3, .tdvh = 1,
+ .tfs = 15, .tli = 13, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 3, .trfs = 8, .trp = 14, .tack = 3, .tss = 7,
+ },
+ { .tcyc = 2, .t2cyc = 5, .tds = 0, .tdh = 0, .tdvs = 1, .tdvh = 1,
+ .tfs = 16, .tli = 14, .tmli = 2, .taz = 1, .tzah = 2, .tenv = 2,
+ .tsr = 2, .trfs = 7, .trp = 13, .tack = 2, .tss = 6,
+ },
+ { .tcyc = 3, .t2cyc = 6, .tds = 1, .tdh = 1, .tdvs = 1, .tdvh = 1,
+ .tfs = 12, .tli = 10, .tmli = 3, .taz = 2, .tzah = 3, .tenv = 3,
+ .tsr = 3, .trfs = 7, .trp = 12, .tack = 3, .tss = 7,
+ },
+};
+
+/* ======================================================================== */
/* Bit definitions inside the registers */
#define MPC52xx_ATA_HOSTCONF_SMR 0x80000000UL /* State machine reset */
@@ -66,6 +189,7 @@ static const int ataspec_ta[5] = { 35
#define MPC52xx_ATA_HOSTSTAT_WERR 0x01000000UL /* Write Error */
#define MPC52xx_ATA_FIFOSTAT_EMPTY 0x01 /* FIFO Empty */
+#define MPC52xx_ATA_FIFOSTAT_ERROR 0x40 /* FIFO Error */
#define MPC52xx_ATA_DMAMODE_WRITE 0x01 /* Write DMA */
#define MPC52xx_ATA_DMAMODE_READ 0x02 /* Read DMA */
@@ -75,6 +199,8 @@ static const int ataspec_ta[5] = { 35
#define MPC52xx_ATA_DMAMODE_FR 0x20 /* FIFO Reset */
#define MPC52xx_ATA_DMAMODE_HUT 0x40 /* Host UDMA burst terminate */
+#define MAX_DMA_BUFFERS 128
+#define MAX_DMA_BUFFER_SIZE 0x20000u
/* Structure of the hardware registers */
struct mpc52xx_ata {
@@ -133,6 +259,9 @@ struct mpc52xx_ata {
u8 reserved21[2];
};
+/* BestComm locking */
+unsigned long pata_mpc52xx_ata_dma_lock;
+struct bcom_task *pata_mpc52xx_ata_dma_task;
/* ======================================================================== */
/* Aux fns */
@@ -165,6 +294,41 @@ mpc52xx_ata_compute_pio_timings(struct m
return 0;
}
+static int
+mpc52xx_ata_compute_mdma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
+{
+ struct mpc52xx_ata_timings *timing = &priv->timings[dev];
+ const struct mdmaspec *s = &priv->mdmaspec[speed];
+
+ if (speed < 0 || speed > 2)
+ return -EINVAL;
+
+ timing->mdma1 = (s->t0M << 24) | (s->td << 16) | (s->tkw << 8) | (s->tm);
+ timing->mdma2 = (s->th << 24) | (s->tj << 16) | (s->tn << 8);
+ timing->using_udma = 0;
+
+ return 0;
+}
+
+static int
+mpc52xx_ata_compute_udma_timings(struct mpc52xx_ata_priv *priv, int dev, int speed)
+{
+ struct mpc52xx_ata_timings *timing = &priv->timings[dev];
+ const struct udmaspec *s = &priv->udmaspec[speed];
+
+ if (speed < 0 || speed > 2)
+ return -EINVAL;
+
+ timing->udma1 = (s->t2cyc << 24) | (s->tcyc << 16) | (s->tds << 8) | (s->tdh);
+ timing->udma2 = (s->tdvs << 24) | (s->tdvh << 16) | (s->tfs << 8) | (s->tli);
+ timing->udma3 = (s->tmli << 24) | (s->taz << 16) | (s->tenv << 8) | (s->tsr);
+ timing->udma4 = (s->tss << 24) | (s->trfs << 16) | (s->trp << 8) | (s->tack);
+ timing->udma5 = (s->tzah << 24);
+ timing->using_udma = 1;
+
+ return 0;
+}
+
static void
mpc52xx_ata_apply_timings(struct mpc52xx_ata_priv *priv, int device)
{
@@ -173,14 +337,13 @@ mpc52xx_ata_apply_timings(struct mpc52xx
out_be32(®s->pio1, timing->pio1);
out_be32(®s->pio2, timing->pio2);
- out_be32(®s->mdma1, 0);
- out_be32(®s->mdma2, 0);
- out_be32(®s->udma1, 0);
- out_be32(®s->udma2, 0);
- out_be32(®s->udma3, 0);
- out_be32(®s->udma4, 0);
- out_be32(®s->udma5, 0);
-
+ out_be32(®s->mdma1, timing->mdma1);
+ out_be32(®s->mdma2, timing->mdma2);
+ out_be32(®s->udma1, timing->udma1);
+ out_be32(®s->udma2, timing->udma2);
+ out_be32(®s->udma3, timing->udma3);
+ out_be32(®s->udma4, timing->udma4);
+ out_be32(®s->udma5, timing->udma5);
priv->csel = device;
}
@@ -245,6 +408,29 @@ mpc52xx_ata_set_piomode(struct ata_port
mpc52xx_ata_apply_timings(priv, adev->devno);
}
static void
+mpc52xx_ata_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ int rv;
+
+ if (adev->dma_mode >= XFER_UDMA_0) {
+ int dma = adev->dma_mode - XFER_UDMA_0;
+ rv = mpc52xx_ata_compute_udma_timings(priv, adev->devno, dma);
+ } else {
+ int dma = adev->dma_mode - XFER_MW_DMA_0;
+ rv = mpc52xx_ata_compute_mdma_timings(priv, adev->devno, dma);
+ }
+
+ if (rv) {
+ dev_alert(ap->dev,
+ "Trying to select invalid DMA mode %d\n",
+ adev->dma_mode);
+ return;
+ }
+
+ mpc52xx_ata_apply_timings(priv, adev->devno);
+}
+static void
mpc52xx_ata_dev_select(struct ata_port *ap, unsigned int device)
{
struct mpc52xx_ata_priv *priv = ap->host->private_data;
@@ -255,16 +441,192 @@ mpc52xx_ata_dev_select(struct ata_port *
ata_sff_dev_select(ap,device);
}
+static int
+mpc52xx_ata_build_dmatable(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE), si;
+ struct scatterlist *sg;
+ int count = 0;
+
+ if (read)
+ bcom_ata_rx_prepare(priv->dmatsk);
+ else
+ bcom_ata_tx_prepare(priv->dmatsk);
+
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
+ dma_addr_t cur_addr = sg_dma_address(sg);
+ u32 cur_len = sg_dma_len(sg);
+
+ while (cur_len) {
+ unsigned int tc = min(cur_len, MAX_DMA_BUFFER_SIZE);
+ struct bcom_ata_bd *bd = (struct bcom_ata_bd *) bcom_prepare_next_buffer(priv->dmatsk);
+
+ if (read) {
+ bd->status = tc;
+ bd->src_pa = (__force u32) priv->ata_regs_pa +
+ offsetof(struct mpc52xx_ata, fifo_data);
+ bd->dst_pa = (__force u32) cur_addr;
+ } else {
+ bd->status = tc;
+ bd->src_pa = (__force u32) cur_addr;
+ bd->dst_pa = (__force u32) priv->ata_regs_pa +
+ offsetof(struct mpc52xx_ata, fifo_data);
+ }
+
+ bcom_submit_next_buffer(priv->dmatsk, NULL);
+
+ cur_addr += tc;
+ cur_len -= tc;
+ count++;
+
+ if (count > MAX_DMA_BUFFERS) {
+ dev_alert(ap->dev, "dma table"
+ "too small\n");
+ goto use_pio_instead;
+ }
+ }
+ }
+ return 1;
+
+ use_pio_instead:
+ bcom_ata_reset_bd(priv->dmatsk);
+ return 0;
+}
+
+static void
+mpc52xx_bmdma_setup(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+ struct mpc52xx_ata __iomem *regs = priv->ata_regs;
+
+ unsigned int read = !(qc->tf.flags & ATA_TFLAG_WRITE);
+ u8 dma_mode;
+
+ if (!mpc52xx_ata_build_dmatable(qc))
+ dev_alert(ap->dev, "%s: %i, return 1?\n",
+ __func__, __LINE__);
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ dev_alert(ap->dev, "%s: FIFO error detected: 0x%02x!\n",
+ __func__, in_8(&priv->ata_regs->fifo_status));
+
+ if (read) {
+ dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_READ |
+ MPC52xx_ATA_DMAMODE_FE;
+
+ /* Setup FIFO if direction changed */
+ if (priv->mpc52xx_ata_dma_last_write != 0) {
+ priv->mpc52xx_ata_dma_last_write = 0;
+
+ /* Configure FIFO with granularity to 7 */
+ out_8(®s->fifo_control, 7);
+ out_be16(®s->fifo_alarm, 128);
+
+ /* Set FIFO Reset bit (FR) */
+ out_8(®s->dma_mode, MPC52xx_ATA_DMAMODE_FR);
+ }
+ } else {
+ dma_mode = MPC52xx_ATA_DMAMODE_IE | MPC52xx_ATA_DMAMODE_WRITE;
+
+ /* Setup FIFO if direction changed */
+ if (priv->mpc52xx_ata_dma_last_write != 1) {
+ priv->mpc52xx_ata_dma_last_write = 1;
+
+ /* Configure FIFO with granularity to 4 */
+ out_8(®s->fifo_control, 4);
+ out_be16(®s->fifo_alarm, 128);
+ }
+ }
+
+ if (priv->timings[qc->dev->devno].using_udma)
+ dma_mode |= MPC52xx_ATA_DMAMODE_UDMA;
+
+ out_8(®s->dma_mode, dma_mode);
+ priv->waiting_for_dma = ATA_DMA_ACTIVE;
+
+ ata_wait_idle(ap);
+ ap->ops->sff_exec_command(ap, &qc->tf);
+}
+
+static void
+mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ /* LocalBus lock */
+ while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
+ ;
+
+ bcom_set_task_auto_start(priv->dmatsk->tasknum, priv->dmatsk->tasknum);
+ bcom_enable(priv->dmatsk);
+}
+
+static void
+mpc52xx_bmdma_stop(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ bcom_disable(priv->dmatsk);
+ bcom_ata_reset_bd(priv->dmatsk);
+
+ /* LocalBus unlock*/
+ clear_bit(0, &pata_mpc52xx_ata_dma_lock);
+
+ priv->waiting_for_dma = 0;
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR)
+ dev_alert(ap->dev, "%s: FIFO error detected: 0x%02x!\n",
+ __func__, in_8(&priv->ata_regs->fifo_status));
+}
+
+static u8
+mpc52xx_bmdma_status(struct ata_port *ap)
+{
+ struct mpc52xx_ata_priv *priv = ap->host->private_data;
+
+ /* Check FIFO is OK... */
+ if(in_8(&priv->ata_regs->fifo_status) & MPC52xx_ATA_FIFOSTAT_ERROR) {
+ dev_alert(ap->dev, "%s: FIFO error detected: 0x%02x!\n",
+ __func__, in_8(&priv->ata_regs->fifo_status));
+ return priv->waiting_for_dma | ATA_DMA_ERR;
+ }
+
+ return priv->waiting_for_dma;
+}
+
+static irqreturn_t
+mpc52xx_ata_task_irq(int irq, void *vpriv)
+{
+ struct mpc52xx_ata_priv *priv = vpriv;
+ priv->waiting_for_dma |= ATA_DMA_INTR;
+
+ return IRQ_HANDLED;
+}
+
static struct scsi_host_template mpc52xx_ata_sht = {
ATA_PIO_SHT(DRV_NAME),
};
static struct ata_port_operations mpc52xx_ata_port_ops = {
.inherits = &ata_sff_port_ops,
- .sff_dev_select = mpc52xx_ata_dev_select,
- .cable_detect = ata_cable_40wire,
+
.set_piomode = mpc52xx_ata_set_piomode,
- .post_internal_cmd = ATA_OP_NULL,
+ .set_dmamode = mpc52xx_ata_set_dmamode,
+ .sff_dev_select = mpc52xx_ata_dev_select,
+
+ .bmdma_setup = mpc52xx_bmdma_setup,
+ .bmdma_start = mpc52xx_bmdma_start,
+ .bmdma_stop = mpc52xx_bmdma_stop,
+ .bmdma_status = mpc52xx_bmdma_status,
+
+ .qc_prep = ata_noop_qc_prep,
};
static int __devinit
@@ -281,9 +643,11 @@ mpc52xx_ata_init_one(struct device *dev,
ap = host->ports[0];
ap->flags |= ATA_FLAG_SLAVE_POSS;
- ap->pio_mask = 0x1f; /* Up to PIO4 */
- ap->mwdma_mask = 0x00; /* No MWDMA */
- ap->udma_mask = 0x00; /* No UDMA */
+ ap->pio_mask = ATA_PIO4; /* Up to PIO4 */
+#ifdef CONFIG_PATA_MPC52xx_DMA
+ ap->mwdma_mask = ATA_MWDMA2; /* Up to MWDMA2 */
+ ap->udma_mask = ATA_UDMA2; /* Up to UDMA2 */
+#endif
ap->ops = &mpc52xx_ata_port_ops;
host->private_data = priv;
@@ -333,7 +697,7 @@ mpc52xx_ata_probe(struct of_device *op,
int ata_irq;
struct mpc52xx_ata __iomem *ata_regs;
struct mpc52xx_ata_priv *priv;
- int rv;
+ int rv, ret, task_irq;
/* Get ipb frequency */
ipb_freq = mpc52xx_find_ipb_freq(op->node);
@@ -389,8 +753,34 @@ mpc52xx_ata_probe(struct of_device *op,
priv->ipb_period = 1000000000 / (ipb_freq / 1000);
priv->ata_regs = ata_regs;
+ priv->ata_regs_pa = res_mem.start;
priv->ata_irq = ata_irq;
priv->csel = -1;
+ priv->mpc52xx_ata_dma_last_write = -1;
+
+ if (ipb_freq/1000000 == 66) {
+ priv->mdmaspec = mdmaspec66;
+ priv->udmaspec = udmaspec66;
+ } else {
+ priv->mdmaspec = mdmaspec132;
+ priv->udmaspec = udmaspec132;
+ }
+
+ pata_mpc52xx_ata_dma_lock = 0;
+ priv->dmatsk = bcom_ata_init(MAX_DMA_BUFFERS, MAX_DMA_BUFFER_SIZE);
+ pata_mpc52xx_ata_dma_task = priv->dmatsk;
+ if (!priv->dmatsk) {
+ dev_alert(&op->dev, "Failed to initialize BestComm task!\n");
+ rv = -ENOMEM;
+ goto err;
+ }
+
+ task_irq = bcom_get_task_irq(priv->dmatsk);
+ ret = request_irq(task_irq, &mpc52xx_ata_task_irq, IRQF_DISABLED,
+ "ATA task", priv);
+ if (ret)
+ dev_alert(&op->dev, "request_irq failed with: "
+ "%i\n", ret);
/* Init the hw */
rv = mpc52xx_ata_hw_init(priv);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-07-03 15:35 ` Tim Yamin
@ 2008-07-03 23:47 ` Benjamin Herrenschmidt
2008-07-04 3:20 ` Grant Likely
2008-07-04 3:26 ` Grant Likely
1 sibling, 1 reply; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2008-07-03 23:47 UTC (permalink / raw)
To: Tim Yamin; +Cc: linuxppc-dev
On Thu, 2008-07-03 at 16:35 +0100, Tim Yamin wrote:
> >> +static void
> >> +mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
> >> +{
> >> + struct ata_port *ap = qc->ap;
> >> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> >> +
> >> + /* LocalBus lock */
> >> + while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
> >> + ;
> >
> > Need to be able to bail on timeout.
>
> A deadlock can't occur within the PATA driver because you won't have
> two DMA requests happening at once, so there is no point in adding a
> timeout. And even if you do have a timeout, you'd have to drop the I/O
> request somehow, so it's not really a good idea. If anything else
> needs to touch the DMA lock, it should do so in a sensible fashion...
But why a hand-coded lock with bitops ? Why not a real spinlock then ?
The later is more efficient anyway.
Ben.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-07-03 23:47 ` Benjamin Herrenschmidt
@ 2008-07-04 3:20 ` Grant Likely
0 siblings, 0 replies; 9+ messages in thread
From: Grant Likely @ 2008-07-04 3:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Tim Yamin, linuxppc-dev
On Fri, Jul 04, 2008 at 09:47:03AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2008-07-03 at 16:35 +0100, Tim Yamin wrote:
> > >> +static void
> > >> +mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
> > >> +{
> > >> + struct ata_port *ap = qc->ap;
> > >> + struct mpc52xx_ata_priv *priv = ap->host->private_data;
> > >> +
> > >> + /* LocalBus lock */
> > >> + while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
> > >> + ;
> > >
> > > Need to be able to bail on timeout.
> >
> > A deadlock can't occur within the PATA driver because you won't have
> > two DMA requests happening at once, so there is no point in adding a
> > timeout. And even if you do have a timeout, you'd have to drop the I/O
> > request somehow, so it's not really a good idea. If anything else
> > needs to touch the DMA lock, it should do so in a sensible fashion...
>
> But why a hand-coded lock with bitops ? Why not a real spinlock then ?
> The later is more efficient anyway.
Umm, yeah.... (don't know why I didn't clue into that). This definitely
should be a spinlock.
g.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
2008-07-03 15:35 ` Tim Yamin
2008-07-03 23:47 ` Benjamin Herrenschmidt
@ 2008-07-04 3:26 ` Grant Likely
1 sibling, 0 replies; 9+ messages in thread
From: Grant Likely @ 2008-07-04 3:26 UTC (permalink / raw)
To: Tim Yamin; +Cc: linuxppc-dev
On Thu, Jul 03, 2008 at 04:35:27PM +0100, Tim Yamin wrote:
> There's also what I believe to be a hardware bug if you have high levels
> of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
> the address bus seems to sometimes get corrupted with ATA commands while
> the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
>
> I've asked Freescale about this but have not received a reply yet -- if
> anybody from Freescale has any ideas please contact me; I can supply some
> analyzer traces if needed. Therefore, for now, do not enable DMA if you
> need reliable LocalPlus Bus unless you do a fixup in your driver as
> follows:
>
> Locking example:
>
> while (test_and_set_bit(0, &pata_mpc52xx_ata_dma_lock) != 0)
> {
> struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
>
> if(bcom_buffer_done_2(tsk))
> return 1;
> }
>
> return 0;
>
> (Save the return value to `flags`)
>
> Unlocking example:
>
> if(flags == 0)
> clear_bit(0, &pata_mpc52xx_ata_dma_lock);
>
One more thing. For this description to be of any use, it needs to be
documented where somebody will actually see it. The commit message is
great for describing *why* a change is needed, but it's not seen
so much after the change is merged.
This description should probably be added to the header comment block of
the ATA driver, and it should be talked about in the Kconfig help text
too.
g.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-07-04 3:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-27 12:44 [PATCH]: [MPC5200] (v2) Add ATA DMA support Tim Yamin
2008-06-30 15:40 ` Daniel Schnell
2008-07-01 23:49 ` Grant Likely
2008-07-02 12:48 ` Tim Yamin
2008-07-02 17:30 ` Grant Likely
2008-07-03 15:35 ` Tim Yamin
2008-07-03 23:47 ` Benjamin Herrenschmidt
2008-07-04 3:20 ` Grant Likely
2008-07-04 3:26 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).