Hello Vaibhav, Afzal, Vaibhav,

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 <hvaib...@ti.com>
> Signed-off-by: Afzal Mohammed <af...@ti.com>
> Signed-off-by: Vaibhav Bedia <vaibhav.be...@ti.com>

...

> +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.

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.

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...

Also, a question about the L4_WKUP clockdomain:

> +static struct clockdomain l4_wkup_am33xx_clkdm = {
> +     .name           = "l4_wkup_clkdm",
> +     .pwrdm          = { .name = "wkup_pwrdm" },
> +     .cm_inst        = AM33XX_CM_WKUP_MOD,
> +     .clkdm_offs     = AM33XX_CM_WKUP_CLKSTCTRL_OFFSET,
> +     .clktrctrl_mask = AM33XX_CLKTRCTRL_MASK,
> +     .flags          = (CLKDM_CAN_SWSUP | CLKDM_NO_AUTODEPS),
> +};

Table 8-89 "CM_WKUP_CLKSTCTRL Register Field Descriptions" of SPRUH73D 
claims that this clockdomain supports hardware-supervised automatic 
clockdomain transitions.  Again this conflicts with Table 8-9.  Is this a 
documentation bug, or should we update the patch?


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to