| Message ID | 1344417101-5015-8-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/08/2012 04:11 AM, Peter Ujfalusi wrote: > Instead of the callback (which modifies control module register) use the > McBSP module's SYSCONFIG register to disable smart-idle mode when the > sidetone is enabled. > Store the original SIDLEMODE configuration and restore it when the sidetone > has been disabled. Is this another case in which it is required to change the idle-mode on a per-use-case basis? In the past I was trying to do the same with HDMI [1]. In that case it was decided to add the HWMOD_SWSUP_SIDLE to hwmod data with the drawback of using no-idle/force-idle rather than smart-idle[2][3]. [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg60226.html [2] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70463.html [3] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70653.html > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > sound/soc/omap/mcbsp.c | 18 ++++++++++++++---- > sound/soc/omap/mcbsp.h | 1 + > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c > index c870023..e008898 100644 > --- a/sound/soc/omap/mcbsp.c > +++ b/sound/soc/omap/mcbsp.c > @@ -257,8 +257,15 @@ static void omap_st_on(struct omap_mcbsp *mcbsp) > { > unsigned int w; > > - if (mcbsp->pdata->enable_st_clock) > - mcbsp->pdata->enable_st_clock(mcbsp->id, 1); > + /* > + * Sidetone uses McBSP ICLK - which must not idle when sidetones > + * are enabled or sidetones start sounding ugly. > + */ > + w = MCBSP_READ(mcbsp, SYSCON); > + mcbsp->st_data->mcbsp_sidle = (w >> 3) & 0x3; > + w &= ~SIDLEMODE(0x3); > + w |= SIDLEMODE(0x1); > + MCBSP_WRITE(mcbsp, SYSCON, w); Wouldn't this create a mismatch between the SYSCONFIG register and McBSP omap_hwmod._sysc_cache? Perhaps this could be done with a future omap_hwmod API? Ricardo > > /* Enable McBSP Sidetone */ > w = MCBSP_READ(mcbsp, SSELCR); > @@ -279,8 +286,11 @@ static void omap_st_off(struct omap_mcbsp *mcbsp) > w = MCBSP_READ(mcbsp, SSELCR); > MCBSP_WRITE(mcbsp, SSELCR, w & ~(SIDETONEEN)); > > - if (mcbsp->pdata->enable_st_clock) > - mcbsp->pdata->enable_st_clock(mcbsp->id, 0); > + /* Restore the SIDLEMODE as it was before the ST has been started */ > + w = MCBSP_READ(mcbsp, SYSCON); > + w &= ~SIDLEMODE(0x3); > + w |= SIDLEMODE(mcbsp->st_data->mcbsp_sidle); > + MCBSP_WRITE(mcbsp, SYSCON, w); > } > > static void omap_st_fir_write(struct omap_mcbsp *mcbsp, s16 *fir) > diff --git a/sound/soc/omap/mcbsp.h b/sound/soc/omap/mcbsp.h > index 49a6725..ba82846 100644 > --- a/sound/soc/omap/mcbsp.h > +++ b/sound/soc/omap/mcbsp.h > @@ -280,6 +280,7 @@ struct omap_mcbsp_st_data { > int nr_taps; /* Number of filter coefficients in use */ > s16 ch0gain; > s16 ch1gain; > + unsigned int mcbsp_sidle; > }; > > struct omap_mcbsp { >
Hi Ricardo, On 08/09/2012 01:12 AM, Ricardo Neri wrote: > Is this another case in which it is required to change the idle-mode on a > per-use-case basis? In the past I was trying to do the same with HDMI [1]. In > that case it was decided to add the HWMOD_SWSUP_SIDLE to hwmod data with the > drawback of using no-idle/force-idle rather than smart-idle[2][3]. This only concerns use case on OMAP3 when the sidetone is in use. In all other use cases we want to use smart-idle (music playback, FM TX/RX, etc). The main problem here is that the sidetone block have broken sidle line, it can not prevent McBSP iclk to be gated. The sidetone block is using the ICLK of the McBSP port (OMAP3 has ST block attached to McBSP2 and 3 ports only). > [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg60226.html > [2] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70463.html > [3] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70653.html >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> sound/soc/omap/mcbsp.c | 18 ++++++++++++++---- >> sound/soc/omap/mcbsp.h | 1 + >> 2 files changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c >> index c870023..e008898 100644 >> --- a/sound/soc/omap/mcbsp.c >> +++ b/sound/soc/omap/mcbsp.c >> @@ -257,8 +257,15 @@ static void omap_st_on(struct omap_mcbsp *mcbsp) >> { >> unsigned int w; >> >> - if (mcbsp->pdata->enable_st_clock) >> - mcbsp->pdata->enable_st_clock(mcbsp->id, 1); >> + /* >> + * Sidetone uses McBSP ICLK - which must not idle when sidetones >> + * are enabled or sidetones start sounding ugly. >> + */ >> + w = MCBSP_READ(mcbsp, SYSCON); >> + mcbsp->st_data->mcbsp_sidle = (w >> 3) & 0x3; >> + w &= ~SIDLEMODE(0x3); >> + w |= SIDLEMODE(0x1); >> + MCBSP_WRITE(mcbsp, SYSCON, w); > > Wouldn't this create a mismatch between the SYSCONFIG register and McBSP > omap_hwmod._sysc_cache? We modify the bits runtime (if the ST is enabled), after the hwmod already configured the McBSP port to s-idle. We change the McBSP port's s-idle mode back to it's original before pm_runtime_put(). Previously we did the same thing in PRCM level which was as ugly as this workaround with the penalty of a callback function to mach-omap2 since we can only got access to PRCM register API there. > Perhaps this could be done with a future omap_hwmod API? But there's no such an API and I'm not aware any ongoing effort to have one. Still the issue mostly remains since we need to modify other block's s-idle mode. If sidetone block is enabled we need to change the McBSP port's s-idle mode to no-idle. In most of the use cases the McBSP need to be in smart-idle mode since it provides significant power saving.
Hi Peter, On 08/09/2012 02:05 AM, Peter Ujfalusi wrote: > Hi Ricardo, > > On 08/09/2012 01:12 AM, Ricardo Neri wrote: >> Is this another case in which it is required to change the idle-mode on a >> per-use-case basis? In the past I was trying to do the same with HDMI [1]. In >> that case it was decided to add the HWMOD_SWSUP_SIDLE to hwmod data with the >> drawback of using no-idle/force-idle rather than smart-idle[2][3]. > > This only concerns use case on OMAP3 when the sidetone is in use. In all other > use cases we want to use smart-idle (music playback, FM TX/RX, etc). > The main problem here is that the sidetone block have broken sidle line, it > can not prevent McBSP iclk to be gated. > The sidetone block is using the ICLK of the McBSP port (OMAP3 has ST block > attached to McBSP2 and 3 ports only). The situation was similar with HDMI. We need no-idle only when playing audio and smart-idle in all other use cases. With HWMOD_SWSUP_SIDLE we are in no-idle/force-idle all the time. :/ > > >> [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg60226.html >> [2] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70463.html >> [3] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70653.html >>> >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>> --- >>> sound/soc/omap/mcbsp.c | 18 ++++++++++++++---- >>> sound/soc/omap/mcbsp.h | 1 + >>> 2 files changed, 15 insertions(+), 4 deletions(-) >>> >>> diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c >>> index c870023..e008898 100644 >>> --- a/sound/soc/omap/mcbsp.c >>> +++ b/sound/soc/omap/mcbsp.c >>> @@ -257,8 +257,15 @@ static void omap_st_on(struct omap_mcbsp *mcbsp) >>> { >>> unsigned int w; >>> >>> - if (mcbsp->pdata->enable_st_clock) >>> - mcbsp->pdata->enable_st_clock(mcbsp->id, 1); >>> + /* >>> + * Sidetone uses McBSP ICLK - which must not idle when sidetones >>> + * are enabled or sidetones start sounding ugly. >>> + */ >>> + w = MCBSP_READ(mcbsp, SYSCON); >>> + mcbsp->st_data->mcbsp_sidle = (w >> 3) & 0x3; >>> + w &= ~SIDLEMODE(0x3); >>> + w |= SIDLEMODE(0x1); >>> + MCBSP_WRITE(mcbsp, SYSCON, w); >> >> Wouldn't this create a mismatch between the SYSCONFIG register and McBSP >> omap_hwmod._sysc_cache? > > We modify the bits runtime (if the ST is enabled), after the hwmod already > configured the McBSP port to s-idle. We change the McBSP port's s-idle mode > back to it's original before pm_runtime_put(). > > Previously we did the same thing in PRCM level which was as ugly as this > workaround with the penalty of a callback function to mach-omap2 since we can > only got access to PRCM register API there. > >> Perhaps this could be done with a future omap_hwmod API? > > But there's no such an API and I'm not aware any ongoing effort to have one. > Still the issue mostly remains since we need to modify other block's s-idle > mode. If sidetone block is enabled we need to change the McBSP port's s-idle > mode to no-idle. Would omap_hwmod_set_slave_idlemode make the workaround look less ugly? In this case you are changing other block's idle-mode, though. > > In most of the use cases the McBSP need to be in smart-idle mode since it > provides significant power saving. Yes, I was just trying to use this case to justify the need of a driver to be able to set the idle-mode for particular use cases. It would be nice to be able to use an API (present or future) to set to no-idle if required by a particular use-case and still be able to take advantage of smart-idle in other use cases. Ricardo >
diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c index c870023..e008898 100644 --- a/sound/soc/omap/mcbsp.c +++ b/sound/soc/omap/mcbsp.c @@ -257,8 +257,15 @@ static void omap_st_on(struct omap_mcbsp *mcbsp) { unsigned int w; - if (mcbsp->pdata->enable_st_clock) - mcbsp->pdata->enable_st_clock(mcbsp->id, 1); + /* + * Sidetone uses McBSP ICLK - which must not idle when sidetones + * are enabled or sidetones start sounding ugly. + */ + w = MCBSP_READ(mcbsp, SYSCON); + mcbsp->st_data->mcbsp_sidle = (w >> 3) & 0x3; + w &= ~SIDLEMODE(0x3); + w |= SIDLEMODE(0x1); + MCBSP_WRITE(mcbsp, SYSCON, w); /* Enable McBSP Sidetone */ w = MCBSP_READ(mcbsp, SSELCR); @@ -279,8 +286,11 @@ static void omap_st_off(struct omap_mcbsp *mcbsp) w = MCBSP_READ(mcbsp, SSELCR); MCBSP_WRITE(mcbsp, SSELCR, w & ~(SIDETONEEN)); - if (mcbsp->pdata->enable_st_clock) - mcbsp->pdata->enable_st_clock(mcbsp->id, 0); + /* Restore the SIDLEMODE as it was before the ST has been started */ + w = MCBSP_READ(mcbsp, SYSCON); + w &= ~SIDLEMODE(0x3); + w |= SIDLEMODE(mcbsp->st_data->mcbsp_sidle); + MCBSP_WRITE(mcbsp, SYSCON, w); } static void omap_st_fir_write(struct omap_mcbsp *mcbsp, s16 *fir) diff --git a/sound/soc/omap/mcbsp.h b/sound/soc/omap/mcbsp.h index 49a6725..ba82846 100644 --- a/sound/soc/omap/mcbsp.h +++ b/sound/soc/omap/mcbsp.h @@ -280,6 +280,7 @@ struct omap_mcbsp_st_data { int nr_taps; /* Number of filter coefficients in use */ s16 ch0gain; s16 ch1gain; + unsigned int mcbsp_sidle; }; struct omap_mcbsp {
Instead of the callback (which modifies control module register) use the McBSP module's SYSCONFIG register to disable smart-idle mode when the sidetone is enabled. Store the original SIDLEMODE configuration and restore it when the sidetone has been disabled. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- sound/soc/omap/mcbsp.c | 18 ++++++++++++++---- sound/soc/omap/mcbsp.h | 1 + 2 files changed, 15 insertions(+), 4 deletions(-)