| Message ID | 20221014102151.108539-3-p.rajanbabu@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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: 14 October 2022 05:32 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 14, 2022 at 03:51:47PM +0530, Padmanabhan Rajanbabu wrote: > > 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 > > It's not clear to me why the solution here is to move the configuration to the > sound card rather than to improve the I2S driver to be able to cope with > whatever the restrictions are on the PSR in these systems - it seems more > cumbersome for system integrators, especially since you've not documented > the issues or how to configure it. Could you expand on what the constraints > are here and why it's not possible for the driver to figure things out (given > some quirk information)? Thank you for reviewing the patch. In Samsung I2S CPU controller, to derive the frame clock, we are supposed to configure the PSR and RFS internal dividers. i.e. OPCLK -> PSR -> RCLK -> RFS -> Frame clock Where: OPCLK - Operational clock PSR - Operational clock prescaler RCLK - Root Clock (derived from OPCLK based on PSR) RFS - Root frequency selection (divider) Frame clock - Sample frequency (derived from RCLK based on RFS) Ultimately, PSR = OPCLK / Frame clock / RFS Unlike other platforms utilizing Samsung CPU DAI, FSD SoC has a limitation on operational clock, where the clock frequency is fixed (66 MHz) and cannot be modified. Assuming that an userspace application wants perform playback @44100 Hz and assuming that RFS divider value is configured as 256, the PSR value will yield to 66 MHz / 44.1 KHz / 256 = 5 However if HW uses PSR = 5 to derive the frame clock from operational clock, then RCLK = OPCLK / PSR = 66 MHz / 5 = 13.2 MHz Frame clock = RCLK / RFS = 13.2 MHz / 256 = 51562 Hz The actual frame clock derived based on PSR is now different from what user application has intended. The situation did not improve even if the RFS is swept throughout the entire valid range. We can overcome this scenario to an extent if we can get a flexibility to Configure both PSR as well as RFS. i.e. to achieve frame clock of 44100 Hz, if PSR = 23 and RFS = 64 then frame clock = 66 MHz / 23 / 64 = 44836 Hz Although the sample frequency is not precise, it is very much closer to the Intended frequency, when compared to that of the existing solution. Since this scenario is specific to FSD SoC and has no changes in the Samsung I2S CPU DAI, the configuration is being done from the sound card of FSD SoC during hw_params. Please let me know if you think this scenario can be approached in any other way possible, rather than configuring 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?
On 21/10/2022 04:00, Padmanabhan Rajanbabu wrote: >> It's not clear to me why the solution here is to move the configuration to > the >> sound card rather than to improve the I2S driver to be able to cope with >> whatever the restrictions are on the PSR in these systems - it seems more >> cumbersome for system integrators, especially since you've not documented >> the issues or how to configure it. Could you expand on what the > constraints >> are here and why it's not possible for the driver to figure things out > (given >> some quirk information)? > > Thank you for reviewing the patch. > > In Samsung I2S CPU controller, to derive the frame clock, we are supposed to > configure the PSR and RFS internal dividers. i.e. > > OPCLK -> PSR -> RCLK -> RFS -> Frame clock > > Where: > OPCLK - Operational clock > PSR - Operational clock prescaler > RCLK - Root Clock (derived from OPCLK based on PSR) > RFS - Root frequency selection (divider) > Frame clock - Sample frequency (derived from RCLK based on RFS) > > Ultimately, > > PSR = OPCLK / Frame clock / RFS > > Unlike other platforms utilizing Samsung CPU DAI, FSD SoC has a limitation > on > operational clock, where the clock frequency is fixed (66 MHz) and cannot be > modified. > > Assuming that an userspace application wants perform playback @44100 Hz > and assuming that RFS divider value is configured as 256, the PSR value will > yield to > > 66 MHz / 44.1 KHz / 256 = 5 > > However if HW uses PSR = 5 to derive the frame clock from operational clock, > then > > RCLK = OPCLK / PSR = 66 MHz / 5 = 13.2 MHz > Frame clock = RCLK / RFS = 13.2 MHz / 256 = 51562 Hz > > The actual frame clock derived based on PSR is now different from what user > application has intended. The situation did not improve even if the RFS is > swept throughout the entire valid range. > > We can overcome this scenario to an extent if we can get a flexibility to > Configure both PSR as well as RFS. > > i.e. to achieve frame clock of 44100 Hz, if PSR = 23 and RFS = 64 > then frame clock = 66 MHz / 23 / 64 = 44836 Hz > > Although the sample frequency is not precise, it is very much closer to the > Intended frequency, when compared to that of the existing solution. Since > this > scenario is specific to FSD SoC and has no changes in the Samsung I2S CPU > DAI, > the configuration is being done from the sound card of FSD SoC during > hw_params. > > Please let me know if you think this scenario can be approached in any other > way possible, rather than configuring from sound card. Entire new driver for this, instead of improving existing Samsung drivers... no, it is no the way. If you followed this approach you would send 20 drivers for each "specific" quirk difference. Best regards, Krzysztof
> -----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?
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: 09 November 2022 11:09 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 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? Okay, I can understand the reason for de-linking these changes from the machine Driver. I'll post the v2 patches integrating the PSR changes into cpu dai driver. Thanks, Padmanabhan R.
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(-)