Message-Id: <202512270112.19801.pisa@fel.cvut.cz>
Date: Sat, 27 Dec 2025 01:12:19 +0100
From: Pavel Pisa <pisa@....cvut.cz>
To: Ondrej Ille <ondrej.ille@...il.com>
Cc: David Laight <david.laight.linux@...il.com>,
 "Marc Kleine-Budde" <mkl@...gutronix.de>,
 Andrea Daoud <andreadaoud6@...il.com>,
 linux-can@...r.kernel.org,
 Wolfgang Grandegger <wg@...ndegger.com>,
 "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 netdev@...r.kernel.org,
 Jan Altenberg <Jan.Altenberg@...dl.org>
Subject: Re: ctucanfd: possible coding error in ctucan_set_secondary_sample_point causing SSP not enabled

Dear Ondrej Ille,

On Friday 26 of December 2025 23:45:55 Ondrej Ille wrote:
> Hello everyone,
>
> As for this specific case, I am aware of it for longer time
>
> > but the last time when we met with Ondrej Ille this part
> > as been the last one on the table and the firm confirmation
> > what is the best value have not been stated.
>
> Pavel, the sample point should definitely be set to the variant of
> "measured + offset". That is
> what why the offset is calculated above the way it is. The aim is to place
> the sample point
> on CAN_RX "as-if the same as normal sample point", just with the TX->RX
> delay accounted for.
> One only needs to be careful that the value is not in Time Quantas, but in
> "number of cycles".
> But AFAICT should be accounted for by the ssp_offset calculation.
>
> As for leaving there comment, or leaving it up to the compiler to optimize
> away since the value is
> anyway initialized to zero, I don't know what is the preferred way in
> kernel.
> Personally, I would not leave any "meaningless code", so, I think comment
> is better.

I am not sure, I would probably prefer code there because it can be
easily modified to other value or make it configurable. So this
would look like

        if (dbt->bitrate > 1000000) {
                /* Calculate SSP in minimal time quanta */
                ssp_offset = (priv->can.clock.freq / 1000) * dbt->sample_point / dbt->bitrate;

                if (ssp_offset > 127) {
                        netdev_warn(ndev, "SSP offset saturated to 127\n");
                        ssp_offset = 127;
                }

                ssp_cfg = FIELD_PREP(REG_TRV_DELAY_SSP_OFFSET, ssp_offset);
                ssp_cfg |= FIELD_PREP(REG_TRV_DELAY_SSP_SRC, 0x0);
        }

It matches currents state of the driver in the IP CORE repository where
you have propagated change lat week

https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/commit/6188ca673f823873f7b37dbb588a2d4c0a0cc98c

But I would suggest to cover even else case for dbt->bitrate <= 1 Mbit/s by else
statement which should be probably 

        } else {
                ssp_cfg = FIELD_PREP(REG_TRV_DELAY_SSP_OFFSET, 0);
                ssp_cfg |= FIELD_PREP(REG_TRV_DELAY_SSP_SRC, 0x0);
        }

It is right that ctucan_set_secondary_sample_point() is called from ctucan_chip_start()
only and it is called from ctucan_do_set_mode() and ctucan_open() only and the chip
reset is done in both paths prior to ctucan_chip_start() but I think that
state should be defined there even for case that core is only stopped by
by REG_MODE_ENA and then re-enabled an no leftover from setup with higher
data bitrate to some followup change to lower one should be left there.

If there is already some defined way how to control/enable/disable
SSP from userspace for some of other drivers then I would
ted to have such option for CTU CAN FD as well to have
option to test/diagnose and correct (in the case of some problems)
its behavior without driver recompile.  

> 1) the driver is fixed on 4 Tx Buffers synthesis value
>
> > for CTU CAN FD IP core. I am not aware about any other value
> > in real use (FPGA or silicon) but if the core is synthesized
> > with other value then driver would fail.  If more are used,
> > then current driver code stuck on Tx empty infinite interrupt,
> > if less, messages would be lost.
> > The option to obtain the number of Tx buffers from hardware
> > has been added into design and we have proper code in RTEMS
> > driver
>
> Yes, this would be useful, I believe there is a tracking issue for that. I
> believe querying it
> at run-time is the simplest way to do it, but I don't know if it is "the
> right way" in kernel.

I think that it is OK, we have there already option to propagate
value from PCI and OF specific CTU CAN FD mapping into base code
but it is fixed on four buffers fr now and no mapping to OF is provided.
So I would change it such that ntxbufs = 0 specified by PCI or OF
mapping would result in configuration from info register.
If the requested ntxbufs from PCI or OF is higher than value reported
by info register then it should be limited as well and final limit
should be some define

#define CTUCANFD_NTXBUFS_MAX 8

We have that tested info register reading in RTEMS driver
for 2.5 as well as older branch so I hope that risk
of breaking someone's CTU CAN FD specific integration
is relatively small.

I prepare patches. The SSP one can be considered as bugfix
if Marc, David or somebody with the need proposes that
and may it be it can get to 6.19. Or I prepare both
for the next merge window.

Best wishes,

                Pavel

                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@....felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Powered by blists - more mailing lists