diff mbox series

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

Message ID 20210707120323.401785-3-benjamin.gaignard@collabora.com (mailing list archive)
State New
Headers show
Series Add support of HDMI for rk3568 | expand

Commit Message

Benjamin Gaignard July 7, 2021, 12:03 p.m. UTC
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@collabora.com>
---
version 2:
- Add the clocks needed for the phy.

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++++
 1 file changed, 68 insertions(+)

Comments

Alex Bee July 13, 2021, 11:40 a.m. UTC | #1
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@collabora.com>
> ---
> 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.

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?

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)

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 = {
>
Andy Yan July 14, 2021, 1:26 a.m. UTC | #2
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@collabora.com>
>> ---
>> 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
diff mbox series

Patch

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 = {