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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
ptp: Add driver for R-Car Gen4

This patch adds a standalone platform driver for the gPTP timer block
on R-Car Gen4 SoCs (S4, V4H, V4M). The timer shares its module clock
(and reset) with either RSWITCH or RTSN on S4/V4H, and has its own
module clock on V4M. A pre-existing implementation exists at
drivers/net/ethernet/renesas/rcar_gen4_ptp.c consumed by the RSWITCH
and RTSN drivers.

> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index b93640ca08b7..3593fd9da92a 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig

[ ... ]

[Medium]
A pre-existing driver drivers/net/ethernet/renesas/rcar_gen4_ptp.c
already implements essentially the same hardware programming
(identical register offsets, identical adjfine/gettime/settime/adjtime
bodies, identical addend derivation) and is still consumed by RSWITCH
and RTSN via rcar_gen4_ptp_alloc() / rcar_gen4_ptp_register().

After this patch, the same silicon can be described two different ways
in DT (embedded in RTSN as reg-names "tsnes"/"gptp" versus standalone
renesas,rcar-gen4-gptp) and driven by two different code paths.

On r8a779g0 the DTS already keeps the 0xe6449000 gPTP registers mapped
both from the tsn0 node and from the new gptp node, so nothing
prevents both drivers from mapping the same MMIO region and racing on
control writes.

Is there a migration plan for the existing consumers, and can anything
in the binding or driver reject the double-bind case where the same
register block is described both ways in one DT?

> diff --git a/drivers/ptp/ptp_rcar_gen4.c b/drivers/ptp/ptp_rcar_gen4.c
> new file mode 100644
> index 000000000000..0d862849cd4c
> --- /dev/null
> +++ b/drivers/ptp/ptp_rcar_gen4.c

[ ... ]

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

[Low]
Is querying clk_get_rate() before the module is powered up safe here?
The clock is queried before pm_runtime_enable() / pm_runtime_get_sync()
runs, and some clock providers legitimately return 0 for a gated
clock, which would make probe return -ENODEV even when the rate is
well-defined once the module is powered. The older driver received
the rate from the Ethernet parent driver after its own
pm_runtime_get_sync() had run.

[Medium]
The DT binding declares "resets" as a required property, but the
driver never acquires a reset_control (no reset_control_get() /
reset_control_deassert() / reset_control_reset()).

On the Renesas CPG-MSSR reset controller, reset deassertion is a
separate operation from clock enable; clocks are not implicitly
deasserted.

On V4H (r8a779g0) this driver depends implicitly on the RTSN driver
calling reset_control_reset() on the shared module reset ID 2723, but
that reset_control_reset() will also asynchronously reset the gPTP
block after it has been programmed here. On V4M (r8a779h0) where the
binding also applies and no sibling driver exists, nothing would ever
deassert the reset.

Should the driver acquire and deassert its own reset_control, or
should the binding be revised so that the driver does not appear to
support V4M standalone?

> +
> +	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_get_sync(dev);
> +
> +	iowrite32(priv->default_addend, priv->base + PTPTIVC0_REG);
> +	iowrite32(1, priv->base + PTPTMEC_REG);

[High]
Should the return value of pm_runtime_get_sync() be checked here?

The kernel documentation for pm_runtime_get_sync() notes that the
usage counter is incremented in all cases, including error, and
recommends pm_runtime_resume_and_get() when the caller needs to check
success.

If resume fails, the module clock may remain gated and the subsequent
iowrite32() to PTPTIVC0_REG / PTPTMEC_REG would touch a clock-gated
CPG-MSSR module, which can raise a bus error / SError on ARM64. On
top of that, the usage counter is leaked on the error path.

Would something like this be more appropriate:

    ret = pm_runtime_resume_and_get(dev);
    if (ret) {
        pm_runtime_disable(dev);
        return ret;
    }

> +
> +	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);
> +	}

[Low]
The remove path writes PTPTMDC_REG = 1 to disable the timer before
dropping the runtime PM reference, but this error path (when
ptp_clock_register() fails) does not. Since the driver has no
runtime_suspend callback that would stop the timer, should the probe
error path also issue iowrite32(1, priv->base + PTPTMDC_REG) for
symmetry with remove()?

> +
> +	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);
> +}

[Medium]
There are no system-wide dev_pm_ops on this driver (no .suspend /
.resume / .freeze / .thaw / .restore, no SET_LATE_SYSTEM_SLEEP_PM_OPS).

Even with the DT placing the device in R8A779G0_PD_ALWAYS_ON, module
clocks can still be gated by pm_runtime_force_suspend() during a
system suspend/resume cycle. When that happens, PTPTIVC0 (addend),
PTPTMEC (enable), and the timer counter are all liable to lose state,
and there is no callback that reprograms them on resume.

Because ptp_clock_register() exposes /dev/ptpN to userspace, would
ptp4l / phc2sys see a silently wrong clock after a suspend/resume
cycle? The older driver was tied to the Ethernet driver's PM chain,
so this hazard is newly introduced by making the PTP block a
standalone driver.

> +
> +static const struct of_device_id ptp_rcar_gen4_of_match[] = {
> +	{ .compatible = "renesas,rcar-gen4-gptp", },
> +	{ /* Sentinel */ },
> +};

[ ... ]
-- 
This is an AI-generated review.