diff mbox

[pm_wip/voltdm_nm,v2,2/3] OMAP4: PM: VC: introduce concept of default channel

Message ID 87pqmvn6gk.fsf@ti.com (mailing list archive)
State Not Applicable
Delegated to: Kevin Hilman
Headers show

Commit Message

Kevin Hilman June 2, 2011, 11:45 p.m. UTC 
Nishanth Menon <nm@ti.com> writes:

> Due to a TRM bug, the current code assumes that channel0(core) is the default
> channel. With the additional explanation provided by the hardware team, it is
> clear that PRM_VC_CFG_CHANNEL register allows for flexibility of configuring
> for various PMIC configurations. For example, the I2C slave address on TWL6030
> when using 4430 configuration could potentially use the same slave address for
> all domains, while in 4460 configuration, we drive MPU using TPS; Core and IVA
> using TWL6030. To allow this flexibility, we state in existing parameters using
> a flag to indicate usage of default channel.
>
> In addition, the TRM update clears up the confusion on the fact that MPU is
> infact the default channel on OMAP4.
>
> We update the same here.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>

There's too much going on in this patch not described in the changelog
(extra error checking, volt & cmd reg checking etc.)

Also, I don't like the USE_DEFAULT_CHANNEL_I2C_PARAM, mainly because it
adds clutter without any obvious value.  To me, zero is a perfectly good
value to signify "use default channel value", especially since that's
the value used by the hardware.

Incidentally, adding this doesn't actually change current behavior.
Currently, I use zero (an unconfigured value in the VC settings) to
signify it should use default settings.  In your patch, you defined
USE_DEFAULT_CHANNEL_I2C_PARAM as 0x10000 (17 bits) but assign it to 16
bit fields, which means they are just set to zero. :)

So rather than take this patch, I'm just going to fold the following
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Nishanth Menon June 2, 2011, 11:53 p.m. UTC | #1 
On Thu, Jun 2, 2011 at 18:45, Kevin Hilman <khilman@ti.com> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> Due to a TRM bug, the current code assumes that channel0(core) is the default
>> channel. With the additional explanation provided by the hardware team, it is
>> clear that PRM_VC_CFG_CHANNEL register allows for flexibility of configuring
>> for various PMIC configurations. For example, the I2C slave address on TWL6030
>> when using 4430 configuration could potentially use the same slave address for
>> all domains, while in 4460 configuration, we drive MPU using TPS; Core and IVA
>> using TWL6030. To allow this flexibility, we state in existing parameters using
>> a flag to indicate usage of default channel.
>>
>> In addition, the TRM update clears up the confusion on the fact that MPU is
>> infact the default channel on OMAP4.
>>
>> We update the same here.
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>
> There's too much going on in this patch not described in the changelog
> (extra error checking, volt & cmd reg checking etc.)
>
> Also, I don't like the USE_DEFAULT_CHANNEL_I2C_PARAM, mainly because it
> adds clutter without any obvious value.  To me, zero is a perfectly good
> value to signify "use default channel value", especially since that's
> the value used by the hardware.
The reason is that 0 is a valid i2c register address. 8 bits is used
in VC and I wanted one bit which was further away, hence the 16 bit.
The usage could be that in .volt_reg_addr or in .cmd_reg_addr I could
set this up with the macro. and bingo, we use the default domain's
configuration.

This is esp useful if we had a single pmic reg for all domains.. I
mean the VC design allows for it, even though we dont use it
currently.
>
> Incidentally, adding this doesn't actually change current behavior.
> Currently, I use zero (an unconfigured value in the VC settings) to
> signify it should use default settings.  In your patch, you defined
> USE_DEFAULT_CHANNEL_I2C_PARAM as 0x10000 (17 bits) but assign it to 16
> bit fields, which means they are just set to zero. :)


> So rather than take this patch, I'm just going to fold the following
> diff into "OMAP3+: VC: abstract out channel configuration" in voltdm_b.
> I'll also update the changelog noting that the TRM is wrong here in that
> it describes CORE as the default channel when it's in fact MPU.

I intend to post a new series including my PMIC changes as well early
next week(mondayish hopefully). Can we hold off review of any further
of my voltage fixes patches till then?

> Kevin
>
> diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
> index fba352d..fa48944 100644
> --- a/arch/arm/mach-omap2/vc.c
> +++ b/arch/arm/mach-omap2/vc.c
> @@ -33,21 +33,20 @@
>  * - command configuration address (RAC) and enable bit (RACEN)
>  * - command values for ON, ONLP, RET and OFF (CMD)
>  *
> - * This function currently only allows flexible configuration of
> - * the non-default channel (e.g. non-zero channels.)  Starting with
> - * OMAP4, only the non-zero channels can be configured.  Channel zero
> - * always uses the channel zero register values.  Therefore, the
> - * same limitation is imposed on OMAP3 for consistency.
> + * This function currently only allows flexible configuration of the
> + * non-default channel.  Starting with OMAP4, there are more than 2
> + * channels, with one defined as the default (on OMAP4, it's MPU.)
> + * Only the non-default channel can be configured.
>  */
>  static int omap_vc_config_channel(struct voltagedomain *voltdm)
>  {
>        struct omap_vc_channel *vc = voltdm->vc;
>
>        /*
> -        * For channel zero, the only configurable bit is RACEN.
> +        * For default channel, the only configurable bit is RACEN.
>         * All others must stay at zero (see function comment above.)
>         */
> -       if (!vc->cfg_channel_sa_shift)
> +       if (vc->is_default_channel)
>                vc->cfg_channel &= CFG_CHANNEL_RACEN;
>
>        voltdm->rmw(CFG_CHANNEL_MASK << vc->cfg_channel_sa_shift,
> diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
> index f0fb61f..c216b57 100644
> --- a/arch/arm/mach-omap2/vc.h
> +++ b/arch/arm/mach-omap2/vc.h
> @@ -84,6 +84,8 @@ struct omap_vc_channel {
>        u32 smps_cmdra_mask;
>        u8 cmdval_reg;
>        u8 cfg_channel_sa_shift;
> +
> +       bool is_default_channel;
>  };
>
>  extern struct omap_vc_channel omap3_vc_mpu;
> diff --git a/arch/arm/mach-omap2/vc44xx_data.c b/arch/arm/mach-omap2/vc44xx_data.c
> index fe4f4e5..5844b20 100644
> --- a/arch/arm/mach-omap2/vc44xx_data.c
> +++ b/arch/arm/mach-omap2/vc44xx_data.c
> @@ -58,6 +58,7 @@ struct omap_vc_channel omap4_vc_mpu = {
>        .smps_volra_mask = OMAP4430_VOLRA_VDD_MPU_L_MASK,
>        .smps_cmdra_mask = OMAP4430_CMDRA_VDD_MPU_L_MASK,
>        .cfg_channel_sa_shift = OMAP4430_SA_VDD_MPU_L_SHIFT,
> +       .is_default_channel = true,
>  };
>
>  struct omap_vc_channel omap4_vc_iva = {
>


Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman June 3, 2011, 12:10 a.m. UTC | #2 
"Menon, Nishanth" <nm@ti.com> writes:

> On Thu, Jun 2, 2011 at 18:45, Kevin Hilman <khilman@ti.com> wrote:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> Due to a TRM bug, the current code assumes that channel0(core) is the default
>>> channel. With the additional explanation provided by the hardware team, it is
>>> clear that PRM_VC_CFG_CHANNEL register allows for flexibility of configuring
>>> for various PMIC configurations. For example, the I2C slave address on TWL6030
>>> when using 4430 configuration could potentially use the same slave address for
>>> all domains, while in 4460 configuration, we drive MPU using TPS; Core and IVA
>>> using TWL6030. To allow this flexibility, we state in existing parameters using
>>> a flag to indicate usage of default channel.
>>>
>>> In addition, the TRM update clears up the confusion on the fact that MPU is
>>> infact the default channel on OMAP4.
>>>
>>> We update the same here.
>>>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>
>> There's too much going on in this patch not described in the changelog
>> (extra error checking, volt & cmd reg checking etc.)
>>
>> Also, I don't like the USE_DEFAULT_CHANNEL_I2C_PARAM, mainly because it
>> adds clutter without any obvious value.  To me, zero is a perfectly good
>> value to signify "use default channel value", especially since that's
>> the value used by the hardware.
> The reason is that 0 is a valid i2c register address. 8 bits is used
> in VC and I wanted one bit which was further away, hence the 16 bit.
> The usage could be that in .volt_reg_addr or in .cmd_reg_addr I could
> set this up with the macro. and bingo, we use the default domain's
> configuration.
>
> This is esp useful if we had a single pmic reg for all domains.. I
> mean the VC design allows for it, even though we dont use it
> currently.

OK.

See, it's easy to convince me that something is needed/useful.  Of
course, I prefer to be conviced by the changelog. :)  

Please make this feature into a dedicated patch with an appropriately
descriptive changelog.  Thanks.

>>
>> Incidentally, adding this doesn't actually change current behavior.
>> Currently, I use zero (an unconfigured value in the VC settings) to
>> signify it should use default settings.  In your patch, you defined
>> USE_DEFAULT_CHANNEL_I2C_PARAM as 0x10000 (17 bits) but assign it to 16
>> bit fields, which means they are just set to zero. :)
>
>
>> So rather than take this patch, I'm just going to fold the following
>> diff into "OMAP3+: VC: abstract out channel configuration" in voltdm_b.
>> I'll also update the changelog noting that the TRM is wrong here in that
>> it describes CORE as the default channel when it's in fact MPU.
>
> I intend to post a new series including my PMIC changes as well early
> next week(mondayish hopefully). Can we hold off review of any further
> of my voltage fixes patches till then?
>

Sure.  It's the first time anyone as asked me not to review patches. :)
I'll gladly comply.

I've already folded the minimal change I proposed into the original
patch locally, and will push updated voltdm_* branches by the end of
today.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon June 3, 2011, 12:15 a.m. UTC | #3 
On Thu, Jun 2, 2011 at 19:10, Kevin Hilman <khilman@ti.com> wrote:
> "Menon, Nishanth" <nm@ti.com> writes:
>
>> On Thu, Jun 2, 2011 at 18:45, Kevin Hilman <khilman@ti.com> wrote:
>>> Nishanth Menon <nm@ti.com> writes:
>>>
>>>> Due to a TRM bug, the current code assumes that channel0(core) is the default
>>>> channel. With the additional explanation provided by the hardware team, it is
>>>> clear that PRM_VC_CFG_CHANNEL register allows for flexibility of configuring
>>>> for various PMIC configurations. For example, the I2C slave address on TWL6030
>>>> when using 4430 configuration could potentially use the same slave address for
>>>> all domains, while in 4460 configuration, we drive MPU using TPS; Core and IVA
>>>> using TWL6030. To allow this flexibility, we state in existing parameters using
>>>> a flag to indicate usage of default channel.
>>>>
>>>> In addition, the TRM update clears up the confusion on the fact that MPU is
>>>> infact the default channel on OMAP4.
>>>>
>>>> We update the same here.
>>>>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>
>>> There's too much going on in this patch not described in the changelog
>>> (extra error checking, volt & cmd reg checking etc.)
>>>
>>> Also, I don't like the USE_DEFAULT_CHANNEL_I2C_PARAM, mainly because it
>>> adds clutter without any obvious value.  To me, zero is a perfectly good
>>> value to signify "use default channel value", especially since that's
>>> the value used by the hardware.
>> The reason is that 0 is a valid i2c register address. 8 bits is used
>> in VC and I wanted one bit which was further away, hence the 16 bit.
>> The usage could be that in .volt_reg_addr or in .cmd_reg_addr I could
>> set this up with the macro. and bingo, we use the default domain's
>> configuration.
>>
>> This is esp useful if we had a single pmic reg for all domains.. I
>> mean the VC design allows for it, even though we dont use it
>> currently.
>
> OK.
>
> See, it's easy to convince me that something is needed/useful.  Of
> course, I prefer to be conviced by the changelog. :)
>
> Please make this feature into a dedicated patch with an appropriately
> descriptive changelog.  Thanks.

Yes, I agree. it probably is a better idea to break this thing up - I
guess I was a bit lazyish considering these were targetted to be
squashed.. not in the next series :)
>
>>>
>>> Incidentally, adding this doesn't actually change current behavior.
>>> Currently, I use zero (an unconfigured value in the VC settings) to
>>> signify it should use default settings.  In your patch, you defined
>>> USE_DEFAULT_CHANNEL_I2C_PARAM as 0x10000 (17 bits) but assign it to 16
>>> bit fields, which means they are just set to zero. :)
>>
>>
>>> So rather than take this patch, I'm just going to fold the following
>>> diff into "OMAP3+: VC: abstract out channel configuration" in voltdm_b.
>>> I'll also update the changelog noting that the TRM is wrong here in that
>>> it describes CORE as the default channel when it's in fact MPU.
>>
>> I intend to post a new series including my PMIC changes as well early
>> next week(mondayish hopefully). Can we hold off review of any further
>> of my voltage fixes patches till then?
>>
>
> Sure.  It's the first time anyone as asked me not to review patches. :)
> I'll gladly comply.
>
> I've already folded the minimal change I proposed into the original
> patch locally, and will push updated voltdm_* branches by the end of
> today.

:) Thanks. I will rework the patches and line up a series from cpufreq
and voltage domain perspective in the upcoming days..

Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman June 3, 2011, 5:27 p.m. UTC | #4 
Hi Nishanth,

Kevin Hilman <khilman@ti.com> writes:

[...]

>
> So rather than take this patch, I'm just going to fold the following
> diff into "OMAP3+: VC: abstract out channel configuration" in voltdm_b.
> I'll also update the changelog noting that the TRM is wrong here in that
> it describes CORE as the default channel when it's in fact MPU.
>

FYI...

I modified this slightly in the current voltdm_* branch in that I'm now
using a 'u8 flags' in the VC struct intead of a bool for this since  I expect
to have some other reasons to use various flags.

Kevin

>
> diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
> index fba352d..fa48944 100644
> --- a/arch/arm/mach-omap2/vc.c
> +++ b/arch/arm/mach-omap2/vc.c
> @@ -33,21 +33,20 @@
>   * - command configuration address (RAC) and enable bit (RACEN)
>   * - command values for ON, ONLP, RET and OFF (CMD)
>   *
> - * This function currently only allows flexible configuration of
> - * the non-default channel (e.g. non-zero channels.)  Starting with
> - * OMAP4, only the non-zero channels can be configured.  Channel zero
> - * always uses the channel zero register values.  Therefore, the
> - * same limitation is imposed on OMAP3 for consistency.
> + * This function currently only allows flexible configuration of the
> + * non-default channel.  Starting with OMAP4, there are more than 2
> + * channels, with one defined as the default (on OMAP4, it's MPU.)
> + * Only the non-default channel can be configured.
>   */
>  static int omap_vc_config_channel(struct voltagedomain *voltdm)
>  {
>  	struct omap_vc_channel *vc = voltdm->vc;
>  
>  	/*
> -	 * For channel zero, the only configurable bit is RACEN.
> +	 * For default channel, the only configurable bit is RACEN.
>  	 * All others must stay at zero (see function comment above.)
>  	 */
> -	if (!vc->cfg_channel_sa_shift)
> +	if (vc->is_default_channel)
>  		vc->cfg_channel &= CFG_CHANNEL_RACEN;
>  
>  	voltdm->rmw(CFG_CHANNEL_MASK << vc->cfg_channel_sa_shift,
> diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
> index f0fb61f..c216b57 100644
> --- a/arch/arm/mach-omap2/vc.h
> +++ b/arch/arm/mach-omap2/vc.h
> @@ -84,6 +84,8 @@ struct omap_vc_channel {
>  	u32 smps_cmdra_mask;
>  	u8 cmdval_reg;
>  	u8 cfg_channel_sa_shift;
> +
> +	bool is_default_channel;
>  };
>  
>  extern struct omap_vc_channel omap3_vc_mpu;
> diff --git a/arch/arm/mach-omap2/vc44xx_data.c b/arch/arm/mach-omap2/vc44xx_data.c
> index fe4f4e5..5844b20 100644
> --- a/arch/arm/mach-omap2/vc44xx_data.c
> +++ b/arch/arm/mach-omap2/vc44xx_data.c
> @@ -58,6 +58,7 @@ struct omap_vc_channel omap4_vc_mpu = {
>  	.smps_volra_mask = OMAP4430_VOLRA_VDD_MPU_L_MASK,
>  	.smps_cmdra_mask = OMAP4430_CMDRA_VDD_MPU_L_MASK,
>  	.cfg_channel_sa_shift = OMAP4430_SA_VDD_MPU_L_SHIFT,
> +	.is_default_channel = true,
>  };
>  
>  struct omap_vc_channel omap4_vc_iva = {
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff into "OMAP3+: VC: abstract out channel configuration" in voltdm_b.
I'll also update the changelog noting that the TRM is wrong here in that
it describes CORE as the default channel when it's in fact MPU.

Kevin

diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
index fba352d..fa48944 100644
--- a/arch/arm/mach-omap2/vc.c
+++ b/arch/arm/mach-omap2/vc.c
@@ -33,21 +33,20 @@ 
  * - command configuration address (RAC) and enable bit (RACEN)
  * - command values for ON, ONLP, RET and OFF (CMD)
  *
- * This function currently only allows flexible configuration of
- * the non-default channel (e.g. non-zero channels.)  Starting with
- * OMAP4, only the non-zero channels can be configured.  Channel zero
- * always uses the channel zero register values.  Therefore, the
- * same limitation is imposed on OMAP3 for consistency.
+ * This function currently only allows flexible configuration of the
+ * non-default channel.  Starting with OMAP4, there are more than 2
+ * channels, with one defined as the default (on OMAP4, it's MPU.)
+ * Only the non-default channel can be configured.
  */
 static int omap_vc_config_channel(struct voltagedomain *voltdm)
 {
 	struct omap_vc_channel *vc = voltdm->vc;
 
 	/*
-	 * For channel zero, the only configurable bit is RACEN.
+	 * For default channel, the only configurable bit is RACEN.
 	 * All others must stay at zero (see function comment above.)
 	 */
-	if (!vc->cfg_channel_sa_shift)
+	if (vc->is_default_channel)
 		vc->cfg_channel &= CFG_CHANNEL_RACEN;
 
 	voltdm->rmw(CFG_CHANNEL_MASK << vc->cfg_channel_sa_shift,
diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
index f0fb61f..c216b57 100644
--- a/arch/arm/mach-omap2/vc.h
+++ b/arch/arm/mach-omap2/vc.h
@@ -84,6 +84,8 @@  struct omap_vc_channel {
 	u32 smps_cmdra_mask;
 	u8 cmdval_reg;
 	u8 cfg_channel_sa_shift;
+
+	bool is_default_channel;
 };
 
 extern struct omap_vc_channel omap3_vc_mpu;
diff --git a/arch/arm/mach-omap2/vc44xx_data.c b/arch/arm/mach-omap2/vc44xx_data.c
index fe4f4e5..5844b20 100644
--- a/arch/arm/mach-omap2/vc44xx_data.c
+++ b/arch/arm/mach-omap2/vc44xx_data.c
@@ -58,6 +58,7 @@  struct omap_vc_channel omap4_vc_mpu = {
 	.smps_volra_mask = OMAP4430_VOLRA_VDD_MPU_L_MASK,
 	.smps_cmdra_mask = OMAP4430_CMDRA_VDD_MPU_L_MASK,
 	.cfg_channel_sa_shift = OMAP4430_SA_VDD_MPU_L_SHIFT,
+	.is_default_channel = true,
 };
 
 struct omap_vc_channel omap4_vc_iva = {