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

[Stephen Warren]

On 12/11/18 11:12 AM, Stephen Warren wrote:
> On 12/11/18 10:41 AM, Jean-Jacques Hiblot wrote:
>>
>> 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,
>>   };
> 
> That's fine by me. I guess it'll cause some shell commands to give odd 
> results, but if it makes the system boot that's OK.

Tested-by: Stephen Warren <swarren at nvidia.com>

You need to add "int" return type to:
+static tegra186_bpmp_probe_chip(struct ...

I note that there are many many other I2C bus drivers that don't 
implement .probe_chip, for example:

./drivers/rtc/i2c_rtc_emul.c
./drivers/i2c/i2c-uniphier-f.c
./drivers/i2c/cros_ec_tunnel.c
./drivers/i2c/ast_i2c.c
./drivers/i2c/sandbox_i2c.c
./drivers/i2c/meson_i2c.c
./drivers/i2c/mv_i2c.c
./drivers/i2c/at91_i2c.c
... I stopped looking at the grep results, so there are more I didn't 
bother listing here.

I think you should fix the DM I2C core to print an error/warning message 
if a driver is registered without .probe_chip being implemented. 
Otherwise, many other boards will have the same issue that Jetson does.