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

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

[Guodong Xu]

With this, user can add a 'resets' property into dw_mmc dts
node, and when driver probe and parse_dt, it will call
reset APIs to reset dw_mmc host controller.

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  | 7 +++++++
 include/linux/mmc/dw_mmc.h | 6 ++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 242f9a0..d3a7376 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))
+		pdata->rstc = NULL;
+	else
+		reset_control_deassert(pdata->rstc);
+
 	/* find out number of slots supported */
 	of_property_read_u32(np, "num-slots", &pdata->num_slots);
 
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 2/2] mmc: dw_mmc: add resets support to dw_mci_parse_dt()

[Shawn Lin]

Hi Guodong,

On 2016/3/3 9:33, Guodong Xu wrote:
> With this, user can add a 'resets' property into dw_mmc dts
> node, and when driver probe and parse_dt, it will call
> reset APIs to reset dw_mmc host controller.
>
> Please also refer to Documentation/devicetree/bindings/reset/reset.txt

I have no hard objection for this patch, but I'd rather not add it
unless we actually need it. Could you elaborate more about any futher
actions you will take if it's applied(i.e: deal with some unrecoverable
broken case)?  If we just reset the controller while probing, actually I
can't find any problems without it based on tons of my reboot test.


>
> 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  | 7 +++++++
>   include/linux/mmc/dw_mmc.h | 6 ++++--
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 242f9a0..d3a7376 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))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +

if the PTR_ERR(pdata->rstc) is -EPROBE_DEFER, should we defer probing
the driver?

>   	/* find out number of slots supported */
>   	of_property_read_u32(np, "num-slots", &pdata->num_slots);
>
> 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;
>   };
>


-- 
Best Regards
Shawn Lin

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

[zhangfei]

On 03/03/2016 10:00 AM, Shawn Lin wrote:
> Hi Guodong,
>
> On 2016/3/3 9:33, Guodong Xu wrote:
>> With this, user can add a 'resets' property into dw_mmc dts
>> node, and when driver probe and parse_dt, it will call
>> reset APIs to reset dw_mmc host controller.
>>
>> Please also refer to Documentation/devicetree/bindings/reset/reset.txt
>
> I have no hard objection for this patch, but I'd rather not add it
> unless we actually need it. Could you elaborate more about any futher
> actions you will take if it's applied(i.e: deal with some unrecoverable
> broken case)?  If we just reset the controller while probing, actually I
> can't find any problems without it based on tons of my reboot test.

mmc register maybe abnormal state, if mmc is used in uefi, like boot 
from emmc.
So we need reset mmc register when kernel boot up, instead of assuming 
mmc is in clean state.
Next step is adding reset node in dts for drivers/mmc/host/dw_mmc-k3.c 
as reset driver is already there.

>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 242f9a0..d3a7376 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))
>> +        pdata->rstc = NULL;
>> +    else
>> +        reset_control_deassert(pdata->rstc);
>> +
>
> if the PTR_ERR(pdata->rstc) is -EPROBE_DEFER, should we defer probing
> the driver?
Yes, good catch.

Thanks

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

[Shawn Lin]

在 2016/3/4 11:27, zhangfei 写道:
>
>
> On 03/03/2016 10:00 AM, Shawn Lin wrote:
>> Hi Guodong,
>>
>> On 2016/3/3 9:33, Guodong Xu wrote:
>>> With this, user can add a 'resets' property into dw_mmc dts
>>> node, and when driver probe and parse_dt, it will call
>>> reset APIs to reset dw_mmc host controller.
>>>
>>> Please also refer to Documentation/devicetree/bindings/reset/reset.txt
>>
>> I have no hard objection for this patch, but I'd rather not add it
>> unless we actually need it. Could you elaborate more about any futher
>> actions you will take if it's applied(i.e: deal with some unrecoverable
>> broken case)?  If we just reset the controller while probing, actually I
>> can't find any problems without it based on tons of my reboot test.
>
> mmc register maybe abnormal state, if mmc is used in uefi, like boot
> from emmc.
> So we need reset mmc register when kernel boot up, instead of assuming
> mmc is in clean state.
> Next step is adding reset node in dts for drivers/mmc/host/dw_mmc-k3.c
> as reset driver is already there.

Fair enough. Thanks for explaining.

>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 242f9a0..d3a7376 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))
>>> +        pdata->rstc = NULL;
>>> +    else
>>> +        reset_control_deassert(pdata->rstc);
>>> +
>>
>> if the PTR_ERR(pdata->rstc) is -EPROBE_DEFER, should we defer probing
>> the driver?
> Yes, good catch.
>
> Thanks
>
>
>


-- 
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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>

[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

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