Re: [PATCH v2 02/15] net: rnpgbe: Add n500/n210 chip support
From: Vadim Fedorenko
Date: Tue Jul 22 2025 - 06:27:19 EST
On 22/07/2025 10:51, Yibo Dong wrote:
On Mon, Jul 21, 2025 at 03:21:23PM +0100, Vadim Fedorenko wrote:
On 21/07/2025 12:32, Dong Yibo wrote:
Initialize n500/n210 chip bar resource map and
dma, eth, mbx ... info for future use.
Signed-off-by: Dong Yibo <dong100@xxxxxxxxx>
---
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
[...]
+/**
+ * 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().
+
+ /* 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;
+}
[...]
@@ -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.
/**
@@ -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.