Re: [PATCH] OMAP3: hwmod: support to specify the offset position of various SYSCONFIG register bits.
Hi Thara,
this mostly looks good, but there are a few things to fix:
On Mon, 11 Jan 2010, Thara Gopinath wrote:
> In OMAP3 Some modules like Smartreflex do not have the regular sysconfig
> register.Instead clockactivity bits are part of another register at a
> different bit position than the usual bit positions 8 and 9.
>
> In OMAP4, a new scheme is available due to the new protocol
> between the PRCM and the IPs. Depending of the scheme, the SYSCONFIG
> bitfields position will be different.
> The IP_REVISION register should be at offset 0x00.
> It should contain a SCHEME field From this we can determine legacy or HL.
>
> 31:30 SCHEME Used to distinguish between old scheme and current.
> Read 0x0: Legacy ASP or WTBU scheme
> Read 0x1: Highlander 0.8 scheme
>
> For legacy IP
> 13:12 MIDLEMODE
> 11:8 CLOCKACTIVITY
> 6 EMUSOFT
> 5 EMUFREE
> 4:3 SIDLEMODE
> 2 ENAWAKEUP
> 1 SOFTRESET
> 0 AUTOIDLE
>
> For Highlander IP, the bit position in SYSCONFIG is (for simple target):
> 5:4 STANDBYMODE (Ex MIDLEMODE)
> 3:2 IDLEMODE (Ex SIDLEMODE)
> 1 FREEEMU (Ex EMUFREE)
> 0 SOFTRESET
>
> Unfortunately In OMAP4 also some IPs will not follow any of these
> two schemes. This is the case at least for McASP, SmartReflex
> and some security IPs.
>
> This patch introduces a new field sysc_fields in omap_hwmod_sysconfig which
> can be used by the hwmod structures to specify the offsets for the
> sysconfig register of the IP.Also two static structures
> omap_hwmod_sysc_fields and omap_hwmod_sysc_highlander are defined
> which can be used directly to populate the sysc_fields if the IP follows
> legacy or highlander scheme. If the IP follows none of these two schemes
> a new omap_hwmod_sysc_fields structure has to be defined and
> passed as part of omap_hwmod_sysconfig.
>
> Signed-off-by: Thara Gopinath
> Signed-off-by: Benoit Cousson
> Signed-off-by: Paul Walmsley
I haven't signed off on this one yet :-)
> ---
>
> This patch is based off Kevin's tree origin/pm-wip/omap_device branch
>
> arch/arm/mach-omap2/omap_hwmod.c | 14 --
> arch/arm/plat-omap/include/plat/omap_hwmod.h | 26
> +-
> 2 files changed, 29 insertions(+), 11 deletions(-)
>
> Index: linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
> ===
> --- linux-omap-pm.orig/arch/arm/mach-omap2/omap_hwmod.c
> +++ linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
> @@ -135,12 +135,23 @@ static void _write_sysconfig(u32 v, stru
> static int _set_master_standbymode(struct omap_hwmod *oh, u8 standbymode,
> u32 *v)
> {
> + u32 mstandby_mask = 0;
> + u8 mstandby_shift = 0;
There's no reason these variables, or the similar variables below, need to
be pre-initialized to zero. Hopefully the compiler would optimize these
assignments out, but in any case, please remove them, unless they are
absolutely needed.
> +
> if (!oh->sysconfig ||
> !(oh->sysconfig->sysc_flags & SYSC_HAS_MIDLEMODE))
> return -EINVAL;
>
> - *v &= ~SYSC_MIDLEMODE_MASK;
> - *v |= __ffs(standbymode) << SYSC_MIDLEMODE_SHIFT;
> + if (!oh->sysconfig->sysc_fields) {
> + pr_warning("offset struct for sysconfig not provided\n");
Maybe consider WARN() instead? That way, the user can see a stack dump
and know what the offending function is.
> + return -EINVAL;
> + }
> +
> + mstandby_shift = oh->sysconfig->sysc_fields->midle_shift;
> + mstandby_mask = (0x3 << mstandby_shift);
> +
> + *v &= ~mstandby_mask;
> + *v |= __ffs(standbymode) << mstandby_shift;
>
> return 0;
> }
> @@ -157,12 +168,22 @@ static int _set_master_standbymode(struc
> */
> static int _set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode, u32 *v)
> {
> + u32 sidle_mask = 0;
> + u8 sidle_shift = 0;
See above re: unnecessary assignment
> +
> if (!oh->sysconfig ||
> !(oh->sysconfig->sysc_flags & SYSC_HAS_SIDLEMODE))
> return -EINVAL;
>
> - *v &= ~SYSC_SIDLEMODE_MASK;
> - *v |= __ffs(idlemode) << SYSC_SIDLEMODE_SHIFT;
> + if (!oh->sysconfig->sysc_fields) {
> + pr_warning("offset struct for sysconfig not provided\n");
> + return -EINVAL;
> + }
Please add a blank line after closing this code block, as you did above
> + sidle_shift = oh->sysconfig->sysc_fields->sidle_shift;
> + sidle_mask = (0x3 << sidle_shift);
> +
> + *v &= ~sidle_mask;
> + *v |= __ffs(idlemode) << sidle_shift;
>
> return 0;
> }
> @@ -180,12 +201,22 @@ static int _set_slave_idlemode(struct om
> */
> static int _set_clockactivity(struct omap_hwmod *oh, u8 clockact, u32 *v)
> {
> + u32 clkact_mask = 0;
> + u8 clkact_shift = 0;
See above re: unnecessary assignment
> +
> if (!oh->sysconfig ||
> !(oh->sysconfig->sysc_flags & SY
[PATCH] OMAP3: hwmod: support to specify the offset position of various SYSCONFIG register bits.
In OMAP3 Some modules like Smartreflex do not have the regular sysconfig
register.Instead clockactivity bits are part of another register at a
different bit position than the usual bit positions 8 and 9.
In OMAP4, a new scheme is available due to the new protocol
between the PRCM and the IPs. Depending of the scheme, the SYSCONFIG
bitfields position will be different.
The IP_REVISION register should be at offset 0x00.
It should contain a SCHEME field From this we can determine legacy or HL.
31:30 SCHEME Used to distinguish between old scheme and current.
Read 0x0: Legacy ASP or WTBU scheme
Read 0x1: Highlander 0.8 scheme
For legacy IP
13:12 MIDLEMODE
11:8 CLOCKACTIVITY
6 EMUSOFT
5 EMUFREE
4:3 SIDLEMODE
2 ENAWAKEUP
1 SOFTRESET
0 AUTOIDLE
For Highlander IP, the bit position in SYSCONFIG is (for simple target):
5:4 STANDBYMODE (Ex MIDLEMODE)
3:2 IDLEMODE (Ex SIDLEMODE)
1 FREEEMU (Ex EMUFREE)
0 SOFTRESET
Unfortunately In OMAP4 also some IPs will not follow any of these
two schemes. This is the case at least for McASP, SmartReflex
and some security IPs.
This patch introduces a new field sysc_fields in omap_hwmod_sysconfig which
can be used by the hwmod structures to specify the offsets for the
sysconfig register of the IP.Also two static structures
omap_hwmod_sysc_fields and omap_hwmod_sysc_highlander are defined
which can be used directly to populate the sysc_fields if the IP follows
legacy or highlander scheme. If the IP follows none of these two schemes
a new omap_hwmod_sysc_fields structure has to be defined and
passed as part of omap_hwmod_sysconfig.
Signed-off-by: Thara Gopinath
Signed-off-by: Benoit Cousson
Signed-off-by: Paul Walmsley
---
This patch is based off Kevin's tree origin/pm-wip/omap_device branch
arch/arm/mach-omap2/omap_hwmod.c | 14 --
arch/arm/plat-omap/include/plat/omap_hwmod.h | 26 +-
2 files changed, 29 insertions(+), 11 deletions(-)
Index: linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
===
--- linux-omap-pm.orig/arch/arm/mach-omap2/omap_hwmod.c
+++ linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
@@ -135,12 +135,23 @@ static void _write_sysconfig(u32 v, stru
static int _set_master_standbymode(struct omap_hwmod *oh, u8 standbymode,
u32 *v)
{
+ u32 mstandby_mask = 0;
+ u8 mstandby_shift = 0;
+
if (!oh->sysconfig ||
!(oh->sysconfig->sysc_flags & SYSC_HAS_MIDLEMODE))
return -EINVAL;
- *v &= ~SYSC_MIDLEMODE_MASK;
- *v |= __ffs(standbymode) << SYSC_MIDLEMODE_SHIFT;
+ if (!oh->sysconfig->sysc_fields) {
+ pr_warning("offset struct for sysconfig not provided\n");
+ return -EINVAL;
+ }
+
+ mstandby_shift = oh->sysconfig->sysc_fields->midle_shift;
+ mstandby_mask = (0x3 << mstandby_shift);
+
+ *v &= ~mstandby_mask;
+ *v |= __ffs(standbymode) << mstandby_shift;
return 0;
}
@@ -157,12 +168,22 @@ static int _set_master_standbymode(struc
*/
static int _set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode, u32 *v)
{
+ u32 sidle_mask = 0;
+ u8 sidle_shift = 0;
+
if (!oh->sysconfig ||
!(oh->sysconfig->sysc_flags & SYSC_HAS_SIDLEMODE))
return -EINVAL;
- *v &= ~SYSC_SIDLEMODE_MASK;
- *v |= __ffs(idlemode) << SYSC_SIDLEMODE_SHIFT;
+ if (!oh->sysconfig->sysc_fields) {
+ pr_warning("offset struct for sysconfig not provided\n");
+ return -EINVAL;
+ }
+ sidle_shift = oh->sysconfig->sysc_fields->sidle_shift;
+ sidle_mask = (0x3 << sidle_shift);
+
+ *v &= ~sidle_mask;
+ *v |= __ffs(idlemode) << sidle_shift;
return 0;
}
@@ -180,12 +201,22 @@ static int _set_slave_idlemode(struct om
*/
static int _set_clockactivity(struct omap_hwmod *oh, u8 clockact, u32 *v)
{
+ u32 clkact_mask = 0;
+ u8 clkact_shift = 0;
+
if (!oh->sysconfig ||
!(oh->sysconfig->sysc_flags & SYSC_HAS_CLOCKACTIVITY))
return -EINVAL;
- *v &= ~SYSC_CLOCKACTIVITY_MASK;
- *v |= clockact << SYSC_CLOCKACTIVITY_SHIFT;
+ if (!oh->sysconfig->sysc_fields) {
+ pr_warning("offset struct for sysconfig not provided\n");
+ return -EINVAL;
+ }
+ clkact_shift = oh->sysconfig->sysc_fields->clkact_shift;
+ clkact_mask = (0x3 << clkact_shift);
+
+ *v &= ~clkact_mask;
+ *v |= clockact << clkact_shift;
return 0;
}
@@ -200,11 +231,19 @@ static int _set_clockactivity(struct oma
*/
static int _set_softreset(struct omap_hwmod *oh, u32 *v)
{
+ u32 softrst_mask = 0;
+
if (!oh->sysconfig ||
!(oh->sysconfig->sysc_flags & SYSC_HAS_SOFTRESET))
return -EINVAL;
- *v |= SYSC_SOFTRESET_M
