| Message ID | 731ea688-04ef-4f02-9d01-3e9026981057@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/4] wifi: rtw88: 8821cu: Fix firmware upload fail | expand |
On Tue, Feb 27, 2024 at 02:18:20PM +0200, Bitterblue Smith wrote: > RTL8822CU, RTL8822BU, and RTL8821CU need an extra register write after > reading and writing certain addresses. > > Without this, the firmware upload fails approximately more than 50% of > the time. Great stuff. I have seen these firmware upload failures as well and couldn't make any sense of it. > > Tested with RTL8811CU (Tenda U9 V2.0) which is the same as RTL8821CU > but without Bluetooth. > > Fixes: a82dfd33d123 ("wifi: rtw88: Add common USB chip support") > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- > drivers/net/wireless/realtek/rtw88/usb.c | 46 ++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c > index 3c4f28c306a9..ff01f7056ca9 100644 > --- a/drivers/net/wireless/realtek/rtw88/usb.c > +++ b/drivers/net/wireless/realtek/rtw88/usb.c > @@ -33,6 +33,42 @@ static void rtw_usb_fill_tx_checksum(struct rtw_usb *rtwusb, > rtw_tx_fill_txdesc_checksum(rtwdev, &pkt_info, skb->data); > } > > +#define REG_ON_SEC 0x00 > +#define REG_OFF_SEC 0x01 > +#define REG_LOCAL_SEC 0x02 > + > +static void rtw_usb_reg_sec(struct rtw_dev *rtwdev, u32 addr, __le32 *data) > +{ > + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); > + struct usb_device *udev = rtwusb->udev; > + u8 current_reg_sec; > + u16 t_reg = 0x4e0; > + u8 t_len = 1; > + int status; > + > + if (addr < 0xFE00) { > + if (addr <= 0xff) > + current_reg_sec = REG_ON_SEC; > + else if (0x1000 <= addr && addr <= 0x10ff) > + current_reg_sec = REG_ON_SEC; > + else > + current_reg_sec = REG_OFF_SEC; > + } else { > + current_reg_sec = REG_LOCAL_SEC; > + } > + > + if (current_reg_sec != REG_ON_SEC) > + return; Is there something we want to do with current_reg_sec == REG_LOCAL_SEC or current_reg_sec == REG_OFF_SEC later? If not the above could be rewritten as: if (addr > 0xff && addr < 0x1000) return; if (addr > 0x10ff) return; ... Sascha
Adding Sean Mollet because I forgot earlier. On 27/02/2024 14:56, Sascha Hauer wrote: > On Tue, Feb 27, 2024 at 02:18:20PM +0200, Bitterblue Smith wrote: >> + if (addr < 0xFE00) { >> + if (addr <= 0xff) >> + current_reg_sec = REG_ON_SEC; >> + else if (0x1000 <= addr && addr <= 0x10ff) >> + current_reg_sec = REG_ON_SEC; >> + else >> + current_reg_sec = REG_OFF_SEC; >> + } else { >> + current_reg_sec = REG_LOCAL_SEC; >> + } >> + >> + if (current_reg_sec != REG_ON_SEC) >> + return; > > Is there something we want to do with current_reg_sec == REG_LOCAL_SEC > or current_reg_sec == REG_OFF_SEC later? If not the above could be > rewritten as: > > if (addr > 0xff && addr < 0x1000) > return; > if (addr > 0x10ff) > return; > > ... Dunno, I just copied the code from the other drivers: https://github.com/morrownr/8821cu-20210916/blob/5b39398e2de146edeb76716420f3288f508bea61/os_dep/linux/usb_ops_linux.c#L171 https://github.com/morrownr/88x2bu-20210702/blob/bb6e514230791010a34daf0d6ccf55ef97309dbf/os_dep/linux/usb_ops_linux.c#L171 https://github.com/thales-dis-dr/rtl8822CU/blob/4182c79e0c5362dcea46088dab9fed27795b5579/os_dep/linux/usb_ops_linux.c#L171
On Tue, Feb 27, 2024 at 06:27:51PM +0200, Bitterblue Smith wrote: > Adding Sean Mollet because I forgot earlier. > > On 27/02/2024 14:56, Sascha Hauer wrote: > > On Tue, Feb 27, 2024 at 02:18:20PM +0200, Bitterblue Smith wrote: > >> + if (addr < 0xFE00) { > >> + if (addr <= 0xff) > >> + current_reg_sec = REG_ON_SEC; > >> + else if (0x1000 <= addr && addr <= 0x10ff) > >> + current_reg_sec = REG_ON_SEC; > >> + else > >> + current_reg_sec = REG_OFF_SEC; > >> + } else { > >> + current_reg_sec = REG_LOCAL_SEC; > >> + } > >> + > >> + if (current_reg_sec != REG_ON_SEC) > >> + return; > > > > Is there something we want to do with current_reg_sec == REG_LOCAL_SEC > > or current_reg_sec == REG_OFF_SEC later? If not the above could be > > rewritten as: > > > > if (addr > 0xff && addr < 0x1000) > > return; > > if (addr > 0x10ff) > > return; > > > > ... > > Dunno, I just copied the code from the other drivers: > > https://github.com/morrownr/8821cu-20210916/blob/5b39398e2de146edeb76716420f3288f508bea61/os_dep/linux/usb_ops_linux.c#L171 Ok, nothing is done with current_reg_sec here as well, so I suggest rewriting the check like I suggested. Sascha
On Tue, Feb 27, 2024 at 02:18:20PM +0200, Bitterblue Smith wrote: > RTL8822CU, RTL8822BU, and RTL8821CU need an extra register write after > reading and writing certain addresses. > > Without this, the firmware upload fails approximately more than 50% of > the time. > > Tested with RTL8811CU (Tenda U9 V2.0) which is the same as RTL8821CU > but without Bluetooth. > > Fixes: a82dfd33d123 ("wifi: rtw88: Add common USB chip support") > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- On a RTL8821CU based chip this indeed makes the firmware upload failures go away. Tested-by: Sascha Hauer <s.hauer@pengutronix.de> Sascha > drivers/net/wireless/realtek/rtw88/usb.c | 46 ++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c > index 3c4f28c306a9..ff01f7056ca9 100644 > --- a/drivers/net/wireless/realtek/rtw88/usb.c > +++ b/drivers/net/wireless/realtek/rtw88/usb.c > @@ -33,6 +33,42 @@ static void rtw_usb_fill_tx_checksum(struct rtw_usb *rtwusb, > rtw_tx_fill_txdesc_checksum(rtwdev, &pkt_info, skb->data); > } > > +#define REG_ON_SEC 0x00 > +#define REG_OFF_SEC 0x01 > +#define REG_LOCAL_SEC 0x02 > + > +static void rtw_usb_reg_sec(struct rtw_dev *rtwdev, u32 addr, __le32 *data) > +{ > + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); > + struct usb_device *udev = rtwusb->udev; > + u8 current_reg_sec; > + u16 t_reg = 0x4e0; > + u8 t_len = 1; > + int status; > + > + if (addr < 0xFE00) { > + if (addr <= 0xff) > + current_reg_sec = REG_ON_SEC; > + else if (0x1000 <= addr && addr <= 0x10ff) > + current_reg_sec = REG_ON_SEC; > + else > + current_reg_sec = REG_OFF_SEC; > + } else { > + current_reg_sec = REG_LOCAL_SEC; > + } > + > + if (current_reg_sec != REG_ON_SEC) > + return; > + > + status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), > + RTW_USB_CMD_REQ, RTW_USB_CMD_WRITE, > + t_reg, 0, data, t_len, 500); > + > + if (status != t_len && status != -ENODEV) > + rtw_err(rtwdev, "%s: reg 0x%x, usb write %u fail, status: %d\n", > + __func__, t_reg, t_len, status); > +} > + > static u32 rtw_usb_read(struct rtw_dev *rtwdev, u32 addr, u16 len) > { > struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); > @@ -58,6 +94,11 @@ static u32 rtw_usb_read(struct rtw_dev *rtwdev, u32 addr, u16 len) > rtw_err(rtwdev, "read register 0x%x failed with %d\n", > addr, ret); > > + if (rtwdev->chip->id == RTW_CHIP_TYPE_8822C || > + rtwdev->chip->id == RTW_CHIP_TYPE_8822B || > + rtwdev->chip->id == RTW_CHIP_TYPE_8821C) > + rtw_usb_reg_sec(rtwdev, addr, data); > + > return le32_to_cpu(*data); > } > > @@ -111,6 +152,11 @@ static void rtw_usb_write(struct rtw_dev *rtwdev, u32 addr, u32 val, int len) > if (ret < 0 && ret != -ENODEV && count++ < 4) > rtw_err(rtwdev, "write register 0x%x failed with %d\n", > addr, ret); > + > + if (rtwdev->chip->id == RTW_CHIP_TYPE_8822C || > + rtwdev->chip->id == RTW_CHIP_TYPE_8822B || > + rtwdev->chip->id == RTW_CHIP_TYPE_8821C) > + rtw_usb_reg_sec(rtwdev, addr, data); > } > > static void rtw_usb_write8(struct rtw_dev *rtwdev, u32 addr, u8 val) > -- > 2.43.2 >
> -----Original Message----- > From: Sascha Hauer <sha@pengutronix.de> > Sent: Wednesday, February 28, 2024 4:56 PM > To: Bitterblue Smith <rtl8821cerfe2@gmail.com> > Cc: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com>; Sean Mollet <sean@malmoset.com> > Subject: Re: [PATCH 1/4] wifi: rtw88: 8821cu: Fix firmware upload fail > > On Tue, Feb 27, 2024 at 06:27:51PM +0200, Bitterblue Smith wrote: > > Adding Sean Mollet because I forgot earlier. > > > > On 27/02/2024 14:56, Sascha Hauer wrote: > > > On Tue, Feb 27, 2024 at 02:18:20PM +0200, Bitterblue Smith wrote: > > >> + if (addr < 0xFE00) { > > >> + if (addr <= 0xff) > > >> + current_reg_sec = REG_ON_SEC; > > >> + else if (0x1000 <= addr && addr <= 0x10ff) > > >> + current_reg_sec = REG_ON_SEC; > > >> + else > > >> + current_reg_sec = REG_OFF_SEC; > > >> + } else { > > >> + current_reg_sec = REG_LOCAL_SEC; > > >> + } > > >> + > > >> + if (current_reg_sec != REG_ON_SEC) > > >> + return; > > > > > > Is there something we want to do with current_reg_sec == REG_LOCAL_SEC > > > or current_reg_sec == REG_OFF_SEC later? If not the above could be > > > rewritten as: > > > > > > if (addr > 0xff && addr < 0x1000) > > > return; > > > if (addr > 0x10ff) > > > return; > > > > > > ... > > > > Dunno, I just copied the code from the other drivers: > > > > > https://github.com/morrownr/8821cu-20210916/blob/5b39398e2de146edeb76716420f3288f508bea61/os_dep/linux > /usb_ops_linux.c#L171 > > Ok, nothing is done with current_reg_sec here as well, so I suggest > rewriting the check like I suggested. I also prefer rewriting the code, but we can add comments to describe there are three sections: 1. on (0x00~0xFF;0x1000~0x10FF): this section is always powered on 2. off (< 0xFE00; but not on section): this section could be powered off 3. local (>= 0xFE00): usb specific registers section Since only on-section needs special deal, maybe positively listing register ranges would be clear, like bool reg_on_sec = false; if ((addr >= 0x00 && addr <= 0xFF) || (addr >= 0x1000 && addr <= 0x10FF)) reg_on_sec = true; if (!reg_on_sec) return; Ping-Ke
> -----Original Message----- > From: Ping-Ke Shih <pkshih@realtek.com> > Sent: Thursday, February 29, 2024 9:37 AM > To: Sascha Hauer <sha@pengutronix.de>; Bitterblue Smith <rtl8821cerfe2@gmail.com> > Cc: linux-wireless@vger.kernel.org; Sean Mollet <sean@malmoset.com> > Subject: RE: [PATCH 1/4] wifi: rtw88: 8821cu: Fix firmware upload fail > > 2. off (< 0xFE00; but not on section): this section could be powered off 2. off (< 0xFE00; exclude on-section): this section could be powered off Correct wording to avoid confusing.
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c index 3c4f28c306a9..ff01f7056ca9 100644 --- a/drivers/net/wireless/realtek/rtw88/usb.c +++ b/drivers/net/wireless/realtek/rtw88/usb.c @@ -33,6 +33,42 @@ static void rtw_usb_fill_tx_checksum(struct rtw_usb *rtwusb, rtw_tx_fill_txdesc_checksum(rtwdev, &pkt_info, skb->data); } +#define REG_ON_SEC 0x00 +#define REG_OFF_SEC 0x01 +#define REG_LOCAL_SEC 0x02 + +static void rtw_usb_reg_sec(struct rtw_dev *rtwdev, u32 addr, __le32 *data) +{ + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); + struct usb_device *udev = rtwusb->udev; + u8 current_reg_sec; + u16 t_reg = 0x4e0; + u8 t_len = 1; + int status; + + if (addr < 0xFE00) { + if (addr <= 0xff) + current_reg_sec = REG_ON_SEC; + else if (0x1000 <= addr && addr <= 0x10ff) + current_reg_sec = REG_ON_SEC; + else + current_reg_sec = REG_OFF_SEC; + } else { + current_reg_sec = REG_LOCAL_SEC; + } + + if (current_reg_sec != REG_ON_SEC) + return; + + status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), + RTW_USB_CMD_REQ, RTW_USB_CMD_WRITE, + t_reg, 0, data, t_len, 500); + + if (status != t_len && status != -ENODEV) + rtw_err(rtwdev, "%s: reg 0x%x, usb write %u fail, status: %d\n", + __func__, t_reg, t_len, status); +} + static u32 rtw_usb_read(struct rtw_dev *rtwdev, u32 addr, u16 len) { struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); @@ -58,6 +94,11 @@ static u32 rtw_usb_read(struct rtw_dev *rtwdev, u32 addr, u16 len) rtw_err(rtwdev, "read register 0x%x failed with %d\n", addr, ret); + if (rtwdev->chip->id == RTW_CHIP_TYPE_8822C || + rtwdev->chip->id == RTW_CHIP_TYPE_8822B || + rtwdev->chip->id == RTW_CHIP_TYPE_8821C) + rtw_usb_reg_sec(rtwdev, addr, data); + return le32_to_cpu(*data); } @@ -111,6 +152,11 @@ static void rtw_usb_write(struct rtw_dev *rtwdev, u32 addr, u32 val, int len) if (ret < 0 && ret != -ENODEV && count++ < 4) rtw_err(rtwdev, "write register 0x%x failed with %d\n", addr, ret); + + if (rtwdev->chip->id == RTW_CHIP_TYPE_8822C || + rtwdev->chip->id == RTW_CHIP_TYPE_8822B || + rtwdev->chip->id == RTW_CHIP_TYPE_8821C) + rtw_usb_reg_sec(rtwdev, addr, data); } static void rtw_usb_write8(struct rtw_dev *rtwdev, u32 addr, u8 val)
RTL8822CU, RTL8822BU, and RTL8821CU need an extra register write after reading and writing certain addresses. Without this, the firmware upload fails approximately more than 50% of the time. Tested with RTL8811CU (Tenda U9 V2.0) which is the same as RTL8821CU but without Bluetooth. Fixes: a82dfd33d123 ("wifi: rtw88: Add common USB chip support") Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> --- drivers/net/wireless/realtek/rtw88/usb.c | 46 ++++++++++++++++++++++++ 1 file changed, 46 insertions(+)