Messages in this thread | | | From | Jerome Brunet <> | Subject | Re: [PATCH] clk: meson: gxbb: Add the spread spectrum bit for MPLL0 on GXBB | Date | Mon, 18 Oct 2021 13:55:32 +0200 |
| |
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
| |