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

[Heiko Stuebner]

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@xxxxxxxxxxxxxx>
> > 
> > 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]);
...


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 ;-)


Heiko