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

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

[Marek Vasut]

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
__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.
+	 *
+	 * 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;
 
-- 
2.35.1

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

[Marek Vasut]

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
__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.
+	 *
+	 * 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;
 
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Adrian Hunter]

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;
>  

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

[Adrian Hunter]

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;
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marek Vasut]

On 10/26/22 08:07, Adrian Hunter wrote:
> 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

Fixed

>> __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" ?

No, I wasn't aware of those.

Is that the preferred approach to this fix, over handling it in the driver ?

I think the driver-side fix would be preferable, because it also fixes 
systems which use legacy DTs without the sdhci-caps properties, which 
would be all ZynqMP systems thus far.

(and I would also still prefer to get feedback from Xilinx on why does 
the value specified in UG1087 not match what is read out of the hardware)

[...]

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

[Marek Vasut]

On 10/26/22 08:07, Adrian Hunter wrote:
> 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

Fixed

>> __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" ?

No, I wasn't aware of those.

Is that the preferred approach to this fix, over handling it in the driver ?

I think the driver-side fix would be preferable, because it also fixes 
systems which use legacy DTs without the sdhci-caps properties, which 
would be all ZynqMP systems thus far.

(and I would also still prefer to get feedback from Xilinx on why does 
the value specified in UG1087 not match what is read out of the hardware)

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Potthuri, Sai Krishna]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, October 26, 2022 2:50 PM
> To: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On 10/26/22 08:07, Adrian Hunter wrote:
> > 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_capabilit
> >> ies.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
> 
> Fixed
> 
> >> __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" ?
> 
> No, I wasn't aware of those.
> 
> Is that the preferred approach to this fix, over handling it in the driver ?
> 
> I think the driver-side fix would be preferable, because it also fixes systems
> which use legacy DTs without the sdhci-caps properties, which would be all
> ZynqMP systems thus far.
> 
> (and I would also still prefer to get feedback from Xilinx on why does the
> value specified in UG1087 not match what is read out of the hardware)
Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL, we have
an ERRATA for the same.
https://support.xilinx.com/s/article/68550?language=en_US

Xilinx recommendation is to program the appropriate value in the retuning
timer count field based on the specific requirements via DT property.

Regards
Sai Krishna

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

[Potthuri, Sai Krishna]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, October 26, 2022 2:50 PM
> To: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On 10/26/22 08:07, Adrian Hunter wrote:
> > 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_capabilit
> >> ies.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
> 
> Fixed
> 
> >> __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" ?
> 
> No, I wasn't aware of those.
> 
> Is that the preferred approach to this fix, over handling it in the driver ?
> 
> I think the driver-side fix would be preferable, because it also fixes systems
> which use legacy DTs without the sdhci-caps properties, which would be all
> ZynqMP systems thus far.
> 
> (and I would also still prefer to get feedback from Xilinx on why does the
> value specified in UG1087 not match what is read out of the hardware)
Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL, we have
an ERRATA for the same.
https://support.xilinx.com/s/article/68550?language=en_US

Xilinx recommendation is to program the appropriate value in the retuning
timer count field based on the specific requirements via DT property.

Regards
Sai Krishna
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marek Vasut]

On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> Hi Marek,

Hi,

>>>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabilit
>>>> ies.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
>>
>> Fixed
>>
>>>> __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" ?
>>
>> No, I wasn't aware of those.
>>
>> Is that the preferred approach to this fix, over handling it in the driver ?
>>
>> I think the driver-side fix would be preferable, because it also fixes systems
>> which use legacy DTs without the sdhci-caps properties, which would be all
>> ZynqMP systems thus far.
>>
>> (and I would also still prefer to get feedback from Xilinx on why does the
>> value specified in UG1087 not match what is read out of the hardware)
> Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL, we have
> an ERRATA for the same.
> https://support.xilinx.com/s/article/68550?language=en_US
> 
> Xilinx recommendation is to program the appropriate value in the retuning
> timer count field based on the specific requirements via DT property.

