Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
On 18 May 2016 at 19:21, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget >> *g, >> * even though host mode might be active. Don't actually perform >> * device-specific initialization until device mode is activated. >> */ >> >> + if (pm_runtime_suspended(dwc->dev)) { >> + spin_unlock_irqrestore(&dwc->lock, flags); >> + return 0; >> + } >> >> + ret = __dwc3_gadget_start(dwc); >> + if (ret) >> + goto err1; >> >> So I think the dwc3 core can enter suspend mode before gadget function >> is ready to call the 'usb_gadget_udc_start()' and >> 'usb_udc_connect_control()', then if the dwc3 core has entered >> suspended mode, we need to return success when starting the gadget, >> and leave the gadget starting action from gadget resume. What do you >> think about that? Thanks. > > Well, if this makes it work properly. Then, yeah; looks okay to me. I'll > add this to the patch introducing runtime PM. OK. >>> >>> I've updated the branch with slightly modified version of your >>> changes. Can you test again just to make sure it still works ? >>> >>> Basically, here's what I did: >>> >>> on dwc3_gadget_start: >>> >>> - __dwc3_gadget_start(dwc); >>> + if (pm_runtime_active(dwc->dev)) >>> + __dwc3_gadget_start(dwc); >>> + >> >> Great. >> >>> >>> on run_stop, I kept the same thing. >>> >>> you just need to replace "usb: dwc3: implement runtime PM" with the new >>> version from my branch. >> >> Yeah, it can work well on my platform with your new patch. > > cool, thanks again :-) I'll drop my "not for merging note" and add your > "Tested-by" (assuming it's okay for you that I do it). It's okay for me. Thanks. > > cheers > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: > @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g, > * even though host mode might be active. Don't actually perform > * device-specific initialization until device mode is activated. > */ > > + if (pm_runtime_suspended(dwc->dev)) { > + spin_unlock_irqrestore(&dwc->lock, flags); > + return 0; > + } > > + ret = __dwc3_gadget_start(dwc); > + if (ret) > + goto err1; > > So I think the dwc3 core can enter suspend mode before gadget function > is ready to call the 'usb_gadget_udc_start()' and > 'usb_udc_connect_control()', then if the dwc3 core has entered > suspended mode, we need to return success when starting the gadget, > and leave the gadget starting action from gadget resume. What do you > think about that? Thanks. Well, if this makes it work properly. Then, yeah; looks okay to me. I'll add this to the patch introducing runtime PM. >>> >>> OK. >> >> I've updated the branch with slightly modified version of your >> changes. Can you test again just to make sure it still works ? >> >> Basically, here's what I did: >> >> on dwc3_gadget_start: >> >> - __dwc3_gadget_start(dwc); >> + if (pm_runtime_active(dwc->dev)) >> + __dwc3_gadget_start(dwc); >> + > > Great. > >> >> on run_stop, I kept the same thing. >> >> you just need to replace "usb: dwc3: implement runtime PM" with the new >> version from my branch. > > Yeah, it can work well on my platform with your new patch. cool, thanks again :-) I'll drop my "not for merging note" and add your "Tested-by" (assuming it's okay for you that I do it). cheers -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, On 18 May 2016 at 18:22, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) { u32 reg; u32 timeout = 500, i; + if (pm_runtime_suspended(dwc->dev)) + return 0; (2) @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g, * even though host mode might be active. Don't actually perform * device-specific initialization until device mode is activated. */ + if (pm_runtime_suspended(dwc->dev)) { + spin_unlock_irqrestore(&dwc->lock, flags); + return 0; + } + ret = __dwc3_gadget_start(dwc); + if (ret) + goto err1; So I think the dwc3 core can enter suspend mode before gadget function is ready to call the 'usb_gadget_udc_start()' and 'usb_udc_connect_control()', then if the dwc3 core has entered suspended mode, we need to return success when starting the gadget, and leave the gadget starting action from gadget resume. What do you think about that? Thanks. >>> >>> Well, if this makes it work properly. Then, yeah; looks okay to me. I'll >>> add this to the patch introducing runtime PM. >> >> OK. > > I've updated the branch with slightly modified version of your > changes. Can you test again just to make sure it still works ? > > Basically, here's what I did: > > on dwc3_gadget_start: > > - __dwc3_gadget_start(dwc); > + if (pm_runtime_active(dwc->dev)) > + __dwc3_gadget_start(dwc); > + Great. > > on run_stop, I kept the same thing. > > you just need to replace "usb: dwc3: implement runtime PM" with the new > version from my branch. Yeah, it can work well on my platform with your new patch. > > cheers > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: >>> @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3 >>> *dwc, int is_on, int suspend) >>> { >>> u32 reg; >>> u32 timeout = 500, i; >>> >>> + if (pm_runtime_suspended(dwc->dev)) >>> + return 0; >>> >>> (2) >>> @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g, >>> * even though host mode might be active. Don't actually perform >>> * device-specific initialization until device mode is activated. >>> */ >>> >>> + if (pm_runtime_suspended(dwc->dev)) { >>> + spin_unlock_irqrestore(&dwc->lock, flags); >>> + return 0; >>> + } >>> >>> + ret = __dwc3_gadget_start(dwc); >>> + if (ret) >>> + goto err1; >>> >>> So I think the dwc3 core can enter suspend mode before gadget function >>> is ready to call the 'usb_gadget_udc_start()' and >>> 'usb_udc_connect_control()', then if the dwc3 core has entered >>> suspended mode, we need to return success when starting the gadget, >>> and leave the gadget starting action from gadget resume. What do you >>> think about that? Thanks. >> >> Well, if this makes it work properly. Then, yeah; looks okay to me. I'll >> add this to the patch introducing runtime PM. > > OK. I've updated the branch with slightly modified version of your changes. Can you test again just to make sure it still works ? Basically, here's what I did: on dwc3_gadget_start: - __dwc3_gadget_start(dwc); + if (pm_runtime_active(dwc->dev)) + __dwc3_gadget_start(dwc); + on run_stop, I kept the same thing. you just need to replace "usb: dwc3: implement runtime PM" with the new version from my branch. cheers -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
On 18 May 2016 at 18:12, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: Baolin Wang writes: > Make sense. cool, if you wanna work on it, let me know and I can give some details of what I have in mind. >>> >>> Could you explain details to me, and I wanna continue to optimize the >>> power management things. Thanks. >> >> I have it half-way done. Have a look at my dwc3-fix-suspend branch on >> k.org. I haven't sent because I'm not getting a PME event. Can you test >> on your end and let me know what happens? >> >> I applied some patches (showing below) about suspend/resume at your >> dwc3-fix-suspend branch with my glue layer runtime_* callbacks on my >> testing platform. >> usb: dwc3: implement runtime PM >> usb: dwc3: core: simplify suspend/resume operations >> usb: dwc3: core: re-factor init and exit paths >> usb: dwc3: core: get rid of DWC3_PM_OPS macro >> usb: dwc3: gadget: fix gadget suspend/resume >> usb: dwc3: gadget: re-factor ->udc_start and ->udc_stop >> >> Then I tested the cable connect/disconnect action to see the dwc core >> resume/suspend. It looks work well on my platform, except that I did >> some extra 2 modifications like below: >> (1) >> @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3 >> *dwc, int is_on, int suspend) >> { >> u32 reg; >> u32 timeout = 500, i; >> >> + if (pm_runtime_suspended(dwc->dev)) >> + return 0; >> >> (2) >> @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g, >> * even though host mode might be active. Don't actually perform >> * device-specific initialization until device mode is activated. >> */ >> >> + if (pm_runtime_suspended(dwc->dev)) { >> + spin_unlock_irqrestore(&dwc->lock, flags); >> + return 0; >> + } >> >> + ret = __dwc3_gadget_start(dwc); >> + if (ret) >> + goto err1; >> >> So I think the dwc3 core can enter suspend mode before gadget function >> is ready to call the 'usb_gadget_udc_start()' and >> 'usb_udc_connect_control()', then if the dwc3 core has entered >> suspended mode, we need to return success when starting the gadget, >> and leave the gadget starting action from gadget resume. What do you >> think about that? Thanks. > > Well, if this makes it work properly. Then, yeah; looks okay to me. I'll > add this to the patch introducing runtime PM. OK. > > Thanks a lot for testing on your side :-) You are welcome:) > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: >>> Baolin Wang writes: Make sense. >>> >>> cool, if you wanna work on it, let me know and I can give some details >>> of what I have in mind. >> >> Could you explain details to me, and I wanna continue to optimize the >> power management things. Thanks. > > I have it half-way done. Have a look at my dwc3-fix-suspend branch on > k.org. I haven't sent because I'm not getting a PME event. Can you test > on your end and let me know what happens? > > I applied some patches (showing below) about suspend/resume at your > dwc3-fix-suspend branch with my glue layer runtime_* callbacks on my > testing platform. > usb: dwc3: implement runtime PM > usb: dwc3: core: simplify suspend/resume operations > usb: dwc3: core: re-factor init and exit paths > usb: dwc3: core: get rid of DWC3_PM_OPS macro > usb: dwc3: gadget: fix gadget suspend/resume > usb: dwc3: gadget: re-factor ->udc_start and ->udc_stop > > Then I tested the cable connect/disconnect action to see the dwc core > resume/suspend. It looks work well on my platform, except that I did > some extra 2 modifications like below: > (1) > @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3 > *dwc, int is_on, int suspend) > { > u32 reg; > u32 timeout = 500, i; > > + if (pm_runtime_suspended(dwc->dev)) > + return 0; > > (2) > @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g, > * even though host mode might be active. Don't actually perform > * device-specific initialization until device mode is activated. > */ > > + if (pm_runtime_suspended(dwc->dev)) { > + spin_unlock_irqrestore(&dwc->lock, flags); > + return 0; > + } > > + ret = __dwc3_gadget_start(dwc); > + if (ret) > + goto err1; > > So I think the dwc3 core can enter suspend mode before gadget function > is ready to call the 'usb_gadget_udc_start()' and > 'usb_udc_connect_control()', then if the dwc3 core has entered > suspended mode, we need to return success when starting the gadget, > and leave the gadget starting action from gadget resume. What do you > think about that? Thanks. Well, if this makes it work properly. Then, yeah; looks okay to me. I'll add this to the patch introducing runtime PM. Thanks a lot for testing on your side :-) -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi Felipe, On 17 May 2016 at 18:47, Baolin Wang wrote: > On 17 May 2016 at 17:25, Felipe Balbi wrote: >> >> Hi, >> >> Baolin Wang writes: >>> Make sense. >> >> cool, if you wanna work on it, let me know and I can give some details >> of what I have in mind. > > Could you explain details to me, and I wanna continue to optimize the > power management things. Thanks. I have it half-way done. Have a look at my dwc3-fix-suspend branch on k.org. I haven't sent because I'm not getting a PME event. Can you test on your end and let me know what happens? I applied some patches (showing below) about suspend/resume at your dwc3-fix-suspend branch with my glue layer runtime_* callbacks on my testing platform. usb: dwc3: implement runtime PM usb: dwc3: core: simplify suspend/resume operations usb: dwc3: core: re-factor init and exit paths usb: dwc3: core: get rid of DWC3_PM_OPS macro usb: dwc3: gadget: fix gadget suspend/resume usb: dwc3: gadget: re-factor ->udc_start and ->udc_stop Then I tested the cable connect/disconnect action to see the dwc core resume/suspend. It looks work well on my platform, except that I did some extra 2 modifications like below: (1) @@ -1485,16 +1490,11 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) { u32 reg; u32 timeout = 500, i; + if (pm_runtime_suspended(dwc->dev)) + return 0; (2) @@ -1748,15 +1754,25 @@ static int dwc3_gadget_start(struct usb_gadget *g, * even though host mode might be active. Don't actually perform * device-specific initialization until device mode is activated. */ + if (pm_runtime_suspended(dwc->dev)) { + spin_unlock_irqrestore(&dwc->lock, flags); + return 0; + } + ret = __dwc3_gadget_start(dwc); + if (ret) + goto err1; So I think the dwc3 core can enter suspend mode before gadget function is ready to call the 'usb_gadget_udc_start()' and 'usb_udc_connect_control()', then if the dwc3 core has entered suspended mode, we need to return success when starting the gadget, and leave the gadget starting action from gadget resume. What do you think about that? Thanks. >>> >>> OK. I try to test on my platform to see what will happen. >> >> great, thanks. You might need to patch your glue layer with proper >> runtime_* callbacks, btw. > > Make sense. Thanks. > >> >> -- >> balbi > > > > -- > Baolin.wang > Best Regards -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
On 17 May 2016 at 17:25, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> Make sense. > > cool, if you wanna work on it, let me know and I can give some details > of what I have in mind. Could you explain details to me, and I wanna continue to optimize the power management things. Thanks. >>> >>> I have it half-way done. Have a look at my dwc3-fix-suspend branch on >>> k.org. I haven't sent because I'm not getting a PME event. Can you test >>> on your end and let me know what happens? >> >> OK. I try to test on my platform to see what will happen. > > great, thanks. You might need to patch your glue layer with proper > runtime_* callbacks, btw. Make sense. Thanks. > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: > Make sense. cool, if you wanna work on it, let me know and I can give some details of what I have in mind. >>> >>> Could you explain details to me, and I wanna continue to optimize the >>> power management things. Thanks. >> >> I have it half-way done. Have a look at my dwc3-fix-suspend branch on >> k.org. I haven't sent because I'm not getting a PME event. Can you test >> on your end and let me know what happens? > > OK. I try to test on my platform to see what will happen. great, thanks. You might need to patch your glue layer with proper runtime_* callbacks, btw. -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
On 17 May 2016 at 16:00, Felipe Balbi wrote: > > Hi > > Baolin Wang writes: >> Hi Felipe, >> >> On 13 May 2016 at 20:46, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Baolin Wang writes: >>> why does it need restart? Why is dwc3 powered off? Who powers it off? >> >> Because when the dwc3 Vbus is off (no cable pluging in now), >> especially for some mobile device, the system need to power off the >> dwc3 to save power in this situation. > > but dwc3 doesn't do this by itself, so who's doing it? Yes, the dwc3 clock is controlled by the Soc system, so the Soc system can disable the dwc3 clock when there is no cable plugging in. >>> >>> understood. >>> >>> This looks like a *really* bad power management implementation. Do you >>> have hibernation enabled? Do you have Clock gating enabled? Which dwc3 >>> version are you using? How was it configured? >> >> This is not hibernation, we want to power off the dwc3 to save power >> when no cable plugging in. Yes, we have clock gating, at this >> situation we will disable the clock and shutdown the phy to save >> power. For mobile device, most time no cable plugging in, so we need >> to think about the power consuming. How do you think this requirement? > > Well, seems like you're missing *proper* runtime PM. I've been meaning > to work on it for weeks, but I still have a few other things to do > before I get to that. In any case, we don't need to do what you did > here. There are better ways. Make sense. >>> >>> cool, if you wanna work on it, let me know and I can give some details >>> of what I have in mind. >> >> Could you explain details to me, and I wanna continue to optimize the >> power management things. Thanks. > > I have it half-way done. Have a look at my dwc3-fix-suspend branch on > k.org. I haven't sent because I'm not getting a PME event. Can you test > on your end and let me know what happens? OK. I try to test on my platform to see what will happen. > > Note that if cable is disconnected, we will drop RUN/STOP bit. On > runtime_resume, we will restart the controller from scratch (skipping > memory allocations, of course) > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi Baolin Wang writes: > Hi Felipe, > > On 13 May 2016 at 20:46, Felipe Balbi wrote: >> >> Hi, >> >> Baolin Wang writes: >> why does it need restart? Why is dwc3 powered off? Who powers it off? > > Because when the dwc3 Vbus is off (no cable pluging in now), > especially for some mobile device, the system need to power off the > dwc3 to save power in this situation. but dwc3 doesn't do this by itself, so who's doing it? >>> >>> Yes, the dwc3 clock is controlled by the Soc system, so the Soc system >>> can disable the dwc3 clock when there is no cable plugging in. >> >> understood. >> >> This looks like a *really* bad power management implementation. Do you >> have hibernation enabled? Do you have Clock gating enabled? Which dwc3 >> version are you using? How was it configured? > > This is not hibernation, we want to power off the dwc3 to save power > when no cable plugging in. Yes, we have clock gating, at this > situation we will disable the clock and shutdown the phy to save > power. For mobile device, most time no cable plugging in, so we need > to think about the power consuming. How do you think this requirement? Well, seems like you're missing *proper* runtime PM. I've been meaning to work on it for weeks, but I still have a few other things to do before I get to that. In any case, we don't need to do what you did here. There are better ways. >>> >>> Make sense. >> >> cool, if you wanna work on it, let me know and I can give some details >> of what I have in mind. > > Could you explain details to me, and I wanna continue to optimize the > power management things. Thanks. I have it half-way done. Have a look at my dwc3-fix-suspend branch on k.org. I haven't sent because I'm not getting a PME event. Can you test on your end and let me know what happens? Note that if cable is disconnected, we will drop RUN/STOP bit. On runtime_resume, we will restart the controller from scratch (skipping memory allocations, of course) -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi Felipe, On 13 May 2016 at 20:46, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: > why does it need restart? Why is dwc3 powered off? Who powers it off? Because when the dwc3 Vbus is off (no cable pluging in now), especially for some mobile device, the system need to power off the dwc3 to save power in this situation. >>> >>> but dwc3 doesn't do this by itself, so who's doing it? >> >> Yes, the dwc3 clock is controlled by the Soc system, so the Soc system >> can disable the dwc3 clock when there is no cable plugging in. > > understood. > > This looks like a *really* bad power management implementation. Do you > have hibernation enabled? Do you have Clock gating enabled? Which dwc3 > version are you using? How was it configured? This is not hibernation, we want to power off the dwc3 to save power when no cable plugging in. Yes, we have clock gating, at this situation we will disable the clock and shutdown the phy to save power. For mobile device, most time no cable plugging in, so we need to think about the power consuming. How do you think this requirement? >>> >>> Well, seems like you're missing *proper* runtime PM. I've been meaning >>> to work on it for weeks, but I still have a few other things to do >>> before I get to that. In any case, we don't need to do what you did >>> here. There are better ways. >> >> Make sense. > > cool, if you wanna work on it, let me know and I can give some details > of what I have in mind. Could you explain details to me, and I wanna continue to optimize the power management things. Thanks. > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
On 13 May 2016 at 20:46, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: > why does it need restart? Why is dwc3 powered off? Who powers it off? Because when the dwc3 Vbus is off (no cable pluging in now), especially for some mobile device, the system need to power off the dwc3 to save power in this situation. >>> >>> but dwc3 doesn't do this by itself, so who's doing it? >> >> Yes, the dwc3 clock is controlled by the Soc system, so the Soc system >> can disable the dwc3 clock when there is no cable plugging in. > > understood. > > This looks like a *really* bad power management implementation. Do you > have hibernation enabled? Do you have Clock gating enabled? Which dwc3 > version are you using? How was it configured? This is not hibernation, we want to power off the dwc3 to save power when no cable plugging in. Yes, we have clock gating, at this situation we will disable the clock and shutdown the phy to save power. For mobile device, most time no cable plugging in, so we need to think about the power consuming. How do you think this requirement? >>> >>> Well, seems like you're missing *proper* runtime PM. I've been meaning >>> to work on it for weeks, but I still have a few other things to do >>> before I get to that. In any case, we don't need to do what you did >>> here. There are better ways. >> >> Make sense. > > cool, if you wanna work on it, let me know and I can give some details > of what I have in mind. OK. I would like to do that, please help to give me some details. Thanks. > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: why does it need restart? Why is dwc3 powered off? Who powers it off? >>> >>> Because when the dwc3 Vbus is off (no cable pluging in now), >>> especially for some mobile device, the system need to power off the >>> dwc3 to save power in this situation. >> >> but dwc3 doesn't do this by itself, so who's doing it? > > Yes, the dwc3 clock is controlled by the Soc system, so the Soc system > can disable the dwc3 clock when there is no cable plugging in. understood. This looks like a *really* bad power management implementation. Do you have hibernation enabled? Do you have Clock gating enabled? Which dwc3 version are you using? How was it configured? >>> >>> This is not hibernation, we want to power off the dwc3 to save power >>> when no cable plugging in. Yes, we have clock gating, at this >>> situation we will disable the clock and shutdown the phy to save >>> power. For mobile device, most time no cable plugging in, so we need >>> to think about the power consuming. How do you think this requirement? >> >> Well, seems like you're missing *proper* runtime PM. I've been meaning >> to work on it for weeks, but I still have a few other things to do >> before I get to that. In any case, we don't need to do what you did >> here. There are better ways. > > Make sense. cool, if you wanna work on it, let me know and I can give some details of what I have in mind. -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
On 13 May 2016 at 20:09, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 6254b2f..dada5c6 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array { * 1 - -3.5dB de-emphasis * 2 - No de-emphasis * 3 - Reserved + * @can_save_power: set if the gadget will power off when no cable plug in. >>> >>> nope >>> + * @need_restart: set if we need to restart the gadget. >>> >>> why does it need restart? Why is dwc3 powered off? Who powers it off? >> >> Because when the dwc3 Vbus is off (no cable pluging in now), >> especially for some mobile device, the system need to power off the >> dwc3 to save power in this situation. > > but dwc3 doesn't do this by itself, so who's doing it? Yes, the dwc3 clock is controlled by the Soc system, so the Soc system can disable the dwc3 clock when there is no cable plugging in. > >>> This looks like a *really* bad power management implementation. Do you >>> have hibernation enabled? Do you have Clock gating enabled? Which dwc3 >>> version are you using? How was it configured? >> >> This is not hibernation, we want to power off the dwc3 to save power >> when no cable plugging in. Yes, we have clock gating, at this >> situation we will disable the clock and shutdown the phy to save >> power. For mobile device, most time no cable plugging in, so we need >> to think about the power consuming. How do you think this requirement? > > Well, seems like you're missing *proper* runtime PM. I've been meaning > to work on it for weeks, but I still have a few other things to do > before I get to that. In any case, we don't need to do what you did > here. There are better ways. Make sense. > >>> Anyway, which platform are you dealing with? Why is dwc3 off while VBUS >>> is off? How do you handle host mode? >> >> On Spreadtrum platform, for thinking about some mobile devices with > > I meant the SoC ;-) It's their own SoC? Are we getting glue-layer > patches any time soon? Yes, it's their own SoC. Thanks. > >> strict power management. This is just for gadget mode, we don't power >> off the dwc3 when it is host mode. Thanks. > > okay, thanks > > -- > balbi -- Baolin.wang Best Regards
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: >>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>> index 6254b2f..dada5c6 100644 >>> --- a/drivers/usb/dwc3/core.h >>> +++ b/drivers/usb/dwc3/core.h >>> @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array { >>> * 1 - -3.5dB de-emphasis >>> * 2 - No de-emphasis >>> * 3 - Reserved >>> + * @can_save_power: set if the gadget will power off when no cable plug in. >> >> nope >> >>> + * @need_restart: set if we need to restart the gadget. >> >> why does it need restart? Why is dwc3 powered off? Who powers it off? > > Because when the dwc3 Vbus is off (no cable pluging in now), > especially for some mobile device, the system need to power off the > dwc3 to save power in this situation. but dwc3 doesn't do this by itself, so who's doing it? >> This looks like a *really* bad power management implementation. Do you >> have hibernation enabled? Do you have Clock gating enabled? Which dwc3 >> version are you using? How was it configured? > > This is not hibernation, we want to power off the dwc3 to save power > when no cable plugging in. Yes, we have clock gating, at this > situation we will disable the clock and shutdown the phy to save > power. For mobile device, most time no cable plugging in, so we need > to think about the power consuming. How do you think this requirement? Well, seems like you're missing *proper* runtime PM. I've been meaning to work on it for weeks, but I still have a few other things to do before I get to that. In any case, we don't need to do what you did here. There are better ways. >> Anyway, which platform are you dealing with? Why is dwc3 off while VBUS >> is off? How do you handle host mode? > > On Spreadtrum platform, for thinking about some mobile devices with I meant the SoC ;-) It's their own SoC? Are we getting glue-layer patches any time soon? > strict power management. This is just for gadget mode, we don't power > off the dwc3 when it is host mode. Thanks. okay, thanks -- balbi signature.asc Description: PGP signature
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi Felipe, On 13 May 2016 at 18:40, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> Currently on some platforms, the gadget device can be power off to >> save power when the Vbus is off, which means no cable plugging in >> now. In this situation we should defer starting the gadget until the >> gadget device is power on by connecting host. > > okay, you need to be a looot more specific about this. From a > basic look at the patch, there's no way I'll ever accept it, see below > >> Signed-off-by: Baolin Wang >> --- >> drivers/usb/dwc3/core.c |5 +- >> drivers/usb/dwc3/core.h | 14 >> drivers/usb/dwc3/gadget.c| 144 >> ++ >> drivers/usb/dwc3/platform_data.h |1 + >> 4 files changed, 131 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 34277ce..825462a 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -109,7 +109,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) >> * dwc3_soft_reset - Issue soft reset >> * @dwc: Pointer to our controller context structure >> */ >> -static int dwc3_soft_reset(struct dwc3 *dwc) >> +int dwc3_soft_reset(struct dwc3 *dwc) > > this *CANNOT* and *WILL* not be exposed to anything other than > core.c. We don't want anybody else resetting dwc3 willy-nilly. > >> @@ -253,7 +253,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, >> unsigned length) >> * >> * Returns 0 on success otherwise negative errno. >> */ >> -static int dwc3_event_buffers_setup(struct dwc3 *dwc) >> +int dwc3_event_buffers_setup(struct dwc3 *dwc) > > likewise > >> @@ -948,6 +948,7 @@ static int dwc3_probe(struct platform_device *pdev) >> >> dwc->hsphy_interface = pdata->hsphy_interface; >> fladj = pdata->fladj_value; >> + dwc->can_save_power = pdata->can_save_power; > > sounds like a pointless flag IMHO. > >> } >> >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 6254b2f..dada5c6 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array { >> * 1 - -3.5dB de-emphasis >> * 2 - No de-emphasis >> * 3 - Reserved >> + * @can_save_power: set if the gadget will power off when no cable plug in. > > nope > >> + * @need_restart: set if we need to restart the gadget. > > why does it need restart? Why is dwc3 powered off? Who powers it off? Because when the dwc3 Vbus is off (no cable pluging in now), especially for some mobile device, the system need to power off the dwc3 to save power in this situation. > > This looks like a *really* bad power management implementation. Do you > have hibernation enabled? Do you have Clock gating enabled? Which dwc3 > version are you using? How was it configured? This is not hibernation, we want to power off the dwc3 to save power when no cable plugging in. Yes, we have clock gating, at this situation we will disable the clock and shutdown the phy to save power. For mobile device, most time no cable plugging in, so we need to think about the power consuming. How do you think this requirement? Thanks. > >> + * @cable_connected: set if one usb cable is plugging in. > > not necessary, we already infer that from RUN/STOP and reset interrupt. > >> @@ -876,6 +879,9 @@ struct dwc3 { >> >> unsignedtx_de_emphasis_quirk:1; >> unsignedtx_de_emphasis:2; >> + unsignedcan_save_power:1; >> + unsignedneed_restart:1; >> + unsignedcable_connected:1; >> }; >> >> /* >> -- */ >> @@ -1026,6 +1032,8 @@ struct dwc3_gadget_ep_cmd_params { >> /* prototypes */ >> void dwc3_set_mode(struct dwc3 *dwc, u32 mode); >> int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc); >> +int dwc3_soft_reset(struct dwc3 *dwc); >> +int dwc3_event_buffers_setup(struct dwc3 *dwc); > > makes me cringe > >> @@ -1052,6 +1060,8 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum >> dwc3_link_state state); >> int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep, >> unsigned cmd, struct dwc3_gadget_ep_cmd_params *params); >> int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 >> param); >> +void dwc3_gadget_connect(struct dwc3 *dwc); >> +void dwc3_gadget_disconnect(struct dwc3 *dwc); > > hell no > >> #else >> static inline int dwc3_gadget_init(struct dwc3 *dwc) >> { return 0; } >> @@ -1071,6 +1081,10 @@ static inline int dwc3_send_gadget_ep_cmd(struct dwc3 >> *dwc, unsigned ep, >> static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc, >> int cmd, u32 param) >> { return 0; } >> +static inline void dwc3_gadget_connect(struct dwc3 *dwc) >> +{ } >> +static inline voi
Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Hi, Baolin Wang writes: > Currently on some platforms, the gadget device can be power off to > save power when the Vbus is off, which means no cable plugging in > now. In this situation we should defer starting the gadget until the > gadget device is power on by connecting host. okay, you need to be a looot more specific about this. From a basic look at the patch, there's no way I'll ever accept it, see below > Signed-off-by: Baolin Wang > --- > drivers/usb/dwc3/core.c |5 +- > drivers/usb/dwc3/core.h | 14 > drivers/usb/dwc3/gadget.c| 144 > ++ > drivers/usb/dwc3/platform_data.h |1 + > 4 files changed, 131 insertions(+), 33 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 34277ce..825462a 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -109,7 +109,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) > * dwc3_soft_reset - Issue soft reset > * @dwc: Pointer to our controller context structure > */ > -static int dwc3_soft_reset(struct dwc3 *dwc) > +int dwc3_soft_reset(struct dwc3 *dwc) this *CANNOT* and *WILL* not be exposed to anything other than core.c. We don't want anybody else resetting dwc3 willy-nilly. > @@ -253,7 +253,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, > unsigned length) > * > * Returns 0 on success otherwise negative errno. > */ > -static int dwc3_event_buffers_setup(struct dwc3 *dwc) > +int dwc3_event_buffers_setup(struct dwc3 *dwc) likewise > @@ -948,6 +948,7 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc->hsphy_interface = pdata->hsphy_interface; > fladj = pdata->fladj_value; > + dwc->can_save_power = pdata->can_save_power; sounds like a pointless flag IMHO. > } > > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 6254b2f..dada5c6 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array { > * 1 - -3.5dB de-emphasis > * 2 - No de-emphasis > * 3 - Reserved > + * @can_save_power: set if the gadget will power off when no cable plug in. nope > + * @need_restart: set if we need to restart the gadget. why does it need restart? Why is dwc3 powered off? Who powers it off? This looks like a *really* bad power management implementation. Do you have hibernation enabled? Do you have Clock gating enabled? Which dwc3 version are you using? How was it configured? > + * @cable_connected: set if one usb cable is plugging in. not necessary, we already infer that from RUN/STOP and reset interrupt. > @@ -876,6 +879,9 @@ struct dwc3 { > > unsignedtx_de_emphasis_quirk:1; > unsignedtx_de_emphasis:2; > + unsignedcan_save_power:1; > + unsignedneed_restart:1; > + unsignedcable_connected:1; > }; > > /* > -- */ > @@ -1026,6 +1032,8 @@ struct dwc3_gadget_ep_cmd_params { > /* prototypes */ > void dwc3_set_mode(struct dwc3 *dwc, u32 mode); > int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc); > +int dwc3_soft_reset(struct dwc3 *dwc); > +int dwc3_event_buffers_setup(struct dwc3 *dwc); makes me cringe > @@ -1052,6 +1060,8 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum > dwc3_link_state state); > int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep, > unsigned cmd, struct dwc3_gadget_ep_cmd_params *params); > int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 > param); > +void dwc3_gadget_connect(struct dwc3 *dwc); > +void dwc3_gadget_disconnect(struct dwc3 *dwc); hell no > #else > static inline int dwc3_gadget_init(struct dwc3 *dwc) > { return 0; } > @@ -1071,6 +1081,10 @@ static inline int dwc3_send_gadget_ep_cmd(struct dwc3 > *dwc, unsigned ep, > static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc, > int cmd, u32 param) > { return 0; } > +static inline void dwc3_gadget_connect(struct dwc3 *dwc) > +{ } > +static inline void dwc3_gadget_disconnect(struct dwc3 *dwc) > +{ } > #endif > > /* power management interface */ > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 8e4a1b1..90805f9 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1512,13 +1512,75 @@ static int dwc3_gadget_set_selfpowered(struct > usb_gadget *g, > return 0; > } > > +void dwc3_gadget_connect(struct dwc3 *dwc) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&dwc->lock, flags); > + dwc->cable_connected = true; > + spin_unlock_irqrestore(&dwc->lock, flags); > +} > + > +void dwc3_gadget_disconnect(struct dwc3 *dwc) > +{ > + unsigned long flags; > + > +
[PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on
Currently on some platforms, the gadget device can be power off to save power when the Vbus is off, which means no cable plugging in now. In this situation we should defer starting the gadget until the gadget device is power on by connecting host. Signed-off-by: Baolin Wang --- drivers/usb/dwc3/core.c |5 +- drivers/usb/dwc3/core.h | 14 drivers/usb/dwc3/gadget.c| 144 ++ drivers/usb/dwc3/platform_data.h |1 + 4 files changed, 131 insertions(+), 33 deletions(-) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 34277ce..825462a 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -109,7 +109,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) * dwc3_soft_reset - Issue soft reset * @dwc: Pointer to our controller context structure */ -static int dwc3_soft_reset(struct dwc3 *dwc) +int dwc3_soft_reset(struct dwc3 *dwc) { unsigned long timeout; u32 reg; @@ -253,7 +253,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length) * * Returns 0 on success otherwise negative errno. */ -static int dwc3_event_buffers_setup(struct dwc3 *dwc) +int dwc3_event_buffers_setup(struct dwc3 *dwc) { struct dwc3_event_buffer*evt; int n; @@ -948,6 +948,7 @@ static int dwc3_probe(struct platform_device *pdev) dwc->hsphy_interface = pdata->hsphy_interface; fladj = pdata->fladj_value; + dwc->can_save_power = pdata->can_save_power; } dwc->lpm_nyet_threshold = lpm_nyet_threshold; diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 6254b2f..dada5c6 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array { * 1 - -3.5dB de-emphasis * 2 - No de-emphasis * 3 - Reserved + * @can_save_power: set if the gadget will power off when no cable plug in. + * @need_restart: set if we need to restart the gadget. + * @cable_connected: set if one usb cable is plugging in. */ struct dwc3 { struct usb_ctrlrequest *ctrl_req; @@ -876,6 +879,9 @@ struct dwc3 { unsignedtx_de_emphasis_quirk:1; unsignedtx_de_emphasis:2; + unsignedcan_save_power:1; + unsignedneed_restart:1; + unsignedcable_connected:1; }; /* -- */ @@ -1026,6 +1032,8 @@ struct dwc3_gadget_ep_cmd_params { /* prototypes */ void dwc3_set_mode(struct dwc3 *dwc, u32 mode); int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc); +int dwc3_soft_reset(struct dwc3 *dwc); +int dwc3_event_buffers_setup(struct dwc3 *dwc); /* check whether we are on the DWC_usb31 core */ static inline bool dwc3_is_usb31(struct dwc3 *dwc) @@ -1052,6 +1060,8 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state); int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep, unsigned cmd, struct dwc3_gadget_ep_cmd_params *params); int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param); +void dwc3_gadget_connect(struct dwc3 *dwc); +void dwc3_gadget_disconnect(struct dwc3 *dwc); #else static inline int dwc3_gadget_init(struct dwc3 *dwc) { return 0; } @@ -1071,6 +1081,10 @@ static inline int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep, static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc, int cmd, u32 param) { return 0; } +static inline void dwc3_gadget_connect(struct dwc3 *dwc) +{ } +static inline void dwc3_gadget_disconnect(struct dwc3 *dwc) +{ } #endif /* power management interface */ diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 8e4a1b1..90805f9 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1512,13 +1512,75 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g, return 0; } +void dwc3_gadget_connect(struct dwc3 *dwc) +{ + unsigned long flags; + + spin_lock_irqsave(&dwc->lock, flags); + dwc->cable_connected = true; + spin_unlock_irqrestore(&dwc->lock, flags); +} + +void dwc3_gadget_disconnect(struct dwc3 *dwc) +{ + unsigned long flags; + + spin_lock_irqsave(&dwc->lock, flags); + dwc->cable_connected = false; + spin_unlock_irqrestore(&dwc->lock, flags); +} + +static bool dwc3_gadget_is_connected(struct dwc3 *dwc) +{ + /* +* If the gadget is always power on, then no need to check if the +* cable is plugin or not. +*/ + if (!dwc->can_save_power) + return true; + + return dwc->cable_connected; +} + +static int __dwc3_gadget_start(struct dwc3 *dwc); + static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend) { u32