[PATCH] ASoC: sgtl5000: add delay before first I2C access

[PATCH] ASoC: sgtl5000: add delay before first I2C access

[Eric Nelson]

To quote from section 1.3.1 of the data sheet:
	The SGTL5000 has an internal reset that is deasserted
	8 SYS_MCLK cycles after all power rails have been brought
	up. After this time, communication can start

	...
	1.0μs represents 8 SYS_MCLK cycles at the minimum 8.0 MHz SYS_MCLK.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 sound/soc/codecs/sgtl5000.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 29cf7ce..7d65a5b 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1462,6 +1462,9 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Need 8 clocks before I2C accesses */
+	usleep_range(1, 2);
+
 	/* read chip information */
 	ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, &reg);
 	if (ret)
-- 
1.9.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Eric Nelson]

To quote from section 1.3.1 of the data sheet:
	The SGTL5000 has an internal reset that is deasserted
	8 SYS_MCLK cycles after all power rails have been brought
	up. After this time, communication can start

	...
	1.0uS represents 8 SYS_MCLK cycles at the minimum 8.0 MHz SYS_MCLK.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
V2 replaces usleep_range with udelay and fixes microseconds in comment.

 sound/soc/codecs/sgtl5000.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 29cf7ce..7d65a5b 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1462,6 +1462,9 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Need 8 clocks before I2C accesses */
+	udelay(1);
+
 	/* read chip information */
 	ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, &reg);
 	if (ret)
-- 
1.9.1

Re: [PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Fabio Estevam]

Hi Eric,

On Fri, Jan 30, 2015 at 7:07 PM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:
> To quote from section 1.3.1 of the data sheet:
>         The SGTL5000 has an internal reset that is deasserted
>         8 SYS_MCLK cycles after all power rails have been brought
>         up. After this time, communication can start
>
>         ...
>         1.0uS represents 8 SYS_MCLK cycles at the minimum 8.0 MHz SYS_MCLK.

Small detail: Should be us instead of uS.

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>

Re: [PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Nikolay Dimitrov]

Hi Eric,

On 01/30/2015 11:31 PM, Fabio Estevam wrote:
> Hi Eric,
>
> On Fri, Jan 30, 2015 at 7:07 PM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
>> To quote from section 1.3.1 of the data sheet:
>>          The SGTL5000 has an internal reset that is deasserted
>>          8 SYS_MCLK cycles after all power rails have been brought
>>          up. After this time, communication can start
>>
>>          ...
>>          1.0uS represents 8 SYS_MCLK cycles at the minimum 8.0 MHz SYS_MCLK.
>
> Small detail: Should be us instead of uS.

FYI - If you're observing issues with communicating with SGTL5000 I2C,
please make sure also that the chip has a valid clock signal on
SYS_MCLK, otherwise it won't respond on I2C transactions (I2C will work
with any SYS_MCLK in the range 8-27MHz).

Kind regards,
Nikolay

Re: [PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Eric Nelson]

Hi Nikolay,

On 01/30/2015 02:50 PM, Nikolay Dimitrov wrote:
> Hi Eric,
> 
> On 01/30/2015 11:31 PM, Fabio Estevam wrote:
>> Hi Eric,
>>
>> On Fri, Jan 30, 2015 at 7:07 PM, Eric Nelson
>> <eric.nelson@boundarydevices.com> wrote:
>>> To quote from section 1.3.1 of the data sheet:
>>>          The SGTL5000 has an internal reset that is deasserted
>>>          8 SYS_MCLK cycles after all power rails have been brought
>>>          up. After this time, communication can start
>>>
>>>          ...
>>>          1.0uS represents 8 SYS_MCLK cycles at the minimum 8.0 MHz
>>> SYS_MCLK.
>>
>> Small detail: Should be us instead of uS.
> 
> FYI - If you're observing issues with communicating with SGTL5000 I2C,
> please make sure also that the chip has a valid clock signal on
> SYS_MCLK, otherwise it won't respond on I2C transactions (I2C will work
> with any SYS_MCLK in the range 8-27MHz).
> 

Thanks for that, but the issue we're seeing and that made me spot this
was a very intermittent problem (1 in many 1000's of boots) reported by
a customer on our 3.10.17 code base.

We haven't been able to repeat the issue, but the failure is in the
initial read of the CHIP_ID register (i.e. the first I2C access) and
the docs clearly state that a 1us delay is needed.

I haven't found the commit that removed it, but earlier versions
of sgtl5000.c had a udelay(10) before the first I2C access.

Regards,


Eric

Re: [PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Nikolay Dimitrov]

Hi Eric,

On 01/31/2015 12:02 AM, Eric Nelson wrote:
> Hi Nikolay,
>
> On 01/30/2015 02:50 PM, Nikolay Dimitrov wrote:
>> Hi Eric,
>>
>> On 01/30/2015 11:31 PM, Fabio Estevam wrote:
>>> Hi Eric,
>>>
>>> On Fri, Jan 30, 2015 at 7:07 PM, Eric Nelson
>>> <eric.nelson@boundarydevices.com> wrote:
>>>> To quote from section 1.3.1 of the data sheet:
>>>>           The SGTL5000 has an internal reset that is deasserted
>>>>           8 SYS_MCLK cycles after all power rails have been brought
>>>>           up. After this time, communication can start
>>>>
>>>>           ...
>>>>           1.0uS represents 8 SYS_MCLK cycles at the minimum 8.0 MHz
>>>> SYS_MCLK.
>>>
>>> Small detail: Should be us instead of uS.
>>
>> FYI - If you're observing issues with communicating with SGTL5000 I2C,
>> please make sure also that the chip has a valid clock signal on
>> SYS_MCLK, otherwise it won't respond on I2C transactions (I2C will work
>> with any SYS_MCLK in the range 8-27MHz).
>>
>
> Thanks for that, but the issue we're seeing and that made me spot this
> was a very intermittent problem (1 in many 1000's of boots) reported by
> a customer on our 3.10.17 code base.
>
> We haven't been able to repeat the issue, but the failure is in the
> initial read of the CHIP_ID register (i.e. the first I2C access) and
> the docs clearly state that a 1us delay is needed.
>
> I haven't found the commit that removed it, but earlier versions
> of sgtl5000.c had a udelay(10) before the first I2C access.

I see. Had similar issue with 3.10.17 (fsl), when sometimes on
hot-reset the (imx6) I2C controller was somehow locked-up on boot and
prevented the driver from reading CHIP_ID, and thus no sound at all.
But on 3.17x/3.18x mainline kernels + riotboard, this initial SGTL5000
configuration is much much better, haven't seen cases when it doesn't
detect the codec.

If you can reproduce the defect, you can try to exercise the I2C
channel and see whether it still works at all.

I'm mentioning this because it's a different root-cause that has quite
similar observable effect. Otherwise I truly agree that reset timings
as per datasheet are very good to be followed.

Kind regards,
Nikolay

Re: [PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Eric Nelson]

Thanks Nikolay,

On 01/30/2015 03:24 PM, Nikolay Dimitrov wrote:
> Hi Eric,
> 
> On 01/31/2015 12:02 AM, Eric Nelson wrote:
>> Hi Nikolay,
>>
>> On 01/30/2015 02:50 PM, Nikolay Dimitrov wrote:
>>> Hi Eric,
>>>
>>> On 01/30/2015 11:31 PM, Fabio Estevam wrote:
>>>> Hi Eric,
>>>>
>>>> On Fri, Jan 30, 2015 at 7:07 PM, Eric Nelson
>>>> <eric.nelson@boundarydevices.com> wrote:
>>>>> To quote from section 1.3.1 of the data sheet:
>>>>>           The SGTL5000 has an internal reset that is deasserted
>>>>>           8 SYS_MCLK cycles after all power rails have been brought
>>>>>           up. After this time, communication can start
>>>>>
>>>>>           ...
>>>>>           1.0uS represents 8 SYS_MCLK cycles at the minimum 8.0 MHz
>>>>> SYS_MCLK.
>>>>
>>>> Small detail: Should be us instead of uS.
>>>
>>> FYI - If you're observing issues with communicating with SGTL5000 I2C,
>>> please make sure also that the chip has a valid clock signal on
>>> SYS_MCLK, otherwise it won't respond on I2C transactions (I2C will work
>>> with any SYS_MCLK in the range 8-27MHz).
>>>
>>
>> Thanks for that, but the issue we're seeing and that made me spot this
>> was a very intermittent problem (1 in many 1000's of boots) reported by
>> a customer on our 3.10.17 code base.
>>
>> We haven't been able to repeat the issue, but the failure is in the
>> initial read of the CHIP_ID register (i.e. the first I2C access) and
>> the docs clearly state that a 1us delay is needed.
>>
>> I haven't found the commit that removed it, but earlier versions
>> of sgtl5000.c had a udelay(10) before the first I2C access.
> 
> I see. Had similar issue with 3.10.17 (fsl), when sometimes on
> hot-reset the (imx6) I2C controller was somehow locked-up on boot and
> prevented the driver from reading CHIP_ID, and thus no sound at all.
> But on 3.17x/3.18x mainline kernels + riotboard, this initial SGTL5000
> configuration is much much better, haven't seen cases when it doesn't
> detect the codec.
> 
> If you can reproduce the defect, you can try to exercise the I2C
> channel and see whether it still works at all.
> 

Have you been reading my e-mails with the customer ;) ?

We're doing a bunch of testing, trying to force I2C accesses to
it and the other I2C device on that channel of Nitrogen6X
(the ISL1208 RTC).

> I'm mentioning this because it's a different root-cause that has quite
> similar observable effect. Otherwise I truly agree that reset timings
> as per datasheet are very good to be followed.
> 

Yep. and so the patch...

Regards,


Eric

Re: [PATCH V2] ASoC: sgtl5000: add delay before first I2C access

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 458 bytes --]

On Fri, Jan 30, 2015 at 02:07:55PM -0700, Eric Nelson wrote:
> To quote from section 1.3.1 of the data sheet:
> 	The SGTL5000 has an internal reset that is deasserted
> 	8 SYS_MCLK cycles after all power rails have been brought
> 	up. After this time, communication can start

Applied, though given the normal speeds of I2C I'm always a bit
suspicous that something like this that's messing around with
microsecond delays is actually masking something else.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]