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

[Jonathan Marek]

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@xxxxxxxx>
>
> Other that few comments bellow:
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> > ---
> >   .../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;