On 7/19/25 1: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. What I'm saying is that you should put this information in the commit message > >>> >>> 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. The comment style. > > 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. This is not conclusive either. Does the firmware of the bluetooth chip configure the pin, or is the reset pin connected to an always-on PCIe supply or similar? > > >>> + * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared. At a glance, they're only cleared in qca_setup(), regardless of their state >>> + * 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 Please write it down somewhere Konrad
