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

Cousson, Benoit
2011-03-01T11:17:49Z

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