drm/msm: cleanup gpu bindings

The plan is to use the OPP bindings. For now, remove the documentation for qcom,gpu-pwrlevels, and make the driver fall back to a safe low clock if the node is not present.

Note that no upstream dtb use this node. For now we keep compatibility with this node to avoid breaking compatibility with downstream android dt files.

Signed-off-by Rob Clark <[email protected]>

Quoted 4 lines · Rob Clark

The plan is to use the OPP bindings. For now, remove the documentation for qcom,gpu-pwrlevels, and make the driver fall back to a safe low clock if the node is not present.

Quoted 5 lines

Note that no upstream dtb use this node. For now we keep compatibility with this node to avoid breaking compatibility with downstream android dt files.

Signed-off-by Rob Clark <[email protected]>

Will we need the bus frequency knobs that I see in the old pwrlevels node? If so, what would the plan be for doing that within OPP?

Quoted 6 lines · Eric Anholt

Quoted 4 lines · Rob Clark

The plan is to use the OPP bindings. For now, remove the documentation for qcom,gpu-pwrlevels, and make the driver fall back to a safe low clock if the node is not present.

Quoted 8 lines

Quoted 5 lines

Note that no upstream dtb use this node. For now we keep compatibility with this node to avoid breaking compatibility with downstream android dt files.

Signed-off-by Rob Clark <[email protected]>

Will we need the bus frequency knobs that I see in the old pwrlevels node? If so, what would the plan be for doing that within OPP?

So, that I think is one of the open questions. Jordan knows this stuff a lot better than I, but my understanding is that bus and clk scale *basically* independently, except that a given gpu clk we want a different minimum bus clk.

(I'm not sure if that is a functional requirement, or just what qcom arrived at after performance tuning..)

There is some work ongoing to get some sort of upstream bus scaling scaling, although I'm not really sure yet what the bindings for this would look like.

So basically short answer is "I don't know.. there are too many open questions". Maybe in the end we re-introduce qcom,gpu-pwrlevels. But I think for now the approach of not documenting it and have safe/slow clk fallback in the driver is a reasonable way to move forward with getting some basic gpu nodes into upstream dts files.

Quoted 6 lines · Eric Anholt

Quoted 4 lines · Rob Clark

The plan is to use the OPP bindings. For now, remove the documentation for qcom,gpu-pwrlevels, and make the driver fall back to a safe low clock if the node is not present.

Quoted 8 lines

Quoted 5 lines

Note that no upstream dtb use this node. For now we keep compatibility with this node to avoid breaking compatibility with downstream android dt files.

Signed-off-by Rob Clark <[email protected]>

Will we need the bus frequency knobs that I see in the old pwrlevels node? If so, what would the plan be for doing that within OPP?

So, that I think is one of the open questions. Jordan knows this stuff a lot better than I, but my understanding is that bus and clk scale *basically* independently, except that a given gpu clk we want a different minimum bus clk.

(I'm not sure if that is a functional requirement, or just what qcom arrived at after performance tuning..)

There is some work ongoing to get some sort of upstream bus scaling scaling, although I'm not really sure yet what the bindings for this would look like.

So basically short answer is "I don't know.. there are too many open questions". Maybe in the end we re-introduce qcom,gpu-pwrlevels. But I think for now the approach of not documenting it and have safe/slow clk fallback in the driver is a reasonable way to move forward with getting some basic gpu nodes into upstream dts files.

The plan is to use the OPP bindings. For now, remove the documentation for qcom,gpu-pwrlevels, and make the driver fall back to a safe low clock if the node is not present.

Note that no upstream dtb use this node. For now we keep compatibility with this node to avoid breaking compatibility with downstream android dt files.

Signed-off-by Rob Clark <[email protected]>

---
 Documentation/devicetree/bindings/display/msm/gpu.txt | 15 ---------------
 drivers/gpu/drm/msm/adreno/adreno_device.c            |  6 ++++--
 2 files changed, 4 insertions(+), 17 deletions(-)

The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.

Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.

Quoted 8 lines · Rob Clark

The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.

Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.

The gpulist[] seems pretty short, like you could just have 7 compatible strings in dt_match[] and point them directly at corresponding the gpulist[] entry. Or are there lots of patch levels, with the same struct adreno_info values?

Quoted 11 lines · Eric Anholt

Quoted 8 lines · Rob Clark

The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.

Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.

Quoted 4 lines

The gpulist[] seems pretty short, like you could just have 7 compatible strings in dt_match[] and point them directly at corresponding the gpulist[] entry. Or are there lots of patch levels, with the same struct adreno_info values?

The list currently is rather incomplete (which may or may not matter, I guess it comes down to how many different phones/tablets out there people try to get an upstream kernel working on). And it has those ANY_ID wildcard entries.

A full list of all the gpu's including all their patchlevels would be quite long.

We might end up wanting to split the quirks out differently, since those tend to apply to specific patchlevel's.. I had to change the existing entry for a530 from a530.* to a530.2 for this reason. But that won't effect the bindings so that is an implementation detail we can worry about later.

Quoted 22 lines · Rob Clark

Quoted 11 lines · Eric Anholt

Quoted 8 lines · Rob Clark

The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.

Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.

Quoted 4 lines

The gpulist[] seems pretty short, like you could just have 7 compatible strings in dt_match[] and point them directly at corresponding the gpulist[] entry. Or are there lots of patch levels, with the same struct adreno_info values?

The list currently is rather incomplete (which may or may not matter, I guess it comes down to how many different phones/tablets out there people try to get an upstream kernel working on). And it has those ANY_ID wildcard entries.

Quoted 8 lines

A full list of all the gpu's including all their patchlevels would be quite long.

We might end up wanting to split the quirks out differently, since those tend to apply to specific patchlevel's.. I had to change the existing entry for a530 from a530.* to a530.2 for this reason. But that won't effect the bindings so that is an implementation detail we can worry about later.

I think that using the of_match_device() mechanism from the specific strings listed as the compatible would make more sense than this string parsing. You have to write a gpulist[] entry anyway for the module to load. So that means adding about 7 values today to the compatible string dt_match, and the code to use of_match_device() to get your struct adreno_info. Then the current version number lookup loop would be just for the old DT compatibility and there's no string parsing.

However, it's your driver. The code seems correct, and using specific compatible strings is an obviously good step.

Reviewed-by Eric Anholt <[email protected]>

The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.

Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.

Signed-off-by Rob Clark <[email protected]>

---
 .../devicetree/bindings/display/msm/gpu.txt        | 11 +++---
 drivers/gpu/drm/msm/adreno/adreno_device.c         | 43 +++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_drv.c                      |  1 +
 3 files changed, 49 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/display/msm/gpu.txt +6 -5

index 747b984..7ac3052 100644
@@ -1,7 +1,11 @@
 Qualcomm adreno/snapdragon GPU
 
 Required properties:
-- compatible: "qcom,adreno-3xx"
+- compatible: "qcom,adreno-XYZ.W", "qcom,adreno"
+    for example: "qcom,adreno-306.0", "qcom,adreno"
+  Note that you need to list the less specific "qcom,adreno" (since this
+  is what the device is matched on), in addition to the more specific
+  with the chip-id.
 - reg: Physical base address and length of the controller's registers.
 - interrupts: The interrupt signal from the gpu.
 - clocks: device clocks

@@ -10,8 +14,6 @@ Required properties:
   * "core_clk"
   * "iface_clk"
   * "mem_iface_clk"
-- qcom,chipid: gpu chip-id.  Note this may become optional for future
-  devices if we can reliably read the chipid from hw
 
 Example:
 

@@ -19,7 +21,7 @@ Example:
...
 
 	gpu: qcom,kgsl-3d0@4300000 {
-		compatible = "qcom,adreno-3xx";
+		compatible = "qcom,adreno-320.2", "qcom,adreno";
 		reg = <0x04300000 0x20000>;
 		reg-names = "kgsl_3d0_reg_memory";
 		interrupts = <GIC_SPI 80 0>;

@@ -32,6 +34,5 @@ Example:
 		    <&mmcc GFX3D_CLK>,
 		    <&mmcc GFX3D_AHB_CLK>,
 		    <&mmcc MMSS_IMEM_AHB_CLK>;
-		qcom,chipid = <0x03020100>;
 	};
 };

drivers/gpu/drm/msm/adreno/adreno_device.c +1 -0

index 7b9181b2..fdaef67 100644
@@ -189,6 +189,46 @@ static const struct {
 	{ "qcom,gpu-quirk-fault-detect-mask", ADRENO_QUIRK_FAULT_DETECT_MASK },
 };
 
+static int find_chipid(struct device *dev, u32 *chipid)

drivers/gpu/drm/msm/adreno/adreno_device.c +31 -1

+		unsigned rev, patch;
+
+		if (sscanf(compat, "qcom,adreno-%u.%u", &rev, &patch) != 2)
+			continue;
+
+		*chipid = 0;
+		*chipid |= (rev / 100) << 24;  /* core */
+		rev %= 100;
+		*chipid |= (rev / 10) << 16;   /* major */
+		rev %= 10;
+		*chipid |= rev << 8;           /* minor */
+		*chipid |= patch;
+
+		return 0;
+	}
+
+	/* and if that fails, fall back to legacy "qcom,chipid" property: */
+	ret = of_property_read_u32(node, "qcom,chipid", chipid);
+	if (ret)
+		return ret;
+
+	dev_warn(dev, "Using legacy qcom,chipid binding!\n");
+	dev_warn(dev, "Use compatible qcom,adreno-%u%u%u.%u instead.\n",
+			(*chipid >> 24) & 0xff, (*chipid >> 16) & 0xff,
+			(*chipid >> 8) & 0xff, *chipid & 0xff);
+
+	return 0;
+}
+
 static int adreno_bind(struct device *dev, struct device *master, void *data)
 {
 	static struct adreno_platform_config config = {};

@@ -196,7 +236,7 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 	u32 val;
 	int ret, i;
 
-	ret = of_property_read_u32(node, "qcom,chipid", &val);
+	ret = find_chipid(dev, &val);
 	if (ret) {
 		dev_err(dev, "could not find chipid: %d\n", ret);
 		return ret;

@@ -265,6 +305,7 @@ static int adreno_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id dt_match[] = {
+	{ .compatible = "qcom,adreno" },
 	{ .compatible = "qcom,adreno-3xx" },
 	/* for backwards compat w/ downstream kgsl DT files: */
 	{ .compatible = "qcom,kgsl-3d0" },

drivers/gpu/drm/msm/msm_drv.c +1 -0

index e29bb66..6b85c41 100644
@@ -985,6 +985,7 @@ static int add_display_components(struct device *dev,
  * as components.
  */
 static const struct of_device_id msm_gpu_match[] = {
+	{ .compatible = "qcom,adreno" },
 	{ .compatible = "qcom,adreno-3xx" },
 	{ .compatible = "qcom,kgsl-3d0" },
 	{ },

Signature · 1 line

2.9.3

Quoted 4 lines · Rob Clark

The original way we determined the gpu version was based on downstream bindings from android kernel. A cleaner way is to get the version from the compatible string.

Quoted 4 lines

Note that no upstream dtb uses these bindings. But the code still supports falling back to the legacy bindings (with a warning), so that we are still compatible with the gpu dt node from android device kernels.

Fine for one driver/binding, but we wouldn't really want to do carry downstream compatibility for everything.

Quoted 59 lines

Signed-off-by Rob Clark <[email protected]>

---
 .../devicetree/bindings/display/msm/gpu.txt        | 11 +++---
 drivers/gpu/drm/msm/adreno/adreno_device.c         | 43 +++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_drv.c                      |  1 +
 3 files changed, 49 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/display/msm/gpu.txt +6 -5

index 747b984..7ac3052 100644
@@ -1,7 +1,11 @@
 Qualcomm adreno/snapdragon GPU
 
 Required properties:
-- compatible: "qcom,adreno-3xx"
+- compatible: "qcom,adreno-XYZ.W", "qcom,adreno"
+    for example: "qcom,adreno-306.0", "qcom,adreno"
+  Note that you need to list the less specific "qcom,adreno" (since this
+  is what the device is matched on), in addition to the more specific
+  with the chip-id.
 - reg: Physical base address and length of the controller's registers.
 - interrupts: The interrupt signal from the gpu.
 - clocks: device clocks

@@ -10,8 +14,6 @@ Required properties:
   * "core_clk"
   * "iface_clk"
   * "mem_iface_clk"
-- qcom,chipid: gpu chip-id.  Note this may become optional for future
-  devices if we can reliably read the chipid from hw
 
 Example:
 

@@ -19,7 +21,7 @@ Example:
...
 
      gpu: qcom,kgsl-3d0@4300000 {
-             compatible = "qcom,adreno-3xx";
+             compatible = "qcom,adreno-320.2", "qcom,adreno";
              reg = <0x04300000 0x20000>;
              reg-names = "kgsl_3d0_reg_memory";
              interrupts = <GIC_SPI 80 0>;

@@ -32,6 +34,5 @@ Example:
                  <&mmcc GFX3D_CLK>,
                  <&mmcc GFX3D_AHB_CLK>,
                  <&mmcc MMSS_IMEM_AHB_CLK>;
-             qcom,chipid = <0x03020100>;
      };
 };

drivers/gpu/drm/msm/adreno/adreno_device.c +1 -0

index 7b9181b2..fdaef67 100644
@@ -189,6 +189,46 @@ static const struct {
      { "qcom,gpu-quirk-fault-detect-mask", ADRENO_QUIRK_FAULT_DETECT_MASK },
 };
 
+static int find_chipid(struct device *dev, u32 *chipid)

drivers/gpu/drm/msm/adreno/adreno_device.c static const struct { · line 202

Quoted 10 lines

drivers/gpu/drm/msm/adreno/adreno_device.c +10 -0

+{
+     struct device_node *node = dev->of_node;
+     struct property *prop;
+     const char *compat;
+     int ret;
+
+     /* first search the compat strings for qcom,adreno-XYZ.W: */
+     prop = of_find_property(node, "compatible", NULL);
+     for (compat = of_prop_next_string(prop, NULL); compat;
+          compat = of_prop_next_string(prop, compat)) {

of_property_for_each_string

However, you specify in the binding it should be the 1st string, so you really don't need a loop here and could use of_property_read_string_index.

With that,

Acked-by Rob Herring <[email protected]>

This was never documented or used in upstream dtb. It is used by downstream bindings from android device kernels. But the quirks are a property of the gpu revision, and as such are redundant to be listed separately in dt. Instead, move the quirks to the device table.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  4 ++--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 18 ++++--------------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  1 -
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  4 +---
 4 files changed, 7 insertions(+), 20 deletions(-)

Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.

Cc Rob Herring <[email protected]>

Signed-off-by Rob Clark <[email protected]>

drivers/gpu/drm/msm/msm_drv.c +5 -0

+struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
+{
+	struct clk *clk;
+	char name2[32];
+

Quoted 5 lines · Rob Clark

Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.

Cc Rob Herring <[email protected]>

Signed-off-by Rob Clark <[email protected]>

Huh, I don't think I would have cleaned up DT bindings in exchange for adding driver code like this. But the code seems correct, so other than one optional suggestion:

Reviewed-by Eric Anholt <[email protected]>

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +10 -0

+struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
+{
+     struct clk *clk;
+     char name2[32];
+
+     clk = devm_clk_get(&pdev->dev, name);
+     if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
+             return clk;
+
+     snprintf(name2, sizeof(name2), "%s_clk", name);

drivers/gpu/drm/msm/msm_drv.c module_param(dumpstate, bool, 0600); · line 108

Quoted 5 lines

drivers/gpu/drm/msm/msm_drv.c +5 -0

+
+     clk = devm_clk_get(&pdev->dev, name2);
+     if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
+             dev_warn(&pdev->dev, "Using legacy clk name binding.  Use "
+                             "\"%s\" instead of \"%s\"\n", name, name2);

Drop the second "|| PTR_ERR(clk)" case, so that you only get one warning printed at boot if deferring happens?

Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.

Cc Rob Herring <[email protected]>

Signed-off-by Rob Clark <[email protected]>

---
 Documentation/devicetree/bindings/display/msm/gpu.txt | 12 ++++++------
 drivers/gpu/drm/msm/msm_drv.c                         | 19 +++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h                         |  1 +
 drivers/gpu/drm/msm/msm_gpu.c                         |  7 +++----
 4 files changed, 29 insertions(+), 10 deletions(-)