Re: [GIT PULL] FPGA changes for 4.10

[atull]

On Thu, 1 Dec 2016, Greg KH wrote:

> On Wed, Nov 30, 2016 at 10:32:55AM -0600, Alan Tull wrote:
> > Hi Greg,
> > 
> > Please pull these changes for FPGA.
> > 
> > Thanks!
> > Alan
> > 
> > The following changes since commit e663c5dbad2999aa824045c8e01fed459d1fc833:
> > 
> >   uio: pruss: add clk_disable() (2016-11-29 20:43:12 +0100)
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/atull/linux-fpga.git 
> > tags/fpga-for-greg-20161129
> 
> Now pulled and pushed out, thanks.

Thanks!

Alan

> 
> greg k-h
> 

Re: [GIT PULL] FPGA changes for 4.10

[atull]

On Thu, 1 Dec 2016, Greg KH wrote:

> On Wed, Nov 30, 2016 at 10:32:55AM -0600, Alan Tull wrote:
> > Hi Greg,
> > 
> > Please pull these changes for FPGA.
> > 
> > Thanks!
> > Alan
> > 
> > The following changes since commit e663c5dbad2999aa824045c8e01fed459d1fc833:
> > 
> >   uio: pruss: add clk_disable() (2016-11-29 20:43:12 +0100)
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/atull/linux-fpga.git 
> > tags/fpga-for-greg-20161129
> 
> Now pulled and pushed out, thanks.

Thanks!

Alan

> 
> greg k-h
> 

Re: [PATCH] fpga: fix sparse warnings in fpga-mgr and fpga-bridge

[atull]

On Sat, 3 Dec 2016, Moritz Fischer wrote:

> On Fri, Dec 2, 2016 at 1:23 PM, Dinh Nguyen  wrote:
> > Fix up these sparse warnings:
> >
> > drivers/fpga/fpga-mgr.c:189:21: warning: symbol '__fpga_mgr_get' was not
> > declared. Should it be static?
> > drivers/fpga/fpga-bridge.c:30:12: warning: symbol 'bridge_list_lock' was
> > not declared. Should it be static?
> >
> > Signed-off-by: Dinh Nguyen 
> Acked-by: Moritz Fischer 

Acked-by: Alan Tull 

> 
> > ---
> >  drivers/fpga/fpga-bridge.c | 2 +-
> >  drivers/fpga/fpga-mgr.c| 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> > index 33ee83e..e04a899 100644
> > --- a/drivers/fpga/fpga-bridge.c
> > +++ b/drivers/fpga/fpga-bridge.c
> > @@ -27,7 +27,7 @@ static DEFINE_IDA(fpga_bridge_ida);
> >  static struct class *fpga_bridge_class;
> >
> >  /* Lock for adding/removing bridges to linked lists*/
> > -spinlock_t bridge_list_lock;
> > +static spinlock_t bridge_list_lock;
> >
> >  static int fpga_bridge_of_node_match(struct device *dev, const void *data)
> >  {
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > index f0a69d3..32860dd 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -186,7 +186,7 @@ static struct attribute *fpga_mgr_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(fpga_mgr);
> >
> > -struct fpga_manager *__fpga_mgr_get(struct device *dev)
> > +static struct fpga_manager *__fpga_mgr_get(struct device *dev)
> >  {
> > struct fpga_manager *mgr;
> > int ret = -ENODEV;
> > --
> > 2.8.3
> >
> 
> Thanks,
> Moritz
> 

Re: [PATCH] fpga: fix sparse warnings in fpga-mgr and fpga-bridge

[atull]

On Sat, 3 Dec 2016, Moritz Fischer wrote:

> On Fri, Dec 2, 2016 at 1:23 PM, Dinh Nguyen  wrote:
> > Fix up these sparse warnings:
> >
> > drivers/fpga/fpga-mgr.c:189:21: warning: symbol '__fpga_mgr_get' was not
> > declared. Should it be static?
> > drivers/fpga/fpga-bridge.c:30:12: warning: symbol 'bridge_list_lock' was
> > not declared. Should it be static?
> >
> > Signed-off-by: Dinh Nguyen 
> Acked-by: Moritz Fischer 

Acked-by: Alan Tull 

> 
> > ---
> >  drivers/fpga/fpga-bridge.c | 2 +-
> >  drivers/fpga/fpga-mgr.c| 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> > index 33ee83e..e04a899 100644
> > --- a/drivers/fpga/fpga-bridge.c
> > +++ b/drivers/fpga/fpga-bridge.c
> > @@ -27,7 +27,7 @@ static DEFINE_IDA(fpga_bridge_ida);
> >  static struct class *fpga_bridge_class;
> >
> >  /* Lock for adding/removing bridges to linked lists*/
> > -spinlock_t bridge_list_lock;
> > +static spinlock_t bridge_list_lock;
> >
> >  static int fpga_bridge_of_node_match(struct device *dev, const void *data)
> >  {
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > index f0a69d3..32860dd 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -186,7 +186,7 @@ static struct attribute *fpga_mgr_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(fpga_mgr);
> >
> > -struct fpga_manager *__fpga_mgr_get(struct device *dev)
> > +static struct fpga_manager *__fpga_mgr_get(struct device *dev)
> >  {
> > struct fpga_manager *mgr;
> > int ret = -ENODEV;
> > --
> > 2.8.3
> >
> 
> Thanks,
> Moritz
> 

Re: [PATCH v3 3/3] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[atull]

On Wed, 30 Nov 2016, Joshua Clayton wrote:

Hi Joshua,

> Hi Alan,
> 
> On 11/30/2016 09:45 AM, atull wrote:
> > On Wed, 30 Nov 2016, Joshua Clayton wrote:
> >
> > Hi Clayton,
> >
> > I just have a few minor one line changes below.  Only one
> > is operational, I should have caught that earlier.
> >
> Thanks for the speedy review.
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_ef_match);
> >> +
> >> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> >> +{
> >> +  return mgr->state;
> >> +}
> > This function gets called once to initialize mgr->state in
> > fpga_mgr_register().  So it should at least return the state the FPGA
> > is at then.  If it is unknown, it can just return
> > FPGA_MGR_STATE_UNKNOWN.
> >
> I guess I didn't understand the purpose of this function.
> The driver has access to the status pin at this phase, so I can return
> FPGA_MGR_STATE_UNKNOWN or FPGA_MGR_STATE_RESET depending
> on the state of that pin.
> 
> >> +
> >> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> >> +   const char *buf, size_t count)
> > Minor, but please fix the indentation of 'const' to match that of
> > 'struct' above.  checkpatch.pl is probably issuing warnings
> > about that.
> I double checked. The indentation is correct here. It only has
> The appearance of being off by one due to the diff format.

Yes, I understand.

> >> +{
> >> +  struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> >> +  int i;
> >> +
> >> +  if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> >> +  dev_err(>dev, "Partial reconfiguration not supported.\n");
> >> +  return -EINVAL;
> >> +  }
> >> +
> >> +  gpiod_set_value(conf->config, 0);
> >> +  usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> >> +  if (gpiod_get_value(conf->status) == 1) {
> >> +  dev_err(>dev, "Status pin should be low.\n");
> >> +  return -EIO;
> >> +  }
> >> +
> >> +  gpiod_set_value(conf->config, 1);
> >> +  for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> >> +  usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> >> +  if (gpiod_get_value(conf->status))
> >> +  return 0;
> >> +  }
> >> +
> >> +  dev_err(>dev, "Status pin not ready.\n");
> >> +  return -EIO;
> >> +}
> >> +
> >> +static void rev_buf(void *buf, size_t len)
> >> +{
> >> +  u32 *fw32 = (u32 *)buf;
> >> +  const u32 *fw_end = (u32 *)(buf + len);
> >> +
> >> +  /* set buffer to lsb first */
> >> +  while (fw32 < fw_end) {
> >> +  *fw32 = bitrev8x4(*fw32);
> >> +  fw32++;
> >> +  }
> >> +}
> >> +
> >> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> >> +  size_t count)
> > Please fix alignment here also.
> Same as above. Indentation is OK.
> 
> 
> I'll get a v4 turned around soon.

No rush since the other two patches need acks from
their respective maintainers and this won't be able
to go in without them.  But with that one change
it looks good to me.

Alan

> Thanks,
> Joshua
> 

Re: [PATCH v3 3/3] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[atull]

On Wed, 30 Nov 2016, Joshua Clayton wrote:

Hi Joshua,

> Hi Alan,
> 
> On 11/30/2016 09:45 AM, atull wrote:
> > On Wed, 30 Nov 2016, Joshua Clayton wrote:
> >
> > Hi Clayton,
> >
> > I just have a few minor one line changes below.  Only one
> > is operational, I should have caught that earlier.
> >
> Thanks for the speedy review.
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_ef_match);
> >> +
> >> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> >> +{
> >> +  return mgr->state;
> >> +}
> > This function gets called once to initialize mgr->state in
> > fpga_mgr_register().  So it should at least return the state the FPGA
> > is at then.  If it is unknown, it can just return
> > FPGA_MGR_STATE_UNKNOWN.
> >
> I guess I didn't understand the purpose of this function.
> The driver has access to the status pin at this phase, so I can return
> FPGA_MGR_STATE_UNKNOWN or FPGA_MGR_STATE_RESET depending
> on the state of that pin.
> 
> >> +
> >> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> >> +   const char *buf, size_t count)
> > Minor, but please fix the indentation of 'const' to match that of
> > 'struct' above.  checkpatch.pl is probably issuing warnings
> > about that.
> I double checked. The indentation is correct here. It only has
> The appearance of being off by one due to the diff format.

Yes, I understand.

> >> +{
> >> +  struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> >> +  int i;
> >> +
> >> +  if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> >> +  dev_err(>dev, "Partial reconfiguration not supported.\n");
> >> +  return -EINVAL;
> >> +  }
> >> +
> >> +  gpiod_set_value(conf->config, 0);
> >> +  usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> >> +  if (gpiod_get_value(conf->status) == 1) {
> >> +  dev_err(>dev, "Status pin should be low.\n");
> >> +  return -EIO;
> >> +  }
> >> +
> >> +  gpiod_set_value(conf->config, 1);
> >> +  for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> >> +  usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> >> +  if (gpiod_get_value(conf->status))
> >> +  return 0;
> >> +  }
> >> +
> >> +  dev_err(>dev, "Status pin not ready.\n");
> >> +  return -EIO;
> >> +}
> >> +
> >> +static void rev_buf(void *buf, size_t len)
> >> +{
> >> +  u32 *fw32 = (u32 *)buf;
> >> +  const u32 *fw_end = (u32 *)(buf + len);
> >> +
> >> +  /* set buffer to lsb first */
> >> +  while (fw32 < fw_end) {
> >> +  *fw32 = bitrev8x4(*fw32);
> >> +  fw32++;
> >> +  }
> >> +}
> >> +
> >> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> >> +  size_t count)
> > Please fix alignment here also.
> Same as above. Indentation is OK.
> 
> 
> I'll get a v4 turned around soon.

No rush since the other two patches need acks from
their respective maintainers and this won't be able
to go in without them.  But with that one change
it looks good to me.

Alan

> Thanks,
> Joshua
> 

Re: [PATCH v3 0/3] Altera Cyclone Passive Serial SPI FPGA Manager

[atull]

On Wed, 30 Nov 2016, Joshua Clayton wrote:

Hi Joshua,

The DT bindings will need Rob Herring's ack.  The bitrev.h
changes will need Russell King's ack.

I've made some comments on patch 3/3 but it looks good to me
besides that.

Once we have those other acks, please submit your v4 including fixes
for my comments and whatever else comes up.  I'm hoping it will be
minor and with that done, v4 can go in.

When you send in v4, please also cc our new mailing list that Moritz
made: linux-f...@vger.kernel.org

Alan

> This series adds an FPGA manager for Altera cyclone FPGAs
> that can program them using an spi port and a couple of gpios, using
> Alteras passive serial protocol.
> 
> Changes from v2:
> 
> - Merged patch 3 and 4 as suggested in review by Moritz Fischer
> - Changed FPGA_MIN_DELAY from 250 to 50 ms is the time advertized by
>   Altera. This now works, as we don't assume it is done
> 
> Changes from v1:
> - Changed the name from cyclone-spi-fpga-mgr to cyclone-ps-spi-fpga-mgr
>   This name change was requested by Alan Tull, to be specific about which
>   programming method is being employed on the fpga.
> - Changed the name of the reset-gpio to config-gpio to closer match the
>   way the pins are described in the Altera manual
> - Moved MODULE_LICENCE, _AUTHOR, and _DESCRIPTION to the bottom
> 
> - Added a bitrev8x4() function to the bitrev headers and implemented ARM
>  const, runtime, and ARM specific faster versions (This may end up
>  needing to be a standalone patch)
> 
> - Moved the bitswapping into cyclonespi_write(), as requested.
>   This falls short of my desired generic lsb first spi support, but is a step
>   in that direction.
> 
> - Fixed whitespace problems introduced during refactoring
> 
> - Replaced magic number for initial delay with a descriptive macro
> - Poll the fpga to see when it is ready rather than a fixed 1 ms sleep
> 
> Joshua Clayton (3):
>   lib: add bitrev8x4()
>   doc: dt: add cyclone-spi binding document
>   fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
> 
>  .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt  |  23 +++
>  arch/arm/include/asm/bitrev.h  |   5 +
>  drivers/fpga/Kconfig   |   7 +
>  drivers/fpga/Makefile  |   1 +
>  drivers/fpga/cyclone-ps-spi.c  | 176 
> +
>  include/linux/bitrev.h |  26 +++
>  6 files changed, 238 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
> 
> -- 
> 2.9.3
> 
> 

Re: [PATCH v3 0/3] Altera Cyclone Passive Serial SPI FPGA Manager

[atull]

On Wed, 30 Nov 2016, Joshua Clayton wrote:

Hi Joshua,

The DT bindings will need Rob Herring's ack.  The bitrev.h
changes will need Russell King's ack.

I've made some comments on patch 3/3 but it looks good to me
besides that.

Once we have those other acks, please submit your v4 including fixes
for my comments and whatever else comes up.  I'm hoping it will be
minor and with that done, v4 can go in.

When you send in v4, please also cc our new mailing list that Moritz
made: linux-f...@vger.kernel.org

Alan

> This series adds an FPGA manager for Altera cyclone FPGAs
> that can program them using an spi port and a couple of gpios, using
> Alteras passive serial protocol.
> 
> Changes from v2:
> 
> - Merged patch 3 and 4 as suggested in review by Moritz Fischer
> - Changed FPGA_MIN_DELAY from 250 to 50 ms is the time advertized by
>   Altera. This now works, as we don't assume it is done
> 
> Changes from v1:
> - Changed the name from cyclone-spi-fpga-mgr to cyclone-ps-spi-fpga-mgr
>   This name change was requested by Alan Tull, to be specific about which
>   programming method is being employed on the fpga.
> - Changed the name of the reset-gpio to config-gpio to closer match the
>   way the pins are described in the Altera manual
> - Moved MODULE_LICENCE, _AUTHOR, and _DESCRIPTION to the bottom
> 
> - Added a bitrev8x4() function to the bitrev headers and implemented ARM
>  const, runtime, and ARM specific faster versions (This may end up
>  needing to be a standalone patch)
> 
> - Moved the bitswapping into cyclonespi_write(), as requested.
>   This falls short of my desired generic lsb first spi support, but is a step
>   in that direction.
> 
> - Fixed whitespace problems introduced during refactoring
> 
> - Replaced magic number for initial delay with a descriptive macro
> - Poll the fpga to see when it is ready rather than a fixed 1 ms sleep
> 
> Joshua Clayton (3):
>   lib: add bitrev8x4()
>   doc: dt: add cyclone-spi binding document
>   fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
> 
>  .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt  |  23 +++
>  arch/arm/include/asm/bitrev.h  |   5 +
>  drivers/fpga/Kconfig   |   7 +
>  drivers/fpga/Makefile  |   1 +
>  drivers/fpga/cyclone-ps-spi.c  | 176 
> +
>  include/linux/bitrev.h |  26 +++
>  6 files changed, 238 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
> 
> -- 
> 2.9.3
> 
> 

Re: [PATCH v3 3/3] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[atull]

On Wed, 30 Nov 2016, Joshua Clayton wrote:

Hi Clayton,

I just have a few minor one line changes below.  Only one
is operational, I should have caught that earlier.

> cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial"
> interface on Altera Cyclone FPGAS.
> 
> This is one of the simpler ways to set up an FPGA at runtime.
> The signal interface is close to unidirectional spi with lsb first.
> 
> Signed-off-by: Joshua Clayton 
> ---
>  drivers/fpga/Kconfig  |   7 ++
>  drivers/fpga/Makefile |   1 +
>  drivers/fpga/cyclone-ps-spi.c | 176 
> ++
>  3 files changed, 184 insertions(+)
>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index cd84934..2462707 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,13 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_CYCLONE_PS_SPI
> + tristate "Altera Cyclone FPGA Passive Serial over SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Altera Cyclone using the
> +   passive serial interface over SPI
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..8f93930 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI)+= cyclone-ps-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c
> new file mode 100644
> index 000..57a520d
> --- /dev/null
> +++ b/drivers/fpga/cyclone-ps-spi.c
> @@ -0,0 +1,176 @@
> +/**
> + * Copyright (c) 2015 United Western Technologies, Corporation
> + *
> + * Joshua Clayton 
> + *
> + * Manage Altera fpga firmware that is loaded over spi.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera fpgas.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define FPGA_RESET_TIME  50   /* time in usecs to trigger FPGA 
> config */
> +#define FPGA_MIN_DELAY   50   /* min usecs to wait for config 
> status */
> +#define FPGA_MAX_DELAY   1000 /* max usecs to wait for config 
> status */
> +
> +struct cyclonespi_conf {
> + struct gpio_desc *config;
> + struct gpio_desc *status;
> + struct spi_device *spi;
> +};
> +
> +static const struct of_device_id of_ef_match[] = {
> + { .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);
> +
> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> + return mgr->state;
> +}

This function gets called once to initialize mgr->state in
fpga_mgr_register().  So it should at least return the state the FPGA
is at then.  If it is unknown, it can just return
FPGA_MGR_STATE_UNKNOWN.

> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> +  const char *buf, size_t count)

Minor, but please fix the indentation of 'const' to match that of
'struct' above.  checkpatch.pl is probably issuing warnings
about that.

> +{
> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> + int i;
> +
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> + dev_err(>dev, "Partial reconfiguration not supported.\n");
> + return -EINVAL;
> + }
> +
> + gpiod_set_value(conf->config, 0);
> + usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> + if (gpiod_get_value(conf->status) == 1) {
> + dev_err(>dev, "Status pin should be low.\n");
> + return -EIO;
> + }
> +
> + gpiod_set_value(conf->config, 1);
> + for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> + usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> + if (gpiod_get_value(conf->status))
> + return 0;
> + }
> +
> + dev_err(>dev, "Status pin not ready.\n");
> + return -EIO;
> +}
> +
> +static void rev_buf(void *buf, size_t len)
> +{
> + u32 *fw32 = (u32 *)buf;
> + const u32 *fw_end = (u32 *)(buf + len);
> +
> + /* set buffer to lsb first */
> + while (fw32 < fw_end) {
> + *fw32 = bitrev8x4(*fw32);
> + fw32++;
> + }
> +}
> +
> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> + size_t count)

Please fix alignment here also.

> +{
> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> + const 

Re: [PATCH v3 3/3] fpga manager: Add cyclone-ps-spi driver for Altera FPGAs

[atull]

On Wed, 30 Nov 2016, Joshua Clayton wrote:

Hi Clayton,

I just have a few minor one line changes below.  Only one
is operational, I should have caught that earlier.

> cyclone-ps-spi loads FPGA firmware over spi, using the "passive serial"
> interface on Altera Cyclone FPGAS.
> 
> This is one of the simpler ways to set up an FPGA at runtime.
> The signal interface is close to unidirectional spi with lsb first.
> 
> Signed-off-by: Joshua Clayton 
> ---
>  drivers/fpga/Kconfig  |   7 ++
>  drivers/fpga/Makefile |   1 +
>  drivers/fpga/cyclone-ps-spi.c | 176 
> ++
>  3 files changed, 184 insertions(+)
>  create mode 100644 drivers/fpga/cyclone-ps-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index cd84934..2462707 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,13 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_CYCLONE_PS_SPI
> + tristate "Altera Cyclone FPGA Passive Serial over SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Altera Cyclone using the
> +   passive serial interface over SPI
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..8f93930 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_PS_SPI)+= cyclone-ps-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c
> new file mode 100644
> index 000..57a520d
> --- /dev/null
> +++ b/drivers/fpga/cyclone-ps-spi.c
> @@ -0,0 +1,176 @@
> +/**
> + * Copyright (c) 2015 United Western Technologies, Corporation
> + *
> + * Joshua Clayton 
> + *
> + * Manage Altera fpga firmware that is loaded over spi.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera fpgas.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define FPGA_RESET_TIME  50   /* time in usecs to trigger FPGA 
> config */
> +#define FPGA_MIN_DELAY   50   /* min usecs to wait for config 
> status */
> +#define FPGA_MAX_DELAY   1000 /* max usecs to wait for config 
> status */
> +
> +struct cyclonespi_conf {
> + struct gpio_desc *config;
> + struct gpio_desc *status;
> + struct spi_device *spi;
> +};
> +
> +static const struct of_device_id of_ef_match[] = {
> + { .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);
> +
> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> + return mgr->state;
> +}

This function gets called once to initialize mgr->state in
fpga_mgr_register().  So it should at least return the state the FPGA
is at then.  If it is unknown, it can just return
FPGA_MGR_STATE_UNKNOWN.

> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> +  const char *buf, size_t count)

Minor, but please fix the indentation of 'const' to match that of
'struct' above.  checkpatch.pl is probably issuing warnings
about that.

> +{
> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> + int i;
> +
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> + dev_err(>dev, "Partial reconfiguration not supported.\n");
> + return -EINVAL;
> + }
> +
> + gpiod_set_value(conf->config, 0);
> + usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> + if (gpiod_get_value(conf->status) == 1) {
> + dev_err(>dev, "Status pin should be low.\n");
> + return -EIO;
> + }
> +
> + gpiod_set_value(conf->config, 1);
> + for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> + usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> + if (gpiod_get_value(conf->status))
> + return 0;
> + }
> +
> + dev_err(>dev, "Status pin not ready.\n");
> + return -EIO;
> +}
> +
> +static void rev_buf(void *buf, size_t len)
> +{
> + u32 *fw32 = (u32 *)buf;
> + const u32 *fw_end = (u32 *)(buf + len);
> +
> + /* set buffer to lsb first */
> + while (fw32 < fw_end) {
> + *fw32 = bitrev8x4(*fw32);
> + fw32++;
> + }
> +}
> +
> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> + size_t count)

Please fix alignment here also.

> +{
> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> + const char *fw_data = buf;
> + const char *fw_data_end 

Re: [PATCH 2/2] MAINTAINERS: Add myself as co-maintainer to fpga mgr framework.

[atull]

On Mon, 21 Nov 2016, Moritz Fischer wrote:

> Add myself as co-maintainer to fpga mgr framework.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Greg Kroah-Hartman 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-f...@vger.kernel.org
> ---
> Hi all,
> 
> Lately we've fallen behind a bit on reviewing patches lately.

Hi Moritz,

drivers/fpga has been in the upstream kernel a year now.  Most of that
time, traffic has been very slow.  Recently we had more traffic while
I was travelling and moving to a new office, both cases leaving me
with bad network connectivity.  Things will probably return to normal.
I appreciate your passion and all your effort reviewing stuff.  I
don't see a need for two maintainers at this point.

Alan

> 
> 'Behind the scenes', Alan has worked on getting a git tree setup
> for us on kernel.org and I've worked on getting the mailing list setup.
> The mailing list is up and running (see patch [1/2]) of this series.
> 
> Greg said he'd be happy to take pull requests from now on, so I hope
> once we have figured out the exact workflow things will be much smoother
> getting your patches merged into the fpga mgr framework.
> 
> Feel free to comment / speak up if you're not cool with this change.
> 
> Thanks,
> 
> Moritz
> 
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 19527a0..1856594 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5004,7 +5004,7 @@ K:  fmc_d.*register
>  
>  FPGA MANAGER FRAMEWORK
>  M:   Alan Tull 
> -R:   Moritz Fischer 
> +M:   Moritz Fischer 
>  L:   linux-f...@vger.kernel.org
>  S:   Maintained
>  F:   drivers/fpga/
> -- 
> 2.7.4
> 
> 

Re: [PATCH 2/2] MAINTAINERS: Add myself as co-maintainer to fpga mgr framework.

[atull]

On Mon, 21 Nov 2016, Moritz Fischer wrote:

> Add myself as co-maintainer to fpga mgr framework.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Greg Kroah-Hartman 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-f...@vger.kernel.org
> ---
> Hi all,
> 
> Lately we've fallen behind a bit on reviewing patches lately.

Hi Moritz,

drivers/fpga has been in the upstream kernel a year now.  Most of that
time, traffic has been very slow.  Recently we had more traffic while
I was travelling and moving to a new office, both cases leaving me
with bad network connectivity.  Things will probably return to normal.
I appreciate your passion and all your effort reviewing stuff.  I
don't see a need for two maintainers at this point.

Alan

> 
> 'Behind the scenes', Alan has worked on getting a git tree setup
> for us on kernel.org and I've worked on getting the mailing list setup.
> The mailing list is up and running (see patch [1/2]) of this series.
> 
> Greg said he'd be happy to take pull requests from now on, so I hope
> once we have figured out the exact workflow things will be much smoother
> getting your patches merged into the fpga mgr framework.
> 
> Feel free to comment / speak up if you're not cool with this change.
> 
> Thanks,
> 
> Moritz
> 
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 19527a0..1856594 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5004,7 +5004,7 @@ K:  fmc_d.*register
>  
>  FPGA MANAGER FRAMEWORK
>  M:   Alan Tull 
> -R:   Moritz Fischer 
> +M:   Moritz Fischer 
>  L:   linux-f...@vger.kernel.org
>  S:   Maintained
>  F:   drivers/fpga/
> -- 
> 2.7.4
> 
> 

Re: [PATCH v4 1/2] of: Add vendor prefix for Lattice Semiconductor

[atull]

On Sat, 29 Oct 2016, Joel Holdsworth wrote:

> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 1992aa9..d64a835 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -146,6 +146,7 @@ kosagiSutajio Ko-Usagi PTE Ltd.
>  kyo  Kyocera Corporation
>  lacieLaCie
>  lantiq   Lantiq Semiconductor
> +lattice  Lattice Semiconductor
>  lenovo   Lenovo Group Ltd.
>  lg   LG Corporation
>  linuxLinux-specific binding
> -- 
> 2.7.4
> 
> 

Acked-by: Alan Tull 

Re: [PATCH v4 1/2] of: Add vendor prefix for Lattice Semiconductor

[atull]

On Sat, 29 Oct 2016, Joel Holdsworth wrote:

> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 1992aa9..d64a835 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -146,6 +146,7 @@ kosagiSutajio Ko-Usagi PTE Ltd.
>  kyo  Kyocera Corporation
>  lacieLaCie
>  lantiq   Lantiq Semiconductor
> +lattice  Lattice Semiconductor
>  lenovo   Lenovo Group Ltd.
>  lg   LG Corporation
>  linuxLinux-specific binding
> -- 
> 2.7.4
> 
> 

Acked-by: Alan Tull 

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[atull]

On Mon, 14 Nov 2016, Rob Herring wrote:

> On Sun, Nov 06, 2016 at 07:49:21PM -0700, Joel Holdsworth wrote:
> > This adds documentation of the device tree bindings of the Lattice iCE40
> > FPGA driver for the FPGA manager framework.
> > 
> > Signed-off-by: Joel Holdsworth 
> > ---
> >  .../bindings/fpga/lattice-ice40-fpga-mgr.txt| 21 
> > +
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> 
> Acked-by: Rob Herring 
> 

Acked-by: Alan Tull 

Re: [PATCH v8 2/3] Documentation: Add binding document for Lattice iCE40 FPGA manager

[atull]

On Mon, 14 Nov 2016, Rob Herring wrote:

> On Sun, Nov 06, 2016 at 07:49:21PM -0700, Joel Holdsworth wrote:
> > This adds documentation of the device tree bindings of the Lattice iCE40
> > FPGA driver for the FPGA manager framework.
> > 
> > Signed-off-by: Joel Holdsworth 
> > ---
> >  .../bindings/fpga/lattice-ice40-fpga-mgr.txt| 21 
> > +
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> 
> Acked-by: Rob Herring 
> 

Acked-by: Alan Tull 

Re: [patch] ARM: socfpga: checking the wrong variable

[atull]

On Tue, 15 Nov 2016, Moritz Fischer wrote:

> Hi Dan,
> On Tue, Nov 15, 2016 at 1:54 AM, Dan Carpenter  
> wrote:
> > This is a cut and paste bug.  We had intended to check "sysmgr".
> >
> > Fixes: e5f8efa5c8bf ("ARM: socfpga: fpga bridge driver support")
> > Signed-off-by: Dan Carpenter 
> Acked-by: Moritz Fischer 

Hi Dan,

Acked-by: Alan Tull 

Thanks for catching this!

Alan

> 
> >
> > diff --git a/drivers/fpga/altera-fpga2sdram.c 
> > b/drivers/fpga/altera-fpga2sdram.c
> > index 7ab358e..d4eeb74 100644
> > --- a/drivers/fpga/altera-fpga2sdram.c
> > +++ b/drivers/fpga/altera-fpga2sdram.c
> > @@ -123,7 +123,7 @@ static int alt_fpga_bridge_probe(struct platform_device 
> > *pdev)
> > }
> >
> > sysmgr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> > -   if (IS_ERR(priv->sdrctl)) {
> > +   if (IS_ERR(sysmgr)) {
> > dev_err(dev, "regmap for altr,sys-mgr lookup failed.\n");
> > return PTR_ERR(sysmgr);
> > }
> 
> Thanks,
> 
> Moritz
> 

Re: [patch] ARM: socfpga: checking the wrong variable

[atull]

On Tue, 15 Nov 2016, Moritz Fischer wrote:

> Hi Dan,
> On Tue, Nov 15, 2016 at 1:54 AM, Dan Carpenter  
> wrote:
> > This is a cut and paste bug.  We had intended to check "sysmgr".
> >
> > Fixes: e5f8efa5c8bf ("ARM: socfpga: fpga bridge driver support")
> > Signed-off-by: Dan Carpenter 
> Acked-by: Moritz Fischer 

Hi Dan,

Acked-by: Alan Tull 

Thanks for catching this!

Alan

> 
> >
> > diff --git a/drivers/fpga/altera-fpga2sdram.c 
> > b/drivers/fpga/altera-fpga2sdram.c
> > index 7ab358e..d4eeb74 100644
> > --- a/drivers/fpga/altera-fpga2sdram.c
> > +++ b/drivers/fpga/altera-fpga2sdram.c
> > @@ -123,7 +123,7 @@ static int alt_fpga_bridge_probe(struct platform_device 
> > *pdev)
> > }
> >
> > sysmgr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> > -   if (IS_ERR(priv->sdrctl)) {
> > +   if (IS_ERR(sysmgr)) {
> > dev_err(dev, "regmap for altr,sys-mgr lookup failed.\n");
> > return PTR_ERR(sysmgr);
> > }
> 
> Thanks,
> 
> Moritz
> 

Re: [PATCH 3/4] fpga mgr: zynq: Add support for encrypted bitstreams

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

This looks good.  Probably the socfpga changes could get
folded into this patch (was patch 4/4) unless you thought of
a reason not to (after that patch is changed to see if the
MSEL bits are set to enable decrypt).

There also could be a uncompress cap as well since cyclone 5
supports both compressed and encrypted images and has bits
in the MSEL for them (I mentioned separately in my comments
about patch 4/4).

For the Zynq, does the encrypt bit denote a requirement that
the part will only take an encrypted image or is it an
option that it supports?  IIRC (and my brain is currently
pretty tired), if the MSEL for Cyclone5 is set for
encryption, the bitsream must be encrypted (same for
compress).  That might change the meaning of this stuff a
bit but probably doesn't necessitate a change in the
implementation.  It also makes the sysfs that much more
useful as it allows the users to know what type of image
they are required to provide.

Thanks,
Alan

> Add new flag FPGA_MGR_DECRYPT_BISTREAM as well as a matching
> capability FPGA_MGR_CAP_DECRYPT to allow for on-the-fly
> decryption of an encrypted bitstream.
> 
> If the system is not booted in secure mode AES & HMAC units
> are disabled by the boot ROM, therefore the capability
> is not available.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  drivers/fpga/fpga-mgr.c   |  7 +++
>  drivers/fpga/zynq-fpga.c  | 21 +++--
>  include/linux/fpga/fpga-mgr.h |  2 ++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 98230b7..e4d08e1 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -61,6 +61,12 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   return -ENOTSUPP;
>   }
>  
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_DECRYPT, mgr->caps)) {
> + dev_err(dev, "Bitstream decryption not supported\n");
> + return -ENOTSUPP;
> + }
> +
>   /*
>* Call the low level driver's write_init function.  This will do the
>* device-specific things to get the FPGA into the state where it is
> @@ -170,6 +176,7 @@ static const char * const state_str[] = {
>  static const char * const cap_str[] = {
>   [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
>   [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> + [FPGA_MGR_CAP_DECRYPT] = "Decrypt bitstream on the fly",
>  };
>  
>  static ssize_t name_show(struct device *dev,
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 1d37ff0..0aa4705 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -71,6 +71,10 @@
>  #define CTRL_PCAP_PR_MASKBIT(27)
>  /* Enable PCAP */
>  #define CTRL_PCAP_MODE_MASK  BIT(26)
> +/* Needed to reduce clock rate for secure config */
> +#define CTRL_PCAP_RATE_EN_MASK   BIT(25)
> +/* System booted in secure mode */
> +#define CTRL_SEC_EN_MASK BIT(7)
>  
>  /* Miscellaneous Control Register bit definitions */
>  /* Internal PCAP loopback */
> @@ -252,12 +256,20 @@ static int zynq_fpga_ops_write_init(struct fpga_manager 
> *mgr, u32 flags,
>  
>   /* set configuration register with following options:
>* - enable PCAP interface
> -  * - set throughput for maximum speed
> +  * - set throughput for maximum speed (if we're not decrypting)
>* - set CPU in user mode
>*/
>   ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> - zynq_fpga_write(priv, CTRL_OFFSET,
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM) {
> + zynq_fpga_write(priv, CTRL_OFFSET,
> + (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK |
> +  CTRL_PCAP_RATE_EN_MASK | ctrl));
> +
> + } else {
> + ctrl &= ~CTRL_PCAP_RATE_EN_MASK;
> + zynq_fpga_write(priv, CTRL_OFFSET,
>   (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl));
> + }
>  
>   /* check that we have room in the command queue */
>   status = zynq_fpga_read(priv, STATUS_OFFSET);
> @@ -412,6 +424,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>   struct resource *res;
>   fpga_mgr_cap_mask_t caps;
>   int err;
> + u32 tmp;
>  
>   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   if (!priv)
> @@ -466,6 +479,10 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>   fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
>   fpga_mgr_cap_set(FPGA_MGR_CAP_PARTIAL_RECONF, caps);
>  
> + /* only works if we booted in secure mode */
> +   

Re: [PATCH 3/4] fpga mgr: zynq: Add support for encrypted bitstreams

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

This looks good.  Probably the socfpga changes could get
folded into this patch (was patch 4/4) unless you thought of
a reason not to (after that patch is changed to see if the
MSEL bits are set to enable decrypt).

There also could be a uncompress cap as well since cyclone 5
supports both compressed and encrypted images and has bits
in the MSEL for them (I mentioned separately in my comments
about patch 4/4).

For the Zynq, does the encrypt bit denote a requirement that
the part will only take an encrypted image or is it an
option that it supports?  IIRC (and my brain is currently
pretty tired), if the MSEL for Cyclone5 is set for
encryption, the bitsream must be encrypted (same for
compress).  That might change the meaning of this stuff a
bit but probably doesn't necessitate a change in the
implementation.  It also makes the sysfs that much more
useful as it allows the users to know what type of image
they are required to provide.

Thanks,
Alan

> Add new flag FPGA_MGR_DECRYPT_BISTREAM as well as a matching
> capability FPGA_MGR_CAP_DECRYPT to allow for on-the-fly
> decryption of an encrypted bitstream.
> 
> If the system is not booted in secure mode AES & HMAC units
> are disabled by the boot ROM, therefore the capability
> is not available.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  drivers/fpga/fpga-mgr.c   |  7 +++
>  drivers/fpga/zynq-fpga.c  | 21 +++--
>  include/linux/fpga/fpga-mgr.h |  2 ++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 98230b7..e4d08e1 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -61,6 +61,12 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   return -ENOTSUPP;
>   }
>  
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_DECRYPT, mgr->caps)) {
> + dev_err(dev, "Bitstream decryption not supported\n");
> + return -ENOTSUPP;
> + }
> +
>   /*
>* Call the low level driver's write_init function.  This will do the
>* device-specific things to get the FPGA into the state where it is
> @@ -170,6 +176,7 @@ static const char * const state_str[] = {
>  static const char * const cap_str[] = {
>   [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
>   [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> + [FPGA_MGR_CAP_DECRYPT] = "Decrypt bitstream on the fly",
>  };
>  
>  static ssize_t name_show(struct device *dev,
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 1d37ff0..0aa4705 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -71,6 +71,10 @@
>  #define CTRL_PCAP_PR_MASKBIT(27)
>  /* Enable PCAP */
>  #define CTRL_PCAP_MODE_MASK  BIT(26)
> +/* Needed to reduce clock rate for secure config */
> +#define CTRL_PCAP_RATE_EN_MASK   BIT(25)
> +/* System booted in secure mode */
> +#define CTRL_SEC_EN_MASK BIT(7)
>  
>  /* Miscellaneous Control Register bit definitions */
>  /* Internal PCAP loopback */
> @@ -252,12 +256,20 @@ static int zynq_fpga_ops_write_init(struct fpga_manager 
> *mgr, u32 flags,
>  
>   /* set configuration register with following options:
>* - enable PCAP interface
> -  * - set throughput for maximum speed
> +  * - set throughput for maximum speed (if we're not decrypting)
>* - set CPU in user mode
>*/
>   ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> - zynq_fpga_write(priv, CTRL_OFFSET,
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM) {
> + zynq_fpga_write(priv, CTRL_OFFSET,
> + (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK |
> +  CTRL_PCAP_RATE_EN_MASK | ctrl));
> +
> + } else {
> + ctrl &= ~CTRL_PCAP_RATE_EN_MASK;
> + zynq_fpga_write(priv, CTRL_OFFSET,
>   (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl));
> + }
>  
>   /* check that we have room in the command queue */
>   status = zynq_fpga_read(priv, STATUS_OFFSET);
> @@ -412,6 +424,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>   struct resource *res;
>   fpga_mgr_cap_mask_t caps;
>   int err;
> + u32 tmp;
>  
>   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   if (!priv)
> @@ -466,6 +479,10 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>   fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
>   fpga_mgr_cap_set(FPGA_MGR_CAP_PARTIAL_RECONF, caps);
>  
> + /* only works if we booted in secure mode */
> + tmp = zynq_fpga_read(priv, CTRL_OFFSET);
> + if (tmp & CTRL_SEC_EN_MASK)
> + 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

On Mon, 14 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

> Hi Alan,
> 
> On Mon, Nov 14, 2016 at 6:06 AM, atull <at...@opensource.altera.com> wrote:
> > On Mon, 7 Nov 2016, Moritz Fischer wrote:
> >
> >> Add FPGA capabilities as a way to express the capabilities
> >> of a given FPGA manager.
> >>
> >> Removes code duplication by comparing the low-level driver's
> >> capabilities at the framework level rather than having each driver
> >> check for supported operations in the write_init() callback.
> >>
> >> This allows for extending with additional capabilities, similar
> >> to the the dmaengine framework's implementation.
> >>
> >> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com>
> >> Cc: Alan Tull <at...@opensource.altera.com>
> >> Cc: Michal Simek <michal.si...@xilinx.com>
> >> Cc: Sören Brinkmann <soren.brinkm...@xilinx.com>
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-arm-ker...@lists.infradead.org
> >> ---
> >>
> >> Changes from RFC:
> >> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> >>
> >> Note:
> >>
> >> If people disagree on the typedef being a 'false positive' I can fix
> >> that in a future rev of the patchset.
> >>
> >> Thanks,
> >>
> >> Moritz
> >>
> >> ---
> >>  drivers/fpga/fpga-mgr.c   | 15 ++
> >>  drivers/fpga/socfpga.c| 10 +-
> >>  drivers/fpga/zynq-fpga.c  |  7 ++-
> >>  include/linux/fpga/fpga-mgr.h | 46 
> >> ++-
> >>  4 files changed, 71 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> >> index 953dc91..ed57c17 100644
> >> --- a/drivers/fpga/fpga-mgr.c
> >> +++ b/drivers/fpga/fpga-mgr.c
> >> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 
> >> flags, const char *buf,
> >>   struct device *dev = >dev;
> >>   int ret;
> >>
> >> + if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> >> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> >> + dev_err(dev, "Partial reconfiguration not supported\n");
> >> + return -ENOTSUPP;
> >> + }
> >> +
> >> + if (flags & FPGA_MGR_FULL_RECONFIG &&
> >> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> >> + dev_err(dev, "Full reconfiguration not supported\n");
> >> + return -ENOTSUPP;
> >> + }
> >> +
> >
> > Could you move the checks to their own function like
> > 'fpga_mgr_check_caps()' or something?  I really like it if we can keep
> > the functions short, like a screen or so where it's practicle to do
> > so and I could see the number of caps growing here.
> 
> Absolutely. Great suggestion.
> 
> > The only counter argument I could think of is if a cap affects the sequence
> > in this function.  Hmmm...
> 
> Oh you mean the cap being there affecting the sequence in *this* function?
> I'd suggest we address that when we run into a cap that requires this.

Yes, I agree.

Alan

> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

On Mon, 14 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

> Hi Alan,
> 
> On Mon, Nov 14, 2016 at 6:06 AM, atull  wrote:
> > On Mon, 7 Nov 2016, Moritz Fischer wrote:
> >
> >> Add FPGA capabilities as a way to express the capabilities
> >> of a given FPGA manager.
> >>
> >> Removes code duplication by comparing the low-level driver's
> >> capabilities at the framework level rather than having each driver
> >> check for supported operations in the write_init() callback.
> >>
> >> This allows for extending with additional capabilities, similar
> >> to the the dmaengine framework's implementation.
> >>
> >> Signed-off-by: Moritz Fischer 
> >> Cc: Alan Tull 
> >> Cc: Michal Simek 
> >> Cc: Sören Brinkmann 
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-arm-ker...@lists.infradead.org
> >> ---
> >>
> >> Changes from RFC:
> >> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> >>
> >> Note:
> >>
> >> If people disagree on the typedef being a 'false positive' I can fix
> >> that in a future rev of the patchset.
> >>
> >> Thanks,
> >>
> >> Moritz
> >>
> >> ---
> >>  drivers/fpga/fpga-mgr.c   | 15 ++
> >>  drivers/fpga/socfpga.c| 10 +-
> >>  drivers/fpga/zynq-fpga.c  |  7 ++-
> >>  include/linux/fpga/fpga-mgr.h | 46 
> >> ++-
> >>  4 files changed, 71 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> >> index 953dc91..ed57c17 100644
> >> --- a/drivers/fpga/fpga-mgr.c
> >> +++ b/drivers/fpga/fpga-mgr.c
> >> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 
> >> flags, const char *buf,
> >>   struct device *dev = >dev;
> >>   int ret;
> >>
> >> + if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> >> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> >> + dev_err(dev, "Partial reconfiguration not supported\n");
> >> + return -ENOTSUPP;
> >> + }
> >> +
> >> + if (flags & FPGA_MGR_FULL_RECONFIG &&
> >> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> >> + dev_err(dev, "Full reconfiguration not supported\n");
> >> + return -ENOTSUPP;
> >> + }
> >> +
> >
> > Could you move the checks to their own function like
> > 'fpga_mgr_check_caps()' or something?  I really like it if we can keep
> > the functions short, like a screen or so where it's practicle to do
> > so and I could see the number of caps growing here.
> 
> Absolutely. Great suggestion.
> 
> > The only counter argument I could think of is if a cap affects the sequence
> > in this function.  Hmmm...
> 
> Oh you mean the cap being there affecting the sequence in *this* function?
> I'd suggest we address that when we run into a cap that requires this.

Yes, I agree.

Alan

> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH 2/4] fpga mgr: Expose FPGA capabilities to userland via sysfs

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

One nit below.  Otherwise,

Acked-by: Alan Tull 

Alan


> Expose FPGA capabilities to userland via sysfs.
> 
> Add Documentation for currently supported capabilities
> that get exported via sysfs.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  Documentation/ABI/testing/sysfs-class-fpga-manager | 16 
>  drivers/fpga/fpga-mgr.c| 20 
>  include/linux/fpga/fpga-mgr.h  |  2 ++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager 
> b/Documentation/ABI/testing/sysfs-class-fpga-manager
> index 23056c5..d9aee21 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
> @@ -35,3 +35,19 @@ Description:   Read fpga manager state as a string.
>   * write complete= Doing post programming steps
>   * write complete error  = Error while doing post programming
>   * operating = FPGA is programmed and operating
> +
> +What:/sys/class/fpga_manager/fpga/capabilities
> +Date:November 2016
> +KernelVersion:   4.9
> +Contact: Moritz Fischer 
> +Description: Read fpga manager capabilities as a string.
> + The intent is to provide userspace with information on what
> + operations the particular instance can execute.
> +
> + Each line expresses a capability that is available on the
> + particular instance of an fpga manager.
> + Supported so far:
> +
> + * Full reconfiguration
> + * Partial reconfiguration
> + * Decrypt bitstream on the fly

Decrypt gets added in a later patch.

> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index ed57c17..98230b7 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -167,6 +167,11 @@ static const char * const state_str[] = {
>   [FPGA_MGR_STATE_OPERATING] ="operating",
>  };
>  
> +static const char * const cap_str[] = {
> + [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
> + [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> +};
> +
>  static ssize_t name_show(struct device *dev,
>struct device_attribute *attr, char *buf)
>  {
> @@ -183,10 +188,25 @@ static ssize_t state_show(struct device *dev,
>   return sprintf(buf, "%s\n", state_str[mgr->state]);
>  }
>  
> +static ssize_t capabilities_show(struct device *dev,
> +  struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> + char *start = buf;
> + enum fpga_mgr_capability cap;
> +
> + for_each_fpga_mgr_cap_mask(cap, mgr->caps)
> + buf += sprintf(buf, "%s\n", cap_str[cap]);
> +
> + return buf - start;
> +}
> +
> +static DEVICE_ATTR_RO(capabilities);
>  static DEVICE_ATTR_RO(name);
>  static DEVICE_ATTR_RO(state);
>  
>  static struct attribute *fpga_mgr_attrs[] = {
> + _attr_capabilities.attr,
>   _attr_name.attr,
>   _attr_state.attr,
>   NULL,
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index e73429c..9bb96a5 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -108,6 +108,8 @@ static inline void __fpga_mgr_cap_set(enum 
> fpga_mgr_capability cap,
>   set_bit(cap, mask->bits);
>  }
>  
> +#define for_each_fpga_mgr_cap_mask(cap, mask) \
> + for_each_set_bit(cap, mask.bits, FPGA_MGR_CAP_END)
>  
>  /**
>   * struct fpga_manager_ops - ops for low level fpga manager drivers
> -- 
> 2.10.0
> 
> 

Re: [PATCH 2/4] fpga mgr: Expose FPGA capabilities to userland via sysfs

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

One nit below.  Otherwise,

Acked-by: Alan Tull 

Alan


> Expose FPGA capabilities to userland via sysfs.
> 
> Add Documentation for currently supported capabilities
> that get exported via sysfs.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  Documentation/ABI/testing/sysfs-class-fpga-manager | 16 
>  drivers/fpga/fpga-mgr.c| 20 
>  include/linux/fpga/fpga-mgr.h  |  2 ++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager 
> b/Documentation/ABI/testing/sysfs-class-fpga-manager
> index 23056c5..d9aee21 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
> @@ -35,3 +35,19 @@ Description:   Read fpga manager state as a string.
>   * write complete= Doing post programming steps
>   * write complete error  = Error while doing post programming
>   * operating = FPGA is programmed and operating
> +
> +What:/sys/class/fpga_manager/fpga/capabilities
> +Date:November 2016
> +KernelVersion:   4.9
> +Contact: Moritz Fischer 
> +Description: Read fpga manager capabilities as a string.
> + The intent is to provide userspace with information on what
> + operations the particular instance can execute.
> +
> + Each line expresses a capability that is available on the
> + particular instance of an fpga manager.
> + Supported so far:
> +
> + * Full reconfiguration
> + * Partial reconfiguration
> + * Decrypt bitstream on the fly

Decrypt gets added in a later patch.

> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index ed57c17..98230b7 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -167,6 +167,11 @@ static const char * const state_str[] = {
>   [FPGA_MGR_STATE_OPERATING] ="operating",
>  };
>  
> +static const char * const cap_str[] = {
> + [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
> + [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> +};
> +
>  static ssize_t name_show(struct device *dev,
>struct device_attribute *attr, char *buf)
>  {
> @@ -183,10 +188,25 @@ static ssize_t state_show(struct device *dev,
>   return sprintf(buf, "%s\n", state_str[mgr->state]);
>  }
>  
> +static ssize_t capabilities_show(struct device *dev,
> +  struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> + char *start = buf;
> + enum fpga_mgr_capability cap;
> +
> + for_each_fpga_mgr_cap_mask(cap, mgr->caps)
> + buf += sprintf(buf, "%s\n", cap_str[cap]);
> +
> + return buf - start;
> +}
> +
> +static DEVICE_ATTR_RO(capabilities);
>  static DEVICE_ATTR_RO(name);
>  static DEVICE_ATTR_RO(state);
>  
>  static struct attribute *fpga_mgr_attrs[] = {
> + _attr_capabilities.attr,
>   _attr_name.attr,
>   _attr_state.attr,
>   NULL,
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index e73429c..9bb96a5 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -108,6 +108,8 @@ static inline void __fpga_mgr_cap_set(enum 
> fpga_mgr_capability cap,
>   set_bit(cap, mask->bits);
>  }
>  
> +#define for_each_fpga_mgr_cap_mask(cap, mask) \
> + for_each_set_bit(cap, mask.bits, FPGA_MGR_CAP_END)
>  
>  /**
>   * struct fpga_manager_ops - ops for low level fpga manager drivers
> -- 
> 2.10.0
> 
> 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Add FPGA capabilities as a way to express the capabilities
> of a given FPGA manager.
> 
> Removes code duplication by comparing the low-level driver's
> capabilities at the framework level rather than having each driver
> check for supported operations in the write_init() callback.
> 
> This allows for extending with additional capabilities, similar
> to the the dmaengine framework's implementation.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> 
> Changes from RFC:
> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> 
> Note:
> 
> If people disagree on the typedef being a 'false positive' I can fix
> that in a future rev of the patchset.
> 
> Thanks,
> 
> Moritz
> 
> ---
>  drivers/fpga/fpga-mgr.c   | 15 ++
>  drivers/fpga/socfpga.c| 10 +-
>  drivers/fpga/zynq-fpga.c  |  7 ++-
>  include/linux/fpga/fpga-mgr.h | 46 
> ++-
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 953dc91..ed57c17 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   struct device *dev = >dev;
>   int ret;
>  
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> + dev_err(dev, "Partial reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + if (flags & FPGA_MGR_FULL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> + dev_err(dev, "Full reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +

Could you move the checks to their own function like
'fpga_mgr_check_caps()' or something?  I really like it if we can keep
the functions short, like a screen or so where it's practicle to do
so and I could see the number of caps growing here.  The only
counter argument I could think of is if a cap affects the sequence
in this function.  Hmmm...

Alan

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Add FPGA capabilities as a way to express the capabilities
> of a given FPGA manager.
> 
> Removes code duplication by comparing the low-level driver's
> capabilities at the framework level rather than having each driver
> check for supported operations in the write_init() callback.
> 
> This allows for extending with additional capabilities, similar
> to the the dmaengine framework's implementation.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> 
> Changes from RFC:
> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> 
> Note:
> 
> If people disagree on the typedef being a 'false positive' I can fix
> that in a future rev of the patchset.
> 
> Thanks,
> 
> Moritz
> 
> ---
>  drivers/fpga/fpga-mgr.c   | 15 ++
>  drivers/fpga/socfpga.c| 10 +-
>  drivers/fpga/zynq-fpga.c  |  7 ++-
>  include/linux/fpga/fpga-mgr.h | 46 
> ++-
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 953dc91..ed57c17 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   struct device *dev = >dev;
>   int ret;
>  
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> + dev_err(dev, "Partial reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + if (flags & FPGA_MGR_FULL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> + dev_err(dev, "Full reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +

Could you move the checks to their own function like
'fpga_mgr_check_caps()' or something?  I really like it if we can keep
the functions short, like a screen or so where it's practicle to do
so and I could see the number of caps growing here.  The only
counter argument I could think of is if a cap affects the sequence
in this function.  Hmmm...

Alan

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Add FPGA capabilities as a way to express the capabilities
> of a given FPGA manager.
> 
> Removes code duplication by comparing the low-level driver's
> capabilities at the framework level rather than having each driver
> check for supported operations in the write_init() callback.
> 
> This allows for extending with additional capabilities, similar
> to the the dmaengine framework's implementation.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> 
> Changes from RFC:
> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> 
> Note:
> 
> If people disagree on the typedef being a 'false positive' I can fix
> that in a future rev of the patchset.
> 
> Thanks,
> 
> Moritz

Hi Moritz,

As I said at the Plumbers, I wasn't so sure about replacing
7 lines of code with 70 to reduce code duplication.  But it
looks useful to me and I guess I'm ok with it.  This will need
to be rebased onto the current linux-next master since my
device tree overlays stuff went in last week.

Alan

> 
> ---
>  drivers/fpga/fpga-mgr.c   | 15 ++
>  drivers/fpga/socfpga.c| 10 +-
>  drivers/fpga/zynq-fpga.c  |  7 ++-
>  include/linux/fpga/fpga-mgr.h | 46 
> ++-
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 953dc91..ed57c17 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   struct device *dev = >dev;
>   int ret;
>  
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> + dev_err(dev, "Partial reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + if (flags & FPGA_MGR_FULL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> + dev_err(dev, "Full reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +
>   /*
>* Call the low level driver's write_init function.  This will do the
>* device-specific things to get the FPGA into the state where it is
> @@ -245,12 +257,14 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
>   * @dev: fpga manager device from pdev
>   * @name:fpga manager name
>   * @mops:pointer to structure of fpga manager ops
> + * @caps:fpga manager capabilities
>   * @priv:fpga manager private data
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
>  int fpga_mgr_register(struct device *dev, const char *name,
> const struct fpga_manager_ops *mops,
> +   fpga_mgr_cap_mask_t caps,
> void *priv)
>  {
>   struct fpga_manager *mgr;
> @@ -282,6 +296,7 @@ int fpga_mgr_register(struct device *dev, const char 
> *name,
>   mgr->name = name;
>   mgr->mops = mops;
>   mgr->priv = priv;
> + mgr->caps = caps;
>  
>   /*
>* Initialize framework state by requesting low level driver read state
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 27d2ff2..fd9760c 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -413,10 +413,6 @@ static int socfpga_fpga_ops_configure_init(struct 
> fpga_manager *mgr, u32 flags,
>   struct socfpga_fpga_priv *priv = mgr->priv;
>   int ret;
>  
> - if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> - dev_err(>dev, "Partial reconfiguration not supported.\n");
> - return -EINVAL;
> - }
>   /* Steps 1 - 5: Reset the FPGA */
>   ret = socfpga_fpga_reset(mgr);
>   if (ret)
> @@ -555,6 +551,7 @@ static int socfpga_fpga_probe(struct platform_device 
> *pdev)
>   struct device *dev = >dev;
>   struct socfpga_fpga_priv *priv;
>   struct resource *res;
> + fpga_mgr_cap_mask_t caps;
>   int ret;
>  
>   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -580,8 +577,11 @@ static int socfpga_fpga_probe(struct platform_device 
> *pdev)
>   if (ret)
>   return ret;
>  
> + fpga_mgr_cap_zero();
> + fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> +
>   return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> -  _fpga_ops, priv);
> +  _fpga_ops, caps, priv);
>  }
>  
>  static int socfpga_fpga_remove(struct platform_device *pdev)
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index c2fb412..1d37ff0 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -410,6 +410,7 @@ 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Add FPGA capabilities as a way to express the capabilities
> of a given FPGA manager.
> 
> Removes code duplication by comparing the low-level driver's
> capabilities at the framework level rather than having each driver
> check for supported operations in the write_init() callback.
> 
> This allows for extending with additional capabilities, similar
> to the the dmaengine framework's implementation.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> 
> Changes from RFC:
> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> 
> Note:
> 
> If people disagree on the typedef being a 'false positive' I can fix
> that in a future rev of the patchset.
> 
> Thanks,
> 
> Moritz

Hi Moritz,

As I said at the Plumbers, I wasn't so sure about replacing
7 lines of code with 70 to reduce code duplication.  But it
looks useful to me and I guess I'm ok with it.  This will need
to be rebased onto the current linux-next master since my
device tree overlays stuff went in last week.

Alan

> 
> ---
>  drivers/fpga/fpga-mgr.c   | 15 ++
>  drivers/fpga/socfpga.c| 10 +-
>  drivers/fpga/zynq-fpga.c  |  7 ++-
>  include/linux/fpga/fpga-mgr.h | 46 
> ++-
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 953dc91..ed57c17 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   struct device *dev = >dev;
>   int ret;
>  
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> + dev_err(dev, "Partial reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + if (flags & FPGA_MGR_FULL_RECONFIG &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> + dev_err(dev, "Full reconfiguration not supported\n");
> + return -ENOTSUPP;
> + }
> +
>   /*
>* Call the low level driver's write_init function.  This will do the
>* device-specific things to get the FPGA into the state where it is
> @@ -245,12 +257,14 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
>   * @dev: fpga manager device from pdev
>   * @name:fpga manager name
>   * @mops:pointer to structure of fpga manager ops
> + * @caps:fpga manager capabilities
>   * @priv:fpga manager private data
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
>  int fpga_mgr_register(struct device *dev, const char *name,
> const struct fpga_manager_ops *mops,
> +   fpga_mgr_cap_mask_t caps,
> void *priv)
>  {
>   struct fpga_manager *mgr;
> @@ -282,6 +296,7 @@ int fpga_mgr_register(struct device *dev, const char 
> *name,
>   mgr->name = name;
>   mgr->mops = mops;
>   mgr->priv = priv;
> + mgr->caps = caps;
>  
>   /*
>* Initialize framework state by requesting low level driver read state
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 27d2ff2..fd9760c 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -413,10 +413,6 @@ static int socfpga_fpga_ops_configure_init(struct 
> fpga_manager *mgr, u32 flags,
>   struct socfpga_fpga_priv *priv = mgr->priv;
>   int ret;
>  
> - if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> - dev_err(>dev, "Partial reconfiguration not supported.\n");
> - return -EINVAL;
> - }
>   /* Steps 1 - 5: Reset the FPGA */
>   ret = socfpga_fpga_reset(mgr);
>   if (ret)
> @@ -555,6 +551,7 @@ static int socfpga_fpga_probe(struct platform_device 
> *pdev)
>   struct device *dev = >dev;
>   struct socfpga_fpga_priv *priv;
>   struct resource *res;
> + fpga_mgr_cap_mask_t caps;
>   int ret;
>  
>   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -580,8 +577,11 @@ static int socfpga_fpga_probe(struct platform_device 
> *pdev)
>   if (ret)
>   return ret;
>  
> + fpga_mgr_cap_zero();
> + fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> +
>   return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> -  _fpga_ops, priv);
> +  _fpga_ops, caps, priv);
>  }
>  
>  static int socfpga_fpga_remove(struct platform_device *pdev)
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index c2fb412..1d37ff0 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -410,6 +410,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>   struct device *dev = >dev;
>   struct 

Re: [PATCH 4/4] fpga mgr: socfpga: Expose support for encrypted bitstreams

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Expose support for on the fly decryption of bitstreams.
> This needs no additional work or configuration,
> so just expose the new capability.

Hi Moritz,

When we talked about this, I was thinking about the arria10
support which I'd done more recently.  c5 and a10 are
quite different here.

The c5 datasheet:
  https://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf

Look for the 'stat' register on page 4-12 onwards.  This
register exposes the setting of the msel pins (are a dipswitch
on some boards).  The msel pins determine the programming
mode and whether it is expecting an encrypted and/or
compressed bitstream.  So you could read this reg and
set the capabilities accordingly.

For arria10, encryption is enabled and if the bitstream
says it's encrypted, the driver handles it.

Alan

> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> 
> Alan,
> 
> can you please let me know if that works this way, or where to find
> information on encrypted bitstreams? I have a CycloneV SoCFPGA to test
> on ...
> 
> Cheers,
> 
> Moritz
> ---
>  drivers/fpga/socfpga.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index fd9760c..ab57ec0c 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -579,6 +579,7 @@ static int socfpga_fpga_probe(struct platform_device 
> *pdev)
>  
>   fpga_mgr_cap_zero();
>   fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> + fpga_mgr_cap_set(FPGA_MGR_CAP_DECRYPT, caps);
>  
>   return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
>_fpga_ops, caps, priv);
> -- 
> 2.10.0
> 
> 

Re: [PATCH 4/4] fpga mgr: socfpga: Expose support for encrypted bitstreams

[atull]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Expose support for on the fly decryption of bitstreams.
> This needs no additional work or configuration,
> so just expose the new capability.

Hi Moritz,

When we talked about this, I was thinking about the arria10
support which I'd done more recently.  c5 and a10 are
quite different here.

The c5 datasheet:
  https://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf

Look for the 'stat' register on page 4-12 onwards.  This
register exposes the setting of the msel pins (are a dipswitch
on some boards).  The msel pins determine the programming
mode and whether it is expecting an encrypted and/or
compressed bitstream.  So you could read this reg and
set the capabilities accordingly.

For arria10, encryption is enabled and if the bitstream
says it's encrypted, the driver handles it.

Alan

> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> 
> Alan,
> 
> can you please let me know if that works this way, or where to find
> information on encrypted bitstreams? I have a CycloneV SoCFPGA to test
> on ...
> 
> Cheers,
> 
> Moritz
> ---
>  drivers/fpga/socfpga.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index fd9760c..ab57ec0c 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -579,6 +579,7 @@ static int socfpga_fpga_probe(struct platform_device 
> *pdev)
>  
>   fpga_mgr_cap_zero();
>   fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> + fpga_mgr_cap_set(FPGA_MGR_CAP_DECRYPT, caps);
>  
>   return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
>_fpga_ops, caps, priv);
> -- 
> 2.10.0
> 
> 

Re: [[PATCH repost v21] 01/11] of/overlay: add of overlay notifications

[atull]

On Thu, 10 Nov 2016, Greg Kroah-Hartman wrote:

> On Tue, Nov 01, 2016 at 02:14:22PM -0500, Alan Tull wrote:
> > This patch add of overlay notifications.
> 
> Your crazy way of doing [ ] in the subject messed up git, please don't
> do that...
> 
> Just do:
>   [PATCH repost v21 01/11] 
> 
> No need for extra [[ ]] please.
> 
> 

Yes, didn't mean to do that.  I saw that after I sent it out.  Sorry for 
the trouble.

Alan

Re: [[PATCH repost v21] 01/11] of/overlay: add of overlay notifications

[atull]

On Thu, 10 Nov 2016, Greg Kroah-Hartman wrote:

> On Tue, Nov 01, 2016 at 02:14:22PM -0500, Alan Tull wrote:
> > This patch add of overlay notifications.
> 
> Your crazy way of doing [ ] in the subject messed up git, please don't
> do that...
> 
> Just do:
>   [PATCH repost v21 01/11] 
> 
> No need for extra [[ ]] please.
> 
> 

Yes, didn't mean to do that.  I saw that after I sent it out.  Sorry for 
the trouble.

Alan

Re: [PATCH v7 3/3] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Fri, 4 Nov 2016, Joel Holdsworth wrote:

> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
> 
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
> 
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
> 
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processor with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
> 
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
> 
> An example of such a device is the icoBoard; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.
> ---
>  drivers/fpga/Kconfig |   6 ++
>  drivers/fpga/Makefile|   1 +
>  drivers/fpga/ice40-spi.c | 198 
> +++
>  3 files changed, 205 insertions(+)
>  create mode 100644 drivers/fpga/ice40-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..85ff429 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_ICE40_SPI
> + tristate "Lattice iCE40 SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..adb5811 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
> new file mode 100644
> index 000..a977f19
> --- /dev/null
> +++ b/drivers/fpga/ice40-spi.c
> @@ -0,0 +1,198 @@
> +/*
> + * FPGA Manager Driver for Lattice iCE40.
> + *
> + *  Copyright (c) 2016 Joel Holdsworth
> + *
> + * 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 driver adds support to the FPGA manager for configuring the SRAM of
> + * Lattice iCE40 FPGAs through slave SPI.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

Hi Joel,

It needs
 #include 
to be able to build.

Alan

> +
> +#define ICE40_SPI_FPGAMGR_RESET_DELAY 1 /* us (>200ns) */
> +#define ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY 1200 /* us */
> +
> +#define ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BITS 49 /* bits */
> +
> +struct ice40_fpga_priv {
> + struct spi_device *dev;
> + struct gpio_desc *reset;
> + struct gpio_desc *cdone;
> +};
> +
> +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> +
> + return gpiod_get_value(priv->cdone) ? FPGA_MGR_STATE_OPERATING :
> + FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +  const char *buf, size_t count)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> + struct spi_device *dev = priv->dev;
> + struct spi_message message;
> + int ret;
> +
> + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + dev_err(>dev,
> + "Partial reconfiguration is not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + /* Lock the bus, assert CRESET_B and SS_B and delay >200ns */
> + spi_bus_lock(dev->master);
> +
> + gpiod_set_value(priv->reset, 1);
> +
> + spi_message_init();
> + spi_message_add_tail(&(struct 

Re: [PATCH v7 3/3] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Fri, 4 Nov 2016, Joel Holdsworth wrote:

> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
> 
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
> 
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
> 
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processor with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
> 
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
> 
> An example of such a device is the icoBoard; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.
> ---
>  drivers/fpga/Kconfig |   6 ++
>  drivers/fpga/Makefile|   1 +
>  drivers/fpga/ice40-spi.c | 198 
> +++
>  3 files changed, 205 insertions(+)
>  create mode 100644 drivers/fpga/ice40-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..85ff429 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_ICE40_SPI
> + tristate "Lattice iCE40 SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..adb5811 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
> new file mode 100644
> index 000..a977f19
> --- /dev/null
> +++ b/drivers/fpga/ice40-spi.c
> @@ -0,0 +1,198 @@
> +/*
> + * FPGA Manager Driver for Lattice iCE40.
> + *
> + *  Copyright (c) 2016 Joel Holdsworth
> + *
> + * 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 driver adds support to the FPGA manager for configuring the SRAM of
> + * Lattice iCE40 FPGAs through slave SPI.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

Hi Joel,

It needs
 #include 
to be able to build.

Alan

> +
> +#define ICE40_SPI_FPGAMGR_RESET_DELAY 1 /* us (>200ns) */
> +#define ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY 1200 /* us */
> +
> +#define ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BITS 49 /* bits */
> +
> +struct ice40_fpga_priv {
> + struct spi_device *dev;
> + struct gpio_desc *reset;
> + struct gpio_desc *cdone;
> +};
> +
> +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> +
> + return gpiod_get_value(priv->cdone) ? FPGA_MGR_STATE_OPERATING :
> + FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +  const char *buf, size_t count)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> + struct spi_device *dev = priv->dev;
> + struct spi_message message;
> + int ret;
> +
> + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + dev_err(>dev,
> + "Partial reconfiguration is not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + /* Lock the bus, assert CRESET_B and SS_B and delay >200ns */
> + spi_bus_lock(dev->master);
> +
> + gpiod_set_value(priv->reset, 1);
> +
> + spi_message_init();
> + spi_message_add_tail(&(struct 

Re: [PATCH v7 3/3] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Fri, 4 Nov 2016, Joel Holdsworth wrote:

> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
> 
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
> 
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
> 
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processor with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
> 
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
> 
> An example of such a device is the icoBoard; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.
> ---
>  drivers/fpga/Kconfig |   6 ++
>  drivers/fpga/Makefile|   1 +
>  drivers/fpga/ice40-spi.c | 198 
> +++
>  3 files changed, 205 insertions(+)
>  create mode 100644 drivers/fpga/ice40-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..85ff429 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_ICE40_SPI
> + tristate "Lattice iCE40 SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..adb5811 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
> new file mode 100644
> index 000..a977f19
> --- /dev/null
> +++ b/drivers/fpga/ice40-spi.c
> @@ -0,0 +1,198 @@
> +/*
> + * FPGA Manager Driver for Lattice iCE40.
> + *
> + *  Copyright (c) 2016 Joel Holdsworth
> + *
> + * 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 driver adds support to the FPGA manager for configuring the SRAM of
> + * Lattice iCE40 FPGAs through slave SPI.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

Hi Joel,

The build breaks without this:

#include 

Alan

> +
> +#define ICE40_SPI_FPGAMGR_RESET_DELAY 1 /* us (>200ns) */
> +#define ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY 1200 /* us */
> +
> +#define ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BITS 49 /* bits */
> +
> +struct ice40_fpga_priv {
> + struct spi_device *dev;
> + struct gpio_desc *reset;
> + struct gpio_desc *cdone;
> +};
> +
> +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> +
> + return gpiod_get_value(priv->cdone) ? FPGA_MGR_STATE_OPERATING :
> + FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +  const char *buf, size_t count)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> + struct spi_device *dev = priv->dev;
> + struct spi_message message;
> + int ret;
> +
> + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + dev_err(>dev,
> + "Partial reconfiguration is not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + /* Lock the bus, assert CRESET_B and SS_B and delay >200ns */
> + spi_bus_lock(dev->master);
> +
> + gpiod_set_value(priv->reset, 1);
> +
> + spi_message_init();
> + spi_message_add_tail(&(struct 

Re: [PATCH v7 3/3] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Fri, 4 Nov 2016, Joel Holdsworth wrote:

> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
> 
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
> 
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
> 
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processor with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
> 
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
> 
> An example of such a device is the icoBoard; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.
> ---
>  drivers/fpga/Kconfig |   6 ++
>  drivers/fpga/Makefile|   1 +
>  drivers/fpga/ice40-spi.c | 198 
> +++
>  3 files changed, 205 insertions(+)
>  create mode 100644 drivers/fpga/ice40-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..85ff429 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_ICE40_SPI
> + tristate "Lattice iCE40 SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..adb5811 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
> new file mode 100644
> index 000..a977f19
> --- /dev/null
> +++ b/drivers/fpga/ice40-spi.c
> @@ -0,0 +1,198 @@
> +/*
> + * FPGA Manager Driver for Lattice iCE40.
> + *
> + *  Copyright (c) 2016 Joel Holdsworth
> + *
> + * 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 driver adds support to the FPGA manager for configuring the SRAM of
> + * Lattice iCE40 FPGAs through slave SPI.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

Hi Joel,

The build breaks without this:

#include 

Alan

> +
> +#define ICE40_SPI_FPGAMGR_RESET_DELAY 1 /* us (>200ns) */
> +#define ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY 1200 /* us */
> +
> +#define ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BITS 49 /* bits */
> +
> +struct ice40_fpga_priv {
> + struct spi_device *dev;
> + struct gpio_desc *reset;
> + struct gpio_desc *cdone;
> +};
> +
> +static enum fpga_mgr_states ice40_fpga_ops_state(struct fpga_manager *mgr)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> +
> + return gpiod_get_value(priv->cdone) ? FPGA_MGR_STATE_OPERATING :
> + FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +  const char *buf, size_t count)
> +{
> + struct ice40_fpga_priv *priv = mgr->priv;
> + struct spi_device *dev = priv->dev;
> + struct spi_message message;
> + int ret;
> +
> + if ((flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + dev_err(>dev,
> + "Partial reconfiguration is not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + /* Lock the bus, assert CRESET_B and SS_B and delay >200ns */
> + spi_bus_lock(dev->master);
> +
> + gpiod_set_value(priv->reset, 1);
> +
> + spi_message_init();
> + spi_message_add_tail(&(struct 

Re: [RFC] fpga: Pull checks for supported operations into framework

[atull]

On Mon, 31 Oct 2016, Moritz Fischer wrote:

> Found a couple of issues, will resubmit after cleaning up. Feel free to add
> general feedback on the idea anyways in the meantime. Sorry for double post.

Hi Moritz,

This looks good and useful to me.  One comment below.

> 
> On Sun, Oct 30, 2016 at 11:12 AM, Moritz Fischer
>  wrote:
> > Most of the drivers only support a subset of {PARTIAL, FULL}
> > reconfiguration.
> > Pull duplicate checks in each driver into the framework.
> >
> > Signed-off-by: Moritz Fischer 
> > Cc: Alan Tull 
> > Cc: Michal Simek 
> > Cc: Sören Brinkmann 
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > ---
> > Hi all,
> >
> > with the new drivers (ice40, altera-ps-spi) being submitted I've noticed
> > we're duplicating this check over and over again,
> > so I figured we might as well pull it into the framework.
> >
> > I'm not sure if there are gonna be other 'flags' we need to support
> > in the short term (we talked about byte-swapping ...)
> >
> > Note: This patch goes on top of greg's char-misc-testing  that already
> > contains Alan's latest changes to support the A10 and won't apply
> > to master.
> >
> > Cheers,
> >
> > Moritz
> >
> > ---
> >  drivers/fpga/fpga-mgr.c   | 12 
> >  drivers/fpga/socfpga-a10.c|  4 +++-
> >  drivers/fpga/socfpga.c|  3 ++-
> >  drivers/fpga/zynq-fpga.c  |  3 ++-
> >  include/linux/fpga/fpga-mgr.h |  9 +++--
> >  5 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > index c58b4c4..85f17d8 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -49,6 +49,11 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, struct 
> > fpga_image_info *info,
> > struct device *dev = >dev;
> > int ret;
> >
> > +   if (!(mgr->supported_flags & info->flags)) {
> > +   dev_err(dev, "Unsupported flags passed\n");
> > +   return -ENOTSUPP;
> > +   }
> 
> That condition is obviously garbage ...
> > +
> > /*
> >  * Call the low level driver's write_init function.  This will do 
> > the
> >  * device-specific things to get the FPGA into the state where it is
> > @@ -252,6 +257,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >   */
> >  int fpga_mgr_register(struct device *dev, const char *name,
> >   const struct fpga_manager_ops *mops,
> > + u32 supported_flags,
> >   void *priv)
> >  {
> > struct fpga_manager *mgr;
> > @@ -268,6 +274,11 @@ int fpga_mgr_register(struct device *dev, const char 
> > *name,
> > return -EINVAL;
> > }
> >
> > +   if (!supported_flags) {
> > +   dev_err(dev, "Attempt to register with no supported 
> > flags\n");
> > +   return -EINVAL;
> > +   }
> > +
> > mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> > if (!mgr)
> > return -ENOMEM;
> > @@ -282,6 +293,7 @@ int fpga_mgr_register(struct device *dev, const char 
> > *name,
> >
> > mgr->name = name;
> > mgr->mops = mops;
> > +   mgr->supported_flags = supported_flags;
> > mgr->priv = priv;
> >
> > /*
> > diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
> > index ccd9fb2..e7c82ff 100644
> > --- a/drivers/fpga/socfpga-a10.c
> > +++ b/drivers/fpga/socfpga-a10.c
> > @@ -519,7 +519,9 @@ static int socfpga_a10_fpga_probe(struct 
> > platform_device *pdev)
> > }
> >
> > return fpga_mgr_register(dev, "SoCFPGA Arria10 FPGA Manager",
> > -_a10_fpga_mgr_ops, priv);
> > +_a10_fpga_mgr_ops,
> > +FPGA_MGR_PARTIAL_RECONFIG,
> > +priv);
> >  }
> >
> >  static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> > index b6672e6..3285b7d 100644
> > --- a/drivers/fpga/socfpga.c
> > +++ b/drivers/fpga/socfpga.c
> > @@ -582,7 +582,8 @@ static int socfpga_fpga_probe(struct platform_device 
> > *pdev)
> > return ret;
> >
> > return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> > -_fpga_ops, priv);
> > +_fpga_ops, FPGA_MGR_FULL_RECONFIG,
> > +priv);
> >  }
> >
> >  static int socfpga_fpga_remove(struct platform_device *pdev)
> > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> > index 249682e..1dabd25 100644
> > --- a/drivers/fpga/zynq-fpga.c
> > +++ b/drivers/fpga/zynq-fpga.c
> > @@ -413,6 +413,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
> > struct zynq_fpga_priv *priv;
> >   

Re: [RFC] fpga: Pull checks for supported operations into framework

[atull]

On Mon, 31 Oct 2016, Moritz Fischer wrote:

> Found a couple of issues, will resubmit after cleaning up. Feel free to add
> general feedback on the idea anyways in the meantime. Sorry for double post.

Hi Moritz,

This looks good and useful to me.  One comment below.

> 
> On Sun, Oct 30, 2016 at 11:12 AM, Moritz Fischer
>  wrote:
> > Most of the drivers only support a subset of {PARTIAL, FULL}
> > reconfiguration.
> > Pull duplicate checks in each driver into the framework.
> >
> > Signed-off-by: Moritz Fischer 
> > Cc: Alan Tull 
> > Cc: Michal Simek 
> > Cc: Sören Brinkmann 
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > ---
> > Hi all,
> >
> > with the new drivers (ice40, altera-ps-spi) being submitted I've noticed
> > we're duplicating this check over and over again,
> > so I figured we might as well pull it into the framework.
> >
> > I'm not sure if there are gonna be other 'flags' we need to support
> > in the short term (we talked about byte-swapping ...)
> >
> > Note: This patch goes on top of greg's char-misc-testing  that already
> > contains Alan's latest changes to support the A10 and won't apply
> > to master.
> >
> > Cheers,
> >
> > Moritz
> >
> > ---
> >  drivers/fpga/fpga-mgr.c   | 12 
> >  drivers/fpga/socfpga-a10.c|  4 +++-
> >  drivers/fpga/socfpga.c|  3 ++-
> >  drivers/fpga/zynq-fpga.c  |  3 ++-
> >  include/linux/fpga/fpga-mgr.h |  9 +++--
> >  5 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > index c58b4c4..85f17d8 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -49,6 +49,11 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, struct 
> > fpga_image_info *info,
> > struct device *dev = >dev;
> > int ret;
> >
> > +   if (!(mgr->supported_flags & info->flags)) {
> > +   dev_err(dev, "Unsupported flags passed\n");
> > +   return -ENOTSUPP;
> > +   }
> 
> That condition is obviously garbage ...
> > +
> > /*
> >  * Call the low level driver's write_init function.  This will do 
> > the
> >  * device-specific things to get the FPGA into the state where it is
> > @@ -252,6 +257,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >   */
> >  int fpga_mgr_register(struct device *dev, const char *name,
> >   const struct fpga_manager_ops *mops,
> > + u32 supported_flags,
> >   void *priv)
> >  {
> > struct fpga_manager *mgr;
> > @@ -268,6 +274,11 @@ int fpga_mgr_register(struct device *dev, const char 
> > *name,
> > return -EINVAL;
> > }
> >
> > +   if (!supported_flags) {
> > +   dev_err(dev, "Attempt to register with no supported 
> > flags\n");
> > +   return -EINVAL;
> > +   }
> > +
> > mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> > if (!mgr)
> > return -ENOMEM;
> > @@ -282,6 +293,7 @@ int fpga_mgr_register(struct device *dev, const char 
> > *name,
> >
> > mgr->name = name;
> > mgr->mops = mops;
> > +   mgr->supported_flags = supported_flags;
> > mgr->priv = priv;
> >
> > /*
> > diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
> > index ccd9fb2..e7c82ff 100644
> > --- a/drivers/fpga/socfpga-a10.c
> > +++ b/drivers/fpga/socfpga-a10.c
> > @@ -519,7 +519,9 @@ static int socfpga_a10_fpga_probe(struct 
> > platform_device *pdev)
> > }
> >
> > return fpga_mgr_register(dev, "SoCFPGA Arria10 FPGA Manager",
> > -_a10_fpga_mgr_ops, priv);
> > +_a10_fpga_mgr_ops,
> > +FPGA_MGR_PARTIAL_RECONFIG,
> > +priv);
> >  }
> >
> >  static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> > index b6672e6..3285b7d 100644
> > --- a/drivers/fpga/socfpga.c
> > +++ b/drivers/fpga/socfpga.c
> > @@ -582,7 +582,8 @@ static int socfpga_fpga_probe(struct platform_device 
> > *pdev)
> > return ret;
> >
> > return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> > -_fpga_ops, priv);
> > +_fpga_ops, FPGA_MGR_FULL_RECONFIG,
> > +priv);
> >  }
> >
> >  static int socfpga_fpga_remove(struct platform_device *pdev)
> > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> > index 249682e..1dabd25 100644
> > --- a/drivers/fpga/zynq-fpga.c
> > +++ b/drivers/fpga/zynq-fpga.c
> > @@ -413,6 +413,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
> > struct zynq_fpga_priv *priv;
> > struct resource *res;
> > int err;
> > +   u32 flags = FPGA_MGR_FULL_RECONFIG | FPGA_MGR_PARTIAL_RECONFIG;
> >
> >  

Re: [v2 2/2] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Tue, 25 Oct 2016, Joel Holdsworth wrote:

> 
> > Hi Joel,
> > 
> > Thanks for submitting your driver!
> > 
> > I didn't see any huge problems, just minor things below...
> > 
> > Alan
> > 
> 
> Hi Alan, Thanks for your feedback. I've implemented all your suggestions and
> I'll resubmit.
> 
> I had a question about the status of the fpga-manager framework. Is there
> active work going on? I was wondering if there were any patches for specifying
> a firmware file from device-tree?

Yes!  I have submitted patches for doing exactly that.  About to
submit v21.  Here's patch 1 of v20:

https://patchwork.kernel.org/patch/9379859/

> and/or if there were any patches for loading
> firmware from userspace - something like this: "cat firmware.bin >
> /dev/fpga0"?

We discussed doing a char driver interface.  That's where all this
started.  It met with lots of resistance.

> 
> Only being able to load firmware from kernel C-code is rather a limitation -
> though I suppose the framework is quite early in development.
> 
> Thanks
> Joel
> 

Re: [v2 2/2] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Tue, 25 Oct 2016, Joel Holdsworth wrote:

> 
> > Hi Joel,
> > 
> > Thanks for submitting your driver!
> > 
> > I didn't see any huge problems, just minor things below...
> > 
> > Alan
> > 
> 
> Hi Alan, Thanks for your feedback. I've implemented all your suggestions and
> I'll resubmit.
> 
> I had a question about the status of the fpga-manager framework. Is there
> active work going on? I was wondering if there were any patches for specifying
> a firmware file from device-tree?

Yes!  I have submitted patches for doing exactly that.  About to
submit v21.  Here's patch 1 of v20:

https://patchwork.kernel.org/patch/9379859/

> and/or if there were any patches for loading
> firmware from userspace - something like this: "cat firmware.bin >
> /dev/fpga0"?

We discussed doing a char driver interface.  That's where all this
started.  It met with lots of resistance.

> 
> Only being able to load firmware from kernel C-code is rather a limitation -
> though I suppose the framework is quite early in development.
> 
> Thanks
> Joel
> 

Re: [v2 2/2] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Mon, 24 Oct 2016, Joel Holdsworth wrote:

> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
> 
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
> 
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
> 
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processer with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
> 
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
> 
> An example of such a device is the icoBOARD; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.
> ---
>  .../bindings/fpga/lattice-ice40-fpga-mgr.txt   |  23 +++
>  drivers/fpga/Kconfig   |   6 +
>  drivers/fpga/Makefile  |   1 +
>  drivers/fpga/ice40spi.c| 212 
> +
>  4 files changed, 242 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
>  create mode 100644 drivers/fpga/ice40spi.c
> 

Hi Joel,

Thanks for submitting your driver!

I didn't see any huge problems, just minor things below...

Alan

> diff --git 
> a/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt 
> b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> new file mode 100644
> index 000..b253ac8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> @@ -0,0 +1,23 @@
> +Lattice iCE40 FPGA Manager
> +
> +Required properties:
> +- compatible:should contain "lattice,ice40-fpga-mgr"
> +- reg:   SPI chip select
> +- spi-max-frequency: Maximum SPI frequency (>=100, <=2500)
> +- cdone-gpios:   GPIO connected to CDONE pin
> +- creset_b-gpios:GPIO connected to CRESET_B pin. Note that CRESET_B is
> + treated as an active-low output because the signal is
> + treated as an enable signal, rather than a reset. This
> + is necessary because the FPGA will enter Master SPI
> + mode and drive SCK with a clock signal, potentially
> + jamming other devices on the bus, unless CRESET_B is
> + held high until the firmware is loaded.

Both could be singular ...-gpio since only one gpio should be specified.

> +
> +Example:
> + ice40: ice40@0 {
> + compatible = "lattice,ice40-fpga-mgr";
> + reg = <0>;
> + spi-max-frequency = <100>;
> + cdone-gpios = < 24 GPIO_ACTIVE_HIGH>;
> + creset_b-gpios = < 22 GPIO_ACTIVE_LOW>;
> + };
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..85ff429 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_ICE40_SPI
> + tristate "Lattice iCE40 SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..6c809cc 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40spi.o

Could this be ice40-spi.c?

>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/ice40spi.c b/drivers/fpga/ice40spi.c
> new file mode 100644
> index 000..ab5ee86
> --- /dev/null
> +++ b/drivers/fpga/ice40spi.c
> @@ -0,0 +1,212 @@
> +/*
> + * FPGA Manager Driver 

Re: [v2 2/2] fpga: Add support for Lattice iCE40 FPGAs

[atull]

On Mon, 24 Oct 2016, Joel Holdsworth wrote:

> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
> and very regular structure, designed for low-cost, high-volume consumer
> and system applications.
> 
> This patch adds support to the FPGA manager for configuring the SRAM of
> iCE40LM, iCE40LP, iCE40HX, iCE40 Ultra, iCE40 UltraLite and iCE40
> UltraPlus devices, through slave SPI.
> 
> The iCE40 family is notable because it is the first FPGA family to have
> complete reverse engineered bit-stream documentation for the iCE40LP and
> iCE40HX devices. Furthermore, there is now a Free Software Verilog
> synthesis tool-chain: the "IceStorm" tool-chain.
> 
> This project is the work of Clifford Wolf, who is the maintainer of
> Yosys Verilog RTL synthesis framework, and Mathias Lasser, with notable
> contributions from "Cotton Seed", the main author of "arachne-pnr"; a
> place-and-route tool for iCE40 FPGAs.
> 
> Having a Free Software synthesis tool-chain offers interesting
> opportunities for embedded devices that are able reconfigure themselves
> with open firmware that is generated on the device itself. For example
> a mobile device might have an application processer with an iCE40 FPGA
> attached, which implements slave devices, or through which the processor
> communicates with other devices through the FPGA fabric.
> 
> A kernel driver for the iCE40 is useful, because in some cases, the FPGA
> may need to be configured before other devices can be accessed.
> 
> An example of such a device is the icoBOARD; a RaspberryPI HAT which
> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
> Digilent-compatible PMOD modules. A PMOD module may contain a device
> with which the kernel communicates, via the FPGA.
> ---
>  .../bindings/fpga/lattice-ice40-fpga-mgr.txt   |  23 +++
>  drivers/fpga/Kconfig   |   6 +
>  drivers/fpga/Makefile  |   1 +
>  drivers/fpga/ice40spi.c| 212 
> +
>  4 files changed, 242 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
>  create mode 100644 drivers/fpga/ice40spi.c
> 

Hi Joel,

Thanks for submitting your driver!

I didn't see any huge problems, just minor things below...

Alan

> diff --git 
> a/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt 
> b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> new file mode 100644
> index 000..b253ac8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/lattice-ice40-fpga-mgr.txt
> @@ -0,0 +1,23 @@
> +Lattice iCE40 FPGA Manager
> +
> +Required properties:
> +- compatible:should contain "lattice,ice40-fpga-mgr"
> +- reg:   SPI chip select
> +- spi-max-frequency: Maximum SPI frequency (>=100, <=2500)
> +- cdone-gpios:   GPIO connected to CDONE pin
> +- creset_b-gpios:GPIO connected to CRESET_B pin. Note that CRESET_B is
> + treated as an active-low output because the signal is
> + treated as an enable signal, rather than a reset. This
> + is necessary because the FPGA will enter Master SPI
> + mode and drive SCK with a clock signal, potentially
> + jamming other devices on the bus, unless CRESET_B is
> + held high until the firmware is loaded.

Both could be singular ...-gpio since only one gpio should be specified.

> +
> +Example:
> + ice40: ice40@0 {
> + compatible = "lattice,ice40-fpga-mgr";
> + reg = <0>;
> + spi-max-frequency = <100>;
> + cdone-gpios = < 24 GPIO_ACTIVE_HIGH>;
> + creset_b-gpios = < 22 GPIO_ACTIVE_LOW>;
> + };
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d614102..85ff429 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  
>  if FPGA
>  
> +config FPGA_MGR_ICE40_SPI
> + tristate "Lattice iCE40 SPI"
> + depends on SPI
> + help
> +   FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..6c809cc 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40spi.o

Could this be ice40-spi.c?

>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/ice40spi.c b/drivers/fpga/ice40spi.c
> new file mode 100644
> index 000..ab5ee86
> --- /dev/null
> +++ b/drivers/fpga/ice40spi.c
> @@ -0,0 +1,212 @@
> +/*
> + * FPGA Manager Driver 

Re: [PATCH v20 10/10] fpga-manager: Add Socfpga Arria10 support

[atull]

On Tue, 18 Oct 2016, Moritz Fischer wrote:

> On Mon, Oct 17, 2016 at 11:09:41AM -0500, Alan Tull wrote:
> > Add low level driver to support reprogramming FPGAs for Altera
> > SoCFPGA Arria10.
> > 
> > Signed-off-by: Alan Tull 
> 
> Reviewed-by: Moritz Fischer 

> > +
> > +MODULE_AUTHOR("Alan Tull ");
> > +MODULE_DESCRIPTION("SoCFPGA Arria10 FPGA Manager");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.10.1
> > 
> 
> Looking good,
> 
> Moritz
> 

Hi Moritz,

Thanks!

Alan

Re: [PATCH v20 10/10] fpga-manager: Add Socfpga Arria10 support

[atull]

On Tue, 18 Oct 2016, Moritz Fischer wrote:

> On Mon, Oct 17, 2016 at 11:09:41AM -0500, Alan Tull wrote:
> > Add low level driver to support reprogramming FPGAs for Altera
> > SoCFPGA Arria10.
> > 
> > Signed-off-by: Alan Tull 
> 
> Reviewed-by: Moritz Fischer 

> > +
> > +MODULE_AUTHOR("Alan Tull ");
> > +MODULE_DESCRIPTION("SoCFPGA Arria10 FPGA Manager");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.10.1
> > 
> 
> Looking good,
> 
> Moritz
> 

Hi Moritz,

Thanks!

Alan

Re: [PATCH v20 03/10] add bindings document for altera freeze bridge

[atull]

On Tue, 18 Oct 2016, Rob Herring wrote:

> On Mon, Oct 17, 2016 at 11:09:34AM -0500, Alan Tull wrote:
> > Add bindings document for the Altera Freeze Bridge.  A Freeze
> > Bridge is used to gate traffic to/from a region of a FPGA
> > such that that region can be reprogrammed.  The Freeze Bridge
> > exist in FPGA fabric that is not currently being reconfigured.
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > ---
> > v19: Added in v19 of patchset, uses fpga image info struct
> > v20: fix one underscore to hyphen
> > ---
> >  .../bindings/fpga/altera-freeze-bridge.txt | 23 
> > ++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> 
> Acked-by: Rob Herring 
> 

Thanks!

Alan

Re: [PATCH v20 03/10] add bindings document for altera freeze bridge

[atull]

On Tue, 18 Oct 2016, Rob Herring wrote:

> On Mon, Oct 17, 2016 at 11:09:34AM -0500, Alan Tull wrote:
> > Add bindings document for the Altera Freeze Bridge.  A Freeze
> > Bridge is used to gate traffic to/from a region of a FPGA
> > such that that region can be reprogrammed.  The Freeze Bridge
> > exist in FPGA fabric that is not currently being reconfigured.
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > ---
> > v19: Added in v19 of patchset, uses fpga image info struct
> > v20: fix one underscore to hyphen
> > ---
> >  .../bindings/fpga/altera-freeze-bridge.txt | 23 
> > ++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> 
> Acked-by: Rob Herring 
> 

Thanks!

Alan

Re: [PATCH v20 02/10] doc: fpga-mgr: add fpga image info to api

[atull]

On Mon, 17 Oct 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> couple of nits inline and some comments on ordering the patches ;-)
> 
> On Mon, Oct 17, 2016 at 6:09 PM, Alan Tull  
> wrote:
> > This patch adds a minor change in the FPGA Mangager API
> 
> s/Mangager/Manager/

Yup!

> 
> > to hold information that is specific to an FPGA image
> > file.  This change is expected to bring little, if any,
> > pain.
> >
> > An FPGA image file will have particulars that affect how the
> > image is programmed to the FPGA.  One example is that
> > current 'flags' currently has one bit which shows whether the
> > FPGA image was built for full reconfiguration or partial
> > reconfiguration.  Another example is timeout values for
> > enabling or disabling the bridges in the FPGA.  As the
> > complexity of the FPGA design increases, the bridges in the
> > FPGA may take longer times to enable or disable.
> 
> According for the current ordering bridges are not yet defined if we
> merge patches in this order?
> Not terrible imho, but I thought I'd point it out. Would swapping the
> order make sense?

Probably, yes.

> 
> I also think [5/10] should be squashed together with this commit to
> make it an atomic change.

So far my bindings and code have gone in separately.
Bindings through Rob and code through Greg KH or Dinh.

> 
> Apart from my comments above feel free to add my Acked-by
> 
> Thanks for keeping this going,
> 
> Moritz
> 

Thanks for all the code reviews!

Alan

Re: [PATCH v20 02/10] doc: fpga-mgr: add fpga image info to api

[atull]

On Mon, 17 Oct 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> couple of nits inline and some comments on ordering the patches ;-)
> 
> On Mon, Oct 17, 2016 at 6:09 PM, Alan Tull  
> wrote:
> > This patch adds a minor change in the FPGA Mangager API
> 
> s/Mangager/Manager/

Yup!

> 
> > to hold information that is specific to an FPGA image
> > file.  This change is expected to bring little, if any,
> > pain.
> >
> > An FPGA image file will have particulars that affect how the
> > image is programmed to the FPGA.  One example is that
> > current 'flags' currently has one bit which shows whether the
> > FPGA image was built for full reconfiguration or partial
> > reconfiguration.  Another example is timeout values for
> > enabling or disabling the bridges in the FPGA.  As the
> > complexity of the FPGA design increases, the bridges in the
> > FPGA may take longer times to enable or disable.
> 
> According for the current ordering bridges are not yet defined if we
> merge patches in this order?
> Not terrible imho, but I thought I'd point it out. Would swapping the
> order make sense?

Probably, yes.

> 
> I also think [5/10] should be squashed together with this commit to
> make it an atomic change.

So far my bindings and code have gone in separately.
Bindings through Rob and code through Greg KH or Dinh.

> 
> Apart from my comments above feel free to add my Acked-by
> 
> Thanks for keeping this going,
> 
> Moritz
> 

Thanks for all the code reviews!

Alan

Re: [PATCH v20 06/10] fpga: add fpga bridge framework

[atull]

On Mon, 17 Oct 2016, Alan Tull wrote:

> +/**
> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> + *
> + * @np: node pointer of a FPGA bridge
> + * @info: fpga image specific information
> + *
> + * Return fpga_bridge struct if successful.
> + * Return -EBUSY if someone already has a reference to the bridge.
> + * Return -ENODEV if @np is not a FPGA Bridge.
> + */
> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> +struct fpga_image_info *info)
> +
> +{
> + struct device *dev;
> + struct fpga_bridge *bridge;
> + int ret = -ENODEV;
> +
> + of_node_get(np);

I thought I had fixed this.  This of_node_get is unmatched,
never called in fpga_bridge_put.  And it's not necessary
since class_find_device will do kobject_get on the child
device anyway.  So I should remove this of_node_get
and the of_node_put below.

> +
> + dev = class_find_device(fpga_bridge_class, NULL, np,
> + fpga_bridge_of_node_match);
> + if (!dev)
> + goto err_dev;
> +
> + bridge = to_fpga_bridge(dev);
> + if (!bridge)
> + goto err_dev;
> +
> + bridge->info = info;
> +
> + if (!mutex_trylock(>mutex)) {
> + ret = -EBUSY;
> + goto err_dev;
> + }
> +
> + if (!try_module_get(dev->parent->driver->owner))
> + goto err_ll_mod;
> +
> + dev_dbg(>dev, "get\n");
> +
> + return bridge;
> +
> +err_ll_mod:
> + mutex_unlock(>mutex);
> +err_dev:
> + put_device(dev);
> + of_node_put(np);

Remove of_node_put.

> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
> +
> +/**
> + * fpga_bridge_put - release a reference to a bridge
> + *
> + * @bridge: FPGA bridge
> + */
> +void fpga_bridge_put(struct fpga_bridge *bridge)
> +{
> + dev_dbg(>dev, "put\n");
> +
> + bridge->info = NULL;
> + module_put(bridge->dev.parent->driver->owner);
> + mutex_unlock(>mutex);
> + put_device(>dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_put);

Re: [PATCH v20 06/10] fpga: add fpga bridge framework

[atull]

On Mon, 17 Oct 2016, Alan Tull wrote:

> +/**
> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> + *
> + * @np: node pointer of a FPGA bridge
> + * @info: fpga image specific information
> + *
> + * Return fpga_bridge struct if successful.
> + * Return -EBUSY if someone already has a reference to the bridge.
> + * Return -ENODEV if @np is not a FPGA Bridge.
> + */
> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> +struct fpga_image_info *info)
> +
> +{
> + struct device *dev;
> + struct fpga_bridge *bridge;
> + int ret = -ENODEV;
> +
> + of_node_get(np);

I thought I had fixed this.  This of_node_get is unmatched,
never called in fpga_bridge_put.  And it's not necessary
since class_find_device will do kobject_get on the child
device anyway.  So I should remove this of_node_get
and the of_node_put below.

> +
> + dev = class_find_device(fpga_bridge_class, NULL, np,
> + fpga_bridge_of_node_match);
> + if (!dev)
> + goto err_dev;
> +
> + bridge = to_fpga_bridge(dev);
> + if (!bridge)
> + goto err_dev;
> +
> + bridge->info = info;
> +
> + if (!mutex_trylock(>mutex)) {
> + ret = -EBUSY;
> + goto err_dev;
> + }
> +
> + if (!try_module_get(dev->parent->driver->owner))
> + goto err_ll_mod;
> +
> + dev_dbg(>dev, "get\n");
> +
> + return bridge;
> +
> +err_ll_mod:
> + mutex_unlock(>mutex);
> +err_dev:
> + put_device(dev);
> + of_node_put(np);

Remove of_node_put.

> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
> +
> +/**
> + * fpga_bridge_put - release a reference to a bridge
> + *
> + * @bridge: FPGA bridge
> + */
> +void fpga_bridge_put(struct fpga_bridge *bridge)
> +{
> + dev_dbg(>dev, "put\n");
> +
> + bridge->info = NULL;
> + module_put(bridge->dev.parent->driver->owner);
> + mutex_unlock(>mutex);
> + put_device(>dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_put);

Re: [PATCH v19 01/12] fpga: add bindings document for fpga region

[atull]

On Sat, 8 Oct 2016, Rob Herring wrote:

> On Wed, Sep 28, 2016 at 01:21:49PM -0500, Alan Tull wrote:
> > New bindings document for FPGA Region to support programming
> > FPGA's under Device Tree control
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Moritz Fischer 
> > ---
> > v9:  initial version added to this patchset
> > v10: s/fpga/FPGA/g
> >  replace DT overlay example with slightly more complicated example
> >  move to staging/simple-fpga-bus
> > v11: No change in this patch for v11 of the patch set
> > v12: Moved out of staging.
> >  Changed to use FPGA bridges framework instead of resets
> >  for bridges.
> > v13: bridge@0xff2 -> bridge@ff20, etc
> >  Leave out directly talking about overlays
> >  Remove regs and clocks directly under simple-fpga-bus in example
> >  Use common "firmware-name" binding instead of "fpga-firmware"
> > v14: Use firmware-name in bindings description
> >  Call it FPGA Area
> >  Remove bindings that specify FPGA Manager and FPGA Bridges
> > v15: Cleanup as per Rob's comments
> >  Combine usage doc with bindings document
> >  Document as being Altera specific
> >  Additions and changes to add FPGA Bus
> > v16: Reworked to document FPGA Regions
> >  rename altera-fpga-bus-fpga-area.txt -> fpga-region.txt
> >  Remove references that made it sound exclusive to Altera
> >  Remove altr, prefix from fpga-bus and fpga-area compatible strings
> >  Added Moritz' usage example with Xilinx
> >  Cleaned up unit addresses
> > v17: Lots of rewrites to try to make things clearer
> >  Clarify that overlay can be rejected if FPGA isn't programmed
> >  Add external-fpga-config binding already used in u-boot
> >  Change partial-reconfig binding to partial-fpga-config to align
> >with existing u-boot binding format *-fpga-config
> >  Add a document from Xilinx' website
> > v18: Fix node names underscores to be hyphens
> >  Fix copy/pasted duplicate nodes in diagram
> > v19: Fix more underscores
> >  Make FPGA regions to be children of bridges
> >  General cleanup and clarification
> > ---
> >  .../devicetree/bindings/fpga/fpga-region.txt   | 494 
> > +
> >  1 file changed, 494 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> 
> Reviewed-by: Rob Herring 
> 
> Nice job.
> 
> Rob
> 

Thanks, Rob!  I'll add your reviewed-by in v20 just
to make it even.  

Besides that, squashing some patches and some
minor changes for the Arria10 FPGA manager support.

Alan

Re: [PATCH v19 01/12] fpga: add bindings document for fpga region

[atull]

On Sat, 8 Oct 2016, Rob Herring wrote:

> On Wed, Sep 28, 2016 at 01:21:49PM -0500, Alan Tull wrote:
> > New bindings document for FPGA Region to support programming
> > FPGA's under Device Tree control
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Moritz Fischer 
> > ---
> > v9:  initial version added to this patchset
> > v10: s/fpga/FPGA/g
> >  replace DT overlay example with slightly more complicated example
> >  move to staging/simple-fpga-bus
> > v11: No change in this patch for v11 of the patch set
> > v12: Moved out of staging.
> >  Changed to use FPGA bridges framework instead of resets
> >  for bridges.
> > v13: bridge@0xff2 -> bridge@ff20, etc
> >  Leave out directly talking about overlays
> >  Remove regs and clocks directly under simple-fpga-bus in example
> >  Use common "firmware-name" binding instead of "fpga-firmware"
> > v14: Use firmware-name in bindings description
> >  Call it FPGA Area
> >  Remove bindings that specify FPGA Manager and FPGA Bridges
> > v15: Cleanup as per Rob's comments
> >  Combine usage doc with bindings document
> >  Document as being Altera specific
> >  Additions and changes to add FPGA Bus
> > v16: Reworked to document FPGA Regions
> >  rename altera-fpga-bus-fpga-area.txt -> fpga-region.txt
> >  Remove references that made it sound exclusive to Altera
> >  Remove altr, prefix from fpga-bus and fpga-area compatible strings
> >  Added Moritz' usage example with Xilinx
> >  Cleaned up unit addresses
> > v17: Lots of rewrites to try to make things clearer
> >  Clarify that overlay can be rejected if FPGA isn't programmed
> >  Add external-fpga-config binding already used in u-boot
> >  Change partial-reconfig binding to partial-fpga-config to align
> >with existing u-boot binding format *-fpga-config
> >  Add a document from Xilinx' website
> > v18: Fix node names underscores to be hyphens
> >  Fix copy/pasted duplicate nodes in diagram
> > v19: Fix more underscores
> >  Make FPGA regions to be children of bridges
> >  General cleanup and clarification
> > ---
> >  .../devicetree/bindings/fpga/fpga-region.txt   | 494 
> > +
> >  1 file changed, 494 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> 
> Reviewed-by: Rob Herring 
> 
> Nice job.
> 
> Rob
> 

Thanks, Rob!  I'll add your reviewed-by in v20 just
to make it even.  

Besides that, squashing some patches and some
minor changes for the Arria10 FPGA manager support.

Alan

Re: [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas

[atull]

On Thu, 6 Oct 2016, Joshua Clayton wrote:

> cyclonespi loads fpga firmware over spi, using the "passive serial"
> interface on Altera Cyclone FPGAS.
> 
> one of the simpler ways to set up an fpga at runtime.
> The signal interface is close to unidirectional spi with lsb first.
> 
> Signed-off-by: Joshua Clayton 
> ---
>  drivers/fpga/Kconfig  |   6 ++
>  drivers/fpga/Makefile |   1 +
>  drivers/fpga/cyclonespi.c | 173 
> ++
>  3 files changed, 180 insertions(+)
>  create mode 100644 drivers/fpga/cyclonespi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index cd84934..ccad5b1 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  

Hi Joshua,

This looks really good.  Mostly minor comments.

>  if FPGA
>  
> +config FPGA_MGR_CYCLONE_SPI

Suggestions here and below to keep PS in the name of things.
Such as FPGA_MGR_CYCLONE_PS_SPI.

> + tristate "Altera Cyclone V SPI"

"Altera Cyclone V Passive Serial over SPI" ?

> + depends on SPI
> + help
> +   FPGA manager driver support for Altera Cyclone V over SPI
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..c03f40de 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_SPI)   += cyclonespi.o

cyclone-ps-spi.o and change name of c file.

>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/cyclonespi.c b/drivers/fpga/cyclonespi.c
> new file mode 100644
> index 000..1ffa67c
> --- /dev/null
> +++ b/drivers/fpga/cyclonespi.c
> @@ -0,0 +1,173 @@
> +/**
> + * Copyright (c) 2015 United Western Technologies, Corporation
> + *
> + * Joshua Clayton 
> + *
> + * Manage Altera fpga firmware that is loaded over spi.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera fpgas.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Joshua Clayton ");
> +MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");

Please move these 3 MODULE_* lines to the bottom of this file.

> +
> +struct cyclonespi_conf {
> + struct gpio_desc *reset;
> + struct gpio_desc *status;
> + struct spi_device *spi;
> +};
> +
> +static const struct of_device_id of_ef_match[] = {
> + { .compatible = "altr,cyclonespi-fpga-mgr", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);
> +
> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> + return mgr->state;

fpga-mgr.c's fpga_mgr_register() reads the initial state
from this function.  Please return one of the values of enum
fpga_mgr_states here.  If state isn't known, then use
FPGA_MGR_STATE_UNKNOWN.

> +}
> +
> +static inline u32 revbit8x4(u32 n)
> +{
> + n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> + n = ((n & 0xUL) >> 2) | ((n & 0xUL) << 2);
> + n = ((n & 0xUL) >> 1) | ((n & 0xUL) << 1);
> + return n;
> +}
> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> + u32 *fw32 = (u32 *)buf;
> + const u32 *fw_end = (u32 *)(buf + count);
> +
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> + dev_err(>dev, "Partial reconfiguration not supported.\n");
> + return -EINVAL;
> + }
> +
> + gpiod_set_value(conf->reset, 0);
> + udelay(50);

Moritz brought up whether the delay/sleep values should be
configurable.  It looks like these are set values and have
probably been increased for good measure.  These could be
macros named to contain symbol names in the datasheet's PS
timing waveform, if that is where these are coming from.

The timing diagram has the Tcf2st0 as 800nSec max and
Tcf2st1 (below) as 40uSec.  I guess either we are looking at
different specs or you just found that you got better
results if you padded the values out?

I would normally suggest a convention of macro names
that maps neatly to the value names in the datasheet
such as CYCLONE_PS_TCF2ST0_USEC.  The problem is
the units.


> + if (gpiod_get_value(conf->status) == 1) {
> + dev_err(>dev, "Status pin should be low.\n");
> + return -EIO;
> + }
> +
> + gpiod_set_value(conf->reset, 1);
> + msleep(1);

CYCLONE_PS_TCF2ST1_USEC?  I'm not sure what to suggest
because the units in 

Re: [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas

[atull]

On Thu, 6 Oct 2016, Joshua Clayton wrote:

> cyclonespi loads fpga firmware over spi, using the "passive serial"
> interface on Altera Cyclone FPGAS.
> 
> one of the simpler ways to set up an fpga at runtime.
> The signal interface is close to unidirectional spi with lsb first.
> 
> Signed-off-by: Joshua Clayton 
> ---
>  drivers/fpga/Kconfig  |   6 ++
>  drivers/fpga/Makefile |   1 +
>  drivers/fpga/cyclonespi.c | 173 
> ++
>  3 files changed, 180 insertions(+)
>  create mode 100644 drivers/fpga/cyclonespi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index cd84934..ccad5b1 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -13,6 +13,12 @@ config FPGA
>  

Hi Joshua,

This looks really good.  Mostly minor comments.

>  if FPGA
>  
> +config FPGA_MGR_CYCLONE_SPI

Suggestions here and below to keep PS in the name of things.
Such as FPGA_MGR_CYCLONE_PS_SPI.

> + tristate "Altera Cyclone V SPI"

"Altera Cyclone V Passive Serial over SPI" ?

> + depends on SPI
> + help
> +   FPGA manager driver support for Altera Cyclone V over SPI
> +
>  config FPGA_MGR_SOCFPGA
>   tristate "Altera SOCFPGA FPGA Manager"
>   depends on ARCH_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8d83fc6..c03f40de 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_CYCLONE_SPI)   += cyclonespi.o

cyclone-ps-spi.o and change name of c file.

>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
> diff --git a/drivers/fpga/cyclonespi.c b/drivers/fpga/cyclonespi.c
> new file mode 100644
> index 000..1ffa67c
> --- /dev/null
> +++ b/drivers/fpga/cyclonespi.c
> @@ -0,0 +1,173 @@
> +/**
> + * Copyright (c) 2015 United Western Technologies, Corporation
> + *
> + * Joshua Clayton 
> + *
> + * Manage Altera fpga firmware that is loaded over spi.
> + * Firmware must be in binary "rbf" format.
> + * Works on Cyclone V. Should work on cyclone series.
> + * May work on other Altera fpgas.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Joshua Clayton ");
> +MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");

Please move these 3 MODULE_* lines to the bottom of this file.

> +
> +struct cyclonespi_conf {
> + struct gpio_desc *reset;
> + struct gpio_desc *status;
> + struct spi_device *spi;
> +};
> +
> +static const struct of_device_id of_ef_match[] = {
> + { .compatible = "altr,cyclonespi-fpga-mgr", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_ef_match);
> +
> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
> +{
> + return mgr->state;

fpga-mgr.c's fpga_mgr_register() reads the initial state
from this function.  Please return one of the values of enum
fpga_mgr_states here.  If state isn't known, then use
FPGA_MGR_STATE_UNKNOWN.

> +}
> +
> +static inline u32 revbit8x4(u32 n)
> +{
> + n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> + n = ((n & 0xUL) >> 2) | ((n & 0xUL) << 2);
> + n = ((n & 0xUL) >> 1) | ((n & 0xUL) << 1);
> + return n;
> +}
> +
> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> + u32 *fw32 = (u32 *)buf;
> + const u32 *fw_end = (u32 *)(buf + count);
> +
> + if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> + dev_err(>dev, "Partial reconfiguration not supported.\n");
> + return -EINVAL;
> + }
> +
> + gpiod_set_value(conf->reset, 0);
> + udelay(50);

Moritz brought up whether the delay/sleep values should be
configurable.  It looks like these are set values and have
probably been increased for good measure.  These could be
macros named to contain symbol names in the datasheet's PS
timing waveform, if that is where these are coming from.

The timing diagram has the Tcf2st0 as 800nSec max and
Tcf2st1 (below) as 40uSec.  I guess either we are looking at
different specs or you just found that you got better
results if you padded the values out?

I would normally suggest a convention of macro names
that maps neatly to the value names in the datasheet
such as CYCLONE_PS_TCF2ST0_USEC.  The problem is
the units.


> + if (gpiod_get_value(conf->status) == 1) {
> + dev_err(>dev, "Status pin should be low.\n");
> + return -EIO;
> + }
> +
> + gpiod_set_value(conf->reset, 1);
> + msleep(1);

CYCLONE_PS_TCF2ST1_USEC?  I'm not sure what to suggest
because the units in the timing diagram I'm looking at are
nanoseconds but this ended up being a 

Re: [PATCH v19 03/12] add bindings document for altera freeze bridge

[atull]

On Sat, 8 Oct 2016, Rob Herring wrote:

> On Wed, Sep 28, 2016 at 01:21:51PM -0500, Alan Tull wrote:
> > Add bindings document for the Altera Freeze Bridge.  A Freeze
> > Bridge is used to gate traffic to/from a region of a FPGA
> > such that that region can be reprogrammed.  The Freeze Bridge
> > exist in FPGA fabric that is not currently being reconfigured.
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > ---
> > v19: Added in v19 of patchset, uses fpga image info struct
> > ---
> >  .../bindings/fpga/altera-freeze-bridge.txt | 23 
> > ++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt 
> > b/Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> > new file mode 100644
> > index 000..97ecc11
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> > @@ -0,0 +1,23 @@
> > +Altera Freeze Bridge Controller Driver
> > +
> > +The Altera Freeze Bridge Controller manages one or more freeze bridges.
> > +The controller can freeze/disable the bridges which prevents signal
> > +changes from passing through the bridge.  The controller can also
> > +unfreeze/enable the bridges which allows traffic to pass through the
> > +bridge normally.
> > +
> > +Required properties:
> > +- compatible   : Should contain "altr,freeze-bridge-controller"
> 
> Only one version of the h/w?

The h/w has a version register.  Currently the driver reads
the reg and exits if rev != 2.  Future version support can be
keyed off this register.

> 
> > +- regs : base address and size for freeze bridge module
> > +
> > +Optional properties:
> > +- bridge-enable: 0 if driver should disable bridge at startup
> > + 1 if driver should enable bridge at startup
> > + Default is to leave bridge in current state.
> > +
> > +Example:
> > +   freeze_controller@10450 {
> 
> Underscore...

Yes.

> 
> > +   compatible = "altr,freeze-bridge-controller";
> > +   regs = <0x1000 0x10>;
> > +   bridge-enable = <0>;
> > +   };
> > -- 
> > 2.9.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v19 03/12] add bindings document for altera freeze bridge

[atull]

On Sat, 8 Oct 2016, Rob Herring wrote:

> On Wed, Sep 28, 2016 at 01:21:51PM -0500, Alan Tull wrote:
> > Add bindings document for the Altera Freeze Bridge.  A Freeze
> > Bridge is used to gate traffic to/from a region of a FPGA
> > such that that region can be reprogrammed.  The Freeze Bridge
> > exist in FPGA fabric that is not currently being reconfigured.
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > ---
> > v19: Added in v19 of patchset, uses fpga image info struct
> > ---
> >  .../bindings/fpga/altera-freeze-bridge.txt | 23 
> > ++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt 
> > b/Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> > new file mode 100644
> > index 000..97ecc11
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-freeze-bridge.txt
> > @@ -0,0 +1,23 @@
> > +Altera Freeze Bridge Controller Driver
> > +
> > +The Altera Freeze Bridge Controller manages one or more freeze bridges.
> > +The controller can freeze/disable the bridges which prevents signal
> > +changes from passing through the bridge.  The controller can also
> > +unfreeze/enable the bridges which allows traffic to pass through the
> > +bridge normally.
> > +
> > +Required properties:
> > +- compatible   : Should contain "altr,freeze-bridge-controller"
> 
> Only one version of the h/w?

The h/w has a version register.  Currently the driver reads
the reg and exits if rev != 2.  Future version support can be
keyed off this register.

> 
> > +- regs : base address and size for freeze bridge module
> > +
> > +Optional properties:
> > +- bridge-enable: 0 if driver should disable bridge at startup
> > + 1 if driver should enable bridge at startup
> > + Default is to leave bridge in current state.
> > +
> > +Example:
> > +   freeze_controller@10450 {
> 
> Underscore...

Yes.

> 
> > +   compatible = "altr,freeze-bridge-controller";
> > +   regs = <0x1000 0x10>;
> > +   bridge-enable = <0>;
> > +   };
> > -- 
> > 2.9.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> On Fri, Oct 7, 2016 at 11:21 AM, atull <at...@opensource.altera.com> wrote:
> 
> > Moritz, Can you remind me what that issue was there (or point me to
> > that email, I can't find it)?  I don't think I had a problem with that
> > in your case.  In general I think if these drivers can take the
> > bitstream that comes from the manufacturer's tools and stuff it into
> > the FPGA, then we are accomplishing what we want.  So I am OK with
> > this here.  The intent of the driver is to load a standard rbf, same
> > as the other Altera FPGA drivers.
> 
> My first version of the zynq fpga manager had byte swapping in there
> and detection of which format was used. Greg even (accidentially)
> merged my initial code
> which I then cleaned up in 4d10eaff5bfc69997a769f9c83b749f0a8c542fa
> ("fpga: zynq-fpga: Change fw format to handle bin instead of bit.") to
> address the review
> comments.
> 
> I do see your point about useability, and if this is something that
> keeps coming up
> we could pull that into the framework with a flag to SWAP or as part
> of the image_information
> struct.
> 
> Thoughts?

I agree.  Some day we may want to see it useful to extend
the flow to do that.  Currently it's not needed so we can
wait until we see clearly what is needed.

Alan

> 
> Cheers,
> 
> Moritz
> 
> [1] https://mail-archive.com/linux-kernel@vger.kernel.org/msg995245.html
> 

Re: [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> On Fri, Oct 7, 2016 at 11:21 AM, atull  wrote:
> 
> > Moritz, Can you remind me what that issue was there (or point me to
> > that email, I can't find it)?  I don't think I had a problem with that
> > in your case.  In general I think if these drivers can take the
> > bitstream that comes from the manufacturer's tools and stuff it into
> > the FPGA, then we are accomplishing what we want.  So I am OK with
> > this here.  The intent of the driver is to load a standard rbf, same
> > as the other Altera FPGA drivers.
> 
> My first version of the zynq fpga manager had byte swapping in there
> and detection of which format was used. Greg even (accidentially)
> merged my initial code
> which I then cleaned up in 4d10eaff5bfc69997a769f9c83b749f0a8c542fa
> ("fpga: zynq-fpga: Change fw format to handle bin instead of bit.") to
> address the review
> comments.
> 
> I do see your point about useability, and if this is something that
> keeps coming up
> we could pull that into the framework with a flag to SWAP or as part
> of the image_information
> struct.
> 
> Thoughts?

I agree.  Some day we may want to see it useful to extend
the flow to do that.  Currently it's not needed so we can
wait until we see clearly what is needed.

Alan

> 
> Cheers,
> 
> Moritz
> 
> [1] https://mail-archive.com/linux-kernel@vger.kernel.org/msg995245.html
> 

Re: [PATCH 2/3] doc: dt: add cyclone-spi binding document

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> > +referred to as "passive serial".
> > +The passive serial link is not technically spi, and might require extra
> > +circuits in order to play nicely with other spi slaves on the same bus.
> > +
> > +See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
> > +
> > +Required properties:
> > +- compatible  : should contain "altr,cyclonespi-fpga-mgr"
> 
> Alan, do you guys have any input on the compat string?

We want to make it clear that this is for fpga configuration
using a specific programming method (passive serial).

How about altr,cyclone-ps-spi ?

> 
> I think generally the bindings should go before the actual usage in
> your patch series. Meaning you wanna document the binding
> before you use it. I think this patch should be [1/3].

I agree.

Thanks,
Alan

> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH 2/3] doc: dt: add cyclone-spi binding document

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> > +referred to as "passive serial".
> > +The passive serial link is not technically spi, and might require extra
> > +circuits in order to play nicely with other spi slaves on the same bus.
> > +
> > +See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
> > +
> > +Required properties:
> > +- compatible  : should contain "altr,cyclonespi-fpga-mgr"
> 
> Alan, do you guys have any input on the compat string?

We want to make it clear that this is for fpga configuration
using a specific programming method (passive serial).

How about altr,cyclone-ps-spi ?

> 
> I think generally the bindings should go before the actual usage in
> your patch series. Meaning you wanna document the binding
> before you use it. I think this patch should be [1/3].

I agree.

Thanks,
Alan

> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> > +static inline u32 revbit8x4(u32 n)
> > +{
> > +   n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> > +   n = ((n & 0xUL) >> 2) | ((n & 0xUL) << 2);
> > +   n = ((n & 0xUL) >> 1) | ((n & 0xUL) << 1);
> > +   return n;
> > +}
> 
> During the Zynq FPGA manager reviews we decided that manipulating the 
> bitstream
> to be consumable by the driver is userland's job.

Moritz, Can you remind me what that issue was there (or point me to
that email, I can't find it)?  I don't think I had a problem with that
in your case.  In general I think if these drivers can take the
bitstream that comes from the manufacturer's tools and stuff it into
the FPGA, then we are accomplishing what we want.  So I am OK with
this here.  The intent of the driver is to load a standard rbf, same
as the other Altera FPGA drivers.

There is a problem here though it will be easy to fix.  This call to
revbit8x4 should happen in cyclonespi_write(), not in
cyclonespi_write_init(). The reason for that is that write_init() may
just get the first chunk of the image (the header) and that write()
will be called multiple times for the remaining chunks.  The current
FPGA manager API won't show this problem since you have to give
fpga_mgr_buf_load the whole image buffer at once.  But it is easy to
imagine that some time in the future we may want to expand the FPGA
manager API to support streaming where we don't have the whole buffer.

Thanks for submitting, Joshua.  Will be looking at this over the
next several days.

Alan

Re: [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas

[atull]

On Fri, 7 Oct 2016, Moritz Fischer wrote:

> > +static inline u32 revbit8x4(u32 n)
> > +{
> > +   n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
> > +   n = ((n & 0xUL) >> 2) | ((n & 0xUL) << 2);
> > +   n = ((n & 0xUL) >> 1) | ((n & 0xUL) << 1);
> > +   return n;
> > +}
> 
> During the Zynq FPGA manager reviews we decided that manipulating the 
> bitstream
> to be consumable by the driver is userland's job.

Moritz, Can you remind me what that issue was there (or point me to
that email, I can't find it)?  I don't think I had a problem with that
in your case.  In general I think if these drivers can take the
bitstream that comes from the manufacturer's tools and stuff it into
the FPGA, then we are accomplishing what we want.  So I am OK with
this here.  The intent of the driver is to load a standard rbf, same
as the other Altera FPGA drivers.

There is a problem here though it will be easy to fix.  This call to
revbit8x4 should happen in cyclonespi_write(), not in
cyclonespi_write_init(). The reason for that is that write_init() may
just get the first chunk of the image (the header) and that write()
will be called multiple times for the remaining chunks.  The current
FPGA manager API won't show this problem since you have to give
fpga_mgr_buf_load the whole image buffer at once.  But it is easy to
imagine that some time in the future we may want to expand the FPGA
manager API to support streaming where we don't have the whole buffer.

Thanks for submitting, Joshua.  Will be looking at this over the
next several days.

Alan

Re: [PATCH v19 12/12] fpga-manager: Add Socfpga Arria10 support

[atull]

On Thu, 29 Sep 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> On Wed, Sep 28, 2016 at 11:22 AM, Alan Tull  
> wrote:
> 
> > +static void socfpga_a10_fpga_generate_dclks(struct a10_fpga_priv *priv,
> > +   u32 count)
> > +{
> > +   u32 val;
> > +   unsigned int i;
> > +
> > +   /* Clear any existing DONE status. */
> > +   regmap_write(priv->regmap, A10_FPGAMGR_DCLKSTAT_OFST,
> > +A10_FPGAMGR_DCLKSTAT_DCLKDONE);
> > +
> > +   /* Issue the DCLK regmap. */
> > +   regmap_write(priv->regmap, A10_FPGAMGR_DCLKCNT_OFST, count);
> > +
> > +   /* wait till the dclkcnt done */
> > +   for (i = 0; i < 100; i++) {
> > +   regmap_read(priv->regmap, A10_FPGAMGR_DCLKSTAT_OFST, );
> > +   if (val)
> > +   break;
> > +   udelay(1);
> > +   }
> 
> It's quite new, but regmap_read_poll_timeout() might be a good fit here?

Yes

> 
> > +static int socfpga_a10_fpga_encrypted(struct fpga_manager *mgr,
> > + u32 *buf32, size_t buf32_size)
> > +{
> > +   int encrypt;
> > +
> > +   if (buf32_size < 70)
> > +   return -EINVAL;
> > +
> > +   encrypt = ((buf32[69] >> 2) & 3) != 0;
> > +
> > +   dev_dbg(>dev, "header word %d = %08x encrypt=%d\n",
> > +   69, buf32[69], encrypt);
> Maybe a named constants for magic 69 / 70 value :)

Sure

> 
> > +static int socfpga_a10_fpga_compressed(struct fpga_manager *mgr,
> > +  u32 *buf32, size_t buf32_size)
> > +{
> > +   int compress;
> > +
> > +   if (buf32_size < 230)
> > +   return -EINVAL;
> > +
> > +   compress = !((buf32[229] >> 1) & 1);
> > +
> > +   dev_dbg(>dev, "header word %d = %08x compress=%d\n",
> > +   229, buf32[229], compress);
> > +
> > +   return compress;
> > +}
> Same here, a comment on 229/230 would work too I guess.
> 
> > +/* Start the FPGA programming by initialize the FPGA Manager */
> > +static int socfpga_a10_fpga_write_init(struct fpga_manager *mgr,
> > +  struct fpga_image_info *info,
> > +  const char *buf, size_t count)
> > +{
> > +   struct a10_fpga_priv *priv = mgr->priv;
> > +   unsigned int cfg_width;
> > +   u32 msel, stat, mask;
> > +   int ret;
> > +
> > +   if (info->flags & FPGA_MGR_PARTIAL_RECONFIG)
> > +   cfg_width = CFGWDTH_16;
> > +   else
> > +   return -EINVAL;
> 
> So we can *only* do partial reconfig? Am I missing something here?

Correct, only PR for now.

> 
> > +   /* Do some dclks, wait for pr_ready */
> > +   socfpga_a10_fpga_generate_dclks(priv, 0x7ff);
> 
> Maybe a named constant?

OK.  Thanks for the review!

Alan

> 
> Cheers,
> Moritz
> 

Re: [PATCH v19 12/12] fpga-manager: Add Socfpga Arria10 support

[atull]

On Thu, 29 Sep 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> On Wed, Sep 28, 2016 at 11:22 AM, Alan Tull  
> wrote:
> 
> > +static void socfpga_a10_fpga_generate_dclks(struct a10_fpga_priv *priv,
> > +   u32 count)
> > +{
> > +   u32 val;
> > +   unsigned int i;
> > +
> > +   /* Clear any existing DONE status. */
> > +   regmap_write(priv->regmap, A10_FPGAMGR_DCLKSTAT_OFST,
> > +A10_FPGAMGR_DCLKSTAT_DCLKDONE);
> > +
> > +   /* Issue the DCLK regmap. */
> > +   regmap_write(priv->regmap, A10_FPGAMGR_DCLKCNT_OFST, count);
> > +
> > +   /* wait till the dclkcnt done */
> > +   for (i = 0; i < 100; i++) {
> > +   regmap_read(priv->regmap, A10_FPGAMGR_DCLKSTAT_OFST, );
> > +   if (val)
> > +   break;
> > +   udelay(1);
> > +   }
> 
> It's quite new, but regmap_read_poll_timeout() might be a good fit here?

Yes

> 
> > +static int socfpga_a10_fpga_encrypted(struct fpga_manager *mgr,
> > + u32 *buf32, size_t buf32_size)
> > +{
> > +   int encrypt;
> > +
> > +   if (buf32_size < 70)
> > +   return -EINVAL;
> > +
> > +   encrypt = ((buf32[69] >> 2) & 3) != 0;
> > +
> > +   dev_dbg(>dev, "header word %d = %08x encrypt=%d\n",
> > +   69, buf32[69], encrypt);
> Maybe a named constants for magic 69 / 70 value :)

Sure

> 
> > +static int socfpga_a10_fpga_compressed(struct fpga_manager *mgr,
> > +  u32 *buf32, size_t buf32_size)
> > +{
> > +   int compress;
> > +
> > +   if (buf32_size < 230)
> > +   return -EINVAL;
> > +
> > +   compress = !((buf32[229] >> 1) & 1);
> > +
> > +   dev_dbg(>dev, "header word %d = %08x compress=%d\n",
> > +   229, buf32[229], compress);
> > +
> > +   return compress;
> > +}
> Same here, a comment on 229/230 would work too I guess.
> 
> > +/* Start the FPGA programming by initialize the FPGA Manager */
> > +static int socfpga_a10_fpga_write_init(struct fpga_manager *mgr,
> > +  struct fpga_image_info *info,
> > +  const char *buf, size_t count)
> > +{
> > +   struct a10_fpga_priv *priv = mgr->priv;
> > +   unsigned int cfg_width;
> > +   u32 msel, stat, mask;
> > +   int ret;
> > +
> > +   if (info->flags & FPGA_MGR_PARTIAL_RECONFIG)
> > +   cfg_width = CFGWDTH_16;
> > +   else
> > +   return -EINVAL;
> 
> So we can *only* do partial reconfig? Am I missing something here?

Correct, only PR for now.

> 
> > +   /* Do some dclks, wait for pr_ready */
> > +   socfpga_a10_fpga_generate_dclks(priv, 0x7ff);
> 
> Maybe a named constant?

OK.  Thanks for the review!

Alan

> 
> Cheers,
> Moritz
> 

Re: [PATCH v18 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Tue, 9 Aug 2016, Paul Gortmaker wrote:

> [Re: [PATCH v18 6/6] ARM: socfpga: fpga bridge driver support] On 08/08/2016 
> (Mon 13:44) Moritz Fischer wrote:
> 
> > Hi Alan,
> > 
> > On Mon, Aug 8, 2016 at 12:18 PM, atull <at...@opensource.altera.com> wrote:
> > 
> > >> Please don't use module.h in drivers controlled by a bool
> > >> Kconfig setting.
> > >>
> > >> THanks,
> > >> Paul.
> > >>
> > >
> > > Thanks for the feedback.  Can you provide an example of what you
> > > would consider to be proper usage in the kernel?
> > 
> > 
> > I think Paul is suggesting to use
> > 
> > static int __init alt_fpga_bridge_init(void)
> > {
> > platform_driver_register(_fpga_bridge_driver);
> > }
> > 
> > device_initcall(alt_fpga_bridge_init);
> > 
> > or better:
> > 
> > builtin_platform_driver(_fpga_bridge_driver);
> > 
> > Like for example in: drivers/cpuidle/cpuidle-mvebu-v7.c
> 
> Yes, pretty much that -- if you have a bool Kconfig, you should be using
> builtin registration functions, and have no need for module.h or
> anything MODULE_ or any module_init/module_exit calls.
> 
> An empty file containing nothing but #include  will
> cause cpp to emit about 750k of goop, so we really should only be using
> it for drivers that are genuinely modular; i.e. tristate Kconfig.
> 
> Thanks,
> Paul.
> --
> 
> > 
> > Cheers,
> > 
> > Moritz
> 

Thanks for the feedback and explanations!

I've retested my stuff with it all built as modules (mgr, bridged,
and fpga-region) and it all works that way as well as built in.
So I'll fix up the Kconfig as tristates for everybody.  Also
I'll add some dependencies as FPGA_REGION should be dependent
on FPGA_BRIDGE.

Alan

Re: [PATCH v18 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Tue, 9 Aug 2016, Paul Gortmaker wrote:

> [Re: [PATCH v18 6/6] ARM: socfpga: fpga bridge driver support] On 08/08/2016 
> (Mon 13:44) Moritz Fischer wrote:
> 
> > Hi Alan,
> > 
> > On Mon, Aug 8, 2016 at 12:18 PM, atull  wrote:
> > 
> > >> Please don't use module.h in drivers controlled by a bool
> > >> Kconfig setting.
> > >>
> > >> THanks,
> > >> Paul.
> > >>
> > >
> > > Thanks for the feedback.  Can you provide an example of what you
> > > would consider to be proper usage in the kernel?
> > 
> > 
> > I think Paul is suggesting to use
> > 
> > static int __init alt_fpga_bridge_init(void)
> > {
> > platform_driver_register(_fpga_bridge_driver);
> > }
> > 
> > device_initcall(alt_fpga_bridge_init);
> > 
> > or better:
> > 
> > builtin_platform_driver(_fpga_bridge_driver);
> > 
> > Like for example in: drivers/cpuidle/cpuidle-mvebu-v7.c
> 
> Yes, pretty much that -- if you have a bool Kconfig, you should be using
> builtin registration functions, and have no need for module.h or
> anything MODULE_ or any module_init/module_exit calls.
> 
> An empty file containing nothing but #include  will
> cause cpp to emit about 750k of goop, so we really should only be using
> it for drivers that are genuinely modular; i.e. tristate Kconfig.
> 
> Thanks,
> Paul.
> --
> 
> > 
> > Cheers,
> > 
> > Moritz
> 

Thanks for the feedback and explanations!

I've retested my stuff with it all built as modules (mgr, bridged,
and fpga-region) and it all works that way as well as built in.
So I'll fix up the Kconfig as tristates for everybody.  Also
I'll add some dependencies as FPGA_REGION should be dependent
on FPGA_BRIDGE.

Alan

Re: [PATCH v2] fpga manager: Add hardware dependency to Zynq driver

[atull]

On Thu, 8 Sep 2016, Jean Delvare wrote:

Hi Greg,

Can you take in this patch?

Thanks,
Alan

> The Zynq FPGA manager driver serves no purpose on other architectures
> so hide it unless build-testing.
> 
> Signed-off-by: Jean Delvare 
> Acked-by: Moritz Fischer 
> Acked-by: Alan Tull 
> Acked-by: Michal Simek 
> Cc: "Sören Brinkmann" 
> ---
> Changes since v1:
> * Rebased on top of v4.8-rc5.
> 
> I have already sent this patch on 2016-01-22 and 2016-07-07, it was
> approved by everybody involved each time, but apparently it got lost
> somewhere as I still can't find it upstream.
> 
>  drivers/fpga/Kconfig |1 +
>  1 file changed, 1 insertion(+)
> 
> --- linux-4.8-rc5.orig/drivers/fpga/Kconfig   2016-09-08 10:22:11.166730687 
> +0200
> +++ linux-4.8-rc5/drivers/fpga/Kconfig2016-09-08 10:22:38.621067814 
> +0200
> @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
>  
>  config FPGA_MGR_ZYNQ_FPGA
>   tristate "Xilinx Zynq FPGA"
> + depends on ARCH_ZYNQ || COMPILE_TEST
>   depends on HAS_DMA
>   help
> FPGA manager driver support for Xilinx Zynq FPGAs.
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> 

Re: [PATCH v2] fpga manager: Add hardware dependency to Zynq driver

[atull]

On Thu, 8 Sep 2016, Jean Delvare wrote:

Hi Greg,

Can you take in this patch?

Thanks,
Alan

> The Zynq FPGA manager driver serves no purpose on other architectures
> so hide it unless build-testing.
> 
> Signed-off-by: Jean Delvare 
> Acked-by: Moritz Fischer 
> Acked-by: Alan Tull 
> Acked-by: Michal Simek 
> Cc: "Sören Brinkmann" 
> ---
> Changes since v1:
> * Rebased on top of v4.8-rc5.
> 
> I have already sent this patch on 2016-01-22 and 2016-07-07, it was
> approved by everybody involved each time, but apparently it got lost
> somewhere as I still can't find it upstream.
> 
>  drivers/fpga/Kconfig |1 +
>  1 file changed, 1 insertion(+)
> 
> --- linux-4.8-rc5.orig/drivers/fpga/Kconfig   2016-09-08 10:22:11.166730687 
> +0200
> +++ linux-4.8-rc5/drivers/fpga/Kconfig2016-09-08 10:22:38.621067814 
> +0200
> @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
>  
>  config FPGA_MGR_ZYNQ_FPGA
>   tristate "Xilinx Zynq FPGA"
> + depends on ARCH_ZYNQ || COMPILE_TEST
>   depends on HAS_DMA
>   help
> FPGA manager driver support for Xilinx Zynq FPGAs.
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> 

Re: [PATCH v18 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 14 Jul 2016, Paul Gortmaker wrote:

> On Tue, Jul 12, 2016 at 3:36 PM, Alan Tull  
> wrote:
> > Supports Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> >
> > Allows enabling/disabling the bridges through the FPGA
> > Bridge Framework API functions.
> >
> > The fpga2sdram driver only supports enabling and disabling
> > of the ports that been configured early on.  This is due to
> > a hardware limitation where the read, write, and command
> > ports on the fpga2sdram bridge can only be reconfigured
> > while there are no transactions to the sdram, i.e. when
> > running out of OCRAM before the kernel boots.
> >
> > Device tree property 'init-val' configures the driver to
> > enable or disable the bridge during probe.  If the property
> > does not exist, the driver will leave the bridge in its
> > current state.
> >
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > Signed-off-by: Dinh Nguyen 
> > ---
> > v2:  Use resets instead of directly writing reset registers
> > v12: Bump version to align with simple-fpga-bus version
> >  Get rid of the sysfs interface
> >  fpga2sdram: get configuration stored in handoff register
> > v13: Remove unneeded WARN_ON
> >  Change property from init-val to bridge-enable
> >  Checkpatch cleanup
> >  Fix email address
> > v14: use module_platform_driver
> >  remove unused struct field and some #defines
> >  don't really need exclamation points on error msgs
> >  *const* struct fpga_bridge_ops
> > v15: No change in this patch for v15 of this patch set
> > v16: No change in this patch for v16 of this patch set
> > v17: No change to this patch for v17 of this patch set
> > v18: Eliminate need to specify reset names since only one reset
> > ---
> >  drivers/fpga/Kconfig |   7 ++
> >  drivers/fpga/Makefile|   1 +
> >  drivers/fpga/altera-fpga2sdram.c | 174 
> >  drivers/fpga/altera-hps2fpga.c   | 213 
> > +++
> >  4 files changed, 395 insertions(+)
> >  create mode 100644 drivers/fpga/altera-fpga2sdram.c
> >  create mode 100644 drivers/fpga/altera-hps2fpga.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index ec81e21..b346166 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -39,6 +39,13 @@ config FPGA_BRIDGE
> >   Say Y here if you want to support bridges connected between host
> >  processors and FPGAs or between FPGAs.
> >
> > +config SOCFPGA_FPGA_BRIDGE
> > +   bool "Altera SoCFPGA FPGA Bridges"
> > +   depends on ARCH_SOCFPGA && FPGA_BRIDGE
> > +   help
> > + Say Y to enable drivers for FPGA bridges for Altera SOCFPGA
> > + devices.
> > +
> >  endif # FPGA
> >
> >  endmenu
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 8d746c3..e658436 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)  += zynq-fpga.o
> >
> >  # FPGA Bridge Drivers
> >  obj-$(CONFIG_FPGA_BRIDGE)  += fpga-bridge.o
> > +obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)  += altera-hps2fpga.o 
> > altera-fpga2sdram.o
> >
> >  # High Level Interfaces
> >  obj-$(CONFIG_FPGA_REGION)  += fpga-region.o
> > diff --git a/drivers/fpga/altera-fpga2sdram.c 
> > b/drivers/fpga/altera-fpga2sdram.c
> > new file mode 100644
> > index 000..91f4a40
> > --- /dev/null
> > +++ b/drivers/fpga/altera-fpga2sdram.c
> > @@ -0,0 +1,174 @@
> > +/*
> > + * FPGA to SDRAM Bridge Driver for Altera SoCFPGA Devices
> > + *
> > + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
> > + *
> > + * 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 .
> > + */
> > +
> > +/*
> > + * This driver manages a bridge between an FPGA and the SDRAM used by the 
> > ARM
> > + * host processor system (HPS).
> > + *
> > + * The bridge contains 4 read ports, 4 write ports, and 6 command ports.
> > + * Reconfiguring these ports requires that no SDRAM transactions occur 
> > during
> > + * reconfiguration.  The code reconfiguring the ports cannot run out of 
> > SDRAM
> > + * nor can the FPGA access the SDRAM during reconfiguration.  This driver 
> > 

Re: [PATCH v18 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 14 Jul 2016, Paul Gortmaker wrote:

> On Tue, Jul 12, 2016 at 3:36 PM, Alan Tull  
> wrote:
> > Supports Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> >
> > Allows enabling/disabling the bridges through the FPGA
> > Bridge Framework API functions.
> >
> > The fpga2sdram driver only supports enabling and disabling
> > of the ports that been configured early on.  This is due to
> > a hardware limitation where the read, write, and command
> > ports on the fpga2sdram bridge can only be reconfigured
> > while there are no transactions to the sdram, i.e. when
> > running out of OCRAM before the kernel boots.
> >
> > Device tree property 'init-val' configures the driver to
> > enable or disable the bridge during probe.  If the property
> > does not exist, the driver will leave the bridge in its
> > current state.
> >
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > Signed-off-by: Dinh Nguyen 
> > ---
> > v2:  Use resets instead of directly writing reset registers
> > v12: Bump version to align with simple-fpga-bus version
> >  Get rid of the sysfs interface
> >  fpga2sdram: get configuration stored in handoff register
> > v13: Remove unneeded WARN_ON
> >  Change property from init-val to bridge-enable
> >  Checkpatch cleanup
> >  Fix email address
> > v14: use module_platform_driver
> >  remove unused struct field and some #defines
> >  don't really need exclamation points on error msgs
> >  *const* struct fpga_bridge_ops
> > v15: No change in this patch for v15 of this patch set
> > v16: No change in this patch for v16 of this patch set
> > v17: No change to this patch for v17 of this patch set
> > v18: Eliminate need to specify reset names since only one reset
> > ---
> >  drivers/fpga/Kconfig |   7 ++
> >  drivers/fpga/Makefile|   1 +
> >  drivers/fpga/altera-fpga2sdram.c | 174 
> >  drivers/fpga/altera-hps2fpga.c   | 213 
> > +++
> >  4 files changed, 395 insertions(+)
> >  create mode 100644 drivers/fpga/altera-fpga2sdram.c
> >  create mode 100644 drivers/fpga/altera-hps2fpga.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index ec81e21..b346166 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -39,6 +39,13 @@ config FPGA_BRIDGE
> >   Say Y here if you want to support bridges connected between host
> >  processors and FPGAs or between FPGAs.
> >
> > +config SOCFPGA_FPGA_BRIDGE
> > +   bool "Altera SoCFPGA FPGA Bridges"
> > +   depends on ARCH_SOCFPGA && FPGA_BRIDGE
> > +   help
> > + Say Y to enable drivers for FPGA bridges for Altera SOCFPGA
> > + devices.
> > +
> >  endif # FPGA
> >
> >  endmenu
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 8d746c3..e658436 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)  += zynq-fpga.o
> >
> >  # FPGA Bridge Drivers
> >  obj-$(CONFIG_FPGA_BRIDGE)  += fpga-bridge.o
> > +obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)  += altera-hps2fpga.o 
> > altera-fpga2sdram.o
> >
> >  # High Level Interfaces
> >  obj-$(CONFIG_FPGA_REGION)  += fpga-region.o
> > diff --git a/drivers/fpga/altera-fpga2sdram.c 
> > b/drivers/fpga/altera-fpga2sdram.c
> > new file mode 100644
> > index 000..91f4a40
> > --- /dev/null
> > +++ b/drivers/fpga/altera-fpga2sdram.c
> > @@ -0,0 +1,174 @@
> > +/*
> > + * FPGA to SDRAM Bridge Driver for Altera SoCFPGA Devices
> > + *
> > + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
> > + *
> > + * 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 .
> > + */
> > +
> > +/*
> > + * This driver manages a bridge between an FPGA and the SDRAM used by the 
> > ARM
> > + * host processor system (HPS).
> > + *
> > + * The bridge contains 4 read ports, 4 write ports, and 6 command ports.
> > + * Reconfiguring these ports requires that no SDRAM transactions occur 
> > during
> > + * reconfiguration.  The code reconfiguring the ports cannot run out of 
> > SDRAM
> > + * nor can the FPGA access the SDRAM during reconfiguration.  This driver 
> > does
> > + * not support reconfiguring the ports.  The ports are configured by code
> > + * running out of on 

Re: [PATCH v18 2/6] ARM: socfpga: add bindings document for fpga bridge drivers

[atull]

On Mon, 1 Aug 2016, Rob Herring wrote:

> On Tue, Jul 12, 2016 at 02:36:41PM -0500, Alan Tull wrote:
> > Add bindings documentation for Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > Signed-off-by: Dinh Nguyen 
> > ---
> > v2:  separate into 2 documents for the 2 drivers
> > v12: bump version to line up with simple-fpga-bus version
> >  remove Linux specific notes such as references to sysfs
> >  move non-DT specific documentation elsewhere
> >  remove bindings that would have been used to pass configuration
> >  clean up formatting
> > v13: Remove 'label' property
> >  Change property from init-val to bridge-enable
> >  Fix email address
> > v14: Add resets
> >  Change order of bridges to put lw bridge (controlling bridge) first
> > v15: No change in this patch for v15 of this patch set
> > v16: Added regs property, cleaned up unit addresses
> > v17: No change to this patch in v17 of patch set
> > v18: Changed to not need reset-names property
> >  node names as fpga-bridge@...
> >  labels as fpga_bridge
> >  Add address of fpgaportrst to give and address for f2s bridge
> > ---
> >  .../bindings/fpga/altera-fpga2sdram-bridge.txt | 16 +
> >  .../bindings/fpga/altera-hps2fpga-bridge.txt   | 39 
> > ++
> >  2 files changed, 55 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> 
> Acked-by: Rob Herring 
> 

Thanks!

Alan

Re: [PATCH v18 2/6] ARM: socfpga: add bindings document for fpga bridge drivers

[atull]

On Mon, 1 Aug 2016, Rob Herring wrote:

> On Tue, Jul 12, 2016 at 02:36:41PM -0500, Alan Tull wrote:
> > Add bindings documentation for Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> > 
> > Signed-off-by: Alan Tull 
> > Signed-off-by: Matthew Gerlach 
> > Signed-off-by: Dinh Nguyen 
> > ---
> > v2:  separate into 2 documents for the 2 drivers
> > v12: bump version to line up with simple-fpga-bus version
> >  remove Linux specific notes such as references to sysfs
> >  move non-DT specific documentation elsewhere
> >  remove bindings that would have been used to pass configuration
> >  clean up formatting
> > v13: Remove 'label' property
> >  Change property from init-val to bridge-enable
> >  Fix email address
> > v14: Add resets
> >  Change order of bridges to put lw bridge (controlling bridge) first
> > v15: No change in this patch for v15 of this patch set
> > v16: Added regs property, cleaned up unit addresses
> > v17: No change to this patch in v17 of patch set
> > v18: Changed to not need reset-names property
> >  node names as fpga-bridge@...
> >  labels as fpga_bridge
> >  Add address of fpgaportrst to give and address for f2s bridge
> > ---
> >  .../bindings/fpga/altera-fpga2sdram-bridge.txt | 16 +
> >  .../bindings/fpga/altera-hps2fpga-bridge.txt   | 39 
> > ++
> >  2 files changed, 55 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> 
> Acked-by: Rob Herring 
> 

Thanks!

Alan

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 28 Jul 2016, Trent Piepho wrote:

> On Thu, 2016-07-28 at 10:21 -0500, atull wrote:
> > > >
> > > > This isn't going work if more than one bridge is used.  Each bridge has
> > > > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > > > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > > > will turn off the 1st bridge when it re-write the l3 register.
> > > 
> > > I can confirm this is exactly what happens with tag
> > > "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
> > > from altera-opensource/linux-socfpga which includes more or less the
> > > code in this patch. If you have 2 bridges (lw-hps2fpga and hps2fpga)
> > > you end up with only one of them being visible. Easily spot by logging
> > > l3_remap_value being passed to regmap_write()...
> > > 
> > 
> > Anatolij kindly provided a patch for this issue.  I'll push it
> > to my github repo when I can.
> 
> I still think a better solution would be to allow the syscon driver
> manage shared access.  The purpose of syscon is to manage access to a
> shared resource from multiple devices.  And regmap already has the
> ability to cache a write-only register and allow thread safe access to
> modify bits in said register.  The problem is just the pain of trying to
> do anything to syscon DT bindings.  Something like "write-only" in the
> syscon binding that sets a couple values in the regmap_config is all
> that's necessary.
> 
> Might as well not use syscon at all and have the bridge driver map the
> l3regs itself, since it doesn't really use syscon for anything.
> 

I agree.  Just need time to do it.

Alan

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 28 Jul 2016, Trent Piepho wrote:

> On Thu, 2016-07-28 at 10:21 -0500, atull wrote:
> > > >
> > > > This isn't going work if more than one bridge is used.  Each bridge has
> > > > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > > > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > > > will turn off the 1st bridge when it re-write the l3 register.
> > > 
> > > I can confirm this is exactly what happens with tag
> > > "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
> > > from altera-opensource/linux-socfpga which includes more or less the
> > > code in this patch. If you have 2 bridges (lw-hps2fpga and hps2fpga)
> > > you end up with only one of them being visible. Easily spot by logging
> > > l3_remap_value being passed to regmap_write()...
> > > 
> > 
> > Anatolij kindly provided a patch for this issue.  I'll push it
> > to my github repo when I can.
> 
> I still think a better solution would be to allow the syscon driver
> manage shared access.  The purpose of syscon is to manage access to a
> shared resource from multiple devices.  And regmap already has the
> ability to cache a write-only register and allow thread safe access to
> modify bits in said register.  The problem is just the pain of trying to
> do anything to syscon DT bindings.  Something like "write-only" in the
> syscon binding that sets a couple values in the regmap_config is all
> that's necessary.
> 
> Might as well not use syscon at all and have the bridge driver map the
> l3regs itself, since it doesn't really use syscon for anything.
> 

I agree.  Just need time to do it.

Alan

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 28 Jul 2016, Andrea Galbusera wrote:

> On Fri, Jun 10, 2016 at 4:18 AM, Trent Piepho  wrote:
> > On Fri, 2016-02-05 at 15:30 -0600, at...@opensource.altera.com wrote:
> >> Supports Altera SOCFPGA bridges:
> >>  * fpga2sdram
> >>  * fpga2hps
> >>  * hps2fpga
> >>  * lwhps2fpga
> >>
> >> Allows enabling/disabling the bridges through the FPGA
> >> Bridge Framework API functions.
> >
> > I'm replying to v16 because it exists on gmane, while v17 appears not
> > to.  lkml.org's forward feature appears to be broken so I can't reply to
> > that message (no way to get message-id).  But v17 of this patch should
> > be the same.  If a v18 was posted, I've not been able to find it.
> >
> >> diff --git a/drivers/fpga/altera-hps2fpga.c 
> >> b/drivers/fpga/altera-hps2fpga.c
> >> new file mode 100644
> >> index 000..c15df47
> >> --- /dev/null
> >> +++ b/drivers/fpga/altera-hps2fpga.c
> >> @@ -0,0 +1,213 @@
> >> +/*
> >> + * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
> >> + *
> >> + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
> >> + *
> >> + * 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 .
> >> + */
> >> +
> >> +/*
> >> + * This driver manages bridges on a Altera SOCFPGA between the ARM host
> >> + * processor system (HPS) and the embedded FPGA.
> >> + *
> >> + * This driver supports enabling and disabling of the configured ports, 
> >> which
> >> + * allows for safe reprogramming of the FPGA, assuming that the new FPGA 
> >> image
> >> + * uses the same port configuration.  Bridges must be disabled before
> >> + * reprogramming the FPGA and re-enabled after the FPGA has been 
> >> programmed.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +#define ALT_L3_REMAP_OFST0x0
> >> +#define ALT_L3_REMAP_MPUZERO_MSK 0x0001
> >> +#define ALT_L3_REMAP_H2F_MSK 0x0008
> >> +#define ALT_L3_REMAP_LWH2F_MSK   0x0010
> >> +
> >> +#define HPS2FPGA_BRIDGE_NAME "hps2fpga"
> >> +#define LWHPS2FPGA_BRIDGE_NAME   "lwhps2fpga"
> >> +#define FPGA2HPS_BRIDGE_NAME "fpga2hps"
> >> +
> >> +struct altera_hps2fpga_data {
> >> + const char *name;
> >> + struct reset_control *bridge_reset;
> >> + struct regmap *l3reg;
> >> + /* The L3 REMAP register is write only, so keep a cached value. */
> >> + unsigned int l3_remap_value;
> >> + unsigned int remap_mask;
> >> + struct clk *clk;
> >> +};
> >> +
> >> +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> >> +{
> >> + struct altera_hps2fpga_data *priv = bridge->priv;
> >> +
> >> + return reset_control_status(priv->bridge_reset);
> >> +}
> >> +
> >> +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> >> + bool enable)
> >> +{
> >> + int ret;
> >> +
> >> + /* bring bridge out of reset */
> >> + if (enable)
> >> + ret = reset_control_deassert(priv->bridge_reset);
> >> + else
> >> + ret = reset_control_assert(priv->bridge_reset);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Allow bridge to be visible to L3 masters or not */
> >> + if (priv->remap_mask) {
> >> + priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> >
> > Doesn't seem like this belongs here.  I realize the write-only register
> > is a problem.  Maybe the syscon driver should be initializing this
> > value?
> >
> >> +
> >> + if (enable)
> >> + priv->l3_remap_value |= priv->remap_mask;
> >> + else
> >> + priv->l3_remap_value &= ~priv->remap_mask;
> >> +
> >> + ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> >> +priv->l3_remap_value);
> >
> > This isn't going work if more than one bridge is used.  Each bridge has
> > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > will turn off the 1st bridge when it re-write the l3 register.
> 
> I can confirm this is exactly what happens with tag
> "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of 

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Thu, 28 Jul 2016, Andrea Galbusera wrote:

> On Fri, Jun 10, 2016 at 4:18 AM, Trent Piepho  wrote:
> > On Fri, 2016-02-05 at 15:30 -0600, at...@opensource.altera.com wrote:
> >> Supports Altera SOCFPGA bridges:
> >>  * fpga2sdram
> >>  * fpga2hps
> >>  * hps2fpga
> >>  * lwhps2fpga
> >>
> >> Allows enabling/disabling the bridges through the FPGA
> >> Bridge Framework API functions.
> >
> > I'm replying to v16 because it exists on gmane, while v17 appears not
> > to.  lkml.org's forward feature appears to be broken so I can't reply to
> > that message (no way to get message-id).  But v17 of this patch should
> > be the same.  If a v18 was posted, I've not been able to find it.
> >
> >> diff --git a/drivers/fpga/altera-hps2fpga.c 
> >> b/drivers/fpga/altera-hps2fpga.c
> >> new file mode 100644
> >> index 000..c15df47
> >> --- /dev/null
> >> +++ b/drivers/fpga/altera-hps2fpga.c
> >> @@ -0,0 +1,213 @@
> >> +/*
> >> + * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
> >> + *
> >> + *  Copyright (C) 2013-2015 Altera Corporation, All Rights Reserved.
> >> + *
> >> + * 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 .
> >> + */
> >> +
> >> +/*
> >> + * This driver manages bridges on a Altera SOCFPGA between the ARM host
> >> + * processor system (HPS) and the embedded FPGA.
> >> + *
> >> + * This driver supports enabling and disabling of the configured ports, 
> >> which
> >> + * allows for safe reprogramming of the FPGA, assuming that the new FPGA 
> >> image
> >> + * uses the same port configuration.  Bridges must be disabled before
> >> + * reprogramming the FPGA and re-enabled after the FPGA has been 
> >> programmed.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +#define ALT_L3_REMAP_OFST0x0
> >> +#define ALT_L3_REMAP_MPUZERO_MSK 0x0001
> >> +#define ALT_L3_REMAP_H2F_MSK 0x0008
> >> +#define ALT_L3_REMAP_LWH2F_MSK   0x0010
> >> +
> >> +#define HPS2FPGA_BRIDGE_NAME "hps2fpga"
> >> +#define LWHPS2FPGA_BRIDGE_NAME   "lwhps2fpga"
> >> +#define FPGA2HPS_BRIDGE_NAME "fpga2hps"
> >> +
> >> +struct altera_hps2fpga_data {
> >> + const char *name;
> >> + struct reset_control *bridge_reset;
> >> + struct regmap *l3reg;
> >> + /* The L3 REMAP register is write only, so keep a cached value. */
> >> + unsigned int l3_remap_value;
> >> + unsigned int remap_mask;
> >> + struct clk *clk;
> >> +};
> >> +
> >> +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> >> +{
> >> + struct altera_hps2fpga_data *priv = bridge->priv;
> >> +
> >> + return reset_control_status(priv->bridge_reset);
> >> +}
> >> +
> >> +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> >> + bool enable)
> >> +{
> >> + int ret;
> >> +
> >> + /* bring bridge out of reset */
> >> + if (enable)
> >> + ret = reset_control_deassert(priv->bridge_reset);
> >> + else
> >> + ret = reset_control_assert(priv->bridge_reset);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Allow bridge to be visible to L3 masters or not */
> >> + if (priv->remap_mask) {
> >> + priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> >
> > Doesn't seem like this belongs here.  I realize the write-only register
> > is a problem.  Maybe the syscon driver should be initializing this
> > value?
> >
> >> +
> >> + if (enable)
> >> + priv->l3_remap_value |= priv->remap_mask;
> >> + else
> >> + priv->l3_remap_value &= ~priv->remap_mask;
> >> +
> >> + ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> >> +priv->l3_remap_value);
> >
> > This isn't going work if more than one bridge is used.  Each bridge has
> > its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> > have just the bit for it's own remap set.  The 2nd bridge to be enabled
> > will turn off the 1st bridge when it re-write the l3 register.
> 
> I can confirm this is exactly what happens with tag
> "rel_socfpga-4.1.22-ltsi_16.06.02_pr" of socfpga-4.1.22-ltsi branch
> from 

Re: [PATCH] fpga: zynq: Add hardware dependencies

[atull]

On Sun, 17 Jul 2016, Moritz Fischer wrote:

> On Thu, Jul 7, 2016 at 1:07 AM, Jean Delvare  wrote:
> > The zynq-fpga driver is specific to its architecture, so do not
> > propose it on other architectures, unless build-testing.
> >
> > Signed-off-by: Jean Delvare 
> > Cc: Moritz Fischer 
> > Cc: Alan Tull 
> > Cc: Greg Kroah-Hartman 
> > ---
> >  drivers/fpga/Kconfig |1 +
> >  1 file changed, 1 insertion(+)
> >
> > --- linux-4.7-rc6.orig/drivers/fpga/Kconfig 2016-07-04 
> > 08:01:00.0 +0200
> > +++ linux-4.7-rc6/drivers/fpga/Kconfig  2016-07-07 09:29:50.395807394 +0200
> > @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
> >
> >  config FPGA_MGR_ZYNQ_FPGA
> > tristate "Xilinx Zynq FPGA"
> > +   depends on ARCH_ZYNQ || COMPILE_TEST
> > help
> >   FPGA manager driver support for Xilinx Zynq FPGAs.
> >
> >
> > --
> > Jean Delvare
> > SUSE L3 Support
> 
> Acked-By: Moritz Fischer 
> 

Acked-by: Alan Tull 

Re: [PATCH] fpga: zynq: Add hardware dependencies

[atull]

On Sun, 17 Jul 2016, Moritz Fischer wrote:

> On Thu, Jul 7, 2016 at 1:07 AM, Jean Delvare  wrote:
> > The zynq-fpga driver is specific to its architecture, so do not
> > propose it on other architectures, unless build-testing.
> >
> > Signed-off-by: Jean Delvare 
> > Cc: Moritz Fischer 
> > Cc: Alan Tull 
> > Cc: Greg Kroah-Hartman 
> > ---
> >  drivers/fpga/Kconfig |1 +
> >  1 file changed, 1 insertion(+)
> >
> > --- linux-4.7-rc6.orig/drivers/fpga/Kconfig 2016-07-04 
> > 08:01:00.0 +0200
> > +++ linux-4.7-rc6/drivers/fpga/Kconfig  2016-07-07 09:29:50.395807394 +0200
> > @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
> >
> >  config FPGA_MGR_ZYNQ_FPGA
> > tristate "Xilinx Zynq FPGA"
> > +   depends on ARCH_ZYNQ || COMPILE_TEST
> > help
> >   FPGA manager driver support for Xilinx Zynq FPGAs.
> >
> >
> > --
> > Jean Delvare
> > SUSE L3 Support
> 
> Acked-By: Moritz Fischer 
> 

Acked-by: Alan Tull 

Re: [PATCH 1/2] ARM: socfpga: add bindings doc for arria10 fpga manager

[atull]

On Sun, 17 Jul 2016, Moritz Fischer wrote:

> Acked-By: Moritz Fischer 

Thanks Moritz!

Alan

> 
> On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull  
> wrote:
> > Add a device tree bindings document for the SoCFPGA Arria10
> > FPGA Manager driver.
> >
> > Signed-off-by: Alan Tull 
> > ---
> >  .../bindings/fpga/altera-socfpga-a10-fpga-mgr.txt | 19 
> > +++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt 
> > b/Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> > new file mode 100644
> > index 000..2fd8e7a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> > @@ -0,0 +1,19 @@
> > +Altera SOCFPGA Arria10 FPGA Manager
> > +
> > +Required properties:
> > +- compatible : should contain "altr,socfpga-a10-fpga-mgr"
> > +- reg: base address and size for memory mapped io.
> > +   - The first index is for FPGA manager register access.
> > +   - The second index is for writing FPGA configuration data.
> > +- resets : Phandle and reset specifier for the device's reset.
> > +- clocks : Clocks used by the device.
> > +
> > +Example:
> > +
> > +   fpga_mgr: fpga-mgr@ffd03000 {
> > +   compatible = "altr,socfpga-a10-fpga-mgr";
> > +   reg = <0xffd03000 0x100
> > +  0xffcfe400 0x20>;
> > +   clocks = <_mp_clk>;
> > +   resets = < FPGAMGR_RESET>;
> > +   };
> > --
> > 2.9.1
> >
> 

Re: [PATCH 1/2] ARM: socfpga: add bindings doc for arria10 fpga manager

[atull]

On Sun, 17 Jul 2016, Moritz Fischer wrote:

> Acked-By: Moritz Fischer 

Thanks Moritz!

Alan

> 
> On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull  
> wrote:
> > Add a device tree bindings document for the SoCFPGA Arria10
> > FPGA Manager driver.
> >
> > Signed-off-by: Alan Tull 
> > ---
> >  .../bindings/fpga/altera-socfpga-a10-fpga-mgr.txt | 19 
> > +++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt 
> > b/Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> > new file mode 100644
> > index 000..2fd8e7a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> > @@ -0,0 +1,19 @@
> > +Altera SOCFPGA Arria10 FPGA Manager
> > +
> > +Required properties:
> > +- compatible : should contain "altr,socfpga-a10-fpga-mgr"
> > +- reg: base address and size for memory mapped io.
> > +   - The first index is for FPGA manager register access.
> > +   - The second index is for writing FPGA configuration data.
> > +- resets : Phandle and reset specifier for the device's reset.
> > +- clocks : Clocks used by the device.
> > +
> > +Example:
> > +
> > +   fpga_mgr: fpga-mgr@ffd03000 {
> > +   compatible = "altr,socfpga-a10-fpga-mgr";
> > +   reg = <0xffd03000 0x100
> > +  0xffcfe400 0x20>;
> > +   clocks = <_mp_clk>;
> > +   resets = < FPGAMGR_RESET>;
> > +   };
> > --
> > 2.9.1
> >
> 

Re: [PATCH 1/2] ARM: socfpga: add bindings doc for arria10 fpga manager

[atull]

On Sat, 16 Jul 2016, Rob Herring wrote:

> On Tue, Jul 12, 2016 at 02:07:08PM -0500, Alan Tull wrote:
> > Add a device tree bindings document for the SoCFPGA Arria10
> > FPGA Manager driver.
> > 
> > Signed-off-by: Alan Tull 
> > ---
> >  .../bindings/fpga/altera-socfpga-a10-fpga-mgr.txt | 19 
> > +++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> 
> Acked-by: Rob Herring 
> 

Thanks!

Alan

Re: [PATCH 1/2] ARM: socfpga: add bindings doc for arria10 fpga manager

[atull]

On Sat, 16 Jul 2016, Rob Herring wrote:

> On Tue, Jul 12, 2016 at 02:07:08PM -0500, Alan Tull wrote:
> > Add a device tree bindings document for the SoCFPGA Arria10
> > FPGA Manager driver.
> > 
> > Signed-off-by: Alan Tull 
> > ---
> >  .../bindings/fpga/altera-socfpga-a10-fpga-mgr.txt | 19 
> > +++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/fpga/altera-socfpga-a10-fpga-mgr.txt
> 
> Acked-by: Rob Herring 
> 

Thanks!

Alan

Re: [PATCH 2/2] fpga-manager: Add Socfpga Arria10 support

[atull]

On Tue, 12 Jul 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> couple of nits inline below.
> 

Hi Moritz!

Thanks for your review!

> On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull  
> wrote:
> 
> > +static int socfpga_a10_fpga_write_complete(struct fpga_manager *mgr, u32 
> > flags)
> > +{
> > +   struct a10_fpga_priv *priv = mgr->priv;
> > +   u32 reg;
> > +   int ret;
> > +
> > +   /* Wait for pr_done */
> > +   ret = socfpga_a10_fpga_wait_for_pr_done(priv);
> > +
> > +   /* Clear pr_request */
> > +   regmap_update_bits(priv->regmap, A10_FPGAMGR_IMGCFG_CTL_01_OFST,
> > +  A10_FPGAMGR_IMGCFG_CTL_01_S2F_PR_REQUEST, 0);
> > +
> > +   /* Send some clocks to clear out any errors */
> > +   socfpga_a10_fpga_generate_dclks(priv, 256);
> > +
> > +   /* Disable s2f dclk and data */
> > +   regmap_update_bits(priv->regmap, A10_FPGAMGR_IMGCFG_CTL_02_OFST,
> > +  A10_FPGAMGR_IMGCFG_CTL_02_EN_CFG_CTRL, 0);
> 
> Maybe replace 0 with named constant.

Generally I use named constants, but since regmap_updates_bits uses a
mask, it is clear that we're clearing the
A10_FPGAMGR_IMGCFG_CTL_02_EN_CFG_CTRL bit.


> > +static int socfpga_a10_fpga_probe(struct platform_device *pdev)
> > +{
> > +   struct device *dev = >dev;
> > +   struct a10_fpga_priv *priv;
> > +   void __iomem *reg_base;
> > +   struct resource *res;
> > +   int ret;
> > +
> > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   /* First mmio base is for register access */
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +   reg_base = devm_ioremap_resource(dev, res);
> > +   if (IS_ERR(reg_base))
> > +   return PTR_ERR(reg_base);
> > +
> > +   /* Second mmio base is for writing FPGA image data */
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +   priv->fpga_data_addr = devm_ioremap_resource(dev, res);
> > +   if (IS_ERR(priv->fpga_data_addr))
> > +   return PTR_ERR(priv->fpga_data_addr);
> > +
> > +   /* regmap for register access */
> > +   priv->regmap = devm_regmap_init_mmio(dev, reg_base,
> > +
> > _a10_fpga_regmap_config);
> > +   if (IS_ERR(priv->regmap))
> > +   return -ENODEV;
> > +
> > +   priv->clk = devm_clk_get(dev, NULL);
> > +   if (IS_ERR(priv->clk)) {
> > +   dev_err(dev, "no clock specified\n");
> > +   return PTR_ERR(priv->clk);
> > +   }
> > +
> > +   ret = clk_prepare_enable(priv->clk);
> > +   if (ret) {
> > +   dev_err(dev, "could not enable clock\n");
> > +   clk_put(priv->clk);
> 
> Seen that you used devm_clk_get() is this one necessary?

Yes, this is wrong.  I'll fix it.

Thanks,
Alan

> 
> > +static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > +{
> > +   struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > +   struct a10_fpga_priv *priv = mgr->priv;
> > +
> > +   fpga_mgr_unregister(>dev);
> > +   clk_disable_unprepare(priv->clk);
> > +   clk_put(priv->clk);
> 
> Same here, if needed at all shouldn't it be devm_clk_put() ?
> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH 2/2] fpga-manager: Add Socfpga Arria10 support

[atull]

On Tue, 12 Jul 2016, Moritz Fischer wrote:

> Hi Alan,
> 
> couple of nits inline below.
> 

Hi Moritz!

Thanks for your review!

> On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull  
> wrote:
> 
> > +static int socfpga_a10_fpga_write_complete(struct fpga_manager *mgr, u32 
> > flags)
> > +{
> > +   struct a10_fpga_priv *priv = mgr->priv;
> > +   u32 reg;
> > +   int ret;
> > +
> > +   /* Wait for pr_done */
> > +   ret = socfpga_a10_fpga_wait_for_pr_done(priv);
> > +
> > +   /* Clear pr_request */
> > +   regmap_update_bits(priv->regmap, A10_FPGAMGR_IMGCFG_CTL_01_OFST,
> > +  A10_FPGAMGR_IMGCFG_CTL_01_S2F_PR_REQUEST, 0);
> > +
> > +   /* Send some clocks to clear out any errors */
> > +   socfpga_a10_fpga_generate_dclks(priv, 256);
> > +
> > +   /* Disable s2f dclk and data */
> > +   regmap_update_bits(priv->regmap, A10_FPGAMGR_IMGCFG_CTL_02_OFST,
> > +  A10_FPGAMGR_IMGCFG_CTL_02_EN_CFG_CTRL, 0);
> 
> Maybe replace 0 with named constant.

Generally I use named constants, but since regmap_updates_bits uses a
mask, it is clear that we're clearing the
A10_FPGAMGR_IMGCFG_CTL_02_EN_CFG_CTRL bit.


> > +static int socfpga_a10_fpga_probe(struct platform_device *pdev)
> > +{
> > +   struct device *dev = >dev;
> > +   struct a10_fpga_priv *priv;
> > +   void __iomem *reg_base;
> > +   struct resource *res;
> > +   int ret;
> > +
> > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   /* First mmio base is for register access */
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +   reg_base = devm_ioremap_resource(dev, res);
> > +   if (IS_ERR(reg_base))
> > +   return PTR_ERR(reg_base);
> > +
> > +   /* Second mmio base is for writing FPGA image data */
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +   priv->fpga_data_addr = devm_ioremap_resource(dev, res);
> > +   if (IS_ERR(priv->fpga_data_addr))
> > +   return PTR_ERR(priv->fpga_data_addr);
> > +
> > +   /* regmap for register access */
> > +   priv->regmap = devm_regmap_init_mmio(dev, reg_base,
> > +
> > _a10_fpga_regmap_config);
> > +   if (IS_ERR(priv->regmap))
> > +   return -ENODEV;
> > +
> > +   priv->clk = devm_clk_get(dev, NULL);
> > +   if (IS_ERR(priv->clk)) {
> > +   dev_err(dev, "no clock specified\n");
> > +   return PTR_ERR(priv->clk);
> > +   }
> > +
> > +   ret = clk_prepare_enable(priv->clk);
> > +   if (ret) {
> > +   dev_err(dev, "could not enable clock\n");
> > +   clk_put(priv->clk);
> 
> Seen that you used devm_clk_get() is this one necessary?

Yes, this is wrong.  I'll fix it.

Thanks,
Alan

> 
> > +static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > +{
> > +   struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > +   struct a10_fpga_priv *priv = mgr->priv;
> > +
> > +   fpga_mgr_unregister(>dev);
> > +   clk_disable_unprepare(priv->clk);
> > +   clk_put(priv->clk);
> 
> Same here, if needed at all shouldn't it be devm_clk_put() ?
> 
> Cheers,
> 
> Moritz
> 

Re: [PATCH 2/2] fpga-manager: Add Socfpga Arria10 support

[atull]

On Tue, 12 Jul 2016, Russell King - ARM Linux wrote:

> On Tue, Jul 12, 2016 at 02:31:05PM -0700, Moritz Fischer wrote:
> > On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull  
> > wrote:
> > > +   priv->clk = devm_clk_get(dev, NULL);
> > > +   if (IS_ERR(priv->clk)) {
> > > +   dev_err(dev, "no clock specified\n");
> > > +   return PTR_ERR(priv->clk);
> > > +   }
> > > +
> > > +   ret = clk_prepare_enable(priv->clk);
> > > +   if (ret) {
> > > +   dev_err(dev, "could not enable clock\n");
> > > +   clk_put(priv->clk);
> > 
> > Seen that you used devm_clk_get() is this one necessary?
> 
> That's actually a bug.

Yes, it's wrong.  And not needed anyway.

>  Never clk_put() a devm_clk_get()'d clock.
> devm_clk_put() is what you want if provided.  However, I think
> this clk_put() call is useless here.
> 
> > > +static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > > +{
> > > +   struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > > +   struct a10_fpga_priv *priv = mgr->priv;
> > > +
> > > +   fpga_mgr_unregister(>dev);
> > > +   clk_disable_unprepare(priv->clk);
> > > +   clk_put(priv->clk);
> > 
> > Same here, if needed at all shouldn't it be devm_clk_put() ?
> 
> And also useless here, as the whole point of the devm_* stuff is to
> clean up the resources that were claimed on probe failure or when
> the device is unbound from its driver.

Thanks for pointing this out.  I'll take out the clk_put's.

Alan

> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

Re: [PATCH 2/2] fpga-manager: Add Socfpga Arria10 support

[atull]

On Tue, 12 Jul 2016, Russell King - ARM Linux wrote:

> On Tue, Jul 12, 2016 at 02:31:05PM -0700, Moritz Fischer wrote:
> > On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull  
> > wrote:
> > > +   priv->clk = devm_clk_get(dev, NULL);
> > > +   if (IS_ERR(priv->clk)) {
> > > +   dev_err(dev, "no clock specified\n");
> > > +   return PTR_ERR(priv->clk);
> > > +   }
> > > +
> > > +   ret = clk_prepare_enable(priv->clk);
> > > +   if (ret) {
> > > +   dev_err(dev, "could not enable clock\n");
> > > +   clk_put(priv->clk);
> > 
> > Seen that you used devm_clk_get() is this one necessary?
> 
> That's actually a bug.

Yes, it's wrong.  And not needed anyway.

>  Never clk_put() a devm_clk_get()'d clock.
> devm_clk_put() is what you want if provided.  However, I think
> this clk_put() call is useless here.
> 
> > > +static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > > +{
> > > +   struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > > +   struct a10_fpga_priv *priv = mgr->priv;
> > > +
> > > +   fpga_mgr_unregister(>dev);
> > > +   clk_disable_unprepare(priv->clk);
> > > +   clk_put(priv->clk);
> > 
> > Same here, if needed at all shouldn't it be devm_clk_put() ?
> 
> And also useless here, as the whole point of the devm_* stuff is to
> clean up the resources that were claimed on probe failure or when
> the device is unbound from its driver.

Thanks for pointing this out.  I'll take out the clk_put's.

Alan

> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

Re: [PATCH v18 0/6] Device Tree support for FPGA Programming

[atull]

On Tue, 12 Jul 2016, Alan Tull wrote:

> v18 has very minimal changes to address comments about the device
> tree bindings and device tree examples in the bindings document.
> 
> The diffstat from v17 is:
>   4 files changed, 11 insertions(+), 18 deletions(-)
> If there are more changes, it would be easier to
> send these upstream and just start adding small
> patches to fix whatever else.

As with v17, this patchset needs the "of overlay notifications"
patch, which has been approved.

  https://lkml.org/lkml/2016/4/19/704

> 
> Alan Tull (6):
>   fpga: add bindings document for fpga region
>   ARM: socfpga: add bindings document for fpga bridge drivers
>   add sysfs document for fpga bridge class
>   fpga: add fpga bridge framework
>   fpga: fpga-region: device tree control for FPGA
>   ARM: socfpga: fpga bridge driver support
> 
>  Documentation/ABI/testing/sysfs-class-fpga-bridge  |  11 +
>  .../bindings/fpga/altera-fpga2sdram-bridge.txt |  16 +
>  .../bindings/fpga/altera-hps2fpga-bridge.txt   |  39 ++
>  .../devicetree/bindings/fpga/fpga-region.txt   | 491 ++
>  drivers/fpga/Kconfig   |  21 +
>  drivers/fpga/Makefile  |   7 +
>  drivers/fpga/altera-fpga2sdram.c   | 174 +++
>  drivers/fpga/altera-hps2fpga.c | 213 
>  drivers/fpga/fpga-bridge.c | 388 ++
>  drivers/fpga/fpga-region.c | 555 
> +
>  include/linux/fpga/fpga-bridge.h   |  55 ++
>  include/linux/fpga/fpga-mgr.h  |   2 +
>  12 files changed, 1972 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
>  create mode 100644 drivers/fpga/altera-fpga2sdram.c
>  create mode 100644 drivers/fpga/altera-hps2fpga.c
>  create mode 100644 drivers/fpga/fpga-bridge.c
>  create mode 100644 drivers/fpga/fpga-region.c
>  create mode 100644 include/linux/fpga/fpga-bridge.h
> 
> -- 
> 2.9.1
> 
> 

Re: [PATCH v18 0/6] Device Tree support for FPGA Programming

[atull]

On Tue, 12 Jul 2016, Alan Tull wrote:

> v18 has very minimal changes to address comments about the device
> tree bindings and device tree examples in the bindings document.
> 
> The diffstat from v17 is:
>   4 files changed, 11 insertions(+), 18 deletions(-)
> If there are more changes, it would be easier to
> send these upstream and just start adding small
> patches to fix whatever else.

As with v17, this patchset needs the "of overlay notifications"
patch, which has been approved.

  https://lkml.org/lkml/2016/4/19/704

> 
> Alan Tull (6):
>   fpga: add bindings document for fpga region
>   ARM: socfpga: add bindings document for fpga bridge drivers
>   add sysfs document for fpga bridge class
>   fpga: add fpga bridge framework
>   fpga: fpga-region: device tree control for FPGA
>   ARM: socfpga: fpga bridge driver support
> 
>  Documentation/ABI/testing/sysfs-class-fpga-bridge  |  11 +
>  .../bindings/fpga/altera-fpga2sdram-bridge.txt |  16 +
>  .../bindings/fpga/altera-hps2fpga-bridge.txt   |  39 ++
>  .../devicetree/bindings/fpga/fpga-region.txt   | 491 ++
>  drivers/fpga/Kconfig   |  21 +
>  drivers/fpga/Makefile  |   7 +
>  drivers/fpga/altera-fpga2sdram.c   | 174 +++
>  drivers/fpga/altera-hps2fpga.c | 213 
>  drivers/fpga/fpga-bridge.c | 388 ++
>  drivers/fpga/fpga-region.c | 555 
> +
>  include/linux/fpga/fpga-bridge.h   |  55 ++
>  include/linux/fpga/fpga-mgr.h  |   2 +
>  12 files changed, 1972 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
>  create mode 100644 drivers/fpga/altera-fpga2sdram.c
>  create mode 100644 drivers/fpga/altera-hps2fpga.c
>  create mode 100644 drivers/fpga/fpga-bridge.c
>  create mode 100644 drivers/fpga/fpga-region.c
>  create mode 100644 include/linux/fpga/fpga-bridge.h
> 
> -- 
> 2.9.1
> 
> 

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Fri, 10 Jun 2016, Trent Piepho wrote:

> On Fri, 2016-02-05 at 15:30 -0600, at...@opensource.altera.com wrote:
> > Supports Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> > 
> > Allows enabling/disabling the bridges through the FPGA
> > Bridge Framework API functions.
> 
> I'm replying to v16 because it exists on gmane, while v17 appears not
> to.  lkml.org's forward feature appears to be broken so I can't reply to
> that message (no way to get message-id).  But v17 of this patch should
> be the same.  If a v18 was posted, I've not been able to find it.

Hi Trent,

Yes, we're up to v17. V18 will be soon, but v16 is good enough for
the purposes of this review.

> > +
> > +#define ALT_L3_REMAP_OFST  0x0
> > +#define ALT_L3_REMAP_MPUZERO_MSK   0x0001
> > +#define ALT_L3_REMAP_H2F_MSK   0x0008
> > +#define ALT_L3_REMAP_LWH2F_MSK 0x0010
> > +
> > +#define HPS2FPGA_BRIDGE_NAME   "hps2fpga"
> > +#define LWHPS2FPGA_BRIDGE_NAME "lwhps2fpga"
> > +#define FPGA2HPS_BRIDGE_NAME   "fpga2hps"
> > +
> > +struct altera_hps2fpga_data {
> > +   const char *name;
> > +   struct reset_control *bridge_reset;
> > +   struct regmap *l3reg;
> > +   /* The L3 REMAP register is write only, so keep a cached value. */
> > +   unsigned int l3_remap_value;
> > +   unsigned int remap_mask;
> > +   struct clk *clk;
> > +};
> > +
> > +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> > +{
> > +   struct altera_hps2fpga_data *priv = bridge->priv;
> > +
> > +   return reset_control_status(priv->bridge_reset);
> > +}
> > +
> > +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> > +   bool enable)
> > +{
> > +   int ret;
> > +
> > +   /* bring bridge out of reset */
> > +   if (enable)
> > +   ret = reset_control_deassert(priv->bridge_reset);
> > +   else
> > +   ret = reset_control_assert(priv->bridge_reset);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Allow bridge to be visible to L3 masters or not */
> > +   if (priv->remap_mask) {
> > +   priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> 
> Doesn't seem like this belongs here.  I realize the write-only register
> is a problem.  Maybe the syscon driver should be initializing this
> value?
> 
> > +
> > +   if (enable)
> > +   priv->l3_remap_value |= priv->remap_mask;
> > +   else
> > +   priv->l3_remap_value &= ~priv->remap_mask;
> > +
> > +   ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> > +  priv->l3_remap_value);
> 
> This isn't going work if more than one bridge is used.  Each bridge has
> its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> have just the bit for it's own remap set.  The 2nd bridge to be enabled
> will turn off the 1st bridge when it re-write the l3 register.
> 
> If all the bridges shared a static global to cache the reg, then this
> problem would be a replaced by a race, since nothing would be managing
> concurrent access to that global from the independent bridge devices.
> 
> How about using the already existing regmap cache ability take care of
> this?  Use regmap_update_bits() to update just the desired bit and let
> remap take care of keeping track caching the register and protecting
> access from multiple users.  It should support that and it should
> support write-only registers, with the creator of the regmap (the syscon
> driver in this case) supplying the initial value of the write-only reg.
> Which is where ALT_L3_REMAP_MPUZERO_MSK could go in.

Please correct me if I'm wrong, but I think that regmap supports
the features you are talking about, but not syscon.

One simple solution would be to take l3_remap_value out of the priv
and let it be shared by all h2f bridges.  That involves the least
amount of change.

> 
> 
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > +   return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > +}
> > +
> > +static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
> > +   .enable_set = alt_hps2fpga_enable_set,
> > +   .enable_show = alt_hps2fpga_enable_show,
> > +};
> > +
> > +static struct altera_hps2fpga_data hps2fpga_data  = {
> > +   .name = HPS2FPGA_BRIDGE_NAME,
> > +   .remap_mask = ALT_L3_REMAP_H2F_MSK,
> > +};
> 
> Each of these data structs also includes space for all the private data
> field of the drivers' state.  Seems a bit inefficient if only two of
> them are configuration data.  It also means only one device of each type
> can exists.  If one creates two bridges of the same type they'll
> (silently) share a priv data struct and randomly break.  And the config
> data structs can't be const.

Our hardware doesn't 

Re: [PATCH v16 6/6] ARM: socfpga: fpga bridge driver support

[atull]

On Fri, 10 Jun 2016, Trent Piepho wrote:

> On Fri, 2016-02-05 at 15:30 -0600, at...@opensource.altera.com wrote:
> > Supports Altera SOCFPGA bridges:
> >  * fpga2sdram
> >  * fpga2hps
> >  * hps2fpga
> >  * lwhps2fpga
> > 
> > Allows enabling/disabling the bridges through the FPGA
> > Bridge Framework API functions.
> 
> I'm replying to v16 because it exists on gmane, while v17 appears not
> to.  lkml.org's forward feature appears to be broken so I can't reply to
> that message (no way to get message-id).  But v17 of this patch should
> be the same.  If a v18 was posted, I've not been able to find it.

Hi Trent,

Yes, we're up to v17. V18 will be soon, but v16 is good enough for
the purposes of this review.

> > +
> > +#define ALT_L3_REMAP_OFST  0x0
> > +#define ALT_L3_REMAP_MPUZERO_MSK   0x0001
> > +#define ALT_L3_REMAP_H2F_MSK   0x0008
> > +#define ALT_L3_REMAP_LWH2F_MSK 0x0010
> > +
> > +#define HPS2FPGA_BRIDGE_NAME   "hps2fpga"
> > +#define LWHPS2FPGA_BRIDGE_NAME "lwhps2fpga"
> > +#define FPGA2HPS_BRIDGE_NAME   "fpga2hps"
> > +
> > +struct altera_hps2fpga_data {
> > +   const char *name;
> > +   struct reset_control *bridge_reset;
> > +   struct regmap *l3reg;
> > +   /* The L3 REMAP register is write only, so keep a cached value. */
> > +   unsigned int l3_remap_value;
> > +   unsigned int remap_mask;
> > +   struct clk *clk;
> > +};
> > +
> > +static int alt_hps2fpga_enable_show(struct fpga_bridge *bridge)
> > +{
> > +   struct altera_hps2fpga_data *priv = bridge->priv;
> > +
> > +   return reset_control_status(priv->bridge_reset);
> > +}
> > +
> > +static int _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> > +   bool enable)
> > +{
> > +   int ret;
> > +
> > +   /* bring bridge out of reset */
> > +   if (enable)
> > +   ret = reset_control_deassert(priv->bridge_reset);
> > +   else
> > +   ret = reset_control_assert(priv->bridge_reset);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Allow bridge to be visible to L3 masters or not */
> > +   if (priv->remap_mask) {
> > +   priv->l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> 
> Doesn't seem like this belongs here.  I realize the write-only register
> is a problem.  Maybe the syscon driver should be initializing this
> value?
> 
> > +
> > +   if (enable)
> > +   priv->l3_remap_value |= priv->remap_mask;
> > +   else
> > +   priv->l3_remap_value &= ~priv->remap_mask;
> > +
> > +   ret = regmap_write(priv->l3reg, ALT_L3_REMAP_OFST,
> > +  priv->l3_remap_value);
> 
> This isn't going work if more than one bridge is used.  Each bridge has
> its own priv and thus priv->l3_remap_value.  Each bridge's priv will
> have just the bit for it's own remap set.  The 2nd bridge to be enabled
> will turn off the 1st bridge when it re-write the l3 register.
> 
> If all the bridges shared a static global to cache the reg, then this
> problem would be a replaced by a race, since nothing would be managing
> concurrent access to that global from the independent bridge devices.
> 
> How about using the already existing regmap cache ability take care of
> this?  Use regmap_update_bits() to update just the desired bit and let
> remap take care of keeping track caching the register and protecting
> access from multiple users.  It should support that and it should
> support write-only registers, with the creator of the regmap (the syscon
> driver in this case) supplying the initial value of the write-only reg.
> Which is where ALT_L3_REMAP_MPUZERO_MSK could go in.

Please correct me if I'm wrong, but I think that regmap supports
the features you are talking about, but not syscon.

One simple solution would be to take l3_remap_value out of the priv
and let it be shared by all h2f bridges.  That involves the least
amount of change.

> 
> 
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > +   return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > +}
> > +
> > +static const struct fpga_bridge_ops altera_hps2fpga_br_ops = {
> > +   .enable_set = alt_hps2fpga_enable_set,
> > +   .enable_show = alt_hps2fpga_enable_show,
> > +};
> > +
> > +static struct altera_hps2fpga_data hps2fpga_data  = {
> > +   .name = HPS2FPGA_BRIDGE_NAME,
> > +   .remap_mask = ALT_L3_REMAP_H2F_MSK,
> > +};
> 
> Each of these data structs also includes space for all the private data
> field of the drivers' state.  Seems a bit inefficient if only two of
> them are configuration data.  It also means only one device of each type
> can exists.  If one creates two bridges of the same type they'll
> (silently) share a priv data struct and randomly break.  And the config
> data structs can't be const.

Our hardware doesn't 

Re: [PATCH v2] fpga: FPGA_MGR_ZYNQ_FPGA should depend on HAS_DMA

[atull]

On Sun, 5 Jun 2016, Geert Uytterhoeven wrote:

> If NO_DMA=y:
> 
> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
> 
> Add a dependency on HAS_DMA to fix this.
> 
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Moritz Fischer 

Acked-by: Alan Tull 

Thanks!
Alan

> ---
> v2:
>   - Add Reviewed-by,
>   - Updated error message for recent kernels,
>   - Submit to Greg, as requested by Alan.
> ---
>  drivers/fpga/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index c9b9fdf6cfbbeb6d..d61410299ec0cf80 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
>  
>  config FPGA_MGR_ZYNQ_FPGA
>   tristate "Xilinx Zynq FPGA"
> + depends on HAS_DMA
>   help
> FPGA manager driver support for Xilinx Zynq FPGAs.
>  
> -- 
> 1.9.1
> 
> 

Re: [PATCH v2] fpga: FPGA_MGR_ZYNQ_FPGA should depend on HAS_DMA

[atull]

On Sun, 5 Jun 2016, Geert Uytterhoeven wrote:

> If NO_DMA=y:
> 
> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
> 
> Add a dependency on HAS_DMA to fix this.
> 
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Moritz Fischer 

Acked-by: Alan Tull 

Thanks!
Alan

> ---
> v2:
>   - Add Reviewed-by,
>   - Updated error message for recent kernels,
>   - Submit to Greg, as requested by Alan.
> ---
>  drivers/fpga/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index c9b9fdf6cfbbeb6d..d61410299ec0cf80 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
>  
>  config FPGA_MGR_ZYNQ_FPGA
>   tristate "Xilinx Zynq FPGA"
> + depends on HAS_DMA
>   help
> FPGA manager driver support for Xilinx Zynq FPGAs.
>  
> -- 
> 1.9.1
> 
> 

Re: [PATCH v2] fpga: FPGA_MGR_ZYNQ_FPGA should depend on HAS_DMA

[atull]

On Mon, 6 Jun 2016, Moritz Fischer wrote:

> Hi Alan, Geert
> 
> On Mon, Jun 6, 2016 at 11:02 AM, atull <at...@opensource.altera.com> wrote:
> > On Sun, 5 Jun 2016, Geert Uytterhoeven wrote:
> >
> >> If NO_DMA=y:
> >>
> >> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
> >>
> >> Add a dependency on HAS_DMA to fix this.
> >>
> >> Signed-off-by: Geert Uytterhoeven <ge...@linux-m68k.org>
> >> Reviewed-by: Moritz Fischer <moritz.fisc...@ettus.com>
> >> ---
> >> v2:
> >>   - Add Reviewed-by,
> >>   - Updated error message for recent kernels,
> >>   - Submit to Greg, as requested by Alan.
> >
> > Hi Geert,
> >
> > Thank you!
> 
> We should probably pick this one over Sudip's duplicate?
> 
> Cheers,
> 
> Moritz
> 

Yes, Geert originally submitted 11/9/2015.

Alan

Re: [PATCH v2] fpga: FPGA_MGR_ZYNQ_FPGA should depend on HAS_DMA

[atull]

On Mon, 6 Jun 2016, Moritz Fischer wrote:

> Hi Alan, Geert
> 
> On Mon, Jun 6, 2016 at 11:02 AM, atull  wrote:
> > On Sun, 5 Jun 2016, Geert Uytterhoeven wrote:
> >
> >> If NO_DMA=y:
> >>
> >> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
> >>
> >> Add a dependency on HAS_DMA to fix this.
> >>
> >> Signed-off-by: Geert Uytterhoeven 
> >> Reviewed-by: Moritz Fischer 
> >> ---
> >> v2:
> >>   - Add Reviewed-by,
> >>   - Updated error message for recent kernels,
> >>   - Submit to Greg, as requested by Alan.
> >
> > Hi Geert,
> >
> > Thank you!
> 
> We should probably pick this one over Sudip's duplicate?
> 
> Cheers,
> 
> Moritz
> 

Yes, Geert originally submitted 11/9/2015.

Alan

Re: [PATCH v2] fpga: FPGA_MGR_ZYNQ_FPGA should depend on HAS_DMA

[atull]

On Sun, 5 Jun 2016, Geert Uytterhoeven wrote:

> If NO_DMA=y:
> 
> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
> 
> Add a dependency on HAS_DMA to fix this.
> 
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Moritz Fischer 
> ---
> v2:
>   - Add Reviewed-by,
>   - Updated error message for recent kernels,
>   - Submit to Greg, as requested by Alan.

Hi Geert,

Thank you!

Alan

> ---
>  drivers/fpga/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index c9b9fdf6cfbbeb6d..d61410299ec0cf80 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
>  
>  config FPGA_MGR_ZYNQ_FPGA
>   tristate "Xilinx Zynq FPGA"
> + depends on HAS_DMA
>   help
> FPGA manager driver support for Xilinx Zynq FPGAs.
>  
> -- 
> 1.9.1
> 
> 

Re: [PATCH v2] fpga: FPGA_MGR_ZYNQ_FPGA should depend on HAS_DMA

[atull]

On Sun, 5 Jun 2016, Geert Uytterhoeven wrote:

> If NO_DMA=y:
> 
> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
> 
> Add a dependency on HAS_DMA to fix this.
> 
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Moritz Fischer 
> ---
> v2:
>   - Add Reviewed-by,
>   - Updated error message for recent kernels,
>   - Submit to Greg, as requested by Alan.

Hi Geert,

Thank you!

Alan

> ---
>  drivers/fpga/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index c9b9fdf6cfbbeb6d..d61410299ec0cf80 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -21,6 +21,7 @@ config FPGA_MGR_SOCFPGA
>  
>  config FPGA_MGR_ZYNQ_FPGA
>   tristate "Xilinx Zynq FPGA"
> + depends on HAS_DMA
>   help
> FPGA manager driver support for Xilinx Zynq FPGAs.
>  
> -- 
> 1.9.1
> 
> 

Re: [PATCH 17/54] MAINTAINERS: Add file patterns for fpga device tree bindings

[atull]

On Sun, 22 May 2016, Geert Uytterhoeven wrote:

> Submitters of device tree binding documentation may forget to CC
> the subsystem maintainer if this is missing.
> 
> Signed-off-by: Geert Uytterhoeven 
> Cc: Alan Tull 
> Cc: Moritz Fischer 
> ---
> Please apply this patch directly if you want to be involved in device
> tree binding documentation for your subsystem.
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 75138c09dd603093..ef8a33c07ddceb3b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4769,6 +4769,7 @@ FPGA MANAGER FRAMEWORK
>  M:   Alan Tull 
>  R:   Moritz Fischer 
>  S:   Maintained
> +F:   Documentation/devicetree/bindings/fpga/
>  F:   drivers/fpga/

Looks good.

Alan

>  F:   include/linux/fpga/fpga-mgr.h
>  W:   http://www.rocketboards.org
> -- 
> 1.9.1
> 
> 

Re: [PATCH 17/54] MAINTAINERS: Add file patterns for fpga device tree bindings

[atull]

On Sun, 22 May 2016, Geert Uytterhoeven wrote:

> Submitters of device tree binding documentation may forget to CC
> the subsystem maintainer if this is missing.
> 
> Signed-off-by: Geert Uytterhoeven 
> Cc: Alan Tull 
> Cc: Moritz Fischer 
> ---
> Please apply this patch directly if you want to be involved in device
> tree binding documentation for your subsystem.
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 75138c09dd603093..ef8a33c07ddceb3b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4769,6 +4769,7 @@ FPGA MANAGER FRAMEWORK
>  M:   Alan Tull 
>  R:   Moritz Fischer 
>  S:   Maintained
> +F:   Documentation/devicetree/bindings/fpga/
>  F:   drivers/fpga/

Looks good.

Alan

>  F:   include/linux/fpga/fpga-mgr.h
>  W:   http://www.rocketboards.org
> -- 
> 1.9.1
> 
> 

Re: [PATCH v3] of/overlay: add of overlay notifications

[atull]

On Tue, 19 Apr 2016, Rob Herring wrote:

> On Thu, Mar 3, 2016 at 9:10 AM, Alan Tull  wrote:
> > This patch add of overlay notifications.
> >
> > When DT overlays are being added, some drivers/subsystems
> > need to see device tree overlays before the changes go into
> > the live tree.
> >
> > This is distinct from reconfig notifiers that are
> > post-apply or post-remove and which issue very granular
> > notifications without providing access to the context
> > of a whole overlay.
> >
> > The following 4 notificatons are issued:
> >   OF_OVERLAY_PRE_APPLY
> >   OF_OVERLAY_POST_APPLY
> >   OF_OVERLAY_PRE_REMOVE
> >   OF_OVERLAY_POST_REMOVE
> >
> > In the case of pre-apply notification, if the notifier
> > returns error, the overlay will be rejected.
> >
> > This patch exports two functions for registering/unregistering
> > notifications:
> >   of_overlay_notifier_register(struct notifier_block *nb)
> >   of_overlay_notifier_unregister(struct notifier_block *nb)
> >
> > The of_mutex is held during these notifications. The
> > notification data includes pointers to the overlay target
> > and the overlay:
> >
> > struct of_overlay_notify_data {
> >struct device_node *overlay;
> >struct device_node *target;
> > };
> >
> > Signed-off-by: Alan Tull 
> > ---
> > v2: add missing 'static inline' in of.h
> > v3: fix build for !OF_OVERLAY in of.h
> > add a note in the header that the of_mutex is held
> > ---
> >  drivers/of/overlay.c |   47 ++-
> >  include/linux/of.h   |   25 +
> >  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> This looks fine to me, but apply it when you have a user.
> 
> Acked-by: Rob Herring 
> 
> Rob

Thanks!

I think I'll be the first user.

Alan

> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3] of/overlay: add of overlay notifications

[atull]

On Tue, 19 Apr 2016, Rob Herring wrote:

> On Thu, Mar 3, 2016 at 9:10 AM, Alan Tull  wrote:
> > This patch add of overlay notifications.
> >
> > When DT overlays are being added, some drivers/subsystems
> > need to see device tree overlays before the changes go into
> > the live tree.
> >
> > This is distinct from reconfig notifiers that are
> > post-apply or post-remove and which issue very granular
> > notifications without providing access to the context
> > of a whole overlay.
> >
> > The following 4 notificatons are issued:
> >   OF_OVERLAY_PRE_APPLY
> >   OF_OVERLAY_POST_APPLY
> >   OF_OVERLAY_PRE_REMOVE
> >   OF_OVERLAY_POST_REMOVE
> >
> > In the case of pre-apply notification, if the notifier
> > returns error, the overlay will be rejected.
> >
> > This patch exports two functions for registering/unregistering
> > notifications:
> >   of_overlay_notifier_register(struct notifier_block *nb)
> >   of_overlay_notifier_unregister(struct notifier_block *nb)
> >
> > The of_mutex is held during these notifications. The
> > notification data includes pointers to the overlay target
> > and the overlay:
> >
> > struct of_overlay_notify_data {
> >struct device_node *overlay;
> >struct device_node *target;
> > };
> >
> > Signed-off-by: Alan Tull 
> > ---
> > v2: add missing 'static inline' in of.h
> > v3: fix build for !OF_OVERLAY in of.h
> > add a note in the header that the of_mutex is held
> > ---
> >  drivers/of/overlay.c |   47 ++-
> >  include/linux/of.h   |   25 +
> >  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> This looks fine to me, but apply it when you have a user.
> 
> Acked-by: Rob Herring 
> 
> Rob

Thanks!

I think I'll be the first user.

Alan

> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>