[PATCH 0/8] mmc: dw_mmc: allow tuning using the clk-phase api

[PATCH 0/8] mmc: dw_mmc: allow tuning using the clk-phase api

[Heiko Stuebner]

This series resurrects and adapts some individual patches whose sum
enable the dw_mmc hosts on Rockchip socs to tune clock phases using
the generic phase api (rk3288 and following have this capability).

The changes to the original mmc-phase clocks are expanded by further
findings resulting from devices being used in the field.

Similarly the regulator handling changes do use the brand new
regulator_set_voltage_triplet call to allow specifying lower and upper
limits. One possible point of discussion are the two voltage ranges
that are tried for the 3.3V signal level. Trying to stay near vmmc
at first and only then opening the range to the full 2.7-3.6V.

This mainly circumvents a shortcoming of the regulator voltage
setting, in that even with regulator_set_voltage_triplet the regulator
framework will take the lowest possible voltage when the possible
voltages are below the target voltage. While it may be ideal to solve
this on the regulator side, I'm not seeing this appearing in the short
term, mainly because all regulator parts (including regulator drivers)
are keyed to selecting the lowest voltage from the range, while on the
mmc side we know which voltages may work and trying this in two steps
does not create to much overhead, as unsupported voltages are already
filtered out by the regulator_is_voltage_selected calls.


Alexandru M Stan (3):
  mmc: dw_mmc: dt-binding: Add tuning related things
  mmc: dw_mmc: Generic MMC tuning with the clock phase framework
  ARM: dts: rockchip: Add drive/sample clocks for rk3288 dw_mmc devices

Douglas Anderson (4):
  clk: rockchip: Allow more precision for some mmc clock phases
  clk: rockchip: Make calculations use rounding
  mmc: core: Add mmc_regulator_set_vqmmc()
  mmc: dw_mmc: Use mmc_regulator_set_vqmmc in
    start_signal_voltage_switch

Heiko Stuebner (1):
  ARM: dts: rockchip: add tuning related settings to veyron devices

 .../devicetree/bindings/mmc/synopsys-dw-mshc.txt   |  14 +-
 arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi         |   7 +-
 arch/arm/boot/dts/rk3288-veyron.dtsi               |   6 +
 arch/arm/boot/dts/rk3288.dtsi                      |  20 +--
 drivers/clk/rockchip/clk-mmc-phase.c               |  54 ++++---
 drivers/mmc/core/core.c                            |  68 +++++++++
 drivers/mmc/host/dw_mmc.c                          | 159 +++++++++++++++++++--
 include/linux/mmc/dw_mmc.h                         |   3 +
 include/linux/mmc/host.h                           |   7 +
 9 files changed, 295 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH 1/8] clk: rockchip: Allow more precision for some mmc clock phases

[Heiko Stuebner]

From: Douglas Anderson <dianders@chromium.org>

Because of the inexact nature of the extra MMC delay elements (it's
not possible to keep the phase monotonic and to also make phases (mod
90) > 70), we previously only allowed phases (mod 90) of 22.5, 45,
and 67.5.

But it's not the end of the world if the MMC clock phase goes
non-monotonic.  At most we'll be 25 degrees off.  It's way better to
test more phases to look for bad ones than to be 25 degrees off, because
in the case of MMC really the point is to find bad phases and get as far
asway from the as possible.  If we get to test extra phases by going
slightly non-monotonic then that might be fine.  Worst case we would
end up at a phases that's slight differnt than the one we wanted, but
at least we'd still be quite far away from the a bad phase.

Signed-off-by: Douglas Anderson <dianders@chromium.org>

Fold in more precise variance-values of 44-77 instead of 40-80.
Fold in the actual removal of the monotonic requirement and adapt
patch message accordingly.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-mmc-phase.c | 45 ++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index 9b61342..a797d86 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -45,8 +45,8 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
 #define PSECS_PER_SEC 1000000000000LL
 
 /*
- * Each fine delay is between 40ps-80ps. Assume each fine delay is 60ps to
- * simplify calculations. So 45degs could be anywhere between 33deg and 66deg.
+ * Each fine delay is between 44ps-77ps. Assume each fine delay is 60ps to
+ * simplify calculations. So 45degs could be anywhere between 33deg and 57.8deg.
  */
 #define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC 60
 
@@ -84,22 +84,37 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)
 	u32 raw_value;
 	u64 delay;
 
-	/* allow 22 to be 22.5 */
-	degrees++;
-	/* floor to 22.5 increment */
-	degrees -= ((degrees) * 10 % 225) / 10;
-
 	nineties = degrees / 90;
-	/* 22.5 multiples */
-	remainder = (degrees % 90) / 22;
-
+	remainder = (degrees % 90);
+
+	/*
+	 * Due to the inexact nature of the "fine" delay, we might
+	 * actually go non-monotonic.  We don't go _too_ monotonic
+	 * though, so we should be OK.  Here are options of how we may
+	 * work:
+	 *
+	 * Ideally we end up with:
+	 *   1.0, 2.0, ..., 69.0, 70.0, ...,  89.0, 90.0
+	 *
+	 * On one extreme (if delay is actually 44ps):
+	 *   .73, 1.5, ..., 50.6, 51.3, ...,  65.3, 90.0
+	 * The other (if delay is actually 77ps):
+	 *   1.3, 2.6, ..., 88.6. 89.8, ..., 114.0, 90
+	 *
+	 * It's possible we might make a delay that is up to 25
+	 * degrees off from what we think we're making.  That's OK
+	 * though because we should be REALLY far from any bad range.
+	 */
+
+	/*
+	 * Convert to delay; do a little extra work to make sure we
+	 * don't overflow 32-bit / 64-bit numbers.
+	 */
 	delay = PSECS_PER_SEC;
-	do_div(delay, rate);
-	/* / 360 / 22.5 */
-	do_div(delay, 16);
-	do_div(delay, ROCKCHIP_MMC_DELAY_ELEMENT_PSEC);
-
 	delay *= remainder;
+	do_div(delay, 10000);
+	do_div(delay, (rate / 1000) * 36 * ROCKCHIP_MMC_DELAY_ELEMENT_PSEC);
+
 	delay_num = (u8) min(delay, 255ULL);
 
 	raw_value = delay_num ? ROCKCHIP_MMC_DELAY_SEL : 0;
-- 
2.1.4

[PATCH 2/8] clk: rockchip: Make calculations use rounding

[Heiko Stuebner]

From: Douglas Anderson <dianders@chromium.org>

Let's use DIV_ROUND_CLOSEST for rounding, not just truncating
division.  This lets us get closer to the right rate.

Before this:
  set_phase(86) delay_nums=26 reg[0xf000420c]=0x468 actual_degrees=83
  set_phase(89) delay_nums=27 reg[0xf000420c]=0x46c actual_degrees=86

After this:
  set_phase(86) delay_nums=27 reg[0xf000420c]=0x46c actual_degrees=86
  set_phase(89) delay_nums=28 reg[0xf000420c]=0x470 actual_degrees=90

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-mmc-phase.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index a797d86..bc24e5a 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -69,7 +69,7 @@ static int rockchip_mmc_get_phase(struct clk_hw *hw)
 
 		delay_num = (raw_value & ROCKCHIP_MMC_DELAYNUM_MASK);
 		delay_num >>= ROCKCHIP_MMC_DELAYNUM_OFFSET;
-		degrees += delay_num * factor / 10000;
+		degrees += DIV_ROUND_CLOSEST(delay_num * factor, 10000);
 	}
 
 	return degrees % 360;
@@ -82,7 +82,7 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)
 	u8 nineties, remainder;
 	u8 delay_num;
 	u32 raw_value;
-	u64 delay;
+	u32 delay;
 
 	nineties = degrees / 90;
 	remainder = (degrees % 90);
@@ -110,12 +110,13 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)
 	 * Convert to delay; do a little extra work to make sure we
 	 * don't overflow 32-bit / 64-bit numbers.
 	 */
-	delay = PSECS_PER_SEC;
+	delay = 10000000; /* PSECS_PER_SEC / 10000 / 10 */
 	delay *= remainder;
-	do_div(delay, 10000);
-	do_div(delay, (rate / 1000) * 36 * ROCKCHIP_MMC_DELAY_ELEMENT_PSEC);
+	delay = DIV_ROUND_CLOSEST(delay,
+			(rate / 1000) * 36 *
+				(ROCKCHIP_MMC_DELAY_ELEMENT_PSEC / 10));
 
-	delay_num = (u8) min(delay, 255ULL);
+	delay_num = (u8) min_t(u32, delay, 255);
 
 	raw_value = delay_num ? ROCKCHIP_MMC_DELAY_SEL : 0;
 	raw_value |= delay_num << ROCKCHIP_MMC_DELAYNUM_OFFSET;
-- 
2.1.4

[PATCH 3/8] mmc: core: Add mmc_regulator_set_vqmmc()

[Heiko Stuebner]

From: Douglas Anderson <dianders@chromium.org>

This adds logic to the MMC core to set VQMMC.  This is expected to be
called by MMC drivers like dw_mmc as part of (or instead of) their
start_signal_voltage_switch() callback.

A few notes:

* When setting the signal voltage to 3.3V we do our best to make VQMMC
  and VMMC match.  It's been reported that this makes some old cards
  happy since they were tested back in the day before UHS when VQMMC
  and VMMC were provided by the same regulator.  A nice side effect of
  this is that we don't end up on the hairy edge of VQMMC (2.7V),
  which some EEs claim is a little too close to the minimum for
  comfort.
  If this is not supported by the supplying regulator we try to find
  a suitable voltage within the whole 2.7V-3.6V area of the spec.

* When setting the signal voltage to 1.8V or 1.2V we aim for that
  specific voltage instead of picking the lowest one in the range.

* We very purposely don't print errors in mmc_regulator_set_vqmmc().
  There are cases where the MMC core will try several different
  voltages and we don't want to pollute the logs.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/mmc/core/core.c  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |  7 +++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0520064..9dc0b65 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1437,6 +1437,74 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
 }
 EXPORT_SYMBOL_GPL(mmc_regulator_set_ocr);
 
+static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
+						  int min_uV, int target_uV,
+						  int max_uV)
+{
+	/*
+	 * Check if supported first to avoid errors since we may try several
+	 * signal levels during power up and don't want to show errors.
+	 */
+	if (!regulator_is_supported_voltage(regulator, min_uV, max_uV))
+		return -EINVAL;
+
+	return regulator_set_voltage_triplet(regulator, min_uV, target_uV,
+					     max_uV);
+}
+
+/**
+ * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
+ *
+ * For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible.
+ * That will match the behavior of old boards where VQMMC and VMMC were supplied
+ * by the same supply.  The Bus Operating conditions for 3.3V signaling in the
+ * SD card spec also define VQMMC in terms of VMMC.
+ * If this is not possible we'll try the full 2.7-3.6V of the spec.
+ *
+ * For 1.2V and 1.8V signaling we'll try to get as close as possible to the
+ * requested voltage.  This is definitely a good idea for UHS where there's a
+ * separate regulator on the card that's trying to make 1.8V and it's best if
+ * we match.
+ *
+ * This function is expected to be used by a controller's
+ * start_signal_voltage_switch() function.
+ */
+int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	int volt, min_uV, max_uV;
+
+	/* If no vqmmc supply then we can't change the voltage */
+	if (IS_ERR(mmc->supply.vqmmc))
+		return -EINVAL;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_120:
+		return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+						1100000, 1200000, 1300000);
+	case MMC_SIGNAL_VOLTAGE_180:
+		return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+						1700000, 1800000, 1950000);
+	case MMC_SIGNAL_VOLTAGE_330:
+		volt = regulator_get_voltage(mmc->supply.vmmc);
+		if (volt < 0)
+			return volt;
+
+		min_uV = max(volt - 300000, 2700000);
+		max_uV = min(volt + 300000, 3600000);
+
+		/* try to stay close to vmmc@first */
+		if (!mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+						min_uV, volt, max_uV))
+			return 0;
+
+		return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+						2700000, volt, 3600000);
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(mmc_regulator_set_vqmmc);
+
 #endif /* CONFIG_REGULATOR */
 
 int mmc_regulator_get_supply(struct mmc_host *mmc)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 83b81fd..a2a78eb 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -423,6 +423,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);
 int mmc_regulator_set_ocr(struct mmc_host *mmc,
 			struct regulator *supply,
 			unsigned short vdd_bit);
+int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios);
 #else
 static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
 {
@@ -435,6 +436,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
 {
 	return 0;
 }
+
+static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc,
+					  struct mmc_ios *ios)
+{
+	return -EINVAL;
+}
 #endif
 
 int mmc_regulator_get_supply(struct mmc_host *mmc);
-- 
2.1.4

[PATCH 4/8] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch

[Heiko Stuebner]

From: Douglas Anderson <dianders@chromium.org>

We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
helper has some advantages:

1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
   VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
   compatibility.

2. We get rid of a few more warnings when probing unsupported
   voltages.

3. We get rid of some non-dw_mmc specific code in dw_mmc.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/mmc/host/dw_mmc.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index fcbf552..b1b7e7f 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1279,7 +1279,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	const struct dw_mci_drv_data *drv_data = host->drv_data;
 	u32 uhs;
 	u32 v18 = SDMMC_UHS_18V << slot->id;
-	int min_uv, max_uv;
 	int ret;
 
 	if (drv_data && drv_data->switch_voltage)
@@ -1291,22 +1290,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	 * does no harm but you need to set the regulator directly.  Try both.
 	 */
 	uhs = mci_readl(host, UHS_REG);
-	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
-		min_uv = 2700000;
-		max_uv = 3600000;
+	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
 		uhs &= ~v18;
-	} else {
-		min_uv = 1700000;
-		max_uv = 1950000;
+	else
 		uhs |= v18;
-	}
+
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
+		ret = mmc_regulator_set_vqmmc(mmc, ios);
 
 		if (ret) {
 			dev_dbg(&mmc->class_dev,
-					 "Regulator set error %d: %d - %d\n",
-					 ret, min_uv, max_uv);
+					 "Regulator set error %d - %s V\n",
+					 ret, uhs & v18 ? "1.8" : "3.3");
 			return ret;
 		}
 	}
-- 
2.1.4

[PATCH 5/8] mmc: dw_mmc: dt-binding: Add tuning related things

[Heiko Stuebner]

From: Alexandru M Stan <amstan@chromium.org>

Add ciu_drv, ciu_sample clocks and default-sample-phase. This will later
be used by tuning code.

We do not touch ciu_drive (and by extension define default-drive-phase).
Drive phase is mostly used to define minimum hold times, while one could
write some code to determine what phase meets the minimum hold time
(ex 10 degrees) this will not work with the current clock phase framework
(which floors angles, so we'll get 0 deg, and there's no way to know what
resolution the floors happen at). We assume that the default drive angles
set by the hardware are good enough.

Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
index 346c609..5edadc2 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
@@ -42,11 +42,13 @@ Optional properties:
 * clocks: from common clock binding: handle to biu and ciu clocks for the
   bus interface unit clock and the card interface unit clock.
 
-* clock-names: from common clock binding: Shall be "biu" and "ciu".
-  If the biu clock is missing we'll simply skip enabling it.  If the
-  ciu clock is missing we'll just assume that the clock is running at
+* clock-names: from common clock binding: Shall be "biu", "ciu", "ciu_drv" and
+  "ciu_sample".  If the biu clock is missing we'll simply skip enabling it.
+  If the ciu clock is missing we'll just assume that the clock is running at
   clock-frequency.  It is an error to omit both the ciu clock and the
-  clock-frequency.
+  clock-frequency.  "ciu_drv" and "ciu_sample" are used to control the clock
+  phases, "ciu_sample" is required for tuning high speed modes (if no other
+  custom tuning method is defined).
 
 * clock-frequency: should be the frequency (in Hz) of the ciu clock.  If this
   is specified and the ciu clock is specified then we'll try to set the ciu
@@ -75,6 +77,10 @@ Optional properties:
 * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is
   specified we'll defer probe until we can find this regulator.
 
+* default-sample-phase: The default phase to set ciu_sample at probing, low
+  speeds or in case where all phases work at tuning time. If not specified
+  0 deg will be used.
+
 Aliases:
 
 - All the MSHC controller nodes should be represented in the aliases node using
-- 
2.1.4

[PATCH 6/8] mmc: dw_mmc: Generic MMC tuning with the clock phase framework

[Heiko Stuebner]

From: Alexandru M Stan <amstan@chromium.org>

This algorithm will try 1 degree increments, since there's no way to tell
what resolution the underlying phase code uses. As an added bonus, doing
many tunings yields better results since some tests are run more than once
(ex: if the underlying driver uses 45 degree increments, the tuning code
will try the same angle more than once).

It will then construct a list of good phase ranges (even ranges that cross
360/0), will pick the biggest range then it will set the sample_clk to the
middle of that range.

We do not touch ciu_drive (and by extension define default-drive-phase).
Drive phase is mostly used to define minimum hold times, while one could
write some code to determine what phase meets the minimum hold time (ex 10
degrees) this will not work with the current clock phase framework (which
floors angles, so we'll get 0 deg, and there's no way to know what
resolution the floors happen at). We assume that the default drive angles
set by the hardware are good enough.

If a device has device specific code (like exynos) then that will still
take precedence, otherwise this new code will execute. If the device wants
to tune, but has no sample_clk defined we'll return EIO with an error
message.

Signed-off-by: Alexandru M Stan <amstan@chromium.org>

Convert to mmc_send_tuning()
Fold in from the ChromeOS-tree:
 - mmc: dw_mmc: Change tuning to only 16 phases
 - mmc: dw_mmc: Test more phases
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/mmc/host/dw_mmc.c  | 142 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/dw_mmc.h |   3 +
 2 files changed, 145 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index b1b7e7f..13bcde0 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1194,6 +1194,12 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (drv_data && drv_data->set_ios)
 		drv_data->set_ios(slot->host, ios);
 
+	/* Make sure we use phases which we can enumerate with */
+	if (!IS_ERR(slot->host->sample_clk)) {
+		clk_set_phase(slot->host->sample_clk,
+			      slot->host->default_sample_phase);
+	}
+
 	switch (ios->power_mode) {
 	case MMC_POWER_UP:
 		if (!IS_ERR(mmc->supply.vmmc)) {
@@ -1414,6 +1420,127 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	spin_unlock_irqrestore(&host->irq_lock, irqflags);
 }
 
+#define NUM_PHASES			360
+#define TUNING_ITERATION_TO_PHASE(i)	(DIV_ROUND_UP((i) * 360, NUM_PHASES))
+
+static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot)
+{
+	struct dw_mci *host = slot->host;
+	struct mmc_host *mmc = slot->mmc;
+	int ret = 0;
+	int i;
+	bool v, prev_v = 0, first_v;
+	struct range_t {
+		int start;
+		int end; /* inclusive */
+	};
+	struct range_t *ranges;
+	unsigned int range_count = 0;
+	int longest_range_len = -1;
+	int longest_range = -1;
+	int middle_phase;
+
+	if (IS_ERR(host->sample_clk)) {
+		dev_err(host->dev, "Tuning clock (sample_clk) not defined.\n");
+		return -EIO;
+	}
+
+	ranges = kmalloc_array(NUM_PHASES / 2 + 1, sizeof(*ranges), GFP_KERNEL);
+	if (!ranges)
+		return -ENOMEM;
+
+	/* Try each phase and extract good ranges */
+	for (i = 0; i < NUM_PHASES; ) {
+		clk_set_phase(host->sample_clk, TUNING_ITERATION_TO_PHASE(i));
+
+		v = !mmc_send_tuning(mmc);
+
+		if (i == 0)
+			first_v = v;
+
+		if ((!prev_v) && v) {
+			range_count++;
+			ranges[range_count-1].start = i;
+		}
+		if (v) {
+			ranges[range_count-1].end = i;
+			i++;
+		} else if (i == NUM_PHASES - 1) {
+			/* No extra skipping rules if we're at the end */
+			i++;
+		} else {
+			/*
+			 * No need to check too close to an invalid
+			 * one since testing bad phases is slow.  Skip
+			 * 20 degrees.
+			 */
+			i += DIV_ROUND_UP(20 * NUM_PHASES, 360);
+
+			/* Always test the last one */
+			if (i >= NUM_PHASES)
+				i = NUM_PHASES - 1;
+		}
+
+		prev_v = v;
+	}
+
+	if (range_count == 0) {
+		dev_warn(host->dev, "All phases bad!");
+		ret = -EIO;
+		goto free;
+	}
+
+	/* wrap around case, merge the end points */
+	if ((range_count > 1) && first_v && v) {
+		ranges[0].start = ranges[range_count-1].start;
+		range_count--;
+	}
+
+	if (ranges[0].start == 0 && ranges[0].end == NUM_PHASES - 1) {
+		clk_set_phase(host->sample_clk, host->default_sample_phase);
+		dev_info(host->dev, "All phases work, using default phase %d.",
+			 host->default_sample_phase);
+		goto free;
+	}
+
+	/* Find the longest range */
+	for (i = 0; i < range_count; i++) {
+		int len = (ranges[i].end - ranges[i].start + 1);
+
+		if (len < 0)
+			len += NUM_PHASES;
+
+		if (longest_range_len < len) {
+			longest_range_len = len;
+			longest_range = i;
+		}
+
+		dev_dbg(host->dev, "Good phase range %d-%d (%d len)\n",
+			TUNING_ITERATION_TO_PHASE(ranges[i].start),
+			TUNING_ITERATION_TO_PHASE(ranges[i].end),
+			len
+		);
+	}
+
+	dev_dbg(host->dev, "Best phase range %d-%d (%d len)\n",
+		TUNING_ITERATION_TO_PHASE(ranges[longest_range].start),
+		TUNING_ITERATION_TO_PHASE(ranges[longest_range].end),
+		longest_range_len
+	);
+
+	middle_phase = ranges[longest_range].start + longest_range_len / 2;
+	middle_phase %= NUM_PHASES;
+	dev_info(host->dev, "Successfully tuned phase to %d\n",
+		 TUNING_ITERATION_TO_PHASE(middle_phase));
+
+	clk_set_phase(host->sample_clk,
+		      TUNING_ITERATION_TO_PHASE(middle_phase));
+
+free:
+	kfree(ranges);
+	return ret;
+}
+
 static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct dw_mci_slot *slot = mmc_priv(mmc);
@@ -1423,6 +1550,8 @@ static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 	if (drv_data && drv_data->execute_tuning)
 		err = drv_data->execute_tuning(slot);
+	else
+		err = dw_mci_execute_generic_tuning(slot);
 	return err;
 }
 
@@ -2741,6 +2870,11 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
 		pdata->bus_hz = clock_frequency;
 
+	if (of_property_read_u32(np, "default-sample-phase",
+					&host->default_sample_phase)) {
+		host->default_sample_phase = 0;
+	}
+
 	if (drv_data && drv_data->parse_dt) {
 		ret = drv_data->parse_dt(host);
 		if (ret)
@@ -2843,6 +2977,14 @@ int dw_mci_probe(struct dw_mci *host)
 		host->bus_hz = clk_get_rate(host->ciu_clk);
 	}
 
+	host->drv_clk = devm_clk_get(host->dev, "ciu_drv");
+	if (IS_ERR(host->drv_clk))
+		dev_dbg(host->dev, "ciu_drv not available\n");
+
+	host->sample_clk = devm_clk_get(host->dev, "ciu_sample");
+	if (IS_ERR(host->sample_clk))
+		dev_dbg(host->dev, "ciu_sample not available\n");
+
 	if (!host->bus_hz) {
 		dev_err(host->dev,
 			"Platform data must supply bus speed\n");
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 134c574..40187ba 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -176,7 +176,10 @@ struct dw_mci {
 	void			*priv;
 	struct clk		*biu_clk;
 	struct clk		*ciu_clk;
+	struct clk		*drv_clk;
+	struct clk		*sample_clk;
 	struct dw_mci_slot	*slot[MAX_MCI_SLOTS];
+	int			default_sample_phase;
 
 	/* FIFO push and pull */
 	int			fifo_depth;
-- 
2.1.4

[PATCH 7/8] ARM: dts: rockchip: Add drive/sample clocks for rk3288 dw_mmc devices

[Heiko Stuebner]

From: Alexandru M Stan <amstan@chromium.org>

The drive/sample clocks can be phase shifted.  The drive clock
could be used in a future patch to adjust hold times.  The sample
clock is used for tuning.

Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3288.dtsi | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 118fe74..b084bc6 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -222,8 +222,9 @@
 	sdmmc: dwmmc at ff0c0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		clock-freq-min-max = <400000 150000000>;
-		clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>;
-		clock-names = "biu", "ciu";
+		clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>,
+			 <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>;
+		clock-names = "biu", "ciu", "ciu_drv", "ciu_sample";
 		fifo-depth = <0x100>;
 		interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
 		reg = <0xff0c0000 0x4000>;
@@ -233,8 +234,9 @@
 	sdio0: dwmmc at ff0d0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		clock-freq-min-max = <400000 150000000>;
-		clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>;
-		clock-names = "biu", "ciu";
+		clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>,
+			 <&cru SCLK_SDIO0_DRV>, <&cru SCLK_SDIO0_SAMPLE>;
+		clock-names = "biu", "ciu", "ciu_drv", "ciu_sample";
 		fifo-depth = <0x100>;
 		interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
 		reg = <0xff0d0000 0x4000>;
@@ -244,8 +246,9 @@
 	sdio1: dwmmc at ff0e0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		clock-freq-min-max = <400000 150000000>;
-		clocks = <&cru HCLK_SDIO1>, <&cru SCLK_SDIO1>;
-		clock-names = "biu", "ciu";
+		clocks = <&cru HCLK_SDIO1>, <&cru SCLK_SDIO1>,
+			 <&cru SCLK_SDIO1_DRV>, <&cru SCLK_SDIO1_SAMPLE>;
+		clock-names = "biu", "ciu", "ciu_drv", "ciu_sample";
 		fifo-depth = <0x100>;
 		interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
 		reg = <0xff0e0000 0x4000>;
@@ -255,8 +258,9 @@
 	emmc: dwmmc at ff0f0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		clock-freq-min-max = <400000 150000000>;
-		clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>;
-		clock-names = "biu", "ciu";
+		clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
+			 <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
+		clock-names = "biu", "ciu", "ciu_drv", "ciu_sample";
 		fifo-depth = <0x100>;
 		interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
 		reg = <0xff0f0000 0x4000>;
-- 
2.1.4

[PATCH 8/8] ARM: dts: rockchip: add tuning related settings to veyron devices

[Heiko Stuebner]

This allows the tuning code to run and use higher speeds on capable cards.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi | 7 ++++++-
 arch/arm/boot/dts/rk3288-veyron.dtsi       | 6 ++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi b/arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi
index b5334ec..24a7a11 100644
--- a/arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi
@@ -90,7 +90,7 @@
 	regulators {
 		vccio_sd: LDO_REG4 {
 			regulator-name = "vccio_sd";
-			regulator-min-microvolt = <3300000>;
+			regulator-min-microvolt = <1800000>;
 			regulator-max-microvolt = <3300000>;
 			regulator-state-mem {
 				regulator-off-in-suspend;
@@ -116,7 +116,12 @@
 	cap-sd-highspeed;
 	card-detect-delay = <200>;
 	cd-gpios = <&gpio7 5 GPIO_ACTIVE_LOW>;
+	default-sample-phase = <90>;
 	num-slots = <1>;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
+	sd-uhs-sdr104;
 	vmmc-supply = <&vcc33_sd>;
 	vqmmc-supply = <&vccio_sd>;
 };
diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index df7d70d..394aa80 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -194,7 +194,9 @@
 	broken-cd;
 	bus-width = <8>;
 	cap-mmc-highspeed;
+	default-sample-phase = <158>;
 	disable-wp;
+	mmc-hs200-1_8v;
 	mmc-pwrseq = <&emmc_pwrseq>;
 	non-removable;
 	num-slots = <1>;
@@ -409,6 +411,10 @@
 	num-slots = <1>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdio0_clk &sdio0_cmd &sdio0_bus4>;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
+	sd-uhs-sdr104;
 	vmmc-supply = <&vcc33_sys>;
 	vqmmc-supply = <&vcc18_wl>;
 };
-- 
2.1.4

[PATCH 5/8] mmc: dw_mmc: dt-binding: Add tuning related things

[Jaehoon Chung]

Hi, Heiko.

On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
> From: Alexandru M Stan <amstan@chromium.org>
> 
> Add ciu_drv, ciu_sample clocks and default-sample-phase. This will later
> be used by tuning code.

As i know, ciu_drv and ciu_sample clocks are generated with "ciu" clock.
But in these patch-set, ciu_drv and ciu_sample are controlled by clock framework.
It's a little strange.
Are there ciu_drv and ciu_sample clock on Rockchip?

Best Regards,
Jaehoon Chung

> 
> We do not touch ciu_drive (and by extension define default-drive-phase).
> Drive phase is mostly used to define minimum hold times, while one could
> write some code to determine what phase meets the minimum hold time
> (ex 10 degrees) this will not work with the current clock phase framework
> (which floors angles, so we'll get 0 deg, and there's no way to know what
> resolution the floors happen at). We assume that the default drive angles
> set by the hardware are good enough.
> 
> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> index 346c609..5edadc2 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -42,11 +42,13 @@ Optional properties:
>  * clocks: from common clock binding: handle to biu and ciu clocks for the
>    bus interface unit clock and the card interface unit clock.
>  
> -* clock-names: from common clock binding: Shall be "biu" and "ciu".
> -  If the biu clock is missing we'll simply skip enabling it.  If the
> -  ciu clock is missing we'll just assume that the clock is running at
> +* clock-names: from common clock binding: Shall be "biu", "ciu", "ciu_drv" and
> +  "ciu_sample".  If the biu clock is missing we'll simply skip enabling it.
> +  If the ciu clock is missing we'll just assume that the clock is running at
>    clock-frequency.  It is an error to omit both the ciu clock and the
> -  clock-frequency.
> +  clock-frequency.  "ciu_drv" and "ciu_sample" are used to control the clock
> +  phases, "ciu_sample" is required for tuning high speed modes (if no other
> +  custom tuning method is defined).
>  
>  * clock-frequency: should be the frequency (in Hz) of the ciu clock.  If this
>    is specified and the ciu clock is specified then we'll try to set the ciu
> @@ -75,6 +77,10 @@ Optional properties:
>  * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is
>    specified we'll defer probe until we can find this regulator.
>  
> +* default-sample-phase: The default phase to set ciu_sample at probing, low
> +  speeds or in case where all phases work at tuning time. If not specified
> +  0 deg will be used.
> +
>  Aliases:
>  
>  - All the MSHC controller nodes should be represented in the aliases node using
> 

[PATCH 5/8] mmc: dw_mmc: dt-binding: Add tuning related things

[Heiko Stuebner]

Hi Jaehoon,

Am Mittwoch, 2. September 2015, 14:01:52 schrieb Jaehoon Chung:
> Hi, Heiko.
> 
> On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
> > From: Alexandru M Stan <amstan@chromium.org>
> > 
> > Add ciu_drv, ciu_sample clocks and default-sample-phase. This will later
> > be used by tuning code.
> 
> As i know, ciu_drv and ciu_sample clocks are generated with "ciu" clock.
> But in these patch-set, ciu_drv and ciu_sample are controlled by clock
> framework. It's a little strange.
> Are there ciu_drv and ciu_sample clock on Rockchip?

Yes on Rockchip SoCs the drv and sample clock registers are residing inside 
the clock controller and not in the dw_mmc block.

See drivers/clk/rockchip/clk-mmcphase.c and clk-rk3288.c around line 490 .


Heiko

> > We do not touch ciu_drive (and by extension define default-drive-phase).
> > Drive phase is mostly used to define minimum hold times, while one could
> > write some code to determine what phase meets the minimum hold time
> > (ex 10 degrees) this will not work with the current clock phase framework
> > (which floors angles, so we'll get 0 deg, and there's no way to know what
> > resolution the floors happen at). We assume that the default drive angles
> > set by the hardware are good enough.
> > 
> > Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 14
> >  ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt index
> > 346c609..5edadc2 100644
> > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > 
> > @@ -42,11 +42,13 @@ Optional properties:
> >  * clocks: from common clock binding: handle to biu and ciu clocks for the
> >  
> >    bus interface unit clock and the card interface unit clock.
> > 
> > -* clock-names: from common clock binding: Shall be "biu" and "ciu".
> > -  If the biu clock is missing we'll simply skip enabling it.  If the
> > -  ciu clock is missing we'll just assume that the clock is running at
> > +* clock-names: from common clock binding: Shall be "biu", "ciu",
> > "ciu_drv" and +  "ciu_sample".  If the biu clock is missing we'll simply
> > skip enabling it. +  If the ciu clock is missing we'll just assume that
> > the clock is running at> 
> >    clock-frequency.  It is an error to omit both the ciu clock and the
> > 
> > -  clock-frequency.
> > +  clock-frequency.  "ciu_drv" and "ciu_sample" are used to control the
> > clock +  phases, "ciu_sample" is required for tuning high speed modes (if
> > no other +  custom tuning method is defined).
> > 
> >  * clock-frequency: should be the frequency (in Hz) of the ciu clock.  If
> >  this>  
> >    is specified and the ciu clock is specified then we'll try to set the
> >    ciu
> > 
> > @@ -75,6 +77,10 @@ Optional properties:
> >  * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is
> >  
> >    specified we'll defer probe until we can find this regulator.
> > 
> > +* default-sample-phase: The default phase to set ciu_sample at probing,
> > low +  speeds or in case where all phases work at tuning time. If not
> > specified +  0 deg will be used.
> > +
> > 
> >  Aliases:
> >  
> >  - All the MSHC controller nodes should be represented in the aliases node
> >  using

[PATCH 3/8] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 31 August 2015 at 20:24, Heiko Stuebner <heiko@sntech.de> wrote:
> From: Douglas Anderson <dianders@chromium.org>
>
> This adds logic to the MMC core to set VQMMC.  This is expected to be
> called by MMC drivers like dw_mmc as part of (or instead of) their
> start_signal_voltage_switch() callback.
>
> A few notes:
>
> * When setting the signal voltage to 3.3V we do our best to make VQMMC
>   and VMMC match.  It's been reported that this makes some old cards
>   happy since they were tested back in the day before UHS when VQMMC
>   and VMMC were provided by the same regulator.  A nice side effect of
>   this is that we don't end up on the hairy edge of VQMMC (2.7V),
>   which some EEs claim is a little too close to the minimum for
>   comfort.
>   If this is not supported by the supplying regulator we try to find
>   a suitable voltage within the whole 2.7V-3.6V area of the spec.
>
> * When setting the signal voltage to 1.8V or 1.2V we aim for that
>   specific voltage instead of picking the lowest one in the range.
>
> * We very purposely don't print errors in mmc_regulator_set_vqmmc().
>   There are cases where the MMC core will try several different
>   voltages and we don't want to pollute the logs.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/mmc/core/core.c  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h |  7 +++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0520064..9dc0b65 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1437,6 +1437,74 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
>  }
>  EXPORT_SYMBOL_GPL(mmc_regulator_set_ocr);
>
> +static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
> +                                                 int min_uV, int target_uV,
> +                                                 int max_uV)
> +{
> +       /*
> +        * Check if supported first to avoid errors since we may try several
> +        * signal levels during power up and don't want to show errors.
> +        */
> +       if (!regulator_is_supported_voltage(regulator, min_uV, max_uV))
> +               return -EINVAL;
> +
> +       return regulator_set_voltage_triplet(regulator, min_uV, target_uV,
> +                                            max_uV);
> +}
> +
> +/**
> + * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
> + *
> + * For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible.

Looking at the code, I don't think this statement is entirely true.
Isn't it so that we will be trying with a maximum tolerance of 0.3 V
towards the VMMC voltage level (then fall-back to the complete range)?
Perhaps you can find a better way to describe that in the change log.

Just to be clear, I believe this approach make sense but I appreciate
some more details about the policy, both in the code and in the change
log.

> + * That will match the behavior of old boards where VQMMC and VMMC were supplied
> + * by the same supply.  The Bus Operating conditions for 3.3V signaling in the
> + * SD card spec also define VQMMC in terms of VMMC.
> + * If this is not possible we'll try the full 2.7-3.6V of the spec.
> + *
> + * For 1.2V and 1.8V signaling we'll try to get as close as possible to the
> + * requested voltage.  This is definitely a good idea for UHS where there's a
> + * separate regulator on the card that's trying to make 1.8V and it's best if
> + * we match.
> + *
> + * This function is expected to be used by a controller's
> + * start_signal_voltage_switch() function.
> + */
> +int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       int volt, min_uV, max_uV;
> +
> +       /* If no vqmmc supply then we can't change the voltage */
> +       if (IS_ERR(mmc->supply.vqmmc))
> +               return -EINVAL;

In general vqmmc is considered as an optional regulator and that's
also how host drivers treat it. So perhaps it would make sense to
return 0 here instead of an error code or what do you think?

> +
> +       switch (ios->signal_voltage) {
> +       case MMC_SIGNAL_VOLTAGE_120:
> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                                               1100000, 1200000, 1300000);
> +       case MMC_SIGNAL_VOLTAGE_180:
> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                                               1700000, 1800000, 1950000);
> +       case MMC_SIGNAL_VOLTAGE_330:
> +               volt = regulator_get_voltage(mmc->supply.vmmc);

Before invoking regulator_get_voltage(), we need to check for an
existing regulator handle for vmmc.

Moreover, as the regulator handle to vmmc is optional, perhaps we
should fall-back to use the ios->vdd bit to find the current voltage
level?

> +               if (volt < 0)
> +                       return volt;
> +
> +               min_uV = max(volt - 300000, 2700000);
> +               max_uV = min(volt + 300000, 3600000);

These calculations deserves a comment.

> +
> +               /* try to stay close to vmmc at first */
> +               if (!mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                                               min_uV, volt, max_uV))
> +                       return 0;
> +
> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                                               2700000, volt, 3600000);
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(mmc_regulator_set_vqmmc);
> +
>  #endif /* CONFIG_REGULATOR */
>
>  int mmc_regulator_get_supply(struct mmc_host *mmc)
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 83b81fd..a2a78eb 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -423,6 +423,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);
>  int mmc_regulator_set_ocr(struct mmc_host *mmc,
>                         struct regulator *supply,
>                         unsigned short vdd_bit);
> +int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios);
>  #else
>  static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
>  {
> @@ -435,6 +436,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
>  {
>         return 0;
>  }
> +
> +static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc,
> +                                         struct mmc_ios *ios)
> +{
> +       return -EINVAL;

According to my upper comment about vqmmc being optional, perhaps this
shouldn't be treated as an error!?

> +}
>  #endif
>
>  int mmc_regulator_get_supply(struct mmc_host *mmc);
> --
> 2.1.4
>

Finally, I wonder if you have considered to handle the
regulator_enable|disable() calls from within this new API? Currently
host driver deals with that themselves, but it would be nice to get
some consolidation around that. Similar to what we already have for
the vmmc regulator through the mmc_regulator_set_ocr() API.

If you considered this, I am fine with adding that kind of
functionality as a separate patch on top of this one, it gets easier
to discuss/review.

Kind regards
Uffe

[PATCH 3/8] mmc: core: Add mmc_regulator_set_vqmmc()

[Doug Anderson]

Ulf,

On Wed, Sep 2, 2015 at 4:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> +/**
>> + * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
>> + *
>> + * For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible.
>
> Looking at the code, I don't think this statement is entirely true.
> Isn't it so that we will be trying with a maximum tolerance of 0.3 V
> towards the VMMC voltage level (then fall-back to the complete range)?
> Perhaps you can find a better way to describe that in the change log.

If regulator_set_voltage_triplet() is ever implemented more correctly
then the description here is correct.  ...the problem is that
regulator_set_voltage_triplet() is still using the same shortcut that
regulator_set_voltage_tol() was using.


>> +int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
>> +{
>> +       int volt, min_uV, max_uV;
>> +
>> +       /* If no vqmmc supply then we can't change the voltage */
>> +       if (IS_ERR(mmc->supply.vqmmc))
>> +               return -EINVAL;
>
> In general vqmmc is considered as an optional regulator and that's
> also how host drivers treat it. So perhaps it would make sense to
> return 0 here instead of an error code or what do you think?

The idea is that since this is intended to be called by
start_signal_voltage_switch() and having no vqmmc should be considered
an error for start_signal_voltage_switch() then it should be an error
here.  What do you think?


>> +
>> +               /* try to stay close to vmmc at first */
>> +               if (!mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>> +                                               min_uV, volt, max_uV))
>> +                       return 0;
>> +
>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>> +                                               2700000, volt, 3600000);

The whole fact that there are two calls here is really just because of
the limitations of the current implementation of
regulator_set_voltage_triplet().  If that implementation is ever fixed
then we'd just need a single call.  Probably worth a comment saying
that?

[PATCH 3/8] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 2 September 2015 at 18:20, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Wed, Sep 2, 2015 at 4:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> +/**
>>> + * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
>>> + *
>>> + * For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible.
>>
>> Looking at the code, I don't think this statement is entirely true.
>> Isn't it so that we will be trying with a maximum tolerance of 0.3 V
>> towards the VMMC voltage level (then fall-back to the complete range)?
>> Perhaps you can find a better way to describe that in the change log.
>
> If regulator_set_voltage_triplet() is ever implemented more correctly
> then the description here is correct.  ...the problem is that
> regulator_set_voltage_triplet() is still using the same shortcut that
> regulator_set_voltage_tol() was using.

Okay, let's mention that somehow.

>
>
>>> +int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
>>> +{
>>> +       int volt, min_uV, max_uV;
>>> +
>>> +       /* If no vqmmc supply then we can't change the voltage */
>>> +       if (IS_ERR(mmc->supply.vqmmc))
>>> +               return -EINVAL;
>>
>> In general vqmmc is considered as an optional regulator and that's
>> also how host drivers treat it. So perhaps it would make sense to
>> return 0 here instead of an error code or what do you think?
>
> The idea is that since this is intended to be called by
> start_signal_voltage_switch() and having no vqmmc should be considered
> an error for start_signal_voltage_switch() then it should be an error
> here.  What do you think?

Okay!

>
>
>>> +
>>> +               /* try to stay close to vmmc at first */
>>> +               if (!mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>>> +                                               min_uV, volt, max_uV))
>>> +                       return 0;
>>> +
>>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>>> +                                               2700000, volt, 3600000);
>
> The whole fact that there are two calls here is really just because of
> the limitations of the current implementation of
> regulator_set_voltage_triplet().  If that implementation is ever fixed
> then we'd just need a single call.  Probably worth a comment saying
> that?

Yes, please!

Kind regards
Uffe

[PATCH 6/8] mmc: dw_mmc: Generic MMC tuning with the clock phase framework

[Jaehoon Chung]

Dear, Heiko.

On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
> From: Alexandru M Stan <amstan@chromium.org>
> 
> This algorithm will try 1 degree increments, since there's no way to tell
> what resolution the underlying phase code uses. As an added bonus, doing
> many tunings yields better results since some tests are run more than once
> (ex: if the underlying driver uses 45 degree increments, the tuning code
> will try the same angle more than once).
> 
> It will then construct a list of good phase ranges (even ranges that cross
> 360/0), will pick the biggest range then it will set the sample_clk to the
> middle of that range.
> 
> We do not touch ciu_drive (and by extension define default-drive-phase).
> Drive phase is mostly used to define minimum hold times, while one could
> write some code to determine what phase meets the minimum hold time (ex 10
> degrees) this will not work with the current clock phase framework (which
> floors angles, so we'll get 0 deg, and there's no way to know what
> resolution the floors happen at). We assume that the default drive angles
> set by the hardware are good enough.
> 
> If a device has device specific code (like exynos) then that will still
> take precedence, otherwise this new code will execute. If the device wants
> to tune, but has no sample_clk defined we'll return EIO with an error
> message.

Which point is "_generic_"? I don't find the code that control the register relevant to CLK_DRV/SMPL PHASE.
It seems that posted the similar patches at u-boot mailing list..

Best Regards,
Jaehoon Chung

> 
> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> 
> Convert to mmc_send_tuning()
> Fold in from the ChromeOS-tree:
>  - mmc: dw_mmc: Change tuning to only 16 phases
>  - mmc: dw_mmc: Test more phases
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/mmc/host/dw_mmc.c  | 142 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/dw_mmc.h |   3 +
>  2 files changed, 145 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index b1b7e7f..13bcde0 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1194,6 +1194,12 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	if (drv_data && drv_data->set_ios)
>  		drv_data->set_ios(slot->host, ios);
>  
> +	/* Make sure we use phases which we can enumerate with */
> +	if (!IS_ERR(slot->host->sample_clk)) {
> +		clk_set_phase(slot->host->sample_clk,
> +			      slot->host->default_sample_phase);
> +	}
> +
>  	switch (ios->power_mode) {
>  	case MMC_POWER_UP:
>  		if (!IS_ERR(mmc->supply.vmmc)) {
> @@ -1414,6 +1420,127 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  	spin_unlock_irqrestore(&host->irq_lock, irqflags);
>  }
>  
> +#define NUM_PHASES			360
> +#define TUNING_ITERATION_TO_PHASE(i)	(DIV_ROUND_UP((i) * 360, NUM_PHASES))
> +
> +static int dw_mci_execute_generic_tuning(struct dw_mci_slot *slot)
> +{
> +	struct dw_mci *host = slot->host;
> +	struct mmc_host *mmc = slot->mmc;
> +	int ret = 0;
> +	int i;
> +	bool v, prev_v = 0, first_v;
> +	struct range_t {
> +		int start;
> +		int end; /* inclusive */
> +	};
> +	struct range_t *ranges;
> +	unsigned int range_count = 0;
> +	int longest_range_len = -1;
> +	int longest_range = -1;
> +	int middle_phase;
> +
> +	if (IS_ERR(host->sample_clk)) {
> +		dev_err(host->dev, "Tuning clock (sample_clk) not defined.\n");
> +		return -EIO;
> +	}
> +
> +	ranges = kmalloc_array(NUM_PHASES / 2 + 1, sizeof(*ranges), GFP_KERNEL);
> +	if (!ranges)
> +		return -ENOMEM;
> +
> +	/* Try each phase and extract good ranges */
> +	for (i = 0; i < NUM_PHASES; ) {
> +		clk_set_phase(host->sample_clk, TUNING_ITERATION_TO_PHASE(i));
> +
> +		v = !mmc_send_tuning(mmc);
> +
> +		if (i == 0)
> +			first_v = v;
> +
> +		if ((!prev_v) && v) {
> +			range_count++;
> +			ranges[range_count-1].start = i;
> +		}
> +		if (v) {
> +			ranges[range_count-1].end = i;
> +			i++;
> +		} else if (i == NUM_PHASES - 1) {
> +			/* No extra skipping rules if we're at the end */
> +			i++;
> +		} else {
> +			/*
> +			 * No need to check too close to an invalid
> +			 * one since testing bad phases is slow.  Skip
> +			 * 20 degrees.
> +			 */
> +			i += DIV_ROUND_UP(20 * NUM_PHASES, 360);
> +
> +			/* Always test the last one */
> +			if (i >= NUM_PHASES)
> +				i = NUM_PHASES - 1;
> +		}
> +
> +		prev_v = v;
> +	}
> +
> +	if (range_count == 0) {
> +		dev_warn(host->dev, "All phases bad!");
> +		ret = -EIO;
> +		goto free;
> +	}
> +
> +	/* wrap around case, merge the end points */
> +	if ((range_count > 1) && first_v && v) {
> +		ranges[0].start = ranges[range_count-1].start;
> +		range_count--;
> +	}
> +
> +	if (ranges[0].start == 0 && ranges[0].end == NUM_PHASES - 1) {
> +		clk_set_phase(host->sample_clk, host->default_sample_phase);
> +		dev_info(host->dev, "All phases work, using default phase %d.",
> +			 host->default_sample_phase);
> +		goto free;
> +	}
> +
> +	/* Find the longest range */
> +	for (i = 0; i < range_count; i++) {
> +		int len = (ranges[i].end - ranges[i].start + 1);
> +
> +		if (len < 0)
> +			len += NUM_PHASES;
> +
> +		if (longest_range_len < len) {
> +			longest_range_len = len;
> +			longest_range = i;
> +		}
> +
> +		dev_dbg(host->dev, "Good phase range %d-%d (%d len)\n",
> +			TUNING_ITERATION_TO_PHASE(ranges[i].start),
> +			TUNING_ITERATION_TO_PHASE(ranges[i].end),
> +			len
> +		);
> +	}
> +
> +	dev_dbg(host->dev, "Best phase range %d-%d (%d len)\n",
> +		TUNING_ITERATION_TO_PHASE(ranges[longest_range].start),
> +		TUNING_ITERATION_TO_PHASE(ranges[longest_range].end),
> +		longest_range_len
> +	);
> +
> +	middle_phase = ranges[longest_range].start + longest_range_len / 2;
> +	middle_phase %= NUM_PHASES;
> +	dev_info(host->dev, "Successfully tuned phase to %d\n",
> +		 TUNING_ITERATION_TO_PHASE(middle_phase));
> +
> +	clk_set_phase(host->sample_clk,
> +		      TUNING_ITERATION_TO_PHASE(middle_phase));
> +
> +free:
> +	kfree(ranges);
> +	return ret;
> +}
> +
>  static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
> @@ -1423,6 +1550,8 @@ static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  
>  	if (drv_data && drv_data->execute_tuning)
>  		err = drv_data->execute_tuning(slot);
> +	else
> +		err = dw_mci_execute_generic_tuning(slot);
>  	return err;
>  }
>  
> @@ -2741,6 +2870,11 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>  	if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>  		pdata->bus_hz = clock_frequency;
>  
> +	if (of_property_read_u32(np, "default-sample-phase",
> +					&host->default_sample_phase)) {
> +		host->default_sample_phase = 0;
> +	}
> +
>  	if (drv_data && drv_data->parse_dt) {
>  		ret = drv_data->parse_dt(host);
>  		if (ret)
> @@ -2843,6 +2977,14 @@ int dw_mci_probe(struct dw_mci *host)
>  		host->bus_hz = clk_get_rate(host->ciu_clk);
>  	}
>  
> +	host->drv_clk = devm_clk_get(host->dev, "ciu_drv");
> +	if (IS_ERR(host->drv_clk))
> +		dev_dbg(host->dev, "ciu_drv not available\n");
> +
> +	host->sample_clk = devm_clk_get(host->dev, "ciu_sample");
> +	if (IS_ERR(host->sample_clk))
> +		dev_dbg(host->dev, "ciu_sample not available\n");
> +
>  	if (!host->bus_hz) {
>  		dev_err(host->dev,
>  			"Platform data must supply bus speed\n");
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 134c574..40187ba 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -176,7 +176,10 @@ struct dw_mci {
>  	void			*priv;
>  	struct clk		*biu_clk;
>  	struct clk		*ciu_clk;
> +	struct clk		*drv_clk;
> +	struct clk		*sample_clk;
>  	struct dw_mci_slot	*slot[MAX_MCI_SLOTS];
> +	int			default_sample_phase;
>  
>  	/* FIFO push and pull */
>  	int			fifo_depth;
> 

[PATCH 6/8] mmc: dw_mmc: Generic MMC tuning with the clock phase framework

[Heiko Stübner]

Hi,

Am Dienstag, 15. September 2015, 17:25:38 schrieb Jaehoon Chung:
> On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
> > From: Alexandru M Stan <amstan@chromium.org>
> > 
> > This algorithm will try 1 degree increments, since there's no way to tell
> > what resolution the underlying phase code uses. As an added bonus, doing
> > many tunings yields better results since some tests are run more than once
> > (ex: if the underlying driver uses 45 degree increments, the tuning code
> > will try the same angle more than once).
> > 
> > It will then construct a list of good phase ranges (even ranges that cross
> > 360/0), will pick the biggest range then it will set the sample_clk to the
> > middle of that range.
> > 
> > We do not touch ciu_drive (and by extension define default-drive-phase).
> > Drive phase is mostly used to define minimum hold times, while one could
> > write some code to determine what phase meets the minimum hold time (ex 10
> > degrees) this will not work with the current clock phase framework (which
> > floors angles, so we'll get 0 deg, and there's no way to know what
> > resolution the floors happen at). We assume that the default drive angles
> > set by the hardware are good enough.
> > 
> > If a device has device specific code (like exynos) then that will still
> > take precedence, otherwise this new code will execute. If the device wants
> > to tune, but has no sample_clk defined we'll return EIO with an error
> > message.
> 
> Which point is "_generic_"? I don't find the code that control the register
> relevant to CLK_DRV/SMPL PHASE. It seems that posted the similar patches at
> u-boot mailing list..

The "generic" part is that it uses the clk phase API for dw_mmc 
implementations where the clkgen controlling interface is outside the dw_mmc 
IP itself. So it's open for other implementations as well.

But if you are more comfortable with it, I can also move it into the dw_mmc-
rockchip variant for the time being, until another user comes along.


Heiko

[PATCH 6/8] mmc: dw_mmc: Generic MMC tuning with the clock phase framework

[Jaehoon Chung]

Hi,

On 09/16/2015 07:09 AM, Heiko St?bner wrote:
> Hi,
> 
> Am Dienstag, 15. September 2015, 17:25:38 schrieb Jaehoon Chung:
>> On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
>>> From: Alexandru M Stan <amstan@chromium.org>
>>>
>>> This algorithm will try 1 degree increments, since there's no way to tell
>>> what resolution the underlying phase code uses. As an added bonus, doing
>>> many tunings yields better results since some tests are run more than once
>>> (ex: if the underlying driver uses 45 degree increments, the tuning code
>>> will try the same angle more than once).
>>>
>>> It will then construct a list of good phase ranges (even ranges that cross
>>> 360/0), will pick the biggest range then it will set the sample_clk to the
>>> middle of that range.
>>>
>>> We do not touch ciu_drive (and by extension define default-drive-phase).
>>> Drive phase is mostly used to define minimum hold times, while one could
>>> write some code to determine what phase meets the minimum hold time (ex 10
>>> degrees) this will not work with the current clock phase framework (which
>>> floors angles, so we'll get 0 deg, and there's no way to know what
>>> resolution the floors happen at). We assume that the default drive angles
>>> set by the hardware are good enough.
>>>
>>> If a device has device specific code (like exynos) then that will still
>>> take precedence, otherwise this new code will execute. If the device wants
>>> to tune, but has no sample_clk defined we'll return EIO with an error
>>> message.
>>
>> Which point is "_generic_"? I don't find the code that control the register
>> relevant to CLK_DRV/SMPL PHASE. It seems that posted the similar patches at
>> u-boot mailing list..
> 
> The "generic" part is that it uses the clk phase API for dw_mmc 
> implementations where the clkgen controlling interface is outside the dw_mmc 
> IP itself. So it's open for other implementations as well.

Designware IP also has the CLK phase register(UHS_REG_EXT register)...
if this code is related with it, it should be located into dw-mmc.c.

> 
> But if you are more comfortable with it, I can also move it into the dw_mmc-
> rockchip variant for the time being, until another user comes along.

I think more better that this code is located into dw_mmc-rockchip. how about?

Best Regards,
Jaehoon Chung

> 
> 
> Heiko
> 
> 

[PATCH 6/8] mmc: dw_mmc: Generic MMC tuning with the clock phase framework

[Heiko Stübner]

Hi,

Am Mittwoch, 16. September 2015, 11:30:26 schrieb Jaehoon Chung:
> On 09/16/2015 07:09 AM, Heiko St?bner wrote:
> > Am Dienstag, 15. September 2015, 17:25:38 schrieb Jaehoon Chung:
> >> On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
> >>> From: Alexandru M Stan <amstan@chromium.org>
> >>> 
> >>> This algorithm will try 1 degree increments, since there's no way to
> >>> tell
> >>> what resolution the underlying phase code uses. As an added bonus, doing
> >>> many tunings yields better results since some tests are run more than
> >>> once
> >>> (ex: if the underlying driver uses 45 degree increments, the tuning code
> >>> will try the same angle more than once).
> >>> 
> >>> It will then construct a list of good phase ranges (even ranges that
> >>> cross
> >>> 360/0), will pick the biggest range then it will set the sample_clk to
> >>> the
> >>> middle of that range.
> >>> 
> >>> We do not touch ciu_drive (and by extension define default-drive-phase).
> >>> Drive phase is mostly used to define minimum hold times, while one could
> >>> write some code to determine what phase meets the minimum hold time (ex
> >>> 10
> >>> degrees) this will not work with the current clock phase framework
> >>> (which
> >>> floors angles, so we'll get 0 deg, and there's no way to know what
> >>> resolution the floors happen at). We assume that the default drive
> >>> angles
> >>> set by the hardware are good enough.
> >>> 
> >>> If a device has device specific code (like exynos) then that will still
> >>> take precedence, otherwise this new code will execute. If the device
> >>> wants
> >>> to tune, but has no sample_clk defined we'll return EIO with an error
> >>> message.
> >> 
> >> Which point is "_generic_"? I don't find the code that control the
> >> register
> >> relevant to CLK_DRV/SMPL PHASE. It seems that posted the similar patches
> >> at
> >> u-boot mailing list..
> > 
> > The "generic" part is that it uses the clk phase API for dw_mmc
> > implementations where the clkgen controlling interface is outside the
> > dw_mmc IP itself. So it's open for other implementations as well.
> 
> Designware IP also has the CLK phase register(UHS_REG_EXT register)...
> if this code is related with it, it should be located into dw-mmc.c.

UHS_REG_EXT is acutally "reserved" on both the rk3288 as well as the rk3368. 
rk3036/rk3128 (Cortex-A7) provide a bit description, but the tuning 
documentation still uses the controls located in the clock controller.

So I guess UHS_REG_EXT is the real "generic" solution.

> > But if you are more comfortable with it, I can also move it into the
> > dw_mmc- rockchip variant for the time being, until another user comes
> > along.
> I think more better that this code is located into dw_mmc-rockchip. how
> about?

As described above, moving that to the rockchip part sounds sensible. And I 
guess we can think more about it, once a second user appears.


Heiko

[PATCH 6/8] mmc: dw_mmc: Generic MMC tuning with the clock phase framework

[Jaehoon Chung]

Hi,

On 09/16/2015 11:52 PM, Heiko St?bner wrote:
> Hi,
> 
> Am Mittwoch, 16. September 2015, 11:30:26 schrieb Jaehoon Chung:
>> On 09/16/2015 07:09 AM, Heiko St?bner wrote:
>>> Am Dienstag, 15. September 2015, 17:25:38 schrieb Jaehoon Chung:
>>>> On 09/01/2015 03:24 AM, Heiko Stuebner wrote:
>>>>> From: Alexandru M Stan <amstan@chromium.org>
>>>>>
>>>>> This algorithm will try 1 degree increments, since there's no way to
>>>>> tell
>>>>> what resolution the underlying phase code uses. As an added bonus, doing
>>>>> many tunings yields better results since some tests are run more than
>>>>> once
>>>>> (ex: if the underlying driver uses 45 degree increments, the tuning code
>>>>> will try the same angle more than once).
>>>>>
>>>>> It will then construct a list of good phase ranges (even ranges that
>>>>> cross
>>>>> 360/0), will pick the biggest range then it will set the sample_clk to
>>>>> the
>>>>> middle of that range.
>>>>>
>>>>> We do not touch ciu_drive (and by extension define default-drive-phase).
>>>>> Drive phase is mostly used to define minimum hold times, while one could
>>>>> write some code to determine what phase meets the minimum hold time (ex
>>>>> 10
>>>>> degrees) this will not work with the current clock phase framework
>>>>> (which
>>>>> floors angles, so we'll get 0 deg, and there's no way to know what
>>>>> resolution the floors happen at). We assume that the default drive
>>>>> angles
>>>>> set by the hardware are good enough.
>>>>>
>>>>> If a device has device specific code (like exynos) then that will still
>>>>> take precedence, otherwise this new code will execute. If the device
>>>>> wants
>>>>> to tune, but has no sample_clk defined we'll return EIO with an error
>>>>> message.
>>>>
>>>> Which point is "_generic_"? I don't find the code that control the
>>>> register
>>>> relevant to CLK_DRV/SMPL PHASE. It seems that posted the similar patches
>>>> at
>>>> u-boot mailing list..
>>>
>>> The "generic" part is that it uses the clk phase API for dw_mmc
>>> implementations where the clkgen controlling interface is outside the
>>> dw_mmc IP itself. So it's open for other implementations as well.
>>
>> Designware IP also has the CLK phase register(UHS_REG_EXT register)...
>> if this code is related with it, it should be located into dw-mmc.c.
> 
> UHS_REG_EXT is acutally "reserved" on both the rk3288 as well as the rk3368. 
> rk3036/rk3128 (Cortex-A7) provide a bit description, but the tuning 
> documentation still uses the controls located in the clock controller.
> 
> So I guess UHS_REG_EXT is the real "generic" solution.
> 
>>> But if you are more comfortable with it, I can also move it into the
>>> dw_mmc- rockchip variant for the time being, until another user comes
>>> along.
>> I think more better that this code is located into dw_mmc-rockchip. how
>> about?
> 
> As described above, moving that to the rockchip part sounds sensible. And I 
> guess we can think more about it, once a second user appears.

Sure, we can think more about this.
As you knew, clock phase is closely related to the timing issue.
So clock phase scheme needs to control however.
In future, if somebody introduce the similar control as rockchip, we can discuss about it.

Best Regards,
Jaehoon Chung

> 
> 
> Heiko
>