ASoC: Intel: haswell: Power transition refactor
diff mbox series

Message ID 20200330194520.13253-1-cezary.rojewski@intel.com
State Accepted
Commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a
Headers show
Series
  • ASoC: Intel: haswell: Power transition refactor
Related show

Commit Message

Cezary Rojewski March 30, 2020, 7:45 p.m. UTC
Update D0 <-> D3 sequence to correctly transition hardware and DSP core
from and to D3. On top of that, set SHIM registers to their recommended
defaults during D0 and D3 proceduces as HW does not reset registers for
us.

Connected to:
[alsa-devel][BUG] bdw-rt5650 DSP boot timeout
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-July/153098.html

Github issue ticket reference:
https://github.com/thesofproject/linux/pull/1842

Tested on:
- BDW-Y RVP with rt286
- SAMUS with rt5677

Proposed solution (both in July 2019 and on github):
'Revert "ASoC: Intel: Work around to fix HW d3 potential crash issue"'
is NAKed as it only covers the problem up and actually brings back the
undefined behavior: some registers (e.g.: APLLSE) are describing LPT
offsets rather than WPT ones. In consequence, during power-transitions
driver issues incorrect writes and leaves the regs of interest alone.

Existing patch - the non-revert - does not resolve the HW D3 issue at
all as it ignores the recommended sequence and does not initialize
hardware registers as expected. And thus, leaving things as are is also
unacceptable.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/haswell/sst-haswell-dsp.c | 185 ++++++++++++----------
 1 file changed, 104 insertions(+), 81 deletions(-)

Comments

Cezary Rojewski April 6, 2020, 8:48 a.m. UTC | #1
On 2020-03-30 21:45, Cezary Rojewski wrote:
> Update D0 <-> D3 sequence to correctly transition hardware and DSP core
> from and to D3. On top of that, set SHIM registers to their recommended
> defaults during D0 and D3 proceduces as HW does not reset registers for
> us.
> 
> Connected to:
> [alsa-devel][BUG] bdw-rt5650 DSP boot timeout
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-July/153098.html
> 
> Github issue ticket reference:
> https://github.com/thesofproject/linux/pull/1842
> 
> Tested on:
> - BDW-Y RVP with rt286
> - SAMUS with rt5677
> 
> Proposed solution (both in July 2019 and on github):
> 'Revert "ASoC: Intel: Work around to fix HW d3 potential crash issue"'
> is NAKed as it only covers the problem up and actually brings back the
> undefined behavior: some registers (e.g.: APLLSE) are describing LPT
> offsets rather than WPT ones. In consequence, during power-transitions
> driver issues incorrect writes and leaves the regs of interest alone.
> 
> Existing patch - the non-revert - does not resolve the HW D3 issue at
> all as it ignores the recommended sequence and does not initialize
> hardware registers as expected. And thus, leaving things as are is also
> unacceptable.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---

Pierre, your thoughts on this? This has already been confirmed working.
Pierre-Louis Bossart April 6, 2020, 3:02 p.m. UTC | #2
On 4/6/20 3:48 AM, Cezary Rojewski wrote:
> On 2020-03-30 21:45, Cezary Rojewski wrote:
>> Update D0 <-> D3 sequence to correctly transition hardware and DSP core
>> from and to D3. On top of that, set SHIM registers to their recommended
>> defaults during D0 and D3 proceduces as HW does not reset registers for
>> us.
>>
>> Connected to:
>> [alsa-devel][BUG] bdw-rt5650 DSP boot timeout
>> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-July/153098.html 
>>
>>
>> Github issue ticket reference:
>> https://github.com/thesofproject/linux/pull/1842
>>
>> Tested on:
>> - BDW-Y RVP with rt286
>> - SAMUS with rt5677
>>
>> Proposed solution (both in July 2019 and on github):
>> 'Revert "ASoC: Intel: Work around to fix HW d3 potential crash issue"'
>> is NAKed as it only covers the problem up and actually brings back the
>> undefined behavior: some registers (e.g.: APLLSE) are describing LPT
>> offsets rather than WPT ones. In consequence, during power-transitions
>> driver issues incorrect writes and leaves the regs of interest alone.
>>
>> Existing patch - the non-revert - does not resolve the HW D3 issue at
>> all as it ignores the recommended sequence and does not initialize
>> hardware registers as expected. And thus, leaving things as are is also
>> unacceptable.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
> 
> Pierre, your thoughts on this? This has already been confirmed working.

I don't have any specific knowledge on Broadwell to comment. I also 
haven't had time to test this patch, I was expecting Ross to provide his 
Tested-by tag?
Ross Zwisler April 6, 2020, 6:09 p.m. UTC | #3
On Mon, Mar 30, 2020 at 09:45:20PM +0200, Cezary Rojewski wrote:
> Update D0 <-> D3 sequence to correctly transition hardware and DSP core
> from and to D3. On top of that, set SHIM registers to their recommended
> defaults during D0 and D3 proceduces as HW does not reset registers for
> us.
> 
> Connected to:
> [alsa-devel][BUG] bdw-rt5650 DSP boot timeout
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-July/153098.html
> 
> Github issue ticket reference:
> https://github.com/thesofproject/linux/pull/1842
> 
> Tested on:
> - BDW-Y RVP with rt286
> - SAMUS with rt5677
> 
> Proposed solution (both in July 2019 and on github):
> 'Revert "ASoC: Intel: Work around to fix HW d3 potential crash issue"'
> is NAKed as it only covers the problem up and actually brings back the
> undefined behavior: some registers (e.g.: APLLSE) are describing LPT
> offsets rather than WPT ones. In consequence, during power-transitions
> driver issues incorrect writes and leaves the regs of interest alone.
> 
> Existing patch - the non-revert - does not resolve the HW D3 issue at
> all as it ignores the recommended sequence and does not initialize
> hardware registers as expected. And thus, leaving things as are is also
> unacceptable.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

Tested-by: Ross Zwisler <zwisler@google.com>
Cezary Rojewski April 13, 2020, 4:38 p.m. UTC | #4
On 2020-04-06 17:02, Pierre-Louis Bossart wrote:
> On 4/6/20 3:48 AM, Cezary Rojewski wrote:
>> On 2020-03-30 21:45, Cezary Rojewski wrote:
>>> Update D0 <-> D3 sequence to correctly transition hardware and DSP core
>>> from and to D3. On top of that, set SHIM registers to their recommended
>>> defaults during D0 and D3 proceduces as HW does not reset registers for
>>> us.
>>>
>>> Connected to:
>>> [alsa-devel][BUG] bdw-rt5650 DSP boot timeout
>>> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-July/153098.html 
>>>
>>>
>>> Github issue ticket reference:
>>> https://github.com/thesofproject/linux/pull/1842
>>>
>>> Tested on:
>>> - BDW-Y RVP with rt286
>>> - SAMUS with rt5677
>>>
>>> Proposed solution (both in July 2019 and on github):
>>> 'Revert "ASoC: Intel: Work around to fix HW d3 potential crash issue"'
>>> is NAKed as it only covers the problem up and actually brings back the
>>> undefined behavior: some registers (e.g.: APLLSE) are describing LPT
>>> offsets rather than WPT ones. In consequence, during power-transitions
>>> driver issues incorrect writes and leaves the regs of interest alone.
>>>
>>> Existing patch - the non-revert - does not resolve the HW D3 issue at
>>> all as it ignores the recommended sequence and does not initialize
>>> hardware registers as expected. And thus, leaving things as are is also
>>> unacceptable.
>>>
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>
>> Pierre, your thoughts on this? This has already been confirmed working.
> 
> I don't have any specific knowledge on Broadwell to comment. I also 
> haven't had time to test this patch, I was expecting Ross to provide his 
> Tested-by tag?

Hello,

Ross has provided his Tested-by tag already. Patch has been tested by 
Intel & Google side both. Given problem's impact, this fix is considered 
a critical one. I think we are good-to-go for quite a while now?

