| Message ID | 20230103045613.100309-3-p.rajanbabu@samsung.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [v2,1/5] ASoC: dt-bindings: Add FSD I2S controller bindings | expand |
On 03/01/2023 05:56, Padmanabhan Rajanbabu wrote: > Add support for enabling I2S controller on FSD platform. > > FSD I2S controller is based on Exynos7 I2S controller, supporting > 2CH playback/capture in I2S mode and 7.1CH playback/capture in TDM > mode. > > Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com> > --- > sound/soc/samsung/i2s-regs.h | 1 + > sound/soc/samsung/i2s.c | 57 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h > index b4b5d6053503..4444c857d0c0 100644 > --- a/sound/soc/samsung/i2s-regs.h > +++ b/sound/soc/samsung/i2s-regs.h > @@ -132,6 +132,7 @@ > #define EXYNOS7_MOD_RCLK_192FS 7 > > #define PSR_PSREN (1 << 15) > +#define PSR_PSVAL(x) (((x - 1) << 8) & 0x3f00) > > #define FIC_TX2COUNT(x) (((x) >> 24) & 0xf) > #define FIC_TX1COUNT(x) (((x) >> 16) & 0xf) > diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c > index 9505200f3d11..dcb5c438cb2f 100644 > --- a/sound/soc/samsung/i2s.c > +++ b/sound/soc/samsung/i2s.c > @@ -50,6 +50,10 @@ struct samsung_i2s_dai_data { > u32 quirks; > unsigned int pcm_rates; > const struct samsung_i2s_variant_regs *i2s_variant_regs; > + void (*fixup_early)(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai); > + void (*fixup_late)(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai); > }; > > struct i2s_dai { > @@ -111,6 +115,10 @@ struct samsung_i2s_priv { > u32 suspend_i2spsr; > > const struct samsung_i2s_variant_regs *variant_regs; > + void (*fixup_early)(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai); > + void (*fixup_late)(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai); > u32 quirks; > > /* The clock provider's data */ > @@ -940,6 +948,10 @@ static int i2s_trigger(struct snd_pcm_substream *substream, > case SNDRV_PCM_TRIGGER_RESUME: > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > pm_runtime_get_sync(dai->dev); > + > + if (priv->fixup_early) > + priv->fixup_early(substream, dai); > + > spin_lock_irqsave(&priv->lock, flags); > > if (config_setup(i2s)) { > @@ -947,6 +959,13 @@ static int i2s_trigger(struct snd_pcm_substream *substream, > return -EINVAL; > } > Except several warnings this patch generates, this is a bit surprising: > + spin_unlock_irqrestore(&priv->lock, flags); You have critical section which you now break into two. You cannot do this usually. How the synchronization is now kept? > + > + if (priv->fixup_late) > + priv->fixup_late(substream, dai); > + > + spin_lock_irqsave(&priv->lock, flags); > + > if (capture) > i2s_rxctrl(i2s, 1); > else Best regards, Krzysztof
On Tue, Jan 03, 2023 at 10:26:10AM +0530, Padmanabhan Rajanbabu wrote: > +void fsd_i2s_fixup_early(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); > + struct i2s_dai *i2s = to_info(asoc_rtd_to_cpu(rtd, 0)); > + struct i2s_dai *other = get_other_dai(i2s); > + > + if (!is_opened(other)) { > + i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK, 192, SND_SOC_CLOCK_OUT); > + i2s_set_sysclk(dai, SAMSUNG_I2S_OPCLK, 0, MOD_OPCLK_PCLK); > + } > +} This looks like we're just hard coding to 192kHz?
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] > Sent: 03 January 2023 04:39 PM > To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>; > lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com; > perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com; > alim.akhtar@samsung.com; rcsekar@samsung.com; > aswani.reddy@samsung.com > Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org > Subject: Re: [PATCH v2 2/5] ASoC: samsung: i2s: add support for FSD I2S > > On 03/01/2023 05:56, Padmanabhan Rajanbabu wrote: > > Add support for enabling I2S controller on FSD platform. > > > > FSD I2S controller is based on Exynos7 I2S controller, supporting 2CH > > playback/capture in I2S mode and 7.1CH playback/capture in TDM mode. > > > > Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com> > > --- > > sound/soc/samsung/i2s-regs.h | 1 + > > sound/soc/samsung/i2s.c | 57 > ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 58 insertions(+) > > > > diff --git a/sound/soc/samsung/i2s-regs.h > > b/sound/soc/samsung/i2s-regs.h index b4b5d6053503..4444c857d0c0 > 100644 > > --- a/sound/soc/samsung/i2s-regs.h > > +++ b/sound/soc/samsung/i2s-regs.h > > @@ -132,6 +132,7 @@ > > #define EXYNOS7_MOD_RCLK_192FS 7 > > > > #define PSR_PSREN (1 << 15) > > +#define PSR_PSVAL(x) (((x - 1) << 8) & 0x3f00) > > > > #define FIC_TX2COUNT(x) (((x) >> 24) & 0xf) > > #define FIC_TX1COUNT(x) (((x) >> 16) & 0xf) > > diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index > > 9505200f3d11..dcb5c438cb2f 100644 > > --- a/sound/soc/samsung/i2s.c > > +++ b/sound/soc/samsung/i2s.c > > @@ -50,6 +50,10 @@ struct samsung_i2s_dai_data { > > u32 quirks; > > unsigned int pcm_rates; > > const struct samsung_i2s_variant_regs *i2s_variant_regs; > > + void (*fixup_early)(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai); > > + void (*fixup_late)(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai); > > }; > > > > struct i2s_dai { > > @@ -111,6 +115,10 @@ struct samsung_i2s_priv { > > u32 suspend_i2spsr; > > > > const struct samsung_i2s_variant_regs *variant_regs; > > + void (*fixup_early)(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai); > > + void (*fixup_late)(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai); > > u32 quirks; > > > > /* The clock provider's data */ > > @@ -940,6 +948,10 @@ static int i2s_trigger(struct snd_pcm_substream > *substream, > > case SNDRV_PCM_TRIGGER_RESUME: > > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > > pm_runtime_get_sync(dai->dev); > > + > > + if (priv->fixup_early) > > + priv->fixup_early(substream, dai); > > + > > spin_lock_irqsave(&priv->lock, flags); > > > > if (config_setup(i2s)) { > > @@ -947,6 +959,13 @@ static int i2s_trigger(struct snd_pcm_substream > *substream, > > return -EINVAL; > > } > > > > Except several warnings this patch generates, this is a bit surprising: > > > + spin_unlock_irqrestore(&priv->lock, flags); > > You have critical section which you now break into two. You cannot do this > usually. How the synchronization is now kept? > The actual reason behind breaking the critical section is to allow the use of already existing functions related to configuration of CDCLK and OPCLK source. Based on the review comments from previous patch-set related to the actual need of custom sound card driver, we did migrate to simple-card, where Exynos specific configurations like RCLKSRC selection, OPCLK and CDCLK configuration is not being handled by the simple-card. To overcome this scenario, fixups has been added during i2s_trigger to let Exynos users to configure the Exynos I2S specific dividers and mux. Rather than re-implementing the routines (or) configurations already Available in the driver, the fixup functions can call the i2s_set_sysclk and similar functions directly (which also takes the spin lock). But we noticed that fixup_late implemented for FSD may not require releasing of spinlock as it involves PSR configuration and will not cause any harm if it is still kept inside the existing critical section only. I'll update the patch to keep the fixup_late inside critical section and will post the same in the next patch set. > > + > > + if (priv->fixup_late) > > + priv->fixup_late(substream, dai); > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + > > if (capture) > > i2s_rxctrl(i2s, 1); > > else > > Best regards, > Krzysztof Thanks, Padmanabhan R.
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: 03 January 2023 11:39 PM > To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com> > Cc: lgirdwood@gmail.com; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com; > perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com; > alim.akhtar@samsung.com; rcsekar@samsung.com; > aswani.reddy@samsung.com; alsa-devel@alsa-project.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung- > soc@vger.kernel.org > Subject: Re: [PATCH v2 2/5] ASoC: samsung: i2s: add support for FSD I2S > > On Tue, Jan 03, 2023 at 10:26:10AM +0530, Padmanabhan Rajanbabu wrote: > > > +void fsd_i2s_fixup_early(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai) > > +{ > > + struct snd_soc_pcm_runtime *rtd = > asoc_substream_to_rtd(substream); > > + struct i2s_dai *i2s = to_info(asoc_rtd_to_cpu(rtd, 0)); > > + struct i2s_dai *other = get_other_dai(i2s); > > + > > + if (!is_opened(other)) { > > + i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK, 192, > SND_SOC_CLOCK_OUT); > > + i2s_set_sysclk(dai, SAMSUNG_I2S_OPCLK, 0, > MOD_OPCLK_PCLK); > > + } > > +} > > This looks like we're just hard coding to 192kHz? Not actually. The value 192 being passed is for the RFS divider based on which the Root clock source is divided to generate bit-clock and frame-clock in master mode. But, FSD SoC is utilizing the Exynos7-I2S controller in slave mode, where bit-clock and frame-clock is sourced by the codec. Therefore the sampling of data happens with codec clock source and not based on the clock source from RCLK. However, we still need RFS and BFS configured to default value for the proper operation of the controller. The current operation being performed above is to change the Codec clock direction to "out", so that codec will use this clock source to generate bit clock and frame clock from its own PLL. I'll make the changes in the next patch set to pass 0 instead of 192 here, so that RFS and BFS will be configured to default value in config_setup function. Thanks, Padmanabhan R.
diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h index b4b5d6053503..4444c857d0c0 100644 --- a/sound/soc/samsung/i2s-regs.h +++ b/sound/soc/samsung/i2s-regs.h @@ -132,6 +132,7 @@ #define EXYNOS7_MOD_RCLK_192FS 7 #define PSR_PSREN (1 << 15) +#define PSR_PSVAL(x) (((x - 1) << 8) & 0x3f00) #define FIC_TX2COUNT(x) (((x) >> 24) & 0xf) #define FIC_TX1COUNT(x) (((x) >> 16) & 0xf) diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index 9505200f3d11..dcb5c438cb2f 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -50,6 +50,10 @@ struct samsung_i2s_dai_data { u32 quirks; unsigned int pcm_rates; const struct samsung_i2s_variant_regs *i2s_variant_regs; + void (*fixup_early)(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai); + void (*fixup_late)(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai); }; struct i2s_dai { @@ -111,6 +115,10 @@ struct samsung_i2s_priv { u32 suspend_i2spsr; const struct samsung_i2s_variant_regs *variant_regs; + void (*fixup_early)(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai); + void (*fixup_late)(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai); u32 quirks; /* The clock provider's data */ @@ -940,6 +948,10 @@ static int i2s_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: pm_runtime_get_sync(dai->dev); + + if (priv->fixup_early) + priv->fixup_early(substream, dai); + spin_lock_irqsave(&priv->lock, flags); if (config_setup(i2s)) { @@ -947,6 +959,13 @@ static int i2s_trigger(struct snd_pcm_substream *substream, return -EINVAL; } + spin_unlock_irqrestore(&priv->lock, flags); + + if (priv->fixup_late) + priv->fixup_late(substream, dai); + + spin_lock_irqsave(&priv->lock, flags); + if (capture) i2s_rxctrl(i2s, 1); else @@ -1410,6 +1429,8 @@ static int samsung_i2s_probe(struct platform_device *pdev) if (np) { priv->quirks = i2s_dai_data->quirks; + priv->fixup_early = i2s_dai_data->fixup_early; + priv->fixup_late = i2s_dai_data->fixup_late; } else { if (!i2s_pdata) { dev_err(&pdev->dev, "Missing platform data\n"); @@ -1563,6 +1584,31 @@ static int samsung_i2s_remove(struct platform_device *pdev) return 0; } +void fsd_i2s_fixup_early(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct i2s_dai *i2s = to_info(asoc_rtd_to_cpu(rtd, 0)); + struct i2s_dai *other = get_other_dai(i2s); + + if (!is_opened(other)) { + i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK, 192, SND_SOC_CLOCK_OUT); + i2s_set_sysclk(dai, SAMSUNG_I2S_OPCLK, 0, MOD_OPCLK_PCLK); + } +} + +void fsd_i2s_fixup_late(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct samsung_i2s_priv *priv = snd_soc_dai_get_drvdata(dai); + struct i2s_dai *i2s = to_info(asoc_rtd_to_cpu(rtd, 0)); + struct i2s_dai *other = get_other_dai(i2s); + + if (!is_opened(other)) + writel(PSR_PSVAL(2) | PSR_PSREN, priv->addr + I2SPSR); +} + static const struct samsung_i2s_variant_regs i2sv3_regs = { .bfs_off = 1, .rfs_off = 3, @@ -1652,6 +1698,14 @@ static const struct samsung_i2s_dai_data i2sv5_dai_type_i2s1 __maybe_unused = { .i2s_variant_regs = &i2sv5_i2s1_regs, }; +static const struct samsung_i2s_dai_data fsd_dai_type __maybe_unused = { + .quirks = QUIRK_SEC_DAI | QUIRK_NEED_RSTCLR | QUIRK_SUPPORTS_TDM, + .pcm_rates = SNDRV_PCM_RATE_8000_192000, + .i2s_variant_regs = &i2sv7_regs, + .fixup_early = fsd_i2s_fixup_early, + .fixup_late = fsd_i2s_fixup_late, +}; + static const struct platform_device_id samsung_i2s_driver_ids[] = { { .name = "samsung-i2s", @@ -1678,6 +1732,9 @@ static const struct of_device_id exynos_i2s_match[] = { }, { .compatible = "samsung,exynos7-i2s1", .data = &i2sv5_dai_type_i2s1, + }, { + .compatible = "tesla,fsd-i2s", + .data = &fsd_dai_type, }, {}, };
Add support for enabling I2S controller on FSD platform. FSD I2S controller is based on Exynos7 I2S controller, supporting 2CH playback/capture in I2S mode and 7.1CH playback/capture in TDM mode. Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com> --- sound/soc/samsung/i2s-regs.h | 1 + sound/soc/samsung/i2s.c | 57 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)