Hi,

On Tue, Aug 20, 2019 at 08:46:30AM +0300, Sergey Suloev wrote:
> Hi, Maxime,
>
> On 8/19/19 10:25 PM, Maxime Ripard wrote:
> > From: Maxime Ripard <[email protected]>
> >
> > The i2s controller supports TDM, for up to 8 slots. Let's support the TDM
> > API.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > sound/soc/sunxi/sun4i-i2s.c | 40 ++++++++++++++++++++++++++++++++------
> > 1 file changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index 0dac09814b65..4f76daeaaed7 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -168,6 +168,8 @@ struct sun4i_i2s {
> > struct reset_control *rst;
> > unsigned int mclk_freq;
> > + unsigned int slots;
> > + unsigned int slot_width;
> > struct snd_dmaengine_dai_dma_data capture_dma_data;
> > struct snd_dmaengine_dai_dma_data playback_dma_data;
> > @@ -287,7 +289,7 @@ static bool sun4i_i2s_oversample_is_valid(unsigned int oversample)
> > static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
> > unsigned int rate,
> > - unsigned int channels,
> > + unsigned int slots,
> > unsigned int word_size)
> > {
> > struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > @@ -335,7 +337,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
> > bclk_parent_rate = i2s->variant->get_bclk_parent_rate(i2s);
> > bclk_div = sun4i_i2s_get_bclk_div(i2s, bclk_parent_rate,
> > - rate, channels, word_size);
> > + rate, slots, word_size);
> > if (bclk_div < 0) {
> > dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div);
> > return -EINVAL;
> > @@ -419,6 +421,10 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > const struct snd_pcm_hw_params *params)
> > {
> > unsigned int channels = params_channels(params);
> > + unsigned int slots = channels;
> > +
> > + if (i2s->slots)
> > + slots = i2s->slots;
> > /* Map the channels for playback and capture */
> > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> > @@ -428,7 +434,6 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > SUN4I_I2S_CHAN_SEL_MASK,
> > SUN4I_I2S_CHAN_SEL(channels));
> > -
> > regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
> > SUN4I_I2S_CHAN_SEL_MASK,
> > SUN4I_I2S_CHAN_SEL(channels));
> > @@ -452,10 +457,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > struct snd_soc_dai *dai)
> > {
> > struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > + unsigned int word_size = params_width(params);
> > unsigned int channels = params_channels(params);
> > + unsigned int slots = channels;
> > int ret, sr, wss;
> > u32 width;
> > + if (i2s->slots)
> > + slots = i2s->slots;
> > +
> > + if (i2s->slot_width)
> > + word_size = i2s->slot_width;
> > +
> > ret = i2s->variant->set_chan_cfg(i2s, params);
> > if (ret < 0) {
> > dev_err(dai->dev, "Invalid channel configuration\n");
> > @@ -477,15 +490,14 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > if (sr < 0)
> > return -EINVAL;
> > - wss = i2s->variant->get_wss(i2s, params_width(params));
> > + wss = i2s->variant->get_wss(i2s, word_size);
> > if (wss < 0)
> > return -EINVAL;
> > regmap_field_write(i2s->field_fmt_wss, wss);
> > regmap_field_write(i2s->field_fmt_sr, sr);
> > - return sun4i_i2s_set_clk_rate(dai, params_rate(params),
> > - channels, params_width(params));
> > + return sun4i_i2s_set_clk_rate(dai, params_rate(params), slots, word_size);
> > }
> > static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > @@ -785,10 +797,26 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > return 0;
> > }
> > +static int sun4i_i2s_set_tdm_slot(struct snd_soc_dai *dai,
> > + unsigned int tx_mask, unsigned int rx_mask,
> > + int slots, int slot_width)
> > +{
> > + struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > +
> > + if (slots > 8)
> > + return -EINVAL;
> > +
> > + i2s->slots = slots;
> > + i2s->slot_width = slot_width;
> > +
> > + return 0;
> > +}
> > +
> > static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > .hw_params = sun4i_i2s_hw_params,
> > .set_fmt = sun4i_i2s_set_fmt,
> > .set_sysclk = sun4i_i2s_set_sysclk,
> > + .set_tdm_slot = sun4i_i2s_set_tdm_slot,
> > .trigger = sun4i_i2s_trigger,
> > };
>
> it seems like you forgot to implement sun4i_i2s_dai_ops.set_bclk_ratio
> because, as I far as I understand, it should alter tdm slots functionality
> indirectly.

As far as I can see, while this indeed changes a few things on the TDM
setup, it's optional, orthogonal and it has a single user in the tree
(some intel platform card).

So I'd say that if someone ever needs it, we can have it, but it's not
a blocker.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (5.16 kB)
signature.asc (235.00 B)
Download all attachments

2019-08-21 12:10:03

On Mon, Aug 19, 2019 at 09:25:28PM +0200, Maxime Ripard wrote:
> From: Maxime Ripard <[email protected]>
>
> In addition to the I2S format, the controller also supports the DSP_*
> formats.

This doesn't seem to apply against current code, please check and
resend.

Attachments:

(No filename) (282.00 B)
signature.asc (499.00 B)
Download all attachments

2019-08-23 11:07:19

by Sergey Suloev

[permalink] [raw]

Subject: Re: [PATCH 20/21] ASoC: sun4i-i2s: Add support for TDM slots

Hi, Maxime,

On 8/21/19 3:05 PM, Maxime Ripard wrote:
> Hi,
>
> On Tue, Aug 20, 2019 at 08:46:30AM +0300, Sergey Suloev wrote:
>> Hi, Maxime,
>>
>> On 8/19/19 10:25 PM, Maxime Ripard wrote:
>>> From: Maxime Ripard <[email protected]>
>>>
>>> The i2s controller supports TDM, for up to 8 slots. Let's support the TDM
>>> API.
>>>
>>> Signed-off-by: Maxime Ripard <[email protected]>
>>> ---
>>> sound/soc/sunxi/sun4i-i2s.c | 40 ++++++++++++++++++++++++++++++++------
>>> 1 file changed, 34 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
>>> index 0dac09814b65..4f76daeaaed7 100644
>>> --- a/sound/soc/sunxi/sun4i-i2s.c
>>> +++ b/sound/soc/sunxi/sun4i-i2s.c
>>> @@ -168,6 +168,8 @@ struct sun4i_i2s {
>>> struct reset_control *rst;
>>> unsigned int mclk_freq;
>>> + unsigned int slots;
>>> + unsigned int slot_width;
>>> struct snd_dmaengine_dai_dma_data capture_dma_data;
>>> struct snd_dmaengine_dai_dma_data playback_dma_data;
>>> @@ -287,7 +289,7 @@ static bool sun4i_i2s_oversample_is_valid(unsigned int oversample)
>>> static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
>>> unsigned int rate,
>>> - unsigned int channels,
>>> + unsigned int slots,
>>> unsigned int word_size)
>>> {
>>> struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>> @@ -335,7 +337,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
>>> bclk_parent_rate = i2s->variant->get_bclk_parent_rate(i2s);
>>> bclk_div = sun4i_i2s_get_bclk_div(i2s, bclk_parent_rate,
>>> - rate, channels, word_size);
>>> + rate, slots, word_size);
>>> if (bclk_div < 0) {
>>> dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div);
>>> return -EINVAL;
>>> @@ -419,6 +421,10 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
>>> const struct snd_pcm_hw_params *params)
>>> {
>>> unsigned int channels = params_channels(params);
>>> + unsigned int slots = channels;
>>> +
>>> + if (i2s->slots)
>>> + slots = i2s->slots;
>>> /* Map the channels for playback and capture */
>>> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
>>> @@ -428,7 +434,6 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
>>> regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
>>> SUN4I_I2S_CHAN_SEL_MASK,
>>> SUN4I_I2S_CHAN_SEL(channels));
>>> -
>>> regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
>>> SUN4I_I2S_CHAN_SEL_MASK,
>>> SUN4I_I2S_CHAN_SEL(channels));
>>> @@ -452,10 +457,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
>>> struct snd_soc_dai *dai)
>>> {
>>> struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>> + unsigned int word_size = params_width(params);
>>> unsigned int channels = params_channels(params);
>>> + unsigned int slots = channels;
>>> int ret, sr, wss;
>>> u32 width;
>>> + if (i2s->slots)
>>> + slots = i2s->slots;
>>> +
>>> + if (i2s->slot_width)
>>> + word_size = i2s->slot_width;
>>> +
>>> ret = i2s->variant->set_chan_cfg(i2s, params);
>>> if (ret < 0) {
>>> dev_err(dai->dev, "Invalid channel configuration\n");
>>> @@ -477,15 +490,14 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
>>> if (sr < 0)
>>> return -EINVAL;
>>> - wss = i2s->variant->get_wss(i2s, params_width(params));
>>> + wss = i2s->variant->get_wss(i2s, word_size);
>>> if (wss < 0)
>>> return -EINVAL;
>>> regmap_field_write(i2s->field_fmt_wss, wss);
>>> regmap_field_write(i2s->field_fmt_sr, sr);
>>> - return sun4i_i2s_set_clk_rate(dai, params_rate(params),
>>> - channels, params_width(params));
>>> + return sun4i_i2s_set_clk_rate(dai, params_rate(params), slots, word_size);
>>> }
>>> static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>>> @@ -785,10 +797,26 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
>>> return 0;
>>> }
>>> +static int sun4i_i2s_set_tdm_slot(struct snd_soc_dai *dai,
>>> + unsigned int tx_mask, unsigned int rx_mask,
>>> + int slots, int slot_width)
>>> +{
>>> + struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>> +
>>> + if (slots > 8)
>>> + return -EINVAL;
>>> +
>>> + i2s->slots = slots;
>>> + i2s->slot_width = slot_width;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
>>> .hw_params = sun4i_i2s_hw_params,
>>> .set_fmt = sun4i_i2s_set_fmt,
>>> .set_sysclk = sun4i_i2s_set_sysclk,
>>> + .set_tdm_slot = sun4i_i2s_set_tdm_slot,
>>> .trigger = sun4i_i2s_trigger,
>>> };
>> it seems like you forgot to implement sun4i_i2s_dai_ops.set_bclk_ratio
>> because, as I far as I understand, it should alter tdm slots functionality
>> indirectly.
> As far as I can see, while this indeed changes a few things on the TDM
> setup, it's optional, orthogonal and it has a single user in the tree
> (some intel platform card).
>
> So I'd say that if someone ever needs it, we can have it, but it's not
> a blocker.

"orthogonal" meaning that one can achieve the same effect with using
"sun4i_i2s_set_tdm_slot" instead of "set_bclk_ratio" ?

For example, for WM8731 codec in "non-USB" master mode should generate
BCLK = 64 * FS, and I had to implement "set_bclk_ratio" in order to
setup it. Note, that this is 100% mandatory condition for the code to
operate in this mode.

Did you mean that now the correct way would be using
"sun4i_i2s_set_tdm_slot" instead ?

Thank you

>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

2019-08-27 08:21:50

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 00/21] ASoC: sun4i-i2s: Number of fixes and TDM Support

Hi everyone,

On Tue, Aug 20, 2019 at 3:25 AM Maxime Ripard <[email protected]> wrote:
>
> From: Maxime Ripard <[email protected]>
>
> Hi,
>
> This series aims at fixing a number of issues in the current i2s driver,
> mostly related to the i2s master support and the A83t support. It also uses
> that occasion to cleanup a few things and simplify the driver. Finally, it
> builds on those fixes and cleanups to introduce TDM and DSP formats support.
>
> Let me know what you think,
> Maxime
>
> Marcus Cooper (1):
> ASoC: sun4i-i2s: Fix the MCLK and BCLK dividers on newer SoCs
>
> Maxime Ripard (20):
> ASoC: sun4i-i2s: Register regmap and PCM before our component
> ASoC: sun4i-i2s: Switch to devm for PCM register
> ASoC: sun4i-i2s: Replace call to params_channels by local variable
> ASoC: sun4i-i2s: Move the channel configuration to a callback
> ASoC: sun4i-i2s: Move the format configuration to a callback
> ASoC: sun4i-i2s: Rework MCLK divider calculation
> ASoC: sun4i-i2s: Don't use the oversample to calculate BCLK
> ASoC: sun4i-i2s: Use module clock as BCLK parent on newer SoCs
> ASoC: sun4i-i2s: RX and TX counter registers are swapped
> ASoC: sun4i-i2s: Use the actual format width instead of an hardcoded one
> ASoC: sun4i-i2s: Fix LRCK and BCLK polarity offsets on newer SoCs
> ASoC: sun4i-i2s: Fix the LRCK polarity
> ASoC: sun4i-i2s: Fix WSS and SR fields for the A83t
> ASoC: sun4i-i2s: Fix MCLK Enable bit offset on A83t
> ASoC: sun4i-i2s: Fix the LRCK period on A83t
> ASoC: sun4i-i2s: Remove duplicated quirks structure

Unfortunately the patches that "fix" support on the A83T actually break it.
The confusion stems from the user manual not actually documenting the I2S
controller. Instead it documents the TDM controller, which is very similar
or the same as the I2S controller in the H3. The I2S controller that we
actually support in this driver is not the TDM controller, but three other
I2S controllers that are only mentioned in the memory map. Support for this
was done by referencing the BSP kernel, which has separate driver instances
for each controller instance, both I2S and TDM.

Now to remedy this I could send reverts for all the "A83t" patches, and
fixes for all the others that affect the A83t quirks. However the fixes
tags existing in the tree would be wrong and confusing. That might be a
pain for the stable kernel maintainers.

Any suggestions on how to proceed?

Regards
ChenYu

> ASoC: sun4i-i2s: Pass the channels number as an argument
> ASoC: sun4i-i2s: Support more channels
> ASoC: sun4i-i2s: Add support for TDM slots
> ASoC: sun4i-i2s: Add support for DSP formats
>
> sound/soc/sunxi/sun4i-i2s.c | 660 ++++++++++++++++++++-----------------
> 1 file changed, 372 insertions(+), 288 deletions(-)
>
> base-commit: d45331b00ddb179e291766617259261c112db872
> --
> git-series 0.9.1

2019-08-27 09:26:59

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: Applied "ASoC: sun4i-i2s: Fix the MCLK and BCLK dividers on newer SoCs" to the asoc tree

Hi,

On Wed, Aug 21, 2019 at 1:41 AM Mark Brown <[email protected]> wrote:
>
> The patch
>
> ASoC: sun4i-i2s: Fix the MCLK and BCLK dividers on newer SoCs
>
> has been applied to the asoc tree at
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4
>
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
>
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
>
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
>
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
>
> Thanks,
> Mark
>
> From c1d3a921d72bd21f266ca28c15213fbe78160a4b Mon Sep 17 00:00:00 2001
> From: Maxime Ripard <[email protected]>
> Date: Mon, 19 Aug 2019 21:25:16 +0200
> Subject: [PATCH] ASoC: sun4i-i2s: Fix the MCLK and BCLK dividers on newer SoCs
>
> From: Marcus Cooper <[email protected]>

The authorship of this patch looks to be wrong. Maybe it's a tooling issue.
I imagine it might have happened if Maxime created the patches using
`git format-patch` with his @bootlin.com address, then sent them with his
@kernel.org address, and `git send-email` swapped out the "From:" header
and prepended it to the body.

Either way the "From:" line looks odd in the commit log.

ChenYu

> The clock division dividers have changed between the older (A10/A31) and
> newer (H3, A64, etc) SoCs.
>
> While this was addressed through an offset on some SoCs, it was missing
> some dividers as well, so the support wasn't perfect. Let's introduce a
> pointer in the quirk structure for the divider calculation functions to use
> so we can have the proper range now.
>
> Signed-off-by: Marcus Cooper <[email protected]>
> [Maxime: Fix the commit log, use a field in the quirk structure]
> Signed-off-by: Maxime Ripard <[email protected]>
> Link: https://lore.kernel.org/r/0e5b4abf06cd3202354315201c6af44caeb20236.1566242458.git-series.maxime.ripard@bootlin.com
> Signed-off-by: Mark Brown <[email protected]>

2019-08-27 09:38:01

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH 00/21] ASoC: sun4i-i2s: Number of fixes and TDM Support

Hi,

On Tue, Aug 27, 2019 at 04:20:24PM +0800, Chen-Yu Tsai wrote:
> Hi everyone,
>
> On Tue, Aug 20, 2019 at 3:25 AM Maxime Ripard <[email protected]> wrote:
> >
> > From: Maxime Ripard <[email protected]>
> >
> > Hi,
> >
> > This series aims at fixing a number of issues in the current i2s driver,
> > mostly related to the i2s master support and the A83t support. It also uses
> > that occasion to cleanup a few things and simplify the driver. Finally, it
> > builds on those fixes and cleanups to introduce TDM and DSP formats support.
> >
> > Let me know what you think,
> > Maxime
> >
> > Marcus Cooper (1):
> > ASoC: sun4i-i2s: Fix the MCLK and BCLK dividers on newer SoCs
> >
> > Maxime Ripard (20):
> > ASoC: sun4i-i2s: Register regmap and PCM before our component
> > ASoC: sun4i-i2s: Switch to devm for PCM register
> > ASoC: sun4i-i2s: Replace call to params_channels by local variable
> > ASoC: sun4i-i2s: Move the channel configuration to a callback
> > ASoC: sun4i-i2s: Move the format configuration to a callback
> > ASoC: sun4i-i2s: Rework MCLK divider calculation
> > ASoC: sun4i-i2s: Don't use the oversample to calculate BCLK
> > ASoC: sun4i-i2s: Use module clock as BCLK parent on newer SoCs
> > ASoC: sun4i-i2s: RX and TX counter registers are swapped
> > ASoC: sun4i-i2s: Use the actual format width instead of an hardcoded one
> > ASoC: sun4i-i2s: Fix LRCK and BCLK polarity offsets on newer SoCs
> > ASoC: sun4i-i2s: Fix the LRCK polarity
> > ASoC: sun4i-i2s: Fix WSS and SR fields for the A83t
> > ASoC: sun4i-i2s: Fix MCLK Enable bit offset on A83t
> > ASoC: sun4i-i2s: Fix the LRCK period on A83t
> > ASoC: sun4i-i2s: Remove duplicated quirks structure
>
> Unfortunately the patches that "fix" support on the A83T actually break it.
> The confusion stems from the user manual not actually documenting the I2S
> controller. Instead it documents the TDM controller, which is very similar
> or the same as the I2S controller in the H3. The I2S controller that we
> actually support in this driver is not the TDM controller, but three other
> I2S controllers that are only mentioned in the memory map. Support for this
> was done by referencing the BSP kernel, which has separate driver instances
> for each controller instance, both I2S and TDM.
>
> Now to remedy this I could send reverts for all the "A83t" patches, and
> fixes for all the others that affect the A83t quirks. However the fixes
> tags existing in the tree would be wrong and confusing. That might be a
> pain for the stable kernel maintainers.
>
> Any suggestions on how to proceed?

I've just sent two patches to address that (adding a comment in the
process so that hopefully it doesn't happen again).

Let me know if it works, and sorry for the mess :/
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (2.88 kB)
signature.asc (235.00 B)
Download all attachments

2019-08-27 10:49:09

by Mark Brown

[permalink] [raw]

Subject: Re: Applied "ASoC: sun4i-i2s: Fix the MCLK and BCLK dividers on newer SoCs" to the asoc tree

On Tue, Aug 27, 2019 at 05:25:21PM +0800, Chen-Yu Tsai wrote:

> > From: Marcus Cooper <[email protected]>

> The authorship of this patch looks to be wrong. Maybe it's a tooling issue.
> I imagine it might have happened if Maxime created the patches using
> `git format-patch` with his @bootlin.com address, then sent them with his
> @kernel.org address, and `git send-email` swapped out the "From:" header
> and prepended it to the body.

No idea. In any case it's kind of hard to fix at this point
since there's a merge in the way. This is why I like using
branches for things but Linus doesn't :/

> Either way the "From:" line looks odd in the commit log.

This is what git format-patch does when sending stuff via e-mail
if the author is different from the sender, it uses that to
override things.

Attachments:

(No filename) (828.00 B)
signature.asc (499.00 B)
Download all attachments