From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-mmc@vger.kernel.org,
Linus Walleij <linus.walleij@linaro.org>,
Chris Ball <chris@printf.net>,
Ulf Hansson <ulf.hansson@linaro.org>,
Russell King <linux@arm.linux.org.uk>,
linux-kernel@vger.kernel.org, agross@codeaurora.org,
linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2] mmc: mmci: Add qcom dml support to the driver.
Date: Fri, 25 Jul 2014 05:07:29 +0100 [thread overview]
Message-ID: <53D1D801.6030802@linaro.org> (raw)
In-Reply-To: <53D03E23.1000801@codeaurora.org>
On 23/07/14 23:58, Stephen Boyd wrote:
> On 07/18/14 13:53, Srinivas Kandagatla wrote:
>>
>> @@ -468,6 +473,11 @@ static void mmci_dma_setup(struct mmci_host *host)
>> if (max_seg_size < host->mmc->max_seg_size)
>> host->mmc->max_seg_size = max_seg_size;
>> }
>> +
>> + if (variant->qcom_dml && host->dma_rx_channel && host->dma_tx_channel) {
>> + if (dml_hw_init(host, host->mmc->parent->of_node))
>> + variant->qcom_dml = false;
>> + }
>
> Style nit: Braces aren't needed.
Will fix it in next spin.
>
>> +
>> +void dml_start_xfer(struct mmci_host *host, struct mmc_data *data)
>> +{
>> + u32 config;
>> + void __iomem *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_relaxed(base + DML_CONFIG);
>> + config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_X_SEL;
>> + config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_DISABLE;
>> + writel_relaxed(config, base + DML_CONFIG);
>> +
>> + /* Set the Producer BAM block size */
>> + writel_relaxed(data->blksz, base + DML_PRODUCER_BAM_BLOCK_SIZE);
>> +
>> + /* Set Producer BAM Transaction size */
>> + writel_relaxed(data->blocks * data->blksz,
>> + base + DML_PRODUCER_BAM_TRANS_SIZE);
>> + /* Set Producer Transaction End bit */
>> + config = readl_relaxed(base + DML_CONFIG);
>> + config |= PRODUCER_TRANS_END_EN;
>> + writel_relaxed(config, base + DML_CONFIG);
>> + /* Trigger producer */
>> + writel_relaxed(1, base + DML_PRODUCER_START);
>> + } else {
>> + /* Write operation: configure DML for consumer operation */
>> + /* Set consumer CRCI-x and disable producer CRCI*/
>> + config = readl_relaxed(base + DML_CONFIG);
>> + config = (config & ~CONSUMER_CRCI_MSK) | CONSUMER_CRCI_X_SEL;
>> + config = (config & ~PRODUCER_CRCI_MSK) | PRODUCER_CRCI_DISABLE;
>> + writel_relaxed(config, base + DML_CONFIG);
>> + /* Clear Producer Transaction End bit */
>> + config = readl_relaxed(base + DML_CONFIG);
>> + config &= ~PRODUCER_TRANS_END_EN;
>> + writel_relaxed(config, base + DML_CONFIG);
>> + /* Trigger consumer */
>> + writel_relaxed(1, base + DML_CONSUMER_START);
>> + }
>
> I think we need a memory barrier here. Does the DML need to be enabled
> before the mmci driver sets MCI_DPSM_DMAENABLE? My understanding is that
> relaxed accesses to different devices aren't ordered with respect to one
> another. So not having a memory barrier here could cause the
> consumer/producer to be enabled after MCI_DPSM_DMAENABLE is set in the
> mmci hardware. Downstream I see that we have an mb(), but I think we
> could just a wmb() because we're just ordering with the mmci hardware right?
yes, I think wmb should work. I will fix this.
>
> Is there any ordering required between bam and dml? I think enabling bam
> could happen after enabling the consumer unless we had a wmb() after we
> kick off bam and before we write a 1 to DML_CONSUMER_START.
>
Am not sure if there is any strict ordering requirement between bam and
dml. But I can see a wmb in bam_start_dma().
>> +}
>> +
>> +static int of_get_dml_pipe_index(struct device_node *np, const char *name)
>
> return type of u32?
>
This function is expected to return negative error codes. I think I
should change u32 to int in dml_hw_init().
>> +{
>> + int index;
>> + struct of_phandle_args dma_spec;
>> +
>> + index = of_property_match_string(np, "dma-names", name);
>> +
>> + if (index < 0)
>> + return -ENODEV;
>> +
>> + if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index,
>> + &dma_spec))
>> + return -ENODEV;
>> +
>> + if (dma_spec.args_count)
>> + return dma_spec.args[0];
>> +
>> + return -ENODEV;
>> +}
>
> Much cleaner!
>
>> +
>> +/* Initialize the dml hardware connected to SD Card controller */
>> +int dml_hw_init(struct mmci_host *host, struct device_node *np)
>> +{
>> + u32 config;
>> + void __iomem *base;
>> + u32 consumer_id, producer_id;
>> +
>> + consumer_id = of_get_dml_pipe_index(np, "tx");
>> + producer_id = of_get_dml_pipe_index(np, "rx");
>> +
>> + if (producer_id < 0 || consumer_id < 0)
>> + return -ENODEV;
>> +
>> + base = host->base + DML_OFFSET;
>> +
>> + /* Reset the DML block */
>> + writel_relaxed(1, 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_relaxed(config, base + DML_CONFIG);
>> +
>> + /*
>> + * Initialize the logical BAM pipe size for producer
>> + * and consumer.
>> + */
>> + writel_relaxed(PRODUCER_PIPE_LOGICAL_SIZE,
>> + base + DML_PRODUCER_PIPE_LOGICAL_SIZE);
>> + writel_relaxed(CONSUMER_PIPE_LOGICAL_SIZE,
>> + base + DML_CONSUMER_PIPE_LOGICAL_SIZE);
>> +
>> + /* Initialize Producer/consumer pipe id */
>> + writel_relaxed(producer_id | (consumer_id << CONSUMER_PIPE_ID_SHFT),
>> + base + DML_PIPE_ID);
>> +
>
> We have an mb() here too. Not sure why though. There's lots of comments
> downstream but not where it actually would help!
I will add mb here and try to add some comments here.
>
>> + 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..412e9a3
>> --- /dev/null
>> +++ b/drivers/mmc/host/mmci_qcom_dml.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + *
>> + * Copyright (c) 2014, Code Aurora Forum. All rights reserved.
>
> Hmm I see:
>
> Copyright (c) 2011, The Linux Foundation. All rights reserved.
>
> in these files. Can you use that?
https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/blob/refs/heads/inforce/ifc6410/v1.5:/drivers/mmc/host/msm_sdcc_dml.c
in 3.4 kernel has:
Copyright (c) 2011, Code Aurora Forum. All rights reserved.
Do you still want to add Linux Foundation copyright in it.
thanks,
srini
>
next prev parent reply other threads:[~2014-07-25 4:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-11 11:48 [RFC PATCH] mmc: mmci: Add qcom dml support to the driver Srinivas Kandagatla
2014-07-11 13:49 ` Linus Walleij
2014-07-11 19:36 ` Srinivas Kandagatla
2014-07-17 19:36 ` [PATCH v1] " Srinivas Kandagatla
2014-07-17 19:41 ` Srinivas Kandagatla
2014-07-17 23:06 ` Stephen Boyd
2014-07-18 6:42 ` Srinivas Kandagatla
2014-07-18 20:53 ` [PATCH v2] " Srinivas Kandagatla
2014-07-23 14:52 ` Linus Walleij
2014-07-23 22:58 ` Stephen Boyd
2014-07-25 4:07 ` Srinivas Kandagatla [this message]
2014-07-28 6:22 ` [PATCH v3] " Srinivas Kandagatla
2014-07-29 2:50 ` [PATCH v4] " Srinivas Kandagatla
2014-07-29 17:55 ` Stephen Boyd
2014-07-30 4:12 ` Srinivas Kandagatla
2014-08-12 8:40 ` Ulf Hansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53D1D801.6030802@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=agross@codeaurora.org \
--cc=chris@printf.net \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=sboyd@codeaurora.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).