diff mbox

[PATCHv3,8/8] i2c: omap: cleanup the sysc write

Message ID 1352118223-3796-9-git-send-email-shubhrajyoti@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shubhrajyoti Datta Nov. 5, 2012, 12:23 p.m. UTC
Currently after the reset the sysc is written with hardcoded values.
The patch reads the sysc register and writes back the same value
after reset.

- Some unnecessary rev checks can be optimised.
- Also due to whatever reason the hwmod flags are changed
we will not reset the values.
- In some of the cases the minor values of the 2430 register
is different(0x37) in that case the autoidle setting may be missed.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   20 +++++---------------
 1 files changed, 5 insertions(+), 15 deletions(-)

Comments

Felipe Balbi Nov. 5, 2012, 2:14 p.m. UTC | #1
Hi,

On Mon, Nov 05, 2012 at 05:53:43PM +0530, Shubhrajyoti D wrote:
> Currently after the reset the sysc is written with hardcoded values.
> The patch reads the sysc register and writes back the same value
> after reset.
> 
> - Some unnecessary rev checks can be optimised.
> - Also due to whatever reason the hwmod flags are changed
> we will not reset the values.
> - In some of the cases the minor values of the 2430 register
> is different(0x37) in that case the autoidle setting may be missed.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   20 +++++---------------
>  1 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 25f1564..a09acdc 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -302,7 +302,11 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
>  static int omap_i2c_reset(struct omap_i2c_dev *dev)
>  {
>  	unsigned long timeout;
> +	u16 sysc;
> +
>  	if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
> +		sysc = omap_i2c_read_reg(dev, OMAP_I2C_SYSC_REG);
> +
>  		/* Disable I2C controller before soft reset */
>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
>  			omap_i2c_read_reg(dev, OMAP_I2C_CON_REG) &
> @@ -324,22 +328,8 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>  		}
>  
>  		/* SYSC register is cleared by the reset; rewrite it */
> -		if (dev->rev == OMAP_I2C_REV_ON_2430) {
> -
> -			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> -					   SYSC_AUTOIDLE_MASK);
> +		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
>  
> -		} else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
> -			dev->syscstate = SYSC_AUTOIDLE_MASK;
> -			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
> -			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
> -			      __ffs(SYSC_SIDLEMODE_MASK));
> -			dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK <<
> -			      __ffs(SYSC_CLOCKACTIVITY_MASK));
> -
> -			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> -							dev->syscstate);
> -		}

not sure if this will work. What about the first time you call reset() ?
won't SYSC just contain the reset values ?
Shubhrajyoti Datta Nov. 5, 2012, 2:23 p.m. UTC | #2
On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote:
>> -							dev->syscstate);
>> > -		}
> not sure if this will work. What about the first time you call reset() ?
> won't SYSC just contain the reset values ?
No actually the hwmod sets the value.
Felipe Balbi Nov. 5, 2012, 2:25 p.m. UTC | #3
Hi,

On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote:
> On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote:
> >> -							dev->syscstate);
> >> > -		}
> > not sure if this will work. What about the first time you call reset() ?
> > won't SYSC just contain the reset values ?
> No actually the hwmod sets the value.

ahaaa, ok good. Let's get an Ack from Benoit, then.
Benoit Cousson Nov. 5, 2012, 5:24 p.m. UTC | #4
On 11/5/2012 3:25 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote:
>> On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote:
>>>> -							dev->syscstate);
>>>>> -		}
>>> not sure if this will work. What about the first time you call reset() ?
>>> won't SYSC just contain the reset values ?
>> No actually the hwmod sets the value.
>
> ahaaa, ok good. Let's get an Ack from Benoit, then.

Well, in fact, I'm a little bit surprised that we still have to hack the 
SYSC directly without using an omap_device API. I know that we have to 
do some weird stuff for reseting that IP, but didn't we already exposed 
something to allow that?

Regards,
Benoit
Shubhrajyoti Datta Nov. 5, 2012, 6:29 p.m. UTC | #5
On 11/5/12, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 11/5/2012 3:25 PM, Felipe Balbi wrote:
>> Hi,
>>
>> On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote:
>>> On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote:
>>>>> -							dev->syscstate);
>>>>>> -		}
>>>> not sure if this will work. What about the first time you call reset()
>>>> ?
>>>> won't SYSC just contain the reset values ?
>>> No actually the hwmod sets the value.
>>
>> ahaaa, ok good. Let's get an Ack from Benoit, then.
>
> Well, in fact, I'm a little bit surprised that we still have to hack the

there was an attempt [1]

the pdata stuff may have issues with dt having to deal with more pdata [2]


> SYSC directly without using an omap_device API.

 Paul was not happy with omap device api  [3]

As far as the patch is concerned it is only getting rid of the hard coded flags
and the rev check.

> I know that we have to
> do some weird stuff for reseting that IP, but didn't we already exposed
> something to allow that?
>
> Regards,
> Benoit

[1]  http://www.spinics.net/lists/linux-i2c/msg06810.html
[2]  http://www.spinics.net/lists/linux-i2c/msg06937.html
[3] http://www.spinics.net/lists/linux-i2c/msg06943.html
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 25f1564..a09acdc 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -302,7 +302,11 @@  static void __omap_i2c_init(struct omap_i2c_dev *dev)
 static int omap_i2c_reset(struct omap_i2c_dev *dev)
 {
 	unsigned long timeout;
+	u16 sysc;
+
 	if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
+		sysc = omap_i2c_read_reg(dev, OMAP_I2C_SYSC_REG);
+
 		/* Disable I2C controller before soft reset */
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
 			omap_i2c_read_reg(dev, OMAP_I2C_CON_REG) &
@@ -324,22 +328,8 @@  static int omap_i2c_reset(struct omap_i2c_dev *dev)
 		}
 
 		/* SYSC register is cleared by the reset; rewrite it */
-		if (dev->rev == OMAP_I2C_REV_ON_2430) {
-
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
-					   SYSC_AUTOIDLE_MASK);
+		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, sysc);
 
-		} else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
-			dev->syscstate = SYSC_AUTOIDLE_MASK;
-			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
-			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
-			      __ffs(SYSC_SIDLEMODE_MASK));
-			dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK <<
-			      __ffs(SYSC_CLOCKACTIVITY_MASK));
-
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
-							dev->syscstate);
-		}
 	}
 	return 0;
 }