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

[Melin Tomas]

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.


thanks,

Tomas