Date:	Fri, 9 Oct 2015 11:33:36 -0500
From:	Josh Cartwright <joshc@...com>
To:	Moritz Fischer <moritz.fischer@...us.com>
Cc:	michal.simek@...inx.com, soren.brinkmann@...inx.com,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	linux@....linux.org.uk, dinguyen@...nsource.altera.com,
	atull@...nsource.altera.com, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	gregkh@...uxfoundation.org
Subject: Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx
 Zynq 7000

Hey Moritz-

On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code heavily borrows from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@...us.com>
[..]
> +++ b/drivers/fpga/zynq-fpga.c
[..]
> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
> +{
> +	u32 intr_status;
> +	struct zynq_fpga_priv *priv = data;
> +
> +	spin_lock(&priv->lock);

I'm confused about the locking here.  You have this lock, but it's only
used in the isr.  It's claimed purpose is to protect 'error', but that
clearly isn't happening.

> +	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
> +
> +	if (!intr_status) {
> +		spin_unlock(&priv->lock);
> +		return IRQ_NONE;
> +	}
> +
> +	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
> +
> +	if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
> +		complete(&priv->dma_done);

Just so I understand, wouldn't you also want to complete() in the error
case, too?

> +	if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
> +			IXR_ERROR_FLAGS_MASK) {
> +		priv->error = true;
> +		dev_err(priv->dev, "%s dma error\n", __func__);
> +	}
> +	spin_unlock(&priv->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +				    const char *buf, size_t count)
> +{
> +	struct zynq_fpga_priv *priv;
> +	u32 ctrl, status;
> +	int err;
> +
> +	priv = mgr->priv;
> +
> +	err = clk_enable(priv->clk);
> +	if (err)
> +		return err;
> +
> +	/* only reset if we're not doing partial reconfig */
> +	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> +		/* assert AXI interface resets */
> +		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +			     FPGA_RST_ALL_MASK);
> +
> +		/* disable level shifters */
> +		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +			     LVL_SHFTR_DISABLE_ALL_MASK);
> +		/* enable output level shifters */
> +		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +			     LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> +		/* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +		 * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +		 * to make sure the rising edge actually happens
> +		 */
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +				       STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +				       STATUS_PCFG_INIT_MASK), 20, 0);

And if we timeout?

> +
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +				       STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +	}
> +
> +	clk_disable(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +			       const char *buf, size_t count)
> +{
> +	struct zynq_fpga_priv *priv;
> +	int err;
> +	char *kbuf;
> +	size_t i, in_count;
> +	dma_addr_t dma_addr;
> +	u32 transfer_length = 0;
> +	bool endian_swap = false;
> +
> +	in_count = count;
> +	priv = mgr->priv;
> +
> +	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	memcpy(kbuf, buf, count);
> +
> +	/* look for the sync word */
> +	for (i = 0; i < count - 4; i++) {
> +		if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> +			dev_dbg(priv->dev, "Found normal sync word\n");
> +			endian_swap = false;
> +			break;
> +		}
> +		if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> +			dev_dbg(priv->dev, "Found swapped sync word\n");
> +			endian_swap = true;
> +			break;
> +		}
> +	}

How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.

> +	/* remove the header, align the data on word boundary */
> +	if (i != count - 4) {
> +		count -= i;
> +		memmove(kbuf, kbuf + i, count);
> +	}
> +
> +	/* fixup endianness of the data */
> +	if (endian_swap) {
> +		for (i = 0; i < count; i += 4) {

Aren't we writing beyond the buffer, if count isn't word-aligned?

> +			u32 *p = (u32 *)&kbuf[i];
> +			*p = swab32(*p);
> +		}
> +	}
> +
> +	/* enable clock */
> +	err = clk_enable(priv->clk);
> +	if (err)
> +		goto out_free;
> +
> +	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
> +
> +	/* enable DMA and error IRQs */
> +	zynq_fpga_unmask_irqs(priv);
> +
> +	priv->error = false;
> +
> +	/* the +1 in the src addr is used to hold off on DMA_DONE IRQ */
> +	/* until both AXI and PCAP are done */
> +	if (count < PAGE_SIZE)
> +		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr + 1));
> +	else
> +		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr));
> +
> +	zynq_fpga_write(priv, DMA_DEST_ADDR_OFFSET, (u32)DMA_INVALID_ADDRESS);
> +
> +	/* convert #bytes to #words */
> +	transfer_length = (count + 3) / 4;
> +
> +	zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, transfer_length);
> +	zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
> +
> +	wait_for_completion_interruptible(&priv->dma_done);

And if we're interrupted?

> +	if (priv->error) {
> +		dev_err(priv->dev, "Error configuring FPGA.\n");
> +		err = -EFAULT;
> +	}
> +
> +	/* disable DMA and error IRQs */
> +	zynq_fpga_mask_irqs(priv);
> +
> +	clk_disable(priv->clk);
> +
> +out_free:
> +	dma_free_coherent(priv->dev, in_count, kbuf, dma_addr);
> +
> +	return err;
> +}
[..]
> +static int zynq_fpga_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct zynq_fpga_priv *priv;
> +	struct resource *res;
> +	u32 ctrl_reg;
> +	int ret;
> +
[..]
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0) {
> +		dev_err(dev, "No IRQ available");
> +		return priv->irq;
> +	}
> +
> +	ret = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
> +			       dev_name(dev), priv);
> +	if (IS_ERR_VALUE(ret))

This is the wrong check for error in this case.  You should check 'ret'
being non-zero.

> +		return ret;
> +
> +	priv->clk = devm_clk_get(dev, "ref_clk");
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "input clock not found\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "unable to enable clock\n");
> +		return ret;
> +	}
> +
> +	/* unlock the device */
> +	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
> +
> +	/* set configuration register with following options:
> +	* - reset FPGA
> +	* - enable PCAP interface for partial reconfig
> +	* - set throughput for maximum speed
> +	* - set CPU in user mode
> +	*/
> +	ctrl_reg = zynq_fpga_read(priv, CTRL_OFFSET);
> +	zynq_fpga_write(priv, CTRL_OFFSET, (CTRL_PCFG_PROG_B_MASK |
> +		CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl_reg));
> +
> +	/* ensure internal PCAP loopback is disabled */
> +	ctrl_reg = zynq_fpga_read(priv, MCTRL_OFFSET);
> +	zynq_fpga_write(priv, MCTRL_OFFSET, (~MCTRL_PCAP_LPBK_MASK & ctrl_reg));

Why do all of this initialization in probe()?  Is it necessary to read
FPGA state()?
>
> +
> +	ret = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
> +				&zynq_fpga_ops, priv);
> +	if (ret) {
> +		dev_err(dev, "unable to register FPGA manager");
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}

I would have expected the clock to have been disabled after even a
successful probe.

> +	return 0;
> +}
> +
> +static int zynq_fpga_remove(struct platform_device *pdev)
> +{
> +	fpga_mgr_unregister(&pdev->dev);

Your clock management is unbalanced.

  Josh

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists