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

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Wed, 7 Feb 2024 12:05:11 -0800

On 2/7/2024 11:56 AM, Dmitry Baryshkov wrote:
On Wed, 7 Feb 2024 at 20:48, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



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@xxxxxxxxxx>
---
   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?

looks good


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

It is necessary to be sure that we can perform to_platform_device() on
the next line.


Got it but on the next line we are using to_platform_device on dpu's 
parent device. So shouldnt the check also be dpu_kms->pdev->dev.parent.bus?


+     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?

Yep, why not.



+     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)
   {










