Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752206AbdGYOgt (ORCPT ); Tue, 25 Jul 2017 10:36:49 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:57295 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751843AbdGYOgs (ORCPT ); Tue, 25 Jul 2017 10:36:48 -0400 Date: Tue, 25 Jul 2017 16:36:46 +0200 From: Maxime Ripard To: codekipper@gmail.com Cc: linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, lgirdwood@gmail.com, broonie@kernel.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, be17068@iperbole.bo.it Subject: Re: [PATCH v2 2/2] ASoC: sun4i-i2s: Add support for H3 Message-ID: <20170725143646.mlqhzfqnoenwtg7v@flea> References: <20170722065352.7635-1-codekipper@gmail.com> <20170722065352.7635-3-codekipper@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="47eea77pdbrntzuw" Content-Disposition: inline In-Reply-To: <20170722065352.7635-3-codekipper@gmail.com> User-Agent: NeoMutt/20170714 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11071 Lines: 328 --47eea77pdbrntzuw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Sat, Jul 22, 2017 at 08:53:52AM +0200, codekipper@gmail.com wrote: > From: Marcus Cooper >=20 > 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. >=20 > Signed-off-by: Marcus Cooper > --- > .../devicetree/bindings/sound/sun4i-i2s.txt | 2 + > sound/soc/sunxi/sun4i-i2s.c | 202 +++++++++++++++= ++++++ > 2 files changed, 204 insertions(+) >=20 > diff --git a/Documentation/devicetree/bindings/sound/sun4i-i2s.txt b/Docu= mentation/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 mapp= ed > region. > - interrupts: should contain the I2S interrupt. > @@ -22,6 +23,7 @@ Required properties: > =20 > Required properties for the following compatibles: > - "allwinner,sun6i-a31-i2s" > + - "allwinner,sun8i-h3-i2s" > - resets: phandle to the reset line for this codec > =20 > 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 > =20 > #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 @@ > =20 > #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 > =20 > #define SUN4I_I2S_FIFO_CTRL_REG 0x14 > @@ -72,8 +75,10 @@ > #define SUN4I_I2S_DMA_INT_CTRL_RX_DRQ_EN BIT(3) > =20 > #define SUN4I_I2S_INT_STA_REG 0x20 > +#define SUN8I_I2S_FIFO_TX_REG 0x20 > =20 > #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) > =20 > +#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)) > =20 > #define SUN4I_I2S_RX_CHAN_SEL_REG 0x38 > #define SUN4I_I2S_RX_CHAN_MAP_REG 0x3c > =20 > +#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. > }; > =20 > 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; > =20 > const struct sun4i_i2s_quirks *variant; > }; > @@ -152,6 +180,14 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_bclk= _div[] =3D { > { .div =3D 8, .val =3D 3 }, > { .div =3D 12, .val =3D 4 }, > { .div =3D 16, .val =3D 5 }, > + { .div =3D 24, .val =3D 6 }, > + { .div =3D 32, .val =3D 7 }, > + { .div =3D 48, .val =3D 8 }, > + { .div =3D 64, .val =3D 9 }, > + { .div =3D 96, .val =3D 10 }, > + { .div =3D 128, .val =3D 11 }, > + { .div =3D 176, .val =3D 12 }, > + { .div =3D 192, .val =3D 13 }, > }; > =20 > static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] =3D { > @@ -163,6 +199,13 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_mclk= _div[] =3D { > { .div =3D 12, .val =3D 5 }, > { .div =3D 16, .val =3D 6 }, > { .div =3D 24, .val =3D 7 }, > + { .div =3D 32, .val =3D 8 }, > + { .div =3D 48, .val =3D 9 }, > + { .div =3D 64, .val =3D 10 }, > + { .div =3D 96, .val =3D 11 }, > + { .div =3D 128, .val =3D 12 }, > + { .div =3D 176, .val =3D 13 }, > + { .div =3D 192, .val =3D 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, > =20 > regmap_field_write(i2s->field_clkdiv_mclk_en, 1); > =20 > + /* Set sync period */ > + if (i2s->variant->has_fmt_set_lrck_period) > + regmap_field_write(i2s->field_fmt_set_lrck_period, 0x1f); > + > return 0; > } > =20 > @@ -284,6 +331,13 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substr= eam *substream, > if (params_channels(params) !=3D 2) > return -EINVAL; > =20 > + 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_substre= am *substream, > regmap_field_write(i2s->field_rxchansel, > SUN4I_I2S_CHAN_SEL(params_channels(params))); > =20 > + if (i2s->variant->has_chsel_tx_chen) > + regmap_field_write(i2s->field_chsel_tx_chen, > + SUN4I_I2S_TX_CHAN_EN(params_channels(params))); > =20 > 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 =3D snd_soc_dai_get_drvdata(dai); > u32 val; > + u32 offset =3D 0; > u32 bclk_polarity =3D SUN4I_I2S_FMT0_POLARITY_NORMAL; > u32 lrclk_polarity =3D SUN4I_I2S_FMT0_POLARITY_NORMAL; > =20 > @@ -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 =3D SUN4I_I2S_FMT0_FMT_I2S; > + offset =3D 1; > break; > case SND_SOC_DAIFMT_LEFT_J: > val =3D 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; > } > =20 > + 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); > =20 > /* 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 =3D SUN8I_I2S_CTRL_BCLK_OUT | > + SUN8I_I2S_CTRL_LRCK_OUT; > + break; > + case SND_SOC_DAIFMT_CBM_CFM: > + /* BCLK and LRCLK slave */ > + val =3D 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); > } > =20 > /* Set significant bits in our FIFOs */ > @@ -642,6 +737,15 @@ static bool sun4i_i2s_volatile_reg(struct device *de= v, unsigned int reg) > } > } > =20 > +static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg) > +{ > + > + if (reg =3D=3D 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 --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --47eea77pdbrntzuw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZd1d+AAoJEBx+YmzsjxAgdYoP/0dAIzKdvNSIkHMiEjInYFi4 40PUr2L+FH1tUyVJU/5h/QRPHT2xGVtA54VXLZOx4ogq3XhxpowKwbJg9upIHV75 ci5wNcPIX6Z2DJoblxZMfHBfrmTYKs0w6dwOimbbNSE5YanqvfNuTD9fUfNcAndY +K+P3dLqdsa42bxndRK73ZxCNMzRR70nrp492V4eKZJyMWWC2tYLve9hGl2Ty+aV KS/SHDG8IUw95bj3zkqYnjyWZldi1rhLusrCr5Piup8qOCf/hRs0KhCFfggURhaD Xy41ylJb+evkV33hGZFu4rrWavGxPu0TR/9kNQuH6AwN2CS6mWCCxwQzmM0SyO1J 7C9SQBbq5wsCFBivVxDDy/2L72cswkrWrMgm0AzXPihfyn9O3yZIR7URyrQGfme7 Kgwb5hphv9EkF3GzT3ImrCMcheb308l95CpDKUiV+JaC6VfRoVzSL+oUyr+tScVA EM8HTS0WpfN1dGFwEoHnoABoe0uVC9+LwnrATlhSM8sGs6pXwH7OlTD61JGAxOoj +wZ8sSkR6P+Xar7bfbW0grLxYY3RS6YJbxFWyePoKAfnJXPx5en+GV2xm1lIrt+T YQurWwJouS1iLHA1+01BnzMNgVsrJ3QawQO8AkMYnw4LHWfLDCj8wohXjNd5ODFq Rm89BGjfHaPZ5oRNBOnk =YFXt -----END PGP SIGNATURE----- --47eea77pdbrntzuw--