Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752444AbdHJApw (ORCPT ); Wed, 9 Aug 2017 20:45:52 -0400 Received: from lucky1.263xmail.com ([211.157.147.130]:51134 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752094AbdHJApu (ORCPT ); Wed, 9 Aug 2017 20:45:50 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: shawn.lin@rock-chips.com X-FST-TO: fengbaopeng@hisilicon.com X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: shawn.lin@rock-chips.com X-UNIQUE-TAG: <4a38507c8f7924693f2bf8526c95abe5> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Cc: shawn.lin@rock-chips.com, "adrian.hunter@intel.com" , "guodong.xu@linaro.org" , "Fengbaopeng (kevin, Kirin Solution Dept)" Subject: =?UTF-8?Q?Re:_=e7=ad=94=e5=a4=8d:_[PATCH_v9_2/2]_mmc:_dw=5fmmc-k3:_?= =?UTF-8?Q?add_sd_support_for_hi3660?= To: "liwei (CM)" , "ulf.hansson@linaro.org" , "jh80.chung@samsung.com" , "wsa+renesas@sang-engineering.com" , "hkallweit1@gmail.com" , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20170809032519.96219-1-liwei213@huawei.com> <20170809032519.96219-2-liwei213@huawei.com> <1699CE87DE933F49876AD744B5DC140FA0F515@DGGEMM506-MBS.china.huawei.com> From: Shawn Lin Message-ID: <69845fb9-ccef-9172-321d-f0c319eaea17@rock-chips.com> Date: Thu, 10 Aug 2017 08:45:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1699CE87DE933F49876AD744B5DC140FA0F515@DGGEMM506-MBS.china.huawei.com> Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14900 Lines: 473 On 2017/8/9 14:56, liwei (CM) wrote: > Hi, Shawn > Thank you for your patience and carefulness, I'm sorry maybe I miss your comments. > 1.for the changelog,do you me as following: > ??liwei??Major changes in v8: > - modify patch v7 name and dependency order. > > Major changes in v9: > - solve review comments for Ulf Hansson. > *use mmc_regulator_set_vqmmc() instead of regulator_set_voltage(). > Firstly send the patch to yourself and check it in your inbox and you could see that there is already a '------' between your commit msg and the files you change. So inline your changelog there. > --------------------------------------------------------------------- > 2. some of this macro newly added aren't used anywhere. > ??liwei??yes, PULL_DOWN BIT(1) / PULL_UP BIT(0) aren't used anywhere, I will remove them in patch v10 > > 3. > +#define SDMMC_UHS_REG_EXT 0x108 >> +#define SDMMC_ENABLE_SHIFT 0x110 > ??liwei??They modified at patch v4, I move common register for dwmmc controller to dwmmc header file, and they are used as following in dw_mci_hs_set_timing() > mci_writel(host, UHS_REG_EXT, reg_value); > mci_writel(host, ENABLE_SHIFT, enable_shift); > > 4. Does it work for hard-coding the delay timing? All the different boards with different cards work fine? > And tuning process is also work for SDR50 although it's optional. So how would that work for your case? > ??liwei??Yes, they work fine with different boards with different cards, we tested it. In fact, these values are the generally appropriate values given by our controller chip colleagues. > For SDR50 it will not come into dw_mci_hi3660_execute_tuning() and use the given value in hs_timing_cfg[][]{} when dw_mci_hi3660_init(); > > 5. > + for (i = 0; i < NUM_PHASES; ++i, ++smpl_phase) { >> + smpl_phase %= 32; >> + >> + mci_writel(host, TMOUT, ~0); > > Why you need this? > ??liwei??This is what SDR104 needs to find the optimum phase point. I meant I didn't get the point of why you set the TMOUT here. Tips: For further wise, please don't top post and inline your answer in the previous mail so that we could better trace down the issue we are talking about. > > Thank you again and look forward to your reply! > > -----?ʼ?ԭ??----- > ??????: Shawn Lin [mailto:shawn.lin@rock-chips.com] > ????ʱ??: 2017??8??9?? 11:48 > ?ռ???: liwei (CM); ulf.hansson@linaro.org; jh80.chung@samsung.com; wsa+renesas@sang-engineering.com; hkallweit1@gmail.com; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org > ????: adrian.hunter@intel.com; shawn.lin@rock-chips.com; guodong.xu@linaro.org > ????: Re: [PATCH v9 2/2] mmc: dw_mmc-k3: add sd support for hi3660 > > > On 2017/8/9 11:25, Li Wei wrote: >> Add sd card support for hi3660 soc >> >> Signed-off-by: Li Wei >> Signed-off-by: Chen Jun >> > > I did some comment for your v6 but probably you miss them. > > And it's still incorrect for your changelog..... > >> -------------------------------------------------- >> 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. >> >> Major changes in v9: >> - solve review comments for Ulf Hansson. >> *use mmc_regulator_set_vqmmc() instead of regulator_set_voltage(). >> --- > > should be here... > >> drivers/mmc/host/dw_mmc-k3.c | 301 +++++++++++++++++++++++++++++++++++++++++++ >> drivers/mmc/host/dw_mmc.h | 2 + >> 2 files changed, 303 insertions(+) >> >> diff --git a/drivers/mmc/host/dw_mmc-k3.c >> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..f6910bed55ef 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 >> +#include >> #include >> #include >> #include >> @@ -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) >> + > > I said some of this macro newly added aren't used anywhere. > >> 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 */ >> + } >> +}; >> + > > Does it work for hard-coding the delay timing? All the different boards with different cards work fine? > > And tuning process is also work for SDR50 although it's optional. So how would that work for your case? > >> 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,236 @@ 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); > > Why you need this? > >> + 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; >> + 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); >> + else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) >> + ret = dw_mci_set_sel18(host, 1); >> + if (ret) >> + return ret; >> + >> + if (!IS_ERR(mmc->supply.vqmmc)) { >> + ret = mmc_regulator_set_vqmmc(mmc, ios); >> + if (ret) { >> + dev_err(host->dev, "Regulator set error %d\n", ret); >> + 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 > > where you use these two? > >> #define SDMMC_DATA(x) (x) >> /* >> * Registers to support idmac 64-bit address mode >> >