[PATCH v3 0/2] mmc: dw_mmc: controller reset support

[PATCH v3 0/2] mmc: dw_mmc: controller reset support

[Guodong Xu]

mmc controller registers may in abnormal state if mmc is used in
bootloader, eg. to load kernel from eMMC. Some controllers cann't
clear their registers when clk is set. They use dedicated reset
logics to do this.

In this patch, a 'resets' property is added into dw_mmc dts
node. When driver does 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.

Chip vendor's actual reset logics is implemented in reset driver, not
in dw_mmc code.

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

Guodong Xu (2):
  Documentation: synopsys-dw-mshc: add binding for resets
  mmc: dw_mmc: add resets support to dw_mmc

 .../devicetree/bindings/mmc/synopsys-dw-mshc.txt     |  4 ++++
 drivers/mmc/host/dw_mmc.c                            | 20 +++++++++++++++++++-
 include/linux/mmc/dw_mmc.h                           |  6 ++++--
 3 files changed, 27 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v3 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 v3 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  | 20 +++++++++++++++++++-
 include/linux/mmc/dw_mmc.h |  6 ++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 242f9a0..d0a4535 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2878,6 +2878,13 @@ 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);
+	}
+
 	/* find out number of slots supported */
 	of_property_read_u32(np, "num-slots", &pdata->num_slots);
 
@@ -2949,7 +2956,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 +3021,9 @@ int dw_mci_probe(struct dw_mci *host)
 		}
 	}
 
+	if (!IS_ERR(host->pdata->rstc))
+		reset_control_deassert(host->pdata->rstc);
+
 	setup_timer(&host->cmd11_timer,
 		    dw_mci_cmd11_timer, (unsigned long)host);
 
@@ -3164,6 +3176,9 @@ err_dmaunmap:
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	if (!IS_ERR(host->pdata->rstc))
+		reset_control_assert(host->pdata->rstc);
+
 err_clk_ciu:
 	if (!IS_ERR(host->ciu_clk))
 		clk_disable_unprepare(host->ciu_clk);
@@ -3196,6 +3211,9 @@ void dw_mci_remove(struct dw_mci *host)
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	if (!IS_ERR(host->pdata->rstc))
+		reset_control_assert(host->pdata->rstc);
+
 	if (!IS_ERR(host->ciu_clk))
 		clk_disable_unprepare(host->ciu_clk);
 
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 7b41c6d..b95cd84 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -14,9 +14,10 @@
 #ifndef LINUX_MMC_DW_MMC_H
 #define LINUX_MMC_DW_MMC_H
 
-#include <linux/scatterlist.h>
-#include <linux/mmc/core.h>
 #include <linux/dmaengine.h>
+#include <linux/mmc/core.h>
+#include <linux/reset.h>
+#include <linux/scatterlist.h>
 
 #define MAX_MCI_SLOTS	2
 
@@ -260,6 +261,7 @@ struct dw_mci_board {
 	/* delay in mS before detecting cards after interrupt */
 	u32 detect_delay_ms;
 
+	struct reset_control *rstc;
 	struct dw_mci_dma_ops *dma_ops;
 	struct dma_pdata *data;
 };
-- 
1.9.1

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

[Jaehoon Chung]

modified Rob's mail address.

On 03/30/2016 04:24 PM, 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.

Do you mean mmc(card side) register or dwmmc host controller's register on host side?

According to dwmmc controller TMR, there are two reset signals. One is reset_n, other is rst_n.
It seems this patch is relevant to reset_n(For host). (rst_n is hardware reset for card.)

So could you clarify better? Then it's helpful to me for understanding..

It seems that it means "mmc" is card, mmc registers is host controller register, right?

> 
> 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  | 20 +++++++++++++++++++-
>  include/linux/mmc/dw_mmc.h |  6 ++++--
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 242f9a0..d0a4535 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2878,6 +2878,13 @@ 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);
> +	}
> +
>  	/* find out number of slots supported */
>  	of_property_read_u32(np, "num-slots", &pdata->num_slots);
>  
> @@ -2949,7 +2956,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 +3021,9 @@ int dw_mci_probe(struct dw_mci *host)
>  		}
>  	}
>  
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_deassert(host->pdata->rstc);
> +
>  	setup_timer(&host->cmd11_timer,
>  		    dw_mci_cmd11_timer, (unsigned long)host);
>  
> @@ -3164,6 +3176,9 @@ err_dmaunmap:
>  	if (host->use_dma && host->dma_ops->exit)
>  		host->dma_ops->exit(host);
>  
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_assert(host->pdata->rstc);

location is correct?

> +
>  err_clk_ciu:
>  	if (!IS_ERR(host->ciu_clk))
>  		clk_disable_unprepare(host->ciu_clk);
> @@ -3196,6 +3211,9 @@ void dw_mci_remove(struct dw_mci *host)
>  	if (host->use_dma && host->dma_ops->exit)
>  		host->dma_ops->exit(host);
>  
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_assert(host->pdata->rstc);
> +
>  	if (!IS_ERR(host->ciu_clk))
>  		clk_disable_unprepare(host->ciu_clk);
>  
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 7b41c6d..b95cd84 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -14,9 +14,10 @@
>  #ifndef LINUX_MMC_DW_MMC_H
>  #define LINUX_MMC_DW_MMC_H
>  
> -#include <linux/scatterlist.h>
> -#include <linux/mmc/core.h>
>  #include <linux/dmaengine.h>
> +#include <linux/mmc/core.h>
> +#include <linux/reset.h>
> +#include <linux/scatterlist.h>

Why did you touch other things?

>  
>  #define MAX_MCI_SLOTS	2
>  
> @@ -260,6 +261,7 @@ struct dw_mci_board {
>  	/* delay in mS before detecting cards after interrupt */
>  	u32 detect_delay_ms;
>  
> +	struct reset_control *rstc;
>  	struct dw_mci_dma_ops *dma_ops;
>  	struct dma_pdata *data;
>  };
> 

Re: [PATCH v3 0/2] mmc: dw_mmc: controller reset support

[Shawn Lin]

+ Heiko
On 2016/3/30 15:24, Guodong Xu wrote:
> mmc controller registers may in abnormal state if mmc is used in
> bootloader, eg. to load kernel from eMMC. Some controllers cann't
> clear their registers when clk is set. They use dedicated reset
> logics to do this.
>
> In this patch, a 'resets' property is added into dw_mmc dts
> node. When driver does 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.
>
> Chip vendor's actual reset logics is implemented in reset driver, not
> in dw_mmc code.
>
> Please also refer to Documentation/devicetree/bindings/reset/reset.txt
>
> Guodong Xu (2):
>    Documentation: synopsys-dw-mshc: add binding for resets
>    mmc: dw_mmc: add resets support to dw_mmc
>
>   .../devicetree/bindings/mmc/synopsys-dw-mshc.txt     |  4 ++++
>   drivers/mmc/host/dw_mmc.c                            | 20 +++++++++++++++++++-
>   include/linux/mmc/dw_mmc.h                           |  6 ++++--
>   3 files changed, 27 insertions(+), 3 deletions(-)
>


-- 
Best Regards
Shawn Lin

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

[Heiko Stuebner]

Am Mittwoch, 30. März 2016, 20:40:31 schrieb Jaehoon Chung:
> modified Rob's mail address.
> 
> On 03/30/2016 04:24 PM, 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.
> 
> Do you mean mmc(card side) register or dwmmc host controller's register on
> host side?
> 
> According to dwmmc controller TMR, there are two reset signals. One is
> reset_n, other is rst_n. It seems this patch is relevant to reset_n(For
> host). (rst_n is hardware reset for card.)
> 
> So could you clarify better? Then it's helpful to me for understanding..

I think that actually means a reset of controller IP block logic, outside 
the control of the dw_mmc block itself.

On Rockchip SoCs this gets triggered from the CRU (clock and reset unit), so 
I guess if I'm reading the manual correctly, should be the reset_n signal of 
the ip block.

rst_n on the other hand gets triggered through a dw_mmc register setting and 
is already handled by the dw_mmc driver.


Heiko

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

[Heiko Stuebner]

Am Mittwoch, 30. März 2016, 15:24:56 schrieb Guodong Xu:

[...]

