[PATCH 0/4] drm/vc4: add HDMI CEC support

[PATCH 0/4] drm/vc4: add HDMI CEC support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch series adds support for HDMI CEC to the vc4 drm driver.
This series is based on the mainline kernel as of yesterday since
both the vc4 and cec patches for the 4.13 merge window are now merged
in that kernel.

Note: the first cec patch is independent of the vc4 patches and will be
merged via the media subsystem for 4.14. Without that patch everything
will still work, but it will attempt to retry messages twice as many
times as it should.

This has been tested with the Raspberry Pi 2B and 3B. I don't have older
rpi's, so I can't test those.

Many thanks to Eric Anholt for his help with this driver!

There is one open issue: when booting the rpi without an HDMI cable 
connected, then CEC won't work. But neither apparently does HDMI since
reconnecting it will not bring back any display.

If you boot with the HDMI cable connected, then all is well and 
disconnecting and reconnecting the cable will do the right thing.

I don't understand what is going on here, but it does not appear to
be CEC related and the same problem occurs without this patch series.

You also need to update your config.txt with this line to prevent the
firmware from eating the CEC interrupts:

mask_gpu_interrupt1=0x100

Eric, I've experimented with setting hdmi_ignore_cec=1 but that simply
doesn't work. Instead that disables CEC completely. With this
mask_gpu_interrupt1 setting everything works perfectly. This also
prevents the firmware from sending the initial Active Source CEC
message so the CPU has full control over the CEC bus, as it should.

My main concern is that this is rather magical, but it is not
something I have any control over.

Regards,

	Hans

Eric Anholt (1):
  drm/vc4: Add register defines for CEC.

Hans Verkuil (3):
  cec: be smarter about detecting the number of attempts made
  drm/vc4: prepare for CEC support
  drm/vc4: add HDMI CEC support

 drivers/gpu/drm/vc4/Kconfig    |   8 ++
 drivers/gpu/drm/vc4/vc4_hdmi.c | 278 +++++++++++++++++++++++++++++++++++------
 drivers/gpu/drm/vc4/vc4_regs.h | 113 +++++++++++++++++
 drivers/media/cec/cec-adap.c   |   9 +-
 4 files changed, 371 insertions(+), 37 deletions(-)

-- 
2.11.0

[PATCH 1/4] cec: be smarter about detecting the number of attempts made

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Some hardware does more than one attempt. So when it calls
cec_transmit_done when an error occurred it will e.g. use an error count
of 2 instead of 1.

The framework always assumed a single attempt, but now it is smarter
and will sum the counters to detect how many attempts were made.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/cec/cec-adap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index bf45977b2823..e9284dbdc880 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -472,9 +472,14 @@ void cec_transmit_done(struct cec_adapter *adap, u8 status, u8 arb_lost_cnt,
 {
 	struct cec_data *data;
 	struct cec_msg *msg;
+	unsigned int attempts_made = arb_lost_cnt + nack_cnt +
+				     low_drive_cnt + error_cnt;
 	u64 ts = ktime_get_ns();
 
 	dprintk(2, "%s: status %02x\n", __func__, status);
+	if (attempts_made < 1)
+		attempts_made = 1;
+
 	mutex_lock(&adap->lock);
 	data = adap->transmitting;
 	if (!data) {
@@ -507,10 +512,10 @@ void cec_transmit_done(struct cec_adapter *adap, u8 status, u8 arb_lost_cnt,
 	 * the hardware didn't signal that it retried itself (by setting
 	 * CEC_TX_STATUS_MAX_RETRIES), then we will retry ourselves.
 	 */
-	if (data->attempts > 1 &&
+	if (data->attempts > attempts_made &&
 	    !(status & (CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_OK))) {
 		/* Retry this message */
-		data->attempts--;
+		data->attempts -= attempts_made;
 		if (msg->timeout)
 			dprintk(2, "retransmit: %*ph (attempts: %d, wait for 0x%02x)\n",
 				msg->len, msg->msg, data->attempts, msg->reply);
-- 
2.11.0

[PATCH 2/4] drm/vc4: prepare for CEC support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

In order to support CEC the hsm clock needs to be enabled in
vc4_hdmi_bind(), not in vc4_hdmi_encoder_enable(). Otherwise you wouldn't
be able to support CEC when there is no hotplug detect signal, which is
required by some monitors that turn off the HPD when in standby, but keep
the CEC bus alive so they can be woken up.

The HDMI core also has to be enabled in vc4_hdmi_bind() for the same
reason.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 59 +++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ed63d4e85762..e0104f96011e 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -463,11 +463,6 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder)
 	HD_WRITE(VC4_HD_VID_CTL,
 		 HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
 
-	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
-	udelay(1);
-	HD_WRITE(VC4_HD_M_CTL, 0);
-
-	clk_disable_unprepare(hdmi->hsm_clock);
 	clk_disable_unprepare(hdmi->pixel_clock);
 
 	ret = pm_runtime_put(&hdmi->pdev->dev);
@@ -509,16 +504,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
 		return;
 	}
 
-	/* This is the rate that is set by the firmware.  The number
-	 * needs to be a bit higher than the pixel clock rate
-	 * (generally 148.5Mhz).
-	 */
-	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
-	if (ret) {
-		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
-		return;
-	}
-
 	ret = clk_set_rate(hdmi->pixel_clock,
 			   mode->clock * 1000 *
 			   ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
@@ -533,20 +518,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
 		return;
 	}
 
-	ret = clk_prepare_enable(hdmi->hsm_clock);
-	if (ret) {
-		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
-			  ret);
-		clk_disable_unprepare(hdmi->pixel_clock);
-		return;
-	}
-
-	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
-	udelay(1);
-	HD_WRITE(VC4_HD_M_CTL, 0);
-
-	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
-
 	HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
 		   VC4_HDMI_SW_RESET_HDMI |
 		   VC4_HDMI_SW_RESET_FORMAT_DETECT);
@@ -1205,6 +1176,23 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 		return -EPROBE_DEFER;
 	}
 
+	/* This is the rate that is set by the firmware.  The number
+	 * needs to be a bit higher than the pixel clock rate
+	 * (generally 148.5Mhz).
+	 */
+	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
+	if (ret) {
+		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
+		goto err_put_i2c;
+	}
+
+	ret = clk_prepare_enable(hdmi->hsm_clock);
+	if (ret) {
+		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
+			  ret);
+		goto err_put_i2c;
+	}
+
 	/* Only use the GPIO HPD pin if present in the DT, otherwise
 	 * we'll use the HDMI core's register.
 	 */
@@ -1216,7 +1204,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 							 &hpd_gpio_flags);
 		if (hdmi->hpd_gpio < 0) {
 			ret = hdmi->hpd_gpio;
-			goto err_put_i2c;
+			goto err_unprepare_hsm;
 		}
 
 		hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
@@ -1224,6 +1212,14 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 
 	vc4->hdmi = hdmi;
 
