Messages in this thread Patch in this message | | | From | "liwei (CM)" <> | Subject | 答复: [PATCH v8 2/2] mmc: dw mmc-k3: add sd support for hi3660 | Date | Wed, 9 Aug 2017 03:04:55 +0000 |
| |
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
| |