On Wed, Dec 1, 2010 at 4:45 PM, Cousson, Benoit <b-cousson@xxxxxx> 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<kishon@xxxxxx> >>> >>> wrote: >>>>> >>>>> On Mon, Oct 11, 2010 at 11:48 AM, ABRAHAM, KISHON >>> >>> VIJAY<kishon@xxxxxx> 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 majordomo@xxxxxxxxxxxxxxx > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html