Don't subtract 1 from the power index. This was added in commit
2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
for unknown reasons. The vendor drivers don't do this.
Also correct the calculations of values written to
REG_OFDM0_X{C,D}_TX_IQ_IMBALANCE. According to the vendor driver,
these are used for TX power training.
With these changes rtl8xxxu sets the TX power of RTL8192CU the same
as the vendor driver.
None of this appears to have any effect on my RTL8192CU device.
---
My RTL8192CU with rtl8xxxu has lower upload speed compared to the
vendor driver, so I compared the register writes and found
differences in the TX power stuff.
Jes, do you remember why you subtracted 1 from the power index?
That has to be on purpose. The other differences look unintentional.
---
.../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 5d102a1246a3..e111fb2b2c30 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -1510,8 +1510,8 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
group = rtl8xxxu_gen1_channel_to_group(channel);
- cck[0] = priv->cck_tx_power_index_A[group] - 1;
- cck[1] = priv->cck_tx_power_index_B[group] - 1;
+ cck[0] = priv->cck_tx_power_index_A[group];
+ cck[1] = priv->cck_tx_power_index_B[group];
if (priv->hi_pa) {
if (cck[0] > 0x20)
@@ -1522,10 +1522,6 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
ofdm[0] = priv->ht40_1s_tx_power_index_A[group];
ofdm[1] = priv->ht40_1s_tx_power_index_B[group];
- if (ofdm[0])
- ofdm[0] -= 1;
- if (ofdm[1])
- ofdm[1] -= 1;
ofdmbase[0] = ofdm[0] + priv->ofdm_tx_power_index_diff[group].a;
ofdmbase[1] = ofdm[1] + priv->ofdm_tx_power_index_diff[group].b;
@@ -1614,20 +1610,22 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
rtl8xxxu_write32(priv, REG_TX_AGC_A_MCS15_MCS12,
mcs_a + power_base->reg_0e1c);
+ val8 = u32_get_bits(mcs_a + power_base->reg_0e1c, 0xff000000);
for (i = 0; i < 3; i++) {
if (i != 2)
- val8 = (mcsbase[0] > 8) ? (mcsbase[0] - 8) : 0;
+ val8 = (val8 > 8) ? (val8 - 8) : 0;
else
- val8 = (mcsbase[0] > 6) ? (mcsbase[0] - 6) : 0;
+ val8 = (val8 > 6) ? (val8 - 6) : 0;
rtl8xxxu_write8(priv, REG_OFDM0_XC_TX_IQ_IMBALANCE + i, val8);
}
rtl8xxxu_write32(priv, REG_TX_AGC_B_MCS15_MCS12,
mcs_b + power_base->reg_0868);
+ val8 = u32_get_bits(mcs_b + power_base->reg_0868, 0xff000000);
for (i = 0; i < 3; i++) {
if (i != 2)
- val8 = (mcsbase[1] > 8) ? (mcsbase[1] - 8) : 0;
+ val8 = (val8 > 8) ? (val8 - 8) : 0;
else
- val8 = (mcsbase[1] > 6) ? (mcsbase[1] - 6) : 0;
+ val8 = (val8 > 6) ? (val8 - 6) : 0;
rtl8xxxu_write8(priv, REG_OFDM0_XD_TX_IQ_IMBALANCE + i, val8);
}
}
--
2.41.0
> -----Original Message-----
> From: Bitterblue Smith <[email protected]>
> Sent: Monday, July 10, 2023 2:38 AM
> To: [email protected]
> Cc: Jes Sorensen <[email protected]>; Ping-Ke Shih <[email protected]>
> Subject: [PATCH RFC] wifi: rtl8xxxu: Fix the TX power of RTL8192CU, RTL8723AU
>
> Don't subtract 1 from the power index. This was added in commit
> 2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
> for unknown reasons. The vendor drivers don't do this.
>
> Also correct the calculations of values written to
> REG_OFDM0_X{C,D}_TX_IQ_IMBALANCE. According to the vendor driver,
> these are used for TX power training.
>
> With these changes rtl8xxxu sets the TX power of RTL8192CU the same
> as the vendor driver.
>
> None of this appears to have any effect on my RTL8192CU device.
> ---
> My RTL8192CU with rtl8xxxu has lower upload speed compared to the
> vendor driver, so I compared the register writes and found
> differences in the TX power stuff.
>
> Jes, do you remember why you subtracted 1 from the power index?
> That has to be on purpose. The other differences look unintentional.
> ---
> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 5d102a1246a3..e111fb2b2c30 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -1510,8 +1510,8 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>
> group = rtl8xxxu_gen1_channel_to_group(channel);
>
> - cck[0] = priv->cck_tx_power_index_A[group] - 1;
> - cck[1] = priv->cck_tx_power_index_B[group] - 1;
> + cck[0] = priv->cck_tx_power_index_A[group];
> + cck[1] = priv->cck_tx_power_index_B[group];
>
> if (priv->hi_pa) {
> if (cck[0] > 0x20)
> @@ -1522,10 +1522,6 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>
> ofdm[0] = priv->ht40_1s_tx_power_index_A[group];
> ofdm[1] = priv->ht40_1s_tx_power_index_B[group];
> - if (ofdm[0])
> - ofdm[0] -= 1;
> - if (ofdm[1])
> - ofdm[1] -= 1;
>
> ofdmbase[0] = ofdm[0] + priv->ofdm_tx_power_index_diff[group].a;
> ofdmbase[1] = ofdm[1] + priv->ofdm_tx_power_index_diff[group].b;
> @@ -1614,20 +1610,22 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>
> rtl8xxxu_write32(priv, REG_TX_AGC_A_MCS15_MCS12,
> mcs_a + power_base->reg_0e1c);
> + val8 = u32_get_bits(mcs_a + power_base->reg_0e1c, 0xff000000);
Comparing to vendor driver, I think the logic here is the same.
> for (i = 0; i < 3; i++) {
> if (i != 2)
> - val8 = (mcsbase[0] > 8) ? (mcsbase[0] - 8) : 0;
> + val8 = (val8 > 8) ? (val8 - 8) : 0;
> else
> - val8 = (mcsbase[0] > 6) ? (mcsbase[0] - 6) : 0;
> + val8 = (val8 > 6) ? (val8 - 6) : 0;
Would you like val8 = min_t(int, val8 - 6, 0); ?
Even, merge two branches into one.
base = i != 2 ? 8 : 6;
val8 = min_t(int, val8 - base, 0);
[...]
On 10/07/2023 04:42, Ping-Ke Shih wrote:
>
>
>> -----Original Message-----
>> From: Bitterblue Smith <[email protected]>
>> Sent: Monday, July 10, 2023 2:38 AM
>> To: [email protected]
>> Cc: Jes Sorensen <[email protected]>; Ping-Ke Shih <[email protected]>
>> Subject: [PATCH RFC] wifi: rtl8xxxu: Fix the TX power of RTL8192CU, RTL8723AU
>>
>> Don't subtract 1 from the power index. This was added in commit
>> 2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
>> for unknown reasons. The vendor drivers don't do this.
>>
>> Also correct the calculations of values written to
>> REG_OFDM0_X{C,D}_TX_IQ_IMBALANCE. According to the vendor driver,
>> these are used for TX power training.
>>
>> With these changes rtl8xxxu sets the TX power of RTL8192CU the same
>> as the vendor driver.
>>
>> None of this appears to have any effect on my RTL8192CU device.
>> ---
>> My RTL8192CU with rtl8xxxu has lower upload speed compared to the
>> vendor driver, so I compared the register writes and found
>> differences in the TX power stuff.
>>
>> Jes, do you remember why you subtracted 1 from the power index?
>> That has to be on purpose. The other differences look unintentional.
>> ---
>> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 ++++++++----------
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index 5d102a1246a3..e111fb2b2c30 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -1510,8 +1510,8 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>>
>> group = rtl8xxxu_gen1_channel_to_group(channel);
>>
>> - cck[0] = priv->cck_tx_power_index_A[group] - 1;
>> - cck[1] = priv->cck_tx_power_index_B[group] - 1;
>> + cck[0] = priv->cck_tx_power_index_A[group];
>> + cck[1] = priv->cck_tx_power_index_B[group];
>>
>> if (priv->hi_pa) {
>> if (cck[0] > 0x20)
>> @@ -1522,10 +1522,6 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>>
>> ofdm[0] = priv->ht40_1s_tx_power_index_A[group];
>> ofdm[1] = priv->ht40_1s_tx_power_index_B[group];
>> - if (ofdm[0])
>> - ofdm[0] -= 1;
>> - if (ofdm[1])
>> - ofdm[1] -= 1;
>>
>> ofdmbase[0] = ofdm[0] + priv->ofdm_tx_power_index_diff[group].a;
>> ofdmbase[1] = ofdm[1] + priv->ofdm_tx_power_index_diff[group].b;
>> @@ -1614,20 +1610,22 @@ rtl8xxxu_gen1_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>>
>> rtl8xxxu_write32(priv, REG_TX_AGC_A_MCS15_MCS12,
>> mcs_a + power_base->reg_0e1c);
>> + val8 = u32_get_bits(mcs_a + power_base->reg_0e1c, 0xff000000);
>
> Comparing to vendor driver, I think the logic here is the same.
>
>> for (i = 0; i < 3; i++) {
>> if (i != 2)
>> - val8 = (mcsbase[0] > 8) ? (mcsbase[0] - 8) : 0;
>> + val8 = (val8 > 8) ? (val8 - 8) : 0;
>> else
>> - val8 = (mcsbase[0] > 6) ? (mcsbase[0] - 6) : 0;
>> + val8 = (val8 > 6) ? (val8 - 6) : 0;
>
> Would you like val8 = min_t(int, val8 - 6, 0); ?
>
> Even, merge two branches into one.
> base = i != 2 ? 8 : 6;
> val8 = min_t(int, val8 - base, 0);
>
> [...]
>
I think max_t, but otherwise yes, that looks good.
> -----Original Message-----
> From: Bitterblue Smith <[email protected]>
> Sent: Friday, July 14, 2023 1:24 AM
> To: Ping-Ke Shih <[email protected]>; [email protected]
> Cc: Jes Sorensen <[email protected]>
> Subject: Re: [PATCH RFC] wifi: rtl8xxxu: Fix the TX power of RTL8192CU, RTL8723AU
>
> On 10/07/2023 04:42, Ping-Ke Shih wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bitterblue Smith <[email protected]>
> >> Sent: Monday, July 10, 2023 2:38 AM
> >> To: [email protected]
> >> Cc: Jes Sorensen <[email protected]>; Ping-Ke Shih <[email protected]>
> >> Subject: [PATCH RFC] wifi: rtl8xxxu: Fix the TX power of RTL8192CU, RTL8723AU
> >>
> >> for (i = 0; i < 3; i++) {
> >> if (i != 2)
> >> - val8 = (mcsbase[0] > 8) ? (mcsbase[0] - 8) : 0;
> >> + val8 = (val8 > 8) ? (val8 - 8) : 0;
> >> else
> >> - val8 = (mcsbase[0] > 6) ? (mcsbase[0] - 6) : 0;
> >> + val8 = (val8 > 6) ? (val8 - 6) : 0;
> >
> > Would you like val8 = min_t(int, val8 - 6, 0); ?
> >
> > Even, merge two branches into one.
> > base = i != 2 ? 8 : 6;
> > val8 = min_t(int, val8 - base, 0);
> >
> > [...]
> >
>
> I think max_t, but otherwise yes, that looks good.
>
Oops. You are right. Sorry for the mistakes.