| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.DEB.2.10.1510280645200.2865@atull-730U3E-740U3E>
Date: Wed, 28 Oct 2015 06:51:49 -0600
From: atull <atull@...nsource.altera.com>
To: Steffen Trumtrar <s.trumtrar@...gutronix.de>
CC: <gregkh@...uxfoundation.org>,
Moritz Fischer <moritz.fischer@...us.com>,
Josh Cartwright <joshc@...com>, <monstr@...str.eu>,
<michal.simek@...inx.com>, Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
"Jonathan Corbet" <corbet@....net>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<pantelis.antoniou@...sulko.com>, <delicious.quinoa@...il.com>,
<dinguyen@...nsource.altera.com>,
Matthew Gerlach <mgerlach@...era.com>
Subject: Re: [PATCH v12 6/6] ARM: socfpga: fpga bridge driver support
On Wed, 28 Oct 2015, Steffen Trumtrar wrote:
> On Tue, Oct 27, 2015 at 05:09:15PM -0500, atull@...nsource.altera.com wrote:
> > From: Alan Tull <atull@...nsource.altera.com>
> >
> > 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 <dinguyen@...nsource.altera.com>
> > Signed-off-by: Dinh Nguyen <dinguyen@...nsource.altera.com>
> > Signed-off-by: Matthew Gerlach <mgerlach@...era.com>
> > ---
> > 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
> > ---
> > drivers/fpga/Kconfig | 7 ++
> > drivers/fpga/Makefile | 1 +
> > drivers/fpga/altera-fpga2sdram.c | 185 ++++++++++++++++++++++++++++++
> > drivers/fpga/altera-hps2fpga.c | 234 ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 427 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 143072b..004b83a 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 9302662..cc01db3 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.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_SIMPLE_FPGA_BUS) += simple-fpga-bus.o
> > diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
> > new file mode 100644
> > index 0000000..ee56903
> > --- /dev/null
> > +++ b/drivers/fpga/altera-fpga2sdram.c
> > @@ -0,0 +1,185 @@
> > +/*
> > + * 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 <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/*
> > + * 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 <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>
> > +
> > +#define ALT_SDR_CTL_FPGAPORTRST_OFST 0x80
> > +#define ALT_SDR_CTL_FPGAPORTRST_PORTRSTN_MSK 0x00003fff
> > +#define ALT_SDR_CTL_FPGAPORTRST_RD_SHIFT 0
> > +#define ALT_SDR_CTL_FPGAPORTRST_WR_SHIFT 4
> > +#define ALT_SDR_CTL_FPGAPORTRST_CTRL_SHIFT 8
> > +
> > +#define SYSMGR_ISWGRP_HANDOFF3 (0x8C)
> > +#define ISWGRP_HANDOFF_FPGA2SDR SYSMGR_ISWGRP_HANDOFF3
> > +
> > +#define F2S_BRIDGE_NAME "fpga2sdram"
> > +
> > +struct alt_fpga2sdram_data {
> > + struct device *dev;
> > + struct regmap *sdrctl;
> > + int mask;
> > +};
> > +
> > +static int alt_fpga2sdram_enable_show(struct fpga_bridge *bridge)
> > +{
> > + struct alt_fpga2sdram_data *priv = bridge->priv;
> > + int value;
> > +
> > + regmap_read(priv->sdrctl, ALT_SDR_CTL_FPGAPORTRST_OFST, &value);
> > +
> > + return (value & priv->mask) == priv->mask;
> > +}
> > +
> > +static inline int _alt_fpga2sdram_enable_set(struct alt_fpga2sdram_data *priv,
> > + bool enable)
> > +{
> > + return regmap_update_bits(priv->sdrctl, ALT_SDR_CTL_FPGAPORTRST_OFST,
> > + priv->mask, enable ? priv->mask : 0);
> > +}
> > +
> > +static int alt_fpga2sdram_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > + return _alt_fpga2sdram_enable_set(bridge->priv, enable);
> > +}
> > +
> > +struct prop_map {
> > + char *prop_name;
> > + uint32_t *prop_value;
> > + uint32_t prop_max;
> > +};
> > +
> > +struct fpga_bridge_ops altera_fpga2sdram_br_ops = {
> > + .enable_set = alt_fpga2sdram_enable_set,
> > + .enable_show = alt_fpga2sdram_enable_show,
> > +};
> > +
> > +static const struct of_device_id altera_fpga_of_match[] = {
> > + { .compatible = "altr,socfpga-fpga2sdram-bridge" },
> > + {},
> > +};
> > +
> > +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct alt_fpga2sdram_data *priv;
> > + uint32_t init_val;
> > + struct regmap *sysmgr;
> > + int ret = 0;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->dev = dev;
> > +
> > + priv->sdrctl = syscon_regmap_lookup_by_compatible("altr,sdr-ctl");
> > + if (IS_ERR(priv->sdrctl)) {
> > + dev_err(dev, "regmap for altr,sdr-ctl lookup failed.\n");
> > + return PTR_ERR(priv->sdrctl);
> > + }
> > +
> > + sysmgr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> > + if (IS_ERR(priv->sdrctl)) {
> > + dev_err(dev, "regmap for altr,sys-mgr lookup failed.\n");
> > + return PTR_ERR(sysmgr);
> > + }
> > +
> > + /* Get f2s bridge configuration saved in handoff register */
> > + regmap_read(sysmgr, SYSMGR_ISWGRP_HANDOFF3, &priv->mask);
> > +
> > + ret = fpga_bridge_register(dev, F2S_BRIDGE_NAME,
> > + &altera_fpga2sdram_br_ops, priv);
> > + if (ret)
> > + return ret;
> > +
> > + dev_info(dev, "driver initialized with handoff %08x\n", priv->mask);
>
> Hm, what does this do? All I can get from the documentation is, that this
> is a general purpose register and has nothing to do with this particular bridge.
>
It's described at the top of the file. This bridge has to be configured before
the kernel boots, and this is a way of passing that configuration.
> > +
> > + if (!of_property_read_u32(dev->of_node, "init-val", &init_val)) {
> > + if (init_val > 1) {
> > + dev_warn(dev, "invalid init-val %u > 1\n", init_val);
> > + } else {
> > + dev_info(dev, "%s bridge\n",
> > + (init_val ? "enabling" : "disabling"));
> > + ret = _alt_fpga2sdram_enable_set(priv, init_val);
> > + if (ret) {
> > + fpga_bridge_unregister(&pdev->dev);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int alt_fpga_bridge_remove(struct platform_device *pdev)
> > +{
> > + fpga_bridge_unregister(&pdev->dev);
> > +
> > + return 0;
> > +}
> > +
> > +MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
> > +
> > +static struct platform_driver altera_fpga_driver = {
> > + .remove = alt_fpga_bridge_remove,
> > + .driver = {
> > + .name = "altera_fpga2sdram_bridge",
> > + .of_match_table = of_match_ptr(altera_fpga_of_match),
> > + },
> > +};
> > +
> > +static int __init alt_fpga_bridge_init(void)
> > +{
> > + return platform_driver_probe(&altera_fpga_driver,
> > + alt_fpga_bridge_probe);
> > +}
> > +
> > +static void __exit alt_fpga_bridge_exit(void)
> > +{
> > + platform_driver_unregister(&altera_fpga_driver);
> > +}
> > +
> > +module_init(alt_fpga_bridge_init);
> > +module_exit(alt_fpga_bridge_exit);
> > +
> > +MODULE_DESCRIPTION("Altera SoCFPGA FPGA to SDRAM Bridge");
> > +MODULE_AUTHOR("Alan Tull <atull@...nsource.altera.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
> > new file mode 100644
> > index 0000000..3ec6394
> > --- /dev/null
> > +++ b/drivers/fpga/altera-hps2fpga.c
> > @@ -0,0 +1,234 @@
> > +/*
> > + * 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 SOCFPGA_RSTMGR_BRGMODRST 0x1c
> > +#define ALT_RSTMGR_BRGMODRST_H2F_MSK 0x00000001
> > +#define ALT_RSTMGR_BRGMODRST_LWH2F_MSK 0x00000002
> > +#define ALT_RSTMGR_BRGMODRST_F2H_MSK 0x00000004
> > +
> > +#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 {
> This name and the alt_hps2fpga_xxx functions are confusing, as you have
> one bridge of this name but use it for all the bridges.
I think it's OK.
>
> > + 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 reset_mask;
> > + 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;
>
> Why do you remap the On-chip RAM here. This doesn't belong in this
> driver.
We have to when we enable or disable.
>
> > +
> > + 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);
>
> regmap_update_bits? Otherwise you will overwrite the other bridges
> settings.
This works. This is a write-only register.
>
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > + return _alt_hps2fpga_enable_set(bridge->priv, enable);
> > +}
> > +
> > +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,
> > + .reset_mask = ALT_RSTMGR_BRGMODRST_H2F_MSK,
> > + .remap_mask = ALT_L3_REMAP_H2F_MSK,
> > +};
> > +
> > +static struct altera_hps2fpga_data lwhps2fpga_data = {
> > + .name = LWHPS2FPGA_BRIDGE_NAME,
> > + .reset_mask = ALT_RSTMGR_BRGMODRST_LWH2F_MSK,
> > + .remap_mask = ALT_L3_REMAP_LWH2F_MSK,
> > +};
> > +
> > +static struct altera_hps2fpga_data fpga2hps_data = {
> > + .name = FPGA2HPS_BRIDGE_NAME,
> > + .reset_mask = ALT_RSTMGR_BRGMODRST_F2H_MSK,
> > +};
> > +
> > +static const struct of_device_id altera_fpga_of_match[] = {
> > + { .compatible = "altr,socfpga-hps2fpga-bridge",
> > + .data = &hps2fpga_data },
> > + { .compatible = "altr,socfpga-lwhps2fpga-bridge",
> > + .data = &lwhps2fpga_data },
> > + { .compatible = "altr,socfpga-fpga2hps-bridge",
> > + .data = &fpga2hps_data },
> > + {},
> > +};
> > +
> > +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct altera_hps2fpga_data *priv;
> > + const struct of_device_id *of_id;
> > + uint32_t init_val;
> > + int ret;
> > +
> > + of_id = of_match_device(altera_fpga_of_match, dev);
> > + priv = (struct altera_hps2fpga_data *)of_id->data;
> > + WARN_ON(!priv);
>
> Why don't you just return here if you have a NULL pointer?
Actually we don't need that WARN_ON. I can take it out.
>
> > +
> > + priv->bridge_reset = devm_reset_control_get(dev, priv->name);
> > + if (IS_ERR(priv->bridge_reset)) {
> > + dev_err(dev, "Could not get %s reset control!\n", priv->name);
> > + return PTR_ERR(priv->bridge_reset);
> > + }
> > +
> > + priv->l3reg = syscon_regmap_lookup_by_compatible("altr,l3regs");
> > + if (IS_ERR(priv->l3reg)) {
> > + dev_err(dev, "regmap for altr,l3regs lookup failed.\n");
> > + return PTR_ERR(priv->l3reg);
> > + }
> > +
> > + priv->clk = of_clk_get(dev->of_node, 0);
> > + 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");
> > + return -EBUSY;
> > + }
> > +
> > + ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
> > + priv);
> > + if (ret)
> > + return ret;
> > +
> > + if (!of_property_read_u32(dev->of_node, "init-val", &init_val)) {
> > + if (init_val > 1) {
> > + dev_warn(dev, "invalid init-val %u > 1\n", init_val);
> > + } else {
> > + dev_info(dev, "%s bridge\n",
> > + (init_val ? "enabling" : "disabling"));
> > +
> > + ret = _alt_hps2fpga_enable_set(priv, init_val);
> > + if (ret) {
> > + fpga_bridge_unregister(&pdev->dev);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int alt_fpga_bridge_remove(struct platform_device *pdev)
> > +{
> > + struct fpga_bridge *bridge = platform_get_drvdata(pdev);
> > + struct altera_hps2fpga_data *priv = bridge->priv;
> > +
> > + fpga_bridge_unregister(&pdev->dev);
> > +
> > + clk_disable_unprepare(priv->clk);
> > + clk_put(priv->clk);
> > +
> > + return 0;
> > +}
> > +
> > +MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
> > +
> > +static struct platform_driver altera_fpga_driver = {
> > + .remove = alt_fpga_bridge_remove,
> > + .driver = {
> > + .name = "altera_hps2fpga_bridge",
> > + .of_match_table = of_match_ptr(altera_fpga_of_match),
> > + },
> > +};
> > +
> > +static int __init alt_fpga_bridge_init(void)
> > +{
> > + return platform_driver_probe(&altera_fpga_driver,
> > + alt_fpga_bridge_probe);
> > +}
> > +
> > +static void __exit alt_fpga_bridge_exit(void)
> > +{
> > + platform_driver_unregister(&altera_fpga_driver);
> > +}
> > +
> > +module_init(alt_fpga_bridge_init);
> > +module_exit(alt_fpga_bridge_exit);
> > +
> > +MODULE_DESCRIPTION("Altera SoCFPGA HPS to FPGA Bridge");
> > +MODULE_AUTHOR("Alan Tull <atull@...nsource.altera.com>");
> > +MODULE_LICENSE("GPL v2");
>
> Regards,
> Steffen
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists