[PATCHv6 0/3] GE Healthcare PPD firmware upgrade driver for ACHC

[PATCHv6 0/3] GE Healthcare PPD firmware upgrade driver for ACHC

[Sebastian Reichel]

Hi,

The PPD has a secondary processor (NXP Kinetis K20), which can be
programmed from the main system. It is connected to the main processor
by having it's EzPort interface connected to the SPI bus. Currently
both (normal and EzPort) interfaces are simply exposed to userspace.
This does not work for the EzPort, since EzPort usage requires a device
reset. The proper solution is to do the flashing from kernel space
with properly timed toggling of EzPort chip-select and reset line. In
PATCHv2 it was suggested, that this should happen via an SPI ancillary
device, so this is how it has been implemented now. The SPI core
changes have been applied in PATCHv5 and are part of v5.14-rc1.

Changes since PATCHv5:
 * https://lore.kernel.org/lkml/20210621175359.126729-1-sebastian.reichel@collabora.com/
 * Rebased to v5.14-rc1
 * Fixed compilation as module
 * Dropped no longer needed module.h include from ezport code

Changes since PATCHv4:
 * https://lore.kernel.org/lkml/20210609151235.48964-1-sebastian.reichel@collabora.com/
 * Add Rob's Acked-by to ge-achc binding update
 * Don't use of_property_read_u32_index() in of_spi_parse_dt()
 * Don't build separate module for EzPort code
 * Use GPL2-only for the header
 * ACHC_MAX_FREQ -> ACHC_MAX_FREQ_HZ
 * Only accept '1' for the sysfs files, not any data
 * Update sysfs file documentation
 * Rebased to spi-next tree (b8f9dce0f4eb)

Changes since PATCHv3:
 * https://lore.kernel.org/lkml/20210528113346.37137-1-sebastian.reichel@collabora.com/
 * Add Rob's Acked-by to 2nd patch
 * use GPL-2-only instead of GPL-2+
 * use %zu for printing a size_t
 * use driver's .dev_groups to register sysfs group
 * Add sysfs property documentation
 * split EzPort and ACHC drivers into separate patches
 * drop minItems/maxItems from achc binding, which seems to fix the problems
   reported by dt_binding_check. The information of two items being required
   is implied by the explicit item list.
 * drop spidev functionality for the main SPI interface. The current firmware
   communicates via UART and adding spidev support is complex. If future firmware
   releases start using it, spidev support for the main interface can be added
   later.

Changes since PATCHv2:
 * https://lore.kernel.org/lkml/20180327135259.30890-1-sebastian.reichel@collabora.co.uk/
 * add SPI core support for ancillary devices
 * modify ACHC binding to make use of ancillary device
 * rewrite driver to use ancillary device
 * rebased to 5.13-rc1

Changes since PATCHv1:
 * https://lore.kernel.org/lkml/20180320172201.2065-1-sebastian.reichel@collabora.co.uk/
 * split DT binding update into its own patch
 * add sysfs attribute documentation
 * fix problem reported by kbuild test robot

-- Sebastian

Sebastian Reichel (3):
  dt-bindings: misc: ge-achc: Convert to DT schema format
  ARM: dts: imx53-ppd: Fix ACHC entry
  misc: gehc-achc: new driver

 .../ABI/testing/sysfs-driver-ge-achc          |  14 +
 .../devicetree/bindings/misc/ge-achc.txt      |  26 -
 .../devicetree/bindings/misc/ge-achc.yaml     |  65 +++
 arch/arm/boot/dts/imx53-ppd.dts               |  23 +-
 drivers/misc/Kconfig                          |  11 +
 drivers/misc/Makefile                         |   2 +
 drivers/misc/gehc-achc.c                      | 136 +++++
 drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
 drivers/misc/nxp-ezport.h                     |   9 +
 drivers/spi/spidev.c                          |   1 -
 10 files changed, 718 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
 delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
 create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml
 create mode 100644 drivers/misc/gehc-achc.c
 create mode 100644 drivers/misc/nxp-ezport.c
 create mode 100644 drivers/misc/nxp-ezport.h

-- 
2.30.2

[PATCHv6 1/3] dt-bindings: misc: ge-achc: Convert to DT schema format

[Sebastian Reichel]

Convert the binding to DT schema format. Also update the binding
to fix shortcomings

 * Add "nxp,kinetis-k20" fallback compatible
 * add programming SPI interface and reset GPIO
 * add main clock
 * add voltage supplies
 * drop spi-max-frequency from required properties,
   driver will setup max. frequency

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/misc/ge-achc.txt      | 26 --------
 .../devicetree/bindings/misc/ge-achc.yaml     | 65 +++++++++++++++++++
 2 files changed, 65 insertions(+), 26 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
 create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml

diff --git a/Documentation/devicetree/bindings/misc/ge-achc.txt b/Documentation/devicetree/bindings/misc/ge-achc.txt
deleted file mode 100644
index 77df94d7a32f..000000000000
--- a/Documentation/devicetree/bindings/misc/ge-achc.txt
+++ /dev/null
@@ -1,26 +0,0 @@
-* GE Healthcare USB Management Controller
-
-A device which handles data aquisition from compatible USB based peripherals.
-SPI is used for device management.
-
-Note: This device does not expose the peripherals as USB devices.
-
-Required properties:
-
-- compatible : Should be "ge,achc"
-
-Required SPI properties:
-
-- reg : Should be address of the device chip select within
-  the controller.
-
-- spi-max-frequency : Maximum SPI clocking speed of device in Hz, should be
-  1MHz for the GE ACHC.
-
-Example:
-
-spidev0: spi@0 {
-	compatible = "ge,achc";
-	reg = <0>;
-	spi-max-frequency = <1000000>;
-};
diff --git a/Documentation/devicetree/bindings/misc/ge-achc.yaml b/Documentation/devicetree/bindings/misc/ge-achc.yaml
new file mode 100644
index 000000000000..ff07aa62ed57
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/ge-achc.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+# Copyright (C) 2021 GE Inc.
+# Copyright (C) 2021 Collabora Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/ge-achc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GE Healthcare USB Management Controller
+
+description: |
+  A device which handles data acquisition from compatible USB based peripherals.
+  SPI is used for device management.
+
+  Note: This device does not expose the peripherals as USB devices.
+
+maintainers:
+  - Sebastian Reichel <sre@kernel.org>
+
+properties:
+  compatible:
+    items:
+      - const: ge,achc
+      - const: nxp,kinetis-k20
+
+  clocks:
+    maxItems: 1
+
+  vdd-supply:
+    description: Digital power supply regulator on VDD pin
+
+  vdda-supply:
+    description: Analog power supply regulator on VDDA pin
+
+  reg:
+    items:
+      - description: Control interface
+      - description: Firmware programming interface
+
+  reset-gpios:
+    description: GPIO used for hardware reset.
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        spi@1 {
+            compatible = "ge,achc", "nxp,kinetis-k20";
+            reg = <1>, <0>;
+            clocks = <&achc_24M>;
+            reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.30.2

[PATCHv6 2/3] ARM: dts: imx53-ppd: Fix ACHC entry

[Sebastian Reichel]

PPD has only one ACHC device, which effectively is a Kinetis
microcontroller. It has one SPI interface used for normal
communication. Additionally it's possible to flash the device
firmware using NXP's EzPort protocol by correctly driving a
second chip select pin and the device reset pin.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm/boot/dts/imx53-ppd.dts | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts
index 5a5fa6190a52..37d0cffea99c 100644
--- a/arch/arm/boot/dts/imx53-ppd.dts
+++ b/arch/arm/boot/dts/imx53-ppd.dts
@@ -70,6 +70,12 @@ cko2_11M: sgtl-clock-cko2 {
 		clock-frequency = <11289600>;
 	};
 
+	achc_24M: achc-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+	};
+
 	sgtlsound: sound {
 		compatible = "fsl,imx53-cpuvo-sgtl5000",
 			     "fsl,imx-audio-sgtl5000";
@@ -314,16 +320,13 @@ &gpio4 11 GPIO_ACTIVE_LOW
 		    &gpio4 12 GPIO_ACTIVE_LOW>;
 	status = "okay";
 
-	spidev0: spi@0 {
-		compatible = "ge,achc";
-		reg = <0>;
-		spi-max-frequency = <1000000>;
-	};
-
-	spidev1: spi@1 {
-		compatible = "ge,achc";
-		reg = <1>;
-		spi-max-frequency = <1000000>;
+	spidev0: spi@1 {
+		compatible = "ge,achc", "nxp,kinetis-k20";
+		reg = <1>, <0>;
+		vdd-supply = <&reg_3v3>;
+		vdda-supply = <&reg_3v3>;
+		clocks = <&achc_24M>;
+		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
 	};
 
 	gpioxra0: gpio@2 {
-- 
2.30.2

[PATCHv6 3/3] misc: gehc-achc: new driver

[Sebastian Reichel]

General Electric Healthcare's PPD has a secondary processor from
NXP's Kinetis K20 series. That device has two SPI chip selects:

The main interface's behaviour depends on the loaded firmware
and is currently unused.

The secondary interface can be used to update the firmware using
EzPort protocol. This is implemented by this driver using the
kernel's firmware API. It's not done during probe time, since
the device has non-volatile memory and flashing lasts almost 3
minutes.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../ABI/testing/sysfs-driver-ge-achc          |  14 +
 drivers/misc/Kconfig                          |  11 +
 drivers/misc/Makefile                         |   2 +
 drivers/misc/gehc-achc.c                      | 136 +++++
 drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
 drivers/misc/nxp-ezport.h                     |   9 +
 drivers/spi/spidev.c                          |   1 -
 7 files changed, 640 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
 create mode 100644 drivers/misc/gehc-achc.c
 create mode 100644 drivers/misc/nxp-ezport.c
 create mode 100644 drivers/misc/nxp-ezport.h

diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc
new file mode 100644
index 000000000000..f02812731082
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-ge-achc
@@ -0,0 +1,14 @@
+What:		/sys/bus/spi/<dev>/update_firmware
+Date:		Jul 2021
+Contact:	sebastian.reichel@collabora.com
+Description:	Write 1 to this file to update the ACHC microcontroller
+		firmware via the EzPort interface. For this the kernel
+		will load "achc.bin" via the firmware API (so usually
+		from /lib/firmware). The write will block until the FW
+		has either been flashed successfully or an error occured.
+
+What:		/sys/bus/spi/<dev>/reset
+Date:		Jul 2021
+Contact:	sebastian.reichel@collabora.com
+Description:	Write 1 to this file to reset the microcontroller via the
+		reset GPIO. The write will block until the reset completes.
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f4fb5c52b863..1ebf7000671d 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC
 	  MFGPTs have a better resolution and max interval than the
 	  generic PIT, and are suitable for use as high-res timers.
 
+config GEHC_ACHC
+	tristate "GEHC ACHC support"
+	depends on SPI && SYSFS
+	select FW_LOADER
+	help
+	  Support for GE ACHC microcontroller, that is part of the GE
+	  PPD device.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gehc-achc.
+
 config HP_ILO
 	tristate "Channel interface driver for the HP iLO processor"
 	depends on PCI
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e92a56d4442f..b50180796931 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
 obj-$(CONFIG_SGI_XP)		+= sgi-xp/
 obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
 obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
+obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc-mod.o
+gehc-achc-mod-objs		:= gehc-achc.o nxp-ezport.o
 obj-$(CONFIG_HP_ILO)		+= hpilo.o
 obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
 obj-$(CONFIG_ISL29003)		+= isl29003.o
diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c
new file mode 100644
index 000000000000..893940e7ca4e
--- /dev/null
+++ b/drivers/misc/gehc-achc.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf
+ *
+ * Copyright (C) 2018-2021 Collabora
+ * Copyright (C) 2018-2021 GE Healthcare
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/of.h>
+#include "nxp-ezport.h"
+
+#define ACHC_MAX_FREQ_HZ 300000
+
+struct achc_data {
+	struct spi_device *main;
+	struct spi_device *ezport;
+	struct gpio_desc *reset;
+
+	struct mutex device_lock; /* avoid concurrent device access */
+};
+
+static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct achc_data *achc = dev_get_drvdata(dev);
+	int ret;
+
+	if (count != 1 || buf[0] != '1')
+		return -EINVAL;
+
+	mutex_lock(&achc->device_lock);
+	ret = ezport_flash(achc->ezport, achc->reset, "achc.bin");
+	mutex_unlock(&achc->device_lock);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_WO(update_firmware);
+
+static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct achc_data *achc = dev_get_drvdata(dev);
+
+	if (count != 1 || buf[0] != '1')
+		return -EINVAL;
+
+	mutex_lock(&achc->device_lock);
+	ezport_reset(achc->reset);
+	mutex_unlock(&achc->device_lock);
+
+	return count;
+}
+static DEVICE_ATTR_WO(reset);
+
+static struct attribute *gehc_achc_attrs[] = {
+	&dev_attr_update_firmware.attr,
+	&dev_attr_reset.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(gehc_achc);
+
+static void unregister_ezport(void *data)
+{
+	struct spi_device *ezport = data;
+
+	spi_unregister_device(ezport);
+}
+
+static int gehc_achc_probe(struct spi_device *spi)
+{
+	struct achc_data *achc;
+	int ezport_reg, ret;
+
+	spi->max_speed_hz = ACHC_MAX_FREQ_HZ;
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_0;
+
+	achc = devm_kzalloc(&spi->dev, sizeof(*achc), GFP_KERNEL);
+	if (!achc)
+		return -ENOMEM;
+	spi_set_drvdata(spi, achc);
+	achc->main = spi;
+
+	mutex_init(&achc->device_lock);
+
+	ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");
+
+	achc->ezport = spi_new_ancillary_device(spi, ezport_reg);
+	if (IS_ERR(achc->ezport))
+		return PTR_ERR(achc->ezport);
+
+	ret = devm_add_action_or_reset(&spi->dev, unregister_ezport, achc->ezport);
+	if (ret)
+		return ret;
+
+	achc->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(achc->reset))
+		return dev_err_probe(&spi->dev, PTR_ERR(achc->reset), "Could not get reset gpio\n");
+
+	return 0;
+}
+
+static const struct spi_device_id gehc_achc_id[] = {
+	{ "ge,achc", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, gehc_achc_id);
+
+static const struct of_device_id gehc_achc_of_match[] = {
+	{ .compatible = "ge,achc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, gehc_achc_of_match);
+
+static struct spi_driver gehc_achc_spi_driver = {
+	.driver = {
+		.name	= "gehc-achc",
+		.of_match_table = gehc_achc_of_match,
+		.dev_groups = gehc_achc_groups,
+	},
+	.probe		= gehc_achc_probe,
+	.id_table	= gehc_achc_id,
+};
+module_spi_driver(gehc_achc_spi_driver);
+
+MODULE_DESCRIPTION("GEHC ACHC driver");
+MODULE_AUTHOR("Sebastian Reichel <sebastian.reichel@collabora.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/nxp-ezport.c b/drivers/misc/nxp-ezport.c
new file mode 100644
index 000000000000..1096af5690c8
--- /dev/null
+++ b/drivers/misc/nxp-ezport.c
@@ -0,0 +1,468 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * This library implements NXP's EzPort protocol used to
+ * flash the firmware of Kinetis microcontrollers, such
+ * as https://www.nxp.com/part/MK20FN1M0VMD12 and is
+ * intended to be used as ancillary SPI device.
+ *
+ * Copyright (C) 2018-2021 Collabora
+ * Copyright (C) 2018-2021 GE Healthcare
+ */
+
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
+#include "nxp-ezport.h"
+
+#define EZPORT_RESET_DELAY_MS	100
+#define EZPORT_STARTUP_DELAY_MS	200
+#define EZPORT_WRITE_WAIT_MS	10
+#define EZPORT_TRANSFER_SIZE	2048
+
+#define EZPORT_CMD_SP		0x02 /* flash section program */
+#define EZPORT_CMD_RDSR		0x05 /* read status register */
+#define EZPORT_CMD_WREN		0x06 /* write enable */
+#define EZPORT_CMD_FAST_READ	0x0b /* flash read data at high speed */
+#define EZPORT_CMD_RESET	0xb9 /* reset chip */
+#define EZPORT_CMD_BE		0xc7 /* bulk erase */
+#define EZPORT_CMD_SE		0xd8 /* sector erase */
+
+#define EZPORT_SECTOR_SIZE	4096
+#define EZPORT_SECTOR_MASK	(EZPORT_SECTOR_SIZE - 1)
+
+#define EZPORT_STATUS_WIP	BIT(0) /* write in progress */
+#define EZPORT_STATUS_WEN	BIT(1) /* write enable */
+#define EZPORT_STATUS_BEDIS	BIT(2) /* bulk erase disable */
+#define EZPORT_STATUS_FLEXRAM	BIT(3) /* FlexRAM mode */
+#define EZPORT_STATUS_WEF	BIT(6) /* write error flag */
+#define EZPORT_STATUS_FS	BIT(7) /* flash security */
+
+/**
+ * ezport_reset - reset ezport device
+ * @reset: the gpio connected to the device reset pin
+ *
+ * Context: can sleep
+ */
+void ezport_reset(struct gpio_desc *reset)
+{
+	gpiod_set_value(reset, 1);
+	msleep(EZPORT_RESET_DELAY_MS);
+	gpiod_set_value(reset, 0);
+	msleep(EZPORT_STARTUP_DELAY_MS);
+}
+
+static int ezport_start_programming(struct spi_device *spi, struct gpio_desc *reset)
+{
+	struct spi_message msg;
+	struct spi_transfer assert_cs = {
+		.cs_change   = 1,
+	};
+	struct spi_transfer release_cs = { };
+	int ret;
+
+	spi_bus_lock(spi->master);
+
+	/* assert chip select */
+	spi_message_init(&msg);
+	spi_message_add_tail(&assert_cs, &msg);
+	ret = spi_sync_locked(spi, &msg);
+	if (ret)
+		goto fail;
+
+	msleep(EZPORT_STARTUP_DELAY_MS);
+
+	/* reset with asserted chip select to switch into programming mode */
+	ezport_reset(reset);
+
+	/* release chip select */
+	spi_message_init(&msg);
+	spi_message_add_tail(&release_cs, &msg);
+	ret = spi_sync_locked(spi, &msg);
+
+fail:
+	spi_bus_unlock(spi->master);
+	return ret;
+}
+
+static void ezport_stop_programming(struct spi_device *spi, struct gpio_desc *reset)
+{
+	/* reset without asserted chip select to return into normal mode */
+	spi_bus_lock(spi->master);
+	ezport_reset(reset);
+	spi_bus_unlock(spi->master);
+}
+
+static int ezport_get_status_register(struct spi_device *spi)
+{
+	int ret;
+
+	ret = spi_w8r8(spi, EZPORT_CMD_RDSR);
+	if (ret < 0)
+		return ret;
+	if (ret == 0xff) {
+		dev_err(&spi->dev, "Invalid EzPort status, EzPort is not functional!\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int ezport_soft_reset(struct spi_device *spi)
+{
+	u8 cmd = EZPORT_CMD_RESET;
+	int ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	msleep(EZPORT_STARTUP_DELAY_MS);
+
+	return 0;
+}
+
+static int ezport_send_simple(struct spi_device *spi, u8 cmd)
+{
+	int ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	return ezport_get_status_register(spi);
+}
+
+static int ezport_wait_write(struct spi_device *spi, u32 retries)
+{
+	int ret;
+	u32 i;
+
+	for (i = 0; i < retries; i++) {
+		ret = ezport_get_status_register(spi);
+		if (ret > 0 && !(ret & EZPORT_STATUS_WIP))
+			break;
+		msleep(EZPORT_WRITE_WAIT_MS);
+	}
+
+	return ret;
+}
+
+static int ezport_write_enable(struct spi_device *spi)
+{
+	int ret = 0, retries = 3;
+
+	for (retries = 0; retries < 3; retries++) {
+		ret = ezport_send_simple(spi, EZPORT_CMD_WREN);
+		if (ret > 0 && ret & EZPORT_STATUS_WEN)
+			break;
+	}
+
+	if (!(ret & EZPORT_STATUS_WEN)) {
+		dev_err(&spi->dev, "EzPort write enable timed out\n");
+		return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int ezport_bulk_erase(struct spi_device *spi)
+{
+	int ret;
+	static const u8 cmd = EZPORT_CMD_BE;
+
+	dev_dbg(&spi->dev, "EzPort bulk erase...\n");
+
+	ret = ezport_write_enable(spi);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = ezport_wait_write(spi, 1000);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int ezport_section_erase(struct spi_device *spi, u32 address)
+{
+	u8 query[] = {EZPORT_CMD_SE, (address >> 16) & 0xff, (address >> 8) & 0xff, address & 0xff};
+	int ret;
+
+	dev_dbg(&spi->dev, "Ezport section erase @ 0x%06x...\n", address);
+
+	if (address & EZPORT_SECTOR_MASK)
+		return -EINVAL;
+
+	ret = ezport_write_enable(spi);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_write(spi, query, sizeof(query));
+	if (ret < 0)
+		return ret;
+
+	return ezport_wait_write(spi, 200);
+}
+
+static int ezport_flash_transfer(struct spi_device *spi, u32 address,
+				 const u8 *payload, size_t payload_size)
+{
+	struct spi_transfer xfers[2] = {};
+	u8 *command;
+	int ret;
+
+	dev_dbg(&spi->dev, "EzPort write %zu bytes @ 0x%06x...\n", payload_size, address);
+
+	command = kmalloc(4, GFP_KERNEL | GFP_DMA);
+	if (!command)
+		return -ENOMEM;
+
+	command[0] = EZPORT_CMD_SP;
+	command[1] = address >> 16;
+	command[2] = address >> 8;
+	command[3] = address >> 0;
+
+	xfers[0].tx_buf = command;
+	xfers[0].len = 4;
+
+	xfers[1].tx_buf = payload;
+	xfers[1].len = payload_size;
+
+	ret = spi_sync_transfer(spi, xfers, 2);
+	kfree(command);
+	if (ret < 0)
+		return ret;
+
+	return ezport_wait_write(spi, 40);
+}
+
+static int ezport_flash_compare(struct spi_device *spi, u32 address,
+				const u8 *payload, size_t payload_size)
+{
+	struct spi_transfer xfers[2] = {};
+	u8 *buffer;
+	int ret;
+
+	buffer = kmalloc(payload_size + 4, GFP_KERNEL | GFP_DMA);
+	if (!buffer)
+		return -ENOMEM;
+
+	buffer[0] = EZPORT_CMD_FAST_READ;
+	buffer[1] = address >> 16;
+	buffer[2] = address >> 8;
+	buffer[3] = address >> 0;
+
+	xfers[0].tx_buf = buffer;
+	xfers[0].len = 4;
+
+	xfers[1].rx_buf = buffer + 4;
+	xfers[1].len = payload_size;
+
+	ret = spi_sync_transfer(spi, xfers, 2);
+	if (ret)
+		goto err;
+
+	ret = memcmp(payload, buffer + 4, payload_size);
+	if (ret) {
+		ret = -EBADMSG;
+		dev_dbg(&spi->dev, "Verfication failure @ %06x", address);
+		print_hex_dump_bytes("fw:  ", DUMP_PREFIX_OFFSET, payload, payload_size);
+		print_hex_dump_bytes("dev: ", DUMP_PREFIX_OFFSET, buffer + 4, payload_size);
+	}
+
+err:
+	kfree(buffer);
+	return ret;
+}
+
+static int ezport_firmware_compare_data(struct spi_device *spi,
+					const u8 *data, size_t size)
+{
+	int ret;
+	u32 address = 0;
+	u32 transfer_size;
+
+	dev_dbg(&spi->dev, "EzPort compare data with %zu bytes...\n", size);
+
+	ret = ezport_get_status_register(spi);
+	if (ret < 0)
+		return ret;
+
+	if (ret & EZPORT_STATUS_FS) {
+		dev_info(&spi->dev, "Device is in secure mode (status=0x%02x)!\n", ret);
+		dev_info(&spi->dev, "FW verification is not possible\n");
+		return -EACCES;
+	}
+
+	while (size - address > 0) {
+		transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
+
+		ret = ezport_flash_compare(spi, address, data+address, transfer_size);
+		if (ret)
+			return ret;
+
+		address += transfer_size;
+	}
+
+	return 0;
+}
+
+static int ezport_firmware_flash_data(struct spi_device *spi,
+				      const u8 *data, size_t size)
+{
+	int ret;
+	u32 address = 0;
+	u32 transfer_size;
+
+	dev_dbg(&spi->dev, "EzPort flash data with %zu bytes...\n", size);
+
+	ret = ezport_get_status_register(spi);
+	if (ret < 0)
+		return ret;
+
+	if (ret & EZPORT_STATUS_FS) {
+		ret = ezport_bulk_erase(spi);
+		if (ret < 0)
+			return ret;
+		if (ret & EZPORT_STATUS_FS)
+			return -EINVAL;
+	}
+
+	while (size - address > 0) {
+		if (!(address & EZPORT_SECTOR_MASK)) {
+			ret = ezport_section_erase(spi, address);
+			if (ret < 0)
+				return ret;
+			if (ret & EZPORT_STATUS_WIP || ret & EZPORT_STATUS_WEF)
+				return -EIO;
+		}
+
+		transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
+
+		ret = ezport_flash_transfer(spi, address,
+					    data+address, transfer_size);
+		if (ret < 0)
+			return ret;
+		else if (ret & EZPORT_STATUS_WIP)
+			return -ETIMEDOUT;
+		else if (ret & EZPORT_STATUS_WEF)
+			return -EIO;
+
+		address += transfer_size;
+	}
+
+	ret = ezport_soft_reset(spi);
+	if (ret < 0)
+		dev_warn(&spi->dev, "EzPort reset failed!\n");
+
+	dev_dbg(&spi->dev, "EzPort verify flashed data...\n");
+	ret = ezport_firmware_compare_data(spi, data, size);
+
+	/* allow missing FW verfication in secure mode */
+	if (ret == -EACCES)
+		ret = 0;
+
+	if (ret < 0)
+		dev_err(&spi->dev, "Failed to verify flashed data: %d\n", ret);
+
+	return ret;
+}
+
+static int ezport_firmware_load(struct spi_device *spi, const char *fwname)
+{
+	const struct firmware *fw;
+	int ret;
+
+	ret = request_firmware(&fw, fwname, &spi->dev);
+	if (ret) {
+		dev_err(&spi->dev, "Could not get firmware: %d\n", ret);
+		return ret;
+	}
+
+	ret = ezport_firmware_flash_data(spi, fw->data, fw->size);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int ezport_firmware_verify(struct spi_device *spi, const char *fwname)
+{
+	const struct firmware *fw;
+	int ret;
+
+	ret = request_firmware(&fw, fwname, &spi->dev);
+	if (ret) {
+		dev_err(&spi->dev, "Could not get firmware: %d\n", ret);
+		return ret;
+	}
+
+	ret = ezport_firmware_compare_data(spi, fw->data, fw->size);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+/**
+ * ezport_flash - flash device firmware
+ * @spi: SPI device for NXP EzPort interface
+ * @reset: the gpio connected to the device reset pin
+ * @fwname: filename of the firmware that should be flashed
+ *
+ * Context: can sleep
+ *
+ * Return: 0 on success; negative errno on failure
+ */
+int ezport_flash(struct spi_device *spi, struct gpio_desc *reset, const char *fwname)
+{
+	int ret;
+
+	ret = ezport_start_programming(spi, reset);
+	if (ret)
+		return ret;
+
+	ret = ezport_firmware_load(spi, fwname);
+
+	ezport_stop_programming(spi, reset);
+
+	if (ret)
+		dev_err(&spi->dev, "Failed to flash firmware: %d\n", ret);
+	else
+		dev_dbg(&spi->dev, "Finished FW flashing!\n");
+
+	return ret;
+}
+
+/**
+ * ezport_verify - verify device firmware
+ * @spi: SPI device for NXP EzPort interface
+ * @reset: the gpio connected to the device reset pin
+ * @fwname: filename of the firmware that should be compared
+ *
+ * Context: can sleep
+ *
+ * Return: 0 on success; negative errno on failure
+ */
+int ezport_verify(struct spi_device *spi, struct gpio_desc *reset, const char *fwname)
+{
+	int ret;
+
+	ret = ezport_start_programming(spi, reset);
+	if (ret)
+		return ret;
+
+	ret = ezport_firmware_verify(spi, fwname);
+
+	ezport_stop_programming(spi, reset);
+
+	if (ret)
+		dev_err(&spi->dev, "Failed to verify firmware: %d\n", ret);
+
+	return ret;
+}
diff --git a/drivers/misc/nxp-ezport.h b/drivers/misc/nxp-ezport.h
new file mode 100644
index 000000000000..fc48af82409b
--- /dev/null
+++ b/drivers/misc/nxp-ezport.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __NXP_EZPORT_H
+#define __NXP_EZPORT_H
+
+void ezport_reset(struct gpio_desc *reset);
+int ezport_flash(struct spi_device *spi, struct gpio_desc *reset, const char *fwname);
+int ezport_verify(struct spi_device *spi, struct gpio_desc *reset, const char *fwname);
+
+#endif
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 24e9469ea35b..6dc29ce3b4bf 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -677,7 +677,6 @@ static struct class *spidev_class;
 static const struct of_device_id spidev_dt_ids[] = {
 	{ .compatible = "rohm,dh2228fv" },
 	{ .compatible = "lineartechnology,ltc2488" },
-	{ .compatible = "ge,achc" },
 	{ .compatible = "semtech,sx1301" },
 	{ .compatible = "lwn,bk4" },
 	{ .compatible = "dh,dhcom-board" },
-- 
2.30.2

Re: [PATCHv6 3/3] misc: gehc-achc: new driver

[Greg Kroah-Hartman]

On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:
> General Electric Healthcare's PPD has a secondary processor from
> NXP's Kinetis K20 series. That device has two SPI chip selects:
> 
> The main interface's behaviour depends on the loaded firmware
> and is currently unused.
> 
> The secondary interface can be used to update the firmware using
> EzPort protocol. This is implemented by this driver using the
> kernel's firmware API. It's not done during probe time, since
> the device has non-volatile memory and flashing lasts almost 3
> minutes.

In thinking about this some more, why does it matter?  Spin up a
workqueue when probing and do the firmware loading then.  That way you
do not end up creating yet another custom user/kernel api just to do
something as trivial as loading the firmware for a device.

And I think the firmware loader even handles async loading, or at least
it used to, maybe not anymore, it's a complex api, I recommend reading
the docs...



> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../ABI/testing/sysfs-driver-ge-achc          |  14 +
>  drivers/misc/Kconfig                          |  11 +
>  drivers/misc/Makefile                         |   2 +
>  drivers/misc/gehc-achc.c                      | 136 +++++
>  drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
>  drivers/misc/nxp-ezport.h                     |   9 +

Why is there two different modules here for the same piece of hardware?


>  drivers/spi/spidev.c                          |   1 -
>  7 files changed, 640 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
>  create mode 100644 drivers/misc/gehc-achc.c
>  create mode 100644 drivers/misc/nxp-ezport.c
>  create mode 100644 drivers/misc/nxp-ezport.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc
> new file mode 100644
> index 000000000000..f02812731082
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc
> @@ -0,0 +1,14 @@
> +What:		/sys/bus/spi/<dev>/update_firmware
> +Date:		Jul 2021
> +Contact:	sebastian.reichel@collabora.com
> +Description:	Write 1 to this file to update the ACHC microcontroller
> +		firmware via the EzPort interface. For this the kernel
> +		will load "achc.bin" via the firmware API (so usually
> +		from /lib/firmware). The write will block until the FW
> +		has either been flashed successfully or an error occured.
> +
> +What:		/sys/bus/spi/<dev>/reset
> +Date:		Jul 2021
> +Contact:	sebastian.reichel@collabora.com
> +Description:	Write 1 to this file to reset the microcontroller via the
> +		reset GPIO. The write will block until the reset completes.
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f4fb5c52b863..1ebf7000671d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC
>  	  MFGPTs have a better resolution and max interval than the
>  	  generic PIT, and are suitable for use as high-res timers.
>  
> +config GEHC_ACHC
> +	tristate "GEHC ACHC support"
> +	depends on SPI && SYSFS
> +	select FW_LOADER
> +	help
> +	  Support for GE ACHC microcontroller, that is part of the GE
> +	  PPD device.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called gehc-achc.
> +
>  config HP_ILO
>  	tristate "Channel interface driver for the HP iLO processor"
>  	depends on PCI
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e92a56d4442f..b50180796931 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
>  obj-$(CONFIG_SGI_XP)		+= sgi-xp/
>  obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
>  obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
> +obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc-mod.o
> +gehc-achc-mod-objs		:= gehc-achc.o nxp-ezport.o

Yeah, you are creating 2 modules for one Kconfig option, why not put
these together into 1?  Makes the code smaller.




>  obj-$(CONFIG_HP_ILO)		+= hpilo.o
>  obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
>  obj-$(CONFIG_ISL29003)		+= isl29003.o
> diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c
> new file mode 100644
> index 000000000000..893940e7ca4e
> --- /dev/null
> +++ b/drivers/misc/gehc-achc.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf
> + *
> + * Copyright (C) 2018-2021 Collabora
> + * Copyright (C) 2018-2021 GE Healthcare
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
> +#include "nxp-ezport.h"
> +
> +#define ACHC_MAX_FREQ_HZ 300000
> +
> +struct achc_data {
> +	struct spi_device *main;
> +	struct spi_device *ezport;
> +	struct gpio_desc *reset;
> +
> +	struct mutex device_lock; /* avoid concurrent device access */
> +};
> +
> +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct achc_data *achc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (count != 1 || buf[0] != '1')
> +		return -EINVAL;

There a core kernel call to see if the data written to a sysfs file is
"true/false" I would recommend, if you stick with this, to use that
instead.

thanks,

greg k-h

Re: [PATCHv6 3/3] misc: gehc-achc: new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 7349 bytes --]

Hi Greg,

On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:
> > General Electric Healthcare's PPD has a secondary processor from
> > NXP's Kinetis K20 series. That device has two SPI chip selects:
> > 
> > The main interface's behaviour depends on the loaded firmware
> > and is currently unused.
> > 
> > The secondary interface can be used to update the firmware using
> > EzPort protocol. This is implemented by this driver using the
> > kernel's firmware API. It's not done during probe time, since
> > the device has non-volatile memory and flashing lasts almost 3
> > minutes.
> 
> In thinking about this some more, why does it matter?  Spin up a
> workqueue when probing and do the firmware loading then. That way you
> do not end up creating yet another custom user/kernel api just to do
> something as trivial as loading the firmware for a device.
> 
> And I think the firmware loader even handles async loading, or at least
> it used to, maybe not anymore, it's a complex api, I recommend reading
> the docs...

Flashing the firmware during boot instead of on-demand is not
a good idea for two reasons:

1. This will wear the flash memory of the microcontroller for no
   good reason.

2. Even if the flashing happens in the background, the microcontroller
   will not be usable while it is being programmed. Thus the system
   boot takes 3 minutes instead of 30 seconds.

Note, that this is not the first device with this kind of problem.
I've seen this before for e.g. atmel touchscreen [0] basically does
the same via (undocumented) mxt_update_fw_store().

[0] drivers/input/touchscreen/atmel_mxt_ts.c

> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../ABI/testing/sysfs-driver-ge-achc          |  14 +
> >  drivers/misc/Kconfig                          |  11 +
> >  drivers/misc/Makefile                         |   2 +
> >  drivers/misc/gehc-achc.c                      | 136 +++++
> >  drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
> >  drivers/misc/nxp-ezport.h                     |   9 +
> 
> Why is there two different modules here for the same piece of hardware?

It's only one module, but I put all the generic ezport code into its
own file for easy code reuse. I can put it all into gehc-achc and
thus reduce the chance of code reuse if somebody else needs to do
ezport flashing. Since both files are built into the same module
the size savings of merging both files are minimal.

> >  drivers/spi/spidev.c                          |   1 -
> >  7 files changed, 640 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
> >  create mode 100644 drivers/misc/gehc-achc.c
> >  create mode 100644 drivers/misc/nxp-ezport.c
> >  create mode 100644 drivers/misc/nxp-ezport.h
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc
> > new file mode 100644
> > index 000000000000..f02812731082
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc
> > @@ -0,0 +1,14 @@
> > +What:		/sys/bus/spi/<dev>/update_firmware
> > +Date:		Jul 2021
> > +Contact:	sebastian.reichel@collabora.com
> > +Description:	Write 1 to this file to update the ACHC microcontroller
> > +		firmware via the EzPort interface. For this the kernel
> > +		will load "achc.bin" via the firmware API (so usually
> > +		from /lib/firmware). The write will block until the FW
> > +		has either been flashed successfully or an error occured.
> > +
> > +What:		/sys/bus/spi/<dev>/reset
> > +Date:		Jul 2021
> > +Contact:	sebastian.reichel@collabora.com
> > +Description:	Write 1 to this file to reset the microcontroller via the
> > +		reset GPIO. The write will block until the reset completes.
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index f4fb5c52b863..1ebf7000671d 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC
> >  	  MFGPTs have a better resolution and max interval than the
> >  	  generic PIT, and are suitable for use as high-res timers.
> >  
> > +config GEHC_ACHC
> > +	tristate "GEHC ACHC support"
> > +	depends on SPI && SYSFS
> > +	select FW_LOADER
> > +	help
> > +	  Support for GE ACHC microcontroller, that is part of the GE
> > +	  PPD device.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called gehc-achc.
> > +
> >  config HP_ILO
> >  	tristate "Channel interface driver for the HP iLO processor"
> >  	depends on PCI
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index e92a56d4442f..b50180796931 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
> >  obj-$(CONFIG_SGI_XP)		+= sgi-xp/
> >  obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
> >  obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
> > +obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc-mod.o
> > +gehc-achc-mod-objs		:= gehc-achc.o nxp-ezport.o
> 
> Yeah, you are creating 2 modules for one Kconfig option, why not put
> these together into 1?  Makes the code smaller.

Actually the above builds both C files into objects and then both
objects into 1 module. It's a common pattern in some parts of the
kernel, e.g. drm.

> >  obj-$(CONFIG_HP_ILO)		+= hpilo.o
> >  obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
> >  obj-$(CONFIG_ISL29003)		+= isl29003.o
> > diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c
> > new file mode 100644
> > index 000000000000..893940e7ca4e
> > --- /dev/null
> > +++ b/drivers/misc/gehc-achc.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf
> > + *
> > + * Copyright (C) 2018-2021 Collabora
> > + * Copyright (C) 2018-2021 GE Healthcare
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/of.h>
> > +#include "nxp-ezport.h"
> > +
> > +#define ACHC_MAX_FREQ_HZ 300000
> > +
> > +struct achc_data {
> > +	struct spi_device *main;
> > +	struct spi_device *ezport;
> > +	struct gpio_desc *reset;
> > +
> > +	struct mutex device_lock; /* avoid concurrent device access */
> > +};
> > +
> > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct achc_data *achc = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	if (count != 1 || buf[0] != '1')
> > +		return -EINVAL;
> 
> There a core kernel call to see if the data written to a sysfs file is
> "true/false" I would recommend, if you stick with this, to use that
> instead.

I have problems understanding that sentence, it seems to be missing
a word.

As far as I can tell -EINVAL is usually used when the user supplied
data is not formatted correctly for sysfs store routines, e.g. lot's
of them are returning -EINVAL like this:

rc = kstrtoul(buf, 0, &value);
if (rc)
    return rc;

Did I miss something?

Thanks,

-- Sebastian

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

Re: [PATCHv6 3/3] misc: gehc-achc: new driver

[Greg Kroah-Hartman]

On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote:
> Hi Greg,
> 
> On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:
> > > General Electric Healthcare's PPD has a secondary processor from
> > > NXP's Kinetis K20 series. That device has two SPI chip selects:
> > > 
> > > The main interface's behaviour depends on the loaded firmware
> > > and is currently unused.
> > > 
> > > The secondary interface can be used to update the firmware using
> > > EzPort protocol. This is implemented by this driver using the
> > > kernel's firmware API. It's not done during probe time, since
> > > the device has non-volatile memory and flashing lasts almost 3
> > > minutes.
> > 
> > In thinking about this some more, why does it matter?  Spin up a
> > workqueue when probing and do the firmware loading then. That way you
> > do not end up creating yet another custom user/kernel api just to do
> > something as trivial as loading the firmware for a device.
> > 
> > And I think the firmware loader even handles async loading, or at least
> > it used to, maybe not anymore, it's a complex api, I recommend reading
> > the docs...
> 
> Flashing the firmware during boot instead of on-demand is not
> a good idea for two reasons:
> 
> 1. This will wear the flash memory of the microcontroller for no
>    good reason.

Why would you boot with this hardware and not want the firmware loaded?

> 2. Even if the flashing happens in the background, the microcontroller
>    will not be usable while it is being programmed. Thus the system
>    boot takes 3 minutes instead of 30 seconds.

Why would the system boot depend on the firmware being loaded?

You have to wait sometime, might as well get the first 30 seconds of
waiting out of the way while the rest of the system boots :)

> Note, that this is not the first device with this kind of problem.
> I've seen this before for e.g. atmel touchscreen [0] basically does
> the same via (undocumented) mxt_update_fw_store().
> 
> [0] drivers/input/touchscreen/atmel_mxt_ts.c

Yes, but everyone else uses the normal firmware api method of doing
this, don't take one odd driver as being the "rule" :)

> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  .../ABI/testing/sysfs-driver-ge-achc          |  14 +
> > >  drivers/misc/Kconfig                          |  11 +
> > >  drivers/misc/Makefile                         |   2 +
> > >  drivers/misc/gehc-achc.c                      | 136 +++++
> > >  drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
> > >  drivers/misc/nxp-ezport.h                     |   9 +
> > 
> > Why is there two different modules here for the same piece of hardware?
> 
> It's only one module, but I put all the generic ezport code into its
> own file for easy code reuse. I can put it all into gehc-achc and
> thus reduce the chance of code reuse if somebody else needs to do
> ezport flashing. Since both files are built into the same module
> the size savings of merging both files are minimal.

Sorry, you are right, I read the code and Makefile wrong.

But, from an organizing point of view, having 2 random files in that
directory that are not named alike being built together into one module
is not obvious at all.

For now, keep it to one file.  If future users come along that need this
method, split the code then, no need to do extra work at this point in
time for unknown uses.

> > > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
> > > +				     const char *buf, size_t count)
> > > +{
> > > +	struct achc_data *achc = dev_get_drvdata(dev);
> > > +	int ret;
> > > +
> > > +	if (count != 1 || buf[0] != '1')
> > > +		return -EINVAL;
> > 
> > There a core kernel call to see if the data written to a sysfs file is
> > "true/false" I would recommend, if you stick with this, to use that
> > instead.
> 
> I have problems understanding that sentence, it seems to be missing
> a word.
> 
> As far as I can tell -EINVAL is usually used when the user supplied
> data is not formatted correctly for sysfs store routines, e.g. lot's
> of them are returning -EINVAL like this:
> 
> rc = kstrtoul(buf, 0, &value);
> if (rc)
>     return rc;
> 
> Did I miss something?

I was trying to say "use kstrtoul()" instead of trying to hand-roll much
the same thing.  Sorry for not being specific.

thanks,

greg k-h

Re: [PATCHv6 3/3] misc: gehc-achc: new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 6103 bytes --]

Hi,

On Tue, Jul 13, 2021 at 07:41:51AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote:
> > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:
> > > > General Electric Healthcare's PPD has a secondary processor from
> > > > NXP's Kinetis K20 series. That device has two SPI chip selects:
> > > > 
> > > > The main interface's behaviour depends on the loaded firmware
> > > > and is currently unused.
> > > > 
> > > > The secondary interface can be used to update the firmware using
> > > > EzPort protocol. This is implemented by this driver using the
> > > > kernel's firmware API. It's not done during probe time, since
> > > > the device has non-volatile memory and flashing lasts almost 3
> > > > minutes.
> > > 
> > > In thinking about this some more, why does it matter?  Spin up a
> > > workqueue when probing and do the firmware loading then. That way you
> > > do not end up creating yet another custom user/kernel api just to do
> > > something as trivial as loading the firmware for a device.
> > > 
> > > And I think the firmware loader even handles async loading, or at least
> > > it used to, maybe not anymore, it's a complex api, I recommend reading
> > > the docs...
> > 
> > Flashing the firmware during boot instead of on-demand is not
> > a good idea for two reasons:
> > 
> > 1. This will wear the flash memory of the microcontroller for no
> >    good reason.
> 
> Why would you boot with this hardware and not want the firmware
> loaded?

As written in the commit message this code is updateing the firmware
in non-volatile memory, so the previously flashed FW will be used
until a new one is flashed.

The datasheet for the memory being programmed states, that it has a
guaranteed programming endurance of 10.000 times. So programming it
at every boot instead of manually when a new FW should be flashed
(i.e. once every few years) shortens the device life time considerably.

> > 2. Even if the flashing happens in the background, the microcontroller
> >    will not be usable while it is being programmed. Thus the system
> >    boot takes 3 minutes instead of 30 seconds.
> 
> Why would the system boot depend on the firmware being loaded?
> 
> You have to wait sometime, might as well get the first 30 seconds of
> waiting out of the way while the rest of the system boots :)

The microcontroller collects sensor data for the system and is
essential for the overall functionality of the medical device.
While the firmware flashing is on-going the device cannot be
used normally, since the microcontroller will not process and
forward the data.

> > Note, that this is not the first device with this kind of problem.
> > I've seen this before for e.g. atmel touchscreen [0] basically does
> > the same via (undocumented) mxt_update_fw_store().
> > 
> > [0] drivers/input/touchscreen/atmel_mxt_ts.c
> 
> Yes, but everyone else uses the normal firmware api method of doing
> this, don't take one odd driver as being the "rule" :)

Most FW updates happen to volatile memory (= firmware data is lost
on power loss) and must be programmed at boot time. So I'm not
looking for the average FW update driver, but for the odd driver
also updating FW in non-volatile memory. Those often have the
same limitations as the Kinetis K20 has:

 * limited amount of programming cycles available
 * slow programming speed

> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > ---
> > > >  .../ABI/testing/sysfs-driver-ge-achc          |  14 +
> > > >  drivers/misc/Kconfig                          |  11 +
> > > >  drivers/misc/Makefile                         |   2 +
> > > >  drivers/misc/gehc-achc.c                      | 136 +++++
> > > >  drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
> > > >  drivers/misc/nxp-ezport.h                     |   9 +
> > > 
> > > Why is there two different modules here for the same piece of hardware?
> > 
> > It's only one module, but I put all the generic ezport code into its
> > own file for easy code reuse. I can put it all into gehc-achc and
> > thus reduce the chance of code reuse if somebody else needs to do
> > ezport flashing. Since both files are built into the same module
> > the size savings of merging both files are minimal.
> 
> Sorry, you are right, I read the code and Makefile wrong.
> 
> But, from an organizing point of view, having 2 random files in that
> directory that are not named alike being built together into one module
> is not obvious at all.
> 
> For now, keep it to one file.  If future users come along that need this
> method, split the code then, no need to do extra work at this point in
> time for unknown uses.

Ok, I will put it all into gehc-achc. I doubt somebody will find
ezport code in there in the future, but that's not my problem :)

> > > > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
> > > > +				     const char *buf, size_t count)
> > > > +{
> > > > +	struct achc_data *achc = dev_get_drvdata(dev);
> > > > +	int ret;
> > > > +
> > > > +	if (count != 1 || buf[0] != '1')
> > > > +		return -EINVAL;
> > > 
> > > There a core kernel call to see if the data written to a sysfs file is
> > > "true/false" I would recommend, if you stick with this, to use that
> > > instead.
> > 
> > I have problems understanding that sentence, it seems to be missing
> > a word.
> > 
> > As far as I can tell -EINVAL is usually used when the user supplied
> > data is not formatted correctly for sysfs store routines, e.g. lot's
> > of them are returning -EINVAL like this:
> > 
> > rc = kstrtoul(buf, 0, &value);
> > if (rc)
> >     return rc;
> > 
> > Did I miss something?
> 
> I was trying to say "use kstrtoul()" instead of trying to hand-roll much
> the same thing.  Sorry for not being specific.

Ok.

Thanks,

-- Sebastian

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

Re: [PATCHv6 3/3] misc: gehc-achc: new driver

[Greg Kroah-Hartman]

On Tue, Jul 13, 2021 at 12:46:00PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jul 13, 2021 at 07:41:51AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote:
> > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:
> > > > > General Electric Healthcare's PPD has a secondary processor from
> > > > > NXP's Kinetis K20 series. That device has two SPI chip selects:
> > > > > 
> > > > > The main interface's behaviour depends on the loaded firmware
> > > > > and is currently unused.
> > > > > 
> > > > > The secondary interface can be used to update the firmware using
> > > > > EzPort protocol. This is implemented by this driver using the
> > > > > kernel's firmware API. It's not done during probe time, since
> > > > > the device has non-volatile memory and flashing lasts almost 3
> > > > > minutes.
> > > > 
> > > > In thinking about this some more, why does it matter?  Spin up a
> > > > workqueue when probing and do the firmware loading then. That way you
> > > > do not end up creating yet another custom user/kernel api just to do
> > > > something as trivial as loading the firmware for a device.
> > > > 
> > > > And I think the firmware loader even handles async loading, or at least
> > > > it used to, maybe not anymore, it's a complex api, I recommend reading
> > > > the docs...
> > > 
> > > Flashing the firmware during boot instead of on-demand is not
> > > a good idea for two reasons:
> > > 
> > > 1. This will wear the flash memory of the microcontroller for no
> > >    good reason.
> > 
> > Why would you boot with this hardware and not want the firmware
> > loaded?
> 
> As written in the commit message this code is updateing the firmware
> in non-volatile memory, so the previously flashed FW will be used
> until a new one is flashed.
> 
> The datasheet for the memory being programmed states, that it has a
> guaranteed programming endurance of 10.000 times. So programming it
> at every boot instead of manually when a new FW should be flashed
> (i.e. once every few years) shortens the device life time considerably.

Ah, ok, that was not obvious at all.  This is much more like a "BIOS
update" in that it is infrequent.  You might want to document the heck
out of this so others are not confused like me.

thanks,

greg k-h

Re: [PATCHv6 1/3] dt-bindings: misc: ge-achc: Convert to DT schema format

[Rob Herring]

On Mon, 12 Jul 2021 17:02:40 +0200, Sebastian Reichel wrote:
> Convert the binding to DT schema format. Also update the binding
> to fix shortcomings
> 
>  * Add "nxp,kinetis-k20" fallback compatible
>  * add programming SPI interface and reset GPIO
>  * add main clock
>  * add voltage supplies
>  * drop spi-max-frequency from required properties,
>    driver will setup max. frequency
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../devicetree/bindings/misc/ge-achc.txt      | 26 --------
>  .../devicetree/bindings/misc/ge-achc.yaml     | 65 +++++++++++++++++++
>  2 files changed, 65 insertions(+), 26 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
>  create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: [[1], [0]] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: Additional items are not allowed ([0] was unexpected)
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml
\ndoc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCHv6 1/3] dt-bindings: misc: ge-achc: Convert to DT schema format

[Rob Herring]

On Tue, Jul 13, 2021 at 8:03 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, 12 Jul 2021 17:02:40 +0200, Sebastian Reichel wrote:
> > Convert the binding to DT schema format. Also update the binding
> > to fix shortcomings
> >
> >  * Add "nxp,kinetis-k20" fallback compatible
> >  * add programming SPI interface and reset GPIO
> >  * add main clock
> >  * add voltage supplies
> >  * drop spi-max-frequency from required properties,
> >    driver will setup max. frequency
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../devicetree/bindings/misc/ge-achc.txt      | 26 --------
> >  .../devicetree/bindings/misc/ge-achc.yaml     | 65 +++++++++++++++++++
> >  2 files changed, 65 insertions(+), 26 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
> >  create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: [[1], [0]] is too long
>         From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: Additional items are not allowed ([0] was unexpected)
>         From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml

Ignore this. The necessary change is in v5.14-rc1.

Rob