On Thu, Oct 06 2016 at 13:45 -0600, Ulf Hansson wrote:

On 6 October 2016 at 17:40, Lina Iyer [off-list ref] wrote:

quoted

On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:

quoted

On 5 October 2016 at 22:31, Lina Iyer [off-list ref] wrote:

quoted

Allow PM Domain states to be defined dynamically by the drivers. This
removes the limitation on the maximum number of states possible for a
domain.

Cc: Axel Haslam <redacted>
Suggested-by: Ulf Hansson <redacted>
Signed-off-by: Lina Iyer <redacted>

<...>

quoted

quoted

-#define GENPD_MAX_NUM_STATES   8 /* Number of possible low power states
*/
-
 enum gpd_status {
        GPD_STATE_ACTIVE = 0,   /* PM domain is active */
        GPD_STATE_POWER_OFF,    /* PM domain is off */

@@ -70,7 +68,7 @@ struct generic_pm_domain {
        void (*detach_dev)(struct generic_pm_domain *domain,
                           struct device *dev);
        unsigned int flags;             /* Bit field of configs for genpd

*/
-       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
+       struct genpd_power_state *states;
        unsigned int state_count; /* number of states */
        unsigned int state_idx; /* state that genpd will go to when off
*/

--
2.7.4

In general I like the improvement, but..

This change implies that ->states may very well be NULL. This isn't
validated by genpd's internal logic when power off/on the domain
(genpd_power_on|off(), __default_power_down_ok()). You need to fix
this, somehow.

Good point.

quoted

Perhaps the easiest solutions is, when pm_genpd_init() finds that
->state is NULL, that we allocate a struct genpd_power_state with
array size of 1 and assign it to ->states. Although, doing this also
means you need to track that genpd was responsible for the the
allocation, so it must also free the data from within genpd_remove().

Unless you have other ideas!?

I can think of some hacks, but they are uglier than the problem we are
trying to solve. We could drop this patch. Real world situations would
not have more than 8 states and if there is one, we can think about it
then.

The problem with the current approach is that we waste some memory as
we always have an array of 8 states per genpd. In the worst case,
which currently is the most common case, only 1 out of 8 states is
being used.

So, let's not be lazy here and instead take the opportunity to fix
this, and especially I think this makes sense, before we go on and add
the DT parsing of the domain-idle-states.

Hmm.. We are not wasting much memory in comparison, but if you insist,
sure.

The more sophisticated method would probably be to use kobject/kref,
but let's not go there for now. Instead let's try an easy method of
just tracking whether the allocations had been made internally by
genpd, via adding a "bool state_allocated to the struct
generic_pm_domain. Would that work?

It would work.

i.
How about an additional static state by default in the domain structure,
if the platform does not provide a state then the default structure is
used. That way we dont have to track it. But it does waste memory
eqivalent to a state, when there are states provided by the platform.

ii.
I could add a void *free to the domain structure and save the memory
allocated by default in the *free. At domain remove, we just do a kfree
on free.

iii.
Use a boolean flag.

Thanks,
Lina