[PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly

[PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly

[Douglas Anderson]

Historically for Rockchip devices we've relied on the power-on
default (or perhaps the firmware setting) to get the correct drive
phase for dw_mmc devices.  This worked OK for the most part, but:

* Relying on the setting just "being right" is a bit fragile.

* As soon as there is an instance where the power on default is wrong or
  where the firmware didn't configure this properly then we'll get a
  mysterious failure.

Let's explicitly set this phase in the kernel.

The comments inside this patch try to explain the situation quite
throughly, but the high level overview of this is:

Before this patch on rk3288 devices tested:
* eMMC: 180 degrees
* SDMMC/SDIO0/SDIO1: 90 degrees

After this patch:
* Use 90 degree phase offset usually.
* Use 180 degree phase offset for MMC_DDR52, SDR104, HS200.

That means we are _changing_ behavior for those devices in this way:

* If we have HS200 eMMC or DDR52 eMMC, we'll run ID mode at 90
  degrees (vs 180) but otherwise have no change.

* For any non-HS200 / non-DDR52 eMMC devices we'll now _always_ run at
  90 degrees (vs 180).  It seems fairly unlikely that building modern
  hardware is using an eMMC that isn't using DDR52 or HS200, of course.

* For SDR104 cards we'll now run with 180 degree phase offset (vs 90).
  It's expected that 90 degree phase offset would have worked OK, but
  this gives us extra margin.

I have tested this by inserting my collection of uSD cards (mostly UHS,
though a few not) into a veyron_minnie and confirmed that they still
seem to enumerate properly.  For a subset of them I tried putting a
filesystem on them and also tried running mmc_test.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v2:
- Now use 90 degrees for some modes; updated comments to say why.

 drivers/mmc/host/dw_mmc-rockchip.c | 64 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 8c20b81cafd8..8068fa887db8 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -66,6 +66,70 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 	/* Make sure we use phases which we can enumerate with */
 	if (!IS_ERR(priv->sample_clk))
 		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
+
+	/*
+	 * Set the drive phase offset based on speed mode to achieve hold times.
+	 *
+	 * That this is _not_ a value that is dynamically tuned and is also
+	 * _not_ a value that will vary from board to board.  It is a value
+	 * that could vary between different SoC models if they had massively
+	 * different output clock delays inside their dw_mmc IP block (delay_o),
+	 * but since it's OK to overshoot a little we don't need to do complex
+	 * calculations and can pick values that will just work for everyone.
+	 *
+	 * When picking values we'll stick with picking 0/90/180/270 since
+	 * those can be made very accurately on all known Rockchip SoCs.
+	 *
+	 * Note that these values match values from the DesignWare Databook
+	 * tables for the most part except for SDR12 and "ID mode".  For those
+	 * two modes the databook calculations assume a clock in of 50MHz.  As
+	 * seen above, we always use a clock in rate that is exactly the
+	 * card's input clock (times RK3288_CLKGEN_DIV, but that gets divided
+	 * back out before the controller sees it).
+	 *
+	 * From measurement of a single device, it appears that delay_o is
+	 * about .5 ns.  Since we try to leave a bit of margin, it's expected
+	 * that numbers here will be fine even with much larger delay_o
+	 * (the 1.4 ns assumed by the DesignWare Databook would result in the
+	 * same results, for instance).
+	 */
+	if (!IS_ERR(priv->drv_clk)) {
+		int phase;
+
+		/*
+		 * In almost all cases a 90 degree phase offset will provide
+		 * sufficient hold times across all valid input clock rates
+		 * assuming delay_o is not absurd for a given SoC.  We'll use
+		 * that as a default.
+		 */
+		phase = 90;
+
+		switch (ios->timing) {
+		case MMC_TIMING_MMC_DDR52:
+			/*
+			 * Since clock in rate with MMC_DDR52 is doubled when
+			 * bus width is 8 we need to double the phase offset
+			 * to get the same timings.
+			 */
+			if (ios->bus_width == MMC_BUS_WIDTH_8)
+				phase = 180;
+			break;
+		case MMC_TIMING_UHS_SDR104:
+		case MMC_TIMING_MMC_HS200:
+			/*
+			 * In the case of 150 MHz clock (typical max for
+			 * Rockchip SoCs), 90 degree offset will add a delay
+			 * of 1.67 ns.  That will meet min hold time of .8 ns
+			 * as long as clock output delay is < .87 ns.  On
+			 * SoCs measured this seems to be OK, but it doesn't
+			 * hurt to give margin here, so we use 180.
+			 */
+			phase = 180;
+			break;
+		}
+
+		clk_set_phase(priv->drv_clk, phase);
+	}
 }
 
 #define NUM_PHASES			360
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly

[Shawn Lin]

On 2016/5/12 5:40, Douglas Anderson wrote:
> Historically for Rockchip devices we've relied on the power-on
> default (or perhaps the firmware setting) to get the correct drive
> phase for dw_mmc devices.  This worked OK for the most part, but:
>
> * Relying on the setting just "being right" is a bit fragile.
>
> * As soon as there is an instance where the power on default is wrong or
>   where the firmware didn't configure this properly then we'll get a
>   mysterious failure.
>
> Let's explicitly set this phase in the kernel.
>
> The comments inside this patch try to explain the situation quite
> throughly, but the high level overview of this is:
>
> Before this patch on rk3288 devices tested:
> * eMMC: 180 degrees
> * SDMMC/SDIO0/SDIO1: 90 degrees
>
> After this patch:
> * Use 90 degree phase offset usually.
> * Use 180 degree phase offset for MMC_DDR52, SDR104, HS200.

Thanks for doing this.

Reviewed-by: Shawn Lin<shawn.lin@rock-chips.com>

>
> That means we are _changing_ behavior for those devices in this way:
>
> * If we have HS200 eMMC or DDR52 eMMC, we'll run ID mode at 90
>   degrees (vs 180) but otherwise have no change.
>
> * For any non-HS200 / non-DDR52 eMMC devices we'll now _always_ run at
>   90 degrees (vs 180).  It seems fairly unlikely that building modern
>   hardware is using an eMMC that isn't using DDR52 or HS200, of course.
>
> * For SDR104 cards we'll now run with 180 degree phase offset (vs 90).
>   It's expected that 90 degree phase offset would have worked OK, but
>   this gives us extra margin.
>
> I have tested this by inserting my collection of uSD cards (mostly UHS,
> though a few not) into a veyron_minnie and confirmed that they still
> seem to enumerate properly.  For a subset of them I tried putting a
> filesystem on them and also tried running mmc_test.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Now use 90 degrees for some modes; updated comments to say why.
>
>  drivers/mmc/host/dw_mmc-rockchip.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 8c20b81cafd8..8068fa887db8 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -66,6 +66,70 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>  	/* Make sure we use phases which we can enumerate with */
>  	if (!IS_ERR(priv->sample_clk))
>  		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
> +
> +	/*
> +	 * Set the drive phase offset based on speed mode to achieve hold times.
> +	 *
> +	 * That this is _not_ a value that is dynamically tuned and is also
> +	 * _not_ a value that will vary from board to board.  It is a value
> +	 * that could vary between different SoC models if they had massively
> +	 * different output clock delays inside their dw_mmc IP block (delay_o),
> +	 * but since it's OK to overshoot a little we don't need to do complex
> +	 * calculations and can pick values that will just work for everyone.
> +	 *
> +	 * When picking values we'll stick with picking 0/90/180/270 since
> +	 * those can be made very accurately on all known Rockchip SoCs.
> +	 *
> +	 * Note that these values match values from the DesignWare Databook
> +	 * tables for the most part except for SDR12 and "ID mode".  For those
> +	 * two modes the databook calculations assume a clock in of 50MHz.  As
> +	 * seen above, we always use a clock in rate that is exactly the
> +	 * card's input clock (times RK3288_CLKGEN_DIV, but that gets divided
> +	 * back out before the controller sees it).
> +	 *
> +	 * From measurement of a single device, it appears that delay_o is
> +	 * about .5 ns.  Since we try to leave a bit of margin, it's expected
> +	 * that numbers here will be fine even with much larger delay_o
> +	 * (the 1.4 ns assumed by the DesignWare Databook would result in the
> +	 * same results, for instance).
> +	 */
> +	if (!IS_ERR(priv->drv_clk)) {
> +		int phase;
> +
> +		/*
> +		 * In almost all cases a 90 degree phase offset will provide
> +		 * sufficient hold times across all valid input clock rates
> +		 * assuming delay_o is not absurd for a given SoC.  We'll use
> +		 * that as a default.
> +		 */
> +		phase = 90;
> +
> +		switch (ios->timing) {
> +		case MMC_TIMING_MMC_DDR52:
> +			/*
> +			 * Since clock in rate with MMC_DDR52 is doubled when
> +			 * bus width is 8 we need to double the phase offset
> +			 * to get the same timings.
> +			 */
> +			if (ios->bus_width == MMC_BUS_WIDTH_8)
> +				phase = 180;
> +			break;
> +		case MMC_TIMING_UHS_SDR104:
> +		case MMC_TIMING_MMC_HS200:
> +			/*
> +			 * In the case of 150 MHz clock (typical max for
> +			 * Rockchip SoCs), 90 degree offset will add a delay
> +			 * of 1.67 ns.  That will meet min hold time of .8 ns
> +			 * as long as clock output delay is < .87 ns.  On
> +			 * SoCs measured this seems to be OK, but it doesn't
> +			 * hurt to give margin here, so we use 180.
> +			 */
> +			phase = 180;
> +			break;
> +		}
> +
> +		clk_set_phase(priv->drv_clk, phase);
> +	}
>  }
>
>  #define NUM_PHASES			360
>


-- 
Best Regards
Shawn Lin

[PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly

[Doug Anderson]

Hi,

On Wed, May 11, 2016 at 2:40 PM, Douglas Anderson <dianders@chromium.org> wrote:
> Historically for Rockchip devices we've relied on the power-on
> default (or perhaps the firmware setting) to get the correct drive
> phase for dw_mmc devices.  This worked OK for the most part, but:
>
> * Relying on the setting just "being right" is a bit fragile.
>
> * As soon as there is an instance where the power on default is wrong or
>   where the firmware didn't configure this properly then we'll get a
>   mysterious failure.
>
> Let's explicitly set this phase in the kernel.
>
> The comments inside this patch try to explain the situation quite
> throughly, but the high level overview of this is:
>
> Before this patch on rk3288 devices tested:
> * eMMC: 180 degrees
> * SDMMC/SDIO0/SDIO1: 90 degrees
>
> After this patch:
> * Use 90 degree phase offset usually.
> * Use 180 degree phase offset for MMC_DDR52, SDR104, HS200.
>
> That means we are _changing_ behavior for those devices in this way:
>
> * If we have HS200 eMMC or DDR52 eMMC, we'll run ID mode at 90
>   degrees (vs 180) but otherwise have no change.
>
> * For any non-HS200 / non-DDR52 eMMC devices we'll now _always_ run at
>   90 degrees (vs 180).  It seems fairly unlikely that building modern
>   hardware is using an eMMC that isn't using DDR52 or HS200, of course.
>
> * For SDR104 cards we'll now run with 180 degree phase offset (vs 90).
>   It's expected that 90 degree phase offset would have worked OK, but
>   this gives us extra margin.
>
> I have tested this by inserting my collection of uSD cards (mostly UHS,
> though a few not) into a veyron_minnie and confirmed that they still
> seem to enumerate properly.  For a subset of them I tried putting a
> filesystem on them and also tried running mmc_test.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Now use 90 degrees for some modes; updated comments to say why.
>
>  drivers/mmc/host/dw_mmc-rockchip.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)

Just a note that I have since found out that recent patches in the
kernel in fact _do_ init the drive phase and apparently do it
improperly.  That means that $SUBJECT patch actually is expected to
fix real problems on real devices.

See <https://patchwork.kernel.org/patch/9085311/> for some details.

-Doug