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

[Daniel Mack <zonque@gmail.com>]

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