Sender: Larry Finger <larry.finger@gmail.com>
Message-ID: <2c354523-1603-0d0b-1880-1ff8dd00a58b@lwfinger.net>
Date:   Fri, 19 Nov 2021 14:06:54 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] rtw89: add AXIDMA and TX FIFO dump in mac_mem_dump
Content-Language: en-US
To:     Ping-Ke Shih <pkshih@realtek.com>, kvalo@codeaurora.org
Cc:     linux-wireless@vger.kernel.org, leo.li@realtek.com
References: <20211119055919.12954-1-pkshih@realtek.com>
From:   Larry Finger <Larry.Finger@lwfinger.net>
In-Reply-To: <20211119055919.12954-1-pkshih@realtek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/18/21 23:59, Ping-Ke Shih wrote:
> From: Chia-Yuan Li <leo.li@realtek.com>
> 
> The AXIDMA is tx/rx packet transmission between PCIE host
> and device, and TX FIFO is MAC TX data.
> We dump them to ensure these memory buffers correct.
> 
> Signed-off-by: Chia-Yuan Li <leo.li@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

The code in this patch is fine, but I think you may want to change the commit 
message. Te mo, "We dump them to ensure these memory buffers correct" means that 
the change here will fix the memory buffers. I think you want to say "We dump 
them to verify that these memory buffers are correct." This means that you are 
providing a way to dump the buffers so that their correctness can be determined.

Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net"

Larry

> ---
>   drivers/net/wireless/realtek/rtw89/debug.c |  5 +++++
>   drivers/net/wireless/realtek/rtw89/mac.h   | 10 ++++++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/debug.c b/drivers/net/wireless/realtek/rtw89/debug.c
> index 29eb188c888c7..dabee20b37c49 100644
> --- a/drivers/net/wireless/realtek/rtw89/debug.c
> +++ b/drivers/net/wireless/realtek/rtw89/debug.c
> @@ -723,6 +723,7 @@ rtw89_debug_priv_mac_mem_dump_select(struct file *filp,
>   }
>   
>   static const u32 mac_mem_base_addr_table[RTW89_MAC_MEM_MAX] = {
> +	[RTW89_MAC_MEM_AXIDMA]	        = AXIDMA_BASE_ADDR,
>   	[RTW89_MAC_MEM_SHARED_BUF]	= SHARED_BUF_BASE_ADDR,
>   	[RTW89_MAC_MEM_DMAC_TBL]	= DMAC_TBL_BASE_ADDR,
>   	[RTW89_MAC_MEM_SHCUT_MACHDR]	= SHCUT_MACHDR_BASE_ADDR,
> @@ -735,6 +736,10 @@ static const u32 mac_mem_base_addr_table[RTW89_MAC_MEM_MAX] = {
>   	[RTW89_MAC_MEM_BA_CAM]		= BA_CAM_BASE_ADDR,
>   	[RTW89_MAC_MEM_BCN_IE_CAM0]	= BCN_IE_CAM0_BASE_ADDR,
>   	[RTW89_MAC_MEM_BCN_IE_CAM1]	= BCN_IE_CAM1_BASE_ADDR,
> +	[RTW89_MAC_MEM_TXD_FIFO_0]	= TXD_FIFO_0_BASE_ADDR,
> +	[RTW89_MAC_MEM_TXD_FIFO_1]	= TXD_FIFO_1_BASE_ADDR,
> +	[RTW89_MAC_MEM_TXDATA_FIFO_0]	= TXDATA_FIFO_0_BASE_ADDR,
> +	[RTW89_MAC_MEM_TXDATA_FIFO_1]	= TXDATA_FIFO_1_BASE_ADDR,
>   };
>   
>   static void rtw89_debug_dump_mac_mem(struct seq_file *m,
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> index 6f3db8a2a9c2a..94cd29bd83d77 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -227,6 +227,7 @@ enum rtw89_mac_dbg_port_sel {
>   /* SRAM mem dump */
>   #define R_AX_INDIR_ACCESS_ENTRY 0x40000
>   
> +#define	AXIDMA_BASE_ADDR		0x18006000
>   #define	STA_SCHED_BASE_ADDR		0x18808000
>   #define	RXPLD_FLTR_CAM_BASE_ADDR	0x18813000
>   #define	SECURITY_CAM_BASE_ADDR		0x18814000
> @@ -240,10 +241,15 @@ enum rtw89_mac_dbg_port_sel {
>   #define	DMAC_TBL_BASE_ADDR		0x18800000
>   #define	SHCUT_MACHDR_BASE_ADDR		0x18800800
>   #define	BCN_IE_CAM1_BASE_ADDR		0x188A0000
> +#define	TXD_FIFO_0_BASE_ADDR		0x18856200
> +#define	TXD_FIFO_1_BASE_ADDR		0x188A1080
> +#define	TXDATA_FIFO_0_BASE_ADDR		0x18856000
> +#define	TXDATA_FIFO_1_BASE_ADDR		0x188A1000
>   
>   #define CCTL_INFO_SIZE		32
>   
>   enum rtw89_mac_mem_sel {
> +	RTW89_MAC_MEM_AXIDMA,
>   	RTW89_MAC_MEM_SHARED_BUF,
>   	RTW89_MAC_MEM_DMAC_TBL,
>   	RTW89_MAC_MEM_SHCUT_MACHDR,
> @@ -256,6 +262,10 @@ enum rtw89_mac_mem_sel {
>   	RTW89_MAC_MEM_BA_CAM,
>   	RTW89_MAC_MEM_BCN_IE_CAM0,
>   	RTW89_MAC_MEM_BCN_IE_CAM1,
> +	RTW89_MAC_MEM_TXD_FIFO_0,
> +	RTW89_MAC_MEM_TXD_FIFO_1,
> +	RTW89_MAC_MEM_TXDATA_FIFO_0,
> +	RTW89_MAC_MEM_TXDATA_FIFO_1,
>   
>   	/* keep last */
>   	RTW89_MAC_MEM_LAST,
>