[RFC,00/15] Add Samus Hotwording for RT5677

Message

Curtis Malainey Sept. 6, 2019, 7:46 p.m. UTC

This patch series adds the hotwording implementation used in the
Pixelbook on the RT5677 driver.

Known Issues:
There is a known issue where the system will fail to detect a hotword if
suspended while the stream is open. This is due to the fact that the
haswell-dsp suspends its I2S MCLK before the RT5677 suspends which
causes the writes and reads to become corrupted as a result. Any
recommendations to correct this behaviour would be appreciated.

Ben Zhang (12):
  ASoC: rt5677: keep analog power register at SND_SOC_BIAS_OFF
  ASoC: rt5677: Add a PCM device for streaming hotword via SPI
  ASoC: rt5677: Load firmware via SPI
  ASoC: rt5677: Auto enable/disable DSP for hotwording
  ASoC: bdw-rt5677: Add a DAI link for rt5677 SPI PCM device
  ASoC: rt5677: Enable jack detect while DSP is running
  ASoC: rt5677: Use delayed work for DSP firmware load
  ASoC: rt5677: Add DAPM audio path for hotword stream
  ASoC: rt5677: Mark reg RT5677_PWR_ANLG2 as volatile
  ASoC: rt5677: Stop and restart DSP over suspend/resume
  ASoC: rt5677: Transfer one period at a time over SPI
  ASoC: rt5677: Disable irq at suspend

Curtis Malainey (3):
  ASoC: rt5677: Remove magic number register writes
  ASoC: rt5677: Allow VAD to be shut on/off at all times
  ASoC: rt5677: Turn on MCLK1 for DSP via DAPM

 sound/soc/codecs/rt5677-spi.c       | 401 ++++++++++++++++++++++
 sound/soc/codecs/rt5677-spi.h       |   1 +
 sound/soc/codecs/rt5677.c           | 496 +++++++++++++++++++++++-----
 sound/soc/codecs/rt5677.h           |   9 +-
 sound/soc/intel/boards/bdw-rt5677.c |  18 +
 5 files changed, 843 insertions(+), 82 deletions(-)

Comments

Pierre-Louis Bossart Sept. 6, 2019, 8:40 p.m. UTC | #1

On 9/6/19 2:46 PM, Curtis Malainey wrote:
> This patch series adds the hotwording implementation used in the
> Pixelbook on the RT5677 driver.
> 
> Known Issues:
> There is a known issue where the system will fail to detect a hotword if
> suspended while the stream is open. This is due to the fact that the
> haswell-dsp suspends its I2S MCLK before the RT5677 suspends which
> causes the writes and reads to become corrupted as a result. Any
> recommendations to correct this behaviour would be appreciated.

I don't get what 'suspend' and 'stream' refer to. is this pm_runtime, 
s2idle, system capture, SPI capture?

Can you elaborate on the sequence?

Curtis Malainey Sept. 6, 2019, 9:09 p.m. UTC | #2

Curtis Malainey | Software Engineer | [e-mail] | 650-898-3849


On Fri, Sep 6, 2019 at 1:41 PM Pierre-Louis Bossart <
[e-mail]> wrote:
>
> On 9/6/19 2:46 PM, Curtis Malainey wrote:
> > This patch series adds the hotwording implementation used in the
> > Pixelbook on the RT5677 driver.
> >
> > Known Issues:
> > There is a known issue where the system will fail to detect a hotword if
> > suspended while the stream is open. This is due to the fact that the
> > haswell-dsp suspends its I2S MCLK before the RT5677 suspends which
> > causes the writes and reads to become corrupted as a result. Any
> > recommendations to correct this behaviour would be appreciated.
>
> I don't get what 'suspend' and 'stream' refer to. is this pm_runtime,
> s2idle, system capture, SPI capture?
>
> Can you elaborate on the sequence?
Definitely can,

   1. open hotwording pcm with arecord in non-blocking mode
      - Codec won't send any data over SPI until the hotword is detected
   2. put system into S3 (see order of callbacks as follows)
      1. HSW DSP suspended which suspends stops I2S MCLK
      2. RT5677 suspended, all pm writes are lost due to the fact that the
      codec is still in DSP mode but has no clock
   3. System resumes and fails to restore the RT5677 due to the fact that
   the regmap is now out of sync

