On Thu, Jun 2, 2011 at 18:45, Kevin Hilman <khilman@xxxxxx> wrote:
> Nishanth Menon <nm@xxxxxx> 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@xxxxxx>
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
