[PATCH 0/2] ath5k: fixing broken power gain calibration at 5GHz and txpower handling

[PATCH 0/2] ath5k: fixing broken power gain calibration at 5GHz and txpower handling

[Thomas Huehn]

This patch series fix a wrong power gain calibration in ath5k for
entire 5GHz band and enables consistent usage of tx_power specified by
the user.

(1)
Function ath5k_eeprom_read_target_rate_pwr_info() read 10 lines of
rate_pcal_info[i].target_power which worngly produces e.g. from 
Winstron CM9:
[   38.988000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[0].freq = 4920, rate_pcal_info[0].target_power_6to24 = 36, rate_pcal_info[0].target_power_54 = 24
[   39.004000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[1].freq = 5040, rate_pcal_info[1].target_power_6to24 = 36, rate_pcal_info[1].target_power_54 = 24
[   39.020000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[2].freq = 5170, rate_pcal_info[2].target_power_6to24 = 36, rate_pcal_info[2].target_power_54 = 26
[   39.040000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[3].freq = 5240, rate_pcal_info[3].target_power_6to24 = 38, rate_pcal_info[3].target_power_54 = 26
[   39.056000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[4].freq = 5320, rate_pcal_info[4].target_power_6to24 = 38, rate_pcal_info[4].target_power_54 = 26
[   39.076000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[5].freq = 5500, rate_pcal_info[5].target_power_6to24 = 38, rate_pcal_info[5].target_power_54 = 26
[   39.092000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[6].freq = 5700, rate_pcal_info[6].target_power_6to24 = 36, rate_pcal_info[6].target_power_54 = 26
[   39.112000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[7].freq = 5825, rate_pcal_info[7].target_power_6to24 = 34, rate_pcal_info[7].target_power_54 = 24
[   39.128000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[8].freq = 5360, rate_pcal_info[8].target_power_6to24 = 39, rate_pcal_info[8].target_power_54 = 39
[   39.144000] [ath5k_eeprom_read_target_rate_pwr_info] rate_pcal_info[9].freq = 5720, rate_pcal_info[9].target_power_6to24 = 38, rate_pcal_info[9].target_power_54 = 38

Beside that both last 2 lines hold invalid power gain values the last
line is responsible that subsequent interpolation functions apply
full power gain to all rates at all frequencies greater
rate_pcal_info[max].freq, i.e. > 5720 MHz. This leads to spectral
distortion when then global tx_power is higher than the expected
rate_pcal_info[xy].target_power_54 and therfore gives rates above
24MBit a very poor performance in therms of throughput. The first
patch reduces the eeprom calibration reads to 8. A validation
against madwifi showed 8x eeprom reads at 5GHz as well.

(2)
Furthermore the tx_power specified by the user is not respected in
cases where function phy_init() got called: tx_power is always
set to ah->ah_txpower.txp_cur_pwr which is never updated with the
txpower value specified by the user instead it equales max chan power
and is even incremented. So this wrong behavior amplifies the
occurrence of the performance problems coming from the wrong
calibration in cases the user set a low power. The second patch
fixes this by properly introduce and respect user tx_power.

Greetings Thomas

[PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Thomas Huehn]

This patch reduces the per rate target power eeprom reads for
AR5K_EEPROM_MODE_11A from 10 to 8, as there are only 8 valid
power curve entries on the eeprom. The former 10 reads lead to
wrong per rate power limits where rates above 24MBit could be
amplified with to high distortion leading to bad performance.
This was fix validated against the madwifi codebase and a new
AR5K_EEPROM_N_5GHZ_RATE_CHAN 8 is defined.

Signed-off-by: Thomas Huehn <thomas@net.t-labs.tu-berlin.de>
---
madwifi cross validation check. Thx to Felix Fiethkau
---
 drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
 drivers/net/wireless/ath/ath5k/eeprom.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath5k/eeprom.c b/drivers/net/wireless/ath/ath5k/eeprom.c
index 4026c90..b7e0258 100644
--- a/drivers/net/wireless/ath/ath5k/eeprom.c
+++ b/drivers/net/wireless/ath/ath5k/eeprom.c
@@ -1482,7 +1482,7 @@ ath5k_eeprom_read_target_rate_pwr_info(struct ath5k_hw *ah, unsigned int mode)
 	case AR5K_EEPROM_MODE_11A:
 		offset += AR5K_EEPROM_TARGET_PWR_OFF_11A(ee->ee_version);
 		rate_pcal_info = ee->ee_rate_tpwr_a;
-		ee->ee_rate_target_pwr_num[mode] = AR5K_EEPROM_N_5GHZ_CHAN;
+		ee->ee_rate_target_pwr_num[mode] = AR5K_EEPROM_N_5GHZ_RATE_CHAN;
 		break;
 	case AR5K_EEPROM_MODE_11B:
 		offset += AR5K_EEPROM_TARGET_PWR_OFF_11B(ee->ee_version);
diff --git a/drivers/net/wireless/ath/ath5k/eeprom.h b/drivers/net/wireless/ath/ath5k/eeprom.h
index dc2bcfe..94a9bbe 100644
--- a/drivers/net/wireless/ath/ath5k/eeprom.h
+++ b/drivers/net/wireless/ath/ath5k/eeprom.h
@@ -182,6 +182,7 @@
 #define AR5K_EEPROM_EEP_DELTA		10
 #define AR5K_EEPROM_N_MODES		3
 #define AR5K_EEPROM_N_5GHZ_CHAN		10
+#define AR5K_EEPROM_N_5GHZ_RATE_CHAN	8
 #define AR5K_EEPROM_N_2GHZ_CHAN		3
 #define AR5K_EEPROM_N_2GHZ_CHAN_2413	4
 #define	AR5K_EEPROM_N_2GHZ_CHAN_MAX	4
-- 
1.7.11.1

Re: [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/7/23 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> This patch reduces the per rate target power eeprom reads for
> AR5K_EEPROM_MODE_11A from 10 to 8, as there are only 8 valid
> power curve entries on the eeprom. The former 10 reads lead to
> wrong per rate power limits where rates above 24MBit could be
> amplified with to high distortion leading to bad performance.
> This was fix validated against the madwifi codebase and a new
> AR5K_EEPROM_N_5GHZ_RATE_CHAN 8 is defined.
>
> Signed-off-by: Thomas Huehn <thomas@net.t-labs.tu-berlin.de>
> ---
> madwifi cross validation check. Thx to Felix Fiethkau
> ---


Thanks for catching this but the bug is elsewhere:

I've tried to document hw code as much as I can, eeprom stuff is still
missing and I'm sorry for that but here is what comments say on phy.c
@ DOC: Per-rate tx power setting, that should give you an idea of
what's happening...

3498  * Rate power table contains indices to PCDAC/PDADC table (0.5dB steps -
3499  * x values) and is indexed as follows:
3500  * rates[0] - rates[7] -> OFDM rates
3501  * rates[8] - rates[14] -> CCK rates
3502  * rates[15] -> XR rates (they all have the same power)

So guess why the last 2 "invalid" power gain values have the same
power levels for all rates ? They are for XR mode (they've put it
there on the eeprom because XR mode also operates at 5GHz) and since
XR stuff is stripped off Madwifi (at least the public HAL) that's why
it only reads 8 of the piers.

Now notice that the last 2 readings on your previous post start from a
different frequency and most important that 5360 < 5825  and that
makes sense since the idea is to have only 2 points to interpolate
between them for the whole XR range. So we have 2 frequency "ranges"
to search, the one  is the 11a mode from 4920 to 5825 and the other
one is for XR mode from 5360 to 5720. When the first range ends the
other starts.

The bug is here:

ath5k_get_rate_pcal_data:
2704         max = ee->ee_rate_target_pwr_num[mode] - 1;
[...]
2713         if (target > rpinfo[max].freq) {
2714                 idx_l = idx_r = max;
2715                 goto done;
2716         }

So please instead of tweaking the EEPROM code just change
max = ee->ee_rate_target_pwr_num[mode] - 1;
to
max = ee->ee_rate_target_pwr_num[mode] - 3;
in case of AR5K_MODE_11A

Each function has a job to do, ath5k_eeprom_read_target_rate_pwr_info
should read EEPROM contents, not decide what we'll use on
interpolation and what we won't use. Also since EEPROM code uses
offsets etc I don't want to change the way we read it because if we
miss something then the whole dataset will get "shifted" and we'll get
weird stuff :P


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Felix Fietkau]

On 2012-07-25 8:42 PM, Nick Kossifidis wrote:
> 2012/7/23 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>> This patch reduces the per rate target power eeprom reads for
>> AR5K_EEPROM_MODE_11A from 10 to 8, as there are only 8 valid
>> power curve entries on the eeprom. The former 10 reads lead to
>> wrong per rate power limits where rates above 24MBit could be
>> amplified with to high distortion leading to bad performance.
>> This was fix validated against the madwifi codebase and a new
>> AR5K_EEPROM_N_5GHZ_RATE_CHAN 8 is defined.
>>
>> Signed-off-by: Thomas Huehn <thomas@net.t-labs.tu-berlin.de>
>> ---
>> madwifi cross validation check. Thx to Felix Fiethkau
>> ---
> 
> 
> Thanks for catching this but the bug is elsewhere:
> 
> I've tried to document hw code as much as I can, eeprom stuff is still
> missing and I'm sorry for that but here is what comments say on phy.c
> @ DOC: Per-rate tx power setting, that should give you an idea of
> what's happening...
> 
> 3498  * Rate power table contains indices to PCDAC/PDADC table (0.5dB steps -
> 3499  * x values) and is indexed as follows:
> 3500  * rates[0] - rates[7] -> OFDM rates
> 3501  * rates[8] - rates[14] -> CCK rates
> 3502  * rates[15] -> XR rates (they all have the same power)
> 
> So guess why the last 2 "invalid" power gain values have the same
> power levels for all rates ? They are for XR mode (they've put it
> there on the eeprom because XR mode also operates at 5GHz) and since
> XR stuff is stripped off Madwifi (at least the public HAL) that's why
> it only reads 8 of the piers.
It's not just stripped in the public HAL. Every driver that I've looked
at only reads 8 of the piers in the function that parses the EEPROM.

- Felix

Re: [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/7/25 Felix Fietkau <nbd@openwrt.org>:
> On 2012-07-25 8:42 PM, Nick Kossifidis wrote:
>> 2012/7/23 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>> This patch reduces the per rate target power eeprom reads for
>>> AR5K_EEPROM_MODE_11A from 10 to 8, as there are only 8 valid
>>> power curve entries on the eeprom. The former 10 reads lead to
>>> wrong per rate power limits where rates above 24MBit could be
>>> amplified with to high distortion leading to bad performance.
>>> This was fix validated against the madwifi codebase and a new
>>> AR5K_EEPROM_N_5GHZ_RATE_CHAN 8 is defined.
>>>
>>> Signed-off-by: Thomas Huehn <thomas@net.t-labs.tu-berlin.de>
>>> ---
>>> madwifi cross validation check. Thx to Felix Fiethkau
>>> ---
>>
>>
>> Thanks for catching this but the bug is elsewhere:
>>
>> I've tried to document hw code as much as I can, eeprom stuff is still
>> missing and I'm sorry for that but here is what comments say on phy.c
>> @ DOC: Per-rate tx power setting, that should give you an idea of
>> what's happening...
>>
>> 3498  * Rate power table contains indices to PCDAC/PDADC table (0.5dB steps -
>> 3499  * x values) and is indexed as follows:
>> 3500  * rates[0] - rates[7] -> OFDM rates
>> 3501  * rates[8] - rates[14] -> CCK rates
>> 3502  * rates[15] -> XR rates (they all have the same power)
>>
>> So guess why the last 2 "invalid" power gain values have the same
>> power levels for all rates ? They are for XR mode (they've put it
>> there on the eeprom because XR mode also operates at 5GHz) and since
>> XR stuff is stripped off Madwifi (at least the public HAL) that's why
>> it only reads 8 of the piers.
> It's not just stripped in the public HAL. Every driver that I've looked
> at only reads 8 of the piers in the function that parses the EEPROM.
>
> - Felix

XR mode stuff is being stripped out from anything I got (docs etc) so
in some cases I don't understand something I blame XR :P However I
double checked this and it seems you are right:

a) 11a has 10 cal piers for the channel power table and 8 cal piers
for the rate powertable, not only that but the 8 are optional and
might not even cover the whole freq range.

b) I printed out the offsets and what happens it that the last 2
measurements we got have the same format because they are the 802.11b
cal piers and they have the same power levels because 11b is CCK so it
makes sense.

c) Something fishy is going on with ee->ee_xr_power[mode], is this the
max rate power for all channels on XR mode ? Is that possible to have
the same power index (and a high one) for all channels ? That's the
main reason I suspected these two values have something to do with XR.
If that's the case shouldn't we use that instead of rates[0] on
ath5k_setup_rate_powertable for XR rates ? Does any of the drivers you
've seen use ee->ee_xr_power[mode] ?

Anyway I left the EEPROM code unmaintained for a long time, the
related TODOs on wireless.kernel.org wiki are there too. Since I now
have documentation on EEPROM I'll start working on it again and clean
it up.


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Thomas Huehn]

Hi Nick,

Nick Kossifidis schrieb:

> Thanks for catching this but the bug is elsewhere:

> So guess why the last 2 "invalid" power gain values have the same
> power levels for all rates ? They are for XR mode (they've put it
> there on the eeprom because XR mode also operates at 5GHz) and since
> XR stuff is stripped off Madwifi (at least the public HAL) that's why
> it only reads 8 of the piers.
> 
> Now notice that the last 2 readings on your previous post start from a
> different frequency and most important that 5360 < 5825  and that
> makes sense since the idea is to have only 2 points to interpolate
> between them for the whole XR range. So we have 2 frequency "ranges"
> to search, the one  is the 11a mode from 4920 to 5825 and the other
> one is for XR mode from 5360 to 5720. When the first range ends the
> other starts.
> 
> The bug is here:
> 
> ath5k_get_rate_pcal_data:
> 2704         max = ee->ee_rate_target_pwr_num[mode] - 1;
> [...]
> 2713         if (target > rpinfo[max].freq) {
> 2714                 idx_l = idx_r = max;
> 2715                 goto done;
> 2716         }
> 
> So please instead of tweaking the EEPROM code just change
> max = ee->ee_rate_target_pwr_num[mode] - 1;
> to
> max = ee->ee_rate_target_pwr_num[mode] - 3;
> in case of AR5K_MODE_11A 


The calibration data for 802.11a in the standard conform mode should
only consider 8 reads from the eeprom, thats why I introduced this new
define... a "-3" is from my view of readability not as traceable.
Any other non standard mode as you mentioned XR as one of, could just
ready its necessary lines of the eeprom ones it get supported in ath5k.

Greetings Thomas

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick,
>
> [...]
>
> The calibration data for 802.11a in the standard conform mode should
> only consider 8 reads from the eeprom, thats why I introduced this new
> define... a "-3" is from my view of readability not as traceable.
> Any other non standard mode as you mentioned XR as one of, could just
> ready its necessary lines of the eeprom ones it get supported in ath5k.
>
> Greetings Thomas

Sorry for the mess Thomas, guess I should start trusting the docs etc
I have and leave rev. engineering mode from now on :P

Acked-by: Nick Kossifidis <mickflemm@gmail.com>

-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Thomas Huehn]

Hi all,

After several experiments on cm9 and dcma82 cards, I figured out that
this patch does not solve the max_power calibration problem as intended.

The partly reduction to 8 with this:
#define AR5K_EEPROM_N_5GHZ_CHAN		10
#define AR5K_EEPROM_N_5GHZ_RATE_CHAN	8

... creates a wrong power curve on the card, as function
ath5k_eeprom_read_freq_list() runs its while loop still 10 times, which
results in wrong AR5K_EEPROM_READ(o++, val) readings, leading the card
to use a very low power level over all.
This should also be limited to 8, as I tested it.

My suggestion is to just set:
#define AR5K_EEPROM_N_5GHZ_CHAN		8

.. without introducing a separate variable, it is not needed.
I will send a v2.

While I am browsing through /ath5k/eeprom.c there are several other
suspicious places where 10 eeprom lines of chips like 5111, 5112, 2413
depending on their EEPROM Version are read. I can not test this, as I
have only CM9 and DCMA82 over here, but I guess those 10 line reads are
also wrong. Can someone test this ?


Greetings Thomas


> --- a/drivers/net/wireless/ath/ath5k/eeprom.c
> +++ b/drivers/net/wireless/ath/ath5k/eeprom.c
> @@ -1482,7 +1482,7 @@ ath5k_eeprom_read_target_rate_pwr_info(struct ath5k_hw *ah, unsigned int mode)
>  	case AR5K_EEPROM_MODE_11A:
>  		offset += AR5K_EEPROM_TARGET_PWR_OFF_11A(ee->ee_version);
>  		rate_pcal_info = ee->ee_rate_tpwr_a;
> -		ee->ee_rate_target_pwr_num[mode] = AR5K_EEPROM_N_5GHZ_CHAN;
> +		ee->ee_rate_target_pwr_num[mode] = AR5K_EEPROM_N_5GHZ_RATE_CHAN;
>  		break;
>  	case AR5K_EEPROM_MODE_11B:
>  		offset += AR5K_EEPROM_TARGET_PWR_OFF_11B(ee->ee_version);
> diff --git a/drivers/net/wireless/ath/ath5k/eeprom.h b/drivers/net/wireless/ath/ath5k/eeprom.h
> index dc2bcfe..94a9bbe 100644
> --- a/drivers/net/wireless/ath/ath5k/eeprom.h
> +++ b/drivers/net/wireless/ath/ath5k/eeprom.h
> @@ -182,6 +182,7 @@
>  #define AR5K_EEPROM_EEP_DELTA		10
>  #define AR5K_EEPROM_N_MODES		3
>  #define AR5K_EEPROM_N_5GHZ_CHAN		10
> +#define AR5K_EEPROM_N_5GHZ_RATE_CHAN	8
>  #define AR5K_EEPROM_N_2GHZ_CHAN		3
>  #define AR5K_EEPROM_N_2GHZ_CHAN_2413	4
>  #define	AR5K_EEPROM_N_2GHZ_CHAN_MAX	4

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/8/4 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi all,
>
> After several experiments on cm9 and dcma82 cards, I figured out that
> this patch does not solve the max_power calibration problem as intended.
>
> The partly reduction to 8 with this:
> #define AR5K_EEPROM_N_5GHZ_CHAN         10
> #define AR5K_EEPROM_N_5GHZ_RATE_CHAN    8
>
> ... creates a wrong power curve on the card, as function
> ath5k_eeprom_read_freq_list() runs its while loop still 10 times, which
> results in wrong AR5K_EEPROM_READ(o++, val) readings, leading the card
> to use a very low power level over all.
> This should also be limited to 8, as I tested it.
>
> My suggestion is to just set:
> #define AR5K_EEPROM_N_5GHZ_CHAN         8
>
> .. without introducing a separate variable, it is not needed.
> I will send a v2.
>
> While I am browsing through /ath5k/eeprom.c there are several other
> suspicious places where 10 eeprom lines of chips like 5111, 5112, 2413
> depending on their EEPROM Version are read. I can not test this, as I
> have only CM9 and DCMA82 over here, but I guess those 10 line reads are
> also wrong. Can someone test this ?
>
>
> Greetings Thomas
>

There is nothing suspicious about it, it's what EEPROM docs say:

a) For 11a mode there are 10 calibration peers and for b/g there are 3
for chips < 2413 and 4 on later chips.
b) We first read the frequencies, if freq != 0 we use the calibration
info, else we ignore it.
c) We then read the power deltas for each calibration pier
d) Then we store that info for use by the channel power table calibration

Also what do you mean wrong curve and how did you actually test your
card and measured the tx power ?

-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Thomas Huehn]

Hi Nick,

> There is nothing suspicious about it, it's what EEPROM docs say:
> 
> a) For 11a mode there are 10 calibration peers 


As I tested AR5413, AR5414 and AR5213 there are only 8 eeprom lines for
802.11a that provide valid per rate target power levels. I am curious if
this also holds for other chips.

> Also what do you mean wrong curve and how did you actually test your
> card and measured the tx power ?
> 


My first patch only changed the read iterations in function
ath5k_eeprom_read_target_rate_pwr_info() from 10 reads to 8. Having only
this, I measured received rssi values while changing the tx_power on a
sender. I did not observe any changes in rssi, so I went back to the
code to investigate.
And the function ath5k_eeprom_init_11a_pcal_freq() (my hardware eeprom
version is > 3.3) there is ath5k_eeprom_read_freq_list(..,10,..) called,
and those 10 iterations produce 2 wrong frequency peer values. If there
are only 8 iterations used, I am able to measure a 15 dB variation in
rssi values at the receiver side, while changing the tx_power on the
sender. For the chips I tested, ath5k_eeprom_read_freq_list() should
only read 8 pears for 802.11a.

With 'suspicious' I meant e.g. function
ath5k_eeprom_init_11a_pcal_freq(), as it checks the eeprom version and
if > 3.3 it calls
ath5k_eeprom_init_11a_pcal_freq(..,AR5K_EEPROM_N_5GHZ_CHAN,..), in the
else case, it reads 10 hardcoded freq.pears .. I am questioning if this
is correct, so maybe someone has the chance to test this.


