RE: [PATCH rtw-next v3] wifi: rtw89: usb: Support switching to USB 3 mode

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

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> The Realtek wifi 6/7 devices which support USB 3 are weird: when first
> plugged in, they pretend to be USB 2. The driver needs to send some
> commands to the device, which make it disappear and come back as a
> USB 3 device.
> 
> Implement the required commands in rtw89.
> 
> Add a new function rtw89_usb_write32_quiet() to avoid the warnings
> when writing to R_{AX,BE}_PAD_CTRL2. Even though the write succeeds,
> usb_control_msg() returns -EPROTO, probably because the USB device
> disappears immediately. This results in some confusing warnings in
> the kernel log.
> 
> When a USB 3 device is plugged into a USB 2 port, rtw89 will try to
> switch it to USB 3 mode only once. The device will disappear and come
> back still in USB 2 mode, of course.

As we always try to switch USB 3, is it needed to add a hint to users
to plug USB 2 port if he has a bad performance on 2GHz band?

> 
> Tested with RTL8832AU, RTL8832BU, RTL8832CU, and RTL8912AU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Some minor suggestions. Otherwise, 

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index c6d55e669776..bfe004a49ccc 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -12,7 +12,7 @@
>  static void rtw89_usb_read_port_complete(struct urb *urb);
> 
>  static void rtw89_usb_vendorreq(struct rtw89_dev *rtwdev, u32 addr,
> -                               void *data, u16 len, u8 reqtype)
> +                               void *data, u16 len, u8 reqtype, bool warn)
>  {
>         struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
>         struct usb_device *udev = rtwusb->udev;
> @@ -52,7 +52,7 @@ static void rtw89_usb_vendorreq(struct rtw89_dev *rtwdev, u32 addr,
> 
>                 if (ret == -ESHUTDOWN || ret == -ENODEV)
>                         set_bit(RTW89_FLAG_UNPLUGGED, rtwdev->flags);
> -               else if (ret < 0)
> +               else if (ret < 0 && warn)
>                         rtw89_warn(rtwdev,
>                                    "usb %s%u 0x%x fail ret=%d value=0x%x attempt=%d\n",
>                                    str_read_write(reqtype == RTW89_USB_VENQT_READ),
> @@ -80,7 +80,7 @@ static u32 rtw89_usb_read_cmac(struct rtw89_dev *rtwdev, u32 addr)
> 
>         for (count = 0; ; count++) {
>                 rtw89_usb_vendorreq(rtwdev, addr32, &data, 4,
> -                                   RTW89_USB_VENQT_READ);
> +                                   RTW89_USB_VENQT_READ, true);

Not sure if you like to implement a __rtw89_usb_vendorreq(), and 
preserve rtw89_usb_vendorreq() as a wrapper with original prototype.
Then, no need to stir here. 

I mean:

__rtw89_usb_vendorreq(..., bool warn);

static void rtw89_usb_vendorreq(struct rtw89_dev *rtwdev, u32 addr,
				void *data, u16 len, u8 reqtype)
{
	__rtw89_usb_vendorreq(rtwdev, addr, data, len, reqtype, true);
}

(just a soft suggestion)

> 
>                 val32 = le32_to_cpu(data);
>                 if (val32 != RTW89_R32_DEAD)

[...]