[PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Martin Blumenstingl]

Christian reports that 48kHz audio does not work on his WeTek Play 2
(which uses a GXBB SoC), while 44.1kHz audio works fine on the same
board. He also reports that 48kHz audio works on GXL and GXM SoCs,
which are using an (almost) identical AIU (audio controller).

Experimenting has shown that MPLL0 is causing this problem. In the .dts
we have by default:
	assigned-clocks = <&clkc CLKID_MPLL0>,
			  <&clkc CLKID_MPLL1>,
			  <&clkc CLKID_MPLL2>;
	assigned-clock-rates = <294912000>,
			       <270950400>,
			       <393216000>;
The MPLL0 rate is divisible by 48kHz without remainder and the MPLL1
rate is divisible by 44.1kHz without remainder. Swapping these two clock
rates "fixes" 48kHz audio but breaks 44.1kHz audio.

Everything looks normal when looking at the info provided by the common
clock framework while playing 48kHz audio (via I2S with mclk-fs = 256):
        mpll_prediv                 1        1        0  2000000000
           mpll0_div                1        1        0   294909641
              mpll0                 1        1        0   294909641
                 cts_amclk_sel       1        1        0   294909641
                    cts_amclk_div       1        1        0    12287902
                       cts_amclk       1        1        0    12287902

meson-clk-msr however shows that the actual MPLL0 clock is off by more
than 38MHz:
        mp0_out               333322917    +/-10416Hz

The 3.14 vendor kernel uses the following code to enable SSEN only for
MPLL0 (where con_reg2 is HHI_MPLL_CNTL and SSEN_shift is 25):
	if (strncmp(hw->clk->name, "mpll_clk_out0", 13) == 0) {
		val = readl(mpll->con_reg2);
		val |= 1 <<  mpll->SSEN_shift;
		writel(val, mpll->con_reg2);
	}

Add the SSEN (spread spectrum enable) bit and add the
CLK_MESON_MPLL_SPREAD_SPECTRUM flag to enable this bit for MPLL0. Do
this for GXBB *only* since GXL doesn't seem to care if this bit is set
or not, meaning that meson-clk-msr always sees (approximately) the same
frequency as common clock framework.

Fixes: 8925dbd03bb29b ("clk: meson: gxbb: no spread spectrum on mpll")
Reported-by: Christian Hewitt <christianshewitt@gmail.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/gxbb.c | 50 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
index d6eed760327d..673bc915c7d9 100644
--- a/drivers/clk/meson/gxbb.c
+++ b/drivers/clk/meson/gxbb.c
@@ -713,6 +713,41 @@ static struct clk_regmap gxbb_mpll_prediv = {
 };
 
 static struct clk_regmap gxbb_mpll0_div = {
+	.data = &(struct meson_clk_mpll_data){
+		.sdm = {
+			.reg_off = HHI_MPLL_CNTL7,
+			.shift   = 0,
+			.width   = 14,
+		},
+		.sdm_en = {
+			.reg_off = HHI_MPLL_CNTL7,
+			.shift   = 15,
+			.width	 = 1,
+		},
+		.n2 = {
+			.reg_off = HHI_MPLL_CNTL7,
+			.shift   = 16,
+			.width   = 9,
+		},
+		.ssen = {
+			.reg_off = HHI_MPLL_CNTL,
+			.shift   = 25,
+			.width   = 1,
+		},
+		.flags = CLK_MESON_MPLL_SPREAD_SPECTRUM,
+		.lock = &meson_clk_lock,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "mpll0_div",
+		.ops = &meson_clk_mpll_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&gxbb_mpll_prediv.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap gxl_mpll0_div = {
 	.data = &(struct meson_clk_mpll_data){
 		.sdm = {
 			.reg_off = HHI_MPLL_CNTL7,
@@ -749,7 +784,16 @@ static struct clk_regmap gxbb_mpll0 = {
 	.hw.init = &(struct clk_init_data){
 		.name = "mpll0",
 		.ops = &clk_regmap_gate_ops,
-		.parent_hws = (const struct clk_hw *[]) { &gxbb_mpll0_div.hw },
+		.parent_data = &(const struct clk_parent_data) {
+			/*
+			 * Note:
+			 * GXL and GXBB have different SSEN requirements. We
+			 * fallback to the global naming string mechanism so
+			 * mpll0_div picks up the appropriate one.
+			 */
+			.name = "mpll0_div",
+			.index = -1,
+		},
 		.num_parents = 1,
 		.flags = CLK_SET_RATE_PARENT,
 	},
@@ -3044,7 +3088,7 @@ static struct clk_hw_onecell_data gxl_hw_onecell_data = {
 		[CLKID_VAPB_1]		    = &gxbb_vapb_1.hw,
 		[CLKID_VAPB_SEL]	    = &gxbb_vapb_sel.hw,
 		[CLKID_VAPB]		    = &gxbb_vapb.hw,
-		[CLKID_MPLL0_DIV]	    = &gxbb_mpll0_div.hw,
+		[CLKID_MPLL0_DIV]	    = &gxl_mpll0_div.hw,
 		[CLKID_MPLL1_DIV]	    = &gxbb_mpll1_div.hw,
 		[CLKID_MPLL2_DIV]	    = &gxbb_mpll2_div.hw,
 		[CLKID_MPLL_PREDIV]	    = &gxbb_mpll_prediv.hw,
@@ -3439,7 +3483,7 @@ static struct clk_regmap *const gxl_clk_regmaps[] = {
 	&gxbb_mpll0,
 	&gxbb_mpll1,
 	&gxbb_mpll2,
-	&gxbb_mpll0_div,
+	&gxl_mpll0_div,
 	&gxbb_mpll1_div,
 	&gxbb_mpll2_div,
 	&gxbb_cts_amclk_div,
-- 
2.33.1

Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Christian Hewitt]

> On 16 Oct 2021, at 6:59 pm, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> 
> Christian reports that 48kHz audio does not work on his WeTek Play 2
> (which uses a GXBB SoC), while 44.1kHz audio works fine on the same
> board. He also reports that 48kHz audio works on GXL and GXM SoCs,
> which are using an (almost) identical AIU (audio controller).
> 
> Experimenting has shown that MPLL0 is causing this problem. In the .dts
> we have by default:
> 	assigned-clocks = <&clkc CLKID_MPLL0>,
> 			  <&clkc CLKID_MPLL1>,
> 			  <&clkc CLKID_MPLL2>;
> 	assigned-clock-rates = <294912000>,
> 			       <270950400>,
> 			       <393216000>;
> The MPLL0 rate is divisible by 48kHz without remainder and the MPLL1
> rate is divisible by 44.1kHz without remainder. Swapping these two clock
> rates "fixes" 48kHz audio but breaks 44.1kHz audio.
> 
> Everything looks normal when looking at the info provided by the common
> clock framework while playing 48kHz audio (via I2S with mclk-fs = 256):
>        mpll_prediv                 1        1        0  2000000000
>           mpll0_div                1        1        0   294909641
>              mpll0                 1        1        0   294909641
>                 cts_amclk_sel       1        1        0   294909641
>                    cts_amclk_div       1        1        0    12287902
>                       cts_amclk       1        1        0    12287902
> 
> meson-clk-msr however shows that the actual MPLL0 clock is off by more
> than 38MHz:
>        mp0_out               333322917    +/-10416Hz
> 
> The 3.14 vendor kernel uses the following code to enable SSEN only for
> MPLL0 (where con_reg2 is HHI_MPLL_CNTL and SSEN_shift is 25):
> 	if (strncmp(hw->clk->name, "mpll_clk_out0", 13) == 0) {
> 		val = readl(mpll->con_reg2);
> 		val |= 1 <<  mpll->SSEN_shift;
> 		writel(val, mpll->con_reg2);
> 	}
> 
> Add the SSEN (spread spectrum enable) bit and add the
> CLK_MESON_MPLL_SPREAD_SPECTRUM flag to enable this bit for MPLL0. Do
> this for GXBB *only* since GXL doesn't seem to care if this bit is set
> or not, meaning that meson-clk-msr always sees (approximately) the same
> frequency as common clock framework.
> 
> Fixes: 8925dbd03bb29b ("clk: meson: gxbb: no spread spectrum on mpll")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Tested with a selection of different media at 41KHz and 48KHz on GXBB
(WeTek Play2 and Hub) and GXL (LePotato).

Tested-by: Christian Hewitt <christianshewitt@gmail.com>

> ---
> drivers/clk/meson/gxbb.c | 50 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
> index d6eed760327d..673bc915c7d9 100644
> --- a/drivers/clk/meson/gxbb.c
> +++ b/drivers/clk/meson/gxbb.c
> @@ -713,6 +713,41 @@ static struct clk_regmap gxbb_mpll_prediv = {
> };
> 
> static struct clk_regmap gxbb_mpll0_div = {
> +	.data = &(struct meson_clk_mpll_data){
> +		.sdm = {
> +			.reg_off = HHI_MPLL_CNTL7,
> +			.shift   = 0,
> +			.width   = 14,
> +		},
> +		.sdm_en = {
> +			.reg_off = HHI_MPLL_CNTL7,
> +			.shift   = 15,
> +			.width	 = 1,
> +		},
> +		.n2 = {
> +			.reg_off = HHI_MPLL_CNTL7,
> +			.shift   = 16,
> +			.width   = 9,
> +		},
> +		.ssen = {
> +			.reg_off = HHI_MPLL_CNTL,
> +			.shift   = 25,
> +			.width   = 1,
> +		},
> +		.flags = CLK_MESON_MPLL_SPREAD_SPECTRUM,
> +		.lock = &meson_clk_lock,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "mpll0_div",
> +		.ops = &meson_clk_mpll_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&gxbb_mpll_prediv.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap gxl_mpll0_div = {
> 	.data = &(struct meson_clk_mpll_data){
> 		.sdm = {
> 			.reg_off = HHI_MPLL_CNTL7,
> @@ -749,7 +784,16 @@ static struct clk_regmap gxbb_mpll0 = {
> 	.hw.init = &(struct clk_init_data){
> 		.name = "mpll0",
> 		.ops = &clk_regmap_gate_ops,
> -		.parent_hws = (const struct clk_hw *[]) { &gxbb_mpll0_div.hw },
> +		.parent_data = &(const struct clk_parent_data) {
> +			/*
> +			 * Note:
> +			 * GXL and GXBB have different SSEN requirements. We
> +			 * fallback to the global naming string mechanism so
> +			 * mpll0_div picks up the appropriate one.
> +			 */
> +			.name = "mpll0_div",
> +			.index = -1,
> +		},
> 		.num_parents = 1,
> 		.flags = CLK_SET_RATE_PARENT,
> 	},
> @@ -3044,7 +3088,7 @@ static struct clk_hw_onecell_data gxl_hw_onecell_data = {
> 		[CLKID_VAPB_1]		    = &gxbb_vapb_1.hw,
> 		[CLKID_VAPB_SEL]	    = &gxbb_vapb_sel.hw,
> 		[CLKID_VAPB]		    = &gxbb_vapb.hw,
> -		[CLKID_MPLL0_DIV]	    = &gxbb_mpll0_div.hw,
> +		[CLKID_MPLL0_DIV]	    = &gxl_mpll0_div.hw,
> 		[CLKID_MPLL1_DIV]	    = &gxbb_mpll1_div.hw,
> 		[CLKID_MPLL2_DIV]	    = &gxbb_mpll2_div.hw,
> 		[CLKID_MPLL_PREDIV]	    = &gxbb_mpll_prediv.hw,
> @@ -3439,7 +3483,7 @@ static struct clk_regmap *const gxl_clk_regmaps[] = {
> 	&gxbb_mpll0,
> 	&gxbb_mpll1,
> 	&gxbb_mpll2,
> -	&gxbb_mpll0_div,
> +	&gxl_mpll0_div,
> 	&gxbb_mpll1_div,
> 	&gxbb_mpll2_div,
> 	&gxbb_cts_amclk_div,
> -- 
> 2.33.1
> 

Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Jerome Brunet]

On Sat 16 Oct 2021 at 16:59, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Christian reports that 48kHz audio does not work on his WeTek Play 2
> (which uses a GXBB SoC), while 44.1kHz audio works fine on the same
> board. He also reports that 48kHz audio works on GXL and GXM SoCs,
> which are using an (almost) identical AIU (audio controller).

The above is a bit "personal" - it is not great fit for the commit
description. Please rephrase or put it in comment section bellow

>
> Experimenting has shown that MPLL0 is causing this problem. In the .dts
> we have by default:
> 	assigned-clocks = <&clkc CLKID_MPLL0>,
> 			  <&clkc CLKID_MPLL1>,
> 			  <&clkc CLKID_MPLL2>;
> 	assigned-clock-rates = <294912000>,
> 			       <270950400>,
> 			       <393216000>;
> The MPLL0 rate is divisible by 48kHz without remainder and the MPLL1
> rate is divisible by 44.1kHz without remainder. Swapping these two clock
> rates "fixes" 48kHz audio but breaks 44.1kHz audio.
>
> Everything looks normal when looking at the info provided by the common
> clock framework while playing 48kHz audio (via I2S with mclk-fs = 256):
>         mpll_prediv                 1        1        0  2000000000
>            mpll0_div                1        1        0   294909641
>               mpll0                 1        1        0   294909641
>                  cts_amclk_sel       1        1        0   294909641
>                     cts_amclk_div       1        1        0    12287902
>                        cts_amclk       1        1        0    12287902
>
> meson-clk-msr however shows that the actual MPLL0 clock is off by more
> than 38MHz:
>         mp0_out               333322917    +/-10416Hz
>
> The 3.14 vendor kernel uses the following code to enable SSEN only for
> MPLL0 (where con_reg2 is HHI_MPLL_CNTL and SSEN_shift is 25):
> 	if (strncmp(hw->clk->name, "mpll_clk_out0", 13) == 0) {
> 		val = readl(mpll->con_reg2);
> 		val |= 1 <<  mpll->SSEN_shift;
> 		writel(val, mpll->con_reg2);
> 	}
>
> Add the SSEN (spread spectrum enable) bit and add the
> CLK_MESON_MPLL_SPREAD_SPECTRUM flag to enable this bit for MPLL0. Do
> this for GXBB *only* since GXL doesn't seem to care if this bit is set
> or not, meaning that meson-clk-msr always sees (approximately) the same
> frequency as common clock framework.

 1 - it is odd that we need to poke a bit in the register related to the
 fixed PLL but ok ...
 2 - 3.14 does yes, 4.9 does not soooo ... no real proof there
 3 - That is the most important to me: the effect you described clearly is
 not spread spectrum.

Spread spectrum varies the frequencies quickly, IOW it makes the
frequencies unstable. Some stuff do not need a particularly stable rate
and it can help with EM compatibility. This is not desirable for audio.

So 2 things:
 - If this bit really enables spread spectrum on MPLL0 (or worse, the
 Fixed PLL) - checking clk measure is not enough. It is just a mean of
 the rate seen by the SoC itself. You would not see the effect of the
 spread spectrum here ... you need to capture the clock output with a
 scope for that.

 - Or the bit is incorrectly documented (or DDS0_SSEN does not mean
 spread spectrum). If it is not a spread spectrum function, then this
 patch seems to indicate it is and it is misleading.

Either way, I'm not OK with it.

To me, the rate drop that happens when you flip this bit looks more like
the effect SDM_EN should have.

Could you check the internal values (n2 and sdm) compare this to the
output rate you actually get ? see if this leads to anything ? does
SDM_EN really has an effect on this MPLL ? it is a combination of both ?

>
> Fixes: 8925dbd03bb29b ("clk: meson: gxbb: no spread spectrum on mpll")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/gxbb.c | 50 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
> index d6eed760327d..673bc915c7d9 100644
> --- a/drivers/clk/meson/gxbb.c
> +++ b/drivers/clk/meson/gxbb.c
> @@ -713,6 +713,41 @@ static struct clk_regmap gxbb_mpll_prediv = {
>  };
>  
>  static struct clk_regmap gxbb_mpll0_div = {
> +	.data = &(struct meson_clk_mpll_data){
> +		.sdm = {
> +			.reg_off = HHI_MPLL_CNTL7,
> +			.shift   = 0,
> +			.width   = 14,
> +		},
> +		.sdm_en = {
> +			.reg_off = HHI_MPLL_CNTL7,
> +			.shift   = 15,
> +			.width	 = 1,
> +		},
> +		.n2 = {
> +			.reg_off = HHI_MPLL_CNTL7,
> +			.shift   = 16,
> +			.width   = 9,
> +		},
> +		.ssen = {
> +			.reg_off = HHI_MPLL_CNTL,
> +			.shift   = 25,
> +			.width   = 1,
> +		},
> +		.flags = CLK_MESON_MPLL_SPREAD_SPECTRUM,
> +		.lock = &meson_clk_lock,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "mpll0_div",
> +		.ops = &meson_clk_mpll_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&gxbb_mpll_prediv.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap gxl_mpll0_div = {
>  	.data = &(struct meson_clk_mpll_data){
>  		.sdm = {
>  			.reg_off = HHI_MPLL_CNTL7,
> @@ -749,7 +784,16 @@ static struct clk_regmap gxbb_mpll0 = {
>  	.hw.init = &(struct clk_init_data){
>  		.name = "mpll0",
>  		.ops = &clk_regmap_gate_ops,
> -		.parent_hws = (const struct clk_hw *[]) { &gxbb_mpll0_div.hw },
> +		.parent_data = &(const struct clk_parent_data) {
> +			/*
> +			 * Note:
> +			 * GXL and GXBB have different SSEN requirements. We
> +			 * fallback to the global naming string mechanism so
> +			 * mpll0_div picks up the appropriate one.
> +			 */
> +			.name = "mpll0_div",
> +			.index = -1,
> +		},
>  		.num_parents = 1,
>  		.flags = CLK_SET_RATE_PARENT,
>  	},
> @@ -3044,7 +3088,7 @@ static struct clk_hw_onecell_data gxl_hw_onecell_data = {
>  		[CLKID_VAPB_1]		    = &gxbb_vapb_1.hw,
>  		[CLKID_VAPB_SEL]	    = &gxbb_vapb_sel.hw,
>  		[CLKID_VAPB]		    = &gxbb_vapb.hw,
> -		[CLKID_MPLL0_DIV]	    = &gxbb_mpll0_div.hw,
> +		[CLKID_MPLL0_DIV]	    = &gxl_mpll0_div.hw,
>  		[CLKID_MPLL1_DIV]	    = &gxbb_mpll1_div.hw,
>  		[CLKID_MPLL2_DIV]	    = &gxbb_mpll2_div.hw,
>  		[CLKID_MPLL_PREDIV]	    = &gxbb_mpll_prediv.hw,
> @@ -3439,7 +3483,7 @@ static struct clk_regmap *const gxl_clk_regmaps[] = {
>  	&gxbb_mpll0,
>  	&gxbb_mpll1,
>  	&gxbb_mpll2,
> -	&gxbb_mpll0_div,
> +	&gxl_mpll0_div,
>  	&gxbb_mpll1_div,
>  	&gxbb_mpll2_div,
>  	&gxbb_cts_amclk_div,

Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Martin Blumenstingl]

Hi Jerome,

On Mon, Oct 18, 2021 at 12:19 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>
> On Sat 16 Oct 2021 at 16:59, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > Christian reports that 48kHz audio does not work on his WeTek Play 2
> > (which uses a GXBB SoC), while 44.1kHz audio works fine on the same
> > board. He also reports that 48kHz audio works on GXL and GXM SoCs,
> > which are using an (almost) identical AIU (audio controller).
>
> The above is a bit "personal" - it is not great fit for the commit
> description. Please rephrase or put it in comment section bellow
sure, I can rephrase that

[...]
> > Add the SSEN (spread spectrum enable) bit and add the
> > CLK_MESON_MPLL_SPREAD_SPECTRUM flag to enable this bit for MPLL0. Do
> > this for GXBB *only* since GXL doesn't seem to care if this bit is set
> > or not, meaning that meson-clk-msr always sees (approximately) the same
> > frequency as common clock framework.
>
>  1 - it is odd that we need to poke a bit in the register related to the
>  fixed PLL but ok ...
>  2 - 3.14 does yes, 4.9 does not soooo ... no real proof there
The fact that 4.9 doesn't do it is also no proof for anything either.
Amlogic hasn't ported forward GXBB support to their 4.9 kernel. Even
"mesongxbb.dtsi" is missing there [0]
So in my opinion the code from Amlogic's patched 4.9 kernel cannot be
used as reference for anything GXBB related. Only the 3.14 code can be
used as reference.

[...]
> So 2 things:
>  - If this bit really enables spread spectrum on MPLL0 (or worse, the
>  Fixed PLL) - checking clk measure is not enough. It is just a mean of
>  the rate seen by the SoC itself. You would not see the effect of the
>  spread spectrum here ... you need to capture the clock output with a
>  scope for that.
I suspect that a board with pin headers is needed to route the signal there?
Personally I don't have any GXBB board(s).

>  - Or the bit is incorrectly documented (or DDS0_SSEN does not mean
>  spread spectrum). If it is not a spread spectrum function, then this
>  patch seems to indicate it is and it is misleading.
>
> Either way, I'm not OK with it.
>
> To me, the rate drop that happens when you flip this bit looks more like
> the effect SDM_EN should have.
>
> Could you check the internal values (n2 and sdm) compare this to the
> output rate you actually get ? see if this leads to anything ? does
> SDM_EN really has an effect on this MPLL ? it is a combination of both ?
Christian has provided a dump of the HHI registers (via userspace,
regmap makes them accessible).
On GXBB WP2 HHI_MPLL_CNTL7 is set to 0x0006f208
On GXL Le Potato (which he used for comparison as it wasn't affected
by the MPLL0 issue) HHI_MPLL_CNTL7 is set to 0x0006b208

If you're interested in the full HHI register dump you can find it here: [1]
GXBB WP2 is on the left and GXL Le Potato is on the right.

The difference here is BIT(14). un-setting BIT(14) (documented as
EN_DDS0) did not change anything according to Christian's test.
That also means that SDM, SDM_EN and N2 have the expected values.
I manually did the maths:
(2000000000Hz * 16384) / ((16384 * 6) + 12808) = 294909640.7Hz
which matches what clk_summary sees:
294909641Hz

Just to clarify that I am not wasting Christian's time when I ask him
to test something:
next up we'll set SDM_EN to 0 (instead of 1) and see the reaction using clk-msr?


Best regards,
Martin


[0] https://github.com/hardkernel/linux/tree/db88db3864c5d6903fb11f6528874887d1c473ce/arch/arm64/boot/dts/amlogic
[1] https://pastebin.com/raw/1cjPFsfa

Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Jerome Brunet]

On Mon 18 Oct 2021 at 13:44, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Mon, Oct 18, 2021 at 12:19 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>>
>> On Sat 16 Oct 2021 at 16:59, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> > Christian reports that 48kHz audio does not work on his WeTek Play 2
>> > (which uses a GXBB SoC), while 44.1kHz audio works fine on the same
>> > board. He also reports that 48kHz audio works on GXL and GXM SoCs,
>> > which are using an (almost) identical AIU (audio controller).
>>
>> The above is a bit "personal" - it is not great fit for the commit
>> description. Please rephrase or put it in comment section bellow
> sure, I can rephrase that
>
> [...]
>> > Add the SSEN (spread spectrum enable) bit and add the
>> > CLK_MESON_MPLL_SPREAD_SPECTRUM flag to enable this bit for MPLL0. Do
>> > this for GXBB *only* since GXL doesn't seem to care if this bit is set
>> > or not, meaning that meson-clk-msr always sees (approximately) the same
>> > frequency as common clock framework.
>>
>>  1 - it is odd that we need to poke a bit in the register related to the
>>  fixed PLL but ok ...
>>  2 - 3.14 does yes, 4.9 does not soooo ... no real proof there
> The fact that 4.9 doesn't do it is also no proof for anything either.
> Amlogic hasn't ported forward GXBB support to their 4.9 kernel. Even
> "mesongxbb.dtsi" is missing there [0]
> So in my opinion the code from Amlogic's patched 4.9 kernel cannot be
> used as reference for anything GXBB related. Only the 3.14 code can be
> used as reference.
>
> [...]
>> So 2 things:
>>  - If this bit really enables spread spectrum on MPLL0 (or worse, the
>>  Fixed PLL) - checking clk measure is not enough. It is just a mean of
>>  the rate seen by the SoC itself. You would not see the effect of the
>>  spread spectrum here ... you need to capture the clock output with a
>>  scope for that.
> I suspect that a board with pin headers is needed to route the signal there?
> Personally I don't have any GXBB board(s).
>
>>  - Or the bit is incorrectly documented (or DDS0_SSEN does not mean
>>  spread spectrum). If it is not a spread spectrum function, then this
>>  patch seems to indicate it is and it is misleading.
>>
>> Either way, I'm not OK with it.
>>
>> To me, the rate drop that happens when you flip this bit looks more like
>> the effect SDM_EN should have.
>>
>> Could you check the internal values (n2 and sdm) compare this to the
>> output rate you actually get ? see if this leads to anything ? does
>> SDM_EN really has an effect on this MPLL ? it is a combination of both ?
> Christian has provided a dump of the HHI registers (via userspace,
> regmap makes them accessible).
> On GXBB WP2 HHI_MPLL_CNTL7 is set to 0x0006f208
> On GXL Le Potato (which he used for comparison as it wasn't affected
> by the MPLL0 issue) HHI_MPLL_CNTL7 is set to 0x0006b208
>
> If you're interested in the full HHI register dump you can find it here: [1]
> GXBB WP2 is on the left and GXL Le Potato is on the right.
>
> The difference here is BIT(14). un-setting BIT(14) (documented as
> EN_DDS0) did not change anything according to Christian's test.
> That also means that SDM, SDM_EN and N2 have the expected values.
> I manually did the maths:
> (2000000000Hz * 16384) / ((16384 * 6) + 12808) = 294909640.7Hz
> which matches what clk_summary sees:
> 294909641Hz

 ... and (2000000000Hz * 16384) / ((16384 * 6) = 333MHz which is fairly close
 to what you get w/o flipping the bit

>
> Just to clarify that I am not wasting Christian's time when I ask him
> to test something:
> next up we'll set SDM_EN to 0 (instead of 1) and see the reaction using clk-msr?
>

For example yes. I am asking check a bit more what this bit does and
what it does not:
 - I need confirmation whether or not it does spread spectrum. Yes this
 needs to be observed on a SoC pin, like MCLK with a fairly low divider
 to the averaging effect which could partially mask spread spectrum.

 - Get an idea what it actually does. The 2 calculations above are an
 hint. (Spread spectrum does not change the rate mean value)

>
> Best regards,
> Martin
>
>
> [0] https://github.com/hardkernel/linux/tree/db88db3864c5d6903fb11f6528874887d1c473ce/arch/arm64/boot/dts/amlogic
> [1] https://pastebin.com/raw/1cjPFsfa

Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Martin Blumenstingl]

Hi Jerome,

On Mon, Oct 18, 2021 at 2:03 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
[...]
> > The difference here is BIT(14). un-setting BIT(14) (documented as
> > EN_DDS0) did not change anything according to Christian's test.
> > That also means that SDM, SDM_EN and N2 have the expected values.
> > I manually did the maths:
> > (2000000000Hz * 16384) / ((16384 * 6) + 12808) = 294909640.7Hz
> > which matches what clk_summary sees:
> > 294909641Hz
>
>  ... and (2000000000Hz * 16384) / ((16384 * 6) = 333MHz which is fairly close
>  to what you get w/o flipping the bit
This is actually a great hint. So far MPLL clocks have "just worked"
for me and I didn't have to work with this.
With your explanation it makes sense that SDM_EN makes the hardware
use or ignore the SDM value.

[...]
> For example yes. I am asking check a bit more what this bit does and
> what it does not:
>  - I need confirmation whether or not it does spread spectrum. Yes this
>  needs to be observed on a SoC pin, like MCLK with a fairly low divider
>  to the averaging effect which could partially mask spread spectrum.
I did some more tests with Christian. It turns out that on GXBB
HHI_MPLL_CNTL7[15] has no impact on the rate seen by meson-clk-msr.
On the other hand, HHI_MPLL_CNTL[25] makes MPLL0 use or ignore the SDM
value (again, verified through meson-clk-msr).

>  - Get an idea what it actually does. The 2 calculations above are an
>  hint. (Spread spectrum does not change the rate mean value)
Indeed!
My conclusion is that on GXBB:
1) HHI_MPLL_CNTL[25] doesn't control the spread spectrum setting of
MPLL0 - just like you thought
2) HHI_MPLL_CNTL[25] is actually SDM_EN (and HHI_MPLL_CNTL7[15]
doesn't seem to have any impact on MPLL0's output rate)

Please let me know if there's anything else we can test.
Else I'll send a patch for making HHI_MPLL_CNTL[25] the SDM_EN bit of
MPLL0 on GXBB.


Best regards,
Martin

Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB

[Jerome Brunet]

On Wed 20 Oct 2021 at 20:16, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Mon, Oct 18, 2021 at 2:03 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> > The difference here is BIT(14). un-setting BIT(14) (documented as
>> > EN_DDS0) did not change anything according to Christian's test.
>> > That also means that SDM, SDM_EN and N2 have the expected values.
>> > I manually did the maths:
>> > (2000000000Hz * 16384) / ((16384 * 6) + 12808) = 294909640.7Hz
>> > which matches what clk_summary sees:
>> > 294909641Hz
>>
>>  ... and (2000000000Hz * 16384) / ((16384 * 6) = 333MHz which is fairly close
>>  to what you get w/o flipping the bit
> This is actually a great hint. So far MPLL clocks have "just worked"
> for me and I didn't have to work with this.
> With your explanation it makes sense that SDM_EN makes the hardware
> use or ignore the SDM value.
>
> [...]
>> For example yes. I am asking check a bit more what this bit does and
>> what it does not:
>>  - I need confirmation whether or not it does spread spectrum. Yes this
>>  needs to be observed on a SoC pin, like MCLK with a fairly low divider
>>  to the averaging effect which could partially mask spread spectrum.
> I did some more tests with Christian. It turns out that on GXBB
> HHI_MPLL_CNTL7[15] has no impact on the rate seen by meson-clk-msr.
> On the other hand, HHI_MPLL_CNTL[25] makes MPLL0 use or ignore the SDM
> value (again, verified through meson-clk-msr).
>
>>  - Get an idea what it actually does. The 2 calculations above are an
>>  hint. (Spread spectrum does not change the rate mean value)
> Indeed!
> My conclusion is that on GXBB:
> 1) HHI_MPLL_CNTL[25] doesn't control the spread spectrum setting of
> MPLL0 - just like you thought
> 2) HHI_MPLL_CNTL[25] is actually SDM_EN (and HHI_MPLL_CNTL7[15]
> doesn't seem to have any impact on MPLL0's output rate)
>
> Please let me know if there's anything else we can test.
> Else I'll send a patch for making HHI_MPLL_CNTL[25] the SDM_EN bit of
> MPLL0 on GXBB.

Fine by me if you are confident that HHI_MPLL_CNTL7[15] has indeed no
effect. Maybe add a comment about this oddity.

>
>
> Best regards,
> Martin