linux-kernel - Re: [PATCH] drm/msm/dsi: support CPHY mode for 7nm pll/phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 5 Mar 2021 18:06:43 -0500
From:   Jonathan Marek <jonathan@...ek.ca>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        freedreno@...ts.freedesktop.org
Cc:     Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Kalyan Thota <kalyan_t@...eaurora.org>,
        Jordan Crouse <jcrouse@...eaurora.org>,
        "Kristian H. Kristensen" <hoegsberg@...gle.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Rikard Falkeborn <rikard.falkeborn@...il.com>,
        Emil Velikov <emil.velikov@...labora.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Konrad Dybcio <konradybcio@...il.com>,
        Dave Airlie <airlied@...hat.com>,
        "open list:DRM DRIVER FOR MSM ADRENO GPU" 
        <linux-arm-msm@...r.kernel.org>,
        "open list:DRM DRIVER FOR MSM ADRENO GPU" 
        <dri-devel@...ts.freedesktop.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm/dsi: support CPHY mode for 7nm pll/phy

On 3/5/21 5:45 PM, Dmitry Baryshkov wrote:
> On 15/02/2021 19:27, Jonathan Marek wrote:
>> Add the required changes to support 7nm pll/phy in CPHY mode.
>>
>> This adds a "qcom,dsi-phy-cphy-mode" property for the PHY node to enable
>> the CPHY mode.
>>
>> Signed-off-by: Jonathan Marek <jonathan@...ek.ca>
> 
> Other that few comments bellow:
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> 
>> ---
>>   .../devicetree/bindings/display/msm/dsi.txt   |  1 +
>>   drivers/gpu/drm/msm/dsi/dsi.c                 | 12 +--
>>   drivers/gpu/drm/msm/dsi/dsi.h                 |  6 +-
>>   drivers/gpu/drm/msm/dsi/dsi.xml.h             |  2 +
>>   drivers/gpu/drm/msm/dsi/dsi_host.c            | 34 +++++--
>>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c         | 49 +++++++++-
>>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.h         |  3 +
>>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c     | 89 ++++++++++++++-----
>>   drivers/gpu/drm/msm/dsi/pll/dsi_pll.c         |  4 +-
>>   drivers/gpu/drm/msm/dsi/pll/dsi_pll.h         |  5 +-
>>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_7nm.c     | 71 +++++++++------
>>   11 files changed, 210 insertions(+), 66 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
>> b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> index b9a64d3ff184..7ffc86a9816b 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
>> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> @@ -124,6 +124,7 @@ Required properties:
>>   Optional properties:
>>   - qcom,dsi-phy-regulator-ldo-mode: Boolean value indicating if the 
>> LDO mode PHY
>>     regulator is wanted.
>> +- qcom,dsi-phy-cphy-mode: Boolean value indicating if CPHY mode is 
>> wanted.
>>   - qcom,mdss-mdp-transfer-time-us:    Specifies the dsi transfer time 
>> for command mode
>>                       panels in microseconds. Driver uses this number 
>> to adjust
>>                       the clock rate according to the expected 
>> transfer time.
> 
> This should go in a separate patch, shan't it?
> 
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>> b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 627048851d99..68d8547f7264 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -13,7 +13,7 @@ struct drm_encoder *msm_dsi_get_encoder(struct 
>> msm_dsi *msm_dsi)
>>       return msm_dsi->encoder;
>>   }
>> -static int dsi_get_phy(struct msm_dsi *msm_dsi)
>> +static int dsi_get_phy(struct msm_dsi *msm_dsi, bool *cphy_mode)
> 
> I see no need to pass the 'cphy_mode' through the bool pointer and back 
> to msm_dsi_host_init. What about just putting it into struct msm_dsi?
> 

Because it doesn't need to be stored in msm_dsi (need it in msm_dsi_host 
which doesn't have access to msm_dsi). But I suppose it doesn't hurt to 
also have it in msm_dsi and make things a bit cleaner.

>>   {
>>       struct platform_device *pdev = msm_dsi->pdev;
>>       struct platform_device *phy_pdev;
>> @@ -29,6 +29,7 @@ static int dsi_get_phy(struct msm_dsi *msm_dsi)
>>       if (phy_pdev)
>>           msm_dsi->phy = platform_get_drvdata(phy_pdev);
>> +    *cphy_mode = of_property_read_bool(phy_node, 
>> "qcom,dsi-phy-cphy-mode");
>>       of_node_put(phy_node);
>>       if (!phy_pdev || !msm_dsi->phy) {
>> @@ -65,6 +66,7 @@ static void dsi_destroy(struct msm_dsi *msm_dsi)
>>   static struct msm_dsi *dsi_init(struct platform_device *pdev)
>>   {
>>       struct msm_dsi *msm_dsi;
>> +    bool cphy_mode;
>>       int ret;
>>       if (!pdev)
>> @@ -79,13 +81,13 @@ static struct msm_dsi *dsi_init(struct 
>> platform_device *pdev)
>>       msm_dsi->pdev = pdev;
>>       platform_set_drvdata(pdev, msm_dsi);
>> -    /* Init dsi host */
>> -    ret = msm_dsi_host_init(msm_dsi);
>> +    /* GET dsi PHY */
>> +    ret = dsi_get_phy(msm_dsi, &cphy_mode);
>>       if (ret)
>>           goto destroy_dsi;
>> -    /* GET dsi PHY */
>> -    ret = dsi_get_phy(msm_dsi);
>> +    /* Init dsi host */
>> +    ret = msm_dsi_host_init(msm_dsi, cphy_mode);
>>       if (ret)
>>           goto destroy_dsi;
> 
>