drivers: can: flexcan: fix timing parameter limits #39542
Conversation
henrikbrixandersen
added
bug
|
This bug is present in both v2.5-branch, v2.6-branch, and v2.7-branch (along with main). Should we backport to all of these? |
henrikbrixandersen
force-pushed
the
mcux_flexcan_pseg2
branch
from
0e72e35 to
9efde24
Compare
|
2.7 for sure since it is LTS. I think 2.5/2.6 would only get security fixes and really only 2.6 given our support contract. |
drivers/can/can_mcux_flexcan.c
Outdated
There was a problem hiding this comment.
Love magic numbers ... should we comment here where these numbers come from? Reference manual?
There was a problem hiding this comment.
Pffft ;-)
Good point. I have added a somewhat lengthy comment trying to explain the origin of the limits.
|
If I follow the explanations in the linked issue, we are still calculating with physical values in If my understanding is correct, I agree with the fix. |
Fix the limits for the timing parameter calculations. The lower limit for the phase_seg2 value is wrongly specified as 1 to 7, but 1U is substracted before writing it to the CTRL1:PSEG2 register field. This results in register field values between 0 and 6, but 0 is an invalid value for the PSEG2 register field. The upper limits for several of the timing parameters are wrong as well, but this does not result in invalid register field values being calculated. It can, however, result in not being able to meet CAN timing requirements. The confusion in specifying the limits likely stems from the timing calculations and timing limits using the "physical" values, whereas the registers fields all use the "physical" value minus 1. When the datasheet says "The valid programmable values are 1-7", the corresponding limits should be set to 2 to 8 to take the "minus 1" into account. Fixes: zephyrproject-rtos#39541 Signed-off-by: Henrik Brix Andersen <[email protected]>
henrikbrixandersen
force-pushed
the
mcux_flexcan_pseg2
branch
from
9efde24 to
ad7e288
Compare
That is mostly correct, but even if the existing timing limits where based on register values, the lower limit for PSEG2 is wrong. The initial commit (8b6c1bd) introduced the limits and used them without the "minus 1U" before writing it to the registers, but this was fixed in a later commit (d7a5b9f). I guess the second commit should have updated the limits as well. The lower limit for PSEG2 has been wrong all along. |
5 participants
Fix the limits for the timing parameter calculations.
The lower limit for the phase_seg2 value is wrongly specified as 1 to 7, but 1U is substracted before writing it to the CTRL1:PSEG2 register bits, resulting in register values between 0 and 6, but 0 is an invalid value for the PSEG2 register bits.
The upper limits for several of the timing parameters are wrong as well, but this does not result in invalid register values being calculated. It can, however, result in not being able to meet CAN timing requirements.
The confusion in specifying the limits likely stems from the timing calculations and timing limits using the "real" values, whereas the registers all use the "real" value minus 1. When the datasheet says "The valid programmable values are 1–7", the corresponding limits should be set to 2 to 8 to take the "minus 1" into account.
Fixes: #39541
Signed-off-by: Henrik Brix Andersen [email protected]