arm64: dts: qcom: sc7280: Introduce herobrine-rev1

When processing sc7280 device trees, I can see:

Warning (simple_bus_reg): /soc@0/gmu@3d69000:
simple-bus unit address format error, expected "3d6a000"

There's a clear typo in the node name. Fix it.

Fixes 96c471970b7b ("arm64: dts: qcom: sc7280: Add gpu support")

Signed-off-by Douglas Anderson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---

(no changes since v1)

  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/boot/dts/qcom/sc7280.dtsi +0 -0

index 937c2e0e93eb..eab7a8505053 100644

This factors out a device tree fragment from some sc7280 device trees. It represents the device tree bits that should be included for "Chrome" based sc7280 boards. On these boards the bootloader (Coreboot
+ Depthcharge) configures things slightly different than the bootloader that Qualcomm provides. The modem firmware on these boards also works differently than on other Qulacomm products and thus the reserved memory map needs to be adjusted.

[...]

Though sc7280 itself doesn't need any of the defines in gpio.h, it's highly likely that the actual boards will use them. Let's add the include to the sc7280.dtsi file so that boards don't need to do it.

Suggested-by Konrad Dybcio <[email protected]>

Signed-off-by Douglas Anderson <[email protected]>

---

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +7 -0

+};
+
+/* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */
+
+&ap_spi_fp {
+       status = "okay";
+};

arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi new file +4 -0

new file mode 100644
index 000000000000..24c34ddebd18
@@ -0,0 +1,778 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine baseboard device tree source
+ *

arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi new file +10 -0

+
+               vin-supply = <&ppvar_sys>;
+       };
+
+       pp3300_codec: pp3300-codec-regulator {
+               compatible = "regulator-fixed";
+               regulator-name = "pp3300_codec";
+
+               regulator-min-microvolt = <3300000>;
+               regulator-max-microvolt = <3300000>;

arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi new file +7 -0

+               compatible = "regulator-fixed";
+               regulator-name = "pp3300_left_in_mlb";
+
+               regulator-min-microvolt = <3300000>;
+               regulator-max-microvolt = <3300000>;
+
+               gpio = <&tlmm 80 GPIO_ACTIVE_HIGH>;

Quoting Douglas Anderson (2022-01-25 14:44:22)

Quoted 16 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +10 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+/dts-v1/;
+
+#include "sc7280-herobrine.dtsi"

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 14

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

+
+/ {
+       model = "Google Herobrine (rev1+)";
+       compatible = "google,herobrine", "qcom,sc7280";

Can we stop adding "qcom,sc7280" to the board compatible string? It looks out of place. It's the compatible for the SoC and should really be the compatible for the /soc node.

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 15

Quoted 1 line

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +1 -0

+       pp3300_left_in_mlb: pp3300-left-in-mlb {

Sometimes '-regulator' is left out. Is that intentional? I suppose it would be better if every node had regulator postfix, but it may be too long of a node name?

Hi,

Quoted 18 lines · Stephen Boyd

Quoting Douglas Anderson (2022-01-25 14:44:22)

Quoted 16 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +10 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+/dts-v1/;
+
+#include "sc7280-herobrine.dtsi"

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 14

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

+
+/ {
+       model = "Google Herobrine (rev1+)";
+       compatible = "google,herobrine", "qcom,sc7280";

Can we stop adding "qcom,sc7280" to the board compatible string? It looks out of place. It's the compatible for the SoC and should really be the compatible for the /soc node.

I don't have any objections, but I feel like this is the type of thing I'd like Bjorn to have the final say on. What say you, Bjorn?

Quoted 6 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 15

Quoted 2 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +1 -0

+       pp3300_left_in_mlb: pp3300-left-in-mlb {

Sometimes '-regulator' is left out. Is that intentional? I suppose it would be better if every node had regulator postfix, but it may be too long of a node name?

Huh, you're right. No, it's not intentional. It looks like it was that way on herobrine-rev0 too... It also looks like it's missing on "pp3300-hub" on trogdor.

Happy to add "-regulator" in the herobrine / trogdor nodes that need it. I'll probably do it as a follow-on patch if that works OK? Then I can just clean them all up at once...

-Doug

Quoted 31 lines

Hi,

Quoted 18 lines · Stephen Boyd

Quoting Douglas Anderson (2022-01-25 14:44:22)

Quoted 16 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +10 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+/dts-v1/;
+
+#include "sc7280-herobrine.dtsi"

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 14

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

+
+/ {
+       model = "Google Herobrine (rev1+)";
+       compatible = "google,herobrine", "qcom,sc7280";

Can we stop adding "qcom,sc7280" to the board compatible string? It looks out of place. It's the compatible for the SoC and should really be the compatible for the /soc node.

Quoted 3 lines

I don't have any objections, but I feel like this is the type of thing I'd like Bjorn to have the final say on. What say you, Bjorn?

One practical case I can think of right away, where this matters is in cpufreq-dt-plat.c where we blocklist qcom,sc7280.

I don't know if we rely on this in any other places, but I'm not keen on seeing a bunch of board-specific compatibles sprinkled throughout the implementation - it's annoying enough having to add each platform to these drivers.

Quoting Bjorn Andersson (2022-01-25 19:01:31)

Quoted 39 lines · Doug Anderson

Quoted 31 lines

Hi,

Quoted 18 lines · Stephen Boyd

Quoting Douglas Anderson (2022-01-25 14:44:22)

Quoted 16 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +10 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+/dts-v1/;
+
+#include "sc7280-herobrine.dtsi"

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 14

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

+
+/ {
+       model = "Google Herobrine (rev1+)";
+       compatible = "google,herobrine", "qcom,sc7280";

Can we stop adding "qcom,sc7280" to the board compatible string? It looks out of place. It's the compatible for the SoC and should really be the compatible for the /soc node.

Quoted 3 lines

I don't have any objections, but I feel like this is the type of thing I'd like Bjorn to have the final say on. What say you, Bjorn?

One practical case I can think of right away, where this matters is in cpufreq-dt-plat.c where we blocklist qcom,sc7280.

Quoted 4 lines

I don't know if we rely on this in any other places, but I'm not keen on seeing a bunch of board-specific compatibles sprinkled throughout the implementation - it's annoying enough having to add each platform to these drivers.

Looking at sc7180, grep only shows cpufreq-dt-plat.c

 $ git grep qcom,sc7180\" -- drivers
 drivers/cpufreq/cpufreq-dt-platdev.c:   { .compatible = "qcom,sc7180", },

Simplest solution would be to look at / and /soc for a compatible string.

 $ git grep -W 'soc[^:]*{' -- arch/arm*/boot/dts/ | grep compatible |
grep -v "simple-bus"

doesn't show many hits. The first hit is "ti,omap-infra" which is actually inside an soc node, but even then I don't see anything that matches the cpufreq-dt-plat.c lists.

----8<-----

drivers/cpufreq/cpufreq-dt-platdev.c +7 -3

index ca1d103ec449..32bfe453f8b4 100644
@@ -179,25 +179,29 @@ static bool __init cpu0_node_has_opp_v2_prop(void)
 static int __init cpufreq_dt_platdev_init(void)
 {
 	struct device_node *np = of_find_node_by_path("/");
+	struct device_node *soc_np = of_find_node_by_path("/soc");
 	const struct of_device_id *match;
 	const void *data = NULL;
 
-	if (!np)
+	if (!np && !soc_np)
 		return -ENODEV;
 
 	match = of_match_node(allowlist, np);
-	if (match) {
+	if (match || (match = of_match_node(allowlist, soc_np))) {
 		data = match->data;
 		goto create_pdev;
 	}
 
-	if (cpu0_node_has_opp_v2_prop() && !of_match_node(blocklist, np))
+	if (cpu0_node_has_opp_v2_prop() && !of_match_node(blocklist, np) &&
+	    !of_match_node(blocklist, soc_np))
 		goto create_pdev;
 
+	of_node_put(soc_np);
 	of_node_put(np);
 	return -ENODEV;
 
 create_pdev:
+	of_node_put(soc_np);
 	of_node_put(np);
 	return PTR_ERR_OR_ZERO(platform_device_register_data(NULL, "cpufreq-dt",
 			       -1, data,

Quoted 40 lines · Stephen Boyd

Quoting Bjorn Andersson (2022-01-25 19:01:31)

Quoted 39 lines · Doug Anderson

Quoted 31 lines

Hi,

Quoted 18 lines · Stephen Boyd

Quoting Douglas Anderson (2022-01-25 14:44:22)

Quoted 16 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +10 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+/dts-v1/;
+
+#include "sc7280-herobrine.dtsi"

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 14

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

+
+/ {
+       model = "Google Herobrine (rev1+)";
+       compatible = "google,herobrine", "qcom,sc7280";

Can we stop adding "qcom,sc7280" to the board compatible string? It looks out of place. It's the compatible for the SoC and should really be the compatible for the /soc node.

Quoted 3 lines

I don't have any objections, but I feel like this is the type of thing I'd like Bjorn to have the final say on. What say you, Bjorn?

One practical case I can think of right away, where this matters is in cpufreq-dt-plat.c where we blocklist qcom,sc7280.

Quoted 7 lines

Quoted 4 lines

I don't know if we rely on this in any other places, but I'm not keen on seeing a bunch of board-specific compatibles sprinkled throughout the implementation - it's annoying enough having to add each platform to these drivers.

Looking at sc7180, grep only shows cpufreq-dt-plat.c

Good, then we handle all other platform specifics in drivers using platform-specific compatibles.

Quoted 6 lines

 $ git grep qcom,sc7180\" -- drivers
 drivers/cpufreq/cpufreq-dt-platdev.c:   { .compatible = "qcom,sc7180", },

Simplest solution would be to look at / and /soc for a compatible string.

You mean that / would contain the device's compatible and /soc the soc's compatible? I'm afraid I don't see how this would help you - you still need the compatible in the dts, just now in two places.

Either we leave it as is - which follows my interpretation of what the DT spec says - or we (and the DT maitainers) agree that it shouldn't be there (because this dtb won't run on any random qcom,sc7180 anyways) at all.

Quoted 5 lines

Either we leave it as is - which follows my interpretation of what the DT spec says - or we (and the DT maitainers) agree that it shouldn't be there (because this dtb won't run on any random qcom,sc7180 anyways) at all.

I'm curious what part of the DT spec says that we should have the SoC in there? I know I've always done it, but it's always just been following the examples of what was done before. When talking about the root node, I see this in the devicetree-specification-v0.4-rc1 spec:

---

Specifies a list of platform architectures with which this platform is compatible. This property can be used by operating systems in selecting platform specific code. The recommended form of the property value is: "manufacturer,model"

For example:
compatible = "fsl,mpc8572ds"

---

That doesn't say anything about putting the SoC there.

I would also note that I'd be at least moderately inclined to land things as-is and deal with this in a follow-up patch, though I'm happy to spin if that's what people agree upon too. This is not a new problem and so it doesn't seem like it makes sense to glom dealing with it into this patch series...

Hi,

Quoted 23 lines · Doug Anderson

Quoted 5 lines

Either we leave it as is - which follows my interpretation of what the DT spec says - or we (and the DT maitainers) agree that it shouldn't be there (because this dtb won't run on any random qcom,sc7180 anyways) at all.

I'm curious what part of the DT spec says that we should have the SoC in there? I know I've always done it, but it's always just been following the examples of what was done before. When talking about the root node, I see this in the devicetree-specification-v0.4-rc1 spec:

---

Specifies a list of platform architectures with which this platform is compatible. This property can be used by operating systems in selecting platform specific code. The recommended form of the property value is: "manufacturer,model"

For example:
compatible = "fsl,mpc8572ds"

---

Quoted 8 lines

That doesn't say anything about putting the SoC there.

I would also note that I'd be at least moderately inclined to land things as-is and deal with this in a follow-up patch, though I'm happy to spin if that's what people agree upon too. This is not a new problem and so it doesn't seem like it makes sense to glom dealing with it into this patch series...

I noticed that you applied the first 4 patches in the series (thanks!) but not this one. Are we waiting to get agreement on this before landing? As per above, I think it'd be OK to land as-is and then I'm happy to do a follow-up patch to clean this up since this isn't a new issue. Having this patch outstanding makes it a little confusing with the other cleanup patches that I'm posting... ;-)

Quoted 41 lines

Quoting Bjorn Andersson (2022-01-25 19:01:31)

Quoted 39 lines · Doug Anderson

Quoted 31 lines

Hi,

Quoted 18 lines · Stephen Boyd

Quoting Douglas Anderson (2022-01-25 14:44:22)

Quoted 16 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +10 -0

new file mode 100644
index 000000000000..f95273052da0
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Herobrine board device tree source
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+/dts-v1/;
+
+#include "sc7280-herobrine.dtsi"

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts line 14

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts new file +4 -0

+
+/ {
+       model = "Google Herobrine (rev1+)";
+       compatible = "google,herobrine", "qcom,sc7280";

Can we stop adding "qcom,sc7280" to the board compatible string? It looks out of place. It's the compatible for the SoC and should really be the compatible for the /soc node.

Quoted 3 lines

I don't have any objections, but I feel like this is the type of thing I'd like Bjorn to have the final say on. What say you, Bjorn?

One practical case I can think of right away, where this matters is in cpufreq-dt-plat.c where we blocklist qcom,sc7280.

Quoted 7 lines

Quoted 4 lines

I don't know if we rely on this in any other places, but I'm not keen on seeing a bunch of board-specific compatibles sprinkled throughout the implementation - it's annoying enough having to add each platform to these drivers.

Looking at sc7180, grep only shows cpufreq-dt-plat.c

Good, then we handle all other platform specifics in drivers using platform-specific compatibles.

Quoted 3 lines

 $ git grep qcom,sc7180\" -- drivers
 drivers/cpufreq/cpufreq-dt-platdev.c:   { .compatible = "qcom,sc7180", },

Quoted 3 lines

Simplest solution would be to look at / and /soc for a compatible string.

You mean that / would contain the device's compatible and /soc the soc's compatible? I'm afraid I don't see how this would help you - you still need the compatible in the dts, just now in two places.

Either we leave it as is - which follows my interpretation of what the DT spec says - or we (and the DT maitainers) agree that it shouldn't be there (because this dtb won't run on any random qcom,sc7180 anyways) at all.

Hi,

Quoted 11 lines · Stephen Boyd

----8<-----

drivers/cpufreq/cpufreq-dt-platdev.c +0 -0

index ca1d103ec449..32bfe453f8b4 100644
@@ -179,25 +179,29 @@ static bool __init cpu0_node_has_opp_v2_prop(void)
 static int __init cpufreq_dt_platdev_init(void)
 {
        struct device_node *np = of_find_node_by_path("/");

drivers/cpufreq/cpufreq-dt-platdev.c static bool __init cpu0_node_has_opp_v2_prop(void) · line 182

Quoted 1 line

drivers/cpufreq/cpufreq-dt-platdev.c +1 -0

+       struct device_node *soc_np = of_find_node_by_path("/soc");

Seems that some device trees have "/soc" and others "/soc@0". For at least a little context, see commit a1875bf98290 ("arm64: dts: qcom:
sdm845: Add unit name to soc node"). Since (presumably) this would only be for newer SoCs then I guess you should search for "/soc@0"? ...and then if we ever have a SoC that's not @0 then we would have to iterate through all SoCs

This series adds support for herobrine-rev1. Note that it's likely that with the introduction of -rev1 we can drop -rev0 support, but we'll keep it for now (though we won't try to "fit it in" and share code with it).

This series is confirmed to boot herobrine-rev1 atop mainline, commit 0280e3c58f92 ("Merge tag 'nfs-for-5.17-1' of
git://git.linux-nfs.org/projects/anna/linux-nfs"), though it requires a hack to work around a misconfigured DMA for i2c14
(https://crrev.com/c/3378660)

[...]