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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new standalone driver drives the same gPTP register block
  that drivers/net/ethernet/renesas/rcar_gen4_ptp.c already drives on…
- [Medium] ptp_rcar_gen4_probe() ignores the return value of
  pm_runtime_resume_and_get(dev) at line 179.
- [Medium] ptp: torn hardware state exposed during
  _ptp_rcar_gen4_settime()
- [Medium] ptp: potential time tearing in gettime due to unlatched
  sequential reads
- [Low] The binding (renesas,rcar-gen4-gptp.yaml) lists `resets` as
  required, and the V4H DTS node sets resets = <&cpg 2723>.
- [Low] The commit message for "ptp: Add driver for R-Car Gen4" has
  several typos and grammar errors in the only explanation of the…
- [Low] The PTPTIVC0 comment in ptp_rcar_gen4_probe() (lines 170-174) has
  'perid' for 'period' and calls bits[26:0] 'decimal' when they are a…
- [Low] In ptp_rcar_gen4_probe(), the timer is enabled (iowrite32(1,
  PTPTMEC_REG), line 182) before the fallible ptp_clock_register() call.

--

ptp: Add driver for R-Car Gen4

This commit adds a standalone platform driver for the gPTP timer block
found on R-Car Gen4 SoCs (S4, V4H and V4M). The driver maps the timer
registers, programs the default increment from the module clock rate, and
registers a PTP clock with adjfine, adjtime, gettime64 and settime64 ops.

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

[Severity: Low]
This isn't a bug, but there are a few typos and grammar errors in the
commit message:

  "not completely in depended of" -> "not completely independent of"
  "On R-Car S4 is gated" -> "On R-Car S4 it is gated"
  "On R-Car V4H is gated" -> "On R-Car V4H it is gated"
  "the system have" -> "the system has"
  "the documentation referees to it" -> "the documentation refers to it"
  "The gPTP device do have" -> "The gPTP device does have"

> diff --git a/drivers/ptp/ptp_rcar_gen4.c b/drivers/ptp/ptp_rcar_gen4.c
> new file mode 100644
> index 0000000000000..e82b6c5abc306
> --- /dev/null
> +++ b/drivers/ptp/ptp_rcar_gen4.c
> @@ -0,0 +1,225 @@

[ ... ]

> +static int ptp_rcar_gen4_probe(struct platform_device *pdev)
> +{
> +	struct ptp_rcar_gen4_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	unsigned long rate;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);

[Severity: Medium]
Is this the same register block that
drivers/net/ethernet/renesas/rcar_gen4_ptp.c already drives for RTSN (V4H)
and RSWITCH (S4)? The register offsets, the adjfine, gettime, settime and
adjtime bodies, the addend formula and max_adj all look like a copy of that
code. The commit message doesn't mention the existing implementation or
explain how the two are meant to coexist.

On V4H, the gptp node added in this series and the existing tsn0 node in
r8a779g0.dtsi describe the same range:

    gptp: ptp@e6449000 { ... reg = <0 0xe6449000 0 0x500>; ... }
    tsn0: ethernet@e6460000 {
        reg = <0 0xe6460000 0 0x7000>, <0 0xe6449000 0 0x500>;
        reg-names = "tsnes", "gptp";
        ...
    }

rtsn_probe() requests the "gptp" region with devm_ioremap_resource() and
registers its own PHC through rcar_gen4_ptp_register(). Here,
ptp_rcar_gen4_probe() requests the same region again through
devm_platform_ioremap_resource().

If a board enables both &gptp (so RAVB can use it through "ptp-timer") and
&tsn0, does whichever device probes second fail with -EBUSY? That would
lose either the RTSN network interface or the standalone PHC.

The DTS commit in this series says the module "is shared between the RAVB
and RTSN Ethernet devices", but rtsn and rswitch are left unchanged. Is
there a plan to move them over to this driver?

> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);

[Severity: Low]
The renesas,rcar-gen4-gptp.yaml binding lists resets as required, and the
V4H node uses resets = <&cpg 2723>. Should probe also get and deassert
the reset? ptp_rcar_gen4_probe() gets only the register range and the
clock, and ptp_rcar_gen4_remove() never asserts a reset.

Suppose the module reset is still asserted at boot, for example on V4M
where the gPTP block has its own reset. Would the PTPTIVC0 and PTPTMEC
writes later in probe have any effect? A PHC would still be registered.

The commit message also says the reset is shared with RTSN on V4H, and
rtsn_reset() pulses it with reset_control_reset(). Without a reset handle,
how would this driver coordinate with that, or reprogram the timer after
the pulse?

[ ... ]

> +	/* Default timer increment in ns.
> +	 * bit[31:27] - integer
> +	 * bit[26:0]  - decimal
> +	 * increment[ns] = perid[ns] * 2^27 => (1ns * 2^27) / rate[hz]
> +	 */
> +
> +	priv->default_addend = div_s64(1000000000LL << 27, rate);

[Severity: Low]
This isn't a bug, but the comment has a typo (perid -> period). Also,
bits [26:0] are a binary fraction rather than decimal.

The formula in the comment doesn't match the code either. The code
computes period[ns] * 2^27 = (1e9 * 2^27) / rate[Hz], not
(1ns * 2^27) / rate[hz].

The comment looks like it was copied from
rcar_gen4_ptp_rate_to_increment() in
drivers/net/ethernet/renesas/rcar_gen4_ptp.c.

> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_resume_and_get(dev);

[Severity: Medium]
Should the return value of pm_runtime_resume_and_get() be checked here?
The v6 changelog says pm_runtime_get_sync() was replaced so that failures
could be handled, but the result is still ignored.

When pm_runtime_resume_and_get() fails, it drops the usage count itself
and returns the error. The device is not resumed and no reference is held.
Probe then goes on to do this:

    iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);
    iowrite32(1, priv->base + PTPTMEC_REG);

The driver never calls clk_prepare_enable(). The module clock is only
turned on by the runtime PM resume, so these writes may go to an unclocked
block.

ptp_clock_register() can still succeed after that. Userspace would then
get a PHC whose gettime, settime and adjtime access unclocked MMIO.

The later puts would also be unbalanced: the pm_runtime_put_sync() in the
ptp_clock_register() error path below, and the one in
ptp_rcar_gen4_remove():

    iowrite32(1, priv->base + PTPTMDC_REG);

    pm_runtime_put_sync(dev);
    pm_runtime_disable(dev);

Wouldn't rpm_drop_usage_count() then warn "Runtime PM usage count
underflow!"?

> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);
> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +
> +	priv->clock = ptp_clock_register(&priv->info, dev);
> +	if (IS_ERR(priv->clock)) {
> +		pm_runtime_put_sync(dev);
> +		pm_runtime_disable(dev);
> +		return PTR_ERR(priv->clock);
> +	}

[Severity: Medium]
_ptp_rcar_gen4_settime() first stops the timer and clears the three
PTPTOVC offset registers. It then writes PTPTMEC to start the timer
again, and only after that programs the new offset. Between the
PTPTMEC write and the final PTPTOVC00 write, the counter runs with a
zero or partially written offset.

This timer is the time base shared by the Ethernet MACs on these SoCs
(RSWITCH, RTSN, and RAVB through ptp-timer). Could a packet that is
timestamped inside that window get a time near zero? Could it get the
new upper seconds word combined with a stale lower word?

The same window opens on every ptp_rcar_gen4_adjtime() call, since it
goes through this helper. So this is not limited to an initial
settime.

Is there a reason the timer has to be enabled before the offset is
loaded? If the hardware allows it, writing PTPTOVC20, PTPTOVC10 and
PTPTOVC00 while the timer is still stopped, and only then writing
PTPTMEC, would close the window.

If the hardware instead applies the offset atomically on the PTPTOVC00
write, could a comment say so? In that case, is the intermediate
zeroing of the offset registers needed at all?

[Severity: Low]
The PTPTMEC write above starts the timer before ptp_clock_register(),
which can fail. Should this error path also stop the timer, the way
ptp_rcar_gen4_remove() does before it drops runtime PM?

    iowrite32(1, priv->base + PTPTMDC_REG);

On S4 and V4H the gating clock belongs to RSWITCH or RTSN and may stay
on. Could the counter then keep running with no driver bound?

> +
> +	return 0;
> +}

[ ... ]

> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Renesas R-Car Gen4 gPTP device driver
> + *
> + * Copyright (C) 2026 Renesas Electronics Corporation
> + * Copyright (C) 2026 Niklas Söderlund <niklas.soderlund@ragnatech.se>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/types.h>
> +
> +#define PTPTMEC_REG		0x0010
> +#define PTPTMDC_REG		0x0014
> +#define PTPTIVC0_REG		0x0020
> +#define PTPTOVC00_REG		0x0030
> +#define PTPTOVC10_REG		0x0034
> +#define PTPTOVC20_REG		0x0038
> +#define PTPGPTPTM00_REG		0x0050
> +#define PTPGPTPTM10_REG		0x0054
> +#define PTPGPTPTM20_REG		0x0058
> +
> +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;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	iowrite32(addend, priv->base + PTPTIVC0_REG);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void _ptp_rcar_gen4_gettime(struct ptp_clock_info *ptp,
> +				   struct timespec64 *ts)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	ts->tv_nsec = ioread32(priv->base + PTPGPTPTM00_REG);
> +	ts->tv_sec = ioread32(priv->base + PTPGPTPTM10_REG) |
> +		((s64)ioread32(priv->base + PTPGPTPTM20_REG) << 32);
> +}
> +
> +static int ptp_rcar_gen4_gettime(struct ptp_clock_info *ptp,
> +				 struct timespec64 *ts)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	_ptp_rcar_gen4_gettime(ptp, ts);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void _ptp_rcar_gen4_settime(struct ptp_clock_info *ptp,
> +				   const struct timespec64 *ts)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	iowrite32(1, priv->base + PTPTMDC_REG);
> +	iowrite32(0, priv->base + PTPTOVC20_REG);
> +	iowrite32(0, priv->base + PTPTOVC10_REG);
> +	iowrite32(0, priv->base + PTPTOVC00_REG);
> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +	iowrite32(ts->tv_sec >> 32, priv->base + PTPTOVC20_REG);
> +	iowrite32(ts->tv_sec, priv->base + PTPTOVC10_REG);
> +	iowrite32(ts->tv_nsec, priv->base + PTPTOVC00_REG);
> +}
> +
> +static int ptp_rcar_gen4_settime(struct ptp_clock_info *ptp,
> +				 const struct timespec64 *ts)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	_ptp_rcar_gen4_settime(ptp, ts);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int ptp_rcar_gen4_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct ptp_rcar_gen4_priv *priv = ptp_to_priv(ptp);
> +	struct timespec64 ts;
> +	unsigned long flags;
> +	s64 now;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	_ptp_rcar_gen4_gettime(ptp, &ts);
> +	now = ktime_to_ns(timespec64_to_ktime(ts));
> +	ts = ns_to_timespec64(now + delta);
> +	_ptp_rcar_gen4_settime(ptp, &ts);
> +	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,
> +	.adjfine = ptp_rcar_gen4_adjfine,
> +	.adjtime = ptp_rcar_gen4_adjtime,
> +	.gettime64 = ptp_rcar_gen4_gettime,
> +	.settime64 = ptp_rcar_gen4_settime,
> +};
> +
> +static int ptp_rcar_gen4_probe(struct platform_device *pdev)
> +{
> +	struct ptp_rcar_gen4_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	unsigned long rate;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	rate = clk_get_rate(priv->clk);
> +	if (!rate)
> +		return -ENODEV;
> +
> +	spin_lock_init(&priv->lock);
> +
> +	priv->info = ptp_rcar_gen4_info;
> +
> +	/* Default timer increment in ns.
> +	 * bit[31:27] - integer
> +	 * bit[26:0]  - decimal
> +	 * increment[ns] = perid[ns] * 2^27 => (1ns * 2^27) / rate[hz]
> +	 */
> +
> +	priv->default_addend = div_s64(1000000000LL << 27, rate);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_resume_and_get(dev);
> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);
> +	iowrite32(1, priv->base + PTPTMEC_REG);
> +
> +	priv->clock = ptp_clock_register(&priv->info, dev);
> +	if (IS_ERR(priv->clock)) {
> +		pm_runtime_put_sync(dev);
> +		pm_runtime_disable(dev);
> +		return PTR_ERR(priv->clock);
> +	}
> +
> +	return 0;
> +}
> +
> +static void ptp_rcar_gen4_remove(struct platform_device *pdev)
> +{
> +	struct ptp_rcar_gen4_priv *priv = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	ptp_clock_unregister(priv->clock);
> +
> +	iowrite32(1, priv->base + PTPTMDC_REG);
> +
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +}
> +
> +static const struct of_device_id ptp_rcar_gen4_of_match[] = {
> +	{ .compatible = "renesas,rcar-gen4-gptp", },
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ptp_rcar_gen4_of_match);
> +
> +static struct platform_driver ptp_rcar_gen4_driver = {
> +	.driver = {
> +		.name = "ptp-rcar-gen4",
> +		.of_match_table = ptp_rcar_gen4_of_match,
> +	},
> +	.probe    = ptp_rcar_gen4_probe,
> +	.remove   = ptp_rcar_gen4_remove,
> +};
> +module_platform_driver(ptp_rcar_gen4_driver);
> +
> +MODULE_AUTHOR("Niklas Söderlund");
> +MODULE_DESCRIPTION("Renesas R-Car Gen4 gPTP driver");
> +MODULE_LICENSE("GPL");

[Severity: Medium]
_ptp_rcar_gen4_gettime() reads PTPGPTPTM00 (nanoseconds) and then
PTPGPTPTM10 and PTPGPTPTM20 (seconds). These are three separate MMIO
reads with no capture command and no retry. Does the Gen4 gPTP block
latch the seconds registers when PTPGPTPTM00 is read?

If it does not, a second rollover between the first and second read
pairs a nanosecond value from just before the boundary with the
incremented seconds. The returned time then jumps almost a full
second ahead. The Gen3 ravb code issues GCCR_TCR_CAPTURE before
reading the time registers for this reason.

This matters more in ptp_rcar_gen4_adjtime(). It takes the result of
_ptp_rcar_gen4_gettime(), adds delta, and writes it back with
_ptp_rcar_gen4_settime(). A torn read there would be committed to the
hardware clock, not just returned to one caller.

If the hardware does latch on the PTPGPTPTM00 read, could a comment
document that? If not, would it be enough to read the seconds before
and after the nanosecond read and retry when they differ?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007185920.4042783-1-niklas.soderlund%2Brenesas%40ragnatech.se