Message-Id: <202208041008.15122.pisa@cmp.felk.cvut.cz>
Date:   Thu, 4 Aug 2022 10:08:15 +0200
From:   Pavel Pisa <pisa@....felk.cvut.cz>
To:     "Marc Kleine-Budde" <mkl@...gutronix.de>,
        Matej Vasilevski <matej.vasilevski@...nam.cz>
Cc:     Ondrej Ille <ondrej.ille@...il.com>,
        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>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames

Hello Marc,

On Wednesday 03 of August 2022 10:37:18 Marc Kleine-Budde wrote:
> On 02.08.2022 18:20:17, Pavel Pisa wrote:
> > Hello Marc,
> >
> > thanks for feedback.
> >
> > On Tuesday 02 of August 2022 11:29:07 Marc Kleine-Budde wrote:
> > > On 01.08.2022 20:46:54, Matej Vasilevski wrote:
> > > > This patch adds support for retrieving hardware timestamps to RX and
> > > > error CAN frames. It uses timecounter and cyclecounter structures,
> > > > because the timestamping counter width depends on the IP core
> > > > integration (it might not always be 64-bit).
> > > > For platform devices, you should specify "ts_clk" clock in device
> > > > tree. For PCI devices, the timestamping frequency is assumed to be
> > > > the same as bus frequency.
> > > >
> > > > Signed-off-by: Matej Vasilevski <matej.vasilevski@...nam.cz>
> > > > ---
> > > >  drivers/net/can/ctucanfd/Makefile             |   2 +-
> > > >  drivers/net/can/ctucanfd/ctucanfd.h           |  20 ++
> > > >  drivers/net/can/ctucanfd/ctucanfd_base.c      | 214
> > > > +++++++++++++++++- drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 
> > > > 87 +++++++ 4 files changed, 315 insertions(+), 8 deletions(-)
> > > >  create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> >
> > ...
> >
> > > > +	if (ts_high2 != ts_high)
> > > > +		ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> > > > +
> > > > +	return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> > > > +}
> > > > +
> > > >  #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF,
> > > > ctucan_read32(priv, CTUCANFD_STATUS))) #define
> > > > CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA,
> > > > ctucan_read32(priv, CTUCANFD_MODE)))
> > >
> > > please make these static inline bool functions.
> >
> > We put that to TODO list. But I prefer to prepare separate followup
> > patch later.
>
> ACK. I noticed later that these were not modified by this patch. Sorry
> for the noise

OK

> > > > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > > >  		return 0;
> > > >  	}
> > > >
> > > > -	ctucan_read_rx_frame(priv, cf, ffw);
> > > > +	ctucan_read_rx_frame(priv, cf, ffw, &timestamp);
> > > > +	if (priv->timestamp_enabled)
> > > > +		ctucan_skb_set_timestamp(priv, skb, timestamp);
> > >
> > > Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> > > happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> > > right?
> >
> > I am not sure about which possible problem do you think.
> > But ctucan_read_timestamp_counter() is fully reentrant
> > and has no side effect on the core. So there is no
> > problem.
>
> ctucan_read_timestamp_counter() is reentrant, but on 32 bit systems the
> update of tc->cycle_last isn't.
>
> [...]

Good catch, so we probably should use atomic there or we need to add
spinlock, but I think that atomic is optimal solution there.

