[PATCH v1 0/6] PolarFire SoC Auto Update Support

[PATCH v1 0/6] PolarFire SoC Auto Update Support

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey all,

This patchset adds support for the "Auto Update" feature on PolarFire
SoC that allows for writing an FPGA bistream to the SPI flash connected
to the system controller.
On powercycle (or reboot depending on how the firmware implements the
openSBI SRST extension) "Auto Update" will take place, and program the
FPGA with the contents of the SPI flash - provided that that image is
valid and an actual upgrade from that already programmed!

Unfortunately, this series is not really testable yet - the Engineering
Sample silicon on most dev boards has a bug in the QSPI controller
connected to the system controller's flash and cannot access it.
Pre-production and later silicon has this bug fixed.

I previously posted an RFC about my approach in this driver, since as a
flash-based FPGA we are somewhat different to the existing
self-reprogramming drivers here. That RFC is here:
https://lore.kernel.org/linux-fpga/20221121225748.124900-1-conor@kernel.org/

This series depends on the following fixes:
https://patchwork.kernel.org/project/linux-riscv/list/?series=714160

The patch adding the driver depends on the soc patches earlier in the
series, so taking both through the same tree makes sense. Depending on
sequencing with the dependencies, me taking it through the soc tree
(with Acks etc of course) may make the most sense.

The other caveat here I guess is that this uses debugfs to trigger the
write, as we do not yet have a userspace for this yet!

Cheers,
Conor.

CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Rob Herring <robh+dt@kernel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: Moritz Fischer <mdf@kernel.org>
CC: Wu Hao <hao.wu@intel.com>
CC: Xu Yilun <yilun.xu@intel.com>
CC: Tom Rix <trix@redhat.com>
CC: linux-riscv@lists.infradead.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-fpga@vger.kernel.org

Conor Dooley (6):
  soc: microchip: mpfs: add a prefix to rx_callback()
  dt-bindings: soc: microchip: add a property for system controller
    flash
  soc: microchip: mpfs: enable access to the system controller's flash
  soc: microchip: mpfs: add auto-update subdev to system controller
  fpga: add PolarFire SoC Auto Update support
  riscv: dts: microchip: add the mpfs' system controller qspi &
    associated flash

 .../microchip,mpfs-sys-controller.yaml        |  10 +
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |  21 +
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  24 +-
 drivers/fpga/Kconfig                          |   9 +
 drivers/fpga/Makefile                         |   1 +
 drivers/fpga/microchip-auto-update.c          | 495 ++++++++++++++++++
 drivers/soc/microchip/Kconfig                 |   1 +
 drivers/soc/microchip/mpfs-sys-controller.c   |  33 +-
 include/soc/microchip/mpfs.h                  |   2 +
 9 files changed, 586 insertions(+), 10 deletions(-)
 create mode 100644 drivers/fpga/microchip-auto-update.c

-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 1/6] soc: microchip: mpfs: add a prefix to rx_callback()

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Add a prefix to the function name to match the rest of the file.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/soc/microchip/mpfs-sys-controller.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c
index 973a748d324c..11616e3c9ac8 100644
--- a/drivers/soc/microchip/mpfs-sys-controller.c
+++ b/drivers/soc/microchip/mpfs-sys-controller.c
@@ -73,7 +73,7 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *sys_controller, struct
 }
 EXPORT_SYMBOL(mpfs_blocking_transaction);
 
-static void rx_callback(struct mbox_client *client, void *msg)
+static void mpfs_sys_controller_rx_callback(struct mbox_client *client, void *msg)
 {
 	struct mpfs_sys_controller *sys_controller =
 		container_of(client, struct mpfs_sys_controller, client);
@@ -119,7 +119,7 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	sys_controller->client.dev = dev;
-	sys_controller->client.rx_callback = rx_callback;
+	sys_controller->client.rx_callback = mpfs_sys_controller_rx_callback;
 	sys_controller->client.tx_block = 1U;
 	sys_controller->client.tx_tout = msecs_to_jiffies(MPFS_SYS_CTRL_TIMEOUT_MS);
 
-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 2/6] dt-bindings: soc: microchip: add a property for system controller flash

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The system controller "shares" a SPI flash device with a QSPI controller
in the MSS. This flash is used to store FPGA bitstreams & other
metadata. IAP and Auto Upgrade both write images to this flash that the
System Controller will use to re-program the FPGA.

Add a phandle property signifying which flash device is connected to the
system controller.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../soc/microchip/microchip,mpfs-sys-controller.yaml   | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml
index 04ffee3a7c59..97a7cb74cbf9 100644
--- a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml
+++ b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml
@@ -26,6 +26,16 @@ properties:
   compatible:
     const: microchip,mpfs-sys-controller
 
+  microchip,bitstream-flash:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      The SPI flash connected to the system controller's QSPI controller.
+      The system controller may retrieve FPGA bitstreams from this flash to
+      perform In-Application Programming (IAP) or during device initialisation
+      for Auto Update. The MSS and system controller have separate QSPI
+      controllers and this flash is connected to both. Software running in the
+      MSS can write bitstreams to the flash.
+
 required:
   - compatible
   - mboxes
-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 2/6] dt-bindings: soc: microchip: add a property for system controller flash

[Rob Herring]

On Fri, Feb 17, 2023 at 04:40:19PM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The system controller "shares" a SPI flash device with a QSPI controller
> in the MSS. This flash is used to store FPGA bitstreams & other
> metadata. IAP and Auto Upgrade both write images to this flash that the
> System Controller will use to re-program the FPGA.
> 
> Add a phandle property signifying which flash device is connected to the
> system controller.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../soc/microchip/microchip,mpfs-sys-controller.yaml   | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml
> index 04ffee3a7c59..97a7cb74cbf9 100644
> --- a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml
> +++ b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml
> @@ -26,6 +26,16 @@ properties:
>    compatible:
>      const: microchip,mpfs-sys-controller
>  
> +  microchip,bitstream-flash:
> +    $ref: "/schemas/types.yaml#/definitions/phandle"

Drop quotes.

With that,

Reviewed-by: Rob Herring <robh@kernel.org>

> +    description:
> +      The SPI flash connected to the system controller's QSPI controller.
> +      The system controller may retrieve FPGA bitstreams from this flash to
> +      perform In-Application Programming (IAP) or during device initialisation
> +      for Auto Update. The MSS and system controller have separate QSPI
> +      controllers and this flash is connected to both. Software running in the
> +      MSS can write bitstreams to the flash.
> +
>  required:
>    - compatible
>    - mboxes
> -- 
> 2.39.1
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 3/6] soc: microchip: mpfs: enable access to the system controller's flash

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The system controller has a flash that contains images used to reprogram
the FPGA using IAP (In-Application Programming).
Introduce a function that allows a driver with a reference to the system
controller to get one to a flash device attached to it.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/soc/microchip/Kconfig               |  1 +
 drivers/soc/microchip/mpfs-sys-controller.c | 20 ++++++++++++++++++++
 include/soc/microchip/mpfs.h                |  2 ++
 3 files changed, 23 insertions(+)

diff --git a/drivers/soc/microchip/Kconfig b/drivers/soc/microchip/Kconfig
index eb656b33156b..9b0fdd95276e 100644
--- a/drivers/soc/microchip/Kconfig
+++ b/drivers/soc/microchip/Kconfig
@@ -1,6 +1,7 @@
 config POLARFIRE_SOC_SYS_CTRL
 	tristate "POLARFIRE_SOC_SYS_CTRL"
 	depends on POLARFIRE_SOC_MAILBOX
+	depends on MTD
 	help
 	  This driver adds support for the PolarFire SoC (MPFS) system controller.
 
diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c
index 11616e3c9ac8..bcbb4bab09e5 100644
--- a/drivers/soc/microchip/mpfs-sys-controller.c
+++ b/drivers/soc/microchip/mpfs-sys-controller.c
@@ -12,6 +12,8 @@
 #include <linux/kref.h>
 #include <linux/module.h>
 #include <linux/jiffies.h>
+#include <linux/mtd/mtd.h>
+#include <linux/spi/spi.h>
 #include <linux/interrupt.h>
 #include <linux/of_platform.h>
 #include <linux/mailbox_client.h>
@@ -30,6 +32,7 @@ struct mpfs_sys_controller {
 	struct mbox_client client;
 	struct mbox_chan *chan;
 	struct completion c;
+	struct mtd_info *flash;
 	struct kref consumers;
 };
 
@@ -97,6 +100,12 @@ static void mpfs_sys_controller_put(void *data)
 	kref_put(&sys_controller->consumers, mpfs_sys_controller_delete);
 }
 
+struct mtd_info *mpfs_sys_controller_get_flash(struct mpfs_sys_controller *mpfs_client)
+{
+	return mpfs_client->flash;
+}
+EXPORT_SYMBOL(mpfs_sys_controller_get_flash);
+
 static struct platform_device subdevs[] = {
 	{
 		.name		= "mpfs-rng",
@@ -112,12 +121,23 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mpfs_sys_controller *sys_controller;
+	struct device_node *np;
 	int i, ret;
 
 	sys_controller = kzalloc(sizeof(*sys_controller), GFP_KERNEL);
 	if (!sys_controller)
 		return -ENOMEM;
 
+	np = of_parse_phandle(dev->of_node, "microchip,bitstream-flash", 0);
+	if (!np)
+		goto no_flash;
+
+	sys_controller->flash = of_get_mtd_device_by_node(np);
+	of_node_put(np);
+	if (IS_ERR(sys_controller->flash))
+		return dev_err_probe(dev, PTR_ERR(sys_controller->flash), "Failed to get flash\n");
+
+no_flash:
 	sys_controller->client.dev = dev;
 	sys_controller->client.rx_callback = mpfs_sys_controller_rx_callback;
 	sys_controller->client.tx_block = 1U;
diff --git a/include/soc/microchip/mpfs.h b/include/soc/microchip/mpfs.h
index f916dcde457f..09722f83b0ca 100644
--- a/include/soc/microchip/mpfs.h
+++ b/include/soc/microchip/mpfs.h
@@ -38,6 +38,8 @@ int mpfs_blocking_transaction(struct mpfs_sys_controller *mpfs_client, struct mp
 
 struct mpfs_sys_controller *mpfs_sys_controller_get(struct device *dev);
 
+struct mtd_info *mpfs_sys_controller_get_flash(struct mpfs_sys_controller *mpfs_client);
+
 #endif /* if IS_ENABLED(CONFIG_POLARFIRE_SOC_SYS_CTRL) */
 
 #if IS_ENABLED(CONFIG_MCHP_CLK_MPFS)
-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 4/6] soc: microchip: mpfs: add auto-update subdev to system controller

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The PolarFire SoC's system controller offers the ability to re-program
the FPGA from a user application via two, related, mechanisms.
In-Application Programming (IAP) is not ideal for use in Linux, as it
will immediately take down the system when requested. Auto Update is
preferred, as it will only take affect at device power up*, allowing the
OS (and potential applications in AMP) to be shut down gracefully.

* Auto Update occurs at device initialisation, which can also be
  triggered by device reset - possible with the v2023.02 version of the
  Hart Software Services (HSS) and reference design.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/soc/microchip/mpfs-sys-controller.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/microchip/mpfs-sys-controller.c b/drivers/soc/microchip/mpfs-sys-controller.c
index bcbb4bab09e5..223eec66edf9 100644
--- a/drivers/soc/microchip/mpfs-sys-controller.c
+++ b/drivers/soc/microchip/mpfs-sys-controller.c
@@ -114,7 +114,11 @@ static struct platform_device subdevs[] = {
 	{
 		.name		= "mpfs-generic-service",
 		.id		= -1,
-	}
+	},
+	{
+		.name		= "mpfs-auto-update",
+		.id		= -1,
+	},
 };
 
 static int mpfs_sys_controller_probe(struct platform_device *pdev)
@@ -156,7 +160,6 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sys_controller);
 
