[PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Zubair Lutfullah]

Reg_cache variable is used to lock step enable register
from being accessed and written by both TSC and ADC
at the same time.
However, it isn't updated anywhere in the code at all.

If both TSC and ADC are used, eventually 1FFFF is always
written enabling all 16 steps uselessly causing a mess.

Patch fixes it by correcting the locks and updates the
variable by reading the step enable register

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/mfd/ti_am335x_tscadc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index cd74d59..9f3f07a 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -57,10 +57,10 @@ EXPORT_SYMBOL_GPL(am335x_tsc_se_update);
 void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
 {
 	spin_lock(&tsadc->reg_lock);
+	tsadc->reg_se_cache = tscadc_readl(tsadc, REG_SE);
 	tsadc->reg_se_cache |= val;
-	spin_unlock(&tsadc->reg_lock);
-
 	am335x_tsc_se_update(tsadc);
+	spin_unlock(&tsadc->reg_lock);
 }
 EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
 
-- 
1.7.9.5

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Lee Jones]

On Mon, 05 Aug 2013, Zubair Lutfullah wrote:

> Reg_cache variable is used to lock step enable register
> from being accessed and written by both TSC and ADC
> at the same time.
> However, it isn't updated anywhere in the code at all.
> 
> If both TSC and ADC are used, eventually 1FFFF is always
> written enabling all 16 steps uselessly causing a mess.
> 
> Patch fixes it by correcting the locks and updates the
> variable by reading the step enable register
> 
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Better that it comes from somewhere.

Applied, thanks.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Sebastian Andrzej Siewior]

On 08/07/2013 10:40 AM, Lee Jones wrote:
> On Mon, 05 Aug 2013, Zubair Lutfullah wrote:
> 
>> Reg_cache variable is used to lock step enable register
>> from being accessed and written by both TSC and ADC
>> at the same time.
>> However, it isn't updated anywhere in the code at all.
>>
>> If both TSC and ADC are used, eventually 1FFFF is always
>> written enabling all 16 steps uselessly causing a mess.
>>
>> Patch fixes it by correcting the locks and updates the
>> variable by reading the step enable register
>>
>> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
>> ---
>>  drivers/mfd/ti_am335x_tscadc.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Better that it comes from somewhere.

I don't understand. All three functions are used before the patch has
been applied:

$ git grep -l am335x_tsc_se_set
drivers/iio/adc/ti_am335x_adc.c
drivers/input/touchscreen/ti_am335x_tsc.c
drivers/mfd/ti_am335x_tscadc.c

$ git grep -l am335x_tsc_se_clr
drivers/iio/adc/ti_am335x_adc.c
drivers/input/touchscreen/ti_am335x_tsc.c
drivers/mfd/ti_am335x_tscadc.c

$ git grep -l am335x_tsc_se_update
drivers/iio/adc/ti_am335x_adc.c
drivers/input/touchscreen/ti_am335x_tsc.c
drivers/mfd/ti_am335x_tscadc.c
include/linux/mfd/ti_am335x_tscadc.h

It has been initialized to 0 by time the mfd part was loaded and
updated via …_set() from both parts (TSC & ADC). The lock ensured that
we never lose or add bits due to a race. So I don't understand why we
end up with 0x1FFFF.
Could some please explain to me how this can happen?

I added reg_se_cache to cache the content of REG_SE once and
synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
after "work" has been done. So you need to know the old value or TSC may
disable ADC and the other way around.

In tree (staging-next) I see that reg_se_cache ended being pointless.
am335x_tsc_se_update() is no longer used from TSC or ADC. Only the
_set() and _clr() functions are used which (both) read back the content
of the REG_SE register before calling am335x_tsc_se_update().

That makes me think that we might cut of one part by accident. On the
other hand Zubair said that he tested using ADC & TSC at the same time
and it worked. So I have to double check if the HW really resets the
content back to zero or not; maybe there is another explanation :)

One thing that is an issue is that now the _set() function is using the
lock without disabling interrupts and is called from non-IRQ
(tiadc_read_raw()) and IRQ (titsc_irq()) context which might lead to
deadlock. I'm going to send a patch for this.

> Applied, thanks.

Sebastian

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Lee Jones]

On Tue, 22 Oct 2013, Sebastian Andrzej Siewior wrote:

