[PATCH 00/15] drm/vc4: hdmi: Add CEC support for the BCM2711

[PATCH 00/15] drm/vc4: hdmi: Add CEC support for the BCM2711

[Maxime Ripard]

Hi,

Here's a series introducing the CEC support for the BCM2711 found on the
RaspberryPi4.

The BCM2711 HDMI controller uses a similar layout for the CEC registers, the
main difference being that the interrupt handling part is now shared between
both HDMI controllers.

This series is mainly about fixing a couple of bugs, reworking the driver to
support having two different interrupts, one for each direction, provided by an
external irqchip, and enables the irqchip driver for the controller we have.

This has been tested on an RPi3 and RPi4, but requires the latest firmware.
It's is based on the 10 and 12 bpc series.

Here is the cec-compliance output:

$ cec-ctl --tuner -p 1.0.0.0
The CEC adapter doesn't allow setting the physical address manually, ignore this option.

Driver Info:
	Driver Name                : vc4_hdmi
	Adapter Name               : vc4
	Capabilities               : 0x0000010e
		Logical Addresses
		Transmit
		Passthrough
	Driver version             : 5.10.0
	Available Logical Addresses: 1
	Physical Address           : 1.0.0.0
	Logical Address Mask       : 0x0008
	CEC Version                : 2.0
	Vendor ID                  : 0x000c03 (HDMI)
	OSD Name                   : Tuner
	Logical Addresses          : 1 (Allow RC Passthrough)

	  Logical Address          : 3 (Tuner 1)
	    Primary Device Type    : Tuner
	    Logical Address Type   : Tuner
	    All Device Types       : Tuner
	    RC TV Profile          : None
	    Device Features        :
		None

$ cec-compliance
cec-compliance SHA                 : not available
Driver Info:
	Driver Name                : vc4_hdmi
	Adapter Name               : vc4
	Capabilities               : 0x0000010e
		Logical Addresses
		Transmit
		Passthrough
	Driver version             : 5.10.0
	Available Logical Addresses: 1
	Physical Address           : 1.0.0.0
	Logical Address Mask       : 0x0008
	CEC Version                : 2.0
	Vendor ID                  : 0x000c03 (HDMI)
	OSD Name                   : Tuner
	Logical Addresses          : 1 (Allow RC Passthrough)

	  Logical Address          : 3 (Tuner 1)
	    Primary Device Type    : Tuner
	    Logical Address Type   : Tuner
	    All Device Types       : Tuner
	    RC TV Profile          : None
	    Device Features        :
		None

Compliance test for vc4_hdmi device /dev/cec0:

    The test results mean the following:
        OK                  Supported correctly by the device.
        OK (Not Supported)  Not supported and not mandatory for the device.
        OK (Presumed)       Presumably supported.  Manually check to confirm.
        OK (Unexpected)     Supported correctly but is not expected to be supported for this device.
        OK (Refused)        Supported by the device, but was refused.
        FAIL                Failed and was expected to be supported by this device.

Find remote devices:
	Polling: OK

Network topology:
	System Information for device 0 (TV) from device 3 (Tuner 1):
		CEC Version                : 2.0
		Physical Address           : 0.0.0.0
		Primary Device Type        : TV
		Vendor ID                  : 0x000c03 (HDMI)
		OSD Name                   : 'test-124'
		Power Status               : Tx, OK, Rx, OK, Feature Abort

Total for vc4_hdmi device /dev/cec0: 1, Succeeded: 1, Failed: 0, Warnings: 0

Let me know what you think,
Maxime

Dom Cobley (5):
  drm/vc4: hdmi: Move hdmi reset to bind
  drm/vc4: hdmi: Fix register offset with longer CEC messages
  drm/vc4: hdmi: Fix up CEC registers
  drm/vc4: hdmi: Restore cec physical address on reconnect
  drm/vc4: hdmi: Remove cec_available flag

Maxime Ripard (10):
  irqchip: Allow to compile bcmstb on other platforms
  drm/vc4: hdmi: Compute the CEC clock divider from the clock rate
  drm/vc4: hdmi: Update the CEC clock divider on HSM rate change
  drm/vc4: hdmi: Introduce a CEC clock
  drm/vc4: hdmi: Split the interrupt handlers
  drm/vc4: hdmi: Support BCM2711 CEC interrupt setup
  drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts
  dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts
  ARM: dts: bcm2711: Add the BSC interrupt controller
  ARM: dts: bcm2711: Add the CEC interrupt controller

 .../bindings/display/brcm,bcm2711-hdmi.yaml   |  20 +-
 arch/arm/boot/dts/bcm2711.dtsi                |  30 +++
 drivers/gpu/drm/vc4/vc4_hdmi.c                | 224 +++++++++++++-----
 drivers/gpu/drm/vc4/vc4_hdmi.h                |  11 +-
 drivers/gpu/drm/vc4/vc4_hdmi_regs.h           |   4 +-
 drivers/irqchip/Kconfig                       |   2 +-
 6 files changed, 232 insertions(+), 59 deletions(-)

-- 
2.28.0

[PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Maxime Ripard]

The BCM2711 uses a number of instances of the bcmstb-l2 controller in its
display engine. Let's allow the driver to be enabled through KConfig.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/irqchip/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c6098eee0c7c..f1e58de117dc 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -131,7 +131,7 @@ config BCM7120_L2_IRQ
 	select IRQ_DOMAIN
 
 config BRCMSTB_L2_IRQ
-	bool
+	bool "Broadcom STB L2 Interrupt Controller"
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
-- 
2.28.0

[PATCH 02/15] drm/vc4: hdmi: Move hdmi reset to bind

[Maxime Ripard]

From: Dom Cobley <popcornmix@gmail.com>

The hdmi reset got moved to a later point in the commit 9045e91a476b
("drm/vc4: hdmi: Add reset callback").

However, the reset now occurs after vc4_hdmi_cec_init and so tramples
the setup of registers like HDMI_CEC_CNTRL_1

This only affects pi0-3 as on pi4 the cec registers are in a separate
block

Fixes: 9045e91a476b ("drm/vc4: hdmi: Add reset callback")
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 8006bddc8fbb..3df1747dd917 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -773,9 +773,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 		return;
 	}
 
-	if (vc4_hdmi->variant->reset)
-		vc4_hdmi->variant->reset(vc4_hdmi);
-
 	if (vc4_hdmi->variant->phy_init)
 		vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state);
 
@@ -1865,6 +1862,9 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 	vc4_hdmi->disable_wifi_frequencies =
 		of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
 
+	if (vc4_hdmi->variant->reset)
+		vc4_hdmi->variant->reset(vc4_hdmi);
+
 	pm_runtime_enable(dev);
 
 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
-- 
2.28.0

[PATCH 03/15] drm/vc4: hdmi: Fix register offset with longer CEC messages

[Maxime Ripard]

From: Dom Cobley <popcornmix@gmail.com>

The code prior to 311e305fdb4e ("drm/vc4: hdmi: Implement a register
layout abstraction") was relying on the fact that the register offset
was incremented by 4 for each readl call. That worked since the register
width is 4 bytes.

However, since that commit the HDMI_READ macro is now taking an enum,
and the offset doesn't increment by 4 but 1 now. Divide the index by 4
to fix this.

Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction")
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 3df1747dd917..28b78ea885ea 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1434,13 +1434,20 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
 
 static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1)
 {
+	struct drm_device *dev = vc4_hdmi->connector.dev;
 	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);
+
+	if (msg->len > 16) {
+		drm_err(dev, "Attempting to read too much data (%d)\n", msg->len);
+		return;
+	}
+
 	for (i = 0; i < msg->len; i += 4) {
-		u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + i);
+		u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + (i >> 2));
 
 		msg->msg[i] = val & 0xff;
 		msg->msg[i + 1] = (val >> 8) & 0xff;
@@ -1533,11 +1540,17 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 				      u32 signal_free_time, struct cec_msg *msg)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+	struct drm_device *dev = vc4_hdmi->connector.dev;
 	u32 val;
 	unsigned int i;
 
+	if (msg->len > 16) {
+		drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len);
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < msg->len; i += 4)
-		HDMI_WRITE(HDMI_CEC_TX_DATA_1 + i,
+		HDMI_WRITE(HDMI_CEC_TX_DATA_1 + (i >> 2),
 			   (msg->msg[i]) |
 			   (msg->msg[i + 1] << 8) |
 			   (msg->msg[i + 2] << 16) |
-- 
2.28.0

[PATCH 04/15] drm/vc4: hdmi: Fix up CEC registers

[Maxime Ripard]

From: Dom Cobley <popcornmix@gmail.com>

The commit 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout
abstraction") forgot one CEC register, and made a copy and paste mistake
for another one. Fix those mistakes.

Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction")
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
index 013fd57febd8..20a1438a72cb 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
@@ -29,6 +29,7 @@ enum vc4_hdmi_field {
 	HDMI_CEC_CPU_MASK_SET,
 	HDMI_CEC_CPU_MASK_STATUS,
 	HDMI_CEC_CPU_STATUS,
+	HDMI_CEC_CPU_SET,
 
 	/*
 	 * Transmit data, first byte is low byte of the 32-bit reg.
@@ -199,9 +200,10 @@ static const struct vc4_hdmi_register vc4_hdmi_fields[] = {
 	VC4_HDMI_REG(HDMI_TX_PHY_RESET_CTL, 0x02c0),
 	VC4_HDMI_REG(HDMI_TX_PHY_CTL_0, 0x02c4),
 	VC4_HDMI_REG(HDMI_CEC_CPU_STATUS, 0x0340),
+	VC4_HDMI_REG(HDMI_CEC_CPU_SET, 0x0344),
 	VC4_HDMI_REG(HDMI_CEC_CPU_CLEAR, 0x0348),
 	VC4_HDMI_REG(HDMI_CEC_CPU_MASK_STATUS, 0x034c),
-	VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x034c),
+	VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x0350),
 	VC4_HDMI_REG(HDMI_CEC_CPU_MASK_CLEAR, 0x0354),
 	VC4_HDMI_REG(HDMI_RAM_PACKET_START, 0x0400),
 };
-- 
2.28.0

[PATCH 05/15] drm/vc4: hdmi: Restore cec physical address on reconnect

[Maxime Ripard]

From: Dom Cobley <popcornmix@gmail.com>

Currently we call cec_phys_addr_invalidate on a hotplug deassert.
That may be due to a TV power cycling, or an AVR being switched
on (and switching edid).

This makes CEC unusable since our controller wouldn't have a physical
address anymore.

Set it back up again on the hotplug assert.

Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support")
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 28b78ea885ea..eff3bac562c6 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -136,20 +136,29 @@ static enum drm_connector_status
 vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
+	bool connected = false;
 
 	if (vc4_hdmi->hpd_gpio) {
 		if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^
 		    vc4_hdmi->hpd_active_low)
-			return connector_status_connected;
-		cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
-		return connector_status_disconnected;
-	}
-
-	if (drm_probe_ddc(vc4_hdmi->ddc))
-		return connector_status_connected;
-
+			connected = true;
+	} else if (drm_probe_ddc(vc4_hdmi->ddc))
+		connected = true;
 	if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)
+		connected = true;
+	if (connected) {
+		if (connector->status != connector_status_connected) {
+			struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);
+
+			if (edid) {
+				cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
+				vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);
+				drm_connector_update_edid_property(connector, edid);
+				kfree(edid);
+			}
+		}
 		return connector_status_connected;
+	}
 	cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
 	return connector_status_disconnected;
 }
-- 
2.28.0

[PATCH 06/15] drm/vc4: hdmi: Compute the CEC clock divider from the clock rate

[Maxime Ripard]

The CEC clock divider needs to output a frequency of 40kHz from the HSM
rate on the BCM2835. The driver used to have a fixed frequency for it,
but that changed and we now need to compute it dynamically to maintain
the proper rate.

Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index eff3bac562c6..0c53d7427d15 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1586,6 +1586,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 {
 	struct cec_connector_info conn_info;
 	struct platform_device *pdev = vc4_hdmi->pdev;
+	u16 clk_cnt;
 	u32 value;
 	int ret;
 
@@ -1611,8 +1612,9 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 	 * divider: the hsm_clock rate and this divider setting will
 	 * give a 40 kHz CEC clock.
 	 */
+	clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
 	value |= VC4_HDMI_CEC_ADDR_MASK |
-		 (4091 << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
+		 (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
 	HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
 	ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0),
 					vc4_cec_irq_handler,
-- 
2.28.0

[PATCH 07/15] drm/vc4: hdmi: Update the CEC clock divider on HSM rate change

[Maxime Ripard]

As part of the enable sequence we might change the HSM clock rate if the
pixel rate is different than the one we were already dealing with.

On the BCM2835 however, the CEC clock derives from the HSM clock so any
rate change will need to be reflected in the CEC clock divider to output
40kHz.

Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 39 +++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 0c53d7427d15..b93ee3e26e2b 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -132,6 +132,27 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
 		   HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL);
 }
 
+#ifdef CONFIG_DRM_VC4_HDMI_CEC
+static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
+{
+	u16 clk_cnt;
+	u32 value;
+
+	value = HDMI_READ(HDMI_CEC_CNTRL_1);
+	value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
+
+	/*
+	 * Set the clock divider: the hsm_clock rate and this divider
+	 * setting will give a 40 kHz CEC clock.
+	 */
+	clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
+	value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;
+	HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
+}
+#else
+static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
+#endif
+
 static enum drm_connector_status
 vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -761,6 +782,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 		return;
 	}
 
+	vc4_hdmi_cec_update_clk_div(vc4_hdmi);
+
 	/*
 	 * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup
 	 * at 300MHz.
@@ -1586,7 +1609,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 {
 	struct cec_connector_info conn_info;
 	struct platform_device *pdev = vc4_hdmi->pdev;
-	u16 clk_cnt;
 	u32 value;
 	int ret;
 
@@ -1605,17 +1627,14 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 	cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
 
 	HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff);
+
 	value = HDMI_READ(HDMI_CEC_CNTRL_1);
-	value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
-	/*
-	 * Set the logical address to Unregistered and set the clock
-	 * divider: the hsm_clock rate and this divider setting will
-	 * give a 40 kHz CEC clock.
-	 */
-	clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
-	value |= VC4_HDMI_CEC_ADDR_MASK |
-		 (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
+	/* Set the logical address to Unregistered */
+	value |= VC4_HDMI_CEC_ADDR_MASK;
 	HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
+
+	vc4_hdmi_cec_update_clk_div(vc4_hdmi);
+
 	ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0),
 					vc4_cec_irq_handler,
 					vc4_cec_irq_handler_thread, 0,
-- 
2.28.0

[PATCH 08/15] drm/vc4: hdmi: Introduce a CEC clock

[Maxime Ripard]

While the BCM2835 had the CEC clock derived from the HSM clock, the
BCM2711 has a dedicated parent clock for it.

Let's introduce a separate clock for it so that we can handle both
cases.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++-
 drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index b93ee3e26e2b..0debd22bc992 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
 	 * Set the clock divider: the hsm_clock rate and this divider
 	 * setting will give a 40 kHz CEC clock.
 	 */
-	clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
+	clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ;
 	value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;
 	HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
 }
@@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
 		return PTR_ERR(vc4_hdmi->hsm_clock);
 	}
 	vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
+	vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock;
 
 	return 0;
 }
@@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
 		return PTR_ERR(vc4_hdmi->audio_clock);
 	}
 
+	vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");
+	if (IS_ERR(vc4_hdmi->cec_clock)) {
+		DRM_ERROR("Failed to get CEC clock\n");
+		return PTR_ERR(vc4_hdmi->cec_clock);
+	}
+
 	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
 	if (IS_ERR(vc4_hdmi->reset)) {
 		DRM_ERROR("Failed to get HDMI reset line\n");
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 720914761261..adc4bf33ff15 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -155,6 +155,7 @@ struct vc4_hdmi {
 	bool cec_tx_ok;
 	bool cec_irq_was_rx;
 
+	struct clk *cec_clock;
 	struct clk *pixel_clock;
 	struct clk *hsm_clock;
 	struct clk *audio_clock;
-- 
2.28.0

[PATCH 09/15] drm/vc4: hdmi: Split the interrupt handlers

[Maxime Ripard]

The BCM2711 has two different interrupt sources to transmit and receive
CEC messages, provided through an external interrupt chip shared between
the two HDMI interrupt controllers.

The rest of the CEC controller is identical though so we need to change
a bit the code organisation to share the code as much as possible, yet
still allowing to register independant handlers.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 86 +++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 0debd22bc992..80a81fcea315 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1442,15 +1442,22 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
 }
 
 #ifdef CONFIG_DRM_VC4_HDMI_CEC
-static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
+static irqreturn_t vc4_cec_irq_handler_rx_thread(int irq, void *priv)
 {
 	struct vc4_hdmi *vc4_hdmi = priv;
 
-	if (vc4_hdmi->cec_irq_was_rx) {
-		if (vc4_hdmi->cec_rx_msg.len)
-			cec_received_msg(vc4_hdmi->cec_adap,
-					 &vc4_hdmi->cec_rx_msg);
-	} else if (vc4_hdmi->cec_tx_ok) {
+	if (vc4_hdmi->cec_rx_msg.len)
+		cec_received_msg(vc4_hdmi->cec_adap,
+				 &vc4_hdmi->cec_rx_msg);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t vc4_cec_irq_handler_tx_thread(int irq, void *priv)
+{
+	struct vc4_hdmi *vc4_hdmi = priv;
+
+	if (vc4_hdmi->cec_tx_ok) {
 		cec_transmit_done(vc4_hdmi->cec_adap, CEC_TX_STATUS_OK,
 				  0, 0, 0, 0);
 	} else {
@@ -1464,6 +1471,19 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
+{
+	struct vc4_hdmi *vc4_hdmi = priv;
+	irqreturn_t ret;
+
+	if (vc4_hdmi->cec_irq_was_rx)
+		ret = vc4_cec_irq_handler_rx_thread(irq, priv);
+	else
+		ret = vc4_cec_irq_handler_tx_thread(irq, priv);
+
+	return ret;
+}
+
 static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1)
 {
 	struct drm_device *dev = vc4_hdmi->connector.dev;
@@ -1488,31 +1508,55 @@ static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1)
 	}
 }
 
+static irqreturn_t vc4_cec_irq_handler_tx_bare(int irq, void *priv)
+{
+	struct vc4_hdmi *vc4_hdmi = priv;
+	u32 cntrl1;
+
+	cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1);
+	vc4_hdmi->cec_tx_ok = cntrl1 & VC4_HDMI_CEC_TX_STATUS_GOOD;
+	cntrl1 &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
+	HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t vc4_cec_irq_handler_rx_bare(int irq, void *priv)
+{
+	struct vc4_hdmi *vc4_hdmi = priv;
+	u32 cntrl1;
+
+	vc4_hdmi->cec_rx_msg.len = 0;
+	cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1);
+	vc4_cec_read_msg(vc4_hdmi, cntrl1);
+	cntrl1 |= VC4_HDMI_CEC_CLEAR_RECEIVE_OFF;
+	HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1);
+	cntrl1 &= ~VC4_HDMI_CEC_CLEAR_RECEIVE_OFF;
+
+	HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1);
+
+	return IRQ_WAKE_THREAD;
+}
+
 static irqreturn_t vc4_cec_irq_handler(int irq, void *priv)
 {
 	struct vc4_hdmi *vc4_hdmi = priv;
 	u32 stat = HDMI_READ(HDMI_CEC_CPU_STATUS);
-	u32 cntrl1, cntrl5;
+	irqreturn_t ret;
+	u32 cntrl5;
 
 	if (!(stat & VC4_HDMI_CPU_CEC))
 		return IRQ_NONE;
-	vc4_hdmi->cec_rx_msg.len = 0;
-	cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1);
+
 	cntrl5 = HDMI_READ(HDMI_CEC_CNTRL_5);
 	vc4_hdmi->cec_irq_was_rx = cntrl5 & VC4_HDMI_CEC_RX_CEC_INT;
-	if (vc4_hdmi->cec_irq_was_rx) {
-		vc4_cec_read_msg(vc4_hdmi, cntrl1);
-		cntrl1 |= VC4_HDMI_CEC_CLEAR_RECEIVE_OFF;
-		HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1);
-		cntrl1 &= ~VC4_HDMI_CEC_CLEAR_RECEIVE_OFF;
-	} else {
-		vc4_hdmi->cec_tx_ok = cntrl1 & VC4_HDMI_CEC_TX_STATUS_GOOD;
-		cntrl1 &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
-	}
-	HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1);
+	if (vc4_hdmi->cec_irq_was_rx)
+		ret = vc4_cec_irq_handler_rx_bare(irq, priv);
+	else
+		ret = vc4_cec_irq_handler_tx_bare(irq, priv);
+
 	HDMI_WRITE(HDMI_CEC_CPU_CLEAR, VC4_HDMI_CPU_CEC);
