[Freedreno] [RFC] drm/msm/adreno: clean up gpu bindings
Rob Clark
robdclark at gmail.com
Thu Jan 26 19:51:58 UTC 2017
On Thu, Jan 26, 2017 at 2:11 PM, Rob Herring <robh at kernel.org> wrote:
> On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark at gmail.com> 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.
I'm not sure how common it is, but I'm pretty sure in the past I've
seen same gpu (but maybe not same patchlevel).
Also, there tend to be multiple revisions of the SoC which may or may
not bump the gpu patchlevel. I think it is quite likely to be
insanity to try and figure out gpu revision from SoC..
>> 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.
jfyi, I don't believe we have any adreno gpu nodes upstream..
(but as long as it is easy, I like to keep backwards compat since
otherwise I'll end up helping someone debug a bindings mismatch issue
sooner or later.. and I'm lazy that way ;-))
>> (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 at gmail.com>
>> ---
>> .../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 at 0 {
>> - qcom,gpu-freq = <450000000>;
>> - };
>> - qcom,gpu-pwrlevel at 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 at 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 at 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 at 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:
sorry, misleading comment in non-upstream dtsi file.. original patch
that went in integration branch had 0x05030004 (since I think Jordan
thought db820c had a 8x96-pro (821).. I fixed that up locally but
didn't change the comment.
I believe the revision we have in db820c is patchlevel 2. (Not
entirely sure which devices have patchlevel 3)
> "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?
That is my current plan. Not quite how the code works right now, but
we should not include these nodes in upstream .dtsi
(It does end up making the table of devices bigger, since currently in
some cases we can get away with wildcard entries for patchlevel.. but
the quirks are a property of the gpu revision so better not to have
them in dt)
BR,
-R
>>
>> 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
>>
More information about the Freedreno
mailing list