Received-SPF: pass (google.com: domain of linux-wireless+bounces-5305-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <387fa334-cc78-44fb-a638-0da9d104250c@gmail.com>
Date: Wed, 27 Mar 2024 00:13:26 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 06/12] wifi: rtlwifi: Add rtl8192du/phy.{c,h}
Content-Language: en-US
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
 "s.l-h@gmx.de" <s.l-h@gmx.de>, "chewitt@libreelec.tv" <chewitt@libreelec.tv>
References: <7f4b3309-1580-48f3-9426-29f1eb4052fd@gmail.com>
 <367ae147-3f2d-4ecc-989f-62fd7d82ebfc@gmail.com>
 <59596a57735c713a135ec64b05b8031ad9149421.camel@realtek.com>
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <59596a57735c713a135ec64b05b8031ad9149421.camel@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 22/03/2024 07:22, Ping-Ke Shih wrote:
> On Wed, 2024-03-20 at 21:38 +0200, Bitterblue Smith wrote:

> [...]
> 
>> +void rtl92d_phy_lc_calibrate(struct ieee80211_hw *hw, bool is2t)
>> +{
>> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
>> +       struct rtl_hal *rtlhal = &rtlpriv->rtlhal;
>> +       struct rtl_phy *rtlphy = &rtlpriv->phy;
>> +       u32 timeout = 2000, timecount = 0;
>> +
>> +       while (rtlpriv->mac80211.act_scanning && timecount < timeout) {
>> +               udelay(50);
>> +               timecount += 50;
> 
> What is the purpose? 
> 
> Even if you really need it, just mdelay(2) or something like that? > 

This comes from rtl8192de. The TX power tracking can re-do LC
calibration. Someone decided that shouldn't happen while
scanning. I don't know why.

>> +       }
>> +
>> +       rtlphy->lck_inprogress = true;
>> +       RTPRINT(rtlpriv, FINIT, INIT_IQK,
>> +               "LCK:Start!!! currentband %x delay %d ms\n",
>> +               rtlhal->current_bandtype, timecount);
>> +
>> +       _rtl92d_phy_lc_calibrate_sw(hw, is2t);
>> +
>> +       rtlphy->lck_inprogress = false;
>> +       RTPRINT(rtlpriv, FINIT, INIT_IQK,  "LCK:Finish!!!\n");
>> +}
>> +
> 
> [...]
> 
>> +u8 rtl92d_phy_sw_chnl(struct ieee80211_hw *hw)
>> +{
>> + 
> 
> [...]
> 
>> +       while (rtlphy->lck_inprogress && timecount < timeout) {
>> +               mdelay(50);
>> +               timecount += 50;
>> +       }
> 
> Could LCK and switch channel happen simultaneously?
> Can you point out the case?
> 

When a scan coincides with the TX power tracking deciding to
re-do LC calibration, I guess.

> 
>> +       if (rtlhal->macphymode == SINGLEMAC_SINGLEPHY &&
>> +           rtlhal->bandset == BAND_ON_BOTH) {
>> +               ret_value = rtl_get_bbreg(hw, RFPGA0_XAB_RFPARAMETER,
>> +                                         MASKDWORD);
>> +               if (rtlphy->current_channel > 14 && !(ret_value & BIT(0)))
>> +                       rtl92d_phy_switch_wirelessband(hw, BAND_ON_5G);
>> +               else if (rtlphy->current_channel <= 14 && (ret_value & BIT(0)))
>> +                       rtl92d_phy_switch_wirelessband(hw, BAND_ON_2_4G);
>> +       }
>> +       switch (rtlhal->current_bandtype) {
>> +       case BAND_ON_5G:
>> +               /* Get first channel error when change between
>> +                * 5G and 2.4G band.
>> +                */
>> +               if (WARN_ONCE(channel <= 14, "rtl8192de: 5G but channel<=14\n"))
>> +                       return 0;
>> +               break;
>> +       case BAND_ON_2_4G:
>> +               /* Get first channel error when change between
>> +                * 5G and 2.4G band.
>> +                */
>> +               if (WARN_ONCE(channel > 14, "rtl8192de: 2G but channel>14\n"))
>> +                       return 0;
>> +               break;
>> +       default:
>> +               WARN_ONCE(true, "rtl8192de: Invalid WirelessMode(%#x)!!\n",
>> +                         rtlpriv->mac80211.mode);
>> +               break;
>> +       }
>> +       rtlphy->sw_chnl_inprogress = true;
>> +       if (channel == 0)
>> +               channel = 1;
> 
> Can this really happen? 
> 

I think it can't happen in rtl8192du. I will remove this check.

> 
> 
>> +
>> +       /* notice fw know band status  0x81[1]/0x53[1] = 0: 5G, 1: 2G */
>> +       if (rtlhal->current_bandtype == BAND_ON_2_4G) {
>> +               value8 = rtl_read_byte(rtlpriv, mac_reg);
>> +               value8 |= BIT(1);
>> +               rtl_write_byte(rtlpriv, mac_reg, value8);
>> +       } else {
>> +               value8 = rtl_read_byte(rtlpriv, mac_reg);
>> +               value8 &= (~BIT(1));
>> +               rtl_write_byte(rtlpriv, mac_reg, value8);
>> +       }
>> +
>> +       if (rtlhal->macphymode == SINGLEMAC_SINGLEPHY) {
>> +               value8 = rtl_read_byte(rtlpriv, REG_MAC0);
>> +               rtl_write_byte(rtlpriv, REG_MAC0, value8 | MAC0_ON);
>> +       } else {
>> +               mutex_lock(&globalmutex_power);
> 
> What do you need 'global' mutex? Usually, we want to support multiple instances
> run simultaneously. Does this mutext across instances? 
> I feel not. Please move them into struct rtl_priv.
> 

With the dual MAC version of RTL8192DU two instances of the
driver will access the same chip. This is why we have some
global mutexes. I think multiple devices can still work at the
same time, they will just slow each other down a little during
hardware init/deinit.

>> +               if (rtlhal->interfaceindex == 0) {
>> +                       value8 = rtl_read_byte(rtlpriv, REG_MAC0);
>> +                       rtl_write_byte(rtlpriv, REG_MAC0, value8 | MAC0_ON);
>> +               } else {
>> +                       value8 = rtl_read_byte(rtlpriv, REG_MAC1);
>> +                       rtl_write_byte(rtlpriv, REG_MAC1, value8 | MAC1_ON);
>> +               }
>> +               value8 = rtl_read_byte(rtlpriv, REG_POWER_OFF_IN_PROCESS);
>> +               mutex_unlock(&globalmutex_power);
>