Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752576AbdHIDFZ (ORCPT ); Tue, 8 Aug 2017 23:05:25 -0400 Received: from szxga03-in.huawei.com ([45.249.212.189]:10401 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752265AbdHIDFY (ORCPT ); Tue, 8 Aug 2017 23:05:24 -0400 From: "liwei (CM)" To: Ulf Hansson CC: Adrian Hunter , Jaehoon Chung , Shawn Lin , Wolfram Sang , Heiner Kallweit , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Guodong Xu Subject: =?utf-8?B?562U5aSNOiBbUEFUQ0ggdjggMi8yXSBtbWM6IGR3X21tYy1rMzogYWRkIHNk?= =?utf-8?Q?_support_for_hi3660?= Thread-Topic: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660 Thread-Index: AQHTEDYHyboEIR1lkUy9fH4daIry56J7U/Rw Date: Wed, 9 Aug 2017 03:04:55 +0000 Message-ID: <1699CE87DE933F49876AD744B5DC140FA0F4D9@DGGEMM506-MBS.china.huawei.com> References: <20170802031742.60363-1-liwei213@huawei.com> <20170802031742.60363-2-liwei213@huawei.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.189.155.72] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020202.598A7BDD.00CC,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=169.254.4.222, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: b26d76f4e35521eab8a0b92a95ddcd1d Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v7935VUx018397 Content-Length: 5621 Lines: 144 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 wrote: > Add sd card support for hi3660 soc > > Signed-off-by: Li Wei > Signed-off-by: Chen Jun > > 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