| Message ID | 1444344307-22509-4-git-send-email-moritz.fischer@ettus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ettus.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
Hi Josh, thanks for the review! On Fri, Oct 9, 2015 at 6:33 PM, Josh Cartwright <joshc@ni.com> wrote: > 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@ettus.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. Ouch, yes ... > >> + 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? Ehrm ... yes. Definitely. > >> + 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? Ehrm ... then we should cleanup & return ... > >> + >> + 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? See above ... > >> + >> + 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? Ok ok ... got it... > >> + } >> + >> + 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. That's a good question. Personally I do only care about one of both, but that's just because I get to decide for my targets... Opinions from the Xilinx guys? > >> + /* 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? One should deal with that ... > >> + 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. Good catch, will fix ... > >> + 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()? Hmmm ... good catch, again. Will rework... >> >> + >> + 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. Yes, you're right. Will do. > >> + return 0; >> +} >> + >> +static int zynq_fpga_remove(struct platform_device *pdev) >> +{ >> + fpga_mgr_unregister(&pdev->dev); > > Your clock management is unbalanced. Arghh > > Josh Thanks for reviewing, seems like I got one or two things to fix ;-) Moritz
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; > +} > +
On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote: > 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. Bah, who cares about that, it's not a requirement at all. greg k-h
On Fri, 9 Oct 2015, Greg KH wrote: > On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote: > > 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. > > Bah, who cares about that, it's not a requirement at all. > > greg k-h > Nice to know! I get slammed every time for that! Alan
Hi Alan, thanks for your feedback! On Fri, Oct 9, 2015 at 8:09 PM, atull <atull@opensource.altera.com> wrote: > 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. Will do. > >> + >> +/* 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. Yeah, you're probably right. > >> + >> + /* 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; >> +} >> + Cheers & enjoy your vacation Moritz
>>> +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; >>> + } This is bin format >>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) { >>> + dev_dbg(priv->dev, "Found swapped sync word\n"); >>> + endian_swap = true; >>> + break; >>> + } This is bit format from header >>> + } >> >> 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. > > That's a good question. Personally I do only care about one of both, > but that's just because I get to decide for my targets... > Opinions from the Xilinx guys? Don't know full history about this but in past bitstream in BIT format was used. Which is header (partially decoding in u-boot for example) with data. On zynq native format is BIN which is format without header and data is swapped. This code just detects which format is used. If BIT, header is skipped and data is swapped to BIN format. Back to origin question if this is something what can be handled from user space. And answer is - yes it can be handled there. But based on my experience it is very useful to be able to handle BIT because it is built by tools by default. Also with BIN format you are loosing record what this data bitstream targets. Header in BIT gives you at least some ideas. Thanks, Michal
?On 12-10-15 13:16, Michal Simek wrote: > >>>> +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; >>>> + } > > This is bin format > >>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) { >>>> + dev_dbg(priv->dev, "Found swapped sync word\n"); >>>> + endian_swap = true; >>>> + break; >>>> + } > > This is bit format from header > >>>> + } >>> >>> 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. >> >> That's a good question. Personally I do only care about one of both, >> but that's just because I get to decide for my targets... >> Opinions from the Xilinx guys? > > Don't know full history about this but in past bitstream in BIT format > was used. Which is header (partially decoding in u-boot for example) > with data. > On zynq native format is BIN which is format without header and data is > swapped. > This code just detects which format is used. If BIT, header is skipped > and data is swapped to BIN format. > > Back to origin question if this is something what can be handled from > user space. And answer is - yes it can be handled there. > But based on my experience it is very useful to be able to handle BIT > because it is built by tools by default. > Also with BIN format you are loosing record what this data bitstream > targets. Header in BIT gives you at least some ideas. People should stop using "cat" to program the FPGA and use a userspace tool instead. I've already released such tools under GPL, so anyone can pick up on it and extend it as required. The header for the "bit" format is completely ignored (you can't even use it to determine if the bitstream is compatible with the current device) so there's no point in carrying it around. On the zynq, doing the "swap" in userspace was measurably faster than having the driver handle it, and that was even without using NEON instructions for byte swapping. I admit that being able to do "cat static.bit > /dev/xdevcfg" has had its uses. But it's not something that belongs in mainline Linux. Probably one of the key reasons that the "bit" format is still popular is that getting the Vivado tools to create a proper "bin" that will actually work on the Zynq is about as easy as nailing jelly to a tree. We've been using a simple Python script to do the bit->bin conversion for that reason. Using the "bin" format in the driver keeps it simple and singular. Userspace tools can add whatever wrappers and headers they feel appropriate to it, these checks don't belong in the driver since they will be application specific. For example, some users would want to verify that a partial bitstream actually matches the static part that's currently in the FPGA. Mike. Kind regards, Mike Looijmans System Expert TOPIC Embedded Products Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 Telefax: +31 (0) 499 33 69 70 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
Hi Mike, On 10/12/2015 02:22 PM, Mike Looijmans wrote: > On 12-10-15 13:16, Michal Simek wrote: >> >>>>> +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; >>>>> + } >> >> This is bin format >> >>>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) { >>>>> + dev_dbg(priv->dev, "Found swapped sync word\n"); >>>>> + endian_swap = true; >>>>> + break; >>>>> + } >> >> This is bit format from header >> >>>>> + } >>>> >>>> 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. >>> >>> That's a good question. Personally I do only care about one of both, >>> but that's just because I get to decide for my targets... >>> Opinions from the Xilinx guys? >> >> Don't know full history about this but in past bitstream in BIT format >> was used. Which is header (partially decoding in u-boot for example) >> with data. >> On zynq native format is BIN which is format without header and data is >> swapped. >> This code just detects which format is used. If BIT, header is skipped >> and data is swapped to BIN format. >> >> Back to origin question if this is something what can be handled from >> user space. And answer is - yes it can be handled there. >> But based on my experience it is very useful to be able to handle BIT >> because it is built by tools by default. >> Also with BIN format you are loosing record what this data bitstream >> targets. Header in BIT gives you at least some ideas. > > People should stop using "cat" to program the FPGA and use a userspace > tool instead. I've already released such tools under GPL, so anyone can > pick up on it and extend it as required. Link? This is fpga manager based driver where "cat" won't be used. > > The header for the "bit" format is completely ignored (you can't even > use it to determine if the bitstream is compatible with the current > device) so there's no point in carrying it around. up2you what you want to do with it. If you work with different boards with different FPGAs it is at least helpful to know if X.bit target this or that board. Unfortunately I am not aware about any public document which describe what there is written. > On the zynq, doing > the "swap" in userspace was measurably faster than having the driver > handle it, and that was even without using NEON instructions for byte > swapping. > > I admit that being able to do "cat static.bit > /dev/xdevcfg" has had > its uses. But it's not something that belongs in mainline Linux. It is about comfort but I have really not a problem that driver will handle just BIN format. > Probably one of the key reasons that the "bit" format is still popular > is that getting the Vivado tools to create a proper "bin" that will > actually work on the Zynq is about as easy as nailing jelly to a tree. > We've been using a simple Python script to do the bit->bin conversion > for that reason. In vivado it is one tcl cmd. But truth is that I don't really get why BIN is not generated by default. > Using the "bin" format in the driver keeps it simple and singular. > Userspace tools can add whatever wrappers and headers they feel > appropriate to it, these checks don't belong in the driver since they > will be application specific. For example, some users would want to > verify that a partial bitstream actually matches the static part that's > currently in the FPGA. agree. Thanks, Michal
?On 12-10-15 14:38, Michal Simek wrote: > Hi Mike, > > On 10/12/2015 02:22 PM, Mike Looijmans wrote: >> On 12-10-15 13:16, Michal Simek wrote: >>> >>>>>> +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; >>>>>> + } >>> >>> This is bin format >>> >>>>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) { >>>>>> + dev_dbg(priv->dev, "Found swapped sync word\n"); >>>>>> + endian_swap = true; >>>>>> + break; >>>>>> + } >>> >>> This is bit format from header >>> >>>>>> + } >>>>> >>>>> 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. >>>> >>>> That's a good question. Personally I do only care about one of both, >>>> but that's just because I get to decide for my targets... >>>> Opinions from the Xilinx guys? >>> >>> Don't know full history about this but in past bitstream in BIT format >>> was used. Which is header (partially decoding in u-boot for example) >>> with data. >>> On zynq native format is BIN which is format without header and data is >>> swapped. >>> This code just detects which format is used. If BIT, header is skipped >>> and data is swapped to BIN format. >>> >>> Back to origin question if this is something what can be handled from >>> user space. And answer is - yes it can be handled there. >>> But based on my experience it is very useful to be able to handle BIT >>> because it is built by tools by default. >>> Also with BIN format you are loosing record what this data bitstream >>> targets. Header in BIT gives you at least some ideas. >> >> People should stop using "cat" to program the FPGA and use a userspace >> tool instead. I've already released such tools under GPL, so anyone can >> pick up on it and extend it as required. > > Link? https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261 Will need some work to combine into a single tool though. > This is fpga manager based driver where "cat" won't be used. Haven't looked into it yet, but I guess at some point one will have to stream some data from userspace into the device, right? >> The header for the "bit" format is completely ignored (you can't even >> use it to determine if the bitstream is compatible with the current >> device) so there's no point in carrying it around. > > up2you what you want to do with it. If you work with different boards > with different FPGAs it is at least helpful to know if X.bit target this > or that board. Unfortunately I am not aware about any public document > which describe what there is written. > >> On the zynq, doing >> the "swap" in userspace was measurably faster than having the driver >> handle it, and that was even without using NEON instructions for byte >> swapping. >> >> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had >> its uses. But it's not something that belongs in mainline Linux. > > It is about comfort but I have really not a problem that driver will > handle just BIN format. > >> Probably one of the key reasons that the "bit" format is still popular >> is that getting the Vivado tools to create a proper "bin" that will >> actually work on the Zynq is about as easy as nailing jelly to a tree. >> We've been using a simple Python script to do the bit->bin conversion >> for that reason. > > In vivado it is one tcl cmd. But truth is that I don't really get why > BIN is not generated by default. If I recall correctly, Vivado strips the "bit" header but doesn't swap the bytes, so the resulting bin file won't work. >> Using the "bin" format in the driver keeps it simple and singular. >> Userspace tools can add whatever wrappers and headers they feel >> appropriate to it, these checks don't belong in the driver since they >> will be application specific. For example, some users would want to >> verify that a partial bitstream actually matches the static part that's >> currently in the FPGA. > > agree. > > Thanks, > Michal > > Kind regards, Mike Looijmans System Expert TOPIC Embedded Products Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 Telefax: +31 (0) 499 33 69 70 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
On 10/13/2015 07:33 AM, Mike Looijmans wrote: > On 12-10-15 14:38, Michal Simek wrote: >> Hi Mike, >> >> On 10/12/2015 02:22 PM, Mike Looijmans wrote: >>> On 12-10-15 13:16, Michal Simek wrote: >>>> >>>>>>> +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; >>>>>>> + } >>>> >>>> This is bin format >>>> >>>>>>> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) { >>>>>>> + dev_dbg(priv->dev, "Found swapped sync >>>>>>> word\n"); >>>>>>> + endian_swap = true; >>>>>>> + break; >>>>>>> + } >>>> >>>> This is bit format from header >>>> >>>>>>> + } >>>>>> >>>>>> 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. >>>>> >>>>> That's a good question. Personally I do only care about one of both, >>>>> but that's just because I get to decide for my targets... >>>>> Opinions from the Xilinx guys? >>>> >>>> Don't know full history about this but in past bitstream in BIT format >>>> was used. Which is header (partially decoding in u-boot for example) >>>> with data. >>>> On zynq native format is BIN which is format without header and data is >>>> swapped. >>>> This code just detects which format is used. If BIT, header is skipped >>>> and data is swapped to BIN format. >>>> >>>> Back to origin question if this is something what can be handled from >>>> user space. And answer is - yes it can be handled there. >>>> But based on my experience it is very useful to be able to handle BIT >>>> because it is built by tools by default. >>>> Also with BIN format you are loosing record what this data bitstream >>>> targets. Header in BIT gives you at least some ideas. >>> >>> People should stop using "cat" to program the FPGA and use a userspace >>> tool instead. I've already released such tools under GPL, so anyone can >>> pick up on it and extend it as required. >> >> Link? > > https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp > > https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261 > > > Will need some work to combine into a single tool though. > >> This is fpga manager based driver where "cat" won't be used. > > Haven't looked into it yet, but I guess at some point one will have to > stream some data from userspace into the device, right? Currently loading bitstream via firmware interface is used. > >>> The header for the "bit" format is completely ignored (you can't even >>> use it to determine if the bitstream is compatible with the current >>> device) so there's no point in carrying it around. >> >> up2you what you want to do with it. If you work with different boards >> with different FPGAs it is at least helpful to know if X.bit target this >> or that board. Unfortunately I am not aware about any public document >> which describe what there is written. >> >>> On the zynq, doing >>> the "swap" in userspace was measurably faster than having the driver >>> handle it, and that was even without using NEON instructions for byte >>> swapping. >>> >>> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had >>> its uses. But it's not something that belongs in mainline Linux. >> >> It is about comfort but I have really not a problem that driver will >> handle just BIN format. >> >>> Probably one of the key reasons that the "bit" format is still popular >>> is that getting the Vivado tools to create a proper "bin" that will >>> actually work on the Zynq is about as easy as nailing jelly to a tree. >>> We've been using a simple Python script to do the bit->bin conversion >>> for that reason. >> >> In vivado it is one tcl cmd. But truth is that I don't really get why >> BIN is not generated by default. > > If I recall correctly, Vivado strips the "bit" header but doesn't swap > the bytes, so the resulting bin file won't work. I have built bitstream from vivado 2014.4 and if you dump bit you will see this. All stuff in front of 0xaa995566 at 0xa0 is header. 00000000 00 09 0f f0 0f f0 0f f0 0f f0 00 00 01 61 00 32 |.............a.2| 00000010 64 65 73 69 67 6e 5f 31 5f 77 72 61 70 70 65 72 |design_1_wrapper| 00000020 3b 55 73 65 72 49 44 3d 30 58 46 46 46 46 46 46 |;UserID=0XFFFFFF| 00000030 46 46 3b 56 65 72 73 69 6f 6e 3d 32 30 31 34 2e |FF;Version=2014.| 00000040 34 00 62 00 0c 37 7a 30 32 30 63 6c 67 34 38 34 |4.b..7z020clg484| 00000050 00 63 00 0b 32 30 31 35 2f 31 30 2f 31 33 00 64 |.c..2015/10/13.d| 00000060 00 09 31 30 3a 35 33 3a 33 38 00 65 00 3d ba fc |..10:53:38.e.=..| 00000070 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| * 00000090 00 00 00 bb 11 22 00 44 ff ff ff ff ff ff ff ff |.....".D........| 000000a0 aa 99 55 66 20 00 00 00 30 02 20 01 00 00 00 00 |..Uf ...0. .....| On the other hand bin format has also small header which can be skipped for devcfg usage but doesn't need to be (I have tested it) I do use this command for bitstream in bin format generation (it was in edk 14.7). promgen -p bin -data_width 32 -b -u 0x0 *.bit -w 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| * 00000020 bb 00 00 00 44 00 22 11 ff ff ff ff ff ff ff ff |....D.".........| 00000030 66 55 99 aa 00 00 00 20 01 20 02 30 00 00 00 00 |fU..... . .0....| 00000040 01 00 02 30 00 00 00 00 01 80 00 30 00 00 00 00 |...0.......0....| 00000050 00 00 00 20 01 80 00 30 07 00 00 00 00 00 00 20 |... ...0....... | 00000060 00 00 00 20 01 60 02 30 00 00 00 00 01 20 01 30 |... .`.0..... .0| 00000070 e5 3f 00 02 01 c0 01 30 00 00 00 00 01 80 01 30 |.?.....0.......0| 00000080 93 70 72 03 01 80 00 30 09 00 00 00 00 00 00 20 |.pr....0....... | 00000090 01 c0 00 30 01 04 00 00 01 a0 00 30 01 05 00 00 |...0.......0....| 000000a0 01 c0 00 30 00 00 00 00 01 00 03 30 00 00 00 00 |...0.......0....| 000000b0 00 00 00 20 00 00 00 20 00 00 00 20 00 00 00 20 |... ... ... ... | Thanks, Michal
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index dfc1f1e..c9b9fdf 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -19,6 +19,11 @@ config FPGA_MGR_SOCFPGA help FPGA manager driver support for Altera SOCFPGA. +config FPGA_MGR_ZYNQ_FPGA + tristate "Xilinx Zynq FPGA" + help + FPGA manager driver support for Xilinx Zynq FPGAs. + endif # FPGA endmenu diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index ba6c5c5..8d83fc6 100644 --- a/drivers/fpga/Makefile +++ b/drivers/fpga/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_FPGA) += fpga-mgr.o # FPGA Manager Drivers obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o +obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c new file mode 100644 index 0000000..7d0d703 --- /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> + +/* 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 */ + +/* 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 */ + +struct zynq_fpga_priv { + struct device *dev; + int irq; + struct clk *clk; + + void __iomem *io_base; + struct regmap *slcr; + + /* this protects the error flag */ + spinlock_t lock; + bool error; + + struct completion dma_done; +}; + +static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset, + u32 val) +{ + writel(val, priv->io_base + offset); +} + +static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv, + u32 offset) +{ + return readl(priv->io_base + offset); +} + +#define zynq_fpga_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us) \ + readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \ + timeout_us) + +static void zynq_fpga_mask_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_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)); +} + +static irqreturn_t zynq_fpga_isr(int irq, void *data) +{ + u32 intr_status; + struct zynq_fpga_priv *priv = data; + + spin_lock(&priv->lock); + 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); + + 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); + + 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; +} + +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; + } + } + + /* 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) { + 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); + 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_ops_write_complete(struct fpga_manager *mgr, u32 flags) +{ + struct zynq_fpga_priv *priv = mgr->priv; + int err; + u32 intr_status; + + err = clk_enable(priv->clk); + if (err) + return err; + + err = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, intr_status, + intr_status & IXR_PCFG_DONE_MASK, 20, + 10000); + clk_disable(priv->clk); + + if (err) + return err; + + /* only in not partial reconfiguration do we need to deassert reset */ + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) { + /* enable all level shifters */ + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET, + LVL_SHFTR_ENABLE_PL_TO_PS); + + /* deassert AXI interface resets */ + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET, + FPGA_RST_NONE_MASK); + } + + return 0; +} + +static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr) +{ + int err; + u32 intr_status; + struct zynq_fpga_priv *priv; + + priv = mgr->priv; + + err = clk_enable(priv->clk); + if (err) + return FPGA_MGR_STATE_UNKNOWN; + + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET); + clk_disable(priv->clk); + + if (intr_status & IXR_PCFG_DONE_MASK) + return FPGA_MGR_STATE_OPERATING; + + return FPGA_MGR_STATE_UNKNOWN; +} + +static const struct fpga_manager_ops zynq_fpga_ops = { + .state = zynq_fpga_ops_state, + .write_init = zynq_fpga_ops_write_init, + .write = zynq_fpga_ops_write, + .write_complete = zynq_fpga_ops_write_complete, +}; + +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 = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + platform_set_drvdata(pdev, priv); + priv->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv->io_base = devm_ioremap_resource(dev, res); + if (IS_ERR(priv->io_base)) + return PTR_ERR(priv->io_base); + + priv->slcr = syscon_regmap_lookup_by_phandle(dev->of_node, + "syscon"); + if (IS_ERR(priv->slcr)) { + dev_err(dev, "unable to get zynq-slcr regmap"); + return PTR_ERR(priv->slcr); + } + + init_completion(&priv->dma_done); + + 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)) + 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)); + + 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; + } + return 0; +} + +static int zynq_fpga_remove(struct platform_device *pdev) +{ + fpga_mgr_unregister(&pdev->dev); + + return 0; +} + +static const struct of_device_id zynq_fpga_of_match[] = { + { .compatible = "xlnx,zynq-devcfg-1.0", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, zynq_fpga_of_match); + +static struct platform_driver zynq_fpga_driver = { + .probe = zynq_fpga_probe, + .remove = zynq_fpga_remove, + .driver = { + .name = "zynq_fpga_manager", + .of_match_table = of_match_ptr(zynq_fpga_of_match), + }, +}; + +module_platform_driver(zynq_fpga_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>"); +MODULE_AUTHOR("Michal Simek <michal.simek@xilinx.com>"); +MODULE_DESCRIPTION("Xilinx Zynq FPGA Manager"); +MODULE_ALIAS("fpga:zynq");
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@ettus.com> --- drivers/fpga/Kconfig | 5 + drivers/fpga/Makefile | 1 + drivers/fpga/zynq-fpga.c | 478 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 484 insertions(+) create mode 100644 drivers/fpga/zynq-fpga.c