| Message ID | 1373526505-27115-1-git-send-email-padma.v@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Padmavathi, On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote: > Exynos5420 added support for I2S TDM mode. For this, there are some > register changes in the I2S controller. This patch adds the relevant > register changes to support I2S in normal mode. This patch adds a > quirk for TDM mode and if TDM mode is present all the relevent changes > will be applied. > > Signed-off-by: Padmavathi Venna <padma.v@samsung.com> > [abrestic: style cleanup and documentation] > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> > Reviewed-on: https://gerrit-int.chromium.org/37840 > Reviewed-by: Simon Glass <sjg@google.com> > --- > .../devicetree/bindings/sound/samsung-i2s.txt | 2 + > include/linux/platform_data/asoc-s3c.h | 1 + > sound/soc/samsung/i2s-regs.h | 51 ++++++--- > sound/soc/samsung/i2s.c | 117 > ++++++++++++++++---- 4 files changed, 132 insertions(+), 39 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt > b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index > 025e66b..b8593d5 100644 > --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt > +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt > @@ -28,6 +28,8 @@ Optional SoC Specific Properties: > enabled or disabled based on need. > - samsung,supports-secdai:If I2S block has a secondary FIFO and internal > DMA, then this flag is enabled. > +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag > + must be enabled. I think this should be rather handled by a different compatible value, not a flag. Also a word about what this TDM mode is would be nice. Best regards, Tomasz > - samsung,idma-addr: Internal DMA register base address of the audio > sub system(used in secondary sound source). > - pinctrl-0: Should specify pin control groups used for this controller. > diff --git a/include/linux/platform_data/asoc-s3c.h > b/include/linux/platform_data/asoc-s3c.h index 8827259..9efc04d 100644 > --- a/include/linux/platform_data/asoc-s3c.h > +++ b/include/linux/platform_data/asoc-s3c.h > @@ -36,6 +36,7 @@ struct samsung_i2s { > */ > #define QUIRK_NO_MUXPSR (1 << 2) > #define QUIRK_NEED_RSTCLR (1 << 3) > +#define QUIRK_SUPPORTS_TDM (1 << 4) > /* Quirks of the I2S controller */ > u32 quirks; > dma_addr_t idma_addr; > diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h > index c0e6d9a..821a502 100644 > --- a/sound/soc/samsung/i2s-regs.h > +++ b/sound/soc/samsung/i2s-regs.h > @@ -31,6 +31,10 @@ > #define I2SLVL1ADDR 0x34 > #define I2SLVL2ADDR 0x38 > #define I2SLVL3ADDR 0x3c > +#define I2SSTR1 0x40 > +#define I2SVER 0x44 > +#define I2SFIC2 0x48 > +#define I2STDM 0x4c > > #define CON_RSTCLR (1 << 31) > #define CON_FRXOFSTATUS (1 << 26) > @@ -95,24 +99,39 @@ > #define MOD_RXONLY (1 << 8) > #define MOD_TXRX (2 << 8) > #define MOD_MASK (3 << 8) > -#define MOD_LR_LLOW (0 << 7) > -#define MOD_LR_RLOW (1 << 7) > -#define MOD_SDF_IIS (0 << 5) > -#define MOD_SDF_MSB (1 << 5) > -#define MOD_SDF_LSB (2 << 5) > -#define MOD_SDF_MASK (3 << 5) > -#define MOD_RCLK_256FS (0 << 3) > -#define MOD_RCLK_512FS (1 << 3) > -#define MOD_RCLK_384FS (2 << 3) > -#define MOD_RCLK_768FS (3 << 3) > -#define MOD_RCLK_MASK (3 << 3) > -#define MOD_BCLK_32FS (0 << 1) > -#define MOD_BCLK_48FS (1 << 1) > -#define MOD_BCLK_16FS (2 << 1) > -#define MOD_BCLK_24FS (3 << 1) > -#define MOD_BCLK_MASK (3 << 1) > +#define MOD_LRP_SHIFT 7 > +#define MOD_LR_LLOW 0 > +#define MOD_LR_RLOW 1 > +#define MOD_SDF_SHIFT 5 > +#define MOD_SDF_IIS 0 > +#define MOD_SDF_MSB 1 > +#define MOD_SDF_LSB 2 > +#define MOD_SDF_MASK 3 > +#define MOD_RCLK_SHIFT 3 > +#define MOD_RCLK_256FS 0 > +#define MOD_RCLK_512FS 1 > +#define MOD_RCLK_384FS 2 > +#define MOD_RCLK_768FS 3 > +#define MOD_RCLK_MASK 3 > +#define MOD_BCLK_SHIFT 1 > +#define MOD_BCLK_32FS 0 > +#define MOD_BCLK_48FS 1 > +#define MOD_BCLK_16FS 2 > +#define MOD_BCLK_24FS 3 > +#define MOD_BCLK_MASK 3 > #define MOD_8BIT (1 << 0) > > +#define EXYNOS5420_MOD_LRP_SHIFT 15 > +#define EXYNOS5420_MOD_SDF_SHIFT 6 > +#define EXYNOS5420_MOD_RCLK_SHIFT 4 > +#define EXYNOS5420_MOD_BCLK_SHIFT 0 > +#define EXYNOS5420_MOD_BCLK_64FS 4 > +#define EXYNOS5420_MOD_BCLK_96FS 5 > +#define EXYNOS5420_MOD_BCLK_128FS 6 > +#define EXYNOS5420_MOD_BCLK_192FS 7 > +#define EXYNOS5420_MOD_BCLK_256FS 8 > +#define EXYNOS5420_MOD_BCLK_MASK 0xf > + > #define MOD_CDCLKCON (1 << 12) > > #define PSR_PSREN (1 << 15) > diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c > index 3fcf8d7..398f8db 100644 > --- a/sound/soc/samsung/i2s.c > +++ b/sound/soc/samsung/i2s.c > @@ -198,7 +198,13 @@ static inline bool is_manager(struct i2s_dai *i2s) > /* Read RCLK of I2S (in multiples of LRCLK) */ > static inline unsigned get_rfs(struct i2s_dai *i2s) > { > - u32 rfs = (readl(i2s->addr + I2SMOD) >> 3) & 0x3; > + u32 rfs; > + > + if (i2s->quirks & QUIRK_SUPPORTS_TDM) > + rfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_RCLK_SHIFT; > + else > + rfs = readl(i2s->addr + I2SMOD) >> MOD_RCLK_SHIFT; > + rfs &= MOD_RCLK_MASK; > > switch (rfs) { > case 3: return 768; > @@ -212,21 +218,26 @@ static inline unsigned get_rfs(struct i2s_dai *i2s) > static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs) > { > u32 mod = readl(i2s->addr + I2SMOD); > + int rfs_shift; > > - mod &= ~MOD_RCLK_MASK; > + if (i2s->quirks & QUIRK_SUPPORTS_TDM) > + rfs_shift = EXYNOS5420_MOD_RCLK_SHIFT; > + else > + rfs_shift = MOD_RCLK_SHIFT; > + mod &= ~(MOD_RCLK_MASK << rfs_shift); > > switch (rfs) { > case 768: > - mod |= MOD_RCLK_768FS; > + mod |= (MOD_RCLK_768FS << rfs_shift); > break; > case 512: > - mod |= MOD_RCLK_512FS; > + mod |= (MOD_RCLK_512FS << rfs_shift); > break; > case 384: > - mod |= MOD_RCLK_384FS; > + mod |= (MOD_RCLK_384FS << rfs_shift); > break; > default: > - mod |= MOD_RCLK_256FS; > + mod |= (MOD_RCLK_256FS << rfs_shift); > break; > } > > @@ -236,9 +247,22 @@ static inline void set_rfs(struct i2s_dai *i2s, > unsigned rfs) /* Read Bit-Clock of I2S (in multiples of LRCLK) */ > static inline unsigned get_bfs(struct i2s_dai *i2s) > { > - u32 bfs = (readl(i2s->addr + I2SMOD) >> 1) & 0x3; > + u32 bfs; > + > + if (i2s->quirks & QUIRK_SUPPORTS_TDM) { > + bfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_BCLK_SHIFT; > + bfs &= EXYNOS5420_MOD_BCLK_MASK; > + } else { > + bfs = readl(i2s->addr + I2SMOD) >> MOD_BCLK_SHIFT; > + bfs &= MOD_BCLK_MASK; > + } > > switch (bfs) { > + case 8: return 256; > + case 7: return 192; > + case 6: return 128; > + case 5: return 96; > + case 4: return 64; > case 3: return 24; > case 2: return 16; > case 1: return 48; > @@ -250,21 +274,50 @@ static inline unsigned get_bfs(struct i2s_dai *i2s) > static inline void set_bfs(struct i2s_dai *i2s, unsigned bfs) > { > u32 mod = readl(i2s->addr + I2SMOD); > + int bfs_shift; > + int tdm = i2s->quirks & QUIRK_SUPPORTS_TDM; > > - mod &= ~MOD_BCLK_MASK; > + if (tdm) { > + bfs_shift = EXYNOS5420_MOD_BCLK_SHIFT; > + mod &= ~(EXYNOS5420_MOD_BCLK_MASK << bfs_shift); > + } else { > + bfs_shift = MOD_BCLK_SHIFT; > + mod &= ~(MOD_BCLK_MASK << bfs_shift); > + } > + > + /* Non-TDM I2S controllers do not support BCLK > 48 * FS */ > + if (!tdm && bfs > 48) { > + dev_err(&i2s->pdev->dev, "Unsupported BCLK divider\n"); > + return; > + } > > switch (bfs) { > case 48: > - mod |= MOD_BCLK_48FS; > + mod |= (MOD_BCLK_48FS << bfs_shift); > break; > case 32: > - mod |= MOD_BCLK_32FS; > + mod |= (MOD_BCLK_32FS << bfs_shift); > break; > case 24: > - mod |= MOD_BCLK_24FS; > + mod |= (MOD_BCLK_24FS << bfs_shift); > break; > case 16: > - mod |= MOD_BCLK_16FS; > + mod |= (MOD_BCLK_16FS << bfs_shift); > + break; > + case 64: > + mod |= (EXYNOS5420_MOD_BCLK_64FS << bfs_shift); > + break; > + case 96: > + mod |= (EXYNOS5420_MOD_BCLK_96FS << bfs_shift); > + break; > + case 128: > + mod |= (EXYNOS5420_MOD_BCLK_128FS << bfs_shift); > + break; > + case 192: > + mod |= (EXYNOS5420_MOD_BCLK_192FS << bfs_shift); > + break; > + case 256: > + mod |= (EXYNOS5420_MOD_BCLK_256FS << bfs_shift); > break; > default: > dev_err(&i2s->pdev->dev, "Wrong BCLK Divider!\n"); > @@ -492,19 +545,31 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, > struct i2s_dai *i2s = to_info(dai); > u32 mod = readl(i2s->addr + I2SMOD); > u32 tmp = 0; > + int lrp_shift, sdf_shift, sdf_mask, lrp_rlow; > + > + if (i2s->quirks & QUIRK_SUPPORTS_TDM) { > + lrp_shift = EXYNOS5420_MOD_LRP_SHIFT; > + sdf_shift = EXYNOS5420_MOD_SDF_SHIFT; > + } else { > + lrp_shift = MOD_LRP_SHIFT; > + sdf_shift = MOD_SDF_SHIFT; > + } > + > + sdf_mask = MOD_SDF_MASK << sdf_shift; > + lrp_rlow = MOD_LR_RLOW << lrp_shift; > > /* Format is priority */ > switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { > case SND_SOC_DAIFMT_RIGHT_J: > - tmp |= MOD_LR_RLOW; > - tmp |= MOD_SDF_MSB; > + tmp |= lrp_rlow; > + tmp |= (MOD_SDF_MSB << sdf_shift); > break; > case SND_SOC_DAIFMT_LEFT_J: > - tmp |= MOD_LR_RLOW; > - tmp |= MOD_SDF_LSB; > + tmp |= lrp_rlow; > + tmp |= (MOD_SDF_LSB << sdf_shift); > break; > case SND_SOC_DAIFMT_I2S: > - tmp |= MOD_SDF_IIS; > + tmp |= (MOD_SDF_IIS << sdf_shift); > break; > default: > dev_err(&i2s->pdev->dev, "Format not supported\n"); > @@ -519,10 +584,10 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, > case SND_SOC_DAIFMT_NB_NF: > break; > case SND_SOC_DAIFMT_NB_IF: > - if (tmp & MOD_LR_RLOW) > - tmp &= ~MOD_LR_RLOW; > + if (tmp & lrp_rlow) > + tmp &= ~lrp_rlow; > else > - tmp |= MOD_LR_RLOW; > + tmp |= lrp_rlow; > break; > default: > dev_err(&i2s->pdev->dev, "Polarity not supported\n"); > @@ -544,15 +609,18 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, > return -EINVAL; > } > > + /* > + * Don't change the I2S mode if any controller is active on this > + * channel. > + */ > if (any_active(i2s) && > - ((mod & (MOD_SDF_MASK | MOD_LR_RLOW > - | MOD_SLAVE)) != tmp)) { > + ((mod & (sdf_mask | lrp_rlow | MOD_SLAVE)) != tmp)) { > dev_err(&i2s->pdev->dev, > "%s:%d Other DAI busy\n", __func__, __LINE__); > return -EAGAIN; > } > > - mod &= ~(MOD_SDF_MASK | MOD_LR_RLOW | MOD_SLAVE); > + mod &= ~(sdf_mask | lrp_rlow | MOD_SLAVE); > mod |= tmp; > writel(mod, i2s->addr + I2SMOD); > > @@ -1170,6 +1238,9 @@ static int samsung_i2s_probe(struct platform_device > *pdev) if (of_find_property(np, "samsung,supports-rstclr", NULL)) > quirks |= QUIRK_NEED_RSTCLR; > > + if (of_find_property(np, "samsung,supports-tdm", NULL)) > + quirks |= QUIRK_SUPPORTS_TDM; > + > if (of_property_read_u32(np, "samsung,idma-addr", > &idma_addr)) { > if (quirks & QUIRK_SEC_DAI) { -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote: > -#define MOD_LR_LLOW (0 << 7) > -#define MOD_LR_RLOW (1 << 7) > -#define MOD_SDF_IIS (0 << 5) > -#define MOD_SDF_MSB (1 << 5) > -#define MOD_SDF_LSB (2 << 5) > -#define MOD_SDF_MASK (3 << 5) > +#define MOD_LR_LLOW 0 > +#define MOD_LR_RLOW 1 > +#define MOD_SDF_SHIFT 5 > +#define MOD_SDF_IIS 0 > +#define MOD_SDF_MSB 1 > +#define MOD_SDF_LSB 2 > +#define MOD_SDF_MASK 3 This patch has an awful lot of coding style changes like this which are just coding style changes and not implementing TDM support. These should be done separately, not as part of the same patch, in order to make the code easier to review. > case 768: > - mod |= MOD_RCLK_768FS; > + mod |= (MOD_RCLK_768FS << rfs_shift); > break; This stuff is another example. I think the change itself should be fine but I'm not 100% sure I'm correctly identifying what's a stylistic change and what's a functional change.
On Thursday 11 of July 2013 11:48:04 Mark Brown wrote: > On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote: > > -#define MOD_LR_LLOW (0 << 7) > > -#define MOD_LR_RLOW (1 << 7) > > -#define MOD_SDF_IIS (0 << 5) > > -#define MOD_SDF_MSB (1 << 5) > > -#define MOD_SDF_LSB (2 << 5) > > -#define MOD_SDF_MASK (3 << 5) > > > > +#define MOD_LR_LLOW 0 > > +#define MOD_LR_RLOW 1 > > +#define MOD_SDF_SHIFT 5 > > +#define MOD_SDF_IIS 0 > > +#define MOD_SDF_MSB 1 > > +#define MOD_SDF_LSB 2 > > +#define MOD_SDF_MASK 3 > > This patch has an awful lot of coding style changes like this which > are just coding style changes and not implementing TDM support. These > should be done separately, not as part of the same patch, in order to > make the code easier to review. > > > case 768: > > - mod |= MOD_RCLK_768FS; > > + mod |= (MOD_RCLK_768FS << rfs_shift); > > > > break; > > This stuff is another example. > > I think the change itself should be fine but I'm not 100% sure I'm > correctly identifying what's a stylistic change and what's a functional > change. Right. This could be split into two patches, first reworking the style to give more flexibility with operations on registers and another one adding TDM specific changes, like new bitfield definitions and conditional handling of register accesses to account for new bitfield locations. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 12:43:22PM +0200, Tomasz Figa wrote: > On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote: > > +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag > > + must be enabled. > I think this should be rather handled by a different compatible value, not a This is a bit of a larger project sadly, there was resistance to doing the DT bindings based on compatible strings using the IP version :(
On Thursday 11 of July 2013 12:20:23 Mark Brown wrote: > On Thu, Jul 11, 2013 at 12:43:22PM +0200, Tomasz Figa wrote: > > On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote: > > > +- samsung,supports-tdm: If the I2S controller supports TDM, then this > > > flag > > > + must be enabled. > > > > I think this should be rather handled by a different compatible value, not > > a > This is a bit of a larger project sadly, there was resistance to doing > the DT bindings based on compatible strings using the IP version :( I'm not sure if this case is related to this problem in any way. I just suggested introducing a new include like samsung,exynos5420-i2s, a opposed to using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is the first one to have this TDM or whatever mode. (I'd still like to hear what it is...) Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote: > On Thursday 11 of July 2013 12:20:23 Mark Brown wrote: > > This is a bit of a larger project sadly, there was resistance to doing > > the DT bindings based on compatible strings using the IP version :( > I'm not sure if this case is related to this problem in any way. I just > suggested introducing a new include like samsung,exynos5420-i2s, a opposed to > using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is Well, it *should* be samsung,i2s-vN where N is the version number of the IP but documentation on the versioning has been patchy hence this whole thinng. > the first one to have this TDM or whatever mode. (I'd still like to hear what > it is...) What it *should* be is the option to send more than one audio stream down a link using time division multiplexing.
On Thu, Jul 11, 2013 at 4:37 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Thursday 11 of July 2013 11:48:04 Mark Brown wrote: >> On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote: >> > -#define MOD_LR_LLOW (0 << 7) >> > -#define MOD_LR_RLOW (1 << 7) >> > -#define MOD_SDF_IIS (0 << 5) >> > -#define MOD_SDF_MSB (1 << 5) >> > -#define MOD_SDF_LSB (2 << 5) >> > -#define MOD_SDF_MASK (3 << 5) >> > >> > +#define MOD_LR_LLOW 0 >> > +#define MOD_LR_RLOW 1 >> > +#define MOD_SDF_SHIFT 5 >> > +#define MOD_SDF_IIS 0 >> > +#define MOD_SDF_MSB 1 >> > +#define MOD_SDF_LSB 2 >> > +#define MOD_SDF_MASK 3 >> >> This patch has an awful lot of coding style changes like this which >> are just coding style changes and not implementing TDM support. These >> should be done separately, not as part of the same patch, in order to >> make the code easier to review. >> >> > case 768: >> > - mod |= MOD_RCLK_768FS; >> > + mod |= (MOD_RCLK_768FS << rfs_shift); >> > >> > break; >> >> This stuff is another example. >> >> I think the change itself should be fine but I'm not 100% sure I'm >> correctly identifying what's a stylistic change and what's a functional >> change. > > Right. This could be split into two patches, first reworking the style to give > more flexibility with operations on registers and another one adding TDM > specific changes, like new bitfield definitions and conditional handling of > register accesses to account for new bitfield locations. > > Best regards, > Tomasz > Ok. I will bisect the changes into two patches as suggested. Thanks Padma -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Jul 11, 2013 at 6:31 PM, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote: >> On Thursday 11 of July 2013 12:20:23 Mark Brown wrote: > >> > This is a bit of a larger project sadly, there was resistance to doing >> > the DT bindings based on compatible strings using the IP version :( > >> I'm not sure if this case is related to this problem in any way. I just >> suggested introducing a new include like samsung,exynos5420-i2s, a opposed to >> using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is > > Well, it *should* be samsung,i2s-vN where N is the version number of the > IP but documentation on the versioning has been patchy hence this whole > thinng. > A new version number is added when a there was some change in the IP like adding a internal mux to the IP, adding multi channel support, adding reset control bit. This was done in the older code with platform device. Same thing is followed here. So as previously done, adding a new compatible name like "samsung,i2s-v6" with new quirk "samsung,supports-tdm" is okey? I will mention about this versioning info in the Documentation in the next patch. >> the first one to have this TDM or whatever mode. (I'd still like to hear what >> it is...) > > What it *should* be is the option to send more than one audio stream > down a link using time division multiplexing. Thanks Padma -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 12, 2013 at 10:07:22AM +0530, Padma Venkat wrote: > A new version number is added when a there was some change in the IP > like adding a internal mux to the IP, adding multi channel support, > adding reset control bit. This was done in the older code with > platform device. > Same thing is followed here. So as previously done, adding a new > compatible name like "samsung,i2s-v6" with new quirk > "samsung,supports-tdm" is okey? Yes, this is good but the quirk should just be found by software based on the compatible string rather than in the DT.
On Thursday 11 of July 2013 14:01:39 Mark Brown wrote: > On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote: > > On Thursday 11 of July 2013 12:20:23 Mark Brown wrote: > > > This is a bit of a larger project sadly, there was resistance to doing > > > the DT bindings based on compatible strings using the IP version :( > > > > I'm not sure if this case is related to this problem in any way. I just > > suggested introducing a new include like samsung,exynos5420-i2s, a opposed > > to using samsung,exynos5250-i2s and adding flags, since the I2S in > > Exynos5420 is > Well, it *should* be samsung,i2s-vN where N is the version number of the > IP but documentation on the versioning has been patchy hence this whole > thinng. Oh, the binding for samsung-i2s uses IP versions. Isn't that slightly broken? At least with Samsung bindings, I think we happened to agree on a single convention of naming compatible values after the first SoC in which such block first appeared. I wonder what happened with this i2s binding that made it end up outside this convention. > > the first one to have this TDM or whatever mode. (I'd still like to hear > > what it is...) > > What it *should* be is the option to send more than one audio stream > down a link using time division multiplexing. OK. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 12, 2013 at 12:13:17PM +0200, Tomasz Figa wrote: > On Thursday 11 of July 2013 14:01:39 Mark Brown wrote: > > Well, it *should* be samsung,i2s-vN where N is the version number of the > > IP but documentation on the versioning has been patchy hence this whole > > thinng. > Oh, the binding for samsung-i2s uses IP versions. Isn't that slightly broken? > At least with Samsung bindings, I think we happened to agree on a single > convention of naming compatible values after the first SoC in which such block > first appeared. > I wonder what happened with this i2s binding that made it end up outside this > convention. I think it predates that convention, plus it's been quite common to put two different I2S blocks in the same SoC (for totally sensible reasons) so it's not always been clear that the SoC name is sufficiently unique.
Hi Mark, On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Jul 12, 2013 at 10:07:22AM +0530, Padma Venkat wrote: > >> A new version number is added when a there was some change in the IP >> like adding a internal mux to the IP, adding multi channel support, >> adding reset control bit. This was done in the older code with >> platform device. >> Same thing is followed here. So as previously done, adding a new >> compatible name like "samsung,i2s-v6" with new quirk >> "samsung,supports-tdm" is okey? > > Yes, this is good but the quirk should just be found by software based > on the compatible string rather than in the DT. You mean I just need to initialize the quirks in the driver based on the compatible string? So what about the quirks that I added previously on 5250? Should I also follow the same thing for other quirks? Or it need to be passed via driver data as it is done in spi driver? Please clarify me. Thanks Padma -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 23, 2013 at 09:38:00AM +0530, Padma Venkat wrote: > On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown <broonie@kernel.org> wrote: > > Yes, this is good but the quirk should just be found by software based > > on the compatible string rather than in the DT. > You mean I just need to initialize the quirks in the driver based on > the compatible string? That's the gold standard. > So what about the quirks that I added previously on 5250? Should I > also follow the same thing for other quirks? Yes, ideally. > Or it need to be passed via driver data as it is done in spi driver? > Please clarify me. This is a good way to initialise things based on the name - the core will do the lookups for you.
diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index 025e66b..b8593d5 100644 --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt @@ -28,6 +28,8 @@ Optional SoC Specific Properties: enabled or disabled based on need. - samsung,supports-secdai:If I2S block has a secondary FIFO and internal DMA, then this flag is enabled. +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag + must be enabled. - samsung,idma-addr: Internal DMA register base address of the audio sub system(used in secondary sound source). - pinctrl-0: Should specify pin control groups used for this controller. diff --git a/include/linux/platform_data/asoc-s3c.h b/include/linux/platform_data/asoc-s3c.h index 8827259..9efc04d 100644 --- a/include/linux/platform_data/asoc-s3c.h +++ b/include/linux/platform_data/asoc-s3c.h @@ -36,6 +36,7 @@ struct samsung_i2s { */ #define QUIRK_NO_MUXPSR (1 << 2) #define QUIRK_NEED_RSTCLR (1 << 3) +#define QUIRK_SUPPORTS_TDM (1 << 4) /* Quirks of the I2S controller */ u32 quirks; dma_addr_t idma_addr; diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h index c0e6d9a..821a502 100644 --- a/sound/soc/samsung/i2s-regs.h +++ b/sound/soc/samsung/i2s-regs.h @@ -31,6 +31,10 @@ #define I2SLVL1ADDR 0x34 #define I2SLVL2ADDR 0x38 #define I2SLVL3ADDR 0x3c +#define I2SSTR1 0x40 +#define I2SVER 0x44 +#define I2SFIC2 0x48 +#define I2STDM 0x4c #define CON_RSTCLR (1 << 31) #define CON_FRXOFSTATUS (1 << 26) @@ -95,24 +99,39 @@ #define MOD_RXONLY (1 << 8) #define MOD_TXRX (2 << 8) #define MOD_MASK (3 << 8) -#define MOD_LR_LLOW (0 << 7) -#define MOD_LR_RLOW (1 << 7) -#define MOD_SDF_IIS (0 << 5) -#define MOD_SDF_MSB (1 << 5) -#define MOD_SDF_LSB (2 << 5) -#define MOD_SDF_MASK (3 << 5) -#define MOD_RCLK_256FS (0 << 3) -#define MOD_RCLK_512FS (1 << 3) -#define MOD_RCLK_384FS (2 << 3) -#define MOD_RCLK_768FS (3 << 3) -#define MOD_RCLK_MASK (3 << 3) -#define MOD_BCLK_32FS (0 << 1) -#define MOD_BCLK_48FS (1 << 1) -#define MOD_BCLK_16FS (2 << 1) -#define MOD_BCLK_24FS (3 << 1) -#define MOD_BCLK_MASK (3 << 1) +#define MOD_LRP_SHIFT 7 +#define MOD_LR_LLOW 0 +#define MOD_LR_RLOW 1 +#define MOD_SDF_SHIFT 5 +#define MOD_SDF_IIS 0 +#define MOD_SDF_MSB 1 +#define MOD_SDF_LSB 2 +#define MOD_SDF_MASK 3 +#define MOD_RCLK_SHIFT 3 +#define MOD_RCLK_256FS 0 +#define MOD_RCLK_512FS 1 +#define MOD_RCLK_384FS 2 +#define MOD_RCLK_768FS 3 +#define MOD_RCLK_MASK 3 +#define MOD_BCLK_SHIFT 1 +#define MOD_BCLK_32FS 0 +#define MOD_BCLK_48FS 1 +#define MOD_BCLK_16FS 2 +#define MOD_BCLK_24FS 3 +#define MOD_BCLK_MASK 3 #define MOD_8BIT (1 << 0) +#define EXYNOS5420_MOD_LRP_SHIFT 15 +#define EXYNOS5420_MOD_SDF_SHIFT 6 +#define EXYNOS5420_MOD_RCLK_SHIFT 4 +#define EXYNOS5420_MOD_BCLK_SHIFT 0 +#define EXYNOS5420_MOD_BCLK_64FS 4 +#define EXYNOS5420_MOD_BCLK_96FS 5 +#define EXYNOS5420_MOD_BCLK_128FS 6 +#define EXYNOS5420_MOD_BCLK_192FS 7 +#define EXYNOS5420_MOD_BCLK_256FS 8 +#define EXYNOS5420_MOD_BCLK_MASK 0xf + #define MOD_CDCLKCON (1 << 12) #define PSR_PSREN (1 << 15) diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index 3fcf8d7..398f8db 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -198,7 +198,13 @@ static inline bool is_manager(struct i2s_dai *i2s) /* Read RCLK of I2S (in multiples of LRCLK) */ static inline unsigned get_rfs(struct i2s_dai *i2s) { - u32 rfs = (readl(i2s->addr + I2SMOD) >> 3) & 0x3; + u32 rfs; + + if (i2s->quirks & QUIRK_SUPPORTS_TDM) + rfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_RCLK_SHIFT; + else + rfs = readl(i2s->addr + I2SMOD) >> MOD_RCLK_SHIFT; + rfs &= MOD_RCLK_MASK; switch (rfs) { case 3: return 768; @@ -212,21 +218,26 @@ static inline unsigned get_rfs(struct i2s_dai *i2s) static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs) { u32 mod = readl(i2s->addr + I2SMOD); + int rfs_shift; - mod &= ~MOD_RCLK_MASK; + if (i2s->quirks & QUIRK_SUPPORTS_TDM) + rfs_shift = EXYNOS5420_MOD_RCLK_SHIFT; + else + rfs_shift = MOD_RCLK_SHIFT; + mod &= ~(MOD_RCLK_MASK << rfs_shift); switch (rfs) { case 768: - mod |= MOD_RCLK_768FS; + mod |= (MOD_RCLK_768FS << rfs_shift); break; case 512: - mod |= MOD_RCLK_512FS; + mod |= (MOD_RCLK_512FS << rfs_shift); break; case 384: - mod |= MOD_RCLK_384FS; + mod |= (MOD_RCLK_384FS << rfs_shift); break; default: - mod |= MOD_RCLK_256FS; + mod |= (MOD_RCLK_256FS << rfs_shift); break; } @@ -236,9 +247,22 @@ static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs) /* Read Bit-Clock of I2S (in multiples of LRCLK) */ static inline unsigned get_bfs(struct i2s_dai *i2s) { - u32 bfs = (readl(i2s->addr + I2SMOD) >> 1) & 0x3; + u32 bfs; + + if (i2s->quirks & QUIRK_SUPPORTS_TDM) { + bfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_BCLK_SHIFT; + bfs &= EXYNOS5420_MOD_BCLK_MASK; + } else { + bfs = readl(i2s->addr + I2SMOD) >> MOD_BCLK_SHIFT; + bfs &= MOD_BCLK_MASK; + } switch (bfs) { + case 8: return 256; + case 7: return 192; + case 6: return 128; + case 5: return 96; + case 4: return 64; case 3: return 24; case 2: return 16; case 1: return 48; @@ -250,21 +274,50 @@ static inline unsigned get_bfs(struct i2s_dai *i2s) static inline void set_bfs(struct i2s_dai *i2s, unsigned bfs) { u32 mod = readl(i2s->addr + I2SMOD); + int bfs_shift; + int tdm = i2s->quirks & QUIRK_SUPPORTS_TDM; - mod &= ~MOD_BCLK_MASK; + if (tdm) { + bfs_shift = EXYNOS5420_MOD_BCLK_SHIFT; + mod &= ~(EXYNOS5420_MOD_BCLK_MASK << bfs_shift); + } else { + bfs_shift = MOD_BCLK_SHIFT; + mod &= ~(MOD_BCLK_MASK << bfs_shift); + } + + /* Non-TDM I2S controllers do not support BCLK > 48 * FS */ + if (!tdm && bfs > 48) { + dev_err(&i2s->pdev->dev, "Unsupported BCLK divider\n"); + return; + } switch (bfs) { case 48: - mod |= MOD_BCLK_48FS; + mod |= (MOD_BCLK_48FS << bfs_shift); break; case 32: - mod |= MOD_BCLK_32FS; + mod |= (MOD_BCLK_32FS << bfs_shift); break; case 24: - mod |= MOD_BCLK_24FS; + mod |= (MOD_BCLK_24FS << bfs_shift); break; case 16: - mod |= MOD_BCLK_16FS; + mod |= (MOD_BCLK_16FS << bfs_shift); + break; + case 64: + mod |= (EXYNOS5420_MOD_BCLK_64FS << bfs_shift); + break; + case 96: + mod |= (EXYNOS5420_MOD_BCLK_96FS << bfs_shift); + break; + case 128: + mod |= (EXYNOS5420_MOD_BCLK_128FS << bfs_shift); + break; + case 192: + mod |= (EXYNOS5420_MOD_BCLK_192FS << bfs_shift); + break; + case 256: + mod |= (EXYNOS5420_MOD_BCLK_256FS << bfs_shift); break; default: dev_err(&i2s->pdev->dev, "Wrong BCLK Divider!\n"); @@ -492,19 +545,31 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, struct i2s_dai *i2s = to_info(dai); u32 mod = readl(i2s->addr + I2SMOD); u32 tmp = 0; + int lrp_shift, sdf_shift, sdf_mask, lrp_rlow; + + if (i2s->quirks & QUIRK_SUPPORTS_TDM) { + lrp_shift = EXYNOS5420_MOD_LRP_SHIFT; + sdf_shift = EXYNOS5420_MOD_SDF_SHIFT; + } else { + lrp_shift = MOD_LRP_SHIFT; + sdf_shift = MOD_SDF_SHIFT; + } + + sdf_mask = MOD_SDF_MASK << sdf_shift; + lrp_rlow = MOD_LR_RLOW << lrp_shift; /* Format is priority */ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_RIGHT_J: - tmp |= MOD_LR_RLOW; - tmp |= MOD_SDF_MSB; + tmp |= lrp_rlow; + tmp |= (MOD_SDF_MSB << sdf_shift); break; case SND_SOC_DAIFMT_LEFT_J: - tmp |= MOD_LR_RLOW; - tmp |= MOD_SDF_LSB; + tmp |= lrp_rlow; + tmp |= (MOD_SDF_LSB << sdf_shift); break; case SND_SOC_DAIFMT_I2S: - tmp |= MOD_SDF_IIS; + tmp |= (MOD_SDF_IIS << sdf_shift); break; default: dev_err(&i2s->pdev->dev, "Format not supported\n"); @@ -519,10 +584,10 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, case SND_SOC_DAIFMT_NB_NF: break; case SND_SOC_DAIFMT_NB_IF: - if (tmp & MOD_LR_RLOW) - tmp &= ~MOD_LR_RLOW; + if (tmp & lrp_rlow) + tmp &= ~lrp_rlow; else - tmp |= MOD_LR_RLOW; + tmp |= lrp_rlow; break; default: dev_err(&i2s->pdev->dev, "Polarity not supported\n"); @@ -544,15 +609,18 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, return -EINVAL; } + /* + * Don't change the I2S mode if any controller is active on this + * channel. + */ if (any_active(i2s) && - ((mod & (MOD_SDF_MASK | MOD_LR_RLOW - | MOD_SLAVE)) != tmp)) { + ((mod & (sdf_mask | lrp_rlow | MOD_SLAVE)) != tmp)) { dev_err(&i2s->pdev->dev, "%s:%d Other DAI busy\n", __func__, __LINE__); return -EAGAIN; } - mod &= ~(MOD_SDF_MASK | MOD_LR_RLOW | MOD_SLAVE); + mod &= ~(sdf_mask | lrp_rlow | MOD_SLAVE); mod |= tmp; writel(mod, i2s->addr + I2SMOD); @@ -1170,6 +1238,9 @@ static int samsung_i2s_probe(struct platform_device *pdev) if (of_find_property(np, "samsung,supports-rstclr", NULL)) quirks |= QUIRK_NEED_RSTCLR; + if (of_find_property(np, "samsung,supports-tdm", NULL)) + quirks |= QUIRK_SUPPORTS_TDM; + if (of_property_read_u32(np, "samsung,idma-addr", &idma_addr)) { if (quirks & QUIRK_SEC_DAI) {