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: <20200501193141.30293-1-rad@semihalf.com> <3ad44b75-387f-da75-d7b2-3a16ed00550c@linux.intel.com>
 <CAOs-w0LPeKgooa_98x_Jkzus-Y5Kad7pDby0CriDGb6nTp_6sA@mail.gmail.com>
 <8b97bf43-ddd8-df81-90e7-9e87c19af1ab@linux.intel.com> <CAOs-w0KvKcRnDkMtdAV2n_3XLfwk9KgM65QEO=6+qNshobJx3g@mail.gmail.com>
 <2a88f81f-c7a3-f9fc-06a3-c39496b57d0c@linux.intel.com>
In-Reply-To: <2a88f81f-c7a3-f9fc-06a3-c39496b57d0c@linux.intel.com>
From:   =?UTF-8?Q?Rados=C5=82aw_Biernacki?= <rad@semihalf.com>
Date:   Tue, 8 Sep 2020 20:24:03 +0200
Message-ID: <CAOs-w0KRipoK5xHGf4eDkydd5GPE6HXpnKy1WVZctBkM4CiuDg@mail.gmail.com>
Subject: Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in
 nau88125_* machine
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     Lech Betlej <Lech.Betlej@intel.com>, alsa-devel@alsa-project.org,
        Todd Broch <tbroch@google.com>,
        Harshapriya <harshapriya.n@intel.com>,
        Jie Yang <yang.jie@linux.intel.com>,
        John Hsu <KCHSU0@nuvoton.com>, Takashi Iwai <tiwai@suse.com>,
        "Sienkiewicz, Michal" <michal.sienkiewicz@intel.com>,
        linux-kernel@vger.kernel.org,
        Liam Girdwood <liam.r.girdwood@linux.intel.com>,
        Ben Zhang <benzh@chromium.org>,
        Mac Chiang <mac.chiang@intel.com>,
        Yong Zhi <yong.zhi@intel.com>, Marcin Wojtas <mw@semihalf.com>,
        Vamshi Krishna <vamshi.krishna.gopal@intel.com>,
        Alex Levin <levinale@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for the tip!
Let me reformat the patch.

wt., 8 wrz 2020 o 20:06 Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> napisa=C5=82(a):

>
>
>
> On 9/8/20 12:42 PM, Rados=C5=82aw Biernacki wrote:
> > Sorry for missing the response for so long.
> > Somehow lost this thread in my mailbox.
>
> That happens to all of us!
>
> > =C5=9Br., 6 maj 2020 o 00:04 Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com> napisa=C5=82(a):
> >>
> >>
> >>>>> This single fix address two issues on machines with nau88125:
> >>>>> 1) Audio distortion, due to lack of required clock rate on MCLK lin=
e
> >>>>> 2) Loud audible "pops" on headphones if there is no sysclk during n=
au8825
> >>>>>       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 datashe=
et, 256*FS
> >>>>>       sysclk gives the best audio quality and the only way to achie=
ve 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 thi=
s?
> >>>
> >>> According to Intel, silicon has a limitation, on SKL/KBL only clk_id =
=3D
> >>> SKL_XTAL, .name =3D "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 source=
s.
> >>>
> >>> 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_para=
m.  This
> >>>>>       patch reduces pop by letting nau8825 keep using its internal =
VCO clock
> >>>>>       during widget power up sequence, until SNDRV_PCM_TRIGGER_STAR=
T when
> >>>>>       MCLK/FS is available. Once device resumes, the system will on=
ly enable
> >>>>>       power sequence for playback without doing hardware parameter,=
 audio
> >>>>>       format, and PLL configure. In the mean time, the jack detecio=
n sequence
> >>>>>       has changed PLL parameters and switched to internal clock. Th=
us, the
> >>>>>       playback signal distorted without correct PLL parameters.  Th=
at is why
> >>>>>       we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUM=
E case.
> >>>>
> >>>> IIRC the FS can be controlled with the clk_ api with the Skylake dri=
ver,
> >>>> as done for some KBL platforms. Or is this not supported by the firm=
ware
> >>>> 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 b=
e
> >> a question for Lech/Cezary.
> >>
> >> kbl_rt5663_max98927.c:          ret =3D clk_prepare_enable(priv->mclk)=
;
> >> kbl_rt5663_max98927.c:          ret =3D clk_prepare_enable(priv->sclk)=
;
> >> kbl_rt5663_rt5514_max98927.c:           ret =3D
> >> clk_prepare_enable(priv->mclk);
> >> kbl_rt5663_rt5514_max98927.c:           ret =3D
> >> clk_prepare_enable(priv->sclk);
> >> kbl_rt5663_rt5514_max98927.c:                   ret =3D
> >> 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".
>
>
>