Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On 01/03/2024 09:10, Kalle Valo wrote:
> Marc Gonzalez wrote:
>
>> Kalle Valo wrote:
>>
>>> Here's one example where in ath10k we use a feature bit as a workaround:
>>>
>>> /* Don't trust error code from otp.bin */
>>> ATH10K_FW_FEATURE_IGNORE_OTP_RESULT = 7,
>>>
>>>
>>>
>>> if (!(skip_otp || test_bit(ATH10K_FW_FEATURE_IGNORE_OTP_RESULT,
>>>ar->running_fw->fw_file.fw_features)) &&
>>> result != 0) {
>>> ath10k_err(ar, "otp calibration failed: %d", result);
>>> return -EINVAL;
>>> }
>>>
>>> BTW for modifying firmware-N.bin files we have a script here:
>>>
>>> https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-fwencoder
>>
>> If I understand correctly, you are saying that there is
>> (maybe... probably) a bug in the FW, so it makes sense to
>> tag that specific FW file with a special bit which the kernel
>> will interpret as "this FW is broken in a specific way;
>> and here's how to work around the issue."
>>
>> So this bit would serve the same purpose as my proposed
>> "qcom,no-msa-ready-indicator" bit (that bit existed instead
>> in my board's device tree).
>>
>> The problem I see is that the firmware files are signed.
>> Thus, changing a single bit breaks the verification...
>> UNLESS the FW format allows for a signed section ALONG-SIDE
>> an unsigned section?
>
> firmware-N.bin is ath10k specific container file format and we (the
> Linux community) have full access to it using ath10k-fwencoder, there's
> no signing or anything like that. One of the major reasons why it was
> designed was to handle differences between firmware branches, just like
> in this case.
>
> Of course plan A should be to fix the firmware but if that doesn't work
> out then plan B could be using the feature bit in firmware-N.bin.
>
> BTW related to this Dmitry is extending firmware-N.bin handling for
> WCN3990, you will most likely need to use that:
>
> https://patchwork.kernel.org/project/linux-wireless/cover/20240130-wcn3990-firmware-path-v1-0-826b93202...@linaro.org/
If I understand correctly (happy to have anyone correct any
misunderstandings), if the FW cannot be fixed (for any reason),
then we would have to do something like this:
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index 0032f8aa892ff..c8778ebe922af 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -769,6 +769,7 @@ static const char *const ath10k_core_fw_feature_str[] = {
[ATH10K_FW_FEATURE_SINGLE_CHAN_INFO_PER_CHANNEL] =
"single-chan-info-per-channel",
[ATH10K_FW_FEATURE_PEER_FIXED_RATE] = "peer-fixed-rate",
[ATH10K_FW_FEATURE_IRAM_RECOVERY] = "iram-recovery",
+ [ATH10K_FW_FEATURE_NO_MSA_READY] = "no-msa-ready-indicator",
};
static unsigned int ath10k_core_get_fw_feature_str(char *buf,
@@ -2941,6 +2942,9 @@ int ath10k_core_start(struct ath10k *ar, enum
ath10k_firmware_mode mode,
ar->running_fw = fw;
+ if (test_bit(ATH10K_FW_FEATURE_NO_MSA_READY, fw->fw_file.fw_features))
+ ar->fake_msa_ready = true;
+
if (!test_bit(ATH10K_FW_FEATURE_NON_BMI,
ar->running_fw->fw_file.fw_features)) {
ath10k_bmi_start(ar);
diff --git a/drivers/net/wireless/ath/ath10k/core.h
b/drivers/net/wireless/ath/ath10k/core.h
index c110d15528bd0..ce71097b97a1d 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -835,6 +835,9 @@ enum ath10k_fw_features {
/* Firmware support IRAM recovery */
ATH10K_FW_FEATURE_IRAM_RECOVERY = 22,
+ /* Firware does not send MSA_READY indicator */
+ ATH10K_FW_FEATURE_NO_MSA_READY = 23,
+
/* keep last */
ATH10K_FW_FEATURE_COUNT,
};
@@ -1151,6 +1154,9 @@ struct ath10k {
u8 cfg_tx_chainmask;
u8 cfg_rx_chainmask;
+ /* FW does not send MSA_READY indicator. Fake it */
+ bool fake_msa_ready; /* bool or u8? or s8? or bitfield? */
+
struct completion install_key_done;
int last_wmi_vdev_start_status;
diff --git a/drivers/net/wireless/ath/ath10k/qmi.c
b/drivers/net/wireless/ath/ath10k/qmi.c
index 38e939f572a9e..0776e79b25f3a 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);
+ if (ar->fake_msa_ready)
+ ath10k_qmi_event_msa_ready(qmi);
break;
case ATH10K_QMI_EVENT_SERVER_EXIT:
ath10k_qmi_event_server_exit(qmi);
--
Regards
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On 29/02/2024 19:40, Conor Dooley wrote: > On Wed, Feb 28, 2024 at 06:37:08PM +0200, Kalle Valo wrote: > >> Marc Gonzalez wrote: >> >>> As mentioned in my other reply, there are several msm8998-based >>> devices affected by this issue. Is it not appropriate to consider >>> a kernel-based work-around? >> >> Sorry, not following you here. But I'll try to answer anyway: >> >> I have understood that Device Tree is supposed to describe hardware, not >> software. This is why having this property in DT does not look right >> place for this. For example, if the ath10k firmware is fixed then DT >> would have to be changed even though nothing changed in hardware. But of >> course DT maintainers have the final say. > > I dunno, if the firmware affects the functionality of the hardware in a > way that cannot be detected from the operating system at runtime how > else is it supposed to deal with that? > The devicetree is supposed to describe hardware, yes, but at a certain > point the line between firmware and hardware is invisible :) > Not describing software is mostly about not using it to determine > software policy in the operating system. Recording here what was discussed a few days ago on IRC: If all msm8998 boards are affected, then it /might/ make sense to work around the issue for ALL msm8998 boards: diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 0776e79b25f3a..9da06da518fb6 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -1076,6 +1076,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size) qmi->ar = ar; ar_snoc->qmi = qmi; + if (of_device_is_compatible(of_root, "qcom,msm8998") + qmi->no_point_in_waiting_for_msa_ready_indicator = true; + if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm")) qmi->msa_fixed_perm = true; Thus, anyone porting an msm8998 board to mainline would automatically get the work-around, without having to hunt down the feature bit, and tweak the FW files. -- Regards
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote: > On 29/02/2024 19:40, Conor Dooley wrote: > > > On Wed, Feb 28, 2024 at 06:37:08PM +0200, Kalle Valo wrote: > > > >> Marc Gonzalez wrote: > >> > >>> As mentioned in my other reply, there are several msm8998-based > >>> devices affected by this issue. Is it not appropriate to consider > >>> a kernel-based work-around? > >> > >> Sorry, not following you here. But I'll try to answer anyway: > >> > >> I have understood that Device Tree is supposed to describe hardware, not > >> software. This is why having this property in DT does not look right > >> place for this. For example, if the ath10k firmware is fixed then DT > >> would have to be changed even though nothing changed in hardware. But of > >> course DT maintainers have the final say. > > > > I dunno, if the firmware affects the functionality of the hardware in a > > way that cannot be detected from the operating system at runtime how > > else is it supposed to deal with that? > > The devicetree is supposed to describe hardware, yes, but at a certain > > point the line between firmware and hardware is invisible :) > > Not describing software is mostly about not using it to determine > > software policy in the operating system. > > Recording here what was discussed a few days ago on IRC: > > If all msm8998 boards are affected, then it /might/ make sense > to work around the issue for ALL msm8998 boards: > > diff --git a/drivers/net/wireless/ath/ath10k/qmi.c > b/drivers/net/wireless/ath/ath10k/qmi.c > index 0776e79b25f3a..9da06da518fb6 100644 > --- a/drivers/net/wireless/ath/ath10k/qmi.c > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > @@ -1076,6 +1076,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size) > qmi->ar = ar; > ar_snoc->qmi = qmi; > > + if (of_device_is_compatible(of_root, "qcom,msm8998") > + qmi->no_point_in_waiting_for_msa_ready_indicator = true; > + > if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm")) > qmi->msa_fixed_perm = true; > > > Thus, anyone porting an msm8998 board to mainline would automatically > get the work-around, without having to hunt down the feature bit, > and tweak the FW files. 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) signature.asc Description: PGP signature
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On Mon, 4 Mar 2024 at 21:34, Conor Dooley wrote: > > On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote: > > On 29/02/2024 19:40, Conor Dooley wrote: > > > > > On Wed, Feb 28, 2024 at 06:37:08PM +0200, Kalle Valo wrote: > > > > > >> Marc Gonzalez wrote: > > >> > > >>> As mentioned in my other reply, there are several msm8998-based > > >>> devices affected by this issue. Is it not appropriate to consider > > >>> a kernel-based work-around? > > >> > > >> Sorry, not following you here. But I'll try to answer anyway: > > >> > > >> I have understood that Device Tree is supposed to describe hardware, not > > >> software. This is why having this property in DT does not look right > > >> place for this. For example, if the ath10k firmware is fixed then DT > > >> would have to be changed even though nothing changed in hardware. But of > > >> course DT maintainers have the final say. > > > > > > I dunno, if the firmware affects the functionality of the hardware in a > > > way that cannot be detected from the operating system at runtime how > > > else is it supposed to deal with that? > > > The devicetree is supposed to describe hardware, yes, but at a certain > > > point the line between firmware and hardware is invisible :) > > > Not describing software is mostly about not using it to determine > > > software policy in the operating system. > > > > Recording here what was discussed a few days ago on IRC: > > > > If all msm8998 boards are affected, then it /might/ make sense > > to work around the issue for ALL msm8998 boards: > > > > diff --git a/drivers/net/wireless/ath/ath10k/qmi.c > > b/drivers/net/wireless/ath/ath10k/qmi.c > > index 0776e79b25f3a..9da06da518fb6 100644 > > --- a/drivers/net/wireless/ath/ath10k/qmi.c > > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > > @@ -1076,6 +1076,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size) > > qmi->ar = ar; > > ar_snoc->qmi = qmi; > > > > + if (of_device_is_compatible(of_root, "qcom,msm8998") > > + qmi->no_point_in_waiting_for_msa_ready_indicator = true; > > + > > if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm")) > > qmi->msa_fixed_perm = true; > > > > > > Thus, anyone porting an msm8998 board to mainline would automatically > > get the work-around, without having to hunt down the feature bit, > > and tweak the FW files. > > How come the root node comes into this, don't you have a soc-specific > compatible for the integration on this SoC? No. Ath10k uses WiFi SoC as an SoC designator rather than the main SoC. My 2c: I think it's easier to fix the firmware features bits, it's more future proof (and error proof). > (I am assuming that this is not the SDIO variant, given then it'd not be > fixed to this particular implementation) -- With best wishes Dmitry
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On Mon, Mar 04, 2024 at 09:37:00PM +0200, Dmitry Baryshkov wrote: > On Mon, 4 Mar 2024 at 21:34, Conor Dooley wrote: > > > > On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote: > > > On 29/02/2024 19:40, Conor Dooley wrote: > > > > > > > On Wed, Feb 28, 2024 at 06:37:08PM +0200, Kalle Valo wrote: > > > > > > > >> Marc Gonzalez wrote: > > > >> > > > >>> As mentioned in my other reply, there are several msm8998-based > > > >>> devices affected by this issue. Is it not appropriate to consider > > > >>> a kernel-based work-around? > > > >> > > > >> Sorry, not following you here. But I'll try to answer anyway: > > > >> > > > >> I have understood that Device Tree is supposed to describe hardware, > > > >> not > > > >> software. This is why having this property in DT does not look right > > > >> place for this. For example, if the ath10k firmware is fixed then DT > > > >> would have to be changed even though nothing changed in hardware. But > > > >> of > > > >> course DT maintainers have the final say. > > > > > > > > I dunno, if the firmware affects the functionality of the hardware in a > > > > way that cannot be detected from the operating system at runtime how > > > > else is it supposed to deal with that? > > > > The devicetree is supposed to describe hardware, yes, but at a certain > > > > point the line between firmware and hardware is invisible :) > > > > Not describing software is mostly about not using it to determine > > > > software policy in the operating system. > > > > > > Recording here what was discussed a few days ago on IRC: > > > > > > If all msm8998 boards are affected, then it /might/ make sense > > > to work around the issue for ALL msm8998 boards: > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/qmi.c > > > b/drivers/net/wireless/ath/ath10k/qmi.c > > > index 0776e79b25f3a..9da06da518fb6 100644 > > > --- a/drivers/net/wireless/ath/ath10k/qmi.c > > > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > > > @@ -1076,6 +1076,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size) > > > qmi->ar = ar; > > > ar_snoc->qmi = qmi; > > > > > > + if (of_device_is_compatible(of_root, "qcom,msm8998") > > > + qmi->no_point_in_waiting_for_msa_ready_indicator = true; > > > + > > > if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm")) > > > qmi->msa_fixed_perm = true; > > > > > > > > > Thus, anyone porting an msm8998 board to mainline would automatically > > > get the work-around, without having to hunt down the feature bit, > > > and tweak the FW files. > > > > How come the root node comes into this, don't you have a soc-specific > > compatible for the integration on this SoC? > > No. Ath10k uses WiFi SoC as an SoC designator rather than the main SoC. Suitability of either fix aside, can you explain this to me? Is the "WiFi SoC" accessible from the "main SoC" at a regular MMIO address? The "ath10k" compatible says it is SDIO-based & the other two compatibles seem to be MMIO. signature.asc Description: PGP signature
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On Mon, 4 Mar 2024 at 21:46, Conor Dooley wrote: > > On Mon, Mar 04, 2024 at 09:37:00PM +0200, Dmitry Baryshkov wrote: > > On Mon, 4 Mar 2024 at 21:34, Conor Dooley wrote: > > > > > > On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote: > > > > On 29/02/2024 19:40, Conor Dooley wrote: > > > > > > > > > On Wed, Feb 28, 2024 at 06:37:08PM +0200, Kalle Valo wrote: > > > > > > > > > >> Marc Gonzalez wrote: > > > > >> > > > > >>> As mentioned in my other reply, there are several msm8998-based > > > > >>> devices affected by this issue. Is it not appropriate to consider > > > > >>> a kernel-based work-around? > > > > >> > > > > >> Sorry, not following you here. But I'll try to answer anyway: > > > > >> > > > > >> I have understood that Device Tree is supposed to describe hardware, > > > > >> not > > > > >> software. This is why having this property in DT does not look right > > > > >> place for this. For example, if the ath10k firmware is fixed then DT > > > > >> would have to be changed even though nothing changed in hardware. > > > > >> But of > > > > >> course DT maintainers have the final say. > > > > > > > > > > I dunno, if the firmware affects the functionality of the hardware in > > > > > a > > > > > way that cannot be detected from the operating system at runtime how > > > > > else is it supposed to deal with that? > > > > > The devicetree is supposed to describe hardware, yes, but at a certain > > > > > point the line between firmware and hardware is invisible :) > > > > > Not describing software is mostly about not using it to determine > > > > > software policy in the operating system. > > > > > > > > Recording here what was discussed a few days ago on IRC: > > > > > > > > If all msm8998 boards are affected, then it /might/ make sense > > > > to work around the issue for ALL msm8998 boards: > > > > > > > > diff --git a/drivers/net/wireless/ath/ath10k/qmi.c > > > > b/drivers/net/wireless/ath/ath10k/qmi.c > > > > index 0776e79b25f3a..9da06da518fb6 100644 > > > > --- a/drivers/net/wireless/ath/ath10k/qmi.c > > > > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > > > > @@ -1076,6 +1076,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 > > > > msa_size) > > > > qmi->ar = ar; > > > > ar_snoc->qmi = qmi; > > > > > > > > + if (of_device_is_compatible(of_root, "qcom,msm8998") > > > > + qmi->no_point_in_waiting_for_msa_ready_indicator = true; > > > > + > > > > if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm")) > > > > qmi->msa_fixed_perm = true; > > > > > > > > > > > > Thus, anyone porting an msm8998 board to mainline would automatically > > > > get the work-around, without having to hunt down the feature bit, > > > > and tweak the FW files. > > > > > > How come the root node comes into this, don't you have a soc-specific > > > compatible for the integration on this SoC? > > > > No. Ath10k uses WiFi SoC as an SoC designator rather than the main SoC. > > Suitability of either fix aside, can you explain this to me? Is the "WiFi > SoC" accessible from the "main SoC" at a regular MMIO address? The > "ath10k" compatible says it is SDIO-based & the other two compatibles > seem to be MMIO. Yes, this is correct. MSM8996 uses PCI to access WiFi chip, MSM8998 uses MMIO. -- With best wishes Dmitry
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On Mon, Mar 04, 2024 at 09:59:13PM +0200, Dmitry Baryshkov wrote: > On Mon, 4 Mar 2024 at 21:46, Conor Dooley wrote: > > On Mon, Mar 04, 2024 at 09:37:00PM +0200, Dmitry Baryshkov wrote: > > > On Mon, 4 Mar 2024 at 21:34, Conor Dooley wrote: > > > > On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote: > > > > > Thus, anyone porting an msm8998 board to mainline would automatically > > > > > get the work-around, without having to hunt down the feature bit, > > > > > and tweak the FW files. > > > > > > > > How come the root node comes into this, don't you have a soc-specific > > > > compatible for the integration on this SoC? > > > > > > No. Ath10k uses WiFi SoC as an SoC designator rather than the main SoC. > > > > Suitability of either fix aside, can you explain this to me? Is the "WiFi > > SoC" accessible from the "main SoC" at a regular MMIO address? The > > "ath10k" compatible says it is SDIO-based & the other two compatibles > > seem to be MMIO. > > Yes, this is correct. MSM8996 uses PCI to access WiFi chip, MSM8998 uses MMIO. Thanks. 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. signature.asc Description: PGP signature
Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
On Mon, 4 Mar 2024 at 22:17, Conor Dooley wrote: > > On Mon, Mar 04, 2024 at 09:59:13PM +0200, Dmitry Baryshkov wrote: > > On Mon, 4 Mar 2024 at 21:46, Conor Dooley wrote: > > > On Mon, Mar 04, 2024 at 09:37:00PM +0200, Dmitry Baryshkov wrote: > > > > On Mon, 4 Mar 2024 at 21:34, Conor Dooley wrote: > > > > > On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote: > > > > > > > Thus, anyone porting an msm8998 board to mainline would > > > > > > automatically > > > > > > get the work-around, without having to hunt down the feature bit, > > > > > > and tweak the FW files. > > > > > > > > > > How come the root node comes into this, don't you have a soc-specific > > > > > compatible for the integration on this SoC? > > > > > > > > No. Ath10k uses WiFi SoC as an SoC designator rather than the main SoC. > > > > > > Suitability of either fix aside, can you explain this to me? Is the "WiFi > > > SoC" accessible from the "main SoC" at a regular MMIO address? The > > > "ath10k" compatible says it is SDIO-based & the other two compatibles > > > seem to be MMIO. > > > > Yes, this is correct. MSM8996 uses PCI to access WiFi chip, MSM8998 uses > > MMIO. > > Thanks. > > 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. We can probably start with "historically established" here. From the hardware point of view msm8998, sdm845 and several other chipsets use a variant of the same wcn3990 WiFi+BT chip. The actual issue is in the DSP firmware, so it can be handled via the firmware-related means. On the other hand, I see qcom,snoc-host-cap-8bit-quirk property, so one can use DT properties to describe the issue. -- With best wishes Dmitry
