On Thu, Feb 20, 2025 at 10:38:03PM +0800, Jie Luo wrote: > > > 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". Given it is pretty well hidden, and not documented, it makes me think you should not be using it. The macros you are expected to use from that file are all well documented. > > This cast does not look good. > > Yes, we can remove the cast. To some extent, this is a symptom. Why is the cast there in the first place? Cast suggest bad design, not thinking about types, thinking it is actual O.K. to cast between types. Please look at all the casts you have. Is it because of bad design? If so, please fix your types to eliminate the casts. > > 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. Please extend the set of FIELD_{GET,PREP} macros to cover your use case. Document them to the level of the existing macros. Submit the patch to: Yury Norov <yury.norov@xxxxxxxxx> (maintainer:BITMAP API) Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> (reviewer:BITMAP API) etc and see what they say about this. Andrew
