Re: [PATCH v3 2/4] drm/msm/dpu: support binding to the mdp5 devices

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> Existing MDP5 devices have slightly different bindings. The main
> register region is called `mdp_phys' instead of `mdp'. Also vbif
> register regions are a part of the parent, MDSS device. Add support for
> handling this binding differences.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 98 ++++++++++++++++++++++++++-------
>   drivers/gpu/drm/msm/msm_drv.h           |  3 +
>   drivers/gpu/drm/msm/msm_io_utils.c      | 13 +++++
>   3 files changed, 93 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 723cc1d82143..aa9e0ad33ebb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1197,6 +1197,78 @@ static int dpu_kms_init(struct drm_device *ddev)
>   	return 0;
>   }
>   
> +static int dpu_kms_mmap_mdp5(struct dpu_kms *dpu_kms)
> +{
> +	struct platform_device *pdev = dpu_kms->pdev;
> +	struct platform_device *mdss_dev;
> +	int ret;
> +
> +	if (dpu_kms->pdev->dev.bus != &platform_bus_type)
> +		return -EINVAL;
> +

!dev_is_platform() perhaps?

But I would like to understand this check a bit more. Can you pls 
explain for which case this check was added?

> +	mdss_dev = to_platform_device(dpu_kms->pdev->dev.parent);
> +
> +	dpu_kms->mmio = msm_ioremap(pdev, "mdp_phys");
> +	if (IS_ERR(dpu_kms->mmio)) {
> +		ret = PTR_ERR(dpu_kms->mmio);
> +		DPU_ERROR("mdp register memory map failed: %d\n", ret);
> +		dpu_kms->mmio = NULL;
> +		return ret;
> +	}
> +	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> +
> +	dpu_kms->vbif[VBIF_RT] = msm_ioremap_mdss(mdss_dev,
> +						  dpu_kms->pdev,
> +						  "vbif_phys");
> +	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> +		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> +		DPU_ERROR("vbif register memory map failed: %d\n", ret);
> +		dpu_kms->vbif[VBIF_RT] = NULL;
> +		return ret;
> +	}
> +
> +	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_mdss(mdss_dev,
> +						   dpu_kms->pdev,
> +						   "vbif_nrt_phys");

Do you think a "quiet" version would be better?


> +	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> +		dpu_kms->vbif[VBIF_NRT] = NULL;
> +		DPU_DEBUG("VBIF NRT is not defined");
> +	}
> +
> +	return 0;
> +}
> +
> +static int dpu_kms_mmap_dpu(struct dpu_kms *dpu_kms)
> +{
> +	struct platform_device *pdev = dpu_kms->pdev;
> +	int ret;
> +
> +	dpu_kms->mmio = msm_ioremap(pdev, "mdp");
> +	if (IS_ERR(dpu_kms->mmio)) {
> +		ret = PTR_ERR(dpu_kms->mmio);
> +		DPU_ERROR("mdp register memory map failed: %d\n", ret);
> +		dpu_kms->mmio = NULL;
> +		return ret;
> +	}
> +	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> +
> +	dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
> +	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> +		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> +		DPU_ERROR("vbif register memory map failed: %d\n", ret);
> +		dpu_kms->vbif[VBIF_RT] = NULL;
> +		return ret;
> +	}
> +
> +	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
> +	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> +		dpu_kms->vbif[VBIF_NRT] = NULL;
> +		DPU_DEBUG("VBIF NRT is not defined");
> +	}
> +
> +	return 0;
> +}
> +
>   static int dpu_dev_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1230,28 +1302,12 @@ static int dpu_dev_probe(struct platform_device *pdev)
>   
>   	dpu_kms->base.irq = irq;
>   
> -	dpu_kms->mmio = msm_ioremap(pdev, "mdp");
> -	if (IS_ERR(dpu_kms->mmio)) {
> -		ret = PTR_ERR(dpu_kms->mmio);
> -		DPU_ERROR("mdp register memory map failed: %d\n", ret);
> -		dpu_kms->mmio = NULL;
> -		return ret;
> -	}
> -	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> -
> -	dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
> -	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> -		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> -		DPU_ERROR("vbif register memory map failed: %d\n", ret);
> -		dpu_kms->vbif[VBIF_RT] = NULL;
> +	if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, "qcom,mdp5"))
> +		ret = dpu_kms_mmap_mdp5(dpu_kms);
> +	else
> +		ret = dpu_kms_mmap_dpu(dpu_kms);
> +	if (ret)
>   		return ret;
> -	}
> -
> -	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
> -	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> -		dpu_kms->vbif[VBIF_NRT] = NULL;
> -		DPU_DEBUG("VBIF NRT is not defined");
> -	}
>   
>   	ret = dpu_kms_parse_data_bus_icc_path(dpu_kms);
>   	if (ret)
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 16a7cbc0b7dd..01e783130054 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -476,6 +476,9 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name);
>   void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
>   		phys_addr_t *size);
>   void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> +			       struct platform_device *dev,
> +			       const char *name);
>   
>   struct icc_path *msm_icc_get(struct device *dev, const char *name);
>   
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> index 59d2788c4510..afedd61c3e28 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -50,6 +50,19 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>   	return clk;
>   }
>   
> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> +			       struct platform_device *pdev,
> +			       const char *name)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource_byname(mdss_pdev, IORESOURCE_MEM, name);
> +	if (!res)
> +		return ERR_PTR(-EINVAL);
> +
> +	return devm_ioremap_resource(&pdev->dev, res);
> +}
> +
>   static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
>   				  bool quiet, phys_addr_t *psize)
>   {
>