Add fundamental firmware (FW) communication operations via PF-FW
mailbox, including:
- FW sync (via HW info query with retries)
- HW reset (post FW command to reset hardware)
- MAC address retrieval (request FW for port-specific MAC)
- Power management (powerup/powerdown notification to FW)

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |   4 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    |   1 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 246 ++++++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 121 +++++++++
 5 files changed, 374 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h

diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index 5fc878ada4b1..de8bcb7772ab 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -7,4 +7,5 @@
 obj-$(CONFIG_MGBE) += rnpgbe.o
 rnpgbe-objs := rnpgbe_main.o\
 	       rnpgbe_chip.o\
-	       rnpgbe_mbx.o
+	       rnpgbe_mbx.o\
+	       rnpgbe_mbx_fw.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 7450bfc5ee98..41b580f2168f 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -5,6 +5,7 @@
 #define _RNPGBE_H
 
 #include <linux/types.h>
+#include <linux/mutex.h>
 
 enum rnpgbe_boards {
 	board_n500,
@@ -24,6 +25,8 @@ struct mucse_mbx_info {
 	u32 delay_us;
 	u16 fw_req;
 	u16 fw_ack;
+	/* lock for only one use mbx */
+	struct mutex lock;
 	/* fw <--> pf mbx */
 	u32 fwpf_shm_base;
 	u32 pf2fw_mbx_ctrl;
@@ -34,6 +37,7 @@ struct mucse_mbx_info {
 struct mucse_hw {
 	void __iomem *hw_addr;
 	struct mucse_mbx_info mbx;
+	u8 pfvfnum;
 };
 
 struct mucse {
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
index 7501a55c3134..8998fee2a615 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -416,5 +416,6 @@ void mucse_init_mbx_params_pf(struct mucse_hw *hw)
 	mbx->stats.msgs_rx = 0;
 	mbx->stats.reqs = 0;
 	mbx->stats.acks = 0;
+	mutex_init(&mbx->lock);
 	mucse_mbx_reset(hw);
 }
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
new file mode 100644
index 000000000000..0612d39869fc
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/if_ether.h>
+#include <linux/bitfield.h>
+
+#include "rnpgbe.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * mucse_fw_send_cmd_wait_resp - Send cmd req and wait for response
+ * @hw: pointer to the HW structure
+ * @req: pointer to the cmd req structure
+ * @reply: pointer to the fw reply structure
+ *
+ * mucse_fw_send_cmd_wait_resp sends req to pf-fw mailbox and wait
+ * reply from fw.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+static int mucse_fw_send_cmd_wait_resp(struct mucse_hw *hw,
+				       struct mbx_fw_cmd_req *req,
+				       struct mbx_fw_cmd_reply *reply)
+{
+	int len = le16_to_cpu(req->datalen);
+	int retry_cnt = 3;
+	int err;
+
+	mutex_lock(&hw->mbx.lock);
+	err = mucse_write_and_wait_ack_mbx(hw, (u32 *)req, len);
+	if (err)
+		goto out;
+	do {
+		err = mucse_poll_and_read_mbx(hw, (u32 *)reply,
+					      sizeof(*reply));
+		if (err)
+			goto out;
+		/* mucse_write_and_wait_ack_mbx return 0 means fw has
+		 * received request, wait for the expect opcode
+		 * reply with 'retry_cnt' times.
+		 */
+	} while (--retry_cnt >= 0 && reply->opcode != req->opcode);
+out:
+	mutex_unlock(&hw->mbx.lock);
+	if (!err && retry_cnt < 0)
+		return -ETIMEDOUT;
+	if (!err && reply->error_code)
+		return -EIO;
+
+	return err;
+}
+
+/**
+ * build_get_hw_info_req - build req with GET_HW_INFO opcode
+ * @req: pointer to the cmd req structure
+ **/
+static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(GET_HW_INFO);
+	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+}
+
+/**
+ * mucse_mbx_get_info - Get hw info from fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_get_info tries to get hw info from hw.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+static int mucse_mbx_get_info(struct mucse_hw *hw)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	struct mucse_hw_info info = {};
+	int err;
+
+	build_get_hw_info_req(&req);
+
+	err = mucse_fw_send_cmd_wait_resp(hw, &req, &reply);
+	if (!err) {
+		memcpy(&info, &reply.hw_info, sizeof(struct mucse_hw_info));
+		mucse_hw_info_update_host_endian(&info);
+		hw->pfvfnum = FIELD_GET(GENMASK_U16(7, 0),
+					le16_to_cpu(info.pfnum));
+	}
+
+	return err;
+}
+
+/**
+ * mucse_mbx_sync_fw - Try to sync with fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_sync_fw tries to sync with fw. It is only called in
+ * probe. Nothing (register network) todo if failed.
+ * Try more times to do sync.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_sync_fw(struct mucse_hw *hw)
+{
+	int try_cnt = 3;
+	int err;
+
+	do {
+		err = mucse_mbx_get_info(hw);
+	} while (err == -ETIMEDOUT && try_cnt--);
+
+	return err;
+}
+
+/**
+ * build_powerup - build req with powerup opcode
+ * @req: pointer to the cmd req structure
+ * @is_powerup: true for powerup, false for powerdown
+ **/
+static void build_powerup(struct mbx_fw_cmd_req *req,
+			  bool is_powerup)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(POWER_UP);
+	req->datalen = cpu_to_le16(sizeof(req->powerup) +
+				   MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+	/* fw needs this to reply correct cmd */
+	req->powerup.version = cpu_to_le32(GENMASK_U32(31, 0));
+	if (is_powerup)
+		req->powerup.status = cpu_to_le32(1);
+	else
+		req->powerup.status = cpu_to_le32(0);
+}
+
+/**
+ * mucse_mbx_powerup - Echo fw to powerup
+ * @hw: pointer to the HW structure
+ * @is_powerup: true for powerup, false for powerdown
+ *
+ * mucse_mbx_powerup echo fw to change working frequency
+ * to normal after received true, and reduce working frequency
+ * if false.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_powerup(struct mucse_hw *hw, bool is_powerup)
+{
+	struct mbx_fw_cmd_req req = {};
+	int len;
+	int err;
+
+	build_powerup(&req, is_powerup);
+	len = le16_to_cpu(req.datalen);
+	mutex_lock(&hw->mbx.lock);
+	err = mucse_write_and_wait_ack_mbx(hw, (u32 *)&req, len);
+	mutex_unlock(&hw->mbx.lock);
+
+	return err;
+}
+
+/**
+ * build_reset_hw_req - build req with RESET_HW opcode
+ * @req: pointer to the cmd req structure
+ **/
+static void build_reset_hw_req(struct mbx_fw_cmd_req *req)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(RESET_HW);
+	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+}
+
+/**
+ * mucse_mbx_reset_hw - Posts a mbx req to reset hw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_reset_hw posts a mbx req to firmware to reset hw.
+ * We use mucse_fw_send_cmd_wait_resp to wait hw reset ok.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_reset_hw(struct mucse_hw *hw)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+
+	build_reset_hw_req(&req);
+
+	return mucse_fw_send_cmd_wait_resp(hw, &req, &reply);
+}
+
+/**
+ * build_get_macaddress_req - build req with get_mac opcode
+ * @req: pointer to the cmd req structure
+ * @port_mask: port valid for this cmd
+ * @pfvfnum: pfvfnum for this cmd
+ **/
+static void build_get_macaddress_req(struct mbx_fw_cmd_req *req,
+				     int port_mask, int pfvfnum)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(GET_MAC_ADDRESS);
+	req->datalen = cpu_to_le16(sizeof(req->get_mac_addr) +
+				   MUCSE_MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+	req->get_mac_addr.port_mask = cpu_to_le32(port_mask);
+	req->get_mac_addr.pfvf_num = cpu_to_le32(pfvfnum);
+}
+
+/**
+ * mucse_mbx_get_macaddr - Posts a mbx req to request macaddr
+ * @hw: pointer to the HW structure
+ * @pfvfnum: index of pf/vf num
+ * @mac_addr: pointer to store mac_addr
+ * @port: port index
+ *
+ * mucse_mbx_get_macaddr posts a mbx req to firmware to get mac_addr.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			  u8 *mac_addr,
+			  int port)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	int err;
+
+	build_get_macaddress_req(&req, BIT(port), pfvfnum);
+	err = mucse_fw_send_cmd_wait_resp(hw, &req, &reply);
+	if (err)
+		return err;
+
+	if (le32_to_cpu(reply.mac_addr.ports) & BIT(port))
+		memcpy(mac_addr, reply.mac_addr.addrs[port].mac, ETH_ALEN);
+	else
+		return -ENODATA;
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
new file mode 100644
index 000000000000..9dee6029e630
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_MBX_FW_H
+#define _RNPGBE_MBX_FW_H
+
+#include <linux/types.h>
+
+#include "rnpgbe.h"
+
+#define MUCSE_MBX_REQ_HDR_LEN 24
+
+enum MUCSE_FW_CMD {
+	GET_HW_INFO = 0x0601,
+	GET_MAC_ADDRESS = 0x0602,
+	RESET_HW = 0x0603,
+	POWER_UP = 0x0803,
+};
+
+struct mucse_hw_info {
+	u8 link_stat;
+	u8 port_mask;
+	__le32 speed;
+	__le16 phy_type;
+	__le16 nic_mode;
+	__le16 pfnum;
+	__le32 fw_version;
+	__le32 axi_mhz;
+	union {
+		u8 port_id[4];
+		__le32 port_ids;
+	};
+	__le32 bd_uid;
+	__le32 phy_id;
+	__le32 wol_status;
+	union {
+		__le32 ext_info;
+		struct {
+			u32 valid : 1;
+			u32 wol_en : 1;
+			u32 pci_preset_runtime_en : 1;
+			u32 smbus_en : 1;
+			u32 ncsi_en : 1;
+			u32 rpu_en : 1;
+			u32 v2 : 1;
+			u32 pxe_en : 1;
+			u32 mctp_en : 1;
+			u32 yt8614 : 1;
+			u32 pci_ext_reset : 1;
+			u32 rpu_availble : 1;
+			u32 fw_lldp_ability : 1;
+			u32 lldp_enabled : 1;
+			u32 only_1g : 1;
+			u32 force_down_en: 1;
+		} e_host;
+	};
+} __packed;
+
+/* FW stores extended information in 'ext_info' as a 32-bit
+ * little-endian value. To make these flags easily accessible in the
+ * kernel (via named 'bitfields' instead of raw bitmask operations),
+ * we use the union's 'e_host' struct, which provides named bits
+ * (e.g., 'wol_en', 'smbus_en')
+ */
+static inline void mucse_hw_info_update_host_endian(struct mucse_hw_info *info)
+{
+	u32 host_val = le32_to_cpu(info->ext_info);
+
+	memcpy(&info->e_host, &host_val, sizeof(info->e_host));
+}
+
+struct mbx_fw_cmd_req {
+	__le16 flags;
+	__le16 opcode;
+	__le16 datalen;
+	__le16 ret_value;
+	__le32 cookie_lo;
+	__le32 cookie_hi;
+	__le32 reply_lo;
+	__le32 reply_hi;
+	union {
+		u8 data[32];
+		struct {
+			__le32 version;
+			__le32 status;
+		} powerup;
+		struct {
+			__le32 port_mask;
+			__le32 pfvf_num;
+		} get_mac_addr;
+	};
+} __packed;
+
+struct mbx_fw_cmd_reply {
+	__le16 flags;
+	__le16 opcode;
+	__le16 error_code;
+	__le16 datalen;
+	__le32 cookie_lo;
+	__le32 cookie_hi;
+	union {
+		u8 data[40];
+		struct mac_addr {
+			__le32 ports;
+			struct _addr {
+				/* for macaddr:01:02:03:04:05:06
+				 * mac-hi=0x01020304 mac-lo=0x05060000
+				 */
+				u8 mac[8];
+			} addrs[4];
+		} mac_addr;
+		struct mucse_hw_info hw_info;
+	};
+} __packed;
+
+int mucse_mbx_sync_fw(struct mucse_hw *hw);
+int mucse_mbx_powerup(struct mucse_hw *hw, bool is_powerup);
+int mucse_mbx_reset_hw(struct mucse_hw *hw);
+int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			  u8 *mac_addr, int port);
+#endif /* _RNPGBE_MBX_FW_H */
-- 
2.25.1

On 16/09/2025 12:29, Dong Yibo wrote:
> Add fundamental firmware (FW) communication operations via PF-FW
> mailbox, including:
> - FW sync (via HW info query with retries)
> - HW reset (post FW command to reset hardware)
> - MAC address retrieval (request FW for port-specific MAC)
> - Power management (powerup/powerdown notification to FW)
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

small nits below


> +static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
> +{
> +	req->flags = 0;
> +	req->opcode = cpu_to_le16(GET_HW_INFO);
> +	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
> +	req->reply_lo = 0;
> +	req->reply_hi = 0;
> +}

All these build*() functions re-init flags and reply to 0, but all
mbx_fw_cmd_req are zero-inited on the stack. Might be better clean
things assignments, but no strong opinion because the code is explicit

If you will think of refactoring this part, it might be a good idea to
avoid build*() functions at all and do proper initialization of
mbx_fw_cmd_req in callers?

> +
> +/**
> + * mucse_mbx_get_info - Get hw info from fw
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_get_info tries to get hw info from hw.
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +static int mucse_mbx_get_info(struct mucse_hw *hw)
> +{
> +	struct mbx_fw_cmd_reply reply = {};
> +	struct mbx_fw_cmd_req req = {};

something like:

struct mbx_fw_cmd_req req =
	{
	  .opcode = cpu_to_le16(GET_HW_INFO),
	  .datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN),
	}

On Wed, Sep 17, 2025 at 11:45:31AM +0100, Vadim Fedorenko wrote:
> On 16/09/2025 12:29, Dong Yibo wrote:
> > Add fundamental firmware (FW) communication operations via PF-FW
> > mailbox, including:
> > - FW sync (via HW info query with retries)
> > - HW reset (post FW command to reset hardware)
> > - MAC address retrieval (request FW for port-specific MAC)
> > - Power management (powerup/powerdown notification to FW)
> > 
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> 
> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> 
> small nits below
> 
> 
> > +static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
> > +{
> > +	req->flags = 0;
> > +	req->opcode = cpu_to_le16(GET_HW_INFO);
> > +	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
> > +	req->reply_lo = 0;
> > +	req->reply_hi = 0;
> > +}
> 
> All these build*() functions re-init flags and reply to 0, but all
> mbx_fw_cmd_req are zero-inited on the stack. Might be better clean
> things assignments, but no strong opinion because the code is explicit
> 
> If you will think of refactoring this part, it might be a good idea to
> avoid build*() functions at all and do proper initialization of
> mbx_fw_cmd_req in callers?
> 
> > +
> > +/**
> > + * mucse_mbx_get_info - Get hw info from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_info tries to get hw info from hw.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int mucse_mbx_get_info(struct mucse_hw *hw)
> > +{
> > +	struct mbx_fw_cmd_reply reply = {};
> > +	struct mbx_fw_cmd_req req = {};
> 
> something like:
> 
> struct mbx_fw_cmd_req req =
> 	{
> 	  .opcode = cpu_to_le16(GET_HW_INFO),
> 	  .datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN),
> 	}
> 
> 
> 

That's a good idea! That makes the code more compact.
I think I should update this as your suggestion.

Regarding adding your "Reviewed-by" tag in the next version:
Would it be acceptable to include it when I submit the updated patch (with
the initialization logic adjusted), or should I wait for your further
review of the modified code first?

Thanks for your feedback.

On 17/09/2025 12:05, Yibo Dong wrote:
> On Wed, Sep 17, 2025 at 11:45:31AM +0100, Vadim Fedorenko wrote:
>> On 16/09/2025 12:29, Dong Yibo wrote:
>>> Add fundamental firmware (FW) communication operations via PF-FW
>>> mailbox, including:
>>> - FW sync (via HW info query with retries)
>>> - HW reset (post FW command to reset hardware)
>>> - MAC address retrieval (request FW for port-specific MAC)
>>> - Power management (powerup/powerdown notification to FW)
>>>
>>> Signed-off-by: Dong Yibo <dong100@mucse.com>
>>
>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>>
>> small nits below
>>
>>
>>> +static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
>>> +{
>>> +	req->flags = 0;
>>> +	req->opcode = cpu_to_le16(GET_HW_INFO);
>>> +	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
>>> +	req->reply_lo = 0;
>>> +	req->reply_hi = 0;
>>> +}
>>
>> All these build*() functions re-init flags and reply to 0, but all
>> mbx_fw_cmd_req are zero-inited on the stack. Might be better clean
>> things assignments, but no strong opinion because the code is explicit
>>
>> If you will think of refactoring this part, it might be a good idea to
>> avoid build*() functions at all and do proper initialization of
>> mbx_fw_cmd_req in callers?
>>
>>> +
>>> +/**
>>> + * mucse_mbx_get_info - Get hw info from fw
>>> + * @hw: pointer to the HW structure
>>> + *
>>> + * mucse_mbx_get_info tries to get hw info from hw.
>>> + *
>>> + * Return: 0 on success, negative errno on failure
>>> + **/
>>> +static int mucse_mbx_get_info(struct mucse_hw *hw)
>>> +{
>>> +	struct mbx_fw_cmd_reply reply = {};
>>> +	struct mbx_fw_cmd_req req = {};
>>
>> something like:
>>
>> struct mbx_fw_cmd_req req =
>> 	{
>> 	  .opcode = cpu_to_le16(GET_HW_INFO),
>> 	  .datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN),
>> 	}
>>
>>
>>
> 
> That's a good idea! That makes the code more compact.
> I think I should update this as your suggestion.
> 
> Regarding adding your "Reviewed-by" tag in the next version:
> Would it be acceptable to include it when I submit the updated patch (with
> the initialization logic adjusted), or should I wait for your further
> review of the modified code first?

If you will submit another version with this refactoring, I'll better do
another review.

On Wed, Sep 17, 2025 at 03:02:13PM +0100, Vadim Fedorenko wrote:
> On 17/09/2025 12:05, Yibo Dong wrote:
> > On Wed, Sep 17, 2025 at 11:45:31AM +0100, Vadim Fedorenko wrote:
> > > On 16/09/2025 12:29, Dong Yibo wrote:
> > > > Add fundamental firmware (FW) communication operations via PF-FW
> > > > mailbox, including:
> > > > - FW sync (via HW info query with retries)
> > > > - HW reset (post FW command to reset hardware)
> > > > - MAC address retrieval (request FW for port-specific MAC)
> > > > - Power management (powerup/powerdown notification to FW)
> > > > 
> > > > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > > 
> > > Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> > > 
> > > small nits below
> > > 
> > > 
> > > > +static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
> > > > +{
> > > > +	req->flags = 0;
> > > > +	req->opcode = cpu_to_le16(GET_HW_INFO);
> > > > +	req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
> > > > +	req->reply_lo = 0;
> > > > +	req->reply_hi = 0;
> > > > +}
> > > 
> > > All these build*() functions re-init flags and reply to 0, but all
> > > mbx_fw_cmd_req are zero-inited on the stack. Might be better clean
> > > things assignments, but no strong opinion because the code is explicit
> > > 
> > > If you will think of refactoring this part, it might be a good idea to
> > > avoid build*() functions at all and do proper initialization of
> > > mbx_fw_cmd_req in callers?
> > > 
> > > > +
> > > > +/**
> > > > + * mucse_mbx_get_info - Get hw info from fw
> > > > + * @hw: pointer to the HW structure
> > > > + *
> > > > + * mucse_mbx_get_info tries to get hw info from hw.
> > > > + *
> > > > + * Return: 0 on success, negative errno on failure
> > > > + **/
> > > > +static int mucse_mbx_get_info(struct mucse_hw *hw)
> > > > +{
> > > > +	struct mbx_fw_cmd_reply reply = {};
> > > > +	struct mbx_fw_cmd_req req = {};
> > > 
> > > something like:
> > > 
> > > struct mbx_fw_cmd_req req =
> > > 	{
> > > 	  .opcode = cpu_to_le16(GET_HW_INFO),
> > > 	  .datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN),
> > > 	}
> > > 
> > > 
> > > 
> > 
> > That's a good idea! That makes the code more compact.
> > I think I should update this as your suggestion.
> > 
> > Regarding adding your "Reviewed-by" tag in the next version:
> > Would it be acceptable to include it when I submit the updated patch (with
> > the initialization logic adjusted), or should I wait for your further
> > review of the modified code first?
> 
> If you will submit another version with this refactoring, I'll better do
> another review.
> 

I see, I will submit another version later, with this refactoring.
Looking forward to your next review.

Thanks for your feedback.