-
-	return IRQ_WAKE_THREAD;
+	return ret;
 }
 
 static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
-- 
2.28.0

[PATCH 10/15] drm/vc4: hdmi: Support BCM2711 CEC interrupt setup

[Maxime Ripard]

The HDMI controller found in the BCM2711 has an external interrupt
controller for the CEC and hotplug interrupt shared between the two
instances.

Let's add a variant flag to register a single interrupt handler and
deals with the interrupt handler setup, or two interrupt handlers
relying on an external irqchip.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 42 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  7 ++++++
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 80a81fcea315..d208b7d1d937 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1593,9 +1593,11 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
 			   ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
 			   ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
 
-		HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
+		if (!vc4_hdmi->variant->external_irq_controller)
+			HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
 	} else {
-		HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
+		if (!vc4_hdmi->variant->external_irq_controller)
+			HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
 		HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
 			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
 	}
@@ -1670,8 +1672,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 	cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector);
 	cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
 
-	HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff);
-
 	value = HDMI_READ(HDMI_CEC_CNTRL_1);
 	/* Set the logical address to Unregistered */
 	value |= VC4_HDMI_CEC_ADDR_MASK;
@@ -1679,12 +1679,32 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 
 	vc4_hdmi_cec_update_clk_div(vc4_hdmi);
 
-	ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0),
-					vc4_cec_irq_handler,
-					vc4_cec_irq_handler_thread, 0,
-					"vc4 hdmi cec", vc4_hdmi);
-	if (ret)
-		goto err_delete_cec_adap;
+	if (vc4_hdmi->variant->external_irq_controller) {
+		ret = devm_request_threaded_irq(&pdev->dev,
+						platform_get_irq_byname(pdev, "cec-rx"),
+						vc4_cec_irq_handler_rx_bare,
+						vc4_cec_irq_handler_rx_thread, 0,
+						"vc4 hdmi cec rx", vc4_hdmi);
+		if (ret)
+			goto err_delete_cec_adap;
+
+		ret = devm_request_threaded_irq(&pdev->dev,
+						platform_get_irq_byname(pdev, "cec-tx"),
+						vc4_cec_irq_handler_tx_bare,
+						vc4_cec_irq_handler_tx_thread, 0,
+						"vc4 hdmi cec tx", vc4_hdmi);
+		if (ret)
+			goto err_delete_cec_adap;
+	} else {
+		HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff);
+
+		ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0),
+						vc4_cec_irq_handler,
+						vc4_cec_irq_handler_thread, 0,
+						"vc4 hdmi cec", vc4_hdmi);
+		if (ret)
+			goto err_delete_cec_adap;
+	}
 
 	ret = cec_register_adapter(vc4_hdmi->cec_adap, &pdev->dev);
 	if (ret < 0)
@@ -2083,6 +2103,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = {
 		PHY_LANE_CK,
 	},
 	.unsupported_odd_h_timings	= true,
+	.external_irq_controller	= true,
 
 	.init_resources		= vc5_hdmi_init_resources,
 	.csc_setup		= vc5_hdmi_csc_setup,
@@ -2109,6 +2130,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi1_variant = {
 		PHY_LANE_2,
 	},
 	.unsupported_odd_h_timings	= true,
