Re: [PATCH v3 OMAP3 PM]: Remove IVA state conflict between PM and DspBridge code

[Kevin Hilman <khilman@deeprootsystems.com>]

shweta gulati <shweta.gulati@ti.com> writes:

> From: Shweta Gulati <shweta.gulati@ti.com>
>
> This version of patch incorporates review comments which includes
> shifting the code change in specific function 'omap3_iva_idle' and
> removing iva_pwrdm from pwrst_list rather than checking all the pwrdms
> in list to exclude iva_pwrdm. 

Comments about the version history of a patch don't belong here.  They
belong after the '---' so they are not picked up in the git history.

> The PM code should not set latency on IVA power state based on 
> the flag 'enable_off_mode'.

Why?  A changelog is about answering *both* 'what' and 'why'

Also the SRF change should be a separate patch since SRF is PM branch
only.  The primary change here should be targetted for mainline.

> This is taken care of in this version.   
>
> Signed-off-by: Sripathy Vishwanath <vishwanath.bs@ti.com>
> Signed-off-by: Shweta Gulati <shweta.gulati@ti.com>
> ---

I *strongly* recommend using git-format-patch, then manually editing
this part to add patch version history.

> Index: linux-omap-pm/arch/arm/mach-omap2/pm34xx.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/mach-omap2/pm34xx.c
> +++ linux-omap-pm/arch/arm/mach-omap2/pm34xx.c
> @@ -786,6 +786,12 @@ static void __init omap3_iva_idle(void)
>  			  OMAP3430_RST2_IVA2 |
>  			  OMAP3430_RST3_IVA2,
>  			  OMAP3430_IVA2_MOD, OMAP2_RM_RSTCTRL);
> +	/* Put the IVA2 In Idle */
> +	prm_rmw_mod_reg_bits(OMAP3430_LASTPOWERSTATEENTERED_MASK, 0,
> +				OMAP3430_IVA2_MOD, OMAP2_PM_PWSTCTRL);

huh?

this is confusing for multiple reasons

1. The comment is wrong.  You're setting the nex powerstate to OFF.

2. LASTPOWERSTATEENTERED is a field in PM_PREPWSTST, not in PM_PWSTCTRL,
   so the field your changing is the POWERSTATE field of PM_PWRSTCTRL.

3. setting this state is already done in pwrdms_setup()


> +	/* Make Clock transition Automatic*/

nit: need a space before the '*/'

> +	cm_rmw_mod_reg_bits(OMAP3430_CLKTRCTRL_IVA2_MASK, 0x3,
> +				OMAP3430_IVA2_MOD, OMAP2_CM_CLKSTCTRL);
>  }
>  
>  static void __init omap3_d2d_idle(void)
> @@ -1074,8 +1080,11 @@ static int __init pwrdms_setup(struct po
>  	if (!pwrst)
>  		return -ENOMEM;
>  	pwrst->pwrdm = pwrdm;
> -	pwrst->next_state = PWRDM_POWER_RET;
> -	list_add(&pwrst->node, &pwrst_list);
> +	if (strcmp("iva2_pwrdm", pwrdm->name)) {
> +		pwrst->next_state = PWRDM_POWER_RET;
> +		list_add(&pwrst->node, &pwrst_list);
> +	} else
> +		 pwrst->next_state = PWRDM_POWER_OFF;

See Documentation/CodingStyle about use of {}.  If one part
of an 'if' has {}s, the other(s) should as well.

>  	if (pwrdm_has_hdwr_sar(pwrdm))
>  		pwrdm_enable_hdwr_sar(pwrdm);
> Index: linux-omap-pm/arch/arm/mach-omap2/resource34xx.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/mach-omap2/resource34xx.c
> +++ linux-omap-pm/arch/arm/mach-omap2/resource34xx.c
> @@ -140,7 +140,8 @@ int set_pd_latency(struct shared_resourc
>  	}
>  
>  	if (!enable_off_mode && pd_lat_level == PD_LATENCY_OFF)
> -		pd_lat_level = PD_LATENCY_RET;
> +		if (strcmp("iva2_pwrdm", pwrdm->name))
> +			pd_lat_level = PD_LATENCY_RET;
>  
>  	resp->curr_level = pd_lat_level;
>  	set_pwrdm_state(pwrdm, pd_lat_level);

This change should be a separate patch including a changelog that
answers "why?"

Kevin