Re: [PATCH v2 2/2] drm/rockchip: dw_hdmi: add rk3568 support

[Andy Yan <andy.yan@xxxxxxxxxxxxxx>]

Hi Alex:

On 7/13/21 7:40 PM, Alex Bee wrote:
> Hi Benjamin,
>
> Am 07.07.21 um 14:03 schrieb Benjamin Gaignard:
> > Add a new dw_hdmi_plat_data struct and new compatible for rk3568.
> > This version of the HDMI hardware block need two clocks to provide
> > phy reference clock: hclk_vio and hclk.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> > ---
> > version 2:
> > - Add the clocks needed for the phy.
>
> If got Alega's comment correct, it wasn't about the hclks.
> It looks like for this variant, there is another reference clock 
> required (for the phy) like vpll is already (looks like downstream 
> uses HPLL ( = "HDMI-PLL" ?) for that - which also has to switch the 
> frequency according the the drm mode rate - the two clocks you added 
> here are get just enabled (and disabled) here.


Yes, it's HPLL, and the frequency of HPLL and drm mode rate(vop dclk) 
should keep 1:1.

> Alega, Andy: Is it really required to enable hclk_vio and hclk(_vop) 
> in the hdmi driver? Are they required to be enabled for the other 
> output variants (i.e. mipi, dsi, rgb ....) as well and shouldn't 
> better be enabled in the (not-yet existing) vop2 driver?


hclk_vop should be enabled, other wise you can't access hdmi registers. 
This is only required for HDMI(mipi dis, edp, rgb don't need it)

> Overall: I'm not sure of the benefit of adding this hdmi variant for a 
> SoC where the display driver isn't implemented upstream yet. The 
> "VOP2" IP seems widely new and should probably be ported first. (even 
> if the HDMI part seems a low hanging fruit according to the vendor 
> sources)


Yes, VOP2 IP is widely totaly new and complicated, I have a plan to do 
the upstream. But I am in a rush now, so please give me a lite time😁.

> Best,
> Alex
>
> >   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++++
> >   1 file changed, 68 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c 
> > b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > index 830bdd5e9b7ce..dc0e255e45745 100644
> > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > @@ -50,6 +50,10 @@
> >   #define RK3399_GRF_SOC_CON20        0x6250
> >   #define RK3399_HDMI_LCDC_SEL        BIT(6)
> >   +#define RK3568_GRF_VO_CON1        0x0364
> > +#define RK3568_HDMI_SDAIN_MSK        BIT(15)
> > +#define RK3568_HDMI_SCLIN_MSK        BIT(14)
> > +
> >   #define HIWORD_UPDATE(val, mask)    (val | (mask) << 16)
> >     /**
> > @@ -71,6 +75,8 @@ struct rockchip_hdmi {
> >       const struct rockchip_hdmi_chip_data *chip_data;
> >       struct clk *vpll_clk;
> >       struct clk *grf_clk;
> > +    struct clk *hclk_vio;
> > +    struct clk *hclk_vop;
> >       struct dw_hdmi *hdmi;
> >       struct phy *phy;
> >   };
> > @@ -216,6 +222,26 @@ static int rockchip_hdmi_parse_dt(struct 
> > rockchip_hdmi *hdmi)
> >           return PTR_ERR(hdmi->grf_clk);
> >       }
> >   +    hdmi->hclk_vio = devm_clk_get(hdmi->dev, "hclk_vio");
> > +    if (PTR_ERR(hdmi->hclk_vio) == -ENOENT) {
> > +        hdmi->hclk_vio = NULL;
> > +    } else if (PTR_ERR(hdmi->hclk_vio) == -EPROBE_DEFER) {
> > +        return -EPROBE_DEFER;
> > +    } else if (IS_ERR(hdmi->hclk_vio)) {
> > +        dev_err(hdmi->dev, "failed to get hclk_vio clock\n");
> > +        return PTR_ERR(hdmi->hclk_vio);
> > +    }
> > +
> > +    hdmi->hclk_vop = devm_clk_get(hdmi->dev, "hclk");
> > +    if (PTR_ERR(hdmi->hclk_vop) == -ENOENT) {
> > +        hdmi->hclk_vop = NULL;
> > +    } else if (PTR_ERR(hdmi->hclk_vop) == -EPROBE_DEFER) {
> > +        return -EPROBE_DEFER;
> > +    } else if (IS_ERR(hdmi->hclk_vop)) {
> > +        dev_err(hdmi->dev, "failed to get hclk_vop clock\n");
> > +        return PTR_ERR(hdmi->hclk_vop);
> > +    }
> > +
> >       return 0;
> >   }
> >   @@ -467,6 +493,19 @@ static const struct dw_hdmi_plat_data 
> > rk3399_hdmi_drv_data = {
> >       .use_drm_infoframe = true,
> >   };
> >   +static struct rockchip_hdmi_chip_data rk3568_chip_data = {
> > +    .lcdsel_grf_reg = -1,
> > +};
> > +
> > +static const struct dw_hdmi_plat_data rk3568_hdmi_drv_data = {
> > +    .mode_valid = dw_hdmi_rockchip_mode_valid,
> > +    .mpll_cfg   = rockchip_mpll_cfg,
> > +    .cur_ctr    = rockchip_cur_ctr,
> > +    .phy_config = rockchip_phy_config,
> > +    .phy_data = &rk3568_chip_data,
> > +    .use_drm_infoframe = true,
> > +};
> > +
> >   static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
> >       { .compatible = "rockchip,rk3228-dw-hdmi",
> >         .data = &rk3228_hdmi_drv_data
> > @@ -480,6 +519,9 @@ static const struct of_device_id 
> > dw_hdmi_rockchip_dt_ids[] = {
> >       { .compatible = "rockchip,rk3399-dw-hdmi",
> >         .data = &rk3399_hdmi_drv_data
> >       },
> > +    { .compatible = "rockchip,rk3568-dw-hdmi",
> > +      .data = &rk3568_hdmi_drv_data
> > +    },
> >       {},
> >   };
> >   MODULE_DEVICE_TABLE(of, dw_hdmi_rockchip_dt_ids);
> > @@ -536,6 +578,28 @@ static int dw_hdmi_rockchip_bind(struct device 
> > *dev, struct device *master,
> >           return ret;
> >       }
> >   +    ret = clk_prepare_enable(hdmi->hclk_vio);
> > +    if (ret) {
> > +        dev_err(hdmi->dev, "Failed to enable HDMI hclk_vio: %d\n",
> > +            ret);
> > +        return ret;
> > +    }
> > +
> > +    ret = clk_prepare_enable(hdmi->hclk_vop);
> > +    if (ret) {
> > +        dev_err(hdmi->dev, "Failed to enable HDMI hclk_vop: %d\n",
> > +            ret);
> > +        return ret;
> > +    }
> > +
> > +    if (hdmi->chip_data == &rk3568_chip_data) {
> > +        regmap_write(hdmi->regmap, RK3568_GRF_VO_CON1,
> > +                 HIWORD_UPDATE(RK3568_HDMI_SDAIN_MSK |
> > +                       RK3568_HDMI_SCLIN_MSK,
> > +                       RK3568_HDMI_SDAIN_MSK |
> > +                       RK3568_HDMI_SCLIN_MSK));
> > +    }
> > +
> >       hdmi->phy = devm_phy_optional_get(dev, "hdmi");
> >       if (IS_ERR(hdmi->phy)) {
> >           ret = PTR_ERR(hdmi->phy);
> > @@ -559,6 +623,8 @@ static int dw_hdmi_rockchip_bind(struct device 
> > *dev, struct device *master,
> >           ret = PTR_ERR(hdmi->hdmi);
> >           drm_encoder_cleanup(encoder);
> >           clk_disable_unprepare(hdmi->vpll_clk);
> > +        clk_disable_unprepare(hdmi->hclk_vio);
> > +        clk_disable_unprepare(hdmi->hclk_vop);
> >       }
> >         return ret;
> > @@ -571,6 +637,8 @@ static void dw_hdmi_rockchip_unbind(struct device 
> > *dev, struct device *master,
> >         dw_hdmi_unbind(hdmi->hdmi);
> >       clk_disable_unprepare(hdmi->vpll_clk);
> > +    clk_disable_unprepare(hdmi->hclk_vio);
> > +    clk_disable_unprepare(hdmi->hclk_vop);
> >   }
> >     static const struct component_ops dw_hdmi_rockchip_ops = {