| Message ID | 5011ceef-5100-441d-b169-dabba135d27f@iinet.net.au (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ASoC: pcm3168a: Add option to force clock consumer | expand |
On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote: > Hi, > > Resending with signoff tag and including linux-sound on CC. As covered in submitting-patches.rst please put any administrative information after the --- so that it is is automatically removed by tooling. There is no need to resubmit for this alone.
On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote: > Scenario is that DAC's clock pins are tied to ADC's clock pins, with > codec acting as clock producer for the I2S bus, and the CPU has a single > pair of I2S clock pins used for both playback and capture. > ,------------------- > --------- ------------| LRCK DAC > LRCK|-------+--/ -------| BCK (producer) C > CPU | | / | O > BCK|-------|---+--/ | ---------------- D > consumer | \ | | E > --------- ---------------| LRCK ADC C > `-----------| BCK (consumer) > `-------------------- > Both DAI links will have codec acting as a clock producer, but we need > > to configure the codec so that only one of ADC/DAC drives the lines. This looks like something that should be done in the board configuration, not hard coded in the CODEC driver. > Add DT options to support this configuration. adc-force-cons/dac-force-cons > > cause the corresponding component to be configured in consumer mode. Any DT bindings should documented which should be done in a separate patch. > + // Force clock consumer mode if needed > + if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC) > + ms = 0; > + if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC) > + ms = 0; The clock consumer mode should just be configured via the standard set_dai_fmt() operation. > + if (dev->of_node) { > + pcm3168a->adc_fc = of_property_read_bool(dev->of_node, > + "adc-force-cons"); > + pcm3168a->dac_fc = of_property_read_bool(dev->of_node, > + "dac-force-cons"); > + } These properties are not documented, and you don't need to query to see if there's an OF node to query if a property is present.
On 6/12/2024 11:22 pm, Mark Brown wrote: > On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote: > >> Scenario is that DAC's clock pins are tied to ADC's clock pins, with >> codec acting as clock producer for the I2S bus, and the CPU has a single >> pair of I2S clock pins used for both playback and capture. >> ,------------------- >> --------- ------------| LRCK DAC >> LRCK|-------+--/ -------| BCK (producer) C >> CPU | | / | O >> BCK|-------|---+--/ | ---------------- D >> consumer | \ | | E >> --------- ---------------| LRCK ADC C >> `-----------| BCK (consumer) >> `-------------------- >> Both DAI links will have codec acting as a clock producer, but we need >> >> to configure the codec so that only one of ADC/DAC drives the lines. > This looks like something that should be done in the board > configuration, not hard coded in the CODEC driver. > >> Add DT options to support this configuration. adc-force-cons/dac-force-cons >> >> cause the corresponding component to be configured in consumer mode. > Any DT bindings should documented which should be done in a separate > patch. I will most certainly do this in v3 if I convince you that the DT bindings should be added in the first place :) > > >> + // Force clock consumer mode if needed >> + if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC) >> + ms = 0; >> + if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC) >> + ms = 0; > The clock consumer mode should just be configured via the standard > set_dai_fmt() operation. I did try to do this using simple_card and I documented my attempt and why it seems to fail in this message: https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@iinet.net.au/ Basically, simple_card appears to set the CPU as producer if you don't specify a producer. I am not sure whether this is a bug. Also, there aren't any examples I could find in the documentation of DAI links with clock consumer on both ends, so I was wondering if it is even valid. There is some SoC code that seems like it might not work properly in this scenario: https://github.com/torvalds/linux/blob/b8f52214c61a5b99a54168145378e91b40d10c90/sound/soc/soc-core.c#L1465 >> + if (dev->of_node) { >> + pcm3168a->adc_fc = of_property_read_bool(dev->of_node, >> + "adc-force-cons"); >> + pcm3168a->dac_fc = of_property_read_bool(dev->of_node, >> + "dac-force-cons"); >> + } > These properties are not documented, and you don't need to query to see > if there's an OF node to query if a property is present.
On Sat, Dec 07, 2024 at 12:16:28AM +1100, Stephen Gordon wrote: > On 6/12/2024 11:22 pm, Mark Brown wrote: > > On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote: > > > + // Force clock consumer mode if needed > > > + if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC) > > > + ms = 0; > > > + if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC) > > > + ms = 0; > > The clock consumer mode should just be configured via the standard > > set_dai_fmt() operation. > I did try to do this using simple_card and I documented my attempt and why > it seems to > > fail in this message: You should use one of the audio-graph-card bindings for anything new. You probably want to fix the formatting in your mail client, it's doing somet very weird line wrapping and inserting spurious blank lines. > https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@iinet.net.au/ > Basically, simple_card appears to set the CPU as producer if you don't > specify a > producer. I am not sure whether this is a bug. Well, if nothing is configured it's got to pick a default? > Also, there aren't any examples I could find in the documentation of DAI > links with > > clock consumer on both ends, so I was wondering if it is even valid. No, that's not valid. The CODEC is the clock provider here and should understand that. I *think* the audio graph cards can handle this case by having both CODEC DAIs on a single PCM but I could be misremembering and it may only be the DPCM cases.
On Fri, 6 Dec 2024 14:13:14 +0000 Mark Brown <broonie@kernel.org> wrote: > On Sat, Dec 07, 2024 at 12:16:28AM +1100, Stephen Gordon wrote: > > On 6/12/2024 11:22 pm, Mark Brown wrote: > > > On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote: > > > > > + // Force clock consumer mode if needed > > > > + if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC) > > > > + ms = 0; > > > > + if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC) > > > > + ms = 0; > > > The clock consumer mode should just be configured via the standard > > > set_dai_fmt() operation. > > > I did try to do this using simple_card and I documented my attempt > > and why it seems to > > > > fail in this message: > > You should use one of the audio-graph-card bindings for anything new. > > You probably want to fix the formatting in your mail client, it's > doing somet very weird line wrapping and inserting spurious blank > lines. > > > https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@iinet.net.au/ > > > > > Basically, simple_card appears to set the CPU as producer if you > > don't specify a > > > producer. I am not sure whether this is a bug. > > Well, if nothing is configured it's got to pick a default? > > > Also, there aren't any examples I could find in the documentation > > of DAI links with > > > > clock consumer on both ends, so I was wondering if it is even > > valid. > > No, that's not valid. The CODEC is the clock provider here and should > understand that. I *think* the audio graph cards can handle this case > by having both CODEC DAIs on a single PCM but I could be > misremembering and it may only be the DPCM cases. Thanks for confirming that a link must have a producer on one end. The challenge is to configure it so that the codec end of the DAI link is a producer, but the dai_fmt on the codec's DAI gets set to consumer. I'll try to do this using the audio graph cards. Hopefully the formatting is better, I have changed mail clients. Stephen
Hi Stephen > > You should use one of the audio-graph-card bindings for anything new. Using audio-graph-card is good idea, but all simple_card/audio-graph-card/audio-graph-card2 are using same logic around here. So, you will have same issue on audio-graph-card too. > > > Basically, simple_card appears to set the CPU as producer if you > > > don't specify a producer. I am not sure whether this is a bug. > > > > Well, if nothing is configured it's got to pick a default? If my understand was correct, your issue can be solved... dailink_out_master: simple-audio-card,dai-link@0 { ... => pcm3168_playback: codec { ... }; }; dailink_in_slave: simple-audio-card,dai-link@1 { => bitclock-master = <&pcm3168_playback>; => frame-master = <&pcm3168_playback>; ... }; asoc_simple_parse_daifmt() is just checking where the node was codec node or not. So, if bitclock-master/frame-master were produced, but was not codec, both CPU/Codec can be consumer ? # Clock producer/consumer settings is very confusable, because it was # Codec base, and has flip, etc... Thank you for your help !! Best regards --- Kuninori Morimoto
On Mon, 9 Dec 2024 01:58:59 +0000 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Stephen > > > > You should use one of the audio-graph-card bindings for anything > > > new. > > Using audio-graph-card is good idea, but all > simple_card/audio-graph-card/audio-graph-card2 are using same logic > around here. So, you will have same issue on audio-graph-card too. > > > > > Basically, simple_card appears to set the CPU as producer if you > > > > don't specify a producer. I am not sure whether this is a bug. > > > > > > > > > > Well, if nothing is configured it's got to pick a default? > > If my understand was correct, your issue can be solved... > > dailink_out_master: simple-audio-card,dai-link@0 { > ... > => pcm3168_playback: codec { > ... > }; > }; > dailink_in_slave: simple-audio-card,dai-link@1 { > => bitclock-master = <&pcm3168_playback>; > => frame-master = <&pcm3168_playback>; > ... > }; > > asoc_simple_parse_daifmt() is just checking where the node was codec > node or not. So, if bitclock-master/frame-master were produced, but > was not codec, both CPU/Codec can be consumer ? > > # Clock producer/consumer settings is very confusable, because it was > # Codec base, and has flip, etc... > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto Hi Morimoto-san, This is one of the things I tried initially, but I wasn't sure if it was a valid configuration. I just tried it again with debug enabled (see dt_snippet.txt) and I get the attached (see dbgout.txt) in dmesg. It is trying to set the CPU end as producer. I think it's because asoc_simple_parse_daifmt() checks whether the bit/frame phandles passed match the codec phandle - since it doesn't (it's a different codec), it sets the DAI link as "consumer mode" (i.e. clocks come from CPU). Therefore, the CPU side gets configured as producer. I am using the version from 6.12.3 as that is the latest I can build. Regards Stephen [ 688.239375] asoc-simple-card soc@107c000000:sound: link 2, dais 4, ccnf 0 [ 688.239394] asoc-simple-card soc@107c000000:sound: link_of (/soc@107c000000/sound/simple-audio-card,dai-link@1) [ 688.267041] asoc-simple-card soc@107c000000:sound: link 2, dais 4, ccnf 0 [ 688.267057] asoc-simple-card soc@107c000000:sound: link_of (/soc@107c000000/sound/simple-audio-card,dai-link@1) [ 688.267103] asoc-simple-card soc@107c000000:sound: link_of (/soc@107c000000/sound/simple-audio-card,dai-link@0) [ 688.267128] asoc-simple-card soc@107c000000:sound: Card Name: i2smulti [ 688.267131] asoc-simple-card soc@107c000000:sound: DAI0 [ 688.267133] asoc-simple-card soc@107c000000:sound: cpu num = 1 [ 688.267136] asoc-simple-card soc@107c000000:sound: cpu slots = 2 [ 688.267138] asoc-simple-card soc@107c000000:sound: cpu slot width = 32 [ 688.267141] asoc-simple-card soc@107c000000:sound: cpu sysclk = 50000000Hz [ 688.267143] asoc-simple-card soc@107c000000:sound: cpu direction = IN [ 688.267145] asoc-simple-card soc@107c000000:sound: codec num = 1 [ 688.267147] asoc-simple-card soc@107c000000:sound: codec slots = 2 [ 688.267149] asoc-simple-card soc@107c000000:sound: codec slot width = 32 [ 688.267151] asoc-simple-card soc@107c000000:sound: codec sysclk = 24576000Hz [ 688.267153] asoc-simple-card soc@107c000000:sound: codec direction = IN [ 688.267155] asoc-simple-card soc@107c000000:sound: dai name = 1f000a4000.i2s-pcm3168a-adc [ 688.267157] asoc-simple-card soc@107c000000:sound: dai format = 4001 [ 688.267160] asoc-simple-card soc@107c000000:sound: DAI1 [ 688.267162] asoc-simple-card soc@107c000000:sound: cpu num = 1 [ 688.267164] asoc-simple-card soc@107c000000:sound: cpu slots = 2 [ 688.267166] asoc-simple-card soc@107c000000:sound: cpu slot width = 32 [ 688.267168] asoc-simple-card soc@107c000000:sound: cpu sysclk = 50000000Hz [ 688.267170] asoc-simple-card soc@107c000000:sound: cpu direction = IN [ 688.267172] asoc-simple-card soc@107c000000:sound: codec num = 1 [ 688.267174] asoc-simple-card soc@107c000000:sound: codec slots = 2 [ 688.267175] asoc-simple-card soc@107c000000:sound: codec slot width = 32 [ 688.267177] asoc-simple-card soc@107c000000:sound: codec sysclk = 24576000Hz [ 688.267179] asoc-simple-card soc@107c000000:sound: codec direction = IN [ 688.267181] asoc-simple-card soc@107c000000:sound: dai name = 1f000a4000.i2s-pcm3168a-dac [ 688.267183] asoc-simple-card soc@107c000000:sound: dai format = 1001 [ 688.267424] designware-i2s 1f000a4000.i2s: ASoC: error at snd_soc_dai_set_fmt on 1f000a4000.i2s: -22 [ 688.267596] asoc-simple-card soc@107c000000:sound: probe with driver asoc-simple-card failed with error -22 fragment@2 { target = <&sound>; __overlay__ { compatible = "simple-audio-card"; #address-cells = <1>; #size-cells = <0>; i2s-controller = <&i2s_clk_consumer>; status="okay"; simple-audio-card,name = "i2smulti"; simple-audio-card,format = "i2s"; dailink_out_master: simple-audio-card,dai-link@0 { reg = <0>; format = "i2s"; bitclock-master = <&pcm3168_playback>; frame-master = <&pcm3168_playback>; cpu { sound-dai = <&i2s_clk_consumer>; dai-tdm-slot-num = <2>; dai-tdm-slot-width = <32>; }; pcm3168_playback: codec { sound-dai = <&pcm3168a 0>; dai-tdm-slot-num = <2>; dai-tdm-slot-width = <32>; }; }; dailink_in_slave: simple-audio-card,dai-link@1 { reg = <1>; format = "i2s"; bitclock-master = <&pcm3168_playback>; frame-master = <&pcm3168_playback>; cpu { sound-dai = <&i2s_clk_consumer>; dai-tdm-slot-num = <2>; dai-tdm-slot-width = <32>; }; pcm3168_capture: codec { sound-dai = <&pcm3168a 1>; dai-tdm-slot-num = <2>; dai-tdm-slot-width = <32>; }; }; }; };
Hi Mark Cc Stephen > It is trying to set the CPU end as producer. I think it's because > asoc_simple_parse_daifmt() checks whether the bit/frame phandles passed > match the codec phandle - since it doesn't (it's a different codec), > it sets the DAI link as "consumer mode" (i.e. clocks come from CPU). > Therefore, the CPU side gets configured as producer. (snip) > > # Clock producer/consumer settings is very confusable, because it was > > # Codec base, and has flip, etc... Ah.. indeed, but hmm... Current ASoC provider/consumer settings for clock/frame is set via dai_link->dai_fmt. static int soc_init_pcm_runtime(...) { ... => ret = snd_soc_runtime_set_dai_fmt(..., dai_link->dai_fmt); ... } And use it for both Codec (A) and CPU (B) with flipping (C) int snd_soc_runtime_set_dai_fmt(..., dai_fmt) { ... ^ for_each_rtd_codec_dais(rtd, i, codec_dai) { (A) ... v } /* Flip the polarity for the "CPU" end of link */ (C) dai_fmt = snd_soc_daifmt_clock_provider_flipped(dai_fmt); ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { (B) ... v } ... } So, I think we can't use both CPU/Codec as "consumer" (or "provider") on current ASoC, but what do you think, Mark ? Because of ASoC history, the clock/frame provider/consumer settings was from Codec point of view (CBx_CFx), and "Sound Card" is still based on this. OTOH, each CPU/Codec driver is now using own base (Bx_Fx). Because of it we need flipping (C). From "Sound Card" point of view, setup directly Bx_Fx base instead of CBx_CFx base is not a big deal. But if we do it, we need to have such setting for each DAI, instead of common dai_link. Now dai_link->dai_fmt is including 4 type of info 1. DAI hardware audio formats 2. DAI Clock gating 3. DAI hardware signal polarity 4. DAI hardware clock providers/consumers I think we want to separate 4 (and maybe 2 too ?) from it to support more flexible system ? Legacy system dai_link->dai_fmt // include all 1, 2, 3, 4 New system dai_link->dai_fmt // include 1, 2, 3 (or 1, 3) dai_link->cpus[idx].fmt // include 4 (or 2, 4) fmt = dai_link->dai_fmt | dai_link->cpus[i].fmt "flip" has effect to 4 only, so New system has no issue with flipping on dai_link->dai_fmt. > simple_card/audio-graph-card/audio-graph-card2 are using same logic around here. I'm sorry, this was wrong. simple_card/audio-graph-card are using same function/method, but audio-graph-card2 is using own method to handing dai_link->dai_fmt Thank you for your help !! Best regards --- Kuninori Morimoto
> From "Sound Card" point of view, setup directly Bx_Fx base instead of > CBx_CFx base is not a big deal. But if we do it, we need to have such > setting for each DAI, instead of common dai_link. > > Now dai_link->dai_fmt is including 4 type of info > > 1. DAI hardware audio formats > 2. DAI Clock gating > 3. DAI hardware signal polarity > 4. DAI hardware clock providers/consumers > > I think we want to separate 4 (and maybe 2 too ?) from it to support > more flexible system ? > > Legacy system > dai_link->dai_fmt // include all 1, 2, 3, 4 > > New system > dai_link->dai_fmt // include 1, 2, 3 (or 1, 3) > dai_link->cpus[idx].fmt // include 4 (or 2, 4) > > fmt = dai_link->dai_fmt | dai_link->cpus[i].fmt > > "flip" has effect to 4 only, so New system has no issue with flipping > on dai_link->dai_fmt. Hi Morimoto-san, This is a great idea. I'd add that #4 could still be in dai_link->dai_fmt, but the new field in CPU/codec (snd_soc_dai_link_component.fmt) should override the setting (not a simple OR). This would preserve backward compatibility (I think). It greatly improves support for multiple codecs sharing clock lines, where 1 codec is the producer and others are consumers. The bit/frame logic in simple/graph cards should be able to be changed to handle this properly too. Without this type of override, the original patch is needed (or the card driver needs to write registers directly, which seems bad). I will wait to hear Mark's opinion. Stephen
Hi Stephen, Mark > > From "Sound Card" point of view, setup directly Bx_Fx base instead of > > CBx_CFx base is not a big deal. But if we do it, we need to have such > > setting for each DAI, instead of common dai_link. > > > > Now dai_link->dai_fmt is including 4 type of info > > > > 1. DAI hardware audio formats > > 2. DAI Clock gating > > 3. DAI hardware signal polarity > > 4. DAI hardware clock providers/consumers > > > > I think we want to separate 4 (and maybe 2 too ?) from it to support > > more flexible system ? > > > > Legacy system > > dai_link->dai_fmt // include all 1, 2, 3, 4 > > > > New system > > dai_link->dai_fmt // include 1, 2, 3 (or 1, 3) > > dai_link->cpus[idx].fmt // include 4 (or 2, 4) > > > > fmt = dai_link->dai_fmt | dai_link->cpus[i].fmt > > > > "flip" has effect to 4 only, so New system has no issue with flipping > > on dai_link->dai_fmt. > > Hi Morimoto-san, > > This is a great idea. I'd add that #4 could still be in > dai_link->dai_fmt, but the new field in CPU/codec > (snd_soc_dai_link_component.fmt) should override the setting (not a > simple OR). This would preserve backward compatibility (I think). It > greatly improves support for multiple codecs sharing clock lines, where > 1 codec is the producer and others are consumers. The bit/frame logic in > simple/graph cards should be able to be changed to handle this properly too. > > Without this type of override, the original patch is needed (or the card > driver needs to write registers directly, which seems bad). > > I will wait to hear Mark's opinion. I'm now preparing the patch for it. I want to more test it. I think I can post it next week if Mark is OK for it. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/codecs/pcm3168a.c b/sound/soc/codecs/pcm3168a.c index 9d6431338..1a4cf8d63 100644 --- a/sound/soc/codecs/pcm3168a.c +++ b/sound/soc/codecs/pcm3168a.c @@ -61,6 +61,7 @@ struct pcm3168a_priv { struct clk *scki; struct gpio_desc *gpio_rst; unsigned long sysclk; + bool adc_fc, dac_fc; // Force clock consumer mode struct pcm3168a_io_params io_params[2]; struct snd_soc_dai_driver dai_drv[2]; @@ -479,6 +480,12 @@ static int pcm3168a_hw_params(struct snd_pcm_substream *substream, ms = 0; } + // Force clock consumer mode if needed + if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC) + ms = 0; + if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC) + ms = 0; + format = io_params->format;
Hi, Resending with signoff tag and including linux-sound on CC. Scenario is that DAC's clock pins are tied to ADC's clock pins, with codec acting as clock producer for the I2S bus, and the CPU has a single pair of I2S clock pins used for both playback and capture. ,------------------- --------- ------------| LRCK DAC LRCK|-------+--/ -------| BCK (producer) C CPU | | / | O BCK|-------|---+--/ | ---------------- D consumer | \ | | E --------- ---------------| LRCK ADC C `-----------| BCK (consumer) `-------------------- Both DAI links will have codec acting as a clock producer, but we need to configure the codec so that only one of ADC/DAC drives the lines. Add DT options to support this configuration. adc-force-cons/dac-force-cons cause the corresponding component to be configured in consumer mode. Signed-off-by: Stephen Gordon <gordoste@iinet.net.au> if (io_params->slot_width) @@ -757,6 +764,15 @@ int pcm3168a_probe(struct device *dev, struct regmap *regmap) pcm3168a->sysclk = clk_get_rate(pcm3168a->scki); + adc_fc = false; + dac_fc = false; + if (dev->of_node) { + pcm3168a->adc_fc = of_property_read_bool(dev->of_node, + "adc-force-cons"); + pcm3168a->dac_fc = of_property_read_bool(dev->of_node, + "dac-force-cons"); + } + for (i = 0; i < ARRAY_SIZE(pcm3168a->supplies); i++) pcm3168a->supplies[i].supply = pcm3168a_supply_names[i];