[v4,6/9] ASoC: sun4i-i2s: Add multi-lane functionality
diff mbox series

Message ID 20190603174735.21002-7-codekipper@gmail.com
State New
Headers show
Series
  • ASoC: sun4i-i2s: Updates to the driver
Related show

Commit Message

Code Kipper June 3, 2019, 5:47 p.m. UTC
From: Marcus Cooper <codekipper@gmail.com>

The i2s block supports multi-lane i2s output however this functionality
is only possible in earlier SoCs where the pins are exposed and for
the i2s block used for HDMI audio on the later SoCs.

To enable this functionality, an optional property has been added to
the bindings.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
---
 sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

Comments

Maxime Ripard June 4, 2019, 7:58 a.m. UTC | #1
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
> From: Marcus Cooper <codekipper@gmail.com>
>
> The i2s block supports multi-lane i2s output however this functionality
> is only possible in earlier SoCs where the pins are exposed and for
> the i2s block used for HDMI audio on the later SoCs.
>
> To enable this functionality, an optional property has been added to
> the bindings.
>
> Signed-off-by: Marcus Cooper <codekipper@gmail.com>

I'd like to have Mark's input on this, but I'm really worried about
the interaction with the proper TDM support.

Our fundamental issue is that the controller can have up to 8
channels, but either on 4 lines (instead of 1), or 8 channels on 1
(like proper TDM) (or any combination between the two, but that should
be pretty rare).

You're trying to do the first one, and I'm trying to do the second one.

There's a number of assumptions later on that will break the TDM case,
see below for examples

> ---
>  sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index bca73b3c0d74..75217fb52bfa 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -23,7 +23,7 @@
>
>  #define SUN4I_I2S_CTRL_REG		0x00
>  #define SUN4I_I2S_CTRL_SDO_EN_MASK		GENMASK(11, 8)
> -#define SUN4I_I2S_CTRL_SDO_EN(sdo)			BIT(8 + (sdo))
> +#define SUN4I_I2S_CTRL_SDO_EN(lines)		(((1 << lines) - 1) << 8)
>  #define SUN4I_I2S_CTRL_MODE_MASK		BIT(5)
>  #define SUN4I_I2S_CTRL_MODE_SLAVE			(1 << 5)
>  #define SUN4I_I2S_CTRL_MODE_MASTER			(0 << 5)
> @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
>  	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>  	int sr, wss, channels;
>  	u32 width;
> +	int lines;
>
>  	channels = params_channels(params);
> -	if (channels != 2) {
> +	if ((channels > dai->driver->playback.channels_max) ||
> +		(channels < dai->driver->playback.channels_min)) {
>  		dev_err(dai->dev, "Unsupported number of channels: %d\n",
>  			channels);
>  		return -EINVAL;
>  	}
>
> +	lines = (channels + 1) / 2;
> +
> +	/* Enable the required output lines */
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> +			   SUN4I_I2S_CTRL_SDO_EN_MASK,
> +			   SUN4I_I2S_CTRL_SDO_EN(lines));
> +

This has the assumption that each line will have 2 channels, which is wrong.

>  	if (i2s->variant->is_h3_i2s_based) {
>  		regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
>  				   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
>  	}
>
>  	/* Map the channels for playback and capture */
> -	regmap_field_write(i2s->field_txchanmap, 0x76543210);
>  	regmap_field_write(i2s->field_rxchanmap, 0x00003210);
> +	regmap_field_write(i2s->field_txchanmap, 0x10);
> +	if (i2s->variant->is_h3_i2s_based) {
> +		if (channels > 2)
> +			regmap_write(i2s->regmap,
> +				     SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
> +		if (channels > 4)
> +			regmap_write(i2s->regmap,
> +				     SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
> +		if (channels > 6)
> +			regmap_write(i2s->regmap,
> +				     SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
> +	}

And this creates a mapping matching that.

>  	/* Configure the channels */
>  	regmap_field_write(i2s->field_txchansel,
> @@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,
>  static int sun4i_i2s_probe(struct platform_device *pdev)
>  {
>  	struct sun4i_i2s *i2s;
> +	struct snd_soc_dai_driver *soc_dai;
>  	struct resource *res;
>  	void __iomem *regs;
> -	int irq, ret;
> +	int irq, ret, val;
>
>  	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
>  	if (!i2s)
> @@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
>  	i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
>  	i2s->capture_dma_data.maxburst = 8;
>
> +	soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
> +			       sizeof(*soc_dai), GFP_KERNEL);
> +	if (!soc_dai) {
> +		ret = -ENOMEM;
> +		goto err_pm_disable;
> +	}
> +
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				  "allwinner,playback-channels", &val)) {
> +		if (val >= 2 && val <= 8)
> +			soc_dai->playback.channels_max = val;
> +	}
> +

I'm not quite sure how this works.

of_property_read_u32 will return 0, so you will enter in the
condition. But what happens if the property is missing?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Code Kipper June 4, 2019, 8:43 a.m. UTC | #2
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
> > From: Marcus Cooper <codekipper@gmail.com>
> >
> > The i2s block supports multi-lane i2s output however this functionality
> > is only possible in earlier SoCs where the pins are exposed and for
> > the i2s block used for HDMI audio on the later SoCs.
> >
> > To enable this functionality, an optional property has been added to
> > the bindings.
> >
> > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
>
> I'd like to have Mark's input on this, but I'm really worried about
> the interaction with the proper TDM support.
>
> Our fundamental issue is that the controller can have up to 8
> channels, but either on 4 lines (instead of 1), or 8 channels on 1
> (like proper TDM) (or any combination between the two, but that should
> be pretty rare).

I understand...maybe the TDM needs to be extended to support this to consider
channel mapping and multiple transfer lines. I was thinking about the later when
someone was requesting support on IIRC a while ago, I thought masking might
of been a solution. These can wait as the only consumer at the moment is
LibreELEC and we can patch it there.
Do you have any ideas Master?
CK
>
> You're trying to do the first one, and I'm trying to do the second one.
>
> There's a number of assumptions later on that will break the TDM case,
> see below for examples
>
> > ---
> >  sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index bca73b3c0d74..75217fb52bfa 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -23,7 +23,7 @@
> >
> >  #define SUN4I_I2S_CTRL_REG           0x00
> >  #define SUN4I_I2S_CTRL_SDO_EN_MASK           GENMASK(11, 8)
> > -#define SUN4I_I2S_CTRL_SDO_EN(sdo)                   BIT(8 + (sdo))
> > +#define SUN4I_I2S_CTRL_SDO_EN(lines)         (((1 << lines) - 1) << 8)
> >  #define SUN4I_I2S_CTRL_MODE_MASK             BIT(5)
> >  #define SUN4I_I2S_CTRL_MODE_SLAVE                    (1 << 5)
> >  #define SUN4I_I2S_CTRL_MODE_MASTER                   (0 << 5)
> > @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> >       struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> >       int sr, wss, channels;
> >       u32 width;
> > +     int lines;
> >
> >       channels = params_channels(params);
> > -     if (channels != 2) {
> > +     if ((channels > dai->driver->playback.channels_max) ||
> > +             (channels < dai->driver->playback.channels_min)) {
> >               dev_err(dai->dev, "Unsupported number of channels: %d\n",
> >                       channels);
> >               return -EINVAL;
> >       }
> >
> > +     lines = (channels + 1) / 2;
> > +
> > +     /* Enable the required output lines */
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > +                        SUN4I_I2S_CTRL_SDO_EN_MASK,
> > +                        SUN4I_I2S_CTRL_SDO_EN(lines));
> > +
>
> This has the assumption that each line will have 2 channels, which is wrong.
>
> >       if (i2s->variant->is_h3_i2s_based) {
> >               regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> >                                  SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> > @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> >       }
> >
> >       /* Map the channels for playback and capture */
> > -     regmap_field_write(i2s->field_txchanmap, 0x76543210);
> >       regmap_field_write(i2s->field_rxchanmap, 0x00003210);
> > +     regmap_field_write(i2s->field_txchanmap, 0x10);
> > +     if (i2s->variant->is_h3_i2s_based) {
> > +             if (channels > 2)
> > +                     regmap_write(i2s->regmap,
> > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
> > +             if (channels > 4)
> > +                     regmap_write(i2s->regmap,
> > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
> > +             if (channels > 6)
> > +                     regmap_write(i2s->regmap,
> > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
> > +     }
>
> And this creates a mapping matching that.
>
> >       /* Configure the channels */
> >       regmap_field_write(i2s->field_txchansel,
> > @@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,
> >  static int sun4i_i2s_probe(struct platform_device *pdev)
> >  {
> >       struct sun4i_i2s *i2s;
> > +     struct snd_soc_dai_driver *soc_dai;
> >       struct resource *res;
> >       void __iomem *regs;
> > -     int irq, ret;
> > +     int irq, ret, val;
> >
> >       i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> >       if (!i2s)
> > @@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
> >       i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
> >       i2s->capture_dma_data.maxburst = 8;
> >
> > +     soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
> > +                            sizeof(*soc_dai), GFP_KERNEL);
> > +     if (!soc_dai) {
> > +             ret = -ENOMEM;
> > +             goto err_pm_disable;
> > +     }
> > +
> > +     if (!of_property_read_u32(pdev->dev.of_node,
> > +                               "allwinner,playback-channels", &val)) {
> > +             if (val >= 2 && val <= 8)
> > +                     soc_dai->playback.channels_max = val;
> > +     }
> > +
>
> I'm not quite sure how this works.
>
> of_property_read_u32 will return 0, so you will enter in the
> condition. But what happens if the property is missing?
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Christopher Obbard June 4, 2019, 9:02 a.m. UTC | #3
On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
>
> On Tue, 4 Jun 2019 at 09:58, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
> > > From: Marcus Cooper <codekipper@gmail.com>
> > >
> > > The i2s block supports multi-lane i2s output however this functionality
> > > is only possible in earlier SoCs where the pins are exposed and for
> > > the i2s block used for HDMI audio on the later SoCs.
> > >
> > > To enable this functionality, an optional property has been added to
> > > the bindings.
> > >
> > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> >
> > I'd like to have Mark's input on this, but I'm really worried about
> > the interaction with the proper TDM support.
> >
> > Our fundamental issue is that the controller can have up to 8
> > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > (like proper TDM) (or any combination between the two, but that should
> > be pretty rare).
>
> I understand...maybe the TDM needs to be extended to support this to consider
> channel mapping and multiple transfer lines. I was thinking about the later when
> someone was requesting support on IIRC a while ago, I thought masking might
> of been a solution. These can wait as the only consumer at the moment is
> LibreELEC and we can patch it there.

Hi Marcus,

FWIW, the TI McASP driver has support for TDM & (i think?) multiple
transfer lines which are called serializers.
Maybe this can help with inspiration?
see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/ti/davinci-mcasp.c
sample DTS:

&mcasp0 {
    #sound-dai-cells = <0>;
    status = "okay";
    pinctrl-names = "default";
    pinctrl-0 = <&mcasp0_pins>;

    op-mode = <0>;
    tdm-slots = <8>;
    serial-dir = <
        2 0 1 0
        0 0 0 0
        0 0 0 0
        0 0 0 0
    >;
    tx-num-evt = <1>;
    rx-num-evt = <1>;
};


Cheers!

> Do you have any ideas Master?
> CK
> >
> > You're trying to do the first one, and I'm trying to do the second one.
> >
> > There's a number of assumptions later on that will break the TDM case,
> > see below for examples
> >
> > > ---
> > >  sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 39 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > index bca73b3c0d74..75217fb52bfa 100644
> > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > @@ -23,7 +23,7 @@
> > >
> > >  #define SUN4I_I2S_CTRL_REG           0x00
> > >  #define SUN4I_I2S_CTRL_SDO_EN_MASK           GENMASK(11, 8)
> > > -#define SUN4I_I2S_CTRL_SDO_EN(sdo)                   BIT(8 + (sdo))
> > > +#define SUN4I_I2S_CTRL_SDO_EN(lines)         (((1 << lines) - 1) << 8)
> > >  #define SUN4I_I2S_CTRL_MODE_MASK             BIT(5)
> > >  #define SUN4I_I2S_CTRL_MODE_SLAVE                    (1 << 5)
> > >  #define SUN4I_I2S_CTRL_MODE_MASTER                   (0 << 5)
> > > @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > >       struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > >       int sr, wss, channels;
> > >       u32 width;
> > > +     int lines;
> > >
> > >       channels = params_channels(params);
> > > -     if (channels != 2) {
> > > +     if ((channels > dai->driver->playback.channels_max) ||
> > > +             (channels < dai->driver->playback.channels_min)) {
> > >               dev_err(dai->dev, "Unsupported number of channels: %d\n",
> > >                       channels);
> > >               return -EINVAL;
> > >       }
> > >
> > > +     lines = (channels + 1) / 2;
> > > +
> > > +     /* Enable the required output lines */
> > > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > +                        SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > +                        SUN4I_I2S_CTRL_SDO_EN(lines));
> > > +
> >
> > This has the assumption that each line will have 2 channels, which is wrong.
> >
> > >       if (i2s->variant->is_h3_i2s_based) {
> > >               regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> > >                                  SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> > > @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > >       }
> > >
> > >       /* Map the channels for playback and capture */
> > > -     regmap_field_write(i2s->field_txchanmap, 0x76543210);
> > >       regmap_field_write(i2s->field_rxchanmap, 0x00003210);
> > > +     regmap_field_write(i2s->field_txchanmap, 0x10);
> > > +     if (i2s->variant->is_h3_i2s_based) {
> > > +             if (channels > 2)
> > > +                     regmap_write(i2s->regmap,
> > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
> > > +             if (channels > 4)
> > > +                     regmap_write(i2s->regmap,
> > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
> > > +             if (channels > 6)
> > > +                     regmap_write(i2s->regmap,
> > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
> > > +     }
> >
> > And this creates a mapping matching that.
> >
> > >       /* Configure the channels */
> > >       regmap_field_write(i2s->field_txchansel,
> > > @@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,
> > >  static int sun4i_i2s_probe(struct platform_device *pdev)
> > >  {
> > >       struct sun4i_i2s *i2s;
> > > +     struct snd_soc_dai_driver *soc_dai;
> > >       struct resource *res;
> > >       void __iomem *regs;
> > > -     int irq, ret;
> > > +     int irq, ret, val;
> > >
> > >       i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> > >       if (!i2s)
> > > @@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
> > >       i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
> > >       i2s->capture_dma_data.maxburst = 8;
> > >
> > > +     soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
> > > +                            sizeof(*soc_dai), GFP_KERNEL);
> > > +     if (!soc_dai) {
> > > +             ret = -ENOMEM;
> > > +             goto err_pm_disable;
> > > +     }
> > > +
> > > +     if (!of_property_read_u32(pdev->dev.of_node,
> > > +                               "allwinner,playback-channels", &val)) {
> > > +             if (val >= 2 && val <= 8)
> > > +                     soc_dai->playback.channels_max = val;
> > > +     }
> > > +
> >
> > I'm not quite sure how this works.
> >
> > of_property_read_u32 will return 0, so you will enter in the
> > condition. But what happens if the property is missing?
> >
> > Maxime
> >
> > --
> > Maxime Ripard, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAEKpxB%3DRdYF9eEvAJ%2BR7sT6OtdtBWjhMM1am%2BEhaN%3D9ZO9Gd2A%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
Code Kipper June 4, 2019, 9:38 a.m. UTC | #4
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard <chris@64studio.com> wrote:
>
> On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
> >
> > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
> > > > From: Marcus Cooper <codekipper@gmail.com>
> > > >
> > > > The i2s block supports multi-lane i2s output however this functionality
> > > > is only possible in earlier SoCs where the pins are exposed and for
> > > > the i2s block used for HDMI audio on the later SoCs.
> > > >
> > > > To enable this functionality, an optional property has been added to
> > > > the bindings.
> > > >
> > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > >
> > > I'd like to have Mark's input on this, but I'm really worried about
> > > the interaction with the proper TDM support.
> > >
> > > Our fundamental issue is that the controller can have up to 8
> > > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > > (like proper TDM) (or any combination between the two, but that should
> > > be pretty rare).
> >
> > I understand...maybe the TDM needs to be extended to support this to consider
> > channel mapping and multiple transfer lines. I was thinking about the later when
> > someone was requesting support on IIRC a while ago, I thought masking might
> > of been a solution. These can wait as the only consumer at the moment is
> > LibreELEC and we can patch it there.
>
> Hi Marcus,
>
> FWIW, the TI McASP driver has support for TDM & (i think?) multiple
> transfer lines which are called serializers.
> Maybe this can help with inspiration?
> see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/ti/davinci-mcasp.c
> sample DTS:
>
> &mcasp0 {
>     #sound-dai-cells = <0>;
>     status = "okay";
>     pinctrl-names = "default";
>     pinctrl-0 = <&mcasp0_pins>;
>
>     op-mode = <0>;
>     tdm-slots = <8>;
>     serial-dir = <
>         2 0 1 0
>         0 0 0 0
>         0 0 0 0
>         0 0 0 0
>     >;
>     tx-num-evt = <1>;
>     rx-num-evt = <1>;
> };
>
>
> Cheers!

Thanks, this looks good.
CK
>
> > Do you have any ideas Master?
> > CK
> > >
> > > You're trying to do the first one, and I'm trying to do the second one.
> > >
> > > There's a number of assumptions later on that will break the TDM case,
> > > see below for examples
> > >
> > > > ---
> > > >  sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 39 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > > index bca73b3c0d74..75217fb52bfa 100644
> > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > @@ -23,7 +23,7 @@
> > > >
> > > >  #define SUN4I_I2S_CTRL_REG           0x00
> > > >  #define SUN4I_I2S_CTRL_SDO_EN_MASK           GENMASK(11, 8)
> > > > -#define SUN4I_I2S_CTRL_SDO_EN(sdo)                   BIT(8 + (sdo))
> > > > +#define SUN4I_I2S_CTRL_SDO_EN(lines)         (((1 << lines) - 1) << 8)
> > > >  #define SUN4I_I2S_CTRL_MODE_MASK             BIT(5)
> > > >  #define SUN4I_I2S_CTRL_MODE_SLAVE                    (1 << 5)
> > > >  #define SUN4I_I2S_CTRL_MODE_MASTER                   (0 << 5)
> > > > @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > > >       struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > >       int sr, wss, channels;
> > > >       u32 width;
> > > > +     int lines;
> > > >
> > > >       channels = params_channels(params);
> > > > -     if (channels != 2) {
> > > > +     if ((channels > dai->driver->playback.channels_max) ||
> > > > +             (channels < dai->driver->playback.channels_min)) {
> > > >               dev_err(dai->dev, "Unsupported number of channels: %d\n",
> > > >                       channels);
> > > >               return -EINVAL;
> > > >       }
> > > >
> > > > +     lines = (channels + 1) / 2;
> > > > +
> > > > +     /* Enable the required output lines */
> > > > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > +                        SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > +                        SUN4I_I2S_CTRL_SDO_EN(lines));
> > > > +
> > >
> > > This has the assumption that each line will have 2 channels, which is wrong.
> > >
> > > >       if (i2s->variant->is_h3_i2s_based) {
> > > >               regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> > > >                                  SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> > > > @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > > >       }
> > > >
> > > >       /* Map the channels for playback and capture */
> > > > -     regmap_field_write(i2s->field_txchanmap, 0x76543210);
> > > >       regmap_field_write(i2s->field_rxchanmap, 0x00003210);
> > > > +     regmap_field_write(i2s->field_txchanmap, 0x10);
> > > > +     if (i2s->variant->is_h3_i2s_based) {
> > > > +             if (channels > 2)
> > > > +                     regmap_write(i2s->regmap,
> > > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
> > > > +             if (channels > 4)
> > > > +                     regmap_write(i2s->regmap,
> > > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
> > > > +             if (channels > 6)
> > > > +                     regmap_write(i2s->regmap,
> > > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
> > > > +     }
> > >
> > > And this creates a mapping matching that.
> > >
> > > >       /* Configure the channels */
> > > >       regmap_field_write(i2s->field_txchansel,
> > > > @@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,
> > > >  static int sun4i_i2s_probe(struct platform_device *pdev)
> > > >  {
> > > >       struct sun4i_i2s *i2s;
> > > > +     struct snd_soc_dai_driver *soc_dai;
> > > >       struct resource *res;
> > > >       void __iomem *regs;
> > > > -     int irq, ret;
> > > > +     int irq, ret, val;
> > > >
> > > >       i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> > > >       if (!i2s)
> > > > @@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
> > > >       i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
> > > >       i2s->capture_dma_data.maxburst = 8;
> > > >
> > > > +     soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
> > > > +                            sizeof(*soc_dai), GFP_KERNEL);
> > > > +     if (!soc_dai) {
> > > > +             ret = -ENOMEM;
> > > > +             goto err_pm_disable;
> > > > +     }
> > > > +
> > > > +     if (!of_property_read_u32(pdev->dev.of_node,
> > > > +                               "allwinner,playback-channels", &val)) {
> > > > +             if (val >= 2 && val <= 8)
> > > > +                     soc_dai->playback.channels_max = val;
> > > > +     }
> > > > +
> > >
> > > I'm not quite sure how this works.
> > >
> > > of_property_read_u32 will return 0, so you will enter in the
> > > condition. But what happens if the property is missing?
> > >
> > > Maxime
> > >
> > > --
> > > Maxime Ripard, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
> >
> > --
> > You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAEKpxB%3DRdYF9eEvAJ%2BR7sT6OtdtBWjhMM1am%2BEhaN%3D9ZO9Gd2A%40mail.gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
Jernej Škrabec July 30, 2019, 5:57 p.m. UTC | #5
Hi!

Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
> On Tue, 4 Jun 2019 at 11:02, Christopher Obbard <chris@64studio.com> wrote:
> > On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
> > > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard <maxime.ripard@bootlin.com> 
wrote:
> > > > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
> > > > > From: Marcus Cooper <codekipper@gmail.com>
> > > > > 
> > > > > The i2s block supports multi-lane i2s output however this
> > > > > functionality
> > > > > is only possible in earlier SoCs where the pins are exposed and for
> > > > > the i2s block used for HDMI audio on the later SoCs.
> > > > > 
> > > > > To enable this functionality, an optional property has been added to
> > > > > the bindings.
> > > > > 
> > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > > > 
> > > > I'd like to have Mark's input on this, but I'm really worried about
> > > > the interaction with the proper TDM support.
> > > > 
> > > > Our fundamental issue is that the controller can have up to 8
> > > > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > > > (like proper TDM) (or any combination between the two, but that should
> > > > be pretty rare).
> > > 
> > > I understand...maybe the TDM needs to be extended to support this to
> > > consider channel mapping and multiple transfer lines. I was thinking
> > > about the later when someone was requesting support on IIRC a while
> > > ago, I thought masking might of been a solution. These can wait as the
> > > only consumer at the moment is LibreELEC and we can patch it there.
> > 
> > Hi Marcus,
> > 
> > FWIW, the TI McASP driver has support for TDM & (i think?) multiple
> > transfer lines which are called serializers.
> > Maybe this can help with inspiration?
> > see
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/s
> > ound/soc/ti/davinci-mcasp.c sample DTS:
> > 
> > &mcasp0 {
> > 
> >     #sound-dai-cells = <0>;
> >     status = "okay";
> >     pinctrl-names = "default";
> >     pinctrl-0 = <&mcasp0_pins>;
> >     
> >     op-mode = <0>;
> >     tdm-slots = <8>;
> >     serial-dir = <
> >     
> >         2 0 1 0
> >         0 0 0 0
> >         0 0 0 0
> >         0 0 0 0
> >     >
> >     >;
> >     
> >     tx-num-evt = <1>;
> >     rx-num-evt = <1>;
> > 
> > };
> > 
> > 
> > Cheers!
> 
> Thanks, this looks good.

I would really like to see this issue resolved, because HDMI audio support in 
mainline Linux for those SoCs is long overdue.

However, there is a possibility to still add HDMI audio suport (stereo only) 
even if this issue is not completely solved. If we agree that configuration of 
channels would be solved with additional property as Christopher suggested, 
support for >2 channels can be left for a later time when support for that 
property would be implemented. Currently, stereo HDMI audio support can be 
added with a few patches.

I think all I2S cores are really the same, no matter if internally connected 
to HDMI controller or routed to pins, so it would make sense to use same 
compatible for all of them. It's just that those I2S cores which are routed to 
pins can use only one lane and >2 channels can be used only in TDM mode of 
operation, if I understand this correctly.

New property would have to be optional, so it's omission would result in same 
channel configuration as it is already present, to preserve compatibility with 
old device trees. And this mode is already sufficient for stereo HDMI audio 
support.

Side note: HDMI audio with current sun4i-i2s driver has a delay (about a 
second), supposedly because DW HDMI controller automatically generates CTS 
value based on I2S clock (auto CTS value generation is enabled per DesignWare 
recomendation for DW HDMI I2S interface). I2S driver from BSP Linux solves 
that by having I2S clock output enabled all the time. Should this be flagged 
with some additional property in DT?

Best regards,
Jernej

> CK
> 
> > > Do you have any ideas Master?
> > > CK
> > > 
> > > > You're trying to do the first one, and I'm trying to do the second
> > > > one.
> > > > 
> > > > There's a number of assumptions later on that will break the TDM case,
> > > > see below for examples
> > > > 
> > > > > ---
> > > > > 
> > > > >  sound/soc/sunxi/sun4i-i2s.c | 44
> > > > >  ++++++++++++++++++++++++++++++++-----
> > > > >  1 file changed, 39 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > index bca73b3c0d74..75217fb52bfa 100644
> > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > @@ -23,7 +23,7 @@
> > > > > 
> > > > >  #define SUN4I_I2S_CTRL_REG           0x00
> > > > >  #define SUN4I_I2S_CTRL_SDO_EN_MASK           GENMASK(11, 8)
> > > > > 
> > > > > -#define SUN4I_I2S_CTRL_SDO_EN(sdo)                   BIT(8 + (sdo))
> > > > > +#define SUN4I_I2S_CTRL_SDO_EN(lines)         (((1 << lines) - 1) <<
> > > > > 8)
> > > > > 
> > > > >  #define SUN4I_I2S_CTRL_MODE_MASK             BIT(5)
> > > > >  #define SUN4I_I2S_CTRL_MODE_SLAVE                    (1 << 5)
> > > > >  #define SUN4I_I2S_CTRL_MODE_MASTER                   (0 << 5)
> > > > > 
> > > > > @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct
> > > > > snd_pcm_substream *substream,> > > > 
> > > > >       struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > >       int sr, wss, channels;
> > > > >       u32 width;
> > > > > 
> > > > > +     int lines;
> > > > > 
> > > > >       channels = params_channels(params);
> > > > > 
> > > > > -     if (channels != 2) {
> > > > > +     if ((channels > dai->driver->playback.channels_max) ||
> > > > > +             (channels < dai->driver->playback.channels_min)) {
> > > > > 
> > > > >               dev_err(dai->dev, "Unsupported number of channels:
> > > > >               %d\n",
> > > > >               
> > > > >                       channels);
> > > > >               
> > > > >               return -EINVAL;
> > > > >       
> > > > >       }
> > > > > 
> > > > > +     lines = (channels + 1) / 2;
> > > > > +
> > > > > +     /* Enable the required output lines */
> > > > > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > +                        SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > +                        SUN4I_I2S_CTRL_SDO_EN(lines));
> > > > > +
> > > > 
> > > > This has the assumption that each line will have 2 channels, which is
> > > > wrong.> > > 
> > > > >       if (i2s->variant->is_h3_i2s_based) {
> > > > >       
> > > > >               regmap_update_bits(i2s->regmap,
> > > > >               SUN8I_I2S_CHAN_CFG_REG,
> > > > >               
> > > > >                                  SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK
> > > > >                                  ,
> > > > > 
> > > > > @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct
> > > > > snd_pcm_substream *substream,> > > > 
> > > > >       }
> > > > >       
> > > > >       /* Map the channels for playback and capture */
> > > > > 
> > > > > -     regmap_field_write(i2s->field_txchanmap, 0x76543210);
> > > > > 
> > > > >       regmap_field_write(i2s->field_rxchanmap, 0x00003210);
> > > > > 
> > > > > +     regmap_field_write(i2s->field_txchanmap, 0x10);
> > > > > +     if (i2s->variant->is_h3_i2s_based) {
> > > > > +             if (channels > 2)
> > > > > +                     regmap_write(i2s->regmap,
> > > > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+4,
> > > > > 0x32);
> > > > > +             if (channels > 4)
> > > > > +                     regmap_write(i2s->regmap,
> > > > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+8,
> > > > > 0x54);
> > > > > +             if (channels > 6)
> > > > > +                     regmap_write(i2s->regmap,
> > > > > +                                  SUN8I_I2S_TX_CHAN_MAP_REG+12,
> > > > > 0x76);
> > > > > +     }
> > > > 
> > > > And this creates a mapping matching that.
> > > > 
> > > > >       /* Configure the channels */
> > > > >       regmap_field_write(i2s->field_txchansel,
> > > > > 
> > > > > @@ -1057,9 +1077,10 @@ static int
> > > > > sun4i_i2s_init_regmap_fields(struct device *dev,> > > > 
> > > > >  static int sun4i_i2s_probe(struct platform_device *pdev)
> > > > >  {
> > > > >  
> > > > >       struct sun4i_i2s *i2s;
> > > > > 
> > > > > +     struct snd_soc_dai_driver *soc_dai;
> > > > > 
> > > > >       struct resource *res;
> > > > >       void __iomem *regs;
> > > > > 
> > > > > -     int irq, ret;
> > > > > +     int irq, ret, val;
> > > > > 
> > > > >       i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> > > > >       if (!i2s)
> > > > > 
> > > > > @@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct
> > > > > platform_device *pdev)> > > > 
> > > > >       i2s->capture_dma_data.addr = res->start +
> > > > >       SUN4I_I2S_FIFO_RX_REG;
> > > > >       i2s->capture_dma_data.maxburst = 8;
> > > > > 
> > > > > +     soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
> > > > > +                            sizeof(*soc_dai), GFP_KERNEL);
> > > > > +     if (!soc_dai) {
> > > > > +             ret = -ENOMEM;
> > > > > +             goto err_pm_disable;
> > > > > +     }
> > > > > +
> > > > > +     if (!of_property_read_u32(pdev->dev.of_node,
> > > > > +                               "allwinner,playback-channels",
> > > > > &val)) {
> > > > > +             if (val >= 2 && val <= 8)
> > > > > +                     soc_dai->playback.channels_max = val;
> > > > > +     }
> > > > > +
> > > > 
> > > > I'm not quite sure how this works.
> > > > 
> > > > of_property_read_u32 will return 0, so you will enter in the
> > > > condition. But what happens if the property is missing?
> > > > 
> > > > Maxime
> > > > 
> > > > --
> > > > Maxime Ripard, Bootlin
> > > > Embedded Linux and Kernel engineering
> > > > https://bootlin.com
> > > 
> > > --
> > > You received this message because you are subscribed to the Google
> > > Groups "linux-sunxi" group. To unsubscribe from this group and stop
> > > receiving emails from it, send an email to
> > > linux-sunxi+unsubscribe@googlegroups.com. To view this discussion on
> > > the web, visit
> > > https://groups.google.com/d/msgid/linux-sunxi/CAEKpxB%3DRdYF9eEvAJ%2BR7
> > > sT6OtdtBWjhMM1am%2BEhaN%3D9ZO9Gd2A%40mail.gmail.com. For more options,
> > > visit https://groups.google.com/d/optout.
Maxime Ripard July 31, 2019, 12:29 p.m. UTC | #6
On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
> Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
> > On Tue, 4 Jun 2019 at 11:02, Christopher Obbard <chris@64studio.com> wrote:
> > > On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
> > > > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard <maxime.ripard@bootlin.com>
> wrote:
> > > > > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
> > > > > > From: Marcus Cooper <codekipper@gmail.com>
> > > > > >
> > > > > > The i2s block supports multi-lane i2s output however this
> > > > > > functionality
> > > > > > is only possible in earlier SoCs where the pins are exposed and for
> > > > > > the i2s block used for HDMI audio on the later SoCs.
> > > > > >
> > > > > > To enable this functionality, an optional property has been added to
> > > > > > the bindings.
> > > > > >
> > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > > > >
> > > > > I'd like to have Mark's input on this, but I'm really worried about
> > > > > the interaction with the proper TDM support.
> > > > >
> > > > > Our fundamental issue is that the controller can have up to 8
> > > > > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > > > > (like proper TDM) (or any combination between the two, but that should
> > > > > be pretty rare).
> > > >
> > > > I understand...maybe the TDM needs to be extended to support this to
> > > > consider channel mapping and multiple transfer lines. I was thinking
> > > > about the later when someone was requesting support on IIRC a while
> > > > ago, I thought masking might of been a solution. These can wait as the
> > > > only consumer at the moment is LibreELEC and we can patch it there.
> > >
> > > Hi Marcus,
> > >
> > > FWIW, the TI McASP driver has support for TDM & (i think?) multiple
> > > transfer lines which are called serializers.
> > > Maybe this can help with inspiration?
> > > see
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/s
> > > ound/soc/ti/davinci-mcasp.c sample DTS:
> > >
> > > &mcasp0 {
> > >
> > >     #sound-dai-cells = <0>;
> > >     status = "okay";
> > >     pinctrl-names = "default";
> > >     pinctrl-0 = <&mcasp0_pins>;
> > >
> > >     op-mode = <0>;
> > >     tdm-slots = <8>;
> > >     serial-dir = <
> > >
> > >         2 0 1 0
> > >         0 0 0 0
> > >         0 0 0 0
> > >         0 0 0 0
> > >     >
> > >     >;
> > >
> > >     tx-num-evt = <1>;
> > >     rx-num-evt = <1>;
> > >
> > > };
> > >
> > > Cheers!
> >
> > Thanks, this looks good.
>
> I would really like to see this issue resolved, because HDMI audio support in
> mainline Linux for those SoCs is long overdue.
>
> However, there is a possibility to still add HDMI audio suport (stereo only)
> even if this issue is not completely solved. If we agree that configuration of
> channels would be solved with additional property as Christopher suggested,
> support for >2 channels can be left for a later time when support for that
> property would be implemented. Currently, stereo HDMI audio support can be
> added with a few patches.
>
> I think all I2S cores are really the same, no matter if internally connected
> to HDMI controller or routed to pins, so it would make sense to use same
> compatible for all of them. It's just that those I2S cores which are routed to
> pins can use only one lane and >2 channels can be used only in TDM mode of
> operation, if I understand this correctly.
>
> New property would have to be optional, so it's omission would result in same
> channel configuration as it is already present, to preserve compatibility with
> old device trees. And this mode is already sufficient for stereo HDMI audio
> support.

Yeah, it looks like a good plan.

> Side note: HDMI audio with current sun4i-i2s driver has a delay (about a
> second), supposedly because DW HDMI controller automatically generates CTS
> value based on I2S clock (auto CTS value generation is enabled per DesignWare
> recomendation for DW HDMI I2S interface).

Is that a constant delay during the audio playback, or only at startup?

> I2S driver from BSP Linux solves that by having I2S clock output
> enabled all the time. Should this be flagged with some additional
> property in DT?

I'd say that if the codec has that requirement, then it should be
between the codec and the DAI, the DT doesn't really have anything to
do with this.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Jernej Škrabec Aug. 1, 2019, 5:31 a.m. UTC | #7
Dne sreda, 31. julij 2019 ob 14:29:53 CEST je Maxime Ripard napisal(a):
> On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
> > Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
> > > On Tue, 4 Jun 2019 at 11:02, Christopher Obbard <chris@64studio.com> 
wrote:
> > > > On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
> > > > > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard
> > > > > <maxime.ripard@bootlin.com>
> > 
> > wrote:
> > > > > > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com 
wrote:
> > > > > > > From: Marcus Cooper <codekipper@gmail.com>
> > > > > > > 
> > > > > > > The i2s block supports multi-lane i2s output however this
> > > > > > > functionality
> > > > > > > is only possible in earlier SoCs where the pins are exposed and
> > > > > > > for
> > > > > > > the i2s block used for HDMI audio on the later SoCs.
> > > > > > > 
> > > > > > > To enable this functionality, an optional property has been
> > > > > > > added to
> > > > > > > the bindings.
> > > > > > > 
> > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > > > > > 
> > > > > > I'd like to have Mark's input on this, but I'm really worried
> > > > > > about
> > > > > > the interaction with the proper TDM support.
> > > > > > 
> > > > > > Our fundamental issue is that the controller can have up to 8
> > > > > > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > > > > > (like proper TDM) (or any combination between the two, but that
> > > > > > should
> > > > > > be pretty rare).
> > > > > 
> > > > > I understand...maybe the TDM needs to be extended to support this to
> > > > > consider channel mapping and multiple transfer lines. I was thinking
> > > > > about the later when someone was requesting support on IIRC a while
> > > > > ago, I thought masking might of been a solution. These can wait as
> > > > > the
> > > > > only consumer at the moment is LibreELEC and we can patch it there.
> > > > 
> > > > Hi Marcus,
> > > > 
> > > > FWIW, the TI McASP driver has support for TDM & (i think?) multiple
> > > > transfer lines which are called serializers.
> > > > Maybe this can help with inspiration?
> > > > see
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > > > e/s
> > > > ound/soc/ti/davinci-mcasp.c sample DTS:
> > > > 
> > > > &mcasp0 {
> > > > 
> > > >     #sound-dai-cells = <0>;
> > > >     status = "okay";
> > > >     pinctrl-names = "default";
> > > >     pinctrl-0 = <&mcasp0_pins>;
> > > >     
> > > >     op-mode = <0>;
> > > >     tdm-slots = <8>;
> > > >     serial-dir = <
> > > >     
> > > >         2 0 1 0
> > > >         0 0 0 0
> > > >         0 0 0 0
> > > >         0 0 0 0
> > > >     >
> > > >     >;
> > > >     
> > > >     tx-num-evt = <1>;
> > > >     rx-num-evt = <1>;
> > > > 
> > > > };
> > > > 
> > > > Cheers!
> > > 
> > > Thanks, this looks good.
> > 
> > I would really like to see this issue resolved, because HDMI audio support
> > in mainline Linux for those SoCs is long overdue.
> > 
> > However, there is a possibility to still add HDMI audio suport (stereo
> > only) even if this issue is not completely solved. If we agree that
> > configuration of channels would be solved with additional property as
> > Christopher suggested, support for >2 channels can be left for a later
> > time when support for that property would be implemented. Currently,
> > stereo HDMI audio support can be added with a few patches.
> > 
> > I think all I2S cores are really the same, no matter if internally
> > connected to HDMI controller or routed to pins, so it would make sense to
> > use same compatible for all of them. It's just that those I2S cores which
> > are routed to pins can use only one lane and >2 channels can be used only
> > in TDM mode of operation, if I understand this correctly.
> > 
> > New property would have to be optional, so it's omission would result in
> > same channel configuration as it is already present, to preserve
> > compatibility with old device trees. And this mode is already sufficient
> > for stereo HDMI audio support.
> 
> Yeah, it looks like a good plan.
> 
> > Side note: HDMI audio with current sun4i-i2s driver has a delay (about a
> > second), supposedly because DW HDMI controller automatically generates CTS
> > value based on I2S clock (auto CTS value generation is enabled per
> > DesignWare recomendation for DW HDMI I2S interface).
> 
> Is that a constant delay during the audio playback, or only at startup?

I think it's just at startup, e.g. if you're watching movie, audio is in sync, 
it's just that first second or so is silent.

> 
> > I2S driver from BSP Linux solves that by having I2S clock output
> > enabled all the time. Should this be flagged with some additional
> > property in DT?
> 
> I'd say that if the codec has that requirement, then it should be
> between the codec and the DAI, the DT doesn't really have anything to
> do with this.

Ok, but how to communicate that fact to I2S driver then? BSP driver solves 
that by using different compatible, but as I said before, I2S cores are not 
really different, so this seems wrong.

Best regards,
Jernej
Chen-Yu Tsai Aug. 6, 2019, 6:22 a.m. UTC | #8
On Thu, Aug 1, 2019 at 1:32 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne sreda, 31. julij 2019 ob 14:29:53 CEST je Maxime Ripard napisal(a):
> > On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
> > > Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
> > > > On Tue, 4 Jun 2019 at 11:02, Christopher Obbard <chris@64studio.com>
> wrote:
> > > > > On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
> > > > > > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard
> > > > > > <maxime.ripard@bootlin.com>
> > >
> > > wrote:
> > > > > > > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com
> wrote:
> > > > > > > > From: Marcus Cooper <codekipper@gmail.com>
> > > > > > > >
> > > > > > > > The i2s block supports multi-lane i2s output however this
> > > > > > > > functionality
> > > > > > > > is only possible in earlier SoCs where the pins are exposed and
> > > > > > > > for
> > > > > > > > the i2s block used for HDMI audio on the later SoCs.
> > > > > > > >
> > > > > > > > To enable this functionality, an optional property has been
> > > > > > > > added to
> > > > > > > > the bindings.
> > > > > > > >
> > > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > > > > > >
> > > > > > > I'd like to have Mark's input on this, but I'm really worried
> > > > > > > about
> > > > > > > the interaction with the proper TDM support.
> > > > > > >
> > > > > > > Our fundamental issue is that the controller can have up to 8
> > > > > > > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > > > > > > (like proper TDM) (or any combination between the two, but that
> > > > > > > should
> > > > > > > be pretty rare).
> > > > > >
> > > > > > I understand...maybe the TDM needs to be extended to support this to
> > > > > > consider channel mapping and multiple transfer lines. I was thinking
> > > > > > about the later when someone was requesting support on IIRC a while
> > > > > > ago, I thought masking might of been a solution. These can wait as
> > > > > > the
> > > > > > only consumer at the moment is LibreELEC and we can patch it there.
> > > > >
> > > > > Hi Marcus,
> > > > >
> > > > > FWIW, the TI McASP driver has support for TDM & (i think?) multiple
> > > > > transfer lines which are called serializers.
> > > > > Maybe this can help with inspiration?
> > > > > see
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > > > > e/s
> > > > > ound/soc/ti/davinci-mcasp.c sample DTS:
> > > > >
> > > > > &mcasp0 {
> > > > >
> > > > >     #sound-dai-cells = <0>;
> > > > >     status = "okay";
> > > > >     pinctrl-names = "default";
> > > > >     pinctrl-0 = <&mcasp0_pins>;
> > > > >
> > > > >     op-mode = <0>;
> > > > >     tdm-slots = <8>;
> > > > >     serial-dir = <
> > > > >
> > > > >         2 0 1 0
> > > > >         0 0 0 0
> > > > >         0 0 0 0
> > > > >         0 0 0 0
> > > > >     >
> > > > >     >;
> > > > >
> > > > >     tx-num-evt = <1>;
> > > > >     rx-num-evt = <1>;
> > > > >
> > > > > };
> > > > >
> > > > > Cheers!
> > > >
> > > > Thanks, this looks good.
> > >
> > > I would really like to see this issue resolved, because HDMI audio support
> > > in mainline Linux for those SoCs is long overdue.
> > >
> > > However, there is a possibility to still add HDMI audio suport (stereo
> > > only) even if this issue is not completely solved. If we agree that
> > > configuration of channels would be solved with additional property as
> > > Christopher suggested, support for >2 channels can be left for a later
> > > time when support for that property would be implemented. Currently,
> > > stereo HDMI audio support can be added with a few patches.
> > >
> > > I think all I2S cores are really the same, no matter if internally
> > > connected to HDMI controller or routed to pins, so it would make sense to
> > > use same compatible for all of them. It's just that those I2S cores which
> > > are routed to pins can use only one lane and >2 channels can be used only
> > > in TDM mode of operation, if I understand this correctly.
> > >
> > > New property would have to be optional, so it's omission would result in
> > > same channel configuration as it is already present, to preserve
> > > compatibility with old device trees. And this mode is already sufficient
> > > for stereo HDMI audio support.
> >
> > Yeah, it looks like a good plan.
> >
> > > Side note: HDMI audio with current sun4i-i2s driver has a delay (about a
> > > second), supposedly because DW HDMI controller automatically generates CTS
> > > value based on I2S clock (auto CTS value generation is enabled per
> > > DesignWare recomendation for DW HDMI I2S interface).
> >
> > Is that a constant delay during the audio playback, or only at startup?
>
> I think it's just at startup, e.g. if you're watching movie, audio is in sync,
> it's just that first second or so is silent.
>
> >
> > > I2S driver from BSP Linux solves that by having I2S clock output
> > > enabled all the time. Should this be flagged with some additional
> > > property in DT?
> >
> > I'd say that if the codec has that requirement, then it should be
> > between the codec and the DAI, the DT doesn't really have anything to
> > do with this.
>
> Ok, but how to communicate that fact to I2S driver then? BSP driver solves
> that by using different compatible, but as I said before, I2S cores are not
> really different, so this seems wrong.

Maybe we could make the DW-HDMI I2S driver require the I2S clock be on all
the time? You wouldn't need any changes to the DT.

ChenYu
Maxime Ripard Aug. 12, 2019, 10:02 a.m. UTC | #9
On Tue, Aug 06, 2019 at 02:22:13PM +0800, Chen-Yu Tsai wrote:
> On Thu, Aug 1, 2019 at 1:32 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> >
> > Dne sreda, 31. julij 2019 ob 14:29:53 CEST je Maxime Ripard napisal(a):
> > > On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
> > > > Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
> > > > > On Tue, 4 Jun 2019 at 11:02, Christopher Obbard <chris@64studio.com>
> > wrote:
> > > > > > On Tue, 4 Jun 2019 at 09:43, Code Kipper <codekipper@gmail.com> wrote:
> > > > > > > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard
> > > > > > > <maxime.ripard@bootlin.com>
> > > >
> > > > wrote:
> > > > > > > > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com
> > wrote:
> > > > > > > > > From: Marcus Cooper <codekipper@gmail.com>
> > > > > > > > >
> > > > > > > > > The i2s block supports multi-lane i2s output however this
> > > > > > > > > functionality
> > > > > > > > > is only possible in earlier SoCs where the pins are exposed and
> > > > > > > > > for
> > > > > > > > > the i2s block used for HDMI audio on the later SoCs.
> > > > > > > > >
> > > > > > > > > To enable this functionality, an optional property has been
> > > > > > > > > added to
> > > > > > > > > the bindings.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > > > > > > >
> > > > > > > > I'd like to have Mark's input on this, but I'm really worried
> > > > > > > > about
> > > > > > > > the interaction with the proper TDM support.
> > > > > > > >
> > > > > > > > Our fundamental issue is that the controller can have up to 8
> > > > > > > > channels, but either on 4 lines (instead of 1), or 8 channels on 1
> > > > > > > > (like proper TDM) (or any combination between the two, but that
> > > > > > > > should
> > > > > > > > be pretty rare).
> > > > > > >
> > > > > > > I understand...maybe the TDM needs to be extended to support this to
> > > > > > > consider channel mapping and multiple transfer lines. I was thinking
> > > > > > > about the later when someone was requesting support on IIRC a while
> > > > > > > ago, I thought masking might of been a solution. These can wait as
> > > > > > > the
> > > > > > > only consumer at the moment is LibreELEC and we can patch it there.
> > > > > >
> > > > > > Hi Marcus,
> > > > > >
> > > > > > FWIW, the TI McASP driver has support for TDM & (i think?) multiple
> > > > > > transfer lines which are called serializers.
> > > > > > Maybe this can help with inspiration?
> > > > > > see
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > > > > > e/s
> > > > > > ound/soc/ti/davinci-mcasp.c sample DTS:
> > > > > >
> > > > > > &mcasp0 {
> > > > > >
> > > > > >     #sound-dai-cells = <0>;
> > > > > >     status = "okay";
> > > > > >     pinctrl-names = "default";
> > > > > >     pinctrl-0 = <&mcasp0_pins>;
> > > > > >
> > > > > >     op-mode = <0>;
> > > > > >     tdm-slots = <8>;
> > > > > >     serial-dir = <
> > > > > >
> > > > > >         2 0 1 0
> > > > > >         0 0 0 0
> > > > > >         0 0 0 0
> > > > > >         0 0 0 0
> > > > > >     >
> > > > > >     >;
> > > > > >
> > > > > >     tx-num-evt = <1>;
> > > > > >     rx-num-evt = <1>;
> > > > > >
> > > > > > };
> > > > > >
> > > > > > Cheers!
> > > > >
> > > > > Thanks, this looks good.
> > > >
> > > > I would really like to see this issue resolved, because HDMI audio support
> > > > in mainline Linux for those SoCs is long overdue.
> > > >
> > > > However, there is a possibility to still add HDMI audio suport (stereo
> > > > only) even if this issue is not completely solved. If we agree that
> > > > configuration of channels would be solved with additional property as
> > > > Christopher suggested, support for >2 channels can be left for a later
> > > > time when support for that property would be implemented. Currently,
> > > > stereo HDMI audio support can be added with a few patches.
> > > >
> > > > I think all I2S cores are really the same, no matter if internally
> > > > connected to HDMI controller or routed to pins, so it would make sense to
> > > > use same compatible for all of them. It's just that those I2S cores which
> > > > are routed to pins can use only one lane and >2 channels can be used only
> > > > in TDM mode of operation, if I understand this correctly.
> > > >
> > > > New property would have to be optional, so it's omission would result in
> > > > same channel configuration as it is already present, to preserve
> > > > compatibility with old device trees. And this mode is already sufficient
> > > > for stereo HDMI audio support.
> > >
> > > Yeah, it looks like a good plan.
> > >
> > > > Side note: HDMI audio with current sun4i-i2s driver has a delay (about a
> > > > second), supposedly because DW HDMI controller automatically generates CTS
> > > > value based on I2S clock (auto CTS value generation is enabled per
> > > > DesignWare recomendation for DW HDMI I2S interface).
> > >
> > > Is that a constant delay during the audio playback, or only at startup?
> >
> > I think it's just at startup, e.g. if you're watching movie, audio is in sync,
> > it's just that first second or so is silent.
> >
> > >
> > > > I2S driver from BSP Linux solves that by having I2S clock output
> > > > enabled all the time. Should this be flagged with some additional
> > > > property in DT?
> > >
> > > I'd say that if the codec has that requirement, then it should be
> > > between the codec and the DAI, the DT doesn't really have anything to
> > > do with this.
> >
> > Ok, but how to communicate that fact to I2S driver then? BSP driver solves
> > that by using different compatible, but as I said before, I2S cores are not
> > really different, so this seems wrong.
>
> Maybe we could make the DW-HDMI I2S driver require the I2S clock be on all
> the time? You wouldn't need any changes to the DT.

That's an option, but I'd really like to avoid it if possible.

I guess we could also just add a delay in the powerup path in the HDMI
case? Would it work?

maxime

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

Patch
diff mbox series

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index bca73b3c0d74..75217fb52bfa 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -23,7 +23,7 @@ 
 
 #define SUN4I_I2S_CTRL_REG		0x00
 #define SUN4I_I2S_CTRL_SDO_EN_MASK		GENMASK(11, 8)
-#define SUN4I_I2S_CTRL_SDO_EN(sdo)			BIT(8 + (sdo))
+#define SUN4I_I2S_CTRL_SDO_EN(lines)		(((1 << lines) - 1) << 8)
 #define SUN4I_I2S_CTRL_MODE_MASK		BIT(5)
 #define SUN4I_I2S_CTRL_MODE_SLAVE			(1 << 5)
 #define SUN4I_I2S_CTRL_MODE_MASTER			(0 << 5)
@@ -355,14 +355,23 @@  static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 	int sr, wss, channels;
 	u32 width;
+	int lines;
 
 	channels = params_channels(params);
-	if (channels != 2) {
+	if ((channels > dai->driver->playback.channels_max) ||
+		(channels < dai->driver->playback.channels_min)) {
 		dev_err(dai->dev, "Unsupported number of channels: %d\n",
 			channels);
 		return -EINVAL;
 	}
 
+	lines = (channels + 1) / 2;
+
+	/* Enable the required output lines */
+	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+			   SUN4I_I2S_CTRL_SDO_EN_MASK,
+			   SUN4I_I2S_CTRL_SDO_EN(lines));
+
 	if (i2s->variant->is_h3_i2s_based) {
 		regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
 				   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
@@ -373,8 +382,19 @@  static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	/* Map the channels for playback and capture */
-	regmap_field_write(i2s->field_txchanmap, 0x76543210);
 	regmap_field_write(i2s->field_rxchanmap, 0x00003210);
+	regmap_field_write(i2s->field_txchanmap, 0x10);
+	if (i2s->variant->is_h3_i2s_based) {
+		if (channels > 2)
+			regmap_write(i2s->regmap,
+				     SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
+		if (channels > 4)
+			regmap_write(i2s->regmap,
+				     SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
+		if (channels > 6)
+			regmap_write(i2s->regmap,
+				     SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
+	}
 
 	/* Configure the channels */
 	regmap_field_write(i2s->field_txchansel,
@@ -1057,9 +1077,10 @@  static int sun4i_i2s_init_regmap_fields(struct device *dev,
 static int sun4i_i2s_probe(struct platform_device *pdev)
 {
 	struct sun4i_i2s *i2s;
+	struct snd_soc_dai_driver *soc_dai;
 	struct resource *res;
 	void __iomem *regs;
-	int irq, ret;
+	int irq, ret, val;
 
 	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
 	if (!i2s)
@@ -1126,6 +1147,19 @@  static int sun4i_i2s_probe(struct platform_device *pdev)
 	i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
 	i2s->capture_dma_data.maxburst = 8;
 
+	soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
+			       sizeof(*soc_dai), GFP_KERNEL);
+	if (!soc_dai) {
+		ret = -ENOMEM;
+		goto err_pm_disable;
+	}
+
+	if (!of_property_read_u32(pdev->dev.of_node,
+				  "allwinner,playback-channels", &val)) {
+		if (val >= 2 && val <= 8)
+			soc_dai->playback.channels_max = val;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = sun4i_i2s_runtime_resume(&pdev->dev);
@@ -1135,7 +1169,7 @@  static int sun4i_i2s_probe(struct platform_device *pdev)
 
 	ret = devm_snd_soc_register_component(&pdev->dev,
 					      &sun4i_i2s_component,
-					      &sun4i_i2s_dai, 1);
+					      soc_dai, 1);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register DAI\n");
 		goto err_suspend;