Existing bindings for the gpu are based on the downstream android kgsl driver (a subset of the downstream bindings). But that isn't really how we want the upstream bindings to look.
For now we have held off on adding gpu nodes to upstream dt files, but now that all the dependencies are in place for some devices, it is time to clean things up so we can start adding this missing gpu nodes.
Note that these patches preserve compatibility with downstream dt files because, at this point, it is easy and convenient to not break people's patchsets for upstream support of various devices.
Rob Clark (4): drm/msm: remove qcom,gpu-pwrlevels bindings drm/msm: drop qcom,chipid drm/msm: drop quirks binding drm/msm: drop _clk suffix from clk names
.../devicetree/bindings/display/msm/gpu.txt | 38 ++++--------- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +- drivers/gpu/drm/msm/adreno/adreno_device.c | 65 ++++++++++++++++------ drivers/gpu/drm/msm/adreno/adreno_gpu.c | 1 - drivers/gpu/drm/msm/adreno/adreno_gpu.h | 4 +- drivers/gpu/drm/msm/msm_drv.c | 20 +++++++ drivers/gpu/drm/msm/msm_drv.h | 1 + drivers/gpu/drm/msm/msm_gpu.c | 7 +-- 8 files changed, 88 insertions(+), 52 deletions(-)
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 robdclark@gmail.com --- Documentation/devicetree/bindings/display/msm/gpu.txt | 15 --------------- drivers/gpu/drm/msm/adreno/adreno_device.c | 6 ++++-- 2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt index 67d0a58..747b984 100644 --- a/Documentation/devicetree/bindings/display/msm/gpu.txt +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt @@ -12,12 +12,6 @@ Required properties: * "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 -- qcom,gpu-pwrlevels: list of operating points - - compatible: "qcom,gpu-pwrlevels" - - for each qcom,gpu-pwrlevel: - - qcom,gpu-freq: requested gpu clock speed - - NOTE: downstream android driver defines additional parameters to - configure memory bandwidth scaling per OPP.
Example:
@@ -39,14 +33,5 @@ Example: <&mmcc GFX3D_AHB_CLK>, <&mmcc MMSS_IMEM_AHB_CLK>; qcom,chipid = <0x03020100>; - qcom,gpu-pwrlevels { - compatible = "qcom,gpu-pwrlevels"; - qcom,gpu-pwrlevel@0 { - qcom,gpu-freq = <450000000>; - }; - qcom,gpu-pwrlevel@1 { - qcom,gpu-freq = <27000000>; - }; - }; }; }; diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index e130b5e..7b9181b2 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -224,8 +224,10 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) }
if (!config.fast_rate) { - dev_err(dev, "could not find clk rates\n"); - return -ENXIO; + dev_warn(dev, "could not find clk rates\n"); + /* This is a safe low speed for all devices: */ + config.fast_rate = 200000000; + config.slow_rate = 27000000; }
for (i = 0; i < ARRAY_SIZE(quirks); i++)
Rob Clark robdclark@gmail.com writes:
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 robdclark@gmail.com
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?
On Mon, Jan 30, 2017 at 1:21 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
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 robdclark@gmail.com
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.
BR, -R
Rob Clark robdclark@gmail.com writes:
On Mon, Jan 30, 2017 at 1:21 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
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 robdclark@gmail.com
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.
Yeah, letting the upstream DT bind without the custom OPP stuff for now seems like progress. If we find that the safe fast freq is too high then we can drop it down later, and that would just put more pressure on getting the OPP work finished.
Reviewed-by: Eric Anholt eric@anholt.net
On Mon, Jan 30, 2017 at 01:35:47PM -0500, Rob Clark wrote:
On Mon, Jan 30, 2017 at 1:21 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
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 robdclark@gmail.com
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.
Rob has it right. On a fully optimized platform the bus does scale independently from the GPU but when we switch GPU levels up we need to immediately kick the bus to a new baseline level otherwise it underruns.
Eventually somebody will have to figure out how to make OPP work with both device and bus frequency. Perhaps that will happen by the time useful bus scaling hits the kernel, otherwise we will have to suffer along with two tables and a closer relationship between the GPU driver and the devfreq governor than any of us are comfortable with. Luckily for this discussion that someday is in the future and we can focus on doing the frqeuency right.
Jordan
On Mon, Jan 30, 2017 at 11:49:18AM -0500, Rob Clark wrote:
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 robdclark@gmail.com
Documentation/devicetree/bindings/display/msm/gpu.txt | 15 --------------- drivers/gpu/drm/msm/adreno/adreno_device.c | 6 ++++-- 2 files changed, 4 insertions(+), 17 deletions(-)
Acked-by: Rob Herring robh@kernel.org
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 robdclark@gmail.com --- .../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(-)
diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt index 747b984..7ac3052 100644 --- a/Documentation/devicetree/bindings/display/msm/gpu.txt +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt @@ -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>; }; }; diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 7b9181b2..fdaef67 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -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) +{ + 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)) { + 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" }, diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index e29bb66..6b85c41 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -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" }, { },
Rob Clark robdclark@gmail.com writes:
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?
On Mon, Jan 30, 2017 at 1:09 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
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?
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.
BR, -R
Rob Clark robdclark@gmail.com writes:
On Mon, Jan 30, 2017 at 1:09 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
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?
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.
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 eric@anholt.net
On Mon, Jan 30, 2017 at 2:54 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
On Mon, Jan 30, 2017 at 1:09 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
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?
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.
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.
well, the problem is still that we would end up needing entries for each gpu version + patchlevel, which I was trying to avoid.. I think otherwise, if we started adding more adreno variants the table would get quite large. The ANY_ID entries keep the table size down currently.
However, it's your driver. The code seems correct, and using specific compatible strings is an obviously good step.
I may possibly re-work this in the future, but was leaning more towards perhaps introducing some sort of of_match_device_wildcard(id, dev, ...) type function, and allowing a compat string match like "qcom,adreno-%1u%1u%1u.%u"
I guess maybe re-arranging this so multiple compat table entries point back to the same 'struct adreno_info' might be workable, but that list could still grow quite long. At any rate, that doesn't change how the bindings look so that can come later.
Reviewed-by: Eric Anholt eric@anholt.net
thanks
BR, -R
On Mon, Jan 30, 2017 at 11:49:19AM -0500, Rob Clark wrote:
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.
Fine for one driver/binding, but we wouldn't really want to do carry downstream compatibility for everything.
Signed-off-by: Rob Clark robdclark@gmail.com
.../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(-)
diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt index 747b984..7ac3052 100644 --- a/Documentation/devicetree/bindings/display/msm/gpu.txt +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt @@ -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";@@ -32,6 +34,5 @@ Example: <&mmcc GFX3D_CLK>, <&mmcc GFX3D_AHB_CLK>, <&mmcc MMSS_IMEM_AHB_CLK>;
qcom,chipid = <0x03020100>;}; diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 7b9181b2..fdaef67 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -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) +{
- 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 robh@kernel.org
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" },
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index e29bb66..6b85c41 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -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" }, { },
-- 2.9.3
On Wed, Feb 1, 2017 at 12:09 PM, Rob Herring robh@kernel.org wrote:
On Mon, Jan 30, 2017 at 11:49:19AM -0500, Rob Clark wrote:
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.
Fine for one driver/binding, but we wouldn't really want to do carry downstream compatibility for everything.
yeah, I'm not necessarily signing up to support the downstream bindings forever.. but for now the benefit outweighs the cost. We'll see how that goes when we have OPP and bus scaling in the mix.
Signed-off-by: Rob Clark robdclark@gmail.com
.../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(-)
diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt index 747b984..7ac3052 100644 --- a/Documentation/devicetree/bindings/display/msm/gpu.txt +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt @@ -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>; };}; diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 7b9181b2..fdaef67 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -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) +{
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.
I suppose that I don't need to have that restriction about being 1st string.. otoh, from looking at other dt nodes, that the more specific is supposed to come first, so I guess it doesn't hurt to enforce it..
With that,
Acked-by: Rob Herring robh@kernel.org
Thx
BR, -R
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 robdclark@gmail.com --- 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(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 5f8b368..71b30dd 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -543,7 +543,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu) /* Enable RBBM error reporting bits */ gpu_write(gpu, REG_A5XX_RBBM_AHB_CNTL0, 0x00000001);
- if (adreno_gpu->quirks & ADRENO_QUIRK_FAULT_DETECT_MASK) { + if (adreno_gpu->info->quirks & ADRENO_QUIRK_FAULT_DETECT_MASK) { /* * Mask out the activity signals from RB1-3 to avoid false * positives @@ -597,7 +597,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
gpu_write(gpu, REG_A5XX_PC_DBG_ECO_CNTL, (0x400 << 11 | 0x300 << 22));
- if (adreno_gpu->quirks & ADRENO_QUIRK_TWO_PASS_USE_WFI) + if (adreno_gpu->info->quirks & ADRENO_QUIRK_TWO_PASS_USE_WFI) gpu_rmw(gpu, REG_A5XX_PC_DBG_ECO_CNTL, 0, (1 << 8));
gpu_write(gpu, REG_A5XX_PC_DBG_ECO_CNTL, 0xc0200100); diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index fdaef67..4d0745d9 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -75,12 +75,14 @@ static const struct adreno_info gpulist[] = { .gmem = (SZ_1M + SZ_512K), .init = a4xx_gpu_init, }, { - .rev = ADRENO_REV(5, 3, 0, ANY_ID), + .rev = ADRENO_REV(5, 3, 0, 2), .revn = 530, .name = "A530", .pm4fw = "a530_pm4.fw", .pfpfw = "a530_pfp.fw", .gmem = SZ_1M, + .quirks = ADRENO_QUIRK_TWO_PASS_USE_WFI | + ADRENO_QUIRK_FAULT_DETECT_MASK, .init = a5xx_gpu_init, .gpmufw = "a530v3_gpmu.fw2", }, @@ -181,14 +183,6 @@ static void set_gpu_pdev(struct drm_device *dev, priv->gpu_pdev = pdev; }
-static const struct { - const char *str; - uint32_t flag; -} quirks[] = { - { "qcom,gpu-quirk-two-pass-use-wfi", ADRENO_QUIRK_TWO_PASS_USE_WFI }, - { "qcom,gpu-quirk-fault-detect-mask", ADRENO_QUIRK_FAULT_DETECT_MASK }, -}; - static int find_chipid(struct device *dev, u32 *chipid) { struct device_node *node = dev->of_node; @@ -234,7 +228,7 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) static struct adreno_platform_config config = {}; struct device_node *child, *node = dev->of_node; u32 val; - int ret, i; + int ret;
ret = find_chipid(dev, &val); if (ret) { @@ -270,10 +264,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) config.slow_rate = 27000000; }
- for (i = 0; i < ARRAY_SIZE(quirks); i++) - if (of_property_read_bool(node, quirks[i].str)) - config.quirks |= quirks[i].flag; - dev->platform_data = &config; set_gpu_pdev(dev_get_drvdata(master), to_platform_device(dev));
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index bc2224b..f67e6f8 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -352,7 +352,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, adreno_gpu->gmem = adreno_gpu->info->gmem; adreno_gpu->revn = adreno_gpu->info->revn; adreno_gpu->rev = config->rev; - adreno_gpu->quirks = config->quirks;
gpu->fast_rate = config->fast_rate; gpu->slow_rate = config->slow_rate; diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h index e8d55b0..42e444a 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h @@ -75,6 +75,7 @@ struct adreno_info { const char *pm4fw, *pfpfw; const char *gpmufw; uint32_t gmem; + enum adreno_quirks quirks; struct msm_gpu *(*init)(struct drm_device *dev); };
@@ -116,8 +117,6 @@ struct adreno_gpu { * code (a3xx_gpu.c) and stored in this common location. */ const unsigned int *reg_offsets; - - uint32_t quirks; }; #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
@@ -128,7 +127,6 @@ struct adreno_platform_config { #ifdef DOWNSTREAM_CONFIG_MSM_BUS_SCALING struct msm_bus_scale_pdata *bus_scale_table; #endif - uint32_t quirks; };
#define ADRENO_IDLE_TIMEOUT msecs_to_jiffies(1000)
Rob Clark robdclark@gmail.com writes:
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 robdclark@gmail.com
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(-)
Nice cleanup!
Reviewed-by: Eric Anholt eric@anholt.net
Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.
Cc: Rob Herring robh@kernel.org Signed-off-by: Rob Clark robdclark@gmail.com --- 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(-)
diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt index 7ac3052..43fac0f 100644 --- a/Documentation/devicetree/bindings/display/msm/gpu.txt +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt @@ -11,9 +11,9 @@ Required properties: - clocks: device clocks See ../clocks/clock-bindings.txt for details. - clock-names: the following clocks are required: - * "core_clk" - * "iface_clk" - * "mem_iface_clk" + * "core" + * "iface" + * "mem_iface"
Example:
@@ -27,9 +27,9 @@ Example: interrupts = <GIC_SPI 80 0>; interrupt-names = "kgsl_3d0_irq"; clock-names = - "core_clk", - "iface_clk", - "mem_iface_clk"; + "core", + "iface", + "mem_iface"; clocks = <&mmcc GFX3D_CLK>, <&mmcc GFX3D_AHB_CLK>, diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 6b85c41..a51d783 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -91,6 +91,25 @@ module_param(dumpstate, bool, 0600); * Util/helpers: */
+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); + + 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); + + return clk; +} + void __iomem *msm_ioremap(struct platform_device *pdev, const char *name, const char *dbgname) { diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index ed4dad3..5f6f48f 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -318,6 +318,7 @@ static inline int msm_debugfs_late_init(struct drm_device *dev) { return 0; } static inline void msm_rd_dump_submit(struct msm_gem_submit *submit) {} #endif
+struct clk *msm_clk_get(struct platform_device *pdev, const char *name); void __iomem *msm_ioremap(struct platform_device *pdev, const char *name, const char *dbgname); void msm_writel(u32 data, void __iomem *addr); diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index d8420be..7b29843 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -564,8 +564,7 @@ static irqreturn_t irq_handler(int irq, void *data) }
static const char *clk_names[] = { - "core_clk", "iface_clk", "rbbmtimer_clk", "mem_clk", - "mem_iface_clk", "alt_mem_iface_clk", + "core", "iface", "rbbmtimer", "mem", "mem_iface", "alt_mem_iface", };
int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev, @@ -629,13 +628,13 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
/* Acquire clocks: */ for (i = 0; i < ARRAY_SIZE(clk_names); i++) { - gpu->grp_clks[i] = devm_clk_get(&pdev->dev, clk_names[i]); + gpu->grp_clks[i] = msm_clk_get(pdev, clk_names[i]); DBG("grp_clks[%s]: %p", clk_names[i], gpu->grp_clks[i]); if (IS_ERR(gpu->grp_clks[i])) gpu->grp_clks[i] = NULL; }
- gpu->ebi1_clk = devm_clk_get(&pdev->dev, "bus_clk"); + gpu->ebi1_clk = msm_clk_get(pdev, "bus"); DBG("ebi1_clk: %p", gpu->ebi1_clk); if (IS_ERR(gpu->ebi1_clk)) gpu->ebi1_clk = NULL;
Rob Clark robdclark@gmail.com writes:
Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.
Cc: Rob Herring robh@kernel.org Signed-off-by: Rob Clark robdclark@gmail.com
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 eric@anholt.net
+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);
- 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?
On Mon, Jan 30, 2017 at 1:15 PM, Eric Anholt eric@anholt.net wrote:
Rob Clark robdclark@gmail.com writes:
Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.
Cc: Rob Herring robh@kernel.org Signed-off-by: Rob Clark robdclark@gmail.com
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 eric@anholt.net
+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);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?
oh, good point.. I've fixed that up locally. I don't think we'd actually hit this case currently for gpu clks, since for gpu this codepath happens on first open() of the device file. But I should mention I have a slighly sneaky ulterior motive, which is that we could use the same cleanup for display related clks (which do currently upstream use the _clk suffix).
BR, -R
On Mon, Jan 30, 2017 at 11:49:21AM -0500, Rob Clark wrote:
Suggested by Rob Herring. We still support the old names for compatibility with downstream android dt files.
Cc: Rob Herring robh@kernel.org Signed-off-by: Rob Clark robdclark@gmail.com
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(-)
Acked-by: Rob Herring robh@kernel.org
dri-devel@lists.freedesktop.org
-
Eric Anholt -
Jordan Crouse -
Rob Clark -
Rob Herring