[PATCH] ASoC: fsl_ssi: remove explicit register defaults

Subsystems: freescale soc sound drivers, sound, sound - soc layer / dynamic audio power management (asoc), the rest

STALE3876d

11 messages, 3 authors, 2016-02-01 · open the first message on its own page

[PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Maciej S. Szmigiero <hidden>
Date: 2016-01-18 19:08:06

There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

However, the cache needs to be fully populated at probe
time to avoid non-atomic allocations during register
access.

Special case here is imx21-class SSI, since
according to datasheet it don't have SACC{ST,EN,DIS}
regs.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <redacted>
---
 sound/soc/fsl/fsl_ssi.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

This replaces "ASoC: fsl_ssi: remove register defaults"
submission.

It may be good to delay merging this until commit
d51fe1f393a6 ("regmap: pass buffer size to regmap_raw_read() in regcache_hw_init()")
reaches ASoC tree since without that regmap fix
fsl_ssi will report error at probe time.
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..ed8de1035cda 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
 	struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-	{CCSR_SSI_SCR,     0x00000000},
-	{CCSR_SSI_SIER,    0x00003003},
-	{CCSR_SSI_STCR,    0x00000200},
-	{CCSR_SSI_SRCR,    0x00000200},
-	{CCSR_SSI_STCCR,   0x00040000},
-	{CCSR_SSI_SRCCR,   0x00040000},
-	{CCSR_SSI_SACNT,   0x00000000},
-	{CCSR_SSI_STMSK,   0x00000000},
-	{CCSR_SSI_SRMSK,   0x00000000},
-	{CCSR_SSI_SACCEN,  0x00000000},
-	{CCSR_SSI_SACCDIS, 0x00000000},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 	.val_bits = 32,
 	.reg_stride = 4,
 	.val_format_endian = REGMAP_ENDIAN_NATIVE,
-	.reg_defaults = fsl_ssi_reg_defaults,
-	.num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+	.num_reg_defaults_raw = CCSR_SSI_SACCDIS / sizeof(uint32_t) + 1,
 	.readable_reg = fsl_ssi_readable_reg,
 	.volatile_reg = fsl_ssi_volatile_reg,
 	.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
 	bool imx;
+	bool imx21regs; /* imx21-class SSI - no SACC{ST,EN,DIS} regs */
 	bool offline_config;
 	u32 sisr_write_mask;
 };
@@ -303,6 +289,7 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
 	.imx = true,
+	.imx21regs = true,
 	.offline_config = true,
 	.sisr_write_mask = 0,
 };
