Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6013 bytes --]

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@seznam.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

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

[...]

> > > +
> > > +	/* 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().

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

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().

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.

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.

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

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]