[PATCH v2 2/2] ASoC: sun4i-i2s: Add support for H3

Code Kipper codekipper at gmail.com
Tue Jul 25 10:52:15 PDT 2017


On 25 July 2017 at 16:36, Maxime Ripard
<maxime.ripard at free-electrons.com> wrote:
> Hi,
>
> On Sat, Jul 22, 2017 at 08:53:52AM +0200, codekipper at gmail.com wrote:
>> From: Marcus Cooper <codekipper at gmail.com>
>>
>> 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 at gmail.com>
>> ---
>>  .../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.

Ok...you especially see it when looking at the patch. I'll add a
comment and move everything to the bottom.
>
>>  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.

ack

>
>>  };
>>
>>  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.
Yeah...this is a late addition to the change and I've not experimented
with the values. Maybe I can add this at a later stage.

>
>>  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.

I'll look into this....
BR,
CK
>
> Thanks!
> maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



More information about the linux-arm-kernel mailing list