Greetings Thomas

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/8/5 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick,
>
>> There is nothing suspicious about it, it's what EEPROM docs say:
>>
>> a) For 11a mode there are 10 calibration peers
>
>
> As I tested AR5413, AR5414 and AR5213 there are only 8 eeprom lines for
> 802.11a that provide valid per rate target power levels. I am curious if
> this also holds for other chips.
>

Based on EEPROM docs I have it's 8 per rate target power levels for
all chips after 5210.

>> Also what do you mean wrong curve and how did you actually test your
>> card and measured the tx power ?
>>
>
>
> My first patch only changed the read iterations in function
> ath5k_eeprom_read_target_rate_pwr_info() from 10 reads to 8. Having only
> this, I measured received rssi values while changing the tx_power on a
> sender. I did not observe any changes in rssi, so I went back to the
> code to investigate.

Your patch is correct, that doesn't mean it 'll fix everything ;-)

To give you a picture not all vendors follow Atheros's refference
design, most of them focus on supporting the max power, not all power
levels, so their calibration curves don't provide much. This mostly
happens on chips that can have extrenal preamplifiers and components
so vendors have the freedom to do whatever they want. This doesn't
only happen on ath5k, in my tests with MadWiFi I also didn't see any
tx power changes (you can also see that on the TODO description on
wireless.kernel.org). So have that in mind when testing for tx power
changes.

I'm not saying the code is perfect or bugless I'm saying that you
can't be sure, maybe a cross-check with MadWiFi would help to
understand if it's our fault or vendor's choice.

> And the function ath5k_eeprom_init_11a_pcal_freq() (my hardware eeprom
> version is > 3.3) there is ath5k_eeprom_read_freq_list(..,10,..) called,
> and those 10 iterations produce 2 wrong frequency peer values. If there
> are only 8 iterations used, I am able to measure a 15 dB variation in
> rssi values at the receiver side, while changing the tx_power on the
> sender. For the chips I tested, ath5k_eeprom_read_freq_list() should
> only read 8 pears for 802.11a.
>
> With 'suspicious' I meant e.g. function
> ath5k_eeprom_init_11a_pcal_freq(), as it checks the eeprom version and
> if > 3.3 it calls
> ath5k_eeprom_init_11a_pcal_freq(..,AR5K_EEPROM_N_5GHZ_CHAN,..), in the
> else case, it reads 10 hardcoded freq.pears .. I am questioning if this
> is correct, so maybe someone has the chance to test this.
>

According to EEPROM docs there are 10 calibration piers on all chips
after 5111 for 11a but some of them are not used and the way to test
this is if frequency is zero. Now here is a bug in the EEPROM code
that might result the wrong reads you get, check this out:

on ath5k_eeprom_read_freq_list we do this

if (!freq1)
 break;

instead of this

if (!freq1)
 continue;

so maybe the offset at the end of this function is wrong, can you
please test if this change fixes your readings ?

You can also see that HAL code does the same:
#define NUM_11A_EEPROM_CHANNELS	10
#define NUM_11A_EEPROM_CHANNELS_2413    10

while (numPiers < NUM_11A_EEPROM_CHANNELS) {
	EEREAD(off++);
	if ((eeval & freqmask) == 0)
		break;
	freq[numPiers++] = fbin2freq(ee,
		eeval & freqmask);

	if (((eeval >> 8) & freqmask) == 0)
		break;
	freq[numPiers++] = fbin2freq(ee,
		(eeval>>8) & freqmask);
}

Some more infos on the EEPROM code:

Most of this code was written by rev-engineering when working on
ath_info tool a few years back when we didn't had any docs, all we had
were some EEPROM dumps posted on the internet (the output of the
eeprom tool that comes with Atheros's ART program). When we got
documentation and HAL's source some parts of this code were fixed but
we haven't done a complete review since then.

-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Thomas Huehn]

Hi Nick

> According to EEPROM docs there are 10 calibration piers on all chips
> after 5111 for 11a but some of them are not used and the way to test
> this is if frequency is zero. Now here is a bug in the EEPROM code
> that might result the wrong reads you get, check this out:
> 
> on ath5k_eeprom_read_freq_list we do this
> 
> if (!freq1)
>  break;
> 
> instead of this
> 
> if (!freq1)
>  continue;


That did the trick !
So reading 10 calibration piers and using the fix from above works in
terms of tx_power changes on CM9 and DCMA82 are properly used by the
hardware.

I will send a patch for that.


Greetings Thomas

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/8/5 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick
>
>> According to EEPROM docs there are 10 calibration piers on all chips
>> after 5111 for 11a but some of them are not used and the way to test
>> this is if frequency is zero. Now here is a bug in the EEPROM code
>> that might result the wrong reads you get, check this out:
>>
>> on ath5k_eeprom_read_freq_list we do this
>>
>> if (!freq1)
>>  break;
>>
>> instead of this
>>
>> if (!freq1)
>>  continue;
>
>
> That did the trick !
> So reading 10 calibration piers and using the fix from above works in
> terms of tx_power changes on CM9 and DCMA82 are properly used by the
> hardware.
>
> I will send a patch for that.
>
>
> Greetings Thomas
>

CCing Adrian in case this bug is also present on newer HAL sources on
FreeBSD etc...

Thanks for testing/patching Thomas, I 'll try to find some time to
work on EEPROM code again, I know it's a mess :P


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 1/2] ath5k: fix wrong per rate target power eeprom reads for AR5K_EEPROM_MODE_11A

[Nick Kossifidis]

2012/8/5 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick
>
>> According to EEPROM docs there are 10 calibration piers on all chips
>> after 5111 for 11a but some of them are not used and the way to test
>> this is if frequency is zero. Now here is a bug in the EEPROM code
>> that might result the wrong reads you get, check this out:
>>
>> on ath5k_eeprom_read_freq_list we do this
>>
>> if (!freq1)
>>  break;
>>
>> instead of this
>>
>> if (!freq1)
>>  continue;
>
>
> That did the trick !
> So reading 10 calibration piers and using the fix from above works in
> terms of tx_power changes on CM9 and DCMA82 are properly used by the
> hardware.
>
> I will send a patch for that.
>
>
> Greetings Thomas
>

I have some time right now so I'll re-send my patches (one of them
needs a fix anyway) together with this one to make John's job easier.
Send your EEPROM patch on top of them so that he can see it (remember
to CC him).


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

[PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Thomas Huehn]

In such cases where phy_init() function got called, tx_power is always
set to ah->ah_txpower.txp_cur_pwr which is never updated with txpower
specified by the user instead it equale max chan power and got
potentially incremented by ah_txpower.txp_offset.
In any case the card was switching to a txpower level higher that
someone specified to use. This patch fix this by introducing
ah_txpower.txp_user_pwr which holds the tx_power specified at userland.
Function ath5k_hw_txpower is restructured and uses the value at
ah_txpower.txp_user_pwr directly.

Signed-off-by: Thomas Huehn <thomas@net.t-labs.tu-berlin.de>
---
restructure of ath5k_hw_txpower as suggested by Felix Fietkau.
---
 drivers/net/wireless/ath/ath5k/ath5k.h |  1 +
 drivers/net/wireless/ath/ath5k/base.c  |  3 +++
 drivers/net/wireless/ath/ath5k/phy.c   | 27 ++++++++++++++-------------
 3 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 64a453a..89d9ac54 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1418,6 +1418,7 @@ struct ath5k_hw {
 		s16		txp_min_pwr;
 		s16		txp_max_pwr;
 		s16		txp_cur_pwr;
+		s16		txp_user_pwr;
 		/* Values in 0.5dB units */
 		s16		txp_offset;
 		s16		txp_ofdm;
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 8c4c040..e831f69 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2951,6 +2951,9 @@ ath5k_init(struct ieee80211_hw *hw)
 		hw->queues = 1;
 	}
 
+	/* init tx_power setting to maximum */
+	ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER;
+
 	tasklet_init(&ah->rxtq, ath5k_tasklet_rx, (unsigned long)ah);
 	tasklet_init(&ah->txtq, ath5k_tasklet_tx, (unsigned long)ah);
 	tasklet_init(&ah->beacontq, ath5k_tasklet_beacon, (unsigned long)ah);
diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 8b71a2d..728ff07 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -3583,14 +3583,12 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
  * ath5k_hw_txpower() - Set transmission power limit for a given channel
  * @ah: The &struct ath5k_hw
  * @channel: The &struct ieee80211_channel
- * @txpower: Requested tx power in 0.5dB steps
  *
  * Combines all of the above to set the requested tx power limit
- * on hw.
+ * on hw to ah->ah_txpower.txp_user_pwr.
  */
 static int
-ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
-		 u8 txpower)
+ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel)
 {
 	struct ath5k_rate_pcal_info rate_info;
 	struct ieee80211_channel *curr_channel = ah->ah_current_channel;
@@ -3598,11 +3596,6 @@ ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
 	u8 type;
 	int ret;
 
-	if (txpower > AR5K_TUNE_MAX_TXPOWER) {
-		ATH5K_ERR(ah, "invalid tx power: %u\n", txpower);
-		return -EINVAL;
-	}
-
 	ee_mode = ath5k_eeprom_mode_from_channel(channel);
 	if (ee_mode < 0) {
 		ATH5K_ERR(ah,
@@ -3667,7 +3660,7 @@ ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
 	ath5k_get_rate_pcal_data(ah, channel, &rate_info);
 
 	/* Setup rate power table */
-	ath5k_setup_rate_powertable(ah, txpower, &rate_info, ee_mode);
+	ath5k_setup_rate_powertable(ah, ah->ah_txpower.txp_user_pwr, &rate_info, ee_mode);
 
 	/* Write rate power table on hw */
 	ath5k_hw_reg_write(ah, AR5K_TXPOWER_OFDM(3, 24) |
@@ -3717,8 +3710,16 @@ ath5k_hw_set_txpower_limit(struct ath5k_hw *ah, u8 txpower)
 {
 	ATH5K_DBG(ah, ATH5K_DEBUG_TXPOWER,
 		"changing txpower to %d\n", txpower);
+	if (txpower) {
+		ah->ah_txpower.txp_user_pwr = txpower;
+
+		if (ah->ah_txpower.txp_user_pwr > AR5K_TUNE_MAX_TXPOWER) {
+			ATH5K_ERR(ah, "invalid tx power: %u\n", ah->ah_txpower.txp_user_pwr);
+			return -EINVAL;
+		}
+	}
 
-	return ath5k_hw_txpower(ah, ah->ah_current_channel, txpower);
+	return ath5k_hw_txpower(ah, ah->ah_current_channel);
 }
 
 
@@ -3789,8 +3790,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
 	 * RF buffer settings on 5211/5212+ so that we
 	 * properly set curve indices.
 	 */
-	ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
-			ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
+	ret = ath5k_hw_txpower(ah, channel);
+
 	if (ret)
 		return ret;
 
-- 
1.7.11.1

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Bob Copeland]

On Mon, Jul 23, 2012 at 06:01:15PM +0200, Thomas Huehn wrote:
>  	/* Setup rate power table */
> -	ath5k_setup_rate_powertable(ah, txpower, &rate_info, ee_mode);
> +	ath5k_setup_rate_powertable(ah, ah->ah_txpower.txp_user_pwr, &rate_info, ee_mode);

txpower used to be .5 dB units -- is it still?

> -	ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
> -			ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
> +	ret = ath5k_hw_txpower(ah, channel);
> +

Did this / 2 move elsewhere or was it wrong before?  Is txp_cur_pwr still
used for anything?

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Thomas Huehn]

Hi Bob,

Bob Copeland schrieb:

> On Mon, Jul 23, 2012 at 06:01:15PM +0200, Thomas Huehn wrote:
>>  	/* Setup rate power table */
>> -	ath5k_setup_rate_powertable(ah, txpower, &rate_info, ee_mode);
>> +	ath5k_setup_rate_powertable(ah, ah->ah_txpower.txp_user_pwr, &rate_info, ee_mode);
> 
> txpower used to be .5 dB units -- is it still?

ah->ah_txpower.txp_user_pwr is now in 1dB units, as it is triggered from
mac802.11

>> -	ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
>> -			ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
>> +	ret = ath5k_hw_txpower(ah, channel);
>> +
> 
> Did this / 2 move elsewhere or was it wrong before?  Is txp_cur_pwr still
> used for anything?

the "/2" is not needed anymore as I also skipped the "*2" in
ath5k_hw_set_txpower_limit .. so I avoid this calculations.

Felix is goint to use txp_cur_pwr, so for now it is not used ... maybe
he can explain where it will be needed.

Greetings Thomas

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Thomas Huehn]

Hi all,

After applying this 2 patches I got the following eeprom read on an
Wistron CM9:

[   38.880000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[0].freq
= 4920, rate[0].target_power_6to24 = 36, rate[0].target_power_36 = 32,
rate[0].target_power_48 = 28, rate[0].target_power_54 = 24
[   38.896000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[1].freq
= 5040, rate[1].target_power_6to24 = 36, rate[1].target_power_36 = 32,
rate[1].target_power_48 = 28, rate[1].target_power_54 = 24
[   38.916000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[2].freq
= 5170, rate[2].target_power_6to24 = 36, rate[2].target_power_36 = 32,
rate[2].target_power_48 = 30, rate[2].target_power_54 = 26
[   38.936000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[3].freq
= 5240, rate[3].target_power_6to24 = 38, rate[3].target_power_36 = 34,
rate[3].target_power_48 = 30, rate[3].target_power_54 = 26
[   38.952000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[4].freq
= 5320, rate[4].target_power_6to24 = 38, rate[4].target_power_36 = 34,
rate[4].target_power_48 = 30, rate[4].target_power_54 = 26
[   38.972000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[5].freq
= 5500, rate[5].target_power_6to24 = 38, rate[5].target_power_36 = 32,
rate[5].target_power_48 = 28, rate[5].target_power_54 = 26
[   38.992000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[6].freq
= 5700, rate[6].target_power_6to24 = 36, rate[6].target_power_36 = 32,
rate[6].target_power_48 = 28, rate[6].target_power_54 = 26
[   39.012000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[7].freq
= 5825, rate[7].target_power_6to24 = 34, rate[7].target_power_36 = 30,
rate[7].target_power_48 = 26, rate[7].target_power_54 = 24
[   39.036000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[0].freq
= 2412, rate[0].target_power_6to24 = 39, rate[0].target_power_36 = 39,
rate[0].target_power_48 = 39, rate[0].target_power_54 = 39
[   39.056000] ath5k: phy0: [bluse ath5k_eeprom] rate_pcal_info[1].freq
= 2484, rate[1].target_power_6to24 = 38, rate[1].target_power_36 = 38,
rate[1].target_power_48 = 38, rate[1].target_power_54 = 38

The 5 GHz values look ok for me, but is the 2,4 GHz amp really that
linear to support all powers at all rates ?

Greetings Thomas

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Bob Copeland]

On Mon, Jul 23, 2012 at 08:25:26PM +0200, Thomas Huehn wrote:
> > Did this / 2 move elsewhere or was it wrong before?  Is txp_cur_pwr still
> > used for anything?
> 
> the "/2" is not needed anymore as I also skipped the "*2" in
> ath5k_hw_set_txpower_limit .. so I avoid this calculations.
> 
> Felix is goint to use txp_cur_pwr, so for now it is not used ... maybe
> he can explain where it will be needed.

Ok great, thanks for the clarification.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis]

2012/7/23 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> In such cases where phy_init() function got called, tx_power is always
> set to ah->ah_txpower.txp_cur_pwr which is never updated with txpower
> specified by the user instead it equale max chan power and got
> potentially incremented by ah_txpower.txp_offset.

Never updated ?

It gets updated when setting the rate powertable (the final step of
the whole process):

ath5k_setup_rate_powertable:
3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];

> In any case the card was switching to a txpower level higher that
> someone specified to use. This patch fix this by introducing
> ah_txpower.txp_user_pwr which holds the tx_power specified at userland.
> Function ath5k_hw_txpower is restructured and uses the value at
>  directly.
>
> Signed-off-by: Thomas Huehn <thomas@net.t-labs.tu-berlin.de>
> ---
> restructure of ath5k_hw_txpower as suggested by Felix Fietkau.
> ---
>  drivers/net/wireless/ath/ath5k/ath5k.h |  1 +
>  drivers/net/wireless/ath/ath5k/base.c  |  3 +++
>  drivers/net/wireless/ath/ath5k/phy.c   | 27 ++++++++++++++-------------
>  3 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
> index 64a453a..89d9ac54 100644
> --- a/drivers/net/wireless/ath/ath5k/ath5k.h
> +++ b/drivers/net/wireless/ath/ath5k/ath5k.h
> @@ -1418,6 +1418,7 @@ struct ath5k_hw {
>                 s16             txp_min_pwr;
>                 s16             txp_max_pwr;
>                 s16             txp_cur_pwr;
> +               s16             txp_user_pwr;
>                 /* Values in 0.5dB units */
>                 s16             txp_offset;
>                 s16             txp_ofdm;

We already have
1334         int                     power_level;    /* Requested tx
power in dBm */

and
s16             txp_cur_pwr;

What's the reason to use another one ? If you want a different
type/name just change power_pevel.

We use power_level on ath5k_config when someone updates txpower:

209         if ((changed & IEEE80211_CONF_CHANGE_POWER) &&
210         (ah->power_level != conf->power_level)) {
211                 ah->power_level = conf->power_level;
212
213                 /* Half dB steps */
214                 ath5k_hw_set_txpower_limit(ah, (conf->power_level * 2));
215         }

And on each descriptor (it's ignored for now by hardware since we
haven't enabled TPC):

723         ret = ah->ah_setup_tx_desc(ah, ds, pktlen,
724                 ieee80211_get_hdrlen_from_skb(skb), padsize,
725                 get_hw_packet_type(skb),
726                 (ah->power_level * 2),
727                 hw_rate,
728                 info->control.rates[0].count, keyidx, ah->ah_tx_ant, flags,
729                 cts_rate, duration);


We use txp_cur_pwr when setting txpower level on ath5k_setup_rate_powertable

3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];

as I mentioned above, and in ath5k_hw_phy_init to preserve this
setting across resets

3792         ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
3793                         ah->ah_txpower.txp_cur_pwr / 2 :
AR5K_TUNE_MAX_TXPOWER);

To give you an idea why we already have 2 places to keep tx power
setting, base.c/mac80211-ops.c should use power_level thats in 1db
steps (and makes sense to someone reading the driver part and doesn't
want to understand why we use 0.5 and 0.25db steps on the hw
functions), and ah->ah_txpower.txp_cur_pwr on phy.c functions
internally.

I find this to be clean and simple and I don't see any problem with
it, if I did some cleanup that would be maybe to change power_level to
ah_tx_power_level and from int to s16 to match the rest but other than
that I think it's O.K. the way it is..

> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 8c4c040..e831f69 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -2951,6 +2951,9 @@ ath5k_init(struct ieee80211_hw *hw)
>                 hw->queues = 1;
>         }
>
> +       /* init tx_power setting to maximum */
> +       ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER;
> +
>         tasklet_init(&ah->rxtq, ath5k_tasklet_rx, (unsigned long)ah);
>         tasklet_init(&ah->txtq, ath5k_tasklet_tx, (unsigned long)ah);
>         tasklet_init(&ah->beacontq, ath5k_tasklet_beacon, (unsigned long)ah);

No need to do that, if txpower is not initialized we use max power
anyway on phy_init

3792         ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
3793                         ah->ah_txpower.txp_cur_pwr / 2 :
AR5K_TUNE_MAX_TXPOWER);

> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
> index 8b71a2d..728ff07 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -3583,14 +3583,12 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
>   * ath5k_hw_txpower() - Set transmission power limit for a given channel
>   * @ah: The &struct ath5k_hw
>   * @channel: The &struct ieee80211_channel
> - * @txpower: Requested tx power in 0.5dB steps
>   *
>   * Combines all of the above to set the requested tx power limit
> - * on hw.
> + * on hw to ah->ah_txpower.txp_user_pwr.
>   */
>  static int
> -ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
> -                u8 txpower)
> +ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel)
>  {
>         struct ath5k_rate_pcal_info rate_info;
>         struct ieee80211_channel *curr_channel = ah->ah_current_channel;
> @@ -3598,11 +3596,6 @@ ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
>         u8 type;
>         int ret;
>
> -       if (txpower > AR5K_TUNE_MAX_TXPOWER) {
> -               ATH5K_ERR(ah, "invalid tx power: %u\n", txpower);
> -               return -EINVAL;
> -       }
> -
>         ee_mode = ath5k_eeprom_mode_from_channel(channel);
>         if (ee_mode < 0) {
>                 ATH5K_ERR(ah,
> @@ -3667,7 +3660,7 @@ ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
>         ath5k_get_rate_pcal_data(ah, channel, &rate_info);
>
>         /* Setup rate power table */
> -       ath5k_setup_rate_powertable(ah, txpower, &rate_info, ee_mode);
> +       ath5k_setup_rate_powertable(ah, ah->ah_txpower.txp_user_pwr, &rate_info, ee_mode);
>

txpower here comes from above in 0.5db steps as the kerneldoc says,
that's what ath5k_setup_rate_powertable expects, by passing
ah->ah_txpower.txp_user_pwr directly to  ath5k_setup_rate_powertable,
it would result getting half the power requested because hw expects
this to be an index to the channel powertable (check the
implementation of ath5k_setup_rate_powertable) that uses 0.5db steps.

>         /* Write rate power table on hw */
>         ath5k_hw_reg_write(ah, AR5K_TXPOWER_OFDM(3, 24) |
> @@ -3717,8 +3710,16 @@ ath5k_hw_set_txpower_limit(struct ath5k_hw *ah, u8 txpower)
>  {
>         ATH5K_DBG(ah, ATH5K_DEBUG_TXPOWER,
>                 "changing txpower to %d\n", txpower);
> +       if (txpower) {
> +               ah->ah_txpower.txp_user_pwr = txpower;
> +
> +               if (ah->ah_txpower.txp_user_pwr > AR5K_TUNE_MAX_TXPOWER) {
> +                       ATH5K_ERR(ah, "invalid tx power: %u\n", ah->ah_txpower.txp_user_pwr);
> +                       return -EINVAL;
> +               }
> +       }
>

Again this would result getting half the power requested, you should
*2 this if you want it to be used directly by
ath5k_setup_rate_powertable later.

> -       return ath5k_hw_txpower(ah, ah->ah_current_channel, txpower);
> +       return ath5k_hw_txpower(ah, ah->ah_current_channel);
>  }
>
>
> @@ -3789,8 +3790,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
>          * RF buffer settings on 5211/5212+ so that we
>          * properly set curve indices.
>          */
> -       ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
> -                       ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
> +       ret = ath5k_hw_txpower(ah, channel);
> +
>         if (ret)
>                 return ret;
>
> --
> 1.7.11.1
>

To summarize IMHO this patch doesn't fix anything and breaks current
txpower setting so from my point of view it's a NACK.

-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Thomas Huehn]

Hi Nick,

Nick Kossifidis schrieb:

> 2012/7/23 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>> In such cases where phy_init() function got called, tx_power is always
>> set to ah->ah_txpower.txp_cur_pwr which is never updated with txpower
>> specified by the user instead it equale max chan power and got
>> potentially incremented by ah_txpower.txp_offset.
> 
> Never updated ?
> 
> It gets updated when setting the rate powertable (the final step of
> the whole process):
> 


As it took a while that I got my head into the function calls, let me
try to explain what is going wrong in the current state of ath5k
regarding tx_power.

In short: ah->ah_txpower.txp_cur_pwr is probably intended to respect the
power level set by the user, BUT it is jut reused and changed in
different places that make it difficult to follow and brake the tx_power
level specified.

-long-version-in-code:

- assume you configure your device to use 17 dBm and max power is 19dBm
and you are just in the boot process

-in the last steps to init you Atheros Wifi, the mac802.11 calls
hw_config, as it recognise a change in tx-power from maximum(19dBm) to
user_txpower(17dBm)

-this leads in case of ath5k to call the ops representative function
ath5k_config... which transforms 1 dBm steps in 2 half-dBm steps ...
concrete: ath5k_hw_set_txpower_limit(..34 half-dBm..) is called which
calls ath5k_hw_txpower(..34 half-dBm..) .. and in here .. just before
writing the power to registers.. it calls
ath5k_setup_rate_powertable(..34 half-dBm..)

- now lets jump into ath5k_setup_rate_powertable(..34 half-dBm..):
our 34 half-dBm is now transformed to 68 quarter-dBm stored in max_pwr.
max_pwr is re-assigned as to be the minimum of (our user_txpower 68
quarter_dBm and ah->ah_txpower.txp_max_pwr) / 2 ... so it is set back to
34 half_dBm

-one step further:
rates[0..5] = min(max_pwr (34 half dBm), 		rate_info->target_power_6to24
(38 half-dBm);

-now it gets interesting:
for i [1..16] rates[i] = rates[i] + OFFSET 	(line 3560)
{
.. and OFFSET is in my case 18 .. so rates[0] = 34+21 = 55
}
ah->ah_txpower.txp_cur_pwr = 2 * rates[0] = 2 * 55 half_dBm = 110 half_dBm

+BREAK: ah->ah_txpower.txp_cur_pwr is at 110 half_dBm but not yet used.
+in ALL cases where ath5k_hw_phy_init() is called ... queue reset, wifi
up/down, iwconfig set same txpower ..  we reuse this wrong set txpower
variable which does not represent the user setting

-now our ah->ah_txpower.txp_cur_pwr is just reused in the function
ath5k_hw_phy_init():
ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr (110
half_dBm)?
	ah->ah_txpower.txp_cur_pwr / 2 (55 half_dBm): AR5K_TUNE_MAX_TXPOWER (63
half_dBm));

.. which leads to the call ath5k_hw_txpower(..AR5K_TUNE_MAX_TXPOWER (63
half-dBm)..)

.. to its end ... txpower is changed from 17 dBm to the maximum. So the
OFFSET summation is the cause of a wrong ah->ah_txpower.txp_cur_pwr
usage. My path fixes exactly that and keeps ah->ah_txpower.txp_cur_pwr
unused for Felix :)

Greetings Thomas

Re: [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis]

2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick,
>
> Nick Kossifidis schrieb:
>
>> 2012/7/23 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>> In such cases where phy_init() function got called, tx_power is always
>>> set to ah->ah_txpower.txp_cur_pwr which is never updated with txpower
>>> specified by the user instead it equale max chan power and got
>>> potentially incremented by ah_txpower.txp_offset.
>>
>> Never updated ?
>>
>> It gets updated when setting the rate powertable (the final step of
>> the whole process):
>>
>
>
> As it took a while that I got my head into the function calls, let me
> try to explain what is going wrong in the current state of ath5k
> regarding tx_power.
>
> In short: ah->ah_txpower.txp_cur_pwr is probably intended to respect the
> power level set by the user, BUT it is jut reused and changed in
> different places that make it difficult to follow and brake the tx_power
> level specified.
>
> -long-version-in-code:
>
> - assume you configure your device to use 17 dBm and max power is 19dBm
> and you are just in the boot process
>
> -in the last steps to init you Atheros Wifi, the mac802.11 calls
> hw_config, as it recognise a change in tx-power from maximum(19dBm) to
> user_txpower(17dBm)
>
> -this leads in case of ath5k to call the ops representative function
> ath5k_config... which transforms 1 dBm steps in 2 half-dBm steps ...
> concrete: ath5k_hw_set_txpower_limit(..34 half-dBm..) is called which
> calls ath5k_hw_txpower(..34 half-dBm..) .. and in here .. just before
> writing the power to registers.. it calls
> ath5k_setup_rate_powertable(..34 half-dBm..)
>
> - now lets jump into ath5k_setup_rate_powertable(..34 half-dBm..):
> our 34 half-dBm is now transformed to 68 quarter-dBm stored in max_pwr.
> max_pwr is re-assigned as to be the minimum of (our user_txpower 68
> quarter_dBm and ah->ah_txpower.txp_max_pwr) / 2 ... so it is set back to
> 34 half_dBm
>
> -one step further:
> rates[0..5] = min(max_pwr (34 half dBm),                rate_info->target_power_6to24
> (38 half-dBm);
>
> -now it gets interesting:
> for i [1..16] rates[i] = rates[i] + OFFSET      (line 3560)
> {
> .. and OFFSET is in my case 18 .. so rates[0] = 34+21 = 55
> }
> ah->ah_txpower.txp_cur_pwr = 2 * rates[0] = 2 * 55 half_dBm = 110 half_dBm
>
> +BREAK: ah->ah_txpower.txp_cur_pwr is at 110 half_dBm but not yet used.
> +in ALL cases where ath5k_hw_phy_init() is called ... queue reset, wifi
> up/down, iwconfig set same txpower ..  we reuse this wrong set txpower
> variable which does not represent the user setting
>
> -now our ah->ah_txpower.txp_cur_pwr is just reused in the function
> ath5k_hw_phy_init():
> ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr (110
> half_dBm)?
>         ah->ah_txpower.txp_cur_pwr / 2 (55 half_dBm): AR5K_TUNE_MAX_TXPOWER (63
> half_dBm));
>
> .. which leads to the call ath5k_hw_txpower(..AR5K_TUNE_MAX_TXPOWER (63
> half-dBm)..)
>
> .. to its end ... txpower is changed from 17 dBm to the maximum. So the
> OFFSET summation is the cause of a wrong ah->ah_txpower.txp_cur_pwr
> usage. My path fixes exactly that and keeps ah->ah_txpower.txp_cur_pwr
> unused for Felix :)
>
> Greetings Thomas
>
>

There is nothing in your patch that suggests that's related to this.
Anyway there's a simple way to fix this:

Just move this:

3575         /* Min/max in 0.25dB units */
3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
3578         ah->ah_txpower.txp_ofdm = rates[7];

above the for loop and you are done.

Note rates[i] don't hold tx power values, they hold indices to the
channel powertable.

-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Thomas Huehn]

Hi Nick,

Nick Kossifidis schrieb:

> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:

> There is nothing in your patch that suggests that's related to this.
> Anyway there's a simple way to fix this:
> 
> Just move this:
> 
> 3575         /* Min/max in 0.25dB units */
> 3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
> 3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
> 3578         ah->ah_txpower.txp_ofdm = rates[7];
> 
> above the for loop and you are done.
> 
> Note rates[i] don't hold tx power values, they hold indices to the
> channel powertable.
> 


Are you agreeing that current ath5k txpower handling set from user space
is not working and we need to fix it ?
Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
on a channel witch other max power levels ?

Lets wait on Felix opinion what he has in mind.

Greetings Thomas

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis]

2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick,
>
> Nick Kossifidis schrieb:
>
>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>
>> There is nothing in your patch that suggests that's related to this.
>> Anyway there's a simple way to fix this:
>>
>> Just move this:
>>
>> 3575         /* Min/max in 0.25dB units */
>> 3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>> 3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>> 3578         ah->ah_txpower.txp_ofdm = rates[7];
>>
>> above the for loop and you are done.
>>
>> Note rates[i] don't hold tx power values, they hold indices to the
>> channel powertable.
>>
>
>
> Are you agreeing that current ath5k txpower handling set from user space
> is not working and we need to fix it ?

Is that a rhetorical question ? Just check out the TODOs on wireless.kernel.org.

The current code does not preserve the tx power value across resets,
thats the problem and the change I mentioned above fixes it (patch on
the way, it's just that where I am right now I don't have bw to
download wireless-testing) but other than that when we set tx power
without reseting at least it does what it's supposed to do (and the
result is the same with madwifi, using a spectrum analyzer or another
client/monitor interface you see some power levels supported or only
the max txpower supported, it's really up to the vendor, not all of
them follow Atheros's reference designs). Your patch passes 1dbm units
on a function that expects 0.5dbm units, you fix the problem with
preserving tx power but you break the tx power setting.

The change I mentioned above fixes the problem without introducing new
variables just because "Felix will use the other one", I don't
understand why you have a problem with that and why you think I don't
want this to get fixed...

> Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
> on a channel witch other max power levels ?

That won't be a problem, when channel changes we 'll call

reset -> phy_init -> set_txpower -> (calibration) -> set rate target power

and then it's going to get limited before it's written on hw here:

max_pwr = min(max_pwr, (u16) ah->ah_txpower.txp_max_pwr) / 2;

txp_max_pwr is initialized on calibration (the max power for this
channel), then it gets limited by CTL edge information on EEPROM, then
by max_pwr and then max_pwr is limited by rate_info->target_power_X
from EEPROM to create rates[i]. We write rates[i] on hw, not
txp_cur_pwr.

> Lets wait on Felix opinion what he has in mind.
>
> Greetings Thomas

I 'd rather wait for Felix's patches instead to understand what is he up to.


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Felix Fietkau]

On 2012-07-26 12:20 PM, Nick Kossifidis wrote:
> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>> Hi Nick,
>>
>> Nick Kossifidis schrieb:
>>
>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>
>>> There is nothing in your patch that suggests that's related to this.
>>> Anyway there's a simple way to fix this:
>>>
>>> Just move this:
>>>
>>> 3575         /* Min/max in 0.25dB units */
>>> 3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>>> 3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>>> 3578         ah->ah_txpower.txp_ofdm = rates[7];
>>>
>>> above the for loop and you are done.
>>>
>>> Note rates[i] don't hold tx power values, they hold indices to the
>>> channel powertable.
>>>
>>
>>
>> Are you agreeing that current ath5k txpower handling set from user space
>> is not working and we need to fix it ?
> 
> Is that a rhetorical question ? Just check out the TODOs on wireless.kernel.org.
> 
> The current code does not preserve the tx power value across resets,
> thats the problem and the change I mentioned above fixes it (patch on
> the way, it's just that where I am right now I don't have bw to
> download wireless-testing) but other than that when we set tx power
> without reseting at least it does what it's supposed to do (and the
> result is the same with madwifi, using a spectrum analyzer or another
> client/monitor interface you see some power levels supported or only
> the max txpower supported, it's really up to the vendor, not all of
> them follow Atheros's reference designs). Your patch passes 1dbm units
> on a function that expects 0.5dbm units, you fix the problem with
> preserving tx power but you break the tx power setting.
> 
> The change I mentioned above fixes the problem without introducing new
> variables just because "Felix will use the other one", I don't
> understand why you have a problem with that and why you think I don't
> want this to get fixed...
> 
>> Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
>> on a channel witch other max power levels ?
> 
> That won't be a problem, when channel changes we 'll call
> 
> reset -> phy_init -> set_txpower -> (calibration) -> set rate target power
> 
> and then it's going to get limited before it's written on hw here:
> 
> max_pwr = min(max_pwr, (u16) ah->ah_txpower.txp_max_pwr) / 2;
> 
> txp_max_pwr is initialized on calibration (the max power for this
> channel), then it gets limited by CTL edge information on EEPROM, then
> by max_pwr and then max_pwr is limited by rate_info->target_power_X
> from EEPROM to create rates[i]. We write rates[i] on hw, not
> txp_cur_pwr.
I think it's a bad idea to store the user's choice of txpower in a
variable that internally gets reused to store the hw limit. Even when
the offset isn't added to it, it's still fragile.

A problem with this is that different channels have different max power
values, so if you switch to a channel with a lower power, and then
switch back (without explicitly changing txpower inbetween), don't you
then end up with less power than you configured?

This can be easily avoided by storing the user's txpower choice
separately from the actual hw limit...

- Felix

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis]

