| Message ID | 1373411961-23812-4-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, July 10, 2013, Doug Anderson wrote: > If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up > looping around forever. This has been seen to happen on exynos5420 > silicon despite the fact that we haven't enabled any wakeup events. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Changes in v2: > - Use suspend_noirq as per James Hogan. > > drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c > index f013e7e..36b9620 100644 > --- a/drivers/mmc/host/dw_mmc-exynos.c > +++ b/drivers/mmc/host/dw_mmc-exynos.c > @@ -30,6 +30,7 @@ > #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ > SDMMC_CLKSEL_CCLK_DRIVE(y) | \ > SDMMC_CLKSEL_CCLK_DIVIDER(z)) > +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) > > #define SDMMC_CMD_USE_HOLD_REG BIT(29) > > @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) > return 0; > } > > +/** > + * dw_mci_exynos_resume_noirq - Exynos-specific resume code > + * > + * We have seen cases (at least on the exynos5420) where turning off the INT > + * power rail during suspend will leave the WAKEUP_INT bit in the CLKSEL > + * register asserted. This bit is 1 to indicate that it fired and we can > + * clear it by writing a 1 back. Clear it to prevent interrupts from going off > + * constantly. > + */ As I know this bit is auto-cleared. Did you find the cause of this problem? How about your GPIO setting in sleep? Currently, we don't know why the problem is happened. At least, we should make it clear. Thanks, Seungwon Jeon > + > +static int dw_mci_exynos_resume_noirq(struct dw_mci *host) > +{ > + u32 clksel; > + > + clksel = mci_readl(host, CLKSEL); > + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) > + mci_writel(host, CLKSEL, clksel); > + > + return 0; > +} > + > static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) > { > /* > @@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = { > .caps = exynos_dwmmc_caps, > .init = dw_mci_exynos_priv_init, > .setup_clock = dw_mci_exynos_setup_clock, > + .resume_noirq = dw_mci_exynos_resume_noirq, > .prepare_command = dw_mci_exynos_prepare_command, > .set_ios = dw_mci_exynos_set_ios, > .parse_dt = dw_mci_exynos_parse_dt, > -- > 1.8.3 > > -- > 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
Seungwon, On Wed, Jul 10, 2013 at 7:54 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > On Wed, July 10, 2013, Doug Anderson wrote: >> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up >> looping around forever. This has been seen to happen on exynos5420 >> silicon despite the fact that we haven't enabled any wakeup events. >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> --- >> Changes in v2: >> - Use suspend_noirq as per James Hogan. >> >> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c >> index f013e7e..36b9620 100644 >> --- a/drivers/mmc/host/dw_mmc-exynos.c >> +++ b/drivers/mmc/host/dw_mmc-exynos.c >> @@ -30,6 +30,7 @@ >> #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ >> SDMMC_CLKSEL_CCLK_DRIVE(y) | \ >> SDMMC_CLKSEL_CCLK_DIVIDER(z)) >> +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) >> >> #define SDMMC_CMD_USE_HOLD_REG BIT(29) >> >> @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) >> return 0; >> } >> >> +/** >> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code >> + * >> + * We have seen cases (at least on the exynos5420) where turning off the INT >> + * power rail during suspend will leave the WAKEUP_INT bit in the CLKSEL >> + * register asserted. This bit is 1 to indicate that it fired and we can >> + * clear it by writing a 1 back. Clear it to prevent interrupts from going off >> + * constantly. >> + */ > As I know this bit is auto-cleared. > Did you find the cause of this problem? > How about your GPIO setting in sleep? > Currently, we don't know why the problem is happened. > At least, we should make it clear. Yes, the documentation that I have says that this bit is "auto cleared" as well but doesn't indicate under what conditions it is auto cleared. From testing how this bit reacts I have found that writing a 1 to it clears the bit--in other words it behaves like bits in RINTSTS. That's a terrible design for a bit in a register with shared config but appears to be how it works. Note: in a sense it will be "auto cleared" because doing a read-modify-write of any other bits in this register will clear the interrupt. I have asked for official confirmation. We have found that on exynos5420 bits 8-10 of CLKSEL are marked as RESERVED. Those bits are documented to enable the WAKEUP_INT on exynos5250. My best guess is that these bits are not used on exynos5420 and the WAKEUP_INT line is left floating, which means it can fire randomly. I have also asked for official confirmation about this. I will likely merge this change locally in our kernel tree while waiting for a response. If you would like to wait before Acking that's very reasonable. Do you have any other problems with this change assuming my understanding above is correct? -Doug
On Thu, July 11, 2013, Doug Anderson wrote: > Seungwon, > > On Wed, Jul 10, 2013 at 7:54 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > > On Wed, July 10, 2013, Doug Anderson wrote: > >> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up > >> looping around forever. This has been seen to happen on exynos5420 > >> silicon despite the fact that we haven't enabled any wakeup events. > >> > >> Signed-off-by: Doug Anderson <dianders@chromium.org> > >> --- > >> Changes in v2: > >> - Use suspend_noirq as per James Hogan. > >> > >> drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++ > >> 1 file changed, 23 insertions(+) > >> > >> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c > >> index f013e7e..36b9620 100644 > >> --- a/drivers/mmc/host/dw_mmc-exynos.c > >> +++ b/drivers/mmc/host/dw_mmc-exynos.c > >> @@ -30,6 +30,7 @@ > >> #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ > >> SDMMC_CLKSEL_CCLK_DRIVE(y) | \ > >> SDMMC_CLKSEL_CCLK_DIVIDER(z)) > >> +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) > >> > >> #define SDMMC_CMD_USE_HOLD_REG BIT(29) > >> > >> @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) > >> return 0; > >> } > >> > >> +/** > >> + * dw_mci_exynos_resume_noirq - Exynos-specific resume code > >> + * > >> + * We have seen cases (at least on the exynos5420) where turning off the INT > >> + * power rail during suspend will leave the WAKEUP_INT bit in the CLKSEL > >> + * register asserted. This bit is 1 to indicate that it fired and we can > >> + * clear it by writing a 1 back. Clear it to prevent interrupts from going off > >> + * constantly. > >> + */ > > As I know this bit is auto-cleared. > > Did you find the cause of this problem? > > How about your GPIO setting in sleep? > > Currently, we don't know why the problem is happened. > > At least, we should make it clear. > > Yes, the documentation that I have says that this bit is "auto > cleared" as well but doesn't indicate under what conditions it is auto > cleared. From testing how this bit reacts I have found that writing a > 1 to it clears the bit--in other words it behaves like bits in > RINTSTS. That's a terrible design for a bit in a register with shared > config but appears to be how it works. > > Note: in a sense it will be "auto cleared" because doing a > read-modify-write of any other bits in this register will clear the > interrupt. > > I have asked for official confirmation. > > We have found that on exynos5420 bits 8-10 of CLKSEL are marked as > RESERVED. Those bits are documented to enable the WAKEUP_INT on > exynos5250. My best guess is that these bits are not used on > exynos5420 and the WAKEUP_INT line is left floating, which means it > can fire randomly. I have also asked for official confirmation about > this. Sorry for late response. Yes, it's not clear. If you get the confirmation, could you share this problem? Possibly, auto-clear may not be implemented. Then, manual should be correct. Thanks, Seungwon Jeon > > > I will likely merge this change locally in our kernel tree while > waiting for a response. If you would like to wait before Acking > that's very reasonable. Do you have any other problems with this > change assuming my understanding above is correct? > > -Doug > -- > 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
Seungwon, On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: > Sorry for late response. > Yes, it's not clear. > If you get the confirmation, could you share this problem? > Possibly, auto-clear may not be implemented. > Then, manual should be correct. I just got an update from my contacts. They confirm that bit 11 is not automatically cleared and that writing to it will clear it. Hopefully this information will make it into the next version of any documentation that you receive as well. It is still unclear exactly why the WAKEUP_INT was being asserted on our board despite the fact that all of the wakeup control signals (bits 10:8) were 0. That is still being investigated. -Doug
Seungwon, On Wed, Jul 31, 2013 at 9:18 AM, Doug Anderson <dianders@chromium.org> wrote: > Seungwon, > > On Mon, Jul 15, 2013 at 5:09 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote: >> Sorry for late response. >> Yes, it's not clear. >> If you get the confirmation, could you share this problem? >> Possibly, auto-clear may not be implemented. >> Then, manual should be correct. > > I just got an update from my contacts. They confirm that bit 11 is > not automatically cleared and that writing to it will clear it. > Hopefully this information will make it into the next version of any > documentation that you receive as well. > > It is still unclear exactly why the WAKEUP_INT was being asserted on > our board despite the fact that all of the wakeup control signals > (bits 10:8) were 0. That is still being investigated. I have further confirmed from my contacts at Samsung that this is a real silicon errata and that clearing the WAKEUP_INT as I am doing in this series is the right workaround. New patch coming shortly based against ToT linux (v3.11-rc4-20-g0fff106). I have confirmed that it applies cleanly to mmc-next, though I haven't tried booting with that tree.
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index f013e7e..36b9620 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -30,6 +30,7 @@ #define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \ SDMMC_CLKSEL_CCLK_DRIVE(y) | \ SDMMC_CLKSEL_CCLK_DIVIDER(z)) +#define SDMMC_CLKSEL_WAKEUP_INT BIT(11) #define SDMMC_CMD_USE_HOLD_REG BIT(29) @@ -102,6 +103,27 @@ static int dw_mci_exynos_setup_clock(struct dw_mci *host) return 0; } +/** + * dw_mci_exynos_resume_noirq - Exynos-specific resume code + * + * We have seen cases (at least on the exynos5420) where turning off the INT + * power rail during suspend will leave the WAKEUP_INT bit in the CLKSEL + * register asserted. This bit is 1 to indicate that it fired and we can + * clear it by writing a 1 back. Clear it to prevent interrupts from going off + * constantly. + */ + +static int dw_mci_exynos_resume_noirq(struct dw_mci *host) +{ + u32 clksel; + + clksel = mci_readl(host, CLKSEL); + if (clksel & SDMMC_CLKSEL_WAKEUP_INT) + mci_writel(host, CLKSEL, clksel); + + return 0; +} + static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr) { /* @@ -165,6 +187,7 @@ static const struct dw_mci_drv_data exynos_drv_data = { .caps = exynos_dwmmc_caps, .init = dw_mci_exynos_priv_init, .setup_clock = dw_mci_exynos_setup_clock, + .resume_noirq = dw_mci_exynos_resume_noirq, .prepare_command = dw_mci_exynos_prepare_command, .set_ios = dw_mci_exynos_set_ios, .parse_dt = dw_mci_exynos_parse_dt,
If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up looping around forever. This has been seen to happen on exynos5420 silicon despite the fact that we haven't enabled any wakeup events. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Changes in v2: - Use suspend_noirq as per James Hogan. drivers/mmc/host/dw_mmc-exynos.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)