Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

From: atull
Date: Fri Oct 09 2015 - 14:24:28 EST


On Thu, 8 Oct 2015, Moritz Fischer wrote:

> --- /dev/null
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -0,0 +1,478 @@
> +/*
> + * Copyright (c) 2011-2015 Xilinx Inc.
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> + * in their vendor tree.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>

Hi Moritz,

That was fast! I just have a couple of very minor comments...

Please alphabetize the #includes.

> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_FPGA_RST_CTRL_OFFSET 0x240 /* FPGA Software Reset Control */
> +#define SLCR_LVL_SHFTR_EN_OFFSET 0x900 /* Level Shifters Enable */
> +
> +/* Constant Definitions */
> +#define CTRL_OFFSET 0x00 /* Control Register */
> +#define LOCK_OFFSET 0x04 /* Lock Register */
> +#define INT_STS_OFFSET 0x0c /* Interrupt Status Register */
> +#define INT_MASK_OFFSET 0x10 /* Interrupt Mask Register */
> +#define STATUS_OFFSET 0x14 /* Status Register */
> +#define DMA_SRC_ADDR_OFFSET 0x18 /* DMA Source Address Register */
> +#define DMA_DEST_ADDR_OFFSET 0x1c /* DMA Destination Address Reg */
> +#define DMA_SRC_LEN_OFFSET 0x20 /* DMA Source Transfer Length */
> +#define DMA_DEST_LEN_OFFSET 0x24 /* DMA Destination Transfer */
> +#define UNLOCK_OFFSET 0x34 /* Unlock Register */
> +#define MCTRL_OFFSET 0x80 /* Misc. Control Register */

Please fix up the indenting.

> +
> +/* Control Register Bit definitions */
> +#define CTRL_PCFG_PROG_B_MASK BIT(30) /* Program signal to reset FPGA */
> +#define CTRL_PCAP_PR_MASK BIT(27) /* Enable PCAP for PR */
> +#define CTRL_PCAP_MODE_MASK BIT(26) /* Enable PCAP */
> +
> +/* Miscellaneous Control Register bit definitions */
> +#define MCTRL_PCAP_LPBK_MASK BIT(4) /* Internal PCAP loopback */
> +
> +/* Status register bit definitions */
> +#define STATUS_PCFG_INIT_MASK BIT(4) /* FPGA init status */
> +
> +/* Interrupt Status/Mask Register Bit definitions */
> +#define IXR_DMA_DONE_MASK BIT(13) /* DMA command done */
> +#define IXR_D_P_DONE_MASK BIT(12) /* DMA and PCAP cmd done */
> +#define IXR_PCFG_DONE_MASK BIT(2) /* FPGA programmed */
> +#define IXR_ERROR_FLAGS_MASK 0x00F0F860
> +#define IXR_ALL_MASK 0xF8F7F87F
> +
> +/* Miscellaneous constant values */
> +#define DMA_INVALID_ADDRESS GENMASK(31, 0) /* Invalid DMA address */
> +#define UNLOCK_MASK 0x757bdf0d /* Used to unlock the device */
> +
> +/* Masks for controlling stuff in SLCR */
> +#define LVL_SHFTR_DISABLE_ALL_MASK 0x0 /* Disable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PS_TO_PL 0xa /* Enable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PL_TO_PS 0xf /* Enable all Level shifters */
> +#define FPGA_RST_ALL_MASK 0xf /* Enable global resets */
> +#define FPGA_RST_NONE_MASK 0x0 /* Disable global resets */
> +

> +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;

You might not even need to enable/disable the clock if not doing PR.
Does this driver support both full reconfig and partial reconfig?

> +
> + /* 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);
> +
> + 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);
> +
> + 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);
> + }
> +
> + clk_disable(priv->clk);
> +
> + return 0;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/