Re: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset
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
>>> Cc: Stable kernel (v3.19+)
>>> ---
>>> 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);
>>>
>>>
>
signature.asc
Description: OpenPGP digital signature
RE: [PATCH 5/8] i2c: img-scb: reset interrupts in img_i2c_soft_reset
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.
Thanks
Sifan
> Cheers
> James
>
> >
> > Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB
> > driver")
> > Signed-off-by: Sifan Naeem
> > Cc: Stable kernel (v3.19+)
> > ---
> > 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);
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Cheers
James
>
> Fixes: 27bce4 ("i2c: img-scb: Add Imagination Technologies I2C SCB driver")
> Signed-off-by: Sifan Naeem
> Cc: Stable kernel (v3.19+)
> ---
> 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);
>
>
signature.asc
Description: OpenPGP digital signature
