| Message ID | 20240315112745.63230-1-bastien.curutchet@bootlin.com |
|---|---|
| Headers | show |
| Series | ASoC: ti: davinci-i2s: Add features to McBSP driver | expand |
On Fri, Mar 15, 2024 at 12:27:36PM +0100, Bastien Curutchet wrote: > - dev_err(&pdev->dev, "no mem resource?\n"); > - return -ENODEV; > + return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n"); > } dev_err_probe() with a fixed error code doesn't seem to make much sense, the whole point is to handle deferral but for a straight lookup like this that can't happen.
On Fri, Mar 15, 2024 at 12:27:41PM +0100, Bastien Curutchet wrote: > McBSP can generate an SYNCERR when unexpected frame pulses are > detected. The driver always disables this feature and ignore the > unexpected frame pulses. What does "unexpected" mean? > Enable the generation of SYNCERR by the McBSP according to the > 'ti,enable-sync-err' device-tree property. Why would this be optional, and how is this reported - I'm not seeing any interrupt handling updates?
On Fri, Mar 15, 2024 at 12:27:42PM +0100, Bastien Curutchet wrote: > McBSP has free-running mode where serial clocks continue to run during > emulation halts. This mode is always enabled by the driver. > > Disable this free-running mode according to the 'ti,disable-free-run' > device-tree property. This sounds like SND_SOC_DAIFMT_CONT rather than a DT property.
Hi Mark, On Fri, 15 Mar 2024 14:07:13 +0000 Mark Brown <broonie@kernel.org> wrote: > On Fri, Mar 15, 2024 at 12:27:36PM +0100, Bastien Curutchet wrote: > > > - dev_err(&pdev->dev, "no mem resource?\n"); > > - return -ENODEV; > > + return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n"); > > } > > dev_err_probe() with a fixed error code doesn't seem to make much sense, > the whole point is to handle deferral but for a straight lookup like > this that can't happen. The error code is uniformly formatted and the error path is more compact. https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963 IMHO, to benefit of these feature, it makes sense to use it even with a fixed error code. Best regards, Hervé
On Fri, Mar 15, 2024 at 03:23:32PM +0100, Herve Codina wrote: > Mark Brown <broonie@kernel.org> wrote: > > dev_err_probe() with a fixed error code doesn't seem to make much sense, > > the whole point is to handle deferral but for a straight lookup like > > this that can't happen. > The error code is uniformly formatted and the error path is more compact. > https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963 > IMHO, to benefit of these feature, it makes sense to use it even with a fixed > error code. I'm not convinced TBH, the fixed error code smells pretty bad.
Hi Mark, On 3/15/24 15:09, Mark Brown wrote: > On Fri, Mar 15, 2024 at 12:27:41PM +0100, Bastien Curutchet wrote: > >> McBSP can generate an SYNCERR when unexpected frame pulses are >> detected. The driver always disables this feature and ignore the >> unexpected frame pulses. > > What does "unexpected" mean? Unexpected frame sync pulse is defined in datasheet as a sync pulse that occurs <N> bit clocks earlier than the last transmitted bit of the previous frame. The <N> can be configured through registers. > >> Enable the generation of SYNCERR by the McBSP according to the >> 'ti,enable-sync-err' device-tree property. > > Why would this be optional, and how is this reported - I'm not seeing > any interrupt handling updates? It is possible to deliberately ignore them and that is what is done today in the driver. This is reported as a status bit in a register. An interrupt can indeed be generated from this but I'm not using it (now at least). I use the fact that McBSP automatically drops previous element and starts a new reception when an unexpected frame pulse occurs. Best regards, Bastien
On Fri, Mar 15, 2024 at 03:45:24PM +0100, Bastien Curutchet wrote: > On 3/15/24 15:09, Mark Brown wrote: > > On Fri, Mar 15, 2024 at 12:27:41PM +0100, Bastien Curutchet wrote: > > > McBSP can generate an SYNCERR when unexpected frame pulses are > > > detected. The driver always disables this feature and ignore the > > > unexpected frame pulses. > > What does "unexpected" mean? > Unexpected frame sync pulse is defined in datasheet as a sync pulse that > occurs <N> bit clocks earlier than the last transmitted bit of the previous > frame. The <N> can be configured through registers. > > > Enable the generation of SYNCERR by the McBSP according to the > > > 'ti,enable-sync-err' device-tree property. > > Why would this be optional, and how is this reported - I'm not seeing > > any interrupt handling updates? > It is possible to deliberately ignore them and that is what is done today in > the driver. > This is reported as a status bit in a register. An interrupt can indeed be > generated from this but I'm not using it (now at least). > I use the fact that McBSP automatically drops previous element and starts a > new reception when an unexpected frame pulse occurs. That sounds like a very standard behaviour for incorrect clocking. I don't think this needs configuration at all, just enable this mode.
Hi Mark, On 3/15/24 15:40, Mark Brown wrote: > On Fri, Mar 15, 2024 at 03:23:32PM +0100, Herve Codina wrote: >> Mark Brown <broonie@kernel.org> wrote: > >>> dev_err_probe() with a fixed error code doesn't seem to make much sense, >>> the whole point is to handle deferral but for a straight lookup like >>> this that can't happen. > >> The error code is uniformly formatted and the error path is more compact. >> https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963 > >> IMHO, to benefit of these feature, it makes sense to use it even with a fixed >> error code. > > I'm not convinced TBH, the fixed error code smells pretty bad. Ok. I'll keep the dev_err() for the fixed errors then, and use the dev_err_probe() for the others, would that be ok ? Best regards, Bastien
On Mon, Mar 18, 2024 at 08:40:24AM +0100, Bastien Curutchet wrote: > On 3/15/24 15:40, Mark Brown wrote: > > I'm not convinced TBH, the fixed error code smells pretty bad. > Ok. I'll keep the dev_err() for the fixed errors then, and use the > dev_err_probe() for the others, would that be ok ? Yes, when we can get a deferral it's the right thing.
On 15/03/2024 13:27, Bastien Curutchet wrote: > TDM is not supported by the McBSP driver. The McBSP datasheet does not > name explicitly TDM as a supported format but it is possible to configure > the McBSP to do TDM if all slots are used by McBSP. > > Add TDM support. It uses single-phase frame. Slot width is used to > compute the McBSP's word length. > > Implement the set_tdm_slot() hook of snd_soc_dai_ops struct. It only > supports TDM if all slots are used by McBSP. > > The snd_soc_dai_driver's capture.channels_max is updated from 2 to 128. > 128 is the maximum frame length when using single-phase frame. > playback.channels_max has not been updated as I could not test TDM on > playbacks with my hardware. > > This was tested on platform designed off of DAVINCI/OMAP_L138 with BP_FC > format so this is only supported format for TDM yet. A check is done in > davinci_i2s_set_dai_fmt() to prevent TDM to be used with other formats > > Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> > --- > sound/soc/ti/davinci-i2s.c | 81 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 77 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c > index f4514d4dff07..4adaed010700 100644 > --- a/sound/soc/ti/davinci-i2s.c > +++ b/sound/soc/ti/davinci-i2s.c > @@ -160,6 +160,9 @@ struct davinci_mcbsp_dev { > unsigned int fmt; > int clk_div; > bool i2s_accurate_sck; > + > + int tdm_slots; > + int slot_width; > }; > > static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev, > @@ -213,6 +216,57 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback) > toggle_clock(dev, playback); > } > > +static int davinci_i2s_tdm_word_length(int tdm_slot_width) > +{ > + switch (tdm_slot_width) { > + case 8: > + return DAVINCI_MCBSP_WORD_8; > + case 12: > + return DAVINCI_MCBSP_WORD_12; > + case 16: > + return DAVINCI_MCBSP_WORD_16; > + case 20: > + return DAVINCI_MCBSP_WORD_20; > + case 24: > + return DAVINCI_MCBSP_WORD_24; > + case 32: > + return DAVINCI_MCBSP_WORD_32; > + default: > + return -EINVAL; > + } > +} > + > +static int davinci_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai, > + unsigned int tx_mask, > + unsigned int rx_mask, > + int slots, int slot_width) > +{ > + struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(cpu_dai); > + > + dev_dbg(dev->dev, "%s - slots %d, slot_width %d\n", __func__, slots, slot_width); The __func__ can be ommited, it is better to leave it for dynamic debugging by adding "dyndbg=+pmf" module parameter if needed. > + > + if (slots > 128 || !slots) { > + dev_err(dev->dev, "Invalid number of slots\n"); > + return -EINVAL; > + } > + > + if (rx_mask != (1 << slots) - 1) { > + dev_err(dev->dev, "Invalid RX mask (0x%08x) : all slots must be used by McBSP\n", > + rx_mask); > + return -EINVAL; This is only a restriction for RX? > + } > + > + if (davinci_i2s_tdm_word_length(slot_width) < 0) { > + dev_err(dev->dev, "%s: Unsupported slot_width %d\n", __func__, slot_width); > + return -EINVAL; > + } > + > + dev->tdm_slots = slots; > + dev->slot_width = slot_width; > + > + return 0; > +} > + > #define DEFAULT_BITPERSAMPLE 16 > > static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, > @@ -228,6 +282,13 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, > DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1); > > dev->fmt = fmt; > + > + if ((dev->tdm_slots || dev->slot_width) && > + ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_BP_FC)) { > + dev_err(dev->dev, "TDM is only supported for BP_FC format\n"); > + return -EINVAL; I think this is not a valid statement, Fsync can be generated internally or coming from external source in TDM mode also. > + } > + > /* set master/slave audio interface */ > switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) { > case SND_SOC_DAIFMT_BP_FP: > @@ -383,7 +444,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream, > > master = dev->fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK; > fmt = params_format(params); > - mcbsp_word_length = asp_word_length[fmt]; > + if (dev->slot_width) > + mcbsp_word_length = davinci_i2s_tdm_word_length(dev->slot_width); > + else > + mcbsp_word_length = asp_word_length[fmt]; > + > + if (mcbsp_word_length < 0) > + return mcbsp_word_length; > > switch (master) { > case SND_SOC_DAIFMT_BP_FP: > @@ -483,8 +550,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream, > switch (master) { > case SND_SOC_DAIFMT_BP_FP: > case SND_SOC_DAIFMT_BP_FC: > - rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0); > - xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0); > + if (dev->tdm_slots > 0) { > + rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(dev->tdm_slots - 1); > + xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(dev->tdm_slots - 1); > + } else { > + rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0); > + xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0); > + } > break; > case SND_SOC_DAIFMT_BC_FC: > case SND_SOC_DAIFMT_BC_FP: > @@ -609,6 +681,7 @@ static const struct snd_soc_dai_ops davinci_i2s_dai_ops = { > .hw_params = davinci_i2s_hw_params, > .set_fmt = davinci_i2s_set_dai_fmt, > .set_clkdiv = davinci_i2s_dai_set_clkdiv, > + .set_tdm_slot = davinci_i2s_set_tdm_slot, > > }; > > @@ -621,7 +694,7 @@ static struct snd_soc_dai_driver davinci_i2s_dai = { > }, > .capture = { > .channels_min = 2, > - .channels_max = 2, > + .channels_max = 128, > .rates = DAVINCI_I2S_RATES, > .formats = DAVINCI_I2S_FORMATS, > },
On 15/03/2024 13:27, Bastien Curutchet wrote: > The McBSP's DX pin that outputs serial data during playback streams can > be used during capture streams to repeatedly output a chosen pattern. > For instance, this can be useful to drive an active-low signal during > captures (by choosing <0> as output pattern). Are there really any other use of this than to pull down or up the DX pin (0 or 0xffff)? If you just use the pin as GPIO then you don't need to change anything in the driver, The playback would not erach the pin, so no need to block it. > Enable this behaviour when the device-tree property 'ti,drive-dx' is > present. DX pin is driven with the provided pattern every time a > capture stream is launched. It is an interesting use of the hardware... You are controlling an external device (light an LED when capture is on)? > This property is not compatible with classic playback stream so > davinci_i2s_trigger() returns an error if a playback stream is started > while 'ti,drive-dx' flag is present. Propbaly add the .startup() callback and block the playback right there? > > This has been tested on a board designed of a DAVINCI/OMAP-L138 where > the DX pin is linked to the chip select pin of the converters of the > capture side. Isn't the DX will be pulled down as soon as the McBSP is enabled? Can you just re-configure the PUPD_SEL for the pin group to make the pin to be pulled the other way? > Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> > --- > sound/soc/ti/davinci-i2s.c | 74 ++++++++++++++++++++++++++++++++------ > 1 file changed, 63 insertions(+), 11 deletions(-) > > diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c > index 13e349e7a6ec..e289a84bdd6a 100644 > --- a/sound/soc/ti/davinci-i2s.c > +++ b/sound/soc/ti/davinci-i2s.c > @@ -101,6 +101,9 @@ > #define DAVINCI_MCBSP_PCR_FSRM (1 << 10) > #define DAVINCI_MCBSP_PCR_FSXM (1 << 11) > > +#define PLAYBACK_CLOCK 1 > +#define CAPTURE_CLOCK 0 > + > enum { > DAVINCI_MCBSP_WORD_8 = 0, > DAVINCI_MCBSP_WORD_12, > @@ -164,6 +167,8 @@ struct davinci_mcbsp_dev { > > bool sync_err; > bool free_run; > + bool drive_dx; > + u32 dx_val; > }; > > static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev, > @@ -187,6 +192,19 @@ static void toggle_clock(struct davinci_mcbsp_dev *dev, int playback) > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, dev->pcr); > } > > +static int davinci_drive_dx(struct davinci_mcbsp_dev *dev) > +{ > + unsigned int spcr; > + > + davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val); > + > + spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); > + spcr |= DAVINCI_MCBSP_SPCR_XRST; > + davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr); > + > + return 0; > +} > + > static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev, > struct snd_pcm_substream *substream) > { > @@ -194,6 +212,9 @@ static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev, > u32 spcr; > u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST; > > + if (!playback && dev->drive_dx) > + davinci_drive_dx(dev); > + > /* Enable transmitter or receiver */ > spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); > spcr |= mask; if (dev->drive_dx) { spcr |= DAVINCI_MCBSP_SPCR_XRST; davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val); } and no need for the davinci_drive_dx() function, plus it makes it symmetric of what you do on stop() > @@ -212,9 +233,17 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback) > /* Reset transmitter/receiver and sample rate/frame sync generators */ > spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); > spcr &= ~(DAVINCI_MCBSP_SPCR_GRST | DAVINCI_MCBSP_SPCR_FRST); > - spcr &= playback ? ~DAVINCI_MCBSP_SPCR_XRST : ~DAVINCI_MCBSP_SPCR_RRST; > - davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr); > - toggle_clock(dev, playback); > + > + if (!playback) { > + spcr &= ~DAVINCI_MCBSP_SPCR_RRST; > + davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr); > + toggle_clock(dev, CAPTURE_CLOCK); > + } > + if (playback || dev->drive_dx) { > + spcr &= ~DAVINCI_MCBSP_SPCR_XRST; > + davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr); > + toggle_clock(dev, PLAYBACK_CLOCK); > + } > } > > static int davinci_i2s_tdm_word_length(int tdm_slot_width) > @@ -408,6 +437,10 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, > } > if (inv_fs == true) > pcr ^= (DAVINCI_MCBSP_PCR_FSXP | DAVINCI_MCBSP_PCR_FSRP); > + > + if (dev->drive_dx) > + pcr |= DAVINCI_MCBSP_PCR_CLKRP; > + > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SRGR_REG, srgr); > dev->pcr = pcr; > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, pcr); > @@ -562,6 +595,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream, > xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1); > } > > + if (dev->drive_dx) > + xcr |= DAVINCI_MCBSP_XCR_XDATDLY(2); > + > if (params_channels(params) == 2) { > element_cnt = 2; > if (double_fmt[fmt] && dev->enable_channel_combine) { > @@ -611,9 +647,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream, > xcr |= DAVINCI_MCBSP_XCR_XWDLEN1(mcbsp_word_length) | > DAVINCI_MCBSP_XCR_XWDLEN2(mcbsp_word_length); > > - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK || dev->drive_dx) > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_XCR_REG, xcr); > - else > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_RCR_REG, rcr); > > pr_debug("%s - %d srgr=%X\n", __func__, __LINE__, srgr); > @@ -628,16 +664,21 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream, > struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(dai); > int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK); > u32 spcr; > - u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST; > > davinci_mcbsp_stop(dev, playback); > > spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); > - if (spcr & mask) { > + if (spcr & DAVINCI_MCBSP_SPCR_XRST) { > /* start off disabled */ > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, > - spcr & ~mask); > - toggle_clock(dev, playback); > + spcr & ~DAVINCI_MCBSP_SPCR_XRST); > + toggle_clock(dev, PLAYBACK_CLOCK); > + } > + if (spcr & DAVINCI_MCBSP_SPCR_RRST) { > + /* start off disabled */ > + davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, > + spcr & ~DAVINCI_MCBSP_SPCR_RRST); > + toggle_clock(dev, CAPTURE_CLOCK); > } > if (dev->pcr & (DAVINCI_MCBSP_PCR_FSXM | DAVINCI_MCBSP_PCR_FSRM | > DAVINCI_MCBSP_PCR_CLKXM | DAVINCI_MCBSP_PCR_CLKRM)) { > @@ -646,7 +687,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream, > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr); > } > > - if (playback) { > + if (playback || dev->drive_dx) { > /* Enable the transmitter */ > spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); > spcr |= DAVINCI_MCBSP_SPCR_XRST; > @@ -659,7 +700,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream, > spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); > spcr &= ~DAVINCI_MCBSP_SPCR_XRST; > davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr); > - toggle_clock(dev, playback); > + toggle_clock(dev, PLAYBACK_CLOCK); > } > > return 0; > @@ -672,6 +713,11 @@ static int davinci_i2s_trigger(struct snd_pcm_substream *substream, int cmd, > int ret = 0; > int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK); > > + if (playback && dev->drive_dx) { > + dev_err(dev->dev, "Playback is not allowed when drive-cs flag is set\n"); > + return -EINVAL; > + } > + > switch (cmd) { > case SNDRV_PCM_TRIGGER_START: > case SNDRV_PCM_TRIGGER_RESUME: > @@ -779,6 +825,12 @@ static int davinci_i2s_probe(struct platform_device *pdev) > > dev->free_run = !of_property_read_bool(pdev->dev.of_node, "ti,disable-free-run"); > dev->sync_err = of_property_read_bool(pdev->dev.of_node, "ti,enable-sync-err"); > + dev->drive_dx = false; no need to initialize it to 0, dev is allocated with devm_kzalloc() > + ret = of_property_read_u32(pdev->dev.of_node, "ti,drive-dx", &dev->dx_val); > + if (ret && ret != -EINVAL) > + return ret; > + if (!ret) > + dev->drive_dx = true; if (!ret) dev->drive_dx = true; else if (ret != -EINVAL) return ret; > > /* setup DMA, first TX, then RX */ > dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
Hi Péter, >> +static int davinci_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai, >> + unsigned int tx_mask, >> + unsigned int rx_mask, >> + int slots, int slot_width) >> +{ >> + struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(cpu_dai); >> + >> + dev_dbg(dev->dev, "%s - slots %d, slot_width %d\n", __func__, slots, slot_width); > > The __func__ can be ommited, it is better to leave it for dynamic > debugging by adding "dyndbg=+pmf" module parameter if needed. > True, I'll remove the __func__. >> + >> + if (slots > 128 || !slots) { >> + dev_err(dev->dev, "Invalid number of slots\n"); >> + return -EINVAL; >> + } >> + >> + if (rx_mask != (1 << slots) - 1) { >> + dev_err(dev->dev, "Invalid RX mask (0x%08x) : all slots must be used by McBSP\n", >> + rx_mask); >> + return -EINVAL; > > This is only a restriction for RX? > Nope you're right, I'll add the same for tx_mask. >> + } >> + >> + if (davinci_i2s_tdm_word_length(slot_width) < 0) { >> + dev_err(dev->dev, "%s: Unsupported slot_width %d\n", __func__, slot_width); >> + return -EINVAL; >> + } >> + >> + dev->tdm_slots = slots; >> + dev->slot_width = slot_width; >> + >> + return 0; >> +} >> + >> #define DEFAULT_BITPERSAMPLE 16 >> >> static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, >> @@ -228,6 +282,13 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, >> DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1); >> >> dev->fmt = fmt; >> + >> + if ((dev->tdm_slots || dev->slot_width) && >> + ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_BP_FC)) { >> + dev_err(dev->dev, "TDM is only supported for BP_FC format\n"); >> + return -EINVAL; > > I think this is not a valid statement, Fsync can be generated internally > or coming from external source in TDM mode also. > My hardware allow me to only test BP_FC so I wished to put some 'barriers' in front of untested things. Best regards, Bastien
Hi Péter, On 3/19/24 19:29, Péter Ujfalusi wrote: > > > On 15/03/2024 13:27, Bastien Curutchet wrote: >> The McBSP's DX pin that outputs serial data during playback streams can >> be used during capture streams to repeatedly output a chosen pattern. >> For instance, this can be useful to drive an active-low signal during >> captures (by choosing <0> as output pattern). > > Are there really any other use of this than to pull down or up the DX > pin (0 or 0xffff) I don't know, indeed today I can only think about these two patterns. I tried to do something in a 'generic' way so it can evolve if needed. > If you just use the pin as GPIO then you don't need to change anything > in the driver, The playback would not erach the pin, so no need to block it. > >> Enable this behaviour when the device-tree property 'ti,drive-dx' is >> present. DX pin is driven with the provided pattern every time a >> capture stream is launched. > > It is an interesting use of the hardware... You are controlling an > external device (light an LED when capture is on)? Yes I control the chip select pin of the ADC that is sending data to DR pin, that's why I need the DX pin to be synchronized with capture streams. >> This property is not compatible with classic playback stream so >> davinci_i2s_trigger() returns an error if a playback stream is started >> while 'ti,drive-dx' flag is present. > > Propbaly add the .startup() callback and block the playback right there? > Ok, TBH my mastery of the sound subsystem is not high enough to have an opinion of where this should go so I'll trust you on this. >> >> This has been tested on a board designed of a DAVINCI/OMAP-L138 where >> the DX pin is linked to the chip select pin of the converters of the >> capture side. > > Isn't the DX will be pulled down as soon as the McBSP is enabled? > Can you just re-configure the PUPD_SEL for the pin group to make the pin > to be pulled the other way? > Well, the acquisition chain in my use case is a bit convoluted. The DX pin's main purpose is to drive ADC chip select but it is also connected to other components and all this needs synchronization upon captures. I'll integrate your feedback about the code in next iteration, thank you. Best regards, Bastien
HI Bastien, On 20/03/2024 09:31, Bastien Curutchet wrote: >>> + if ((dev->tdm_slots || dev->slot_width) && >>> + ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != >>> SND_SOC_DAIFMT_BP_FC)) { >>> + dev_err(dev->dev, "TDM is only supported for BP_FC format\n"); >>> + return -EINVAL; >> >> I think this is not a valid statement, Fsync can be generated internally >> or coming from external source in TDM mode also. >> > > My hardware allow me to only test BP_FC so I wished to put some > 'barriers' in front of untested things. I don't see restrictions on the other changes affecting this to be lifted. I would allow TDM for full clock provider mode also. > > > Best regards, > Bastien
Hi Bastien, On 20/03/2024 10:52, Bastien Curutchet wrote: > Hi Péter, > > On 3/19/24 19:29, Péter Ujfalusi wrote: >> >> >> On 15/03/2024 13:27, Bastien Curutchet wrote: >>> The McBSP's DX pin that outputs serial data during playback streams can >>> be used during capture streams to repeatedly output a chosen pattern. >>> For instance, this can be useful to drive an active-low signal during >>> captures (by choosing <0> as output pattern). >> >> Are there really any other use of this than to pull down or up the DX >> pin (0 or 0xffff) > > I don't know, indeed today I can only think about these two patterns. > I tried to do something in a 'generic' way so it can evolve if needed. I think the definition of the 'ti,drive-dx' is somehow odd. It allows you to set it to 0x1234 and the DX pin will show 0x1234 when you capture 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no? If you have 4 channel capture then I won't speculate what will be on the DX pin ;) Would not be better to say that the DX pin will be driven low or high during capture _and_ disable the playback support? > >> If you just use the pin as GPIO then you don't need to change anything >> in the driver, The playback would not erach the pin, so no need to >> block it. >> >>> Enable this behaviour when the device-tree property 'ti,drive-dx' is >>> present. DX pin is driven with the provided pattern every time a >>> capture stream is launched. >> >> It is an interesting use of the hardware... You are controlling an >> external device (light an LED when capture is on)? > > Yes I control the chip select pin of the ADC that is sending data to DR > pin, that's why I need the DX pin to be synchronized with capture > streams. I see. Still a a novel use of a feature ;) > >>> This property is not compatible with classic playback stream so >>> davinci_i2s_trigger() returns an error if a playback stream is started >>> while 'ti,drive-dx' flag is present. >> >> Propbaly add the .startup() callback and block the playback right there? >> > > Ok, TBH my mastery of the sound subsystem is not high enough to have an > opinion of where this should go so I'll trust you on this. It would be more elegant to only create PCM for the capture in this case, but I would not bother with it. Stopping user right at startup time is second better. >>> >>> This has been tested on a board designed of a DAVINCI/OMAP-L138 where >>> the DX pin is linked to the chip select pin of the converters of the >>> capture side. >> >> Isn't the DX will be pulled down as soon as the McBSP is enabled? >> Can you just re-configure the PUPD_SEL for the pin group to make the pin >> to be pulled the other way? >> > > Well, the acquisition chain in my use case is a bit convoluted. The DX > pin's main purpose is to drive ADC chip select but it is also connected > to other components and all this needs synchronization upon captures. OK, thanks for the explanation.
On 20/03/2024 17:42, Péter Ujfalusi wrote: >>> On 15/03/2024 13:27, Bastien Curutchet wrote: >>>> The McBSP's DX pin that outputs serial data during playback streams can >>>> be used during capture streams to repeatedly output a chosen pattern. >>>> For instance, this can be useful to drive an active-low signal during >>>> captures (by choosing <0> as output pattern). >>> >>> Are there really any other use of this than to pull down or up the DX >>> pin (0 or 0xffff) >> >> I don't know, indeed today I can only think about these two patterns. >> I tried to do something in a 'generic' way so it can evolve if needed. > > I think the definition of the 'ti,drive-dx' is somehow odd. It allows > you to set it to 0x1234 and the DX pin will show 0x1234 when you capture > 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no? > If you have 4 channel capture then I won't speculate what will be on the > DX pin ;) > > Would not be better to say that the DX pin will be driven low or high > during capture _and_ disable the playback support? After some thinking, it might be still better to use the DX pin as GPIO and either have a custom machine driver which would handle it (set low when a capture trigger happens) or connect it in DAPM as a supply, bias or something and ASoC would handle it automagically. I think that would be cleaner in many ways. What do you think?
Hi Péter, On 3/20/24 21:30, Péter Ujfalusi wrote: > > > On 20/03/2024 17:42, Péter Ujfalusi wrote: >>>> On 15/03/2024 13:27, Bastien Curutchet wrote: >>>>> The McBSP's DX pin that outputs serial data during playback streams can >>>>> be used during capture streams to repeatedly output a chosen pattern. >>>>> For instance, this can be useful to drive an active-low signal during >>>>> captures (by choosing <0> as output pattern). >>>> >>>> Are there really any other use of this than to pull down or up the DX >>>> pin (0 or 0xffff) >>> >>> I don't know, indeed today I can only think about these two patterns. >>> I tried to do something in a 'generic' way so it can evolve if needed. >> >> I think the definition of the 'ti,drive-dx' is somehow odd. It allows >> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture >> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no? >> If you have 4 channel capture then I won't speculate what will be on the >> DX pin ;) >> >> Would not be better to say that the DX pin will be driven low or high >> during capture _and_ disable the playback support? > > After some thinking, it might be still better to use the DX pin as GPIO > and either have a custom machine driver which would handle it (set low > when a capture trigger happens) or connect it in DAPM as a supply, bias > or something and ASoC would handle it automagically. > > I think that would be cleaner in many ways. What do you think? > I agree, that would be cleaner. I ran a few tests to see if that would work on my hardware. It doesn't ... So I looked back to the schematics and found two reasons : * the DX pin needs to be in sync with the clock. * the DX pin needs to be in a high-impedance state between two frames so a pull-up can drive it back up. Actually, the DX pin is also linked to the FSR pin so it provides the frame clock to the capture stream. Bast regards, Bastien
Hi Bastien, On 3/21/24 17:14, Bastien Curutchet wrote: >>> I think the definition of the 'ti,drive-dx' is somehow odd. It allows >>> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture >>> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no? >>> If you have 4 channel capture then I won't speculate what will be on the >>> DX pin ;) >>> >>> Would not be better to say that the DX pin will be driven low or high >>> during capture _and_ disable the playback support? >> >> After some thinking, it might be still better to use the DX pin as GPIO >> and either have a custom machine driver which would handle it (set low >> when a capture trigger happens) or connect it in DAPM as a supply, bias >> or something and ASoC would handle it automagically. >> >> I think that would be cleaner in many ways. What do you think? >> > I agree, that would be cleaner. I ran a few tests to see if that would > work on my hardware. It doesn't ... So I looked back to the schematics > and found two reasons : > * the DX pin needs to be in sync with the clock. I'm not sure what this means, sync with which clock? > * the DX pin needs to be in a high-impedance state between two frames > so a pull-up can drive it back up. Actually, the DX pin is also > linked to the FSR pin so it provides the frame clock to the capture > stream. Hrm, you are using the DX pin as FSR for the capture? Why not McBSP.FSR pin? Looking back to the patch, one thing stood out: you are setting the XDATDLY to 2. You have some sort of T1 framing on the bus? The pullup will make the DX line high in for the framing bit, right? Or you simulate another FSR line with T1 framing DX? The 'ti,drive-dx' sounds like a bad property for sure, you have T1 framing and driving the DX to certain level. It is like DSP_A (1 bit delay) playing constant 0x2 ? Can you use aplay /dev/zero and a DT property to select T1 framing for the playback? Or that would be too coarse for timing the start of playback and capture?
Hi Péter, On 3/21/24 19:31, Péter Ujfalusi wrote: > Hi Bastien, > > On 3/21/24 17:14, Bastien Curutchet wrote: >>>> I think the definition of the 'ti,drive-dx' is somehow odd. It allows >>>> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture >>>> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no? >>>> If you have 4 channel capture then I won't speculate what will be on the >>>> DX pin ;) >>>> >>>> Would not be better to say that the DX pin will be driven low or high >>>> during capture _and_ disable the playback support? >>> >>> After some thinking, it might be still better to use the DX pin as GPIO >>> and either have a custom machine driver which would handle it (set low >>> when a capture trigger happens) or connect it in DAPM as a supply, bias >>> or something and ASoC would handle it automagically. >>> >>> I think that would be cleaner in many ways. What do you think? >>> >> I agree, that would be cleaner. I ran a few tests to see if that would >> work on my hardware. It doesn't ... So I looked back to the schematics >> and found two reasons : >> * the DX pin needs to be in sync with the clock. > > I'm not sure what this means, sync with which clock? > Sorry, that was not very clear, I meant sync with the bit block that is output on McBSP.CLKR pin. >> * the DX pin needs to be in a high-impedance state between two frames >> so a pull-up can drive it back up. Actually, the DX pin is also >> linked to the FSR pin so it provides the frame clock to the capture >> stream. > > Hrm, you are using the DX pin as FSR for the capture? Why not McBSP.FSR pin > The McBSP.FSR pin is used for the capture but is driven by the McBSP.DX pin. Both pins are linked together. > Looking back to the patch, one thing stood out: you are setting the > XDATDLY to 2. > You have some sort of T1 framing on the bus? The pullup will make the DX > line high in for the framing bit, right? > Or you simulate another FSR line with T1 framing DX? > Yes the goal is to simulate an FSR. > The 'ti,drive-dx' sounds like a bad property for sure, you have T1 > framing and driving the DX to certain level. > It is like DSP_A (1 bit delay) playing constant 0x2 ? > > Can you use aplay /dev/zero and a DT property to select T1 framing for > the playback? Or that would be too coarse for timing the start of > playback and capture? > That's a good idea, thank you. I'll try this and come back to you. Best regards, Bastien
Hi Péter, >> Can you use aplay /dev/zero and a DT property to select T1 framing for >> the playback? Or that would be too coarse for timing the start of >> playback and capture? >> > > That's a good idea, thank you. I'll try this and come back to you. > > I tried it and it works fine, thank you. We still need to run some performance tests before fully adopting it but anyway I'll send a new iteration of the patch series that drops the drive-dx part and just keeps a DT property to select T1 framing. Best regards, Bastien
This series aims to add some features to McBSP driver. Convert bindings from .txt to .yaml. Add possibility to use an external clock as sample rate generator's input. Add handling of new formats (TDM, S24_LE, BP_FC). Add optional properties in DT: - ti,enable-sync-err : Enable the detection of unexpected frame pulses - ti,disable-free-run : Disable the free-running mode where McBSP drives serial clocks during emulation halt - ti,drive-dx : Outputs a chosen pattern on DX pin during capture streams. This has been tested on a platform designed off of the DAVINCI/OMAP-L138 connected to 3 daisy-chained AD7767. An external clock drives the sample rate generator through the CLKS pin. The hardware I have only allowed me to test acquisition side of McBSP. It is connected to a 6 channels TDM and acts as Bit clock provider and Frame clock consumer. Bastien Curutchet (13): ASoC: dt-bindings: davinci-mcbsp: convert McBSP bindings to yaml schema ASoC: dt-bindings: davinci-mcbsp: Add new properties ASoC: ti: davinci-i2s: Remove the unused clk_input_pin attribute ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe ASoC: ti: davinci-i2s: Use external clock to drive sample rate generator ASoC: ti: davinci-i2s: Delete unnecessary assignment ASoC: ti: davinci-i2s: Add TDM support ASoC: ti: davinci-i2s: Add handling of BP_FC format ASoC: ti: davinci-i2s: Enable unexpected frame pulses detection ASoC: ti: davinci-i2s: Make free-running mode optional ASoC: ti: davinci-i2s: Add S24_LE to supported formats ASoC: dt-bindings: davinic-mcbsp: Add the 'ti,drive-dx' property ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams .../bindings/sound/davinci-mcbsp.txt | 50 --- .../bindings/sound/davinci-mcbsp.yaml | 119 +++++++ include/linux/platform_data/davinci_asp.h | 15 - sound/soc/ti/davinci-i2s.c | 333 ++++++++++++++---- 4 files changed, 376 insertions(+), 141 deletions(-) delete mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.txt create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml