diff mbox

[v2,4/7] drm/rockchip: dw_hdmi: add inno hdmi phy ops

Message ID 1506735886-151761-1-git-send-email-algea.cao@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

crj Sept. 30, 2017, 1:44 a.m. UTC 
Because some RK chips use inno hdmi phy, such as RK3328, we add
inno hdmi phy ops.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 110 +++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 3 deletions(-)

Comments

Heiko Stübner Dec. 9, 2017, 5:09 p.m. UTC | #1 
Hi Algea,

Am Samstag, 30. September 2017, 09:44:46 CET schrieb Algea Cao:
> Because some RK chips use inno hdmi phy, such as RK3328, we add
> inno hdmi phy ops.
> 
> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 110 +++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 09c77f9..7658b2f 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -12,7 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> -
> +#include <linux/phy/phy.h>
>  #include <drm/drm_of.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -26,6 +26,18 @@
>  #define RK3288_HDMI_LCDC_SEL		BIT(4)
>  #define RK3399_GRF_SOC_CON20		0x6250
>  #define RK3399_HDMI_LCDC_SEL		BIT(6)
> +#define RK3328_GRF_SOC_CON2		0x0408
> +#define RK3328_DDC_MASK_EN		((3 << 10) | (3 << (10 + 16)))
> +#define RK3328_GRF_SOC_CON3		0x040c
> +#define RK3328_IO_CTRL_BY_HDMI		(0xf0000000 | BIT(13) | BIT(12))
> +#define RK3328_GRF_SOC_CON4		0x0410
> +#define RK3328_IO_3V_DOMAIN		(7 << (9 + 16))
> +#define RK3328_IO_5V_DOMAIN		((7 << 9) | (3 << (9 + 16)))

This is slightly confusing. (9+16) is obviously the write-enable-mask, so
shouldn't the write-enable-mask not at least cover the changed bits?


> +static enum drm_connector_status
> +inno_dw_hdmi_phy_read_hpd(struct dw_hdmi *dw_hdmi, void *data)
> +{
> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> +	enum drm_connector_status status;
> +
> +	status = dw_hdmi_phy_read_hpd(dw_hdmi, data);
> +
> +	if (hdmi->dev_type == RK3228_HDMI)
> +		return status;

There is no need to encode the functionality for both variants
(and possibly later ones) into one function.

As Neil said in his reply to patch2, we don't really want to carry
that dev_type property, so why don't you just define this as
rk3328_inno_phy_read_hpd and then use it like 

static const struct dw_hdmi_phy_ops rk3228_dw_hdmi_phy_ops = {
	.init		= inno_dw_hdmi_phy_init,
	.disable	= inno_dw_hdmi_phy_disable,
	.read_hpd	= dw_hdmi_phy_read_hpd,
};

static const struct dw_hdmi_phy_ops rk3328_dw_hdmi_phy_ops = {
	.init		= inno_dw_hdmi_phy_init,
	.disable	= inno_dw_hdmi_phy_disable,
	.read_hpd	= rk3328_inno_phy_read_hpd,
};

> +	if (status == connector_status_connected)
> +		regmap_write(hdmi->regmap,
> +			     RK3328_GRF_SOC_CON4,
> +			     RK3328_IO_5V_DOMAIN);
> +	else
> +		regmap_write(hdmi->regmap,
> +			     RK3328_GRF_SOC_CON4,
> +			     RK3328_IO_3V_DOMAIN);

That should definitely get a comment explaining what this is doing and
why :-) . Especially as it seems related to the plug/unplug events.

According to the TRM, the hdmiphy IP block has its own interrupt. Can
you tell me if it is firing on hotplug events? Because I'm wondering if
these GRF settings wouldn't be better live in the hdmiphy driver
[if it gets an irq on plug/unplug], which in turn would make the phy
handling in dw_hdmi a lot easier.

> +	return status;
> +}
> +

[...]

> @@ -361,7 +453,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  		return -ENOMEM;
>  
>  	match = of_match_node(dw_hdmi_rockchip_dt_ids, pdev->dev.of_node);
> -	plat_data = match->data;
> +	plat_data = (struct dw_hdmi_plat_data *)match->data;
>  	hdmi->dev = &pdev->dev;
>  	hdmi->chip_data = plat_data->phy_data;
>  	hdmi->dev_type = plat_data->dev_type;
> @@ -383,6 +475,18 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  		return ret;
>  	}
>  
> +	if (hdmi->dev_type == RK3328_HDMI || hdmi->dev_type == RK3228_HDMI) {
> +		hdmi->phy = devm_phy_get(dev, "hdmi_phy");
> +		if (IS_ERR(hdmi->phy)) {
> +			ret = PTR_ERR(hdmi->phy);
> +			dev_err(dev, "failed to get phy: %d\n", ret);
> +			return ret;
> +		}
> +		plat_data->phy_data = hdmi;
> +		ret = inno_dw_hdmi_init(hdmi);
> +		if (ret < 0)
> +			return ret;
> +	}

You could also just declare it optional in the binding, try to get the phy
via devm_phy_optional_get and if it's NULL just assume that it's the
regular internal phy as with previous socs. That way you can drop the
dev-type-check.

Aka, if a phy is defined we are pretty sure we want to use that one :-) .


Heiko
diff mbox

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 09c77f9..7658b2f 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -12,7 +12,7 @@ 
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
-
+#include <linux/phy/phy.h>
 #include <drm/drm_of.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
@@ -26,6 +26,18 @@ 
 #define RK3288_HDMI_LCDC_SEL		BIT(4)
 #define RK3399_GRF_SOC_CON20		0x6250
 #define RK3399_HDMI_LCDC_SEL		BIT(6)
+#define RK3328_GRF_SOC_CON2		0x0408
+#define RK3328_DDC_MASK_EN		((3 << 10) | (3 << (10 + 16)))
+#define RK3328_GRF_SOC_CON3		0x040c
+#define RK3328_IO_CTRL_BY_HDMI		(0xf0000000 | BIT(13) | BIT(12))
+#define RK3328_GRF_SOC_CON4		0x0410
+#define RK3328_IO_3V_DOMAIN		(7 << (9 + 16))
+#define RK3328_IO_5V_DOMAIN		((7 << 9) | (3 << (9 + 16)))
+#define RK3328_HPD_3V			(BIT(8 + 16) | BIT(13 + 16))
+#define RK3228_GRF_SOC_CON2		0x0408
+#define RK3228_DDC_MASK_EN		((3 << 13) | (3 << (13 + 16)))
+#define RK3228_GRF_SOC_CON6		0x0418
+#define RK3228_IO_3V_DOMAIN		((7 << 4) | (7 << (4 + 16)))
 
 #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
 
@@ -49,10 +61,82 @@  struct rockchip_hdmi {
 	enum dw_hdmi_devtype dev_type;
 	struct clk *vpll_clk;
 	struct clk *grf_clk;
+	struct phy *phy;
 };
 
 #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
 
+static int
+inno_dw_hdmi_phy_init(struct dw_hdmi *dw_hdmi, void *data,
+		      struct drm_display_mode *mode)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+
+	return phy_power_on(hdmi->phy);
+}
+
+static void inno_dw_hdmi_phy_disable(struct dw_hdmi *dw_hdmi, void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+
+	phy_power_off(hdmi->phy);
+}
+
+static enum drm_connector_status
+inno_dw_hdmi_phy_read_hpd(struct dw_hdmi *dw_hdmi, void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+	enum drm_connector_status status;
+
+	status = dw_hdmi_phy_read_hpd(dw_hdmi, data);
+
+	if (hdmi->dev_type == RK3228_HDMI)
+		return status;
+
+	if (status == connector_status_connected)
+		regmap_write(hdmi->regmap,
+			     RK3328_GRF_SOC_CON4,
+			     RK3328_IO_5V_DOMAIN);
+	else
+		regmap_write(hdmi->regmap,
+			     RK3328_GRF_SOC_CON4,
+			     RK3328_IO_3V_DOMAIN);
+	return status;
+}
+
+static int inno_dw_hdmi_init(struct rockchip_hdmi *hdmi)
+{
+	int ret;
+
+	ret = clk_prepare_enable(hdmi->grf_clk);
+	if (ret < 0) {
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
+		return -EPROBE_DEFER;
+	}
+	if (hdmi->dev_type == RK3328_HDMI) {
+		/* Map HPD pin to 3V io */
+		regmap_write(hdmi->regmap,
+			     RK3328_GRF_SOC_CON4,
+			     RK3328_IO_3V_DOMAIN |
+			     RK3328_HPD_3V);
+		/* Map ddc pin to 5V io */
+		regmap_write(hdmi->regmap,
+			     RK3328_GRF_SOC_CON3,
+			     RK3328_IO_CTRL_BY_HDMI);
+		regmap_write(hdmi->regmap,
+			     RK3328_GRF_SOC_CON2,
+			     RK3328_DDC_MASK_EN |
+			     BIT(18));
+	} else {
+		regmap_write(hdmi->regmap, RK3228_GRF_SOC_CON2,
+			     RK3228_DDC_MASK_EN);
+		regmap_write(hdmi->regmap, RK3228_GRF_SOC_CON6,
+			     RK3228_IO_3V_DOMAIN);
+	}
+	clk_disable_unprepare(hdmi->grf_clk);
+	return 0;
+}
+
 static const struct dw_hdmi_mpll_config rockchip_mpll_cfg[] = {
 	{
 		27000000, {
@@ -294,6 +378,12 @@  static const struct drm_encoder_helper_funcs dw_hdmi_rockchip_encoder_helper_fun
 	.atomic_check = dw_hdmi_rockchip_encoder_atomic_check,
 };
 
+static const struct dw_hdmi_phy_ops inno_dw_hdmi_phy_ops = {
+	.init		= inno_dw_hdmi_phy_init,
+	.disable	= inno_dw_hdmi_phy_disable,
+	.read_hpd	= inno_dw_hdmi_phy_read_hpd,
+};
+
 static struct rockchip_hdmi_chip_data rk3288_chip_data = {
 	.lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
 	.lcdsel_big = HIWORD_UPDATE(0, RK3288_HDMI_LCDC_SEL),
@@ -317,6 +407,8 @@  static struct rockchip_hdmi_chip_data rk3399_chip_data = {
 
 static const struct dw_hdmi_plat_data rk3328_hdmi_drv_data = {
 	.mode_valid = dw_hdmi_rockchip_mode_valid,
+	.phy_ops    = &inno_dw_hdmi_phy_ops,
+	.phy_name   = "inno_dw_hdmi_phy2",
 	.dev_type   = RK3328_HDMI,
 };
 static const struct dw_hdmi_plat_data rk3399_hdmi_drv_data = {
@@ -346,7 +438,7 @@  static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 				 void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	const struct dw_hdmi_plat_data *plat_data;
+	struct dw_hdmi_plat_data *plat_data;
 	const struct of_device_id *match;
 	struct drm_device *drm = data;
 	struct drm_encoder *encoder;
@@ -361,7 +453,7 @@  static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 		return -ENOMEM;
 
 	match = of_match_node(dw_hdmi_rockchip_dt_ids, pdev->dev.of_node);
-	plat_data = match->data;
+	plat_data = (struct dw_hdmi_plat_data *)match->data;
 	hdmi->dev = &pdev->dev;
 	hdmi->chip_data = plat_data->phy_data;
 	hdmi->dev_type = plat_data->dev_type;
@@ -383,6 +475,18 @@  static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
+	if (hdmi->dev_type == RK3328_HDMI || hdmi->dev_type == RK3228_HDMI) {
+		hdmi->phy = devm_phy_get(dev, "hdmi_phy");
+		if (IS_ERR(hdmi->phy)) {
+			ret = PTR_ERR(hdmi->phy);
+			dev_err(dev, "failed to get phy: %d\n", ret);
+			return ret;
+		}
+		plat_data->phy_data = hdmi;
+		ret = inno_dw_hdmi_init(hdmi);
+		if (ret < 0)
+			return ret;
+	}
 	drm_encoder_helper_add(encoder, &dw_hdmi_rockchip_encoder_helper_funcs);
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);