[RFC] iio: adc: at91: fix acking DRDY irq (again)

Commit Message

Ladislav Michl June 11, 2019, 11:56 a.m. UTC

Hi there,

(and sorry for unsubscribing from this list as I'm not able to follow
all development and missed previous discussions [*] :( )

It seems that none of these fixes are dealing with problem properly,
however they certainly improve it.

SAM9G20 Datasheet DS60001516A rev Oct-17 states in chapter 39.5.4:
  "Reading one of the ADC_CDR registers clears the corresponding EOC bit.
   Reading ADC_LCDR clears the DRDY bit and the EOC bit corresponding to
   the last converted channel."
That implies reading ADC_LCDR each time irq handler is called with DRDY
bit set and patch bellow does exactly that.

However, that does not fix all issues. Commit 09c6bdee5118 ("iio: adc:
at91: disable adc channel interrupt in timeout case") states in commit log:
  "If 2 different channels are queried we can end up with a situation
   where two interrupts are enabled, but only one interrupt is
   cleared in the interrupt handler"
How this can ever happen? Querying using IIO_CHAN_INFO_RAW is serialized
with st->lock and even if that happens we have only one st->chnb field
to store channel we started conversion for. So EOC handler does know
which channel just finished conversion as we do not test for EOC bit.

Driver also contains some code for TC triggers. How is that supposed to
work? [**] The very same manual states in chapter 39.5.5:
  "If one of the TIOA outputs is selected, the corresponding Timer Counter
   channel must be programmed in Waveform Mode."
There are two drivers touching TC: drivers/clocksource/timer-atmel-tcb.c
and drivers/pwm/pwm-atmel-tcb.c, they seem to conflict each other and
none of them is anyhow related to ADC driver. Here it would seem
appropriate to have TC MFD driver and allocate timers for ADC, PWM and
clocksource from there.

I know it is a long time since ADC driver was developed, but it would
really help if any of its principal authors could answer questions above.

Eventual answer is appreciated in advance,
	ladis

[*]  starting here: https://marc.info/?l=linux-iio&m=154885673507542 and
     here: https://marc.info/?l=linux-iio&m=153777586127721
[**] libiio is obviously unable to deal with at91_adc:
iio_readdev -t trigger0 fffe0000.adc voltage0
WARNING: High-speed mode not enabled
Unable to refill buffer: Connection timed out

Comments

Alexandre Belloni June 12, 2019, 8:15 a.m. UTC | #1

On 11/06/2019 13:56:03+0200, Ladislav Michl wrote:
> Driver also contains some code for TC triggers. How is that supposed to
> work? [**] The very same manual states in chapter 39.5.5:
>   "If one of the TIOA outputs is selected, the corresponding Timer Counter
>    channel must be programmed in Waveform Mode."
> There are two drivers touching TC: drivers/clocksource/timer-atmel-tcb.c
> and drivers/pwm/pwm-atmel-tcb.c, they seem to conflict each other and

They don't, they can work simultaneously, on different TCBs. I'm still
planning to rework pwm-atmel-tcb to switch it to the proper binding.

> none of them is anyhow related to ADC driver. Here it would seem
> appropriate to have TC MFD driver and allocate timers for ADC, PWM and
> clocksource from there.

No, MFD is way too late for clocksource, this would break some platforms.

However, there is definitively some timer framework that is missing to
allow handling of timers that are not used as clocksource/clockevent
devices. So indeed, there is a missing piece to make the TC trigger
work.

Ladislav Michl June 12, 2019, 9:31 a.m. UTC | #2

On Wed, Jun 12, 2019 at 10:15:33AM +0200, Alexandre Belloni wrote:
> On 11/06/2019 13:56:03+0200, Ladislav Michl wrote:
> > Driver also contains some code for TC triggers. How is that supposed to
> > work? [**] The very same manual states in chapter 39.5.5:
> >   "If one of the TIOA outputs is selected, the corresponding Timer Counter
> >    channel must be programmed in Waveform Mode."
> > There are two drivers touching TC: drivers/clocksource/timer-atmel-tcb.c
> > and drivers/pwm/pwm-atmel-tcb.c, they seem to conflict each other and
> 
> They don't, they can work simultaneously, on different TCBs. I'm still
> planning to rework pwm-atmel-tcb to switch it to the proper binding.

Is there any draft how should that "proper binding" look like?
By "conflict" I mean DT can be written so both race for the same resource,
while I would expect timer definition with PWM and ADC using its phandle.

> > none of them is anyhow related to ADC driver. Here it would seem
> > appropriate to have TC MFD driver and allocate timers for ADC, PWM and
> > clocksource from there.
> 
> No, MFD is way too late for clocksource, this would break some platforms.
> 
> However, there is definitively some timer framework that is missing to
> allow handling of timers that are not used as clocksource/clockevent
> devices. So indeed, there is a missing piece to make the TC trigger
> work.

Can that be done similar way drivers/clocksource/timer-ti-dm.c is
implemented or do you have something else in mind?

Thank you,
	ladis

Alexandre Belloni June 20, 2019, 8:43 a.m. UTC | #3

On 12/06/2019 11:31:42+0200, Ladislav Michl wrote:
> On Wed, Jun 12, 2019 at 10:15:33AM +0200, Alexandre Belloni wrote:
> > On 11/06/2019 13:56:03+0200, Ladislav Michl wrote:
> > > Driver also contains some code for TC triggers. How is that supposed to
> > > work? [**] The very same manual states in chapter 39.5.5:
> > >   "If one of the TIOA outputs is selected, the corresponding Timer Counter
> > >    channel must be programmed in Waveform Mode."
> > > There are two drivers touching TC: drivers/clocksource/timer-atmel-tcb.c
> > > and drivers/pwm/pwm-atmel-tcb.c, they seem to conflict each other and
> > 
> > They don't, they can work simultaneously, on different TCBs. I'm still
> > planning to rework pwm-atmel-tcb to switch it to the proper binding.
> 
> Is there any draft how should that "proper binding" look like?
> By "conflict" I mean DT can be written so both race for the same resource,
> while I would expect timer definition with PWM and ADC using its phandle.
> 

Last part of https://lore.kernel.org/lkml/[e-mail]/

This is not yet upstream but I'm planning to resend sometime in July.


> > > none of them is anyhow related to ADC driver. Here it would seem
> > > appropriate to have TC MFD driver and allocate timers for ADC, PWM and
> > > clocksource from there.
> > 
> > No, MFD is way too late for clocksource, this would break some platforms.
> > 
> > However, there is definitively some timer framework that is missing to
> > allow handling of timers that are not used as clocksource/clockevent
> > devices. So indeed, there is a missing piece to make the TC trigger
> > work.
> 
> Can that be done similar way drivers/clocksource/timer-ti-dm.c is
> implemented or do you have something else in mind?
> 

Yes, something similar but this could probably be made more generic as
this would benefit many other platforms too (i.e broadcom, nxp,
amlogic).

Patch

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index d23709ed9049..c086fa335b30 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -262,9 +262,6 @@  static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
 
 	iio_trigger_notify_done(idev->trig);
 
-	/* Needed to ACK the DRDY interruption */
-	at91_adc_readl(st, AT91_ADC_LCDR);
-
 	enable_irq(st->irq);
 
 	return IRQ_HANDLED;
@@ -280,8 +277,6 @@  static void handle_adc_eoc_trigger(int irq, struct iio_dev *idev)
 		iio_trigger_poll(idev->trig);
 	} else {
 		st->last_value = at91_adc_readl(st, AT91_ADC_CHAN(st, st->chnb));
-		/* Needed to ACK the DRDY interruption */
-		at91_adc_readl(st, AT91_ADC_LCDR);
 		st->done = true;
 		wake_up_interruptible(&st->wq_data_avail);
 	}
@@ -357,9 +352,11 @@  static irqreturn_t at91_adc_rl_interrupt(int irq, void *private)
 	u32 status = at91_adc_readl(st, st->registers->status_register);
 	unsigned int reg;
 
-	status &= at91_adc_readl(st, AT91_ADC_IMR);
-	if (status & GENMASK(st->num_channels - 1, 0))
+	if (status & st->registers->drdy_mask) {
 		handle_adc_eoc_trigger(irq, idev);
+		/* Needed to ACK the DRDY interruption */
+		at91_adc_readl(st, AT91_ADC_LCDR);
+	}
 
 	if (status & AT91RL_ADC_IER_PEN) {
 		/* Disabling pen debounce is required to get a NOPEN irq */
@@ -425,8 +422,11 @@  static irqreturn_t at91_adc_9x5_interrupt(int irq, void *private)
 		AT91_ADC_IER_YRDY |
 		AT91_ADC_IER_PRDY;
 
-	if (status & GENMASK(st->num_channels - 1, 0))
+	if (status & st->registers->drdy_mask) {
 		handle_adc_eoc_trigger(irq, idev);
+		/* Needed to ACK the DRDY interruption */
+		at91_adc_readl(st, AT91_ADC_LCDR);
+	}
 
 	if (status & AT91_ADC_IER_PEN) {
 		at91_adc_writel(st, AT91_ADC_IDR, AT91_ADC_IER_PEN);