[PATCH 0/4] FPGAs as Memory Technology Devices in U-Boot

[PATCH 0/4] FPGAs as Memory Technology Devices in U-Boot

[u-boot]

As shown at a presentation in the recent OpenEmbedded Workshop,
it is possible to configure an FPGA in Passive Serial mode
using a standard SPI controller, each FPGA getting its own chipselect.

https://pretalx.com/openembedded-workshop-2023/talk/D3AQ3R/

This allows you to add the FPGA to the devicetree and to use standard MTD commands, instead of the FPGA commands.


I.E: The SPI portion is

&spi1 {
    u-boot,dm-spl;
    status = "okay";
    pinctrl-names = "default";
    pinctrl-0 = <&spi1_pins>;
    num-cs = <4>; /* Needed for GPIO cs */
    cs-gpios =
        <&gpio0 12 GPIO_ACTIVE_LOW>, /* D18,0:12 uart1_ctsn.spi1_cs0 */
        <&gpio0 13 GPIO_ACTIVE_LOW>, /* D17,0:13 uart1_rtsn.spi1_cs1 */
        <&gpio0 17 GPIO_ACTIVE_LOW>, /* K15,0:17 mii_txd2.spi1_cs2 */
        <&gpio0 16 GPIO_ACTIVE_LOW>; /* J18,0:16 mii_txd3.spi1_cs3 */
        spi-max-frequency = <10000000>; ;
        gpio_spi0: gpio_spi@0 {...}
        gpio_spi1: gpio_spi@1 {...}
        spi-fpga-cfg@2 {...} /* FPGA #1 */
        spy-fpga-cfg@3 {...} /* FPGA #2 */
};

The FPGA part is.

spi-fpga-cfg@2 { /* Intel Cyclone 10, 10CL010 */
    #address-cells = <1>;
    #size-cells = <1>;
    compatible = "intel,cyclone10";
    reg = <2>; /* Chip select 2 */
    spi-max-frequency = <10000000>;
    fpga = "spif"; /* Installed as /dev/spif */
    config-size = <368011>;
    nconfig-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>; /* ,3:15 */
    nstat-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>; /* ,3:19 */
    confd-gpios = <&gpio3 18 GPIO_ACTIVE_HIGH>; /* ,3:18 */
    crc-error-gpios= <&gpio2 1 GPIO_ACTIVE_HIGH>; /* ,2:01 */
    partition@0 {
        label = "spi-fpga";
        reg = <0x0000000 0x8000>;
    };
};

To configure the FPGA, you load the config info into RAM and write it to the FPGA.

U-BOOT> mtd read spi1 ${loadaddr} 0 ${filesize} # read from SPI
U-BOOT> mtd write fpga0 ${loadaddr} 0 ${filesize} # configure FPGA

A driver will pulse the nCONFIG pin of the FPGA, do an SPI transfer
and then check the FPGA status outputs.

Since the MTD command set can be used (and is needed anyway)
the FPGA command set can be removed from the U-Boot both simplifying
the user interface and reducing code size of the u-boot image.

It relies on the (hopefully) existing SPI driver for the chip in u-boot
so it should be easy to use in most systems (as long as the H/W is designed for it)


A linux driver, using the same principle would allow the FPGA to be
configured using a simple statement.

$ cat <bitfile> > /dev/fpga


The approach has been tested on a development board using an AM335x and 2 x Cyclone 10.

The changes needed are
* adding the FPGA class in mtd-abi.h
* The "mtd" command hardwires the transfer to be RAW and no OOB.
* A driver wrapping the control signals around an SPI transfer
  1.Claim SPI bus
  2.Pulse nCONFIG low for 40 us,
  3.Wait for nSTATUS high
  4.Transfer bitstream using U-Boot SPI transfer
  5.Release SPI bus
  6.Wait until CONFIG_DONE (or error on nSTATUS)

[PATCH 1/4] include/mtd/mtd-abi.h: Add FPGA as MTD device
[PATCH 2/4] cmd/mtd.c: Support FPGAs in mtd command
[PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)
[PATCH 4/4] mtd/Kconfig,Makefile support FPGA

[PATCH 1/4] include/mtd/mtd-abi.h: Add FPGA as MTD device

[u-boot]

From: Ulf Samuelsson <ulf@emagii.com>

Signed-off-by: Ulf Samuelsson <ulf@emagii.com>
---
 include/mtd/mtd-abi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index ea244fbaeb..cd826b64dd 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -89,6 +89,7 @@ struct mtd_write_req {
 #define MTD_DATAFLASH		6
 #define MTD_UBIVOLUME		7
 #define MTD_MLCNANDFLASH	8	/* MLC NAND (including TLC) */
+#define	MTD_FPGA		9
 
 #define MTD_WRITEABLE		0x400	/* Device is writeable */
 #define MTD_BIT_WRITEABLE	0x800	/* Single bits can be flipped */
@@ -100,6 +101,7 @@ struct mtd_write_req {
 #define MTD_CAP_RAM		(MTD_WRITEABLE | MTD_BIT_WRITEABLE | MTD_NO_ERASE)
 #define MTD_CAP_NORFLASH	(MTD_WRITEABLE | MTD_BIT_WRITEABLE)
 #define MTD_CAP_NANDFLASH	(MTD_WRITEABLE)
+#define MTD_CAP_FPGA		(MTD_WRITEABLE | MTD_NO_ERASE)
 
 /* Obsolete ECC byte placement modes (used with obsolete MEMGETOOBSEL) */
 #define MTD_NANDECC_OFF		0	// Switch off ECC (Not recommended)
-- 
2.17.1

[PATCH 2/4] cmd/mtd.c: Support FPGAs in mtd command

[u-boot]

From: Ulf Samuelsson <ulf@emagii.com>

Signed-off-by: Ulf Samuelsson <ulf@emagii.com>
---
 cmd/mtd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index eb6e2d6892..09d5fdaa11 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -158,6 +158,9 @@ static void mtd_show_device(struct mtd_info *mtd)
 	case MTD_MLCNANDFLASH:
 		printf("MLC NAND flash\n");
 		break;
+	case MTD_FPGA:
+		printf("FPGA\n");
+		break;
 	case MTD_ABSENT:
 	default:
 		printf("Unknown\n");
@@ -275,6 +278,11 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
 	raw = strstr(cmd, ".raw");
 	woob = strstr(cmd, ".oob");
 	write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
+	if (mtd->type == MTD_FPGA) {
+		raw = true;
+		woob = false;
+		write_empty_pages = true;
+	}
 
 	argc -= 2;
 	argv += 2;
-- 
2.17.1

[PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[u-boot]

From: Ulf Samuelsson <ulf@emagii.com>

Signed-off-by: Ulf Samuelsson <ulf@emagii.com>
---
 drivers/mtd/fpga/Kconfig      |  47 ++++++
 drivers/mtd/fpga/Makefile     |   6 +
 drivers/mtd/fpga/cyclone_10.c | 278 ++++++++++++++++++++++++++++++++++
 3 files changed, 331 insertions(+)
 create mode 100644 drivers/mtd/fpga/Kconfig
 create mode 100644 drivers/mtd/fpga/Makefile
 create mode 100644 drivers/mtd/fpga/cyclone_10.c

diff --git a/drivers/mtd/fpga/Kconfig b/drivers/mtd/fpga/Kconfig
new file mode 100644
index 0000000000..e3aa8c4522
--- /dev/null
+++ b/drivers/mtd/fpga/Kconfig
@@ -0,0 +1,47 @@
+menu "SPI FPGA Support"
+
+config DM_SPI_FPGA
+	bool "Enable Driver Model for FPGA configuration"
+	depends on DM && DM_SPI
+	imply SPI_FPGA
+	help
+	  Enable driver model for FPGAs configurable using SPI.
+	  This SPI FPGA interface
+	  (spi_fpga_probe(), spi_fpga_write(), etc.) is then
+	  implemented by the SPI FPGA uclass.
+	  There is one standard SPI FPGA driver which knows how to probe
+	  chips supported by U-Boot. The uclass interface is defined in
+	  include/spi_fpga.h
+	  SPI and SPI FPGA must be enabled together
+	  (it is not possible to use driver model for one and not the other).
+
+if DM_SPI_FPGA
+
+config SPI_FPGA_MTD
+	bool "SPI FPGA MTD support"
+	depends on MTD
+	help
+	  Enable the MTD support for the FPGA SPI Passive Serial,
+	  This allows mtd_write commands to load an FPGA using passive serial
+	  If unsure, say N
+
+config SPI_FPGA_INTEL
+	bool "Intel/Altera FPGA Passive Serial configuration using SPI"
+	help
+	  Add support for various Intel SPI FPGA chips
+
+config SPI_FPGA_XILINX
+	bool "Xilinx FPGA Passive Serial configuration using SPI"
+	help
+	  Add support for various Xilinx FPGA chips
+
+config SPI_FPGA_CYCLONE10
+	bool "Cyclone 10 SPI FPGA MTD support"
+	depends on SPI_FPGA_MTD && SPI_FPGA_INTEL
+	help
+	  Enable the MTD support for the Cyclone 10 FPGA
+	  If unsure, say N
+
+endif
+
+endmenu # menu "SPI FPGA Support"
diff --git a/drivers/mtd/fpga/Makefile b/drivers/mtd/fpga/Makefile
new file mode 100644
index 0000000000..2cf19fc7cf
--- /dev/null
+++ b/drivers/mtd/fpga/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# (C) Copyright 2006
+# Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+
+obj-$(CONFIG_SPI_FPGA_CYCLONE10) += cyclone_10.o
diff --git a/drivers/mtd/fpga/cyclone_10.c b/drivers/mtd/fpga/cyclone_10.c
new file mode 100644
index 0000000000..41e273211e
--- /dev/null
+++ b/drivers/mtd/fpga/cyclone_10.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MTD Driver for Passive Serial configuration of Cyclone 10
+ *
+ * Copyright (C) 2020 Bombardier Transportation
+ * Ulf Samuelsson <ext.ulf.samuelsson@rail.bombardier.com>
+ * Ulf Samuelsson <ulf@emagii.com>
+ *
+ */
+
+#include <common.h>
+#include <console.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdt_support.h>
+//#include <flash.h>
+#include <mtd.h>
+#include <malloc.h>
+#include <spi.h>
+#include <asm/io.h>
+#include <asm/gpio.h>
+#include <linux/delay.h>
+#include <dm/device_compat.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/*
+ * How many milliseconds from CONF_DONE high to enter user mode
+ * Datasheet says 650 us, Delay 2 ms to be safe...
+ */
+#define	USER_MODE_DELAY				2
+
+struct cyc10_plat {
+	struct udevice		*dev;
+	struct spi_slave	*spi;
+	char			name[8];
+	struct gpio_desc	nconfig;
+	struct gpio_desc	nstatus;
+	struct gpio_desc	conf_done;
+	struct gpio_desc	crc_error;
+	u32			cs;
+	int			flags;
+	int			config_size;
+};
+
+static inline void write_nCONFIG(struct cyc10_plat *fpga, int value)
+{
+	dm_gpio_set_value(&fpga->nconfig, value);
+}
+
+static inline int read_nSTATUS(struct cyc10_plat *fpga)
+{
+	int val = dm_gpio_get_value(&fpga->nstatus);
+	if (val < 0) {
+		printf("%s: Failure reading nSTATUS; error=%d\n", fpga->name, val);
+	}
+	return val;
+}
+
+static inline int read_CONFIG_DONE(struct cyc10_plat *fpga)
+{
+	int val = dm_gpio_get_value(&fpga->conf_done);
+	if (val < 0) {
+		printf("%s: Failure reading CONFIG_DONE; error=%d\n", fpga->name, val);
+	}
+	return val;
+}
+
+static inline int read_CRC_ERROR(struct cyc10_plat *fpga)
+{
+	int val = dm_gpio_get_value(&fpga->crc_error);
+	if (val < 0) {
+		printf("%s: Failure reading CRC_ERROR; error=%d\n", fpga->name, val);
+	}
+	return val;
+}
+
+/*
+ * Service routine to read status register until ready, or timeout occurs.
+ * Returns non-zero if error.
+ */
+static int cyc10_wait_until_conf_done(struct cyc10_plat *fpga)
+{
+	unsigned long timebase;
+	timebase = get_timer(0);
+
+	while (get_timer(timebase) < USER_MODE_DELAY) {
+		if (read_nSTATUS(fpga) == 0)		/* Bad configuration */
+			return -EIO;
+		if (read_CONFIG_DONE(fpga) == 1)	/* Ready */
+			return 0;
+	}
+
+	dev_err(fpga->dev, "fpga configuration timeout\n");
+	return -ETIMEDOUT;
+}
+
+
+/* FPGA Passive Serial configuration is not erasable */
+static int cyc10_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	instr->state = MTD_ERASE_DONE;
+	return 0;
+}
+
+/* FPGA Passive Serial configuration is not readable, do dummy read */
+static int cyc10_read(struct mtd_info *mtd, loff_t from, size_t len,
+			    size_t *retlen, u_char *buf)
+{
+	for (int i = 0 ; i < len ; i++)
+		buf[i] = 0xFF;
+	*retlen = len;
+	return 0;
+}
+
+static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t len,
+			     size_t *retlen, const u_char *buf)
+{
+	struct udevice *dev = mtd->dev;
+	struct spi_slave *slave = dev_get_parent_priv(dev);
+	struct cyc10_plat *fpga = dev_get_plat(dev);
+	int ret;
+
+	debug("%s: Claiming SPI Bus\n", fpga->name);
+	ret = spi_claim_bus(slave);
+	if (ret) {
+		printf("Failed to claim SPI bus\n");
+		return ret;
+	}
+
+	debug("%s: Starting configuration\n", fpga->name);
+	/* The FPGA configuration start by a positive edge on nCONFIG */
+	write_nCONFIG(fpga, 0);
+	udelay(50);	// Should be low for at least 40 us according to spec.
+	debug("nSTATUS = %d\n", read_nSTATUS(fpga));
+	write_nCONFIG(fpga, 1);
+	/* The FPGA is ready when nSTATUS is high */
+	{
+		int timeout = 200;
+		while(1) {
+			if (read_nSTATUS(fpga) != 0) {
+				break;
+			}
+			udelay(10);
+			timeout -= 10;
+			if (timeout < 0) {
+				debug("nSTATUS remains low\n");
+				return -ETIMEDOUT;
+			}
+			debug("nSTATUS = %d\n", read_nSTATUS(fpga));
+		}
+	}
+
+	/* FPGA needs some additional clocks to start up, add 8 bits */
+	/* This means we read past the buffer, which is hopefully OK */
+	debug("%s: Writing %d bytes\n", fpga->name, len);
+	ret = spi_xfer(slave, (len * 8) + 8, buf, NULL,
+		       SPI_XFER_ONCE | SPI_LSB_FIRST);
+
+	debug("%s: Releasing SPI Bus\n", fpga->name);
+	spi_release_bus(slave);
+
+	debug("%s: Waiting for CONF_DONE\n", fpga->name);
+
+	ret = cyc10_wait_until_conf_done(fpga);
+	*retlen = len;
+	return ret;
+}
+
+static void cyc10_sync(struct mtd_info *mtd)
+{
+}
+
+static int cyc10_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	return 0;
+}
+
+static int cyc10_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	return 0;
+}
+
+static int pin_request(struct udevice *dev, const char *pin_name, struct gpio_desc *pin, int mode)
+{
+	int	ret;
+	debug("gpio request: %s", pin_name);
+	if (gpio_request_by_name(dev, pin_name, 0, pin, mode )) {
+		debug("FAIL");
+		ret = -EINVAL;
+	} else {
+		ret = 0;
+		debug("OK");
+		debug("%s:%s: %s:[%d]\n", __func__, pin_name, pin->dev->name, pin->offset);
+	}
+	return ret;
+}
+
+static int cyc10_probe(struct udevice *dev)
+{
+	struct spi_slave *slave = dev_get_parent_priv(dev);
+	struct dm_spi_slave_plat *plat = dev_get_parent_plat(dev);
+	struct cyc10_plat *fpga = dev_get_plat(dev);
+	struct mtd_info *mtd;
+	int ret = 0;
+	const void *blob = gd->fdt_blob;
+	int node = dev_of_offset(dev);
+	/*
+	 * Use "fpga" property as device name if it exists.
+	 * Otherwise, use "fpga#" as device name # € {0..3}«»©“”nµªßðđŋħ
+	 */
+	const unsigned char *name = fdtdec_locate_byte_array(blob, node, "fpga", 4);
+	if (name == NULL) {
+		for (int i = 0; i < 3; i++) {
+			sprintf(fpga->name, "fpga%d", i);
+			mtd = get_mtd_device_nm(fpga->name);
+			if (PTR_ERR(mtd) == -ENODEV) {
+				goto allocate;
+			}
+		}
+		printf("Cannot allocate FPGA device\n");
+		return -ENODEV;
+	} else {
+		strncpy(fpga->name, (char *) name, sizeof(fpga->name));
+	}
+
+allocate:
+	fpga->spi = slave;
+	fpga->dev = dev;
+	fpga->cs  = plat->cs;
+
+	debug("%s: slave=%p, cs=%d\n", __func__, slave, fpga->cs);
+	fpga->config_size = fdtdec_get_int(blob, node, "config-size", 0);
+	debug("%s:config-size: %s:[%d]\n", __func__, fpga->name, fpga->config_size);
+
+	ret  = pin_request(dev, "nconfig-gpios",	&fpga->nconfig,   GPIOD_IS_OUT);
+	ret |= pin_request(dev, "nstat-gpios",		&fpga->nstatus,   GPIOD_IS_IN);
+	ret |= pin_request(dev, "confd-gpios",		&fpga->conf_done, GPIOD_IS_IN);
+	if (ret != 0)
+		return ret;
+	ret  = pin_request(dev, "crc-error-gpios",	&fpga->crc_error, GPIOD_IS_IN);	// May fail.
+
+	mtd = dev_get_uclass_priv(dev);
+	mtd->dev = dev;
+	mtd->name		= fpga->name;
+	mtd->type		= MTD_FPGA;
+	mtd->flags		= MTD_WRITEABLE;
+	mtd->size		= fpga->config_size;
+	mtd->writesize		= fpga->config_size;
+	mtd->writebufsize	= mtd->writesize;
+	mtd->_erase		= cyc10_erase;
+	mtd->_read		= cyc10_read;
+	mtd->_write		= cyc10_write;
+	mtd->_sync		= cyc10_sync;
+	mtd->_lock		= cyc10_lock;
+	mtd->_unlock		= cyc10_unlock;
+	mtd->numeraseregions = 0;
+	mtd->erasesize = 0x10000;
+	if (add_mtd_device(mtd)) {
+		debug("%s: registering %s as MTD failed\n", __func__, fpga->name);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static const struct udevice_id cyc10_ids[] = {
+	{ .compatible = "intel,cyclone10" },
+	{}
+};
+
+U_BOOT_DRIVER(cyc10) = {
+	.name	= "cyc10",
+	.id	= UCLASS_MTD,
+	.of_match = cyc10_ids,
+//	.priv_auto_alloc_size	= sizeof(struct cyc10_priv),
+	.plat_auto = sizeof(struct cyc10_plat),
+	.probe	= cyc10_probe,
+};
-- 
2.17.1

[PATCH 4/4] mtd/Kconfig,Makefile support FPGA

[u-boot]

From: Ulf Samuelsson <ulf@emagii.com>

Signed-off-by: Ulf Samuelsson <ulf@emagii.com>
---
 drivers/mtd/Kconfig  | 2 ++
 drivers/mtd/Makefile | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index af45ef00da..495211e314 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -238,6 +238,8 @@ config SYS_MAX_FLASH_BANKS_DETECT
 	 to reduce the effective number of flash bank, between 0 and
 	 CONFIG_SYS_MAX_FLASH_BANKS
 
+source "drivers/mtd/fpga/Kconfig"
+
 source "drivers/mtd/nand/Kconfig"
 
 config SYS_NAND_MAX_OOBFREE
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 3a78590aaa..d15ca24ec5 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -24,6 +24,7 @@ endif
 obj-y += nand/
 obj-y += onenand/
 obj-y += spi/
+obj-$(CONFIG_DM_SPI_FPGA) += fpga/
 obj-$(CONFIG_MTD_UBI) += ubi/
 
 #SPL/TPL build
-- 
2.17.1

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Marek Vasut]

On 2/11/23 11:07, u-boot@emagii.com wrote:

[...]

> +static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t len,
> +			     size_t *retlen, const u_char *buf)
> +{
> +	struct udevice *dev = mtd->dev;
> +	struct spi_slave *slave = dev_get_parent_priv(dev);
> +	struct cyc10_plat *fpga = dev_get_plat(dev);
> +	int ret;

Do I read this right, that the 'write' callback is the only one doing 
meaningful work, all the other callbacks are just empty stubs ?

Why not update drivers/fpga/cyclon2.c which is Passive Serial 
implementation already present in U-Boot for Altera Cyclone II FPGA , 
with Cyclone 10 FPGA support ? I believe the PS protocol changed very 
little.

[...]

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
 > On 2/11/23 11:07, u-boot@emagii.com wrote:
 >
 > [...]
 >
 >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t len,
 >> +                 size_t *retlen, const u_char *buf)
 >> +{
 >> +    struct udevice *dev = mtd->dev;
 >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
 >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
 >> +    int ret;
 >
 > Do I read this right, that the 'write' callback is the only one doing
 > meaningful work, all the other callbacks are just empty stubs ?
 > Yes, you cannot read back the configuration data.


 > Why not update drivers/fpga/cyclon2.c which is Passive Serial
 > implementation already present in U-Boot for Altera Cyclone II FPGA ,
 > with Cyclone 10 FPGA support ? I believe the PS protocol changed very
 > little.
Since the MTD command set is enough to configure the FPGA, the FPGA 
commands can be removed from the build. The FPGA command set requires 
you to supply addresses, but the MTD command set uses devices.

So:
* Smaller U-Boot image
* Simplified user interface.
* The FPGA is an SPI peripheral, so why not add it to the SPI part of 
the device tree?

Best Regards
Ulf Samuelsson

 >
 > [...]

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Marek Vasut]

