On 12/19/13 15:28, Sebastian Andrzej Siewior wrote: > The ADC driver always programs all possible ADC values and discards > them except for the value IIO asked for. On the am335x-evm the driver > programs four values and it takes 500us to gather them. Reducing the number > of conversations down to the (required) one also reduces the busy loop down > to 125us. > > This leads to another error, namely the FIFOCOUNT register is sometimes > (like one out of 10 attempts) not updated in time leading to EBUSY. > The next read has the FIFOCOUNT register updated. > Checking for the ADCSTAT register for being idle isn't a good choice either. > The problem is that if TSC is used at the same time, the HW completes the > conversation for ADC *and* before the driver noticed it, the HW begins to > perform a TSC conversation and so the driver never seen the HW idle. The > next time we would have two values in the FIFO but since the driver reads > everything we always see the current one. > So instead of polling for the IDLE bit in ADCStatus register, we should > check the FIFOCOUNT register. It should be one instead of zero because we > request one value. > > This change in turn leads to another error. Sometimes if TSC & ADC are > used together the TSC starts becoming interrupts even if nobody becoming -> generating? > actually touched the touchscreen. The interrupts seem valid because TSC's > FIFO is filled with values for each channel of the TSC. This condition stops > after a few ADC reads but will occur again. Not good. > > On top of this (even without the changes I just mentioned) there is a ADC > & TSC lockup condition which was reported to me by Jeff Lance including the > following test case: > A busy loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw" > and a mug on touch screen. With this setup, the hardware will lockup after > something between 20 minutes and it could take up to a couple of hours. > During that lockup, the ADCSTAT register says 0x30 (or 0x70) which means > STEP_ID = IDLE and FSM_BUSY = yes. That means the hardware says that it is > idle and busy at the same time which is an invalid condition. > yikes. > For all this reasons I decided to rework this TSC/ADC part and add a > handshake / synchronization here: > First the ADC signals that it needs the HW and writes a 0 mask into the > SE register. The HW (if active) will complete the current conversation > and become idle. The TSC driver will gather the values from the FIFO > (woken up by an interrupt) and won't "enable" another conversation. > Instead it will wake up the ADC driver which is already waiting. The ADC > driver will start "its" conversation and once it is done, it will > enable the TSC steps so the TSC will work again. > > After this rework I haven't observed the lockup so far. Plus the busy > loop has been reduced from 500us to 125us. > > The continues-read mode remains unchanged. > Thanks for the detailed explanation.
> Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de> Hmm. I'm not really an expert in this complex driver, but your explanation is clear and the code appears to implement what you describe. Hopefully we'll see a fix for the continuous reads as well. Fiddly little device! Acked-by: Jonathan Cameron <ji...@kernel.org> > --- > drivers/iio/adc/ti_am335x_adc.c | 64 > ++++++++++++++++++++++++++---------- > drivers/mfd/ti_am335x_tscadc.c | 63 +++++++++++++++++++++++++++++------ > include/linux/mfd/ti_am335x_tscadc.h | 4 +++ > 3 files changed, 103 insertions(+), 28 deletions(-) > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index 6822b9f..31e786e 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -60,6 +60,24 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev) > return step_en; > } > > +static u32 get_adc_chan_step_mask(struct tiadc_device *adc_dev, > + struct iio_chan_spec const *chan) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) { > + if (chan->channel == adc_dev->channel_line[i]) { > + u32 step; > + > + step = adc_dev->channel_step[i]; > + /* +1 for the charger */ > + return 1 << (step + 1); > + } > + } > + WARN_ON(1); > + return 0; > +} > + > static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan) > { > return 1 << adc_dev->channel_step[chan]; > @@ -329,34 +347,43 @@ static int tiadc_read_raw(struct iio_dev *indio_dev, > unsigned int fifo1count, read, stepid; > bool found = false; > u32 step_en; > - unsigned long timeout = jiffies + usecs_to_jiffies > - (IDLE_TIMEOUT * adc_dev->channels); > + unsigned long timeout; > > if (iio_buffer_enabled(indio_dev)) > return -EBUSY; > > - step_en = get_adc_step_mask(adc_dev); > + step_en = get_adc_chan_step_mask(adc_dev, chan); > + if (!step_en) > + return -EINVAL; > + > + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT); > + while (fifo1count--) > + tiadc_readl(adc_dev, REG_FIFO1); > + > am335x_tsc_se_set_once(adc_dev->mfd_tscadc, step_en); > > - /* Wait for ADC sequencer to complete sampling */ > - while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) { > - if (time_after(jiffies, timeout)) > + timeout = jiffies + usecs_to_jiffies > + (IDLE_TIMEOUT * adc_dev->channels); > + /* Wait for Fifo threshold interrupt */ > + while (1) { > + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT); > + if (fifo1count) > + break; > + > + if (time_after(jiffies, timeout)) { > + am335x_tsc_se_adc_done(adc_dev->mfd_tscadc); > return -EAGAIN; > + } > } > map_val = chan->channel + TOTAL_CHANNELS; > > /* > - * When the sub-system is first enabled, > - * the sequencer will always start with the > - * lowest step (1) and continue until step (16). > - * For ex: If we have enabled 4 ADC channels and > - * currently use only 1 out of them, the > - * sequencer still configures all the 4 steps, > - * leading to 3 unwanted data. > - * Hence we need to flush out this data. > + * We check the complete FIFO. We programmed just one entry but in case > + * something went wrong we left empty handed (-EAGAIN previously) and > + * then the value apeared somehow in the FIFO we would have two entries. > + * Therefore we read every item and keep only the latest version of the > + * requested channel. > */ > - > - fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT); > for (i = 0; i < fifo1count; i++) { > read = tiadc_readl(adc_dev, REG_FIFO1); > stepid = read & FIFOREAD_CHNLID_MASK; > @@ -368,6 +395,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev, > *val = (u16) read; > } > } > + am335x_tsc_se_adc_done(adc_dev->mfd_tscadc); > > if (found == false) > return -EBUSY; > @@ -495,8 +523,8 @@ static int tiadc_resume(struct device *dev) > tiadc_writel(adc_dev, REG_CTRL, restore); > > tiadc_step_config(indio_dev); > - am335x_tsc_se_set(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps); > - > + am335x_tsc_se_set_cache(adc_dev->mfd_tscadc, > + adc_dev->buffer_en_ch_steps); > return 0; > } > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > index 157f569..d4e8604 100644 > --- a/drivers/mfd/ti_am335x_tscadc.c > +++ b/drivers/mfd/ti_am335x_tscadc.c > @@ -24,6 +24,7 @@ > #include <linux/pm_runtime.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/sched.h> > > #include <linux/mfd/ti_am335x_tscadc.h> > > @@ -48,31 +49,71 @@ static const struct regmap_config tscadc_regmap_config = { > .val_bits = 32, > }; > > -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc) > -{ > - tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache); > -} > - > void am335x_tsc_se_set_cache(struct ti_tscadc_dev *tsadc, u32 val) > { > unsigned long flags; > > spin_lock_irqsave(&tsadc->reg_lock, flags); > - tsadc->reg_se_cache |= val; > - am335x_tsc_se_update(tsadc); > + tsadc->reg_se_cache = val; > + if (tsadc->adc_waiting) > + wake_up(&tsadc->reg_se_wait); > + else if (!tsadc->adc_in_use) > + tscadc_writel(tsadc, REG_SE, val); > + > spin_unlock_irqrestore(&tsadc->reg_lock, flags); > } > EXPORT_SYMBOL_GPL(am335x_tsc_se_set_cache); > > +static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc) > +{ > + DEFINE_WAIT(wait); > + u32 reg; > + > + /* > + * disable TSC steps so it does not run while the ADC is using it. If > + * write 0 while it is running (it just started or was already running) > + * then it completes all steps that were enabled and stops then. > + */ > + tscadc_writel(tsadc, REG_SE, 0); > + reg = tscadc_readl(tsadc, REG_ADCFSM); > + if (reg & SEQ_STATUS) { > + tsadc->adc_waiting = true; > + prepare_to_wait(&tsadc->reg_se_wait, &wait, > + TASK_UNINTERRUPTIBLE); > + spin_unlock_irq(&tsadc->reg_lock); > + > + schedule(); > + > + spin_lock_irq(&tsadc->reg_lock); > + finish_wait(&tsadc->reg_se_wait, &wait); > + > + reg = tscadc_readl(tsadc, REG_ADCFSM); > + WARN_ON(reg & SEQ_STATUS); > + tsadc->adc_waiting = false; > + } > + tsadc->adc_in_use = true; > +} > + > void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val) > { > + spin_lock_irq(&tsadc->reg_lock); > + am335x_tscadc_need_adc(tsadc); > + > + tscadc_writel(tsadc, REG_SE, val); > + spin_unlock_irq(&tsadc->reg_lock); > +} > +EXPORT_SYMBOL_GPL(am335x_tsc_se_set_once); > + > +void am335x_tsc_se_adc_done(struct ti_tscadc_dev *tsadc) > +{ > unsigned long flags; > > spin_lock_irqsave(&tsadc->reg_lock, flags); > - tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache | val); > + tsadc->adc_in_use = false; > + tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache); > spin_unlock_irqrestore(&tsadc->reg_lock, flags); > } > -EXPORT_SYMBOL_GPL(am335x_tsc_se_set_once); > +EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_done); > > void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val) > { > @@ -80,7 +121,7 @@ void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 > val) > > spin_lock_irqsave(&tsadc->reg_lock, flags); > tsadc->reg_se_cache &= ~val; > - am335x_tsc_se_update(tsadc); > + tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache); > spin_unlock_irqrestore(&tsadc->reg_lock, flags); > } > EXPORT_SYMBOL_GPL(am335x_tsc_se_clr); > @@ -188,6 +229,8 @@ static int ti_tscadc_probe(struct platform_device > *pdev) > } > > spin_lock_init(&tscadc->reg_lock); > + init_waitqueue_head(&tscadc->reg_se_wait); > + > pm_runtime_enable(&pdev->dev); > pm_runtime_get_sync(&pdev->dev); > > diff --git a/include/linux/mfd/ti_am335x_tscadc.h > b/include/linux/mfd/ti_am335x_tscadc.h > index 2fa9c06..fb96c84 100644 > --- a/include/linux/mfd/ti_am335x_tscadc.h > +++ b/include/linux/mfd/ti_am335x_tscadc.h > @@ -159,6 +159,9 @@ struct ti_tscadc_dev { > int adc_cell; /* -1 if not used */ > struct mfd_cell cells[TSCADC_CELLS]; > u32 reg_se_cache; > + bool adc_waiting; > + bool adc_in_use; > + wait_queue_head_t reg_se_wait; > spinlock_t reg_lock; > unsigned int clk_div; > > @@ -179,5 +182,6 @@ static inline struct ti_tscadc_dev > *ti_tscadc_dev_get(struct platform_device *p) > void am335x_tsc_se_set_cache(struct ti_tscadc_dev *tsadc, u32 val); > void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val); > void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val); > +void am335x_tsc_se_adc_done(struct ti_tscadc_dev *tsadc); > > #endif > -- 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/