On 13 February 2017 at 13:06, Anssi Hannula <anssi.hannula@xxxxxxxxxx> wrote:
> sdhci_arasan_get_timeout_clock() divides the frequency it has with (1 <<
> (13 + divisor)).
>
> However, the divisor is not some Arasan-specific value, but instead is
> just the Data Timeout Counter Value from the SDHCI Timeout Control
> Register.
>
> Applying it here like this is wrong as the sdhci driver already takes
> that value into account when calculating timeouts, and in fact it *sets*
> that register value based on how long a timeout is wanted.
>
> Additionally, sdhci core interprets the .get_timeout_clock callback
> return value as if it were read from hardware registers, i.e. the unit
> should be kHz or MHz depending on SDHCI_TIMEOUT_CLK_UNIT capability bit.
> This bit is set at least on the tested Zynq-7000 SoC.
>
> With the tested hardware (SDHCI_TIMEOUT_CLK_UNIT set) this results in
> too high a timeout clock rate being reported, causing the core to use
> longer-than-needed timeouts. Additionally, on a partitioned MMC
> (therefore having erase_group_def bit set) mmc_calc_max_discard()
> disables discard support as it looks like controller does not support
> the long timeouts needed for that.
>
> Do not apply the extra divisor and return the timeout clock in the
> expected unit.
>
> Tested with a Zynq-7000 SoC and a partitioned Toshiba THGBMAG5A1JBAWR
> eMMC card.
>
> Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
> Fixes: e3ec3a3d11ad ("mmc: arasan: Add driver for Arasan SDHCI")
> Cc: <stable@xxxxxxxxxxxxxxx>
Since this seems to fix the problem, I have applied this for fixes. Thanks!
If additional changes are needed, let's do those on top.
Kind regards
Uffe
> ---
>
> The .get_timeout_clock situation seems rather messy altogether, but I
> wasn't sure which direction to take it to and it is probably best to
> have that separate from this bugfix, anyway, so I kept the scope
> limited.
>
>
> drivers/mmc/host/sdhci-of-arasan.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 410a55b..1cfd7f9 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -28,13 +28,9 @@
> #include "sdhci-pltfm.h"
> #include <linux/of.h>
>
> -#define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
> #define SDHCI_ARASAN_VENDOR_REGISTER 0x78
>
> #define VENDOR_ENHANCED_STROBE BIT(0)
> -#define CLK_CTRL_TIMEOUT_SHIFT 16
> -#define CLK_CTRL_TIMEOUT_MASK (0xf << CLK_CTRL_TIMEOUT_SHIFT)
> -#define CLK_CTRL_TIMEOUT_MIN_EXP 13
>
> #define PHY_CLK_TOO_SLOW_HZ 400000
>
> @@ -163,15 +159,15 @@ static int sdhci_arasan_syscon_write(struct sdhci_host *host,
>
> static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> {
> - u32 div;
> unsigned long freq;
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>
> - div = readl(host->ioaddr + SDHCI_ARASAN_CLK_CTRL_OFFSET);
> - div = (div & CLK_CTRL_TIMEOUT_MASK) >> CLK_CTRL_TIMEOUT_SHIFT;
> + /* SDHCI timeout clock is in kHz */
> + freq = DIV_ROUND_UP(clk_get_rate(pltfm_host->clk), 1000);
>
> - freq = clk_get_rate(pltfm_host->clk);
> - freq /= 1 << (CLK_CTRL_TIMEOUT_MIN_EXP + div);
> + /* or in MHz */
> + if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
> + freq = DIV_ROUND_UP(freq, 1000);
>
> return freq;
> }
> --
> 2.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html