Re: [PATCH v2 02/15] net: rnpgbe: Add n500/n210 chip support

[Andrew Lunn <andrew@lunn.ch>]

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

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

> +#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.

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

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

> +#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.

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


>  {
>  	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