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

[Mark Brown]

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.


signature.asc
Description: PGP signature

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

[Fabio Estevam]

On Mon, Feb 1, 2016 at 7:30 PM, Maciej S. Szmigiero
 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

[Maciej S. Szmigiero]

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

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

[Mark Brown]

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.


signature.asc
Description: PGP signature

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

[Maciej S. Szmigiero]

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
>>  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

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

[Maciej S. Szmigiero]

On 01.02.2016 13:13, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
>  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

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

[Fabio Estevam]

Hi Maciej,

On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
 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

[Maciej S. Szmigiero]

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
>  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 
> 
> 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

[Fabio Estevam]

Hi Maciej,

On Mon, Jan 18, 2016 at 5:07 PM, 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.
>
> 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 

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

[Mark Brown]

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.


signature.asc
Description: PGP signature

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

[Maciej S. Szmigiero]

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

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

[Fabio Estevam]

On Mon, Feb 1, 2016 at 7:30 PM, Maciej S. Szmigiero
 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

[Mark Brown]

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.


signature.asc
Description: PGP signature

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

[Maciej S. Szmigiero]

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
>  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 
> 
> 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

[Fabio Estevam]

Hi Maciej,

On Mon, Jan 18, 2016 at 5:07 PM, 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.
>
> 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 

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

[Fabio Estevam]

Hi Maciej,

On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
 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

[Maciej S. Szmigiero]

On 01.02.2016 13:13, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
>  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

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

[Maciej S. Szmigiero]

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
>>  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