From: Dong Aisheng <[email protected]>
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 <[email protected]>
Signed-off-by: Joakim Zhang <[email protected]>
---
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, ®s->mcr);
while (timeout-- && (priv->read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
- udelay(10);
+ udelay(20);
if (priv->read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK)
return -ETIMEDOUT;
--
2.17.1
On 1/31/19 7:58 AM, Joakim Zhang wrote:
> From: Dong Aisheng <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Joakim Zhang <[email protected]>
> ---
> 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, ®s->mcr);
>
> while (timeout-- && (priv->read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
> - udelay(10);
> + udelay(20);
>
> if (priv->read(®s->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 |
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 2019年1月31日 15:34
> To: Joakim Zhang <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; Aisheng
> Dong <[email protected]>
> Subject: Re: [PATCH] can: flexcan: fix timeout when set small bitrate
>
> On 1/31/19 7:58 AM, Joakim Zhang wrote:
> > From: Dong Aisheng <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Joakim Zhang <[email protected]>
> > ---
> > 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, ®s->mcr);
> >
> > while (timeout-- && (priv->read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
> > - udelay(10);
> > + udelay(20);
> >
> > if (priv->read(®s->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 |
> 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 <[email protected]>
> > > Signed-off-by: Joakim Zhang <[email protected]>
> > > ---
> > > 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, ®s->mcr);
> > >
> > > while (timeout-- && (priv->read(®s->mcr) &
> FLEXCAN_MCR_FRZ_ACK))
> > > - udelay(10);
> > > + udelay(20);
> > >
> > > if (priv->read(®s->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 |
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 |
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde <[email protected]>
> Sent: 2019年1月31日 17:12
> To: Joakim Zhang <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>; Aisheng
> Dong <[email protected]>
> 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 |