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@xxxxxxx> >>> --- >>> Cc: Michal Simek <michal.simek@xxxxxxxxxx> >>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx> >>> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >>> To: linux-mmc@xxxxxxxxxxxxxxx >>> --- >>> 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) > > [...]
