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

Jörg Sommer
2025-09-16T18:43:19Z

[-- Attachment #1: Type: text/plain, Size: 21412 bytes --]

Hi,

only some minor suggestions.

Dong Yibo schrieb am Di 16. Sep, 19:29 (+0800):
> 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 | 420 ++++++++++++++++++
> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 20 +
> 7 files changed, 550 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 @@
> #
>
> obj-$(CONFIG_MGBE) += rnpgbe.o
> -rnpgbe-objs := rnpgbe_main.o
> +rnpgbe-objs := rnpgbe_main.o\
> + rnpgbe_chip.o\
> + rnpgbe_mbx.o
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 3b122dd508ce..7450bfc5ee98 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -4,13 +4,36 @@
> #ifndef _RNPGBE_H
> #define _RNPGBE_H
>
> +#include <linux/types.h>
> +
> enum rnpgbe_boards {
> board_n500,
> board_n210
> };
>
> +struct mucse_mbx_stats {
> + u32 msgs_tx; /* Number of messages sent from PF to fw */
> + u32 msgs_rx; /* Number of messages received from fw to PF */
> + u32 acks; /* Number of ACKs received from firmware */
> + u32 reqs; /* Number of requests sent to firmware */
> +};
> +
> +struct mucse_mbx_info {
> + struct mucse_mbx_stats stats;
> + u32 timeout_us;
> + u32 delay_us;
> + u16 fw_req;
> + u16 fw_ack;
> + /* fw <--> pf mbx */
> + u32 fwpf_shm_base;
> + u32 pf2fw_mbx_ctrl;
> + u32 fwpf_mbx_mask;
> + u32 fwpf_ctrl_base;
> +};
> +
> struct mucse_hw {
> void __iomem *hw_addr;
> + struct mucse_mbx_info mbx;
> };
>
> struct mucse {
> @@ -19,6 +42,8 @@ struct mucse {
> struct mucse_hw hw;
> };
>
> +int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
> +
> /* Device IDs */
> #define PCI_VENDOR_ID_MUCSE 0x8848
> #define PCI_DEVICE_ID_N500_QUAD_PORT 0x8308
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> new file mode 100644
> index 000000000000..86f1c75796b0
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#include <linux/errno.h>
> +
> +#include "rnpgbe.h"
> +#include "rnpgbe_hw.h"
> +#include "rnpgbe_mbx.h"
> +
> +/**
> + * rnpgbe_init_n500 - Setup n500 hw info
> + * @hw: hw information structure
> + *
> + * rnpgbe_init_n500 initializes all private
> + * structure for n500
> + **/
> +static void rnpgbe_init_n500(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + mbx->fwpf_ctrl_base = MUCSE_N500_FWPF_CTRL_BASE;
> + mbx->fwpf_shm_base = MUCSE_N500_FWPF_SHM_BASE;
> +}
> +
> +/**
> + * rnpgbe_init_n210 - Setup n210 hw info
> + * @hw: hw information structure
> + *
> + * rnpgbe_init_n210 initializes all private
> + * structure for n210
> + **/
> +static void rnpgbe_init_n210(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + mbx->fwpf_ctrl_base = MUCSE_N210_FWPF_CTRL_BASE;
> + mbx->fwpf_shm_base = MUCSE_N210_FWPF_SHM_BASE;
> +}
> +
> +/**
> + * 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

Maybe say that -EINVAL is the only error returned.

> + **/
> +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;
> + }
> + /* init_params with mbx base */
> + mucse_init_mbx_params_pf(hw);
> +
> + return 0;
> +}
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index 3a779806e8be..aad4cb2f4164 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -4,5 +4,12 @@
> #ifndef _RNPGBE_HW_H
> #define _RNPGBE_HW_H
>
> +#define MUCSE_N500_FWPF_CTRL_BASE 0x28b00
> +#define MUCSE_N500_FWPF_SHM_BASE 0x2d000
> +#define MUCSE_GBE_PFFW_MBX_CTRL_OFFSET 0x5500
> +#define MUCSE_GBE_FWPF_MBX_MASK_OFFSET 0x5700
> +#define MUCSE_N210_FWPF_CTRL_BASE 0x29400
> +#define MUCSE_N210_FWPF_SHM_BASE 0x2d900
> +
> #define RNPGBE_MAX_QUEUES 8
> #endif /* _RNPGBE_HW_H */
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 0afe39621661..c6cfb54f7c59 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -68,6 +68,11 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
> }
>
> hw->hw_addr = hw_addr;
> + err = rnpgbe_init_hw(hw, board_type);
> + if (err) {
> + dev_err(&pdev->dev, "Init hw err %d\n", err);
> + goto err_free_net;
> + }
>
> return 0;
>
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> new file mode 100644
> index 000000000000..7501a55c3134
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> @@ -0,0 +1,420 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2022 - 2025 Mucse Corporation. */
> +
> +#include <linux/errno.h>
> +#include <linux/bitfield.h>
> +#include <linux/iopoll.h>
> +
> +#include "rnpgbe_mbx.h"
> +
> +/**
> + * mbx_data_rd32 - Reads reg with base mbx->fwpf_shm_base
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + *
> + * Return: register value
> + **/
> +static u32 mbx_data_rd32(struct mucse_mbx_info *mbx, u32 reg)
> +{
> + struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> + return readl(hw->hw_addr + mbx->fwpf_shm_base + reg);
> +}
> +
> +/**
> + * mbx_data_wr32 - Writes value to reg with base mbx->fwpf_shm_base
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + * @value: value to be written
> + *
> + **/
> +static void mbx_data_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
> +{
> + struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> + writel(value, hw->hw_addr + mbx->fwpf_shm_base + reg);
> +}
> +
> +/**
> + * mbx_ctrl_rd32 - Reads reg with base mbx->fwpf_ctrl_base
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + *
> + * Return: register value
> + **/
> +static u32 mbx_ctrl_rd32(struct mucse_mbx_info *mbx, u32 reg)
> +{
> + struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> + return readl(hw->hw_addr + mbx->fwpf_ctrl_base + reg);
> +}
> +
> +/**
> + * mbx_ctrl_wr32 - Writes value to reg with base mbx->fwpf_ctrl_base
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + * @value: value to be written
> + *
> + **/
> +static void mbx_ctrl_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
> +{
> + struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> + writel(value, hw->hw_addr + mbx->fwpf_ctrl_base + reg);
> +}
> +
> +/**
> + * mucse_mbx_get_lock_pf - Write ctrl and read back lock status
> + * @hw: pointer to the HW structure
> + *
> + * Return: register value after write
> + **/
> +static u32 mucse_mbx_get_lock_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 reg = MUCSE_MBX_PF2FW_CTRL(mbx);
> +
> + mbx_ctrl_wr32(mbx, reg, MUCSE_MBX_PFU);
> +
> + return mbx_ctrl_rd32(mbx, reg);
> +}
> +
> +/**
> + * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
> + * @hw: pointer to the HW structure
> + *
> + * Pair with mucse_release_mbx_lock_pf()
> + * This function maybe used in an irq handler.
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 val;
> +
> + return read_poll_timeout_atomic(mucse_mbx_get_lock_pf,
> + val, val & MUCSE_MBX_PFU,
> + mbx->delay_us,
> + mbx->timeout_us,
> + false, hw);
> +}
> +
> +/**
> + * mucse_release_mbx_lock_pf - Release mailbox lock
> + * @hw: pointer to the HW structure
> + * @req: send a request or not
> + *
> + * Pair with mucse_obtain_mbx_lock_pf():
> + * - Releases the mailbox lock by clearing MUCSE_MBX_PFU bit
> + * - Simultaneously sends the request by setting MUCSE_MBX_REQ bit
> + * if req is true
> + * (Both bits are in the same mailbox control register,
> + * so operations are combined)
> + **/
> +static void mucse_release_mbx_lock_pf(struct mucse_hw *hw, bool req)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 reg = MUCSE_MBX_PF2FW_CTRL(mbx);
> +
> + if (req)
> + mbx_ctrl_wr32(mbx, reg, MUCSE_MBX_REQ);
> + else
> + mbx_ctrl_wr32(mbx, reg, 0);

Maybe combine this to:

mbx_ctrl_wr32(mbx, reg, req ? MUCSE_MBX_REQ : 0);

or also inlining reg:

mbx_ctrl_wr32(mbx, MUCSE_MBX_PF2FW_CTRL(mbx), req ? MUCSE_MBX_REQ : 0);

> +}
> +
> +/**
> + * mucse_mbx_get_fwreq - Read fw req from reg
> + * @mbx: pointer to the mbx structure
> + *
> + * Return: the fwreq value
> + **/
> +static u16 mucse_mbx_get_fwreq(struct mucse_mbx_info *mbx)
> +{
> + u32 val = mbx_data_rd32(mbx, MUCSE_MBX_FW2PF_CNT);
> +
> + return FIELD_GET(GENMASK_U32(15, 0), val);
> +}
> +
> +/**
> + * mucse_mbx_inc_pf_ack - Increase ack
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_inc_pf_ack reads pf_ack from hw, then writes
> + * 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 val;
> +
> + val = mbx_data_rd32(mbx, MUCSE_MBX_PF2FW_CNT);
> + ack = FIELD_GET(GENMASK_U32(31, 16), val);
> + ack++;
> + val &= ~GENMASK_U32(31, 16);
> + val |= FIELD_PREP(GENMASK_U32(31, 16), ack);
> + mbx_data_wr32(mbx, MUCSE_MBX_PF2FW_CNT, val);
> + hw->mbx.stats.msgs_rx++;
> +}
> +
> +/**
> + * mucse_read_mbx_pf - Read a message from the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * mucse_read_mbx_pf copies a message from the mbx 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;

* Make this `const int` or I'm in favour of inlining, because it's used only
once.

* Maybe `size / sizeof(*msg)` makes it more clearly where the 4 is coming
from.

> + int ret;
> + int i;

Because this variable is only used in the for-loop, I would narrow its scope
and write `for (int i = 0;`

> +
> + 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);
> + /* 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 I get this right, fw_req == 0 means that the request was send, but
nothing returned so far. Would not be -EAGAIN a better return value in this
case?

And fw_req == hw->mbx.fw_req means that we've got the old fw_req again which
means there was an error (-EIO).

> + if (fw_req != 0 && fw_req != hw->mbx.fw_req) {
> + hw->mbx.stats.reqs++;
> + return 0;
> + }
> +
> + return -EIO;

Only a suggestion: Might it be clearer to flip the cases and handle the if
as error case and continue with the success case?

if (fw_req == 0 || fw_req == hw->mbx.fw_req)
return -EIO;

hw->mbx.stats.reqs++;
return 0;

> +}
> +
> +/**
> + * 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 val;
> +
> + return read_poll_timeout(mucse_check_for_msg_pf,
> + val, !val, mbx->delay_us,
> + mbx->timeout_us,
> + 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, negative errno on failure
> + **/
> +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 reads pf_req from hw, then writes
> + * 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);

Why not assign the values in the declaration like done with mbx?

> + 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
> + *
> + * Return: 0 if it successfully copied message into the buffer

... "otherwise the error from obtaining the lock" or something better.

> + **/
> +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]);
> +
> + /* flush acks as we are overwriting the message buffer */
> + hw->mbx.fw_ack = mucse_mbx_get_fwack(mbx);
> + mucse_mbx_inc_pf_req(hw);
> + mucse_release_mbx_lock_pf(hw, true);
> +
> + return 0;
> +}
> +
> +/**
> + * mucse_check_for_ack_pf - Check to see if the fw has ACKed
> + * @hw: pointer to the HW structure
> + *
> + * Return: 0 if the fw has set the Status bit or else -EIO
> + **/
> +static int mucse_check_for_ack_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u16 fw_ack;
> +
> + fw_ack = mucse_mbx_get_fwack(mbx);
> + /* chip's register is reset to 0 when rc send reset
> + * mbx command. This causes 'fw_ack != hw->mbx.fw_ack'
> + * be TRUE before fw really reply. Driver must wait fw reset
> + * done reply before using chip, we must check no-zero.
> + **/
> + if (fw_ack != 0 && fw_ack != hw->mbx.fw_ack) {
> + hw->mbx.stats.acks++;
> + return 0;
> + }
> +
> + return -EIO;
> +}
> +
> +/**
> + * mucse_poll_for_ack - Wait for message acknowledgment
> + * @hw: pointer to the HW structure
> + *
> + * Return: 0 if it successfully received a message acknowledgment,
> + * else negative errno
> + **/
> +static int mucse_poll_for_ack(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int val;
> +
> + return read_poll_timeout(mucse_check_for_ack_pf,
> + val, !val, mbx->delay_us,
> + mbx->timeout_us,
> + false, hw);
> +}
> +
> +/**
> + * mucse_write_and_wait_ack_mbx - Write a message to the mailbox, wait for ack
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * Return: 0 if it successfully copied message into the buffer and
> + * received an ack to that message within delay * timeout_cnt period
> + **/
> +int mucse_write_and_wait_ack_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> + int ret;
> +
> + ret = mucse_write_mbx_pf(hw, msg, size);

