[PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[Guodong Xu]

Add resets property to synopsys-dw-mshc bindings. It is intended to
represent the hardware reset signal present internally in some host
controller IC designs.

See Documentation/devicetree/bindings/reset/reset.txt for details.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
index 8636f5a..4e00e85 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
@@ -39,6 +39,10 @@ Required Properties:
 
 Optional properties:
 
+* resets: phandle + reset specifier pair, intended to represent hardware
+  reset signal present internally in some host controller IC designs.
+  See Documentation/devicetree/bindings/reset/reset.txt for details.
+
 * clocks: from common clock binding: handle to biu and ciu clocks for the
   bus interface unit clock and the card interface unit clock.
 
-- 
1.9.1

[PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Guodong Xu]

mmc registers may in abnormal state if mmc is used in bootloader,
eg. to support booting from eMMC. So we need reset mmc registers
when kernel boots up, instead of assuming mmc is in clean state.

With this patch, user can add a 'resets' property into dw_mmc dts
node. When driver parse_dt and probe, it calls reset API to
deassert the 'reset' of dw_mmc host controller. When probe error or
remove, it calls reset API to assert it.

Please also refer to Documentation/devicetree/bindings/reset/reset.txt

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 242f9a0..281ea9c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2878,6 +2878,14 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
+	/* find reset controller when exist */
+	pdata->rstc = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(pdata->rstc)) {
+		if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
+			return ERR_PTR(-EPROBE_DEFER);
+		pdata->rstc = NULL;
+	}
+
 	/* find out number of slots supported */
 	of_property_read_u32(np, "num-slots", &pdata->num_slots);
 
@@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host)
 
 	if (!host->pdata) {
 		host->pdata = dw_mci_parse_dt(host);
-		if (IS_ERR(host->pdata)) {
+		if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		else if (IS_ERR(host->pdata)) {
 			dev_err(host->dev, "platform data not available\n");
 			return -EINVAL;
 		}
@@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
 		}
 	}
 
+	if (host->pdata->rstc != NULL)
+		reset_control_deassert(host->pdata->rstc);
+
 	setup_timer(&host->cmd11_timer,
 		    dw_mci_cmd11_timer, (unsigned long)host);
 
@@ -3164,6 +3177,9 @@ err_dmaunmap:
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	if (host->pdata->rstc != NULL)
+		reset_control_assert(host->pdata->rstc);
+
 err_clk_ciu:
 	if (!IS_ERR(host->ciu_clk))
 		clk_disable_unprepare(host->ciu_clk);
@@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host)
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	if (host->pdata->rstc != NULL)
+		reset_control_assert(host->pdata->rstc);
+
 	if (!IS_ERR(host->ciu_clk))
 		clk_disable_unprepare(host->ciu_clk);
 
 	if (!IS_ERR(host->biu_clk))
 		clk_disable_unprepare(host->biu_clk);
+
 }
 EXPORT_SYMBOL(dw_mci_remove);
 
-- 
1.9.1

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8268 bytes --]

Hi Guodong,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.5-rc6 next-20160304]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Guodong-Xu/Documentation-synopsys-dw-mshc-add-binding-for-resets/20160306-164955
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: sparc64-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All errors (new ones prefixed by >>):

   drivers/mmc/host/dw_mmc.c: In function 'dw_mci_parse_dt':
>> drivers/mmc/host/dw_mmc.c:2882:7: error: 'struct dw_mci_board' has no member named 'rstc'
     pdata->rstc = devm_reset_control_get_optional(dev, NULL);
          ^
>> drivers/mmc/host/dw_mmc.c:2882:2: error: implicit declaration of function 'devm_reset_control_get_optional' [-Werror=implicit-function-declaration]
     pdata->rstc = devm_reset_control_get_optional(dev, NULL);
     ^
   drivers/mmc/host/dw_mmc.c:2883:18: error: 'struct dw_mci_board' has no member named 'rstc'
     if (IS_ERR(pdata->rstc)) {
                     ^
   drivers/mmc/host/dw_mmc.c:2884:20: error: 'struct dw_mci_board' has no member named 'rstc'
      if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
                       ^
   drivers/mmc/host/dw_mmc.c:2886:8: error: 'struct dw_mci_board' has no member named 'rstc'
      pdata->rstc = NULL;
           ^
   drivers/mmc/host/dw_mmc.c: In function 'dw_mci_probe':
   drivers/mmc/host/dw_mmc.c:3025:17: error: 'struct dw_mci_board' has no member named 'rstc'
     if (host->pdata->rstc != NULL)
                    ^
>> drivers/mmc/host/dw_mmc.c:3026:3: error: implicit declaration of function 'reset_control_deassert' [-Werror=implicit-function-declaration]
      reset_control_deassert(host->pdata->rstc);
      ^
   drivers/mmc/host/dw_mmc.c:3026:37: error: 'struct dw_mci_board' has no member named 'rstc'
      reset_control_deassert(host->pdata->rstc);
                                        ^
   drivers/mmc/host/dw_mmc.c:3180:17: error: 'struct dw_mci_board' has no member named 'rstc'
     if (host->pdata->rstc != NULL)
                    ^
>> drivers/mmc/host/dw_mmc.c:3181:3: error: implicit declaration of function 'reset_control_assert' [-Werror=implicit-function-declaration]
      reset_control_assert(host->pdata->rstc);
      ^
   drivers/mmc/host/dw_mmc.c:3181:35: error: 'struct dw_mci_board' has no member named 'rstc'
      reset_control_assert(host->pdata->rstc);
                                      ^
   drivers/mmc/host/dw_mmc.c: In function 'dw_mci_remove':
   drivers/mmc/host/dw_mmc.c:3215:17: error: 'struct dw_mci_board' has no member named 'rstc'
     if (host->pdata->rstc != NULL)
                    ^
   drivers/mmc/host/dw_mmc.c:3216:35: error: 'struct dw_mci_board' has no member named 'rstc'
      reset_control_assert(host->pdata->rstc);
                                      ^
   cc1: some warnings being treated as errors

vim +2882 drivers/mmc/host/dw_mmc.c

  2876	
  2877		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
  2878		if (!pdata)
  2879			return ERR_PTR(-ENOMEM);
  2880	
  2881		/* find reset controller when exist */
> 2882		pdata->rstc = devm_reset_control_get_optional(dev, NULL);
  2883		if (IS_ERR(pdata->rstc)) {
  2884			if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
  2885				return ERR_PTR(-EPROBE_DEFER);
> 2886			pdata->rstc = NULL;
  2887		}
  2888	
  2889		/* find out number of slots supported */
  2890		of_property_read_u32(np, "num-slots", &pdata->num_slots);
  2891	
  2892		if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
  2893			dev_info(dev,
  2894				 "fifo-depth property not found, using value of FIFOTH register as default\n");
  2895	
  2896		of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
  2897	
  2898		if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
  2899			pdata->bus_hz = clock_frequency;
  2900	
  2901		if (drv_data && drv_data->parse_dt) {
  2902			ret = drv_data->parse_dt(host);
  2903			if (ret)
  2904				return ERR_PTR(ret);
  2905		}
  2906	
  2907		if (of_find_property(np, "supports-highspeed", NULL)) {
  2908			dev_info(dev, "supports-highspeed property is deprecated.\n");
  2909			pdata->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
  2910		}
  2911	
  2912		return pdata;
  2913	}
  2914	
  2915	#else /* CONFIG_OF */
  2916	static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
  2917	{
  2918		return ERR_PTR(-EINVAL);
  2919	}
  2920	#endif /* CONFIG_OF */
  2921	
  2922	static void dw_mci_enable_cd(struct dw_mci *host)
  2923	{
  2924		unsigned long irqflags;
  2925		u32 temp;
  2926		int i;
  2927		struct dw_mci_slot *slot;
  2928	
  2929		/*
  2930		 * No need for CD if all slots have a non-error GPIO
  2931		 * as well as broken card detection is found.
  2932		 */
  2933		for (i = 0; i < host->num_slots; i++) {
  2934			slot = host->slot[i];
  2935			if (slot->mmc->caps & MMC_CAP_NEEDS_POLL)
  2936				return;
  2937	
  2938			if (IS_ERR_VALUE(mmc_gpio_get_cd(slot->mmc)))
  2939				break;
  2940		}
  2941		if (i == host->num_slots)
  2942			return;
  2943	
  2944		spin_lock_irqsave(&host->irq_lock, irqflags);
  2945		temp = mci_readl(host, INTMASK);
  2946		temp  |= SDMMC_INT_CD;
  2947		mci_writel(host, INTMASK, temp);
  2948		spin_unlock_irqrestore(&host->irq_lock, irqflags);
  2949	}
  2950	
  2951	int dw_mci_probe(struct dw_mci *host)
  2952	{
  2953		const struct dw_mci_drv_data *drv_data = host->drv_data;
  2954		int width, i, ret = 0;
  2955		u32 fifo_size;
  2956		int init_slots = 0;
  2957	
  2958		if (!host->pdata) {
  2959			host->pdata = dw_mci_parse_dt(host);
  2960			if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
  2961				return -EPROBE_DEFER;
  2962			else if (IS_ERR(host->pdata)) {
  2963				dev_err(host->dev, "platform data not available\n");
  2964				return -EINVAL;
  2965			}
  2966		}
  2967	
  2968		host->biu_clk = devm_clk_get(host->dev, "biu");
  2969		if (IS_ERR(host->biu_clk)) {
  2970			dev_dbg(host->dev, "biu clock not available\n");
  2971		} else {
  2972			ret = clk_prepare_enable(host->biu_clk);
  2973			if (ret) {
  2974				dev_err(host->dev, "failed to enable biu clock\n");
  2975				return ret;
  2976			}
  2977		}
  2978	
  2979		host->ciu_clk = devm_clk_get(host->dev, "ciu");
  2980		if (IS_ERR(host->ciu_clk)) {
  2981			dev_dbg(host->dev, "ciu clock not available\n");
  2982			host->bus_hz = host->pdata->bus_hz;
  2983		} else {
  2984			ret = clk_prepare_enable(host->ciu_clk);
  2985			if (ret) {
  2986				dev_err(host->dev, "failed to enable ciu clock\n");
  2987				goto err_clk_biu;
  2988			}
  2989	
  2990			if (host->pdata->bus_hz) {
  2991				ret = clk_set_rate(host->ciu_clk, host->pdata->bus_hz);
  2992				if (ret)
  2993					dev_warn(host->dev,
  2994						 "Unable to set bus rate to %uHz\n",
  2995						 host->pdata->bus_hz);
  2996			}
  2997			host->bus_hz = clk_get_rate(host->ciu_clk);
  2998		}
  2999	
  3000		if (!host->bus_hz) {
  3001			dev_err(host->dev,
  3002				"Platform data must supply bus speed\n");
  3003			ret = -ENODEV;
  3004			goto err_clk_ciu;
  3005		}
  3006	
  3007		if (drv_data && drv_data->init) {
  3008			ret = drv_data->init(host);
  3009			if (ret) {
  3010				dev_err(host->dev,
  3011					"implementation specific init failed\n");
  3012				goto err_clk_ciu;
  3013			}
  3014		}
  3015	
  3016		if (drv_data && drv_data->setup_clock) {
  3017			ret = drv_data->setup_clock(host);
  3018			if (ret) {
  3019				dev_err(host->dev,
  3020					"implementation specific clock setup failed\n");
  3021				goto err_clk_ciu;
  3022			}
  3023		}
  3024	
  3025		if (host->pdata->rstc != NULL)
> 3026			reset_control_deassert(host->pdata->rstc);
  3027	
  3028		setup_timer(&host->cmd11_timer,
  3029			    dw_mci_cmd11_timer, (unsigned long)host);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 45097 bytes --]

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Shawn Lin]

On 2016/3/6 16:47, Guodong Xu wrote:
> mmc registers may in abnormal state if mmc is used in bootloader,
> eg. to support booting from eMMC. So we need reset mmc registers
> when kernel boots up, instead of assuming mmc is in clean state.
>
> With this patch, user can add a 'resets' property into dw_mmc dts
> node. When driver parse_dt and probe, it calls reset API to
> deassert the 'reset' of dw_mmc host controller. When probe error or
> remove, it calls reset API to assert it.
>
> Please also refer to Documentation/devicetree/bindings/reset/reset.txt
>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>

Really should V2 and add the changelog.

> ---
>   drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 242f9a0..281ea9c 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2878,6 +2878,14 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>   	if (!pdata)
>   		return ERR_PTR(-ENOMEM);
>
> +	/* find reset controller when exist */
> +	pdata->rstc = devm_reset_control_get_optional(dev, NULL);
> +	if (IS_ERR(pdata->rstc)) {
> +		if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> +			return ERR_PTR(-EPROBE_DEFER);
> +		pdata->rstc = NULL;

maybe we can remove "pdata->rstc = NULL", and directly
use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)"
statement


> +	}
> +
>   	/* find out number of slots supported */
>   	of_property_read_u32(np, "num-slots", &pdata->num_slots);
>
> @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host)
>
>   	if (!host->pdata) {
>   		host->pdata = dw_mci_parse_dt(host);
> -		if (IS_ERR(host->pdata)) {
> +		if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;

please fix the coding style here.

> +		else if (IS_ERR(host->pdata)) {
>   			dev_err(host->dev, "platform data not available\n");
>   			return -EINVAL;
>   		}
> @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>   		}
>   	}
>
> +	if (host->pdata->rstc != NULL)
> +		reset_control_deassert(host->pdata->rstc);
> +

sorry, I can't follow your intention here. Shouldn't it be something
like "assert mmc -> may need delay -> deassert mmc". As your current
code, nothing happend right?

>   	setup_timer(&host->cmd11_timer,
>   		    dw_mci_cmd11_timer, (unsigned long)host);
>
> @@ -3164,6 +3177,9 @@ err_dmaunmap:
>   	if (host->use_dma && host->dma_ops->exit)
>   		host->dma_ops->exit(host);
>
> +	if (host->pdata->rstc != NULL)
> +		reset_control_assert(host->pdata->rstc);
> +
>   err_clk_ciu:
>   	if (!IS_ERR(host->ciu_clk))
>   		clk_disable_unprepare(host->ciu_clk);
> @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host)
>   	if (host->use_dma && host->dma_ops->exit)
>   		host->dma_ops->exit(host);
>
> +	if (host->pdata->rstc != NULL)
> +		reset_control_assert(host->pdata->rstc);
> +
>   	if (!IS_ERR(host->ciu_clk))
>   		clk_disable_unprepare(host->ciu_clk);
>
>   	if (!IS_ERR(host->biu_clk))
>   		clk_disable_unprepare(host->biu_clk);
> +
>   }

unnecessary new line here.

>   EXPORT_SYMBOL(dw_mci_remove);
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Jaehoon Chung]

On 03/29/2016 11:22 AM, Shawn Lin wrote:
> 在 2016/3/25 13:35, Guodong Xu 写道:
>> Hi, Shawn
>>
>> Sorry I replied late. I added comments below.
>>
>> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com
>> <mailto:shawn.lin@rock-chips.com>> wrote:
>>
>>     On 2016/3/6 16:47, Guodong Xu wrote:
>>
>>         mmc registers may in abnormal state if mmc is used in bootloader,
>>         eg. to support booting from eMMC. So we need reset mmc registers
>>         when kernel boots up, instead of assuming mmc is in clean state.
>>
>>         With this patch, user can add a 'resets' property into dw_mmc dts
>>         node. When driver parse_dt and probe, it calls reset API to
>>         deassert the 'reset' of dw_mmc host controller. When probe error or
>>         remove, it calls reset API to assert it.
>>
>>         Please also refer to
>>         Documentation/devicetree/bindings/reset/reset.txt
>>
>>         Signed-off-by: Guodong Xu <guodong.xu@linaro.org
>>         <mailto:guodong.xu@linaro.org>>
>>         Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com
>>         <mailto:kong.kongxinwei@hisilicon.com>>
>>         Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org
>>         <mailto:zhangfei.gao@linaro.org>>
>>
>>
>>     Really should V2 and add the changelog.
>>
>>
>> Yes, will do. next version I sent will be labelled as V3.
>>
>>
>>
>>         ---
>>            drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
>>            1 file changed, 21 insertions(+), 1 deletion(-)
>>
>>         diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>         index 242f9a0..281ea9c 100644
>>         --- a/drivers/mmc/host/dw_mmc.c
>>         +++ b/drivers/mmc/host/dw_mmc.c
>>         @@ -2878,6 +2878,14 @@ static struct dw_mci_board
>>         *dw_mci_parse_dt(struct dw_mci *host)
>>                  if (!pdata)
>>                          return ERR_PTR(-ENOMEM);
>>
>>         +       /* find reset controller when exist */
>>         +       pdata->rstc = devm_reset_control_get_optional(dev, NULL);
>>         +       if (IS_ERR(pdata->rstc)) {
>>         +               if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>>         +                       return ERR_PTR(-EPROBE_DEFER);
>>         +               pdata->rstc = NULL;
>>
>>
>>     maybe we can remove "pdata->rstc = NULL", and directly
>>     use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)"
>>     statement
>>
>>
>> Yes, will do.
>> I see your point, other lines in this file are using IS_ERR(!..), I will
>> use this style too.
>>
>>         +       }
>>         +
>>                  /* find out number of slots supported */
>>                  of_property_read_u32(np, "num-slots", &pdata->num_slots);
>>
>>         @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host)
>>
>>                  if (!host->pdata) {
>>                          host->pdata = dw_mci_parse_dt(host);
>>         -               if (IS_ERR(host->pdata)) {
>>         +               if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
>>         +                       return -EPROBE_DEFER;
>>
>>
>>     please fix the coding style here.
>>
>>
>> Do you mean to add additional {} for this 'if' , like this?
>>
>>     +               if (PTR_ERR(host->pdata) == -EPROBE_DEFER) {
>>     +                       return -EPROBE_DEFER;
>>
>>     +                }
>>
>> I will add {}.
>>
>>
>>
>>         +               else if (IS_ERR(host->pdata)) {
>>                                  dev_err(host->dev, "platform data not
>>         available\n");
>>                                  return -EINVAL;
>>                          }
>>         @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>                          }
>>                  }
>>
>>         +       if (host->pdata->rstc != NULL)
>>         +               reset_control_deassert(host->pdata->rstc);
>>         +
>>
>>
>>     sorry, I can't follow your intention here. Shouldn't it be something
>>     like "assert mmc -> may need delay -> deassert mmc". As your current
>>     code, nothing happend right?
>>
>>
>> The chip exits from bootloader with this bit asserted. And when entering
>> kernel, we only need to deassert.
>>
>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>> in _remove().
> 
> I catch your point. From the previous discussion, we add it to make sure
> dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in  bootloader, so you can also
> dessert it in bootloaer to make sure dw_mmc work fine when probing
> in kernel. In that way, we don't need this patch?

Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET.
Could you check this?

Best Regards,
Jaehoon Chung

> 
> More to think, Is it ok to match the behaviour of bootloader stage?
> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
> I want to fix you issue on kernel stage, I need a new round of
> assert->delay->deassert.
> 
> 
>>
>>
>>
>>                  setup_timer(&host->cmd11_timer,
>>                              dw_mci_cmd11_timer, (unsigned long)host);
>>
>>         @@ -3164,6 +3177,9 @@ err_dmaunmap:
>>                  if (host->use_dma && host->dma_ops->exit)
>>                          host->dma_ops->exit(host);
>>
>>         +       if (host->pdata->rstc != NULL)
>>         +               reset_control_assert(host->pdata->rstc);
>>         +
>>            err_clk_ciu:
>>                  if (!IS_ERR(host->ciu_clk))
>>                          clk_disable_unprepare(host->ciu_clk);
>>         @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host)
>>                  if (host->use_dma && host->dma_ops->exit)
>>                          host->dma_ops->exit(host);
>>
>>         +       if (host->pdata->rstc != NULL)
>>         +               reset_control_assert(host->pdata->rstc);
>>         +
>>                  if (!IS_ERR(host->ciu_clk))
>>                          clk_disable_unprepare(host->ciu_clk);
>>
>>                  if (!IS_ERR(host->biu_clk))
>>                          clk_disable_unprepare(host->biu_clk);
>>         +
>>            }
>>
>>
>>     unnecessary new line here.
>>
>>
>> Will fix.
>>
>> -Guodong
>>
>>
>>            EXPORT_SYMBOL(dw_mci_remove);
>>
>>
>>
>>
>>     --
>>     Best Regards
>>     Shawn Lin
>>
>>
> 
> 
> 

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Shawn Lin]

在 2016/3/29 13:56, Jaehoon Chung 写道:
> On 03/29/2016 11:22 AM, Shawn Lin wrote:
>> 在 2016/3/25 13:35, Guodong Xu 写道:
>>> Hi, Shawn
>>>
>>> Sorry I replied late. I added comments below.
>>>
>>> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com
>>> <mailto:shawn.lin@rock-chips.com>> wrote:
>>>
>>>      On 2016/3/6 16:47, Guodong Xu wrote:
>>>
>>>          mmc registers may in abnormal state if mmc is used in bootloader,
>>>          eg. to support booting from eMMC. So we need reset mmc registers
>>>          when kernel boots up, instead of assuming mmc is in clean state.
>>>
>>>          With this patch, user can add a 'resets' property into dw_mmc dts
>>>          node. When driver parse_dt and probe, it calls reset API to
>>>          deassert the 'reset' of dw_mmc host controller. When probe error or
>>>          remove, it calls reset API to assert it.
>>>
>>>          Please also refer to
>>>          Documentation/devicetree/bindings/reset/reset.txt
>>>
>>>          Signed-off-by: Guodong Xu <guodong.xu@linaro.org
>>>          <mailto:guodong.xu@linaro.org>>
>>>          Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com
>>>          <mailto:kong.kongxinwei@hisilicon.com>>
>>>          Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org
>>>          <mailto:zhangfei.gao@linaro.org>>
>>>
>>>
>>>      Really should V2 and add the changelog.
>>>
>>>
>>> Yes, will do. next version I sent will be labelled as V3.
>>>
>>>
>>>
>>>          ---
>>>             drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
>>>             1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>>          diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>          index 242f9a0..281ea9c 100644
>>>          --- a/drivers/mmc/host/dw_mmc.c
>>>          +++ b/drivers/mmc/host/dw_mmc.c
>>>          @@ -2878,6 +2878,14 @@ static struct dw_mci_board
>>>          *dw_mci_parse_dt(struct dw_mci *host)
>>>                   if (!pdata)
>>>                           return ERR_PTR(-ENOMEM);
>>>
>>>          +       /* find reset controller when exist */
>>>          +       pdata->rstc = devm_reset_control_get_optional(dev, NULL);
>>>          +       if (IS_ERR(pdata->rstc)) {
>>>          +               if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>>>          +                       return ERR_PTR(-EPROBE_DEFER);
>>>          +               pdata->rstc = NULL;
>>>
>>>
>>>      maybe we can remove "pdata->rstc = NULL", and directly
>>>      use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)"
>>>      statement
>>>
>>>
>>> Yes, will do.
>>> I see your point, other lines in this file are using IS_ERR(!..), I will
>>> use this style too.
>>>
>>>          +       }
>>>          +
>>>                   /* find out number of slots supported */
>>>                   of_property_read_u32(np, "num-slots", &pdata->num_slots);
>>>
>>>          @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>
>>>                   if (!host->pdata) {
>>>                           host->pdata = dw_mci_parse_dt(host);
>>>          -               if (IS_ERR(host->pdata)) {
>>>          +               if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
>>>          +                       return -EPROBE_DEFER;
>>>
>>>
>>>      please fix the coding style here.
>>>
>>>
>>> Do you mean to add additional {} for this 'if' , like this?
>>>
>>>      +               if (PTR_ERR(host->pdata) == -EPROBE_DEFER) {
>>>      +                       return -EPROBE_DEFER;
>>>
>>>      +                }
>>>
>>> I will add {}.
>>>
>>>
>>>
>>>          +               else if (IS_ERR(host->pdata)) {
>>>                                   dev_err(host->dev, "platform data not
>>>          available\n");
>>>                                   return -EINVAL;
>>>                           }
>>>          @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>                           }
>>>                   }
>>>
>>>          +       if (host->pdata->rstc != NULL)
>>>          +               reset_control_deassert(host->pdata->rstc);
>>>          +
>>>
>>>
>>>      sorry, I can't follow your intention here. Shouldn't it be something
>>>      like "assert mmc -> may need delay -> deassert mmc". As your current
>>>      code, nothing happend right?
>>>
>>>
>>> The chip exits from bootloader with this bit asserted. And when entering
>>> kernel, we only need to deassert.
>>>
>>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>>> in _remove().
>>
>> I catch your point. From the previous discussion, we add it to make sure
>> dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in  bootloader, so you can also
>> dessert it in bootloaer to make sure dw_mmc work fine when probing
>> in kernel. In that way, we don't need this patch?
>
> Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET.
> Could you check this?
>

MMC_CAP_HW_RESET actually reset the mmc card, but Guodong means to
reset the controller rather than mmc card :)


> Best Regards,
> Jaehoon Chung
>
>>
>> More to think, Is it ok to match the behaviour of bootloader stage?
>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
>> I want to fix you issue on kernel stage, I need a new round of
>> assert->delay->deassert.
>>
>>
>>>
>>>
>>>
>>>                   setup_timer(&host->cmd11_timer,
>>>                               dw_mci_cmd11_timer, (unsigned long)host);
>>>
>>>          @@ -3164,6 +3177,9 @@ err_dmaunmap:
>>>                   if (host->use_dma && host->dma_ops->exit)
>>>                           host->dma_ops->exit(host);
>>>
>>>          +       if (host->pdata->rstc != NULL)
>>>          +               reset_control_assert(host->pdata->rstc);
>>>          +
>>>             err_clk_ciu:
>>>                   if (!IS_ERR(host->ciu_clk))
>>>                           clk_disable_unprepare(host->ciu_clk);
>>>          @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host)
>>>                   if (host->use_dma && host->dma_ops->exit)
>>>                           host->dma_ops->exit(host);
>>>
>>>          +       if (host->pdata->rstc != NULL)
>>>          +               reset_control_assert(host->pdata->rstc);
>>>          +
>>>                   if (!IS_ERR(host->ciu_clk))
>>>                           clk_disable_unprepare(host->ciu_clk);
>>>
>>>                   if (!IS_ERR(host->biu_clk))
>>>                           clk_disable_unprepare(host->biu_clk);
>>>          +
>>>             }
>>>
>>>
>>>      unnecessary new line here.
>>>
>>>
>>> Will fix.
>>>
>>> -Guodong
>>>
>>>
>>>             EXPORT_SYMBOL(dw_mci_remove);
>>>
>>>
>>>
>>>
>>>      --
>>>      Best Regards
>>>      Shawn Lin
>>>
>>>
>>
>>
>>
>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Jaehoon Chung]

On 03/29/2016 03:09 PM, Shawn Lin wrote:
> 在 2016/3/29 13:56, Jaehoon Chung 写道:
>> On 03/29/2016 11:22 AM, Shawn Lin wrote:
>>> 在 2016/3/25 13:35, Guodong Xu 写道:
>>>> Hi, Shawn
>>>>
>>>> Sorry I replied late. I added comments below.
>>>>
>>>> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com
>>>> <mailto:shawn.lin@rock-chips.com>> wrote:
>>>>
>>>>      On 2016/3/6 16:47, Guodong Xu wrote:
>>>>
>>>>          mmc registers may in abnormal state if mmc is used in bootloader,
>>>>          eg. to support booting from eMMC. So we need reset mmc registers
>>>>          when kernel boots up, instead of assuming mmc is in clean state.
>>>>
>>>>          With this patch, user can add a 'resets' property into dw_mmc dts
>>>>          node. When driver parse_dt and probe, it calls reset API to
>>>>          deassert the 'reset' of dw_mmc host controller. When probe error or
>>>>          remove, it calls reset API to assert it.
>>>>
>>>>          Please also refer to
>>>>          Documentation/devicetree/bindings/reset/reset.txt
>>>>
>>>>          Signed-off-by: Guodong Xu <guodong.xu@linaro.org
>>>>          <mailto:guodong.xu@linaro.org>>
>>>>          Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com
>>>>          <mailto:kong.kongxinwei@hisilicon.com>>
>>>>          Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org
>>>>          <mailto:zhangfei.gao@linaro.org>>
>>>>
>>>>
>>>>      Really should V2 and add the changelog.
>>>>
>>>>
>>>> Yes, will do. next version I sent will be labelled as V3.
>>>>
>>>>
>>>>
>>>>          ---
>>>>             drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
>>>>             1 file changed, 21 insertions(+), 1 deletion(-)
>>>>
>>>>          diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>          index 242f9a0..281ea9c 100644
>>>>          --- a/drivers/mmc/host/dw_mmc.c
>>>>          +++ b/drivers/mmc/host/dw_mmc.c
>>>>          @@ -2878,6 +2878,14 @@ static struct dw_mci_board
>>>>          *dw_mci_parse_dt(struct dw_mci *host)
>>>>                   if (!pdata)
>>>>                           return ERR_PTR(-ENOMEM);
>>>>
>>>>          +       /* find reset controller when exist */
>>>>          +       pdata->rstc = devm_reset_control_get_optional(dev, NULL);
>>>>          +       if (IS_ERR(pdata->rstc)) {
>>>>          +               if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>>>>          +                       return ERR_PTR(-EPROBE_DEFER);
>>>>          +               pdata->rstc = NULL;
>>>>
>>>>
>>>>      maybe we can remove "pdata->rstc = NULL", and directly
>>>>      use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)"
>>>>      statement
>>>>
>>>>
>>>> Yes, will do.
>>>> I see your point, other lines in this file are using IS_ERR(!..), I will
>>>> use this style too.
>>>>
>>>>          +       }
>>>>          +
>>>>                   /* find out number of slots supported */
>>>>                   of_property_read_u32(np, "num-slots", &pdata->num_slots);
>>>>
>>>>          @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>>
>>>>                   if (!host->pdata) {
>>>>                           host->pdata = dw_mci_parse_dt(host);
>>>>          -               if (IS_ERR(host->pdata)) {
>>>>          +               if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
>>>>          +                       return -EPROBE_DEFER;
>>>>
>>>>
>>>>      please fix the coding style here.
>>>>
>>>>
>>>> Do you mean to add additional {} for this 'if' , like this?
>>>>
>>>>      +               if (PTR_ERR(host->pdata) == -EPROBE_DEFER) {
>>>>      +                       return -EPROBE_DEFER;
>>>>
>>>>      +                }
>>>>
>>>> I will add {}.
>>>>
>>>>
>>>>
>>>>          +               else if (IS_ERR(host->pdata)) {
>>>>                                   dev_err(host->dev, "platform data not
>>>>          available\n");
>>>>                                   return -EINVAL;
>>>>                           }
>>>>          @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>>                           }
>>>>                   }
>>>>
>>>>          +       if (host->pdata->rstc != NULL)
>>>>          +               reset_control_deassert(host->pdata->rstc);
>>>>          +
>>>>
>>>>
>>>>      sorry, I can't follow your intention here. Shouldn't it be something
>>>>      like "assert mmc -> may need delay -> deassert mmc". As your current
>>>>      code, nothing happend right?
>>>>
>>>>
>>>> The chip exits from bootloader with this bit asserted. And when entering
>>>> kernel, we only need to deassert.
>>>>
>>>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>>>> in _remove().
>>>
>>> I catch your point. From the previous discussion, we add it to make sure
>>> dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in  bootloader, so you can also
>>> dessert it in bootloaer to make sure dw_mmc work fine when probing
>>> in kernel. In that way, we don't need this patch?
>>
>> Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET.
>> Could you check this?
>>
> 
> MMC_CAP_HW_RESET actually reset the mmc card, but Guodong means to
> reset the controller rather than mmc card :)

We have talked about FBE scenarios, right? :)
Now, I remembered it.

> 
> 
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> More to think, Is it ok to match the behaviour of bootloader stage?
>>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
>>> I want to fix you issue on kernel stage, I need a new round of
>>> assert->delay->deassert.
>>>
>>>
>>>>
>>>>
>>>>
>>>>                   setup_timer(&host->cmd11_timer,
>>>>                               dw_mci_cmd11_timer, (unsigned long)host);
>>>>
>>>>          @@ -3164,6 +3177,9 @@ err_dmaunmap:
>>>>                   if (host->use_dma && host->dma_ops->exit)
>>>>                           host->dma_ops->exit(host);
>>>>
>>>>          +       if (host->pdata->rstc != NULL)
>>>>          +               reset_control_assert(host->pdata->rstc);
>>>>          +
>>>>             err_clk_ciu:
>>>>                   if (!IS_ERR(host->ciu_clk))
>>>>                           clk_disable_unprepare(host->ciu_clk);
>>>>          @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host)
>>>>                   if (host->use_dma && host->dma_ops->exit)
>>>>                           host->dma_ops->exit(host);
>>>>
>>>>          +       if (host->pdata->rstc != NULL)
>>>>          +               reset_control_assert(host->pdata->rstc);
>>>>          +
>>>>                   if (!IS_ERR(host->ciu_clk))
>>>>                           clk_disable_unprepare(host->ciu_clk);
>>>>
>>>>                   if (!IS_ERR(host->biu_clk))
>>>>                           clk_disable_unprepare(host->biu_clk);
>>>>          +
>>>>             }
>>>>
>>>>
>>>>      unnecessary new line here.
>>>>
>>>>
>>>> Will fix.
>>>>
>>>> -Guodong
>>>>
>>>>
>>>>             EXPORT_SYMBOL(dw_mci_remove);
>>>>
>>>>
>>>>
>>>>
>>>>      --
>>>>      Best Regards
>>>>      Shawn Lin
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>>
> 
> 

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[zhangfei]

On 03/29/2016 10:22 AM, Shawn Lin wrote:

>>
>>
>>
>>         +               else if (IS_ERR(host->pdata)) {
>>                                  dev_err(host->dev, "platform data not
>>         available\n");
>>                                  return -EINVAL;
>>                          }
>>         @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>                          }
>>                  }
>>
>>         +       if (host->pdata->rstc != NULL)
>>         +               reset_control_deassert(host->pdata->rstc);
>>         +
>>
>>
>>     sorry, I can't follow your intention here. Shouldn't it be something
>>     like "assert mmc -> may need delay -> deassert mmc". As your current
>>     code, nothing happend right?
Should be abstracted in reset driver.

>>
>>
>> The chip exits from bootloader with this bit asserted. And when entering
>> kernel, we only need to deassert.
>>
>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>> in _remove().
>
> I catch your point. From the previous discussion, we add it to make sure
> dw_mmc in good state after leaving bootloader to kernel. But My real
> question is that you can assert it in  bootloader, so you can also
> dessert it in bootloaer to make sure dw_mmc work fine when probing
> in kernel. In that way, we don't need this patch?

uefi does not have exit point, and kernel may not assume mmc controller 
state is always correct when boot.
If Uefi need copy Image from mmc, mmc controller is in working state.
When jump to kernel, uefi mmc driver can not recover itself.
If kernel assume mmc controller state is clean, mmc will be in abnormal 
state.
Some controller will clear itself when set clock, however, hip660 does 
not, it need special register to access.


>
> More to think, Is it ok to match the behaviour of bootloader stage?
> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
> I want to fix you issue on kernel stage, I need a new round of
> assert->delay->deassert.

The process like delay (if required) should be abstracted in reset driver.
reset framework just export reset_control_assert/reset_control_deassert API.
Unfortunately not find clear description in Documentation/.
Suppose deassert is like start, while assert is like stop.

drivers/reset/core.c
reset_control_deassert - deasserts the reset line
reset_control_assert - asserts the reset line

More example:
drivers/mmc/host/sdhci-st.c
drivers/mmc/host/sunxi-mmc.c
drivers/usb/host/ohci-platform.c
drivers/i2c/busses/i2c-mv64xxx.c

Thanks

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Jaehoon Chung]

On 03/29/2016 05:23 PM, zhangfei wrote:
> 
> 
> On 03/29/2016 10:22 AM, Shawn Lin wrote:
> 
>>>
>>>
>>>
>>>         +               else if (IS_ERR(host->pdata)) {
>>>                                  dev_err(host->dev, "platform data not
>>>         available\n");
>>>                                  return -EINVAL;
>>>                          }
>>>         @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>                          }
>>>                  }
>>>
>>>         +       if (host->pdata->rstc != NULL)
>>>         +               reset_control_deassert(host->pdata->rstc);
>>>         +
>>>
>>>
>>>     sorry, I can't follow your intention here. Shouldn't it be something
>>>     like "assert mmc -> may need delay -> deassert mmc". As your current
>>>     code, nothing happend right?
> Should be abstracted in reset driver.
> 
>>>
>>>
>>> The chip exits from bootloader with this bit asserted. And when entering
>>> kernel, we only need to deassert.
>>>
>>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>>> in _remove().
>>
>> I catch your point. From the previous discussion, we add it to make sure
>> dw_mmc in good state after leaving bootloader to kernel. But My real
>> question is that you can assert it in  bootloader, so you can also
>> dessert it in bootloaer to make sure dw_mmc work fine when probing
>> in kernel. In that way, we don't need this patch?
> 
> uefi does not have exit point, and kernel may not assume mmc controller state is always correct when boot.
> If Uefi need copy Image from mmc, mmc controller is in working state.
> When jump to kernel, uefi mmc driver can not recover itself.
> If kernel assume mmc controller state is clean, mmc will be in abnormal state.
> Some controller will clear itself when set clock, however, hip660 does not, it need special register to access.
> 
> 
>>
>> More to think, Is it ok to match the behaviour of bootloader stage?
>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
>> I want to fix you issue on kernel stage, I need a new round of
>> assert->delay->deassert.
> 
> The process like delay (if required) should be abstracted in reset driver.
> reset framework just export reset_control_assert/reset_control_deassert API.
> Unfortunately not find clear description in Documentation/.
> Suppose deassert is like start, while assert is like stop.

First, this patch need to resend after fixing.
Could you or Guodong resend these patches as V2 or V3?

Best Regards,
Jaehoon Chung

> 
> drivers/reset/core.c
> reset_control_deassert - deasserts the reset line
> reset_control_assert - asserts the reset line
> 
> More example:
> drivers/mmc/host/sdhci-st.c
> drivers/mmc/host/sunxi-mmc.c
> drivers/usb/host/ohci-platform.c
> drivers/i2c/busses/i2c-mv64xxx.c
> 
> Thanks
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Shawn Lin]

在 2016/3/29 16:23, zhangfei 写道:
>
>
> On 03/29/2016 10:22 AM, Shawn Lin wrote:
>
>>>
>>>
>>>
>>>         +               else if (IS_ERR(host->pdata)) {
>>>                                  dev_err(host->dev, "platform data not
>>>         available\n");
>>>                                  return -EINVAL;
>>>                          }
>>>         @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>                          }
>>>                  }
>>>
>>>         +       if (host->pdata->rstc != NULL)
>>>         +               reset_control_deassert(host->pdata->rstc);
>>>         +
>>>
>>>
>>>     sorry, I can't follow your intention here. Shouldn't it be something
>>>     like "assert mmc -> may need delay -> deassert mmc". As your current
>>>     code, nothing happend right?
> Should be abstracted in reset driver.
>
>>>
>>>
>>> The chip exits from bootloader with this bit asserted. And when entering
>>> kernel, we only need to deassert.
>>>
>>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>>> in _remove().
>>
>> I catch your point. From the previous discussion, we add it to make sure
>> dw_mmc in good state after leaving bootloader to kernel. But My real
>> question is that you can assert it in  bootloader, so you can also
>> dessert it in bootloaer to make sure dw_mmc work fine when probing
>> in kernel. In that way, we don't need this patch?
>
> uefi does not have exit point, and kernel may not assume mmc controller
> state is always correct when boot.
> If Uefi need copy Image from mmc, mmc controller is in working state.
> When jump to kernel, uefi mmc driver can not recover itself.
> If kernel assume mmc controller state is clean, mmc will be in abnormal
> state.
> Some controller will clear itself when set clock, however, hip660 does
> not, it need special register to access.
>

yep, I understand your requirement.

>
>>
>> More to think, Is it ok to match the behaviour of bootloader stage?
>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
>> I want to fix you issue on kernel stage, I need a new round of
>> assert->delay->deassert.
>
> The process like delay (if required) should be abstracted in reset driver.
> reset framework just export reset_control_assert/reset_control_deassert
> API.
> Unfortunately not find clear description in Documentation/.
> Suppose deassert is like start, while assert is like stop.
>

yes, no docs except dt-bindings..... So seems the usage of these two
APIs depends on the implementation of reset controller driver

> drivers/reset/core.c
> reset_control_deassert - deasserts the reset line
> reset_control_assert - asserts the reset line
>
> More example:
> drivers/mmc/host/sdhci-st.c
> drivers/mmc/host/sunxi-mmc.c
> drivers/usb/host/ohci-platform.c
> drivers/i2c/busses/i2c-mv64xxx.c

But I'm afraid I have to nack here....

Looking at these files:
drivers/dma/stm32-dma.c
drivers/net/ethernet/mediatek/mtk_eth_soc.c
drivers/devfreq/tegra-devfreq.c
drivers/crypto/rockchip/rk3288_crypto.c
drivers/thermal/rockchip_thermal.c
drivers/thermal/tegra_soctherm.c
drivers/i2c/busses/i2c-tegra.c
....

they just do the way like: assert->[delay](maybe abstracted)->deassert
I think these drivers are vendor specific, so they can do
the reset operations in consistent with the implementation of
their platforms' reset controller drivers.

But, dw_mmc is shared by many Socs. So It's not good to do it in
dw_mci_probe, otherwise you force my reset controller driver to be
implemented in the same way as yours..... Right?

How about move it to your drv_data->init callback?


>
> Thanks
>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[zhangfei]

On 03/30/2016 08:46 AM, Shawn Lin wrote:
> 在 2016/3/29 16:23, zhangfei 写道:

>>> More to think, Is it ok to match the behaviour of bootloader stage?
>>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
>>> I want to fix you issue on kernel stage, I need a new round of
>>> assert->delay->deassert.
>>
>> The process like delay (if required) should be abstracted in reset
>> driver.
>> reset framework just export reset_control_assert/reset_control_deassert
>> API.
>> Unfortunately not find clear description in Documentation/.
>> Suppose deassert is like start, while assert is like stop.
>>
>
> yes, no docs except dt-bindings..... So seems the usage of these two
> APIs depends on the implementation of reset controller driver
>
>> drivers/reset/core.c
>> reset_control_deassert - deasserts the reset line
>> reset_control_assert - asserts the reset line
>>
>> More example:
>> drivers/mmc/host/sdhci-st.c
>> drivers/mmc/host/sunxi-mmc.c
>> drivers/usb/host/ohci-platform.c
>> drivers/i2c/busses/i2c-mv64xxx.c
>
> But I'm afraid I have to nack here....
>
> Looking at these files:
> drivers/dma/stm32-dma.c
> drivers/net/ethernet/mediatek/mtk_eth_soc.c
> drivers/devfreq/tegra-devfreq.c
> drivers/crypto/rockchip/rk3288_crypto.c
> drivers/thermal/rockchip_thermal.c
> drivers/thermal/tegra_soctherm.c
> drivers/i2c/busses/i2c-tegra.c
> ....
>
> they just do the way like: assert->[delay](maybe abstracted)->deassert
> I think these drivers are vendor specific, so they can do
> the reset operations in consistent with the implementation of
> their platforms' reset controller drivers.

Yes, have checked drivers/i2c/busses/i2c-tegra.c
         reset_control_assert(i2c_dev->rst);
         udelay(2);
         reset_control_deassert(i2c_dev->rst);

This usage looks strange, reset does not mean gpio reset, it maybe 
register accessing.
I think all these three operation should be abstracted to deassert 
function, while cover the details for sharing.

However, not doc describing the assert/deassert behavior so causing 
confusion.

>
> But, dw_mmc is shared by many Socs. So It's not good to do it in
> dw_mci_probe, otherwise you force my reset controller driver to be
> implemented in the same way as yours..... Right?
>
> How about move it to your drv_data->init callback?
Yes, we can.
But firstly we should consider put in common file for sharing, otherwise 
there maybe many assert/deassert in dw_mmc-xx.c.

Guodong may send another version and wait for Jaehoon's decision.

Thanks

Re: [PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[Jaehoon Chung]

Hi Goudong,

On 03/06/2016 05:47 PM, Guodong Xu wrote:
> Add resets property to synopsys-dw-mshc bindings. It is intended to
> represent the hardware reset signal present internally in some host
> controller IC designs.
> 
> See Documentation/devicetree/bindings/reset/reset.txt for details.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> index 8636f5a..4e00e85 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -39,6 +39,10 @@ Required Properties:
>  
>  Optional properties:
>  
> +* resets: phandle + reset specifier pair, intended to represent hardware
> +  reset signal present internally in some host controller IC designs.
> +  See Documentation/devicetree/bindings/reset/reset.txt for details.

Is this reset property for common dwmmc IP controller?

Best Regards,
Jaehoon Chung

> +
>  * clocks: from common clock binding: handle to biu and ciu clocks for the
>    bus interface unit clock and the card interface unit clock.
>  
> 

Re: [PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[Shawn Lin]

Hi Jaehoon,

On 2016/3/7 8:53, Jaehoon Chung wrote:
> Hi Goudong,
>
> On 03/06/2016 05:47 PM, Guodong Xu wrote:
>> Add resets property to synopsys-dw-mshc bindings. It is intended to
>> represent the hardware reset signal present internally in some host
>> controller IC designs.
>>
>> See Documentation/devicetree/bindings/reset/reset.txt for details.
>>
>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>   Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>> index 8636f5a..4e00e85 100644
>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>> @@ -39,6 +39,10 @@ Required Properties:
>>
>>   Optional properties:
>>
>> +* resets: phandle + reset specifier pair, intended to represent hardware
>> +  reset signal present internally in some host controller IC designs.
>> +  See Documentation/devicetree/bindings/reset/reset.txt for details.
>
> Is this reset property for common dwmmc IP controller?

I think so. By discussion with my ASIC team, it's provided by synopsys.
 From dw_mmc databook version 270a, section 3.2.5, FBE Scenarios:

An FBE occurs due to an AHB error response on the AHB bus. This is a
system error, so the software driver should not perform any further
programming to the DWC_mobile_storage. The only recovery mechanism
from such scenarios is to do one of the following:
■ Issue a hard reset by asserting the reset_n signal
■ Do a program controller reset by writing to the CTRL[0] register

the reset_n signal can be used to reset all the internal logic block
with dwmmc and reset the register value to default stat.

Note: reset_n is a internal signal, which is diff from rst_n for mmc hw
reset. (refer to databook section 5.2 Signal Descriptions, table 5-1)

>
> Best Regards,
> Jaehoon Chung
>
>> +
>>   * clocks: from common clock binding: handle to biu and ciu clocks for the
>>     bus interface unit clock and the card interface unit clock.
>>
>>
>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[Jaehoon Chung]

Hi Shawn,

On 03/07/2016 06:35 PM, Shawn Lin wrote:
> Hi Jaehoon,
> 
> On 2016/3/7 8:53, Jaehoon Chung wrote:
>> Hi Goudong,
>>
>> On 03/06/2016 05:47 PM, Guodong Xu wrote:
>>> Add resets property to synopsys-dw-mshc bindings. It is intended to
>>> represent the hardware reset signal present internally in some host
>>> controller IC designs.
>>>
>>> See Documentation/devicetree/bindings/reset/reset.txt for details.
>>>
>>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> ---
>>>   Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>>> index 8636f5a..4e00e85 100644
>>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>>> @@ -39,6 +39,10 @@ Required Properties:
>>>
>>>   Optional properties:
>>>
>>> +* resets: phandle + reset specifier pair, intended to represent hardware
>>> +  reset signal present internally in some host controller IC designs.
>>> +  See Documentation/devicetree/bindings/reset/reset.txt for details.
>>
>> Is this reset property for common dwmmc IP controller?
> 
> I think so. By discussion with my ASIC team, it's provided by synopsys.
> From dw_mmc databook version 270a, section 3.2.5, FBE Scenarios:
> 
> An FBE occurs due to an AHB error response on the AHB bus. This is a
> system error, so the software driver should not perform any further
> programming to the DWC_mobile_storage. The only recovery mechanism
> from such scenarios is to do one of the following:
> ■ Issue a hard reset by asserting the reset_n signal
> ■ Do a program controller reset by writing to the CTRL[0] register
> 
> the reset_n signal can be used to reset all the internal logic block
> with dwmmc and reset the register value to default stat.
> 
> Note: reset_n is a internal signal, which is diff from rst_n for mmc hw
> reset. (refer to databook section 5.2 Signal Descriptions, table 5-1)

Thanks for this information. :)

> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> +
>>>   * clocks: from common clock binding: handle to biu and ciu clocks for the
>>>     bus interface unit clock and the card interface unit clock.
>>>
>>>
>>
>>
>>
>>
> 
> 

[PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[Guodong Xu]

Add resets property to synopsys-dw-mshc bindings. It is intended to
represent the hardware reset signal present internally in some host
controller IC designs.

See Documentation/devicetree/bindings/reset/reset.txt for details.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
index 8636f5a..9b4896c 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
@@ -39,6 +39,10 @@ Required Properties:
 
 Optional properties:
 
+* resets: phandle + reset specifier pair, intended to represent hardware
+  reset signal present internally in some host controller IC designs.
+  See Documentation/devicetree/bindings/reset/reset.txt for details.
+
 * clocks: from common clock binding: handle to biu and ciu clocks for the
   bus interface unit clock and the card interface unit clock.
 
@@ -48,7 +52,7 @@ Optional properties:
   clock-frequency.  It is an error to omit both the ciu clock and the
   clock-frequency.
 
-* clock-frequency: should be the frequency (in Hz) of the ciu clock.  If this
+* clock-frequency: should be tke frequency (in Hz) of the ciu clock.  If this
   is specified and the ciu clock is specified then we'll try to set the ciu
   clock to this at probe time.
 
-- 
1.9.1

Re: [PATCH 1/2] Documentation: synopsys-dw-mshc: add binding for resets

[Rob Herring]

On Thu, Mar 03, 2016 at 09:33:37AM +0800, Guodong Xu wrote:
> Add resets property to synopsys-dw-mshc bindings. It is intended to
> represent the hardware reset signal present internally in some host
> controller IC designs.
> 
> See Documentation/devicetree/bindings/reset/reset.txt for details.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> index 8636f5a..9b4896c 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -39,6 +39,10 @@ Required Properties:
>  
>  Optional properties:
>  
> +* resets: phandle + reset specifier pair, intended to represent hardware
> +  reset signal present internally in some host controller IC designs.
> +  See Documentation/devicetree/bindings/reset/reset.txt for details.
> +
>  * clocks: from common clock binding: handle to biu and ciu clocks for the
>    bus interface unit clock and the card interface unit clock.
>  
> @@ -48,7 +52,7 @@ Optional properties:
>    clock-frequency.  It is an error to omit both the ciu clock and the
>    clock-frequency.
>  
> -* clock-frequency: should be the frequency (in Hz) of the ciu clock.  If this
> +* clock-frequency: should be tke frequency (in Hz) of the ciu clock.  If this

You've added a typo here.

Otherwise,

Acked-by: Rob Herring <robh@kernel.org>