On Mon, Jul 21, 2025 at 05:25:12PM +0200, Andrew Lunn wrote:

quoted

+struct mii_regs {
+	unsigned int addr; /* MII Address */
+	unsigned int data; /* MII Data */
+	unsigned int addr_shift; /* MII address shift */
+	unsigned int reg_shift; /* MII reg shift */
+	unsigned int addr_mask; /* MII address mask */
+	unsigned int reg_mask; /* MII reg mask */
+	unsigned int clk_csr_shift;
+	unsigned int clk_csr_mask;
+};

So MII interests me, being the MDIO/PHY maintainer....

You have introduced this without any user, which is not good, so i
cannot see how it is actually used. It is better to introduce
structures in the patch which makes use of them.

Please add this only when you add the mdiobus driver, so i can see how
it is used. Please look at the other structures you have here. Please
add them as they are actually used.

Yes, you are right, I should add the structures when they are actually
used. I will improve it.

quoted

+struct mucse_hw {
+	void *back;
+	u8 pfvfnum;
+	u8 pfvfnum_system;
+	u8 __iomem *hw_addr;
+	u8 __iomem *ring_msix_base;

I spotted this somewhere else. A u8 __iomem * is odd. Why is this not
a void *? ioremap() returns a void __iomem *, and all the readb(),
readw(), readX() functions expect a void * __iomem. So this looks odd.

Got it, I will change it. I just consider the wrong cast before. Sorry
for not check this define error.

quoted

+#define m_rd_reg(reg) readl(reg)
+#define m_wr_reg(reg, val) writel((val), reg)

Please don't wrap standard functions like this. Everybody knows what
readl() does. Nobody has any idea what m_rd_reg() does! You are just
making your driver harder to understand and maintain.

Got it.

quoted

+	mac->mii.addr = RNPGBE_MII_ADDR;
+	mac->mii.data = RNPGBE_MII_DATA;
+	mac->mii.addr_shift = 11;
+	mac->mii.addr_mask = 0x0000F800;

GENMASK()? If you are using these helpers correctly, you probably
don't need the _shift members.

quoted

+	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;

That is suspicious. But until i see the PHY handling code, it is hard
to say.

Those code should move to the patch which really use it.

quoted

+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);
+
+	/* 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;

This variant does not have an MDIO bus?

Some hw capabilies, such as queue numbers and hardware module 
reg-offset(dma_base_addr, eth_base_addr ..) in this function. Don't
have an MDIO bus now.

quoted

+#define RNPGBE_RING_BASE (0x1000)
+#define RNPGBE_MAC_BASE (0x20000)
+#define RNPGBE_ETH_BASE (0x10000)

Please drop all the () on plain constants. You only need () when it is
an expression.

Got it, I will fix this.

quoted

+			      const struct rnpgbe_info *ii)

I don't really see how the variable name ii has anything to do with
rnpgbe_info. I know naming is hard, but why not call it info?

Got it, ii is unclear, I will use info instead.

quoted

 {
 	struct mucse *mucse = NULL;
+	struct mucse_hw *hw = NULL;
+	u8 __iomem *hw_addr = NULL;
 	struct net_device *netdev;
 	static int bd_number;
+	u32 dma_version = 0;
+	int err = 0;
+	u32 queues;
 
-	netdev = alloc_etherdev_mq(sizeof(struct mucse), 1);
+	queues = ii->total_queue_pair_cnts;
+	netdev = alloc_etherdev_mq(sizeof(struct mucse), queues);

I pointed out this before. Try to avoid changing code added in
previous patches. I just wasted time looking up what the function is
called which allocates a single queue, and writing a review comment.

Waiting reviewers time is a good way to get less/slower reviews.

	Andrew

Yes, I got it before, and I really tried to improve my code.
But this is really hard to avoid here. 'queues' is from ii->total_queue_pair_cnts
which is added in patch2. Maybe I should move the alloc_etherdev_mq to
patch2, never use it in patch1? And this conditon can improve.

thanks for your feedback.