All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535
@ 2022-03-19 15:10 ` Alvin Šipraga
  0 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-03-19 15:10 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
  Cc: Alvin Šipraga, dri-devel, linux-kernel

From: Alvin Šipraga <alsi@bang-olufsen.dk>

We have an ADV7535 which is nominally supported by this driver. These
two patches fix up the driver to get CEC working too.

The first adds the basic support by correcting some register offsets.

The second addresses an issue we saw with CEC RX on the ADV7535. It
hasn't been tested with the other chips (e.g. ADV7533), although it
should be compatible. I'm sending it against drm-misc-next because the
issue wasn't reported for other chips, and ADV7535 didn't have CEC
support before. But feel free to take it into -fixes instead.

Alvin Šipraga (2):
  drm: bridge: adv7511: enable CEC support for ADV7535
  drm: bridge: adv7511: use non-legacy mode for CEC RX

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  27 ++++-
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 116 +++++++++++++------
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  22 +++-
 3 files changed, 119 insertions(+), 46 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535
@ 2022-03-19 15:10 ` Alvin Šipraga
  0 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-03-19 15:10 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
  Cc: linux-kernel, dri-devel, Alvin Šipraga

From: Alvin Šipraga <alsi@bang-olufsen.dk>

We have an ADV7535 which is nominally supported by this driver. These
two patches fix up the driver to get CEC working too.

The first adds the basic support by correcting some register offsets.

The second addresses an issue we saw with CEC RX on the ADV7535. It
hasn't been tested with the other chips (e.g. ADV7533), although it
should be compatible. I'm sending it against drm-misc-next because the
issue wasn't reported for other chips, and ADV7535 didn't have CEC
support before. But feel free to take it into -fixes instead.

Alvin Šipraga (2):
  drm: bridge: adv7511: enable CEC support for ADV7535
  drm: bridge: adv7511: use non-legacy mode for CEC RX

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  27 ++++-
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 116 +++++++++++++------
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  22 +++-
 3 files changed, 119 insertions(+), 46 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] drm: bridge: adv7511: enable CEC support for ADV7535
  2022-03-19 15:10 ` Alvin Šipraga
@ 2022-03-19 15:10   ` Alvin Šipraga
  -1 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-03-19 15:10 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
  Cc: Alvin Šipraga, dri-devel, linux-kernel

From: Alvin Šipraga <alsi@bang-olufsen.dk>

Like the ADV7533, the ADV7535 has an offset for the CEC register map,
and it is the same value (ADV7533_REG_CEC_OFFSET = 0x70).

Rather than testing for numerous chip types in the offset calculations
throughout the driver, just compute it during driver probe and put it in
the private adv7511 data structure.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 +
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 18 ++++++------------
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  5 +++--
 3 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 6a882891d91c..da6d8ee2cd84 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -335,6 +335,7 @@ struct adv7511 {
 
 	struct regmap *regmap;
 	struct regmap *regmap_cec;
+	unsigned int reg_cec_offset;
 	enum drm_connector_status status;
 	bool powered;
 
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
index 28d9becc939c..1f619389e201 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
@@ -21,8 +21,7 @@
 
 static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 {
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	unsigned int val;
 
 	if (regmap_read(adv7511->regmap_cec,
@@ -73,8 +72,7 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 
 void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 {
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
 				ADV7511_INT1_CEC_TX_ARBIT_LOST |
 				ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
@@ -118,8 +116,7 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 
 	if (adv7511->i2c_cec == NULL)
 		return -EIO;
@@ -165,8 +162,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	unsigned int i, free_idx = ADV7511_MAX_ADDRS;
 
 	if (!adv7511->cec_enabled_adap)
@@ -235,8 +231,7 @@ static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 				     u32 signal_free_time, struct cec_msg *msg)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	u8 len = msg->len;
 	unsigned int i;
 
@@ -289,8 +284,7 @@ static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
 
 int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 {
-	unsigned int offset = adv7511->type == ADV7533 ?
-						ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	int ret = adv7511_cec_parse_dt(dev, adv7511);
 
 	if (ret)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 005bf18682ff..0be65a1ffc47 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1027,8 +1027,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
 	struct i2c_client *i2c = to_i2c_client(dev);
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
-		reg -= ADV7533_REG_CEC_OFFSET;
+	reg -= adv7511->reg_cec_offset;
 
 	switch (reg) {
 	case ADV7511_REG_CEC_RX_FRAME_HDR:
@@ -1073,6 +1072,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
 		ret = adv7533_patch_cec_registers(adv);
 		if (ret)
 			goto err;
+
+		adv->reg_cec_offset = ADV7533_REG_CEC_OFFSET;
 	}
 
 	return 0;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 1/2] drm: bridge: adv7511: enable CEC support for ADV7535
@ 2022-03-19 15:10   ` Alvin Šipraga
  0 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-03-19 15:10 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
  Cc: linux-kernel, dri-devel, Alvin Šipraga

From: Alvin Šipraga <alsi@bang-olufsen.dk>

Like the ADV7533, the ADV7535 has an offset for the CEC register map,
and it is the same value (ADV7533_REG_CEC_OFFSET = 0x70).

Rather than testing for numerous chip types in the offset calculations
throughout the driver, just compute it during driver probe and put it in
the private adv7511 data structure.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 +
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 18 ++++++------------
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  5 +++--
 3 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 6a882891d91c..da6d8ee2cd84 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -335,6 +335,7 @@ struct adv7511 {
 
 	struct regmap *regmap;
 	struct regmap *regmap_cec;
+	unsigned int reg_cec_offset;
 	enum drm_connector_status status;
 	bool powered;
 
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
index 28d9becc939c..1f619389e201 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
@@ -21,8 +21,7 @@
 
 static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 {
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	unsigned int val;
 
 	if (regmap_read(adv7511->regmap_cec,
@@ -73,8 +72,7 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 
 void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 {
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
 				ADV7511_INT1_CEC_TX_ARBIT_LOST |
 				ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
@@ -118,8 +116,7 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 
 	if (adv7511->i2c_cec == NULL)
 		return -EIO;
@@ -165,8 +162,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	unsigned int i, free_idx = ADV7511_MAX_ADDRS;
 
 	if (!adv7511->cec_enabled_adap)
@@ -235,8 +231,7 @@ static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 				     u32 signal_free_time, struct cec_msg *msg)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
-	unsigned int offset = adv7511->type == ADV7533 ?
-					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	u8 len = msg->len;
 	unsigned int i;
 
@@ -289,8 +284,7 @@ static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
 
 int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 {
-	unsigned int offset = adv7511->type == ADV7533 ?
-						ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int offset = adv7511->reg_cec_offset;
 	int ret = adv7511_cec_parse_dt(dev, adv7511);
 
 	if (ret)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 005bf18682ff..0be65a1ffc47 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1027,8 +1027,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
 	struct i2c_client *i2c = to_i2c_client(dev);
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
-		reg -= ADV7533_REG_CEC_OFFSET;
+	reg -= adv7511->reg_cec_offset;
 
 	switch (reg) {
 	case ADV7511_REG_CEC_RX_FRAME_HDR:
@@ -1073,6 +1072,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
 		ret = adv7533_patch_cec_registers(adv);
 		if (ret)
 			goto err;
+
+		adv->reg_cec_offset = ADV7533_REG_CEC_OFFSET;
 	}
 
 	return 0;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] drm: bridge: adv7511: use non-legacy mode for CEC RX
  2022-03-19 15:10 ` Alvin Šipraga
@ 2022-03-19 15:10   ` Alvin Šipraga
  -1 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-03-19 15:10 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
  Cc: Alvin Šipraga, dri-devel, linux-kernel

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The ADV7511 family of bridges supports two modes for CEC RX: legacy and
non-legacy mode. The only difference is whether the chip uses a single
CEC RX buffer, or uses all three available RX buffers. Currently the
adv7511 driver uses legacy mode.

While debugging a stall in CEC RX on an ADV7535, we reached out to
Analog Devices, who suggested to use non-legacy mode instead.  According
to the programming guide for the ADV7511 [1], and the register control
manual of the ADV7535 [2], this is the default behaviour on reset. As
previously stated, the adv7511 driver currently overrides this to legacy
mode.

This patch updates the adv7511 driver to instead use non-legacy mode
with all three CEC RX buffers. As a result of this change, we no longer
experience any stalling of CEC RX with the ADV7535. It is not known why
non-legacy mode solves this particular issue, but besides this, no
functional change is to be expected by this patch. Please note that this
has only been tested on an ADV7535.

What follows is a brief description of the non-legacy mode interrupt
handling behaviour. The programming guide in [1] gives a more detailed
explanation.

With three RX buffers, the interrupt handler checks the CEC_RX_STATUS
register (renamed from CEC_RX_ENABLE in this patch), which contains
2-bit psuedo-timestamps for each of the RX buffers. The RX timestamps
for each buffer represent the time of arrival for the CEC frame held in
a given buffer, with lower timestamp values indicating chronologically
older frames. A special value of 0 indicates that the given RX buffer
is inactive and should be skipped. The interrupt handler parses these
timestamps and then reads the active RX buffers in the prescribed order
using the same logic as before. Changes have been made to ensure that
the correct RX buffer is cleared after processing. This clearing
procesure also sets the timestamp of the given RX buffer to 0 to mark it
as inactive.

[1] https://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf
    cf. CEC Map, register 0x4A, bit 3, default value 1:
    0 = Use only buffer 0 to store CEC frames (Legacy mode)
    1 = Use all 3 buffers to stores the CEC frames (Non-legacy mode)

[2] The ADV7535 register control manual is under NDA, but trust me when
    I say that non-legacy CEC RX mode is the default here too. Here the
    register is offset by 0x70 and has an address of 0xBA in the DSI_CEC
    regiser map.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     | 26 +++++-
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 98 +++++++++++++++-----
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++-
 3 files changed, 109 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index da6d8ee2cd84..9e3bb8a8ee40 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -209,10 +209,16 @@
 #define ADV7511_REG_CEC_TX_ENABLE	0x11
 #define ADV7511_REG_CEC_TX_RETRY	0x12
 #define ADV7511_REG_CEC_TX_LOW_DRV_CNT	0x14
-#define ADV7511_REG_CEC_RX_FRAME_HDR	0x15
-#define ADV7511_REG_CEC_RX_FRAME_DATA0	0x16
-#define ADV7511_REG_CEC_RX_FRAME_LEN	0x25
-#define ADV7511_REG_CEC_RX_ENABLE	0x26
+#define ADV7511_REG_CEC_RX1_FRAME_HDR	0x15
+#define ADV7511_REG_CEC_RX1_FRAME_DATA0	0x16
+#define ADV7511_REG_CEC_RX1_FRAME_LEN	0x25
+#define ADV7511_REG_CEC_RX_STATUS	0x26
+#define ADV7511_REG_CEC_RX2_FRAME_HDR	0x27
+#define ADV7511_REG_CEC_RX2_FRAME_DATA0	0x28
+#define ADV7511_REG_CEC_RX2_FRAME_LEN	0x37
+#define ADV7511_REG_CEC_RX3_FRAME_HDR	0x38
+#define ADV7511_REG_CEC_RX3_FRAME_DATA0	0x39
+#define ADV7511_REG_CEC_RX3_FRAME_LEN	0x48
 #define ADV7511_REG_CEC_RX_BUFFERS	0x4a
 #define ADV7511_REG_CEC_LOG_ADDR_MASK	0x4b
 #define ADV7511_REG_CEC_LOG_ADDR_0_1	0x4c
@@ -220,6 +226,18 @@
 #define ADV7511_REG_CEC_CLK_DIV		0x4e
 #define ADV7511_REG_CEC_SOFT_RESET	0x50
 
+static const u8 ADV7511_REG_CEC_RX_FRAME_HDR[] = {
+	ADV7511_REG_CEC_RX1_FRAME_HDR,
+	ADV7511_REG_CEC_RX2_FRAME_HDR,
+	ADV7511_REG_CEC_RX3_FRAME_HDR,
+};
+
+static const u8 ADV7511_REG_CEC_RX_FRAME_LEN[] = {
+	ADV7511_REG_CEC_RX1_FRAME_LEN,
+	ADV7511_REG_CEC_RX2_FRAME_LEN,
+	ADV7511_REG_CEC_RX3_FRAME_LEN,
+};
+
 #define ADV7533_REG_CEC_OFFSET		0x70
 
 enum adv7511_input_clock {
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
index 1f619389e201..399f625a50c8 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
@@ -17,7 +17,8 @@
 
 #define ADV7511_INT1_CEC_MASK \
 	(ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
-	 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
+	 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1 | \
+	 ADV7511_INT1_CEC_RX_READY2 | ADV7511_INT1_CEC_RX_READY3)
 
 static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 {
@@ -70,25 +71,16 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 	}
 }
 
-void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
+static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
 {
 	unsigned int offset = adv7511->reg_cec_offset;
-	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
-				ADV7511_INT1_CEC_TX_ARBIT_LOST |
-				ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
 	struct cec_msg msg = {};
 	unsigned int len;
 	unsigned int val;
 	u8 i;
 
-	if (irq1 & irq_tx_mask)
-		adv_cec_tx_raw_status(adv7511, irq1);
-
-	if (!(irq1 & ADV7511_INT1_CEC_RX_READY1))
-		return;
-
 	if (regmap_read(adv7511->regmap_cec,
-			ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
+			ADV7511_REG_CEC_RX_FRAME_LEN[rx_buf] + offset, &len))
 		return;
 
 	msg.len = len & 0x1f;
@@ -101,18 +93,76 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 
 	for (i = 0; i < msg.len; i++) {
 		regmap_read(adv7511->regmap_cec,
-			    i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
+			    i + ADV7511_REG_CEC_RX_FRAME_HDR[rx_buf] + offset,
+			    &val);
 		msg.msg[i] = val;
 	}
 
-	/* toggle to re-enable rx 1 */
-	regmap_write(adv7511->regmap_cec,
-		     ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
-	regmap_write(adv7511->regmap_cec,
-		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
+	/* Toggle RX Ready Clear bit to re-enable this RX buffer */
+	regmap_update_bits(adv7511->regmap_cec,
+			   ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf),
+			   BIT(rx_buf));
+	regmap_update_bits(adv7511->regmap_cec,
+			   ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf), 0);
+
 	cec_received_msg(adv7511->cec_adap, &msg);
 }
 
+void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
+{
+	unsigned int offset = adv7511->reg_cec_offset;
+	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
+				ADV7511_INT1_CEC_TX_ARBIT_LOST |
+				ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
+	const u32 irq_rx_mask = ADV7511_INT1_CEC_RX_READY1 |
+				ADV7511_INT1_CEC_RX_READY2 |
+				ADV7511_INT1_CEC_RX_READY3;
+	unsigned int rx_status;
+	int rx_order[3] = { -1, -1, -1 };
+	int i;
+
+	if (irq1 & irq_tx_mask)
+		adv_cec_tx_raw_status(adv7511, irq1);
+
+	if (!(irq1 & irq_rx_mask))
+		return;
+
+	if (regmap_read(adv7511->regmap_cec,
+			ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
+		return;
+
+	/*
+	 * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
+	 * buffers 0, 1, and 2 in bits [1:0], [3:2], and [5:4] respectively.
+	 * The values are to be interpreted as follows:
+	 *
+	 *   0 = buffer unused
+	 *   1 = buffer contains oldest received frame (if applicable)
+	 *   2 = buffer contains second oldest received frame (if applicable)
+	 *   3 = buffer contains third oldest received frame (if applicable)
+	 *
+	 * Fill rx_order with the sequence of RX buffer indices to
+	 * read from in order, where -1 indicates that there are no
+	 * more buffers to process.
+	 */
+	for (i = 0; i < 3; i++) {
+		unsigned int timestamp = (rx_status >> (2 * i)) & 0x3;
+
+		if (timestamp)
+			rx_order[timestamp - 1] = i;
+	}
+
+	/* Read CEC RX buffers in the appropriate order as prescribed above */
+	for (i = 0; i < 3; i++) {
+		int rx_buf = rx_order[i];
+
+		if (rx_buf < 0)
+			break;
+
+		adv7511_cec_rx(adv7511, rx_buf);
+	}
+}
+
 static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
@@ -126,11 +176,11 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 		regmap_update_bits(adv7511->regmap_cec,
 				   ADV7511_REG_CEC_CLK_DIV + offset,
 				   0x03, 0x01);
-		/* legacy mode and clear all rx buffers */
+		/* non-legacy mode and clear all rx buffers */
 		regmap_write(adv7511->regmap_cec,
-			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
+			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x0f);
 		regmap_write(adv7511->regmap_cec,
-			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
+			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
 		/* initially disable tx */
 		regmap_update_bits(adv7511->regmap_cec,
 				   ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
@@ -138,7 +188,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 		/* tx: ready */
 		/* tx: arbitration lost */
 		/* tx: retry timeout */
-		/* rx: ready 1 */
+		/* rx: ready 1-3 */
 		regmap_update_bits(adv7511->regmap,
 				   ADV7511_REG_INT_ENABLE(1), 0x3f,
 				   ADV7511_INT1_CEC_MASK);
@@ -304,9 +354,9 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 	regmap_write(adv7511->regmap_cec,
 		     ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
 
-	/* legacy mode */
+	/* non-legacy mode - use all three RX buffers */
 	regmap_write(adv7511->regmap_cec,
-		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
+		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
 
 	regmap_write(adv7511->regmap_cec,
 		     ADV7511_REG_CEC_CLK_DIV + offset,
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 0be65a1ffc47..ffb034daee45 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1030,10 +1030,19 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
 	reg -= adv7511->reg_cec_offset;
 
 	switch (reg) {
-	case ADV7511_REG_CEC_RX_FRAME_HDR:
-	case ADV7511_REG_CEC_RX_FRAME_DATA0...
-		ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
-	case ADV7511_REG_CEC_RX_FRAME_LEN:
+	case ADV7511_REG_CEC_RX1_FRAME_HDR:
+	case ADV7511_REG_CEC_RX1_FRAME_DATA0...
+		ADV7511_REG_CEC_RX1_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX1_FRAME_LEN:
+	case ADV7511_REG_CEC_RX2_FRAME_HDR:
+	case ADV7511_REG_CEC_RX2_FRAME_DATA0...
+		ADV7511_REG_CEC_RX2_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX2_FRAME_LEN:
+	case ADV7511_REG_CEC_RX3_FRAME_HDR:
+	case ADV7511_REG_CEC_RX3_FRAME_DATA0...
+		ADV7511_REG_CEC_RX3_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX3_FRAME_LEN:
+	case ADV7511_REG_CEC_RX_STATUS:
 	case ADV7511_REG_CEC_RX_BUFFERS:
 	case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
 		return true;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] drm: bridge: adv7511: use non-legacy mode for CEC RX
@ 2022-03-19 15:10   ` Alvin Šipraga
  0 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-03-19 15:10 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter
  Cc: linux-kernel, dri-devel, Alvin Šipraga

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The ADV7511 family of bridges supports two modes for CEC RX: legacy and
non-legacy mode. The only difference is whether the chip uses a single
CEC RX buffer, or uses all three available RX buffers. Currently the
adv7511 driver uses legacy mode.

While debugging a stall in CEC RX on an ADV7535, we reached out to
Analog Devices, who suggested to use non-legacy mode instead.  According
to the programming guide for the ADV7511 [1], and the register control
manual of the ADV7535 [2], this is the default behaviour on reset. As
previously stated, the adv7511 driver currently overrides this to legacy
mode.

This patch updates the adv7511 driver to instead use non-legacy mode
with all three CEC RX buffers. As a result of this change, we no longer
experience any stalling of CEC RX with the ADV7535. It is not known why
non-legacy mode solves this particular issue, but besides this, no
functional change is to be expected by this patch. Please note that this
has only been tested on an ADV7535.

What follows is a brief description of the non-legacy mode interrupt
handling behaviour. The programming guide in [1] gives a more detailed
explanation.

With three RX buffers, the interrupt handler checks the CEC_RX_STATUS
register (renamed from CEC_RX_ENABLE in this patch), which contains
2-bit psuedo-timestamps for each of the RX buffers. The RX timestamps
for each buffer represent the time of arrival for the CEC frame held in
a given buffer, with lower timestamp values indicating chronologically
older frames. A special value of 0 indicates that the given RX buffer
is inactive and should be skipped. The interrupt handler parses these
timestamps and then reads the active RX buffers in the prescribed order
using the same logic as before. Changes have been made to ensure that
the correct RX buffer is cleared after processing. This clearing
procesure also sets the timestamp of the given RX buffer to 0 to mark it
as inactive.

[1] https://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf
    cf. CEC Map, register 0x4A, bit 3, default value 1:
    0 = Use only buffer 0 to store CEC frames (Legacy mode)
    1 = Use all 3 buffers to stores the CEC frames (Non-legacy mode)

[2] The ADV7535 register control manual is under NDA, but trust me when
    I say that non-legacy CEC RX mode is the default here too. Here the
    register is offset by 0x70 and has an address of 0xBA in the DSI_CEC
    regiser map.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     | 26 +++++-
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 98 +++++++++++++++-----
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++-
 3 files changed, 109 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index da6d8ee2cd84..9e3bb8a8ee40 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -209,10 +209,16 @@
 #define ADV7511_REG_CEC_TX_ENABLE	0x11
 #define ADV7511_REG_CEC_TX_RETRY	0x12
 #define ADV7511_REG_CEC_TX_LOW_DRV_CNT	0x14
-#define ADV7511_REG_CEC_RX_FRAME_HDR	0x15
-#define ADV7511_REG_CEC_RX_FRAME_DATA0	0x16
-#define ADV7511_REG_CEC_RX_FRAME_LEN	0x25
-#define ADV7511_REG_CEC_RX_ENABLE	0x26
+#define ADV7511_REG_CEC_RX1_FRAME_HDR	0x15
+#define ADV7511_REG_CEC_RX1_FRAME_DATA0	0x16
+#define ADV7511_REG_CEC_RX1_FRAME_LEN	0x25
+#define ADV7511_REG_CEC_RX_STATUS	0x26
+#define ADV7511_REG_CEC_RX2_FRAME_HDR	0x27
+#define ADV7511_REG_CEC_RX2_FRAME_DATA0	0x28
+#define ADV7511_REG_CEC_RX2_FRAME_LEN	0x37
+#define ADV7511_REG_CEC_RX3_FRAME_HDR	0x38
+#define ADV7511_REG_CEC_RX3_FRAME_DATA0	0x39
+#define ADV7511_REG_CEC_RX3_FRAME_LEN	0x48
 #define ADV7511_REG_CEC_RX_BUFFERS	0x4a
 #define ADV7511_REG_CEC_LOG_ADDR_MASK	0x4b
 #define ADV7511_REG_CEC_LOG_ADDR_0_1	0x4c
@@ -220,6 +226,18 @@
 #define ADV7511_REG_CEC_CLK_DIV		0x4e
 #define ADV7511_REG_CEC_SOFT_RESET	0x50
 
+static const u8 ADV7511_REG_CEC_RX_FRAME_HDR[] = {
+	ADV7511_REG_CEC_RX1_FRAME_HDR,
+	ADV7511_REG_CEC_RX2_FRAME_HDR,
+	ADV7511_REG_CEC_RX3_FRAME_HDR,
+};
+
+static const u8 ADV7511_REG_CEC_RX_FRAME_LEN[] = {
+	ADV7511_REG_CEC_RX1_FRAME_LEN,
+	ADV7511_REG_CEC_RX2_FRAME_LEN,
+	ADV7511_REG_CEC_RX3_FRAME_LEN,
+};
+
 #define ADV7533_REG_CEC_OFFSET		0x70
 
 enum adv7511_input_clock {
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
index 1f619389e201..399f625a50c8 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
@@ -17,7 +17,8 @@
 
 #define ADV7511_INT1_CEC_MASK \
 	(ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
-	 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
+	 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1 | \
+	 ADV7511_INT1_CEC_RX_READY2 | ADV7511_INT1_CEC_RX_READY3)
 
 static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 {
@@ -70,25 +71,16 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
 	}
 }
 
-void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
+static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
 {
 	unsigned int offset = adv7511->reg_cec_offset;
-	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
-				ADV7511_INT1_CEC_TX_ARBIT_LOST |
-				ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
 	struct cec_msg msg = {};
 	unsigned int len;
 	unsigned int val;
 	u8 i;
 
-	if (irq1 & irq_tx_mask)
-		adv_cec_tx_raw_status(adv7511, irq1);
-
-	if (!(irq1 & ADV7511_INT1_CEC_RX_READY1))
-		return;
-
 	if (regmap_read(adv7511->regmap_cec,
-			ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
+			ADV7511_REG_CEC_RX_FRAME_LEN[rx_buf] + offset, &len))
 		return;
 
 	msg.len = len & 0x1f;
@@ -101,18 +93,76 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
 
 	for (i = 0; i < msg.len; i++) {
 		regmap_read(adv7511->regmap_cec,
-			    i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
+			    i + ADV7511_REG_CEC_RX_FRAME_HDR[rx_buf] + offset,
+			    &val);
 		msg.msg[i] = val;
 	}
 
-	/* toggle to re-enable rx 1 */
-	regmap_write(adv7511->regmap_cec,
-		     ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
-	regmap_write(adv7511->regmap_cec,
-		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
+	/* Toggle RX Ready Clear bit to re-enable this RX buffer */
+	regmap_update_bits(adv7511->regmap_cec,
+			   ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf),
+			   BIT(rx_buf));
+	regmap_update_bits(adv7511->regmap_cec,
+			   ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf), 0);
+
 	cec_received_msg(adv7511->cec_adap, &msg);
 }
 
+void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
+{
+	unsigned int offset = adv7511->reg_cec_offset;
+	const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
+				ADV7511_INT1_CEC_TX_ARBIT_LOST |
+				ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
+	const u32 irq_rx_mask = ADV7511_INT1_CEC_RX_READY1 |
+				ADV7511_INT1_CEC_RX_READY2 |
+				ADV7511_INT1_CEC_RX_READY3;
+	unsigned int rx_status;
+	int rx_order[3] = { -1, -1, -1 };
+	int i;
+
+	if (irq1 & irq_tx_mask)
+		adv_cec_tx_raw_status(adv7511, irq1);
+
+	if (!(irq1 & irq_rx_mask))
+		return;
+
+	if (regmap_read(adv7511->regmap_cec,
+			ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
+		return;
+
+	/*
+	 * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
+	 * buffers 0, 1, and 2 in bits [1:0], [3:2], and [5:4] respectively.
+	 * The values are to be interpreted as follows:
+	 *
+	 *   0 = buffer unused
+	 *   1 = buffer contains oldest received frame (if applicable)
+	 *   2 = buffer contains second oldest received frame (if applicable)
+	 *   3 = buffer contains third oldest received frame (if applicable)
+	 *
+	 * Fill rx_order with the sequence of RX buffer indices to
+	 * read from in order, where -1 indicates that there are no
+	 * more buffers to process.
+	 */
+	for (i = 0; i < 3; i++) {
+		unsigned int timestamp = (rx_status >> (2 * i)) & 0x3;
+
+		if (timestamp)
+			rx_order[timestamp - 1] = i;
+	}
+
+	/* Read CEC RX buffers in the appropriate order as prescribed above */
+	for (i = 0; i < 3; i++) {
+		int rx_buf = rx_order[i];
+
+		if (rx_buf < 0)
+			break;
+
+		adv7511_cec_rx(adv7511, rx_buf);
+	}
+}
+
 static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct adv7511 *adv7511 = cec_get_drvdata(adap);
@@ -126,11 +176,11 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 		regmap_update_bits(adv7511->regmap_cec,
 				   ADV7511_REG_CEC_CLK_DIV + offset,
 				   0x03, 0x01);
-		/* legacy mode and clear all rx buffers */
+		/* non-legacy mode and clear all rx buffers */
 		regmap_write(adv7511->regmap_cec,
-			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
+			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x0f);
 		regmap_write(adv7511->regmap_cec,
-			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
+			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
 		/* initially disable tx */
 		regmap_update_bits(adv7511->regmap_cec,
 				   ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
@@ -138,7 +188,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
 		/* tx: ready */
 		/* tx: arbitration lost */
 		/* tx: retry timeout */
-		/* rx: ready 1 */
+		/* rx: ready 1-3 */
 		regmap_update_bits(adv7511->regmap,
 				   ADV7511_REG_INT_ENABLE(1), 0x3f,
 				   ADV7511_INT1_CEC_MASK);
@@ -304,9 +354,9 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 	regmap_write(adv7511->regmap_cec,
 		     ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
 
-	/* legacy mode */
+	/* non-legacy mode - use all three RX buffers */
 	regmap_write(adv7511->regmap_cec,
-		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
+		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
 
 	regmap_write(adv7511->regmap_cec,
 		     ADV7511_REG_CEC_CLK_DIV + offset,
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 0be65a1ffc47..ffb034daee45 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1030,10 +1030,19 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
 	reg -= adv7511->reg_cec_offset;
 
 	switch (reg) {
-	case ADV7511_REG_CEC_RX_FRAME_HDR:
-	case ADV7511_REG_CEC_RX_FRAME_DATA0...
-		ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
-	case ADV7511_REG_CEC_RX_FRAME_LEN:
+	case ADV7511_REG_CEC_RX1_FRAME_HDR:
+	case ADV7511_REG_CEC_RX1_FRAME_DATA0...
+		ADV7511_REG_CEC_RX1_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX1_FRAME_LEN:
+	case ADV7511_REG_CEC_RX2_FRAME_HDR:
+	case ADV7511_REG_CEC_RX2_FRAME_DATA0...
+		ADV7511_REG_CEC_RX2_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX2_FRAME_LEN:
+	case ADV7511_REG_CEC_RX3_FRAME_HDR:
+	case ADV7511_REG_CEC_RX3_FRAME_DATA0...
+		ADV7511_REG_CEC_RX3_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX3_FRAME_LEN:
+	case ADV7511_REG_CEC_RX_STATUS:
 	case ADV7511_REG_CEC_RX_BUFFERS:
 	case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
 		return true;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] drm: bridge: adv7511: enable CEC support for ADV7535
  2022-03-19 15:10   ` Alvin Šipraga
@ 2022-04-22 14:25     ` Robert Foss
  -1 siblings, 0 replies; 14+ messages in thread
From: Robert Foss @ 2022-04-22 14:25 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, David Airlie, Daniel Vetter, Alvin Šipraga,
	dri-devel, linux-kernel

On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> Like the ADV7533, the ADV7535 has an offset for the CEC register map,
> and it is the same value (ADV7533_REG_CEC_OFFSET = 0x70).
>
> Rather than testing for numerous chip types in the offset calculations
> throughout the driver, just compute it during driver probe and put it in
> the private adv7511 data structure.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 +
>  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 18 ++++++------------
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  5 +++--
>  3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 6a882891d91c..da6d8ee2cd84 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -335,6 +335,7 @@ struct adv7511 {
>
>         struct regmap *regmap;
>         struct regmap *regmap_cec;
> +       unsigned int reg_cec_offset;
>         enum drm_connector_status status;
>         bool powered;
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 28d9becc939c..1f619389e201 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -21,8 +21,7 @@
>
>  static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>  {
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         unsigned int val;
>
>         if (regmap_read(adv7511->regmap_cec,
> @@ -73,8 +72,7 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>
>  void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  {
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
>                                 ADV7511_INT1_CEC_TX_ARBIT_LOST |
>                                 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
> @@ -118,8 +116,7 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>
>         if (adv7511->i2c_cec == NULL)
>                 return -EIO;
> @@ -165,8 +162,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         unsigned int i, free_idx = ADV7511_MAX_ADDRS;
>
>         if (!adv7511->cec_enabled_adap)
> @@ -235,8 +231,7 @@ static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>                                      u32 signal_free_time, struct cec_msg *msg)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         u8 len = msg->len;
>         unsigned int i;
>
> @@ -289,8 +284,7 @@ static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
>
>  int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>  {
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                               ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         int ret = adv7511_cec_parse_dt(dev, adv7511);
>
>         if (ret)
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 005bf18682ff..0be65a1ffc47 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1027,8 +1027,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>         struct i2c_client *i2c = to_i2c_client(dev);
>         struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>
> -       if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> -               reg -= ADV7533_REG_CEC_OFFSET;
> +       reg -= adv7511->reg_cec_offset;
>
>         switch (reg) {
>         case ADV7511_REG_CEC_RX_FRAME_HDR:
> @@ -1073,6 +1072,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>                 ret = adv7533_patch_cec_registers(adv);
>                 if (ret)
>                         goto err;
> +
> +               adv->reg_cec_offset = ADV7533_REG_CEC_OFFSET;
>         }
>
>         return 0;

Reviewed-by: Robert Foss <robert.foss@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] drm: bridge: adv7511: enable CEC support for ADV7535
@ 2022-04-22 14:25     ` Robert Foss
  0 siblings, 0 replies; 14+ messages in thread
From: Robert Foss @ 2022-04-22 14:25 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Jonas Karlman, David Airlie, dri-devel, Neil Armstrong,
	linux-kernel, Jernej Skrabec, Laurent Pinchart, Andrzej Hajda,
	Alvin Šipraga

On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> Like the ADV7533, the ADV7535 has an offset for the CEC register map,
> and it is the same value (ADV7533_REG_CEC_OFFSET = 0x70).
>
> Rather than testing for numerous chip types in the offset calculations
> throughout the driver, just compute it during driver probe and put it in
> the private adv7511 data structure.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  1 +
>  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 18 ++++++------------
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  5 +++--
>  3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 6a882891d91c..da6d8ee2cd84 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -335,6 +335,7 @@ struct adv7511 {
>
>         struct regmap *regmap;
>         struct regmap *regmap_cec;
> +       unsigned int reg_cec_offset;
>         enum drm_connector_status status;
>         bool powered;
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 28d9becc939c..1f619389e201 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -21,8 +21,7 @@
>
>  static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>  {
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         unsigned int val;
>
>         if (regmap_read(adv7511->regmap_cec,
> @@ -73,8 +72,7 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>
>  void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  {
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
>                                 ADV7511_INT1_CEC_TX_ARBIT_LOST |
>                                 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
> @@ -118,8 +116,7 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>
>         if (adv7511->i2c_cec == NULL)
>                 return -EIO;
> @@ -165,8 +162,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         unsigned int i, free_idx = ADV7511_MAX_ADDRS;
>
>         if (!adv7511->cec_enabled_adap)
> @@ -235,8 +231,7 @@ static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>                                      u32 signal_free_time, struct cec_msg *msg)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                       ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         u8 len = msg->len;
>         unsigned int i;
>
> @@ -289,8 +284,7 @@ static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
>
>  int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>  {
> -       unsigned int offset = adv7511->type == ADV7533 ?
> -                                               ADV7533_REG_CEC_OFFSET : 0;
> +       unsigned int offset = adv7511->reg_cec_offset;
>         int ret = adv7511_cec_parse_dt(dev, adv7511);
>
>         if (ret)
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 005bf18682ff..0be65a1ffc47 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1027,8 +1027,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>         struct i2c_client *i2c = to_i2c_client(dev);
>         struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>
> -       if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> -               reg -= ADV7533_REG_CEC_OFFSET;
> +       reg -= adv7511->reg_cec_offset;
>
>         switch (reg) {
>         case ADV7511_REG_CEC_RX_FRAME_HDR:
> @@ -1073,6 +1072,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>                 ret = adv7533_patch_cec_registers(adv);
>                 if (ret)
>                         goto err;
> +
> +               adv->reg_cec_offset = ADV7533_REG_CEC_OFFSET;
>         }
>
>         return 0;

Reviewed-by: Robert Foss <robert.foss@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] drm: bridge: adv7511: use non-legacy mode for CEC RX
  2022-03-19 15:10   ` Alvin Šipraga
@ 2022-04-22 14:36     ` Robert Foss
  -1 siblings, 0 replies; 14+ messages in thread
From: Robert Foss @ 2022-04-22 14:36 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, David Airlie, Daniel Vetter, Alvin Šipraga,
	dri-devel, linux-kernel

On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> The ADV7511 family of bridges supports two modes for CEC RX: legacy and
> non-legacy mode. The only difference is whether the chip uses a single
> CEC RX buffer, or uses all three available RX buffers. Currently the
> adv7511 driver uses legacy mode.
>
> While debugging a stall in CEC RX on an ADV7535, we reached out to
> Analog Devices, who suggested to use non-legacy mode instead.  According
> to the programming guide for the ADV7511 [1], and the register control
> manual of the ADV7535 [2], this is the default behaviour on reset. As
> previously stated, the adv7511 driver currently overrides this to legacy
> mode.
>
> This patch updates the adv7511 driver to instead use non-legacy mode
> with all three CEC RX buffers. As a result of this change, we no longer
> experience any stalling of CEC RX with the ADV7535. It is not known why
> non-legacy mode solves this particular issue, but besides this, no
> functional change is to be expected by this patch. Please note that this
> has only been tested on an ADV7535.
>
> What follows is a brief description of the non-legacy mode interrupt
> handling behaviour. The programming guide in [1] gives a more detailed
> explanation.
>
> With three RX buffers, the interrupt handler checks the CEC_RX_STATUS
> register (renamed from CEC_RX_ENABLE in this patch), which contains
> 2-bit psuedo-timestamps for each of the RX buffers. The RX timestamps
> for each buffer represent the time of arrival for the CEC frame held in
> a given buffer, with lower timestamp values indicating chronologically
> older frames. A special value of 0 indicates that the given RX buffer
> is inactive and should be skipped. The interrupt handler parses these
> timestamps and then reads the active RX buffers in the prescribed order
> using the same logic as before. Changes have been made to ensure that
> the correct RX buffer is cleared after processing. This clearing
> procesure also sets the timestamp of the given RX buffer to 0 to mark it
> as inactive.
>
> [1] https://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf
>     cf. CEC Map, register 0x4A, bit 3, default value 1:
>     0 = Use only buffer 0 to store CEC frames (Legacy mode)
>     1 = Use all 3 buffers to stores the CEC frames (Non-legacy mode)
>
> [2] The ADV7535 register control manual is under NDA, but trust me when
>     I say that non-legacy CEC RX mode is the default here too. Here the
>     register is offset by 0x70 and has an address of 0xBA in the DSI_CEC
>     regiser map.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     | 26 +++++-
>  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 98 +++++++++++++++-----
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++-
>  3 files changed, 109 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index da6d8ee2cd84..9e3bb8a8ee40 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -209,10 +209,16 @@
>  #define ADV7511_REG_CEC_TX_ENABLE      0x11
>  #define ADV7511_REG_CEC_TX_RETRY       0x12
>  #define ADV7511_REG_CEC_TX_LOW_DRV_CNT 0x14
> -#define ADV7511_REG_CEC_RX_FRAME_HDR   0x15
> -#define ADV7511_REG_CEC_RX_FRAME_DATA0 0x16
> -#define ADV7511_REG_CEC_RX_FRAME_LEN   0x25
> -#define ADV7511_REG_CEC_RX_ENABLE      0x26
> +#define ADV7511_REG_CEC_RX1_FRAME_HDR  0x15
> +#define ADV7511_REG_CEC_RX1_FRAME_DATA0        0x16
> +#define ADV7511_REG_CEC_RX1_FRAME_LEN  0x25
> +#define ADV7511_REG_CEC_RX_STATUS      0x26
> +#define ADV7511_REG_CEC_RX2_FRAME_HDR  0x27
> +#define ADV7511_REG_CEC_RX2_FRAME_DATA0        0x28
> +#define ADV7511_REG_CEC_RX2_FRAME_LEN  0x37
> +#define ADV7511_REG_CEC_RX3_FRAME_HDR  0x38
> +#define ADV7511_REG_CEC_RX3_FRAME_DATA0        0x39
> +#define ADV7511_REG_CEC_RX3_FRAME_LEN  0x48
>  #define ADV7511_REG_CEC_RX_BUFFERS     0x4a
>  #define ADV7511_REG_CEC_LOG_ADDR_MASK  0x4b
>  #define ADV7511_REG_CEC_LOG_ADDR_0_1   0x4c
> @@ -220,6 +226,18 @@
>  #define ADV7511_REG_CEC_CLK_DIV                0x4e
>  #define ADV7511_REG_CEC_SOFT_RESET     0x50
>
> +static const u8 ADV7511_REG_CEC_RX_FRAME_HDR[] = {
> +       ADV7511_REG_CEC_RX1_FRAME_HDR,
> +       ADV7511_REG_CEC_RX2_FRAME_HDR,
> +       ADV7511_REG_CEC_RX3_FRAME_HDR,
> +};
> +
> +static const u8 ADV7511_REG_CEC_RX_FRAME_LEN[] = {
> +       ADV7511_REG_CEC_RX1_FRAME_LEN,
> +       ADV7511_REG_CEC_RX2_FRAME_LEN,
> +       ADV7511_REG_CEC_RX3_FRAME_LEN,
> +};
> +
>  #define ADV7533_REG_CEC_OFFSET         0x70
>
>  enum adv7511_input_clock {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 1f619389e201..399f625a50c8 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -17,7 +17,8 @@
>
>  #define ADV7511_INT1_CEC_MASK \
>         (ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
> -        ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
> +        ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1 | \
> +        ADV7511_INT1_CEC_RX_READY2 | ADV7511_INT1_CEC_RX_READY3)
>
>  static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>  {
> @@ -70,25 +71,16 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>         }
>  }
>
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
>  {
>         unsigned int offset = adv7511->reg_cec_offset;
> -       const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> -                               ADV7511_INT1_CEC_TX_ARBIT_LOST |
> -                               ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
>         struct cec_msg msg = {};
>         unsigned int len;
>         unsigned int val;
>         u8 i;
>
> -       if (irq1 & irq_tx_mask)
> -               adv_cec_tx_raw_status(adv7511, irq1);
> -
> -       if (!(irq1 & ADV7511_INT1_CEC_RX_READY1))
> -               return;
> -
>         if (regmap_read(adv7511->regmap_cec,
> -                       ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
> +                       ADV7511_REG_CEC_RX_FRAME_LEN[rx_buf] + offset, &len))
>                 return;
>
>         msg.len = len & 0x1f;
> @@ -101,18 +93,76 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>
>         for (i = 0; i < msg.len; i++) {
>                 regmap_read(adv7511->regmap_cec,
> -                           i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
> +                           i + ADV7511_REG_CEC_RX_FRAME_HDR[rx_buf] + offset,
> +                           &val);
>                 msg.msg[i] = val;
>         }
>
> -       /* toggle to re-enable rx 1 */
> -       regmap_write(adv7511->regmap_cec,
> -                    ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
> -       regmap_write(adv7511->regmap_cec,
> -                    ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
> +       /* Toggle RX Ready Clear bit to re-enable this RX buffer */
> +       regmap_update_bits(adv7511->regmap_cec,
> +                          ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf),
> +                          BIT(rx_buf));
> +       regmap_update_bits(adv7511->regmap_cec,
> +                          ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf), 0);
> +
>         cec_received_msg(adv7511->cec_adap, &msg);
>  }
>
> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +{
> +       unsigned int offset = adv7511->reg_cec_offset;
> +       const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> +                               ADV7511_INT1_CEC_TX_ARBIT_LOST |
> +                               ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
> +       const u32 irq_rx_mask = ADV7511_INT1_CEC_RX_READY1 |
> +                               ADV7511_INT1_CEC_RX_READY2 |
> +                               ADV7511_INT1_CEC_RX_READY3;
> +       unsigned int rx_status;
> +       int rx_order[3] = { -1, -1, -1 };
> +       int i;
> +
> +       if (irq1 & irq_tx_mask)
> +               adv_cec_tx_raw_status(adv7511, irq1);
> +
> +       if (!(irq1 & irq_rx_mask))
> +               return;
> +
> +       if (regmap_read(adv7511->regmap_cec,
> +                       ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
> +               return;
> +
> +       /*
> +        * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
> +        * buffers 0, 1, and 2 in bits [1:0], [3:2], and [5:4] respectively.
> +        * The values are to be interpreted as follows:
> +        *
> +        *   0 = buffer unused
> +        *   1 = buffer contains oldest received frame (if applicable)
> +        *   2 = buffer contains second oldest received frame (if applicable)
> +        *   3 = buffer contains third oldest received frame (if applicable)
> +        *
> +        * Fill rx_order with the sequence of RX buffer indices to
> +        * read from in order, where -1 indicates that there are no
> +        * more buffers to process.
> +        */
> +       for (i = 0; i < 3; i++) {
> +               unsigned int timestamp = (rx_status >> (2 * i)) & 0x3;
> +
> +               if (timestamp)
> +                       rx_order[timestamp - 1] = i;
> +       }
> +
> +       /* Read CEC RX buffers in the appropriate order as prescribed above */
> +       for (i = 0; i < 3; i++) {
> +               int rx_buf = rx_order[i];
> +
> +               if (rx_buf < 0)
> +                       break;
> +
> +               adv7511_cec_rx(adv7511, rx_buf);
> +       }
> +}
> +
>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> @@ -126,11 +176,11 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>                 regmap_update_bits(adv7511->regmap_cec,
>                                    ADV7511_REG_CEC_CLK_DIV + offset,
>                                    0x03, 0x01);
> -               /* legacy mode and clear all rx buffers */
> +               /* non-legacy mode and clear all rx buffers */
>                 regmap_write(adv7511->regmap_cec,
> -                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
> +                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0x0f);
>                 regmap_write(adv7511->regmap_cec,
> -                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
> +                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
>                 /* initially disable tx */
>                 regmap_update_bits(adv7511->regmap_cec,
>                                    ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
> @@ -138,7 +188,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>                 /* tx: ready */
>                 /* tx: arbitration lost */
>                 /* tx: retry timeout */
> -               /* rx: ready 1 */
> +               /* rx: ready 1-3 */
>                 regmap_update_bits(adv7511->regmap,
>                                    ADV7511_REG_INT_ENABLE(1), 0x3f,
>                                    ADV7511_INT1_CEC_MASK);
> @@ -304,9 +354,9 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>         regmap_write(adv7511->regmap_cec,
>                      ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
>
> -       /* legacy mode */
> +       /* non-legacy mode - use all three RX buffers */
>         regmap_write(adv7511->regmap_cec,
> -                    ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
> +                    ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
>
>         regmap_write(adv7511->regmap_cec,
>                      ADV7511_REG_CEC_CLK_DIV + offset,
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 0be65a1ffc47..ffb034daee45 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1030,10 +1030,19 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>         reg -= adv7511->reg_cec_offset;
>
>         switch (reg) {
> -       case ADV7511_REG_CEC_RX_FRAME_HDR:
> -       case ADV7511_REG_CEC_RX_FRAME_DATA0...
> -               ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
> -       case ADV7511_REG_CEC_RX_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX1_FRAME_HDR:
> +       case ADV7511_REG_CEC_RX1_FRAME_DATA0...
> +               ADV7511_REG_CEC_RX1_FRAME_DATA0 + 14:
> +       case ADV7511_REG_CEC_RX1_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX2_FRAME_HDR:
> +       case ADV7511_REG_CEC_RX2_FRAME_DATA0...
> +               ADV7511_REG_CEC_RX2_FRAME_DATA0 + 14:
> +       case ADV7511_REG_CEC_RX2_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX3_FRAME_HDR:
> +       case ADV7511_REG_CEC_RX3_FRAME_DATA0...
> +               ADV7511_REG_CEC_RX3_FRAME_DATA0 + 14:
> +       case ADV7511_REG_CEC_RX3_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX_STATUS:
>         case ADV7511_REG_CEC_RX_BUFFERS:
>         case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
>                 return true;

This is a bit of a nitpick, but the syntax of these "case X...Y"
statements was kind of hard to parse, do you mind putting them on one
line?

With or without the above suggestion fixed, r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] drm: bridge: adv7511: use non-legacy mode for CEC RX
@ 2022-04-22 14:36     ` Robert Foss
  0 siblings, 0 replies; 14+ messages in thread
From: Robert Foss @ 2022-04-22 14:36 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Jonas Karlman, David Airlie, dri-devel, Neil Armstrong,
	linux-kernel, Jernej Skrabec, Laurent Pinchart, Andrzej Hajda,
	Alvin Šipraga

On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> The ADV7511 family of bridges supports two modes for CEC RX: legacy and
> non-legacy mode. The only difference is whether the chip uses a single
> CEC RX buffer, or uses all three available RX buffers. Currently the
> adv7511 driver uses legacy mode.
>
> While debugging a stall in CEC RX on an ADV7535, we reached out to
> Analog Devices, who suggested to use non-legacy mode instead.  According
> to the programming guide for the ADV7511 [1], and the register control
> manual of the ADV7535 [2], this is the default behaviour on reset. As
> previously stated, the adv7511 driver currently overrides this to legacy
> mode.
>
> This patch updates the adv7511 driver to instead use non-legacy mode
> with all three CEC RX buffers. As a result of this change, we no longer
> experience any stalling of CEC RX with the ADV7535. It is not known why
> non-legacy mode solves this particular issue, but besides this, no
> functional change is to be expected by this patch. Please note that this
> has only been tested on an ADV7535.
>
> What follows is a brief description of the non-legacy mode interrupt
> handling behaviour. The programming guide in [1] gives a more detailed
> explanation.
>
> With three RX buffers, the interrupt handler checks the CEC_RX_STATUS
> register (renamed from CEC_RX_ENABLE in this patch), which contains
> 2-bit psuedo-timestamps for each of the RX buffers. The RX timestamps
> for each buffer represent the time of arrival for the CEC frame held in
> a given buffer, with lower timestamp values indicating chronologically
> older frames. A special value of 0 indicates that the given RX buffer
> is inactive and should be skipped. The interrupt handler parses these
> timestamps and then reads the active RX buffers in the prescribed order
> using the same logic as before. Changes have been made to ensure that
> the correct RX buffer is cleared after processing. This clearing
> procesure also sets the timestamp of the given RX buffer to 0 to mark it
> as inactive.
>
> [1] https://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf
>     cf. CEC Map, register 0x4A, bit 3, default value 1:
>     0 = Use only buffer 0 to store CEC frames (Legacy mode)
>     1 = Use all 3 buffers to stores the CEC frames (Non-legacy mode)
>
> [2] The ADV7535 register control manual is under NDA, but trust me when
>     I say that non-legacy CEC RX mode is the default here too. Here the
>     register is offset by 0x70 and has an address of 0xBA in the DSI_CEC
>     regiser map.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     | 26 +++++-
>  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 98 +++++++++++++++-----
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++-
>  3 files changed, 109 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index da6d8ee2cd84..9e3bb8a8ee40 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -209,10 +209,16 @@
>  #define ADV7511_REG_CEC_TX_ENABLE      0x11
>  #define ADV7511_REG_CEC_TX_RETRY       0x12
>  #define ADV7511_REG_CEC_TX_LOW_DRV_CNT 0x14
> -#define ADV7511_REG_CEC_RX_FRAME_HDR   0x15
> -#define ADV7511_REG_CEC_RX_FRAME_DATA0 0x16
> -#define ADV7511_REG_CEC_RX_FRAME_LEN   0x25
> -#define ADV7511_REG_CEC_RX_ENABLE      0x26
> +#define ADV7511_REG_CEC_RX1_FRAME_HDR  0x15
> +#define ADV7511_REG_CEC_RX1_FRAME_DATA0        0x16
> +#define ADV7511_REG_CEC_RX1_FRAME_LEN  0x25
> +#define ADV7511_REG_CEC_RX_STATUS      0x26
> +#define ADV7511_REG_CEC_RX2_FRAME_HDR  0x27
> +#define ADV7511_REG_CEC_RX2_FRAME_DATA0        0x28
> +#define ADV7511_REG_CEC_RX2_FRAME_LEN  0x37
> +#define ADV7511_REG_CEC_RX3_FRAME_HDR  0x38
> +#define ADV7511_REG_CEC_RX3_FRAME_DATA0        0x39
> +#define ADV7511_REG_CEC_RX3_FRAME_LEN  0x48
>  #define ADV7511_REG_CEC_RX_BUFFERS     0x4a
>  #define ADV7511_REG_CEC_LOG_ADDR_MASK  0x4b
>  #define ADV7511_REG_CEC_LOG_ADDR_0_1   0x4c
> @@ -220,6 +226,18 @@
>  #define ADV7511_REG_CEC_CLK_DIV                0x4e
>  #define ADV7511_REG_CEC_SOFT_RESET     0x50
>
> +static const u8 ADV7511_REG_CEC_RX_FRAME_HDR[] = {
> +       ADV7511_REG_CEC_RX1_FRAME_HDR,
> +       ADV7511_REG_CEC_RX2_FRAME_HDR,
> +       ADV7511_REG_CEC_RX3_FRAME_HDR,
> +};
> +
> +static const u8 ADV7511_REG_CEC_RX_FRAME_LEN[] = {
> +       ADV7511_REG_CEC_RX1_FRAME_LEN,
> +       ADV7511_REG_CEC_RX2_FRAME_LEN,
> +       ADV7511_REG_CEC_RX3_FRAME_LEN,
> +};
> +
>  #define ADV7533_REG_CEC_OFFSET         0x70
>
>  enum adv7511_input_clock {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 1f619389e201..399f625a50c8 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -17,7 +17,8 @@
>
>  #define ADV7511_INT1_CEC_MASK \
>         (ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
> -        ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
> +        ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1 | \
> +        ADV7511_INT1_CEC_RX_READY2 | ADV7511_INT1_CEC_RX_READY3)
>
>  static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>  {
> @@ -70,25 +71,16 @@ static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>         }
>  }
>
> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
>  {
>         unsigned int offset = adv7511->reg_cec_offset;
> -       const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> -                               ADV7511_INT1_CEC_TX_ARBIT_LOST |
> -                               ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
>         struct cec_msg msg = {};
>         unsigned int len;
>         unsigned int val;
>         u8 i;
>
> -       if (irq1 & irq_tx_mask)
> -               adv_cec_tx_raw_status(adv7511, irq1);
> -
> -       if (!(irq1 & ADV7511_INT1_CEC_RX_READY1))
> -               return;
> -
>         if (regmap_read(adv7511->regmap_cec,
> -                       ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
> +                       ADV7511_REG_CEC_RX_FRAME_LEN[rx_buf] + offset, &len))
>                 return;
>
>         msg.len = len & 0x1f;
> @@ -101,18 +93,76 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>
>         for (i = 0; i < msg.len; i++) {
>                 regmap_read(adv7511->regmap_cec,
> -                           i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
> +                           i + ADV7511_REG_CEC_RX_FRAME_HDR[rx_buf] + offset,
> +                           &val);
>                 msg.msg[i] = val;
>         }
>
> -       /* toggle to re-enable rx 1 */
> -       regmap_write(adv7511->regmap_cec,
> -                    ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
> -       regmap_write(adv7511->regmap_cec,
> -                    ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
> +       /* Toggle RX Ready Clear bit to re-enable this RX buffer */
> +       regmap_update_bits(adv7511->regmap_cec,
> +                          ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf),
> +                          BIT(rx_buf));
> +       regmap_update_bits(adv7511->regmap_cec,
> +                          ADV7511_REG_CEC_RX_BUFFERS + offset, BIT(rx_buf), 0);
> +
>         cec_received_msg(adv7511->cec_adap, &msg);
>  }
>
> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +{
> +       unsigned int offset = adv7511->reg_cec_offset;
> +       const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
> +                               ADV7511_INT1_CEC_TX_ARBIT_LOST |
> +                               ADV7511_INT1_CEC_TX_RETRY_TIMEOUT;
> +       const u32 irq_rx_mask = ADV7511_INT1_CEC_RX_READY1 |
> +                               ADV7511_INT1_CEC_RX_READY2 |
> +                               ADV7511_INT1_CEC_RX_READY3;
> +       unsigned int rx_status;
> +       int rx_order[3] = { -1, -1, -1 };
> +       int i;
> +
> +       if (irq1 & irq_tx_mask)
> +               adv_cec_tx_raw_status(adv7511, irq1);
> +
> +       if (!(irq1 & irq_rx_mask))
> +               return;
> +
> +       if (regmap_read(adv7511->regmap_cec,
> +                       ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
> +               return;
> +
> +       /*
> +        * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
> +        * buffers 0, 1, and 2 in bits [1:0], [3:2], and [5:4] respectively.
> +        * The values are to be interpreted as follows:
> +        *
> +        *   0 = buffer unused
> +        *   1 = buffer contains oldest received frame (if applicable)
> +        *   2 = buffer contains second oldest received frame (if applicable)
> +        *   3 = buffer contains third oldest received frame (if applicable)
> +        *
> +        * Fill rx_order with the sequence of RX buffer indices to
> +        * read from in order, where -1 indicates that there are no
> +        * more buffers to process.
> +        */
> +       for (i = 0; i < 3; i++) {
> +               unsigned int timestamp = (rx_status >> (2 * i)) & 0x3;
> +
> +               if (timestamp)
> +                       rx_order[timestamp - 1] = i;
> +       }
> +
> +       /* Read CEC RX buffers in the appropriate order as prescribed above */
> +       for (i = 0; i < 3; i++) {
> +               int rx_buf = rx_order[i];
> +
> +               if (rx_buf < 0)
> +                       break;
> +
> +               adv7511_cec_rx(adv7511, rx_buf);
> +       }
> +}
> +
>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  {
>         struct adv7511 *adv7511 = cec_get_drvdata(adap);
> @@ -126,11 +176,11 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>                 regmap_update_bits(adv7511->regmap_cec,
>                                    ADV7511_REG_CEC_CLK_DIV + offset,
>                                    0x03, 0x01);
> -               /* legacy mode and clear all rx buffers */
> +               /* non-legacy mode and clear all rx buffers */
>                 regmap_write(adv7511->regmap_cec,
> -                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
> +                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0x0f);
>                 regmap_write(adv7511->regmap_cec,
> -                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
> +                            ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
>                 /* initially disable tx */
>                 regmap_update_bits(adv7511->regmap_cec,
>                                    ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
> @@ -138,7 +188,7 @@ static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>                 /* tx: ready */
>                 /* tx: arbitration lost */
>                 /* tx: retry timeout */
> -               /* rx: ready 1 */
> +               /* rx: ready 1-3 */
>                 regmap_update_bits(adv7511->regmap,
>                                    ADV7511_REG_INT_ENABLE(1), 0x3f,
>                                    ADV7511_INT1_CEC_MASK);
> @@ -304,9 +354,9 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>         regmap_write(adv7511->regmap_cec,
>                      ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
>
> -       /* legacy mode */
> +       /* non-legacy mode - use all three RX buffers */
>         regmap_write(adv7511->regmap_cec,
> -                    ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
> +                    ADV7511_REG_CEC_RX_BUFFERS + offset, 0x08);
>
>         regmap_write(adv7511->regmap_cec,
>                      ADV7511_REG_CEC_CLK_DIV + offset,
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 0be65a1ffc47..ffb034daee45 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1030,10 +1030,19 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>         reg -= adv7511->reg_cec_offset;
>
>         switch (reg) {
> -       case ADV7511_REG_CEC_RX_FRAME_HDR:
> -       case ADV7511_REG_CEC_RX_FRAME_DATA0...
> -               ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
> -       case ADV7511_REG_CEC_RX_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX1_FRAME_HDR:
> +       case ADV7511_REG_CEC_RX1_FRAME_DATA0...
> +               ADV7511_REG_CEC_RX1_FRAME_DATA0 + 14:
> +       case ADV7511_REG_CEC_RX1_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX2_FRAME_HDR:
> +       case ADV7511_REG_CEC_RX2_FRAME_DATA0...
> +               ADV7511_REG_CEC_RX2_FRAME_DATA0 + 14:
> +       case ADV7511_REG_CEC_RX2_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX3_FRAME_HDR:
> +       case ADV7511_REG_CEC_RX3_FRAME_DATA0...
> +               ADV7511_REG_CEC_RX3_FRAME_DATA0 + 14:
> +       case ADV7511_REG_CEC_RX3_FRAME_LEN:
> +       case ADV7511_REG_CEC_RX_STATUS:
>         case ADV7511_REG_CEC_RX_BUFFERS:
>         case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
>                 return true;

This is a bit of a nitpick, but the syntax of these "case X...Y"
statements was kind of hard to parse, do you mind putting them on one
line?

With or without the above suggestion fixed, r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535
  2022-03-19 15:10 ` Alvin Šipraga
@ 2022-08-29 16:02   ` Robert Foss
  -1 siblings, 0 replies; 14+ messages in thread
From: Robert Foss @ 2022-08-29 16:02 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, David Airlie, Daniel Vetter, Alvin Šipraga,
	dri-devel, linux-kernel

Hi Alvin,

Sorry about being slow to get to this series.

Can you rebase it on drm-misc-next and send out the next version?

On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> We have an ADV7535 which is nominally supported by this driver. These
> two patches fix up the driver to get CEC working too.
>
> The first adds the basic support by correcting some register offsets.
>
> The second addresses an issue we saw with CEC RX on the ADV7535. It
> hasn't been tested with the other chips (e.g. ADV7533), although it
> should be compatible. I'm sending it against drm-misc-next because the
> issue wasn't reported for other chips, and ADV7535 didn't have CEC
> support before. But feel free to take it into -fixes instead.
>
> Alvin Šipraga (2):
>   drm: bridge: adv7511: enable CEC support for ADV7535
>   drm: bridge: adv7511: use non-legacy mode for CEC RX
>
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  27 ++++-
>  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 116 +++++++++++++------
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  22 +++-
>  3 files changed, 119 insertions(+), 46 deletions(-)
>
> --
> 2.35.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535
@ 2022-08-29 16:02   ` Robert Foss
  0 siblings, 0 replies; 14+ messages in thread
From: Robert Foss @ 2022-08-29 16:02 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Jonas Karlman, David Airlie, dri-devel, Neil Armstrong,
	linux-kernel, Jernej Skrabec, Laurent Pinchart, Andrzej Hajda,
	Alvin Šipraga

Hi Alvin,

Sorry about being slow to get to this series.

Can you rebase it on drm-misc-next and send out the next version?

On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
>
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> We have an ADV7535 which is nominally supported by this driver. These
> two patches fix up the driver to get CEC working too.
>
> The first adds the basic support by correcting some register offsets.
>
> The second addresses an issue we saw with CEC RX on the ADV7535. It
> hasn't been tested with the other chips (e.g. ADV7533), although it
> should be compatible. I'm sending it against drm-misc-next because the
> issue wasn't reported for other chips, and ADV7535 didn't have CEC
> support before. But feel free to take it into -fixes instead.
>
> Alvin Šipraga (2):
>   drm: bridge: adv7511: enable CEC support for ADV7535
>   drm: bridge: adv7511: use non-legacy mode for CEC RX
>
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  27 ++++-
>  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 116 +++++++++++++------
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  22 +++-
>  3 files changed, 119 insertions(+), 46 deletions(-)
>
> --
> 2.35.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535
  2022-08-29 16:02   ` Robert Foss
@ 2022-08-30  7:54     ` Alvin Šipraga
  -1 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-08-30  7:54 UTC (permalink / raw)
  To: Robert Foss
  Cc: Alvin Šipraga, Andrzej Hajda, Neil Armstrong,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie,
	Daniel Vetter, dri-devel, linux-kernel

Hi Robert,

On Mon, Aug 29, 2022 at 06:02:17PM +0200, Robert Foss wrote:
> Hi Alvin,
> 
> Sorry about being slow to get to this series.

Nothing to be sorry about - in fact it was already applied! ;-)

ab0af093bf90 drm: bridge: adv7511: use non-legacy mode for CEC RX
0aae7623b495 drm: bridge: adv7511: enable CEC support for ADV7535

The one patch I am still waiting for feedback on is here:

https://lore.kernel.org/all/20220319152932.995904-1-alvin@pqrs.dk/

But it is an RFC patch and not of an urgent nature.

Thanks!

Kind regards,
Alvin

> 
> Can you rebase it on drm-misc-next and send out the next version?
> 
> On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
> >
> > From: Alvin Šipraga <alsi@bang-olufsen.dk>
> >
> > We have an ADV7535 which is nominally supported by this driver. These
> > two patches fix up the driver to get CEC working too.
> >
> > The first adds the basic support by correcting some register offsets.
> >
> > The second addresses an issue we saw with CEC RX on the ADV7535. It
> > hasn't been tested with the other chips (e.g. ADV7533), although it
> > should be compatible. I'm sending it against drm-misc-next because the
> > issue wasn't reported for other chips, and ADV7535 didn't have CEC
> > support before. But feel free to take it into -fixes instead.
> >
> > Alvin Šipraga (2):
> >   drm: bridge: adv7511: enable CEC support for ADV7535
> >   drm: bridge: adv7511: use non-legacy mode for CEC RX
> >
> >  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  27 ++++-
> >  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 116 +++++++++++++------
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  22 +++-
> >  3 files changed, 119 insertions(+), 46 deletions(-)
> >
> > --
> > 2.35.1
> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535
@ 2022-08-30  7:54     ` Alvin Šipraga
  0 siblings, 0 replies; 14+ messages in thread
From: Alvin Šipraga @ 2022-08-30  7:54 UTC (permalink / raw)
  To: Robert Foss
  Cc: Jonas Karlman, David Airlie, dri-devel, Neil Armstrong,
	linux-kernel, Jernej Skrabec, Alvin Šipraga,
	Laurent Pinchart, Andrzej Hajda

Hi Robert,

On Mon, Aug 29, 2022 at 06:02:17PM +0200, Robert Foss wrote:
> Hi Alvin,
> 
> Sorry about being slow to get to this series.

Nothing to be sorry about - in fact it was already applied! ;-)

ab0af093bf90 drm: bridge: adv7511: use non-legacy mode for CEC RX
0aae7623b495 drm: bridge: adv7511: enable CEC support for ADV7535

The one patch I am still waiting for feedback on is here:

https://lore.kernel.org/all/20220319152932.995904-1-alvin@pqrs.dk/

But it is an RFC patch and not of an urgent nature.

Thanks!

Kind regards,
Alvin

> 
> Can you rebase it on drm-misc-next and send out the next version?
> 
> On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@pqrs.dk> wrote:
> >
> > From: Alvin Šipraga <alsi@bang-olufsen.dk>
> >
> > We have an ADV7535 which is nominally supported by this driver. These
> > two patches fix up the driver to get CEC working too.
> >
> > The first adds the basic support by correcting some register offsets.
> >
> > The second addresses an issue we saw with CEC RX on the ADV7535. It
> > hasn't been tested with the other chips (e.g. ADV7533), although it
> > should be compatible. I'm sending it against drm-misc-next because the
> > issue wasn't reported for other chips, and ADV7535 didn't have CEC
> > support before. But feel free to take it into -fixes instead.
> >
> > Alvin Šipraga (2):
> >   drm: bridge: adv7511: enable CEC support for ADV7535
> >   drm: bridge: adv7511: use non-legacy mode for CEC RX
> >
> >  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  27 ++++-
> >  drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 116 +++++++++++++------
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  22 +++-
> >  3 files changed, 119 insertions(+), 46 deletions(-)
> >
> > --
> > 2.35.1
> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-08-30  7:54 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-19 15:10 [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535 Alvin Šipraga
2022-03-19 15:10 ` Alvin Šipraga
2022-03-19 15:10 ` [PATCH 1/2] drm: bridge: adv7511: enable " Alvin Šipraga
2022-03-19 15:10   ` Alvin Šipraga
2022-04-22 14:25   ` Robert Foss
2022-04-22 14:25     ` Robert Foss
2022-03-19 15:10 ` [PATCH 2/2] drm: bridge: adv7511: use non-legacy mode for CEC RX Alvin Šipraga
2022-03-19 15:10   ` Alvin Šipraga
2022-04-22 14:36   ` Robert Foss
2022-04-22 14:36     ` Robert Foss
2022-08-29 16:02 ` [PATCH 0/2] drm: bridge: adv7511: CEC support for ADV7535 Robert Foss
2022-08-29 16:02   ` Robert Foss
2022-08-30  7:54   ` Alvin Šipraga
2022-08-30  7:54     ` Alvin Šipraga

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.