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;
References: <20211016145939.15643-1-martin.blumenstingl@googlemail.com>
 <1j5ytuvdmw.fsf@starbuckisacylon.baylibre.com>
 <CAFBinCBGZi3MRqTRshyCkq8AAaqHi2NkZVV80ppZr4Lx=xWOWA@mail.gmail.com>
User-agent: mu4e 1.6.6; emacs 27.1
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.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>
Subject: Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0
 on GXBB
Date:   Mon, 18 Oct 2021 13:55:32 +0200
In-reply-to: <CAFBinCBGZi3MRqTRshyCkq8AAaqHi2NkZVV80ppZr4Lx=xWOWA@mail.gmail.com>
Message-ID: <1jmtn6tu99.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


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