Re: [PATCH net-next v11 3/5] net: rnpgbe: Add basic mbx ops support

["Anwar, Md Danish" <a0501179@ti.com>]

On 9/9/2025 5:39 PM, Dong Yibo wrote:
> Add fundamental mailbox (MBX) communication operations between PF (Physical
> Function) and firmware for n500/n210 chips
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
>  drivers/net/ethernet/mucse/rnpgbe/Makefile    |   4 +-
>  drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  25 ++
>  .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   |  70 +++
>  drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |   7 +
>  .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   |   5 +
>  .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    | 425 ++++++++++++++++++
>  .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h    |  20 +
>  7 files changed, 555 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
>  create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
>  create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> 
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> index 9df536f0d04c..5fc878ada4b1 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -5,4 +5,6 @@
>  #

[ ... ]

> +
> +/**
> + * rnpgbe_init_hw - Setup hw info according to board_type
> + * @hw: hw information structure
> + * @board_type: board type
> + *
> + * rnpgbe_init_hw initializes all hw data
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
> +{
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +
> +	mbx->pf2fw_mbx_ctrl = MUCSE_GBE_PFFW_MBX_CTRL_OFFSET;
> +	mbx->fwpf_mbx_mask = MUCSE_GBE_FWPF_MBX_MASK_OFFSET;
> +
> +	switch (board_type) {
> +	case board_n500:
> +		rnpgbe_init_n500(hw);
> +	break;
> +	case board_n210:
> +		rnpgbe_init_n210(hw);
> +	break;
> +	default:
> +		return -EINVAL;
> +	}

The indentation of this switch block seems off to me.

As per the coding guidlines
https://www.kernel.org/doc/html/v4.14/process/coding-style.html#indentation

Break statements should be at the same indentation level as the case
code. The current indentation has the "break" statements at the same
level as the case labels, which is inconsistent.

This should be like,

	switch (board_type) {
	case board_n500:
		rnpgbe_init_n500(hw);
		break;
	case board_n210:
		rnpgbe_init_n210(hw);
		break;
	default:
		return -EINVAL;
	}

> +	/* init_params with mbx base */
> +	mucse_init_mbx_params_pf(hw);
> +
> +	return 0;
> +}

[ ... ]

> +/**
> + * mucse_read_mbx_pf - Read a message from the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * This function copies a message from the mailbox buffer to the caller's
> + * memory buffer. The presumption is that the caller knows that there was
> + * a message due to a fw request so no polling for message is needed.
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +	int size_in_words = size / 4;
> +	int ret;
> +	int i;
> +
> +	ret = mucse_obtain_mbx_lock_pf(hw);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < size_in_words; i++)
> +		msg[i] = mbx_data_rd32(mbx, MUCSE_MBX_FWPF_SHM + 4 * i);

The array indexing calculation should use multiplication by sizeof(u32)
instead of hardcoded 4.

> +	/* Hw needs write data_reg at last */
> +	mbx_data_wr32(mbx, MUCSE_MBX_FWPF_SHM, 0);
> +	/* flush reqs as we have read this request data */
> +	hw->mbx.fw_req = mucse_mbx_get_fwreq(mbx);
> +	mucse_mbx_inc_pf_ack(hw);
> +	mucse_release_mbx_lock_pf(hw, false);
> +
> +	return 0;
> +}
> +
> +/**
> + * mucse_check_for_msg_pf - Check to see if the fw has sent mail
> + * @hw: pointer to the HW structure
> + *
> + * Return: 0 if the fw has set the Status bit or else -EIO
> + **/
> +static int mucse_check_for_msg_pf(struct mucse_hw *hw)
> +{
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +	u16 fw_req;
> +
> +	fw_req = mucse_mbx_get_fwreq(mbx);
> +	/* chip's register is reset to 0 when rc send reset
> +	 * mbx command. This causes 'fw_req != hw->mbx.fw_req'
> +	 * be TRUE before fw really reply. Driver must wait fw reset
> +	 * done reply before using chip, we must check no-zero.
> +	 **/
> +	if (fw_req != 0 && fw_req != hw->mbx.fw_req) {
> +		hw->mbx.stats.reqs++;
> +		return 0;
> +	}
> +
> +	return -EIO;
> +}
> +
> +/**
> + * mucse_poll_for_msg - Wait for message notification
> + * @hw: pointer to the HW structure
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +static int mucse_poll_for_msg(struct mucse_hw *hw)
> +{
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +	int count = mbx->timeout_cnt;
> +	int val;
> +
> +	return read_poll_timeout(mucse_check_for_msg_pf,
> +				 val, !val, mbx->usec_delay,
> +				 count * mbx->usec_delay,
> +				 false, hw);
> +}
> +
> +/**
> + * mucse_poll_and_read_mbx - Wait for message notification and receive message
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * Return: 0 if it successfully received a message notification and
> + * copied it into the receive buffer
> + **/
> +int mucse_poll_and_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +	int ret;
> +
> +	ret = mucse_poll_for_msg(hw);
> +	if (ret)
> +		return ret;
> +
> +	return mucse_read_mbx_pf(hw, msg, size);
> +}
> +
> +/**
> + * mucse_mbx_get_fwack - Read fw ack from reg
> + * @mbx: pointer to the MBX structure
> + *
> + * Return: the fwack value
> + **/
> +static u16 mucse_mbx_get_fwack(struct mucse_mbx_info *mbx)
> +{
> +	u32 val = mbx_data_rd32(mbx, MUCSE_MBX_FW2PF_CNT);
> +
> +	return FIELD_GET(GENMASK_U32(31, 16), val);
> +}
> +
> +/**
> + * 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 val;
> +
> +	val = mbx_data_rd32(mbx, MUCSE_MBX_PF2FW_CNT);
> +	req = FIELD_GET(GENMASK_U32(15, 0), val);
> +	req++;
> +	val &= ~GENMASK_U32(15, 0);
> +	val |= FIELD_PREP(GENMASK_U32(15, 0), req);
> +	mbx_data_wr32(mbx, MUCSE_MBX_PF2FW_CNT, val);
> +	hw->mbx.stats.msgs_tx++;
> +}
> +
> +/**
> + * mucse_write_mbx_pf - Place a message in the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * This function maybe used in an irq handler.
> + *
> + * Return: 0 if it successfully copied message into the buffer
> + **/
> +static int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +	int size_in_words = size / 4;
> +	int ret;
> +	int i;
> +
> +	ret = mucse_obtain_mbx_lock_pf(hw);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < size_in_words; i++)
> +		mbx_data_wr32(mbx, MUCSE_MBX_FWPF_SHM + i * 4, msg[i]);

Same issue as above - should use sizeof(u32) instead of hardcoded 4 for
portability.

> +
> +	/* flush acks as we are overwriting the message buffer */
> +	hw->mbx.fw_ack = mucse_mbx_get_fwack(mbx);


-- 
Thanks and Regards,
Md Danish Anwar