| Message ID | 20240909073049.3423035-1-quic_santrame@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: ath12k: fix tx power, max reg power update to firmware | expand |
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'
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 :)
"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" <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.
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>
Santhosh Ramesh <quic_santrame@quicinc.com> wrote: > 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> Acked-by: Kalle Valo <kvalo@kernel.org>
On Fri Sep 20, 2024 at 6:43 AM CEST, Santhosh Ramesh wrote: > "Nicolas Escande" <nico.escande@gmail.com> writes: [...] > > > > 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. Hey Santhosh, Unless I'm mistaken (which is very much possible) I didn't see any patch posted to fix that. Weren't you guys supposed to have done it by now ? > > > > 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. Thanks
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;