[alsa-devel] [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
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.
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 mail@maciej.szmigiero.name --- sound/soc/fsl/fsl_ssi.c | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 40dfd8a36484..f6ef7d64acb3 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,6 @@ 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), .readable_reg = fsl_ssi_readable_reg, .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg,
On Sun, Dec 20, 2015 at 6:33 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name 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.
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 mail@maciej.szmigiero.name
Reviewed-by: Fabio Estevam fabio.estevam@nxp.com
The patch
ASoC: fsl_ssi: remove register defaults
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 9f2d0be4c64f96ac575d164ede47becec1eececf Mon Sep 17 00:00:00 2001
From: "Maciej S. Szmigiero" mail@maciej.szmigiero.name Date: Sun, 20 Dec 2015 21:33:00 +0100 Subject: [PATCH] ASoC: fsl_ssi: remove register defaults
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.
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 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 | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 40dfd8a36484..f6ef7d64acb3 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,6 @@ 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), .readable_reg = fsl_ssi_readable_reg, .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg,
Maciej S. Szmigiero 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.
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. Szmigieromail@maciej.szmigiero.name
Acked-by: Timur Tabi timur@tabi.org
I'm surprised that we're actually encouraging drivers to contain hard-coded register values.
Hi Maciej,
On Sun, Dec 20, 2015 at 6:33 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name 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.
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 mail@maciej.szmigiero.name
This patch causes the following issue in linux-next:
[ 2.526984] ------------[ cut here ]------------ [ 2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xf4/0x124() [ 2.540771] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)) [ 2.546175] Modules linked in: [ 2.549447] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc8-next-20160111 #204 [ 2.557021] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 2.563553] Backtrace: [ 2.566040] [<c00136d8>] (dump_backtrace) from [<c0013874>] (show_stack+0x18/0x1c) [ 2.573615] r6:00000ac3 r5:00000000 r4:00000000 r3:00000000 [ 2.579362] [<c001385c>] (show_stack) from [<c02de89c>] (dump_stack+0x88/0xa4) [ 2.586607] [<c02de814>] (dump_stack) from [<c002bcac>] (warn_slowpath_common+0x80/0xbc) [ 2.594702] r5:c0071ed0 r4:ef055b90 [ 2.598326] [<c002bc2c>] (warn_slowpath_common) from [<c002bd8c>] (warn_slowpath_fmt+0x38/0x40) [ 2.607028] r8:00000004 r7:00000004 r6:024080c0 r5:024080c0 r4:60000093 [ 2.613829] [<c002bd58>] (warn_slowpath_fmt) from [<c0071ed0>] (lockdep_trace_alloc+0xf4/0x124) [ 2.622532] r3:c09a1634 r2:c099dc0c [ 2.626161] [<c0071ddc>] (lockdep_trace_alloc) from [<c010bc0c>] (kmem_cache_alloc+0x30/0x174) [ 2.634778] r4:ef001f00 r3:c0b02a88 [ 2.638407] [<c010bbdc>] (kmem_cache_alloc) from [<c03edac8>] (regcache_rbtree_write+0x150/0x724) [ 2.647283] r10:00000000 r9:00000010 r8:00000004 r7:00000004 r6:0000002c r5:00000000 [ 2.655203] r4:00000000 [ 2.657767] [<c03ed978>] (regcache_rbtree_write) from [<c03ec7d8>] (regcache_write+0x5c/0x64) [ 2.666295] r10:ef0f1c80 r9:ef0f6500 r8:00000001 r7:ef055cbc r6:00000010 r5:00000000 [ 2.674215] r4:eeaaf000 [ 2.676778] [<c03ec77c>] (regcache_write) from [<c03ea4c0>] (_regmap_read+0xa8/0xc0) [ 2.684526] r6:00000000 [ 2.685780] mmc2: new DDR MMC card at address 0001 [ 2.686495] mmcblk1: mmc2:0001 SEM08G 7.40 GiB [ 2.686792] mmcblk1boot0: mmc2:0001 SEM08G partition 1 2.00 MiB [ 2.687092] mmcblk1boot1: mmc2:0001 SEM08G partition 2 2.00 MiB [ 2.687388] mmcblk1rpmb: mmc2:0001 SEM08G partition 3 128 KiB [ 2.713792] r5:00000010 r4:eeaaf000 r3:00000000 [ 2.718660] [<c03ea418>] (_regmap_read) from [<c03ea51c>] (regmap_read+0x44/0x64) [ 2.726147] r7:00001000 r6:ef055cbc r5:00000010 r4:eeaaf000 [ 2.731890] [<c03ea4d8>] (regmap_read) from [<c05bbcd8>] (_fsl_ssi_set_dai_fmt+0xa4/0x440) [ 2.740159] r6:00001001 r5:eeaaf000 r4:eeaaee10 r3:01400454 [ 2.745897] [<c05bbc34>] (_fsl_ssi_set_dai_fmt) from [<c05bc090>] (fsl_ssi_set_dai_fmt+0x1c/0x20) [ 2.754773] r10:ef0f1c80 r8:ef118800 r7:00001001 r6:00000000 r5:00000001 r4:ef0f1700 [ 2.762706] [<c05bc074>] (fsl_ssi_set_dai_fmt) from [<c059dbe8>] (snd_soc_runtime_set_dai_fmt+0x104/0x154) [ 2.772375] [<c059dae4>] (snd_soc_runtime_set_dai_fmt) from [<c05a289c>] (snd_soc_register_card+0xc14/0xdd0) [ 2.782206] r10:eeaa889c r9:00000002 r8:eeaa8810 r7:ef118800 r6:00000001 r5:00000000 [ 2.790126] r4:ef0f1c80 r3:00000000 [ 2.793750] [<c05a1c88>] (snd_soc_register_card) from [<c05ae1f0>] (devm_snd_soc_register_card+0x38/0x78) [ 2.803321] r10:ef20b420 r9:00000000 r8:eeaa8810 r7:ef1cf410 r6:eeaa889c r5:ef0f6490 [ 2.811240] r4:eeaa889c [ 2.813806] [<c05ae1b8>] (devm_snd_soc_register_card) from [<c05c2668>] (imx_wm8962_probe+0x2fc/0x3a0) [ 2.823116] r7:ef1cf410 r6:eeaa889c r5:c085ad98 r4:ef1cf400 [ 2.828858] [<c05c236c>] (imx_wm8962_probe) from [<c03d3f40>] (platform_drv_probe+0x58/0xb4) [ 2.837300] r10:00000000 r9:000000de r8:c0b645f4 r7:c0b645f4 r6:fffffdfb r5:ef1cf410 [ 2.845220] r4:fffffffe [ 2.847788] [<c03d3ee8>] (platform_drv_probe) from [<c03d26fc>] (driver_probe_device+0x1f8/0x2b4) [ 2.856665] r7:00000000 r6:c1379780 r5:c1379778 r4:ef1cf410 [ 2.862405] [<c03d2504>] (driver_probe_device) from [<c03d2854>] (__driver_attach+0x9c/0xa0) [ 2.870846] r10:00000000 r8:c0ad7b30 r7:00000000 r6:ef1cf444 r5:c0b645f4 r4:ef1cf410 [ 2.878776] [<c03d27b8>] (__driver_attach) from [<c03d0ae8>] (bus_for_each_dev+0x5c/0x90) [ 2.886956] r6:c03d27b8 r5:c0b645f4 r4:00000000 r3:ef1b695c [ 2.892695] [<c03d0a8c>] (bus_for_each_dev) from [<c03d1f14>] (driver_attach+0x20/0x28) [ 2.900703] r6:c0b2f9f0 r5:ef0f1400 r4:c0b645f4 [ 2.905383] [<c03d1ef4>] (driver_attach) from [<c03d1c14>] (bus_add_driver+0xec/0x1fc) [ 2.913313] [<c03d1b28>] (bus_add_driver) from [<c03d30f8>] (driver_register+0x80/0xfc) [ 2.921320] r7:c0ae684c r6:ef0f63c0 r5:c0b06188 r4:c0b645f4 [ 2.927059] [<c03d3078>] (driver_register) from [<c03d3dcc>] (__platform_driver_register+0x38/0x4c) [ 2.936109] r5:c0b06188 r4:c0b06188 [ 2.939733] [<c03d3d94>] (__platform_driver_register) from [<c0ad7b48>] (imx_wm8962_driver_init+0x18/0x20) [ 2.949398] [<c0ad7b30>] (imx_wm8962_driver_init) from [<c00098dc>] (do_one_initcall+0x88/0x1e4) [ 2.958200] [<c0009854>] (do_one_initcall) from [<c0a8ce84>] (kernel_init_freeable+0x11c/0x1f0) [ 2.966902] r10:c0ae6858 r9:000000de r8:00000000 r7:c0ae684c r6:c0b6f540 r5:00000006 [ 2.974822] r4:c0af86ec [ 2.977386] [<c0a8cd68>] (kernel_init_freeable) from [<c07c7448>] (kernel_init+0x10/0xf8) [ 2.985566] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c07c7438 [ 2.993486] r4:00000000 [ 2.996050] [<c07c7438>] (kernel_init) from [<c000ff30>] (ret_from_fork+0x14/0x24) [ 3.003624] r4:00000000 r3:00000000 [ 3.007256] ---[ end trace c4f2be75666d4572 ]---
On Mon, Jan 11, 2016 at 10:04 AM, Fabio Estevam festevam@gmail.com wrote:
This patch causes the following issue in linux-next:
[ 2.526984] ------------[ cut here ]------------ [ 2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xf4/0x124() [ 2.540771] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)) [ 2.546175] Modules linked in: [ 2.549447] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc8-next-20160111 #204 [ 2.557021] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 2.563553] Backtrace: [ 2.566040] [<c00136d8>] (dump_backtrace) from [<c0013874>] (show_stack+0x18/0x1c) [ 2.573615] r6:00000ac3 r5:00000000 r4:00000000 r3:00000000 [ 2.579362] [<c001385c>] (show_stack) from [<c02de89c>] (dump_stack+0x88/0xa4) [ 2.586607] [<c02de814>] (dump_stack) from [<c002bcac>] (warn_slowpath_common+0x80/0xbc) [ 2.594702] r5:c0071ed0 r4:ef055b90 [ 2.598326] [<c002bc2c>] (warn_slowpath_common) from [<c002bd8c>] (warn_slowpath_fmt+0x38/0x40) [ 2.607028] r8:00000004 r7:00000004 r6:024080c0 r5:024080c0 r4:60000093 [ 2.613829] [<c002bd58>] (warn_slowpath_fmt) from [<c0071ed0>] (lockdep_trace_alloc+0xf4/0x124) [ 2.622532] r3:c09a1634 r2:c099dc0c [ 2.626161] [<c0071ddc>] (lockdep_trace_alloc) from [<c010bc0c>] (kmem_cache_alloc+0x30/0x174) [ 2.634778] r4:ef001f00 r3:c0b02a88 [ 2.638407] [<c010bbdc>] (kmem_cache_alloc) from [<c03edac8>] (regcache_rbtree_write+0x150/0x724) [ 2.647283] r10:00000000 r9:00000010 r8:00000004 r7:00000004 r6:0000002c r5:00000000 [ 2.655203] r4:00000000 [ 2.657767] [<c03ed978>] (regcache_rbtree_write) from [<c03ec7d8>] (regcache_write+0x5c/0x64)
This fixes the warning:
--- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -180,7 +180,6 @@ static const struct regmap_config fsl_ssi_regconfig = { .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg, .writeable_reg = fsl_ssi_writeable_reg, - .cache_type = REGCACHE_RBTREE, };
Is this the correct fix?
Hi Fabio,
Thanks for testing.
On 11.01.2016 13:10, Fabio Estevam wrote:
On Mon, Jan 11, 2016 at 10:04 AM, Fabio Estevam festevam@gmail.com wrote:
This patch causes the following issue in linux-next:
[ 2.526984] ------------[ cut here ]------------ [ 2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xf4/0x124() [ 2.540771] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)) [ 2.546175] Modules linked in: [ 2.549447] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc8-next-20160111 #204 [ 2.557021] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 2.563553] Backtrace: [ 2.566040] [<c00136d8>] (dump_backtrace) from [<c0013874>] (show_stack+0x18/0x1c) [ 2.573615] r6:00000ac3 r5:00000000 r4:00000000 r3:00000000 [ 2.579362] [<c001385c>] (show_stack) from [<c02de89c>] (dump_stack+0x88/0xa4) [ 2.586607] [<c02de814>] (dump_stack) from [<c002bcac>] (warn_slowpath_common+0x80/0xbc) [ 2.594702] r5:c0071ed0 r4:ef055b90 [ 2.598326] [<c002bc2c>] (warn_slowpath_common) from [<c002bd8c>] (warn_slowpath_fmt+0x38/0x40) [ 2.607028] r8:00000004 r7:00000004 r6:024080c0 r5:024080c0 r4:60000093 [ 2.613829] [<c002bd58>] (warn_slowpath_fmt) from [<c0071ed0>] (lockdep_trace_alloc+0xf4/0x124) [ 2.622532] r3:c09a1634 r2:c099dc0c [ 2.626161] [<c0071ddc>] (lockdep_trace_alloc) from [<c010bc0c>] (kmem_cache_alloc+0x30/0x174) [ 2.634778] r4:ef001f00 r3:c0b02a88 [ 2.638407] [<c010bbdc>] (kmem_cache_alloc) from [<c03edac8>] (regcache_rbtree_write+0x150/0x724) [ 2.647283] r10:00000000 r9:00000010 r8:00000004 r7:00000004 r6:0000002c r5:00000000 [ 2.655203] r4:00000000 [ 2.657767] [<c03ed978>] (regcache_rbtree_write) from [<c03ec7d8>] (regcache_write+0x5c/0x64)
This fixes the warning:
--- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -180,7 +180,6 @@ static const struct regmap_config fsl_ssi_regconfig = { .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg, .writeable_reg = fsl_ssi_writeable_reg,
.cache_type = REGCACHE_RBTREE,};
Is this the correct fix?
This will disable register cache so it isn't right. Could you try REGCACHE_FLAT instead, please?
Looks like the problem here is rbtree cache does some non-atomic allocations in read / write path when not supplied with default register values.
Best regards, Maciej Szmigiero
Hi Maciej,
On Mon, Jan 11, 2016 at 11:57 AM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
Hi Fabio,
This will disable register cache so it isn't right. Could you try REGCACHE_FLAT instead, please?
Yes, with REGCACHE_FLAT I don't get the warning.
Regards,
Fabio Estevam
Hi Fabio,
On 11.01.2016 15:05, Fabio Estevam wrote:
Hi Maciej,
On Mon, Jan 11, 2016 at 11:57 AM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
Hi Fabio,
This will disable register cache so it isn't right. Could you try REGCACHE_FLAT instead, please?
Yes, with REGCACHE_FLAT I don't get the warning.
Regards,
Is it possible for you to test the following updated patch?
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 40dfd8a36484..16ee5745c992 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) { @@ -184,14 +170,27 @@ static bool fsl_ssi_writeable_reg(struct device *dev, unsigned int reg) } }
+static const struct regmap_config fsl_ssi_regconfig_imx21 = { + .max_register = CCSR_SSI_SRMSK, + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .val_format_endian = REGMAP_ENDIAN_NATIVE, + .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1, + .readable_reg = fsl_ssi_readable_reg, + .volatile_reg = fsl_ssi_volatile_reg, + .precious_reg = fsl_ssi_precious_reg, + .writeable_reg = fsl_ssi_writeable_reg, + .cache_type = REGCACHE_RBTREE, +}; + static const struct regmap_config fsl_ssi_regconfig = { .max_register = CCSR_SSI_SACCDIS, .reg_bits = 32, .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 / 4 + 1, .readable_reg = fsl_ssi_readable_reg, .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg, @@ -201,6 +200,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
struct fsl_ssi_soc_data { bool imx; + bool imx21regs; bool offline_config; u32 sisr_write_mask; }; @@ -295,6 +295,7 @@ struct fsl_ssi_private {
static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = { .imx = false, + .imx21regs = false, .offline_config = true, .sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC | CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | @@ -303,12 +304,14 @@ 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, };
static struct fsl_ssi_soc_data fsl_ssi_imx35 = { .imx = true, + .imx21regs = false, .offline_config = true, .sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC | CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | @@ -317,6 +320,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
static struct fsl_ssi_soc_data fsl_ssi_imx51 = { .imx = true, + .imx21regs = false, .offline_config = false, .sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1, @@ -586,8 +590,11 @@ 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); + + 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 +1404,7 @@ static int fsl_ssi_probe(struct platform_device *pdev) struct resource *res; void __iomem *iomem; char name[64]; + const struct regmap_config *regconfig;
of_id = of_match_device(fsl_ssi_ids, &pdev->dev); if (!of_id || !of_id->data) @@ -1444,15 +1452,21 @@ static int fsl_ssi_probe(struct platform_device *pdev) return PTR_ERR(iomem); ssi_private->ssi_phys = res->start;
+ if (ssi_private->soc->imx21regs) + regconfig = &fsl_ssi_regconfig_imx21; + else + regconfig = &fsl_ssi_regconfig; + 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");
You'll also need to apply regmap fix from http://www.spinics.net/lists/kernel/msg2161934.html to make it work.
Thanks in advance.
Fabio Estevam
Best regards, Maciej Szmigiero
Maciej S. Szmigiero wrote:
+static const struct regmap_config fsl_ssi_regconfig_imx21 = {
- .max_register = CCSR_SSI_SRMSK,
- .reg_bits = 32,
- .val_bits = 32,
- .reg_stride = 4,
- .val_format_endian = REGMAP_ENDIAN_NATIVE,
- .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
- .readable_reg = fsl_ssi_readable_reg,
- .volatile_reg = fsl_ssi_volatile_reg,
- .precious_reg = fsl_ssi_precious_reg,
- .writeable_reg = fsl_ssi_writeable_reg,
- .cache_type = REGCACHE_RBTREE,
+};
- static const struct regmap_config fsl_ssi_regconfig = { .max_register = CCSR_SSI_SACCDIS, .reg_bits = 32, .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 / 4 + 1, .readable_reg = fsl_ssi_readable_reg, .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg,
Is this really necessary? Why do we need separate register configs for one specific SOC? There are already too many "if (some_stupid_imx_variant)" blocks in this driver.
On 17.01.2016 01:10, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
+static const struct regmap_config fsl_ssi_regconfig_imx21 = {
- .max_register = CCSR_SSI_SRMSK,
- .reg_bits = 32,
- .val_bits = 32,
- .reg_stride = 4,
- .val_format_endian = REGMAP_ENDIAN_NATIVE,
- .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
- .readable_reg = fsl_ssi_readable_reg,
- .volatile_reg = fsl_ssi_volatile_reg,
- .precious_reg = fsl_ssi_precious_reg,
- .writeable_reg = fsl_ssi_writeable_reg,
- .cache_type = REGCACHE_RBTREE,
+};
- static const struct regmap_config fsl_ssi_regconfig = { .max_register = CCSR_SSI_SACCDIS, .reg_bits = 32, .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 / 4 + 1, .readable_reg = fsl_ssi_readable_reg, .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg,
Is this really necessary? Why do we need separate register configs for one specific SOC? There are already too many "if (some_stupid_imx_variant)" blocks in this driver.
This is because (at least according to the datasheet) imx21-class SSI registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree (drivers/mxc/ssi/registers.h) seems to confirm that these registers aren't present at least on some SSI (or SoC) models.
Best regards, Maciej Szmigiero
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree (drivers/mxc/ssi/registers.h) seems to confirm that these registers aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
On 17.01.2016 06:16, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree (drivers/mxc/ssi/registers.h) seems to confirm that these registers aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct used then as template - I will post updated patch.
Maciej
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
On 17.01.2016 06:16, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree (drivers/mxc/ssi/registers.h) seems to confirm that these registers aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct used then as template - I will post updated patch.
Updated patch: diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 40dfd8a36484..105de76dd2fc 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 / 4 + 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; bool offline_config; u32 sisr_write_mask; }; @@ -295,6 +281,7 @@ struct fsl_ssi_private {
static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = { .imx = false, + .imx21regs = false, .offline_config = true, .sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC | CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | @@ -303,12 +290,14 @@ 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, };
static struct fsl_ssi_soc_data fsl_ssi_imx35 = { .imx = true, + .imx21regs = false, .offline_config = true, .sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC | CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | @@ -317,6 +306,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
static struct fsl_ssi_soc_data fsl_ssi_imx51 = { .imx = true, + .imx21regs = false, .offline_config = false, .sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1, @@ -586,8 +576,11 @@ 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); + + 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 +1390,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 +1438,22 @@ 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 have less regs */ + regconfig.max_register = CCSR_SSI_SRMSK; + regconfig.num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 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); + ®config); } 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, + ®config); } if (IS_ERR(ssi_private->regs)) { dev_err(&pdev->dev, "Failed to init register map\n");
Also needs regmap fix from http://www.spinics.net/lists/kernel/msg2161934.html
Maciej Szmigiero
Maciej S. Szmigiero wrote:
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
On 17.01.2016 06:16, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree (drivers/mxc/ssi/registers.h) seems to confirm that these registers aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct used then as template - I will post updated patch.
Updated patch: diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 40dfd8a36484..105de76dd2fc 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 / 4 + 1,
Replace "4" with "sizeof(uint32_t).
.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;
Please add a comment explaining why this is needed.
bool offline_config; u32 sisr_write_mask; }; @@ -295,6 +281,7 @@ struct fsl_ssi_private {
static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = { .imx = false,
- .imx21regs = false,
This is unnecessary. The default is already 0 (false).
.offline_config = true, .sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC | CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | @@ -303,12 +290,14 @@ 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, };
static struct fsl_ssi_soc_data fsl_ssi_imx35 = { .imx = true,
.imx21regs = false,
Same here.
.offline_config = true, .sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC | CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | @@ -317,6 +306,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
static struct fsl_ssi_soc_data fsl_ssi_imx51 = { .imx = true,
- .imx21regs = false, .offline_config = false, .sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 | CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1,
@@ -586,8 +576,11 @@ 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);
- if (!ssi_private->soc->imx21regs) {
regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);regmap_write(regs, CCSR_SSI_SACCEN, 0x300);- }
This needs a comment.
/* * Enable SSI, Transmit and Receive. AC97 has to communicate with the @@ -1397,6 +1390,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 +1438,22 @@ 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 have less regs */
First of all, it would be "fewer regs", but even better would be to say that certain regs don't exist.
However, I wonder if this patch is necessary at all. If the regs don't exist on an i.MX 21, does it really matter if we write to them?
regconfig.max_register = CCSR_SSI_SRMSK;regconfig.num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 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);
®config);
"ipg", iomem, &fsl_ssi_regconfig);
"ipg", iomem,®config);
What's wrong with the original indentation? It looks nicer than what you're doing here.
} if (IS_ERR(ssi_private->regs)) { dev_err(&pdev->dev, "Failed to init register map\n");
Also needs regmap fix from http://www.spinics.net/lists/kernel/msg2161934.html
Maciej Szmigiero
On 17.01.2016 19:38, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
On 17.01.2016 06:16, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree (drivers/mxc/ssi/registers.h) seems to confirm that these registers aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have two structures?
Looks like it can be done with just one static regmap config struct used then as template - I will post updated patch.
Patch updated according to Timur's suggestions (needs regmap fix): 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); + ®config); } 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, ®config); } if (IS_ERR(ssi_private->regs)) { dev_err(&pdev->dev, "Failed to init register map\n");
However, I wonder if this patch is necessary at all. If the regs don't exist on an i.MX 21, does it really matter if we write to them?
At least i.MX6 datasheet SSI description has the following information: "Transfer bus errors are generated upon response to the following: * Write transfer to a read-only register. * Read or write access to a register space beyond the last populated register of the SSI in its memory map (up until the end of the allocated memory address range of the SSI)."
Maciej Szmigiero
Hi Maciej,
On Sun, Jan 17, 2016 at 8:02 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
Patch updated according to Timur's suggestions (needs regmap fix): diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
Tested your patch against linux-next and it did not give me any warnings:
Tested-by: Fabio Estevam fabio.estevam@nxp.com
Hi Fabio,
On 18.01.2016 13:51, Fabio Estevam wrote:
Hi Maciej,
On Sun, Jan 17, 2016 at 8:02 PM, Maciej S. Szmigiero mail@maciej.szmigiero.name wrote:
Patch updated according to Timur's suggestions (needs regmap fix): diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
Tested your patch against linux-next and it did not give me any warnings:
Tested-by: Fabio Estevam fabio.estevam@nxp.com
Thanks for testing, I have submitted this version now.
Maciej
The patch
ASoC: fsl_ssi: remove explicit register defaults
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 5c408fee254633a5be69505bc86c6b034f871ab4 Mon Sep 17 00:00:00 2001
From: "Maciej S. Szmigiero" mail@maciej.szmigiero.name Date: Mon, 18 Jan 2016 20:07:44 +0100 Subject: [PATCH] ASoC: fsl_ssi: remove explicit register defaults
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 mail@maciej.szmigiero.name Tested-by: Fabio Estevam fabio.estevam@nxp.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/fsl/fsl_ssi.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
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); + ®config); } 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, ®config); } if (IS_ERR(ssi_private->regs)) { dev_err(&pdev->dev, "Failed to init register map\n");
The patch
ASoC: fsl_ssi: remove explicit register defaults
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 6139b1b184d5f6257f692dc814dbcd47e4cf90f1 Mon Sep 17 00:00:00 2001
From: "Maciej S. Szmigiero" mail@maciej.szmigiero.name Date: Mon, 18 Jan 2016 20:07:44 +0100 Subject: [PATCH] ASoC: fsl_ssi: remove explicit register defaults
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 mail@maciej.szmigiero.name Tested-by: Fabio Estevam fabio.estevam@nxp.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/fsl/fsl_ssi.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 40dfd8a..ed8de10 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); + ®config); } 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, ®config); } if (IS_ERR(ssi_private->regs)) { dev_err(&pdev->dev, "Failed to init register map\n");
On Mon, Jan 11, 2016 at 10:10:56AM -0200, Fabio Estevam wrote:
On Mon, Jan 11, 2016 at 10:04 AM, Fabio Estevam festevam@gmail.com wrote:
[ 2.526984] ------------[ cut here ]------------ [ 2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xf4/0x124()
This fixes the warning:
--- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -180,7 +180,6 @@ static const struct regmap_config fsl_ssi_regconfig = { .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg, .writeable_reg = fsl_ssi_writeable_reg,
.cache_type = REGCACHE_RBTREE,};
Is this the correct fix?
I suspect not, it looks like the driver is using the cache for suspend/resume handling. I've dropped the patch for now. Either the driver should explicitly write to the relevant registers outside of interrupt context to ensure the cache entry exists or it should keep the defaults and explicitly write them to hardware at startup to ensure sync (the former is more likely to be safe).
On 11.01.2016 15:00, Mark Brown wrote:
On Mon, Jan 11, 2016 at 10:10:56AM -0200, Fabio Estevam wrote:
On Mon, Jan 11, 2016 at 10:04 AM, Fabio Estevam festevam@gmail.com wrote:
[ 2.526984] ------------[ cut here ]------------ [ 2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xf4/0x124()
This fixes the warning:
--- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -180,7 +180,6 @@ static const struct regmap_config fsl_ssi_regconfig = { .volatile_reg = fsl_ssi_volatile_reg, .precious_reg = fsl_ssi_precious_reg, .writeable_reg = fsl_ssi_writeable_reg,
.cache_type = REGCACHE_RBTREE,};
Is this the correct fix?
I suspect not, it looks like the driver is using the cache for suspend/resume handling. I've dropped the patch for now. Either the driver should explicitly write to the relevant registers outside of interrupt context to ensure the cache entry exists or it should keep the defaults and explicitly write them to hardware at startup to ensure sync (the former is more likely to be safe).
Is it acceptable to switch it to flat cache instead to not keep the register defaults in driver?
Maciej
On Mon, Jan 11, 2016 at 03:10:20PM +0100, Maciej S. Szmigiero wrote:
On 11.01.2016 15:00, Mark Brown wrote:
I suspect not, it looks like the driver is using the cache for suspend/resume handling. I've dropped the patch for now. Either the driver should explicitly write to the relevant registers outside of interrupt context to ensure the cache entry exists or it should keep the defaults and explicitly write them to hardware at startup to ensure sync (the former is more likely to be safe).
Is it acceptable to switch it to flat cache instead to not keep the register defaults in driver?
That's possibly problematic because the flat cache will of necessity end up with defaults (of 0 from the kzalloc()) for all the registers. You'll still have default values in the cache, though some of the behaviour around optimising syncs does change without them explicitly given. It does deal with the allocation issue but given that the issue was incorrect defaults I'd be a bit concerned.
Mark Brown wrote:
That's possibly problematic because the flat cache will of necessity end up with defaults (of 0 from the kzalloc()) for all the registers. You'll still have default values in the cache, though some of the behaviour around optimising syncs does change without them explicitly given. It does deal with the allocation issue but given that the issue was incorrect defaults I'd be a bit concerned.
Ok, I'm confused. Granted, all of this regcache stuff was added after I stopped working on this driver, so I'm out of the loop. But it appears that the regcache cannot properly handle an uninitialized cache. I would expect it to know to perform hard reads of any registers that are uninitialized.
If the regcache wants to have an initialized cache, then it should automatically perform reads an all non-volatile, non-precious registers at initialization.
On Mon, Jan 11, 2016 at 09:45:37AM -0600, Timur Tabi wrote:
Ok, I'm confused. Granted, all of this regcache stuff was added after I stopped working on this driver, so I'm out of the loop. But it appears that the regcache cannot properly handle an uninitialized cache. I would expect it to know to perform hard reads of any registers that are uninitialized.
regcache handles this fine, it's perfectly happy to just go and allocate the cache as registers get used (this is why the code that's doing the allocation exists...). What is causing problems here is that the first access to the register is happening in interrupt context so we can't do a GFP_KERNEL allocation for it. Most users don't do anything at all in interrupt context so it's not an issue for them, drivers that want to use regmap in interrupt context need to handle this.
We can't rely on knowing which registers are valid and which registers can be read without side effects, it's optional for drivers to provide that information. Even with that information it's not always clear that we want to stop and read every single value when we are initialising the device, that might be excessively slow (remember a lot of regmap devices are I2C or SPI connected, some with large register maps). We should have a helper to do that though for drivers where it does make sense.
Mark Brown wrote:
regcache handles this fine, it's perfectly happy to just go and allocate the cache as registers get used (this is why the code that's doing the allocation exists...). What is causing problems here is that the first access to the register is happening in interrupt context so we can't do a GFP_KERNEL allocation for it.
Considering how small and not-sparse the SSI register space is, would using REGCACHE_FLAT be appropriate?
On Mon, Jan 11, 2016 at 07:23:54PM -0600, Timur Tabi wrote:
Mark Brown wrote:
regcache handles this fine, it's perfectly happy to just go and allocate the cache as registers get used (this is why the code that's doing the allocation exists...). What is causing problems here is that the first access to the register is happening in interrupt context so we can't do a GFP_KERNEL allocation for it.
Considering how small and not-sparse the SSI register space is, would using REGCACHE_FLAT be appropriate?
Quite possibly (it'll be more efficient and it's intended for such use cases) but as I said in my other reply that then has the issue that it implicitly gives default values to all the registers so I'd expect we still need to handle the cache initialisation explicitly (or alternatively the hardware sync with the cache on startup).
Mark Brown wrote:
Quite possibly (it'll be more efficient and it's intended for such use cases) but as I said in my other reply that then has the issue that it implicitly gives default values to all the registers so I'd expect we still need to handle the cache initialisation explicitly (or alternatively the hardware sync with the cache on startup).
Why does REGCACHE_FLAT assume that all registers have a default value of 0? Shouldn't it have the same behavior w.r.t. cache values as REGCACHE_RBTREE?
participants (4)
-
Fabio Estevam -
Maciej S. Szmigiero -
Mark Brown -
Timur Tabi