-----Original Message-----
From: Yuval Mintz [mailto:yuv...@mellanox.com]
Sent: 27 March 2018 19:07
To: Kalluru, Sudarsana <sudarsana.kall...@cavium.com>
Cc: da...@davemloft.net; netdev@vger.kernel.org; Elior, Ariel
<ariel.el...@cavium.com>
Subject: Re: [PATCH net-next 3/4] qed: Adapter flash update support.
On Mon, Mar 26, 2018 at 03:13:47AM -0700, Sudarsana Reddy Kalluru wrote:
> This patch adds the required driver support for updating the flash or
> non volatile memory of the adapter. At highlevel, flash upgrade
> comprises of reading the flash images from the input file, validating
> the images and writing it to the respective paritions.
s/it/them/
[...]
> + *
> /----------------------------------------------------------------------\
> + * 0B | 0x4 [command index]
> |
> + * 4B | image_type | Options | Number of register settings
> |
> + * 8B | Value
> |
> + * 12B | Mask
> |
> + * 16B | Offset
> |
> + *
> \----------------------------------------------------------------------/
> + * There can be several Value-Mask-Offset sets as specified by 'Number
> of...'.
> + * Options - 0'b - Calculate & Update CRC for image */ static int
> +qed_nvm_flash_image_access(struct qed_dev *cdev, const u8 **data,
> + bool *check_resp)
> +{
> + struct qed_nvm_image_att nvm_image;
> + struct qed_hwfn *p_hwfn;
> + bool is_crc = false;
> + u32 image_type;
> + int rc = 0, i;
> + u16 len;
> +
+
> + nvm_image.start_addr = p_hwfn->nvm_info.image_att[i].nvm_start_addr;
> + nvm_image.length = p_hwfn->nvm_info.image_att[i].len;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "Read image %02x; type = %08x; NVM [%08x,...,%08x]\n",
> + **data, nvm_image.start_addr, image_type,
> + nvm_image.start_addr + nvm_image.length - 1);
Looks like 3rd and 4th printed parameters are flipped.
> + (*data)++;
> + is_crc = !!(**data);
If you'd actually want to be able to use the reserved bits
[forward-compatibility] in the future, you should check bit 0 instead of
checking the byte.
> + (*data)++;
> + len = *((u16 *)*data);
> + *data += 2;
[...]
> +
> +/* Binary file format -
> + *
> /----------------------------------------------------------------------\
> + * 0B | 0x3 [command index]
> |
> + * 4B | b'0: check_response? | b'1-127 reserved
> |
This shows there are 128 bits in a 4 byte field.
> + * 8B | File-type | reserved
> |
> + *
> \----------------------------------------------------------------------/
> + * Start a new file of the provided type
> + */
> +static int qed_nvm_flash_image_file_start(struct qed_dev *cdev,
> + const u8 **data, bool *check_resp) {
> + int rc;
> +
> + *data += 4;
> + *check_resp = !!(**data);
Like above
> + *data += 4;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "About to start a new file of type %02x\n", **data);
> + rc = qed_mcp_nvm_put_file_begin(cdev, **data);
> + *data += 4;
> +
> + return rc;
> +}
> +
> +/* Binary file format -
> + *
> /----------------------------------------------------------------------\
> + * 0B | 0x2 [command index]
> |
> + * 4B | Length in bytes
> |
> + * 8B | b'0: check_response? | b'1-127 reserved
> |
Same as above
> + * 12B | Offset in bytes
> |
> + * 16B | Data ...
> |
> + *
> \----------------------------------------------------------------------/
> + * Write data as part of a file that was previously started. Data should
> be
> + * of length equal to that provided in the message
> + */
> +static int qed_nvm_flash_image_file_data(struct qed_dev *cdev,
> + const u8 **data, bool *check_resp) {
> + u32 offset, len;
> + int rc;
> +
> + *data += 4;
> + len = *((u32 *)(*data));
> + *data += 4;
> + *check_resp = !!(**data);
Same as above
> + *data += 4;
> + offset = *((u32 *)(*data));
> + *data += 4;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "About to write File-data: %08x bytes to offset %08x\n",
> + len, offset);
> +
> + rc = qed_mcp_nvm_write(cdev, QED_PUT_FILE_DATA, offset,
> + (char *)(*data), len);
> + *data += len;
> +
> + return rc;
> +}
[...]
> +
> +static int qed_nvm_flash(struct qed_dev *cdev, const char *name) {
> + rc = qed_nvm_flash_image_validate(cdev, image, &data);
> + if (rc)
> + goto exit;
> +
> + while (data < data_end) {
> + bool check_resp = false;
> +
> + /* Parse the actual command */
> + cmd_type = *((u32 *)data);
What's the final format of the file? Is it LE?
<Sudarsana> The file contents are in LE format. Thanks for the detailed
review. Will re-submit the patch series with the review comments incorporated.
> + switch (cmd_type) {
> + case QED_NVM_FLASH_CMD_FILE_DATA:
> + rc = qed_nvm_flash_image_file_data(cdev, &data,
> + &check_resp);
> + break;
> + case QED_NVM_FLASH_CMD_FILE_START:
> + rc = qed_nvm_flash_image_file_start(cdev, &data,
> + &check_resp);
> + break;
> + case QED_NVM_FLASH_CMD_NVM_CHANGE:
> + rc = qed_nvm_flash_image_access(cdev, &data,
> + &check_resp);
> + break;
> + default:
> + DP_ERR(cdev, "Unknown command %08x\n", cmd_type);
> + rc = -EINVAL;
> + break;
Either goto or drop the print; You're getting from the next condition.
> + }
> +
> + if (rc) {
> + DP_ERR(cdev, "Command %08x failed\n", cmd_type);
> + goto exit;
> + }
> +
> + /* Check response if needed */
> + if (check_resp) {
> + u32 mcp_response = 0;
> +
> + if (qed_mcp_nvm_resp(cdev, (u8 *)&mcp_response)) {
> + DP_ERR(cdev, "Failed getting MCP response\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + switch (mcp_response & FW_MSG_CODE_MASK) {
> + case FW_MSG_CODE_OK:
> + case FW_MSG_CODE_NVM_OK:
> + case FW_MSG_CODE_NVM_PUT_FILE_FINISH_OK:
> + case FW_MSG_CODE_PHY_OK:
> + break;
> + default:
> + DP_ERR(cdev, "MFW returns error: %08x\n",
> + mcp_response);
> + rc = -EINVAL;
> + goto exit;
> + }
> + }
> + }
> +
> +exit:
> + release_firmware(image);
> +
> + return rc;
> +}
> +
