Re: [Freedreno] [PATCH 1/4] drm/msm: remove qcom, gpu-pwrlevels bindings

[Jordan Crouse]

On Mon, Jan 30, 2017 at 01:35:47PM -0500, Rob Clark wrote:
> On Mon, Jan 30, 2017 at 1:21 PM, Eric Anholt  wrote:
> > Rob Clark  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 
> >
> > 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

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 2/4] drm/msm: drop qcom,chipid

[Rob Clark]

On Wed, Feb 1, 2017 at 12:09 PM, Rob Herring  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 
>> ---
>>  .../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@430 {
>> - compatible = "qcom,adreno-3xx";
>> + compatible = "qcom,adreno-320.2", "qcom,adreno";
>>   reg = <0x0430 0x2>;
>>   reg-names = "kgsl_3d0_reg_memory";
>>   interrupts = ;
>> @@ -32,6 +34,5 @@ Example:
>>   < GFX3D_CLK>,
>>   < GFX3D_AHB_CLK>,
>>   < 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 


Thx

BR,
-R
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 2/4] drm/msm: drop qcom,chipid

[Rob Herring]

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 
> ---
>  .../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@430 {
> - compatible = "qcom,adreno-3xx";
> + compatible = "qcom,adreno-320.2", "qcom,adreno";
>   reg = <0x0430 0x2>;
>   reg-names = "kgsl_3d0_reg_memory";
>   interrupts = ;
> @@ -32,6 +34,5 @@ Example:
>   < GFX3D_CLK>,
>   < GFX3D_AHB_CLK>,
>   < 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 


> + unsigned rev, patch;
> +
> + if (sscanf(compat, "qcom,adreno-%u.%u", , ) != 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", );
> + ret = find_chipid(dev, );
>   if (ret) {
>   dev_err(dev, "could not find chipid: %d\n", ret);
>   

Re: [Freedreno] [PATCH 1/4] drm/msm: remove qcom, gpu-pwrlevels bindings

[Rob Herring]

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 
> ---
>  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 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno