[U-Boot] Regression: dm: i2c: Make i2c_get_chip_for_busnum() fail if the chip is not detected

[Jean-Jacques Hiblot]

On 11/12/2018 05:41, Heiko Schocher wrote:
> Hello Stephen,
>
> Am 10.12.2018 um 19:23 schrieb Stephen Warren:
>> The following commit:
>>
>>> dm: i2c: Make i2c_get_chip_for_busnum() fail if the chip is not 
>>> detected
>>>     i2c_get_chip_for_busnum() really should check the presence of 
>>> the chip on
>>>     the bus. Most of the users of this function assume that this is 
>>> done.
>>
>> ... causes a boot failure on NVIDIA Jetson TX2:
>
> :-(
>
> Thanks for detecting so fast!
>
>>> U-Boot 2019.01-rc1-00220-g7ff485c68b7e (Dec 10 2018 - 11:20:41 -0700)
>>>
>>> TEGRA186
>>> Model: NVIDIA P2771-0000-500
>>> DRAM:  7.8 GiB
>>> tegra_ivc_read_get_next_frame() timed out (-12)
>>> tegra_board_init: Cannot find MAX77620 I2C chip
>>> initcall sequence 00000000fffa95a8 failed at call 0000000080083480 
>>> (err=-110)
>>> ### ERROR ### Please RESET the board ###
>>
>> This may be due to the fact the bus in question is implemented by RPC 
>> to a separate CPU, and that mechanism hasn't been used with probing 
>> before. In general though, there's not guarantee that probing will 
>> work even on a local/native I2C bus, since different chips don't 
>> support all probe methods (see i2c-detect in Linux, which supports 
>> various different probing methods due to this), so I'm rather 
>> surprised this change was implemented. Is it really necessary? I 
>> believe we should revert it.

The probe method is not the same in u-boot as in i2c-detect. In u-boot 
there is no data transfer, the probe only sends the address on the bus 
and fails if the device does not respond with a ACK (or if something 
else goes wrong). Every I2C device supports this kind of probe by design.

Errors could happen though:

- device not present, or not powered up or in reset state

- bus not ready (in your case, maybe the CPU doing the actual work is 
not ready)

- bus speed too high.

In all those cases this could be fixed in the board specific code.


While I agree that a commit should not break platforms, I'm not 
convinced that reverting the commit is the right solution: in 
tegra_board_init() the call to i2c_get_chip_for_busnum() is followed by 
a call to dm_i2c_write(). Assuming that we remove the offending commit, 
i2c_get_chip_for_busnum() would not fail anymore, but in this case the 
following call to dm_i2c_write() should fail. If it doesn't then I 
suspect that there is something wrong in the tegra I2C bus driver that 
makes it unable to transfer only the address word.

JJ

>
> Hmm... yes, you are right.
>
> Ah, Jean-Jacques first had another approach, see:
>
> https://lists.denx.de/pipermail/u-boot/2018-October/343230.html
>
> May it is possible to switch back to this approach ?
>
> bye,
> Heiko