Re: [PATCH 1/2] can: Add Loongson CAN-FD controller support

[Vincent Mailhol <mailhol@kernel.org>]

On 04/06/2026 at 07:52, Binbin Zhou wrote:
> Hi Vincent:
> 
> Sorry for the late reply.
> 
> On Tue, May 26, 2026 at 11:58 PM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> On 21/05/2026 at 05:24, Binbin Zhou wrote:
>>> Hi Vincent:
>>>
>>> On Thu, May 7, 2026 at 1:50 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> (...)
>>
>>>> Use the CAN TDC framework to get the SSP value (c.f. struct can_tdc,
>>>> struct can_tdc_const and can_fd_tdc_is_enabled())
>>>
>>> Last question:
>>>
>>> In the general framework, the calculation condition for tdco is
>>> (dbt->brp == 1 || dbt->brp == 2), which does not seem to correspond to
>>> the current condition (dbt->bitrate > 1000000).
>>
>> Where does the current condition comes from? Is it some empirical test?

You did not answer this question. Where does this idea come from? Is it
an individual initiative from one of your hardware engineers or does it
come from any literature or research paper?

>> FYI, the
>>
>>   (dbt->brp == 1 || dbt->brp == 2)
>>
>> check comes from ISO 11898.

My feeling is that you were not aware of the BRP <= 2 criteria. That
criteria comes from ISO 11898-1. Was your hardware team aware of this
BRP <= 2 ISO criteria? If yes, what made them decide to not use it?

>>> Although for CANFD, rates below 1 Mbit/s would negate the primary
>>> advantages of FD, from a controller hardware design perspective, I
>>> would still prefer to retain the condition (dbt->bitrate > 1000000).
>>>
>>> Do you have any other suggestions?
>>
>> Using TDC on low bitrates is indeed unstable and can lead to bus errors.
>> But the condition which you suggest
>>
>>   (dbt->bitrate > 1000000)
>>
>> is indirectly linked with the brp values. That's why the brp check is
>> sufficient (and more precise than the empirical 1 Mbit/s threshold value).
>>
>> Are you able to trigger any bus errors when using TDC with a brp value
>> of 1 or 2 and a bitrate below 1 Mbit/sec? If no, there is no reason to
>> add this extra check and we can continue to rely on the framework test.

You also didn't answer this second question:

  Are you able to trigger any bus errors when using TDC with a brp value
  of 1 or 2 and a bitrate below 1 Mbit/sec?

> After offline discussions with our hardware engineers, we have decided
> to retain the existing decision criteria.
> 
> The threshold is set to `1 Mbit/s` primarily based on the CAN 2.0
> physical layer protocol, which supports a maximum rate of 1 Mbit/s;
> this decision also takes into account that at low baud rates, the bit
> duration is sufficiently long, so there is generally no need to enable
> the second sampling point. Conversely, if the CAN controller’s time
> quanta clock frequency is very high, causing the BRP to consistently
> exceed 2 while traversing parameters, TDC may not be enabled.
>> Additionally, there is a low probability of bus errors occurring
during testing.

Can you prove that the 1Mbit/sec threshold gives a *lower* probability
of bus errors occurring compared to the BRP <= 2 rule?

The point is that to be accepted, it is insufficient for your new
criteria to just match the current criteria. It has to *improve* the
current situation. Otherwise, more code for same performance means that
we just bloated the kernel with a useless check.

> So, in your opinion, should we keep it as is?

An important aspect for the kernel maintainability is to centralize code
as much as possible. If you can prove that your 1Mbit/sec threshold is
*better* than the current rule, then this check must go in the framework
(e.g. modify can_fd_tdc_is_enabled()) so that all the drivers can
benefit from it. If not, it has no reason to exist.

As much as possible, we should avoid driver individual initiatives, so
keeping this condition in your driver is a hard no for me (one exception
for local workaround would only be if your device has a defect which
need some unique logic for fixing).

So, either convince me through some tangible proof that your criteria
improves the ISO criteria (e.g. empirical test showing bus error
reduction when adding your rule) or drop the idea.


Yours sincerely,
Vincent Mailhol