RE: [PATCH 6/7] [RFC] OMAP: hwmod: SYSCONFIG register modification for MCBSP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux