[PATCH 0/2] Fix PHY init timeout issues

[PATCH 0/2] Fix PHY init timeout issues

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Fix PHY init timeout issues:

1. Clock Stability issue causing PHY timeout

2. Timeout taking longer than needed on AC5X.
   Solve by constantly testing the PHY init bit
   until it toggles, but up to 100X timeout factor.

Elad Nachman (2):
  mmc: xenon: fix PHY init clock stability
  mmc: xenon: add timeout for PHY init complete

 drivers/mmc/host/sdhci-xenon-phy.c | 54 ++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] mmc: xenon: fix PHY init clock stability

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Each time SD/mmc phy is initialized, at times, in some of
the attempts, phy fails to completes its initialization
which results into timeout error. Per the HW spec, it is
a pre-requisite to ensure a stable SD clock before a phy
initialization is attempted.

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/mmc/host/sdhci-xenon-phy.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
index 8cf3a375de65..4e99197b62c3 100644
--- a/drivers/mmc/host/sdhci-xenon-phy.c
+++ b/drivers/mmc/host/sdhci-xenon-phy.c
@@ -216,6 +216,24 @@ static int xenon_alloc_emmc_phy(struct sdhci_host *host)
 	return 0;
 }
 
+static int xenon_check_stability_internal_clk(struct sdhci_host *host)
+{
+	u32 reg;
+	ktime_t timeout;
+
+	/* Wait max 20 ms */
+	timeout = ktime_add_ms(ktime_get(), 20);
+	while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
+		& SDHCI_CLOCK_INT_STABLE)) {
+		if (ktime_after(ktime_get(), timeout)) {
+			dev_err(mmc_dev(host->mmc), "phy_init: Internal clock never stabilized.\n");
+			return -ETIMEDOUT;
+		}
+		usleep_range(900, 1100);
+	}
+	return 0;
+}
+
 /*
  * eMMC 5.0/5.1 PHY init/re-init.
  * eMMC PHY init should be executed after:
@@ -232,6 +250,11 @@ static int xenon_emmc_phy_init(struct sdhci_host *host)
 	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
 	struct xenon_emmc_phy_regs *phy_regs = priv->emmc_phy_regs;
 
+	int ret = xenon_check_stability_internal_clk(host);
+
+	if (ret)
+		return ret;
+
 	reg = sdhci_readl(host, phy_regs->timing_adj);
 	reg |= XENON_PHY_INITIALIZAION;
 	sdhci_writel(host, reg, phy_regs->timing_adj);
-- 
2.25.1

[PATCH 2/2] mmc: xenon: add timeout for PHY init complete

[Elad Nachman]

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;
 	}
 
-- 
2.25.1

Re: [PATCH 1/2] mmc: xenon: fix PHY init clock stability

[Ulf Hansson]

On Wed, 7 Feb 2024 at 18:04, Elad Nachman <enachman@marvell.com> wrote:
>
> From: Elad Nachman <enachman@marvell.com>
>
> Each time SD/mmc phy is initialized, at times, in some of
> the attempts, phy fails to completes its initialization
> which results into timeout error. Per the HW spec, it is
> a pre-requisite to ensure a stable SD clock before a phy
> initialization is attempted.
>
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  drivers/mmc/host/sdhci-xenon-phy.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
> index 8cf3a375de65..4e99197b62c3 100644
> --- a/drivers/mmc/host/sdhci-xenon-phy.c
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> @@ -216,6 +216,24 @@ static int xenon_alloc_emmc_phy(struct sdhci_host *host)
>         return 0;
>  }
>
> +static int xenon_check_stability_internal_clk(struct sdhci_host *host)
> +{
> +       u32 reg;
> +       ktime_t timeout;
> +
> +       /* Wait max 20 ms */
> +       timeout = ktime_add_ms(ktime_get(), 20);
> +       while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
> +               & SDHCI_CLOCK_INT_STABLE)) {
> +               if (ktime_after(ktime_get(), timeout)) {
> +                       dev_err(mmc_dev(host->mmc), "phy_init: Internal clock never stabilized.\n");
> +                       return -ETIMEDOUT;
> +               }
> +               usleep_range(900, 1100);
> +       }
> +       return 0;

Please convert the above into readx_poll_timeout() or similar.

> +}
> +
>  /*
>   * eMMC 5.0/5.1 PHY init/re-init.
>   * eMMC PHY init should be executed after:
> @@ -232,6 +250,11 @@ static int xenon_emmc_phy_init(struct sdhci_host *host)
>         struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>         struct xenon_emmc_phy_regs *phy_regs = priv->emmc_phy_regs;
>
> +       int ret = xenon_check_stability_internal_clk(host);
> +
> +       if (ret)
> +               return ret;
> +
>         reg = sdhci_readl(host, phy_regs->timing_adj);
>         reg |= XENON_PHY_INITIALIZAION;
>         sdhci_writel(host, reg, phy_regs->timing_adj);

Kind regards
Uffe

Re: [PATCH 2/2] mmc: xenon: add timeout for PHY init complete

[Ulf Hansson]

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