Messages in this thread |  | | Date | Thu, 4 Sep 2025 10:38:51 +0800 | | From | Yibo Dong <> | | Subject | Re: [PATCH net-next v10 3/5] net: rnpgbe: Add basic mbx ops support |
| |
On Wed, Sep 03, 2025 at 05:44:31PM +0100, Simon Horman wrote: > On Wed, Sep 03, 2025 at 10:54:28AM +0800, Dong Yibo wrote: > > Initialize basic mbx function. > > > > Signed-off-by: Dong Yibo <dong100@mucse.com> > > ... > > > +/** > > + * mucse_mbx_inc_pf_req - Increase req > > + * @hw: pointer to the HW structure > > + * > > + * mucse_mbx_inc_pf_req read pf_req from hw, then write > > + * new value back after increase > > + **/ > > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw) > > +{ > > + struct mucse_mbx_info *mbx = &hw->mbx; > > + u16 req; > > + u32 v; > > + > > + v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER); > > + req = (v & GENMASK_U32(15, 0)); > > nit1: Unnecessary parentheses > nit2: I would have used FIELD_GET here in conjunction with something like. > > #define MBX_PF2FW_COUNTER_MASK GENMASK_U32(15, 0) >
Got it, maybe update it like this?
req = FIELD_GET(GENMASK_U32(15, 0), v);
> > + req++; > > + v &= GENMASK_U32(31, 16); > > + v |= req; > > And using FIELD_PREP is probably more succinct here. > > Likewise in the following function >
Got it, maybe update it like this?
req++; v = (v & ~GENMASK_U32(15, 0)) | FIELD_PREP(GENMASK_U32(15, 0), req);
> > + mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v); > > + hw->mbx.stats.msgs_tx++; > > +} > > + > > +/** > > + * mucse_mbx_inc_pf_ack - Increase ack > > + * @hw: pointer to the HW structure > > + * > > + * mucse_mbx_inc_pf_ack read pf_ack from hw, then write > > + * new value back after increase > > + **/ > > +static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw) > > +{ > > + struct mucse_mbx_info *mbx = &hw->mbx; > > + u16 ack; > > + u32 v; > > + > > + v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER); > > + ack = (v >> 16) & GENMASK_U32(15, 0); > > + ack++; > > + v &= GENMASK_U32(15, 0); > > + v |= (ack << 16); > > + mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v); > > + hw->mbx.stats.msgs_rx++; > > +} > > ... >
Maybe like this ? ... v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER); ack = FIELD_GET(GENMASK_U32(31, 16), v); ack++; v = (v & GENMASK_U32(15, 0)) | FIELD_PREP(GENMASK_U32(31, 16), ack); mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v); ...
> > +/** > > + * mucse_mbx_reset - Reset mbx info, sync info from regs > > + * @hw: pointer to the HW structure > > + * > > + * This function reset all mbx variables to default. > > + **/ > > +static void mucse_mbx_reset(struct mucse_hw *hw) > > +{ > > + struct mucse_mbx_info *mbx = &hw->mbx; > > + u32 v; > > + > > + v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER); > > + hw->mbx.fw_req = v & GENMASK_U32(15, 0); > > + hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0); > > I'd use FIELD_GET here too. >
Maybe like this? hw->mbx.fw_req = FIELD_GET(GENMASK_U32(15, 0), v); hw->mbx.fw_ack = FIELD_GET(GENMASK_U32(31, 16), v);
> > + mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0); > > + mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16)); > > +} > > ... >
Thanks for your feedback.
|  |