RE: [PATCH v3 02/15] net/txgbe: add new SW-FW mailbox interface

["Jiawen Wu" <jiawenwu@trustnetic.com>]

> @@ -113,54 +102,148 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 *buffer,
>  {
>  	u32 hdr_size = sizeof(struct txgbe_hic_hdr);
>  	struct txgbe_hic_hdr *resp = (struct txgbe_hic_hdr *)buffer;
> +	struct txgbe_hic_hdr *recv_hdr;
>  	u16 buf_len;
> -	s32 err;
> -	u32 bi;
> +	s32 err = 0;
> +	u32 bi, i;
>  	u32 dword_len;
> +	u8 send_cmd;
> 
>  	if (length == 0 || length > TXGBE_PMMBX_BSIZE) {
>  		DEBUGOUT("Buffer length failure buffersize=%d.", length);
>  		return TXGBE_ERR_HOST_INTERFACE_COMMAND;
>  	}
> 
> -	/* Take management host interface semaphore */
> -	err = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWMBX);
> -	if (err)
> -		return err;
> +	/* Calculate length in DWORDs. We must be DWORD aligned */
> +	if (length % sizeof(u32)) {
> +		DEBUGOUT("Buffer length failure, not aligned to dword");
> +		return TXGBE_ERR_INVALID_ARGUMENT;
> +	}
> 
> -	err = txgbe_hic_unlocked(hw, buffer, length, timeout);
> -	if (err)
> -		goto rel_out;
> +	if (hw->mac.type == txgbe_mac_aml || hw->mac.type == txgbe_mac_aml40) {
> +		/* try to get lock */
> +		while (rte_atomic32_test_and_set(&hw->swfw_busy)) {
> +			timeout--;
> +			if (!timeout)
> +				return TXGBE_ERR_TIMEOUT;
> +			usec_delay(1000);
> +		}
> +
> +		/* index to unique seq id for each mbox message */
> +		resp->cksum_or_index.index = hw->swfw_index;
> +		send_cmd = resp->cmd;
> +
> +		/* Calculate length in DWORDs */
> +		dword_len = length >> 2;
> +
> +		/* write data to SW-FW mbox array */
> +		for (i = 0; i < dword_len; i++) {
> +			wr32a(hw, TXGBE_AML_MNG_MBOX_SW2FW,
> +					i, rte_cpu_to_le_32(buffer[i]));
> +			/* write flush */
> +			rd32a(hw, TXGBE_AML_MNG_MBOX_SW2FW, i);
> +		}
> +
> +		/* amlite: generate interrupt to notify FW */
> +		wr32m(hw, TXGBE_AML_MNG_MBOX_CTL_SW2FW,
> +				  TXGBE_AML_MNG_MBOX_NOTIFY, 0);
> +		wr32m(hw, TXGBE_AML_MNG_MBOX_CTL_SW2FW,
> +				  TXGBE_AML_MNG_MBOX_NOTIFY, TXGBE_AML_MNG_MBOX_NOTIFY);
> +
> +		/* Calculate length in DWORDs */
> +		dword_len = hdr_size >> 2;
> +
> +		/* polling reply from FW */
> +		timeout = 50;
> +		do {
> +			timeout--;
> +			usec_delay(1000);
> +
> +			/* read hdr */
> +			for (bi = 0; bi < dword_len; bi++)
> +				buffer[bi] = rd32a(hw, TXGBE_AML_MNG_MBOX_FW2SW, bi);
> +
> +			/* check hdr */
> +			recv_hdr = (struct txgbe_hic_hdr *)buffer;
> +
> +			if (recv_hdr->cmd == send_cmd &&
> +			    recv_hdr->cksum_or_index.index == hw->swfw_index)
> +				break;
> +		} while (timeout);
> +
> +		if (!timeout) {
> +			PMD_DRV_LOG(ERR, "Polling from FW messages timeout, cmd is 0x%x, index is %d",
> +				send_cmd, hw->swfw_index);
> +			err = TXGBE_ERR_TIMEOUT;
> +			goto rel_out;
> +		}
> +
> +		/* expect no reply from FW then return */
> +		/* release lock if return */
> +		if (!return_data)
> +			goto rel_out;
> +
> +		/* If there is any thing in data position pull it in */
> +		buf_len = recv_hdr->buf_len;
> +		if (buf_len == 0)
> +			goto rel_out;
> +
> +		if (length < buf_len + hdr_size) {
> +			DEBUGOUT("Buffer not large enough for reply message.");
> +			err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
> +			goto rel_out;
> +		}
> 
> -	if (!return_data)
> -		goto rel_out;
> +		/* Calculate length in DWORDs, add 3 for odd lengths */
> +		dword_len = (buf_len + 3) >> 2;
> +		for (; bi <= dword_len; bi++)
> +			buffer[bi] = rd32a(hw, TXGBE_AML_MNG_MBOX_FW2SW, bi);
> +	} else {
> +		/* Take management host interface semaphore */
> +		err = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWMBX);
> +		if (err)
> +			return err;
> 
> -	/* Calculate length in DWORDs */
> -	dword_len = hdr_size >> 2;
> +		err = txgbe_hic_unlocked(hw, buffer, length, timeout);
> +		if (err)
> +			goto rel_out;
> 
> -	/* first pull in the header so we know the buffer length */
> -	for (bi = 0; bi < dword_len; bi++)
> -		buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> +		if (!return_data)
> +			goto rel_out;
> 
> -	buf_len = resp->buf_len;
> -	if (!buf_len)
> -		goto rel_out;
> +		/* Calculate length in DWORDs */
> +		dword_len = hdr_size >> 2;
> 
> -	if (length < buf_len + hdr_size) {
> -		DEBUGOUT("Buffer not large enough for reply message.");
> -		err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
> -		goto rel_out;
> -	}
> +		/* first pull in the header so we know the buffer length */
> +		for (bi = 0; bi < dword_len; bi++)
> +			buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> +
> +		buf_len = resp->buf_len;
> +		if (!buf_len)
> +			goto rel_out;
> 
> -	/* Calculate length in DWORDs, add 3 for odd lengths */
> -	dword_len = (buf_len + 3) >> 2;
> +		if (length < buf_len + hdr_size) {
> +			DEBUGOUT("Buffer not large enough for reply message.");
> +			err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
> +			goto rel_out;
> +		}
> 
> -	/* Pull in the rest of the buffer (bi is where we left off) */
> -	for (; bi <= dword_len; bi++)
> -		buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> +		/* Calculate length in DWORDs, add 3 for odd lengths */
> +		dword_len = (buf_len + 3) >> 2;
> 
> +		/* Pull in the rest of the buffer (bi is where we left off) */
> +		for (; bi <= dword_len; bi++)
> +			buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> +	}
>  rel_out:
> -	hw->mac.release_swfw_sync(hw, TXGBE_MNGSEM_SWMBX);
> +	if (hw->mac.type == txgbe_mac_aml || hw->mac.type == txgbe_mac_aml40) {
> +		/* index++, index replace txgbe_hic_hdr.checksum */
> +		hw->swfw_index = resp->cksum_or_index.index == TXGBE_HIC_HDR_INDEX_MAX ?
> +					  0 : resp->cksum_or_index.index + 1;
> +		rte_atomic32_clear(&hw->swfw_busy);
> +	} else {
> +		hw->mac.release_swfw_sync(hw, TXGBE_MNGSEM_SWMBX);
> +	}
> 
>  	return err;
>  }

It would be more appropriate to divide then into two functions with
different mac type, as their contents differ greatly.

> @@ -187,22 +276,27 @@ s32 txgbe_hic_sr_read(struct txgbe_hw *hw, u32 addr, u8 *buf, int len)
>  	command.hdr.req.cmd = FW_READ_SHADOW_RAM_CMD;
>  	command.hdr.req.buf_lenh = 0;
>  	command.hdr.req.buf_lenl = FW_READ_SHADOW_RAM_LEN;
> -	command.hdr.req.checksum = FW_DEFAULT_CHECKSUM;
>  	command.address = cpu_to_be32(addr);
>  	command.length = cpu_to_be16(len);
> +	if (hw->mac.type == txgbe_mac_raptor)
> +		command.hdr.req.cksum_or_index.checksum = FW_DEFAULT_CHECKSUM;

This change is unnecessary:

> @@ -68,21 +70,30 @@ struct txgbe_hic_hdr {
>  		u8 cmd_resv;
>  		u8 ret_status;
>  	} cmd_or_resp;
> -	u8 checksum;
> +	union {
> +		u8 checksum;
> +		u8 index;
> +	} cksum_or_index;
>  };
> 
>  struct txgbe_hic_hdr2_req {
>  	u8 cmd;
>  	u8 buf_lenh;
>  	u8 buf_lenl;
> -	u8 checksum;
> +	union {
> +		u8 checksum;
> +		u8 index;
> +	} cksum_or_index;
>  };
> 
>  struct txgbe_hic_hdr2_rsp {
>  	u8 cmd;
>  	u8 buf_lenl;
>  	u8 buf_lenh_status;     /* 7-5: high bits of buf_len, 4-0: status */
> -	u8 checksum;
> +	union {
> +		u8 checksum;
> +		u8 index;
> +	} cksum_or_index;
>  };

No need to name the union.