| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1699CE87DE933F49876AD744B5DC140FA0F4D9@DGGEMM506-MBS.china.huawei.com>
Date: Wed, 9 Aug 2017 03:04:55 +0000
From: "liwei (CM)" <liwei213@...wei.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Adrian Hunter <adrian.hunter@...el.com>,
Jaehoon Chung <jh80.chung@...sung.com>,
Shawn Lin <shawn.lin@...k-chips.com>,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
Heiner Kallweit <hkallweit1@...il.com>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Guodong Xu <guodong.xu@...aro.org>
Subject: 答复: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660
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@...aro.org]
发送时间: 2017年8月8日 19:04
收件人: liwei (CM)
抄送: Adrian Hunter; Jaehoon Chung; Shawn Lin; Wolfram Sang; Heiner Kallweit; linux-mmc@...r.kernel.org; linux-kernel@...r.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@...wei.com> wrote:
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei <liwei213@...wei.com>
> Signed-off-by: Chen Jun <chenjun14@...wei.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
Powered by blists - more mailing lists