On Tue, Jan 24, 2017 at 12:11:32PM -0500, Rob Clark wrote:
> So, cleaning up the GPU bindings is something that has been on my TODO
> list for a while, but always $bigger_fires. Existing bindings are a bit
> ugly, but served a purpose when too many of the other drivers the GPU
> depends on where still working their way upstream. But now enough of
> that is in place for a few devices, that we should start trying to get
> the GPU nodes into the upstream dts files.
>
> This drops the "qcom,chipid" property and parses the GPU revision out of
> the compat string. It does mean you need to list both "qcom,adreno" and
> the more specific string, for example "qcom,adreno-530.2". I'm not sure
> if that is "weird" or anyone has better ideas (hence this RFC).
>
> It also drops "qcom,gpu-pwrlevels". Jordan tells me we can probably
> replace that w/ OPP bindings, which seems more sane. For now, the code
> just falls back to a super-slow safe clock until we get the OPP bindings
> sorted.
>
> In both cases, the code is still compatible with the old bindings, so
> dts files that exist on top of upstream will still work. But it is
> removed from the documentation, and for merging GPU nodes in upstream
> dts files, we should use the new bindings.
>
> (I'll ofc split out the .dtsi changes, but figured for an RFC it was
> easier to include them in this patch for discussion.)
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
> ---
> .../devicetree/bindings/display/msm/gpu.txt | 24 +++----------
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 3 +-
> arch/arm64/boot/dts/qcom/msm8996.dtsi | 8 +----
> drivers/gpu/drm/msm/adreno/adreno_device.c | 42 ++++++++++++++++++++--
> drivers/gpu/drm/msm/msm_drv.c | 1 +
> 5 files changed, 47 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
> index 67d0a58..760194d 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,14 +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
> -- 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:
>
> @@ -38,15 +34,5 @@ Example:
> <&mmcc GFX3D_CLK>,
> <&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/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 681486c..6dcbda6 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -758,7 +758,7 @@
> };
>
> adreno-3xx@01c00000 {
> - compatible = "qcom,adreno-3xx";
> + compatible = "qcom,adreno-306.0", "qcom,adreno";
> #stream-id-cells = <16>;
> reg = <0x01c00000 0x20000>;
> reg-names = "kgsl_3d0_reg_memory";
> @@ -779,7 +779,6 @@
> <&gcc GCC_BIMC_GPU_CLK>,
> <&gcc GFX3D_CLK_SRC>;
> power-domains = <&gcc OXILI_GDSC>;
> - qcom,chipid = <0x03000600>;
> qcom,gpu-pwrlevels {
> compatible = "qcom,gpu-pwrlevels";
> qcom,gpu-pwrlevel@0 {
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index c5226a7..941493f 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -873,7 +873,7 @@
> };
>
> adreno-3xx@b00000 {
> - compatible = "qcom,adreno-3xx";
> + compatible = "qcom,adreno-530.2", "qcom,adreno";
> #stream-id-cells = <16>;
>
> reg = <0xb00000 0x3f000>;
> @@ -899,12 +899,6 @@
> power-domains = <&mmcc GPU_GDSC>;
> iommus = <&adreno_smmu 0>;
>
> - /* There are patchlevel 3 chips in the world (Snapdragon
> - * (820) but they are functionally similar to the 821 in
> - * the code so we can safely set the chipset as
> - * patchlevel 4. */
> - qcom,chipid = <0x05030002>;
> -
> qcom,gpu-quirk-two-pass-use-wfi;
> qcom,gpu-quirk-fault-detect-mask;
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index e130b5e..71300d0 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -189,6 +189,40 @@ static const struct {
> { "qcom,gpu-quirk-fault-detect-mask", ADRENO_QUIRK_FAULT_DETECT_MASK },
> };
>
> +static int find_chipid(struct device_node *node, u32 *chipid)
> +{
> + 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;
In theory we could use the qcom,adreno string as a clue that we should dare to
read the version from the hardware. I think that these days we are pretty good
with the hardware version as long as we have a fallback for when they mess it
up (and they will mess it up eventually).
> static int adreno_bind(struct device *dev, struct device *master, void *data)
> {
> static struct adreno_platform_config config = {};
> @@ -196,7 +230,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(node, &val);
> if (ret) {
> dev_err(dev, "could not find chipid: %d\n", ret);
> return ret;
> @@ -224,8 +258,9 @@ 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 = config.slow_rate = 27000000;
Does this boot? 27000000 was designed to be a non-operable level but I've never
figured out how the rounding is supposed to work. If it works, awesome,
otherwise for now we'll probably need to pick something in the mid 200s for thexi
fast rate and let the clk subsystem do its magic. Or make OPP go.
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
