-----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:

quoted

On Mon, Jul 5, 2021 at 1:59 AM Pkshih [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]

quoted

quoted

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.

quoted

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