Initialize get mac from hw, register the netdev.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 18 +++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 73 ++++++++++++++++++
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  1 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 75 +++++++++++++++++++
 4 files changed, 167 insertions(+)

diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 7ab1cbb432f6..7e51a8871b71 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/netdevice.h>
 
 extern const struct rnpgbe_info rnpgbe_n500_info;
 extern const struct rnpgbe_info rnpgbe_n210_info;
@@ -82,6 +83,15 @@ struct mucse_mbx_info {
 	u32 fw2pf_mbox_vec;
 };
 
+struct mucse_hw_operations {
+	int (*reset_hw)(struct mucse_hw *hw);
+	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
+};
+
+enum {
+	mucse_driver_insmod,
+};
+
 struct mucse_hw {
 	u8 pfvfnum;
 	void __iomem *hw_addr;
@@ -91,12 +101,17 @@ struct mucse_hw {
 	u32 axi_mhz;
 	u32 bd_uid;
 	enum rnpgbe_hw_type hw_type;
+	const struct mucse_hw_operations *ops;
 	struct mucse_dma_info dma;
 	struct mucse_eth_info eth;
 	struct mucse_mac_info mac;
 	struct mucse_mbx_info mbx;
+	u32 flags;
+#define M_FLAGS_INIT_MAC_ADDRESS BIT(0)
 	u32 driver_version;
 	u16 usecstocount;
+	int lane;
+	u8 perm_addr[ETH_ALEN];
 };
 
 struct mucse {
@@ -117,4 +132,7 @@ struct rnpgbe_info {
 #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
 #define PCI_DEVICE_ID_N210 0x8208
 #define PCI_DEVICE_ID_N210L 0x820a
+
+#define dma_wr32(dma, reg, val) writel((val), (dma)->dma_base_addr + (reg))
+#define dma_rd32(dma, reg) readl((dma)->dma_base_addr + (reg))
 #endif /* _RNPGBE_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index e0c6f47efd4c..aba44b31eae3 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -1,11 +1,83 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2020 - 2025 Mucse Corporation. */
 
+#include <linux/pci.h>
 #include <linux/string.h>
+#include <linux/etherdevice.h>
 
 #include "rnpgbe.h"
 #include "rnpgbe_hw.h"
 #include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * rnpgbe_get_permanent_mac - Get permanent mac
+ * @hw: hw information structure
+ * @mac_addr: pointer to store mac
+ *
+ * rnpgbe_get_permanent_mac tries to get mac from hw.
+ * It use eth_random_addr if failed.
+ **/
+static void rnpgbe_get_permanent_mac(struct mucse_hw *hw,
+				     u8 *mac_addr)
+{
+	struct device *dev = &hw->pdev->dev;
+
+	if (mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->lane) ||
+	    !is_valid_ether_addr(mac_addr)) {
+		dev_warn(dev, "Failed to get valid MAC from FW, using random\n");
+		eth_random_addr(mac_addr);
+	}
+
+	hw->flags |= M_FLAGS_INIT_MAC_ADDRESS;
+}
+
+/**
+ * rnpgbe_reset_hw_ops - Do a hardware reset
+ * @hw: hw information structure
+ *
+ * rnpgbe_reset_hw_ops calls fw to do a hardware
+ * reset, and cleans some regs to default.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
+{
+	struct mucse_dma_info *dma = &hw->dma;
+	int err;
+
+	dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
+	err = mucse_mbx_fw_reset_phy(hw);
+	if (err)
+		return err;
+	/* Store the permanent mac address */
+	if (!(hw->flags & M_FLAGS_INIT_MAC_ADDRESS))
+		rnpgbe_get_permanent_mac(hw, hw->perm_addr);
+
+	return 0;
+}
+
+/**
+ * rnpgbe_driver_status_hw_ops - Echo driver status to hw
+ * @hw: hw information structure
+ * @enable: true or false status
+ * @mode: status mode
+ **/
+static void rnpgbe_driver_status_hw_ops(struct mucse_hw *hw,
+					bool enable,
+					int mode)
+{
+	switch (mode) {
+	case mucse_driver_insmod:
+		mucse_mbx_ifinsmod(hw, enable);
+		break;
+	}
+}
+
+static const struct mucse_hw_operations rnpgbe_hw_ops = {
+	.reset_hw = &rnpgbe_reset_hw_ops,
+	.driver_status = &rnpgbe_driver_status_hw_ops,
+};
 
 /**
  * rnpgbe_init_common - Setup common attribute
@@ -25,6 +97,7 @@ static void rnpgbe_init_common(struct mucse_hw *hw)
 	mac->mac_addr = hw->hw_addr + RNPGBE_MAC_BASE;
 
 	hw->mbx.ops = &mucse_mbx_ops_generic;
+	hw->ops = &rnpgbe_hw_ops;
 }
 
 /**
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index aee037e3219d..4e07328ccf82 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -9,6 +9,7 @@
 #define RNPGBE_ETH_BASE 0x10000
 /**************** DMA Registers ****************************/
 #define RNPGBE_DMA_DUMY 0x000c
+#define RNPGBE_DMA_AXI_EN 0x0010
 /**************** CHIP Resource ****************************/
 #define RNPGBE_MAX_QUEUES 8
 #endif /* _RNPGBE_HW_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index 1fef7fa30208..a377ecaa0da5 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -8,6 +8,7 @@
 #include <linux/etherdevice.h>
 
 #include "rnpgbe.h"
+#include "rnpgbe_mbx_fw.h"
 
 static const char rnpgbe_driver_name[] = "rnpgbe";
 static const struct rnpgbe_info *rnpgbe_info_tbl[] = {
@@ -34,6 +35,55 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
 	{0, },
 };
 
+/**
+ * rnpgbe_open - Called when a network interface is made active
+ * @netdev: network interface device structure
+ *
+ * The open entry point is called when a network interface is made
+ * active by the system (IFF_UP).
+ *
+ * @return: 0 on success, negative value on failure
+ **/
+static int rnpgbe_open(struct net_device *netdev)
+{
+	return 0;
+}
+
+/**
+ * rnpgbe_close - Disables a network interface
+ * @netdev: network interface device structure
+ *
+ * The close entry point is called when an interface is de-activated
+ * by the OS.
+ *
+ * @return: 0, this is not allowed to fail
+ **/
+static int rnpgbe_close(struct net_device *netdev)
+{
+	return 0;
+}
+
+/**
+ * rnpgbe_xmit_frame - Send a skb to driver
+ * @skb: skb structure to be sent
+ * @netdev: network interface device structure
+ *
+ * @return: NETDEV_TX_OK or NETDEV_TX_BUSY
+ **/
+static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
+				     struct net_device *netdev)
+{
+		dev_kfree_skb_any(skb);
+		netdev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops rnpgbe_netdev_ops = {
+	.ndo_open = rnpgbe_open,
+	.ndo_stop = rnpgbe_close,
+	.ndo_start_xmit = rnpgbe_xmit_frame,
+};
+
 /**
  * rnpgbe_add_adapter - Add netdev for this pci_dev
  * @pdev: PCI device information structure
@@ -103,6 +153,27 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
 	hw->dma.dma_version = dma_version;
 	hw->driver_version = 0x0002040f;
 	info->init(hw);
+	hw->mbx.ops->init_params(hw);
+	/* echo fw driver insmod to control hw */
+	hw->ops->driver_status(hw, true, mucse_driver_insmod);
+	err = mucse_mbx_get_capability(hw);
+	if (err) {
+		dev_err(&pdev->dev,
+			"mucse_mbx_get_capability failed! %d\n",
+			err);
+		goto err_free_net;
+	}
+	netdev->netdev_ops = &rnpgbe_netdev_ops;
+	netdev->watchdog_timeo = 5 * HZ;
+	err = hw->ops->reset_hw(hw);
+	if (err) {
+		dev_err(&pdev->dev, "Hw reset failed %d\n", err);
+		goto err_free_net;
+	}
+	eth_hw_addr_set(netdev, hw->perm_addr);
+	err = register_netdev(netdev);
+	if (err)
+		goto err_free_net;
 	return 0;
 
 err_free_net:
@@ -166,12 +237,16 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 static void rnpgbe_rm_adapter(struct pci_dev *pdev)
 {
 	struct mucse *mucse = pci_get_drvdata(pdev);
+	struct mucse_hw *hw = &mucse->hw;
 	struct net_device *netdev;
 
 	if (!mucse)
 		return;
 	netdev = mucse->netdev;
+	if (netdev->reg_state == NETREG_REGISTERED)
+		unregister_netdev(netdev);
 	mucse->netdev = NULL;
+	hw->ops->driver_status(hw, false, mucse_driver_insmod);
 	free_netdev(netdev);
 }
 
-- 
2.25.1

> +struct mucse_hw_operations {
> +	int (*reset_hw)(struct mucse_hw *hw);
> +	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
> +};

Again, there is only one instance of this. Will there be more?

> + * rnpgbe_get_permanent_mac - Get permanent mac
> + * @hw: hw information structure
> + * @mac_addr: pointer to store mac
> + *
> + * rnpgbe_get_permanent_mac tries to get mac from hw.
> + * It use eth_random_addr if failed.
> + **/
> +static void rnpgbe_get_permanent_mac(struct mucse_hw *hw,
> +				     u8 *mac_addr)
> +{
> +	struct device *dev = &hw->pdev->dev;
> +
> +	if (mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->lane) ||
> +	    !is_valid_ether_addr(mac_addr)) {
> +		dev_warn(dev, "Failed to get valid MAC from FW, using random\n");
> +		eth_random_addr(mac_addr);
> +	}

With a function named rnpgbe_get_permanent_mac(), i would not expect
it to return a random MAC address. If there is no permanent MAC
address, return -EINVAL, and let the caller does with the error.

> +static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
> +{
> +	struct mucse_dma_info *dma = &hw->dma;
> +	int err;
> +
> +	dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
> +	err = mucse_mbx_fw_reset_phy(hw);
> +	if (err)
> +		return err;
> +	/* Store the permanent mac address */
> +	if (!(hw->flags & M_FLAGS_INIT_MAC_ADDRESS))

What do this hw->flags add to the driver? Why is it here?

>  static void rnpgbe_rm_adapter(struct pci_dev *pdev)
>  {
>  	struct mucse *mucse = pci_get_drvdata(pdev);
> +	struct mucse_hw *hw = &mucse->hw;
>  	struct net_device *netdev;
>  
>  	if (!mucse)
>  		return;
>  	netdev = mucse->netdev;
> +	if (netdev->reg_state == NETREG_REGISTERED)
> +		unregister_netdev(netdev);

Is that possible?

>  	mucse->netdev = NULL;
> +	hw->ops->driver_status(hw, false, mucse_driver_insmod);
>  	free_netdev(netdev);
>  }
>  
> -- 
> 2.25.1
>

On Fri, Aug 15, 2025 at 05:42:05AM +0200, Andrew Lunn wrote:
> > +struct mucse_hw_operations {
> > +	int (*reset_hw)(struct mucse_hw *hw);
> > +	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
> > +};
> 
> Again, there is only one instance of this. Will there be more?
> 

It is one instance now, but maybe more hw in the furture.
I want to keep this...

> > + * rnpgbe_get_permanent_mac - Get permanent mac
> > + * @hw: hw information structure
> > + * @mac_addr: pointer to store mac
> > + *
> > + * rnpgbe_get_permanent_mac tries to get mac from hw.
> > + * It use eth_random_addr if failed.
> > + **/
> > +static void rnpgbe_get_permanent_mac(struct mucse_hw *hw,
> > +				     u8 *mac_addr)
> > +{
> > +	struct device *dev = &hw->pdev->dev;
> > +
> > +	if (mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->lane) ||
> > +	    !is_valid_ether_addr(mac_addr)) {
> > +		dev_warn(dev, "Failed to get valid MAC from FW, using random\n");
> > +		eth_random_addr(mac_addr);
> > +	}
> 
> With a function named rnpgbe_get_permanent_mac(), i would not expect
> it to return a random MAC address. If there is no permanent MAC
> address, return -EINVAL, and let the caller does with the error.
> 

Ok, I will update this.

> > +static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
> > +{
> > +	struct mucse_dma_info *dma = &hw->dma;
> > +	int err;
> > +
> > +	dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
> > +	err = mucse_mbx_fw_reset_phy(hw);
> > +	if (err)
> > +		return err;
> > +	/* Store the permanent mac address */
> > +	if (!(hw->flags & M_FLAGS_INIT_MAC_ADDRESS))
> 
> What do this hw->flags add to the driver? Why is it here?
> 

It is used to init 'permanent addr' only once.
rnpgbe_reset_hw_ops maybe called when netdev down or hw hang, no need
try to get 'permanent addr' more times.

> >  static void rnpgbe_rm_adapter(struct pci_dev *pdev)
> >  {
> >  	struct mucse *mucse = pci_get_drvdata(pdev);
> > +	struct mucse_hw *hw = &mucse->hw;
> >  	struct net_device *netdev;
> >  
> >  	if (!mucse)
> >  		return;
> >  	netdev = mucse->netdev;
> > +	if (netdev->reg_state == NETREG_REGISTERED)
> > +		unregister_netdev(netdev);
> 
> Is that possible?
> 

Maybe probe failed before register_netdev? Then rmmod the driver.

> >  	mucse->netdev = NULL;
> > +	hw->ops->driver_status(hw, false, mucse_driver_insmod);
> >  	free_netdev(netdev);
> >  }
> >  
> > -- 
> > 2.25.1
> > 
>

> > > +static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
> > > +{
> > > +	struct mucse_dma_info *dma = &hw->dma;
> > > +	int err;
> > > +
> > > +	dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
> > > +	err = mucse_mbx_fw_reset_phy(hw);
> > > +	if (err)
> > > +		return err;
> > > +	/* Store the permanent mac address */
> > > +	if (!(hw->flags & M_FLAGS_INIT_MAC_ADDRESS))
> > 
> > What do this hw->flags add to the driver? Why is it here?
> > 
> 
> It is used to init 'permanent addr' only once.
> rnpgbe_reset_hw_ops maybe called when netdev down or hw hang, no need
> try to get 'permanent addr' more times.

It normally costs ~0 to ask the firmware something. So it is generally
simpler to just ask it. If the firmware is dead, you should not really
care, the RPC should timeout, ETIMEDOUT will get returned to user
space, and likely everything else will fail anyway.
 
> > >  static void rnpgbe_rm_adapter(struct pci_dev *pdev)
> > >  {
> > >  	struct mucse *mucse = pci_get_drvdata(pdev);
> > > +	struct mucse_hw *hw = &mucse->hw;
> > >  	struct net_device *netdev;
> > >  
> > >  	if (!mucse)
> > >  		return;
> > >  	netdev = mucse->netdev;
> > > +	if (netdev->reg_state == NETREG_REGISTERED)
> > > +		unregister_netdev(netdev);
> > 
> > Is that possible?
> > 
> 
> Maybe probe failed before register_netdev? Then rmmod the driver.

Functions like this come in pairs. There is some sort of setup
function, and a corresponding teardown function. probe/remove,
open/close. In Linux, if the first fails, the second is never called.

	Andrew

On Fri, Aug 15, 2025 at 08:06:35PM +0200, Andrew Lunn wrote:
> > > > +static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
> > > > +{
> > > > +	struct mucse_dma_info *dma = &hw->dma;
> > > > +	int err;
> > > > +
> > > > +	dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
> > > > +	err = mucse_mbx_fw_reset_phy(hw);
> > > > +	if (err)
> > > > +		return err;
> > > > +	/* Store the permanent mac address */
> > > > +	if (!(hw->flags & M_FLAGS_INIT_MAC_ADDRESS))
> > > 
> > > What do this hw->flags add to the driver? Why is it here?
> > > 
> > 
> > It is used to init 'permanent addr' only once.
> > rnpgbe_reset_hw_ops maybe called when netdev down or hw hang, no need
> > try to get 'permanent addr' more times.
> 
> It normally costs ~0 to ask the firmware something. So it is generally
> simpler to just ask it. If the firmware is dead, you should not really
> care, the RPC should timeout, ETIMEDOUT will get returned to user
> space, and likely everything else will fail anyway.
>  

Ok, I will remove 'M_FLAGS_INIT_MAC_ADDRESS', and ask the firmware when
the function is called.

> > > >  static void rnpgbe_rm_adapter(struct pci_dev *pdev)
> > > >  {
> > > >  	struct mucse *mucse = pci_get_drvdata(pdev);
> > > > +	struct mucse_hw *hw = &mucse->hw;
> > > >  	struct net_device *netdev;
> > > >  
> > > >  	if (!mucse)
> > > >  		return;
> > > >  	netdev = mucse->netdev;
> > > > +	if (netdev->reg_state == NETREG_REGISTERED)
> > > > +		unregister_netdev(netdev);
> > > 
> > > Is that possible?
> > > 
> > 
> > Maybe probe failed before register_netdev? Then rmmod the driver.
> 
> Functions like this come in pairs. There is some sort of setup
> function, and a corresponding teardown function. probe/remove,
> open/close. In Linux, if the first fails, the second is never called.
> 
> 	Andrew
> 

Got it, 'if (netdev->reg_state == NETREG_REGISTERED)' will be removed.

Thansk for you feedback.

On 14/08/25 1:08 pm, Dong Yibo wrote:
> Initialize get mac from hw, register the netdev.
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
>  drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 18 +++++
>  .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 73 ++++++++++++++++++
>  drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  1 +
>  .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 75 +++++++++++++++++++
>  4 files changed, 167 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 7ab1cbb432f6..7e51a8871b71 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -6,6 +6,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/netdevice.h>
>  
>  extern const struct rnpgbe_info rnpgbe_n500_info;
>  extern const struct rnpgbe_info rnpgbe_n210_info;
> @@ -82,6 +83,15 @@ struct mucse_mbx_info {
>  	u32 fw2pf_mbox_vec;
>  };
>  
> +struct mucse_hw_operations {
> +	int (*reset_hw)(struct mucse_hw *hw);
> +	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
> +};
> +
> +enum {
> +	mucse_driver_insmod,
> +};
> +
>  struct mucse_hw {
>  	u8 pfvfnum;
>  	void __iomem *hw_addr;
> @@ -91,12 +101,17 @@ struct mucse_hw {
>  	u32 axi_mhz;
>  	u32 bd_uid;
>  	enum rnpgbe_hw_type hw_type;
> +	const struct mucse_hw_operations *ops;
>  	struct mucse_dma_info dma;
>  	struct mucse_eth_info eth;
>  	struct mucse_mac_info mac;
>  	struct mucse_mbx_info mbx;
> +	u32 flags;
> +#define M_FLAGS_INIT_MAC_ADDRESS BIT(0)
>  	u32 driver_version;
>  	u16 usecstocount;
> +	int lane;
> +	u8 perm_addr[ETH_ALEN];
>  };
>  
>  struct mucse {
> @@ -117,4 +132,7 @@ struct rnpgbe_info {
>  #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
>  #define PCI_DEVICE_ID_N210 0x8208
>  #define PCI_DEVICE_ID_N210L 0x820a
> +
> +#define dma_wr32(dma, reg, val) writel((val), (dma)->dma_base_addr + (reg))
> +#define dma_rd32(dma, reg) readl((dma)->dma_base_addr + (reg))

These macros could collide with other definitions. Consider prefixing
them with the driver name (rnpgbe_dma_wr32).

I don't see these macros getting used anywhere in this series. They
should be introduced when they are used.

>  #endif /* _RNPGBE_H */
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index e0c6f47efd4c..aba44b31eae3 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -1,11 +1,83 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright(c) 2020 - 2025 Mucse Corporation. */
>  
> +#include <linux/pci.h>
>  #include <linux/string.h>
> +#include <linux/etherdevice.h>
>  
>  #include "rnpgbe.h"
>  #include "rnpgbe_hw.h"
>  #include "rnpgbe_mbx.h"
> +#include "rnpgbe_mbx_fw.h"

> +/**
> + * rnpgbe_xmit_frame - Send a skb to driver
> + * @skb: skb structure to be sent
> + * @netdev: network interface device structure
> + *
> + * @return: NETDEV_TX_OK or NETDEV_TX_BUSY
> + **/
> +static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
> +				     struct net_device *netdev)
> +{
> +		dev_kfree_skb_any(skb);
> +		netdev->stats.tx_dropped++;
> +		return NETDEV_TX_OK;
> +}

You didn't fix this extra indentation. This was present in v3 as well

https://lore.kernel.org/all/94eeae65-0e4b-45ef-a9c0-6bc8d37ae789@ti.com/#:~:text=skb)%3B%0A%3E%20%2B%09%09return%20NETDEV_TX_OK%3B%0A%3E%20%2B-,%7D,-Extra%20indentation%20on

> +
> +static const struct net_device_ops rnpgbe_netdev_ops = {
> +	.ndo_open = rnpgbe_open,
> +	.ndo_stop = rnpgbe_close,


-- 
Thanks and Regards,
Danish

On Thu, Aug 14, 2025 at 05:44:51PM +0530, MD Danish Anwar wrote:
> On 14/08/25 1:08 pm, Dong Yibo wrote:
> > Initialize get mac from hw, register the netdev.
> > 
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > ---
> >  drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 18 +++++
> >  .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 73 ++++++++++++++++++
> >  drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  1 +
> >  .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 75 +++++++++++++++++++
> >  4 files changed, 167 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > index 7ab1cbb432f6..7e51a8871b71 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > @@ -6,6 +6,7 @@
> >  
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/netdevice.h>
> >  
> >  extern const struct rnpgbe_info rnpgbe_n500_info;
> >  extern const struct rnpgbe_info rnpgbe_n210_info;
> > @@ -82,6 +83,15 @@ struct mucse_mbx_info {
> >  	u32 fw2pf_mbox_vec;
> >  };
> >  
> > +struct mucse_hw_operations {
> > +	int (*reset_hw)(struct mucse_hw *hw);
> > +	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
> > +};
> > +
> > +enum {
> > +	mucse_driver_insmod,
> > +};
> > +
> >  struct mucse_hw {
> >  	u8 pfvfnum;
> >  	void __iomem *hw_addr;
> > @@ -91,12 +101,17 @@ struct mucse_hw {
> >  	u32 axi_mhz;
> >  	u32 bd_uid;
> >  	enum rnpgbe_hw_type hw_type;
> > +	const struct mucse_hw_operations *ops;
> >  	struct mucse_dma_info dma;
> >  	struct mucse_eth_info eth;
> >  	struct mucse_mac_info mac;
> >  	struct mucse_mbx_info mbx;
> > +	u32 flags;
> > +#define M_FLAGS_INIT_MAC_ADDRESS BIT(0)
> >  	u32 driver_version;
> >  	u16 usecstocount;
> > +	int lane;
> > +	u8 perm_addr[ETH_ALEN];
> >  };
> >  
> >  struct mucse {
> > @@ -117,4 +132,7 @@ struct rnpgbe_info {
> >  #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
> >  #define PCI_DEVICE_ID_N210 0x8208
> >  #define PCI_DEVICE_ID_N210L 0x820a
> > +
> > +#define dma_wr32(dma, reg, val) writel((val), (dma)->dma_base_addr + (reg))
> > +#define dma_rd32(dma, reg) readl((dma)->dma_base_addr + (reg))
> 
> These macros could collide with other definitions. Consider prefixing
> them with the driver name (rnpgbe_dma_wr32).
> 
> I don't see these macros getting used anywhere in this series. They
> should be introduced when they are used.
> 

dma_wr32 is used in rnpgbe_reset_hw_ops (rnpgbe_chip.c). I rename it to
rnpgbe_dma_wr32.
dma_rd32 is not used, I will remove it.

On Thu, Aug 14, 2025 at 05:44:51PM +0530, MD Danish Anwar wrote:
> On 14/08/25 1:08 pm, Dong Yibo wrote:
> > Initialize get mac from hw, register the netdev.
> > 
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > ---
> >  drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 18 +++++
> >  .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 73 ++++++++++++++++++
> >  drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  1 +
> >  .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 75 +++++++++++++++++++
> >  4 files changed, 167 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > index 7ab1cbb432f6..7e51a8871b71 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > @@ -6,6 +6,7 @@
> >  
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/netdevice.h>
> >  
> >  extern const struct rnpgbe_info rnpgbe_n500_info;
> >  extern const struct rnpgbe_info rnpgbe_n210_info;
> > @@ -82,6 +83,15 @@ struct mucse_mbx_info {
> >  	u32 fw2pf_mbox_vec;
> >  };
> >  
> > +struct mucse_hw_operations {
> > +	int (*reset_hw)(struct mucse_hw *hw);
> > +	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
> > +};
> > +
> > +enum {
> > +	mucse_driver_insmod,
> > +};
> > +
> >  struct mucse_hw {
> >  	u8 pfvfnum;
> >  	void __iomem *hw_addr;
> > @@ -91,12 +101,17 @@ struct mucse_hw {
> >  	u32 axi_mhz;
> >  	u32 bd_uid;
> >  	enum rnpgbe_hw_type hw_type;
> > +	const struct mucse_hw_operations *ops;
> >  	struct mucse_dma_info dma;
> >  	struct mucse_eth_info eth;
> >  	struct mucse_mac_info mac;
> >  	struct mucse_mbx_info mbx;
> > +	u32 flags;
> > +#define M_FLAGS_INIT_MAC_ADDRESS BIT(0)
> >  	u32 driver_version;
> >  	u16 usecstocount;
> > +	int lane;
> > +	u8 perm_addr[ETH_ALEN];
> >  };
> >  
> >  struct mucse {
> > @@ -117,4 +132,7 @@ struct rnpgbe_info {
> >  #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
> >  #define PCI_DEVICE_ID_N210 0x8208
> >  #define PCI_DEVICE_ID_N210L 0x820a
> > +
> > +#define dma_wr32(dma, reg, val) writel((val), (dma)->dma_base_addr + (reg))
> > +#define dma_rd32(dma, reg) readl((dma)->dma_base_addr + (reg))
> 
> These macros could collide with other definitions. Consider prefixing
> them with the driver name (rnpgbe_dma_wr32).
> 
> I don't see these macros getting used anywhere in this series. They
> should be introduced when they are used.
> 

Got it, I will introduce codes when they are used.

> >  #endif /* _RNPGBE_H */
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > index e0c6f47efd4c..aba44b31eae3 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > @@ -1,11 +1,83 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright(c) 2020 - 2025 Mucse Corporation. */
> >  
> > +#include <linux/pci.h>
> >  #include <linux/string.h>
> > +#include <linux/etherdevice.h>
> >  
> >  #include "rnpgbe.h"
> >  #include "rnpgbe_hw.h"
> >  #include "rnpgbe_mbx.h"
> > +#include "rnpgbe_mbx_fw.h"
> 
> > +/**
> > + * rnpgbe_xmit_frame - Send a skb to driver
> > + * @skb: skb structure to be sent
> > + * @netdev: network interface device structure
> > + *
> > + * @return: NETDEV_TX_OK or NETDEV_TX_BUSY
> > + **/
> > +static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
> > +				     struct net_device *netdev)
> > +{
> > +		dev_kfree_skb_any(skb);
> > +		netdev->stats.tx_dropped++;
> > +		return NETDEV_TX_OK;
> > +}
> 
> You didn't fix this extra indentation. This was present in v3 as well
> 
> https://lore.kernel.org/all/94eeae65-0e4b-45ef-a9c0-6bc8d37ae789@ti.com/#:~:text=skb)%3B%0A%3E%20%2B%09%09return%20NETDEV_TX_OK%3B%0A%3E%20%2B-,%7D,-Extra%20indentation%20on
> 

Sorry, I missed fix this, I will fix it in the next version.

> > +
> > +static const struct net_device_ops rnpgbe_netdev_ops = {
> > +	.ndo_open = rnpgbe_open,
> > +	.ndo_stop = rnpgbe_close,
> 
> 
> -- 
> Thanks and Regards,
> Danish
> 
> 

Thanks for your feedback