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 <[email protected]>
> ---
> 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);
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 <[email protected]>
>> ---
>> 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);
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 <[email protected]>
>>> ---
>>> 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