[PATCH v2 00/22] Ziirave_wdt driver fixes

[PATCH v2 00/22] Ziirave_wdt driver fixes

[Andrey Smirnov]

Everyone,

This series contains various fixes/improvements for ziirave_wdt
driver. Hopefully each commit is self-explanatory.

Feedback is welcome!

Thanks,
Andrey Smirnov

Changes since [v1]:

    - Collected Reviewied-by from Guenter

    - Added two more error messages to "watchdog: ziirave_wdt: Be more
      verbose during firmware update" (Guenter, I kept your
      Reviewied-by there since that change seemed trivial enough)

    - "watchdog: ziirave_wdt: Don't bail out on unexpected timeout
      value" changed to select a default timeout of 30 seconds if read
      out timeout value is out of valid range

    - Additional fixes I wrote while working with watchdog firmware
      added to the series


Andrey Smirnov (22):
  watchdog: ziirave_wdt: Add missing newline
  watchdog: ziirave_wdt: Be verbose about errors in probe()
  watchdog: ziirave_wdt: Be more verbose during firmware update
  watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
  watchdog: ziirave_wdt: Log bootloader/firmware info during probe
  watchdog: ziirave_wdt: Simplify ziirave_firm_write_pkt()
  watchdog: ziirave_wdt: Check packet length only once
  watchdog: ziirave_wdt: Skip zeros when calculating checksum
  watchdog: ziirave_wdt: Fix incorrect use of ARRAY_SIZE
  watchdog: ziirave_wdt: Zero out only what's necessary
  watchdog: ziirave_wdt: Make use of put_unaligned_le16
  watchdog: ziirave_wdt: Don't check if ihex record length is zero
  watchdog: ziirave_wdt: Don't read out more than 'len' firmware bytes
  watchdog: ziirave_wdt: Don't try to program readonly flash
  watchdog: ziirave_wdt: Fix misleading error message
  watchdog: ziirave_wdt: Fix JUMP_TO_BOOTLOADER payload
  watchdog: ziirave_wdt: Fix DOWNLOAD_END payload
  watchdog: ziirave_wdt: Fix RESET_PROCESSOR payload
  watchdog: ziirave_wdt: Drop status polling code
  watchdog: ziirave_wdt: Fix DOWNLOAD_START payload
  watchdog: ziirave_wdt: Drop ziirave_firm_write_block_data()
  watchdog: ziirave_wdt: Update checked I2C functionality mask

 drivers/watchdog/ziirave_wdt.c | 351 ++++++++++++++++-----------------
 1 file changed, 173 insertions(+), 178 deletions(-)

-- 
2.21.0

[PATCH v2 01/22] watchdog: ziirave_wdt: Add missing newline

[Andrey Smirnov]

Add missing newline.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index dec660c509b3..6ec028fb2635 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -671,7 +671,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		if (ret)
 			return ret;
 
-		dev_info(&client->dev, "Timeout set to %ds.",
+		dev_info(&client->dev, "Timeout set to %ds\n",
 			 w_priv->wdd.timeout);
 	}
 
-- 
2.21.0

[PATCH v2 02/22] watchdog: ziirave_wdt: Be verbose about errors in probe()

[Andrey Smirnov]

The driver is quite silent in case of probe failure, which makes it
more difficult to diagnose problem from the kernel log. Add logging to
all of the silent error paths ziirave_wdt_probe() to improve that.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 6ec028fb2635..8c71341a9c1c 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -658,8 +658,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	 */
 	if (w_priv->wdd.timeout == 0) {
 		val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT);
-		if (val < 0)
+		if (val < 0) {
+			dev_err(&client->dev, "Failed to read timeout\n");
 			return val;
+		}
 
 		if (val < ZIIRAVE_TIMEOUT_MIN)
 			return -ENODEV;
@@ -668,8 +670,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	} else {
 		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
 					      w_priv->wdd.timeout);
-		if (ret)
+		if (ret) {
+			dev_err(&client->dev, "Failed to set timeout\n");
 			return ret;
+		}
 
 		dev_info(&client->dev, "Timeout set to %ds\n",
 			 w_priv->wdd.timeout);
@@ -681,34 +685,46 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 
 	/* If in unconfigured state, set to stopped */
 	val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE);
-	if (val < 0)
+	if (val < 0) {
+		dev_err(&client->dev, "Failed to read state\n");
 		return val;
+	}
 
 	if (val == ZIIRAVE_STATE_INITIAL)
 		ziirave_wdt_stop(&w_priv->wdd);
 
 	ret = ziirave_wdt_init_duration(client);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to init duration\n");
 		return ret;
+	}
 
 	ret = ziirave_wdt_revision(client, &w_priv->firmware_rev,
 				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to read firmware version\n");
 		return ret;
+	}
 
 	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
 				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to read bootloader version\n");
 		return ret;
+	}
 
 	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
 						ZIIRAVE_WDT_RESET_REASON);
-	if (w_priv->reset_reason < 0)
+	if (w_priv->reset_reason < 0) {
+		dev_err(&client->dev, "Failed to read reset reason\n");
 		return w_priv->reset_reason;
+	}
 
 	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons) ||
-	    !ziirave_reasons[w_priv->reset_reason])
+	    !ziirave_reasons[w_priv->reset_reason]) {
+		dev_err(&client->dev, "Invalid reset reason\n");
 		return -ENODEV;
+	}
 
 	ret = watchdog_register_device(&w_priv->wdd);
 
-- 
2.21.0

[PATCH v2 03/22] watchdog: ziirave_wdt: Be more verbose during firmware update

[Andrey Smirnov]

Add more error logging to ziirave_firm_upload() for diagnostics.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 8c71341a9c1c..b3e255b40209 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -335,14 +335,18 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER, 1,
 				      false);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to jump to bootloader\n");
 		return ret;
+	}
 
 	msleep(500);
 
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1, true);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to start download\n");
 		return ret;
+	}
 
 	msleep(500);
 
@@ -438,14 +442,20 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 
 	/* End download operation */
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_END, 1, false);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to end firmware download: %d\n", ret);
 		return ret;
