[U-Boot] [PATCH v2 2/2] xilinx_xiic: Fix transfer initialisation

Fri Jun 28 09:55:40 UTC 2019

Hello Tomas,

Am 28.06.2019 um 10:58 schrieb Melin Tomas:
> Hi,
> 
> + Richard, Ben
> 
> 
> On 6/27/19 3:05 PM, Heiko Schocher wrote:
>> Hi Marek, Tomas,
>>
>> Am 27.06.2019 um 13:32 schrieb Marek Vasut:
>>> On 6/27/19 7:53 AM, Melin Tomas wrote:
>>>>
>>>> On 6/27/19 4:41 AM, Marek Vasut wrote:
>>>>> On 6/26/19 8:17 PM, Melin Tomas wrote:
>>>>>> On 6/26/19 4:36 PM, Marek Vasut wrote:
>>>>>>> On 6/26/19 3:19 PM, Melin Tomas wrote:
>>>>>>>> On 6/26/19 3:48 PM, Marek Vasut wrote:
>>>>>>>>> On 6/26/19 2:45 PM, Melin Tomas wrote:
>>>>>>>>>> On 6/26/19 3:26 PM, Marek Vasut wrote:
>>>>>>>>>>> On 6/26/19 2:19 PM, Melin Tomas wrote:
>>>>>>>>>>>> On 6/26/19 2:49 PM, Marek Vasut wrote:
>>>>>>>>>>>>> On 6/26/19 1:25 PM, Melin Tomas wrote:
>>>>>>>>>>>>>> On 6/26/19 1:47 PM, Marek Vasut wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 6/26/19 12:39 PM, Melin Tomas wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As such, it's probably a good idea to keep the same
>>>>>>>>>>>>>>>> delay values here as
>>>>>>>>>>>>>>>> in the original driver unless good reason to use
>>>>>>>>>>>>>>>> something else.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As what goes for the original reasoning for 3ms, the
>>>>>>>>>>>>>>>> commit history does
>>>>>>>>>>>>>>>> not mention that so I cannot comment.
>>>>>>>>>>>>>>> So would you be so kind and research this ?
>>>>>>>>>>>>>> Based on a short study of other i2c bus drivers it seems
>>>>>>>>>>>>>> most have bus
>>>>>>>>>>>>>> busy timeout checks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The timeout values seems to differ, ranging from
>>>>>>>>>>>>>> milliseconds to seconds.
>>>>>>>>>>>>> Yep
>>>>>>>>>>>>>
>>>>>>>>>>>>>> So probably it's just a number, after all it's just a
>>>>>>>>>>>>>> check to know if
>>>>>>>>>>>>>> we are good to go.
>>>>>>>>>>>>> And is the number large enough ?
>>>>>>>>>>>> As mentioned, good approach is probably using value known to
>>>>>>>>>>>> work
>>>>>>>>>>>> instead of
>>>>>>>>>>>>
>>>>>>>>>>>> guessing a new number.
>>>>>>>>>>> So why did kernel pick that specific number ? Surely there
>>>>>>>>>>> was some
>>>>>>>>>>> reasoning, they didn't just pull it out of /dev/random .
>>>>>>>>>> Yes, history does not tell.
>>>>>>>>>>
>>>>>>>>>> I do see that this driver uses timeout of 1000ms for bus busy
>>>>>>>>>> when
>>>>>>>>>> probing, perhaps you can enlighten how that number was
>>>>>>>>>> concluded? If
>>>>>>>>>> that could give some clues about this.
>>>>>>>>> I don't know.
>>>>>>>> But you are author of that line?
>>>>>>> +    ret = wait_for_bit_8(priv->base + XIIC_SR_REG_OFFSET,
>>>>>>> +                 XIIC_SR_BUS_BUSY_MASK, false, 3, true);
>>>>>>> +
>>>>>>>
>>>>>>> comes from 2/2 ?
>>>>>> No, I was referring to the already existing wait_for_bit_8 of bus
>>>>>> busy
>>>>>>
>>>>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/i2c/xilinx_xiic.c#L294
>>>>>>
>>>>> Oh, this. When probing, you need to pick some arbitrary amount of time
>>>>> after which you give up and conclude there isn't any device at that
>>>>> address. 1 second should give enough time to even slow devices on slow
>>>>> busses. If some device is too slow or doesn't respond, too bad. That's
>>>>> also why i2c is NOT a bus which could be probed, there are simple
>>>>> devices which do not respond to addressing in any way.
>>>>>
>>>>> So this probably didn't help you determine why you should wait some
>>>>> time
>>>>> for bus busy when sending messages, since this is unrelated timeout.
>>>>>
>>>>>>>>> You're the patch author, it's your responsibility to know why
>>>>>>>>> you're
>>>>>>>>> adding/changing the code you're adding/changing.
>>>>>>>> yes, and the reasoning is:
>>>>>>>>
>>>>>>>> * the value has been deemed good in original driver. If it would
>>>>>>>> be bad,
>>>>>>>> probably it would have been changed during course of time
>>>>>>>>
>>>>>>>> * the value has been tested for this driver as well with success
>>>>>>> So shouldn't there be some upper bound on the bus busy time ,
>>>>>>> demanded
>>>>>>> either by the i2c bus spec or the xiic core spec ?
>>>>>> In case some of the devices on the bus misbehaves, bus could
>>>>>> potentially
>>>>>> stay
>>>>>>
>>>>>> busy until device reset or power-cycle.
>>>>> My concern here would be e.g. EEPROM programming, which is slow, so I
>>>>> wonder whether this timeout is enough. But maybe the xiic datasheet
>>>>> says
>>>>> something about maximum delay , or somehow tells you how to derive the
>>>>> delay from bus clock speed ...
>>>>
>>>> Specification only mentions to check bus busy status prior to starting,
>>>>
>>>> no mention about delays AFAIS.
>>>>
>>>> Even with standard low speed 100kHz, 3ms would equal 300 clock
>>>>
>>>> cycles waiting.
>>>>
>>>> Unless you have other suggestion, I suggest proceeding with this delay
>>>> value,
>>>>
>>>> as explained why in previous messages.
>>>
>>> There was no explanation besides "Linux does it this way, so it must be
>>> right", and I don't really like that. But ultimately, this is up to
>>> Heiko now.
>>
>> May it makes sense to ask Richard or Ben which have the driver
>> and the comment with "3 mS" introduced in linux?
>>
>> See linux commit:
>> commit e1d5b6598cdc33257fe68302ae9db81d2f7bb883
>> Author: Richard Röjfors <richard.rojfors at pelagicore.com>
>> Date:   Thu Feb 11 10:42:00 2010 +0100
>>
>>      i2c: Add support for Xilinx XPS IIC Bus Interface
>>
>>      This patch adds support for the Xilinx XPS IIC Bus Interface.
>>
>>      The driver uses the dynamic mode, supporting to put several
>>      I2C messages in the FIFO to reduce the number of interrupts.
>>
>>      It has the same feature as ocores, it can be passed a list
>>      of devices that will be added when the bus is probed.
>>
>>      Signed-off-by: Richard Röjfors <richard.rojfors at pelagicore.com>
>>      Signed-off-by: Ben Dooks <ben-linux at fluff.org>
>>
>>
>> Beside of that, wait_for_bit_8() drops a debug message, if
>> timeout, so that is good ...
>>
>> Remains the question, what is a good timeout value ?
>>
>> I tend to say, we could set the timeout higher, as the SR
>> register is polled all millisecond, and if bus is not busy
>> we loose no time.
> ok, so increase timeout to 1000ms or so?
>>
>> But the problem remains, what a good timeout is here ...
>>
>> So may we should add here a printf, if wait_for_bit_8 returns with
>> -ETIMEDOUT, so users may get in panic and report, and we can find a
>> better timeout?
> 
> Sounds ok to me.
> 
> So I'll update the patch to
> 
> * have longer 1000ms timeout
> 
> * print message in case of timeout from bus busy to catch possibly
> incorrect timeout estimate
> 
> Please let me know if this approach does not sound good to any of you.

For me it sounds good, many thanks!

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