Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1701661 > unrolled thread

[PATCH v8 1/2] mmc: dw_mmc: move controller reset before driver init

Started byLi Wei <liwei213@huawei.com>
First post2017-08-02 05:20 +0200
Last post2017-08-08 12:50 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v8 1/2] mmc: dw_mmc: move controller reset before driver init Li Wei <liwei213@huawei.com> - 2017-08-02 05:20 +0200
    [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660 Li Wei <liwei213@huawei.com> - 2017-08-02 05:20 +0200
      Re: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660 Ulf Hansson <ulf.hansson@linaro.org> - 2017-08-08 13:10 +0200
        答复: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660 "liwei (CM)" <liwei213@huawei.com> - 2017-08-09 05:10 +0200
    Re: [PATCH v8 1/2] mmc: dw_mmc: move controller reset before driver init Ulf Hansson <ulf.hansson@linaro.org> - 2017-08-08 12:50 +0200

#1701661 — [PATCH v8 1/2] mmc: dw_mmc: move controller reset before driver init

FromLi Wei <liwei213@huawei.com>
Date2017-08-02 05:20 +0200
Subject[PATCH v8 1/2] mmc: dw_mmc: move controller reset before driver init
Message-ID<u9Smu-4n4-7@gated-at.bofh.it>
This commit modifies dw_mci_probe(), it moves reset assertion before
drv_data->init(host)

Some driver needs to access controller registers in its .init() ops. So,
in order to make such access safe, we should do controller reset before
.init() being called.

Signed-off-by: Wei Li <liwei213@huawei.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: Chen Jun <chenjun14@huawei.com>
---
 drivers/mmc/host/dw_mmc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a9dfb26972f2..f2fa928e1a12 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3067,6 +3067,12 @@ int dw_mci_probe(struct dw_mci *host)
 		goto err_clk_ciu;
 	}
 
+	if (!IS_ERR(host->pdata->rstc)) {
+		reset_control_assert(host->pdata->rstc);
+		usleep_range(10, 50);
+		reset_control_deassert(host->pdata->rstc);
+	}
+
 	if (drv_data && drv_data->init) {
 		ret = drv_data->init(host);
 		if (ret) {
@@ -3076,12 +3082,6 @@ int dw_mci_probe(struct dw_mci *host)
 		}
 	}
 
-	if (!IS_ERR(host->pdata->rstc)) {
-		reset_control_assert(host->pdata->rstc);
-		usleep_range(10, 50);
-		reset_control_deassert(host->pdata->rstc);
-	}
-
 	setup_timer(&host->cmd11_timer,
 		    dw_mci_cmd11_timer, (unsigned long)host);
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1701666 — [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660

FromLi Wei <liwei213@huawei.com>
Date2017-08-02 05:20 +0200
Subject[PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660
Message-ID<u9Smu-4n4-17@gated-at.bofh.it>
In reply to#1701661
Add sd card support for hi3660 soc

Signed-off-by: Li Wei <liwei213@huawei.com>
Signed-off-by: Chen Jun <chenjun14@huawei.com>

Major changes in v3:
 - solve review comments from Heiner Kallweit.
   *use the GENMASK and FIELD_PREP macros replace the bit shift operation.
   *use usleep_range() replace udelay() and mdelay().

Major changes in v4:
 - solve review comments from Jaehoon Chung.
   *move common register for dwmmc controller to dwmmc header file.
   *modify definitions type of some register variables.
   *get rid of the magic numbers.

Major changes in v5:
 - further improve coding style.

Major changes in v6:
 - solve review comments for Jaehoon Chung.
   *modify dw_mci_hi3660_set_ios() to static.
   *fix the comment style.

Major changes in v7:
 - solve review comments for John Stultz.
   *remove reset code in dw_mmc-k3.c,use reset in core mmc.

Major changes in v8:
 - modify patch v7 name and dependency order.
---
 drivers/mmc/host/dw_mmc-k3.c | 311 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/dw_mmc.h    |   2 +
 2 files changed, 313 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index e38fb0020bb1..efe18bf445ff 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -8,6 +8,8 @@
  * (at your option) any later version.
  */
 
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/mfd/syscon.h>
 #include <linux/mmc/host.h>
@@ -28,7 +30,38 @@
 #define AO_SCTRL_SEL18		BIT(10)
 #define AO_SCTRL_CTRL3		0x40C
 
+#define DWMMC_SDIO_ID 2
+
+#define SOC_SCTRL_SCPERCTRL5    (0x314)
+#define SDCARD_IO_SEL18         BIT(2)
+
+#define SDCARD_RD_THRESHOLD  (512)
+
+#define GENCLK_DIV (7)
+
+#define GPIO_CLK_ENABLE                   BIT(16)
+#define GPIO_CLK_DIV_MASK                 GENMASK(11, 8)
+#define GPIO_USE_SAMPLE_DLY_MASK          GENMASK(13, 13)
+#define UHS_REG_EXT_SAMPLE_PHASE_MASK     GENMASK(20, 16)
+#define UHS_REG_EXT_SAMPLE_DRVPHASE_MASK  GENMASK(25, 21)
+#define UHS_REG_EXT_SAMPLE_DLY_MASK       GENMASK(30, 26)
+
+#define TIMING_MODE     3
+#define TIMING_CFG_NUM 10
+
+#define PULL_DOWN BIT(1)
+#define PULL_UP   BIT(0)
+
+#define NUM_PHASES (40)
+
+#define ENABLE_SHIFT_MIN_SMPL (4)
+#define ENABLE_SHIFT_MAX_SMPL (12)
+#define USE_DLY_MIN_SMPL (11)
+#define USE_DLY_MAX_SMPL (14)
+
 struct k3_priv {
+	int ctrl_id;
+	u32 cur_speed;
 	struct regmap	*reg;
 };
 
@@ -38,6 +71,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
 	0
 };
 
+struct hs_timing {
+	u32 drv_phase;
+	u32 smpl_dly;
+	u32 smpl_phase_max;
+	u32 smpl_phase_min;
+};
+
+struct hs_timing hs_timing_cfg[TIMING_MODE][TIMING_CFG_NUM] = {
+	{ /* reserved */ },
+	{ /* SD */
+		{7, 0, 15, 15,},  /* 0: LEGACY 400k */
+		{6, 0,  4,  4,},  /* 1: MMC_HS */
+		{6, 0,  3,  3,},  /* 2: SD_HS */
+		{6, 0, 15, 15,},  /* 3: SDR12 */
+		{6, 0,  2,  2,},  /* 4: SDR25 */
+		{4, 0, 11,  0,},  /* 5: SDR50 */
+		{6, 4, 15,  0,},  /* 6: SDR104 */
+		{0},              /* 7: DDR50 */
+		{0},              /* 8: DDR52 */
+		{0},              /* 9: HS200 */
+	},
+	{ /* SDIO */
+		{7, 0, 15, 15,},  /* 0: LEGACY 400k */
+		{0},              /* 1: MMC_HS */
+		{6, 0, 15, 15,},  /* 2: SD_HS */
+		{6, 0, 15, 15,},  /* 3: SDR12 */
+		{6, 0,  0,  0,},  /* 4: SDR25 */
+		{4, 0, 12,  0,},  /* 5: SDR50 */
+		{5, 4, 15,  0,},  /* 6: SDR104 */
+		{0},              /* 7: DDR50 */
+		{0},              /* 8: DDR52 */
+		{0},              /* 9: HS200 */
+	}
+};
+
 static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 {
 	int ret;
@@ -66,6 +134,10 @@ static int dw_mci_hi6220_parse_dt(struct dw_mci *host)
 	if (IS_ERR(priv->reg))
 		priv->reg = NULL;
 
+	priv->ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
+	if (priv->ctrl_id < 0)
+		priv->ctrl_id = 0;
+
 	host->priv = priv;
 	return 0;
 }
@@ -144,7 +216,246 @@ static const struct dw_mci_drv_data hi6220_data = {
 	.execute_tuning		= dw_mci_hi6220_execute_tuning,
 };
 
+static void dw_mci_hs_set_timing(struct dw_mci *host, int timing,
+				     int smpl_phase)
+{
+	u32 drv_phase;
+	u32 smpl_dly;
+	u32 use_smpl_dly = 0;
+	u32 enable_shift = 0;
+	u32 reg_value;
+	int ctrl_id;
+	struct k3_priv *priv;
+
+	priv = host->priv;
+	ctrl_id = priv->ctrl_id;
+
+	drv_phase = hs_timing_cfg[ctrl_id][timing].drv_phase;
+	smpl_dly   = hs_timing_cfg[ctrl_id][timing].smpl_dly;
+	if (smpl_phase == -1)
+		smpl_phase = (hs_timing_cfg[ctrl_id][timing].smpl_phase_max +
+			     hs_timing_cfg[ctrl_id][timing].smpl_phase_min) / 2;
+
+	switch (timing) {
+	case MMC_TIMING_UHS_SDR104:
+		if (smpl_phase >= USE_DLY_MIN_SMPL &&
+				smpl_phase <= USE_DLY_MAX_SMPL)
+			use_smpl_dly = 1;
+			/* fallthrough */
+	case MMC_TIMING_UHS_SDR50:
+		if (smpl_phase >= ENABLE_SHIFT_MIN_SMPL &&
+				smpl_phase <= ENABLE_SHIFT_MAX_SMPL)
+			enable_shift = 1;
+		break;
+	}
+
+	mci_writel(host, GPIO, 0x0);
+	usleep_range(5, 10);
+
+	reg_value = FIELD_PREP(UHS_REG_EXT_SAMPLE_PHASE_MASK, smpl_phase) |
+		    FIELD_PREP(UHS_REG_EXT_SAMPLE_DLY_MASK, smpl_dly) |
+		    FIELD_PREP(UHS_REG_EXT_SAMPLE_DRVPHASE_MASK, drv_phase);
+	mci_writel(host, UHS_REG_EXT, reg_value);
+
+	mci_writel(host, ENABLE_SHIFT, enable_shift);
+
+	reg_value = FIELD_PREP(GPIO_CLK_DIV_MASK, GENCLK_DIV) |
+			     FIELD_PREP(GPIO_USE_SAMPLE_DLY_MASK, use_smpl_dly);
+	mci_writel(host, GPIO, (unsigned int)reg_value | GPIO_CLK_ENABLE);
+
+	/* We should delay 1ms wait for timing setting finished. */
+	usleep_range(1000, 2000);
+}
+
+static int dw_mci_hi3660_init(struct dw_mci *host)
+{
+	mci_writel(host, CDTHRCTL, SDMMC_SET_THLD(SDCARD_RD_THRESHOLD,
+		    SDMMC_CARD_RD_THR_EN));
+
+	dw_mci_hs_set_timing(host, MMC_TIMING_LEGACY, -1);
+	host->bus_hz /= (GENCLK_DIV + 1);
+
+	return 0;
+}
+
+static int dw_mci_set_sel18(struct dw_mci *host, bool set)
+{
+	int ret;
+	unsigned int val;
+	struct k3_priv *priv;
+
+	priv = host->priv;
+
+	val = set ? SDCARD_IO_SEL18 : 0;
+	ret = regmap_update_bits(priv->reg, SOC_SCTRL_SCPERCTRL5,
+				 SDCARD_IO_SEL18, val);
+	if (ret) {
+		dev_err(host->dev, "sel18 %u error\n", val);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void dw_mci_hi3660_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	int ret;
+	unsigned long wanted;
+	unsigned long actual;
+	struct k3_priv *priv = host->priv;
+
+	if (!ios->clock || ios->clock == priv->cur_speed)
+		return;
+
+	wanted = ios->clock * (GENCLK_DIV + 1);
+	ret = clk_set_rate(host->ciu_clk, wanted);
+	if (ret) {
+		dev_err(host->dev, "failed to set rate %luHz\n", wanted);
+		return;
+	}
+	actual = clk_get_rate(host->ciu_clk);
+
+	dw_mci_hs_set_timing(host, ios->timing, -1);
+	host->bus_hz = actual / (GENCLK_DIV + 1);
+	host->current_speed = 0;
+	priv->cur_speed = host->bus_hz;
+}
+
+static int dw_mci_get_best_clksmpl(unsigned int sample_flag)
+{
+	int i;
+	int interval;
+	unsigned int v;
+	unsigned int len;
+	unsigned int range_start = 0;
+	unsigned int range_length = 0;
+	unsigned int middle_range = 0;
+
+	if (!sample_flag)
+		return -EIO;
+
+	if (~sample_flag == 0)
+		return 0;
+
+	i = ffs(sample_flag) - 1;
+
+	/*
+	* A clock cycle is divided into 32 phases,
+	* each of which is represented by a bit,
+	* finding the optimal phase.
+	*/
+	while (i < 32) {
+		v = ror32(sample_flag, i);
+		len = ffs(~v) - 1;
+
+		if (len > range_length) {
+			range_length = len;
+			range_start = i;
+		}
+
+		interval = ffs(v >> len) - 1;
+		if (interval < 0)
+			break;
+
+		i += len + interval;
+	}
+
+	middle_range = range_start + range_length / 2;
+	if (middle_range >= 32)
+		middle_range %= 32;
+
+	return middle_range;
+}
+
+static int dw_mci_hi3660_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
+{
+	int i = 0;
+	struct dw_mci *host = slot->host;
+	struct mmc_host *mmc = slot->mmc;
+	int smpl_phase = 0;
+	u32 tuning_sample_flag = 0;
+	int best_clksmpl = 0;
+
+	for (i = 0; i < NUM_PHASES; ++i, ++smpl_phase) {
+		smpl_phase %= 32;
+
+		mci_writel(host, TMOUT, ~0);
+		dw_mci_hs_set_timing(host, mmc->ios.timing, smpl_phase);
+
+		if (!mmc_send_tuning(mmc, opcode, NULL))
+			tuning_sample_flag |= (1 << smpl_phase);
+		else
+			tuning_sample_flag &= ~(1 << smpl_phase);
+	}
+
+	best_clksmpl = dw_mci_get_best_clksmpl(tuning_sample_flag);
+	if (best_clksmpl < 0) {
+		dev_err(host->dev, "All phases bad!\n");
+		return -EIO;
+	}
+
+	dw_mci_hs_set_timing(host, mmc->ios.timing, best_clksmpl);
+
+	dev_info(host->dev, "tuning ok best_clksmpl %u tuning_sample_flag %x\n",
+		 best_clksmpl, tuning_sample_flag);
+	return 0;
+}
+
+static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
+					struct mmc_ios *ios)
+{
+	int ret;
+	int min_uv = 0;
+	int max_uv = 0;
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct k3_priv *priv;
+	struct dw_mci *host;
+
+	host = slot->host;
+	priv = host->priv;
+
+	if (!priv || !priv->reg)
+		return 0;
+
+	if (priv->ctrl_id == DWMMC_SDIO_ID)
+		return 0;
+
+	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
+		ret = dw_mci_set_sel18(host, 0);
+		if (ret)
+			return ret;
+		min_uv = 2950000;
+		max_uv = 2950000;
+	} else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
+		ret = dw_mci_set_sel18(host, 1);
+		if (ret)
+			return ret;
+		min_uv = 1800000;
+		max_uv = 1800000;
+	}
+
+	if (IS_ERR_OR_NULL(mmc->supply.vqmmc))
+		return 0;
+
+	ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
+	if (ret) {
+		dev_err(host->dev, "Regulator set error %d: %d - %d\n",
+			ret, min_uv, max_uv);
+		return ret;
+	}
+	return 0;
+}
+
+static const struct dw_mci_drv_data hi3660_data = {
+	.init = dw_mci_hi3660_init,
+	.set_ios = dw_mci_hi3660_set_ios,
+	.parse_dt = dw_mci_hi6220_parse_dt,
+	.execute_tuning = dw_mci_hi3660_execute_tuning,
+	.switch_voltage  = dw_mci_hi3660_switch_voltage,
+};
+
 static const struct of_device_id dw_mci_k3_match[] = {
+	{ .compatible = "hisilicon,hi3660-dw-mshc", .data = &hi3660_data, },
 	{ .compatible = "hisilicon,hi4511-dw-mshc", .data = &k3_drv_data, },
 	{ .compatible = "hisilicon,hi6220-dw-mshc", .data = &hi6220_data, },
 	{},
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 75da3756955d..5403758bf621 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -314,6 +314,8 @@ struct dw_mci_board {
 #define SDMMC_DSCADDR		0x094
 #define SDMMC_BUFADDR		0x098
 #define SDMMC_CDTHRCTL		0x100
+#define SDMMC_UHS_REG_EXT	0x108
+#define SDMMC_ENABLE_SHIFT	0x110
 #define SDMMC_DATA(x)		(x)
 /*
 * Registers to support idmac 64-bit address mode
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1706342 — Re: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-08-08 13:10 +0200
SubjectRe: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660
Message-ID<ucayD-7uw-39@gated-at.bofh.it>
In reply to#1701666
On 2 August 2017 at 05:17, Li Wei <liwei213@huawei.com> wrote:
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei <liwei213@huawei.com>
> Signed-off-by: Chen Jun <chenjun14@huawei.com>
>
> Major changes in v3:
>  - solve review comments from Heiner Kallweit.
>    *use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>    *use usleep_range() replace udelay() and mdelay().
>
> Major changes in v4:
>  - solve review comments from Jaehoon Chung.
>    *move common register for dwmmc controller to dwmmc header file.
>    *modify definitions type of some register variables.
>    *get rid of the magic numbers.
>
> Major changes in v5:
>  - further improve coding style.
>
> Major changes in v6:
>  - solve review comments for Jaehoon Chung.
>    *modify dw_mci_hi3660_set_ios() to static.
>    *fix the comment style.
>
> Major changes in v7:
>  - solve review comments for John Stultz.
>    *remove reset code in dw_mmc-k3.c,use reset in core mmc.
>
> Major changes in v8:
>  - modify patch v7 name and dependency order.

Version history is really great information, however it doesn't belong
inside the change log itsefl. Instead add "---" and a newline here,
then put it all below that.

[...]

> +static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
> +                                       struct mmc_ios *ios)
> +{
> +       int ret;
> +       int min_uv = 0;
> +       int max_uv = 0;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
> +       struct k3_priv *priv;
> +       struct dw_mci *host;
> +
> +       host = slot->host;
> +       priv = host->priv;
> +
> +       if (!priv || !priv->reg)
> +               return 0;
> +
> +       if (priv->ctrl_id == DWMMC_SDIO_ID)
> +               return 0;
> +
> +       if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> +               ret = dw_mci_set_sel18(host, 0);
> +               if (ret)
> +                       return ret;
> +               min_uv = 2950000;
> +               max_uv = 2950000;
> +       } else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> +               ret = dw_mci_set_sel18(host, 1);
> +               if (ret)
> +                       return ret;
> +               min_uv = 1800000;
> +               max_uv = 1800000;
> +       }
> +
> +       if (IS_ERR_OR_NULL(mmc->supply.vqmmc))
> +               return 0;
> +
> +       ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);

We have an API, mmc_regulator_set_vqmmc(), that you probably should
use here instead.

> +       if (ret) {
> +               dev_err(host->dev, "Regulator set error %d: %d - %d\n",
> +                       ret, min_uv, max_uv);
> +               return ret;
> +       }
> +       return 0;
> +}
> +
> +static const struct dw_mci_drv_data hi3660_data = {
> +       .init = dw_mci_hi3660_init,
> +       .set_ios = dw_mci_hi3660_set_ios,
> +       .parse_dt = dw_mci_hi6220_parse_dt,
> +       .execute_tuning = dw_mci_hi3660_execute_tuning,
> +       .switch_voltage  = dw_mci_hi3660_switch_voltage,
> +};
> +
>  static const struct of_device_id dw_mci_k3_match[] = {
> +       { .compatible = "hisilicon,hi3660-dw-mshc", .data = &hi3660_data, },
>         { .compatible = "hisilicon,hi4511-dw-mshc", .data = &k3_drv_data, },
>         { .compatible = "hisilicon,hi6220-dw-mshc", .data = &hi6220_data, },
>         {},
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 75da3756955d..5403758bf621 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -314,6 +314,8 @@ struct dw_mci_board {
>  #define SDMMC_DSCADDR          0x094
>  #define SDMMC_BUFADDR          0x098
>  #define SDMMC_CDTHRCTL         0x100
> +#define SDMMC_UHS_REG_EXT      0x108
> +#define SDMMC_ENABLE_SHIFT     0x110
>  #define SDMMC_DATA(x)          (x)
>  /*
>  * Registers to support idmac 64-bit address mode
> --
> 2.11.0
>

Overall the code looks okay to me, however I am wondering about the
relationship with the original k3 driver code and hi6220 code. Almost
no code is being re-used between the different variants. Why is that?

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1707028 — 答复: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660

From"liwei (CM)" <liwei213@huawei.com>
Date2017-08-09 05:10 +0200
Subject答复: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660
Message-ID<ucpxD-16N-5@gated-at.bofh.it>
In reply to#1706342
Hi, Ulf
Thank you very much for your advice.
1. Version history is really great information, however it doesn't belong inside the change log itsefl. Instead add "---" and a newline here, then put it all below that.
【LiWei】OK，I will fix it;

2. We have an API, mmc_regulator_set_vqmmc(), that you probably should use here instead.
【LiWei】Yes, you are right.I use mmc_regulator_set_vqmmc() instead and test OK, I will modify it in patch v9;

3. Overall the code looks okay to me, however I am wondering about the relationship with the original k3 driver code and hi6220 code. Almost no code is being re-used between the different variants. Why is that?
【LiWei】So far, it seems that dw_mci_hi3660_set_ios() and dw_mci_hi3660_switch_voltage() are not re-used, I think the main reason is that Hi3660 support uhs-sdr12/-uhs-sdr25/uhs-sdr50/uhs-sdr104, On the other hand, 
we have custom register settings, such as:
#define AO_SCTRL_SEL18		BIT(10)
#define AO_SCTRL_CTRL3		0x40C
#define SOC_SCTRL_SCPERCTRL5    (0x314)
#define SDCARD_IO_SEL18         BIT(2)
So we can't merge relevant code.

Please help review patch v9, thank you!

-----邮件原件-----
发件人: Ulf Hansson [mailto:ulf.hansson@linaro.org] 
发送时间: 2017年8月8日 19:04
收件人: liwei (CM)
抄送: Adrian Hunter; Jaehoon Chung; Shawn Lin; Wolfram Sang; Heiner Kallweit; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Guodong Xu
主题: Re: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660

On 2 August 2017 at 05:17, Li Wei <liwei213@huawei.com> wrote:
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei <liwei213@huawei.com>
> Signed-off-by: Chen Jun <chenjun14@huawei.com>
>
> Major changes in v3:
>  - solve review comments from Heiner Kallweit.
>    *use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>    *use usleep_range() replace udelay() and mdelay().
>
> Major changes in v4:
>  - solve review comments from Jaehoon Chung.
>    *move common register for dwmmc controller to dwmmc header file.
>    *modify definitions type of some register variables.
>    *get rid of the magic numbers.
>
> Major changes in v5:
>  - further improve coding style.
>
> Major changes in v6:
>  - solve review comments for Jaehoon Chung.
>    *modify dw_mci_hi3660_set_ios() to static.
>    *fix the comment style.
>
> Major changes in v7:
>  - solve review comments for John Stultz.
>    *remove reset code in dw_mmc-k3.c,use reset in core mmc.
>
> Major changes in v8:
>  - modify patch v7 name and dependency order.

Version history is really great information, however it doesn't belong inside the change log itsefl. Instead add "---" and a newline here, then put it all below that.

[...]

> +static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
> +                                       struct mmc_ios *ios) {
> +       int ret;
> +       int min_uv = 0;
> +       int max_uv = 0;
> +       struct dw_mci_slot *slot = mmc_priv(mmc);
> +       struct k3_priv *priv;
> +       struct dw_mci *host;
> +
> +       host = slot->host;
> +       priv = host->priv;
> +
> +       if (!priv || !priv->reg)
> +               return 0;
> +
> +       if (priv->ctrl_id == DWMMC_SDIO_ID)
> +               return 0;
> +
> +       if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> +               ret = dw_mci_set_sel18(host, 0);
> +               if (ret)
> +                       return ret;
> +               min_uv = 2950000;
> +               max_uv = 2950000;
> +       } else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> +               ret = dw_mci_set_sel18(host, 1);
> +               if (ret)
> +                       return ret;
> +               min_uv = 1800000;
> +               max_uv = 1800000;
> +       }
> +
> +       if (IS_ERR_OR_NULL(mmc->supply.vqmmc))
> +               return 0;
> +
> +       ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, 
> + max_uv);

We have an API, mmc_regulator_set_vqmmc(), that you probably should use here instead.

> +       if (ret) {
> +               dev_err(host->dev, "Regulator set error %d: %d - %d\n",
> +                       ret, min_uv, max_uv);
> +               return ret;
> +       }
> +       return 0;
> +}
> +
> +static const struct dw_mci_drv_data hi3660_data = {
> +       .init = dw_mci_hi3660_init,
> +       .set_ios = dw_mci_hi3660_set_ios,
> +       .parse_dt = dw_mci_hi6220_parse_dt,
> +       .execute_tuning = dw_mci_hi3660_execute_tuning,
> +       .switch_voltage  = dw_mci_hi3660_switch_voltage, };
> +
>  static const struct of_device_id dw_mci_k3_match[] = {
> +       { .compatible = "hisilicon,hi3660-dw-mshc", .data = 
> + &hi3660_data, },
>         { .compatible = "hisilicon,hi4511-dw-mshc", .data = &k3_drv_data, },
>         { .compatible = "hisilicon,hi6220-dw-mshc", .data = &hi6220_data, },
>         {},
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h 
> index 75da3756955d..5403758bf621 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -314,6 +314,8 @@ struct dw_mci_board {
>  #define SDMMC_DSCADDR          0x094
>  #define SDMMC_BUFADDR          0x098
>  #define SDMMC_CDTHRCTL         0x100
> +#define SDMMC_UHS_REG_EXT      0x108
> +#define SDMMC_ENABLE_SHIFT     0x110
>  #define SDMMC_DATA(x)          (x)
>  /*
>  * Registers to support idmac 64-bit address mode
> --
> 2.11.0
>

Overall the code looks okay to me, however I am wondering about the relationship with the original k3 driver code and hi6220 code. Almost no code is being re-used between the different variants. Why is that?

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1706321

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-08-08 12:50 +0200
Message-ID<ucafg-767-17@gated-at.bofh.it>
In reply to#1701661
On 2 August 2017 at 05:17, Li Wei <liwei213@huawei.com> wrote:
> This commit modifies dw_mci_probe(), it moves reset assertion before
> drv_data->init(host)
>
> Some driver needs to access controller registers in its .init() ops. So,
> in order to make such access safe, we should do controller reset before
> .init() being called.
>
> Signed-off-by: Wei Li <liwei213@huawei.com>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: Chen Jun <chenjun14@huawei.com>

This looks good to me. However, I need an ack from Jaehoon to pick this up.

Kind regards
Uffe

> ---
>  drivers/mmc/host/dw_mmc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index a9dfb26972f2..f2fa928e1a12 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -3067,6 +3067,12 @@ int dw_mci_probe(struct dw_mci *host)
>                 goto err_clk_ciu;
>         }
>
> +       if (!IS_ERR(host->pdata->rstc)) {
> +               reset_control_assert(host->pdata->rstc);
> +               usleep_range(10, 50);
> +               reset_control_deassert(host->pdata->rstc);
> +       }
> +
>         if (drv_data && drv_data->init) {
>                 ret = drv_data->init(host);
>                 if (ret) {
> @@ -3076,12 +3082,6 @@ int dw_mci_probe(struct dw_mci *host)
>                 }
>         }
>
> -       if (!IS_ERR(host->pdata->rstc)) {
> -               reset_control_assert(host->pdata->rstc);
> -               usleep_range(10, 50);
> -               reset_control_deassert(host->pdata->rstc);
> -       }
> -
>         setup_timer(&host->cmd11_timer,
>                     dw_mci_cmd11_timer, (unsigned long)host);
>
> --
> 2.11.0
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web