diff mbox

[fpga,4/9] fpga zynq: Check for errors after completing DMA

Message ID 1478732303-13718-5-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jason Gunthorpe Nov. 9, 2016, 10:58 p.m. UTC 
The completion did not check the interrupt status to see if any error
bits were asserted, check error bits and dump some registers if things
went wrong.

A few fixes are needed to make this work, the IXR_ERROR_FLAGS_MASK was
wrong, it included the done bits, which shows a bug in mask/unmask_irqs
which were using the wrong bits, simplify all of this stuff.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 55 +++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

Comments

Moritz Fischer Nov. 17, 2016, 6:10 a.m. UTC | #1 
Hi Jason,

couple of minor things inline below:

On Wed, Nov 9, 2016 at 2:58 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> The completion did not check the interrupt status to see if any error
> bits were asserted, check error bits and dump some registers if things
> went wrong.
>
> A few fixes are needed to make this work, the IXR_ERROR_FLAGS_MASK was
> wrong, it included the done bits, which shows a bug in mask/unmask_irqs
> which were using the wrong bits, simplify all of this stuff.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/fpga/zynq-fpga.c | 55 +++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 40cf0feaca7c..3ffc5fcc3072 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -89,7 +89,7 @@
>  #define IXR_D_P_DONE_MASK              BIT(12)
>   /* FPGA programmed */
>  #define IXR_PCFG_DONE_MASK             BIT(2)
> -#define IXR_ERROR_FLAGS_MASK           0x00F0F860
> +#define IXR_ERROR_FLAGS_MASK           0x00F0C860

True. Ouch.

>  #define IXR_ALL_MASK                   0xF8F7F87F
>
>  /* Miscellaneous constant values */
> @@ -144,23 +144,10 @@ static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv,
>         readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \
>                            timeout_us)
>
> -static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
> +static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
> +                                         u32 enable)
>  {
> -       u32 intr_mask;
> -
> -       intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
> -       zynq_fpga_write(priv, INT_MASK_OFFSET,
> -                       intr_mask | IXR_DMA_DONE_MASK | IXR_ERROR_FLAGS_MASK);
> -}
> -
> -static void zynq_fpga_unmask_irqs(struct zynq_fpga_priv *priv)
> -{
> -       u32 intr_mask;
> -
> -       intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
> -       zynq_fpga_write(priv, INT_MASK_OFFSET,
> -                       intr_mask
> -                       & ~(IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK));
> +       zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);

Not a fan of this ~enable here. Function name indicates you're doing
the opposite
(see below comment).

>  }
>
>  static irqreturn_t zynq_fpga_isr(int irq, void *data)
> @@ -168,7 +155,7 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
>         struct zynq_fpga_priv *priv = data;
>
>         /* disable DMA and error IRQs */
> -       zynq_fpga_mask_irqs(priv);
> +       zynq_fpga_set_irq_mask(priv, 0);
>
>         complete(&priv->dma_done);
>
> @@ -314,6 +301,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>                                const char *buf, size_t count)
>  {
>         struct zynq_fpga_priv *priv;
> +       const char *why;
>         int err;
>         char *kbuf;
>         dma_addr_t dma_addr;
> @@ -337,7 +325,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>         reinit_completion(&priv->dma_done);
>
>         /* enable DMA and error IRQs */
> -       zynq_fpga_unmask_irqs(priv);
> +       zynq_fpga_set_irq_mask(priv, IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK);

I find the naming here confusing. This sets ~(IXR_D_P_DONE_MASK |
IXR_ERROR_FLAGS_MASK)
as mask value, to enable the D_P_DONE and error IRQs yet the function
name indicates the opposite.
>
>         /* the +1 in the src addr is used to hold off on DMA_DONE IRQ
>          * until both AXI and PCAP are done ...
> @@ -352,16 +340,35 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>         intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
>         zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
>
> +       if (intr_status & IXR_ERROR_FLAGS_MASK) {
> +               why = "DMA reported error";
> +               err = -EIO;
> +               goto out_report;
> +       }
> +
>         if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
> -               dev_err(priv->dev, "Error configuring FPGA\n");
> -               err = -EFAULT;
> +               why = "DMA did not complete";
> +               err = -EIO;
> +               goto out_report;
>         }
>
> +       err = 0;
> +       goto out_clk;
> +
> +out_report:
> +       dev_err(priv->dev,
> +               "%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
> +               why,
> +               intr_status,
> +               zynq_fpga_read(priv, CTRL_OFFSET),
> +               zynq_fpga_read(priv, LOCK_OFFSET),
> +               zynq_fpga_read(priv, INT_MASK_OFFSET),
> +               zynq_fpga_read(priv, STATUS_OFFSET),
> +               zynq_fpga_read(priv, MCTRL_OFFSET));
> +out_clk:
>         clk_disable(priv->clk);
> -

Personally found it more readable with newline.

>  out_free:
>         dma_free_coherent(priv->dev, count, kbuf, dma_addr);
> -

Same here.
>         return err;
>  }
>
> @@ -475,7 +482,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>         /* unlock the device */
>         zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
>
> -       zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);
> +       zynq_fpga_set_irq_mask(priv, 0);
>         zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
>         err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
>                                priv);
> --
> 2.1.4
>

Thanks,

Moritz
Jason Gunthorpe Nov. 17, 2016, 6:28 p.m. UTC | #2 
On Wed, Nov 16, 2016 at 10:10:35PM -0800, Moritz Fischer wrote:
> >   /* FPGA programmed */
> >  #define IXR_PCFG_DONE_MASK             BIT(2)
> > -#define IXR_ERROR_FLAGS_MASK           0x00F0F860
> > +#define IXR_ERROR_FLAGS_MASK           0x00F0C860
> 
> True. Ouch.

Yeah, this only works at all by accident..


> > -static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
> > +static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
> > +                                         u32 enable)
> >  {
> > -       u32 intr_mask;
> > -
> > -       intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
> > -       zynq_fpga_write(priv, INT_MASK_OFFSET,
> > -                       intr_mask | IXR_DMA_DONE_MASK | IXR_ERROR_FLAGS_MASK);
> > -}
> > +       zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
> 
> Not a fan of this ~enable here. Function name indicates you're doing
> the opposite

Lets call it zynq_fpga_set_irq then

The ~ is best placed here because it is after the compiler has coerced
the argument into u32 so the ~ will always do the right
thing

I've been reading the IXR_*_MASK as indicating it is a bit pattern for
the INT_MASK register not as actually meaning literal masking.

> > +out_clk:
> >         clk_disable(priv->clk);
> > -
> 
> Personally found it more readable with newline.

sure

Jason
diff mbox

Patch

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 40cf0feaca7c..3ffc5fcc3072 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -89,7 +89,7 @@ 
 #define IXR_D_P_DONE_MASK		BIT(12)
  /* FPGA programmed */
 #define IXR_PCFG_DONE_MASK		BIT(2)
-#define IXR_ERROR_FLAGS_MASK		0x00F0F860
+#define IXR_ERROR_FLAGS_MASK		0x00F0C860
 #define IXR_ALL_MASK			0xF8F7F87F
 
 /* Miscellaneous constant values */
@@ -144,23 +144,10 @@  static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv,
 	readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \
 			   timeout_us)
 
-static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
+static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
+					  u32 enable)
 {
-	u32 intr_mask;
-
-	intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
-	zynq_fpga_write(priv, INT_MASK_OFFSET,
-			intr_mask | IXR_DMA_DONE_MASK | IXR_ERROR_FLAGS_MASK);
-}
-
-static void zynq_fpga_unmask_irqs(struct zynq_fpga_priv *priv)
-{
-	u32 intr_mask;
-
-	intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
-	zynq_fpga_write(priv, INT_MASK_OFFSET,
-			intr_mask
-			& ~(IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK));
+	zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
 static irqreturn_t zynq_fpga_isr(int irq, void *data)
@@ -168,7 +155,7 @@  static irqreturn_t zynq_fpga_isr(int irq, void *data)
 	struct zynq_fpga_priv *priv = data;
 
 	/* disable DMA and error IRQs */
-	zynq_fpga_mask_irqs(priv);
+	zynq_fpga_set_irq_mask(priv, 0);
 
 	complete(&priv->dma_done);
 
@@ -314,6 +301,7 @@  static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 			       const char *buf, size_t count)
 {
 	struct zynq_fpga_priv *priv;
+	const char *why;
 	int err;
 	char *kbuf;
 	dma_addr_t dma_addr;
@@ -337,7 +325,7 @@  static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	reinit_completion(&priv->dma_done);
 
 	/* enable DMA and error IRQs */
-	zynq_fpga_unmask_irqs(priv);
+	zynq_fpga_set_irq_mask(priv, IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK);
 
 	/* the +1 in the src addr is used to hold off on DMA_DONE IRQ
 	 * until both AXI and PCAP are done ...
@@ -352,16 +340,35 @@  static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
 	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
 
+	if (intr_status & IXR_ERROR_FLAGS_MASK) {
+		why = "DMA reported error";
+		err = -EIO;
+		goto out_report;
+	}
+
 	if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
-		dev_err(priv->dev, "Error configuring FPGA\n");
-		err = -EFAULT;
+		why = "DMA did not complete";
+		err = -EIO;
+		goto out_report;
 	}
 
+	err = 0;
+	goto out_clk;
+
+out_report:
+	dev_err(priv->dev,
+		"%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
+		why,
+		intr_status,
+		zynq_fpga_read(priv, CTRL_OFFSET),
+		zynq_fpga_read(priv, LOCK_OFFSET),
+		zynq_fpga_read(priv, INT_MASK_OFFSET),
+		zynq_fpga_read(priv, STATUS_OFFSET),
+		zynq_fpga_read(priv, MCTRL_OFFSET));
+out_clk:
 	clk_disable(priv->clk);
-
 out_free:
 	dma_free_coherent(priv->dev, count, kbuf, dma_addr);
-
 	return err;
 }
 
@@ -475,7 +482,7 @@  static int zynq_fpga_probe(struct platform_device *pdev)
 	/* unlock the device */
 	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
 
-	zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);
+	zynq_fpga_set_irq_mask(priv, 0);
 	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
 	err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
 			       priv);