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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