> On 08/07/2013 10:40 AM, Lee Jones wrote:
> > On Mon, 05 Aug 2013, Zubair Lutfullah wrote:
> > 
> >> Reg_cache variable is used to lock step enable register
> >> from being accessed and written by both TSC and ADC
> >> at the same time.
> >> However, it isn't updated anywhere in the code at all.
> >>
> >> If both TSC and ADC are used, eventually 1FFFF is always
> >> written enabling all 16 steps uselessly causing a mess.
> >>
> >> Patch fixes it by correcting the locks and updates the
> >> variable by reading the step enable register
> >>
> >> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> >> ---
> >>  drivers/mfd/ti_am335x_tscadc.c |    4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Better that it comes from somewhere.
> 
> I don't understand. All three functions are used before the patch has
> been applied:
> 
> $ git grep -l am335x_tsc_se_set
> drivers/iio/adc/ti_am335x_adc.c
> drivers/input/touchscreen/ti_am335x_tsc.c
> drivers/mfd/ti_am335x_tscadc.c
> 
> $ git grep -l am335x_tsc_se_clr
> drivers/iio/adc/ti_am335x_adc.c
> drivers/input/touchscreen/ti_am335x_tsc.c
> drivers/mfd/ti_am335x_tscadc.c
> 
> $ git grep -l am335x_tsc_se_update
> drivers/iio/adc/ti_am335x_adc.c
> drivers/input/touchscreen/ti_am335x_tsc.c
> drivers/mfd/ti_am335x_tscadc.c
> include/linux/mfd/ti_am335x_tscadc.h

Okay. So what does this mean?

> It has been initialized to 0 by time the mfd part was loaded and
> updated via …_set() from both parts (TSC & ADC). 
> The lock ensured that
> we never lose or add bits due to a race. So I don't understand why we
> end up with 0x1FFFF.
> Could some please explain to me how this can happen?

I don't have any h/w to actually test this, so you two are going to
have to fudge this out by yourselves.

> I added reg_se_cache to cache the content of REG_SE once and
> synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
> after "work" has been done. So you need to know the old value or TSC may
> disable ADC and the other way around.
> 
> In tree (staging-next) I see that reg_se_cache ended being pointless.
> am335x_tsc_se_update() is no longer used from TSC or ADC. Only the
> _set() and _clr() functions are used which (both) read back the content
> of the REG_SE register before calling am335x_tsc_se_update().

Not sure I get this point.

> That makes me think that we might cut of one part by accident. On the
> other hand Zubair said that he tested using ADC & TSC at the same time
> and it worked. So I have to double check if the HW really resets the
> content back to zero or not; maybe there is another explanation :)
> 
> One thing that is an issue is that now the _set() function is using the
> lock without disabling interrupts and is called from non-IRQ
> (tiadc_read_raw()) and IRQ (titsc_irq()) context which might lead to
> deadlock. I'm going to send a patch for this.

I see the patch, but let's sort this out first, before I apply it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Sebastian Andrzej Siewior]

On 10/22/2013 05:48 PM, Lee Jones wrote:
> On Tue, 22 Oct 2013, Sebastian Andrzej Siewior wrote:
> 
>> On 08/07/2013 10:40 AM, Lee Jones wrote:
>>> On Mon, 05 Aug 2013, Zubair Lutfullah wrote:
>>>
>>>> Reg_cache variable is used to lock step enable register
>>>> from being accessed and written by both TSC and ADC
>>>> at the same time.
>>>> However, it isn't updated anywhere in the code at all.
>>>>
>>>> If both TSC and ADC are used, eventually 1FFFF is always
>>>> written enabling all 16 steps uselessly causing a mess.
>>>>
>>>> Patch fixes it by correcting the locks and updates the
>>>> variable by reading the step enable register
>>>>
>>>> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
>>>> ---
>>>>  drivers/mfd/ti_am335x_tscadc.c |    4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Better that it comes from somewhere.
>>
>> I don't understand. All three functions are used before the patch has
>> been applied:
>>
>> $ git grep -l am335x_tsc_se_set
>> drivers/iio/adc/ti_am335x_adc.c
>> drivers/input/touchscreen/ti_am335x_tsc.c
>> drivers/mfd/ti_am335x_tscadc.c
>>
>> $ git grep -l am335x_tsc_se_clr
>> drivers/iio/adc/ti_am335x_adc.c
>> drivers/input/touchscreen/ti_am335x_tsc.c
>> drivers/mfd/ti_am335x_tscadc.c
>>
>> $ git grep -l am335x_tsc_se_update
>> drivers/iio/adc/ti_am335x_adc.c
>> drivers/input/touchscreen/ti_am335x_tsc.c
>> drivers/mfd/ti_am335x_tscadc.c
>> include/linux/mfd/ti_am335x_tscadc.h
> 
> Okay. So what does this mean?

That means I don't understand the commit message. It says
"However, it isn't updated anywhere in the code at all." but as you see
all three functions (set, update) are used. You said "Better that it
comes from somewhere" so I assumed since two people were looking at
this I forgot something.

>> It has been initialized to 0 by time the mfd part was loaded and
>> updated via …_set() from both parts (TSC & ADC). 
>> The lock ensured that
>> we never lose or add bits due to a race. So I don't understand why we
>> end up with 0x1FFFF.
>> Could some please explain to me how this can happen?
> 
> I don't have any h/w to actually test this, so you two are going to
> have to fudge this out by yourselves.

This wasn't directed to you :)

> 
>> I added reg_se_cache to cache the content of REG_SE once and
>> synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
>> after "work" has been done. So you need to know the old value or TSC may
>> disable ADC and the other way around.
>>
>> In tree (staging-next) I see that reg_se_cache ended being pointless.
>> am335x_tsc_se_update() is no longer used from TSC or ADC. Only the
>> _set() and _clr() functions are used which (both) read back the content
>> of the REG_SE register before calling am335x_tsc_se_update().
> 
> Not sure I get this point.

The point is that reg_se_cache is (under the lock) set to the value
read from the HW, ORed by the value which is passed as an argument and
then written back to HW. This makes the reg_se_cache in the struct
pointless and a local variable would do the same job.

> 
>> That makes me think that we might cut of one part by accident. On the
>> other hand Zubair said that he tested using ADC & TSC at the same time
>> and it worked. So I have to double check if the HW really resets the
>> content back to zero or not; maybe there is another explanation :)
>>
>> One thing that is an issue is that now the _set() function is using the
>> lock without disabling interrupts and is called from non-IRQ
>> (tiadc_read_raw()) and IRQ (titsc_irq()) context which might lead to
>> deadlock. I'm going to send a patch for this.
> 
> I see the patch, but let's sort this out first, before I apply it.

Please apply the patch to fix the possible deadlock situation which we
will have in the next merge window. I didn't revert or made any other
changes just have this sorted out in time while the deadlock is gone.

Sebastian

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Zubair Lutfullah :]

On Tue, Oct 22, 2013 at 06:28:13PM +0200, Sebastian Andrzej Siewior wrote:
> On 10/22/2013 05:48 PM, Lee Jones wrote:
> > On Tue, 22 Oct 2013, Sebastian Andrzej Siewior wrote:
> > 
> >> On 08/07/2013 10:40 AM, Lee Jones wrote:
> >>> On Mon, 05 Aug 2013, Zubair Lutfullah wrote:
> >>>
> >>>> Reg_cache variable is used to lock step enable register
> >>>> from being accessed and written by both TSC and ADC
> >>>> at the same time.
> >>>> However, it isn't updated anywhere in the code at all.
> >>>>
> >>>> If both TSC and ADC are used, eventually 1FFFF is always
> >>>> written enabling all 16 steps uselessly causing a mess.
> >>>>
> >>>> Patch fixes it by correcting the locks and updates the
> >>>> variable by reading the step enable register
> >>>>
> >>>> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> >>>> ---
> >>>>  drivers/mfd/ti_am335x_tscadc.c |    4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> Better that it comes from somewhere.
> >>
> That means I don't understand the commit message. It says
> "However, it isn't updated anywhere in the code at all." but as you see
> all three functions (set, update) are used. You said "Better that it
> comes from somewhere" so I assumed since two people were looking at
> this I forgot something.
> 
> >> It has been initialized to 0 by time the mfd part was loaded and
> >> updated via …_set() from both parts (TSC & ADC). 
> >> The lock ensured that
> >> we never lose or add bits due to a race. So I don't understand why we
> >> end up with 0x1FFFF.
> >> Could some please explain to me how this can happen?

Let me elaborate this.

If I enable TSC(4 wire) and ADC Channel 1 and 2 (out of channels 0,1,2,3).

In the previous code, I would get reg_se_cache variable as 0x1FFF6. Two bits zero
as channel 0 and 3 are disabled. And the rest of the steps enabled for TSC.

Now I disable ADC channel 1 and 2. And enable ADC channel 0 and 3.
In the previous code, I would then get reg_se_cache variable as 0x1FFFF. There 
was no code to zero the bits of the disabled channels.

am335x_tsc_se_set was used effectively.
but am335x_tsc_se_clr was only used in the drivers _remove function.

Over time, the variable reg_se_cache would become 0x1FFFF.

Enabled steps in REG_SE become 0 in ADC single shot mode.
Enabled steps in REG_SE become 0 in TSC events as well.

But in continuous sampling mode for ADC,  REG_SE steps
for those channels remain enabled.

This required the patches.
REG_SE is read in am335x_tsc_se_set so that the enabled steps of the ADC 
are read when the TSC is enabling its steps after an event.

> >> I added reg_se_cache to cache the content of REG_SE once and
> >> synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
> >> after "work" has been done. So you need to know the old value or TSC may
> >> disable ADC and the other way around.
> >>
> >> In tree (staging-next) I see that reg_se_cache ended being pointless.
> >> am335x_tsc_se_update() is no longer used from TSC or ADC. Only the
> >> _set() and _clr() functions are used which (both) read back the content
> >> of the REG_SE register before calling am335x_tsc_se_update().
> > 
> > Not sure I get this point.
> 
> The point is that reg_se_cache is (under the lock) set to the value
> read from the HW, ORed by the value which is passed as an argument and
> then written back to HW. This makes the reg_se_cache in the struct
> pointless and a local variable would do the same job.

Haven't looked at the code again. But if I remember correctly,
This makes sense. There is room for optimization and reg_se_cache
is useless now?

> 
> > 
> >> That makes me think that we might cut of one part by accident. On the
> >> other hand Zubair said that he tested using ADC & TSC at the same time
> >> and it worked. So I have to double check if the HW really resets the
> >> content back to zero or not; maybe there is another explanation :)
> >>

I'm still in the wedding mode. But thought I'd reply to these queries.
The HW worked when I tested it..

> >> One thing that is an issue is that now the _set() function is using the
> >> lock without disabling interrupts and is called from non-IRQ
> >> (tiadc_read_raw()) and IRQ (titsc_irq()) context which might lead to
> >> deadlock. I'm going to send a patch for this.
> > 
> > I see the patch, but let's sort this out first, before I apply it.
> 
> Please apply the patch to fix the possible deadlock situation which we
> will have in the next merge window. I didn't revert or made any other
> changes just have this sorted out in time while the deadlock is gone.
> 

Noticed it and forgot. Sorry.

I hope this clears the confusion.
There is room for some optimization. The se_cache variable can be removed now
I think.

Zubair

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Lee Jones]

On Tue, 22 Oct 2013, Sebastian Andrzej Siewior wrote:

> On 08/07/2013 10:40 AM, Lee Jones wrote:
> > On Mon, 05 Aug 2013, Zubair Lutfullah wrote:
> > 
> >> Reg_cache variable is used to lock step enable register
> >> from being accessed and written by both TSC and ADC
> >> at the same time.
> >> However, it isn't updated anywhere in the code at all.
> >>
> >> If both TSC and ADC are used, eventually 1FFFF is always
> >> written enabling all 16 steps uselessly causing a mess.
> >>
> >> Patch fixes it by correcting the locks and updates the
> >> variable by reading the step enable register
> >>
> >> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> >> ---
> >>  drivers/mfd/ti_am335x_tscadc.c |    4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Better that it comes from somewhere.
> 
> I don't understand. All three functions are used before the patch has
> been applied:
> 
> $ git grep -l am335x_tsc_se_set
> drivers/iio/adc/ti_am335x_adc.c
> drivers/input/touchscreen/ti_am335x_tsc.c
> drivers/mfd/ti_am335x_tscadc.c
> 
> $ git grep -l am335x_tsc_se_clr
> drivers/iio/adc/ti_am335x_adc.c
> drivers/input/touchscreen/ti_am335x_tsc.c
> drivers/mfd/ti_am335x_tscadc.c
> 
> $ git grep -l am335x_tsc_se_update
> drivers/iio/adc/ti_am335x_adc.c
> drivers/input/touchscreen/ti_am335x_tsc.c
> drivers/mfd/ti_am335x_tscadc.c
> include/linux/mfd/ti_am335x_tscadc.h
> 
> It has been initialized to 0 by time the mfd part was loaded and
> updated via …_set() from both parts (TSC & ADC). The lock ensured that
> we never lose or add bits due to a race. So I don't understand why we
> end up with 0x1FFFF.
> Could some please explain to me how this can happen?
> 
> I added reg_se_cache to cache the content of REG_SE once and
> synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
> after "work" has been done. So you need to know the old value or TSC may
> disable ADC and the other way around.

Yep, it's initialised as '0'.

12.5.1.15 STEPENABLE Register (offset = 54h) [reset = 0h]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Sebastian Andrzej Siewior]

On 10/22/2013 06:05 PM, Lee Jones wrote:
>> I added reg_se_cache to cache the content of REG_SE once and
>> synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
>> after "work" has been done. So you need to know the old value or TSC may
>> disable ADC and the other way around.
> 
> Yep, it's initialised as '0'.
> 
> 12.5.1.15 STEPENABLE Register (offset = 54h) [reset = 0h]

Ehm yes but!. After init it is set to 0, correct. The value was never
read from the HW. It was always set via  am335x_tsc_se_set() to "cache
| argument" and written to HW from both sides (TSC, ADC). This
initialization is done at ->probe() time in both drivers.

The value remains (remained) constant over the whole time
so both drivers only called am335x_tsc_se_update() to set the value
(the enabled steps of both sides) back to the register (because after
the conversation the value was 0 according to my memory) and since
32bit reads are atomic I didn't use a lock here.

Sebastian

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Lee Jones]

On Tue, 22 Oct 2013, Sebastian Andrzej Siewior wrote:

> On 10/22/2013 06:05 PM, Lee Jones wrote:
> >> I added reg_se_cache to cache the content of REG_SE once and
> >> synchronize it among TSC & ADC access. REG_SE is set to 0 by the HW
> >> after "work" has been done. So you need to know the old value or TSC may
> >> disable ADC and the other way around.
> > 
> > Yep, it's initialised as '0'.
> > 
> > 12.5.1.15 STEPENABLE Register (offset = 54h) [reset = 0h]
> 
> Ehm yes but!. After init it is set to 0, correct. The value was never
> read from the HW. It was always set via  am335x_tsc_se_set() to "cache
> | argument" and written to HW from both sides (TSC, ADC). This
> initialization is done at ->probe() time in both drivers.
> 
> The value remains (remained) constant over the whole time
> so both drivers only called am335x_tsc_se_update() to set the value
> (the enabled steps of both sides) back to the register (because after
> the conversation the value was 0 according to my memory) and since
> 32bit reads are atomic I didn't use a lock here.

Hmm.. I'm starting to see what you mean.

So what's the point of the read before write then?

Why don't you use the cache all of the time?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Sebastian Andrzej Siewior]

On 10/22/2013 07:06 PM, Lee Jones wrote:
> Hmm.. I'm starting to see what you mean.
> 
> So what's the point of the read before write then?
> 
> Why don't you use the cache all of the time?

I'm waiting for Zabair for this. The last thing he said is that he is
going to have a wedding which might involve some off time but then this
was on the 7th this month.
Depending how long it takes I start digging myself after I finish some
other things.

Sebastian

Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache

[Lee Jones]

On Tue, 22 Oct 2013, Sebastian Andrzej Siewior wrote:

> On 10/22/2013 07:06 PM, Lee Jones wrote:
> > Hmm.. I'm starting to see what you mean.
> > 
> > So what's the point of the read before write then?
> > 
> > Why don't you use the cache all of the time?
> 
> I'm waiting for Zabair for this. The last thing he said is that he is
> going to have a wedding which might involve some off time but then this
> was on the 7th this month.
> Depending how long it takes I start digging myself after I finish some
> other things.

Sorry, I meant in am335x_tsc_se_cl().

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog