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

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

-- 
Best regards,
Marek Vasut