[PATCH v2 00/14] GMU-less A6xx support (A610, A619_holi)

[PATCH v2 00/14] GMU-less A6xx support (A610, A619_holi)

[Konrad Dybcio]

v1 -> v2:
- Fix A630 values in [2/14]
- Fix [6/14] for GMU-equipped GPUs

Link to v1: https://lore.kernel.org/linux-arm-msm/20230126151618.225127-1-konrad.dybcio@linaro.org/

This series concludes my couple-weeks-long suffering of figuring out
the ins and outs of the "non-standard" A6xx GPUs which feature no GMU.

The GMU functionality is essentially emulated by parting out a
"GMU wrapper" region, which is essentially just a register space
within the GPU. It's modeled to be as similar to the actual GMU
as possible while staying as unnecessary as we can make it - there's
no IRQs, communicating with a microcontroller, no RPMh communication
etc. etc. I tried to reuse as much code as possible without making
a mess where every even line is used for GMU and every odd line is
used for GMU wrapper..

This series contains:
- plumbing for non-GMU operation, if-ing out GMU calls based on
  GMU presence
- GMU wrapper support
- A610 support (w/ speedbin)
- A619 support (w/ speedbin)
- couple of minor fixes and improvements
- VDDCX/VDDGX scaling fix for non-GMU GPUs (concerns more than just
  A6xx)
- Enablement of opp interconnect properties

A619_holi works perfectly fine using the already-present A619 support
in mesa. A610 needs more work on that front, but can already replay
command traces captures on downstream.

NOTE: the "drm/msm/a6xx: Add support for A619_holi" patch contains
two occurences of 0x18 used in place of a register #define, as it's
supposed to be RBBM_GPR0_CNTL, but that will only be present after
mesa-side changes are merged and headers are synced from there.

Speedbin patches depend on:
https://lore.kernel.org/linux-arm-msm/20230120172233.1905761-1-konrad.dybcio@linaro.org/


Konrad Dybcio (14):
  drm/msm/a6xx: De-staticize sptprac en/disable functions
  drm/msm/a6xx: Extend UBWC config
  drm/msm/a6xx: Introduce GMU wrapper support
  drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init
  drm/msm/adreno: Disable has_cached_coherent for A610/A619_holi
  drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs
  drm/msm/a6xx: Add support for A619_holi
  drm/msm/a6xx: Add A610 support
  drm/msm/a6xx: Fix some A619 tunables
  drm/msm/a6xx: Fix up A6XX protected registers
  drm/msm/a6xx: Enable optional icc voting from OPP tables
  drm/msm/a6xx: Use "else if" in GPU speedbin rev matching
  drm/msm/a6xx: Add A619_holi speedbin support
  drm/msm/a6xx: Add A610 speedbin support

 drivers/gpu/drm/msm/adreno/a6xx_gmu.c       |  55 ++-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h       |   2 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c       | 427 +++++++++++++++++---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |   1 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c |  14 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c  |  34 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c     |   4 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.h     |  19 +-
 drivers/gpu/drm/msm/msm_gpu_devfreq.c       |   2 +-
 9 files changed, 492 insertions(+), 66 deletions(-)

-- 
2.39.1

[PATCH v2 01/14] drm/msm/a6xx: De-staticize sptprac en/disable functions

[Konrad Dybcio]

These two will be reused by at least A619_holi in the non-gmu
paths. De-staticize them to make it possible.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 ++--
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index f3c9600221d4..90e636dcdd5b 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -354,7 +354,7 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
 }
 
 /* Enable CPU control of SPTP power power collapse */
-static int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
+int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
 {
 	int ret;
 	u32 val;
@@ -376,7 +376,7 @@ static int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
 }
 
 /* Disable CPU control of SPTP power power collapse */
-static void a6xx_sptprac_disable(struct a6xx_gmu *gmu)
+void a6xx_sptprac_disable(struct a6xx_gmu *gmu)
 {
 	u32 val;
 	int ret;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index e034935b3986..ec28abdd327b 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -186,5 +186,7 @@ int a6xx_hfi_set_freq(struct a6xx_gmu *gmu, int index);
 
 bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu);
 bool a6xx_gmu_sptprac_is_on(struct a6xx_gmu *gmu);
+void a6xx_sptprac_disable(struct a6xx_gmu *gmu);
+int a6xx_sptprac_enable(struct a6xx_gmu *gmu);
 
 #endif
-- 
2.39.1

Re: [PATCH v2 01/14] drm/msm/a6xx: De-staticize sptprac en/disable functions

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> These two will be reused by at least A619_holi in the non-gmu
> paths. De-staticize them to make it possible.

Nit: 'remove static annotation' or something like that.

Other than that:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 ++--
>   drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 ++
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index f3c9600221d4..90e636dcdd5b 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -354,7 +354,7 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
>   }
>   
>   /* Enable CPU control of SPTP power power collapse */
> -static int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
> +int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
>   {
>   	int ret;
>   	u32 val;
> @@ -376,7 +376,7 @@ static int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
>   }
>   
>   /* Disable CPU control of SPTP power power collapse */
> -static void a6xx_sptprac_disable(struct a6xx_gmu *gmu)
> +void a6xx_sptprac_disable(struct a6xx_gmu *gmu)
>   {
>   	u32 val;
>   	int ret;
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> index e034935b3986..ec28abdd327b 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> @@ -186,5 +186,7 @@ int a6xx_hfi_set_freq(struct a6xx_gmu *gmu, int index);
>   
>   bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu);
>   bool a6xx_gmu_sptprac_is_on(struct a6xx_gmu *gmu);
> +void a6xx_sptprac_disable(struct a6xx_gmu *gmu);
> +int a6xx_sptprac_enable(struct a6xx_gmu *gmu);
>   
>   #endif

-- 
With best wishes
Dmitry

[PATCH v2 02/14] drm/msm/a6xx: Extend UBWC config

[Konrad Dybcio]

Port setting min_access_length, ubwc_mode and upper_bit from downstream.
Values were validated using downstream device trees for SM8[123]50 and
left default (as per downstream) elsewhere.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 29 +++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index c5f5d0bb3fdc..8855d798bbb3 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -786,17 +786,25 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
 static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
-	u32 lower_bit = 2;
+	u32 lower_bit = 1;
+	u32 upper_bit = 0;
 	u32 amsbc = 0;
 	u32 rgb565_predicator = 0;
 	u32 uavflagprd_inv = 0;
+	u32 min_acc_len = 0;
+	u32 ubwc_mode = 0;
 
 	/* a618 is using the hw default values */
 	if (adreno_is_a618(adreno_gpu))
 		return;
 
-	if (adreno_is_a640_family(adreno_gpu))
+	if (adreno_is_a630(adreno_gpu))
+		lower_bit = 2;
+
+	if (adreno_is_a640_family(adreno_gpu)) {
 		amsbc = 1;
+		lower_bit = 2;
+	}
 
 	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
 		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
@@ -807,18 +815,23 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
 	}
 
 	if (adreno_is_7c3(adreno_gpu)) {
-		lower_bit = 1;
 		amsbc = 1;
 		rgb565_predicator = 1;
 		uavflagprd_inv = 2;
 	}
 
 	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
-		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
-	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
-	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
-		uavflagprd_inv << 4 | lower_bit << 1);
-	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
+		  rgb565_predicator << 11 | upper_bit << 10 | amsbc << 4 |
+		  min_acc_len << 3 | lower_bit << 1 | ubwc_mode);
+
+	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, upper_bit << 4 |
+		  min_acc_len << 3 | lower_bit << 1 | ubwc_mode);
+
+	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, upper_bit << 10 |
+		  uavflagprd_inv << 4 | min_acc_len << 3 |
+		  lower_bit << 1 | ubwc_mode);
+
+	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | lower_bit << 21);
 }
 
 static int a6xx_cp_init(struct msm_gpu *gpu)
-- 
2.39.1

Re: [PATCH v2 02/14] drm/msm/a6xx: Extend UBWC config

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> Port setting min_access_length, ubwc_mode and upper_bit from downstream.
> Values were validated using downstream device trees for SM8[123]50 and
> left default (as per downstream) elsewhere.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 29 +++++++++++++++++++--------
>   1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index c5f5d0bb3fdc..8855d798bbb3 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -786,17 +786,25 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>   static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>   {
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> -	u32 lower_bit = 2;
> +	u32 lower_bit = 1;

Any reason to change the default value here?
If it is to match chipsets you are adding, it might be worth splitting 
this change to that patch.

> +	u32 upper_bit = 0;
>   	u32 amsbc = 0;
>   	u32 rgb565_predicator = 0;
>   	u32 uavflagprd_inv = 0;
> +	u32 min_acc_len = 0;
> +	u32 ubwc_mode = 0;
>   
>   	/* a618 is using the hw default values */
>   	if (adreno_is_a618(adreno_gpu))
>   		return;
>   
> -	if (adreno_is_a640_family(adreno_gpu))
> +	if (adreno_is_a630(adreno_gpu))
> +		lower_bit = 2;
> +
> +	if (adreno_is_a640_family(adreno_gpu)) {
>   		amsbc = 1;
> +		lower_bit = 2;
> +	}
>   
>   	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>   		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
> @@ -807,18 +815,23 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>   	}
>   
>   	if (adreno_is_7c3(adreno_gpu)) {
> -		lower_bit = 1;
>   		amsbc = 1;
>   		rgb565_predicator = 1;
>   		uavflagprd_inv = 2;
>   	}
>   
>   	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
> -		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
> -	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
> -	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
> -		uavflagprd_inv << 4 | lower_bit << 1);
> -	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
> +		  rgb565_predicator << 11 | upper_bit << 10 | amsbc << 4 |
> +		  min_acc_len << 3 | lower_bit << 1 | ubwc_mode);
> +
> +	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, upper_bit << 4 |
> +		  min_acc_len << 3 | lower_bit << 1 | ubwc_mode);
> +
> +	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, upper_bit << 10 |
> +		  uavflagprd_inv << 4 | min_acc_len << 3 |
> +		  lower_bit << 1 | ubwc_mode);
> +
> +	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | lower_bit << 21);
>   }
>   
>   static int a6xx_cp_init(struct msm_gpu *gpu)

-- 
With best wishes
Dmitry

Re: [PATCH v2 02/14] drm/msm/a6xx: Extend UBWC config

[Konrad Dybcio]

On 17.02.2023 21:46, Dmitry Baryshkov wrote:
> On 14/02/2023 19:31, Konrad Dybcio wrote:
>> Port setting min_access_length, ubwc_mode and upper_bit from downstream.
>> Values were validated using downstream device trees for SM8[123]50 and
>> left default (as per downstream) elsewhere.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 29 +++++++++++++++++++--------
>>   1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index c5f5d0bb3fdc..8855d798bbb3 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -786,17 +786,25 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>   static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>   {
>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> -    u32 lower_bit = 2;
>> +    u32 lower_bit = 1;
> 
> Any reason to change the default value here?
> If it is to match chipsets you are adding, it might be worth splitting this change to that patch.
Not really now that I think about it, especially since the
correct default value should be zero:

-- part of msm-4.19 --
bit = adreno_dev->highest_bank_bit ? adreno_dev->highest_bank_bit - 13 : 0;
lower_bit = bit & 0x3;
upper_bit = (bit >> 0x2) & 1;

where adreno_dev->highest_bank_bit is read from the dt property
"qcom,highest-bank-bit"

Anyway, I should be able to verify it for all the SoCs which
we support.

Konrad
> 
>> +    u32 upper_bit = 0;
>>       u32 amsbc = 0;
>>       u32 rgb565_predicator = 0;
>>       u32 uavflagprd_inv = 0;
>> +    u32 min_acc_len = 0;
>> +    u32 ubwc_mode = 0;
>>         /* a618 is using the hw default values */
>>       if (adreno_is_a618(adreno_gpu))
>>           return;
>>   -    if (adreno_is_a640_family(adreno_gpu))
>> +    if (adreno_is_a630(adreno_gpu))
>> +        lower_bit = 2;
>> +
>> +    if (adreno_is_a640_family(adreno_gpu)) {
>>           amsbc = 1;
>> +        lower_bit = 2;
>> +    }
>>         if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>>           /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>> @@ -807,18 +815,23 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>       }
>>         if (adreno_is_7c3(adreno_gpu)) {
>> -        lower_bit = 1;
>>           amsbc = 1;
>>           rgb565_predicator = 1;
>>           uavflagprd_inv = 2;
>>       }
>>         gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
>> -        rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
>> -    gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
>> -    gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
>> -        uavflagprd_inv << 4 | lower_bit << 1);
>> -    gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
>> +          rgb565_predicator << 11 | upper_bit << 10 | amsbc << 4 |
>> +          min_acc_len << 3 | lower_bit << 1 | ubwc_mode);
>> +
>> +    gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, upper_bit << 4 |
>> +          min_acc_len << 3 | lower_bit << 1 | ubwc_mode);
>> +
>> +    gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, upper_bit << 10 |
>> +          uavflagprd_inv << 4 | min_acc_len << 3 |
>> +          lower_bit << 1 | ubwc_mode);
>> +
>> +    gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | lower_bit << 21);
>>   }
>>     static int a6xx_cp_init(struct msm_gpu *gpu)
> 

[PATCH v2 03/14] drm/msm/a6xx: Introduce GMU wrapper support

[Konrad Dybcio]

Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
but don't implement the associated GMUs. This is due to the fact that
the GMU directly pokes at RPMh. Sadly, this means we have to take care
of enabling & scaling power rails, clocks and bandwidth ourselves.

Reuse existing Adreno-common code and modify the deeply-GMU-infused
A6XX code to facilitate these GPUs. This involves if-ing out lots
of GMU callbacks and introducing a new type of GMU - GMU wrapper.
This is essentially a register region which is convenient to model
as a device. We'll use it for managing the GDSCs.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c       |  51 ++++-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c       | 198 +++++++++++++++++---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |   1 +
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c |  14 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.h     |   6 +
 5 files changed, 233 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 90e636dcdd5b..5aa9f3ef41c2 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1474,6 +1474,7 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, struct platform_device *pdev,
 
 void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
 {
+	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
 	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
 	struct platform_device *pdev = to_platform_device(gmu->dev);
 
@@ -1493,10 +1494,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
 	gmu->mmio = NULL;
 	gmu->rscc = NULL;
 
-	a6xx_gmu_memory_free(gmu);
+	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
+		a6xx_gmu_memory_free(gmu);
 
-	free_irq(gmu->gmu_irq, gmu);
-	free_irq(gmu->hfi_irq, gmu);
+		free_irq(gmu->gmu_irq, gmu);
+		free_irq(gmu->hfi_irq, gmu);
+	}
 
 	/* Drop reference taken in of_find_device_by_node */
 	put_device(gmu->dev);
@@ -1504,6 +1507,48 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
 	gmu->initialized = false;
 }
 
+int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
+{
+	struct platform_device *pdev = of_find_device_by_node(node);
+	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
+	int ret;
+
+	if (!pdev)
+		return -ENODEV;
+
+	gmu->dev = &pdev->dev;
+
+	of_dma_configure(gmu->dev, node, true);
+
+	pm_runtime_enable(gmu->dev);
+
+	/* Mark legacy for manual SPTPRAC control */
+	gmu->legacy = true;
+
+	/* Map the GMU registers */
+	gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu");
+	if (IS_ERR(gmu->mmio)) {
+		ret = PTR_ERR(gmu->mmio);
+		goto err_mmio;
+	}
+
+	/* Get a link to the GX power domain to reset the GPU */
+	gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
+
+	gmu->initialized = true;
+
+	return 0;
+
+err_mmio:
+	iounmap(gmu->mmio);
+	ret = -ENODEV;
+
+	/* Drop reference taken in of_find_device_by_node */
+	put_device(gmu->dev);
+
+	return ret;
+}
+
 int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 {
 	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 8855d798bbb3..72bf5c9f7ff1 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -20,9 +20,11 @@ static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
 
-	/* Check that the GMU is idle */
-	if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
-		return false;
+	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
+		/* Check that the GMU is idle */
+		if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
+			return false;
+	}
 
 	/* Check tha the CX master is idle */
 	if (gpu_read(gpu, REG_A6XX_RBBM_STATUS) &
@@ -612,13 +614,15 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
 		return;
 
 	/* Disable SP clock before programming HWCG registers */
-	gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
+	if (!adreno_has_gmu_wrapper(adreno_gpu))
+		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
 
 	for (i = 0; (reg = &adreno_gpu->info->hwcg[i], reg->offset); i++)
 		gpu_write(gpu, reg->offset, state ? reg->value : 0);
 
 	/* Enable SP clock */
-	gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
+	if (!adreno_has_gmu_wrapper(adreno_gpu))
+		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
 
 	gpu_write(gpu, REG_A6XX_RBBM_CLOCK_CNTL, state ? clock_cntl_on : 0);
 }
@@ -994,10 +998,13 @@ static int hw_init(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
 	int ret;
 
-	/* Make sure the GMU keeps the GPU on while we set it up */
-	a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
+	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
+		/* Make sure the GMU keeps the GPU on while we set it up */
+		a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
+	}
 
 	/* Clear GBIF halt in case GX domain was not collapsed */
 	if (a6xx_has_gbif(adreno_gpu))
@@ -1123,6 +1130,17 @@ static int hw_init(struct msm_gpu *gpu)
 			0x3f0243f0);
 	}
 
+	if (adreno_has_gmu_wrapper(adreno_gpu)) {
+		/* Do it here, as GMU wrapper only inits the GMU for memory reservation etc. */
+
+		/* Set up the CX GMU counter 0 to count busy ticks */
+		gmu_write(gmu, REG_A6XX_GPU_GMU_AO_GPU_CX_BUSY_MASK, 0xff000000);
+
+		/* Enable power counter 0 */
+		gmu_rmw(gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_SELECT_0, 0xff, BIT(5));
+		gmu_write(gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_ENABLE, 1);
+	}
+
 	/* Protect registers from the CP */
 	a6xx_set_cp_protect(gpu);
 
@@ -1231,6 +1249,8 @@ static int hw_init(struct msm_gpu *gpu)
 	}
 
 out:
+	if (adreno_has_gmu_wrapper(adreno_gpu))
+		return ret;
 	/*
 	 * Tell the GMU that we are done touching the GPU and it can start power
 	 * management
@@ -1265,6 +1285,9 @@ static void a6xx_dump(struct msm_gpu *gpu)
 	adreno_dump(gpu);
 }
 
+#define GBIF_GX_HALT_MASK	BIT(0)
+#define GBIF_CLIENT_HALT_MASK	BIT(0)
+#define GBIF_ARB_HALT_MASK	BIT(1)
 #define VBIF_RESET_ACK_TIMEOUT	100
 #define VBIF_RESET_ACK_MASK	0x00f0
 
@@ -1296,7 +1319,8 @@ static void a6xx_recover(struct msm_gpu *gpu)
 	 * Turn off keep alive that might have been enabled by the hang
 	 * interrupt
 	 */
