[RFC]ASoC: Intel: haswell-dsp: refine D0<->D3 transition sequence #1873

keyonjie · 2020-03-08T16:17:22Z

There was some minor mismatch between the commit message and the code
change in the commit "ASoC: Intel: Work around to fix HW D3 potential
crash issue", the gap basically is:

DSP trunck clock gating (DTCGE) can cause FW crashes, disable it in
D0.
Keep D3 power gating disabled (D3PGD = 1) in D3.

Fixes: 0d2135e ("ASoC: Intel: Work around to fix HW D3 potential crash issue")
Signed-off-by: Keyon Jie yang.jie@linux.intel.com

kv2019i

If tests are good on the affected devices, this is a better fix than reverting the whole change. I had one note on the commit message w.r.t. additional of core stalling that is in the patch.

kv2019i · 2020-03-09T11:24:03Z

sound/soc/intel/haswell/sst-haswell-dsp.c

@keyonjie This seems extra with regards to commit message. Probably good to mention why this is added.

@kv2019i yes, let me add it, thanks.

plbossart

@keyonjie the sequences still diverge from the recommendations?

plbossart · 2020-03-09T18:59:05Z

sound/soc/intel/haswell/sst-haswell-dsp.c

@keyonjie

in the D0->D3 transition, you are missing the "Enable DTCG to save power"

4. Shutdown PLL, disable MCLK(clkctl.smos = 0), Enable DTCG to save power

@plbossart the DTCG was enabled at the end of the function (line 288), I thought it can meet the power saving requirement to enable it a little bit later here and didn't change it.

plbossart · 2020-03-09T19:08:24Z

sound/soc/intel/haswell/sst-haswell-dsp.c

for the D3->D0 transition, you are setting the D0 state before powering up the memories

4. Power up necessary SRAM and wait at least for 18 clock cycles for every bank you have powered up

You are also combining steps 6 and 7

6. Restore MCLK (clkctl.smos, disabled in D3 entry point 4) 7. Stall and reset core, set CSR

You are missing the last step

11.Unstall core

also the hw_sleep() makes no sense, you are doing this:

/* put DSP into reset and stall */ sst_dsp_shim_update_bits(sst, SST_CSR, SST_CSR_24MHZ_LPCS | SST_CSR_RST | SST_CSR_STALL, SST_CSR_RST | SST_CSR_STALL | SST_CSR_24MHZ_LPCS); hsw_set_dsp_D3(sst);

but withing hw_set_dsp_D3, you also play with the stall:

static void hsw_set_dsp_D3(struct sst_dsp *sst) { u32 val; u32 reg; /* Disable core clock gating (VDRTCTL2.DCLCGE = 0) */ reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2); reg &= ~SST_VDRTCL2_DCLCGE; writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2); /* Stall and reset core */ sst_dsp_shim_update_bits_unlocked(sst, SST_CSR, SST_CSR_RST | SST_CSR_STALL, SST_CSR_RST | SST_CSR_STALL);

for the D3->D0 transition, you are setting the D0 state before powering up the memories

4. Power up necessary SRAM and wait at least for 18 clock cycles for every bank you have powered up

The necessary SRAM is enabled dynamically later when the FW module is reloaded, at this point here, we don't have information about which SRAM blocks are necessary. You are right, we not are following up the sequence exactly. But if we say none block is necessary here, then it's fine?

You are also combining steps 6 and 7

They are not combined, but maybe the fisrt CSR_STALL is superfluous?

/* stall DSP core, set clk to 192/96Mhz */ //Keyon: The CSR_STALL here might be superfluous? sst_dsp_shim_update_bits_unlocked(sst, SST_CSR, SST_CSR_STALL | SST_CSR_DCS_MASK, SST_CSR_STALL | SST_CSR_DCS(4)); /* Set 24MHz MCLK, prevent local clock gating, enable SSP0 clock */ //Keyon: 6.Restore MCLK (clkctl.smos) sst_dsp_shim_update_bits_unlocked(sst, SST_CLKCTL, SST_CLKCTL_MASK | SST_CLKCTL_DCPLCG | SST_CLKCTL_SCOE0, SST_CLKCTL_MASK | SST_CLKCTL_DCPLCG | SST_CLKCTL_SCOE0); /* Stall and reset core, set CSR */ //Keyon: 7. Stall and reset core, set CSR sst_dsp_shim_update_bits_unlocked(sst, SST_CSR, SST_CSR_RST | SST_CSR_STALL, SST_CSR_RST | SST_CSR_STALL);

6. Restore MCLK (clkctl.smos, disabled in D3 entry point 4) 7. Stall and reset core, set CSR

You are missing the last step

11.Unstall core

This is left and will do in hsw_boot(), we need unstall DSP core until FW is downloaded.

also the hw_sleep() makes no sense, you are doing this:

/* put DSP into reset and stall */ sst_dsp_shim_update_bits(sst, SST_CSR, SST_CSR_24MHZ_LPCS | SST_CSR_RST | SST_CSR_STALL, SST_CSR_RST | SST_CSR_STALL | SST_CSR_24MHZ_LPCS); hsw_set_dsp_D3(sst);

but withing hw_set_dsp_D3, you also play with the stall:

static void hsw_set_dsp_D3(struct sst_dsp *sst) { u32 val; u32 reg; /* Disable core clock gating (VDRTCTL2.DCLCGE = 0) */ reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2); reg &= ~SST_VDRTCL2_DCLCGE; writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2); /* Stall and reset core */ sst_dsp_shim_update_bits_unlocked(sst, SST_CSR, SST_CSR_RST | SST_CSR_STALL, SST_CSR_RST | SST_CSR_STALL);

Thanks, so adding stall and reset core here in my RFC commit looks not wise, let me keep it as it was.

keyonjie

Thanks for reviewing @plbossart .
There was some historic reason that why it was not exactly following the recommendations:

the code is for both HSW and BDW, and following the dsp ops(.boot(), .stall(). .wake(), .sleep()) from Baytrail, to avoid big framework change, I did some innovation as you mentioned for those not so important sequence(per my understanding) change, comparing with the recommendations.
except the recommended steps, we still some other items to cover, e.g. FW downloading and unstall DSP core after that, CSR2.SDFD and HMDC for SSP DMA, IPC interrupts enable/disable, etc.

keyonjie · 2020-03-10T02:14:10Z

sound/soc/intel/haswell/sst-haswell-dsp.c

@plbossart the DTCG was enabled at the end of the function (line 288), I thought it can meet the power saving requirement to enable it a little bit later here and didn't change it.

keyonjie · 2020-03-10T05:04:21Z

@keyonjie the sequences still diverge from the recommendations?

@plbossart as I stated below, there was some historic reasons that why it was not exactly following the recommendations.

The try to following the recommendations to spit DSP core reset/unreset from hsw_reset() is not a simple change, let me remove that change here.

There was some minor mismatch between the commit message and the code change in the commit "ASoC: Intel: Work around to fix HW D3 potential crash issue", the gap basically is: 1. DSP trunck clock gating (DTCGE) can cause FW crashes, disable it in D0. 2. Keep D3 power gating disabled (D3PGD = 1) in D3. Fixes: 0d2135e ("ASoC: Intel: Work around to fix HW D3 potential crash issue") Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2020-03-10T05:18:40Z

@plbossart @kv2019i @cujomalainey the commit is updated, please help review, thanks.

cujomalainey · 2020-03-10T19:48:59Z

Hi Keyon, we are running repro testing here

keyonjie · 2020-03-12T04:00:12Z

Hi Keyon, we are running repro testing here

Thanks @cujomalainey, please help let us know once you got any results.

cujomalainey · 2020-03-12T04:11:56Z

We are having trouble reproducing the bug without the fix currently (at least Pierre can) but our testing is also a bit slowed by changing to WFH

keyonjie · 2020-03-12T04:24:04Z

@cujomalainey got it, stay safe.

cujomalainey · 2020-03-12T18:26:12Z

Thanks, Keyon. 3k boots in and no luck, waiting to hear back from our BDW uprev team.

cujomalainey · 2020-03-12T18:37:42Z

Correction, I may have been producing the bug, but I had setup my test incorrectly. I was checking for no cards, but it looks like the HDMI driver is fine in a failure case. I will fix my test.

plbossart · 2020-03-30T20:16:09Z

solution provided upstream. Thanks @keyonjie

cujomalainey · 2020-03-30T23:58:23Z

Thanks @keyonjie for helping get this thing sorted out

keyonjie · 2020-03-31T06:25:53Z

Good that the issue is fixed, though I feel somewhat dizzy with those sequence changes.

keyonjie requested review from dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners March 8, 2020 16:17

keyonjie mentioned this pull request Mar 8, 2020

[RFC] Resolve DMA boot bug in BDW in SST driver #1842

Closed

kv2019i requested changes Mar 9, 2020

View reviewed changes

ranj063 requested a review from cujomalainey March 9, 2020 16:49

plbossart requested changes Mar 9, 2020

View reviewed changes

keyonjie commented Mar 10, 2020

View reviewed changes

keyonjie force-pushed the bdw-fix branch from 923a3d3 to 151eab2 Compare March 10, 2020 05:09

plbossart closed this Mar 30, 2020

[RFC]ASoC: Intel: haswell-dsp: refine D0<->D3 transition sequence #1873

[RFC]ASoC: Intel: haswell-dsp: refine D0<->D3 transition sequence #1873

Uh oh!

Conversation

keyonjie commented Mar 8, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjie Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjie Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjie Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Mar 10, 2020

Uh oh!

keyonjie commented Mar 10, 2020

Uh oh!

cujomalainey commented Mar 10, 2020

Uh oh!

keyonjie commented Mar 12, 2020

Uh oh!

cujomalainey commented Mar 12, 2020

Uh oh!

keyonjie commented Mar 12, 2020

Uh oh!

cujomalainey commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cujomalainey commented Mar 12, 2020

Uh oh!

plbossart commented Mar 30, 2020

Uh oh!

cujomalainey commented Mar 30, 2020

Uh oh!

keyonjie commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cujomalainey commented Mar 12, 2020 •

edited

Loading