Enable CEC on rk3399

The documentation already mentions "cec" optional clock, but currently the driver doesn't enable it.

Hi Pierre-Hugues,

Quoted 2 lines · Pierre-Hugues Husson

The documentation already mentions "cec" optional clock, but currently the driver doesn't enable it.

The cec clock is enabled at dw_hdmi_cec_enable().

As far as I understand, dw_hdmi_cec_enable only gates
the CEC clock inside the Synopsis IP,
but the SOC still has to provide a specific CEC clock to it. To enable such an external CEC clock, the binding documentation [1] mentions a "cec" optional clock, and I'm not seeing any code in dw-hdmi to enable it.

The documentation already mentions "cec" optional clock, but currently the driver doesn't enable it.

Signed-off-by Pierre-Hugues Husson <[email protected]>

---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +17 -0

index bf14214fa464..5007cdf43131 100644
@@ -138,6 +138,7 @@ struct dw_hdmi {
 	struct device *dev;
 	struct clk *isfr_clk;
 	struct clk *iahb_clk;
+	struct clk *cec_clk;
 	struct dw_hdmi_i2c *i2c;
 
 	struct hdmi_data_info hdmi_data;

@@ -2382,6 +2383,18 @@ __dw_hdmi_probe(struct platform_device *pdev,
 		goto err_isfr;
 	}
 
+	hdmi->cec_clk = devm_clk_get(hdmi->dev, "cec");
+	if (IS_ERR(hdmi->cec_clk)) {
+		hdmi->cec_clk = NULL;
+	} else {
+		ret = clk_prepare_enable(hdmi->cec_clk);
+		if (ret) {
+			dev_err(hdmi->dev, "Cannot enable HDMI cec clock: %d\n",
+					ret);
+			goto err_res;
+		}
+	}
+
 	/* Product and revision IDs */
 	hdmi->version = (hdmi_readb(hdmi, HDMI_DESIGN_ID) << 8)
 		      | (hdmi_readb(hdmi, HDMI_REVISION_ID) << 0);

@@ -2518,6 +2531,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
 		cec_notifier_put(hdmi->cec_notifier);
 
 	clk_disable_unprepare(hdmi->iahb_clk);
+	if (hdmi->cec_clk)
+		clk_disable_unprepare(hdmi->cec_clk);
 err_isfr:
 	clk_disable_unprepare(hdmi->isfr_clk);
 err_res:

@@ -2541,6 +2556,8 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 
 	clk_disable_unprepare(hdmi->iahb_clk);
 	clk_disable_unprepare(hdmi->isfr_clk);
+	if (hdmi->cec_clk)
+		clk_disable_unprepare(hdmi->cec_clk);
 
 	if (hdmi->i2c)
 		i2c_del_adapter(&hdmi->i2c->adap);

Quoted 4 lines · Pierre-Hugues Husson

The documentation already mentions "cec" optional clock, but currently the driver doesn't enable it.

Signed-off-by Pierre-Hugues Husson <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Thanks!

Hans

Quoted 54 lines

---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +17 -0

index bf14214fa464..5007cdf43131 100644
@@ -138,6 +138,7 @@ struct dw_hdmi {
 	struct device *dev;
 	struct clk *isfr_clk;
 	struct clk *iahb_clk;
+	struct clk *cec_clk;
 	struct dw_hdmi_i2c *i2c;
 
 	struct hdmi_data_info hdmi_data;

@@ -2382,6 +2383,18 @@ __dw_hdmi_probe(struct platform_device *pdev,
 		goto err_isfr;
 	}
 
+	hdmi->cec_clk = devm_clk_get(hdmi->dev, "cec");
+	if (IS_ERR(hdmi->cec_clk)) {
+		hdmi->cec_clk = NULL;
+	} else {
+		ret = clk_prepare_enable(hdmi->cec_clk);
+		if (ret) {
+			dev_err(hdmi->dev, "Cannot enable HDMI cec clock: %d\n",
+					ret);
+			goto err_res;
+		}
+	}
+
 	/* Product and revision IDs */
 	hdmi->version = (hdmi_readb(hdmi, HDMI_DESIGN_ID) << 8)
 		      | (hdmi_readb(hdmi, HDMI_REVISION_ID) << 0);

@@ -2518,6 +2531,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
 		cec_notifier_put(hdmi->cec_notifier);
 
 	clk_disable_unprepare(hdmi->iahb_clk);
+	if (hdmi->cec_clk)
+		clk_disable_unprepare(hdmi->cec_clk);
 err_isfr:
 	clk_disable_unprepare(hdmi->isfr_clk);
 err_res:

@@ -2541,6 +2556,8 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 
 	clk_disable_unprepare(hdmi->iahb_clk);
 	clk_disable_unprepare(hdmi->isfr_clk);
+	if (hdmi->cec_clk)
+		clk_disable_unprepare(hdmi->cec_clk);
 
 	if (hdmi->i2c)
 		i2c_del_adapter(&hdmi->i2c->adap);

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c __dw_hdmi_probe(struct platform_device *pdev, · line 2388

Quoted 7 lines

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +3 -0

@@ -2382,6 +2383,18 @@ __dw_hdmi_probe(struct platform_device *pdev,
              goto err_isfr;
      }
 
+     hdmi->cec_clk = devm_clk_get(hdmi->dev, "cec");
+     if (IS_ERR(hdmi->cec_clk)) {
+             hdmi->cec_clk = NULL;

What if devm_clk_get() returns EPROBE_DEFER? Does that really mean the clock does not exist?

Signed-off-by Pierre-Hugues Husson <[email protected]>

Signed-off-by Pierre-Hugues Husson <[email protected]>

Enable CEC on firefly-rk3399.
Tested on a TV with cec-ctl --playback; cec-ctl -S

Pierre-Hugues Husson (3):
drm: bridge: synopsys/dw-hdmi: Enable cec clock
arm64: dts: rockchip: add the cec clk for dw-mipi-hdmi on rk3399
arm64: dts: rockchip: enable cec pin for rk3399 firefly

 arch/arm64/boot/dts/rockchip/rk3399-firefly.dts |  2 ++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi        |  8 ++++++--
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c       | 16 ++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

Nice! I had a similar dw-hdmi.c patch pending but got around to posting it.

I'll brush off my old rk3288 patches and see if I can get CEC enabled for my firefly-reload. I was close to getting it work, but I guess missed the "enable cec pin" change.

Hi Hans,

Quoted 5 lines

Nice! I had a similar dw-hdmi.c patch pending but got around to posting it.

I'll brush off my old rk3288 patches and see if I can get CEC enabled for my firefly-reload. I was close to getting it work, but I guess missed the "enable cec pin" change.

Please note that on rk3288, there are two CEC pins, and you must write in RK3288_GRF_SOC_CON8 which pin you're using.
On the firefly-reload, the pin used is GPIO7C0, while the default pin configuration is GPIO7C7.

So far only the hdmi cec supports using one of two different pins as source, so add the route switching for it.

Signed-off-by Heiko Stuebner <[email protected]>

---
If I didn't mess up any numbering, the pinctrl change should look like the following patch.

Hope that helps
Heiko

 drivers/pinctrl/pinctrl-rockchip.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/pinctrl/pinctrl-rockchip.c +20 -0

index b5cb7858ffdc..8dd49e2e144b 100644
@@ -884,6 +884,24 @@ static struct rockchip_mux_route_data rk3228_mux_route_data[] = {
 	},
 };
 
+static struct rockchip_mux_route_data rk3288_mux_route_data[] = {
+	{
+		/* edphdmi_cecinoutt1 */
+		.bank_num = 7,
+		.pin = 16,
+		.func = 2,
+		.route_offset = 0x264,
+		.route_val = BIT(16 + 12) | BIT(12),
+	}, {
+		/* edphdmi_cecinout */
+		.bank_num = 7,
+		.pin = 23,
+		.func = 4,
+		.route_offset = 0x264,
+		.route_val = BIT(16 + 12),
+	},
+};
+
 static struct rockchip_mux_route_data rk3328_mux_route_data[] = {
 	{
 		/* uart2dbg_rxm0 */

@@ -3391,6 +3409,8 @@ static struct rockchip_pin_ctrl rk3288_pin_ctrl = {
 		.type			= RK3288,
 		.grf_mux_offset		= 0x0,
 		.pmu_mux_offset		= 0x84,
+		.iomux_routes		= rk3288_mux_route_data,
+		.niomux_routes		= ARRAY_SIZE(rk3288_mux_route_data),
 		.pull_calc_reg		= rk3288_calc_pull_reg_and_bit,
 		.drv_calc_reg		= rk3288_calc_drv_reg_and_bit,
 };

Quoted 4 lines

So far only the hdmi cec supports using one of two different pins as source, so add the route switching for it.

Signed-off-by Heiko Stuebner <[email protected]>

Just tested this on my firefly reload and it works great!

Tested-by Hans Verkuil <[email protected]>

I'll post some dts patches later today to fully bring up the first HDMI output on the Firefly Reload.

Will you process this patch further to get it mainlined?

Hi Hans,

Am Freitag, 20. Oktober 2017, 09:28:58 CEST schrieb Hans Verkuil:

Quoted 4 lines · Heiko Stuebner

Quoted 3 lines

So far only the hdmi cec supports using one of two different pins as source, so add the route switching for it.

Quoted 5 lines

Quoted 1 line

Signed-off-by Heiko Stuebner <[email protected]>

Just tested this on my firefly reload and it works great!

Tested-by Hans Verkuil <[email protected]>

oh cool. I really only wrote this based on the soc manual, so it actually surprises me, that it works on the first try :-)

Quoted 5 lines

I'll post some dts patches later today to fully bring up the first HDMI output on the Firefly Reload.

Will you process this patch further to get it mainlined?

Yep, I'll do that.

Quoted 11 lines

Hi Hans,

Am Freitag, 20. Oktober 2017, 09:28:58 CEST schrieb Hans Verkuil:

Quoted 8 lines · Heiko Stuebner

Quoted 4 lines

So far only the hdmi cec supports using one of two different pins as source, so add the route switching for it.

Signed-off-by Heiko Stuebner <[email protected]>

Just tested this on my firefly reload and it works great!

Quoted 4 lines

Quoted 1 line

Tested-by Hans Verkuil <[email protected]>

oh cool. I really only wrote this based on the soc manual, so it actually surprises me, that it works on the first try :-)

One note though: I've only tested it on my Firefly Reload. I don't have a regular Firefly, so I can't be certain it works there. Just covering my ass here :-)

Am Samstag, 14. Oktober 2017, 15:14:40 CEST schrieb Pierre-Hugues Husson:

Quoted 11 lines

Hi Hans,

Quoted 5 lines

Nice! I had a similar dw-hdmi.c patch pending but got around to posting it.

I'll brush off my old rk3288 patches and see if I can get CEC enabled for my firefly-reload. I was close to getting it work, but I guess missed the "enable cec pin" change.

Please note that on rk3288, there are two CEC pins, and you must write in RK3288_GRF_SOC_CON8 which pin you're using.
On the firefly-reload, the pin used is GPIO7C0, while the default pin configuration is GPIO7C7.

And as an additional note, later socs have even more of these pin-routing settings and we currently have infrastructure in the pinctrl driver to do this automatically depending on the pinctrl settings.

So most likely this can also be added there for the rk3288.

Heiko

Quoted 11 lines

Am Samstag, 14. Oktober 2017, 15:14:40 CEST schrieb Pierre-Hugues Husson:

Quoted 9 lines

Quoted 5 lines

Nice! I had a similar dw-hdmi.c patch pending but got around to posting it.

I'll brush off my old rk3288 patches and see if I can get CEC enabled for my firefly-reload. I was close to getting it work, but I guess missed the "enable cec pin" change.

Please note that on rk3288, there are two CEC pins, and you must write in RK3288_GRF_SOC_CON8 which pin you're using.
On the firefly-reload, the pin used is GPIO7C0, while the default pin configuration is GPIO7C7.

Quoted 9 lines

And as an additional note, later socs have even more of these pin-routing settings and we currently have infrastructure in the pinctrl driver to do this automatically depending on the pinctrl settings.

So most likely this can also be added there for the rk3288.

Heiko

How does 'GPIO7C0' translate to a 'rockchip,pins' line?

I have this in rk3288-firefly-reload.dts:

       hdmi {
                hdmi_cec: hdmi-cec {
                        rockchip,pins = <7 16 RK_FUNC_2 &pcfg_pull_none>;
                };
        };

I think this is correct. You can find my patches for the firefly reload here:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=firefly2

I don't have access to my firefly reload until Friday, so I can't test this until then.

I'd be very grateful though if you can check the last three patches in that branch. It's just a test branch, so the subject/changelog of those patches are incomplete.

The very last patch adds CEC support for the firefly/firefly beta. I don't have that hardware, so I won't be able to test that.