diff mbox series

wifi: mt76: mt76x02: correct external LNA gain

Message ID TYAP286MB0315A2BAF9E9E74B2377EE4CBCFAA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM (mailing list archive)
State Superseded
Delegated to: Felix Fietkau
Headers show
Series wifi: mt76: mt76x02: correct external LNA gain | expand

Commit Message

Shiji Yang Sept. 19, 2023, 4:48 a.m. UTC 
Referring to the MT761{0,2} EEPROM content, setting the corresponding
EEPROM control bit means enabling external LNA. In this case, we
should use the EEMROM LNA gain instead of 0.

Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
---
 drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Felix Fietkau Sept. 19, 2023, 11:24 a.m. UTC | #1 
On 19.09.23 06:48, Shiji Yang wrote:
> Referring to the MT761{0,2} EEPROM content, setting the corresponding
> EEPROM control bit means enabling external LNA. In this case, we
> should use the EEMROM LNA gain instead of 0.
> 
> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
> ---
>   drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
> index 0acabba2d..a0b95509a 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
> @@ -135,9 +135,9 @@ u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
>   	u8 lna;
>   
>   	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
> -	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
> +	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_2G))
>   		*lna_2g = 0;
> -	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
> +	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_5G))
>   		memset(lna_5g, 0, sizeof(s8) * 3);
>   
>   	if (chan->band == NL80211_BAND_2GHZ)

I took a closer look at the interpretation of these flags and how they
are handled in the vendor driver. From what I can tell, on MT76x2 LNA
gain should only be used as part of the RSSI calculation for internal
LNA devices. In the MT76x0 code, I see no such checks, so the LNA gain
should be used unconditionally.
What do you think about this patch?

Thanks,

- Felix

---
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
@@ -131,15 +131,8 @@ u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
  			s8 *lna_2g, s8 *lna_5g,
  			struct ieee80211_channel *chan)
  {
-	u16 val;
  	u8 lna;
  
-	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
-		*lna_2g = 0;
-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
-		memset(lna_5g, 0, sizeof(s8) * 3);
-
  	if (chan->band == NL80211_BAND_2GHZ)
  		lna = *lna_2g;
  	else if (chan->hw_value <= 64)
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c
@@ -256,7 +256,8 @@ void mt76x2_read_rx_gain(struct mt76x02_dev *dev)
  	struct ieee80211_channel *chan = dev->mphy.chandef.chan;
  	int channel = chan->hw_value;
  	s8 lna_5g[3], lna_2g;
-	u8 lna;
+	bool use_lna;
+	u8 lna = 0;
  	u16 val;
  
  	if (chan->band == NL80211_BAND_2GHZ)
@@ -275,7 +276,14 @@ void mt76x2_read_rx_gain(struct mt76x02_dev *dev)
  	dev->cal.rx.mcu_gain |= (lna_5g[1] & 0xff) << 16;
  	dev->cal.rx.mcu_gain |= (lna_5g[2] & 0xff) << 24;
  
-	lna = mt76x02_get_lna_gain(dev, &lna_2g, lna_5g, chan);
+	if (chan->band == NL80211_BAND_2GHZ)
+		use_lna = !(val & MT_EE_NIC_CONF_1_LNA_EXT_2G);
+	else
+		use_lna = !(val & MT_EE_NIC_CONF_1_LNA_EXT_5G);
+
+	if (use_lna)
+		lna = mt76x02_get_lna_gain(dev, &lna_2g, lna_5g, chan);
+
  	dev->cal.rx.lna_gain = mt76x02_sign_extend(lna, 8);
  }
  EXPORT_SYMBOL_GPL(mt76x2_read_rx_gain);
Felix Fietkau Sept. 19, 2023, 11:39 a.m. UTC | #2 
On 19.09.23 13:24, Felix Fietkau wrote:
> On 19.09.23 06:48, Shiji Yang wrote:
>> Referring to the MT761{0,2} EEPROM content, setting the corresponding
>> EEPROM control bit means enabling external LNA. In this case, we
>> should use the EEMROM LNA gain instead of 0.
>> 
>> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
>> ---
>>   drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>> index 0acabba2d..a0b95509a 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>> @@ -135,9 +135,9 @@ u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
>>   	u8 lna;
>>   
>>   	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
>> -	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
>> +	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_2G))
>>   		*lna_2g = 0;
>> -	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
>> +	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_5G))
>>   		memset(lna_5g, 0, sizeof(s8) * 3);
>>   
>>   	if (chan->band == NL80211_BAND_2GHZ)
> 
> I took a closer look at the interpretation of these flags and how they
> are handled in the vendor driver. From what I can tell, on MT76x2 LNA
> gain should only be used as part of the RSSI calculation for internal
> LNA devices. In the MT76x0 code, I see no such checks, so the LNA gain
> should be used unconditionally.
> What do you think about this patch?
> 
> Thanks,
> 
> - Felix

Sorry, there was a missing line in the last patch. Here is the right
version:

--- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
@@ -131,15 +131,8 @@ u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
  			s8 *lna_2g, s8 *lna_5g,
  			struct ieee80211_channel *chan)
  {
-	u16 val;
  	u8 lna;
  
-	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
-		*lna_2g = 0;
-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
-		memset(lna_5g, 0, sizeof(s8) * 3);
-
  	if (chan->band == NL80211_BAND_2GHZ)
  		lna = *lna_2g;
  	else if (chan->hw_value <= 64)
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c
@@ -256,7 +256,8 @@ void mt76x2_read_rx_gain(struct mt76x02_dev *dev)
  	struct ieee80211_channel *chan = dev->mphy.chandef.chan;
  	int channel = chan->hw_value;
  	s8 lna_5g[3], lna_2g;
-	u8 lna;
+	bool use_lna;
+	u8 lna = 0;
  	u16 val;
  
  	if (chan->band == NL80211_BAND_2GHZ)
@@ -275,7 +276,15 @@ void mt76x2_read_rx_gain(struct mt76x02_dev *dev)
  	dev->cal.rx.mcu_gain |= (lna_5g[1] & 0xff) << 16;
  	dev->cal.rx.mcu_gain |= (lna_5g[2] & 0xff) << 24;
  
-	lna = mt76x02_get_lna_gain(dev, &lna_2g, lna_5g, chan);
+	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
+	if (chan->band == NL80211_BAND_2GHZ)
+		use_lna = !(val & MT_EE_NIC_CONF_1_LNA_EXT_2G);
+	else
+		use_lna = !(val & MT_EE_NIC_CONF_1_LNA_EXT_5G);
+
+	if (use_lna)
+		lna = mt76x02_get_lna_gain(dev, &lna_2g, lna_5g, chan);
+
  	dev->cal.rx.lna_gain = mt76x02_sign_extend(lna, 8);
  }
  EXPORT_SYMBOL_GPL(mt76x2_read_rx_gain);
Shiji Yang Sept. 19, 2023, 12:38 p.m. UTC | #3 
On Tue, 19 Sep 2023 13:39:48 +0200, Felix Fietkau wrote:

>On 19.09.23 13:24, Felix Fietkau wrote:
>> On 19.09.23 06:48, Shiji Yang wrote:
>>> Referring to the MT761{0,2} EEPROM content, setting the corresponding
>>> EEPROM control bit means enabling external LNA. In this case, we
>>> should use the EEMROM LNA gain instead of 0.
>>> 
>>> Signed-off-by: Shiji Yang <yangshiji66@outlook.com>
>>> ---
>>>   drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>>> index 0acabba2d..a0b95509a 100644
>>> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>>> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>>> @@ -135,9 +135,9 @@ u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
>>>   	u8 lna;
>>>   
>>>   	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
>>> -	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
>>> +	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_2G))
>>>   		*lna_2g = 0;
>>> -	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
>>> +	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_5G))
>>>   		memset(lna_5g, 0, sizeof(s8) * 3);
>>>   
>>>   	if (chan->band == NL80211_BAND_2GHZ)
>> 
>> I took a closer look at the interpretation of these flags and how they
>> are handled in the vendor driver. From what I can tell, on MT76x2 LNA
>> gain should only be used as part of the RSSI calculation for internal
>> LNA devices. In the MT76x0 code, I see no such checks, so the LNA gain
>> should be used unconditionally.
>> What do you think about this patch?
>> 
>> Thanks,
>> 
>> - Felix
>
>Sorry, there was a missing line in the last patch. Here is the right
>version:
>
>--- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
>@@ -131,15 +131,8 @@ u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
>  			s8 *lna_2g, s8 *lna_5g,
>  			struct ieee80211_channel *chan)
>  {
>-	u16 val;
>  	u8 lna;
>  
>-	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
>-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
>-		*lna_2g = 0;
>-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
>-		memset(lna_5g, 0, sizeof(s8) * 3);
>-
>  	if (chan->band == NL80211_BAND_2GHZ)
>  		lna = *lna_2g;
>  	else if (chan->hw_value <= 64)
>--- a/drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c
>+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c
>@@ -256,7 +256,8 @@ void mt76x2_read_rx_gain(struct mt76x02_dev *dev)
>  	struct ieee80211_channel *chan = dev->mphy.chandef.chan;
>  	int channel = chan->hw_value;
>  	s8 lna_5g[3], lna_2g;
>-	u8 lna;
>+	bool use_lna;
>+	u8 lna = 0;
>  	u16 val;
>  
>  	if (chan->band == NL80211_BAND_2GHZ)
>@@ -275,7 +276,15 @@ void mt76x2_read_rx_gain(struct mt76x02_dev *dev)
>  	dev->cal.rx.mcu_gain |= (lna_5g[1] & 0xff) << 16;
>  	dev->cal.rx.mcu_gain |= (lna_5g[2] & 0xff) << 24;
>  
>-	lna = mt76x02_get_lna_gain(dev, &lna_2g, lna_5g, chan);
>+	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
>+	if (chan->band == NL80211_BAND_2GHZ)
>+		use_lna = !(val & MT_EE_NIC_CONF_1_LNA_EXT_2G);
>+	else
>+		use_lna = !(val & MT_EE_NIC_CONF_1_LNA_EXT_5G);
>+
>+	if (use_lna)
>+		lna = mt76x02_get_lna_gain(dev, &lna_2g, lna_5g, chan);
>+
>  	dev->cal.rx.lna_gain = mt76x02_sign_extend(lna, 8);
>  }
>  EXPORT_SYMBOL_GPL(mt76x2_read_rx_gain);
>

Your patch looks better than mine. Yes, I also found that the MT7610
stock driver always unconditionally reads lna gain from eeprom. So
please keep your fix version.

Regards,
Shiji Yang
diff mbox series

Patch

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
index 0acabba2d..a0b95509a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_eeprom.c
@@ -135,9 +135,9 @@  u8 mt76x02_get_lna_gain(struct mt76x02_dev *dev,
 	u8 lna;
 
 	val = mt76x02_eeprom_get(dev, MT_EE_NIC_CONF_1);
-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_2G)
+	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_2G))
 		*lna_2g = 0;
-	if (val & MT_EE_NIC_CONF_1_LNA_EXT_5G)
+	if (!(val & MT_EE_NIC_CONF_1_LNA_EXT_5G))
 		memset(lna_5g, 0, sizeof(s8) * 3);
 
 	if (chan->band == NL80211_BAND_2GHZ)