| Message ID | 1422652075-13604-1-git-send-email-eric.nelson@boundarydevices.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 58cc9c9a175885bbf6bae3acf18233d0a8229a84 |
| Headers | show |
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>
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
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
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
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
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.
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, ®); if (ret)
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(+)