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

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

[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:

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

Thanks.

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

[Heiko Schocher]

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.

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[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

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

[Stephen Warren]

On 12/10/18 9:41 PM, 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.
> 
> 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 ?

That looks like the same approach?

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

[Stephen Warren]

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

[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,
  };

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

[Stephen Warren]

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.

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

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

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

[Simon Glass]

Hi Stephen,

On Mon, 10 Dec 2018 at 11:23, Stephen Warren <swarren@wwwdotorg.org> wrote:
>
> 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:
>
> > 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.

i2c_get_chip_for_busnum() is a legacy function. New code should use
the device tree to handle this case.

Can this code be moved into a PMIC / regulator driver?

Regards,
Simon