Re: [PM-WIP/voltdm_c][PATCH 06/11] OMAP4: PM: VC: fix channel bit offset for MPU

["Menon, Nishanth" <nm@ti.com>]

On Wed, May 18, 2011 at 04:34, Kevin Hilman <khilman@ti.com> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> Patch "OMAP3+: VC: abstract out channel configuration" abstracts out
>> VC channel configuration. However, TRM has it's little surprises such
>> as the following for channel_cfg:
>> CFG_CHANNEL_SA    BIT(0)
>> CFG_CHANNEL_RAV   BIT(1)
>> CFG_CHANNEL_RAC   BIT(2)
>> CFG_CHANNEL_RACEN BIT(3)
>> CFG_CHANNEL_CMD   BIT(4)
>> is valid for core and iva, *but* for mpu, the channel offsets
>> are as follows:
>> CFG_CHANNEL_SA    BIT(0)
>> CFG_CHANNEL_CMD   BIT(1)
>> CFG_CHANNEL_RAV   BIT(2)
>> CFG_CHANNEL_RAC   BIT(3)
>> CFG_CHANNEL_RACEN BIT(4)
>
> sigh, glad to see the IP designers have something to do. :(
/me thinks "lets say 'integration folks'.." ;)

>
> Hmm, and I don't suppose there's any chance we can tell the difference
> based on IP revision information from the IP?  After looking at the TRM
> and the functional spec, there's a difference.  In the funcional spec,
> all 3 OMAP4 VCs have the "default" order, but in the TRM, the MPU is
> different.  Which one is right?   I assume you found this because of a
> bug in testing, so I take it the TRM is right.
Unfortunately, that is what I have been burnt previously upon as well
- in all cases, I have been told, "in case of conflict funcspec Vs
TRM, believe the TRM" as there is integration aspects of an IP for an
SoC involved :(. Unfortunately in this case, MPU caused core to
transition until fixed.

>
>> to handle this on the fly, add a structure to describe this
>> and use the structure for vc44xx mpu definition. use the
>> default for rest of the domains.
>
> IMO, while it makes us generate a few more struct in the data, I think
> it's cleaner to not treat this as an exception.  IOW, just define
> the channel struct(s) in each data file, so every VC has one associated
> with it.  Probably also need a WARN() and graceful failure during init
> if a channel doesn't have a channel config defined.
..

>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>>  arch/arm/mach-omap2/vc.c          |   35 +++++++++++++++++++----------------
>>  arch/arm/mach-omap2/vc.h          |   33 +++++++++++++++++++++++++++++++++
>>  arch/arm/mach-omap2/vc44xx_data.c |   10 ++++++++++
>>  3 files changed, 62 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
>> index f8185d2..2add945 100644
>> --- a/arch/arm/mach-omap2/vc.c
>> +++ b/arch/arm/mach-omap2/vc.c
[..]
>>
>> +     /* if there is an exception case, use the exception data */
>> +     if (!vc->cfg_ch_data)
>> +             cfg_channel_data = &cfg_channel_common;
>> +     else
>> +             cfg_channel_data = vc->cfg_ch_data;
>
> Based on the above,  this block could be dropped, and...
Except I will have to replicate this for OMAP3 and 4 - which is
possible.. but replicated data which is needed only during init
:( did not want vc pointer to contain __initdata pointer (dangling
ones after boot)

>
>>       vc->cfg_channel = 0;
>>
>>       /* get PMIC/board specific settings */
>> @@ -284,7 +286,7 @@ void __init omap_vc_init_channel(struct voltagedomain *voltdm)
>>       voltdm->rmw(vc->smps_sa_mask,
>>                   vc->i2c_slave_addr << __ffs(vc->smps_sa_mask),
>>                   vc->common->smps_sa_reg);
>> -     vc->cfg_channel |= CFG_CHANNEL_SA;
>> +     vc->cfg_channel |= cfg_channel_data->cfg_channel_sa;
>
> ...this would look like
>
>        vc->cfg_channel |= vc->cfg_ch_bits->sa;
>
> and similar for below.
ok..
[...]

>> diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
>> index f0fb61f..bbd8f1f 100644
>> --- a/arch/arm/mach-omap2/vc.h
>> +++ b/arch/arm/mach-omap2/vc.h
>> @@ -62,11 +62,42 @@ struct omap_vc_common {
>>       u8 i2c_mcode_mask;
>>  };
>>
>> +/*
>> + * Channel configuration bits, common for OMAP3 & 4
>> + * OMAP3 register: PRM_VC_CH_CONF
>> + * OMAP4 register: PRM_VC_CFG_CHANNEL
>> + */
>> +#define CFG_CHANNEL_SA    BIT(0)
>> +#define CFG_CHANNEL_RAV   BIT(1)
>> +#define CFG_CHANNEL_RAC   BIT(2)
>> +#define CFG_CHANNEL_RACEN BIT(3)
>> +#define CFG_CHANNEL_CMD   BIT(4)
>> +#define CFG_CHANNEL_MASK 0x3f
>> +/**
>> + * struct omap_vc_channel_cfg - describe the cfg_channel bitfield
>> + * @cfg_channel_sa:  SA_VDD_xxx_L
>> + * @cfg_channel_rav: RAV_VDD_xxx_L
>> + * @cfg_channel_rac: RAC_VDD_xxx_L
>> + * @cfg_channel_racen:       RACEN_VDD_xxx_L
>> + * @cfg_channel_cmd: CMD_VDD_xxx_L
>
> You should drop the cfg_channel_ prefix here (and below.)
ok..

>
>> + *
>> + * by default it uses CFG_CHANNEL_xyz regs, but in OMAP4 MPU,
>> + * there is a need for an exception case.
>
> This comment isn't really needed here.  IOW, let's assume it's not an
> exception, as new SoCs might decide to change again.
true.

[..]
>>   */
>>  struct omap_vc_channel {
>>       /* channel state */
>> @@ -74,6 +105,7 @@ struct omap_vc_channel {
>>       u8 volt_reg_addr;
>>       u8 cmd_reg_addr;
>>       u8 cfg_channel;
>> +     struct omap_vc_channel_cfg *cfg_ch_data;
>
> s/cfg_ch_data/cfg_ch_bits/
ok.

>
>>       u16 setup_time;
>>       bool i2c_high_speed;
>>
>> @@ -86,6 +118,7 @@ struct omap_vc_channel {
>>       u8 cfg_channel_sa_shift;
>>  };
>>
>> +
>
> stray whitespace change
oops :( will fix in rev2

[...]


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