2012/7/26 Felix Fietkau <nbd@openwrt.org>:
> On 2012-07-26 12:20 PM, Nick Kossifidis wrote:
>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>> Hi Nick,
>>>
>>> Nick Kossifidis schrieb:
>>>
>>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>
>>>> There is nothing in your patch that suggests that's related to this.
>>>> Anyway there's a simple way to fix this:
>>>>
>>>> Just move this:
>>>>
>>>> 3575         /* Min/max in 0.25dB units */
>>>> 3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>>>> 3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>>>> 3578         ah->ah_txpower.txp_ofdm = rates[7];
>>>>
>>>> above the for loop and you are done.
>>>>
>>>> Note rates[i] don't hold tx power values, they hold indices to the
>>>> channel powertable.
>>>>
>>>
>>>
>>> Are you agreeing that current ath5k txpower handling set from user space
>>> is not working and we need to fix it ?
>>
>> Is that a rhetorical question ? Just check out the TODOs on wireless.kernel.org.
>>
>> The current code does not preserve the tx power value across resets,
>> thats the problem and the change I mentioned above fixes it (patch on
>> the way, it's just that where I am right now I don't have bw to
>> download wireless-testing) but other than that when we set tx power
>> without reseting at least it does what it's supposed to do (and the
>> result is the same with madwifi, using a spectrum analyzer or another
>> client/monitor interface you see some power levels supported or only
>> the max txpower supported, it's really up to the vendor, not all of
>> them follow Atheros's reference designs). Your patch passes 1dbm units
>> on a function that expects 0.5dbm units, you fix the problem with
>> preserving tx power but you break the tx power setting.
>>
>> The change I mentioned above fixes the problem without introducing new
>> variables just because "Felix will use the other one", I don't
>> understand why you have a problem with that and why you think I don't
>> want this to get fixed...
>>
>>> Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
>>> on a channel witch other max power levels ?
>>
>> That won't be a problem, when channel changes we 'll call
>>
>> reset -> phy_init -> set_txpower -> (calibration) -> set rate target power
>>
>> and then it's going to get limited before it's written on hw here:
>>
>> max_pwr = min(max_pwr, (u16) ah->ah_txpower.txp_max_pwr) / 2;
>>
>> txp_max_pwr is initialized on calibration (the max power for this
>> channel), then it gets limited by CTL edge information on EEPROM, then
>> by max_pwr and then max_pwr is limited by rate_info->target_power_X
>> from EEPROM to create rates[i]. We write rates[i] on hw, not
>> txp_cur_pwr.
> I think it's a bad idea to store the user's choice of txpower in a
> variable that internally gets reused to store the hw limit. Even when
> the offset isn't added to it, it's still fragile.
>
> A problem with this is that different channels have different max power
> values, so if you switch to a channel with a lower power, and then
> switch back (without explicitly changing txpower inbetween), don't you
> then end up with less power than you configured?
>
> This can be easily avoided by storing the user's txpower choice
> separately from the actual hw limit...
>
> - Felix

