[PATCH 1/2] TSD: arm64: dts: rockchip: use codec as clock master

[PATCH 1/2] TSD: arm64: dts: rockchip: use codec as clock master

[Jakob Unterwurzacher]

From: Ermin Sunj <ermin.sunj@theobroma-systems.com>

If the codec is not the clock master, the MCLK needs to be
synchronous to both I2S_SCL ans I2S_LRCLK. We do not have that
on Haikou, causing distorted audio.

Before:

 Running audioloopback.py script on Ringneck, 1kHz
 output sine wave is not stable and shows distortion.

After:

 10h stress tests audioloopback.py failed only one time.
 That is 0.00014% failure rate.

Signed-off-by: Ermin Sunj <ermin.sunj@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
index 3a447d03e2a8..dafeef0c2dab 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
@@ -68,8 +68,10 @@ i2s0-sound {
 		simple-audio-card,format = "i2s";
 		simple-audio-card,name = "Haikou,I2S-codec";
 		simple-audio-card,mclk-fs = <512>;
+		simple-audio-card,frame-master = <&sgtl5000_codec>;
+		simple-audio-card,bitclock-master = <&sgtl5000_codec>;
 
-		simple-audio-card,codec {
+		sgtl5000_codec: simple-audio-card,codec {
 			clocks = <&sgtl5000_clk>;
 			sound-dai = <&sgtl5000>;
 		};
-- 
2.39.2

[PATCH 2/2] TSD: arm64: dts: rockchip: set system-clock-fixed in sgtl5000_codec

[Jakob Unterwurzacher]

Having sgtl5000_clk defines as "fixed-clock" is not enough to prevent
the dai subsystem from overwriting the frequency via sgtl5000_set_dai_sysclk.

Setting system-clock-fixed does the job, and now a 1kHz sine wave
comes out as actually 1kHz, no matter the sample rate of the source.

Testcase: These should sound the same:

 speaker-test -r 48000 -t sine -f 1000
 speaker-test -r 24000 -t sine -f 1000

Also remove the clock link here as having it in sgtl5000 and
sgtl5000_codec causes duplicate clock unprepares with associated
backtrace.
---
 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
index dafeef0c2dab..2cad02096271 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
@@ -72,8 +72,10 @@ i2s0-sound {
 		simple-audio-card,bitclock-master = <&sgtl5000_codec>;
 
 		sgtl5000_codec: simple-audio-card,codec {
-			clocks = <&sgtl5000_clk>;
 			sound-dai = <&sgtl5000>;
+			// Prevent the dai subsystem from overwriting the clock
+			// frequency. We are using a fixed-frequency oscillator.
+			system-clock-fixed;
 		};
 
 		simple-audio-card,cpu {
-- 
2.39.2

Re: [PATCH 1/2] TSD: arm64: dts: rockchip: use codec as clock master

[Heiko Stübner]

Hi Jakob,

the change itself looks ok, but there are some logistic changes needed.

For one, please drop the local TSD prefix from patches submitted upstream.
The patch subject should be something like

	arm64: dts: rockchip: use codec as clock master on px30-ringneck

Am Mittwoch, 23. August 2023, 14:19:59 CEST schrieb Jakob Unterwurzacher:
> From: Ermin Sunj <ermin.sunj@theobroma-systems.com>
> 
> If the codec is not the clock master, the MCLK needs to be
> synchronous to both I2S_SCL ans I2S_LRCLK. We do not have that
> on Haikou, causing distorted audio.
> 
> Before:
> 
>  Running audioloopback.py script on Ringneck, 1kHz
>  output sine wave is not stable and shows distortion.
> 
> After:
> 
>  10h stress tests audioloopback.py failed only one time.
>  That is 0.00014% failure rate.
> 
> Signed-off-by: Ermin Sunj <ermin.sunj@theobroma-systems.com>

As sender of the patch you need to add another Signed-off-by line
of your own. With this you indicate that you were allowed to submit
the patch. So this needs two SOB-lines, one for Ermin as the original
author and one for you as the submitter.

Probably same for the second patch.


Thanks
Heiko

> ---
>  arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
> index 3a447d03e2a8..dafeef0c2dab 100644
> --- a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
> +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
> @@ -68,8 +68,10 @@ i2s0-sound {
>  		simple-audio-card,format = "i2s";
>  		simple-audio-card,name = "Haikou,I2S-codec";
>  		simple-audio-card,mclk-fs = <512>;
> +		simple-audio-card,frame-master = <&sgtl5000_codec>;
> +		simple-audio-card,bitclock-master = <&sgtl5000_codec>;
>  
> -		simple-audio-card,codec {
> +		sgtl5000_codec: simple-audio-card,codec {
>  			clocks = <&sgtl5000_clk>;
>  			sound-dai = <&sgtl5000>;
>  		};
> 

Re: [PATCH 1/2] TSD: arm64: dts: rockchip: use codec as clock master

[Krzysztof Kozlowski]

On 23/08/2023 14:19, Jakob Unterwurzacher wrote:
> From: Ermin Sunj <ermin.sunj@theobroma-systems.com>
> 
> If the codec is not the clock master, the MCLK needs to be
> synchronous to both I2S_SCL ans I2S_LRCLK. We do not have that
> on Haikou, causing distorted audio.
> 
> Before:
> 
>  Running audioloopback.py script on Ringneck, 1kHz
>  output sine wave is not stable and shows distortion.
> 
> After:
> 
>  10h stress tests audioloopback.py failed only one time.
>  That is 0.00014% failure rate.

What is TSD? Why it is in the subject prefix?

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Best regards,
Krzysztof

Re: [PATCH 1/2] TSD: arm64: dts: rockchip: use codec as clock master

[Jakob Unterwurzacher]

On 25.08.23 08:49, Krzysztof Kozlowski wrote:
> 
> What is TSD? Why it is in the subject prefix?
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof, sorry about that, Heiko commented similarily.

TSD means "Theobroma Systems Design" but it should not have been used in 
the patch, v2 fixes that and also gets the SOB lines in order:

https://lore.kernel.org/lkml/20230823131651.586934-1-jakob.unterwurzacher@theobroma-systems.com/T/

Thanks,
Jakob