[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 18:10, Stephen Warren wrote:
> On 12/11/18 2:44 AM, Jean-Jacques Hiblot wrote:
>>
>> 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.
>
> Yes, I imagine that our other CPU doesn't support zero-length 
> transfers. However, that's not going to change. Our only choice is not 
> to do this unnecessary probing.
>
> Even if we were going to modify the Tegra I2C bus driver to solve or 
> work around this, we would still need to:
>
> a) Revert the change.
> b) Develop the fix.
> c) Re-apply the original change.
>
> ... to reduce the time window where the code is broken. Right now 
> everyone working on Tegra U-Boot is rather swamped so spending time on 
> fixing this regression is rather annoying...

How about implementing the  probe_chip() callback.

diff --git a/drivers/i2c/tegra186_bpmp_i2c.c 
b/drivers/i2c/tegra186_bpmp_i2c.c
index b4fff43..6256d27 100644
--- a/drivers/i2c/tegra186_bpmp_i2c.c
+++ b/drivers/i2c/tegra186_bpmp_i2c.c
@@ -85,6 +85,11 @@ static int tegra186_bpmp_i2c_xfer(struct udevice 
*dev, struct i2c_msg *msg,
         return 0;
  }

+static tegra186_bpmp_probe_chip(struct udevice *bus, uint chip_addr, 
uint chip_flags)
+{
+       return 0;
+}
+
  static int tegra186_bpmp_i2c_probe(struct udevice *dev)
  {
         struct tegra186_bpmp_i2c *priv = dev_get_priv(dev);
@@ -101,6 +106,7 @@ static int tegra186_bpmp_i2c_probe(struct udevice *dev)

  static const struct dm_i2c_ops tegra186_bpmp_i2c_ops = {
         .xfer = tegra186_bpmp_i2c_xfer,
+       .probe_chip = tegra186_bpmp_probe_chip,
  };