| Message ID | 1457254062-22677-2-git-send-email-guodong.xu@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <guodong.xu@linaro.org> > Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> Really should V2 and add the changelog. > --- > drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 242f9a0..281ea9c 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -2878,6 +2878,14 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host) > if (!pdata) > return ERR_PTR(-ENOMEM); > > + /* find reset controller when exist */ > + pdata->rstc = devm_reset_control_get_optional(dev, NULL); > + if (IS_ERR(pdata->rstc)) { > + if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER) > + return ERR_PTR(-EPROBE_DEFER); > + pdata->rstc = NULL; maybe we can remove "pdata->rstc = NULL", and directly use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)" statement > + } > + > /* find out number of slots supported */ > of_property_read_u32(np, "num-slots", &pdata->num_slots); > > @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host) > > if (!host->pdata) { > host->pdata = dw_mci_parse_dt(host); > - if (IS_ERR(host->pdata)) { > + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) > + return -EPROBE_DEFER; please fix the coding style here. > + else if (IS_ERR(host->pdata)) { > dev_err(host->dev, "platform data not available\n"); > return -EINVAL; > } > @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) > } > } > > + if (host->pdata->rstc != NULL) > + reset_control_deassert(host->pdata->rstc); > + sorry, I can't follow your intention here. Shouldn't it be something like "assert mmc -> may need delay -> deassert mmc". As your current code, nothing happend right? > setup_timer(&host->cmd11_timer, > dw_mci_cmd11_timer, (unsigned long)host); > > @@ -3164,6 +3177,9 @@ err_dmaunmap: > if (host->use_dma && host->dma_ops->exit) > host->dma_ops->exit(host); > > + if (host->pdata->rstc != NULL) > + reset_control_assert(host->pdata->rstc); > + > err_clk_ciu: > if (!IS_ERR(host->ciu_clk)) > clk_disable_unprepare(host->ciu_clk); > @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host) > if (host->use_dma && host->dma_ops->exit) > host->dma_ops->exit(host); > > + if (host->pdata->rstc != NULL) > + reset_control_assert(host->pdata->rstc); > + > if (!IS_ERR(host->ciu_clk)) > clk_disable_unprepare(host->ciu_clk); > > if (!IS_ERR(host->biu_clk)) > clk_disable_unprepare(host->biu_clk); > + > } unnecessary new line here. > EXPORT_SYMBOL(dw_mci_remove); > >
? 2016/3/25 13:35, Guodong Xu ??: > Hi, Shawn > > Sorry I replied late. I added comments below. > > On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com > <mailto:shawn.lin@rock-chips.com>> wrote: > > On 2016/3/6 16:47, Guodong Xu wrote: > > mmc registers may in abnormal state if mmc is used in bootloader, > eg. to support booting from eMMC. So we need reset mmc registers > when kernel boots up, instead of assuming mmc is in clean state. > > With this patch, user can add a 'resets' property into dw_mmc dts > node. When driver parse_dt and probe, it calls reset API to > deassert the 'reset' of dw_mmc host controller. When probe error or > remove, it calls reset API to assert it. > > Please also refer to > Documentation/devicetree/bindings/reset/reset.txt > > Signed-off-by: Guodong Xu <guodong.xu@linaro.org > <mailto:guodong.xu@linaro.org>> > Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com > <mailto:kong.kongxinwei@hisilicon.com>> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org > <mailto:zhangfei.gao@linaro.org>> > > > Really should V2 and add the changelog. > > > Yes, will do. next version I sent will be labelled as V3. > > > > --- > drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 242f9a0..281ea9c 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -2878,6 +2878,14 @@ static struct dw_mci_board > *dw_mci_parse_dt(struct dw_mci *host) > if (!pdata) > return ERR_PTR(-ENOMEM); > > + /* find reset controller when exist */ > + pdata->rstc = devm_reset_control_get_optional(dev, NULL); > + if (IS_ERR(pdata->rstc)) { > + if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER) > + return ERR_PTR(-EPROBE_DEFER); > + pdata->rstc = NULL; > > > maybe we can remove "pdata->rstc = NULL", and directly > use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)" > statement > > > Yes, will do. > I see your point, other lines in this file are using IS_ERR(!..), I will > use this style too. > > + } > + > /* find out number of slots supported */ > of_property_read_u32(np, "num-slots", &pdata->num_slots); > > @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host) > > if (!host->pdata) { > host->pdata = dw_mci_parse_dt(host); > - if (IS_ERR(host->pdata)) { > + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > > > please fix the coding style here. > > > Do you mean to add additional {} for this 'if' , like this? > > + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) { > + return -EPROBE_DEFER; > > + } > > I will add {}. > > > > + else if (IS_ERR(host->pdata)) { > dev_err(host->dev, "platform data not > available\n"); > return -EINVAL; > } > @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) > } > } > > + if (host->pdata->rstc != NULL) > + reset_control_deassert(host->pdata->rstc); > + > > > sorry, I can't follow your intention here. Shouldn't it be something > like "assert mmc -> may need delay -> deassert mmc". As your current > code, nothing happend right? > > > The chip exits from bootloader with this bit asserted. And when entering > kernel, we only need to deassert. > > In my current code, the driver deassert mmc in _probe(), and assert mmc > in _remove(). I catch your point. From the previous discussion, we add it to make sure dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in bootloader, so you can also dessert it in bootloaer to make sure dw_mmc work fine when probing in kernel. In that way, we don't need this patch? 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 > > -- 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
On 03/29/2016 11:22 AM, Shawn Lin wrote: > ? 2016/3/25 13:35, Guodong Xu ??: >> Hi, Shawn >> >> Sorry I replied late. I added comments below. >> >> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com >> <mailto:shawn.lin@rock-chips.com>> wrote: >> >> On 2016/3/6 16:47, Guodong Xu wrote: >> >> mmc registers may in abnormal state if mmc is used in bootloader, >> eg. to support booting from eMMC. So we need reset mmc registers >> when kernel boots up, instead of assuming mmc is in clean state. >> >> With this patch, user can add a 'resets' property into dw_mmc dts >> node. When driver parse_dt and probe, it calls reset API to >> deassert the 'reset' of dw_mmc host controller. When probe error or >> remove, it calls reset API to assert it. >> >> Please also refer to >> Documentation/devicetree/bindings/reset/reset.txt >> >> Signed-off-by: Guodong Xu <guodong.xu@linaro.org >> <mailto:guodong.xu@linaro.org>> >> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com >> <mailto:kong.kongxinwei@hisilicon.com>> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org >> <mailto:zhangfei.gao@linaro.org>> >> >> >> Really should V2 and add the changelog. >> >> >> Yes, will do. next version I sent will be labelled as V3. >> >> >> >> --- >> drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 242f9a0..281ea9c 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -2878,6 +2878,14 @@ static struct dw_mci_board >> *dw_mci_parse_dt(struct dw_mci *host) >> if (!pdata) >> return ERR_PTR(-ENOMEM); >> >> + /* find reset controller when exist */ >> + pdata->rstc = devm_reset_control_get_optional(dev, NULL); >> + if (IS_ERR(pdata->rstc)) { >> + if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER) >> + return ERR_PTR(-EPROBE_DEFER); >> + pdata->rstc = NULL; >> >> >> maybe we can remove "pdata->rstc = NULL", and directly >> use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)" >> statement >> >> >> Yes, will do. >> I see your point, other lines in this file are using IS_ERR(!..), I will >> use this style too. >> >> + } >> + >> /* find out number of slots supported */ >> of_property_read_u32(np, "num-slots", &pdata->num_slots); >> >> @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host) >> >> if (!host->pdata) { >> host->pdata = dw_mci_parse_dt(host); >> - if (IS_ERR(host->pdata)) { >> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> >> >> please fix the coding style here. >> >> >> Do you mean to add additional {} for this 'if' , like this? >> >> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) { >> + return -EPROBE_DEFER; >> >> + } >> >> I will add {}. >> >> >> >> + else if (IS_ERR(host->pdata)) { >> dev_err(host->dev, "platform data not >> available\n"); >> return -EINVAL; >> } >> @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) >> } >> } >> >> + if (host->pdata->rstc != NULL) >> + reset_control_deassert(host->pdata->rstc); >> + >> >> >> sorry, I can't follow your intention here. Shouldn't it be something >> like "assert mmc -> may need delay -> deassert mmc". As your current >> code, nothing happend right? >> >> >> The chip exits from bootloader with this bit asserted. And when entering >> kernel, we only need to deassert. >> >> In my current code, the driver deassert mmc in _probe(), and assert mmc >> in _remove(). > > I catch your point. From the previous discussion, we add it to make sure > dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in bootloader, so you can also > dessert it in bootloaer to make sure dw_mmc work fine when probing > in kernel. In that way, we don't need this patch? Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET. Could you check this? Best Regards, Jaehoon Chung > > More to think, Is it ok to match the behaviour of bootloader stage? > My bootloader doesn't assert the reset pin of dw_mmc, so it seams if > I want to fix you issue on kernel stage, I need a new round of > assert->delay->deassert. > > >> >> >> >> setup_timer(&host->cmd11_timer, >> dw_mci_cmd11_timer, (unsigned long)host); >> >> @@ -3164,6 +3177,9 @@ err_dmaunmap: >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> >> + if (host->pdata->rstc != NULL) >> + reset_control_assert(host->pdata->rstc); >> + >> err_clk_ciu: >> if (!IS_ERR(host->ciu_clk)) >> clk_disable_unprepare(host->ciu_clk); >> @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host) >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> >> + if (host->pdata->rstc != NULL) >> + reset_control_assert(host->pdata->rstc); >> + >> if (!IS_ERR(host->ciu_clk)) >> clk_disable_unprepare(host->ciu_clk); >> >> if (!IS_ERR(host->biu_clk)) >> clk_disable_unprepare(host->biu_clk); >> + >> } >> >> >> unnecessary new line here. >> >> >> Will fix. >> >> -Guodong >> >> >> EXPORT_SYMBOL(dw_mci_remove); >> >> >> >> >> -- >> Best Regards >> Shawn Lin >> >> > > > -- 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
? 2016/3/29 13:56, Jaehoon Chung ??: > On 03/29/2016 11:22 AM, Shawn Lin wrote: >> ? 2016/3/25 13:35, Guodong Xu ??: >>> Hi, Shawn >>> >>> Sorry I replied late. I added comments below. >>> >>> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com >>> <mailto:shawn.lin@rock-chips.com>> wrote: >>> >>> On 2016/3/6 16:47, Guodong Xu wrote: >>> >>> mmc registers may in abnormal state if mmc is used in bootloader, >>> eg. to support booting from eMMC. So we need reset mmc registers >>> when kernel boots up, instead of assuming mmc is in clean state. >>> >>> With this patch, user can add a 'resets' property into dw_mmc dts >>> node. When driver parse_dt and probe, it calls reset API to >>> deassert the 'reset' of dw_mmc host controller. When probe error or >>> remove, it calls reset API to assert it. >>> >>> Please also refer to >>> Documentation/devicetree/bindings/reset/reset.txt >>> >>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org >>> <mailto:guodong.xu@linaro.org>> >>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com >>> <mailto:kong.kongxinwei@hisilicon.com>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org >>> <mailto:zhangfei.gao@linaro.org>> >>> >>> >>> Really should V2 and add the changelog. >>> >>> >>> Yes, will do. next version I sent will be labelled as V3. >>> >>> >>> >>> --- >>> drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++- >>> 1 file changed, 21 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index 242f9a0..281ea9c 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -2878,6 +2878,14 @@ static struct dw_mci_board >>> *dw_mci_parse_dt(struct dw_mci *host) >>> if (!pdata) >>> return ERR_PTR(-ENOMEM); >>> >>> + /* find reset controller when exist */ >>> + pdata->rstc = devm_reset_control_get_optional(dev, NULL); >>> + if (IS_ERR(pdata->rstc)) { >>> + if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER) >>> + return ERR_PTR(-EPROBE_DEFER); >>> + pdata->rstc = NULL; >>> >>> >>> maybe we can remove "pdata->rstc = NULL", and directly >>> use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)" >>> statement >>> >>> >>> Yes, will do. >>> I see your point, other lines in this file are using IS_ERR(!..), I will >>> use this style too. >>> >>> + } >>> + >>> /* find out number of slots supported */ >>> of_property_read_u32(np, "num-slots", &pdata->num_slots); >>> >>> @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host) >>> >>> if (!host->pdata) { >>> host->pdata = dw_mci_parse_dt(host); >>> - if (IS_ERR(host->pdata)) { >>> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> >>> >>> please fix the coding style here. >>> >>> >>> Do you mean to add additional {} for this 'if' , like this? >>> >>> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) { >>> + return -EPROBE_DEFER; >>> >>> + } >>> >>> I will add {}. >>> >>> >>> >>> + else if (IS_ERR(host->pdata)) { >>> dev_err(host->dev, "platform data not >>> available\n"); >>> return -EINVAL; >>> } >>> @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) >>> } >>> } >>> >>> + if (host->pdata->rstc != NULL) >>> + reset_control_deassert(host->pdata->rstc); >>> + >>> >>> >>> sorry, I can't follow your intention here. Shouldn't it be something >>> like "assert mmc -> may need delay -> deassert mmc". As your current >>> code, nothing happend right? >>> >>> >>> The chip exits from bootloader with this bit asserted. And when entering >>> kernel, we only need to deassert. >>> >>> In my current code, the driver deassert mmc in _probe(), and assert mmc >>> in _remove(). >> >> I catch your point. From the previous discussion, we add it to make sure >> dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in bootloader, so you can also >> dessert it in bootloaer to make sure dw_mmc work fine when probing >> in kernel. In that way, we don't need this patch? > > Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET. > Could you check this? > MMC_CAP_HW_RESET actually reset the mmc card, but Guodong means to reset the controller rather than mmc card :) > Best Regards, > Jaehoon Chung > >> >> More to think, Is it ok to match the behaviour of bootloader stage? >> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if >> I want to fix you issue on kernel stage, I need a new round of >> assert->delay->deassert. >> >> >>> >>> >>> >>> setup_timer(&host->cmd11_timer, >>> dw_mci_cmd11_timer, (unsigned long)host); >>> >>> @@ -3164,6 +3177,9 @@ err_dmaunmap: >>> if (host->use_dma && host->dma_ops->exit) >>> host->dma_ops->exit(host); >>> >>> + if (host->pdata->rstc != NULL) >>> + reset_control_assert(host->pdata->rstc); >>> + >>> err_clk_ciu: >>> if (!IS_ERR(host->ciu_clk)) >>> clk_disable_unprepare(host->ciu_clk); >>> @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host) >>> if (host->use_dma && host->dma_ops->exit) >>> host->dma_ops->exit(host); >>> >>> + if (host->pdata->rstc != NULL) >>> + reset_control_assert(host->pdata->rstc); >>> + >>> if (!IS_ERR(host->ciu_clk)) >>> clk_disable_unprepare(host->ciu_clk); >>> >>> if (!IS_ERR(host->biu_clk)) >>> clk_disable_unprepare(host->biu_clk); >>> + >>> } >>> >>> >>> unnecessary new line here. >>> >>> >>> Will fix. >>> >>> -Guodong >>> >>> >>> EXPORT_SYMBOL(dw_mci_remove); >>> >>> >>> >>> >>> -- >>> Best Regards >>> Shawn Lin >>> >>> >> >> >> > > > >
On 03/29/2016 03:09 PM, Shawn Lin wrote: > ? 2016/3/29 13:56, Jaehoon Chung ??: >> On 03/29/2016 11:22 AM, Shawn Lin wrote: >>> ? 2016/3/25 13:35, Guodong Xu ??: >>>> Hi, Shawn >>>> >>>> Sorry I replied late. I added comments below. >>>> >>>> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@rock-chips.com >>>> <mailto:shawn.lin@rock-chips.com>> wrote: >>>> >>>> On 2016/3/6 16:47, Guodong Xu wrote: >>>> >>>> mmc registers may in abnormal state if mmc is used in bootloader, >>>> eg. to support booting from eMMC. So we need reset mmc registers >>>> when kernel boots up, instead of assuming mmc is in clean state. >>>> >>>> With this patch, user can add a 'resets' property into dw_mmc dts >>>> node. When driver parse_dt and probe, it calls reset API to >>>> deassert the 'reset' of dw_mmc host controller. When probe error or >>>> remove, it calls reset API to assert it. >>>> >>>> Please also refer to >>>> Documentation/devicetree/bindings/reset/reset.txt >>>> >>>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org >>>> <mailto:guodong.xu@linaro.org>> >>>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com >>>> <mailto:kong.kongxinwei@hisilicon.com>> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org >>>> <mailto:zhangfei.gao@linaro.org>> >>>> >>>> >>>> Really should V2 and add the changelog. >>>> >>>> >>>> Yes, will do. next version I sent will be labelled as V3. >>>> >>>> >>>> >>>> --- >>>> drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++- >>>> 1 file changed, 21 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>>> index 242f9a0..281ea9c 100644 >>>> --- a/drivers/mmc/host/dw_mmc.c >>>> +++ b/drivers/mmc/host/dw_mmc.c >>>> @@ -2878,6 +2878,14 @@ static struct dw_mci_board >>>> *dw_mci_parse_dt(struct dw_mci *host) >>>> if (!pdata) >>>> return ERR_PTR(-ENOMEM); >>>> >>>> + /* find reset controller when exist */ >>>> + pdata->rstc = devm_reset_control_get_optional(dev, NULL); >>>> + if (IS_ERR(pdata->rstc)) { >>>> + if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER) >>>> + return ERR_PTR(-EPROBE_DEFER); >>>> + pdata->rstc = NULL; >>>> >>>> >>>> maybe we can remove "pdata->rstc = NULL", and directly >>>> use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)" >>>> statement >>>> >>>> >>>> Yes, will do. >>>> I see your point, other lines in this file are using IS_ERR(!..), I will >>>> use this style too. >>>> >>>> + } >>>> + >>>> /* find out number of slots supported */ >>>> of_property_read_u32(np, "num-slots", &pdata->num_slots); >>>> >>>> @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host) >>>> >>>> if (!host->pdata) { >>>> host->pdata = dw_mci_parse_dt(host); >>>> - if (IS_ERR(host->pdata)) { >>>> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) >>>> + return -EPROBE_DEFER; >>>> >>>> >>>> please fix the coding style here. >>>> >>>> >>>> Do you mean to add additional {} for this 'if' , like this? >>>> >>>> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) { >>>> + return -EPROBE_DEFER; >>>> >>>> + } >>>> >>>> I will add {}. >>>> >>>> >>>> >>>> + else if (IS_ERR(host->pdata)) { >>>> dev_err(host->dev, "platform data not >>>> available\n"); >>>> return -EINVAL; >>>> } >>>> @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) >>>> } >>>> } >>>> >>>> + if (host->pdata->rstc != NULL) >>>> + reset_control_deassert(host->pdata->rstc); >>>> + >>>> >>>> >>>> sorry, I can't follow your intention here. Shouldn't it be something >>>> like "assert mmc -> may need delay -> deassert mmc". As your current >>>> code, nothing happend right? >>>> >>>> >>>> The chip exits from bootloader with this bit asserted. And when entering >>>> kernel, we only need to deassert. >>>> >>>> In my current code, the driver deassert mmc in _probe(), and assert mmc >>>> in _remove(). >>> >>> I catch your point. From the previous discussion, we add it to make sure >>> dw_mmc in good state after leaving bootloader to kernel. But My real question is that you can assert it in bootloader, so you can also >>> dessert it in bootloaer to make sure dw_mmc work fine when probing >>> in kernel. In that way, we don't need this patch? >> >> Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET. >> Could you check this? >> > > MMC_CAP_HW_RESET actually reset the mmc card, but Guodong means to > reset the controller rather than mmc card :) We have talked about FBE scenarios, right? :) Now, I remembered it. > > >> Best Regards, >> Jaehoon Chung >> >>> >>> More to think, Is it ok to match the behaviour of bootloader stage? >>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if >>> I want to fix you issue on kernel stage, I need a new round of >>> assert->delay->deassert. >>> >>> >>>> >>>> >>>> >>>> setup_timer(&host->cmd11_timer, >>>> dw_mci_cmd11_timer, (unsigned long)host); >>>> >>>> @@ -3164,6 +3177,9 @@ err_dmaunmap: >>>> if (host->use_dma && host->dma_ops->exit) >>>> host->dma_ops->exit(host); >>>> >>>> + if (host->pdata->rstc != NULL) >>>> + reset_control_assert(host->pdata->rstc); >>>> + >>>> err_clk_ciu: >>>> if (!IS_ERR(host->ciu_clk)) >>>> clk_disable_unprepare(host->ciu_clk); >>>> @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host) >>>> if (host->use_dma && host->dma_ops->exit) >>>> host->dma_ops->exit(host); >>>> >>>> + if (host->pdata->rstc != NULL) >>>> + reset_control_assert(host->pdata->rstc); >>>> + >>>> if (!IS_ERR(host->ciu_clk)) >>>> clk_disable_unprepare(host->ciu_clk); >>>> >>>> if (!IS_ERR(host->biu_clk)) >>>> clk_disable_unprepare(host->biu_clk); >>>> + >>>> } >>>> >>>> >>>> unnecessary new line here. >>>> >>>> >>>> Will fix. >>>> >>>> -Guodong >>>> >>>> >>>> EXPORT_SYMBOL(dw_mci_remove); >>>> >>>> >>>> >>>> >>>> -- >>>> Best Regards >>>> Shawn Lin >>>> >>>> >>> >>> >>> >> >> >> >> > > -- 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
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 -- 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
On 03/29/2016 05:23 PM, zhangfei wrote: > > > On 03/29/2016 10:22 AM, Shawn Lin wrote: > >>> >>> >>> >>> + else if (IS_ERR(host->pdata)) { >>> dev_err(host->dev, "platform data not >>> available\n"); >>> return -EINVAL; >>> } >>> @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) >>> } >>> } >>> >>> + if (host->pdata->rstc != NULL) >>> + reset_control_deassert(host->pdata->rstc); >>> + >>> >>> >>> sorry, I can't follow your intention here. Shouldn't it be something >>> like "assert mmc -> may need delay -> deassert mmc". As your current >>> code, nothing happend right? > Should be abstracted in reset driver. > >>> >>> >>> The chip exits from bootloader with this bit asserted. And when entering >>> kernel, we only need to deassert. >>> >>> In my current code, the driver deassert mmc in _probe(), and assert mmc >>> in _remove(). >> >> I catch your point. From the previous discussion, we add it to make sure >> dw_mmc in good state after leaving bootloader to kernel. But My real >> question is that you can assert it in bootloader, so you can also >> dessert it in bootloaer to make sure dw_mmc work fine when probing >> in kernel. In that way, we don't need this patch? > > uefi does not have exit point, and kernel may not assume mmc controller state is always correct when boot. > If Uefi need copy Image from mmc, mmc controller is in working state. > When jump to kernel, uefi mmc driver can not recover itself. > If kernel assume mmc controller state is clean, mmc will be in abnormal state. > Some controller will clear itself when set clock, however, hip660 does not, it need special register to access. > > >> >> More to think, Is it ok to match the behaviour of bootloader stage? >> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if >> I want to fix you issue on kernel stage, I need a new round of >> assert->delay->deassert. > > The process like delay (if required) should be abstracted in reset driver. > reset framework just export reset_control_assert/reset_control_deassert API. > Unfortunately not find clear description in Documentation/. > Suppose deassert is like start, while assert is like stop. First, this patch need to resend after fixing. Could you or Guodong resend these patches as V2 or V3? Best Regards, Jaehoon Chung > > drivers/reset/core.c > reset_control_deassert - deasserts the reset line > reset_control_assert - asserts the reset line > > More example: > drivers/mmc/host/sdhci-st.c > drivers/mmc/host/sunxi-mmc.c > drivers/usb/host/ohci-platform.c > drivers/i2c/busses/i2c-mv64xxx.c > > Thanks > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- 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
? 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 > > > >
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 -- 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
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);