Re: [PATCH v2 2/2] OMAP: omap_device: API to modify AUTOIDLE and SMARTIDLE bits

["Cousson, Benoit" <b-cousson@ti.com>]

Hi Kishon,

Sorry for this late reply.

I'm fine with this omap_device API which is well aligned with the 
discussion we had. I just have few minor comments about the split 
omap_hwmod / omap_device.

To summarize, you should not hack directly hwmod internal attributes 
from the upper layer.

You should simply expose the same number of API at hwmod level instead 
of relying on a single omap_hwmod_set_slave_idlemode with miscellaneous 
parameters.

On 1/31/2011 1:34 PM, ABRAHAM, KISHON VIJAY wrote:
> Provide APIs to be used by the driver in order to modify AUTOIDLE
> and SIDLE bits. These APIs in turn call hwmod APIs to modify the
> SYSCONFIG register.
>
> Signed-off-by: Kishon Vijay Abraham I<kishon@ti.com>
> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> Cc: Paul Walmsley<paul@pwsan.com>
> ---
>   arch/arm/plat-omap/include/plat/omap_device.h |    6 +
>   arch/arm/plat-omap/omap_device.c              |  176 +++++++++++++++++++++++++
>   2 files changed, 182 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
> index e4c349f..47ad0ab 100644
> --- a/arch/arm/plat-omap/include/plat/omap_device.h
> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
> @@ -110,6 +110,12 @@ struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
>   u32 omap_device_get_context_loss_count(struct platform_device *pdev);
>
>   /* Other */
> +int omap_device_noidle(struct omap_device *od);
> +int omap_device_smartidle(struct omap_device *od);
> +int omap_device_forceidle(struct omap_device *od);
> +int omap_device_default_idle(struct omap_device *od);
> +int omap_device_enable_autoidle(struct omap_device *od);
> +int omap_device_disable_autoidle(struct omap_device *od);
>
>   int omap_device_idle_hwmods(struct omap_device *od);
>   int omap_device_enable_hwmods(struct omap_device *od);
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index 57adb27..da8609a 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -726,6 +726,182 @@ void __iomem *omap_device_get_rt_va(struct omap_device *od)
>   	return omap_hwmod_get_mpu_rt_va(od->hwmods[0]);
>   }
>
> +/**
> + * omap_device_noidle - set the device's slave idlemode to no idle
> + * @od: struct omap_device *
> + *
> + * Sets the IP block's OCP slave idlemode in hardware to no idle.
> + * Intended to be used by drivers that have some erratum that requires direct
> + * manipulation of the SIDLEMODE bits.  Returns -EINVAL if @od is in invalid
> + * state, or passes along the return value from
> + * omap_hwmod_set_slave_idlemode().
> + */
> +int omap_device_noidle(struct omap_device *od)
> +{
> +	int retval = 0, i;
> +
> +	if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
> +		WARN(1, "omap_device: %s.%d: %s() called from invalid state "
> +			"%d\n", od->pdev.name, od->pdev.id, __func__,
> +			od->_state);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i<  od->hwmods_cnt; i++)
> +		retval |= omap_hwmod_set_slave_idlemode(od->hwmods[i],
> +					HWMOD_IDLEMODE_NO);
> +
> +	return retval;
> +}
> +
> +
> +/**
> + * omap_device_smartidle - set the device's slave idlemode to smart idle
> + * @od: struct omap_device *
> + *
> + * Sets the IP block's OCP slave idlemode in hardware to smart idle.
> + * Intended to be used by drivers that have some erratum that requires direct
> + * manipulation of the SIDLEMODE bits.  Returns -EINVAL if @od is in invalid
> + * state, or passes along the return value from
> + * omap_hwmod_set_slave_idlemode().
> + */
> +int omap_device_smartidle(struct omap_device *od)
> +{
> +	int retval = 0, i;
> +
> +	if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
> +		WARN(1, "omap_device: %s.%d: %s() called from invalid state "
> +			"%d\n", od->pdev.name, od->pdev.id, __func__,
> +			od->_state);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i<  od->hwmods_cnt; i++)
> +		retval |= omap_hwmod_set_slave_idlemode(od->hwmods[i],
> +					HWMOD_IDLEMODE_SMART);
> +
> +	return retval;
> +}
> +
> +
> +/**
> + * omap_device_forceidle - set the device's slave idlemode to force idle
> + * @od: struct omap_device *
> + *
> + * Sets the IP block's OCP slave idlemode in hardware to force idle.
> + * Intended to be used by drivers that have some erratum that requires direct
> + * manipulation of the SIDLEMODE bits.  Returns -EINVAL if @od is in invalid
> + * state, or passes along the return value from
> + * omap_hwmod_set_slave_idlemode().
> + */
> +int omap_device_forceidle(struct omap_device *od)
> +{
> +	int retval = 0, i;
> +
> +	if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
> +		WARN(1, "omap_device: %s.%d: %s() called from invalid state "
> +			"%d\n", od->pdev.name, od->pdev.id, __func__,
> +			od->_state);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i<  od->hwmods_cnt; i++)
> +		retval |= omap_hwmod_set_slave_idlemode(od->hwmods[i],
> +					HWMOD_IDLEMODE_FORCE);
> +
> +	return retval;
> +}
> +
> +/**
> + * omap_device_default_idle - set the device's slave idlemode to no idle or
> + * smart idle based on the hwmod flag
> + * @od: struct omap_device *
> + *
> + * Sets the IP block's OCP slave idlemode in hardware to no idle or smart idle
> + * depending on status of the flag HWMOD_SWSUP_SIDLE.
> + * Intended to be used by drivers that have some erratum that requires direct
> + * manipulation of the SIDLEMODE bits.  Returns -EINVAL if @od is in invalid
> + * state, or passes along the return value from
> + * omap_hwmod_set_slave_idlemode().
> + */
> +int omap_device_default_idle(struct omap_device *od)
> +{
> +	int retval = 0, i;
> +	u8 idlemode;
> +
> +	if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
> +		WARN(1, "omap_device: %s.%d: %s() called from invalid state "
> +			"%d\n", od->pdev.name, od->pdev.id, __func__,
> +		od->_state);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i<  od->hwmods_cnt; i++) {
> +		idlemode = (od->hwmods[i]->flags&  HWMOD_SWSUP_SIDLE) ?
> +				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
> +		retval |= omap_hwmod_set_slave_idlemode(od->hwmods[i],
> +				idlemode);

That one is the worst :-) You should definitively not hack directly 
hwmod internal attributes from the upper layer.

> +	}
> +
> +	return retval;
> +}
> +
> +/*
> + * omap_device_enable_autoidle - enable the device's OCP slave autoidle
> + * @od: struct omap_device *
> + *
> + * Sets the IP block's OCP slave autoidle in hardware.
> + * Intended to be used by drivers that have some erratum that requires direct
> + * manipulation of the AUTOIDLE bits.  Returns -EINVAL if @od is in invalid
> + * state, or passes along the return value from
> + * omap_hwmod_set_slave_autoidle().
> + */
> +int omap_device_enable_autoidle(struct omap_device *od)
> +{
> +	int retval = 0, i;
> +
> +	if (od->_state != OMAP_DEVICE_STATE_ENABLED) {
> +		WARN(1, "omap_device: %s.%d: %s() called from invalid state "
> +			"%d\n", od->pdev.name, od->pdev.id, __func__,
> +			od->_state);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i<  od->hwmods_cnt; i++)
> +		retval |= omap_hwmod_set_slave_autoidle(od->hwmods[i],
> +					true);

You are using the same API to send either HWMOD_XXX flags or boolean, 
and that's quite confusing.

As I said, you'd better duplicate these APIs at hwmod level.

Benoit