[PATCH] ASoC: Intel: haswell: Power transition refactor
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(-)
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"); }
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.
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?
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
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
I just tested speaker playback on Dell XPS13 and Samus Chromebook to double-check my UCM2 changes for SOF were indeed backwards compatible with the SST driver case. Well, my changes are fine but the kernel not so much.
With a 5.8-rc1 kernel w/ the SST driver, sounds played through pulseaudio are rendered too slowly with clicky artefacts. Using the alsa hw device works fine. In some cases, the sound rendered by PulseAudio become clear again after a while. Restarting the UI and testing degrades the audio again.
Reverting this patch - identified with git bisect - solves the issue on both devices, pulseaudio works fine again without any transient behavior. I spent 15mn monkey-testing and the audio quality was always good when this patch is reverted.
I have no idea what the fixes were, but going from a somewhat random D3 exit problem to a 100% reproducible issue is problematic. I trust both Cezary and Ross did test this patch, but could it be that pulseaudio tests were skipped?
8ec7d6043263ecf250b9b7c0dd8ade899487538a is the first bad commit commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a Author: Cezary Rojewski cezary.rojewski@intel.com Date: Mon Mar 30 21:45:20 2020 +0200
ASoC: Intel: haswell: Power transition refactor
On Thu, Jun 18, 2020 at 6:19 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
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
I just tested speaker playback on Dell XPS13 and Samus Chromebook to double-check my UCM2 changes for SOF were indeed backwards compatible with the SST driver case. Well, my changes are fine but the kernel not so much.
With a 5.8-rc1 kernel w/ the SST driver, sounds played through pulseaudio are rendered too slowly with clicky artefacts. Using the alsa hw device works fine. In some cases, the sound rendered by PulseAudio become clear again after a while. Restarting the UI and testing degrades the audio again.
Reverting this patch - identified with git bisect - solves the issue on both devices, pulseaudio works fine again without any transient behavior. I spent 15mn monkey-testing and the audio quality was always good when this patch is reverted.
I have no idea what the fixes were, but going from a somewhat random D3 exit problem to a 100% reproducible issue is problematic. I trust both Cezary and Ross did test this patch, but could it be that pulseaudio tests were skipped?
We reverted this patch locally due to regressions and raised the issue with Cezary on Github, we got no response.
Curtis
8ec7d6043263ecf250b9b7c0dd8ade899487538a is the first bad commit commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a Author: Cezary Rojewski cezary.rojewski@intel.com Date: Mon Mar 30 21:45:20 2020 +0200
ASoC: Intel: haswell: Power transition refactor
On 2020-06-19 3:21 AM, Curtis Malainey wrote:
On Thu, Jun 18, 2020 at 6:19 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
We reverted this patch locally due to regressions and raised the issue with Cezary on Github, we got no response.
Curtis
8ec7d6043263ecf250b9b7c0dd8ade899487538a is the first bad commit commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a Author: Cezary Rojewski cezary.rojewski@intel.com Date: Mon Mar 30 21:45:20 2020 +0200
ASoC: Intel: haswell: Power transition refactor
Hello,
This is the very first time I see hear about the issue. You raised no issue Curtis, instead, you did write a comment mentioning me in Closed thread thesofproject/linux which isn't even the driver issue relates to.
If you scroll up a bit, in the very same thread there is a message notifying about official path for such issues. Said message was ack'ed by management before posting and that's why it's split from technical explanation.
We've received no response from Harsha and Cedrik about the issue being risen. Official HSD-ticket is left unchanged since my feedback from 3rd April.
Help me help you - don't wait until problem escalates. Adhere to official protocols, notify early and stay in contact. Last time when your 'SOF github-IntelSST BDW' ticket finally did arrive at my desk, I drove back to campus, borrowed the only SAMUS we have and by the end of the week, the problem was fixed. Monday Mar30 you had the official response and patches applied.
I've forwarded your issue to required entities within Intel so issue is tracked appropriately.
Regards, Czarek
On Fri, Jun 19, 2020 at 1:34 AM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2020-06-19 3:21 AM, Curtis Malainey wrote:
On Thu, Jun 18, 2020 at 6:19 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
We reverted this patch locally due to regressions and raised the issue with Cezary on Github, we got no response.
Curtis
8ec7d6043263ecf250b9b7c0dd8ade899487538a is the first bad commit commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a Author: Cezary Rojewski cezary.rojewski@intel.com Date: Mon Mar 30 21:45:20 2020 +0200
ASoC: Intel: haswell: Power transition refactorHello,
This is the very first time I see hear about the issue. You raised no issue Curtis, instead, you did write a comment mentioning me in Closed thread thesofproject/linux which isn't even the driver issue relates to.
That thread was directed to getting that fixed, you were active on the thread, regardless of whether the repo is or not. The bugs fell past their SLOs that were sent to your team through issue trackers which meant your team was not responding.
If you scroll up a bit, in the very same thread there is a message notifying about official path for such issues. Said message was ack'ed by management before posting and that's why it's split from technical explanation.
And if you scroll down you see this comment from Ross https://github.com/thesofproject/linux/pull/1842#issuecomment-606232124 We both attended meetings with your team where this request was ignored. It took you only a couple of days to fix once we took this approach yet it sat in the backlog for months. Forgive me if I have little faith in your "official path." This was a major blocker for us and it sat untouched.
We've received no response from Harsha and Cedrik about the issue being risen. Official HSD-ticket is left unchanged since my feedback from 3rd April.
When someone tags you in a comment it is your job to read it as a Github developer, regardless of the status of the thread.
Help me help you - don't wait until problem escalates. Adhere to official protocols, notify early and stay in contact. Last time when your 'SOF github-IntelSST BDW' ticket finally did arrive at my desk, I drove back to campus, borrowed the only SAMUS we have and by the end of the week, the problem was fixed. Monday Mar30 you had the official response and patches applied.
Yes, after months of trying to get this fixed through the "official path" and failing. Don't let the issue escalate outside the trackers in the first place. Be active and respond to high priority requests. I still have yet to see a response from intel regarding a solution on any of the bugs regarding this issue. Our PM pinged Carol many times during the course of getting this fixed with no response. I don't see why I should post there when posting here clearly got a quicker response. In fact your are actually CCed on the bug where the revert was posted and you didn't even respond. Don't feed me lines.
I've forwarded your issue to required entities within Intel so issue is tracked appropriately.
Regards, Czarek
On 2020-06-19 8:24 PM, Curtis Malainey wrote:
On Fri, Jun 19, 2020 at 1:34 AM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2020-06-19 3:21 AM, Curtis Malainey wrote:
On Thu, Jun 18, 2020 at 6:19 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
We reverted this patch locally due to regressions and raised the issue with Cezary on Github, we got no response.
Curtis
8ec7d6043263ecf250b9b7c0dd8ade899487538a is the first bad commit commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a Author: Cezary Rojewski cezary.rojewski@intel.com Date: Mon Mar 30 21:45:20 2020 +0200
ASoC: Intel: haswell: Power transition refactorHello,
This is the very first time I see hear about the issue. You raised no issue Curtis, instead, you did write a comment mentioning me in Closed thread thesofproject/linux which isn't even the driver issue relates to.
That thread was directed to getting that fixed, you were active on the thread, regardless of whether the repo is or not. The bugs fell past their SLOs that were sent to your team through issue trackers which meant your team was not responding.
If you scroll up a bit, in the very same thread there is a message notifying about official path for such issues. Said message was ack'ed by management before posting and that's why it's split from technical explanation.
And if you scroll down you see this comment from Ross https://github.com/thesofproject/linux/pull/1842#issuecomment-606232124 We both attended meetings with your team where this request was ignored. It took you only a couple of days to fix once we took this approach yet it sat in the backlog for months. Forgive me if I have little faith in your "official path." This was a major blocker for us and it sat untouched.
We've received no response from Harsha and Cedrik about the issue being risen. Official HSD-ticket is left unchanged since my feedback from 3rd April.
When someone tags you in a comment it is your job to read it as a Github developer, regardless of the status of the thread.
Help me help you - don't wait until problem escalates. Adhere to official protocols, notify early and stay in contact. Last time when your 'SOF github-IntelSST BDW' ticket finally did arrive at my desk, I drove back to campus, borrowed the only SAMUS we have and by the end of the week, the problem was fixed. Monday Mar30 you had the official response and patches applied.
Yes, after months of trying to get this fixed through the "official path" and failing. Don't let the issue escalate outside the trackers in the first place. Be active and respond to high priority requests. I still have yet to see a response from intel regarding a solution on any of the bugs regarding this issue. Our PM pinged Carol many times during the course of getting this fixed with no response. I don't see why I should post there when posting here clearly got a quicker response. In fact your are actually CCed on the bug where the revert was posted and you didn't even respond. Don't feed me lines.
I've forwarded your issue to required entities within Intel so issue is tracked appropriately.
Regards, Czarek
Let's make something clear - none of people from our companies found on the list who actively post changes or review them are decisive. Neither me nor you, Liam and Pierre and whoever else you find missing. That's the truth.
From Intel's perspective, I'm a resource. And those usually work with priority list in mind. If I were to take request from every mention/ tag/ CC/ To, you'd be waiting at least till Feb next year as that's roughly my current schedule. Under no circumstances treat SOF github or google-partner account as mean for assigning Intel's resources to fit your needs. You may have different deal with OTC but I'm not even part of SOF project team Curtis, thesofproject/linux isn't in my scope. If you insist on details, my github account was added to SOF project to help them deliver probe feature for you guys. When in need, priority list is shifted and resources are allocated where necessary. So, considering I've been helping at least 8 diff projects within past months, these should demand my full attention daily from then on?
No. That's management job - deal with issue prioritization as they see fit. I cannot speak for Harsha's, Cedrik's or Carol's teams and won't be defending them here. If what you say is true, it's sad official path failed so badly.
Thanks for being honest though, Curtis. I prefer facing the truth upfront. While as a dev lead I cannot escalate anything myself really, I'll certainly make sure your message is sound for those who can.
Regards, Czarek
On Fri, Jun 19, 2020 at 12:12 PM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2020-06-19 8:24 PM, Curtis Malainey wrote:
On Fri, Jun 19, 2020 at 1:34 AM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2020-06-19 3:21 AM, Curtis Malainey wrote:
On Thu, Jun 18, 2020 at 6:19 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
We reverted this patch locally due to regressions and raised the issue with Cezary on Github, we got no response.
Curtis
8ec7d6043263ecf250b9b7c0dd8ade899487538a is the first bad commit commit 8ec7d6043263ecf250b9b7c0dd8ade899487538a Author: Cezary Rojewski cezary.rojewski@intel.com Date: Mon Mar 30 21:45:20 2020 +0200
ASoC: Intel: haswell: Power transition refactorHello,
This is the very first time I see hear about the issue. You raised no issue Curtis, instead, you did write a comment mentioning me in Closed thread thesofproject/linux which isn't even the driver issue relates to.
That thread was directed to getting that fixed, you were active on the thread, regardless of whether the repo is or not. The bugs fell past their SLOs that were sent to your team through issue trackers which meant your team was not responding.
If you scroll up a bit, in the very same thread there is a message notifying about official path for such issues. Said message was ack'ed by management before posting and that's why it's split from technical explanation.
And if you scroll down you see this comment from Ross https://github.com/thesofproject/linux/pull/1842#issuecomment-606232124 We both attended meetings with your team where this request was ignored. It took you only a couple of days to fix once we took this approach yet it sat in the backlog for months. Forgive me if I have little faith in your "official path." This was a major blocker for us and it sat untouched.
We've received no response from Harsha and Cedrik about the issue being risen. Official HSD-ticket is left unchanged since my feedback from 3rd April.
When someone tags you in a comment it is your job to read it as a Github developer, regardless of the status of the thread.
Help me help you - don't wait until problem escalates. Adhere to official protocols, notify early and stay in contact. Last time when your 'SOF github-IntelSST BDW' ticket finally did arrive at my desk, I drove back to campus, borrowed the only SAMUS we have and by the end of the week, the problem was fixed. Monday Mar30 you had the official response and patches applied.
Yes, after months of trying to get this fixed through the "official path" and failing. Don't let the issue escalate outside the trackers in the first place. Be active and respond to high priority requests. I still have yet to see a response from intel regarding a solution on any of the bugs regarding this issue. Our PM pinged Carol many times during the course of getting this fixed with no response. I don't see why I should post there when posting here clearly got a quicker response. In fact your are actually CCed on the bug where the revert was posted and you didn't even respond. Don't feed me lines.
I've forwarded your issue to required entities within Intel so issue is tracked appropriately.
Regards, Czarek
Let's make something clear - none of people from our companies found on the list who actively post changes or review them are decisive. Neither me nor you, Liam and Pierre and whoever else you find missing. That's the truth.
Agreed
From Intel's perspective, I'm a resource. And those usually work with priority list in mind. If I were to take request from every mention/ tag/ CC/ To, you'd be waiting at least till Feb next year as that's roughly my current schedule. Under no circumstances treat SOF github or google-partner account as mean for assigning Intel's resources to fit your needs.
This is a matter of philosophy but I do disagree with this given that we have no access to your trackers but you do to ours, so that makes bug tracking hard if we won't unify on a single bug. There has to be collaboration somewhere.
You may have different deal with OTC but I'm not even part of SOF project team Curtis, thesofproject/linux isn't in my scope. If you insist on details, my github account was added to SOF project to help them deliver probe feature for you guys. When in need, priority list is shifted and resources are allocated where necessary. So, considering I've been helping at least 8 diff projects within past months, these should demand my full attention daily from then on?
Project balancing is part of the job. I balance a similar workload but does that mean that bugs can go unanswered. Acking is still better than silence.
No. That's management job - deal with issue prioritization as they see fit. I cannot speak for Harsha's, Cedrik's or Carol's teams and won't be defending them here. If what you say is true, it's sad official path failed so badly.
Thanks for being honest though, Curtis. I prefer facing the truth upfront. While as a dev lead I cannot escalate anything myself really, I'll certainly make sure your message is sound for those who can.
Thanks for propagating this information.
Curtis
Regards, Czarek
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
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
participants (5)
-
Cezary Rojewski -
Curtis Malainey -
Mark Brown -
Pierre-Louis Bossart -
Ross Zwisler