diff mbox

[4/5] usb: musb: dsps: add support for suspend and resume

Message ID 1385408393-19707-5-git-send-email-zonque@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Mack Nov. 25, 2013, 7:39 p.m. UTC
The dsps platform needs to save save some registers at suspend time and
restore them after resume. This patch adds a struct for these registers,
and also lets the musb core know that the core registers need to be
saved as well.

We also have to call musb_port_reset() for this platform upon resume, so
this function has to be made non-static.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/usb/musb/musb_core.h    |  1 +
 drivers/usb/musb/musb_dsps.c    | 59 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_host.h    |  2 ++
 drivers/usb/musb/musb_virthub.c |  2 +-
 4 files changed, 63 insertions(+), 1 deletion(-)

Comments

Felipe Balbi Nov. 25, 2013, 7:48 p.m. UTC | #1
On Mon, Nov 25, 2013 at 08:39:52PM +0100, Daniel Mack wrote:
> The dsps platform needs to save save some registers at suspend time and
> restore them after resume. This patch adds a struct for these registers,
> and also lets the musb core know that the core registers need to be
> saved as well.
> 
> We also have to call musb_port_reset() for this platform upon resume, so
> this function has to be made non-static.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/usb/musb/musb_core.h    |  1 +
>  drivers/usb/musb/musb_dsps.c    | 59 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/musb/musb_host.h    |  2 ++
>  drivers/usb/musb/musb_virthub.c |  2 +-
>  4 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index 29f7cd7..a423037 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -295,6 +295,7 @@ struct musb {
>  
>  	irqreturn_t		(*isr)(int, void *);
>  	struct work_struct	irq_work;
> +
>  	u16			hwvers;
>  
>  	u16			intrrxe;
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index e57d712..361ddf8 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -112,6 +112,19 @@ struct dsps_musb_wrapper {
>  	u8		poll_seconds;
>  };
>  
> +/*
> + * register shadow for suspend
> + */
> +struct dsps_context {
> +	u32 control;
> +	u32 epintr;
> +	u32 coreintr;
> +	u32 phy_utmi;
> +	u32 mode;
> +	u32 tx_mode;
> +	u32 rx_mode;
> +};
> +
>  /**
>   * DSPS glue structure.
>   */
> @@ -121,6 +134,8 @@ struct dsps_glue {
>  	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
>  	struct timer_list timer;	/* otg_workaround timer */
>  	unsigned long last_timer;    /* last timer data for each instance */
> +
> +	struct dsps_context context;
>  };
>  
>  static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
> @@ -506,6 +521,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
>  	}
>  	pdata.config = config;
>  	pdata.platform_ops = &dsps_ops;
> +	pdata.restore_after_suspend = 1;
>  
>  	config->num_eps = get_int_prop(dn, "mentor,num-eps");
>  	config->ram_bits = get_int_prop(dn, "mentor,ram-bits");
> @@ -632,11 +648,54 @@ static const struct of_device_id musb_dsps_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
>  
> +#ifdef CONFIG_PM
> +static int dsps_suspend(struct device *dev)
> +{
> +	struct dsps_glue *glue = dev_get_drvdata(dev);
> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
> +	struct musb *musb = platform_get_drvdata(glue->musb);
> +	void __iomem *mbase = musb->ctrl_base;
> +
> +	glue->context.control = dsps_readl(mbase, wrp->control);
> +	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
> +	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
> +	glue->context.phy_utmi = dsps_readl(mbase, wrp->phy_utmi);
> +	glue->context.mode = dsps_readl(mbase, wrp->mode);
> +	glue->context.tx_mode = dsps_readl(mbase, wrp->tx_mode);
> +	glue->context.rx_mode = dsps_readl(mbase, wrp->rx_mode);
> +
> +	return 0;
> +}
> +
> +static int dsps_resume(struct device *dev)
> +{
> +	struct dsps_glue *glue = dev_get_drvdata(dev);
> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
> +	struct musb *musb = platform_get_drvdata(glue->musb);
> +	void __iomem *mbase = musb->ctrl_base;
> +
> +	dsps_writel(mbase, wrp->control, glue->context.control);
> +	dsps_writel(mbase, wrp->epintr_set, glue->context.epintr);
> +	dsps_writel(mbase, wrp->coreintr_set, glue->context.coreintr);
> +	dsps_writel(mbase, wrp->phy_utmi, glue->context.phy_utmi);
> +	dsps_writel(mbase, wrp->mode, glue->context.mode);
> +	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
> +	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
> +
> +	musb_port_reset(musb, false);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
> +
>  static struct platform_driver dsps_usbss_driver = {
>  	.probe		= dsps_probe,
>  	.remove         = dsps_remove,
>  	.driver         = {
>  		.name   = "musb-dsps",
> +		.pm	= &dsps_pm_ops,
>  		.of_match_table	= musb_dsps_of_match,
>  	},
>  };
> diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
> index e660af9..7436c24 100644
> --- a/drivers/usb/musb/musb_host.h
> +++ b/drivers/usb/musb/musb_host.h
> @@ -93,6 +93,7 @@ extern void musb_root_disconnect(struct musb *musb);
>  extern void musb_host_resume_root_hub(struct musb *musb);
>  extern void musb_host_poke_root_hub(struct musb *musb);
>  extern void musb_port_suspend(struct musb *musb, bool do_suspend);
> +extern void musb_port_reset(struct musb *musb, bool do_reset);
>  #else
>  static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
>  {
> @@ -123,6 +124,7 @@ static inline void musb_host_resume_root_hub(struct musb *musb)	{}
>  static inline void musb_host_poll_rh_status(struct musb *musb)	{}
>  static inline void musb_host_poke_root_hub(struct musb *musb)	{}
>  static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
> +static inline void musb_port_reset(struct musb *musb)		{}
>  #endif
>  
>  struct usb_hcd;
> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> index e977441..24e46c0 100644
> --- a/drivers/usb/musb/musb_virthub.c
> +++ b/drivers/usb/musb/musb_virthub.c
> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>  	}
>  }
>  
> -static void musb_port_reset(struct musb *musb, bool do_reset)
> +void musb_port_reset(struct musb *musb, bool do_reset)

NAK, this should not be called from the glue layers at all. If anything
call from musb_core's resume callback. That will only be called after
parent's resume has been called anyway.
Daniel Mack Nov. 25, 2013, 8:04 p.m. UTC | #2
On 11/25/2013 08:48 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 08:39:52PM +0100, Daniel Mack wrote:
>> The dsps platform needs to save save some registers at suspend time and
>> restore them after resume. This patch adds a struct for these registers,
>> and also lets the musb core know that the core registers need to be
>> saved as well.
>>
>> We also have to call musb_port_reset() for this platform upon resume, so
>> this function has to be made non-static.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  drivers/usb/musb/musb_core.h    |  1 +
>>  drivers/usb/musb/musb_dsps.c    | 59 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/musb/musb_host.h    |  2 ++
>>  drivers/usb/musb/musb_virthub.c |  2 +-
>>  4 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
>> index 29f7cd7..a423037 100644
>> --- a/drivers/usb/musb/musb_core.h
>> +++ b/drivers/usb/musb/musb_core.h
>> @@ -295,6 +295,7 @@ struct musb {
>>  
>>  	irqreturn_t		(*isr)(int, void *);
>>  	struct work_struct	irq_work;
>> +
>>  	u16			hwvers;
>>  
>>  	u16			intrrxe;
>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
>> index e57d712..361ddf8 100644
>> --- a/drivers/usb/musb/musb_dsps.c
>> +++ b/drivers/usb/musb/musb_dsps.c
>> @@ -112,6 +112,19 @@ struct dsps_musb_wrapper {
>>  	u8		poll_seconds;
>>  };
>>  
>> +/*
>> + * register shadow for suspend
>> + */
>> +struct dsps_context {
>> +	u32 control;
>> +	u32 epintr;
>> +	u32 coreintr;
>> +	u32 phy_utmi;
>> +	u32 mode;
>> +	u32 tx_mode;
>> +	u32 rx_mode;
>> +};
>> +
>>  /**
>>   * DSPS glue structure.
>>   */
>> @@ -121,6 +134,8 @@ struct dsps_glue {
>>  	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
>>  	struct timer_list timer;	/* otg_workaround timer */
>>  	unsigned long last_timer;    /* last timer data for each instance */
>> +
>> +	struct dsps_context context;
>>  };
>>  
>>  static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
>> @@ -506,6 +521,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
>>  	}
>>  	pdata.config = config;
>>  	pdata.platform_ops = &dsps_ops;
>> +	pdata.restore_after_suspend = 1;
>>  
>>  	config->num_eps = get_int_prop(dn, "mentor,num-eps");
>>  	config->ram_bits = get_int_prop(dn, "mentor,ram-bits");
>> @@ -632,11 +648,54 @@ static const struct of_device_id musb_dsps_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
>>  
>> +#ifdef CONFIG_PM
>> +static int dsps_suspend(struct device *dev)
>> +{
>> +	struct dsps_glue *glue = dev_get_drvdata(dev);
>> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
>> +	struct musb *musb = platform_get_drvdata(glue->musb);
>> +	void __iomem *mbase = musb->ctrl_base;
>> +
>> +	glue->context.control = dsps_readl(mbase, wrp->control);
>> +	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
>> +	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
>> +	glue->context.phy_utmi = dsps_readl(mbase, wrp->phy_utmi);
>> +	glue->context.mode = dsps_readl(mbase, wrp->mode);
>> +	glue->context.tx_mode = dsps_readl(mbase, wrp->tx_mode);
>> +	glue->context.rx_mode = dsps_readl(mbase, wrp->rx_mode);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dsps_resume(struct device *dev)
>> +{
>> +	struct dsps_glue *glue = dev_get_drvdata(dev);
>> +	const struct dsps_musb_wrapper *wrp = glue->wrp;
>> +	struct musb *musb = platform_get_drvdata(glue->musb);
>> +	void __iomem *mbase = musb->ctrl_base;
>> +
>> +	dsps_writel(mbase, wrp->control, glue->context.control);
>> +	dsps_writel(mbase, wrp->epintr_set, glue->context.epintr);
>> +	dsps_writel(mbase, wrp->coreintr_set, glue->context.coreintr);
>> +	dsps_writel(mbase, wrp->phy_utmi, glue->context.phy_utmi);
>> +	dsps_writel(mbase, wrp->mode, glue->context.mode);
>> +	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
>> +	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
>> +
>> +	musb_port_reset(musb, false);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
>> +
>>  static struct platform_driver dsps_usbss_driver = {
>>  	.probe		= dsps_probe,
>>  	.remove         = dsps_remove,
>>  	.driver         = {
>>  		.name   = "musb-dsps",
>> +		.pm	= &dsps_pm_ops,
>>  		.of_match_table	= musb_dsps_of_match,
>>  	},
>>  };
>> diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
>> index e660af9..7436c24 100644
>> --- a/drivers/usb/musb/musb_host.h
>> +++ b/drivers/usb/musb/musb_host.h
>> @@ -93,6 +93,7 @@ extern void musb_root_disconnect(struct musb *musb);
>>  extern void musb_host_resume_root_hub(struct musb *musb);
>>  extern void musb_host_poke_root_hub(struct musb *musb);
>>  extern void musb_port_suspend(struct musb *musb, bool do_suspend);
>> +extern void musb_port_reset(struct musb *musb, bool do_reset);
>>  #else
>>  static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
>>  {
>> @@ -123,6 +124,7 @@ static inline void musb_host_resume_root_hub(struct musb *musb)	{}
>>  static inline void musb_host_poll_rh_status(struct musb *musb)	{}
>>  static inline void musb_host_poke_root_hub(struct musb *musb)	{}
>>  static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
>> +static inline void musb_port_reset(struct musb *musb)		{}
>>  #endif
>>  
>>  struct usb_hcd;
>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
>> index e977441..24e46c0 100644
>> --- a/drivers/usb/musb/musb_virthub.c
>> +++ b/drivers/usb/musb/musb_virthub.c
>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>>  	}
>>  }
>>  
>> -static void musb_port_reset(struct musb *musb, bool do_reset)
>> +void musb_port_reset(struct musb *musb, bool do_reset)
> 
> NAK, this should not be called from the glue layers at all. If anything
> call from musb_core's resume callback. That will only be called after
> parent's resume has been called anyway.

Given that this driver is successfully used with suspend/resume on other
platforms without that reset, but it's clearly needed for dsps, I'm
fairly sure this should be a glue-layer specific thing. Hence the change.

I think it'll break things on other platforms if we unconditionally
reset the port after resume, but I can't test it. Anyone?


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
Felipe Balbi Nov. 25, 2013, 8:08 p.m. UTC | #3
Hi,

On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
> >> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> >> index e977441..24e46c0 100644
> >> --- a/drivers/usb/musb/musb_virthub.c
> >> +++ b/drivers/usb/musb/musb_virthub.c
> >> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> >>  	}
> >>  }
> >>  
> >> -static void musb_port_reset(struct musb *musb, bool do_reset)
> >> +void musb_port_reset(struct musb *musb, bool do_reset)
> > 
> > NAK, this should not be called from the glue layers at all. If anything
> > call from musb_core's resume callback. That will only be called after
> > parent's resume has been called anyway.
> 
> Given that this driver is successfully used with suspend/resume on other
> platforms without that reset, but it's clearly needed for dsps, I'm
> fairly sure this should be a glue-layer specific thing. Hence the change.
> 
> I think it'll break things on other platforms if we unconditionally
> reset the port after resume, but I can't test it. Anyone?