+	.external_irq_controller	= true,
 
 	.init_resources		= vc5_hdmi_init_resources,
 	.csc_setup		= vc5_hdmi_csc_setup,
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index adc4bf33ff15..27352827f70c 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -64,6 +64,13 @@ struct vc4_hdmi_variant {
 	/* The BCM2711 cannot deal with odd horizontal pixel timings */
 	bool unsupported_odd_h_timings;
 
+	/*
+	 * The BCM2711 CEC/hotplug IRQ controller is shared between the
+	 * two HDMI controllers, and we have a proper irqchip driver for
+	 * it.
+	 */
+	bool external_irq_controller;
+
 	/* Callback to get the resources (memory region, interrupts,
 	 * clocks, etc) for that variant.
 	 */
-- 
2.28.0

[PATCH 11/15] drm/vc4: hdmi: Remove cec_available flag

[Maxime Ripard]

From: Dom Cobley <popcornmix@gmail.com>

Now that our HDMI controller supports CEC for the BCM2711, let's remove
that flag.

Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ----
 drivers/gpu/drm/vc4/vc4_hdmi.h | 3 ---
 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d208b7d1d937..327638d93032 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1658,9 +1658,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 	u32 value;
 	int ret;
 
-	if (!vc4_hdmi->variant->cec_available)
-		return 0;
-
 	vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops,
 						  vc4_hdmi, "vc4",
 						  CEC_CAP_DEFAULTS |
@@ -2074,7 +2071,6 @@ static const struct vc4_hdmi_variant bcm2835_variant = {
 	.debugfs_name		= "hdmi_regs",
 	.card_name		= "vc4-hdmi",
 	.max_pixel_clock	= 162000000,
-	.cec_available		= true,
 	.registers		= vc4_hdmi_fields,
 	.num_registers		= ARRAY_SIZE(vc4_hdmi_fields),
 
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 27352827f70c..c93ada62f429 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -42,9 +42,6 @@ struct vc4_hdmi_variant {
 	/* Filename to expose the registers in debugfs */
 	const char *debugfs_name;
 
-	/* Set to true when the CEC support is available */
-	bool cec_available;
-
 	/* Maximum pixel clock supported by the controller (in Hz) */
 	unsigned long long max_pixel_clock;
 
-- 
2.28.0

[PATCH 12/15] drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts

[Maxime Ripard]

We introduced the BCM2711 support to the vc4 HDMI controller with 5.10,
but this was lacking any of the interrupts of the CEC controller so we
have to deal with the backward compatibility.

Do so by simply ignoring the CEC setup if the DT doesn't have the
interrupts property.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 327638d93032..69217c68d3a4 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1655,9 +1655,15 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 {
 	struct cec_connector_info conn_info;
 	struct platform_device *pdev = vc4_hdmi->pdev;
+	struct device *dev = &pdev->dev;
 	u32 value;
 	int ret;
 
+	if (!of_find_property(dev->of_node, "interrupts", NULL)) {
+		dev_warn(dev, "'interrupts' DT property is missing, no CEC\n");
+		return 0;
+	}
+
 	vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops,
 						  vc4_hdmi, "vc4",
 						  CEC_CAP_DEFAULTS |
-- 
2.28.0

[PATCH 13/15] dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts

[Maxime Ripard]

The CEC and hotplug interrupts were missing when that binding was
introduced, let's add them in now that we've figured out how it works.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 .../bindings/display/brcm,bcm2711-hdmi.yaml   | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
index 7ce06f9f9f8e..6e8ac910bdd8 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
@@ -53,6 +53,24 @@ properties:
       - const: audio
       - const: cec
 
+  interrupts:
+    items:
+      - description: CEC TX interrupt
+      - description: CEC RX interrupt
+      - description: CEC stuck at low interrupt
+      - description: Wake-up interrupt
+      - description: Hotplug connected interrupt
+      - description: Hotplug removed interrupt
+
+  interrupt-names:
+    items:
+      - const: cec-tx
+      - const: cec-rx
+      - const: cec-low
+      - const: wakeup
+      - const: hpd-connected
+      - const: hpd-removed
+
   ddc:
     allOf:
       - $ref: /schemas/types.yaml#/definitions/phandle
@@ -90,7 +108,7 @@ required:
   - resets
   - ddc
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
-- 
2.28.0

[PATCH 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller

[Maxime Ripard]

The BSC controllers used for the HDMI DDC have an interrupt controller
shared between both instances. Let's add it to avoid polling.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 arch/arm/boot/dts/bcm2711.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 4847dd305317..8bb46ae76a92 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -308,6 +308,14 @@ dvp: clock@7ef00000 {
 			#reset-cells = <1>;
 		};
 
+		bsc_intr: interrupt-controller@7ef00040 {
+			compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc";
+			reg = <0x7ef00040 0x30>;
+			interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		hdmi0: hdmi@7ef00700 {
 			compatible = "brcm,bcm2711-hdmi0";
 			reg = <0x7ef00700 0x300>,
@@ -341,6 +349,8 @@ ddc0: i2c@7ef04500 {
 			reg = <0x7ef04500 0x100>, <0x7ef00b00 0x300>;
 			reg-names = "bsc", "auto-i2c";
 			clock-frequency = <97500>;
+			interrupt-parent = <&bsc_intr>;
+			interrupts = <0>;
 			status = "disabled";
 		};
 
@@ -377,6 +387,8 @@ ddc1: i2c@7ef09500 {
 			reg = <0x7ef09500 0x100>, <0x7ef05b00 0x300>;
 			reg-names = "bsc", "auto-i2c";
 			clock-frequency = <97500>;
+			interrupt-parent = <&bsc_intr>;
+			interrupts = <1>;
 			status = "disabled";
 		};
 	};
-- 
2.28.0

[PATCH 15/15] ARM: dts: bcm2711: Add the CEC interrupt controller

[Maxime Ripard]

The CEC and hotplug interrupts go through an interrupt controller shared
between the two HDMI controllers.

Let's add that interrupt controller and the interrupts for both HDMI
controllers

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 arch/arm/boot/dts/bcm2711.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 8bb46ae76a92..154cf6d35333 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -316,6 +316,14 @@ bsc_intr: interrupt-controller@7ef00040 {
 			#interrupt-cells = <1>;
 		};
 
+		aon_intr: interrupt-controller@7ef00100 {
+			compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc";
+			reg = <0x7ef00100 0x30>;
+			interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		hdmi0: hdmi@7ef00700 {
 			compatible = "brcm,bcm2711-hdmi0";
 			reg = <0x7ef00700 0x300>,
@@ -338,6 +346,11 @@ hdmi0: hdmi@7ef00700 {
 				    "hd";
 			clock-names = "hdmi", "bvb", "audio", "cec";
 			resets = <&dvp 0>;
+			interrupt-parent = <&aon_intr>;
+			interrupts = <0>, <1>, <2>,
+				     <3>, <4>, <5>;
+			interrupt-names = "cec-tx", "cec-rx", "cec-low",
+					  "wakeup", "hpd-connected", "hpd-removed";
 			ddc = <&ddc0>;
 			dmas = <&dma 10>;
 			dma-names = "audio-rx";
@@ -377,6 +390,11 @@ hdmi1: hdmi@7ef05700 {
 			ddc = <&ddc1>;
 			clock-names = "hdmi", "bvb", "audio", "cec";
 			resets = <&dvp 1>;
+			interrupt-parent = <&aon_intr>;
+			interrupts = <6>, <7>, <8>,
+				     <9>, <10>, <11>;
+			interrupt-names = "cec-tx", "cec-rx", "cec-low",
+					  "wakeup", "hpd-connected", "hpd-removed";
 			dmas = <&dma 17>;
 			dma-names = "audio-rx";
 			status = "disabled";
-- 
2.28.0

Re: [PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Florian Fainelli]

On 12/10/2020 5:46 AM, Maxime Ripard wrote:
> The BCM2711 uses a number of instances of the bcmstb-l2 controller in its
> display engine. Let's allow the driver to be enabled through KConfig.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller

[Florian Fainelli]

On 12/10/2020 5:46 AM, Maxime Ripard wrote:
> The BSC controllers used for the HDMI DDC have an interrupt controller
> shared between both instances. Let's add it to avoid polling.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 15/15] ARM: dts: bcm2711: Add the CEC interrupt controller

[Florian Fainelli]

On 12/10/2020 5:46 AM, Maxime Ripard wrote:
> The CEC and hotplug interrupts go through an interrupt controller shared
> between the two HDMI controllers.
> 
> Let's add that interrupt controller and the interrupts for both HDMI
> controllers
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Marc Zyngier]

Hi Maxime,

On 2020-12-10 13:46, Maxime Ripard wrote:
> The BCM2711 uses a number of instances of the bcmstb-l2 controller in 
> its
> display engine. Let's allow the driver to be enabled through KConfig.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/irqchip/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index c6098eee0c7c..f1e58de117dc 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -131,7 +131,7 @@ config BCM7120_L2_IRQ
>  	select IRQ_DOMAIN
> 
>  config BRCMSTB_L2_IRQ
> -	bool
> +	bool "Broadcom STB L2 Interrupt Controller"
>  	select GENERIC_IRQ_CHIP
>  	select IRQ_DOMAIN

I'm always sceptical of making interrupt controllers user-selectable.
Who is going to know that they need to pick that one?

I'd be much more in favour of directly selecting this symbol
from DRM_VC4_HDMI_CEC, since there is an obvious dependency.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Maxime Ripard]

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

Hi Marc,

On Thu, Dec 10, 2020 at 05:59:09PM +0000, Marc Zyngier wrote:
> On 2020-12-10 13:46, Maxime Ripard wrote:
> > The BCM2711 uses a number of instances of the bcmstb-l2 controller in
> > its
> > display engine. Let's allow the driver to be enabled through KConfig.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/irqchip/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index c6098eee0c7c..f1e58de117dc 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -131,7 +131,7 @@ config BCM7120_L2_IRQ
> >  	select IRQ_DOMAIN
> > 
> >  config BRCMSTB_L2_IRQ
> > -	bool
> > +	bool "Broadcom STB L2 Interrupt Controller"
> >  	select GENERIC_IRQ_CHIP
> >  	select IRQ_DOMAIN
> 
> I'm always sceptical of making interrupt controllers user-selectable.
> Who is going to know that they need to pick that one?
> 
> I'd be much more in favour of directly selecting this symbol
> from DRM_VC4_HDMI_CEC, since there is an obvious dependency.

It's a bit weird to me that the HDMI CEC support selects it, since that
interrupt controller is external and here no matter what. Would
selecting it from the ARCH_* Kconfig option work for you?

Thanks!
Maxime

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

Re: [PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Marc Zyngier]

Hi Maxime,

On 2020-12-14 15:27, Maxime Ripard wrote:
> Hi Marc,
> 
> On Thu, Dec 10, 2020 at 05:59:09PM +0000, Marc Zyngier wrote:

[...]

>> I'm always sceptical of making interrupt controllers user-selectable.
>> Who is going to know that they need to pick that one?
>> 
>> I'd be much more in favour of directly selecting this symbol
>> from DRM_VC4_HDMI_CEC, since there is an obvious dependency.
> 
> It's a bit weird to me that the HDMI CEC support selects it, since that
> interrupt controller is external and here no matter what.

 From glancing at the series, I was under the impression that these
controllers were there for the sole benefit of the HDMI controllers.
Is there anything else connected to them?

> Would selecting it from the ARCH_* Kconfig option work for you?

Sure. My only ask is that the low level plumbing is selected without
requiring any user guesswork.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 03/15] drm/vc4: hdmi: Fix register offset with longer CEC messages

[Dave Stevenson]

Hi Dom & Maxime

On Thu, 10 Dec 2020 at 13:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> From: Dom Cobley <popcornmix@gmail.com>
>
> The code prior to 311e305fdb4e ("drm/vc4: hdmi: Implement a register
> layout abstraction") was relying on the fact that the register offset
> was incremented by 4 for each readl call. That worked since the register
> width is 4 bytes.
>
> However, since that commit the HDMI_READ macro is now taking an enum,
> and the offset doesn't increment by 4 but 1 now. Divide the index by 4
> to fix this.
>
> Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction")
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 3df1747dd917..28b78ea885ea 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1434,13 +1434,20 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
>
>  static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1)
>  {
> +       struct drm_device *dev = vc4_hdmi->connector.dev;
>         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);
> +
> +       if (msg->len > 16) {
> +               drm_err(dev, "Attempting to read too much data (%d)\n", msg->len);
> +               return;
> +       }
> +
>         for (i = 0; i < msg->len; i += 4) {
> -               u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + i);
> +               u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + (i >> 2));
>
>                 msg->msg[i] = val & 0xff;
>                 msg->msg[i + 1] = (val >> 8) & 0xff;
> @@ -1533,11 +1540,17 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>                                       u32 signal_free_time, struct cec_msg *msg)
>  {
>         struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
> +       struct drm_device *dev = vc4_hdmi->connector.dev;
>         u32 val;
>         unsigned int i;
>
> +       if (msg->len > 16) {
> +               drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len);
> +               return -ENOMEM;
> +       }
> +
>         for (i = 0; i < msg->len; i += 4)
> -               HDMI_WRITE(HDMI_CEC_TX_DATA_1 + i,
> +               HDMI_WRITE(HDMI_CEC_TX_DATA_1 + (i >> 2),
>                            (msg->msg[i]) |
>                            (msg->msg[i + 1] << 8) |
>                            (msg->msg[i + 2] << 16) |
> --
> 2.28.0
>

Re: [PATCH 00/15] drm/vc4: hdmi: Add CEC support for the BCM2711

[Hans Verkuil]

Hi Maxime,

On 10/12/2020 14:46, Maxime Ripard wrote:
> Hi,
> 
> Here's a series introducing the CEC support for the BCM2711 found on the
> RaspberryPi4.
> 
> The BCM2711 HDMI controller uses a similar layout for the CEC registers, the
> main difference being that the interrupt handling part is now shared between
> both HDMI controllers.
> 
> This series is mainly about fixing a couple of bugs, reworking the driver to
> support having two different interrupts, one for each direction, provided by an
> external irqchip, and enables the irqchip driver for the controller we have.
> 
> This has been tested on an RPi3 and RPi4, but requires the latest firmware.
> It's is based on the 10 and 12 bpc series.

This series looks good to me. Before I give my Acked-by for this series, can you
confirm that it is possible to transmit the Image View On message on both outputs
of the RPi4 when the HPD is low?

See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt
on how to test this with a Pulse-Eight device.

This should work.

Regards,

	Hans

> 
> Here is the cec-compliance output:
> 
> $ cec-ctl --tuner -p 1.0.0.0
> The CEC adapter doesn't allow setting the physical address manually, ignore this option.
> 
> Driver Info:
> 	Driver Name                : vc4_hdmi
> 	Adapter Name               : vc4
> 	Capabilities               : 0x0000010e
> 		Logical Addresses
> 		Transmit
> 		Passthrough
> 	Driver version             : 5.10.0
> 	Available Logical Addresses: 1
> 	Physical Address           : 1.0.0.0
> 	Logical Address Mask       : 0x0008
> 	CEC Version                : 2.0
> 	Vendor ID                  : 0x000c03 (HDMI)
> 	OSD Name                   : Tuner
> 	Logical Addresses          : 1 (Allow RC Passthrough)
> 
> 	  Logical Address          : 3 (Tuner 1)
> 	    Primary Device Type    : Tuner
> 	    Logical Address Type   : Tuner
> 	    All Device Types       : Tuner
> 	    RC TV Profile          : None
> 	    Device Features        :
> 		None
> 
> $ cec-compliance
> cec-compliance SHA                 : not available
> Driver Info:
> 	Driver Name                : vc4_hdmi
> 	Adapter Name               : vc4
> 	Capabilities               : 0x0000010e
> 		Logical Addresses
> 		Transmit
> 		Passthrough
> 	Driver version             : 5.10.0
> 	Available Logical Addresses: 1
> 	Physical Address           : 1.0.0.0
> 	Logical Address Mask       : 0x0008
> 	CEC Version                : 2.0
> 	Vendor ID                  : 0x000c03 (HDMI)
> 	OSD Name                   : Tuner
> 	Logical Addresses          : 1 (Allow RC Passthrough)
> 
> 	  Logical Address          : 3 (Tuner 1)
> 	    Primary Device Type    : Tuner
> 	    Logical Address Type   : Tuner
> 	    All Device Types       : Tuner
> 	    RC TV Profile          : None
> 	    Device Features        :
> 		None
> 
> Compliance test for vc4_hdmi device /dev/cec0:
> 
>     The test results mean the following:
>         OK                  Supported correctly by the device.
>         OK (Not Supported)  Not supported and not mandatory for the device.
>         OK (Presumed)       Presumably supported.  Manually check to confirm.
>         OK (Unexpected)     Supported correctly but is not expected to be supported for this device.
>         OK (Refused)        Supported by the device, but was refused.
>         FAIL                Failed and was expected to be supported by this device.
> 
> Find remote devices:
> 	Polling: OK
> 
> Network topology:
> 	System Information for device 0 (TV) from device 3 (Tuner 1):
> 		CEC Version                : 2.0
> 		Physical Address           : 0.0.0.0
> 		Primary Device Type        : TV
> 		Vendor ID                  : 0x000c03 (HDMI)
> 		OSD Name                   : 'test-124'
> 		Power Status               : Tx, OK, Rx, OK, Feature Abort
> 
> Total for vc4_hdmi device /dev/cec0: 1, Succeeded: 1, Failed: 0, Warnings: 0
> 
> Let me know what you think,
> Maxime
> 
> Dom Cobley (5):
>   drm/vc4: hdmi: Move hdmi reset to bind
>   drm/vc4: hdmi: Fix register offset with longer CEC messages
>   drm/vc4: hdmi: Fix up CEC registers
>   drm/vc4: hdmi: Restore cec physical address on reconnect
>   drm/vc4: hdmi: Remove cec_available flag
> 
> Maxime Ripard (10):
>   irqchip: Allow to compile bcmstb on other platforms
>   drm/vc4: hdmi: Compute the CEC clock divider from the clock rate
>   drm/vc4: hdmi: Update the CEC clock divider on HSM rate change
>   drm/vc4: hdmi: Introduce a CEC clock
>   drm/vc4: hdmi: Split the interrupt handlers
>   drm/vc4: hdmi: Support BCM2711 CEC interrupt setup
>   drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts
>   dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts
>   ARM: dts: bcm2711: Add the BSC interrupt controller
>   ARM: dts: bcm2711: Add the CEC interrupt controller
> 
>  .../bindings/display/brcm,bcm2711-hdmi.yaml   |  20 +-
>  arch/arm/boot/dts/bcm2711.dtsi                |  30 +++
>  drivers/gpu/drm/vc4/vc4_hdmi.c                | 224 +++++++++++++-----
>  drivers/gpu/drm/vc4/vc4_hdmi.h                |  11 +-
>  drivers/gpu/drm/vc4/vc4_hdmi_regs.h           |   4 +-
>  drivers/irqchip/Kconfig                       |   2 +-
>  6 files changed, 232 insertions(+), 59 deletions(-)
> 

Re: [PATCH 00/15] drm/vc4: hdmi: Add CEC support for the BCM2711

[Maxime Ripard]

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

Hi Hans,

On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote:
> Hi Maxime,
> 
> On 10/12/2020 14:46, Maxime Ripard wrote:
> > Hi,
> > 
> > Here's a series introducing the CEC support for the BCM2711 found on the
> > RaspberryPi4.
> > 
> > The BCM2711 HDMI controller uses a similar layout for the CEC registers, the
> > main difference being that the interrupt handling part is now shared between
> > both HDMI controllers.
> > 
> > This series is mainly about fixing a couple of bugs, reworking the driver to
> > support having two different interrupts, one for each direction, provided by an
> > external irqchip, and enables the irqchip driver for the controller we have.
> > 
> > This has been tested on an RPi3 and RPi4, but requires the latest firmware.
> > It's is based on the 10 and 12 bpc series.
> 
> This series looks good to me. Before I give my Acked-by for this series, can you
> confirm that it is possible to transmit the Image View On message on both outputs
> of the RPi4 when the HPD is low?
> 
> See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt
> on how to test this with a Pulse-Eight device.
> 
> This should work.

This is the output on the RPi4:

# cec-ctl --playback
Driver Info:
	Driver Name                : vc4_hdmi
	Adapter Name               : vc4
	Capabilities               : 0x0000010e
		Logical Addresses
		Transmit
		Passthrough
	Driver version             : 5.10.0
	Available Logical Addresses: 1
	Physical Address           : f.f.f.f
	Logical Address Mask       : 0x0000
	CEC Version                : 2.0
	Vendor ID                  : 0x000c03 (HDMI)
	OSD Name                   : Playback
	Logical Addresses          : 1 (Allow RC Passthrough)

	  Logical Address          : Not Allocated
	    Primary Device Type    : Playback
	    Logical Address Type   : Playback
	    All Device Types       : Playback
	    RC TV Profile          : None
	    Device Features        :
		None

# cec-ctl -t0 --image-view-on
Driver Info:
	Driver Name                : vc4_hdmi
	Adapter Name               : vc4
	Capabilities               : 0x0000010e
		Logical Addresses
		Transmit
		Passthrough
	Driver version             : 5.10.0
	Available Logical Addresses: 1
	Physical Address           : f.f.f.f
	Logical Address Mask       : 0x0000
	CEC Version                : 2.0
	Vendor ID                  : 0x000c03 (HDMI)
	OSD Name                   : Playback
	Logical Addresses          : 1 (Allow RC Passthrough)

	  Logical Address          : Not Allocated
	    Primary Device Type    : Playback
	    Logical Address Type   : Playback
	    All Device Types       : Playback
	    RC TV Profile          : None
	    Device Features        :
		None


Transmit from Unregistered to TV (15 to 0):
CEC_MSG_IMAGE_VIEW_ON (0x04)
	Sequence: 1 Tx Timestamp: 77.631s


And this is the output on my desktop with the Pulse-Eight:
$ sudo cec-ctl -p0.0.0.0 --tv
Driver Info:
	Driver Name                : pulse8-cec
	Adapter Name               : serio0
	Capabilities               : 0x0000003f
		Physical Address
		Logical Addresses
		Transmit
		Passthrough
		Remote Control Support
		Monitor All
	Driver version             : 5.9.8
	Available Logical Addresses: 1
	Connector Info             : None
	Physical Address           : 0.0.0.0
	Logical Address Mask       : 0x0001
	CEC Version                : 2.0
	Vendor ID                  : 0x000c03 (HDMI)
	OSD Name                   : 'TV  '
	Logical Addresses          : 1 (Allow RC Passthrough)

	  Logical Address          : 0 (TV)
	    Primary Device Type    : TV
	    Logical Address Type   : TV
	    All Device Types       : TV
	    RC TV Profile          : None
	    Device Features        :
		None

$ sudo cec-ctl -M
Driver Info:
	Driver Name                : pulse8-cec
	Adapter Name               : serio0
	Capabilities               : 0x0000003f
		Physical Address
		Logical Addresses
		Transmit
		Passthrough
		Remote Control Support
		Monitor All
	Driver version             : 5.9.8
	Available Logical Addresses: 1
	Connector Info             : None
	Physical Address           : 0.0.0.0
	Logical Address Mask       : 0x0001
	CEC Version                : 2.0
	Vendor ID                  : 0x000c03 (HDMI)
	OSD Name                   : 'TV  '
	Logical Addresses          : 1 (Allow RC Passthrough)

	  Logical Address          : 0 (TV)
	    Primary Device Type    : TV
	    Logical Address Type   : TV
	    All Device Types       : TV
	    RC TV Profile          : None
	    Device Features        :
		None



Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no
Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04)

So it looks like it's working as expected?

Maxime

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

Re: [PATCH 00/15] drm/vc4: hdmi: Add CEC support for the BCM2711

[Hans Verkuil]

On 17/12/2020 11:49, Maxime Ripard wrote:
> Hi Hans,
> 
> On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote:
>> Hi Maxime,
>>
>> On 10/12/2020 14:46, Maxime Ripard wrote:
>>> Hi,
>>>
>>> Here's a series introducing the CEC support for the BCM2711 found on the
>>> RaspberryPi4.
>>>
>>> The BCM2711 HDMI controller uses a similar layout for the CEC registers, the
>>> main difference being that the interrupt handling part is now shared between
>>> both HDMI controllers.
>>>
>>> This series is mainly about fixing a couple of bugs, reworking the driver to
>>> support having two different interrupts, one for each direction, provided by an
>>> external irqchip, and enables the irqchip driver for the controller we have.
>>>
>>> This has been tested on an RPi3 and RPi4, but requires the latest firmware.
>>> It's is based on the 10 and 12 bpc series.
>>
>> This series looks good to me. Before I give my Acked-by for this series, can you
>> confirm that it is possible to transmit the Image View On message on both outputs
>> of the RPi4 when the HPD is low?
>>
>> See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt
>> on how to test this with a Pulse-Eight device.
>>
>> This should work.
> 
> This is the output on the RPi4:
> 
> # cec-ctl --playback
> Driver Info:
> 	Driver Name                : vc4_hdmi
> 	Adapter Name               : vc4
> 	Capabilities               : 0x0000010e
> 		Logical Addresses
> 		Transmit
> 		Passthrough
> 	Driver version             : 5.10.0
> 	Available Logical Addresses: 1
> 	Physical Address           : f.f.f.f
> 	Logical Address Mask       : 0x0000
> 	CEC Version                : 2.0
> 	Vendor ID                  : 0x000c03 (HDMI)
> 	OSD Name                   : Playback
> 	Logical Addresses          : 1 (Allow RC Passthrough)
> 
> 	  Logical Address          : Not Allocated
> 	    Primary Device Type    : Playback
> 	    Logical Address Type   : Playback
> 	    All Device Types       : Playback
> 	    RC TV Profile          : None
> 	    Device Features        :
> 		None
> 
> # cec-ctl -t0 --image-view-on
> Driver Info:
> 	Driver Name                : vc4_hdmi
> 	Adapter Name               : vc4
> 	Capabilities               : 0x0000010e
> 		Logical Addresses
> 		Transmit
> 		Passthrough
> 	Driver version             : 5.10.0
> 	Available Logical Addresses: 1
> 	Physical Address           : f.f.f.f
> 	Logical Address Mask       : 0x0000
> 	CEC Version                : 2.0
> 	Vendor ID                  : 0x000c03 (HDMI)
> 	OSD Name                   : Playback
> 	Logical Addresses          : 1 (Allow RC Passthrough)
> 
> 	  Logical Address          : Not Allocated
> 	    Primary Device Type    : Playback
> 	    Logical Address Type   : Playback
> 	    All Device Types       : Playback
> 	    RC TV Profile          : None
> 	    Device Features        :
> 		None
> 
> 
> Transmit from Unregistered to TV (15 to 0):
> CEC_MSG_IMAGE_VIEW_ON (0x04)
> 	Sequence: 1 Tx Timestamp: 77.631s
> 
> 
> And this is the output on my desktop with the Pulse-Eight:
> $ sudo cec-ctl -p0.0.0.0 --tv
> Driver Info:
> 	Driver Name                : pulse8-cec
> 	Adapter Name               : serio0
> 	Capabilities               : 0x0000003f
> 		Physical Address
> 		Logical Addresses
> 		Transmit
> 		Passthrough
> 		Remote Control Support
> 		Monitor All
> 	Driver version             : 5.9.8
> 	Available Logical Addresses: 1
> 	Connector Info             : None
> 	Physical Address           : 0.0.0.0
> 	Logical Address Mask       : 0x0001
> 	CEC Version                : 2.0
> 	Vendor ID                  : 0x000c03 (HDMI)
> 	OSD Name                   : 'TV  '
> 	Logical Addresses          : 1 (Allow RC Passthrough)
> 
> 	  Logical Address          : 0 (TV)
> 	    Primary Device Type    : TV
> 	    Logical Address Type   : TV
> 	    All Device Types       : TV
> 	    RC TV Profile          : None
> 	    Device Features        :
> 		None
> 
> $ sudo cec-ctl -M
> Driver Info:
> 	Driver Name                : pulse8-cec
> 	Adapter Name               : serio0
> 	Capabilities               : 0x0000003f
> 		Physical Address
> 		Logical Addresses
> 		Transmit
> 		Passthrough
> 		Remote Control Support
> 		Monitor All
> 	Driver version             : 5.9.8
> 	Available Logical Addresses: 1
> 	Connector Info             : None
> 	Physical Address           : 0.0.0.0
> 	Logical Address Mask       : 0x0001
> 	CEC Version                : 2.0
> 	Vendor ID                  : 0x000c03 (HDMI)
> 	OSD Name                   : 'TV  '
> 	Logical Addresses          : 1 (Allow RC Passthrough)
> 
> 	  Logical Address          : 0 (TV)
> 	    Primary Device Type    : TV
> 	    Logical Address Type   : TV
> 	    All Device Types       : TV
> 	    RC TV Profile          : None
> 	    Device Features        :
> 		None
> 
> 
> 
> Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no
> Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04)
> 
> So it looks like it's working as expected?

Yes, it looks good. Make sure you test this for both outputs of the RPi4. If it
works for both, then you can add my

Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

for this series.

Very nice work, thank you for doing this!

Regards,

	Hans

> 
> Maxime
> 

Re: [PATCH 00/15] drm/vc4: hdmi: Add CEC support for the BCM2711

[Maxime Ripard]

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

Hi Hans,

On Thu, Dec 17, 2020 at 11:53:42AM +0100, Hans Verkuil wrote:
> On 17/12/2020 11:49, Maxime Ripard wrote:
> > Hi Hans,
> > 
> > On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote:
> >> Hi Maxime,
> >>
> >> On 10/12/2020 14:46, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> Here's a series introducing the CEC support for the BCM2711 found on the
> >>> RaspberryPi4.
> >>>
> >>> The BCM2711 HDMI controller uses a similar layout for the CEC registers, the
> >>> main difference being that the interrupt handling part is now shared between
> >>> both HDMI controllers.
> >>>
> >>> This series is mainly about fixing a couple of bugs, reworking the driver to
> >>> support having two different interrupts, one for each direction, provided by an
> >>> external irqchip, and enables the irqchip driver for the controller we have.
> >>>
> >>> This has been tested on an RPi3 and RPi4, but requires the latest firmware.
> >>> It's is based on the 10 and 12 bpc series.
> >>
> >> This series looks good to me. Before I give my Acked-by for this series, can you
> >> confirm that it is possible to transmit the Image View On message on both outputs
> >> of the RPi4 when the HPD is low?
> >>
> >> See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt
> >> on how to test this with a Pulse-Eight device.
> >>
> >> This should work.
> > 
> > This is the output on the RPi4:
> > 
> > # cec-ctl --playback
> > Driver Info:
> > 	Driver Name                : vc4_hdmi
> > 	Adapter Name               : vc4
> > 	Capabilities               : 0x0000010e
> > 		Logical Addresses
> > 		Transmit
> > 		Passthrough
> > 	Driver version             : 5.10.0
> > 	Available Logical Addresses: 1
> > 	Physical Address           : f.f.f.f
> > 	Logical Address Mask       : 0x0000
> > 	CEC Version                : 2.0
> > 	Vendor ID                  : 0x000c03 (HDMI)
> > 	OSD Name                   : Playback
> > 	Logical Addresses          : 1 (Allow RC Passthrough)
> > 
> > 	  Logical Address          : Not Allocated
> > 	    Primary Device Type    : Playback
> > 	    Logical Address Type   : Playback
> > 	    All Device Types       : Playback
> > 	    RC TV Profile          : None
> > 	    Device Features        :
> > 		None
> > 
> > # cec-ctl -t0 --image-view-on
> > Driver Info:
> > 	Driver Name                : vc4_hdmi
> > 	Adapter Name               : vc4
> > 	Capabilities               : 0x0000010e
> > 		Logical Addresses
> > 		Transmit
> > 		Passthrough
> > 	Driver version             : 5.10.0
> > 	Available Logical Addresses: 1
> > 	Physical Address           : f.f.f.f
> > 	Logical Address Mask       : 0x0000
> > 	CEC Version                : 2.0
> > 	Vendor ID                  : 0x000c03 (HDMI)
> > 	OSD Name                   : Playback
> > 	Logical Addresses          : 1 (Allow RC Passthrough)
> > 
> > 	  Logical Address          : Not Allocated
> > 	    Primary Device Type    : Playback
> > 	    Logical Address Type   : Playback
> > 	    All Device Types       : Playback
> > 	    RC TV Profile          : None
> > 	    Device Features        :
> > 		None
> > 
> > 
> > Transmit from Unregistered to TV (15 to 0):
> > CEC_MSG_IMAGE_VIEW_ON (0x04)
> > 	Sequence: 1 Tx Timestamp: 77.631s
> > 
> > 
> > And this is the output on my desktop with the Pulse-Eight:
> > $ sudo cec-ctl -p0.0.0.0 --tv
> > Driver Info:
> > 	Driver Name                : pulse8-cec
> > 	Adapter Name               : serio0
> > 	Capabilities               : 0x0000003f
> > 		Physical Address
> > 		Logical Addresses
> > 		Transmit
> > 		Passthrough
> > 		Remote Control Support
> > 		Monitor All
> > 	Driver version             : 5.9.8
> > 	Available Logical Addresses: 1
> > 	Connector Info             : None
> > 	Physical Address           : 0.0.0.0
> > 	Logical Address Mask       : 0x0001
> > 	CEC Version                : 2.0
> > 	Vendor ID                  : 0x000c03 (HDMI)
> > 	OSD Name                   : 'TV  '
> > 	Logical Addresses          : 1 (Allow RC Passthrough)
> > 
> > 	  Logical Address          : 0 (TV)
> > 	    Primary Device Type    : TV
> > 	    Logical Address Type   : TV
> > 	    All Device Types       : TV
> > 	    RC TV Profile          : None
> > 	    Device Features        :
> > 		None
> > 
> > $ sudo cec-ctl -M
> > Driver Info:
> > 	Driver Name                : pulse8-cec
> > 	Adapter Name               : serio0
> > 	Capabilities               : 0x0000003f
> > 		Physical Address
> > 		Logical Addresses
> > 		Transmit
> > 		Passthrough
> > 		Remote Control Support
> > 		Monitor All
> > 	Driver version             : 5.9.8
> > 	Available Logical Addresses: 1
> > 	Connector Info             : None
> > 	Physical Address           : 0.0.0.0
> > 	Logical Address Mask       : 0x0001
> > 	CEC Version                : 2.0
> > 	Vendor ID                  : 0x000c03 (HDMI)
> > 	OSD Name                   : 'TV  '
> > 	Logical Addresses          : 1 (Allow RC Passthrough)
> > 
> > 	  Logical Address          : 0 (TV)
> > 	    Primary Device Type    : TV
> > 	    Logical Address Type   : TV
> > 	    All Device Types       : TV
> > 	    RC TV Profile          : None
> > 	    Device Features        :
> > 		None
> > 
> > 
> > 
> > Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no
> > Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04)
> > 
> > So it looks like it's working as expected?
> 
> Yes, it looks good. Make sure you test this for both outputs of the RPi4.

It's a good thing you asked, I don't appear to get CEC interrupts from
HDMI1. I'll fix it and send another version (probably not before the end
of december though).

> If it works for both, then you can add my
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> for this series.
> 
> Very nice work, thank you for doing this!

Thanks!

I'll hold your a-b until the next version though, fixing hdmi1 might
change a few things.

Maxime

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

Re: [PATCH 02/15] drm/vc4: hdmi: Move hdmi reset to bind

[Dave Stevenson]

Hi Maxime & Dom

On Thu, 10 Dec 2020 at 13:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> From: Dom Cobley <popcornmix@gmail.com>
>
> The hdmi reset got moved to a later point in the commit 9045e91a476b
> ("drm/vc4: hdmi: Add reset callback").
>
> However, the reset now occurs after vc4_hdmi_cec_init and so tramples
> the setup of registers like HDMI_CEC_CNTRL_1
>
> This only affects pi0-3 as on pi4 the cec registers are in a separate
> block

It does mean that this reset only happens once on bind rather than on
every pre_crtc_configure, but as this really is the big reset the
entire block I don't see it needing to be triggered on every
configure.

> Fixes: 9045e91a476b ("drm/vc4: hdmi: Add reset callback")
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 8006bddc8fbb..3df1747dd917 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -773,9 +773,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>                 return;
>         }
>
> -       if (vc4_hdmi->variant->reset)
> -               vc4_hdmi->variant->reset(vc4_hdmi);
> -
>         if (vc4_hdmi->variant->phy_init)
>                 vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state);
>
> @@ -1865,6 +1862,9 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>         vc4_hdmi->disable_wifi_frequencies =
>                 of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
>
> +       if (vc4_hdmi->variant->reset)
> +               vc4_hdmi->variant->reset(vc4_hdmi);
> +
>         pm_runtime_enable(dev);
>
>         drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> --
> 2.28.0
>

