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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Xinwei Kong <[email protected]>
Signed-off-by: Zhangfei Gao <[email protected]>
---
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
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
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 <[email protected]>
> Signed-off-by: Xinwei Kong <[email protected]>
> Signed-off-by: Zhangfei Gao <[email protected]>
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
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 <[email protected]>
> Acked-by: Rob Herring <[email protected]>
> ---
> 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.
>
>
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 <[email protected]>
>> Acked-by: Rob Herring <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> Acked-by: Rob Herring <[email protected]>
>>> ---
>>> 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.
>>>
>>>
>>
>>
>>
>>
>
>
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 <[email protected]
>> <mailto:[email protected]>> 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 <[email protected]
>> <mailto:[email protected]>>
>> Signed-off-by: Xinwei Kong <[email protected]
>> <mailto:[email protected]>>
>> Signed-off-by: Zhangfei Gao <[email protected]
>> <mailto:[email protected]>>
>>
>>
>> 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
>>
>>
>
>
>
在 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 <[email protected]
>>> <mailto:[email protected]>> 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 <[email protected]
>>> <mailto:[email protected]>>
>>> Signed-off-by: Xinwei Kong <[email protected]
>>> <mailto:[email protected]>>
>>> Signed-off-by: Zhangfei Gao <[email protected]
>>> <mailto:[email protected]>>
>>>
>>>
>>> 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
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 <[email protected]
>>>> <mailto:[email protected]>> 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 <[email protected]
>>>> <mailto:[email protected]>>
>>>> Signed-off-by: Xinwei Kong <[email protected]
>>>> <mailto:[email protected]>>
>>>> Signed-off-by: Zhangfei Gao <[email protected]
>>>> <mailto:[email protected]>>
>>>>
>>>>
>>>> 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
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>>
>
>
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
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
在 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
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