All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: jh80.chung@samsung.com, Ulf Hansson <ulf.hansson@linaro.org>,
	Heiko Stuebner <heiko@sntech.de>,
	shawn.lin@rock-chips.com
Cc: linux-rockchip@lists.infradead.org, briannorris@chromium.org,
	Douglas Anderson <dianders@chromium.org>,
	linux-mmc@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: [PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly
Date: Wed, 11 May 2016 14:40:48 -0700	[thread overview]
Message-ID: <1463002848-580-1-git-send-email-dianders@chromium.org> (raw)

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

WARNING: multiple messages have this Message-ID (diff)
From: dianders@chromium.org (Douglas Anderson)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly
Date: Wed, 11 May 2016 14:40:48 -0700	[thread overview]
Message-ID: <1463002848-580-1-git-send-email-dianders@chromium.org> (raw)

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

             reply	other threads:[~2016-05-11 21:41 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-11 21:40 Douglas Anderson [this message]
2016-05-11 21:40 ` [PATCH v2] mmc: dw_mmc: rockchip: Set the drive phase properly Douglas Anderson
2016-05-12  3:13 ` Shawn Lin
2016-05-12  3:13   ` Shawn Lin
2016-05-12 18:15 ` Doug Anderson
2016-05-12 18:15   ` Doug Anderson
2016-05-12 18:15   ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1463002848-580-1-git-send-email-dianders@chromium.org \
    --to=dianders@chromium.org \
    --cc=briannorris@chromium.org \
    --cc=heiko@sntech.de \
    --cc=jh80.chung@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=shawn.lin@rock-chips.com \
    --cc=ulf.hansson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.