> -----Original Message-----
> From: Cousson, Benoit
> Sent: Tuesday, November 30, 2010 9:33 PM
> To: ABRAHAM, KISHON VIJAY; Paul Walmsley
> Cc: Kevin Hilman; Basak, Partha; linux-omap@xxxxxxxxxxxxxxx; Kamat,
> Nishant; Varadarajan, Charulatha; Datta, Shubhrajyoti; ext-
> eero.nurkkala@xxxxxxxxx; eduardo.valentin@xxxxxxxxx
> Subject: Re: [PATCH 6/7] [RFC] OMAP: hwmod: SYSCONFIG register
> modification for MCBSP
>
> 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.
>
> It is probably much more appropriate than a require / release in that
> case?
>
> 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
