Re: [PATCH] mmc: sdhci-of-arasan: Override SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP

[Adrian Hunter <adrian.hunter@intel.com>]

On 25/10/22 22:15, Marek Vasut wrote:
> On Xilinx ZynqMP, the reg_capabilities (SDIO) Register
> 
> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabilities.html#
> Absolute Address  0x00FF160040 (SD0)
> Reset Value       0x280737EC6481
> 
> really reads 0x200737EC6481 . The interesting part is the
> top 32 bits, which are SDHCI_CAPABILITIES_1 = 0x2007. The
> missing 0x800 is SDHCI_RETUNING_TIMER_COUNT_MASK=0, which
> makes the SDHCI core disable retuning timer.
> 
> Fix this up here by explicitly setting tuning_count to 8
> as it should be, otherwise an eMMC might fail in various
> thermal conditions
> 
> Note that the diff is best shown with -w option, this makes it
> visible what happened with !sdhci_arasan->has_cqe conditional,
> which is placed between sdhci_setup_host() and __sdhci_add_host()
> calls. Since sdhci_add_host() is also a sequence of these two
> calls and host->tuning_count must be overriden before calling

overriden -> overridden

> __sdhci_add_host(), call the two calls separately and do all
> the adjustments between them in either case.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-mmc@vger.kernel.org
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 57 ++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 3997cad1f793d..465498f2a7c0f 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -1521,37 +1521,56 @@ static int sdhci_arasan_register_sdclk(struct sdhci_arasan_data *sdhci_arasan,
>  	return 0;
>  }
>  
> -static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
> +static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan,
> +				 struct device *dev)
>  {
>  	struct sdhci_host *host = sdhci_arasan->host;
>  	struct cqhci_host *cq_host;
>  	bool dma64;
>  	int ret;
>  
> -	if (!sdhci_arasan->has_cqe)
> -		return sdhci_add_host(host);
> -
>  	ret = sdhci_setup_host(host);
>  	if (ret)
>  		return ret;
>  
> -	cq_host = devm_kzalloc(host->mmc->parent,
> -			       sizeof(*cq_host), GFP_KERNEL);
> -	if (!cq_host) {
> -		ret = -ENOMEM;
> -		goto cleanup;
> -	}
> +	/*
> +	 * On Xilinx ZynqMP, the reg_capabilities (SDIO) Register
> +	 *
> +	 * https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabilities.html#
> +	 * Absolute Address  0x00FF160040 (SD0)
> +	 * Reset Value	     0x280737EC6481
> +	 *
> +	 * really reads 0x200737EC6481 . The interesting part is the
> +	 * top 32 bits, which are SDHCI_CAPABILITIES_1 = 0x2007. The
> +	 * missing 0x800 is SDHCI_RETUNING_TIMER_COUNT_MASK=0, which
> +	 * makes the SDHCI core disable retuning timer.

Are you aware that caps can be changed in DT via "sdhci-caps" and
"sdhci-caps-mask" ?

> +	 *
> +	 * Fix this up here by explicitly setting tuning_count to 8
> +	 * as it should be, otherwise an eMMC might fail in various
> +	 * thermal conditions.
> +	 */
> +	if (of_device_is_compatible(dev->of_node, "xlnx,zynqmp-8.9a"))
> +		host->tuning_count = 1 << (8 - 1);
> +
> +	if (sdhci_arasan->has_cqe) {
> +		cq_host = devm_kzalloc(host->mmc->parent,
> +				       sizeof(*cq_host), GFP_KERNEL);
> +		if (!cq_host) {
> +			ret = -ENOMEM;
> +			goto cleanup;
> +		}
>  
> -	cq_host->mmio = host->ioaddr + SDHCI_ARASAN_CQE_BASE_ADDR;
> -	cq_host->ops = &sdhci_arasan_cqhci_ops;
> +		cq_host->mmio = host->ioaddr + SDHCI_ARASAN_CQE_BASE_ADDR;
> +		cq_host->ops = &sdhci_arasan_cqhci_ops;
>  
> -	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
> -	if (dma64)
> -		cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
> +		dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
> +		if (dma64)
> +			cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
>  
> -	ret = cqhci_init(cq_host, host->mmc, dma64);
> -	if (ret)
> -		goto cleanup;
> +		ret = cqhci_init(cq_host, host->mmc, dma64);
> +		if (ret)
> +			goto cleanup;
> +	}
>  
>  	ret = __sdhci_add_host(host);
>  	if (ret)
> @@ -1711,7 +1730,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  			host->mmc->caps2 |= MMC_CAP2_CQE_DCMD;
>  	}
>  
> -	ret = sdhci_arasan_add_host(sdhci_arasan);
> +	ret = sdhci_arasan_add_host(sdhci_arasan, &pdev->dev);
>  	if (ret)
>  		goto err_add_host;
>