On 2/12/23 20:52, Ulf Samuelsson wrote:
> 
> 
> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
>  > On 2/11/23 11:07, u-boot@emagii.com wrote:
>  >
>  > [...]
>  >
>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t len,
>  >> +                 size_t *retlen, const u_char *buf)
>  >> +{
>  >> +    struct udevice *dev = mtd->dev;
>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
>  >> +    int ret;
>  >
>  > Do I read this right, that the 'write' callback is the only one doing
>  > meaningful work, all the other callbacks are just empty stubs ?
>  > Yes, you cannot read back the configuration data.

That makes it look like any framework which supports "write" callback 
would be suitable, not just MTD framework.

>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
>  > implementation already present in U-Boot for Altera Cyclone II FPGA ,
>  > with Cyclone 10 FPGA support ? I believe the PS protocol changed very
>  > little.
> Since the MTD command set is enough to configure the FPGA, the FPGA 
> commands can be removed from the build. The FPGA command set requires 
> you to supply addresses, but the MTD command set uses devices.
> 
> So:
> * Smaller U-Boot image

The MTD framework is rather large, compared to the trivial FPGA 
framework. Can you back this claim with any numbers ?

> * Simplified user interface.

If I am to select between 'fpga load' and 'mtd write' for FPGA bitstream 
loading , my obvious choice would be 'fpga load' . How is using 'mtd 
write' any better or simpler ?

> * The FPGA is an SPI peripheral, so why not add it to the SPI part of 
> the device tree?

You can add the device into DT and still operate it using the U-Boot 
FPGA framework, just add the DT support. Why not do it that way ?

Let me be blunt about this, I have this feeling that what is happening 
here is just overloading of MTD framework with unrelated functionality 
(FPGA bitstream loading). MTD framework simply is not the right place, 
esp. if there is dedicated FPGA framework, with existing Altera PS 
driver no less.

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-12 kl. 21:01, skrev Marek Vasut:
 > On 2/12/23 20:52, Ulf Samuelsson wrote:
 >>
 >>
 >> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
 >>  > On 2/11/23 11:07, u-boot@emagii.com wrote:
 >>  >
 >>  > [...]
 >>  >
 >>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t len,
 >>  >> +                 size_t *retlen, const u_char *buf)
 >>  >> +{
 >>  >> +    struct udevice *dev = mtd->dev;
 >>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
 >>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
 >>  >> +    int ret;
 >>  >
 >>  > Do I read this right, that the 'write' callback is the only one doing
 >>  > meaningful work, all the other callbacks are just empty stubs ?
 >>  > Yes, you cannot read back the configuration data.
 >
 > That makes it look like any framework which supports "write" callback 
would be suitable, not just MTD framework.
 >
 >>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
 >>  > implementation already present in U-Boot for Altera Cyclone II FPGA ,
 >>  > with Cyclone 10 FPGA support ? I believe the PS protocol changed very
 >>  > little.
 >> Since the MTD command set is enough to configure the FPGA, the FPGA 
commands can be removed from the build. The FPGA command set requires 
you to supply addresses, but the MTD command set uses devices.
 >>
 >> So:
 >> * Smaller U-Boot image
 >
 > The MTD framework is rather large, compared to the trivial FPGA 
framework. Can you back this claim with any numbers ?

I am assuming that the MTD framework is needed anyway.
We certainly need it.
 >
 >> * Simplified user interface.
 >
 > If I am to select between 'fpga load' and 'mtd write' for FPGA 
bitstream loading , my obvious choice would be 'fpga load' . How is 
using 'mtd write' any better or simpler ?
 >
fpga load 0   ${loadaddr} ${filesize}
mtd write spy ${loadaddr}

The questions I ask myself.
So is "0" the "spy" FPGA or the "spx" FPGA?
Why should I have to remember the size of the FPGA bitstream?

 >> * The FPGA is an SPI peripheral, so why not add it to the SPI part 
of the device tree?
 >
 > You can add the device into DT and still operate it using the U-Boot 
FPGA framework, just add the DT support. Why not do it that way ?

I don't think you can add the device into DT in U-Boot as it is today.
You can create FPGA contents and add that to the device tree, but not
the configuration itself. At least, I have not seen it.
If I have missed it, where is an example?

The FPGA framework is not really setup to use device-tree to describe
configuration.

Only 5 defconfigs in U-Boot uses the FPGA framework.
* astro_mcf5373l_defconfig
* syzygy_hub_defconfig
* xilinx_zynqmp_virt_defconfig
* xilinx_zynq_virt_defconfig
* bitmain_antminer_s9_defconfig

Their device trees have leafs for configuration:
* compatible = "fpga-region";
* compatible = "xlnx,zynq-devcfg-1.0";
Neither "fpga-region" or "xlnx,zynq-devcfg-1.0" have any compatible 
drivers, so there is really no support for configuration based on 
device-tree at the moment.

If I look at boards using the Altera, no board uses a driver
to configure the FPGA. Instead they implement a number of callbacks
in the board files which manually handles the SPI bus outside the SPI 
driver. No trace of device tree.


 >
 > Let me be blunt about this, I have this feeling that what is 
happening here is just overloading of MTD framework with unrelated 
functionality (FPGA bitstream loading). MTD framework simply is not the 
right place, esp. if there is dedicated FPGA framework, with existing 
Altera PS driver no less.

When you are configuring an FPGA, you are writing a bitstream
to an SRAM inside the FPGA. SRAM are memories.

The MTD framework does exactly what is needed for passive serial.
I do not see that there is another place to add a device-tree based
FPGA configuration, so you basically have to replicate the MTD 
framework, in order to have FPGA configuration in the device tree.


Best Regards
Ulf Samuelsson

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Marek Vasut]