+	}
 
 	/* Reset the processor */
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_RESET_PROCESSOR, 1,
 				      false);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to reset the watchdog: %d\n", ret);
 		return ret;
+	}
 
 	msleep(500);
 
-- 
2.21.0

[PATCH v2 04/22] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value

[Andrey Smirnov]

Reprogramming bootloader on watchdog MCU will result in reported
default timeout value of "0". That in turn will be unnecessarily
rejected by the driver as invalid device (-ENODEV). Simplify probe to
read stored timeout value, set it to a sane default if it is bogus,
and then program that value unconditionally.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index b3e255b40209..a11b92383c5f 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -23,6 +23,7 @@
 
 #define ZIIRAVE_TIMEOUT_MIN	3
 #define ZIIRAVE_TIMEOUT_MAX	255
+#define ZIIRAVE_TIMEOUT_DEFAULT	30
 
 #define ZIIRAVE_PING_VALUE	0x0
 
@@ -673,22 +674,21 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 			return val;
 		}
 
-		if (val < ZIIRAVE_TIMEOUT_MIN)
-			return -ENODEV;
+		if (val > ZIIRAVE_TIMEOUT_MAX ||
+		    val < ZIIRAVE_TIMEOUT_MIN)
+			val = ZIIRAVE_TIMEOUT_DEFAULT;
 
 		w_priv->wdd.timeout = val;
-	} else {
-		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
-					      w_priv->wdd.timeout);
-		if (ret) {
-			dev_err(&client->dev, "Failed to set timeout\n");
-			return ret;
-		}
+	}
 
-		dev_info(&client->dev, "Timeout set to %ds\n",
-			 w_priv->wdd.timeout);
+	ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout);
+	if (ret) {
+		dev_err(&client->dev, "Failed to set timeout\n");
+		return ret;
 	}
 
+	dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout);
+
 	watchdog_set_nowayout(&w_priv->wdd, nowayout);
 
 	i2c_set_clientdata(client, w_priv);
-- 
2.21.0

Re: [PATCH v2 04/22] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:48PM -0700, Andrey Smirnov wrote:
> Reprogramming bootloader on watchdog MCU will result in reported
> default timeout value of "0". That in turn will be unnecessarily
> rejected by the driver as invalid device (-ENODEV). Simplify probe to
> read stored timeout value, set it to a sane default if it is bogus,
> and then program that value unconditionally.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 05/22] watchdog: ziirave_wdt: Log bootloader/firmware info during probe

[Andrey Smirnov]

Log bootloader/firmware info during probe. This information is
available via sysfs already, but it's really helpful to have this in
kernel log during startup as well.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index a11b92383c5f..75c066602c00 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -69,6 +69,9 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 #define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER		0x0c
 #define ZIIRAVE_CMD_DOWNLOAD_PACKET		0x0e
 
+#define ZIIRAVE_FW_VERSION_FMT	"02.%02u.%02u"
+#define ZIIRAVE_BL_VERSION_FMT	"01.%02u.%02u"
+
 struct ziirave_wdt_rev {
 	unsigned char major;
 	unsigned char minor;
@@ -489,7 +492,7 @@ static ssize_t ziirave_wdt_sysfs_show_firm(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = sprintf(buf, "02.%02u.%02u", w_priv->firmware_rev.major,
+	ret = sprintf(buf, ZIIRAVE_FW_VERSION_FMT, w_priv->firmware_rev.major,
 		      w_priv->firmware_rev.minor);
 
 	mutex_unlock(&w_priv->sysfs_mutex);
@@ -512,7 +515,7 @@ static ssize_t ziirave_wdt_sysfs_show_boot(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = sprintf(buf, "01.%02u.%02u", w_priv->bootloader_rev.major,
+	ret = sprintf(buf, ZIIRAVE_BL_VERSION_FMT, w_priv->bootloader_rev.major,
 		      w_priv->bootloader_rev.minor);
 
 	mutex_unlock(&w_priv->sysfs_mutex);
@@ -579,7 +582,8 @@ static ssize_t ziirave_wdt_sysfs_store_firm(struct device *dev,
 		goto unlock_mutex;
 	}
 
-	dev_info(&client->dev, "Firmware updated to version 02.%02u.%02u\n",
+	dev_info(&client->dev,
+		 "Firmware updated to version " ZIIRAVE_FW_VERSION_FMT "\n",
 		 w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
 
 	/* Restore the watchdog timeout */
@@ -716,6 +720,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	dev_info(&client->dev,
+		 "Firmware version: " ZIIRAVE_FW_VERSION_FMT "\n",
+		 w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
+
 	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
 				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
 	if (ret) {
@@ -723,6 +731,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	dev_info(&client->dev,
+		 "Bootloader version: " ZIIRAVE_BL_VERSION_FMT "\n",
+		 w_priv->bootloader_rev.major, w_priv->bootloader_rev.minor);
+
 	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
 						ZIIRAVE_WDT_RESET_REASON);
 	if (w_priv->reset_reason < 0) {
-- 
2.21.0

[PATCH v2 06/22] watchdog: ziirave_wdt: Simplify ziirave_firm_write_pkt()

[Andrey Smirnov]

There no reason why ziirave_firm_write_pkt() has to take firmware
data via 'struct ihex_binrec' and it can just take address, data pointer
and data length as individual arguments. Make this change to allow us
to drastically simplify handling page crossing case by removing all of
the extra code required to split 'struct ihex_binrec' into two.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 116 +++++++++++++--------------------
 1 file changed, 44 insertions(+), 72 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 75c066602c00..b2d5ff0c22fe 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -51,6 +51,7 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 #define ZIIRAVE_FIRM_PKT_DATA_SIZE	16
 #define ZIIRAVE_FIRM_FLASH_MEMORY_START	0x1600
 #define ZIIRAVE_FIRM_FLASH_MEMORY_END	0x2bbf
+#define ZIIRAVE_FIRM_PAGE_SIZE		128
 
 /* Received and ready for next Download packet. */
 #define ZIIRAVE_FIRM_DOWNLOAD_ACK	1
@@ -244,27 +245,26 @@ static int ziirave_firm_write_byte(struct watchdog_device *wdd, u8 command,
  *     Data0 .. Data15: Array of 16 bytes of data.
  *     Checksum: Checksum byte to verify data integrity.
  */
-static int ziirave_firm_write_pkt(struct watchdog_device *wdd,
-				  const struct ihex_binrec *rec)
+static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
+				    u32 addr, const u8 *data, u8 len)
 {
+	const u16 addr16 = (u16)addr / 2;
 	struct i2c_client *client = to_i2c_client(wdd->parent);
 	u8 i, checksum = 0, packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE];
 	int ret;
-	u16 addr;
 
 	memset(packet, 0, ARRAY_SIZE(packet));
 
 	/* Packet length */
-	packet[0] = (u8)be16_to_cpu(rec->len);
+	packet[0] = len;
 	/* Packet address */
-	addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1;
-	packet[1] = addr & 0xff;
-	packet[2] = (addr & 0xff00) >> 8;
+	packet[1] = addr16 & 0xff;
+	packet[2] = (addr16 & 0xff00) >> 8;
 
 	/* Packet data */
-	if (be16_to_cpu(rec->len) > ZIIRAVE_FIRM_PKT_DATA_SIZE)
+	if (len > ZIIRAVE_FIRM_PKT_DATA_SIZE)
 		return -EMSGSIZE;
-	memcpy(packet + 3, rec->data, be16_to_cpu(rec->len));
+	memcpy(packet + 3, data, len);
 
 	/* Packet checksum */
 	for (i = 0; i < ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1; i++)
@@ -276,11 +276,35 @@ static int ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	if (ret)
 		dev_err(&client->dev,
 		      "Failed to write firmware packet at address 0x%04x: %d\n",
-		      addr, ret);
+		      addr16, ret);
 
 	return ret;
 }
 
+static int ziirave_firm_write_pkt(struct watchdog_device *wdd,
+				  u32 addr, const u8 *data, u8 len)
+{
+	const u8 max_write_len = ZIIRAVE_FIRM_PAGE_SIZE -
+		(addr - ALIGN_DOWN(addr, ZIIRAVE_FIRM_PAGE_SIZE));
+	int ret;
+
+	if (len > max_write_len) {
+		/*
+		 * If data crossed page boundary we need to split this
+		 * write in two
+		 */
+		ret = __ziirave_firm_write_pkt(wdd, addr, data, max_write_len);
+		if (ret)
+			return ret;
+
+		addr += max_write_len;
+		data += max_write_len;
+		len  -= max_write_len;
+	}
+
+	return __ziirave_firm_write_pkt(wdd, addr, data, len);
+}
+
 static int ziirave_firm_verify(struct watchdog_device *wdd,
 			       const struct firmware *fw)
 {
@@ -333,9 +357,8 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 			       const struct firmware *fw)
 {
 	struct i2c_client *client = to_i2c_client(wdd->parent);
-	int ret, words_till_page_break;
 	const struct ihex_binrec *rec;
-	struct ihex_binrec *rec_new;
+	int ret;
 
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER, 1,
 				      false);
@@ -366,68 +389,17 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 			return -EMSGSIZE;
 		}
 
-		/* Calculate words till page break */
-		words_till_page_break = (64 - ((be32_to_cpu(rec->addr) >> 1) &
-					 0x3f));
-		if ((be16_to_cpu(rec->len) >> 1) > words_till_page_break) {
-			/*
-			 * Data in passes page boundary, so we need to split in
-			 * two blocks of data. Create a packet with the first
-			 * block of data.
-			 */
-			rec_new = kzalloc(sizeof(struct ihex_binrec) +
-					  (words_till_page_break << 1),
-					  GFP_KERNEL);
-			if (!rec_new)
-				return -ENOMEM;
-
-			rec_new->len = cpu_to_be16(words_till_page_break << 1);
-			rec_new->addr = rec->addr;
-			memcpy(rec_new->data, rec->data,
-			       be16_to_cpu(rec_new->len));
-
-			ret = ziirave_firm_write_pkt(wdd, rec_new);
-			kfree(rec_new);
-			if (ret)
-				return ret;
-
-			/* Create a packet with the second block of data */
-			rec_new = kzalloc(sizeof(struct ihex_binrec) +
-					  be16_to_cpu(rec->len) -
-					  (words_till_page_break << 1),
-					  GFP_KERNEL);
-			if (!rec_new)
-				return -ENOMEM;
-
-			/* Remaining bytes */
-			rec_new->len = rec->len -
-				       cpu_to_be16(words_till_page_break << 1);
-
-			rec_new->addr = cpu_to_be32(be32_to_cpu(rec->addr) +
-					(words_till_page_break << 1));
-
-			memcpy(rec_new->data,
-			       rec->data + (words_till_page_break << 1),
-			       be16_to_cpu(rec_new->len));
-
-			ret = ziirave_firm_write_pkt(wdd, rec_new);
-			kfree(rec_new);
-			if (ret)
-				return ret;
-		} else {
-			ret = ziirave_firm_write_pkt(wdd, rec);
-			if (ret)
-				return ret;
-		}
+		ret = ziirave_firm_write_pkt(wdd, be32_to_cpu(rec->addr),
+					     rec->data, be16_to_cpu(rec->len));
+		if (ret)
+			return ret;
 	}
 
-	/* For end of download, the length field will be set to 0 */
-	rec_new = kzalloc(sizeof(struct ihex_binrec) + 1, GFP_KERNEL);
-	if (!rec_new)
-		return -ENOMEM;
-
-	ret = ziirave_firm_write_pkt(wdd, rec_new);
-	kfree(rec_new);
+	/*
+	 * Finish firmware download process by sending a zero length
+	 * payload
+	 */
+	ret = ziirave_firm_write_pkt(wdd, 0, NULL, 0);
 	if (ret) {
 		dev_err(&client->dev, "Failed to send EMPTY packet: %d\n", ret);
 		return ret;
-- 
2.21.0

Re: [PATCH v2 06/22] watchdog: ziirave_wdt: Simplify ziirave_firm_write_pkt()

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:50PM -0700, Andrey Smirnov wrote:
> There no reason why ziirave_firm_write_pkt() has to take firmware
> data via 'struct ihex_binrec' and it can just take address, data pointer
> and data length as individual arguments. Make this change to allow us
> to drastically simplify handling page crossing case by removing all of
> the extra code required to split 'struct ihex_binrec' into two.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 07/22] watchdog: ziirave_wdt: Check packet length only once

[Andrey Smirnov]

We don't need to check for packet length more than once, so drop the
extra check in ziirave_firm_upload(). While at it move the check at
the very start of __ziirave_firm_write_pkt(), as to not waste any time
preparing a packet we'll never use.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index b2d5ff0c22fe..9d9c669b8b47 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -253,6 +253,13 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	u8 i, checksum = 0, packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE];
 	int ret;
 
+	/* Check max data size */
+	if (len > ZIIRAVE_FIRM_PKT_DATA_SIZE) {
+		dev_err(&client->dev, "Firmware packet too long (%d)\n",
+			len);
+		return -EMSGSIZE;
+	}
+
 	memset(packet, 0, ARRAY_SIZE(packet));
 
 	/* Packet length */
@@ -261,9 +268,6 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	packet[1] = addr16 & 0xff;
 	packet[2] = (addr16 & 0xff00) >> 8;
 
-	/* Packet data */
-	if (len > ZIIRAVE_FIRM_PKT_DATA_SIZE)
-		return -EMSGSIZE;
 	memcpy(packet + 3, data, len);
 
 	/* Packet checksum */
@@ -382,13 +386,6 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 		if (!be16_to_cpu(rec->len))
 			break;
 
-		/* Check max data size */
-		if (be16_to_cpu(rec->len) > ZIIRAVE_FIRM_PKT_DATA_SIZE) {
-			dev_err(&client->dev, "Firmware packet too long (%d)\n",
-				be16_to_cpu(rec->len));
-			return -EMSGSIZE;
-		}
-
 		ret = ziirave_firm_write_pkt(wdd, be32_to_cpu(rec->addr),
 					     rec->data, be16_to_cpu(rec->len));
 		if (ret)
-- 
2.21.0

Re: [PATCH v2 07/22] watchdog: ziirave_wdt: Check packet length only once

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:51PM -0700, Andrey Smirnov wrote:
> We don't need to check for packet length more than once, so drop the
> extra check in ziirave_firm_upload(). While at it move the check at
> the very start of __ziirave_firm_write_pkt(), as to not waste any time
> preparing a packet we'll never use.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 08/22] watchdog: ziirave_wdt: Skip zeros when calculating checksum

[Andrey Smirnov]

Zeros don't contribute anything to checksum value, so we can skip
unused portion of the packet when calculating its checksum.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 9d9c669b8b47..19da0910c2d1 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -271,7 +271,7 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	memcpy(packet + 3, data, len);
 
 	/* Packet checksum */
-	for (i = 0; i < ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1; i++)
+	for (i = 0; i < len + 3; i++)
 		checksum += packet[i];
 	packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1] = checksum;
 
-- 
2.21.0

Re: [PATCH v2 08/22] watchdog: ziirave_wdt: Skip zeros when calculating checksum

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:52PM -0700, Andrey Smirnov wrote:
> Zeros don't contribute anything to checksum value, so we can skip
> unused portion of the packet when calculating its checksum.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 09/22] watchdog: ziirave_wdt: Fix incorrect use of ARRAY_SIZE

[Andrey Smirnov]

Both memset() and ziirave_firm_write_block_data() expect length in
bytes as an argument, not a number of elements in array. It just
happens that in this particular case both values are equal. Modify the
code to use sizeof() instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 19da0910c2d1..e0f55cbdc603 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -203,7 +203,7 @@ static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u16 addr)
 
 	return i2c_smbus_write_block_data(client,
 					  ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR,
-					  ARRAY_SIZE(address), address);
+					  sizeof(address), address);
 }
 
 static int ziirave_firm_write_block_data(struct watchdog_device *wdd,
@@ -260,7 +260,7 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 		return -EMSGSIZE;
 	}
 
-	memset(packet, 0, ARRAY_SIZE(packet));
+	memset(packet, 0, sizeof(packet));
 
 	/* Packet length */
 	packet[0] = len;
@@ -276,7 +276,7 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1] = checksum;
 
 	ret = ziirave_firm_write_block_data(wdd, ZIIRAVE_CMD_DOWNLOAD_PACKET,
-					    ARRAY_SIZE(packet), packet, true);
+					    sizeof(packet), packet, true);
 	if (ret)
 		dev_err(&client->dev,
 		      "Failed to write firmware packet at address 0x%04x: %d\n",
-- 
2.21.0

Re: [PATCH v2 09/22] watchdog: ziirave_wdt: Fix incorrect use of ARRAY_SIZE

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:53PM -0700, Andrey Smirnov wrote:
> Both memset() and ziirave_firm_write_block_data() expect length in
> bytes as an argument, not a number of elements in array. It just
> happens that in this particular case both values are equal. Modify the
> code to use sizeof() instead.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 10/22] watchdog: ziirave_wdt: Zero out only what's necessary

[Andrey Smirnov]

Instead of zeroing out all of the packet and then overwriting a
significant portion of those zeros via memcpy(), zero out only a
portion of the packet that is known to not contain any data.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index e0f55cbdc603..69694f2836d7 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -260,8 +260,6 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 		return -EMSGSIZE;
 	}
 
-	memset(packet, 0, sizeof(packet));
-
 	/* Packet length */
 	packet[0] = len;
 	/* Packet address */
@@ -269,6 +267,7 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	packet[2] = (addr16 & 0xff00) >> 8;
 
 	memcpy(packet + 3, data, len);
+	memset(packet + 3 + len, 0, ZIIRAVE_FIRM_PKT_DATA_SIZE - len);
 
 	/* Packet checksum */
 	for (i = 0; i < len + 3; i++)
-- 
2.21.0

Re: [PATCH v2 10/22] watchdog: ziirave_wdt: Zero out only what's necessary

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:54PM -0700, Andrey Smirnov wrote:
> Instead of zeroing out all of the packet and then overwriting a
> significant portion of those zeros via memcpy(), zero out only a
> portion of the packet that is known to not contain any data.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 11/22] watchdog: ziirave_wdt: Make use of put_unaligned_le16

[Andrey Smirnov]

Instead of doing this explicitly use put_unaligned_le16() to place
16-bit address value into command payload.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 69694f2836d7..38cf3ca329d7 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -21,6 +21,8 @@
 #include <linux/version.h>
 #include <linux/watchdog.h>
 
+#include <asm/unaligned.h>
+
 #define ZIIRAVE_TIMEOUT_MIN	3
 #define ZIIRAVE_TIMEOUT_MAX	255
 #define ZIIRAVE_TIMEOUT_DEFAULT	30
@@ -198,8 +200,7 @@ static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u16 addr)
 	struct i2c_client *client = to_i2c_client(wdd->parent);
 	u8 address[2];
 
-	address[0] = addr & 0xff;
-	address[1] = (addr >> 8) & 0xff;
+	put_unaligned_le16(addr, address);
 
 	return i2c_smbus_write_block_data(client,
 					  ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR,
@@ -263,8 +264,7 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	/* Packet length */
 	packet[0] = len;
 	/* Packet address */
-	packet[1] = addr16 & 0xff;
-	packet[2] = (addr16 & 0xff00) >> 8;
+	put_unaligned_le16(addr16, packet + 1);
 
 	memcpy(packet + 3, data, len);
 	memset(packet + 3 + len, 0, ZIIRAVE_FIRM_PKT_DATA_SIZE - len);
-- 
2.21.0

Re: [PATCH v2 11/22] watchdog: ziirave_wdt: Make use of put_unaligned_le16

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:55PM -0700, Andrey Smirnov wrote:
> Instead of doing this explicitly use put_unaligned_le16() to place
> 16-bit address value into command payload.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 12/22] watchdog: ziirave_wdt: Don't check if ihex record length is zero

[Andrey Smirnov]

Ihex_next_binrec() will return NULL if next record's 'len' is zero, so
explicit checks for that in the driver are unnecessary. Drop them.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 38cf3ca329d7..4b95467bf239 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -318,10 +318,6 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
 	u16 addr;
 
 	for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
-		/* Zero length marks end of records */
-		if (!be16_to_cpu(rec->len))
-			break;
-
 		addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1;
 		if (addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
 		    addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
@@ -381,10 +377,6 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 	msleep(500);
 
 	for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
-		/* Zero length marks end of records */
-		if (!be16_to_cpu(rec->len))
-			break;
-
 		ret = ziirave_firm_write_pkt(wdd, be32_to_cpu(rec->addr),
 					     rec->data, be16_to_cpu(rec->len));
 		if (ret)
-- 
2.21.0

Re: [PATCH v2 12/22] watchdog: ziirave_wdt: Don't check if ihex record length is zero

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:56PM -0700, Andrey Smirnov wrote:
> Ihex_next_binrec() will return NULL if next record's 'len' is zero, so
> explicit checks for that in the driver are unnecessary. Drop them.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 13/22] watchdog: ziirave_wdt: Don't read out more than 'len' firmware bytes

[Andrey Smirnov]

We only compare first 'len' bytes of read firmware, so we don't need
to read more that that.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 4b95467bf239..f05095b08016 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -318,6 +318,8 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
 	u16 addr;
 
 	for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
+		const u16 len = be16_to_cpu(rec->len);
+
 		addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1;
 		if (addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
 		    addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
@@ -331,7 +333,7 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
 			return ret;
 		}
 
-		for (i = 0; i < ARRAY_SIZE(data); i++) {
+		for (i = 0; i < len; i++) {
 			ret = i2c_smbus_read_byte_data(client,
 						ZIIRAVE_CMD_DOWNLOAD_READ_BYTE);
 			if (ret < 0) {
@@ -342,7 +344,7 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
 			data[i] = ret;
 		}
 
-		if (memcmp(data, rec->data, be16_to_cpu(rec->len))) {
+		if (memcmp(data, rec->data, len)) {
 			dev_err(&client->dev,
 				"Firmware mismatch at address 0x%04x\n", addr);
 			return -EINVAL;
-- 
2.21.0

Re: [PATCH v2 13/22] watchdog: ziirave_wdt: Don't read out more than 'len' firmware bytes

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:57PM -0700, Andrey Smirnov wrote:
> We only compare first 'len' bytes of read firmware, so we don't need
> to read more that that.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 14/22] watchdog: ziirave_wdt: Don't try to program readonly flash

[Andrey Smirnov]

Bootloader code will ignore any attempts to write data to any flash
area outside of [ZIIRAVE_FIRM_FLASH_MEMORY_START;
ZIIRAVE_FIRM_FLASH_MEMORY_END]. Firmware update code already have an
appropriate check to skip those areas when validating updated
firmware. Firmware programming code, OTOH, does not and will
needlessly send no-op I2C traffic. Add an appropriate check to
__ziirave_firm_write_pkt() so as to save all of that wasted effort.

While at it, normalize all of the address handling code to use full
32-bit address in units of bytes and convert it to an appropriate
value only in places where that is necessary.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index f05095b08016..a3cc936858ad 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -51,8 +51,8 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 
 #define ZIIRAVE_FIRM_PKT_TOTAL_SIZE	20
 #define ZIIRAVE_FIRM_PKT_DATA_SIZE	16
-#define ZIIRAVE_FIRM_FLASH_MEMORY_START	0x1600
-#define ZIIRAVE_FIRM_FLASH_MEMORY_END	0x2bbf
+#define ZIIRAVE_FIRM_FLASH_MEMORY_START	(2 * 0x1600)
+#define ZIIRAVE_FIRM_FLASH_MEMORY_END	(2 * 0x2bbf)
 #define ZIIRAVE_FIRM_PAGE_SIZE		128
 
 /* Received and ready for next Download packet. */
@@ -195,12 +195,13 @@ static int ziirave_firm_wait_for_ack(struct watchdog_device *wdd)
 	return ret == ZIIRAVE_FIRM_DOWNLOAD_ACK ? 0 : -EIO;
 }
 
-static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u16 addr)
+static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u32 addr)
 {
 	struct i2c_client *client = to_i2c_client(wdd->parent);
+	const u16 addr16 = (u16)addr / 2;
 	u8 address[2];
 
-	put_unaligned_le16(addr, address);
+	put_unaligned_le16(addr16, address);
 
 	return i2c_smbus_write_block_data(client,
 					  ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR,
@@ -234,6 +235,12 @@ static int ziirave_firm_write_byte(struct watchdog_device *wdd, u8 command,
 					     wait_for_ack);
 }
 
+static bool ziirave_firm_addr_readonly(u32 addr)
+{
+	return addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
+	       addr > ZIIRAVE_FIRM_FLASH_MEMORY_END;
+}
+
 /*
  * ziirave_firm_write_pkt() - Build and write a firmware packet
  *
@@ -261,6 +268,16 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 		return -EMSGSIZE;
 	}
 
+	/*
+	 * Ignore packets that are targeting program memory outisde of
+	 * app partition, since they will be ignored by the
+	 * bootloader. At the same time, we need to make sure we'll
+	 * allow zero length packet that will be sent as the last step
+	 * of firmware update
+	 */
+	if (len && ziirave_firm_addr_readonly(addr))
+		return 0;
+
 	/* Packet length */
 	packet[0] = len;
 	/* Packet address */
@@ -279,7 +296,7 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 	if (ret)
 		dev_err(&client->dev,
 		      "Failed to write firmware packet at address 0x%04x: %d\n",
-		      addr16, ret);
+		      addr, ret);
 
 	return ret;
 }
@@ -315,14 +332,12 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
 	const struct ihex_binrec *rec;
 	int i, ret;
 	u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE];
-	u16 addr;
 
 	for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
 		const u16 len = be16_to_cpu(rec->len);
+		const u32 addr = be32_to_cpu(rec->addr);
 
-		addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1;
-		if (addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
-		    addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
+		if (ziirave_firm_addr_readonly(addr))
 			continue;
 
 		ret = ziirave_firm_set_read_addr(wdd, addr);
-- 
2.21.0

Re: [PATCH v2 14/22] watchdog: ziirave_wdt: Don't try to program readonly flash

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:58PM -0700, Andrey Smirnov wrote:
> Bootloader code will ignore any attempts to write data to any flash
> area outside of [ZIIRAVE_FIRM_FLASH_MEMORY_START;
> ZIIRAVE_FIRM_FLASH_MEMORY_END]. Firmware update code already have an
> appropriate check to skip those areas when validating updated
> firmware. Firmware programming code, OTOH, does not and will
> needlessly send no-op I2C traffic. Add an appropriate check to
> __ziirave_firm_write_pkt() so as to save all of that wasted effort.
> 
> While at it, normalize all of the address handling code to use full
> 32-bit address in units of bytes and convert it to an appropriate
> value only in places where that is necessary.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 15/22] watchdog: ziirave_wdt: Fix misleading error message

[Andrey Smirnov]

Fix misleading error message in ziirave_wdt_init_duration(). Saying
"unable to set ..." implies that an attempt at communication with
watchdog device has taken palce and was not successful. In this case,
however, all it indicates is that no reset pulse duration was
specified either via kernel parameter or Device Tree. Re-phase the log
message to be more clear about benign nature of this event.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index a3cc936858ad..0c150f3cf408 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -603,7 +603,7 @@ static int ziirave_wdt_init_duration(struct i2c_client *client)
 						   &reset_duration);
 		if (ret) {
 			dev_info(&client->dev,
-				 "Unable to set reset pulse duration, using default\n");
+			 "No reset pulse duration specified, using default\n");
 			return 0;
 		}
 	}
-- 
2.21.0

Re: [PATCH v2 15/22] watchdog: ziirave_wdt: Fix misleading error message

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:08:59PM -0700, Andrey Smirnov wrote:
> Fix misleading error message in ziirave_wdt_init_duration(). Saying
> "unable to set ..." implies that an attempt at communication with
> watchdog device has taken palce and was not successful. In this case,
> however, all it indicates is that no reset pulse duration was
> specified either via kernel parameter or Device Tree. Re-phase the log
> message to be more clear about benign nature of this event.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 16/22] watchdog: ziirave_wdt: Fix JUMP_TO_BOOTLOADER payload

[Andrey Smirnov]

Bootloader firmware expects the following traffic for
JUMP_TO_BOOTLOADER:

S Addr Wr [A] 0x0c [A] 0x01 [A] P

using ziirave_firm_write_byte() will result in

S Addr Wr [A] 0x0c [A] 0x01 [A] 0x01 [A] P

which happens to work because firmware will ignore any extra bytes and
expected magic value matches byte count sent by
i2c_smbus_write_block_data(). Fix this by converting the code to use
i2c_smbus_write_byte_data() instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 0c150f3cf408..0185b9175cc0 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -72,6 +72,8 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 #define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER		0x0c
 #define ZIIRAVE_CMD_DOWNLOAD_PACKET		0x0e
 
+#define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER_MAGIC	1
+
 #define ZIIRAVE_FW_VERSION_FMT	"02.%02u.%02u"
 #define ZIIRAVE_BL_VERSION_FMT	"01.%02u.%02u"
 
@@ -376,8 +378,9 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 	const struct ihex_binrec *rec;
 	int ret;
 
-	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER, 1,
-				      false);
+	ret = i2c_smbus_write_byte_data(client,
+					ZIIRAVE_CMD_JUMP_TO_BOOTLOADER,
+					ZIIRAVE_CMD_JUMP_TO_BOOTLOADER_MAGIC);
 	if (ret) {
 		dev_err(&client->dev, "Failed to jump to bootloader\n");
 		return ret;
-- 
2.21.0

Re: [PATCH v2 16/22] watchdog: ziirave_wdt: Fix JUMP_TO_BOOTLOADER payload

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:00PM -0700, Andrey Smirnov wrote:
> Bootloader firmware expects the following traffic for
> JUMP_TO_BOOTLOADER:
> 
> S Addr Wr [A] 0x0c [A] 0x01 [A] P
> 
> using ziirave_firm_write_byte() will result in
> 
> S Addr Wr [A] 0x0c [A] 0x01 [A] 0x01 [A] P
> 
> which happens to work because firmware will ignore any extra bytes and
> expected magic value matches byte count sent by
> i2c_smbus_write_block_data(). Fix this by converting the code to use
> i2c_smbus_write_byte_data() instead.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 17/22] watchdog: ziirave_wdt: Fix DOWNLOAD_END payload

[Andrey Smirnov]

Bootloader firmware expects the following traffic for DOWNLOAD_END:

S Addr Wr [A] 0x11 [A] P

using ziirave_firm_write_byte() will result in

S Addr Wr [A] 0x11 [A] 0x01 [A] 0x01 [A] P

which happens to work because firmware will ignore any extra bytes
sent. Fix this by converting the code to use i2c_smbus_write_byte()
instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 0185b9175cc0..a598780d73d3 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -425,7 +425,7 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 	}
 
 	/* End download operation */
-	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_END, 1, false);
+	ret = i2c_smbus_write_byte(client, ZIIRAVE_CMD_DOWNLOAD_END);
 	if (ret) {
 		dev_err(&client->dev,
 			"Failed to end firmware download: %d\n", ret);
-- 
2.21.0

Re: [PATCH v2 17/22] watchdog: ziirave_wdt: Fix DOWNLOAD_END payload

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:01PM -0700, Andrey Smirnov wrote:
> Bootloader firmware expects the following traffic for DOWNLOAD_END:
> 
> S Addr Wr [A] 0x11 [A] P
> 
> using ziirave_firm_write_byte() will result in
> 
> S Addr Wr [A] 0x11 [A] 0x01 [A] 0x01 [A] P
> 
> which happens to work because firmware will ignore any extra bytes
> sent. Fix this by converting the code to use i2c_smbus_write_byte()
> instead.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 18/22] watchdog: ziirave_wdt: Fix RESET_PROCESSOR payload

[Andrey Smirnov]

Bootloader firmware expects the following traffic for
RESET_PROCESSOR:

S Addr Wr [A] 0x0b [A] 0x01 [A] P

using ziirave_firm_write_byte() will result in

S Addr Wr [A] 0x0b [A] 0x01 [A] 0x01 [A] P

which happens to work because firmware will ignore any extra bytes and
expected magic value matches byte count sent by
i2c_smbus_write_block_data(). Fix this by converting the code to use
i2c_smbus_write_byte_data() instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index a598780d73d3..92df27350e41 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -73,6 +73,7 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 #define ZIIRAVE_CMD_DOWNLOAD_PACKET		0x0e
 
 #define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER_MAGIC	1
+#define ZIIRAVE_CMD_RESET_PROCESSOR_MAGIC	1
 
 #define ZIIRAVE_FW_VERSION_FMT	"02.%02u.%02u"
 #define ZIIRAVE_BL_VERSION_FMT	"01.%02u.%02u"
@@ -433,8 +434,9 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 	}
 
 	/* Reset the processor */
-	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_RESET_PROCESSOR, 1,
-				      false);
+	ret = i2c_smbus_write_byte_data(client,
+					ZIIRAVE_CMD_RESET_PROCESSOR,
+					ZIIRAVE_CMD_RESET_PROCESSOR_MAGIC);
 	if (ret) {
 		dev_err(&client->dev,
 			"Failed to reset the watchdog: %d\n", ret);
-- 
2.21.0

Re: [PATCH v2 18/22] watchdog: ziirave_wdt: Fix RESET_PROCESSOR payload

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:02PM -0700, Andrey Smirnov wrote:
> Bootloader firmware expects the following traffic for
> RESET_PROCESSOR:
> 
> S Addr Wr [A] 0x0b [A] 0x01 [A] P
> 
> using ziirave_firm_write_byte() will result in
> 
> S Addr Wr [A] 0x0b [A] 0x01 [A] 0x01 [A] P
> 
> which happens to work because firmware will ignore any extra bytes and
> expected magic value matches byte count sent by
> i2c_smbus_write_block_data(). Fix this by converting the code to use
> i2c_smbus_write_byte_data() instead.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 19/22] watchdog: ziirave_wdt: Drop status polling code

[Andrey Smirnov]

Bootloader firmware doesn't implement DOWNLOAD_START or
DOWNLOAD_PACKET in a non-blocking way. It will stretch the clock of
the first status byte read until the operation is complete. Polling
for the status is not really necessary since it will always succed on
the first try. Replace polling code with a simple single byte read to
simplify things.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 92df27350e41..681f65349779 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -57,11 +57,6 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 
 /* Received and ready for next Download packet. */
 #define ZIIRAVE_FIRM_DOWNLOAD_ACK	1
-/* Currently writing to flash. Retry Download status in a moment! */
-#define ZIIRAVE_FIRM_DOWNLOAD_BUSY	2
-
-/* Wait for ACK timeout in ms */
-#define ZIIRAVE_FIRM_WAIT_FOR_ACK_TIMEOUT	50
 
 /* Firmware commands */
 #define ZIIRAVE_CMD_DOWNLOAD_START		0x10
@@ -175,25 +170,16 @@ static unsigned int ziirave_wdt_get_timeleft(struct watchdog_device *wdd)
 	return ret;
 }
 
-static int ziirave_firm_wait_for_ack(struct watchdog_device *wdd)
+static int ziirave_firm_read_ack(struct watchdog_device *wdd)
 {
 	struct i2c_client *client = to_i2c_client(wdd->parent);
 	int ret;
-	unsigned long timeout;
-
-	timeout = jiffies + msecs_to_jiffies(ZIIRAVE_FIRM_WAIT_FOR_ACK_TIMEOUT);
-	do {
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
 
-		usleep_range(5000, 10000);
-
-		ret = i2c_smbus_read_byte(client);
-		if (ret < 0) {
-			dev_err(&client->dev, "Failed to read byte\n");
-			return ret;
-		}
-	} while (ret == ZIIRAVE_FIRM_DOWNLOAD_BUSY);
+	ret = i2c_smbus_read_byte(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read status byte\n");
+		return ret;
+	}
 
 	return ret == ZIIRAVE_FIRM_DOWNLOAD_ACK ? 0 : -EIO;
 }
@@ -226,7 +212,7 @@ static int ziirave_firm_write_block_data(struct watchdog_device *wdd,
 	}
 
 	if (wait_for_ack)
-		ret = ziirave_firm_wait_for_ack(wdd);
+		ret = ziirave_firm_read_ack(wdd);
 
 	return ret;
 }
-- 
2.21.0

Re: [PATCH v2 19/22] watchdog: ziirave_wdt: Drop status polling code

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:03PM -0700, Andrey Smirnov wrote:
> Bootloader firmware doesn't implement DOWNLOAD_START or
> DOWNLOAD_PACKET in a non-blocking way. It will stretch the clock of
> the first status byte read until the operation is complete. Polling
> for the status is not really necessary since it will always succed on
> the first try. Replace polling code with a simple single byte read to
> simplify things.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 20/22] watchdog: ziirave_wdt: Fix DOWNLOAD_START payload

[Andrey Smirnov]

Bootloader firmware expects the following traffic for DOWNLOAD_END:

S Addr Wr [A] 0x10 [A] P

using ziirave_firm_write_byte() will result in

S Addr Wr [A] 0x10 [A] 0x01 [A] 0x01 [A] P

which happens to work because firmware will ignore any extra bytes
sent. Fix this by converting the code to use i2c_smbus_write_byte()
instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 681f65349779..ed69fa82e09c 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -217,13 +217,6 @@ static int ziirave_firm_write_block_data(struct watchdog_device *wdd,
 	return ret;
 }
 
-static int ziirave_firm_write_byte(struct watchdog_device *wdd, u8 command,
-				   u8 byte, bool wait_for_ack)
-{
-	return ziirave_firm_write_block_data(wdd, command, 1, &byte,
-					     wait_for_ack);
-}
-
 static bool ziirave_firm_addr_readonly(u32 addr)
 {
 	return addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
@@ -375,12 +368,18 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 
 	msleep(500);
 
-	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1, true);
+	ret = i2c_smbus_write_byte(client, ZIIRAVE_CMD_DOWNLOAD_START);
 	if (ret) {
 		dev_err(&client->dev, "Failed to start download\n");
 		return ret;
 	}
 
+	ret = ziirave_firm_read_ack(wdd);
+	if (ret) {
+		dev_err(&client->dev, "No ACK for start download\n");
+		return ret;
+	}
+
 	msleep(500);
 
 	for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
-- 
2.21.0

Re: [PATCH v2 20/22] watchdog: ziirave_wdt: Fix DOWNLOAD_START payload

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:04PM -0700, Andrey Smirnov wrote:
> Bootloader firmware expects the following traffic for DOWNLOAD_END:
> 
> S Addr Wr [A] 0x10 [A] P
> 
> using ziirave_firm_write_byte() will result in
> 
> S Addr Wr [A] 0x10 [A] 0x01 [A] 0x01 [A] P
> 
> which happens to work because firmware will ignore any extra bytes
> sent. Fix this by converting the code to use i2c_smbus_write_byte()
> instead.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 21/22] watchdog: ziirave_wdt: Drop ziirave_firm_write_block_data()

[Andrey Smirnov]

There's only one user of ziirave_firm_write_block_data(), so we may as
well inline it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index ed69fa82e09c..48278034cda6 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -197,26 +197,6 @@ static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u32 addr)
 					  sizeof(address), address);
 }
 
-static int ziirave_firm_write_block_data(struct watchdog_device *wdd,
-					 u8 command, u8 length, const u8 *data,
-					 bool wait_for_ack)
-{
-	struct i2c_client *client = to_i2c_client(wdd->parent);
-	int ret;
-
-	ret = i2c_smbus_write_block_data(client, command, length, data);
-	if (ret) {
-		dev_err(&client->dev,
-			"Failed to send command 0x%02x: %d\n", command, ret);
-		return ret;
-	}
-
-	if (wait_for_ack)
-		ret = ziirave_firm_read_ack(wdd);
-
-	return ret;
-}
-
 static bool ziirave_firm_addr_readonly(u32 addr)
 {
 	return addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
@@ -273,8 +253,15 @@ static int __ziirave_firm_write_pkt(struct watchdog_device *wdd,
 		checksum += packet[i];
 	packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1] = checksum;
 
-	ret = ziirave_firm_write_block_data(wdd, ZIIRAVE_CMD_DOWNLOAD_PACKET,
-					    sizeof(packet), packet, true);
+	ret = i2c_smbus_write_block_data(client, ZIIRAVE_CMD_DOWNLOAD_PACKET,
+					 sizeof(packet), packet);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to send DOWNLOAD_PACKET: %d\n", ret);
+		return ret;
+	}
+
+	ret = ziirave_firm_read_ack(wdd);
 	if (ret)
 		dev_err(&client->dev,
 		      "Failed to write firmware packet at address 0x%04x: %d\n",
-- 
2.21.0

Re: [PATCH v2 21/22] watchdog: ziirave_wdt: Drop ziirave_firm_write_block_data()

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:05PM -0700, Andrey Smirnov wrote:
> There's only one user of ziirave_firm_write_block_data(), so we may as
> well inline it.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v2 22/22] watchdog: ziirave_wdt: Update checked I2C functionality mask

[Andrey Smirnov]

Update checked I2C functionality mask to reflect all of the SMBus
primitives used by this driver.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 48278034cda6..4a363a8b2d20 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -602,7 +602,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	struct ziirave_wdt_data *w_priv;
 	int val;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE |
+				     I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WRITE_BLOCK_DATA))
 		return -ENODEV;
 
 	w_priv = devm_kzalloc(&client->dev, sizeof(*w_priv), GFP_KERNEL);
-- 
2.21.0

Re: [PATCH v2 22/22] watchdog: ziirave_wdt: Update checked I2C functionality mask

[Guenter Roeck]

On Mon, Aug 12, 2019 at 01:09:06PM -0700, Andrey Smirnov wrote:
> Update checked I2C functionality mask to reflect all of the SMBus
> primitives used by this driver.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>