If I see it right, this function is the only user of mucse_write_mbx_pf.

> + if (ret)
> + return ret;
> + return mucse_poll_for_ack(hw);

The same here?

> +}
> +
> +/**
> + * mucse_mbx_reset - Reset mbx info, sync info from regs
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_reset resets all mbx variables to default.
> + **/
> +static void mucse_mbx_reset(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 val;
> +
> + val = mbx_data_rd32(mbx, MUCSE_MBX_FW2PF_CNT);

Because val is assigned only once, you could make it `const u32 val = ...`

> + hw->mbx.fw_req = FIELD_GET(GENMASK_U32(15, 0), val);
> + hw->mbx.fw_ack = FIELD_GET(GENMASK_U32(31, 16), val);
> + mbx_ctrl_wr32(mbx, MUCSE_MBX_PF2FW_CTRL(mbx), 0);
> + mbx_ctrl_wr32(mbx, MUCSE_MBX_FWPF_MASK(mbx), GENMASK_U32(31, 16));
> +}
> +
> +/**
> + * mucse_init_mbx_params_pf - Set initial values for pf mailbox
> + * @hw: pointer to the HW structure
> + *
> + * Initializes the hw->mbx struct to correct values for pf mailbox
> + */
> +void mucse_init_mbx_params_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + mbx->delay_us = 100;
> + mbx->timeout_us = 4 * USEC_PER_SEC;
> + mbx->stats.msgs_tx = 0;
> + mbx->stats.msgs_rx = 0;
> + mbx->stats.reqs = 0;
> + mbx->stats.acks = 0;
> + mucse_mbx_reset(hw);

Is mucse_init_mbx_params_pf the only user of mucse_mbx_reset? If so, the
function should inlined.

> +}
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> new file mode 100644
> index 000000000000..eac99f13b6ff
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#ifndef _RNPGBE_MBX_H
> +#define _RNPGBE_MBX_H
> +
> +#include "rnpgbe.h"
> +
> +#define MUCSE_MBX_FW2PF_CNT 0
> +#define MUCSE_MBX_PF2FW_CNT 4
> +#define MUCSE_MBX_FWPF_SHM 8

Are these constants used by other c files or should they be private to
rnpgbe_mbx.c?

> +#define MUCSE_MBX_PF2FW_CTRL(mbx) ((mbx)->pf2fw_mbx_ctrl)
> +#define MUCSE_MBX_FWPF_MASK(mbx) ((mbx)->fwpf_mbx_mask)
> +#define MUCSE_MBX_REQ BIT(0) /* Request a req to mailbox */
> +#define MUCSE_MBX_PFU BIT(3) /* PF owns the mailbox buffer */
> +
> +int mucse_write_and_wait_ack_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
> +void mucse_init_mbx_params_pf(struct mucse_hw *hw);
> +int mucse_poll_and_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
> +#endif /* _RNPGBE_MBX_H */
> --
> 2.25.1
>
>

--
“Perl—the only language that looks the same
before and after RSA encryption.” (Keith Bostic)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]