| Message ID | 1422346289-9348-2-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
Hi, Marek. your patch should be conflicted with "https://patchwork.kernel.org/patch/5698421/" On 01/27/2015 05:11 PM, Marek Szyprowski wrote: > There are boards (like Hardkernel's Odroid boards) on which eMMC card's > reset line is connected to SoC GPIO line instead of the hardware reset > logic. In case of such boards, before performing system reboot, > additional reset of eMMC card is required to boot again properly. > This patch adds code for handling such cases. mmc core supported to hw_reset function. So i think we can use it. It's called at only initial time to clear the previous status. But i think it can be called at reboot time. (it needs to implement codes.) how about? Best Regards, Jaehoon Chung > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > .../devicetree/bindings/mmc/exynos-dw-mshc.txt | 6 +++ > drivers/mmc/host/dw_mmc-exynos.c | 43 +++++++++++++++++++++- > 2 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt > index ee4fc0576c7d..fc53d335e7db 100644 > --- a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt > +++ b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt > @@ -50,6 +50,12 @@ Required Properties: > - if CIU clock divider value is 0 (that is divide by 1), both tx and rx > phase shift clocks should be 0. > > +Optional properties: > + > +* dw-mshc-reset-gpios: optional property specifying gpio for the eMMC nreset > + line, it will be triggered on system reboot to properly reset eMMC card for > + next system boot. > + > Required properties for a slot (Deprecated - Recommend to use one slot per host): > > * gpios: specifies a list of gpios used for command, clock and data bus. The > diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c > index 509365cb22c6..2add5a93859d 100644 > --- a/drivers/mmc/host/dw_mmc-exynos.c > +++ b/drivers/mmc/host/dw_mmc-exynos.c > @@ -12,12 +12,14 @@ > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/clk.h> > +#include <linux/delay.h> > #include <linux/mmc/host.h> > #include <linux/mmc/dw_mmc.h> > #include <linux/mmc/mmc.h> > #include <linux/of.h> > #include <linux/of_gpio.h> > #include <linux/slab.h> > +#include <linux/reboot.h> > > #include "dw_mmc.h" > #include "dw_mmc-pltfm.h" > @@ -77,8 +79,23 @@ struct dw_mci_exynos_priv_data { > u32 sdr_timing; > u32 ddr_timing; > u32 cur_speed; > + struct gpio_desc *reset_gpio; > + struct notifier_block reset_nb; > }; > > +static int dw_mci_restart_handler(struct notifier_block *this, > + unsigned long mode, void *cmd) > +{ > + struct dw_mci_exynos_priv_data *data; > + data = container_of(this, struct dw_mci_exynos_priv_data, reset_nb); > + > + gpiod_direction_output(data->reset_gpio, 0); > + mdelay(150); > + gpiod_direction_output(data->reset_gpio, 1); > + > + return NOTIFY_DONE; > +} > + > static struct dw_mci_exynos_compatible { > char *compatible; > enum dw_mci_exynos_type ctrl_type; > @@ -295,7 +312,20 @@ static int dw_mci_exynos_parse_dt(struct dw_mci *host) > return ret; > > priv->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0], timing[1], div); > + > + priv->reset_gpio = devm_gpiod_get_optional(host->dev, > + "samsung,dw-mshc-reset", > + GPIOD_OUT_HIGH); > + if (!IS_ERR_OR_NULL(priv->reset_gpio)) { > + priv->reset_nb.notifier_call = dw_mci_restart_handler; > + priv->reset_nb.priority = 255; > + ret = register_restart_handler(&priv->reset_nb); > + if (ret) > + dev_err(host->dev, "cannot register restart handler\n"); > + } > + > host->priv = priv; > + > return 0; > } > > @@ -490,6 +520,17 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) > return dw_mci_pltfm_register(pdev, drv_data); > } > > +static int dw_mci_exynos_remove(struct platform_device *pdev) > +{ > + struct dw_mci *host = platform_get_drvdata(pdev); > + struct dw_mci_exynos_priv_data *priv = host->priv; > + > + if (priv->reset_gpio) > + unregister_restart_handler(&priv->reset_nb); > + > + return dw_mci_pltfm_remove(pdev); > +} > + > static const struct dev_pm_ops dw_mci_exynos_pmops = { > SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) > .resume_noirq = dw_mci_exynos_resume_noirq, > @@ -499,7 +540,7 @@ static const struct dev_pm_ops dw_mci_exynos_pmops = { > > static struct platform_driver dw_mci_exynos_pltfm_driver = { > .probe = dw_mci_exynos_probe, > - .remove = __exit_p(dw_mci_pltfm_remove), > + .remove = dw_mci_exynos_remove, > .driver = { > .name = "dwmmc_exynos", > .of_match_table = dw_mci_exynos_match, > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello! Jaehoon Chung wrote: > mmc core supported to hw_reset function. > So i think we can use it. It's called at only initial time to clear the previous status. > But i think it can be called at reboot time. (it needs to implement codes.) > how about? I don't think that's going the work. The problem here is that depending on the board configuration, the eMMC might carry the bootloader. If the eMMC subsystem is not properly reset _during_ reboot, the board won't even start since no bootloader is found. So we don't even reach the point where the kernel does anything. With best wishes, Tobias > > Best Regards, > Jaehoon Chung > >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> .../devicetree/bindings/mmc/exynos-dw-mshc.txt | 6 +++ >> drivers/mmc/host/dw_mmc-exynos.c | 43 +++++++++++++++++++++- >> 2 files changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt >> index ee4fc0576c7d..fc53d335e7db 100644 >> --- a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt >> +++ b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt >> @@ -50,6 +50,12 @@ Required Properties: >> - if CIU clock divider value is 0 (that is divide by 1), both tx and rx >> phase shift clocks should be 0. >> >> +Optional properties: >> + >> +* dw-mshc-reset-gpios: optional property specifying gpio for the eMMC nreset >> + line, it will be triggered on system reboot to properly reset eMMC card for >> + next system boot. >> + >> Required properties for a slot (Deprecated - Recommend to use one slot per host): >> >> * gpios: specifies a list of gpios used for command, clock and data bus. The >> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c >> index 509365cb22c6..2add5a93859d 100644 >> --- a/drivers/mmc/host/dw_mmc-exynos.c >> +++ b/drivers/mmc/host/dw_mmc-exynos.c >> @@ -12,12 +12,14 @@ >> #include <linux/module.h> >> #include <linux/platform_device.h> >> #include <linux/clk.h> >> +#include <linux/delay.h> >> #include <linux/mmc/host.h> >> #include <linux/mmc/dw_mmc.h> >> #include <linux/mmc/mmc.h> >> #include <linux/of.h> >> #include <linux/of_gpio.h> >> #include <linux/slab.h> >> +#include <linux/reboot.h> >> >> #include "dw_mmc.h" >> #include "dw_mmc-pltfm.h" >> @@ -77,8 +79,23 @@ struct dw_mci_exynos_priv_data { >> u32 sdr_timing; >> u32 ddr_timing; >> u32 cur_speed; >> + struct gpio_desc *reset_gpio; >> + struct notifier_block reset_nb; >> }; >> >> +static int dw_mci_restart_handler(struct notifier_block *this, >> + unsigned long mode, void *cmd) >> +{ >> + struct dw_mci_exynos_priv_data *data; >> + data = container_of(this, struct dw_mci_exynos_priv_data, reset_nb); >> + >> + gpiod_direction_output(data->reset_gpio, 0); >> + mdelay(150); >> + gpiod_direction_output(data->reset_gpio, 1); >> + >> + return NOTIFY_DONE; >> +} >> + >> static struct dw_mci_exynos_compatible { >> char *compatible; >> enum dw_mci_exynos_type ctrl_type; >> @@ -295,7 +312,20 @@ static int dw_mci_exynos_parse_dt(struct dw_mci *host) >> return ret; >> >> priv->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0], timing[1], div); >> + >> + priv->reset_gpio = devm_gpiod_get_optional(host->dev, >> + "samsung,dw-mshc-reset", >> + GPIOD_OUT_HIGH); >> + if (!IS_ERR_OR_NULL(priv->reset_gpio)) { >> + priv->reset_nb.notifier_call = dw_mci_restart_handler; >> + priv->reset_nb.priority = 255; >> + ret = register_restart_handler(&priv->reset_nb); >> + if (ret) >> + dev_err(host->dev, "cannot register restart handler\n"); >> + } >> + >> host->priv = priv; >> + >> return 0; >> } >> >> @@ -490,6 +520,17 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) >> return dw_mci_pltfm_register(pdev, drv_data); >> } >> >> +static int dw_mci_exynos_remove(struct platform_device *pdev) >> +{ >> + struct dw_mci *host = platform_get_drvdata(pdev); >> + struct dw_mci_exynos_priv_data *priv = host->priv; >> + >> + if (priv->reset_gpio) >> + unregister_restart_handler(&priv->reset_nb); >> + >> + return dw_mci_pltfm_remove(pdev); >> +} >> + >> static const struct dev_pm_ops dw_mci_exynos_pmops = { >> SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) >> .resume_noirq = dw_mci_exynos_resume_noirq, >> @@ -499,7 +540,7 @@ static const struct dev_pm_ops dw_mci_exynos_pmops = { >> >> static struct platform_driver dw_mci_exynos_pltfm_driver = { >> .probe = dw_mci_exynos_probe, >> - .remove = __exit_p(dw_mci_pltfm_remove), >> + .remove = dw_mci_exynos_remove, >> .driver = { >> .name = "dwmmc_exynos", >> .of_match_table = dw_mci_exynos_match, >> > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 January 2015 at 13:41, Tobias Jakobi <liquid.acid@gmx.net> wrote: > Hello! > > Jaehoon Chung wrote: >> mmc core supported to hw_reset function. >> So i think we can use it. It's called at only initial time to clear the previous status. >> But i think it can be called at reboot time. (it needs to implement codes.) >> how about? > I don't think that's going the work. The problem here is that depending > on the board configuration, the eMMC might carry the bootloader. If the > eMMC subsystem is not properly reset _during_ reboot, the board won't > even start since no bootloader is found. So we don't even reach the > point where the kernel does anything. I guess it depends on what's being done during the reboot sequence. The most proper thing would be to let the boot loader control the GPIO to trigger the HW reset, but that would mean the boot loader need to know about board specific configurations, such as which GPIO pin. So maybe SOC vendors need to state what GPIO pin to use and don't leave that as a configurable option. >From kernel perspective, the best we can do is to the GPIO, when doing a controlled reset (soft reset, or whatever we call it), but I am not sure where that should be done? Is there a guarantee that the mmc bus' ->shutdown() callback gets called in this sequence? Moreover, adding the reset GPIO as part of the initialization procedure in the mmc core, gives us other benefits and it won't hurt. So no matter, I think it's worth to proceed and discuss Marek's proposal. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson wrote: > On 28 January 2015 at 13:41, Tobias Jakobi <liquid.acid@gmx.net> wrote: >> Hello! >> >> Jaehoon Chung wrote: >>> mmc core supported to hw_reset function. >>> So i think we can use it. It's called at only initial time to clear the previous status. >>> But i think it can be called at reboot time. (it needs to implement codes.) >>> how about? >> I don't think that's going the work. The problem here is that depending >> on the board configuration, the eMMC might carry the bootloader. If the >> eMMC subsystem is not properly reset _during_ reboot, the board won't >> even start since no bootloader is found. So we don't even reach the >> point where the kernel does anything. > > I guess it depends on what's being done during the reboot sequence. > > The most proper thing would be to let the boot loader control the GPIO > to trigger the HW reset, but that would mean the boot loader need to > know about board specific configurations, such as which GPIO pin. So > maybe SOC vendors need to state what GPIO pin to use and don't leave > that as a configurable option. Not the bootloader, but the iROM code would have to do this, and as far as I know the iROM can't be modified. Or am I missing something here? With best wishes, Tobias -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 January 2015 at 16:23, Tobias Jakobi <liquid.acid@gmx.net> wrote: > Ulf Hansson wrote: >> On 28 January 2015 at 13:41, Tobias Jakobi <liquid.acid@gmx.net> wrote: >>> Hello! >>> >>> Jaehoon Chung wrote: >>>> mmc core supported to hw_reset function. >>>> So i think we can use it. It's called at only initial time to clear the previous status. >>>> But i think it can be called at reboot time. (it needs to implement codes.) >>>> how about? >>> I don't think that's going the work. The problem here is that depending >>> on the board configuration, the eMMC might carry the bootloader. If the >>> eMMC subsystem is not properly reset _during_ reboot, the board won't >>> even start since no bootloader is found. So we don't even reach the >>> point where the kernel does anything. >> >> I guess it depends on what's being done during the reboot sequence. >> >> The most proper thing would be to let the boot loader control the GPIO >> to trigger the HW reset, but that would mean the boot loader need to >> know about board specific configurations, such as which GPIO pin. So >> maybe SOC vendors need to state what GPIO pin to use and don't leave >> that as a configurable option. > Not the bootloader, but the iROM code would have to do this, and as far > as I know the iROM can't be modified. Or am I missing something here? You are right! The "bootloader" can very likely be stored in ROM code. That's why I also said the GPIO shouldn't be configurable. :-) Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt index ee4fc0576c7d..fc53d335e7db 100644 --- a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt @@ -50,6 +50,12 @@ Required Properties: - if CIU clock divider value is 0 (that is divide by 1), both tx and rx phase shift clocks should be 0. +Optional properties: + +* dw-mshc-reset-gpios: optional property specifying gpio for the eMMC nreset + line, it will be triggered on system reboot to properly reset eMMC card for + next system boot. + Required properties for a slot (Deprecated - Recommend to use one slot per host): * gpios: specifies a list of gpios used for command, clock and data bus. The diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 509365cb22c6..2add5a93859d 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -12,12 +12,14 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/clk.h> +#include <linux/delay.h> #include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/mmc/mmc.h> #include <linux/of.h> #include <linux/of_gpio.h> #include <linux/slab.h> +#include <linux/reboot.h> #include "dw_mmc.h" #include "dw_mmc-pltfm.h" @@ -77,8 +79,23 @@ struct dw_mci_exynos_priv_data { u32 sdr_timing; u32 ddr_timing; u32 cur_speed; + struct gpio_desc *reset_gpio; + struct notifier_block reset_nb; }; +static int dw_mci_restart_handler(struct notifier_block *this, + unsigned long mode, void *cmd) +{ + struct dw_mci_exynos_priv_data *data; + data = container_of(this, struct dw_mci_exynos_priv_data, reset_nb); + + gpiod_direction_output(data->reset_gpio, 0); + mdelay(150); + gpiod_direction_output(data->reset_gpio, 1); + + return NOTIFY_DONE; +} + static struct dw_mci_exynos_compatible { char *compatible; enum dw_mci_exynos_type ctrl_type; @@ -295,7 +312,20 @@ static int dw_mci_exynos_parse_dt(struct dw_mci *host) return ret; priv->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0], timing[1], div); + + priv->reset_gpio = devm_gpiod_get_optional(host->dev, + "samsung,dw-mshc-reset", + GPIOD_OUT_HIGH); + if (!IS_ERR_OR_NULL(priv->reset_gpio)) { + priv->reset_nb.notifier_call = dw_mci_restart_handler; + priv->reset_nb.priority = 255; + ret = register_restart_handler(&priv->reset_nb); + if (ret) + dev_err(host->dev, "cannot register restart handler\n"); + } + host->priv = priv; + return 0; } @@ -490,6 +520,17 @@ static int dw_mci_exynos_probe(struct platform_device *pdev) return dw_mci_pltfm_register(pdev, drv_data); } +static int dw_mci_exynos_remove(struct platform_device *pdev) +{ + struct dw_mci *host = platform_get_drvdata(pdev); + struct dw_mci_exynos_priv_data *priv = host->priv; + + if (priv->reset_gpio) + unregister_restart_handler(&priv->reset_nb); + + return dw_mci_pltfm_remove(pdev); +} + static const struct dev_pm_ops dw_mci_exynos_pmops = { SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume) .resume_noirq = dw_mci_exynos_resume_noirq, @@ -499,7 +540,7 @@ static const struct dev_pm_ops dw_mci_exynos_pmops = { static struct platform_driver dw_mci_exynos_pltfm_driver = { .probe = dw_mci_exynos_probe, - .remove = __exit_p(dw_mci_pltfm_remove), + .remove = dw_mci_exynos_remove, .driver = { .name = "dwmmc_exynos", .of_match_table = dw_mci_exynos_match,
There are boards (like Hardkernel's Odroid boards) on which eMMC card's reset line is connected to SoC GPIO line instead of the hardware reset logic. In case of such boards, before performing system reboot, additional reset of eMMC card is required to boot again properly. This patch adds code for handling such cases. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- .../devicetree/bindings/mmc/exynos-dw-mshc.txt | 6 +++ drivers/mmc/host/dw_mmc-exynos.c | 43 +++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-)