| Message ID | ebbda69c-63c1-4003-bf97-c3adf3ccb9e3@freebox.fr |
|---|---|
| Headers | show |
| Series | Work around missing MSA_READY indicator for msm8998 devices | expand |
On 29/04/2024 15:01, Marc Gonzalez wrote: > Work around missing MSA_READY indicator in ath10k driver > (apply work-around for all msm8998 devices) > > CHANGELOG v3 > - Add a paragraph in binding commit to explain why we use > a DT property instead of a firmware feature bit. > - Warn if the "no_msa_ready_indicator" property is true, > but we actually receive the indicator. > > Marc Gonzalez (3): > dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop > wifi: ath10k: do not always wait for MSA_READY indicator > arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi > > Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml | 5 +++++ > arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 + > drivers/net/wireless/ath/ath10k/qmi.c | 11 +++++++++++ > drivers/net/wireless/ath/ath10k/qmi.h | 1 + > 4 files changed, 18 insertions(+) > I wonder if you could infer the workaround based on firmware version, instead of kernel passed flag ? --- bod
On Tue, Apr 30, 2024 at 12:24:40AM +0100, Bryan O'Donoghue wrote: > On 29/04/2024 15:01, Marc Gonzalez wrote: > > Work around missing MSA_READY indicator in ath10k driver > > (apply work-around for all msm8998 devices) > > > > CHANGELOG v3 > > - Add a paragraph in binding commit to explain why we use > > a DT property instead of a firmware feature bit. > > - Warn if the "no_msa_ready_indicator" property is true, > > but we actually receive the indicator. > > > > Marc Gonzalez (3): > > dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop > > wifi: ath10k: do not always wait for MSA_READY indicator > > arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi > > > > Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml | 5 +++++ > > arch/arm64/boot/dts/qcom/msm8998.dtsi | 1 + > > drivers/net/wireless/ath/ath10k/qmi.c | 11 +++++++++++ > > drivers/net/wireless/ath/ath10k/qmi.h | 1 + > > 4 files changed, 18 insertions(+) > > > > I wonder if you could infer the workaround based on firmware version, > instead of kernel passed flag ? > It's been a while, but I attempted this for the similar workaround for SDM845 RB3 et al. I vaguely remember concluding that the different devices I worked on, had firmware from different branches without any suitable version scheme to use for such comparison - which is why we have "qcom,snoc-host-cap-8bit-quirk;" in those nodes (which apparently is not documented in the yaml). I'd also rather see us ask Marc spending time on some more fruitful exercise... Regards, Bjorn
On 30/04/2024 06:06, Kalle Valo wrote: > Bjorn Andersson wrote: > >> On Mon, Apr 29, 2024 at 04:04:51PM +0200, 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. >>> >>> Kalle Valo first suggested setting a bit in firmware-5.bin to trigger >>> work-around in the driver. However, firmware-5.bin is parsed too late. >>> So we are stuck with a DT property. >>> >>> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr> >>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> >> >> This says "Pierre-Hugues certifies the origin of the patch" then "Marc >> certifies the origin of the patch". This would have to imply that >> Pierre-Hugues authored the patch, but you're listed as the author... >> >> Perhaps a suitable answer to this question would be to add >> "Co-developed-by: Pierre-Hugues ..." above his s-o-b, which implies that >> the two of you jointly came up with this and both certify the origin. > > BTW I can add that in the pending branch, no need to resend because of > this. Just need guidance from Marc. I typed this patch all by myself with my grubby little paws. You can drop PH's S-o-b. >> Other than that, I think this looks good, so please upon addressing this >> problem feel free to add my: >> >> Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > Thanks, I'll then add this as well. Cool. Almost there :)
On 4/29/2024 7:04 AM, 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. > > Kalle Valo first suggested setting a bit in firmware-5.bin to trigger > work-around in the driver. However, firmware-5.bin is parsed too late. > So we are stuck with a DT property. > > 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 | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml > index 9b3ef4bc37325..9070a41f7fc07 100644 > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml > @@ -122,6 +122,11 @@ properties: > Whether to skip executing an SCM call that reassigns the memory > region ownership. > > + qcom,no-msa-ready-indicator: > + type: boolean > + description: > + Don't wait for MSA_READY indicator to complete init. > + > qcom,smem-states: > $ref: /schemas/types.yaml#/definitions/phandle-array > description: State bits used by the AP to signal the WLAN Q6. with SOB cleaned up: Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 4/29/2024 7:07 AM, 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. > > cf. also > https://wiki.postmarketos.org/wiki/Qualcomm_Snapdragon_835_(MSM8998)#WLAN > > Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Marc Gonzalez <mgonzalez@freebox.fr> writes: > On 30/04/2024 06:06, Kalle Valo wrote: > >> Bjorn Andersson wrote: >> >>> On Mon, Apr 29, 2024 at 04:04:51PM +0200, 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. >>>> >>>> Kalle Valo first suggested setting a bit in firmware-5.bin to trigger >>>> work-around in the driver. However, firmware-5.bin is parsed too late. >>>> So we are stuck with a DT property. >>>> >>>> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr> >>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> >>> >>> This says "Pierre-Hugues certifies the origin of the patch" then "Marc >>> certifies the origin of the patch". This would have to imply that >>> Pierre-Hugues authored the patch, but you're listed as the author... >>> >>> Perhaps a suitable answer to this question would be to add >>> "Co-developed-by: Pierre-Hugues ..." above his s-o-b, which implies that >>> the two of you jointly came up with this and both certify the origin. >> >> BTW I can add that in the pending branch, no need to resend because of >> this. Just need guidance from Marc. > > I typed this patch all by myself with my grubby little paws. > You can drop PH's S-o-b. > >>> Other than that, I think this looks good, so please upon addressing this >>> problem feel free to add my: >>> >>> Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com> >> >> Thanks, I'll then add this as well. > > Cool. Almost there :) All I need is an ack from DT maintainers for this patch. DT maintainers: I think this is the best option and I can't think of any other solution so I would prefer to take this approach to our ath.git tree if it's ok for you. IIRC someone suggested testing for firmware version string but I suspect that has the same problem as the firmware-N.bin approach: ath10k gets the firmware version too late. And besides it's difficult to maintain such a list in ath10k, it would always need kernel updates when there's a new firmware etc.
Marc Gonzalez <mgonzalez@freebox.fr> writes: > On 30/04/2024 06:06, Kalle Valo wrote: > >> Bjorn Andersson wrote: >> >>> On Mon, Apr 29, 2024 at 04:04:51PM +0200, 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. >>>> >>>> Kalle Valo first suggested setting a bit in firmware-5.bin to trigger >>>> work-around in the driver. However, firmware-5.bin is parsed too late. >>>> So we are stuck with a DT property. >>>> >>>> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr> >>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr> >>> >>> This says "Pierre-Hugues certifies the origin of the patch" then "Marc >>> certifies the origin of the patch". This would have to imply that >>> Pierre-Hugues authored the patch, but you're listed as the author... >>> >>> Perhaps a suitable answer to this question would be to add >>> "Co-developed-by: Pierre-Hugues ..." above his s-o-b, which implies that >>> the two of you jointly came up with this and both certify the origin. >> >> BTW I can add that in the pending branch, no need to resend because of >> this. Just need guidance from Marc. > > I typed this patch all by myself with my grubby little paws. > You can drop PH's S-o-b. Thanks. Please check that my modifications in the pending branch are correct: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=3aec20a8e797b28d32e75291cc070d5913bf6dab https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=df5b4bec31b0736a453d507762c5b3d098d5c733 I can freely edit commits in the pending branch, it's just a temporary branch for testing.
On 07/05/2024 19:03, Kalle Valo wrote: > Thanks. Please check that my modifications in the pending branch are > correct: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=3aec20a8e797b28d32e75291cc070d5913bf6dab > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=df5b4bec31b0736a453d507762c5b3d098d5c733 > > I can freely edit commits in the pending branch, it's just a temporary > branch for testing. Looks good to me. Bjorn, can you take patch 3 in your fix branch for msm? Regards
On Mon, 29 Apr 2024 16:01:41 +0200, Marc Gonzalez wrote: > Work around missing MSA_READY indicator in ath10k driver > (apply work-around for all msm8998 devices) > > CHANGELOG v3 > - Add a paragraph in binding commit to explain why we use > a DT property instead of a firmware feature bit. > - Warn if the "no_msa_ready_indicator" property is true, > but we actually receive the indicator. > > [...] Applied, thanks! [3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi commit: 737abcabe97bb37e38be2504acd28ad779dbaf3d Best regards,