Re: [PATCH net-next v1 3/5] net: tn40xx: add basic Tx handling

[Simon Horman <horms@kernel.org>]

On Mon, Apr 22, 2024 at 04:29:13PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Thu, 18 Apr 2024 18:23:06 +0100
> Simon Horman <horms@kernel.org> wrote:
> 
> > On Mon, Apr 15, 2024 at 07:43:50PM +0900, FUJITA Tomonori wrote:
> >> This patch adds device specific structures to initialize the hardware
> >> with basic Tx handling. The original driver loads the embedded
> >> firmware in the header file. This driver is implemented to use the
> >> firmware APIs.
> >> 
> >> The Tx logic uses three major data structures; two ring buffers with
> >> NIC and one database. One ring buffer is used to send information
> >> about packets to be sent for NIC. The other is used to get information
> >> from NIC about packet that are sent. The database is used to keep the
> >> information about DMA mapping. After a packet is sent, the db is used
> >> to free the resource used for the packet.
> >> 
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > 
> > Hi Fujita-san,
> > 
> > Some review from my side.
> 
> Thanks a lot!

Likewise, thanks for addressing most of my concerns.

> >> +static int bdx_fifo_alloc(struct bdx_priv *priv, struct fifo *f, int fsz_type,
> >> +			  u16 reg_cfg0, u16 reg_cfg1, u16 reg_rptr, u16 reg_wptr)
> > 
> > Please consider using a soft limit on line length of 80 characters
> > in Networking code.
> 
> Sure, fixed.
> 
> >> +{
> >> +	u16 memsz = FIFO_SIZE * (1 << fsz_type);
> > 
> > I'm not sure if fsz_type has a meaning here - perhaps it comes from the
> > datasheet. But if not, perhaps 'order' would be a more intuitive
> > name for this parameter. Similarly for the txd_size and txf_size
> > fields of struct bdx_priv, the sz_type field of bdx_tx_db_init(),
> > and possibly elsewhere.
> 
> I don't have the datasheet of this hardware (so I have to leave alone
> many magic values from the original driver).
> 
> The driver writes this 'fsz_type' to a register so I guess this is
> called something like fifo_size_type for the hardware. I'll rename if
> you prefer.

No strong preference here.

> >> +
> >> +	memset(f, 0, sizeof(struct fifo));
> >> +	/* 1K extra space is allocated at the end of the fifo to simplify
> >> +	 * processing of descriptors that wraps around fifo's end.
> >> +	 */
> >> +	f->va = dma_alloc_coherent(&priv->pdev->dev,
> >> +				   memsz + FIFO_EXTRA_SPACE, &f->da, GFP_KERNEL);
> >> +	if (!f->va)
> >> +		return -ENOMEM;
> >> +
> >> +	f->reg_cfg0 = reg_cfg0;
> >> +	f->reg_cfg1 = reg_cfg1;
> >> +	f->reg_rptr = reg_rptr;
> >> +	f->reg_wptr = reg_wptr;
> >> +	f->rptr = 0;
> >> +	f->wptr = 0;
> >> +	f->memsz = memsz;
> >> +	f->size_mask = memsz - 1;
> >> +	write_reg(priv, reg_cfg0, (u32)((f->da & TX_RX_CFG0_BASE) | fsz_type));
> > 
> > For consistency should this be use H32_64()?:
> > 
> > 		H32_64((f->da & TX_RX_CFG0_BASE) | fsz_type)
> 
> L32_64() if we use here?
>
> The driver splits 64 bits value (f->da) and writes them to reg_cfg0
> and reg_cfg1?

Yes, my mistake. L32_64() seems appropriate here.

...

> > There are a lot of magic numbers below.
> > Could these be converted into #defines with meaningful names?
> 
> Without the datasheet, I'm not sure what names are appropriate..

Ok, understood.

> >> +		write_reg(priv, 0x1010, 0x217);	/*ETHSD.REFCLK_CONF  */
> >> +		write_reg(priv, 0x104c, 0x4c);	/*ETHSD.L0_RX_PCNT  */
> >> +		write_reg(priv, 0x1050, 0x4c);	/*ETHSD.L1_RX_PCNT  */
> >> +		write_reg(priv, 0x1054, 0x4c);	/*ETHSD.L2_RX_PCNT  */
> >> +		write_reg(priv, 0x1058, 0x4c);	/*ETHSD.L3_RX_PCNT  */
> >> +		write_reg(priv, 0x102c, 0x434);	/*ETHSD.L0_TX_PCNT  */
> >> +		write_reg(priv, 0x1030, 0x434);	/*ETHSD.L1_TX_PCNT  */
> >> +		write_reg(priv, 0x1034, 0x434);	/*ETHSD.L2_TX_PCNT  */
> >> +		write_reg(priv, 0x1038, 0x434);	/*ETHSD.L3_TX_PCNT  */
> >> +		write_reg(priv, 0x6300, 0x0400);	/*MAC.PCS_CTRL */
> > 
> > ...
> > 
> >> +static int bdx_hw_reset(struct bdx_priv *priv)
> >> +{
> >> +	u32 val, i;
> >> +
> >> +	/* Reset sequences: read, write 1, read, write 0 */
> >> +	val = read_reg(priv, REG_CLKPLL);
> >> +	write_reg(priv, REG_CLKPLL, (val | CLKPLL_SFTRST) + 0x8);
> >> +	udelay(50);
> > 
> > Checkpatch recommends using usleep_range() here
> > after consulting with Documentation/timers/timers-howto.rst.
> > TBH, I'm unsure of the merit of this advice.
> 
> Yeah, I run checkpatch but don't have the datascheet so I'm not sure
> what range are appropriate.

I'd guess that a range of 50 - 100 would be fine.
But I take your point about not having the datasheet,
so perhaps it is safest to just keep the udelay() for now.

> 
> 
> >> +	val = read_reg(priv, REG_CLKPLL);
> >> +	write_reg(priv, REG_CLKPLL, val & ~CLKPLL_SFTRST);
> >> +
> >> +	/* Check that the PLLs are locked and reset ended */
> >> +	for (i = 0; i < 70; i++, mdelay(10)) {
> >> +		if ((read_reg(priv, REG_CLKPLL) & CLKPLL_LKD) == CLKPLL_LKD) {
> >> +			udelay(50);
> > 
> > Ditto.
> > 
> >> +			/* Do any PCI-E read transaction */
> >> +			read_reg(priv, REG_RXD_CFG0_0);
> >> +			return 0;
> >> +		}
> >> +	}
> >> +	return 1;
> >> +}
> >> +
> >> +static int bdx_sw_reset(struct bdx_priv *priv)
> > 
> > nit: This function always returns zero, and the caller ignores the
> >      return value. It's return type could be void.
> 
> Fixed.
> 
> >> +static void bdx_setmulti(struct net_device *ndev)
> >> +{
> >> +	struct bdx_priv *priv = netdev_priv(ndev);
> >> +
> >> +	u32 rxf_val =
> >> +	    GMAC_RX_FILTER_AM | GMAC_RX_FILTER_AB | GMAC_RX_FILTER_OSEN |
> >> +	    GMAC_RX_FILTER_TXFC;
> >> +	int i;
> >> +
> >> +	/* IMF - imperfect (hash) rx multicast filter */
> >> +	/* PMF - perfect rx multicast filter */
> >> +
> >> +	/* FIXME: RXE(OFF) */
> > 
> > Is there a plan to fix this, and the TBD below?
> 
> I just left the original code comment alone. Not sure what I should do
> here. better to remove completely?

Usually it's best not to have such comments.
But if it's a carry-over from existing code,
then I suppose it is best to leave it as is.

> 
> >> diff --git a/drivers/net/ethernet/tehuti/tn40.h b/drivers/net/ethernet/tehuti/tn40.h
> > 
> > ...
> > 
> >> +#if BITS_PER_LONG == 64
> >> +#define H32_64(x) ((u32)((u64)(x) >> 32))
> >> +#define L32_64(x) ((u32)((u64)(x) & 0xffffffff))
> >> +#elif BITS_PER_LONG == 32
> >> +#define H32_64(x) 0
> >> +#define L32_64(x) ((u32)(x))
> >> +#else /* BITS_PER_LONG == ?? */
> >> +#error BITS_PER_LONG is undefined. Must be 64 or 32
> >> +#endif /* BITS_PER_LONG */
> > 
> > I am curious to know why it is valid to mask off the upper 64 bits
> > on systems with 32 bit longs. As far as I see this is used
> > in conjunction for supplying DMA addresses to the NIC.
> > But it's not obvious how that relates to the length
> > of longs on the host.
> 
> I suppose that the original driver tries to use the length of
> dma_addr_t (CONFIG_ARCH_DMA_ADDR_T_64BIT?) here.
> 
> > Probably I'm missing something very obvious here.
> > But if not, my follow-up would be to suggest using
> > upper_32_bits() and lower_32_bits().
> 
> You prefer to remove ifdef 
> 
> #define H32_64(x) upper_32_bits(x)
> #define L32_64(x) lower_32_bits(x)
> 
> ?
> 
> or replace H32_64 and L32_64 with upper_32_bits and lower_32_bits
> respectively?

I'd go with the last option if you think it is safe to do so.
But if you think it's a bit risky, perhaps it's best to keep
the code as is for now.

> >> +#define BDX_TXF_DESC_SZ 16
> >> +#define BDX_MAX_TX_LEVEL (priv->txd_fifo0.m.memsz - 16)
> >> +#define BDX_MIN_TX_LEVEL 256
> >> +#define BDX_NO_UPD_PACKETS 40
> >> +#define BDX_MAX_MTU BIT(14)
> >> +
> >> +#define PCK_TH_MULT 128
> >> +#define INT_COAL_MULT 2
> >> +
> >> +#define BITS_MASK(nbits) ((1 << (nbits)) - 1)
> > 
> >> +#define GET_BITS_SHIFT(x, nbits, nshift) (((x) >> (nshift)) & BITS_MASK(nbits))
> >> +#define BITS_SHIFT_MASK(nbits, nshift) (BITS_MASK(nbits) << (nshift))
> >> +#define BITS_SHIFT_VAL(x, nbits, nshift) (((x) & BITS_MASK(nbits)) << (nshift))
> >> +#define BITS_SHIFT_CLEAR(x, nbits, nshift) \
> >> +	((x) & (~BITS_SHIFT_MASK(nbits, (nshift))))
> >> +
> >> +#define GET_INT_COAL(x) GET_BITS_SHIFT(x, 15, 0)
> >> +#define GET_INT_COAL_RC(x) GET_BITS_SHIFT(x, 1, 15)
> >> +#define GET_RXF_TH(x) GET_BITS_SHIFT(x, 4, 16)
> >> +#define GET_PCK_TH(x) GET_BITS_SHIFT(x, 4, 20)
> > 
> > It feels like using of GENMASK and FIELD_GET is appropriate here.
> 
> Sure, I'll replace the above macros with GENMASK and FIELD_GET. 
> 
> >> +#define INT_REG_VAL(coal, coal_rc, rxf_th, pck_th) \
> >> +	((coal) | ((coal_rc) << 15) | ((rxf_th) << 16) | ((pck_th) << 20))
> > 
> > This looks like a candidate for GENMASK and FILED_PREP.
> 
> like the following?
> 
> #define INT_REG_VAL(coal, coal_rc, rxf_th, pck_th)      \
> 	FIELD_PREP(GENMASK(14, 0), (coal)) |            \
> 	FIELD_PREP(GENMASK(15, 15), (coal_rc)) |        \
> 	FIELD_PREP(GENMASK(19, 16), (rxf_th)) |         \
> 	FIELD_PREP(GENMASK(31, 20), (pck_th))

Yes, I think so.
I think you can use BIT(15) in place of GENMASK(15, 15).

...