Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine

[Radosław Biernacki]

Thanks for the tip!
Let me reformat the patch.

wt., 8 wrz 2020 o 20:06 Pierre-Louis Bossart
<pierre-louis.bossart@xxxxxxxxxxxxxxx> napisał(a):

>
>
>
> On 9/8/20 12:42 PM, Radosław Biernacki wrote:
> > Sorry for missing the response for so long.
> > Somehow lost this thread in my mailbox.
>
> That happens to all of us!
>
> > śr., 6 maj 2020 o 00:04 Pierre-Louis Bossart
> > <pierre-louis.bossart@xxxxxxxxxxxxxxx> napisał(a):
> >>
> >>
> >>>>> This single fix address two issues on machines with nau88125:
> >>>>> 1) Audio distortion, due to lack of required clock rate on MCLK line
> >>>>> 2) Loud audible "pops" on headphones if there is no sysclk during nau8825
> >>>>>       playback power up sequence
> >>>>>
> >>>>> Explanation for:
> >>>>> 1) Due to Skylake HW limitation, MCLK pin can only output 24MHz clk
> >>>>>       rate (it can be only connected to XTAL parent clk). The BCLK pin
> >>>>>       can be driven by dividers and therefore FW is able to set it to rate
> >>>>>       required by chosen audio format. According to nau8825 datasheet, 256*FS
> >>>>>       sysclk gives the best audio quality and the only way to achieve this
> >>>>>       (taking into account the above limitations) its to regenerate the MCLK
> >>>>>       from BCLK on nau8825 side by FFL. Without required clk rate, audio is
> >>>>>       distorted by added harmonics.
> >>>>
> >>>> The BCLK is going to be a multiple of 50 * Fs due to clocking
> >>>> restrictions. Can the codec regenerate a good-enough sysclk from this?
> >>>
> >>> According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
> >>> SKL_XTAL, .name = "xtal" is available for IO domain.
> >>> As mentioned in the commit:
> >>> MCLK is generated by using 24MHz Xtal directly or applying a divider
> >>> (so no way of achieving the rate required by audio format).
> >>> BCLK/FS is generated from 24MHz and uses dividers and additional
> >>> padding bits are used to match the clock source.
> >>> Next gen silicon has the possibility of using additional clock sources.
> >>>
> >>> Summing up, using MCLK from SKL to NAU88L25 is not an option.
> >>> The only option we found is to use BCLK and regen the required clock
> >>> rate by FLL on the NAU88l25 side.
> >>
> >> Right, this 24 MHz is a recurring problem.
> >> But what I was asking was if the NAU8825 is fine working with e.g. a
> >> 2.4MHz bit clock. i.e. with 25 bit slots or padding at the end of the frame?
> >
> >  From our tests NAU8825 is working fine with these parameters.
> > Also the output audio signal looks fine on the scope and FFT and there
> > are no audible glitches.
> >
> >>
> >>>
> >>>>>
> >>>>> 2) Currently Skylake does not output MCLK/FS when the back-end DAI op
> >>>>>       hw_param is called, so we cannot switch to MCLK/FS in hw_param.  This
> >>>>>       patch reduces pop by letting nau8825 keep using its internal VCO clock
> >>>>>       during widget power up sequence, until SNDRV_PCM_TRIGGER_START when
> >>>>>       MCLK/FS is available. Once device resumes, the system will only enable
> >>>>>       power sequence for playback without doing hardware parameter, audio
> >>>>>       format, and PLL configure. In the mean time, the jack detecion sequence
> >>>>>       has changed PLL parameters and switched to internal clock. Thus, the
> >>>>>       playback signal distorted without correct PLL parameters.  That is why
> >>>>>       we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case.
> >>>>
> >>>> IIRC the FS can be controlled with the clk_ api with the Skylake driver,
> >>>> as done for some KBL platforms. Or is this not supported by the firmware
> >>>> used by this machine?
> >>>
> >>> According to Ben, SKL had limitations in FW for managing the clk's
> >>> back in the days.
> >>> Can you point to the other driver you mention so we can cross check?
> >>
> >> There are two KBL drivers that control the SSP clocks from the machine
> >> driver, but indeed I don't know if this would work on Firmware, it'd be
> >> a question for Lech/Cezary.
> >>
> >> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->mclk);
> >> kbl_rt5663_max98927.c:          ret = clk_prepare_enable(priv->sclk);
> >> kbl_rt5663_rt5514_max98927.c:           ret =
> >> clk_prepare_enable(priv->mclk);
> >> kbl_rt5663_rt5514_max98927.c:           ret =
> >> clk_prepare_enable(priv->sclk);
> >> kbl_rt5663_rt5514_max98927.c:                   ret =
> >> clk_prepare_enable(priv->mclk);
> >>
> >
> > Czarek answered this and we got the same response from other Intel
> > devs while consulting this change:
> > FW cannot request a chosen rate (48k) for MCLK pin as it does not
> > "align with what's present on SKL hw".
> >
> > The only way we found out for NAU8825 to cooperate at chosen rate with
> > SKL HW is to regen the MCLK from BCLK by FLL as mentioned above.
> > NHTL is used to set SSP0 (48k, 24/25 bit on 24MHz crystal).
> > If I get all of this right, use of NHTL and HW "abilities" would
> > explain why there is no call to change SSP from a machine driver.
> >
> >
> > If all of this is ok I will send V3 with msleep() removed.
>
> Sounds good.
>
> You may want to simplify your commit message and just state what you
> described, e.g.
>
> "Since 64xfs clocks cannot be generated, the NAU8825 is configured to
> re-generate its system clock from the BCLK using the FLL. The link is
> configured to use a 48kHz frame rate, and 24 bits in 25-bit slot. The
> SSP configuration is extracted from NHLT settings and not dynamically
> changed. Listening tests and measurements do not show any distortion or
> issues".
>
>
>