[PATCH v5 0/3] Add HDMI audio on the rk3588 SoC

[PATCH v5 0/3] Add HDMI audio on the rk3588 SoC

[Detlev Casanova]

To support HDMI audio on the rk3588 based devices, the generic HDMI
Codec framework is used in the dw-hdmi-qp DRM bridge driver.

The implementation is mainly based on the downstream driver, ported to the
generic HDMI Codec framework [1] recently merged in the master branch.
The parameters computation has been kept as is and the data stored in the
dw_hdmi_qp struct as been cleaned up.

The table for the N values has been edited to reflect N recommended values
as well as CTS recommended values.

The downstream kernel also implements a machine driver for HDMI audio but
it is doing exactly what the simple-audio-card driver does, so use that
instead in the RK3588 SoC device tree.

Based on Linus' master branch.

[1]: https://lore.kernel.org/all/20241224-drm-bridge-hdmi-connector-v10-0-dc89577cd438@linaro.org/

Changes since v4:
 - Moved hdmi0_audio node the rk3588-base.dtsi
 - Enable hdmi0_audio in rk3588-rock-5b.dts

Changes since v3:
 - Renamed function to start with dw_hdmi_qp

Changes since v2:
 - Also clear the audio infoframe
 - Write AUDI_CONTENTS0 to its default value in case it gets overwritten.
 - Store tmds_char_rate in the dw_hdmi_qp struct in atomic_enable
 - Clear tmds_char_rate in atomic_disable and only write registers when
   tmds_char_rate is not 0.
 - Do not use connector_state duplicates

Changes since v1:
 - Remove useless audio_mutex (was used downstream for multiple drivers access
   to audio functions)
 - Let hdmi_codec build and setup audio infoframes
 - Only access audio registers when connector is connected
 - Rebased on master branch


Detlev Casanova (2):
  arm64: dts: rockchip: Add HDMI0 audio output for rk3588 SoC
  arm64: dts: rockchip: Enable HDMI0 audio output for Rock 5B

Sugar Zhang (1):
  drm/bridge: synopsys: Add audio support for dw-hdmi-qp

 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  17 +
 .../boot/dts/rockchip/rk3588-rock-5b.dts      |   8 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c  | 497 ++++++++++++++++++
 3 files changed, 522 insertions(+)

-- 
2.48.1

