| Message ID | 15bba89b920e29e27de4cfaac546834fba5d1a76.1555672441.git.arturp@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: Fix and improve power saving modes. | expand |
Hi, On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > - Added a default param "power_saving" to enable or > disable hibernation or partial power down features. > > - Printed hibernation param in hw_params_show and > power_saving param in params_show. > > Signed-off-by: Artur Petrosyan <arturp@synopsys.com> > Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> > --- > drivers/usb/dwc2/core.h | 3 +++ > drivers/usb/dwc2/debugfs.c | 2 ++ > drivers/usb/dwc2/params.c | 19 +++++++++++++------ > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index 30bab8463c96..9221933ab64e 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -373,6 +373,8 @@ enum dwc2_ep0_state { > * case. > * 0 - No (default) > * 1 - Yes > + * @power_saving: Specifies if power saving is enabled or not. If it is > + * enabled power_down functionality will be enabled. > * @power_down: Specifies whether the controller support power_down. > * If power_down is enabled, the controller will enter > * power_down in both peripheral and host mode when Why are you adding a new parameter? power_saving should be exactly the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that anywhere you need it. Having two parameters like you're doing is just asking for them to get out of sync. ...and, in fact, I think they will get out of sync. On rk3288, for instance: -> dwc2_set_default_params() ---> power_saving = true ---> dwc2_set_param_power_down() -----> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL -> set_params(), which is actually dwc2_set_rk_params() ---> power_down = 0 ...so at the end of dwc2_init_params() you will have power_saving = true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems bad. ...and, in fact: # grep '^power' /sys/kernel/debug/*.usb/params /sys/kernel/debug/ff540000.usb/params:power_saving : 1 /sys/kernel/debug/ff540000.usb/params:power_down : 0 /sys/kernel/debug/ff580000.usb/params:power_saving : 1 /sys/kernel/debug/ff580000.usb/params:power_down : 0 ...so while you could fix all of the various set_params() functions, it seems better to just drop this patch since I don't think it buys anything. -Doug
Hi, On 4/27/2019 00:46, Doug Anderson wrote: > Hi, > > On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> - Added a default param "power_saving" to enable or >> disable hibernation or partial power down features. >> >> - Printed hibernation param in hw_params_show and >> power_saving param in params_show. >> >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> >> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> >> --- >> drivers/usb/dwc2/core.h | 3 +++ >> drivers/usb/dwc2/debugfs.c | 2 ++ >> drivers/usb/dwc2/params.c | 19 +++++++++++++------ >> 3 files changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >> index 30bab8463c96..9221933ab64e 100644 >> --- a/drivers/usb/dwc2/core.h >> +++ b/drivers/usb/dwc2/core.h >> @@ -373,6 +373,8 @@ enum dwc2_ep0_state { >> * case. >> * 0 - No (default) >> * 1 - Yes >> + * @power_saving: Specifies if power saving is enabled or not. If it is >> + * enabled power_down functionality will be enabled. >> * @power_down: Specifies whether the controller support power_down. >> * If power_down is enabled, the controller will enter >> * power_down in both peripheral and host mode when > > Why are you adding a new parameter? power_saving should be exactly > the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that > anywhere you need it. Customers should have a parameter using which they will disable entire power saving hibernation and Partial Power Down support. power_down is used to see which power saving mode we got (hibernation/partial power down). > > Having two parameters like you're doing is just asking for them to get > out of sync. ...and, in fact, I think they will get out of sync. On > rk3288, for instance: > > -> dwc2_set_default_params() > ---> power_saving = true > ---> dwc2_set_param_power_down() > -----> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL > -> set_params(), which is actually dwc2_set_rk_params() > ---> power_down = 0 Setting power_down = 0 is a wrong and old option of disabling power saving feature because if we set power_down = 0 then it shows that there is no support for any power saving mode. That is why this patch is introduced to provide an easier way of disabling power saving modes. > > > ...so at the end of dwc2_init_params() you will have power_saving = > true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems > bad. ...and, in fact: > > # grep '^power' /sys/kernel/debug/*.usb/params > /sys/kernel/debug/ff540000.usb/params:power_saving : 1 > /sys/kernel/debug/ff540000.usb/params:power_down : 0 > /sys/kernel/debug/ff580000.usb/params:power_saving : 1 > /sys/kernel/debug/ff580000.usb/params:power_down : 0 > > > ...so while you could fix all of the various set_params() functions, > it seems better to just drop this patch since I don't think it buys > anything. I don't think we should drop this patch. Because, it is introducing the correct way of disabling power saving (hibernation/partial power down modes). Explanation is listed above. > > -Doug >
Hi, On Mon, Apr 29, 2019 at 4:30 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/27/2019 00:46, Doug Anderson wrote: > > Hi, > > > > On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> - Added a default param "power_saving" to enable or > >> disable hibernation or partial power down features. > >> > >> - Printed hibernation param in hw_params_show and > >> power_saving param in params_show. > >> > >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> > >> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> > >> --- > >> drivers/usb/dwc2/core.h | 3 +++ > >> drivers/usb/dwc2/debugfs.c | 2 ++ > >> drivers/usb/dwc2/params.c | 19 +++++++++++++------ > >> 3 files changed, 18 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > >> index 30bab8463c96..9221933ab64e 100644 > >> --- a/drivers/usb/dwc2/core.h > >> +++ b/drivers/usb/dwc2/core.h > >> @@ -373,6 +373,8 @@ enum dwc2_ep0_state { > >> * case. > >> * 0 - No (default) > >> * 1 - Yes > >> + * @power_saving: Specifies if power saving is enabled or not. If it is > >> + * enabled power_down functionality will be enabled. > >> * @power_down: Specifies whether the controller support power_down. > >> * If power_down is enabled, the controller will enter > >> * power_down in both peripheral and host mode when > > > > Why are you adding a new parameter? power_saving should be exactly > > the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that > > anywhere you need it. > Customers should have a parameter using which they will disable entire > power saving hibernation and Partial Power Down support. > > power_down is used to see which power saving mode we got > (hibernation/partial power down). > > > > > Having two parameters like you're doing is just asking for them to get > > out of sync. ...and, in fact, I think they will get out of sync. On > > rk3288, for instance: > > > > -> dwc2_set_default_params() > > ---> power_saving = true > > ---> dwc2_set_param_power_down() > > -----> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL > > -> set_params(), which is actually dwc2_set_rk_params() > > ---> power_down = 0 > Setting power_down = 0 is a wrong and old option of disabling power > saving feature because if we set power_down = 0 then it shows that there > is no support for any power saving mode. That is why this patch is > introduced to provide an easier way of disabling power saving modes. If setting "power_down = 0" is wrong then please update your patch to remove all the mainline code that sets power_down to 0. Presumably this means you'd want to convert that code over to using "power_saving = False". Perhaps then I can see your vision of how this works more clearly. NOTE: I'm curious how you envision what someone would do if they had a core that supported hibernation but they only wanted to enable partial power down. I guess then they'd have to set "power_saving = True" and then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your vision of the world is: // Example 1: Core supports power savings but we want disabled // (no code since this is the default) // Example 2: Pick the best power saving available params->power_saving = True // Example 3: Supports hibernation, but we only want partial: params->power_saving = True params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL My vision of the world is: // Example 1: Core supports power savings but we want disabled params->power_down = DWC2_POWER_DOWN_PARAM_NONE // Example 2: Pick the best power saving available // (no code since this is the default) // Example 3: Supports hibernation, but we only want partial: params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL I like that in my vision of the world "pick the best" is the default (though I suppose we need to fix the driver so it actually works) and that there's only one variable so you don't have extra confusion. > > ...so at the end of dwc2_init_params() you will have power_saving = > > true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems > > bad. ...and, in fact: > > > > # grep '^power' /sys/kernel/debug/*.usb/params > > /sys/kernel/debug/ff540000.usb/params:power_saving : 1 > > /sys/kernel/debug/ff540000.usb/params:power_down : 0 > > /sys/kernel/debug/ff580000.usb/params:power_saving : 1 > > /sys/kernel/debug/ff580000.usb/params:power_down : 0 > > > > > > ...so while you could fix all of the various set_params() functions, > > it seems better to just drop this patch since I don't think it buys > > anything. > I don't think we should drop this patch. Because, it is introducing the > correct way of disabling power saving (hibernation/partial power down > modes). Explanation is listed above. I personally see no benefit still. It's just as clear to me for someone to set "power_down = DWC2_POWER_DOWN_PARAM_NONE" as it is to set "power_savings = False". -Doug
Hi, On 4/29/2019 21:41, Doug Anderson wrote: > Hi, > > On Mon, Apr 29, 2019 at 4:30 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >> Hi, >> >> On 4/27/2019 00:46, Doug Anderson wrote: >>> Hi, >>> >>> On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan >>> <Arthur.Petrosyan@synopsys.com> wrote: >>>> >>>> - Added a default param "power_saving" to enable or >>>> disable hibernation or partial power down features. >>>> >>>> - Printed hibernation param in hw_params_show and >>>> power_saving param in params_show. >>>> >>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com> >>>> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> >>>> --- >>>> drivers/usb/dwc2/core.h | 3 +++ >>>> drivers/usb/dwc2/debugfs.c | 2 ++ >>>> drivers/usb/dwc2/params.c | 19 +++++++++++++------ >>>> 3 files changed, 18 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>>> index 30bab8463c96..9221933ab64e 100644 >>>> --- a/drivers/usb/dwc2/core.h >>>> +++ b/drivers/usb/dwc2/core.h >>>> @@ -373,6 +373,8 @@ enum dwc2_ep0_state { >>>> * case. >>>> * 0 - No (default) >>>> * 1 - Yes >>>> + * @power_saving: Specifies if power saving is enabled or not. If it is >>>> + * enabled power_down functionality will be enabled. >>>> * @power_down: Specifies whether the controller support power_down. >>>> * If power_down is enabled, the controller will enter >>>> * power_down in both peripheral and host mode when >>> >>> Why are you adding a new parameter? power_saving should be exactly >>> the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that >>> anywhere you need it. >> Customers should have a parameter using which they will disable entire >> power saving hibernation and Partial Power Down support. >> >> power_down is used to see which power saving mode we got >> (hibernation/partial power down). >> >>> >>> Having two parameters like you're doing is just asking for them to get >>> out of sync. ...and, in fact, I think they will get out of sync. On >>> rk3288, for instance: >>> >>> -> dwc2_set_default_params() >>> ---> power_saving = true >>> ---> dwc2_set_param_power_down() >>> -----> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL >>> -> set_params(), which is actually dwc2_set_rk_params() >>> ---> power_down = 0 >> Setting power_down = 0 is a wrong and old option of disabling power >> saving feature because if we set power_down = 0 then it shows that there >> is no support for any power saving mode. That is why this patch is >> introduced to provide an easier way of disabling power saving modes. > > If setting "power_down = 0" is wrong then please update your patch to > remove all the mainline code that sets power_down to 0. Presumably > this means you'd want to convert that code over to using "power_saving > = False". Perhaps then I can see your vision of how this works more > clearly. Yes this is a good idea. > > NOTE: I'm curious how you envision what someone would do if they had a > core that supported hibernation but they only wanted to enable partial > power down. I guess then they'd have to set "power_saving = True" and > then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your > vision of the world is: I have implemented everything based on programming guide and data book. Core can only support hibernation or partial power down based on the configuration parameters. There cannot be two modes simultaneously of power saving only one of them. The power_down flag is set to DWC2_POWER_DOWN_PARAM_PARTIAL , DWC2_POWER_DOWN_PARAM_HIBERNATION or DWC2_POWER_DOWN_PARAM_NONE while checking the hw parameters. So it just indicates which power down mode is supporting the core. > > > // Example 1: Core supports power savings but we want disabled > // (no code since this is the default) > > // Example 2: Pick the best power saving available > params->power_saving = True > > // Example 3: Supports hibernation, but we only want partial: > params->power_saving = True > params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL > > > My vision of the world is: > > // Example 1: Core supports power savings but we want disabled > params->power_down = DWC2_POWER_DOWN_PARAM_NONE If we do so it will show that core doesn't support both hibernation and partial power down but the core actually supports one of them. > > // Example 2: Pick the best power saving available > // (no code since this is the default) > > // Example 3: Supports hibernation, but we only want partial: > params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL There is only one mode available at one core hibernation or partial power down. > > > I like that in my vision of the world "pick the best" is the default > (though I suppose we need to fix the driver so it actually works) and > that there's only one variable so you don't have extra confusion. Confusion will be when let's say core supports hibernation but you set power_down to DWC2_POWER_DOWN_PARAM_NONE. This will mean that core doesn't support no power saving option. But in reality it does and it is hibernation for instance. > > >>> ...so at the end of dwc2_init_params() you will have power_saving = >>> true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems >>> bad. ...and, in fact: >>> >>> # grep '^power' /sys/kernel/debug/*.usb/params >>> /sys/kernel/debug/ff540000.usb/params:power_saving : 1 >>> /sys/kernel/debug/ff540000.usb/params:power_down : 0 >>> /sys/kernel/debug/ff580000.usb/params:power_saving : 1 >>> /sys/kernel/debug/ff580000.usb/params:power_down : 0 >>> >>> >>> ...so while you could fix all of the various set_params() functions, >>> it seems better to just drop this patch since I don't think it buys >>> anything. >> I don't think we should drop this patch. Because, it is introducing the >> correct way of disabling power saving (hibernation/partial power down >> modes). Explanation is listed above. > > I personally see no benefit still. It's just as clear to me for > someone to set "power_down = DWC2_POWER_DOWN_PARAM_NONE" as it is to > set "power_savings = False". Your vision of the world to set "power_down = DWC2_POWER_DOWN_PARAM_NONE" will indicate that there is no power saving mode available on the core. > > -Doug >
Hi, On Tue, Apr 30, 2019 at 5:45 AM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > > If setting "power_down = 0" is wrong then please update your patch to > > remove all the mainline code that sets power_down to 0. Presumably > > this means you'd want to convert that code over to using "power_saving > > = False". Perhaps then I can see your vision of how this works more > > clearly. > Yes this is a good idea. Actually, I have a feeling it won't work, at least not without more changes. ...and that's part of the problem with your patch. Specifically dwc2 works by first filling in the "default" parameters. Then the per-platform config function runs and overrides the defaults. If the per-platform config function overrides the "power_saving" parameter then it will be too late. > > NOTE: I'm curious how you envision what someone would do if they had a > > core that supported hibernation but they only wanted to enable partial > > power down. I guess then they'd have to set "power_saving = True" and > > then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your > > vision of the world is: > I have implemented everything based on programming guide and data book. > Core can only support hibernation or partial power down based on the > configuration parameters. There cannot be two modes simultaneously of > power saving only one of them. Ah, this is new information to me. I assumed they were supersets of each other, so if you supported hibernation you also supported partial power down. Thanks for clearing that up! > The power_down flag is set to DWC2_POWER_DOWN_PARAM_PARTIAL , > DWC2_POWER_DOWN_PARAM_HIBERNATION or DWC2_POWER_DOWN_PARAM_NONE while > checking the hw parameters. So it just indicates which power down mode > is supporting the core. I don't think this is what the params are about. The params are about the currently configured parameters, not about what the core supports. This is why all the other code checks the actual value of the params to decide whether to do power savings. -Doug
On 4/30/2019 19:23, Doug Anderson wrote: > Hi, > > On Tue, Apr 30, 2019 at 5:45 AM Artur Petrosyan > <Arthur.Petrosyan@synopsys.com> wrote: >> >>> If setting "power_down = 0" is wrong then please update your patch to >>> remove all the mainline code that sets power_down to 0. Presumably >>> this means you'd want to convert that code over to using "power_saving >>> = False". Perhaps then I can see your vision of how this works more >>> clearly. >> Yes this is a good idea. > > Actually, I have a feeling it won't work, at least not without more > changes. ...and that's part of the problem with your patch. I have lot of testes which prove that the patch is working. If there is any case that the patch is not working please mention. If possible provide the logs of the failure. > > Specifically dwc2 works by first filling in the "default" parameters. > Then the per-platform config function runs and overrides the defaults. > If the per-platform config function overrides the "power_saving" > parameter then it will be too late. > > >>> NOTE: I'm curious how you envision what someone would do if they had a >>> core that supported hibernation but they only wanted to enable partial >>> power down. I guess then they'd have to set "power_saving = True" and >>> then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your >>> vision of the world is: >> I have implemented everything based on programming guide and data book. >> Core can only support hibernation or partial power down based on the >> configuration parameters. There cannot be two modes simultaneously of >> power saving only one of them. > > Ah, this is new information to me. I assumed they were supersets of > each other, so if you supported hibernation you also supported partial > power down. Thanks for clearing that up! Welcome. All of that information is listed in the data book. > > >> The power_down flag is set to DWC2_POWER_DOWN_PARAM_PARTIAL , >> DWC2_POWER_DOWN_PARAM_HIBERNATION or DWC2_POWER_DOWN_PARAM_NONE while >> checking the hw parameters. So it just indicates which power down mode >> is supporting the core. > > I don't think this is what the params are about. The params are about > the currently configured parameters, not about what the core supports. > This is why all the other code checks the actual value of the params > to decide whether to do power savings. So for the power saving which should be currently configured parameter I have added power_saving flag. > > -Doug >
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 30bab8463c96..9221933ab64e 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -373,6 +373,8 @@ enum dwc2_ep0_state { * case. * 0 - No (default) * 1 - Yes + * @power_saving: Specifies if power saving is enabled or not. If it is + * enabled power_down functionality will be enabled. * @power_down: Specifies whether the controller support power_down. * If power_down is enabled, the controller will enter * power_down in both peripheral and host mode when @@ -469,6 +471,7 @@ struct dwc2_core_params { bool uframe_sched; bool external_id_pin_ctl; + unsigned int power_saving:1; int power_down; #define DWC2_POWER_DOWN_PARAM_NONE 0 #define DWC2_POWER_DOWN_PARAM_PARTIAL 1 diff --git a/drivers/usb/dwc2/debugfs.c b/drivers/usb/dwc2/debugfs.c index 7f62f4cdc265..8b27cf0b682b 100644 --- a/drivers/usb/dwc2/debugfs.c +++ b/drivers/usb/dwc2/debugfs.c @@ -695,6 +695,7 @@ static int params_show(struct seq_file *seq, void *v) print_param_hex(seq, p, ahbcfg); print_param(seq, p, uframe_sched); print_param(seq, p, external_id_pin_ctl); + print_param(seq, p, power_saving); print_param(seq, p, power_down); print_param(seq, p, lpm); print_param(seq, p, lpm_clock_gating); @@ -746,6 +747,7 @@ static int hw_params_show(struct seq_file *seq, void *v) print_param(seq, hw, num_dev_perio_in_ep); print_param(seq, hw, total_fifo_size); print_param(seq, hw, power_optimized); + print_param(seq, hw, hibernation); print_param(seq, hw, utmi_phy_data_width); print_param_hex(seq, hw, snpsid); print_param_hex(seq, hw, dev_ep_dirs); diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c index 24ff5f21cb25..d7cc336aa1b7 100644 --- a/drivers/usb/dwc2/params.c +++ b/drivers/usb/dwc2/params.c @@ -263,12 +263,18 @@ static void dwc2_set_param_power_down(struct dwc2_hsotg *hsotg) { int val; - if (hsotg->hw_params.hibernation) - val = 2; - else if (hsotg->hw_params.power_optimized) - val = 1; - else - val = 0; + if (!hsotg->params.power_saving) { + val = DWC2_POWER_DOWN_PARAM_NONE; + dev_dbg(hsotg->dev, "%s: Power saving is disabled.\n", + __func__); + } else { + if (hsotg->hw_params.hibernation) + val = DWC2_POWER_DOWN_PARAM_HIBERNATION; + else if (hsotg->hw_params.power_optimized) + val = DWC2_POWER_DOWN_PARAM_PARTIAL; + else + val = DWC2_POWER_DOWN_PARAM_NONE; + } hsotg->params.power_down = val; } @@ -290,6 +296,7 @@ static void dwc2_set_default_params(struct dwc2_hsotg *hsotg) dwc2_set_param_phy_type(hsotg); dwc2_set_param_speed(hsotg); dwc2_set_param_phy_utmi_width(hsotg); + p->power_saving = true; dwc2_set_param_power_down(hsotg); p->phy_ulpi_ddr = false; p->phy_ulpi_ext_vbus = false;