Re: [RFC PATCH v7 01/10] mmc: dw_mmc: Add external dma interface support
From: Shawn Lin
Date: Tue Sep 15 2015 - 05:05:14 EST
On 2015/9/15 16:08, Jaehoon Chung wrote:
Hi, Shawn.
[...]
-config MMC_DW_IDMAC
- bool "Internal DMAC interface"
- depends on MMC_DW
- help
- This selects support for the internal DMAC block within the Synopsys
- Designware Mobile Storage IP block. This disables the external DMA
- interface.
+ PIO, internal DMA mode and external DMA mode.
In future, i will add the quirk for broken IDMAC.
This patch is absolutely depended on HCON register, but some IP can be broken it.
how about?
That's fine to add broken IDMAC if some IP can support reading tranfer
mode from HCON.(I check IP 270a and 240a, they supports)
config MMC_DW_PLTFM
tristate "Synopsys Designware MCI Support as platform device"
[...]
+static void dw_mci_edmac_stop_dma(struct dw_mci *host)
+{
+ dmaengine_terminate_all(host->dms->ch);
Does it need not to check "return value"?
Doesn't need. dmaengine_terminate_all return -ENOSYS if sub-dma drivers
doesn't implement terminate_all hook. Then nearly all the sub-dma
drivers always return 0(success) . That's why none of mmc host drivers
to check the return value here.
+}
+
+static int dw_mci_edmac_start_dma(struct dw_mci *host,
+ unsigned int sg_len)
+{
+ struct dma_slave_config cfg;
+ struct dma_async_tx_descriptor *desc = NULL;
+ struct scatterlist *sgl = host->data->sg;
+ const u32 mszs[] = {1, 4, 8, 16, 32, 64, 128, 256};
+ u32 sg_elems = host->data->sg_len;
+ u32 fifoth_val;
+ u32 fifo_offset = host->fifo_reg - host->regs;
+ int ret = 0;
+
+ /* Set external dma config: burst size, burst width */
+ cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
+ cfg.src_addr = cfg.dst_addr;
+ cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+
+ /* Match burst msize with external dma config */
+ fifoth_val = mci_readl(host, FIFOTH);
+ cfg.dst_maxburst = mszs[(fifoth_val >> 28) & 0x7];
+ cfg.src_maxburst = cfg.dst_maxburst;
The above configuration needs to set it at every time?
I think so. We call dw_mci_adjust_fifoth to make
configuraton more reasonable every time. So this setting correlates to
the fifoth calculated by dw_mci_adjust_fifoth.
+
+ if (host->data->flags & MMC_DATA_WRITE)
+ cfg.direction = DMA_MEM_TO_DEV;
+ else /* MMC_DATA_READ */
Can be removed the comment.
okay.
+ cfg.direction = DMA_DEV_TO_MEM;
+
+ ret = dmaengine_slave_config(host->dms->ch, &cfg);
[...]
+ mmc->max_segs = 64;
+ mmc->max_blk_size = 65536;
+ mmc->max_blk_count = 65535;
+ mmc->max_req_size =
+ mmc->max_blk_size * mmc->max_blk_count;
+ mmc->max_seg_size = mmc->max_req_size;
WARNING: suspect code indent for conditional statements (8, 24)
#349: FILE: drivers/mmc/host/dw_mmc.c:2599:
Sorry, but I can't find it?
./scripts/checkpatch.pl -f drivers/mmc/host/dw_mmc.c
total: 0 errors, 0 warnings, 3310 lines checked
drivers/mmc/host/dw_mmc.c has no obvious style problems and is ready for
submission.
+ } else if (host->use_dma == TRANS_MODE_EDMAC) {
+ mmc->max_segs = 64;
} else {
[...]
+ */
trans_mode can't take HCON value, but trans_mode reassigned to "TRANS_MODE_IDMAC" or "TRANS_MODE_EDMAC"..
It's reassigned to host->use_dma...why can't use the host->use_dma?
Your code..
1. trans_mode <- HCON value
2. Check trans_mode which interface use.
then trans_mode <- TRANS_MODE_IDMAC/EDMAC/PIO
3. host->use_dma <- trans_mode
isn't?
It can be replaced to "host->use_dma" instead of "trans_mode".
yep, I intented to introduce a variable to make others clear the
mismatch meaning of databook. If you feel ok, I will remove trans_mode
in v8. :)
+ trans_mode = SDMMC_GET_TRANS_MODE(mci_readl(host, HCON));
+ if (trans_mode == DMA_INTERFACE_IDMA) {
[...]
if (!host->dma_ops)
goto no_dma;
This checking seems unnecessary, after applied your code.
Ahh... that's right, thanks for pointing it out.
Best Regards,
Jaehoon Chung
@@ -2562,12 +2733,12 @@ static void dw_mci_init_dma(struct dw_mci *host)
goto no_dma;
}
- host->use_dma = 1;
+ host->use_dma = trans_mode;
return;
no_dma:
dev_info(host->dev, "Using PIO mode.\n");
- host->use_dma = 0;
+ host->use_dma = trans_mode;
}
static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
@@ -2650,10 +2821,9 @@ static bool dw_mci_reset(struct dw_mci *host)
}
}
-#if IS_ENABLED(CONFIG_MMC_DW_IDMAC)
- /* It is also recommended that we reset and reprogram idmac */
- dw_mci_idmac_reset(host);
-#endif
+ if (host->use_dma == TRANS_MODE_IDMAC)
+ /* It is also recommended that we reset and reprogram idmac */
+ dw_mci_idmac_reset(host);
ret = true;
@@ -3067,6 +3237,9 @@ EXPORT_SYMBOL(dw_mci_remove);
*/
int dw_mci_suspend(struct dw_mci *host)
{
+ if (host->use_dma && host->dma_ops->exit)
+ host->dma_ops->exit(host);
+
return 0;
}
EXPORT_SYMBOL(dw_mci_suspend);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 8ce4674..811d467 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -148,6 +148,12 @@
#define SDMMC_SET_FIFOTH(m, r, t) (((m) & 0x7) << 28 | \
((r) & 0xFFF) << 16 | \
((t) & 0xFFF))
+/* HCON register defines */
+#define DMA_INTERFACE_IDMA (0x0)
+#define DMA_INTERFACE_DWDMA (0x1)
+#define DMA_INTERFACE_GDMA (0x2)
+#define DMA_INTERFACE_NODMA (0x3)
+#define SDMMC_GET_TRANS_MODE(x) (((x)>>16) & 0x3)
/* Internal DMAC interrupt defines */
#define SDMMC_IDMAC_INT_AI BIT(9)
#define SDMMC_IDMAC_INT_NI BIT(8)
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 134c574..f67b2ec 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -16,6 +16,7 @@
#include <linux/scatterlist.h>
#include <linux/mmc/core.h>
+#include <linux/dmaengine.h>
#define MAX_MCI_SLOTS 2
@@ -40,6 +41,17 @@ enum {
struct mmc_data;
+enum {
+ TRANS_MODE_PIO = 0,
+ TRANS_MODE_IDMAC,
+ TRANS_MODE_EDMAC
+};
+
+struct dw_mci_dma_slave {
+ struct dma_chan *ch;
+ enum dma_transfer_direction direction;
+};
+
/**
* struct dw_mci - MMC controller state shared between all slots
* @lock: Spinlock protecting the queue and associated data.
@@ -154,7 +166,14 @@ struct dw_mci {
dma_addr_t sg_dma;
void *sg_cpu;
const struct dw_mci_dma_ops *dma_ops;
+ /* For idmac */
unsigned int ring_size;
+
+ /* For edmac */
+ struct dw_mci_dma_slave *dms;
+ /* Registers's physical base address */
+ void *phy_regs;
+
u32 cmd_status;
u32 data_status;
u32 stop_cmdr;
@@ -208,8 +227,8 @@ struct dw_mci {
struct dw_mci_dma_ops {
/* DMA Ops */
int (*init)(struct dw_mci *host);
- void (*start)(struct dw_mci *host, unsigned int sg_len);
- void (*complete)(struct dw_mci *host);
+ int (*start)(struct dw_mci *host, unsigned int sg_len);
+ void (*complete)(void *host);
void (*stop)(struct dw_mci *host);
void (*cleanup)(struct dw_mci *host);
void (*exit)(struct dw_mci *host);
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/