your original commit log only says "we need to issue port reset" but it
never explains why. It could very well be you just found a device which
needs to be reset when coming out of suspend and it misses a quirk flag ?

In any case, those functions should never be called by the glue, if
reset needs to be called, it must be called by musb-hdrc.ko, if you need
a flag, pass a flag but I need a really good explanation in your commit
log of why that's necessary.
Daniel Mack Nov. 25, 2013, 8:26 p.m. UTC | #4
On 11/25/2013 09:08 PM, Felipe Balbi wrote:
> On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
>>>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
>>>> index e977441..24e46c0 100644
>>>> --- a/drivers/usb/musb/musb_virthub.c
>>>> +++ b/drivers/usb/musb/musb_virthub.c
>>>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>>>>  	}
>>>>  }
>>>>  
>>>> -static void musb_port_reset(struct musb *musb, bool do_reset)
>>>> +void musb_port_reset(struct musb *musb, bool do_reset)
>>>
>>> NAK, this should not be called from the glue layers at all. If anything
>>> call from musb_core's resume callback. That will only be called after
>>> parent's resume has been called anyway.
>>
>> Given that this driver is successfully used with suspend/resume on other
>> platforms without that reset, but it's clearly needed for dsps, I'm
>> fairly sure this should be a glue-layer specific thing. Hence the change.
>>
>> I think it'll break things on other platforms if we unconditionally
>> reset the port after resume, but I can't test it. Anyone?
> 
> your original commit log only says "we need to issue port reset" but it
> never explains why. It could very well be you just found a device which
> needs to be reset when coming out of suspend and it misses a quirk flag ?

