Re: [PATCH net-next v4 2/3] ptp: Add driver for R-Car Gen4

[Niklas Söderlund]

Hi Vadim,

Thanks for your feedback.

On 2026-07-02 15:39:25 +0100, Vadim Fedorenko wrote:
> On 02/07/2026 13:55, Niklas Söderlund wrote:
> > Add driver for the gPTP timer found on R-Car Gen4 devices. The timer is
> > system-wide and shared by different Ethernet devices on each Gen4
> > platform. The operation of the timer is however not completely in
> > depended of the systems Ethernet devices.
> > 
> >    - On R-Car S4 is gated by the RSWITCH Ethernet module clock.
> > 
> >    - On R-Car V4H is gated by the RTSN Ethernet module clock.
> > 
> >    - On R-Car V4M is gated by its own module clock, the system have
> >      neither RTSN or RSWITCH device. But the module clock is the same as
> >      RTSN on V4H and the documentation referees to it as tsn (EtherTSN).
> > 
> > The gPTP device do have its own register space on all three platforms.
> > But on S4 and V4H it will share its clock and reset property with
> > RSWITCH or RTSN, respectively.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > * Changes since v3
> > - Clamp increment calculated to register limitations.
> > - Check return value of clk_get_rate().
> > - Disable PM if ptp_clock_register() fails.
> > ---
> 
> [...]
> 
> > +struct ptp_rcar_gen4_priv {
> > +	void __iomem *base;
> > +	struct clk *clk;
> > +
> > +	struct ptp_clock *clock;
> > +	struct ptp_clock_info info;
> > +
> > +	spinlock_t lock;	/* Registers access. */
> > +	s64 default_addend;
> > +};
> > +
> > +#define ptp_to_priv(ptp) container_of(ptp, struct ptp_rcar_gen4_priv, info)
> > +
> > +static int ptp_rcar_gen4_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> > +{
> > +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> > +	s64 addend = priv->default_addend;
> > +	bool neg_adj = scaled_ppm < 0;
> > +	unsigned long flags;
> > +	s64 diff;
> > +
> > +	if (neg_adj)
> > +		scaled_ppm = -scaled_ppm;
> > +	diff = div_s64(addend * scaled_ppm_to_ppb(scaled_ppm), NSEC_PER_SEC);
> > +	addend = neg_adj ? addend - diff : addend + diff;
> > +
> > +	/* Clamp value to register limits, defined as in nanoseconds.
> > +	 * bit[31:27] - integer
> > +	 * bit[26:0]  - decimal
> > +	 */
> > +	addend = clamp_val(addend, 0, UINT_MAX);
> 
> is it always positive number?

Yes.

The value written to the PTPTIVC0_REG register is the timer increment in 
ns per clock pulse. The PTP is clocked by different rates on different 
SoC. 200Mhz on V4H and 320Mhz on S4. On each pulse the PTP timer is 
incremented by this value. For example,

If clock frequency is 50 MHz, 100Mhz, 200Mhz, 320MHz or 400MHz,
it should be respectively set to a value around 0xA0000000, 0x50000000, 
0x28000000, 0x19000000 or 0x14000000.

> 
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	iowrite32(addend, priv->base + PTPTIVC0_REG);
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> [...]
> 
> > +static struct ptp_clock_info ptp_rcar_gen4_info = {
> > +	.owner = THIS_MODULE,
> > +	.name = "R-Car Gen4 gPTP",
> > +	.max_adj = 50000000,
> 
> even though clamping addend may work, I would suggest adjusting
> ".max_adj" value to the one which will not make addend overflow.
> And as a reminder, .max_adj is the absolute value in ppb that can be set
> for a single call of .adjfine - the value is checked against
> [-(.max_adj),.max_adj] range.

Thanks for the reminder, but is this not then correct and prevents any 
overflow? From the calculations above.

(a)     s64 addend = priv->default_addend;
	...
(b)     if (neg_adj)
(b)                scaled_ppm = -scaled_ppm;
(c)     diff = div_s64(addend * scaled_ppm_to_ppb(scaled_ppm), NSEC_PER_SEC);
(d)     addend = neg_adj ? addend - diff : addend + diff;

Section (a) copies the default addend value. This value is calculated at 
probe time as a function of the PTP module clock (50 MHz, 100Mhz, 
200Mhz, 320MHz or 400MHz) to a known value. The max value being 
0xA0000000 for a 50Mhz module clock.

Sections (b) and (d) deals with the value we add at each clock tick is 
in fact based on the PTP module clock itself.

So the one place we could overflow is in section (c). Lets look at the 
worse case scenario.

addend = priv->default_addend = <from PTP clock worse case 50 MHz> = 0xA0000000
scaled_ppm_to_ppb(scaled_ppm) = .max_adj = 50000000

    addend * scaled_ppm_to_ppb(scaled_ppm)
        = 0xA0000000 * 50000000
            = 0x1DCD65000000000

That is OK and fits inside a s64.

    div_s64(0x1DCD65000000000, NSEC_PER_SEC)
        = div_s64(0x1DCD65000000000, 1000000000L)
            = 0x8000000

That is OK and does not overflow.

But I agree it's not very clear and depends on knowing from the 
datasheet that the possible PTP clock rates are.

How can I best move forward here? Drop the clamp added and depend on the 
.max_adj value to prevent the overflow with the implicit knowledge of 
the clock rates? I can extend the comment in the probe function of how 
priv->default_addend is computed, but for somebody looking at the 
calculation that might not help much.

> 
> > +	.adjfine = ptp_rcar_gen4_adjfine,
> > +	.adjtime = ptp_rcar_gen4_adjtime,
> > +	.gettime64 = ptp_rcar_gen4_gettime,
> > +	.settime64 = ptp_rcar_gen4_settime,
> > +};

-- 
Kind Regards,
Niklas Söderlund