On Wed, 10 Dec 2014, Michal Simek wrote: > On 12/09/2014 09:14 PM, atull@xxxxxxxxxxxxxxxxxxxxx wrote: > > From: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> > > > > Add driver to fpga manager framework to allow configuration > > of FPGA in Altera SoC FPGA parts. > > > > Signed-off-by: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> > > --- > > v2: fpga_manager struct now contains struct device > > fpga_manager_register parameters now take device > > > > v3: move to drivers/staging > > here should be that v4 is completely new patch in this series > as you are describing in cover letter. > > > --- > > drivers/staging/fpga/Kconfig | 6 + > > drivers/staging/fpga/Makefile | 1 + > > drivers/staging/fpga/altera.c | 789 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 796 insertions(+) > > create mode 100644 drivers/staging/fpga/altera.c > > > > diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig > > index e775b17..2944e3c 100644 > > --- a/drivers/staging/fpga/Kconfig > > +++ b/drivers/staging/fpga/Kconfig > > @@ -18,4 +18,10 @@ config FPGA_MGR_SYSFS > > help > > FPGA Manager SysFS interface. > > > > +config FPGA_MGR_ALTERA > > + tristate "Altera" > > + depends on FPGA > > + help > > + FPGA manager driver support for configuring Altera FPGAs. > > + > > endmenu > > diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile > > index c8a676f..6e0d2bf 100644 > > --- a/drivers/staging/fpga/Makefile > > +++ b/drivers/staging/fpga/Makefile > > @@ -8,3 +8,4 @@ fpga-mgr-core-y += fpga-mgr.o > > obj-$(CONFIG_FPGA) += fpga-mgr-core.o > > > > # FPGA Manager Drivers > > +obj-$(CONFIG_FPGA_MGR_ALTERA) += altera.o > > diff --git a/drivers/staging/fpga/altera.c b/drivers/staging/fpga/altera.c > > new file mode 100644 > > index 0000000..14a16b4 > > --- /dev/null > > +++ b/drivers/staging/fpga/altera.c > > @@ -0,0 +1,789 @@ > > +/* > > + * FPGA Manager Driver for Altera FPGAs > > + * > > + * Copyright (C) 2013-2014 Altera Corporation > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms and conditions of the GNU General Public License, > > + * version 2, as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope 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. > > + * > > + * You should have received a copy of the GNU General Public License along with > > + * this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > +#include <linux/cdev.h> > > +#include <linux/completion.h> > > +#include <linux/delay.h> > > +#include <linux/fs.h> > > +#include <linux/fpga-mgr.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/irq.h> > > +#include <linux/kernel.h> > > +#include <linux/list.h> > > +#include <linux/mm.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_device.h> > > +#include <linux/of_irq.h> > > +#include <linux/of_platform.h> > > +#include <linux/pm.h> > > +#include <linux/slab.h> > > +#include <linux/string.h> > > +#include <linux/types.h> > > +#include <linux/regulator/consumer.h> > > This is pretty long list I believe you will be able to shorten it a little bit. > I've done this now in v5. > > + > > +/* Controls whether to use the Configuration with DCLK steps */ > > +#ifndef _ALTTERA_FPGAMGR_USE_DCLK > > +#define _ALTTERA_FPGAMGR_USE_DCLK 0 > > +#endif > > This is more or less config option which you should reflect in binding. > I just removed the unused code. > > + > > +/* Register offsets */ > > +#define ALTERA_FPGAMGR_STAT_OFST 0x0 > > +#define ALTERA_FPGAMGR_CTL_OFST 0x4 > > +#define ALTERA_FPGAMGR_DCLKCNT_OFST 0x8 > > +#define ALTERA_FPGAMGR_DCLKSTAT_OFST 0xc > > +#define ALTERA_FPGAMGR_GPIO_INTEN_OFST 0x830 > > +#define ALTERA_FPGAMGR_GPIO_INTMSK_OFST 0x834 > > +#define ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST 0x838 > > +#define ALTERA_FPGAMGR_GPIO_INT_POL_OFST 0x83c > > +#define ALTERA_FPGAMGR_GPIO_INTSTAT_OFST 0x840 > > +#define ALTERA_FPGAMGR_GPIO_RAW_INTSTAT_OFST 0x844 > > +#define ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST 0x84c > > +#define ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST 0x850 > > + > > +/* Register bit defines */ > > +/* ALTERA_FPGAMGR_STAT register mode field values */ > > +#define ALTERA_FPGAMGR_STAT_POWER_UP 0x0 /*ramping*/ > > +#define ALTERA_FPGAMGR_STAT_RESET 0x1 > > +#define ALTERA_FPGAMGR_STAT_CFG 0x2 > > +#define ALTERA_FPGAMGR_STAT_INIT 0x3 > > +#define ALTERA_FPGAMGR_STAT_USER_MODE 0x4 > > +#define ALTERA_FPGAMGR_STAT_UNKNOWN 0x5 > > +#define ALTERA_FPGAMGR_STAT_STATE_MASK 0x7 > > +/* This is a flag value that doesn't really happen in this register field */ > > +#define ALTERA_FPGAMGR_STAT_POWER_OFF 0x0 > > + > > +#define MSEL_PP16_FAST_NOAES_NODC 0x0 > > +#define MSEL_PP16_FAST_AES_NODC 0x1 > > +#define MSEL_PP16_FAST_AESOPT_DC 0x2 > > +#define MSEL_PP16_SLOW_NOAES_NODC 0x4 > > +#define MSEL_PP16_SLOW_AES_NODC 0x5 > > +#define MSEL_PP16_SLOW_AESOPT_DC 0x6 > > +#define MSEL_PP32_FAST_NOAES_NODC 0x8 > > +#define MSEL_PP32_FAST_AES_NODC 0x9 > > +#define MSEL_PP32_FAST_AESOPT_DC 0xa > > +#define MSEL_PP32_SLOW_NOAES_NODC 0xc > > +#define MSEL_PP32_SLOW_AES_NODC 0xd > > +#define MSEL_PP32_SLOW_AESOPT_DC 0xe > > +#define ALTERA_FPGAMGR_STAT_MSEL_MASK 0x000000f8 > > +#define ALTERA_FPGAMGR_STAT_MSEL_SHIFT 3 > > + > > +/* ALTERA_FPGAMGR_CTL register */ > > +#define ALTERA_FPGAMGR_CTL_EN 0x00000001 > > +#define ALTERA_FPGAMGR_CTL_NCE 0x00000002 > > +#define ALTERA_FPGAMGR_CTL_NCFGPULL 0x00000004 > > + > > +#define CDRATIO_X1 0x00000000 > > +#define CDRATIO_X2 0x00000040 > > +#define CDRATIO_X4 0x00000080 > > +#define CDRATIO_X8 0x000000c0 > > +#define ALTERA_FPGAMGR_CTL_CDRATIO_MASK 0x000000c0 > > + > > +#define ALTERA_FPGAMGR_CTL_AXICFGEN 0x00000100 > > + > > +#define CFGWDTH_16 0x00000000 > > +#define CFGWDTH_32 0x00000200 > > +#define ALTERA_FPGAMGR_CTL_CFGWDTH_MASK 0x00000200 > > + > > +/* ALTERA_FPGAMGR_DCLKSTAT register */ > > +#define ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE 0x1 > > + > > +/* ALTERA_FPGAMGR_GPIO_* registers share the same bit positions */ > > +#define ALTERA_FPGAMGR_MON_NSTATUS 0x0001 > > +#define ALTERA_FPGAMGR_MON_CONF_DONE 0x0002 > > +#define ALTERA_FPGAMGR_MON_INIT_DONE 0x0004 > > +#define ALTERA_FPGAMGR_MON_CRC_ERROR 0x0008 > > +#define ALTERA_FPGAMGR_MON_CVP_CONF_DONE 0x0010 > > +#define ALTERA_FPGAMGR_MON_PR_READY 0x0020 > > +#define ALTERA_FPGAMGR_MON_PR_ERROR 0x0040 > > +#define ALTERA_FPGAMGR_MON_PR_DONE 0x0080 > > +#define ALTERA_FPGAMGR_MON_NCONFIG_PIN 0x0100 > > +#define ALTERA_FPGAMGR_MON_NSTATUS_PIN 0x0200 > > +#define ALTERA_FPGAMGR_MON_CONF_DONE_PIN 0x0400 > > +#define ALTERA_FPGAMGR_MON_FPGA_POWER_ON 0x0800 > > +#define ALTERA_FPGAMGR_MON_STATUS_MASK 0x0fff > > + > > +#define ALTERA_FPGAMGR_NUM_SUPPLIES 3 > > +#define ALTERA_RESUME_TIMEOUT 3 > > + > > +#if IS_ENABLED(CONFIG_REGULATOR) > > +/* In power-up order. Reverse for power-down. */ > > +static const char *supply_names[ALTERA_FPGAMGR_NUM_SUPPLIES] = { > > + "FPGA-1.5V", > > + "FPGA-1.1V", > > + "FPGA-2.5V", > > +}; > > +#endif > > __maybe_unused should work here. > OK > > + > > +struct altera_fpga_priv { > > + void __iomem *fpga_base_addr; > > + void __iomem *fpga_data_addr; > > + struct completion status_complete; > > + int irq; > > + struct regulator *fpga_supplies[ALTERA_FPGAMGR_NUM_SUPPLIES]; > > +}; > > + > > +struct cfgmgr_mode { > > + /* Values to set in the CTRL register */ > > + u32 ctrl; > > + > > + /* flag that this table entry is a valid mode */ > > + bool valid; > > +}; > > + > > +/* For ALTERA_FPGAMGR_STAT_MSEL field */ > > +static struct cfgmgr_mode cfgmgr_modes[] = { > > + [MSEL_PP16_FAST_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 }, > > + [MSEL_PP16_FAST_AES_NODC] = { CFGWDTH_16 | CDRATIO_X2, 1 }, > > + [MSEL_PP16_FAST_AESOPT_DC] = { CFGWDTH_16 | CDRATIO_X4, 1 }, > > + [MSEL_PP16_SLOW_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 }, > > + [MSEL_PP16_SLOW_AES_NODC] = { CFGWDTH_16 | CDRATIO_X2, 1 }, > > + [MSEL_PP16_SLOW_AESOPT_DC] = { CFGWDTH_16 | CDRATIO_X4, 1 }, > > + [MSEL_PP32_FAST_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 }, > > + [MSEL_PP32_FAST_AES_NODC] = { CFGWDTH_32 | CDRATIO_X4, 1 }, > > + [MSEL_PP32_FAST_AESOPT_DC] = { CFGWDTH_32 | CDRATIO_X8, 1 }, > > + [MSEL_PP32_SLOW_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 }, > > + [MSEL_PP32_SLOW_AES_NODC] = { CFGWDTH_32 | CDRATIO_X4, 1 }, > > + [MSEL_PP32_SLOW_AESOPT_DC] = { CFGWDTH_32 | CDRATIO_X8, 1 }, > > +}; > > + > > +static u32 altera_fpga_reg_readl(struct altera_fpga_priv *priv, u32 reg_offset) > > +{ > > + return readl(priv->fpga_base_addr + reg_offset); > > +} > > + > > +static void altera_fpga_reg_writel(struct altera_fpga_priv *priv, > > + u32 reg_offset, u32 value) > > +{ > > + writel(value, priv->fpga_base_addr + reg_offset); > > +} > > + > > +static u32 altera_fpga_reg_raw_readl(struct altera_fpga_priv *priv, > > + u32 reg_offset) > > +{ > > + return __raw_readl(priv->fpga_base_addr + reg_offset); > > +} > > + > > +static void altera_fpga_reg_raw_writel(struct altera_fpga_priv *priv, > > + u32 reg_offset, u32 value) > > +{ > > + __raw_writel(value, priv->fpga_base_addr + reg_offset); > > +} > > + > > +static void altera_fpga_data_writel(struct altera_fpga_priv *priv, u32 value) > > +{ > > + writel(value, priv->fpga_data_addr); > > +} > > + > > +static inline void altera_fpga_reg_set_bitsl(struct altera_fpga_priv *priv, > > + u32 offset, u32 bits) > > +{ > > + u32 val; > > + > > + val = altera_fpga_reg_readl(priv, offset); > > + val |= bits; > > + altera_fpga_reg_writel(priv, offset, val); > > +} > > + > > +static inline void altera_fpga_reg_clr_bitsl(struct altera_fpga_priv *priv, > > + u32 offset, u32 bits) > > +{ > > + u32 val; > > + > > + val = altera_fpga_reg_readl(priv, offset); > > + val &= ~bits; > > + altera_fpga_reg_writel(priv, offset, val); > > +} > > + > > +static int altera_fpga_mon_status_get(struct altera_fpga_priv *priv) > > +{ > > + return altera_fpga_reg_readl(priv, > > + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST) & > > + ALTERA_FPGAMGR_MON_STATUS_MASK; > > +} > > + > > +static int altera_fpga_state_get(struct altera_fpga_priv *priv) > > +{ > > + int status = altera_fpga_mon_status_get(priv); > > + > > + if ((status & ALTERA_FPGAMGR_MON_FPGA_POWER_ON) == 0) > > + return ALTERA_FPGAMGR_STAT_POWER_OFF; > > + > > + return altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST) & > > + ALTERA_FPGAMGR_STAT_STATE_MASK; > > +} > > + > > +static void altera_fpga_clear_done_status(struct altera_fpga_priv *priv) > > +{ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST, > > + ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE); > > +} > > + > > +/* > > + * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear > > + * the complete status. > > + */ > > kernel-doc format please. > Not really necessary. But I have done it for the core fpga-mgr.c. > > +static int altera_fpga_dclk_set_and_wait_clear(struct altera_fpga_priv *priv, > > + u32 count) > > +{ > > + int timeout = 2; > > + u32 done; > > + > > + /* Clear any existing DONE status. */ > > + if (altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST)) > > + altera_fpga_clear_done_status(priv); > > + > > + /* Issue the DCLK count. */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKCNT_OFST, count); > > + > > + /* Poll DCLKSTAT to see if it completed in the timeout period. */ > > + do { > > + done = altera_fpga_reg_readl(priv, > > + ALTERA_FPGAMGR_DCLKSTAT_OFST); > > + if (done == ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE) { > > + altera_fpga_clear_done_status(priv); > > + return 0; > > + } > > + if (count <= 4) > > + udelay(1); > > + else > > + msleep(20); > > This looks weird to me. I simplified it, due to other code going away. > > > + } while (timeout--); > > + > > + return -ETIMEDOUT; > > +} > > + > > +static int altera_fpga_wait_for_state(struct altera_fpga_priv *priv, u32 state) > > +{ > > + int timeout = 2; > > + > > + /* > > + * HW doesn't support an interrupt for changes in state, so poll to see > > + * if it matches the requested state within the timeout period. > > + */ > > + do { > > + if ((altera_fpga_state_get(priv) & state) != 0) > > + return 0; > > + msleep(20); > > + } while (timeout--); > > + > > + return -ETIMEDOUT; > > +} > > + > > +static void altera_fpga_enable_irqs(struct altera_fpga_priv *priv, u32 irqs) > > +{ > > + /* set irqs to level sensitive */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST, 0); > > + > > + /* set interrupt polarity */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INT_POL_OFST, irqs); > > + > > + /* clear irqs */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, irqs); > > + > > + /* unmask interrupts */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTMSK_OFST, 0); > > + > > + /* enable interrupts */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, irqs); > > +} > > + > > +static void altera_fpga_disable_irqs(struct altera_fpga_priv *priv) > > +{ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0); > > +} > > + > > +static irqreturn_t altera_fpga_isr(int irq, void *dev_id) > > +{ > > + struct altera_fpga_priv *priv = dev_id; > > + u32 irqs, st; > > + bool conf_done, nstatus; > > + > > + /* clear irqs */ > > + irqs = altera_fpga_reg_raw_readl(priv, > > + ALTERA_FPGAMGR_GPIO_INTSTAT_OFST); > > + > > + altera_fpga_reg_raw_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, > > + irqs); > > + > > + st = altera_fpga_reg_raw_readl(priv, > > + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST); > > + conf_done = (st & ALTERA_FPGAMGR_MON_CONF_DONE) != 0; > > + nstatus = (st & ALTERA_FPGAMGR_MON_NSTATUS) != 0; > > + > > + /* success */ > > + if (conf_done && nstatus) { > > + /* disable irqs */ > > + altera_fpga_reg_raw_writel(priv, > > + ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0); > > + complete(&priv->status_complete); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int altera_fpga_wait_for_config_done(struct altera_fpga_priv *priv) > > +{ > > + int timeout, ret = 0; > > + > > + altera_fpga_disable_irqs(priv); > > + init_completion(&priv->status_complete); > > + altera_fpga_enable_irqs(priv, ALTERA_FPGAMGR_MON_CONF_DONE); > > + > > + timeout = wait_for_completion_interruptible_timeout( > > + &priv->status_complete, > > + msecs_to_jiffies(10)); > > + if (timeout == 0) > > + ret = -ETIMEDOUT; > > + > > + altera_fpga_disable_irqs(priv); > > + return ret; > > +} > > + > > +static int altera_fpga_cfg_mode_get(struct altera_fpga_priv *priv) > > +{ > > + u32 msel; > > + > > + msel = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST); > > + msel &= ALTERA_FPGAMGR_STAT_MSEL_MASK; > > + msel >>= ALTERA_FPGAMGR_STAT_MSEL_SHIFT; > > + > > + /* Check that this MSEL setting is supported */ > > + if ((msel >= sizeof(cfgmgr_modes)/sizeof(struct cfgmgr_mode)) || > > + !cfgmgr_modes[msel].valid) > > + return -EINVAL; > > + > > + return msel; > > +} > > + > > +static int altera_fpga_cfg_mode_set(struct altera_fpga_priv *priv) > > +{ > > + u32 ctrl_reg, mode; > > + > > + /* get value from MSEL pins */ > > + mode = altera_fpga_cfg_mode_get(priv); > > + if (mode < 0) > > + return mode; > > + > > + /* Adjust CTRL for the CDRATIO */ > > + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST); > > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CDRATIO_MASK; > > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CFGWDTH_MASK; > > + ctrl_reg |= cfgmgr_modes[mode].ctrl; > > + > > + /* Set NCE to 0. */ > > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCE; > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg); > > + > > + return 0; > > +} > > + > > +static int altera_fpga_reset(struct altera_fpga_priv *priv) > > +{ > > + u32 ctrl_reg, status; > > + int ret; > > + > > + /* > > + * Step 1: > > + * - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode > > + * - Set CTRL.NCE to 0 > > + */ > > + ret = altera_fpga_cfg_mode_set(priv); > > + if (ret) > > + return ret; > > + > > + /* Step 2: Set CTRL.EN to 1 */ > > + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST, > > + ALTERA_FPGAMGR_CTL_EN); > > + > > + /* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */ > > + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST); > > + ctrl_reg |= ALTERA_FPGAMGR_CTL_NCFGPULL; > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg); > > + > > + /* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */ > > + status = altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_RESET); > > + > > + /* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */ > > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCFGPULL; > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg); > > + > > + /* Timeout waiting for reset */ > > + if (status) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +/* > > + * Prepare the FPGA to receive the configuration data. > > + */ > > kernel-doc > > > +static int altera_fpga_ops_configure_init(struct fpga_manager *mgr) > > +{ > > + struct altera_fpga_priv *priv = mgr->priv; > > + int ret; > > + > > + /* Steps 1 - 5: Reset the FPGA */ > > + ret = altera_fpga_reset(priv); > > + if (ret) > > + return ret; > > + > > + /* Step 6: Wait for FPGA to enter configuration phase */ > > + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_CFG)) > > + return -ETIMEDOUT; > > + > > + /* Step 7: Clear nSTATUS interrupt */ > > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, > > + ALTERA_FPGAMGR_MON_NSTATUS); > > + > > + /* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */ > > + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST, > > + ALTERA_FPGAMGR_CTL_AXICFGEN); > > + > > + return 0; > > +} > > + > > +/* > > + * Step 9: write data to the FPGA data register > > + */ > > step 9 here? > > > +static int altera_fpga_ops_configure_write(struct fpga_manager *mgr, > > + const char *buf, size_t count) > > +{ > > + struct altera_fpga_priv *priv = mgr->priv; > > + u32 *buffer_32 = (u32 *)buf; > > + size_t i = 0; > > + > > + if (count <= 0) > > + return -EINVAL; > > + > > + /* Write out the complete 32-bit chunks. */ > > + while (count >= sizeof(u32)) { > > + altera_fpga_data_writel(priv, buffer_32[i++]); > > + count -= sizeof(u32); > > + } > > + > > + /* Write out remaining non 32-bit chunks. */ > > + switch (count) { > > + case 3: > > + altera_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff); > > + break; > > + case 2: > > + altera_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff); > > + break; > > + case 1: > > + altera_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff); > > + break; > > + default: > > + /* This will never happen. */ > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static int altera_fpga_ops_configure_complete(struct fpga_manager *mgr) > > +{ > > + struct altera_fpga_priv *priv = mgr->priv; > > + u32 status; > > + > > + /* > > + * Step 10: > > + * - Observe CONF_DONE and nSTATUS (active low) > > + * - if CONF_DONE = 1 and nSTATUS = 1, configuration was successful > > + * - if CONF_DONE = 0 and nSTATUS = 0, configuration failed > > + */ > > + status = altera_fpga_wait_for_config_done(priv); > > + if (status) > > + return status; > > + > > + /* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */ > > + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST, > > + ALTERA_FPGAMGR_CTL_AXICFGEN); > > + > > + /* > > + * Step 12: > > + * - Write 4 to DCLKCNT > > + * - Wait for STATUS.DCNTDONE = 1 > > + * - Clear W1C bit in STATUS.DCNTDONE > > + */ > > + if (altera_fpga_dclk_set_and_wait_clear(priv, 4)) > > + return -ETIMEDOUT; > > + > > +#if _ALTTERA_FPGAMGR_USE_DCLK > > + /* Step 13: Wait for STATUS.MODE to report INIT or USER MODE */ > > + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_INIT | > > + ALTERA_FPGAMGR_STAT_USER_MODE)) > > + return -ETIMEDOUT; > > + > > + /* > > + * Extra steps for Configuration with DCLK for Initialization Phase > > + * Step 14 (using 4.2.1.2 steps), 15 (using 4.2.1.2 steps) > > + * - Write 0x5000 to DCLKCNT == the number of clocks needed to exit > > + * the Initialization Phase. > > + * - Poll until STATUS.DCNTDONE = 1, write 1 to clear > > + */ > > + if (altera_fpga_dclk_set_and_wait_clear(priv, 0x5000)) > > + return -ETIMEDOUT; > > +#endif > > + > > + /* Step 13: Wait for STATUS.MODE to report USER MODE */ > > + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_USER_MODE)) > > + return -ETIMEDOUT; > > + > > + /* Step 14: Set CTRL.EN to 0 */ > > + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST, > > + ALTERA_FPGAMGR_CTL_EN); > > + > > + return 0; > > +} > > + > > +static int altera_fpga_ops_reset(struct fpga_manager *mgr) > > +{ > > + return altera_fpga_reset(mgr->priv); > > +} > > looks like not useful code - just use altera_fpga_reset instead of ops. > Done. > btw: just a generic question - isn't it better to use any better > name than altera_fpga. You can have different loader in future > and this is very generic. > Good feedback. Changed from altera.c to socfpga.c > > + > > +/* Translate state register values to FPGA framework state */ > > +static const int altera_state_to_framework_state[] = { > > + [ALTERA_FPGAMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF, > > + [ALTERA_FPGAMGR_STAT_RESET] = FPGA_MGR_STATE_RESET, > > + [ALTERA_FPGAMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT, > > + [ALTERA_FPGAMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT, > > + [ALTERA_FPGAMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING, > > + [ALTERA_FPGAMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN, > > +}; > > + > > +static int altera_fpga_ops_state(struct fpga_manager *mgr) > > here should return enum - look at 5/6 comment. > Yes > > +{ > > + struct altera_fpga_priv *priv = mgr->priv; > > + u32 state; > > this is also int not unsigned. > Yes > > + int ret; > > + > > + state = altera_fpga_state_get(priv); > > + > > + if (state < ARRAY_SIZE(altera_state_to_framework_state)) > > + ret = altera_state_to_framework_state[state]; > > + else > > + ret = FPGA_MGR_STATE_UNKNOWN; > > + > > + return ret; > > +} > > + > > +#if IS_ENABLED(CONFIG_REGULATOR) > > Instead of this look below > > > +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv) > > +{ > > + int i, ret; > > + > > use this > > if (!IS_ENABLED(CONFIG_REGULATOR)) > return 0; > > Then you can simple compile code just once. > > The same change for all these functions. > Thanks. I only had to do it in one of the functions. If CONFIG_REGULATOR is not enabled, regulator_enable() will return 0. > > + for (i = 0; i < ALTERA_FPGAMGR_NUM_SUPPLIES; i++) { > > + ret = regulator_enable(priv->fpga_supplies[i]); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv) > > +{ > > + int i; > > + > > + for (i = ALTERA_FPGAMGR_NUM_SUPPLIES - 1; i >= 0; i--) > > + regulator_disable(priv->fpga_supplies[i]); > > +} > > + > > +static int altera_fpga_regulator_probe(struct platform_device *pdev, > > + struct altera_fpga_priv *priv) > > +{ > > + struct regulator *supply; > > + unsigned int i; > > + > > + for (i = 0; i < ALTERA_FPGAMGR_NUM_SUPPLIES; i++) { > > + supply = devm_regulator_get_optional(&pdev->dev, > > + supply_names[i]); > > + if (IS_ERR(supply)) { > > + dev_err(&pdev->dev, "could not get regulators"); > > + return -EPROBE_DEFER; > > + } > > + priv->fpga_supplies[i] = supply; > > + } > > + > > + return altera_fpga_regulators_on(priv); > > +} > > +#else > > +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv) > > +{ > > + return 0; > > +} > > + > > +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv) > > +{ > > +} > > + > > +static int altera_fpga_regulator_probe(struct platform_device *pdev, > > + struct altera_fpga_priv *priv) > > +{ > > + return 0; > > +} > > +#endif > > + > > +static int altera_fpga_suspend(struct fpga_manager *mgr) > > +{ > > + struct altera_fpga_priv *priv = mgr->priv; > > + > > + altera_fpga_regulators_power_off(priv); > > + > > + return 0; > > +} > > + > > +static int altera_fpga_resume(struct fpga_manager *mgr) > > +{ > > + struct altera_fpga_priv *priv = mgr->priv; > > + u32 state; > > + unsigned int timeout; > > + int ret; > > + > > + ret = altera_fpga_regulators_on(priv); > > + if (ret) > > + return ret; > > + > > + for (timeout = 0; timeout < ALTERA_RESUME_TIMEOUT; timeout++) { > > + state = altera_fpga_state_get(priv); > > + if (state != ALTERA_FPGAMGR_STAT_POWER_OFF) > > + break; > > + msleep(20); > > + } > > + if (state == ALTERA_FPGAMGR_STAT_POWER_OFF) > > + return -ETIMEDOUT; > > + > > + return ret; > > +} > > + > > +struct fpga_manager_ops altera_altera_fpga_ops = { > > static here. > Yes > > + .reset = altera_fpga_ops_reset, > > + .state = altera_fpga_ops_state, > > + .write_init = altera_fpga_ops_configure_init, > > + .write = altera_fpga_ops_configure_write, > > + .write_complete = altera_fpga_ops_configure_complete, > > + .suspend = altera_fpga_suspend, > > + .resume = altera_fpga_resume, > > +}; > > + > > +static int altera_fpga_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = pdev->dev.of_node; > > + struct altera_fpga_priv *priv; > > + int ret; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + ret = altera_fpga_regulator_probe(pdev, priv); > > + if (ret) > > + return ret; > > + > > + priv->fpga_base_addr = of_iomap(np, 0); > > + if (!priv->fpga_base_addr) > > + return -EINVAL; > > + > > + priv->fpga_data_addr = of_iomap(np, 1); > > + if (!priv->fpga_data_addr) { > > + ret = -EINVAL; > > + goto err_unmap_base_addr; > > + } > > + > > + priv->irq = irq_of_parse_and_map(np, 0); > > + if (priv->irq == NO_IRQ) { > > NO_IRQ is not defined for all archs that's why you will get compilation error. > > <= 0 should be fine here. > > > + ret = -EINVAL; > > + goto err_unmap_data_addr; > > + } > > Anyway for all of these you should be able to use > platform_get_resource > platform_get_irq functions > > then you have simplified error path here. > OK > > > + > > + ret = request_irq(priv->irq, altera_fpga_isr, 0, "altera-fpga-mgr", > > + priv); > > + if (ret < 0) > > + goto err_dispose_irq; > > + > > + ret = fpga_mgr_register(dev, "Altera FPGA Manager", > > + &altera_altera_fpga_ops, priv); > > + if (ret) > > + goto err_free_irq; > > + > > + return 0; > > + > > +err_free_irq: > > + free_irq(priv->irq, priv); > > +err_dispose_irq: > > + irq_dispose_mapping(priv->irq); > > +err_unmap_data_addr: > > + iounmap(priv->fpga_data_addr); > > +err_unmap_base_addr: > > + iounmap(priv->fpga_base_addr); > > + return ret; > > +} > > + > > +static int altera_fpga_remove(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct fpga_manager *mgr = to_fpga_manager(dev); > > + struct altera_fpga_priv *priv = mgr->priv; > > + > > + fpga_mgr_remove(dev); > > + free_irq(priv->irq, priv); > > + irq_dispose_mapping(priv->irq); > > + iounmap(priv->fpga_data_addr); > > + iounmap(priv->fpga_base_addr); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_OF > > +static const struct of_device_id altera_fpga_of_match[] = { > > + { .compatible = "altr,fpga-mgr", }, > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(of, altera_fpga_of_match); > > +#endif > > + > > +static struct platform_driver altera_fpga_driver = { > > + .remove = altera_fpga_remove, > > + .driver = { > > + .name = "altera_fpga_manager", > > + .owner = THIS_MODULE, > > This line should go away > Yes > > + .of_match_table = of_match_ptr(altera_fpga_of_match), > > + }, > > + .probe = altera_fpga_probe, > > I tend to have probe and remove close to each other. > OK > > +}; > > + > > +static int __init altera_fpga_init(void) > > +{ > > + return platform_driver_register(&altera_fpga_driver); > > +} > > + > > +static void __exit altera_fpga_exit(void) > > +{ > > + platform_driver_unregister(&altera_fpga_driver); > > +} > > + > > +module_init(altera_fpga_init); > > +module_exit(altera_fpga_exit); > > module_platform_driver here > Yes > > + > > +MODULE_AUTHOR("Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>"); > > +MODULE_DESCRIPTION("Altera FPGA Manager"); > > +MODULE_LICENSE("GPL v2"); > > > > Thanks, > Michal > Thanks for the review. Alan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
