Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211016145939.15643-1-martin.blumenstingl@googlemail.com> <1j5ytuvdmw.fsf@starbuckisacylon.baylibre.com>
In-Reply-To: <1j5ytuvdmw.fsf@starbuckisacylon.baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Mon, 18 Oct 2021 13:44:35 +0200
Message-ID: <CAFBinCBGZi3MRqTRshyCkq8AAaqHi2NkZVV80ppZr4Lx=xWOWA@mail.gmail.com>
Subject: Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0
 on GXBB
To:     Jerome Brunet <jbrunet@baylibre.com>
Cc:     linux-amlogic@lists.infradead.org,
        Neil Armstrong <narmstrong@baylibre.com>,
        linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org,
        Christian Hewitt <christianshewitt@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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