Re: [PATCH v1 5/6] drm/rockchip: dw_hdmi: introduce the VPLL clock setting

[Heiko Stübner]

Hi Yakir,

Am Montag, 11. Juli 2016, 19:05:49 schrieb Yakir Yang:
> For RK3399 HDMI, there is an external clock need for HDMI PHY,
> and it should keep the same clock rate with VOP DCLK.
> 
> VPLL have supported the clock for HDMI PHY, but there is no
> clock divider bewteen VPLL and HDMI PHY. So we need to set the
> VPLL rate manually in HDMI driver.

I don't think reserving the vpll for the hdmi at all times is the right way to 
go. 

While I do agree that reserving the VPLL (and NPLL on the rk3288) for graphics 
use looks like the right way, I think the core Rockchip drm driver should be 
responsible for managing it and also for deciding which output encoder gets to 
use it.

While true that on the Chromebook device-types the edp is static and hdmi 
needs the broad range of dynamic frequencies, this is not necessarily the case 
for all future device types and/or socs.

In the other thread we discussed adding that as rockchip,dclk-pll = <&...>; to 
the base display-subsystem node, Doug didn't manage to find time to respond yet 
though - and is on vacation right now.

I still believe that would be the best solution :-) .


That property could list 1 or even 2 plls, depending on the soc or board 
layout - maybe someone frees up the cpll in some special layout or something.
If none are listed, then the drm driver would need to cope with its available 
clocks.

Implementation-wise you could even still keep your shortcut until we encounter 
a device with different , let the drm use the pll for hdmi only until someone 
comes up with a better concept, but still the binding should be correct and 
versatile from the start.


Heiko

> ---
>  .../bindings/display/rockchip/dw_hdmi-rockchip.txt |  3 ++-
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 25
> +++++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> index 4e573d2..4e23ca4 100644
> ---
> a/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> +++
> b/Documentation/devicetree/bindings/display/rockchip/dw_hdmi-rockchip.txt
> @@ -17,7 +17,8 @@ Required properties:
> 
>  Optional properties
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> -- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
> +- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec",
> +		       phandle to the VPLL clock, name should be "vpll".
> 
>  Example:
>  hdmi: hdmi@ff980000 {
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 329099b..701bb73 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -7,10 +7,12 @@
>   * (at your option) any later version.
>   */
> 
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +
>  #include <drm/drm_of.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -33,6 +35,7 @@ struct rockchip_hdmi {
>  	struct regmap *regmap;
>  	struct drm_encoder encoder;
>  	enum dw_hdmi_devtype dev_type;
> +	struct clk *vpll_clk;
>  };
> 
>  #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
> @@ -145,6 +148,7 @@ static const struct dw_hdmi_phy_config
> rockchip_phy_config[] = { static int rockchip_hdmi_parse_dt(struct
> rockchip_hdmi *hdmi)
>  {
>  	struct device_node *np = hdmi->dev->of_node;
> +	int ret;
> 
>  	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>  	if (IS_ERR(hdmi->regmap)) {
> @@ -152,6 +156,22 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi
> *hdmi) return PTR_ERR(hdmi->regmap);
>  	}
> 
> +	hdmi->vpll_clk = devm_clk_get(hdmi->dev, "vpll");
> +	if (PTR_ERR(hdmi->vpll_clk) == -ENOENT) {
> +		hdmi->vpll_clk = NULL;
> +	} else if (PTR_ERR(hdmi->vpll_clk) == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;
> +	} else if (IS_ERR(hdmi->vpll_clk)) {
> +		dev_err(hdmi->dev, "failed to get grf clock\n");
> +		return PTR_ERR(hdmi->vpll_clk);
> +	}
> +
> +	ret = clk_prepare_enable(hdmi->vpll_clk);
> +	if (ret) {
> +		dev_err(hdmi->dev, "Failed to enable HDMI vpll: %d\n", ret);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
> 
> @@ -194,6 +214,9 @@ static void dw_hdmi_rockchip_encoder_mode_set(struct
> drm_encoder *encoder, struct drm_display_mode *mode,
>  					      struct drm_display_mode *adj_mode)
>  {
> +	struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
> +
> +	clk_set_rate(hdmi->vpll_clk, adj_mode->clock * 1000);
>  }
> 
>  static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)