| Message ID | 20221014102151.108539-3-p.rajanbabu@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] ASoC: samsung: i2s: TDM Support for CPU DAI driver | expand |
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: 21 October 2022 05:24 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 2/6] ASoC: samsung: i2s: configure PSR from sound card > > On Fri, Oct 21, 2022 at 01:30:25PM +0530, Padmanabhan Rajanbabu wrote: > > > We can overcome this scenario to an extent if we can get a flexibility > > to Configure both PSR as well as RFS. > > Why does it make sense for the machine driver to worry about this rather > than having the I2S controller driver configure the clock tree? ____________________________________________ _____ | __ | | | | | \ | |CMU| | | \ | |FSD |- |---|-|--------->| \ _________ _________ | |___ | | | |op_clk0| | | | | | | | | | |MUX|----| PSR |----| RFS |--cdclk | | | | | | |_______| |_______| | | | |--------->| / | | | op_clk1 | / | | | |_ / | | |___________________________________________| | |-----> To other FSD SoC Peripherals In FSD I2S, the clock source is not an independent source but a common clock source being shared by many IPs in the same domain. Changing the clock tree will impact other IPs in the domain as they are dependent on the same source for functionality. We can understand your point to bring the PSR changes under the I2S CPU DAI driver by adding a separate compatible and data for the FSD SoC. But If we take the example of existing sound cards such as sound/soc/samsung/tm2_wm5110.c, the op_clk is supplied via external audio pll to the controller and PLL configuration is taken care by the sound card. Since the configuration of PLL is more specific to the tm2 platform, it makes use of the flexibility of changing the RFS and BFS using the sysclk and clkdiv hooks provided by exynos7-i2s CPU DAI along with PLL tuning for precise sampling frequency. Similar to the above example, the choice of clock source under discussion is not a limitation of exynos7-i2s controller, but instead is a limitation on the FSD SoC. By using the proposed change, we can ensure that the exynos CPU DAI driver is giving additional hooks similar to existing hooks for BFS, RFS and CDCLK direction so that sound cards can use snd_soc_dai_set_sysclk and snd_soc_dai_set_clkdiv to customize the same. An alternative approach is to use the cpu dai as bit clock and frame clock consumer (i.e. in slave mode) so that codec can supply the MCLK to FSD for playback and capture. But this will deviate from the actual usecase for FSD SoC, where the CPU DAI is intended to function as master.
On Tue, Nov 08, 2022 at 10:53:40AM +0530, Padmanabhan Rajanbabu wrote: > > > We can overcome this scenario to an extent if we can get a flexibility > > > to Configure both PSR as well as RFS. > > Why does it make sense for the machine driver to worry about this rather > > than having the I2S controller driver configure the clock tree? > _____ | __ > | > | | | | \ > | > |CMU| | | \ > | > |FSD |- |---|-|--------->| \ _________ _________ > | > |___ | | | |op_clk0| | | | | > | | > | | | |MUX|----| PSR |----| RFS > |--cdclk | > | | | | | |_______| |_______| > | > | | |--------->| / > | > | | op_clk1 | / > | > | | |_ / > | > | |___________________________________________| > | > |-----> To other FSD SoC Peripherals > In FSD I2S, the clock source is not an independent source but a common clock > source being shared by many IPs in the same domain. > Changing the clock tree will impact other IPs in the domain as they are > dependent on the same source for functionality. I'm not sure I follow. Perhaps your diagram is unclear but it looks like PSR and RFS are both after a mux which appears to select which clock is going to be used by the I2S controller? The usage by other clocks appears to be upstream of the mux and dividers. > We can understand your point to bring the PSR changes under the I2S CPU DAI > driver by adding a separate compatible and data for the FSD SoC. But If we > take > the example of existing sound cards such as sound/soc/samsung/tm2_wm5110.c, > the op_clk is supplied via external audio pll to the controller and PLL > configuration > is taken care by the sound card. Since the configuration of PLL is more > specific to > the tm2 platform, it makes use of the flexibility of changing the RFS and > BFS > using the sysclk and clkdiv hooks provided by exynos7-i2s CPU DAI along with > PLL tuning for precise sampling frequency. The big reason for the clocking control (and indeed having a custom machine driver) with the WM5110 is that it has multiple clocks to control and a good deal of flexibility with placing them in clock domains and so on which have power and performance impacts. It's frankly a bit unclear to me if the CPU I2S controller even needs the bitclock configuring given that the clocks are being driven by the CODEC there, but regardless it's not clear to me why the I2S controller would need anything other than the input clock to the block configuring? > Similar to the above example, the choice of clock source under discussion is > not a > limitation of exynos7-i2s controller, but instead is a limitation on the FSD > SoC. > By using the proposed change, we can ensure that the exynos CPU DAI driver > is > giving additional hooks similar to existing hooks for BFS, RFS and CDCLK > direction > so that sound cards can use snd_soc_dai_set_sysclk and > snd_soc_dai_set_clkdiv > to customize the same. I'm still not seeing anything that articulates why pushing the configuration of the dividers within the block into the machine driver solves a problem here. Again, what's the upside to configuring clocks that are purely within the block?
diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h index cb2be4a3b70b..e2581dc73df2 100644 --- a/sound/soc/samsung/i2s-regs.h +++ b/sound/soc/samsung/i2s-regs.h @@ -132,6 +132,8 @@ #define EXYNOS7_MOD_RCLK_192FS 7 #define PSR_PSREN (1 << 15) +#define PSR_PSVAL_SHIFT 8 +#define PSR_PSVAL_MASK 0x3f #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 fb806b0af6ab..a96286b27f1d 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -59,10 +59,10 @@ struct i2s_dai { /* Frame clock */ unsigned frmclk; /* - * Specifically requested RCLK, BCLK by machine driver. + * Specifically requested RCLK, BCLK and PSR by machine driver. * 0 indicates CPU driver is free to choose any value. */ - unsigned rfs, bfs; + unsigned int rfs, bfs, psr; /* Pointer to the Primary_Fifo if this is Sec_Fifo, NULL otherwise */ struct i2s_dai *pri_dai; /* Pointer to the Secondary_Fifo if it has one, NULL otherwise */ @@ -389,6 +389,17 @@ static inline int get_blc(struct i2s_dai *i2s) } } +static inline unsigned int get_psval(struct i2s_dai *i2s) +{ + struct samsung_i2s_priv *priv = i2s->priv; + u32 psr; + + psr = readl(priv->addr + I2SPSR) >> PSR_PSVAL_SHIFT; + psr &= PSR_PSVAL_MASK; + + return (psr + 1); +} + /* TX channel control */ static void i2s_txctrl(struct i2s_dai *i2s, int on) { @@ -994,7 +1005,11 @@ static int config_setup(struct i2s_dai *i2s) return 0; if (!(priv->quirks & QUIRK_NO_MUXPSR)) { - psr = priv->rclk_srcrate / i2s->frmclk / rfs; + if (i2s->psr) + psr = i2s->psr; + else + psr = priv->rclk_srcrate / i2s->frmclk / rfs; + writel(((psr - 1) << 8) | PSR_PSREN, priv->addr + I2SPSR); dev_dbg(&i2s->pdev->dev, "RCLK_SRC=%luHz PSR=%u, RCLK=%dfs, BCLK=%dfs\n", @@ -1072,6 +1087,18 @@ static int i2s_set_clkdiv(struct snd_soc_dai *dai, i2s->bfs = div; pm_runtime_put(dai->dev); break; + case SAMSUNG_I2S_DIV_RCLK: + pm_runtime_get_sync(dai->dev); + if ((any_active(i2s) && div && (get_psval(i2s) != div)) + || (other && other->psr && (other->psr != div))) { + pm_runtime_put(dai->dev); + dev_err(&i2s->pdev->dev, + "%s:%d Other DAI busy\n", __func__, __LINE__); + return -EAGAIN; + } + i2s->psr = div; + pm_runtime_put(dai->dev); + break; default: dev_err(&i2s->pdev->dev, "Invalid clock divider(%d)\n", div_id); @@ -1140,9 +1167,10 @@ static int samsung_i2s_dai_probe(struct snd_soc_dai *dai) other->idma_playback.addr); } - /* Reset any constraint on RFS and BFS */ + /* Reset any constraint on RFS, BFS and PSR*/ i2s->rfs = 0; i2s->bfs = 0; + i2s->psr = 0; spin_lock_irqsave(&priv->lock, flags); i2s_txctrl(i2s, 0); diff --git a/sound/soc/samsung/i2s.h b/sound/soc/samsung/i2s.h index 78b475ef98d9..e783d33fdfac 100644 --- a/sound/soc/samsung/i2s.h +++ b/sound/soc/samsung/i2s.h @@ -13,6 +13,7 @@ #define SAMSUNG_I2S_DAI_SEC "samsung-i2s-sec" #define SAMSUNG_I2S_DIV_BCLK 1 +#define SAMSUNG_I2S_DIV_RCLK 2 #define SAMSUNG_I2S_RCLKSRC_0 0 #define SAMSUNG_I2S_RCLKSRC_1 1
Currently the prescaler value in samsung I2S dai is calculated by dividing the peripheral input clock frequency with frame clock frequency and root clock frequency divider. This prescaler value is used to divide the input clock to generate root clock (RCLK) from which frame clock is generated for I2S communication. However for the platforms which does not have a dedicated audio PLL as an input clock source, the prescaler divider will not generate accurate root clock frequency, which inturn affects sampling frequency also. To overcome this scenario, support has been added to let the sound card identify right prescaler divider value and configure the prescaler (PSR) divider directly the from the sound card to achieve near accurate sample frequencies Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com> --- sound/soc/samsung/i2s-regs.h | 2 ++ sound/soc/samsung/i2s.c | 36 ++++++++++++++++++++++++++++++++---- sound/soc/samsung/i2s.h | 1 + 3 files changed, 35 insertions(+), 4 deletions(-)