I think I can really rule that out, as I tested with multiple USB sticks
and also tested the same sticks on other embedded platforms.

> In any case, those functions should never be called by the glue, if
> reset needs to be called, it must be called by musb-hdrc.ko, if you need
> a flag, pass a flag but I need a really good explanation in your commit
> log of why that's necessary.

Well, if I'd only knew exactly why. All these patches are really the
result of many days of trial and error, with multiple drivers (musb,
cppi41, ...) involved. And as I said - I have no documentation of the
musb core itself.

So yes, I can do it the other way around and pass a flag, but what I
can't provide is a good explanantion why the dsps glue behaves
differently here than others. I'm curious myself, and all I know is that
with this reset in place, things work as expected on AM33xx.


Thanks for your feedback,
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
Felipe Balbi Nov. 25, 2013, 8:46 p.m. UTC | #5
Hi,

On Mon, Nov 25, 2013 at 09:26:55PM +0100, Daniel Mack wrote:
> On 11/25/2013 09:08 PM, Felipe Balbi wrote:
> > On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
> >>>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> >>>> index e977441..24e46c0 100644
> >>>> --- a/drivers/usb/musb/musb_virthub.c
> >>>> +++ b/drivers/usb/musb/musb_virthub.c
> >>>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -static void musb_port_reset(struct musb *musb, bool do_reset)
> >>>> +void musb_port_reset(struct musb *musb, bool do_reset)
> >>>
> >>> NAK, this should not be called from the glue layers at all. If anything
> >>> call from musb_core's resume callback. That will only be called after
> >>> parent's resume has been called anyway.
> >>
> >> Given that this driver is successfully used with suspend/resume on other
> >> platforms without that reset, but it's clearly needed for dsps, I'm
> >> fairly sure this should be a glue-layer specific thing. Hence the change.
> >>
> >> I think it'll break things on other platforms if we unconditionally
> >> reset the port after resume, but I can't test it. Anyone?
> > 
> > your original commit log only says "we need to issue port reset" but it
> > never explains why. It could very well be you just found a device which
> > needs to be reset when coming out of suspend and it misses a quirk flag ?
> 
> I think I can really rule that out, as I tested with multiple USB sticks
> and also tested the same sticks on other embedded platforms.
> 
> > In any case, those functions should never be called by the glue, if
> > reset needs to be called, it must be called by musb-hdrc.ko, if you need
> > a flag, pass a flag but I need a really good explanation in your commit
> > log of why that's necessary.
> 
> Well, if I'd only knew exactly why. All these patches are really the
> result of many days of trial and error, with multiple drivers (musb,
> cppi41, ...) involved. And as I said - I have no documentation of the
> musb core itself.
> 
> So yes, I can do it the other way around and pass a flag, but what I
> can't provide is a good explanantion why the dsps glue behaves
> differently here than others. I'm curious myself, and all I know is that
> with this reset in place, things work as expected on AM33xx.

ok, then let's pass a flag. Meanwhile I'll try to figure out internally
why we need that reset. As Alan said, usb-persist should already for a
bus-reset when resuming, right ?
Daniel Mack Nov. 25, 2013, 8:54 p.m. UTC | #6
On 11/25/2013 09:46 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 25, 2013 at 09:26:55PM +0100, Daniel Mack wrote:
>> On 11/25/2013 09:08 PM, Felipe Balbi wrote:
>>> On Mon, Nov 25, 2013 at 09:04:16PM +0100, Daniel Mack wrote:
>>>>>> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
>>>>>> index e977441..24e46c0 100644
>>>>>> --- a/drivers/usb/musb/musb_virthub.c
>>>>>> +++ b/drivers/usb/musb/musb_virthub.c
>>>>>> @@ -109,7 +109,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> -static void musb_port_reset(struct musb *musb, bool do_reset)
>>>>>> +void musb_port_reset(struct musb *musb, bool do_reset)
>>>>>
>>>>> NAK, this should not be called from the glue layers at all. If anything
>>>>> call from musb_core's resume callback. That will only be called after
>>>>> parent's resume has been called anyway.
>>>>
>>>> Given that this driver is successfully used with suspend/resume on other
>>>> platforms without that reset, but it's clearly needed for dsps, I'm
>>>> fairly sure this should be a glue-layer specific thing. Hence the change.
>>>>
>>>> I think it'll break things on other platforms if we unconditionally
>>>> reset the port after resume, but I can't test it. Anyone?
>>>
>>> your original commit log only says "we need to issue port reset" but it
>>> never explains why. It could very well be you just found a device which
>>> needs to be reset when coming out of suspend and it misses a quirk flag ?
>>
>> I think I can really rule that out, as I tested with multiple USB sticks
>> and also tested the same sticks on other embedded platforms.
>>
>>> In any case, those functions should never be called by the glue, if
>>> reset needs to be called, it must be called by musb-hdrc.ko, if you need
>>> a flag, pass a flag but I need a really good explanation in your commit
>>> log of why that's necessary.
>>
>> Well, if I'd only knew exactly why. All these patches are really the
>> result of many days of trial and error, with multiple drivers (musb,
>> cppi41, ...) involved. And as I said - I have no documentation of the
>> musb core itself.
>>
>> So yes, I can do it the other way around and pass a flag, but what I
>> can't provide is a good explanantion why the dsps glue behaves
>> differently here than others. I'm curious myself, and all I know is that
>> with this reset in place, things work as expected on AM33xx.
> 
> ok, then let's pass a flag. Meanwhile I'll try to figure out internally
> why we need that reset.

Well, we don't need a reset, I wasn't quite precise here. What we need
is to manually *de*assert the reset when we resume. Note the 'false'
flag that is passed to musb_port_reset().

> As Alan said, usb-persist should already for a
> bus-reset when resuming, right ?

The explicit un-reset is really needed, otherwise the port will
reenumerate the device, and a previously mounted filesystem will become
invalid of course. The log looks like this in that case:

[   17.045641] PM: resume of devices complete after 166.084 msecs
[   17.056461] PM: Sending message for resetting M3 state machine
[   17.063451] Restarting tasks ... [   17.071767] usb 1-1: USB
disconnect, device number 2
done.
[   17.403402] usb 1-1: new high-speed USB device number 3 using musb-hdrc
[   17.766959] usb 1-1: New USB device found, idVendor=058f, idProduct=6387
[   17.774849] usb 1-1: New USB device strings: Mfr=1, Product=2,
SerialNumber=3



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
Felipe Balbi Nov. 25, 2013, 8:55 p.m. UTC | #7
Hi,

On Mon, Nov 25, 2013 at 09:54:56PM +0100, Daniel Mack wrote:
> > As Alan said, usb-persist should already for a
> > bus-reset when resuming, right ?
> 
> The explicit un-reset is really needed, otherwise the port will
> reenumerate the device, and a previously mounted filesystem will become
> invalid of course. The log looks like this in that case:
> 
> [   17.045641] PM: resume of devices complete after 166.084 msecs
> [   17.056461] PM: Sending message for resetting M3 state machine
> [   17.063451] Restarting tasks ... [   17.071767] usb 1-1: USB
> disconnect, device number 2
> done.
> [   17.403402] usb 1-1: new high-speed USB device number 3 using musb-hdrc
> [   17.766959] usb 1-1: New USB device found, idVendor=058f, idProduct=6387
> [   17.774849] usb 1-1: New USB device strings: Mfr=1, Product=2,
> SerialNumber=3

got it now ;-) Thanks
diff mbox

Patch

diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 29f7cd7..a423037 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -295,6 +295,7 @@  struct musb {
 
 	irqreturn_t		(*isr)(int, void *);
 	struct work_struct	irq_work;
+
 	u16			hwvers;
 
 	u16			intrrxe;
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index e57d712..361ddf8 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -112,6 +112,19 @@  struct dsps_musb_wrapper {
 	u8		poll_seconds;
 };
 
+/*
+ * register shadow for suspend
+ */
+struct dsps_context {
+	u32 control;
+	u32 epintr;
+	u32 coreintr;
+	u32 phy_utmi;
+	u32 mode;
+	u32 tx_mode;
+	u32 rx_mode;
+};
+
 /**
  * DSPS glue structure.
  */
@@ -121,6 +134,8 @@  struct dsps_glue {
 	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
 	struct timer_list timer;	/* otg_workaround timer */
 	unsigned long last_timer;    /* last timer data for each instance */
+
+	struct dsps_context context;
 };
 
 static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
@@ -506,6 +521,7 @@  static int dsps_create_musb_pdev(struct dsps_glue *glue,
 	}
 	pdata.config = config;
 	pdata.platform_ops = &dsps_ops;
+	pdata.restore_after_suspend = 1;
 
 	config->num_eps = get_int_prop(dn, "mentor,num-eps");
 	config->ram_bits = get_int_prop(dn, "mentor,ram-bits");
@@ -632,11 +648,54 @@  static const struct of_device_id musb_dsps_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, musb_dsps_of_match);
 
