On 2018-10-17 04:23, Doug Anderson wrote:
Hi,
On Wed, Oct 10, 2018 at 4:53 AM Govind Singh <govinds@xxxxxxxxxxxxxx>
wrote:
Add missing optional properties in WCN3990 wifi node.
Signed-off-by: Govind Singh <govinds@xxxxxxxxxxxxxx>
---
.../bindings/net/wireless/qcom,ath10k.txt | 28
++++++++++++++++------
1 file changed, 21 insertions(+), 7 deletions(-)
Point of order: please CC LKML on _all_ your patches. Yes, it's a
firehose. CCing LKML allows your patches to be found on
lore.kernel.org's patchwork and also allows people to find your
patches via <https://lkml.kernel.org/r/MSG_ID> links.
diff --git
a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
index 7fd4e8c..f831bb1 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
@@ -37,12 +37,20 @@ Optional properties:
- clocks: List of clock specifiers, must contain an entry for each
required
entry in clock-names.
- clock-names: Should contain the clock names "wifi_wcss_cmd",
"wifi_wcss_ref",
- "wifi_wcss_rtc".
-- interrupts: List of interrupt lines. Must contain an entry
- for each entry in the interrupt-names property.
+ "wifi_wcss_rtc" for "qcom,ipq4019-wifi" compatible
target and
+ "cxo_ref_clk_pin", "smmu_aggre2_noc_clk" for
"qcom,wcn3990-wifi"
+ compatible target.
I always get confused with these big bindings patches that hide
everything under a big "Optional properties" section to avoid
specifying which properties are actually optional and which ones are
required.
After your patch and thinking about "qcom,wcn3990-wifi" in particular,
it's unclear to me which of the following is true (or maybe something
totally different I didn't think of)
A) On wcn3990-wifi these clocks should be a required property but it's
only listed under "Optional" because it's not used for some of the
different WiFi drivers using this same bindings.
These clocks are optional as it is voted by firmware in newer fw
versions.
During transient state in case of fw crash, fw might remove the vote in
its error/fatal handler. The apps vote helps in avoiding un-clocked
hw(copy engine)
access in transient state till driver recovers.
B) On wcn3990-wifi these clocks should either both be there or neither.
With the above explanation can you suggest where these controls should
fall.
C) On wcm3990-wifi you can specify zero, either, or both of these
clocks. AKA they are independently optional.
It might make sense to reorganize this bindings to make this clearer?
...not just for clock but for interrupts / regulators as well. Maybe
you need to break this down into sections per class of compatible
string, or add a list per compatible string down below?
can you pls point me to some reference for the change you are expecting.
I will check and rework accordingly.
Also: even stranger is that even though you list two clocks here the
current driver I see in linuxnext only has "cxo_ref_clk_pin".
smmu_aggre2_noc_clk is not applicable to SDM845 and required for other
msm platforms.
I will remove smmu_aggre2_noc_clk reference and add when this clock is
available
in upstream for respective target.
+- interrupts: reference to the list of 17 interrupt no's for
"qcom,ipq4019-wifi"
+ compatible target.
+ reference to the list of 12 interrupt no's for
"qcom,wcn3990-wifi"
+ compatible target.
+ Must contain interrupt-names property per entry for
+ "qcom,ath10k", "qcom,ipq4019-wifi" compatible targets.
...and just to add some credence to my concerns above, "interrupts"
are currently listed under "Optional" properties but I don't think
that the wcn3990 driver will actually work if you don't specify any of
the interrupts, right? AKA for wcn3990 they are _not_ optional and
you must have exactly 12 interrupts.
Yes, for other chip-set(QCAxx) also it should not be optional.
I will move interrupt block to Required properties.
One separate issue I have is with your example, which you didn't
change in this patch. You should fix the example with the same
feedback that I had to your patch ("dts: arm64/sdm845: Add WCN3990
WLAN module device node").
sure , will do in next revision.
-Doug
