[v3,1/3] dt: bindings: add missing dt properties for WCN3990 wifi node

Message ID 1539172376-19269-2-git-send-email-govinds@codeaurora.org
State Not Applicable
Headers show
Series
  • Enable ath10k wcn3990 wifi driver support on sdm845
Related show

Commit Message

Govind Singh Oct. 10, 2018, 11:52 a.m.
Add missing optional properties in WCN3990 wifi node.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 .../bindings/net/wireless/qcom,ath10k.txt          | 28 ++++++++++++++++------
 1 file changed, 21 insertions(+), 7 deletions(-)

Comments

Rob Herring Oct. 12, 2018, 4:18 p.m. | #1
On Wed, 10 Oct 2018 17:22:54 +0530, Govind Singh wrote:
> Add missing optional properties in WCN3990 wifi node.
> 
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  .../bindings/net/wireless/qcom,ath10k.txt          | 28 ++++++++++++++++------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Doug Anderson Oct. 16, 2018, 10:53 p.m. | #2
Hi,

On Wed, Oct 10, 2018 at 4:53 AM Govind Singh <govinds@codeaurora.org> wrote:
>
> Add missing optional properties in WCN3990 wifi node.
>
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  .../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.

B) On wcn3990-wifi these clocks should either both be there or neither.

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?

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".


> +- 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.


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").

-Doug
Stephen Boyd Oct. 17, 2018, 7:41 a.m. | #3
Quoting Govind Singh (2018-10-10 04:52:54)
> 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.
> +- interrupts: reference to the list of 17 interrupt no's for "qcom,ipq4019-wifi"

Nitpick: Can you write out "numbers" instead of "no's"?

> +             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.
> +
>  - interrupt-names: Must include the entries for MSI interrupt
>                    names ("msi0" to "msi15") and legacy interrupt
> -                  name ("legacy"),
> +                  name ("legacy") for "qcom,ath10k", "qcom,ipq4019-wifi"
> +                  compatible targets.
>  - qcom,msi_addr: MSI interrupt address.
>  - qcom,msi_base: Base value to add before writing MSI data into
>                 MSI address register.
> @@ -55,7 +63,8 @@ Optional properties:
>  - qcom,ath10k-pre-calibration-data : pre calibration data as an array,
>                                      the length can vary between hw versions.
>  - <supply-name>-supply: handle to the regulator device tree node
> -                          optional "supply-name" is "vdd-0.8-cx-mx".
> +                          optional "supply-name" are "vdd-0.8-cx-mx",
> +                          "vdd-1.8-xo", "vdd-1.3-rfa" and "vdd-3.3-ch0".

Why can't the wifi firmware manage these regulators itself?

And these names don't seem to match any sort of schematic or really
describe what this device is. From what I can tell, we've combined the
off-SoC wifi module with the on-SoC wifi I/O space into one DT node here
and then relied on that node to make some driver probe in the kernel to
do wifi stuff. Can we model this properly by actually showing that
there's something in the SoC, and there's something outside the SoC, and
these two things are connected by having two nodes and a phandle between
them?

>  
>  Example (to supply the calibration data alone):
>  
> @@ -133,8 +142,10 @@ wifi@18000000 {
>                 compatible = "qcom,wcn3990-wifi";
>                 reg = <0x18800000 0x800000>;
>                 reg-names = "membase";
> -               clocks = <&clock_gcc clk_aggre2_noc_clk>;
> -               clock-names = "smmu_aggre2_noc_clk"
> +               clocks = <&clock_gcc clk_aggre2_noc_clk>,
> +                        <&clock_gcc clk_rf_clk2_pin>;
> +               clock-names = "smmu_aggre2_noc_clk",
> +                             "cxo_ref_clk_pin";
>                 interrupts =
>                            <0 130 0 /* CE0 */ >,
>                            <0 131 0 /* CE1 */ >,

Patch

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.
+- 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.
+
 - interrupt-names: Must include the entries for MSI interrupt
 		   names ("msi0" to "msi15") and legacy interrupt
-		   name ("legacy"),
+		   name ("legacy") for "qcom,ath10k", "qcom,ipq4019-wifi"
+		   compatible targets.
 - qcom,msi_addr: MSI interrupt address.
 - qcom,msi_base: Base value to add before writing MSI data into
 		MSI address register.
@@ -55,7 +63,8 @@  Optional properties:
 - qcom,ath10k-pre-calibration-data : pre calibration data as an array,
 				     the length can vary between hw versions.
 - <supply-name>-supply: handle to the regulator device tree node
-			   optional "supply-name" is "vdd-0.8-cx-mx".
+			   optional "supply-name" are "vdd-0.8-cx-mx",
+			   "vdd-1.8-xo", "vdd-1.3-rfa" and "vdd-3.3-ch0".
 
 Example (to supply the calibration data alone):
 
@@ -133,8 +142,10 @@  wifi@18000000 {
 		compatible = "qcom,wcn3990-wifi";
 		reg = <0x18800000 0x800000>;
 		reg-names = "membase";
-		clocks = <&clock_gcc clk_aggre2_noc_clk>;
-		clock-names = "smmu_aggre2_noc_clk"
+		clocks = <&clock_gcc clk_aggre2_noc_clk>,
+			 <&clock_gcc clk_rf_clk2_pin>;
+		clock-names = "smmu_aggre2_noc_clk",
+			      "cxo_ref_clk_pin";
 		interrupts =
 			   <0 130 0 /* CE0 */ >,
 			   <0 131 0 /* CE1 */ >,
@@ -149,4 +160,7 @@  wifi@18000000 {
 			   <0 140 0 /* CE10 */ >,
 			   <0 141 0 /* CE11 */ >;
 		vdd-0.8-cx-mx-supply = <&pm8998_l5>;
+		vdd-1.8-xo-supply = <&vreg_l7a_1p8>;
+		vdd-1.3-rfa-supply = <&vreg_l17a_1p3>;
+		vdd-3.3-ch0-supply = <&vreg_l25a_3p3>;
 };