irq flood happen when run
cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
imx8qxp_adc_read_raw()
{
...
enable irq
/* adc start */
writel(1, adc->regs + IMX8QXP_ADR_ADC_SWTRIG);
^^^^ trigger irq flood.
wait_for_completion_interruptible_timeout();
readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO);
^^^^ clear irq here.
...
}
There is only FIFO watermark interrupt at this ADC controller.
IRQ line will be assert until software read data from FIFO.
So IRQ flood happen during wait_for_completion_interruptible_timeout().
Move FIFO read into irq handle to avoid irq flood.
Fixes: 1e23dcaa1a9f ("iio: imx8qxp-adc: Add driver support for NXP IMX8QXP ADC")
Cc: [email protected]
Signed-off-by: Frank Li <[email protected]>
---
Change from v1 to v2
- move complete() after read fifo
drivers/iio/adc/imx8qxp-adc.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/imx8qxp-adc.c b/drivers/iio/adc/imx8qxp-adc.c
index 36777b827165..f5a0fc9e64c5 100644
--- a/drivers/iio/adc/imx8qxp-adc.c
+++ b/drivers/iio/adc/imx8qxp-adc.c
@@ -86,6 +86,8 @@
#define IMX8QXP_ADC_TIMEOUT msecs_to_jiffies(100)
+#define IMX8QXP_ADC_MAX_FIFO_SIZE 16
+
struct imx8qxp_adc {
struct device *dev;
void __iomem *regs;
@@ -95,6 +97,7 @@ struct imx8qxp_adc {
/* Serialise ADC channel reads */
struct mutex lock;
struct completion completion;
+ u32 fifo[IMX8QXP_ADC_MAX_FIFO_SIZE];
};
#define IMX8QXP_ADC_CHAN(_idx) { \
@@ -238,8 +241,7 @@ static int imx8qxp_adc_read_raw(struct iio_dev *indio_dev,
return ret;
}
- *val = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
- readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
+ *val = adc->fifo[0];
mutex_unlock(&adc->lock);
return IIO_VAL_INT;
@@ -265,10 +267,15 @@ static irqreturn_t imx8qxp_adc_isr(int irq, void *dev_id)
{
struct imx8qxp_adc *adc = dev_id;
u32 fifo_count;
+ int i;
fifo_count = FIELD_GET(IMX8QXP_ADC_FCTRL_FCOUNT_MASK,
readl(adc->regs + IMX8QXP_ADR_ADC_FCTRL));
+ for (i = 0; i < fifo_count; i++)
+ adc->fifo[i] = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
+ readl_relaxed(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
+
if (fifo_count)
complete(&adc->completion);
--
2.34.1
> -----Original Message-----
> From: Frank Li <[email protected]>
> Sent: 2022??12??1?? 22:01
> To: Bough Chen <[email protected]>
> Cc: [email protected]; [email protected]; Frank Li <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call
> imx8qxp_adc_read_raw()
>
> irq flood happen when run
> cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
>
> imx8qxp_adc_read_raw()
> {
> ...
> enable irq
> /* adc start */
> writel(1, adc->regs + IMX8QXP_ADR_ADC_SWTRIG);
> ^^^^ trigger irq flood.
> wait_for_completion_interruptible_timeout();
> readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO);
> ^^^^ clear irq here.
> ...
> }
>
> There is only FIFO watermark interrupt at this ADC controller.
> IRQ line will be assert until software read data from FIFO.
> So IRQ flood happen during wait_for_completion_interruptible_timeout().
>
> Move FIFO read into irq handle to avoid irq flood.
>
> Fixes: 1e23dcaa1a9f ("iio: imx8qxp-adc: Add driver support for NXP IMX8QXP
> ADC")
> Cc: [email protected]
>
> Signed-off-by: Frank Li <[email protected]>
Thanks for the quick fix. The total read count in irq handler is fifo_count which is read from register, this is reasonable.
Reviewed-by: Haibo Chen <[email protected]>
Best Regards
Haibo Chen
> ---
> Change from v1 to v2
> - move complete() after read fifo
>
>
> drivers/iio/adc/imx8qxp-adc.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/imx8qxp-adc.c b/drivers/iio/adc/imx8qxp-adc.c index
> 36777b827165..f5a0fc9e64c5 100644
> --- a/drivers/iio/adc/imx8qxp-adc.c
> +++ b/drivers/iio/adc/imx8qxp-adc.c
> @@ -86,6 +86,8 @@
>
> #define IMX8QXP_ADC_TIMEOUT msecs_to_jiffies(100)
>
> +#define IMX8QXP_ADC_MAX_FIFO_SIZE 16
> +
> struct imx8qxp_adc {
> struct device *dev;
> void __iomem *regs;
> @@ -95,6 +97,7 @@ struct imx8qxp_adc {
> /* Serialise ADC channel reads */
> struct mutex lock;
> struct completion completion;
> + u32 fifo[IMX8QXP_ADC_MAX_FIFO_SIZE];
> };
>
> #define IMX8QXP_ADC_CHAN(_idx) { \
> @@ -238,8 +241,7 @@ static int imx8qxp_adc_read_raw(struct iio_dev
> *indio_dev,
> return ret;
> }
>
> - *val = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> - readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> + *val = adc->fifo[0];
>
> mutex_unlock(&adc->lock);
> return IIO_VAL_INT;
> @@ -265,10 +267,15 @@ static irqreturn_t imx8qxp_adc_isr(int irq, void
> *dev_id) {
> struct imx8qxp_adc *adc = dev_id;
> u32 fifo_count;
> + int i;
>
> fifo_count = FIELD_GET(IMX8QXP_ADC_FCTRL_FCOUNT_MASK,
> readl(adc->regs + IMX8QXP_ADR_ADC_FCTRL));
>
> + for (i = 0; i < fifo_count; i++)
> + adc->fifo[i] = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> + readl_relaxed(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> +
> if (fifo_count)
> complete(&adc->completion);
>
> --
> 2.34.1
On 02 12月 22 03:23:55, Bough Chen wrote:
> > -----Original Message-----
> > From: Frank Li <[email protected]>
> > Sent: 2022年12月1日 22:01
> > To: Bough Chen <[email protected]>
> > Cc: [email protected]; [email protected]; Frank Li <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; dl-linux-imx <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Subject: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call
> > imx8qxp_adc_read_raw()
> >
> > irq flood happen when run
> > cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
> >
> > imx8qxp_adc_read_raw()
> > {
> > ...
> > enable irq
> > /* adc start */
> > writel(1, adc->regs + IMX8QXP_ADR_ADC_SWTRIG);
> > ^^^^ trigger irq flood.
> > wait_for_completion_interruptible_timeout();
> > readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO);
> > ^^^^ clear irq here.
> > ...
> > }
> >
> > There is only FIFO watermark interrupt at this ADC controller.
> > IRQ line will be assert until software read data from FIFO.
> > So IRQ flood happen during wait_for_completion_interruptible_timeout().
> >
> > Move FIFO read into irq handle to avoid irq flood.
> >
> > Fixes: 1e23dcaa1a9f ("iio: imx8qxp-adc: Add driver support for NXP IMX8QXP
> > ADC")
> > Cc: [email protected]
> >
> > Signed-off-by: Frank Li <[email protected]>
>
> Thanks for the quick fix. The total read count in irq handler is fifo_count which is read from register, this is reasonable.
>
> Reviewed-by: Haibo Chen <[email protected]>
The same, thanks.
Reviewed-by: Cai Huoqing <[email protected]>
>
> Best Regards
> Haibo Chen
> > ---
> > Change from v1 to v2
> > - move complete() after read fifo
> >
> >
> > drivers/iio/adc/imx8qxp-adc.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/adc/imx8qxp-adc.c b/drivers/iio/adc/imx8qxp-adc.c index
> > 36777b827165..f5a0fc9e64c5 100644
> > --- a/drivers/iio/adc/imx8qxp-adc.c
> > +++ b/drivers/iio/adc/imx8qxp-adc.c
> > @@ -86,6 +86,8 @@
> >
> > #define IMX8QXP_ADC_TIMEOUT msecs_to_jiffies(100)
> >
> > +#define IMX8QXP_ADC_MAX_FIFO_SIZE 16
> > +
> > struct imx8qxp_adc {
> > struct device *dev;
> > void __iomem *regs;
> > @@ -95,6 +97,7 @@ struct imx8qxp_adc {
> > /* Serialise ADC channel reads */
> > struct mutex lock;
> > struct completion completion;
> > + u32 fifo[IMX8QXP_ADC_MAX_FIFO_SIZE];
> > };
> >
> > #define IMX8QXP_ADC_CHAN(_idx) { \
> > @@ -238,8 +241,7 @@ static int imx8qxp_adc_read_raw(struct iio_dev
> > *indio_dev,
> > return ret;
> > }
> >
> > - *val = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> > - readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> > + *val = adc->fifo[0];
> >
> > mutex_unlock(&adc->lock);
> > return IIO_VAL_INT;
> > @@ -265,10 +267,15 @@ static irqreturn_t imx8qxp_adc_isr(int irq, void
> > *dev_id) {
> > struct imx8qxp_adc *adc = dev_id;
> > u32 fifo_count;
> > + int i;
> >
> > fifo_count = FIELD_GET(IMX8QXP_ADC_FCTRL_FCOUNT_MASK,
> > readl(adc->regs + IMX8QXP_ADR_ADC_FCTRL));
> >
> > + for (i = 0; i < fifo_count; i++)
> > + adc->fifo[i] = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> > + readl_relaxed(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> > +
> > if (fifo_count)
> > complete(&adc->completion);
> >
> > --
> > 2.34.1
>
On Fri, 2 Dec 2022 17:05:54 +0800
Cai Huoqing <[email protected]> wrote:
> On 02 12月 22 03:23:55, Bough Chen wrote:
> > > -----Original Message-----
> > > From: Frank Li <[email protected]>
> > > Sent: 2022年12月1日 22:01
> > > To: Bough Chen <[email protected]>
> > > Cc: [email protected]; [email protected]; Frank Li <[email protected]>;
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; dl-linux-imx <[email protected]>;
> > > [email protected]; [email protected]; [email protected]
> > > Subject: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call
> > > imx8qxp_adc_read_raw()
> > >
> > > irq flood happen when run
> > > cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
> > >
> > > imx8qxp_adc_read_raw()
> > > {
> > > ...
> > > enable irq
> > > /* adc start */
> > > writel(1, adc->regs + IMX8QXP_ADR_ADC_SWTRIG);
> > > ^^^^ trigger irq flood.
> > > wait_for_completion_interruptible_timeout();
> > > readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO);
> > > ^^^^ clear irq here.
> > > ...
> > > }
> > >
> > > There is only FIFO watermark interrupt at this ADC controller.
> > > IRQ line will be assert until software read data from FIFO.
> > > So IRQ flood happen during wait_for_completion_interruptible_timeout().
> > >
> > > Move FIFO read into irq handle to avoid irq flood.
> > >
> > > Fixes: 1e23dcaa1a9f ("iio: imx8qxp-adc: Add driver support for NXP IMX8QXP
> > > ADC")
> > > Cc: [email protected]
> > >
> > > Signed-off-by: Frank Li <[email protected]>
> >
> > Thanks for the quick fix. The total read count in irq handler is fifo_count which is read from register, this is reasonable.
> >
> > Reviewed-by: Haibo Chen <[email protected]>
> The same, thanks.
> Reviewed-by: Cai Huoqing <[email protected]>
I briefly wondered if it would be better to return the newest value,
but given it's not obvious and they will be close together in this case,
I'm fine with this.
Applied to the fixes-togreg branch of iio.git and marked for
stable inclusion. Note I'm unlikely to send a pull request until after rc1 now
so this will take a few weeks to land upstream.
Thanks,
Jonathan
> >
> > Best Regards
> > Haibo Chen
> > > ---
> > > Change from v1 to v2
> > > - move complete() after read fifo
> > >
> > >
> > > drivers/iio/adc/imx8qxp-adc.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/imx8qxp-adc.c b/drivers/iio/adc/imx8qxp-adc.c index
> > > 36777b827165..f5a0fc9e64c5 100644
> > > --- a/drivers/iio/adc/imx8qxp-adc.c
> > > +++ b/drivers/iio/adc/imx8qxp-adc.c
> > > @@ -86,6 +86,8 @@
> > >
> > > #define IMX8QXP_ADC_TIMEOUT msecs_to_jiffies(100)
> > >
> > > +#define IMX8QXP_ADC_MAX_FIFO_SIZE 16
> > > +
> > > struct imx8qxp_adc {
> > > struct device *dev;
> > > void __iomem *regs;
> > > @@ -95,6 +97,7 @@ struct imx8qxp_adc {
> > > /* Serialise ADC channel reads */
> > > struct mutex lock;
> > > struct completion completion;
> > > + u32 fifo[IMX8QXP_ADC_MAX_FIFO_SIZE];
> > > };
> > >
> > > #define IMX8QXP_ADC_CHAN(_idx) { \
> > > @@ -238,8 +241,7 @@ static int imx8qxp_adc_read_raw(struct iio_dev
> > > *indio_dev,
> > > return ret;
> > > }
> > >
> > > - *val = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> > > - readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> > > + *val = adc->fifo[0];
> > >
> > > mutex_unlock(&adc->lock);
> > > return IIO_VAL_INT;
> > > @@ -265,10 +267,15 @@ static irqreturn_t imx8qxp_adc_isr(int irq, void
> > > *dev_id) {
> > > struct imx8qxp_adc *adc = dev_id;
> > > u32 fifo_count;
> > > + int i;
> > >
> > > fifo_count = FIELD_GET(IMX8QXP_ADC_FCTRL_FCOUNT_MASK,
> > > readl(adc->regs + IMX8QXP_ADR_ADC_FCTRL));
> > >
> > > + for (i = 0; i < fifo_count; i++)
> > > + adc->fifo[i] = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> > > + readl_relaxed(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> > > +
> > > if (fifo_count)
> > > complete(&adc->completion);
> > >
> > > --
> > > 2.34.1
> >