答复: [PATCH] mmc:dw_mmc-k3: add sd support for hi3660

2017-06-12 Thread liwei (CM) 
Hi, Heiner

Sorry late for reply you, thank you very much for your advice. 
My changes are as follows and I will send the PATCH-V3 later.


-邮件原件-
发件人: Heiner Kallweit [mailto:hkallwe...@gmail.com] 
发送时间: 2017年5月17日 5:52
收件人: liwei (CM); ulf.hans...@linaro.org; adrian.hun...@intel.com; 
jh80.ch...@samsung.com; shawn@rock-chips.com; 
wsa+rene...@sang-engineering.com; linux-...@vger.kernel.org; 
linux-kernel@vger.kernel.org
抄送: guodong...@linaro.org
主题: Re: [PATCH] mmc:dw_mmc-k3: add sd support for hi3660

Am 16.05.2017 um 14:26 schrieb liwei:
> Add sd card support for hi3660 soc
> 
> Signed-off-by: Li Wei 
> Signed-off-by: Chen Jun 
> ---
>  drivers/mmc/host/dw_mmc-k3.c | 311 
> +++
>  1 file changed, 311 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc-k3.c 
> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..59d6e0870fb1 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -8,6 +8,7 @@
>   * (at your option) any later version.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -28,7 +29,44 @@
>  #define AO_SCTRL_SEL18   BIT(10)
>  #define AO_SCTRL_CTRL3   0x40C
>  
> +#define DWMMC_SD_ID   1
> +#define DWMMC_SDIO_ID 2
> +
> +#define SOC_SCTRL_SCPERCTRL5(0x314)
> +#define SDCARD_IO_SEL18 BIT(2)
> +
> +#define GENCLK_DIV (7)
> +
> +#define GPIO_CLK_ENABLE BIT(16)
> +#define GPIO_CLK_DIV(x) (((x) & 0xf) << 8)
> +#define GPIO_USE_SAMPLE_DLY(x)  (((x) & 0x1) << 13)
> +#define UHS_REG_EXT_SAMPLE_PHASE(x) (((x) & 0x1f) << 16)
> +#define UHS_REG_EXT_SAMPLE_DLY(x)   (((x) & 0x1f) << 26)
> +#define UHS_REG_EXT_SAMPLE_DRVPHASE(x)  (((x) & 0x1f) << 21)

Using the GENMASK and FIELD_PREP macros may be a good alternative here,
e.g.:
#define GPIO_CLK_DIV_MASK GENMASK(11, 8) Then in the code use 
FIELD_PREP(GPIO_CLK_DIV_MASK, x)

And the bit field definitions should follow the register defines, e.g.:

#define REG_1 0x00
#define   REG_1_FIELD_1 GENMASK(a, b)
#define   REG_1_FIELD_2 GENMASK(c, d)

This makes it easier for people checking the code against the chip spec.
【liwei】I'll fix this issue and update the patch;

> +#define SDMMC_UHS_REG_EXT_VALUE(x, y, z) (UHS_REG_EXT_SAMPLE_PHASE(x) |\
> +   UHS_REG_EXT_SAMPLE_DLY(y) |\
> +   UHS_REG_EXT_SAMPLE_DRVPHASE(z)) 
> #define SDMMC_GPIO_VALUE(x, y) 
> +(GPIO_CLK_DIV(x) | GPIO_USE_SAMPLE_DLY(y))

Both macros are used only once. So are they actually needed?
【liwei】Yes, they are not needed, I'll update the patch;

> +
> +#define SDMMC_UHS_REG_EXT0x108
> +#define SDMMC_ENABLE_SHIFT   0x110
> +
> +#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)
> +
>  struct k3_priv {
> + u8 ctrl_id;
> + u32 cur_speed;
>   struct regmap   *reg;
>  };
>  
> @@ -38,6 +76,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
>   0
>  };
>  
> +struct hs_timing {
> + int drv_phase;
> + int sam_dly;
> + int sam_phase_max;
> + int sam_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 */
> + }
> +};
> +
>  static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios 
> *ios)  {
>   int ret;
> @@ -66,6 +139,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;
>  }
> @@

答复: [PATCH] mmc:dw_mmc-k3: add sd support for hi3660

2017-06-12 Thread liwei (CM) 
Hi, Heiner

Sorry late for reply you, thank you very much for your advice. 
My changes are as follows and I will send the PATCH-V3 later.


-邮件原件-
发件人: Heiner Kallweit [mailto:hkallwe...@gmail.com] 
发送时间: 2017年5月17日 5:52
收件人: liwei (CM); ulf.hans...@linaro.org; adrian.hun...@intel.com; 
jh80.ch...@samsung.com; shawn@rock-chips.com; 
wsa+rene...@sang-engineering.com; linux-...@vger.kernel.org; 
linux-kernel@vger.kernel.org
抄送: guodong...@linaro.org
主题: Re: [PATCH] mmc:dw_mmc-k3: add sd support for hi3660

Am 16.05.2017 um 14:26 schrieb liwei:
> Add sd card support for hi3660 soc
> 
> Signed-off-by: Li Wei 
> Signed-off-by: Chen Jun 
> ---
>  drivers/mmc/host/dw_mmc-k3.c | 311 
> +++
>  1 file changed, 311 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc-k3.c 
> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..59d6e0870fb1 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -8,6 +8,7 @@
>   * (at your option) any later version.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -28,7 +29,44 @@
>  #define AO_SCTRL_SEL18   BIT(10)
>  #define AO_SCTRL_CTRL3   0x40C
>  
> +#define DWMMC_SD_ID   1
> +#define DWMMC_SDIO_ID 2
> +
> +#define SOC_SCTRL_SCPERCTRL5(0x314)
> +#define SDCARD_IO_SEL18 BIT(2)
> +
> +#define GENCLK_DIV (7)
> +
> +#define GPIO_CLK_ENABLE BIT(16)
> +#define GPIO_CLK_DIV(x) (((x) & 0xf) << 8)
> +#define GPIO_USE_SAMPLE_DLY(x)  (((x) & 0x1) << 13)
> +#define UHS_REG_EXT_SAMPLE_PHASE(x) (((x) & 0x1f) << 16)
> +#define UHS_REG_EXT_SAMPLE_DLY(x)   (((x) & 0x1f) << 26)
> +#define UHS_REG_EXT_SAMPLE_DRVPHASE(x)  (((x) & 0x1f) << 21)

Using the GENMASK and FIELD_PREP macros may be a good alternative here,
e.g.:
#define GPIO_CLK_DIV_MASK GENMASK(11, 8) Then in the code use 
FIELD_PREP(GPIO_CLK_DIV_MASK, x)

And the bit field definitions should follow the register defines, e.g.:

#define REG_1 0x00
#define   REG_1_FIELD_1 GENMASK(a, b)
#define   REG_1_FIELD_2 GENMASK(c, d)

This makes it easier for people checking the code against the chip spec.
【liwei】I'll fix this issue and update the patch;

> +#define SDMMC_UHS_REG_EXT_VALUE(x, y, z) (UHS_REG_EXT_SAMPLE_PHASE(x) |\
> +   UHS_REG_EXT_SAMPLE_DLY(y) |\
> +   UHS_REG_EXT_SAMPLE_DRVPHASE(z)) 
> #define SDMMC_GPIO_VALUE(x, y) 
> +(GPIO_CLK_DIV(x) | GPIO_USE_SAMPLE_DLY(y))

Both macros are used only once. So are they actually needed?
【liwei】Yes, they are not needed, I'll update the patch;

> +
> +#define SDMMC_UHS_REG_EXT0x108
> +#define SDMMC_ENABLE_SHIFT   0x110
> +
> +#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)
> +
>  struct k3_priv {
> + u8 ctrl_id;
> + u32 cur_speed;
>   struct regmap   *reg;
>  };
>  
> @@ -38,6 +76,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
>   0
>  };
>  
> +struct hs_timing {
> + int drv_phase;
> + int sam_dly;
> + int sam_phase_max;
> + int sam_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 */
> + }
> +};
> +
>  static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios 
> *ios)  {
>   int ret;
> @@ -66,6 +139,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 +221,236 @@ static const struct