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

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 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

[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
> 
>