From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932317AbcG1Oh0 (ORCPT ); Thu, 28 Jul 2016 10:37:26 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:47900 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758761AbcG1OhA (ORCPT ); Thu, 28 Jul 2016 10:37:00 -0400 Subject: Re: [PATCH v3] watchdog: ziirave_wdt: Add support to upload the firmware. To: Enric Balletbo i Serra , linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org References: <1469702269-32268-1-git-send-email-enric.balletbo@collabora.com> Cc: Wim Van Sebroeck , Guenter Roeck , Martyn Welch From: Martyn Welch Message-ID: <579A1881.7010600@collabora.co.uk> Date: Thu, 28 Jul 2016 15:36:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.8.0 MIME-Version: 1.0 In-Reply-To: <1469702269-32268-1-git-send-email-enric.balletbo@collabora.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28/07/16 11:37, Enric Balletbo i Serra wrote: > This patch adds and entry to the sysfs to start firmware upload process > on the specified device with the requested firmware. > > The uploading of the firmware needs only to happen once per firmware > upgrade, as the firmware is stored in persistent storage. If the > firmware upload or the firmware verification fails then we print and > error message and exit. > > Signed-off-by: Enric Balletbo i Serra > --- > > Changes since v2: (requested by Guenter) > - Remove ihex parsing, convert the file into binary before passing it to the > kernel (ihex2fw ). > - New binary firmware parsed using the ihex.h helpers. > - Protect firmware upgrade with a mutex. > - Remove a noisy message. > - Remove some unneded memset. > - Remove some unnecessary () > > Changes since v1: (requested by Martyn) > - Remove two defines that are not used. > - Fix typo in documentation count -> length > > drivers/watchdog/ziirave_wdt.c | 363 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 363 insertions(+) > > diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c > index fa1efef..81c8545 100644 > --- a/drivers/watchdog/ziirave_wdt.c > +++ b/drivers/watchdog/ziirave_wdt.c > @@ -18,7 +18,10 @@ > * GNU General Public License for more details. > */ > > +#include > #include > +#include > +#include > #include > #include > #include > @@ -36,6 +39,8 @@ > #define ZIIRAVE_STATE_OFF 0x1 > #define ZIIRAVE_STATE_ON 0x2 > > +#define ZIIRAVE_FW_NAME "ziirave_wdt.fw" > + > static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL, > "host request", NULL, "illegal configuration", > "illegal instruction", "illegal trap", > @@ -50,12 +55,32 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL, > #define ZIIRAVE_WDT_PING 0x9 > #define ZIIRAVE_WDT_RESET_DURATION 0xa > > +#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 > + > +/* 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 > + > +/* Firmware commands */ > +#define ZIIRAVE_CMD_DOWNLOAD_START 0x10 > +#define ZIIRAVE_CMD_DOWNLOAD_END 0x11 > +#define ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR 0x12 > +#define ZIIRAVE_CMD_DOWNLOAD_READ_BYTE 0x13 > +#define ZIIRAVE_CMD_RESET_PROCESSOR 0x0b > +#define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER 0x0c > +#define ZIIRAVE_CMD_DOWNLOAD_PACKET 0x0e > + > struct ziirave_wdt_rev { > unsigned char major; > unsigned char minor; > }; > > struct ziirave_wdt_data { > + struct mutex sysfs_mutex; I don't think this is the best name for the mutex given that it's only used with firmware upload. "firmware_mutex"?ffir > struct watchdog_device wdd; > struct ziirave_wdt_rev bootloader_rev; > struct ziirave_wdt_rev firmware_rev; > @@ -146,6 +171,288 @@ static unsigned int ziirave_wdt_get_timeleft(struct watchdog_device *wdd) > return ret; > } > > +static int ziirave_firm_wait_for_ack(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + int ret; > + > + do { > + usleep_range(5000, 5100); > + 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); > + > + return ret == ZIIRAVE_FIRM_DOWNLOAD_ACK ? 0 : -EIO; > +} > + > +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; > + > + return i2c_smbus_write_block_data(client, > + ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR, > + ARRAY_SIZE(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_wait_for_ack(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); > +} > + > +/* > + * ziirave_firm_write_pkt() - Build and write a firmware packet > + * > + * A packet to send to the firmware is composed by following bytes: > + * Length | Addr0 | Addr1 | Data0 .. Data15 | Checksum | > + * Where, > + * Length: A data byte containing the length of the data. > + * Addr0: Low byte of the address. > + * Addr1: High byte of the address. > + * 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) > +{ > + 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 address */ > + addr = (be32_to_cpu(rec->addr) & 0xffff) >> 1; > + packet[1] = (u8)(addr & 0xff); > + packet[2] = (u8)((addr & 0xff00) >> 8); > + > + /* Packet data */ > + if (be16_to_cpu(rec->len) > ZIIRAVE_FIRM_PKT_DATA_SIZE) > + return -EMSGSIZE; > + memcpy(packet + 3, rec->data, be16_to_cpu(rec->len)); > + > + /* Packet checksum */ > + for (i = 0; i < ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1; i++) > + checksum += packet[i]; > + packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1] = checksum; > + > + ret = ziirave_firm_write_block_data(wdd, ZIIRAVE_CMD_DOWNLOAD_PACKET, > + ARRAY_SIZE(packet), packet, true); > + if (ret) > + dev_err(&client->dev, > + "Failed to write firmware packet at address 0x%04x: %d\n", > + addr, ret); > + > + return ret; > +} > + > +static int ziirave_firm_verify(struct watchdog_device *wdd, > + const struct firmware *fw) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + 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)) { > + /* 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) > + continue; > + > + ret = ziirave_firm_set_read_addr(wdd, addr); > + if (ret) { > + dev_err(&client->dev, > + "Failed to send SET_READ_ADDR command: %d\n", > + ret); > + return ret; > + } > + > + for (i = 0; i < ARRAY_SIZE(data); i++) { > + ret = i2c_smbus_read_byte_data(client, > + ZIIRAVE_CMD_DOWNLOAD_READ_BYTE); > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to READ DATA: %d\n", ret); > + return ret; > + } > + data[i] = (u8)ret; > + } > + > + if (memcmp(data, rec->data, be16_to_cpu(rec->len))) { > + dev_err(&client->dev, > + "Firmware mismatch at address 0x%04x\n", addr); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +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; > + > + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER, 1, > + false); > + if (ret) > + return ret; > + > + msleep(500); > + > + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1, true); > + if (ret) > + return ret; > + > + 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; > + > + /* 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; > + } > + > + /* 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; > + } > + } > + > + /* 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); > + if (ret) { > + dev_err(&client->dev, "Failed to send EMPTY packet: %d\n", ret); > + return ret; > + } > + > + /* This sleep seems to be required */ > + msleep(20); > + > + /* Start firmware verification */ > + ret = ziirave_firm_verify(wdd, fw); > + if (ret) { > + dev_err(&client->dev, > + "Failed to verify firmware: %d\n", ret); > + return ret; > + } > + > + /* End download operation */ > + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_END, 1, false); > + if (ret) > + return ret; > + > + /* Reset the processor */ > + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_RESET_PROCESSOR, 1, > + false); > + if (ret) > + return ret; > + > + msleep(500); > + > + return 0; > +} > + > static const struct watchdog_info ziirave_wdt_info = { > .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING, > .identity = "Zodiac RAVE Watchdog", > @@ -201,10 +508,64 @@ static ssize_t ziirave_wdt_sysfs_show_reason(struct device *dev, > static DEVICE_ATTR(reset_reason, S_IRUGO, ziirave_wdt_sysfs_show_reason, > NULL); > > +static ssize_t ziirave_wdt_sysfs_store_firm(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ Am I right in thinking firmware upgrade will get triggered whatever is written to it? Wouldn't it be safer to only start the upgrade if a specific string is written to the sysfs entry? (Performing the upgrade is basically going to disable the watchdog is it not?) > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client); > + const struct firmware *fw; > + int err; > + > + err = mutex_lock_interruptible(&w_priv->sysfs_mutex); > + if (err) > + return err; > + > + err = request_ihex_firmware(&fw, ZIIRAVE_FW_NAME, dev); > + if (err) { > + dev_err(&client->dev, "Failed to request ihex firmware\n"); > + goto unlock_sysfs; > + } > + > + err = ziirave_firm_upload(&w_priv->wdd, fw); > + if (err) { > + dev_err(&client->dev, "The firmware update failed: %d\n", err); > + goto release_firmware; > + } > + > + /* Update firmware version */ > + err = ziirave_wdt_revision(client, &w_priv->firmware_rev, > + ZIIRAVE_WDT_FIRM_VER_MAJOR); > + if (err) { > + dev_err(&client->dev, "Failed to read firmware version: %d\n", > + err); > + goto release_firmware; > + } > + > + dev_info(&client->dev, "Firmware updated to version 02.%02u.%02u\n", > + w_priv->firmware_rev.major, w_priv->firmware_rev.minor); > + > + /* Restore the watchdog timeout */ > + err = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout); > + if (err) > + dev_err(&client->dev, "Failed to set timeout: %d\n", err); > + > +release_firmware: > + release_firmware(fw); > +unlock_sysfs: > + mutex_unlock(&w_priv->sysfs_mutex); > + > + return err ? err : count; > +} > + > +static DEVICE_ATTR(update_firmware, S_IWUSR, NULL, > + ziirave_wdt_sysfs_store_firm); > + > static struct attribute *ziirave_wdt_attrs[] = { > &dev_attr_firmware_version.attr, > &dev_attr_bootloader_version.attr, > &dev_attr_reset_reason.attr, > + &dev_attr_update_firmware.attr, > NULL > }; > ATTRIBUTE_GROUPS(ziirave_wdt); > @@ -252,6 +613,8 @@ static int ziirave_wdt_probe(struct i2c_client *client, > if (!w_priv) > return -ENOMEM; > > + mutex_init(&w_priv->sysfs_mutex); > + > w_priv->wdd.info = &ziirave_wdt_info; > w_priv->wdd.ops = &ziirave_wdt_ops; > w_priv->wdd.min_timeout = ZIIRAVE_TIMEOUT_MIN; >