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

[Yakir Yang <ykk@rock-chips.com>]

Heiko,

On 07/12/2016 10:27 PM, Heiko Stübner wrote:
> 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.

Great idea. Let a separate drm-pll driver to maintain all connector's 
clock requirement.

> 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.

Someone should start to prepare the drm core pll driver, it's great idea.

BR,
- Yakir

>
> 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)
>
>
>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel