[PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Johannes Zink]

Lattice MachXO2 FPGAs have internal configuration flash which can be
reprogrammed over different interfaces. The former driver implementation
supported programming via SPI, this patch series adds programming via
I2C.

The first 4 patches convert the MachXO2 Slave binding from textual
format to YAML and add additional features like different flash areas to
be erased upon a programming cycle, a GPIO which can be used to
explicitely initialize the programming sequence, and finally I2C as
additional programming interface.

The following 10 patches clean up and refactor the previous machxo2-spi
driver code, extract functionalities common to both spi and i2c
programming interfaces as a preparation, add additional flash areas to
be erased and signalling for start of the programming sequence via gpio.

Since the original driver did not yield enough time to erase machxo2
variants with large flash memory, a variation of erase timeout handling
is added with another patch, introducing a more datasheet conformant way
of dealing with large flash sizes due to larger LUT counts.

The final patch adds the I2C bus as an additional interface for
programming.

Johannes Zink (15):
  dt-bindings: fpga: convert Lattice MachXO2 Slave binding to YAML
  dt-bindings: fpga: machxo2-slave: add erasure properties
  dt-bindings: fpga: machxo2-slave: add pin for program sequence init
  dt-bindings: fpga: machxo2-slave: add lattice,machxo2-slave-i2c
    compatible
  fpga: machxo2-spi: remove #ifdef DEBUG
  fpga: machxo2-spi: factor out status check for readability
  fpga: machxo2-spi: fix big-endianness incompatibility
  fpga: machxo2-spi: simplify with spi_sync_transfer()
  fpga: machxo2-spi: simplify spi write commands
  fpga: machxo2-spi: prepare extraction of common code
  fpga: machxo2: move non-spi-related functionality to common code
  fpga: machxo2: improve status register dump
  fpga: machxo2: add optional additional flash areas to be erased
  fpga: machxo2: add program initialization signalling via gpio
  fpga: machxo2: extend erase timeout for machxo2 FPGA

Peter Jensen (1):
  fpga: machxo2: add configuration over i2c

 .../bindings/fpga/lattice,machxo2-slave.yaml  |  80 ++++
 .../bindings/fpga/lattice-machxo2-spi.txt     |  29 --
 drivers/fpga/Kconfig                          |  14 +
 drivers/fpga/Makefile                         |   2 +
 drivers/fpga/machxo2-common.c                 | 392 ++++++++++++++++++
 drivers/fpga/machxo2-common.h                 |  43 ++
 drivers/fpga/machxo2-i2c.c                    | 137 ++++++
 drivers/fpga/machxo2-spi.c                    | 366 ++--------------
 8 files changed, 712 insertions(+), 351 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
 delete mode 100644 Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt
 create mode 100644 drivers/fpga/machxo2-common.c
 create mode 100644 drivers/fpga/machxo2-common.h
 create mode 100644 drivers/fpga/machxo2-i2c.c

-- 
2.30.2

[PATCH 01/16] dt-bindings: fpga: convert Lattice MachXO2 Slave binding to YAML

[Johannes Zink]

This commit prepares adding additional properties to the machxo2-slave
device. No functional changes.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 .../bindings/fpga/lattice,machxo2-slave.yaml  | 46 +++++++++++++++++++
 .../bindings/fpga/lattice-machxo2-spi.txt     | 29 ------------
 2 files changed, 46 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
 delete mode 100644 Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt

diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
new file mode 100644
index 000000000000..d05acd6b0fc6
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/lattice,machxo2-slave.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lattice MachXO2 Slave FPGA Manager Device Tree Bindings
+
+maintainers:
+  - Johannes Zink <j.zink@pengutronix.de>
+
+description: |
+  Device used for loading the bitstream of Lattice MachXO2 FPGAs. The
+  programming sequence is described in FPGA-TN-02155 on www.latticesemi.com
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: lattice,machxo2-slave-spi
+    then:
+      $ref: /schemas/spi/spi-peripheral-props.yaml#
+properties:
+  compatible:
+    enum:
+      - lattice,machxo2-slave-spi
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        fpga@0 {
+            compatible = "lattice,machxo2-slave-spi";
+            spi-max-frequency = <8000000>;
+            reg = <0>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt b/Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt
deleted file mode 100644
index a8c362eb160c..000000000000
--- a/Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt
+++ /dev/null
@@ -1,29 +0,0 @@
-Lattice MachXO2 Slave SPI FPGA Manager
-
-Lattice MachXO2 FPGAs support a method of loading the bitstream over
-'slave SPI' interface.
-
-See 'MachXO2ProgrammingandConfigurationUsageGuide.pdf' on www.latticesemi.com
-
-Required properties:
-- compatible: should contain "lattice,machxo2-slave-spi"
-- reg: spi chip select of the FPGA
-
-Example for full FPGA configuration:
-
-	fpga-region0 {
-		compatible = "fpga-region";
-		fpga-mgr = <&fpga_mgr_spi>;
-		#address-cells = <0x1>;
-		#size-cells = <0x1>;
-	};
-
-	spi1: spi@2000 {
-        ...
-
-		fpga_mgr_spi: fpga-mgr@0 {
-			compatible = "lattice,machxo2-slave-spi";
-			spi-max-frequency = <8000000>;
-			reg = <0>;
-		};
-	};
-- 
2.30.2

[PATCH 02/16] dt-bindings: fpga: machxo2-slave: add erasure properties

[Johannes Zink]

This patch introduces additional memory areas of the machxo2-slave fpga
to be erased.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 .../bindings/fpga/lattice,machxo2-slave.yaml      | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
index d05acd6b0fc6..78f0da8f772f 100644
--- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
+++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
@@ -26,6 +26,19 @@ properties:
     enum:
       - lattice,machxo2-slave-spi
 
+  lattice,erase-sram:
+    type: boolean
+    description: SRAM is to be erased during flash erase operation
+
+  lattice,erase-feature-row:
+    type: boolean
+    description: Feature row is to be erased during flash erase operation
+
+  lattice,erase-userflash:
+    type: boolean
+    description: |
+      UFM (user flash memory) is to be erased during flash erase operation
+
 required:
   - compatible
   - reg
@@ -42,5 +55,7 @@ examples:
             compatible = "lattice,machxo2-slave-spi";
             spi-max-frequency = <8000000>;
             reg = <0>;
+            lattice,erase-sram;
+            lattice,erase-feature-row;
         };
     };
-- 
2.30.2

[PATCH 03/16] dt-bindings: fpga: machxo2-slave: add pin for program sequence init

[Johannes Zink]

This commit adds a pin which initiates the FPGA programming sequence
once pulsed low.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 .../devicetree/bindings/fpga/lattice,machxo2-slave.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
index 78f0da8f772f..03dc134ec7b8 100644
--- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
+++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
@@ -26,6 +26,12 @@ properties:
     enum:
       - lattice,machxo2-slave-spi
 
+  program-gpios:
+    maxItems: 1
+    description: |
+      GPIO Output tied to the FPGA's n_program pin to initiate a
+      programming sequence. This pin is active low.
+
   lattice,erase-sram:
     type: boolean
     description: SRAM is to be erased during flash erase operation
@@ -57,5 +63,6 @@ examples:
             reg = <0>;
             lattice,erase-sram;
             lattice,erase-feature-row;
+            lattice,program-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>
         };
     };
-- 
2.30.2

[PATCH 04/16] dt-bindings: fpga: machxo2-slave: add lattice,machxo2-slave-i2c compatible

[Johannes Zink]

Lattice MachXO2 FPGAs allow reconfiguration over I2C as well as over
SPI. Add the I2C option to the binding as well.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 .../bindings/fpga/lattice,machxo2-slave.yaml         | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
index 03dc134ec7b8..d48d92f27c92 100644
--- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
+++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
@@ -21,10 +21,22 @@ allOf:
             const: lattice,machxo2-slave-spi
     then:
       $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: lattice,machxo2-slave-i2c
+    then:
+      properties:
+        reg:
+          description: I2C address
+          maxItems: 1
+
 properties:
   compatible:
     enum:
       - lattice,machxo2-slave-spi
+      - lattice,machxo2-slave-i2c
 
   program-gpios:
     maxItems: 1
-- 
2.30.2

[PATCH 05/16] fpga: machxo2-spi: remove #ifdef DEBUG

[Johannes Zink]

This provides dynamic debug support, pr_debug checks anyway if DEBUG is
defined statically or is activated dynamically.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-spi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 905607992a12..39dd62359821 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -88,7 +88,6 @@ static int get_status(struct spi_device *spi, unsigned long *status)
 	return 0;
 }
 
-#ifdef DEBUG
 static const char *get_err_string(u8 err)
 {
 	switch (err) {
@@ -104,16 +103,13 @@ static const char *get_err_string(u8 err)
 
 	return "Default switch case";
 }
-#endif
 
 static void dump_status_reg(unsigned long *status)
 {
-#ifdef DEBUG
 	pr_debug("machxo2 status: 0x%08lX - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
 		 *status, test_bit(DONE, status), test_bit(ENAB, status),
 		 test_bit(BUSY, status), test_bit(FAIL, status),
 		 test_bit(DVER, status), get_err_string(get_err(status)));
-#endif
 }
 
 static int wait_until_not_busy(struct spi_device *spi)
-- 
2.30.2

[PATCH 06/16] fpga: machxo2-spi: factor out status check for readability

[Johannes Zink]

We have the same sequence at two different places, so factor it out
into a helper to improve readability.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-spi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 39dd62359821..5e12612c7289 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -167,14 +167,19 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
 	return ret;
 }
 
+static bool machxo2_status_done(unsigned long status)
+{
+	return !test_bit(BUSY, &status) && test_bit(DONE, &status) &&
+		get_err(&status) == ENOERR;
+}
+
 static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager *mgr)
 {
 	struct spi_device *spi = mgr->priv;
 	unsigned long status;
 
 	get_status(spi, &status);
-	if (!test_bit(BUSY, &status) && test_bit(DONE, &status) &&
-	    get_err(&status) == ENOERR)
+	if (machxo2_status_done(status))
 		return FPGA_MGR_STATE_OPERATING;
 
 	return FPGA_MGR_STATE_UNKNOWN;
@@ -329,8 +334,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 		/* check refresh status */
 		get_status(spi, &status);
 		dump_status_reg(&status);
-		if (!test_bit(BUSY, &status) && test_bit(DONE, &status) &&
-		    get_err(&status) == ENOERR)
+		if (machxo2_status_done(status))
 			break;
 		if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
 			machxo2_cleanup(mgr);
-- 
2.30.2

[PATCH 07/16] fpga: machxo2-spi: fix big-endianness incompatibility

[Johannes Zink]

The SPI message is written into the lowest-addressed bits of an
unsigned long variable, but be32_to_cpu is called on the least
significant bits of the variable. On big-endian 64-bit systems,
this would give a wrong result. Fix this by using the fixed-size
u32 instead of unsigned long. This clashes with the use of
test_bit, which is unnecessary for a single u32 variable, so
we adjust all usage sites appropriately and prefix the macros
with MACHXO2_ while at it.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-spi.c | 110 ++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 55 deletions(-)

diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 5e12612c7289..d1a8f28e04e7 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/bitfield.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -41,41 +42,40 @@
 #define MACHXO2_BUF_SIZE		(MACHXO2_PAGE_SIZE + 4)
 
 /* Status register bits, errors and error mask */
-#define BUSY	12
-#define DONE	8
-#define DVER	27
-#define ENAB	9
-#define ERRBITS	23
-#define ERRMASK	7
-#define FAIL	13
-
-#define ENOERR	0 /* no error */
-#define EID	1
-#define ECMD	2
-#define ECRC	3
-#define EPREAM	4 /* preamble error */
-#define EABRT	5 /* abort error */
-#define EOVERFL	6 /* overflow error */
-#define ESDMEOF	7 /* SDM EOF */
-
-static inline u8 get_err(unsigned long *status)
+#define MACHXO2_BUSY		BIT(12)
+#define MACHXO2_DONE		BIT(8)
+#define MACHXO2_DVER		BIT(27)
+#define MACHXO2_ENAB		BIT(9)
+#define MACHXO2_ERR		GENMASK(25, 23)
+#define MACHXO2_ERR_ENOERR	0 /* no error */
+#define MACHXO2_ERR_EID		1
+#define MACHXO2_ERR_ECMD	2
+#define MACHXO2_ERR_ECRC	3
+#define MACHXO2_ERR_EPREAM	4 /* preamble error */
+#define MACHXO2_ERR_EABRT	5 /* abort error */
+#define MACHXO2_ERR_EOVERFL	6 /* overflow error */
+#define MACHXO2_ERR_ESDMEOF	7 /* SDM EOF */
+#define MACHXO2_FAIL		BIT(13)
+
+static inline u8 get_err(u32 status)
 {
-	return (*status >> ERRBITS) & ERRMASK;
+	return FIELD_GET(MACHXO2_ERR, status);
 }
 
-static int get_status(struct spi_device *spi, unsigned long *status)
+static int get_status(struct spi_device *spi, u32 *status)
 {
 	struct spi_message msg;
 	struct spi_transfer rx, tx;
 	static const u8 cmd[] = LSC_READ_STATUS;
+	__be32 tmp;
 	int ret;
 
 	memset(&rx, 0, sizeof(rx));
 	memset(&tx, 0, sizeof(tx));
 	tx.tx_buf = cmd;
 	tx.len = sizeof(cmd);
-	rx.rx_buf = status;
-	rx.len = 4;
+	rx.rx_buf = &tmp;
+	rx.len = sizeof(tmp);
 	spi_message_init(&msg);
 	spi_message_add_tail(&tx, &msg);
 	spi_message_add_tail(&rx, &msg);
@@ -83,7 +83,7 @@ static int get_status(struct spi_device *spi, unsigned long *status)
 	if (ret)
 		return ret;
 
-	*status = be32_to_cpu(*status);
+	*status = be32_to_cpu(tmp);
 
 	return 0;
 }
@@ -91,30 +91,30 @@ static int get_status(struct spi_device *spi, unsigned long *status)
 static const char *get_err_string(u8 err)
 {
 	switch (err) {
-	case ENOERR:	return "No Error";
-	case EID:	return "ID ERR";
-	case ECMD:	return "CMD ERR";
-	case ECRC:	return "CRC ERR";
-	case EPREAM:	return "Preamble ERR";
-	case EABRT:	return "Abort ERR";
-	case EOVERFL:	return "Overflow ERR";
-	case ESDMEOF:	return "SDM EOF";
+	case MACHXO2_ERR_ENOERR:	return "No Error";
+	case MACHXO2_ERR_EID:		return "ID ERR";
+	case MACHXO2_ERR_ECMD:		return "CMD ERR";
+	case MACHXO2_ERR_ECRC:		return "CRC ERR";
+	case MACHXO2_ERR_EPREAM:	return "Preamble ERR";
+	case MACHXO2_ERR_EABRT:		return "Abort ERR";
+	case MACHXO2_ERR_EOVERFL:	return "Overflow ERR";
+	case MACHXO2_ERR_ESDMEOF:	return "SDM EOF";
 	}
 
-	return "Default switch case";
+	return "Unknown";
 }
 
-static void dump_status_reg(unsigned long *status)
+static void dump_status_reg(u32 status)
 {
-	pr_debug("machxo2 status: 0x%08lX - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
-		 *status, test_bit(DONE, status), test_bit(ENAB, status),
-		 test_bit(BUSY, status), test_bit(FAIL, status),
-		 test_bit(DVER, status), get_err_string(get_err(status)));
+	pr_debug("machxo2 status: 0x%08X - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
+		  status, !!FIELD_GET(MACHXO2_DONE, status), !!FIELD_GET(MACHXO2_ENAB, status),
+		  !!FIELD_GET(MACHXO2_BUSY, status), !!FIELD_GET(MACHXO2_FAIL, status),
+		  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
 }
 
 static int wait_until_not_busy(struct spi_device *spi)
 {
-	unsigned long status;
+	u32 status;
 	int ret, loop = 0;
 
 	do {
@@ -123,7 +123,7 @@ static int wait_until_not_busy(struct spi_device *spi)
 			return ret;
 		if (++loop >= MACHXO2_MAX_BUSY_LOOP)
 			return -EBUSY;
-	} while (test_bit(BUSY, &status));
+	} while (status & MACHXO2_BUSY);
 
 	return 0;
 }
@@ -169,14 +169,14 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
 
 static bool machxo2_status_done(unsigned long status)
 {
-	return !test_bit(BUSY, &status) && test_bit(DONE, &status) &&
-		get_err(&status) == ENOERR;
+	return (((status & (MACHXO2_BUSY | MACHXO2_DONE)) == MACHXO2_DONE) &&
+		get_err(status) == MACHXO2_ERR_ENOERR);
 }
 
 static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager *mgr)
 {
 	struct spi_device *spi = mgr->priv;
-	unsigned long status;
+	u32 status;
 
 	get_status(spi, &status);
 	if (machxo2_status_done(status))
@@ -195,7 +195,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	static const u8 enable[] = ISC_ENABLE;
 	static const u8 erase[] = ISC_ERASE;
 	static const u8 initaddr[] = LSC_INITADDRESS;
-	unsigned long status;
+	u32 status;
 	int ret;
 
 	if ((info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
@@ -205,7 +205,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	}
 
 	get_status(spi, &status);
-	dump_status_reg(&status);
+	dump_status_reg(status);
 	memset(tx, 0, sizeof(tx));
 	spi_message_init(&msg);
 	tx[0].tx_buf = &enable;
@@ -226,11 +226,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 		goto fail;
 
 	get_status(spi, &status);
-	if (test_bit(FAIL, &status)) {
+	if (status & MACHXO2_FAIL) {
 		ret = -EINVAL;
 		goto fail;
 	}
-	dump_status_reg(&status);
+	dump_status_reg(status);
 
 	spi_message_init(&msg);
 	tx[2].tx_buf = &initaddr;
@@ -241,7 +241,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 		goto fail;
 
 	get_status(spi, &status);
-	dump_status_reg(&status);
+	dump_status_reg(status);
 
 	return 0;
 fail:
@@ -258,7 +258,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 	struct spi_transfer tx;
 	static const u8 progincr[] = LSC_PROGINCRNV;
 	u8 payload[MACHXO2_BUF_SIZE];
-	unsigned long status;
+	u32 status;
 	int i, ret;
 
 	if (count % MACHXO2_PAGE_SIZE != 0) {
@@ -266,7 +266,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 		return -EINVAL;
 	}
 	get_status(spi, &status);
-	dump_status_reg(&status);
+	dump_status_reg(status);
 	memcpy(payload, &progincr, sizeof(progincr));
 	for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
 		memcpy(&payload[sizeof(progincr)], &buf[i], MACHXO2_PAGE_SIZE);
@@ -284,7 +284,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 		}
 	}
 	get_status(spi, &status);
-	dump_status_reg(&status);
+	dump_status_reg(status);
 
 	return 0;
 }
@@ -297,7 +297,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 	struct spi_transfer tx[2];
 	static const u8 progdone[] = ISC_PROGRAMDONE;
 	static const u8 refresh[] = LSC_REFRESH;
-	unsigned long status;
+	u32 status;
 	int ret, refreshloop = 0;
 
 	memset(tx, 0, sizeof(tx));
@@ -313,8 +313,8 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 		goto fail;
 
 	get_status(spi, &status);
-	dump_status_reg(&status);
-	if (!test_bit(DONE, &status)) {
+	dump_status_reg(status);
+	if (!(status & MACHXO2_DONE)) {
 		machxo2_cleanup(mgr);
 		ret = -EINVAL;
 		goto fail;
@@ -333,7 +333,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 
 		/* check refresh status */
 		get_status(spi, &status);
-		dump_status_reg(&status);
+		dump_status_reg(status);
 		if (machxo2_status_done(status))
 			break;
 		if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
@@ -344,7 +344,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 	} while (1);
 
 	get_status(spi, &status);
-	dump_status_reg(&status);
+	dump_status_reg(status);
 
 	return 0;
 fail:
-- 
2.30.2

[PATCH 08/16] fpga: machxo2-spi: simplify with spi_sync_transfer()

[Johannes Zink]

Use helper functions in order to improve readability.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-spi.c | 96 ++++++++++++++------------------------
 1 file changed, 36 insertions(+), 60 deletions(-)

diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index d1a8f28e04e7..7f7d1066ddee 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -64,22 +64,17 @@ static inline u8 get_err(u32 status)
 
 static int get_status(struct spi_device *spi, u32 *status)
 {
-	struct spi_message msg;
-	struct spi_transfer rx, tx;
+	struct spi_transfer transfers[2] = {};
 	static const u8 cmd[] = LSC_READ_STATUS;
 	__be32 tmp;
 	int ret;
 
-	memset(&rx, 0, sizeof(rx));
-	memset(&tx, 0, sizeof(tx));
-	tx.tx_buf = cmd;
-	tx.len = sizeof(cmd);
-	rx.rx_buf = &tmp;
-	rx.len = sizeof(tmp);
-	spi_message_init(&msg);
-	spi_message_add_tail(&tx, &msg);
-	spi_message_add_tail(&rx, &msg);
-	ret = spi_sync(spi, &msg);
+	transfers[0].tx_buf = cmd;
+	transfers[0].len = sizeof(cmd);
+	transfers[1].rx_buf = &tmp;
+	transfers[1].len = sizeof(tmp);
+
+	ret = spi_sync_transfer(spi, transfers, ARRAY_SIZE(transfers));
 	if (ret)
 		return ret;
 
@@ -131,18 +126,14 @@ static int wait_until_not_busy(struct spi_device *spi)
 static int machxo2_cleanup(struct fpga_manager *mgr)
 {
 	struct spi_device *spi = mgr->priv;
-	struct spi_message msg;
-	struct spi_transfer tx[2];
 	static const u8 erase[] = ISC_ERASE;
 	static const u8 refresh[] = LSC_REFRESH;
+	struct spi_transfer tx = {};
 	int ret;
 
-	memset(tx, 0, sizeof(tx));
-	spi_message_init(&msg);
-	tx[0].tx_buf = &erase;
-	tx[0].len = sizeof(erase);
-	spi_message_add_tail(&tx[0], &msg);
-	ret = spi_sync(spi, &msg);
+	tx.tx_buf = &erase;
+	tx.len = sizeof(erase);
+	ret = spi_sync_transfer(spi, &tx, 1);
 	if (ret)
 		goto fail;
 
@@ -150,13 +141,11 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
 	if (ret)
 		goto fail;
 
-	spi_message_init(&msg);
-	tx[1].tx_buf = &refresh;
-	tx[1].len = sizeof(refresh);
-	tx[1].delay.value = MACHXO2_REFRESH_USEC;
-	tx[1].delay.unit = SPI_DELAY_UNIT_USECS;
-	spi_message_add_tail(&tx[1], &msg);
-	ret = spi_sync(spi, &msg);
+	tx.tx_buf = &refresh;
+	tx.len = sizeof(refresh);
+	tx.delay.value = MACHXO2_REFRESH_USEC;
+	tx.delay.unit = SPI_DELAY_UNIT_USECS;
+	ret = spi_sync_transfer(spi, &tx, 1);
 	if (ret)
 		goto fail;
 
@@ -190,8 +179,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 			      const char *buf, size_t count)
 {
 	struct spi_device *spi = mgr->priv;
-	struct spi_message msg;
-	struct spi_transfer tx[3];
+	struct spi_transfer tx[2] = {};
 	static const u8 enable[] = ISC_ENABLE;
 	static const u8 erase[] = ISC_ERASE;
 	static const u8 initaddr[] = LSC_INITADDRESS;
@@ -206,18 +194,15 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 
 	get_status(spi, &status);
 	dump_status_reg(status);
-	memset(tx, 0, sizeof(tx));
-	spi_message_init(&msg);
+
 	tx[0].tx_buf = &enable;
 	tx[0].len = sizeof(enable);
 	tx[0].delay.value = MACHXO2_LOW_DELAY_USEC;
 	tx[0].delay.unit = SPI_DELAY_UNIT_USECS;
-	spi_message_add_tail(&tx[0], &msg);
 
 	tx[1].tx_buf = &erase;
 	tx[1].len = sizeof(erase);
-	spi_message_add_tail(&tx[1], &msg);
-	ret = spi_sync(spi, &msg);
+	ret = spi_sync_transfer(spi, tx, ARRAY_SIZE(tx));
 	if (ret)
 		goto fail;
 
@@ -232,11 +217,9 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	}
 	dump_status_reg(status);
 
-	spi_message_init(&msg);
-	tx[2].tx_buf = &initaddr;
-	tx[2].len = sizeof(initaddr);
-	spi_message_add_tail(&tx[2], &msg);
-	ret = spi_sync(spi, &msg);
+	tx[0].tx_buf = &initaddr;
+	tx[0].len = sizeof(initaddr);
+	ret = spi_sync_transfer(spi, &tx[0], 1);
 	if (ret)
 		goto fail;
 
@@ -254,8 +237,6 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 			 size_t count)
 {
 	struct spi_device *spi = mgr->priv;
-	struct spi_message msg;
-	struct spi_transfer tx;
 	static const u8 progincr[] = LSC_PROGINCRNV;
 	u8 payload[MACHXO2_BUF_SIZE];
 	u32 status;
@@ -269,15 +250,15 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 	dump_status_reg(status);
 	memcpy(payload, &progincr, sizeof(progincr));
 	for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
+		struct spi_transfer tx = {};
+
 		memcpy(&payload[sizeof(progincr)], &buf[i], MACHXO2_PAGE_SIZE);
-		memset(&tx, 0, sizeof(tx));
-		spi_message_init(&msg);
+
 		tx.tx_buf = payload;
 		tx.len = MACHXO2_BUF_SIZE;
 		tx.delay.value = MACHXO2_HIGH_DELAY_USEC;
 		tx.delay.unit = SPI_DELAY_UNIT_USECS;
-		spi_message_add_tail(&tx, &msg);
-		ret = spi_sync(spi, &msg);
+		ret = spi_sync_transfer(spi, &tx, 1);
 		if (ret) {
 			dev_err(&mgr->dev, "Error loading the bitstream.\n");
 			return ret;
@@ -293,19 +274,15 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 				  struct fpga_image_info *info)
 {
 	struct spi_device *spi = mgr->priv;
-	struct spi_message msg;
-	struct spi_transfer tx[2];
+	struct spi_transfer tx = {};
 	static const u8 progdone[] = ISC_PROGRAMDONE;
 	static const u8 refresh[] = LSC_REFRESH;
 	u32 status;
 	int ret, refreshloop = 0;
 
-	memset(tx, 0, sizeof(tx));
-	spi_message_init(&msg);
-	tx[0].tx_buf = &progdone;
-	tx[0].len = sizeof(progdone);
-	spi_message_add_tail(&tx[0], &msg);
-	ret = spi_sync(spi, &msg);
+	tx.tx_buf = &progdone;
+	tx.len = sizeof(progdone);
+	ret = spi_sync_transfer(spi, &tx, 1);
 	if (ret)
 		goto fail;
 	ret = wait_until_not_busy(spi);
@@ -320,14 +297,13 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 		goto fail;
 	}
 
+	tx.tx_buf = &refresh;
+	tx.len = sizeof(refresh);
+	tx.delay.value = MACHXO2_REFRESH_USEC;
+	tx.delay.unit = SPI_DELAY_UNIT_USECS;
+
 	do {
-		spi_message_init(&msg);
-		tx[1].tx_buf = &refresh;
-		tx[1].len = sizeof(refresh);
-		tx[1].delay.value = MACHXO2_REFRESH_USEC;
-		tx[1].delay.unit = SPI_DELAY_UNIT_USECS;
-		spi_message_add_tail(&tx[1], &msg);
-		ret = spi_sync(spi, &msg);
+		ret = spi_sync_transfer(spi, &tx, 1);
 		if (ret)
 			goto fail;
 
-- 
2.30.2

[PATCH 09/16] fpga: machxo2-spi: simplify spi write commands

[Johannes Zink]

Refactor the spi transfer preparation into a separate function. This
commit prepares moving the non-spi-specific part of the programming
sequence into a separate file.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-spi.c | 128 +++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 49 deletions(-)

diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 7f7d1066ddee..d696b1cfb18a 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -65,7 +65,7 @@ static inline u8 get_err(u32 status)
 static int get_status(struct spi_device *spi, u32 *status)
 {
 	struct spi_transfer transfers[2] = {};
-	static const u8 cmd[] = LSC_READ_STATUS;
+	u8 cmd[] = LSC_READ_STATUS;
 	__be32 tmp;
 	int ret;
 
@@ -107,7 +107,7 @@ static void dump_status_reg(u32 status)
 		  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
 }
 
-static int wait_until_not_busy(struct spi_device *spi)
+static int machxo2_wait_until_not_busy(struct spi_device *spi)
 {
 	u32 status;
 	int ret, loop = 0;
@@ -123,29 +123,57 @@ static int wait_until_not_busy(struct spi_device *spi)
 	return 0;
 }
 
+struct machxo2_cmd {
+	u8 *cmd;
+	size_t cmd_len;
+	u16 delay_us;
+};
+
+static int machxo2_write_spi(struct spi_device *spi, struct machxo2_cmd *cmds, size_t cmd_count)
+{
+	struct spi_transfer *transfers;
+	int i, ret;
+
+	transfers = kcalloc(cmd_count, sizeof(*transfers), GFP_KERNEL);
+	for (i = 0; i < cmd_count; i++) {
+		transfers[i].tx_buf = cmds[i].cmd;
+		transfers[i].len = cmds[i].cmd_len;
+
+		if (cmds[i].delay_us) {
+			transfers[i].delay.value = cmds[i].delay_us;
+			transfers[i].delay.unit = SPI_DELAY_UNIT_USECS;
+		}
+	}
+
+	ret = spi_sync_transfer(spi, transfers, cmd_count);
+
+	kfree(transfers);
+
+	return ret;
+}
+
 static int machxo2_cleanup(struct fpga_manager *mgr)
 {
 	struct spi_device *spi = mgr->priv;
-	static const u8 erase[] = ISC_ERASE;
-	static const u8 refresh[] = LSC_REFRESH;
-	struct spi_transfer tx = {};
+	u8 erase[] = ISC_ERASE;
+	u8 refresh[] = LSC_REFRESH;
+	struct machxo2_cmd cmd = {};
 	int ret;
 
-	tx.tx_buf = &erase;
-	tx.len = sizeof(erase);
-	ret = spi_sync_transfer(spi, &tx, 1);
+	cmd.cmd = erase;
+	cmd.cmd_len = sizeof(erase);
+	ret = machxo2_write_spi(spi, &cmd, 1);
 	if (ret)
 		goto fail;
 
-	ret = wait_until_not_busy(spi);
+	ret = machxo2_wait_until_not_busy(spi);
 	if (ret)
 		goto fail;
 
-	tx.tx_buf = &refresh;
-	tx.len = sizeof(refresh);
-	tx.delay.value = MACHXO2_REFRESH_USEC;
-	tx.delay.unit = SPI_DELAY_UNIT_USECS;
-	ret = spi_sync_transfer(spi, &tx, 1);
+	cmd.cmd = refresh;
+	cmd.cmd_len = sizeof(refresh);
+	cmd.delay_us = MACHXO2_REFRESH_USEC;
+	ret = machxo2_write_spi(spi, &cmd, 1);
 	if (ret)
 		goto fail;
 
@@ -179,10 +207,10 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 			      const char *buf, size_t count)
 {
 	struct spi_device *spi = mgr->priv;
-	struct spi_transfer tx[2] = {};
-	static const u8 enable[] = ISC_ENABLE;
-	static const u8 erase[] = ISC_ERASE;
-	static const u8 initaddr[] = LSC_INITADDRESS;
+	u8 enable[] = ISC_ENABLE;
+	u8 erase[] = ISC_ERASE;
+	u8 initaddr[] = LSC_INITADDRESS;
+	struct machxo2_cmd cmd[2] = {};
 	u32 status;
 	int ret;
 
@@ -195,18 +223,18 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	get_status(spi, &status);
 	dump_status_reg(status);
 
-	tx[0].tx_buf = &enable;
-	tx[0].len = sizeof(enable);
-	tx[0].delay.value = MACHXO2_LOW_DELAY_USEC;
-	tx[0].delay.unit = SPI_DELAY_UNIT_USECS;
+	cmd[0].cmd = enable;
+	cmd[0].cmd_len = sizeof(enable);
+	cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC;
+
+	cmd[1].cmd = erase;
+	cmd[1].cmd_len = sizeof(erase);
+	ret = machxo2_write_spi(spi, cmd, ARRAY_SIZE(cmd));
 
-	tx[1].tx_buf = &erase;
-	tx[1].len = sizeof(erase);
-	ret = spi_sync_transfer(spi, tx, ARRAY_SIZE(tx));
 	if (ret)
 		goto fail;
 
-	ret = wait_until_not_busy(spi);
+	ret = machxo2_wait_until_not_busy(spi);
 	if (ret)
 		goto fail;
 
@@ -217,9 +245,9 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	}
 	dump_status_reg(status);
 
-	tx[0].tx_buf = &initaddr;
-	tx[0].len = sizeof(initaddr);
-	ret = spi_sync_transfer(spi, &tx[0], 1);
+	cmd[0].cmd = initaddr;
+	cmd[0].cmd_len = sizeof(initaddr);
+	ret = machxo2_write_spi(spi, &cmd[0], 1);
 	if (ret)
 		goto fail;
 
@@ -237,8 +265,9 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 			 size_t count)
 {
 	struct spi_device *spi = mgr->priv;
-	static const u8 progincr[] = LSC_PROGINCRNV;
+	u8 progincr[] = LSC_PROGINCRNV;
 	u8 payload[MACHXO2_BUF_SIZE];
+	struct machxo2_cmd cmd = {};
 	u32 status;
 	int i, ret;
 
@@ -248,17 +277,19 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 	}
 	get_status(spi, &status);
 	dump_status_reg(status);
+
+	cmd.cmd = payload;
+	cmd.cmd_len = MACHXO2_BUF_SIZE;
+	cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
+
 	memcpy(payload, &progincr, sizeof(progincr));
 	for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
-		struct spi_transfer tx = {};
-
 		memcpy(&payload[sizeof(progincr)], &buf[i], MACHXO2_PAGE_SIZE);
 
-		tx.tx_buf = payload;
-		tx.len = MACHXO2_BUF_SIZE;
-		tx.delay.value = MACHXO2_HIGH_DELAY_USEC;
-		tx.delay.unit = SPI_DELAY_UNIT_USECS;
-		ret = spi_sync_transfer(spi, &tx, 1);
+		cmd.cmd = payload;
+		cmd.cmd_len = MACHXO2_BUF_SIZE;
+		cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
+		ret = machxo2_write_spi(spi, &cmd, 1);
 		if (ret) {
 			dev_err(&mgr->dev, "Error loading the bitstream.\n");
 			return ret;
@@ -274,18 +305,18 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 				  struct fpga_image_info *info)
 {
 	struct spi_device *spi = mgr->priv;
-	struct spi_transfer tx = {};
-	static const u8 progdone[] = ISC_PROGRAMDONE;
-	static const u8 refresh[] = LSC_REFRESH;
+	struct machxo2_cmd cmd = {};
+	u8 progdone[] = ISC_PROGRAMDONE;
+	u8 refresh[] = LSC_REFRESH;
 	u32 status;
 	int ret, refreshloop = 0;
 
-	tx.tx_buf = &progdone;
-	tx.len = sizeof(progdone);
-	ret = spi_sync_transfer(spi, &tx, 1);
+	cmd.cmd = progdone;
+	cmd.cmd_len = sizeof(progdone);
+	ret = machxo2_write_spi(spi, &cmd, 1);
 	if (ret)
 		goto fail;
-	ret = wait_until_not_busy(spi);
+	ret = machxo2_wait_until_not_busy(spi);
 	if (ret)
 		goto fail;
 
@@ -297,13 +328,12 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 		goto fail;
 	}
 
-	tx.tx_buf = &refresh;
-	tx.len = sizeof(refresh);
-	tx.delay.value = MACHXO2_REFRESH_USEC;
-	tx.delay.unit = SPI_DELAY_UNIT_USECS;
+	cmd.cmd = refresh;
+	cmd.cmd_len = sizeof(refresh);
+	cmd.delay_us = MACHXO2_REFRESH_USEC;
 
 	do {
-		ret = spi_sync_transfer(spi, &tx, 1);
+		ret = machxo2_write_spi(spi, &cmd, 1);
 		if (ret)
 			goto fail;
 
-- 
2.30.2

[PATCH 10/16] fpga: machxo2-spi: prepare extraction of common code

[Johannes Zink]

This commit introduces a machxo2_common_priv structure which is used
instead of holding the driver's spi_device directly as priv member of the
fpga_mgr structure.

Additionally it serves as a container for a machxo2_spi_priv
struct, this prepares the addition of i2c as another bus in a later commit.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-spi.c | 80 +++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 28 deletions(-)

diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index d696b1cfb18a..5f1d6505f828 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/spi/spi.h>
+#include <linux/container_of.h>
 
 /* MachXO2 Programming Guide - sysCONFIG Programming Commands */
 #define IDCODE_PUB		{0xe0, 0x00, 0x00, 0x00}
@@ -57,13 +58,27 @@
 #define MACHXO2_ERR_ESDMEOF	7 /* SDM EOF */
 #define MACHXO2_FAIL		BIT(13)
 
+struct machxo2_common_priv {
+};
+
+struct machxo2_spi_priv {
+	struct machxo2_common_priv common;
+	struct spi_device *spi;
+};
+
+static inline struct machxo2_spi_priv *to_machxo2_spi_priv(struct machxo2_common_priv *priv)
+{
+	return container_of(priv, struct machxo2_spi_priv, common);
+}
+
 static inline u8 get_err(u32 status)
 {
 	return FIELD_GET(MACHXO2_ERR, status);
 }
 
-static int get_status(struct spi_device *spi, u32 *status)
+static int get_status(struct machxo2_common_priv *priv, u32 *status)
 {
+	struct spi_device *spi = to_machxo2_spi_priv(priv)->spi;
 	struct spi_transfer transfers[2] = {};
 	u8 cmd[] = LSC_READ_STATUS;
 	__be32 tmp;
@@ -107,13 +122,13 @@ static void dump_status_reg(u32 status)
 		  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
 }
 
-static int machxo2_wait_until_not_busy(struct spi_device *spi)
+static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
 {
 	u32 status;
 	int ret, loop = 0;
 
 	do {
-		ret = get_status(spi, &status);
+		ret = get_status(priv, &status);
 		if (ret)
 			return ret;
 		if (++loop >= MACHXO2_MAX_BUSY_LOOP)
@@ -129,8 +144,10 @@ struct machxo2_cmd {
 	u16 delay_us;
 };
 
-static int machxo2_write_spi(struct spi_device *spi, struct machxo2_cmd *cmds, size_t cmd_count)
+static int machxo2_write_spi(struct machxo2_common_priv *priv,
+			     struct machxo2_cmd *cmds, size_t cmd_count)
 {
+	struct spi_device *spi = to_machxo2_spi_priv(priv)->spi;
 	struct spi_transfer *transfers;
 	int i, ret;
 
@@ -154,7 +171,7 @@ static int machxo2_write_spi(struct spi_device *spi, struct machxo2_cmd *cmds, s
 
 static int machxo2_cleanup(struct fpga_manager *mgr)
 {
-	struct spi_device *spi = mgr->priv;
+	struct machxo2_common_priv *priv = mgr->priv;
 	u8 erase[] = ISC_ERASE;
 	u8 refresh[] = LSC_REFRESH;
 	struct machxo2_cmd cmd = {};
@@ -162,18 +179,18 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
 
 	cmd.cmd = erase;
 	cmd.cmd_len = sizeof(erase);
-	ret = machxo2_write_spi(spi, &cmd, 1);
+	ret = machxo2_write_spi(priv, &cmd, 1);
 	if (ret)
 		goto fail;
 
-	ret = machxo2_wait_until_not_busy(spi);
+	ret = machxo2_wait_until_not_busy(priv);
 	if (ret)
 		goto fail;
 
 	cmd.cmd = refresh;
 	cmd.cmd_len = sizeof(refresh);
 	cmd.delay_us = MACHXO2_REFRESH_USEC;
-	ret = machxo2_write_spi(spi, &cmd, 1);
+	ret = machxo2_write_spi(priv, &cmd, 1);
 	if (ret)
 		goto fail;
 
@@ -192,10 +209,10 @@ static bool machxo2_status_done(unsigned long status)
 
 static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager *mgr)
 {
-	struct spi_device *spi = mgr->priv;
+	struct machxo2_common_priv *priv = mgr->priv;
 	u32 status;
 
-	get_status(spi, &status);
+	get_status(priv, &status);
 	if (machxo2_status_done(status))
 		return FPGA_MGR_STATE_OPERATING;
 
@@ -206,7 +223,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 			      struct fpga_image_info *info,
 			      const char *buf, size_t count)
 {
-	struct spi_device *spi = mgr->priv;
+	struct machxo2_common_priv *priv = mgr->priv;
 	u8 enable[] = ISC_ENABLE;
 	u8 erase[] = ISC_ERASE;
 	u8 initaddr[] = LSC_INITADDRESS;
@@ -220,7 +237,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 		return -ENOTSUPP;
 	}
 
-	get_status(spi, &status);
+	get_status(priv, &status);
 	dump_status_reg(status);
 
 	cmd[0].cmd = enable;
@@ -234,11 +251,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	if (ret)
 		goto fail;
 
-	ret = machxo2_wait_until_not_busy(spi);
+	ret = machxo2_wait_until_not_busy(priv);
 	if (ret)
 		goto fail;
 
-	get_status(spi, &status);
+	get_status(priv, &status);
 	if (status & MACHXO2_FAIL) {
 		ret = -EINVAL;
 		goto fail;
@@ -247,11 +264,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 
 	cmd[0].cmd = initaddr;
 	cmd[0].cmd_len = sizeof(initaddr);
-	ret = machxo2_write_spi(spi, &cmd[0], 1);
+	ret = machxo2_write_spi(priv, &cmd[0], 1);
 	if (ret)
 		goto fail;
 
-	get_status(spi, &status);
+	get_status(priv, &status);
 	dump_status_reg(status);
 
 	return 0;
@@ -264,7 +281,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 			 size_t count)
 {
-	struct spi_device *spi = mgr->priv;
+	struct machxo2_common_priv *priv = mgr->priv;
 	u8 progincr[] = LSC_PROGINCRNV;
 	u8 payload[MACHXO2_BUF_SIZE];
 	struct machxo2_cmd cmd = {};
@@ -275,7 +292,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 		dev_err(&mgr->dev, "Malformed payload.\n");
 		return -EINVAL;
 	}
-	get_status(spi, &status);
+	get_status(priv, &status);
 	dump_status_reg(status);
 
 	cmd.cmd = payload;
@@ -289,13 +306,13 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 		cmd.cmd = payload;
 		cmd.cmd_len = MACHXO2_BUF_SIZE;
 		cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
-		ret = machxo2_write_spi(spi, &cmd, 1);
+		ret = machxo2_write_spi(priv, &cmd, 1);
 		if (ret) {
 			dev_err(&mgr->dev, "Error loading the bitstream.\n");
 			return ret;
 		}
 	}
-	get_status(spi, &status);
+	get_status(priv, &status);
 	dump_status_reg(status);
 
 	return 0;
@@ -304,7 +321,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 static int machxo2_write_complete(struct fpga_manager *mgr,
 				  struct fpga_image_info *info)
 {
-	struct spi_device *spi = mgr->priv;
+	struct machxo2_common_priv *priv = mgr->priv;
 	struct machxo2_cmd cmd = {};
 	u8 progdone[] = ISC_PROGRAMDONE;
 	u8 refresh[] = LSC_REFRESH;
@@ -313,14 +330,14 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 
 	cmd.cmd = progdone;
 	cmd.cmd_len = sizeof(progdone);
-	ret = machxo2_write_spi(spi, &cmd, 1);
+	ret = machxo2_write_spi(priv, &cmd, 1);
 	if (ret)
 		goto fail;
-	ret = machxo2_wait_until_not_busy(spi);
+	ret = machxo2_wait_until_not_busy(priv);
 	if (ret)
 		goto fail;
 
-	get_status(spi, &status);
+	get_status(priv, &status);
 	dump_status_reg(status);
 	if (!(status & MACHXO2_DONE)) {
 		machxo2_cleanup(mgr);
@@ -333,12 +350,12 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 	cmd.delay_us = MACHXO2_REFRESH_USEC;
 
 	do {
-		ret = machxo2_write_spi(spi, &cmd, 1);
+		ret = machxo2_write_spi(priv, &cmd, 1);
 		if (ret)
 			goto fail;
 
 		/* check refresh status */
-		get_status(spi, &status);
+		get_status(priv, &status);
 		dump_status_reg(status);
 		if (machxo2_status_done(status))
 			break;
@@ -349,7 +366,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 		}
 	} while (1);
 
-	get_status(spi, &status);
+	get_status(priv, &status);
 	dump_status_reg(status);
 
 	return 0;
@@ -368,6 +385,7 @@ static const struct fpga_manager_ops machxo2_ops = {
 
 static int machxo2_spi_probe(struct spi_device *spi)
 {
+	struct machxo2_spi_priv *priv;
 	struct device *dev = &spi->dev;
 	struct fpga_manager *mgr;
 
@@ -376,8 +394,14 @@ static int machxo2_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->spi = spi;
+
 	mgr = devm_fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager",
-				     &machxo2_ops, spi);
+				     &machxo2_ops, &priv->common);
 	return PTR_ERR_OR_ZERO(mgr);
 }
 
-- 
2.30.2

[PATCH 11/16] fpga: machxo2: move non-spi-related functionality to common code

[Johannes Zink]

This commit seperates the general programming algorithm from
bus-specific access functions. This is a preparation for adding i2c as
another bus.

While at it: rename some functions to allow easier separation between
spi-specific functions and common code.

While at it: clean up includes

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/Kconfig          |   6 +
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/machxo2-common.c | 310 +++++++++++++++++++++++++++++++++
 drivers/fpga/machxo2-common.h |  37 ++++
 drivers/fpga/machxo2-spi.c    | 318 +---------------------------------
 5 files changed, 359 insertions(+), 313 deletions(-)
 create mode 100644 drivers/fpga/machxo2-common.c
 create mode 100644 drivers/fpga/machxo2-common.h

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 6c416955da53..e5869a732246 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -77,9 +77,15 @@ config FPGA_MGR_ICE40_SPI
 	help
 	  FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
 
+config FPGA_MGR_MACHXO2_COMMON
+	tristate
+	help
+	  FPGA manager driver common code for Lattice MachXO2 configuration
+
 config FPGA_MGR_MACHXO2_SPI
 	tristate "Lattice MachXO2 SPI"
 	depends on SPI
+	select FPGA_MGR_MACHXO2_COMMON
 	help
 	  FPGA manager driver support for Lattice MachXO2 configuration
 	  over slave SPI interface.
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 42ae8b58abce..f247a8de83ad 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)	+= altera-cvp.o
 obj-$(CONFIG_FPGA_MGR_ALTERA_PS_SPI)	+= altera-ps-spi.o
 obj-$(CONFIG_FPGA_MGR_ICE40_SPI)	+= ice40-spi.o
+obj-$(CONFIG_FPGA_MGR_MACHXO2_COMMON)	+= machxo2-common.o
 obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI)	+= machxo2-spi.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
new file mode 100644
index 000000000000..33127ee67d19
--- /dev/null
+++ b/drivers/fpga/machxo2-common.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Lattice MachXO2 Common Driver
+ *
+ * Manage Lattice FPGA firmware
+ *
+ * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com>
+ * Copyright (C) 2022 Pengutronix, Johannes Zink <kernel@pengutronix.de>
+ */
+
+#include <linux/delay.h>
+#include <linux/bitfield.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include "machxo2-common.h"
+
+#define MACHXO2_LOW_DELAY_USEC          5
+#define MACHXO2_HIGH_DELAY_USEC         200
+#define MACHXO2_REFRESH_USEC            4800
+#define MACHXO2_MAX_BUSY_LOOP           128
+#define MACHXO2_MAX_REFRESH_LOOP        16
+
+#define MACHXO2_PAGE_SIZE               16
+#define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
+
+/* Status register bits, errors and error mask */
+#define MACHXO2_BUSY		BIT(12)
+#define MACHXO2_DONE		BIT(8)
+#define MACHXO2_DVER		BIT(27)
+#define MACHXO2_ENAB		BIT(9)
+#define MACHXO2_ERR		GENMASK(25, 23)
+#define MACHXO2_ERR_ENOERR	0 /* no error */
+#define MACHXO2_ERR_EEID	1
+#define MACHXO2_ERR_EECMD	2
+#define MACHXO2_ERR_EECRC	3
+#define MACHXO2_ERR_EEPREAM	4 /* preamble error */
+#define MACHXO2_ERR_EEABRT	5 /* abort error */
+#define MACHXO2_ERR_EEOVERFL	6 /* overflow error */
+#define MACHXO2_ERR_EESDMEOF	7 /* SDM EOF */
+#define MACHXO2_FAIL		BIT(13)
+
+
+static inline u8 get_err(u32 status)
+{
+	return FIELD_GET(MACHXO2_ERR, status);
+}
+
+static const char *get_err_string(u8 err)
+{
+	switch (err) {
+	case MACHXO2_ERR_ENOERR:	return "No Error";
+	case MACHXO2_ERR_EEID:		return "ID ERR";
+	case MACHXO2_ERR_EECMD:		return "CMD ERR";
+	case MACHXO2_ERR_EECRC:		return "CRC ERR";
+	case MACHXO2_ERR_EEPREAM:	return "Preamble ERR";
+	case MACHXO2_ERR_EEABRT:	return "Abort ERR";
+	case MACHXO2_ERR_EEOVERFL:	return "Overflow ERR";
+	case MACHXO2_ERR_EESDMEOF:	return "SDM EOF";
+	}
+
+	return "Unknown";
+}
+
+static void dump_status_reg(u32 status)
+{
+        pr_debug("machxo2 status: 0x%08X - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
+                  status, !!FIELD_GET(MACHXO2_DONE, status), !!FIELD_GET(MACHXO2_ENAB, status),
+                  !!FIELD_GET(MACHXO2_BUSY, status), !!FIELD_GET(MACHXO2_FAIL, status),
+                  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
+}
+
+static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
+{
+	u32 status;
+	int ret, loop = 0;
+
+	do {
+		ret = priv->get_status(priv, &status);
+		if (ret)
+			return ret;
+		if (++loop >= MACHXO2_MAX_BUSY_LOOP)
+			return -EBUSY;
+	} while (status & MACHXO2_BUSY);
+
+	return 0;
+}
+
+static int machxo2_cleanup(struct fpga_manager *mgr)
+{
+	struct machxo2_common_priv *priv = mgr->priv;
+	u8 erase[] = ISC_ERASE;
+	u8 refresh[] = LSC_REFRESH;
+	struct machxo2_cmd cmd = {};
+	int ret;
+
+	cmd.cmd = erase;
+	cmd.cmd_len = sizeof(erase);
+	ret = priv->write_commands(priv, &cmd, 1);
+	if (ret)
+		goto fail;
+
+	ret = machxo2_wait_until_not_busy(priv);
+	if (ret)
+		goto fail;
+
+	cmd.cmd = refresh;
+	cmd.cmd_len = sizeof(refresh);
+	cmd.delay_us = MACHXO2_REFRESH_USEC;
+	ret = priv->write_commands(priv, &cmd, 1);
+	if (ret)
+		goto fail;
+
+	return 0;
+fail:
+	dev_err(&mgr->dev, "Cleanup failed\n");
+
+	return ret;
+}
+
+static bool machxo2_status_done(unsigned long status)
+{
+	return (((status & (MACHXO2_BUSY | MACHXO2_DONE)) == MACHXO2_DONE) &&
+		get_err(status) == MACHXO2_ERR_ENOERR);
+}
+
+static enum fpga_mgr_states machxo2_state(struct fpga_manager *mgr)
+{
+	struct machxo2_common_priv *priv = mgr->priv;
+	u32 status;
+
+	priv->get_status(priv, &status);
+	if (machxo2_status_done(status))
+		return FPGA_MGR_STATE_OPERATING;
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static int machxo2_write_init(struct fpga_manager *mgr,
+			      struct fpga_image_info *info,
+			      const char *buf, size_t count)
+{
+	struct machxo2_common_priv *priv = mgr->priv;
+	u8 enable[] = ISC_ENABLE;
+	u8 erase[] = ISC_ERASE;
+	u8 initaddr[] = LSC_INITADDRESS;
+	struct machxo2_cmd cmd[2] = {};
+	u32 status;
+	int ret;
+
+	if ((info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+		dev_err(&mgr->dev,
+			"Partial reconfiguration is not supported\n");
+		return -ENOTSUPP;
+	}
+
+	priv->get_status(priv, &status);
+	dump_status_reg(status);
+
+	cmd[0].cmd = enable;
+	cmd[0].cmd_len = sizeof(enable);
+	cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC;
+
+	cmd[1].cmd = erase;
+	cmd[1].cmd_len = sizeof(erase);
+	ret = priv->write_commands(priv, cmd, 2);
+	if (ret)
+		goto fail;
+
+	ret = machxo2_wait_until_not_busy(priv);
+	if (ret)
+		goto fail;
+
+	priv->get_status(priv, &status);
+	if (status & MACHXO2_FAIL) {
+		ret = -EINVAL;
+		goto fail;
+	}
+	dump_status_reg(status);
+
+	cmd[0].cmd = initaddr;
+	cmd[0].cmd_len = sizeof(initaddr);
+	ret = priv->write_commands(priv, cmd, 1);
+	if (ret)
+		goto fail;
+
+	priv->get_status(priv, &status);
+	dump_status_reg(status);
+
+	return 0;
+fail:
+	dev_err(&mgr->dev, "Error during FPGA init.\n");
+
+	return ret;
+}
+
+static int machxo2_write(struct fpga_manager *mgr, const char *buf,
+			 size_t count)
+{
+	struct machxo2_common_priv *priv = mgr->priv;
+	u8 progincr[] = LSC_PROGINCRNV;
+	u8 payload[MACHXO2_BUF_SIZE];
+	struct machxo2_cmd cmd = {};
+	u32 status;
+	int i, ret;
+
+	if (count % MACHXO2_PAGE_SIZE != 0) {
+		dev_err(&mgr->dev, "Malformed payload.\n");
+		return -EINVAL;
+	}
+	priv->get_status(priv, &status);
+	dump_status_reg(status);
+	cmd.cmd = payload;
+	cmd.cmd_len = MACHXO2_BUF_SIZE;
+	cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
+
+	memcpy(payload, &progincr, sizeof(progincr));
+	for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
+		memcpy(&payload[sizeof(progincr)], &buf[i], MACHXO2_PAGE_SIZE);
+
+		cmd.cmd = payload;
+		cmd.cmd_len = MACHXO2_BUF_SIZE;
+		cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
+		ret = priv->write_commands(priv, &cmd, 1);
+		if (ret) {
+			dev_err(&mgr->dev, "Error loading the bitstream.\n");
+			return ret;
+		}
+	}
+	priv->get_status(priv, &status);
+	dump_status_reg(status);
+
+	return 0;
+}
+
+static int machxo2_write_complete(struct fpga_manager *mgr,
+				  struct fpga_image_info *info)
+{
+	struct machxo2_common_priv *priv = mgr->priv;
+	struct machxo2_cmd cmd = {};
+	u8 progdone[] = ISC_PROGRAMDONE;
+	u8 refresh[] = LSC_REFRESH;
+	u32 status;
+	int ret, refreshloop = 0;
+
+	cmd.cmd = progdone;
+	cmd.cmd_len = sizeof(progdone);
+	ret = priv->write_commands(priv, &cmd, 1);
+	if (ret)
+		goto fail;
+	ret = machxo2_wait_until_not_busy(priv);
+	if (ret)
+		goto fail;
+
+	priv->get_status(priv, &status);
+	dump_status_reg(status);
+	if (!(status & MACHXO2_DONE)) {
+		machxo2_cleanup(mgr);
+		ret = -EINVAL;
+		goto fail;
+	}
+
+	cmd.cmd = refresh;
+	cmd.cmd_len = sizeof(refresh);
+	cmd.delay_us = MACHXO2_REFRESH_USEC;
+
+	do {
+		ret = priv->write_commands(priv, &cmd, 1);
+		if (ret)
+			goto fail;
+
+		/* check refresh status */
+		priv->get_status(priv, &status);
+		dump_status_reg(status);
+		if (machxo2_status_done(status))
+			break;
+		if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
+			machxo2_cleanup(mgr);
+			ret = -EINVAL;
+			goto fail;
+		}
+	} while (1);
+
+	priv->get_status(priv, &status);
+	dump_status_reg(status);
+
+	return 0;
+fail:
+	dev_err(&mgr->dev, "Refresh failed.\n");
+
+	return ret;
+}
+
+static const struct fpga_manager_ops machxo2_ops = {
+	.state = machxo2_state,
+	.write_init = machxo2_write_init,
+	.write = machxo2_write,
+	.write_complete = machxo2_write_complete,
+};
+
+int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev)
+{
+	struct fpga_manager *mgr;
+
+	mgr = devm_fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager",
+				     &machxo2_ops, priv);
+
+	return PTR_ERR_OR_ZERO(mgr);
+}
diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h
new file mode 100644
index 000000000000..908203644209
--- /dev/null
+++ b/drivers/fpga/machxo2-common.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0  */
+/*
+ * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com>
+ * Copyright (C) 2021 Peter Jensen <pdj@bang-olufsen.dk>
+ * Copyright (C) 2022 Pengutronix, Johannes Zink <kernel@pengutronix.de>
+ */
+
+#ifndef __LINUX_FPGA_MGR_MACHXO2_COMMON_H
+#define __LINUX_FPGA_MGR_MACHXO2_COMMON_H
+
+#include <linux/types.h>
+
+/* MachXO2 Programming Guide - sysCONFIG Programming Commands */
+#define IDCODE_PUB              {0xe0, 0x00, 0x00, 0x00}
+#define ISC_ENABLE              {0xc6, 0x08, 0x00, 0x00}
+#define ISC_ERASE               {0x0e, 0x04, 0x00, 0x00}
+#define ISC_PROGRAMDONE         {0x5e, 0x00, 0x00, 0x00}
+#define LSC_INITADDRESS         {0x46, 0x00, 0x00, 0x00}
+#define LSC_PROGINCRNV          {0x70, 0x00, 0x00, 0x01}
+#define LSC_READ_STATUS         {0x3c, 0x00, 0x00, 0x00}
+#define LSC_REFRESH             {0x79, 0x00, 0x00, 0x00}
+
+struct machxo2_cmd {
+	u8 *cmd;
+	size_t cmd_len;
+	u16 delay_us;
+};
+
+struct machxo2_common_priv {
+	int (*write_commands)(struct machxo2_common_priv *priv,
+			     struct machxo2_cmd *cmds, size_t cmd_count);
+	int (*get_status)(struct machxo2_common_priv *priv, u32 *status);
+};
+
+int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev);
+
+#endif
diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 5f1d6505f828..30965a3c293e 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -8,24 +8,10 @@
  * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com>
  */
 
-#include <linux/delay.h>
-#include <linux/bitfield.h>
-#include <linux/fpga/fpga-mgr.h>
-#include <linux/gpio/consumer.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/spi/spi.h>
 #include <linux/container_of.h>
-
-/* MachXO2 Programming Guide - sysCONFIG Programming Commands */
-#define IDCODE_PUB		{0xe0, 0x00, 0x00, 0x00}
-#define ISC_ENABLE		{0xc6, 0x08, 0x00, 0x00}
-#define ISC_ERASE		{0x0e, 0x04, 0x00, 0x00}
-#define ISC_PROGRAMDONE		{0x5e, 0x00, 0x00, 0x00}
-#define LSC_INITADDRESS		{0x46, 0x00, 0x00, 0x00}
-#define LSC_PROGINCRNV		{0x70, 0x00, 0x00, 0x01}
-#define LSC_READ_STATUS		{0x3c, 0x00, 0x00, 0x00}
-#define LSC_REFRESH		{0x79, 0x00, 0x00, 0x00}
+#include "machxo2-common.h"
 
 /*
  * Max CCLK in Slave SPI mode according to 'MachXO2 Family Data
@@ -33,34 +19,6 @@
  */
 #define MACHXO2_MAX_SPEED		66000000
 
-#define MACHXO2_LOW_DELAY_USEC		5
-#define MACHXO2_HIGH_DELAY_USEC		200
-#define MACHXO2_REFRESH_USEC		4800
-#define MACHXO2_MAX_BUSY_LOOP		128
-#define MACHXO2_MAX_REFRESH_LOOP	16
-
-#define MACHXO2_PAGE_SIZE		16
-#define MACHXO2_BUF_SIZE		(MACHXO2_PAGE_SIZE + 4)
-
-/* Status register bits, errors and error mask */
-#define MACHXO2_BUSY		BIT(12)
-#define MACHXO2_DONE		BIT(8)
-#define MACHXO2_DVER		BIT(27)
-#define MACHXO2_ENAB		BIT(9)
-#define MACHXO2_ERR		GENMASK(25, 23)
-#define MACHXO2_ERR_ENOERR	0 /* no error */
-#define MACHXO2_ERR_EID		1
-#define MACHXO2_ERR_ECMD	2
-#define MACHXO2_ERR_ECRC	3
-#define MACHXO2_ERR_EPREAM	4 /* preamble error */
-#define MACHXO2_ERR_EABRT	5 /* abort error */
-#define MACHXO2_ERR_EOVERFL	6 /* overflow error */
-#define MACHXO2_ERR_ESDMEOF	7 /* SDM EOF */
-#define MACHXO2_FAIL		BIT(13)
-
-struct machxo2_common_priv {
-};
-
 struct machxo2_spi_priv {
 	struct machxo2_common_priv common;
 	struct spi_device *spi;
@@ -71,12 +29,7 @@ static inline struct machxo2_spi_priv *to_machxo2_spi_priv(struct machxo2_common
 	return container_of(priv, struct machxo2_spi_priv, common);
 }
 
-static inline u8 get_err(u32 status)
-{
-	return FIELD_GET(MACHXO2_ERR, status);
-}
-
-static int get_status(struct machxo2_common_priv *priv, u32 *status)
+static int machxo2_get_status_spi(struct machxo2_common_priv *priv, u32 *status)
 {
 	struct spi_device *spi = to_machxo2_spi_priv(priv)->spi;
 	struct spi_transfer transfers[2] = {};
@@ -98,52 +51,6 @@ static int get_status(struct machxo2_common_priv *priv, u32 *status)
 	return 0;
 }
 
-static const char *get_err_string(u8 err)
-{
-	switch (err) {
-	case MACHXO2_ERR_ENOERR:	return "No Error";
-	case MACHXO2_ERR_EID:		return "ID ERR";
-	case MACHXO2_ERR_ECMD:		return "CMD ERR";
-	case MACHXO2_ERR_ECRC:		return "CRC ERR";
-	case MACHXO2_ERR_EPREAM:	return "Preamble ERR";
-	case MACHXO2_ERR_EABRT:		return "Abort ERR";
-	case MACHXO2_ERR_EOVERFL:	return "Overflow ERR";
-	case MACHXO2_ERR_ESDMEOF:	return "SDM EOF";
-	}
-
-	return "Unknown";
-}
-
-static void dump_status_reg(u32 status)
-{
-	pr_debug("machxo2 status: 0x%08X - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
-		  status, !!FIELD_GET(MACHXO2_DONE, status), !!FIELD_GET(MACHXO2_ENAB, status),
-		  !!FIELD_GET(MACHXO2_BUSY, status), !!FIELD_GET(MACHXO2_FAIL, status),
-		  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
-}
-
-static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
-{
-	u32 status;
-	int ret, loop = 0;
-
-	do {
-		ret = get_status(priv, &status);
-		if (ret)
-			return ret;
-		if (++loop >= MACHXO2_MAX_BUSY_LOOP)
-			return -EBUSY;
-	} while (status & MACHXO2_BUSY);
-
-	return 0;
-}
-
-struct machxo2_cmd {
-	u8 *cmd;
-	size_t cmd_len;
-	u16 delay_us;
-};
-
 static int machxo2_write_spi(struct machxo2_common_priv *priv,
 			     struct machxo2_cmd *cmds, size_t cmd_count)
 {
@@ -169,225 +76,10 @@ static int machxo2_write_spi(struct machxo2_common_priv *priv,
 	return ret;
 }
 
-static int machxo2_cleanup(struct fpga_manager *mgr)
-{
-	struct machxo2_common_priv *priv = mgr->priv;
-	u8 erase[] = ISC_ERASE;
-	u8 refresh[] = LSC_REFRESH;
-	struct machxo2_cmd cmd = {};
-	int ret;
-
-	cmd.cmd = erase;
-	cmd.cmd_len = sizeof(erase);
-	ret = machxo2_write_spi(priv, &cmd, 1);
-	if (ret)
-		goto fail;
-
-	ret = machxo2_wait_until_not_busy(priv);
-	if (ret)
-		goto fail;
-
-	cmd.cmd = refresh;
-	cmd.cmd_len = sizeof(refresh);
-	cmd.delay_us = MACHXO2_REFRESH_USEC;
-	ret = machxo2_write_spi(priv, &cmd, 1);
-	if (ret)
-		goto fail;
-
-	return 0;
-fail:
-	dev_err(&mgr->dev, "Cleanup failed\n");
-
-	return ret;
-}
-
-static bool machxo2_status_done(unsigned long status)
-{
-	return (((status & (MACHXO2_BUSY | MACHXO2_DONE)) == MACHXO2_DONE) &&
-		get_err(status) == MACHXO2_ERR_ENOERR);
-}
-
-static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager *mgr)
-{
-	struct machxo2_common_priv *priv = mgr->priv;
-	u32 status;
-
-	get_status(priv, &status);
-	if (machxo2_status_done(status))
-		return FPGA_MGR_STATE_OPERATING;
-
-	return FPGA_MGR_STATE_UNKNOWN;
-}
-
-static int machxo2_write_init(struct fpga_manager *mgr,
-			      struct fpga_image_info *info,
-			      const char *buf, size_t count)
-{
-	struct machxo2_common_priv *priv = mgr->priv;
-	u8 enable[] = ISC_ENABLE;
-	u8 erase[] = ISC_ERASE;
-	u8 initaddr[] = LSC_INITADDRESS;
-	struct machxo2_cmd cmd[2] = {};
-	u32 status;
-	int ret;
-
-	if ((info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
-		dev_err(&mgr->dev,
-			"Partial reconfiguration is not supported\n");
-		return -ENOTSUPP;
-	}
-
-	get_status(priv, &status);
-	dump_status_reg(status);
-
-	cmd[0].cmd = enable;
-	cmd[0].cmd_len = sizeof(enable);
-	cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC;
-
-	cmd[1].cmd = erase;
-	cmd[1].cmd_len = sizeof(erase);
-	ret = machxo2_write_spi(spi, cmd, ARRAY_SIZE(cmd));
-
-	if (ret)
-		goto fail;
-
-	ret = machxo2_wait_until_not_busy(priv);
-	if (ret)
-		goto fail;
-
-	get_status(priv, &status);
-	if (status & MACHXO2_FAIL) {
-		ret = -EINVAL;
-		goto fail;
-	}
-	dump_status_reg(status);
-
-	cmd[0].cmd = initaddr;
-	cmd[0].cmd_len = sizeof(initaddr);
-	ret = machxo2_write_spi(priv, &cmd[0], 1);
-	if (ret)
-		goto fail;
-
-	get_status(priv, &status);
-	dump_status_reg(status);
-
-	return 0;
-fail:
-	dev_err(&mgr->dev, "Error during FPGA init.\n");
-
-	return ret;
-}
-
-static int machxo2_write(struct fpga_manager *mgr, const char *buf,
-			 size_t count)
-{
-	struct machxo2_common_priv *priv = mgr->priv;
-	u8 progincr[] = LSC_PROGINCRNV;
-	u8 payload[MACHXO2_BUF_SIZE];
-	struct machxo2_cmd cmd = {};
-	u32 status;
-	int i, ret;
-
-	if (count % MACHXO2_PAGE_SIZE != 0) {
-		dev_err(&mgr->dev, "Malformed payload.\n");
-		return -EINVAL;
-	}
-	get_status(priv, &status);
-	dump_status_reg(status);
-
-	cmd.cmd = payload;
-	cmd.cmd_len = MACHXO2_BUF_SIZE;
-	cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
-
-	memcpy(payload, &progincr, sizeof(progincr));
-	for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
-		memcpy(&payload[sizeof(progincr)], &buf[i], MACHXO2_PAGE_SIZE);
-
-		cmd.cmd = payload;
-		cmd.cmd_len = MACHXO2_BUF_SIZE;
-		cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
-		ret = machxo2_write_spi(priv, &cmd, 1);
-		if (ret) {
-			dev_err(&mgr->dev, "Error loading the bitstream.\n");
-			return ret;
-		}
-	}
-	get_status(priv, &status);
-	dump_status_reg(status);
-
-	return 0;
-}
-
-static int machxo2_write_complete(struct fpga_manager *mgr,
-				  struct fpga_image_info *info)
-{
-	struct machxo2_common_priv *priv = mgr->priv;
-	struct machxo2_cmd cmd = {};
-	u8 progdone[] = ISC_PROGRAMDONE;
-	u8 refresh[] = LSC_REFRESH;
-	u32 status;
-	int ret, refreshloop = 0;
-
-	cmd.cmd = progdone;
-	cmd.cmd_len = sizeof(progdone);
-	ret = machxo2_write_spi(priv, &cmd, 1);
-	if (ret)
-		goto fail;
-	ret = machxo2_wait_until_not_busy(priv);
-	if (ret)
-		goto fail;
-
-	get_status(priv, &status);
-	dump_status_reg(status);
-	if (!(status & MACHXO2_DONE)) {
-		machxo2_cleanup(mgr);
-		ret = -EINVAL;
-		goto fail;
-	}
-
-	cmd.cmd = refresh;
-	cmd.cmd_len = sizeof(refresh);
-	cmd.delay_us = MACHXO2_REFRESH_USEC;
-
-	do {
-		ret = machxo2_write_spi(priv, &cmd, 1);
-		if (ret)
-			goto fail;
-
-		/* check refresh status */
-		get_status(priv, &status);
-		dump_status_reg(status);
-		if (machxo2_status_done(status))
-			break;
-		if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
-			machxo2_cleanup(mgr);
-			ret = -EINVAL;
-			goto fail;
-		}
-	} while (1);
-
-	get_status(priv, &status);
-	dump_status_reg(status);
-
-	return 0;
-fail:
-	dev_err(&mgr->dev, "Refresh failed.\n");
-
-	return ret;
-}
-
-static const struct fpga_manager_ops machxo2_ops = {
-	.state = machxo2_spi_state,
-	.write_init = machxo2_write_init,
-	.write = machxo2_write,
-	.write_complete = machxo2_write_complete,
-};
-
 static int machxo2_spi_probe(struct spi_device *spi)
 {
 	struct machxo2_spi_priv *priv;
 	struct device *dev = &spi->dev;
-	struct fpga_manager *mgr;
 
 	if (spi->max_speed_hz > MACHXO2_MAX_SPEED) {
 		dev_err(dev, "Speed is too high\n");
@@ -399,10 +91,10 @@ static int machxo2_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	priv->spi = spi;
+	priv->common.write_commands = machxo2_write_spi;
+	priv->common.get_status = machxo2_get_status_spi;
 
-	mgr = devm_fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager",
-				     &machxo2_ops, &priv->common);
-	return PTR_ERR_OR_ZERO(mgr);
+	return machxo2_common_init(&(priv->common), dev);
 }
 
 #ifdef CONFIG_OF
-- 
2.30.2

[PATCH 12/16] fpga: machxo2: improve status register dump

[Johannes Zink]

Since the previous patches introduce a priv datastructure, the dev
pointer can now be stored upon init. This allows the dump_status_reg
function to use dev_dbg instead of a raw pr_debug.

No functional changes.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-common.c | 28 +++++++++++++++-------------
 drivers/fpga/machxo2-common.h |  1 +
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
index 33127ee67d19..71f886a60cba 100644
--- a/drivers/fpga/machxo2-common.c
+++ b/drivers/fpga/machxo2-common.c
@@ -63,12 +63,12 @@ static const char *get_err_string(u8 err)
 	return "Unknown";
 }
 
-static void dump_status_reg(u32 status)
+static void dump_status_reg(struct device *dev, u32 status)
 {
-        pr_debug("machxo2 status: 0x%08X - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
-                  status, !!FIELD_GET(MACHXO2_DONE, status), !!FIELD_GET(MACHXO2_ENAB, status),
-                  !!FIELD_GET(MACHXO2_BUSY, status), !!FIELD_GET(MACHXO2_FAIL, status),
-                  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
+	dev_dbg(dev, "0x%08X - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
+		status, !!FIELD_GET(MACHXO2_DONE, status), !!FIELD_GET(MACHXO2_ENAB, status),
+		!!FIELD_GET(MACHXO2_BUSY, status), !!FIELD_GET(MACHXO2_FAIL, status),
+		!!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
 }
 
 static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
@@ -156,7 +156,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	}
 
 	priv->get_status(priv, &status);
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 
 	cmd[0].cmd = enable;
 	cmd[0].cmd_len = sizeof(enable);
@@ -177,7 +177,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 		ret = -EINVAL;
 		goto fail;
 	}
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 
 	cmd[0].cmd = initaddr;
 	cmd[0].cmd_len = sizeof(initaddr);
@@ -186,7 +186,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 		goto fail;
 
 	priv->get_status(priv, &status);
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 
 	return 0;
 fail:
@@ -210,7 +210,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 		return -EINVAL;
 	}
 	priv->get_status(priv, &status);
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 	cmd.cmd = payload;
 	cmd.cmd_len = MACHXO2_BUF_SIZE;
 	cmd.delay_us = MACHXO2_HIGH_DELAY_USEC;
@@ -229,7 +229,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
 		}
 	}
 	priv->get_status(priv, &status);
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 
 	return 0;
 }
@@ -254,7 +254,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 		goto fail;
 
 	priv->get_status(priv, &status);
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 	if (!(status & MACHXO2_DONE)) {
 		machxo2_cleanup(mgr);
 		ret = -EINVAL;
@@ -272,7 +272,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 
 		/* check refresh status */
 		priv->get_status(priv, &status);
-		dump_status_reg(status);
+		dump_status_reg(priv->dev, status);
 		if (machxo2_status_done(status))
 			break;
 		if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
@@ -283,7 +283,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 	} while (1);
 
 	priv->get_status(priv, &status);
-	dump_status_reg(status);
+	dump_status_reg(priv->dev, status);
 
 	return 0;
 fail:
@@ -303,6 +303,8 @@ int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev)
 {
 	struct fpga_manager *mgr;
 
+	priv->dev = dev;
+
 	mgr = devm_fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager",
 				     &machxo2_ops, priv);
 
diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h
index 908203644209..a16060602a3f 100644
--- a/drivers/fpga/machxo2-common.h
+++ b/drivers/fpga/machxo2-common.h
@@ -30,6 +30,7 @@ struct machxo2_common_priv {
 	int (*write_commands)(struct machxo2_common_priv *priv,
 			     struct machxo2_cmd *cmds, size_t cmd_count);
 	int (*get_status)(struct machxo2_common_priv *priv, u32 *status);
+	struct device *dev;
 };
 
 int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev);
-- 
2.30.2

[PATCH 13/16] fpga: machxo2: add optional additional flash areas to be erased

[Johannes Zink]

This patch allows additional flash areas to be erased, i.e. not only the
configuration flash, but also sram, feature row and UFM (user flash
memory) can be erased.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-common.c | 40 +++++++++++++++++++++++++++++------
 drivers/fpga/machxo2-common.h |  1 +
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
index 71f886a60cba..d93c304cceb9 100644
--- a/drivers/fpga/machxo2-common.c
+++ b/drivers/fpga/machxo2-common.c
@@ -10,10 +10,13 @@
 
 #include <linux/delay.h>
 #include <linux/bitfield.h>
+#include <linux/byteorder/generic.h>
+#include <asm-generic/unaligned.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/property.h>
 #include "machxo2-common.h"
 
 #define MACHXO2_LOW_DELAY_USEC          5
@@ -41,6 +44,16 @@
 #define MACHXO2_ERR_EESDMEOF	7 /* SDM EOF */
 #define MACHXO2_FAIL		BIT(13)
 
+/*
+ * second byte ('operand') of ISC_ERASE can be ORed with the
+ * following bitmasks to not only erase configuration flash,
+ * but also SRAM, Feature Row or UFM, respectively. See MachXO2
+ * Programming and Configuration Usage Guide
+ */
+#define ISC_ERASE_SRAM		BIT(16)
+#define ISC_ERASE_FEATURE_ROW	BIT(17)
+#define ISC_ERASE_UFM		BIT(19)
+
 
 static inline u8 get_err(u32 status)
 {
@@ -90,13 +103,13 @@ static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
 static int machxo2_cleanup(struct fpga_manager *mgr)
 {
 	struct machxo2_common_priv *priv = mgr->priv;
-	u8 erase[] = ISC_ERASE;
 	u8 refresh[] = LSC_REFRESH;
 	struct machxo2_cmd cmd = {};
 	int ret;
 
-	cmd.cmd = erase;
-	cmd.cmd_len = sizeof(erase);
+	cmd.cmd = (u8 *)&priv->erase_cmd;
+	cmd.cmd_len = sizeof(priv->erase_cmd);
+
 	ret = priv->write_commands(priv, &cmd, 1);
 	if (ret)
 		goto fail;
@@ -143,7 +156,6 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 {
 	struct machxo2_common_priv *priv = mgr->priv;
 	u8 enable[] = ISC_ENABLE;
-	u8 erase[] = ISC_ERASE;
 	u8 initaddr[] = LSC_INITADDRESS;
 	struct machxo2_cmd cmd[2] = {};
 	u32 status;
@@ -162,8 +174,9 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	cmd[0].cmd_len = sizeof(enable);
 	cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC;
 
-	cmd[1].cmd = erase;
-	cmd[1].cmd_len = sizeof(erase);
+	cmd[1].cmd = (u8 *)&priv->erase_cmd;
+	cmd[1].cmd_len = sizeof(priv->erase_cmd);
+
 	ret = priv->write_commands(priv, cmd, 2);
 	if (ret)
 		goto fail;
@@ -302,6 +315,21 @@ static const struct fpga_manager_ops machxo2_ops = {
 int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev)
 {
 	struct fpga_manager *mgr;
+	u8 erase[] = ISC_ERASE;
+	u32 erase_cmd;
+
+	erase_cmd = get_unaligned_be32(erase);
+
+	if (device_property_read_bool(dev, "lattice,erase-sram"))
+		erase_cmd |= ISC_ERASE_SRAM;
+
+	if (device_property_read_bool(dev, "lattice,erase-feature-row"))
+		erase_cmd |= ISC_ERASE_FEATURE_ROW;
+
+	if (device_property_read_bool(dev, "lattice,erase-userflash"))
+		erase_cmd |= ISC_ERASE_UFM;
+
+	priv->erase_cmd = cpu_to_be32(erase_cmd);
 
 	priv->dev = dev;
 
diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h
index a16060602a3f..1b54154f91b1 100644
--- a/drivers/fpga/machxo2-common.h
+++ b/drivers/fpga/machxo2-common.h
@@ -31,6 +31,7 @@ struct machxo2_common_priv {
 			     struct machxo2_cmd *cmds, size_t cmd_count);
 	int (*get_status)(struct machxo2_common_priv *priv, u32 *status);
 	struct device *dev;
+	__be32 erase_cmd;
 };
 
 int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev);
-- 
2.30.2

[PATCH 14/16] fpga: machxo2: add program initialization signalling via gpio

[Johannes Zink]

Before initiating the programming sequence, this patch generates a low
pulse on FPGA's program_n pin which enables the FPGA's in circuit
programming interface.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-common.c | 15 +++++++++++++++
 drivers/fpga/machxo2-common.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
index d93c304cceb9..ccf9a50fc590 100644
--- a/drivers/fpga/machxo2-common.c
+++ b/drivers/fpga/machxo2-common.c
@@ -167,6 +167,17 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 		return -ENOTSUPP;
 	}
 
+	if (priv->fpga_program_n) {
+		/* set fpga in program mode. The timing is chosen very
+		 * conservatively, since the MachXO2 Family Datasheet
+		 * indicates a low pulse longer than 55ns will be accepted
+		 */
+		gpiod_set_value_cansleep(priv->fpga_program_n, 1);
+		usleep_range(1000, 1500);
+		gpiod_set_value_cansleep(priv->fpga_program_n, 0);
+		usleep_range(1000, 1500);
+	}
+
 	priv->get_status(priv, &status);
 	dump_status_reg(priv->dev, status);
 
@@ -331,6 +342,10 @@ int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev)
 
 	priv->erase_cmd = cpu_to_be32(erase_cmd);
 
+	priv->fpga_program_n = devm_gpiod_get_optional(dev,
+						       "lattice,program",
+						       GPIOD_OUT_LOW);
+
 	priv->dev = dev;
 
 	mgr = devm_fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager",
diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h
index 1b54154f91b1..0f9f53b48152 100644
--- a/drivers/fpga/machxo2-common.h
+++ b/drivers/fpga/machxo2-common.h
@@ -32,6 +32,7 @@ struct machxo2_common_priv {
 	int (*get_status)(struct machxo2_common_priv *priv, u32 *status);
 	struct device *dev;
 	__be32 erase_cmd;
+	struct gpio_desc *fpga_program_n;
 };
 
 int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev);
-- 
2.30.2

[PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Johannes Zink]

Measurements showed that some FPGAs take significantly longer than the
default wait function supplied. The datasheet inidicates up to 30
seconds erase times for some MachXO2 FPGAs, depending on the number of
LUTs (and the corresponding configuration flash size).

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
index ccf9a50fc590..e8967cdee2c6 100644
--- a/drivers/fpga/machxo2-common.c
+++ b/drivers/fpga/machxo2-common.c
@@ -17,6 +17,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/property.h>
+#include <linux/iopoll.h>
+#include <linux/time.h>
 #include "machxo2-common.h"
 
 #define MACHXO2_LOW_DELAY_USEC          5
@@ -24,6 +26,8 @@
 #define MACHXO2_REFRESH_USEC            4800
 #define MACHXO2_MAX_BUSY_LOOP           128
 #define MACHXO2_MAX_REFRESH_LOOP        16
+#define MACHXO2_MAX_ERASE_USEC          (30 * USEC_PER_SEC)
+#define MACHXO2_ERASE_USEC_SLEEP        (20 * USEC_PER_MSEC)
 
 #define MACHXO2_PAGE_SIZE               16
 #define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
@@ -54,6 +58,18 @@
 #define ISC_ERASE_FEATURE_ROW	BIT(17)
 #define ISC_ERASE_UFM		BIT(19)
 
+static inline int machxo2_wait_until_not_busy_timeout(struct machxo2_common_priv *priv)
+{
+	int ret, pollret;
+	u32 status = MACHXO2_BUSY;
+
+	pollret = read_poll_timeout(priv->get_status, ret,
+				    (ret && ret != -EAGAIN) || !(status & MACHXO2_BUSY),
+				    MACHXO2_ERASE_USEC_SLEEP, MACHXO2_MAX_ERASE_USEC,
+				    true, priv, &status);
+
+	return ret ?: pollret;
+}
 
 static inline u8 get_err(u32 status)
 {
@@ -114,6 +130,12 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
 	if (ret)
 		goto fail;
 
+	ret = machxo2_wait_until_not_busy_timeout(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "Erase operation failed (%d)", ret);
+		goto fail;
+	}
+
 	ret = machxo2_wait_until_not_busy(priv);
 	if (ret)
 		goto fail;
@@ -192,9 +214,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	if (ret)
 		goto fail;
 
-	ret = machxo2_wait_until_not_busy(priv);
-	if (ret)
+	ret = machxo2_wait_until_not_busy_timeout(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "Erase operation failed (%d)", ret);
 		goto fail;
+	}
 
 	priv->get_status(priv, &status);
 	if (status & MACHXO2_FAIL) {
-- 
2.30.2

[PATCH 16/16] fpga: machxo2: add configuration over i2c

[Johannes Zink]

From: Peter Jensen <pdj@bang-olufsen.dk>

The configuration flash of the machxo2 fpga can also be erased and
written over i2c instead of spi. Add this functionality to the
refactored common driver. Since some commands are shorter over I2C than
they are over SPI some quirks are added to the common driver in order to
account for that.

Signed-off-by: Peter Jensen <pdj@bang-olufsen.dk>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/fpga/Kconfig          |   8 ++
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/machxo2-common.c |  15 +++-
 drivers/fpga/machxo2-common.h |   3 +
 drivers/fpga/machxo2-i2c.c    | 137 ++++++++++++++++++++++++++++++++++
 5 files changed, 163 insertions(+), 1 deletion(-)
 create mode 100644 drivers/fpga/machxo2-i2c.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index e5869a732246..97081bbd7c19 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -90,6 +90,14 @@ config FPGA_MGR_MACHXO2_SPI
 	  FPGA manager driver support for Lattice MachXO2 configuration
 	  over slave SPI interface.
 
+config FPGA_MGR_MACHXO2_I2C
+	tristate "Lattice MachXO2 I2C"
+	depends on I2C
+	select FPGA_MGR_MACHXO2_COMMON
+	help
+	  FPGA manager driver support for Lattice MachXO2 configuration
+	  over slave I2C interface.
+
 config FPGA_MGR_TS73XX
 	tristate "Technologic Systems TS-73xx SBC FPGA Manager"
 	depends on ARCH_EP93XX && MACH_TS72XX
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index f247a8de83ad..fcdf79f4d424 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_FPGA_MGR_ALTERA_PS_SPI)	+= altera-ps-spi.o
 obj-$(CONFIG_FPGA_MGR_ICE40_SPI)	+= ice40-spi.o
 obj-$(CONFIG_FPGA_MGR_MACHXO2_COMMON)	+= machxo2-common.o
 obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI)	+= machxo2-spi.o
+obj-$(CONFIG_FPGA_MGR_MACHXO2_I2C)	+= machxo2-i2c.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
 obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
index e8967cdee2c6..0a3c126675da 100644
--- a/drivers/fpga/machxo2-common.c
+++ b/drivers/fpga/machxo2-common.c
@@ -100,7 +100,7 @@ static void dump_status_reg(struct device *dev, u32 status)
 		!!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
 }
 
-static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
+int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
 {
 	u32 status;
 	int ret, loop = 0;
@@ -143,6 +143,11 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
 	cmd.cmd = refresh;
 	cmd.cmd_len = sizeof(refresh);
 	cmd.delay_us = MACHXO2_REFRESH_USEC;
+
+	/* quirk: refresh command over i2c is 1 byte shorter */
+	if (priv->refresh_3b)
+		cmd.cmd_len--;
+
 	ret = priv->write_commands(priv, &cmd, 1);
 	if (ret)
 		goto fail;
@@ -207,6 +212,10 @@ static int machxo2_write_init(struct fpga_manager *mgr,
 	cmd[0].cmd_len = sizeof(enable);
 	cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC;
 
+	/* quirk: enable command over i2c is 1 byte shorter */
+	if (priv->enable_3b)
+		cmd[0].cmd_len--;
+
 	cmd[1].cmd = (u8 *)&priv->erase_cmd;
 	cmd[1].cmd_len = sizeof(priv->erase_cmd);
 
@@ -313,6 +322,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 	cmd.cmd_len = sizeof(refresh);
 	cmd.delay_us = MACHXO2_REFRESH_USEC;
 
+	/* quirk: refresh command over i2c is 1 byte shorter */
+	if (priv->refresh_3b)
+		cmd.cmd_len--;
+
 	do {
 		ret = priv->write_commands(priv, &cmd, 1);
 		if (ret)
diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h
index 0f9f53b48152..8c09345adee5 100644
--- a/drivers/fpga/machxo2-common.h
+++ b/drivers/fpga/machxo2-common.h
@@ -32,9 +32,12 @@ struct machxo2_common_priv {
 	int (*get_status)(struct machxo2_common_priv *priv, u32 *status);
 	struct device *dev;
 	__be32 erase_cmd;
+	u8 enable_3b:1;
+	u8 refresh_3b:1;
 	struct gpio_desc *fpga_program_n;
 };
 
 int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev);
+int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv);
 
 #endif
diff --git a/drivers/fpga/machxo2-i2c.c b/drivers/fpga/machxo2-i2c.c
new file mode 100644
index 000000000000..a309016def1c
--- /dev/null
+++ b/drivers/fpga/machxo2-i2c.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lattice MachXO2 Slave I2C Driver
+ *
+ * Manage Lattice FPGA firmware that is loaded over I2C using
+ * the slave serial configuration interface.
+ *
+ * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com>
+ * Copyright (C) 2021 Peter Jensen <pdj@bang-olufsen.dk>
+ */
+
+#include <linux/i2c.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include "machxo2-common.h"
+
+
+struct machxo2_i2c_priv {
+	struct machxo2_common_priv common;
+	struct i2c_client *client;
+};
+
+static inline struct machxo2_i2c_priv *to_machxo2_i2c_priv(struct machxo2_common_priv *common)
+{
+	return container_of(common, struct machxo2_i2c_priv, common);
+}
+
+static int machxo2_i2c_get_status(struct machxo2_common_priv *bus, u32 *status)
+{
+	struct machxo2_i2c_priv *i2cPriv = to_machxo2_i2c_priv(bus);
+	struct i2c_client *client = i2cPriv->client;
+	u8 read_status[] = LSC_READ_STATUS;
+	__be32 tmp;
+	int ret;
+	struct i2c_msg msg[] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.buf = read_status,
+			.len = ARRAY_SIZE(read_status),
+		}, {
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.buf = (u8 *) &tmp,
+			.len = sizeof(tmp)
+		}
+	};
+
+	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (ret < 0)
+		return ret;
+	if (ret != ARRAY_SIZE(msg))
+		return -EIO;
+	*status = be32_to_cpu(tmp);
+
+	return 0;
+}
+
+static int machxo2_i2c_write(struct machxo2_common_priv *common,
+			     struct machxo2_cmd *cmds, size_t cmd_count)
+{
+	struct machxo2_i2c_priv *i2c_priv = to_machxo2_i2c_priv(common);
+	struct i2c_client *client = i2c_priv->client;
+	size_t i;
+	int ret;
+
+	for (i = 0; i < cmd_count; i++) {
+		struct i2c_msg msg[] = {
+			{
+				.addr = client->addr,
+				.buf = cmds[i].cmd,
+				.len = cmds[i].cmd_len,
+			},
+		};
+
+		ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+		if (ret < 0)
+			return ret;
+		if (ret != ARRAY_SIZE(msg))
+			return -EIO;
+		if (cmds[i].delay_us)
+			usleep_range(cmds[i].delay_us, cmds[i].delay_us +
+				     cmds[i].delay_us / 4);
+		if (i < cmd_count - 1) /* on any iteration except for the last one... */
+			ret = machxo2_wait_until_not_busy(common);
+	}
+
+	return 0;
+}
+
+static int machxo2_i2c_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct machxo2_i2c_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(struct machxo2_i2c_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	priv->common.get_status = machxo2_i2c_get_status;
+	priv->common.write_commands = machxo2_i2c_write;
+
+	/* Commands are usually 4b, but these aren't for i2c */
+	priv->common.enable_3b = true;
+	priv->common.refresh_3b = true;
+
+	return machxo2_common_init(&priv->common, dev);
+}
+
+static const struct of_device_id of_match[] = {
+	{ .compatible = "lattice,machxo2-slave-i2c", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_match);
+
+static const struct i2c_device_id lattice_ids[] = {
+	{ "machxo2-slave-i2c", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, lattice_ids);
+
+static struct i2c_driver machxo2_i2c_driver = {
+	.driver = {
+		.name = "machxo2-slave-i2c",
+		.of_match_table = of_match_ptr(of_match),
+	},
+	.probe = machxo2_i2c_probe,
+	.id_table = lattice_ids,
+};
+
+module_i2c_driver(machxo2_i2c_driver);
+
+MODULE_AUTHOR("Peter Jensen <pdj@bang-olufsen.dk>");
+MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Ivan Bornyakov]

Hi, Johannes!

I just came across your patches. Surprisingly, our work interferes.

I recently posted patch-series for configuring ECP5 and was asked to make
generalized sysCONFIG driver with support for both ECP5 and MachXO2, which
I did. Sadly I don't have hardware with MachXO2, but you clearly do :)

Please, take a look at

https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/

and please help test MachXO2 variant. When we pull this off, you may add I2C
interface on top.

Re: [PATCH 03/16] dt-bindings: fpga: machxo2-slave: add pin for program sequence init

[Rob Herring]

On Thu, 25 Aug 2022 16:13:30 +0200, Johannes Zink wrote:
> This commit adds a pin which initiates the FPGA programming sequence
> once pulsed low.
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../devicetree/bindings/fpga/lattice,machxo2-slave.yaml    | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.example.dts:28.51-52 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:384: Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1420: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Johannes Zink]

On Thu, 2022-08-25 at 18:25 +0300, Ivan Bornyakov wrote:
> Hi, Johannes!

Hi Ivan,
> 
> I just came across your patches. Surprisingly, our work interferes.
> 
> I recently posted patch-series for configuring ECP5 and was asked to
> make
> generalized sysCONFIG driver with support for both ECP5 and MachXO2,
> which
> I did. 

That looks very interesting indeed.

> Sadly I don't have hardware with MachXO2, but you clearly do :)
> 
> Please, take a look at
> 
>  
> https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> 
> and please help test MachXO2 variant. When we pull this off, you may
> add I2C
> interface on top.
> 
> 
> 

my hardware has only I2C connected to the MachXO2 (hence the patch
series...), so I cannot test your patches directly. 

Since adding I2C requires some quirks with respect to the programming
commands (some are differ to the SPI ones, ...) it will take me some
time to add my patches on top of yours in order to test, but after
having had a short glance at your patch series, I think it should be
feasible.

Though, I think you should allow the program-gpios, init-gpios and
done-gpios for machxo2 and have them as optional, at least for machxo2.

Best regards
Johannes

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 03/16] dt-bindings: fpga: machxo2-slave: add pin for program sequence init

[Johannes Zink]

On Thu, 2022-08-25 at 13:51 -0500, Rob Herring wrote:
> On Thu, 25 Aug 2022 16:13:30 +0200, Johannes Zink wrote:
> > This commit adds a pin which initiates the FPGA programming
> > sequence
> > once pulsed low.
> > 
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  .../devicetree/bindings/fpga/lattice,machxo2-slave.yaml    | 7
> > +++++++
> >  1 file changed, 7 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/fpga/lattice,machxo2-
> slave.example.dts:28.51-52 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:384:
> Documentation/devicetree/bindings/fpga/lattice,machxo2-
> slave.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1420: dt_binding_check] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/patch/
> 
> This check can fail if there are any dependencies. The base for a
> patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 
> 
Hi, 

I was able to reproduce the error and will fix it in v2. 

Thanks and best regards
Johannes

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Ivan Bornyakov]

On Fri, Aug 26, 2022 at 08:32:49AM +0200, Johannes Zink wrote:
> On Thu, 2022-08-25 at 18:25 +0300, Ivan Bornyakov wrote:
> > Hi, Johannes!
> 
> Hi Ivan,
> > 
> > I just came across your patches. Surprisingly, our work interferes.
> > 
> > I recently posted patch-series for configuring ECP5 and was asked to
> > make
> > generalized sysCONFIG driver with support for both ECP5 and MachXO2,
> > which
> > I did. 
> 
> That looks very interesting indeed.
> 
> > Sadly I don't have hardware with MachXO2, but you clearly do :)
> > 
> > Please, take a look at
> > 
> >  
> > https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> > 
> > and please help test MachXO2 variant. When we pull this off, you may
> > add I2C
> > interface on top.
> > 
> > 
> > 
> 
> my hardware has only I2C connected to the MachXO2 (hence the patch
> series...), so I cannot test your patches directly.

That's unfortunate, anyway please join the review so your changes would
be easier to apply on top.

> 
> Since adding I2C requires some quirks with respect to the programming
> commands (some are differ to the SPI ones, ...) it will take me some
> time to add my patches on top of yours in order to test, but after
> having had a short glance at your patch series, I think it should be
> feasible.
> 
> Though, I think you should allow the program-gpios, init-gpios and
> done-gpios for machxo2 and have them as optional, at least for machxo2.
> 
> Best regards
> Johannes
> 
> -- 
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Sascha Hauer]

Hi Ivan,

On Thu, Aug 25, 2022 at 06:25:14PM +0300, Ivan Bornyakov wrote:
> Hi, Johannes!
> 
> I just came across your patches. Surprisingly, our work interferes.
> 
> I recently posted patch-series for configuring ECP5 and was asked to make
> generalized sysCONFIG driver with support for both ECP5 and MachXO2, which
> I did. Sadly I don't have hardware with MachXO2, but you clearly do :)
> 
> Please, take a look at
> 
> https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> 
> and please help test MachXO2 variant. When we pull this off, you may add I2C
> interface on top.

You are adding a new driver for something we already have a driver for
in the tree. The final goal should be that we only have a single driver
for sysconfig based FPGAs in the tree. Johannes' series is a step in
that direction: He cleans up the existing driver and starts abstracting
out common sysconfig functions so that they can be used by both the I2C
and SPI interface. He just told me that the abstraction is likely not
enough to integrate ECP5 support right away, one reason being that the
machxo2 has a flash whereas the ECP5 does not.

Unless you can explain why the existing driver is broken beyond repair
I think we should rather incrementally improve the existing driver
instead of adding a new one with a conflicting compatible.
So despite you were in the room earlier I think you should rather base
your work on Johannes' series.

Just my 2 cents, maybe one of the maintainers has a few words on it.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Ivan Bornyakov]

On Fri, Aug 26, 2022 at 10:25:39AM +0200, Sascha Hauer wrote:
> Hi Ivan,
> 
> On Thu, Aug 25, 2022 at 06:25:14PM +0300, Ivan Bornyakov wrote:
> > Hi, Johannes!
> > 
> > I just came across your patches. Surprisingly, our work interferes.
> > 
> > I recently posted patch-series for configuring ECP5 and was asked to make
> > generalized sysCONFIG driver with support for both ECP5 and MachXO2, which
> > I did. Sadly I don't have hardware with MachXO2, but you clearly do :)
> > 
> > Please, take a look at
> > 
> > https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> > 
> > and please help test MachXO2 variant. When we pull this off, you may add I2C
> > interface on top.
> 
> You are adding a new driver for something we already have a driver for
> in the tree.

My intent was to add new driver and drop old one once the new driver is
proven to be working.

> The final goal should be that we only have a single driver
> for sysconfig based FPGAs in the tree.

It is.

> Johannes' series is a step in
> that direction: He cleans up the existing driver and starts abstracting
> out common sysconfig functions so that they can be used by both the I2C
> and SPI interface. He just told me that the abstraction is likely not
> enough to integrate ECP5 support right away, one reason being that the
> machxo2 has a flash whereas the ECP5 does not.
> 
> Unless you can explain why the existing driver is broken beyond repair
> I think we should rather incrementally improve the existing driver
> instead of adding a new one with a conflicting compatible.

Yeah, conflicting compatible was my oversight.

> So despite you were in the room earlier I think you should rather base
> your work on Johannes' series.

We on par here. You guys didn't join ongoing work, I didn't rework
existing driver.

> 
> Just my 2 cents, maybe one of the maintainers has a few words on it.
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Ivan Bornyakov]

On Fri, Aug 26, 2022 at 12:00:44PM +0300, Ivan Bornyakov wrote:
> On Fri, Aug 26, 2022 at 10:25:39AM +0200, Sascha Hauer wrote:
> > Hi Ivan,
> > 
> > On Thu, Aug 25, 2022 at 06:25:14PM +0300, Ivan Bornyakov wrote:
> > > Hi, Johannes!
> > > 
> > > I just came across your patches. Surprisingly, our work interferes.
> > > 
> > > I recently posted patch-series for configuring ECP5 and was asked to make
> > > generalized sysCONFIG driver with support for both ECP5 and MachXO2, which
> > > I did. Sadly I don't have hardware with MachXO2, but you clearly do :)
> > > 
> > > Please, take a look at
> > > 
> > > https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> > > 
> > > and please help test MachXO2 variant. When we pull this off, you may add I2C
> > > interface on top.
> > 
> > You are adding a new driver for something we already have a driver for
> > in the tree.
> 
> My intent was to add new driver and drop old one once the new driver is
> proven to be working.
> 
> > The final goal should be that we only have a single driver
> > for sysconfig based FPGAs in the tree.
> 
> It is.
> 
> > Johannes' series is a step in
> > that direction: He cleans up the existing driver and starts abstracting
> > out common sysconfig functions so that they can be used by both the I2C
> > and SPI interface. He just told me that the abstraction is likely not
> > enough to integrate ECP5 support right away, one reason being that the
> > machxo2 has a flash whereas the ECP5 does not.
> > 
> > Unless you can explain why the existing driver is broken beyond repair
> > I think we should rather incrementally improve the existing driver
> > instead of adding a new one with a conflicting compatible.
> 
> Yeah, conflicting compatible was my oversight.
> 

Wait! They are not conflicting :) The new one is "lattice,machxo2-fpga-mgr",
the old one is "lattice,machxo2-slave-spi"

> > So despite you were in the room earlier I think you should rather base
> > your work on Johannes' series.
> 
> We on par here. You guys didn't join ongoing work, I didn't rework
> existing driver.
> 
> > 
> > Just my 2 cents, maybe one of the maintainers has a few words on it.
> > 
> > Sascha
> > 
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

[Xu Yilun]

On 2022-08-26 at 12:19:12 +0300, Ivan Bornyakov wrote:
> On Fri, Aug 26, 2022 at 12:00:44PM +0300, Ivan Bornyakov wrote:
> > On Fri, Aug 26, 2022 at 10:25:39AM +0200, Sascha Hauer wrote:
> > > Hi Ivan,
> > > 
> > > On Thu, Aug 25, 2022 at 06:25:14PM +0300, Ivan Bornyakov wrote:
> > > > Hi, Johannes!
> > > > 
> > > > I just came across your patches. Surprisingly, our work interferes.
> > > > 
> > > > I recently posted patch-series for configuring ECP5 and was asked to make
> > > > generalized sysCONFIG driver with support for both ECP5 and MachXO2, which
> > > > I did. Sadly I don't have hardware with MachXO2, but you clearly do :)
> > > > 
> > > > Please, take a look at
> > > > 
> > > > https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> > > > 
> > > > and please help test MachXO2 variant. When we pull this off, you may add I2C
> > > > interface on top.
> > > 
> > > You are adding a new driver for something we already have a driver for
> > > in the tree.
> > 
> > My intent was to add new driver and drop old one once the new driver is
> > proven to be working.
> > 
> > > The final goal should be that we only have a single driver
> > > for sysconfig based FPGAs in the tree.
> > 
> > It is.
> > 
> > > Johannes' series is a step in
> > > that direction: He cleans up the existing driver and starts abstracting
> > > out common sysconfig functions so that they can be used by both the I2C
> > > and SPI interface. He just told me that the abstraction is likely not
> > > enough to integrate ECP5 support right away, one reason being that the
> > > machxo2 has a flash whereas the ECP5 does not.
> > > 
> > > Unless you can explain why the existing driver is broken beyond repair
> > > I think we should rather incrementally improve the existing driver
> > > instead of adding a new one with a conflicting compatible.
> > 
> > Yeah, conflicting compatible was my oversight.
> > 
> 
> Wait! They are not conflicting :) The new one is "lattice,machxo2-fpga-mgr",
> the old one is "lattice,machxo2-slave-spi"
> 
> > > So despite you were in the room earlier I think you should rather base
> > > your work on Johannes' series.
> > 
> > We on par here. You guys didn't join ongoing work, I didn't rework
> > existing driver.

I didn't look into the code yet, so maybe some misunderstanding.

I'd rather we pay more attention on the code design for the FULL feature
of this sysCONFIG interface, rather than worrying about whose patches should
go first.

So just review patches for each other and collabrate for a well design,
then your features can be accepted faster.

Thanks,
Yilun

> > 
> > > 
> > > Just my 2 cents, maybe one of the maintainers has a few words on it.
> > > 
> > > Sascha
> > > 
> > > 
> > > -- 
> > > Pengutronix e.K.                           |                             |
> > > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Re: [PATCH 02/16] dt-bindings: fpga: machxo2-slave: add erasure properties

[Xu Yilun]

On 2022-08-25 at 16:13:29 +0200, Johannes Zink wrote:
> This patch introduces additional memory areas of the machxo2-slave fpga
> to be erased.
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../bindings/fpga/lattice,machxo2-slave.yaml      | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> index d05acd6b0fc6..78f0da8f772f 100644
> --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> @@ -26,6 +26,19 @@ properties:
>      enum:
>        - lattice,machxo2-slave-spi
>  
> +  lattice,erase-sram:
> +    type: boolean
> +    description: SRAM is to be erased during flash erase operation
> +
> +  lattice,erase-feature-row:
> +    type: boolean
> +    description: Feature row is to be erased during flash erase operation
> +
> +  lattice,erase-userflash:
> +    type: boolean
> +    description: |
> +      UFM (user flash memory) is to be erased during flash erase operation

In which conditions should we decide to erase each area?

Thanks,
Yilun

> +
>  required:
>    - compatible
>    - reg
> @@ -42,5 +55,7 @@ examples:
>              compatible = "lattice,machxo2-slave-spi";
>              spi-max-frequency = <8000000>;
>              reg = <0>;
> +            lattice,erase-sram;
> +            lattice,erase-feature-row;
>          };
>      };
> -- 
> 2.30.2
> 

Re: [PATCH 03/16] dt-bindings: fpga: machxo2-slave: add pin for program sequence init

[Xu Yilun]

On 2022-08-25 at 16:13:30 +0200, Johannes Zink wrote:
> This commit adds a pin which initiates the FPGA programming sequence
> once pulsed low.

Why we don't have to use this pin before?

Thanks,
Yilun

> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../devicetree/bindings/fpga/lattice,machxo2-slave.yaml    | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> index 78f0da8f772f..03dc134ec7b8 100644
> --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> @@ -26,6 +26,12 @@ properties:
>      enum:
>        - lattice,machxo2-slave-spi
>  
> +  program-gpios:
> +    maxItems: 1
> +    description: |
> +      GPIO Output tied to the FPGA's n_program pin to initiate a
> +      programming sequence. This pin is active low.
> +
>    lattice,erase-sram:
>      type: boolean
>      description: SRAM is to be erased during flash erase operation
> @@ -57,5 +63,6 @@ examples:
>              reg = <0>;
>              lattice,erase-sram;
>              lattice,erase-feature-row;
> +            lattice,program-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>
>          };
>      };
> -- 
> 2.30.2
> 

Re: [PATCH 07/16] fpga: machxo2-spi: fix big-endianness incompatibility

[Xu Yilun]

On 2022-08-25 at 16:13:34 +0200, Johannes Zink wrote:
> The SPI message is written into the lowest-addressed bits of an
> unsigned long variable, but be32_to_cpu is called on the least
> significant bits of the variable. On big-endian 64-bit systems,
> this would give a wrong result. Fix this by using the fixed-size
> u32 instead of unsigned long. This clashes with the use of
> test_bit, which is unnecessary for a single u32 variable, so
> we adjust all usage sites appropriately and prefix the macros
> with MACHXO2_ while at it.
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  drivers/fpga/machxo2-spi.c | 110 ++++++++++++++++++-------------------
>  1 file changed, 55 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
> index 5e12612c7289..d1a8f28e04e7 100644
> --- a/drivers/fpga/machxo2-spi.c
> +++ b/drivers/fpga/machxo2-spi.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/bitfield.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> @@ -41,41 +42,40 @@
>  #define MACHXO2_BUF_SIZE		(MACHXO2_PAGE_SIZE + 4)
>  
>  /* Status register bits, errors and error mask */
> -#define BUSY	12
> -#define DONE	8
> -#define DVER	27
> -#define ENAB	9
> -#define ERRBITS	23
> -#define ERRMASK	7
> -#define FAIL	13
> -
> -#define ENOERR	0 /* no error */
> -#define EID	1
> -#define ECMD	2
> -#define ECRC	3
> -#define EPREAM	4 /* preamble error */
> -#define EABRT	5 /* abort error */
> -#define EOVERFL	6 /* overflow error */
> -#define ESDMEOF	7 /* SDM EOF */
> -
> -static inline u8 get_err(unsigned long *status)
> +#define MACHXO2_BUSY		BIT(12)
> +#define MACHXO2_DONE		BIT(8)
> +#define MACHXO2_DVER		BIT(27)
> +#define MACHXO2_ENAB		BIT(9)
> +#define MACHXO2_ERR		GENMASK(25, 23)
> +#define MACHXO2_ERR_ENOERR	0 /* no error */
> +#define MACHXO2_ERR_EID		1
> +#define MACHXO2_ERR_ECMD	2
> +#define MACHXO2_ERR_ECRC	3
> +#define MACHXO2_ERR_EPREAM	4 /* preamble error */
> +#define MACHXO2_ERR_EABRT	5 /* abort error */
> +#define MACHXO2_ERR_EOVERFL	6 /* overflow error */
> +#define MACHXO2_ERR_ESDMEOF	7 /* SDM EOF */
> +#define MACHXO2_FAIL		BIT(13)
> +
> +static inline u8 get_err(u32 status)
>  {
> -	return (*status >> ERRBITS) & ERRMASK;
> +	return FIELD_GET(MACHXO2_ERR, status);
>  }

So far the changes have nothing to do with the endian issue, is it? So
please put it into another patch.

>  
> -static int get_status(struct spi_device *spi, unsigned long *status)
> +static int get_status(struct spi_device *spi, u32 *status)
>  {
>  	struct spi_message msg;
>  	struct spi_transfer rx, tx;
>  	static const u8 cmd[] = LSC_READ_STATUS;
> +	__be32 tmp;
>  	int ret;
>  
>  	memset(&rx, 0, sizeof(rx));
>  	memset(&tx, 0, sizeof(tx));
>  	tx.tx_buf = cmd;
>  	tx.len = sizeof(cmd);
> -	rx.rx_buf = status;
> -	rx.len = 4;
> +	rx.rx_buf = &tmp;
> +	rx.len = sizeof(tmp);
>  	spi_message_init(&msg);
>  	spi_message_add_tail(&tx, &msg);
>  	spi_message_add_tail(&rx, &msg);
> @@ -83,7 +83,7 @@ static int get_status(struct spi_device *spi, unsigned long *status)
>  	if (ret)
>  		return ret;
>  
> -	*status = be32_to_cpu(*status);
> +	*status = be32_to_cpu(tmp);

Why not directly operate on the status, I don't see the difference.

>  
>  	return 0;
>  }
> @@ -91,30 +91,30 @@ static int get_status(struct spi_device *spi, unsigned long *status)
>  static const char *get_err_string(u8 err)
>  {
>  	switch (err) {
> -	case ENOERR:	return "No Error";
> -	case EID:	return "ID ERR";
> -	case ECMD:	return "CMD ERR";
> -	case ECRC:	return "CRC ERR";
> -	case EPREAM:	return "Preamble ERR";
> -	case EABRT:	return "Abort ERR";
> -	case EOVERFL:	return "Overflow ERR";
> -	case ESDMEOF:	return "SDM EOF";
> +	case MACHXO2_ERR_ENOERR:	return "No Error";
> +	case MACHXO2_ERR_EID:		return "ID ERR";
> +	case MACHXO2_ERR_ECMD:		return "CMD ERR";
> +	case MACHXO2_ERR_ECRC:		return "CRC ERR";
> +	case MACHXO2_ERR_EPREAM:	return "Preamble ERR";
> +	case MACHXO2_ERR_EABRT:		return "Abort ERR";
> +	case MACHXO2_ERR_EOVERFL:	return "Overflow ERR";
> +	case MACHXO2_ERR_ESDMEOF:	return "SDM EOF";
>  	}

Same concern, no relation to the commit message

>  
> -	return "Default switch case";
> +	return "Unknown";
>  }
>  
> -static void dump_status_reg(unsigned long *status)
> +static void dump_status_reg(u32 status)
>  {
> -	pr_debug("machxo2 status: 0x%08lX - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
> -		 *status, test_bit(DONE, status), test_bit(ENAB, status),
> -		 test_bit(BUSY, status), test_bit(FAIL, status),
> -		 test_bit(DVER, status), get_err_string(get_err(status)));
> +	pr_debug("machxo2 status: 0x%08X - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n",
> +		  status, !!FIELD_GET(MACHXO2_DONE, status), !!FIELD_GET(MACHXO2_ENAB, status),
> +		  !!FIELD_GET(MACHXO2_BUSY, status), !!FIELD_GET(MACHXO2_FAIL, status),
> +		  !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));

Same concern. I'll not point out one by one below.

Thanks,
Yilun

>  }


>  
>  static int wait_until_not_busy(struct spi_device *spi)
>  {
> -	unsigned long status;
> +	u32 status;
>  	int ret, loop = 0;
>  
>  	do {
> @@ -123,7 +123,7 @@ static int wait_until_not_busy(struct spi_device *spi)
>  			return ret;
>  		if (++loop >= MACHXO2_MAX_BUSY_LOOP)
>  			return -EBUSY;
> -	} while (test_bit(BUSY, &status));
> +	} while (status & MACHXO2_BUSY);
>  
>  	return 0;
>  }
> @@ -169,14 +169,14 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
>  
>  static bool machxo2_status_done(unsigned long status)
>  {
> -	return !test_bit(BUSY, &status) && test_bit(DONE, &status) &&
> -		get_err(&status) == ENOERR;
> +	return (((status & (MACHXO2_BUSY | MACHXO2_DONE)) == MACHXO2_DONE) &&
> +		get_err(status) == MACHXO2_ERR_ENOERR);
>  }
>  
>  static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager *mgr)
>  {
>  	struct spi_device *spi = mgr->priv;
> -	unsigned long status;
> +	u32 status;
>  
>  	get_status(spi, &status);
>  	if (machxo2_status_done(status))
> @@ -195,7 +195,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  	static const u8 enable[] = ISC_ENABLE;
>  	static const u8 erase[] = ISC_ERASE;
>  	static const u8 initaddr[] = LSC_INITADDRESS;
> -	unsigned long status;
> +	u32 status;
>  	int ret;
>  
>  	if ((info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> @@ -205,7 +205,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  	}
>  
>  	get_status(spi, &status);
> -	dump_status_reg(&status);
> +	dump_status_reg(status);
>  	memset(tx, 0, sizeof(tx));
>  	spi_message_init(&msg);
>  	tx[0].tx_buf = &enable;
> @@ -226,11 +226,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  		goto fail;
>  
>  	get_status(spi, &status);
> -	if (test_bit(FAIL, &status)) {
> +	if (status & MACHXO2_FAIL) {
>  		ret = -EINVAL;
>  		goto fail;
>  	}
> -	dump_status_reg(&status);
> +	dump_status_reg(status);
>  
>  	spi_message_init(&msg);
>  	tx[2].tx_buf = &initaddr;
> @@ -241,7 +241,7 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  		goto fail;
>  
>  	get_status(spi, &status);
> -	dump_status_reg(&status);
> +	dump_status_reg(status);
>  
>  	return 0;
>  fail:
> @@ -258,7 +258,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
>  	struct spi_transfer tx;
>  	static const u8 progincr[] = LSC_PROGINCRNV;
>  	u8 payload[MACHXO2_BUF_SIZE];
> -	unsigned long status;
> +	u32 status;
>  	int i, ret;
>  
>  	if (count % MACHXO2_PAGE_SIZE != 0) {
> @@ -266,7 +266,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
>  		return -EINVAL;
>  	}
>  	get_status(spi, &status);
> -	dump_status_reg(&status);
> +	dump_status_reg(status);
>  	memcpy(payload, &progincr, sizeof(progincr));
>  	for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
>  		memcpy(&payload[sizeof(progincr)], &buf[i], MACHXO2_PAGE_SIZE);
> @@ -284,7 +284,7 @@ static int machxo2_write(struct fpga_manager *mgr, const char *buf,
>  		}
>  	}
>  	get_status(spi, &status);
> -	dump_status_reg(&status);
> +	dump_status_reg(status);
>  
>  	return 0;
>  }
> @@ -297,7 +297,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>  	struct spi_transfer tx[2];
>  	static const u8 progdone[] = ISC_PROGRAMDONE;
>  	static const u8 refresh[] = LSC_REFRESH;
> -	unsigned long status;
> +	u32 status;
>  	int ret, refreshloop = 0;
>  
>  	memset(tx, 0, sizeof(tx));
> @@ -313,8 +313,8 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>  		goto fail;
>  
>  	get_status(spi, &status);
> -	dump_status_reg(&status);
> -	if (!test_bit(DONE, &status)) {
> +	dump_status_reg(status);
> +	if (!(status & MACHXO2_DONE)) {
>  		machxo2_cleanup(mgr);
>  		ret = -EINVAL;
>  		goto fail;
> @@ -333,7 +333,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>  
>  		/* check refresh status */
>  		get_status(spi, &status);
> -		dump_status_reg(&status);
> +		dump_status_reg(status);
>  		if (machxo2_status_done(status))
>  			break;
>  		if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
> @@ -344,7 +344,7 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>  	} while (1);
>  
>  	get_status(spi, &status);
> -	dump_status_reg(&status);
> +	dump_status_reg(status);
>  
>  	return 0;
>  fail:
> -- 
> 2.30.2
> 

Re: [PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Xu Yilun]

On 2022-08-25 at 16:13:42 +0200, Johannes Zink wrote:
> Measurements showed that some FPGAs take significantly longer than the
> default wait function supplied. The datasheet inidicates up to 30
> seconds erase times for some MachXO2 FPGAs, depending on the number of
> LUTs (and the corresponding configuration flash size).
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
> index ccf9a50fc590..e8967cdee2c6 100644
> --- a/drivers/fpga/machxo2-common.c
> +++ b/drivers/fpga/machxo2-common.c
> @@ -17,6 +17,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/property.h>
> +#include <linux/iopoll.h>
> +#include <linux/time.h>
>  #include "machxo2-common.h"
>  
>  #define MACHXO2_LOW_DELAY_USEC          5
> @@ -24,6 +26,8 @@
>  #define MACHXO2_REFRESH_USEC            4800
>  #define MACHXO2_MAX_BUSY_LOOP           128
>  #define MACHXO2_MAX_REFRESH_LOOP        16
> +#define MACHXO2_MAX_ERASE_USEC          (30 * USEC_PER_SEC)
> +#define MACHXO2_ERASE_USEC_SLEEP        (20 * USEC_PER_MSEC)
>  
>  #define MACHXO2_PAGE_SIZE               16
>  #define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
> @@ -54,6 +58,18 @@
>  #define ISC_ERASE_FEATURE_ROW	BIT(17)
>  #define ISC_ERASE_UFM		BIT(19)
>  
> +static inline int machxo2_wait_until_not_busy_timeout(struct machxo2_common_priv *priv)
> +{
> +	int ret, pollret;
> +	u32 status = MACHXO2_BUSY;
> +
> +	pollret = read_poll_timeout(priv->get_status, ret,
> +				    (ret && ret != -EAGAIN) || !(status & MACHXO2_BUSY),
> +				    MACHXO2_ERASE_USEC_SLEEP, MACHXO2_MAX_ERASE_USEC,
> +				    true, priv, &status);

Why just taking care of erase timeout? I see the busy wait in many
places.

> +
> +	return ret ?: pollret;
> +}
>  
>  static inline u8 get_err(u32 status)
>  {
> @@ -114,6 +130,12 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
>  	if (ret)
>  		goto fail;
>  
> +	ret = machxo2_wait_until_not_busy_timeout(priv);
> +	if (ret) {
> +		dev_err(&mgr->dev, "Erase operation failed (%d)", ret);
> +		goto fail;
> +	}
> +
>  	ret = machxo2_wait_until_not_busy(priv);

Is this line still needed?

>  	if (ret)
>  		goto fail;
> @@ -192,9 +214,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  	if (ret)
>  		goto fail;
>  
> -	ret = machxo2_wait_until_not_busy(priv);
> -	if (ret)
> +	ret = machxo2_wait_until_not_busy_timeout(priv);
> +	if (ret) {
> +		dev_err(&mgr->dev, "Erase operation failed (%d)", ret);
>  		goto fail;
> +	}
>  
>  	priv->get_status(priv, &status);
>  	if (status & MACHXO2_FAIL) {
> -- 
> 2.30.2
> 

Re: [PATCH 16/16] fpga: machxo2: add configuration over i2c

[Xu Yilun]

On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote:
> From: Peter Jensen <pdj@bang-olufsen.dk>
> 
> The configuration flash of the machxo2 fpga can also be erased and
> written over i2c instead of spi. Add this functionality to the
> refactored common driver. Since some commands are shorter over I2C than
> they are over SPI some quirks are added to the common driver in order to
> account for that.
> 
> Signed-off-by: Peter Jensen <pdj@bang-olufsen.dk>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  drivers/fpga/Kconfig          |   8 ++
>  drivers/fpga/Makefile         |   1 +
>  drivers/fpga/machxo2-common.c |  15 +++-
>  drivers/fpga/machxo2-common.h |   3 +
>  drivers/fpga/machxo2-i2c.c    | 137 ++++++++++++++++++++++++++++++++++
>  5 files changed, 163 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/fpga/machxo2-i2c.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index e5869a732246..97081bbd7c19 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -90,6 +90,14 @@ config FPGA_MGR_MACHXO2_SPI
>  	  FPGA manager driver support for Lattice MachXO2 configuration
>  	  over slave SPI interface.
>  
> +config FPGA_MGR_MACHXO2_I2C
> +	tristate "Lattice MachXO2 I2C"
> +	depends on I2C
> +	select FPGA_MGR_MACHXO2_COMMON
> +	help
> +	  FPGA manager driver support for Lattice MachXO2 configuration
> +	  over slave I2C interface.
> +
>  config FPGA_MGR_TS73XX
>  	tristate "Technologic Systems TS-73xx SBC FPGA Manager"
>  	depends on ARCH_EP93XX && MACH_TS72XX
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index f247a8de83ad..fcdf79f4d424 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_FPGA_MGR_ALTERA_PS_SPI)	+= altera-ps-spi.o
>  obj-$(CONFIG_FPGA_MGR_ICE40_SPI)	+= ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_MACHXO2_COMMON)	+= machxo2-common.o
>  obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI)	+= machxo2-spi.o
> +obj-$(CONFIG_FPGA_MGR_MACHXO2_I2C)	+= machxo2-i2c.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>  obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
> diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
> index e8967cdee2c6..0a3c126675da 100644
> --- a/drivers/fpga/machxo2-common.c
> +++ b/drivers/fpga/machxo2-common.c
> @@ -100,7 +100,7 @@ static void dump_status_reg(struct device *dev, u32 status)
>  		!!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status)));
>  }
>  
> -static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
> +int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv)
>  {
>  	u32 status;
>  	int ret, loop = 0;
> @@ -143,6 +143,11 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
>  	cmd.cmd = refresh;
>  	cmd.cmd_len = sizeof(refresh);
>  	cmd.delay_us = MACHXO2_REFRESH_USEC;
> +
> +	/* quirk: refresh command over i2c is 1 byte shorter */
> +	if (priv->refresh_3b)
> +		cmd.cmd_len--;
> +
>  	ret = priv->write_commands(priv, &cmd, 1);
>  	if (ret)
>  		goto fail;
> @@ -207,6 +212,10 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  	cmd[0].cmd_len = sizeof(enable);
>  	cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC;
>  
> +	/* quirk: enable command over i2c is 1 byte shorter */
> +	if (priv->enable_3b)
> +		cmd[0].cmd_len--;
> +
>  	cmd[1].cmd = (u8 *)&priv->erase_cmd;
>  	cmd[1].cmd_len = sizeof(priv->erase_cmd);
>  
> @@ -313,6 +322,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>  	cmd.cmd_len = sizeof(refresh);
>  	cmd.delay_us = MACHXO2_REFRESH_USEC;
>  
> +	/* quirk: refresh command over i2c is 1 byte shorter */
> +	if (priv->refresh_3b)
> +		cmd.cmd_len--;
> +
>  	do {
>  		ret = priv->write_commands(priv, &cmd, 1);
>  		if (ret)
> diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h
> index 0f9f53b48152..8c09345adee5 100644
> --- a/drivers/fpga/machxo2-common.h
> +++ b/drivers/fpga/machxo2-common.h
> @@ -32,9 +32,12 @@ struct machxo2_common_priv {
>  	int (*get_status)(struct machxo2_common_priv *priv, u32 *status);
>  	struct device *dev;
>  	__be32 erase_cmd;
> +	u8 enable_3b:1;
> +	u8 refresh_3b:1;
>  	struct gpio_desc *fpga_program_n;
>  };
>  
>  int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev);
> +int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv);
>  
>  #endif
> diff --git a/drivers/fpga/machxo2-i2c.c b/drivers/fpga/machxo2-i2c.c
> new file mode 100644
> index 000000000000..a309016def1c
> --- /dev/null
> +++ b/drivers/fpga/machxo2-i2c.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Lattice MachXO2 Slave I2C Driver
> + *
> + * Manage Lattice FPGA firmware that is loaded over I2C using
> + * the slave serial configuration interface.
> + *
> + * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com>
> + * Copyright (C) 2021 Peter Jensen <pdj@bang-olufsen.dk>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include "machxo2-common.h"
> +
> +
> +struct machxo2_i2c_priv {
> +	struct machxo2_common_priv common;
> +	struct i2c_client *client;
> +};
> +
> +static inline struct machxo2_i2c_priv *to_machxo2_i2c_priv(struct machxo2_common_priv *common)
> +{
> +	return container_of(common, struct machxo2_i2c_priv, common);
> +}
> +
> +static int machxo2_i2c_get_status(struct machxo2_common_priv *bus, u32 *status)
> +{
> +	struct machxo2_i2c_priv *i2cPriv = to_machxo2_i2c_priv(bus);
> +	struct i2c_client *client = i2cPriv->client;
> +	u8 read_status[] = LSC_READ_STATUS;

The command word could also be bus agnostic. I think a callback like
write_then_read(bus, txbuf, n_tx, rxbuf, n_rx) could be a better
abstraction.

> +	__be32 tmp;
> +	int ret;
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr = client->addr,
> +			.flags = 0,
> +			.buf = read_status,
> +			.len = ARRAY_SIZE(read_status),
> +		}, {
> +			.addr = client->addr,
> +			.flags = I2C_M_RD,
> +			.buf = (u8 *) &tmp,
> +			.len = sizeof(tmp)
> +		}
> +	};
> +
> +	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != ARRAY_SIZE(msg))
> +		return -EIO;
> +	*status = be32_to_cpu(tmp);
> +
> +	return 0;
> +}
> +
> +static int machxo2_i2c_write(struct machxo2_common_priv *common,
> +			     struct machxo2_cmd *cmds, size_t cmd_count)
> +{
> +	struct machxo2_i2c_priv *i2c_priv = to_machxo2_i2c_priv(common);
> +	struct i2c_client *client = i2c_priv->client;
> +	size_t i;
> +	int ret;
> +
> +	for (i = 0; i < cmd_count; i++) {
> +		struct i2c_msg msg[] = {
> +			{
> +				.addr = client->addr,
> +				.buf = cmds[i].cmd,
> +				.len = cmds[i].cmd_len,
> +			},
> +		};
> +
> +		ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> +		if (ret < 0)
> +			return ret;
> +		if (ret != ARRAY_SIZE(msg))
> +			return -EIO;
> +		if (cmds[i].delay_us)
> +			usleep_range(cmds[i].delay_us, cmds[i].delay_us +
> +				     cmds[i].delay_us / 4);
> +		if (i < cmd_count - 1) /* on any iteration except for the last one... */
> +			ret = machxo2_wait_until_not_busy(common);

Seems no need to implement the loop and wait in transportation layer,
they are common. A callback like write(bus, txbuf, n_tx) is better?

Thanks,
Yilun

> +	}
> +
> +	return 0;
> +}
> +
> +static int machxo2_i2c_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct machxo2_i2c_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(struct machxo2_i2c_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	priv->common.get_status = machxo2_i2c_get_status;
> +	priv->common.write_commands = machxo2_i2c_write;
> +
> +	/* Commands are usually 4b, but these aren't for i2c */
> +	priv->common.enable_3b = true;
> +	priv->common.refresh_3b = true;
> +
> +	return machxo2_common_init(&priv->common, dev);
> +}
> +
> +static const struct of_device_id of_match[] = {
> +	{ .compatible = "lattice,machxo2-slave-i2c", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_match);
> +
> +static const struct i2c_device_id lattice_ids[] = {
> +	{ "machxo2-slave-i2c", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, lattice_ids);
> +
> +static struct i2c_driver machxo2_i2c_driver = {
> +	.driver = {
> +		.name = "machxo2-slave-i2c",
> +		.of_match_table = of_match_ptr(of_match),
> +	},
> +	.probe = machxo2_i2c_probe,
> +	.id_table = lattice_ids,
> +};
> +
> +module_i2c_driver(machxo2_i2c_driver);
> +
> +MODULE_AUTHOR("Peter Jensen <pdj@bang-olufsen.dk>");
> +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C");
> +MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 

Re: [PATCH 07/16] fpga: machxo2-spi: fix big-endianness incompatibility

[Johannes Zink]

Hi Yilun, 

On Mon, 2022-08-29 at 16:19 +0800, Xu Yilun wrote:
> On 2022-08-25 at 16:13:34 +0200, Johannes Zink wrote:
> > The SPI message is written into the lowest-addressed bits of an
> > unsigned long variable, but be32_to_cpu is called on the least
> > significant bits of the variable. On big-endian 64-bit systems,
> > this would give a wrong result. Fix this by using the fixed-size
> > u32 instead of unsigned long. This clashes with the use of
> > test_bit, which is unnecessary for a single u32 variable, so
> > we adjust all usage sites appropriately and prefix the macros
> > with MACHXO2_ while at it.
> > 
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  drivers/fpga/machxo2-spi.c | 110 ++++++++++++++++++---------------
> > ----
> >  1 file changed, 55 insertions(+), 55 deletions(-)
> > 
> > diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-
> > spi.c
> > index 5e12612c7289..d1a8f28e04e7 100644
> > --- a/drivers/fpga/machxo2-spi.c
> > +++ b/drivers/fpga/machxo2-spi.c
> > @@ -9,6 +9,7 @@
> >   */
> >  
> >  #include <linux/delay.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> > @@ -41,41 +42,40 @@
> >  #define MACHXO2_BUF_SIZE               (MACHXO2_PAGE_SIZE + 4)
> >  
> >  /* Status register bits, errors and error mask */
> > -#define BUSY   12
> > -#define DONE   8
> > -#define DVER   27
> > -#define ENAB   9
> > -#define ERRBITS        23
> > -#define ERRMASK        7
> > -#define FAIL   13
> > -
> > -#define ENOERR 0 /* no error */
> > -#define EID    1
> > -#define ECMD   2
> > -#define ECRC   3
> > -#define EPREAM 4 /* preamble error */
> > -#define EABRT  5 /* abort error */
> > -#define EOVERFL        6 /* overflow error */
> > -#define ESDMEOF        7 /* SDM EOF */
> > -
> > -static inline u8 get_err(unsigned long *status)
> > +#define MACHXO2_BUSY           BIT(12)
> > +#define MACHXO2_DONE           BIT(8)
> > +#define MACHXO2_DVER           BIT(27)
> > +#define MACHXO2_ENAB           BIT(9)
> > +#define MACHXO2_ERR            GENMASK(25, 23)
> > +#define MACHXO2_ERR_ENOERR     0 /* no error */
> > +#define MACHXO2_ERR_EID                1
> > +#define MACHXO2_ERR_ECMD       2
> > +#define MACHXO2_ERR_ECRC       3
> > +#define MACHXO2_ERR_EPREAM     4 /* preamble error */
> > +#define MACHXO2_ERR_EABRT      5 /* abort error */
> > +#define MACHXO2_ERR_EOVERFL    6 /* overflow error */
> > +#define MACHXO2_ERR_ESDMEOF    7 /* SDM EOF */
> > +#define MACHXO2_FAIL           BIT(13)
> > +
> > +static inline u8 get_err(u32 status)
> >  {
> > -       return (*status >> ERRBITS) & ERRMASK;
> > +       return FIELD_GET(MACHXO2_ERR, status);
> >  }
> 
> So far the changes have nothing to do with the endian issue, is it?
> So
> please put it into another patch.

This is explained in the commit message. We need to get rid of unsigned
long, but test_bit requires using that type. Using the BIT and GENMASK
macros instead allows us to work on a u32 directly.

Admittedly, prefixing the macros with MACHXO2_ can be separated out,
but as I am already touching them to make them into masks, which is
required when not using test_bit, I chose to squash this here.

This is also mentioned in the commit message. If you prefer, I do
the rename in a separate patch. I can do that for v2, but I don't
see how I can cleanly separate s/unsigned long/u32/ and test_bit
removal.


> 
> >  
> > -static int get_status(struct spi_device *spi, unsigned long
> > *status)
> > +static int get_status(struct spi_device *spi, u32 *status)
> >  {
> >         struct spi_message msg;
> >         struct spi_transfer rx, tx;
> >         static const u8 cmd[] = LSC_READ_STATUS;
> > +       __be32 tmp;
> >         int ret;
> >  
> >         memset(&rx, 0, sizeof(rx));
> >         memset(&tx, 0, sizeof(tx));
> >         tx.tx_buf = cmd;
> >         tx.len = sizeof(cmd);
> > -       rx.rx_buf = status;
> > -       rx.len = 4;
> > +       rx.rx_buf = &tmp;
> > +       rx.len = sizeof(tmp);
> >         spi_message_init(&msg);
> >         spi_message_add_tail(&tx, &msg);
> >         spi_message_add_tail(&rx, &msg);
> > @@ -83,7 +83,7 @@ static int get_status(struct spi_device *spi,
> > unsigned long *status)
> >         if (ret)
> >                 return ret;
> >  
> > -       *status = be32_to_cpu(*status);
> > +       *status = be32_to_cpu(tmp);
> 
> Why not directly operate on the status, I don't see the difference.

I wanted to state more clearly that the value read in the rx message is
big endian and needs conversion. 

Besides, since be32_to_cpu takes a __be32 as an argument, passing 
status, which is an u32, should give a static analyzer warning.

> 
> >  
> >         return 0;
> >  }
> > @@ -91,30 +91,30 @@ static int get_status(struct spi_device *spi,
> > unsigned long *status)
> >  static const char *get_err_string(u8 err)
> >  {
> >         switch (err) {
> > -       case ENOERR:    return "No Error";
> > -       case EID:       return "ID ERR";
> > -       case ECMD:      return "CMD ERR";
> > -       case ECRC:      return "CRC ERR";
> > -       case EPREAM:    return "Preamble ERR";
> > -       case EABRT:     return "Abort ERR";
> > -       case EOVERFL:   return "Overflow ERR";
> > -       case ESDMEOF:   return "SDM EOF";
> > +       case MACHXO2_ERR_ENOERR:        return "No Error";
> > +       case MACHXO2_ERR_EID:           return "ID ERR";
> > +       case MACHXO2_ERR_ECMD:          return "CMD ERR";
> > +       case MACHXO2_ERR_ECRC:          return "CRC ERR";
> > +       case MACHXO2_ERR_EPREAM:        return "Preamble ERR";
> > +       case MACHXO2_ERR_EABRT:         return "Abort ERR";
> > +       case MACHXO2_ERR_EOVERFL:       return "Overflow ERR";
> > +       case MACHXO2_ERR_ESDMEOF:       return "SDM EOF";
> >         }
> 
> Same concern, no relation to the commit message

please see my comment above. If you prefer, I split it out in a
separate patch.

> 
> >  
> > -       return "Default switch case";
> > +       return "Unknown";
> >  }
> >  
> > -static void dump_status_reg(unsigned long *status)
> > +static void dump_status_reg(u32 status)
> >  {
> > -       pr_debug("machxo2 status: 0x%08lX - done=%d, cfgena=%d,
> > busy=%d, fail=%d, devver=%d, err=%s\n",
> > -                *status, test_bit(DONE, status), test_bit(ENAB,
> > status),
> > -                test_bit(BUSY, status), test_bit(FAIL, status),
> > -                test_bit(DVER, status),
> > get_err_string(get_err(status)));
> > +       pr_debug("machxo2 status: 0x%08X - done=%d, cfgena=%d,
> > busy=%d, fail=%d, devver=%d, err=%s\n",
> > +                 status, !!FIELD_GET(MACHXO2_DONE, status),
> > !!FIELD_GET(MACHXO2_ENAB, status),
> > +                 !!FIELD_GET(MACHXO2_BUSY, status),
> > !!FIELD_GET(MACHXO2_FAIL, status),
> > +                 !!FIELD_GET(MACHXO2_DVER, status),
> > get_err_string(get_err(status)));
> 
> Same concern. I'll not point out one by one below.
> 
> Thanks,
> Yilun
> 

as I explained in the commit message, I am removing test_bit. As far as
the macro renaming is concerned, please see my comments above.

Best regards
Johannes

> >  }
> 
> 
> >  
> >  static int wait_until_not_busy(struct spi_device *spi)
> >  {
> > -       unsigned long status;
> > +       u32 status;
> >         int ret, loop = 0;
> >  
> >         do {
> > @@ -123,7 +123,7 @@ static int wait_until_not_busy(struct
> > spi_device *spi)
> >                         return ret;
> >                 if (++loop >= MACHXO2_MAX_BUSY_LOOP)
> >                         return -EBUSY;
> > -       } while (test_bit(BUSY, &status));
> > +       } while (status & MACHXO2_BUSY);
> >  
> >         return 0;
> >  }
> > @@ -169,14 +169,14 @@ static int machxo2_cleanup(struct
> > fpga_manager *mgr)
> >  
> >  static bool machxo2_status_done(unsigned long status)
> >  {
> > -       return !test_bit(BUSY, &status) && test_bit(DONE, &status)
> > &&
> > -               get_err(&status) == ENOERR;
> > +       return (((status & (MACHXO2_BUSY | MACHXO2_DONE)) ==
> > MACHXO2_DONE) &&
> > +               get_err(status) == MACHXO2_ERR_ENOERR);
> >  }
> >  
> >  static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager
> > *mgr)
> >  {
> >         struct spi_device *spi = mgr->priv;
> > -       unsigned long status;
> > +       u32 status;
> >  
> >         get_status(spi, &status);
> >         if (machxo2_status_done(status))
> > @@ -195,7 +195,7 @@ static int machxo2_write_init(struct
> > fpga_manager *mgr,
> >         static const u8 enable[] = ISC_ENABLE;
> >         static const u8 erase[] = ISC_ERASE;
> >         static const u8 initaddr[] = LSC_INITADDRESS;
> > -       unsigned long status;
> > +       u32 status;
> >         int ret;
> >  
> >         if ((info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> > @@ -205,7 +205,7 @@ static int machxo2_write_init(struct
> > fpga_manager *mgr,
> >         }
> >  
> >         get_status(spi, &status);
> > -       dump_status_reg(&status);
> > +       dump_status_reg(status);
> >         memset(tx, 0, sizeof(tx));
> >         spi_message_init(&msg);
> >         tx[0].tx_buf = &enable;
> > @@ -226,11 +226,11 @@ static int machxo2_write_init(struct
> > fpga_manager *mgr,
> >                 goto fail;
> >  
> >         get_status(spi, &status);
> > -       if (test_bit(FAIL, &status)) {
> > +       if (status & MACHXO2_FAIL) {
> >                 ret = -EINVAL;
> >                 goto fail;
> >         }
> > -       dump_status_reg(&status);
> > +       dump_status_reg(status);
> >  
> >         spi_message_init(&msg);
> >         tx[2].tx_buf = &initaddr;
> > @@ -241,7 +241,7 @@ static int machxo2_write_init(struct
> > fpga_manager *mgr,
> >                 goto fail;
> >  
> >         get_status(spi, &status);
> > -       dump_status_reg(&status);
> > +       dump_status_reg(status);
> >  
> >         return 0;
> >  fail:
> > @@ -258,7 +258,7 @@ static int machxo2_write(struct fpga_manager
> > *mgr, const char *buf,
> >         struct spi_transfer tx;
> >         static const u8 progincr[] = LSC_PROGINCRNV;
> >         u8 payload[MACHXO2_BUF_SIZE];
> > -       unsigned long status;
> > +       u32 status;
> >         int i, ret;
> >  
> >         if (count % MACHXO2_PAGE_SIZE != 0) {
> > @@ -266,7 +266,7 @@ static int machxo2_write(struct fpga_manager
> > *mgr, const char *buf,
> >                 return -EINVAL;
> >         }
> >         get_status(spi, &status);
> > -       dump_status_reg(&status);
> > +       dump_status_reg(status);
> >         memcpy(payload, &progincr, sizeof(progincr));
> >         for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
> >                 memcpy(&payload[sizeof(progincr)], &buf[i],
> > MACHXO2_PAGE_SIZE);
> > @@ -284,7 +284,7 @@ static int machxo2_write(struct fpga_manager
> > *mgr, const char *buf,
> >                 }
> >         }
> >         get_status(spi, &status);
> > -       dump_status_reg(&status);
> > +       dump_status_reg(status);
> >  
> >         return 0;
> >  }
> > @@ -297,7 +297,7 @@ static int machxo2_write_complete(struct
> > fpga_manager *mgr,
> >         struct spi_transfer tx[2];
> >         static const u8 progdone[] = ISC_PROGRAMDONE;
> >         static const u8 refresh[] = LSC_REFRESH;
> > -       unsigned long status;
> > +       u32 status;
> >         int ret, refreshloop = 0;
> >  
> >         memset(tx, 0, sizeof(tx));
> > @@ -313,8 +313,8 @@ static int machxo2_write_complete(struct
> > fpga_manager *mgr,
> >                 goto fail;
> >  
> >         get_status(spi, &status);
> > -       dump_status_reg(&status);
> > -       if (!test_bit(DONE, &status)) {
> > +       dump_status_reg(status);
> > +       if (!(status & MACHXO2_DONE)) {
> >                 machxo2_cleanup(mgr);
> >                 ret = -EINVAL;
> >                 goto fail;
> > @@ -333,7 +333,7 @@ static int machxo2_write_complete(struct
> > fpga_manager *mgr,
> >  
> >                 /* check refresh status */
> >                 get_status(spi, &status);
> > -               dump_status_reg(&status);
> > +               dump_status_reg(status);
> >                 if (machxo2_status_done(status))
> >                         break;
> >                 if (++refreshloop == MACHXO2_MAX_REFRESH_LOOP) {
> > @@ -344,7 +344,7 @@ static int machxo2_write_complete(struct
> > fpga_manager *mgr,
> >         } while (1);
> >  
> >         get_status(spi, &status);
> > -       dump_status_reg(&status);
> > +       dump_status_reg(status);
> >  
> >         return 0;
> >  fail:
> > -- 
> > 2.30.2
> > 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Johannes Zink]

Hi Yilun, 

On Mon, 2022-08-29 at 17:26 +0800, Xu Yilun wrote:
> On 2022-08-25 at 16:13:42 +0200, Johannes Zink wrote:
> > Measurements showed that some FPGAs take significantly longer than
> > the
> > default wait function supplied. The datasheet inidicates up to 30
> > seconds erase times for some MachXO2 FPGAs, depending on the number
> > of
> > LUTs (and the corresponding configuration flash size).
> > 
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-
> > common.c
> > index ccf9a50fc590..e8967cdee2c6 100644
> > --- a/drivers/fpga/machxo2-common.c
> > +++ b/drivers/fpga/machxo2-common.c
> > @@ -17,6 +17,8 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/property.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/time.h>
> >  #include "machxo2-common.h"
> >  
> >  #define MACHXO2_LOW_DELAY_USEC          5
> > @@ -24,6 +26,8 @@
> >  #define MACHXO2_REFRESH_USEC            4800
> >  #define MACHXO2_MAX_BUSY_LOOP           128
> >  #define MACHXO2_MAX_REFRESH_LOOP        16
> > +#define MACHXO2_MAX_ERASE_USEC          (30 * USEC_PER_SEC)
> > +#define MACHXO2_ERASE_USEC_SLEEP        (20 * USEC_PER_MSEC)
> >  
> >  #define MACHXO2_PAGE_SIZE               16
> >  #define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
> > @@ -54,6 +58,18 @@
> >  #define ISC_ERASE_FEATURE_ROW  BIT(17)
> >  #define ISC_ERASE_UFM          BIT(19)
> >  
> > +static inline int machxo2_wait_until_not_busy_timeout(struct
> > machxo2_common_priv *priv)
> > +{
> > +       int ret, pollret;
> > +       u32 status = MACHXO2_BUSY;
> > +
> > +       pollret = read_poll_timeout(priv->get_status, ret,
> > +                                   (ret && ret != -EAGAIN) ||
> > !(status & MACHXO2_BUSY),
> > +                                   MACHXO2_ERASE_USEC_SLEEP,
> > MACHXO2_MAX_ERASE_USEC,
> > +                                   true, priv, &status);
> 
> Why just taking care of erase timeout? I see the busy wait in many
> places.
> 

Erasing the flash memory takes significantly longer than the other
operations (up to 30s), which is why I decided to use this separate
implementation. For other commands the fpga indicates no-more-busy much
faster than for the erase_flash command.

> > +
> > +       return ret ?: pollret;
> > +}
> >  
> >  static inline u8 get_err(u32 status)
> >  {
> > @@ -114,6 +130,12 @@ static int machxo2_cleanup(struct fpga_manager
> > *mgr)
> >         if (ret)
> >                 goto fail;
> >  
> > +       ret = machxo2_wait_until_not_busy_timeout(priv);
> > +       if (ret) {
> > +               dev_err(&mgr->dev, "Erase operation failed (%d)",
> > ret);
> > +               goto fail;
> > +       }
> > +
> >         ret = machxo2_wait_until_not_busy(priv);
> 
> Is this line still needed?

agreed, this line should become obsolete, since if we reach this point
the fpga is not indicating busy any longer or the wait has been aborted
due to an error. I will remove it in v2.

> 
> >         if (ret)
> >                 goto fail;
> > @@ -192,9 +214,11 @@ static int machxo2_write_init(struct
> > fpga_manager *mgr,
> >         if (ret)
> >                 goto fail;
> >  
> > -       ret = machxo2_wait_until_not_busy(priv);
> > -       if (ret)
> > +       ret = machxo2_wait_until_not_busy_timeout(priv);
> > +       if (ret) {
> > +               dev_err(&mgr->dev, "Erase operation failed (%d)",
> > ret);
> >                 goto fail;
> > +       }
> >  
> >         priv->get_status(priv, &status);
> >         if (status & MACHXO2_FAIL) {
> > -- 
> > 2.30.2
> > 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 16/16] fpga: machxo2: add configuration over i2c

[Johannes Zink]

Hi Yilun, 

On Mon, 2022-08-29 at 17:47 +0800, Xu Yilun wrote:
> On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote:
> > From: Peter Jensen <pdj@bang-olufsen.dk>
> > 
> > The configuration flash of the machxo2 fpga can also be erased and
> > written over i2c instead of spi. Add this functionality to the
> > refactored common driver. Since some commands are shorter over I2C
> > than
> > they are over SPI some quirks are added to the common driver in
> > order to
> > account for that.
> > 
> > Signed-off-by: Peter Jensen <pdj@bang-olufsen.dk>
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---

[snip]
> 

> > +static int machxo2_i2c_get_status(struct machxo2_common_priv *bus,
> > u32 *status)
> > +{
> > +       struct machxo2_i2c_priv *i2cPriv =
> > to_machxo2_i2c_priv(bus);
> > +       struct i2c_client *client = i2cPriv->client;
> > +       u8 read_status[] = LSC_READ_STATUS;
> 
> The command word could also be bus agnostic. I think a callback like
> write_then_read(bus, txbuf, n_tx, rxbuf, n_rx) could be a better
> abstraction.

I agree. The only command reading from the fpga is the get_status 
functionality but your proposal provides a cleaner implementation. 
I will add it in v2.
> 
> > +       __be32 tmp;
> > +       int ret;
> > +       struct i2c_msg msg[] = {
> > +               {
> > +                       .addr = client->addr,
> > +                       .flags = 0,
> > +                       .buf = read_status,
> > +                       .len = ARRAY_SIZE(read_status),
> > +               }, {
> > +                       .addr = client->addr,
> > +                       .flags = I2C_M_RD,
> > +                       .buf = (u8 *) &tmp,
> > +                       .len = sizeof(tmp)
> > +               }
> > +       };
> > +
> > +       ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > +       if (ret < 0)
> > +               return ret;
> > +       if (ret != ARRAY_SIZE(msg))
> > +               return -EIO;
> > +       *status = be32_to_cpu(tmp);
> > +
> > +       return 0;
> > +}
> > +
> > +static int machxo2_i2c_write(struct machxo2_common_priv *common,
> > +                            struct machxo2_cmd *cmds, size_t
> > cmd_count)
> > +{
> > +       struct machxo2_i2c_priv *i2c_priv =
> > to_machxo2_i2c_priv(common);
> > +       struct i2c_client *client = i2c_priv->client;
> > +       size_t i;
> > +       int ret;
> > +
> > +       for (i = 0; i < cmd_count; i++) {
> > +               struct i2c_msg msg[] = {
> > +                       {
> > +                               .addr = client->addr,
> > +                               .buf = cmds[i].cmd,
> > +                               .len = cmds[i].cmd_len,
> > +                       },
> > +               };
> > +
> > +               ret = i2c_transfer(client->adapter, msg,
> > ARRAY_SIZE(msg));
> > +               if (ret < 0)
> > +                       return ret;
> > +               if (ret != ARRAY_SIZE(msg))
> > +                       return -EIO;
> > +               if (cmds[i].delay_us)
> > +                       usleep_range(cmds[i].delay_us,
> > cmds[i].delay_us +
> > +                                    cmds[i].delay_us / 4);
> > +               if (i < cmd_count - 1) /* on any iteration except
> > for the last one... */
> > +                       ret = machxo2_wait_until_not_busy(common);
> 
> Seems no need to implement the loop and wait in transportation layer,
> they are common. A callback like write(bus, txbuf, n_tx) is better?
> 
> Thanks,
> Yilun

I have chosen this implementation mostly due to the fact that I don't
have a SPI machxo2 device to test against, so I am intentionally
keeping changes to a minimum. 

Moving the wait between single commands into the transport layer is not
functionally equivalent, e.g. the ISC_ENABLE - ISC_ERASE command
sequence in the machxo2_write_init function would require two separate
messages with a wait time between them, which would deassert the CS
line between sending the messages via SPI if not sent as a sequence of
SPI transfers. For some of the commands, the fpga requires a delay
between the different commands, which was implemented by setting the
delay property of the spi transfer objects in the original driver.

This implementation tries to mimic the timing behaviour of the SPI
transfer delay property for the I2C implementation. 

Best regards
Johannes

> 
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int machxo2_i2c_probe(struct i2c_client *client,
> > +                       const struct i2c_device_id *id)
> > +{
> > +       struct device *dev = &client->dev;
> > +       struct machxo2_i2c_priv *priv;
> > +
> > +       priv = devm_kzalloc(dev, sizeof(struct machxo2_i2c_priv),
> > GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->client = client;
> > +       priv->common.get_status = machxo2_i2c_get_status;
> > +       priv->common.write_commands = machxo2_i2c_write;
> > +
> > +       /* Commands are usually 4b, but these aren't for i2c */
> > +       priv->common.enable_3b = true;
> > +       priv->common.refresh_3b = true;
> > +
> > +       return machxo2_common_init(&priv->common, dev);
> > +}
> > +
> > +static const struct of_device_id of_match[] = {
> > +       { .compatible = "lattice,machxo2-slave-i2c", },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_match);
> > +
> > +static const struct i2c_device_id lattice_ids[] = {
> > +       { "machxo2-slave-i2c", 0 },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, lattice_ids);
> > +
> > +static struct i2c_driver machxo2_i2c_driver = {
> > +       .driver = {
> > +               .name = "machxo2-slave-i2c",
> > +               .of_match_table = of_match_ptr(of_match),
> > +       },
> > +       .probe = machxo2_i2c_probe,
> > +       .id_table = lattice_ids,
> > +};
> > +
> > +module_i2c_driver(machxo2_i2c_driver);
> > +
> > +MODULE_AUTHOR("Peter Jensen <pdj@bang-olufsen.dk>");
> > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.30.2
> > 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 16/16] fpga: machxo2: add configuration over i2c

[Xu Yilun]

On 2022-08-29 at 15:21:19 +0200, Johannes Zink wrote:
> Hi Yilun, 
> 
> On Mon, 2022-08-29 at 17:47 +0800, Xu Yilun wrote:
> > On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote:
> > > From: Peter Jensen <pdj@bang-olufsen.dk>
> > > 
> > > The configuration flash of the machxo2 fpga can also be erased and
> > > written over i2c instead of spi. Add this functionality to the
> > > refactored common driver. Since some commands are shorter over I2C
> > > than
> > > they are over SPI some quirks are added to the common driver in
> > > order to
> > > account for that.
> > > 
> > > Signed-off-by: Peter Jensen <pdj@bang-olufsen.dk>
> > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > ---
> 
> [snip]
> > 
> 
> > > +static int machxo2_i2c_get_status(struct machxo2_common_priv *bus,
> > > u32 *status)
> > > +{
> > > +       struct machxo2_i2c_priv *i2cPriv =
> > > to_machxo2_i2c_priv(bus);
> > > +       struct i2c_client *client = i2cPriv->client;
> > > +       u8 read_status[] = LSC_READ_STATUS;
> > 
> > The command word could also be bus agnostic. I think a callback like
> > write_then_read(bus, txbuf, n_tx, rxbuf, n_rx) could be a better
> > abstraction.
> 
> I agree. The only command reading from the fpga is the get_status 
> functionality but your proposal provides a cleaner implementation. 
> I will add it in v2.
> > 
> > > +       __be32 tmp;
> > > +       int ret;
> > > +       struct i2c_msg msg[] = {
> > > +               {
> > > +                       .addr = client->addr,
> > > +                       .flags = 0,
> > > +                       .buf = read_status,
> > > +                       .len = ARRAY_SIZE(read_status),
> > > +               }, {
> > > +                       .addr = client->addr,
> > > +                       .flags = I2C_M_RD,
> > > +                       .buf = (u8 *) &tmp,
> > > +                       .len = sizeof(tmp)
> > > +               }
> > > +       };
> > > +
> > > +       ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > > +       if (ret < 0)
> > > +               return ret;
> > > +       if (ret != ARRAY_SIZE(msg))
> > > +               return -EIO;
> > > +       *status = be32_to_cpu(tmp);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int machxo2_i2c_write(struct machxo2_common_priv *common,
> > > +                            struct machxo2_cmd *cmds, size_t
> > > cmd_count)
> > > +{
> > > +       struct machxo2_i2c_priv *i2c_priv =
> > > to_machxo2_i2c_priv(common);
> > > +       struct i2c_client *client = i2c_priv->client;
> > > +       size_t i;
> > > +       int ret;
> > > +
> > > +       for (i = 0; i < cmd_count; i++) {
> > > +               struct i2c_msg msg[] = {
> > > +                       {
> > > +                               .addr = client->addr,
> > > +                               .buf = cmds[i].cmd,
> > > +                               .len = cmds[i].cmd_len,
> > > +                       },
> > > +               };
> > > +
> > > +               ret = i2c_transfer(client->adapter, msg,
> > > ARRAY_SIZE(msg));
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +               if (ret != ARRAY_SIZE(msg))
> > > +                       return -EIO;
> > > +               if (cmds[i].delay_us)
> > > +                       usleep_range(cmds[i].delay_us,
> > > cmds[i].delay_us +
> > > +                                    cmds[i].delay_us / 4);
> > > +               if (i < cmd_count - 1) /* on any iteration except
> > > for the last one... */
> > > +                       ret = machxo2_wait_until_not_busy(common);
> > 
> > Seems no need to implement the loop and wait in transportation layer,
> > they are common. A callback like write(bus, txbuf, n_tx) is better?
> > 
> > Thanks,
> > Yilun
> 
> I have chosen this implementation mostly due to the fact that I don't
> have a SPI machxo2 device to test against, so I am intentionally
> keeping changes to a minimum. 
> 
> Moving the wait between single commands into the transport layer is not
> functionally equivalent, e.g. the ISC_ENABLE - ISC_ERASE command
> sequence in the machxo2_write_init function would require two separate
> messages with a wait time between them, which would deassert the CS
> line between sending the messages via SPI if not sent as a sequence of
> SPI transfers. For some of the commands, the fpga requires a delay
> between the different commands, which was implemented by setting the
> delay property of the spi transfer objects in the original driver.

Not sure if it is really a problem, but I remember SPI has various APIs
to deal with different requirements.

> 
> This implementation tries to mimic the timing behaviour of the SPI
> transfer delay property for the I2C implementation. 

Could you firstly try on that until we have real problem? Ideally this
is a cleaner implementation, is it?

Thanks,
Yilun

> 
> Best regards
> Johannes
> 
> > 
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int machxo2_i2c_probe(struct i2c_client *client,
> > > +                       const struct i2c_device_id *id)
> > > +{
> > > +       struct device *dev = &client->dev;
> > > +       struct machxo2_i2c_priv *priv;
> > > +
> > > +       priv = devm_kzalloc(dev, sizeof(struct machxo2_i2c_priv),
> > > GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       priv->client = client;
> > > +       priv->common.get_status = machxo2_i2c_get_status;
> > > +       priv->common.write_commands = machxo2_i2c_write;
> > > +
> > > +       /* Commands are usually 4b, but these aren't for i2c */
> > > +       priv->common.enable_3b = true;
> > > +       priv->common.refresh_3b = true;
> > > +
> > > +       return machxo2_common_init(&priv->common, dev);
> > > +}
> > > +
> > > +static const struct of_device_id of_match[] = {
> > > +       { .compatible = "lattice,machxo2-slave-i2c", },
> > > +       { },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, of_match);
> > > +
> > > +static const struct i2c_device_id lattice_ids[] = {
> > > +       { "machxo2-slave-i2c", 0 },
> > > +       { },
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, lattice_ids);
> > > +
> > > +static struct i2c_driver machxo2_i2c_driver = {
> > > +       .driver = {
> > > +               .name = "machxo2-slave-i2c",
> > > +               .of_match_table = of_match_ptr(of_match),
> > > +       },
> > > +       .probe = machxo2_i2c_probe,
> > > +       .id_table = lattice_ids,
> > > +};
> > > +
> > > +module_i2c_driver(machxo2_i2c_driver);
> > > +
> > > +MODULE_AUTHOR("Peter Jensen <pdj@bang-olufsen.dk>");
> > > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C");
> > > +MODULE_LICENSE("GPL");
> > > -- 
> > > 2.30.2
> > > 
> > 
> 
> -- 
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 

Re: [PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Xu Yilun]

On 2022-08-29 at 12:51:19 +0200, Johannes Zink wrote:
> Hi Yilun, 
> 
> On Mon, 2022-08-29 at 17:26 +0800, Xu Yilun wrote:
> > On 2022-08-25 at 16:13:42 +0200, Johannes Zink wrote:
> > > Measurements showed that some FPGAs take significantly longer than
> > > the
> > > default wait function supplied. The datasheet inidicates up to 30
> > > seconds erase times for some MachXO2 FPGAs, depending on the number
> > > of
> > > LUTs (and the corresponding configuration flash size).
> > > 
> > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > ---
> > >  drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-
> > > common.c
> > > index ccf9a50fc590..e8967cdee2c6 100644
> > > --- a/drivers/fpga/machxo2-common.c
> > > +++ b/drivers/fpga/machxo2-common.c
> > > @@ -17,6 +17,8 @@
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/property.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/time.h>
> > >  #include "machxo2-common.h"
> > >  
> > >  #define MACHXO2_LOW_DELAY_USEC          5
> > > @@ -24,6 +26,8 @@
> > >  #define MACHXO2_REFRESH_USEC            4800
> > >  #define MACHXO2_MAX_BUSY_LOOP           128
> > >  #define MACHXO2_MAX_REFRESH_LOOP        16
> > > +#define MACHXO2_MAX_ERASE_USEC          (30 * USEC_PER_SEC)
> > > +#define MACHXO2_ERASE_USEC_SLEEP        (20 * USEC_PER_MSEC)
> > >  
> > >  #define MACHXO2_PAGE_SIZE               16
> > >  #define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
> > > @@ -54,6 +58,18 @@
> > >  #define ISC_ERASE_FEATURE_ROW  BIT(17)
> > >  #define ISC_ERASE_UFM          BIT(19)
> > >  
> > > +static inline int machxo2_wait_until_not_busy_timeout(struct
> > > machxo2_common_priv *priv)
> > > +{
> > > +       int ret, pollret;
> > > +       u32 status = MACHXO2_BUSY;
> > > +
> > > +       pollret = read_poll_timeout(priv->get_status, ret,
> > > +                                   (ret && ret != -EAGAIN) ||
> > > !(status & MACHXO2_BUSY),
> > > +                                   MACHXO2_ERASE_USEC_SLEEP,
> > > MACHXO2_MAX_ERASE_USEC,
> > > +                                   true, priv, &status);
> > 
> > Why just taking care of erase timeout? I see the busy wait in many
> > places.
> > 
> 
> Erasing the flash memory takes significantly longer than the other
> operations (up to 30s), which is why I decided to use this separate
> implementation. For other commands the fpga indicates no-more-busy much
> faster than for the erase_flash command.

It is almost always better to have a relatively measureable timeout,
unless it is really time critical. Apparently spi/i2c transfer is not
time critical itself. So since you have implemented a better function,
why not use it?

Thanks,
Yilun

> 
> > > +
> > > +       return ret ?: pollret;
> > > +}
> > >  
> > >  static inline u8 get_err(u32 status)
> > >  {
> > > @@ -114,6 +130,12 @@ static int machxo2_cleanup(struct fpga_manager
> > > *mgr)
> > >         if (ret)
> > >                 goto fail;
> > >  
> > > +       ret = machxo2_wait_until_not_busy_timeout(priv);
> > > +       if (ret) {
> > > +               dev_err(&mgr->dev, "Erase operation failed (%d)",
> > > ret);
> > > +               goto fail;
> > > +       }
> > > +
> > >         ret = machxo2_wait_until_not_busy(priv);
> > 
> > Is this line still needed?
> 
> agreed, this line should become obsolete, since if we reach this point
> the fpga is not indicating busy any longer or the wait has been aborted
> due to an error. I will remove it in v2.
> 
> > 
> > >         if (ret)
> > >                 goto fail;
> > > @@ -192,9 +214,11 @@ static int machxo2_write_init(struct
> > > fpga_manager *mgr,
> > >         if (ret)
> > >                 goto fail;
> > >  
> > > -       ret = machxo2_wait_until_not_busy(priv);
> > > -       if (ret)
> > > +       ret = machxo2_wait_until_not_busy_timeout(priv);
> > > +       if (ret) {
> > > +               dev_err(&mgr->dev, "Erase operation failed (%d)",
> > > ret);
> > >                 goto fail;
> > > +       }
> > >  
> > >         priv->get_status(priv, &status);
> > >         if (status & MACHXO2_FAIL) {
> > > -- 
> > > 2.30.2
> > > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 

Re: [PATCH 01/16] dt-bindings: fpga: convert Lattice MachXO2 Slave binding to YAML

[Rob Herring]

On Thu, Aug 25, 2022 at 04:13:28PM +0200, Johannes Zink wrote:
> This commit prepares adding additional properties to the machxo2-slave
> device. No functional changes.

Please use get_maintainers.pl so *all* the maintainers/lists get Cc'ed. 

> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../bindings/fpga/lattice,machxo2-slave.yaml  | 46 +++++++++++++++++++
>  .../bindings/fpga/lattice-machxo2-spi.txt     | 29 ------------
>  2 files changed, 46 insertions(+), 29 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
>  delete mode 100644 Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> new file mode 100644
> index 000000000000..d05acd6b0fc6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/lattice,machxo2-slave.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Lattice MachXO2 Slave FPGA Manager Device Tree Bindings
> +
> +maintainers:
> +  - Johannes Zink <j.zink@pengutronix.de>
> +
> +description: |
> +  Device used for loading the bitstream of Lattice MachXO2 FPGAs. The
> +  programming sequence is described in FPGA-TN-02155 on www.latticesemi.com
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: lattice,machxo2-slave-spi
> +    then:
> +      $ref: /schemas/spi/spi-peripheral-props.yaml#

You don't need the 'if' because the schema is only applied if the 
compatible matches.

Blank line needed here.

> +properties:
> +  compatible:
> +    enum:
> +      - lattice,machxo2-slave-spi
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fpga@0 {
> +            compatible = "lattice,machxo2-slave-spi";
> +            spi-max-frequency = <8000000>;
> +            reg = <0>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt b/Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt
> deleted file mode 100644
> index a8c362eb160c..000000000000
> --- a/Documentation/devicetree/bindings/fpga/lattice-machxo2-spi.txt
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -Lattice MachXO2 Slave SPI FPGA Manager
> -
> -Lattice MachXO2 FPGAs support a method of loading the bitstream over
> -'slave SPI' interface.
> -
> -See 'MachXO2ProgrammingandConfigurationUsageGuide.pdf' on www.latticesemi.com
> -
> -Required properties:
> -- compatible: should contain "lattice,machxo2-slave-spi"
> -- reg: spi chip select of the FPGA
> -
> -Example for full FPGA configuration:
> -
> -	fpga-region0 {
> -		compatible = "fpga-region";
> -		fpga-mgr = <&fpga_mgr_spi>;
> -		#address-cells = <0x1>;
> -		#size-cells = <0x1>;
> -	};
> -
> -	spi1: spi@2000 {
> -        ...
> -
> -		fpga_mgr_spi: fpga-mgr@0 {
> -			compatible = "lattice,machxo2-slave-spi";
> -			spi-max-frequency = <8000000>;
> -			reg = <0>;
> -		};
> -	};
> -- 
> 2.30.2
> 
> 

Re: [PATCH 02/16] dt-bindings: fpga: machxo2-slave: add erasure properties

[Rob Herring]

On Thu, Aug 25, 2022 at 04:13:29PM +0200, Johannes Zink wrote:
> This patch introduces additional memory areas of the machxo2-slave fpga
> to be erased.

Why?

> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../bindings/fpga/lattice,machxo2-slave.yaml      | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> index d05acd6b0fc6..78f0da8f772f 100644
> --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> @@ -26,6 +26,19 @@ properties:
>      enum:
>        - lattice,machxo2-slave-spi
>  
> +  lattice,erase-sram:
> +    type: boolean
> +    description: SRAM is to be erased during flash erase operation
> +
> +  lattice,erase-feature-row:
> +    type: boolean
> +    description: Feature row is to be erased during flash erase operation
> +
> +  lattice,erase-userflash:
> +    type: boolean
> +    description: |
> +      UFM (user flash memory) is to be erased during flash erase operation

These seem like policy. It this something that's really static to a 
particular board rather than something the user would configure each 
time.

Rob

Re: [PATCH 04/16] dt-bindings: fpga: machxo2-slave: add lattice,machxo2-slave-i2c compatible

[Rob Herring]

On Thu, Aug 25, 2022 at 04:13:31PM +0200, Johannes Zink wrote:
> Lattice MachXO2 FPGAs allow reconfiguration over I2C as well as over
> SPI. Add the I2C option to the binding as well.
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  .../bindings/fpga/lattice,machxo2-slave.yaml         | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> index 03dc134ec7b8..d48d92f27c92 100644
> --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> @@ -21,10 +21,22 @@ allOf:
>              const: lattice,machxo2-slave-spi
>      then:
>        $ref: /schemas/spi/spi-peripheral-props.yaml#

Okay, I guess you do need the if/then. Technically, you don't need it 
til this patch, but that's fine.

The allOf/if part should go after 'required' section though.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: lattice,machxo2-slave-i2c
> +    then:
> +      properties:
> +        reg:
> +          description: I2C address
> +          maxItems: 1

'reg' is needed in either case. The only conditional part is the 
description which you don't need. So this can be dropped.

> +
>  properties:
>    compatible:
>      enum:
>        - lattice,machxo2-slave-spi
> +      - lattice,machxo2-slave-i2c
>  
>    program-gpios:
>      maxItems: 1
> -- 
> 2.30.2
> 
> 

Re: [PATCH 02/16] dt-bindings: fpga: machxo2-slave: add erasure properties

[Johannes Zink]

Hi Rob, 

On Tue, 2022-08-30 at 15:36 -0500, Rob Herring wrote:
> On Thu, Aug 25, 2022 at 04:13:29PM +0200, Johannes Zink wrote:
> > This patch introduces additional memory areas of the machxo2-slave
> > fpga
> > to be erased.
> 
> Why?
> 

Depending on the bitstream loaded to the FPGA, parts of the Flash
Memory or SRAM can hold configuration data which is non-volatile over
erase cycles. With this property, the board integrator, who knows about
the fpga design, can decide whether these areas shall be erased on
update or not. As an example, think of MAC addresses for a softcore
network interface stored in UFM (user flash memory), the board
integrator might want to decide to protect this memory area over
reflashing the fpga.

> > 
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  .../bindings/fpga/lattice,machxo2-slave.yaml      | 15
> > +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> > index d05acd6b0fc6..78f0da8f772f 100644
> > --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > slave.yaml
> > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > slave.yaml
> > @@ -26,6 +26,19 @@ properties:
> >      enum:
> >        - lattice,machxo2-slave-spi
> >  
> > +  lattice,erase-sram:
> > +    type: boolean
> > +    description: SRAM is to be erased during flash erase operation
> > +
> > +  lattice,erase-feature-row:
> > +    type: boolean
> > +    description: Feature row is to be erased during flash erase
> > operation
> > +
> > +  lattice,erase-userflash:
> > +    type: boolean
> > +    description: |
> > +      UFM (user flash memory) is to be erased during flash erase
> > operation
> 
> These seem like policy. It this something that's really static to a 
> particular board rather than something the user would configure each 
> time.

From the usecases I can think of, for a given board with a given FPGA
design this is static. 

Best regards
Johannes

> 
> Rob
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 04/16] dt-bindings: fpga: machxo2-slave: add lattice,machxo2-slave-i2c compatible

[Johannes Zink]

Hi Rob,

On Tue, 2022-08-30 at 15:40 -0500, Rob Herring wrote:
> On Thu, Aug 25, 2022 at 04:13:31PM +0200, Johannes Zink wrote:
> > Lattice MachXO2 FPGAs allow reconfiguration over I2C as well as
> > over
> > SPI. Add the I2C option to the binding as well.
> > 
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  .../bindings/fpga/lattice,machxo2-slave.yaml         | 12
> > ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> > index 03dc134ec7b8..d48d92f27c92 100644
> > --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > slave.yaml
> > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > slave.yaml
> > @@ -21,10 +21,22 @@ allOf:
> >              const: lattice,machxo2-slave-spi
> >      then:
> >        $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> Okay, I guess you do need the if/then. Technically, you don't need it
> til this patch, but that's fine.
> 
> The allOf/if part should go after 'required' section though.

ack.
> 
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: lattice,machxo2-slave-i2c
> > +    then:
> > +      properties:
> > +        reg:
> > +          description: I2C address
> > +          maxItems: 1
> 
> 'reg' is needed in either case. The only conditional part is the 
> description which you don't need. So this can be dropped.

ack. I will revisit both for v2.

Best regards
Johannes
> 
> > +
> >  properties:
> >    compatible:
> >      enum:
> >        - lattice,machxo2-slave-spi
> > +      - lattice,machxo2-slave-i2c
> >  
> >    program-gpios:
> >      maxItems: 1
> > -- 
> > 2.30.2
> > 
> > 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 01/16] dt-bindings: fpga: convert Lattice MachXO2 Slave binding to YAML

[Johannes Zink]

Hi Rob,

On Tue, 2022-08-30 at 15:30 -0500, Rob Herring wrote:
> On Thu, Aug 25, 2022 at 04:13:28PM +0200, Johannes Zink wrote:
> > This commit prepares adding additional properties to the machxo2-
> > slave
> > device. No functional changes.
> 
> Please use get_maintainers.pl so *all* the maintainers/lists get
> Cc'ed. 
> 

ok, will do that in the future.

> > 
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  .../bindings/fpga/lattice,machxo2-slave.yaml  | 46
> > +++++++++++++++++++
> >  .../bindings/fpga/lattice-machxo2-spi.txt     | 29 ------------
> >  2 files changed, 46 insertions(+), 29 deletions(-)
> >  create mode 100644
> > Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/fpga/lattice-
> > machxo2-spi.txt
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-slave.yaml
> > new file mode 100644
> > index 000000000000..d05acd6b0fc6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > slave.yaml
> > @@ -0,0 +1,46 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:  
> > http://devicetree.org/schemas/fpga/lattice,machxo2-slave.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Lattice MachXO2 Slave FPGA Manager Device Tree Bindings
> > +
> > +maintainers:
> > +  - Johannes Zink <j.zink@pengutronix.de>
> > +
> > +description: |
> > +  Device used for loading the bitstream of Lattice MachXO2 FPGAs.
> > The
> > +  programming sequence is described in FPGA-TN-02155 on  
> > www.latticesemi.com
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: lattice,machxo2-slave-spi
> > +    then:
> > +      $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> You don't need the 'if' because the schema is only applied if the 
> compatible matches.
> 
> Blank line needed here.

ack, will fix it in v2

Best regards
Johannes

> 
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - lattice,machxo2-slave-spi
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    spi0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        fpga@0 {
> > +            compatible = "lattice,machxo2-slave-spi";
> > +            spi-max-frequency = <8000000>;
> > +            reg = <0>;
> > +        };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/fpga/lattice-
> > machxo2-spi.txt b/Documentation/devicetree/bindings/fpga/lattice-
> > machxo2-spi.txt
> > deleted file mode 100644
> > index a8c362eb160c..000000000000
> > --- a/Documentation/devicetree/bindings/fpga/lattice-machxo2-
> > spi.txt
> > +++ /dev/null
> > @@ -1,29 +0,0 @@
> > -Lattice MachXO2 Slave SPI FPGA Manager
> > -
> > -Lattice MachXO2 FPGAs support a method of loading the bitstream
> > over
> > -'slave SPI' interface.
> > -
> > -See 'MachXO2ProgrammingandConfigurationUsageGuide.pdf' on  
> > www.latticesemi.com
> > -
> > -Required properties:
> > -- compatible: should contain "lattice,machxo2-slave-spi"
> > -- reg: spi chip select of the FPGA
> > -
> > -Example for full FPGA configuration:
> > -
> > -       fpga-region0 {
> > -               compatible = "fpga-region";
> > -               fpga-mgr = <&fpga_mgr_spi>;
> > -               #address-cells = <0x1>;
> > -               #size-cells = <0x1>;
> > -       };
> > -
> > -       spi1: spi@2000 {
> > -        ...
> > -
> > -               fpga_mgr_spi: fpga-mgr@0 {
> > -                       compatible = "lattice,machxo2-slave-spi";
> > -                       spi-max-frequency = <8000000>;
> > -                       reg = <0>;
> > -               };
> > -       };
> > -- 
> > 2.30.2
> > 
> > 
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 02/16] dt-bindings: fpga: machxo2-slave: add erasure properties

[Johannes Zink]

Hi Yilun, 

On Mon, 2022-08-29 at 23:09 +0800, Xu Yilun wrote:
> On 2022-08-29 at 10:41:42 +0200, Johannes Zink wrote:
> > On Mon, 2022-08-29 at 15:39 +0800, Xu Yilun wrote:
> > > On 2022-08-25 at 16:13:29 +0200, Johannes Zink wrote:
> > > > This patch introduces additional memory areas of the machxo2-
> > > > slave
> > > > fpga
> > > > to be erased.
> > > > 
> > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > > ---
> > > >  .../bindings/fpga/lattice,machxo2-slave.yaml      | 15
> > > > +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > index d05acd6b0fc6..78f0da8f772f 100644
> > > > --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > @@ -26,6 +26,19 @@ properties:
> > > >      enum:
> > > >        - lattice,machxo2-slave-spi
> > > >  
> > > > +  lattice,erase-sram:
> > > > +    type: boolean
> > > > +    description: SRAM is to be erased during flash erase
> > > > operation
> > > > +
> > > > +  lattice,erase-feature-row:
> > > > +    type: boolean
> > > > +    description: Feature row is to be erased during flash
> > > > erase
> > > > operation
> > > > +
> > > > +  lattice,erase-userflash:
> > > > +    type: boolean
> > > > +    description: |
> > > > +      UFM (user flash memory) is to be erased during flash
> > > > erase
> > > > operation
> > > 
> > > In which conditions should we decide to erase each area?
> > > 
> > > Thanks,
> > > Yilun
> > 
> > Hi Yilun, 
> > 
> > the flash regions to be erased depend on the system design or
> > usecase.
> > For example, if non-volatile configuration is stored in the user
> > flash
> > memory, you might want to keep it from being erased in an in-field
> > upgrade, but you might want to clear it at factory bringup.
> 
> So these are all about user requirement, not the hardware
> capabilities.
> I think you should not put them here. If the user wants a different
> erase option supported by HW, why the driver prevents it?
> 
> Thanks,
> Yilun

I think it is rather a decision made by board-integrator, who will also
write the DT, than a decision made by the user at runtime, because the
integrator may decide to use the UFM (user flash memory) as a non-
volatile storage, e.g. for mac addresses to a softcore ethernet mac
implementation, or may decide to keep the security and readout-
protection flags in the user row from being erased.

On the other hand I do not have a very strong preference to have these
properties set via the device tree (I also guess that you may have
different usecases in mind), it simply appeared to me to fit quite well
when I thought about how this property is used. 

If you prefer this property to be set by another interface, please
suggest how to hand these information into the driver, since I am not
too familiar with the fpga-mgr framework and its interfaces. I can then
migrate to it for v2.

Best regards
Johannes

> 
> > 
> > Best regards
> > Johannes
> > > 
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > > @@ -42,5 +55,7 @@ examples:
> > > >              compatible = "lattice,machxo2-slave-spi";
> > > >              spi-max-frequency = <8000000>;
> > > >              reg = <0>;
> > > > +            lattice,erase-sram;
> > > > +            lattice,erase-feature-row;
> > > >          };
> > > >      };
> > > > -- 
> > > > 2.30.2
> > > > 
> > > 
> > 
> > -- 
> > Pengutronix e.K.                | Johannes Zink                  |
> > Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> > 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> > Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> > 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 03/16] dt-bindings: fpga: machxo2-slave: add pin for program sequence init

[Johannes Zink]

Hi Yilun, 
On Mon, 2022-08-29 at 23:21 +0800, Xu Yilun wrote:
> On 2022-08-29 at 11:01:16 +0200, Johannes Zink wrote:
> > On Mon, 2022-08-29 at 15:45 +0800, Xu Yilun wrote:
> > > On 2022-08-25 at 16:13:30 +0200, Johannes Zink wrote:
> > > > This commit adds a pin which initiates the FPGA programming
> > > > sequence
> > > > once pulsed low.
> > > 
> > > Why we don't have to use this pin before?
> > > 
> > > Thanks,
> > > Yilun
> > > 
> > 
> > According to the MachXO2 Programming and Configuration User Guide
> > (FPGA-TN-02155-4.4) one of the 3 following methods can be used to
> > enter
> > the programming mode: 
> > 
> > - asserting a low pulse on the program gpio
> > - cycling power to the MachXO2
> > - sending the refresh command using a configuration port
> > 
> > In most cases, the refresh command being sent on initialization of
> > the
> > driver (as in the orignal driver) will do the job, but since you
> > can
> > deactivate the configuration ports, asserting the program gpio is
> > the
> > safe way to enter programming mode. Since the original driver did
> > not
> > support setting it, I added it as optional to the binding in order
> > not
> > to introduce any breakage. 
> 
> So do we need to skip the 3rd method if we already have the 2nd?
> 
> Thanks,
> Yilun 

the datasheet suggests that the methods are not mutually exclusive, so
doing both will not hurt. Since I want to keep backwards compatibility
with the existing driver (which only supports the 3rd option: sending
the refresh command), i'd rather not drop it in order not to introduce
any breakage. 

However, in my application I cannot power-cycle the FPGA (method 2) and
the configuration port is disabled after in-factory initial programming
via a security bit in the Feature Row Flash Area, which only leaves me
with Method 1 (asserting a low pulse on the program_n pin of the FPGA)
to enter programming mode for flashing another bitstream to the FPGA. 

This patch adds support for exactly this pin, while for other system
setups both of the other methods will work just as they did before.

Best regards
Johannes

> 
> > 
> > Best regards
> > Johannes
> > 
> > > > 
> > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > > ---
> > > >  .../devicetree/bindings/fpga/lattice,machxo2-slave.yaml    | 7
> > > > +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > index 78f0da8f772f..03dc134ec7b8 100644
> > > > --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > slave.yaml
> > > > @@ -26,6 +26,12 @@ properties:
> > > >      enum:
> > > >        - lattice,machxo2-slave-spi
> > > >  
> > > > +  program-gpios:
> > > > +    maxItems: 1
> > > > +    description: |
> > > > +      GPIO Output tied to the FPGA's n_program pin to initiate
> > > > a
> > > > +      programming sequence. This pin is active low.
> > > > +
> > > >    lattice,erase-sram:
> > > >      type: boolean
> > > >      description: SRAM is to be erased during flash erase
> > > > operation
> > > > @@ -57,5 +63,6 @@ examples:
> > > >              reg = <0>;
> > > >              lattice,erase-sram;
> > > >              lattice,erase-feature-row;
> > > > +            lattice,program-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>
> > > >          };
> > > >      };
> > > > -- 
> > > > 2.30.2
> > > > 
> > > 
> > 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Johannes Zink]

Hi Yilun, 

On Mon, 2022-08-29 at 22:57 +0800, Xu Yilun wrote:
> On 2022-08-29 at 12:51:19 +0200, Johannes Zink wrote:
> > Hi Yilun, 
> > 
> > On Mon, 2022-08-29 at 17:26 +0800, Xu Yilun wrote:
> > > On 2022-08-25 at 16:13:42 +0200, Johannes Zink wrote:
> > > > Measurements showed that some FPGAs take significantly longer
> > > > than
> > > > the
> > > > default wait function supplied. The datasheet inidicates up to
> > > > 30
> > > > seconds erase times for some MachXO2 FPGAs, depending on the
> > > > number
> > > > of
> > > > LUTs (and the corresponding configuration flash size).
> > > > 
> > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > > ---

[snip]
 
> > > > +static inline int machxo2_wait_until_not_busy_timeout(struct
> > > > machxo2_common_priv *priv)
> > > > +{
> > > > +       int ret, pollret;
> > > > +       u32 status = MACHXO2_BUSY;
> > > > +
> > > > +       pollret = read_poll_timeout(priv->get_status, ret,
> > > > +                                   (ret && ret != -EAGAIN) ||
> > > > !(status & MACHXO2_BUSY),
> > > > +                                   MACHXO2_ERASE_USEC_SLEEP,
> > > > MACHXO2_MAX_ERASE_USEC,
> > > > +                                   true, priv, &status);
> > > 
> > > Why just taking care of erase timeout? I see the busy wait in
> > > many
> > > places.
> > > 
> > 
> > Erasing the flash memory takes significantly longer than the other
> > operations (up to 30s), which is why I decided to use this separate
> > implementation. For other commands the fpga indicates no-more-busy
> > much
> > faster than for the erase_flash command.
> 
> It is almost always better to have a relatively measureable timeout,
> unless it is really time critical. Apparently spi/i2c transfer is not
> time critical itself. So since you have implemented a better
> function,
> why not use it?
> 
> Thanks,
> Yilun

That's a fair point. I will look in the datasheet how long the timeouts
on the other operations should be set and will replace the former busy
wait implementation in v2. I would not expect any breakage from that.

Best regards
Johannes

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 03/16] dt-bindings: fpga: machxo2-slave: add pin for program sequence init

[Johannes Zink]

On Wed, 2022-08-31 at 09:51 +0200, Johannes Zink wrote:
> Hi Yilun, 
> On Mon, 2022-08-29 at 23:21 +0800, Xu Yilun wrote:
> > On 2022-08-29 at 11:01:16 +0200, Johannes Zink wrote:
> > > On Mon, 2022-08-29 at 15:45 +0800, Xu Yilun wrote:
> > > > On 2022-08-25 at 16:13:30 +0200, Johannes Zink wrote:
> > > > > This commit adds a pin which initiates the FPGA programming
> > > > > sequence
> > > > > once pulsed low.
> > > > 
> > > > Why we don't have to use this pin before?
> > > > 
> > > > Thanks,
> > > > Yilun
> > > > 
> > > 
> > > According to the MachXO2 Programming and Configuration User Guide
> > > (FPGA-TN-02155-4.4) one of the 3 following methods can be used to
> > > enter
> > > the programming mode: 
> > > 
> > > - asserting a low pulse on the program gpio
> > > - cycling power to the MachXO2
> > > - sending the refresh command using a configuration port
> > > 
> > > In most cases, the refresh command being sent on initialization
> > > of
> > > the
> > > driver (as in the orignal driver) will do the job, but since you
> > > can
> > > deactivate the configuration ports, asserting the program gpio is
> > > the
> > > safe way to enter programming mode. Since the original driver did
> > > not
> > > support setting it, I added it as optional to the binding in
> > > order
> > > not
> > > to introduce any breakage. 
> > 
> > So do we need to skip the 3rd method if we already have the 2nd?
> > 
> > Thanks,
> > Yilun 
> 
> the datasheet suggests that the methods are not mutually exclusive,
> so
> doing both will not hurt. Since I want to keep backwards
> compatibility
> with the existing driver (which only supports the 3rd option: sending
> the refresh command), i'd rather not drop it in order not to
> introduce
> any breakage. 
> 

On a quick side note: Sending the refresh command is required in any
case, if using the "Slave SPI" mode (i.e. flashing the bitstream via
SPI) or the I2C mode. So according to the MachXO2 Programming and
Configuration Usage Guide, this is strictly non-optional.

Best regards
Johannes

> However, in my application I cannot power-cycle the FPGA (method 2)
> and
> the configuration port is disabled after in-factory initial
> programming
> via a security bit in the Feature Row Flash Area, which only leaves
> me
> with Method 1 (asserting a low pulse on the program_n pin of the
> FPGA)
> to enter programming mode for flashing another bitstream to the
> FPGA. 
> 
> This patch adds support for exactly this pin, while for other system
> setups both of the other methods will work just as they did before.
> 
> Best regards
> Johannes
> 
> > 
> > > 
> > > Best regards
> > > Johannes
> > > 
> > > > > 
> > > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > > > ---
> > > > >  .../devicetree/bindings/fpga/lattice,machxo2-slave.yaml    |
> > > > > 7
> > > > > +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > index 78f0da8f772f..03dc134ec7b8 100644
> > > > > --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > @@ -26,6 +26,12 @@ properties:
> > > > >      enum:
> > > > >        - lattice,machxo2-slave-spi
> > > > >  
> > > > > +  program-gpios:
> > > > > +    maxItems: 1
> > > > > +    description: |
> > > > > +      GPIO Output tied to the FPGA's n_program pin to
> > > > > initiate
> > > > > a
> > > > > +      programming sequence. This pin is active low.
> > > > > +
> > > > >    lattice,erase-sram:
> > > > >      type: boolean
> > > > >      description: SRAM is to be erased during flash erase
> > > > > operation
> > > > > @@ -57,5 +63,6 @@ examples:
> > > > >              reg = <0>;
> > > > >              lattice,erase-sram;
> > > > >              lattice,erase-feature-row;
> > > > > +            lattice,program-gpios = <&gpio1 2
> > > > > GPIO_ACTIVE_LOW>
> > > > >          };
> > > > >      };
> > > > > -- 
> > > > > 2.30.2
> > > > > 
> > > > 
> > > 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 16/16] fpga: machxo2: add configuration over i2c

[Johannes Zink]

Hi Yilun,

On Mon, 2022-08-29 at 22:45 +0800, Xu Yilun wrote:
> On 2022-08-29 at 15:21:19 +0200, Johannes Zink wrote:
> > Hi Yilun, 
> > 
> > On Mon, 2022-08-29 at 17:47 +0800, Xu Yilun wrote:
> > > On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote:
> > > > From: Peter Jensen <pdj@bang-olufsen.dk>
> > > > 
> > > > The configuration flash of the machxo2 fpga can also be erased
> > > > and
> > > > written over i2c instead of spi. Add this functionality to the
> > > > refactored common driver. Since some commands are shorter over
> > > > I2C
> > > > than
> > > > they are over SPI some quirks are added to the common driver in
> > > > order to
> > > > account for that.
> > > > 
> > > > Signed-off-by: Peter Jensen <pdj@bang-olufsen.dk>
> > > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > > ---
[snip]
> > > > 
> > 
> > > 
> > > > +
> > > > +static int machxo2_i2c_write(struct machxo2_common_priv
> > > > *common,
> > > > +                            struct machxo2_cmd *cmds, size_t
> > > > cmd_count)
> > > > +{
> > > > +       struct machxo2_i2c_priv *i2c_priv =
> > > > to_machxo2_i2c_priv(common);
> > > > +       struct i2c_client *client = i2c_priv->client;
> > > > +       size_t i;
> > > > +       int ret;
> > > > +
> > > > +       for (i = 0; i < cmd_count; i++) {
> > > > +               struct i2c_msg msg[] = {
> > > > +                       {
> > > > +                               .addr = client->addr,
> > > > +                               .buf = cmds[i].cmd,
> > > > +                               .len = cmds[i].cmd_len,
> > > > +                       },
> > > > +               };
> > > > +
> > > > +               ret = i2c_transfer(client->adapter, msg,
> > > > ARRAY_SIZE(msg));
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +               if (ret != ARRAY_SIZE(msg))
> > > > +                       return -EIO;
> > > > +               if (cmds[i].delay_us)
> > > > +                       usleep_range(cmds[i].delay_us,
> > > > cmds[i].delay_us +
> > > > +                                    cmds[i].delay_us / 4);
> > > > +               if (i < cmd_count - 1) /* on any iteration
> > > > except
> > > > for the last one... */
> > > > +                       ret =
> > > > machxo2_wait_until_not_busy(common);
> > > 
> > > Seems no need to implement the loop and wait in transportation
> > > layer,
> > > they are common. A callback like write(bus, txbuf, n_tx) is
> > > better?
> > > 
> > > Thanks,
> > > Yilun
> > 
> > I have chosen this implementation mostly due to the fact that I
> > don't
> > have a SPI machxo2 device to test against, so I am intentionally
> > keeping changes to a minimum. 
> > 
> > Moving the wait between single commands into the transport layer is
> > not
> > functionally equivalent, e.g. the ISC_ENABLE - ISC_ERASE command
> > sequence in the machxo2_write_init function would require two
> > separate
> > messages with a wait time between them, which would deassert the CS
> > line between sending the messages via SPI if not sent as a sequence
> > of
> > SPI transfers. For some of the commands, the fpga requires a delay
> > between the different commands, which was implemented by setting
> > the
> > delay property of the spi transfer objects in the original driver.
> 
> Not sure if it is really a problem, but I remember SPI has various
> APIs
> to deal with different requirements.

I assume this could probably be implemented by clearing the cs_change
bit in the SPI transfer, though just sending multiple transfers in
sequence with the appropriate timing appears a bit more elegant to me,
since it doesn't reimplement the behaviour for spi, it simply extends
the i2c part for what is not supported natively in the i2c api. Either
way, some sort of waiting has to be implemented (please see my comment
below).

Also, please bear in mind that I do not have SPI connected on my board,
which is why I opted to stay as close as possible to the original
implementation and only refactor the spi transfers with functionally
equivalent code in order to keep the risk of breaking things as low as
possible.

> 
> > 
> > This implementation tries to mimic the timing behaviour of the SPI
> > transfer delay property for the I2C implementation. 
> 
> Could you firstly try on that until we have real problem? Ideally
> this
> is a cleaner implementation, is it?

The delays themselves are required by the device family datasheet and
by the sysConfig protocol definition, as part of the command sequence
timing. 

Since I have no SPI connected on my hardware, I am only able to test
the I2C implementation, which works well with the timings taken from
the original spi driver (except for the erase timeout, which needs to
be extended as seen in Patch 15 of this series). Extending the delay
times such that usleep_range can be used has proven to be acceptable,
at least for the i2c implementation. 

Best regards
Johannes

> 
> Thanks,
> Yilun
> 
> > 
> > Best regards
> > Johannes
> > 
> > > 
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int machxo2_i2c_probe(struct i2c_client *client,
> > > > +                       const struct i2c_device_id *id)
> > > > +{
> > > > +       struct device *dev = &client->dev;
> > > > +       struct machxo2_i2c_priv *priv;
> > > > +
> > > > +       priv = devm_kzalloc(dev, sizeof(struct
> > > > machxo2_i2c_priv),
> > > > GFP_KERNEL);
> > > > +       if (!priv)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       priv->client = client;
> > > > +       priv->common.get_status = machxo2_i2c_get_status;
> > > > +       priv->common.write_commands = machxo2_i2c_write;
> > > > +
> > > > +       /* Commands are usually 4b, but these aren't for i2c */
> > > > +       priv->common.enable_3b = true;
> > > > +       priv->common.refresh_3b = true;
> > > > +
> > > > +       return machxo2_common_init(&priv->common, dev);
> > > > +}
> > > > +
> > > > +static const struct of_device_id of_match[] = {
> > > > +       { .compatible = "lattice,machxo2-slave-i2c", },
> > > > +       { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, of_match);
> > > > +
> > > > +static const struct i2c_device_id lattice_ids[] = {
> > > > +       { "machxo2-slave-i2c", 0 },
> > > > +       { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i2c, lattice_ids);
> > > > +
> > > > +static struct i2c_driver machxo2_i2c_driver = {
> > > > +       .driver = {
> > > > +               .name = "machxo2-slave-i2c",
> > > > +               .of_match_table = of_match_ptr(of_match),
> > > > +       },
> > > > +       .probe = machxo2_i2c_probe,
> > > > +       .id_table = lattice_ids,
> > > > +};
> > > > +
> > > > +module_i2c_driver(machxo2_i2c_driver);
> > > > +
> > > > +MODULE_AUTHOR("Peter Jensen <pdj@bang-olufsen.dk>");
> > > > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C");
> > > > +MODULE_LICENSE("GPL");
> > > > -- 
> > > > 2.30.2
-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH 02/16] dt-bindings: fpga: machxo2-slave: add erasure properties

[Xu Yilun]

On 2022-08-31 at 09:38:31 +0200, Johannes Zink wrote:
> Hi Yilun, 
> 
> On Mon, 2022-08-29 at 23:09 +0800, Xu Yilun wrote:
> > On 2022-08-29 at 10:41:42 +0200, Johannes Zink wrote:
> > > On Mon, 2022-08-29 at 15:39 +0800, Xu Yilun wrote:
> > > > On 2022-08-25 at 16:13:29 +0200, Johannes Zink wrote:
> > > > > This patch introduces additional memory areas of the machxo2-
> > > > > slave
> > > > > fpga
> > > > > to be erased.
> > > > > 
> > > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > > > > ---
> > > > >  .../bindings/fpga/lattice,machxo2-slave.yaml      | 15
> > > > > +++++++++++++++
> > > > >  1 file changed, 15 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > index d05acd6b0fc6..78f0da8f772f 100644
> > > > > --- a/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > +++ b/Documentation/devicetree/bindings/fpga/lattice,machxo2-
> > > > > slave.yaml
> > > > > @@ -26,6 +26,19 @@ properties:
> > > > >      enum:
> > > > >        - lattice,machxo2-slave-spi
> > > > >  
> > > > > +  lattice,erase-sram:
> > > > > +    type: boolean
> > > > > +    description: SRAM is to be erased during flash erase
> > > > > operation
> > > > > +
> > > > > +  lattice,erase-feature-row:
> > > > > +    type: boolean
> > > > > +    description: Feature row is to be erased during flash
> > > > > erase
> > > > > operation
> > > > > +
> > > > > +  lattice,erase-userflash:
> > > > > +    type: boolean
> > > > > +    description: |
> > > > > +      UFM (user flash memory) is to be erased during flash
> > > > > erase
> > > > > operation
> > > > 
> > > > In which conditions should we decide to erase each area?
> > > > 
> > > > Thanks,
> > > > Yilun
> > > 
> > > Hi Yilun, 
> > > 
> > > the flash regions to be erased depend on the system design or
> > > usecase.
> > > For example, if non-volatile configuration is stored in the user
> > > flash
> > > memory, you might want to keep it from being erased in an in-field
> > > upgrade, but you might want to clear it at factory bringup.
> > 
> > So these are all about user requirement, not the hardware
> > capabilities.
> > I think you should not put them here. If the user wants a different
> > erase option supported by HW, why the driver prevents it?
> > 
> > Thanks,
> > Yilun
> 
> I think it is rather a decision made by board-integrator, who will also
> write the DT, than a decision made by the user at runtime, because the
> integrator may decide to use the UFM (user flash memory) as a non-
> volatile storage, e.g. for mac addresses to a softcore ethernet mac
> implementation, or may decide to keep the security and readout-
> protection flags in the user row from being erased.
> 
> On the other hand I do not have a very strong preference to have these
> properties set via the device tree (I also guess that you may have
> different usecases in mind), it simply appeared to me to fit quite well
> when I thought about how this property is used. 
> 
> If you prefer this property to be set by another interface, please
> suggest how to hand these information into the driver, since I am not
> too familiar with the fpga-mgr framework and its interfaces. I can then
> migrate to it for v2.

I looked into the MACHOX2 driver & SPEC again, and found the major work
is to program the flash. I think this is actually not within the scope of
FPGA manager framework.

The FPGA manager focus on the reprogramming (or so called configuration in
MACHOX2 spec) of the FPGA region (FPGA active logic), and the removal &
enumeration of the devices within the region, before and after
reprogramming.

The existing MACHOX2 driver actually combines the reprograming of the
flash & the FPGA region, it hides many details about flash. Which is
barely OK at that time. But when you are trying to extend more
functionalities about flash, it is not proper here.

So my idea is to separate the 2 steps:
1. managing the flash, this should be implemented in other subsystem,
maybe MTD?
2. the configuration of the FPGA region retrieved from flash, this is
within the scope of FPGA manager. We don't have code now, maybe it's the
time to work on it.

Thanks,
Yilun

> 
> Best regards
> Johannes
> 
> > 
> > > 
> > > Best regards
> > > Johannes
> > > > 
> > > > > +
> > > > >  required:
> > > > >    - compatible
> > > > >    - reg
> > > > > @@ -42,5 +55,7 @@ examples:
> > > > >              compatible = "lattice,machxo2-slave-spi";
> > > > >              spi-max-frequency = <8000000>;
> > > > >              reg = <0>;
> > > > > +            lattice,erase-sram;
> > > > > +            lattice,erase-feature-row;
> > > > >          };
> > > > >      };
> > > > > -- 
> > > > > 2.30.2
> > > > > 
> > > > 
> > > 
> > > -- 
> > > Pengutronix e.K.                | Johannes Zink                  |
> > > Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> > > 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> > > Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> > > 
> > 
> 
> -- 
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
>