Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB
From: Jerome Brunet <jbrunet@baylibre.com>
Date: 2021-10-18 10:19:42
Also in:
linux-amlogic, linux-clk, lkml
On Sat 16 Oct 2021 at 16:59, Martin Blumenstingl [off-list ref] 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 ?
quoted hunk ↗ jump to hunk
Fixes: 8925dbd03bb29b ("clk: meson: gxbb: no spread spectrum on mpll") Reported-by: Christian Hewitt <redacted> 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,
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel