Re: [PATCH v1 03/15] net/xsc: add the register HAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] net/xsc: duplicate declarations, dead definitions, and missing types.h in HAL
--

commit 2bedc4243c5f325b253b8d3867197c1b8085594e
Author: Xin Tian <tianx@yunsilicon.com>

net/xsc: add the register HAL

This commit adds the hardware abstraction layer for translating logical
register accesses into BAR-relative MMIO for the Andes chip generation. It
implements doorbell encodings, command-queue register accessors, and WQE/CQE
and MR helper functions.

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c b/drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c
> new file mode 100644
> index 0000000000000..69f0daae5abe6
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c
> @@ -0,0 +1,629 @@

[ ... ]

> +static void andes_ring_tx_doorbell(struct xsc_core_device *xdev,
> +				   struct xsc_hw_reg *regs,
> +				   void __iomem *bar, u32 sqn, u32 next_pid,
> +				   bool mdb)
> +{
> +	u32 db = FIELD_PREP(XSC_ANDES_TX_DB_DATA0_NEXT_PID_MASK, next_pid) |
> +		 FIELD_PREP(XSC_ANDES_TX_DB_DATA0_QP_ID_MASK, sqn);
> +	u32 db_reg = mdb ? QPM_TX_MDB_BASE_REG_ADDR + (sqn & (XSC_MDB_NUM - 1)) * sizeof(u32) :
> +		regs->tx_db;
> +
> +	/* Make sure that descriptors are written before
> +	 * updating doorbell record and ringing the doorbell
> +	 */
> +	wmb();
> +
> +	writel(db, REG_ADDR(bar, db_reg));
> +}

[ ... ]

> +static u8 andes_get_mr_page_mode(u8 page_shift)
> +{
> +enum {
> +	PAGE_SHIFT_4K	= 12,
> +	XSC_PAGE_SHIFT_64K	= 16,
> +	XSC_PAGE_SHIFT_2M	= 21,
> +	XSC_PAGE_SHIFT_1G	= 30,
> +};
> +
> +enum {
> +	XSC_PAGE_MODE_4K	= 0,
> +	XSC_PAGE_MODE_64K	= 1,
> +	XSC_PAGE_MODE_2M	= 2,
> +	XSC_PAGE_MODE_1G	= 3,
> +};
> +
> +	return (page_shift == XSC_PAGE_SHIFT_4K ? XSC_PAGE_MODE_4K :
> +		(page_shift == XSC_PAGE_SHIFT_64K ? XSC_PAGE_MODE_64K :
> +		(page_shift == XSC_PAGE_SHIFT_2M ? XSC_PAGE_MODE_2M : XSC_PAGE_MODE_1G)));
> +}

[ ... ]

> +struct andes_send_wqe_ctrl_seg {
> +	__le32		data0;
> +	__le32		msg_len;
> +	__le32		data2;
> +	__le32		data3;
> +};
> +
> +static void andes_set_wqe_id(void *cseg, u32 wqe_id)
> +{
> +	struct xsc_send_wqe_ctrl_seg *seg = cseg;
> +	u32 val = le32_to_cpu(seg->data0);
> +
> +	seg->data0 = cpu_to_le32((val & ~XSC_WQE_CTRL_SEG_DATA0_WQE_ID_MASK) |
> +				 FIELD_PREP(XSC_WQE_CTRL_SEG_DATA0_WQE_ID_MASK,
> +					    (u16)wqe_id));
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/hal/xsc_hal.h b/drivers/net/ethernet/yunsilicon/xsc/pci/hal/xsc_hal.h
> new file mode 100644
> index 0000000000000..f6292a91ba2fb
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/hal/xsc_hal.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2024 - 2026, Shanghai Yunsilicon Technology Co., Ltd.
> + * All rights reserved.
> + */
> +
> +#ifndef XSC_HAL_H
> +#define XSC_HAL_H
> +
> +struct xsc_core_device;
> +
> +/* HAL entry points exported to the PCI/RDMA/ETH layers */
> +void xsc_init_hal(struct xsc_core_device *xdev);
> +void xsc_set_mtt_info(struct xsc_core_device *xdev);
> +void xsc_set_pf_db_addr(struct xsc_core_device *xdev,
> +			u64 tx_db, u64 rx_db, u64 cq_db, u64 cq_reg, u64 eq_db);
> +void xsc_get_db_addr(struct xsc_core_device *xdev,
> +		     u64 *tx_db, u64 *rx_db, u64 *cq_db, u64 *cq_reg, u64 *eq_db);
> +
> +void xsc_update_tx_db(struct xsc_core_device *xdev, u32 sqn, u32 next_pid);
> +void xsc_update_rx_db(struct xsc_core_device *xdev, u32 rqn, u32 next_pid);
> +void xsc_arm_cq(struct xsc_core_device *xdev, u32 cqn, u32 next_cid, u8 solicited);
> +void xsc_update_cq_ci(struct xsc_core_device *xdev, u32 cqn, u32 next_cid);

[Severity: Low]
This isn't a bug, but there are a few unused definitions and duplicated
declarations across the new HAL files:

In drivers/net/ethernet/yunsilicon/xsc/pci/hal/xsc_hal.h, the prototypes use
u64, u32, u16, u8, and bool without including linux/types.h, and 19 of the
function declarations (such as xsc_init_hal(), xsc_set_mtt_info(),
xsc_set_pf_db_addr(), xsc_get_db_addr(), and the doorbell and WQE/CQE
helpers) duplicate prototypes that are already declared in
drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h:

drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h:
    void xsc_init_hal(struct xsc_core_device *xdev);

    u8 xsc_get_mr_page_mode(struct xsc_core_device *xdev, u8 page_shift);
    u64 xsc_get_max_mr_size(struct xsc_core_device *xdev);
    ...

In drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c:
- In andes_ring_tx_doorbell(), the xdev parameter is unused.
- In andes_get_mr_page_mode(), the local unindented enum defines
  PAGE_SHIFT_4K and XSC_PAGE_SHIFT_1G, neither of which is used because the
  return expression compares against XSC_PAGE_SHIFT_4K from
  common/xsc_core.h and defaults to XSC_PAGE_MODE_1G.
- struct andes_send_wqe_ctrl_seg is defined right above andes_set_wqe_id()
  but is never referenced because andes_set_wqe_id() casts cseg to
  struct xsc_send_wqe_ctrl_seg from common/xsc_core.h; similarly,
  struct andes_cqe and struct andes_data_seg duplicate struct xsc_cqe and
  struct xsc_wqe_data_seg in common/xsc_core.h.

Could these unused definitions be dropped and the header declarations
consolidated with common/xsc_core.h (and linux/types.h included in
xsc_hal.h)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=3