Received-SPF: pass (google.com: domain of linux-kernel+bounces-17661-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 5 Jan 2024 10:33:11 +0100
From: Jiri Pirko <jiri@resnulli.us>
To: Justin Lai <justinlai0215@realtek.com>
Cc: kuba@kernel.org, davem@davemloft.net, edumazet@google.com,
	pabeni@redhat.com, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, andrew@lunn.ch, pkshih@realtek.com,
	larry.chiu@realtek.com
Subject: Re: [PATCH net-next v15 01/13] rtase: Add pci table supported in
 this module
Message-ID: <ZZfM1yKMIpSgGWGZ@nanopsycho>
References: <20240105085737.376885-1-justinlai0215@realtek.com>
 <20240105085737.376885-2-justinlai0215@realtek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240105085737.376885-2-justinlai0215@realtek.com>

Fri, Jan 05, 2024 at 09:57:25AM CET, justinlai0215@realtek.com wrote:
>Add pci table supported in this module, and implement pci_driver function
>to initialize this driver, remove this driver, or shutdown this driver.
>
>Signed-off-by: Justin Lai <justinlai0215@realtek.com>
>---
> drivers/net/ethernet/realtek/rtase/rtase.h    | 336 ++++++++++
> .../net/ethernet/realtek/rtase/rtase_main.c   | 617 ++++++++++++++++++
> 2 files changed, 953 insertions(+)
> create mode 100644 drivers/net/ethernet/realtek/rtase/rtase.h
> create mode 100644 drivers/net/ethernet/realtek/rtase/rtase_main.c
>
>diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h b/drivers/net/ethernet/realtek/rtase/rtase.h
>new file mode 100644
>index 000000000000..9874be9cd153
>--- /dev/null
>+++ b/drivers/net/ethernet/realtek/rtase/rtase.h
>@@ -0,0 +1,336 @@
>+/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>+/*
>+ *  rtase is the Linux device driver released for Realtek Automotive Switch
>+ *  controllers with PCI-Express interface.
>+ *
>+ *  Copyright(c) 2023 Realtek Semiconductor Corp.
>+ */
>+
>+#ifndef _RTASE_H_
>+#define _RTASE_H_
>+
>+/* the low 32 bit address of receive buffer must be 8-byte alignment. */
>+#define RTK_RX_ALIGN 8
>+
>+#define HW_VER_MASK 0x7C800000
>+
>+#define RX_DMA_BURST_256       4
>+#define TX_DMA_BURST_UNLIMITED 7
>+#define RX_BUF_SIZE            (PAGE_SIZE - \
>+				SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
>+#define MAX_JUMBO_SIZE         (RX_BUF_SIZE - VLAN_ETH_HLEN - ETH_FCS_LEN)
>+
>+/* 3 means InterFrameGap = the shortest one */
>+#define INTERFRAMEGAP 0x03
>+
>+#define RTASE_REGS_SIZE     256
>+#define RTASE_PCI_REGS_SIZE 0x100
>+
>+#define MULTICAST_FILTER_MASK  GENMASK(30, 26)
>+#define MULTICAST_FILTER_LIMIT 32
>+
>+#define RTASE_VLAN_FILTER_ENTRY_NUM 32
>+#define RTASE_NUM_TX_QUEUE 8
>+#define RTASE_NUM_RX_QUEUE 4
>+
>+#define RTASE_TXQ_CTRL      1
>+#define RTASE_FUNC_TXQ_NUM  1
>+#define RTASE_FUNC_RXQ_NUM  1
>+#define RTASE_INTERRUPT_NUM 1
>+
>+#define MITI_TIME_COUNT_MASK     GENMASK(3, 0)
>+#define MITI_TIME_UNIT_MASK      GENMASK(7, 4)
>+#define MITI_DEFAULT_TIME        128
>+#define MITI_MAX_TIME            491520
>+#define MITI_PKT_NUM_COUNT_MASK  GENMASK(11, 8)
>+#define MITI_PKT_NUM_UNIT_MASK   GENMASK(13, 12)
>+#define MITI_DEFAULT_PKT_NUM     64
>+#define MITI_MAX_PKT_NUM_IDX     3
>+#define MITI_MAX_PKT_NUM_UNIT    16
>+#define MITI_MAX_PKT_NUM         240
>+#define MITI_COUNT_BIT_NUM       4
>+
>+#define RTASE_NUM_MSIX 4
>+
>+#define RTASE_DWORD_MOD 16
>+
>+/*****************************************************************************/
>+enum rtase_registers {
>+	RTASE_MAC0   = 0x0000,
>+	RTASE_MAC4   = 0x0004,
>+	RTASE_MAR0   = 0x0008,
>+	RTASE_MAR1   = 0x000C,
>+	RTASE_DTCCR0 = 0x0010,
>+	RTASE_DTCCR4 = 0x0014,
>+#define COUNTER_RESET BIT(0)
>+#define COUNTER_DUMP  BIT(3)
>+
>+	RTASE_FCR    = 0x0018,
>+#define FCR_RXQ_MASK    GENMASK(5, 4)
>+#define FCR_VLAN_FTR_EN BIT(1)
>+
>+	RTASE_LBK_CTRL = 0x001A,
>+#define LBK_ATLD BIT(1)
>+#define LBK_CLR  BIT(0)
>+
>+	RTASE_TX_DESC_ADDR0   = 0x0020,
>+	RTASE_TX_DESC_ADDR4   = 0x0024,
>+	RTASE_TX_DESC_COMMAND = 0x0028,
>+#define TX_DESC_CMD_CS BIT(15)
>+#define TX_DESC_CMD_WE BIT(14)
>+
>+	RTASE_BOOT_CTL  = 0x6004,
>+	RTASE_CLKSW_SET = 0x6018,
>+
>+	RTASE_CHIP_CMD = 0x0037,
>+#define STOP_REQ      BIT(7)
>+#define STOP_REQ_DONE BIT(6)
>+#define RE            BIT(3)
>+#define TE            BIT(2)
>+
>+	RTASE_IMR0 = 0x0038,
>+	RTASE_ISR0 = 0x003C,
>+#define TOK7 BIT(30)
>+#define TOK6 BIT(28)
>+#define TOK5 BIT(26)
>+#define TOK4 BIT(24)
>+#define FOVW BIT(6)
>+#define RDU  BIT(4)
>+#define TOK  BIT(2)
>+#define ROK  BIT(0)
>+
>+	RTASE_IMR1 = 0x0800,
>+	RTASE_ISR1 = 0x0802,
>+#define Q_TOK BIT(4)
>+#define Q_RDU BIT(1)
>+#define Q_ROK BIT(0)
>+
>+	RTASE_EPHY_ISR = 0x6014,
>+	RTASE_EPHY_IMR = 0x6016,
>+
>+	RTASE_TX_CONFIG_0 = 0x0040,
>+#define TX_INTER_FRAME_GAP_MASK GENMASK(25, 24)
>+	/* DMA burst value (0-7) is shift this many bits */
>+#define TX_DMA_MASK             GENMASK(10, 8)
>+
>+	RTASE_RX_CONFIG_0 = 0x0044,
>+#define RX_SINGLE_FETCH  BIT(14)
>+#define RX_SINGLE_TAG    BIT(13)
>+#define RX_MX_DMA_MASK   GENMASK(10, 8)
>+#define ACPT_FLOW        BIT(7)
>+#define ACCEPT_ERR       BIT(5)
>+#define ACCEPT_RUNT      BIT(4)
>+#define ACCEPT_BROADCAST BIT(3)
>+#define ACCEPT_MULTICAST BIT(2)
>+#define ACCEPT_MYPHYS    BIT(1)
>+#define ACCEPT_ALLPHYS   BIT(0)
>+#define ACCEPT_MASK      (ACPT_FLOW | ACCEPT_ERR | ACCEPT_RUNT | \
>+			  ACCEPT_BROADCAST | ACCEPT_MULTICAST | \
>+			  ACCEPT_MYPHYS | ACCEPT_ALLPHYS)
>+
>+	RTASE_RX_CONFIG_1 = 0x0046,
>+#define RX_MAX_FETCH_DESC_MASK  GENMASK(15, 11)
>+#define RX_NEW_DESC_FORMAT_EN   BIT(8)
>+#define OUTER_VLAN_DETAG_EN     BIT(7)
>+#define INNER_VLAN_DETAG_EN     BIT(6)
>+#define PCIE_NEW_FLOW           BIT(2)
>+#define PCIE_RELOAD_En          BIT(0)

In this header, you have *a lot* of defines that are very specific to
rtase, yet they don't have the prefix so the reader can't identify them
right away. Seeing things like "ACCEPT_ERR" or "MAX_JUMBO_SIZE" might
falsely lead the reader to think this is something common throught the
kernel code.

Please pit prefix to all of these.


>+
>+	RTASE_EEM = 0x0050,
>+#define EEM_UNLOCK 0xC0
>+
>+	RTASE_TDFNR  = 0x0057,
>+	RTASE_TPPOLL = 0x0090,
>+	RTASE_PDR    = 0x00B0,
>+	RTASE_FIFOR  = 0x00D3,
>+#define TX_FIFO_EMPTY BIT(5)
>+#define RX_FIFO_EMPTY BIT(4)
>+
>+	RTASE_PCPR = 0x00D8,
>+#define PCPR_VLAN_FTR_EN BIT(6)
>+
>+	RTASE_RMS       = 0x00DA,
>+	RTASE_CPLUS_CMD = 0x00E0,
>+#define FORCE_RXFLOW_EN BIT(11)
>+#define FORCE_TXFLOW_EN BIT(10)
>+#define RX_CHKSUM       BIT(5)
>+
>+	RTASE_Q0_RX_DESC_ADDR0 = 0x00E4,
>+	RTASE_Q0_RX_DESC_ADDR4 = 0x00E8,
>+	RTASE_Q1_RX_DESC_ADDR0 = 0x4000,
>+	RTASE_Q1_RX_DESC_ADDR4 = 0x4004,
>+	RTASE_MTPS             = 0x00EC,
>+#define TAG_NUM_SEL_MASK  GENMASK(10, 8)
>+
>+	RTASE_MISC = 0x00F2,
>+#define RX_DV_GATE_EN BIT(3)
>+
>+	RTASE_TFUN_CTRL = 0x0400,
>+#define TX_NEW_DESC_FORMAT_EN BIT(0)
>+
>+	RTASE_TX_CONFIG_1 = 0x203E,
>+#define TC_MODE_MASK  GENMASK(11, 10)
>+
>+	RTASE_TOKSEL      = 0x2046,
>+	RTASE_RFIFONFULL  = 0x4406,
>+	RTASE_INT_MITI_TX = 0x0A00,
>+	RTASE_INT_MITI_RX = 0x0A80,
>+
>+	RTASE_VLAN_ENTRY_MEM_0 = 0x7234,
>+	RTASE_VLAN_ENTRY_0     = 0xAC80,
>+};
>+
>+enum desc_status_bit {
>+	DESC_OWN = BIT(31), /* Descriptor is owned by NIC */
>+	RING_END = BIT(30), /* End of descriptor ring */
>+};
>+
>+enum sw_flag_content {
>+	SWF_MSI_ENABLED  = BIT(1),
>+	SWF_MSIX_ENABLED = BIT(2),
>+};
>+
>+#define RSVD_MASK 0x3FFFC000
>+
>+struct tx_desc {

The note above also applies to enum and structs names too.


>+	__le32 opts1;
>+	__le32 opts2;
>+	__le64 addr;
>+	__le32 opts3;
>+	__le32 reserved1;
>+	__le32 reserved2;
>+	__le32 reserved3;
>+} __packed;

[...]