Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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 > --- > 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 0x1. > >> 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 0x1. 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 0x1. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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.lutful...@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 0x1. 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 0x1. 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 0x1. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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 --- 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 0x1. >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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 > >> --- > >> 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 0x1. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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 > >> --- > >> 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 0x1. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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 >> --- >> 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 0x1. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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.lutful...@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 0x1. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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.lutful...@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 0x1. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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.lutful...@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 0x1. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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.lutful...@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 0x1. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: ti_am335x_tscadc: fix spin lock and reg_cache
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 1 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.lutful...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
