[alsa-devel] [PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile
SACNT register should be marked volatile since its WR and RD bits are cleared by SSI after completing the relevant operation. This unbreaks AC'97 register access.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name --- sound/soc/fsl/fsl_ssi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index e3abad5f980a..cc22354d7758 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -146,6 +146,7 @@ static bool fsl_ssi_volatile_reg(struct device *dev, unsigned int reg) case CCSR_SSI_SRX1: case CCSR_SSI_SISR: case CCSR_SSI_SFCSR: + case CCSR_SSI_SACNT: case CCSR_SSI_SACADD: case CCSR_SSI_SACDAT: case CCSR_SSI_SATAG: @@ -239,8 +240,9 @@ struct fsl_ssi_private { unsigned int baudclk_streams; unsigned int bitclk_freq;
- /*regcache for SFCSR*/ + /* regcache for volatile regs */ u32 regcache_sfcsr; + u32 regcache_sacnt;
/* DMA params */ struct snd_dmaengine_dai_dma_data dma_params_tx; @@ -1587,6 +1589,8 @@ static int fsl_ssi_suspend(struct device *dev)
regmap_read(regs, CCSR_SSI_SFCSR, &ssi_private->regcache_sfcsr); + regmap_read(regs, CCSR_SSI_SACNT, + &ssi_private->regcache_sacnt);
regcache_cache_only(regs, true); regcache_mark_dirty(regs); @@ -1605,6 +1609,8 @@ static int fsl_ssi_resume(struct device *dev) CCSR_SSI_SFCSR_RFWM1_MASK | CCSR_SSI_SFCSR_TFWM1_MASK | CCSR_SSI_SFCSR_RFWM0_MASK | CCSR_SSI_SFCSR_TFWM0_MASK, ssi_private->regcache_sfcsr); + regmap_write(regs, CCSR_SSI_SACNT, + ssi_private->regcache_sacnt);
return regcache_sync(regs); }
On Sun, Dec 20, 2015 at 6:30 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
SACNT register should be marked volatile since its WR and RD bits are cleared by SSI after completing the relevant operation. This unbreaks AC'97 register access.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name
Reviewed-by: Fabio Estevam fabio.estevam@nxp.com
On Sun, Dec 20, 2015 at 2:30 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
SACNT register should be marked volatile since its WR and RD bits are cleared by SSI after completing the relevant operation. This unbreaks AC'97 register access.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name
These patches seem okay, but can we hold off merging them until I get back from vacation and have a chance to review them?
Timur,
On Thu, Dec 24, 2015 at 2:12 PM, Timur Tabi timur@tabi.org wrote:
On Sun, Dec 20, 2015 at 2:30 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
SACNT register should be marked volatile since its WR and RD bits are cleared by SSI after completing the relevant operation. This unbreaks AC'97 register access.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name
These patches seem okay, but can we hold off merging them until I get back from vacation and have a chance to review them?
Have you had a chance to review these patches?
The patch
ASoC: fsl_ssi: mark SACNT register volatile
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 3f1c241f0f5f90046258e6b8d4aeb6463ffdc08e Mon Sep 17 00:00:00 2001
From: "Maciej S. Szmigiero" mail@maciej.szmigiero.name Date: Sun, 20 Dec 2015 21:30:25 +0100 Subject: [PATCH] ASoC: fsl_ssi: mark SACNT register volatile
SACNT register should be marked volatile since its WR and RD bits are cleared by SSI after completing the relevant operation. This unbreaks AC'97 register access.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")
Signed-off-by: Maciej S. Szmigiero mail@maciej.szmigiero.name Reviewed-by: Fabio Estevam fabio.estevam@nxp.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/fsl/fsl_ssi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index e3abad5f980a..cc22354d7758 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -146,6 +146,7 @@ static bool fsl_ssi_volatile_reg(struct device *dev, unsigned int reg) case CCSR_SSI_SRX1: case CCSR_SSI_SISR: case CCSR_SSI_SFCSR: + case CCSR_SSI_SACNT: case CCSR_SSI_SACADD: case CCSR_SSI_SACDAT: case CCSR_SSI_SATAG: @@ -239,8 +240,9 @@ struct fsl_ssi_private { unsigned int baudclk_streams; unsigned int bitclk_freq;
- /*regcache for SFCSR*/ + /* regcache for volatile regs */ u32 regcache_sfcsr; + u32 regcache_sacnt;
/* DMA params */ struct snd_dmaengine_dai_dma_data dma_params_tx; @@ -1587,6 +1589,8 @@ static int fsl_ssi_suspend(struct device *dev)
regmap_read(regs, CCSR_SSI_SFCSR, &ssi_private->regcache_sfcsr); + regmap_read(regs, CCSR_SSI_SACNT, + &ssi_private->regcache_sacnt);
regcache_cache_only(regs, true); regcache_mark_dirty(regs); @@ -1605,6 +1609,8 @@ static int fsl_ssi_resume(struct device *dev) CCSR_SSI_SFCSR_RFWM1_MASK | CCSR_SSI_SFCSR_TFWM1_MASK | CCSR_SSI_SFCSR_RFWM0_MASK | CCSR_SSI_SFCSR_TFWM0_MASK, ssi_private->regcache_sfcsr); + regmap_write(regs, CCSR_SSI_SACNT, + ssi_private->regcache_sacnt);
return regcache_sync(regs); }
Maciej S. Szmigiero wrote:
- regmap_write(regs, CCSR_SSI_SACNT,
ssi_private->regcache_sacnt);
So I'm not familiar with all of the regcache features, but I understand this patch. I was wondering if it makes sense to write the same exact value that was read previously. Isn't it possible for the WR or RD bits to change between fsl_ssi_suspend() and fsl_ssi_resume()? That is, should we be doing this instead?
u32 temp; regmap_read(regs, CCSR_SSI_SACNT, &temp); temp &= 0x18; // preserve WR and RD regmap_write(regs, CCSR_SSI_SACNT, (ssi_private->regcache_sacnt & ~0x18) | temp);
Hi Timur,
Thanks for review.
On 10.01.2016 22:33, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
- regmap_write(regs, CCSR_SSI_SACNT,
ssi_private->regcache_sacnt);So I'm not familiar with all of the regcache features, but I understand this patch. I was wondering if it makes sense to write the same exact value that was read previously. Isn't it possible for the WR or RD bits to change between fsl_ssi_suspend() and fsl_ssi_resume()?
These bits are only set in fsl_ssi_ac97_{read,write} which then wait 100usecs before returning. This should be enough for SSI core to finish the relevant operation and clear the bits again, so theoretically they shouldn't be set outside these functions.
However, if AC'97 register access is done concurrently with suspend or resume the read / written reg data might be corrupted.
It looks to me this is indeed possible since SSI PM callbacks are set in its platform driver struct but ASoC core only calls PM callbacks in snd_soc_dai_driver (which SSI driver don't set).
If I am correct with this reasoning then these callbacks need to be added to snd_soc_dai_driver but platform driver ones should still be provided in case the driver is loaded but the sound card is not yet registered.
I've CCed Zidan since he originally added PM support to this driver.
That is, should we be doing this instead?
u32 temp; regmap_read(regs, CCSR_SSI_SACNT, &temp); temp &= 0x18; // preserve WR and RD regmap_write(regs, CCSR_SSI_SACNT, (ssi_private->regcache_sacnt & ~0x18) | temp);
Maciej
participants (4)
-
Fabio Estevam -
Maciej S. Szmigiero -
Mark Brown -
Timur Tabi