[PATCH v5 1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp

[Detlev Casanova]

From: Sugar Zhang <sugar.zhang@rock-chips.com>

Register the dw-hdmi-qp bridge driver as an HDMI audio codec.

The register values computation functions (for n) are based on the
downstream driver, as well as the register writing functions.

The driver uses the generic HDMI Codec framework in order to implement
the HDMI audio support.

Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 497 +++++++++++++++++++
 1 file changed, 497 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index b281cabfe992e..ea5d8599cb56a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -36,6 +36,88 @@
 
 #define SCRAMB_POLL_DELAY_MS	3000
 
+/*
+ * Unless otherwise noted, entries in this table are 100% optimization.
+ * Values can be obtained from dw_hdmi_qp_compute_n() but that function is
+ * slow so we pre-compute values we expect to see.
+ *
+ * The values for TMDS 25175, 25200, 27000, 54000, 74250 and 148500 kHz are
+ * the recommended N values specified in the Audio chapter of the HDMI
+ * specification.
+ */
+static const struct dw_hdmi_audio_tmds_n {
+	unsigned long tmds;
+	unsigned int n_32k;
+	unsigned int n_44k1;
+	unsigned int n_48k;
+} common_tmds_n_table[] = {
+	{ .tmds = 25175000,  .n_32k = 4576,  .n_44k1 = 7007,  .n_48k = 6864, },
+	{ .tmds = 25200000,  .n_32k = 4096,  .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 27000000,  .n_32k = 4096,  .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 28320000,  .n_32k = 4096,  .n_44k1 = 5586,  .n_48k = 6144, },
+	{ .tmds = 30240000,  .n_32k = 4096,  .n_44k1 = 5642,  .n_48k = 6144, },
+	{ .tmds = 31500000,  .n_32k = 4096,  .n_44k1 = 5600,  .n_48k = 6144, },
+	{ .tmds = 32000000,  .n_32k = 4096,  .n_44k1 = 5733,  .n_48k = 6144, },
+	{ .tmds = 33750000,  .n_32k = 4096,  .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 36000000,  .n_32k = 4096,  .n_44k1 = 5684,  .n_48k = 6144, },
+	{ .tmds = 40000000,  .n_32k = 4096,  .n_44k1 = 5733,  .n_48k = 6144, },
+	{ .tmds = 49500000,  .n_32k = 4096,  .n_44k1 = 5488,  .n_48k = 6144, },
+	{ .tmds = 50000000,  .n_32k = 4096,  .n_44k1 = 5292,  .n_48k = 6144, },
+	{ .tmds = 54000000,  .n_32k = 4096,  .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 65000000,  .n_32k = 4096,  .n_44k1 = 7056,  .n_48k = 6144, },
+	{ .tmds = 68250000,  .n_32k = 4096,  .n_44k1 = 5376,  .n_48k = 6144, },
+	{ .tmds = 71000000,  .n_32k = 4096,  .n_44k1 = 7056,  .n_48k = 6144, },
+	{ .tmds = 72000000,  .n_32k = 4096,  .n_44k1 = 5635,  .n_48k = 6144, },
+	{ .tmds = 73250000,  .n_32k = 11648, .n_44k1 = 14112, .n_48k = 6144, },
+	{ .tmds = 74250000,  .n_32k = 4096,  .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 75000000,  .n_32k = 4096,  .n_44k1 = 5880,  .n_48k = 6144, },
+	{ .tmds = 78750000,  .n_32k = 4096,  .n_44k1 = 5600,  .n_48k = 6144, },
+	{ .tmds = 78800000,  .n_32k = 4096,  .n_44k1 = 5292,  .n_48k = 6144, },
+	{ .tmds = 79500000,  .n_32k = 4096,  .n_44k1 = 4704,  .n_48k = 6144, },
+	{ .tmds = 83500000,  .n_32k = 4096,  .n_44k1 = 7056,  .n_48k = 6144, },
+	{ .tmds = 85500000,  .n_32k = 4096,  .n_44k1 = 5488,  .n_48k = 6144, },
+	{ .tmds = 88750000,  .n_32k = 4096,  .n_44k1 = 14112, .n_48k = 6144, },
+	{ .tmds = 97750000,  .n_32k = 4096,  .n_44k1 = 14112, .n_48k = 6144, },
+	{ .tmds = 101000000, .n_32k = 4096,  .n_44k1 = 7056,  .n_48k = 6144, },
+	{ .tmds = 106500000, .n_32k = 4096,  .n_44k1 = 4704,  .n_48k = 6144, },
+	{ .tmds = 108000000, .n_32k = 4096,  .n_44k1 = 5684,  .n_48k = 6144, },
+	{ .tmds = 115500000, .n_32k = 4096,  .n_44k1 = 5712,  .n_48k = 6144, },
+	{ .tmds = 119000000, .n_32k = 4096,  .n_44k1 = 5544,  .n_48k = 6144, },
+	{ .tmds = 135000000, .n_32k = 4096,  .n_44k1 = 5488,  .n_48k = 6144, },
+	{ .tmds = 146250000, .n_32k = 11648, .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 148500000, .n_32k = 4096,  .n_44k1 = 6272,  .n_48k = 6144, },
+	{ .tmds = 154000000, .n_32k = 4096,  .n_44k1 = 5544,  .n_48k = 6144, },
+	{ .tmds = 162000000, .n_32k = 4096,  .n_44k1 = 5684,  .n_48k = 6144, },
+
+	/* For 297 MHz+ HDMI spec have some other rule for setting N */
+	{ .tmds = 297000000, .n_32k = 3073,  .n_44k1 = 4704,  .n_48k = 5120, },
+	{ .tmds = 594000000, .n_32k = 3073,  .n_44k1 = 9408,  .n_48k = 10240,},
+
+	/* End of table */
+	{ .tmds = 0,         .n_32k = 0,     .n_44k1 = 0,     .n_48k = 0,    },
+};
+
+/*
+ * These are the CTS values as recommended in the Audio chapter of the HDMI
+ * specification.
+ */
+static const struct dw_hdmi_audio_tmds_cts {
+	unsigned long tmds;
+	unsigned int cts_32k;
+	unsigned int cts_44k1;
+	unsigned int cts_48k;
+} common_tmds_cts_table[] = {
+	{ .tmds = 25175000,  .cts_32k = 28125,  .cts_44k1 = 31250,  .cts_48k = 28125,  },
+	{ .tmds = 25200000,  .cts_32k = 25200,  .cts_44k1 = 28000,  .cts_48k = 25200,  },
+	{ .tmds = 27000000,  .cts_32k = 27000,  .cts_44k1 = 30000,  .cts_48k = 27000,  },
+	{ .tmds = 54000000,  .cts_32k = 54000,  .cts_44k1 = 60000,  .cts_48k = 54000,  },
+	{ .tmds = 74250000,  .cts_32k = 74250,  .cts_44k1 = 82500,  .cts_48k = 74250,  },
+	{ .tmds = 148500000, .cts_32k = 148500, .cts_44k1 = 165000, .cts_48k = 148500, },
+
+	/* End of table */
+	{ .tmds = 0,         .cts_32k = 0,      .cts_44k1 = 0,      .cts_48k = 0,      },
+};
+
 struct dw_hdmi_qp_i2c {
 	struct i2c_adapter	adap;
 
@@ -60,6 +142,8 @@ struct dw_hdmi_qp {
 	} phy;
 
 	struct regmap *regm;
+
+	unsigned long tmds_char_rate;
 };
 
 static void dw_hdmi_qp_write(struct dw_hdmi_qp *hdmi, unsigned int val,
@@ -83,6 +167,354 @@ static void dw_hdmi_qp_mod(struct dw_hdmi_qp *hdmi, unsigned int data,
 	regmap_update_bits(hdmi->regm, reg, mask, data);
 }
 
+static struct dw_hdmi_qp *dw_hdmi_qp_from_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct dw_hdmi_qp, bridge);
+}
+
+static void dw_hdmi_qp_set_cts_n(struct dw_hdmi_qp *hdmi, unsigned int cts,
+			   unsigned int n)
+{
+	/* Set N */
+	dw_hdmi_qp_mod(hdmi, n, AUDPKT_ACR_N_VALUE, AUDPKT_ACR_CONTROL0);
+
+	/* Set CTS */
+	if (cts)
+		dw_hdmi_qp_mod(hdmi, AUDPKT_ACR_CTS_OVR_EN, AUDPKT_ACR_CTS_OVR_EN_MSK,
+			  AUDPKT_ACR_CONTROL1);
+	else
+		dw_hdmi_qp_mod(hdmi, 0, AUDPKT_ACR_CTS_OVR_EN_MSK,
+			  AUDPKT_ACR_CONTROL1);
+
+	dw_hdmi_qp_mod(hdmi, AUDPKT_ACR_CTS_OVR_VAL(cts), AUDPKT_ACR_CTS_OVR_VAL_MSK,
+		  AUDPKT_ACR_CONTROL1);
+}
+
+static int dw_hdmi_qp_match_tmds_n_table(struct dw_hdmi_qp *hdmi,
+				   unsigned long pixel_clk,
+				   unsigned long freq)
+{
+	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
+	int i;
+
+	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
+		if (pixel_clk == common_tmds_n_table[i].tmds) {
+			tmds_n = &common_tmds_n_table[i];
+			break;
+		}
+	}
+
+	if (tmds_n == NULL)
+		return -ENOENT;
+
+	switch (freq) {
+	case 32000:
+		return tmds_n->n_32k;
+	case 44100:
+	case 88200:
+	case 176400:
+		return (freq / 44100) * tmds_n->n_44k1;
+	case 48000:
+	case 96000:
+	case 192000:
+		return (freq / 48000) * tmds_n->n_48k;
+	default:
+		return -ENOENT;
+	}
+}
+
+static u64 dw_hdmi_qp_audio_math_diff(unsigned int freq, unsigned int n,
+				unsigned int pixel_clk)
+{
+	u64 final, diff;
+	u64 cts;
+
+	final = (u64)pixel_clk * n;
+
+	cts = final;
+	do_div(cts, 128 * freq);
+
+	diff = final - (u64)cts * (128 * freq);
+
+	return diff;
+}
+
+static unsigned int dw_hdmi_qp_compute_n(struct dw_hdmi_qp *hdmi,
+				   unsigned long pixel_clk,
+				   unsigned long freq)
+{
+	unsigned int min_n = DIV_ROUND_UP((128 * freq), 1500);
+	unsigned int max_n = (128 * freq) / 300;
+	unsigned int ideal_n = (128 * freq) / 1000;
+	unsigned int best_n_distance = ideal_n;
+	unsigned int best_n = 0;
+	u64 best_diff = U64_MAX;
+	int n;
+
+	/* If the ideal N could satisfy the audio math, then just take it */
+	if (dw_hdmi_qp_audio_math_diff(freq, ideal_n, pixel_clk) == 0)
+		return ideal_n;
+
+	for (n = min_n; n <= max_n; n++) {
+		u64 diff = dw_hdmi_qp_audio_math_diff(freq, n, pixel_clk);
+
+		if (diff < best_diff || (diff == best_diff &&
+		    abs(n - ideal_n) < best_n_distance)) {
+			best_n = n;
+			best_diff = diff;
+			best_n_distance = abs(best_n - ideal_n);
+		}
+
+		/*
+		 * The best N already satisfy the audio math, and also be
+		 * the closest value to ideal N, so just cut the loop.
+		 */
+		if ((best_diff == 0) && (abs(n - ideal_n) > best_n_distance))
+			break;
+	}
+
+	return best_n;
+}
+
+static unsigned int dw_hdmi_qp_find_n(struct dw_hdmi_qp *hdmi, unsigned long pixel_clk,
+				unsigned long sample_rate)
+{
+	int n = dw_hdmi_qp_match_tmds_n_table(hdmi, pixel_clk, sample_rate);
+
+	if (n > 0)
+		return n;
+
+	dev_warn(hdmi->dev, "Rate %lu missing; compute N dynamically\n",
+		 pixel_clk);
+
+	return dw_hdmi_qp_compute_n(hdmi, pixel_clk, sample_rate);
+}
+
+static unsigned int dw_hdmi_qp_find_cts(struct dw_hdmi_qp *hdmi, unsigned long pixel_clk,
+				  unsigned long sample_rate)
+{
+	const struct dw_hdmi_audio_tmds_cts *tmds_cts = NULL;
+	int i;
+
+	for (i = 0; common_tmds_cts_table[i].tmds != 0; i++) {
+		if (pixel_clk == common_tmds_cts_table[i].tmds) {
+			tmds_cts = &common_tmds_cts_table[i];
+			break;
+		}
+	}
+
+	if (tmds_cts == NULL)
+		return 0;
+
+	switch (sample_rate) {
+	case 32000:
+		return tmds_cts->cts_32k;
+	case 44100:
+	case 88200:
+	case 176400:
+		return tmds_cts->cts_44k1;
+	case 48000:
+	case 96000:
+	case 192000:
+		return tmds_cts->cts_48k;
+	default:
+		return -ENOENT;
+	}
+}
+
+static void dw_hdmi_qp_set_audio_interface(struct dw_hdmi_qp *hdmi,
+					   struct hdmi_codec_daifmt *fmt,
+					   struct hdmi_codec_params *hparms)
+{
+	u32 conf0 = 0;
+
+	/* Reset the audio data path of the AVP */
+	dw_hdmi_qp_write(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWINIT_P, GLOBAL_SWRESET_REQUEST);
+
+	/* Disable AUDS, ACR, AUDI */
+	dw_hdmi_qp_mod(hdmi, 0,
+		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
+		  PKTSCHED_PKT_EN);
+
+	/* Clear the audio FIFO */
+	dw_hdmi_qp_write(hdmi, AUDIO_FIFO_CLR_P, AUDIO_INTERFACE_CONTROL0);
+
+	/* Select I2S interface as the audio source */
+	dw_hdmi_qp_mod(hdmi, AUD_IF_I2S, AUD_IF_SEL_MSK, AUDIO_INTERFACE_CONFIG0);
+
+	/* Enable the active i2s lanes */
+	switch (hparms->channels) {
+	case 7 ... 8:
+		conf0 |= I2S_LINES_EN(3);
+		fallthrough;
+	case 5 ... 6:
+		conf0 |= I2S_LINES_EN(2);
+		fallthrough;
+	case 3 ... 4:
+		conf0 |= I2S_LINES_EN(1);
+		fallthrough;
+	default:
+		conf0 |= I2S_LINES_EN(0);
+		break;
+	}
+
+	dw_hdmi_qp_mod(hdmi, conf0, I2S_LINES_EN_MSK, AUDIO_INTERFACE_CONFIG0);
+
+	/*
+	 * Enable bpcuv generated internally for L-PCM, or received
+	 * from stream for NLPCM/HBR.
+	 */
+	switch (fmt->bit_fmt) {
+	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
+		conf0 = (hparms->channels == 8) ? AUD_HBR : AUD_ASP;
+		conf0 |= I2S_BPCUV_RCV_EN;
+		break;
+	default:
+		conf0 = AUD_ASP | I2S_BPCUV_RCV_DIS;
+		break;
+	}
+
+	dw_hdmi_qp_mod(hdmi, conf0, I2S_BPCUV_RCV_MSK | AUD_FORMAT_MSK,
+		  AUDIO_INTERFACE_CONFIG0);
+
+	/* Enable audio FIFO auto clear when overflow */
+	dw_hdmi_qp_mod(hdmi, AUD_FIFO_INIT_ON_OVF_EN, AUD_FIFO_INIT_ON_OVF_MSK,
+		  AUDIO_INTERFACE_CONFIG0);
+}
+
+/*
+ * When transmitting IEC60958 linear PCM audio, these registers allow to
+ * configure the channel status information of all the channel status
+ * bits in the IEC60958 frame. For the moment this configuration is only
+ * used when the I2S audio interface, General Purpose Audio (GPA),
+ * or AHB audio DMA (AHBAUDDMA) interface is active
+ * (for S/PDIF interface this information comes from the stream).
+ */
+static void dw_hdmi_qp_set_channel_status(struct dw_hdmi_qp *hdmi,
+					  u8 *channel_status, bool ref2stream)
+{
+	/*
+	 * AUDPKT_CHSTATUS_OVR0: { RSV, RSV, CS1, CS0 }
+	 * AUDPKT_CHSTATUS_OVR1: { CS6, CS5, CS4, CS3 }
+	 *
+	 *      |  7  |  6  |  5  |  4  |  3  |  2  |  1  |  0  |
+	 * CS0: |   Mode    |        d        |  c  |  b  |  a  |
+	 * CS1: |               Category Code                   |
+	 * CS2: |    Channel Number     |     Source Number     |
+	 * CS3: |    Clock Accuracy     |     Sample Freq       |
+	 * CS4: |    Ori Sample Freq    |     Word Length       |
+	 * CS5: |                                   |   CGMS-A  |
+	 * CS6~CS23: Reserved
+	 *
+	 * a: use of channel status block
+	 * b: linear PCM identification: 0 for lpcm, 1 for nlpcm
+	 * c: copyright information
+	 * d: additional format information
+	 */
+
+	if (ref2stream)
+		channel_status[0] |= IEC958_AES0_NONAUDIO;
+
+	if ((dw_hdmi_qp_read(hdmi, AUDIO_INTERFACE_CONFIG0) & GENMASK(25, 24)) == AUD_HBR) {
+		/* fixup cs for HBR */
+		channel_status[3] = (channel_status[3] & 0xf0) | IEC958_AES3_CON_FS_768000;
+		channel_status[4] = (channel_status[4] & 0x0f) | IEC958_AES4_CON_ORIGFS_NOTID;
+	}
+
+	dw_hdmi_qp_write(hdmi, channel_status[0] | (channel_status[1] << 8),
+		    AUDPKT_CHSTATUS_OVR0);
+
+	regmap_bulk_write(hdmi->regm, AUDPKT_CHSTATUS_OVR1, &channel_status[3], 1);
+
+	if (ref2stream)
+		dw_hdmi_qp_mod(hdmi, 0,
+			  AUDPKT_PBIT_FORCE_EN_MASK | AUDPKT_CHSTATUS_OVR_EN_MASK,
+			  AUDPKT_CONTROL0);
+	else
+		dw_hdmi_qp_mod(hdmi, AUDPKT_PBIT_FORCE_EN | AUDPKT_CHSTATUS_OVR_EN,
+			  AUDPKT_PBIT_FORCE_EN_MASK | AUDPKT_CHSTATUS_OVR_EN_MASK,
+			  AUDPKT_CONTROL0);
+}
+
+static void dw_hdmi_qp_set_sample_rate(struct dw_hdmi_qp *hdmi, unsigned long long tmds_char_rate,
+				       unsigned int sample_rate)
+{
+	unsigned int n, cts;
+
+	n = dw_hdmi_qp_find_n(hdmi, tmds_char_rate, sample_rate);
+	cts = dw_hdmi_qp_find_cts(hdmi, tmds_char_rate, sample_rate);
+
+	dw_hdmi_qp_set_cts_n(hdmi, cts, n);
+}
+
+static int dw_hdmi_qp_audio_enable(struct drm_connector *connector,
+				   struct drm_bridge *bridge)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+
+	if (hdmi->tmds_char_rate)
+		dw_hdmi_qp_mod(hdmi, 0, AVP_DATAPATH_PACKET_AUDIO_SWDISABLE, GLOBAL_SWDISABLE);
+
+	return 0;
+}
+
+static int dw_hdmi_qp_audio_prepare(struct drm_connector *connector,
+				    struct drm_bridge *bridge,
+				    struct hdmi_codec_daifmt *fmt,
+				    struct hdmi_codec_params *hparms)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+	bool ref2stream = false;
+
+	if (!hdmi->tmds_char_rate)
+		return -ENODEV;
+
+	if (fmt->bit_clk_provider | fmt->frame_clk_provider) {
+		dev_err(hdmi->dev, "unsupported clock settings\n");
+		return -EINVAL;
+	}
+
+	if (fmt->bit_fmt == SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE)
+		ref2stream = true;
+
+	dw_hdmi_qp_set_audio_interface(hdmi, fmt, hparms);
+	dw_hdmi_qp_set_sample_rate(hdmi, hdmi->tmds_char_rate, hparms->sample_rate);
+	dw_hdmi_qp_set_channel_status(hdmi, hparms->iec.status, ref2stream);
+	drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector, &hparms->cea);
+
+	return 0;
+}
+
+static void dw_hdmi_qp_audio_disable_regs(struct dw_hdmi_qp *hdmi)
+{
+	/*
+	 * Keep ACR, AUDI, AUDS packet always on to make SINK device
+	 * active for better compatibility and user experience.
+	 *
+	 * This also fix POP sound on some SINK devices which wakeup
+	 * from suspend to active.
+	 */
+	dw_hdmi_qp_mod(hdmi, I2S_BPCUV_RCV_DIS, I2S_BPCUV_RCV_MSK,
+		       AUDIO_INTERFACE_CONFIG0);
+	dw_hdmi_qp_mod(hdmi, AUDPKT_PBIT_FORCE_EN | AUDPKT_CHSTATUS_OVR_EN,
+		       AUDPKT_PBIT_FORCE_EN_MASK | AUDPKT_CHSTATUS_OVR_EN_MASK,
+		AUDPKT_CONTROL0);
+
+	dw_hdmi_qp_mod(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWDISABLE,
+		       AVP_DATAPATH_PACKET_AUDIO_SWDISABLE, GLOBAL_SWDISABLE);
+}
+
+static void dw_hdmi_qp_audio_disable(struct drm_connector *connector,
+				     struct drm_bridge *bridge)
+{
+	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
+
+	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
+
+	if (hdmi->tmds_char_rate)
+		dw_hdmi_qp_audio_disable_regs(hdmi);
+}
+
 static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
 			       unsigned char *buf, unsigned int length)
 {
@@ -361,6 +793,51 @@ static int dw_hdmi_qp_config_drm_infoframe(struct dw_hdmi_qp *hdmi,
 	return 0;
 }
 
+/*
+ * Static values documented in the TRM
+ * Different values are only used for debug purposes
+ */
+#define DW_HDMI_QP_AUDIO_INFOFRAME_HB1	0x1
+#define DW_HDMI_QP_AUDIO_INFOFRAME_HB2	0xa
+
+static int dw_hdmi_qp_config_audio_infoframe(struct dw_hdmi_qp *hdmi,
+					     const u8 *buffer, size_t len)
+{
+	/*
+	 * AUDI_CONTENTS0: { RSV, HB2, HB1, RSV }
+	 * AUDI_CONTENTS1: { PB3, PB2, PB1, PB0 }
+	 * AUDI_CONTENTS2: { PB7, PB6, PB5, PB4 }
+	 *
+	 * PB0: CheckSum
+	 * PB1: | CT3    | CT2  | CT1  | CT0  | F13  | CC2 | CC1 | CC0 |
+	 * PB2: | F27    | F26  | F25  | SF2  | SF1  | SF0 | SS1 | SS0 |
+	 * PB3: | F37    | F36  | F35  | F34  | F33  | F32 | F31 | F30 |
+	 * PB4: | CA7    | CA6  | CA5  | CA4  | CA3  | CA2 | CA1 | CA0 |
+	 * PB5: | DM_INH | LSV3 | LSV2 | LSV1 | LSV0 | F52 | F51 | F50 |
+	 * PB6~PB10: Reserved
+	 *
+	 * AUDI_CONTENTS0 default value defined by HDMI specification,
+	 * and shall only be changed for debug purposes.
+	 */
+	u32 header_bytes = (DW_HDMI_QP_AUDIO_INFOFRAME_HB1 << 8) |
+			  (DW_HDMI_QP_AUDIO_INFOFRAME_HB2 << 16);
+
+	regmap_bulk_write(hdmi->regm, PKT_AUDI_CONTENTS0, &header_bytes, 1);
+	regmap_bulk_write(hdmi->regm, PKT_AUDI_CONTENTS1, &buffer[3], 1);
+	regmap_bulk_write(hdmi->regm, PKT_AUDI_CONTENTS2, &buffer[4], 1);
+
+	/* Enable ACR, AUDI, AMD */
+	dw_hdmi_qp_mod(hdmi,
+		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDI_TX_EN | PKTSCHED_AMD_TX_EN,
+		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDI_TX_EN | PKTSCHED_AMD_TX_EN,
+		  PKTSCHED_PKT_EN);
+
+	/* Enable AUDS */
+	dw_hdmi_qp_mod(hdmi, PKTSCHED_AUDS_TX_EN, PKTSCHED_AUDS_TX_EN, PKTSCHED_PKT_EN);
+
+	return 0;
+}
+
 static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
 					    struct drm_bridge_state *old_state)
 {
@@ -382,6 +859,7 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
 		dev_dbg(hdmi->dev, "%s mode=HDMI rate=%llu\n",
 			__func__, conn_state->hdmi.tmds_char_rate);
 		op_mode = 0;
+		hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
 	} else {
 		dev_dbg(hdmi->dev, "%s mode=DVI\n", __func__);
 		op_mode = OPMODE_DVI;
@@ -400,6 +878,8 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = bridge->driver_private;
 
+	hdmi->tmds_char_rate = 0;
+
 	hdmi->phy.ops->disable(hdmi, hdmi->phy.data);
 }
 
