dtschema: i2c: messy situation about timeouts

dtschema: i2c: messy situation about timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

Hey guys,

we have quite a messy situation regarding I2C timeouts in the dtschema.
Partly because I was too busy to pay detailed attention, partly because
reviewing dtschema changes happen on Github which I totally missed. No
complaining, though, here are my observations and suggestions to get it
straight. Comments are more than welcome.

- "i2c-transfer-timeout-us"

Description says "Number of microseconds to wait before considering an
I2C transfer has failed."

To me, this binding is very descriptive and makes sense. We should keep
it. Sadly, it is the newest one and we already have others.


- "i2c-scl-has-clk-low-timeout"

AFAIU this binding tells that the controller can do clock stretching.
But what for? I don't see why this is important for clients. If
anything, then it would be interesting if the *client* can do clock
stretching and if the controller can actually handle that. But no need
to describe it in DT, we have this as an adapter quirk already
'I2C_AQ_NO_CLK_STRETCH'. Two controllers use it, but no client checks
for it so far. Coming back to this binding, it is also unused in the
kernel.

Suggestion: let's remove it


- "i2c-scl-clk-low-timeout-us"

The description says "Number of microseconds the clock line needs to be
pulled down in order to force a waiting state." What does "forcing a
waiting state" mean here? I don't understand this description.

It is used in the i2c-mpc driver. The use case is simply to put it into
the 'struct i2c_adapter.timeout' member. That timeout is used to
determine if a transfer failed. So, to me, "i2c-transfer-timeout-us"
makes a lot more sense to use here.

Suggestion: let's remove this binding and conver i2c-mpc to
"i2c-transfer-timeout-us". Yes, not nice to have two deprecated
bindings, but things happened.


So, these are my thoughts about the current situation. I might have
missed something, so if you have anything to add, I am all ears.
Comments really welcome!

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dtschema: i2c: messy situation about timeouts

[Rob Herring]

On Mon, Feb 26, 2024 at 4:08 AM Wolfram Sang <wsa@kernel.org> wrote:
>
> Hey guys,
>
> we have quite a messy situation regarding I2C timeouts in the dtschema.
> Partly because I was too busy to pay detailed attention, partly because
> reviewing dtschema changes happen on Github which I totally missed. No
> complaining, though, here are my observations and suggestions to get it
> straight. Comments are more than welcome.
>
> - "i2c-transfer-timeout-us"
>
> Description says "Number of microseconds to wait before considering an
> I2C transfer has failed."
>
> To me, this binding is very descriptive and makes sense. We should keep
> it. Sadly, it is the newest one and we already have others.
>
>
> - "i2c-scl-has-clk-low-timeout"
>
> AFAIU this binding tells that the controller can do clock stretching.
> But what for? I don't see why this is important for clients. If
> anything, then it would be interesting if the *client* can do clock
> stretching and if the controller can actually handle that. But no need
> to describe it in DT, we have this as an adapter quirk already
> 'I2C_AQ_NO_CLK_STRETCH'. Two controllers use it, but no client checks
> for it so far. Coming back to this binding, it is also unused in the
> kernel.
>
> Suggestion: let's remove it

Seems like it can be implied by compatible... I'll defer to Andi.

>
>
> - "i2c-scl-clk-low-timeout-us"
>
> The description says "Number of microseconds the clock line needs to be
> pulled down in order to force a waiting state." What does "forcing a
> waiting state" mean here? I don't understand this description.

Does the commit msg or PR help?:
https://github.com/devicetree-org/dt-schema/pull/103

>
> It is used in the i2c-mpc driver. The use case is simply to put it into
> the 'struct i2c_adapter.timeout' member. That timeout is used to
> determine if a transfer failed. So, to me, "i2c-transfer-timeout-us"
> makes a lot more sense to use here.
>
> Suggestion: let's remove this binding and conver i2c-mpc to
> "i2c-transfer-timeout-us". Yes, not nice to have two deprecated
> bindings, but things happened.

Maybe the core code should read it instead?

I think we should mark as deprecated rather than remove unless we can
just remove the properties from the kernel. The reason being that
every property the kernel accesses should be documented. I might start
actually checking that. That's already done for compatible strings,
but those are mostly defined by a common pattern we can extract while
properties are less so.

Rob

Re: dtschema: i2c: messy situation about timeouts

[Chris Packham]

On 26/02/24 23:08, Wolfram Sang wrote:
> Hey guys,
>
> we have quite a messy situation regarding I2C timeouts in the dtschema.
> Partly because I was too busy to pay detailed attention, partly because
> reviewing dtschema changes happen on Github which I totally missed. No
> complaining, though, here are my observations and suggestions to get it
> straight. Comments are more than welcome.
>
> - "i2c-transfer-timeout-us"
>
> Description says "Number of microseconds to wait before considering an
> I2C transfer has failed."
>
> To me, this binding is very descriptive and makes sense. We should keep
> it. Sadly, it is the newest one and we already have others.
>
>
> - "i2c-scl-has-clk-low-timeout"
>
> AFAIU this binding tells that the controller can do clock stretching.
> But what for? I don't see why this is important for clients. If
> anything, then it would be interesting if the *client* can do clock
> stretching and if the controller can actually handle that. But no need
> to describe it in DT, we have this as an adapter quirk already
> 'I2C_AQ_NO_CLK_STRETCH'.

Hmm I know of a few adapters that should probably set 
I2C_AQ_NO_CLK_STRETCH based on some Errata. Probably just a 
documentation exercise. It would be nice to reject clients that need to 
do clock stretching but often it happens as a side effect rather than 
being intentional (I've seen this with i2c clients implemented in 
microcontrollers).

>   Two controllers use it, but no client checks
> for it so far. Coming back to this binding, it is also unused in the
> kernel.
>
> Suggestion: let's remove it
>
>
> - "i2c-scl-clk-low-timeout-us"
>
> The description says "Number of microseconds the clock line needs to be
> pulled down in order to force a waiting state." What does "forcing a
> waiting state" mean here? I don't understand this description.
>
> It is used in the i2c-mpc driver. The use case is simply to put it into
> the 'struct i2c_adapter.timeout' member. That timeout is used to
> determine if a transfer failed. So, to me, "i2c-transfer-timeout-us"
> makes a lot more sense to use here.
>
> Suggestion: let's remove this binding and conver i2c-mpc to
> "i2c-transfer-timeout-us". Yes, not nice to have two deprecated
> bindings, but things happened.

Sounds like a good idea. We'd obviously need to keep support for the 
existing property but it wouldn't be hard to add 
"i2c-transfer-timeout-us". I'll try to whip up a patch for that sometime 
this week, just need to dust off my Freescale boards.

> So, these are my thoughts about the current situation. I might have
> missed something, so if you have anything to add, I am all ears.
> Comments really welcome!
>
> Happy hacking,
>
>     Wolfram
>

Re: dtschema: i2c: messy situation about timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

Hi Rob,

> > - "i2c-scl-clk-low-timeout-us"
> >
> > The description says "Number of microseconds the clock line needs to be
> > pulled down in order to force a waiting state." What does "forcing a
> > waiting state" mean here? I don't understand this description.
> 
> Does the commit msg or PR help?:
> https://github.com/devicetree-org/dt-schema/pull/103

I checked it beforehand. Sadly, it didn't help me.

> > It is used in the i2c-mpc driver. The use case is simply to put it into
> > the 'struct i2c_adapter.timeout' member. That timeout is used to
> > determine if a transfer failed. So, to me, "i2c-transfer-timeout-us"
> > makes a lot more sense to use here.
> >
> > Suggestion: let's remove this binding and conver i2c-mpc to
> > "i2c-transfer-timeout-us". Yes, not nice to have two deprecated
> > bindings, but things happened.
> 
> Maybe the core code should read it instead?

That's an interesting idea. I'll try to implement this tomorrow.

> I think we should mark as deprecated rather than remove unless we can
> just remove the properties from the kernel. The reason being that

You are right. I should have said "depreacte" instead of remove here.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dtschema: i2c: messy situation about timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]

Hi Chris,

> > - "i2c-scl-has-clk-low-timeout"
> >
> > AFAIU this binding tells that the controller can do clock stretching.
> > But what for? I don't see why this is important for clients. If
> > anything, then it would be interesting if the *client* can do clock
> > stretching and if the controller can actually handle that. But no need
> > to describe it in DT, we have this as an adapter quirk already
> > 'I2C_AQ_NO_CLK_STRETCH'.
> 
> Hmm I know of a few adapters that should probably set 
> I2C_AQ_NO_CLK_STRETCH based on some Errata. Probably just a 

That would be helpful if you could add that. I always guessed there
should be more controllers needing the flag but never encountered them
personally.

> documentation exercise. It would be nice to reject clients that need to 
> do clock stretching but often it happens as a side effect rather than 
> being intentional (I've seen this with i2c clients implemented in 
> microcontrollers).

I guess the way forward with that is we add a flag like
I2C_CLIENT_CLK_STRETCH and let the core figure out if controller and
client are compatible. Dunno what to do if not, though. Reject? Throw a
warning that there might be problems? Probably reject by default unless
overridden by something-meaning-i-know-the-risks.

> > Suggestion: let's remove this binding and conver i2c-mpc to
> > "i2c-transfer-timeout-us". Yes, not nice to have two deprecated
> > bindings, but things happened.
> 
> Sounds like a good idea. We'd obviously need to keep support for the 
> existing property but it wouldn't be hard to add 
> "i2c-transfer-timeout-us". I'll try to whip up a patch for that sometime 
> this week, just need to dust off my Freescale boards.

Please wait a little with the implementation. I want to try Rob's idea
to let the core set the timeout flag initially. Dusting off the boards
for testing would be awesome, though :)

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dtschema: i2c: messy situation about timeouts

[Andi Shyti]

Hi Wolfram,

On Mon, Feb 26, 2024 at 11:08:27AM +0100, Wolfram Sang wrote:
> Hey guys,
> 
> we have quite a messy situation regarding I2C timeouts in the dtschema.
> Partly because I was too busy to pay detailed attention, partly because
> reviewing dtschema changes happen on Github which I totally missed. No
> complaining, though, here are my observations and suggestions to get it
> straight. Comments are more than welcome.
> 
> - "i2c-transfer-timeout-us"
> 
> Description says "Number of microseconds to wait before considering an
> I2C transfer has failed."
> 
> To me, this binding is very descriptive and makes sense. We should keep
> it. Sadly, it is the newest one and we already have others.
> 
> 
> - "i2c-scl-has-clk-low-timeout"
> 
> AFAIU this binding tells that the controller can do clock stretching.
> But what for?

One of the controllers that was sent a while back required some
hardware description because, in some versions, clock stretching
was supported in the hardware.

Depending on this, the driver could either use it or force the
clock GPIO value down.

That's why I made this change.

The naming is a bit fancy, but it depends on the spec used as a
reference; smbus, i2c or specific drivers often call it in a
different way.

The naming is a bit fancy, but it depends on the specification
used as a reference; SMBus, I2C, or specific drivers often refer
to it differently.

> I don't see why this is important for clients. If
> anything, then it would be interesting if the *client* can do clock
> stretching and if the controller can actually handle that. But no need
> to describe it in DT, we have this as an adapter quirk already
> 'I2C_AQ_NO_CLK_STRETCH'. Two controllers use it, but no client checks
> for it so far. Coming back to this binding, it is also unused in the
> kernel.
> 
> Suggestion: let's remove it
> 
> 
> - "i2c-scl-clk-low-timeout-us"
> 
> The description says "Number of microseconds the clock line needs to be
> pulled down in order to force a waiting state." What does "forcing a
> waiting state" mean here? I don't understand this description.

It comes from the specification. The clock stretching is given as
an interval that can be tweaked depending on the hardware.

So far I haven't seen anyone using it, though.

Andi

> It is used in the i2c-mpc driver. The use case is simply to put it into
> the 'struct i2c_adapter.timeout' member. That timeout is used to
> determine if a transfer failed. So, to me, "i2c-transfer-timeout-us"
> makes a lot more sense to use here.
> 
> Suggestion: let's remove this binding and conver i2c-mpc to
> "i2c-transfer-timeout-us". Yes, not nice to have two deprecated
> bindings, but things happened.
> 
> 
> So, these are my thoughts about the current situation. I might have
> missed something, so if you have anything to add, I am all ears.
> Comments really welcome!
> 
> Happy hacking,
> 
>    Wolfram
> 

Re: dtschema: i2c: messy situation about timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]

Hi Andi,

> > - "i2c-scl-has-clk-low-timeout"
> > 
> > AFAIU this binding tells that the controller can do clock stretching.
> > But what for?
> 
> One of the controllers that was sent a while back required some
> hardware description because, in some versions, clock stretching
> was supported in the hardware.

I see. Still, I think this can (and should be handled) with
I2C_AQ_NO_CLK_STRETCH.

> The naming is a bit fancy, but it depends on the specification
> used as a reference; SMBus, I2C, or specific drivers often refer
> to it differently.

Yes. I'd give I2C specs most importance, controller documentation least.
And probably a salt of personal preference :)

> > - "i2c-scl-clk-low-timeout-us"
> > 
> > The description says "Number of microseconds the clock line needs to be
> > pulled down in order to force a waiting state." What does "forcing a
> > waiting state" mean here? I don't understand this description.
> 
> It comes from the specification. The clock stretching is given as
> an interval that can be tweaked depending on the hardware.

You mean the maximum clock stretching is tweakable? That, in deed, could
be a binding in the future, in theory. Yet, it would need support in the
client drivers. Like a touchscreen driver which assumes a reset after a
certain time of inactivity.

> So far I haven't seen anyone using it, though.

The MPC driver used it, just for a different purpose.  It was used for a
timeout of the total transfer. That's a different thing. So, I suggested
the conversion.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dtschema: i2c: messy situation about timeouts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]


> > > - "i2c-scl-clk-low-timeout-us"
> > > 
> > > The description says "Number of microseconds the clock line needs to be
> > > pulled down in order to force a waiting state." What does "forcing a
> > > waiting state" mean here? I don't understand this description.
> > 
> > It comes from the specification. The clock stretching is given as
> > an interval that can be tweaked depending on the hardware.
> 
> You mean the maximum clock stretching is tweakable? That, in deed, could
> be a binding in the future, in theory. Yet, it would need support in the
> client drivers. Like a touchscreen driver which assumes a reset after a
> certain time of inactivity.

To sum it up: a binding defining the maximum time for clock stretching
makes sense in theory. I am currently not aware of a controller where
this could be used (but I surely don't know them all). Most of them keep
SCL low as long as they are busy internally. Not tweakable. So, we defer
this until there is a usecase.

If we ever add it, the above name of the binding cannot be used anymore
because i2c-mpc used it with a different purpose. Not so bad IMO because
"scl-clk" is a pleonasm anyway. I'd suggest "i2c-scl-max-stretch-us" but
am open for suggestions then.

This one can just be deprecated, I'd say.

Happy hacking!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]