Re: [RFC 1/2] ARM: OMAP2+: hwmod: Add refcounting for modulemode shared by multiple hwmods

[Rajendra Nayak <rnayak@ti.com>]

On Monday 26 May 2014 04:14 PM, Archit Taneja wrote:
> Generally, IP blocks/modules within a clock domain each have their own
> CM_x_CLKCTRL register, each having it's own MODULEMODE field to manage the
> module.
> 
> DSS clockdoain, however, has multiple modules in it, but only one register
> named CM_DSS_DSS_CLKCTRL, which contains one MODULEMODE register field.
> 
> Until now, we defined '.prcm.omap4.modulemode' only for the top level DSS
> hwmod("dss_core") and didn't define it for other DSS hwmods(like "dss_dispc",
> "dss_dsi1" and so on). This made the omapdss driver work as the top level DSS
> platform device and the rest had a parent-child relationship. This ensured that
> the parent hwmod("dss_core") is enabled if any of the children hwmods are
> enabled while using omapdss.
> 
> This method, however, doesn't work when each hwmod is enabled individually.
> This happens early in boot in omap_hwmod_setup_all, where each hwmod is enabled,
> and then reset and idled. All the 'children' DSS hwmods fail to enable and
> reset, since they don't enable modulemode.
> 
> The way to make such modules work both during initialization and when used by
> pm runtime API in the driver would be to add refcounting for enabling/disabling
> the modulemode field.
> 
> We add a new flag bit for the flag in omap_hwmod_omap4_prcm, which tells
> omap_hwmod that this hwmod uses a modulemode field shared by other hwmods.
> 
> We create a struct called 'modulemode_shared'. The hwmod data file should define
> a static instance of this struct. Each hwmod that uses this modulemode field
> should hold a pointer to this instance.
> 
> omap_hwmod's soc enable_module and disable_module ops set the MODULEMODE
> reigster bits only when the first module using it is enabled, or the last module
> using it is disabled. We serialize accesses to the struct with a spinlock.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c | 123 +++++++++++++++++++++++++++++++++------
>  arch/arm/mach-omap2/omap_hwmod.h |  38 +++++++++---
>  2 files changed, 133 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 66c60fe..b42718d 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -973,17 +973,33 @@ static void _disable_optional_clocks(struct omap_hwmod *oh)
>   */
>  static void _omap4_enable_module(struct omap_hwmod *oh)
>  {
> +	unsigned long flags;
> +	struct modulemode_shared *mmode = NULL;
> +	bool enable = true;
> +
>  	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
>  		return;
>  
>  	pr_debug("omap_hwmod: %s: %s: %d\n",
>  		 oh->name, __func__, oh->prcm.omap4.modulemode);
>  
> -	omap4_cminst_module_enable(oh->prcm.omap4.modulemode,
> -				   oh->clkdm->prcm_partition,
> -				   oh->clkdm->cm_inst,
> -				   oh->clkdm->clkdm_offs,
> -				   oh->prcm.omap4.clkctrl_offs);
> +	if (oh->prcm.omap4.flags & HWMOD_OMAP4_MODULEMODE_SHARED) {
> +		mmode = oh->prcm.omap4.modulemode_ref;
> +
> +		spin_lock_irqsave(&mmode->lock, flags);
> +
> +		enable = mmode->refcnt++ == 0;
> +	}
> +
> +	if (enable)
> +		omap4_cminst_module_enable(oh->prcm.omap4.modulemode,
> +					   oh->clkdm->prcm_partition,
> +					   oh->clkdm->cm_inst,
> +					   oh->clkdm->clkdm_offs,
> +					   oh->prcm.omap4.clkctrl_offs);
> +
> +	if (mmode)
> +		spin_unlock_irqrestore(&mmode->lock, flags);
>  }
>  
>  /**
> @@ -995,15 +1011,32 @@ static void _omap4_enable_module(struct omap_hwmod *oh)
>   */
>  static void _am33xx_enable_module(struct omap_hwmod *oh)
>  {
> +	unsigned long flags;
> +	struct modulemode_shared *mmode = NULL;
> +	bool enable = true;
> +
>  	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
>  		return;
>  
>  	pr_debug("omap_hwmod: %s: %s: %d\n",
>  		 oh->name, __func__, oh->prcm.omap4.modulemode);
>  
> -	am33xx_cm_module_enable(oh->prcm.omap4.modulemode, oh->clkdm->cm_inst,
> -				oh->clkdm->clkdm_offs,
> -				oh->prcm.omap4.clkctrl_offs);
> +	if (oh->prcm.omap4.flags & HWMOD_OMAP4_MODULEMODE_SHARED) {
> +		mmode = oh->prcm.omap4.modulemode_ref;
> +
> +		spin_lock_irqsave(&mmode->lock, flags);
> +
> +		enable = mmode->refcnt++ == 0;
> +	}
> +
> +	if (enable)
> +		am33xx_cm_module_enable(oh->prcm.omap4.modulemode,
> +					oh->clkdm->cm_inst,
> +					oh->clkdm->clkdm_offs,
> +					oh->prcm.omap4.clkctrl_offs);
> +
> +	if (mmode)
> +		spin_unlock_irqrestore(&mmode->lock, flags);
>  }
>  
>  /**
> @@ -1846,6 +1879,9 @@ static bool _are_any_hardreset_lines_asserted(struct omap_hwmod *oh)
>  static int _omap4_disable_module(struct omap_hwmod *oh)
>  {
>  	int v;
> +	unsigned long flags;
> +	struct modulemode_shared *mmode = NULL;
> +	bool disable = true;
>  
>  	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
>  		return -EINVAL;
> @@ -1859,15 +1895,30 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>  
>  	pr_debug("omap_hwmod: %s: %s\n", oh->name, __func__);
>  
> -	omap4_cminst_module_disable(oh->clkdm->prcm_partition,
> +	if (oh->prcm.omap4.flags & HWMOD_OMAP4_MODULEMODE_SHARED) {
> +		mmode = oh->prcm.omap4.modulemode_ref;
> +
> +		spin_lock_irqsave(&mmode->lock, flags);
> +
> +		WARN_ON(mmode->refcnt == 0);
> +
> +		disable = --mmode->refcnt == 0;
> +	}
> +
> +	if (disable) {
> +		omap4_cminst_module_disable(oh->clkdm->prcm_partition,
>  				    oh->clkdm->cm_inst,
>  				    oh->clkdm->clkdm_offs,
>  				    oh->prcm.omap4.clkctrl_offs);
>  
> -	v = _omap4_wait_target_disable(oh);
> -	if (v)
> -		pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> -			oh->name);
> +		v = _omap4_wait_target_disable(oh);
> +		if (v)
> +			pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> +				oh->name);
> +	}
> +
> +	if (mmode)
> +		spin_unlock_irqrestore(&mmode->lock, flags);
>  
>  	return 0;
>  }
> @@ -1882,6 +1933,9 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>  static int _am33xx_disable_module(struct omap_hwmod *oh)
>  {
>  	int v;
> +	unsigned long flags;
> +	struct modulemode_shared *mmode = NULL;
> +	bool disable = true;
>  
>  	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
>  		return -EINVAL;
> @@ -1891,13 +1945,28 @@ static int _am33xx_disable_module(struct omap_hwmod *oh)
>  	if (_are_any_hardreset_lines_asserted(oh))
>  		return 0;
>  
> -	am33xx_cm_module_disable(oh->clkdm->cm_inst, oh->clkdm->clkdm_offs,
> -				 oh->prcm.omap4.clkctrl_offs);
> +	if (oh->prcm.omap4.flags & HWMOD_OMAP4_MODULEMODE_SHARED) {
> +		mmode = oh->prcm.omap4.modulemode_ref;
>  
> -	v = _am33xx_wait_target_disable(oh);
> -	if (v)
> -		pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> -			oh->name);
> +		spin_lock_irqsave(&mmode->lock, flags);
> +
> +		WARN_ON(mmode->refcnt == 0);
> +
> +		disable = --mmode->refcnt == 0;
> +	}
> +
> +	if (disable) {
> +		am33xx_cm_module_disable(oh->clkdm->cm_inst, oh->clkdm->clkdm_offs,
> +					 oh->prcm.omap4.clkctrl_offs);
> +
> +		v = _am33xx_wait_target_disable(oh);
> +		if (v)
> +			pr_warn("omap_hwmod: %s: _wait_target_disable failed\n",
> +				oh->name);
> +	}
> +
> +	if (mmode)
> +		spin_unlock_irqrestore(&mmode->lock, flags);
>  
>  	return 0;
>  }
> @@ -2751,6 +2820,13 @@ static int __init _register(struct omap_hwmod *oh)
>  	if (_lookup(oh->name))
>  		return -EEXIST;
>  
> +	if (oh->prcm.omap4.flags & HWMOD_OMAP4_MODULEMODE_SHARED &&
> +			!oh->prcm.omap4.modulemode_ref) {

You might also want to check for someone populating a modulemode_ref but
failing to populate the flag?

Alternatively, Since you expect a modulemode_ref to be always available for all modules which
share modulemode, that in itself can be used to identify such modules without the
need of an additional flag?

> +		pr_err("omap_hwmod: %s shares modulemode, but doesn't hold a ref to it\n",
> +			oh->name);
> +		return -EINVAL;
> +	}
> +
>  	list_add_tail(&oh->node, &omap_hwmod_list);
>  
>  	INIT_LIST_HEAD(&oh->master_ports);
> @@ -2759,6 +2835,15 @@ static int __init _register(struct omap_hwmod *oh)
>  
>  	oh->_state = _HWMOD_STATE_REGISTERED;
>  
> +	if (oh->prcm.omap4.flags & HWMOD_OMAP4_MODULEMODE_SHARED) {
> +		struct modulemode_shared *mmode = oh->prcm.omap4.modulemode_ref;
> +
> +		if (!mmode->registered) {
> +			spin_lock_init(&mmode->lock);
> +			mmode->registered = true;

If this is only used to keep track of the spin_lock being initialized, maybe it'll be
more readable if you just call it mmode->spin_lock_init = true.

> +		}
> +	}
> +
>  	/*
>  	 * XXX Rather than doing a strcmp(), this should test a flag
>  	 * set in the hwmod data, inserted by the autogenerator code.
> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
> index 0f97d63..64c2af6 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.h
> +++ b/arch/arm/mach-omap2/omap_hwmod.h
> @@ -432,13 +432,31 @@ struct omap_hwmod_omap2_prcm {
>  };
>  
>  /*
> + * struct for managing modulemode field shared accross IP blocks
> + * @lock: spinlock to searilze enables/disables performed by different
> + *     hwmods sharing the same modulemode field
> + * @refcnt: keep a refcount of the enabled hwmods using this modulemode
> + *     field
> + * @registered: a flag to track whether the struct has been registered
> + */
> +struct modulemode_shared {
> +	spinlock_t	lock;
> +	unsigned	refcnt;
> +	bool		registered;
> +};
> +
> +/*
>   * Possible values for struct omap_hwmod_omap4_prcm.flags
>   *
>   * HWMOD_OMAP4_NO_CONTEXT_LOSS_BIT: Some IP blocks don't have a PRCM
>   *     module-level context loss register associated with them; this
>   *     flag bit should be set in those cases
> + * HWMOD_OMAP4_MODULEMODE_SHARED: Some IP blocks belonging to the same
> + *     clock domain may have a shared MODULEMODE field; this flag bit
> + *     should be set in those cases
>   */
>  #define HWMOD_OMAP4_NO_CONTEXT_LOSS_BIT		(1 << 0)
> +#define HWMOD_OMAP4_MODULEMODE_SHARED		(1 << 1)
>  
>  /**
>   * struct omap_hwmod_omap4_prcm - OMAP4-specific PRCM data
> @@ -450,6 +468,7 @@ struct omap_hwmod_omap2_prcm {
>   * @submodule_wkdep_bit: bit shift of the WKDEP range
>   * @flags: PRCM register capabilities for this IP block
>   * @modulemode: allowable modulemodes
> + * @modulemode_ref: pointer to modulemode struct shared by multiple hwmods
>   * @context_lost_counter: Count of module level context lost
>   *
>   * If @lostcontext_mask is not defined, context loss check code uses
> @@ -458,15 +477,16 @@ struct omap_hwmod_omap2_prcm {
>   * more hwmods.
>   */
>  struct omap_hwmod_omap4_prcm {
> -	u16		clkctrl_offs;
> -	u16		rstctrl_offs;
> -	u16		rstst_offs;
> -	u16		context_offs;
> -	u32		lostcontext_mask;
> -	u8		submodule_wkdep_bit;
> -	u8		modulemode;
> -	u8		flags;
> -	int		context_lost_counter;
> +	u16				clkctrl_offs;
> +	u16				rstctrl_offs;
> +	u16				rstst_offs;
> +	u16				context_offs;
> +	u32				lostcontext_mask;
> +	u8				submodule_wkdep_bit;
> +	u8				modulemode;
> +	struct modulemode_shared	*modulemode_ref;
> +	u8				flags;
> +	int				context_lost_counter;
>  };
>  
>  
>