-	gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
+	if (!adreno_has_gmu_wrapper(adreno_gpu))
+		gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
 
 	pm_runtime_dont_use_autosuspend(&gpu->pdev->dev);
 
@@ -1320,6 +1344,35 @@ static void a6xx_recover(struct msm_gpu *gpu)
 	/* Call into gpucc driver to poll for cx gdsc collapse */
 	reset_control_reset(gpu->cx_collapse);
 
+	/* Software-reset the GPU */
+	if (adreno_has_gmu_wrapper(adreno_gpu)) {
+		/* Halt the GX side of GBIF */
+		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
+		spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
+			   GBIF_GX_HALT_MASK);
+
+		/* Halt new client requests on GBIF */
+		gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK);
+		spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) &
+			   (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK);
+
+		/* Halt all AXI requests on GBIF */
+		gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK);
+		spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) &
+			   (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK);
+
+		/* Clear the halts */
+		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
+
+		if (adreno_is_a619_holi(adreno_gpu))
+			gpu_write(gpu, 0x18, 0);
+		else
+			gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
+
+		/* This *really* needs to go through before we do anything else! */
+		mb();
+	}
+
 	pm_runtime_use_autosuspend(&gpu->pdev->dev);
 
 	if (active_submits)
@@ -1504,7 +1557,8 @@ static void a6xx_fault_detect_irq(struct msm_gpu *gpu)
 	 * Force the GPU to stay on until after we finish
 	 * collecting information
 	 */
-	gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 1);
+	if (!adreno_has_gmu_wrapper(adreno_gpu))
+		gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 1);
 
 	DRM_DEV_ERROR(&gpu->pdev->dev,
 		"gpu fault ring %d fence %x status %8.8X rb %4.4x/%4.4x ib1 %16.16llX/%4.4x ib2 %16.16llX/%4.4x\n",
@@ -1669,6 +1723,7 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
 	int ret;
 
 	gpu->needs_hw_init = true;
@@ -1676,35 +1731,81 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
 	trace_msm_gpu_resume(0);
 
 	mutex_lock(&a6xx_gpu->gmu.lock);
-	ret = a6xx_gmu_resume(a6xx_gpu);
-	mutex_unlock(&a6xx_gpu->gmu.lock);
-	if (ret)
-		return ret;
 
-	msm_devfreq_resume(gpu);
+	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
+		ret = a6xx_gmu_resume(a6xx_gpu);
+		mutex_unlock(&a6xx_gpu->gmu.lock);
+		if (ret)
+			return ret;
 
-	a6xx_llc_activate(a6xx_gpu);
+		msm_devfreq_resume(gpu);
 
-	return 0;
+		a6xx_llc_activate(a6xx_gpu);
+	} else {
+		pm_runtime_get_sync(gmu->dev);
+		if (!IS_ERR_OR_NULL(gmu->gxpd))
+			pm_runtime_get_sync(gmu->gxpd);
+
+		/* Set the core clock, having VDD scaling in mind */
+		ret = dev_pm_opp_set_rate(gpu->dev->dev, gpu->fast_rate);
+		if (ret)
+			return ret;
+
+		ret = clk_bulk_prepare_enable(gpu->nr_clocks, gpu->grp_clks);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(gpu->ebi1_clk);
+		if (ret)
+			return ret;
+
+		mutex_unlock(&a6xx_gpu->gmu.lock);
+
+		msm_devfreq_resume(gpu);
+	}
+
+	return ret;
 }
 
 static int a6xx_pm_suspend(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
 	int i, ret;
 
 	trace_msm_gpu_suspend(0);
 
-	a6xx_llc_deactivate(a6xx_gpu);
+	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
+		a6xx_llc_deactivate(a6xx_gpu);
 
-	msm_devfreq_suspend(gpu);
+		msm_devfreq_suspend(gpu);
 
-	mutex_lock(&a6xx_gpu->gmu.lock);
-	ret = a6xx_gmu_stop(a6xx_gpu);
-	mutex_unlock(&a6xx_gpu->gmu.lock);
-	if (ret)
-		return ret;
+		mutex_lock(&a6xx_gpu->gmu.lock);
+		ret = a6xx_gmu_stop(a6xx_gpu);
+		mutex_unlock(&a6xx_gpu->gmu.lock);
+		if (ret)
+			return ret;
+	} else {
+		msm_devfreq_suspend(gpu);
+
+		mutex_lock(&a6xx_gpu->gmu.lock);
+
+		ret = clk_prepare_enable(gpu->ebi1_clk);
+		if (ret)
+			return ret;
+
+		ret = clk_set_rate(gpu->core_clk, 0);
+		if (ret)
+			return ret;
+
+		if (!IS_ERR_OR_NULL(gmu->gxpd))
+			pm_runtime_put_sync(gmu->gxpd);
+
+		pm_runtime_put_sync(gmu->dev);
+
+		mutex_unlock(&a6xx_gpu->gmu.lock);
+	}
 
 	if (a6xx_gpu->shadow_bo)
 		for (i = 0; i < gpu->nr_rings; i++)
@@ -1720,6 +1821,11 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
 
+	if (adreno_has_gmu_wrapper(adreno_gpu)) {
+		*value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO);
+		return 0;
+	}
+
 	mutex_lock(&a6xx_gpu->gmu.lock);
 
 	/* Force the GPU power on so we can read this register */
@@ -1757,7 +1863,8 @@ static void a6xx_destroy(struct msm_gpu *gpu)
 		drm_gem_object_put(a6xx_gpu->shadow_bo);
 	}
 
-	a6xx_llc_slices_destroy(a6xx_gpu);
+	if (!adreno_has_gmu_wrapper(adreno_gpu))
+		a6xx_llc_slices_destroy(a6xx_gpu);
 
 	a6xx_gmu_remove(a6xx_gpu);
 
@@ -2020,6 +2127,34 @@ static const struct adreno_gpu_funcs funcs = {
 	.get_timestamp = a6xx_get_timestamp,
 };
 
+static const struct adreno_gpu_funcs funcs_gmuwrapper = {
+	.base = {
+		.get_param = adreno_get_param,
+		.set_param = adreno_set_param,
+		.hw_init = a6xx_hw_init,
+		.pm_suspend = a6xx_pm_suspend,
+		.pm_resume = a6xx_pm_resume,
+		.recover = a6xx_recover,
+		.submit = a6xx_submit,
+		.active_ring = a6xx_active_ring,
+		.irq = a6xx_irq,
+		.destroy = a6xx_destroy,
+#if defined(CONFIG_DRM_MSM_GPU_STATE)
+		.show = a6xx_show,
+#endif
+		.gpu_busy = a6xx_gpu_busy,
+#if defined(CONFIG_DRM_MSM_GPU_STATE)
+		.gpu_state_get = a6xx_gpu_state_get,
+		.gpu_state_put = a6xx_gpu_state_put,
+#endif
+		.create_address_space = a6xx_create_address_space,
+		.create_private_address_space = a6xx_create_private_address_space,
+		.get_rptr = a6xx_get_rptr,
+		.progress = a6xx_progress,
+	},
+	.get_timestamp = a6xx_get_timestamp,
+};
+
 struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -2052,7 +2187,9 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 			adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), info->rev)))
 		adreno_gpu->base.hw_apriv = true;
 
-	a6xx_llc_slices_init(pdev, a6xx_gpu);
+	/* No LLCC on non-RPMh (and by extension, non-GMU) SoCs */
+	if (info && info->quirks & ~ADRENO_QUIRK_GMU_WRAPPER)
+		a6xx_llc_slices_init(pdev, a6xx_gpu);
 
 	ret = a6xx_set_supported_hw(&pdev->dev, config->rev);
 	if (ret) {
@@ -2060,7 +2197,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 		return ERR_PTR(ret);
 	}
 
-	ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
+	if (info && info->quirks & ADRENO_QUIRK_GMU_WRAPPER)
+		ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_gmuwrapper, 1);
+	else
+		ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
 	if (ret) {
 		a6xx_destroy(&(a6xx_gpu->base.base));
 		return ERR_PTR(ret);
@@ -2075,11 +2215,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 
 	/* Check if there is a GMU phandle and set it up */
 	node = of_parse_phandle(pdev->dev.of_node, "qcom,gmu", 0);
-
 	/* FIXME: How do we gracefully handle this? */
 	BUG_ON(!node);
 
-	ret = a6xx_gmu_init(a6xx_gpu, node);
+	if (adreno_has_gmu_wrapper(adreno_gpu))
+		ret = a6xx_gmu_wrapper_init(a6xx_gpu, node);
+	else
+		ret = a6xx_gmu_init(a6xx_gpu, node);
 	of_node_put(node);
 	if (ret) {
 		a6xx_destroy(&(a6xx_gpu->base.base));
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
index eea2e60ce3b7..51a7656072fa 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
@@ -76,6 +76,7 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
 void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
 
 int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
+int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
 void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);
 
 void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index b7e217d00a22..e11e8a02ac22 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -1041,16 +1041,18 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
 	/* Get the generic state from the adreno core */
 	adreno_gpu_state_get(gpu, &a6xx_state->base);
 
-	a6xx_get_gmu_registers(gpu, a6xx_state);
+	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
+		a6xx_get_gmu_registers(gpu, a6xx_state);
 
-	a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
-	a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
-	a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
+		a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
+		a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
+		a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
 
-	a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
+		a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
+	}
 
 	/* If GX isn't on the rest of the data isn't going to be accessible */
-	if (!a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
+	if (!adreno_has_gmu_wrapper(adreno_gpu) && !a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
 		return &a6xx_state->base;
 
 	/* Get the banks of indexed registers */
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index b4f9b1343d63..7c5e0a90b5fb 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -32,6 +32,7 @@ enum {
 #define ADRENO_QUIRK_TWO_PASS_USE_WFI		BIT(0)
 #define ADRENO_QUIRK_FAULT_DETECT_MASK		BIT(1)
 #define ADRENO_QUIRK_LMLOADKILL_DISABLE		BIT(2)
+#define ADRENO_QUIRK_GMU_WRAPPER		BIT(3)
 
 struct adreno_rev {
 	uint8_t  core;
@@ -145,6 +146,11 @@ struct adreno_platform_config {
 
 bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
 
+static inline bool adreno_has_gmu_wrapper(struct adreno_gpu *gpu)
+{
+	return !!(gpu->info->quirks & ADRENO_QUIRK_GMU_WRAPPER);
+}
+
 static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
 {
 	return (gpu->revn < 300);
-- 
2.39.1

Re: [PATCH v2 03/14] drm/msm/a6xx: Introduce GMU wrapper support

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
> but don't implement the associated GMUs. This is due to the fact that
> the GMU directly pokes at RPMh. Sadly, this means we have to take care
> of enabling & scaling power rails, clocks and bandwidth ourselves.
> 
> Reuse existing Adreno-common code and modify the deeply-GMU-infused
> A6XX code to facilitate these GPUs. This involves if-ing out lots
> of GMU callbacks and introducing a new type of GMU - GMU wrapper.
> This is essentially a register region which is convenient to model
> as a device. We'll use it for managing the GDSCs.

Why do you call it a wrapper?

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c       |  51 ++++-
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c       | 198 +++++++++++++++++---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |   1 +
>   drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c |  14 +-
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h     |   6 +
>   5 files changed, 233 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 90e636dcdd5b..5aa9f3ef41c2 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -1474,6 +1474,7 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, struct platform_device *pdev,
>   
>   void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>   {
> +	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>   	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>   	struct platform_device *pdev = to_platform_device(gmu->dev);
>   
> @@ -1493,10 +1494,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>   	gmu->mmio = NULL;
>   	gmu->rscc = NULL;
>   
> -	a6xx_gmu_memory_free(gmu);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +		a6xx_gmu_memory_free(gmu);
>   
> -	free_irq(gmu->gmu_irq, gmu);
> -	free_irq(gmu->hfi_irq, gmu);
> +		free_irq(gmu->gmu_irq, gmu);
> +		free_irq(gmu->hfi_irq, gmu);
> +	}
>   
>   	/* Drop reference taken in of_find_device_by_node */
>   	put_device(gmu->dev);
> @@ -1504,6 +1507,48 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>   	gmu->initialized = false;
>   }
>   
> +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(node);
> +	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
> +	int ret;
> +
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	gmu->dev = &pdev->dev;
> +
> +	of_dma_configure(gmu->dev, node, true);
> +
> +	pm_runtime_enable(gmu->dev);
> +
> +	/* Mark legacy for manual SPTPRAC control */
> +	gmu->legacy = true;
> +
> +	/* Map the GMU registers */
> +	gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu");
> +	if (IS_ERR(gmu->mmio)) {
> +		ret = PTR_ERR(gmu->mmio);
> +		goto err_mmio;
> +	}
> +
> +	/* Get a link to the GX power domain to reset the GPU */
> +	gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
> +
> +	gmu->initialized = true;
> +
> +	return 0;
> +
> +err_mmio:
> +	iounmap(gmu->mmio);
> +	ret = -ENODEV;
> +
> +	/* Drop reference taken in of_find_device_by_node */
> +	put_device(gmu->dev);
> +
> +	return ret;
> +}
> +
>   int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
>   {
>   	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 8855d798bbb3..72bf5c9f7ff1 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -20,9 +20,11 @@ static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>   
> -	/* Check that the GMU is idle */
> -	if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
> -		return false;
> +	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +		/* Check that the GMU is idle */
> +		if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
> +			return false;
> +	}
>   
>   	/* Check tha the CX master is idle */
>   	if (gpu_read(gpu, REG_A6XX_RBBM_STATUS) &
> @@ -612,13 +614,15 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
>   		return;
>   
>   	/* Disable SP clock before programming HWCG registers */
> -	gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
>   
>   	for (i = 0; (reg = &adreno_gpu->info->hwcg[i], reg->offset); i++)
>   		gpu_write(gpu, reg->offset, state ? reg->value : 0);
>   
>   	/* Enable SP clock */
> -	gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
>   
>   	gpu_write(gpu, REG_A6XX_RBBM_CLOCK_CNTL, state ? clock_cntl_on : 0);
>   }
> @@ -994,10 +998,13 @@ static int hw_init(struct msm_gpu *gpu)
>   {
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> +	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>   	int ret;
>   
> -	/* Make sure the GMU keeps the GPU on while we set it up */
> -	a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +		/* Make sure the GMU keeps the GPU on while we set it up */
> +		a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
> +	}
>   
>   	/* Clear GBIF halt in case GX domain was not collapsed */
>   	if (a6xx_has_gbif(adreno_gpu))
> @@ -1123,6 +1130,17 @@ static int hw_init(struct msm_gpu *gpu)
>   			0x3f0243f0);
>   	}
>   
> +	if (adreno_has_gmu_wrapper(adreno_gpu)) {
> +		/* Do it here, as GMU wrapper only inits the GMU for memory reservation etc. */
> +
> +		/* Set up the CX GMU counter 0 to count busy ticks */
> +		gmu_write(gmu, REG_A6XX_GPU_GMU_AO_GPU_CX_BUSY_MASK, 0xff000000);
> +
> +		/* Enable power counter 0 */
> +		gmu_rmw(gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_SELECT_0, 0xff, BIT(5));
> +		gmu_write(gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_ENABLE, 1);
> +	}
> +
>   	/* Protect registers from the CP */
>   	a6xx_set_cp_protect(gpu);
>   
> @@ -1231,6 +1249,8 @@ static int hw_init(struct msm_gpu *gpu)
>   	}
>   
>   out:
> +	if (adreno_has_gmu_wrapper(adreno_gpu))
> +		return ret;
>   	/*
>   	 * Tell the GMU that we are done touching the GPU and it can start power
>   	 * management
> @@ -1265,6 +1285,9 @@ static void a6xx_dump(struct msm_gpu *gpu)
>   	adreno_dump(gpu);
>   }
>   
> +#define GBIF_GX_HALT_MASK	BIT(0)
> +#define GBIF_CLIENT_HALT_MASK	BIT(0)
> +#define GBIF_ARB_HALT_MASK	BIT(1)
>   #define VBIF_RESET_ACK_TIMEOUT	100
>   #define VBIF_RESET_ACK_MASK	0x00f0
>   
> @@ -1296,7 +1319,8 @@ static void a6xx_recover(struct msm_gpu *gpu)
>   	 * Turn off keep alive that might have been enabled by the hang
>   	 * interrupt
>   	 */
> -	gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +		gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
>   
>   	pm_runtime_dont_use_autosuspend(&gpu->pdev->dev);
>   
> @@ -1320,6 +1344,35 @@ static void a6xx_recover(struct msm_gpu *gpu)
>   	/* Call into gpucc driver to poll for cx gdsc collapse */
>   	reset_control_reset(gpu->cx_collapse);
>   
> +	/* Software-reset the GPU */
> +	if (adreno_has_gmu_wrapper(adreno_gpu)) {
> +		/* Halt the GX side of GBIF */
> +		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
> +		spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
> +			   GBIF_GX_HALT_MASK);
> +
> +		/* Halt new client requests on GBIF */
> +		gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK);
> +		spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) &
> +			   (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK);
> +
> +		/* Halt all AXI requests on GBIF */
> +		gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK);
> +		spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) &
> +			   (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK);
> +
> +		/* Clear the halts */
> +		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
> +
> +		if (adreno_is_a619_holi(adreno_gpu))
> +			gpu_write(gpu, 0x18, 0);
> +		else
> +			gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
> +
> +		/* This *really* needs to go through before we do anything else! */
> +		mb();
> +	}
> +
>   	pm_runtime_use_autosuspend(&gpu->pdev->dev);
>   
>   	if (active_submits)
> @@ -1504,7 +1557,8 @@ static void a6xx_fault_detect_irq(struct msm_gpu *gpu)
>   	 * Force the GPU to stay on until after we finish
>   	 * collecting information
>   	 */
> -	gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 1);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +		gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 1);
>   
>   	DRM_DEV_ERROR(&gpu->pdev->dev,
>   		"gpu fault ring %d fence %x status %8.8X rb %4.4x/%4.4x ib1 %16.16llX/%4.4x ib2 %16.16llX/%4.4x\n",
> @@ -1669,6 +1723,7 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>   {
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> +	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>   	int ret;
>   
>   	gpu->needs_hw_init = true;
> @@ -1676,35 +1731,81 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>   	trace_msm_gpu_resume(0);
>   
>   	mutex_lock(&a6xx_gpu->gmu.lock);
> -	ret = a6xx_gmu_resume(a6xx_gpu);
> -	mutex_unlock(&a6xx_gpu->gmu.lock);
> -	if (ret)
> -		return ret;
>   
> -	msm_devfreq_resume(gpu);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +		ret = a6xx_gmu_resume(a6xx_gpu);
> +		mutex_unlock(&a6xx_gpu->gmu.lock);
> +		if (ret)
> +			return ret;
>   
> -	a6xx_llc_activate(a6xx_gpu);
> +		msm_devfreq_resume(gpu);
>   
> -	return 0;
> +		a6xx_llc_activate(a6xx_gpu);
> +	} else {
> +		pm_runtime_get_sync(gmu->dev);
> +		if (!IS_ERR_OR_NULL(gmu->gxpd))
> +			pm_runtime_get_sync(gmu->gxpd);
> +
> +		/* Set the core clock, having VDD scaling in mind */
> +		ret = dev_pm_opp_set_rate(gpu->dev->dev, gpu->fast_rate);
> +		if (ret)
> +			return ret;

Both this function and the suspend one below would leave the mutex 
enabled in the error path.

I'd add a6xx_gmu_wrapper_suspend() / _resume() instead, call them 
conditionally close to the plain a6xx_gmu_suspend() / _resume and then 
add a !wrapper contition to the a6xx_llc_(de)activate() calls.

> +
> +		ret = clk_bulk_prepare_enable(gpu->nr_clocks, gpu->grp_clks);
> +		if (ret)
> +			return ret;
> +
> +		ret = clk_prepare_enable(gpu->ebi1_clk);
> +		if (ret)
> +			return ret;
> +
> +		mutex_unlock(&a6xx_gpu->gmu.lock);
> +
> +		msm_devfreq_resume(gpu);
> +	}
> +
> +	return ret;
>   }
>   
>   static int a6xx_pm_suspend(struct msm_gpu *gpu)
>   {
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> +	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>   	int i, ret;
>   
>   	trace_msm_gpu_suspend(0);
>   
> -	a6xx_llc_deactivate(a6xx_gpu);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +		a6xx_llc_deactivate(a6xx_gpu);
>   
> -	msm_devfreq_suspend(gpu);
> +		msm_devfreq_suspend(gpu);
>   
> -	mutex_lock(&a6xx_gpu->gmu.lock);
> -	ret = a6xx_gmu_stop(a6xx_gpu);
> -	mutex_unlock(&a6xx_gpu->gmu.lock);
> -	if (ret)
> -		return ret;
> +		mutex_lock(&a6xx_gpu->gmu.lock);
> +		ret = a6xx_gmu_stop(a6xx_gpu);
> +		mutex_unlock(&a6xx_gpu->gmu.lock);
> +		if (ret)
> +			return ret;
> +	} else {
> +		msm_devfreq_suspend(gpu);
> +
> +		mutex_lock(&a6xx_gpu->gmu.lock);
> +
> +		ret = clk_prepare_enable(gpu->ebi1_clk);
> +		if (ret)
> +			return ret;
> +
> +		ret = clk_set_rate(gpu->core_clk, 0);
> +		if (ret)
> +			return ret;
> +
> +		if (!IS_ERR_OR_NULL(gmu->gxpd))
> +			pm_runtime_put_sync(gmu->gxpd);
> +
> +		pm_runtime_put_sync(gmu->dev);
> +
> +		mutex_unlock(&a6xx_gpu->gmu.lock);
> +	}
>   
>   	if (a6xx_gpu->shadow_bo)
>   		for (i = 0; i < gpu->nr_rings; i++)
> @@ -1720,6 +1821,11 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>   
> +	if (adreno_has_gmu_wrapper(adreno_gpu)) {
> +		*value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO);
> +		return 0;
> +	}
> +
>   	mutex_lock(&a6xx_gpu->gmu.lock);
>   
>   	/* Force the GPU power on so we can read this register */
> @@ -1757,7 +1863,8 @@ static void a6xx_destroy(struct msm_gpu *gpu)
>   		drm_gem_object_put(a6xx_gpu->shadow_bo);
>   	}
>   
> -	a6xx_llc_slices_destroy(a6xx_gpu);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +		a6xx_llc_slices_destroy(a6xx_gpu);
>   
>   	a6xx_gmu_remove(a6xx_gpu);
>   
> @@ -2020,6 +2127,34 @@ static const struct adreno_gpu_funcs funcs = {
>   	.get_timestamp = a6xx_get_timestamp,
>   };
>   
> +static const struct adreno_gpu_funcs funcs_gmuwrapper = {
> +	.base = {
> +		.get_param = adreno_get_param,
> +		.set_param = adreno_set_param,
> +		.hw_init = a6xx_hw_init,
> +		.pm_suspend = a6xx_pm_suspend,
> +		.pm_resume = a6xx_pm_resume,
> +		.recover = a6xx_recover,
> +		.submit = a6xx_submit,
> +		.active_ring = a6xx_active_ring,
> +		.irq = a6xx_irq,
> +		.destroy = a6xx_destroy,
> +#if defined(CONFIG_DRM_MSM_GPU_STATE)
> +		.show = a6xx_show,
> +#endif
> +		.gpu_busy = a6xx_gpu_busy,
> +#if defined(CONFIG_DRM_MSM_GPU_STATE)
> +		.gpu_state_get = a6xx_gpu_state_get,
> +		.gpu_state_put = a6xx_gpu_state_put,
> +#endif
> +		.create_address_space = a6xx_create_address_space,
> +		.create_private_address_space = a6xx_create_private_address_space,
> +		.get_rptr = a6xx_get_rptr,
> +		.progress = a6xx_progress,
> +	},
> +	.get_timestamp = a6xx_get_timestamp,
> +};
> +
>   struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
> @@ -2052,7 +2187,9 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   			adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), info->rev)))
>   		adreno_gpu->base.hw_apriv = true;
>   
> -	a6xx_llc_slices_init(pdev, a6xx_gpu);
> +	/* No LLCC on non-RPMh (and by extension, non-GMU) SoCs */
> +	if (info && info->quirks & ~ADRENO_QUIRK_GMU_WRAPPER)
> +		a6xx_llc_slices_init(pdev, a6xx_gpu);
>   
>   	ret = a6xx_set_supported_hw(&pdev->dev, config->rev);
>   	if (ret) {
> @@ -2060,7 +2197,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   		return ERR_PTR(ret);
>   	}
>   
> -	ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
> +	if (info && info->quirks & ADRENO_QUIRK_GMU_WRAPPER)

I think !info should be a fatal error here.

Also you can set gpu->info early in this function and use the existing 
adreno_has_gmu_wrapper() instead of hand-coding it here.

> +		ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_gmuwrapper, 1);
> +	else
> +		ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
>   	if (ret) {
>   		a6xx_destroy(&(a6xx_gpu->base.base));
>   		return ERR_PTR(ret);
> @@ -2075,11 +2215,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   
>   	/* Check if there is a GMU phandle and set it up */
>   	node = of_parse_phandle(pdev->dev.of_node, "qcom,gmu", 0);
> -
>   	/* FIXME: How do we gracefully handle this? */
>   	BUG_ON(!node);
>   
> -	ret = a6xx_gmu_init(a6xx_gpu, node);
> +	if (adreno_has_gmu_wrapper(adreno_gpu))
> +		ret = a6xx_gmu_wrapper_init(a6xx_gpu, node);
> +	else
> +		ret = a6xx_gmu_init(a6xx_gpu, node);
>   	of_node_put(node);
>   	if (ret) {
>   		a6xx_destroy(&(a6xx_gpu->base.base));
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index eea2e60ce3b7..51a7656072fa 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -76,6 +76,7 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
>   void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
>   
>   int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
> +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
>   void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);
>   
>   void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index b7e217d00a22..e11e8a02ac22 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -1041,16 +1041,18 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
>   	/* Get the generic state from the adreno core */
>   	adreno_gpu_state_get(gpu, &a6xx_state->base);
>   
> -	a6xx_get_gmu_registers(gpu, a6xx_state);
> +	if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +		a6xx_get_gmu_registers(gpu, a6xx_state);
>   
> -	a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
> -	a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
> -	a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
> +		a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
> +		a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
> +		a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
>   
> -	a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
> +		a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
> +	}
>   
>   	/* If GX isn't on the rest of the data isn't going to be accessible */
> -	if (!a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
> +	if (!adreno_has_gmu_wrapper(adreno_gpu) && !a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
>   		return &a6xx_state->base;
>   
>   	/* Get the banks of indexed registers */
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index b4f9b1343d63..7c5e0a90b5fb 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -32,6 +32,7 @@ enum {
>   #define ADRENO_QUIRK_TWO_PASS_USE_WFI		BIT(0)
>   #define ADRENO_QUIRK_FAULT_DETECT_MASK		BIT(1)
>   #define ADRENO_QUIRK_LMLOADKILL_DISABLE		BIT(2)
> +#define ADRENO_QUIRK_GMU_WRAPPER		BIT(3)
>   
>   struct adreno_rev {
>   	uint8_t  core;
> @@ -145,6 +146,11 @@ struct adreno_platform_config {
>   
>   bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>   
> +static inline bool adreno_has_gmu_wrapper(struct adreno_gpu *gpu)
> +{
> +	return !!(gpu->info->quirks & ADRENO_QUIRK_GMU_WRAPPER);
> +}
> +
>   static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
>   {
>   	return (gpu->revn < 300);

-- 
With best wishes
Dmitry

Re: [PATCH v2 03/14] drm/msm/a6xx: Introduce GMU wrapper support

[Konrad Dybcio]

On 17.02.2023 22:37, Dmitry Baryshkov wrote:
> On 14/02/2023 19:31, Konrad Dybcio wrote:
>> Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
>> but don't implement the associated GMUs. This is due to the fact that
>> the GMU directly pokes at RPMh. Sadly, this means we have to take care
>> of enabling & scaling power rails, clocks and bandwidth ourselves.
>>
>> Reuse existing Adreno-common code and modify the deeply-GMU-infused
>> A6XX code to facilitate these GPUs. This involves if-ing out lots
>> of GMU callbacks and introducing a new type of GMU - GMU wrapper.
>> This is essentially a register region which is convenient to model
>> as a device. We'll use it for managing the GDSCs.
> 
> Why do you call it a wrapper?
That's what Qualcomm calls it.. The GMU-less GPUs have (almost) all the
same GMU GX/CX registers as the real GMUs in this 'wrapper' region, so
that lets us reuse some code with gmu_(read/write/rmw) calls.

> 
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c       |  51 ++++-
>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c       | 198 +++++++++++++++++---
>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |   1 +
>>   drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c |  14 +-
>>   drivers/gpu/drm/msm/adreno/adreno_gpu.h     |   6 +
>>   5 files changed, 233 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> index 90e636dcdd5b..5aa9f3ef41c2 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> @@ -1474,6 +1474,7 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, struct platform_device *pdev,
>>     void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>>   {
>> +    struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>>       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>       struct platform_device *pdev = to_platform_device(gmu->dev);
>>   @@ -1493,10 +1494,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>>       gmu->mmio = NULL;
>>       gmu->rscc = NULL;
>>   -    a6xx_gmu_memory_free(gmu);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        a6xx_gmu_memory_free(gmu);
>>   -    free_irq(gmu->gmu_irq, gmu);
>> -    free_irq(gmu->hfi_irq, gmu);
>> +        free_irq(gmu->gmu_irq, gmu);
>> +        free_irq(gmu->hfi_irq, gmu);
>> +    }
>>         /* Drop reference taken in of_find_device_by_node */
>>       put_device(gmu->dev);
>> @@ -1504,6 +1507,48 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>>       gmu->initialized = false;
>>   }
>>   +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
>> +{
>> +    struct platform_device *pdev = of_find_device_by_node(node);
>> +    struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>> +    int ret;
>> +
>> +    if (!pdev)
>> +        return -ENODEV;
>> +
>> +    gmu->dev = &pdev->dev;
>> +
>> +    of_dma_configure(gmu->dev, node, true);
>> +
>> +    pm_runtime_enable(gmu->dev);
>> +
>> +    /* Mark legacy for manual SPTPRAC control */
>> +    gmu->legacy = true;
>> +
>> +    /* Map the GMU registers */
>> +    gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu");
>> +    if (IS_ERR(gmu->mmio)) {
>> +        ret = PTR_ERR(gmu->mmio);
>> +        goto err_mmio;
>> +    }
>> +
>> +    /* Get a link to the GX power domain to reset the GPU */
>> +    gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
>> +
>> +    gmu->initialized = true;
>> +
>> +    return 0;
>> +
>> +err_mmio:
>> +    iounmap(gmu->mmio);
>> +    ret = -ENODEV;
>> +
>> +    /* Drop reference taken in of_find_device_by_node */
>> +    put_device(gmu->dev);
>> +
>> +    return ret;
>> +}
>> +
>>   int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
>>   {
>>       struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index 8855d798bbb3..72bf5c9f7ff1 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -20,9 +20,11 @@ static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>   -    /* Check that the GMU is idle */
>> -    if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
>> -        return false;
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        /* Check that the GMU is idle */
>> +        if (!a6xx_gmu_isidle(&a6xx_gpu->gmu))
>> +            return false;
>> +    }
>>         /* Check tha the CX master is idle */
>>       if (gpu_read(gpu, REG_A6XX_RBBM_STATUS) &
>> @@ -612,13 +614,15 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
>>           return;
>>         /* Disable SP clock before programming HWCG registers */
>> -    gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +        gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
>>         for (i = 0; (reg = &adreno_gpu->info->hwcg[i], reg->offset); i++)
>>           gpu_write(gpu, reg->offset, state ? reg->value : 0);
>>         /* Enable SP clock */
>> -    gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +        gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
>>         gpu_write(gpu, REG_A6XX_RBBM_CLOCK_CNTL, state ? clock_cntl_on : 0);
>>   }
>> @@ -994,10 +998,13 @@ static int hw_init(struct msm_gpu *gpu)
>>   {
>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +    struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>       int ret;
>>   -    /* Make sure the GMU keeps the GPU on while we set it up */
>> -    a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        /* Make sure the GMU keeps the GPU on while we set it up */
>> +        a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
>> +    }
>>         /* Clear GBIF halt in case GX domain was not collapsed */
>>       if (a6xx_has_gbif(adreno_gpu))
>> @@ -1123,6 +1130,17 @@ static int hw_init(struct msm_gpu *gpu)
>>               0x3f0243f0);
>>       }
>>   +    if (adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        /* Do it here, as GMU wrapper only inits the GMU for memory reservation etc. */
>> +
>> +        /* Set up the CX GMU counter 0 to count busy ticks */
>> +        gmu_write(gmu, REG_A6XX_GPU_GMU_AO_GPU_CX_BUSY_MASK, 0xff000000);
>> +
>> +        /* Enable power counter 0 */
>> +        gmu_rmw(gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_SELECT_0, 0xff, BIT(5));
>> +        gmu_write(gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_ENABLE, 1);
>> +    }
>> +
>>       /* Protect registers from the CP */
>>       a6xx_set_cp_protect(gpu);
>>   @@ -1231,6 +1249,8 @@ static int hw_init(struct msm_gpu *gpu)
>>       }
>>     out:
>> +    if (adreno_has_gmu_wrapper(adreno_gpu))
>> +        return ret;
>>       /*
>>        * Tell the GMU that we are done touching the GPU and it can start power
>>        * management
>> @@ -1265,6 +1285,9 @@ static void a6xx_dump(struct msm_gpu *gpu)
>>       adreno_dump(gpu);
>>   }
>>   +#define GBIF_GX_HALT_MASK    BIT(0)
>> +#define GBIF_CLIENT_HALT_MASK    BIT(0)
>> +#define GBIF_ARB_HALT_MASK    BIT(1)
>>   #define VBIF_RESET_ACK_TIMEOUT    100
>>   #define VBIF_RESET_ACK_MASK    0x00f0
>>   @@ -1296,7 +1319,8 @@ static void a6xx_recover(struct msm_gpu *gpu)
>>        * Turn off keep alive that might have been enabled by the hang
>>        * interrupt
>>        */
>> -    gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +        gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
>>         pm_runtime_dont_use_autosuspend(&gpu->pdev->dev);
>>   @@ -1320,6 +1344,35 @@ static void a6xx_recover(struct msm_gpu *gpu)
>>       /* Call into gpucc driver to poll for cx gdsc collapse */
>>       reset_control_reset(gpu->cx_collapse);
>>   +    /* Software-reset the GPU */
>> +    if (adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        /* Halt the GX side of GBIF */
>> +        gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
>> +        spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
>> +               GBIF_GX_HALT_MASK);
>> +
>> +        /* Halt new client requests on GBIF */
>> +        gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK);
>> +        spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) &
>> +               (GBIF_CLIENT_HALT_MASK)) == GBIF_CLIENT_HALT_MASK);
>> +
>> +        /* Halt all AXI requests on GBIF */
>> +        gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_ARB_HALT_MASK);
>> +        spin_until((gpu_read(gpu, REG_A6XX_GBIF_HALT_ACK) &
>> +               (GBIF_ARB_HALT_MASK)) == GBIF_ARB_HALT_MASK);
>> +
>> +        /* Clear the halts */
>> +        gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
>> +
>> +        if (adreno_is_a619_holi(adreno_gpu))
>> +            gpu_write(gpu, 0x18, 0);
>> +        else
>> +            gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
>> +
>> +        /* This *really* needs to go through before we do anything else! */
>> +        mb();
>> +    }
>> +
>>       pm_runtime_use_autosuspend(&gpu->pdev->dev);
>>         if (active_submits)
>> @@ -1504,7 +1557,8 @@ static void a6xx_fault_detect_irq(struct msm_gpu *gpu)
>>        * Force the GPU to stay on until after we finish
>>        * collecting information
>>        */
>> -    gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 1);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +        gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 1);
>>         DRM_DEV_ERROR(&gpu->pdev->dev,
>>           "gpu fault ring %d fence %x status %8.8X rb %4.4x/%4.4x ib1 %16.16llX/%4.4x ib2 %16.16llX/%4.4x\n",
>> @@ -1669,6 +1723,7 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>>   {
>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +    struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>       int ret;
>>         gpu->needs_hw_init = true;
>> @@ -1676,35 +1731,81 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>>       trace_msm_gpu_resume(0);
>>         mutex_lock(&a6xx_gpu->gmu.lock);
>> -    ret = a6xx_gmu_resume(a6xx_gpu);
>> -    mutex_unlock(&a6xx_gpu->gmu.lock);
>> -    if (ret)
>> -        return ret;
>>   -    msm_devfreq_resume(gpu);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        ret = a6xx_gmu_resume(a6xx_gpu);
>> +        mutex_unlock(&a6xx_gpu->gmu.lock);
>> +        if (ret)
>> +            return ret;
>>   -    a6xx_llc_activate(a6xx_gpu);
>> +        msm_devfreq_resume(gpu);
>>   -    return 0;
>> +        a6xx_llc_activate(a6xx_gpu);
>> +    } else {
>> +        pm_runtime_get_sync(gmu->dev);
>> +        if (!IS_ERR_OR_NULL(gmu->gxpd))
>> +            pm_runtime_get_sync(gmu->gxpd);
>> +
>> +        /* Set the core clock, having VDD scaling in mind */
>> +        ret = dev_pm_opp_set_rate(gpu->dev->dev, gpu->fast_rate);
>> +        if (ret)
>> +            return ret;
> 
> Both this function and the suspend one below would leave the mutex enabled in the error path.
> 
> I'd add a6xx_gmu_wrapper_suspend() / _resume() instead, call them conditionally close to the plain a6xx_gmu_suspend() / _resume and then add a !wrapper contition to the a6xx_llc_(de)activate() calls.
Sounds like a (much better) plan!

> 
>> +
>> +        ret = clk_bulk_prepare_enable(gpu->nr_clocks, gpu->grp_clks);
>> +        if (ret)
>> +            return ret;
>> +
>> +        ret = clk_prepare_enable(gpu->ebi1_clk);
>> +        if (ret)
>> +            return ret;
>> +
>> +        mutex_unlock(&a6xx_gpu->gmu.lock);
>> +
>> +        msm_devfreq_resume(gpu);
>> +    }
>> +
>> +    return ret;
>>   }
>>     static int a6xx_pm_suspend(struct msm_gpu *gpu)
>>   {
>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>> +    struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
>>       int i, ret;
>>         trace_msm_gpu_suspend(0);
>>   -    a6xx_llc_deactivate(a6xx_gpu);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        a6xx_llc_deactivate(a6xx_gpu);
>>   -    msm_devfreq_suspend(gpu);
>> +        msm_devfreq_suspend(gpu);
>>   -    mutex_lock(&a6xx_gpu->gmu.lock);
>> -    ret = a6xx_gmu_stop(a6xx_gpu);
>> -    mutex_unlock(&a6xx_gpu->gmu.lock);
>> -    if (ret)
>> -        return ret;
>> +        mutex_lock(&a6xx_gpu->gmu.lock);
>> +        ret = a6xx_gmu_stop(a6xx_gpu);
>> +        mutex_unlock(&a6xx_gpu->gmu.lock);
>> +        if (ret)
>> +            return ret;
>> +    } else {
>> +        msm_devfreq_suspend(gpu);
>> +
>> +        mutex_lock(&a6xx_gpu->gmu.lock);
>> +
>> +        ret = clk_prepare_enable(gpu->ebi1_clk);
>> +        if (ret)
>> +            return ret;
>> +
>> +        ret = clk_set_rate(gpu->core_clk, 0);
>> +        if (ret)
>> +            return ret;
>> +
>> +        if (!IS_ERR_OR_NULL(gmu->gxpd))
>> +            pm_runtime_put_sync(gmu->gxpd);
>> +
>> +        pm_runtime_put_sync(gmu->dev);
>> +
>> +        mutex_unlock(&a6xx_gpu->gmu.lock);
>> +    }
>>         if (a6xx_gpu->shadow_bo)
>>           for (i = 0; i < gpu->nr_rings; i++)
>> @@ -1720,6 +1821,11 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
>>       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>>   +    if (adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        *value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO);
>> +        return 0;
>> +    }
>> +
>>       mutex_lock(&a6xx_gpu->gmu.lock);
>>         /* Force the GPU power on so we can read this register */
>> @@ -1757,7 +1863,8 @@ static void a6xx_destroy(struct msm_gpu *gpu)
>>           drm_gem_object_put(a6xx_gpu->shadow_bo);
>>       }
>>   -    a6xx_llc_slices_destroy(a6xx_gpu);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +        a6xx_llc_slices_destroy(a6xx_gpu);
>>         a6xx_gmu_remove(a6xx_gpu);
>>   @@ -2020,6 +2127,34 @@ static const struct adreno_gpu_funcs funcs = {
>>       .get_timestamp = a6xx_get_timestamp,
>>   };
>>   +static const struct adreno_gpu_funcs funcs_gmuwrapper = {
>> +    .base = {
>> +        .get_param = adreno_get_param,
>> +        .set_param = adreno_set_param,
>> +        .hw_init = a6xx_hw_init,
>> +        .pm_suspend = a6xx_pm_suspend,
>> +        .pm_resume = a6xx_pm_resume,
>> +        .recover = a6xx_recover,
>> +        .submit = a6xx_submit,
>> +        .active_ring = a6xx_active_ring,
>> +        .irq = a6xx_irq,
>> +        .destroy = a6xx_destroy,
>> +#if defined(CONFIG_DRM_MSM_GPU_STATE)
>> +        .show = a6xx_show,
>> +#endif
>> +        .gpu_busy = a6xx_gpu_busy,
>> +#if defined(CONFIG_DRM_MSM_GPU_STATE)
>> +        .gpu_state_get = a6xx_gpu_state_get,
>> +        .gpu_state_put = a6xx_gpu_state_put,
>> +#endif
>> +        .create_address_space = a6xx_create_address_space,
>> +        .create_private_address_space = a6xx_create_private_address_space,
>> +        .get_rptr = a6xx_get_rptr,
>> +        .progress = a6xx_progress,
>> +    },
>> +    .get_timestamp = a6xx_get_timestamp,
>> +};
>> +
>>   struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>>   {
>>       struct msm_drm_private *priv = dev->dev_private;
>> @@ -2052,7 +2187,9 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>>               adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), info->rev)))
>>           adreno_gpu->base.hw_apriv = true;
>>   -    a6xx_llc_slices_init(pdev, a6xx_gpu);
>> +    /* No LLCC on non-RPMh (and by extension, non-GMU) SoCs */
>> +    if (info && info->quirks & ~ADRENO_QUIRK_GMU_WRAPPER)
>> +        a6xx_llc_slices_init(pdev, a6xx_gpu);
>>         ret = a6xx_set_supported_hw(&pdev->dev, config->rev);
>>       if (ret) {
>> @@ -2060,7 +2197,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>>           return ERR_PTR(ret);
>>       }
>>   -    ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
>> +    if (info && info->quirks & ADRENO_QUIRK_GMU_WRAPPER)
> 
> I think !info should be a fatal error here.
Right, without it, a6xx_gpu_init would never get called..

> 
> Also you can set gpu->info early in this function and use the existing adreno_has_gmu_wrapper() instead of hand-coding it here.
Ack

Konrad
> 
>> +        ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_gmuwrapper, 1);
>> +    else
>> +        ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
>>       if (ret) {
>>           a6xx_destroy(&(a6xx_gpu->base.base));
>>           return ERR_PTR(ret);
>> @@ -2075,11 +2215,13 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>>         /* Check if there is a GMU phandle and set it up */
>>       node = of_parse_phandle(pdev->dev.of_node, "qcom,gmu", 0);
>> -
>>       /* FIXME: How do we gracefully handle this? */
>>       BUG_ON(!node);
>>   -    ret = a6xx_gmu_init(a6xx_gpu, node);
>> +    if (adreno_has_gmu_wrapper(adreno_gpu))
>> +        ret = a6xx_gmu_wrapper_init(a6xx_gpu, node);
>> +    else
>> +        ret = a6xx_gmu_init(a6xx_gpu, node);
>>       of_node_put(node);
>>       if (ret) {
>>           a6xx_destroy(&(a6xx_gpu->base.base));
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> index eea2e60ce3b7..51a7656072fa 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> @@ -76,6 +76,7 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
>>   void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
>>     int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
>> +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
>>   void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);
>>     void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
>> index b7e217d00a22..e11e8a02ac22 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
>> @@ -1041,16 +1041,18 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
>>       /* Get the generic state from the adreno core */
>>       adreno_gpu_state_get(gpu, &a6xx_state->base);
>>   -    a6xx_get_gmu_registers(gpu, a6xx_state);
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu)) {
>> +        a6xx_get_gmu_registers(gpu, a6xx_state);
>>   -    a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
>> -    a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
>> -    a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
>> +        a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
>> +        a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
>> +        a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
>>   -    a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
>> +        a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
>> +    }
>>         /* If GX isn't on the rest of the data isn't going to be accessible */
>> -    if (!a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
>> +    if (!adreno_has_gmu_wrapper(adreno_gpu) && !a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
>>           return &a6xx_state->base;
>>         /* Get the banks of indexed registers */
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>> index b4f9b1343d63..7c5e0a90b5fb 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>> @@ -32,6 +32,7 @@ enum {
>>   #define ADRENO_QUIRK_TWO_PASS_USE_WFI        BIT(0)
>>   #define ADRENO_QUIRK_FAULT_DETECT_MASK        BIT(1)
>>   #define ADRENO_QUIRK_LMLOADKILL_DISABLE        BIT(2)
>> +#define ADRENO_QUIRK_GMU_WRAPPER        BIT(3)
>>     struct adreno_rev {
>>       uint8_t  core;
>> @@ -145,6 +146,11 @@ struct adreno_platform_config {
>>     bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>>   +static inline bool adreno_has_gmu_wrapper(struct adreno_gpu *gpu)
>> +{
>> +    return !!(gpu->info->quirks & ADRENO_QUIRK_GMU_WRAPPER);
>> +}
>> +
>>   static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
>>   {
>>       return (gpu->revn < 300);
> 

Re: [PATCH v2 03/14] drm/msm/a6xx: Introduce GMU wrapper support

[Dmitry Baryshkov]

On 17/02/2023 23:41, Konrad Dybcio wrote:
> 
> 
> On 17.02.2023 22:37, Dmitry Baryshkov wrote:
>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>> Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
>>> but don't implement the associated GMUs. This is due to the fact that
>>> the GMU directly pokes at RPMh. Sadly, this means we have to take care
>>> of enabling & scaling power rails, clocks and bandwidth ourselves.
>>>
>>> Reuse existing Adreno-common code and modify the deeply-GMU-infused
>>> A6XX code to facilitate these GPUs. This involves if-ing out lots
>>> of GMU callbacks and introducing a new type of GMU - GMU wrapper.
>>> This is essentially a register region which is convenient to model
>>> as a device. We'll use it for managing the GDSCs.
>>
>> Why do you call it a wrapper?
> That's what Qualcomm calls it.. The GMU-less GPUs have (almost) all the
> same GMU GX/CX registers as the real GMUs in this 'wrapper' region, so
> that lets us reuse some code with gmu_(read/write/rmw) calls.
> 

Ack. If you can add this to the commit message, it would be great.

-- 
With best wishes
Dmitry

Re: [PATCH v2 03/14] drm/msm/a6xx: Introduce GMU wrapper support

[Konrad Dybcio]

On 17.02.2023 22:44, Dmitry Baryshkov wrote:
> On 17/02/2023 23:41, Konrad Dybcio wrote:
>>
>>
>> On 17.02.2023 22:37, Dmitry Baryshkov wrote:
>>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>>> Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
>>>> but don't implement the associated GMUs. This is due to the fact that
>>>> the GMU directly pokes at RPMh. Sadly, this means we have to take care
>>>> of enabling & scaling power rails, clocks and bandwidth ourselves.
>>>>
>>>> Reuse existing Adreno-common code and modify the deeply-GMU-infused
>>>> A6XX code to facilitate these GPUs. This involves if-ing out lots
>>>> of GMU callbacks and introducing a new type of GMU - GMU wrapper.
>>>> This is essentially a register region which is convenient to model
>>>> as a device. We'll use it for managing the GDSCs.
>>>
>>> Why do you call it a wrapper?
>> That's what Qualcomm calls it.. The GMU-less GPUs have (almost) all the
>> same GMU GX/CX registers as the real GMUs in this 'wrapper' region, so
>> that lets us reuse some code with gmu_(read/write/rmw) calls.
>>
> 
> Ack. If you can add this to the commit message, it would be great.
Sure! I spent so much time on this that I can't really tell what's obvious
and what's not anymore, heh.

Konrad
> 

[PATCH v2 04/14] drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init

[Konrad Dybcio]

Currently we're only deasserting REG_A6XX_RBBM_GBIF_HALT, but we also
need REG_A6XX_GBIF_HALT to be set to 0. For GMU-equipped GPUs this is
done in a6xx_bus_clear_pending_transactions(), but for the GMU-less
ones we have to do it *somewhere*. Unhalting both side by side sounds
like a good plan and it won't cause any issues if it's unnecessary.

Also, add a memory barrier to ensure it's gone through.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 72bf5c9f7ff1..75cf94b03c29 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1007,8 +1007,12 @@ static int hw_init(struct msm_gpu *gpu)
 	}
 
 	/* Clear GBIF halt in case GX domain was not collapsed */
-	if (a6xx_has_gbif(adreno_gpu))
+	if (a6xx_has_gbif(adreno_gpu)) {
+		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
 		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
+		/* Let's make extra sure that the GPU can access the memory.. */
+		mb();
+	}
 
 	gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0);
 
-- 
2.39.1

[PATCH v2 05/14] drm/msm/adreno: Disable has_cached_coherent for A610/A619_holi

[Konrad Dybcio]

These SKUs don't support the feature. Disable it to make the GPU stop
crashing after almost each and every submission - the received data on
the GPU end was simply incomplete in garbled, resulting in almost nothing
being executed properly.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 36f062c7582f..82757f005a1a 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -540,7 +540,13 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 		config.rev.minor, config.rev.patchid);
 
 	priv->is_a2xx = config.rev.core == 2;
-	priv->has_cached_coherent = config.rev.core >= 6;
+
+	if (config.rev.core >= 6) {
+		/* Exclude A610 and A619_holi */
+		if (!(adreno_cmp_rev(ADRENO_REV(6, 1, 0, ANY_ID), config.rev) ||
+		      adreno_cmp_rev(ADRENO_REV(6, 1, 9, 1), config.rev)))
+			priv->has_cached_coherent = true;
+	}
 
 	gpu = info->init(drm);
 	if (IS_ERR(gpu)) {
-- 
2.39.1

Re: [PATCH v2 05/14] drm/msm/adreno: Disable has_cached_coherent for A610/A619_holi

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> These SKUs don't support the feature. Disable it to make the GPU stop
> crashing after almost each and every submission - the received data on
> the GPU end was simply incomplete in garbled, resulting in almost nothing
> being executed properly.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_device.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 36f062c7582f..82757f005a1a 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -540,7 +540,13 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
>   		config.rev.minor, config.rev.patchid);
>   
>   	priv->is_a2xx = config.rev.core == 2;
> -	priv->has_cached_coherent = config.rev.core >= 6;
> +
> +	if (config.rev.core >= 6) {
> +		/* Exclude A610 and A619_holi */
> +		if (!(adreno_cmp_rev(ADRENO_REV(6, 1, 0, ANY_ID), config.rev) ||
> +		      adreno_cmp_rev(ADRENO_REV(6, 1, 9, 1), config.rev)))
> +			priv->has_cached_coherent = true;
> +	}

I'd suggest something like:

if (config.rev.core >= 6 &&
     !(info.quirks & ADRENO_QUIRK_NO_CACHE_COHERENT))
         priv->has_cache_coherent = true;

Let's keep all the information and quirks in a single place.

>   
>   	gpu = info->init(drm);
>   	if (IS_ERR(gpu)) {

-- 
With best wishes
Dmitry

[PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Konrad Dybcio]

Currently we only utilize the OPP table connected to the GPU for
getting (available) frequencies. We do however need to scale the
voltage rail(s) accordingly to ensure that we aren't trying to
run the GPU at 1GHz with a VDD_LOW vote, as that would result in
an otherwise inexplainable hang.

Tell the OPP framework that we want to scale the "core" clock
and swap out the clk_set_rate to a dev_pm_opp_set_rate in
msm_devfreq_target() to enable usage of required-opps and by
extension proper voltage level/corner scaling.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
 drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index ce6b76c45b6f..15e405e4f977 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	const char *gpu_name;
 	u32 speedbin;
 
+	/* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
+	if (!IS_ERR(devm_clk_get(dev, "core")))
+		devm_pm_opp_set_clkname(dev, "core");
+
 	adreno_gpu->funcs = funcs;
 	adreno_gpu->info = adreno_info(config->rev);
 	adreno_gpu->gmem = adreno_gpu->info->gmem;
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index e27dbf12b5e8..ea70c1c32d94 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
 		gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
 		mutex_unlock(&df->lock);
 	} else {
-		clk_set_rate(gpu->core_clk, *freq);
+		dev_pm_opp_set_rate(dev, *freq);
 	}
 
 	dev_pm_opp_put(opp);
-- 
2.39.1

Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> Currently we only utilize the OPP table connected to the GPU for
> getting (available) frequencies. We do however need to scale the
> voltage rail(s) accordingly to ensure that we aren't trying to
> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
> an otherwise inexplainable hang.
> 
> Tell the OPP framework that we want to scale the "core" clock
> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
> msm_devfreq_target() to enable usage of required-opps and by
> extension proper voltage level/corner scaling.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
>   drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index ce6b76c45b6f..15e405e4f977 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   	const char *gpu_name;
>   	u32 speedbin;
>   
> +	/* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
> +	if (!IS_ERR(devm_clk_get(dev, "core")))
> +		devm_pm_opp_set_clkname(dev, "core");

Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin 
after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn 
sets gpu->core_clk.

Ideally you can call devm_pm_opp_set_clkname() from that function. Or 
maybe completely drop gpu->core_clk and always use 
devm_pm_opp_set_clk_rate().

> +
>   	adreno_gpu->funcs = funcs;
>   	adreno_gpu->info = adreno_info(config->rev);
>   	adreno_gpu->gmem = adreno_gpu->info->gmem;
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index e27dbf12b5e8..ea70c1c32d94 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>   		gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>   		mutex_unlock(&df->lock);
>   	} else {
> -		clk_set_rate(gpu->core_clk, *freq);
> +		dev_pm_opp_set_rate(dev, *freq);

This is not enough, there are calls to clk_set_rate(gpu->core_clk) in 
msm_gpu.c which are called from the suspend/resume path.

>   	}
>   
>   	dev_pm_opp_put(opp);

-- 
With best wishes
Dmitry

Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Konrad Dybcio]

On 17.02.2023 22:07, Dmitry Baryshkov wrote:
> On 14/02/2023 19:31, Konrad Dybcio wrote:
>> Currently we only utilize the OPP table connected to the GPU for
>> getting (available) frequencies. We do however need to scale the
>> voltage rail(s) accordingly to ensure that we aren't trying to
>> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
>> an otherwise inexplainable hang.
>>
>> Tell the OPP framework that we want to scale the "core" clock
>> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
>> msm_devfreq_target() to enable usage of required-opps and by
>> extension proper voltage level/corner scaling.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
>>   drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> index ce6b76c45b6f..15e405e4f977 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>       const char *gpu_name;
>>       u32 speedbin;
>>   +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
>> +    if (!IS_ERR(devm_clk_get(dev, "core")))
>> +        devm_pm_opp_set_clkname(dev, "core");
> 
> Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn sets gpu->core_clk.
> 
> Ideally you can call devm_pm_opp_set_clkname() from that function.


Or maybe completely drop gpu->core_clk and always use devm_pm_opp_set_clk_rate().
That would break non-OPP targets, last of which were probably added N=big years ago..
I'm not sure these would still work, as I think we've got rid of some ugly
clock getters that were looking for both "core" and "core_clk" etc.

See 8db0b6c7b636376789e356d861c3c6c35dcb6913 for what seems to be the most recent
example of non-OPP.

IMX51/53 also have no OPP tables and are using the (AFAIK) now-defunct _clk-suffixed
clock-names.

I'd be more than happy to rip out some of this legacy code and convert it
to something modern like OPP, but I'm not sure you guys would like it considering
the breakage on (arguably ancient and borderline retired) platforms.

This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
of which there are none..

> 
>> +
>>       adreno_gpu->funcs = funcs;
>>       adreno_gpu->info = adreno_info(config->rev);
>>       adreno_gpu->gmem = adreno_gpu->info->gmem;
>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> index e27dbf12b5e8..ea70c1c32d94 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>>           gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>>           mutex_unlock(&df->lock);
>>       } else {
>> -        clk_set_rate(gpu->core_clk, *freq);
>> +        dev_pm_opp_set_rate(dev, *freq);
> 
> This is not enough, there are calls to clk_set_rate(gpu->core_clk) in msm_gpu.c which are called from the suspend/resume path.
Right, good catch.

Konrad
> 
>>       }
>>         dev_pm_opp_put(opp);
> 

Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Dmitry Baryshkov]

On 18/02/2023 13:04, Konrad Dybcio wrote:
> 
> 
> On 17.02.2023 22:07, Dmitry Baryshkov wrote:
>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>> Currently we only utilize the OPP table connected to the GPU for
>>> getting (available) frequencies. We do however need to scale the
>>> voltage rail(s) accordingly to ensure that we aren't trying to
>>> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
>>> an otherwise inexplainable hang.
>>>
>>> Tell the OPP framework that we want to scale the "core" clock
>>> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
>>> msm_devfreq_target() to enable usage of required-opps and by
>>> extension proper voltage level/corner scaling.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> index ce6b76c45b6f..15e405e4f977 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>        const char *gpu_name;
>>>        u32 speedbin;
>>>    +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
>>> +    if (!IS_ERR(devm_clk_get(dev, "core")))
>>> +        devm_pm_opp_set_clkname(dev, "core");
>>
>> Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn sets gpu->core_clk.
>>
>> Ideally you can call devm_pm_opp_set_clkname() from that function.
> 
> 
>> Or maybe completely drop gpu->core_clk and always use devm_pm_opp_set_clk_rate().
> That would break non-OPP targets, last of which were probably added N=big years ago..

No. In the lack of OPP tables, dev_pm_opp_clk_set_rate() should behave 
exactly like the clk_set_rate().

> I'm not sure these would still work, as I think we've got rid of some ugly
> clock getters that were looking for both "core" and "core_clk" etc.

We still support core vs core_clk, see the get_clocks() at msm_gpu.c and 
then msm_clk_bulk_get_clock(). However we might mimick this function and 
call devm_pm_opp_set_clkname() with the proper name ("core" or "core_clk").

> 
> See 8db0b6c7b636376789e356d861c3c6c35dcb6913 for what seems to be the most recent
> example of non-OPP.
> 
> IMX51/53 also have no OPP tables and are using the (AFAIK) now-defunct _clk-suffixed
> clock-names.

It works, I tested it during this cycle.

> 
> I'd be more than happy to rip out some of this legacy code and convert it
> to something modern like OPP, but I'm not sure you guys would like it considering
> the breakage on (arguably ancient and borderline retired) platforms.

I think, we should try switching to OPP-for-everybody, granted the 
promise of dev_pm_opp_set_clk_rate() being backwards compatible with 
bare clk_set_rate().

> 
> This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
> of which there are none..
> 
>>
>>> +
>>>        adreno_gpu->funcs = funcs;
>>>        adreno_gpu->info = adreno_info(config->rev);
>>>        adreno_gpu->gmem = adreno_gpu->info->gmem;
>>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>> index e27dbf12b5e8..ea70c1c32d94 100644
>>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>>>            gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>>>            mutex_unlock(&df->lock);
>>>        } else {
>>> -        clk_set_rate(gpu->core_clk, *freq);
>>> +        dev_pm_opp_set_rate(dev, *freq);
>>
>> This is not enough, there are calls to clk_set_rate(gpu->core_clk) in msm_gpu.c which are called from the suspend/resume path.
> Right, good catch.
> 
> Konrad
>>
>>>        }
>>>          dev_pm_opp_put(opp);
>>

-- 
With best wishes
Dmitry

Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Konrad Dybcio]

On 18.02.2023 17:47, Dmitry Baryshkov wrote:
> On 18/02/2023 13:04, Konrad Dybcio wrote:
>>
>>
>> On 17.02.2023 22:07, Dmitry Baryshkov wrote:
>>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>>> Currently we only utilize the OPP table connected to the GPU for
>>>> getting (available) frequencies. We do however need to scale the
>>>> voltage rail(s) accordingly to ensure that we aren't trying to
>>>> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
>>>> an otherwise inexplainable hang.
>>>>
>>>> Tell the OPP framework that we want to scale the "core" clock
>>>> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
>>>> msm_devfreq_target() to enable usage of required-opps and by
>>>> extension proper voltage level/corner scaling.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
>>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
>>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> index ce6b76c45b6f..15e405e4f977 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>>        const char *gpu_name;
>>>>        u32 speedbin;
>>>>    +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
>>>> +    if (!IS_ERR(devm_clk_get(dev, "core")))
>>>> +        devm_pm_opp_set_clkname(dev, "core");
>>>
>>> Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn sets gpu->core_clk.
>>>
>>> Ideally you can call devm_pm_opp_set_clkname() from that function.
>>
>>
>>> Or maybe completely drop gpu->core_clk and always use devm_pm_opp_set_clk_rate().
>> That would break non-OPP targets, last of which were probably added N=big years ago..
> 
> No. In the lack of OPP tables, dev_pm_opp_clk_set_rate() should behave exactly like the clk_set_rate().
Not sure if that's what you meant, but if a device lacks OPP,
devm_pm_opp_set_rate will return -ENODEV.

If you meant "if we can't find an opp table, behave as if we
called clk_set_rate", a discussion on #freedreno with robclark
indicates he'd accept getting rid of non-opp code, provided we
construct a table if need be, since we have the data required
to do so ([FMIN=27MHz, FMAX=fast_rate]).

> 
>> I'm not sure these would still work, as I think we've got rid of some ugly
>> clock getters that were looking for both "core" and "core_clk" etc.
> 
> We still support core vs core_clk, see the get_clocks() at msm_gpu.c and then msm_clk_bulk_get_clock(). However we might mimick this function and call devm_pm_opp_set_clkname() with the proper name ("core" or "core_clk").
> 
>>
>> See 8db0b6c7b636376789e356d861c3c6c35dcb6913 for what seems to be the most recent
>> example of non-OPP.
>>
>> IMX51/53 also have no OPP tables and are using the (AFAIK) now-defunct _clk-suffixed
>> clock-names.
> 
> It works, I tested it during this cycle.
Oh okay, I had a feeling like that was dropped at one point..

> 
>>
>> I'd be more than happy to rip out some of this legacy code and convert it
>> to something modern like OPP, but I'm not sure you guys would like it considering
>> the breakage on (arguably ancient and borderline retired) platforms.
> 
> I think, we should try switching to OPP-for-everybody, granted the promise of dev_pm_opp_set_clk_rate() being backwards compatible with bare clk_set_rate().
It's not, but as I mentioned, we can easily work around that.

> 
>>
>> This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
>> of which there are none..
...but we want to get devfreq everywhere and it's a few LoC away..

Konrad
>>
>>>
>>>> +
>>>>        adreno_gpu->funcs = funcs;
>>>>        adreno_gpu->info = adreno_info(config->rev);
>>>>        adreno_gpu->gmem = adreno_gpu->info->gmem;
>>>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>>> index e27dbf12b5e8..ea70c1c32d94 100644
>>>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>>>>            gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>>>>            mutex_unlock(&df->lock);
>>>>        } else {
>>>> -        clk_set_rate(gpu->core_clk, *freq);
>>>> +        dev_pm_opp_set_rate(dev, *freq);
>>>
>>> This is not enough, there are calls to clk_set_rate(gpu->core_clk) in msm_gpu.c which are called from the suspend/resume path.
>> Right, good catch.
>>
>> Konrad
>>>
>>>>        }
>>>>          dev_pm_opp_put(opp);
>>>
> 

Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Konrad Dybcio]

On 20.02.2023 10:59, Konrad Dybcio wrote:
> 
> 
> On 18.02.2023 17:47, Dmitry Baryshkov wrote:
>> On 18/02/2023 13:04, Konrad Dybcio wrote:
>>>
>>>
>>> On 17.02.2023 22:07, Dmitry Baryshkov wrote:
>>>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>>>> Currently we only utilize the OPP table connected to the GPU for
>>>>> getting (available) frequencies. We do however need to scale the
>>>>> voltage rail(s) accordingly to ensure that we aren't trying to
>>>>> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
>>>>> an otherwise inexplainable hang.
>>>>>
>>>>> Tell the OPP framework that we want to scale the "core" clock
>>>>> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
>>>>> msm_devfreq_target() to enable usage of required-opps and by
>>>>> extension proper voltage level/corner scaling.
>>>>>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
>>>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
>>>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>>> index ce6b76c45b6f..15e405e4f977 100644
>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>>> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>>>        const char *gpu_name;
>>>>>        u32 speedbin;
>>>>>    +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
>>>>> +    if (!IS_ERR(devm_clk_get(dev, "core")))
>>>>> +        devm_pm_opp_set_clkname(dev, "core");
>>>>
>>>> Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn sets gpu->core_clk.
>>>>
>>>> Ideally you can call devm_pm_opp_set_clkname() from that function.
>>>
>>>
>>>> Or maybe completely drop gpu->core_clk and always use devm_pm_opp_set_clk_rate().
>>> That would break non-OPP targets, last of which were probably added N=big years ago..
>>
>> No. In the lack of OPP tables, dev_pm_opp_clk_set_rate() should behave exactly like the clk_set_rate().
> Not sure if that's what you meant, but if a device lacks OPP,
> devm_pm_opp_set_rate will return -ENODEV.
> 
> If you meant "if we can't find an opp table, behave as if we
> called clk_set_rate", a discussion on #freedreno with robclark
> indicates he'd accept getting rid of non-opp code, provided we
> construct a table if need be, since we have the data required
> to do so ([FMIN=27MHz, FMAX=fast_rate]).
Actually.. that's what happens for gpu-pwrlevels users already..
Well, use>r<, as apq8064 seems to have been the only user of
that upstream, ever..

And for A2XX it looks like it just unconditionally selects 200
MHz..

I think this could be simplified to:

if (opp exists)
    // use opp
else if (adreno_is_a2xx)
    dev_pm_opp_add(dev, 200000000, 0) //device, freq_hz, volt_uV
else if (adreno_is_a320)
    dev_pm_opp_add(dev, 450000000, 0)
else
    // for now the driver sets 200mhz here, but i don't think
    // it's reasonable to keep carrying that behavior for >a2xx
    return -EINVAL


And then we can yank out all clk_set_rate calls just like that!

Konrad
> 
>>
>>> I'm not sure these would still work, as I think we've got rid of some ugly
>>> clock getters that were looking for both "core" and "core_clk" etc.
>>
>> We still support core vs core_clk, see the get_clocks() at msm_gpu.c and then msm_clk_bulk_get_clock(). However we might mimick this function and call devm_pm_opp_set_clkname() with the proper name ("core" or "core_clk").
>>
>>>
>>> See 8db0b6c7b636376789e356d861c3c6c35dcb6913 for what seems to be the most recent
>>> example of non-OPP.
>>>
>>> IMX51/53 also have no OPP tables and are using the (AFAIK) now-defunct _clk-suffixed
>>> clock-names.
>>
>> It works, I tested it during this cycle.
> Oh okay, I had a feeling like that was dropped at one point..
> 
>>
>>>
>>> I'd be more than happy to rip out some of this legacy code and convert it
>>> to something modern like OPP, but I'm not sure you guys would like it considering
>>> the breakage on (arguably ancient and borderline retired) platforms.
>>
>> I think, we should try switching to OPP-for-everybody, granted the promise of dev_pm_opp_set_clk_rate() being backwards compatible with bare clk_set_rate().
> It's not, but as I mentioned, we can easily work around that.
> 
>>
>>>
>>> This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
>>> of which there are none..
> ...but we want to get devfreq everywhere and it's a few LoC away..
> 
> Konrad
>>>
>>>>
>>>>> +
>>>>>        adreno_gpu->funcs = funcs;
>>>>>        adreno_gpu->info = adreno_info(config->rev);
>>>>>        adreno_gpu->gmem = adreno_gpu->info->gmem;
>>>>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>>>> index e27dbf12b5e8..ea70c1c32d94 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>>>>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>>>>>            gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>>>>>            mutex_unlock(&df->lock);
>>>>>        } else {
>>>>> -        clk_set_rate(gpu->core_clk, *freq);
>>>>> +        dev_pm_opp_set_rate(dev, *freq);
>>>>
>>>> This is not enough, there are calls to clk_set_rate(gpu->core_clk) in msm_gpu.c which are called from the suspend/resume path.
>>> Right, good catch.
>>>
>>> Konrad
>>>>
>>>>>        }
>>>>>          dev_pm_opp_put(opp);
>>>>
>>

Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Dmitry Baryshkov]

On Mon, 20 Feb 2023 at 11:59, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> On 18.02.2023 17:47, Dmitry Baryshkov wrote:
> > On 18/02/2023 13:04, Konrad Dybcio wrote:
> >> On 17.02.2023 22:07, Dmitry Baryshkov wrote:
> >>> On 14/02/2023 19:31, Konrad Dybcio wrote:
> >>>> Currently we only utilize the OPP table connected to the GPU for
> >>>> getting (available) frequencies. We do however need to scale the
> >>>> voltage rail(s) accordingly to ensure that we aren't trying to
> >>>> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
> >>>> an otherwise inexplainable hang.
> >>>>
> >>>> Tell the OPP framework that we want to scale the "core" clock
> >>>> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
> >>>> msm_devfreq_target() to enable usage of required-opps and by
> >>>> extension proper voltage level/corner scaling.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
> >>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
> >>>>    2 files changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >>>> index ce6b76c45b6f..15e405e4f977 100644
> >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >>>> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> >>>>        const char *gpu_name;
> >>>>        u32 speedbin;
> >>>>    +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
> >>>> +    if (!IS_ERR(devm_clk_get(dev, "core")))
> >>>> +        devm_pm_opp_set_clkname(dev, "core");
> >>>
> >>> Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn sets gpu->core_clk.
> >>>
> >>> Ideally you can call devm_pm_opp_set_clkname() from that function.
> >>
> >>
> >>> Or maybe completely drop gpu->core_clk and always use devm_pm_opp_set_clk_rate().
> >> That would break non-OPP targets, last of which were probably added N=big years ago..
> >
> > No. In the lack of OPP tables, dev_pm_opp_clk_set_rate() should behave exactly like the clk_set_rate().
> Not sure if that's what you meant, but if a device lacks OPP,
> devm_pm_opp_set_rate will return -ENODEV.
>
> If you meant "if we can't find an opp table, behave as if we
> called clk_set_rate", a discussion on #freedreno with robclark
> indicates he'd accept getting rid of non-opp code, provided we
> construct a table if need be, since we have the data required
> to do so ([FMIN=27MHz, FMAX=fast_rate]).

I was referring to a comment at dev_pm_opp_set_rate():

/*
* For IO devices which require an OPP on some platforms/SoCs
* while just needing to scale the clock on some others
* we look for empty OPP tables with just a clock handle and
* scale only the clk. This makes dev_pm_opp_set_rate()
* equivalent to a clk_set_rate()
*/

Maybe we just need to make sure that the OPP table exists
(devm_pm_opp_of_add_table) to prevent the function from bailing out
early.

>
> >
> >> I'm not sure these would still work, as I think we've got rid of some ugly
> >> clock getters that were looking for both "core" and "core_clk" etc.
> >
> > We still support core vs core_clk, see the get_clocks() at msm_gpu.c and then msm_clk_bulk_get_clock(). However we might mimick this function and call devm_pm_opp_set_clkname() with the proper name ("core" or "core_clk").
> >
> >>
> >> See 8db0b6c7b636376789e356d861c3c6c35dcb6913 for what seems to be the most recent
> >> example of non-OPP.
> >>
> >> IMX51/53 also have no OPP tables and are using the (AFAIK) now-defunct _clk-suffixed
> >> clock-names.
> >
> > It works, I tested it during this cycle.
> Oh okay, I had a feeling like that was dropped at one point..
>
> >
> >>
> >> I'd be more than happy to rip out some of this legacy code and convert it
> >> to something modern like OPP, but I'm not sure you guys would like it considering
> >> the breakage on (arguably ancient and borderline retired) platforms.
> >
> > I think, we should try switching to OPP-for-everybody, granted the promise of dev_pm_opp_set_clk_rate() being backwards compatible with bare clk_set_rate().
> It's not, but as I mentioned, we can easily work around that.
>
> >
> >>
> >> This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
> >> of which there are none..
> ...but we want to get devfreq everywhere and it's a few LoC away..
>
> Konrad
> >>
> >>>
> >>>> +
> >>>>        adreno_gpu->funcs = funcs;
> >>>>        adreno_gpu->info = adreno_info(config->rev);
> >>>>        adreno_gpu->gmem = adreno_gpu->info->gmem;
> >>>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> >>>> index e27dbf12b5e8..ea70c1c32d94 100644
> >>>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> >>>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> >>>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
> >>>>            gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
> >>>>            mutex_unlock(&df->lock);
> >>>>        } else {
> >>>> -        clk_set_rate(gpu->core_clk, *freq);
> >>>> +        dev_pm_opp_set_rate(dev, *freq);
> >>>
> >>> This is not enough, there are calls to clk_set_rate(gpu->core_clk) in msm_gpu.c which are called from the suspend/resume path.
> >> Right, good catch.
> >>
> >> Konrad
> >>>
> >>>>        }
> >>>>          dev_pm_opp_put(opp);
> >>>
> >



-- 
With best wishes
Dmitry

[PATCH v2 07/14] drm/msm/a6xx: Add support for A619_holi

[Konrad Dybcio]

A619_holi is a GMU-less variant of the already-supported A619 GPU.
It's present on at least SM4350 (holi) and SM6375 (blair). No mesa
changes are required. Add the required kernel-side support for it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 37 +++++++++++++++++-----
 drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++++++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 +++
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 75cf94b03c29..c168712a0dc4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -614,14 +614,14 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
 		return;
 
 	/* Disable SP clock before programming HWCG registers */
-	if (!adreno_has_gmu_wrapper(adreno_gpu))
+	if ((!adreno_has_gmu_wrapper(adreno_gpu) || adreno_is_a619_holi(adreno_gpu)))
 		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
 
 	for (i = 0; (reg = &adreno_gpu->info->hwcg[i], reg->offset); i++)
 		gpu_write(gpu, reg->offset, state ? reg->value : 0);
 
 	/* Enable SP clock */
-	if (!adreno_has_gmu_wrapper(adreno_gpu))
+	if ((!adreno_has_gmu_wrapper(adreno_gpu) || adreno_is_a619_holi(adreno_gpu)))
 		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
 
 	gpu_write(gpu, REG_A6XX_RBBM_CLOCK_CNTL, state ? clock_cntl_on : 0);
@@ -1007,7 +1007,12 @@ static int hw_init(struct msm_gpu *gpu)
 	}
 
 	/* Clear GBIF halt in case GX domain was not collapsed */
-	if (a6xx_has_gbif(adreno_gpu)) {
+	if (adreno_is_a619_holi(adreno_gpu)) {
+		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
+		gpu_write(gpu, 0x18, 0);
+		/* Let's make extra sure that the GPU can access the memory.. */
+		mb();
+	} else if (a6xx_has_gbif(adreno_gpu)) {
 		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
 		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
 		/* Let's make extra sure that the GPU can access the memory.. */
@@ -1016,6 +1021,9 @@ static int hw_init(struct msm_gpu *gpu)
 
 	gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0);
 
+	if (adreno_is_a619_holi(adreno_gpu))
+		a6xx_sptprac_enable(gmu);
+
 	/*
 	 * Disable the trusted memory range - we don't actually supported secure
 	 * memory rendering at this point in time and we don't want to block off
@@ -1293,7 +1301,8 @@ static void a6xx_dump(struct msm_gpu *gpu)
 #define GBIF_CLIENT_HALT_MASK	BIT(0)
 #define GBIF_ARB_HALT_MASK	BIT(1)
 #define VBIF_RESET_ACK_TIMEOUT	100
-#define VBIF_RESET_ACK_MASK	0x00f0
+#define VBIF_RESET_ACK_MASK	0xF0
+#define GPR0_GBIF_HALT_REQUEST	0x1E0
 
 static void a6xx_recover(struct msm_gpu *gpu)
 {
@@ -1350,10 +1359,16 @@ static void a6xx_recover(struct msm_gpu *gpu)
 
 	/* Software-reset the GPU */
 	if (adreno_has_gmu_wrapper(adreno_gpu)) {
-		/* Halt the GX side of GBIF */
-		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
-		spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
-			   GBIF_GX_HALT_MASK);
+		if (adreno_is_a619_holi(adreno_gpu)) {
+			gpu_write(gpu, 0x18, GPR0_GBIF_HALT_REQUEST);
+			spin_until((gpu_read(gpu, REG_A6XX_RBBM_VBIF_GX_RESET_STATUS) &
+				   (VBIF_RESET_ACK_MASK)) == VBIF_RESET_ACK_MASK);
+		} else {
+			/* Halt the GX side of GBIF */
+			gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
+			spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
+				   GBIF_GX_HALT_MASK);
+		}
 
 		/* Halt new client requests on GBIF */
 		gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK);
@@ -1763,6 +1778,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
 		if (ret)
 			return ret;
 
+		if (adreno_is_a619_holi(adreno_gpu))
+			a6xx_sptprac_enable(gmu);
+
 		mutex_unlock(&a6xx_gpu->gmu.lock);
 
 		msm_devfreq_resume(gpu);
@@ -1795,6 +1813,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
 
 		mutex_lock(&a6xx_gpu->gmu.lock);
 
+		if (adreno_is_a619_holi(adreno_gpu))
+			a6xx_sptprac_disable(gmu);
+
 		ret = clk_prepare_enable(gpu->ebi1_clk);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 82757f005a1a..71faeb3fd466 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -264,6 +264,19 @@ static const struct adreno_info gpulist[] = {
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
 		.init = a6xx_gpu_init,
+	}, {
+		.rev = ADRENO_REV(6, 1, 9, 1),
+		.revn = 619,
+		.name = "A619_holi",
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_GMU_WRAPPER,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
 	}, {
 		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
 		.revn = 619,
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 7c5e0a90b5fb..16241368c2e4 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -252,6 +252,11 @@ static inline int adreno_is_a619(struct adreno_gpu *gpu)
 	return gpu->revn == 619;
 }
 
+static inline int adreno_is_a619_holi(struct adreno_gpu *gpu)
+{
+	return adreno_is_a619(gpu) && adreno_has_gmu_wrapper(gpu);
+}
+
 static inline int adreno_is_a630(struct adreno_gpu *gpu)
 {
 	return gpu->revn == 630;
-- 
2.39.1

Re: [PATCH v2 07/14] drm/msm/a6xx: Add support for A619_holi

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> A619_holi is a GMU-less variant of the already-supported A619 GPU.
> It's present on at least SM4350 (holi) and SM6375 (blair). No mesa
> changes are required. Add the required kernel-side support for it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 37 +++++++++++++++++-----
>   drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++++++++
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 +++
>   3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 75cf94b03c29..c168712a0dc4 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -614,14 +614,14 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
>   		return;
>   
>   	/* Disable SP clock before programming HWCG registers */
> -	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +	if ((!adreno_has_gmu_wrapper(adreno_gpu) || adreno_is_a619_holi(adreno_gpu)))

Extra parenthesis made me interpret this incorrectly. Maybe you can 
remove them and spit the condition onto two lines? Because my first 
interpretation was:
if (!(has_gmu_wrapper || a619_holi)).


>   		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
>   
>   	for (i = 0; (reg = &adreno_gpu->info->hwcg[i], reg->offset); i++)
>   		gpu_write(gpu, reg->offset, state ? reg->value : 0);
>   
>   	/* Enable SP clock */
> -	if (!adreno_has_gmu_wrapper(adreno_gpu))
> +	if ((!adreno_has_gmu_wrapper(adreno_gpu) || adreno_is_a619_holi(adreno_gpu)))
>   		gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
>   
>   	gpu_write(gpu, REG_A6XX_RBBM_CLOCK_CNTL, state ? clock_cntl_on : 0);
> @@ -1007,7 +1007,12 @@ static int hw_init(struct msm_gpu *gpu)
>   	}
>   
>   	/* Clear GBIF halt in case GX domain was not collapsed */
> -	if (a6xx_has_gbif(adreno_gpu)) {
> +	if (adreno_is_a619_holi(adreno_gpu)) {
> +		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
> +		gpu_write(gpu, 0x18, 0);
> +		/* Let's make extra sure that the GPU can access the memory.. */
> +		mb();
> +	} else if (a6xx_has_gbif(adreno_gpu)) {
>   		gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
>   		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
>   		/* Let's make extra sure that the GPU can access the memory.. */
> @@ -1016,6 +1021,9 @@ static int hw_init(struct msm_gpu *gpu)
>   
>   	gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0);
>   
> +	if (adreno_is_a619_holi(adreno_gpu))
> +		a6xx_sptprac_enable(gmu);
> +
>   	/*
>   	 * Disable the trusted memory range - we don't actually supported secure
>   	 * memory rendering at this point in time and we don't want to block off
> @@ -1293,7 +1301,8 @@ static void a6xx_dump(struct msm_gpu *gpu)
>   #define GBIF_CLIENT_HALT_MASK	BIT(0)
>   #define GBIF_ARB_HALT_MASK	BIT(1)
>   #define VBIF_RESET_ACK_TIMEOUT	100
> -#define VBIF_RESET_ACK_MASK	0x00f0
> +#define VBIF_RESET_ACK_MASK	0xF0
> +#define GPR0_GBIF_HALT_REQUEST	0x1E0
>   
>   static void a6xx_recover(struct msm_gpu *gpu)
>   {
> @@ -1350,10 +1359,16 @@ static void a6xx_recover(struct msm_gpu *gpu)
>   
>   	/* Software-reset the GPU */
>   	if (adreno_has_gmu_wrapper(adreno_gpu)) {
> -		/* Halt the GX side of GBIF */
> -		gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
> -		spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
> -			   GBIF_GX_HALT_MASK);
> +		if (adreno_is_a619_holi(adreno_gpu)) {
> +			gpu_write(gpu, 0x18, GPR0_GBIF_HALT_REQUEST);
> +			spin_until((gpu_read(gpu, REG_A6XX_RBBM_VBIF_GX_RESET_STATUS) &
> +				   (VBIF_RESET_ACK_MASK)) == VBIF_RESET_ACK_MASK);
> +		} else {
> +			/* Halt the GX side of GBIF */
> +			gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
> +			spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
> +				   GBIF_GX_HALT_MASK);
> +		}
>   
>   		/* Halt new client requests on GBIF */
>   		gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK);
> @@ -1763,6 +1778,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>   		if (ret)
>   			return ret;
>   
> +		if (adreno_is_a619_holi(adreno_gpu))
> +			a6xx_sptprac_enable(gmu);
> +
>   		mutex_unlock(&a6xx_gpu->gmu.lock);
>   
>   		msm_devfreq_resume(gpu);
> @@ -1795,6 +1813,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
>   
>   		mutex_lock(&a6xx_gpu->gmu.lock);
>   
> +		if (adreno_is_a619_holi(adreno_gpu))
> +			a6xx_sptprac_disable(gmu);
> +
>   		ret = clk_prepare_enable(gpu->ebi1_clk);
>   		if (ret)
>   			return ret;
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 82757f005a1a..71faeb3fd466 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -264,6 +264,19 @@ static const struct adreno_info gpulist[] = {
>   		.gmem = SZ_512K,
>   		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>   		.init = a6xx_gpu_init,
> +	}, {
> +		.rev = ADRENO_REV(6, 1, 9, 1),

I think this deserves a comment that GMU-enabled sm6350 has patch_id 0 
(if I interpreted the vendor dtsi correctly).

Another option might be to actually check for the qcom,gmu presense and 
add that to the selection conditional.

> +		.revn = 619,
> +		.name = "A619_holi",
> +		.fw = {
> +			[ADRENO_FW_SQE] = "a630_sqe.fw",
> +		},
> +		.gmem = SZ_512K,
> +		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
> +		.quirks = ADRENO_QUIRK_GMU_WRAPPER,
> +		.init = a6xx_gpu_init,
> +		.zapfw = "a615_zap.mdt",
> +		.hwcg = a615_hwcg,
>   	}, {
>   		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
>   		.revn = 619,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 7c5e0a90b5fb..16241368c2e4 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -252,6 +252,11 @@ static inline int adreno_is_a619(struct adreno_gpu *gpu)
>   	return gpu->revn == 619;
>   }
>   
> +static inline int adreno_is_a619_holi(struct adreno_gpu *gpu)
> +{
> +	return adreno_is_a619(gpu) && adreno_has_gmu_wrapper(gpu);
> +}
> +
>   static inline int adreno_is_a630(struct adreno_gpu *gpu)
>   {
>   	return gpu->revn == 630;

-- 
With best wishes
Dmitry

Re: [PATCH v2 07/14] drm/msm/a6xx: Add support for A619_holi

[Konrad Dybcio]

On 17.02.2023 22:19, Dmitry Baryshkov wrote:
> On 14/02/2023 19:31, Konrad Dybcio wrote:
>> A619_holi is a GMU-less variant of the already-supported A619 GPU.
>> It's present on at least SM4350 (holi) and SM6375 (blair). No mesa
>> changes are required. Add the required kernel-side support for it.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 37 +++++++++++++++++-----
>>   drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++++++++
>>   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 +++
>>   3 files changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index 75cf94b03c29..c168712a0dc4 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -614,14 +614,14 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
>>           return;
>>         /* Disable SP clock before programming HWCG registers */
>> -    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +    if ((!adreno_has_gmu_wrapper(adreno_gpu) || adreno_is_a619_holi(adreno_gpu)))
> 
> Extra parenthesis made me interpret this incorrectly. Maybe you can remove them and spit the condition onto two lines? Because my first interpretation was:
> if (!(has_gmu_wrapper || a619_holi)).
Yeah, I agree this is confusing.. will fix.

> 
> 
>>           gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 1, 0);
>>         for (i = 0; (reg = &adreno_gpu->info->hwcg[i], reg->offset); i++)
>>           gpu_write(gpu, reg->offset, state ? reg->value : 0);
>>         /* Enable SP clock */
>> -    if (!adreno_has_gmu_wrapper(adreno_gpu))
>> +    if ((!adreno_has_gmu_wrapper(adreno_gpu) || adreno_is_a619_holi(adreno_gpu)))
>>           gmu_rmw(gmu, REG_A6XX_GPU_GMU_GX_SPTPRAC_CLOCK_CONTROL, 0, 1);
>>         gpu_write(gpu, REG_A6XX_RBBM_CLOCK_CNTL, state ? clock_cntl_on : 0);
>> @@ -1007,7 +1007,12 @@ static int hw_init(struct msm_gpu *gpu)
>>       }
>>         /* Clear GBIF halt in case GX domain was not collapsed */
>> -    if (a6xx_has_gbif(adreno_gpu)) {
>> +    if (adreno_is_a619_holi(adreno_gpu)) {
>> +        gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
>> +        gpu_write(gpu, 0x18, 0);
>> +        /* Let's make extra sure that the GPU can access the memory.. */
>> +        mb();
>> +    } else if (a6xx_has_gbif(adreno_gpu)) {
>>           gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
>>           gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
>>           /* Let's make extra sure that the GPU can access the memory.. */
>> @@ -1016,6 +1021,9 @@ static int hw_init(struct msm_gpu *gpu)
>>         gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0);
>>   +    if (adreno_is_a619_holi(adreno_gpu))
>> +        a6xx_sptprac_enable(gmu);
>> +
>>       /*
>>        * Disable the trusted memory range - we don't actually supported secure
>>        * memory rendering at this point in time and we don't want to block off
>> @@ -1293,7 +1301,8 @@ static void a6xx_dump(struct msm_gpu *gpu)
>>   #define GBIF_CLIENT_HALT_MASK    BIT(0)
>>   #define GBIF_ARB_HALT_MASK    BIT(1)
>>   #define VBIF_RESET_ACK_TIMEOUT    100
>> -#define VBIF_RESET_ACK_MASK    0x00f0
>> +#define VBIF_RESET_ACK_MASK    0xF0
>> +#define GPR0_GBIF_HALT_REQUEST    0x1E0
>>     static void a6xx_recover(struct msm_gpu *gpu)
>>   {
>> @@ -1350,10 +1359,16 @@ static void a6xx_recover(struct msm_gpu *gpu)
>>         /* Software-reset the GPU */
>>       if (adreno_has_gmu_wrapper(adreno_gpu)) {
>> -        /* Halt the GX side of GBIF */
>> -        gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
>> -        spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
>> -               GBIF_GX_HALT_MASK);
>> +        if (adreno_is_a619_holi(adreno_gpu)) {
>> +            gpu_write(gpu, 0x18, GPR0_GBIF_HALT_REQUEST);
>> +            spin_until((gpu_read(gpu, REG_A6XX_RBBM_VBIF_GX_RESET_STATUS) &
>> +                   (VBIF_RESET_ACK_MASK)) == VBIF_RESET_ACK_MASK);
>> +        } else {
>> +            /* Halt the GX side of GBIF */
>> +            gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, GBIF_GX_HALT_MASK);
>> +            spin_until(gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT_ACK) &
>> +                   GBIF_GX_HALT_MASK);
>> +        }
>>             /* Halt new client requests on GBIF */
>>           gpu_write(gpu, REG_A6XX_GBIF_HALT, GBIF_CLIENT_HALT_MASK);
>> @@ -1763,6 +1778,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>>           if (ret)
>>               return ret;
>>   +        if (adreno_is_a619_holi(adreno_gpu))
>> +            a6xx_sptprac_enable(gmu);
>> +
>>           mutex_unlock(&a6xx_gpu->gmu.lock);
>>             msm_devfreq_resume(gpu);
>> @@ -1795,6 +1813,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
>>             mutex_lock(&a6xx_gpu->gmu.lock);
>>   +        if (adreno_is_a619_holi(adreno_gpu))
>> +            a6xx_sptprac_disable(gmu);
>> +
>>           ret = clk_prepare_enable(gpu->ebi1_clk);
>>           if (ret)
>>               return ret;
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
>> index 82757f005a1a..71faeb3fd466 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
>> @@ -264,6 +264,19 @@ static const struct adreno_info gpulist[] = {
>>           .gmem = SZ_512K,
>>           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>           .init = a6xx_gpu_init,
>> +    }, {
>> +        .rev = ADRENO_REV(6, 1, 9, 1),
> 
> I think this deserves a comment that GMU-enabled sm6350 has patch_id 0 (if I interpreted the vendor dtsi correctly).
> 
> Another option might be to actually check for the qcom,gmu presense and add that to the selection conditional.
We pass the GMU wrapper in qcom,gmu = <>, though perhaps setting
the holi-ness based on whether it's "qcom,gmu-x.y.z.a" or
"qcom,gmu-wrapper" would be wiser.. The patch ID is indeterminate
and I *think* one GMU-wrapper A619 has patch id 0..

Konrad
> 
>> +        .revn = 619,
>> +        .name = "A619_holi",
>> +        .fw = {
>> +            [ADRENO_FW_SQE] = "a630_sqe.fw",
>> +        },
>> +        .gmem = SZ_512K,
>> +        .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>> +        .quirks = ADRENO_QUIRK_GMU_WRAPPER,
>> +        .init = a6xx_gpu_init,
>> +        .zapfw = "a615_zap.mdt",
>> +        .hwcg = a615_hwcg,
>>       }, {
>>           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
>>           .revn = 619,
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>> index 7c5e0a90b5fb..16241368c2e4 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>> @@ -252,6 +252,11 @@ static inline int adreno_is_a619(struct adreno_gpu *gpu)
>>       return gpu->revn == 619;
>>   }
>>   +static inline int adreno_is_a619_holi(struct adreno_gpu *gpu)
>> +{
>> +    return adreno_is_a619(gpu) && adreno_has_gmu_wrapper(gpu);
>> +}
>> +
>>   static inline int adreno_is_a630(struct adreno_gpu *gpu)
>>   {
>>       return gpu->revn == 630;
> 

Re: [PATCH v2 07/14] drm/msm/a6xx: Add support for A619_holi

[Dmitry Baryshkov]

On 17/02/2023 23:21, Konrad Dybcio wrote:
> 
> 
> On 17.02.2023 22:19, Dmitry Baryshkov wrote:
>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>> A619_holi is a GMU-less variant of the already-supported A619 GPU.
>>> It's present on at least SM4350 (holi) and SM6375 (blair). No mesa
>>> changes are required. Add the required kernel-side support for it.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 37 +++++++++++++++++-----
>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++++++++
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 +++
>>>    3 files changed, 47 insertions(+), 8 deletions(-)

[...]


>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
>>> index 82757f005a1a..71faeb3fd466 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
>>> @@ -264,6 +264,19 @@ static const struct adreno_info gpulist[] = {
>>>            .gmem = SZ_512K,
>>>            .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>>            .init = a6xx_gpu_init,
>>> +    }, {
>>> +        .rev = ADRENO_REV(6, 1, 9, 1),
>>
>> I think this deserves a comment that GMU-enabled sm6350 has patch_id 0 (if I interpreted the vendor dtsi correctly).
>>
>> Another option might be to actually check for the qcom,gmu presense and add that to the selection conditional.
> We pass the GMU wrapper in qcom,gmu = <>, though perhaps setting
> the holi-ness based on whether it's "qcom,gmu-x.y.z.a" or
> "qcom,gmu-wrapper" would be wiser.. The patch ID is indeterminate
> and I *think* one GMU-wrapper A619 has patch id 0..

I was not aware that GMU-wrapper also adds a GMU device. In this case, 
checking the GMU's compatible strings sounds like a logical approach to me.

> 
> Konrad
>>
>>> +        .revn = 619,
>>> +        .name = "A619_holi",
>>> +        .fw = {
>>> +            [ADRENO_FW_SQE] = "a630_sqe.fw",
>>> +        },
>>> +        .gmem = SZ_512K,
>>> +        .inactive_period = DRM_MSM_INACTIVE_PERIOD,
>>> +        .quirks = ADRENO_QUIRK_GMU_WRAPPER,
>>> +        .init = a6xx_gpu_init,
>>> +        .zapfw = "a615_zap.mdt",
>>> +        .hwcg = a615_hwcg,
>>>        }, {
>>>            .rev = ADRENO_REV(6, 1, 9, ANY_ID),
>>>            .revn = 619,
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> index 7c5e0a90b5fb..16241368c2e4 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> @@ -252,6 +252,11 @@ static inline int adreno_is_a619(struct adreno_gpu *gpu)
>>>        return gpu->revn == 619;
>>>    }
>>>    +static inline int adreno_is_a619_holi(struct adreno_gpu *gpu)
>>> +{
>>> +    return adreno_is_a619(gpu) && adreno_has_gmu_wrapper(gpu);
>>> +}
>>> +
>>>    static inline int adreno_is_a630(struct adreno_gpu *gpu)
>>>    {
>>>        return gpu->revn == 630;
>>

-- 
With best wishes
Dmitry

[PATCH v2 08/14] drm/msm/a6xx: Add A610 support

[Konrad Dybcio]

A610 is one of (if not the) lowest-tier SKUs in the A6XX family. It
features no GMU, as it's implemented solely on SoCs with SMD_RPM.
What's more interesting is that it does not feature a VDDGX line
either, being powered solely by VDDCX and has an unfortunate hardware
quirk that makes its reset line broken - after a couple of assert/
deassert cycles, it will hang for good and will not wake up again.

This GPU requires mesa changes for proper rendering, and lots of them
at that. The command streams are quite far away from any other A6XX
GPU and hence it needs special care. This patch was validated both
by running an (incomplete) downstream mesa with some hacks (frames
rendered correctly, though some instructions made the GPU hangcheck
which is expected - garbage in, garbage out) and by replaying RD
traces captured with the downstream KGSL driver - no crashes there,
ever.

Add support for this GPU on the kernel side, which comes down to
pretty simply adding A612 HWCG tables, altering a few values and
adding a special case for handling the reset line.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 95 ++++++++++++++++++++--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 13 +++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  8 +-
 3 files changed, 106 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index c168712a0dc4..1e259e9901ca 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -254,6 +254,56 @@ static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
 	a6xx_flush(gpu, ring);
 }
 
+const struct adreno_reglist a612_hwcg[] = {
+	{REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
+	{REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
+	{REG_A6XX_RBBM_CLOCK_DELAY_SP0, 0x00000081},
+	{REG_A6XX_RBBM_CLOCK_HYST_SP0, 0x0000f3cf},
+	{REG_A6XX_RBBM_CLOCK_CNTL_TP0, 0x22222222},
+	{REG_A6XX_RBBM_CLOCK_CNTL2_TP0, 0x22222222},
+	{REG_A6XX_RBBM_CLOCK_CNTL3_TP0, 0x22222222},
+	{REG_A6XX_RBBM_CLOCK_CNTL4_TP0, 0x00022222},
+	{REG_A6XX_RBBM_CLOCK_DELAY_TP0, 0x11111111},
+	{REG_A6XX_RBBM_CLOCK_DELAY2_TP0, 0x11111111},
+	{REG_A6XX_RBBM_CLOCK_DELAY3_TP0, 0x11111111},
+	{REG_A6XX_RBBM_CLOCK_DELAY4_TP0, 0x00011111},
+	{REG_A6XX_RBBM_CLOCK_HYST_TP0, 0x77777777},
+	{REG_A6XX_RBBM_CLOCK_HYST2_TP0, 0x77777777},
+	{REG_A6XX_RBBM_CLOCK_HYST3_TP0, 0x77777777},
+	{REG_A6XX_RBBM_CLOCK_HYST4_TP0, 0x00077777},
+	{REG_A6XX_RBBM_CLOCK_CNTL_RB0, 0x22222222},
+	{REG_A6XX_RBBM_CLOCK_CNTL2_RB0, 0x01202222},
+	{REG_A6XX_RBBM_CLOCK_CNTL_CCU0, 0x00002220},
+	{REG_A6XX_RBBM_CLOCK_HYST_RB_CCU0, 0x00040f00},
+	{REG_A6XX_RBBM_CLOCK_CNTL_RAC, 0x05522022},
+	{REG_A6XX_RBBM_CLOCK_CNTL2_RAC, 0x00005555},
+	{REG_A6XX_RBBM_CLOCK_DELAY_RAC, 0x00000011},
+	{REG_A6XX_RBBM_CLOCK_HYST_RAC, 0x00445044},
+	{REG_A6XX_RBBM_CLOCK_CNTL_TSE_RAS_RBBM, 0x04222222},
+	{REG_A6XX_RBBM_CLOCK_MODE_VFD, 0x00002222},
+	{REG_A6XX_RBBM_CLOCK_MODE_GPC, 0x02222222},
+	{REG_A6XX_RBBM_CLOCK_DELAY_HLSQ_2, 0x00000002},
+	{REG_A6XX_RBBM_CLOCK_MODE_HLSQ, 0x00002222},
+	{REG_A6XX_RBBM_CLOCK_DELAY_TSE_RAS_RBBM, 0x00004000},
+	{REG_A6XX_RBBM_CLOCK_DELAY_VFD, 0x00002222},
+	{REG_A6XX_RBBM_CLOCK_DELAY_GPC, 0x00000200},
+	{REG_A6XX_RBBM_CLOCK_DELAY_HLSQ, 0x00000000},
+	{REG_A6XX_RBBM_CLOCK_HYST_TSE_RAS_RBBM, 0x00000000},
+	{REG_A6XX_RBBM_CLOCK_HYST_VFD, 0x00000000},
+	{REG_A6XX_RBBM_CLOCK_HYST_GPC, 0x04104004},
+	{REG_A6XX_RBBM_CLOCK_HYST_HLSQ, 0x00000000},
+	{REG_A6XX_RBBM_CLOCK_CNTL_UCHE, 0x22222222},
+	{REG_A6XX_RBBM_CLOCK_HYST_UCHE, 0x00000004},
+	{REG_A6XX_RBBM_CLOCK_DELAY_UCHE, 0x00000002},
+	{REG_A6XX_RBBM_ISDB_CNT, 0x00000182},
+	{REG_A6XX_RBBM_RAC_THRESHOLD_CNT, 0x00000000},
+	{REG_A6XX_RBBM_SP_HYST_CNT, 0x00000000},
+	{REG_A6XX_RBBM_CLOCK_CNTL_GMU_GX, 0x00000222},
+	{REG_A6XX_RBBM_CLOCK_DELAY_GMU_GX, 0x00000111},
+	{REG_A6XX_RBBM_CLOCK_HYST_GMU_GX, 0x00000555},
+	{},
+};
+
 /* For a615 family (a615, a616, a618 and a619) */
 const struct adreno_reglist a615_hwcg[] = {
 	{REG_A6XX_RBBM_CLOCK_CNTL_SP0,  0x02222222},
@@ -604,6 +654,8 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
 
 	if (adreno_is_a630(adreno_gpu))
 		clock_cntl_on = 0x8aa8aa02;
+	else if (adreno_is_a610(adreno_gpu))
+		clock_cntl_on = 0xaaa8aa82;
 	else
 		clock_cntl_on = 0x8aa8aa82;
 
@@ -798,6 +850,11 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
 	u32 min_acc_len = 0;
 	u32 ubwc_mode = 0;
 
+	if (adreno_is_a610(adreno_gpu)) {
+		min_acc_len = 1;
+		ubwc_mode = 1;
+	}
+
 	/* a618 is using the hw default values */
 	if (adreno_is_a618(adreno_gpu))
 		return;
@@ -1050,13 +1107,13 @@ static int hw_init(struct msm_gpu *gpu)
 	a6xx_set_hwcg(gpu, true);
 
 	/* VBIF/GBIF start*/
-	if (adreno_is_a640_family(adreno_gpu) ||
+	if (adreno_is_a610(adreno_gpu) ||
+	    adreno_is_a640_family(adreno_gpu) ||
 	    adreno_is_a650_family(adreno_gpu)) {
 		gpu_write(gpu, REG_A6XX_GBIF_QSB_SIDE0, 0x00071620);
 		gpu_write(gpu, REG_A6XX_GBIF_QSB_SIDE1, 0x00071620);
 		gpu_write(gpu, REG_A6XX_GBIF_QSB_SIDE2, 0x00071620);
 		gpu_write(gpu, REG_A6XX_GBIF_QSB_SIDE3, 0x00071620);
-		gpu_write(gpu, REG_A6XX_GBIF_QSB_SIDE3, 0x00071620);
 		gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x3);
 	} else {
 		gpu_write(gpu, REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL, 0x3);
@@ -1087,18 +1144,26 @@ static int hw_init(struct msm_gpu *gpu)
 	gpu_write(gpu, REG_A6XX_UCHE_FILTER_CNTL, 0x804);
 	gpu_write(gpu, REG_A6XX_UCHE_CACHE_WAYS, 0x4);
 
-	if (adreno_is_a640_family(adreno_gpu) ||
-	    adreno_is_a650_family(adreno_gpu))
+	if (adreno_is_a640_family(adreno_gpu) || adreno_is_a650_family(adreno_gpu)) {
 		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_2, 0x02000140);
-	else
+		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x8040362c);
+	} else if (adreno_is_a610(adreno_gpu)) {
+		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_2, 0x00800060);
+		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x40201b16);
+	} else {
 		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_2, 0x010000c0);
-	gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x8040362c);
+		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x8040362c);
+	}
 
 	if (adreno_is_a660_family(adreno_gpu))
 		gpu_write(gpu, REG_A6XX_CP_LPAC_PROG_FIFO_SIZE, 0x00000020);
 
 	/* Setting the mem pool size */
-	gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, 128);
+	if (adreno_is_a610(adreno_gpu)) {
+		gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, 48);
+		gpu_write(gpu, REG_A6XX_CP_MEM_POOL_DBG_ADDR, 47);
+	} else
+		gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, 128);
 
 	/* Setting the primFifo thresholds default values,
 	 * and vccCacheSkipDis=1 bit (0x200) for A640 and newer
@@ -1109,6 +1174,8 @@ static int hw_init(struct msm_gpu *gpu)
 		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00200200);
 	else if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu))
 		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00300200);
+	else if (adreno_is_a610(adreno_gpu))
+		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00080000);
 	else
 		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00180000);
 
@@ -1124,8 +1191,10 @@ static int hw_init(struct msm_gpu *gpu)
 	a6xx_set_ubwc_config(gpu);
 
 	/* Enable fault detection */
-	gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL,
-		(1 << 30) | 0x1fffff);
+	if (adreno_is_a610(adreno_gpu))
+		gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30) | 0x3ffff);
+	else
+		gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30) | 0x1fffff);
 
 	gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, 1);
 
@@ -1359,6 +1428,14 @@ static void a6xx_recover(struct msm_gpu *gpu)
 
 	/* Software-reset the GPU */
 	if (adreno_has_gmu_wrapper(adreno_gpu)) {
+		/* 11nm chips (i.e. A610-hosting ones) have HW issues with the reset line */
+		if (!adreno_is_a610(adreno_gpu)) {
+			gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
+			gpu_read(gpu, REG_A6XX_RBBM_SW_RESET_CMD);
+			udelay(100);
+			gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 0);
+		}
+
 		if (adreno_is_a619_holi(adreno_gpu)) {
 			gpu_write(gpu, 0x18, GPR0_GBIF_HALT_REQUEST);
 			spin_until((gpu_read(gpu, REG_A6XX_RBBM_VBIF_GX_RESET_STATUS) &
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 71faeb3fd466..e8449be606b1 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -253,6 +253,19 @@ static const struct adreno_info gpulist[] = {
 		.quirks = ADRENO_QUIRK_LMLOADKILL_DISABLE,
 		.init = a5xx_gpu_init,
 		.zapfw = "a540_zap.mdt",
+	}, {
+		.rev = ADRENO_REV(6, 1, 0, ANY_ID),
+		.revn = 610,
+		.name = "A610",
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+		},
+		.gmem = (SZ_128K + SZ_4K),
+		.inactive_period = 500,
+		.quirks = ADRENO_QUIRK_GMU_WRAPPER,
+		.init = a6xx_gpu_init,
+		.zapfw = "a610_zap.mdt",
+		.hwcg = a612_hwcg,
 	}, {
 		.rev = ADRENO_REV(6, 1, 8, ANY_ID),
 		.revn = 618,
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 16241368c2e4..2dd3dad3122b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -56,7 +56,8 @@ struct adreno_reglist {
 	u32 value;
 };
 
-extern const struct adreno_reglist a615_hwcg[], a630_hwcg[], a640_hwcg[], a650_hwcg[], a660_hwcg[];
+extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_hwcg[], a650_hwcg[];
+extern const struct adreno_reglist a660_hwcg[];
 
 struct adreno_info {
 	struct adreno_rev rev;
@@ -242,6 +243,11 @@ static inline int adreno_is_a540(struct adreno_gpu *gpu)
 	return gpu->revn == 540;
 }
 
+static inline int adreno_is_a610(struct adreno_gpu *gpu)
+{
+	return gpu->revn == 610;
+}
+
 static inline int adreno_is_a618(struct adreno_gpu *gpu)
 {
 	return gpu->revn == 618;
-- 
2.39.1

Re: [PATCH v2 08/14] drm/msm/a6xx: Add A610 support

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> A610 is one of (if not the) lowest-tier SKUs in the A6XX family. It
> features no GMU, as it's implemented solely on SoCs with SMD_RPM.
> What's more interesting is that it does not feature a VDDGX line
> either, being powered solely by VDDCX and has an unfortunate hardware
> quirk that makes its reset line broken - after a couple of assert/
> deassert cycles, it will hang for good and will not wake up again.
> 
> This GPU requires mesa changes for proper rendering, and lots of them
> at that. The command streams are quite far away from any other A6XX
> GPU and hence it needs special care. This patch was validated both
> by running an (incomplete) downstream mesa with some hacks (frames
> rendered correctly, though some instructions made the GPU hangcheck
> which is expected - garbage in, garbage out) and by replaying RD
> traces captured with the downstream KGSL driver - no crashes there,
> ever.
> 
> Add support for this GPU on the kernel side, which comes down to
> pretty simply adding A612 HWCG tables, altering a few values and
> adding a special case for handling the reset line.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Minor nit below.

> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 95 ++++++++++++++++++++--
>   drivers/gpu/drm/msm/adreno/adreno_device.c | 13 +++
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  8 +-
>   3 files changed, 106 insertions(+), 10 deletions(-)
> 

[skipped]

> @@ -1087,18 +1144,26 @@ static int hw_init(struct msm_gpu *gpu)
>   	gpu_write(gpu, REG_A6XX_UCHE_FILTER_CNTL, 0x804);
>   	gpu_write(gpu, REG_A6XX_UCHE_CACHE_WAYS, 0x4);
>   
> -	if (adreno_is_a640_family(adreno_gpu) ||
> -	    adreno_is_a650_family(adreno_gpu))
> +	if (adreno_is_a640_family(adreno_gpu) || adreno_is_a650_family(adreno_gpu)) {

Keep this on two lines please.

>   		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_2, 0x02000140);
> -	else
> +		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x8040362c);
> +	} else if (adreno_is_a610(adreno_gpu)) {
> +		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_2, 0x00800060);
> +		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x40201b16);
> +	} else {
>   		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_2, 0x010000c0);
> -	gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x8040362c);
> +		gpu_write(gpu, REG_A6XX_CP_ROQ_THRESHOLDS_1, 0x8040362c);
> +	}
>   

[skipped the rest]

-- 
With best wishes
Dmitry

[PATCH v2 09/14] drm/msm/a6xx: Fix some A619 tunables

[Konrad Dybcio]

Adreno 619 expects some tunables to be set differently. Make up for it.

Fixes: b7616b5c69e6 ("drm/msm/adreno: Add A619 support")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 1e259e9901ca..503c750216e6 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1174,6 +1174,8 @@ static int hw_init(struct msm_gpu *gpu)
 		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00200200);
 	else if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu))
 		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00300200);
+	else if (adreno_is_a619(adreno_gpu))
+		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00018000);
 	else if (adreno_is_a610(adreno_gpu))
 		gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00080000);
 	else
@@ -1191,7 +1193,9 @@ static int hw_init(struct msm_gpu *gpu)
 	a6xx_set_ubwc_config(gpu);
 
 	/* Enable fault detection */
-	if (adreno_is_a610(adreno_gpu))
+	if (adreno_is_a619(adreno_gpu))
+		gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30) | 0x3fffff);
+	else if (adreno_is_a610(adreno_gpu))
 		gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30) | 0x3ffff);
 	else
 		gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30) | 0x1fffff);
-- 
2.39.1

Re: [PATCH v2 09/14] drm/msm/a6xx: Fix some A619 tunables

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> Adreno 619 expects some tunables to be set differently. Make up for it.
> 
> Fixes: b7616b5c69e6 ("drm/msm/adreno: Add A619 support")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH v2 10/14] drm/msm/a6xx: Fix up A6XX protected registers

[Konrad Dybcio]

One of the protected ranges was too small (compared to the data we
have downstream). Fix it.

Fixes: 408434036958 ("drm/msm/a6xx: update/fix CP_PROTECT initialization")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 503c750216e6..d6b38bfdb3b4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -690,7 +690,7 @@ static const u32 a6xx_protect[] = {
 	A6XX_PROTECT_NORDWR(0x00800, 0x0082),
 	A6XX_PROTECT_NORDWR(0x008a0, 0x0008),
 	A6XX_PROTECT_NORDWR(0x008ab, 0x0024),
-	A6XX_PROTECT_RDONLY(0x008de, 0x00ae),
+	A6XX_PROTECT_RDONLY(0x008d0, 0x00bc),
 	A6XX_PROTECT_NORDWR(0x00900, 0x004d),
 	A6XX_PROTECT_NORDWR(0x0098d, 0x0272),
 	A6XX_PROTECT_NORDWR(0x00e00, 0x0001),
-- 
2.39.1

Re: [PATCH v2 10/14] drm/msm/a6xx: Fix up A6XX protected registers

[Rob Clark]

On Tue, Feb 14, 2023 at 9:32 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> One of the protected ranges was too small (compared to the data we
> have downstream). Fix it.
>
> Fixes: 408434036958 ("drm/msm/a6xx: update/fix CP_PROTECT initialization")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 503c750216e6..d6b38bfdb3b4 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -690,7 +690,7 @@ static const u32 a6xx_protect[] = {
>         A6XX_PROTECT_NORDWR(0x00800, 0x0082),
>         A6XX_PROTECT_NORDWR(0x008a0, 0x0008),
>         A6XX_PROTECT_NORDWR(0x008ab, 0x0024),
> -       A6XX_PROTECT_RDONLY(0x008de, 0x00ae),
> +       A6XX_PROTECT_RDONLY(0x008d0, 0x00bc),

Nak, this is intentional, we need userspace to be able to configure
the CP counters.  Otherwise this would break fdperf, perfetto, etc

(although maybe we should comment where we diverge from downstream)

BR,
-R

>         A6XX_PROTECT_NORDWR(0x00900, 0x004d),
>         A6XX_PROTECT_NORDWR(0x0098d, 0x0272),
>         A6XX_PROTECT_NORDWR(0x00e00, 0x0001),
> --
> 2.39.1
>

Re: [PATCH v2 10/14] drm/msm/a6xx: Fix up A6XX protected registers

[Dmitry Baryshkov]

On 14/02/2023 23:56, Rob Clark wrote:
> On Tue, Feb 14, 2023 at 9:32 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> One of the protected ranges was too small (compared to the data we
>> have downstream). Fix it.
>>
>> Fixes: 408434036958 ("drm/msm/a6xx: update/fix CP_PROTECT initialization")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index 503c750216e6..d6b38bfdb3b4 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -690,7 +690,7 @@ static const u32 a6xx_protect[] = {
>>          A6XX_PROTECT_NORDWR(0x00800, 0x0082),
>>          A6XX_PROTECT_NORDWR(0x008a0, 0x0008),
>>          A6XX_PROTECT_NORDWR(0x008ab, 0x0024),
>> -       A6XX_PROTECT_RDONLY(0x008de, 0x00ae),
>> +       A6XX_PROTECT_RDONLY(0x008d0, 0x00bc),
> 
> Nak, this is intentional, we need userspace to be able to configure
> the CP counters.  Otherwise this would break fdperf, perfetto, etc
> 
> (although maybe we should comment where we diverge from downstream)

Yes, please. Otherwise it is extremely hard to understand the reason for 
diversion between the vendor driver and our one.

> 
> BR,
> -R
> 
>>          A6XX_PROTECT_NORDWR(0x00900, 0x004d),
>>          A6XX_PROTECT_NORDWR(0x0098d, 0x0272),
>>          A6XX_PROTECT_NORDWR(0x00e00, 0x0001),
>> --
>> 2.39.1
>>

-- 
With best wishes
Dmitry

Re: [PATCH v2 10/14] drm/msm/a6xx: Fix up A6XX protected registers

[Konrad Dybcio]

On 15.02.2023 01:10, Dmitry Baryshkov wrote:
> On 14/02/2023 23:56, Rob Clark wrote:
>> On Tue, Feb 14, 2023 at 9:32 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>
>>> One of the protected ranges was too small (compared to the data we
>>> have downstream). Fix it.
>>>
>>> Fixes: 408434036958 ("drm/msm/a6xx: update/fix CP_PROTECT initialization")
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index 503c750216e6..d6b38bfdb3b4 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -690,7 +690,7 @@ static const u32 a6xx_protect[] = {
>>>          A6XX_PROTECT_NORDWR(0x00800, 0x0082),
>>>          A6XX_PROTECT_NORDWR(0x008a0, 0x0008),
>>>          A6XX_PROTECT_NORDWR(0x008ab, 0x0024),
>>> -       A6XX_PROTECT_RDONLY(0x008de, 0x00ae),
>>> +       A6XX_PROTECT_RDONLY(0x008d0, 0x00bc),
>>
>> Nak, this is intentional, we need userspace to be able to configure
>> the CP counters.  Otherwise this would break fdperf, perfetto, etc
>>
>> (although maybe we should comment where we diverge from downstream)
> 
> Yes, please. Otherwise it is extremely hard to understand the reason for diversion between the vendor driver and our one.
+1

I am content with dropping this patch from this series, so long
as you leave a clue for others to not scratch their heads on this!

Konrad
> 
>>
>> BR,
>> -R
>>
>>>          A6XX_PROTECT_NORDWR(0x00900, 0x004d),
>>>          A6XX_PROTECT_NORDWR(0x0098d, 0x0272),
>>>          A6XX_PROTECT_NORDWR(0x00e00, 0x0001),
>>> -- 
>>> 2.39.1
>>>
> 

Re: [PATCH v2 10/14] drm/msm/a6xx: Fix up A6XX protected registers

[Rob Clark]

On Tue, Feb 14, 2023 at 4:38 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 15.02.2023 01:10, Dmitry Baryshkov wrote:
> > On 14/02/2023 23:56, Rob Clark wrote:
> >> On Tue, Feb 14, 2023 at 9:32 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>>
> >>> One of the protected ranges was too small (compared to the data we
> >>> have downstream). Fix it.
> >>>
> >>> Fixes: 408434036958 ("drm/msm/a6xx: update/fix CP_PROTECT initialization")
> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>> ---
> >>>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>> index 503c750216e6..d6b38bfdb3b4 100644
> >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>> @@ -690,7 +690,7 @@ static const u32 a6xx_protect[] = {
> >>>          A6XX_PROTECT_NORDWR(0x00800, 0x0082),
> >>>          A6XX_PROTECT_NORDWR(0x008a0, 0x0008),
> >>>          A6XX_PROTECT_NORDWR(0x008ab, 0x0024),
> >>> -       A6XX_PROTECT_RDONLY(0x008de, 0x00ae),
> >>> +       A6XX_PROTECT_RDONLY(0x008d0, 0x00bc),
> >>
> >> Nak, this is intentional, we need userspace to be able to configure
> >> the CP counters.  Otherwise this would break fdperf, perfetto, etc
> >>
> >> (although maybe we should comment where we diverge from downstream)
> >
> > Yes, please. Otherwise it is extremely hard to understand the reason for diversion between the vendor driver and our one.
> +1
>
> I am content with dropping this patch from this series, so long
> as you leave a clue for others to not scratch their heads on this!

Yeah, I admit it is kinda a trap as-is.  And makes things less obvious
what to do when porting from downstream.  When I get a few minutes
I'll double check that there weren't any other exceptions (I don't
think they were but it has been a while) and add some comments.

BR,
-R

> Konrad
> >
> >>
> >> BR,
> >> -R
> >>
> >>>          A6XX_PROTECT_NORDWR(0x00900, 0x004d),
> >>>          A6XX_PROTECT_NORDWR(0x0098d, 0x0272),
> >>>          A6XX_PROTECT_NORDWR(0x00e00, 0x0001),
> >>> --
> >>> 2.39.1
> >>>
> >

[PATCH v2 11/14] drm/msm/a6xx: Enable optional icc voting from OPP tables

[Konrad Dybcio]

On GMU-equipped GPUs, the GMU requests appropriate bandwidth votes
for us. This is however not the case for the other GPUs. Add the
dev_pm_opp_of_find_icc_paths() call to let the OPP framework handle
bus voting as part of power level setting.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index d6b38bfdb3b4..b08ed127f8c4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2338,5 +2338,9 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 		msm_mmu_set_fault_handler(gpu->aspace->mmu, gpu,
 				a6xx_fault_handler);
 
+	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return gpu;
 }
-- 
2.39.1

Re: [PATCH v2 11/14] drm/msm/a6xx: Enable optional icc voting from OPP tables

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> On GMU-equipped GPUs, the GMU requests appropriate bandwidth votes
> for us. This is however not the case for the other GPUs. Add the
> dev_pm_opp_of_find_icc_paths() call to let the OPP framework handle
> bus voting as part of power level setting.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index d6b38bfdb3b4..b08ed127f8c4 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2338,5 +2338,9 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   		msm_mmu_set_fault_handler(gpu->aspace->mmu, gpu,
>   				a6xx_fault_handler);
>   
> +	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	return gpu;
>   }

-- 
With best wishes
Dmitry

[PATCH v2 12/14] drm/msm/a6xx: Use "else if" in GPU speedbin rev matching

[Konrad Dybcio]

The GPU can only be one at a time. Turn a series of ifs into if +
elseifs to save some CPU cycles.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index b08ed127f8c4..ffe0fd431a76 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2153,16 +2153,16 @@ static u32 fuse_to_supp_hw(struct device *dev, struct adreno_rev rev, u32 fuse)
 	if (adreno_cmp_rev(ADRENO_REV(6, 1, 8, ANY_ID), rev))
 		val = a618_get_speed_bin(fuse);
 
-	if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, ANY_ID), rev))
+	else if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, ANY_ID), rev))
 		val = a619_get_speed_bin(fuse);
 
-	if (adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), rev))
+	else if (adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), rev))
 		val = adreno_7c3_get_speed_bin(fuse);
 
-	if (adreno_cmp_rev(ADRENO_REV(6, 4, 0, ANY_ID), rev))
+	else if (adreno_cmp_rev(ADRENO_REV(6, 4, 0, ANY_ID), rev))
 		val = a640_get_speed_bin(fuse);
 
-	if (adreno_cmp_rev(ADRENO_REV(6, 5, 0, ANY_ID), rev))
+	else if (adreno_cmp_rev(ADRENO_REV(6, 5, 0, ANY_ID), rev))
 		val = a650_get_speed_bin(fuse);
 
 	if (val == UINT_MAX) {
-- 
2.39.1

Re: [PATCH v2 12/14] drm/msm/a6xx: Use "else if" in GPU speedbin rev matching

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> The GPU can only be one at a time. Turn a series of ifs into if +
> elseifs to save some CPU cycles.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

-- 
With best wishes
Dmitry

[PATCH v2 13/14] drm/msm/a6xx: Add A619_holi speedbin support

[Konrad Dybcio]

A619_holi is implemented on at least two SoCs: SM4350 (holi) and SM6375
(blair). This is what seems to be a first occurrence of this happening,
but it's easy to overcome by guarding the SoC-specific fuse values with
of_machine_is_compatible(). Do just that to enable frequency limiting
on these SoCs.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index ffe0fd431a76..94b4d93619ed 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2094,6 +2094,34 @@ static u32 a618_get_speed_bin(u32 fuse)
 	return UINT_MAX;
 }
 
+static u32 a619_holi_get_speed_bin(u32 fuse)
+{
+	/*
+	 * There are (at least) two SoCs implementing A619_holi: SM4350 (holi)
+	 * and SM6375 (blair). Limit the fuse matching to the corresponding
+	 * SoC to prevent bogus frequency setting (as improbable as it may be,
+	 * given unexpected fuse values are.. unexpected! But still possible.)
+	 */
+
+	if (fuse == 0)
+		return 0;
+
+	if (of_machine_is_compatible("qcom,sm4350")) {
+		if (fuse == 138)
+			return 1;
+		else if (fuse == 92)
+			return 2;
+	} else if (of_machine_is_compatible("qcom,sm6375")) {
+		if (fuse == 190)
+			return 1;
+		else if (fuse == 177)
+			return 2;
+	} else
+		pr_warn("Unknown SoC implementing A619_holi!\n");
+
+	return UINT_MAX;
+}
+
 static u32 a619_get_speed_bin(u32 fuse)
 {
 	if (fuse == 0)
@@ -2153,6 +2181,9 @@ static u32 fuse_to_supp_hw(struct device *dev, struct adreno_rev rev, u32 fuse)
 	if (adreno_cmp_rev(ADRENO_REV(6, 1, 8, ANY_ID), rev))
 		val = a618_get_speed_bin(fuse);
 
+	else if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, 1), rev))
+		val = a619_holi_get_speed_bin(fuse);
+
 	else if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, ANY_ID), rev))
 		val = a619_get_speed_bin(fuse);
 
-- 
2.39.1

Re: [PATCH v2 13/14] drm/msm/a6xx: Add A619_holi speedbin support

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> A619_holi is implemented on at least two SoCs: SM4350 (holi) and SM6375
> (blair). This is what seems to be a first occurrence of this happening,
> but it's easy to overcome by guarding the SoC-specific fuse values with
> of_machine_is_compatible(). Do just that to enable frequency limiting
> on these SoCs.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index ffe0fd431a76..94b4d93619ed 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2094,6 +2094,34 @@ static u32 a618_get_speed_bin(u32 fuse)
>   	return UINT_MAX;
>   }
>   
> +static u32 a619_holi_get_speed_bin(u32 fuse)
> +{
> +	/*
> +	 * There are (at least) two SoCs implementing A619_holi: SM4350 (holi)
> +	 * and SM6375 (blair). Limit the fuse matching to the corresponding
> +	 * SoC to prevent bogus frequency setting (as improbable as it may be,
> +	 * given unexpected fuse values are.. unexpected! But still possible.)
> +	 */
> +
> +	if (fuse == 0)
> +		return 0;
> +
> +	if (of_machine_is_compatible("qcom,sm4350")) {
> +		if (fuse == 138)
> +			return 1;
> +		else if (fuse == 92)
> +			return 2;
> +	} else if (of_machine_is_compatible("qcom,sm6375")) {
> +		if (fuse == 190)
> +			return 1;
> +		else if (fuse == 177)
> +			return 2;

Ugh.

> +	} else
> +		pr_warn("Unknown SoC implementing A619_holi!\n");
> +
> +	return UINT_MAX;
> +}
> +
>   static u32 a619_get_speed_bin(u32 fuse)
>   {
>   	if (fuse == 0)
> @@ -2153,6 +2181,9 @@ static u32 fuse_to_supp_hw(struct device *dev, struct adreno_rev rev, u32 fuse)
>   	if (adreno_cmp_rev(ADRENO_REV(6, 1, 8, ANY_ID), rev))
>   		val = a618_get_speed_bin(fuse);
>   
> +	else if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, 1), rev))

I really think it begs to have && !of_find_property(dev->of_node, 
"qcom,gmu") here.

> +		val = a619_holi_get_speed_bin(fuse);
> +
>   	else if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, ANY_ID), rev))
>   		val = a619_get_speed_bin(fuse);
>   

-- 
With best wishes
Dmitry

[PATCH v2 14/14] drm/msm/a6xx: Add A610 speedbin support

[Konrad Dybcio]

A610 is implemented on at least three SoCs: SM6115 (bengal), SM6125
(trinket) and SM6225 (khaje). Trinket does not support speed binning
(only a single SKU exists) and we don't yet support khaje upstream.
Hence, add a fuse mapping table for bengal to allow for per-chip
frequency limiting.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 94b4d93619ed..f2679f9cc137 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2082,6 +2082,30 @@ static bool a6xx_progress(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 	return progress;
 }
 
+static u32 a610_get_speed_bin(u32 fuse)
+{
+	/*
+	 * There are (at least) three SoCs implementing A610: SM6125 (trinket),
+	 * SM6115 (bengal) and SM6225 (khaje). Trinket does not have speedbinning,
+	 * as only a single SKU exists and we don't support khaje upstream yet.
+	 * Hence, this matching table is only valid for bengal and can be easily
+	 * expanded if need be.
+	 */
+
+	if (fuse == 0)
+		return 0;
+	else if (fuse == 206)
+		return 1;
+	else if (fuse == 200)
+		return 2;
+	else if (fuse == 157)
+		return 3;
+	else if (fuse == 127)
+		return 4;
+
+	return UINT_MAX;
+}
+
 static u32 a618_get_speed_bin(u32 fuse)
 {
 	if (fuse == 0)
@@ -2178,6 +2202,9 @@ static u32 fuse_to_supp_hw(struct device *dev, struct adreno_rev rev, u32 fuse)
 {
 	u32 val = UINT_MAX;
 
+	if (adreno_cmp_rev(ADRENO_REV(6, 1, 0, ANY_ID), rev))
+		val = a610_get_speed_bin(fuse);
+
 	if (adreno_cmp_rev(ADRENO_REV(6, 1, 8, ANY_ID), rev))
 		val = a618_get_speed_bin(fuse);
 
-- 
2.39.1

Re: [PATCH v2 14/14] drm/msm/a6xx: Add A610 speedbin support

[Dmitry Baryshkov]

On 14/02/2023 19:31, Konrad Dybcio wrote:
> A610 is implemented on at least three SoCs: SM6115 (bengal), SM6125
> (trinket) and SM6225 (khaje). Trinket does not support speed binning
> (only a single SKU exists) and we don't yet support khaje upstream.
> Hence, add a fuse mapping table for bengal to allow for per-chip
> frequency limiting.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry