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

Tue Dec 11 18:45:22 UTC 2018

On 11/12/2018 19:22, Stephen Warren wrote:
> 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.

It is fine as long as they support xfer with a 0 length message.

I'll post the patch with your tested-by.

Thanks

> 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.