| Message ID | 20190307150024.23628-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | can: flexcan: bump FLEXCAN_TIMEOUT_US to 250 | expand |
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 > * >
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
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
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
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
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
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
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 *
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(-)