On Sun, Sep 04, 2022 at 12:53:27AM +0800, Xu Yilun wrote:

On 2022-08-30 at 12:54:04 +0300, Ivan Bornyakov wrote:

quoted

Add support to the FPGA manager for programming Lattice ECP5 and MachXO2
FPGAs over slave SPI sysCONFIG interface.

Signed-off-by: Ivan Bornyakov <redacted>
---

... snip ...

+static int sysconfig_spi_probe(struct spi_device *spi)
+{
+	const struct sysconfig_fpga_priv *fpga_priv;
+	const struct spi_device_id *dev_id;
+	struct device *dev = &spi->dev;
+	struct sysconfig_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	fpga_priv = of_device_get_match_data(dev);
+	if (!fpga_priv) {
+		dev_id = spi_get_device_id(spi);
+		if (!dev_id)
+			return -ENODEV;
+
+		fpga_priv = (const struct sysconfig_fpga_priv *)dev_id->driver_data;
+	}
+
+	if (!fpga_priv)
+		return -EINVAL;
+
+	if (spi->max_speed_hz > fpga_priv->spi_max_speed_hz) {
+		dev_err(dev, "SPI speed %u is too high, maximum speed is %u\n",
+			spi->max_speed_hz, fpga_priv->spi_max_speed_hz);
+		return -EINVAL;
+	}
+
+	priv->dev = dev;
+	priv->spi = spi;
+	priv->fpga_priv = fpga_priv;
+	priv->sysconfig_transfer = sysconfig_spi_transfer;
+	priv->sysconfig_fpga_mgr_ops.state = sysconfig_ops_state;
+	priv->sysconfig_fpga_mgr_ops.write_init = sysconfig_ops_write_init;

Why initialize the fpga_mgr_ops in spi driver? I think it should be the
sysconfig core driver's job.

The reasoning was that there is bus-specific write callback.

quoted

+	priv->sysconfig_fpga_mgr_ops.write = sysconfig_ops_spi_write;

We already have the transfer callback, why we need a special spi write
here.

Transfer callback performs transaction with single buffer on its input
(with optional read back). Paged write makes one write transaction with
two buffers, command + data, and it is only place where we need to
transfer several buffers. More of that, burst write needs locked bus.
IMHO, it is reasonable to have a bus-specific write.

quoted

+	priv->sysconfig_fpga_mgr_ops.write_complete = sysconfig_ops_write_complete;

same concern

quoted

+
+	return sysconfig_probe(priv);
+}
+

... snip ...

+int sysconfig_ops_write_init(struct fpga_manager *mgr,
+			     struct fpga_image_info *info,
+			     const char *buf, size_t count)
+{
+	const struct sysconfig_fpga_priv *fpga_priv;
+	struct sysconfig_priv *priv;
+	struct device *dev;
+	int ret;
+
+	dev = &mgr->dev;
+	priv = mgr->priv;
+	fpga_priv = priv->fpga_priv;
+
+	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(dev, "Partial reconfiguration is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!fpga_priv->internal_flash) {
+		/* Write directly to SRAM */
+		ret = sysconfig_refresh(priv);
+		if (ret) {
+			dev_err(dev, "Failed to go to program mode\n");
+			return ret;
+		}
+	}
+
+	/* Enter ISC mode */
+	ret = sysconfig_isc_init(priv);
+	if (ret) {
+		dev_err(dev, "Failed to go to ISC mode\n");
+		return ret;
+	}
+
+	/* Initialize the Address Shift Register */
+	ret = sysconfig_lsc_init_addr(priv);
+	if (ret) {
+		dev_err(dev,
+			"Failed to initialize the Address Shift Register\n");
+		return ret;
+	}
+
+	if (fpga_priv->spi_burst_write) {
+		/* Prepare for SPI burst write */
+		ret = sysconfig_lsc_burst_init(priv);

Don't make the sysconfig.c dependent to sysconfig-spi.c, sysconfig.c
should be common and workable without sysconfig-spi.c

Agreed. How about separate this if-block to a bus-specific
sysconfig_ops_spi_write_init()?

quoted

+		if (ret)
+			dev_err(dev,
+				"Failed to prepare for bitstream burst write\n");
+	}
+
+	return ret;
+}
+
+static void sysconfig_cleanup(struct sysconfig_priv *priv)
+{
+	sysconfig_isc_erase(priv);
+	sysconfig_refresh(priv);
+}

... snip ...

+int sysconfig_ops_write_complete(struct fpga_manager *mgr,
+				 struct fpga_image_info *info)
+{
+	const struct sysconfig_fpga_priv *fpga_priv;
+	struct sysconfig_priv *priv;
+	struct device *dev;
+	int ret;
+
+	dev = &mgr->dev;
+	priv = mgr->priv;
+	fpga_priv = priv->fpga_priv;
+
+	if (fpga_priv->spi_burst_write) {
+		ret = sysconfig_lsc_burst_complete(priv);

Same concern

Same as above, how about sysconfig_ops_spi_write_complete()?

quoted

+		if (ret) {
+			dev_err(dev,
+				"Error while waiting bitstream write to finish\n");
+			goto fail;
+		}
+	}
+
+	if (fpga_priv->internal_flash) {
+		ret = sysconfig_isc_prog_done(priv);
+		if (!ret)
+			ret = sysconfig_refresh(priv);
+
+		if (ret) {
+			dev_err(dev, "Failed to enable Self-Download Mode\n");
+			goto fail;
+		}
+	}
+
+	ret = sysconfig_isc_finish(priv);
+
+fail:
+	if (ret)
+		sysconfig_cleanup(priv);
+
+	return ret;
+}

... snip ...

+struct sysconfig_fpga_priv {
+	u32 spi_max_speed_hz;

Don't put bus specific stuff here, this should be common across all
transport.

Personally, I don't see why not. This struct is just a bunch of
FPGA-specific constants which may or may not be used by a bus-specific
sysCONFIG implementation. IMHO it is more convinient to define them in
one place.

quoted

+	u8 isc_enable_operand;
+	bool spi_burst_write;

same concern

quoted

+	bool internal_flash;
+};
+
+extern const struct sysconfig_fpga_priv ecp5_data;
+extern const struct sysconfig_fpga_priv machxo2_data;
+
+struct sysconfig_priv {
+	struct fpga_manager_ops sysconfig_fpga_mgr_ops;
+	const struct sysconfig_fpga_priv *fpga_priv;
+	struct gpio_desc *program;
+	struct gpio_desc *init;
+	struct gpio_desc *done;
+	struct spi_device *spi;

same concern

MachXO2's I2C isc_enable and lsc_refresh commands are 3-bytes long while
SPI variants are 4-bytes long. Having pointer to either spi_device or
i2c_client makes it easy to add quirks about transfer length to
sysconfig_isc_enable() and sysconfig_lsc_refresh() routines.

quoted

+	struct device *dev;
+	int (*sysconfig_transfer)(struct sysconfig_priv *priv,
+				  const void *tx_buf, size_t tx_len,
+				  void *rx_buf, size_t rx_len);
+};
+
+int sysconfig_poll_busy(struct sysconfig_priv *priv);
+int sysconfig_lsc_burst_init(struct sysconfig_priv *priv);
+int sysconfig_lsc_burst_complete(struct sysconfig_priv *priv);
+
+enum fpga_mgr_states sysconfig_ops_state(struct fpga_manager *mgr);
+int sysconfig_ops_write_init(struct fpga_manager *mgr,
+			     struct fpga_image_info *info,
+			     const char *buf, size_t count);
+int sysconfig_ops_write_complete(struct fpga_manager *mgr,
+				 struct fpga_image_info *info);
+
+int sysconfig_probe(struct sysconfig_priv *priv);
+
+#endif /* __LATTICE_SYSCONFIG_H */
-- 
2.37.2