Why is the retuning timer disabled for HS200 mode ?

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

[Marek Vasut]

On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> Hi Marek,

Hi,

>>>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabilit
>>>> ies.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
>>
>> Fixed
>>
>>>> __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" ?
>>
>> No, I wasn't aware of those.
>>
>> Is that the preferred approach to this fix, over handling it in the driver ?
>>
>> I think the driver-side fix would be preferable, because it also fixes systems
>> which use legacy DTs without the sdhci-caps properties, which would be all
>> ZynqMP systems thus far.
>>
>> (and I would also still prefer to get feedback from Xilinx on why does the
>> value specified in UG1087 not match what is read out of the hardware)
> Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL, we have
> an ERRATA for the same.
> https://support.xilinx.com/s/article/68550?language=en_US
> 
> Xilinx recommendation is to program the appropriate value in the retuning
> timer count field based on the specific requirements via DT property.

Why is the retuning timer disabled for HS200 mode ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Potthuri, Sai Krishna]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, December 21, 2022 3:10 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian Hunter
> <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> 
> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> > Hi Marek,
> 
> Hi,
> 
> >>>>
> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
> >>>> it
> >>>> ies.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
> >>
> >> Fixed
> >>
> >>>> __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" ?
> >>
> >> No, I wasn't aware of those.
> >>
> >> Is that the preferred approach to this fix, over handling it in the driver ?
> >>
> >> I think the driver-side fix would be preferable, because it also
> >> fixes systems which use legacy DTs without the sdhci-caps properties,
> >> which would be all ZynqMP systems thus far.
> >>
> >> (and I would also still prefer to get feedback from Xilinx on why
> >> does the value specified in UG1087 not match what is read out of the
> >> hardware)
> > Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL,
> > we have an ERRATA for the same.
> > https://support.xilinx.com/s/article/68550?language=en_US
> >
> > Xilinx recommendation is to program the appropriate value in the
> > retuning timer count field based on the specific requirements via DT
> property.
> 
> Why is the retuning timer disabled for HS200 mode ?
Based on discussions with the Xilinx IP design team, they told retuning is
not required as Xilinx uses DLL for higher frequency modes.
So, we disabled retuning by default even in Xilinx next generation platforms
like Versal.
Even in our internal PVT testing also, without retuning we didn't see any issues.

Did you face any real issue without this re-tuning? If yes, could you please
provide some details about the test case.

Regards
Sai Krishna

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

[Potthuri, Sai Krishna]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, December 21, 2022 3:10 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian Hunter
> <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> 
> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> > Hi Marek,
> 
> Hi,
> 
> >>>>
> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
> >>>> it
> >>>> ies.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
> >>
> >> Fixed
> >>
> >>>> __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" ?
> >>
> >> No, I wasn't aware of those.
> >>
> >> Is that the preferred approach to this fix, over handling it in the driver ?
> >>
> >> I think the driver-side fix would be preferable, because it also
> >> fixes systems which use legacy DTs without the sdhci-caps properties,
> >> which would be all ZynqMP systems thus far.
> >>
> >> (and I would also still prefer to get feedback from Xilinx on why
> >> does the value specified in UG1087 not match what is read out of the
> >> hardware)
> > Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL,
> > we have an ERRATA for the same.
> > https://support.xilinx.com/s/article/68550?language=en_US
> >
> > Xilinx recommendation is to program the appropriate value in the
> > retuning timer count field based on the specific requirements via DT
> property.
> 
> Why is the retuning timer disabled for HS200 mode ?
Based on discussions with the Xilinx IP design team, they told retuning is
not required as Xilinx uses DLL for higher frequency modes.
So, we disabled retuning by default even in Xilinx next generation platforms
like Versal.
Even in our internal PVT testing also, without retuning we didn't see any issues.

Did you face any real issue without this re-tuning? If yes, could you please
provide some details about the test case.

Regards
Sai Krishna
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Adrian Hunter]

On 26/10/22 12:20, Marek Vasut wrote:
> On 10/26/22 08:07, Adrian Hunter wrote:
>> 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
> 
> Fixed
> 
>>> __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" ?
> 
> No, I wasn't aware of those.
> 
> Is that the preferred approach to this fix, over handling it in the driver ?

I guess ideally.  Mainline does not really need the driver
fix because it seems it can be done by DT.  Older kernels
are a separate issue really.

> 
> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.

You could backport support of the properties "sdhci-caps"
and "sdhci-caps-mask".

> 
> (and I would also still prefer to get feedback from Xilinx on why does the value specified in UG1087 not match what is read out of the hardware)
> 
> [...]

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

[Adrian Hunter]

On 26/10/22 12:20, Marek Vasut wrote:
> On 10/26/22 08:07, Adrian Hunter wrote:
>> 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
> 
> Fixed
> 
>>> __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" ?
> 
> No, I wasn't aware of those.
> 
> Is that the preferred approach to this fix, over handling it in the driver ?

I guess ideally.  Mainline does not really need the driver
fix because it seems it can be done by DT.  Older kernels
are a separate issue really.

> 
> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.

You could backport support of the properties "sdhci-caps"
and "sdhci-caps-mask".

> 
> (and I would also still prefer to get feedback from Xilinx on why does the value specified in UG1087 not match what is read out of the hardware)
> 
> [...]


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marek Vasut]

On 12/29/22 13:51, Adrian Hunter wrote:
> On 26/10/22 12:20, Marek Vasut wrote:
>> On 10/26/22 08:07, Adrian Hunter wrote:
>>> 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
>>
>> Fixed
>>
>>>> __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" ?
>>
>> No, I wasn't aware of those.
>>
>> Is that the preferred approach to this fix, over handling it in the driver ?
> 
> I guess ideally.  Mainline does not really need the driver
> fix because it seems it can be done by DT.  Older kernels
> are a separate issue really.
> 
>>
>> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.
> 
> You could backport support of the properties "sdhci-caps"
> and "sdhci-caps-mask".

This won't help. Vivado (the xilinx FPGA design tool) is capable of 
generating DTs, so you can end up with a combination of new Linux kernel 
and old generated DT, which is still missing the 
sdhci-caps/sdhci-caps-mask .

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

[Marek Vasut]

On 12/29/22 13:51, Adrian Hunter wrote:
> On 26/10/22 12:20, Marek Vasut wrote:
>> On 10/26/22 08:07, Adrian Hunter wrote:
>>> 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
>>
>> Fixed
>>
>>>> __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" ?
>>
>> No, I wasn't aware of those.
>>
>> Is that the preferred approach to this fix, over handling it in the driver ?
> 
> I guess ideally.  Mainline does not really need the driver
> fix because it seems it can be done by DT.  Older kernels
> are a separate issue really.
> 
>>
>> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.
> 
> You could backport support of the properties "sdhci-caps"
> and "sdhci-caps-mask".

This won't help. Vivado (the xilinx FPGA design tool) is capable of 
generating DTs, so you can end up with a combination of new Linux kernel 
and old generated DT, which is still missing the 
sdhci-caps/sdhci-caps-mask .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Adrian Hunter]

On 30/12/22 08:42, Marek Vasut wrote:
> On 12/29/22 13:51, Adrian Hunter wrote:
>> On 26/10/22 12:20, Marek Vasut wrote:
>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>> 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
>>>
>>> Fixed
>>>
>>>>> __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" ?
>>>
>>> No, I wasn't aware of those.
>>>
>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>
>> I guess ideally.  Mainline does not really need the driver
>> fix because it seems it can be done by DT.  Older kernels
>> are a separate issue really.
>>
>>>
>>> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.
>>
>> You could backport support of the properties "sdhci-caps"
>> and "sdhci-caps-mask".
> 
> This won't help. Vivado (the xilinx FPGA design tool) is capable of generating DTs, so you can end up with a combination of new Linux kernel and old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .

That is a bit sad.  You might want to push for changing that situation.

Send an updated patch then.

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

[Adrian Hunter]

On 30/12/22 08:42, Marek Vasut wrote:
> On 12/29/22 13:51, Adrian Hunter wrote:
>> On 26/10/22 12:20, Marek Vasut wrote:
>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>> 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
>>>
>>> Fixed
>>>
>>>>> __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" ?
>>>
>>> No, I wasn't aware of those.
>>>
>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>
>> I guess ideally.  Mainline does not really need the driver
>> fix because it seems it can be done by DT.  Older kernels
>> are a separate issue really.
>>
>>>
>>> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.
>>
>> You could backport support of the properties "sdhci-caps"
>> and "sdhci-caps-mask".
> 
> This won't help. Vivado (the xilinx FPGA design tool) is capable of generating DTs, so you can end up with a combination of new Linux kernel and old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .

That is a bit sad.  You might want to push for changing that situation.

Send an updated patch then.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michal Simek]

On 12/30/22 13:57, Adrian Hunter wrote:
> On 30/12/22 08:42, Marek Vasut wrote:
>> On 12/29/22 13:51, Adrian Hunter wrote:
>>> On 26/10/22 12:20, Marek Vasut wrote:
>>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>>> 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
>>>>
>>>> Fixed
>>>>
>>>>>> __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" ?
>>>>
>>>> No, I wasn't aware of those.
>>>>
>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>
>>> I guess ideally.  Mainline does not really need the driver
>>> fix because it seems it can be done by DT.  Older kernels
>>> are a separate issue really.
>>>
>>>>
>>>> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.
>>>
>>> You could backport support of the properties "sdhci-caps"
>>> and "sdhci-caps-mask".
>>
>> This won't help. Vivado (the xilinx FPGA design tool) is capable of generating DTs, so you can end up with a combination of new Linux kernel and old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .
> 
> That is a bit sad.  You might want to push for changing that situation.
> 
> Send an updated patch then.
> 

Xilinx Device Tree Generator, which is the tool for DT generation, was never 
designed to be directly used without any change. It was designed to help you to 
describe the system as much as possible. It means you get the base and you need 
to change things which are not properly described. That's why just do it.

Thanks,
Michal

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

[Michal Simek]

On 12/30/22 13:57, Adrian Hunter wrote:
> On 30/12/22 08:42, Marek Vasut wrote:
>> On 12/29/22 13:51, Adrian Hunter wrote:
>>> On 26/10/22 12:20, Marek Vasut wrote:
>>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>>> 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
>>>>
>>>> Fixed
>>>>
>>>>>> __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" ?
>>>>
>>>> No, I wasn't aware of those.
>>>>
>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>
>>> I guess ideally.  Mainline does not really need the driver
>>> fix because it seems it can be done by DT.  Older kernels
>>> are a separate issue really.
>>>
>>>>
>>>> I think the driver-side fix would be preferable, because it also fixes systems which use legacy DTs without the sdhci-caps properties, which would be all ZynqMP systems thus far.
>>>
>>> You could backport support of the properties "sdhci-caps"
>>> and "sdhci-caps-mask".
>>
>> This won't help. Vivado (the xilinx FPGA design tool) is capable of generating DTs, so you can end up with a combination of new Linux kernel and old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .
> 
> That is a bit sad.  You might want to push for changing that situation.
> 
> Send an updated patch then.
> 

Xilinx Device Tree Generator, which is the tool for DT generation, was never 
designed to be directly used without any change. It was designed to help you to 
describe the system as much as possible. It means you get the base and you need 
to change things which are not properly described. That's why just do it.

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marek Vasut]

