Re: [PATCH v2 2/2] ASoC: sun4i-i2s: Add support for H3
From: Maxime Ripard
Date: Tue Jul 25 2017 - 10:36:53 EST
Hi,
On Sat, Jul 22, 2017 at 08:53:52AM +0200, codekipper@xxxxxxxxx wrote:
> From: Marcus Cooper <codekipper@xxxxxxxxx>
>
> The sun8i-h3 introduces a lot of changes to the i2s block such
> as different register locations, extended clock division and
> more operational modes. As we have to consider the earlier
> implementation then these changes need to be isolated.
>
> Signed-off-by: Marcus Cooper <codekipper@xxxxxxxxx>
> ---
> .../devicetree/bindings/sound/sun4i-i2s.txt | 2 +
> sound/soc/sunxi/sun4i-i2s.c | 202 +++++++++++++++++++++
> 2 files changed, 204 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/sun4i-i2s.txt b/Documentation/devicetree/bindings/sound/sun4i-i2s.txt
> index ee21da865771..fc5da6080759 100644
> --- a/Documentation/devicetree/bindings/sound/sun4i-i2s.txt
> +++ b/Documentation/devicetree/bindings/sound/sun4i-i2s.txt
> @@ -8,6 +8,7 @@ Required properties:
> - compatible: should be one of the following:
> - "allwinner,sun4i-a10-i2s"
> - "allwinner,sun6i-a31-i2s"
> + - "allwinner,sun8i-h3-i2s"
> - reg: physical base address of the controller and length of memory mapped
> region.
> - interrupts: should contain the I2S interrupt.
> @@ -22,6 +23,7 @@ Required properties:
>
> Required properties for the following compatibles:
> - "allwinner,sun6i-a31-i2s"
> + - "allwinner,sun8i-h3-i2s"
> - resets: phandle to the reset line for this codec
>
> Example:
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 1854405cbcb1..2b3c2b28059c 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -26,6 +26,8 @@
> #include <sound/soc-dai.h>
>
> #define SUN4I_I2S_CTRL_REG 0x00
> +#define SUN8I_I2S_CTRL_BCLK_OUT BIT(18)
> +#define SUN8I_I2S_CTRL_LRCK_OUT BIT(17)
> #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8)
> #define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo))
> #define SUN4I_I2S_CTRL_MODE_MASK BIT(5)
> @@ -55,6 +57,7 @@
>
> #define SUN4I_I2S_FMT1_REG 0x08
> #define SUN4I_I2S_FIFO_TX_REG 0x0c
> +#define SUN8I_I2S_INT_STA_REG 0x0c
> #define SUN4I_I2S_FIFO_RX_REG 0x10
>
> #define SUN4I_I2S_FIFO_CTRL_REG 0x14
> @@ -72,8 +75,10 @@
> #define SUN4I_I2S_DMA_INT_CTRL_RX_DRQ_EN BIT(3)
>
> #define SUN4I_I2S_INT_STA_REG 0x20
> +#define SUN8I_I2S_FIFO_TX_REG 0x20
>
> #define SUN4I_I2S_CLK_DIV_REG 0x24
> +#define SUN8I_I2S_CLK_DIV_MCLK_EN 8
> #define SUN4I_I2S_CLK_DIV_MCLK_EN 7
> #define SUN4I_I2S_CLK_DIV_BCLK_MASK GENMASK(6, 4)
> #define SUN4I_I2S_CLK_DIV_BCLK(bclk) ((bclk) << 4)
> @@ -86,16 +91,29 @@
> #define SUN4I_I2S_TX_CHAN_SEL_REG 0x30
> #define SUN4I_I2S_CHAN_SEL(num_chan) (((num_chan) - 1) << 0)
>
> +#define SUN8I_I2S_CHAN_CFG_REG 0x30
> +
> #define SUN4I_I2S_TX_CHAN_MAP_REG 0x34
> #define SUN4I_I2S_TX_CHAN_MAP(chan, sample) ((sample) << (chan << 2))
> +#define SUN8I_I2S_TX_CHAN_SEL_REG 0x34
> +#define SUN8I_I2S_TX_CHAN_OFFSET(offset) (offset << 12)
> #define SUN4I_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1))
>
> #define SUN4I_I2S_RX_CHAN_SEL_REG 0x38
> #define SUN4I_I2S_RX_CHAN_MAP_REG 0x3c
>
> +#define SUN8I_I2S_TX_CHAN_MAP_REG 0x44
> +
> +#define SUN8I_I2S_RX_CHAN_SEL_REG 0x54
> +#define SUN8I_I2S_RX_CHAN_MAP_REG 0x58
> +
I would not interleave those defines. It's a bit hard to see which
generation has which set of registers. I guess you could just move the
new ones to the bottom of the defines.
> struct sun4i_i2s_quirks {
> bool has_reset;
> bool has_master_slave_sel;
> + bool has_fmt_set_lrck_period;
> + bool has_chcfg;
> + bool has_chsel_tx_chen;
> + bool has_chsel_offset;
> unsigned int reg_offset_txdata; /* TX FIFO */
> unsigned int reg_offset_txchanmap;
> unsigned int reg_offset_rxchanmap;
> @@ -113,6 +131,11 @@ struct sun4i_i2s_quirks {
> struct reg_field field_fmt_set_mode;
> struct reg_field field_txchansel;
> struct reg_field field_rxchansel;
> + struct reg_field field_fmt_set_lrck_period;
> + struct reg_field field_chcfg_tx_slot_num;
> + struct reg_field field_chcfg_rx_slot_num;
> + struct reg_field field_chsel_tx_chen;
> + struct reg_field field_chsel_offset;
If you have booleans already, I guess you don't really need the
regmap_fields. You won't make that setup in the !h3 case, so the
regmap_field mapping is going to be useless anyway.
> };
>
> struct sun4i_i2s {
> @@ -136,6 +159,11 @@ struct sun4i_i2s {
> struct regmap_field *field_fmt_set_mode;
> struct regmap_field *field_txchansel;
> struct regmap_field *field_rxchansel;
> + struct regmap_field *field_fmt_set_lrck_period;
> + struct regmap_field *field_chcfg_tx_slot_num;
> + struct regmap_field *field_chcfg_rx_slot_num;
> + struct regmap_field *field_chsel_tx_chen;
> + struct regmap_field *field_chsel_offset;
>
> const struct sun4i_i2s_quirks *variant;
> };
> @@ -152,6 +180,14 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_bclk_div[] = {
> { .div = 8, .val = 3 },
> { .div = 12, .val = 4 },
> { .div = 16, .val = 5 },
> + { .div = 24, .val = 6 },
> + { .div = 32, .val = 7 },
> + { .div = 48, .val = 8 },
> + { .div = 64, .val = 9 },
> + { .div = 96, .val = 10 },
> + { .div = 128, .val = 11 },
> + { .div = 176, .val = 12 },
> + { .div = 192, .val = 13 },
> };
>
> static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] = {
> @@ -163,6 +199,13 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] = {
> { .div = 12, .val = 5 },
> { .div = 16, .val = 6 },
> { .div = 24, .val = 7 },
> + { .div = 32, .val = 8 },
> + { .div = 48, .val = 9 },
> + { .div = 64, .val = 10 },
> + { .div = 96, .val = 11 },
> + { .div = 128, .val = 12 },
> + { .div = 176, .val = 13 },
> + { .div = 192, .val = 14 },
> };
I'm not sure about this one. We might end up on !h3 with an invalid
divider.
> static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i2s,
> @@ -270,6 +313,10 @@ static int sun4i_i2s_set_clk_rate(struct sun4i_i2s *i2s,
>
> regmap_field_write(i2s->field_clkdiv_mclk_en, 1);
>
> + /* Set sync period */
> + if (i2s->variant->has_fmt_set_lrck_period)
> + regmap_field_write(i2s->field_fmt_set_lrck_period, 0x1f);
> +
> return 0;
> }
>
> @@ -284,6 +331,13 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> if (params_channels(params) != 2)
> return -EINVAL;
>
> + if (i2s->variant->has_chcfg) {
> + regmap_field_write(i2s->field_chcfg_tx_slot_num,
> + params_channels(params) - 1);
> + regmap_field_write(i2s->field_chcfg_rx_slot_num,
> + params_channels(params) - 1);
> + }
> +
> /* Map the channels for playback */
> regmap_write(i2s->regmap, i2s->variant->reg_offset_txchanmap,
> 0x76543210);
> @@ -299,6 +353,9 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> regmap_field_write(i2s->field_rxchansel,
> SUN4I_I2S_CHAN_SEL(params_channels(params)));
>
> + if (i2s->variant->has_chsel_tx_chen)
> + regmap_field_write(i2s->field_chsel_tx_chen,
> + SUN4I_I2S_TX_CHAN_EN(params_channels(params)));
>
> switch (params_physical_width(params)) {
> case 16:
> @@ -332,6 +389,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> {
> struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> u32 val;
> + u32 offset = 0;
> u32 bclk_polarity = SUN4I_I2S_FMT0_POLARITY_NORMAL;
> u32 lrclk_polarity = SUN4I_I2S_FMT0_POLARITY_NORMAL;
>
> @@ -339,6 +397,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> case SND_SOC_DAIFMT_I2S:
> val = SUN4I_I2S_FMT0_FMT_I2S;
> + offset = 1;
> break;
> case SND_SOC_DAIFMT_LEFT_J:
> val = SUN4I_I2S_FMT0_FMT_LEFT_J;
> @@ -350,6 +409,19 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> return -EINVAL;
> }
>
> + if (i2s->variant->has_chsel_offset) {
> + /*
> + * offset being set indicates that we're connected to an i2s
> + * device, however offset is only used on the sun8i block and
> + * i2s shares the same setting with the LJ format. Increment
> + * val so that the bit to value to write is correct.
> + */
> + if (offset > 0)
> + val++;
> + /* blck offset determines whether i2s or LJ */
> + regmap_field_write(i2s->field_chsel_offset, offset);
> + }
> +
> regmap_field_write(i2s->field_fmt_set_mode, val);
>
> /* DAI clock polarity */
> @@ -393,6 +465,29 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> SUN4I_I2S_CTRL_MODE_MASK,
> val);
> + } else {
> + /*
> + * The newer i2s block does not have a slave select bit,
> + * instead the clk pins are configured as inputs.
> + */
> + /* DAI clock master masks */
> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBS_CFS:
> + /* BCLK and LRCLK master */
> + val = SUN8I_I2S_CTRL_BCLK_OUT |
> + SUN8I_I2S_CTRL_LRCK_OUT;
> + break;
> + case SND_SOC_DAIFMT_CBM_CFM:
> + /* BCLK and LRCLK slave */
> + val = 0;
> + break;
> + default:
> + return -EINVAL;
> + }
> + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> + SUN8I_I2S_CTRL_BCLK_OUT |
> + SUN8I_I2S_CTRL_LRCK_OUT,
> + val);
> }
>
> /* Set significant bits in our FIFOs */
> @@ -642,6 +737,15 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
> }
> }
>
> +static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +
> + if (reg == SUN8I_I2S_INT_STA_REG)
> + return true;
> +
> + return sun4i_i2s_volatile_reg(dev, reg);
> +}
This means that SUN8I_I2S_FIFO_TX_REG will be treated as volatile.
Thanks!
maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Attachment:
signature.asc
Description: PGP signature