On Wed, 25 Apr 2012, Hiremath, Vaibhav wrote:
> On Wed, Apr 25, 2012 at 05:52:26, Paul Walmsley wrote:
> > A few questions while reviewing this patch:
> >
> > On Tue, 3 Apr 2012, Vaibhav Hiremath wrote:
> >
> > > AM33XX PRCM module consist of, various clockdomains, in all
> > > total we have 18 clockdomains available, with following
> > > controlling options,
> > > - NO Sleep: sleep transition can not be initiated,
> > > - SW Sleep: sw forced sleep transition,
> > > - SW Wakeup: sw forced wakeup transition,
> > > - HW Auto: transitions are based upon hw conditions.
> > >
> > > This patch adds all available clockdomain data, respective
> > > clockdomain operations for AM33XX family of device, and also
> > > integrates it into existing OMAP framework.
> > >
> > > Signed-off-by: Vaibhav Hiremath <hvaibhav@xxxxxx>
> > > Signed-off-by: Afzal Mohammed <afzal@xxxxxx>
> > > Signed-off-by: Vaibhav Bedia <vaibhav.bedia@xxxxxx>
> >
> > ...
> >
> > > +static struct clockdomain l4ls_am33xx_clkdm = {
> > > + .name = "l4ls_clkdm",
> > > + .pwrdm = { .name = "per_pwrdm" },
> > > + .cm_inst = AM33XX_CM_PER_MOD,
> > > + .clkdm_offs = AM33XX_CM_PER_L4LS_CLKSTCTRL_OFFSET,
> > > + .clktrctrl_mask = AM33XX_CLKTRCTRL_MASK,
> > > + .flags = (CLKDM_CAN_SWSUP | CLKDM_NO_AUTODEPS),
> >
> > It looks to me like we don't need the .clktrctrl_mask field, since
> > according to the clockdomains code, the CLKTRCTRL field is at the same bit
> > shift for each clockdomain.
> >
>
> Yeah, we actually don't need it, probably I added for completeness.
> I will remove it in next version.
I've removed them here.
> > Also, since we're not defining any autodeps for the AM335x platform, we
> > shouldn't need the CLKDM_NO_AUTODEPS flag either? Since no autodeps are
> > defined, the default will be to not set any autodeps.
> >
>
> This is required to avoid few "pr_debug", if you don't provide it.
> For example, without this flag set, you will get,
>
> pr_debug("clockdomain: hardware cannot set/clear sleep "
> "dependency affecting %s from %s\n", clkdm1->name,
> clkdm2->name);
>
> Refer to the file omap_hwmod.c, function, _enable(), the call sequence is,
>
> _enable() => _add_initiator_dep() => clkdm_add_sleepdep() =>leads to warning
Seems like the better thing to do is to just avoid calling
_{add,del}_initiator_dep() on the AM335x.
> > Another question is about the CLKTRCTRL bitfields. According to
> > _AM335x ARM Cortex-A8 Microprocessors (MPUs) Technical Reference
> > Manual_ Rev. D (SPRUH73D), most of the clockdomains support NO_SLEEP mode
> > (i.e., CLKTRCTRL = 0x0). That means that technically, we should also set
> > the CLKDM_CAN_DISABLE_AUTO flag. Unless the documentation is incorrect
> > here? In another section (Table 8-9 "Clock Transition Mode Settings"),
> > it claims that CLKTRCTRL = 0 is not supported...
> >
>
> It is not supported, and should be considered as documentation issue.
Okay so I guess the description for this patch (quoted above) is wrong
then also?
- Paul
--
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
