Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On 28/07/15 12:46, Sifan Naeem wrote:
> Hi James,
> 
>> -----Original Message-----
>> From: James Hogan
>> Sent: 28 July 2015 12:36
>> To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org
>> Cc: Stable kernel (v3.19+)
>> Subject: Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset
>>
>> On 27/07/15 12:47, Sifan Naeem wrote:
>>> Reset interrupt enable register and clear any generated interrupts to
>>> make sure of a clean slate after a soft reset. Not doing so might
>>> leave unhandle line status or generated interrupts which can cause
>>> issues when handling new transfers.
>>
>> That already happens after the call to img_i2c_soft_reset, you've just moved
>> it to while the clock is off and the block is in soft reset. So are you saying its
>> important for interrupts and the various event bits to be clear before taking it
>> out of reset and enabling the clock? TBH I'm confused how they could do any
>> harm.
>>
> In " [PATCH 7/8] i2c: img-scb: improve transaction complete handle "
> img_i2c_soft_reset is added to img_i2c_complete_transaction, this will
> be useful in this scenario, not necessarily for when initialising the
> block.

Okay, in that case you should clarify in the commit message that it is
in preparation for later patches, rather than fixing an existing problem.

Cheers
James

> 
> Thanks
> Sifan
>> Cheers
>> James
>>
>>>
>>> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
>>> driver")
>>> Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
>>> Cc: Stable kernel (v3.19+) <stable@vger.kernel.org>
>>> ---
>>>  drivers/i2c/busses/i2c-img-scb.c |   26 ++++++++++++++------------
>>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-img-scb.c
>>> b/drivers/i2c/busses/i2c-img-scb.c
>>> index 51a5be8..653f9bd 100644
>>> --- a/drivers/i2c/busses/i2c-img-scb.c
>>> +++ b/drivers/i2c/busses/i2c-img-scb.c
>>> @@ -507,8 +507,22 @@ static void img_i2c_soft_reset(struct img_i2c
>>> *i2c)  {
>>>  	i2c->t_halt = false;
>>>  	img_i2c_writel(i2c, SCB_CONTROL_REG, 0);
>>> +
>>> +	/* Disable all interrupts */
>>> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
>>> +
>>> +	/* Clear all interrupts */
>>> +	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
>>> +
>>> +	/* Clear the scb_line_status events */
>>> +	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>>> +
>>>  	img_i2c_writel(i2c, SCB_CONTROL_REG,
>>>  		       SCB_CONTROL_CLK_ENABLE |
>> SCB_CONTROL_SOFT_RESET);
>>> +
>>> +	/* Enable interrupts */
>>> +	img_i2c_switch_mode(i2c, MODE_INACTIVE);
>>> +	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>>>  }
>>>
>>>  /* enable or release transaction halt for control of repeated starts
>>> */ @@ -1242,18 +1256,6 @@ static int img_i2c_init(struct img_i2c *i2c)
>>>  	/* Take module out of soft reset and enable clocks */
>>>  	img_i2c_soft_reset(i2c);
>>>
>>> -	/* Disable all interrupts */
>>> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, 0);
>>> -
>>> -	/* Clear all interrupts */
>>> -	img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
>>> -
>>> -	/* Clear the scb_line_status events */
>>> -	img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>>> -
>>> -	/* Enable interrupts */
>>> -	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>>> -
>>>  	/* Perform a synchronous sequence to reset the bus */
>>>  	ret = img_i2c_reset_bus(i2c);
>>>
>>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]