[U-Boot] Regression: dm: i2c: Make i2c_get_chip_for_busnum() fail if the chip is not detected
Stephen Warren
swarren at wwwdotorg.org
Tue Dec 11 18:12:39 UTC 2018
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.
More information about the U-Boot
mailing list