| Message ID | 569D3800.2040801@maciej.szmigiero.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 18, 2016 at 5:07 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. > > 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>
Hi Maciej, On Mon, Jan 18, 2016 at 5:07 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. > > 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> 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
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 > <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. >> >> 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> > > 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
Hi Maciej, On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero <mail@maciej.szmigiero.name> 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.
On 01.02.2016 13:13, Fabio Estevam wrote: > Hi Maciej, > > On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero > <mail@maciej.szmigiero.name> 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. Hmm, I will try to build this tree on UDOO board today and see what happens. Maciej
On 01.02.2016 13:25, Maciej S. Szmigiero wrote: > On 01.02.2016 13:13, Fabio Estevam wrote: >> Hi Maciej, >> >> On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero >> <mail@maciej.szmigiero.name> 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. > > 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
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.
On 01.02.2016 22:10, Mark Brown wrote: > 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. 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
On Mon, Feb 1, 2016 at 7:30 PM, Maciej S. Szmigiero <mail@maciej.szmigiero.name> 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?
On Mon, Feb 01, 2016 at 10:30:53PM +0100, Maciej S. Szmigiero wrote: > On 01.02.2016 22:10, Mark Brown wrote: > > 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.
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");
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> --- 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.