On Tue, Mar 1, 2011 at 4:47 PM, Cousson, Benoit <b-cousson@xxxxxx> wrote:
> 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.
Should it then be like one-one mapping between omap_device APIs and
omap_hwmod APIs, or have a single omap_device API, that takes a additional
parameter and based on the parameter, call the appropriate omap_hwmod API?
>
> 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@xxxxxx>
>> Signed-off-by: Benoit Cousson<b-cousson@xxxxxx>
>> Cc: Paul Walmsley<paul@xxxxxxxxx>
>> ---
>> 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.
OK.
>
> Benoit
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
