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

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

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Thursday, March 9, 2023 5:28 AM
> To: linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>; Jiajie Chen <c@jia.je>
> Subject: [PATCH v2] 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.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
> v2:
>  - Suggestions from Ping-Ke Shih:
>    - Add comma after the last member of enum rtl8xxxu_rtl_chip.
>    - Add comment about struct mutex syson_indirect_access_mutex.
>    - Declare variables in reverse Christmas tree order in
>      rtl8710b_read_efuse().
>    - Remove unnecessary variable ret from rtl8710bu_identify_chip().
>    - Add definition for register 0xaac.
>    - Use the existing macros REG_SYS_FUNC, SYS_FUNC_BBRSTB, and
>      SYS_FUNC_BB_GLB_RSTN instead of magic numbers in
>      rtl8710bu_active_to_lps().
>    - Declare reg_mcu_fw_dl separately in rtl8xxxu_download_firmware().
>  - Add spaces after /* and before */ in some comments.
>  - Rearrange the declarations in rtl8710b_read_efuse8() as well.
>  - Load the right firmware based on the chip manufacturer (UMC/SMIC).
>  - Use the mask 0xc0 instead of 0xf0 to detect the chip manufacturer in
>    rtl8710bu_identify_chip(). There was an extra shift in the vendor
>    driver which I missed.
>  - Make the vid and pid fields of struct rtl8710bu_efuse two bytes
>    each, and the filler field res7 one byte shorter.
> 
>  - I was lazy and didn't do some things the right way in v1. I thought
>    surely there are no more chips to support. But since then I
>    discovered that the RTL8192FU can be bought from Aliexpress for
>    6.66 $. :) It will need the same PHY status parsing as the RTL8710BU,
>    which is why there are these extra changes:
>    - Initialise priv->cck_new_agc in rtl8xxxu_init_device() always,
>      regardless of the chip family.
>    - Pass the PHY status structs to the CCK RSSI functions.
>    - Move the "old AGC" CCK RSSI calculation from
>      rtl8710bu_rx_parse_phystats_type0() to a new function
>      rtl8710b_cck_rssi().
>    - Rename the functions rtl8710bu_rx_parse_phystats* to
>      jaguar2_rx_parse_phystats* and move them to rtl8xxxu_core.c.
>    - Modify the functions jaguar2_rx_parse_phystats_type{1,2} to handle
>      2T2R chips as well.
> ---

[...]

> +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) {

if (!IS_ALIGNED(addr, 4))

> +               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));

Add brace is allowed for this case. Not sure if you prefer this, or miss 
comment before.

> +
> +       if (polling_count == 0)
> +               dev_warn(dev, "%s: Failed to read from 0x%x, 0x806c = 0x%x\n",
> +                        __func__, addr, val32);
> +       else
> +               value = rtl8xxxu_read32(priv, REG_USB_HOST_INDIRECT_DATA_8710B);
> +
> +       mutex_unlock(&priv->syson_indirect_access_mutex);
> +
> +       if (rtl8xxxu_debug & RTL8XXXU_DEBUG_REG_READ)
> +               dev_info(dev, "%s(%04x) = 0x%08x\n", __func__, addr, value);
> +
> +       return value;
> +}
> +
> +static void rtl8710b_indirect_write32(struct rtl8xxxu_priv *priv, u32 addr, u32 val)
> +{
> +       struct device *dev = &priv->udev->dev;
> +       u8 polling_count = 0xff;
> +       u32 val32;
> +
> +       if (addr & 3) {

if (!IS_ALIGNED(addr, 4))

> +               dev_warn(dev, "%s: Aborting because 0x%x is not a multiple of 4.\n",
> +                        __func__, addr);
> +               return;
> +       }
> +
> +       mutex_lock(&priv->syson_indirect_access_mutex);
> +
> +       rtl8xxxu_write32(priv, REG_USB_HOST_INDIRECT_ADDR_8710B, addr);
> +       rtl8xxxu_write32(priv, REG_USB_HOST_INDIRECT_DATA_8710B, val);
> +       rtl8xxxu_write32(priv, REG_EFUSE_INDIRECT_CTRL_8710B, NORMAL_REG_WRITE_OFFSET);
> +
> +       do
> +               val32 = rtl8xxxu_read32(priv, REG_EFUSE_INDIRECT_CTRL_8710B);
> +       while ((val32 & BIT(31)) && (--polling_count > 0));
> +
> +       if (polling_count == 0)
> +               dev_warn(dev, "%s: Failed to write 0x%x to 0x%x, 0x806c = 0x%x\n",
> +                        __func__, val, addr, val32);
> +
> +       mutex_unlock(&priv->syson_indirect_access_mutex);
> +
> +       if (rtl8xxxu_debug & RTL8XXXU_DEBUG_REG_WRITE)
> +               dev_info(dev, "%s(%04x) = 0x%08x\n", __func__, addr, val);
> +}

[...]

Only two minor comments, and v2 looks good to me. So, I run sparse and smatch
to check this patch, and it reports two warnings:

1. drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8710b.c:742 rtl8710bu_config_channel() error: uninitialized symbol 'sec_ch_above'.

This looks like a false-alarm, because 'sec_ch_above' must be set if 'ht40' is true.
But, this should reference back much to know this. 
Maybe, we can set 'sec_ch_above = 0' initially. 


2. drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8710b.c:1487 rtl8710bu_phy_iq_calibrate() error: uninitialized symbol 'reg_e94'.

This could be a false-alarm too. 'reg_e94' must be set if 'candidate >= 0', but
original statement causes smatch hard to determine:

   if (reg_e94 && candidate >= 0)

swap the expressions to fix the warning:

  if (candidate >= 0 && reg_e94)


Ping-Ke