[RFC: PATCH] New API to modify the autoidle bits of sysconfig register

[RFC: PATCH] New API to modify the autoidle bits of sysconfig register

[Kishon Vijay Abraham I]

Though the sysconfig register values shouldn't be modified directly by
the driver, MCBSP should be considered a special case where sysconfig
registers need to be modified dynamically by the driver.

For e.g MCBSP 2 and 3 in OMAP3 has sidetone feature which requires
autoidle to be disabled before starting the sidetone.

This patch creates a new API that forms a wrapper to
_set_module_autoidle() to modify the AUTOIDLE bit.

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: Paul Walmsley<paul@pwsan.com>
Cc: Benoit Cousson<b-cousson@ti.com>
Cc: Partha Basak <p-basak2@ti.com>

---
 arch/arm/mach-omap2/omap_hwmod.c             |   29 ++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 9bd99ad..0d38404 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -969,6 +969,35 @@ int _omap_hwmod_idle(struct omap_hwmod *oh)
 }
 
 /**
+ * omap_hwmod_set_module_autoidle - set the hwmod's OCP slave autoidle
+ * @oh: struct omap_hwmod *
+ * @autoidle: desired AUTOIDLE bitfield value (0 or 1)
+ *
+ * Sets the IP block's OCP slave autoidle in hardware, and updates our
+ * local copy. Intended to be used by drivers that have some erratum
+ * that requires direct manipulation of the AUTOIDLE bits.  Returns
+ * -EINVAL if @oh is null, or passes along the return value from
+ * _set_module_autoidle().
+ */
+int omap_hwmod_set_module_autoidle(struct omap_hwmod *oh, u8 autoidle)
+{
+	u32 v;
+	int retval = 0;
+
+	if (!oh)
+		return -EINVAL;
+
+	v = oh->_sysc_cache;
+
+	retval = _set_module_autoidle(oh, autoidle, &v);
+
+	if (!retval)
+		_write_sysconfig(v, oh);
+
+	return retval;
+}
+
+/**
  * _shutdown - shutdown an omap_hwmod
  * @oh: struct omap_hwmod *
  *
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 6adbb63..7042b86 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -526,6 +526,7 @@ int omap_hwmod_hardreset_deassert(struct omap_hwmod *oh, const char *name);
 int omap_hwmod_hardreset_state(struct omap_hwmod *oh, const char *name);
 
 int omap_hwmod_set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode);
+int omap_hwmod_set_module_autoidle(struct omap_hwmod *oh, u8 autoidle)
 
 int omap_hwmod_reset(struct omap_hwmod *oh);
 void omap_hwmod_ocp_barrier(struct omap_hwmod *oh);
-- 
1.7.0.4

Re: [RFC: PATCH] New API to modify the autoidle bits of sysconfig register

[Kevin Hilman]

Kishon Vijay Abraham I <kishon@ti.com> writes:

> Though the sysconfig register values shouldn't be modified directly by
> the driver, MCBSP should be considered a special case where sysconfig
> registers need to be modified dynamically by the driver.
>
> For e.g MCBSP 2 and 3 in OMAP3 has sidetone feature which requires
> autoidle to be disabled before starting the sidetone.
>
> This patch creates a new API that forms a wrapper to
> _set_module_autoidle() to modify the AUTOIDLE bit.

Since driver should not be using hwmod directly, how is this API going
to be used?  Will this only be called from arch/arm/mach-omap2/mcbsp.c?

> 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: Paul Walmsley<paul@pwsan.com>
> Cc: Benoit Cousson<b-cousson@ti.com>
> Cc: Partha Basak <p-basak2@ti.com>

Subject should contain prefix like 'OMAP: hwmod: ...'

Kevin

> ---
>  arch/arm/mach-omap2/omap_hwmod.c             |   29 ++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 9bd99ad..0d38404 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -969,6 +969,35 @@ int _omap_hwmod_idle(struct omap_hwmod *oh)
>  }
>  
>  /**
> + * omap_hwmod_set_module_autoidle - set the hwmod's OCP slave autoidle
> + * @oh: struct omap_hwmod *
> + * @autoidle: desired AUTOIDLE bitfield value (0 or 1)
> + *
> + * Sets the IP block's OCP slave autoidle in hardware, and updates our
> + * local copy. Intended to be used by drivers that have some erratum
> + * that requires direct manipulation of the AUTOIDLE bits.  Returns
> + * -EINVAL if @oh is null, or passes along the return value from
> + * _set_module_autoidle().
> + */
> +int omap_hwmod_set_module_autoidle(struct omap_hwmod *oh, u8 autoidle)
> +{
> +	u32 v;
> +	int retval = 0;
> +
> +	if (!oh)
> +		return -EINVAL;
> +
> +	v = oh->_sysc_cache;
> +
> +	retval = _set_module_autoidle(oh, autoidle, &v);
> +
> +	if (!retval)
> +		_write_sysconfig(v, oh);
> +
> +	return retval;
> +}
> +
> +/**
>   * _shutdown - shutdown an omap_hwmod
>   * @oh: struct omap_hwmod *
>   *
> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> index 6adbb63..7042b86 100644
> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> @@ -526,6 +526,7 @@ int omap_hwmod_hardreset_deassert(struct omap_hwmod *oh, const char *name);
>  int omap_hwmod_hardreset_state(struct omap_hwmod *oh, const char *name);
>  
>  int omap_hwmod_set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode);
> +int omap_hwmod_set_module_autoidle(struct omap_hwmod *oh, u8 autoidle)
>  
>  int omap_hwmod_reset(struct omap_hwmod *oh);
>  void omap_hwmod_ocp_barrier(struct omap_hwmod *oh);

Re: [RFC: PATCH] OMAP: hwmod: New API to modify the autoidle bits of sysconfig register

[kishon]

On Monday 30 August 2010 08:56 PM, Kevin Hilman wrote:
> Kishon Vijay Abraham I<kishon@ti.com>  writes:
>
>    
>> Though the sysconfig register values shouldn't be modified directly by
>> the driver, MCBSP should be considered a special case where sysconfig
>> registers need to be modified dynamically by the driver.
>>
>> For e.g MCBSP 2 and 3 in OMAP3 has sidetone feature which requires
>> autoidle to be disabled before starting the sidetone.
>>
>> This patch creates a new API that forms a wrapper to
>> _set_module_autoidle() to modify the AUTOIDLE bit.
>>      
> Since driver should not be using hwmod directly, how is this API going
> to be used?  Will this only be called from arch/arm/mach-omap2/mcbsp.c?
>    
     Though driver shouldn't be using hwmod directly, there is no 
corresponding API in omap_device to do the same. So we are planning to
     store the omap_hwmod structure in platform_data during 
arch_initcall (in the callback to omap_hwmod_for_each_by_class). So 
whenever the
     AUTOIDLE bits need to be reset or set, we pass the stored 
omap_hwmod structure to this API. Currently, the functions that needs 
AUTOIDLE
     bit to be modified (omap_st_on, omap_st_off) resides in plat-omap.

     -Kishon
>    
>> 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: Paul Walmsley<paul@pwsan.com>
>> Cc: Benoit Cousson<b-cousson@ti.com>
>> Cc: Partha Basak<p-basak2@ti.com>
>>      
> Subject should contain prefix like 'OMAP: hwmod: ...'
>
> Kevin
>
>    
>> ---
>>   arch/arm/mach-omap2/omap_hwmod.c             |   29 ++++++++++++++++++++++++++
>>   arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
>>   2 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>> index 9bd99ad..0d38404 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -969,6 +969,35 @@ int _omap_hwmod_idle(struct omap_hwmod *oh)
>>   }
>>
>>   /**
>> + * omap_hwmod_set_module_autoidle - set the hwmod's OCP slave autoidle
>> + * @oh: struct omap_hwmod *
>> + * @autoidle: desired AUTOIDLE bitfield value (0 or 1)
>> + *
>> + * Sets the IP block's OCP slave autoidle in hardware, and updates our
>> + * local copy. Intended to be used by drivers that have some erratum
>> + * that requires direct manipulation of the AUTOIDLE bits.  Returns
>> + * -EINVAL if @oh is null, or passes along the return value from
>> + * _set_module_autoidle().
>> + */
>> +int omap_hwmod_set_module_autoidle(struct omap_hwmod *oh, u8 autoidle)
>> +{
>> +	u32 v;
>> +	int retval = 0;
>> +
>> +	if (!oh)
>> +		return -EINVAL;
>> +
>> +	v = oh->_sysc_cache;
>> +
>> +	retval = _set_module_autoidle(oh, autoidle,&v);
>> +
>> +	if (!retval)
>> +		_write_sysconfig(v, oh);
>> +
>> +	return retval;
>> +}
>> +
>> +/**
>>    * _shutdown - shutdown an omap_hwmod
>>    * @oh: struct omap_hwmod *
>>    *
>> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> index 6adbb63..7042b86 100644
>> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
>> @@ -526,6 +526,7 @@ int omap_hwmod_hardreset_deassert(struct omap_hwmod *oh, const char *name);
>>   int omap_hwmod_hardreset_state(struct omap_hwmod *oh, const char *name);
>>
>>   int omap_hwmod_set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode);
>> +int omap_hwmod_set_module_autoidle(struct omap_hwmod *oh, u8 autoidle);
>>
>>   int omap_hwmod_reset(struct omap_hwmod *oh);
>>   void omap_hwmod_ocp_barrier(struct omap_hwmod *oh);
>>      

Re: [RFC: PATCH] OMAP: hwmod: New API to modify the autoidle bits of sysconfig register

[Felipe Balbi]

On Tue, 31 Aug 2010 10:53:36 +0530, kishon <a0393678@ti.com> wrote:
>      Though driver shouldn't be using hwmod directly, there is no 
> corresponding API in omap_device to do the same. So we are planning to
>      store the omap_hwmod structure in platform_data during 
> arch_initcall (in the callback to omap_hwmod_for_each_by_class). So 
> whenever the
>      AUTOIDLE bits need to be reset or set, we pass the stored 
> omap_hwmod structure to this API. Currently, the functions that needs 
> AUTOIDLE
>      bit to be modified (omap_st_on, omap_st_off) resides in plat-omap.

couldn't your API instead be something like:

int omap_hwmod_set_autoidle(struct device *dev, u8 autoidle)
{
	struct omap_hwmod *oh = dev_to_hwmod(dev);

	if (!oh)
		return -ENODEV;
	[...]

	return 0;
}

-- 
balbi

Re: [RFC: PATCH] OMAP: hwmod: New API to modify the autoidle bits of sysconfig register

[kishon]

On Tuesday 31 August 2010 01:43 PM, Felipe Balbi wrote:
> On Tue, 31 Aug 2010 10:53:36 +0530, kishon<a0393678@ti.com>  wrote:
>    
>>       Though driver shouldn't be using hwmod directly, there is no
>> corresponding API in omap_device to do the same. So we are planning to
>>       store the omap_hwmod structure in platform_data during
>> arch_initcall (in the callback to omap_hwmod_for_each_by_class). So
>> whenever the
>>       AUTOIDLE bits need to be reset or set, we pass the stored
>> omap_hwmod structure to this API. Currently, the functions that needs
>> AUTOIDLE
>>       bit to be modified (omap_st_on, omap_st_off) resides in plat-omap.
>>      
> couldn't your API instead be something like:
>
> int omap_hwmod_set_autoidle(struct device *dev, u8 autoidle)
> {
> 	struct omap_hwmod *oh = dev_to_hwmod(dev);
>
> 	if (!oh)
> 		return -ENODEV;
> 	[...]
>
> 	return 0;
> }
>    
     Balbi,
         I couldn't find "dev_to_hwmod()" function. Do you actually mean 
implementing dev_to_hwmod() function?
     I created this API based on "omap_hwmod_set_slave_idlemode()" 
present in lo (omap_hwmod.c) for changing the smart idle bit
     of SYSCONFIG register.

     -Kishon

Re: [RFC: PATCH] OMAP: hwmod: New API to modify the autoidle bits of sysconfig register

[Cousson, Benoit]

On 8/31/2010 4:41 PM, ABRAHAM, KISHON VIJAY wrote:
>
>
> On Tuesday 31 August 2010 01:43 PM, Felipe Balbi wrote:
>> On Tue, 31 Aug 2010 10:53:36 +0530, kishon<a0393678@ti.com>   wrote:
>>
>>>        Though driver shouldn't be using hwmod directly, there is no
>>> corresponding API in omap_device to do the same. So we are planning to
>>>        store the omap_hwmod structure in platform_data during
>>> arch_initcall (in the callback to omap_hwmod_for_each_by_class). So
>>> whenever the
>>>        AUTOIDLE bits need to be reset or set, we pass the stored
>>> omap_hwmod structure to this API. Currently, the functions that needs
>>> AUTOIDLE
>>>        bit to be modified (omap_st_on, omap_st_off) resides in plat-omap.
>>>
>> couldn't your API instead be something like:
>>
>> int omap_hwmod_set_autoidle(struct device *dev, u8 autoidle)
>> {
>> 	struct omap_hwmod *oh = dev_to_hwmod(dev);
>>
>> 	if (!oh)
>> 		return -ENODEV;
>> 	[...]
>>
>> 	return 0;
>> }
>>
>       Balbi,
>           I couldn't find "dev_to_hwmod()" function. Do you actually mean
> implementing dev_to_hwmod() function?
>       I created this API based on "omap_hwmod_set_slave_idlemode()"
> present in lo (omap_hwmod.c) for changing the smart idle bit
>       of SYSCONFIG register.
>
>       -Kishon

That API does not exist. And since a device can contains several hwmods, 
like in your case, this API cannot exist in that form.

The real issue is that we do not want to expose any API relative to PRCM 
management to the driver. Even if you hide that behind some pdata, it is 
still the driver that will call it.

I know that some IPs like MUSB or McBSP requires some unusual idle mode 
change due to HW bugs, but in general, you should try to find a better 
way to handle that in the core framework if possible.

Regards,
Benoit

Re: [RFC: PATCH] OMAP: hwmod: New API to modify the autoidle bits of sysconfig register

[kishon]

On Tuesday 31 August 2010 09:46 PM, Cousson, Benoit wrote:
> On 8/31/2010 4:41 PM, ABRAHAM, KISHON VIJAY wrote:
>    
>>
>> On Tuesday 31 August 2010 01:43 PM, Felipe Balbi wrote:
>>      
>>> On Tue, 31 Aug 2010 10:53:36 +0530, kishon<a0393678@ti.com>    wrote:
>>>
>>>        
>>>>         Though driver shouldn't be using hwmod directly, there is no
>>>> corresponding API in omap_device to do the same. So we are planning to
>>>>         store the omap_hwmod structure in platform_data during
>>>> arch_initcall (in the callback to omap_hwmod_for_each_by_class). So
>>>> whenever the
>>>>         AUTOIDLE bits need to be reset or set, we pass the stored
>>>> omap_hwmod structure to this API. Currently, the functions that needs
>>>> AUTOIDLE
>>>>         bit to be modified (omap_st_on, omap_st_off) resides in plat-omap.
>>>>
>>>>          
>>> couldn't your API instead be something like:
>>>
>>> int omap_hwmod_set_autoidle(struct device *dev, u8 autoidle)
>>> {
>>> 	struct omap_hwmod *oh = dev_to_hwmod(dev);
>>>
>>> 	if (!oh)
>>> 		return -ENODEV;
>>> 	[...]
>>>
>>> 	return 0;
>>> }
>>>
>>>        
>>        Balbi,
>>            I couldn't find "dev_to_hwmod()" function. Do you actually mean
>> implementing dev_to_hwmod() function?
>>        I created this API based on "omap_hwmod_set_slave_idlemode()"
>> present in lo (omap_hwmod.c) for changing the smart idle bit
>>        of SYSCONFIG register.
>>
>>        -Kishon
>>      
> That API does not exist. And since a device can contains several hwmods,
> like in your case, this API cannot exist in that form.
>
> The real issue is that we do not want to expose any API relative to PRCM
> management to the driver. Even if you hide that behind some pdata, it is
> still the driver that will call it.
>
> I know that some IPs like MUSB or McBSP requires some unusual idle mode
> change due to HW bugs, but in general, you should try to find a better
> way to handle that in the core framework if possible.
>
>    
     Benoit,

     I see the trend of using specific flags in hwmod which gets set
     in pm_runtime_get_sync() for scenarios like this where auto idle
     bits or smart idle bits need to be set/reset at runtime. The problem
     in the case of sidetone in MCBSP is that we don't call get_sync or
     put_sync for sidetone separately.
     There is only two options we can think of for sidetone
         1) Always disable autoidle for sidetone (I'm not sure if this will
     have some power implications).
         2) Use of the above API.

     Would be helpful if you can also suggest us some other options as well.

     -Kishon
> Regards,
> Benoit
>