Re: [PATCH] mmc: sdhci-of-arasan: fix incorrect timeout clock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]