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

[Trent Piepho]

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.

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 altera-o

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

[Andrea Galbusera]

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 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()...

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

[Trent Piepho]

On Mon, 2016-06-13 at 14:35 -0500, atull wrote:
> > > +
> > > + /* 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.

From my testing, it will work ok if the syscon driver were to set
syscon_config.cache_type one of the caches.  Since the l3 regs read back
as 0, rather than not being readable at all, making them write-only and
giving a default isn't strictly necessary.

It wouldn't be hard to add a write-only property to 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.

You'll need a spin-lock to protect against concurrent access.


> > > +
> > > +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 contain two devices of any of these three types.

Not yet, but doesn't Arria10 have three FPGA2SDRAM bridges?  But really,
it's just that I think having each device allocated state data for just
that device is more in line with the Linux device model than shared
static state pre-allocated for all devices of the same type to share.
 
> > What if these structs were a different altera_hps_config struct, which
> > the private data struct could then copy or point to?
> > 
> > struct altera_hpsbridge_config {
> > const char *name;
> > uint32_t remap_mask;
> > };
> > 
> > struct altera_hpsbridge_data {
> > const struct altera_hpsbridge_config *config;
> > ...;
> > struct clk *clk;
> > };
> 
> Yes, that sounds good and sensible.  I'll do that in v18.

Another thought, if the "altr,l3regs" binding included not just the
phandle to the l3regs device, but also the bit associated with the
bridge, then there wouldn't need to be bridge specific configuration for
the driver.  The same way the reset and clock properties point not just
to the reset and clock controller, but to the bit in the controller.

> > > +
> > > +static struct altera_hps2fpga_data lwhps2fpga_data  = {
> > > + .name = LWHPS2FPGA_BRIDGE_NAME,
> > > + .remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> > > +};
> > > +
> > > +static struct altera_hps2fpga_data fpga2hps_data  = {
> > > + .name = FPGA2HPS_BRIDGE_NAME,
> > > +};
> > > +
> > > +static const struct of_device_id altera_fpga_of_match[] = {
> > > + { .compatible = "altr,socfpga-hps2fpga-bridge",
> > > +   .data = &hps2fpga_data },
> > > + { .compatible = "altr,socfpga-lwhps2fpga-bridge",
> >

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 conta

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

[Trent Piepho]

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.

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.
Whic

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

[atull]

From: Alan Tull 

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
---
 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 c419d4b..45fd823 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -38,6 +38,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 chip ram before Linux is started and the configuration
+ * is passed in a handoff register in the system manager.
+ *
+ * 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 
+
+#define ALT_SDR_CTL_FPGAPORTRST_OFST   0x80
+#define ALT_SDR_CTL_FPGAPORTRST_PORTRSTN_MSK   0x3fff
+#define ALT_SDR_CTL_FPGAPORTRST_RD_SHIFT   0
+#define ALT_SDR_CTL_FPGAPOR