diff mbox series

wifi: ath12k: fix tx power, max reg power update to firmware

Message ID 20240909073049.3423035-1-quic_santrame@quicinc.com (mailing list archive)
State New
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: fix tx power, max reg power update to firmware | expand

Commit Message

Santhosh Ramesh Sept. 9, 2024, 7:30 a.m. UTC 
From: Sathishkumar Muruganandam <quic_murugana@quicinc.com>

Currently, when the vdev start WMI cmd is sent from host, vdev related
parameters such as max_reg_power, max_power, and max_antenna_gain are
multiplied by 2 before being sent to the firmware. This is incorrect
because the firmware uses 1 dBm steps for power calculations.

This leads to incorrect power values being used in the firmware and
radio, potentially causing incorrect behavior.

Fix the update of max_reg_power, max_power, and max_antenna_gain values
in the ath12k_mac_vdev_start_restart function, ensuring accurate
power settings in the firmware by sending these values as-is,
without multiplication.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00214-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Sathishkumar Muruganandam <quic_murugana@quicinc.com>
Signed-off-by: Santhosh Ramesh <quic_santrame@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: 903aaf66edc97dd5b9e3118d19677291051a9c40

Comments

Nicolas Escande Sept. 9, 2024, 8:20 a.m. UTC | #1 
On Mon Sep 9, 2024 at 9:30 AM CEST, Santhosh Ramesh wrote:

[...]

> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 137394c36460..6d7a005d83b4 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
>  							chandef->chan->band,
>  							arvif->vif->type);
>  	arg.min_power = 0;
> -	arg.max_power = chandef->chan->max_power * 2;
> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
> +	arg.max_power = chandef->chan->max_power;
> +	arg.max_reg_power = chandef->chan->max_reg_power;
> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
>  

For what it's worth, I already got a similar patch from QCA a few month ago, and
it fixed some tx power problems when scanning so I know there is truth in this.

>  	arg.pref_tx_streams = ar->num_tx_chains;
>  	arg.pref_rx_streams = ar->num_rx_chains;
>
> base-commit: 903aaf66edc97dd5b9e3118d19677291051a9c40

But there also was more in the patch:

diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index a97e1399eeed..87e3ae49afa5 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -2563,6 +2563,8 @@ int ath12k_wmi_send_scan_chan_list_cmd(struct ath12k *ar,
 						  WMI_CHAN_REG_INFO1_REG_CLS);
 			*reg2 |= le32_encode_bits(channel_arg->antennamax,
 						  WMI_CHAN_REG_INFO2_ANT_MAX);
+			*reg2 |= le32_encode_bits(channel_arg->maxregpower,
+						  WMI_CHAN_REG_INFO2_MAX_TX_PWR);
 
 			ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
 				   "WMI chan scan list chan[%d] = %u, chan_info->info %8x\n",

It seems that ath11k has a similar piece of code in the equivalent function
ath11k_wmi_send_scan_chan_list_cmd . Isn't this needed ?

Also I see that in ath12k/reg.c in ath12k_reg_update_chan_list we also send to
the firmware:
			ch->maxpower = channel->max_power * 2;
			ch->maxregpower = channel->max_reg_power * 2;
			ch->antennamax = channel->max_antenna_gain * 2;

As it is in a different wmi message, I guess the units are different. 0.5 dBm
increment in struct ath12k_wmi_channel_arg right ? Or should we remove the '*2'
Kalle Valo Sept. 9, 2024, 9:07 a.m. UTC | #2 
"Nicolas Escande" <nico.escande@gmail.com> writes:

> On Mon Sep 9, 2024 at 9:30 AM CEST, Santhosh Ramesh wrote:
>
> [...]
>
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index 137394c36460..6d7a005d83b4 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
>>  							chandef->chan->band,
>>  							arvif->vif->type);
>>  	arg.min_power = 0;
>> -	arg.max_power = chandef->chan->max_power * 2;
>> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
>> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
>> +	arg.max_power = chandef->chan->max_power;
>> +	arg.max_reg_power = chandef->chan->max_reg_power;
>> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
>>  
>
> For what it's worth, I already got a similar patch from QCA a few month ago, and
> it fixed some tx power problems when scanning so I know there is truth in this.

What hardware and what firmware did you use? The firmware branches can
behave differently so whenever testing something it's good to include
that info.
Nicolas Escande Sept. 9, 2024, 10:23 a.m. UTC | #3 
On Mon Sep 9, 2024 at 11:07 AM CEST, Kalle Valo wrote:
> "Nicolas Escande" <nico.escande@gmail.com> writes:
>
> > On Mon Sep 9, 2024 at 9:30 AM CEST, Santhosh Ramesh wrote:
> >
> > [...]
> >
> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> >> index 137394c36460..6d7a005d83b4 100644
> >> --- a/drivers/net/wireless/ath/ath12k/mac.c
> >> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> >> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
> >>  							chandef->chan->band,
> >>  							arvif->vif->type);
> >>  	arg.min_power = 0;
> >> -	arg.max_power = chandef->chan->max_power * 2;
> >> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
> >> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
> >> +	arg.max_power = chandef->chan->max_power;
> >> +	arg.max_reg_power = chandef->chan->max_reg_power;
> >> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
> >>  
> >
> > For what it's worth, I already got a similar patch from QCA a few month ago, and
> > it fixed some tx power problems when scanning so I know there is truth in this.
>
> What hardware and what firmware did you use? The firmware branches can
> behave differently so whenever testing something it's good to include
> that info.

Well what I tested was with the additionnal patch provided. So depending on
Santhosh response, I'll either give the proprer tested by tag, or if the patch
isn't needed. I'll need to find some time to test it :)
Kalle Valo Sept. 9, 2024, 11:15 a.m. UTC | #4 
"Nicolas Escande" <nico.escande@gmail.com> writes:

> On Mon Sep 9, 2024 at 11:07 AM CEST, Kalle Valo wrote:
>
>> "Nicolas Escande" <nico.escande@gmail.com> writes:
>>
>> > On Mon Sep 9, 2024 at 9:30 AM CEST, Santhosh Ramesh wrote:
>> >
>> > [...]
>> >
>> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> >> index 137394c36460..6d7a005d83b4 100644
>> >> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> >> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> >> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
>> >>  							chandef->chan->band,
>> >>  							arvif->vif->type);
>> >>  	arg.min_power = 0;
>> >> -	arg.max_power = chandef->chan->max_power * 2;
>> >> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
>> >> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
>> >> +	arg.max_power = chandef->chan->max_power;
>> >> +	arg.max_reg_power = chandef->chan->max_reg_power;
>> >> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
>> >>  
>> >
>> > For what it's worth, I already got a similar patch from QCA a few month ago, and
>> > it fixed some tx power problems when scanning so I know there is truth in this.
>>
>> What hardware and what firmware did you use? The firmware branches can
>> behave differently so whenever testing something it's good to include
>> that info.
>
> Well what I tested was with the additionnal patch provided. So depending on
> Santhosh response, I'll either give the proprer tested by tag, or if the patch
> isn't needed. I'll need to find some time to test it :)

Can you say what hardware (QCN9274 or WCN7850) you are using? I'm not
asking for Tested-on tag, just some extra information to help the
review.
Nicolas Escande Sept. 9, 2024, 12:55 p.m. UTC | #5 
On Mon Sep 9, 2024 at 1:15 PM CEST, Kalle Valo wrote:
> "Nicolas Escande" <nico.escande@gmail.com> writes:
>
> > On Mon Sep 9, 2024 at 11:07 AM CEST, Kalle Valo wrote:
> >
> >> "Nicolas Escande" <nico.escande@gmail.com> writes:
> >>
> >> > On Mon Sep 9, 2024 at 9:30 AM CEST, Santhosh Ramesh wrote:
> >> >
> >> > [...]
> >> >
> >> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> >> >> index 137394c36460..6d7a005d83b4 100644
> >> >> --- a/drivers/net/wireless/ath/ath12k/mac.c
> >> >> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> >> >> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
> >> >>  							chandef->chan->band,
> >> >>  							arvif->vif->type);
> >> >>  	arg.min_power = 0;
> >> >> -	arg.max_power = chandef->chan->max_power * 2;
> >> >> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
> >> >> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
> >> >> +	arg.max_power = chandef->chan->max_power;
> >> >> +	arg.max_reg_power = chandef->chan->max_reg_power;
> >> >> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
> >> >>  
> >> >
> >> > For what it's worth, I already got a similar patch from QCA a few month ago, and
> >> > it fixed some tx power problems when scanning so I know there is truth in this.
> >>
> >> What hardware and what firmware did you use? The firmware branches can
> >> behave differently so whenever testing something it's good to include
> >> that info.
> >
> > Well what I tested was with the additionnal patch provided. So depending on
> > Santhosh response, I'll either give the proprer tested by tag, or if the patch
> > isn't needed. I'll need to find some time to test it :)
>
> Can you say what hardware (QCN9274 or WCN7850) you are using? I'm not
> asking for Tested-on tag, just some extra information to help the
> review.

Sure, I tested it with a QCN9274, in 2*2x2 (2.4GHz + 5GHz) in station mode with
a special firmware WLAN.WBE.1.2.1-00148.1-QCAHKSWPL_SILICONZ-8.
Without this, all probe requests sent by the firwmare during a scan (both bands)
were send with around 15/20 dBm less power that the data frames.
Santhosh Ramesh Sept. 20, 2024, 4:43 a.m. UTC | #6 
"Nicolas Escande" <nico.escande@gmail.com> writes:
> On Mon Sep 9, 2024 at 9:30 AM CEST, Santhosh Ramesh wrote:
> 
> [...]
> 
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index 137394c36460..6d7a005d83b4 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
>>  							chandef->chan->band,
>>  							arvif->vif->type);
>>  	arg.min_power = 0;
>> -	arg.max_power = chandef->chan->max_power * 2;
>> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
>> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
>> +	arg.max_power = chandef->chan->max_power;
>> +	arg.max_reg_power = chandef->chan->max_reg_power;
>> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
>>  
> 
> For what it's worth, I already got a similar patch from QCA a few month ago, and
> it fixed some tx power problems when scanning so I know there is truth in this.
> 
>>  	arg.pref_tx_streams = ar->num_tx_chains;
>>  	arg.pref_rx_streams = ar->num_rx_chains;
>>
>> base-commit: 903aaf66edc97dd5b9e3118d19677291051a9c40
> 
> But there also was more in the patch:
> 
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index a97e1399eeed..87e3ae49afa5 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -2563,6 +2563,8 @@ int ath12k_wmi_send_scan_chan_list_cmd(struct ath12k *ar,
>  						  WMI_CHAN_REG_INFO1_REG_CLS);
>  			*reg2 |= le32_encode_bits(channel_arg->antennamax,
>  						  WMI_CHAN_REG_INFO2_ANT_MAX);
> +			*reg2 |= le32_encode_bits(channel_arg->maxregpower,
> +						  WMI_CHAN_REG_INFO2_MAX_TX_PWR);
>  
>  			ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
>  				   "WMI chan scan list chan[%d] = %u, chan_info->info %8x\n",
> 
> It seems that ath11k has a similar piece of code in the equivalent function
> ath11k_wmi_send_scan_chan_list_cmd . Isn't this needed ?

This change is in a separate patch, which will be sent for public review soon.

> 
> Also I see that in ath12k/reg.c in ath12k_reg_update_chan_list we also send to
> the firmware:
> 			ch->maxpower = channel->max_power * 2;
> 			ch->maxregpower = channel->max_reg_power * 2;
> 			ch->antennamax = channel->max_antenna_gain * 2;
> 
> As it is in a different wmi message, I guess the units are different. 0.5 dBm
> increment in struct ath12k_wmi_channel_arg right ? Or should we remove the '*2'
> 

Yes, the firmware expects these values in 0.5 dBm increments, so we multiply 
them by 2 in the ath12k_reg_update_chan_list function.
Nicolas Escande Sept. 20, 2024, 7:15 a.m. UTC | #7 
On Fri Sep 20, 2024 at 6:43 AM CEST, Santhosh Ramesh wrote:
[...]
> > 
> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> >> index 137394c36460..6d7a005d83b4 100644
> >> --- a/drivers/net/wireless/ath/ath12k/mac.c
> >> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> >> @@ -7217,9 +7217,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
> >>  							chandef->chan->band,
> >>  							arvif->vif->type);
> >>  	arg.min_power = 0;
> >> -	arg.max_power = chandef->chan->max_power * 2;
> >> -	arg.max_reg_power = chandef->chan->max_reg_power * 2;
> >> -	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
> >> +	arg.max_power = chandef->chan->max_power;
> >> +	arg.max_reg_power = chandef->chan->max_reg_power;
> >> +	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
> >>  
> > 
> > For what it's worth, I already got a similar patch from QCA a few month ago, and
> > it fixed some tx power problems when scanning so I know there is truth in this.
> > 
> >>  	arg.pref_tx_streams = ar->num_tx_chains;
> >>  	arg.pref_rx_streams = ar->num_rx_chains;
> >>
> >> base-commit: 903aaf66edc97dd5b9e3118d19677291051a9c40
> > 
> > But there also was more in the patch:
> > 
> > diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> > index a97e1399eeed..87e3ae49afa5 100644
> > --- a/drivers/net/wireless/ath/ath12k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> > @@ -2563,6 +2563,8 @@ int ath12k_wmi_send_scan_chan_list_cmd(struct ath12k *ar,
> >  						  WMI_CHAN_REG_INFO1_REG_CLS);
> >  			*reg2 |= le32_encode_bits(channel_arg->antennamax,
> >  						  WMI_CHAN_REG_INFO2_ANT_MAX);
> > +			*reg2 |= le32_encode_bits(channel_arg->maxregpower,
> > +						  WMI_CHAN_REG_INFO2_MAX_TX_PWR);
> >  
> >  			ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
> >  				   "WMI chan scan list chan[%d] = %u, chan_info->info %8x\n",
> > 
> > It seems that ath11k has a similar piece of code in the equivalent function
> > ath11k_wmi_send_scan_chan_list_cmd . Isn't this needed ?
>
> This change is in a separate patch, which will be sent for public review soon.

OK

>
> > 
> > Also I see that in ath12k/reg.c in ath12k_reg_update_chan_list we also send to
> > the firmware:
> > 			ch->maxpower = channel->max_power * 2;
> > 			ch->maxregpower = channel->max_reg_power * 2;
> > 			ch->antennamax = channel->max_antenna_gain * 2;
> > 
> > As it is in a different wmi message, I guess the units are different. 0.5 dBm
> > increment in struct ath12k_wmi_channel_arg right ? Or should we remove the '*2'
> > 
>
> Yes, the firmware expects these values in 0.5 dBm increments, so we multiply 
> them by 2 in the ath12k_reg_update_chan_list function.
Good

Then good news this gets mainlined by the way, but it's a shame it takes this
long (like 10 months or so...)

Reviewed-by: Nicolas Escande <nico.escande@gmail.com>
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 137394c36460..6d7a005d83b4 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7217,9 +7217,9 @@  ath12k_mac_vdev_start_restart(struct ath12k_vif *arvif,
 							chandef->chan->band,
 							arvif->vif->type);
 	arg.min_power = 0;
-	arg.max_power = chandef->chan->max_power * 2;
-	arg.max_reg_power = chandef->chan->max_reg_power * 2;
-	arg.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
+	arg.max_power = chandef->chan->max_power;
+	arg.max_reg_power = chandef->chan->max_reg_power;
+	arg.max_antenna_gain = chandef->chan->max_antenna_gain;
 
 	arg.pref_tx_streams = ar->num_tx_chains;
 	arg.pref_rx_streams = ar->num_rx_chains;