| Message ID | 20190627095247.8792-1-chiu@endlessm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU | expand |
On Thu, Jun 27, 2019 at 5:52 PM Chris Chiu <chiu@endlessm.com> wrote: > > The WiFi tx power of RTL8723BU is extremely low after booting. So > the WiFi scan gives very limited AP list and it always fails to > connect to the selected AP. This module only supports 1x1 antenna > and the antenna is switched to bluetooth due to some incorrect > register settings. > > This commit hand over the antenna control to PTA, the wifi signal > will be back to normal and the bluetooth scan can also work at the > same time. However, the btcoexist still needs to be handled under > different circumstances. If there's a BT connection established, > the wifi still fails to connect until disconneting the BT. > > Signed-off-by: Chris Chiu <chiu@endlessm.com> > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++++++--- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > index 3adb1d3d47ac..6c3c70d93ac1 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > /* > * WLAN action by PTA > */ > - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); > + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); > > /* > * BT select S0/S1 controlled by WiFi > @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv)); > > /* > - * 0x280, 0x00, 0x200, 0x80 - not clear > + * Different settings per different antenna position. > + * Antenna switch to BT: 0x280, 0x00 (inverse) > + * Antenna switch to WiFi: 0x0, 0x280 (inverse) > + * Antenna controlled by PTA: 0x200, 0x80 (inverse) > */ > - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); > + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); > > /* > * Software control, antenna at WiFi side > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 8136e268b4e6..87b2179a769e 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) > > /* Check if MAC is already powered on */ > val8 = rtl8xxxu_read8(priv, REG_CR); > + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); > > /* > * Fix 92DU-VC S3 hang with the reason is that secondary mac is not > * initialized. First MAC returns 0xea, second MAC returns 0x00 > */ > - if (val8 == 0xea) > + if (val8 == 0xea || !(val16 & BIT(11))) > macpower = false; > else > macpower = true; > -- > 2.11.0 > Gentle ping. Cheers. Chris
Hi Chris, On Thu, Jun 27, 2019 at 5:53 PM Chris Chiu <chiu@endlessm.com> wrote: > The WiFi tx power of RTL8723BU is extremely low after booting. So > the WiFi scan gives very limited AP list and it always fails to > connect to the selected AP. This module only supports 1x1 antenna > and the antenna is switched to bluetooth due to some incorrect > register settings. > > This commit hand over the antenna control to PTA, the wifi signal > will be back to normal and the bluetooth scan can also work at the > same time. However, the btcoexist still needs to be handled under > different circumstances. If there's a BT connection established, > the wifi still fails to connect until disconneting the BT. > > Signed-off-by: Chris Chiu <chiu@endlessm.com> Really nice work finding this! I know that after this change, you plan to bring over the btcoexist code from the vendor driver (or at least the minimum required code) for a more complete fix, but I'm curious how you found these magic register values and how they compare to the values used by the vendor driver with btcoexist? What's PTA? A type of firmware-implemented btcoexist that works for scanning but doesn't work when a BT connection is actually established? > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > index 3adb1d3d47ac..6c3c70d93ac1 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > /* > * WLAN action by PTA > */ > - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); > + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); The comment above this still says "WLAN action by PTA" and the vendor driver has: //set wlan_act control by PTA pBtCoexist->fBtcWrite1Byte(pBtCoexist, 0x76e, 0x4); but then also: //set wlan_act control by PTA pBtCoexist->fBtcWrite1Byte(pBtCoexist, 0x76e, 0xc); So this change seems to be at least consistent with ambiguity of the vendor driver, do you have any understanding of the extra bit that is now set here? It's not easy to follow the code flow of the vendor driver to see what actually happens, have you checked that, does it end up using the 0xc value? > - * 0x280, 0x00, 0x200, 0x80 - not clear > + * Different settings per different antenna position. > + * Antenna switch to BT: 0x280, 0x00 (inverse) > + * Antenna switch to WiFi: 0x0, 0x280 (inverse) > + * Antenna controlled by PTA: 0x200, 0x80 (inverse) > */ > - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); > + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); I don't quite follow the comment here. Why are there 2 values listed for each possibility, what do you mean by inverse? You say the register settings were incorrect, but the previous value was 0x00 which you now document as "antenna switch to wifi" which sounds like it was already correct? Which value does the vendor driver use? > /* > * Software control, antenna at WiFi side > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 8136e268b4e6..87b2179a769e 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) > > /* Check if MAC is already powered on */ > val8 = rtl8xxxu_read8(priv, REG_CR); > + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); > > /* > * Fix 92DU-VC S3 hang with the reason is that secondary mac is not > * initialized. First MAC returns 0xea, second MAC returns 0x00 > */ > - if (val8 == 0xea) > + if (val8 == 0xea || !(val16 & BIT(11))) > macpower = false; > else > macpower = true; At a glance I can't see which code this corresponds to in the vendor driver, can you point that out? Thanks Daniel
On Mon, Jul 1, 2019 at 4:28 PM Daniel Drake <drake@endlessm.com> wrote: > > Hi Chris, > > On Thu, Jun 27, 2019 at 5:53 PM Chris Chiu <chiu@endlessm.com> wrote: > > The WiFi tx power of RTL8723BU is extremely low after booting. So > > the WiFi scan gives very limited AP list and it always fails to > > connect to the selected AP. This module only supports 1x1 antenna > > and the antenna is switched to bluetooth due to some incorrect > > register settings. > > > > This commit hand over the antenna control to PTA, the wifi signal > > will be back to normal and the bluetooth scan can also work at the > > same time. However, the btcoexist still needs to be handled under > > different circumstances. If there's a BT connection established, > > the wifi still fails to connect until disconneting the BT. > > > > Signed-off-by: Chris Chiu <chiu@endlessm.com> > > Really nice work finding this! > > I know that after this change, you plan to bring over the btcoexist > code from the vendor driver (or at least the minimum required code) > for a more complete fix, but I'm curious how you found these magic > register values and how they compare to the values used by the vendor > driver with btcoexist? > > What's PTA? A type of firmware-implemented btcoexist that works for > scanning but doesn't work when a BT connection is actually > established? > When the vendor driver invokes rtw_btcoex_HAL_Initialize, which will then call halbtc8723b1ant_SetAntPath to configure the registers in this patch. From the code, the registers will have different register settings per the antenna position and the phase. If the driver is in the InitHwConfig phase, the register value is identical to what rtl8xxxu driver does in enable_rf(). However, the vendor driver will do halbtc8723b1ant_PsTdma() twice by halbtc8723b1ant_ActionWifiNotConnected() with the type argument 8 for PTA control about 200ms after InitHwConfig. The _ActionWifiNotConnected is invoked by the BTCOEXIST. I keep seeing the halbtc8723b1ant_PsTdma with type 8 been called every 2 seconds. I don't know what PTA is. I presume it's the mechanism in FW for the automatic antenna selecting instead of manual control. Given the phenomenon that wifi signal still stays low even without bluetooth driver loaded, I believe setting the registers as in halbtc8723b1ant_SetAntPath with BTC_ANT_PATH_PTA also makes sense. > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > > index 3adb1d3d47ac..6c3c70d93ac1 100644 > > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > > @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > > /* > > * WLAN action by PTA > > */ > > - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); > > + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); > > The comment above this still says "WLAN action by PTA" and the vendor > driver has: > //set wlan_act control by PTA > pBtCoexist->fBtcWrite1Byte(pBtCoexist, 0x76e, 0x4); > > but then also: > //set wlan_act control by PTA > pBtCoexist->fBtcWrite1Byte(pBtCoexist, 0x76e, 0xc); > > So this change seems to be at least consistent with ambiguity of the > vendor driver, do you have any understanding of the extra bit that is > now set here? > I think the precise expression for 0x04 is "set wlan act to always low", it's configured for wifi only. > It's not easy to follow the code flow of the vendor driver to see what > actually happens, have you checked that, does it end up using the 0xc > value? > Yes, it ends up with 0x0c not matter what antenna position type is. Unless it's configured wifi only. > > - * 0x280, 0x00, 0x200, 0x80 - not clear > > + * Different settings per different antenna position. > > + * Antenna switch to BT: 0x280, 0x00 (inverse) > > + * Antenna switch to WiFi: 0x0, 0x280 (inverse) > > + * Antenna controlled by PTA: 0x200, 0x80 (inverse) > > */ > > - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); > > + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); > > I don't quite follow the comment here. Why are there 2 values listed > for each possibility, what do you mean by inverse? You say the > register settings were incorrect, but the previous value was 0x00 > which you now document as "antenna switch to wifi" which sounds like > it was already correct? > > Which value does the vendor driver use? > The first column means the value for normal antenna installation, wifi on the main port. The second column is the value for inverse antenna installation. So if I want to manually switch the antenna for BT use, and the antenna installation is inverse, I need to set to 0x280. So 0x80 means I want to switch to PTA and the antenna installation in inverse. The vendor driver's code about this is also in halbtc8723b1ant_SetAntPath. > > /* > > * Software control, antenna at WiFi side > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > index 8136e268b4e6..87b2179a769e 100644 > > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) > > > > /* Check if MAC is already powered on */ > > val8 = rtl8xxxu_read8(priv, REG_CR); > > + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); > > > > /* > > * Fix 92DU-VC S3 hang with the reason is that secondary mac is not > > * initialized. First MAC returns 0xea, second MAC returns 0x00 > > */ > > - if (val8 == 0xea) > > + if (val8 == 0xea || !(val16 & BIT(11))) > > macpower = false; > > else > > macpower = true; > > At a glance I can't see which code this corresponds to in the vendor > driver, can you point that out? > > Thanks > Daniel It's in rtl8723bu_hal_init and the comment says "Check if MAC has already power on". In vendor driver, it's just for output messages but in rtl8xxxu, it will determine whether if the llt_init() and tx related registers being correctly initialized. I sometimes hit the problem of connection failure after boot and it's because the macpower is falsely true. Chris
On 7/1/19 4:27 AM, Daniel Drake wrote: > Hi Chris, > > On Thu, Jun 27, 2019 at 5:53 PM Chris Chiu <chiu@endlessm.com> wrote: >> The WiFi tx power of RTL8723BU is extremely low after booting. So >> the WiFi scan gives very limited AP list and it always fails to >> connect to the selected AP. This module only supports 1x1 antenna >> and the antenna is switched to bluetooth due to some incorrect >> register settings. >> >> This commit hand over the antenna control to PTA, the wifi signal >> will be back to normal and the bluetooth scan can also work at the >> same time. However, the btcoexist still needs to be handled under >> different circumstances. If there's a BT connection established, >> the wifi still fails to connect until disconneting the BT. >> >> Signed-off-by: Chris Chiu <chiu@endlessm.com> > > Really nice work finding this! > > I know that after this change, you plan to bring over the btcoexist > code from the vendor driver (or at least the minimum required code) > for a more complete fix, but I'm curious how you found these magic > register values and how they compare to the values used by the vendor > driver with btcoexist? We definitely don't want to bring over the vendor code, since it's a pile of spaghetti, but we probably need to get something sorted. This went down the drain when the bluetooth driver was added without taking it into account - long after this driver was merged. Cheers, Jes
On 6/27/19 5:52 AM, Chris Chiu wrote: > The WiFi tx power of RTL8723BU is extremely low after booting. So > the WiFi scan gives very limited AP list and it always fails to > connect to the selected AP. This module only supports 1x1 antenna > and the antenna is switched to bluetooth due to some incorrect > register settings. > > This commit hand over the antenna control to PTA, the wifi signal > will be back to normal and the bluetooth scan can also work at the > same time. However, the btcoexist still needs to be handled under > different circumstances. If there's a BT connection established, > the wifi still fails to connect until disconneting the BT. > > Signed-off-by: Chris Chiu <chiu@endlessm.com> > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++++++--- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > index 3adb1d3d47ac..6c3c70d93ac1 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > /* > * WLAN action by PTA > */ > - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); > + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); > > /* > * BT select S0/S1 controlled by WiFi > @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv)); > > /* > - * 0x280, 0x00, 0x200, 0x80 - not clear > + * Different settings per different antenna position. > + * Antenna switch to BT: 0x280, 0x00 (inverse) > + * Antenna switch to WiFi: 0x0, 0x280 (inverse) > + * Antenna controlled by PTA: 0x200, 0x80 (inverse) > */ > - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); > + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); > > /* > * Software control, antenna at WiFi side > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 8136e268b4e6..87b2179a769e 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) > > /* Check if MAC is already powered on */ > val8 = rtl8xxxu_read8(priv, REG_CR); > + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); > > /* > * Fix 92DU-VC S3 hang with the reason is that secondary mac is not > * initialized. First MAC returns 0xea, second MAC returns 0x00 > */ > - if (val8 == 0xea) > + if (val8 == 0xea || !(val16 & BIT(11))) > macpower = false; > else > macpower = true; This part I would like to ask you take a good look at the other chips to make sure you don't break support for 8192cu, 8723au, 8188eu with this. Cheers, Jes
On Tue, Jul 2, 2019 at 8:44 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: > > On 6/27/19 5:52 AM, Chris Chiu wrote: > > The WiFi tx power of RTL8723BU is extremely low after booting. So > > the WiFi scan gives very limited AP list and it always fails to > > connect to the selected AP. This module only supports 1x1 antenna > > and the antenna is switched to bluetooth due to some incorrect > > register settings. > > > > This commit hand over the antenna control to PTA, the wifi signal > > will be back to normal and the bluetooth scan can also work at the > > same time. However, the btcoexist still needs to be handled under > > different circumstances. If there's a BT connection established, > > the wifi still fails to connect until disconneting the BT. > > > > Signed-off-by: Chris Chiu <chiu@endlessm.com> > > --- > > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++++++--- > > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > > index 3adb1d3d47ac..6c3c70d93ac1 100644 > > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c > > @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > > /* > > * WLAN action by PTA > > */ > > - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); > > + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); > > > > /* > > * BT select S0/S1 controlled by WiFi > > @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) > > rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv)); > > > > /* > > - * 0x280, 0x00, 0x200, 0x80 - not clear > > + * Different settings per different antenna position. > > + * Antenna switch to BT: 0x280, 0x00 (inverse) > > + * Antenna switch to WiFi: 0x0, 0x280 (inverse) > > + * Antenna controlled by PTA: 0x200, 0x80 (inverse) > > */ > > - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); > > + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); > > > > /* > > * Software control, antenna at WiFi side > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > index 8136e268b4e6..87b2179a769e 100644 > > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) > > > > /* Check if MAC is already powered on */ > > val8 = rtl8xxxu_read8(priv, REG_CR); > > + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); > > > > /* > > * Fix 92DU-VC S3 hang with the reason is that secondary mac is not > > * initialized. First MAC returns 0xea, second MAC returns 0x00 > > */ > > - if (val8 == 0xea) > > + if (val8 == 0xea || !(val16 & BIT(11))) > > macpower = false; > > else > > macpower = true; > > This part I would like to ask you take a good look at the other chips to > make sure you don't break support for 8192cu, 8723au, 8188eu with this. > > Cheers, > Jes I checked the vendor code of 8192cu and 8188eu, they don't have this part of code to check the REG_CR before power on sequence. I can only find similar code in rtl8723be. if (tmp_u1b != 0 && tmp_u1b !=0xea) rtlhal->mac_func_enable = true; By definition, the BIT(11) of REG_SYS_CLKR in rtl8xxxu_regs.h is SYS_CLK_MAC_CLK_ENABLE. It seems to make sense to check this value for macpower no matter what chip it is. I think I can make it more self-expressive as down below. if (val8 == 0xea || !(val16 & SYS_CLK_MAC_CLK_ENABLE)) And per the comment, this code is for 92DU-VC S3 hang problem and I think an OR check for SYS_CLK_MAC_CLK_ENABLE is still safe for this. Chris
On Tue, Jul 2, 2019 at 8:42 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: > We definitely don't want to bring over the vendor code, since it's a > pile of spaghetti, but we probably need to get something sorted. This > went down the drain when the bluetooth driver was added without taking > it into account - long after this driver was merged. Yeah, I didn't mean bring over quite so literally.. Chris is studying it and figuring out the neatest way to reimplement the required bits. As for the relationship with bluetooth.. actually the bug that Chris is working on here is that the rtl8xxxu wifi signal is totally unusable *until* the bluetooth driver is loaded. Once the bluetooth driver is loaded, at the point of bluetooth firmware upload, the rtl8xxxu signal magiaclly strength becomes good. I think this is consistent with other rtl8xxxu problem reports that we saw lying around, although they had not been diagnosed in so much detail. The rtl8723bu vendor driver does not suffer this problem, it works fine with or without the bluetooth driver in place. Daniel
On Tue, Jul 2, 2019 at 4:01 PM Chris Chiu <chiu@endlessm.com> wrote: > When the vendor driver invokes rtw_btcoex_HAL_Initialize, which will then > call halbtc8723b1ant_SetAntPath to configure the registers in this patch. > From the code, the registers will have different register settings per the > antenna position and the phase. If the driver is in the InitHwConfig phase, > the register value is identical to what rtl8xxxu driver does in enable_rf(). > However, the vendor driver will do halbtc8723b1ant_PsTdma() twice by > halbtc8723b1ant_ActionWifiNotConnected() with the type argument 8 for > PTA control about 200ms after InitHwConfig. The _ActionWifiNotConnected > is invoked by the BTCOEXIST. I keep seeing the halbtc8723b1ant_PsTdma > with type 8 been called every 2 seconds. I see. So this is a measured step towards consistency with the vendor driver. Maybe you can mention these details in the commit message. > Yes, it ends up with 0x0c not matter what antenna position type is. Unless > it's configured wifi only. Also worth mentioning in the commit message then, that the 0xc ACT_CONTROL value is effectively what the working vendor driver uses. > > > - * 0x280, 0x00, 0x200, 0x80 - not clear > > > + * Different settings per different antenna position. > > > + * Antenna switch to BT: 0x280, 0x00 (inverse) > > > + * Antenna switch to WiFi: 0x0, 0x280 (inverse) > > > + * Antenna controlled by PTA: 0x200, 0x80 (inverse) > > > */ > > > - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); > > > + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); > > > > I don't quite follow the comment here. Why are there 2 values listed > > for each possibility, what do you mean by inverse? You say the > > register settings were incorrect, but the previous value was 0x00 > > which you now document as "antenna switch to wifi" which sounds like > > it was already correct? > > > > Which value does the vendor driver use? > > > The first column means the value for normal antenna installation, wifi > on the main port. The second column is the value for inverse antenna > installation. So if I want to manually switch the antenna for BT use, > and the antenna installation is inverse, I need to set to 0x280. So 0x80 > means I want to switch to PTA and the antenna installation in inverse. Still not quite clear what you mean by "inverse" here, but maybe I just don't know anything about antennas. Is it that an antenna connector has two pins and this one swaps the meaning of each pin? Does the new value of 0x80 reflect what the vendor driver does in practice? Thanks Daniel
On 7/3/19 3:42 AM, Daniel Drake wrote: > On Tue, Jul 2, 2019 at 8:42 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: >> We definitely don't want to bring over the vendor code, since it's a >> pile of spaghetti, but we probably need to get something sorted. This >> went down the drain when the bluetooth driver was added without taking >> it into account - long after this driver was merged. > > Yeah, I didn't mean bring over quite so literally.. Chris is studying > it and figuring out the neatest way to reimplement the required bits. > > As for the relationship with bluetooth.. actually the bug that Chris > is working on here is that the rtl8xxxu wifi signal is totally > unusable *until* the bluetooth driver is loaded. So this is not my experience at all from when I wrote the code. The 8723bu dongle I used for it came up just fine. > Once the bluetooth driver is loaded, at the point of bluetooth > firmware upload, the rtl8xxxu signal magiaclly strength becomes good. > I think this is consistent with other rtl8xxxu problem reports that we > saw lying around, although they had not been diagnosed in so much > detail. See this is the very opposite of what I have experienced. The bluetooth driver ruins the signal when it's loaded with my dongle. > The rtl8723bu vendor driver does not suffer this problem, it works > fine with or without the bluetooth driver in place. My point is this seems to be very dongle dependent :( We have to be careful not breaking it for some users while fixing it for others. Cheers, Jes
On 7/2/19 11:25 PM, Chris Chiu wrote: > On Tue, Jul 2, 2019 at 8:44 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: >> >> On 6/27/19 5:52 AM, Chris Chiu wrote: >>> The WiFi tx power of RTL8723BU is extremely low after booting. So >>> the WiFi scan gives very limited AP list and it always fails to >>> connect to the selected AP. This module only supports 1x1 antenna >>> and the antenna is switched to bluetooth due to some incorrect >>> register settings. >>> >>> This commit hand over the antenna control to PTA, the wifi signal >>> will be back to normal and the bluetooth scan can also work at the >>> same time. However, the btcoexist still needs to be handled under >>> different circumstances. If there's a BT connection established, >>> the wifi still fails to connect until disconneting the BT. >>> >>> Signed-off-by: Chris Chiu <chiu@endlessm.com> >>> --- >>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++++++--- >>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- >>> 2 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c >>> index 3adb1d3d47ac..6c3c70d93ac1 100644 >>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c >>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c >>> @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) >>> /* >>> * WLAN action by PTA >>> */ >>> - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); >>> + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); >>> >>> /* >>> * BT select S0/S1 controlled by WiFi >>> @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) >>> rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv)); >>> >>> /* >>> - * 0x280, 0x00, 0x200, 0x80 - not clear >>> + * Different settings per different antenna position. >>> + * Antenna switch to BT: 0x280, 0x00 (inverse) >>> + * Antenna switch to WiFi: 0x0, 0x280 (inverse) >>> + * Antenna controlled by PTA: 0x200, 0x80 (inverse) >>> */ >>> - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); >>> + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); >>> >>> /* >>> * Software control, antenna at WiFi side >>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>> index 8136e268b4e6..87b2179a769e 100644 >>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>> @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) >>> >>> /* Check if MAC is already powered on */ >>> val8 = rtl8xxxu_read8(priv, REG_CR); >>> + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); >>> >>> /* >>> * Fix 92DU-VC S3 hang with the reason is that secondary mac is not >>> * initialized. First MAC returns 0xea, second MAC returns 0x00 >>> */ >>> - if (val8 == 0xea) >>> + if (val8 == 0xea || !(val16 & BIT(11))) >>> macpower = false; >>> else >>> macpower = true; >> >> This part I would like to ask you take a good look at the other chips to >> make sure you don't break support for 8192cu, 8723au, 8188eu with this. >> >> Cheers, >> Jes > > I checked the vendor code of 8192cu and 8188eu, they don't have this part > of code to check the REG_CR before power on sequence. I can only find > similar code in rtl8723be. > if (tmp_u1b != 0 && tmp_u1b !=0xea) > rtlhal->mac_func_enable = true; > > By definition, the BIT(11) of REG_SYS_CLKR in rtl8xxxu_regs.h is > SYS_CLK_MAC_CLK_ENABLE. It seems to make sense to check this value > for macpower no matter what chip it is. I think I can make it more > self-expressive > as down below. > > if (val8 == 0xea || !(val16 & SYS_CLK_MAC_CLK_ENABLE)) Yes, please always use the descriptive defines rather than hard coding the bit numbers. > And per the comment, this code is for 92DU-VC S3 hang problem and I think an > OR check for SYS_CLK_MAC_CLK_ENABLE is still safe for this. Sounds reasonable - keep in mind that some of these bugs may have been fixed for one chip, and then just copied forward. Cheers, Jes
On Wed, Jul 3, 2019 at 8:59 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: > My point is this seems to be very dongle dependent :( We have to be > careful not breaking it for some users while fixing it for others. Do you still have your device? Once we get to the point when you are happy with Chris's two patches here on a code review level, we'll reach out to other driver contributors plus people who previously complained about these types of problems, and see if we can get some wider testing. Larry, do you have these devices, can you help with testing too? Thanks Daniel
On 7/4/19 11:44 PM, Daniel Drake wrote: > On Wed, Jul 3, 2019 at 8:59 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: >> My point is this seems to be very dongle dependent :( We have to be >> careful not breaking it for some users while fixing it for others. > > Do you still have your device? > > Once we get to the point when you are happy with Chris's two patches > here on a code review level, we'll reach out to other driver > contributors plus people who previously complained about these types > of problems, and see if we can get some wider testing. I should have them, but I won't have access to them for another 2.5 weeks unfortunately. Cheers, Jes
On 7/4/19 10:44 PM, Daniel Drake wrote: > On Wed, Jul 3, 2019 at 8:59 PM Jes Sorensen <jes.sorensen@gmail.com> wrote: >> My point is this seems to be very dongle dependent :( We have to be >> careful not breaking it for some users while fixing it for others. > > Do you still have your device? > > Once we get to the point when you are happy with Chris's two patches > here on a code review level, we'll reach out to other driver > contributors plus people who previously complained about these types > of problems, and see if we can get some wider testing. > > Larry, do you have these devices, can you help with testing too? I have some of the devices, and I can help with the testing. Larry
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c index 3adb1d3d47ac..6c3c70d93ac1 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) /* * WLAN action by PTA */ - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04); + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c); /* * BT select S0/S1 controlled by WiFi @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv) rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv)); /* - * 0x280, 0x00, 0x200, 0x80 - not clear + * Different settings per different antenna position. + * Antenna switch to BT: 0x280, 0x00 (inverse) + * Antenna switch to WiFi: 0x0, 0x280 (inverse) + * Antenna controlled by PTA: 0x200, 0x80 (inverse) */ - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00); + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80); /* * Software control, antenna at WiFi side diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c index 8136e268b4e6..87b2179a769e 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) /* Check if MAC is already powered on */ val8 = rtl8xxxu_read8(priv, REG_CR); + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR); /* * Fix 92DU-VC S3 hang with the reason is that secondary mac is not * initialized. First MAC returns 0xea, second MAC returns 0x00 */ - if (val8 == 0xea) + if (val8 == 0xea || !(val16 & BIT(11))) macpower = false; else macpower = true;
The WiFi tx power of RTL8723BU is extremely low after booting. So the WiFi scan gives very limited AP list and it always fails to connect to the selected AP. This module only supports 1x1 antenna and the antenna is switched to bluetooth due to some incorrect register settings. This commit hand over the antenna control to PTA, the wifi signal will be back to normal and the bluetooth scan can also work at the same time. However, the btcoexist still needs to be handled under different circumstances. If there's a BT connection established, the wifi still fails to connect until disconneting the BT. Signed-off-by: Chris Chiu <chiu@endlessm.com> --- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++++++--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-)