On Tue, Jul 22, 2025 at 11:26:44AM +0100, Vadim Fedorenko wrote:

On 22/07/2025 10:51, Yibo Dong wrote:

quoted

On Mon, Jul 21, 2025 at 03:21:23PM +0100, Vadim Fedorenko wrote:

quoted

On 21/07/2025 12:32, Dong Yibo wrote:

quoted

Initialize n500/n210 chip bar resource map and
dma, eth, mbx ... info for future use.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
   drivers/net/ethernet/mucse/rnpgbe/Makefile    |   4 +-
   drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 138 ++++++++++++++++++
   .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 138 ++++++++++++++++++
   drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  27 ++++
   .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   |  68 ++++++++-
   5 files changed, 370 insertions(+), 5 deletions(-)
   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h

[...]

quoted

quoted

quoted

+/**
+ * rnpgbe_get_invariants_n500 - setup for hw info
+ * @hw: hw information structure
+ *
+ * rnpgbe_get_invariants_n500 initializes all private
+ * structure, such as dma, eth, mac and mbx base on
+ * hw->addr for n500
+ **/
+static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
+{
+	struct mucse_dma_info *dma = &hw->dma;
+	struct mucse_eth_info *eth = &hw->eth;
+	struct mucse_mac_info *mac = &hw->mac;
+	struct mucse_mbx_info *mbx = &hw->mbx;
+
+	/* setup msix base */
+	hw->ring_msix_base = hw->hw_addr + 0x28700;
+	/* setup dma info */
+	dma->dma_base_addr = hw->hw_addr;
+	dma->dma_ring_addr = hw->hw_addr + RNPGBE_RING_BASE;
+	dma->max_tx_queues = RNPGBE_MAX_QUEUES;
+	dma->max_rx_queues = RNPGBE_MAX_QUEUES;
+	dma->back = hw;
+	/* setup eth info */
+	eth->eth_base_addr = hw->hw_addr + RNPGBE_ETH_BASE;
+	eth->back = hw;
+	eth->mc_filter_type = 0;
+	eth->mcft_size = RNPGBE_MC_TBL_SIZE;
+	eth->vft_size = RNPGBE_VFT_TBL_SIZE;
+	eth->num_rar_entries = RNPGBE_RAR_ENTRIES;
+	/* setup mac info */
+	mac->mac_addr = hw->hw_addr + RNPGBE_MAC_BASE;
+	mac->back = hw;
+	/* set mac->mii */
+	mac->mii.addr = RNPGBE_MII_ADDR;
+	mac->mii.data = RNPGBE_MII_DATA;
+	mac->mii.addr_shift = 11;
+	mac->mii.addr_mask = 0x0000F800;
+	mac->mii.reg_shift = 6;
+	mac->mii.reg_mask = 0x000007C0;
+	mac->mii.clk_csr_shift = 2;
+	mac->mii.clk_csr_mask = GENMASK(5, 2);
+	mac->clk_csr = 0x02; /* csr 25M */
+	/* hw fixed phy_addr */
+	mac->phy_addr = 0x11;
+
+	mbx->mbx_feature |= MBX_FEATURE_NO_ZERO;
+	/* mbx offset */
+	mbx->vf2pf_mbox_vec_base = 0x28900;
+	mbx->fw2pf_mbox_vec = 0x28b00;
+	mbx->pf_vf_shm_base = 0x29000;
+	mbx->mbx_mem_size = 64;
+	mbx->pf2vf_mbox_ctrl_base = 0x2a100;
+	mbx->pf_vf_mbox_mask_lo = 0x2a200;
+	mbx->pf_vf_mbox_mask_hi = 0;
+	mbx->fw_pf_shm_base = 0x2d000;
+	mbx->pf2fw_mbox_ctrl = 0x2e000;
+	mbx->fw_pf_mbox_mask = 0x2e200;
+	mbx->fw_vf_share_ram = 0x2b000;
+	mbx->share_size = 512;
+
+	/* setup net feature here */
+	hw->feature_flags |= M_NET_FEATURE_SG |
+			     M_NET_FEATURE_TX_CHECKSUM |
+			     M_NET_FEATURE_RX_CHECKSUM |
+			     M_NET_FEATURE_TSO |
+			     M_NET_FEATURE_VLAN_FILTER |
+			     M_NET_FEATURE_VLAN_OFFLOAD |
+			     M_NET_FEATURE_RX_NTUPLE_FILTER |
+			     M_NET_FEATURE_RX_HASH |
+			     M_NET_FEATURE_USO |
+			     M_NET_FEATURE_RX_FCS |
+			     M_NET_FEATURE_STAG_FILTER |
+			     M_NET_FEATURE_STAG_OFFLOAD;
+	/* start the default ahz, update later */
+	hw->usecstocount = 125;
+}
+
+/**
+ * rnpgbe_get_invariants_n210 - setup for hw info
+ * @hw: hw information structure
+ *
+ * rnpgbe_get_invariants_n210 initializes all private
+ * structure, such as dma, eth, mac and mbx base on
+ * hw->addr for n210
+ **/
+static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	/* get invariants based from n500 */
+	rnpgbe_get_invariants_n500(hw);

it's not a good pattern. if you have some configuration that is
shared amoung devices, it's better to create *base() or *common()
helper and call it from each specific initializer. BTW, why do you
name these functions get_invariants*()? They don't get anything, but
rather init/setup configuration values. It's better to rename it
according to the function.

I try to devide hardware to dma, eth, mac, mbx modules. Different
chips may use the same mbx module with different reg-offset in bar.
So I setup reg-offset in get_invariants for each chip. And common code,
such as mbx achieve functions with the reg-offset.
Ok, I will rename it.

I fully understand your intention. My point is that calling
rnpgbe_get_invariants_n500(hw) in rnpgbe_get_invariants_n210() and
then replace almost half of the values is not a good pattern.
It's better to have another function to setup values that are the same
across models, and keep only specifics in *n500() and *n210().

Got your point, I will improve it.

quoted

quoted

quoted

+
+	/* update msix base */
+	hw->ring_msix_base = hw->hw_addr + 0x29000;
+	/* update mbx offset */
+	mbx->vf2pf_mbox_vec_base = 0x29200;
+	mbx->fw2pf_mbox_vec = 0x29400;
+	mbx->pf_vf_shm_base = 0x29900;
+	mbx->mbx_mem_size = 64;
+	mbx->pf2vf_mbox_ctrl_base = 0x2aa00;
+	mbx->pf_vf_mbox_mask_lo = 0x2ab00;
+	mbx->pf_vf_mbox_mask_hi = 0;
+	mbx->fw_pf_shm_base = 0x2d900;
+	mbx->pf2fw_mbox_ctrl = 0x2e900;
+	mbx->fw_pf_mbox_mask = 0x2eb00;
+	mbx->fw_vf_share_ram = 0x2b900;
+	mbx->share_size = 512;
+	/* update hw feature */
+	hw->feature_flags |= M_HW_FEATURE_EEE;
+	hw->usecstocount = 62;
+}

[...]

quoted

quoted

quoted

@@ -58,7 +72,54 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev)
   		 rnpgbe_driver_name, mucse->bd_number);
   	pci_set_drvdata(pdev, mucse);
+	hw = &mucse->hw;
+	hw->back = mucse;
+	hw->hw_type = ii->hw_type;
+
+	switch (hw->hw_type) {
+	case rnpgbe_hw_n500:
+		/* n500 use bar2 */
+		hw_addr = devm_ioremap(&pdev->dev,
+				       pci_resource_start(pdev, 2),
+				       pci_resource_len(pdev, 2));
+		if (!hw_addr) {
+			dev_err(&pdev->dev, "map bar2 failed!\n");
+			return -EIO;
+		}
+
+		/* get dma version */
+		dma_version = m_rd_reg(hw_addr);
+		break;
+	case rnpgbe_hw_n210:
+	case rnpgbe_hw_n210L:
+		/* check bar0 to load firmware */
+		if (pci_resource_len(pdev, 0) == 0x100000)
+			return -EIO;
+		/* n210 use bar2 */
+		hw_addr = devm_ioremap(&pdev->dev,
+				       pci_resource_start(pdev, 2),
+				       pci_resource_len(pdev, 2));
+		if (!hw_addr) {
+			dev_err(&pdev->dev, "map bar2 failed!\n");
+			return -EIO;
+		}
+
+		/* get dma version */
+		dma_version = m_rd_reg(hw_addr);
+		break;
+	default:
+		err = -EIO;
+		goto err_free_net;
+	}
+	hw->hw_addr = hw_addr;
+	hw->dma.dma_version = dma_version;
+	ii->get_invariants(hw);
+
   	return 0;
+
+err_free_net:
+	free_netdev(netdev);
+	return err;
   }

You have err_free_net label, which is used only in really impossible
case of unknown device, while other cases can return directly and
memleak netdev...>>

Yes, It is really impossible case of unknown device. But maybe switch
should always has 'default case'? And if in 'default case', nothing To
do but free_netdev and return err.
Other cases return directly with return 0, and netdev will be freed in
rnpgbe_rm_adapter() when rmmod. Sorry, I may not have got the memleak
point?

Both rnpgbe_hw_n500 and rnpgbe_hw_n200 cases have error paths which
directly return -EIO. In this case netdev is not freed and
rnpgbe_rm_adapter() will not happen as rnpgbe_add_adapter() didn't
succeed.

Yes, you are right, memleak may happen here, I will fix it.

Thanks for your feedback.

quoted

quoted

quoted

   /**

@@ -74,6 +135,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev)
    **/
   static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
   {
+	const struct rnpgbe_info *ii = rnpgbe_info_tbl[id->driver_data];
   	int err;
   	err = pci_enable_device_mem(pdev);

@@ -97,7 +159,7 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
   	pci_set_master(pdev);
   	pci_save_state(pdev);
-	err = rnpgbe_add_adapter(pdev);
+	err = rnpgbe_add_adapter(pdev, ii);
   	if (err)
   		goto err_regions;

Thanks for your feedback.