| Message ID | 1415941651-28962-4-git-send-email-vigneshr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 14 Nov 2014, Vignesh R wrote: > In one shot mode, sequencer automatically disables all enabled steps at > the end of each cycle. (both ADC steps and TSC steps) Hence these steps > need not be saved in reg_se_cache for clearing these steps at a later > stage. > Also, when ADC wakes up Sequencer should not be busy executing any of the > config steps except for the charge step. Previously charge step was 1 ADC > clock cycle and hence it was ignored. > > Signed-off-by: Vignesh R <vigneshr@ti.com> > --- > drivers/mfd/ti_am335x_tscadc.c | 7 +++++-- > include/linux/mfd/ti_am335x_tscadc.h | 1 + > 2 files changed, 6 insertions(+), 2 deletions(-) Code looks better now. Acked-by: Lee Jones <lee.jones@linaro.org> What's the plan for this series? > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > index d877e777cce6..110038859a8d 100644 > --- a/drivers/mfd/ti_am335x_tscadc.c > +++ b/drivers/mfd/ti_am335x_tscadc.c > @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc) > spin_lock_irq(&tsadc->reg_lock); > finish_wait(&tsadc->reg_se_wait, &wait); > > + /* > + * Sequencer should either be idle or > + * busy applying the charge step. > + */ > reg = tscadc_readl(tsadc, REG_ADCFSM); > - WARN_ON(reg & SEQ_STATUS); > + WARN_ON((reg & SEQ_STATUS) && !(reg & CHARGE_STEP)); > tsadc->adc_waiting = false; > } > tsadc->adc_in_use = true; > @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc) > void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val) > { > spin_lock_irq(&tsadc->reg_lock); > - tsadc->reg_se_cache |= val; > am335x_tscadc_need_adc(tsadc); > > tscadc_writel(tsadc, REG_SE, val); > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h > index 3f4e994ace2b..1fd50dcfe47c 100644 > --- a/include/linux/mfd/ti_am335x_tscadc.h > +++ b/include/linux/mfd/ti_am335x_tscadc.h > @@ -128,6 +128,7 @@ > > /* Sequencer Status */ > #define SEQ_STATUS BIT(5) > +#define CHARGE_STEP 0x11 > > #define ADC_CLK 3000000 > #define TOTAL_STEPS 16
On Tue, Nov 18, 2014 at 02:30:05PM +0000, Lee Jones wrote: > On Fri, 14 Nov 2014, Vignesh R wrote: > > > In one shot mode, sequencer automatically disables all enabled steps at > > the end of each cycle. (both ADC steps and TSC steps) Hence these steps > > need not be saved in reg_se_cache for clearing these steps at a later > > stage. > > Also, when ADC wakes up Sequencer should not be busy executing any of the > > config steps except for the charge step. Previously charge step was 1 ADC > > clock cycle and hence it was ignored. > > > > Signed-off-by: Vignesh R <vigneshr@ti.com> > > --- > > drivers/mfd/ti_am335x_tscadc.c | 7 +++++-- > > include/linux/mfd/ti_am335x_tscadc.h | 1 + > > 2 files changed, 6 insertions(+), 2 deletions(-) > > Code looks better now. > > Acked-by: Lee Jones <lee.jones@linaro.org> > > What's the plan for this series? I am waiting for the interested parties to provide more feedback. So far I have seen a few reports saying that they see issues with the series applied.
On Tuesday 18 November 2014 10:42 PM, Dmitry Torokhov wrote: > On Tue, Nov 18, 2014 at 02:30:05PM +0000, Lee Jones wrote: >> On Fri, 14 Nov 2014, Vignesh R wrote: >> >>> In one shot mode, sequencer automatically disables all enabled steps at >>> the end of each cycle. (both ADC steps and TSC steps) Hence these steps >>> need not be saved in reg_se_cache for clearing these steps at a later >>> stage. >>> Also, when ADC wakes up Sequencer should not be busy executing any of the >>> config steps except for the charge step. Previously charge step was 1 ADC >>> clock cycle and hence it was ignored. >>> >>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>> --- >>> drivers/mfd/ti_am335x_tscadc.c | 7 +++++-- >>> include/linux/mfd/ti_am335x_tscadc.h | 1 + >>> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> Code looks better now. >> >> Acked-by: Lee Jones <lee.jones@linaro.org> >> >> What's the plan for this series? > > I am waiting for the interested parties to provide more feedback. So far > I have seen a few reports saying that they see issues with the series > applied. I tested this using lcd7 cape connected to beaglebone black. The latest kernel I could find on this board was a TI BSP based v3.14 kernel. So I had to port these patches to that kernel. Cc Robert Nelson to see if he knows about a more recent kernel supporting that cape. I did not see any breakage with these patches applied although I did not see any noticeable performance improvement as well. I also tested along with continuous loop reading from /sys/bus/iio/devices/iio:device0/in_voltage5_raw I have pushed the kernel I used here in case anyone wants to take a look at my porting. http://git.ti.com/cgit/cgit.cgi/~nsekhar/ti-linux-kernel/nsekhar-ti-linux-kernel.git/log/?h=am335x-tsc-test I also tested this series on AM335x EVM using the v3.18-rc5 kernel. Again, no breakage but no improvement as well. All testing was done using ts_test FWIW, you can add: Tested-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar
> -----Original Message----- > From: Nori, Sekhar > Sent: Thursday, November 20, 2014 7:56 AM > > I also tested this series on AM335x EVM using the v3.18-rc5 kernel. > Again, no breakage but no improvement as well. The primary goal was not necessarily to improve performance of the touchscreen itself. It was to reduce unnecessary CPU overhead introduced by the 275us udelay in the ISR. On a related note, that 275us udelay is not something that worked for all boards. For example, see the following forum thread: http://e2e.ti.com/support/arm/sitara_arm/f/791/p/217587/775152.aspx#775152 In that thread the user was registering multiple press events for a single press. By increasing the udelay to 1.5ms they were able to solve the problem. Of course, having a 1.5ms delay in your ISR is a really bad thing to do... I have another customer that was experiencing the same issue of registering multiple press events, and sure enough the 1.5ms delay "fixed" their problem too. The patches allowed them to remove that gigantic delay from the ISR because that software delay has now become a hardware delay via CHARGECONFIG. That customer is the one that needed 0xB000 which is MUCH larger than the value of 0x400 that was working fine for me on the EVM.
Brad, What you wrote is just the kind of thing one would like to see in the cover letter or change log... On Thu, Nov 20, 2014 at 02:23:30PM +0000, Griffis, Brad wrote: > In that thread the user was registering multiple press events for a single press. By increasing the udelay to 1.5ms they were able to solve the problem. Of course, having a 1.5ms delay in your ISR is a really bad thing to do... I fully support removing the aweful ISR delay, as long as the result works! > I have another customer that was experiencing the same issue of registering multiple press events, and sure enough the 1.5ms delay "fixed" their problem too. The patches allowed them to remove that gigantic delay from the ISR because that software delay has now become a hardware delay via CHARGECONFIG. That customer is the one that needed 0xB000 which is MUCH larger than the value of 0x400 that was working fine for me on the EVM. It would be really nice for TI to explain to board designers how to calculate the proper value. Thanks, Richard
On Thu, Nov 20, 2014 at 07:26:00PM +0530, Sekhar Nori wrote: > I tested this using lcd7 cape connected to beaglebone black. The latest > kernel I could find on this board was a TI BSP based v3.14 kernel. So I > had to port these patches to that kernel. Cc Robert Nelson to see if he > knows about a more recent kernel supporting that cape. What is missing from mainline for the black? (I thought it was fully supported by now.) > I did not see any breakage with these patches applied although I did not > see any noticeable performance improvement as well. So, the jumping on pen-up persists, right? That means the patch series does not provide a general fix for that issue. > I also tested this series on AM335x EVM using the v3.18-rc5 kernel. > Again, no breakage but no improvement as well. You still have jumps on a pen up event? Thanks, Richard
On Thursday 20 November 2014 08:10 PM, Richard Cochran wrote: > On Thu, Nov 20, 2014 at 07:26:00PM +0530, Sekhar Nori wrote: >> I tested this using lcd7 cape connected to beaglebone black. The latest >> kernel I could find on this board was a TI BSP based v3.14 kernel. So I >> had to port these patches to that kernel. Cc Robert Nelson to see if he >> knows about a more recent kernel supporting that cape. > > What is missing from mainline for the black? > > (I thought it was fully supported by now.) Black is supported, but LCD7 cape is not. > >> I did not see any breakage with these patches applied although I did not >> see any noticeable performance improvement as well. > > So, the jumping on pen-up persists, right? That means the patch > series does not provide a general fix for that issue. Not sure how to reproduce the jumping on pen-up. I was able to draw circles, sign my name and actually today I wrote up the whole English alphabet on the screen. Picture here. https://drive.google.com/file/d/0ByWLwvGx8MeyVF9CTlhHU0FHMFk/view?usp=sharing Is there any particular pattern you want me to test with? I would still like to test with a more recent kernel on LCD7 cape. So, if anyone has ideas (other than I porting the LCD7 support) please let me know. Thanks, Sekhar
On Fri, Nov 21, 2014 at 05:40:12PM +0530, Sekhar Nori wrote:
> Not sure how to reproduce the jumping on pen-up.
Does the cursor stay in exactly the same spot when you lift up the
stylus? Then you don't have the issue.
On the BB white using the LCD4 cape and the shipped debian kernel, the
cursor *does* jump away, but not as often or as far as on the custom
design I was working with.
Thanks,
Richard
I tested version 4 of the patch series on a custom design and I saw also the jumps as described by Richard. I played a little with the sample delay and it got better but I couldn't completely remove the jumps. The other issue I had with version 3, the pen_ups during the busy loop, is solved. I experienced some pen_up misses during the busy loop if I used a too high sample delay, without the sample delay it was ok. Thanks, Hannes 2014-11-21 14:10 GMT+01:00 Richard Cochran <richardcochran@gmail.com>: > On Fri, Nov 21, 2014 at 05:40:12PM +0530, Sekhar Nori wrote: >> Not sure how to reproduce the jumping on pen-up. > > Does the cursor stay in exactly the same spot when you lift up the > stylus? Then you don't have the issue. > > On the BB white using the LCD4 cape and the shipped debian kernel, the > cursor *does* jump away, but not as often or as far as on the custom > design I was working with. > > Thanks, > Richard > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 21 November 2014 08:41 PM, Johannes Pointner wrote: > I tested version 4 of the patch series on a custom design and I saw > also the jumps as described by Richard. I played a little with the > sample delay and it got better but I couldn't completely remove the > jumps. And before the patches were applied, there were no jumps at all? Which kernel are you using for testing? Thanks, Sekhar
Before the patches were also jumps but I thought it is something Vignesh should know. Maybe there is some fix for that too? As Richard also noted, it would be nice if ti could let us know how to get the delay values right. By trial and error is IMHO not the best way. For the testing I used 3.16.7. 2014-11-21 16:37 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: > On Friday 21 November 2014 08:41 PM, Johannes Pointner wrote: >> I tested version 4 of the patch series on a custom design and I saw >> also the jumps as described by Richard. I played a little with the >> sample delay and it got better but I couldn't completely remove the >> jumps. > > And before the patches were applied, there were no jumps at all? Which > kernel are you using for testing? > > Thanks, > Sekhar
On Fri, Nov 21, 2014 at 07:17:18PM +0100, Johannes Pointner wrote: > Before the patches were also jumps but I thought it is something > Vignesh should know. Maybe there is some fix for that too? > As Richard also noted, it would be nice if ti could let us know how to > get the delay values right. By trial and error is IMHO not the best > way. That is not only an opinion, it is a matter of fact. TI really dropped the ball on this one. I thought the patch series was a sign they finally were going to properly address this issue. Wrong again. The data sheet for the TI part used to have a reference to an app-note for the touch controller. spruh73f page 1154 The Pen IRQ can only occur if the correct Pen Ctrl bits are high (AFE Pen Ctrl bits 6:5 in the TSC_ADC control register) and if the correct ground transistor biasing is set in the StepConfig [N] Register. Refer to the application notes for the correct settings. I searched high and low for this application note. Then, the data sheet got revised. Thanks, Richard
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > > On Fri, Nov 21, 2014 at 07:17:18PM +0100, Johannes Pointner wrote: > > Before the patches were also jumps but I thought it is something > > Vignesh should know. Maybe there is some fix for that too? I believe I've seen the "jumping" behavior pop up at a couple random times in my testing. I think it relates to the hardware not properly registering a pen-up interrupt. When that's happening can you try running ts_calibrate? If you're not registering a pen-up event then you won't be able to advance through those touch points. Is this behavior related to capturing ADC samples, or do you see this behavior even without capturing ADC samples? > > As Richard also noted, it would be nice if ti could let us know how to > > get the delay values right. By trial and error is IMHO not the best > > way. > > That is not only an opinion, it is a matter of fact. TI really dropped the ball on > this one. I thought the patch series was a sign they finally were going to > properly address this issue. Wrong again. These patches originate from work I did on TI's SDK 7.00 (3.12 kernel). The touchscreen is working beautifully in that environment and we are putting forth significant effort to bring those improvements to the community. Things do not seem to be working as well with the current mainline kernel. I've been reviewing various patches between 3.12 and the mainline to understand why it's different. I believe I have figured this out, and I have discussions going separately with individuals that contributed those patches in order to come up with the best possible solution, i.e. we likely need a few additional changes to this patch set. The CHARGECONFIG delay serves precisely the same purpose as the udelay in the original code base. How did you determine the udelay value in the first place? I went through the effort of figuring out a way to make the delay into a HARDWARE delay instead of a software delay so that it could be removed from the ISR. That delay time relates to "settling time" and so it's going to be dependent on your PCB. You could hook up an oscilloscope and attempt to capture this parameter from the signals themselves. Personally I think that's a lot more difficult than just doing a few iterations of testing. > The data sheet for the TI part used to have a reference to an app-note for > the touch controller. > > spruh73f page 1154 > > The Pen IRQ can only occur if the correct Pen Ctrl bits are high > (AFE Pen Ctrl bits 6:5 in the TSC_ADC control register) and if > the correct ground transistor biasing is set in the StepConfig > [N] Register. Refer to the application notes for the correct > settings. > > I searched high and low for this application note. Then, the data sheet got > revised. Such an application note does not exist, which explains why you didn't find it and why we have removed that reference from the TRM. Sorry for your trouble. We always strive for accurate documentation.
2014-11-21 23:25 GMT+01:00 Griffis, Brad <bgriffis@ti.com>: > >> -----Original Message----- >> From: Richard Cochran [mailto:richardcochran@gmail.com] >> >> On Fri, Nov 21, 2014 at 07:17:18PM +0100, Johannes Pointner wrote: >> > Before the patches were also jumps but I thought it is something >> > Vignesh should know. Maybe there is some fix for that too? > > I believe I've seen the "jumping" behavior pop up at a couple random times in my testing. I think it relates to the hardware not properly registering a pen-up interrupt. When that's happening can you try running ts_calibrate? If you're not registering a pen-up event then you won't be able to advance through those touch points. Is this behavior related to capturing ADC samples, or do you see this behavior even without capturing ADC samples? Is it the right way to test? Because I think the ts_lib will cover misbehavior of the touchscreen driver. > >> > As Richard also noted, it would be nice if ti could let us know how to >> > get the delay values right. By trial and error is IMHO not the best >> > way. >> >> That is not only an opinion, it is a matter of fact. TI really dropped the ball on >> this one. I thought the patch series was a sign they finally were going to >> properly address this issue. Wrong again. > > These patches originate from work I did on TI's SDK 7.00 (3.12 kernel). The touchscreen is working beautifully in that environment and we are putting forth significant effort to bring those improvements to the community. > > Things do not seem to be working as well with the current mainline kernel. I've been reviewing various patches between 3.12 and the mainline to understand why it's different. I believe I have figured this out, and I have discussions going separately with individuals that contributed those patches in order to come up with the best possible solution, i.e. we likely need a few additional changes to this patch set. Ok, then I'm looking forward for a new patch set to test. Furthermore, I would be interested which changes you are talking about. > > The CHARGECONFIG delay serves precisely the same purpose as the udelay in the original code base. How did you determine the udelay value in the first place? I went through the effort of figuring out a way to make the delay into a HARDWARE delay instead of a software delay so that it could be removed from the ISR. That delay time relates to "settling time" and so it's going to be dependent on your PCB. You could hook up an oscilloscope and attempt to capture this parameter from the signals themselves. Personally I think that's a lot more difficult than just doing a few iterations of testing. I did measure with an oscilloscope to get the right charge delay and it looks fine to me. But I can't affect the jumps with the charge delay, the only thing that helped, was to add some sample delay. But it didn't solve it. > >> The data sheet for the TI part used to have a reference to an app-note for >> the touch controller. >> >> spruh73f page 1154 >> >> The Pen IRQ can only occur if the correct Pen Ctrl bits are high >> (AFE Pen Ctrl bits 6:5 in the TSC_ADC control register) and if >> the correct ground transistor biasing is set in the StepConfig >> [N] Register. Refer to the application notes for the correct >> settings. >> >> I searched high and low for this application note. Then, the data sheet got >> revised. > > Such an application note does not exist, which explains why you didn't find it and why we have removed that reference from the TRM. Sorry for your trouble. We always strive for accurate documentation. Thanks, Hannes
On 11/21/2014 02:10 PM, Richard Cochran wrote: > On the BB white using the LCD4 cape and the shipped debian kernel, the > cursor *does* jump away, but not as often or as far as on the custom > design I was working with. This sounds like the ADC is still sampling while the input data becomes invalid. Usually there is a resistor on the wiper line and the touchscreen manual says how much it should be at least. Could you please check if this is properly installed? > > Thanks, > Richard Sebastian
On Mon, Nov 24, 2014 at 09:57:46AM +0100, Sebastian Andrzej Siewior wrote: > On 11/21/2014 02:10 PM, Richard Cochran wrote: > > > On the BB white using the LCD4 cape and the shipped debian kernel, the > > cursor *does* jump away, but not as often or as far as on the custom > > design I was working with. > > This sounds like the ADC is still sampling while the input data becomes > invalid. Usually there is a resistor on the wiper line and the > touchscreen manual says how much it should be at least. Could you > please check if this is properly installed? Wiper line? This is a four wire device. Thanks, Richard
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c index d877e777cce6..110038859a8d 100644 --- a/drivers/mfd/ti_am335x_tscadc.c +++ b/drivers/mfd/ti_am335x_tscadc.c @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc) spin_lock_irq(&tsadc->reg_lock); finish_wait(&tsadc->reg_se_wait, &wait); + /* + * Sequencer should either be idle or + * busy applying the charge step. + */ reg = tscadc_readl(tsadc, REG_ADCFSM); - WARN_ON(reg & SEQ_STATUS); + WARN_ON((reg & SEQ_STATUS) && !(reg & CHARGE_STEP)); tsadc->adc_waiting = false; } tsadc->adc_in_use = true; @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc) void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val) { spin_lock_irq(&tsadc->reg_lock); - tsadc->reg_se_cache |= val; am335x_tscadc_need_adc(tsadc); tscadc_writel(tsadc, REG_SE, val); diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h index 3f4e994ace2b..1fd50dcfe47c 100644 --- a/include/linux/mfd/ti_am335x_tscadc.h +++ b/include/linux/mfd/ti_am335x_tscadc.h @@ -128,6 +128,7 @@ /* Sequencer Status */ #define SEQ_STATUS BIT(5) +#define CHARGE_STEP 0x11 #define ADC_CLK 3000000 #define TOTAL_STEPS 16
In one shot mode, sequencer automatically disables all enabled steps at the end of each cycle. (both ADC steps and TSC steps) Hence these steps need not be saved in reg_se_cache for clearing these steps at a later stage. Also, when ADC wakes up Sequencer should not be busy executing any of the config steps except for the charge step. Previously charge step was 1 ADC clock cycle and hence it was ignored. Signed-off-by: Vignesh R <vigneshr@ti.com> --- drivers/mfd/ti_am335x_tscadc.c | 7 +++++-- include/linux/mfd/ti_am335x_tscadc.h | 1 + 2 files changed, 6 insertions(+), 2 deletions(-)