RE: [PATCH] wifi: rtl8xxxu: Support new chip RTL8710BU aka RTL8188GU

[Ping-Ke Shih <pkshih@realtek.com>]

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Tuesday, February 21, 2023 12:11 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> Subject: [PATCH] wifi: rtl8xxxu: Support new chip RTL8710BU aka RTL8188GU
> 
> This chip is found in cheap "free driver" USB adapters from Aliexpress.
> Initially they pretend to be a CD-ROM containing the driver for Windows.
> "Ejecting" switches the device to wifi mode.
> 
> Features: 2.4 GHz, b/g/n mode, 1T1R, 150 Mbps.
> 
> This chip is more unique than other Realtek chips:
> 
> * The registers at addresses 0x0-0xff, which all the other chips use,
>   can't be used here. New registers at 0x8000-0x80ff must be used
>   instead. And it's not a simple matter of adding 0x8000: 0x2
>   (REG_SYS_FUNC) became 0x8004, 0x80 (REG_MCU_FW_DL) became 0x8090,
>   etc.
> 
> * Also there are a few new registers which must be accessed indirectly
>   because their addresses don't fit in 16 bits. No other chips seem to
>   have these.
> 
> * The vendor driver compiles to 8188gu.ko, but the code calls the chip
>   RTL8710B(U) pretty much everywhere, including messages visible to the
>   user.
> 
> Another difference compared to the other chips supported by rtl8xxxu is
> that it has a new PHY status struct, or three of them actually, from
> which we extract the RSSI, among other things. This is not unique,
> though, just new. The chips supported by rtw88 also use it.
> 
> This was tested with a device from vendor SMIC with romver 1. It's
> possible devices from vendor UMC and/or with romver 2 will fail to
> upload the firmware. The vendor driver has that problem.

Can I know the version of vendor driver you use? 

> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/Kconfig |    2 +-
>  .../net/wireless/realtek/rtl8xxxu/Makefile    |    2 +-
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  304 ++-
>  .../realtek/rtl8xxxu/rtl8xxxu_8188e.c         |    2 +
>  .../realtek/rtl8xxxu/rtl8xxxu_8188f.c         |    4 +-
>  .../realtek/rtl8xxxu/rtl8xxxu_8192c.c         |    2 +
>  .../realtek/rtl8xxxu/rtl8xxxu_8192e.c         |    2 +
>  .../realtek/rtl8xxxu/rtl8xxxu_8710b.c         | 1942 +++++++++++++++++
>  .../realtek/rtl8xxxu/rtl8xxxu_8723a.c         |    2 +
>  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         |    2 +
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c |  181 +-
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_regs.h |   43 +
>  12 files changed, 2435 insertions(+), 53 deletions(-)
>  create mode 100644 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8710b.c
> 

[...]

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index c8cee4a24755..2e5fa9479134 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -103,7 +103,8 @@ enum rtl8xxxu_rtl_chip {
>         RTL8822B = 0x8822b,
>         RTL8703B = 0x8703b,
>         RTL8195A = 0x8195a,
> -       RTL8188F = 0x8188f
> +       RTL8188F = 0x8188f,
> +       RTL8710B = 0x8710b

Add a trailing comma, and then you don't need to stir the last line when adding
an new chip in the future.

[...]

>  struct rtl8xxxu_reg8val {
>         u16 reg;
>         u8 val;
> @@ -1486,6 +1769,7 @@ struct rtl8xxxu_priv {
>         struct rtl8723au_idx ht20_tx_power_diff[RTL8723B_TX_COUNT];
>         struct rtl8723au_idx ht40_tx_power_diff[RTL8723B_TX_COUNT];
>         struct rtl8xxxu_power_base *power_base;
> +       u8 package_type;
>         u32 chip_cut:4;
>         u32 rom_rev:4;
>         u32 is_multi_func:1;
> @@ -1505,6 +1789,7 @@ struct rtl8xxxu_priv {
>         u32 ep_tx_low_queue:1;
>         u32 rx_buf_aggregation:1;
>         u32 cck_agc_report_type:1;
> +       u32 cck_new_agc:1;
>         u8 default_crystal_cap;
>         unsigned int pipe_interrupt;
>         unsigned int pipe_in;
> @@ -1523,6 +1808,8 @@ struct rtl8xxxu_priv {
> 
>         struct mutex h2c_mutex;
> 
> +       struct mutex syson_indirect_access_mutex;
> +

I remember checkpatch will warn if you don't add a comment for mutex/lock.
Though the naming is clear, just follow the rule.

[...]

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8710b.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8710b.c
> new file mode 100644
> index 000000000000..d80125157514
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8710b.c

[...]

> +static u32 rtl8710b_indirect_read32(struct rtl8xxxu_priv *priv, u32 addr)
> +{
> +       struct device *dev = &priv->udev->dev;
> +       u32 val32, value = 0xffffffff;
> +       u8 polling_count = 0xff;
> +
> +       if (addr & 3) {
> +               dev_warn(dev, "%s: Aborting because 0x%x is not a multiple of 4.\n",
> +                        __func__, addr);
> +               return value;
> +       }
> +
> +       mutex_lock(&priv->syson_indirect_access_mutex);
> +
> +       rtl8xxxu_write32(priv, REG_USB_HOST_INDIRECT_ADDR_8710B, addr);
> +       rtl8xxxu_write32(priv, REG_EFUSE_INDIRECT_CTRL_8710B, NORMAL_REG_READ_OFFSET);
> +
> +       do
> +               val32 = rtl8xxxu_read32(priv, REG_EFUSE_INDIRECT_CTRL_8710B);
> +       while ((val32 & BIT(31)) && (--polling_count > 0));

Without a pair of brace, that looks a little odd to me, but the result should
be correct.

[...]

> +static int rtl8710bu_identify_chip(struct rtl8xxxu_priv *priv)
> +{
> +       struct device *dev = &priv->udev->dev;
> +       u32 cfg0, cfg2, vendor;
> +       u8 package_type = 0x7; /* a nonsense value */
> +       int ret = 0;

No need '=0'. Even, don't need 'ret'. 
Just return rtl8xxxu_config_endpoints_no_sie();

[...]

> +
> +       ret = rtl8xxxu_config_endpoints_no_sie(priv);
> +
> +       return ret;
> +}
> +
> +static void rtl8710b_revise_cck_tx_psf(struct rtl8xxxu_priv *priv, u8 channel)
> +{
> +       if (channel == 13) {
> +               /* Normal values */
> +               rtl8xxxu_write32(priv, REG_CCK0_TX_FILTER2, 0x64B80C1C);
> +               rtl8xxxu_write32(priv, REG_CCK0_DEBUG_PORT, 0x00008810);
> +               rtl8xxxu_write32(priv, 0xaac, 0x01235667);

0xaac is another TX filter, so
#define REG_CCK0_TX_FILTER3		0x0aac

> +               /* Special value for channel 13 */
> +               rtl8xxxu_write32(priv, REG_CCK0_TX_FILTER1, 0xd1d80001);
> +       } else if (channel == 14) {
> +               /* Special values for channel 14 */
> +               rtl8xxxu_write32(priv, REG_CCK0_TX_FILTER2, 0x0000B81C);
> +               rtl8xxxu_write32(priv, REG_CCK0_DEBUG_PORT, 0x00000000);
> +               rtl8xxxu_write32(priv, 0xaac, 0x00003667);
> +               /* Normal value */
> +               rtl8xxxu_write32(priv, REG_CCK0_TX_FILTER1, 0xE82C0001);
> +       } else {
> +               /* Restore normal values from the phy init table */
> +               rtl8xxxu_write32(priv, REG_CCK0_TX_FILTER2, 0x64B80C1C);
> +               rtl8xxxu_write32(priv, REG_CCK0_DEBUG_PORT, 0x00008810);
> +               rtl8xxxu_write32(priv, 0xaac, 0x01235667);
> +               rtl8xxxu_write32(priv, REG_CCK0_TX_FILTER1, 0xE82C0001);
> +       }
> +}
> +

[...]

> +static int rtl8710b_read_efuse(struct rtl8xxxu_priv *priv)
> +{
> +       struct device *dev = &priv->udev->dev;
> +       int i, ret = 0;

move these two downward to 'val32'. Declarations should be in reverse
Xmas tree order.

> +       u8 val8, word_mask, header, extheader;
> +       u16 efuse_addr, offset;
> +       u32 val32;
> +
> +       val32 = rtl8710b_read_syson_reg(priv, REG_SYS_EEPROM_CTRL0_8710B);
> +       priv->boot_eeprom = u32_get_bits(val32, EEPROM_BOOT);
> +       priv->has_eeprom = u32_get_bits(val32, EEPROM_ENABLE);
> +

[...]

> +static int rtl8710bu_active_to_lps(struct rtl8xxxu_priv *priv)
> +{
> +       struct device *dev = &priv->udev->dev;
> +       u8 val8;
> +       u16 val16;
> +       u32 val32;
> +       int retry, retval;
> +
> +       /* Tx Pause */
> +       rtl8xxxu_write8(priv, REG_TXPAUSE, 0xff);
> +
> +       retry = 100;
> +       retval = -EBUSY;
> +       /*
> +        * Poll 32 bit wide REG_SCH_TX_CMD for 0x00000000 to ensure no TX is pending.
> +        */
> +       do {
> +               val32 = rtl8xxxu_read32(priv, REG_SCH_TX_CMD);
> +               if (!val32) {
> +                       retval = 0;
> +                       break;
> +               }
> +               udelay(10);
> +       } while (retry--);
> +
> +       if (!retry) {
> +               dev_warn(dev, "Failed to flush TX queue\n");
> +               retval = -EBUSY;
> +               return retval;
> +       }
> +
> +       /* Disable CCK and OFDM, clock gated */
> +       val8 = rtl8xxxu_read8(priv, 0x02);
> +       val8 &= ~BIT(0);
> +       rtl8xxxu_write8(priv, 0x02, val8);

rtl8xxxu has already defined these:

#define REG_SYS_FUNC			0x0002
#define  SYS_FUNC_BBRSTB		BIT(0)
#define  SYS_FUNC_BB_GLB_RSTN		BIT(1)

> +
> +       udelay(2);
> +
> +       /*Whole BB is reset*/
> +       val8 = rtl8xxxu_read8(priv, 0x02);
> +       val8 &= ~BIT(1);
> +       rtl8xxxu_write8(priv, 0x02, val8);
> +
> +       /* Reset MAC TRX */
> +       val16 = rtl8xxxu_read16(priv, REG_CR);
> +       val16 &= 0xff00;
> +       val16 |= CR_HCI_RXDMA_ENABLE | CR_HCI_TXDMA_ENABLE;
> +       val16 &= ~CR_SECURITY_ENABLE;
> +       rtl8xxxu_write16(priv, REG_CR, val16);
> +
> +       /*Respond TxOK to scheduler*/
> +       val8 = rtl8xxxu_read8(priv, REG_DUAL_TSF_RST);
> +       val8 |= DUAL_TSF_TX_OK;
> +       rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, val8);
> +
> +       return retval;
> +}

[...]

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 620a5cc2bfdd..e223ef852899 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c

[...]

> @@ -1904,41 +1929,54 @@ static int rtl8xxxu_download_firmware(struct rtl8xxxu_priv *priv)
>  {
>         int pages, remainder, i, ret;
>         u8 val8;
> -       u16 val16;
> +       u16 val16, reg_mcu_fw_dl;

nit: define reg_mcu_fw_dl separately 

>         u32 val32;
>         u8 *fwptr;
> 
> -       val8 = rtl8xxxu_read8(priv, REG_SYS_FUNC + 1);
> -       val8 |= 4;
> -       rtl8xxxu_write8(priv, REG_SYS_FUNC + 1, val8);
> +       if (priv->rtl_chip == RTL8710B) {
> +               reg_mcu_fw_dl = REG_8051FW_CTRL_V1_8710B;
> +       } else {
> +               reg_mcu_fw_dl = REG_MCU_FW_DL;
> 

[...]

Only some small suggestions.
Thanks for this nice work. 

Ping-Ke