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

[Jonathan Marek <jonathan@marek.ca>]

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@marek.ca>
> 
> Other that few comments bellow:
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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;
> 
>