> @@ -2949,7 +2956,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)) {

how is this related to adding the reset handling?

Making the driver handle probe deferral better should be a separate patch.

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

Wouldn't reset_control_reset be better? The way it is now it would expect 
the reset to be asserted somewhere else before dw_mmc probes?


>  	setup_timer(&host->cmd11_timer,
>  		    dw_mci_cmd11_timer, (unsigned long)host);
> 

[...]

> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 7b41c6d..b95cd84 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -14,9 +14,10 @@
>  #ifndef LINUX_MMC_DW_MMC_H
>  #define LINUX_MMC_DW_MMC_H
> 
> -#include <linux/scatterlist.h>
> -#include <linux/mmc/core.h>
>  #include <linux/dmaengine.h>
> +#include <linux/mmc/core.h>
> +#include <linux/reset.h>
> +#include <linux/scatterlist.h>

unrelated changed regarding the reordering of includes.


Heiko

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

[Heiko Stuebner]

Am Samstag, 2. April 2016, 21:39:11 schrieb Guodong Xu:
> On 2 April 2016 at 02:42, Heiko Stuebner <heiko@sntech.de> wrote:
> > Am Mittwoch, 30. März 2016, 15:24:56 schrieb Guodong Xu:
> > 
> > [...]
> > 
> > > @@ -2949,7 +2956,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)) {
> > 
> > how is this related to adding the reset handling?
> 
> I added this into dw_mci_parse_dt(host), and that's the first time it may
> return -EPROBE_DEFER
> 
>   /* find reset controller when exist */
>   pdata->rstc = devm_reset_control_get_optional(dev, NULL);
> 
> So, I added processing to this error in this patch.

ah, you're right of course


> > Making the driver handle probe deferral better should be a separate
> > patch.> 
> > >                       dev_err(host->dev, "platform data not
> > 
> > available\n");
> > 
> > >                       return -EINVAL;
> > >               
> > >               }
> > > 
> > > @@ -3012,6 +3021,9 @@ int dw_mci_probe(struct dw_mci *host)
> > > 
> > >               }
> > >       
> > >       }
> > > 
> > > +     if (!IS_ERR(host->pdata->rstc))
> > > +             reset_control_deassert(host->pdata->rstc);
> > > +
> > 
> > Wouldn't reset_control_reset be better? The way it is now it would
> > expect
> > the reset to be asserted somewhere else before dw_mmc probes?
> 
> It relates to how the SoC's reset logic is like. One bit set can clear all
> dw_mmc host controller registers. It doesn't need do assert then
> deassert.
> 
> That's what I saw in hi6220 (it integrates three dw_mmc host controller),
> drivers/reset/hisilicon/hi6220_reset.c
> , which I wrote this patch for.

I just realized again that reset_control_reset is a completely separate 
operation (not related to assert / deassert).

What I was originally getting at is that I don't see any assert-counterpart. 
So if the reset-control is already deasserted, nothing will happen on some 
designs.

For example on Rockchip SoCs the reset controller needs the signal to be 
high to assert the reset and the dw_mmc part of the manual explicitly says 
that the "reset_n should be asserted(active-low) for at least two clocks of 
clk or cclk_in".

So I would expect something like

	reset_control_assert(reset);
	usleep_range(x, y);
	reset_control_deassert(reset);

instead of only trying to deassert the reset.


> > >       setup_timer(&host->cmd11_timer,
> > >       
> > >                   dw_mci_cmd11_timer, (unsigned long)host);
> > 
> > [...]
> > 
> > > diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> > > index 7b41c6d..b95cd84 100644
> > > --- a/include/linux/mmc/dw_mmc.h
> > > +++ b/include/linux/mmc/dw_mmc.h
> > > @@ -14,9 +14,10 @@
> > > 
> > >  #ifndef LINUX_MMC_DW_MMC_H
> > >  #define LINUX_MMC_DW_MMC_H
> > > 
> > > -#include <linux/scatterlist.h>
> > > -#include <linux/mmc/core.h>
> > > 
> > >  #include <linux/dmaengine.h>
> > > 
> > > +#include <linux/mmc/core.h>
> > > +#include <linux/reset.h>
> > > +#include <linux/scatterlist.h>
> > 
> > unrelated changed regarding the reordering of includes.
> 
> Making them in the order of alphabetic. If you dislike, I will not add.

It's Jaehoon's call and that change above is pretty small, but generally 
introducing things unrelated to the change you actually want to make is not 
that nice - that's what separate patches are for :-) .


Heiko

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

[Jaehoon Chung]

On 04/02/2016 03:42 AM, Heiko Stuebner wrote:
> Am Mittwoch, 30. März 2016, 20:40:31 schrieb Jaehoon Chung:
>> modified Rob's mail address.
>>
>> On 03/30/2016 04:24 PM, 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.
>>
>> Do you mean mmc(card side) register or dwmmc host controller's register on
>> host side?
>>
>> According to dwmmc controller TMR, there are two reset signals. One is
>> reset_n, other is rst_n. It seems this patch is relevant to reset_n(For
>> host). (rst_n is hardware reset for card.)
>>
>> So could you clarify better? Then it's helpful to me for understanding..
> 
> I think that actually means a reset of controller IP block logic, outside 
> the control of the dw_mmc block itself.
> 
> On Rockchip SoCs this gets triggered from the CRU (clock and reset unit), so 
> I guess if I'm reading the manual correctly, should be the reset_n signal of 
> the ip block.
> 
> rst_n on the other hand gets triggered through a dw_mmc register setting and 
> is already handled by the dw_mmc driver.

Right, this patch is for reset_n signal. I didn't have seen the SoC that reset_n is designed.
(Or i didn't realize...)
If Rockchip is used from CRU (clock and reset unit), then i think that it makes sense.

Thanks for explanation. 

Best Regards,
Jaehoon Chung

> 
> 
> Heiko
> --
> 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 v3 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Guodong Xu]

On 2 April 2016 at 22:03, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Samstag, 2. April 2016, 21:39:11 schrieb Guodong Xu:
>> On 2 April 2016 at 02:42, Heiko Stuebner <heiko@sntech.de> wrote:
>> > Am Mittwoch, 30. März 2016, 15:24:56 schrieb Guodong Xu:
>> >
>> > [...]
>> >
>> > > @@ -2949,7 +2956,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)) {
>> >
>> > how is this related to adding the reset handling?
>>
>> I added this into dw_mci_parse_dt(host), and that's the first time it may
>> return -EPROBE_DEFER
>>
>>   /* find reset controller when exist */
>>   pdata->rstc = devm_reset_control_get_optional(dev, NULL);
>>
>> So, I added processing to this error in this patch.
>
> ah, you're right of course
>
>
>> > Making the driver handle probe deferral better should be a separate
>> > patch.>
>> > >                       dev_err(host->dev, "platform data not
>> >
>> > available\n");
>> >
>> > >                       return -EINVAL;
>> > >
>> > >               }
>> > >
>> > > @@ -3012,6 +3021,9 @@ int dw_mci_probe(struct dw_mci *host)
>> > >
>> > >               }
>> > >
>> > >       }
>> > >
>> > > +     if (!IS_ERR(host->pdata->rstc))
>> > > +             reset_control_deassert(host->pdata->rstc);
>> > > +
>> >
>> > Wouldn't reset_control_reset be better? The way it is now it would
>> > expect
>> > the reset to be asserted somewhere else before dw_mmc probes?
>>
>> It relates to how the SoC's reset logic is like. One bit set can clear all
>> dw_mmc host controller registers. It doesn't need do assert then
>> deassert.
>>
>> That's what I saw in hi6220 (it integrates three dw_mmc host controller),
>> drivers/reset/hisilicon/hi6220_reset.c
>> , which I wrote this patch for.
>
> I just realized again that reset_control_reset is a completely separate
> operation (not related to assert / deassert).
>
> What I was originally getting at is that I don't see any assert-counterpart.
> So if the reset-control is already deasserted, nothing will happen on some
> designs.
>
> For example on Rockchip SoCs the reset controller needs the signal to be
> high to assert the reset and the dw_mmc part of the manual explicitly says
> that the "reset_n should be asserted(active-low) for at least two clocks of
> clk or cclk_in".
>
> So I would expect something like
>
>         reset_control_assert(reset);
>         usleep_range(x, y);
>         reset_control_deassert(reset);
>
> instead of only trying to deassert the reset.
>

After confirmation with SoC hardware engineer, yeah, a correct _assert
action is expected. I will modify it as the above. Regarding
usleep_range(x, y) values, here is suggestion:

+               usleep_range(10, 50); /* 1/400kHz = 2.5us */

400kHz is the minimal bus speed for MMC. It stands for 2.5us per cycle.
10us is 4 cycles, and 50us is 20 cycles.

Does this setting make sense to you?

>
>> > >       setup_timer(&host->cmd11_timer,
>> > >
>> > >                   dw_mci_cmd11_timer, (unsigned long)host);
>> >
>> > [...]
>> >
>> > > diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> > > index 7b41c6d..b95cd84 100644
>> > > --- a/include/linux/mmc/dw_mmc.h
>> > > +++ b/include/linux/mmc/dw_mmc.h
>> > > @@ -14,9 +14,10 @@
>> > >
>> > >  #ifndef LINUX_MMC_DW_MMC_H
>> > >  #define LINUX_MMC_DW_MMC_H
>> > >
>> > > -#include <linux/scatterlist.h>
>> > > -#include <linux/mmc/core.h>
>> > >
>> > >  #include <linux/dmaengine.h>
>> > >
>> > > +#include <linux/mmc/core.h>
>> > > +#include <linux/reset.h>
>> > > +#include <linux/scatterlist.h>
>> >
>> > unrelated changed regarding the reordering of includes.
>>
>> Making them in the order of alphabetic. If you dislike, I will not add.
>
> It's Jaehoon's call and that change above is pretty small, but generally
> introducing things unrelated to the change you actually want to make is not
> that nice - that's what separate patches are for :-) .

Got your point. I will remove this. Make it simple.

-Guodong

>
>
> Heiko