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

Andrea Galbusera
2016-07-28T10:28:30Z

On Fri, Jun 10, 2016 at 4:18 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Fri, 2016-02-05 at 15:30 -0600, atull@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 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()...