@@ -586,8 +573,12 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private *ssi_private)
 	 */
 	regmap_write(regs, CCSR_SSI_SACNT,
 			CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
-	regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-	regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+	/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+	if (!ssi_private->soc->imx21regs) {
+		regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+		regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+	}
 
 	/*
 	 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1388,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *iomem;
 	char name[64];
+	struct regmap_config regconfig = fsl_ssi_regconfig;
 
 	of_id = of_match_device(fsl_ssi_ids, &pdev->dev);
 	if (!of_id || !of_id->data)
@@ -1444,15 +1436,25 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		return PTR_ERR(iomem);
 	ssi_private->ssi_phys = res->start;
 
+	if (ssi_private->soc->imx21regs) {
+		/*
+		 * According to datasheet imx21-class SSI
+		 * don't have SACC{ST,EN,DIS} regs.
+		 */
+		regconfig.max_register = CCSR_SSI_SRMSK;
+		regconfig.num_reg_defaults_raw =
+			CCSR_SSI_SRMSK / sizeof(uint32_t) + 1;
+	}
+
 	ret = of_property_match_string(np, "clock-names", "ipg");
 	if (ret < 0) {
 		ssi_private->has_ipg_clk_name = false;
 		ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
-			&fsl_ssi_regconfig);
+			&regconfig);
 	} else {
 		ssi_private->has_ipg_clk_name = true;
 		ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,
-			"ipg", iomem, &fsl_ssi_regconfig);
+			"ipg", iomem, &regconfig);
 	}
 	if (IS_ERR(ssi_private->regs)) {
 		dev_err(&pdev->dev, "Failed to init register map\n");

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Fabio Estevam <festevam@gmail.com>
Date: 2016-01-18 22:14:26

On Mon, Jan 18, 2016 at 5:07 PM, Maciej S. Szmigiero
[off-list ref] wrote:
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

However, the cache needs to be fully populated at probe
time to avoid non-atomic allocations during register
access.

Special case here is imx21-class SSI, since
according to datasheet it don't have SACC{ST,EN,DIS}
regs.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <redacted>
Tested-by: Fabio Estevam <redacted>

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Fabio Estevam <festevam@gmail.com>
Date: 2016-02-01 12:05:48

Hi Maciej,

On Mon, Jan 18, 2016 at 5:07 PM, Maciej S. Szmigiero
[off-list ref] wrote:
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

However, the cache needs to be fully populated at probe
time to avoid non-atomic allocations during register
access.

Special case here is imx21-class SSI, since
according to datasheet it don't have SACC{ST,EN,DIS}
regs.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <redacted>
I know I have already tested this and it worked fine on a mx6sabresd,
but running linux-next 20160201 on a mx6sl-evk the ssi driver does not
probe anymore:

[    2.216954] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[    2.223412] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[    2.230258] imx-wm8962 sound: ASoC: CPU DAI 202c000.ssi not registered
[    2.236806] imx-wm8962 sound: snd_soc_register_card failed (-517)
[    2.244782] snvs_rtc 20cc000.snvs:snvs-rtc-lp: setting system clock
to 1970-01-01 00:01:14 UTC (74)
[    2.285864] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[    2.292335] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[    2.299572] imx-wm8962 sound: ASoC: CPU DAI 202c000.ssi not registered
[    2.306121] imx-wm8962 sound: snd_soc_register_card failed (-517)

Reverting this patch fixes the problem.

Wandboard also has the same issue:

http://arm-soc.lixom.net/bootlogs/next/next-20160201/wandboard-arm-imx_v6_v7_defconfig.html

Any suggestions?

Thanks

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Maciej S. Szmigiero <hidden>
Date: 2016-02-01 12:08:19

Hi Fabio,

On 01.02.2016 13:05, Fabio Estevam wrote:
Hi Maciej,

On Mon, Jan 18, 2016 at 5:07 PM, Maciej S. Szmigiero
[off-list ref] wrote:
quoted
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

However, the cache needs to be fully populated at probe
time to avoid non-atomic allocations during register
access.

Special case here is imx21-class SSI, since
according to datasheet it don't have SACC{ST,EN,DIS}
regs.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <redacted>
I know I have already tested this and it worked fine on a mx6sabresd,
but running linux-next 20160201 on a mx6sl-evk the ssi driver does not
probe anymore:

[    2.216954] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[    2.223412] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[    2.230258] imx-wm8962 sound: ASoC: CPU DAI 202c000.ssi not registered
[    2.236806] imx-wm8962 sound: snd_soc_register_card failed (-517)
[    2.244782] snvs_rtc 20cc000.snvs:snvs-rtc-lp: setting system clock
to 1970-01-01 00:01:14 UTC (74)
[    2.285864] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[    2.292335] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[    2.299572] imx-wm8962 sound: ASoC: CPU DAI 202c000.ssi not registered
[    2.306121] imx-wm8962 sound: snd_soc_register_card failed (-517)

Reverting this patch fixes the problem.

Wandboard also has the same issue:

http://arm-soc.lixom.net/bootlogs/next/next-20160201/wandboard-arm-imx_v6_v7_defconfig.html

Any suggestions?

Thanks
Is regmap patch from 
http://www.spinics.net/lists/kernel/msg2161934.html
applied to the tested tree?

Best regards,
Maciej

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Fabio Estevam <festevam@gmail.com>
Date: 2016-02-01 12:13:39

Hi Maciej,

On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
[off-list ref] wrote:
Is regmap patch from
http://www.spinics.net/lists/kernel/msg2161934.html
applied to the tested tree?
Yes, linux-next 20160201 contains this patch.

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Maciej S. Szmigiero <hidden>
Date: 2016-02-01 12:26:07

On 01.02.2016 13:13, Fabio Estevam wrote:
Hi Maciej,

On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
[off-list ref] wrote:
quoted
Is regmap patch from
http://www.spinics.net/lists/kernel/msg2161934.html
applied to the tested tree?
Yes, linux-next 20160201 contains this patch.
Hmm, I will try to build this tree on UDOO board
today and see what happens.

Maciej

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Maciej S. Szmigiero <hidden>
Date: 2016-02-01 16:58:26

On 01.02.2016 13:25, Maciej S. Szmigiero wrote:
On 01.02.2016 13:13, Fabio Estevam wrote:
quoted
Hi Maciej,

On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
[off-list ref] wrote:
quoted
Is regmap patch from
http://www.spinics.net/lists/kernel/msg2161934.html
applied to the tested tree?
Yes, linux-next 20160201 contains this patch.
Hmm, I will try to build this tree on UDOO board
today and see what happens.
Looks like the problem occurs because commit
922a9f936e40 ("regmap: mmio: Convert to regmap_bus and fix accessor usage")
has removed ->read callback from MMIO regmap
implementation but it is used (via regmap_raw_read()) by
regcache code to initialize cache if reg default values
weren't provided explicitly.

If I revert this commit the SSI probes successfully again.

Looks like a possible solution would be to change
regmap_raw_read() to do read using _regmap_read in
case the cache is bypassed and there is no ->read
callback defined for regmap implementation.

Maciej

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Mark Brown <broonie@kernel.org>
Date: 2016-02-01 21:10:34

On Mon, Feb 01, 2016 at 05:58:06PM +0100, Maciej S. Szmigiero wrote:
Looks like a possible solution would be to change
regmap_raw_read() to do read using _regmap_read in
case the cache is bypassed and there is no ->read
callback defined for regmap implementation.
No, that's completely broken.  We can't do a raw read from a regmap that
doesn't offer raw access and we shouldn't pretend to do so.  If the
caller is capable of substituting a register by register read the caller
should take responsibility for that.

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Maciej S. Szmigiero <hidden>
Date: 2016-02-01 21:31:11

On 01.02.2016 22:10, Mark Brown wrote:
On Mon, Feb 01, 2016 at 05:58:06PM +0100, Maciej S. Szmigiero wrote:
quoted
Looks like a possible solution would be to change
regmap_raw_read() to do read using _regmap_read in
case the cache is bypassed and there is no ->read
callback defined for regmap implementation.
No, that's completely broken.  We can't do a raw read from a regmap that
doesn't offer raw access and we shouldn't pretend to do so.  If the
caller is capable of substituting a register by register read the caller
should take responsibility for that.
So can regcache initialization be changed to use register by register read
in case raw read fails?

Since other option for drivers like SSI which are memory mapped and
don't offer ability to reset their register values to default would be to
explicitly write driver hardcoded defaults also by doing
register by register write on probe time.

But this would force driver to keep the defaults which I think is bad
(especially for driver that supports multiple generations of hardware
like fsl_ssi which may have different default values).

Maciej

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Fabio Estevam <festevam@gmail.com>
Date: 2016-02-01 21:34:17

On Mon, Feb 1, 2016 at 7:30 PM, Maciej S. Szmigiero
[off-list ref] wrote:
So can regcache initialization be changed to use register by register read
in case raw read fails?

Since other option for drivers like SSI which are memory mapped and
don't offer ability to reset their register values to default would be to
explicitly write driver hardcoded defaults also by doing
register by register write on probe time.
Yes, I had to do the same for sgtl5000:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/sound/soc/codecs/sgtl5000.c?id=af8ee11209e749c75eabf32b2a4ca631f396acf8

Would this approach work here too?

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

From: Mark Brown <broonie@kernel.org>
Date: 2016-02-01 21:37:42

On Mon, Feb 01, 2016 at 10:30:53PM +0100, Maciej S. Szmigiero wrote:
On 01.02.2016 22:10, Mark Brown wrote:
quoted
No, that's completely broken.  We can't do a raw read from a regmap that
doesn't offer raw access and we shouldn't pretend to do so.  If the
caller is capable of substituting a register by register read the caller
should take responsibility for that.
So can regcache initialization be changed to use register by register read
in case raw read fails?
Well, we *do* have full source code access!  I'm just blind writing a
patch to do that.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help