Re: [PATCH net-next v3 04/14] net: ethernet: qualcomm: Initialize PPE buffer management for IPQ9574

[Jie Luo]

On 2/11/2025 9:22 PM, Andrew Lunn wrote:
> > +	/* Configure BM flow control related threshold. */
> > +	PPE_BM_PORT_FC_SET_WEIGHT(bm_fc_val, port_cfg.weight);
> > +	PPE_BM_PORT_FC_SET_RESUME_OFFSET(bm_fc_val, port_cfg.resume_offset);
> > +	PPE_BM_PORT_FC_SET_RESUME_THRESHOLD(bm_fc_val, port_cfg.resume_ceil);
> > +	PPE_BM_PORT_FC_SET_DYNAMIC(bm_fc_val, port_cfg.dynamic);
> > +	PPE_BM_PORT_FC_SET_REACT_LIMIT(bm_fc_val, port_cfg.in_fly_buf);
> > +	PPE_BM_PORT_FC_SET_PRE_ALLOC(bm_fc_val, port_cfg.pre_alloc);
>
> ...
>
> > +#define PPE_BM_PORT_FC_CFG_TBL_ADDR		0x601000
> > +#define PPE_BM_PORT_FC_CFG_TBL_ENTRIES		15
> > +#define PPE_BM_PORT_FC_CFG_TBL_INC		0x10
> > +#define PPE_BM_PORT_FC_W0_REACT_LIMIT		GENMASK(8, 0)
> > +#define PPE_BM_PORT_FC_W0_RESUME_THRESHOLD	GENMASK(17, 9)
> > +#define PPE_BM_PORT_FC_W0_RESUME_OFFSET		GENMASK(28, 18)
> > +#define PPE_BM_PORT_FC_W0_CEILING_LOW		GENMASK(31, 29)
> > +#define PPE_BM_PORT_FC_W1_CEILING_HIGH		GENMASK(7, 0)
> > +#define PPE_BM_PORT_FC_W1_WEIGHT		GENMASK(10, 8)
> > +#define PPE_BM_PORT_FC_W1_DYNAMIC		BIT(11)
> > +#define PPE_BM_PORT_FC_W1_PRE_ALLOC		GENMASK(22, 12)
> > +
> > +#define PPE_BM_PORT_FC_SET_REACT_LIMIT(tbl_cfg, value)	\
> > +	u32p_replace_bits((u32 *)tbl_cfg, value, PPE_BM_PORT_FC_W0_REACT_LIMIT)
> > +#define PPE_BM_PORT_FC_SET_RESUME_THRESHOLD(tbl_cfg, value)	\
> > +	u32p_replace_bits((u32 *)tbl_cfg, value, PPE_BM_PORT_FC_W0_RESUME_THRESHOLD)
>
> Where is u32p_replace_bits()?

u32p_replace_bits is defined by the macro __MAKE_OP(32) in the header
file "include/linux/bitfield.h".

> This cast does not look good. 

Yes, we can remove the cast.

> And this does not look like anything any
> other driver does. I suspect you are not using FIELD_PREP() etc when
> you should.
>
> https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/bitfield.h
>
> 	Andrew

The PPE_BM_XXX macros defined here write to either of two different
32bit words in the register table, and the actual word used (0 or 1)
is hidden within the macro. For example, the below macro.

#define PPE_BM_PORT_FC_SET_CEILING_HIGH(tbl_cfg, value)	\
	u32p_replace_bits((u32 *)(tbl_cfg) + 0x1, value,
	PPE_BM_PORT_FC_W1_CEILING_HIGH)

We could have used FIELD_PREP as well for this purpose. However using
u32p_replace_bits() seemed more convenient and cleaner in this case,
since with FIELD_PREP, we would have needed an assignment statement to
be defined in the macro implementation. We also noticed many other
drivers using u32_replace_bits(). Hope this is ok.