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

[kishon <a0393678@ti.com>]

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.

>
>>
>> Signed-off-by: Kishon Vijay Abraham I<kishon@ti.com>
>> Signed-off-by: Charulatha V<charu@ti.com>
>> Signed-off-by: Shubhrajyoti D<shubhrajyoti@ti.com>
>> Cc: Partha Basak<p-basak2@ti.com>
>> ---
>>    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