Re: [PATCH v1 2/2] rockchip: power-domain: support qos save and restore

[Elaine Zhang <zhangqing@rock-chips.com>]

hi, Heiko:

Thanks for your replay.
For your questions, I also have the same concerns.

On 04/02/2016 12:19 AM, Heiko Stuebner wrote:
> Hi Elaine,
>
> Am Freitag, 1. April 2016, 10:33:45 schrieb Elaine Zhang:
>> I agree with most of your modifications.
>> Except, the u32 *qos_save_regs below
>
> you're right. I didn't take that into account when my open-coding my idea.
> A bit more below:
>
>> On 04/01/2016 12:31 AM, Heiko Stuebner wrote:
>>> Hi Elaine,
>>>
>>> Am Freitag, 18. März 2016, 15:17:24 schrieb Elaine Zhang:
>>>> support qos save and restore when power domain on/off.
>>>>
>>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>>>
>>> overall looks nice already ... some implementation-specific comments
>>> below.>
>>>> ---
>>>>
>>>>    drivers/soc/rockchip/pm_domains.c | 87
>>>>
>>>> +++++++++++++++++++++++++++++++++++++-- 1 file changed, 84
>>>> insertions(+),
>>>> 3 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/rockchip/pm_domains.c
>>>> b/drivers/soc/rockchip/pm_domains.c index 18aee6b..c5f4be6 100644
>>>> --- a/drivers/soc/rockchip/pm_domains.c
>>>> +++ b/drivers/soc/rockchip/pm_domains.c
>>>> @@ -45,10 +45,21 @@ struct rockchip_pmu_info {
>>>>
>>>>    	const struct rockchip_domain_info *domain_info;
>>>>
>>>>    };
>>>>
>>>> +#define MAX_QOS_NODE_NUM	20
>>>> +#define MAX_QOS_REGS_NUM	5
>>>> +#define QOS_PRIORITY		0x08
>>>> +#define QOS_MODE		0x0c
>>>> +#define QOS_BANDWIDTH		0x10
>>>> +#define QOS_SATURATION		0x14
>>>> +#define QOS_EXTCONTROL		0x18
>>>> +
>>>>
>>>>    struct rockchip_pm_domain {
>>>>
>>>>    	struct generic_pm_domain genpd;
>>>>    	const struct rockchip_domain_info *info;
>>>>    	struct rockchip_pmu *pmu;
>>>>
>>>> +	int num_qos;
>>>> +	struct regmap *qos_regmap[MAX_QOS_NODE_NUM];
>>>> +	u32 qos_save_regs[MAX_QOS_NODE_NUM][MAX_QOS_REGS_NUM];
>>>
>>> struct regmap **qos_regmap;
>>> u32 *qos_save_regs;
>>
>> when we save and restore qos registers we need save five regs for every
>> qos. like this :
>> for (i = 0; i < pd->num_qos; i++) {
>> 		regmap_read(pd->qos_regmap[i],
>> 			    QOS_PRIORITY,
>> 			    &pd->qos_save_regs[i][0]);
>> 		regmap_read(pd->qos_regmap[i],
>> 			    QOS_MODE,
>> 			    &pd->qos_save_regs[i][1]);
>> 		regmap_read(pd->qos_regmap[i],
>> 			    QOS_BANDWIDTH,
>> 			    &pd->qos_save_regs[i][2]);
>> 		regmap_read(pd->qos_regmap[i],
>> 			    QOS_SATURATION,
>> 			    &pd->qos_save_regs[i][3]);
>> 		regmap_read(pd->qos_regmap[i],
>> 			    QOS_EXTCONTROL,
>> 			    &pd->qos_save_regs[i][4]);
>> 	}
>> so we can not define qos_save_regs like u32 *qos_save_regs;,
>> and apply buff like
>> pd->qos_save_regs = kcalloc(pd->num_qos * MAX_QOS_REGS_NUM, sizeof(u32),
>> GFP_KERNEL);
>
> so how about simply swapping indices and doing it like
>
> u32 *qos_save_regs[MAX_QOS_REGS_NUM];
>
> for (i = 0; i < MAX_QOS_REGS_NUM; i++) {
> 	qos_save_regs[i] = kcalloc(pd->num_qos, sizeof(u32));
> 	/* error handling here */
> }
>
> ...
> 		regmap_read(pd->qos_regmap[i],
> 			    QOS_SATURATION,
> 			    &pd->qos_save_regs[3][i]);
> ...

I agree with you on this modification.

>
>
> Asked the other way around, how did you measure to set MAX_QOS_REGS_NUM to
> 20? From looking at the rk3399 TRM, it seems there are only 38 QoS
> generators on the SoC in general (24 on the rk3288 with PD_VIO having a
> maximum of 9 qos generators), so preparing for 20 seems a bit overkill ;-)
>
About the MAX_QOS_NODE_NUM I also have some uncertaibty.
Although there are only 38 QoS on the RK3399(24 on the rk3288),but not 
all of the pd need to power on/off.So not all QOS need save and restore.
So about the MAX_QOS_NODE_NUM, what do you suggest.

MAX_QOS_REGS_NUM is 5 because the QOS register is just 5 need save and 
restore.
like :
#define QOS_PRIORITY		0x08
#define QOS_MODE		0x0c
#define QOS_BANDWIDTH		0x10
#define QOS_SATURATION		0x14
#define QOS_EXTCONTROL		0x18
>
> Heiko
>
>
>