From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752041AbcGRVn1 (ORCPT ); Mon, 18 Jul 2016 17:43:27 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:49876 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751853AbcGRVnX (ORCPT ); Mon, 18 Jul 2016 17:43:23 -0400 Subject: Re: [PATCH 2/2] 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: <1466160754-15599-1-git-send-email-enric.balletbo@collabora.com> <1466160754-15599-3-git-send-email-enric.balletbo@collabora.com> Cc: Wim Van Sebroeck , Guenter Roeck , Martyn Welch From: Martyn Welch Message-ID: <578D4D76.8070009@collabora.co.uk> Date: Mon, 18 Jul 2016 22:43:18 +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: <1466160754-15599-3-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 Few comments inline On 17/06/16 11:52, 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 > --- > drivers/watchdog/ziirave_wdt.c | 426 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 426 insertions(+) > > diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c > index fa1efef..63e81bc 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_fw.hex" > + > static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL, > "host request", NULL, "illegal configuration", > "illegal instruction", "illegal trap", > @@ -50,6 +55,30 @@ 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 This doesn't seem to be used anywhere: > +/* Download message error. Ex: Message did not CRC properly */ > +#define ZIIRAVE_FIRM_DOWNLOAD_NAK 0 > +/* 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 Neither is this: > +/* Hardware error writing flash has occurred! */ > +#define ZIIRAVE_FIRM_DOWNLOAD_FAIL 3 > + > +/* > + * 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; > @@ -62,6 +91,23 @@ struct ziirave_wdt_data { > int reset_reason; > }; > > +/* > + * A packet to send to the firmware is composed by following bytes: > + * Count | Addr0 | Addr1 | Data0 .. Data15 | Checksum | > + * Where, Would "Length" below be the same as "Count" above? > + * 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. > + */ > +struct ziirave_fw_pkt_t { > + u8 length; > + u16 addr; > + u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE]; > + u8 checksum; > +} __packed; > + > static int wdt_timeout; > module_param(wdt_timeout, int, 0); > MODULE_PARM_DESC(wdt_timeout, "Watchdog timeout in seconds"); > @@ -146,6 +192,339 @@ 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; Is the required response the same for a NAK and FAIL? > +} > + > +/* > + * Parse Microchip format Hex file (an extended Intel Hex file) to extract > + * address and data. > + */ > +static int ziirave_firm_parse_hex_line(unsigned char *fw_data, > + struct ziirave_fw_pkt_t *fw_pkt, > + bool *addr_has_changed) > +{ > + int count = 0; > + unsigned char *src, dst; > + > + if (*fw_data++ != ':') > + return -EFAULT; > + > + /* locate end of line */ > + for (src = fw_data; *src != '\n'; src += 2) { > + /* > + * Convert the ascii hexadecimal string to its binary > + * representation > + */ > + if (hex2bin(&dst, src, 1)) > + return -EINVAL; > + > + /* Parse line to split addr / data */ > + switch (count) { > + case 0: > + fw_pkt->length = dst; > + break; > + case 1: > + fw_pkt->addr = dst << 8; > + break; > + case 2: > + fw_pkt->addr |= dst; > + fw_pkt->addr >>= 1; > + break; > + case 3: > + /* check if data is an address */ > + if (dst == 0x04) > + *addr_has_changed = true; > + else > + *addr_has_changed = false; > + break; > + case 4: > + case 5: > + if (!*addr_has_changed) > + fw_pkt->data[(count - 4)] = dst; > + break; > + default: > + fw_pkt->data[(count - 4)] = dst; > + break; > + } > + count++; > + } > + > + /* return read value + ':' + '\n' */ > + return (count * 2) + 2; > +} > + > +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]; > + > + memset(address, 0, sizeof(address)); > + 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); > + if (ret) > + dev_err(&client->dev, > + "Failed waiting ACK from command 0x%02x: %d\n", > + command, ret); > + } > + > + 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 int ziirave_firm_write_pkt(struct watchdog_device *wdd, > + struct ziirave_fw_pkt_t *fw_pkt) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + u8 i, checksum = 0, packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE]; > + int ret; > + > + memset(&packet, 0, sizeof(packet)); > + > + packet[0] = fw_pkt->length; > + packet[1] = (u8)(fw_pkt->addr & 0xff); > + packet[2] = (u8)((fw_pkt->addr & 0xff00) >> 8); > + > + /* Copy packet data */ > + memcpy(packet + 3, fw_pkt->data, fw_pkt->length); > + > + /* Compute 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", > + fw_pkt->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); > + int i, ret, read_bytes = 0, total_read_bytes = 0; > + bool addr_has_changed; > + struct ziirave_fw_pkt_t fw_pkt; > + u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE]; > + > + while (total_read_bytes < fw->size) { > + read_bytes = ziirave_firm_parse_hex_line( > + (u8 *)(fw->data + total_read_bytes), > + &fw_pkt, &addr_has_changed); > + > + /* Detect the end of file */ > + total_read_bytes += read_bytes; > + if (total_read_bytes == fw->size) > + break; > + > + if (addr_has_changed) > + continue; > + > + if (fw_pkt.addr < ZIIRAVE_FIRM_FLASH_MEMORY_START || > + fw_pkt.addr > ZIIRAVE_FIRM_FLASH_MEMORY_END) > + continue; > + > + ret = ziirave_firm_set_read_addr(wdd, fw_pkt.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, fw_pkt.data, fw_pkt.length)) { > + dev_err(&client->dev, > + "Firmware mismatch at address 0x%04x\n", > + fw_pkt.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); > + struct ziirave_fw_pkt_t fw_pkt; > + struct ziirave_fw_pkt_t fw_pkt_new; > + int ret, total_read_bytes = 0, words_till_page_break; > + bool addr_has_changed; > + > + 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); > + > + while (total_read_bytes < fw->size) { > + int read_bytes = 0; > + > + read_bytes = ziirave_firm_parse_hex_line( > + (u8 *)(fw->data + total_read_bytes), > + &fw_pkt, &addr_has_changed); > + > + if (read_bytes <= 0) { > + dev_err(&client->dev, > + "Failed to parse HEX file\n"); > + return -EINVAL; > + } > + > + /* Detect the end of file */ > + total_read_bytes += read_bytes; > + if (total_read_bytes == fw->size) { > + /* > + * For end of download, the length field will be set > + * to 0 > + */ > + memset(&fw_pkt, 0, sizeof(fw_pkt)); > + ret = ziirave_firm_write_pkt(wdd, &fw_pkt); > + 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; > + break; > + } > + > + if (addr_has_changed) > + continue; > + > + /* Calculate words till page break */ > + words_till_page_break = (64 - (fw_pkt.addr & 0x3f)); > + if ((fw_pkt.length >> 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. > + */ > + memset(&fw_pkt_new, 0, sizeof(fw_pkt_new)); > + > + fw_pkt_new.length = words_till_page_break << 1; > + fw_pkt_new.addr = fw_pkt.addr; > + memcpy(fw_pkt_new.data, fw_pkt.data, fw_pkt_new.length); > + > + ret = ziirave_firm_write_pkt(wdd, &fw_pkt_new); > + if (ret) > + return ret; > + > + /* Create a packet with the second block of data */ > + memset(&fw_pkt_new, 0, sizeof(fw_pkt_new)); > + > + /* Remaining bytes */ > + fw_pkt_new.length = > + fw_pkt.length - (words_till_page_break << 1); > + fw_pkt_new.addr = fw_pkt.addr + words_till_page_break; > + memcpy(fw_pkt_new.data, > + fw_pkt.data + (words_till_page_break << 1), > + fw_pkt_new.length); > + > + ret = ziirave_firm_write_pkt(wdd, &fw_pkt_new); > + if (ret) > + return ret; > + } else { > + ret = ziirave_firm_write_pkt(wdd, &fw_pkt); > + 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 +580,57 @@ 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) > +{ > + 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 = request_firmware(&fw, ZIIRAVE_FW_NAME, dev); > + if (err) { > + dev_err(&client->dev, "Unable to open firmware %s: %d\n", > + ZIIRAVE_FW_NAME, err); > + return err; > + } > + > + 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); > + return err ? err : count; > +} > + > +static DEVICE_ATTR(update_fw, 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_fw.attr, > NULL > }; > ATTRIBUTE_GROUPS(ziirave_wdt); >