media: i2c: adv748x: add support for HDMI audio

This change implements audio-related V4L2 ioctls for the HDMI subdevice.

The master audio clock is configured for 256fs, as supported by the only device available at the moment. For the same reason, the TDM slot is formatted using left justification of its bits.

Signed-off-by Alexander Riesen <[email protected]>

---
 drivers/media/i2c/adv748x/adv748x-core.c |   6 +
 drivers/media/i2c/adv748x/adv748x-hdmi.c | 182 +++++++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  42 ++++++
 3 files changed, 230 insertions(+)

drivers/media/i2c/adv748x/adv748x-core.c +6 -0

index bc49aa93793c..b6067ffb1e0d 100644
@@ -150,6 +150,12 @@ static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
 	return *error;
 }
 
+int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg, u8 mask,
+			u8 value)
+{
+	return regmap_update_bits(state->regmap[page], reg, mask, value);
+}
+
 /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
  * size to one or more registers.
  *

drivers/media/i2c/adv748x/adv748x-hdmi.c +182 -0

index c557f8fdf11a..9bc9237c9116 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Renesas Electronics Corp.
  */
 
+#include <linux/version.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 

@@ -603,11 +604,186 @@ static const struct v4l2_subdev_pad_ops adv748x_pad_ops_hdmi = {
 	.enum_dv_timings = adv748x_hdmi_enum_dv_timings,
 };
 
+static int adv748x_hdmi_audio_mute(struct adv748x_hdmi *hdmi, int enable)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+
+	return hdmi_update(state, ADV748X_HDMI_MUTE_CTRL,
+			   ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO,
+			   enable ? 0xff : 0);
+}
+
+
+#define HDMI_AOUT_NONE 0
+#define HDMI_AOUT_I2S 1
+#define HDMI_AOUT_I2S_TDM 2
+
+static int adv748x_hdmi_enumaudout(struct adv748x_hdmi *hdmi,
+				   struct v4l2_audioout *a)
+{
+	switch (a->index) {
+	case HDMI_AOUT_NONE:
+		strlcpy(a->name, "None", sizeof(a->name));
+		break;
+	case HDMI_AOUT_I2S:
+		strlcpy(a->name, "I2S/stereo", sizeof(a->name));
+		break;
+	case HDMI_AOUT_I2S_TDM:
+		strlcpy(a->name, "I2S-TDM/multichannel", sizeof(a->name));
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int adv748x_hdmi_g_audout(struct adv748x_hdmi *hdmi,
+				 struct v4l2_audioout *a)
+{
+	a->index = hdmi->audio_out;
+	return adv748x_hdmi_enumaudout(hdmi, a);
+}
+
+static int set_audio_pads_state(struct adv748x_state *state, int on)
+{
+	return io_update(state, ADV748X_IO_PAD_CONTROLS,
+			 ADV748X_IO_PAD_CONTROLS_TRI_AUD |
+			 ADV748X_IO_PAD_CONTROLS_PDN_AUD,
+			 on ? 0 : 0xff);
+}
+
+static int set_dpll_mclk_fs(struct adv748x_state *state, int fs)
+{
+	if (fs % 128 || fs > 768)
+		return -EINVAL;
+	return dpll_update(state, ADV748X_DPLL_MCLK_FS,
+			   ADV748X_DPLL_MCLK_FS_N_MASK, (fs / 128) - 1);
+}
+
+static int set_i2s_format(struct adv748x_state *state, uint outmode,
+			  uint bitwidth)
+{
+	return hdmi_update(state, ADV748X_HDMI_I2S,
+			   ADV748X_HDMI_I2SBITWIDTH_MASK |
+			   ADV748X_HDMI_I2SOUTMODE_MASK,
+			   (outmode << ADV748X_HDMI_I2SOUTMODE_SHIFT) |
+			   bitwidth);
+}
+
+static int set_i2s_tdm_mode(struct adv748x_state *state, int is_tdm)
+{
+	int ret;
+
+	ret = hdmi_update(state, ADV748X_HDMI_AUDIO_MUTE_SPEED,
+			  ADV748X_MAN_AUDIO_DL_BYPASS |
+			  ADV748X_AUDIO_DELAY_LINE_BYPASS,
+			  is_tdm ? 0xff : 0);
+	if (ret < 0)
+		goto fail;
+	ret = hdmi_update(state, ADV748X_HDMI_REG_6D,
+			  ADV748X_I2S_TDM_MODE_ENABLE,
+			  is_tdm ? 0xff : 0);
+	if (ret < 0)
+		goto fail;
+	ret = set_i2s_format(state, ADV748X_I2SOUTMODE_LEFT_J, 24);
+fail:
+	return ret;
+}
+
+static int set_audio_out(struct adv748x_state *state, int aout)
+{
+	int ret;
+
+	switch (aout) {
+	case HDMI_AOUT_NONE:
+		ret = set_audio_pads_state(state, 0);
+		break;
+	case HDMI_AOUT_I2S:
+		ret = set_dpll_mclk_fs(state, 256);
+		if (ret < 0)
+			goto fail;
+		ret = set_i2s_tdm_mode(state, 1);
+		if (ret < 0)
+			goto fail;
+		ret = set_audio_pads_state(state, 1);
+		if (ret < 0)
+			goto fail;
+		break;
+	case HDMI_AOUT_I2S_TDM:
+		ret = set_dpll_mclk_fs(state, 256);
+		if (ret < 0)
+			goto fail;
+		ret = set_i2s_tdm_mode(state, 1);
+		if (ret < 0)
+			goto fail;
+		ret = set_audio_pads_state(state, 1);
+		if (ret < 0)
+			goto fail;
+		break;
+	default:
+		ret = -EINVAL;
+		goto fail;
+	}
+	return 0;
+fail:
+	return ret;
+}
+
+static int adv748x_hdmi_s_audout(struct adv748x_hdmi *hdmi,
+				 const struct v4l2_audioout *a)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+	int ret = set_audio_out(state, a->index);
+
+	if (ret == 0)
+		hdmi->audio_out = a->index;
+	return ret;
+}
+
+static long adv748x_hdmi_querycap(struct adv748x_hdmi *hdmi,
+				  struct v4l2_capability *cap)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+
+	cap->version = LINUX_VERSION_CODE;
+	strlcpy(cap->driver, state->dev->driver->name, sizeof(cap->driver));
+	strlcpy(cap->card, "hdmi", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "i2c:%d-%04x",
+		 i2c_adapter_id(state->client->adapter),
+		 state->client->addr);
+	cap->device_caps = V4L2_CAP_AUDIO | V4L2_CAP_VIDEO_CAPTURE;
+	cap->capabilities = V4L2_CAP_DEVICE_CAPS;
+	return 0;
+}
+
+static long adv748x_hdmi_ioctl(struct v4l2_subdev *sd,
+			       unsigned int cmd, void *arg)
+{
+	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
+
+	switch (cmd) {
+	case VIDIOC_ENUMAUDOUT:
+		return adv748x_hdmi_enumaudout(hdmi, arg);
+	case VIDIOC_S_AUDOUT:
+		return adv748x_hdmi_s_audout(hdmi, arg);
+	case VIDIOC_G_AUDOUT:
+		return adv748x_hdmi_g_audout(hdmi, arg);
+	case VIDIOC_QUERYCAP:
+		return adv748x_hdmi_querycap(hdmi, arg);
+	}
+	return -ENOTTY;
+}
+
+static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = {
+	.ioctl = adv748x_hdmi_ioctl,
+};
+
 /* -----------------------------------------------------------------------------
  * v4l2_subdev_ops
  */
 
 static const struct v4l2_subdev_ops adv748x_ops_hdmi = {
+	.core = &adv748x_core_ops_hdmi,
 	.video = &adv748x_video_ops_hdmi,
 	.pad = &adv748x_pad_ops_hdmi,
 };

@@ -633,6 +809,8 @@ static int adv748x_hdmi_s_ctrl(struct v4l2_ctrl *ctrl)
 	int ret;
 	u8 pattern;
 
+	if (ctrl->id == V4L2_CID_AUDIO_MUTE)
+		return adv748x_hdmi_audio_mute(hdmi, ctrl->val);
 	/* Enable video adjustment first */
 	ret = cp_clrset(state, ADV748X_CP_VID_ADJ,
 			ADV748X_CP_VID_ADJ_ENABLE,

@@ -697,6 +875,8 @@ static int adv748x_hdmi_init_controls(struct adv748x_hdmi *hdmi)
 	v4l2_ctrl_new_std(&hdmi->ctrl_hdl, &adv748x_hdmi_ctrl_ops,
 			  V4L2_CID_HUE, ADV748X_CP_HUE_MIN,
 			  ADV748X_CP_HUE_MAX, 1, ADV748X_CP_HUE_DEF);
+	v4l2_ctrl_new_std(&hdmi->ctrl_hdl, &adv748x_hdmi_ctrl_ops,
+			  V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
 
 	/*
 	 * Todo: V4L2_CID_DV_RX_POWER_PRESENT should also be supported when

@@ -755,6 +935,8 @@ int adv748x_hdmi_init(struct adv748x_hdmi *hdmi)
 
 void adv748x_hdmi_cleanup(struct adv748x_hdmi *hdmi)
 {
+	adv748x_hdmi_audio_mute(hdmi, 1);
+	set_audio_out(adv748x_hdmi_to_state(hdmi), HDMI_AOUT_NONE);
 	v4l2_device_unregister_subdev(&hdmi->sd);
 	media_entity_cleanup(&hdmi->sd.entity);
 	v4l2_ctrl_handler_free(&hdmi->ctrl_hdl);

drivers/media/i2c/adv748x/adv748x.h +42 -0

index db6346a06351..fdda6982e437 100644
@@ -128,6 +128,7 @@ struct adv748x_hdmi {
 		u32 present;
 		unsigned int blocks;
 	} edid;
+	int audio_out;
 };
 
 #define adv748x_ctrl_to_hdmi(ctrl) \

@@ -224,6 +225,11 @@ struct adv748x_state {
 
 #define ADV748X_IO_VID_STD		0x05
 
+#define ADV748X_IO_PAD_CONTROLS		0x0e
+#define ADV748X_IO_PAD_CONTROLS_TRI_AUD	BIT(5)
+#define ADV748X_IO_PAD_CONTROLS_PDN_AUD	BIT(1)
+#define ADV748X_IO_PAD_CONTROLS1	0x1d
+
 #define ADV748X_IO_10			0x10	/* io_reg_10 */
 #define ADV748X_IO_10_CSI4_EN		BIT(7)
 #define ADV748X_IO_10_CSI1_EN		BIT(6)

@@ -246,7 +252,21 @@ struct adv748x_state {
 #define ADV748X_IO_REG_FF		0xff
 #define ADV748X_IO_REG_FF_MAIN_RESET	0xff
 
+/* DPLL Map */
+#define ADV748X_DPLL_MCLK_FS		0xb5
+#define ADV748X_DPLL_MCLK_FS_N_MASK	GENMASK(2, 0)
+
 /* HDMI RX Map */
+#define ADV748X_HDMI_I2S		0x03	/* I2S mode and width */
+#define ADV748X_HDMI_I2SBITWIDTH_MASK	GENMASK(4, 0)
+#define ADV748X_HDMI_I2SOUTMODE_SHIFT	5
+#define ADV748X_HDMI_I2SOUTMODE_MASK	\
+	GENMASK(6, ADV748X_HDMI_I2SOUTMODE_SHIFT)
+#define ADV748X_I2SOUTMODE_I2S 0
+#define ADV748X_I2SOUTMODE_RIGHT_J 1
+#define ADV748X_I2SOUTMODE_LEFT_J 2
+#define ADV748X_I2SOUTMODE_SPDIF 3
+
 #define ADV748X_HDMI_LW1		0x07	/* line width_1 */
 #define ADV748X_HDMI_LW1_VERT_FILTER	BIT(7)
 #define ADV748X_HDMI_LW1_DE_REGEN	BIT(5)

@@ -258,6 +278,16 @@ struct adv748x_state {
 #define ADV748X_HDMI_F1H1		0x0b	/* field1 height_1 */
 #define ADV748X_HDMI_F1H1_INTERLACED	BIT(5)
 
+#define ADV748X_HDMI_MUTE_CTRL		0x1a
+#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4)
+#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASK	GENMASK(3, 1)
+#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE	BIT(0)
+
+#define ADV748X_HDMI_AUDIO_MUTE_SPEED	0x0f
+#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK	GENMASK(4, 0)
+#define ADV748X_MAN_AUDIO_DL_BYPASS BIT(7)
+#define ADV748X_AUDIO_DELAY_LINE_BYPASS BIT(6)
+
 #define ADV748X_HDMI_HFRONT_PORCH	0x20	/* hsync_front_porch_1 */
 #define ADV748X_HDMI_HFRONT_PORCH_MASK	0x1fff
 

@@ -279,6 +309,9 @@ struct adv748x_state {
 #define ADV748X_HDMI_TMDS_1		0x51	/* hdmi_reg_51 */
 #define ADV748X_HDMI_TMDS_2		0x52	/* hdmi_reg_52 */
 
+#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
+#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)
+
 /* HDMI RX Repeater Map */
 #define ADV748X_REPEATER_EDID_SZ	0x70	/* primary_edid_size */
 #define ADV748X_REPEATER_EDID_SZ_SHIFT	4

@@ -393,14 +426,23 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);
 int adv748x_write_block(struct adv748x_state *state, int client_page,
 			unsigned int init_reg, const void *val,
 			size_t val_len);
+int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg,
+			u8 mask, u8 value);
 
 #define io_read(s, r) adv748x_read(s, ADV748X_PAGE_IO, r)
 #define io_write(s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v)
 #define io_clrset(s, r, m, v) io_write(s, r, (io_read(s, r) & ~m) | v)
+#define io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
 
 #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
 #define hdmi_read16(s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, r+1)) & m)
 #define hdmi_write(s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v)
+#define hdmi_update(s, r, m, v) \
+	adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v)
+
+#define dpll_read(s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r)
+#define dpll_update(s, r, m, v) \
+	adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)
 
 #define repeater_read(s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)
 #define repeater_write(s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)

Quoted 1 line

This change implements audio-related V4L2 ioctls for the HDMI subdevice.

This is really where things go wrong. These V4L2 audio ioctls are meant for old PCI TV tuner devices where the audio was implemented as audio jack outputs that are typically looped back to audio inputs on a (PCI) soundcard. And when these ioctls were designed ALSA didn't even exist.

Generally an hdmi driver will configure the i2s audio automatically, which is typically connected to the SoC and controlled by the ALSA driver of the SoC, but there may well be missing features (audio never got a lot of attention in hdmi receivers). So what I would like to know is: what features are missing?

Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls, which would be the right approach for this.

So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be added here:

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html

Hi Hans,

Hans Verkuil, Fri, Mar 13, 2020 09:16:11 +0100:

Quoted 7 lines · Alex Riesen

Quoted 1 line

This change implements audio-related V4L2 ioctls for the HDMI subdevice.

This is really where things go wrong. These V4L2 audio ioctls are meant for old PCI TV tuner devices where the audio was implemented as audio jack outputs that are typically looped back to audio inputs on a (PCI) soundcard. And when these ioctls were designed ALSA didn't even exist.

I see. That was before my time :)

Quoted 4 lines

Generally an hdmi driver will configure the i2s audio automatically, which is typically connected to the SoC and controlled by the ALSA driver of the SoC, but there may well be missing features (audio never got a lot of attention in hdmi receivers). So what I would like to know is: what features are missing?

Well, the audio is missing. The current adv748x driver does not export the audio features of the device at all. There is no code to enable the I2S audio output and it is disabled (all clock and the data lines) by default.

But, by now it seems to be clear that implementation of ALSA SoC DAI interfaces is the way to support the audio.

And I am already slowly working on it.

Quoted 3 lines

Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls, which would be the right approach for this.

Quoted 4 lines

So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be added here:

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html

This seems to be an explicitly "digital video" control class. And it has no control option for mute. Or did you mean a similarly structured new class for "digital audio"?

This feels like an overkill for this particular driver...

Regards,
Alex

Quoted 6 lines

Hans Verkuil, Fri, Mar 13, 2020 09:16:11 +0100:

Quoted 4 lines

Generally an hdmi driver will configure the i2s audio automatically, which is typically connected to the SoC and controlled by the ALSA driver of the SoC, but there may well be missing features (audio never got a lot of attention in hdmi receivers). So what I would like to know is: what features are missing?

Quoted 3 lines

Well, the audio is missing. The current adv748x driver does not export the audio features of the device at all. There is no code to enable the I2S audio output and it is disabled (all clock and the data lines) by default.

Sorry, I was vague in my question. Obviously that needs to be added, but besides adding the low-level i2s support I was wondering if there are additional things that need to be exposed to userspace in order for audio to fully work.

Quoted 8 lines

Quoted 7 lines

Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls, which would be the right approach for this.

So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be added here:

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html

Quoted 3 lines

This seems to be an explicitly "digital video" control class. And it has no control option for mute. Or did you mean a similarly structured new class for "digital audio"?

There are no DV_ audio controls at all today. So any new audio controls would be added to the DV class. But if there is nothing that needs to be exposed, then nothing needs to be added :-)

As the driver has some support for the audio interface of the device, the bindings file should mention it.

Signed-off-by Alexander Riesen <[email protected]>

---
 .../devicetree/bindings/media/i2c/adv748x.txt       | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Not sure if all variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on Salvator-X ES1, so the the ADV7482 endpoint and connection definitions are placed in the board file.

I do assume though that all Salvator variants have the CLK_C clock line hard-wired to the ADV7482 HDMI decoder, and remove it from the list of clocks provided by the R-Car sound system.

The I2C wiring is also likely to persist across the variants (similar to ak4613, connected to the same interface), so that is in the common file.

Signed-off-by Alexander Riesen <[email protected]>

arch/arm64/boot/dts/renesas/salvator-common.dtsi +1 -0

+
 &avb {
        pinctrl-0 = <&avb_pins>;
        pinctrl-names = "default";

@@ -471,12 +475,14 @@
 
                #address-cells = <1>;

arch/arm64/boot/dts/renesas/salvator-common.dtsi +8 -0

                port@7 {
                        reg = <7>;
 

@@ -512,6 +518,14 @@
                                remote-endpoint = <&csi20_in>;
                        };
                };
+
+               port@c {
+                       reg = <12>;
+
+                       adv7482_i2s: endpoint {
+                               /* remote-endpoint defined in the board file */
+                       };
+               };
        };
 
        csa_vdd: adc@7c {

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -2

                function = "ssi";
        };
 

@@ -735,8 +750,8 @@
        pinctrl-0 = <&sound_pins &sound_clk_pins>;
        pinctrl-names = "default";
 
-       /* Single DAI */
-       #sound-dai-cells = <0>;
+       /* multi DAI */
+       #sound-dai-cells = <1>;
 
        /* audio_clkout0/1/2/3 */
        #clock-cells = <1>;

arch/arm64/boot/dts/renesas/salvator-common.dtsi +11 -1

                 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
+       clock-names = "ssi-all",
+                     "ssi.9", "ssi.8", "ssi.7", "ssi.6",
+                     "ssi.5", "ssi.4", "ssi.3", "ssi.2",
+                     "ssi.1", "ssi.0",
+                     "src.9", "src.8", "src.7", "src.6",
+                     "src.5", "src.4", "src.3", "src.2",
+                     "src.1", "src.0",
+                     "mix.1", "mix.0",
+                     "ctu.1", "ctu.0",
+                     "dvc.0", "dvc.1",
+                     "clk_a", "clk_b", "clk_i";
 
        ports {
                #address-cells = <1>;
--

2.24.1.508.g91d2dafee0

Hi Alex,

Thanks for your patch!

Quoted 4 lines · Alex Riesen

Not sure if all variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on Salvator-X ES1, so the the ADV7482 endpoint and connection definitions are placed in the board file.

Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.

Quoted 3 lines

I do assume though that all Salvator variants have the CLK_C clock line hard-wired to the ADV7482 HDMI decoder, and remove it from the list of clocks provided by the R-Car sound system.

Yes, both Salvator-X and Salvator-XS have it wired that way.

But please see below.

Quoted 5 lines

The I2C wiring is also likely to persist across the variants (similar to ak4613, connected to the same interface), so that is in the common file.

Signed-off-by Alexander Riesen <[email protected]>

Below are my comments w.r.t. the board-specific wiring. I'll defer to the multimedia people for commenting on the audio parts.

BTW, what is the status of the other patches in this series?

Quoted 6 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +0 -0

@@ -322,6 +322,10 @@
        clock-frequency = <22579200>;
 };
 

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 327

Quoted 3 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +3 -0

+&audio_clk_c {
+       clock-frequency = <12288000>;
+};

Does the ADV7482 always generate a 12.288 MHz clock signal? Or is this programmable?

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 485

Quoted 5 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -0

 video-receiver@70 {
 	compatible = "adi,adv7482";
...
+   clocks = <&rcar_sound 3>, <&audio_clk_c>;
+   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";

The above declares the Audio CLK C to be a clock input of the ADV7482, while it is an output.

Furthermore, the DT bindings do not document that clocks can be specified.

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 704

Quoted 7 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -1

@@ -686,7 +700,8 @@
        };
 
        sound_pins: sound {
-               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+                        "ssi4_data";

Missing "ss4_ctrl", for the SCK4 and WS4 pins.

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 763

Quoted 5 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +0 -1

@@ -760,8 +775,18 @@
                 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
                 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
                 <&audio_clk_a>, <&cs2000>,
-                <&audio_clk_c>,

Why remove it? This is the list of clock inputs, not outputs.

Geert Uytterhoeven, Mon, Mar 02, 2020 13:28:13 +0100:

Quoted 13 lines

Quoted 4 lines · Alex Riesen

Not sure if all variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on Salvator-X ES1, so the the ADV7482 endpoint and connection definitions are placed in the board file.

Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.

Quoted 3 lines

I do assume though that all Salvator variants have the CLK_C clock line hard-wired to the ADV7482 HDMI decoder, and remove it from the list of clocks provided by the R-Car sound system.

Quoted 1 line

Yes, both Salvator-X and Salvator-XS have it wired that way.

Ok, seems like I can move that part into the common file as well. Integrations of ADV7482 and R-Car which use salvator-common.dts can still redefine the endpoint settings in their board files, right?

Quoted 6 lines

Quoted 6 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +0 -0

@@ -322,6 +322,10 @@
        clock-frequency = <22579200>;
 };
 

Quoted 6 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 327

Quoted 3 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +3 -0

+&audio_clk_c {
+       clock-frequency = <12288000>;
+};

Does the ADV7482 always generate a 12.288 MHz clock signal? Or is this programmable?

Oops. It looks like it is and the value is derived from the sampling rate (48kHz) and the master clock multiplier. Both hard-coded in the board file.

Quoted 8 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 485

Quoted 5 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -0

 video-receiver@70 {
    compatible = "adi,adv7482";
...
+   clocks = <&rcar_sound 3>, <&audio_clk_c>;
+   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";

The above declares the Audio CLK C to be a clock input of the ADV7482, while it is an output.

I would gladly give it right direction if I *really* understood what I was doing...

Quoted 1 line

Furthermore, the DT bindings do not document that clocks can be specified.

Should the DT bindings document that the clock cannot be specified than?

Quoted 9 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 704

Quoted 7 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -1

@@ -686,7 +700,8 @@
        };
 
        sound_pins: sound {
-               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+                        "ssi4_data";

Missing "ss4_ctrl", for the SCK4 and WS4 pins.

I'll add them.
As the device seems to function even without thoes, does this mean the pins in the group are used "on demand" by whatever needs them?

Quoted 7 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 763

Quoted 5 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +0 -1

@@ -760,8 +775,18 @@
                 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
                 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
                 <&audio_clk_a>, <&cs2000>,
-                <&audio_clk_c>,

Why remove it? This is the list of clock inputs, not outputs.

...probably because I was thinking the specification was exactly the other way around.

Does a "clocks = ..." statement always mean input clocks?

I shall correct that and re-test (might take a while, I don't have the hardware anymore).

Quoted 13 lines

Quoted 12 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 327

Quoted 9 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +3 -0

@@ -322,6 +322,10 @@
        clock-frequency = <22579200>;
 };
 
+&audio_clk_c {
+       clock-frequency = <12288000>;
+};

Does the ADV7482 always generate a 12.288 MHz clock signal? Or is this programmable?

Quoted 2 lines

Oops. It looks like it is and the value is derived from the sampling rate (48kHz) and the master clock multiplier. Both hard-coded in the board file.

Where are these hardcoded in the board file?

Even if they are, technically this is a clock output of the ADV7482.

Quoted 11 lines

Quoted 8 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 485

Quoted 5 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -0

 video-receiver@70 {
    compatible = "adi,adv7482";
...
+   clocks = <&rcar_sound 3>, <&audio_clk_c>;
+   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";

The above declares the Audio CLK C to be a clock input of the ADV7482, while it is an output.

I would gladly give it right direction if I *really* understood what I was doing...

:-)

Quoted 3 lines

Quoted 1 line

Furthermore, the DT bindings do not document that clocks can be specified.

Should the DT bindings document that the clock cannot be specified than?

It currently does say so, as it doesn't list "clocks" in its properties section.

Quoted 10 lines

Quoted 9 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 704

Quoted 7 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -1

@@ -686,7 +700,8 @@
        };
 
        sound_pins: sound {
-               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+                        "ssi4_data";

Missing "ss4_ctrl", for the SCK4 and WS4 pins.

Quoted 3 lines

I'll add them.
As the device seems to function even without thoes, does this mean the pins in the group are used "on demand" by whatever needs them?

Probably the SCK4/WS4 functions are the reset-state defaults.

Quoted 2 lines

Does a "clocks = ..." statement always mean input clocks?

Yes it does.
If a device has clock outputs and is thus a clock provider, it should have a #clock-cells property, and this should be documented in the bindings.

A clock consumer will refer to clocks of a provider using the "clocks" property, specifying a clock specifier (phandle and zero or more indices) for each clock referenced.

Geert Uytterhoeven, Mon, Mar 02, 2020 14:47:46 +0100:

Quoted 14 lines · Alex Riesen

Quoted 13 lines

Quoted 12 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 327

Quoted 9 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +3 -0

@@ -322,6 +322,10 @@
        clock-frequency = <22579200>;
 };
 
+&audio_clk_c {
+       clock-frequency = <12288000>;
+};

Does the ADV7482 always generate a 12.288 MHz clock signal? Or is this programmable?

Quoted 4 lines

Quoted 2 lines

Oops. It looks like it is and the value is derived from the sampling rate (48kHz) and the master clock multiplier. Both hard-coded in the board file.

Where are these hardcoded in the board file?

In the endpoint definition, arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts

So the frequency can be set at the run-time, perhaps even derived from endpoint connected to the output. In this case, rsnd_endpoint3, which has the "mclk-fs" setting. Not sure if the sampling rate can be set to something else for the HDMI, though.

Quoted 1 line

Even if they are, technically this is a clock output of the ADV7482.

... which I hope to correct as soon as I steal the hardware from whoever stole it from me...

Quoted 18 lines

Quoted 11 lines

Quoted 8 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 485

Quoted 5 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -0

 video-receiver@70 {
    compatible = "adi,adv7482";
...
+   clocks = <&rcar_sound 3>, <&audio_clk_c>;
+   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";

The above declares the Audio CLK C to be a clock input of the ADV7482, while it is an output.

I would gladly give it right direction if I *really* understood what I was doing...

:-)

Quoted 3 lines

Quoted 1 line

Furthermore, the DT bindings do not document that clocks can be specified.

Should the DT bindings document that the clock cannot be specified than?

Quoted 1 line

It currently does say so, as it doesn't list "clocks" in its properties section.

The bindings documentation file, which we're talking about here and which does not list the specifiable input clocks in its properties, is it the

Documentation/devicetree/bindings/media/i2c/adv748x.txt

?

And this absence of documentation also means that whatever clocks (both input in "clocks=" and output in "#clock-cells") listed in a specific .dts are just an integration detail?

Does this below makes more sense, than?

    video-receiver@70 {
        compatible = "adi,adv7482";
        clocks = <&rcar_sound 3>;
        clock-names = "clk-hdmi-video";
        adv748x_mclk: mclk {
            compatible = "fixed-clock";
            #clock-cells =  <0>;
            /* frequency hard-coded for illustration */
            clock-frequency = <12288000>;
            clock-output-names = "clk-hdmi-i2s-mclk";
        };
    };

Now I'm a bit hazy on how to declare that the MCLK output of the video-receiver@70 is connected to the Audio Clock C of the SoC... Probably remove use of "audio_clk_c" completely?

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi line 704

Quoted 7 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -1

@@ -686,7 +700,8 @@
        };
 
        sound_pins: sound {
-               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+                        "ssi4_data";

Missing "ss4_ctrl", for the SCK4 and WS4 pins.

I'll add them.
As the device seems to function even without thoes, does this mean the pins in the group are used "on demand" by whatever needs them?

Quoted 1 line

Probably the SCK4/WS4 functions are the reset-state defaults.

That ... might require some trial and testing: when I add them to the group, the reset defaults will be overridden by the platform initialization, which is not necessarily the reset default. Will see.

Quoted 9 lines

Quoted 1 line

Does a "clocks = ..." statement always mean input clocks?

Yes it does.
If a device has clock outputs and is thus a clock provider, it should have a #clock-cells property, and this should be documented in the bindings.

A clock consumer will refer to clocks of a provider using the "clocks" property, specifying a clock specifier (phandle and zero or more indices) for each clock referenced.

Something like this?

    &rcar_sound {
        clocks = ...,
                 <&adv748x_mclk>,
                 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
        clock-names = ...,
                      "clk_c",
                      "clk_i";
    };

Quoted 3 lines

And this absence of documentation also means that whatever clocks (both input in "clocks=" and output in "#clock-cells") listed in a specific .dts are just an integration detail?

No, the absence probably means that any clock-related properties in a .dts file will just be ignored.

Looking at the driver source, it indeed has no support related to clocks at all.

Quoted 14 lines

Does this below makes more sense, than?

    video-receiver@70 {
        compatible = "adi,adv7482";
        clocks = <&rcar_sound 3>;
        clock-names = "clk-hdmi-video";
        adv748x_mclk: mclk {
            compatible = "fixed-clock";
            #clock-cells =  <0>;
            /* frequency hard-coded for illustration */
            clock-frequency = <12288000>;
            clock-output-names = "clk-hdmi-i2s-mclk";
        };
    };

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

There is no need for a fixed-clock compatible, nor for clock-frequency and clock-output-names.

But most important: this should be documented in the adv748x DT bindings, and implemented in the adv748x driver.

Quoted 12 lines

Quoted 9 lines

Quoted 1 line

Does a "clocks = ..." statement always mean input clocks?

Yes it does.
If a device has clock outputs and is thus a clock provider, it should have a #clock-cells property, and this should be documented in the bindings.

A clock consumer will refer to clocks of a provider using the "clocks" property, specifying a clock specifier (phandle and zero or more indices) for each clock referenced.

Something like this?

Quoted 8 lines

    &rcar_sound {
        clocks = ...,
                 <&adv748x_mclk>,
                 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
        clock-names = ...,
                      "clk_c",
                      "clk_i";
    };

More or less.

Might become

    find_a_better_label_choice: video-receiver@70 {
            ...
    };

    &rcar_sound {
            clock = ...,
                    <&find_a_better_label_choice 0>,
                    ...
    };

as there may be multiple clock outputs on the ADV7482.

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:

Quoted 8 lines

Quoted 3 lines

And this absence of documentation also means that whatever clocks (both input in "clocks=" and output in "#clock-cells") listed in a specific .dts are just an integration detail?

No, the absence probably means that any clock-related properties in a .dts file will just be ignored.

Looking at the driver source, it indeed has no support related to clocks at all.

...

Quoted 15 lines

Quoted 14 lines

Does this below makes more sense, than?

    video-receiver@70 {
        compatible = "adi,adv7482";
        clocks = <&rcar_sound 3>;
        clock-names = "clk-hdmi-video";
        adv748x_mclk: mclk {
            compatible = "fixed-clock";
            #clock-cells =  <0>;
            /* frequency hard-coded for illustration */
            clock-frequency = <12288000>;
            clock-output-names = "clk-hdmi-i2s-mclk";
        };
    };

Quoted 2 lines

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock for I2S output)... I shall re-check, just in case.

Quoted 5 lines

There is no need for a fixed-clock compatible, nor for clock-frequency and clock-output-names.

But most important: this should be documented in the adv748x DT bindings, and implemented in the adv748x driver.

So if the driver is to export that clock for the kernel (like in this case), it must implement its support?

Quoted 7 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:

Quoted 3 lines

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock for I2S output)... I shall re-check, just in case.

Quoted 8 lines

Quoted 5 lines

There is no need for a fixed-clock compatible, nor for clock-frequency and clock-output-names.

But most important: this should be documented in the adv748x DT bindings, and implemented in the adv748x driver.

So if the driver is to export that clock for the kernel (like in this case), it must implement its support?

Exactly. Unless that pin is hardcoded to output a fixed clock, in which case you can just override the existing audio_clk_c rate.

Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:

Quoted 8 lines · Alex Riesen

Quoted 7 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:

Quoted 3 lines

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock for I2S output)... I shall re-check, just in case.

And you're right, of course: the audio output formatting module of the ADV748x devices provides a set of clock lines related to the I2S pins: the already discussed master clock, left-right channel clock and the serial clock (bit clock?).

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

There is no need for a fixed-clock compatible, nor for clock-frequency and clock-output-names.

But most important: this should be documented in the adv748x DT bindings, and implemented in the adv748x driver.

So if the driver is to export that clock for the kernel (like in this case), it must implement its support?

Exactly. Unless that pin is hardcoded to output a fixed clock, in which case you can just override the existing audio_clk_c rate.

Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate clock in the driver, added a clock provider:

adv748x_probe:

    clk = clk_register_fixed_rate(state->dev,
                                  "clk-hdmi-i2s-mclk",
                                  NULL     /* parent_name */,
                                  0        /* flags */,
                                  12288000 /* rate */);
    of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);

And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c in the list of input clocks of the R-Car-side sound card with the phandle of the adv7482 main node:

salvator-common.dtsi:

    &i2c4 {
        status = "okay";

        adv7482_hdmi_decoder: video-receiver@70 {
            #clock-cells = <0>; // to be replaced with <1>
        };
    };

    &rcar_sound {
        clocks = ..., <&adv7482_hdmi_decoder>, ...;
    };

As everything continues to work as before, I assume that at least the clock dependencies were resolved.

Is there a way to verify that the added input clock is actually used? IOW, if its frequency is actually has been programmed into the ssi4 (R-Car receiving hardware) registers, and not just a left-over from previuos attempts or plain default setting?

As the ADV748x devices seem to provide also the clocks for video outputs, will it make any sense to place the clock definition into the port node? Or should all provided clocks be indexed in the main device node?

Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:

Quoted 6 lines · Alex Riesen

Quoted 5 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:

Quoted 3 lines

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

Quoted 2 lines

Quoted 2 lines

AFAICS, the device can provide only this one clock line (audio master clock for I2S output)... I shall re-check, just in case.

And you're right, of course: the audio output formatting module of the ADV748x devices provides a set of clock lines related to the I2S pins: the already discussed master clock, left-right channel clock and the serial clock (bit clock?).

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

There is no need for a fixed-clock compatible, nor for clock-frequency and clock-output-names.

But most important: this should be documented in the adv748x DT bindings, and implemented in the adv748x driver.

So if the driver is to export that clock for the kernel (like in this case), it must implement its support?

Exactly. Unless that pin is hardcoded to output a fixed clock, in which case you can just override the existing audio_clk_c rate.

Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate clock in the driver, added a clock provider:

adv748x_probe:

    clk = clk_register_fixed_rate(state->dev,
				  "clk-hdmi-i2s-mclk",
				  NULL     /* parent_name */,
				  0        /* flags */,
				  12288000 /* rate */);
    of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);

And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c in the list of input clocks of the R-Car-side sound card with the phandle of the adv7482 main node:

salvator-common.dtsi:

    &i2c4 {
	status = "okay";

	adv7482_hdmi_decoder: video-receiver@70 {
	    #clock-cells = <0>; // to be replaced with <1>
	};
    };

    &rcar_sound {
	clocks = ..., <&adv7482_hdmi_decoder>, ...;
    };

As everything continues to work as before, I assume that at least the clock dependencies were resolved.

Is there a way to verify that the added input clock is actually used? IOW, if its frequency is actually has been programmed into the ssi4 (R-Car receiving hardware) registers, and not just a left-over from previuos attempts or plain default setting?

As the ADV748x devices seem to provide also the clocks for video outputs, will it make any sense to place the clock definition into the port node? Or should all provided clocks be indexed in the main device node?

Quoted 10 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:

Quoted 8 lines · Alex Riesen

Quoted 7 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:

Quoted 3 lines

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock for I2S output)... I shall re-check, just in case.

Quoted 4 lines

And you're right, of course: the audio output formatting module of the ADV748x devices provides a set of clock lines related to the I2S pins: the already discussed master clock, left-right channel clock and the serial clock (bit clock?).

I don't think we need to model the last two clocks through CCF though, they're part of the I2S protocol, not clock sources that need to be explicitly controlled (or queried).

Quoted 26 lines

Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate clock in the driver, added a clock provider:

adv748x_probe:

    clk = clk_register_fixed_rate(state->dev,
				  "clk-hdmi-i2s-mclk",
				  NULL     /* parent_name */,
				  0        /* flags */,
				  12288000 /* rate */);
    of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);

And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c in the list of input clocks of the R-Car-side sound card with the phandle of the adv7482 main node:

salvator-common.dtsi:

    &i2c4 {
	status = "okay";

	adv7482_hdmi_decoder: video-receiver@70 {
	    #clock-cells = <0>; // to be replaced with <1>
	};
    };

Quoted 6 lines

    &rcar_sound {
	clocks = ..., <&adv7482_hdmi_decoder>, ...;
    };

As everything continues to work as before, I assume that at least the clock dependencies were resolved.

This looks good to me.

Quoted 5 lines

Is there a way to verify that the added input clock is actually used? IOW, if its frequency is actually has been programmed into the ssi4 (R-Car receiving hardware) registers, and not just a left-over from previuos attempts or plain default setting?

Quoted 3 lines

As the ADV748x devices seem to provide also the clocks for video outputs, will it make any sense to place the clock definition into the port node? Or should all provided clocks be indexed in the main device node?

Those clocks are part of the CSI-2 protocol and also don't need to be explicitly controlled. As far as I can tell from a quick check of the ADV7482 documentation, only the I2S MCLK is a general-purpose clock that needs to be exposed.

Laurent Pinchart, Fri, Mar 06, 2020 14:16:32 +0100:

Quoted 11 lines · Alex Riesen

Quoted 10 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:

Quoted 8 lines · Alex Riesen

Quoted 7 lines

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:

Quoted 3 lines

The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock for I2S output)... I shall re-check, just in case.

Quoted 8 lines

Quoted 4 lines

And you're right, of course: the audio output formatting module of the ADV748x devices provides a set of clock lines related to the I2S pins: the already discussed master clock, left-right channel clock and the serial clock (bit clock?).

I don't think we need to model the last two clocks through CCF though, they're part of the I2S protocol, not clock sources that need to be explicitly controlled (or queried).

That's good, because I'm right now having hard time finding out how to calculate the frequencies!

Quoted 26 lines

Quoted 26 lines

Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate clock in the driver, added a clock provider:

adv748x_probe:

    clk = clk_register_fixed_rate(state->dev,
				  "clk-hdmi-i2s-mclk",
				  NULL     /* parent_name */,
				  0        /* flags */,
				  12288000 /* rate */);
    of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);

And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c in the list of input clocks of the R-Car-side sound card with the phandle of the adv7482 main node:

salvator-common.dtsi:

    &i2c4 {
	status = "okay";

	adv7482_hdmi_decoder: video-receiver@70 {
	    #clock-cells = <0>; // to be replaced with <1>
	};
    };

Quoted 8 lines

Quoted 6 lines

    &rcar_sound {
	clocks = ..., <&adv7482_hdmi_decoder>, ...;
    };

As everything continues to work as before, I assume that at least the clock dependencies were resolved.

This looks good to me.

Ok, I settle on this than.

Quoted 5 lines

Quoted 5 lines

Is there a way to verify that the added input clock is actually used? IOW, if its frequency is actually has been programmed into the ssi4 (R-Car receiving hardware) registers, and not just a left-over from previuos attempts or plain default setting?

Quoted 8 lines

Quoted 3 lines

As the ADV748x devices seem to provide also the clocks for video outputs, will it make any sense to place the clock definition into the port node? Or should all provided clocks be indexed in the main device node?

Those clocks are part of the CSI-2 protocol and also don't need to be explicitly controlled. As far as I can tell from a quick check of the ADV7482 documentation, only the I2S MCLK is a general-purpose clock that needs to be exposed.

Thanks, that's good to know!

Do you know, by chance, which of the snd_soc* callbacks should be used to implement setting of the MCLK? The one in snd_soc_component_driver or snd_soc_dai_driver->ops (snd_soc_dai_ops)?

Or how the userspace interface looks like? Or, if there is no userspace interface for this, how the MCLK is supposed to be set? Through mclk-fs?

Quoted 4 lines

Quoted 4 lines

Those clocks are part of the CSI-2 protocol and also don't need to be explicitly controlled. As far as I can tell from a quick check of the ADV7482 documentation, only the I2S MCLK is a general-purpose clock that needs to be exposed.

Quoted 6 lines

Do you know, by chance, which of the snd_soc* callbacks should be used to implement setting of the MCLK? The one in snd_soc_component_driver or snd_soc_dai_driver->ops (snd_soc_dai_ops)?

Or how the userspace interface looks like? Or, if there is no userspace interface for this, how the MCLK is supposed to be set? Through mclk-fs?

I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san is the main developer and maintainer of Renesas sound drivers. Morimoto-sensei, would you have an answer to that question ? :-)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Those clocks are part of the CSI-2 protocol and also don't need to be explicitly controlled. As far as I can tell from a quick check of the ADV7482 documentation, only the I2S MCLK is a general-purpose clock that needs to be exposed.

(snip)

Quoted 10 lines

Quoted 6 lines

Do you know, by chance, which of the snd_soc* callbacks should be used to implement setting of the MCLK? The one in snd_soc_component_driver or snd_soc_dai_driver->ops (snd_soc_dai_ops)?

Or how the userspace interface looks like? Or, if there is no userspace interface for this, how the MCLK is supposed to be set? Through mclk-fs?

I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san is the main developer and maintainer of Renesas sound drivers. Morimoto-sensei, would you have an answer to that question ? :-)

In my quick check, it goes to AUDIO_CLKC.
If so, you can update rcar_sound::clocks.

	&rcar_sound {
		...

(quoted diff) +2 -1

-		/* update <audio_clk_b> to <cs2000> */
+		/* update <audio_clk_b> to <cs2000>,
+		 *        <audio_clk_c> to <adv748x> */

		clocks = <&cpg CPG_MOD 1005>,
			...
			 <&audio_clk_a>, <&cs2000>,

(quoted diff) +1 -1

-			 <&audio_clk_c>,
+			 <&adv748x>,

			 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;

Thank you for your help !!

Should the adv748x driver also implement anything to configure the frequency of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of snd_soc_dai_ops?

Or is the driver implementation, which depends on mclk-fs to be 256, the audio stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on the HDMI input, a totally acceptable first attempt at writing a DAI driver?

I'm a bit bothered by that, as the hardware is also capable of decoding stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted formats, 128-768fs MCLK multipliers, and a row of I2S options.

I just find it confusing to place the configuration interfaces. For instance, the patches use the media ioctl for audio output selection to select I2S protocol. While works, it does not feel right (shouldn't it be in the device tree?)

Maybe you can point me at a driver doing something similar? I'm studying media drivers now, but not many of them use ASoC interfaces for devices providing a clock. Or maybe I should better look at sound/soc/...?

Quoted 12 lines

Should the adv748x driver also implement anything to configure the frequency of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of snd_soc_dai_ops?

Or is the driver implementation, which depends on mclk-fs to be 256, the audio stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on the HDMI input, a totally acceptable first attempt at writing a DAI driver?

I'm a bit bothered by that, as the hardware is also capable of decoding stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted formats, 128-768fs MCLK multipliers, and a row of I2S options.

Quoted 8 lines

I just find it confusing to place the configuration interfaces. For instance, the patches use the media ioctl for audio output selection to select I2S protocol. While works, it does not feel right (shouldn't it be in the device tree?)

Maybe you can point me at a driver doing something similar? I'm studying media drivers now, but not many of them use ASoC interfaces for devices providing a clock. Or maybe I should better look at sound/soc/...?

Setting Sound Clock for all cases/patterns are very complex and difficult actually. (ADV7482 configuration) x (ADG divider / selector) x etc, etc...

Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing route clock (= no configuration, fixed clock), and ADG divides it,
and provide best clock to each SSIx.
Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A), and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ? Or providing specific clock for some case is enough
(ADG will automatically select it if necessary).

If ADV7482 needs more detail clock settings combination, then, there is no method to adjust to it.
We need to consider such system somehow.

Kuninori Morimoto, Tue, Mar 10, 2020 02:07:23 +0100:

Quoted 24 lines

Quoted 12 lines

Should the adv748x driver also implement anything to configure the frequency of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of snd_soc_dai_ops?

Or is the driver implementation, which depends on mclk-fs to be 256, the audio stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on the HDMI input, a totally acceptable first attempt at writing a DAI driver?

I'm a bit bothered by that, as the hardware is also capable of decoding stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted formats, 128-768fs MCLK multipliers, and a row of I2S options.

Quoted 8 lines

I just find it confusing to place the configuration interfaces. For instance, the patches use the media ioctl for audio output selection to select I2S protocol. While works, it does not feel right (shouldn't it be in the device tree?)

Maybe you can point me at a driver doing something similar? I'm studying media drivers now, but not many of them use ASoC interfaces for devices providing a clock. Or maybe I should better look at sound/soc/...?

Setting Sound Clock for all cases/patterns are very complex and difficult actually. (ADV7482 configuration) x (ADG divider / selector) x etc, etc...

Quoted 7 lines

Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing route clock (= no configuration, fixed clock), and ADG divides it,
and provide best clock to each SSIx.
Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A), and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ? Or providing specific clock for some case is enough
(ADG will automatically select it if necessary).

In this particular case, the ADV7482 *must* provide the clock, I believe: it extracts the audio stream from the HDMI connection (in addition to everything else) and serves the stream on I2S. Its MCLK line is physically connected to the CLK_C line (which is an input) of the R-Car SoC. The I2S audio transmission does not work if the ADV7482 clock is not programmed (or programmed incorrectly).
Yes, I tried (I also tried programming it incorrectly, just because I didn't know what I was doing).

Quoted 3 lines

If ADV7482 needs more detail clock settings combination, then, there is no method to adjust to it.
We need to consider such system somehow.

Not encouraging...

Maybe I should leave the clock fixed, with the frequency configuration in the device tree, e.g. as adv7482 port node property "clock-frequency". Which feels rather pathetic, but at least serves my purpose (48k, 8x24).

But let me describe the situation as I see it first.

As far as I understand, the SSI4 (Salvator-X board) should be programmed by the snd-soc-rcar driver in the "slave receiver" mode for this use case, which is HDMI input ADV7482 (I2S master, TDM) -> SSI4 (I2S slave)):

[   63.305990] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: rsnd-dai.1 : sysclk = 66666664, direction 0
[   63.306028] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: adv748x-i2s : sysclk = 12288000, direction 1

I am a bit bothered by the fact that sysclk of rsnd-dai.1 does not match that sysclk of adv7482-i2s, but I think it's just DT node configuration.

[   63.306033] asoc_simple_card_set_dailink_name: asoc-audio-graph-card sound: name : rsnd-dai.1-adv748x-i2s
...
[   63.332641] asoc-audio-graph-card sound: adv748x.4-0070 <-> rsnd-dai.1 mapping ok
...
[   63.341317] dapm_connect_dai_link_widgets:  rsnd-dai.1-adv748x-i2s: connected DAI link adv748x.4-0070:Capture -> ec500000.sound:DAI1 Capture
...
[  128.961389] rsnd_write: rcar_sound ec500000.sound: w ssi[4] - SSICR ( 124) : 9ceb0100

Decoding this last line (9ceb0100) gives SSICR.TRMD (bit1) =0, SSICR.SCKD (bit15) =0, SSICR.SWSD (bit14) =0. The combination is documented as "slave receiver". Which, I assume, makes SSI4 use the external clock. Given the received stream looks ok, something also must have set the dividers correctly.

From the above, I conclude, whatever the complexity of the audio system clock configuration, it seems to be implemented for the case.

I only miss a more or less clear way to configure the I2S master (ADV7482, that is).