[PATCH] can: flexcan: bump FLEXCAN_TIMEOUT_US to 250

[PATCH] can: flexcan: bump FLEXCAN_TIMEOUT_US to 250

[Rasmus Villemoes]

While trying to add support for the Flexcan modules on the MPC8309,
I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
succeeds. Checking the leftover value of timeout with a primitive

  pr_err("%s: timeout==%d\n", __func__, timeout);

after the loop in chip_disable() typically shows values around 12-14,
i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
appear. So a timeout value of about twice that seems reasonable.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 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 75ce11395ee8..5f32053f78de 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -165,7 +165,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		(250)
 
 /* FLEXCAN hardware feature flags
  *
-- 
2.20.1

Re: [PATCH] can: flexcan: bump FLEXCAN_TIMEOUT_US to 250

[Rasmus Villemoes]

ping

On 07/03/2019 16.00, Rasmus Villemoes wrote:
> While trying to add support for the Flexcan modules on the MPC8309,
> I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
> succeeds. Checking the leftover value of timeout with a primitive
> 
>   pr_err("%s: timeout==%d\n", __func__, timeout);
> 
> after the loop in chip_disable() typically shows values around 12-14,
> i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
> appear. So a timeout value of about twice that seems reasonable.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  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 75ce11395ee8..5f32053f78de 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -165,7 +165,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		(250)
>  
>  /* FLEXCAN hardware feature flags
>   *
> 

Re: [PATCH] can: flexcan: bump FLEXCAN_TIMEOUT_US to 250

[Marc Kleine-Budde]

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

On 3/7/19 4:00 PM, Rasmus Villemoes wrote:
> While trying to add support for the Flexcan modules on the MPC8309,
> I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
> succeeds. Checking the leftover value of timeout with a primitive
> 
>   pr_err("%s: timeout==%d\n", __func__, timeout);
> 
> after the loop in chip_disable() typically shows values around 12-14,
> i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
> appear. So a timeout value of about twice that seems reasonable.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

I've scheduled the patch by Joakim Zhang:

    9daed89ae8a3 can: flexcan: fix timeout when set small bitrate

that doubles the timeout to 100.

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: bump FLEXCAN_TIMEOUT_US to 250

[Rasmus Villemoes]

On 20/03/2019 14.18, Marc Kleine-Budde wrote:
> On 3/7/19 4:00 PM, Rasmus Villemoes wrote:
>> While trying to add support for the Flexcan modules on the MPC8309,
>> I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
>> succeeds. Checking the leftover value of timeout with a primitive
>>
>>   pr_err("%s: timeout==%d\n", __func__, timeout);
>>
>> after the loop in chip_disable() typically shows values around 12-14,
>> i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
>> appear. So a timeout value of about twice that seems reasonable.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 
> I've scheduled the patch by Joakim Zhang:
> 
>     9daed89ae8a3 can: flexcan: fix timeout when set small bitrate
> 
> that doubles the timeout to 100.

Eh, ok, but that's not sufficient for the MPC8309 (I tried with 100 at
first, but as I write the minimally working timeout value turns out to
be about 140 us). Do you want me to send another patch on top of
9daed89ae8a3, or how should I interpret the above?

Thanks,
Rasmus

Re: [PATCH] can: flexcan: bump FLEXCAN_TIMEOUT_US to 250

[Marc Kleine-Budde]

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

On 3/21/19 3:34 PM, Rasmus Villemoes wrote:
> On 20/03/2019 14.18, Marc Kleine-Budde wrote:
>> On 3/7/19 4:00 PM, Rasmus Villemoes wrote:
>>> While trying to add support for the Flexcan modules on the MPC8309,
>>> I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
>>> succeeds. Checking the leftover value of timeout with a primitive
>>>
>>>   pr_err("%s: timeout==%d\n", __func__, timeout);
>>>
>>> after the loop in chip_disable() typically shows values around 12-14,
>>> i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
>>> appear. So a timeout value of about twice that seems reasonable.
>>>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>
>> I've scheduled the patch by Joakim Zhang:
>>
>>     9daed89ae8a3 can: flexcan: fix timeout when set small bitrate
>>
>> that doubles the timeout to 100.
> 
> Eh, ok, but that's not sufficient for the MPC8309 (I tried with 100 at
> first, but as I write the minimally working timeout value turns out to
> be about 140 us). Do you want me to send another patch on top of
> 9daed89ae8a3, or how should I interpret the above?

Can you propose an updated commit message for Joakim Zhang's patch?

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git/commit/?h=testing&id=9daed89ae8a3fc44ccd2b2bb9c3c4d3e3431904d

I'll add it by hand and increase the timeout to 250. Ok?

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: bump FLEXCAN_TIMEOUT_US to 250

[Rasmus Villemoes]

On 21/03/2019 16.30, Marc Kleine-Budde wrote:
> On 3/21/19 3:34 PM, Rasmus Villemoes wrote:
>> On 20/03/2019 14.18, Marc Kleine-Budde wrote:
>>> On 3/7/19 4:00 PM, Rasmus Villemoes wrote:
>>>> While trying to add support for the Flexcan modules on the MPC8309,
>>>> I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
>>>> succeeds. Checking the leftover value of timeout with a primitive
>>>>
>>>>   pr_err("%s: timeout==%d\n", __func__, timeout);
>>>>
>>>> after the loop in chip_disable() typically shows values around 12-14,
>>>> i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
>>>> appear. So a timeout value of about twice that seems reasonable.
>>>>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>
>>> I've scheduled the patch by Joakim Zhang:
>>>
>>>     9daed89ae8a3 can: flexcan: fix timeout when set small bitrate
>>>
>>> that doubles the timeout to 100.
>>
>> Eh, ok, but that's not sufficient for the MPC8309 (I tried with 100 at
>> first, but as I write the minimally working timeout value turns out to
>> be about 140 us). Do you want me to send another patch on top of
>> 9daed89ae8a3, or how should I interpret the above?
> 
> Can you propose an updated commit message for Joakim Zhang's patch?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git/commit/?h=testing&id=9daed89ae8a3fc44ccd2b2bb9c3c4d3e3431904d
> 
> I'll add it by hand and increase the timeout to 250. Ok?

Sounds fine. In order not to change Joakim's commit log too much, how
about adding something like (with your initials, since you're next in
the sign-off chain)

[mkl: Meanwhile, Rasmus Villemoes reported that even with a timeout of
100, flexcan_probe() fails on the MPC8309, which requires a value of at
least 140 to work reliably. 250 works for everyone.]

? Feel free to edit as you wish.

Thanks,
Rasmus

Re: [PATCH] can: flexcan: bump FLEXCAN_TIMEOUT_US to 250

[Marc Kleine-Budde]

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

On 3/22/19 8:45 AM, Rasmus Villemoes wrote:
> On 21/03/2019 16.30, Marc Kleine-Budde wrote:
>> On 3/21/19 3:34 PM, Rasmus Villemoes wrote:
>>> On 20/03/2019 14.18, Marc Kleine-Budde wrote:
>>>> On 3/7/19 4:00 PM, Rasmus Villemoes wrote:
>>>>> While trying to add support for the Flexcan modules on the MPC8309,
>>>>> I'm hitting ETIMEDOUT in flexcan_chip_disable(). With this, probing
>>>>> succeeds. Checking the leftover value of timeout with a primitive
>>>>>
>>>>>   pr_err("%s: timeout==%d\n", __func__, timeout);
>>>>>
>>>>> after the loop in chip_disable() typically shows values around 12-14,
>>>>> i.e. suggesting that it takes about 110-130 us for the LPM_ACK bit to
>>>>> appear. So a timeout value of about twice that seems reasonable.
>>>>>
>>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>>
>>>> I've scheduled the patch by Joakim Zhang:
>>>>
>>>>     9daed89ae8a3 can: flexcan: fix timeout when set small bitrate
>>>>
>>>> that doubles the timeout to 100.
>>>
>>> Eh, ok, but that's not sufficient for the MPC8309 (I tried with 100 at
>>> first, but as I write the minimally working timeout value turns out to
>>> be about 140 us). Do you want me to send another patch on top of
>>> 9daed89ae8a3, or how should I interpret the above?
>>
>> Can you propose an updated commit message for Joakim Zhang's patch?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git/commit/?h=testing&id=9daed89ae8a3fc44ccd2b2bb9c3c4d3e3431904d
>>
>> I'll add it by hand and increase the timeout to 250. Ok?
> 
> Sounds fine. In order not to change Joakim's commit log too much, how
> about adding something like (with your initials, since you're next in
> the sign-off chain)
> 
> [mkl: Meanwhile, Rasmus Villemoes reported that even with a timeout of
> 100, flexcan_probe() fails on the MPC8309, which requires a value of at
> least 140 to work reliably. 250 works for everyone.]
> 
> ? Feel free to edit as you wish.

Thanks, I've added that to the commit message itself.

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: bump FLEXCAN_TIMEOUT_US to 250

[Rasmus Villemoes]

On 22/03/2019 08.51, Marc Kleine-Budde wrote:
> On 3/22/19 8:45 AM, Rasmus Villemoes wrote:
>> On 21/03/2019 16.30, Marc Kleine-Budde wrote:
>>> On 3/21/19 3:34 PM, Rasmus Villemoes wrote:
>>>> On 20/03/2019 14.18, Marc Kleine-Budde wrote:
>>>
>>> Can you propose an updated commit message for Joakim Zhang's patch?
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git/commit/?h=testing&id=9daed89ae8a3fc44ccd2b2bb9c3c4d3e3431904d
>>>
>>> I'll add it by hand and increase the timeout to 250. Ok?
>>
>> Sounds fine. In order not to change Joakim's commit log too much, how
>> about adding something like (with your initials, since you're next in
>> the sign-off chain)
>>
>> [mkl: Meanwhile, Rasmus Villemoes reported that even with a timeout of
>> 100, flexcan_probe() fails on the MPC8309, which requires a value of at
>> least 140 to work reliably. 250 works for everyone.]
>>
>> ? Feel free to edit as you wish.
> 
> Thanks, I've added that to the commit message itself.

What's the status of this? I can't find anything in -next (as of
next-20190426), neither Joakim's original or the amended patch.

Thanks,
Rasmus