Re: [PATCH 04/15] drm/vc4: hdmi: Fix up CEC registers

[Dave Stevenson]

Hi Maxime & Dom

On Thu, 10 Dec 2020 at 13:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> From: Dom Cobley <popcornmix@gmail.com>
>
> The commit 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout
> abstraction") forgot one CEC register, and made a copy and paste mistake
> for another one. Fix those mistakes.
>
> Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction")
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
> index 013fd57febd8..20a1438a72cb 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
> @@ -29,6 +29,7 @@ enum vc4_hdmi_field {
>         HDMI_CEC_CPU_MASK_SET,
>         HDMI_CEC_CPU_MASK_STATUS,
>         HDMI_CEC_CPU_STATUS,
> +       HDMI_CEC_CPU_SET,
>
>         /*
>          * Transmit data, first byte is low byte of the 32-bit reg.
> @@ -199,9 +200,10 @@ static const struct vc4_hdmi_register vc4_hdmi_fields[] = {
>         VC4_HDMI_REG(HDMI_TX_PHY_RESET_CTL, 0x02c0),
>         VC4_HDMI_REG(HDMI_TX_PHY_CTL_0, 0x02c4),
>         VC4_HDMI_REG(HDMI_CEC_CPU_STATUS, 0x0340),
> +       VC4_HDMI_REG(HDMI_CEC_CPU_SET, 0x0344),
>         VC4_HDMI_REG(HDMI_CEC_CPU_CLEAR, 0x0348),
>         VC4_HDMI_REG(HDMI_CEC_CPU_MASK_STATUS, 0x034c),
> -       VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x034c),
> +       VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x0350),
>         VC4_HDMI_REG(HDMI_CEC_CPU_MASK_CLEAR, 0x0354),
>         VC4_HDMI_REG(HDMI_RAM_PACKET_START, 0x0400),
>  };
> --
> 2.28.0
>

Re: [PATCH 06/15] drm/vc4: hdmi: Compute the CEC clock divider from the clock rate

[Dave Stevenson]

Hi Maxime

On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The CEC clock divider needs to output a frequency of 40kHz from the HSM
> rate on the BCM2835. The driver used to have a fixed frequency for it,
> but that changed and we now need to compute it dynamically to maintain
> the proper rate.
>
> Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

(To be a total pedant it's still a fixed frequency on vc4, but it's
configurable via the variant entry).

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index eff3bac562c6..0c53d7427d15 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1586,6 +1586,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
>  {
>         struct cec_connector_info conn_info;
>         struct platform_device *pdev = vc4_hdmi->pdev;
> +       u16 clk_cnt;
>         u32 value;
>         int ret;
>
> @@ -1611,8 +1612,9 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
>          * divider: the hsm_clock rate and this divider setting will
>          * give a 40 kHz CEC clock.
>          */
> +       clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
>         value |= VC4_HDMI_CEC_ADDR_MASK |
> -                (4091 << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
> +                (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
>         HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
>         ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0),
>                                         vc4_cec_irq_handler,
> --
> 2.28.0
>

Re: [PATCH 08/15] drm/vc4: hdmi: Introduce a CEC clock

[Dave Stevenson]

Hi Maxime

On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
>
> While the BCM2835 had the CEC clock derived from the HSM clock, the
> BCM2711 has a dedicated parent clock for it.
>
> Let's introduce a separate clock for it so that we can handle both
> cases.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++-
>  drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b93ee3e26e2b..0debd22bc992 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
>          * Set the clock divider: the hsm_clock rate and this divider
>          * setting will give a 40 kHz CEC clock.
>          */
> -       clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
> +       clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ;
>         value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;
>         HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
>  }
> @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>                 return PTR_ERR(vc4_hdmi->hsm_clock);
>         }
>         vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
> +       vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock;
>
>         return 0;
>  }
> @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>                 return PTR_ERR(vc4_hdmi->audio_clock);
>         }
>
> +       vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");
> +       if (IS_ERR(vc4_hdmi->cec_clock)) {
> +               DRM_ERROR("Failed to get CEC clock\n");
> +               return PTR_ERR(vc4_hdmi->cec_clock);
> +       }

