> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
> Sent: Thursday, March 9, 2023 5:28 AM
> To: linux-wireless@xxxxxxxxxxxxxxx
> Cc: Jes Sorensen <Jes.Sorensen@xxxxxxxxx>; Ping-Ke Shih <pkshih@xxxxxxxxxxx>; Jiajie Chen <c@xxxxxx>
> 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@xxxxxxxxx>
> ---
> 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