Czarek
Mark Brown April 17, 2020, 6:56 p.m. UTC | #5
On Mon, 30 Mar 2020 21:45:20 +0200, Cezary Rojewski wrote:
> Update D0 <-> D3 sequence to correctly transition hardware and DSP core
> from and to D3. On top of that, set SHIM registers to their recommended
> defaults during D0 and D3 proceduces as HW does not reset registers for
> us.
> 
> Connected to:
> [alsa-devel][BUG] bdw-rt5650 DSP boot timeout
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-July/153098.html
> 
> [...]

Applied, thanks!

[1/1] ASoC: Intel: haswell: Power transition refactor
      commit: 8ec7d6043263ecf250b9b7c0dd8ade899487538a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Patch
diff mbox series

diff --git a/sound/soc/intel/haswell/sst-haswell-dsp.c b/sound/soc/intel/haswell/sst-haswell-dsp.c
index 88c3f63bded9..de80e19454c1 100644
--- a/sound/soc/intel/haswell/sst-haswell-dsp.c
+++ b/sound/soc/intel/haswell/sst-haswell-dsp.c
@@ -243,45 +243,92 @@  static irqreturn_t hsw_irq(int irq, void *context)
 	return ret;
 }
 
+#define CSR_DEFAULT_VALUE 0x8480040E
+#define ISC_DEFAULT_VALUE 0x0
+#define ISD_DEFAULT_VALUE 0x0
+#define IMC_DEFAULT_VALUE 0x7FFF0003
+#define IMD_DEFAULT_VALUE 0x7FFF0003
+#define IPCC_DEFAULT_VALUE 0x0
+#define IPCD_DEFAULT_VALUE 0x0
+#define CLKCTL_DEFAULT_VALUE 0x7FF
+#define CSR2_DEFAULT_VALUE 0x0
+#define LTR_CTRL_DEFAULT_VALUE 0x0
+#define HMD_CTRL_DEFAULT_VALUE 0x0
+
+static void hsw_set_shim_defaults(struct sst_dsp *sst)
+{
+	sst_dsp_shim_write_unlocked(sst, SST_CSR, CSR_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_ISRX, ISC_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_ISRD, ISD_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_IMRX, IMC_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_IMRD, IMD_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_IPCX, IPCC_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_IPCD, IPCD_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_CLKCTL, CLKCTL_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_CSR2, CSR2_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_LTRC, LTR_CTRL_DEFAULT_VALUE);
+	sst_dsp_shim_write_unlocked(sst, SST_HMDC, HMD_CTRL_DEFAULT_VALUE);
+}
+
+/* all clock-gating minus DCLCGE and DTCGE */
+#define SST_VDRTCL2_CG_OTHER	0xB7D
+
 static void hsw_set_dsp_D3(struct sst_dsp *sst)
 {
-	u32 val;
 	u32 reg;
 
-	/* Disable core clock gating (VDRTCTL2.DCLCGE = 0) */
+	/* disable clock core gating */
 	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
-	reg &= ~(SST_VDRTCL2_DCLCGE | SST_VDRTCL2_DTCGE);
+	reg &= ~(SST_VDRTCL2_DCLCGE);
 	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
-	/* enable power gating and switch off DRAM & IRAM blocks */
-	val = readl(sst->addr.pci_cfg + SST_VDRTCTL0);
-	val |= SST_VDRTCL0_DSRAMPGE_MASK |
-		SST_VDRTCL0_ISRAMPGE_MASK;
-	val &= ~(SST_VDRTCL0_D3PGD | SST_VDRTCL0_D3SRAMPGD);
-	writel(val, sst->addr.pci_cfg + SST_VDRTCTL0);
+	/* stall, reset and set 24MHz XOSC */
+	sst_dsp_shim_update_bits_unlocked(sst, SST_CSR,
+			SST_CSR_24MHZ_LPCS | SST_CSR_STALL | SST_CSR_RST,
+			SST_CSR_24MHZ_LPCS | SST_CSR_STALL | SST_CSR_RST);
 
-	/* switch off audio PLL */
-	val = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
-	val |= SST_VDRTCL2_APLLSE_MASK;
-	writel(val, sst->addr.pci_cfg + SST_VDRTCTL2);
+	/* DRAM power gating all */
+	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL0);
+	reg |= SST_VDRTCL0_ISRAMPGE_MASK |
+		SST_VDRTCL0_DSRAMPGE_MASK;
+	reg &= ~(SST_VDRTCL0_D3SRAMPGD);
+	reg |= SST_VDRTCL0_D3PGD;
+	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL0);
+	udelay(50);
 
-	/* disable MCLK(clkctl.smos = 0) */
-	sst_dsp_shim_update_bits_unlocked(sst, SST_CLKCTL,
-		SST_CLKCTL_MASK, 0);
+	/* PLL shutdown enable */
+	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
+	reg |= SST_VDRTCL2_APLLSE_MASK;
+	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
-	/* Set D3 state, delay 50 us */
-	val = readl(sst->addr.pci_cfg + SST_PMCS);
-	val |= SST_PMCS_PS_MASK;
-	writel(val, sst->addr.pci_cfg + SST_PMCS);
-	udelay(50);
+	/* disable MCLK */
+	sst_dsp_shim_update_bits_unlocked(sst, SST_CLKCTL,
+			SST_CLKCTL_MASK, 0);
 
-	/* Enable core clock gating (VDRTCTL2.DCLCGE = 1), delay 50 us */
+	/* switch clock gating */
+	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
+	reg |= SST_VDRTCL2_CG_OTHER;
+	reg &= ~(SST_VDRTCL2_DTCGE);
+	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
+	/* enable DTCGE separatelly */
 	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
-	reg |= SST_VDRTCL2_DCLCGE | SST_VDRTCL2_DTCGE;
+	reg |= SST_VDRTCL2_DTCGE;
 	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
+	/* set shim defaults */
+	hsw_set_shim_defaults(sst);
+
+	/* set D3 */
+	reg = readl(sst->addr.pci_cfg + SST_PMCS);
+	reg |= SST_PMCS_PS_MASK;
+	writel(reg, sst->addr.pci_cfg + SST_PMCS);
 	udelay(50);
 
+	/* enable clock core gating */
+	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
+	reg |= SST_VDRTCL2_DCLCGE;
+	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
+	udelay(50);
 }
 
 static void hsw_reset(struct sst_dsp *sst)
@@ -299,75 +346,62 @@  static void hsw_reset(struct sst_dsp *sst)
 		SST_CSR_RST | SST_CSR_STALL, SST_CSR_STALL);
 }
 
+/* recommended CSR state for power-up */
+#define SST_CSR_D0_MASK (0x18A09C0C | SST_CSR_DCS_MASK)
+
 static int hsw_set_dsp_D0(struct sst_dsp *sst)
 {
-	int tries = 10;
-	u32 reg, fw_dump_bit;
+	u32 reg;
 
-	/* Disable core clock gating (VDRTCTL2.DCLCGE = 0) */
+	/* disable clock core gating */
 	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
-	reg &= ~(SST_VDRTCL2_DCLCGE | SST_VDRTCL2_DTCGE);
+	reg &= ~(SST_VDRTCL2_DCLCGE);
 	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
-	/* Disable D3PG (VDRTCTL0.D3PGD = 1) */
-	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL0);
-	reg |= SST_VDRTCL0_D3PGD;
-	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL0);
+	/* switch clock gating */
+	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
+	reg |= SST_VDRTCL2_CG_OTHER;
+	reg &= ~(SST_VDRTCL2_DTCGE);
+	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
-	/* Set D0 state */
+	/* set D0 */
 	reg = readl(sst->addr.pci_cfg + SST_PMCS);
-	reg &= ~SST_PMCS_PS_MASK;
+	reg &= ~(SST_PMCS_PS_MASK);
 	writel(reg, sst->addr.pci_cfg + SST_PMCS);
 
-	/* check that ADSP shim is enabled */
-	while (tries--) {
-		reg = readl(sst->addr.pci_cfg + SST_PMCS) & SST_PMCS_PS_MASK;
-		if (reg == 0)
-			goto finish;
-
-		msleep(1);
-	}
-
-	return -ENODEV;
-
-finish:
-	/* select SSP1 19.2MHz base clock, SSP clock 0, turn off Low Power Clock */
-	sst_dsp_shim_update_bits_unlocked(sst, SST_CSR,
-		SST_CSR_S1IOCS | SST_CSR_SBCS1 | SST_CSR_LPCS, 0x0);
+	/* DRAM power gating none */
+	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL0);
+	reg &= ~(SST_VDRTCL0_ISRAMPGE_MASK |
+		SST_VDRTCL0_DSRAMPGE_MASK);
+	reg |= SST_VDRTCL0_D3SRAMPGD;
+	reg |= SST_VDRTCL0_D3PGD;
+	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL0);
+	mdelay(10);
 
-	/* stall DSP core, set clk to 192/96Mhz */
-	sst_dsp_shim_update_bits_unlocked(sst,
-		SST_CSR, SST_CSR_STALL | SST_CSR_DCS_MASK,
-		SST_CSR_STALL | SST_CSR_DCS(4));
+	/* set shim defaults */
+	hsw_set_shim_defaults(sst);
 
-	/* Set 24MHz MCLK, prevent local clock gating, enable SSP0 clock */
+	/* restore MCLK */
 	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);
+			SST_CLKCTL_MASK, SST_CLKCTL_MASK);
 
-	/* Stall and reset core, set CSR */
-	hsw_reset(sst);
-
-	/* Enable core clock gating (VDRTCTL2.DCLCGE = 1), delay 50 us */
+	/* PLL shutdown disable */
 	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
-	reg |= SST_VDRTCL2_DCLCGE | SST_VDRTCL2_DTCGE;
+	reg &= ~(SST_VDRTCL2_APLLSE_MASK);
 	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
+	sst_dsp_shim_update_bits_unlocked(sst, SST_CSR,
+			SST_CSR_D0_MASK, SST_CSR_SBCS0 | SST_CSR_SBCS1 |
+			SST_CSR_STALL | SST_CSR_DCS(4));
 	udelay(50);
 
-	/* switch on audio PLL */
+	/* enable clock core gating */
 	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL2);
-	reg &= ~SST_VDRTCL2_APLLSE_MASK;
+	reg |= SST_VDRTCL2_DCLCGE;
 	writel(reg, sst->addr.pci_cfg + SST_VDRTCTL2);
 
-	/* set default power gating control, enable power gating control for all blocks. that is,
-	can't be accessed, please enable each block before accessing. */
-	reg = readl(sst->addr.pci_cfg + SST_VDRTCTL0);
-	reg |= SST_VDRTCL0_DSRAMPGE_MASK | SST_VDRTCL0_ISRAMPGE_MASK;
-	/* for D0, always enable the block(DSRAM[0]) used for FW dump */
-	fw_dump_bit = 1 << SST_VDRTCL0_DSRAMPGE_SHIFT;
-	writel(reg & ~fw_dump_bit, sst->addr.pci_cfg + SST_VDRTCTL0);
-
+	/* clear reset */
+	sst_dsp_shim_update_bits_unlocked(sst, SST_CSR, SST_CSR_RST, 0);
 
 	/* disable DMA finish function for SSP0 & SSP1 */
 	sst_dsp_shim_update_bits_unlocked(sst, SST_CSR2, SST_CSR2_SDFD_SSP1,
@@ -384,12 +418,6 @@  static int hsw_set_dsp_D0(struct sst_dsp *sst)
 	sst_dsp_shim_update_bits(sst, SST_IMRD, (SST_IMRD_DONE | SST_IMRD_BUSY |
 				SST_IMRD_SSP0 | SST_IMRD_DMAC), 0x0);
 
-	/* clear IPC registers */
-	sst_dsp_shim_write(sst, SST_IPCX, 0x0);
-	sst_dsp_shim_write(sst, SST_IPCD, 0x0);
-	sst_dsp_shim_write(sst, 0x80, 0x6);
-	sst_dsp_shim_write(sst, 0xe0, 0x300a);
-
 	return 0;
 }
 
@@ -415,11 +443,6 @@  static void hsw_sleep(struct sst_dsp *sst)
 {
 	dev_dbg(sst->dev, "HSW_PM dsp runtime suspend\n");
 
-	/* 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);
 	dev_dbg(sst->dev, "HSW_PM dsp runtime suspend exit\n");
 }