On 1/2/23 09:24, Michal Simek wrote:
> 
> 
> On 12/30/22 13:57, Adrian Hunter wrote:
>> On 30/12/22 08:42, Marek Vasut wrote:
>>> On 12/29/22 13:51, Adrian Hunter wrote:
>>>> On 26/10/22 12:20, Marek Vasut wrote:
>>>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>>>> 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
>>>>>
>>>>> Fixed
>>>>>
>>>>>>> __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" ?
>>>>>
>>>>> No, I wasn't aware of those.
>>>>>
>>>>> Is that the preferred approach to this fix, over handling it in the 
>>>>> driver ?
>>>>
>>>> I guess ideally.  Mainline does not really need the driver
>>>> fix because it seems it can be done by DT.  Older kernels
>>>> are a separate issue really.
>>>>
>>>>>
>>>>> I think the driver-side fix would be preferable, because it also 
>>>>> fixes systems which use legacy DTs without the sdhci-caps 
>>>>> properties, which would be all ZynqMP systems thus far.
>>>>
>>>> You could backport support of the properties "sdhci-caps"
>>>> and "sdhci-caps-mask".
>>>
>>> This won't help. Vivado (the xilinx FPGA design tool) is capable of 
>>> generating DTs, so you can end up with a combination of new Linux 
>>> kernel and old generated DT, which is still missing the 
>>> sdhci-caps/sdhci-caps-mask .
>>
>> That is a bit sad.  You might want to push for changing that situation.
>>
>> Send an updated patch then.
>>
> 
> Xilinx Device Tree Generator, which is the tool for DT generation, was 
> never designed to be directly used without any change. It was designed 
> to help you to describe the system as much as possible. It means you get 
> the base and you need to change things which are not properly described. 
> That's why just do it.

I am under the impression that petalinux does pull the XSA from Vivado 
and directly builds U-Boot and Linux with DT somehow derived from the 
XSA, maybe using DTG ?

(note that I am not using petalinux)

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

[Marek Vasut]

On 1/2/23 09:24, Michal Simek wrote:
> 
> 
> On 12/30/22 13:57, Adrian Hunter wrote:
>> On 30/12/22 08:42, Marek Vasut wrote:
>>> On 12/29/22 13:51, Adrian Hunter wrote:
>>>> On 26/10/22 12:20, Marek Vasut wrote:
>>>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>>>> 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
>>>>>
>>>>> Fixed
>>>>>
>>>>>>> __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" ?
>>>>>
>>>>> No, I wasn't aware of those.
>>>>>
>>>>> Is that the preferred approach to this fix, over handling it in the 
>>>>> driver ?
>>>>
>>>> I guess ideally.  Mainline does not really need the driver
>>>> fix because it seems it can be done by DT.  Older kernels
>>>> are a separate issue really.
>>>>
>>>>>
>>>>> I think the driver-side fix would be preferable, because it also 
>>>>> fixes systems which use legacy DTs without the sdhci-caps 
>>>>> properties, which would be all ZynqMP systems thus far.
>>>>
>>>> You could backport support of the properties "sdhci-caps"
>>>> and "sdhci-caps-mask".
>>>
>>> This won't help. Vivado (the xilinx FPGA design tool) is capable of 
>>> generating DTs, so you can end up with a combination of new Linux 
>>> kernel and old generated DT, which is still missing the 
>>> sdhci-caps/sdhci-caps-mask .
>>
>> That is a bit sad.  You might want to push for changing that situation.
>>
>> Send an updated patch then.
>>
> 
> Xilinx Device Tree Generator, which is the tool for DT generation, was 
> never designed to be directly used without any change. It was designed 
> to help you to describe the system as much as possible. It means you get 
> the base and you need to change things which are not properly described. 
> That's why just do it.

I am under the impression that petalinux does pull the XSA from Vivado 
and directly builds U-Boot and Linux with DT somehow derived from the 
XSA, maybe using DTG ?

(note that I am not using petalinux)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marek Vasut]

On 12/22/22 10:20, Potthuri, Sai Krishna wrote:
> Hi Marek,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, December 21, 2022 3:10 PM
>> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian Hunter
>> <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
>> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
>> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
>> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
>>
>> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>>>>>
>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
>>>>>> it
>>>>>> ies.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
>>>>
>>>> Fixed
>>>>
>>>>>> __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" ?
>>>>
>>>> No, I wasn't aware of those.
>>>>
>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>>
>>>> I think the driver-side fix would be preferable, because it also
>>>> fixes systems which use legacy DTs without the sdhci-caps properties,
>>>> which would be all ZynqMP systems thus far.
>>>>
>>>> (and I would also still prefer to get feedback from Xilinx on why
>>>> does the value specified in UG1087 not match what is read out of the
>>>> hardware)
>>> Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL,
>>> we have an ERRATA for the same.
>>> https://support.xilinx.com/s/article/68550?language=en_US
>>>
>>> Xilinx recommendation is to program the appropriate value in the
>>> retuning timer count field based on the specific requirements via DT
>> property.
>>
>> Why is the retuning timer disabled for HS200 mode ?
> Based on discussions with the Xilinx IP design team, they told retuning is
> not required as Xilinx uses DLL for higher frequency modes.

Does this require the eMMC "DS" line ?

> So, we disabled retuning by default even in Xilinx next generation platforms
> like Versal.
> Even in our internal PVT testing also, without retuning we didn't see any issues.
> 
> Did you face any real issue without this re-tuning? If yes, could you please
> provide some details about the test case.

Yes, on devices with wider temperature range, the eMMC might suffer read 
failures over time.

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

[Marek Vasut]

On 12/22/22 10:20, Potthuri, Sai Krishna wrote:
> Hi Marek,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, December 21, 2022 3:10 PM
>> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian Hunter
>> <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
>> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
>> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
>> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
>>
>> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>>>>>
>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
>>>>>> it
>>>>>> ies.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
>>>>
>>>> Fixed
>>>>
>>>>>> __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" ?
>>>>
>>>> No, I wasn't aware of those.
>>>>
>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>>
>>>> I think the driver-side fix would be preferable, because it also
>>>> fixes systems which use legacy DTs without the sdhci-caps properties,
>>>> which would be all ZynqMP systems thus far.
>>>>
>>>> (and I would also still prefer to get feedback from Xilinx on why
>>>> does the value specified in UG1087 not match what is read out of the
>>>> hardware)
>>> Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL,
>>> we have an ERRATA for the same.
>>> https://support.xilinx.com/s/article/68550?language=en_US
>>>
>>> Xilinx recommendation is to program the appropriate value in the
>>> retuning timer count field based on the specific requirements via DT
>> property.
>>
>> Why is the retuning timer disabled for HS200 mode ?
> Based on discussions with the Xilinx IP design team, they told retuning is
> not required as Xilinx uses DLL for higher frequency modes.

Does this require the eMMC "DS" line ?

> So, we disabled retuning by default even in Xilinx next generation platforms
> like Versal.
> Even in our internal PVT testing also, without retuning we didn't see any issues.
> 
> Did you face any real issue without this re-tuning? If yes, could you please
> provide some details about the test case.

Yes, on devices with wider temperature range, the eMMC might suffer read 
failures over time.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michal Simek]

On 1/3/23 21:35, Marek Vasut wrote:
> On 1/2/23 09:24, Michal Simek wrote:
>>
>>
>> On 12/30/22 13:57, Adrian Hunter wrote:
>>> On 30/12/22 08:42, Marek Vasut wrote:
>>>> On 12/29/22 13:51, Adrian Hunter wrote:
>>>>> On 26/10/22 12:20, Marek Vasut wrote:
>>>>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>>>>> 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
>>>>>>
>>>>>> Fixed
>>>>>>
>>>>>>>> __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" ?
>>>>>>
>>>>>> No, I wasn't aware of those.
>>>>>>
>>>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>>>
>>>>> I guess ideally.  Mainline does not really need the driver
>>>>> fix because it seems it can be done by DT.  Older kernels
>>>>> are a separate issue really.
>>>>>
>>>>>>
>>>>>> I think the driver-side fix would be preferable, because it also fixes 
>>>>>> systems which use legacy DTs without the sdhci-caps properties, which 
>>>>>> would be all ZynqMP systems thus far.
>>>>>
>>>>> You could backport support of the properties "sdhci-caps"
>>>>> and "sdhci-caps-mask".
>>>>
>>>> This won't help. Vivado (the xilinx FPGA design tool) is capable of 
>>>> generating DTs, so you can end up with a combination of new Linux kernel and 
>>>> old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .
>>>
>>> That is a bit sad.  You might want to push for changing that situation.
>>>
>>> Send an updated patch then.
>>>
>>
>> Xilinx Device Tree Generator, which is the tool for DT generation, was never 
>> designed to be directly used without any change. It was designed to help you 
>> to describe the system as much as possible. It means you get the base and you 
>> need to change things which are not properly described. That's why just do it.
> 
> I am under the impression that petalinux does pull the XSA from Vivado and 
> directly builds U-Boot and Linux with DT somehow derived from the XSA, maybe 
> using DTG ?
> 
> (note that I am not using petalinux)

They do use it but they are not keeping backward compatibility that's why they 
don't need to solve this problem.

Thanks,
Michal

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

[Michal Simek]

On 1/3/23 21:35, Marek Vasut wrote:
> On 1/2/23 09:24, Michal Simek wrote:
>>
>>
>> On 12/30/22 13:57, Adrian Hunter wrote:
>>> On 30/12/22 08:42, Marek Vasut wrote:
>>>> On 12/29/22 13:51, Adrian Hunter wrote:
>>>>> On 26/10/22 12:20, Marek Vasut wrote:
>>>>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>>>>> 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
>>>>>>
>>>>>> Fixed
>>>>>>
>>>>>>>> __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" ?
>>>>>>
>>>>>> No, I wasn't aware of those.
>>>>>>
>>>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>>>
>>>>> I guess ideally.  Mainline does not really need the driver
>>>>> fix because it seems it can be done by DT.  Older kernels
>>>>> are a separate issue really.
>>>>>
>>>>>>
>>>>>> I think the driver-side fix would be preferable, because it also fixes 
>>>>>> systems which use legacy DTs without the sdhci-caps properties, which 
>>>>>> would be all ZynqMP systems thus far.
>>>>>
>>>>> You could backport support of the properties "sdhci-caps"
>>>>> and "sdhci-caps-mask".
>>>>
>>>> This won't help. Vivado (the xilinx FPGA design tool) is capable of 
>>>> generating DTs, so you can end up with a combination of new Linux kernel and 
>>>> old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .
>>>
>>> That is a bit sad.  You might want to push for changing that situation.
>>>
>>> Send an updated patch then.
>>>
>>
>> Xilinx Device Tree Generator, which is the tool for DT generation, was never 
>> designed to be directly used without any change. It was designed to help you 
>> to describe the system as much as possible. It means you get the base and you 
>> need to change things which are not properly described. That's why just do it.
> 
> I am under the impression that petalinux does pull the XSA from Vivado and 
> directly builds U-Boot and Linux with DT somehow derived from the XSA, maybe 
> using DTG ?
> 
> (note that I am not using petalinux)

They do use it but they are not keeping backward compatibility that's why they 
don't need to solve this problem.

Thanks,
Michal


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Potthuri, Sai Krishna]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, January 4, 2023 2:17 AM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian Hunter
> <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Goud,
> Srinivas <srinivas.goud@amd.com>
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On 12/22/22 10:20, Potthuri, Sai Krishna wrote:
> > Hi Marek,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Wednesday, December 21, 2022 3:10 PM
> >> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian
> >> Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> >> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> >> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> >> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> >>
> >> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>>>>>
> >> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
> >>>>>> it
> >>>>>> ies.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
> >>>>
> >>>> Fixed
> >>>>
> >>>>>> __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" ?
> >>>>
> >>>> No, I wasn't aware of those.
> >>>>
> >>>> Is that the preferred approach to this fix, over handling it in the driver ?
> >>>>
> >>>> I think the driver-side fix would be preferable, because it also
> >>>> fixes systems which use legacy DTs without the sdhci-caps
> >>>> properties, which would be all ZynqMP systems thus far.
> >>>>
> >>>> (and I would also still prefer to get feedback from Xilinx on why
> >>>> does the value specified in UG1087 not match what is read out of
> >>>> the
> >>>> hardware)
> >>> Reset value of the retuning timer count is set to 0x0 via ZynqMP
> >>> FSBL, we have an ERRATA for the same.
> >>> https://support.xilinx.com/s/article/68550?language=en_US
> >>>
> >>> Xilinx recommendation is to program the appropriate value in the
> >>> retuning timer count field based on the specific requirements via DT
> >> property.
> >>
> >> Why is the retuning timer disabled for HS200 mode ?
> > Based on discussions with the Xilinx IP design team, they told
> > retuning is not required as Xilinx uses DLL for higher frequency modes.
> 
> Does this require the eMMC "DS" line ?
No.
> 
> > So, we disabled retuning by default even in Xilinx next generation
> > platforms like Versal.
> > Even in our internal PVT testing also, without retuning we didn't see any
> issues.
> >
> > Did you face any real issue without this re-tuning? If yes, could you
> > please provide some details about the test case.
> 
> Yes, on devices with wider temperature range, the eMMC might suffer read
> failures over time.
During Xilinx internal PVT testing we didn't see any issues without doing the
retuning. If you can see the issue at a particular temperature, please let
us know.
 
In case if there is any issue, we can make use of the DT property as mentioned.

Regards
Sai Krishna 

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

[Potthuri, Sai Krishna]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, January 4, 2023 2:17 AM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian Hunter
> <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org; Goud,
> Srinivas <srinivas.goud@amd.com>
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On 12/22/22 10:20, Potthuri, Sai Krishna wrote:
> > Hi Marek,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Wednesday, December 21, 2022 3:10 PM
> >> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Adrian
> >> Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org
> >> Cc: Michal Simek <michal.simek@xilinx.com>; Ulf Hansson
> >> <ulf.hansson@linaro.org>; linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> >> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> >>
> >> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>>>>>
> >> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
> >>>>>> it
> >>>>>> ies.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
> >>>>
> >>>> Fixed
> >>>>
> >>>>>> __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" ?
> >>>>
> >>>> No, I wasn't aware of those.
> >>>>
> >>>> Is that the preferred approach to this fix, over handling it in the driver ?
> >>>>
> >>>> I think the driver-side fix would be preferable, because it also
> >>>> fixes systems which use legacy DTs without the sdhci-caps
> >>>> properties, which would be all ZynqMP systems thus far.
> >>>>
> >>>> (and I would also still prefer to get feedback from Xilinx on why
> >>>> does the value specified in UG1087 not match what is read out of
> >>>> the
> >>>> hardware)
> >>> Reset value of the retuning timer count is set to 0x0 via ZynqMP
> >>> FSBL, we have an ERRATA for the same.
> >>> https://support.xilinx.com/s/article/68550?language=en_US
> >>>
> >>> Xilinx recommendation is to program the appropriate value in the
> >>> retuning timer count field based on the specific requirements via DT
> >> property.
> >>
> >> Why is the retuning timer disabled for HS200 mode ?
> > Based on discussions with the Xilinx IP design team, they told
> > retuning is not required as Xilinx uses DLL for higher frequency modes.
> 
> Does this require the eMMC "DS" line ?
No.
> 
> > So, we disabled retuning by default even in Xilinx next generation
> > platforms like Versal.
> > Even in our internal PVT testing also, without retuning we didn't see any
> issues.
> >
> > Did you face any real issue without this re-tuning? If yes, could you
> > please provide some details about the test case.
> 
> Yes, on devices with wider temperature range, the eMMC might suffer read
> failures over time.
During Xilinx internal PVT testing we didn't see any issues without doing the
retuning. If you can see the issue at a particular temperature, please let
us know.
 
In case if there is any issue, we can make use of the DT property as mentioned.

Regards
Sai Krishna 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel