LinuxLists.cc - [PATCH v2 0/3] fpga bridge framework

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi!

On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
> From: Alan Tull <[email protected]>

(...)

> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> new file mode 100644
> index 0000000..bc24a2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> @@ -0,0 +1,53 @@
> +Altera FPGA/HPS Bridge Driver
> +
> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> +User space can enable or disable the bridge by writing a "1" or a "0",
> +respectively, to its enable file under bridge's entry in
> +/sys/class/fpga-bridge. Typically, one disables the bridges before
> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> +reenabled.
> +

NAK.

This is all linux specific and doesn't belong here.

> +Required properties:
> +
> + - compatible : should contain one of:
> + "altr,socfpga-hps2fpga-bridge"
> + "altr,socfpga-lwhps2fpga-bridge"
> + "altr,socfpga-fpga2hps-bridge"
> +
> + - clocks : clocks used by this module
> +
> + - altr,l3-syscon : phandle of the l3 interconnect module
> +

L3 shouldn't be a syscon. Have you tried dumping the regmap in the debugfs if L3
is a syscon? Doesn't work.

> +Optional properties:
> + - label : name that you want this bridge to show up as under
> + /sys/class/fpga-bridge. Default is br<device#> if this is
> + not specified.
> +

Why? Linux-specific.

> + - init-val : 0 if driver should disable bridge at startup
> + 1 if driver should enable bridge at startup
> + driver leaves bridge in current state if property not
> + specified.
> +

Configuration in the DT? Really?

> +Example:
> + hps_fpgabridge0: fpgabridge@0 {
> + compatible = "altr,socfpga-hps2fpga-bridge";
> + label = "hps2fpga";
> + altr,l3-syscon = <&l3regs>;
> + clocks = <&l4_main_clk>;
> + init-val = <1>;
> + };
> +
> + hps_fpgabridge1: fpgabridge@1 {
> + compatible = "altr,socfpga-lwhps2fpga-bridge";
> + label = "lwhps2fpga";
> + altr,l3-syscon = <&l3regs>;
> + clocks = <&l4_main_clk>;
> + init-val = <0>;
> + };
> +
> + hps_fpgabridge2: fpgabridge@2 {
> + compatible = "altr,socfpga-fpga2hps-bridge";
> + label = "fpga2hps";
> + altr,l3-syscon = <&l3regs>;
> + clocks = <&l4_main_clk>;
> + };

The bridges are the buses into the FPGA. This has to be accomodated.
The bridges have two specified memory ranges: one the address space
of the bus, the second the register space for configuration.

This binding does NOT correctly describe the hardware. Sorry.

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 |

2014-10-24 09:21:06

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Stefan, Allan,

Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
Just wanted to pop in and comment on a few things.

> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <[email protected]> wrote:
>
> Hi!
>
> On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
>> From: Alan Tull <[email protected]>
>
> (...)
>
>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>> new file mode 100644
>> index 0000000..bc24a2e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>> @@ -0,0 +1,53 @@
>> +Altera FPGA/HPS Bridge Driver
>> +
>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
>> +User space can enable or disable the bridge by writing a "1" or a "0",
>> +respectively, to its enable file under bridge's entry in
>> +/sys/class/fpga-bridge. Typically, one disables the bridges before
>> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
>> +reenabled.
>> +
>
> NAK.
>
> This is all linux specific and doesn't belong here.
>

We know. The DT spec hasn’t been updated for a while, and this is going to be
part of what we want to do with the restarting of the ePAPR spec process.

So absolutes like “DT is a hardware description only" might be too strong statements, that
do not work in practice anymore.

There’s no such thing as pure hardware devices lately - there is always a configuration
component; some of it OS specific, some of it not.

We have to be engaged in the process and figure out how to update the spec for what is
now the state of the art in the industry.

Regards

— Pantelis

2014-10-24 10:54:25

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] add sysfs document for fpga bridges

On Thu 2014-10-23 18:51:05, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Add sysfs document for fpga bridges.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-class-fpga-bridge | 5 +++++
> 1 file changed, 5 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
>
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-bridge b/Documentation/ABI/testing/sysfs-class-fpga-bridge
> new file mode 100644
> index 0000000..b3303ae
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-bridge
> @@ -0,0 +1,5 @@
> +What: /sys/class/fpga-bridge/<bridge>/enable
> +Date: October 2014
> +KernelVersion: 3.18
> +Contact: Alan Tull <[email protected]>
> +Description: Enable bridge. Write 1 to bring bridge out of reset.

Specify, what happens when 0 is written there?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2014-10-24 10:57:56

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Thu 2014-10-23 18:51:06, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Add DTS binding documentation for the Altera FPGA bridges.
>
> Signed-off-by: Alan Tull <[email protected]>

Acked-by: Pavel Machek <[email protected]>

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2014-10-24 15:18:18

by atull

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] add sysfs document for fpga bridges

On Fri, 24 Oct 2014, Pavel Machek wrote:

> On Thu 2014-10-23 18:51:05, [email protected] wrote:
> > From: Alan Tull <[email protected]>
> >
> > Add sysfs document for fpga bridges.
> >
> > Signed-off-by: Alan Tull <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-class-fpga-bridge | 5 +++++
> > 1 file changed, 5 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-fpga-bridge b/Documentation/ABI/testing/sysfs-class-fpga-bridge
> > new file mode 100644
> > index 0000000..b3303ae
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-fpga-bridge
> > @@ -0,0 +1,5 @@
> > +What: /sys/class/fpga-bridge/<bridge>/enable
> > +Date: October 2014
> > +KernelVersion: 3.18
> > +Contact: Alan Tull <[email protected]>
> > +Description: Enable bridge. Write 1 to bring bridge out of reset.
>
> Specify, what happens when 0 is written there?
> Pavel

Yes, I will add: 'Write 0 has no effect.'

Alan

> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

2014-10-25 19:20:30

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Pantelis!

On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
> Hi Stefan, Allan,
>
> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
> Just wanted to pop in and comment on a few things.
>
> > On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <[email protected]> wrote:
> >
> > Hi!
> >
> > On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
> >> From: Alan Tull <[email protected]>
> >
> > (...)
> >
> >> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >> new file mode 100644
> >> index 0000000..bc24a2e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >> @@ -0,0 +1,53 @@
> >> +Altera FPGA/HPS Bridge Driver
> >> +
> >> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> >> +User space can enable or disable the bridge by writing a "1" or a "0",
> >> +respectively, to its enable file under bridge's entry in
> >> +/sys/class/fpga-bridge. Typically, one disables the bridges before
> >> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> >> +reenabled.
> >> +
> >
> > NAK.
> >
> > This is all linux specific and doesn't belong here.
> >
>
> We know. The DT spec hasn’t been updated for a while, and this is going to be
> part of what we want to do with the restarting of the ePAPR spec process.
>

As we don't have a new spec yet, I go with the current state of the art.
And I don't see how "file under ... /sys/class/fpga-bridge" fits the
current spec.

> So absolutes like “DT is a hardware description only" might be too strong statements, that
> do not work in practice anymore.
>
> There’s no such thing as pure hardware devices lately - there is always a configuration
> component; some of it OS specific, some of it not.
>

If it really is necessary, I'm not against it, don't get me wrong.
But in the grand scheme I wouldn't say that this fits my experience.
There are some devices that need configuration and often when it is
done in the DT, it is just a shortcut or not thought through.
And can be also introspected dynamically. With some discussion on the
list these bindings often change.

Regarding OS specifics: already there, e.g. console setup in the chosen node.
And other things I saw are ethernet aliases just for u-boot. Not a problem
of the spec, just a problem of u-boot and unneccessary "configuration".
Just to fix a bad bootloader. barebox can handle this without any
such stuff. Maybe we should integrate the DT "overlays" barebox uses into
the in-kernel DTs as well...but does it really help/interest someone who
decides to use u-boot where barebox stores its environment? I guess not.

> We have to be engaged in the process and figure out how to update the spec for what is
> now the state of the art in the industry.
>

Again, not against that if it is necessary. For example your overlay stuff might
be a good update to the spec. Would be better if it is official instead of a "hack".
I want that for SoCFPGA.

But having looked at one or two vendor kernels+DTs, the state of the art in the
industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
ePAPR necessary ;-))

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 |

2014-10-27 11:37:26

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] fpga bridge driver

Hi Alan,

> On Oct 24, 2014, at 02:51 , [email protected] wrote:
>
> From: Alan Tull <[email protected]>
>
> Support for bringing bridges out of reset. Bridges show up in sysfs
> under /sys/class/fpga-bridge and can be enabled/disabled from sysfs
> or from the device tree.
>
> Supports enabling the following hps/fpga bridges:
> * fpga2sdram
> * fpga2hps
> * hps2fpga
> * lwhps2fpga
>
> Control from sysfs:
> Enable:
> $ echo 1 > /sys/class/fpga-bridge/fpga2hps/enable
>
> Disable:
> $ echo 0 > /sys/class/fpga-bridge/fpga2hps/enable
>
> Check enable/disable status (checks for all bits set):
> $ cat /sys/class/fpga-bridge/fpga2hps/enable
> (will print '0' or '1’)
>

Timing of reset? What if someone issues a disable/enable sequence
back to back?

Perhaps you need an explicit reset property, or you need to specify
that the expected behavior is for enable to settle before returning
to user-space.

> Device tree has an optional property 'init-val'. This property
> configures the driver to enable or disable the bridge when instantiated.
> If the property does not exist, the driver will leave the bridge in
> its current state.
>
> Notes:
>
> The L3 remap register is write-only (!) and reads zeros. So
> doing a 'read, modify, write' operation on that register will
> clear the mpuzero bit that was set by u-boot. So we keep
> a cached copy of what we write to it.
>
> The read, write, and command ports on the fpga2sdram bridge can
> only be reconfigured if there are no transactions to the sdram
> during the reconfiguration. Currently, this guarantee can only
> be made by code running out of onchip ram before Linux is started.
> Therefore, this driver only supports enabling and disabling of the
> ports that have already been configured. Since the driver cannot
> determine the port configuration in all cases, the device tree
> must be populated with masks for all of the ports.
>

If you arrange for the the code layout to be in a single span of
memory (and be relocatable) you might be able to support it while
Linux runs (under special conditions). In fact the sequence seems
quite similar to the way suspend/resume works.

> The fpga bridge framework needs be started before the individual
> bridge drivers, and the individual bridge drivers need to be
> started before any other driver for components on the FPGA.
> The earliest the bridge drivers successfully start is during
> arch_init().
>
> Signed-off-by: Alan Tull <[email protected]>
> Signed-off-by: Matthew Gerlach <[email protected]>
> Signed-off-by: Dinh Nguyen <[email protected]>
> ---
> drivers/misc/Kconfig | 3 +-
> drivers/misc/Makefile | 1 +
> drivers/misc/fpga-bridge/Kconfig | 20 +++
> drivers/misc/fpga-bridge/Makefile | 6 +
> drivers/misc/fpga-bridge/altera-fpga2sdram.c | 236 ++++++++++++++++++++++++++
> drivers/misc/fpga-bridge/altera-hps2fpga.c | 220 ++++++++++++++++++++++++
> drivers/misc/fpga-bridge/fpga-bridge.c | 230 +++++++++++++++++++++++++
> drivers/misc/fpga-bridge/fpga-bridge.h | 51 ++++++
> 8 files changed, 766 insertions(+), 1 deletion(-)
> create mode 100644 drivers/misc/fpga-bridge/Kconfig
> create mode 100644 drivers/misc/fpga-bridge/Makefile
> create mode 100644 drivers/misc/fpga-bridge/altera-fpga2sdram.c
> create mode 100644 drivers/misc/fpga-bridge/altera-hps2fpga.c
> create mode 100644 drivers/misc/fpga-bridge/fpga-bridge.c
> create mode 100644 drivers/misc/fpga-bridge/fpga-bridge.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index bbeb451..6792a03 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -527,5 +527,6 @@ source "drivers/misc/vmw_vmci/Kconfig"
> source "drivers/misc/mic/Kconfig"
> source "drivers/misc/genwqe/Kconfig"
> source "drivers/misc/echo/Kconfig"
> -source "drivers/misc/cxl/Kconfig"
> +source "drivers/misc/fpga-bridge/Kconfig"
> +
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7d5c4cd..83dda02 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_C2PORT) += c2port/
> obj-$(CONFIG_HMC6352) += hmc6352.o
> obj-y += eeprom/
> obj-y += cb710/
> +obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge/
> obj-$(CONFIG_SPEAR13XX_PCIE_GADGET) += spear13xx_pcie_gadget.o
> obj-$(CONFIG_VMWARE_BALLOON) += vmw_balloon.o
> obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o
> diff --git a/drivers/misc/fpga-bridge/Kconfig b/drivers/misc/fpga-bridge/Kconfig
> new file mode 100644
> index 0000000..725474e
> --- /dev/null
> +++ b/drivers/misc/fpga-bridge/Kconfig
> @@ -0,0 +1,20 @@
> +#
> +# FPGA bridge manager configuration
> +#
> +
> +menu "FPGA Bridges"
> +
> +config FPGA_BRIDGE
> + tristate "FPGA Bridge Drivers"
> + depends on OF
> + help
> + Say Y here if you want to support bridges connected between host
> + processors and FPGAs or between FPGAs.
> +
> +config ALTERA_SOCFPGA_BRIDGE
> + tristate "Altera SoCFPGA Bridges"
> + depends on FPGA_BRIDGE
> + help
> + Say Y to enable drivers for FPGA bridges for Altera socfpga
> + devices.
> +endmenu
> diff --git a/drivers/misc/fpga-bridge/Makefile b/drivers/misc/fpga-bridge/Makefile
> new file mode 100644
> index 0000000..3700f8a
> --- /dev/null
> +++ b/drivers/misc/fpga-bridge/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for the fpga-bridge drivers
> +#
> +
> +obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o
> +obj-$(CONFIG_ALTERA_SOCFPGA_BRIDGE) += altera-fpga2sdram.o altera-hps2fpga.o
> \ No newline at end of file
> diff --git a/drivers/misc/fpga-bridge/altera-fpga2sdram.c b/drivers/misc/fpga-bridge/altera-fpga2sdram.c
> new file mode 100644
> index 0000000..9e5765e
> --- /dev/null
> +++ b/drivers/misc/fpga-bridge/altera-fpga2sdram.c
> @@ -0,0 +1,236 @@
> +/*
> + * FPGA to sdram Bridge Driver for Altera SoCFPGA Devices
> + *
> + * Copyright (C) 2013 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/>.
> + */
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/regmap.h>
> +#include "fpga-bridge.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 FOUR_BIT_MASK 0xf
> +#define SIX_BIT_MASK 0x3f
> +
> +static struct of_device_id altera_fpga_of_match[];
> +
> +struct alt_fpga2sdram_data {
> + char name[48];
> + struct platform_device *pdev;
> + struct device_node *np;
> + struct regmap *sdrctl;
> + int mask;
> +};
> +
> +static atomic_t instances;
> +
> +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);

You don’t need the enclosing parentheses; value == foo; works.
> +}
> +
> +static inline void _alt_fpga2sdram_enable_set(struct alt_fpga2sdram_data *priv,
> + bool enable)
> +{
> + int value;
> +
> + if (enable)
> + value = priv->mask;
> + else
> + value = 0;
> +
> + regmap_update_bits(priv->sdrctl, ALT_SDR_CTL_FPGAPORTRST_OFST,
> + priv->mask, value);
> +}
> +
> +static void alt_fpga2sdram_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> + _alt_fpga2sdram_enable_set(bridge->priv, enable);
> +}
> +
> +struct prop_map {
> + char *prop_name;
> + uint32_t *prop_value;
> + uint32_t prop_max;
> +};
> +
> +static int alt_fpga2sdram_get_mask(struct alt_fpga2sdram_data *priv)
> +{
> + int i;
> + uint32_t read, write, cmd;
> + struct prop_map map[] = {
> + {"read-ports-mask", &read, FOUR_BIT_MASK},
> + {"write-ports-mask", &write, FOUR_BIT_MASK},
> + {"cmd-ports-mask", &cmd, SIX_BIT_MASK},
> + };

> +
> + for (i = 0; i < ARRAY_SIZE(map); i++) {
> + if (of_property_read_u32(priv->np, map[i].prop_name,
> + map[i].prop_value)) {
> + dev_err(&priv->pdev->dev,
> + "failed to find property, %s\n",
> + map[i].prop_name);
> + return -EINVAL;
> + } else if (*map[i].prop_value > map[i].prop_max) {
> + dev_err(&priv->pdev->dev,
> + "%s value 0x%x > than max 0x%x\n",
> + map[i].prop_name,
> + *map[i].prop_value,
> + map[i].prop_max);
> + return -EINVAL;
> + }

This is too long winded. Direct coding with a sequence of three property reads
is more compact and readable.

> + }
> +
> + priv->mask =
> + (read << ALT_SDR_CTL_FPGAPORTRST_RD_SHIFT) |
> + (write << ALT_SDR_CTL_FPGAPORTRST_WR_SHIFT) |
> + (cmd << ALT_SDR_CTL_FPGAPORTRST_CTRL_SHIFT);
> +
> + return 0;
> +}
> +
> +struct fpga_bridge_ops altera_fpga2sdram_br_ops = {
> + .enable_set = alt_fpga2sdram_enable_set,
> + .enable_show = alt_fpga2sdram_enable_show,
> +};
> +
> +static struct alt_fpga2sdram_data fpga2sdram_data = {
> + .name = "fpga2sdram",
> +};
> +
> +static int alt_fpga_bridge_probe(struct platform_device *pdev)
> +{
> + struct alt_fpga2sdram_data *priv;
> + struct alt_fpga2sdram_data *data;
> + uint32_t init_val;
> + const struct of_device_id *of_id = of_match_device(altera_fpga_of_match,
> + &pdev->dev);
> + int ret = 0;
> +
> + if (atomic_inc_return(&instances) > 1) {
> + atomic_dec(&instances);
> + dev_err(&pdev->dev,
> + "already one instance of driver\n");
> + return -ENODEV;
> + }
> +
> + data = (struct alt_fpga2sdram_data *)of_id->data;
> + WARN_ON(!data);
> +

Do you really need to WARN_ON() here? You’re going to crash either way
a few lines below accessing members of data.
if (!data) return -EINVAL;
And it’s better to put it before the atomic_inc_return.

> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->np = pdev->dev.of_node;
> + priv->pdev = pdev;
> + priv->mask = data->mask;
> + strncpy(priv->name, data->name, ARRAY_SIZE(priv->name));
> +
> + priv->sdrctl = syscon_regmap_lookup_by_phandle(priv->np,
> + "altr,sdr-syscon");
> + if (IS_ERR(priv->sdrctl)) {
> + devm_kfree(&pdev->dev, priv);

No need to devm_kfree.

> + dev_err(&pdev->dev,
> + "regmap for altr,sdr-syscon lookup failed.\n");
> + return PTR_ERR(priv->sdrctl);
> + }
> +
> + ret = alt_fpga2sdram_get_mask(priv);
> + if (ret) {
> + devm_kfree(&pdev->dev, priv);
same.

> + return ret;
> + }
> +
> + ret = register_fpga_bridge(pdev, &altera_fpga2sdram_br_ops,
> + priv->name, priv);
> +
> + if (!ret)
> + return ret;
> +
> + if (of_property_read_u32(priv->np, "init-val", &init_val)) {
> + dev_info(&priv->pdev->dev, "init-val not specified\n");
> + } else if (init_val > 1) {
> + dev_warn(&priv->pdev->dev, "invalid init-val %u > 1\n",
> + init_val);
> + } else {
> + dev_info(&priv->pdev->dev, "%s bridge\n",
> + (init_val ? "enabling" : "disabling"));
> +
> + _alt_fpga2sdram_enable_set(priv, init_val);
> + }
> +
> + return ret;

What happens to the instances atomic counter on failure? In fact I don’t see
the point of it as it is. Do you need to test for a single fpga_bridge instance?

What happens when you have multiple fpgas? Won’t you need multiple bridges?

> +}
> +
> +static int alt_fpga_bridge_remove(struct platform_device *pdev)
> +{
> + remove_fpga_bridge(pdev);
> + atomic_dec(&instances);
> + return 0;
> +}
> +
> +static struct of_device_id altera_fpga_of_match[] = {
> + { .compatible = "altr,socfpga-fpga2sdram-bridge",
> + .data = &fpga2sdram_data },
> + {},
> +};
> +
> +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",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(altera_fpga_of_match),
> + },
> +};
> +
> +static int __init alt_fpga_bridge_init(void)
> +{
> + atomic_set(&instances, 0);
> + 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);
> +}
> +
> +arch_initcall(alt_fpga_bridge_init);
> +module_exit(alt_fpga_bridge_exit);
> +
> +MODULE_DESCRIPTION("Altera SoCFPGA FPGA to SDRAM Bridge");
> +MODULE_AUTHOR("Alan Tull <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/misc/fpga-bridge/altera-hps2fpga.c b/drivers/misc/fpga-bridge/altera-hps2fpga.c
> new file mode 100644
> index 0000000..a6cbdd3
> --- /dev/null
> +++ b/drivers/misc/fpga-bridge/altera-hps2fpga.c
> @@ -0,0 +1,220 @@
> +/*
> + * FPGA to/from HPS Bridge Driver for Altera SoCFPGA Devices
> + *
> + * Copyright (C) 2013 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/>.
> + */
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include "fpga-bridge.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"
> +static struct of_device_id altera_fpga_of_match[];
> +
> +/* The L3 REMAP register is write only, so keep a cached value. */
> +static unsigned int l3_remap_value;
> +
> +struct altera_hps2fpga_data {
> + char name[48];
> + struct platform_device *pdev;
> + struct device_node *np;
> + struct reset_control *bridge_reset;
> + struct regmap *l3reg;
> + unsigned int reset_mask;
> + unsigned int remap_mask;
> +};
> +
> +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 inline void _alt_hps2fpga_enable_set(struct altera_hps2fpga_data *priv,
> + bool enable)
> +{
> + /* bring bridge out of reset */
> + if (enable)
> + reset_control_deassert(priv->bridge_reset);
> + else
> + reset_control_assert(priv->bridge_reset);
> +
> + /* Allow bridge to be visible to L3 masters or not */
> + if (priv->remap_mask) {
> + l3_remap_value |= ALT_L3_REMAP_MPUZERO_MSK;
> +
> + if (enable)
> + l3_remap_value |= priv->remap_mask;
> + else
> + l3_remap_value &= ~priv->remap_mask;
> +
> + regmap_write(priv->l3reg, ALT_L3_REMAP_OFST, l3_remap_value);
> + }
> +}
> +
> +static void alt_hps2fpga_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> + _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 int alt_fpga_bridge_probe(struct platform_device *pdev)
> +{
> + struct altera_hps2fpga_data *priv;
> + const struct of_device_id *of_id;
> + struct device *dev = &pdev->dev;
> + uint32_t init_val;
> + int rc;
> + struct clk *clk;
> +
> + of_id = of_match_device(altera_fpga_of_match, dev);
> + priv = (struct altera_hps2fpga_data *)of_id->data;
> + WARN_ON(!priv);
> +
> + priv->np = dev->of_node;
> + priv->pdev = pdev;
> +
> + 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_phandle(priv->np,
> + "altr,l3-syscon");
> + if (IS_ERR(priv->l3reg)) {
> + dev_err(dev, "regmap for altr,l3regs lookup failed.\n");
> + return PTR_ERR(priv->l3reg);
> + }
> +
> + clk = of_clk_get(pdev->dev.of_node, 0);
> + if (IS_ERR(clk)) {
> + dev_err(dev, "no clock specified\n");
> + return PTR_ERR(clk);
> + }
> +
> + rc = clk_prepare_enable(clk);
> + if (rc) {
> + dev_err(dev, "could not enable clock\n");
> + return -EBUSY;
> + }
> +
> + rc = register_fpga_bridge(pdev, &altera_hps2fpga_br_ops, priv->name,
> + priv);
> + if (rc)
> + return rc;
> +
> + if (of_property_read_u32(priv->np, "init-val", &init_val)) {
> + dev_info(&priv->pdev->dev, "init-val not specified\n");
> + } else if (init_val > 1) {
> + dev_warn(&priv->pdev->dev, "invalid init-val %u > 1\n",
> + init_val);
> + } else {
> + dev_info(&priv->pdev->dev, "%s bridge\n",
> + (init_val ? "enabling" : "disabling"));
> +
> + _alt_hps2fpga_enable_set(priv, init_val);
> + }
> +
> + return rc;
> +}
> +
> +static int alt_fpga_bridge_remove(struct platform_device *pdev)
> +{
> + remove_fpga_bridge(pdev);
> + return 0;
> +}
> +
> +static 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 },
> + {},
> +};
> +
> +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",
> + .owner = THIS_MODULE,
> + .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);
> +}
> +
> +arch_initcall(alt_fpga_bridge_init);
> +module_exit(alt_fpga_bridge_exit);
> +
> +MODULE_DESCRIPTION("Altera SoCFPGA HPS to FPGA Bridge");
> +MODULE_AUTHOR("Alan Tull <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/misc/fpga-bridge/fpga-bridge.c b/drivers/misc/fpga-bridge/fpga-bridge.c
> new file mode 100644
> index 0000000..d70862c
> --- /dev/null
> +++ b/drivers/misc/fpga-bridge/fpga-bridge.c
> @@ -0,0 +1,230 @@
> +/*
> + * fpga bridge driver
> + *
> + * Copyright (C) 2013 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/>.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/slab.h>
> +#include "fpga-bridge.h"
> +
> +static DEFINE_IDA(fpga_bridge_ida);
> +static struct class *fpga_bridge_class;
> +
> +#define FPGA_MAX_DEVICES 256
> +
> +/*
> + * class attributes
> + */
> +static ssize_t fpga_bridge_enable_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct fpga_bridge *bridge = dev_get_drvdata(dev);
> + int enabled;
> +
> + enabled = bridge->br_ops->enable_show(bridge);
> +
> + return sprintf(buf, "%d\n", enabled);
> +}
> +
> +static ssize_t fpga_bridge_enable_set(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_bridge *bridge = dev_get_drvdata(dev);
> + bool enable;
> +
> + if ((count != 1) && (count != 2))
> + return -EINVAL;
> +
> + if ((count == 2) && (buf[1] != '\n'))
> + return -EINVAL;
> +
> + if ((buf[0] != '0') && (buf[0] != '1'))
> + return -EINVAL;
> +
> + enable = (buf[0] == '1’);

Superfluous enclosing parentheses.

> + bridge->br_ops->enable_set(bridge, enable);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR, fpga_bridge_enable_show,
> + fpga_bridge_enable_set);
> +
> +static struct attribute *fpga_bridge_attrs[] = {
> + &dev_attr_enable.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group fpga_bridge_group = {
> + .attrs = fpga_bridge_attrs,
> +};
> +
> +const struct attribute_group *fpga_bridge_groups[] = {
> + &fpga_bridge_group,
> + NULL,
> +};
> +
> +static int fpga_bridge_alloc_id(struct fpga_bridge *bridge, int request_nr)
> +{
> + int nr, start;
> +
> + /* check specified minor number */
> + if (request_nr >= FPGA_MAX_DEVICES) {
> + dev_err(bridge->parent,
> + "Out of device ids (%d)\n", request_nr);
> + return -ENODEV;
> + }
> +
> + /*
> + * If request_nr == -1, dynamically allocate number.
> + * If request_nr >= 0, attempt to get specific number.
> + */
> + if (request_nr == -1)
> + start = 0;
> + else
> + start = request_nr;
> +
> + nr = ida_simple_get(&fpga_bridge_ida, start, FPGA_MAX_DEVICES,
> + GFP_KERNEL);
> +
> + /* return error code */
> + if (nr < 0)
> + return nr;
> +
> + if ((request_nr != -1) && (request_nr != nr)) {

Enclosing parentheses again.

> + dev_err(bridge->parent,
> + "Could not get requested device number (%d)\n", nr);
> + ida_simple_remove(&fpga_bridge_ida, nr);
> + return -ENODEV;
> + }
> +
> + bridge->nr = nr;
> +
> + return 0;
> +}
> +
> +static void fpga_bridge_free_id(int nr)
> +{
> + ida_simple_remove(&fpga_bridge_ida, nr);
> +}
> +
> +int register_fpga_bridge(struct platform_device *pdev,
> + struct fpga_bridge_ops *br_ops,
> + char *name, void *priv)
> +{
> + struct fpga_bridge *bridge;
> + const char *dt_label;
> + int ret;
> +
> + if (!br_ops || !br_ops->enable_set || !br_ops->enable_show) {
> + dev_err(&pdev->dev,
> + "Attempt to register without fpga_bridge_ops\n");
> + return -EINVAL;
> + }
> + if (!name || (name[0] == '\0')) {

Parentheses.

> + dev_err(&pdev->dev, "Attempt to register with no name!\n");
> + return -EINVAL;
> + }
> +
> + bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> + if (!bridge)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, bridge);
> + bridge->br_ops = br_ops;
> + bridge->np = pdev->dev.of_node;
> + bridge->parent = get_device(&pdev->dev);
> + bridge->priv = priv;
> +
> + strlcpy(bridge->name, name, sizeof(bridge->name));
> +
> + ret = fpga_bridge_alloc_id(bridge, pdev->id);
> + if (ret)
> + goto error_kfree;
> +
> + dt_label = of_get_property(bridge->np, "label", NULL);
> + if (dt_label)
> + snprintf(bridge->label, sizeof(bridge->label), "%s", dt_label);
> + else
> + snprintf(bridge->label, sizeof(bridge->label),
> + "br%d", bridge->nr);
> +
> + bridge->dev = device_create(fpga_bridge_class, bridge->parent, 0,
> + bridge, bridge->label);
> + if (IS_ERR(bridge->dev)) {
> + ret = PTR_ERR(bridge->dev);
> + goto error_device;
> + }
> +
> + dev_info(bridge->parent, "fpga bridge [%s] registered as device %s\n",
> + bridge->name, bridge->label);
> +
> + return 0;
> +
> +error_device:
> + fpga_bridge_free_id(bridge->nr);
> +error_kfree:
> + put_device(bridge->parent);
> + kfree(bridge);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(register_fpga_bridge);
> +
> +void remove_fpga_bridge(struct platform_device *pdev)
> +{
> + struct fpga_bridge *bridge = platform_get_drvdata(pdev);
> +
> + if (bridge && bridge->br_ops && bridge->br_ops->fpga_bridge_remove)
> + bridge->br_ops->fpga_bridge_remove(bridge);
> +
> + platform_set_drvdata(pdev, NULL);
> + device_unregister(bridge->dev);
> + fpga_bridge_free_id(bridge->nr);
> + put_device(bridge->parent);
> + kfree(bridge);
> +}
> +EXPORT_SYMBOL_GPL(remove_fpga_bridge);
> +
> +static int __init fpga_bridge_dev_init(void)
> +{
> + pr_info("fpga bridge driver\n");
> +
> + fpga_bridge_class = class_create(THIS_MODULE, "fpga-bridge");
> + if (IS_ERR(fpga_bridge_class))
> + return PTR_ERR(fpga_bridge_class);
> +
> + fpga_bridge_class->dev_groups = fpga_bridge_groups;
> +
> + return 0;
> +}
> +
> +static void __exit fpga_bridge_dev_exit(void)
> +{
> + class_destroy(fpga_bridge_class);
> + ida_destroy(&fpga_bridge_ida);
> +}
> +
> +MODULE_DESCRIPTION("FPGA Bridge Driver");
> +MODULE_AUTHOR("Alan Tull <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> +
> +core_initcall_sync(fpga_bridge_dev_init);
> +module_exit(fpga_bridge_dev_exit);
> diff --git a/drivers/misc/fpga-bridge/fpga-bridge.h b/drivers/misc/fpga-bridge/fpga-bridge.h
> new file mode 100644
> index 0000000..b7b5e70
> --- /dev/null
> +++ b/drivers/misc/fpga-bridge/fpga-bridge.h
> @@ -0,0 +1,51 @@
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/cdev.h>
> +
> +#ifndef _LINUX_FPGA_BRIDGE_H
> +#define _LINUX_FPGA_BRIDGE_H
> +
> +struct fpga_bridge;
> +
> +/*---------------------------------------------------------------------------*/
> +
> +/*
> + * fpga_bridge_ops are the low level functions implemented by a specific
> + * fpga bridge driver.
> + */
> +struct fpga_bridge_ops {
> + /* Returns the FPGA bridge's status */
> + int (*enable_show)(struct fpga_bridge *bridge);
> +
> + /* Enable a FPGA bridge */
> + void (*enable_set)(struct fpga_bridge *bridge, bool enable);
> +
> + /* Set FPGA into a specific state during driver remove */
> + void (*fpga_bridge_remove)(struct fpga_bridge *bridge);
> +};
> +
> +struct fpga_bridge {
> + struct device_node *np;
> + struct device *parent;
> + struct device *dev;
> + struct cdev cdev;
> +
> + int nr;
> + char name[48];
> + char label[48];
> + unsigned long flags;
> + struct fpga_bridge_ops *br_ops;
> +
> + void *priv;
> +};
> +
> +#if defined(CONFIG_FPGA_BRIDGE) || defined(CONFIG_FPGA_BRIDGE_MODULE)
> +
> +int register_fpga_bridge(struct platform_device *pdev,
> + struct fpga_bridge_ops *br_ops,
> + char *name, void *priv);
> +
> +void remove_fpga_bridge(struct platform_device *pdev);
> +
> +#endif /* CONFIG_FPGA_BRIDGE */
> +#endif /* _LINUX_FPGA_BRIDGE_H */
> --
> 1.7.9.5
>

Regards

— Pantelis

2014-10-27 11:48:12

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Alan,

> On Oct 24, 2014, at 02:51 , [email protected] wrote:
>
> From: Alan Tull <[email protected]>
>
> Add DTS binding documentation for the Altera FPGA bridges.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> .../bindings/fpga/altera-fpga2sdram-bridge.txt | 57 ++++++++++++++++++++
> .../bindings/fpga/altera-hps2fpga-bridge.txt | 53 ++++++++++++++++++
> 2 files changed, 110 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> create mode 100644 Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>
> diff --git a/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> new file mode 100644
> index 0000000..cc8f522
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> @@ -0,0 +1,57 @@
> +Altera FPGA To SDRAM Bridge Driver
> +
> +This driver manages a bridge between an FPGA and the SDRAM used by an host
> +processor system (HPS). The bridge contains a number read ports, write ports,
> +and command ports. Reconfiguring these ports requires that no SDRAM
> +transactions occur during reconfiguration. In other words, the code
> +reconfiguring the ports cannot be run out of SDRAM nor can the FPGA access the
> +SDRAM during the reconfiguration. This driver does not support reconfiguring
> +the ports. Typcially, the ports are configured by code running out of onchip
> +ram before Linux is started.
> +
> +This driver supports enabling and disabling of the configured ports all at
> +once, which allows for safe reprogramming of the FPGA from user space, provided
> +the new FPGA image uses the same port configuration. User space can enable or
> +disable the bridge by writing a "1" or a "0", respectively, to its enable file
> +under bridge's entry in /sys/class/fpga-bridge. Typically, one disables the
> +bridges before reprogramming the FPGA. Once the FPGA is reprogrammed, the
> +bridges are reenabled.
> +
> +Required properties:
> +
> + - compatible : "altr,socfpga-fpga2sdram-bridge"
> +
> + - read-ports-mask : Bits 0 to 3 corresponds read ports 0 to 3. A bit set to 1
> + indicates the corresponding read port should be enabled.
> +
> + - write-ports-mask : Bits 0 to 3 corresponds write ports 0 to 3. A bit set
> + to 1 indicates the corresponding write port should be
> + enabled.
> +
> + - cmd-ports-mask : Bits 0 to 5 correspond to command ports 0 to 5. A bit set
> + to 1 indicates the corresponding command port should be
> + enabled.
> +
> + - altr,sdr-syscon : phandle of the sdr module
> +
> +Optional properties:
> +
> + - label : name that you want this bridge to show up as under
> + /sys/class/fpga-bridge. Default is br<device#> if this is
> + not specified.
> +
> + - init-val : 0 if driver should disable bridge at startup
> + 1 if driver should enable bridge at startup
> + driver leaves bridge in current state if property not
> + specified.

Isn’t init-val a boolean property? It’s not named very well.

Along with the label, is kinda hard to defend as configuration in DT.

We need to start thinking about configuration, so let me throw that in
and fan the Monday flames to a nice red-hot color.

/ {
chosen {
linux {
fpga-bridge {
node = <&FPGA_BRIDGE>;
label = “foo”;
enable-bridge-on-startup;
}
};
};
};

We will need accessors for drivers that iterate over the chosen/linux node children
and pick up the properties meant for the given driver node.

We also need to define behaviour in case those configuration properties are absent.

Let the flames begin…

> +
> +Example:
> + fpga2sdram_br: fpgabridge@3 {
> + compatible = "altr,socfpga-fpga2sdram-bridge";
> + label = "fpga2sdram";
> + altr,sdr-syscon = <&sdr>;
> + read-ports-mask = <3>;
> + write-ports-mask = <3>;
> + cmd-ports-mask = <0xd>;
> + init-val = <0>;
> + };
> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> new file mode 100644
> index 0000000..bc24a2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> @@ -0,0 +1,53 @@
> +Altera FPGA/HPS Bridge Driver
> +
> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> +User space can enable or disable the bridge by writing a "1" or a "0",
> +respectively, to its enable file under bridge's entry in
> +/sys/class/fpga-bridge. Typically, one disables the bridges before
> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> +reenabled.
> +
> +Required properties:
> +
> + - compatible : should contain one of:
> + "altr,socfpga-hps2fpga-bridge"
> + "altr,socfpga-lwhps2fpga-bridge"
> + "altr,socfpga-fpga2hps-bridge"
> +
> + - clocks : clocks used by this module
> +
> + - altr,l3-syscon : phandle of the l3 interconnect module
> +
> +Optional properties:
> + - label : name that you want this bridge to show up as under
> + /sys/class/fpga-bridge. Default is br<device#> if this is
> + not specified.
> +
> + - init-val : 0 if driver should disable bridge at startup
> + 1 if driver should enable bridge at startup
> + driver leaves bridge in current state if property not
> + specified.
> +
> +Example:
> + hps_fpgabridge0: fpgabridge@0 {
> + compatible = "altr,socfpga-hps2fpga-bridge";
> + label = "hps2fpga";
> + altr,l3-syscon = <&l3regs>;
> + clocks = <&l4_main_clk>;
> + init-val = <1>;
> + };
> +
> + hps_fpgabridge1: fpgabridge@1 {
> + compatible = "altr,socfpga-lwhps2fpga-bridge";
> + label = "lwhps2fpga";
> + altr,l3-syscon = <&l3regs>;
> + clocks = <&l4_main_clk>;
> + init-val = <0>;
> + };
> +
> + hps_fpgabridge2: fpgabridge@2 {
> + compatible = "altr,socfpga-fpga2hps-bridge";
> + label = "fpga2hps";
> + altr,l3-syscon = <&l3regs>;
> + clocks = <&l4_main_clk>;
> + };
> --
> 1.7.9.5
>

Regards

— Pantelis

2014-10-27 11:54:31

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Stefen,

> On Oct 25, 2014, at 17:42 , Steffen Trumtrar <[email protected]> wrote:
>
> Hi Pantelis!
>
> On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
>> Hi Stefan, Allan,
>>
>> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
>> Just wanted to pop in and comment on a few things.
>>
>>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <[email protected]> wrote:
>>>
>>> Hi!
>>>
>>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
>>>> From: Alan Tull <[email protected]>
>>>
>>> (...)
>>>
>>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>> new file mode 100644
>>>> index 0000000..bc24a2e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>> @@ -0,0 +1,53 @@
>>>> +Altera FPGA/HPS Bridge Driver
>>>> +
>>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
>>>> +User space can enable or disable the bridge by writing a "1" or a "0",
>>>> +respectively, to its enable file under bridge's entry in
>>>> +/sys/class/fpga-bridge. Typically, one disables the bridges before
>>>> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
>>>> +reenabled.
>>>> +
>>>
>>> NAK.
>>>
>>> This is all linux specific and doesn't belong here.
>>>
>>
>> We know. The DT spec hasn’t been updated for a while, and this is going to be
>> part of what we want to do with the restarting of the ePAPR spec process.
>>
>
> As we don't have a new spec yet, I go with the current state of the art.
> And I don't see how "file under ... /sys/class/fpga-bridge" fits the
> current spec.
>
>> So absolutes like “DT is a hardware description only" might be too strong statements, that
>> do not work in practice anymore.
>>
>> There’s no such thing as pure hardware devices lately - there is always a configuration
>> component; some of it OS specific, some of it not.
>>
>
> If it really is necessary, I'm not against it, don't get me wrong.
> But in the grand scheme I wouldn't say that this fits my experience.
> There are some devices that need configuration and often when it is
> done in the DT, it is just a shortcut or not thought through.
> And can be also introspected dynamically. With some discussion on the
> list these bindings often change.
>

Already answered and given a possible way this might work; admittedly this
specific example is not a good fit for what I was trying to say, but whatever.
Let’s get the ball rolling.

> Regarding OS specifics: already there, e.g. console setup in the chosen node.
> And other things I saw are ethernet aliases just for u-boot. Not a problem
> of the spec, just a problem of u-boot and unneccessary "configuration".
> Just to fix a bad bootloader. barebox can handle this without any
> such stuff. Maybe we should integrate the DT "overlays" barebox uses into
> the in-kernel DTs as well...but does it really help/interest someone who
> decides to use u-boot where barebox stores its environment? I guess not.
>

Although I’m not against having DT overlays in the bootloader, I only see them
as a method that helps a platform developer express things easier. I am completely
against having the kernel tied to any particular bootloader.

We’ve all been through the hell where a kernel only boots if the bootloader has
setup the platform “correctly”. This “correctly” has a very loose definition and
99/100 times is extremely badly documented or not at all.

IMHO the bootloader should (as part of the normal boot sequence) only setup the
absolutely bare minimum and then pass control to the kernel as soon as possible.

The full featured bootloader environment should only be presented when the user
requests so.

>> We have to be engaged in the process and figure out how to update the spec for what is
>> now the state of the art in the industry.
>>
>
> Again, not against that if it is necessary. For example your overlay stuff might
> be a good update to the spec. Would be better if it is official instead of a "hack".
> I want that for SoCFPGA.
>
> But having looked at one or two vendor kernels+DTs, the state of the art in the
> industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
> ePAPR necessary ;-))
>

Vendor H/W, vendor DT and a crack pipe. Or as I call it Monday.

> Regards,
> Steffen

Regards

— Pantelis

2014-10-27 15:03:16

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
> > On Oct 24, 2014, at 02:51 , [email protected] wrote:

> > + - init-val : 0 if driver should disable bridge at startup
> > + 1 if driver should enable bridge at startup
> > + driver leaves bridge in current state if property not
> > + specified.

> Isn’t init-val a boolean property? It’s not named very well.

It's not boolean, it's tristate - turn on, turn off or don't touch.

> Along with the label, is kinda hard to defend as configuration in DT.

Yeah... presumably this decision would fall out of the users?

Attachments:

(No filename) (657.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-10-27 15:05:39

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Mark,

> On Oct 27, 2014, at 17:01 , Mark Brown <[email protected]> wrote:
>
> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
>>> On Oct 24, 2014, at 02:51 , [email protected] wrote:
>
>>> + - init-val : 0 if driver should disable bridge at startup
>>> + 1 if driver should enable bridge at startup
>>> + driver leaves bridge in current state if property not
>>> + specified.
>
>> Isn’t init-val a boolean property? It’s not named very well.
>
> It's not boolean, it's tristate - turn on, turn off or don't touch.
>

I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
enable-at-startup; disable-at-startup.

>> Along with the label, is kinda hard to defend as configuration in DT.
>
> Yeah... presumably this decision would fall out of the users?

Well, it’s the user that should make the decision, but the driver should
pick it up. This works but it’s not very nice.

Regards

— Pantelis

2014-10-27 15:23:43

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi!

On Mon, Oct 27, 2014 at 01:54:20PM +0200, Pantelis Antoniou wrote:
> Hi Stefen,
>
> > On Oct 25, 2014, at 17:42 , Steffen Trumtrar <[email protected]> wrote:
> >
> > Hi Pantelis!
> >
> > On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
> >> Hi Stefan, Allan,
> >>
> >> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
> >> Just wanted to pop in and comment on a few things.
> >>
> >>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <[email protected]> wrote:
> >>>
> >>> Hi!
> >>>
> >>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
> >>>> From: Alan Tull <[email protected]>
> >>>
> >>> (...)
> >>>
> >>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>> new file mode 100644
> >>>> index 0000000..bc24a2e
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>> @@ -0,0 +1,53 @@
> >>>> +Altera FPGA/HPS Bridge Driver
> >>>> +
> >>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> >>>> +User space can enable or disable the bridge by writing a "1" or a "0",
> >>>> +respectively, to its enable file under bridge's entry in
> >>>> +/sys/class/fpga-bridge. Typically, one disables the bridges before
> >>>> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> >>>> +reenabled.
> >>>> +
> >>>
> >>> NAK.
> >>>
> >>> This is all linux specific and doesn't belong here.
> >>>
> >>
> >> We know. The DT spec hasn’t been updated for a while, and this is going to be
> >> part of what we want to do with the restarting of the ePAPR spec process.
> >>
> >
> > As we don't have a new spec yet, I go with the current state of the art.
> > And I don't see how "file under ... /sys/class/fpga-bridge" fits the
> > current spec.
> >
> >> So absolutes like “DT is a hardware description only" might be too strong statements, that
> >> do not work in practice anymore.
> >>
> >> There’s no such thing as pure hardware devices lately - there is always a configuration
> >> component; some of it OS specific, some of it not.
> >>
> >
> > If it really is necessary, I'm not against it, don't get me wrong.
> > But in the grand scheme I wouldn't say that this fits my experience.
> > There are some devices that need configuration and often when it is
> > done in the DT, it is just a shortcut or not thought through.
> > And can be also introspected dynamically. With some discussion on the
> > list these bindings often change.
> >
>
> Already answered and given a possible way this might work; admittedly this
> specific example is not a good fit for what I was trying to say, but whatever.
> Let’s get the ball rolling.
>

I would be okay with chosen-node. Not for this driver; but in general.

> > Regarding OS specifics: already there, e.g. console setup in the chosen node.
> > And other things I saw are ethernet aliases just for u-boot. Not a problem
> > of the spec, just a problem of u-boot and unneccessary "configuration".
> > Just to fix a bad bootloader. barebox can handle this without any
> > such stuff. Maybe we should integrate the DT "overlays" barebox uses into
> > the in-kernel DTs as well...but does it really help/interest someone who
> > decides to use u-boot where barebox stores its environment? I guess not.
> >
>
> Although I’m not against having DT overlays in the bootloader, I only see them
> as a method that helps a platform developer express things easier. I am completely
> against having the kernel tied to any particular bootloader.
>
> We’ve all been through the hell where a kernel only boots if the bootloader has
> setup the platform “correctly”. This “correctly” has a very loose definition and
> 99/100 times is extremely badly documented or not at all.
>
> IMHO the bootloader should (as part of the normal boot sequence) only setup the
> absolutely bare minimum and then pass control to the kernel as soon as possible.
>
> The full featured bootloader environment should only be presented when the user
> requests so.
>

Totally agree. The kernel shouldn't be tied to any bootloader if at all possible
(the SoCFPGA is an especially bad example here again, as the pinmuxing can only
happen in the bootloader).
But should the DT include stuff than, that is only interesting for linux?

> >> We have to be engaged in the process and figure out how to update the spec for what is
> >> now the state of the art in the industry.
> >>
> >
> > Again, not against that if it is necessary. For example your overlay stuff might
> > be a good update to the spec. Would be better if it is official instead of a "hack".
> > I want that for SoCFPGA.
> >
> > But having looked at one or two vendor kernels+DTs, the state of the art in the
> > industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
> > ePAPR necessary ;-))
> >
>
> Vendor H/W, vendor DT and a crack pipe. Or as I call it Monday.
>

;-)

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 |

2014-10-27 15:32:29

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
>
> > On Oct 27, 2014, at 17:01 , Mark Brown <[email protected]> wrote:
> >
> > On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
> >>> On Oct 24, 2014, at 02:51 , [email protected] wrote:
> >
> >>> + - init-val : 0 if driver should disable bridge at startup
> >>> + 1 if driver should enable bridge at startup
> >>> + driver leaves bridge in current state if property not
> >>> + specified.
> >
> >> Isn’t init-val a boolean property? It’s not named very well.
> >
> > It's not boolean, it's tristate - turn on, turn off or don't touch.
> >
>
> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
> enable-at-startup; disable-at-startup.
>
> >> Along with the label, is kinda hard to defend as configuration in DT.
> >
> > Yeah... presumably this decision would fall out of the users?
>
> Well, it’s the user that should make the decision, but the driver should
> pick it up. This works but it’s not very nice.
>

Hm, convince me why this AXI bus is so special, that I even need an
"init-val" property? Other buses don't have that.
Why don't I add a property "init-val" to my SPI buses, so I can enable
it in the DT and still have it in reset, just because....

The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
written firmware to the FPGA and I have subnodes on that bus, I have to
get it out of reset and probe everything. Normal procedure, no ?!

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 |

2014-10-27 15:45:15

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Stefan,

> On Oct 27, 2014, at 17:32 , Steffen Trumtrar <[email protected]> wrote:
>
> On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
>> Hi Mark,
>>
>>> On Oct 27, 2014, at 17:01 , Mark Brown <[email protected]> wrote:
>>>
>>> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
>>>>> On Oct 24, 2014, at 02:51 , [email protected] wrote:
>>>
>>>>> + - init-val : 0 if driver should disable bridge at startup
>>>>> + 1 if driver should enable bridge at startup
>>>>> + driver leaves bridge in current state if property not
>>>>> + specified.
>>>
>>>> Isn’t init-val a boolean property? It’s not named very well.
>>>
>>> It's not boolean, it's tristate - turn on, turn off or don't touch.
>>>
>>
>> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
>> enable-at-startup; disable-at-startup.
>>
>>>> Along with the label, is kinda hard to defend as configuration in DT.
>>>
>>> Yeah... presumably this decision would fall out of the users?
>>
>> Well, it’s the user that should make the decision, but the driver should
>> pick it up. This works but it’s not very nice.
>>
>
> Hm, convince me why this AXI bus is so special, that I even need an
> "init-val" property? Other buses don't have that.
> Why don't I add a property "init-val" to my SPI buses, so I can enable
> it in the DT and still have it in reset, just because....
>
> The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
> written firmware to the FPGA and I have subnodes on that bus, I have to
> get it out of reset and probe everything. Normal procedure, no ?!
>

Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
time to be programmed. If someone has already configured the ‘bus’ it is considered
a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
the bitstream already, you don’t want to do it again.

I’m afraid there’s no such analogue with standard hardware busses like SPI, where
the bus setup time is instantaneous.

>
> Regards,
> Steffen
>

Regards

— Pantelis

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

2014-10-27 15:52:12

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Steffen,

> On Oct 27, 2014, at 17:23 , Steffen Trumtrar <[email protected]> wrote:
>
> Hi!
>
> On Mon, Oct 27, 2014 at 01:54:20PM +0200, Pantelis Antoniou wrote:
>> Hi Stefen,
>>
>>> On Oct 25, 2014, at 17:42 , Steffen Trumtrar <[email protected]> wrote:
>>>
>>> Hi Pantelis!
>>>
>>> On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
>>>> Hi Stefan, Allan,
>>>>
>>>> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
>>>> Just wanted to pop in and comment on a few things.
>>>>
>>>>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <[email protected]> wrote:
>>>>>
>>>>> Hi!
>>>>>
>>>>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
>>>>>> From: Alan Tull <[email protected]>
>>>>>
>>>>> (...)
>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..bc24a2e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>>>> @@ -0,0 +1,53 @@
>>>>>> +Altera FPGA/HPS Bridge Driver
>>>>>> +
>>>>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
>>>>>> +User space can enable or disable the bridge by writing a "1" or a "0",
>>>>>> +respectively, to its enable file under bridge's entry in
>>>>>> +/sys/class/fpga-bridge. Typically, one disables the bridges before
>>>>>> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
>>>>>> +reenabled.
>>>>>> +
>>>>>
>>>>> NAK.
>>>>>
>>>>> This is all linux specific and doesn't belong here.
>>>>>
>>>>
>>>> We know. The DT spec hasn’t been updated for a while, and this is going to be
>>>> part of what we want to do with the restarting of the ePAPR spec process.
>>>>
>>>
>>> As we don't have a new spec yet, I go with the current state of the art.
>>> And I don't see how "file under ... /sys/class/fpga-bridge" fits the
>>> current spec.
>>>
>>>> So absolutes like “DT is a hardware description only" might be too strong statements, that
>>>> do not work in practice anymore.
>>>>
>>>> There’s no such thing as pure hardware devices lately - there is always a configuration
>>>> component; some of it OS specific, some of it not.
>>>>
>>>
>>> If it really is necessary, I'm not against it, don't get me wrong.
>>> But in the grand scheme I wouldn't say that this fits my experience.
>>> There are some devices that need configuration and often when it is
>>> done in the DT, it is just a shortcut or not thought through.
>>> And can be also introspected dynamically. With some discussion on the
>>> list these bindings often change.
>>>
>>
>> Already answered and given a possible way this might work; admittedly this
>> specific example is not a good fit for what I was trying to say, but whatever.
>> Let’s get the ball rolling.
>>
>
> I would be okay with chosen-node. Not for this driver; but in general.
>

Let’s figure out how to do it then…

>>> Regarding OS specifics: already there, e.g. console setup in the chosen node.
>>> And other things I saw are ethernet aliases just for u-boot. Not a problem
>>> of the spec, just a problem of u-boot and unneccessary "configuration".
>>> Just to fix a bad bootloader. barebox can handle this without any
>>> such stuff. Maybe we should integrate the DT "overlays" barebox uses into
>>> the in-kernel DTs as well...but does it really help/interest someone who
>>> decides to use u-boot where barebox stores its environment? I guess not.
>>>
>>
>> Although I’m not against having DT overlays in the bootloader, I only see them
>> as a method that helps a platform developer express things easier. I am completely
>> against having the kernel tied to any particular bootloader.
>>
>> We’ve all been through the hell where a kernel only boots if the bootloader has
>> setup the platform “correctly”. This “correctly” has a very loose definition and
>> 99/100 times is extremely badly documented or not at all.
>>
>> IMHO the bootloader should (as part of the normal boot sequence) only setup the
>> absolutely bare minimum and then pass control to the kernel as soon as possible.
>>
>> The full featured bootloader environment should only be presented when the user
>> requests so.
>>
>
> Totally agree. The kernel shouldn't be tied to any bootloader if at all possible
> (the SoCFPGA is an especially bad example here again, as the pinmuxing can only
> happen in the bootloader).

I’m not familiar with SoCFPGA. Why pinmuxing is only possible in the bootloader?
Can’t the bootloader configure the minimum pinmux config and then have Linux setup
the rest?

If we’re feeling particularly nasty, we might just require bootloaders to clean the
hardware state before passing control to Linux (besides memory controller setup) and
see what’s broken.

> But should the DT include stuff than, that is only interesting for linux?
>

Like it or not it’s Linux that’s in the forefront of many hardware designs.

There are configuration settings in DT that are part of the way hardware is presented to
Linux and user-space, and have been for quite some time. We’ve been pretending they don’t
exist, but they are there…

We have a chance to try to get it right, so why not do so?

>>>> We have to be engaged in the process and figure out how to update the spec for what is
>>>> now the state of the art in the industry.
>>>>
>>>
>>> Again, not against that if it is necessary. For example your overlay stuff might
>>> be a good update to the spec. Would be better if it is official instead of a "hack".
>>> I want that for SoCFPGA.
>>>
>>> But having looked at one or two vendor kernels+DTs, the state of the art in the
>>> industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
>>> ePAPR necessary ;-))
>>>
>>
>> Vendor H/W, vendor DT and a crack pipe. Or as I call it Monday.
>>
>
> ;-)
>
> Regards,
> Steffen
>

Regards

— Pantelis

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

2014-10-27 17:18:54

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:

Please fix your mail client to word wrap at less than 80 columns.

> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
> time to be programmed. If someone has already configured the ‘bus’ it is considered
> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
> the bitstream already, you don’t want to do it again.

That's not what your property is saying, though - if it were just about
handover between the bootloader and the kernel that'd be one thing but
it's also got this additional possibility to instruct the kernel that
the device must be either disabled or explicitly programmed. Having a
property that just says "this FPGA is programmed" covers the handover
case but this property does more than that.

Attachments:

(No filename) (859.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-10-27 17:21:32

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Mark,

> On Oct 27, 2014, at 19:17 , Mark Brown <[email protected]> wrote:
>
> On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:
>
> Please fix your mail client to word wrap at less than 80 columns.
>
>> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
>> time to be programmed. If someone has already configured the ‘bus’ it is considered
>> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
>> the bitstream already, you don’t want to do it again.
>
> That's not what your property is saying, though - if it were just about
> handover between the bootloader and the kernel that'd be one thing but
> it's also got this additional possibility to instruct the kernel that
> the device must be either disabled or explicitly programmed. Having a
> property that just says "this FPGA is programmed" covers the handover
> case but this property does more than that.

It’s not my property. This is not my driver. The property is badly-named
as it is IMHO.

Why don’t we let Alan chime in?

Regards

— Pantelis

2014-10-27 17:48:09

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Mon, Oct 27, 2014 at 05:52:02PM +0200, Pantelis Antoniou wrote:
> Hi Steffen,
>
> > On Oct 27, 2014, at 17:23 , Steffen Trumtrar <[email protected]> wrote:
> >
> > Hi!
> >
> > On Mon, Oct 27, 2014 at 01:54:20PM +0200, Pantelis Antoniou wrote:
> >> Hi Stefen,
> >>
> >>> On Oct 25, 2014, at 17:42 , Steffen Trumtrar <[email protected]> wrote:
> >>>
> >>> Hi Pantelis!
> >>>
> >>> On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
> >>>> Hi Stefan, Allan,
> >>>>
> >>>> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
> >>>> Just wanted to pop in and comment on a few things.
> >>>>
> >>>>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <[email protected]> wrote:
> >>>>>
> >>>>> Hi!
> >>>>>
> >>>>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
> >>>>>> From: Alan Tull <[email protected]>
> >>>>>
> >>>>> (...)
> >>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..bc24a2e
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>>>> @@ -0,0 +1,53 @@
> >>>>>> +Altera FPGA/HPS Bridge Driver
> >>>>>> +
> >>>>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> >>>>>> +User space can enable or disable the bridge by writing a "1" or a "0",
> >>>>>> +respectively, to its enable file under bridge's entry in
> >>>>>> +/sys/class/fpga-bridge. Typically, one disables the bridges before
> >>>>>> +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> >>>>>> +reenabled.
> >>>>>> +
> >>>>>
> >>>>> NAK.
> >>>>>
> >>>>> This is all linux specific and doesn't belong here.
> >>>>>
> >>>>
> >>>> We know. The DT spec hasn’t been updated for a while, and this is going to be
> >>>> part of what we want to do with the restarting of the ePAPR spec process.
> >>>>
> >>>
> >>> As we don't have a new spec yet, I go with the current state of the art.
> >>> And I don't see how "file under ... /sys/class/fpga-bridge" fits the
> >>> current spec.
> >>>
> >>>> So absolutes like “DT is a hardware description only" might be too strong statements, that
> >>>> do not work in practice anymore.
> >>>>
> >>>> There’s no such thing as pure hardware devices lately - there is always a configuration
> >>>> component; some of it OS specific, some of it not.
> >>>>
> >>>
> >>> If it really is necessary, I'm not against it, don't get me wrong.
> >>> But in the grand scheme I wouldn't say that this fits my experience.
> >>> There are some devices that need configuration and often when it is
> >>> done in the DT, it is just a shortcut or not thought through.
> >>> And can be also introspected dynamically. With some discussion on the
> >>> list these bindings often change.
> >>>
> >>
> >> Already answered and given a possible way this might work; admittedly this
> >> specific example is not a good fit for what I was trying to say, but whatever.
> >> Let’s get the ball rolling.
> >>
> >
> > I would be okay with chosen-node. Not for this driver; but in general.
> >
>
> Let’s figure out how to do it then…
>
> >>> Regarding OS specifics: already there, e.g. console setup in the chosen node.
> >>> And other things I saw are ethernet aliases just for u-boot. Not a problem
> >>> of the spec, just a problem of u-boot and unneccessary "configuration".
> >>> Just to fix a bad bootloader. barebox can handle this without any
> >>> such stuff. Maybe we should integrate the DT "overlays" barebox uses into
> >>> the in-kernel DTs as well...but does it really help/interest someone who
> >>> decides to use u-boot where barebox stores its environment? I guess not.
> >>>
> >>
> >> Although I’m not against having DT overlays in the bootloader, I only see them
> >> as a method that helps a platform developer express things easier. I am completely
> >> against having the kernel tied to any particular bootloader.
> >>
> >> We’ve all been through the hell where a kernel only boots if the bootloader has
> >> setup the platform “correctly”. This “correctly” has a very loose definition and
> >> 99/100 times is extremely badly documented or not at all.
> >>
> >> IMHO the bootloader should (as part of the normal boot sequence) only setup the
> >> absolutely bare minimum and then pass control to the kernel as soon as possible.
> >>
> >> The full featured bootloader environment should only be presented when the user
> >> requests so.
> >>
> >
> > Totally agree. The kernel shouldn't be tied to any bootloader if at all possible
> > (the SoCFPGA is an especially bad example here again, as the pinmuxing can only
> > happen in the bootloader).
>
> I’m not familiar with SoCFPGA. Why pinmuxing is only possible in the bootloader?
> Can’t the bootloader configure the minimum pinmux config and then have Linux setup
> the rest?
>
> If we’re feeling particularly nasty, we might just require bootloaders to clean the
> hardware state before passing control to Linux (besides memory controller setup) and
> see what’s broken.
>

Okay, offtopic, but:

a) datasheet explicitely states that dynamic pinmuxing is not supported.
b) last time I checked, no documentation at all. You get some magic values spit
out by Altera's tools.
c) you don't have registers. Pinmuxing is done via JTAG scan chain; which you
feed the magic values.

Apart from that, it seems to be the most sane idea, to at least write your
driver in a way, that doesn't make any assumptions on HW state.
But I think this is already common practice in mainline.

> > But should the DT include stuff than, that is only interesting for linux?
> >
>
> Like it or not it’s Linux that’s in the forefront of many hardware designs.
>

I like it; pays my rent ;-)
But I also like freedom to choose...

> There are configuration settings in DT that are part of the way hardware is presented to
> Linux and user-space, and have been for quite some time. We’ve been pretending they don’t
> exist, but they are there…
>

As far as I know, a lot of that are older bindings, where more
things just got applied, because it wasn't clear where DT will go.

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 |

2014-10-27 18:01:26

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:
> Hi Stefan,
>
> > On Oct 27, 2014, at 17:32 , Steffen Trumtrar <[email protected]> wrote:
> >
> > On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
> >> Hi Mark,
> >>
> >>> On Oct 27, 2014, at 17:01 , Mark Brown <[email protected]> wrote:
> >>>
> >>> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
> >>>>> On Oct 24, 2014, at 02:51 , [email protected] wrote:
> >>>
> >>>>> + - init-val : 0 if driver should disable bridge at startup
> >>>>> + 1 if driver should enable bridge at startup
> >>>>> + driver leaves bridge in current state if property not
> >>>>> + specified.
> >>>
> >>>> Isn’t init-val a boolean property? It’s not named very well.
> >>>
> >>> It's not boolean, it's tristate - turn on, turn off or don't touch.
> >>>
> >>
> >> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
> >> enable-at-startup; disable-at-startup.
> >>
> >>>> Along with the label, is kinda hard to defend as configuration in DT.
> >>>
> >>> Yeah... presumably this decision would fall out of the users?
> >>
> >> Well, it’s the user that should make the decision, but the driver should
> >> pick it up. This works but it’s not very nice.
> >>
> >
> > Hm, convince me why this AXI bus is so special, that I even need an
> > "init-val" property? Other buses don't have that.
> > Why don't I add a property "init-val" to my SPI buses, so I can enable
> > it in the DT and still have it in reset, just because....
> >
> > The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
> > written firmware to the FPGA and I have subnodes on that bus, I have to
> > get it out of reset and probe everything. Normal procedure, no ?!
> >
>
> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
> time to be programmed. If someone has already configured the ‘bus’ it is considered
> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
> the bitstream already, you don’t want to do it again.
>
> I’m afraid there’s no such analogue with standard hardware busses like SPI, where
> the bus setup time is instantaneous.

Ah, okay. I see why you got confused.
The bridges are not in any way responsible for loading the FPGA nor will
resetting them reset the bitstream.

The FPGA Manager, a different IP core, is responsible for that. And AFAIK
it has a status bit, that tells you if the FPGA is programmed or not.
Loading the bitstream is in the order of milliseconds.

So from the bridges point of view, when you probe it, you can ask the
FPGA manager if is done, otherwise -EPROBE_DEFER and than go on and
probe the subdevices.

If you are bored, take a look at
http://www.altera.com/literature/hb/cyclone-v/cv_54004.pdf
page 7-2. There you can see the hardware setup.

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 |

2014-10-27 18:03:51

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi Steffen,

> On Oct 27, 2014, at 20:00 , Steffen Trumtrar <[email protected]> wrote:
>
> On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:
>> Hi Stefan,
>>
>>> On Oct 27, 2014, at 17:32 , Steffen Trumtrar <[email protected]> wrote:
>>>
>>> On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
>>>> Hi Mark,
>>>>
>>>>> On Oct 27, 2014, at 17:01 , Mark Brown <[email protected]> wrote:
>>>>>
>>>>> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
>>>>>>> On Oct 24, 2014, at 02:51 , [email protected] wrote:
>>>>>
>>>>>>> + - init-val : 0 if driver should disable bridge at startup
>>>>>>> + 1 if driver should enable bridge at startup
>>>>>>> + driver leaves bridge in current state if property not
>>>>>>> + specified.
>>>>>
>>>>>> Isn’t init-val a boolean property? It’s not named very well.
>>>>>
>>>>> It's not boolean, it's tristate - turn on, turn off or don't touch.
>>>>>
>>>>
>>>> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
>>>> enable-at-startup; disable-at-startup.
>>>>
>>>>>> Along with the label, is kinda hard to defend as configuration in DT.
>>>>>
>>>>> Yeah... presumably this decision would fall out of the users?
>>>>
>>>> Well, it’s the user that should make the decision, but the driver should
>>>> pick it up. This works but it’s not very nice.
>>>>
>>>
>>> Hm, convince me why this AXI bus is so special, that I even need an
>>> "init-val" property? Other buses don't have that.
>>> Why don't I add a property "init-val" to my SPI buses, so I can enable
>>> it in the DT and still have it in reset, just because....
>>>
>>> The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
>>> written firmware to the FPGA and I have subnodes on that bus, I have to
>>> get it out of reset and probe everything. Normal procedure, no ?!
>>>
>>
>> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
>> time to be programmed. If someone has already configured the ‘bus’ it is considered
>> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
>> the bitstream already, you don’t want to do it again.
>>
>> I’m afraid there’s no such analogue with standard hardware busses like SPI, where
>> the bus setup time is instantaneous.
>
> Ah, okay. I see why you got confused.
> The bridges are not in any way responsible for loading the FPGA nor will
> resetting them reset the bitstream.
>
> The FPGA Manager, a different IP core, is responsible for that. And AFAIK
> it has a status bit, that tells you if the FPGA is programmed or not.
> Loading the bitstream is in the order of milliseconds.
>
> So from the bridges point of view, when you probe it, you can ask the
> FPGA manager if is done, otherwise -EPROBE_DEFER and than go on and
> probe the subdevices.
>
>

Ohh, I see. Sorry for the confusion.

> If you are bored, take a look at
> http://www.altera.com/literature/hb/cyclone-v/cv_54004.pdf
> page 7-2. There you can see the hardware setup.
>

Thanks. /me throws it at the pile of things to read…

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

Regards

— Pantelis

2014-10-28 21:26:02

by atull

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Fri, 24 Oct 2014, Steffen Trumtrar wrote:

> Hi!
>

Hi,

I see that my documentation sucks and needs cleanup. I'll try to
answer some of the flames and get a more coherent version out soon.

> On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
> > From: Alan Tull <[email protected]>
>
> (...)
>
> > diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > new file mode 100644
> > index 0000000..bc24a2e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > @@ -0,0 +1,53 @@
> > +Altera FPGA/HPS Bridge Driver
> > +
> > +This driver manages a bridge between a FPGA and a host processor system (HPS).
> > +User space can enable or disable the bridge by writing a "1" or a "0",
> > +respectively, to its enable file under bridge's entry in
> > +/sys/class/fpga-bridge. Typically, one disables the bridges before
> > +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> > +reenabled.
> > +
>
> NAK.
>
> This is all linux specific and doesn't belong here.

Right. This stuff shouldn't be in this document. While I was squashing
patches and cleaning up for posting on the mailing list, I added a sysfs
document and forgot to clean up my DT bindings documents.

>
> > +Required properties:
> > +
> > + - compatible : should contain one of:
> > + "altr,socfpga-hps2fpga-bridge"
> > + "altr,socfpga-lwhps2fpga-bridge"
> > + "altr,socfpga-fpga2hps-bridge"
> > +
> > + - clocks : clocks used by this module
> > +
> > + - altr,l3-syscon : phandle of the l3 interconnect module
> > +
>
> L3 shouldn't be a syscon.

L3 is actually a good candidate for syscon. Lots of registers, each one
affects a different hardware block.

> Have you tried dumping the regmap in the debugfs if L3
> is a syscon? Doesn't work.

Is that a bug in regmap or is that because the register in L3 that
I am actully interested in here is write-only (ugh)?

>
> > +Optional properties:
> > + - label : name that you want this bridge to show up as under
> > + /sys/class/fpga-bridge. Default is br<device#> if this is
> > + not specified.
> > +
>
> Why? Linux-specific.

That was a convience for the user. I can take that out and won't miss it.

>
> > + - init-val : 0 if driver should disable bridge at startup
> > + 1 if driver should enable bridge at startup
> > + driver leaves bridge in current state if property not
> > + specified.
> > +
>
> Configuration in the DT? Really?
>
> > +Example:
> > + hps_fpgabridge0: fpgabridge@0 {
> > + compatible = "altr,socfpga-hps2fpga-bridge";
> > + label = "hps2fpga";
> > + altr,l3-syscon = <&l3regs>;
> > + clocks = <&l4_main_clk>;
> > + init-val = <1>;
> > + };
> > +
> > + hps_fpgabridge1: fpgabridge@1 {
> > + compatible = "altr,socfpga-lwhps2fpga-bridge";
> > + label = "lwhps2fpga";
> > + altr,l3-syscon = <&l3regs>;
> > + clocks = <&l4_main_clk>;
> > + init-val = <0>;
> > + };
> > +
> > + hps_fpgabridge2: fpgabridge@2 {
> > + compatible = "altr,socfpga-fpga2hps-bridge";
> > + label = "fpga2hps";
> > + altr,l3-syscon = <&l3regs>;
> > + clocks = <&l4_main_clk>;
> > + };
>
> The bridges are the buses into the FPGA. This has to be accomodated.
> The bridges have two specified memory ranges: one the address space
> of the bus, the second the register space for configuration.
>
> This binding does NOT correctly describe the hardware. Sorry.
>

OK that was outdated. More cleanup needed. How about this type of
binding? Here's an actual use case for something that has multiple
pieces of soft IP in the FPGA (sysid, gpio). I eliminated a few.

*snippet of DT*
sopc@0 {
device_type = "soc";
ranges;
#address-cells = <0x1>;
#size-cells = <0x1>;
compatible = "ALTR,avalon", "simple-bus";
bus-frequency = <0x0>;

bridge@0xc0000000 {
compatible = "altr,bridge-14.0", "simple-bus";
reg = <0xc0000000 0x20000000 0xff200000 0x200000>;
reg-names = "axi_h2f", "axi_h2f_lw";
clocks = <0x2 0x2 0x2>;
clock-names = "h2f_axi_clock", "h2f_lw_axi_clock",
"f2h_sdram0_clock";
#address-cells = <0x2>;
#size-cells = <0x1>;
ranges = <0x0 0x0 0xc0000000 0x10000
0x1 0x10000 0xff210000 0x8
0x1 0x10040 0xff210040 0x20
0x1 0x10080 0xff210080 0x10
0x1 0x100c0 0xff2100c0 0x10
0x1 0x20000 0xff220000 0x8>;

sysid@0x100010000 {
compatible = "altr,sysid-14.0", "altr,sysid-1.0";
reg = <0x1 0x10000 0x8>;
clocks = <0x2>;
id = <0xacd51402>;
timestamp = <0x540a048e>;
};

gpio@0x100010040 {
compatible = "altr,pio-14.0", "altr,pio-1.0";
reg = <0x1 0x10040 0x20>;
clocks = <0x2>;
altr,gpio-bank-width = <0x4>;
resetvalue = <0x0>;
#gpio-cells = <0x2>;
gpio-controller;
linux,phandle = <0x2a>;
};

/* other hardware that exists on FPGA here */

};
};

Alan

> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2014-10-28 22:01:21

by atull

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Tue, 28 Oct 2014, atull wrote:

> On Fri, 24 Oct 2014, Steffen Trumtrar wrote:

> >
> > > + - init-val : 0 if driver should disable bridge at startup
> > > + 1 if driver should enable bridge at startup
> > > + driver leaves bridge in current state if property not
> > > + specified.
> > > +
> >
> > Configuration in the DT? Really?

The FPGA can be programmed by several different methods (eeprom, jtag,
preloader, Linux). The guiding principle is: whoever programmed the
FPGA should release the bridge. At one point, we thought 'init-val'
would be needed, but I think it can be taken out and we won't miss it.

Alan

2014-10-29 07:57:44

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi!

On Tue, Oct 28, 2014 at 04:19:03PM -0500, atull wrote:
> On Fri, 24 Oct 2014, Steffen Trumtrar wrote:
>
> > Hi!
> >
>
> Hi,
>
> I see that my documentation sucks and needs cleanup. I'll try to
> answer some of the flames and get a more coherent version out soon.
>
> > On Thu, Oct 23, 2014 at 06:51:06PM -0500, [email protected] wrote:
> > > From: Alan Tull <[email protected]>
> >
> > (...)
> >
> > > diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > > new file mode 100644
> > > index 0000000..bc24a2e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > > @@ -0,0 +1,53 @@
> > > +Altera FPGA/HPS Bridge Driver
> > > +
> > > +This driver manages a bridge between a FPGA and a host processor system (HPS).
> > > +User space can enable or disable the bridge by writing a "1" or a "0",
> > > +respectively, to its enable file under bridge's entry in
> > > +/sys/class/fpga-bridge. Typically, one disables the bridges before
> > > +reprogramming the FPGA. Once the FPGA is reprogrammed, the bridges are
> > > +reenabled.
> > > +
> >
> > NAK.
> >
> > This is all linux specific and doesn't belong here.
>
> Right. This stuff shouldn't be in this document. While I was squashing
> patches and cleaning up for posting on the mailing list, I added a sysfs
> document and forgot to clean up my DT bindings documents.
>
> >
> > > +Required properties:
> > > +
> > > + - compatible : should contain one of:
> > > + "altr,socfpga-hps2fpga-bridge"
> > > + "altr,socfpga-lwhps2fpga-bridge"
> > > + "altr,socfpga-fpga2hps-bridge"
> > > +
> > > + - clocks : clocks used by this module
> > > +
> > > + - altr,l3-syscon : phandle of the l3 interconnect module
> > > +
> >
> > L3 shouldn't be a syscon.
>
> L3 is actually a good candidate for syscon. Lots of registers, each one
> affects a different hardware block.
>
> > Have you tried dumping the regmap in the debugfs if L3
> > is a syscon? Doesn't work.
>
> Is that a bug in regmap or is that because the register in L3 that
> I am actully interested in here is write-only (ugh)?
>

That is not a bug in regmap. It is because you only have a register at 0x4008,
than one at 0x4104, that one at 0x5008, etc.... (values from memory, so might
be wrong)

Of course regmap tries to read from the whole register range if specified as
syscon. Which it can't.

So, that shouldn't be a problem though, as I already cooked up a driver for
the L3 with all the ranges specified. The only thing I need to figure out
before I will post it, is how to nicely handle the WO remap register.
I think regmap might be able to handle this itself, but am not sure yet.

> >
> > > +Optional properties:
> > > + - label : name that you want this bridge to show up as under
> > > + /sys/class/fpga-bridge. Default is br<device#> if this is
> > > + not specified.
> > > +
> >
> > Why? Linux-specific.
>
> That was a convience for the user. I can take that out and won't miss it.
>
> >
> > > + - init-val : 0 if driver should disable bridge at startup
> > > + 1 if driver should enable bridge at startup
> > > + driver leaves bridge in current state if property not
> > > + specified.
> > > +
> >
> > Configuration in the DT? Really?
> >
> > > +Example:
> > > + hps_fpgabridge0: fpgabridge@0 {
> > > + compatible = "altr,socfpga-hps2fpga-bridge";
> > > + label = "hps2fpga";
> > > + altr,l3-syscon = <&l3regs>;
> > > + clocks = <&l4_main_clk>;
> > > + init-val = <1>;
> > > + };
> > > +
> > > + hps_fpgabridge1: fpgabridge@1 {
> > > + compatible = "altr,socfpga-lwhps2fpga-bridge";
> > > + label = "lwhps2fpga";
> > > + altr,l3-syscon = <&l3regs>;
> > > + clocks = <&l4_main_clk>;
> > > + init-val = <0>;
> > > + };
> > > +
> > > + hps_fpgabridge2: fpgabridge@2 {
> > > + compatible = "altr,socfpga-fpga2hps-bridge";
> > > + label = "fpga2hps";
> > > + altr,l3-syscon = <&l3regs>;
> > > + clocks = <&l4_main_clk>;
> > > + };
> >
> > The bridges are the buses into the FPGA. This has to be accomodated.
> > The bridges have two specified memory ranges: one the address space
> > of the bus, the second the register space for configuration.
> >
> > This binding does NOT correctly describe the hardware. Sorry.
> >
>
> OK that was outdated. More cleanup needed. How about this type of
> binding? Here's an actual use case for something that has multiple
> pieces of soft IP in the FPGA (sysid, gpio). I eliminated a few.
>
> *snippet of DT*
> sopc@0 {
> device_type = "soc";
> ranges;
> #address-cells = <0x1>;
> #size-cells = <0x1>;
> compatible = "ALTR,avalon", "simple-bus";
> bus-frequency = <0x0>;
>
> bridge@0xc0000000 {
> compatible = "altr,bridge-14.0", "simple-bus";
> reg = <0xc0000000 0x20000000 0xff200000 0x200000>;
> reg-names = "axi_h2f", "axi_h2f_lw";
> clocks = <0x2 0x2 0x2>;
> clock-names = "h2f_axi_clock", "h2f_lw_axi_clock",
> "f2h_sdram0_clock";
> #address-cells = <0x2>;
> #size-cells = <0x1>;
> ranges = <0x0 0x0 0xc0000000 0x10000
> 0x1 0x10000 0xff210000 0x8
> 0x1 0x10040 0xff210040 0x20
> 0x1 0x10080 0xff210080 0x10
> 0x1 0x100c0 0xff2100c0 0x10
> 0x1 0x20000 0xff220000 0x8>;
>
> sysid@0x100010000 {
> compatible = "altr,sysid-14.0", "altr,sysid-1.0";
> reg = <0x1 0x10000 0x8>;
> clocks = <0x2>;
> id = <0xacd51402>;
> timestamp = <0x540a048e>;
> };
>
> gpio@0x100010040 {
> compatible = "altr,pio-14.0", "altr,pio-1.0";
> reg = <0x1 0x10040 0x20>;
> clocks = <0x2>;
> altr,gpio-bank-width = <0x4>;
> resetvalue = <0x0>;
> #gpio-cells = <0x2>;
> gpio-controller;
> linux,phandle = <0x2a>;
> };
>
> /* other hardware that exists on FPGA here */
>
> };
> };
>

Yes, something like this seems appropriate. Again, I also cooked up a driver
for this. I need to figure out why the GPV register writing sometimes doesn't
work as expected and will try to marry it to your FPGA manager framework
before I consider sending it to the list.

My proposed binding at the moment looks like:

Example:

hps2fpga: axibridge@ff500000 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "altr,hps2fpga-axi-bridge";
reg = <0xff500000 0x100000>,
<0xc0000000 0x3c000000>;
clocks = <&l4_mp_clk>, <&l3_main_clk>;
clock-names = "gpv_clk", "data_clk";
resets = <&rst HPS2FPGA_RESET>;
reset-names = "hps2fpga";
altr,gpv-master = <&lwhps2fpga>;
altr,l3-gpv = <&l3regs>;
bus-width = <64>;
status = "disabled";
ranges;
};

Board file example:

&hps2fpga {
bus-width = <32>;
status = "okay";

axi-ip: axi-ip@c0000000 {
compatible = "axi-ip";
reg = <0xc0000000 0x10000>;
clocks = <&h2f_usr2_clk>;
status = "okay";
};
};

So we seem to be almost on the same page. I first had the simple-bus in
there, too. But this will lead to all sorts of problems regarding driver
probing/removing on that bus.

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 |

2014-10-29 08:24:46

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

Hi!

> > > +Optional properties:
> > > + - label : name that you want this bridge to show up as under
> > > + /sys/class/fpga-bridge. Default is br<device#> if this is
> > > + not specified.
> > > +
> >
> > Why? Linux-specific.
>
> That was a convience for the user. I can take that out and won't
> miss it.

Actually make it

- label : optional user-readable name for this bridge

...and it is no longer Linux-specific, and you can keep it if it is
otherwise useful...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2014-10-29 08:25:48

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] add sysfs document for fpga bridges

On Fri 2014-10-24 10:11:26, atull wrote:
> On Fri, 24 Oct 2014, Pavel Machek wrote:
>
> > On Thu 2014-10-23 18:51:05, [email protected] wrote:
> > > From: Alan Tull <[email protected]>
> > >
> > > Add sysfs document for fpga bridges.
> > >
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-fpga-bridge
> > > @@ -0,0 +1,5 @@
> > > +What: /sys/class/fpga-bridge/<bridge>/enable
> > > +Date: October 2014
> > > +KernelVersion: 3.18
> > > +Contact: Alan Tull <[email protected]>
> > > +Description: Enable bridge. Write 1 to bring bridge out of reset.
> >
> > Specify, what happens when 0 is written there?
>
> Yes, I will add: 'Write 0 has no effect.'

Definitely add it, then. The way it is written, I'd expect 0 to put
bridge into reset.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2014-10-29 10:17:58

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ARM: dts: socfpga: fpga bridges bindings docs

On Wed, Oct 29, 2014 at 08:57:01AM +0100, Steffen Trumtrar wrote:

> So, that shouldn't be a problem though, as I already cooked up a driver for
> the L3 with all the ranges specified. The only thing I need to figure out
> before I will post it, is how to nicely handle the WO remap register.
> I think regmap might be able to handle this itself, but am not sure yet.

It's supposed to be able to, not sure if anyone really tests that or not
though so it's possible bugs crept in. Most write only registers can
physically be read so actually come out as volatile rather than write
only even if functionally there is no reason to read.

Attachments:

(No filename) (636.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-10-29 10:31:37