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

[Andrea Galbusera]

On Fri, Jun 10, 2016 at 4:18 AM, Trent Piepho <tpiepho@xxxxxxxxxxxxxx> wrote:
> On Fri, 2016-02-05 at 15:30 -0600, atull@xxxxxxxxxxxxxxxxxxxxx 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 0000000..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 <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +/*
>> + * 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 <linux/clk.h>
>> +#include <linux/fpga/fpga-bridge.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#define ALT_L3_REMAP_OFST                    0x0
>> +#define ALT_L3_REMAP_MPUZERO_MSK             0x00000001
>> +#define ALT_L3_REMAP_H2F_MSK                 0x00000008
>> +#define ALT_L3_REMAP_LWH2F_MSK                       0x00000010
>> +
>> +#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()...