Received-SPF: pass (google.com: domain of linux-kernel+bounces-56825-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240207170425.478558-1-enachman@marvell.com> <20240207170425.478558-3-enachman@marvell.com>
In-Reply-To: <20240207170425.478558-3-enachman@marvell.com>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Wed, 7 Feb 2024 18:25:03 +0100
Message-ID: <CAPDyKFrkCgVeMUy-2UfPNqCDWzZK6AS1EK4MJeOBiHuBxH1k0Q@mail.gmail.com>
Subject: Re: [PATCH 2/2] mmc: xenon: add timeout for PHY init complete
To: Elad Nachman <enachman@marvell.com>
Cc: huziji@marvell.com, adrian.hunter@intel.com, linux-mmc@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Wed, 7 Feb 2024 at 18:04, Elad Nachman <enachman@marvell.com> wrote:
>
> From: Elad Nachman <enachman@marvell.com>
>
> AC5X spec says PHY init complete bit must be polled until zero.
> We see cases in which timeout can take longer than the standard
> calculation on AC5X, which is expected following the spec comment above.
> According to the spec, we must wait as long as it takes for that bit to
> toggle on AC5X.
> Cap that with 100 delay loops so we won't get stuck forever.
>
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  drivers/mmc/host/sdhci-xenon-phy.c | 31 ++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
> index 4e99197b62c3..f551a9e31772 100644
> --- a/drivers/mmc/host/sdhci-xenon-phy.c
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> @@ -109,6 +109,8 @@
>  #define XENON_EMMC_PHY_LOGIC_TIMING_ADJUST     (XENON_EMMC_PHY_REG_BASE + 0x18)
>  #define XENON_LOGIC_TIMING_VALUE               0x00AA8977
>
> +#define XENON_MAX_PHY_TIMEOUT_LOOPS            100
> +
>  /*
>   * List offset of PHY registers and some special register values
>   * in eMMC PHY 5.0 or eMMC PHY 5.1
> @@ -244,7 +246,7 @@ static int xenon_check_stability_internal_clk(struct sdhci_host *host)
>   */
>  static int xenon_emmc_phy_init(struct sdhci_host *host)
>  {
> -       u32 reg;
> +       u32 reg, retry;
>         u32 wait, clock;
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> @@ -282,14 +284,31 @@ static int xenon_emmc_phy_init(struct sdhci_host *host)
>         /* get the wait time */
>         wait /= clock;
>         wait++;
> -       /* wait for host eMMC PHY init completes */
> -       udelay(wait);
>
> -       reg = sdhci_readl(host, phy_regs->timing_adj);
> -       reg &= XENON_PHY_INITIALIZAION;
> +       /*
> +        * AC5X spec says bit must be polled until zero.
> +        * We see cases in which timeout can take longer
> +        * than the standard calculation on AC5X, which is
> +        * expected following the spec comment above.
> +        * According to the spec, we must wait as long as
> +        * it takes for that bit to toggle on AC5X.
> +        * Cap that with 100 delay loops so we won't get
> +        * stuck here forever:
> +        */
> +
> +       retry = XENON_MAX_PHY_TIMEOUT_LOOPS;
> +
> +       do {
> +               /* wait for host eMMC PHY init completes */
> +               udelay(wait);
> +
> +               reg = sdhci_readl(host, phy_regs->timing_adj);
> +               reg &= XENON_PHY_INITIALIZAION;
> +       } while (reg && retry--);
> +
>         if (reg) {
>                 dev_err(mmc_dev(host->mmc), "eMMC PHY init cannot complete after %d us\n",
> -                       wait);
> +                       wait * XENON_MAX_PHY_TIMEOUT_LOOPS);
>                 return -ETIMEDOUT;
>         }

Similar comment as for patch1, please convert to readx_poll_timeout()
or similar.

Kind regards
Uffe