On Wed, Dec 1, 2010 at 4:45 PM, Cousson, Benoit <b-cous...@ti.com> wrote:
> + Govindraj
>
> Hi Partha,
>
> On 12/1/2010 8:14 AM, Basak, Partha wrote:
>>
>>> From: Cousson, Benoit
>>> Sent: Tuesday, November 30, 2010 9:33 PM
>>>
>>> Hi Kishon,
>>>
>>> Sorry, for the delay.
>>>
>>> On 11/22/2010 4:59 PM, ABRAHAM, KISHON VIJAY wrote:
>>>>
>>>> Resending the mail in plain text format..
>>>>
>>>> On Mon, Nov 22, 2010 at 9:20 PM, ABRAHAM, KISHON VIJAY<kis...@ti.com>
>>>
>>> wrote:
>>>>>
>>>>> On Mon, Oct 11, 2010 at 11:48 AM, ABRAHAM, KISHON
>>>
>>> VIJAY<kis...@ti.com> wrote:
>>>>>>
>>>>>> On Friday 08 October 2010 01:12 PM, Cousson, Benoit wrote:
>>>>>>>
>>>>>>> Hi Kishon,
>>>>>>>
>>>>>>> On 10/5/2010 6:37 PM, ABRAHAM, KISHON VIJAY wrote:
>>>>>>>>
>>>>>>>> MCBSP 2 and 3 in OMAP3 has sidetone feature which requires
>>>>>>>> autoidle to be disabled before starting the sidetone. Also
>>>
>>> SYSCONFIG
>>>>>>>>
>>>>>>>> register has to be set with smart idle or no idle depending on
>>>
>>> the
>>>>>>>>
>>>>>>>> dma op mode (threshold or element sync). For doing these
>>>
>>> operations
>>>>>>>>
>>>>>>>> dynamically at runtime, hwmod API'S are used to modify SYSCONFIG
>>>>>>
>>>>>> register
>>>>>>>>
>>>>>>>> directly.
>>>>>>>
>>>>>>> OK, it looks like we don't have the choice... But for the
>>>>>>> implementation, please discussed with Manju on that, He is doing
>>>
>>> the
>>>>>>>
>>>>>>> similar thing for the smartstandby issue on SDMA.
>>>>>>
>>>>>> OK.
>>>>>
>>>>>
>>>>> Looks like we have a problem when we write an API similar to
>>>
>>> the one written
>>>>>
>>>>> by Manju (omap_hwmod_set_master_standbymode()) [1]. In the
>>>
>>> case of McBSP,
>>>>>
>>>>> I have to modify omap_hwmod_set_slave_idlemode() (which is
>>>
>>> already existing),
>>>>>
>>>>> to include something like
>>>>>
>>>>> + if (sf& SYSC_HAS_SIDLEMODE) {
>>>>> + if (idlemode)
>>>>> + idlemode = HWMOD_IDLEMODE_NO;
>>>>> + else
>>>>> + idlemode = (oh->flags&
>>>
>>> HWMOD_SWSUP_SIDLE) ?
>>>>>
>>>>> + HWMOD_IDLEMODE_NO :
>>>
>>> HWMOD_IDLEMODE_SMART;
>>>>>
>>>>> + }
>>>
>>> How to you enable HWMOD_IDLEMODE_FORCE mode in that case?
>>>
>>>>> Then an API like omap_device_require_no_idle() and
>>>
>>> omap_device_release_no_idle()
>>>>>
>>>>> would be written similar to omap_device_require_no_mstandby
>>>
>>> and
>>>>>
>>>>> omap_device_release_no_mstandby() (see [1]) that calls
>>>>> omap_hwmod_set_slave_idlemode() with true/false. Passing true
>>>
>>> to
>>>>>
>>>>> omap_hwmod_set_slave_idlemode() will write SIDLE bits with
>>>>> HWMOD_IDLEMODE_NO and false to it will write
>>>>> HWMOD_IDLEMODE_NO/HWMOD_IDLEMODE_SMART depending on
>>>>> HWMOD_SWSUP_SIDLE to SIDLE bits.
>>>>>
>>>>> This works fine for McBSP since only two modes come to play
>>>
>>> here
>>>>>
>>>>> (HWMOD_IDLEMODE_NO/HWMOD_IDLEMODE_SMART).
>>>>>
>>>>> But omap_hwmod_set_slave_idlemode() API is used by UART
>>>>> (serial.c Please refer [2]) which writes SIDLE bits to
>>>>> HWMOD_IDLEMODE_NO/HWMOD_IDLEMODE_SMART/ and
>>>>> HWMOD_IDLEMODE_FORCE.
>>>
>>> That code should not be there. It was a temporary hacks that should be
>>> replaced eventually with a clean omap_device API like the one you are
>>> currently implementing.
>>> There are a bunch of direct access to omap_hwmod struct that need to be
>>> removed as well.
>>>
>>>>> Modifying omap_hwmod_set_slave_idlemode() will require
>>>>> 1) serial.c to be modified to call functions like
>>>
>>> 'omap_device_require_no_idle(),
>>>>>
>>>>> omap_device_release_no_idle()' and
>>>
>>> 'omap_device_require_force_idle() and
>>>>>
>>>>> omap_device_release_force_idle()' instead of
>>>>> omap_hwmod_set_slave_idlemode() which is currently
>>>
>>> present.
>>>
>>> Yep, you're right.
>>>
>>>>>
>>>>> There are 2 problems associated with it
>>>>> 1) The first problem is having a single API like
>>>
>>> omap_hwmod_set_slave_idlemode() to
>>>>>
>>>>> set two different values like HWMOD_IDLEMODE_NO or
>>>>> HWMOD_IDLEMODE_FORCE (intended to be called by
>>>
>>> omap_device_require_no_idle()
>>>>>
>>>>> and omap_device_require_force_idle() respectively).
>>>
>>> According to the new design
>>>>>
>>>>> omap_hwmod_set_slave_idlemode() is intended to take only
>>>
>>> true/false as
>>>>>
>>>>> arguments.
>>>>>
>>>>> 2) The second problem is having the release API's
>>>
>>> (omap_device_release_no_idle() and
>>>>>
>>>>> omap_device_release_force_idle()) to restore the
>>>
>>> SYSCONFIG to the previous state.
>>>>>
>>>>> Remember, this was not problem for McBSP since only two
>>>
>>> modes were needed.
>>>
>>> And what about 3 APIs?
>>>
>>> omap_device_force_idle
>>> omap_device_no_idle
>>> omap_device_smart_idle
>>
>> Want to reiterate Paul Walmsley's comments on Manju's DMA email dated
>> 11/11/2010
>> <snip>
>> 2. Rather than just using a single omap_device_mstandby() function call,
>> I'd rather see something like omap_device_require_no_standby() and
>> omap_device_release_no_standby(). omap_device_require_no_standby() would
>> force the initiator port into no-standby.
>> omap_device_release_no_standby() would return the initiator port
>> MSTANDBYMODE to whatever the state should be (this depends on whether
>> HWMOD_SWSUP_MSTDBY is set). I'd rather not have the drivers manage the
>> contents of the MSTANDBYMODE bits directly. Then you should be able to
>> drop _get_master_standbymode() and omap_hwmod_get_master_idlemode() also.
>> Can you please make these changes?
>> <snip>
>>
>> The key point is, in the absence of any special situation like errata etc,
>> the IdleMode settings should
>> be dicated solely by the hwmod flags. We should have a mechanism though to
>> supersede this setting& request
>> /release customized settings to handle errata etc.
>>
>> So, it makes sense to have request/release API pairs. In this case,
>> omap_device_request_no_idle()& omap_device_release_no_idle().
>>
>> omap_hwmod_set_slave_idlemode should continue to take absolute values
>> instead of true/false to support
>> all possible settings of IdleMode.
>
> Mmm, and how that email is helping to progress in the discussion?
>
> The issue is not really for the mcbsp but for the serial that need to handle
> the 3 different states.
>
> if (enable) {
> /**
> * Errata 2.15: [UART]:Cannot Acknowledge Idle Requests
> * in Smartidle Mode When Configured for DMA Operations.
> */
> if (uart->dma_enabled)
> idlemode = HWMOD_IDLEMODE_FORCE;
> else
> idlemode = HWMOD_IDLEMODE_SMART;
> } else {
> idlemode = HWMOD_IDLEMODE_NO;
> }
>
> You do need to explicitly set the 3 modes, hence the following 3 APIs:
> omap_device_force_idle
> omap_device_no_idle
> omap_device_smart_idle
Agree, if we have to hide "oh" info from driver use only pdev.
we need those 3 exported API's.
>
> You can potentially add another API to restore the default idle mode:
>
> omap_device_default_idle
If default mode is smart idle mode?
we cant enter low power states as we may need to be in force idle mode.
--
Thanks
Govindraj.R
>
> That seems much simpler than 3 pairs of APIs.
>
> Regards,
> Benoit
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
