schemas: i2c: Add the clock stretching property #102
Conversation
The I2C specification allows for the clock line to be held low
for a specified timeout to force the slave device into a 'wait'
mode. This feature is known as 'Clock stretching' and is
optional.
In the NXP I2C specification, clock stretching is described as
the process of pausing a transaction by holding the SCL line LOW.
The transaction can only continue when the line is released HIGH
again.[*] However, most target devices do not include an SCL
driver and are therefore unable to stretch the clock.
Add the following properties:
- i2c-scl-clk-low-timeout-ms: This property specifies the
duration, in milliseconds, for which the clock is kept low and
a client needs to detect a forced waiting state.
- i2c-scl-has-clk-low-timeout: This property specifies whether
the I2C controller implements the clock stretching property.
It's important to note that this feature should not be confused
with the SMBUS clock timeout, which serves a similar function but
specifies a timeout of 25-35ms. The I2C specification does not
recommend any specific timeout.
[*] NXP, UM10204 - I2C-bus specification and user manual
Rev. 7.0, 1 October 2021, chapter 3.1.9.
Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Changelog:v1 -> v2 |
|
You made the property as in milliseconds, not microseconds how existing Linux kernel drivers uses it. What are the ranges? Timeouts lower than 1 ms are not expected? |
| Indicates whether the controller implements the feature of wait | ||
| induction through SCL low, with the timeout being implemented | ||
| internally by the controller. | ||
|
|
There was a problem hiding this comment.
Why can't this property be implied by the controller's compatible string? Is it board specific?
There was a problem hiding this comment.
Hi Rob,
no, this is not board specific, this is i2c standard. In the commit log you will find the exact reference in the standard document. It's a small property of the standard used by only one controller so far. But, with Ryan Chen's patch, now we have already two controllers using it (in a different way, though).
In my opinion it should go here. But please, let me know if you think it should go somewhere else.
Meantime I will fix property/properties.
Thanks,
Andi
There was a problem hiding this comment.
According to explanation from Ryan (lengthy thread), this is board specific. The timeouts are relevant if board has multi-master setup. However what I was disagreeing in that thread, is that multi-master property should imply this one - at least "i2c-scl-has-clk-low-timeout" (in Ryan's patch it was called "aspeed,timeout:"). I could not understand why multi-master and i2c-scl-has-clk-low-timeout would be separate.
There was a problem hiding this comment.
Hi Krzysztof,
yes, this is board specific for all those boards who are implementing the clock stretching i2c from the standard.
Per se clock stretching is not related to the multi-master property and it's marked as optional even in multi-master controllers. The dependency between clock-stretching and multi-master should be specific to the hardware that Chen is implementing.
Andi
There was a problem hiding this comment.
For example clock stretching can also happen when the master wants to reset the communication because something wrong happened. But it's an optional property that apparently almost no one is using or implementing in upstream drivers.
There was a problem hiding this comment.
Per se clock stretching is not related to the multi-master property and it's marked as optional even in multi-master controllers. The dependency between clock-stretching and multi-master should be specific to the hardware that Chen is implementing.
I never got answer also from Ryan Chen why someone would not enable timeout for a multi-master setup. I understand it is optional, but why one would not enable it?
Maybe it's not optional in his hardware. For what I understood his hardware has implemented the multimaster depending on the clock stretching. Which is weird, but he knows best as he has the datasheets.
For example clock stretching can also happen when the master wants to reset the communication because something wrong happened.
How lack of this timeout property stops the device from using clock stretching? Your commit msg said:
"timeout" and "clock stretching" are synonyms. Apparently people prefer to call it timeout, so does the SMBUS specification. The i2c specification calls it clock stretching. I use them both. Is it confusing?
This property specifies whether the I2C controller implements the clock stretching property.
... so compatible says that. The properties do not say what is implemented by controller, because otherwise you would start documenting entire I2C spec in the bindings. The compatible says it all.
It does, maybe I haven't been clear enough. We have these two properties:
- i2c-scl-clk-low-timeout-us: where the clock stretching (or timeout) is implemented in the driver (see i2c-mpc), or think of i2c-gpio where everything is done in software.
- i2c-scl-has-clk-low-timeout: it's implemented by the controller (Ryan's case) where you ask the controller to start the clock stretching until it ends. (After that there is the master switch, in Ryan's case)
Is it clear?
But it's an optional property that apparently almost no one is using or implementing in upstream drivers.
Drivers do not matter for bindings in terms of argument whether property is valid or not.
I'm not understanding. The first property (-timeout-us) specifies how long should the wait be. Please consider that also i2c slaves might be dependent on this property. Depending on the i2c slave you might need to fine tune the microseconds (milliseconds, indeed :) ) you want to wait. Some slaves might need longer time to have a wait triggered some need a shorter time. So that it can be that the board does not have it, but because of some slaves we might need to implement it in software. (Is it the case of i2c-mpc?)
In the case of Ryan's hardware apparently the board does have it in hardware, but as an optional feature. Apparently needed for the multi-master (which is an optional configuration itself).
If, for example, you have slaves that do not understand what clock stretching is (like most of the sensors) then you don't need it.
There was a problem hiding this comment.
I never got answer also from Ryan Chen why someone would not enable timeout for a multi-master setup. I understand it is optional, but why one would not enable it?
Maybe it's not optional in his hardware.
If it was not optional in his hardware, you do not need that property. Why would you add to the DT bindings a boolean property which is always required? Just imply it from compatible and we are done.
For what I understood his hardware has implemented the multimaster depending on the clock stretching. Which is weird, but he knows best as he has the datasheets.
If his multi-master setup depends on it, then he does not need that property.
How lack of this timeout property stops the device from using clock stretching? Your commit msg said:
"timeout" and "clock stretching" are synonyms. Apparently people prefer to call it timeout, so does the SMBUS specification. The i2c specification calls it clock stretching. I use them both. Is it confusing?
No, but I asked how lack of this property stops the device (controller) from using it. We have lengthy discussion why do you need that property and still no answer.
This property specifies whether the I2C controller implements the clock stretching property.
... so compatible says that. The properties do not say what is implemented by controller, because otherwise you would start documenting entire I2C spec in the bindings. The compatible says it all.
It does, maybe I haven't been clear enough.
If compatible says, there is no need for this property.
We have these two properties:
- i2c-scl-clk-low-timeout-us: where the clock stretching (or timeout) is implemented in the driver (see i2c-mpc), or think of i2c-gpio where everything is done in software.
- i2c-scl-has-clk-low-timeout: it's implemented by the controller (Ryan's case) where you ask the controller to start the clock stretching until it ends. (After that there is the master switch, in Ryan's case)
Is it clear?
This yes, but it's not what exactly I was asking about.
But it's an optional property that apparently almost no one is using or implementing in upstream drivers.
Drivers do not matter for bindings in terms of argument whether property is valid or not.
I'm not understanding.
Which part? That driver does not matter? DTS describes hardware, not driver. Devicetree bindings describe DTS thus it still describe the hardware, not the driver. Using driver as an argument therefore usually does not apply, because driver does not matter.
The first property (-timeout-us) specifies how long should the wait be. Please consider that also i2c slaves might be dependent on this property.
That's the key point and it was nowhere mentioned up to here.
Depending on the i2c slave you might need to fine tune the microseconds (milliseconds, indeed :) ) you want to wait. Some slaves might need longer time to have a wait triggered some need a shorter time. So that it can be that the board does not have it, but because of some slaves we might need to implement it in software. (Is it the case of i2c-mpc?)
So this is the rationale behind the timeout property with value. Now the question why do we ever need the bool timeout property.
In the case of Ryan's hardware apparently the board does have it in hardware, but as an optional feature. Apparently needed for the multi-master (which is an optional configuration itself).
If, for example, you have slaves that do not understand what clock stretching is (like most of the sensors) then you don't need it.
So is the case here for "i2c-scl-has-clk-low-timeout" property?
There was a problem hiding this comment.
[Removing the part above, as I think it all comes to this:]
So is the case here for "i2c-scl-has-clk-low-timeout" property?
OK... I think your logic is:
multi-master is optional -> multimaster depends on timeout -> why have timeout optional?
Because, if I'm reading correctly his driver, he is actually implementing it for other use cases, as well. Besides, he has implemented it both in master and slave side. Check for example the function ast2600_i2c_recover_bus(), that is not related to the multi-master capability, but uses this timeout for bus recovery in case of error. The clock stretching was initially made for bus recovery, indeed.
Consider also that in multimaster configurations, masters become slaves and slaves become masters. So that you have a perfect example where you have slaves needing this. "yes, sure, but if we have multimaster defined, we don't need this"... read above.
Be aware that this is a completely optional feature. If in a phone you don't have slaves that understand clock stratching, you leave it out and save yourself some wait_for_completion_timeout(). If you have it (e.g. in complex MCU systems, or cameras), then you have the option to use this. And it's a pretty cool feature, because it's an efficient way to suddenly interrupt a communication (think of i2c's with over 1GHz of bandwidth).
There was a problem hiding this comment.
Thanks, I guess commit msg could be extended with parts of this rationale.
There was a problem hiding this comment.
Thank you a lot, Krzysztof. I appreciate your review. I will add a better commit log.
The I2C specification allows for the clock line to be held low for a specified timeout to force the slave device into a 'wait' mode. This feature is known as 'Clock stretching' and is optional.
In the NXP I2C specification, clock stretching is described as the process of pausing a transaction by holding the SCL line LOW. The transaction can only continue when the line is released HIGH again.[*] However, most target devices do not include an SCL driver and are therefore unable to stretch the clock.
Add the following properties:
i2c-scl-clk-low-timeout-ms: This property specifies the duration, in milliseconds, for which the clock is kept low and a client needs to detect a forced waiting state.
i2c-scl-has-clk-low-timeout: This property specifies whether the I2C controller implements the clock stretching property.
It's important to note that this feature should not be confused with the SMBUS clock timeout, which serves a similar function but specifies a timeout of 25-35ms. The I2C specification does not recommend any specific timeout.
[*] NXP, UM10204 - I2C-bus specification and user manual
Rev. 7.0, 1 October 2021, chapter 3.1.9.