[PATCH 0/2] GE Healthcare PPD firmware upgrade driver for ACHC

[PATCH 0/2] GE Healthcare PPD firmware upgrade driver for ACHC

[Sebastian Reichel]

Hi,

The PPD has a secondary processor from NXP, 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.

This adds a generic driver to handle firmware flashing via NXP EzPort
and adds a custom compatible for the instance found in GE's device.
The reset gpio is only requested during flashing, since it is also
used for resetting other devices. A better solution for the shared
reset gpio is required at some point, but this does neither affect
the userspace interface, nor the DT interface and should not block
upstreaming this driver.

-- Sebastian

Sebastian Reichel (2):
  misc: ezport-firmware: new driver
  ARM: dts: imx53: PPD: Update ACHC programming interface

 Documentation/devicetree/bindings/misc/ge-achc.txt |  19 +-
 arch/arm/boot/dts/imx53-ppd.dts                    |   5 +-
 drivers/misc/Kconfig                               |   9 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/ezport-firmware.c                     | 487 +++++++++++++++++++++
 5 files changed, 516 insertions(+), 5 deletions(-)
 create mode 100644 drivers/misc/ezport-firmware.c

-- 
2.16.2

[PATCH 1/2] misc: ezport-firmware: new driver

[Sebastian Reichel]

General Electric Healthcare's PPD has a secondary processor from
NXP's Kinetis K20 series. It's firmware can be updated from Linux
using the EzPort interface. This driver implements the firmware
updating process.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 Documentation/devicetree/bindings/misc/ge-achc.txt |  19 +-
 drivers/misc/Kconfig                               |   9 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/ezport-firmware.c                     | 487 +++++++++++++++++++++
 4 files changed, 513 insertions(+), 3 deletions(-)
 create mode 100644 drivers/misc/ezport-firmware.c

diff --git a/Documentation/devicetree/bindings/misc/ge-achc.txt b/Documentation/devicetree/bindings/misc/ge-achc.txt
index 77df94d7a32f..6c6bd6568504 100644
--- a/Documentation/devicetree/bindings/misc/ge-achc.txt
+++ b/Documentation/devicetree/bindings/misc/ge-achc.txt
@@ -7,7 +7,13 @@ Note: This device does not expose the peripherals as USB devices.
 
 Required properties:
 
-- compatible : Should be "ge,achc"
+- compatible : Should be
+  "ge,achc" (normal interface)
+  "ge,achc-ezport" (flashing interface)
+
+Required properties (flashing interface only):
+
+- reset-gpios: GPIO Specifier for the reset GPIO
 
 Required SPI properties:
 
@@ -19,8 +25,15 @@ Required SPI properties:
 
 Example:
 
-spidev0: spi@0 {
-	compatible = "ge,achc";
+spidev1: spi@0 {
+	compatible = "ge,achc-ezport";
 	reg = <0>;
+	spi-max-frequency = <300000>;
+	reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+};
+
+spidev0: spi@1 {
+	compatible = "ge,achc";
+	reg = <1>;
 	spi-max-frequency = <1000000>;
 };
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 03605f8fc0dc..841249111204 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -92,6 +92,15 @@ config DUMMY_IRQ
 	  The sole purpose of this module is to help with debugging of systems on
 	  which spurious IRQs would happen on disabled IRQ vector.
 
+config EZPORT_FW
+	tristate "Kinetis K20 EzPort firmware update support
+	depends on SPI && SYSFS && OF
+	select FW_LOADER
+	---help---
+	  EzPort is the flash interface from NXP Kinetis K20, that can be used to
+	  access the microcontroller's flash memory. This driver supports flashing
+	  a new image using the kernel's firmware API.
+
 config IBM_ASM
 	tristate "Device driver for IBM RSA service processor"
 	depends on X86 && PCI && INPUT
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c3c8624f4d95..61eb228d2a7b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_INTEL_MID_PTI)	+= pti.o
 obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
 obj-$(CONFIG_ATMEL_TCLIB)	+= atmel_tclib.o
 obj-$(CONFIG_DUMMY_IRQ)		+= dummy-irq.o
+obj-$(CONFIG_EZPORT_FW)		+= ezport-firmware.o
 obj-$(CONFIG_ICS932S401)	+= ics932s401.o
 obj-$(CONFIG_LKDTM)		+= lkdtm.o
 obj-$(CONFIG_TIFM_CORE)       	+= tifm_core.o
diff --git a/drivers/misc/ezport-firmware.c b/drivers/misc/ezport-firmware.c
new file mode 100644
index 000000000000..069981b13e69
--- /dev/null
+++ b/drivers/misc/ezport-firmware.c
@@ -0,0 +1,487 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NXP Kinetis EzPort is a flash interface found on NXP microcontrollers
+ * from the Kinetis K20 series. It can be used to update the firmware.
+ * The interface is enabled by enabling the chip-select while resetting
+ * the processor.
+ *
+ * Copyright (C) 2018 Collabora
+ * Copyright (C) 2018 GE Healthcare
+ */
+
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/of_device.h>
+
+#define EZPORT_SPI_RESET_DELAY_MS 50
+#define EZPORT_SPI_POST_RESET_DELAY_MS 500
+#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_DUMMY		0x00
+
+#define EZPORT_FAST_READ_SIZE	5
+#define EZPORT_SP_SIZE		4
+
+#define EZPORT_SECTOR_SIZE	4096
+#define EZPORT_SECTOR_MASK	(EZPORT_SECTOR_SIZE - 1)
+#define EZPORT_WRITE_WAIT_MS	50
+
+#define EZPORT_STATUS_WIP	BIT(0) /* write in progress */
+#define EZPORT_STATUS_WEN	BIT(1) /* write enable */
+#define EZPORT_STATUS_WEF	BIT(6) /* write error flag */
+#define EZPORT_STATUS_FS	BIT(7) /* flash security */
+
+struct ezport_device_info {
+	const char *fwname;
+	u32 flash_size;
+};
+
+/* https://www.nxp.com/part/MK20FN1M0VMD12 */
+static const struct ezport_device_info data_ge_achc = {
+	.fwname = "gehc-achc.fw",
+	.flash_size = 1024*1024,
+};
+
+static int ezport_acquire(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;
+
+	/* reset to return into normal mode */
+	gpiod_set_value(reset, 1);
+	msleep(EZPORT_SPI_RESET_DELAY_MS);
+	gpiod_set_value(reset, 0);
+
+	msleep(EZPORT_SPI_POST_RESET_DELAY_MS);
+
+	/* 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_release(struct gpio_desc *reset)
+{
+	/* reset without chip select to return into normal mode */
+	gpiod_set_value(reset, 1);
+	msleep(EZPORT_SPI_RESET_DELAY_MS);
+	gpiod_set_value(reset, 0);
+
+	msleep(EZPORT_SPI_POST_RESET_DELAY_MS);
+}
+
+static inline int ezport_get_status_register(struct spi_device *spi)
+{
+	return spi_w8r8(spi, EZPORT_CMD_RDSR);
+}
+
+static bool ezport_ready(int statusreg, bool write)
+{
+	if (statusreg < 0)
+		return false;
+	if (statusreg & EZPORT_STATUS_WIP)
+		return false;
+	if (statusreg & EZPORT_STATUS_WEF)
+		return false;
+	if (statusreg & EZPORT_STATUS_FS)
+		return false;
+	if (write && !(statusreg & EZPORT_STATUS_WEN))
+		return false;
+
+	return true;
+}
+
+static int ezport_wait_write(struct spi_device *spi)
+{
+	int ret;
+	u32 i;
+
+	/*
+	 * poll status for write in progress bit. It usually needs 100ms
+	 * to get cleared, but we wait 250ms to be sure.
+	 */
+	for (i = 0; i < 5; i++) {
+		ret = ezport_get_status_register(spi);
+		if (ret < 0)
+			return ret;
+		if (!(ret & EZPORT_STATUS_WIP))
+			break;
+		msleep(EZPORT_WRITE_WAIT_MS);
+	}
+
+	return ret;
+}
+
+static int ezport_write_enable(struct spi_device *spi)
+{
+	static const u8 cmd = EZPORT_CMD_WREN;
+	int ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	return ezport_get_status_register(spi);
+}
+
+static int ezport_bulk_erase(struct spi_device *spi)
+{
+	int ret;
+	static const u8 cmd = EZPORT_CMD_BE;
+
+	ret = ezport_write_enable(spi);
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & EZPORT_STATUS_WEN))
+		return -EIO;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = ezport_wait_write(spi);
+	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, address >> 8, address};
+	int ret;
+
+	if (address & EZPORT_SECTOR_MASK)
+		return -EINVAL;
+
+	ret = ezport_write_enable(spi);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_write(spi, query, ARRAY_SIZE(query));
+	if (ret < 0)
+		return ret;
+
+	return ezport_wait_write(spi);
+}
+
+static int ezport_flash_transfer(struct spi_device *spi, u32 address,
+				 const u8 *payload, size_t size)
+{
+	struct spi_transfer xfers[2] = {};
+	u8 *query;
+	int ret;
+
+	query = kmalloc(EZPORT_SP_SIZE, GFP_KERNEL);
+	if (!query)
+		return -ENOMEM;
+
+	query[0] = EZPORT_CMD_SP;
+	query[1] = address >> 16;
+	query[2] = address >> 8;
+	query[3] = address >> 0;
+
+	xfers[0].len = EZPORT_SP_SIZE;
+	xfers[0].tx_buf = query;
+
+	xfers[1].len = size;
+	xfers[1].tx_buf = payload;
+
+	ret = spi_sync_transfer(spi, xfers, 2);
+
+	kfree(query);
+
+	if (ret < 0)
+		return ret;
+
+	return ezport_wait_write(spi);
+}
+
+static int ezport_read_data(struct spi_device *spi, u32 address,
+			    u8 *buffer, size_t size)
+{
+	struct spi_transfer xfers[2] = {};
+	u8 *query = kmalloc(EZPORT_FAST_READ_SIZE, GFP_KERNEL);
+	int ret;
+
+	if (!query)
+		return -ENOMEM;
+
+	query[0] = EZPORT_CMD_FAST_READ;
+	query[1] = address >> 16;
+	query[2] = address >> 8;
+	query[3] = address >> 0;
+	query[4] = EZPORT_DUMMY;
+
+	xfers[0].len = EZPORT_FAST_READ_SIZE;
+	xfers[0].tx_buf = query;
+
+	xfers[1].len = size;
+	xfers[1].rx_buf = buffer;
+
+	ret = spi_sync_transfer(spi, xfers, 2);
+
+	kfree(query);
+
+	return ret;
+}
+
+static int ezport_firmware_flash_data(struct spi_device *spi,
+				      const u8 *data, size_t size)
+{
+	int ret;
+	u32 address = 0;
+	u32 transfer_size;
+
+	ret = ezport_get_status_register(spi);
+	if (ret < 0)
+		return ret;
+
+	if (ret & EZPORT_STATUS_FS) {
+		dev_dbg(&spi->dev, "bulk erase");
+		ret = ezport_bulk_erase(spi);
+		if (!ezport_ready(ret, false))
+			return ret;
+	}
+
+	ret = ezport_get_status_register(spi);
+	if (!ezport_ready(ret, false)) {
+		dev_err(&spi->dev, "flash memory is not ready!");
+		return ret;
+	}
+
+	while (address < size) {
+		if (!(address & EZPORT_SECTOR_MASK)) {
+			dev_dbg(&spi->dev, "section erase: %x", address);
+			ret = ezport_section_erase(spi, address);
+			if (!ezport_ready(ret, false))
+				return ret;
+		}
+
+		ret = ezport_write_enable(spi);
+		if (!ezport_ready(ret, true))
+			return ret;
+
+		transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
+
+		dev_dbg(&spi->dev, "transfer: %x", address);
+		ret = ezport_flash_transfer(spi, address,
+					    data+address, transfer_size);
+		if (!ezport_ready(ret, false))
+			return ret;
+
+		address += transfer_size;
+	}
+
+	return 0;
+}
+
+static int ezport_firmware_verify_data(struct spi_device *spi,
+				       const u8 *data, size_t size)
+{
+	u32 transfer_size;
+	u32 address = 0;
+	u8 *buffer;
+	int ret;
+
+	ret = ezport_get_status_register(spi);
+	if (!ezport_ready(ret, false)) {
+		dev_err(&spi->dev, "flash not ready: %d\n", ret);
+		return -EBUSY;
+	}
+
+	buffer = kmalloc(EZPORT_TRANSFER_SIZE, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	while (address < size) {
+		transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
+
+		ret = ezport_read_data(spi, address, buffer, transfer_size);
+		if (ret < 0)
+			goto exit_free_buffer;
+
+		ret = memcmp(data + address, buffer, transfer_size);
+		if (ret) {
+			ret = -EBADMSG;
+			goto exit_free_buffer;
+		}
+
+		address += transfer_size;
+	}
+
+exit_free_buffer:
+	kfree(buffer);
+	return ret;
+}
+
+static int ezport_flash(struct spi_device *spi, bool verify)
+{
+	struct ezport_device_info *data = spi_get_drvdata(spi);
+	const struct firmware *fw;
+	struct gpio_desc *reset;
+	int ret;
+
+	ret = request_firmware(&fw, data->fwname, &spi->dev);
+	if (ret) {
+		dev_err(&spi->dev, "Could not get firmware: %d\n", ret);
+		return ret;
+	}
+
+	if (fw->size > data->flash_size) {
+		dev_err(&spi->dev, "FW image does not fit into flash memory!\n");
+		ret = -EFBIG;
+		goto exit_release_fw;
+	}
+
+	reset = gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset)) {
+		ret = PTR_ERR(reset);
+		dev_err(&spi->dev, "Could not get reset gpio: %d\n", ret);
+		goto exit_release_fw;
+	}
+
+	ret = ezport_acquire(spi, reset);
+	if (ret)
+		goto exit_release_gpio;
+
+	if (verify)
+		ret = ezport_firmware_verify_data(spi, fw->data, fw->size);
+	else
+		ret = ezport_firmware_flash_data(spi, fw->data, fw->size);
+
+	if (ret > 0) {
+		dev_err(&spi->dev, "Failure, Status Register: 0x%02x\n", ret);
+		ret = -EIO;
+	}
+
+	ezport_release(reset);
+
+exit_release_gpio:
+	gpiod_put(reset);
+exit_release_fw:
+	release_firmware(fw);
+	return ret;
+}
+
+static ssize_t verify_fw_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	static const char *success = "Firmware matches!\n";
+	static const char *failure = "Firmware does not match!\n";
+	struct spi_device *spi = to_spi_device(dev);
+	int ret = ezport_flash(spi, true);
+
+	if (ret == 0) {
+		strcpy(buf, success);
+		return strlen(success);
+	} else if (ret == -EBADMSG) {
+		strcpy(buf, failure);
+		return strlen(failure);
+	} else {
+		return ret;
+	}
+}
+
+static ssize_t update_fw_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	int ret;
+
+	dev_dbg(dev, "Firmware update started!");
+	ret = ezport_flash(spi, false);
+	if (!ret)
+		dev_dbg(dev, "Firmware update finished!");
+
+	return ret ? ret : count;
+}
+
+static DEVICE_ATTR_WO(update_fw);
+static DEVICE_ATTR_RO(verify_fw);
+
+static struct attribute *ezport_attrs[] = {
+	&dev_attr_update_fw.attr,
+	&dev_attr_verify_fw.attr,
+	NULL
+};
+
+static const struct attribute_group ezport_attr_group = {
+	.attrs = ezport_attrs,
+};
+
+static int ezport_probe(struct spi_device *spi)
+{
+	const struct ezport_device_info *data;
+	struct ezport_device_info *copy;
+	int ret;
+
+	data = of_device_get_match_data(&spi->dev);
+	if (!data)
+		return -ENODEV;
+
+	copy = devm_kmemdup(&spi->dev, data, sizeof(*data), GFP_KERNEL);
+	if (!copy)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, copy);
+
+	ret = devm_device_add_group(&spi->dev, &ezport_attr_group);
+	if (ret) {
+		dev_err(&spi->dev, "create sysfs failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ezport_of_match[] = {
+	{ .compatible = "ge,achc-ezport", .data = &data_ge_achc, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ezport_of_match);
+
+static struct spi_driver ezport_fw_spi_driver = {
+	.driver = {
+		.name	= "ezport-fw",
+		.of_match_table = ezport_of_match,
+	},
+	.probe		= ezport_probe,
+};
+module_spi_driver(ezport_fw_spi_driver);
+
+MODULE_DESCRIPTION("Kinetis EzPort firmware driver");
+MODULE_AUTHOR("Sebastian Reichel <sebastian.reichel@collabora.co.uk>");
+MODULE_LICENSE("GPL");
-- 
2.16.2

[PATCH 2/2] ARM: dts: imx53: PPD: Update ACHC programming interface

[Sebastian Reichel]

Update ACHC programming interface description following the binding
document. This makes it possible to use the new EzPort firmware
update driver.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 arch/arm/boot/dts/imx53-ppd.dts | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts
index 3c87e0d73f63..03c353be2fe7 100644
--- a/arch/arm/boot/dts/imx53-ppd.dts
+++ b/arch/arm/boot/dts/imx53-ppd.dts
@@ -253,9 +253,10 @@
 	status = "okay";
 
 	spidev0: spi@0 {
-		compatible = "ge,achc";
+		compatible = "ge,achc-ezport";
 		reg = <0>;
-		spi-max-frequency = <1000000>;
+		spi-max-frequency = <300000>;
+		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
 	};
 
 	spidev1: spi@1 {
-- 
2.16.2

Re: [PATCH 1/2] misc: ezport-firmware: new driver

[kbuild test robot]

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on shawnguo/for-next]
[also build test WARNING on v4.16-rc6 next-20180322]
[cannot apply to char-misc/char-misc-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20180323-144836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/misc/ezport-firmware.c:296:33: sparse: incompatible types in comparison expression (different type sizes)
   drivers/misc/ezport-firmware.c:329:33: sparse: incompatible types in comparison expression (different type sizes)
   In file included from include/linux/delay.h:22:0,
                    from drivers/misc/ezport-firmware.c:12:
   drivers/misc/ezport-firmware.c: In function 'ezport_firmware_flash_data':
   include/linux/kernel.h:792:16: warning: comparison of distinct pointer types lacks a cast
     (void) (&min1 == &min2);   8-                ^
   include/linux/kernel.h:801:2: note: in expansion of macro '__min'
     __min(typeof(x), typeof(y),   11-  ^~~~~
   drivers/misc/ezport-firmware.c:296:19: note: in expansion of macro 'min'
      transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
                      ^~~
   drivers/misc/ezport-firmware.c: In function 'ezport_firmware_verify_data':
   include/linux/kernel.h:792:16: warning: comparison of distinct pointer types lacks a cast
     (void) (&min1 == &min2);   18-                ^
   include/linux/kernel.h:801:2: note: in expansion of macro '__min'
     __min(typeof(x), typeof(y),   21-  ^~~~~
   drivers/misc/ezport-firmware.c:329:19: note: in expansion of macro 'min'
      transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
                      ^~~

vim +296 drivers/misc/ezport-firmware.c

   259	
   260	static int ezport_firmware_flash_data(struct spi_device *spi,
   261					      const u8 *data, size_t size)
   262	{
   263		int ret;
   264		u32 address = 0;
   265		u32 transfer_size;
   266	
   267		ret = ezport_get_status_register(spi);
   268		if (ret < 0)
   269			return ret;
   270	
   271		if (ret & EZPORT_STATUS_FS) {
   272			dev_dbg(&spi->dev, "bulk erase");
   273			ret = ezport_bulk_erase(spi);
   274			if (!ezport_ready(ret, false))
   275				return ret;
   276		}
   277	
   278		ret = ezport_get_status_register(spi);
   279		if (!ezport_ready(ret, false)) {
   280			dev_err(&spi->dev, "flash memory is not ready!");
   281			return ret;
   282		}
   283	
   284		while (address < size) {
   285			if (!(address & EZPORT_SECTOR_MASK)) {
   286				dev_dbg(&spi->dev, "section erase: %x", address);
   287				ret = ezport_section_erase(spi, address);
   288				if (!ezport_ready(ret, false))
   289					return ret;
   290			}
   291	
   292			ret = ezport_write_enable(spi);
   293			if (!ezport_ready(ret, true))
   294				return ret;
   295	
 > 296			transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
   297	
   298			dev_dbg(&spi->dev, "transfer: %x", address);
   299			ret = ezport_flash_transfer(spi, address,
   300						    data+address, transfer_size);
   301			if (!ezport_ready(ret, false))
   302				return ret;
   303	
   304			address += transfer_size;
   305		}
   306	
   307		return 0;
   308	}
   309	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 1/2] misc: ezport-firmware: new driver

[kbuild test robot]

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on shawnguo/for-next]
[also build test WARNING on v4.16-rc6 next-20180322]
[cannot apply to char-misc/char-misc-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20180323-144836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/misc/ezport-firmware.c:296:33: sparse: incompatible types in comparison expression (different type sizes)
   drivers/misc/ezport-firmware.c:329:33: sparse: incompatible types in comparison expression (different type sizes)
   In file included from include/linux/delay.h:22:0,
                    from drivers/misc/ezport-firmware.c:12:
   drivers/misc/ezport-firmware.c: In function 'ezport_firmware_flash_data':
   include/linux/kernel.h:792:16: warning: comparison of distinct pointer types lacks a cast
     (void) (&min1 == &min2);   8-                ^
   include/linux/kernel.h:801:2: note: in expansion of macro '__min'
     __min(typeof(x), typeof(y),   11-  ^~~~~
   drivers/misc/ezport-firmware.c:296:19: note: in expansion of macro 'min'
      transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
                      ^~~
   drivers/misc/ezport-firmware.c: In function 'ezport_firmware_verify_data':
   include/linux/kernel.h:792:16: warning: comparison of distinct pointer types lacks a cast
     (void) (&min1 == &min2);   18-                ^
   include/linux/kernel.h:801:2: note: in expansion of macro '__min'
     __min(typeof(x), typeof(y),   21-  ^~~~~
   drivers/misc/ezport-firmware.c:329:19: note: in expansion of macro 'min'
      transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
                      ^~~

vim +296 drivers/misc/ezport-firmware.c

   259	
   260	static int ezport_firmware_flash_data(struct spi_device *spi,
   261					      const u8 *data, size_t size)
   262	{
   263		int ret;
   264		u32 address = 0;
   265		u32 transfer_size;
   266	
   267		ret = ezport_get_status_register(spi);
   268		if (ret < 0)
   269			return ret;
   270	
   271		if (ret & EZPORT_STATUS_FS) {
   272			dev_dbg(&spi->dev, "bulk erase");
   273			ret = ezport_bulk_erase(spi);
   274			if (!ezport_ready(ret, false))
   275				return ret;
   276		}
   277	
   278		ret = ezport_get_status_register(spi);
   279		if (!ezport_ready(ret, false)) {
   280			dev_err(&spi->dev, "flash memory is not ready!");
   281			return ret;
   282		}
   283	
   284		while (address < size) {
   285			if (!(address & EZPORT_SECTOR_MASK)) {
   286				dev_dbg(&spi->dev, "section erase: %x", address);
   287				ret = ezport_section_erase(spi, address);
   288				if (!ezport_ready(ret, false))
   289					return ret;
   290			}
   291	
   292			ret = ezport_write_enable(spi);
   293			if (!ezport_ready(ret, true))
   294				return ret;
   295	
 > 296			transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
   297	
   298			dev_dbg(&spi->dev, "transfer: %x", address);
   299			ret = ezport_flash_transfer(spi, address,
   300						    data+address, transfer_size);
   301			if (!ezport_ready(ret, false))
   302				return ret;
   303	
   304			address += transfer_size;
   305		}
   306	
   307		return 0;
   308	}
   309	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 1/2] misc: ezport-firmware: new driver

[Greg Kroah-Hartman]

On Tue, Mar 20, 2018 at 06:22:00PM +0100, Sebastian Reichel wrote:
> General Electric Healthcare's PPD has a secondary processor from
> NXP's Kinetis K20 series. It's firmware can be updated from Linux
> using the EzPort interface. This driver implements the firmware
> updating process.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/misc/ge-achc.txt |  19 +-

Bindings should be in a separate patch, right?

> +static int ezport_read_data(struct spi_device *spi, u32 address,
> +			    u8 *buffer, size_t size)
> +{
> +	struct spi_transfer xfers[2] = {};

You can send SPI data from the stack?

> +	u8 *query = kmalloc(EZPORT_FAST_READ_SIZE, GFP_KERNEL);
> +	int ret;
> +
> +	if (!query)
> +		return -ENOMEM;
> +
> +	query[0] = EZPORT_CMD_FAST_READ;
> +	query[1] = address >> 16;
> +	query[2] = address >> 8;
> +	query[3] = address >> 0;
> +	query[4] = EZPORT_DUMMY;
> +
> +	xfers[0].len = EZPORT_FAST_READ_SIZE;
> +	xfers[0].tx_buf = query;
> +
> +	xfers[1].len = size;
> +	xfers[1].rx_buf = buffer;
> +
> +	ret = spi_sync_transfer(spi, xfers, 2);
> +
> +	kfree(query);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_WO(update_fw);
> +static DEVICE_ATTR_RO(verify_fw);

New sysfs attributes need Documentation/ABI updates please.

Also please fix up the kbuild-reported build errors.

thanks,

greg k-h

Re: [PATCH 1/2] misc: ezport-firmware: new driver

[Sebastian Reichel]

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

Hi,

On Fri, Mar 23, 2018 at 02:17:12PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 20, 2018 at 06:22:00PM +0100, Sebastian Reichel wrote:
> > General Electric Healthcare's PPD has a secondary processor from
> > NXP's Kinetis K20 series. It's firmware can be updated from Linux
> > using the EzPort interface. This driver implements the firmware
> > updating process.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  Documentation/devicetree/bindings/misc/ge-achc.txt |  19 +-
> 
> Bindings should be in a separate patch, right?

Right.

> > +static int ezport_read_data(struct spi_device *spi, u32 address,
> > +			    u8 *buffer, size_t size)
> > +{
> > +	struct spi_transfer xfers[2] = {};
> 
> You can send SPI data from the stack?

I don't think so:

/**
 * struct spi_transfer - a read/write buffer pair
 * @tx_buf: data to be written (dma-safe memory), or NULL
 * @rx_buf: data to be read (dma-safe memory), or NULL

IIUIC only spi_write_then_read() may use the stack, since the
function copies the provided buffer to a second, DMA capable
buffer. That function is obviously not good for big buffers,
though.

> > +	u8 *query = kmalloc(EZPORT_FAST_READ_SIZE, GFP_KERNEL);
> > +	int ret;
> > +
> > +	if (!query)
> > +		return -ENOMEM;
> > +
> > +	query[0] = EZPORT_CMD_FAST_READ;
> > +	query[1] = address >> 16;
> > +	query[2] = address >> 8;
> > +	query[3] = address >> 0;
> > +	query[4] = EZPORT_DUMMY;
> > +
> > +	xfers[0].len = EZPORT_FAST_READ_SIZE;
> > +	xfers[0].tx_buf = query;
> > +
> > +	xfers[1].len = size;
> > +	xfers[1].rx_buf = buffer;
> > +
> > +	ret = spi_sync_transfer(spi, xfers, 2);
> > +
> > +	kfree(query);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_WO(update_fw);
> > +static DEVICE_ATTR_RO(verify_fw);
> 
> New sysfs attributes need Documentation/ABI updates please.

Ok.

> Also please fix up the kbuild-reported build errors.

Of course.

> thanks,

Thanks for the review,

-- Sebastian

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