+	/* HDMI core must be enabled. */
+	if (!(HD_READ(VC4_HD_M_CTL) & VC4_HD_M_ENABLE)) {
+		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
+		udelay(1);
+		HD_WRITE(VC4_HD_M_CTL, 0);
+
+		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
+	}
 	pm_runtime_enable(dev);
 
 	drm_encoder_init(drm, hdmi->encoder, &vc4_hdmi_encoder_funcs,
@@ -1244,6 +1240,8 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 
 err_destroy_encoder:
 	vc4_hdmi_encoder_destroy(hdmi->encoder);
+err_unprepare_hsm:
+	clk_disable_unprepare(hdmi->hsm_clock);
 	pm_runtime_disable(dev);
 err_put_i2c:
 	put_device(&hdmi->ddc->dev);
@@ -1263,6 +1261,7 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master,
 	vc4_hdmi_connector_destroy(hdmi->connector);
 	vc4_hdmi_encoder_destroy(hdmi->encoder);
 
+	clk_disable_unprepare(hdmi->hsm_clock);
 	pm_runtime_disable(dev);
 
 	put_device(&hdmi->ddc->dev);
-- 
2.11.0

[PATCH 3/4] drm/vc4: Add register defines for CEC.

[Hans Verkuil]

From: Eric Anholt <eric@anholt.net>

Basic usage:

poweron: HSM clock should be running.  Set the bit clock divider,
set all the other _US timeouts based on bit clock rate.  Bring RX/TX
reset up and then down.

powerdown: Set RX/TX reset.

interrupt: read CPU_STATUS, write bits that have been handled to
CPU_CLEAR.

Bits are added to /debug/dri/0/hdmi_regs so you can check out the
power-on values.

Signed-off-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c |  16 ++++++
 drivers/gpu/drm/vc4/vc4_regs.h | 108 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index e0104f96011e..b0521e6cc281 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -149,6 +149,22 @@ static const struct {
 	HDMI_REG(VC4_HDMI_VERTB1),
 	HDMI_REG(VC4_HDMI_TX_PHY_RESET_CTL),
 	HDMI_REG(VC4_HDMI_TX_PHY_CTL0),
+
+	HDMI_REG(VC4_HDMI_CEC_CNTRL_1),
+	HDMI_REG(VC4_HDMI_CEC_CNTRL_2),
+	HDMI_REG(VC4_HDMI_CEC_CNTRL_3),
+	HDMI_REG(VC4_HDMI_CEC_CNTRL_4),
+	HDMI_REG(VC4_HDMI_CEC_CNTRL_5),
+	HDMI_REG(VC4_HDMI_CPU_STATUS),
+
+	HDMI_REG(VC4_HDMI_CEC_RX_DATA_1),
+	HDMI_REG(VC4_HDMI_CEC_RX_DATA_2),
+	HDMI_REG(VC4_HDMI_CEC_RX_DATA_3),
+	HDMI_REG(VC4_HDMI_CEC_RX_DATA_4),
+	HDMI_REG(VC4_HDMI_CEC_TX_DATA_1),
+	HDMI_REG(VC4_HDMI_CEC_TX_DATA_2),
+	HDMI_REG(VC4_HDMI_CEC_TX_DATA_3),
+	HDMI_REG(VC4_HDMI_CEC_TX_DATA_4),
 };
 
 static const struct {
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index d382c34c1b9e..b18cc20ee185 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -561,16 +561,124 @@
 # define VC4_HDMI_VERTB_VBP_MASK		VC4_MASK(8, 0)
 # define VC4_HDMI_VERTB_VBP_SHIFT		0
 
+#define VC4_HDMI_CEC_CNTRL_1			0x0e8
+/* Set when the transmission has ended. */
+# define VC4_HDMI_CEC_TX_EOM			BIT(31)
+/* If set, transmission was acked on the 1st or 2nd attempt (only one
+ * retry is attempted).  If in continuous mode, this means TX needs to
+ * be filled if !TX_EOM.
+ */
+# define VC4_HDMI_CEC_TX_STATUS_GOOD		BIT(30)
+# define VC4_HDMI_CEC_RX_EOM			BIT(29)
+# define VC4_HDMI_CEC_RX_STATUS_GOOD		BIT(28)
+/* Number of bytes received for the message. */
+# define VC4_HDMI_CEC_REC_WRD_CNT_MASK		VC4_MASK(27, 24)
+# define VC4_HDMI_CEC_REC_WRD_CNT_SHIFT		24
+/* Sets continuous receive mode.  Generates interrupt after each 8
+ * bytes to signal that RX_DATA should be consumed, and at RX_EOM.
+ *
+ * If disabled, maximum 16 bytes will be received (including header),
+ * and interrupt at RX_EOM.  Later bytes will be acked but not put
+ * into the RX_DATA.
+ */
+# define VC4_HDMI_CEC_RX_CONTINUE		BIT(23)
+# define VC4_HDMI_CEC_TX_CONTINUE		BIT(22)
+/* Set this after a CEC interrupt. */
+# define VC4_HDMI_CEC_CLEAR_RECEIVE_OFF		BIT(21)
+/* Starts a TX.  Will wait for appropriate idel time before CEC
+ * activity. Must be cleared in between transmits.
+ */
+# define VC4_HDMI_CEC_START_XMIT_BEGIN		BIT(20)
+# define VC4_HDMI_CEC_MESSAGE_LENGTH_MASK	VC4_MASK(19, 16)
+# define VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT	16
+/* Device's CEC address */
+# define VC4_HDMI_CEC_ADDR_MASK			VC4_MASK(15, 12)
+# define VC4_HDMI_CEC_ADDR_SHIFT		12
+/* Divides off of HSM clock to generate CEC bit clock. */
+# define VC4_HDMI_CEC_DIV_CLK_CNT_MASK		VC4_MASK(11, 0)
+# define VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT		0
+
+/* Set these fields to how many bit clock cycles get to that many
+ * microseconds.
+ */
+#define VC4_HDMI_CEC_CNTRL_2			0x0ec
+# define VC4_HDMI_CEC_CNT_TO_1500_US_MASK	VC4_MASK(30, 24)
+# define VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT	24
+# define VC4_HDMI_CEC_CNT_TO_1300_US_MASK	VC4_MASK(23, 17)
+# define VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT	17
+# define VC4_HDMI_CEC_CNT_TO_800_US_MASK	VC4_MASK(16, 11)
+# define VC4_HDMI_CEC_CNT_TO_800_US_SHIFT	11
+# define VC4_HDMI_CEC_CNT_TO_600_US_MASK	VC4_MASK(10, 5)
+# define VC4_HDMI_CEC_CNT_TO_600_US_SHIFT	5
+# define VC4_HDMI_CEC_CNT_TO_400_US_MASK	VC4_MASK(4, 0)
+# define VC4_HDMI_CEC_CNT_TO_400_US_SHIFT	0
+
+#define VC4_HDMI_CEC_CNTRL_3			0x0f0
+# define VC4_HDMI_CEC_CNT_TO_2750_US_MASK	VC4_MASK(31, 24)
+# define VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT	24
+# define VC4_HDMI_CEC_CNT_TO_2400_US_MASK	VC4_MASK(23, 16)
+# define VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT	16
+# define VC4_HDMI_CEC_CNT_TO_2050_US_MASK	VC4_MASK(15, 8)
+# define VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT	8
+# define VC4_HDMI_CEC_CNT_TO_1700_US_MASK	VC4_MASK(7, 0)
+# define VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT	0
+
+#define VC4_HDMI_CEC_CNTRL_4			0x0f4
+# define VC4_HDMI_CEC_CNT_TO_4300_US_MASK	VC4_MASK(31, 24)
+# define VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT	24
+# define VC4_HDMI_CEC_CNT_TO_3900_US_MASK	VC4_MASK(23, 16)
+# define VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT	16
+# define VC4_HDMI_CEC_CNT_TO_3600_US_MASK	VC4_MASK(15, 8)
+# define VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT	8
+# define VC4_HDMI_CEC_CNT_TO_3500_US_MASK	VC4_MASK(7, 0)
+# define VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT	0
+
+#define VC4_HDMI_CEC_CNTRL_5			0x0f8
+# define VC4_HDMI_CEC_TX_SW_RESET		BIT(27)
+# define VC4_HDMI_CEC_RX_SW_RESET		BIT(26)
+# define VC4_HDMI_CEC_PAD_SW_RESET		BIT(25)
+# define VC4_HDMI_CEC_MUX_TP_OUT_CEC		BIT(24)
+# define VC4_HDMI_CEC_RX_CEC_INT		BIT(23)
+# define VC4_HDMI_CEC_CLK_PRELOAD_MASK		VC4_MASK(22, 16)
+# define VC4_HDMI_CEC_CLK_PRELOAD_SHIFT		16
+# define VC4_HDMI_CEC_CNT_TO_4700_US_MASK	VC4_MASK(15, 8)
+# define VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT	8
+# define VC4_HDMI_CEC_CNT_TO_4500_US_MASK	VC4_MASK(7, 0)
+# define VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT	0
+
+/* Transmit data, first byte is low byte of the 32-bit reg.  MSB of
+ * each byte transmitted first.
+ */
+#define VC4_HDMI_CEC_TX_DATA_1			0x0fc
+#define VC4_HDMI_CEC_TX_DATA_2			0x100
+#define VC4_HDMI_CEC_TX_DATA_3			0x104
+#define VC4_HDMI_CEC_TX_DATA_4			0x108
+#define VC4_HDMI_CEC_RX_DATA_1			0x10c
+#define VC4_HDMI_CEC_RX_DATA_2			0x110
+#define VC4_HDMI_CEC_RX_DATA_3			0x114
+#define VC4_HDMI_CEC_RX_DATA_4			0x118
+
 #define VC4_HDMI_TX_PHY_RESET_CTL		0x2c0
 
 #define VC4_HDMI_TX_PHY_CTL0			0x2c4
 # define VC4_HDMI_TX_PHY_RNG_PWRDN		BIT(25)
 
+/* Interrupt status bits */
+#define VC4_HDMI_CPU_STATUS			0x340
+#define VC4_HDMI_CPU_SET			0x344
+#define VC4_HDMI_CPU_CLEAR			0x348
+# define VC4_HDMI_CPU_CEC			BIT(6)
+# define VC4_HDMI_CPU_HOTPLUG			BIT(0)
+
 #define VC4_HDMI_GCP(x)				(0x400 + ((x) * 0x4))
 #define VC4_HDMI_RAM_PACKET(x)			(0x400 + ((x) * 0x24))
 #define VC4_HDMI_PACKET_STRIDE			0x24
 
 #define VC4_HD_M_CTL				0x00c
+/* Debug: Current receive value on the CEC pad. */
+# define VC4_HD_CECRXD				BIT(9)
+/* Debug: Override CEC output to 0. */
+# define VC4_HD_CECOVR				BIT(8)
 # define VC4_HD_M_REGISTER_FILE_STANDBY		(3 << 6)
 # define VC4_HD_M_RAM_STANDBY			(3 << 4)
 # define VC4_HD_M_SW_RST			BIT(2)
-- 
2.11.0

[PATCH 4/4] drm/vc4: add HDMI CEC support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch adds support to VC4 for CEC.

To prevent the firmware from eating the CEC interrupts you need to add this to
your config.txt:

mask_gpu_interrupt1=0x100

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/vc4/Kconfig    |   8 ++
 drivers/gpu/drm/vc4/vc4_hdmi.c | 203 ++++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/vc4/vc4_regs.h |   5 +
 3 files changed, 211 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
index 4361bdcfd28a..fdae18aeab4f 100644
--- a/drivers/gpu/drm/vc4/Kconfig
+++ b/drivers/gpu/drm/vc4/Kconfig
@@ -19,3 +19,11 @@ config DRM_VC4
 	  This driver requires that "avoid_warnings=2" be present in
 	  the config.txt for the firmware, to keep it from smashing
 	  our display setup.
+
+config DRM_VC4_HDMI_CEC
+       bool "Broadcom VC4 HDMI CEC Support"
+       depends on DRM_VC4
+       select CEC_CORE
+       help
+	  Choose this option if you have a Broadcom VC4 GPU
+	  and want to use CEC.
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index b0521e6cc281..14e2ece5db94 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -57,6 +57,7 @@
 #include <sound/pcm_drm_eld.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
+#include "media/cec.h"
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
@@ -85,6 +86,11 @@ struct vc4_hdmi {
 	int hpd_gpio;
 	bool hpd_active_low;
 
+	struct cec_adapter *cec_adap;
+	struct cec_msg cec_rx_msg;
+	bool cec_tx_ok;
+	bool cec_irq_was_rx;
+
 	struct clk *pixel_clock;
 	struct clk *hsm_clock;
 };
@@ -156,6 +162,7 @@ static const struct {
 	HDMI_REG(VC4_HDMI_CEC_CNTRL_4),
 	HDMI_REG(VC4_HDMI_CEC_CNTRL_5),
 	HDMI_REG(VC4_HDMI_CPU_STATUS),
+	HDMI_REG(VC4_HDMI_CPU_MASK_STATUS),
 
 	HDMI_REG(VC4_HDMI_CEC_RX_DATA_1),
 	HDMI_REG(VC4_HDMI_CEC_RX_DATA_2),
@@ -232,8 +239,8 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 		if (gpio_get_value_cansleep(vc4->hdmi->hpd_gpio) ^
 		    vc4->hdmi->hpd_active_low)
 			return connector_status_connected;
-		else
-			return connector_status_disconnected;
+		cec_phys_addr_invalidate(vc4->hdmi->cec_adap);
+		return connector_status_disconnected;
 	}
 
 	if (drm_probe_ddc(vc4->hdmi->ddc))
@@ -241,8 +248,8 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 
 	if (HDMI_READ(VC4_HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)
 		return connector_status_connected;
-	else
-		return connector_status_disconnected;
+	cec_phys_addr_invalidate(vc4->hdmi->cec_adap);
+	return connector_status_disconnected;
 }
 
 static void vc4_hdmi_connector_destroy(struct drm_connector *connector)
@@ -263,6 +270,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 	struct edid *edid;
 
 	edid = drm_get_edid(connector, vc4->hdmi->ddc);
+	cec_s_phys_addr_from_edid(vc4->hdmi->cec_adap, edid);
 	if (!edid)
 		return -ENODEV;
 
@@ -1137,6 +1145,165 @@ static void vc4_hdmi_audio_cleanup(struct vc4_hdmi *hdmi)
 		snd_soc_unregister_codec(dev);
 }
 
+#ifdef CONFIG_DRM_VC4_HDMI_CEC
+static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
+{
+	struct vc4_dev *vc4 = priv;
+	struct vc4_hdmi *hdmi = vc4->hdmi;
+
+	if (hdmi->cec_irq_was_rx) {
+		if (hdmi->cec_rx_msg.len)
+			cec_received_msg(hdmi->cec_adap, &hdmi->cec_rx_msg);
+	} else if (hdmi->cec_tx_ok) {
+		cec_transmit_done(hdmi->cec_adap, CEC_TX_STATUS_OK,
+				  0, 0, 0, 0);
+	} else {
+		/*
+		 * This CEC implementation makes 1 retry, so if we
+		 * get a NACK, then that means it made 2 attempts.
+		 */
+		cec_transmit_done(hdmi->cec_adap, CEC_TX_STATUS_NACK,
+				  0, 2, 0, 0);
+	}
+	return IRQ_HANDLED;
+}
+
+static void vc4_cec_read_msg(struct vc4_dev *vc4, u32 cntrl1)
+{
+	struct cec_msg *msg = &vc4->hdmi->cec_rx_msg;
+	unsigned int i;
+
+	msg->len = 1 + ((cntrl1 & VC4_HDMI_CEC_REC_WRD_CNT_MASK) >>
+					VC4_HDMI_CEC_REC_WRD_CNT_SHIFT);
+	for (i = 0; i < msg->len; i += 4) {
+		u32 val = HDMI_READ(VC4_HDMI_CEC_RX_DATA_1 + i);
+
+		msg->msg[i] = val & 0xff;
+		msg->msg[i + 1] = (val >> 8) & 0xff;
+		msg->msg[i + 2] = (val >> 16) & 0xff;
+		msg->msg[i + 3] = (val >> 24) & 0xff;
+	}
+}
+
+static irqreturn_t vc4_cec_irq_handler(int irq, void *priv)
+{
+	struct vc4_dev *vc4 = priv;
+	struct vc4_hdmi *hdmi = vc4->hdmi;
+	u32 stat = HDMI_READ(VC4_HDMI_CPU_STATUS);
+	u32 cntrl1, cntrl5;
+
+	if (!(stat & VC4_HDMI_CPU_CEC))
+		return IRQ_NONE;
+	hdmi->cec_rx_msg.len = 0;
+	cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
+	cntrl5 = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
+	hdmi->cec_irq_was_rx = cntrl5 & VC4_HDMI_CEC_RX_CEC_INT;
+	if (hdmi->cec_irq_was_rx) {
+		vc4_cec_read_msg(vc4, cntrl1);
+		cntrl1 |= VC4_HDMI_CEC_CLEAR_RECEIVE_OFF;
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, cntrl1);
+		cntrl1 &= ~VC4_HDMI_CEC_CLEAR_RECEIVE_OFF;
+	} else {
+		hdmi->cec_tx_ok = cntrl1 & VC4_HDMI_CEC_TX_STATUS_GOOD;
+		cntrl1 &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
+	}
+	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, cntrl1);
+	HDMI_WRITE(VC4_HDMI_CPU_CLEAR, VC4_HDMI_CPU_CEC);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct vc4_dev *vc4 = cec_get_drvdata(adap);
+	u32 hsm_clock = clk_get_rate(vc4->hdmi->hsm_clock);
+	u32 cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
+	u32 divclk = cntrl1 & VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
+	/* clock period in microseconds */
+	u32 usecs = 1000000 / (hsm_clock / divclk);
+	u32 val = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
+
+	val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
+		 VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
+		 VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
+	val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
+	       ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
+
+	if (enable) {
+		cntrl1 &= VC4_HDMI_CEC_DIV_CLK_CNT_MASK |
+			  VC4_HDMI_CEC_ADDR_MASK;
+
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
+			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val);
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_2,
+			 ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
+			 ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
+			 ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
+			 ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
+			 ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_3,
+			 ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
+			 ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
+			 ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
+			 ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_4,
+			 ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
+			 ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
+			 ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
+			 ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
+
+		HDMI_WRITE(VC4_HDMI_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
+	} else {
+		HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
+		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
+			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+	}
+	return 0;
+}
+
+static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
+{
+	struct vc4_dev *vc4 = cec_get_drvdata(adap);
+
+	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1,
+		   (HDMI_READ(VC4_HDMI_CEC_CNTRL_1) & ~VC4_HDMI_CEC_ADDR_MASK) |
+		   (log_addr & 0xf) << VC4_HDMI_CEC_ADDR_SHIFT);
+	return 0;
+}
+
+static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+				      u32 signal_free_time, struct cec_msg *msg)
+{
+	struct vc4_dev *vc4 = cec_get_drvdata(adap);
+	u32 val;
+	unsigned int i;
+
+	for (i = 0; i < msg->len; i += 4)
+		HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
+			   (msg->msg[i]) |
+			   (msg->msg[i + 1] << 8) |
+			   (msg->msg[i + 2] << 16) |
+			   (msg->msg[i + 3] << 24));
+
+	val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
+	val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
+	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
+	val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
+	val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
+	val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
+
+	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
+	return 0;
+}
+
+static const struct cec_adap_ops vc4_hdmi_cec_adap_ops = {
+	.adap_enable = vc4_hdmi_cec_adap_enable,
+	.adap_log_addr = vc4_hdmi_cec_adap_log_addr,
+	.adap_transmit = vc4_hdmi_cec_adap_transmit,
+};
+#endif
+
 static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1247,6 +1414,26 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 		ret = PTR_ERR(hdmi->connector);
 		goto err_destroy_encoder;
 	}
+#ifdef CONFIG_DRM_VC4_HDMI_CEC
+	hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops,
+		vc4, "vc4", CEC_CAP_TRANSMIT | CEC_CAP_LOG_ADDRS |
+		CEC_CAP_PASSTHROUGH | CEC_CAP_RC, 1);
+	ret = PTR_ERR_OR_ZERO(hdmi->cec_adap);
+	if (ret < 0)
+		goto err_destroy_conn;
+	HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, 0xffffffff);
+	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1,
+		   (HDMI_READ(VC4_HDMI_CEC_CNTRL_1) | VC4_HDMI_CEC_ADDR_MASK));
+	ret = devm_request_threaded_irq(dev, platform_get_irq(pdev, 0),
+					vc4_cec_irq_handler,
+					vc4_cec_irq_handler_thread, 0,
+					"vc4 hdmi cec", vc4);
+	if (ret)
+		goto err_delete_cec_adap;
+	ret = cec_register_adapter(hdmi->cec_adap, dev);
+	if (ret < 0)
+		goto err_delete_cec_adap;
+#endif
 
 	ret = vc4_hdmi_audio_init(hdmi);
 	if (ret)
@@ -1254,6 +1441,12 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 
 	return 0;
 
+#ifdef CONFIG_DRM_VC4_HDMI_CEC
+err_delete_cec_adap:
+	cec_delete_adapter(hdmi->cec_adap);
+err_destroy_conn:
+	vc4_hdmi_connector_destroy(hdmi->connector);
+#endif
 err_destroy_encoder:
 	vc4_hdmi_encoder_destroy(hdmi->encoder);
 err_unprepare_hsm:
@@ -1273,7 +1466,7 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master,
 	struct vc4_hdmi *hdmi = vc4->hdmi;
 
 	vc4_hdmi_audio_cleanup(hdmi);
-
+	cec_unregister_adapter(hdmi->cec_adap);
 	vc4_hdmi_connector_destroy(hdmi->connector);
 	vc4_hdmi_encoder_destroy(hdmi->encoder);
 
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index b18cc20ee185..55677bd50f66 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -595,6 +595,7 @@
 # define VC4_HDMI_CEC_ADDR_MASK			VC4_MASK(15, 12)
 # define VC4_HDMI_CEC_ADDR_SHIFT		12
 /* Divides off of HSM clock to generate CEC bit clock. */
+/* With the current defaults the CEC bit clock is 40 kHz = 25 usec */
 # define VC4_HDMI_CEC_DIV_CLK_CNT_MASK		VC4_MASK(11, 0)
 # define VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT		0
 
@@ -670,6 +671,10 @@
 # define VC4_HDMI_CPU_CEC			BIT(6)
 # define VC4_HDMI_CPU_HOTPLUG			BIT(0)
 
+#define VC4_HDMI_CPU_MASK_STATUS		0x34c
+#define VC4_HDMI_CPU_MASK_SET			0x350
+#define VC4_HDMI_CPU_MASK_CLEAR			0x354
+
 #define VC4_HDMI_GCP(x)				(0x400 + ((x) * 0x4))
 #define VC4_HDMI_RAM_PACKET(x)			(0x400 + ((x) * 0x24))
 #define VC4_HDMI_PACKET_STRIDE			0x24
-- 
2.11.0

Re: [PATCH 3/4] drm/vc4: Add register defines for CEC.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> From: Eric Anholt <eric@anholt.net>
>
> Basic usage:
>
> poweron: HSM clock should be running.  Set the bit clock divider,
> set all the other _US timeouts based on bit clock rate.  Bring RX/TX
> reset up and then down.
>
> powerdown: Set RX/TX reset.
>
> interrupt: read CPU_STATUS, write bits that have been handled to
> CPU_CLEAR.
>
> Bits are added to /debug/dri/0/hdmi_regs so you can check out the
> power-on values.

Let's drop my hints to you from the commit message here :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/4] drm/vc4: prepare for CEC support

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 4493 bytes --]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> In order to support CEC the hsm clock needs to be enabled in
> vc4_hdmi_bind(), not in vc4_hdmi_encoder_enable(). Otherwise you wouldn't
> be able to support CEC when there is no hotplug detect signal, which is
> required by some monitors that turn off the HPD when in standby, but keep
> the CEC bus alive so they can be woken up.
>
> The HDMI core also has to be enabled in vc4_hdmi_bind() for the same
> reason.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Ccing Boris, I'd love to see what he thinks of this and if we can do any
better.

Hans, is it true that CEC needs to be on always, or could it only be
enabled when somebody is listening to messages?

>  drivers/gpu/drm/vc4/vc4_hdmi.c | 59 +++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index ed63d4e85762..e0104f96011e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -463,11 +463,6 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder)
>  	HD_WRITE(VC4_HD_VID_CTL,
>  		 HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
>  
> -	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
> -	udelay(1);
> -	HD_WRITE(VC4_HD_M_CTL, 0);
> -
> -	clk_disable_unprepare(hdmi->hsm_clock);
>  	clk_disable_unprepare(hdmi->pixel_clock);
>  
>  	ret = pm_runtime_put(&hdmi->pdev->dev);
> @@ -509,16 +504,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
>  		return;
>  	}
>  
> -	/* This is the rate that is set by the firmware.  The number
> -	 * needs to be a bit higher than the pixel clock rate
> -	 * (generally 148.5Mhz).
> -	 */
> -	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
> -	if (ret) {
> -		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
> -		return;
> -	}
> -
>  	ret = clk_set_rate(hdmi->pixel_clock,
>  			   mode->clock * 1000 *
>  			   ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
> @@ -533,20 +518,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
>  		return;
>  	}
>  
> -	ret = clk_prepare_enable(hdmi->hsm_clock);
> -	if (ret) {
> -		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
> -			  ret);
> -		clk_disable_unprepare(hdmi->pixel_clock);
> -		return;
> -	}
> -
> -	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
> -	udelay(1);
> -	HD_WRITE(VC4_HD_M_CTL, 0);
> -
> -	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
> -
>  	HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
>  		   VC4_HDMI_SW_RESET_HDMI |
>  		   VC4_HDMI_SW_RESET_FORMAT_DETECT);
> @@ -1205,6 +1176,23 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>  		return -EPROBE_DEFER;
>  	}
>  
> +	/* This is the rate that is set by the firmware.  The number
> +	 * needs to be a bit higher than the pixel clock rate
> +	 * (generally 148.5Mhz).
> +	 */
> +	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
> +	if (ret) {
> +		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
> +		goto err_put_i2c;
> +	}
> +
> +	ret = clk_prepare_enable(hdmi->hsm_clock);
> +	if (ret) {
> +		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
> +			  ret);
> +		goto err_put_i2c;
> +	}
> +
>  	/* Only use the GPIO HPD pin if present in the DT, otherwise
>  	 * we'll use the HDMI core's register.
>  	 */
> @@ -1216,7 +1204,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>  							 &hpd_gpio_flags);
>  		if (hdmi->hpd_gpio < 0) {
>  			ret = hdmi->hpd_gpio;
> -			goto err_put_i2c;
> +			goto err_unprepare_hsm;
>  		}
>  
>  		hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
> @@ -1224,6 +1212,14 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>  
>  	vc4->hdmi = hdmi;
>  
> +	/* HDMI core must be enabled. */
> +	if (!(HD_READ(VC4_HD_M_CTL) & VC4_HD_M_ENABLE)) {
> +		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
> +		udelay(1);
> +		HD_WRITE(VC4_HD_M_CTL, 0);
> +
> +		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
> +	}

I'm wondering if there's any impact from leaving VC4_HD_M_ENABLE on
while the HDMI power domain is off.  I don't quite understand the role
of the power domain, and I've fired off an email internally to check if
there are any experts on this hardware still around.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 6470 bytes --]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> This patch adds support to VC4 for CEC.
>
> To prevent the firmware from eating the CEC interrupts you need to add this to
> your config.txt:
>
> mask_gpu_interrupt1=0x100
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

This looks pretty great.  Just a couple of little comments.

> ---
>  drivers/gpu/drm/vc4/Kconfig    |   8 ++
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 203 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/vc4/vc4_regs.h |   5 +
>  3 files changed, 211 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> index 4361bdcfd28a..fdae18aeab4f 100644
> --- a/drivers/gpu/drm/vc4/Kconfig
> +++ b/drivers/gpu/drm/vc4/Kconfig
> @@ -19,3 +19,11 @@ config DRM_VC4
>  	  This driver requires that "avoid_warnings=2" be present in
>  	  the config.txt for the firmware, to keep it from smashing
>  	  our display setup.
> +
> +config DRM_VC4_HDMI_CEC
> +       bool "Broadcom VC4 HDMI CEC Support"
> +       depends on DRM_VC4
> +       select CEC_CORE
> +       help
> +	  Choose this option if you have a Broadcom VC4 GPU
> +	  and want to use CEC.

Do we need a Kconfig for this?  Couldn't we just #ifdef on CEC_CORE
instead?

> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b0521e6cc281..14e2ece5db94 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c

> +static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
> +	u32 hsm_clock = clk_get_rate(vc4->hdmi->hsm_clock);
> +	u32 cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
> +	u32 divclk = cntrl1 & VC4_HDMI_CEC_DIV_CLK_CNT_MASK;

We should probably be setting the divider to a value of our choice,
rather than relying on whatever default value is there.

(Bonus points if we were to do this using a common clk divider, so the
rate shows up in /debug/clk/clk_summary, but I won't require that)

> +	/* clock period in microseconds */
> +	u32 usecs = 1000000 / (hsm_clock / divclk);
> +	u32 val = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
> +
> +	val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
> +		 VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
> +		 VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
> +	val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
> +	       ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
> +
> +	if (enable) {
> +		cntrl1 &= VC4_HDMI_CEC_DIV_CLK_CNT_MASK |
> +			  VC4_HDMI_CEC_ADDR_MASK;
> +
> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
> +			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val);
> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_2,
> +			 ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
> +			 ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
> +			 ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
> +			 ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
> +			 ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_3,
> +			 ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
> +			 ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
> +			 ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
> +			 ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_4,
> +			 ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
> +			 ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
> +			 ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
> +			 ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
> +
> +		HDMI_WRITE(VC4_HDMI_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
> +	} else {
> +		HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
> +			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
> +	}
> +	return 0;
> +}

> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				      u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
> +	u32 val;
> +	unsigned int i;
> +
> +	for (i = 0; i < msg->len; i += 4)
> +		HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
> +			   (msg->msg[i]) |
> +			   (msg->msg[i + 1] << 8) |
> +			   (msg->msg[i + 2] << 16) |
> +			   (msg->msg[i + 3] << 24));
> +
> +	val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
> +	val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
> +	val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
> +	val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
> +	val |= VC4_HDMI_CEC_START_XMIT_BEGIN;

It doesn't look to me like len should have 1 subtracted from it.  The
field has 4 bits for our up-to-16-byte length, and the firmware seems to
be setting it to the same value as a memcpy for the message data uses.

> +
> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
> +	return 0;
> +}
> +
> +static const struct cec_adap_ops vc4_hdmi_cec_adap_ops = {
> +	.adap_enable = vc4_hdmi_cec_adap_enable,
> +	.adap_log_addr = vc4_hdmi_cec_adap_log_addr,
> +	.adap_transmit = vc4_hdmi_cec_adap_transmit,
> +};
> +#endif

> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> index b18cc20ee185..55677bd50f66 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -595,6 +595,7 @@
>  # define VC4_HDMI_CEC_ADDR_MASK			VC4_MASK(15, 12)
>  # define VC4_HDMI_CEC_ADDR_SHIFT		12
>  /* Divides off of HSM clock to generate CEC bit clock. */
> +/* With the current defaults the CEC bit clock is 40 kHz = 25 usec */
>  # define VC4_HDMI_CEC_DIV_CLK_CNT_MASK		VC4_MASK(11, 0)
>  # define VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT		0
>  
> @@ -670,6 +671,10 @@
>  # define VC4_HDMI_CPU_CEC			BIT(6)
>  # define VC4_HDMI_CPU_HOTPLUG			BIT(0)
>  
> +#define VC4_HDMI_CPU_MASK_STATUS		0x34c
> +#define VC4_HDMI_CPU_MASK_SET			0x350
> +#define VC4_HDMI_CPU_MASK_CLEAR			0x354
> +
>  #define VC4_HDMI_GCP(x)				(0x400 + ((x) * 0x4))
>  #define VC4_HDMI_RAM_PACKET(x)			(0x400 + ((x) * 0x24))
>  #define VC4_HDMI_PACKET_STRIDE			0x24
> -- 
> 2.11.0

Maybe squash these changes into the previous patch?  Or we could squash
the previous patch into this one and just tack my signed-off-by on
yours.  Either way's fine with me.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/4] drm/vc4: prepare for CEC support

[Hans Verkuil]

On 12/07/17 20:42, Eric Anholt wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> In order to support CEC the hsm clock needs to be enabled in
>> vc4_hdmi_bind(), not in vc4_hdmi_encoder_enable(). Otherwise you wouldn't
>> be able to support CEC when there is no hotplug detect signal, which is
>> required by some monitors that turn off the HPD when in standby, but keep
>> the CEC bus alive so they can be woken up.
>>
>> The HDMI core also has to be enabled in vc4_hdmi_bind() for the same
>> reason.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Ccing Boris, I'd love to see what he thinks of this and if we can do any
> better.
> 
> Hans, is it true that CEC needs to be on always, or could it only be
> enabled when somebody is listening to messages?

It depends. If a valid physical address is read from the EDID (i.e. we are
connected to a display) then CEC has to be on always otherwise you can't receive
messages that the display (or others) send to you. Note that even if there is
no application listening to messages, the CEC framework will still listen to
and process CEC core messages and remote control passthrough messages.

If there is no physical address, for example because the hotplug detect is low,
then that is a special case: some displays will turn off the HPD when in standby,
but CEC still works. Devices can send an Image View On CEC message to wake up such
displays. This is a corner case explicitly allowed by the CEC spec. In my view this
is a hack in the specification just to work around broken displays. But such
displays really exist, unfortunately.

So in that case CEC has to be enabled only when we transmit a message.

This is what the CEC framework does: the adap_enabled callback is called with
true when a physical address is set and with false when the PA goes away. If you
transmit a message while there is no PA then adap_enabled is called with true
right before the transmit and with false when the transmit finished.

Regards,

	Hans

> 
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 59 +++++++++++++++++++++---------------------
>>  1 file changed, 29 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index ed63d4e85762..e0104f96011e 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -463,11 +463,6 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder)
>>  	HD_WRITE(VC4_HD_VID_CTL,
>>  		 HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
>>  
>> -	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
>> -	udelay(1);
>> -	HD_WRITE(VC4_HD_M_CTL, 0);
>> -
>> -	clk_disable_unprepare(hdmi->hsm_clock);
>>  	clk_disable_unprepare(hdmi->pixel_clock);
>>  
>>  	ret = pm_runtime_put(&hdmi->pdev->dev);
>> @@ -509,16 +504,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
>>  		return;
>>  	}
>>  
>> -	/* This is the rate that is set by the firmware.  The number
>> -	 * needs to be a bit higher than the pixel clock rate
>> -	 * (generally 148.5Mhz).
>> -	 */
>> -	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
>> -	if (ret) {
>> -		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
>> -		return;
>> -	}
>> -
>>  	ret = clk_set_rate(hdmi->pixel_clock,
>>  			   mode->clock * 1000 *
>>  			   ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
>> @@ -533,20 +518,6 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
>>  		return;
>>  	}
>>  
>> -	ret = clk_prepare_enable(hdmi->hsm_clock);
>> -	if (ret) {
>> -		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
>> -			  ret);
>> -		clk_disable_unprepare(hdmi->pixel_clock);
>> -		return;
>> -	}
>> -
>> -	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
>> -	udelay(1);
>> -	HD_WRITE(VC4_HD_M_CTL, 0);
>> -
>> -	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
>> -
>>  	HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
>>  		   VC4_HDMI_SW_RESET_HDMI |
>>  		   VC4_HDMI_SW_RESET_FORMAT_DETECT);
>> @@ -1205,6 +1176,23 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>  		return -EPROBE_DEFER;
>>  	}
>>  
>> +	/* This is the rate that is set by the firmware.  The number
>> +	 * needs to be a bit higher than the pixel clock rate
>> +	 * (generally 148.5Mhz).
>> +	 */
>> +	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
>> +		goto err_put_i2c;
>> +	}
>> +
>> +	ret = clk_prepare_enable(hdmi->hsm_clock);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
>> +			  ret);
>> +		goto err_put_i2c;
>> +	}
>> +
>>  	/* Only use the GPIO HPD pin if present in the DT, otherwise
>>  	 * we'll use the HDMI core's register.
>>  	 */
>> @@ -1216,7 +1204,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>  							 &hpd_gpio_flags);
>>  		if (hdmi->hpd_gpio < 0) {
>>  			ret = hdmi->hpd_gpio;
>> -			goto err_put_i2c;
>> +			goto err_unprepare_hsm;
>>  		}
>>  
>>  		hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
>> @@ -1224,6 +1212,14 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>  
>>  	vc4->hdmi = hdmi;
>>  
>> +	/* HDMI core must be enabled. */
>> +	if (!(HD_READ(VC4_HD_M_CTL) & VC4_HD_M_ENABLE)) {
>> +		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
>> +		udelay(1);
>> +		HD_WRITE(VC4_HD_M_CTL, 0);
>> +
>> +		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
>> +	}
> 
> I'm wondering if there's any impact from leaving VC4_HD_M_ENABLE on
> while the HDMI power domain is off.  I don't quite understand the role
> of the power domain, and I've fired off an email internally to check if
> there are any experts on this hardware still around.
> 

Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Hans Verkuil]

On 12/07/17 21:02, Eric Anholt wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> This patch adds support to VC4 for CEC.
>>
>> To prevent the firmware from eating the CEC interrupts you need to add this to
>> your config.txt:
>>
>> mask_gpu_interrupt1=0x100
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This looks pretty great.  Just a couple of little comments.
> 
>> ---
>>  drivers/gpu/drm/vc4/Kconfig    |   8 ++
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 203 ++++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/vc4/vc4_regs.h |   5 +
>>  3 files changed, 211 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
>> index 4361bdcfd28a..fdae18aeab4f 100644
>> --- a/drivers/gpu/drm/vc4/Kconfig
>> +++ b/drivers/gpu/drm/vc4/Kconfig
>> @@ -19,3 +19,11 @@ config DRM_VC4
>>  	  This driver requires that "avoid_warnings=2" be present in
>>  	  the config.txt for the firmware, to keep it from smashing
>>  	  our display setup.
>> +
>> +config DRM_VC4_HDMI_CEC
>> +       bool "Broadcom VC4 HDMI CEC Support"
>> +       depends on DRM_VC4
>> +       select CEC_CORE
>> +       help
>> +	  Choose this option if you have a Broadcom VC4 GPU
>> +	  and want to use CEC.
> 
> Do we need a Kconfig for this?  Couldn't we just #ifdef on CEC_CORE
> instead?

It's been my practice to do so for all drivers where I added CEC support.
The main reason is that it is an optional feature of the HDMI protocol, so
you simply may not want to use it to avoid loading the 55+ kB of the cec module.
It will likely grow in size in the future as well.

Also (esp. true for embedded devices) the CEC pin might not even be hooked up!

Finally, you may prefer to use e.g. a Pulse-Eight USB adapter for whatever
reason and then you don't need this either.

> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index b0521e6cc281..14e2ece5db94 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> 
>> +static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
>> +	u32 hsm_clock = clk_get_rate(vc4->hdmi->hsm_clock);
>> +	u32 cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>> +	u32 divclk = cntrl1 & VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
> 
> We should probably be setting the divider to a value of our choice,
> rather than relying on whatever default value is there.

Hardcode the divider to 4091, you mean? I can do that.

> 
> (Bonus points if we were to do this using a common clk divider, so the
> rate shows up in /debug/clk/clk_summary, but I won't require that)
> 
>> +	/* clock period in microseconds */
>> +	u32 usecs = 1000000 / (hsm_clock / divclk);
>> +	u32 val = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
>> +
>> +	val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
>> +		 VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
>> +		 VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
>> +	val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
>> +	       ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
>> +
>> +	if (enable) {
>> +		cntrl1 &= VC4_HDMI_CEC_DIV_CLK_CNT_MASK |
>> +			  VC4_HDMI_CEC_ADDR_MASK;
>> +
>> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
>> +			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
>> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val);
>> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_2,
>> +			 ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
>> +			 ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
>> +			 ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
>> +			 ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
>> +			 ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
>> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_3,
>> +			 ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
>> +			 ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
>> +			 ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
>> +			 ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
>> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_4,
>> +			 ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
>> +			 ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
>> +			 ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
>> +			 ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
>> +
>> +		HDMI_WRITE(VC4_HDMI_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
>> +	} else {
>> +		HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
>> +		HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
>> +			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
>> +	}
>> +	return 0;
>> +}
> 
>> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>> +				      u32 signal_free_time, struct cec_msg *msg)
>> +{
>> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
>> +	u32 val;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < msg->len; i += 4)
>> +		HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
>> +			   (msg->msg[i]) |
>> +			   (msg->msg[i + 1] << 8) |
>> +			   (msg->msg[i + 2] << 16) |
>> +			   (msg->msg[i + 3] << 24));
>> +
>> +	val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>> +	val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
>> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
>> +	val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
>> +	val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
>> +	val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
> 
> It doesn't look to me like len should have 1 subtracted from it.  The
> field has 4 bits for our up-to-16-byte length, and the firmware seems to
> be setting it to the same value as a memcpy for the message data uses.

You need to subtract by one. The CEC protocol supports messages of 1-16
bytes in length. Since the message length mask is only 4 bits you need to
encode this in the value 0-15. Hence the '-1', otherwise you would never
be able to send 16 byte messages.

I actually found this when debugging the messages it was transmitting: they
were one too long.

This suggests that the firmware does this wrong. I don't have time tomorrow,
but I'll see if I can do a quick test on Friday to verify that.

> 
>> +
>> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
>> +	return 0;
>> +}
>> +
>> +static const struct cec_adap_ops vc4_hdmi_cec_adap_ops = {
>> +	.adap_enable = vc4_hdmi_cec_adap_enable,
>> +	.adap_log_addr = vc4_hdmi_cec_adap_log_addr,
>> +	.adap_transmit = vc4_hdmi_cec_adap_transmit,
>> +};
>> +#endif
> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
>> index b18cc20ee185..55677bd50f66 100644
>> --- a/drivers/gpu/drm/vc4/vc4_regs.h
>> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
>> @@ -595,6 +595,7 @@
>>  # define VC4_HDMI_CEC_ADDR_MASK			VC4_MASK(15, 12)
>>  # define VC4_HDMI_CEC_ADDR_SHIFT		12
>>  /* Divides off of HSM clock to generate CEC bit clock. */
>> +/* With the current defaults the CEC bit clock is 40 kHz = 25 usec */
>>  # define VC4_HDMI_CEC_DIV_CLK_CNT_MASK		VC4_MASK(11, 0)
>>  # define VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT		0
>>  
>> @@ -670,6 +671,10 @@
>>  # define VC4_HDMI_CPU_CEC			BIT(6)
>>  # define VC4_HDMI_CPU_HOTPLUG			BIT(0)
>>  
>> +#define VC4_HDMI_CPU_MASK_STATUS		0x34c
>> +#define VC4_HDMI_CPU_MASK_SET			0x350
>> +#define VC4_HDMI_CPU_MASK_CLEAR			0x354
>> +
>>  #define VC4_HDMI_GCP(x)				(0x400 + ((x) * 0x4))
>>  #define VC4_HDMI_RAM_PACKET(x)			(0x400 + ((x) * 0x24))
>>  #define VC4_HDMI_PACKET_STRIDE			0x24
>> -- 
>> 2.11.0
> 
> Maybe squash these changes into the previous patch?  Or we could squash
> the previous patch into this one and just tack my signed-off-by on
> yours.  Either way's fine with me.
> 

I prefer the latter option, just combine it all into this patch.

Regards,

	Hans

Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Hans Verkuil]

On 12/07/17 21:43, Hans Verkuil wrote:
> On 12/07/17 21:02, Eric Anholt wrote:
>>> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>> +				      u32 signal_free_time, struct cec_msg *msg)
>>> +{
>>> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
>>> +	u32 val;
>>> +	unsigned int i;
>>> +
>>> +	for (i = 0; i < msg->len; i += 4)
>>> +		HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
>>> +			   (msg->msg[i]) |
>>> +			   (msg->msg[i + 1] << 8) |
>>> +			   (msg->msg[i + 2] << 16) |
>>> +			   (msg->msg[i + 3] << 24));
>>> +
>>> +	val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>>> +	val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
>>> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
>>> +	val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
>>> +	val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
>>> +	val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
>>
>> It doesn't look to me like len should have 1 subtracted from it.  The
>> field has 4 bits for our up-to-16-byte length, and the firmware seems to
>> be setting it to the same value as a memcpy for the message data uses.
> 
> You need to subtract by one. The CEC protocol supports messages of 1-16
> bytes in length. Since the message length mask is only 4 bits you need to
> encode this in the value 0-15. Hence the '-1', otherwise you would never
> be able to send 16 byte messages.
> 
> I actually found this when debugging the messages it was transmitting: they
> were one too long.
> 
> This suggests that the firmware does this wrong. I don't have time tomorrow,
> but I'll see if I can do a quick test on Friday to verify that.

I double-checked this and both the driver and the firmware do the right thing.
Just to be certain I also tried sending a message that uses the full 16 byte
payload and that too went well. So the code is definitely correct.

Regards,

	Hans

Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> On 12/07/17 21:43, Hans Verkuil wrote:
>> On 12/07/17 21:02, Eric Anholt wrote:
>>>> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>>> +				      u32 signal_free_time, struct cec_msg *msg)
>>>> +{
>>>> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
>>>> +	u32 val;
>>>> +	unsigned int i;
>>>> +
>>>> +	for (i = 0; i < msg->len; i += 4)
>>>> +		HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
>>>> +			   (msg->msg[i]) |
>>>> +			   (msg->msg[i + 1] << 8) |
>>>> +			   (msg->msg[i + 2] << 16) |
>>>> +			   (msg->msg[i + 3] << 24));
>>>> +
>>>> +	val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>>>> +	val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
>>>> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
>>>> +	val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
>>>> +	val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
>>>> +	val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
>>>
>>> It doesn't look to me like len should have 1 subtracted from it.  The
>>> field has 4 bits for our up-to-16-byte length, and the firmware seems to
>>> be setting it to the same value as a memcpy for the message data uses.
>> 
>> You need to subtract by one. The CEC protocol supports messages of 1-16
>> bytes in length. Since the message length mask is only 4 bits you need to
>> encode this in the value 0-15. Hence the '-1', otherwise you would never
>> be able to send 16 byte messages.
>> 
>> I actually found this when debugging the messages it was transmitting: they
>> were one too long.
>> 
>> This suggests that the firmware does this wrong. I don't have time tomorrow,
>> but I'll see if I can do a quick test on Friday to verify that.
>
> I double-checked this and both the driver and the firmware do the right thing.
> Just to be certain I also tried sending a message that uses the full 16 byte
> payload and that too went well. So the code is definitely correct.

Great, I'll assume that I just missed the subtraction somewhere in the
layers of firmware code.  Thanks for checking!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]