-	dev_info(&pdev->dev, "Registered MPFS system controller\n");
 
 	for (i = 0; i < ARRAY_SIZE(subdevs); i++) {
 		subdevs[i].dev.parent = dev;
@@ -164,6 +167,8 @@ static int mpfs_sys_controller_probe(struct platform_device *pdev)
 			dev_warn(dev, "Error registering sub device %s\n", subdevs[i].name);
 	}
 
+	dev_info(&pdev->dev, "Registered MPFS system controller\n");
+
 	return 0;
 }
 
-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 5/6] fpga: add PolarFire SoC Auto Update support

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Add support for Auto Update reprogramming of the FPGA fabric on
PolarFire SoC.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/fpga/Kconfig                 |   9 +
 drivers/fpga/Makefile                |   1 +
 drivers/fpga/microchip-auto-update.c | 495 +++++++++++++++++++++++++++
 3 files changed, 505 insertions(+)
 create mode 100644 drivers/fpga/microchip-auto-update.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 6ce143dafd04..0cdd6978a440 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -257,6 +257,15 @@ config FPGA_M10_BMC_SEC_UPDATE
 	  (BMC) and provides support for secure updates for the BMC image,
 	  the FPGA image, the Root Entry Hashes, etc.
 
+config FPGA_MGR_MICROCHIP_AUTO_UPDATE
+	tristate "Microchip PolarFire SoC AUTO UPDATE"
+	depends on POLARFIRE_SOC_SYS_CTRL
+	help
+	  FPGA manager driver support for reprogramming PolarFire SoC from
+	  within Linux, using the Auto Upgrade feature of the system controller.
+
+	  If built as a module, it will be called microchip-auto-update.
+
 config FPGA_MGR_MICROCHIP_SPI
 	tristate "Microchip Polarfire SPI FPGA manager"
 	depends on SPI
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 72e554b4d2f7..a67903edf976 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
 obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)	+= versal-fpga.o
+obj-$(CONFIG_FPGA_MGR_MICROCHIP_AUTO_UPDATE)	+= microchip-auto-update.o
 obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI)	+= microchip-spi.o
 obj-$(CONFIG_FPGA_MGR_LATTICE_SYSCONFIG)	+= lattice-sysconfig.o
 obj-$(CONFIG_FPGA_MGR_LATTICE_SYSCONFIG_SPI)	+= lattice-sysconfig-spi.o
diff --git a/drivers/fpga/microchip-auto-update.c b/drivers/fpga/microchip-auto-update.c
new file mode 100644
index 000000000000..d90085f86b8b
--- /dev/null
+++ b/drivers/fpga/microchip-auto-update.c
@@ -0,0 +1,495 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip Polarfire SoC "Auto Update" FPGA reprogramming.
+ *
+ * Documentation of this functionality is available in the "PolarFire® FPGA and
+ * PolarFire SoC FPGA Programming" User Guide.
+ *
+ * Copyright (c) 2022-2023 Microchip Corporation. All rights reserved.
+ *
+ * Author: Conor Dooley <conor.dooley@microchip.com>
+ */
+#include <linux/debugfs.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/of_device.h>
+#include <linux/sizes.h>
+
+#include <soc/microchip/mpfs.h>
+
+#define AUTO_UPDATE_DEFAULT_MBOX_OFFSET		0u
+#define AUTO_UPDATE_DEFAULT_RESP_OFFSET		0u
+
+#define AUTO_UPDATE_FEATURE_CMD_OPCODE		0x05u
+#define AUTO_UPDATE_FEATURE_CMD_DATA_SIZE	0u
+#define AUTO_UPDATE_FEATURE_RESP_SIZE		33u
+#define AUTO_UPDATE_FEATURE_CMD_DATA		NULL
+#define AUTO_UPDATE_FEATURE_ENABLED		BIT(5)
+
+#define AUTO_UPDATE_AUTHENTICATE_CMD_OPCODE	0x22u
+#define AUTO_UPDATE_AUTHENTICATE_CMD_DATA_SIZE	0u
+#define AUTO_UPDATE_AUTHENTICATE_RESP_SIZE	1u
+#define AUTO_UPDATE_AUTHENTICATE_CMD_DATA	NULL
+
+#define AUTO_UPDATE_PROGRAM_CMD_OPCODE		0x46u
+#define AUTO_UPDATE_PROGRAM_CMD_DATA_SIZE	0u
+#define AUTO_UPDATE_PROGRAM_RESP_SIZE		1u
+#define AUTO_UPDATE_PROGRAM_CMD_DATA		NULL
+
+/*
+ * SPI Flash layout example:
+ * |------------------------------| 0x0000000
+ * | 1 KiB                        |
+ * | SPI "directories"            |
+ * |------------------------------| 0x0000400
+ * | 1 MiB                        |
+ * | Reserved area                |
+ * | Used for bitstream info      |
+ * |------------------------------| 0x0100400
+ * | 20 MiB                       |
+ * | Golden Image                 |
+ * |------------------------------| 0x1500400
+ * | 20 MiB                       |
+ * | Auto Upgrade Image           |
+ * |------------------------------| 0x2900400
+ * | 20 MiB                       |
+ * | Reserved for multi-image IAP |
+ * | Unused for Auto Upgrade      |
+ * |------------------------------| 0x3D00400
+ * | ? B                          |
+ * | Unused                       |
+ * |------------------------------| 0x?
+ */
+#define AUTO_UPDATE_DIRECTORY_BASE	0u
+#define AUTO_UPDATE_DIRECTORY_WIDTH	4u
+#define AUTO_UPDATE_GOLDEN_INDEX	0u
+#define AUTO_UPDATE_UPGRADE_INDEX	1u
+#define AUTO_UPDATE_BLANK_INDEX		2u
+#define AUTO_UPDATE_GOLDEN_DIRECTORY	(AUTO_UPDATE_DIRECTORY_WIDTH * AUTO_UPDATE_GOLDEN_INDEX)
+#define AUTO_UPDATE_UPGRADE_DIRECTORY	(AUTO_UPDATE_DIRECTORY_WIDTH * AUTO_UPDATE_UPGRADE_INDEX)
+#define AUTO_UPDATE_BLANK_DIRECTORY	(AUTO_UPDATE_DIRECTORY_WIDTH * AUTO_UPDATE_BLANK_INDEX)
+#define AUTO_UPDATE_DIRECTORY_SIZE	SZ_1K
+#define AUTO_UPDATE_RESERVED_SIZE	SZ_1M
+#define AUTO_UPDATE_BITSTREAM_BASE	(AUTO_UPDATE_DIRECTORY_SIZE + AUTO_UPDATE_RESERVED_SIZE)
+
+struct mpfs_auto_update_config {
+	u8 feature_response_size;
+};
+
+struct mpfs_auto_update_priv {
+	struct mpfs_sys_controller *sys_controller;
+	struct device *dev;
+	struct fpga_region *region;
+	struct mpfs_auto_update_config *config;
+	struct mtd_info *flash;
+	struct dentry *debugfs_dir;
+};
+
+static struct device *mpfs_auto_update_debug_dev;
+
+static enum fpga_mgr_states mpfs_auto_update_state(struct fpga_manager *mgr)
+{
+	struct mpfs_auto_update_priv *priv = mgr->priv;
+	struct mpfs_mss_response *response;
+	struct mpfs_mss_msg *message;
+	u32 *response_msg;
+	int ret;
+	enum fpga_mgr_states rc = FPGA_MGR_STATE_WRITE_INIT_ERR;
+
+	response_msg = devm_kzalloc(priv->dev, AUTO_UPDATE_FEATURE_RESP_SIZE * sizeof(response_msg),
+				    GFP_KERNEL);
+	if (!response_msg)
+		return FPGA_MGR_STATE_WRITE_INIT_ERR;
+
+	response = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_response), GFP_KERNEL);
+	if (!response) {
+		rc = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		goto free_response_msg;
+	}
+
+	message = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_msg), GFP_KERNEL);
+	if (!message) {
+		rc = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		goto free_response;
+	}
+
+	/*
+	 * To verify that Auto Update is possible, the "Query Security Service
+	 * Request" is performed. Bit 5 of byte 1 is "UL_Auto Update" & if it is
+	 * set, Auto Update is not possible.
+	 * This service has no command data & does not overload mbox_offset.
+	 * The size of the response varies between PolarFire & PolarFire SoC.
+	 */
+	response->resp_msg = response_msg;
+	response->resp_size = AUTO_UPDATE_FEATURE_RESP_SIZE;
+	message->cmd_opcode = AUTO_UPDATE_FEATURE_CMD_OPCODE;
+	message->cmd_data_size = AUTO_UPDATE_FEATURE_CMD_DATA_SIZE;
+	message->response = response;
+	message->cmd_data = AUTO_UPDATE_FEATURE_CMD_DATA;
+	message->mbox_offset = AUTO_UPDATE_DEFAULT_MBOX_OFFSET;
+	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
+
+	ret = mpfs_blocking_transaction(priv->sys_controller, message);
+	if (ret | response->resp_status) {
+		rc = FPGA_MGR_STATE_UNKNOWN;
+		goto free_message;
+	}
+
+	if (!(response_msg[1] & AUTO_UPDATE_FEATURE_ENABLED))
+		rc = FPGA_MGR_STATE_OPERATING;
+
+free_message:
+	devm_kfree(priv->dev, message);
+free_response:
+	devm_kfree(priv->dev, response);
+free_response_msg:
+	devm_kfree(priv->dev, response_msg);
+
+	return rc;
+}
+
+static int mpfs_auto_update_write_init(struct fpga_manager *mgr, struct fpga_image_info *info,
+				       const char *buf, size_t count)
+{
+	/*
+	 * Verifying the Golden Image is idealistic. It will be evaluated
+	 * against the currently programmed image and thus may fail - due to
+	 * either rollback protection (if its an older version than that in use)
+	 * or if the version is the same as that of the in-use image.
+	 * Extracting the information as to why a failure occurred is not
+	 * currently possible due to limitations of the system controller
+	 * driver. If those are fixed, verification of the Golden Image should
+	 * be added here.
+	 */
+	return 0;
+}
+
+static int mpfs_auto_update_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+	/*
+	 * No parsing etc of the bitstream is required. The system controller
+	 * will do all of that itself - including verifying that the bitstream
+	 * is valid.
+	 */
+	struct mpfs_auto_update_priv *priv = mgr->priv;
+	struct erase_info erase;
+	char *buffer;
+	loff_t directory_address = AUTO_UPDATE_UPGRADE_DIRECTORY;
+	size_t bytes_written = 0, bytes_read = 0;
+	size_t erase_size = AUTO_UPDATE_DIRECTORY_SIZE;
+	size_t size_per_bitstream = 0;
+	u32 image_address;
+	int ret;
+
+	priv->flash = mpfs_sys_controller_get_flash(priv->sys_controller);
+	if (!priv->flash)
+		return -EIO;
+
+	erase_size = round_up(erase_size, (u64)priv->flash->erasesize);
+
+	/*
+	 * We need to calculate if we have enough space in the flash for the
+	 * new image.
+	 * First, chop off the first 1 KiB as it's reserved for the directory.
+	 * The 1 MiB reserved for design info needs to be ignored also.
+	 * All that remains is carved into 3 & rounded down to the erasesize.
+	 * If this is smaller than the image size, we abort.
+	 * There's also no need to consume more than 20 MiB per image.
+	 */
+	size_per_bitstream = priv->flash->size - SZ_1K - SZ_1M;
+	size_per_bitstream = round_down(size_per_bitstream / 3, erase_size);
+	if (size_per_bitstream > 20 * SZ_1M)
+		size_per_bitstream = 20 * SZ_1M;
+
+	if (size_per_bitstream < count) {
+		dev_err(priv->dev,
+			"flash device has insufficient capacity to store this bitstream\n");
+		return -EINVAL;
+	}
+
+	image_address = AUTO_UPDATE_BITSTREAM_BASE + AUTO_UPDATE_UPGRADE_INDEX * size_per_bitstream;
+
+	buffer = devm_kzalloc(priv->dev, erase_size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	erase.addr = AUTO_UPDATE_DIRECTORY_BASE;
+	erase.len = erase_size;
+
+	/*
+	 * We need to write the "SPI DIRECTORY" to the first 1 KiB, telling
+	 * the system controller where to find the actual bitstream. Since
+	 * this is spi-nor, we have to read the first eraseblock, erase that
+	 * portion of the flash, modify the data and then write it back.
+	 */
+	ret = mtd_read(priv->flash, AUTO_UPDATE_DIRECTORY_BASE, erase_size, &bytes_read,
+		       (u_char *)buffer);
+	if (ret)
+		goto out;
+
+	if (bytes_read != erase_size) {
+		ret = -EIO;
+		goto out;
+	}
+
+	ret = mtd_erase(priv->flash, &erase);
+	if (ret)
+		goto out;
+
+	/*
+	 * Populate the image address and then zero out the next directory so
+	 * that the system controller doesn't complain if in "Single Image"
+	 * mode.
+	 */
+	memcpy(buffer + AUTO_UPDATE_UPGRADE_DIRECTORY, &image_address, AUTO_UPDATE_DIRECTORY_WIDTH);
+	memset(buffer + AUTO_UPDATE_BLANK_DIRECTORY, 0x0, AUTO_UPDATE_DIRECTORY_WIDTH);
+
+	dev_info(priv->dev, "Writing the image address (%x) to the flash directory (%llx)\n",
+		 image_address, directory_address);
+
+	ret = mtd_write(priv->flash, 0x0, erase_size, &bytes_written, (u_char *)buffer);
+	if (ret)
+		goto out;
+
+	if (bytes_written != erase_size) {
+		ret = -EIO;
+		goto out;
+	}
+
+	/*
+	 * Now the .spi image itself can be written to the flash. Preservation
+	 * of contents here is not important here, unlike the spi "directory"
+	 * which must be RMWed.
+	 */
+	erase.len = round_up(count, (size_t)priv->flash->erasesize);
+	erase.addr = image_address;
+
+	dev_info(priv->dev, "Erasing the flash at address (%x)\n", image_address);
+	ret = mtd_erase(priv->flash, &erase);
+	if (ret)
+		goto out;
+
+	dev_info(priv->dev, "Writing the image to the flash at address (%x)\n", image_address);
+	ret = mtd_write(priv->flash, (loff_t)image_address, count, &bytes_written, buf);
+	if (ret)
+		goto out;
+
+	if (bytes_written != count)
+		ret = -EIO;
+
+out:
+	devm_kfree(priv->dev, buffer);
+	return ret;
+}
+
+static int mpfs_auto_update_write_complete(struct fpga_manager *mgr, struct fpga_image_info *info)
+{
+	struct mpfs_auto_update_priv *priv = mgr->priv;
+	struct mpfs_mss_response *response;
+	struct mpfs_mss_msg *message;
+	u32 *response_msg;
+	int ret;
+
+	response_msg = devm_kzalloc(priv->dev, AUTO_UPDATE_FEATURE_RESP_SIZE * sizeof(response_msg),
+				    GFP_KERNEL);
+	if (!response_msg)
+		return -ENOMEM;
+
+	response = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_response), GFP_KERNEL);
+	if (!response) {
+		ret = -ENOMEM;
+		goto free_response_msg;
+	}
+
+	message = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_msg), GFP_KERNEL);
+	if (!message) {
+		ret = -ENOMEM;
+		goto free_response;
+	}
+
+	/*
+	 * The system controller can verify that an image in the flash is valid.
+	 * Rather than duplicate the check in this driver, call the relevant
+	 * service from the system controller instead.
+	 * This service has no command data and no response data. It overloads
+	 * mbox_offset with the image index in the flash's SPI directory where
+	 * the bitstream is located.
+	 */
+	response->resp_msg = response_msg;
+	response->resp_size = AUTO_UPDATE_AUTHENTICATE_RESP_SIZE;
+	message->cmd_opcode = AUTO_UPDATE_AUTHENTICATE_CMD_OPCODE;
+	message->cmd_data_size = AUTO_UPDATE_AUTHENTICATE_CMD_DATA_SIZE;
+	message->response = response;
+	message->cmd_data = AUTO_UPDATE_AUTHENTICATE_CMD_DATA;
+	message->mbox_offset = AUTO_UPDATE_UPGRADE_INDEX;
+	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
+
+	dev_info(priv->dev, "Running verification of Upgrade Image\n");
+	ret = mpfs_blocking_transaction(priv->sys_controller, message);
+	if (ret | response->resp_status) {
+		dev_warn(priv->dev, "Verification of Upgrade Image failed!\n");
+		ret = ret ? ret : -EBADMSG;
+	}
+
+	dev_info(priv->dev, "Verification of Upgrade Image passed!\n");
+//	/*
+//	 * If the validation has passed, initiate Auto Update.
+//	 * This service has no command data and no response data. It overloads
+//	 * mbox_offset with the image index in the flash's SPI directory where
+//	 * the bitstream is located.
+//	 * Once we attempt Auto Update either:
+//	 * - it passes and the board reboots
+//	 * - it fails and the board reboots to recover
+//	 * - the system controller aborts and we exit "gracefully".
+//	 *   "gracefully" since there is no interrupt produced & it just times
+//	 *   out.
+//	 */
+//	response->resp_msg = response_msg;
+//	response->resp_size = AUTO_UPDATE_PROGRAM_RESP_SIZE;
+//	message->cmd_opcode = AUTO_UPDATE_PROGRAM_CMD_OPCODE;
+//	message->cmd_data_size = AUTO_UPDATE_PROGRAM_CMD_DATA_SIZE;
+//	message->response = response;
+//	message->cmd_data = AUTO_UPDATE_PROGRAM_CMD_DATA;
+//	message->mbox_offset = 0; //field is ignored
+//	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
+//
+//	dev_info(priv->dev, "Running Auto Update command\n");
+//	ret = mpfs_blocking_transaction(priv->sys_controller, message);
+//	if (ret && ret != -ETIMEDOUT)
+//		goto out;
+//
+//	/* *remove this for auto update*
+//	 * This return 0 is dead code. Either the Auto Update will fail, or it will pass
+//	 * & the FPGA will be rebooted in which case mpfs_blocking_transaction()
+//	 * will never return and Linux will die.
+//	 */
+//	return 0;
+
+	devm_kfree(priv->dev, message);
+free_response:
+	devm_kfree(priv->dev, response);
+free_response_msg:
+	devm_kfree(priv->dev, response_msg);
+
+	return ret;
+}
+
+static const struct fpga_manager_ops mpfs_auto_update_ops = {
+	.state = mpfs_auto_update_state,
+	.write_init = mpfs_auto_update_write_init,
+	.write = mpfs_auto_update_write,
+	.write_complete = mpfs_auto_update_write_complete,
+};
+
+static int mpfs_auto_update_run(struct device *dev)
+{
+	struct fpga_manager *mgr;
+	struct fpga_image_info *info;
+	int ret;
+
+	mgr = fpga_mgr_get(dev);
+	info = fpga_image_info_alloc(dev);
+
+	info->firmware_name = devm_kstrdup(dev, "mpfs_bitstream.spi", GFP_KERNEL);
+
+	ret = fpga_mgr_lock(mgr);
+	if (ret)
+		goto free_info;
+
+	ret = fpga_mgr_load(mgr, info);
+	if (ret) {
+		dev_err(dev, "Failed to write the bitstream\n");
+		goto unlock_mgr;
+	}
+
+unlock_mgr:
+	fpga_mgr_unlock(mgr);
+free_info:
+	fpga_image_info_free(info);
+	fpga_mgr_put(mgr);
+
+	return ret;
+}
+
+static ssize_t mpfs_auto_update_exec(struct file *file, const char __user *data, size_t count,
+				     loff_t *ppos)
+{
+	int ret;
+
+	ret = mpfs_auto_update_run(mpfs_auto_update_debug_dev);
+	if (ret)
+		dev_err_probe(mpfs_auto_update_debug_dev, ret, "Auto Update failed");
+
+	return count;
+}
+
+static const struct file_operations mpfs_auto_update_fops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.write = mpfs_auto_update_exec
+};
+
+static int mpfs_auto_update_debugfs_setup(struct mpfs_auto_update_priv *priv)
+{
+	priv->debugfs_dir = debugfs_create_dir("fpga", NULL);
+
+	if(IS_ERR(priv->debugfs_dir))
+		return PTR_ERR(priv->debugfs_dir);
+
+	debugfs_create_file("microchip_exec_update", 0200, priv->debugfs_dir, NULL,
+			    &mpfs_auto_update_fops);
+
+	return 0;
+}
+
+static int mpfs_auto_update_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mpfs_auto_update_priv *priv;
+	struct fpga_manager *mgr;
+	enum fpga_mgr_states state;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->sys_controller = mpfs_sys_controller_get(dev);
+	if (IS_ERR(priv->sys_controller))
+		return dev_err_probe(dev, PTR_ERR(priv->sys_controller),
+				     "Could not register as a sub device of the system controller\n");
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	mgr = devm_fpga_mgr_register(dev, "Microchip MPFS Auto Update FPGA Manager",
+				     &mpfs_auto_update_ops, priv);
+	if (IS_ERR(mgr))
+		return dev_err_probe(dev, PTR_ERR(mgr), "Could not register FPGA manager.\n");
+
+	state = mpfs_auto_update_state(mgr);
+	if (state != FPGA_MGR_STATE_OPERATING)
+		return -EIO;
+
+	ret = mpfs_auto_update_debugfs_setup(priv);
+	if (ret && ret != -ENODEV)
+		return ret;
+
+	mpfs_auto_update_debug_dev = priv->dev;
+
+	return 0;
+}
+
+static struct platform_driver mpfs_auto_update_driver = {
+	.driver = {
+		.name = "mpfs-auto-update",
+	},
+	.probe = mpfs_auto_update_probe,
+};
+module_platform_driver(mpfs_auto_update_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_DESCRIPTION("PolarFire SoC Auto Update FPGA reprogramming");
-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 5/6] fpga: add PolarFire SoC Auto Update support

[Xu Yilun]

On 2023-02-17 at 16:40:22 +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Add support for Auto Update reprogramming of the FPGA fabric on
> PolarFire SoC.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/fpga/Kconfig                 |   9 +
>  drivers/fpga/Makefile                |   1 +
>  drivers/fpga/microchip-auto-update.c | 495 +++++++++++++++++++++++++++
>  3 files changed, 505 insertions(+)
>  create mode 100644 drivers/fpga/microchip-auto-update.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 6ce143dafd04..0cdd6978a440 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -257,6 +257,15 @@ config FPGA_M10_BMC_SEC_UPDATE
>  	  (BMC) and provides support for secure updates for the BMC image,
>  	  the FPGA image, the Root Entry Hashes, etc.
>  
> +config FPGA_MGR_MICROCHIP_AUTO_UPDATE
> +	tristate "Microchip PolarFire SoC AUTO UPDATE"
> +	depends on POLARFIRE_SOC_SYS_CTRL
> +	help
> +	  FPGA manager driver support for reprogramming PolarFire SoC from
> +	  within Linux, using the Auto Upgrade feature of the system controller.
> +
> +	  If built as a module, it will be called microchip-auto-update.
> +
>  config FPGA_MGR_MICROCHIP_SPI
>  	tristate "Microchip Polarfire SPI FPGA manager"
>  	depends on SPI
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 72e554b4d2f7..a67903edf976 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>  obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)	+= versal-fpga.o
> +obj-$(CONFIG_FPGA_MGR_MICROCHIP_AUTO_UPDATE)	+= microchip-auto-update.o
>  obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI)	+= microchip-spi.o
>  obj-$(CONFIG_FPGA_MGR_LATTICE_SYSCONFIG)	+= lattice-sysconfig.o
>  obj-$(CONFIG_FPGA_MGR_LATTICE_SYSCONFIG_SPI)	+= lattice-sysconfig-spi.o
> diff --git a/drivers/fpga/microchip-auto-update.c b/drivers/fpga/microchip-auto-update.c
> new file mode 100644
> index 000000000000..d90085f86b8b
> --- /dev/null
> +++ b/drivers/fpga/microchip-auto-update.c
> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip Polarfire SoC "Auto Update" FPGA reprogramming.
> + *
> + * Documentation of this functionality is available in the "PolarFire® FPGA and
> + * PolarFire SoC FPGA Programming" User Guide.
> + *
> + * Copyright (c) 2022-2023 Microchip Corporation. All rights reserved.
> + *
> + * Author: Conor Dooley <conor.dooley@microchip.com>
> + */
> +#include <linux/debugfs.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/of_device.h>
> +#include <linux/sizes.h>
> +
> +#include <soc/microchip/mpfs.h>
> +
> +#define AUTO_UPDATE_DEFAULT_MBOX_OFFSET		0u
> +#define AUTO_UPDATE_DEFAULT_RESP_OFFSET		0u
> +
> +#define AUTO_UPDATE_FEATURE_CMD_OPCODE		0x05u
> +#define AUTO_UPDATE_FEATURE_CMD_DATA_SIZE	0u
> +#define AUTO_UPDATE_FEATURE_RESP_SIZE		33u
> +#define AUTO_UPDATE_FEATURE_CMD_DATA		NULL
> +#define AUTO_UPDATE_FEATURE_ENABLED		BIT(5)
> +
> +#define AUTO_UPDATE_AUTHENTICATE_CMD_OPCODE	0x22u
> +#define AUTO_UPDATE_AUTHENTICATE_CMD_DATA_SIZE	0u
> +#define AUTO_UPDATE_AUTHENTICATE_RESP_SIZE	1u
> +#define AUTO_UPDATE_AUTHENTICATE_CMD_DATA	NULL
> +
> +#define AUTO_UPDATE_PROGRAM_CMD_OPCODE		0x46u
> +#define AUTO_UPDATE_PROGRAM_CMD_DATA_SIZE	0u
> +#define AUTO_UPDATE_PROGRAM_RESP_SIZE		1u
> +#define AUTO_UPDATE_PROGRAM_CMD_DATA		NULL
> +
> +/*
> + * SPI Flash layout example:
> + * |------------------------------| 0x0000000
> + * | 1 KiB                        |
> + * | SPI "directories"            |
> + * |------------------------------| 0x0000400
> + * | 1 MiB                        |
> + * | Reserved area                |
> + * | Used for bitstream info      |
> + * |------------------------------| 0x0100400
> + * | 20 MiB                       |
> + * | Golden Image                 |
> + * |------------------------------| 0x1500400
> + * | 20 MiB                       |
> + * | Auto Upgrade Image           |
> + * |------------------------------| 0x2900400
> + * | 20 MiB                       |
> + * | Reserved for multi-image IAP |
> + * | Unused for Auto Upgrade      |
> + * |------------------------------| 0x3D00400
> + * | ? B                          |
> + * | Unused                       |
> + * |------------------------------| 0x?
> + */
> +#define AUTO_UPDATE_DIRECTORY_BASE	0u
> +#define AUTO_UPDATE_DIRECTORY_WIDTH	4u
> +#define AUTO_UPDATE_GOLDEN_INDEX	0u
> +#define AUTO_UPDATE_UPGRADE_INDEX	1u
> +#define AUTO_UPDATE_BLANK_INDEX		2u
> +#define AUTO_UPDATE_GOLDEN_DIRECTORY	(AUTO_UPDATE_DIRECTORY_WIDTH * AUTO_UPDATE_GOLDEN_INDEX)
> +#define AUTO_UPDATE_UPGRADE_DIRECTORY	(AUTO_UPDATE_DIRECTORY_WIDTH * AUTO_UPDATE_UPGRADE_INDEX)
> +#define AUTO_UPDATE_BLANK_DIRECTORY	(AUTO_UPDATE_DIRECTORY_WIDTH * AUTO_UPDATE_BLANK_INDEX)
> +#define AUTO_UPDATE_DIRECTORY_SIZE	SZ_1K
> +#define AUTO_UPDATE_RESERVED_SIZE	SZ_1M
> +#define AUTO_UPDATE_BITSTREAM_BASE	(AUTO_UPDATE_DIRECTORY_SIZE + AUTO_UPDATE_RESERVED_SIZE)
> +
> +struct mpfs_auto_update_config {
> +	u8 feature_response_size;
> +};
> +
> +struct mpfs_auto_update_priv {
> +	struct mpfs_sys_controller *sys_controller;
> +	struct device *dev;
> +	struct fpga_region *region;
> +	struct mpfs_auto_update_config *config;
> +	struct mtd_info *flash;
> +	struct dentry *debugfs_dir;
> +};
> +
> +static struct device *mpfs_auto_update_debug_dev;
> +
> +static enum fpga_mgr_states mpfs_auto_update_state(struct fpga_manager *mgr)
> +{
> +	struct mpfs_auto_update_priv *priv = mgr->priv;
> +	struct mpfs_mss_response *response;
> +	struct mpfs_mss_msg *message;
> +	u32 *response_msg;
> +	int ret;
> +	enum fpga_mgr_states rc = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +
> +	response_msg = devm_kzalloc(priv->dev, AUTO_UPDATE_FEATURE_RESP_SIZE * sizeof(response_msg),
> +				    GFP_KERNEL);
> +	if (!response_msg)
> +		return FPGA_MGR_STATE_WRITE_INIT_ERR;
> +
> +	response = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_response), GFP_KERNEL);
> +	if (!response) {
> +		rc = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +		goto free_response_msg;
> +	}
> +
> +	message = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_msg), GFP_KERNEL);
> +	if (!message) {
> +		rc = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +		goto free_response;
> +	}
> +
> +	/*
> +	 * To verify that Auto Update is possible, the "Query Security Service

Why verify the possibility here, if Auto Update is not possible, the
Auto Update device should not be populated, is it?

> +	 * Request" is performed. Bit 5 of byte 1 is "UL_Auto Update" & if it is
> +	 * set, Auto Update is not possible.
> +	 * This service has no command data & does not overload mbox_offset.
> +	 * The size of the response varies between PolarFire & PolarFire SoC.
> +	 */
> +	response->resp_msg = response_msg;
> +	response->resp_size = AUTO_UPDATE_FEATURE_RESP_SIZE;
> +	message->cmd_opcode = AUTO_UPDATE_FEATURE_CMD_OPCODE;
> +	message->cmd_data_size = AUTO_UPDATE_FEATURE_CMD_DATA_SIZE;
> +	message->response = response;
> +	message->cmd_data = AUTO_UPDATE_FEATURE_CMD_DATA;
> +	message->mbox_offset = AUTO_UPDATE_DEFAULT_MBOX_OFFSET;
> +	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
> +
> +	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> +	if (ret | response->resp_status) {
> +		rc = FPGA_MGR_STATE_UNKNOWN;
> +		goto free_message;
> +	}
> +
> +	if (!(response_msg[1] & AUTO_UPDATE_FEATURE_ENABLED))
> +		rc = FPGA_MGR_STATE_OPERATING;
> +
> +free_message:
> +	devm_kfree(priv->dev, message);
> +free_response:
> +	devm_kfree(priv->dev, response);
> +free_response_msg:
> +	devm_kfree(priv->dev, response_msg);
> +
> +	return rc;
> +}
> +
> +static int mpfs_auto_update_write_init(struct fpga_manager *mgr, struct fpga_image_info *info,
> +				       const char *buf, size_t count)
> +{
> +	/*
> +	 * Verifying the Golden Image is idealistic. It will be evaluated
> +	 * against the currently programmed image and thus may fail - due to
> +	 * either rollback protection (if its an older version than that in use)
> +	 * or if the version is the same as that of the in-use image.
> +	 * Extracting the information as to why a failure occurred is not
> +	 * currently possible due to limitations of the system controller
> +	 * driver. If those are fixed, verification of the Golden Image should
> +	 * be added here.
> +	 */
> +	return 0;
> +}
> +
> +static int mpfs_auto_update_write(struct fpga_manager *mgr, const char *buf, size_t count)
> +{
> +	/*
> +	 * No parsing etc of the bitstream is required. The system controller
> +	 * will do all of that itself - including verifying that the bitstream
> +	 * is valid.
> +	 */
> +	struct mpfs_auto_update_priv *priv = mgr->priv;
> +	struct erase_info erase;
> +	char *buffer;
> +	loff_t directory_address = AUTO_UPDATE_UPGRADE_DIRECTORY;
> +	size_t bytes_written = 0, bytes_read = 0;
> +	size_t erase_size = AUTO_UPDATE_DIRECTORY_SIZE;
> +	size_t size_per_bitstream = 0;
> +	u32 image_address;
> +	int ret;
> +
> +	priv->flash = mpfs_sys_controller_get_flash(priv->sys_controller);
> +	if (!priv->flash)
> +		return -EIO;
> +
> +	erase_size = round_up(erase_size, (u64)priv->flash->erasesize);
> +
> +	/*
> +	 * We need to calculate if we have enough space in the flash for the
> +	 * new image.
> +	 * First, chop off the first 1 KiB as it's reserved for the directory.
> +	 * The 1 MiB reserved for design info needs to be ignored also.
> +	 * All that remains is carved into 3 & rounded down to the erasesize.
> +	 * If this is smaller than the image size, we abort.
> +	 * There's also no need to consume more than 20 MiB per image.
> +	 */
> +	size_per_bitstream = priv->flash->size - SZ_1K - SZ_1M;
> +	size_per_bitstream = round_down(size_per_bitstream / 3, erase_size);
> +	if (size_per_bitstream > 20 * SZ_1M)
> +		size_per_bitstream = 20 * SZ_1M;
> +
> +	if (size_per_bitstream < count) {
> +		dev_err(priv->dev,
> +			"flash device has insufficient capacity to store this bitstream\n");
> +		return -EINVAL;
> +	}
> +
> +	image_address = AUTO_UPDATE_BITSTREAM_BASE + AUTO_UPDATE_UPGRADE_INDEX * size_per_bitstream;
> +
> +	buffer = devm_kzalloc(priv->dev, erase_size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	erase.addr = AUTO_UPDATE_DIRECTORY_BASE;
> +	erase.len = erase_size;
> +
> +	/*
> +	 * We need to write the "SPI DIRECTORY" to the first 1 KiB, telling
> +	 * the system controller where to find the actual bitstream. Since
> +	 * this is spi-nor, we have to read the first eraseblock, erase that
> +	 * portion of the flash, modify the data and then write it back.
> +	 */
> +	ret = mtd_read(priv->flash, AUTO_UPDATE_DIRECTORY_BASE, erase_size, &bytes_read,
> +		       (u_char *)buffer);
> +	if (ret)
> +		goto out;
> +
> +	if (bytes_read != erase_size) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	ret = mtd_erase(priv->flash, &erase);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * Populate the image address and then zero out the next directory so
> +	 * that the system controller doesn't complain if in "Single Image"
> +	 * mode.
> +	 */
> +	memcpy(buffer + AUTO_UPDATE_UPGRADE_DIRECTORY, &image_address, AUTO_UPDATE_DIRECTORY_WIDTH);
> +	memset(buffer + AUTO_UPDATE_BLANK_DIRECTORY, 0x0, AUTO_UPDATE_DIRECTORY_WIDTH);

I'm wondering why the image address should be written for every
updating? Seems it is only related to the flash size, not related to
the to-be-programmed bitstream.

> +
> +	dev_info(priv->dev, "Writing the image address (%x) to the flash directory (%llx)\n",
> +		 image_address, directory_address);
> +
> +	ret = mtd_write(priv->flash, 0x0, erase_size, &bytes_written, (u_char *)buffer);
> +	if (ret)
> +		goto out;
> +
> +	if (bytes_written != erase_size) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Now the .spi image itself can be written to the flash. Preservation
> +	 * of contents here is not important here, unlike the spi "directory"
> +	 * which must be RMWed.
> +	 */
> +	erase.len = round_up(count, (size_t)priv->flash->erasesize);
> +	erase.addr = image_address;
> +
> +	dev_info(priv->dev, "Erasing the flash at address (%x)\n", image_address);
> +	ret = mtd_erase(priv->flash, &erase);
> +	if (ret)
> +		goto out;
> +
> +	dev_info(priv->dev, "Writing the image to the flash at address (%x)\n", image_address);
> +	ret = mtd_write(priv->flash, (loff_t)image_address, count, &bytes_written, buf);
> +	if (ret)
> +		goto out;
> +
> +	if (bytes_written != count)
> +		ret = -EIO;
> +
> +out:
> +	devm_kfree(priv->dev, buffer);
> +	return ret;
> +}
> +
> +static int mpfs_auto_update_write_complete(struct fpga_manager *mgr, struct fpga_image_info *info)
> +{
> +	struct mpfs_auto_update_priv *priv = mgr->priv;
> +	struct mpfs_mss_response *response;
> +	struct mpfs_mss_msg *message;
> +	u32 *response_msg;
> +	int ret;
> +
> +	response_msg = devm_kzalloc(priv->dev, AUTO_UPDATE_FEATURE_RESP_SIZE * sizeof(response_msg),
> +				    GFP_KERNEL);
> +	if (!response_msg)
> +		return -ENOMEM;
> +
> +	response = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_response), GFP_KERNEL);
> +	if (!response) {
> +		ret = -ENOMEM;
> +		goto free_response_msg;
> +	}
> +
> +	message = devm_kzalloc(priv->dev, sizeof(struct mpfs_mss_msg), GFP_KERNEL);
> +	if (!message) {
> +		ret = -ENOMEM;
> +		goto free_response;
> +	}
> +
> +	/*
> +	 * The system controller can verify that an image in the flash is valid.
> +	 * Rather than duplicate the check in this driver, call the relevant
> +	 * service from the system controller instead.
> +	 * This service has no command data and no response data. It overloads
> +	 * mbox_offset with the image index in the flash's SPI directory where
> +	 * the bitstream is located.
> +	 */
> +	response->resp_msg = response_msg;
> +	response->resp_size = AUTO_UPDATE_AUTHENTICATE_RESP_SIZE;
> +	message->cmd_opcode = AUTO_UPDATE_AUTHENTICATE_CMD_OPCODE;
> +	message->cmd_data_size = AUTO_UPDATE_AUTHENTICATE_CMD_DATA_SIZE;
> +	message->response = response;
> +	message->cmd_data = AUTO_UPDATE_AUTHENTICATE_CMD_DATA;
> +	message->mbox_offset = AUTO_UPDATE_UPGRADE_INDEX;
> +	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
> +
> +	dev_info(priv->dev, "Running verification of Upgrade Image\n");
> +	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> +	if (ret | response->resp_status) {
> +		dev_warn(priv->dev, "Verification of Upgrade Image failed!\n");
> +		ret = ret ? ret : -EBADMSG;

If verification failed, what happens to the written flash? Auto roll
back?

> +	}
> +
> +	dev_info(priv->dev, "Verification of Upgrade Image passed!\n");
> +//	/*
> +//	 * If the validation has passed, initiate Auto Update.
> +//	 * This service has no command data and no response data. It overloads
> +//	 * mbox_offset with the image index in the flash's SPI directory where
> +//	 * the bitstream is located.
> +//	 * Once we attempt Auto Update either:
> +//	 * - it passes and the board reboots
> +//	 * - it fails and the board reboots to recover
> +//	 * - the system controller aborts and we exit "gracefully".
> +//	 *   "gracefully" since there is no interrupt produced & it just times
> +//	 *   out.
> +//	 */
> +//	response->resp_msg = response_msg;
> +//	response->resp_size = AUTO_UPDATE_PROGRAM_RESP_SIZE;
> +//	message->cmd_opcode = AUTO_UPDATE_PROGRAM_CMD_OPCODE;
> +//	message->cmd_data_size = AUTO_UPDATE_PROGRAM_CMD_DATA_SIZE;
> +//	message->response = response;
> +//	message->cmd_data = AUTO_UPDATE_PROGRAM_CMD_DATA;
> +//	message->mbox_offset = 0; //field is ignored
> +//	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
> +//
> +//	dev_info(priv->dev, "Running Auto Update command\n");
> +//	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> +//	if (ret && ret != -ETIMEDOUT)
> +//		goto out;
> +//
> +//	/* *remove this for auto update*
> +//	 * This return 0 is dead code. Either the Auto Update will fail, or it will pass
> +//	 * & the FPGA will be rebooted in which case mpfs_blocking_transaction()
> +//	 * will never return and Linux will die.
> +//	 */
> +//	return 0;

Why comment out this code block?

Thanks,
Yilun

> +
> +	devm_kfree(priv->dev, message);
> +free_response:
> +	devm_kfree(priv->dev, response);
> +free_response_msg:
> +	devm_kfree(priv->dev, response_msg);
> +
> +	return ret;
> +}
> +
> +static const struct fpga_manager_ops mpfs_auto_update_ops = {
> +	.state = mpfs_auto_update_state,
> +	.write_init = mpfs_auto_update_write_init,
> +	.write = mpfs_auto_update_write,
> +	.write_complete = mpfs_auto_update_write_complete,
> +};
> +
> +static int mpfs_auto_update_run(struct device *dev)
> +{
> +	struct fpga_manager *mgr;
> +	struct fpga_image_info *info;
> +	int ret;
> +
> +	mgr = fpga_mgr_get(dev);
> +	info = fpga_image_info_alloc(dev);
> +
> +	info->firmware_name = devm_kstrdup(dev, "mpfs_bitstream.spi", GFP_KERNEL);
> +
> +	ret = fpga_mgr_lock(mgr);
> +	if (ret)
> +		goto free_info;
> +
> +	ret = fpga_mgr_load(mgr, info);
> +	if (ret) {
> +		dev_err(dev, "Failed to write the bitstream\n");
> +		goto unlock_mgr;
> +	}
> +
> +unlock_mgr:
> +	fpga_mgr_unlock(mgr);
> +free_info:
> +	fpga_image_info_free(info);
> +	fpga_mgr_put(mgr);
> +
> +	return ret;
> +}
> +
> +static ssize_t mpfs_auto_update_exec(struct file *file, const char __user *data, size_t count,
> +				     loff_t *ppos)
> +{
> +	int ret;
> +
> +	ret = mpfs_auto_update_run(mpfs_auto_update_debug_dev);
> +	if (ret)
> +		dev_err_probe(mpfs_auto_update_debug_dev, ret, "Auto Update failed");
> +
> +	return count;
> +}
> +
> +static const struct file_operations mpfs_auto_update_fops = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.write = mpfs_auto_update_exec
> +};
> +
> +static int mpfs_auto_update_debugfs_setup(struct mpfs_auto_update_priv *priv)
> +{
> +	priv->debugfs_dir = debugfs_create_dir("fpga", NULL);
> +
> +	if(IS_ERR(priv->debugfs_dir))
> +		return PTR_ERR(priv->debugfs_dir);
> +
> +	debugfs_create_file("microchip_exec_update", 0200, priv->debugfs_dir, NULL,
> +			    &mpfs_auto_update_fops);
> +
> +	return 0;
> +}
> +
> +static int mpfs_auto_update_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mpfs_auto_update_priv *priv;
> +	struct fpga_manager *mgr;
> +	enum fpga_mgr_states state;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->sys_controller = mpfs_sys_controller_get(dev);
> +	if (IS_ERR(priv->sys_controller))
> +		return dev_err_probe(dev, PTR_ERR(priv->sys_controller),
> +				     "Could not register as a sub device of the system controller\n");
> +
> +	priv->dev = dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	mgr = devm_fpga_mgr_register(dev, "Microchip MPFS Auto Update FPGA Manager",
> +				     &mpfs_auto_update_ops, priv);
> +	if (IS_ERR(mgr))
> +		return dev_err_probe(dev, PTR_ERR(mgr), "Could not register FPGA manager.\n");
> +
> +	state = mpfs_auto_update_state(mgr);
> +	if (state != FPGA_MGR_STATE_OPERATING)
> +		return -EIO;
> +
> +	ret = mpfs_auto_update_debugfs_setup(priv);
> +	if (ret && ret != -ENODEV)
> +		return ret;
> +
> +	mpfs_auto_update_debug_dev = priv->dev;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mpfs_auto_update_driver = {
> +	.driver = {
> +		.name = "mpfs-auto-update",
> +	},
> +	.probe = mpfs_auto_update_probe,
> +};
> +module_platform_driver(mpfs_auto_update_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
> +MODULE_DESCRIPTION("PolarFire SoC Auto Update FPGA reprogramming");
> -- 
> 2.39.1
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 5/6] fpga: add PolarFire SoC Auto Update support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 4239 bytes --]

On Sun, Mar 05, 2023 at 12:38:00AM +0800, Xu Yilun wrote:
> On 2023-02-17 at 16:40:22 +0000, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Add support for Auto Update reprogramming of the FPGA fabric on
> > PolarFire SoC.
> > 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  drivers/fpga/Kconfig                 |   9 +
> >  drivers/fpga/Makefile                |   1 +
> >  drivers/fpga/microchip-auto-update.c | 495 +++++++++++++++++++++++++++
> >  3 files changed, 505 insertions(+)
> >  create mode 100644 drivers/fpga/microchip-auto-update.c
> > +	/*
> > +	 * To verify that Auto Update is possible, the "Query Security Service

> Why verify the possibility here, if Auto Update is not possible, the
> Auto Update device should not be populated, is it?

Good point, I'll check this in probe instead.

> > +	/*
> > +	 * Populate the image address and then zero out the next directory so
> > +	 * that the system controller doesn't complain if in "Single Image"
> > +	 * mode.
> > +	 */
> > +	memcpy(buffer + AUTO_UPDATE_UPGRADE_DIRECTORY, &image_address, AUTO_UPDATE_DIRECTORY_WIDTH);
> > +	memset(buffer + AUTO_UPDATE_BLANK_DIRECTORY, 0x0, AUTO_UPDATE_DIRECTORY_WIDTH);
> 
> I'm wondering why the image address should be written for every
> updating? Seems it is only related to the flash size, not related to
> the to-be-programmed bitstream.

Yah, it doesn't need to be. I'll check it against the expected value &
only write it if needed.

> > +	dev_info(priv->dev, "Running verification of Upgrade Image\n");
> > +	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> > +	if (ret | response->resp_status) {
> > +		dev_warn(priv->dev, "Verification of Upgrade Image failed!\n");
> > +		ret = ret ? ret : -EBADMSG;
> 
> If verification failed, what happens to the written flash? Auto roll
> back?

Nope, that should be left up to userspace to decide what to do. I've got
some improvement to do to the mailbox driver that sits behind
mpfs_blocking_transaction() that I thought was not allowed by the
mailbox framework, so should be able to report better errors for this in
the future.

> > +	}
> > +
> > +	dev_info(priv->dev, "Verification of Upgrade Image passed!\n");
> > +//	/*
> > +//	 * If the validation has passed, initiate Auto Update.
> > +//	 * This service has no command data and no response data. It overloads
> > +//	 * mbox_offset with the image index in the flash's SPI directory where
> > +//	 * the bitstream is located.
> > +//	 * Once we attempt Auto Update either:
> > +//	 * - it passes and the board reboots
> > +//	 * - it fails and the board reboots to recover
> > +//	 * - the system controller aborts and we exit "gracefully".
> > +//	 *   "gracefully" since there is no interrupt produced & it just times
> > +//	 *   out.
> > +//	 */
> > +//	response->resp_msg = response_msg;
> > +//	response->resp_size = AUTO_UPDATE_PROGRAM_RESP_SIZE;
> > +//	message->cmd_opcode = AUTO_UPDATE_PROGRAM_CMD_OPCODE;
> > +//	message->cmd_data_size = AUTO_UPDATE_PROGRAM_CMD_DATA_SIZE;
> > +//	message->response = response;
> > +//	message->cmd_data = AUTO_UPDATE_PROGRAM_CMD_DATA;
> > +//	message->mbox_offset = 0; //field is ignored
> > +//	message->resp_offset = AUTO_UPDATE_DEFAULT_RESP_OFFSET;
> > +//
> > +//	dev_info(priv->dev, "Running Auto Update command\n");
> > +//	ret = mpfs_blocking_transaction(priv->sys_controller, message);
> > +//	if (ret && ret != -ETIMEDOUT)
> > +//		goto out;
> > +//
> > +//	/* *remove this for auto update*
> > +//	 * This return 0 is dead code. Either the Auto Update will fail, or it will pass
> > +//	 * & the FPGA will be rebooted in which case mpfs_blocking_transaction()
> > +//	 * will never return and Linux will die.
> > +//	 */
> > +//	return 0;
> 
> Why comment out this code block?

It was meant to be removed & must have snuck back in a rebase. This is my
test code that initiates the update from Linux, rather than at reboot.

I'm going to take a look at Russ' driver before I submit another version
of this (and the underlying mailbox stuff also needs changes).

Thanks for taking a look,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 6/6] riscv: dts: microchip: add the mpfs' system controller qspi & associated flash

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The system controller's flash can be accessed via an MSS-exposed QSPI
controller sitting, which sits between the mailbox's control & data
registers. On Icicle, it has an MT25QL01GBBB8ESF connected to it.

The system controller and MSS both have separate QSPI controllers, both
of which can access the flash, although the system controller takes
priority.
Unfortunately, on engineering sample silicon, such as that on Icicle
kits, the MSS' QSPI controller cannot write to the flash due to a bug.
As a workaround, a QSPI controller can be implemented in the FPGA
fabric and the IO routing modified to connect it to the flash in place
of the "hard" controller in the MSS.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../boot/dts/microchip/mpfs-icicle-kit.dts    | 21 ++++++++++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       | 24 ++++++++++++++-----
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
index 90b261114763..2dae3f8f33f6 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
@@ -199,6 +199,27 @@ &syscontroller {
 	status = "okay";
 };
 
+&syscontroller_qspi {
+	/*
+	 * The flash *is* there, but Icicle kits that have engineering sample
+	 * silicon (write?) access to this flash to non-functional. The system
+	 * controller itself can actually access it, but the MSS cannot write
+	 * an image there. Instantiating a coreQSPI in the fabric & connecting
+	 * it to the flash instead should work though. Pre-production or later
+	 * silicon does not have this issue.
+	 */
+	status = "disabled";
+
+	sys_ctrl_flash: flash@0 { // MT25QL01GBBB8ESF-0SIT
+		compatible = "jedec,spi-nor";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		spi-max-frequency = <20000000>;
+		spi-rx-bus-width = <1>;
+		reg = <0>;
+	};
+};
+
 &usb {
 	status = "okay";
 	dr_mode = "host";
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 0a9bb84af438..568da2b570c0 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -173,11 +173,6 @@ refclk: mssrefclk {
 		#clock-cells = <0>;
 	};
 
-	syscontroller: syscontroller {
-		compatible = "microchip,mpfs-sys-controller";
-		mboxes = <&mbox 0>;
-	};
-
 	soc {
 		#address-cells = <2>;
 		#size-cells = <2>;
@@ -498,11 +493,28 @@ usb: usb@20201000 {
 
 		mbox: mailbox@37020000 {
 			compatible = "microchip,mpfs-mailbox";
-			reg = <0x0 0x37020000 0x0 0x1000>, <0x0 0x2000318C 0x0 0x40>;
+			reg = <0x0 0x37020000 0x0 0x58>, <0x0 0x2000318C 0x0 0x40>,
+			      <0x0 0x37020800 0x0 0x100>;
 			interrupt-parent = <&plic>;
 			interrupts = <96>;
 			#mbox-cells = <1>;
 			status = "disabled";
 		};
+
+		syscontroller_qspi: spi@37020100 {
+			compatible = "microchip,mpfs-qspi", "microchip,coreqspi-rtl-v2";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x37020100 0x0 0x100>;
+			interrupt-parent = <&plic>;
+			interrupts = <110>;
+			clocks = <&clkcfg CLK_QSPI>; /* this is probably wrong, consult the docs! */
+			status = "disabled";
+		};
+	};
+
+	syscontroller: syscontroller {
+		compatible = "microchip,mpfs-sys-controller";
+		mboxes = <&mbox 0>;
 	};
 };
-- 
2.39.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Xu Yilun]

On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Hey all,
> 
> This patchset adds support for the "Auto Update" feature on PolarFire
> SoC that allows for writing an FPGA bistream to the SPI flash connected
> to the system controller.

I haven't fully checked the patches yet, just some quick comments:

Since this feature is just to R/W the flash, and would not affect the
runtime FPGA region, I don't think an FPGA manager is actually needed.
Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
MTD tool if I remember correctly.

Thanks,
Yilun

> On powercycle (or reboot depending on how the firmware implements the
> openSBI SRST extension) "Auto Update" will take place, and program the
> FPGA with the contents of the SPI flash - provided that that image is
> valid and an actual upgrade from that already programmed!
> 
> Unfortunately, this series is not really testable yet - the Engineering
> Sample silicon on most dev boards has a bug in the QSPI controller
> connected to the system controller's flash and cannot access it.
> Pre-production and later silicon has this bug fixed.
> 
> I previously posted an RFC about my approach in this driver, since as a
> flash-based FPGA we are somewhat different to the existing
> self-reprogramming drivers here. That RFC is here:
> https://lore.kernel.org/linux-fpga/20221121225748.124900-1-conor@kernel.org/
> 
> This series depends on the following fixes:
> https://patchwork.kernel.org/project/linux-riscv/list/?series=714160
> 
> The patch adding the driver depends on the soc patches earlier in the
> series, so taking both through the same tree makes sense. Depending on
> sequencing with the dependencies, me taking it through the soc tree
> (with Acks etc of course) may make the most sense.
> 
> The other caveat here I guess is that this uses debugfs to trigger the
> write, as we do not yet have a userspace for this yet!
> 
> Cheers,
> Conor.
> 
> CC: Conor Dooley <conor.dooley@microchip.com>
> CC: Daire McNamara <daire.mcnamara@microchip.com>
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> CC: Moritz Fischer <mdf@kernel.org>
> CC: Wu Hao <hao.wu@intel.com>
> CC: Xu Yilun <yilun.xu@intel.com>
> CC: Tom Rix <trix@redhat.com>
> CC: linux-riscv@lists.infradead.org
> CC: devicetree@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: linux-fpga@vger.kernel.org
> 
> Conor Dooley (6):
>   soc: microchip: mpfs: add a prefix to rx_callback()
>   dt-bindings: soc: microchip: add a property for system controller
>     flash
>   soc: microchip: mpfs: enable access to the system controller's flash
>   soc: microchip: mpfs: add auto-update subdev to system controller
>   fpga: add PolarFire SoC Auto Update support
>   riscv: dts: microchip: add the mpfs' system controller qspi &
>     associated flash
> 
>  .../microchip,mpfs-sys-controller.yaml        |  10 +
>  .../boot/dts/microchip/mpfs-icicle-kit.dts    |  21 +
>  arch/riscv/boot/dts/microchip/mpfs.dtsi       |  24 +-
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   1 +
>  drivers/fpga/microchip-auto-update.c          | 495 ++++++++++++++++++
>  drivers/soc/microchip/Kconfig                 |   1 +
>  drivers/soc/microchip/mpfs-sys-controller.c   |  33 +-
>  include/soc/microchip/mpfs.h                  |   2 +
>  9 files changed, 586 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/fpga/microchip-auto-update.c
> 
> -- 
> 2.39.1
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1384 bytes --]

On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:

> > This patchset adds support for the "Auto Update" feature on PolarFire
> > SoC that allows for writing an FPGA bistream to the SPI flash connected
> > to the system controller.
> 
> I haven't fully checked the patches yet, just some quick comments:
> 
> Since this feature is just to R/W the flash, and would not affect the
> runtime FPGA region, I don't think an FPGA manager is actually needed.
> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> MTD tool if I remember correctly.

A lack of interest in opening up the system controller to userspace!
You're right in that the writing of the image can be done that way, and
while I was testing I used the userspace bits of mtd along the way - but
for validating that the image we are writing we rely on the system
controller.
I'm really not interested in exposing the system controller's
functionality, especially the bitstream manipulation parts, to userspace
due to the risk of input validation bugs, so at least that side of
things should remain in the kernel.
I suppose I could implement something custom in drivers/soc that does
the validation only, and push the rest out to userspace. Just seemed
fitting to do the whole lot in drivers/fpga.

Cheers,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Russ Weight]

Hi Conor,

On 2/24/23 00:28, Conor Dooley wrote:
> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>> to the system controller.
>> I haven't fully checked the patches yet, just some quick comments:
>>
>> Since this feature is just to R/W the flash, and would not affect the
>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>> MTD tool if I remember correctly.
> A lack of interest in opening up the system controller to userspace!
> You're right in that the writing of the image can be done that way, and
> while I was testing I used the userspace bits of mtd along the way - but
> for validating that the image we are writing we rely on the system
> controller.
> I'm really not interested in exposing the system controller's
> functionality, especially the bitstream manipulation parts, to userspace
> due to the risk of input validation bugs, so at least that side of
> things should remain in the kernel.
> I suppose I could implement something custom in drivers/soc that does
> the validation only, and push the rest out to userspace. Just seemed
> fitting to do the whole lot in drivers/fpga.
Conor,

In case you haven't already looked at the new firmware-upload
support in the firmware-loader, I'll give you some references
here to see if it fit you needs. This would only support the
write (not the read) but it would allow the controller to do
validation on the write.

The is the cover letter which shows a usage example:
https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/

And this is a pointer to the latest documentation for it:
https://docs.kernel.org/driver-api/firmware/fw_upload.html

The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c

- Russ

>
> Cheers,
> Conor.
>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2807 bytes --]

Hey Russ,

On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
> On 2/24/23 00:28, Conor Dooley wrote:
> > On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> >> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> >>> This patchset adds support for the "Auto Update" feature on PolarFire
> >>> SoC that allows for writing an FPGA bistream to the SPI flash connected
> >>> to the system controller.
> >> I haven't fully checked the patches yet, just some quick comments:
> >>
> >> Since this feature is just to R/W the flash, and would not affect the
> >> runtime FPGA region, I don't think an FPGA manager is actually needed.
> >> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> >> MTD tool if I remember correctly.
> > A lack of interest in opening up the system controller to userspace!
> > You're right in that the writing of the image can be done that way, and
> > while I was testing I used the userspace bits of mtd along the way - but
> > for validating that the image we are writing we rely on the system
> > controller.
> > I'm really not interested in exposing the system controller's
> > functionality, especially the bitstream manipulation parts, to userspace
> > due to the risk of input validation bugs, so at least that side of
> > things should remain in the kernel.
> > I suppose I could implement something custom in drivers/soc that does
> > the validation only, and push the rest out to userspace. Just seemed
> > fitting to do the whole lot in drivers/fpga.

> In case you haven't already looked at the new firmware-upload
> support in the firmware-loader, I'll give you some references
> here to see if it fit you needs. This would only support the
> write (not the read) but it would allow the controller to do
> validation on the write.
> 
> The is the cover letter which shows a usage example:
> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
> 
> And this is a pointer to the latest documentation for it:
> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> 
> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c

Sounds interesting, I shall go and take a look. Just quickly, when you
say it wouldn't support the read, what exactly do you mean?
The only read that I am really interested in doing is reading the first
1K of flash as I need to RMW it, but I don't think that that's what you
mean.
Do you mean that I would not be able to dump the firmware using your
fw_upload interface? If so, that's an acceptable constraint IMO.

Your interface also circumvents us (Microchip) defining yet another
method for interacting with the FPGA, since from my nosing around,
everyone seems to do something different.

Cheers,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Russ Weight]

On 2/27/23 14:16, Conor Dooley wrote:
> Hey Russ,
>
> On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
>> On 2/24/23 00:28, Conor Dooley wrote:
>>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>>>> to the system controller.
>>>> I haven't fully checked the patches yet, just some quick comments:
>>>>
>>>> Since this feature is just to R/W the flash, and would not affect the
>>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>>>> MTD tool if I remember correctly.
>>> A lack of interest in opening up the system controller to userspace!
>>> You're right in that the writing of the image can be done that way, and
>>> while I was testing I used the userspace bits of mtd along the way - but
>>> for validating that the image we are writing we rely on the system
>>> controller.
>>> I'm really not interested in exposing the system controller's
>>> functionality, especially the bitstream manipulation parts, to userspace
>>> due to the risk of input validation bugs, so at least that side of
>>> things should remain in the kernel.
>>> I suppose I could implement something custom in drivers/soc that does
>>> the validation only, and push the rest out to userspace. Just seemed
>>> fitting to do the whole lot in drivers/fpga.
>> In case you haven't already looked at the new firmware-upload
>> support in the firmware-loader, I'll give you some references
>> here to see if it fit you needs. This would only support the
>> write (not the read) but it would allow the controller to do
>> validation on the write.
>>
>> The is the cover letter which shows a usage example:
>> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
>>
>> And this is a pointer to the latest documentation for it:
>> https://docs.kernel.org/driver-api/firmware/fw_upload.html
>>
>> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> Sounds interesting, I shall go and take a look. Just quickly, when you
> say it wouldn't support the read, what exactly do you mean?
> The only read that I am really interested in doing is reading the first
> 1K of flash as I need to RMW it, but I don't think that that's what you
> mean.
> Do you mean that I would not be able to dump the firmware using your
> fw_upload interface? If so, that's an acceptable constraint IMO.

Yes - I mean that you couldn't dump the firmware image from userspace
using the fw_upload interface. The sysfs interface allows you to read
and write a temporary buffer, but once you "echo 0 > loading", there
is no sysfs interface associated with the firmware-loader that would
allow you to read the image from flash. Your controller actually does
the writes, so RMW in that context is fine.

- Russ
>
> Your interface also circumvents us (Microchip) defining yet another
> method for interacting with the FPGA, since from my nosing around,
> everyone seems to do something different.
>
> Cheers,
> Conor.
>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3467 bytes --]

On Mon, Feb 27, 2023 at 02:42:30PM -0800, Russ Weight wrote:
> 
> 
> On 2/27/23 14:16, Conor Dooley wrote:
> > Hey Russ,
> >
> > On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
> >> On 2/24/23 00:28, Conor Dooley wrote:
> >>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> >>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> >>>>> This patchset adds support for the "Auto Update" feature on PolarFire
> >>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
> >>>>> to the system controller.
> >>>> I haven't fully checked the patches yet, just some quick comments:
> >>>>
> >>>> Since this feature is just to R/W the flash, and would not affect the
> >>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
> >>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> >>>> MTD tool if I remember correctly.
> >>> A lack of interest in opening up the system controller to userspace!
> >>> You're right in that the writing of the image can be done that way, and
> >>> while I was testing I used the userspace bits of mtd along the way - but
> >>> for validating that the image we are writing we rely on the system
> >>> controller.
> >>> I'm really not interested in exposing the system controller's
> >>> functionality, especially the bitstream manipulation parts, to userspace
> >>> due to the risk of input validation bugs, so at least that side of
> >>> things should remain in the kernel.
> >>> I suppose I could implement something custom in drivers/soc that does
> >>> the validation only, and push the rest out to userspace. Just seemed
> >>> fitting to do the whole lot in drivers/fpga.
> >> In case you haven't already looked at the new firmware-upload
> >> support in the firmware-loader, I'll give you some references
> >> here to see if it fit you needs. This would only support the
> >> write (not the read) but it would allow the controller to do
> >> validation on the write.
> >>
> >> The is the cover letter which shows a usage example:
> >> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
> >>
> >> And this is a pointer to the latest documentation for it:
> >> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> >>
> >> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> > Sounds interesting, I shall go and take a look. Just quickly, when you
> > say it wouldn't support the read, what exactly do you mean?
> > The only read that I am really interested in doing is reading the first
> > 1K of flash as I need to RMW it, but I don't think that that's what you
> > mean.
> > Do you mean that I would not be able to dump the firmware using your
> > fw_upload interface? If so, that's an acceptable constraint IMO.
> 
> Yes - I mean that you couldn't dump the firmware image from userspace
> using the fw_upload interface. The sysfs interface allows you to read
> and write a temporary buffer, but once you "echo 0 > loading", there
> is no sysfs interface associated with the firmware-loader that would
> allow you to read the image from flash. Your controller actually does
> the writes, so RMW in that context is fine.

Ahh cool. I don't really care about dumping the firmware via such a
mechanism, so that sounds good to me.
I'll check out your approach, the immediate thought is that it sounds
suitable to my use case, so thanks!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 4420 bytes --]

Hey Russ,

On Mon, Feb 27, 2023 at 10:56:07PM +0000, Conor Dooley wrote:
> On Mon, Feb 27, 2023 at 02:42:30PM -0800, Russ Weight wrote:
> > On 2/27/23 14:16, Conor Dooley wrote:
> > > On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
> > >> On 2/24/23 00:28, Conor Dooley wrote:
> > >>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
> > >>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
> > >>>>> This patchset adds support for the "Auto Update" feature on PolarFire
> > >>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
> > >>>>> to the system controller.
> > >>>> I haven't fully checked the patches yet, just some quick comments:
> > >>>>
> > >>>> Since this feature is just to R/W the flash, and would not affect the
> > >>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
> > >>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
> > >>>> MTD tool if I remember correctly.
> > >>> A lack of interest in opening up the system controller to userspace!
> > >>> You're right in that the writing of the image can be done that way, and
> > >>> while I was testing I used the userspace bits of mtd along the way - but
> > >>> for validating that the image we are writing we rely on the system
> > >>> controller.
> > >>> I'm really not interested in exposing the system controller's
> > >>> functionality, especially the bitstream manipulation parts, to userspace
> > >>> due to the risk of input validation bugs, so at least that side of
> > >>> things should remain in the kernel.
> > >>> I suppose I could implement something custom in drivers/soc that does
> > >>> the validation only, and push the rest out to userspace. Just seemed
> > >>> fitting to do the whole lot in drivers/fpga.
> > >> In case you haven't already looked at the new firmware-upload
> > >> support in the firmware-loader, I'll give you some references
> > >> here to see if it fit you needs. This would only support the
> > >> write (not the read) but it would allow the controller to do
> > >> validation on the write.
> > >>
> > >> The is the cover letter which shows a usage example:
> > >> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
> > >>
> > >> And this is a pointer to the latest documentation for it:
> > >> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> > >>
> > >> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> > > Sounds interesting, I shall go and take a look. Just quickly, when you
> > > say it wouldn't support the read, what exactly do you mean?
> > > The only read that I am really interested in doing is reading the first
> > > 1K of flash as I need to RMW it, but I don't think that that's what you
> > > mean.
> > > Do you mean that I would not be able to dump the firmware using your
> > > fw_upload interface? If so, that's an acceptable constraint IMO.
> > 
> > Yes - I mean that you couldn't dump the firmware image from userspace
> > using the fw_upload interface. The sysfs interface allows you to read
> > and write a temporary buffer, but once you "echo 0 > loading", there
> > is no sysfs interface associated with the firmware-loader that would
> > allow you to read the image from flash. Your controller actually does
> > the writes, so RMW in that context is fine.
> 
> Ahh cool. I don't really care about dumping the firmware via such a
> mechanism, so that sounds good to me.
> I'll check out your approach, the immediate thought is that it sounds
> suitable to my use case, so thanks!

Taken me a while to get around to it, but thanks for your suggestion.
Looks like it is suitable for what I am trying to do, so in the middle
of working on another version of this using fw_upload.
The enum return codes from write are a little clumsy, but oh well, could
be worse I suppose.

Just one thing I noted (although I rarely pay much attention to/rely on
the driver-api docs when recent drivers exist as usable examples) is
that the docs for this stuff is a wee bit out of date due to some API
changes.
In the code example in this document:
https://docs.kernel.org/driver-api/firmware/fw_upload.html
firmware_upload_register() has fewer arguments than it does when you
look at the signature of the function in the documentation right below
it.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support

[Russ Weight]

Hi Conor,

Sorry for the slow reply - I was out of the office for a week...

On 3/22/23 11:51, Conor Dooley wrote:
> Hey Russ,
>
> On Mon, Feb 27, 2023 at 10:56:07PM +0000, Conor Dooley wrote:
>> On Mon, Feb 27, 2023 at 02:42:30PM -0800, Russ Weight wrote:
>>> On 2/27/23 14:16, Conor Dooley wrote:
>>>> On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
>>>>> On 2/24/23 00:28, Conor Dooley wrote:
>>>>>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>>>>>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>>>>>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>>>>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>>>>>>> to the system controller.
>>>>>>> I haven't fully checked the patches yet, just some quick comments:
>>>>>>>
>>>>>>> Since this feature is just to R/W the flash, and would not affect the
>>>>>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>>>>>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>>>>>>> MTD tool if I remember correctly.
>>>>>> A lack of interest in opening up the system controller to userspace!
>>>>>> You're right in that the writing of the image can be done that way, and
>>>>>> while I was testing I used the userspace bits of mtd along the way - but
>>>>>> for validating that the image we are writing we rely on the system
>>>>>> controller.
>>>>>> I'm really not interested in exposing the system controller's
>>>>>> functionality, especially the bitstream manipulation parts, to userspace
>>>>>> due to the risk of input validation bugs, so at least that side of
>>>>>> things should remain in the kernel.
>>>>>> I suppose I could implement something custom in drivers/soc that does
>>>>>> the validation only, and push the rest out to userspace. Just seemed
>>>>>> fitting to do the whole lot in drivers/fpga.
>>>>> In case you haven't already looked at the new firmware-upload
>>>>> support in the firmware-loader, I'll give you some references
>>>>> here to see if it fit you needs. This would only support the
>>>>> write (not the read) but it would allow the controller to do
>>>>> validation on the write.
>>>>>
>>>>> The is the cover letter which shows a usage example:
>>>>> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
>>>>>
>>>>> And this is a pointer to the latest documentation for it:
>>>>> https://docs.kernel.org/driver-api/firmware/fw_upload.html
>>>>>
>>>>> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
>>>> Sounds interesting, I shall go and take a look. Just quickly, when you
>>>> say it wouldn't support the read, what exactly do you mean?
>>>> The only read that I am really interested in doing is reading the first
>>>> 1K of flash as I need to RMW it, but I don't think that that's what you
>>>> mean.
>>>> Do you mean that I would not be able to dump the firmware using your
>>>> fw_upload interface? If so, that's an acceptable constraint IMO.
>>> Yes - I mean that you couldn't dump the firmware image from userspace
>>> using the fw_upload interface. The sysfs interface allows you to read
>>> and write a temporary buffer, but once you "echo 0 > loading", there
>>> is no sysfs interface associated with the firmware-loader that would
>>> allow you to read the image from flash. Your controller actually does
>>> the writes, so RMW in that context is fine.
>> Ahh cool. I don't really care about dumping the firmware via such a
>> mechanism, so that sounds good to me.
>> I'll check out your approach, the immediate thought is that it sounds
>> suitable to my use case, so thanks!
> Taken me a while to get around to it, but thanks for your suggestion.
> Looks like it is suitable for what I am trying to do, so in the middle
> of working on another version of this using fw_upload.
> The enum return codes from write are a little clumsy, but oh well, could
> be worse I suppose.
>
> Just one thing I noted (although I rarely pay much attention to/rely on
> the driver-api docs when recent drivers exist as usable examples) is
> that the docs for this stuff is a wee bit out of date due to some API
> changes.
> In the code example in this document:
> https://docs.kernel.org/driver-api/firmware/fw_upload.html
> firmware_upload_register() has fewer arguments than it does when you
> look at the signature of the function in the documentation right below
> it.

I saw your patch to fix this. Thanks!

- Russ
>
> Cheers,
> Conor.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv