[U-Boot] [PATCH v2 1/2] xilinx_xiic: Fix fill tx fifo loop

[U-Boot] [PATCH v2 1/2] xilinx_xiic: Fix fill tx fifo loop

[Melin Tomas]

Comparison should be against the actual message length, not loop index.

len is used for stopping while loop, pos is position in message.
stop should be sent when entire message is sent, not when
len and pos meet.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---

Changes in v2:
- Added reasoning to commit message

 drivers/i2c/xilinx_xiic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/xilinx_xiic.c b/drivers/i2c/xilinx_xiic.c
index 83114ed510..e4ca0ab936 100644
--- a/drivers/i2c/xilinx_xiic.c
+++ b/drivers/i2c/xilinx_xiic.c
@@ -149,7 +149,7 @@ static void xiic_fill_tx_fifo(struct xilinx_xiic_priv *priv,
 	while (len--) {
 		u16 data = msg->buf[pos++];
 
-		if (pos == len && nmsgs == 1) {
+		if ((msg->len - pos == 0) && nmsgs == 1) {
 			/* last message in transfer -> STOP */
 			data |= XIIC_TX_DYN_STOP_MASK;
 		}
-- 
2.17.2

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

[Melin Tomas]

Prior to starting a new transfer, conditionally wait for bus to not
be busy.

Reinitialise controller as otherwise operation is not stable.
For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
"i2c: xiic: Do not reset controller before every transfer"")

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
Changes in v2:
- Change variable declaration order
- Change timeout to 3ms

 drivers/i2c/xilinx_xiic.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/xilinx_xiic.c b/drivers/i2c/xilinx_xiic.c
index e4ca0ab936..83386677d5 100644
--- a/drivers/i2c/xilinx_xiic.c
+++ b/drivers/i2c/xilinx_xiic.c
@@ -266,8 +266,17 @@ static void xiic_reinit(struct xilinx_xiic_priv *priv)
 
 static int xilinx_xiic_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 {
+	struct xilinx_xiic_priv *priv = dev_get_priv(dev);
 	int ret = 0;
 
+	ret = wait_for_bit_8(priv->base + XIIC_SR_REG_OFFSET,
+			     XIIC_SR_BUS_BUSY_MASK, false, 3, true);
+
+	if (ret)
+		return ret;
+
+	xiic_reinit(priv);
+
 	for (; nmsgs > 0; nmsgs--, msg++) {
 		if (msg->flags & I2C_M_RD)
 			ret = xilinx_xiic_read_common(dev, msg, nmsgs);
-- 
2.17.2

[U-Boot] [PATCH v2 1/2] xilinx_xiic: Fix fill tx fifo loop

[Marek Vasut]

On 6/25/19 3:29 PM, Melin Tomas wrote:
> Comparison should be against the actual message length, not loop index.
> 
> len is used for stopping while loop, pos is position in message.
> stop should be sent when entire message is sent, not when
> len and pos meet.

Thanks. Just be careful to clamp the transfer length to 65535 bytes,
otherwise you'll hit another overflow.

> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
> 
> Changes in v2:
> - Added reasoning to commit message
> 
>  drivers/i2c/xilinx_xiic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/xilinx_xiic.c b/drivers/i2c/xilinx_xiic.c
> index 83114ed510..e4ca0ab936 100644
> --- a/drivers/i2c/xilinx_xiic.c
> +++ b/drivers/i2c/xilinx_xiic.c
> @@ -149,7 +149,7 @@ static void xiic_fill_tx_fifo(struct xilinx_xiic_priv *priv,
>  	while (len--) {
>  		u16 data = msg->buf[pos++];
>  
> -		if (pos == len && nmsgs == 1) {
> +		if ((msg->len - pos == 0) && nmsgs == 1) {
>  			/* last message in transfer -> STOP */
>  			data |= XIIC_TX_DYN_STOP_MASK;
>  		}
> 


-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

On 6/25/19 3:30 PM, Melin Tomas wrote:
> Prior to starting a new transfer, conditionally wait for bus to not
> be busy.
> 
> Reinitialise controller as otherwise operation is not stable.
> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
> "i2c: xiic: Do not reset controller before every transfer"")
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
> Changes in v2:
> - Change variable declaration order
> - Change timeout to 3ms

Why 3mS ?

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

On 6/25/19 6:15 PM, Marek Vasut wrote:

> On 6/25/19 3:30 PM, Melin Tomas wrote:
>> Prior to starting a new transfer, conditionally wait for bus to not
>> be busy.
>>
>> Reinitialise controller as otherwise operation is not stable.
>> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
>> "i2c: xiic: Do not reset controller before every transfer"")
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>> Changes in v2:
>> - Change variable declaration order
>> - Change timeout to 3ms
> Why 3mS ?

That is value used also in kernel driver.


thanks,

Tomas

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

[Marek Vasut]

On 6/26/19 7:30 AM, Melin Tomas wrote:
> On 6/25/19 6:15 PM, Marek Vasut wrote:
> 
>> On 6/25/19 3:30 PM, Melin Tomas wrote:
>>> Prior to starting a new transfer, conditionally wait for bus to not
>>> be busy.
>>>
>>> Reinitialise controller as otherwise operation is not stable.
>>> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
>>> "i2c: xiic: Do not reset controller before every transfer"")
>>>
>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>> ---
>>> Changes in v2:
>>> - Change variable declaration order
>>> - Change timeout to 3ms
>> Why 3mS ?
> 
> That is value used also in kernel driver.

But why 3mS , why not e.g. 5mS ?

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

On 6/26/19 12:46 PM, Marek Vasut wrote:

> On 6/26/19 7:30 AM, Melin Tomas wrote:
>> On 6/25/19 6:15 PM, Marek Vasut wrote:
>>
>>> On 6/25/19 3:30 PM, Melin Tomas wrote:
>>>> Prior to starting a new transfer, conditionally wait for bus to not
>>>> be busy.
>>>>
>>>> Reinitialise controller as otherwise operation is not stable.
>>>> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
>>>> "i2c: xiic: Do not reset controller before every transfer"")
>>>>
>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>> ---
>>>> Changes in v2:
>>>> - Change variable declaration order
>>>> - Change timeout to 3ms
>>> Why 3mS ?
>> That is value used also in kernel driver.
> But why 3mS , why not e.g. 5mS ?

Quoting from comment: "for instance if previous transfer was terminated 
due to TX error it might be that the bus is on it's way to become 
available give it at most 3 ms to wake"


thanks,

Tomas

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

[Marek Vasut]

On 6/26/19 12:12 PM, Melin Tomas wrote:
> On 6/26/19 12:46 PM, Marek Vasut wrote:
> 
>> On 6/26/19 7:30 AM, Melin Tomas wrote:
>>> On 6/25/19 6:15 PM, Marek Vasut wrote:
>>>
>>>> On 6/25/19 3:30 PM, Melin Tomas wrote:
>>>>> Prior to starting a new transfer, conditionally wait for bus to not
>>>>> be busy.
>>>>>
>>>>> Reinitialise controller as otherwise operation is not stable.
>>>>> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
>>>>> "i2c: xiic: Do not reset controller before every transfer"")
>>>>>
>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Change variable declaration order
>>>>> - Change timeout to 3ms
>>>> Why 3mS ?
>>> That is value used also in kernel driver.
>> But why 3mS , why not e.g. 5mS ?
> 
> Quoting from comment: "for instance if previous transfer was terminated 
> due to TX error it might be that the bus is on it's way to become 
> available give it at most 3 ms to wake"

So where did that 3 mS figure come from ? Is it from a datasheet ? Or
the HDL ? Or is that some arbitrary number ?

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

On 6/26/19 1:26 PM, Marek Vasut wrote:
> On 6/26/19 12:12 PM, Melin Tomas wrote:
>> On 6/26/19 12:46 PM, Marek Vasut wrote:
>>
>>> On 6/26/19 7:30 AM, Melin Tomas wrote:
>>>> On 6/25/19 6:15 PM, Marek Vasut wrote:
>>>>
>>>>> On 6/25/19 3:30 PM, Melin Tomas wrote:
>>>>>> Prior to starting a new transfer, conditionally wait for bus to not
>>>>>> be busy.
>>>>>>
>>>>>> Reinitialise controller as otherwise operation is not stable.
>>>>>> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
>>>>>> "i2c: xiic: Do not reset controller before every transfer"")
>>>>>>
>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - Change variable declaration order
>>>>>> - Change timeout to 3ms
>>>>> Why 3mS ?
>>>> That is value used also in kernel driver.
>>> But why 3mS , why not e.g. 5mS ?
>> Quoting from comment: "for instance if previous transfer was terminated
>> due to TX error it might be that the bus is on it's way to become
>> available give it at most 3 ms to wake"
> So where did that 3 mS figure come from ? Is it from a datasheet ? Or
> the HDL ? Or is that some arbitrary number ?

This driver is based on the kernel driver, and has the same structure as 
that.

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.

thanks,

Tomas

>

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

[Marek Vasut]

On 6/26/19 12:39 PM, Melin Tomas wrote:
> 
> On 6/26/19 1:26 PM, Marek Vasut wrote:
>> On 6/26/19 12:12 PM, Melin Tomas wrote:
>>> On 6/26/19 12:46 PM, Marek Vasut wrote:
>>>
>>>> On 6/26/19 7:30 AM, Melin Tomas wrote:
>>>>> On 6/25/19 6:15 PM, Marek Vasut wrote:
>>>>>
>>>>>> On 6/25/19 3:30 PM, Melin Tomas wrote:
>>>>>>> Prior to starting a new transfer, conditionally wait for bus to not
>>>>>>> be busy.
>>>>>>>
>>>>>>> Reinitialise controller as otherwise operation is not stable.
>>>>>>> For reference, see linux kernel commit: 9656eeebf3f1 ("i2c: Revert
>>>>>>> "i2c: xiic: Do not reset controller before every transfer"")
>>>>>>>
>>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> - Change variable declaration order
>>>>>>> - Change timeout to 3ms
>>>>>> Why 3mS ?
>>>>> That is value used also in kernel driver.
>>>> But why 3mS , why not e.g. 5mS ?
>>> Quoting from comment: "for instance if previous transfer was terminated
>>> due to TX error it might be that the bus is on it's way to become
>>> available give it at most 3 ms to wake"
>> So where did that 3 mS figure come from ? Is it from a datasheet ? Or
>> the HDL ? Or is that some arbitrary number ?
> 
> This driver is based on the kernel driver, and has the same structure as 
> that.

So what, Linux kernel has bugs too.

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

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

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.

So probably it's just a number, after all it's just a check to know if 
we are good to go.

thanks,

Tomas

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

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

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.


thanks,

Tomas

>

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

[Marek Vasut]

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 .

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

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.


thanks,

Tomas


>

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

[Marek Vasut]

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.

You're the patch author, it's your responsibility to know why you're
adding/changing the code you're adding/changing.

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

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?

> 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


thanks,

Tomas

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

[Marek Vasut]

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 ?

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

-- 
Best regards,
Marek Vasut

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

[Melin Tomas]

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

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


thanks,

Tomas

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

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

[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

[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

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

[Heiko Schocher]

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@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@pelagicore.com>
     Signed-off-by: Ben Dooks <ben-linux@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.

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?

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] [PATCH v2 2/2] xilinx_xiic: Fix transfer initialisation

[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@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@pelagicore.com>
>     Signed-off-by: Ben Dooks <ben-linux@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.


thanks,

Tomas

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

[Heiko Schocher]

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@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@pelagicore.com>
>>      Signed-off-by: Ben Dooks <ben-linux@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