| Message ID | 20190917181233.534-2-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] ASoC: tegra: Add a TDM configuration callback | expand |
On 9/17/19 1:12 PM, Ben Dooks wrote: > From: Edward Cragg <edward.cragg@codethink.co.uk> > > Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' > driver. > > Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> > --- > sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c > index ac6983c6bd72..d36b4662b420 100644 > --- a/sound/soc/tegra/tegra30_i2s.c > +++ b/sound/soc/tegra/tegra30_i2s.c > @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, > return 0; > } > > +/* > + * Set up TDM > + */ > +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, > + unsigned int tx_mask, unsigned int rx_mask, > + int slots, int slot_width) > +{ > + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); > + unsigned int mask = 0, val = 0; initialization is not needed. > + > + dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x " > + "slots: 0x%08x " "width: %d\n", > + __func__, tx_mask, rx_mask, slots, slot_width); > + > + /* Set up slots and tx/rx masks */ > + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | > + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | > + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK; > + > + val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | > + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | > + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT); > + > + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); > + > + /* Set FSYNC width */ > + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, > + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, > + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT); > + > + return 0; > +} > + > static int tegra30_i2s_probe(struct snd_soc_dai *dai) > { > struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); > @@ -268,6 +301,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = { > .set_fmt = tegra30_i2s_set_fmt, > .hw_params = tegra30_i2s_hw_params, > .trigger = tegra30_i2s_trigger, > + .set_tdm_slot = tegra30_i2s_set_tdm, > }; > > static const struct snd_soc_dai_driver tegra30_i2s_dai_template = { >
On 17/09/2019 19:12, Ben Dooks wrote: > From: Edward Cragg <edward.cragg@codethink.co.uk> > > Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' > driver. > > Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> > --- > sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c > index ac6983c6bd72..d36b4662b420 100644 > --- a/sound/soc/tegra/tegra30_i2s.c > +++ b/sound/soc/tegra/tegra30_i2s.c > @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, > return 0; > } > > +/* > + * Set up TDM > + */ > +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, > + unsigned int tx_mask, unsigned int rx_mask, > + int slots, int slot_width) > +{ > + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); > + unsigned int mask = 0, val = 0; > + > + dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x " dev_dbg() please. Also I don't think it is necessary to print both the function name and 'setting TDM', the function name should be sufficient. > + "slots: 0x%08x " "width: %d\n", Why are there extra quotes here? > + __func__, tx_mask, rx_mask, slots, slot_width)> + > + /* Set up slots and tx/rx masks */ > + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | > + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | > + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK; > + > + val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | > + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | > + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT); > + > + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); > + > + /* Set FSYNC width */ > + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, > + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, > + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT); What happens if there is only one slot? The fsync will be the width of the slot. Typically, TDM is used with DSP-A/B formats and although the driver does not appear to program the fsync width, it probably should during the tegra30_i2s_set_fmt() depending on the format used rather than here. Cheers Jon
On 2019-09-18 09:42, Jon Hunter wrote: > On 17/09/2019 19:12, Ben Dooks wrote: >> From: Edward Cragg <edward.cragg@codethink.co.uk> >> >> Add a callback to configure TDM settings for the Tegra30 I2S ASoC >> 'platform' >> driver. >> >> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> >> --- >> sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/sound/soc/tegra/tegra30_i2s.c >> b/sound/soc/tegra/tegra30_i2s.c >> index ac6983c6bd72..d36b4662b420 100644 >> --- a/sound/soc/tegra/tegra30_i2s.c >> +++ b/sound/soc/tegra/tegra30_i2s.c >> @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct >> snd_pcm_substream *substream, int cmd, >> return 0; >> } >> >> +/* >> + * Set up TDM >> + */ >> +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, >> + unsigned int tx_mask, unsigned int rx_mask, >> + int slots, int slot_width) >> +{ >> + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); >> + unsigned int mask = 0, val = 0; >> + >> + dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x >> " > > dev_dbg() please. Also I don't think it is necessary to print both the > function name and 'setting TDM', the function name should be > sufficient. Yes, already sorted from previous review. >> + "slots: 0x%08x " "width: %d\n", > > Why are there extra quotes here? No idea, I'll remove these later. >> + __func__, tx_mask, rx_mask, slots, slot_width)> + >> + /* Set up slots and tx/rx masks */ >> + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | >> + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | >> + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK; >> + >> + val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | >> + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | >> + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT); >> + >> + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); >> + >> + /* Set FSYNC width */ >> + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, >> + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, >> + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT); > > What happens if there is only one slot? The fsync will be the width of > the slot. Typically, TDM is used with DSP-A/B formats and although the > driver does not appear to program the fsync width, it probably should > during the tegra30_i2s_set_fmt() depending on the format used rather > than here. Hmm, should we check. The work was done to keep as close to the original client's 2.6 kernel as possible which set the fsync field to a whole data word. We could try and just set this to say 2 here and have a much shorter frame-sync pulse. I'll add a check for slots < 2 and set the fsync width to 2. Thanks for the review. > > Cheers > Jon
On 18/09/2019 11:11, Ben Dooks wrote: > > > On 2019-09-18 09:42, Jon Hunter wrote: >> On 17/09/2019 19:12, Ben Dooks wrote: >>> From: Edward Cragg <edward.cragg@codethink.co.uk> >>> >>> Add a callback to configure TDM settings for the Tegra30 I2S ASoC >>> 'platform' >>> driver. >>> >>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk> >>> --- >>> sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/sound/soc/tegra/tegra30_i2s.c >>> b/sound/soc/tegra/tegra30_i2s.c >>> index ac6983c6bd72..d36b4662b420 100644 >>> --- a/sound/soc/tegra/tegra30_i2s.c >>> +++ b/sound/soc/tegra/tegra30_i2s.c >>> @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct >>> snd_pcm_substream *substream, int cmd, >>> return 0; >>> } >>> >>> +/* >>> + * Set up TDM >>> + */ >>> +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, >>> + unsigned int tx_mask, unsigned int rx_mask, >>> + int slots, int slot_width) >>> +{ >>> + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); >>> + unsigned int mask = 0, val = 0; >>> + >>> + dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: >>> 0x%08x " >> >> dev_dbg() please. Also I don't think it is necessary to print both the >> function name and 'setting TDM', the function name should be sufficient. > > Yes, already sorted from previous review. > >>> + "slots: 0x%08x " "width: %d\n", >> >> Why are there extra quotes here? > > No idea, I'll remove these later. > >>> + __func__, tx_mask, rx_mask, slots, slot_width)> + >>> + /* Set up slots and tx/rx masks */ >>> + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | >>> + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | >>> + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK; >>> + >>> + val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | >>> + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | >>> + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT); >>> + >>> + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); >>> + >>> + /* Set FSYNC width */ >>> + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, >>> + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, >>> + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT); >> >> What happens if there is only one slot? The fsync will be the width of >> the slot. Typically, TDM is used with DSP-A/B formats and although the >> driver does not appear to program the fsync width, it probably should >> during the tegra30_i2s_set_fmt() depending on the format used rather >> than here. > > Hmm, should we check. > > The work was done to keep as close to the original client's 2.6 kernel > as possible which set the fsync field to a whole data word. We could try > and just set this to say 2 here and have a much shorter frame-sync pulse. > > I'll add a check for slots < 2 and set the fsync width to 2. Why 2? From looking at various codecs that support dsp-a/b modes, it is more common for the f-sync to be 1 regardless of the number of slots. Jon
On Wed, Sep 18, 2019 at 11:25:39AM +0100, Jon Hunter wrote: > Why 2? From looking at various codecs that support dsp-a/b modes, it is > more common for the f-sync to be 1 regardless of the number of slots. In DSP modes only one edge really matters anyway so it's not super important how long the pulse is.
On 2019-09-18 11:48, Mark Brown wrote: > On Wed, Sep 18, 2019 at 11:25:39AM +0100, Jon Hunter wrote: > >> Why 2? From looking at various codecs that support dsp-a/b modes, it >> is >> more common for the f-sync to be 1 regardless of the number of slots. > > In DSP modes only one edge really matters anyway so it's not super > important how long the pulse is. I could never get an answer for why this was set as-is on the customer's setup and not sure if I have the ability to re-test this.
On 9/18/19 5:48 AM, Mark Brown wrote: > On Wed, Sep 18, 2019 at 11:25:39AM +0100, Jon Hunter wrote: > >> Why 2? From looking at various codecs that support dsp-a/b modes, it is >> more common for the f-sync to be 1 regardless of the number of slots. > > In DSP modes only one edge really matters anyway so it's not super > important how long the pulse is. There are exceptions to the rule. In the early days of SOF, we had to provide support for amplifiers that did require a pulse larger than a bit. In the SOF IPC we added an 'frame_pulse_width' field to pass the configuration all the way from topology to the firmware and Intel SSP driver. The other quirk we added is the ability to control zero-padding per slot instead of at the end of the frame, e.g. 1 bit of padding after 24 bits when using 4 slots w/ 25 bits in a 100-bit frame.
On Wed, Sep 18, 2019 at 08:33:50AM -0500, Pierre-Louis Bossart wrote: > On 9/18/19 5:48 AM, Mark Brown wrote: > > In DSP modes only one edge really matters anyway so it's not super > > important how long the pulse is. > There are exceptions to the rule. > In the early days of SOF, we had to provide support for amplifiers that did > require a pulse larger than a bit. In the SOF IPC we added an > 'frame_pulse_width' field to pass the configuration all the way from > topology to the firmware and Intel SSP driver. > The other quirk we added is the ability to control zero-padding per slot > instead of at the end of the frame, e.g. 1 bit of padding after 24 bits when > using 4 slots w/ 25 bits in a 100-bit frame. Neither of those is part of the core DSP mode definition though in the same way that constraints like MCLK or BCLK ratios aren't. They're modifiers on top.
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..d36b4662b420 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; } +/* + * Set up TDM + */ +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, + unsigned int tx_mask, unsigned int rx_mask, + int slots, int slot_width) +{ + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); + unsigned int mask = 0, val = 0; + + dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x " + "slots: 0x%08x " "width: %d\n", + __func__, tx_mask, rx_mask, slots, slot_width); + + /* Set up slots and tx/rx masks */ + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK; + + val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT); + + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); + + /* Set FSYNC width */ + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT); + + return 0; +} + static int tegra30_i2s_probe(struct snd_soc_dai *dai) { struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); @@ -268,6 +301,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = { .set_fmt = tegra30_i2s_set_fmt, .hw_params = tegra30_i2s_hw_params, .trigger = tegra30_i2s_trigger, + .set_tdm_slot = tegra30_i2s_set_tdm, }; static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {