| Message ID | 20190405083458.16871-1-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: core: make pwrseq_emmc (partially) support sleepy GPIO controllers | expand |
On Fri, 5 Apr 2019 at 10:35, Andrea Merello <andrea.merello@gmail.com> wrote: > > pwrseq_emmc.c implements a HW reset procedure for eMMC chip by driving a > GPIO line. > > It registers the .reset() cb on mmc_pwrseq_ops and it registers a system > restart notification handler; both of them perform reset by unconditionally > calling gpiod_set_value(). > > If the eMMC reset line is tied to a GPIO controller whose driver can sleep > (i.e. I2C GPIO controller), then the kernel would spit warnings when trying > to reset the eMMC chip by means of .reset() mmc_pwrseq_ops cb (that is > exactly what I'm seeing during boot). > > Furthermore, on system reset we would gets to the system restart > notification handler with disabled interrupts - local_irq_disable() is > called in machine_restart() at least on ARM/ARM64 - and we would be in > trouble when the GPIO driver tries to sleep (which indeed doesn't happen > here, likely because in my case the machine specific code doesn't call > do_kernel_restart(), I guess..). > > This patch fixes the .reset() cb to make use of gpiod_set_value_cansleep(), > so that the eMMC gets reset on boot without complaints, while, since there > isn't that much we can do, we avoid register the restart handler if the > GPIO controller has a sleepy driver (and we spit a dev_notice() message to > let people know).. > > This had been tested on a downstream 4.9 kernel with backported > commit 83f37ee7ba33 ("mmc: pwrseq: Add reset callback to the struct > mmc_pwrseq_ops") and commit ae60fb031cf2 ("mmc: core: Don't do eMMC HW > reset when resuming the eMMC card"), because I couldn't boot my board > otherwise. Maybe worth to RFT. > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> Applied for next, thanks! Perhaps we want this to go stable as well? Kind regards Uffe > > diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c > index efb8a7965dd4..154f4204d58c 100644 > --- a/drivers/mmc/core/pwrseq_emmc.c > +++ b/drivers/mmc/core/pwrseq_emmc.c > @@ -30,19 +30,14 @@ struct mmc_pwrseq_emmc { > > #define to_pwrseq_emmc(p) container_of(p, struct mmc_pwrseq_emmc, pwrseq) > > -static void __mmc_pwrseq_emmc_reset(struct mmc_pwrseq_emmc *pwrseq) > -{ > - gpiod_set_value(pwrseq->reset_gpio, 1); > - udelay(1); > - gpiod_set_value(pwrseq->reset_gpio, 0); > - udelay(200); > -} > - > static void mmc_pwrseq_emmc_reset(struct mmc_host *host) > { > struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq); > > - __mmc_pwrseq_emmc_reset(pwrseq); > + gpiod_set_value_cansleep(pwrseq->reset_gpio, 1); > + udelay(1); > + gpiod_set_value_cansleep(pwrseq->reset_gpio, 0); > + udelay(200); > } > > static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this, > @@ -50,8 +45,11 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this, > { > struct mmc_pwrseq_emmc *pwrseq = container_of(this, > struct mmc_pwrseq_emmc, reset_nb); > + gpiod_set_value(pwrseq->reset_gpio, 1); > + udelay(1); > + gpiod_set_value(pwrseq->reset_gpio, 0); > + udelay(200); > > - __mmc_pwrseq_emmc_reset(pwrseq); > return NOTIFY_DONE; > } > > @@ -72,14 +70,18 @@ static int mmc_pwrseq_emmc_probe(struct platform_device *pdev) > if (IS_ERR(pwrseq->reset_gpio)) > return PTR_ERR(pwrseq->reset_gpio); > > - /* > - * register reset handler to ensure emmc reset also from > - * emergency_reboot(), priority 255 is the highest priority > - * so it will be executed before any system reboot handler. > - */ > - pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb; > - pwrseq->reset_nb.priority = 255; > - register_restart_handler(&pwrseq->reset_nb); > + if (!gpiod_cansleep(pwrseq->reset_gpio)) { > + /* > + * register reset handler to ensure emmc reset also from > + * emergency_reboot(), priority 255 is the highest priority > + * so it will be executed before any system reboot handler. > + */ > + pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb; > + pwrseq->reset_nb.priority = 255; > + register_restart_handler(&pwrseq->reset_nb); > + } else { > + dev_notice(dev, "EMMC reset pin tied to a sleepy GPIO driver; reset on emergency-reboot disabled\n"); > + } > > pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops; > pwrseq->pwrseq.dev = dev; > -- > 2.17.1 >
diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c index efb8a7965dd4..154f4204d58c 100644 --- a/drivers/mmc/core/pwrseq_emmc.c +++ b/drivers/mmc/core/pwrseq_emmc.c @@ -30,19 +30,14 @@ struct mmc_pwrseq_emmc { #define to_pwrseq_emmc(p) container_of(p, struct mmc_pwrseq_emmc, pwrseq) -static void __mmc_pwrseq_emmc_reset(struct mmc_pwrseq_emmc *pwrseq) -{ - gpiod_set_value(pwrseq->reset_gpio, 1); - udelay(1); - gpiod_set_value(pwrseq->reset_gpio, 0); - udelay(200); -} - static void mmc_pwrseq_emmc_reset(struct mmc_host *host) { struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq); - __mmc_pwrseq_emmc_reset(pwrseq); + gpiod_set_value_cansleep(pwrseq->reset_gpio, 1); + udelay(1); + gpiod_set_value_cansleep(pwrseq->reset_gpio, 0); + udelay(200); } static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this, @@ -50,8 +45,11 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this, { struct mmc_pwrseq_emmc *pwrseq = container_of(this, struct mmc_pwrseq_emmc, reset_nb); + gpiod_set_value(pwrseq->reset_gpio, 1); + udelay(1); + gpiod_set_value(pwrseq->reset_gpio, 0); + udelay(200); - __mmc_pwrseq_emmc_reset(pwrseq); return NOTIFY_DONE; } @@ -72,14 +70,18 @@ static int mmc_pwrseq_emmc_probe(struct platform_device *pdev) if (IS_ERR(pwrseq->reset_gpio)) return PTR_ERR(pwrseq->reset_gpio); - /* - * register reset handler to ensure emmc reset also from - * emergency_reboot(), priority 255 is the highest priority - * so it will be executed before any system reboot handler. - */ - pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb; - pwrseq->reset_nb.priority = 255; - register_restart_handler(&pwrseq->reset_nb); + if (!gpiod_cansleep(pwrseq->reset_gpio)) { + /* + * register reset handler to ensure emmc reset also from + * emergency_reboot(), priority 255 is the highest priority + * so it will be executed before any system reboot handler. + */ + pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb; + pwrseq->reset_nb.priority = 255; + register_restart_handler(&pwrseq->reset_nb); + } else { + dev_notice(dev, "EMMC reset pin tied to a sleepy GPIO driver; reset on emergency-reboot disabled\n"); + } pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops; pwrseq->pwrseq.dev = dev;
pwrseq_emmc.c implements a HW reset procedure for eMMC chip by driving a GPIO line. It registers the .reset() cb on mmc_pwrseq_ops and it registers a system restart notification handler; both of them perform reset by unconditionally calling gpiod_set_value(). If the eMMC reset line is tied to a GPIO controller whose driver can sleep (i.e. I2C GPIO controller), then the kernel would spit warnings when trying to reset the eMMC chip by means of .reset() mmc_pwrseq_ops cb (that is exactly what I'm seeing during boot). Furthermore, on system reset we would gets to the system restart notification handler with disabled interrupts - local_irq_disable() is called in machine_restart() at least on ARM/ARM64 - and we would be in trouble when the GPIO driver tries to sleep (which indeed doesn't happen here, likely because in my case the machine specific code doesn't call do_kernel_restart(), I guess..). This patch fixes the .reset() cb to make use of gpiod_set_value_cansleep(), so that the eMMC gets reset on boot without complaints, while, since there isn't that much we can do, we avoid register the restart handler if the GPIO controller has a sleepy driver (and we spit a dev_notice() message to let people know).. This had been tested on a downstream 4.9 kernel with backported commit 83f37ee7ba33 ("mmc: pwrseq: Add reset callback to the struct mmc_pwrseq_ops") and commit ae60fb031cf2 ("mmc: core: Don't do eMMC HW reset when resuming the eMMC card"), because I couldn't boot my board otherwise. Maybe worth to RFT. Signed-off-by: Andrea Merello <andrea.merello@gmail.com>