Initialize basic mbx_fw ops, such as get_capability, reset phy
and so on.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |   1 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 253 ++++++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 126 +++++++++
 4 files changed, 382 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 7999bb99306b..4d2cca59fb23 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -51,6 +51,7 @@ struct mucse_hw {
 	void __iomem *hw_addr;
 	struct pci_dev *pdev;
 	enum rnpgbe_hw_type hw_type;
+	u8 pfvfnum;
 	struct mucse_mbx_info mbx;
 };
 
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..fe626deca3ed
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/pci.h>
+#include <linux/if_ether.h>
+
+#include "rnpgbe.h"
+#include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * mucse_fw_send_cmd_wait - 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 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(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;
+
+	err = mutex_lock_interruptible(&hw->mbx.lock);
+	if (err)
+		return err;
+	err = mucse_write_posted_mbx(hw, (u32 *)req, len);
+	if (err)
+		goto out;
+	do {
+		err = mucse_read_posted_mbx(hw, (u32 *)reply,
+					    sizeof(*reply));
+		if (err)
+			goto out;
+		/* mucse_write_posted_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_phy_abilities_req - build req with get_phy_ability opcode
+ * @req: pointer to the cmd req structure
+ **/
+static void build_phy_abilities_req(struct mbx_fw_cmd_req *req)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(GET_PHY_ABILITY);
+	req->datalen = cpu_to_le16(MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+}
+
+/**
+ * mucse_fw_get_capability - Get hw abilities from fw
+ * @hw: pointer to the HW structure
+ * @abil: pointer to the hw_abilities structure
+ *
+ * mucse_fw_get_capability tries to get hw abilities from
+ * hw.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+static int mucse_fw_get_capability(struct mucse_hw *hw,
+				   struct hw_abilities *abil)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	int err;
+
+	build_phy_abilities_req(&req);
+	err = mucse_fw_send_cmd_wait(hw, &req, &reply);
+	if (!err)
+		memcpy(abil, &reply.hw_abilities, sizeof(*abil));
+	return err;
+}
+
+/**
+ * mucse_mbx_get_capability - Get hw abilities from fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_get_capability tries to get capabities from
+ * hw. Many retrys will do if it is failed.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_get_capability(struct mucse_hw *hw)
+{
+	struct hw_abilities ability = {};
+	int try_cnt = 3;
+	int err;
+	/* It is called once in probe, if failed nothing
+	 * (register network) todo. Try more times to get driver
+	 * and firmware in sync.
+	 */
+	do {
+		err = mucse_fw_get_capability(hw, &ability);
+		if (err)
+			continue;
+		break;
+	} while (try_cnt--);
+
+	if (!err)
+		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
+	return err;
+}
+
+/**
+ * build_ifinsmod - build req with insmod opcode
+ * @req: pointer to the cmd req structure
+ * @is_insmod: true for insmod, false for rmmod
+ **/
+static void build_ifinsmod(struct mbx_fw_cmd_req *req,
+			   bool is_insmod)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(DRIVER_INSMOD);
+	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
+				   MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+#define FIXED_VERSION 0xFFFFFFFF
+	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
+	if (is_insmod)
+		req->ifinsmod.status = cpu_to_le32(1);
+	else
+		req->ifinsmod.status = cpu_to_le32(0);
+}
+
+/**
+ * mucse_mbx_ifinsmod - Echo driver insmod status to hw
+ * @hw: pointer to the HW structure
+ * @is_insmod: true for insmod, false for rmmod
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_ifinsmod(struct mucse_hw *hw, bool is_insmod)
+{
+	struct mbx_fw_cmd_req req = {};
+	int len;
+	int err;
+
+	build_ifinsmod(&req, is_insmod);
+	len = le16_to_cpu(req.datalen);
+	err = mutex_lock_interruptible(&hw->mbx.lock);
+	if (err)
+		return err;
+
+	if (is_insmod) {
+		err = mucse_write_posted_mbx(hw, (u32 *)&req,
+					     len);
+	} else {
+		err = mucse_write_mbx_pf(hw, (u32 *)&req,
+					 len);
+	}
+
+	mutex_unlock(&hw->mbx.lock);
+	return err;
+}
+
+/**
+ * build_reset_phy_req - build req with reset_phy opcode
+ * @req: pointer to the cmd req structure
+ **/
+static void build_reset_phy_req(struct mbx_fw_cmd_req *req)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(RESET_PHY);
+	req->datalen = cpu_to_le16(MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+}
+
+/**
+ * mucse_mbx_fw_reset_phy - Posts a mbx req to reset hw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_fw_reset_phy posts a mbx req to firmware to reset hw.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_fw_reset_phy(struct mucse_hw *hw)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+
+	build_reset_phy_req(&req);
+	return mucse_fw_send_cmd_wait(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_ADDRES);
+	req->datalen = cpu_to_le16(sizeof(req->get_mac_addr) +
+				   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_fw_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_fw_get_macaddr posts a mbx req to firmware to get mac_addr.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_fw_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(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..3efd23ba1aa0
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -0,0 +1,126 @@
+/* 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 <linux/errno.h>
+#include <linux/wait.h>
+
+#include "rnpgbe.h"
+
+#define MBX_REQ_HDR_LEN 24
+
+enum MUCSE_FW_CMD {
+	GET_PHY_ABILITY = 0x0601,
+	GET_MAC_ADDRES = 0x0602,
+	RESET_PHY = 0x0603,
+	DRIVER_INSMOD = 0x0803,
+};
+
+struct hw_abilities {
+	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_ability;
+		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 ability information in 'ext_ability' 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 ability_update_host_endian(struct hw_abilities *abi)
+{
+	u32 host_val = le32_to_cpu(abi->ext_ability);
+
+	abi->e_host = *(typeof(abi->e_host) *)&host_val;
+}
+
+#define FLAGS_DD BIT(0)
+#define FLAGS_ERR BIT(2)
+
+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;
+		} ifinsmod;
+		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 hw_abilities hw_abilities;
+	};
+} __packed;
+
+int mucse_mbx_get_capability(struct mucse_hw *hw);
+int mucse_mbx_ifinsmod(struct mucse_hw *hw, bool status);
+int mucse_mbx_fw_reset_phy(struct mucse_hw *hw);
+int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			 u8 *mac_addr, int port);
+#endif /* _RNPGBE_MBX_FW_H */
-- 
2.25.1

> +/**
> + * mucse_mbx_get_capability - Get hw abilities from fw
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_get_capability tries to get capabities from
> + * hw. Many retrys will do if it is failed.
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +int mucse_mbx_get_capability(struct mucse_hw *hw)
> +{
> +	struct hw_abilities ability = {};
> +	int try_cnt = 3;
> +	int err;
> +	/* It is called once in probe, if failed nothing
> +	 * (register network) todo. Try more times to get driver
> +	 * and firmware in sync.
> +	 */
> +	do {
> +		err = mucse_fw_get_capability(hw, &ability);
> +		if (err)
> +			continue;
> +		break;
> +	} while (try_cnt--);
> +
> +	if (!err)
> +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> +	return err;
> +}

I still think this should be a dedicated function to get the MAC
driver and firmware in sync, using a NOP or version request to the
firmware. The name mucse_mbx_get_capability() does not indicate this
function is special in any way, which is it.

> +/**
> + * build_ifinsmod - build req with insmod opcode
> + * @req: pointer to the cmd req structure
> + * @is_insmod: true for insmod, false for rmmod
> + **/
> +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> +			   bool is_insmod)
> +{
> +	req->flags = 0;
> +	req->opcode = cpu_to_le16(DRIVER_INSMOD);
> +	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> +				   MBX_REQ_HDR_LEN);
> +	req->reply_lo = 0;
> +	req->reply_hi = 0;
> +#define FIXED_VERSION 0xFFFFFFFF
> +	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> +	if (is_insmod)
> +		req->ifinsmod.status = cpu_to_le32(1);
> +	else
> +		req->ifinsmod.status = cpu_to_le32(0);
> +}

Why does the firmware care? What does the firmware do when there is no
kernel driver? How does it behaviour change when the driver loads?

Please try to ensure comment say why you are doing something, not what
you are doing.


    Andrew

---
pw-bot: cr

On Thu, Aug 28, 2025 at 03:09:51PM +0200, Andrew Lunn wrote:
> > +/**
> > + * mucse_mbx_get_capability - Get hw abilities from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_capability tries to get capabities from
> > + * hw. Many retrys will do if it is failed.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > +{
> > +	struct hw_abilities ability = {};
> > +	int try_cnt = 3;
> > +	int err;
> > +	/* It is called once in probe, if failed nothing
> > +	 * (register network) todo. Try more times to get driver
> > +	 * and firmware in sync.
> > +	 */
> > +	do {
> > +		err = mucse_fw_get_capability(hw, &ability);
> > +		if (err)
> > +			continue;
> > +		break;
> > +	} while (try_cnt--);
> > +
> > +	if (!err)
> > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > +	return err;
> > +}
> 
> I still think this should be a dedicated function to get the MAC
> driver and firmware in sync, using a NOP or version request to the
> firmware. The name mucse_mbx_get_capability() does not indicate this
> function is special in any way, which is it.
> 

Maybe I should rename it like this?

/**
 * mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
 * @hw: pointer to the HW structure
 *
 * mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
 * by get capabitiy mbx cmd. Many retrys will do if it is failed.
 *
 * Return: 0 on success, negative errno on failure
 **/
int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
{
	struct hw_abilities ability = {};
	int try_cnt = 3;
	int err;
	/* It is called once in probe, if failed nothing
	 * (register network) todo. Try more times to get driver
	 * and firmware in sync.
	 */
	do {
		err = mucse_fw_get_capability(hw, &ability);
		if (err)
			continue;
		break;
	} while (try_cnt--);

	if (!err)
		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
	return err;
}

> > +/**
> > + * build_ifinsmod - build req with insmod opcode
> > + * @req: pointer to the cmd req structure
> > + * @is_insmod: true for insmod, false for rmmod
> > + **/
> > +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> > +			   bool is_insmod)
> > +{
> > +	req->flags = 0;
> > +	req->opcode = cpu_to_le16(DRIVER_INSMOD);
> > +	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> > +				   MBX_REQ_HDR_LEN);
> > +	req->reply_lo = 0;
> > +	req->reply_hi = 0;
> > +#define FIXED_VERSION 0xFFFFFFFF
> > +	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> > +	if (is_insmod)
> > +		req->ifinsmod.status = cpu_to_le32(1);
> > +	else
> > +		req->ifinsmod.status = cpu_to_le32(0);
> > +}
> 
> Why does the firmware care? What does the firmware do when there is no
> kernel driver? How does it behaviour change when the driver loads?
> 

fw reduce working frequency to save power if no driver is probed to this
chip. And fw change frequency to normal after recieve insmod mbx cmd.

Maybe I should add the comment to func "mucse_mbx_ifinsmod"? 
/**
 * mucse_mbx_ifinsmod - Echo driver insmod status to fw
 * @hw: pointer to the HW structure
 * @is_insmod: true for insmod, false for rmmod
 *
 * mucse_mbx_ifinsmod echo driver insmod status to fw. fw changes working
 * frequency to normal after recieve insmod status, and reduce working
 * frequency if no driver is probed.
 *
 * Return: 0 on success, negative errno on failure
 **/
int mucse_mbx_ifinsmod(struct mucse_hw *hw, bool is_insmod)
{


}

> Please try to ensure comment say why you are doing something, not what
> you are doing.
> 
> 
>     Andrew
> 
> ---
> pw-bot: cr
> 

Thanks for your feedback.

> Maybe I should rename it like this?
> 
> /**
>  * mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
>  * @hw: pointer to the HW structure
>  *
>  * mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
>  * by get capabitiy mbx cmd. Many retrys will do if it is failed.
>  *
>  * Return: 0 on success, negative errno on failure
>  **/
> int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
> {
> 	struct hw_abilities ability = {};
> 	int try_cnt = 3;
> 	int err;
> 	/* It is called once in probe, if failed nothing
> 	 * (register network) todo. Try more times to get driver
> 	 * and firmware in sync.
> 	 */
> 	do {
> 		err = mucse_fw_get_capability(hw, &ability);
> 		if (err)
> 			continue;
> 		break;
> 	} while (try_cnt--);
> 
> 	if (!err)
> 		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> 	return err;
> }

Why so much resistance to a NOP or firmware version, something which
is not that important? Why do you want to combine getting sync and
getting the capabilities?

> fw reduce working frequency to save power if no driver is probed to this
> chip. And fw change frequency to normal after recieve insmod mbx cmd.

So why is this called ifinsmod? Why not power save? If you had called
this power save, i would not of questioned what this does, it is
pretty obvious, and other drivers probably have something
similar. Some drivers probably have something like open/close, which
do similar things. Again, i would not of asked. By not following what
other drivers are doing, you just cause problems for everybody.

So please give this a new name. Not just the function, but also the
name of the firmware op and everything else to do with this. The
firmware does not care what the driver calls it, all it sees is a
binary message format, no names.

Please also go through your driver and look at all the other names. Do
they match what other drivers use. If not, you might want to rename
them, in order to get your code merged with a lot less back and forth
with reviewers.

	Andrew

On Fri, Aug 29, 2025 at 09:48:12PM +0200, Andrew Lunn wrote:
> > Maybe I should rename it like this?
> > 
> > /**
> >  * mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
> >  * @hw: pointer to the HW structure
> >  *
> >  * mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
> >  * by get capabitiy mbx cmd. Many retrys will do if it is failed.
> >  *
> >  * Return: 0 on success, negative errno on failure
> >  **/
> > int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
> > {
> > 	struct hw_abilities ability = {};
> > 	int try_cnt = 3;
> > 	int err;
> > 	/* It is called once in probe, if failed nothing
> > 	 * (register network) todo. Try more times to get driver
> > 	 * and firmware in sync.
> > 	 */
> > 	do {
> > 		err = mucse_fw_get_capability(hw, &ability);
> > 		if (err)
> > 			continue;
> > 		break;
> > 	} while (try_cnt--);
> > 
> > 	if (!err)
> > 		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > 	return err;
> > }
> 
> Why so much resistance to a NOP or firmware version, something which
> is not that important? Why do you want to combine getting sync and
> getting the capabilities?
> 
Maybe like this? 
mucse_mbx_sync_fw is called in probe with try_cnt. 
mucse_mbx_get_info is the same as old mucse_mbx_get_capability.
One function one purpose, not combine two.