+#ifdef CONFIG_PM
+static int dsps_suspend(struct device *dev)
+{
+	struct dsps_glue *glue = dev_get_drvdata(dev);
+	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	struct musb *musb = platform_get_drvdata(glue->musb);
+	void __iomem *mbase = musb->ctrl_base;
+
+	glue->context.control = dsps_readl(mbase, wrp->control);
+	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
+	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
+	glue->context.phy_utmi = dsps_readl(mbase, wrp->phy_utmi);
+	glue->context.mode = dsps_readl(mbase, wrp->mode);
+	glue->context.tx_mode = dsps_readl(mbase, wrp->tx_mode);
+	glue->context.rx_mode = dsps_readl(mbase, wrp->rx_mode);
+
+	return 0;
+}
+
+static int dsps_resume(struct device *dev)
+{
+	struct dsps_glue *glue = dev_get_drvdata(dev);
+	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	struct musb *musb = platform_get_drvdata(glue->musb);
+	void __iomem *mbase = musb->ctrl_base;
+
+	dsps_writel(mbase, wrp->control, glue->context.control);
+	dsps_writel(mbase, wrp->epintr_set, glue->context.epintr);
+	dsps_writel(mbase, wrp->coreintr_set, glue->context.coreintr);
+	dsps_writel(mbase, wrp->phy_utmi, glue->context.phy_utmi);
+	dsps_writel(mbase, wrp->mode, glue->context.mode);
+	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
+	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
+
+	musb_port_reset(musb, false);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(dsps_pm_ops, dsps_suspend, dsps_resume);
+
 static struct platform_driver dsps_usbss_driver = {
 	.probe		= dsps_probe,
 	.remove         = dsps_remove,
 	.driver         = {
 		.name   = "musb-dsps",
+		.pm	= &dsps_pm_ops,
 		.of_match_table	= musb_dsps_of_match,
 	},
 };
diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
index e660af9..7436c24 100644
--- a/drivers/usb/musb/musb_host.h
+++ b/drivers/usb/musb/musb_host.h
@@ -93,6 +93,7 @@  extern void musb_root_disconnect(struct musb *musb);
 extern void musb_host_resume_root_hub(struct musb *musb);
 extern void musb_host_poke_root_hub(struct musb *musb);
 extern void musb_port_suspend(struct musb *musb, bool do_suspend);
+extern void musb_port_reset(struct musb *musb, bool do_reset);
 #else
 static inline struct musb *hcd_to_musb(struct usb_hcd *hcd)
 {
@@ -123,6 +124,7 @@  static inline void musb_host_resume_root_hub(struct musb *musb)	{}
 static inline void musb_host_poll_rh_status(struct musb *musb)	{}
 static inline void musb_host_poke_root_hub(struct musb *musb)	{}
 static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
+static inline void musb_port_reset(struct musb *musb)		{}
 #endif
 
 struct usb_hcd;
diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
index e977441..24e46c0 100644
--- a/drivers/usb/musb/musb_virthub.c
+++ b/drivers/usb/musb/musb_virthub.c
@@ -109,7 +109,7 @@  void musb_port_suspend(struct musb *musb, bool do_suspend)
 	}
 }
 
-static void musb_port_reset(struct musb *musb, bool do_reset)
+void musb_port_reset(struct musb *musb, bool do_reset)
 {
 	u8		power;
 	void __iomem	*mbase = musb->mregs;