On Mon, Jul 21, 2025 at 05:25:12PM +0200, Andrew Lunn wrote:
> > +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.
> > +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.
> > +#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.
> > + 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.
>
> > + 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.
> > +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.
> > +#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.
> > + 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.
> > {
> > 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.
