RE: [PATCH 04/24] rtw89: add debug files

[Pkshih <pkshih@realtek.com>]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 13, 2021 12:50 PM
> To: Brian Norris
> Cc: Pkshih; kvalo@codeaurora.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 04/24] rtw89: add debug files
> 
> On Mon, Jul 12, 2021 at 06:57:37PM -0700, Brian Norris wrote:
> > On Mon, Jul 5, 2021 at 1:59 AM Pkshih <pkshih@realtek.com> wrote:
> > > > -----Original Message-----
> > > > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >
> > > > Based on this and other part of this driver I would recommend to use
> > > > regmap. It will provide to additional interface for the register
> > > > access. And typically for the network devices we have an ethtool
> > > > interface for that.
> > > >
> > >
> > > Could I know the 'regmap' you mentioned?
> >
> > include/linux/regmap.h
> > drivers/base/regmap/
> >
> > It's a driver framework and API for abstracting register accesses,
> > whether they are accessed directly via MMIO, or behind some kind of
> > indirect bus (I2C, SPI, etc.). It also happens to have its own debugfs
> > operators for doing various kinds of register get/set/dump. So if you
> > can successfully teach your driver to use it, then you don't need to
> > implement your own debugfs files for it.

Thanks for the information. I'll study to see if it is possible to use
regmap.

> >
> > I've only ever used regmap with Device Tree systems (which can more
> > easily specify syscon nodes, etc. -- see
> > Documentation/devicetree/bindings/regmap/regmap.txt). I'm totally
> > unfamiliar how to use this with ACPI (which I'm sure you want to
> > support). I'm sure it's possible somehow.
> >
> > FWIW, search engines turn up a few basic articles about it, if you
> > find its documentation or code examples too sparse:
> >
> https://www.collabora.com/news-and-blog/blog/2020/05/27/using-regmaps-to-make-linux-drivers-more-g
> eneric/
> 
> There are not ACPI specific register accesses in this driver. It is
> using simple readl/writel for the PCI accesses.
> 
> This driver would need to use devm_regmap_init() and register own
> regmap_bus. I motivate to use it not only to cover debugfs use case:
> 
> 1. Current driver implements only PCI access, but potentially wont to
> support SDIO and USB buses:
> 	drivers/net/wireless/realtek/rtw89/core.h
> 	enum rtw89_hci_type {
> 		RTW89_HCI_TYPE_PCIE,
> 		RTW89_HCI_TYPE_USB,
> 		RTW89_HCI_TYPE_SDIO,
> 	};
> 
> 
> SDIO and USB buses may return error on any access. So, driver
> should test if return value is error on every access. regmap read/write
> functions separate error value from actual register read value and
> motivate to handle errors in the driver. Suddenly not every mainline driver is
> doing it.

I know there are many changes if this driver supports SDIO and USB; not only
IO error but also running context. However, it is hard to me to imagine 
how to solve these problems before starting to implement.

I'll prepare my knowledge of regmap in advance.

> 
> 2. Current driver implements patter based error detection for the PCI
> bus:
> 
> 	static u32 rtw89_pci_ops_read32_cmac(struct rtw89_dev *rtwdev, u32 addr)
> 	{
> 		struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
> 		u32 val = readl(rtwpci->mmap + addr);
> 		int count;
> 
> 		for (count = 0; ; count++) {
> 			if (val != RTW89_R32_DEAD)
> 				return val;
> 			if (count >= MAC_REG_POOL_COUNT) {
> 				rtw89_warn(rtwdev, "addr %#x = %#x\n", addr, val);
> 				return RTW89_R32_DEAD;
> 			}
> 			rtw89_pci_ops_write32(rtwdev, R_AX_CK_EN, B_AX_CMAC_ALLCKEN);
> 			val = readl(rtwpci->mmap + addr);
> 		}
> 
> 		return val;
> 	}
> 

This isn't due to PCI bus error. Instead, this is because driver and firmware
can read the same register simultaneously, so we retry to get correct value.

> and handle this patter only in one place:
> 	....
> 	int rtw89_mac_check_mac_en(struct rtw89_dev *rtwdev, u8 mac_idx,
> 				   enum rtw89_mac_hwmod_sel sel)
> 	{
> 		....
> 		r_val = rtw89_read32(rtwdev, R_AX_SYS_ISO_CTRL_EXTEND);
> 
> 		....
> 
> 		if (r_val == RTW89_R32_EA || r_val == RTW89_R32_DEAD ||
> 		    (val & r_val) != val)
> 			return -EFAULT;
> 
> 		return 0;
> 	}
> 
> potentially this should be done on every read attempt for the PCI, and
> on every read/write for SDIO and USB buses.

The checking statement is not to check if reading error, but check if a certain
MAC function block is power-on or not.

> 
> 3. This driver has traces of watchdog support on the firmware side, so
> potentially, firmware can return error on any access if it was reset by
> the watchdog.

Firmware can't know if driver access error. It only detects hardware abnormal, and
send error code to driver to reset hardware, even do reconnection.

This depends on the frequency it happens. The user experience would be better
if we do softly reset, like we retry reading mentioned in 2. 

> 
> 4. regmap provide a way to define support register ranges and will warn
> if calculated register offset goes outside of this range.

Got it.

> 
> 5. regamp is endianness aware and will help to make this driver work
> properly on big-endian systems.
> 

As I know, the result of register access is CPU order; there's no endian issue.

In this driver, we must consider the endian of exchange between driver/firmware,
and we write these codes carefully with le32 type and le_xxx API, like le32_set_bits().
I think this driver can run on big-endian machines.

--
Ping-Ke