[2/6] ASoC: samsung: i2s: configure PSR from sound card

  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(-)
  

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)?
  

> -----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.