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