On 8/12/25 10:03 AM, Shuai Zhang wrote:
> Hi Konrad
>
> On 7/19/2025 7:32 AM, Shuai Zhang wrote:
>> Hi Konrad
>>
>> On 7/15/2025 5:11 PM, Konrad Dybcio wrote:
>>> On 7/15/25 7:16 AM, Shuai Zhang wrote:
>>>> the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>>> This leads to reset command timeout.
>>>
>>> This is a description of what goes wrong in terms of the code of
>>> this driver, and it doesn't explain why you gate the code addition
>>> with HCI_QUIRK_NON_PERSISTENT_SETUP, please share more details about
>>> what you're doing, and more importantly, why.
>>>
>>
>> The problem encountered is that when the host actively triggers ssr
>> and collects the coredump data, the bt will send a reset command to
>> the controller. However, due to the aforementioned flag not being set,
>> the reset command times out.
>>
>> I'm not clear whether you want to ask about the function of
>> HCI_QUIRK_NON_PERSISTENT_SETUP or why the changes are placed
>> under if(!HCI_QUIRK_NON_PERSISTENT_SETUP).
>>
>> Regarding the purpose of HCI_QUIRK_NON_PERSISTENT_SETUP,
>> you can refer to this commit. 740011cfe94859df8d05f5400d589a8693b095e7
>>
>> As for why it's placed in if(!HCI_QUIRK_NON_PERSISTENT_SETUP),
>> since HCI_QUIRK_NON_PERSISTENT_SETUP is related to BT_EN, it can be
>> used to determine if BT_EN exists in the DTS.
>>
>>>>
>>>> Signed-off-by: Shuai Zhang <quic_shuaz@xxxxxxxxxxx>
>>>> ---
>>>> drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>> index 4e56782b0..791f8d472 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>>> skb_queue_purge(&qca->rx_memdump_q);
>>>> }
>>>>
>>>> + /* If the SoC always enables the bt_en pin via hardware and the driver
>>>> + * cannot control the bt_en pin of the SoC chip, then during SSR,
>>>
>>> What is the "SoC" here? Bluetooth chip? MSM?
>>
>> yes, Bluetooth chip on qcs9075-evk platform
>>
>>>
>>> What does "enabling the pin via hardware" refer to? Do we ever expect
>>> that a proper platform description skips the bt_en pin?
>>>
>>> Also:
>>>
>>> /*
>>> * If the..
>>>
>>
>> Sorry, I’m not quite sure I follow—could you clarify what you meant?
>> Here is my understanding.
>>
>> Enabling pins through hardware refers to "the pin is pulled up by hardware".
>> qcs9075-evk platform use the m.2 connective card, the bt_en always pull up.
>>
>>
>>>> + * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>>> + * This leads to a reset command timeout failure.
>>>> + * Also, add msleep delay to wait for controller to complete SSR.
>>>
>>> Googling "bluetooth SSR" yields nothing, so it's fair for me to ask
>>> you to explain that acronym.. it's used a number of times across the
>>> driver, so perhaps a comment somewhere at the top in a separate commit
>>> would be good as well. I'm guessing "subsystem reset"?
>>
>> Just to clarify, SSR is short for Subsystem Restart
>>
>>>
>>> Konrad
>>>
>>>> + */
>>>> + if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>> + clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>>>> + clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>>> + msleep(50);
>>>> + }
>>>> +
>>>> clear_bit(QCA_HW_ERROR_EVENT, &qca->flags);
>>>> }
>>>>
>>
>> Shuai
>>
>
> Please let me know if there are any updates. Thank you.
You're expected to address the review comments in a subsequent patchset
revision, in this case please put the answers to the questions I asked
in the commit message, or in the comments, so that someone else can
make sense of the change
Konrad