static int mucse_mbx_get_info(struct mucse_hw *hw)
{
        struct mbx_fw_cmd_reply reply = {};
        struct mbx_fw_cmd_req req = {};
        struct hw_info info = {};
        int err;

        build_get_fw_info_req(&req);
        err = mucse_fw_send_cmd_wait(hw, &req, &reply);
        if (!err) {
                memcpy(&info, &reply.hw_info, sizeof(struct hw_info));
                hw->pfvfnum = le16_to_cpu(info.pfnum) & GENMASK_U16(7, 0);
        }

        return err;
}

/**
 * mucse_mbx_sync_fw - Try to sync with fw
 * @hw: pointer to the HW structure
 *
 * mucse_mbx_sync_fw tries get sync to fw hw.
 * It is only called in probe
 *
 * 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);
                if (err == -ETIMEDOUT)
                        continue;
                break;
        } while (try_cnt--);

        return err;
}

> static int mucse_mbx_get_info(struct mucse_hw *hw)
> {
>         struct mbx_fw_cmd_reply reply = {};
>         struct mbx_fw_cmd_req req = {};
>         struct hw_info info = {};
>         int err;
> 
>         build_get_fw_info_req(&req);
>         err = mucse_fw_send_cmd_wait(hw, &req, &reply);
>         if (!err) {
>                 memcpy(&info, &reply.hw_info, sizeof(struct hw_info));
>                 hw->pfvfnum = le16_to_cpu(info.pfnum) & GENMASK_U16(7, 0);
>         }
> 
>         return err;
> }
> 
> /**
>  * mucse_mbx_sync_fw - Try to sync with fw
>  * @hw: pointer to the HW structure
>  *
>  * mucse_mbx_sync_fw tries get sync to fw hw.
>  * It is only called in probe
>  *
>  * 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);
>                 if (err == -ETIMEDOUT)
>                         continue;
>                 break;
>         } while (try_cnt--);
> 
>         return err;
> }

This looks O.K.

     Andrew

On Fri, Aug 29, 2025 at 09:48:12PM +0200, Andrew Lunn wrote:
> > Maybe I should rename it like this?
> > 
> > /**
> >  * mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
> >  * @hw: pointer to the HW structure
> >  *
> >  * mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
> >  * by get capabitiy mbx cmd. Many retrys will do if it is failed.
> >  *
> >  * Return: 0 on success, negative errno on failure
> >  **/
> > int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
> > {
> > 	struct hw_abilities ability = {};
> > 	int try_cnt = 3;
> > 	int err;
> > 	/* It is called once in probe, if failed nothing
> > 	 * (register network) todo. Try more times to get driver
> > 	 * and firmware in sync.
> > 	 */
> > 	do {
> > 		err = mucse_fw_get_capability(hw, &ability);
> > 		if (err)
> > 			continue;
> > 		break;
> > 	} while (try_cnt--);
> > 
> > 	if (!err)
> > 		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > 	return err;
> > }
> 
> Why so much resistance to a NOP or firmware version, something which
> is not that important? Why do you want to combine getting sync and
> getting the capabilities?
> 

But firmware not offer a NOP command.
(https://lore.kernel.org/netdev/8989E7A85A9468B0+20250825013053.GA2006401@nic-Precision-5820-Tower/) 

I will rename it like 'mucse_mbx_sync_fw', and rename opcode
'GET_PHY_ABILITY = 0x0601' to 'SYNC_FW = 0x0601'.

> > fw reduce working frequency to save power if no driver is probed to this
> > chip. And fw change frequency to normal after recieve insmod mbx cmd.
> 
> So why is this called ifinsmod? Why not power save? If you had called
> this power save, i would not of questioned what this does, it is
> pretty obvious, and other drivers probably have something
> similar. Some drivers probably have something like open/close, which
> do similar things. Again, i would not of asked. By not following what
> other drivers are doing, you just cause problems for everybody.

Sorry for it.

> 
> So please give this a new name. Not just the function, but also the
> name of the firmware op and everything else to do with this. The
> firmware does not care what the driver calls it, all it sees is a
> binary message format, no names.
> 
> Please also go through your driver and look at all the other names. Do
> they match what other drivers use. If not, you might want to rename
> them, in order to get your code merged with a lot less back and forth
> with reviewers.
> 

I see, I will check all names.

> 	Andrew
> 

Thanks for you feedback.