| Message ID | 20170124171132.32761-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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@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 +---- > 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.
On Tue, Jan 24, 2017 at 1:01 PM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > 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@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 +---- >> 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). hmm, it would be nice to be able to just trust the hw.. assuming the registers are always at same offset (and do not tell a lie).. it would take a bit more re-work, since we'd need to go back and look for the register values after we have the clks, etc. >> 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. hmm, damn, you are right.. it does boot but at least on 8x96 it reboots somewhere in gpu-init. But I guess we can pick 200MHz safely. Really this was just a stop-gap until we have OPP table bindings. BR, -R > -- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark@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. > 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@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@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 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 26, 2017 at 2:11 PM, Rob Herring <robh@kernel.org> wrote: > On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark@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@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@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: 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 26, 2017 at 1:51 PM, Rob Clark <robdclark@gmail.com> wrote: > On Thu, Jan 26, 2017 at 2:11 PM, Rob Herring <robh@kernel.org> wrote: >> On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark@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.. I'm not saying you should. My point is gpu revision alone may not be enough. Things can get integrated or configured differently. You could use an SoC based compatible, read the revision registers for the revision, and override wrong revision registers based on SoC compatible (assuming that is rare). >>> 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.. Okay, missed that detail. In that case, one more thing below. > (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 ;-)) Glad *someone* sees the value. >>> (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@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" "_clk" here is redundant. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 26, 2017 at 4:09 PM, Rob Herring <robh@kernel.org> wrote: > On Thu, Jan 26, 2017 at 1:51 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Thu, Jan 26, 2017 at 2:11 PM, Rob Herring <robh@kernel.org> wrote: >>> On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark@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.. > > I'm not saying you should. My point is gpu revision alone may not be > enough. Things can get integrated or configured differently. You could > use an SoC based compatible, read the revision registers for the > revision, and override wrong revision registers based on SoC > compatible (assuming that is rare). Hmm, I'll probably defer to Jordan on this, since he has seen more combinations over the years. I think any integration differences would just amount to which clks/gdsc/etc are wired up. At least what I've seen in downstream kgsl driver, all the things that are conditional are purely keyed to the revision+patchlevel. As far as revision registers, IIRC back in the a3xx days they were not to be trusted. I'm not entirely sure when they became trustworthy. Last I checked, android kgsl driver was not using them. BR, -R >>>> 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.. > > Okay, missed that detail. In that case, one more thing below. > >> (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 ;-)) > > Glad *someone* sees the value. > >>>> (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@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" > > "_clk" here is redundant. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 26, 2017 at 05:03:54PM -0500, Rob Clark wrote: > On Thu, Jan 26, 2017 at 4:09 PM, Rob Herring <robh@kernel.org> wrote: > > On Thu, Jan 26, 2017 at 1:51 PM, Rob Clark <robdclark@gmail.com> wrote: > >> On Thu, Jan 26, 2017 at 2:11 PM, Rob Herring <robh@kernel.org> wrote: > >>> On Tue, Jan 24, 2017 at 11:11 AM, Rob Clark <robdclark@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.. > > > > I'm not saying you should. My point is gpu revision alone may not be > > enough. Things can get integrated or configured differently. You could > > use an SoC based compatible, read the revision registers for the > > revision, and override wrong revision registers based on SoC > > compatible (assuming that is rare). > > Hmm, I'll probably defer to Jordan on this, since he has seen more > combinations over the years. > > I think any integration differences would just amount to which > clks/gdsc/etc are wired up. At least what I've seen in downstream > kgsl driver, all the things that are conditional are purely keyed to > the revision+patchlevel. I think for all practical purposes there is a 1-1 mapping between a GPU version and a SoC at least over the last few years. The last time I can remember was an unfortunate series of revisions in the 3XX family which I think ended up being the catalyst for the hardware team getting their act together in this regard. > As far as revision registers, IIRC back in the a3xx days they were not > to be trusted. I'm not entirely sure when they became trustworthy. > Last I checked, android kgsl driver was not using them. We got burned to a crisp over these back in the day and I have never forgotten it so thats why the downstream driver is exclusively DT based (which I admit is probably too far in the other direction). These days I think they are mostly agreeable again (sometimes they don't get updated for metal spins but I think they even got that figured out too). I'm open to using revision registers again as long as we don't blindly trust them and give ourselves a way to override the version. Jordan
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; + + 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" }, { },
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@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 +---- drivers/gpu/drm/msm/adreno/adreno_device.c | 42 ++++++++++++++++++++-- drivers/gpu/drm/msm/msm_drv.c | 1 + 5 files changed, 47 insertions(+), 31 deletions(-)