Re: [PATCH v1] mmc: mmci: Add qcom dml support to the driver.
From: Srinivas Kandagatla
Date: Fri Jul 18 2014 - 02:43:05 EST
Thanks Stephen for the comments.
On 18/07/14 00:06, Stephen Boyd wrote:
On 07/17/14 12:36, Srinivas Kandagatla wrote:
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b66b351..a83b7b5 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -74,6 +75,7 @@ static unsigned int fmax = 515633;
* @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
* @explicit_mclk_control: enable explicit mclk control in driver.
* @qcom_fifo: enables qcom specific fifo pio read logic.
+ * @qcom_dml: enables qcom specific dml glue for dma transfers.
Maybe s/dml glue/dma glue/ in the comment so we don't refer to the
variable with dml in the name by dml.
Will fix it in v2.
*/
struct variant_data {
unsigned int clkreg;
@@ -569,6 +579,7 @@ static int __mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
struct dma_async_tx_descriptor *desc;
enum dma_data_direction buffer_dirn;
int nr_sg;
+ u32 flags = DMA_CTRL_ACK;
unsigned long to match function signature please.
Ok.
if (data->flags & MMC_DATA_READ) {
conf.direction = DMA_DEV_TO_MEM;
@@ -593,9 +604,12 @@ static int __mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
if (nr_sg == 0)
return -EINVAL;
+ if (host->variant->qcom_dml)
+ flags |= DMA_PREP_INTERRUPT;
+
dmaengine_slave_config(chan, &conf);
desc = dmaengine_prep_slave_sg(chan, data->sg, nr_sg,
- conf.direction, DMA_CTRL_ACK);
+ conf.direction, flags);
if (!desc)
goto unmap_exit;
diff --git a/drivers/mmc/host/mmci_qcom_dml.c b/drivers/mmc/host/mmci_qcom_dml.c
new file mode 100644
index 0000000..e2ecd07
--- /dev/null
+++ b/drivers/mmc/host/mmci_qcom_dml.c
@@ -0,0 +1,171 @@
No license text? I'd think most of this code is from qualcomm's kernel
and those files all have the gpl stuff.
Thanks for spotting, I will copy paste the text from 3.4 kernel.
+ void __iomem *dml_base;
+
+ dml_base = host->base + DML_OFFSET;
+
+ if (data->flags & MMC_DATA_READ) {
+ /* Read operation: configure DML for producer operation */
+ /* Set producer CRCI-x and disable consumer CRCI */
+ config = readl(dml_base + DML_CONFIG);
+ config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_X_SEL;
+ config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_DISABLE;
+ writel(config, (dml_base + DML_CONFIG));
+
+ /* Set the Producer BAM block size */
+ writel(data->blksz, (dml_base + DML_PRODUCER_BAM_BLOCK_SIZE));
+
+ /* Set Producer BAM Transaction size */
+ writel(data->blocks * data->blksz,
+ (dml_base + DML_PRODUCER_BAM_TRANS_SIZE));
+ /* Set Producer Transaction End bit */
+ writel((readl_relaxed(dml_base + DML_CONFIG) |
+ PRODUCER_TRANS_END_EN),
+ (dml_base + DML_CONFIG));
Please use local variables instead of writel(readl()). It makes things
much more readable.
OK, will be fixed in v2.
+ /* Trigger producer */
+ writel(1, (dml_base + DML_PRODUCER_START));
+ } else {
+ /* Write operation: configure DML for consumer operation */
+ /* Set consumer CRCI-x and disable producer CRCI*/
+ config = readl(dml_base + DML_CONFIG);
+ config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_X_SEL;
+ config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_DISABLE;
+ writel(config, (dml_base + DML_CONFIG));
+ /* Clear Producer Transaction End bit */
+ writel((readl_relaxed(dml_base + DML_CONFIG)
+ & ~PRODUCER_TRANS_END_EN),
+ (dml_base + DML_CONFIG));
+ /* Trigger consumer */
+ writel(1, (dml_base + DML_CONSUMER_START));
It would be better to use the relaxed accessors when possible.
Ok, I will use it.
+ }
+}
+
+static int of_get_dml_pipe_index(struct device_node *np, const char *name)
+{
+ int count, i;
+ const char *s;
+ struct of_phandle_args dma_spec;
+
+ if (!np || !name)
Looks unnecessary. name is definitely not NULL because this function is
static and the only caller is right down there. np is most likely not
NULL either, but if it is we should blow up with an oops instead of
silently failing.
I agree its an over do. Will fix it.
+ return -ENODEV;
+
+ count = of_property_count_strings(np, "dma-names");
+ if (count < 0)
+ return -ENODEV;
+
+ for (i = 0; i < count; i++) {
+
+ if (of_property_read_string_index(np, "dma-names", i, &s))
+ continue;
+
+ if (strcmp(name, s))
+ continue;
+
+ if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", i,
+ &dma_spec))
+ continue;
+
+ if (dma_spec.args_count)
+ return dma_spec.args[0];
+ }
Could this be rewritten to use of_property_match_string()?
index = of_property_match_string(np, "dma-names", s);
if (index < 0)
return index;
if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index,
&dma_spec))
return -ENODEV;
Yes this looks simple and it work.
+
+ return -ENODEV;
+}
+
+/* Initialize the dml hardware connected to SD Card controller */
+int dml_hw_init(struct mmci_host *host, struct device_node *np)
+{
+ u32 config = 0;
unnecessary initialization.
+ void __iomem *dml_base;
+ u32 consumer_id = 0, producer_id = 0;
unnecessary initialization.
Agree, I will fix it.
+
+ consumer_id = of_get_dml_pipe_index(np, "tx");
+ producer_id = of_get_dml_pipe_index(np, "rx");
+
+ if (IS_ERR_VALUE(producer_id) || IS_ERR_VALUE(consumer_id))
Is it actually possible for these ids to be very large values? Why not
just <= 0?
Ok.
+ return -ENODEV;
+
+ dml_base = host->base + DML_OFFSET;
+
+ /* Reset the DML block */
+ writel(1, (dml_base + DML_SW_RESET));
+
+ /* Disable the producer and consumer CRCI */
+ config = (PRODUCER_CRCI_DISABLE | CONSUMER_CRCI_DISABLE);
+ /*
+ * Disable the bypass mode. Bypass mode will only be used
+ * if data transfer is to happen in PIO mode and don't
+ * want the BAM interface to connect with SDCC-DML.
+ */
+ config &= ~BYPASS;
+ /*
+ * Disable direct mode as we don't DML to MASTER the AHB bus.
+ * BAM connected with DML should MASTER the AHB bus.
+ */
+ config &= ~DIRECT_MODE;
+ /*
+ * Disable infinite mode transfer as we won't be doing any
+ * infinite size data transfers. All data transfer will be
+ * of finite data size.
+ */
+ config &= ~INFINITE_CONS_TRANS;
+ writel(config, (dml_base + DML_CONFIG));
unnecessary parens (throughout this file actually).
Yes.. Its mostly from 3.4 kernels. I will revisit these ones.
+
+ /*
+ * Initialize the logical BAM pipe size for producer
+ * and consumer.
+ */
+ writel(PRODUCER_PIPE_LOGICAL_SIZE,
+ (dml_base + DML_PRODUCER_PIPE_LOGICAL_SIZE));
+ writel(CONSUMER_PIPE_LOGICAL_SIZE,
+ (dml_base + DML_CONSUMER_PIPE_LOGICAL_SIZE));
+
+ /* Initialize Producer/consumer pipe id */
+ writel(producer_id | (consumer_id << CONSUMER_PIPE_ID_SHFT),
+ (dml_base + DML_PIPE_ID));
+
+ return 0;
+}
diff --git a/drivers/mmc/host/mmci_qcom_dml.h b/drivers/mmc/host/mmci_qcom_dml.h
new file mode 100644
index 0000000..d2c5aa45
--- /dev/null
+++ b/drivers/mmc/host/mmci_qcom_dml.h
@@ -0,0 +1,17 @@
ditto for license text.
Will be done in v2.
+#ifndef __MMC_QCOM_DML_H__
+#define __MMC_QCOM_DML_H__
+
+#ifdef CONFIG_MMC_QCOM_DML
+int dml_hw_init(struct mmci_host *host, struct device_node *np);
+void dml_start_xfer(struct mmci_host *host, struct mmc_data *data);
+#else
+static inline int dml_hw_init(struct mmci_host *host, struct device_node *np)
+{
+ return -ENOSYS;
+}
+static inline void dml_start_xfer(struct mmci_host *host, struct mmc_data *data)
+{
+}
+#endif /* CONFIG_MMC_QCOM_DML */
+
+#endif /* __MMC_QCOM_DML_H__ */
--
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/