çå: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660

[liwei (CM)]

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@xxxxxxxxxx] 
åéæé: 2017å8æ8æ 19:04
æää: liwei (CM)
æé: Adrian Hunter; Jaehoon Chung; Shawn Lin; Wolfram Sang; Heiner Kallweit; linux-mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxxx> wrote:
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei <liwei213@xxxxxxxxxx>
> Signed-off-by: Chen Jun <chenjun14@xxxxxxxxxx>
>
> 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