Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org DD790C4338F
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v6 07/24] rtw89: add MAC files
References: <20210820043538.12424-1-pkshih@realtek.com>
        <20210820043538.12424-8-pkshih@realtek.com>
Date:   Fri, 01 Oct 2021 19:13:51 +0300
In-Reply-To: <20210820043538.12424-8-pkshih@realtek.com> (Ping-Ke Shih's
        message of "Fri, 20 Aug 2021 12:35:21 +0800")
Message-ID: <87fstkiuyo.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Ping-Ke Shih <pkshih@realtek.com> writes:

> Provide interfaces to access MAC function blocks including power-on/off
> configuration.
>
> MAC initialization does power-on, and then enable HCI to download firmware.
> The BB, RF, DMAC and CMAC function blocks are enabled, and then configure
> TXRX settings that contains DMAC, CMAC, interrupt mask, and RPQ parameters.
> DMAC is short for data MAC, including data link engine (DLE), HCI function
> control (HFC), MPDU processor, security engine and so on. CMAC is short for
> control MAC, including scheduler, address CAM, RX filter, CCA control,
> TMAC, RMAC, protocol component and so on.
>
> The full MAC initialization is done for normal use case when user does
> network interface up. When device is probing, driver does partial
> initialization to do power-on and download firmware, because we need to
> read hardware capabilities from efuse and firmware.
>
> MAC supports five ports, so we can have five VIFs at most. To control MAC
> port, we control a set of registers, and the most important one among them
> is port_cfg register named R_AX_PORT_CFG_P0. We can turn on/off the port
> function and configure network type (STA or AP mode) by this register.
> The address and BSSID corresponding to this port are given by address
> and BSSID CAM that is set by firmware via H2C command.
>
> Since BT coexistence code needs to access coex registers, some help
> functions are provided to make WiFi-only case work.
>
> To access MAC registers, normally we use rtw89_writeXX/rtw89_readYY, but
> we use rtw89_mac_txpwr_write32_ZZ to access TX power register with proper
> address range checking.
>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

> +struct rtw89_hfc_prec_cfg rtw_hfc_preccfg_pcie = {
> +	2, 40, 0, 0, 1, 0, 0, 0
> +};

This should be const.

> +static int rtw89_mac_pwr_seq(struct rtw89_dev *rtwdev,
> +			     const struct rtw89_pwr_cfg * const *cfg_seq)
> +{
> +	u32 idx = 0;
> +	const struct rtw89_pwr_cfg *cfg;
> +	int ret;
> +
> +	do {
> +		cfg = cfg_seq[idx];
> +		if (!cfg)
> +			break;
> +
> +		ret = rtw89_mac_sub_pwr_seq(rtwdev, BIT(rtwdev->hal.cv),
> +					    PWR_INTF_MSK_PCIE, cfg);
> +		if (ret)
> +			return -EBUSY;
> +
> +		idx++;
> +	} while (1);

Never ending loop is a bad idea in kernel, please add either a loop
limit or a time limit.

> +static int rtw89_mac_power_switch(struct rtw89_dev *rtwdev, bool on)
> +{
> +#define PWR_ACT 1
> +	const struct rtw89_chip_info *chip = rtwdev->chip;
> +	const struct rtw89_pwr_cfg * const *cfg_seq;
> +	struct rtw89_hal *hal = &rtwdev->hal;
> +	int ret;
> +	u8 val;
> +
> +	if (on)
> +		cfg_seq = chip->pwr_on_seq;
> +	else
> +		cfg_seq = chip->pwr_off_seq;
> +
> +	if (test_bit(RTW89_FLAG_FW_RDY, rtwdev->flags))
> +		__rtw89_leave_ps_mode(rtwdev);
> +
> +	val = rtw89_read8(rtwdev, 0x3F1) & 0x3;

Magic numbers.

> +/* PCIE 64 */
> +struct rtw89_dle_size wde_size0 = {
> +	RTW89_WDE_PG_64, 4095, 1,
> +};
> +
> +/* DLFW */
> +struct rtw89_dle_size wde_size4 = {
> +	RTW89_WDE_PG_64, 0, 4096,
> +};
> +
> +/* PCIE */
> +struct rtw89_dle_size ple_size0 = {
> +	RTW89_PLE_PG_128, 1520, 16,
> +};
> +
> +/* DLFW */
> +struct rtw89_dle_size ple_size4 = {
> +	RTW89_PLE_PG_128, 64, 1472,
> +};
> +
> +/* PCIE 64 */
> +struct rtw89_wde_quota wde_qt0 = {
> +	3792, 196, 0, 107,
> +};
> +
> +/* DLFW */
> +struct rtw89_wde_quota wde_qt4 = {
> +	0, 0, 0, 0,
> +};
> +
> +/* PCIE SCC */
> +struct rtw89_ple_quota ple_qt4 = {
> +	264, 0, 16, 20, 26, 13, 356, 0, 32, 40, 8,
> +};
> +
> +/* PCIE SCC */
> +struct rtw89_ple_quota ple_qt5 = {
> +	264, 0, 32, 20, 64, 13, 1101, 0, 64, 128, 120,
> +};
> +
> +/* DLFW */
> +struct rtw89_ple_quota ple_qt13 = {
> +	0, 0, 16, 48, 0, 0, 0, 0, 0, 0, 0
> +};

These all should be const.

> +static void (*rtw89_mac_c2h_ofld_handler[])(struct rtw89_dev *rtwdev,
> +					    struct sk_buff *c2h, u32 len) = {
> +	[RTW89_MAC_C2H_FUNC_EFUSE_DUMP] = NULL,
> +	[RTW89_MAC_C2H_FUNC_READ_RSP] = NULL,
> +	[RTW89_MAC_C2H_FUNC_PKT_OFLD_RSP] = NULL,
> +	[RTW89_MAC_C2H_FUNC_BCN_RESEND] = NULL,
> +	[RTW89_MAC_C2H_FUNC_MACID_PAUSE] = rtw89_mac_c2h_macid_pause,
> +};
> +
> +static void (*rtw89_mac_c2h_info_handler[])(struct rtw89_dev *rtwdev,
> +					    struct sk_buff *c2h, u32 len) = {
> +	[RTW89_MAC_C2H_FUNC_REC_ACK] = rtw89_mac_c2h_rec_ack,
> +	[RTW89_MAC_C2H_FUNC_DONE_ACK] = rtw89_mac_c2h_done_ack,
> +	[RTW89_MAC_C2H_FUNC_C2H_LOG] = rtw89_mac_c2h_log,
> +};

const?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches