Re: [PATCH v4 6/6] staging: fpga manager: add driver for altera socfpga manager

[atull <atull@opensource.altera.com>]

On Wed, 10 Dec 2014, Michal Simek wrote:

> On 12/09/2014 09:14 PM, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add driver to fpga manager framework to allow configuration
> > of FPGA in Altera SoC FPGA parts.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > 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@opensource.altera.com>");
> > +MODULE_DESCRIPTION("Altera FPGA Manager");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> Thanks,
> Michal
> 

Thanks for the review.

Alan