| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAG3jFyu64Hq=8Jh0Gj=H+tojBmERcjCZ-tq6PoYb9yrfar7iFw@mail.gmail.com>
Date: Fri, 22 Apr 2022 16:36:42 +0200
From: Robert Foss <robert.foss@...aro.org>
To: Alvin Šipraga <alvin@...s.dk>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <narmstrong@...libre.com>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Alvin Šipraga <alsi@...g-olufsen.dk>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drm: bridge: adv7511: use non-legacy mode for CEC RX
On Sat, 19 Mar 2022 at 16:10, Alvin Šipraga <alvin@...s.dk> wrote:
>
> From: Alvin Šipraga <alsi@...g-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@...g-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@...aro.org>
Powered by blists - more mailing lists