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

[Marek Vasut <marex@denx.de>]

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)

[...]