> > > > +
> > > > +	/* Obtain timestamping frequency */
> > > > +	if (pm_enable_call) {
> > > > +		/* Plaftorm device: get tstamp clock from device tree */
> > > > +		priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> > > > +		if (IS_ERR(priv->timestamp_clk)) {
> > > > +			/* Take the core clock frequency instead */
> > > > +			timestamp_freq = can_clk_rate;
> > > > +		} else {
> > > > +			timestamp_freq = clk_get_rate(priv->timestamp_clk);
> > > > +		}
> > >
> > > Who prepares/enabled the timestamp clock? clk_get_rate() is only valid
> > > if the clock is enabled. I know, we violate this for the CAN clock. :/
> >
> > Yes, I have noticed that we miss clk_prepare_enable() in the
> > ctucan_probe_common() and clk_disable_unprepare() in
> > ctucan_platform_remove().
>
> Oh, I missed the fact that the CAN clock is not enabled at all. That
> should be fixed, too, in a separate patch.
>
> So let's focus on the ts_clk here. On DT systems if there's no ts-clk,
> you can assign the normal clk pointer to the priv->timestamp_clk, too.
> Move the calculation of mult, shift and the delays into
> ctucan_timestamp_init(). If ctucan_timestamp_init is not NULL, add a
> clk_prepare_enable() and clk_get_rate(), otherwise use the can_clk_rate.
> Add the corresponding clk_disable_unprepare() to ctucan_timestamp_stop().

OK

> > The need for clock running should be released in ctucan_suspend()
> > and regained in ctucan_resume(). I see that the most CAN drivers
> > use there clk_disable_unprepare/clk_prepare_enable but I am not
> > sure, if this is right. Ma be plain clk_disable/clk_enable should
> > be used for suspend and resume because as I understand, the clock
> > frequency can be recomputed and reset during clk_prepare which
> > would require to recompute bitrate. Do you have some advice
> > what is a right option there?
>
> For the CAN clock, add a prepare_enable to ndo_open, corresponding
> function to ndo_stop. Or better, add these time runtime_pm.

Hmm, there is problem that we have single clock for whole design,
so if we try to touch AXI/APB slave registers without clock setup
then system blocks. So I think that we need to prepare and enable
clocks in probe to allow registers access later. We need it at least
for core bus endian probe and version validation/quirks. May it be
we can disable clocks and reenable them in open.... But it is
a little risky play and needs to ensure that no other path
in the closed driver can attempt to access registers.

Due to use of AXI bridges and other peripherals in Zynq Programmable
Logic (FPGA) we keep forcibly clock enabled. In the fact, this
should be solved some way on level of APB (now renamed in Zynq DST
to AXI) bus mapping. 

> Has system suspend/resume been tested? I think the IP core might be
> powered off during system suspend, so the driver has to restore the
> state of the chip. The easiest would be to run through
> chip_start()/chip_stop().

Hmm, if we do not reconfigure FPGA then it keeps state and if we
lose configuration then whole cycle with DTS overlay is required.
So basically for runtime power down wee need to unload overlay
which removes driver instances and then reload  overlay and instances
again... If you speak about suspend to disk, then I do not expect
to run this way on any platform bus based system in near future.
With PCIe card on PC it is possible... So it is other area to invest
work in future...

> For the possible change of clock rate between probe and ifup, we should
> add a CAN driver framework wide function to re-calculate the bitrates
> with the current clock rate after the prepare_enable.

ACK

> BTW: In an early version of the stm32mp1 device tree some graphics clock
> and the CAN clock shared the same parent clock. The configuration of the
> display (which happened after the probe of the CAN driver ) caused a
> different rate in the CAN clock, resulting in broken bit timings.

So in the fact each CAN device should listen to the clock rate
change notifier...

> > Actual omission is no problem on our systems, be the clock are used
> > in whole FPGA system with AXI connection and has to running already
> > and we use same for timestamping.
> >
> > I would prefer to allow timestamping patch as it is without clock enable
> > and then correct clock enable, disable by another patch for both ts and
> > core clocks.
>
> NACK - if the time stamping clock is added, please with proper handling.
> The core clock can be fixed in a later patch.

OK, how is it with your timing plans. I have already stated to Matej 
Vasilevski that slip of the patch sending after 5.19 release means
that we would not fit in 5.20 probably. If it is so and you, then I
expect that postpone of discussions, replies and thoughts about our
work after 5.20 preparation calms down is preferred on your side.
We focus on preparation of proper series for early 5.21/6.0 cycle.

Thanks for your time

                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
    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