Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <56cfae0e-27b6-2527-fe76-1d0061804903@quicinc.com>
Date:   Mon, 25 Sep 2023 18:50:19 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v6 13/13] wifi: ath11k: send TPC power to firmware for 6
 GHz station
Content-Language: en-US
To:     Aditya Kumar Singh <quic_adisi@quicinc.com>,
        <ath11k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>, <kvalo@kernel.org>,
        <quic_jjohnson@quicinc.com>
References: <20230920082349.29111-1-quic_wgong@quicinc.com>
 <20230920082349.29111-14-quic_wgong@quicinc.com>
 <eb08e0f2-c932-4d79-b2ee-813c2999d1a5@quicinc.com>
 <fbad2af6-9c3f-c241-b820-7820b4200bf4@quicinc.com>
 <60aaa1fc-99cf-476c-af51-e5ad425792f9@quicinc.com>
 <145e9534-6610-9c11-b2b9-87fb8e50ef81@quicinc.com>
 <1c1391b6-9710-44c6-97b2-68ef23409c1b@quicinc.com>
From:   Wen Gong <quic_wgong@quicinc.com>
In-Reply-To: <1c1391b6-9710-44c6-97b2-68ef23409c1b@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 9/25/2023 1:41 PM, Aditya Kumar Singh wrote:
> On 9/25/23 07:45, Wen Gong wrote:
>> On 9/22/2023 9:25 PM, Aditya Kumar Singh wrote:
>>> On 9/22/23 15:42, Wen Gong wrote:
>>>> On 9/22/2023 5:24 PM, Aditya Kumar Singh wrote:
>>>>> On 9/20/23 13:53, Wen Gong wrote:
>>>>>> When station is connected to a 6 GHz AP, it has 2 way to configure
>>>>>> the power limit to firmware. The first way is to send 2 wmi command
>>>>>> WMI_PDEV_PARAM_TXPOWER_LIMIT2G/WMI_PDEV_PARAM_TXPOWER_LIMIT5G to
>>>>>> firmware, the second way is to send WMI_VDEV_SET_TPC_POWER_CMDID to
>>>>>> firmware which include more parameters for power control.
>>>>>>
>>>>>> The first way is disabled in previous patch
>>>>>> "ath11k: discard BSS_CHANGED_TXPOWER when EXT_TPC_REG_SUPPORT for 
>>>>>> 6 GHz".
>>>>>>
>>>>>> Prepare the parameter for wmi command 
>>>>>> WMI_VDEV_SET_TPC_POWER_CMDID and
>>>>>> send the firmware after vdev start response success from 
>>>>>> firmware, it
>>>>>> is for the second way of power control.
>>>>>>
>>>>>> Tested-on: WCN6855 hw2.0 PCI 
>>>>>> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
>>>>>>
>>>>>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
>>>>>> ---
>>>>>>   drivers/net/wireless/ath/ath11k/mac.c | 8 +++++++-
>>>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/ath/ath11k/mac.c 
>>>>>> b/drivers/net/wireless/ath/ath11k/mac.c
>>>>>> index a8ae281d2635..f8b907a758b1 100644
>>>>>> --- a/drivers/net/wireless/ath/ath11k/mac.c
>>>>>> +++ b/drivers/net/wireless/ath/ath11k/mac.c
>>>>>> @@ -7296,6 +7296,12 @@ ath11k_mac_vdev_start_restart(struct 
>>>>>> ath11k_vif *arvif,
>>>>>>           return ret;
>>>>>>       }
>>>>>>   +    if (ath11k_mac_supports_station_tpc(ar, arvif, chandef)) {
>>>>>> +        ath11k_mac_fill_reg_tpc_info(ar, arvif->vif, 
>>>>>> &arvif->chanctx);
>>>>> So we are passing local copy of channel context stored in 
>>>>> arvif->chanctx. Do we need to update it when channel changes?
>>>>>
>>>>> I see that during assignment time, we are copying/updating it and 
>>>>> accordingly the command will be sent to firmware, but what about 
>>>>> when STA moves channel? arvif->chanctx should be updated and tpc 
>>>>> command should be sent again in that case?
>>>>
>>>> This has been discussed before here per question of Johannes:"Could 
>>>> this information change? Should we track it in beacons?":
>>>>
>>>> [PATCH 9/9] mac80211: save transmit power envelope element and 
>>>> power constraint
>>>>
>>>> https://lore.kernel.org/linux-wireless/38e7d9d2eebafa7245a36a0a0396094526eb3efd.camel@sipsolutions.net/ 
>>>>
>>> That's fine. That's w.r.t to TX power change. I'm saying here about 
>>> CSA? What when AP tries to switch channel? For that client need not 
>>> disassociate and associate back right?
>>>
>>> In that case, channel context in mac80211 layer will change. But our 
>>> driver's arvif->chanctx will have previous one only. We are using 
>>> channel context to get the ieee80211_channel which has the PSD 
>>> value, and that value we are sending to firmware via TPC command 
>>> during intial association time. So when channel changes, firmware 
>>> also should be updated with the latest PSD values via TPC command 
>>> for the latest channel right?
>>>
>> You are right. CSA may be change channel bandwidth.
>>
>> Currently we could keep NOT support CSA as well as CSA for MLO since 
>> these are the basic TPC support feature.
>>
>> We could make new patch later to support CSA fro TPC power, OK?
> Not supporting w.r.t MLO CSA is okay. That's WIP. But for non-MLO 
> case, I think it should be supported. And if not, then may be 
> explicitly mention it somewhere as _TODO_ ? What if by the time patch 
> is sent for that, meanwhile a bug is filed?
I will add _TODO_ next version as you said.
>
> Anyways, I will let maintainers take a call on this. I don't have any 
> further comments.
>