linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i2c: cadence: Increase timeout per message if necessary
@ 2022-03-09  9:31 Lucas Tanure
  2022-03-21 15:57 ` Michal Simek
  0 siblings, 1 reply; 6+ messages in thread
From: Lucas Tanure @ 2022-03-09  9:31 UTC (permalink / raw)
  To: Michal Simek
  Cc: linux-arm-kernel, linux-i2c, linux-kernel, patches, Lucas Tanure

Timeout as 1 second sets a upper limit on the length of
the transfer executed, but there is no maximum length of
a write or read message set in i2c_adapter_quirks for this
controller.

To remove that limitation calculate the minimal time
necessary, plus some wiggle room, for every message
and use it instead of the default one second, if
more than one second.

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/i2c/busses/i2c-cadence.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 805c77143a0f..b4c1ad19cdae 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -760,7 +760,7 @@ static void cdns_i2c_master_reset(struct i2c_adapter *adap)
 static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
 		struct i2c_adapter *adap)
 {
-	unsigned long time_left;
+	unsigned long time_left, msg_timeout;
 	u32 reg;
 
 	id->p_msg = msg;
@@ -785,8 +785,16 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
 	else
 		cdns_i2c_msend(id);
 
+	/* Minimal time to execute this message */
+	msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) / id->i2c_clk);
+	/* Plus some wiggle room */
+	msg_timeout += msecs_to_jiffies(500);
+
+	if (msg_timeout < adap->timeout)
+		msg_timeout = adap->timeout;
+
 	/* Wait for the signal of completion */
-	time_left = wait_for_completion_timeout(&id->xfer_done, adap->timeout);
+	time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
 	if (time_left == 0) {
 		cdns_i2c_master_reset(adap);
 		dev_err(id->adap.dev.parent,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] i2c: cadence: Increase timeout per message if necessary
  2022-03-09  9:31 [PATCH] i2c: cadence: Increase timeout per message if necessary Lucas Tanure
@ 2022-03-21 15:57 ` Michal Simek
  2022-03-22 15:34   ` tanureal
  0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2022-03-21 15:57 UTC (permalink / raw)
  To: Lucas Tanure, Michal Simek, Shubhrajyoti Datta
  Cc: linux-arm-kernel, linux-i2c, linux-kernel, patches

+Shubhrajyoti

On 3/9/22 10:31, Lucas Tanure wrote:
> Timeout as 1 second sets a upper limit on the length of
> the transfer executed, but there is no maximum length of
> a write or read message set in i2c_adapter_quirks for this
> controller.

nit: I would expect that you have run any test and you reached an issue.
Would be good to describe what exactly you have tried on which configuration to 
make it super clear.

> 
> To remove that limitation calculate the minimal time
> necessary, plus some wiggle room, for every message
> and use it instead of the default one second, if
> more than one second.
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 805c77143a0f..b4c1ad19cdae 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -760,7 +760,7 @@ static void cdns_i2c_master_reset(struct i2c_adapter *adap)
>   static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
>   		struct i2c_adapter *adap)
>   {
> -	unsigned long time_left;
> +	unsigned long time_left, msg_timeout;
>   	u32 reg;
>   
>   	id->p_msg = msg;
> @@ -785,8 +785,16 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
>   	else
>   		cdns_i2c_msend(id);
>   
> +	/* Minimal time to execute this message */
> +	msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) / id->i2c_clk);
> +	/* Plus some wiggle room */
> +	msg_timeout += msecs_to_jiffies(500);
> +
> +	if (msg_timeout < adap->timeout)
> +		msg_timeout = adap->timeout;
> +
>   	/* Wait for the signal of completion */
> -	time_left = wait_for_completion_timeout(&id->xfer_done, adap->timeout);
> +	time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
>   	if (time_left == 0) {
>   		cdns_i2c_master_reset(adap);
>   		dev_err(id->adap.dev.parent,


If my assumption is right and there is any actual issue you had please send v2 
and feel free to add there my:
Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] i2c: cadence: Increase timeout per message if necessary
  2022-03-21 15:57 ` Michal Simek
@ 2022-03-22 15:34   ` tanureal
  2022-03-22 17:18     ` Michal Simek
  0 siblings, 1 reply; 6+ messages in thread
From: tanureal @ 2022-03-22 15:34 UTC (permalink / raw)
  To: Michal Simek, Michal Simek, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c, linux-kernel, patches

On 3/21/22 3:57 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> +Shubhrajyoti
> 
> On 3/9/22 10:31, Lucas Tanure wrote:
> > Timeout as 1 second sets a upper limit on the length of
> > the transfer executed, but there is no maximum length of
> > a write or read message set in i2c_adapter_quirks for this
> > controller.
> 
> nit: I would expect that you have run any test and you reached an issue.
> Would be good to describe what exactly you have tried on which 
> configuration to make it super clear.
> 
> >
> > To remove that limitation calculate the minimal time
> > necessary, plus some wiggle room, for every message
> > and use it instead of the default one second, if
> > more than one second.
> >
> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> > ---
> >   drivers/i2c/busses/i2c-cadence.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-cadence.c 
> > b/drivers/i2c/busses/i2c-cadence.c
> > index 805c77143a0f..b4c1ad19cdae 100644
> > --- a/drivers/i2c/busses/i2c-cadence.c
> > +++ b/drivers/i2c/busses/i2c-cadence.c
> > @@ -760,7 +760,7 @@ static void cdns_i2c_master_reset(struct 
> > i2c_adapter *adap)
> >   static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg 
> > *msg,
> >           struct i2c_adapter *adap)
> >   {
> > -    unsigned long time_left;
> > +    unsigned long time_left, msg_timeout;
> >       u32 reg;
> >       id->p_msg = msg;
> > @@ -785,8 +785,16 @@ static int cdns_i2c_process_msg(struct cdns_i2c 
> > *id, struct i2c_msg *msg,
> >       else
> >           cdns_i2c_msend(id);
> > +    /* Minimal time to execute this message */
> > +    msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) 
> > / id->i2c_clk);
> > +    /* Plus some wiggle room */
> > +    msg_timeout += msecs_to_jiffies(500);
> > +
> > +    if (msg_timeout < adap->timeout)
> > +        msg_timeout = adap->timeout;
> > +
> >       /* Wait for the signal of completion */
> > -    time_left = wait_for_completion_timeout(&id->xfer_done, 
> > adap->timeout);
> > +    time_left = wait_for_completion_timeout(&id->xfer_done, 
> > msg_timeout);
> >       if (time_left == 0) {
> >           cdns_i2c_master_reset(adap);
> >           dev_err(id->adap.dev.parent,
> 
> 
> If my assumption is right and there is any actual issue you had please 
> send v2 and feel free to add there my:
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> 
> Thanks,
> Michal
> 
> 
> 
The issue happens for I2C devices that have firmware, which will send a big I2C message, but the I2C controller will timeout on it.
That happened for CS35L41 DSP firmware tests, so no particular configuration, just a driver sending firmware blob over I2C.

Thanks,
Lucas

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] i2c: cadence: Increase timeout per message if necessary
  2022-03-22 15:34   ` tanureal
@ 2022-03-22 17:18     ` Michal Simek
  2022-03-23 10:07       ` tanureal
  0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2022-03-22 17:18 UTC (permalink / raw)
  To: tanureal, Michal Simek, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c, linux-kernel, patches



On 3/22/22 16:34, tanureal@opensource.cirrus.com wrote:
> On 3/21/22 3:57 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>> +Shubhrajyoti
>>
>> On 3/9/22 10:31, Lucas Tanure wrote:
>> > Timeout as 1 second sets a upper limit on the length of
>> > the transfer executed, but there is no maximum length of
>> > a write or read message set in i2c_adapter_quirks for this
>> > controller.
>>
>> nit: I would expect that you have run any test and you reached an issue.
>> Would be good to describe what exactly you have tried on which configuration 
>> to make it super clear.
>>
>> >
>> > To remove that limitation calculate the minimal time
>> > necessary, plus some wiggle room, for every message
>> > and use it instead of the default one second, if
>> > more than one second.
>> >
>> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
>> > ---
>> >   drivers/i2c/busses/i2c-cadence.c | 12 ++++++++++--
>> >   1 file changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-cadence.c > 
>> b/drivers/i2c/busses/i2c-cadence.c
>> > index 805c77143a0f..b4c1ad19cdae 100644
>> > --- a/drivers/i2c/busses/i2c-cadence.c
>> > +++ b/drivers/i2c/busses/i2c-cadence.c
>> > @@ -760,7 +760,7 @@ static void cdns_i2c_master_reset(struct > i2c_adapter 
>> *adap)
>> >   static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg > *msg,
>> >           struct i2c_adapter *adap)
>> >   {
>> > -    unsigned long time_left;
>> > +    unsigned long time_left, msg_timeout;
>> >       u32 reg;
>> >       id->p_msg = msg;
>> > @@ -785,8 +785,16 @@ static int cdns_i2c_process_msg(struct cdns_i2c > *id, 
>> struct i2c_msg *msg,
>> >       else
>> >           cdns_i2c_msend(id);
>> > +    /* Minimal time to execute this message */
>> > +    msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) > / 
>> id->i2c_clk);
>> > +    /* Plus some wiggle room */
>> > +    msg_timeout += msecs_to_jiffies(500);
>> > +
>> > +    if (msg_timeout < adap->timeout)
>> > +        msg_timeout = adap->timeout;
>> > +
>> >       /* Wait for the signal of completion */
>> > -    time_left = wait_for_completion_timeout(&id->xfer_done, > adap->timeout);
>> > +    time_left = wait_for_completion_timeout(&id->xfer_done, > msg_timeout);
>> >       if (time_left == 0) {
>> >           cdns_i2c_master_reset(adap);
>> >           dev_err(id->adap.dev.parent,
>>
>>
>> If my assumption is right and there is any actual issue you had please send v2 
>> and feel free to add there my:
>> Acked-by: Michal Simek <michal.simek@xilinx.com>
>>
>> Thanks,
>> Michal
>>
>>
>>
> The issue happens for I2C devices that have firmware, which will send a big I2C 
> message, but the I2C controller will timeout on it.
> That happened for CS35L41 DSP firmware tests, so no particular configuration, 
> just a driver sending firmware blob over I2C.

How big is it?

M

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] i2c: cadence: Increase timeout per message if necessary
  2022-03-22 17:18     ` Michal Simek
@ 2022-03-23 10:07       ` tanureal
  2022-03-23 11:02         ` Michal Simek
  0 siblings, 1 reply; 6+ messages in thread
From: tanureal @ 2022-03-23 10:07 UTC (permalink / raw)
  To: Michal Simek, Michal Simek, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c, linux-kernel, patches

On 3/22/22 5:18 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
> 
> On 3/22/22 16:34, tanureal@opensource.cirrus.com wrote:
> > On 3/21/22 3:57 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> >> +Shubhrajyoti
> >>
> >> On 3/9/22 10:31, Lucas Tanure wrote:
> >> > Timeout as 1 second sets a upper limit on the length of
> >> > the transfer executed, but there is no maximum length of
> >> > a write or read message set in i2c_adapter_quirks for this
> >> > controller.
> >>
> >> nit: I would expect that you have run any test and you reached an issue.
> >> Would be good to describe what exactly you have tried on which 
> >> configuration to make it super clear.
> >>
> >> >
> >> > To remove that limitation calculate the minimal time
> >> > necessary, plus some wiggle room, for every message
> >> > and use it instead of the default one second, if
> >> > more than one second.
> >> >
> >> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> >> > ---
> >> >   drivers/i2c/busses/i2c-cadence.c | 12 ++++++++++--
> >> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/i2c/busses/i2c-cadence.c > 
> >> b/drivers/i2c/busses/i2c-cadence.c
> >> > index 805c77143a0f..b4c1ad19cdae 100644
> >> > --- a/drivers/i2c/busses/i2c-cadence.c
> >> > +++ b/drivers/i2c/busses/i2c-cadence.c
> >> > @@ -760,7 +760,7 @@ static void cdns_i2c_master_reset(struct > 
> >> i2c_adapter *adap)
> >> >   static int cdns_i2c_process_msg(struct cdns_i2c *id, struct 
> >> i2c_msg > *msg,
> >> >           struct i2c_adapter *adap)
> >> >   {
> >> > -    unsigned long time_left;
> >> > +    unsigned long time_left, msg_timeout;
> >> >       u32 reg;
> >> >       id->p_msg = msg;
> >> > @@ -785,8 +785,16 @@ static int cdns_i2c_process_msg(struct 
> >> cdns_i2c > *id, struct i2c_msg *msg,
> >> >       else
> >> >           cdns_i2c_msend(id);
> >> > +    /* Minimal time to execute this message */
> >> > +    msg_timeout = msecs_to_jiffies((1000 * msg->len * 
> >> BITS_PER_BYTE) > / id->i2c_clk);
> >> > +    /* Plus some wiggle room */
> >> > +    msg_timeout += msecs_to_jiffies(500);
> >> > +
> >> > +    if (msg_timeout < adap->timeout)
> >> > +        msg_timeout = adap->timeout;
> >> > +
> >> >       /* Wait for the signal of completion */
> >> > -    time_left = wait_for_completion_timeout(&id->xfer_done, > 
> >> adap->timeout);
> >> > +    time_left = wait_for_completion_timeout(&id->xfer_done, > 
> >> msg_timeout);
> >> >       if (time_left == 0) {
> >> >           cdns_i2c_master_reset(adap);
> >> >           dev_err(id->adap.dev.parent,
> >>
> >>
> >> If my assumption is right and there is any actual issue you had 
> >> please send v2 and feel free to add there my:
> >> Acked-by: Michal Simek <michal.simek@xilinx.com>
> >>
> >> Thanks,
> >> Michal
> >>
> >>
> >>
> > The issue happens for I2C devices that have firmware, which will send 
> > a big I2C message, but the I2C controller will timeout on it.
> > That happened for CS35L41 DSP firmware tests, so no particular 
> > configuration, just a driver sending firmware blob over I2C.
> 
> How big is it?
> 
> M
> 
The firmware has 33868 bytes, and it is split in a few writes. The first one to time out has 20240 bytes:

[   53.398444] cs35l41 0-0040: DSP1: Firmware version: 3
[   53.403522] cs35l41 0-0040: DSP1: cs35l41-dsp1-spk-prot.wmfw: Fri 04 Feb 2022 12:01:42 W. Europe Standard Time
[   55.331688] cdns-i2c e0004000.i2c: timeout waiting on completion
[   55.336721] cs35l41 0-0040: DSP1: cs35l41-dsp1-spk-prot.wmfw.5: Failed to write 20240 bytes at 0 in PM_PACKED: -110

20240 bytes at 100k clock should take 1.6192 seconds, which is more than the current timeout of one second.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] i2c: cadence: Increase timeout per message if necessary
  2022-03-23 10:07       ` tanureal
@ 2022-03-23 11:02         ` Michal Simek
  0 siblings, 0 replies; 6+ messages in thread
From: Michal Simek @ 2022-03-23 11:02 UTC (permalink / raw)
  To: tanureal, Michal Simek, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c, linux-kernel, patches



On 3/23/22 11:07, tanureal@opensource.cirrus.com wrote:
> On 3/22/22 5:18 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>>
>> On 3/22/22 16:34, tanureal@opensource.cirrus.com wrote:
>> > On 3/21/22 3:57 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>> >> +Shubhrajyoti
>> >>
>> >> On 3/9/22 10:31, Lucas Tanure wrote:
>> >> > Timeout as 1 second sets a upper limit on the length of
>> >> > the transfer executed, but there is no maximum length of
>> >> > a write or read message set in i2c_adapter_quirks for this
>> >> > controller.
>> >>
>> >> nit: I would expect that you have run any test and you reached an issue.
>> >> Would be good to describe what exactly you have tried on which >> 
>> configuration to make it super clear.
>> >>
>> >> >
>> >> > To remove that limitation calculate the minimal time
>> >> > necessary, plus some wiggle room, for every message
>> >> > and use it instead of the default one second, if
>> >> > more than one second.
>> >> >
>> >> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
>> >> > ---
>> >> >   drivers/i2c/busses/i2c-cadence.c | 12 ++++++++++--
>> >> >   1 file changed, 10 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/i2c/busses/i2c-cadence.c > >> 
>> b/drivers/i2c/busses/i2c-cadence.c
>> >> > index 805c77143a0f..b4c1ad19cdae 100644
>> >> > --- a/drivers/i2c/busses/i2c-cadence.c
>> >> > +++ b/drivers/i2c/busses/i2c-cadence.c
>> >> > @@ -760,7 +760,7 @@ static void cdns_i2c_master_reset(struct > >> 
>> i2c_adapter *adap)
>> >> >   static int cdns_i2c_process_msg(struct cdns_i2c *id, struct >> i2c_msg 
>> > *msg,
>> >> >           struct i2c_adapter *adap)
>> >> >   {
>> >> > -    unsigned long time_left;
>> >> > +    unsigned long time_left, msg_timeout;
>> >> >       u32 reg;
>> >> >       id->p_msg = msg;
>> >> > @@ -785,8 +785,16 @@ static int cdns_i2c_process_msg(struct >> cdns_i2c > 
>> *id, struct i2c_msg *msg,
>> >> >       else
>> >> >           cdns_i2c_msend(id);
>> >> > +    /* Minimal time to execute this message */
>> >> > +    msg_timeout = msecs_to_jiffies((1000 * msg->len * >> BITS_PER_BYTE) 
>> > / id->i2c_clk);
>> >> > +    /* Plus some wiggle room */
>> >> > +    msg_timeout += msecs_to_jiffies(500);
>> >> > +
>> >> > +    if (msg_timeout < adap->timeout)
>> >> > +        msg_timeout = adap->timeout;
>> >> > +
>> >> >       /* Wait for the signal of completion */
>> >> > -    time_left = wait_for_completion_timeout(&id->xfer_done, > >> 
>> adap->timeout);
>> >> > +    time_left = wait_for_completion_timeout(&id->xfer_done, > >> 
>> msg_timeout);
>> >> >       if (time_left == 0) {
>> >> >           cdns_i2c_master_reset(adap);
>> >> >           dev_err(id->adap.dev.parent,
>> >>
>> >>
>> >> If my assumption is right and there is any actual issue you had >> please 
>> send v2 and feel free to add there my:
>> >> Acked-by: Michal Simek <michal.simek@xilinx.com>
>> >>
>> >> Thanks,
>> >> Michal
>> >>
>> >>
>> >>
>> > The issue happens for I2C devices that have firmware, which will send > a 
>> big I2C message, but the I2C controller will timeout on it.
>> > That happened for CS35L41 DSP firmware tests, so no particular > 
>> configuration, just a driver sending firmware blob over I2C.
>>
>> How big is it?
>>
>> M
>>
> The firmware has 33868 bytes, and it is split in a few writes. The first one to 
> time out has 20240 bytes:
> 
> [   53.398444] cs35l41 0-0040: DSP1: Firmware version: 3
> [   53.403522] cs35l41 0-0040: DSP1: cs35l41-dsp1-spk-prot.wmfw: Fri 04 Feb 2022 
> 12:01:42 W. Europe Standard Time
> [   55.331688] cdns-i2c e0004000.i2c: timeout waiting on completion
> [   55.336721] cs35l41 0-0040: DSP1: cs35l41-dsp1-spk-prot.wmfw.5: Failed to 
> write 20240 bytes at 0 in PM_PACKED: -110
> 
> 20240 bytes at 100k clock should take 1.6192 seconds, which is more than the 
> current timeout of one second.

ok.

thanks,
Michal

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-03-23 11:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-09  9:31 [PATCH] i2c: cadence: Increase timeout per message if necessary Lucas Tanure
2022-03-21 15:57 ` Michal Simek
2022-03-22 15:34   ` tanureal
2022-03-22 17:18     ` Michal Simek
2022-03-23 10:07       ` tanureal
2022-03-23 11:02         ` Michal Simek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).