Received-SPF: pass (google.com: domain of linux-kernel+bounces-57000-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <092a230a-05cb-48ae-daa0-607b15b30177@quicinc.com>
Date: Wed, 7 Feb 2024 12:05:11 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v3 2/4] drm/msm/dpu: support binding to the mdp5 devices
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
        Marijn
 Suijten <marijn.suijten@somainline.org>,
        David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        <linux-arm-msm@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <freedreno@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
References: <20240106-fd-migrate-mdp5-v3-0-3d2750378063@linaro.org>
 <20240106-fd-migrate-mdp5-v3-2-3d2750378063@linaro.org>
 <b10d6097-b13d-4b47-7127-9200e414b7be@quicinc.com>
 <CAA8EJppV_PYMyJBEogONrDVmZ91XmMkr=J0tsioO20GaAW8GoQ@mail.gmail.com>
From: Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAA8EJppV_PYMyJBEogONrDVmZ91XmMkr=J0tsioO20GaAW8GoQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/7/2024 11:56 AM, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 20:48, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@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?
> 
> 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)
>>>    {
>>>
> 
> 
>