On 2/12/23 23:07, Ulf Samuelsson wrote:
> Den 2023-02-12 kl. 21:01, skrev Marek Vasut:
>  > On 2/12/23 20:52, Ulf Samuelsson wrote:
>  >>
>  >>
>  >> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
>  >>  > On 2/11/23 11:07, u-boot@emagii.com wrote:
>  >>  >
>  >>  > [...]
>  >>  >
>  >>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, size_t 
> len,
>  >>  >> +                 size_t *retlen, const u_char *buf)
>  >>  >> +{
>  >>  >> +    struct udevice *dev = mtd->dev;
>  >>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
>  >>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
>  >>  >> +    int ret;
>  >>  >
>  >>  > Do I read this right, that the 'write' callback is the only one 
> doing
>  >>  > meaningful work, all the other callbacks are just empty stubs ?
>  >>  > Yes, you cannot read back the configuration data.
>  >
>  > That makes it look like any framework which supports "write" callback 
> would be suitable, not just MTD framework.
>  >
>  >>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
>  >>  > implementation already present in U-Boot for Altera Cyclone II 
> FPGA ,
>  >>  > with Cyclone 10 FPGA support ? I believe the PS protocol changed 
> very
>  >>  > little.
>  >> Since the MTD command set is enough to configure the FPGA, the FPGA 
> commands can be removed from the build. The FPGA command set requires 
> you to supply addresses, but the MTD command set uses devices.
>  >>
>  >> So:
>  >> * Smaller U-Boot image
>  >
>  > The MTD framework is rather large, compared to the trivial FPGA 
> framework. Can you back this claim with any numbers ?
> 
> I am assuming that the MTD framework is needed anyway.

FPGA and MTD support is orthogonal, you cannot make that assumption.
Consider SoC-FPGA machine booting from eMMC, Altera Cyclone V SoC does 
support that mode of operation, and MTD support can be disabled.

> We certainly need it.
>  >
>  >> * Simplified user interface.
>  >
>  > If I am to select between 'fpga load' and 'mtd write' for FPGA 
> bitstream loading , my obvious choice would be 'fpga load' . How is 
> using 'mtd write' any better or simpler ?
>  >
> fpga load 0   ${loadaddr} ${filesize}
> mtd write spy ${loadaddr}
> 
> The questions I ask myself.
> So is "0" the "spy" FPGA or the "spx" FPGA?

You can use DT /aliases node to enumerate the FPGAs the same way i2c 
busses, SPI NORs, SD/MMC devices etc. are enumerated . Also have a look 
at e.g. 'net list' command, similar functionality can be added to the 
FPGA command to list all registered FPGAs.

> Why should I have to remember the size of the FPGA bitstream?

Because it is not always possible to extract that information from the 
bitstream blob, remember, some of those blobs may be just raw binaries 
of the SRAM/flash content .

>  >> * The FPGA is an SPI peripheral, so why not add it to the SPI part 
> of the device tree?
>  >
>  > You can add the device into DT and still operate it using the U-Boot 
> FPGA framework, just add the DT support. Why not do it that way ?
> 
> I don't think you can add the device into DT in U-Boot as it is today.
> You can create FPGA contents and add that to the device tree, but not
> the configuration itself. At least, I have not seen it.
> If I have missed it, where is an example?

Have a look at the Linux FPGA DT bindings in 
Documentation/devicetree/bindings/fpga/ . You can implement parsing of 
those bindings into the U-Boot FPGA framework and then add your FPGA 
device configuration interface into the DT.

> The FPGA framework is not really setup to use device-tree to describe
> configuration.
> 
> Only 5 defconfigs in U-Boot uses the FPGA framework.
> * astro_mcf5373l_defconfig
> * syzygy_hub_defconfig
> * xilinx_zynqmp_virt_defconfig
> * xilinx_zynq_virt_defconfig

These two ^ cover very much every zynq 7000 and zynqmp device in 
existence, since the way those are used is in combination with provided 
custom board DT.

> * bitmain_antminer_s9_defconfig

There is also arch/arm/mach-socfpga/Kconfig: imply FPGA_SOCFPGA , which 
activates the CONFIG_FPGA on all of Altera Cyclone V/Arria V/Stratix 
10/Agilex and whatever new SoCFPGA Intel has.

> Their device trees have leafs for configuration:
> * compatible = "fpga-region";
> * compatible = "xlnx,zynq-devcfg-1.0";
> Neither "fpga-region" or "xlnx,zynq-devcfg-1.0" have any compatible 
> drivers, so there is really no support for configuration based on 
> device-tree at the moment.

That's correct

> If I look at boards using the Altera, no board uses a driver
> to configure the FPGA. Instead they implement a number of callbacks
> in the board files which manually handles the SPI bus outside the SPI 
> driver. No trace of device tree.

Which boards do you refer to ?

>  > Let me be blunt about this, I have this feeling that what is 
> happening here is just overloading of MTD framework with unrelated 
> functionality (FPGA bitstream loading). MTD framework simply is not the 
> right place, esp. if there is dedicated FPGA framework, with existing 
> Altera PS driver no less.
> 
> When you are configuring an FPGA, you are writing a bitstream
> to an SRAM inside the FPGA. SRAM are memories.
> 
> The MTD framework does exactly what is needed for passive serial.
> I do not see that there is another place to add a device-tree based
> FPGA configuration, so you basically have to replicate the MTD 
> framework, in order to have FPGA configuration in the device tree.

Update the existing FPGA framework to parse the necessary DT bindings.

Please do not overload existing framework (MTD or whatever) with 
unrelated functionality only because it provides the .write callback. 
While this may look like a good idea to cover one very specific use 
case, it would fail once all the other use cases which are already 
covered by the FPGA framework come into picture.

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-12 kl. 23:40, skrev Marek Vasut:
> On 2/12/23 23:07, Ulf Samuelsson wrote:
>> Den 2023-02-12 kl. 21:01, skrev Marek Vasut:
>>  > On 2/12/23 20:52, Ulf Samuelsson wrote:
>>  >>
>>  >>
>>  >> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
>>  >>  > On 2/11/23 11:07, u-boot@emagii.com wrote:
>>  >>  >
>>  >>  > [...]
>>  >>  >
>>  >>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, 
>> size_t len,
>>  >>  >> +                 size_t *retlen, const u_char *buf)
>>  >>  >> +{
>>  >>  >> +    struct udevice *dev = mtd->dev;
>>  >>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
>>  >>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
>>  >>  >> +    int ret;
>>  >>  >
>>  >>  > Do I read this right, that the 'write' callback is the only one 
>> doing
>>  >>  > meaningful work, all the other callbacks are just empty stubs ?
>>  >>  > Yes, you cannot read back the configuration data.
>>  >
>>  > That makes it look like any framework which supports "write" 
>> callback would be suitable, not just MTD framework.
>>  >
>>  >>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
>>  >>  > implementation already present in U-Boot for Altera Cyclone II 
>> FPGA ,
>>  >>  > with Cyclone 10 FPGA support ? I believe the PS protocol 
>> changed very
>>  >>  > little.
>>  >> Since the MTD command set is enough to configure the FPGA, the 
>> FPGA commands can be removed from the build. The FPGA command set 
>> requires you to supply addresses, but the MTD command set uses devices.
>>  >>
>>  >> So:
>>  >> * Smaller U-Boot image
>>  >
>>  > The MTD framework is rather large, compared to the trivial FPGA 
>> framework. Can you back this claim with any numbers ?
>>
>> I am assuming that the MTD framework is needed anyway.
> 
> FPGA and MTD support is orthogonal, you cannot make that assumption.
> Consider SoC-FPGA machine booting from eMMC, Altera Cyclone V SoC does 
> support that mode of operation, and MTD support can be disabled.

If I look at Altera SOCFPGAs, I find that 21 defconfigs have MTD support 
and 3 do not.
* socfpga_agilex_defconfig
* socfpga_chameleonv3_defconfig
* socfpga_n5x_defconfig
so in most cases there would be a reduction in code size.

>> We certainly need it.
>>  >
>>  >> * Simplified user interface.
>>  >
>>  > If I am to select between 'fpga load' and 'mtd write' for FPGA 
>> bitstream loading , my obvious choice would be 'fpga load' . How is 
>> using 'mtd write' any better or simpler ?
>>  >
>> fpga load 0   ${loadaddr} ${filesize}
>> mtd write spy ${loadaddr}
>>
>> The questions I ask myself.
>> So is "0" the "spy" FPGA or the "spx" FPGA?
> 
> You can use DT /aliases node to enumerate the FPGAs the same way i2c 
> busses, SPI NORs, SD/MMC devices etc. are enumerated . Also have a look 
> at e.g. 'net list' command, similar functionality can be added to the 
> FPGA command to list all registered FPGAs.
> 
>> Why should I have to remember the size of the FPGA bitstream?
> 
> Because it is not always possible to extract that information from the 
> bitstream blob, remember, some of those blobs may be just raw binaries 
> of the SRAM/flash content .
> 
The size of the FPGA will be in the devicetree, so there is no need
to supply that in the command itself.

>>  >> * The FPGA is an SPI peripheral, so why not add it to the SPI part 
>> of the device tree?
>>  >
>>  > You can add the device into DT and still operate it using the 
>> U-Boot FPGA framework, just add the DT support. Why not do it that way ?
>>

>> I don't think you can add the device into DT in U-Boot as it is today.
>> You can create FPGA contents and add that to the device tree, but not
>> the configuration itself. At least, I have not seen it.
>> If I have missed it, where is an example?
> 
> Have a look at the Linux FPGA DT bindings in 
> Documentation/devicetree/bindings/fpga/ . You can implement parsing of 
> those bindings into the U-Boot FPGA framework and then add your FPGA 
> device configuration interface into the DT.
> 

The "altr,fpga-passive-serial" driver is very close to what I did.
My driver supports a superset of a devicetree for that driver.
It does not look like U-Boot is setup to use that.

I feel that the FPGA manager needs a total rework to make this work
and I cannot justify that. If someone adds devicetree support to the 
FPGA manager, I will certainly look to adopt to it.

>> The FPGA framework is not really setup to use device-tree to describe
>> configuration.
>>
>> Only 5 defconfigs in U-Boot uses the FPGA framework.
>> * astro_mcf5373l_defconfig
>> * syzygy_hub_defconfig
>> * xilinx_zynqmp_virt_defconfig
>> * xilinx_zynq_virt_defconfig
> 
> These two ^ cover very much every zynq 7000 and zynqmp device in 
> existence, since the way those are used is in combination with provided 
> custom board DT.
> 
>> * bitmain_antminer_s9_defconfig
> 
> There is also arch/arm/mach-socfpga/Kconfig: imply FPGA_SOCFPGA , which 
> activates the CONFIG_FPGA on all of Altera Cyclone V/Arria V/Stratix 
> 10/Agilex and whatever new SoCFPGA Intel has.
> 
>> Their device trees have leafs for configuration:
>> * compatible = "fpga-region";
>> * compatible = "xlnx,zynq-devcfg-1.0";
>> Neither "fpga-region" or "xlnx,zynq-devcfg-1.0" have any compatible 
>> drivers, so there is really no support for configuration based on 
>> device-tree at the moment.
> 
> That's correct
> 
>> If I look at boards using the Altera, no board uses a driver
>> to configure the FPGA. Instead they implement a number of callbacks
>> in the board files which manually handles the SPI bus outside the SPI 
>> driver. No trace of device tree.
> 
> Which boards do you refer to ?
> 
Every board using the Altera drivers have an Altera_desc which contains 
links to callbacks used by the altera driver.

cls; rgrep Altera_desc | grep -v -e drivers -e include
board/beckhoff/mx53cx9020/mx53cx9020.c:static Altera_desc ccat_fpga = {
board/astro/mcf5373l/fpga.c:Altera_desc altera_fpga[FPGA_COUNT] = {
board/theadorable/fpga.c:static Altera_desc altera_fpga[] = {

They all access the FPGA using spi, gpio etc. outside the driver model.

Not a trace of devicetree support unfortunately.


>>  > Let me be blunt about this, I have this feeling that what is 
>> happening here is just overloading of MTD framework with unrelated 
>> functionality (FPGA bitstream loading). MTD framework simply is not 
>> the right place, esp. if there is dedicated FPGA framework, with 
>> existing Altera PS driver no less.
>>
>> When you are configuring an FPGA, you are writing a bitstream
>> to an SRAM inside the FPGA. SRAM are memories.
>>
>> The MTD framework does exactly what is needed for passive serial.
>> I do not see that there is another place to add a device-tree based
>> FPGA configuration, so you basically have to replicate the MTD 
>> framework, in order to have FPGA configuration in the device tree.
> 
> Update the existing FPGA framework to parse the necessary DT bindings.
> 


> Please do not overload existing framework (MTD or whatever) with 
> unrelated functionality only because it provides the .write callback. 
> While this may look like a good idea to cover one very specific use 
> case, it would fail once all the other use cases which are already 
> covered by the FPGA framework come into picture.

I do not see why other chips could not be supported by an MTD driver.
Conceptually, there is not much difference between reading/writing a
NAND flash and configuring an FPGA.

Obviously, someone would have to take the time to do it, but it is 
certainly possible.

Best Regards
Ulf Samuelsson

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Marek Vasut]

On 2/13/23 10:30, Ulf Samuelsson wrote:
> 
> 
> Den 2023-02-12 kl. 23:40, skrev Marek Vasut:
>> On 2/12/23 23:07, Ulf Samuelsson wrote:
>>> Den 2023-02-12 kl. 21:01, skrev Marek Vasut:
>>>  > On 2/12/23 20:52, Ulf Samuelsson wrote:
>>>  >>
>>>  >>
>>>  >> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
>>>  >>  > On 2/11/23 11:07, u-boot@emagii.com wrote:
>>>  >>  >
>>>  >>  > [...]
>>>  >>  >
>>>  >>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, 
>>> size_t len,
>>>  >>  >> +                 size_t *retlen, const u_char *buf)
>>>  >>  >> +{
>>>  >>  >> +    struct udevice *dev = mtd->dev;
>>>  >>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
>>>  >>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
>>>  >>  >> +    int ret;
>>>  >>  >
>>>  >>  > Do I read this right, that the 'write' callback is the only 
>>> one doing
>>>  >>  > meaningful work, all the other callbacks are just empty stubs ?
>>>  >>  > Yes, you cannot read back the configuration data.
>>>  >
>>>  > That makes it look like any framework which supports "write" 
>>> callback would be suitable, not just MTD framework.
>>>  >
>>>  >>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
>>>  >>  > implementation already present in U-Boot for Altera Cyclone II 
>>> FPGA ,
>>>  >>  > with Cyclone 10 FPGA support ? I believe the PS protocol 
>>> changed very
>>>  >>  > little.
>>>  >> Since the MTD command set is enough to configure the FPGA, the 
>>> FPGA commands can be removed from the build. The FPGA command set 
>>> requires you to supply addresses, but the MTD command set uses devices.
>>>  >>
>>>  >> So:
>>>  >> * Smaller U-Boot image
>>>  >
>>>  > The MTD framework is rather large, compared to the trivial FPGA 
>>> framework. Can you back this claim with any numbers ?
>>>
>>> I am assuming that the MTD framework is needed anyway.
>>
>> FPGA and MTD support is orthogonal, you cannot make that assumption.
>> Consider SoC-FPGA machine booting from eMMC, Altera Cyclone V SoC does 
>> support that mode of operation, and MTD support can be disabled.
> 
> If I look at Altera SOCFPGAs, I find that 21 defconfigs have MTD support 
> and 3 do not.
> * socfpga_agilex_defconfig
> * socfpga_chameleonv3_defconfig
> * socfpga_n5x_defconfig
> so in most cases there would be a reduction in code size.

All these machines already have FPGA framework enabled, just use it or 
extend it.

>>> We certainly need it.
>>>  >
>>>  >> * Simplified user interface.
>>>  >
>>>  > If I am to select between 'fpga load' and 'mtd write' for FPGA 
>>> bitstream loading , my obvious choice would be 'fpga load' . How is 
>>> using 'mtd write' any better or simpler ?
>>>  >
>>> fpga load 0   ${loadaddr} ${filesize}
>>> mtd write spy ${loadaddr}
>>>
>>> The questions I ask myself.
>>> So is "0" the "spy" FPGA or the "spx" FPGA?
>>
>> You can use DT /aliases node to enumerate the FPGAs the same way i2c 
>> busses, SPI NORs, SD/MMC devices etc. are enumerated . Also have a 
>> look at e.g. 'net list' command, similar functionality can be added to 
>> the FPGA command to list all registered FPGAs.
>>
>>> Why should I have to remember the size of the FPGA bitstream?
>>
>> Because it is not always possible to extract that information from the 
>> bitstream blob, remember, some of those blobs may be just raw binaries 
>> of the SRAM/flash content .
>>
> The size of the FPGA will be in the devicetree, so there is no need
> to supply that in the command itself.

That stops working once you start dealing with partial reconfiguration 
and the bitstream size varies. Then it is back to $filesize .

>>>  >> * The FPGA is an SPI peripheral, so why not add it to the SPI 
>>> part of the device tree?
>>>  >
>>>  > You can add the device into DT and still operate it using the 
>>> U-Boot FPGA framework, just add the DT support. Why not do it that way ?
>>>
> 
>>> I don't think you can add the device into DT in U-Boot as it is today.
>>> You can create FPGA contents and add that to the device tree, but not
>>> the configuration itself. At least, I have not seen it.
>>> If I have missed it, where is an example?
>>
>> Have a look at the Linux FPGA DT bindings in 
>> Documentation/devicetree/bindings/fpga/ . You can implement parsing of 
>> those bindings into the U-Boot FPGA framework and then add your FPGA 
>> device configuration interface into the DT.
>>
> 
> The "altr,fpga-passive-serial" driver is very close to what I did.

If there is an existing driver, extend it, rework it, but please don't 
introduce new driver that does the same thing.

> My driver supports a superset of a devicetree for that driver.
> It does not look like U-Boot is setup to use that.
> 
> I feel that the FPGA manager needs a total rework to make this work
> and I cannot justify that. If someone adds devicetree support to the 
> FPGA manager, I will certainly look to adopt to it.

Overloading the MTD subsystem only because it has a .write() callback is 
not the way to go either.

>>> The FPGA framework is not really setup to use device-tree to describe
>>> configuration.
>>>
>>> Only 5 defconfigs in U-Boot uses the FPGA framework.
>>> * astro_mcf5373l_defconfig
>>> * syzygy_hub_defconfig
>>> * xilinx_zynqmp_virt_defconfig
>>> * xilinx_zynq_virt_defconfig
>>
>> These two ^ cover very much every zynq 7000 and zynqmp device in 
>> existence, since the way those are used is in combination with 
>> provided custom board DT.
>>
>>> * bitmain_antminer_s9_defconfig
>>
>> There is also arch/arm/mach-socfpga/Kconfig: imply FPGA_SOCFPGA , 
>> which activates the CONFIG_FPGA on all of Altera Cyclone V/Arria 
>> V/Stratix 10/Agilex and whatever new SoCFPGA Intel has.
>>
>>> Their device trees have leafs for configuration:
>>> * compatible = "fpga-region";
>>> * compatible = "xlnx,zynq-devcfg-1.0";
>>> Neither "fpga-region" or "xlnx,zynq-devcfg-1.0" have any compatible 
>>> drivers, so there is really no support for configuration based on 
>>> device-tree at the moment.
>>
>> That's correct
>>
>>> If I look at boards using the Altera, no board uses a driver
>>> to configure the FPGA. Instead they implement a number of callbacks
>>> in the board files which manually handles the SPI bus outside the SPI 
>>> driver. No trace of device tree.
>>
>> Which boards do you refer to ?
>>
> Every board using the Altera drivers have an Altera_desc which contains 
> links to callbacks used by the altera driver.
> 
> cls; rgrep Altera_desc | grep -v -e drivers -e include
> board/beckhoff/mx53cx9020/mx53cx9020.c:static Altera_desc ccat_fpga = {
> board/astro/mcf5373l/fpga.c:Altera_desc altera_fpga[FPGA_COUNT] = {
> board/theadorable/fpga.c:static Altera_desc altera_fpga[] = {
> 
> They all access the FPGA using spi, gpio etc. outside the driver model.
> 
> Not a trace of devicetree support unfortunately.

DT support would have to be added, but that shouldn't be all that hard. 
There are other simple subsystems which were extended the same way.

>>>  > Let me be blunt about this, I have this feeling that what is 
>>> happening here is just overloading of MTD framework with unrelated 
>>> functionality (FPGA bitstream loading). MTD framework simply is not 
>>> the right place, esp. if there is dedicated FPGA framework, with 
>>> existing Altera PS driver no less.
>>>
>>> When you are configuring an FPGA, you are writing a bitstream
>>> to an SRAM inside the FPGA. SRAM are memories.
>>>
>>> The MTD framework does exactly what is needed for passive serial.
>>> I do not see that there is another place to add a device-tree based
>>> FPGA configuration, so you basically have to replicate the MTD 
>>> framework, in order to have FPGA configuration in the device tree.
>>
>> Update the existing FPGA framework to parse the necessary DT bindings.
>>
> 
> 
>> Please do not overload existing framework (MTD or whatever) with 
>> unrelated functionality only because it provides the .write callback. 
>> While this may look like a good idea to cover one very specific use 
>> case, it would fail once all the other use cases which are already 
>> covered by the FPGA framework come into picture.
> 
> I do not see why other chips could not be supported by an MTD driver.
> Conceptually, there is not much difference between reading/writing a
> NAND flash and configuring an FPGA.

See above.

To sum it up:
- The only part of the MTD subsystem that is in use by this driver is 
the write callback, everything else is unused. That does not make MTD 
subsystem the right tool.
- There is existing FPGA subsystem both in U-Boot and Linux, extend it 
if necessary. That also covers the usecases which go past a simple full 
bitstream upload.

> Obviously, someone would have to take the time to do it, but it is 
> certainly possible.

You could make the exact same argument about making the FPGA driver a 
MISC device, CHAR device, BLOCK device, they all have the write 
callback, but that does not mean it is the right fit.

[...]

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-20 kl. 17:17, skrev Marek Vasut:
> On 2/13/23 10:30, Ulf Samuelsson wrote:
>>
>>
>> Den 2023-02-12 kl. 23:40, skrev Marek Vasut:
>>> On 2/12/23 23:07, Ulf Samuelsson wrote:
>>>> Den 2023-02-12 kl. 21:01, skrev Marek Vasut:
>>>>  > On 2/12/23 20:52, Ulf Samuelsson wrote:
>>>>  >>
>>>>  >>
>>>>  >> Den 2023-02-12 kl. 20:31, skrev Marek Vasut:
>>>>  >>  > On 2/11/23 11:07, u-boot@emagii.com wrote:
>>>>  >>  >
>>>>  >>  > [...]
>>>>  >>  >
>>>>  >>  >> +static int cyc10_write(struct mtd_info *mtd, loff_t to, 
>>>> size_t len,
>>>>  >>  >> +                 size_t *retlen, const u_char *buf)
>>>>  >>  >> +{
>>>>  >>  >> +    struct udevice *dev = mtd->dev;
>>>>  >>  >> +    struct spi_slave *slave = dev_get_parent_priv(dev);
>>>>  >>  >> +    struct cyc10_plat *fpga = dev_get_plat(dev);
>>>>  >>  >> +    int ret;
>>>>  >>  >
>>>>  >>  > Do I read this right, that the 'write' callback is the only 
>>>> one doing
>>>>  >>  > meaningful work, all the other callbacks are just empty stubs ?
>>>>  >>  > Yes, you cannot read back the configuration data.
>>>>  >
>>>>  > That makes it look like any framework which supports "write" 
>>>> callback would be suitable, not just MTD framework.
>>>>  >
>>>>  >>  > Why not update drivers/fpga/cyclon2.c which is Passive Serial
>>>>  >>  > implementation already present in U-Boot for Altera Cyclone 
>>>> II FPGA ,
>>>>  >>  > with Cyclone 10 FPGA support ? I believe the PS protocol 
>>>> changed very
>>>>  >>  > little.
>>>>  >> Since the MTD command set is enough to configure the FPGA, the 
>>>> FPGA commands can be removed from the build. The FPGA command set 
>>>> requires you to supply addresses, but the MTD command set uses devices.
>>>>  >>
>>>>  >> So:
>>>>  >> * Smaller U-Boot image
>>>>  >
>>>>  > The MTD framework is rather large, compared to the trivial FPGA 
>>>> framework. Can you back this claim with any numbers ?
>>>>
>>>> I am assuming that the MTD framework is needed anyway.
>>>
>>> FPGA and MTD support is orthogonal, you cannot make that assumption.
>>> Consider SoC-FPGA machine booting from eMMC, Altera Cyclone V SoC 
>>> does support that mode of operation, and MTD support can be disabled.
>>
>> If I look at Altera SOCFPGAs, I find that 21 defconfigs have MTD 
>> support and 3 do not.
>> * socfpga_agilex_defconfig
>> * socfpga_chameleonv3_defconfig
>> * socfpga_n5x_defconfig
>> so in most cases there would be a reduction in code size.
> 
> All these machines already have FPGA framework enabled, just use it or 
> extend it.

The argument was that some boards have FPGA framework without MTD
and that gives overhead for the those board.
Since 21 out of 24 have MTD, the FPGA framework gives overhead
for those boards, which could removed if FPGAs were
installed as MTDs.

> 
>>>> We certainly need it.
>>>>  >
>>>>  >> * Simplified user interface.
>>>>  >
>>>>  > If I am to select between 'fpga load' and 'mtd write' for FPGA 
>>>> bitstream loading , my obvious choice would be 'fpga load' . How is 
>>>> using 'mtd write' any better or simpler ?
>>>>  >
>>>> fpga load 0   ${loadaddr} ${filesize}
>>>> mtd write spy ${loadaddr}
>>>>
>>>> The questions I ask myself.
>>>> So is "0" the "spy" FPGA or the "spx" FPGA?
>>>
>>> You can use DT /aliases node to enumerate the FPGAs the same way i2c 
>>> busses, SPI NORs, SD/MMC devices etc. are enumerated . Also have a 
>>> look at e.g. 'net list' command, similar functionality can be added 
>>> to the FPGA command to list all registered FPGAs.
>>>
>>>> Why should I have to remember the size of the FPGA bitstream?
>>>
>>> Because it is not always possible to extract that information from 
>>> the bitstream blob, remember, some of those blobs may be just raw 
>>> binaries of the SRAM/flash content .
>>>
>> The size of the FPGA will be in the devicetree, so there is no need
>> to supply that in the command itself.
> 
> That stops working once you start dealing with partial reconfiguration 
> and the bitstream size varies. Then it is back to $filesize .

The MTD subsystem supports the concept of partitions.
I suspect that this would allow partial configuration to work quite OK.

> 
>>>>  >> * The FPGA is an SPI peripheral, so why not add it to the SPI 
>>>> part of the device tree?
>>>>  >
>>>>  > You can add the device into DT and still operate it using the 
>>>> U-Boot FPGA framework, just add the DT support. Why not do it that 
>>>> way ?
>>>>
>>
>>>> I don't think you can add the device into DT in U-Boot as it is today.
>>>> You can create FPGA contents and add that to the device tree, but not
>>>> the configuration itself. At least, I have not seen it.
>>>> If I have missed it, where is an example?
>>>
>>> Have a look at the Linux FPGA DT bindings in 
>>> Documentation/devicetree/bindings/fpga/ . You can implement parsing 
>>> of those bindings into the U-Boot FPGA framework and then add your 
>>> FPGA device configuration interface into the DT.
>>>
>>
>> The "altr,fpga-passive-serial" driver is very close to what I did.
> 
> If there is an existing driver, extend it, rework it, but please don't 
> introduce new driver that does the same thing.
> 
The current passive serial solutions does not use the existing SPI 
drivers. They access the SPI peripheral outside the driver.
A new driver would build on the existing SPI drivers.

It is not possible to test if a modified driver break the function  on 
those boards, if you do not have access to the boards.

>> My driver supports a superset of a devicetree for that driver.
>> It does not look like U-Boot is setup to use that.
>>
>> I feel that the FPGA manager needs a total rework to make this work
>> and I cannot justify that. If someone adds devicetree support to the 
>> FPGA manager, I will certainly look to adopt to it.
> 
> Overloading the MTD subsystem only because it has a .write() callback is 
> not the way to go either.
> 
>>>> The FPGA framework is not really setup to use device-tree to describe
>>>> configuration.
>>>>
>>>> Only 5 defconfigs in U-Boot uses the FPGA framework.
>>>> * astro_mcf5373l_defconfig
>>>> * syzygy_hub_defconfig
>>>> * xilinx_zynqmp_virt_defconfig
>>>> * xilinx_zynq_virt_defconfig
>>>
>>> These two ^ cover very much every zynq 7000 and zynqmp device in 
>>> existence, since the way those are used is in combination with 
>>> provided custom board DT.
>>>
>>>> * bitmain_antminer_s9_defconfig
>>>
>>> There is also arch/arm/mach-socfpga/Kconfig: imply FPGA_SOCFPGA , 
>>> which activates the CONFIG_FPGA on all of Altera Cyclone V/Arria 
>>> V/Stratix 10/Agilex and whatever new SoCFPGA Intel has.
>>>
>>>> Their device trees have leafs for configuration:
>>>> * compatible = "fpga-region";
>>>> * compatible = "xlnx,zynq-devcfg-1.0";
>>>> Neither "fpga-region" or "xlnx,zynq-devcfg-1.0" have any compatible 
>>>> drivers, so there is really no support for configuration based on 
>>>> device-tree at the moment.
>>>
>>> That's correct
>>>
>>>> If I look at boards using the Altera, no board uses a driver
>>>> to configure the FPGA. Instead they implement a number of callbacks
>>>> in the board files which manually handles the SPI bus outside the 
>>>> SPI driver. No trace of device tree.
>>>
>>> Which boards do you refer to ?
>>>
>> Every board using the Altera drivers have an Altera_desc which 
>> contains links to callbacks used by the altera driver.
>>
>> cls; rgrep Altera_desc | grep -v -e drivers -e include
>> board/beckhoff/mx53cx9020/mx53cx9020.c:static Altera_desc ccat_fpga = {
>> board/astro/mcf5373l/fpga.c:Altera_desc altera_fpga[FPGA_COUNT] = {
>> board/theadorable/fpga.c:static Altera_desc altera_fpga[] = {
>>
>> They all access the FPGA using spi, gpio etc. outside the driver model.
>>
>> Not a trace of devicetree support unfortunately.
> 
> DT support would have to be added, but that shouldn't be all that hard. 
> There are other simple subsystems which were extended the same way.
>
As I said, I cannot justify to do that change.

>>>>  > Let me be blunt about this, I have this feeling that what is 
>>>> happening here is just overloading of MTD framework with unrelated 
>>>> functionality (FPGA bitstream loading). MTD framework simply is not 
>>>> the right place, esp. if there is dedicated FPGA framework, with 
>>>> existing Altera PS driver no less.
>>>>
>>>> When you are configuring an FPGA, you are writing a bitstream
>>>> to an SRAM inside the FPGA. SRAM are memories.
>>>>
>>>> The MTD framework does exactly what is needed for passive serial.
>>>> I do not see that there is another place to add a device-tree based
>>>> FPGA configuration, so you basically have to replicate the MTD 
>>>> framework, in order to have FPGA configuration in the device tree.
>>>
>>> Update the existing FPGA framework to parse the necessary DT bindings.
>>>
>>
>>
>>> Please do not overload existing framework (MTD or whatever) with 
>>> unrelated functionality only because it provides the .write callback. 
>>> While this may look like a good idea to cover one very specific use 
>>> case, it would fail once all the other use cases which are already 
>>> covered by the FPGA framework come into picture.
>>
>> I do not see why other chips could not be supported by an MTD driver.
>> Conceptually, there is not much difference between reading/writing a
>> NAND flash and configuring an FPGA.
> 
> See above.
> 
> To sum it up:
> - The only part of the MTD subsystem that is in use by this driver is 
> the write callback, everything else is unused. That does not make MTD 
> subsystem the right tool.

No, the MTD subsystem is compatible with the device tree.
You can also get information about the FPGA when you list the mtd devices.
It allows you to support partial reconfiguration through the partition 
feature. You can write any size block to any part of the MTD, and you
can express that in a device tree. That is what is needed.

> - There is existing FPGA subsystem both in U-Boot and Linux, extend it 
> if necessary. That also covers the usecases which go past a simple full 
> bitstream upload.

And for use, that introduces a command set, which we do not need
and adds 50-100kB to the image.

> 
>> Obviously, someone would have to take the time to do it, but it is 
>> certainly possible.
> 
> You could make the exact same argument about making the FPGA driver a 
> MISC device, CHAR device, BLOCK device, they all have the write 
> callback, but that does not mean it is the right fit.
> 
That is because you see the "write" as the only function.

> [...]

What would work is to keep the driver as is, replacing
the MTD stuff internally with an fpga_add after generating a struct 
based on the device tree.

Then it would be part of the FPGA subsystem.
To add this as an Altera passive serial is high risk,
since it cannot be tested on boards using that.
It would simplify things a lot to add it as another driver.

The result would of course be something which adds both complexity and 
code size for us, without any benefit.


Best Regards
Ulf Samuelsson

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Marek Vasut]

On 2/20/23 22:29, Ulf Samuelsson wrote:

[...]

>> To sum it up:
>> - The only part of the MTD subsystem that is in use by this driver is 
>> the write callback, everything else is unused. That does not make MTD 
>> subsystem the right tool.
> 
> No, the MTD subsystem is compatible with the device tree.

That does not justify overloading the subsystem with unrelated hardware 
support for which there is an existing subsystem with existing drivers 
for the functionality you would duplicate in the MTD subsystem. There 
are existing DT bindings for FPGAs, see Linux 
Documentation/devicetree/bindings/fpga/ , use those and add support for 
them into the FPGA subsystem if needed .

> You can also get information about the FPGA when you list the mtd devices.

No, you cannot, because the information you may get out of the FPGA in 
some cases does not map to MTD devices .

> It allows you to support partial reconfiguration through the partition 
> feature. You can write any size block to any part of the MTD, and you
> can express that in a device tree. That is what is needed.

I am afraid it is not as simple as that, see Linux 
Documentation/devicetree/bindings/fpga/fpga-region.txt . Furthermore, 
those DT bindings are already part of Linux and Linux expects the DT to 
contain exactly those bindings, U-Boot tries not to diverge from what 
Linux does in DT.

>> - There is existing FPGA subsystem both in U-Boot and Linux, extend it 
>> if necessary. That also covers the usecases which go past a simple 
>> full bitstream upload.
> 
> And for use, that introduces a command set, which we do not need
> and adds 50-100kB to the image.

If you were to turn everything that needs read/write callback into an 
MTD device, you would be able to save even more space, but that does not 
make it right. You could try and reduce the FPGA command size if there 
is code which is unused for some usecases, that can be selected via 
Kconfig at build time.

>>> Obviously, someone would have to take the time to do it, but it is 
>>> certainly possible.
>>
>> You could make the exact same argument about making the FPGA driver a 
>> MISC device, CHAR device, BLOCK device, they all have the write 
>> callback, but that does not mean it is the right fit.
>>
> That is because you see the "write" as the only function.

Yes, because that is the only callback this patch implements, the rest 
are empty.

>> [...]
> 
> What would work is to keep the driver as is, replacing
> the MTD stuff internally with an fpga_add after generating a struct 
> based on the device tree.
> 
> Then it would be part of the FPGA subsystem.
> To add this as an Altera passive serial is high risk,
> since it cannot be tested on boards using that.
> It would simplify things a lot to add it as another driver.

No, that would make things MUCH worse, because we would have a one-off 
driver used by one system, therefore poorly tested, which duplicates 
functionality of existing driver used by multiple systems and better 
tested. This also increases maintenance burden.

One single driver for this common functionality is always better as it 
gets more testing.

> The result would of course be something which adds both complexity and 
> code size for us, without any benefit.

Please also consider the MTD maintainer point of view.

[...]

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-20 kl. 23:34, skrev Marek Vasut:
> On 2/20/23 22:29, Ulf Samuelsson wrote:
> 
> [...]
> 
>>> To sum it up:
>>> - The only part of the MTD subsystem that is in use by this driver is 
>>> the write callback, everything else is unused. That does not make MTD 
>>> subsystem the right tool.
>>
>> No, the MTD subsystem is compatible with the device tree.
> 
> That does not justify overloading the subsystem with unrelated hardware 
> support for which there is an existing subsystem with existing drivers 
> for the functionality you would duplicate in the MTD subsystem. There 
> are existing DT bindings for FPGAs, see Linux 
> Documentation/devicetree/bindings/fpga/ , use those and add support for 
> them into the FPGA subsystem if needed .


The current u-boot FPGA support is incompatible with the linux 
devicetree for FPGAs.
There is simply nothing there.
That is the reason for the proposal in the first place.


> 
>> You can also get information about the FPGA when you list the mtd 
>> devices.
> 
> No, you cannot, because the information you may get out of the FPGA in 
> some cases does not map to MTD devices .

Please give some examples.

>> It allows you to support partial reconfiguration through the partition 
>> feature. You can write any size block to any part of the MTD, and you
>> can express that in a device tree. That is what is needed.
> 
> I am afraid it is not as simple as that, see Linux 
> Documentation/devicetree/bindings/fpga/fpga-region.txt . Furthermore, 
> those DT bindings are already part of Linux and Linux expects the DT to 
> contain exactly those bindings, U-Boot tries not to diverge from what 
> Linux does in DT.
> 

The device tree in my driver is a superset to what linux does.
It tests more status pins, and provides a name for the FPGA.
It would not be difficult to make it 100% compatible.


>>> - There is existing FPGA subsystem both in U-Boot and Linux, extend 
>>> it if necessary. That also covers the usecases which go past a simple 
>>> full bitstream upload.
>>
>> And for use, that introduces a command set, which we do not need
>> and adds 50-100kB to the image.
> 
> If you were to turn everything that needs read/write callback into an 
> MTD device, you would be able to save even more space, but that does not 
> make it right. You could try and reduce the FPGA command size if there 
> is code which is unused for some usecases, that can be selected via 
> Kconfig at build time.
> 

 From my point of view, Having the FPGA command set duplicates the 
functionality of the MTD.
That is the major contributor to code space expansion.

The thing you are writing to is SRAM which is memory.
If it is right or wrong to use that as an MTD is a matter of opinion.


>>>> Obviously, someone would have to take the time to do it, but it is 
>>>> certainly possible.
>>>
>>> You could make the exact same argument about making the FPGA driver a 
>>> MISC device, CHAR device, BLOCK device, they all have the write 
>>> callback, but that does not mean it is the right fit.
>>>
>> That is because you see the "write" as the only function.
> 
> Yes, because that is the only callback this patch implements, the rest 
> are empty.

You cannot read back the configuration from a Cyclone 10.
A driver for an FPGA where configuration could be read back of course 
would have a read routine as well.
The MTD subsystem can display the information the driver provides.
That is an important piece that would be missing from a MISC or CHAR driver.
You also have a "device" to work with.

> 
>>> [...]
>>
>> What would work is to keep the driver as is, replacing
>> the MTD stuff internally with an fpga_add after generating a struct 
>> based on the device tree.
>>
>> Then it would be part of the FPGA subsystem.
>> To add this as an Altera passive serial is high risk,
>> since it cannot be tested on boards using that.
>> It would simplify things a lot to add it as another driver.
> 
> No, that would make things MUCH worse, because we would have a one-off 
> driver used by one system, therefore poorly tested, which duplicates 
> functionality of existing driver used by multiple systems and better 
> tested. This also increases maintenance burden.


Today you have a custom functionality to access the FPGA in each board 
package.  That only gets tested when building that board.
So if you have 'n' boards, you have 'n' different ways of accessing the 
FPGA. Each poorly tested, if number of users are the measurement.
The only thing in common is the multiplexing code.

Anyone that designs a new system, would have to choose between
using the new driver, or write their own access method.
They cannot use what is in u-boot today, because it is in the board 
directory. Even if they have the same chip as a board with existing 
support, they cannot add it in, since it is in the board directory.
Only solution is to duplicate the code in their own board directory,
but the likelyhood of using the same CPU is slim.

Are you happy with this?

I wager that anyone wanting to use passive serial would adopt the new 
driver.

One solution is of course to ask the maintainers of those boards
if they would consider moving to a device tree solution.
Then the new driver would be the only driver.


> One single driver for this common functionality is always better as it 
> gets more testing.
> 
>> The result would of course be something which adds both complexity and 
>> code size for us, without any benefit.
> 
> Please also consider the MTD maintainer point of view.
> 
I do not see that this adds much more complexity than any other MTD device.

> [...]
Best Regards
Ulf Samuelsson

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Marek Vasut]

On 2/21/23 00:47, Ulf Samuelsson wrote:
> 
> 
> Den 2023-02-20 kl. 23:34, skrev Marek Vasut:
>> On 2/20/23 22:29, Ulf Samuelsson wrote:
>>
>> [...]
>>
>>>> To sum it up:
>>>> - The only part of the MTD subsystem that is in use by this driver 
>>>> is the write callback, everything else is unused. That does not make 
>>>> MTD subsystem the right tool.
>>>
>>> No, the MTD subsystem is compatible with the device tree.
>>
>> That does not justify overloading the subsystem with unrelated 
>> hardware support for which there is an existing subsystem with 
>> existing drivers for the functionality you would duplicate in the MTD 
>> subsystem. There are existing DT bindings for FPGAs, see Linux 
>> Documentation/devicetree/bindings/fpga/ , use those and add support 
>> for them into the FPGA subsystem if needed .
> 
> 
> The current u-boot FPGA support is incompatible with the linux 
> devicetree for FPGAs.
> There is simply nothing there.
> That is the reason for the proposal in the first place.

Add it, that seems like the best approach here.

>>> You can also get information about the FPGA when you list the mtd 
>>> devices.
>>
>> No, you cannot, because the information you may get out of the FPGA in 
>> some cases does not map to MTD devices .
> 
> Please give some examples.

Which FPGA bus bridge(s) are enabled for example . This is something 
present on SoCFPGA solutions , e.g. an AXI bridge into the fabric .

>>> It allows you to support partial reconfiguration through the 
>>> partition feature. You can write any size block to any part of the 
>>> MTD, and you
>>> can express that in a device tree. That is what is needed.
>>
>> I am afraid it is not as simple as that, see Linux 
>> Documentation/devicetree/bindings/fpga/fpga-region.txt . Furthermore, 
>> those DT bindings are already part of Linux and Linux expects the DT 
>> to contain exactly those bindings, U-Boot tries not to diverge from 
>> what Linux does in DT.
>>
> 
> The device tree in my driver is a superset to what linux does.
> It tests more status pins, and provides a name for the FPGA.
> It would not be difficult to make it 100% compatible.

Extend the Linux DT bindings if they are insufficient, but please don't 
try and reinvent the wheel.

>>>> - There is existing FPGA subsystem both in U-Boot and Linux, extend 
>>>> it if necessary. That also covers the usecases which go past a 
>>>> simple full bitstream upload.
>>>
>>> And for use, that introduces a command set, which we do not need
>>> and adds 50-100kB to the image.
>>
>> If you were to turn everything that needs read/write callback into an 
>> MTD device, you would be able to save even more space, but that does 
>> not make it right. You could try and reduce the FPGA command size if 
>> there is code which is unused for some usecases, that can be selected 
>> via Kconfig at build time.
>>
> 
>  From my point of view, Having the FPGA command set duplicates the 
> functionality of the MTD.
> That is the major contributor to code space expansion.
> 
> The thing you are writing to is SRAM which is memory.

There are flash based FPGAs , like Altera MAXV.
There are CPLDs which are not even flash based.

> If it is right or wrong to use that as an MTD is a matter of opinion.

I am still hoping the MTD maintainer would provide input here.

>>>>> Obviously, someone would have to take the time to do it, but it is 
>>>>> certainly possible.
>>>>
>>>> You could make the exact same argument about making the FPGA driver 
>>>> a MISC device, CHAR device, BLOCK device, they all have the write 
>>>> callback, but that does not mean it is the right fit.
>>>>
>>> That is because you see the "write" as the only function.
>>
>> Yes, because that is the only callback this patch implements, the rest 
>> are empty.
> 
> You cannot read back the configuration from a Cyclone 10.

Therefore, the entire read part of whatever subsystem is unnecessary.

Erase as well for most FPGAs which are SRAM based, even for flash based 
FPGA, per-sector erase is not available, since there is no concept of 
sectors or erase blocks at all.

> A driver for an FPGA where configuration could be read back of course 
> would have a read routine as well.
> The MTD subsystem can display the information the driver provides.
> That is an important piece that would be missing from a MISC or CHAR 
> driver.
> You also have a "device" to work with.

All this is already in the FPGA subsystem, and it is designed toward the 
FPGA use case.

>>>> [...]
>>>
>>> What would work is to keep the driver as is, replacing
>>> the MTD stuff internally with an fpga_add after generating a struct 
>>> based on the device tree.
>>>
>>> Then it would be part of the FPGA subsystem.
>>> To add this as an Altera passive serial is high risk,
>>> since it cannot be tested on boards using that.
>>> It would simplify things a lot to add it as another driver.
>>
>> No, that would make things MUCH worse, because we would have a one-off 
>> driver used by one system, therefore poorly tested, which duplicates 
>> functionality of existing driver used by multiple systems and better 
>> tested. This also increases maintenance burden.
> 
> 
> Today you have a custom functionality to access the FPGA in each board 
> package.  That only gets tested when building that board.

No, at least the Altera SoCFPGA and Xilinx anything newer than 
Zynq/ZynqMP is tested regularly on actual hardware, and the code is 
common to all those systems, and lives in drivers/fpga/ .

> So if you have 'n' boards, you have 'n' different ways of accessing the 
> FPGA. Each poorly tested, if number of users are the measurement.
> The only thing in common is the multiplexing code.

Not really, on SoCFPGA systems, even the programming logic is common. 
For external FPGAs, sure, you need to provide the pin or interface 
mapping in some way, via platform data, DT, ACPI, whatever. If that is 
platform data, it should be moved to DT, the FPGA framework DT bindings 
should provide guidance how to do that. But even then, the driver itself 
should be generic, and reside in drivers/fpga/ .

> Anyone that designs a new system, would have to choose between
> using the new driver, or write their own access method.

I vote "reuse existing driver, plug in DT/platform_data/... as needed" .

> They cannot use what is in u-boot today, because it is in the board 
> directory.

This is not true for majority of FPGA systems, see drivers/fpga/ .

> Even if they have the same chip as a board with existing 
> support, they cannot add it in, since it is in the board directory.

Are you actually talking about platform data which are used during 
driver instantiation, or, full driver, in board/ ?

Platform data are fine-ish, full drivers in board/ should be on the way out.

> Only solution is to duplicate the code in their own board directory,
> but the likelyhood of using the same CPU is slim.
> 
> Are you happy with this?

I think we have a vastly different picture of the current situation .

> I wager that anyone wanting to use passive serial would adopt the new 
> driver.
> 
> One solution is of course to ask the maintainers of those boards
> if they would consider moving to a device tree solution.
> Then the new driver would be the only driver.

Put them on CC, there is scripts/get_maintainer.pl script, use -f 
parameter to point it to specific files. I have a feeling a lot of those 
boards with external FPGAs being configured by poking registers in 
board/ are going to be dropped sooner rather than later.

>> One single driver for this common functionality is always better as it 
>> gets more testing.
>>
>>> The result would of course be something which adds both complexity 
>>> and code size for us, without any benefit.
>>
>> Please also consider the MTD maintainer point of view.
>>
> I do not see that this adds much more complexity than any other MTD device.

Again, there is a separate subsystem/framework for FPGAs . I disagree 
one should conflate MTD and FPGA subsystems.

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Michael Walle]

>> If it is right or wrong to use that as an MTD is a matter of opinion.
> 
> I am still hoping the MTD maintainer would provide input here.

I might be missing something, but what is the reasoning here, to add this
to the mtd subsystem? One is saving space, but I agree with Marek, this
isn't a valid argument to just put any (unrelated) stuff into the MTD
subsystem.

Also, as Marek pointed out, there are many different 'programming'
solutions for CPLDs/FPGAs and most of them don't share anything with
MTD. You seem to be just focusing on the "passive serial" one.

Now, I saw you mentioned that

 | the current passive serial solutions does not use the existing SPI
 | drivers.

What do you mean with SPI drivers? SPI flash drivers or SPI controller
drivers? Does the "passive serial solution" expose an SPI bus and you
can access the SPI flash on it in a generic way? Then the solution
should be to write a SPI (controller) driver, the flash should then
be automatically be detected.

-michael

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-21 kl. 10:08, skrev Michael Walle:
>>> If it is right or wrong to use that as an MTD is a matter of opinion.
>> I am still hoping the MTD maintainer would provide input here.
> I might be missing something, but what is the reasoning here, to add this
> to the mtd subsystem? One is saving space, but I agree with Marek, this
> isn't a valid argument to just put any (unrelated) stuff into the MTD
> subsystem.
>
> Also, as Marek pointed out, there are many different 'programming'
> solutions for CPLDs/FPGAs and most of them don't share anything with
> MTD. You seem to be just focusing on the "passive serial" one.
Yes, the passive serial is very different from the other ways of 
configuring an FPGA.

It is write only. You cannot read back the configuration and no partial 
reconfiguration.

You do not have to go through AXI/PCIe busses.

You access through an SPI device, but todays solution does not support 
using the SPI driver.

>
> Now, I saw you mentioned that
>
>   | the current passive serial solutions does not use the existing SPI
>   | drivers.
>
> What do you mean with SPI drivers? SPI flash drivers or SPI controller
> drivers? Does the "passive serial solution" expose an SPI bus and you
> can access the SPI flash on it in a generic way? Then the solution
> should be to write a SPI (controller) driver, the flash should then
> be automatically be detected.

The proposed solution is similar to an SPI flash driver.

The FPGA bitstream is typically stored in the CPU boot flash.

The CPU reads the bitstream and does an SPI transfer to the FPGA.

The FPGA is not connected to any flash memory.


The FPGA chip exposes (in SPI terms) SCK, nCS and MOSI, but not MISO so 
you cannot read back anything.

The driver will return an empty block on read.

The SPI transfer to configure the FPGA is doing

* toggle a GPIO signal (nCONFIG)

* Do an SPI write (using the SPI controller driver)

* After the SPI transfer complete, you check the status of some GPIO 
(nSTATUS etc.).

This is all hidden from the MTD.

What the MTD subsystem sees is a "write-only memory" that has to be 
written with exactly 'n' bytes.

The additional features of MTD simplifies the user interaction by 
exposing a device and
providing info on the device.


The only difference the MTD subsystem would see is
that there is a new subdirectory "fpga" with drivers
and the Kconfig + Makefile changes to support the directory.
Otherwise it plugs right in.


====

The current solutions for passive serial cannot use the SPI controller 
driver
so each board implements SPI controller inside their 'board' files.
You cannot reuse this code in practice, so every one that wants to
have passive serial has to write their own SPI access routines.

====
The FPGA manager does not support device tree, and I will not
be able to spend time on introducing this, as Marek advices.

Best Regards
Ulf Samuelsson

>
> -michael

-- 
Best Regards,
Ulf Samuelsson
eMagii
+46 722 427437
ulf@emagii.com

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Hi Guys,
We are currently discussing how to support passive serial configuration 
of FPGAs in u-boot.

Checking the u-boot source code reveals that only your boards
actually supports this.

board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
$ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
Patrick Bruenn <p.bruenn@beckhoff.com> (maintainer:MX53 CX9020)

board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...
$ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
Wolfgang Wegner <w.wegner@astro-kom.de> (maintainer:MCF5373L BOARD)

The proposed ideas is to have the FPGA as an SPI peripheral
in the device tree.

I have a working version, where the FPGA is an MTD device
and the general idea is to add it to the FPGA manager,
which unfortunately needs a major rewrite to support device tree.

Your board support configuring the FPGA in the 'board' files.


* Are these boards still active and will require updates of u-boot?

* If so, would it be a problem porting the FPGA configuration to the 
device tree?

Marek does not want two drivers supporting passive serial,
and supporting both 'board' drivers and devicetree drivers
seems to result in unclean code.

Best Regards
Ulf Samuelsson

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Michael Walle]

Am 2023-02-21 11:42, schrieb Ulf Samuelsson:
> Den 2023-02-21 kl. 10:08, skrev Michael Walle:
>>>> If it is right or wrong to use that as an MTD is a matter of 
>>>> opinion.
>>> I am still hoping the MTD maintainer would provide input here.
>> I might be missing something, but what is the reasoning here, to add 
>> this
>> to the mtd subsystem? One is saving space, but I agree with Marek, 
>> this
>> isn't a valid argument to just put any (unrelated) stuff into the MTD
>> subsystem.
>> 
>> Also, as Marek pointed out, there are many different 'programming'
>> solutions for CPLDs/FPGAs and most of them don't share anything with
>> MTD. You seem to be just focusing on the "passive serial" one.
> Yes, the passive serial is very different from the other ways of
> configuring an FPGA.
> 
> It is write only. You cannot read back the configuration and no
> partial reconfiguration.
> 
> You do not have to go through AXI/PCIe busses.
> 
> You access through an SPI device, but todays solution does not support
> using the SPI driver.

I can't follow you here.

>> Now, I saw you mentioned that
>> 
>>   | the current passive serial solutions does not use the existing SPI
>>   | drivers.
>> 
>> What do you mean with SPI drivers? SPI flash drivers or SPI controller
>> drivers? Does the "passive serial solution" expose an SPI bus and you
>> can access the SPI flash on it in a generic way? Then the solution
>> should be to write a SPI (controller) driver, the flash should then
>> be automatically be detected.
> 
> The proposed solution is similar to an SPI flash driver.
> 
> The FPGA bitstream is typically stored in the CPU boot flash.
> 
> The CPU reads the bitstream and does an SPI transfer to the FPGA.
> 
> The FPGA is not connected to any flash memory.

Ah I missed that. So you don't want to program the SPI flash behind
the FPGA, but the FPGA directly. You are loading a bitstream from
somewhere and configure the FPGA. Using a SPI like interface, i.e.
it is write only as it lacks the DO.

> The FPGA chip exposes (in SPI terms) SCK, nCS and MOSI, but not MISO
> so you cannot read back anything.

it should read "This FPGA chip". I expect there are much more variants
how to configure an FPGA. Would these also use your proposed solution?

> The driver will return an empty block on read.
> 
> The SPI transfer to configure the FPGA is doing
> 
> * toggle a GPIO signal (nCONFIG)
> 
> * Do an SPI write (using the SPI controller driver)
> 
> * After the SPI transfer complete, you check the status of some GPIO
> (nSTATUS etc.).
> 
> This is all hidden from the MTD.
> 
> What the MTD subsystem sees is a "write-only memory" that has to be
> written with exactly 'n' bytes.

MTD is mainly around erasable blocks, although there are some exotic
things like mtdram. I fail to see how this would apply to your FPGA.

> The additional features of MTD simplifies the user interaction by
> exposing a device and
> providing info on the device.

Which would that be? What are the features of MTD which you need
here? You've mentioned MTD partitions, but I'm not sure these really
apply to partial reconfigurations. I know that Linux has a different
device tree binding for it. So making u-boot use mtd partitions (if
thats possible at all) and not the linux bindings means they'll
diverge, which is something we want to avoid.

> The only difference the MTD subsystem would see is
> that there is a new subdirectory "fpga" with drivers
> and the Kconfig + Makefile changes to support the directory.
> Otherwise it plugs right in.

 From a user point of view this is really confusing. Why would
you sometimes configure an fpga with the mtd command and sometimes
with the fpga command depening whether it is using this passive
programming thingy.

> The current solutions for passive serial cannot use the SPI controller 
> driver
> so each board implements SPI controller inside their 'board' files.
> You cannot reuse this code in practice, so every one that wants to
> have passive serial has to write their own SPI access routines.

Sorry, I didn't follow this too closely. Do you have some pointers?

-michael

> ====
> The FPGA manager does not support device tree, and I will not
> be able to spend time on introducing this, as Marek advices.
> 
> Best Regards
> Ulf Samuelsson
> 
>> 
>> -michael

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Michael Walle]

> Sorry, I didn't follow this too closely. Do you have some pointers?

I just saw your latest mail. Thanks.

-michael

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-21 kl. 02:13, skrev Marek Vasut:
> On 2/21/23 00:47, Ulf Samuelsson wrote:
>>
>>
>> Den 2023-02-20 kl. 23:34, skrev Marek Vasut:
>>> On 2/20/23 22:29, Ulf Samuelsson wrote:
>>>
>>> [...]
>>>
>>>>> To sum it up:
>>>>> - The only part of the MTD subsystem that is in use by this driver 
>>>>> is the write callback, everything else is unused. That does not 
>>>>> make MTD subsystem the right tool.
>>>>
>>>> No, the MTD subsystem is compatible with the device tree.
>>>
>>> That does not justify overloading the subsystem with unrelated 
>>> hardware support for which there is an existing subsystem with 
>>> existing drivers for the functionality you would duplicate in the MTD 
>>> subsystem. There are existing DT bindings for FPGAs, see Linux 
>>> Documentation/devicetree/bindings/fpga/ , use those and add support 
>>> for them into the FPGA subsystem if needed .
>>
>>
>> The current u-boot FPGA support is incompatible with the linux 
>> devicetree for FPGAs.
>> There is simply nothing there.
>> That is the reason for the proposal in the first place.
> 
> Add it, that seems like the best approach here.

As I said, this is not an option for me.

> 
>>>> You can also get information about the FPGA when you list the mtd 
>>>> devices.
>>>
>>> No, you cannot, because the information you may get out of the FPGA 
>>> in some cases does not map to MTD devices .
>>
>> Please give some examples.
> 
> Which FPGA bus bridge(s) are enabled for example . This is something 
> present on SoCFPGA solutions , e.g. an AXI bridge into the fabric .
> 

Is that not something which you handle by placing your FPGA 
configuration block under the bus bridge in the device tree?

In the proposed driver, the FPGA is under the SPI bus and the
FPGA driver calls the access functions of the bus.

>>>> It allows you to support partial reconfiguration through the 
>>>> partition feature. You can write any size block to any part of the 
>>>> MTD, and you
>>>> can express that in a device tree. That is what is needed.
>>>
>>> I am afraid it is not as simple as that, see Linux 
>>> Documentation/devicetree/bindings/fpga/fpga-region.txt . Furthermore, 
>>> those DT bindings are already part of Linux and Linux expects the DT 
>>> to contain exactly those bindings, U-Boot tries not to diverge from 
>>> what Linux does in DT.
>>>
>>
>> The device tree in my driver is a superset to what linux does.
>> It tests more status pins, and provides a name for the FPGA.
>> It would not be difficult to make it 100% compatible.
> 
> Extend the Linux DT bindings if they are insufficient, but please don't 
> try and reinvent the wheel.

That is the plan.

> 
>>>>> - There is existing FPGA subsystem both in U-Boot and Linux, extend 
>>>>> it if necessary. That also covers the usecases which go past a 
>>>>> simple full bitstream upload.
>>>>
>>>> And for use, that introduces a command set, which we do not need
>>>> and adds 50-100kB to the image.
>>>
>>> If you were to turn everything that needs read/write callback into an 
>>> MTD device, you would be able to save even more space, but that does 
>>> not make it right. You could try and reduce the FPGA command size if 
>>> there is code which is unused for some usecases, that can be selected 
>>> via Kconfig at build time.
>>>
>>
>>  From my point of view, Having the FPGA command set duplicates the 
>> functionality of the MTD.
>> That is the major contributor to code space expansion.
>>
>> The thing you are writing to is SRAM which is memory.
> 
> There are flash based FPGAs , like Altera MAXV.
> There are CPLDs which are not even flash based.

AFAIK, they are never configured by a CPU.
They are not supported by U-Boot.

> 
>> If it is right or wrong to use that as an MTD is a matter of opinion.
> 
> I am still hoping the MTD maintainer would provide input here.

There are maintainers of MTD memories but
there is no maintainer of the MTD uclass in MAINTAINERS..

The last major contributors were
Name                    Year            lines
Heiko Schocher          2014-2015       1600
Stefan Roese            2009             811
Boris Brezillon		2017-2018	 600
Miquel Raynal		2018-2019	 564
Sergej Lapin            2013             215
Kyungmin Park           2008             197
Marek Behún		2021	         178

Miquel Raynal is already on copy.

> 
>>>>>> Obviously, someone would have to take the time to do it, but it is 
>>>>>> certainly possible.
>>>>>
>>>>> You could make the exact same argument about making the FPGA driver 
>>>>> a MISC device, CHAR device, BLOCK device, they all have the write 
>>>>> callback, but that does not mean it is the right fit.
>>>>>
>>>> That is because you see the "write" as the only function.
>>>
>>> Yes, because that is the only callback this patch implements, the 
>>> rest are empty.
>>
>> You cannot read back the configuration from a Cyclone 10.
> 
> Therefore, the entire read part of whatever subsystem is unnecessary.
> 
> Erase as well for most FPGAs which are SRAM based, even for flash based 
> FPGA, per-sector erase is not available, since there is no concept of 
> sectors or erase blocks at all.
> 
>> A driver for an FPGA where configuration could be read back of course 
>> would have a read routine as well.
>> The MTD subsystem can display the information the driver provides.
>> That is an important piece that would be missing from a MISC or CHAR 
>> driver.
>> You also have a "device" to work with.
> 
> All this is already in the FPGA subsystem, and it is designed toward the 
> FPGA use case.
> 
>>>>> [...]
>>>>
>>>> What would work is to keep the driver as is, replacing
>>>> the MTD stuff internally with an fpga_add after generating a struct 
>>>> based on the device tree.
>>>>
>>>> Then it would be part of the FPGA subsystem.
>>>> To add this as an Altera passive serial is high risk,
>>>> since it cannot be tested on boards using that.
>>>> It would simplify things a lot to add it as another driver.
>>>
>>> No, that would make things MUCH worse, because we would have a 
>>> one-off driver used by one system, therefore poorly tested, which 
>>> duplicates functionality of existing driver used by multiple systems 
>>> and better tested. This also increases maintenance burden.
>>
>>
>> Today you have a custom functionality to access the FPGA in each board 
>> package.  That only gets tested when building that board.
> 
> No, at least the Altera SoCFPGA and Xilinx anything newer than 
> Zynq/ZynqMP is tested regularly on actual hardware, and the code is 
> common to all those systems, and lives in drivers/fpga/ .
> 
>> So if you have 'n' boards, you have 'n' different ways of accessing 
>> the FPGA. Each poorly tested, if number of users are the measurement.
>> The only thing in common is the multiplexing code.
> 
> Not really, on SoCFPGA systems, even the programming logic is common. 
> For external FPGAs, sure, you need to provide the pin or interface 
> mapping in some way, via platform data, DT, ACPI, whatever. If that is 
> platform data, it should be moved to DT, the FPGA framework DT bindings 
> should provide guidance how to do that. But even then, the driver itself 
> should be generic, and reside in drivers/fpga/ .
> 
>> Anyone that designs a new system, would have to choose between
>> using the new driver, or write their own access method.
> 
> I vote "reuse existing driver, plug in DT/platform_data/... as needed" .

The choice is for passive serial solutions.
You have the choice between
* using the proposed driver (which will use the SPI driver)
* write your own access routine in your board files.

You do not have a choice to use an existing driver, because there
is no existing driver. That is the problem with passive serial.

> 
>> They cannot use what is in u-boot today, because it is in the board 
>> directory.
> 
> This is not true for majority of FPGA systems, see drivers/fpga/ .

The proposed driver replaces the passive serial driver only.
The statement is true for all passive serial.
Please have a look at how it is implemented today.

>> Even if they have the same chip as a board with existing support, they 
>> cannot add it in, since it is in the board directory.
> 
> Are you actually talking about platform data which are used during 
> driver instantiation, or, full driver, in board/ ?
> 
When you use passive serial and connect using SPI, you cannot use
the U-Boot SPI driver. Everyone are accessing their SPI peripheral
to configure the FPGA in the board files.
Each solution is unique to that board.

> Platform data are fine-ish, full drivers in board/ should be on the way 
> out.

I agree totally.

> 
>> Only solution is to duplicate the code in their own board directory,
>> but the likelyhood of using the same CPU is slim.
>>
>> Are you happy with this?
> 
> I think we have a vastly different picture of the current situation .
> 
I speak of passive serial only.

>> I wager that anyone wanting to use passive serial would adopt the new 
>> driver.
>>
>> One solution is of course to ask the maintainers of those boards
>> if they would consider moving to a device tree solution.
>> Then the new driver would be the only driver.
> 
> Put them on CC, there is scripts/get_maintainer.pl script, use -f 
> parameter to point it to specific files. I have a feeling a lot of those 
> boards with external FPGAs being configured by poking registers in 
> board/ are going to be dropped sooner rather than later.
> 

There are only two boards that use the current passive serial driver.

$ rgrep _Passive_Serial_fns
include/ACEX1K.h:} Altera_ACEX1K_Passive_Serial_fns;
include/ACEX1K.h:} Altera_CYC2_Passive_Serial_fns;
drivers/fpga/cyclon2.c:	Altera_CYC2_Passive_Serial_fns ...
drivers/fpga/ACEX1K.c:	Altera_ACEX1K_Passive_Serial_fns ...

board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...

$ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
Patrick Bruenn <p.bruenn@beckhoff.com> (maintainer:MX53 CX9020)
$ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
Wolfgang Wegner <w.wegner@astro-kom.de> (maintainer:MCF5373L BOARD)

Patrick Bruenn <p.bruenn@beckhoff.com>
Wolfgang Wegner <w.wegner@astro-kom.de>

I will ask them in a separate email.

>>> One single driver for this common functionality is always better as 
>>> it gets more testing.
>>>
>>>> The result would of course be something which adds both complexity 
>>>> and code size for us, without any benefit.
>>>
>>> Please also consider the MTD maintainer point of view.
>>>
>> I do not see that this adds much more complexity than any other MTD 
>> device.
> 
> Again, there is a separate subsystem/framework for FPGAs . I disagree 
> one should conflate MTD and FPGA subsystems.

It works well for passive serial.

Best Regards
Ulf Samuelsson

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

Den 2023-02-21 kl. 12:19, skrev Michael Walle:
> Am 2023-02-21 11:42, schrieb Ulf Samuelsson:
>> Den 2023-02-21 kl. 10:08, skrev Michael Walle:
>>>>> If it is right or wrong to use that as an MTD is a matter of opinion.
>>>> I am still hoping the MTD maintainer would provide input here.
>>> I might be missing something, but what is the reasoning here, to add 
>>> this
>>> to the mtd subsystem? One is saving space, but I agree with Marek, this
>>> isn't a valid argument to just put any (unrelated) stuff into the MTD
>>> subsystem.
>>>
>>> Also, as Marek pointed out, there are many different 'programming'
>>> solutions for CPLDs/FPGAs and most of them don't share anything with
>>> MTD. You seem to be just focusing on the "passive serial" one.
>> Yes, the passive serial is very different from the other ways of
>> configuring an FPGA.
>>
>> It is write only. You cannot read back the configuration and no
>> partial reconfiguration.
>>
>> You do not have to go through AXI/PCIe busses.
>>
>> You access through an SPI device, but todays solution does not support
>> using the SPI driver.
> 
> I can't follow you here.
> 

You have a chip that is connected to an SPI bus using SCK, MOSI, nCS.
All the accesses are handled by the SPI controller driver outside the chip.


>>> Now, I saw you mentioned that
>>>
>>>   | the current passive serial solutions does not use the existing SPI
>>>   | drivers.
>>>
>>> What do you mean with SPI drivers? SPI flash drivers or SPI controller
>>> drivers? Does the "passive serial solution" expose an SPI bus and you
>>> can access the SPI flash on it in a generic way? Then the solution
>>> should be to write a SPI (controller) driver, the flash should then
>>> be automatically be detected.
>>
>> The proposed solution is similar to an SPI flash driver.
>>
>> The FPGA bitstream is typically stored in the CPU boot flash.
>>
>> The CPU reads the bitstream and does an SPI transfer to the FPGA.
>>
>> The FPGA is not connected to any flash memory.
> 
> Ah I missed that. So you don't want to program the SPI flash behind
> the FPGA, but the FPGA directly. You are loading a bitstream from
> somewhere and configure the FPGA. Using a SPI like interface, i.e.
> it is write only as it lacks the DO.
> 
Exactly.

>> The FPGA chip exposes (in SPI terms) SCK, nCS and MOSI, but not MISO
>> so you cannot read back anything.
> 
> it should read "This FPGA chip". I expect there are much more variants
> how to configure an FPGA. Would these also use your proposed solution?
> 

There are the following methods I am aware of.

Active Serial - the FPGA is connected to an SPI flash, and loads this
	flash at reset. The CPU may or may not have the capability
	to program the flash, and then it is a normal MTD SPI Flash.
         The FPGA manager is not involved at all.

Passive Serial: The CPU SPI controller (or similar) is connected to
	pins on the FPGA and the only operation allowed is writing
	a fixed size to the FPGA.
	The user needs to know the size of the FPGA.
	The support for this in u-boot is very old-fashioned
	and new users get very little help.
	The user needs to write an SPI access routine as the SPI
	driver structure is incompatible with the FPGA manager
	code for passive serial.

Passive Parallel: The CPU loads the FPGA through the parallell bus.
	I doubt this is rarely used.

SOC:	The Intel/Altera and AMD/Xilinx have internal FPGA
	configuration interfaces. It is parallell.
	They are supported in the FPGA manager.

If the use of MTD is restricted to passive serial, this is OK with me.

>> The driver will return an empty block on read.
>>
>> The SPI transfer to configure the FPGA is doing
>>
>> * toggle a GPIO signal (nCONFIG)
>>
>> * Do an SPI write (using the SPI controller driver)
>>
>> * After the SPI transfer complete, you check the status of some GPIO
>> (nSTATUS etc.).
>>
>> This is all hidden from the MTD.
>>
>> What the MTD subsystem sees is a "write-only memory" that has to be
>> written with exactly 'n' bytes.
> 
> MTD is mainly around erasable blocks, although there are some exotic
> things like mtdram. I fail to see how this would apply to your FPGA.

These are the things I want to achieve.

* transfer data using the SPI driver and not use board files.
   For that, the FPGA should be under the SPI in the device tree.
* The size of the FPGA bitstream should be specified in the device tree.
* The FPGA should be accessed as a device, not an address.
* I want a list of the FPGAs connected in the system
* Minimize code.
* Simplify user interface.

 From the CPU point of view, the FPGA is just a RAM location on the SPI 
bus. It cannot be read, but it can be written.

This is not surprising, because it is simply a RAM.
It happens to have side effects, but that is not important.

The MTD subsystem supplies everything I need, and adding the driver
there is much less work than anything else.


> 
>> The additional features of MTD simplifies the user interaction by
>> exposing a device and
>> providing info on the device.
> 
> Which would that be? What are the features of MTD which you need
> here? You've mentioned MTD partitions, but I'm not sure these really
> apply to partial reconfigurations. I know that Linux has a different
> device tree binding for it. So making u-boot use mtd partitions (if
> thats possible at all) and not the linux bindings means they'll
> diverge, which is something we want to avoid.

You cannot do partial reconfiguration on passive serial,
so it is only of interest if someone feels a need to expand
the MTD to other configuration methods.

Here is how an FPGA region looks in Linux.
If I understand things correctly, you have one memory region per
partial configuration. This looks quite a lot like partitions.

&fpga_region0 {
	#address-cells = <1>;
	#size-cells = <1>;

	firmware-name = "soc_system.rbf";
	fpga-bridges = <&fpga_bridge1>;
	ranges = <0x20000 0xff200000 0x100000>,
		 <0x0 0xc0000000 0x20000000>;

	gpio@10040 {
		compatible = "altr,pio-1.0";
		reg = <0x10040 0x20>;
		altr,ngpio = <4>;
		#gpio-cells = <2>;
		clocks = <2>;
		gpio-controller;
	};

	onchip-memory {
		device_type = "memory";
		compatible = "altr,onchipmem-15.1";
		reg = <0x0 0x10000>;
	};
};

> 
>> The only difference the MTD subsystem would see is
>> that there is a new subdirectory "fpga" with drivers
>> and the Kconfig + Makefile changes to support the directory.
>> Otherwise it plugs right in.
> 
>  From a user point of view this is really confusing. Why would
> you sometimes configure an fpga with the mtd command and sometimes
> with the fpga command depening whether it is using this passive
> programming thingy.

I think that most board designers would select one method of configuring
the FPGA and they would only have either the FPGA or the MTD command 
set. I would be surprised if an engineer would be confused by this.


> 
>> The current solutions for passive serial cannot use the SPI controller 
>> driver
>> so each board implements SPI controller inside their 'board' files.
>> You cannot reuse this code in practice, so every one that wants to
>> have passive serial has to write their own SPI access routines.
> 
> Sorry, I didn't follow this too closely. Do you have some pointers?
> 
> -michael
> 
>> ====
>> The FPGA manager does not support device tree, and I will not
>> be able to spend time on introducing this, as Marek advices.
>>
>> Best Regards
>> Ulf Samuelsson
>>
>>>
>>> -michael

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Michael Walle]

> If the use of MTD is restricted to passive serial, this is OK with me.

Yeah, but that is not how upstream things work. You need to also think
of any other use cases.

> These are the things I want to achieve.
> 
> * transfer data using the SPI driver and not use board files.
>   For that, the FPGA should be under the SPI in the device tree.

So to me it sounds like you actually want a "altera passive spi"
driver which uses the spi subsytem (and not the MTD layer).

> * The size of the FPGA bitstream should be specified in the device 
> tree.

I'm not experienced with the fpga bindings, but maybe they have
something like that. Or you can deduce it from the compatible string.
What's your usecase here? It sounds like a u-boot limitation to me (if
you don't use files).

> * The FPGA should be accessed as a device, not an address.
> * I want a list of the FPGAs connected in the system
> * Minimize code.
> * Simplify user interface.

agreed.

> From the CPU point of view, the FPGA is just a RAM location on the SPI
> bus. It cannot be read, but it can be written.
> 
> This is not surprising, because it is simply a RAM.
> It happens to have side effects, but that is not important.

Can't follow you here. How can the FPGA be a location in RAM if it's
behind an SPI bus? Thats not how SPI works.

> The MTD subsystem supplies everything I need, and adding the driver
> there is much less work than anything else.

Yeah but just because it has everything you need, doesn't mean
it is a good fit for your use case.

Again. I think you want to have a passive serial driver which
represents your fpga as a device which then can be configured.
There is already an empty (sigh) FPGA_UCLASS. That should probably
be extended. Ideally, this should be probed by the linux fpga
bindings.

>>> The additional features of MTD simplifies the user interaction by
>>> exposing a device and
>>> providing info on the device.
>> 
>> Which would that be? What are the features of MTD which you need
>> here? You've mentioned MTD partitions, but I'm not sure these really
>> apply to partial reconfigurations. I know that Linux has a different
>> device tree binding for it. So making u-boot use mtd partitions (if
>> thats possible at all) and not the linux bindings means they'll
>> diverge, which is something we want to avoid.
> 
> You cannot do partial reconfiguration on passive serial,
> so it is only of interest if someone feels a need to expand
> the MTD to other configuration methods.

So.. what MTD features are then used by your approach?

> Here is how an FPGA region looks in Linux.
> If I understand things correctly, you have one memory region per
> partial configuration. This looks quite a lot like partitions.
> 
> &fpga_region0 {
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 
> 	firmware-name = "soc_system.rbf";
> 	fpga-bridges = <&fpga_bridge1>;
> 	ranges = <0x20000 0xff200000 0x100000>,
> 		 <0x0 0xc0000000 0x20000000>;
> 
> 	gpio@10040 {
> 		compatible = "altr,pio-1.0";
> 		reg = <0x10040 0x20>;
> 		altr,ngpio = <4>;
> 		#gpio-cells = <2>;
> 		clocks = <2>;
> 		gpio-controller;
> 	};
> 
> 	onchip-memory {
> 		device_type = "memory";
> 		compatible = "altr,onchipmem-15.1";
> 		reg = <0x0 0x10000>;
> 	};
> };
> 
>> 
>>> The only difference the MTD subsystem would see is
>>> that there is a new subdirectory "fpga" with drivers
>>> and the Kconfig + Makefile changes to support the directory.
>>> Otherwise it plugs right in.
>> 
>>  From a user point of view this is really confusing. Why would
>> you sometimes configure an fpga with the mtd command and sometimes
>> with the fpga command depening whether it is using this passive
>> programming thingy.
> 
> I think that most board designers would select one method of 
> configuring
> the FPGA and they would only have either the FPGA or the MTD command
> set. I would be surprised if an engineer would be confused by this.

As a user I'd expect one command for any similar devices. As a board
integrator, the fpga would be loaded by the board file anyway if needed
during u-boot/bootup.

Just my two cents as a spi-nor reviewer in linux. You'd have to
convince the u-boot maintainers. There is already an FPGA subsystem
in linux so I doubt your approach would make it into linux.

-michael

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Steffen Dirkwinkel]

Hi!

On Tue, 2023-02-21 at 11:58 +0100, Ulf Samuelsson wrote:
> CAUTION: External Email!!
> Hi Guys,
> We are currently discussing how to support passive serial configuration
> of FPGAs in u-boot.
>
> Checking the u-boot source code reveals that only your boards
> actually supports this.
>
> board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
> $ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
> Patrick Bruenn <p.bruenn@beckhoff.com> (maintainer:MX53 CX9020)

This doesn't seem immediately relevant to our board as we don't really use spi,
but the (w)eim interface as far as i can tell right now. (has been a while since I looked
at it, have not read the imx datasheet again to see if this is spi in disguise)

If anything the binding should be shared between u-boot and the kernel. We just had a bug
because of differences between the two.
Something like the "altr,fpga-passive-serial" in arch/arm/boot/dts/imx6q-evi.dts in the kernel would match the
usecase.

We wouldn't be opposed to changing the dt and also just repaired our CI for these devices and can test changes quickly.

Best Regards
Steffen Dirkwinkel

>
> board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...
> $ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
> Wolfgang Wegner <w.wegner@astro-kom.de> (maintainer:MCF5373L BOARD)
>
> The proposed ideas is to have the FPGA as an SPI peripheral
> in the device tree.
>
> I have a working version, where the FPGA is an MTD device
> and the general idea is to add it to the FPGA manager,
> which unfortunately needs a major rewrite to support device tree.
>
> Your board support configuring the FPGA in the 'board' files.
>
>
> * Are these boards still active and will require updates of u-boot?
>
> * If so, would it be a problem porting the FPGA configuration to the
> device tree?
>
> Marek does not want two drivers supporting passive serial,
> and supporting both 'board' drivers and devicetree drivers
> seems to result in unclean code.
>
> Best Regards
> Ulf Samuelsson

This email contains confidential information. If you have received it in error, you must not read, use, copy or pass on this e-mail or its attachments. If you have received the e-mail in error, please inform me immediately by reply e-mail and then delete this e-mail from your system. Thank you

Diese E-Mail enthält vertrauliche Informationen. Sollten Sie sie irrtümlich erhalten haben, dürfen Sie diese E-Mail oder ihre Anhänge nicht lesen, verwenden, kopieren oder weitergeben. Sollten Sie die Mail versehentlich erhalten haben, teilen Sie mir dies bitte umgehend per Antwort-E-Mail mit und löschen Sie diese E-Mail dann aus Ihrem System. Vielen Dank

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Ulf Samuelsson]

On 2023-02-22 20:51, Steffen Dirkwinkel wrote:
> Hi!
> 
> On Tue, 2023-02-21 at 11:58 +0100, Ulf Samuelsson wrote:
>> CAUTION: External Email!!
>> Hi Guys,
>> We are currently discussing how to support passive serial configuration
>> of FPGAs in u-boot.
>>
>> Checking the u-boot source code reveals that only your boards
>> actually supports this.
>>
>> board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
>> $ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
>> Patrick Bruenn <p.bruenn@beckhoff.com> (maintainer:MX53 CX9020)
> 
> This doesn't seem immediately relevant to our board as we don't really use spi,
> but the (w)eim interface as far as i can tell right now. (has been a while since I looked
> at it, have not read the imx datasheet again to see if this is spi in disguise)
> 
> If anything the binding should be shared between u-boot and the kernel. We just had a bug
> because of differences between the two.
> Something like the "altr,fpga-passive-serial" in arch/arm/boot/dts/imx6q-evi.dts in the kernel would match the
> usecase.
> 
> We wouldn't be opposed to changing the dt and also just repaired our CI for these devices and can test changes quickly.
> 
I had a look at your code again.
It appears that you are using the Altera "passive serial" interface in 
U-Boot, but your board routine appears to write 8-bit bytes to the 
external bus.

The only two reasons for this I can think of is that you are either
using an external serializer converting the 8 bit words to
a serial stream or you are actually using the passive "parallell
interface" on the FPGA, but use the "passive serial" routines
in u-boot.

Can you please confirm how the hardware works?


The maintainer of the other board using "passive serial" 
(w.wegner@astro-kom.de <w.wegner@astro-kom.de>)
bounced as an unknown user.  This means that the "board/astro/mcf5373" 
does not have an active MAINTAINER.

The board was added in 2010, and has not seen any updates from
astro-kom.de since then. All the maintenance has been by people 
maintaining subsystems.

Best Regards
Ulf Samuelsson


> Best Regards
> Steffen Dirkwinkel
> 
>>
>> board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...
>> $ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
>> Wolfgang Wegner <w.wegner@astro-kom.de> (maintainer:MCF5373L BOARD)
>>
>> The proposed ideas is to have the FPGA as an SPI peripheral
>> in the device tree.
>>
>> I have a working version, where the FPGA is an MTD device
>> and the general idea is to add it to the FPGA manager,
>> which unfortunately needs a major rewrite to support device tree.
>>
>> Your board support configuring the FPGA in the 'board' files.
>>
>>
>> * Are these boards still active and will require updates of u-boot?
>>
>> * If so, would it be a problem porting the FPGA configuration to the
>> device tree?
>>
>> Marek does not want two drivers supporting passive serial,
>> and supporting both 'board' drivers and devicetree drivers
>> seems to result in unclean code.
>>
>> Best Regards
>> Ulf Samuelsson
> 
> This email contains confidential information. If you have received it in error, you must not read, use, copy or pass on this e-mail or its attachments. If you have received the e-mail in error, please inform me immediately by reply e-mail and then delete this e-mail from your system. Thank you
> 
> Diese E-Mail enthält vertrauliche Informationen. Sollten Sie sie irrtümlich erhalten haben, dürfen Sie diese E-Mail oder ihre Anhänge nicht lesen, verwenden, kopieren oder weitergeben. Sollten Sie die Mail versehentlich erhalten haben, teilen Sie mir dies bitte umgehend per Antwort-E-Mail mit und löschen Sie diese E-Mail dann aus Ihrem System. Vielen Dank
> 
> Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
> Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
> 
> 

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Steffen Dirkwinkel]

> On 23. Feb 2023, at 09:28, Ulf Samuelsson <u-boot@emagii.com> wrote:
>
> CAUTION: External Email!!
>
>
>
> On 2023-02-22 20:51, Steffen Dirkwinkel wrote:
>> Hi!
>> On Tue, 2023-02-21 at 11:58 +0100, Ulf Samuelsson wrote:
>>> CAUTION: External Email!!
>>> Hi Guys,
>>> We are currently discussing how to support passive serial configuration
>>> of FPGAs in u-boot.
>>>
>>> Checking the u-boot source code reveals that only your boards
>>> actually supports this.
>>>
>>> board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
>>> $ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
>>> Patrick Bruenn <p.bruenn@beckhoff.com> (maintainer:MX53 CX9020)
>> This doesn't seem immediately relevant to our board as we don't really use spi,
>> but the (w)eim interface as far as i can tell right now. (has been a while since I looked
>> at it, have not read the imx datasheet again to see if this is spi in disguise)
>> If anything the binding should be shared between u-boot and the kernel. We just had a bug
>> because of differences between the two.
>> Something like the "altr,fpga-passive-serial" in arch/arm/boot/dts/imx6q-evi.dts in the kernel would match the
>> usecase.
>> We wouldn't be opposed to changing the dt and also just repaired our CI for these devices and can test changes quickly.
> I had a look at your code again.
> It appears that you are using the Altera "passive serial" interface in U-Boot, but your board routine appears to write 8-bit bytes to the external bus.
>
> The only two reasons for this I can think of is that you are either
> using an external serializer converting the 8 bit words to
> a serial stream or you are actually using the passive "parallell
> interface" on the FPGA, but use the "passive serial" routines
> in u-boot.
>
> Can you please confirm how the hardware works?

I checked the schematic, we’re using fast_passive_parallel.
Since both are using the same codepaths right now it didn’t matter. I’ll send a patch

>
>
> The maintainer of the other board using "passive serial" (w.wegner@astro-kom.de <w.wegner@astro-kom.de>)
> bounced as an unknown user.  This means that the "board/astro/mcf5373" does not have an active MAINTAINER.
>
> The board was added in 2010, and has not seen any updates from
> https://nospamproxywebp.beckhoff.com/enqsig/link?id=BCAAAABGd7m3lfMOLOIhx3uNR_1Qav4Ffx8-jQB0torD82ts_G8AAAB_Q13kS5en5WiHBszxeeaYjAQwTPXiCeF8gx_zVjByVYRebkfOfUTW4VnpSmQ2VlvWczqoVxWwP7qF1C4VccAGIB-3QCgtp-eMVgM5VcCkSRCJT8HehzvaoUnQ6YYu2ofkD0GMhgmICiFDb8Cv_2k1  since then. All the maintenance has been by people maintaining subsystems.
>
> Best Regards
> Ulf Samuelsson
>
>
>> Best Regards
>> Steffen Dirkwinkel
>>>
>>> board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...
>>> $ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
>>> Wolfgang Wegner <w.wegner@astro-kom.de> (maintainer:MCF5373L BOARD)
>>>
>>> The proposed ideas is to have the FPGA as an SPI peripheral
>>> in the device tree.
>>>
>>> I have a working version, where the FPGA is an MTD device
>>> and the general idea is to add it to the FPGA manager,
>>> which unfortunately needs a major rewrite to support device tree.
>>>
>>> Your board support configuring the FPGA in the 'board' files.
>>>
>>>
>>> * Are these boards still active and will require updates of u-boot?
>>>
>>> * If so, would it be a problem porting the FPGA configuration to the
>>> device tree?
>>>
>>> Marek does not want two drivers supporting passive serial,
>>> and supporting both 'board' drivers and devicetree drivers
>>> seems to result in unclean code.
>>>
>>> Best Regards
>>> Ulf Samuelsson
>> This email contains confidential information. If you have received it in error, you must not read, use, copy or pass on this e-mail or its attachments. If you have received the e-mail in error, please inform me immediately by reply e-mail and then delete this e-mail from your system. Thank you
>> Diese E-Mail enthält vertrauliche Informationen. Sollten Sie sie irrtümlich erhalten haben, dürfen Sie diese E-Mail oder ihre Anhänge nicht lesen, verwenden, kopieren oder weitergeben. Sollten Sie die Mail versehentlich erhalten haben, teilen Sie mir dies bitte umgehend per Antwort-E-Mail mit und löschen Sie diese E-Mail dann aus Ihrem System. Vielen Dank
>> Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
>> Registered office: Verl, Germany | Register court: Guetersloh HRA 7075


This email contains confidential information. If you have received it in error, you must not read, use, copy or pass on this e-mail or its attachments. If you have received the e-mail in error, please inform me immediately by reply e-mail and then delete this e-mail from your system. Thank you

Diese E-Mail enthält vertrauliche Informationen. Sollten Sie sie irrtümlich erhalten haben, dürfen Sie diese E-Mail oder ihre Anhänge nicht lesen, verwenden, kopieren oder weitergeben. Sollten Sie die Mail versehentlich erhalten haben, teilen Sie mir dies bitte umgehend per Antwort-E-Mail mit und löschen Sie diese E-Mail dann aus Ihrem System. Vielen Dank

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [PATCH 3/4] mtd/fpga: add fpga directory to mtd (with Cyclone 10)

[Steffen Dirkwinkel]

> On 23. Feb 2023, at 09:28, Ulf Samuelsson <u-boot@emagii.com> wrote:
>
> CAUTION: External Email!!
>
>
>
> On 2023-02-22 20:51, Steffen Dirkwinkel wrote:
>> Hi!
>> On Tue, 2023-02-21 at 11:58 +0100, Ulf Samuelsson wrote:
>>> CAUTION: External Email!!
>>> Hi Guys,
>>> We are currently discussing how to support passive serial configuration
>>> of FPGAs in u-boot.
>>>
>>> Checking the u-boot source code reveals that only your boards
>>> actually supports this.
>>>
>>> board/beckhoff/mx53cx9020/mx53cx9020.c:...Altera_CYC2_Passive_Serial ...
>>> $ scripts/get_maintainer.pl board/beckhoff/mx53cx9020/mx53cx9020.c
>>> Patrick Bruenn <p.bruenn@beckhoff.com> (maintainer:MX53 CX9020)
>> This doesn't seem immediately relevant to our board as we don't really use spi,
>> but the (w)eim interface as far as i can tell right now. (has been a while since I looked
>> at it, have not read the imx datasheet again to see if this is spi in disguise)
>> If anything the binding should be shared between u-boot and the kernel. We just had a bug
>> because of differences between the two.
>> Something like the "altr,fpga-passive-serial" in arch/arm/boot/dts/imx6q-evi.dts in the kernel would match the
>> usecase.
>> We wouldn't be opposed to changing the dt and also just repaired our CI for these devices and can test changes quickly.
> I had a look at your code again.
> It appears that you are using the Altera "passive serial" interface in U-Boot, but your board routine appears to write 8-bit bytes to the external bus.
>
> The only two reasons for this I can think of is that you are either
> using an external serializer converting the 8 bit words to
> a serial stream or you are actually using the passive "parallell
> interface" on the FPGA, but use the "passive serial" routines
> in u-boot.
>
> Can you please confirm how the hardware works?

I checked the schematic, we’re using fast_passive_parallel.
Since both are using the same codepaths right now it didn’t matter. I’ll send a patch

>
>
> The maintainer of the other board using "passive serial" (w.wegner@astro-kom.de <w.wegner@astro-kom.de>)
> bounced as an unknown user.  This means that the "board/astro/mcf5373" does not have an active MAINTAINER.
>
> The board was added in 2010, and has not seen any updates from
> https://nospamproxywebp.beckhoff.com/enqsig/link?id=BCAAAABGd7m3lfMOLOIhx3uNR_1Qav4Ffx8-jQB0torD82ts_G8AAAB_Q13kS5en5WiHBszxeeaYjAQwTPXiCeF8gx_zVjByVYRebkfOfUTW4VnpSmQ2VlvWczqoVxWwP7qF1C4VccAGIB-3QCgtp-eMVgM5VcCkSRCJT8HehzvaoUnQ6YYu2ofkD0GMhgmICiFDb8Cv_2k1  since then. All the maintenance has been by people maintaining subsystems.
>
> Best Regards
> Ulf Samuelsson
>
>
>> Best Regards
>> Steffen Dirkwinkel
>>>
>>> board/astro/mcf5373l/fpga.c:Altera_CYC2_Passive_Serial ...
>>> $ scripts/get_maintainer.pl board/astro/mcf5373l/fpga.c
>>> Wolfgang Wegner <w.wegner@astro-kom.de> (maintainer:MCF5373L BOARD)
>>>
>>> The proposed ideas is to have the FPGA as an SPI peripheral
>>> in the device tree.
>>>
>>> I have a working version, where the FPGA is an MTD device
>>> and the general idea is to add it to the FPGA manager,
>>> which unfortunately needs a major rewrite to support device tree.
>>>
>>> Your board support configuring the FPGA in the 'board' files.
>>>
>>>
>>> * Are these boards still active and will require updates of u-boot?
>>>
>>> * If so, would it be a problem porting the FPGA configuration to the
>>> device tree?
>>>
>>> Marek does not want two drivers supporting passive serial,
>>> and supporting both 'board' drivers and devicetree drivers
>>> seems to result in unclean code.
>>>
>>> Best Regards
>>> Ulf Samuelsson
>> This email contains confidential information. If you have received it in error, you must not read, use, copy or pass on this e-mail or its attachments. If you have received the e-mail in error, please inform me immediately by reply e-mail and then delete this e-mail from your system. Thank you
>> Diese E-Mail enthält vertrauliche Informationen. Sollten Sie sie irrtümlich erhalten haben, dürfen Sie diese E-Mail oder ihre Anhänge nicht lesen, verwenden, kopieren oder weitergeben. Sollten Sie die Mail versehentlich erhalten haben, teilen Sie mir dies bitte umgehend per Antwort-E-Mail mit und löschen Sie diese E-Mail dann aus Ihrem System. Vielen Dank
>> Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
>> Registered office: Verl, Germany | Register court: Guetersloh HRA 7075


This email contains confidential information. If you have received it in error, you must not read, use, copy or pass on this e-mail or its attachments. If you have received the e-mail in error, please inform me immediately by reply e-mail and then delete this e-mail from your system. Thank you

Diese E-Mail enthält vertrauliche Informationen. Sollten Sie sie irrtümlich erhalten haben, dürfen Sie diese E-Mail oder ihre Anhänge nicht lesen, verwenden, kopieren oder weitergeben. Sollten Sie die Mail versehentlich erhalten haben, teilen Sie mir dies bitte umgehend per Antwort-E-Mail mit und löschen Sie diese E-Mail dann aus Ihrem System. Vielen Dank

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075