Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Laurent Pinchart]

Hi Alex,

On Tue, Mar 10, 2020 at 09:17:14AM +0100, Alex Riesen wrote:
> Kuninori Morimoto, Tue, Mar 10, 2020 02:07:23 +0100:
> > > 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/...?
> > 
> > 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).
> 
> 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).
> 
> > 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).

As a stop-gap measure, until the sound driver programs the clock, you
can set its frequency in DT with the assigned-clock-rates property.

-- 
Regards,

Laurent Pinchart