The rt5677 needs to suspend before the haswell dsp but I am not sure how to
schedule that appropriately. The reason this worked in Samus is because it
launched with a 3.14 kernel which did not
have 0d2135ecadb0b2eec5338a7587ba29724ddf612b ("ASoC: Intel: Work around to
fix HW D3 potential crash issue") which powers down the MCLK when the
haswell DSP is not in use.

Hope that clears things up.

Pierre-Louis Bossart Sept. 6, 2019, 10:13 p.m. UTC | #3

On 9/6/19 4:09 PM, Curtis Malainey wrote:
> 
> 
> Curtis Malainey | Software Engineer | [e-mail] 
> <mailto:[e-mail]> | 650-898-3849
> 
> 
> On Fri, Sep 6, 2019 at 1:41 PM Pierre-Louis Bossart 
> <[e-mail] 
> <mailto:[e-mail]>> wrote:
>  >
>  > On 9/6/19 2:46 PM, Curtis Malainey wrote:
>  > > This patch series adds the hotwording implementation used in the
>  > > Pixelbook on the RT5677 driver.
>  > >
>  > > Known Issues:
>  > > There is a known issue where the system will fail to detect a 
> hotword if
>  > > suspended while the stream is open. This is due to the fact that the
>  > > haswell-dsp suspends its I2S MCLK before the RT5677 suspends which
>  > > causes the writes and reads to become corrupted as a result. Any
>  > > recommendations to correct this behaviour would be appreciated.
>  >
>  > I don't get what 'suspend' and 'stream' refer to. is this pm_runtime,
>  > s2idle, system capture, SPI capture?
>  >
>  > Can you elaborate on the sequence?
> Definitely can,
> 
>  1. open hotwording pcm with arecord in non-blocking mode
>       * Codec won't send any data over SPI until the hotword is detected
>  2. put system into S3 (see order of callbacks as follows)

Before we start digging into dependencies below, is it really possible 
to enter S3 with the hotwording open? I vaguely remember being told that 
such cases would be trapped by the Chrome userspace and the PCM would be 
closed. I don't think anyone on the SOF team testing this case for newer 
platform, so that case on an old platform makes me nervous.

>      1. HSW DSP suspended which suspends stops I2S MCLK
>      2. RT5677 suspended, all pm writes are lost due to the fact that
>         the codec is still in DSP mode but has no clock

there's no real dependency or parent-child relationship between the two 
drivers, is there? so I am wondering if this order is intentional or 
just accidental.
The only thing I can think of is that there are multiple steps during 
the system suspend and maybe we can play with .suspend_late instead of 
.suspend?

>  3. System resumes and fails to restore the RT5677 due to the fact that
>     the regmap is now out of sync
> 
> The rt5677 needs to suspend before the haswell dsp but I am not sure how 
> to schedule that appropriately. The reason this worked in Samus is 
> because it launched with a 3.14 kernel which did not 
> have 0d2135ecadb0b2eec5338a7587ba29724ddf612b ("ASoC: Intel: Work around 
> to fix HW D3 potential crash issue") which powers down the MCLK when the 
> haswell DSP is not in use.
> 
> Hope that clears things up.

Curtis Malainey Sept. 9, 2019, 4:52 p.m. UTC | #4

On Fri, Sep 6, 2019 at 3:13 PM Pierre-Louis Bossart
<[e-mail]> wrote:
>
> On 9/6/19 4:09 PM, Curtis Malainey wrote:
> >
> >
> > Curtis Malainey | Software Engineer | [e-mail]
> > <mailto:[e-mail]> | 650-898-3849
> >
> >
> > On Fri, Sep 6, 2019 at 1:41 PM Pierre-Louis Bossart
> > <[e-mail]
> > <mailto:[e-mail]>> wrote:
> >  >
> >  > On 9/6/19 2:46 PM, Curtis Malainey wrote:
> >  > > This patch series adds the hotwording implementation used in the
> >  > > Pixelbook on the RT5677 driver.
> >  > >
> >  > > Known Issues:
> >  > > There is a known issue where the system will fail to detect a
> > hotword if
> >  > > suspended while the stream is open. This is due to the fact that the
> >  > > haswell-dsp suspends its I2S MCLK before the RT5677 suspends which
> >  > > causes the writes and reads to become corrupted as a result. Any
> >  > > recommendations to correct this behaviour would be appreciated.
> >  >
> >  > I don't get what 'suspend' and 'stream' refer to. is this pm_runtime,
> >  > s2idle, system capture, SPI capture?
> >  >
> >  > Can you elaborate on the sequence?
> > Definitely can,
> >
> >  1. open hotwording pcm with arecord in non-blocking mode
> >       * Codec won't send any data over SPI until the hotword is detected
> >  2. put system into S3 (see order of callbacks as follows)
>
> Before we start digging into dependencies below, is it really possible
> to enter S3 with the hotwording open? I vaguely remember being told that
> such cases would be trapped by the Chrome userspace and the PCM would be
> closed. I don't think anyone on the SOF team testing this case for newer
> platform, so that case on an old platform makes me nervous.
>
I vaguely recall that as well now that you mention it. I will follow
up internally, if that is true then this will be a non-issue from our
point of view.
> >      1. HSW DSP suspended which suspends stops I2S MCLK
> >      2. RT5677 suspended, all pm writes are lost due to the fact that
> >         the codec is still in DSP mode but has no clock
>
> there's no real dependency or parent-child relationship between the two
> drivers, is there? so I am wondering if this order is intentional or
> just accidental.
> The only thing I can think of is that there are multiple steps during
> the system suspend and maybe we can play with .suspend_late instead of
> .suspend?
Not that I am aware of, when used as a standard codec there is no
clock dependency. I will try and see if I can set the pm accordingly.
>
> >  3. System resumes and fails to restore the RT5677 due to the fact that
> >     the regmap is now out of sync
> >
> > The rt5677 needs to suspend before the haswell dsp but I am not sure how
> > to schedule that appropriately. The reason this worked in Samus is
> > because it launched with a 3.14 kernel which did not
> > have 0d2135ecadb0b2eec5338a7587ba29724ddf612b ("ASoC: Intel: Work around
> > to fix HW D3 potential crash issue") which powers down the MCLK when the
> > haswell DSP is not in use.
> >
> > Hope that clears things up.
>