[V4,2/4] mmc: cqhci: DMA Configuration prior to CQE
diff mbox series

Message ID 1545262938-20636-3-git-send-email-skomatineni@nvidia.com
State New
Headers show
Series
  • HW Command Queue support for Tegra SDMMC
Related show

Commit Message

Sowjanya Komatineni Dec. 19, 2018, 11:42 p.m. UTC
eMMC-5.1 JESD84-B51 Spec (Section 6.6.39.1), mentions "Prior to
enabling command queuing, the block size shall be set to 512 B.
Device may respond with an error to CMD46/CMD47 if block size
is not 512 B".

This patch fixes the sequence to follow exact as per the spec.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/mmc/host/cqhci.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Hunter, Adrian Dec. 20, 2018, 12:37 p.m. UTC | #1
On 20/12/18 1:42 AM, Sowjanya Komatineni wrote:
> eMMC-5.1 JESD84-B51 Spec (Section 6.6.39.1), mentions "Prior to
> enabling command queuing, the block size shall be set to 512 B.
> Device may respond with an error to CMD46/CMD47 if block size
> is not 512 B".

This doesn't seem to relate to the host controller implementation.  "The
device" means the eMMC.

We don't want to disable and re-enable in the request function, so that
change is not good for controllers that don't have your problem.  Another
thing to consider is that the block size register may not need to be changed
- for example when cqhci is halted to allow a manual discard, the block size
register is not updated, so I would expect its value to be unchanged.

There are ways you can solve this in your driver.  You could look at using
SDHCI I/O accessors, and/or implement your own ->enable() instead of calling
sdhci_cqe_enable() directly.  Would that be feasible?

> 
> This patch fixes the sequence to follow exact as per the spec.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/mmc/host/cqhci.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
> index 159270e947cf..f701342e7212 100644
> --- a/drivers/mmc/host/cqhci.c
> +++ b/drivers/mmc/host/cqhci.c
> @@ -248,6 +248,9 @@ static void __cqhci_enable(struct cqhci_host *cq_host)
>  		cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
>  	}
>  
> +	if (cq_host->ops->enable)
> +		cq_host->ops->enable(mmc);
> +
>  	cqcfg &= ~(CQHCI_DCMD | CQHCI_TASK_DESC_SZ);
>  
>  	if (mmc->caps2 & MMC_CAP2_CQE_DCMD)
> @@ -273,9 +276,6 @@ static void __cqhci_enable(struct cqhci_host *cq_host)
>  
>  	mmc->cqe_on = true;
>  
> -	if (cq_host->ops->enable)
> -		cq_host->ops->enable(mmc);
> -
>  	/* Ensure all writes are done before interrupts are enabled */
>  	wmb();
>  
> @@ -561,6 +561,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	int tag = cqhci_tag(mrq);
>  	struct cqhci_host *cq_host = mmc->cqe_private;
>  	unsigned long flags;
> +	u32 cqcfg = 0;
>  
>  	if (!cq_host->enabled) {
>  		pr_err("%s: cqhci: not enabled\n", mmc_hostname(mmc));
> @@ -579,8 +580,19 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  			pr_err("%s: cqhci: CQE failed to exit halt state\n",
>  			       mmc_hostname(mmc));
>  		}
> +		/* Configuration must not be changed while enabled */
> +		cqcfg = cqhci_readl(cq_host, CQHCI_CFG);
> +		if (cqcfg & CQHCI_ENABLE) {
> +			cqcfg &= ~CQHCI_ENABLE;
> +			cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
> +		}
> +
>  		if (cq_host->ops->enable)
>  			cq_host->ops->enable(mmc);
> +
> +		cqcfg |= CQHCI_ENABLE;
> +		cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
> +
>  	}
>  
>  	if (mrq->data) {
>
Sowjanya Komatineni Dec. 20, 2018, 9:01 p.m. UTC | #2
Hi Adrian,

Thank you for the feedback.

> This doesn't seem to relate to the host controller implementation.  
> "The device" means the eMMC.

Yes, setting block size of 512B before enabling command queue is a device specific 
requirement not host specific. So thought to update in cqhci driver to follow this 
device specific sequence requirement. This also serves tegra sdhci host strictly
following this device specific requirement.

> We don't want to disable and re-enable in the request function,
> so that change is not good for controllers that don't have your problem.
> Another thing to consider is that the block size register may not need to be changed
> - for example when cqhci is halted to allow a manual discard, the block size register is
> not updated, so I would expect its value to be unchanged.

Once block size is set prior to enabling CQE, it stays same till command queue is 
disabled and I don’t think it needs reconfiguration.

> There are ways you can solve this in your driver.  
> You could look at using SDHCI I/O accessors, and/or implement your own ->enable() 
> instead of calling sdhci_cqe_enable() directly.  Would that be feasible?

Sure, will provide updated patch that takes care of this inside tegra sdhci host once you 
confirm that we don’t plan to fix this device specific sequence requirement in cqhci driver.

-Sowjanya
Hunter, Adrian Dec. 21, 2018, 7:41 a.m. UTC | #3
On 20/12/18 11:01 PM, Sowjanya Komatineni wrote:
> Hi Adrian,
> 
> Thank you for the feedback.
> 
>> This doesn't seem to relate to the host controller implementation.  
>> "The device" means the eMMC.
> 
> Yes, setting block size of 512B before enabling command queue is a device specific 
> requirement not host specific. So thought to update in cqhci driver to follow this 
> device specific sequence requirement. This also serves tegra sdhci host strictly
> following this device specific requirement.

Not sure what you mean.  The eMMC block size is set by CMD16.  In fact CMD16
is not used because 512B is the default so it never needs to be changed.

> 
>> We don't want to disable and re-enable in the request function,
>> so that change is not good for controllers that don't have your problem.
>> Another thing to consider is that the block size register may not need to be changed
>> - for example when cqhci is halted to allow a manual discard, the block size register is
>> not updated, so I would expect its value to be unchanged.
> 
> Once block size is set prior to enabling CQE, it stays same till command queue is 
> disabled and I don’t think it needs reconfiguration.
> 
>> There are ways you can solve this in your driver.  
>> You could look at using SDHCI I/O accessors, and/or implement your own ->enable() 
>> instead of calling sdhci_cqe_enable() directly.  Would that be feasible?
> 
> Sure, will provide updated patch that takes care of this inside tegra sdhci host once you 
> confirm that we don’t plan to fix this device specific sequence requirement in cqhci driver.
> 
> -Sowjanya
>  
>

Patch
diff mbox series

diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
index 159270e947cf..f701342e7212 100644
--- a/drivers/mmc/host/cqhci.c
+++ b/drivers/mmc/host/cqhci.c
@@ -248,6 +248,9 @@  static void __cqhci_enable(struct cqhci_host *cq_host)
 		cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
 	}
 
+	if (cq_host->ops->enable)
+		cq_host->ops->enable(mmc);
+
 	cqcfg &= ~(CQHCI_DCMD | CQHCI_TASK_DESC_SZ);
 
 	if (mmc->caps2 & MMC_CAP2_CQE_DCMD)
@@ -273,9 +276,6 @@  static void __cqhci_enable(struct cqhci_host *cq_host)
 
 	mmc->cqe_on = true;
 
-	if (cq_host->ops->enable)
-		cq_host->ops->enable(mmc);
-
 	/* Ensure all writes are done before interrupts are enabled */
 	wmb();
 
@@ -561,6 +561,7 @@  static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	int tag = cqhci_tag(mrq);
 	struct cqhci_host *cq_host = mmc->cqe_private;
 	unsigned long flags;
+	u32 cqcfg = 0;
 
 	if (!cq_host->enabled) {
 		pr_err("%s: cqhci: not enabled\n", mmc_hostname(mmc));
@@ -579,8 +580,19 @@  static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 			pr_err("%s: cqhci: CQE failed to exit halt state\n",
 			       mmc_hostname(mmc));
 		}
+		/* Configuration must not be changed while enabled */
+		cqcfg = cqhci_readl(cq_host, CQHCI_CFG);
+		if (cqcfg & CQHCI_ENABLE) {
+			cqcfg &= ~CQHCI_ENABLE;
+			cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
+		}
+
 		if (cq_host->ops->enable)
 			cq_host->ops->enable(mmc);
+
+		cqcfg |= CQHCI_ENABLE;
+		cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
+
 	}
 
 	if (mrq->data) {