Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3751-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <fe1a0e3b-3408-4a33-90e9-d4ffcfc7a99b@quicinc.com>
Date: Fri, 19 Apr 2024 07:24:31 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 2/2] Bluetooth: qca: Fix BT enable failure for
 QCA_QCA6390 after disable then warm reboot
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <luiz.dentz@gmail.com>, <luiz.von.dentz@intel.com>,
        <marcel@holtmann.org>
CC: <linux-bluetooth@vger.kernel.org>, <wt@penguintechs.org>,
        <bartosz.golaszewski@linaro.org>
References: <1713449192-25926-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713449192-25926-3-git-send-email-quic_zijuhu@quicinc.com>
 <7cd6f395-1f1b-4a73-840f-d70fff5da220@linaro.org>
 <8751981b-3063-4228-bbec-f36bd544e0fb@quicinc.com>
 <93af3308-d70f-4423-a911-0f437f882462@linaro.org>
 <f4b93d58-56a2-4fa1-88aa-7d5dfb8dcb0e@quicinc.com>
 <c29003b6-5344-43e0-a9e9-c7f4b76e1f1a@linaro.org>
Content-Language: en-US
From: quic_zijuhu <quic_zijuhu@quicinc.com>
In-Reply-To: <c29003b6-5344-43e0-a9e9-c7f4b76e1f1a@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/19/2024 6:38 AM, Krzysztof Kozlowski wrote:
> On 19/04/2024 00:05, quic_zijuhu wrote:
>> On 4/19/2024 4:58 AM, Krzysztof Kozlowski wrote:
>>> On 18/04/2024 22:34, quic_zijuhu wrote:
>>>> On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
>>>>> On 18/04/2024 16:06, Zijun Hu wrote:
>>>>>> From: Zijun Hu <zijuhu@qti.qualcomm.com>
>>>>>>
>>>>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>>>>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>>>>>> not configured by DT or ACPI, the regression issue is that BT can't be
>>>>>> enabled after disable then warm reboot, fixed by correcting conditions
>>>>>> for sending the VSC reset controller within qca_serdev_shutdown().
>>>>>
>>>>> I have trouble understanding what is the bug. Can you rephrase it?
>>>>>
>>>> Think about below sequences:
>>>> cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
>>>> BT is failed to be enabled after warm reboot.
>>>
>>> Still not. Please get someone to help you rephrase it. One long sentence
>>> is not good approach. Describe the problem, how it can be reproduced and
>>> then come with brief explanation how you fixed it (because it is not
>>> obvious to me).
>>>
>> thanks for your suggestions. will optimize commit message based on your suggestions.
>>>>>>
>>>>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Signed-off-by: Zijun Hu <zijuhu@qti.qualcomm.com>
>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>>> ---
>>>>>>  drivers/bluetooth/hci_qca.c | 12 +++++++++---
>>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>> index 160175a23a49..2a47a60ecc25 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
>>>>>>  	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>>>>>>  	struct hci_uart *hu = &qcadev->serdev_hu;
>>>>>>  	struct hci_dev *hdev = hu->hdev;
>>>>>> -	struct qca_data *qca = hu->priv;
>>>>>>  	const u8 ibs_wake_cmd[] = { 0xFD };
>>>>>>  	const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>>>>>  
>>>>>>  	if (qcadev->btsoc_type == QCA_QCA6390) {
>>>>>> -		if (test_bit(QCA_BT_OFF, &qca->flags) ||
>>>>>> -		    !test_bit(HCI_RUNNING, &hdev->flags))
>>>>>> +		if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>>>> +			BT_INFO("QCA do not need to send EDL_RESET_REQ");
>>>>>>  			return;
>>>>>> +		}
>>>>>> +
>>>>>> +		if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>>>>
>>>>> Commit msg does not help me at all to understand why you are changing
>>>>> the test bits.
>>>> it is test bits not changing bits.
>>>
>>> Previously we tested bits for BT off and HCI running. Now not, so you
>>> changed the logic. Maybe it is correct, maybe not, I don't understand why.
>>>
>> if HCI_QUIRK_NON_PERSISTENT_SETUP is set, it means we can and need to do initialization
>> by calling hdev->setup() for every BT enable,  so we don't need to send VSC to reset controller
>> here.
>>
>> if HCI_QUIRK_NON_PERSISTENT_SETUP is cleared. it means we only can or need to do initialization
>> for the first BT enable operation. if HCI_SETUP is set, that means we don't do any BT enable yet
>> and the initialization operations are never performed. so we also don't need to send VSC any more.
>>
>> otherwise, we need to send VSC to reset controller since initialization have been Done regardless of
>> BT state. for this case the serdev have still be opened. so it also don't meet the crash the 272970be3dab
>> fixed.
> 
> Please read again what I request here: your change is not obvious and is
> not explained in commit msg.
> 
> 
i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and HCI_SETUP is generic flag.
> 
> 
> Best regards,
> Krzysztof
>