On 25/07/16 13:24, Ritesh Harjani wrote:
> Sorry about this long delay, was pulled into some release.
> Will be more responsive from now.
Well, I was away, so now it is my delay, sorry.
> We have gone through your comments. We had some queries and wanted to
> discuss more about your initial comments before going onto individual
> comments in the code. It would be great if you could help us understand more
> on your initial comments by answering to our queries.
>
>
> On 7/5/2016 4:45 PM, Adrian Hunter wrote:
>> On 27/06/16 16:22, Ritesh Harjani wrote:
>>> From: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
>>>
>>> Adds command-queue support to SDHCi compliant drivers.
>>
>> CQHCI is not recognized in the SDHCI specification,
> Yes. Not sure about future plan though.
SD Association and JEDEC are different bodies and I have never seen a SD
Association Spec. that mentions eMMC, so I would not expect the SDHCI spec.
ever to recognize CQHCI.
>
>> and SDHCI should
>> facilitate any CQE driver implementation not just CQHCI. That means there
>> are 2 options:
>
> Are we aware of any other vendor specific implementation of CQHCI? Or do you
> foresee this coming through?
No
> Actually CQ host controller interface(CQHCI) is what is proposed in emmc
> JEDEC5.1 which is mentioned as extension to HCI in block diagram. (Fig B.110
> in emmc-5.1 JEDEC).
>
>
>>
>> 1. Provide minimal support to allow individual host drivers to deal with
>> CQHCI directly.
>>
>> 2. Create explicit support for a CQE companion driver and use that to
>> provide common support for CQHCI.
>>
>> I started looking at option 2 and concluded that it was taking SDHCI in the
>> wrong direction because it made drivers more dependent on SDHCI and gave
>> them less flexibility, and it seemed inconsistent with the aim of making
>> SDHCI more like a library.
>>
>> Consequently, I suggest the following:
>>
>> 1. Individual drivers are responsible for initializing and setting up CQHCI
>> and its callbacks, and shutting it down.
>
> CQHCI callbacks is what the concern is. Currently the callbacks are placed
> in SDHCI, because these callbacks are doing nothing other than setting up
> SDHCI registers itself for CQHCI. There is nothing platform specific which
> is happening in these.
> So why do you think that these should be implemented in individual platform
> specific drivers?
> Will that not result into same code duplication in all SDHCI compliant
> platform drivers?
Common functions that do not need CQHCI definitions can still be in sdhci.c.
Some amount of code duplication is the cost of allowing drivers to be able
to do whatever the need to.
Linking CQHCI and SDHCI goes in the opposite direction of making SDHCI more
like a library. It means more callbacks and more potential issues if
drivers need to do anything differently.
>
>
>>
>> 2. SDHCI provides a new callback so that individual drivers can process
>> interrupts and pass them to CQHCI.
> This again will depend upon the initial ask on - do you foresee SDHCI
> facilitating other CQE implementations? Or if you are already aware of
> something else other than CQHCI?
No I don't know if there will ever be another implementation, however it is
not impossible. Obviously it would be much messier for SDHCI to support
different CQE implementations than for individual drivers to support just
the one they use.
>
>
>>
>> 3. SDHCI provides and exports any common helper functions that do not depend
>> directly on CQHCI. For example the functions to set interrupts, timeout,
>> blocks size and dump registers.
>>
>>>
>>> Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
>>> Signed-off-by: Konstantin Dorfman <kdorfman@xxxxxxxxxxxxxx>
>>> Signed-off-by: Venkat Gopalakrishnan <venkatg@xxxxxxxxxxxxxx>
>>> [subhashj@xxxxxxxxxxxxxx: fixed trivial merge conflicts and
>>> compilation error]
>>> Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
>>> [riteshh@xxxxxxxxxxxxxx: fixed merge conflicts]
>>> Signed-off-by: Ritesh Harjani <riteshh@xxxxxxxxxxxxxx>
>>> ---
>>> drivers/mmc/host/sdhci.c | 146
>>> +++++++++++++++++++++++++++++++++++++++++++++--
>>> drivers/mmc/host/sdhci.h | 6 ++
>>> 2 files changed, 148 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 9f5cdaa..0ed9c45 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -32,6 +32,7 @@
>>> #include <linux/mmc/slot-gpio.h>
>>>
>>> #include "sdhci.h"
>>> +#include "cmdq_hci.h"
>>
>> As discussed above, SDHCI should not have any references to CQHCI
>>
>>>
>>> #define DRIVER_NAME "sdhci"
>>>
>>> @@ -2474,6 +2475,20 @@ static void sdhci_data_irq(struct sdhci_host
>>> *host, u32 intmask)
>>> }
>>> }
>>>
>>> +#ifdef CONFIG_MMC_CQ_HCI
>>
>> This won't work if CQHCI is a module. If you fix the dependencies then you
>> can use:
>>
>> #if IS_ENABLED(CONFIG_MMC_CQ_HCI)
>>
>> Or if you select CONFIG_MMC_CQ_HCI in Kconfig then you don't need ifdef at
>> all.
>>
>>> +static irqreturn_t sdhci_cmdq_irq(struct mmc_host *mmc, u32 intmask)
>>> +{
>>> + return cmdq_irq(mmc, intmask);
>>
>> This will give a linker error if the driver is built-in but CQHCI is a
>> module. Probably drivers that use CQHCI should just select it in Kconfig.
>>
>> Another issue is that CQHCI won't be able to interpret SDHCI interrupt bits.
>> I suggest you define a generic set of bit flags not dependent on SDHCI or
>> CQHCI and then we can create a function to convert the SDHCI interrupt
>> status.
>>
>>> +}
>>> +
>>> +#else
>>> +static irqreturn_t sdhci_cmdq_irq(struct mmc_host *mmc, u32 intmask)
>>> +{
>>> + pr_err("%s: rxd cmdq-irq when disabled !!!!\n", mmc_hostname(mmc));
>>> + return IRQ_NONE;
>>> +}
>>> +#endif
>>> +
>>> static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>> {
>>> irqreturn_t result = IRQ_NONE;
>>> @@ -2495,6 +2510,15 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>> }
>>>
>>> do {
>>> + if (host->mmc->card && mmc_card_cmdq(host->mmc->card) &&
>>> + !mmc_host_halt(host->mmc)) {
>>> + pr_debug("*** %s: cmdq intr: 0x%08x\n",
>>> + mmc_hostname(host->mmc),
>>> + intmask);
>>> + result = sdhci_cmdq_irq(host->mmc, intmask);
>>> + goto out;
>>> + }
>>> +
>>
>> We need to create a new SDHCI host op to enable individual drivers to
>> handle the IRQ if they choose. Then it is up to individual drivers to call
>> into CQHCI.
>>
>>> /* Clear selected interrupts. */
>>> mask = intmask & (SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |
>>> SDHCI_INT_BUS_POWER);
>>> @@ -2823,6 +2847,106 @@ static int sdhci_set_dma_mask(struct sdhci_host
>>> *host)
>>> return ret;
>>> }
>>>
>>> +#ifdef CONFIG_MMC_CQ_HCI
>>> +static void sdhci_cmdq_clear_set_irqs(struct mmc_host *mmc, bool clear)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + u32 ier = 0;
>>> +
>>> + ier &= ~SDHCI_INT_ALL_MASK;
>>> +
>>> + if (clear) {
>>> + ier = SDHCI_INT_CMDQ_EN | SDHCI_INT_ERROR_MASK;
>>
>> SDHCI_INT_ERROR_MASK is not ideal here. I would expect to set only the bits
>> we know and want.
>>
>>> + sdhci_writel(host, ier, SDHCI_INT_ENABLE);
>>> + sdhci_writel(host, ier, SDHCI_SIGNAL_ENABLE);
>>> + } else {
>>> + ier = SDHCI_INT_BUS_POWER | SDHCI_INT_DATA_END_BIT |
>>> + SDHCI_INT_DATA_CRC | SDHCI_INT_DATA_TIMEOUT |
>>> + SDHCI_INT_INDEX | SDHCI_INT_END_BIT |
>>> + SDHCI_INT_CRC | SDHCI_INT_TIMEOUT |
>>> + SDHCI_INT_DATA_END | SDHCI_INT_RESPONSE |
>>> + SDHCI_INT_ACMD12ERR;
>>
>> We ought to have the default interrupts defined, so that the same ones are
>> set here.
>>
>>> + sdhci_writel(host, ier, SDHCI_INT_ENABLE);
>>> + sdhci_writel(host, ier, SDHCI_SIGNAL_ENABLE);
>>> + }
>>> +}
>>> +
>>> +static void sdhci_cmdq_set_data_timeout(struct mmc_host *mmc, u32 val)
>>
>> Do we really need different callbacks for ->clear_set_irqs(),
>> ->set_data_timeout(), ->set_block_size() and ->clear_set_dumpregs()? It
>> looks like we could consolidate them all into just 2 i.e. to start and stop
>> CQ mode.
>>
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> +
>>> + sdhci_writeb(host, val, SDHCI_TIMEOUT_CONTROL);
>>> +}
>>
>> We can't expect CQHCI to provide the SDHCI register value. Ideally we don't
>> want to set the highest value but instead calculate the value based on the
>> maximum sized transfer.
>>
>>> +
>>> +static void sdhci_cmdq_dump_vendor_regs(struct mmc_host *mmc)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> +
>>> + sdhci_dumpregs(host);
>>> +}
>>
>> Instead you should export sdhci_dumpregs() and let the individual drivers
>> connect it to CQHCI.
>>
>>> +
>>> +static int sdhci_cmdq_init(struct sdhci_host *host, struct mmc_host *mmc,
>>> + bool dma64)
>>> +{
>>> + return cmdq_init(host->cq_host, mmc, dma64);
>>> +}
>>
>> Individual drivers should initialize their CQE driver implementation. It is
>> not necessarily CQHCI.
>>
>>> +
>>> +static void sdhci_cmdq_set_block_size(struct mmc_host *mmc)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> +
>>> + sdhci_set_blk_size_reg(host, 512, 0);
>>> +}
>>> +
>>> +static void sdhci_cmdq_clear_set_dumpregs(struct mmc_host *mmc, bool set)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> +
>>> + if (host->ops->clear_set_dumpregs)
>>> + host->ops->clear_set_dumpregs(host, set);
> This is vendor specific callback for platform drivers to enable/disable (say
> some TEST bus) registers for debugging purposes.
>
>
>>> +}
>>
>> As questioned further below, what is this for?
>>
>>> +#else
>>> +static void sdhci_cmdq_clear_set_irqs(struct mmc_host *mmc, bool clear)
>>> +{
>>> +
>>> +}
>>> +
>>> +static void sdhci_cmdq_set_data_timeout(struct mmc_host *mmc, u32 val)
>>> +{
>>> +
>>> +}
>>> +
>>> +static void sdhci_cmdq_dump_vendor_regs(struct mmc_host *mmc)
>>> +{
>>> +
>>> +}
>>> +
>>> +static int sdhci_cmdq_init(struct sdhci_host *host, struct mmc_host *mmc,
>>> + bool dma64)
>>> +{
>>> + return -ENOSYS;
>>> +}
>>> +
>>> +static void sdhci_cmdq_set_block_size(struct mmc_host *mmc)
>>> +{
>>> +
>>> +}
>>> +
>>> +static void sdhci_cmdq_clear_set_dumpregs(struct mmc_host *mmc, bool set)
>>> +{
>>> +
>>> +}
>>> +
>>> +#endif
>>> +
>>> +static const struct cmdq_host_ops sdhci_cmdq_ops = {
>>> + .clear_set_irqs = sdhci_cmdq_clear_set_irqs,
>>> + .set_data_timeout = sdhci_cmdq_set_data_timeout,
>>> + .dump_vendor_regs = sdhci_cmdq_dump_vendor_regs,
>>> + .set_block_size = sdhci_cmdq_set_block_size,
>>> + .clear_set_dumpregs = sdhci_cmdq_clear_set_dumpregs,
>>> +};
>>
>> It would be up to individual drivers to provide CQHCI ops.
>>
>>> +
>>> int sdhci_add_host(struct sdhci_host *host)
>>> {
>>> struct mmc_host *mmc;
>>> @@ -3341,11 +3465,25 @@ int sdhci_add_host(struct sdhci_host *host)
>>> if (ret)
>>> goto unled;
>>>
>>> - pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>>> - mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
>>> + if (mmc->caps2 & MMC_CAP2_CMD_QUEUE) {
>>> + bool dma64 = (host->flags & SDHCI_USE_ADMA_64BIT) ?
>>> + true : false;
>>> + ret = sdhci_cmdq_init(host, mmc, dma64);
>>
>> You must initialize before mmc_add_host() because mmc_add_host() also starts
>> the host. If you re-base on top of the SDHCI patches, then individual
>> drivers can take advantage of the new sdhci_setup_host() /
>> __sdhci_add_host() split.
>>
>>> + if (ret)
>>> + pr_err("%s: CMDQ init: failed (%d)\n",
>>> + mmc_hostname(host->mmc), ret);
>>> + else
>>> + host->cq_host->ops = &sdhci_cmdq_ops;
>>> + }
>>> +
>>> + pr_info("%s: SDHCI controller on %s [%s] using %s in %s mode\n",
>>> + mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
>>> (host->flags & SDHCI_USE_ADMA) ?
>>> - (host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
>>> - (host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
>>> + ((host->flags & SDHCI_USE_ADMA_64BIT) ?
>>> + "64-bit ADMA" : "32-bit ADMA") :
>>> + ((host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO"),
>>> + ((mmc->caps2 & MMC_CAP2_CMD_QUEUE) && !ret) ?
>>> + "CMDQ" : "legacy");
>>
>> It is probably better if CQHCI prints its own info. That way it you get a
>> consistent message irrespective of whether the driver uses SDHCI. Also you
>> can print other CQHCI details such as the version.
>>
>>>
>>> sdhci_enable_card_detection(host);
>>>
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 609f87c..fccc750 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -150,6 +150,8 @@
>>> SDHCI_INT_DATA_TIMEOUT | SDHCI_INT_DATA_CRC | \
>>> SDHCI_INT_DATA_END_BIT | SDHCI_INT_ADMA_ERROR | \
>>> SDHCI_INT_BLK_GAP)
>>> +
>>> +#define SDHCI_INT_CMDQ_EN (0x1 << 14)
>>
>> We can define this bit for convenience but we need a comment to say that it
>> is non-standard.
>>
>>> #define SDHCI_INT_ALL_MASK ((unsigned int)-1)
>>>
>>> #define SDHCI_ACMD12_ERR 0x3C
>>> @@ -446,6 +448,7 @@ struct sdhci_host {
>>> #define SDHCI_PV_ENABLED (1<<8) /* Preset value enabled */
>>> #define SDHCI_SDIO_IRQ_ENABLED (1<<9) /* SDIO irq enabled */
>>> #define SDHCI_USE_64_BIT_DMA (1<<12) /* Use 64-bit DMA */
>>> +#define SDHCI_USE_ADMA_64BIT (1<<12) /* Host is 64-bit ADMA
>>> capable */
>>
>> SDHCI_USE_ADMA_64BIT looks redundant.
>>
>>> #define SDHCI_HS400_TUNING (1<<13) /* Tuning for HS400 */
>>>
>>> unsigned int version; /* SDHCI spec. version */
>>> @@ -509,6 +512,8 @@ struct sdhci_host {
>>> unsigned int tuning_mode; /* Re-tuning mode supported by
>>> host */
>>> #define SDHCI_TUNING_MODE_1 0
>>>
>>> + struct cmdq_host *cq_host;
>>> +
>>
>> The individual drivers will have to have their own reference to CQHCI.
>>
>>> unsigned long private[0] ____cacheline_aligned;
>>> };
>>>
>>> @@ -544,6 +549,7 @@ struct sdhci_ops {
>>> void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
>>> void (*platform_init)(struct sdhci_host *host);
>>> void (*card_event)(struct sdhci_host *host);
>>> + void (*clear_set_dumpregs)(struct sdhci_host *host, bool set);
>>
>> What is this for?
>
> This is vendor specific callback to enable/disable some TEST bus registers
> for debugging purposes.
>
>>
>>> void (*voltage_switch)(struct sdhci_host *host);
>>> int (*select_drive_strength)(struct sdhci_host *host,
>>> struct mmc_card *card,
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
