Re: [PATCH RFC 01/27] PM / Domains: core changes for multiple states
[...] >>> +static int genpd_alloc_states_data(struct generic_pm_domain *genpd, >> >> In this patch I would rather just add *one* new "alloc" function and >> name it like below. >> >> static int genpd_alloc_default_state(struct generic_pm_domain *genpd) >> >> I assume the name I suggest for it, indicates what it needs to do and >> *not* needs to do. > > Hi Ulf, > > Thanks for your thorough review! > i will implement your suggestions for the next spin, Great, thanks! > > However I have a doubt about this comment, do you mean that i should get rid > of > genpd_alloc_states_data completly? i had a static number of states before Yes, in this patch - but we still want to alloc the data on the heap. Just alloc a state struct with size of one array. > but that was droped because o wasted memory if no states were needed. There will always be at least *one* state, so we shouldn't waste any memory following my suggestion above. > > if you just meant that it should be added in a separate patch, since i > will be sqashing I think you need to go through review comments for each patch separately. In the end that might very well mean that you should completely drop some patches, rework some and even create some some new. > this patch with "PM / Domains: make governor select deepest state" can > i keep it here? maybe it should go with the dt parsing patch from Marc? I don't think so. Try to keep each logical change in a separate patch. That makes it easier to review and thus also to accept patches. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 01/27] PM / Domains: core changes for multiple states
On Wed, Dec 9, 2015 at 2:58 PM, Ulf Hansson wrote:
> On 17 November 2015 at 23:37, Lina Iyer wrote:
>> From: Axel Haslam
>>
>> From: Axel Haslam
>>
>> Add the core changes to be able to declare multiple states.
>> When trying to set a power domain to off, genpd will be able to choose
>> from an array of states declared by the platform. The power on and off
>> latencies are now tied to a state.
>
> I would like to get an answer to *why* we need this.
>
> For example, we should mention that some HWs supports these multiple
> states - at least.
>
>>
>> States should be declared in ascending order from shallowest to deepest,
>
> I guess *should* isn't good enough. Perhaps *shall* is better?
>
>> deepest meaning the state which takes longer to enter and exit.
>>
>> the power_off and power_on function can use the 'state_idx' field of the
>> generic_pm_domain structure, to distinguish between the different states
>> and act accordingly.
>
> This needs some more explanation.
>
> First, please use the wording of "callbacks", like ->power_on() to
> better describe what function you are talking about.
> Second, what is "state_idx"? What's does it really tell the SOC PM
> domain driver here?
>
>>
>> Example:
>>
>> static int pd1_power_on(struct generic_pm_domain *domain)
>> {
>> /* domain->state_idx = state the domain is coming from */
>> }
>>
>> static int pd1_power_off(struct generic_pm_domain *domain)
>> {
>> /* domain->state_idx = desired powered off state */
>> }
>>
>> const struct genpd_power_state pd_states[] = {
>> {
>> .name = "RET",
>> .power_on_latency_ns = ON_LATENCY_FAST,
>> .power_off_latency_ns = OFF_LATENCY_FAST,
>> },
>> {
>> .name = "DEEP_RET",
>> .power_on_latency_ns = ON_LATENCY_MED,
>> .power_off_latency_ns = OFF_LATENCY_MED,
>> },
>> {
>> .name = "OFF",
>> .power_on_latency_ns = ON_LATENCY_SLOW,
>> .power_off_latency_ns = OFF_LATENCY_SLOW,
>> }
>> };
>>
>> struct generic_pm_domain pd1 = {
>> .name = "PD1",
>> .power_on = pd1_power_on,
>> .power_off = pd1_power_off,
>> [...]
>> };
>>
>> int xxx_init_pm_domain(){
>>
>> pd1->state = copy_of(pd_states);
>> pd1->state_count = ARRAY_SIZE(pd_states);
>> pm_genpd_init(pd1, true);
>>
>> }
>
> Well, even if the above describes this quite good, I think it better
> belongs in the Documentation rather than in the change log.
>
> Can we try to the improve the text in the change-log instead?
>
>>
>> Signed-off-by: Axel Haslam
>> Signed-off-by: Lina Iyer
>> [Lina: Modified genpd state initialization and remove use of
>> save_state_latency_ns in genpd timing data]
>>
>> Signed-off-by: Lina Iyer
>> ---
>> drivers/base/power/domain.c | 136
>> +--
>> drivers/base/power/domain_governor.c | 13 +++-
>> include/linux/pm_domain.h| 10 +++
>> 3 files changed, 151 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index e03b1ad..3242854 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -34,6 +34,12 @@
>> __ret; \
>> })
>>
>> +#define GENPD_MAX_NAME_SIZE 20
>> +
>> +static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
>
> Please avoid forward declarations.
>
> Also, we have just got rid of all *name* based genpd API/functions. I
> don't want us to start adding another.
>
> Or perhaps it's just the name of the function that I don't like!? :-)
>
>> + const struct genpd_power_state *st,
>> + unsigned int st_count);
>> +
>> static LIST_HEAD(gpd_list);
>> static DEFINE_MUTEX(gpd_list_lock);
>>
>> @@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct
>> generic_pm_domain *genpd)
>>
>> static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>> {
>> + unsigned int state_idx = genpd->state_idx;
>> ktime_t time_start;
>> s64 elapsed_ns;
>> int ret;
>> @@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain
>> *genpd, bool timed)
>> return ret;
>>
>> elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
>> - if (elapsed_ns <= genpd->power_on_latency_ns)
>> + if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
>> return ret;
>>
>> - genpd->power_on_latency_ns = elapsed_ns;
>> + genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
>> genpd->max_off_time_changed = true;
>> pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>> genpd->name, "on", elapsed_ns);
>> @@ -131,6 +138,7 @@ static int genpd_power_on(struct gene
Re: [PATCH RFC 01/27] PM / Domains: core changes for multiple states
On 17 November 2015 at 23:37, Lina Iyer wrote:
> From: Axel Haslam
>
> From: Axel Haslam
>
> Add the core changes to be able to declare multiple states.
> When trying to set a power domain to off, genpd will be able to choose
> from an array of states declared by the platform. The power on and off
> latencies are now tied to a state.
I would like to get an answer to *why* we need this.
For example, we should mention that some HWs supports these multiple
states - at least.
>
> States should be declared in ascending order from shallowest to deepest,
I guess *should* isn't good enough. Perhaps *shall* is better?
> deepest meaning the state which takes longer to enter and exit.
>
> the power_off and power_on function can use the 'state_idx' field of the
> generic_pm_domain structure, to distinguish between the different states
> and act accordingly.
This needs some more explanation.
First, please use the wording of "callbacks", like ->power_on() to
better describe what function you are talking about.
Second, what is "state_idx"? What's does it really tell the SOC PM
domain driver here?
>
> Example:
>
> static int pd1_power_on(struct generic_pm_domain *domain)
> {
> /* domain->state_idx = state the domain is coming from */
> }
>
> static int pd1_power_off(struct generic_pm_domain *domain)
> {
> /* domain->state_idx = desired powered off state */
> }
>
> const struct genpd_power_state pd_states[] = {
> {
> .name = "RET",
> .power_on_latency_ns = ON_LATENCY_FAST,
> .power_off_latency_ns = OFF_LATENCY_FAST,
> },
> {
> .name = "DEEP_RET",
> .power_on_latency_ns = ON_LATENCY_MED,
> .power_off_latency_ns = OFF_LATENCY_MED,
> },
> {
> .name = "OFF",
> .power_on_latency_ns = ON_LATENCY_SLOW,
> .power_off_latency_ns = OFF_LATENCY_SLOW,
> }
> };
>
> struct generic_pm_domain pd1 = {
> .name = "PD1",
> .power_on = pd1_power_on,
> .power_off = pd1_power_off,
> [...]
> };
>
> int xxx_init_pm_domain(){
>
> pd1->state = copy_of(pd_states);
> pd1->state_count = ARRAY_SIZE(pd_states);
> pm_genpd_init(pd1, true);
>
> }
Well, even if the above describes this quite good, I think it better
belongs in the Documentation rather than in the change log.
Can we try to the improve the text in the change-log instead?
>
> Signed-off-by: Axel Haslam
> Signed-off-by: Lina Iyer
> [Lina: Modified genpd state initialization and remove use of
> save_state_latency_ns in genpd timing data]
>
> Signed-off-by: Lina Iyer
> ---
> drivers/base/power/domain.c | 136
> +--
> drivers/base/power/domain_governor.c | 13 +++-
> include/linux/pm_domain.h| 10 +++
> 3 files changed, 151 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e03b1ad..3242854 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -34,6 +34,12 @@
> __ret; \
> })
>
> +#define GENPD_MAX_NAME_SIZE 20
> +
> +static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
Please avoid forward declarations.
Also, we have just got rid of all *name* based genpd API/functions. I
don't want us to start adding another.
Or perhaps it's just the name of the function that I don't like!? :-)
> + const struct genpd_power_state *st,
> + unsigned int st_count);
> +
> static LIST_HEAD(gpd_list);
> static DEFINE_MUTEX(gpd_list_lock);
>
> @@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain
> *genpd)
>
> static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> {
> + unsigned int state_idx = genpd->state_idx;
> ktime_t time_start;
> s64 elapsed_ns;
> int ret;
> @@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain
> *genpd, bool timed)
> return ret;
>
> elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> - if (elapsed_ns <= genpd->power_on_latency_ns)
> + if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
> return ret;
>
> - genpd->power_on_latency_ns = elapsed_ns;
> + genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
> genpd->max_off_time_changed = true;
> pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
> genpd->name, "on", elapsed_ns);
> @@ -131,6 +138,7 @@ static int genpd_power_on(struct generic_pm_domain
> *genpd, bool timed)
>
> static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
> {
> + unsigned int state_idx = genpd->state_idx;
> ktime_t time_start;
> s64
[PATCH RFC 01/27] PM / Domains: core changes for multiple states
From: Axel Haslam
From: Axel Haslam
Add the core changes to be able to declare multiple states.
When trying to set a power domain to off, genpd will be able to choose
from an array of states declared by the platform. The power on and off
latencies are now tied to a state.
States should be declared in ascending order from shallowest to deepest,
deepest meaning the state which takes longer to enter and exit.
the power_off and power_on function can use the 'state_idx' field of the
generic_pm_domain structure, to distinguish between the different states
and act accordingly.
Example:
static int pd1_power_on(struct generic_pm_domain *domain)
{
/* domain->state_idx = state the domain is coming from */
}
static int pd1_power_off(struct generic_pm_domain *domain)
{
/* domain->state_idx = desired powered off state */
}
const struct genpd_power_state pd_states[] = {
{
.name = "RET",
.power_on_latency_ns = ON_LATENCY_FAST,
.power_off_latency_ns = OFF_LATENCY_FAST,
},
{
.name = "DEEP_RET",
.power_on_latency_ns = ON_LATENCY_MED,
.power_off_latency_ns = OFF_LATENCY_MED,
},
{
.name = "OFF",
.power_on_latency_ns = ON_LATENCY_SLOW,
.power_off_latency_ns = OFF_LATENCY_SLOW,
}
};
struct generic_pm_domain pd1 = {
.name = "PD1",
.power_on = pd1_power_on,
.power_off = pd1_power_off,
[...]
};
int xxx_init_pm_domain(){
pd1->state = copy_of(pd_states);
pd1->state_count = ARRAY_SIZE(pd_states);
pm_genpd_init(pd1, true);
}
Signed-off-by: Axel Haslam
Signed-off-by: Lina Iyer
[Lina: Modified genpd state initialization and remove use of
save_state_latency_ns in genpd timing data]
Signed-off-by: Lina Iyer
---
drivers/base/power/domain.c | 136 +--
drivers/base/power/domain_governor.c | 13 +++-
include/linux/pm_domain.h| 10 +++
3 files changed, 151 insertions(+), 8 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e03b1ad..3242854 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -34,6 +34,12 @@
__ret; \
})
+#define GENPD_MAX_NAME_SIZE 20
+
+static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+ const struct genpd_power_state *st,
+ unsigned int st_count);
+
static LIST_HEAD(gpd_list);
static DEFINE_MUTEX(gpd_list_lock);
@@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain
*genpd)
static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
+ unsigned int state_idx = genpd->state_idx;
ktime_t time_start;
s64 elapsed_ns;
int ret;
@@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain
*genpd, bool timed)
return ret;
elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
- if (elapsed_ns <= genpd->power_on_latency_ns)
+ if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
return ret;
- genpd->power_on_latency_ns = elapsed_ns;
+ genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
genpd->max_off_time_changed = true;
pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
genpd->name, "on", elapsed_ns);
@@ -131,6 +138,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd,
bool timed)
static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
{
+ unsigned int state_idx = genpd->state_idx;
ktime_t time_start;
s64 elapsed_ns;
int ret;
@@ -147,10 +155,10 @@ static int genpd_power_off(struct generic_pm_domain
*genpd, bool timed)
return ret;
elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
- if (elapsed_ns <= genpd->power_off_latency_ns)
+ if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
return ret;
- genpd->power_off_latency_ns = elapsed_ns;
+ genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
genpd->max_off_time_changed = true;
pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
genpd->name, "off", elapsed_ns);
@@ -582,6 +590,8 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain
*genpd,
|| atomic_read(&genpd->sd_count) > 0)
return;
+ /* Choose the deepest state when suspending */
+ genpd->state_idx = genpd->state_count - 1;
genpd_power_off(genpd, timed);
genpd->status = GPD_STATE_POWER_OFF;
@@ -1205,6 +1215,61 @@ static void genpd_free_dev_data(struct device *dev,
dev_pm_put_subsys_data(dev
