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;
> + }
>
> 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.
>
> 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.
>
> 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.
>
> Please provide your comment on this.
>
> -Kishon
>
> [1] https://patchwork.kernel.org/patch/335591/
> [2] http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/mach-omap2/serial.c;h=edd7c99de38dde5bf877788fb4e48055c0d9fbfa;hb=HEAD
>>
>> >
>> >>
>> >> Signed-off-by: Kishon Vijay Abraham I<kishon@xxxxxx>
>> >> Signed-off-by: Charulatha V<charu@xxxxxx>
>> >> Signed-off-by: Shubhrajyoti D<shubhrajyoti@xxxxxx>
>> >> Cc: Partha Basak<p-basak2@xxxxxx>
>> >> ---
>> >> arch/arm/plat-omap/mcbsp.c | 69
>> ++++++++++++++++++++++++++------------------
>> >> 1 files changed, 41 insertions(+), 28 deletions(-)
>> >>
>> >> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
>> >> index c7c6a83..6b705e1 100644
>> >> --- a/arch/arm/plat-omap/mcbsp.c
>> >> +++ b/arch/arm/plat-omap/mcbsp.c
>> >> @@ -228,10 +228,21 @@ void omap_mcbsp_config(unsigned int id, const
>> struct omap_mcbsp_reg_cfg *config)
>> >> EXPORT_SYMBOL(omap_mcbsp_config);
>> >>
>> >> #ifdef CONFIG_ARCH_OMAP3
>> >> +static struct omap_hwmod **find_hwmods_by_dev(struct device *dev)
>> >> +{
>> >> + struct platform_device *pdev;
>> >> + struct omap_device *od;
>> >> + pdev = container_of(dev, struct platform_device, dev);
>> >> + od = container_of(pdev, struct omap_device, pdev);
>> >> + return od->hwmods;
>> >
>> > Rule #1, don't touch oh in your code. The device should be the only
>> > interface.
>> > If such API is needed, it should be in omap_device. But in your case, I
>> > don't thing it is needed.
>>
>> OK
>> >
>> >> +}
>> >> +
>> >> static void omap_st_on(struct omap_mcbsp *mcbsp)
>> >> {
>> >> unsigned int w;
>> >> + struct omap_hwmod **oh;
>> >>
>> >> + oh = find_hwmods_by_dev(mcbsp->dev);
>> >> /*
>> >> * Sidetone uses McBSP ICLK - which must not idle when sidetones
>> >> * are enabled or sidetones start sounding ugly.
>> >> @@ -244,8 +255,7 @@ static void omap_st_on(struct omap_mcbsp *mcbsp)
>> >> w = MCBSP_READ(mcbsp, SSELCR);
>> >> MCBSP_WRITE(mcbsp, SSELCR, w | SIDETONEEN);
>> >>
>> >> - w = MCBSP_ST_READ(mcbsp, SYSCONFIG);
>> >> - MCBSP_ST_WRITE(mcbsp, SYSCONFIG, w& ~(ST_AUTOIDLE));
>> >> + omap_hwmod_set_module_autoidle(oh[1], 0);
>> >
>> > Don't use internal hwmod API. You have to create a similar omap_device
>> API.
>> > You don't have to select only one hwmod in this case, just change the
>> > sysconfig setting for all hwmod that belong to the omap_device, and it
>> > will be fine. It will avoid you to access one hwmod in particular.
>> > This is the implementation that Manju is doing.
>>
>> OK
>>
>> >
>> >>
>> >> /* Enable Sidetone from Sidetone Core */
>> >> w = MCBSP_ST_READ(mcbsp, SSELCR);
>> >> @@ -255,12 +265,14 @@ static void omap_st_on(struct omap_mcbsp *mcbsp)
>> >> static void omap_st_off(struct omap_mcbsp *mcbsp)
>> >> {
>> >> unsigned int w;
>> >> + struct omap_hwmod **oh;
>> >> +
>> >> + oh = find_hwmods_by_dev(mcbsp->dev);
>> >>
>> >> w = MCBSP_ST_READ(mcbsp, SSELCR);
>> >> MCBSP_ST_WRITE(mcbsp, SSELCR, w& ~(ST_SIDETONEEN));
>> >>
>> >> - w = MCBSP_ST_READ(mcbsp, SYSCONFIG);
>> >> - MCBSP_ST_WRITE(mcbsp, SYSCONFIG, w | ST_AUTOIDLE);
>> >> + omap_hwmod_set_module_autoidle(oh[1], 1);
>> >>
>> >> w = MCBSP_READ(mcbsp, SSELCR);
>> >> MCBSP_WRITE(mcbsp, SSELCR, w& ~(SIDETONEEN));
>> >> @@ -273,9 +285,11 @@ static void omap_st_off(struct omap_mcbsp *mcbsp)
>> >> static void omap_st_fir_write(struct omap_mcbsp *mcbsp, s16 *fir)
>> >> {
>> >> u16 val, i;
>> >> + struct omap_hwmod **oh;
>> >>
>> >> - val = MCBSP_ST_READ(mcbsp, SYSCONFIG);
>> >> - MCBSP_ST_WRITE(mcbsp, SYSCONFIG, val& ~(ST_AUTOIDLE));
>> >> + oh = find_hwmods_by_dev(mcbsp->dev);
>> >> +
>> >> + omap_hwmod_set_module_autoidle(oh[1], 0);
>> >>
>> >> val = MCBSP_ST_READ(mcbsp, SSELCR);
>> >>
>> >> @@ -303,9 +317,11 @@ static void omap_st_chgain(struct omap_mcbsp
>> *mcbsp)
>> >> {
>> >> u16 w;
>> >> struct omap_mcbsp_st_data *st_data = mcbsp->st_data;
>> >> + struct omap_hwmod **oh;
>> >> +
>> >> + oh = find_hwmods_by_dev(mcbsp->dev);
>> >>
>> >> - w = MCBSP_ST_READ(mcbsp, SYSCONFIG);
>> >> - MCBSP_ST_WRITE(mcbsp, SYSCONFIG, w& ~(ST_AUTOIDLE));
>> >> + omap_hwmod_set_module_autoidle(oh[1], 0);
>> >>
>> >> w = MCBSP_ST_READ(mcbsp, SSELCR);
>> >>
>> >> @@ -648,49 +664,46 @@ EXPORT_SYMBOL(omap_mcbsp_get_dma_op_mode);
>> >>
>> >> static inline void omap34xx_mcbsp_request(struct omap_mcbsp *mcbsp)
>> >> {
>> >> + struct omap_hwmod **oh;
>> >> +
>> >> + oh = find_hwmods_by_dev(mcbsp->dev);
>> >> /*
>> >> * Enable wakup behavior, smart idle and all wakeups
>> >> * REVISIT: some wakeups may be unnecessary
>> >> */
>> >> if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
>> >> - u16 syscon;
>> >> -
>> >> - syscon = MCBSP_READ(mcbsp, SYSCON);
>> >> - syscon&= ~(ENAWAKEUP | SIDLEMODE(0x03) |
>> CLOCKACTIVITY(0x03));
>> >>
>> >> if (mcbsp->dma_op_mode == MCBSP_DMA_MODE_THRESHOLD) {
>> >> - syscon |= (ENAWAKEUP | SIDLEMODE(0x02) |
>> >> - CLOCKACTIVITY(0x02));
>> >> - MCBSP_WRITE(mcbsp, WAKEUPEN, XRDYEN | RRDYEN);
>> >> + omap_hwmod_enable_wakeup(oh[0]);
>> >
>> > Thanks to the patch done by Rajendra, the wakeup should enable based on
>> > smartidle config.
>>
>> OK.
>>
>> >
>> >> + omap_hwmod_set_slave_idlemode(oh[0],
>> >> + HWMOD_IDLEMODE_SMART);
>> >> } else {
>> >> - syscon |= SIDLEMODE(0x01);
>> >> + omap_hwmod_disable_wakeup(oh[0]);
>> >> + omap_hwmod_set_slave_idlemode(oh[0],
>> >> + HWMOD_IDLEMODE_NO);
>> >> }
>> >> -
>> >> - MCBSP_WRITE(mcbsp, SYSCON, syscon);
>> >> }
>> >> }
>> >>
>> >> static inline void omap34xx_mcbsp_free(struct omap_mcbsp *mcbsp)
>> >> {
>> >> + struct omap_hwmod **oh;
>> >> +
>> >> + oh = find_hwmods_by_dev(mcbsp->dev);
>> >> /*
>> >> * Disable wakup behavior, smart idle and all wakeups
>> >> */
>> >> if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
>> >
>> > Please try to avoid this cpu_is_xxx checks. You should identified the IP
>> > version if possible or add some SW rev, features or errata in hwmod and
>> > retrieve them during omap_device_build.
>>
>> OK.
>>
>> >
>> >> - u16 syscon;
>> >> -
>> >> - syscon = MCBSP_READ(mcbsp, SYSCON);
>> >> - syscon&= ~(ENAWAKEUP | SIDLEMODE(0x03) |
>> CLOCKACTIVITY(0x03));
>> >> /*
>> >> * HW bug workaround - If no_idle mode is taken, we need
>> to
>> >> * go to smart_idle before going to always_idle, or the
>> >> * device will not hit retention anymore.
>> >> */
>> >
>> > What is that other bug? The changelog does not mention it?
>>
>> This is based on the patch sent by Eero Nurkkala [1]. They've observed
>> when DMA mode is selected to element sync(NO IDLE is set in sysconfig),
>> the device does not hit retention if force idle mode is selected
>> without switching to smart idle..
>>
>> [1] http://kerneltrap.org/mailarchive/alsa-devel/2009/8/20/6333573
>> >
>> >> - syscon |= SIDLEMODE(0x02);
>> >> - MCBSP_WRITE(mcbsp, SYSCON, syscon);
>> >> -
>> >> - syscon&= ~(SIDLEMODE(0x03));
>> >> - MCBSP_WRITE(mcbsp, SYSCON, syscon);
>> >> -
>> >> + omap_hwmod_disable_wakeup(oh[0]);
>> >> + omap_hwmod_set_slave_idlemode(oh[0],
>> >> + HWMOD_IDLEMODE_SMART);
>> >> + omap_hwmod_set_slave_idlemode(oh[0],
>> >> + HWMOD_IDLEMODE_FORCE);
>> >> MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
>> >> }
>> >> }
>> >
>> > 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
