RE: [PATCH net-next v2 01/16] net: txgbe: Store PCI info

On Wednesday, August 31, 2022 8:07 AM, Andrew Lunn wrote:

quoted

+/* cmd_addr is used for some special command:
+ * 1. to be sector address, when implemented erase sector command
+ * 2. to be flash address when implemented read, write flash address
+*/ int txgbe_fmgr_cmd_op(struct txgbe_hw *hw, u32 cmd, u32 cmd_addr)
+{
+	u32 cmd_val = 0, val = 0;
+
+	cmd_val = (cmd << SPI_CLK_CMD_OFFSET) |
+		  (SPI_CLK_DIV << SPI_CLK_DIV_OFFSET) | cmd_addr;
+	wr32(hw, SPI_H_CMD_REG_ADDR, cmd_val);
+
+	return read_poll_timeout(rd32, val, (val & 0x1), 10,

SPI_TIME_OUT_VALUE,

quoted

+				 false, hw, SPI_H_STA_REG_ADDR);
+}
+

quoted

+int txgbe_flash_read_dword(struct txgbe_hw *hw, u32 addr, u32 *data)
+{
+	int ret = 0;
+
+	ret = txgbe_fmgr_cmd_op(hw, SPI_CMD_READ_DWORD, addr);
+	if (ret == -ETIMEDOUT)
+		return ret;

Are you absolutely sure it will never return any other error code?
The pattern in the kernel is

quoted

+	if (ret)
+		return ret;

or

quoted

+	if (ret < 0)
+		return ret;

I found that the function 'read_poll_timeout()' only returns 0 or
-ETIMEDOUT.
But I'll fix it to be safe.

quoted

+int txgbe_check_flash_load(struct txgbe_hw *hw, u32 check_bit) {
+	u32 i = 0, reg = 0;
+	int err = 0;
+
+	/* if there's flash existing */
+	if (!(rd32(hw, TXGBE_SPI_STATUS) &
+	      TXGBE_SPI_STATUS_FLASH_BYPASS)) {
+		/* wait hw load flash done */
+		for (i = 0; i < TXGBE_MAX_FLASH_LOAD_POLL_TIME; i++) {
+			reg = rd32(hw, TXGBE_SPI_ILDR_STATUS);
+			if (!(reg & check_bit)) {
+				/* done */
+				break;
+			}
+			msleep(200);
+		}

This is what iopoll.h is for. Any sort of loop waiting for something to

happen

should use one of the helpers in there.

The description of functions in iopoll.h states that maximum sleep time
should be less than 20ms, is it okay to call it here for 200ms.