[PATCH] can: flexcan: fix timeout when set small bitrate

[PATCH] can: flexcan: fix timeout when set small bitrate

[Joakim Zhang]

From: Dong Aisheng <aisheng.dong@nxp.com>

Current we can meet timeout issue when setting a small bitrate
like 10000 as follows:
root@imx6qdlsolo:~# ip link set can0 up type can bitrate 10000
A link change request failed with some changes committed already.
Interface can0 may have been left with an inconsistent configuration,
please check.
RTNETLINK answers: Connection timed out

It is caused by calling of flexcan_chip_unfreeze() timeout.

Originally the code is using usleep_range(10, 20) for unfreeze operation,
but the patch (8badd65 can: flexcan: avoid calling usleep_range from
interrupt context) changed it into udelay(10) which is only a half delay
of before, there're also some other delay changes.

After only changed unfreeze delay back to udelay(20), the issue is gone.
So other timeout values are kept the same as 8badd65 changed.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 2bca867bcfaa..1d3a9053bbeb 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -530,7 +530,7 @@ static int flexcan_chip_unfreeze(struct flexcan_priv *priv)
 	priv->write(reg, &regs->mcr);
 
 	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK))
-		udelay(10);
+		udelay(20);
 
 	if (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK)
 		return -ETIMEDOUT;
-- 
2.17.1

Re: [PATCH] can: flexcan: fix timeout when set small bitrate

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2059 bytes --]

On 1/31/19 7:58 AM, Joakim Zhang wrote:
> From: Dong Aisheng <aisheng.dong@nxp.com>
> 
> Current we can meet timeout issue when setting a small bitrate
> like 10000 as follows:
> root@imx6qdlsolo:~# ip link set can0 up type can bitrate 10000
> A link change request failed with some changes committed already.
> Interface can0 may have been left with an inconsistent configuration,
> please check.
> RTNETLINK answers: Connection timed out

Which SoC are you using? Which clock rate has the flexcan IP core?

> It is caused by calling of flexcan_chip_unfreeze() timeout.
> 
> Originally the code is using usleep_range(10, 20) for unfreeze operation,
> but the patch (8badd65 can: flexcan: avoid calling usleep_range from
> interrupt context) changed it into udelay(10) which is only a half delay
> of before, there're also some other delay changes.
> 
> After only changed unfreeze delay back to udelay(20), the issue is gone.
> So other timeout values are kept the same as 8badd65 changed.

Can you change FLEXCAN_TIMEOUT_US instead?

> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 2bca867bcfaa..1d3a9053bbeb 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -530,7 +530,7 @@ static int flexcan_chip_unfreeze(struct flexcan_priv *priv)
>  	priv->write(reg, &regs->mcr);
>  
>  	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK))
> -		udelay(10);
> +		udelay(20);
>  
>  	if (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK)
>  		return -ETIMEDOUT;
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] can: flexcan: fix timeout when set small bitrate

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年1月31日 15:34
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Aisheng
> Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH] can: flexcan: fix timeout when set small bitrate
> 
> On 1/31/19 7:58 AM, Joakim Zhang wrote:
> > From: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > Current we can meet timeout issue when setting a small bitrate like
> > 10000 as follows:
> > root@imx6qdlsolo:~# ip link set can0 up type can bitrate 10000 A link
> > change request failed with some changes committed already.
> > Interface can0 may have been left with an inconsistent configuration,
> > please check.
> > RTNETLINK answers: Connection timed out
> 
> Which SoC are you using? Which clock rate has the flexcan IP core?

  We tested on i.MX6 series boards and all met this issue. And ipg clock rate is 66MHZ, per clock rate is 30MHZ.

> > It is caused by calling of flexcan_chip_unfreeze() timeout.
> >
> > Originally the code is using usleep_range(10, 20) for unfreeze
> > operation, but the patch (8badd65 can: flexcan: avoid calling
> > usleep_range from interrupt context) changed it into udelay(10) which
> > is only a half delay of before, there're also some other delay changes.
> >
> > After only changed unfreeze delay back to udelay(20), the issue is gone.
> > So other timeout values are kept the same as 8badd65 changed.
> 
> Can you change FLEXCAN_TIMEOUT_US instead?

  Of course, we can change FLEXCAN_TIMEOUT_US to 100, but this will extend the time of enable/disable/softreset.