@@ -455,6 +935,13 @@ static int dw_hdmi_qp_bridge_clear_infoframe(struct drm_bridge *bridge,
 		dw_hdmi_qp_mod(hdmi, 0, PKTSCHED_DRMI_TX_EN, PKTSCHED_PKT_EN);
 		break;
 
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		dw_hdmi_qp_mod(hdmi, 0,
+			       PKTSCHED_ACR_TX_EN |
+			       PKTSCHED_AUDS_TX_EN |
+			       PKTSCHED_AUDI_TX_EN,
+			       PKTSCHED_PKT_EN);
+		break;
 	default:
 		dev_dbg(hdmi->dev, "Unsupported infoframe type %x\n", type);
 	}
@@ -477,6 +964,9 @@ static int dw_hdmi_qp_bridge_write_infoframe(struct drm_bridge *bridge,
 	case HDMI_INFOFRAME_TYPE_DRM:
 		return dw_hdmi_qp_config_drm_infoframe(hdmi, buffer, len);
 
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		return dw_hdmi_qp_config_audio_infoframe(hdmi, buffer, len);
+
 	default:
 		dev_dbg(hdmi->dev, "Unsupported infoframe type %x\n", type);
 		return 0;
@@ -494,6 +984,9 @@ static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
 	.hdmi_tmds_char_rate_valid = dw_hdmi_qp_bridge_tmds_char_rate_valid,
 	.hdmi_clear_infoframe = dw_hdmi_qp_bridge_clear_infoframe,
 	.hdmi_write_infoframe = dw_hdmi_qp_bridge_write_infoframe,
+	.hdmi_audio_startup = dw_hdmi_qp_audio_enable,
+	.hdmi_audio_shutdown = dw_hdmi_qp_audio_disable,
+	.hdmi_audio_prepare = dw_hdmi_qp_audio_prepare,
 };
 
 static irqreturn_t dw_hdmi_qp_main_hardirq(int irq, void *dev_id)
@@ -603,6 +1096,10 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 	if (IS_ERR(hdmi->bridge.ddc))
 		return ERR_CAST(hdmi->bridge.ddc);
 
+	hdmi->bridge.hdmi_audio_max_i2s_playback_channels = 8;
+	hdmi->bridge.hdmi_audio_dev = dev;
+	hdmi->bridge.hdmi_audio_dai_port = 1;
+
 	ret = devm_drm_bridge_add(dev, &hdmi->bridge);
 	if (ret)
 		return ERR_PTR(ret);
-- 
2.48.1

[PATCH v5 2/3] arm64: dts: rockchip: Add HDMI0 audio output for rk3588 SoC

[Detlev Casanova]

Use the simple-audio-card driver with the hdmi0 QP node as CODEC and
the i2s5 device as CPU.

The simple-audio-card,mclk-fs value is set to 128 as it is done in
the downstream driver.

The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be
used as an audio codec node.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index 8cfa30837ce72..767bdfc06b7ec 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -1369,6 +1369,22 @@ i2s9_8ch: i2s@fddfc000 {
 		status = "disabled";
 	};
 
+	hdmi0_sound: hdmi0-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,mclk-fs = <128>;
+		simple-audio-card,name = "hdmi0";
+		status = "disabled";
+
+		simple-audio-card,codec {
+			sound-dai = <&hdmi0>;
+		};
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s5_8ch>;
+		};
+	};
+
 	hdmi0: hdmi@fde80000 {
 		compatible = "rockchip,rk3588-dw-hdmi-qp";
 		reg = <0x0 0xfde80000 0x0 0x20000>;
@@ -1394,6 +1410,7 @@ hdmi0: hdmi@fde80000 {
 		reset-names = "ref", "hdp";
 		rockchip,grf = <&sys_grf>;
 		rockchip,vo-grf = <&vo1_grf>;
+		#sound-dai-cells = <0>;
 		status = "disabled";
 
 		ports {
-- 
2.48.1

[PATCH v5 3/3] arm64: dts: rockchip: Enable HDMI0 audio output for Rock 5B

[Detlev Casanova]

HDMI audio is available on the Rock 5B HDMI TX port.
Enable it.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index d597112f1d5b8..88ff5d9db2817 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -204,6 +204,10 @@ &gpu {
 	status = "okay";
 };
 
+&hdmi0_sound {
+	status = "okay";
+};
+
 &hdmi0 {
 	status = "okay";
 };
@@ -318,6 +322,10 @@ i2s0_8ch_p0_0: endpoint {
 	};
 };
 
+&i2s5_8ch {
+	status = "okay";
+};
+
 &package_thermal {
 	polling-delay = <1000>;
 
-- 
2.48.1

Re: [PATCH v5 1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp

[Quentin Schulz]

Hi Detlev,

Just some drive-by comment inline.

On 2/3/25 6:16 PM, Detlev Casanova wrote:
> From: Sugar Zhang <sugar.zhang@rock-chips.com>
> 
> Register the dw-hdmi-qp bridge driver as an HDMI audio codec.
> 
> The register values computation functions (for n) are based on the
> downstream driver, as well as the register writing functions.
> 
> The driver uses the generic HDMI Codec framework in order to implement
> the HDMI audio support.
> 
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Tested-by: Quentin Schulz <quentin.schulz@cherry.de>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 497 +++++++++++++++++++
>   1 file changed, 497 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index b281cabfe992e..ea5d8599cb56a 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
[...]
> +static int dw_hdmi_qp_match_tmds_n_table(struct dw_hdmi_qp *hdmi,
> +				   unsigned long pixel_clk,
> +				   unsigned long freq)
> +{
> +	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
> +	int i;
> +
> +	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
> +		if (pixel_clk == common_tmds_n_table[i].tmds) {
> +			tmds_n = &common_tmds_n_table[i];
> +			break;
> +		}
> +	}
> +
> +	if (tmds_n == NULL)
> +		return -ENOENT;
> +
> +	switch (freq) {
> +	case 32000:
> +		return tmds_n->n_32k;
> +	case 44100:
> +	case 88200:
> +	case 176400:
> +		return (freq / 44100) * tmds_n->n_44k1;
> +	case 48000:
> +	case 96000:
> +	case 192000:
> +		return (freq / 48000) * tmds_n->n_48k;
> +	default:
> +		return -ENOENT;
> +	}
> +}
> +
> +static u64 dw_hdmi_qp_audio_math_diff(unsigned int freq, unsigned int n,
> +				unsigned int pixel_clk)
> +{
> +	u64 final, diff;
> +	u64 cts;
> +
> +	final = (u64)pixel_clk * n;
> +
> +	cts = final;
> +	do_div(cts, 128 * freq);
> +
> +	diff = final - (u64)cts * (128 * freq);
> +
> +	return diff;

I believe

final = (u64)pixel_clk * n;
do_div(cts, 128 * freq);
diff = final - (u64)cts * (128 * freq);

is just an elaborate way of doing

((u64)pixel_clk * n) % (128 * freq)

? If that's the case, then I believe do_div(a, b) actually already 
returns the remainder of a by b.

If that's the case, isn't that function simply:

return do_div(mul_u32_u32(pixel_clk, n), mul_u32_u32(freq, 128))

?

We could probably replace mul_u32_u32(freq, 128) with freq * 128 
considering that dw_hdmi_qp_match_tmds_n_table expects freq to be < 
192000, so we won't get an overflow with that.

mul_u32_u32(pixel_clk, n) does the u32*u32->u64 without a temporary C 
variable.

[...]

> +static void dw_hdmi_qp_set_audio_interface(struct dw_hdmi_qp *hdmi,
> +					   struct hdmi_codec_daifmt *fmt,
> +					   struct hdmi_codec_params *hparms)
> +{
> +	u32 conf0 = 0;
> +
> +	/* Reset the audio data path of the AVP */
> +	dw_hdmi_qp_write(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWINIT_P, GLOBAL_SWRESET_REQUEST);
> +
> +	/* Disable AUDS, ACR, AUDI */
> +	dw_hdmi_qp_mod(hdmi, 0,
> +		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
> +		  PKTSCHED_PKT_EN);
> +
> +	/* Clear the audio FIFO */
> +	dw_hdmi_qp_write(hdmi, AUDIO_FIFO_CLR_P, AUDIO_INTERFACE_CONTROL0);
> +
> +	/* Select I2S interface as the audio source */
> +	dw_hdmi_qp_mod(hdmi, AUD_IF_I2S, AUD_IF_SEL_MSK, AUDIO_INTERFACE_CONFIG0);
> +
> +	/* Enable the active i2s lanes */
> +	switch (hparms->channels) {
> +	case 7 ... 8:
> +		conf0 |= I2S_LINES_EN(3);
> +		fallthrough;
> +	case 5 ... 6:
> +		conf0 |= I2S_LINES_EN(2);
> +		fallthrough;
> +	case 3 ... 4:
> +		conf0 |= I2S_LINES_EN(1);
> +		fallthrough;
> +	default:
> +		conf0 |= I2S_LINES_EN(0);
> +		break;
> +	}
> +
> +	dw_hdmi_qp_mod(hdmi, conf0, I2S_LINES_EN_MSK, AUDIO_INTERFACE_CONFIG0);
> +
> +	/*
> +	 * Enable bpcuv generated internally for L-PCM, or received
> +	 * from stream for NLPCM/HBR.
> +	 */
> +	switch (fmt->bit_fmt) {
> +	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
> +		conf0 = (hparms->channels == 8) ? AUD_HBR : AUD_ASP;
> +		conf0 |= I2S_BPCUV_RCV_EN;
> +		break;
> +	default:
> +		conf0 = AUD_ASP | I2S_BPCUV_RCV_DIS;
> +		break;
> +	}
> +
> +	dw_hdmi_qp_mod(hdmi, conf0, I2S_BPCUV_RCV_MSK | AUD_FORMAT_MSK,
> +		  AUDIO_INTERFACE_CONFIG0);
> +
> +	/* Enable audio FIFO auto clear when overflow */
> +	dw_hdmi_qp_mod(hdmi, AUD_FIFO_INIT_ON_OVF_EN, AUD_FIFO_INIT_ON_OVF_MSK,
> +		  AUDIO_INTERFACE_CONFIG0);

This is all very I2S-centric while the HDMI controllers on RK3588 do 
have the ability (according to the TRM) to use S/PDIF instead of I2S. I 
assume the driver should be able to know which format to use based on
simple-audio-card,format property? Is that correct? Then current support 
which doesn't even check for I2S would be fine and not conflict with a 
later commit which would add support for S/PDIF? (Essentially asking if 
we need another DT property for the HDMI controller node or elsewhere to 
specify which mode to run in instead of expecting it to always be I2S).

Cheers,
Quentin

Re: [PATCH v5 2/3] arm64: dts: rockchip: Add HDMI0 audio output for rk3588 SoC

[Quentin Schulz]

Hi Detlev,

On 2/3/25 6:16 PM, Detlev Casanova wrote:
> Use the simple-audio-card driver with the hdmi0 QP node as CODEC and
> the i2s5 device as CPU.
> 
> The simple-audio-card,mclk-fs value is set to 128 as it is done in
> the downstream driver.
> 
> The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be
> used as an audio codec node.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index 8cfa30837ce72..767bdfc06b7ec 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> @@ -1369,6 +1369,22 @@ i2s9_8ch: i2s@fddfc000 {
>   		status = "disabled";
>   	};
>   
> +	hdmi0_sound: hdmi0-sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,mclk-fs = <128>;
> +		simple-audio-card,name = "hdmi0";
> +		status = "disabled";
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&hdmi0>;
> +		};
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&i2s5_8ch>;
> +		};
> +	};
> +

This is an address/bus-less device, so I believe it needs to be put 
among other address/bus-less devices, which for Rockchip SoCs is at the 
top of the DT? c.f. 
https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html

**I** would put it between the firmware and the pmu-a55 nodes.

Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3588 Tiger Haikou

Thanks!
Quentin

Re: [PATCH v5 3/3] arm64: dts: rockchip: Enable HDMI0 audio output for Rock 5B

[Quentin Schulz]

Hi Detlev,

On 2/3/25 6:16 PM, Detlev Casanova wrote:
> HDMI audio is available on the Rock 5B HDMI TX port.
> Enable it.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Will try to remember to send patches for RK3588 Tiger Haikou and RK3588 
Jaguar once those patches get merged :)

Thanks!
Quentin

> ---
>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index d597112f1d5b8..88ff5d9db2817 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -204,6 +204,10 @@ &gpu {
>   	status = "okay";
>   };
>   
> +&hdmi0_sound {
> +	status = "okay";
> +};
> +
>   &hdmi0 {
>   	status = "okay";
>   };
> @@ -318,6 +322,10 @@ i2s0_8ch_p0_0: endpoint {
>   	};
>   };
>   
> +&i2s5_8ch {
> +	status = "okay";
> +};
> +
>   &package_thermal {
>   	polling-delay = <1000>;
>   

Re: [PATCH v5 2/3] arm64: dts: rockchip: Add HDMI0 audio output for rk3588 SoC

[Detlev Casanova]

On Tuesday, 4 February 2025 05:14:37 EST Quentin Schulz wrote:
> Hi Detlev,
> 
> On 2/3/25 6:16 PM, Detlev Casanova wrote:
> > Use the simple-audio-card driver with the hdmi0 QP node as CODEC and
> > the i2s5 device as CPU.
> > 
> > The simple-audio-card,mclk-fs value is set to 128 as it is done in
> > the downstream driver.
> > 
> > The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be
> > used as an audio codec node.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi index
> > 8cfa30837ce72..767bdfc06b7ec 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> > @@ -1369,6 +1369,22 @@ i2s9_8ch: i2s@fddfc000 {
> > 
> >   		status = "disabled";
> >   	
> >   	};
> > 
> > +	hdmi0_sound: hdmi0-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,mclk-fs = <128>;
> > +		simple-audio-card,name = "hdmi0";
> > +		status = "disabled";
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&hdmi0>;
> > +		};
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s5_8ch>;
> > +		};
> > +	};
> > +
> 
> This is an address/bus-less device, so I believe it needs to be put
> among other address/bus-less devices, which for Rockchip SoCs is at the
> top of the DT? c.f.
> https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.
> html
> 
> **I** would put it between the firmware and the pmu-a55 nodes.

I wasn't sure about this. It makes sense to move it with address-less devices, 
but the rk3399 device tree has it above the hdmi node, so I wanted to stay 
consistent with that.

> Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3588 Tiger Haikou
> 
> Thanks!
> Quentin

Re: [PATCH v5 1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp

[Detlev Casanova]

Hi Quentin,

On Tuesday, 4 February 2025 04:59:12 EST Quentin Schulz wrote:
> Hi Detlev,
> 
> Just some drive-by comment inline.
> 
> On 2/3/25 6:16 PM, Detlev Casanova wrote:
> > From: Sugar Zhang <sugar.zhang@rock-chips.com>
> > 
> > Register the dw-hdmi-qp bridge driver as an HDMI audio codec.
> > 
> > The register values computation functions (for n) are based on the
> > downstream driver, as well as the register writing functions.
> > 
> > The driver uses the generic HDMI Codec framework in order to implement
> > the HDMI audio support.
> > 
> > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Tested-by: Quentin Schulz <quentin.schulz@cherry.de>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 497 +++++++++++++++++++
> >   1 file changed, 497 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c index
> > b281cabfe992e..ea5d8599cb56a 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> 
> [...]
> 
> > +static int dw_hdmi_qp_match_tmds_n_table(struct dw_hdmi_qp *hdmi,
> > +				   unsigned long pixel_clk,
> > +				   unsigned long freq)
> > +{
> > +	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
> > +	int i;
> > +
> > +	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
> > +		if (pixel_clk == common_tmds_n_table[i].tmds) {
> > +			tmds_n = &common_tmds_n_table[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (tmds_n == NULL)
> > +		return -ENOENT;
> > +
> > +	switch (freq) {
> > +	case 32000:
> > +		return tmds_n->n_32k;
> > +	case 44100:
> > +	case 88200:
> > +	case 176400:
> > +		return (freq / 44100) * tmds_n->n_44k1;
> > +	case 48000:
> > +	case 96000:
> > +	case 192000:
> > +		return (freq / 48000) * tmds_n->n_48k;
> > +	default:
> > +		return -ENOENT;
> > +	}
> > +}
> > +
> > +static u64 dw_hdmi_qp_audio_math_diff(unsigned int freq, unsigned int n,
> > +				unsigned int pixel_clk)
> > +{
> > +	u64 final, diff;
> > +	u64 cts;
> > +
> > +	final = (u64)pixel_clk * n;
> > +
> > +	cts = final;
> > +	do_div(cts, 128 * freq);
> > +
> > +	diff = final - (u64)cts * (128 * freq);
> > +
> > +	return diff;
> 
> I believe
> 
> final = (u64)pixel_clk * n;
> do_div(cts, 128 * freq);
> diff = final - (u64)cts * (128 * freq);
> 
> is just an elaborate way of doing
> 
> ((u64)pixel_clk * n) % (128 * freq)
> 
> ? If that's the case, then I believe do_div(a, b) actually already
> returns the remainder of a by b.
> 
> If that's the case, isn't that function simply:
> 
> return do_div(mul_u32_u32(pixel_clk, n), mul_u32_u32(freq, 128))
> 
> ?
> 
> We could probably replace mul_u32_u32(freq, 128) with freq * 128
> considering that dw_hdmi_qp_match_tmds_n_table expects freq to be <
> 192000, so we won't get an overflow with that.
> 
> mul_u32_u32(pixel_clk, n) does the u32*u32->u64 without a temporary C
> variable.

I tested this and it is indeed doing the same. I'll change it and also change 
the function return type to u32 as that is what do_div() returns.
That makes sense as the reminder of a/b can't be wider than the size of b.

We just need one variable as the first argument of do_div() is used to store 
the integer division result.

> [...]
> 
> > +static void dw_hdmi_qp_set_audio_interface(struct dw_hdmi_qp *hdmi,
> > +					   struct hdmi_codec_daifmt 
*fmt,
> > +					   struct hdmi_codec_params 
*hparms)
> > +{
> > +	u32 conf0 = 0;
> > +
> > +	/* Reset the audio data path of the AVP */
> > +	dw_hdmi_qp_write(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWINIT_P,
> > GLOBAL_SWRESET_REQUEST); +
> > +	/* Disable AUDS, ACR, AUDI */
> > +	dw_hdmi_qp_mod(hdmi, 0,
> > +		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDS_TX_EN | 
PKTSCHED_AUDI_TX_EN,
> > +		  PKTSCHED_PKT_EN);
> > +
> > +	/* Clear the audio FIFO */
> > +	dw_hdmi_qp_write(hdmi, AUDIO_FIFO_CLR_P, AUDIO_INTERFACE_CONTROL0);
> > +
> > +	/* Select I2S interface as the audio source */
> > +	dw_hdmi_qp_mod(hdmi, AUD_IF_I2S, AUD_IF_SEL_MSK,
> > AUDIO_INTERFACE_CONFIG0); +
> > +	/* Enable the active i2s lanes */
> > +	switch (hparms->channels) {
> > +	case 7 ... 8:
> > +		conf0 |= I2S_LINES_EN(3);
> > +		fallthrough;
> > +	case 5 ... 6:
> > +		conf0 |= I2S_LINES_EN(2);
> > +		fallthrough;
> > +	case 3 ... 4:
> > +		conf0 |= I2S_LINES_EN(1);
> > +		fallthrough;
> > +	default:
> > +		conf0 |= I2S_LINES_EN(0);
> > +		break;
> > +	}
> > +
> > +	dw_hdmi_qp_mod(hdmi, conf0, I2S_LINES_EN_MSK, 
AUDIO_INTERFACE_CONFIG0);
> > +
> > +	/*
> > +	 * Enable bpcuv generated internally for L-PCM, or received
> > +	 * from stream for NLPCM/HBR.
> > +	 */
> > +	switch (fmt->bit_fmt) {
> > +	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
> > +		conf0 = (hparms->channels == 8) ? AUD_HBR : AUD_ASP;
> > +		conf0 |= I2S_BPCUV_RCV_EN;
> > +		break;
> > +	default:
> > +		conf0 = AUD_ASP | I2S_BPCUV_RCV_DIS;
> > +		break;
> > +	}
> > +
> > +	dw_hdmi_qp_mod(hdmi, conf0, I2S_BPCUV_RCV_MSK | AUD_FORMAT_MSK,
> > +		  AUDIO_INTERFACE_CONFIG0);
> > +
> > +	/* Enable audio FIFO auto clear when overflow */
> > +	dw_hdmi_qp_mod(hdmi, AUD_FIFO_INIT_ON_OVF_EN, 
AUD_FIFO_INIT_ON_OVF_MSK,
> > +		  AUDIO_INTERFACE_CONFIG0);
> 
> This is all very I2S-centric while the HDMI controllers on RK3588 do
> have the ability (according to the TRM) to use S/PDIF instead of I2S. I
> assume the driver should be able to know which format to use based on
> simple-audio-card,format property? Is that correct? Then current support
> which doesn't even check for I2S would be fine and not conflict with a
> later commit which would add support for S/PDIF? (Essentially asking if
> we need another DT property for the HDMI controller node or elsewhere to
> specify which mode to run in instead of expecting it to always be I2S).

The hdmi_codec_daifmt::fmt field already has this information, based on the 
simple-audio-card,format = "i2s"; field in the device tree.

I could add a condition in dw_hdmi_qp_audio_prepare() to fail with -EINVAL if 
the devicetree specifies anything else than "i2s" for now.

I'm not willing to implement support for the SPDIF path for now, mainly 
because there's no need for that yet (I2S works well) and the downstream 
kernel doesn't implemented it, meaning it hasn't been tested a lot anyway.

Regards,
Detlev.

Re: [PATCH v5 1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp

[Quentin Schulz]

Hi Detlev,

On 2/6/25 9:40 PM, Detlev Casanova wrote:
> Hi Quentin,
> 
> On Tuesday, 4 February 2025 04:59:12 EST Quentin Schulz wrote:
>> Hi Detlev,
>>
>> Just some drive-by comment inline.
>>
>> On 2/3/25 6:16 PM, Detlev Casanova wrote:
[...]
>>> +static void dw_hdmi_qp_set_audio_interface(struct dw_hdmi_qp *hdmi,
>>> +					   struct hdmi_codec_daifmt
> *fmt,
>>> +					   struct hdmi_codec_params
> *hparms)
>>> +{
>>> +	u32 conf0 = 0;
>>> +
>>> +	/* Reset the audio data path of the AVP */
>>> +	dw_hdmi_qp_write(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWINIT_P,
>>> GLOBAL_SWRESET_REQUEST); +
>>> +	/* Disable AUDS, ACR, AUDI */
>>> +	dw_hdmi_qp_mod(hdmi, 0,
>>> +		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDS_TX_EN |
> PKTSCHED_AUDI_TX_EN,
>>> +		  PKTSCHED_PKT_EN);
>>> +
>>> +	/* Clear the audio FIFO */
>>> +	dw_hdmi_qp_write(hdmi, AUDIO_FIFO_CLR_P, AUDIO_INTERFACE_CONTROL0);
>>> +
>>> +	/* Select I2S interface as the audio source */
>>> +	dw_hdmi_qp_mod(hdmi, AUD_IF_I2S, AUD_IF_SEL_MSK,
>>> AUDIO_INTERFACE_CONFIG0); +
>>> +	/* Enable the active i2s lanes */
>>> +	switch (hparms->channels) {
>>> +	case 7 ... 8:
>>> +		conf0 |= I2S_LINES_EN(3);
>>> +		fallthrough;
>>> +	case 5 ... 6:
>>> +		conf0 |= I2S_LINES_EN(2);
>>> +		fallthrough;
>>> +	case 3 ... 4:
>>> +		conf0 |= I2S_LINES_EN(1);
>>> +		fallthrough;
>>> +	default:
>>> +		conf0 |= I2S_LINES_EN(0);
>>> +		break;
>>> +	}
>>> +
>>> +	dw_hdmi_qp_mod(hdmi, conf0, I2S_LINES_EN_MSK,
> AUDIO_INTERFACE_CONFIG0);
>>> +
>>> +	/*
>>> +	 * Enable bpcuv generated internally for L-PCM, or received
>>> +	 * from stream for NLPCM/HBR.
>>> +	 */
>>> +	switch (fmt->bit_fmt) {
>>> +	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
>>> +		conf0 = (hparms->channels == 8) ? AUD_HBR : AUD_ASP;
>>> +		conf0 |= I2S_BPCUV_RCV_EN;
>>> +		break;
>>> +	default:
>>> +		conf0 = AUD_ASP | I2S_BPCUV_RCV_DIS;
>>> +		break;
>>> +	}
>>> +
>>> +	dw_hdmi_qp_mod(hdmi, conf0, I2S_BPCUV_RCV_MSK | AUD_FORMAT_MSK,
>>> +		  AUDIO_INTERFACE_CONFIG0);
>>> +
>>> +	/* Enable audio FIFO auto clear when overflow */
>>> +	dw_hdmi_qp_mod(hdmi, AUD_FIFO_INIT_ON_OVF_EN,
> AUD_FIFO_INIT_ON_OVF_MSK,
>>> +		  AUDIO_INTERFACE_CONFIG0);
>>
>> This is all very I2S-centric while the HDMI controllers on RK3588 do
>> have the ability (according to the TRM) to use S/PDIF instead of I2S. I
>> assume the driver should be able to know which format to use based on
>> simple-audio-card,format property? Is that correct? Then current support
>> which doesn't even check for I2S would be fine and not conflict with a
>> later commit which would add support for S/PDIF? (Essentially asking if
>> we need another DT property for the HDMI controller node or elsewhere to
>> specify which mode to run in instead of expecting it to always be I2S).
> 
> The hdmi_codec_daifmt::fmt field already has this information, based on the
> simple-audio-card,format = "i2s"; field in the device tree.
> 
> I could add a condition in dw_hdmi_qp_audio_prepare() to fail with -EINVAL if
> the devicetree specifies anything else than "i2s" for now.
> 

All I was asking is whether we can (easily) check this property is set 
to i2s the day we want to implement support for S/PDIF :) If that's the 
case, then I don't think we necessarily need to check it right now, but 
a -EINVAL would be fine too, up to you.

> I'm not willing to implement support for the SPDIF path for now, mainly
> because there's no need for that yet (I2S works well) and the downstream
> kernel doesn't implemented it, meaning it hasn't been tested a lot anyway.
> 

That's fine, just wanted to know if a "backward-compatible" 
implementation for S/PDIF would be possible in the future would we want 
to support it :)

Cheers,
Quentin

Re: [PATCH v5 2/3] arm64: dts: rockchip: Add HDMI0 audio output for rk3588 SoC

[Heiko Stübner]

Hi Detlev,

Am Donnerstag, 6. Februar 2025, 18:17:51 MEZ schrieb Detlev Casanova:
> On Tuesday, 4 February 2025 05:14:37 EST Quentin Schulz wrote:
> > This is an address/bus-less device, so I believe it needs to be put
> > among other address/bus-less devices, which for Rockchip SoCs is at the
> > top of the DT? c.f.
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.
> > html
> > 
> > **I** would put it between the firmware and the pmu-a55 nodes.
> 
> I wasn't sure about this. It makes sense to move it with address-less devices, 
> but the rk3399 device tree has it above the hdmi node, so I wanted to stay 
> consistent with that.

the general "rule" is to
- sort by address (the part after the @)
- if that's not present, alphabetically by node-name (not phandle)
  and have address-less things more to the top
- &phandle {} references are sorted alphabetically

And yes in the past there were more creative variants ... like keeping
hdmi-stuff together sounds really great [also to me in the past ;-) ],
but then you end up with a huge list of exceptions.  [0]

So just following the generic sorting thing causes way less confusion,
hence we go that way for new stuff.


Heiko