Re: [PATCH v1] mmc: mmci: Add qcom dml support to the driver.

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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__ */
>
>