* [PATCH v2 0/3] Work around missing MSA_READY indicator for msm8998 devices @ 2024-03-28 17:24 Marc Gonzalez 2024-03-28 17:36 ` [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop Marc Gonzalez ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Marc Gonzalez @ 2024-03-28 17:24 UTC (permalink / raw) To: Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo Work around missing MSA_READY indicator in ath10k driver (apply work-around for all msm8998 devices) Marc Gonzalez (3): dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop wifi: ath10k: fake missing MSA_READY indicator arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml | 8 ++++++++ arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 + drivers/net/wireless/ath/ath10k/qmi.c | 7 +++++++ drivers/net/wireless/ath/ath10k/qmi.h | 1 + 4 files changed, 17 insertions(+) ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop 2024-03-28 17:24 [PATCH v2 0/3] Work around missing MSA_READY indicator for msm8998 devices Marc Gonzalez @ 2024-03-28 17:36 ` Marc Gonzalez 2024-03-30 18:20 ` Krzysztof Kozlowski 2024-03-28 17:38 ` [PATCH v2 2/3] wifi: ath10k: fake missing MSA_READY indicator Marc Gonzalez 2024-03-28 17:39 ` [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi Marc Gonzalez 2 siblings, 1 reply; 31+ messages in thread From: Marc Gonzalez @ 2024-03-28 17:36 UTC (permalink / raw) To: Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo The ath10k driver waits for an "MSA_READY" indicator to complete initialization. If the indicator is not received, then the device remains unusable. cf. ath10k_qmi_driver_event_work() Several msm8998-based devices are affected by this issue. Oddly, it seems safe to NOT wait for the indicator, and proceed immediately when QMI_EVENT_SERVER_ARRIVE. Jeff Johnson wrote: The feedback I received was "it might be ok to change all ath10k qmi to skip waiting for msa_ready", and it was pointed out that ath11k (and ath12k) do not wait for it. However with so many deployed devices, "might be ok" isn't a strong argument for changing the default behavior. Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> --- Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml index 9b3ef4bc37325..1680604cd1b7e 100644 --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml @@ -122,6 +122,13 @@ properties: Whether to skip executing an SCM call that reassigns the memory region ownership. + qcom,no-msa-ready-indicator: + type: boolean + description: + The driver waits for an MSA_READY indicator to complete init. + If the indicator is not received, interface remains unusable. + Pretending we received the indicator works around the issue. + qcom,smem-states: $ref: /schemas/types.yaml#/definitions/phandle-array description: State bits used by the AP to signal the WLAN Q6. -- 2.34.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop 2024-03-28 17:36 ` [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop Marc Gonzalez @ 2024-03-30 18:20 ` Krzysztof Kozlowski 2024-03-30 18:23 ` Krzysztof Kozlowski 0 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2024-03-30 18:20 UTC (permalink / raw) To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo On 28/03/2024 18:36, Marc Gonzalez wrote: > The ath10k driver waits for an "MSA_READY" indicator > to complete initialization. If the indicator is not > received, then the device remains unusable. > > cf. ath10k_qmi_driver_event_work() > > Several msm8998-based devices are affected by this issue. > Oddly, it seems safe to NOT wait for the indicator, and > proceed immediately when QMI_EVENT_SERVER_ARRIVE. > This is v2, so where is the changelog? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop 2024-03-30 18:20 ` Krzysztof Kozlowski @ 2024-03-30 18:23 ` Krzysztof Kozlowski 2024-03-30 22:04 ` Marc Gonzalez 0 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2024-03-30 18:23 UTC (permalink / raw) To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo On 30/03/2024 19:20, Krzysztof Kozlowski wrote: > On 28/03/2024 18:36, Marc Gonzalez wrote: >> The ath10k driver waits for an "MSA_READY" indicator >> to complete initialization. If the indicator is not >> received, then the device remains unusable. >> >> cf. ath10k_qmi_driver_event_work() >> >> Several msm8998-based devices are affected by this issue. >> Oddly, it seems safe to NOT wait for the indicator, and >> proceed immediately when QMI_EVENT_SERVER_ARRIVE. >> > > This is v2, so where is the changelog? Expecting reviewer to dig previous discussions will not help your case. It helps reviewers if you provide necessary information, like resolution of previous discussion in the changelog. I dig the previous discussion, since you did not mention it here, and it seems you entirely ignored its outcome. That's not a DT property. NAK, sorry. Please go back to v1 and read the comments you got there. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop 2024-03-30 18:23 ` Krzysztof Kozlowski @ 2024-03-30 22:04 ` Marc Gonzalez 2024-04-03 6:44 ` Krzysztof Kozlowski 0 siblings, 1 reply; 31+ messages in thread From: Marc Gonzalez @ 2024-03-30 22:04 UTC (permalink / raw) To: Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo On 30/03/2024 19:23, Krzysztof Kozlowski wrote: > On 30/03/2024 19:20, Krzysztof Kozlowski wrote: > >> On 28/03/2024 18:36, Marc Gonzalez wrote: >> >>> The ath10k driver waits for an "MSA_READY" indicator >>> to complete initialization. If the indicator is not >>> received, then the device remains unusable. >>> >>> cf. ath10k_qmi_driver_event_work() >>> >>> Several msm8998-based devices are affected by this issue. >>> Oddly, it seems safe to NOT wait for the indicator, and >>> proceed immediately when QMI_EVENT_SERVER_ARRIVE. >> >> This is v2, so where is the changelog? > > Expecting reviewer to dig previous discussions will not help your case. > It helps reviewers if you provide necessary information, like resolution > of previous discussion in the changelog. > > I dig the previous discussion, since you did not mention it here, and it > seems you entirely ignored its outcome. That's not a DT property. > > NAK, sorry. Please go back to v1 and read the comments you got there. My apologies for omitting the changelog. And I don't blame you for missing the thread's resolution, since I made a bit of a mess of it with my various messages. The firmware-5.bin approach was deemed DOA since these files are parsed too late with respect to the required work-around. Thus, we went back to either DT or a to-be-written system used in the vendor driver. Jeff Johnson (one of the maintainers) concluded with: "But I'm OK with the DT option as well. Kalle?" Thus, I spun v2 to get Kalle's Ack, and more crucially to give a heads-up to the msm8998 users my patch would impact. Regards ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop 2024-03-30 22:04 ` Marc Gonzalez @ 2024-04-03 6:44 ` Krzysztof Kozlowski 0 siblings, 0 replies; 31+ messages in thread From: Krzysztof Kozlowski @ 2024-04-03 6:44 UTC (permalink / raw) To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo On 30/03/2024 23:04, Marc Gonzalez wrote: > On 30/03/2024 19:23, Krzysztof Kozlowski wrote: > >> On 30/03/2024 19:20, Krzysztof Kozlowski wrote: >> >>> On 28/03/2024 18:36, Marc Gonzalez wrote: >>> >>>> The ath10k driver waits for an "MSA_READY" indicator >>>> to complete initialization. If the indicator is not >>>> received, then the device remains unusable. >>>> >>>> cf. ath10k_qmi_driver_event_work() >>>> >>>> Several msm8998-based devices are affected by this issue. >>>> Oddly, it seems safe to NOT wait for the indicator, and >>>> proceed immediately when QMI_EVENT_SERVER_ARRIVE. >>> >>> This is v2, so where is the changelog? >> >> Expecting reviewer to dig previous discussions will not help your case. >> It helps reviewers if you provide necessary information, like resolution >> of previous discussion in the changelog. >> >> I dig the previous discussion, since you did not mention it here, and it >> seems you entirely ignored its outcome. That's not a DT property. >> >> NAK, sorry. Please go back to v1 and read the comments you got there. > > My apologies for omitting the changelog. > > And I don't blame you for missing the thread's resolution, > since I made a bit of a mess of it with my various messages. > > The firmware-5.bin approach was deemed DOA since these files > are parsed too late with respect to the required work-around. > Thus, we went back to either DT or a to-be-written system used > in the vendor driver. Then please mention it briefly in the commit msg. Explain there why such implementation is the correct way to solve your problem. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 2/3] wifi: ath10k: fake missing MSA_READY indicator 2024-03-28 17:24 [PATCH v2 0/3] Work around missing MSA_READY indicator for msm8998 devices Marc Gonzalez 2024-03-28 17:36 ` [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop Marc Gonzalez @ 2024-03-28 17:38 ` Marc Gonzalez 2024-03-28 17:39 ` [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi Marc Gonzalez 2 siblings, 0 replies; 31+ messages in thread From: Marc Gonzalez @ 2024-03-28 17:38 UTC (permalink / raw) To: Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo The ath10k driver waits for an "MSA_READY" indicator to complete initialization. If the indicator is not received, then the device remains unusable. Several msm8998-based devices are affected by this issue. Oddly, it seems safe to NOT wait for the indicator, and proceed immediately when QMI_EVENT_SERVER_ARRIVE. fw_version 0x100204b2 fw_build_timestamp 2019-09-04 03:01 fw_build_id QC_IMAGE_VERSION_STRING=WLAN.HL.1.0-01202-QCAHLSWMTPLZ-1.221523.2 Jeff Johnson wrote: The feedback I received was "it might be ok to change all ath10k qmi to skip waiting for msa_ready", and it was pointed out that ath11k (and ath12k) do not wait for it. However with so many deployed devices, "might be ok" isn't a strong argument for changing the default behavior. Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> --- drivers/net/wireless/ath/ath10k/qmi.c | 7 +++++++ drivers/net/wireless/ath/ath10k/qmi.h | 1 + 2 files changed, 8 insertions(+) diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 38e939f572a9e..50e28fa37e430 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -1040,6 +1040,10 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) switch (event->type) { case ATH10K_QMI_EVENT_SERVER_ARRIVE: ath10k_qmi_event_server_arrive(qmi); + if (qmi->fake_msa_ready_indicator) { + ath10k_dbg(ar, ATH10K_DBG_QMI, "qmi HACK fake msa_ready indicator"); + ath10k_qmi_event_msa_ready(qmi); + } break; case ATH10K_QMI_EVENT_SERVER_EXIT: ath10k_qmi_event_server_exit(qmi); @@ -1077,6 +1081,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size) if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm")) qmi->msa_fixed_perm = true; + if (of_property_read_bool(dev->of_node, "qcom,no-msa-ready-indicator")) + qmi->fake_msa_ready_indicator = true; + ret = qmi_handle_init(&qmi->qmi_hdl, WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN, &ath10k_qmi_ops, qmi_msg_handler); diff --git a/drivers/net/wireless/ath/ath10k/qmi.h b/drivers/net/wireless/ath/ath10k/qmi.h index 89464239fe96a..c68526aad8946 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.h +++ b/drivers/net/wireless/ath/ath10k/qmi.h @@ -107,6 +107,7 @@ struct ath10k_qmi { char fw_build_timestamp[MAX_TIMESTAMP_LEN + 1]; struct ath10k_qmi_cal_data cal_data[MAX_NUM_CAL_V01]; bool msa_fixed_perm; + bool fake_msa_ready_indicator; enum ath10k_qmi_state state; }; -- 2.34.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-03-28 17:24 [PATCH v2 0/3] Work around missing MSA_READY indicator for msm8998 devices Marc Gonzalez 2024-03-28 17:36 ` [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop Marc Gonzalez 2024-03-28 17:38 ` [PATCH v2 2/3] wifi: ath10k: fake missing MSA_READY indicator Marc Gonzalez @ 2024-03-28 17:39 ` Marc Gonzalez 2024-03-30 18:25 ` Krzysztof Kozlowski 2 siblings, 1 reply; 31+ messages in thread From: Marc Gonzalez @ 2024-03-28 17:39 UTC (permalink / raw) To: Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo The ath10k driver waits for an "MSA_READY" indicator to complete initialization. If the indicator is not received, then the device remains unusable. cf. ath10k_qmi_driver_event_work() Several msm8998-based devices are affected by this issue. Oddly, it seems safe to NOT wait for the indicator, and proceed immediately when QMI_EVENT_SERVER_ARRIVE. Jeff Johnson wrote: The feedback I received was "it might be ok to change all ath10k qmi to skip waiting for msa_ready", and it was pointed out that ath11k (and ath12k) do not wait for it. However with so many deployed devices, "might be ok" isn't a strong argument for changing the default behavior. cf. also https://wiki.postmarketos.org/wiki/Qualcomm_Snapdragon_835_(MSM8998)#WLAN Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> --- arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi index 026b6b97785b5..681a80f4dc9db 100644 --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi @@ -3185,6 +3185,7 @@ wifi: wifi@18800000 { iommus = <&anoc2_smmu 0x1900>, <&anoc2_smmu 0x1901>; qcom,snoc-host-cap-8bit-quirk; + qcom,no-msa-ready-indicator; }; }; }; -- 2.34.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-03-28 17:39 ` [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi Marc Gonzalez @ 2024-03-30 18:25 ` Krzysztof Kozlowski 2024-04-02 14:34 ` Konrad Dybcio 0 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2024-03-30 18:25 UTC (permalink / raw) To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio, Jami Kettunen, Jeffrey Hugo On 28/03/2024 18:39, Marc Gonzalez wrote: > The ath10k driver waits for an "MSA_READY" indicator > to complete initialization. If the indicator is not > received, then the device remains unusable. > > cf. ath10k_qmi_driver_event_work() > > Several msm8998-based devices are affected by this issue. > Oddly, it seems safe to NOT wait for the indicator, and > proceed immediately when QMI_EVENT_SERVER_ARRIVE. > > Jeff Johnson wrote: > > The feedback I received was "it might be ok to change all ath10k qmi > to skip waiting for msa_ready", and it was pointed out that ath11k > (and ath12k) do not wait for it. > > However with so many deployed devices, "might be ok" isn't a strong > argument for changing the default behavior. > I think you got pretty clear comments: "This sounds more like a firmware feature, not a hardware feature." "This is why having this property in DT does not look right place for this." Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-03-30 18:25 ` Krzysztof Kozlowski @ 2024-04-02 14:34 ` Konrad Dybcio 2024-04-02 15:31 ` Marc Gonzalez 0 siblings, 1 reply; 31+ messages in thread From: Konrad Dybcio @ 2024-04-02 14:34 UTC (permalink / raw) To: Krzysztof Kozlowski, Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 30.03.2024 7:25 PM, Krzysztof Kozlowski wrote: > On 28/03/2024 18:39, Marc Gonzalez wrote: >> The ath10k driver waits for an "MSA_READY" indicator >> to complete initialization. If the indicator is not >> received, then the device remains unusable. >> >> cf. ath10k_qmi_driver_event_work() >> >> Several msm8998-based devices are affected by this issue. >> Oddly, it seems safe to NOT wait for the indicator, and >> proceed immediately when QMI_EVENT_SERVER_ARRIVE. >> >> Jeff Johnson wrote: >> >> The feedback I received was "it might be ok to change all ath10k qmi >> to skip waiting for msa_ready", and it was pointed out that ath11k >> (and ath12k) do not wait for it. >> >> However with so many deployed devices, "might be ok" isn't a strong >> argument for changing the default behavior. >> > > I think you got pretty clear comments: > > "This sounds more like a firmware feature, not a hardware feature." > > "This is why having this property in DT does not look right > place for this." Translating from dt maintainer speak to English, a functionally-equivalent resolution of adding an of_machine_is_compatible("qcom,msm8998") is more in line with the guidelines of not sprinkling firmware specifics in DTs Konrad ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 14:34 ` Konrad Dybcio @ 2024-04-02 15:31 ` Marc Gonzalez 2024-04-02 15:55 ` Dmitry Baryshkov 0 siblings, 1 reply; 31+ messages in thread From: Marc Gonzalez @ 2024-04-02 15:31 UTC (permalink / raw) To: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo, Dmitry Baryshkov On 02/04/2024 16:34, Konrad Dybcio wrote: > On 30.03.2024 7:25 PM, Krzysztof Kozlowski wrote: > >> On 28/03/2024 18:39, Marc Gonzalez wrote: >> >>> The ath10k driver waits for an "MSA_READY" indicator >>> to complete initialization. If the indicator is not >>> received, then the device remains unusable. >>> >>> cf. ath10k_qmi_driver_event_work() >>> >>> Several msm8998-based devices are affected by this issue. >>> Oddly, it seems safe to NOT wait for the indicator, and >>> proceed immediately when QMI_EVENT_SERVER_ARRIVE. >>> >>> Jeff Johnson wrote: >>> >>> The feedback I received was "it might be ok to change all ath10k qmi >>> to skip waiting for msa_ready", and it was pointed out that ath11k >>> (and ath12k) do not wait for it. >>> >>> However with so many deployed devices, "might be ok" isn't a strong >>> argument for changing the default behavior. >> >> I think you got pretty clear comments: >> >> "This sounds more like a firmware feature, not a hardware feature." >> >> "This is why having this property in DT does not look right >> place for this." > > Translating from dt maintainer speak to English, a functionally-equivalent > resolution of adding an of_machine_is_compatible("qcom,msm8998") is more > in line with the guidelines of not sprinkling firmware specifics in DTs I'm not so sure about that, as I had proposed + if (of_device_is_compatible(of_root, "qcom,msm8998") + qmi->no_point_in_waiting_for_msa_ready_indicator = true; + To which Conor replied: > How come the root node comes into this, don't you have a soc-specific > compatible for the integration on this SoC? > (I am assuming that this is not the SDIO variant, given then it'd not be > fixed to this particular implementation) Then added: > A SoC-specific compatible sounds like it would be suitable in that case > then, to deal with integration quirks for that specific SoC? I usually > leave the ins and outs of these qcom SoCs to Krzysztof, but I can't help > but wanna know what the justification is here for not using one. Then Krzysztof added: > The WiFi+BT chips are separate products, so they are not usually > considered part of the SoC, even though they can be integrated into the > SoC like here. I guess correct approach would be to add SoC-specific > compatible for them. So, if I understand correctly, I take this to mean that I should: 1) DELETE the qcom,no-msa-ready-indicator boolean property, 2) ADD a "qcom,msm8998-wifi" (name OK?) compatible, 3) ADD that compatible to the wifi node in msm8998.dtsi compatible = "qcom,wcn3990-wifi", "qcom,msm8998-wifi"; 4) In the driver, set qmi->fake_msa_ready_indicator to true if we detect "qcom,msm8998-wifi" And this approach would be acceptable to both ath10k & DT maintainers? Bjarne, Konrad: is it OK to apply the work-around for all msm8998 boards? Regards ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 15:31 ` Marc Gonzalez @ 2024-04-02 15:55 ` Dmitry Baryshkov 2024-04-02 18:22 ` Jeff Johnson ` (3 more replies) 0 siblings, 4 replies; 31+ messages in thread From: Dmitry Baryshkov @ 2024-04-02 15:55 UTC (permalink / raw) To: Marc Gonzalez Cc: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On Tue, 2 Apr 2024 at 18:31, Marc Gonzalez <mgonzalez@freebox.fr> wrote: > > On 02/04/2024 16:34, Konrad Dybcio wrote: > > > On 30.03.2024 7:25 PM, Krzysztof Kozlowski wrote: > > > >> On 28/03/2024 18:39, Marc Gonzalez wrote: > >> > >>> The ath10k driver waits for an "MSA_READY" indicator > >>> to complete initialization. If the indicator is not > >>> received, then the device remains unusable. > >>> > >>> cf. ath10k_qmi_driver_event_work() > >>> > >>> Several msm8998-based devices are affected by this issue. > >>> Oddly, it seems safe to NOT wait for the indicator, and > >>> proceed immediately when QMI_EVENT_SERVER_ARRIVE. > >>> > >>> Jeff Johnson wrote: > >>> > >>> The feedback I received was "it might be ok to change all ath10k qmi > >>> to skip waiting for msa_ready", and it was pointed out that ath11k > >>> (and ath12k) do not wait for it. > >>> > >>> However with so many deployed devices, "might be ok" isn't a strong > >>> argument for changing the default behavior. > >> > >> I think you got pretty clear comments: > >> > >> "This sounds more like a firmware feature, not a hardware feature." > >> > >> "This is why having this property in DT does not look right > >> place for this." > > > > Translating from dt maintainer speak to English, a functionally-equivalent > > resolution of adding an of_machine_is_compatible("qcom,msm8998") is more > > in line with the guidelines of not sprinkling firmware specifics in DTs > > I'm not so sure about that, as I had proposed > > + if (of_device_is_compatible(of_root, "qcom,msm8998") > + qmi->no_point_in_waiting_for_msa_ready_indicator = true; > + > > To which Conor replied: > > > How come the root node comes into this, don't you have a soc-specific > > compatible for the integration on this SoC? > > (I am assuming that this is not the SDIO variant, given then it'd not be > > fixed to this particular implementation) > > > Then added: > > > A SoC-specific compatible sounds like it would be suitable in that case > > then, to deal with integration quirks for that specific SoC? I usually > > leave the ins and outs of these qcom SoCs to Krzysztof, but I can't help > > but wanna know what the justification is here for not using one. > > > Then Krzysztof added: > > > The WiFi+BT chips are separate products, so they are not usually > > considered part of the SoC, even though they can be integrated into the > > SoC like here. I guess correct approach would be to add SoC-specific > > compatible for them. > > > So, if I understand correctly, I take this to mean that I should: > > 1) DELETE the qcom,no-msa-ready-indicator boolean property, > 2) ADD a "qcom,msm8998-wifi" (name OK?) compatible, I'd say, this is not correct. There is no "msm8998-wifi". > 3) ADD that compatible to the wifi node in msm8998.dtsi > compatible = "qcom,wcn3990-wifi", "qcom,msm8998-wifi"; > 4) In the driver, set qmi->fake_msa_ready_indicator to true if we detect "qcom,msm8998-wifi" > > And this approach would be acceptable to both ath10k & DT maintainers? I'd say, we should take a step back and actually verify how this was handled in the vendor kernel. > > Bjarne, Konrad: is it OK to apply the work-around for all msm8998 boards? -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 15:55 ` Dmitry Baryshkov @ 2024-04-02 18:22 ` Jeff Johnson 2024-04-02 19:15 ` Dmitry Baryshkov 2024-04-02 18:25 ` Alexey Minnekhanov ` (2 subsequent siblings) 3 siblings, 1 reply; 31+ messages in thread From: Jeff Johnson @ 2024-04-02 18:22 UTC (permalink / raw) To: Dmitry Baryshkov, Marc Gonzalez Cc: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 4/2/2024 8:55 AM, Dmitry Baryshkov wrote: > I'd say, we should take a step back and actually verify how this was > handled in the vendor kernel. (error handling and other non-QMI code removed from the following for readability) In ath10k we unconditionally call the following in ath10k_qmi_event_server_arrive(): ret = ath10k_qmi_host_cap_send_sync(qmi); ret = ath10k_qmi_msa_mem_info_send_sync_msg(qmi); ret = ath10k_qmi_setup_msa_permissions(qmi); ret = ath10k_qmi_msa_ready_send_sync_msg(qmi); ret = ath10k_qmi_cap_send_sync_msg(qmi); In vendor icnss2 there is conditional logic in icnss_driver_event_server_arrive(): if (priv->device_id == WCN6750_DEVICE_ID || priv->device_id == WCN6450_DEVICE_ID) { ret = wlfw_host_cap_send_sync(priv); } if (priv->device_id == ADRASTEA_DEVICE_ID) { ret = wlfw_msa_mem_info_send_sync_msg(priv); ret = wlfw_msa_ready_send_sync_msg(priv); } ret = wlfw_cap_send_sync_msg(priv); reference: https://git.codelinaro.org/clo/la/platform/vendor/qcom-opensource/wlan/platform/-/blob/wlan-platform.lnx.1.0.r1-rel/icnss2/main.c?ref_type=heads#L890 /jeff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 18:22 ` Jeff Johnson @ 2024-04-02 19:15 ` Dmitry Baryshkov 0 siblings, 0 replies; 31+ messages in thread From: Dmitry Baryshkov @ 2024-04-02 19:15 UTC (permalink / raw) To: Jeff Johnson Cc: Marc Gonzalez, Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On Tue, 2 Apr 2024 at 21:22, Jeff Johnson <quic_jjohnson@quicinc.com> wrote: > > On 4/2/2024 8:55 AM, Dmitry Baryshkov wrote: > > I'd say, we should take a step back and actually verify how this was > > handled in the vendor kernel. > > (error handling and other non-QMI code removed from the following for readability) > > In ath10k we unconditionally call the following in > ath10k_qmi_event_server_arrive(): > ret = ath10k_qmi_host_cap_send_sync(qmi); > ret = ath10k_qmi_msa_mem_info_send_sync_msg(qmi); > ret = ath10k_qmi_setup_msa_permissions(qmi); > ret = ath10k_qmi_msa_ready_send_sync_msg(qmi); > ret = ath10k_qmi_cap_send_sync_msg(qmi); > > In vendor icnss2 there is conditional logic in icnss_driver_event_server_arrive(): Note, wcn3990 is icnss, not icnss2 > if (priv->device_id == WCN6750_DEVICE_ID || > priv->device_id == WCN6450_DEVICE_ID) { > ret = wlfw_host_cap_send_sync(priv); > } > > if (priv->device_id == ADRASTEA_DEVICE_ID) { > ret = wlfw_msa_mem_info_send_sync_msg(priv); > ret = wlfw_msa_ready_send_sync_msg(priv); > } The problem with applying this approach is that here the discriminator is the WiFi device ID. WCN6750, WCN6450 and this ADRASTEA are different WiFi/BT chips. However for msm8998 and e.g. sdm845 there is no easy way to distinguish the WiFi chips. Both platforms use wcn3990 device. > > ret = wlfw_cap_send_sync_msg(priv); > > reference: > https://git.codelinaro.org/clo/la/platform/vendor/qcom-opensource/wlan/platform/-/blob/wlan-platform.lnx.1.0.r1-rel/icnss2/main.c?ref_type=heads#L890 > > /jeff -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 15:55 ` Dmitry Baryshkov 2024-04-02 18:22 ` Jeff Johnson @ 2024-04-02 18:25 ` Alexey Minnekhanov 2024-04-02 19:21 ` Dmitry Baryshkov 2024-04-02 23:32 ` Jeff Johnson 2024-04-03 13:05 ` Marc Gonzalez 2024-04-04 11:57 ` Kalle Valo 3 siblings, 2 replies; 31+ messages in thread From: Alexey Minnekhanov @ 2024-04-02 18:25 UTC (permalink / raw) To: Dmitry Baryshkov, Marc Gonzalez Cc: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 02.04.2024 18:55, Dmitry Baryshkov wrote: > I'd say, we should take a step back and actually verify how this was > handled in the vendor kernel. AFAIK there is no such thing in vendor kernel driver for this, as this startup procedure is likely handled entirely in userspace in cnss_daemon. By the way this workaround is needed also for Wi-Fi in sdm630/660, so no not only msm8998 suffers from this. > This sounds more like a firmware feature, not a hardware feature > having this property in DT does not look right I agree with these 2 points above. This can be handled more nicely as firmware feature encoded in firmware-5.bin using ath10k-fwencoder and not involve any new DT compatibles or properties. -- Regards, Alexey Minnekhanov ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 18:25 ` Alexey Minnekhanov @ 2024-04-02 19:21 ` Dmitry Baryshkov 2024-04-02 23:32 ` Jeff Johnson 1 sibling, 0 replies; 31+ messages in thread From: Dmitry Baryshkov @ 2024-04-02 19:21 UTC (permalink / raw) To: Alexey Minnekhanov Cc: Marc Gonzalez, Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On Tue, 2 Apr 2024 at 21:25, Alexey Minnekhanov <alexeymin@postmarketos.org> wrote: > > > > On 02.04.2024 18:55, Dmitry Baryshkov wrote: > > I'd say, we should take a step back and actually verify how this was > > handled in the vendor kernel. > > > AFAIK there is no such thing in vendor kernel driver for this, as > this startup procedure is likely handled entirely in userspace in > cnss_daemon. > > By the way this workaround is needed also for Wi-Fi in sdm630/660, > so no not only msm8998 suffers from this. Interesting. I have an sdm660 platform. I think I should be able to check these workarounds then. > > > This sounds more like a firmware feature, not a hardware feature > > > having this property in DT does not look right > > I agree with these 2 points above. This can be handled more nicely > as firmware feature encoded in firmware-5.bin using ath10k-fwencoder > and not involve any new DT compatibles or properties. I think Marc has already tried this. The firmware-N.bin, so-called "boot firmware" (because for normal devices it also contains the actual firmware), is loaded much later. See https://lore.kernel.org/ath10k/243a97b7-c298-4307-9f06-8b3a7c3e24fd@freebox.fr/ Probably we have an option of loading the firmware earlier, so that at this stage we already know the quirks set in the firmware-5.bin. But note, I haven't checked if at this point the driver has all the information to select correct firmware blob. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 18:25 ` Alexey Minnekhanov 2024-04-02 19:21 ` Dmitry Baryshkov @ 2024-04-02 23:32 ` Jeff Johnson 1 sibling, 0 replies; 31+ messages in thread From: Jeff Johnson @ 2024-04-02 23:32 UTC (permalink / raw) To: Alexey Minnekhanov, Dmitry Baryshkov, Marc Gonzalez, Kalle Valo Cc: Konrad Dybcio, Krzysztof Kozlowski, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 4/2/2024 11:25 AM, Alexey Minnekhanov wrote: > > > On 02.04.2024 18:55, Dmitry Baryshkov wrote: >> I'd say, we should take a step back and actually verify how this was >> handled in the vendor kernel. > > > AFAIK there is no such thing in vendor kernel driver for this, as > this startup procedure is likely handled entirely in userspace in > cnss_daemon. > So now I'm looking at the cnss-daemon, and there it appears that MSA READY is always expected to be received. There is target-specific logic to set the flags sent to firmware: if (gdata->instance_id == ADRASTEA_ID) { req.msa_ready_enable_valid = 1; req.msa_ready_enable = 1; } else { /* All targets other than Adrastea */ req.fw_mem_ready_enable_valid = 1; req.fw_mem_ready_enable = 1; } Logic to set an internal flag if the message is received: case QMI_WLFW_MSA_READY_IND_V01: gdata->state |= CNSS_MSA_READY; And Adrastea-specific logic to set that flag if it is set in a separate status indicator: static int wlfw_adrastea_init(struct wlfw_client_data *gdata) [...] if (fw_status & QMI_WLFW_MSA_READY_V01) { wsvc_printf_dbg("MSA is ready"); gdata->state |= CNSS_MSA_READY; } So that flag is only set by receiving the QMI_WLFW_MSA_READY_IND_V01 message or, only for Adrastea, if the response to wlfw_send_ind_register_req() indicates MSA_READY Later there is a wait for MSA_READY that has no conditions: while (!CNSS_IS_MSA_READY(gdata->state)) Truthfully this is code I've never had to deal with before, and I've struggled to find developers who have the necessary background. The least disruptive paths seem to either be the DT item or adding a new item for this in struct ath10k_hw_params. Kalle, do you have any different guidance? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 15:55 ` Dmitry Baryshkov 2024-04-02 18:22 ` Jeff Johnson 2024-04-02 18:25 ` Alexey Minnekhanov @ 2024-04-03 13:05 ` Marc Gonzalez 2024-04-03 14:12 ` Dmitry Baryshkov 2024-04-03 14:14 ` Krzysztof Kozlowski 2024-04-04 11:57 ` Kalle Valo 3 siblings, 2 replies; 31+ messages in thread From: Marc Gonzalez @ 2024-04-03 13:05 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 02/04/2024 17:55, Dmitry Baryshkov wrote: > On Tue, 2 Apr 2024 at 18:31, Marc Gonzalez wrote: > >> So, if I understand correctly, I take this to mean that I should: >> >> 1) DELETE the qcom,no-msa-ready-indicator boolean property, >> 2) ADD a "qcom,msm8998-wifi" (name OK?) compatible, > > I'd say, this is not correct. There is no "msm8998-wifi". Can you explain what you mean by: 'There is no "msm8998-wifi".' ? Do you mean that: this compatible string does not exist? (I am proposing that it be created.) Or do you mean that: "msm8998-wifi" is a bad name? I meant to mimic these strings for various sub-blocks: arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-rpm-proc", "qcom,rpm-proc"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-rpmpd"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-qfprom", "qcom,qfprom"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-tsens", "qcom,tsens-v2"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-tsens", "qcom,tsens-v2"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-qmp-pcie-phy"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-qmp-ufs-phy"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-tcsr", "syscon"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-tcsr", "syscon"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-pinctrl"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-mss-pil"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-smmu-v2", "qcom,smmu-v2", arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-gpucc"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-slpi-pas"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-dwc3", "qcom,dwc3"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-qmp-usb3-phy"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-qusb2-phy"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-sdhci", "qcom,sdhci-msm-v4"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-mdss"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-dpu"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-dsi-ctrl", "qcom,mdss-dsi-ctrl"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-dsi-ctrl", "qcom,mdss-dsi-ctrl"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-venus"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-adsp-pas"; arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-apcs-hmss-global", And these strings in ath11k: Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,ipq8074-wifi Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,ipq6018-wifi Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,wcn6750-wifi Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,ipq5018-wifi > I'd say, we should take a step back and actually verify how this was > handled in the vendor kernel. In our commercial product, we use the ath10k driver in the vendor kernel (v4.4 r38-rel). It looks like Jeff has already performed the code analysis wrt vendor vs mainline (including user-space tools). Regards ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-03 13:05 ` Marc Gonzalez @ 2024-04-03 14:12 ` Dmitry Baryshkov 2024-04-03 18:16 ` Marc Gonzalez 2024-04-03 14:14 ` Krzysztof Kozlowski 1 sibling, 1 reply; 31+ messages in thread From: Dmitry Baryshkov @ 2024-04-03 14:12 UTC (permalink / raw) To: Marc Gonzalez Cc: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On Wed, 3 Apr 2024 at 16:05, Marc Gonzalez <mgonzalez@freebox.fr> wrote: > > On 02/04/2024 17:55, Dmitry Baryshkov wrote: > > > On Tue, 2 Apr 2024 at 18:31, Marc Gonzalez wrote: > > > >> So, if I understand correctly, I take this to mean that I should: > >> > >> 1) DELETE the qcom,no-msa-ready-indicator boolean property, > >> 2) ADD a "qcom,msm8998-wifi" (name OK?) compatible, > > > > I'd say, this is not correct. There is no "msm8998-wifi". > > Can you explain what you mean by: > 'There is no "msm8998-wifi".' ? > > Do you mean that: this compatible string does not exist? > (I am proposing that it be created.) > > Or do you mean that: "msm8998-wifi" is a bad name? I mean, it is qcom,wcn3990-wifi, because the chip is wcn3990. > And these strings in ath11k: > > Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,ipq8074-wifi > Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,ipq6018-wifi > Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,wcn6750-wifi > Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml: - qcom,ipq5018-wifi I must admit, I don't know the IPQ product naming (it well might be that it is both the name of the SoC and the WiFI SoC). > > > I'd say, we should take a step back and actually verify how this was > > handled in the vendor kernel. > > In our commercial product, we use the ath10k driver in the vendor kernel (v4.4 r38-rel). I see. > It looks like Jeff has already performed the code analysis > wrt vendor vs mainline (including user-space tools). From his message it looks like we are expected to get MSA READY even on msm8998. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-03 14:12 ` Dmitry Baryshkov @ 2024-04-03 18:16 ` Marc Gonzalez 0 siblings, 0 replies; 31+ messages in thread From: Marc Gonzalez @ 2024-04-03 18:16 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Konrad Dybcio, Krzysztof Kozlowski, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 03/04/2024 16:12, Dmitry Baryshkov wrote: > From [Jeff's] message it looks like we are expected to get MSA READY even on msm8998. This is the code we're using: https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/net/wireless/ath/ath10k/qmi.c When ATH10K_SNOC_DRIVER_EVENT_SERVER_ARRIVE, driver registers an "indicator handler" ath10k_snoc_qmi_wlfw_clnt_ind() It handles QMI_WLFW_FW_READY_IND_V01 by posting ATH10K_SNOC_DRIVER_EVENT_FW_READY_IND which is handled in the ath10k_snoc_driver_event_work() work queue. But QMI_WLFW_MSA_READY_IND_V01 only triggers a debug log and setting qmi_cfg->msa_ready = true; $ git grep '\<msa_ready\>' drivers/net/wireless/ath/ath10k/qmi.c: qmi_cfg->msa_ready = true; drivers/net/wireless/ath/ath10k/qmi.c: qmi_cfg->msa_ready = false; drivers/net/wireless/ath/ath10k/qmi.h: * msa_ready: wlan firmware msa memory ready for board data download drivers/net/wireless/ath/ath10k/qmi.h: bool msa_ready; So basically, the vendor ath10k driver ignores QMI_WLFW_MSA_READY_IND_V01. I will test the following patch which aligns the behavior of mainline driver to that of vendor driver: diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 38e939f572a9e..0e1ab5aca663b 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -1040,6 +1040,7 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) switch (event->type) { case ATH10K_QMI_EVENT_SERVER_ARRIVE: ath10k_qmi_event_server_arrive(qmi); + ath10k_qmi_event_msa_ready(qmi); break; case ATH10K_QMI_EVENT_SERVER_EXIT: ath10k_qmi_event_server_exit(qmi); @@ -1048,7 +1049,7 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) ath10k_qmi_event_fw_ready_ind(qmi); break; case ATH10K_QMI_EVENT_MSA_READY_IND: - ath10k_qmi_event_msa_ready(qmi); + printk(KERN_WARNING "IGNORING MSA_READY INDICATOR"); break; default: ath10k_warn(ar, "invalid event type: %d", event->type); Dmitry Baryshkov reported: Works on sm8150, sdm845, qrb2210 Regards ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-03 13:05 ` Marc Gonzalez 2024-04-03 14:12 ` Dmitry Baryshkov @ 2024-04-03 14:14 ` Krzysztof Kozlowski 1 sibling, 0 replies; 31+ messages in thread From: Krzysztof Kozlowski @ 2024-04-03 14:14 UTC (permalink / raw) To: Marc Gonzalez, Dmitry Baryshkov Cc: Konrad Dybcio, Kalle Valo, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 03/04/2024 15:05, Marc Gonzalez wrote: > On 02/04/2024 17:55, Dmitry Baryshkov wrote: > >> On Tue, 2 Apr 2024 at 18:31, Marc Gonzalez wrote: >> >>> So, if I understand correctly, I take this to mean that I should: >>> >>> 1) DELETE the qcom,no-msa-ready-indicator boolean property, >>> 2) ADD a "qcom,msm8998-wifi" (name OK?) compatible, >> >> I'd say, this is not correct. There is no "msm8998-wifi". > > Can you explain what you mean by: > 'There is no "msm8998-wifi".' ? > > Do you mean that: this compatible string does not exist? > (I am proposing that it be created.) > > Or do you mean that: "msm8998-wifi" is a bad name? msm8998 SoC does not have WiFi. > > > I meant to mimic these strings for various sub-blocks: > > arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-rpm-proc", "qcom,rpm-proc"; > arch/arm64/boot/dts/qcom/msm8998.dtsi: compatible = "qcom,msm8998-rpmpd"; These are all parts of SoC. What you are adding is not part of original SoC, I think. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-02 15:55 ` Dmitry Baryshkov ` (2 preceding siblings ...) 2024-04-03 13:05 ` Marc Gonzalez @ 2024-04-04 11:57 ` Kalle Valo 2024-04-04 12:30 ` Marc Gonzalez 3 siblings, 1 reply; 31+ messages in thread From: Kalle Valo @ 2024-04-04 11:57 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Marc Gonzalez, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes: >> 3) ADD that compatible to the wifi node in msm8998.dtsi >> compatible = "qcom,wcn3990-wifi", "qcom,msm8998-wifi"; >> 4) In the driver, set qmi->fake_msa_ready_indicator to true if we >> detect "qcom,msm8998-wifi" >> >> And this approach would be acceptable to both ath10k & DT maintainers? > > I'd say, we should take a step back and actually verify how this was > handled in the vendor kernel. One comment related to this: usually vendor driver and firmware branches go "hand in hand", meaning that a version of driver supports only one specific firmware branch. And there can be a lot of branches. So even if one branch might have a check for something specific, there are no guarantees what the other N+1 branches do :/ -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-04 11:57 ` Kalle Valo @ 2024-04-04 12:30 ` Marc Gonzalez 2024-04-04 13:14 ` Dmitry Baryshkov 2024-04-04 15:28 ` Kalle Valo 0 siblings, 2 replies; 31+ messages in thread From: Marc Gonzalez @ 2024-04-04 12:30 UTC (permalink / raw) To: Kalle Valo, Dmitry Baryshkov Cc: Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On 04/04/2024 13:57, Kalle Valo wrote: > Dmitry Baryshkov wrote: > >> I'd say, we should take a step back and actually verify how this was >> handled in the vendor kernel. > > One comment related to this: usually vendor driver and firmware branches > go "hand in hand", meaning that a version of driver supports only one > specific firmware branch. And there can be a lot of branches. So even if > one branch might have a check for something specific, there are no > guarantees what the other N+1 branches do :/ The consequences and ramifications of the above comment are not clear to me. Does this mean: "It is pointless to analyze a given version (or even several versions) of the vendor driver downstream, because there are exist a large number of variations of the code." ? And thus, "it is nonsensical to try to "align" the mainline driver to "the" vendor driver, as there is no single "vendor driver"" ? Thus, the following patch (or one functionally-equivalent) is not acceptable? diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 38e939f572a9e..fd9ac9717488a 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -1040,6 +1040,8 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) switch (event->type) { case ATH10K_QMI_EVENT_SERVER_ARRIVE: ath10k_qmi_event_server_arrive(qmi); + printk(KERN_NOTICE "NOT WAITING FOR MSA_READY INDICATOR"); + ath10k_qmi_event_msa_ready(qmi); break; case ATH10K_QMI_EVENT_SERVER_EXIT: ath10k_qmi_event_server_exit(qmi); @@ -1048,7 +1050,7 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) ath10k_qmi_event_fw_ready_ind(qmi); break; case ATH10K_QMI_EVENT_MSA_READY_IND: - ath10k_qmi_event_msa_ready(qmi); + printk(KERN_NOTICE "IGNORING ACTUAL MSA_READY INDICATOR"); break; default: ath10k_warn(ar, "invalid event type: %d", event->type); Regards ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-04 12:30 ` Marc Gonzalez @ 2024-04-04 13:14 ` Dmitry Baryshkov 2024-04-04 15:28 ` Kalle Valo 1 sibling, 0 replies; 31+ messages in thread From: Dmitry Baryshkov @ 2024-04-04 13:14 UTC (permalink / raw) To: Marc Gonzalez Cc: Kalle Valo, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo On Thu, 4 Apr 2024 at 15:30, Marc Gonzalez <mgonzalez@freebox.fr> wrote: > > On 04/04/2024 13:57, Kalle Valo wrote: > > > Dmitry Baryshkov wrote: > > > >> I'd say, we should take a step back and actually verify how this was > >> handled in the vendor kernel. > > > > One comment related to this: usually vendor driver and firmware branches > > go "hand in hand", meaning that a version of driver supports only one > > specific firmware branch. And there can be a lot of branches. So even if > > one branch might have a check for something specific, there are no > > guarantees what the other N+1 branches do :/ > > The consequences and ramifications of the above comment are not clear to me. > > Does this mean: > "It is pointless to analyze a given version (or even several versions) > of the vendor driver downstream, because there are exist a large number > of variations of the code." ? > > And thus, "it is nonsensical to try to "align" the mainline driver to > "the" vendor driver, as there is no single "vendor driver"" ? > > Thus, the following patch (or one functionally-equivalent) is not acceptable? For reference, I tested this patch on sdm845 (db845c), qcm2290 aka qrb2210 (rb1), sm6115 aka qrb4210 (rb2) and sm8150 platforms. I was not able to fully test it on sda660, modem crashes without this patch (there is no MSA_READY indication) and with the patch applied the device hangs, most likely because of the IOMMU or clocking issue. > > diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c > index 38e939f572a9e..fd9ac9717488a 100644 > --- a/drivers/net/wireless/ath/ath10k/qmi.c > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > @@ -1040,6 +1040,8 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) > switch (event->type) { > case ATH10K_QMI_EVENT_SERVER_ARRIVE: > ath10k_qmi_event_server_arrive(qmi); > + printk(KERN_NOTICE "NOT WAITING FOR MSA_READY INDICATOR"); > + ath10k_qmi_event_msa_ready(qmi); > break; > case ATH10K_QMI_EVENT_SERVER_EXIT: > ath10k_qmi_event_server_exit(qmi); > @@ -1048,7 +1050,7 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) > ath10k_qmi_event_fw_ready_ind(qmi); > break; > case ATH10K_QMI_EVENT_MSA_READY_IND: > - ath10k_qmi_event_msa_ready(qmi); > + printk(KERN_NOTICE "IGNORING ACTUAL MSA_READY INDICATOR"); > break; > default: > ath10k_warn(ar, "invalid event type: %d", event->type); > > > > Regards > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-04 12:30 ` Marc Gonzalez 2024-04-04 13:14 ` Dmitry Baryshkov @ 2024-04-04 15:28 ` Kalle Valo 2024-04-08 15:53 ` Marc Gonzalez 1 sibling, 1 reply; 31+ messages in thread From: Kalle Valo @ 2024-04-04 15:28 UTC (permalink / raw) To: Marc Gonzalez Cc: Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Jeffrey Hugo Marc Gonzalez <mgonzalez@freebox.fr> writes: > On 04/04/2024 13:57, Kalle Valo wrote: > >> Dmitry Baryshkov wrote: >> >>> I'd say, we should take a step back and actually verify how this was >>> handled in the vendor kernel. >> >> One comment related to this: usually vendor driver and firmware branches >> go "hand in hand", meaning that a version of driver supports only one >> specific firmware branch. And there can be a lot of branches. So even if >> one branch might have a check for something specific, there are no >> guarantees what the other N+1 branches do :/ > > The consequences and ramifications of the above comment are not clear to me. > > Does this mean: > "It is pointless to analyze a given version (or even several versions) > of the vendor driver downstream, because there are exist a large number > of variations of the code." ? I was trying to say that because the design philosophy between vendor drivers and upstream drivers is very different, we can't 100% trust vendor drivers. It's a very good idea to check what vendor drivers do but we just need to be careful before making any conclusions. Testing real hardware (and corresponding firmware) is the most reliable way to know how different products/firmware work, unfortunately. > And thus, "it is nonsensical to try to "align" the mainline driver to > "the" vendor driver, as there is no single "vendor driver"" ? No no, I'm not saying that. I have suffered this "N+1 different firmware branches behaving slighly differently" problem since ath6kl days so for me this is business as usual, sadly. I'm sure we can find a solution for ath10k. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-04 15:28 ` Kalle Valo @ 2024-04-08 15:53 ` Marc Gonzalez 2024-04-25 9:42 ` Kalle Valo 0 siblings, 1 reply; 31+ messages in thread From: Marc Gonzalez @ 2024-04-08 15:53 UTC (permalink / raw) To: Kalle Valo Cc: Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Marijn Suijten, Jeffrey Hugo On 04/04/2024 17:28, Kalle Valo wrote: > Marc Gonzalez wrote: > >> On 04/04/2024 13:57, Kalle Valo wrote: >> >>> Dmitry Baryshkov wrote: >>> >>>> I'd say, we should take a step back and actually verify how this was >>>> handled in the vendor kernel. >>> >>> One comment related to this: usually vendor driver and firmware branches >>> go "hand in hand", meaning that a version of driver supports only one >>> specific firmware branch. And there can be a lot of branches. So even if >>> one branch might have a check for something specific, there are no >>> guarantees what the other N+1 branches do :/ >> >> The consequences and ramifications of the above comment are not clear to me. >> >> Does this mean: >> "It is pointless to analyze a given version (or even several versions) >> of the vendor driver downstream, because there are exist a large number >> of variations of the code." ? > > I was trying to say that because the design philosophy between vendor > drivers and upstream drivers is very different, we can't 100% trust > vendor drivers. It's a very good idea to check what vendor drivers do > but we just need to be careful before making any conclusions. Testing > real hardware (and corresponding firmware) is the most reliable way to > know how different products/firmware work, unfortunately. > >> And thus, "it is nonsensical to try to "align" the mainline driver to >> "the" vendor driver, as there is no single "vendor driver"" ? > > No no, I'm not saying that. I have suffered this "N+1 different firmware > branches behaving slighly differently" problem since ath6kl days so for > me this is business as usual, sadly. I'm sure we can find a solution for > ath10k. Hello Kalle, I can spin a v3, no problem. Do you prefer: Option A = never waiting for the MSA_READY indicator for ANYONE Option B = not waiting for the MSA_READY indicator when qcom,no-msa-ready-indicator is defined Option C = not waiting for the MSA_READY indicator for certain platforms (based on root compatible) Option D = some other solution not yet discussed Dmitry has tested Option A on 5 platforms, where it does not induce regressions. I worked on msm8998, where Option A (or any equivalent) unbreaks WiFi. Please provide guidance :) Regards ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-08 15:53 ` Marc Gonzalez @ 2024-04-25 9:42 ` Kalle Valo 2024-04-25 11:48 ` Marc Gonzalez 0 siblings, 1 reply; 31+ messages in thread From: Kalle Valo @ 2024-04-25 9:42 UTC (permalink / raw) To: Marc Gonzalez Cc: Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Marijn Suijten, Jeffrey Hugo Marc Gonzalez <mgonzalez@freebox.fr> writes: > On 04/04/2024 17:28, Kalle Valo wrote: > >> Marc Gonzalez wrote: >> >>> On 04/04/2024 13:57, Kalle Valo wrote: >>> >>>> Dmitry Baryshkov wrote: >>>> >>>>> I'd say, we should take a step back and actually verify how this was >>>>> handled in the vendor kernel. >>>> >>>> One comment related to this: usually vendor driver and firmware branches >>>> go "hand in hand", meaning that a version of driver supports only one >>>> specific firmware branch. And there can be a lot of branches. So even if >>>> one branch might have a check for something specific, there are no >>>> guarantees what the other N+1 branches do :/ >>> >>> The consequences and ramifications of the above comment are not clear to me. >>> >>> Does this mean: >>> "It is pointless to analyze a given version (or even several versions) >>> of the vendor driver downstream, because there are exist a large number >>> of variations of the code." ? >> >> I was trying to say that because the design philosophy between vendor >> drivers and upstream drivers is very different, we can't 100% trust >> vendor drivers. It's a very good idea to check what vendor drivers do >> but we just need to be careful before making any conclusions. Testing >> real hardware (and corresponding firmware) is the most reliable way to >> know how different products/firmware work, unfortunately. >> >>> And thus, "it is nonsensical to try to "align" the mainline driver to >>> "the" vendor driver, as there is no single "vendor driver"" ? >> >> No no, I'm not saying that. I have suffered this "N+1 different firmware >> branches behaving slighly differently" problem since ath6kl days so for >> me this is business as usual, sadly. I'm sure we can find a solution for >> ath10k. > > Hello Kalle, > > I can spin a v3, no problem. > > Do you prefer: > > Option A = never waiting for the MSA_READY indicator for ANYONE > Option B = not waiting for the MSA_READY indicator when > qcom,no-msa-ready-indicator is defined > Option C = not waiting for the MSA_READY indicator for certain > platforms (based on root compatible) > Option D = some other solution not yet discussed After firmware-N.bin solution didn't work (sorry about that!) my prerence is option B. > Dmitry has tested Option A on 5 platforms, where it does not induce regressions. > I worked on msm8998, where Option A (or any equivalent) unbreaks WiFi. What do you mean here? Are you saying that option A works on all devices? I'm guessing I'm misunderstanding something. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-25 9:42 ` Kalle Valo @ 2024-04-25 11:48 ` Marc Gonzalez 2024-04-25 15:42 ` Kalle Valo 0 siblings, 1 reply; 31+ messages in thread From: Marc Gonzalez @ 2024-04-25 11:48 UTC (permalink / raw) To: Kalle Valo Cc: Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Marijn Suijten, Jeffrey Hugo On 25/04/2024 11:42, Kalle Valo wrote: > Marc Gonzalez wrote: > >> Do you prefer: >> >> Option A = never waiting for the MSA_READY indicator for ANYONE >> Option B = not waiting for the MSA_READY indicator when >> qcom,no-msa-ready-indicator is defined >> Option C = not waiting for the MSA_READY indicator for certain >> platforms (based on root compatible) >> Option D = some other solution not yet discussed > > After firmware-N.bin solution didn't work (sorry about that!) my > preference is option B. Actually, Option B is this patch series. Could you formally review it? Perhaps one thing I could do slightly differently is to NOT call ath10k_qmi_event_msa_ready() a second time if we DO receive the indicator later. >> Dmitry has tested Option A on 5 platforms, where it does not induce regressions. >> I worked on msm8998, where Option A (or any equivalent) unbreaks WiFi. > > What do you mean here? Are you saying that option A works on all > devices? I'm guessing I'm misunderstanding something. No one serious would ever claim "this works on all devices". Dmitry and I tested the following patch: diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 38e939f572a9e..fd9ac9717488a 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -1040,6 +1040,8 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) switch (event->type) { case ATH10K_QMI_EVENT_SERVER_ARRIVE: ath10k_qmi_event_server_arrive(qmi); + printk(KERN_NOTICE "NOT WAITING FOR MSA_READY INDICATOR"); + ath10k_qmi_event_msa_ready(qmi); break; case ATH10K_QMI_EVENT_SERVER_EXIT: ath10k_qmi_event_server_exit(qmi); @@ -1048,7 +1050,7 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) ath10k_qmi_event_fw_ready_ind(qmi); break; case ATH10K_QMI_EVENT_MSA_READY_IND: - ath10k_qmi_event_msa_ready(qmi); + printk(KERN_NOTICE "IGNORING ACTUAL MSA_READY INDICATOR"); break; default: ath10k_warn(ar, "invalid event type: %d", event->type); Dmitry tested several platforms: > For reference, I tested this patch on sdm845 (db845c), qcm2290 aka > qrb2210 (rb1), sm6115 aka qrb4210 (rb2) and sm8150 platforms. > I was not able to fully test it on sda660, modem crashes without this > patch (there is no MSA_READY indication) and with the patch applied > the device hangs, most likely because of the IOMMU or clocking issue. I tested on apq8098 (msm8998 sibling). Patch makes adapter work on my msm8998 platform. Regards ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-25 11:48 ` Marc Gonzalez @ 2024-04-25 15:42 ` Kalle Valo 2024-04-25 16:02 ` Conor Dooley 0 siblings, 1 reply; 31+ messages in thread From: Kalle Valo @ 2024-04-25 15:42 UTC (permalink / raw) To: Marc Gonzalez Cc: Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Marijn Suijten, Jeffrey Hugo Marc Gonzalez <mgonzalez@freebox.fr> writes: > On 25/04/2024 11:42, Kalle Valo wrote: > >> Marc Gonzalez wrote: >> >>> Do you prefer: >>> >>> Option A = never waiting for the MSA_READY indicator for ANYONE >>> Option B = not waiting for the MSA_READY indicator when >>> qcom,no-msa-ready-indicator is defined >>> Option C = not waiting for the MSA_READY indicator for certain >>> platforms (based on root compatible) >>> Option D = some other solution not yet discussed >> >> After firmware-N.bin solution didn't work (sorry about that!) my >> preference is option B. > > Actually, Option B is this patch series. > Could you formally review it? I'm happy with this series and would take it to ath.git, just need an ack from DT maintainers: https://patchwork.kernel.org/project/linux-wireless/patch/84f20fb5-5d48-419c-8eff-d7044afb81c0@freebox.fr/ > Perhaps one thing I could do slightly differently is to NOT call > ath10k_qmi_event_msa_ready() a second time if we DO receive the > indicator later. Good point. And maybe add an ath10k_warn() message so that we notice if there's a mismatch. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-25 15:42 ` Kalle Valo @ 2024-04-25 16:02 ` Conor Dooley 2024-04-25 16:39 ` Kalle Valo 0 siblings, 1 reply; 31+ messages in thread From: Conor Dooley @ 2024-04-25 16:02 UTC (permalink / raw) To: Kalle Valo Cc: Marc Gonzalez, Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Marijn Suijten, Jeffrey Hugo [-- Attachment #1: Type: text/plain, Size: 1609 bytes --] On Thu, Apr 25, 2024 at 06:42:16PM +0300, Kalle Valo wrote: > Marc Gonzalez <mgonzalez@freebox.fr> writes: > > > On 25/04/2024 11:42, Kalle Valo wrote: > > > >> Marc Gonzalez wrote: > >> > >>> Do you prefer: > >>> > >>> Option A = never waiting for the MSA_READY indicator for ANYONE > >>> Option B = not waiting for the MSA_READY indicator when > >>> qcom,no-msa-ready-indicator is defined > >>> Option C = not waiting for the MSA_READY indicator for certain > >>> platforms (based on root compatible) > >>> Option D = some other solution not yet discussed > >> > >> After firmware-N.bin solution didn't work (sorry about that!) my > >> preference is option B. > > > > Actually, Option B is this patch series. > > Could you formally review it? > > I'm happy with this series and would take it to ath.git, just need an > ack from DT maintainers: As far as I can tell, Krzysztof wanted a commit message update for 1/3. Usually this dts patch would go via the qcom dts tree, so presumably there's a need for an Ack from Bjorn or Konrad? > > https://patchwork.kernel.org/project/linux-wireless/patch/84f20fb5-5d48-419c-8eff-d7044afb81c0@freebox.fr/ > > > Perhaps one thing I could do slightly differently is to NOT call > > ath10k_qmi_event_msa_ready() a second time if we DO receive the > > indicator later. > > Good point. And maybe add an ath10k_warn() message so that we notice if > there's a mismatch. > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi 2024-04-25 16:02 ` Conor Dooley @ 2024-04-25 16:39 ` Kalle Valo 0 siblings, 0 replies; 31+ messages in thread From: Kalle Valo @ 2024-04-25 16:39 UTC (permalink / raw) To: Conor Dooley Cc: Marc Gonzalez, Dmitry Baryshkov, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson, ath10k, wireless, DT, MSM, Rob Herring, Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Jami Kettunen, Marijn Suijten, Jeffrey Hugo Conor Dooley <conor@kernel.org> writes: > On Thu, Apr 25, 2024 at 06:42:16PM +0300, Kalle Valo wrote: > >> Marc Gonzalez <mgonzalez@freebox.fr> writes: >> >> > On 25/04/2024 11:42, Kalle Valo wrote: >> > >> >> Marc Gonzalez wrote: >> >> >> >>> Do you prefer: >> >>> >> >>> Option A = never waiting for the MSA_READY indicator for ANYONE >> >>> Option B = not waiting for the MSA_READY indicator when >> >>> qcom,no-msa-ready-indicator is defined >> >>> Option C = not waiting for the MSA_READY indicator for certain >> >>> platforms (based on root compatible) >> >>> Option D = some other solution not yet discussed >> >> >> >> After firmware-N.bin solution didn't work (sorry about that!) my >> >> preference is option B. >> > >> > Actually, Option B is this patch series. >> > Could you formally review it? >> >> I'm happy with this series and would take it to ath.git, just need an >> ack from DT maintainers: > > As far as I can tell, Krzysztof wanted a commit message update for > 1/3. That's my understanding as well, I assume Marc will submit v3. I marked this patchset as 'Changes Requested' in patchwork. > Usually this dts patch would go via the qcom dts tree, so presumably > there's a need for an Ack from Bjorn or Konrad? Thanks pointing this out. What I meant to say earlier that I'm happy to take patches 1-2 to ath.git but I prefer that patch 3 goes via qcom dts tree. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2024-04-25 16:40 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-28 17:24 [PATCH v2 0/3] Work around missing MSA_READY indicator for msm8998 devices Marc Gonzalez 2024-03-28 17:36 ` [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop Marc Gonzalez 2024-03-30 18:20 ` Krzysztof Kozlowski 2024-03-30 18:23 ` Krzysztof Kozlowski 2024-03-30 22:04 ` Marc Gonzalez 2024-04-03 6:44 ` Krzysztof Kozlowski 2024-03-28 17:38 ` [PATCH v2 2/3] wifi: ath10k: fake missing MSA_READY indicator Marc Gonzalez 2024-03-28 17:39 ` [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi Marc Gonzalez 2024-03-30 18:25 ` Krzysztof Kozlowski 2024-04-02 14:34 ` Konrad Dybcio 2024-04-02 15:31 ` Marc Gonzalez 2024-04-02 15:55 ` Dmitry Baryshkov 2024-04-02 18:22 ` Jeff Johnson 2024-04-02 19:15 ` Dmitry Baryshkov 2024-04-02 18:25 ` Alexey Minnekhanov 2024-04-02 19:21 ` Dmitry Baryshkov 2024-04-02 23:32 ` Jeff Johnson 2024-04-03 13:05 ` Marc Gonzalez 2024-04-03 14:12 ` Dmitry Baryshkov 2024-04-03 18:16 ` Marc Gonzalez 2024-04-03 14:14 ` Krzysztof Kozlowski 2024-04-04 11:57 ` Kalle Valo 2024-04-04 12:30 ` Marc Gonzalez 2024-04-04 13:14 ` Dmitry Baryshkov 2024-04-04 15:28 ` Kalle Valo 2024-04-08 15:53 ` Marc Gonzalez 2024-04-25 9:42 ` Kalle Valo 2024-04-25 11:48 ` Marc Gonzalez 2024-04-25 15:42 ` Kalle Valo 2024-04-25 16:02 ` Conor Dooley 2024-04-25 16:39 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).