| Message ID | 1385408393-19707-2-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote: > It appears not all platforms featuring a musb core need to save the musb > core registers at suspend time and restore them on resume. > > The dsps platform does, however. So add a bit in struct > musb_hdrc_platform_data to let platforms specify their need of such > action being taken. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/usb/musb/musb_core.c | 17 ++++++++++++++++- > include/linux/usb/musb.h | 3 +++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > index 0a43329..a8ded57 100644 > --- a/drivers/usb/musb/musb_core.c > +++ b/drivers/usb/musb/musb_core.c > @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev) > { > struct musb *musb = dev_to_musb(dev); > unsigned long flags; > + struct musb_hdrc_platform_data *plat = dev_get_platdata(dev); we don't want to have platform_data on DT-based boot. It's best to just save those registers unconditionally as it doesn't hurt.
On 11/25/2013 08:44 PM, Felipe Balbi wrote: > Hi, > > On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote: >> It appears not all platforms featuring a musb core need to save the musb >> core registers at suspend time and restore them on resume. >> >> The dsps platform does, however. So add a bit in struct >> musb_hdrc_platform_data to let platforms specify their need of such >> action being taken. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> >> --- >> drivers/usb/musb/musb_core.c | 17 ++++++++++++++++- >> include/linux/usb/musb.h | 3 +++ >> 2 files changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c >> index 0a43329..a8ded57 100644 >> --- a/drivers/usb/musb/musb_core.c >> +++ b/drivers/usb/musb/musb_core.c >> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev) >> { >> struct musb *musb = dev_to_musb(dev); >> unsigned long flags; >> + struct musb_hdrc_platform_data *plat = dev_get_platdata(dev); > > we don't want to have platform_data on DT-based boot. It's best to just > save those registers unconditionally as it doesn't hurt. > My concern about doing it unconditionally from the core is simply that I fear regressions for other platforms. I can of course drop it if you're certain that that's correct. I can only test this on a dsps glue layer, and I have no documentation for the musb core. All I'm left with here is fishing in muddy waters :/ Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Nov 25, 2013 at 09:08:51PM +0100, Daniel Mack wrote: > On 11/25/2013 08:44 PM, Felipe Balbi wrote: > > Hi, > > > > On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote: > >> It appears not all platforms featuring a musb core need to save the musb > >> core registers at suspend time and restore them on resume. > >> > >> The dsps platform does, however. So add a bit in struct > >> musb_hdrc_platform_data to let platforms specify their need of such > >> action being taken. > >> > >> Signed-off-by: Daniel Mack <zonque@gmail.com> > >> --- > >> drivers/usb/musb/musb_core.c | 17 ++++++++++++++++- > >> include/linux/usb/musb.h | 3 +++ > >> 2 files changed, 19 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > >> index 0a43329..a8ded57 100644 > >> --- a/drivers/usb/musb/musb_core.c > >> +++ b/drivers/usb/musb/musb_core.c > >> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev) > >> { > >> struct musb *musb = dev_to_musb(dev); > >> unsigned long flags; > >> + struct musb_hdrc_platform_data *plat = dev_get_platdata(dev); > > > > we don't want to have platform_data on DT-based boot. It's best to just > > save those registers unconditionally as it doesn't hurt. > > > > My concern about doing it unconditionally from the core is simply that I > fear regressions for other platforms. I can of course drop it if you're > certain that that's correct. > > I can only test this on a dsps glue layer, and I have no documentation > for the musb core. All I'm left with here is fishing in muddy waters :/ I really don't think it should cause any issues with any other platform. We can make to leave this soaking in linux-next for a long time and hope people test it.
On 11/25/2013 09:13 PM, Felipe Balbi wrote: > Hi, > > On Mon, Nov 25, 2013 at 09:08:51PM +0100, Daniel Mack wrote: >> On 11/25/2013 08:44 PM, Felipe Balbi wrote: >>> Hi, >>> >>> On Mon, Nov 25, 2013 at 08:39:49PM +0100, Daniel Mack wrote: >>>> It appears not all platforms featuring a musb core need to save the musb >>>> core registers at suspend time and restore them on resume. >>>> >>>> The dsps platform does, however. So add a bit in struct >>>> musb_hdrc_platform_data to let platforms specify their need of such >>>> action being taken. >>>> >>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>>> --- >>>> drivers/usb/musb/musb_core.c | 17 ++++++++++++++++- >>>> include/linux/usb/musb.h | 3 +++ >>>> 2 files changed, 19 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c >>>> index 0a43329..a8ded57 100644 >>>> --- a/drivers/usb/musb/musb_core.c >>>> +++ b/drivers/usb/musb/musb_core.c >>>> @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev) >>>> { >>>> struct musb *musb = dev_to_musb(dev); >>>> unsigned long flags; >>>> + struct musb_hdrc_platform_data *plat = dev_get_platdata(dev); >>> >>> we don't want to have platform_data on DT-based boot. It's best to just >>> save those registers unconditionally as it doesn't hurt. >>> >> >> My concern about doing it unconditionally from the core is simply that I >> fear regressions for other platforms. I can of course drop it if you're >> certain that that's correct. >> >> I can only test this on a dsps glue layer, and I have no documentation >> for the musb core. All I'm left with here is fishing in muddy waters :/ > > I really don't think it should cause any issues with any other platform. > > We can make to leave this soaking in linux-next for a long time and hope > people test it. > Ok, sounds good. I'll do it and repost, along with the other changes. Many thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 25-11-2013 23:39, Daniel Mack wrote: > It appears not all platforms featuring a musb core need to save the musb > core registers at suspend time and restore them on resume. > The dsps platform does, however. So add a bit in struct > musb_hdrc_platform_data to let platforms specify their need of such > action being taken. > Signed-off-by: Daniel Mack <zonque@gmail.com> [...] > diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h > index eb50525..e5a581c 100644 > --- a/include/linux/usb/musb.h > +++ b/include/linux/usb/musb.h > @@ -99,6 +99,9 @@ struct musb_hdrc_platform_data { > /* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */ > u8 mode; > > + /* should the musb core restore registers after suspend? */ > + u8 restore_after_suspend:1; > + Better placement seems to be with 'extvbus' field which is also 1-bit -- that would save space in the structure. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/26/2013 11:50 AM, Sergei Shtylyov wrote: > On 25-11-2013 23:39, Daniel Mack wrote: > >> It appears not all platforms featuring a musb core need to save the musb >> core registers at suspend time and restore them on resume. > >> The dsps platform does, however. So add a bit in struct >> musb_hdrc_platform_data to let platforms specify their need of such >> action being taken. > >> Signed-off-by: Daniel Mack <zonque@gmail.com> > [...] > >> diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h >> index eb50525..e5a581c 100644 >> --- a/include/linux/usb/musb.h >> +++ b/include/linux/usb/musb.h >> @@ -99,6 +99,9 @@ struct musb_hdrc_platform_data { >> /* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */ >> u8 mode; >> >> + /* should the musb core restore registers after suspend? */ >> + u8 restore_after_suspend:1; >> + > > Better placement seems to be with 'extvbus' field which is also 1-bit -- > that would save space in the structure. That patch is deprecated, as things are now done unconditionally, without consulting this flag. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c index 0a43329..a8ded57 100644 --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -2202,6 +2202,7 @@ static int musb_suspend(struct device *dev) { struct musb *musb = dev_to_musb(dev); unsigned long flags; + struct musb_hdrc_platform_data *plat = dev_get_platdata(dev); spin_lock_irqsave(&musb->lock, flags); @@ -2215,16 +2216,30 @@ static int musb_suspend(struct device *dev) */ } + if (plat->restore_after_suspend) + musb_save_context(musb); + spin_unlock_irqrestore(&musb->lock, flags); return 0; } static int musb_resume_noirq(struct device *dev) { - /* for static cmos like DaVinci, register values were preserved + struct musb *musb = dev_to_musb(dev); + struct musb_hdrc_platform_data *plat = dev_get_platdata(dev); + + /* + * For static cmos like DaVinci, register values were preserved * unless for some reason the whole soc powered down or the USB * module got reset through the PSC (vs just being disabled). + * + * The plaform data tells us about the necessity of saving and + * restoring the context across a suspend cycle. */ + + if (plat->restore_after_suspend) + musb_restore_context(musb); + return 0; } diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h index eb50525..e5a581c 100644 --- a/include/linux/usb/musb.h +++ b/include/linux/usb/musb.h @@ -99,6 +99,9 @@ struct musb_hdrc_platform_data { /* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */ u8 mode; + /* should the musb core restore registers after suspend? */ + u8 restore_after_suspend:1; + /* for clk_get() */ const char *clock;
It appears not all platforms featuring a musb core need to save the musb core registers at suspend time and restore them on resume. The dsps platform does, however. So add a bit in struct musb_hdrc_platform_data to let platforms specify their need of such action being taken. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/usb/musb/musb_core.c | 17 ++++++++++++++++- include/linux/usb/musb.h | 3 +++ 2 files changed, 19 insertions(+), 1 deletion(-)