[PATCH v3 0/3] Enable ath10k wcn3990 wifi driver support on sdm845

[PATCH v3 0/3] Enable ath10k wcn3990 wifi driver support on sdm845

[Govind Singh]

This series enables ath10k wifi driver support for WCN3990 target
on sdm845 SOC. This series also updates the missing dt binding documentation
and adds optional iommu property.

Changes since v2:
    dropped [v2,4/4] dts: arm64/sdm845: Enable iommu for WCN3990 wifi module
    device node patch from the series as dependent patch is not yet merged.
    Enabled status flag from sdm845-mtp.dts.

Changes since v1:
    Listed no of interrupts/clocks for each set of compatible.
    Added missing 'wifi' label to sdm845.dtsi.

Govind Singh (3):
  dt: bindings: add missing dt properties for WCN3990 wifi node
  dts: arm64/sdm845: Add WCN3990 WLAN module device node
  dt: bindings: add bindings for wifi iommu node

 .../bindings/net/wireless/qcom,ath10k.txt          | 33 +++++++++++++++++-----
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts            |  8 ++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               | 26 +++++++++++++++++
 3 files changed, 60 insertions(+), 7 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Govind Singh]

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(-)

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>;
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/3] dts: arm64/sdm845: Add WCN3990 WLAN module device node

[Govind Singh]

Add device node for the ath10k SNOC platform driver probe
and add resources required for WCN3990 on SDM845 soc.

Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts |  8 ++++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi    | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index eedfaf8..b89b8dd 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -417,6 +417,14 @@
 	vdda-pll-supply = <&vdda_usb2_ss_core>;
 };
 
+&wifi {
+	status = "okay";
+	vdd-0.8-cx-mx-supply = <&vreg_l5a_0p8>;
+	vdd-1.8-xo-supply = <&vreg_l7a_1p8>;
+	vdd-1.3-rfa-supply = <&vreg_l17a_1p3>;
+	vdd-3.3-ch0-supply = <&vreg_l25a_3p3>;
+};
+
 /* PINCTRL - additions to nodes defined in sdm845.dtsi */
 
 &qup_i2c10_default {
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index b72bdb0..a09f7df 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -87,6 +87,11 @@
 			reg = <0 0x86200000 0 0x2d00000>;
 			no-map;
 		};
+
+		wlan_msa_mem: reserved-memory@96700000 {
+			no-map;
+			reg = <0 0x96700000 0 0x100000>;
+		};
 	};
 
 	cpus {
@@ -1403,5 +1408,26 @@
 				status = "disabled";
 			};
 		};
+
+		wifi: wifi@18800000 {
+			compatible = "qcom,wcn3990-wifi";
+			status = "disabled";
+			reg = <0x18800000 0x800000>;
+			reg-names = "membase";
+			memory-region = <&wlan_msa_mem>;
+			interrupts =
+				<0 413 0 /* CE0 */ >,
+				<0 414 0 /* CE1 */ >,
+				<0 415 0 /* CE2 */ >,
+				<0 416 0 /* CE3 */ >,
+				<0 417 0 /* CE4 */ >,
+				<0 418 0 /* CE5 */ >,
+				<0 420 0 /* CE6 */ >,
+				<0 421 0 /* CE7 */ >,
+				<0 422 0 /* CE8 */ >,
+				<0 423 0 /* CE9 */ >,
+				<0 424 0 /* CE10 */ >,
+				<0 425 0 /* CE11 */ >;
+		};
 	};
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/3] dt: bindings: add bindings for wifi iommu node

[Govind Singh]

WCN3990 wifi module can optionally make use of the IOMMU.
Add binding documentation for phandle to the IOMMU and
the stream id of wifi iommu block.

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

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
index f831bb1..3118d99 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
@@ -65,6 +65,10 @@ Optional properties:
 - <supply-name>-supply: handle to the regulator device tree node
 			   optional "supply-name" are "vdd-0.8-cx-mx",
 			   "vdd-1.8-xo", "vdd-1.3-rfa" and "vdd-3.3-ch0".
+- iommus:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: A list of phandle and IOMMU specifier pairs.
 
 Example (to supply the calibration data alone):
 
@@ -163,4 +167,5 @@ wifi@18000000 {
 		vdd-1.8-xo-supply = <&vreg_l7a_1p8>;
 		vdd-1.3-rfa-supply = <&vreg_l17a_1p3>;
 		vdd-3.3-ch0-supply = <&vreg_l25a_3p3>;
+		iommus = <&apps_smmu 0x0040 0x1>;
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Rob Herring]

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>

Re: [PATCH v3 3/3] dt: bindings: add bindings for wifi iommu node

[Rob Herring]

On Wed, 10 Oct 2018 17:22:56 +0530, Govind Singh wrote:
> WCN3990 wifi module can optionally make use of the IOMMU.
> Add binding documentation for phandle to the IOMMU and
> the stream id of wifi iommu block.
> 
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 0/3] Enable ath10k wcn3990 wifi driver support on sdm845

[Brian Norris]

On Wed, Oct 10, 2018 at 05:22:53PM +0530, Govind Singh wrote:
> This series enables ath10k wifi driver support for WCN3990 target
> on sdm845 SOC. This series also updates the missing dt binding documentation
> and adds optional iommu property.

For the series:

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

Thanks!

Re: [PATCH v3 2/3] dts: arm64/sdm845: Add WCN3990 WLAN module device node

[Doug Anderson]

Hi,

On Wed, Oct 10, 2018 at 4:53 AM Govind Singh <govinds@codeaurora.org> wrote:
>
> Add device node for the ath10k SNOC platform driver probe
> and add resources required for WCN3990 on SDM845 soc.
>
> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts |  8 ++++++++
>  arch/arm64/boot/dts/qcom/sdm845.dtsi    | 26 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index eedfaf8..b89b8dd 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts

nit: it's usually considered nicer to split the patch into two: one
which adds the node to the SoC file and one which enables it in a
board file.