Which method do you think is better?

Best Regards,
Joakim Zhang

> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/flexcan.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 2bca867bcfaa..1d3a9053bbeb 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -530,7 +530,7 @@ static int flexcan_chip_unfreeze(struct flexcan_priv
> *priv)
> >  	priv->write(reg, &regs->mcr);
> >
> >  	while (timeout-- && (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK))
> > -		udelay(10);
> > +		udelay(20);
> >
> >  	if (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK)
> >  		return -ETIMEDOUT;
> >
> 
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

RE: [PATCH] can: flexcan: fix timeout when set small bitrate

[Aisheng Dong]

> From: Joakim Zhang
> Sent: Thursday, January 31, 2019 4:49 PM
[...]
> > > Current we can meet timeout issue when setting a small bitrate like
> > > 10000 as follows:
> > > root@imx6qdlsolo:~# ip link set can0 up type can bitrate 10000 A
> > > link change request failed with some changes committed already.
> > > Interface can0 may have been left with an inconsistent
> > > configuration, please check.
> > > RTNETLINK answers: Connection timed out
> >
> > Which SoC are you using? Which clock rate has the flexcan IP core?
> 
>   We tested on i.MX6 series boards and all met this issue. And ipg clock rate is
> 66MHZ, per clock rate is 30MHZ.
> 
> > > It is caused by calling of flexcan_chip_unfreeze() timeout.
> > >
> > > Originally the code is using usleep_range(10, 20) for unfreeze
> > > operation, but the patch (8badd65 can: flexcan: avoid calling
> > > usleep_range from interrupt context) changed it into udelay(10)
> > > which is only a half delay of before, there're also some other delay
> changes.
> > >
> > > After only changed unfreeze delay back to udelay(20), the issue is gone.
> > > So other timeout values are kept the same as 8badd65 changed.
> >
> > Can you change FLEXCAN_TIMEOUT_US instead?
> 
>   Of course, we can change FLEXCAN_TIMEOUT_US to 100, but this will
> extend the time of enable/disable/softreset.
> Which method do you think is better?
> 

That seems not a big deal for an error case.
So change FLEXCAN_TIMEOUT_US seems like a good suggestion to me.
You can cook a patch with commit message updated. No need keep my author name
as it's a different solution.

Regards
Dong Aisheng

> Best Regards,
> Joakim Zhang
> 
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > >  drivers/net/can/flexcan.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > > index 2bca867bcfaa..1d3a9053bbeb 100644
> > > --- a/drivers/net/can/flexcan.c
> > > +++ b/drivers/net/can/flexcan.c
> > > @@ -530,7 +530,7 @@ static int flexcan_chip_unfreeze(struct
> > > flexcan_priv
> > *priv)
> > >  	priv->write(reg, &regs->mcr);
> > >
> > >  	while (timeout-- && (priv->read(&regs->mcr) &
> FLEXCAN_MCR_FRZ_ACK))
> > > -		udelay(10);
> > > +		udelay(20);
> > >
> > >  	if (priv->read(&regs->mcr) & FLEXCAN_MCR_FRZ_ACK)
> > >  		return -ETIMEDOUT;
> > >
> >
> > Marc
> >
> > --
> > Pengutronix e.K.                  | Marc Kleine-Budde           |
> > Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> > Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> > Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Re: [PATCH] can: flexcan: fix timeout when set small bitrate

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1801 bytes --]

On 1/31/19 9:48 AM, Joakim Zhang wrote:
>> Which SoC are you using? Which clock rate has the flexcan IP core?
> 
>   We tested on i.MX6 series boards and all met this issue. And ipg clock rate is 66MHZ, per clock rate is 30MHZ.

ok

> 
>>> It is caused by calling of flexcan_chip_unfreeze() timeout.
>>>
>>> Originally the code is using usleep_range(10, 20) for unfreeze
>>> operation, but the patch (8badd65 can: flexcan: avoid calling
>>> usleep_range from interrupt context) changed it into udelay(10) which
>>> is only a half delay of before, there're also some other delay changes.
>>>
>>> After only changed unfreeze delay back to udelay(20), the issue is gone.
>>> So other timeout values are kept the same as 8badd65 changed.
>>
>> Can you change FLEXCAN_TIMEOUT_US instead?
> 
>   Of course, we can change FLEXCAN_TIMEOUT_US to 100, but this will extend the time of enable/disable/softreset.
> Which method do you think is better?

If you double to FLEXCAN_TIMEOUT_US to 100, the loops in question will
spin at maximum the double time. But the loops are left as soon as the
condition is satisfied.

It will fix your problem with the 10 kbit/s bitrate. But if there is
some kind of problem with the IP core it will still fail, it just takes
double amount of time (100 µs + overhead) until the function returns.

I don't see any harm in looping longer:
- The previous good case is unchanged.
- The error case takes double amount of time.
- Your problem is hopefully fixed.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] can: flexcan: fix timeout when set small bitrate

[Joakim Zhang]

Hi Marc,

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年1月31日 17:12
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Aisheng
> Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH] can: flexcan: fix timeout when set small bitrate
> 
> On 1/31/19 9:48 AM, Joakim Zhang wrote:
> >> Which SoC are you using? Which clock rate has the flexcan IP core?
> >
> >   We tested on i.MX6 series boards and all met this issue. And ipg clock rate
> is 66MHZ, per clock rate is 30MHZ.
> 
> ok
> 
> >
> >>> It is caused by calling of flexcan_chip_unfreeze() timeout.
> >>>
> >>> Originally the code is using usleep_range(10, 20) for unfreeze
> >>> operation, but the patch (8badd65 can: flexcan: avoid calling
> >>> usleep_range from interrupt context) changed it into udelay(10)
> >>> which is only a half delay of before, there're also some other delay
> changes.
> >>>
> >>> After only changed unfreeze delay back to udelay(20), the issue is gone.
> >>> So other timeout values are kept the same as 8badd65 changed.
> >>
> >> Can you change FLEXCAN_TIMEOUT_US instead?
> >
> >   Of course, we can change FLEXCAN_TIMEOUT_US to 100, but this will
> extend the time of enable/disable/softreset.
> > Which method do you think is better?
> 
> If you double to FLEXCAN_TIMEOUT_US to 100, the loops in question will spin
> at maximum the double time. But the loops are left as soon as the condition is
> satisfied.
> 
> It will fix your problem with the 10 kbit/s bitrate. But if there is some kind of
> problem with the IP core it will still fail, it just takes double amount of time
> (100 µs + overhead) until the function returns.
> 
> I don't see any harm in looping longer:
> - The previous good case is unchanged.
> - The error case takes double amount of time.
> - Your problem is hopefully fixed.

Thanks for your explanation, I will cook a patch then resend.

Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Re: [PATCH] can: flexcan: fix timeout when set small bitrate

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1250 bytes --]

On 1/31/19 10:37 AM, Joakim Zhang wrote:
> Current we can meet timeout issue when setting a small bitrate like 10000
> as follows on i.MX6UL EVK board (ipg clock = 66MHZ, per clock = 30MHZ):
> root@imx6ul7d:~# ip link set can0 up type can bitrate 10000
> A link change request failed with some changes committed already.
> Interface can0 may have been left with an inconsistent configuration, please check.
> RTNETLINK answers: Connection timed out
> 
> It is caused by calling of flexcan_chip_unfreeze() timeout.
> 
> Originally the code is using usleep_range(10, 20) for unfreeze operation,
> but the patch (8badd65 can: flexcan: avoid calling usleep_range from interrupt
> context) changed it into udelay(10) which is only a half delay of before,
> there're also some other delay changes.
> 
> After double to FLEXCAN_TIMEOUT_US to 100 can fix the issue.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Applied to linux-can

Tnx,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] can: flexcan: fix timeout when set small bitrate

[Joakim Zhang]

Kindly Ping...

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2019年1月31日 17:37
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Joakim
> Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH] can: flexcan: fix timeout when set small bitrate
> 
> Current we can meet timeout issue when setting a small bitrate like 10000 as
> follows on i.MX6UL EVK board (ipg clock = 66MHZ, per clock = 30MHZ):
> root@imx6ul7d:~# ip link set can0 up type can bitrate 10000 A link change
> request failed with some changes committed already.
> Interface can0 may have been left with an inconsistent configuration, please
> check.
> RTNETLINK answers: Connection timed out
> 
> It is caused by calling of flexcan_chip_unfreeze() timeout.
> 
> Originally the code is using usleep_range(10, 20) for unfreeze operation, but
> the patch (8badd65 can: flexcan: avoid calling usleep_range from interrupt
> context) changed it into udelay(10) which is only a half delay of before,
> there're also some other delay changes.
> 
> After double to FLEXCAN_TIMEOUT_US to 100 can fix the issue.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 2bca867bcfaa..1f2b4db7da88 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -166,7 +166,7 @@
>  #define FLEXCAN_MB_CNT_LENGTH(x)	(((x) & 0xf) << 16)
>  #define FLEXCAN_MB_CNT_TIMESTAMP(x)	((x) & 0xffff)
> 
> -#define FLEXCAN_TIMEOUT_US		(50)
> +#define FLEXCAN_TIMEOUT_US		(100)
> 
>  /* FLEXCAN hardware feature flags
>   *
> --
> 2.17.1

RE: [PATCH] can: flexcan: fix timeout when set small bitrate

[Aisheng Dong]

> -----Original Message-----
> From: Joakim Zhang
> Sent: Thursday, January 31, 2019 5:37 PM
> To: mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Joakim
> Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH] can: flexcan: fix timeout when set small bitrate
> 
> Current we can meet timeout issue when setting a small bitrate like 10000 as
> follows on i.MX6UL EVK board (ipg clock = 66MHZ, per clock = 30MHZ):
> root@imx6ul7d:~# ip link set can0 up type can bitrate 10000 A link change
> request failed with some changes committed already.
> Interface can0 may have been left with an inconsistent configuration, please
> check.
> RTNETLINK answers: Connection timed out
> 
> It is caused by calling of flexcan_chip_unfreeze() timeout.
> 
> Originally the code is using usleep_range(10, 20) for unfreeze operation, but
> the patch (8badd65 can: flexcan: avoid calling usleep_range from interrupt
> context) changed it into udelay(10) which is only a half delay of before,
> there're also some other delay changes.
> 
> After double to FLEXCAN_TIMEOUT_US to 100 can fix the issue.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Dong Aisheng

[PATCH] can: flexcan: fix timeout when set small bitrate

[Joakim Zhang]

Current we can meet timeout issue when setting a small bitrate like 10000
as follows on i.MX6UL EVK board (ipg clock = 66MHZ, per clock = 30MHZ):
root@imx6ul7d:~# ip link set can0 up type can bitrate 10000
A link change request failed with some changes committed already.
Interface can0 may have been left with an inconsistent configuration, please check.
RTNETLINK answers: Connection timed out

It is caused by calling of flexcan_chip_unfreeze() timeout.

Originally the code is using usleep_range(10, 20) for unfreeze operation,
but the patch (8badd65 can: flexcan: avoid calling usleep_range from interrupt
context) changed it into udelay(10) which is only a half delay of before,
there're also some other delay changes.

After double to FLEXCAN_TIMEOUT_US to 100 can fix the issue.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/flexcan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 2bca867bcfaa..1f2b4db7da88 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -166,7 +166,7 @@
 #define FLEXCAN_MB_CNT_LENGTH(x)	(((x) & 0xf) << 16)
 #define FLEXCAN_MB_CNT_TIMESTAMP(x)	((x) & 0xffff)
 
-#define FLEXCAN_TIMEOUT_US		(50)
+#define FLEXCAN_TIMEOUT_US		(100)
 
 /* FLEXCAN hardware feature flags
  *
-- 
2.17.1