| Message ID | 1478732303-13718-4-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jason, On Wed, Nov 9, 2016 at 2:58 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > It is best practice to clear and mask all interrupts before > associating the IRQ, and this should be done after the clock > is enabled. > > This corrects a bad result from zynq_fpga_ops_state on bootup > where left over latched values in INT_STS_OFFSET caused it to > report an unconfigured FPGA as configured. > > After this change the boot up operating state for an unconfigured > FPGA reports 'unknown'. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/fpga/zynq-fpga.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c > index 86f4377e2b52..40cf0feaca7c 100644 > --- a/drivers/fpga/zynq-fpga.c > +++ b/drivers/fpga/zynq-fpga.c > @@ -460,13 +460,6 @@ static int zynq_fpga_probe(struct platform_device *pdev) > return priv->irq; > } > > - err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, > - dev_name(dev), priv); > - if (err) { > - dev_err(dev, "unable to request IRQ\n"); > - return err; > - } > - > priv->clk = devm_clk_get(dev, "ref_clk"); > if (IS_ERR(priv->clk)) { > dev_err(dev, "input clock not found\n"); > @@ -482,6 +475,16 @@ 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); Named constant please. > + 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); > + if (err) { > + dev_err(dev, "unable to request IRQ\n"); > + clk_unprepare(priv->clk); Your enable count is off in that case. This should be a clk_disable_unprepare() call. <snip> clk_prepare_enable() if(err) { clk_unprepare(); return err; /* enable count = 1? <- huh? */ } clk_disable() enable count = 0 clk_unprepare() prepare count = 0 </snip> > + return err; > + } > + > clk_disable(priv->clk); > > err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager", > -- > 2.1.4 > Thanks, Moritz
On Thu, Nov 10, 2016 at 04:44:39PM -0800, Moritz Fischer wrote: > > + zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF); > > Named constant please. This line gets fixed in the next patch that, lets just leave it, less churn.. > > + 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); > > + if (err) { > > + dev_err(dev, "unable to request IRQ\n"); > > + clk_unprepare(priv->clk); > > Your enable count is off in that case. This should be a clk_disable_unprepare() > call. Yep. FWIW, it feels wrong to leave the ISR enabled but the clk disabled.. Jason
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c index 86f4377e2b52..40cf0feaca7c 100644 --- a/drivers/fpga/zynq-fpga.c +++ b/drivers/fpga/zynq-fpga.c @@ -460,13 +460,6 @@ static int zynq_fpga_probe(struct platform_device *pdev) return priv->irq; } - err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, - dev_name(dev), priv); - if (err) { - dev_err(dev, "unable to request IRQ\n"); - return err; - } - priv->clk = devm_clk_get(dev, "ref_clk"); if (IS_ERR(priv->clk)) { dev_err(dev, "input clock not found\n"); @@ -482,6 +475,16 @@ 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_write(priv, INT_STS_OFFSET, IXR_ALL_MASK); + err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev), + priv); + if (err) { + dev_err(dev, "unable to request IRQ\n"); + clk_unprepare(priv->clk); + return err; + } + clk_disable(priv->clk); err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
It is best practice to clear and mask all interrupts before associating the IRQ, and this should be done after the clock is enabled. This corrects a bad result from zynq_fpga_ops_state on bootup where left over latched values in INT_STS_OFFSET caused it to report an unconfigured FPGA as configured. After this change the boot up operating state for an unconfigured FPGA reports 'unknown'. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/fpga/zynq-fpga.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)