Re: [PATCH] drm/msm: adreno: Make speed-bin support generic

[Jordan Crouse <jcrouse@codeaurora.org>]

On Thu, Nov 12, 2020 at 09:19:04PM +0530, Akhil P Oommen wrote:
> So far a530v2 gpu has support for detecting its supported opps
> based on a fuse value called speed-bin. This patch makes this
> support generic across gpu families. This is in preparation to
> extend speed-bin support to a6x family.
> 
> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
> ---
> This patch is rebased on top of msm-next-staging branch in rob's tree.
> 
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c      | 34 --------------
>  drivers/gpu/drm/msm/adreno/adreno_device.c |  4 ++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 71 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  5 +++
>  4 files changed, 80 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 8fa5c91..7d42321 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1531,38 +1531,6 @@ static const struct adreno_gpu_funcs funcs = {
>  	.get_timestamp = a5xx_get_timestamp,
>  };
>  
> -static void check_speed_bin(struct device *dev)
> -{
> -	struct nvmem_cell *cell;
> -	u32 val;
> -
> -	/*
> -	 * If the OPP table specifies a opp-supported-hw property then we have
> -	 * to set something with dev_pm_opp_set_supported_hw() or the table
> -	 * doesn't get populated so pick an arbitrary value that should
> -	 * ensure the default frequencies are selected but not conflict with any
> -	 * actual bins
> -	 */
> -	val = 0x80;
> -
> -	cell = nvmem_cell_get(dev, "speed_bin");
> -
> -	if (!IS_ERR(cell)) {
> -		void *buf = nvmem_cell_read(cell, NULL);
> -
> -		if (!IS_ERR(buf)) {
> -			u8 bin = *((u8 *) buf);
> -
> -			val = (1 << bin);
> -			kfree(buf);
> -		}
> -
> -		nvmem_cell_put(cell);
> -	}
> -
> -	dev_pm_opp_set_supported_hw(dev, &val, 1);
> -}
> -
>  struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>  {
>  	struct msm_drm_private *priv = dev->dev_private;
> @@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>  
>  	a5xx_gpu->lm_leakage = 0x4E001A;
>  
> -	check_speed_bin(&pdev->dev);
> -
>  	ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4);
>  	if (ret) {
>  		a5xx_destroy(&(a5xx_gpu->base.base));
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 87c8b03..e0ff16c 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -18,6 +18,8 @@ bool snapshot_debugbus = false;
>  MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)");
>  module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600);
>  
> +const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7};
> +
>  static const struct adreno_info gpulist[] = {
>  	{
>  		.rev   = ADRENO_REV(2, 0, 0, 0),
> @@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = {
>  			ADRENO_QUIRK_FAULT_DETECT_MASK,
>  		.init = a5xx_gpu_init,
>  		.zapfw = "a530_zap.mdt",
> +		.speedbins = a530v2_speedbins,
> +		.speedbins_count = ARRAY_SIZE(a530v2_speedbins),
>  	}, {
>  		.rev = ADRENO_REV(5, 4, 0, 2),
>  		.revn = 540,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index f21561d..cdd0c11 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -14,6 +14,7 @@
>  #include <linux/pm_opp.h>
>  #include <linux/slab.h>
>  #include <linux/soc/qcom/mdt_loader.h>
> +#include <linux/nvmem-consumer.h>
>  #include <soc/qcom/ocmem.h>
>  #include "adreno_gpu.h"
>  #include "msm_gem.h"
> @@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
>  			   adreno_ocmem->hdl);
>  }
>  
> +static int adreno_set_supported_hw(struct device *dev,
> +		struct adreno_gpu *adreno_gpu)
> +{
> +	u8 speedbins_count = adreno_gpu->info->speedbins_count;
> +	const u32 *speedbins = adreno_gpu->info->speedbins;

We don't need to make this generic and put it in the table. Just call the
function from the target specific code and pass the speedbin array and size from
there.

> +	struct nvmem_cell *cell;
> +	u32 bin, i;
> +	u32 val = 0;
> +	void *buf, *opp_table;
> +
> +	cell = nvmem_cell_get(dev, "speed_bin");
> +	/*
> +	 * -ENOENT means that the platform doesn't support speedbin which is
> +	 * fine
> +	 */
> +	if (PTR_ERR(cell) == -ENOENT)
> +		return 0;
> +	else if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	/* A speedbin table is must if the platform supports speedbin */
> +	if (!speedbins) {
> +		DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
> +		return -ENOENT;
> +	}
> +
> +	buf = nvmem_cell_read(cell, NULL);
> +	if (IS_ERR(buf)) {
> +		nvmem_cell_put(cell);
> +		return PTR_ERR(buf);
> +	}
> +
> +	bin = *((u32 *) buf);
> +
> +	for (i = 0; i < speedbins_count; i++) {
> +		if (bin == speedbins[i]) {
> +			val = (1 << i);
> +			break;
> +		}
> +	}
> +
> +	kfree(buf);
> +	nvmem_cell_put(cell);
> +
> +	if (!val) {
> +		DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
> +		return -ENOENT;
> +	}
> +
> +	opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1);
> +	if (IS_ERR(opp_table))
> +		return PTR_ERR(opp_table);
> +
> +	adreno_gpu->opp_table = opp_table;
> +	return 0;
> +}
> +
> +static void adreno_put_supported_hw(struct opp_table *opp_table)
> +{
> +	if (opp_table)
> +		dev_pm_opp_put_supported_hw(opp_table);
> +}
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *adreno_gpu,
>  		const struct adreno_gpu_funcs *funcs, int nr_rings)
> @@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  	struct adreno_platform_config *config = dev->platform_data;
>  	struct msm_gpu_config adreno_gpu_config  = { 0 };
>  	struct msm_gpu *gpu = &adreno_gpu->base;
> +	int ret;
>  
>  	adreno_gpu->funcs = funcs;
>  	adreno_gpu->info = adreno_info(config->rev);
> @@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  
>  	adreno_gpu_config.nr_rings = nr_rings;
>  
> +	ret = adreno_set_supported_hw(dev, adreno_gpu);
> +	if (ret)
> +		return ret;

This bit should be in the target specific code
> +
>  	adreno_get_pwrlevels(dev, gpu);
>  
>  	pm_runtime_set_autosuspend_delay(dev,
> @@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
>  
>  	icc_put(gpu->icc_path);
>  	icc_put(gpu->ocmem_icc_path);
> +
> +	adreno_put_supported_hw(adreno_gpu->opp_table);

And this bit too, though it would be easier to just call the put function
directly without having a intermediate function.  Also the OPP function should
be NULL aware but thats a different story.

Jordan
>  }
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index c3775f7..a756ad7 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -55,6 +55,7 @@ struct adreno_reglist {
>  };
>  
>  extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[];
> +extern const u32 a618_speedbins[];
>  
>  struct adreno_info {
>  	struct adreno_rev rev;
> @@ -67,6 +68,8 @@ struct adreno_info {
>  	const char *zapfw;
>  	u32 inactive_period;
>  	const struct adreno_reglist *hwcg;
> +	const u32 *speedbins;
> +	const u8 speedbins_count;
>  };
>  
>  const struct adreno_info *adreno_info(struct adreno_rev rev);
> @@ -112,6 +115,8 @@ struct adreno_gpu {
>  	 * code (a3xx_gpu.c) and stored in this common location.
>  	 */
>  	const unsigned int *reg_offsets;
> +
> +	struct opp_table *opp_table;
>  };
>  #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
>  
> -- 
> 2.7.4
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project