Aren't we adding to the DT binding here and breaking backwards compatibility?
Admittedly CEC didn't work before (and was masked out) for vc5, but do
we need to worry about those with existing DT files that currently
work happily?

Otherwise I'm happy with the patch.

  Dave

> +
>         vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>         if (IS_ERR(vc4_hdmi->reset)) {
>                 DRM_ERROR("Failed to get HDMI reset line\n");
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index 720914761261..adc4bf33ff15 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -155,6 +155,7 @@ struct vc4_hdmi {
>         bool cec_tx_ok;
>         bool cec_irq_was_rx;
>
> +       struct clk *cec_clock;
>         struct clk *pixel_clock;
>         struct clk *hsm_clock;
>         struct clk *audio_clock;
> --
> 2.28.0
>

Re: [PATCH 08/15] drm/vc4: hdmi: Introduce a CEC clock

[Maxime Ripard]

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

Hi Dave,

On Fri, Dec 18, 2020 at 11:37:50AM +0000, Dave Stevenson wrote:
> Hi Maxime
> 
> On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > While the BCM2835 had the CEC clock derived from the HSM clock, the
> > BCM2711 has a dedicated parent clock for it.
> >
> > Let's introduce a separate clock for it so that we can handle both
> > cases.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++-
> >  drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index b93ee3e26e2b..0debd22bc992 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
> >          * Set the clock divider: the hsm_clock rate and this divider
> >          * setting will give a 40 kHz CEC clock.
> >          */
> > -       clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
> > +       clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ;
> >         value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;
> >         HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
> >  }
> > @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> >                 return PTR_ERR(vc4_hdmi->hsm_clock);
> >         }
> >         vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
> > +       vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock;
> >
> >         return 0;
> >  }
> > @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> >                 return PTR_ERR(vc4_hdmi->audio_clock);
> >         }
> >
> > +       vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");
> > +       if (IS_ERR(vc4_hdmi->cec_clock)) {
> > +               DRM_ERROR("Failed to get CEC clock\n");
> > +               return PTR_ERR(vc4_hdmi->cec_clock);
> > +       }
> 
> Aren't we adding to the DT binding here and breaking backwards compatibility?
> Admittedly CEC didn't work before (and was masked out) for vc5, but do
> we need to worry about those with existing DT files that currently
> work happily?

The DT compatibility is not a worry here: I made sure the CEC clock and
range were part of the binding since it's been introduced:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e3725b05b785e73482a194b99bff3d5a1c85140

So we were not using it so far, but it was in the DT all along

Maxime

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

Re: [PATCH 08/15] drm/vc4: hdmi: Introduce a CEC clock

[Dave Stevenson]

On Fri, 18 Dec 2020 at 12:23, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Dave,
>
> On Fri, Dec 18, 2020 at 11:37:50AM +0000, Dave Stevenson wrote:
> > Hi Maxime
> >
> > On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > While the BCM2835 had the CEC clock derived from the HSM clock, the
> > > BCM2711 has a dedicated parent clock for it.
> > >
> > > Let's introduce a separate clock for it so that we can handle both
> > > cases.
> > >
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++-
> > >  drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index b93ee3e26e2b..0debd22bc992 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
> > >          * Set the clock divider: the hsm_clock rate and this divider
> > >          * setting will give a 40 kHz CEC clock.
> > >          */
> > > -       clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
> > > +       clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ;
> > >         value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;
> > >         HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
> > >  }
> > > @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> > >                 return PTR_ERR(vc4_hdmi->hsm_clock);
> > >         }
> > >         vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
> > > +       vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock;
> > >
> > >         return 0;
> > >  }
> > > @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> > >                 return PTR_ERR(vc4_hdmi->audio_clock);
> > >         }
> > >
> > > +       vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");
> > > +       if (IS_ERR(vc4_hdmi->cec_clock)) {
> > > +               DRM_ERROR("Failed to get CEC clock\n");
> > > +               return PTR_ERR(vc4_hdmi->cec_clock);
> > > +       }
> >
> > Aren't we adding to the DT binding here and breaking backwards compatibility?
> > Admittedly CEC didn't work before (and was masked out) for vc5, but do
> > we need to worry about those with existing DT files that currently
> > work happily?
>
> The DT compatibility is not a worry here: I made sure the CEC clock and
> range were part of the binding since it's been introduced:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e3725b05b785e73482a194b99bff3d5a1c85140
>
> So we were not using it so far, but it was in the DT all along

I guess I should have read it then :-)
In which case
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Re: [PATCH 05/15] drm/vc4: hdmi: Restore cec physical address on reconnect

[Dave Stevenson]

Hi  Maxime & Dom

On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
>
> From: Dom Cobley <popcornmix@gmail.com>
>
> Currently we call cec_phys_addr_invalidate on a hotplug deassert.
> That may be due to a TV power cycling, or an AVR being switched
> on (and switching edid).
>
> This makes CEC unusable since our controller wouldn't have a physical
> address anymore.
>
> Set it back up again on the hotplug assert.
>
> Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support")
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 28b78ea885ea..eff3bac562c6 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -136,20 +136,29 @@ static enum drm_connector_status
>  vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>  {
>         struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> +       bool connected = false;
>
>         if (vc4_hdmi->hpd_gpio) {
>                 if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^
>                     vc4_hdmi->hpd_active_low)
> -                       return connector_status_connected;
> -               cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> -               return connector_status_disconnected;
> -       }
> -
> -       if (drm_probe_ddc(vc4_hdmi->ddc))
> -               return connector_status_connected;
> -
> +                       connected = true;
> +       } else if (drm_probe_ddc(vc4_hdmi->ddc))
> +               connected = true;
>         if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)

This needs to become an "else if(...".
It used to be that all the other paths would return, so were mutually
exclusive to this. Now they set a thing and keep going we need to
avoid reading the register should there be a HPD gpio or the ddc probe
succeeds.
Memory says that otherwise Pi3 always reports connected.

I fixed this in a downstream patch already -
https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453a0d3e98c

Otherwise fine.

  Dave

> +               connected = true;
> +       if (connected) {
> +               if (connector->status != connector_status_connected) {
> +                       struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);
> +
> +                       if (edid) {
> +                               cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
> +                               vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);
> +                               drm_connector_update_edid_property(connector, edid);
> +                               kfree(edid);
> +                       }
> +               }
>                 return connector_status_connected;
> +       }
>         cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
>         return connector_status_disconnected;
>  }
> --
> 2.28.0
>

Re: [PATCH 07/15] drm/vc4: hdmi: Update the CEC clock divider on HSM rate change

[Dave Stevenson]

Hi Maxime

On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
>
> As part of the enable sequence we might change the HSM clock rate if the
> pixel rate is different than the one we were already dealing with.
>
> On the BCM2835 however, the CEC clock derives from the HSM clock so any
> rate change will need to be reflected in the CEC clock divider to output
> 40kHz.
>
> Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

I thought we'd got a duplicate patch here, but it's moving code that
was changed in patch 6/15 so it can be called from
vc4_hdmi_encoder_pre_crtc_configure too. Good for confusing me!

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 39 +++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 0c53d7427d15..b93ee3e26e2b 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -132,6 +132,27 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
>                    HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL);
>  }
>
> +#ifdef CONFIG_DRM_VC4_HDMI_CEC
> +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
> +{
> +       u16 clk_cnt;
> +       u32 value;
> +
> +       value = HDMI_READ(HDMI_CEC_CNTRL_1);
> +       value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
> +
> +       /*
> +        * Set the clock divider: the hsm_clock rate and this divider
> +        * setting will give a 40 kHz CEC clock.
> +        */
> +       clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
> +       value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;
> +       HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
> +}
> +#else
> +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
> +#endif
> +
>  static enum drm_connector_status
>  vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>  {
> @@ -761,6 +782,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>                 return;
>         }
>
> +       vc4_hdmi_cec_update_clk_div(vc4_hdmi);
> +
>         /*
>          * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup
>          * at 300MHz.
> @@ -1586,7 +1609,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
>  {
>         struct cec_connector_info conn_info;
>         struct platform_device *pdev = vc4_hdmi->pdev;
> -       u16 clk_cnt;
>         u32 value;
>         int ret;
>
> @@ -1605,17 +1627,14 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
>         cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
>
>         HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff);
> +
>         value = HDMI_READ(HDMI_CEC_CNTRL_1);
> -       value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
> -       /*
> -        * Set the logical address to Unregistered and set the clock
> -        * divider: the hsm_clock rate and this divider setting will
> -        * give a 40 kHz CEC clock.
> -        */
> -       clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
> -       value |= VC4_HDMI_CEC_ADDR_MASK |
> -                (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
> +       /* Set the logical address to Unregistered */
> +       value |= VC4_HDMI_CEC_ADDR_MASK;
>         HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
> +
> +       vc4_hdmi_cec_update_clk_div(vc4_hdmi);
> +
>         ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0),
>                                         vc4_cec_irq_handler,
>                                         vc4_cec_irq_handler_thread, 0,
> --
> 2.28.0
>

Re: [PATCH 12/15] drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts

[Dave Stevenson]

On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
>
> We introduced the BCM2711 support to the vc4 HDMI controller with 5.10,
> but this was lacking any of the interrupts of the CEC controller so we
> have to deal with the backward compatibility.
>
> Do so by simply ignoring the CEC setup if the DT doesn't have the
> interrupts property.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 327638d93032..69217c68d3a4 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1655,9 +1655,15 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
>  {
>         struct cec_connector_info conn_info;
>         struct platform_device *pdev = vc4_hdmi->pdev;
> +       struct device *dev = &pdev->dev;
>         u32 value;
>         int ret;
>
> +       if (!of_find_property(dev->of_node, "interrupts", NULL)) {
> +               dev_warn(dev, "'interrupts' DT property is missing, no CEC\n");
> +               return 0;
> +       }
> +
>         vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops,
>                                                   vc4_hdmi, "vc4",
>                                                   CEC_CAP_DEFAULTS |
> --
> 2.28.0
>

Re: [PATCH 11/15] drm/vc4: hdmi: Remove cec_available flag

[Dave Stevenson]

Hi Dom & Maxime

On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
>
> From: Dom Cobley <popcornmix@gmail.com>
>
> Now that our HDMI controller supports CEC for the BCM2711, let's remove
> that flag.
>
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ----
>  drivers/gpu/drm/vc4/vc4_hdmi.h | 3 ---
>  2 files changed, 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d208b7d1d937..327638d93032 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1658,9 +1658,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
>         u32 value;
>         int ret;
>
> -       if (!vc4_hdmi->variant->cec_available)
> -               return 0;
> -
>         vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops,
>                                                   vc4_hdmi, "vc4",
>                                                   CEC_CAP_DEFAULTS |
> @@ -2074,7 +2071,6 @@ static const struct vc4_hdmi_variant bcm2835_variant = {
>         .debugfs_name           = "hdmi_regs",
>         .card_name              = "vc4-hdmi",
>         .max_pixel_clock        = 162000000,
> -       .cec_available          = true,
>         .registers              = vc4_hdmi_fields,
>         .num_registers          = ARRAY_SIZE(vc4_hdmi_fields),
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index 27352827f70c..c93ada62f429 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -42,9 +42,6 @@ struct vc4_hdmi_variant {
>         /* Filename to expose the registers in debugfs */
>         const char *debugfs_name;
>
> -       /* Set to true when the CEC support is available */
> -       bool cec_available;
> -
>         /* Maximum pixel clock supported by the controller (in Hz) */
>         unsigned long long max_pixel_clock;
>
> --
> 2.28.0
>

Re: [PATCH 05/15] drm/vc4: hdmi: Restore cec physical address on reconnect

[Dave Stevenson]

On Fri, 18 Dec 2020 at 14:21, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi  Maxime & Dom
>
> On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > From: Dom Cobley <popcornmix@gmail.com>
> >
> > Currently we call cec_phys_addr_invalidate on a hotplug deassert.
> > That may be due to a TV power cycling, or an AVR being switched
> > on (and switching edid).
> >
> > This makes CEC unusable since our controller wouldn't have a physical
> > address anymore.
> >
> > Set it back up again on the hotplug assert.
> >
> > Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support")
> > Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index 28b78ea885ea..eff3bac562c6 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -136,20 +136,29 @@ static enum drm_connector_status
> >  vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
> >  {
> >         struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> > +       bool connected = false;
> >
> >         if (vc4_hdmi->hpd_gpio) {
> >                 if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^
> >                     vc4_hdmi->hpd_active_low)
> > -                       return connector_status_connected;
> > -               cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> > -               return connector_status_disconnected;
> > -       }
> > -
> > -       if (drm_probe_ddc(vc4_hdmi->ddc))
> > -               return connector_status_connected;
> > -
> > +                       connected = true;
> > +       } else if (drm_probe_ddc(vc4_hdmi->ddc))
> > +               connected = true;
> >         if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)
>
> This needs to become an "else if(...".
> It used to be that all the other paths would return, so were mutually
> exclusive to this. Now they set a thing and keep going we need to
> avoid reading the register should there be a HPD gpio or the ddc probe
> succeeds.
> Memory says that otherwise Pi3 always reports connected.
>
> I fixed this in a downstream patch already -
> https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453a0d3e98c
>
> Otherwise fine.
>
>   Dave
>
> > +               connected = true;
> > +       if (connected) {
> > +               if (connector->status != connector_status_connected) {
> > +                       struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);
> > +
> > +                       if (edid) {
> > +                               cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
> > +                               vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);
> > +                               drm_connector_update_edid_property(connector, edid);

Actually looking at this again in the context of the other changes, do
we need to call drm_connector_update_edid_property() here?

We've just called drm_get_edid() to get the edid, and that calls
drm_connector_update_edid_property() as well [1]
Updating vc4_hdmi->encoder.hdmi_monitor may be necessary. It's
otherwise done in vc4_hdmi_connector_get_modes, which I sort of expect
to be called almost immediately by the framework when connector_detect
returns "connected". I haven't checked if that is guaranteed though.

vc4_hdmi_connector_get_modes also includes a manual call to
drm_connector_update_edid_property after having just called
drm_get_edid, so that one feels redundant too.

  Dave

[1] https://elixir.bootlin.com/linux/v5.10/source/drivers/gpu/drm/drm_edid.c#L2059

> > +                               kfree(edid);
> > +                       }
> > +               }
> >                 return connector_status_connected;
> > +       }
> >         cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> >         return connector_status_disconnected;
> >  }
> > --
> > 2.28.0
> >

Re: [PATCH 05/15] drm/vc4: hdmi: Restore cec physical address on reconnect

[Maxime Ripard]

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

Hi Dave,

Thanks for your review

On Fri, Dec 18, 2020 at 02:45:54PM +0000, Dave Stevenson wrote:
> On Fri, 18 Dec 2020 at 14:21, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi  Maxime & Dom
> >
> > On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > From: Dom Cobley <popcornmix@gmail.com>
> > >
> > > Currently we call cec_phys_addr_invalidate on a hotplug deassert.
> > > That may be due to a TV power cycling, or an AVR being switched
> > > on (and switching edid).
> > >
> > > This makes CEC unusable since our controller wouldn't have a physical
> > > address anymore.
> > >
> > > Set it back up again on the hotplug assert.
> > >
> > > Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support")
> > > Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++--------
> > >  1 file changed, 17 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index 28b78ea885ea..eff3bac562c6 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -136,20 +136,29 @@ static enum drm_connector_status
> > >  vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
> > >  {
> > >         struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> > > +       bool connected = false;
> > >
> > >         if (vc4_hdmi->hpd_gpio) {
> > >                 if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^
> > >                     vc4_hdmi->hpd_active_low)
> > > -                       return connector_status_connected;
> > > -               cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> > > -               return connector_status_disconnected;
> > > -       }
> > > -
> > > -       if (drm_probe_ddc(vc4_hdmi->ddc))
> > > -               return connector_status_connected;
> > > -
> > > +                       connected = true;
> > > +       } else if (drm_probe_ddc(vc4_hdmi->ddc))
> > > +               connected = true;
> > >         if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)
> >
> > This needs to become an "else if(...".
> > It used to be that all the other paths would return, so were mutually
> > exclusive to this. Now they set a thing and keep going we need to
> > avoid reading the register should there be a HPD gpio or the ddc probe
> > succeeds.
> > Memory says that otherwise Pi3 always reports connected.
> >
> > I fixed this in a downstream patch already -
> > https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453a0d3e98c
> >
> > Otherwise fine.
> >
> >   Dave
> >
> > > +               connected = true;
> > > +       if (connected) {
> > > +               if (connector->status != connector_status_connected) {
> > > +                       struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);
> > > +
> > > +                       if (edid) {
> > > +                               cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
> > > +                               vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);
> > > +                               drm_connector_update_edid_property(connector, edid);
> 
> Actually looking at this again in the context of the other changes, do
> we need to call drm_connector_update_edid_property() here?
> 
> We've just called drm_get_edid() to get the edid, and that calls
> drm_connector_update_edid_property() as well [1]

Yeah, you're right I'll drop it

> Updating vc4_hdmi->encoder.hdmi_monitor may be necessary. It's
> otherwise done in vc4_hdmi_connector_get_modes, which I sort of expect
> to be called almost immediately by the framework when connector_detect
> returns "connected". I haven't checked if that is guaranteed though.
> 
> vc4_hdmi_connector_get_modes also includes a manual call to
> drm_connector_update_edid_property after having just called
> drm_get_edid, so that one feels redundant too.

.get_modes is called in drm_helper_probe_single_connector_modes, which
is usually the helper set in .fill_modes. .fill_modes seems to only be
called when either DRM_IOCTL_MODE_GETCONNECTOR is called, or when the
connector status is forced through sysfs, so it doesn't look like it's
done automatically.

I'm not sure we need to set hdmi_monitor though, it's only used to
configure the display related side, and that can't happen without
get_modes being called.

Maxime

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