On 10/03/2023 02:49, Ping-Ke Shih wrote:
>
>
>> -----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))
>
Nice, that's more readable.
>> + 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.
>
Yes, I prefer it without the braces.
>> +
>> + 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.
>
I will initialise it.
>
> 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)
>
Moving "if (reg_e94)" inside the previous "if (candidate >= 0)" should also
fix it, I think.
if (candidate >= 0) {
reg_e94 = result[candidate][0];
reg_e9c = result[candidate][1];
reg_ea4 = result[candidate][2];
reg_eac = result[candidate][3];
dev_dbg(dev, "%s: candidate is %x\n", __func__, candidate);
dev_dbg(dev, "%s: e94=%x e9c=%x ea4=%x eac=%x\n",
__func__, reg_e94, reg_e9c, reg_ea4, reg_eac);
path_a_ok = true;
if (reg_e94)
rtl8xxxu_fill_iqk_matrix_a(priv, path_a_ok, result,
candidate, (reg_ea4 == 0));
}
>
> Ping-Ke
>
>