> +               wlan_msa_mem: reserved-memory@96700000 {

Everything else calls this "memory@", not "reserved-memory@".
Presumably someone (like Rob H?) thought that it should be "memory".
Please follow the pattern.


> +                       no-map;
> +                       reg = <0 0x96700000 0 0x100000>;

nit: though it doesn't really matter, it'd be nice to follow the
pattern and put "reg" above "no-map" like all the other nodes in this
section.


> +               };
>         };
>
>         cpus {
> @@ -1403,5 +1408,26 @@
>                                 status = "disabled";
>                         };
>                 };
> +
> +               wifi: wifi@18800000 {
> +                       compatible = "qcom,wcn3990-wifi";
> +                       status = "disabled";
> +                       reg = <0x18800000 0x800000>;
> +                       reg-names = "membase";
> +                       memory-region = <&wlan_msa_mem>;
> +                       interrupts =
> +                               <0 413 0 /* CE0 */ >,
> +                               <0 414 0 /* CE1 */ >,
> +                               <0 415 0 /* CE2 */ >,
> +                               <0 416 0 /* CE3 */ >,
> +                               <0 417 0 /* CE4 */ >,
> +                               <0 418 0 /* CE5 */ >,
> +                               <0 420 0 /* CE6 */ >,
> +                               <0 421 0 /* CE7 */ >,
> +                               <0 422 0 /* CE8 */ >,
> +                               <0 423 0 /* CE9 */ >,
> +                               <0 424 0 /* CE10 */ >,
> +                               <0 425 0 /* CE11 */ >;

Please change all the above to look like:

<GIC_SPI num IRQ_TYPE_LEVEL_HIGH>

...specifically:
* GIC_SPI is 0, but GIC_SPI is more documenting.
* having 0 for the final element means 'IRQ_TYPE_NONE'.  On newer
kernels commit 6ef6386ef7c1 ("irqchip/gic-v3: Loudly complain about
the use of IRQ_TYPE_NONE") will cause loud yells if you do this.
* I don't see what the comments about "/* CE# */ buy you.  Remove them.


-Doug

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

[Doug Anderson]

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

Re: [PATCH v3 2/3] dts: arm64/sdm845: Add WCN3990 WLAN module device node

[Stephen Boyd]

Quoting Govind Singh (2018-10-10 04:52:55)
> @@ -1403,5 +1408,26 @@
>                                 status = "disabled";
>                         };
>                 };
> +
> +               wifi: wifi@18800000 {
> +                       compatible = "qcom,wcn3990-wifi";
> +                       status = "disabled";
> +                       reg = <0x18800000 0x800000>;
> +                       reg-names = "membase";
> +                       memory-region = <&wlan_msa_mem>;
> +                       interrupts =
> +                               <0 413 0 /* CE0 */ >,

Please specify these as GIC_SPI and give trigger types (level high I
guess?). I'd also drop the comment and add interrupt-names to match the
comment. It's self documenting that way.

> +                               <0 414 0 /* CE1 */ >,
> +                               <0 415 0 /* CE2 */ >,
> +                               <0 416 0 /* CE3 */ >,
> +                               <0 417 0 /* CE4 */ >,
> +                               <0 418 0 /* CE5 */ >,
> +                               <0 420 0 /* CE6 */ >,
> +                               <0 421 0 /* CE7 */ >,
> +                               <0 422 0 /* CE8 */ >,
> +                               <0 423 0 /* CE9 */ >,
> +                               <0 424 0 /* CE10 */ >,
> +                               <0 425 0 /* CE11 */ >;

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

[Stephen Boyd]

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 */ >,

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

[Govind Singh]

On 2018-10-17 04:23, Doug Anderson wrote:
> 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.
> 
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

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

[Brian Norris]

Hi Stephen and Govind,

I was chatting with Govind, and he seemed to be a bit stalled on this.
I'd encourage him to reply with whatever knowledge he has, because it's
a bit hard to give definitive answers when I don't know all the inner
workings here. (In fact, you, Stephen, probably know more than me about
how Qualcomm MSM chips work.)

First of all, I'll explain the limited bits I do know, and see how they
fit in below. I may be wrong.

WCN3990 is an external module, for supporting BT and Wifi RF components.
It has a host interface for the Wifi, but it's not something the host
knows how to talk to directly at all -- it's an "Analog IQ" interface,
between an internal SoC MAC and PHY processor. Instead, we talk to this
module via the System NOC (?). So while there are basically 2 distinct
hardware components (on-SoC coprocessors, various internal communication
buses, various shared memory regions, etc.; and the external WCN3990
module), there is almost no way for the main processor to "talk" to the
WCN3990 directly.

Another data point to throw into the mix: WCN3990 can apparently be used
on multiple different SoCs -- I see public announcments about SDM835
(and 660?), and I'm currently playing with it on SDM845. So perhaps
there is some value in understanding "WCN3990" as being distinct from
"WiFi MAC/PHY hardware and communication logic found on a MSM SoC." But
they do seem to be very tightly coupled...

Side note: there is also a BT component on the WCN3990 module, and we
*can* talk to that directly over UART. There's a separate binding going
in for that, and it's a much clearer separation to divide "UART
controller" and "BT/UART interface on WCN3990."

On Wed, Oct 17, 2018 at 12:41:29AM -0700, Stephen Boyd wrote:
> Quoting Govind Singh (2018-10-10 04:52:54)
> > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt

> > @@ -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?

In my understanding:
At least 3 of those (all the supplies Govind is adding) are external
pins on the RF module. Why do you assume these are something the
firmware can control? In the schematics I'm looking at, this seems to be
connected to a PMIC. While it's certainly possible this is something the
Q6 processor (running modem and Wifi firmware) can control, it doesn't
seem obvious to me that they *must* be able to.

So I guess I'd say: why not represent these regulators in the device
tree? It seems like a valid hardware description (like I said, 3 power
rail pins on an external module).

Now, your *next* paragraph might have hairier points :)

> And these names don't seem to match any sort of schematic or really

Which names? I see these pin names on the WCN3990 datasheets I see:

 VDD18_IO
 VDD18_XO
 VDD33_CH0
 VDD33_CH1
 VDD13_RFA

3 of those match the 3 Govind is adding, and they appear to line up with
what I see in schematics.

> describe what this device is. From what I can tell, we've combined the

I've described "waht the device is" above to the best of my knowledge.
If you're just looking at schematics, you might possibly be thrown by
the fact that WCN3990 is packaged in a module provided by other vendors,
so it might be labeled by that vendor on the schematic, not by the
Qualcomm WCN3990 name.

> 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

I'll remind you that "properly" is often highly subjective :)

> 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?

Why phandle? Under what bus would you place the WCN3990? As per my
description above, there is really no way to talk to it directly. So if
anything, it seems like it would be a subnode of the node we're
describing here.

In favor of your separation though: there are many ways to utilize
WCN3990 apparently, and I'd imagine the binding might change a bit
depending on the SoC (e.g., different clocks?). So there might be value
in describing the SDM{835,845,...whatever}-wifi-soc-components vs.
external WCN3990. Additionally, I don't know if it's even possible to
utilize a different RF module with the same SoC (is there a possibility
of a, e.g., WCN3991 that can use the same SoC logic?).

But I'm still not totally convinced that splitting this up really makes
much sense. Is there other precedent for splitting out a separate node
for something that we don't talk to at all (no digital interface)? Or do
we just need a more accurate compatible property, that describes the
fact that this is a SDM845+WCN3990 combination? The only thing that
software would ever do with the separate node is look it up to find the
regulators to power it on.

Brian

> >  
> >  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 */ >,

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

[Stephen Boyd]

Quoting Brian Norris (2018-11-02 11:43:17)
> Hi Stephen and Govind,
> 
> I was chatting with Govind, and he seemed to be a bit stalled on this.
> I'd encourage him to reply with whatever knowledge he has, because it's
> a bit hard to give definitive answers when I don't know all the inner
> workings here. (In fact, you, Stephen, probably know more than me about
> how Qualcomm MSM chips work.)
> 
> First of all, I'll explain the limited bits I do know, and see how they
> fit in below. I may be wrong.
> 
> WCN3990 is an external module, for supporting BT and Wifi RF components.
> It has a host interface for the Wifi, but it's not something the host
> knows how to talk to directly at all -- it's an "Analog IQ" interface,
> between an internal SoC MAC and PHY processor. Instead, we talk to this
> module via the System NOC (?). So while there are basically 2 distinct
> hardware components (on-SoC coprocessors, various internal communication
> buses, various shared memory regions, etc.; and the external WCN3990
> module), there is almost no way for the main processor to "talk" to the
> WCN3990 directly.
> 
> Another data point to throw into the mix: WCN3990 can apparently be used
> on multiple different SoCs -- I see public announcments about SDM835
> (and 660?), and I'm currently playing with it on SDM845. So perhaps
> there is some value in understanding "WCN3990" as being distinct from
> "WiFi MAC/PHY hardware and communication logic found on a MSM SoC." But
> they do seem to be very tightly coupled...
> 
> Side note: there is also a BT component on the WCN3990 module, and we
> *can* talk to that directly over UART. There's a separate binding going
> in for that, and it's a much clearer separation to divide "UART
> controller" and "BT/UART interface on WCN3990."

Thanks for the background. I wasn't aware of much about this driver but
taking this information and skimming the driver makes my mental model
believe that the register space here is a custom I/O region in the SoC
used to read/write to the BT/WiFi chip that's outside the SoC. Similar
to i2c, SPI, or even PCI, in the way that the I/O region in the SoC is
essentially the controller that is connected to the external chip. It's
like the hardware engineers stuck the PCI hardware blob inside the SoC
and then made special pins and wires for the thing the PCI device used
to do internally. Or is that completely wrong still?

> 
> On Wed, Oct 17, 2018 at 12:41:29AM -0700, Stephen Boyd wrote:
> > Quoting Govind Singh (2018-10-10 04:52:54)
> > > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> > > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> 
> > > @@ -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?
> 
> In my understanding:
> At least 3 of those (all the supplies Govind is adding) are external
> pins on the RF module. Why do you assume these are something the
> firmware can control? In the schematics I'm looking at, this seems to be
> connected to a PMIC. While it's certainly possible this is something the
> Q6 processor (running modem and Wifi firmware) can control, it doesn't
> seem obvious to me that they *must* be able to.
> 
> So I guess I'd say: why not represent these regulators in the device
> tree? It seems like a valid hardware description (like I said, 3 power
> rail pins on an external module).

Agreed. I want to specify the regulators in DT. I'm mostly asking if
there is firmware that runs and can control these regulators itself. If
so, then I'm lost why that firmware can't manage these itself and let us
ignore these requirements and never specify them in DT. Presumably there
isn't firmware that can manage it?

> 
> Now, your *next* paragraph might have hairier points :)
> 
> > And these names don't seem to match any sort of schematic or really
> 
> Which names? I see these pin names on the WCN3990 datasheets I see:
> 
>  VDD18_IO
>  VDD18_XO
>  VDD33_CH0
>  VDD33_CH1
>  VDD13_RFA
> 
> 3 of those match the 3 Govind is adding, and they appear to line up with
> what I see in schematics.

Yes those three match up, but the other one, vdd-0.8-cx-mx, looks like
the CX and MX power supplies that are typical of Qualcomm designs so
maybe those regulators are used for the I/O region inside the SoC
instead of the off-SoC chip?

> 
> > describe what this device is. From what I can tell, we've combined the
> 
> I've described "waht the device is" above to the best of my knowledge.
> If you're just looking at schematics, you might possibly be thrown by
> the fact that WCN3990 is packaged in a module provided by other vendors,
> so it might be labeled by that vendor on the schematic, not by the
> Qualcomm WCN3990 name.

Thanks!

> 
> > 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?
> 
> Why phandle? Under what bus would you place the WCN3990? As per my
> description above, there is really no way to talk to it directly. So if
> anything, it seems like it would be a subnode of the node we're
> describing here.

I'm not tied to having a phandle at all. I'm mostly trying to make the
following distinction in DT. If a node is under the soc node, it has a
reg property and represents a hardware block inside the SoC. Any
regulators or clocks that the node uses should also either be provided
inside the SoC, or be pins on the SoC that the device can be connected
to. Otherwise, if those regulators aren't going to pins for the SoC,
then it means we have a mash-up of two devices in one place in the DT
hierarchy. This is what doesn't look proper to me, and it's why we would
want to split the node into two nodes, the SoC part and the module part
for the off-SoC WCN chip.

And yes, from what you've told me here it would make sense to make the
WCN chip a subnode of this SoC node instead of a phandle connecting the
two.
 
> 
> In favor of your separation though: there are many ways to utilize
> WCN3990 apparently, and I'd imagine the binding might change a bit
> depending on the SoC (e.g., different clocks?). So there might be value
> in describing the SDM{835,845,...whatever}-wifi-soc-components vs.
> external WCN3990. Additionally, I don't know if it's even possible to
> utilize a different RF module with the same SoC (is there a possibility
> of a, e.g., WCN3991 that can use the same SoC logic?).

Sure. Does it matter though? Even one-off solutions would be better off
if we described what's going on at the board-level so that it isn't
confusing to readers of the schematic and the dts file. Plus, it would
allow the module creator to deliver a dts file for the module without
having to involve the SoC bits and clearly split things so that the SoC
dts file can be written without board assumptions.

> 
> But I'm still not totally convinced that splitting this up really makes
> much sense. Is there other precedent for splitting out a separate node
> for something that we don't talk to at all (no digital interface)? Or do
> we just need a more accurate compatible property, that describes the
> fact that this is a SDM845+WCN3990 combination? The only thing that
> software would ever do with the separate node is look it up to find the
> regulators to power it on.
> 

Agreed, it may seem like overkill, but I'll argue that this part allows
us to easily see where the division of clocks and regulators is, instead
of having to mentally untangle the external module from the internal SoC
bits. I started to compare the AHB and the SNOC bus types in the ath10k
driver but then I decided they were just a little bit different from
each other to where this split wouldn't help that code.

So like you say, if in the future the same SNOC hardware block will be
used with a different WCN chip that has different clock and power
requirements it would be very easy to integrate that by writing a new
sub-device node and driver and doing this split. I admit that there
would be some new work in the ath10k driver to support sub-device
drivers that the bus layer communicates with and it may conflict with
the way the PCI designs are implemented, but I would still argue this is
better from a DT implementer perspective because we can see what things
are on the board and what things are in the SoC.

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

[Brian Norris]

Hi Stephen,

On Mon, Nov 05, 2018 at 08:33:02AM -0800, Stephen Boyd wrote:
> Quoting Brian Norris (2018-11-02 11:43:17)
> > Hi Stephen and Govind,
> > 
> > I was chatting with Govind, and he seemed to be a bit stalled on this.
> > I'd encourage him to reply with whatever knowledge he has, because it's
> > a bit hard to give definitive answers when I don't know all the inner
> > workings here. (In fact, you, Stephen, probably know more than me about
> > how Qualcomm MSM chips work.)
> > 
> > First of all, I'll explain the limited bits I do know, and see how they
> > fit in below. I may be wrong.
> > 
> > WCN3990 is an external module, for supporting BT and Wifi RF components.
> > It has a host interface for the Wifi, but it's not something the host
> > knows how to talk to directly at all -- it's an "Analog IQ" interface,
> > between an internal SoC MAC and PHY processor. Instead, we talk to this
> > module via the System NOC (?). So while there are basically 2 distinct
> > hardware components (on-SoC coprocessors, various internal communication
> > buses, various shared memory regions, etc.; and the external WCN3990
> > module), there is almost no way for the main processor to "talk" to the
> > WCN3990 directly.
> > 
> > Another data point to throw into the mix: WCN3990 can apparently be used
> > on multiple different SoCs -- I see public announcments about SDM835
> > (and 660?), and I'm currently playing with it on SDM845. So perhaps
> > there is some value in understanding "WCN3990" as being distinct from
> > "WiFi MAC/PHY hardware and communication logic found on a MSM SoC." But
> > they do seem to be very tightly coupled...
> > 
> > Side note: there is also a BT component on the WCN3990 module, and we
> > *can* talk to that directly over UART. There's a separate binding going
> > in for that, and it's a much clearer separation to divide "UART
> > controller" and "BT/UART interface on WCN3990."
> 
> Thanks for the background. I wasn't aware of much about this driver but
> taking this information and skimming the driver makes my mental model
> believe that the register space here is a custom I/O region in the SoC
> used to read/write to the BT/WiFi chip that's outside the SoC. Similar
> to i2c, SPI, or even PCI, in the way that the I/O region in the SoC is
> essentially the controller that is connected to the external chip. It's
> like the hardware engineers stuck the PCI hardware blob inside the SoC
> and then made special pins and wires for the thing the PCI device used
> to do internally. Or is that completely wrong still?

I'd still say that's somewhat wrong :)

There is literally a separate processor [1] within the SoC that runs a
hodgepodge of firmware blobs. One of those is a Wifi firmware. We talk
to the Wifi firmware through that I/O region [2] (as well as various
memory-mappings we set up for DMA, etc.). These don't really translate
directly into operations "on the external chip." There is also a bunch
of WLAN logic (MAC / PHY hardware, including ADC/DAC conversion) that
lives on the SoC, and *that* logic drives the external chip for any RF
activity. The interface between the on-chip WLAN logic and the external
RF modules consists of a couple of analog IQ lines and some control
lines. But again, we never actually program anything from the
perspective of the host CPU that looks at all like "IQ" or "RF control"
commands.

To fit your PCI-like analogy: the SoC contains both the PCI controller
and half of the PCI device (all the brains), including all digital
interfaces the host CPU actually knows how to talk to.

[1] https://en.wikipedia.org/wiki/Qualcomm_Hexagon
[2] At least, that's how I understand it. This document is the only
thing I've seen (besides their driver code) that tells me what this I/O
region is ("membase" -- how nice!). It's just listed as an empty
RESERVED block on the only SoC documentation I have.

> > On Wed, Oct 17, 2018 at 12:41:29AM -0700, Stephen Boyd wrote:
> > > Quoting Govind Singh (2018-10-10 04:52:54)
> > > > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> > > > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
> > 
> > > > @@ -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?
> > 
> > In my understanding:
> > At least 3 of those (all the supplies Govind is adding) are external
> > pins on the RF module. Why do you assume these are something the
> > firmware can control? In the schematics I'm looking at, this seems to be
> > connected to a PMIC. While it's certainly possible this is something the
> > Q6 processor (running modem and Wifi firmware) can control, it doesn't
> > seem obvious to me that they *must* be able to.
> > 
> > So I guess I'd say: why not represent these regulators in the device
> > tree? It seems like a valid hardware description (like I said, 3 power
> > rail pins on an external module).
> 
> Agreed. I want to specify the regulators in DT. I'm mostly asking if
> there is firmware that runs and can control these regulators itself. If
> so, then I'm lost why that firmware can't manage these itself and let us
> ignore these requirements and never specify them in DT. Presumably there
> isn't firmware that can manage it?

That's my presumption. But I guess someone from Qualcomm would have to
give you a definitive answer here, as I'm not really an expert on
Qualcomm firmware.

> > Now, your *next* paragraph might have hairier points :)
> > 
> > > And these names don't seem to match any sort of schematic or really
> > 
> > Which names? I see these pin names on the WCN3990 datasheets I see:
> > 
> >  VDD18_IO
> >  VDD18_XO
> >  VDD33_CH0
> >  VDD33_CH1
> >  VDD13_RFA
> > 
> > 3 of those match the 3 Govind is adding, and they appear to line up with
> > what I see in schematics.
> 
> Yes those three match up, but the other one, vdd-0.8-cx-mx, looks like
> the CX and MX power supplies that are typical of Qualcomm designs so
> maybe those regulators are used for the I/O region inside the SoC
> instead of the off-SoC chip?

Good point. This document already wasn't so clear, because basically
everything is optional (I believe Govind is going to fix that), so it's
not clear what was actually intended for use with this WCN3990 binding.
But in practice, it looks like Qualcomm is trying to use this on
"WCN3990", and it does end up pointing at a pin directly on the SoC. I
would probably assume that's dealing with some SoC I/O region, yes. (But
I don't really know for sure.)

So if we're really going for splitting the "WCN3990" (external module)
from the "SDM845 WLAN logic", then we have 3 regulators for the former,
and 1 for the latter.

> > > describe what this device is. From what I can tell, we've combined the
> > 
> > I've described "waht the device is" above to the best of my knowledge.
> > If you're just looking at schematics, you might possibly be thrown by
> > the fact that WCN3990 is packaged in a module provided by other vendors,
> > so it might be labeled by that vendor on the schematic, not by the
> > Qualcomm WCN3990 name.
> 
> Thanks!
> 
> > 
> > > 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?
> > 
> > Why phandle? Under what bus would you place the WCN3990? As per my
> > description above, there is really no way to talk to it directly. So if
> > anything, it seems like it would be a subnode of the node we're
> > describing here.
> 
> I'm not tied to having a phandle at all. I'm mostly trying to make the
> following distinction in DT. If a node is under the soc node, it has a
> reg property and represents a hardware block inside the SoC. Any

The above (reg property) is true.

> regulators or clocks that the node uses should also either be provided
> inside the SoC, or be pins on the SoC that the device can be connected
> to. Otherwise, if those regulators aren't going to pins for the SoC,

The clocks are all for the SoC.

The only piece that doesn't fit is 3 of the 4 regulators -- they appear
to be for the external module, not for the SoC component.

> then it means we have a mash-up of two devices in one place in the DT
> hierarchy. This is what doesn't look proper to me, and it's why we would
> want to split the node into two nodes, the SoC part and the module part
> for the off-SoC WCN chip.
> 
> And yes, from what you've told me here it would make sense to make the
> WCN chip a subnode of this SoC node instead of a phandle connecting the
> two.

I could begrudgingly agree with that.

> > In favor of your separation though: there are many ways to utilize
> > WCN3990 apparently, and I'd imagine the binding might change a bit
> > depending on the SoC (e.g., different clocks?). So there might be value
> > in describing the SDM{835,845,...whatever}-wifi-soc-components vs.
> > external WCN3990. Additionally, I don't know if it's even possible to
> > utilize a different RF module with the same SoC (is there a possibility
> > of a, e.g., WCN3991 that can use the same SoC logic?).
> 
> Sure. Does it matter though? Even one-off solutions would be better off
> if we described what's going on at the board-level so that it isn't
> confusing to readers of the schematic and the dts file.

I suppose it has some limited value.

Side note: I don't even know what I'd call the top-level node. As of
yet, everything Qualcomm has submitted for Wifi here has used the name
WCN3990 (which is the name of the external RF chip) with no reference to
the SoC used (I've only been testing SDM845). I don't even know how much
needs to change (e.g., the Wifi firmware?) if this were to be used on
SDM835 or some other SoC. Maybe we just call it "qcom,sdm845-wifi", with
a "qcom,wcn3990-wifi" subnode (distinguished from "qcom,wcn3990-bt",
which shows up under a UART). Like:

	wifi: wifi@18800000 {
		compatible = "qcom,sdm845-wifi";
		reg = <...>
		clocks = <...>
		vdd-0.8-cx-mx-supply = <...>
		... interrupts, etc. ...

		rf { // I don't know what to call this node. Suggestions
		     // welcome.
			compatible = "qcom,wcn3990-wifi";
			vdd-1.8-xo-supply = <...>;
			vdd-1.3-rfa-supply = <...>;
			vdd-3.3-ch0-supply = <...>;
		};
	};

> Plus, it would
> allow the module creator to deliver a dts file for the module without
> having to involve the SoC bits and clearly split things so that the SoC
> dts file can be written without board assumptions.

Nice joke -- you actually think a module vendor ever looks at this
stuff? :D

In general, all regulator properties tend to be board-specific. So in
any case, the SoC dts tends to leave the regulators out, and the board
file has to know how to find the right node(s) and plug in regulators.
e.g., this isn't that hard:

&wifi {
	foo-supply = <&soc_power_rail>;
	bar-supply = <&external_module_power_rail>;
};

And it's really not much different than:

&wifi {
	foo-supply = <&soc_power_rail>;
};

&rf {
	bar-supply = <&external_module_power_rail>;
};

Anyway, I think that's mostly beside the point.

> > But I'm still not totally convinced that splitting this up really makes
> > much sense. Is there other precedent for splitting out a separate node
> > for something that we don't talk to at all (no digital interface)? Or do
> > we just need a more accurate compatible property, that describes the
> > fact that this is a SDM845+WCN3990 combination? The only thing that
> > software would ever do with the separate node is look it up to find the
> > regulators to power it on.
> > 
> 
> Agreed, it may seem like overkill, but I'll argue that this part allows
> us to easily see where the division of clocks and regulators is, instead
> of having to mentally untangle the external module from the internal SoC
> bits. I started to compare the AHB and the SNOC bus types in the ath10k
> driver but then I decided they were just a little bit different from
> each other to where this split wouldn't help that code.

Yeah...I tried to look into some schematics for some of the Google Wifi
devices that use that driver, for comparison. It actually appears to me
like the IPQ4019 SoC is even more highly-integrated -- the only external
components are some Front-End Modules (?), which do little more than
some amplification, and RX/TX switching. You can find some basic product
info for one of these under the Skyworks SKY85717 name. In practice, we
don't even really have a separate regulator for them (they just run off
one of the main system power rails), and there is zero software
interface to them.

So in that case, the IPQ4019 / AHB stuff is really an "all-in-one" SoC
component, in my understanding. *Maybe* we could split out some
description of the FEM if it really had a set of regulators we needed to
describe. But that seems like overkill.

> So like you say, if in the future the same SNOC hardware block will be
> used with a different WCN chip that has different clock and power
> requirements it would be very easy to integrate that by writing a new
> sub-device node and driver and doing this split. I admit that there
> would be some new work in the ath10k driver to support sub-device
> drivers that the bus layer communicates with and it may conflict with
> the way the PCI designs are implemented, but I would still argue this is
> better from a DT implementer perspective because we can see what things
> are on the board and what things are in the SoC.

If we were to split out a subnode, I doubt we'd actually make a
sub-device and driver -- we'd just have the top-level device look down
into its sub-node to pull out the tiny bits (a few regulators and maybe
a 'compatible' property) that it needs. But then, DT is not supposed to
describe drivers (haha), so that shouldn't be relevant here.

Brian

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

[Brian Norris]

On Tue, Nov 06, 2018 at 04:07:01PM -0800, Brian Norris wrote:
> On Mon, Nov 05, 2018 at 08:33:02AM -0800, Stephen Boyd wrote:
> > And yes, from what you've told me here it would make sense to make the
> > WCN chip a subnode of this SoC node instead of a phandle connecting the
> > two.
> 
> I could begrudgingly agree with that.

...

> 
> 	wifi: wifi@18800000 {
> 		compatible = "qcom,sdm845-wifi";
> 		reg = <...>
> 		clocks = <...>
> 		vdd-0.8-cx-mx-supply = <...>
> 		... interrupts, etc. ...
> 
> 		rf { // I don't know what to call this node. Suggestions
> 		     // welcome.
> 			compatible = "qcom,wcn3990-wifi";
> 			vdd-1.8-xo-supply = <...>;
> 			vdd-1.3-rfa-supply = <...>;
> 			vdd-3.3-ch0-supply = <...>;
> 		};
> 	};

By the way...I realize one reason why I've been "begrudging" on this:
the single-node binding was already reviewed and merged upstream as of
v4.18:

ae316c4cbba2 dt: bindings: add bindings for wcn3990 wifi block

It seems like a lot of needless churn to rewrite the entire binding,
only to
 * make the usage of these regulators a little clearer and
 * possibly help distinguish different variants of WCN3990 usage (e.g.,
   on different SoCs) -- I don't even know how different "WCN3990" looks
   when used on something non-SDM845.

Even if the second bullet point is important, we could fix this by a
more judicious use of 'compatible', rather than inventing whole new
nodes.

Regards,
Brian