That's what ah->power_level already does, what's the point of
introducing another one. Do you think power_level is baldy
used/implemented ?


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Felix Fietkau]

On 2012-07-26 12:31 PM, Nick Kossifidis wrote:
> 2012/7/26 Felix Fietkau <nbd@openwrt.org>:
>> On 2012-07-26 12:20 PM, Nick Kossifidis wrote:
>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>> Hi Nick,
>>>>
>>>> Nick Kossifidis schrieb:
>>>>
>>>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>>
>>>>> There is nothing in your patch that suggests that's related to this.
>>>>> Anyway there's a simple way to fix this:
>>>>>
>>>>> Just move this:
>>>>>
>>>>> 3575         /* Min/max in 0.25dB units */
>>>>> 3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>>>>> 3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>>>>> 3578         ah->ah_txpower.txp_ofdm = rates[7];
>>>>>
>>>>> above the for loop and you are done.
>>>>>
>>>>> Note rates[i] don't hold tx power values, they hold indices to the
>>>>> channel powertable.
>>>>>
>>>>
>>>>
>>>> Are you agreeing that current ath5k txpower handling set from user space
>>>> is not working and we need to fix it ?
>>>
>>> Is that a rhetorical question ? Just check out the TODOs on wireless.kernel.org.
>>>
>>> The current code does not preserve the tx power value across resets,
>>> thats the problem and the change I mentioned above fixes it (patch on
>>> the way, it's just that where I am right now I don't have bw to
>>> download wireless-testing) but other than that when we set tx power
>>> without reseting at least it does what it's supposed to do (and the
>>> result is the same with madwifi, using a spectrum analyzer or another
>>> client/monitor interface you see some power levels supported or only
>>> the max txpower supported, it's really up to the vendor, not all of
>>> them follow Atheros's reference designs). Your patch passes 1dbm units
>>> on a function that expects 0.5dbm units, you fix the problem with
>>> preserving tx power but you break the tx power setting.
>>>
>>> The change I mentioned above fixes the problem without introducing new
>>> variables just because "Felix will use the other one", I don't
>>> understand why you have a problem with that and why you think I don't
>>> want this to get fixed...
>>>
>>>> Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
>>>> on a channel witch other max power levels ?
>>>
>>> That won't be a problem, when channel changes we 'll call
>>>
>>> reset -> phy_init -> set_txpower -> (calibration) -> set rate target power
>>>
>>> and then it's going to get limited before it's written on hw here:
>>>
>>> max_pwr = min(max_pwr, (u16) ah->ah_txpower.txp_max_pwr) / 2;
>>>
>>> txp_max_pwr is initialized on calibration (the max power for this
>>> channel), then it gets limited by CTL edge information on EEPROM, then
>>> by max_pwr and then max_pwr is limited by rate_info->target_power_X
>>> from EEPROM to create rates[i]. We write rates[i] on hw, not
>>> txp_cur_pwr.
>> I think it's a bad idea to store the user's choice of txpower in a
>> variable that internally gets reused to store the hw limit. Even when
>> the offset isn't added to it, it's still fragile.
>>
>> A problem with this is that different channels have different max power
>> values, so if you switch to a channel with a lower power, and then
>> switch back (without explicitly changing txpower inbetween), don't you
>> then end up with less power than you configured?
>>
>> This can be easily avoided by storing the user's txpower choice
>> separately from the actual hw limit...
>>
>> - Felix
> 
> That's what ah->power_level already does, what's the point of
> introducing another one. Do you think power_level is baldy
> used/implemented ?
Unless I'm missing something here, it does not seem to get used at reset
time, only when mac80211 requests a txpower change (and for TPC values
in descriptors).
On every reset, ah->ah_txpower.txp_cur_pwr gets reused as configured tx
power value (and subsequently clamped to the hw channel power limit).
I guess this patch could be simplified to use ah->power_level in
ath5k_hw_phy_init instead of ah->ah_txpower.txp_cur_pwr, but I do see an
issue in the code as it is without this patch.

- Felix

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis]

2012/7/26 Felix Fietkau <nbd@openwrt.org>:
> On 2012-07-26 12:31 PM, Nick Kossifidis wrote:
>> 2012/7/26 Felix Fietkau <nbd@openwrt.org>:
>>> On 2012-07-26 12:20 PM, Nick Kossifidis wrote:
>>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>>> Hi Nick,
>>>>>
>>>>> Nick Kossifidis schrieb:
>>>>>
>>>>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>>>
>>>>>> There is nothing in your patch that suggests that's related to this.
>>>>>> Anyway there's a simple way to fix this:
>>>>>>
>>>>>> Just move this:
>>>>>>
>>>>>> 3575         /* Min/max in 0.25dB units */
>>>>>> 3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>>>>>> 3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>>>>>> 3578         ah->ah_txpower.txp_ofdm = rates[7];
>>>>>>
>>>>>> above the for loop and you are done.
>>>>>>
>>>>>> Note rates[i] don't hold tx power values, they hold indices to the
>>>>>> channel powertable.
>>>>>>
>>>>>
>>>>>
>>>>> Are you agreeing that current ath5k txpower handling set from user space
>>>>> is not working and we need to fix it ?
>>>>
>>>> Is that a rhetorical question ? Just check out the TODOs on wireless.kernel.org.
>>>>
>>>> The current code does not preserve the tx power value across resets,
>>>> thats the problem and the change I mentioned above fixes it (patch on
>>>> the way, it's just that where I am right now I don't have bw to
>>>> download wireless-testing) but other than that when we set tx power
>>>> without reseting at least it does what it's supposed to do (and the
>>>> result is the same with madwifi, using a spectrum analyzer or another
>>>> client/monitor interface you see some power levels supported or only
>>>> the max txpower supported, it's really up to the vendor, not all of
>>>> them follow Atheros's reference designs). Your patch passes 1dbm units
>>>> on a function that expects 0.5dbm units, you fix the problem with
>>>> preserving tx power but you break the tx power setting.
>>>>
>>>> The change I mentioned above fixes the problem without introducing new
>>>> variables just because "Felix will use the other one", I don't
>>>> understand why you have a problem with that and why you think I don't
>>>> want this to get fixed...
>>>>
>>>>> Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
>>>>> on a channel witch other max power levels ?
>>>>
>>>> That won't be a problem, when channel changes we 'll call
>>>>
>>>> reset -> phy_init -> set_txpower -> (calibration) -> set rate target power
>>>>
>>>> and then it's going to get limited before it's written on hw here:
>>>>
>>>> max_pwr = min(max_pwr, (u16) ah->ah_txpower.txp_max_pwr) / 2;
>>>>
>>>> txp_max_pwr is initialized on calibration (the max power for this
>>>> channel), then it gets limited by CTL edge information on EEPROM, then
>>>> by max_pwr and then max_pwr is limited by rate_info->target_power_X
>>>> from EEPROM to create rates[i]. We write rates[i] on hw, not
>>>> txp_cur_pwr.
>>> I think it's a bad idea to store the user's choice of txpower in a
>>> variable that internally gets reused to store the hw limit. Even when
>>> the offset isn't added to it, it's still fragile.
>>>
>>> A problem with this is that different channels have different max power
>>> values, so if you switch to a channel with a lower power, and then
>>> switch back (without explicitly changing txpower inbetween), don't you
>>> then end up with less power than you configured?
>>>
>>> This can be easily avoided by storing the user's txpower choice
>>> separately from the actual hw limit...
>>>
>>> - Felix
>>
>> That's what ah->power_level already does, what's the point of
>> introducing another one. Do you think power_level is baldy
>> used/implemented ?
> Unless I'm missing something here, it does not seem to get used at reset
> time, only when mac80211 requests a txpower change (and for TPC values
> in descriptors).
> On every reset, ah->ah_txpower.txp_cur_pwr gets reused as configured tx
> power value (and subsequently clamped to the hw channel power limit).
> I guess this patch could be simplified to use ah->power_level in
> ath5k_hw_phy_init instead of ah->ah_txpower.txp_cur_pwr, but I do see an
> issue in the code as it is without this patch.
>
> - Felix
>

I think this is a better approach (I'll prepare a proper patch as soon
as I have some bandwidth to work with wireless-testing, maybe
tomorrow)...

--- old/phy.c	2012-07-26 20:40:00.869150187 +0300
+++ new/phy.c	2012-07-26 20:43:25.074710577 +0300
@@ -3562,6 +3562,12 @@
 		for (i = 8; i <= 15; i++)
 			rates[i] -= ah->ah_txpower.txp_cck_ofdm_gainf_delta;

+
+	/* Min/max in 0.25dB units */
+	ah->ah_txpower.txp_min_pwr = 2 * rates[7];
+	ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
+	ah->ah_txpower.txp_ofdm = rates[7];
+
 	/* Now that we have all rates setup use table offset to
 	 * match the power range set by user with the power indices
 	 * on PCDAC/PDADC table */
@@ -3571,11 +3577,6 @@
 		if (rates[i] > 63)
 			rates[i] = 63;
 	}
-
-	/* Min/max in 0.25dB units */
-	ah->ah_txpower.txp_min_pwr = 2 * rates[7];
-	ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
-	ah->ah_txpower.txp_ofdm = rates[7];
 }


@@ -3789,8 +3790,8 @@
 	 * RF buffer settings on 5211/5212+ so that we
 	 * properly set curve indices.
 	 */
-	ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
-			ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
+	ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
+			ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
 	if (ret)
 		return ret;

Works for you ?

BTW is there a way to pass the actual tx power set back to
mac80211/cfg80211 so that user knows what power his card is actually
transmitting at ?

-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Thomas Huehn]

Hi Nick,

Nick Kossifidis schrieb:

> I think this is a better approach (I'll prepare a proper patch as soon
> as I have some bandwidth to work with wireless-testing, maybe
> tomorrow)...
> 
> --- old/phy.c	2012-07-26 20:40:00.869150187 +0300
> +++ new/phy.c	2012-07-26 20:43:25.074710577 +0300
> @@ -3562,6 +3562,12 @@
>  		for (i = 8; i <= 15; i++)
>  			rates[i] -= ah->ah_txpower.txp_cck_ofdm_gainf_delta;
> 
> +
> +	/* Min/max in 0.25dB units */
> +	ah->ah_txpower.txp_min_pwr = 2 * rates[7];
> +	ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
> +	ah->ah_txpower.txp_ofdm = rates[7];
> +
>  	/* Now that we have all rates setup use table offset to
>  	 * match the power range set by user with the power indices
>  	 * on PCDAC/PDADC table */
> @@ -3571,11 +3577,6 @@
>  		if (rates[i] > 63)
>  			rates[i] = 63;
>  	}
> -
> -	/* Min/max in 0.25dB units */
> -	ah->ah_txpower.txp_min_pwr = 2 * rates[7];
> -	ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
> -	ah->ah_txpower.txp_ofdm = rates[7];
>  }
> 

> @@ -3789,8 +3790,8 @@
>  	 * RF buffer settings on 5211/5212+ so that we
>  	 * properly set curve indices.
>  	 */
> -	ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
> -			ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
> +	ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
> +			ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
>  	if (ret)
>  		return ret;
> 
> Works for you ?

works as well.

There are now 2 unused variables as left over:
ah->ah_txpower.txp_cur_pwr
ah->ah_txpower.txp_min_pwr

Do we need them anymore to check for hw chan limit ?

> BTW is there a way to pass the actual tx power set back to
> mac80211/cfg80211 so that user knows what power his card is actually
> transmitting at ?
 

I think that on point of the todo list.

Greetings Thomas

Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis]

2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
> Hi Nick,
>
> Nick Kossifidis schrieb:
>
>> I think this is a better approach (I'll prepare a proper patch as soon
>> as I have some bandwidth to work with wireless-testing, maybe
>> tomorrow)...
>>
>> --- old/phy.c 2012-07-26 20:40:00.869150187 +0300
>> +++ new/phy.c 2012-07-26 20:43:25.074710577 +0300
>> @@ -3562,6 +3562,12 @@
>>               for (i = 8; i <= 15; i++)
>>                       rates[i] -= ah->ah_txpower.txp_cck_ofdm_gainf_delta;
>>
>> +
>> +     /* Min/max in 0.25dB units */
>> +     ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>> +     ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>> +     ah->ah_txpower.txp_ofdm = rates[7];
>> +
>>       /* Now that we have all rates setup use table offset to
>>        * match the power range set by user with the power indices
>>        * on PCDAC/PDADC table */
>> @@ -3571,11 +3577,6 @@
>>               if (rates[i] > 63)
>>                       rates[i] = 63;
>>       }
>> -
>> -     /* Min/max in 0.25dB units */
>> -     ah->ah_txpower.txp_min_pwr = 2 * rates[7];
>> -     ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
>> -     ah->ah_txpower.txp_ofdm = rates[7];
>>  }
>>
>
>> @@ -3789,8 +3790,8 @@
>>        * RF buffer settings on 5211/5212+ so that we
>>        * properly set curve indices.
>>        */
>> -     ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
>> -                     ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
>> +     ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
>> +                     ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
>>       if (ret)
>>               return ret;
>>
>> Works for you ?
>
> works as well.
>
> There are now 2 unused variables as left over:
> ah->ah_txpower.txp_cur_pwr
> ah->ah_txpower.txp_min_pwr
>
> Do we need them anymore to check for hw chan limit ?
>
>> BTW is there a way to pass the actual tx power set back to
>> mac80211/cfg80211 so that user knows what power his card is actually
>> transmitting at ?
>
>
> I think that on point of the todo list.
>

That's what txp_cur_pwr will be used for, min_pwr is left there. I'll
do a cleanup too sometime :-)



-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick