On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark@xxxxxxxxx> 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).
Is version and SoC close to a 1-1 mapping? If one version is in lots
of different SoCs, then you should have an SoC specific compatible.
> 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.
Good.
> (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 +----
Bindings need to go to DT list.
> 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 {
The node names should be 'gpu@' while you're at it.
> - 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>;
'2' is patchlevel 4? And I thought 8996 was 820? In any case, I would
expect the DT in these 2 cases to be different and let the kernel
decide they are "the same" as that could change at some point. You
would normally do this for compatible strings if you want to claim
they are the same:
"qcom,adreno-530.3", "qcom,adreno-530.2", "qcom,adreno";
> -
> qcom,gpu-quirk-two-pass-use-wfi;
> qcom,gpu-quirk-fault-detect-mask;
Can these be implied by the new compatible strings now?
>
> 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;
> +
> + return 0;
> +}
> +
> 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;
> }
>
> for (i = 0; i < ARRAY_SIZE(quirks); i++)
> @@ -263,6 +298,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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
