Re: [PATCH net-next 3/3] ptp: ocp: add WiWi Time Card production image support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says the WiWi register interface 'matches the
  Celestica LitePCIe production image; the VID and flash header differ'.
- [Low] On WiWi, ptp_ocp_probe() makes the PHC visible to userspace at
  ptp_clock_register() (drivers/ptp/ptp_ocp.c:6188).

--

ptp: ocp: add WiWi Time Card production image support

This adds a PCI ID match for the WiWi 2163:1008 Time Card. Before the
card is initialized, the driver checks its PCI layout and core versions.
The ocp_fb_resource entries are translated by 0x02000000 and their
interrupt inputs by 32. UART and XIIC use 32-bit accesses, and a LitePCIe
per-input interrupt gate at BAR0 + 0x1800 is managed during probe and
removal. devlink flash updates are rejected on WiWi cards.

> diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
> index f94b759d9cd65..a81dc113688b6 100644
> --- a/Documentation/networking/devlink/ptp_ocp.rst
> +++ b/Documentation/networking/devlink/ptp_ocp.rst

[ ... ]

> @@ -73,3 +74,35 @@ with the standard devlink status notifications.
>  Example::
>  
>      $ devlink dev flash pci/0000:02:00.0 file adva-cpld.bin component fw.cpld
> +
> +WiWi production firmware
> +========================
> +
> +WiWi Time Cards with PCI ID ``2163:1008`` and subsystem ID ``10ee:0007``
> +use the Celestica register layout behind a LitePCIe bridge. The driver

[ ... ]

> +The image uses the Celestica LitePCIe production map: add ``0x02000000``
> +to the peripheral offsets and use aligned 32-bit MMIO. Interrupt input
> +``n`` uses MSI-X vector ``n + 32``. The per-input enable mask is at BAR0
> +offset ``0x1800``. The driver enables the SPI interrupt before probing

[Severity: Low]
How does this fit with the existing Celestica entry in ptp_ocp_pcidev_id?

The commit message says "the register interface matches the Celestica
LitePCIe production image; the VID and flash header differ". This section
calls the layout "the Celestica LitePCIe production map".

The Celestica entry binds the same table but gets none of this handling:

	{ PCI_DEVICE_DATA(CELESTICA, TIMECARD, &ocp_fb_resource) },

For 18d4:1008, bp->wiwi is false, so:

- ptp_ocp_register_resources() applies neither the 0x02000000 offset nor
  the +32 vector translation.
- ptp_ocp_serial_line() keeps UPIO_MEM.
- probe allocates 1..17 MSI/MSI-X vectors.
- the 0x1800 gate is never opened.

The earlier xiic patch in this series says the LitePCIe bridge raises a
fatal machine check on subword MMIO.

If a card with the Celestica PCI ID can run the LitePCIe production
image, would it need the same handling?

If it can't, could the changelog and this section say that the existing
Celestica entry targets a different image? That would stop the "Celestica
LitePCIe" wording from suggesting otherwise. The line "Leave non-WiWi
resource tables and access modes unchanged" only says that code is
untouched.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 625a85b2dc7e5..ee22423890230 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -6047,6 +6228,11 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  			goto out_dpll;
>  		}
>  	}
> +	/* All handlers and the PHC are ready for timestamp interrupts. */
> +	if (bp->wiwi_msi) {
> +		iowrite32(OCP_WIWI_IRQS, bp->wiwi_msi);
> +		ioread32(bp->wiwi_msi);
> +	}
>  	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);

[Severity: Low]
Can a timestamper stop delivering interrupts if it is armed before this
gate write?

Earlier in ptp_ocp_probe(), this call makes the PHC visible to userspace:

	bp->ptp = ptp_clock_register(&bp->ptp_info, &pdev->dev);

The timing inputs (0-2, 6, 11-16) are only opened here. That happens
after ptp_ocp_complete(), devlink_register() and the DPLL device and pin
registration.

During that window, userspace can arm a timing core:

- PTP_EXTTS_REQUEST or PTP_ENABLE_PPS on /dev/ptpN
- enabling a signal generator through sysfs

Each of these goes through ptp_ocp_ts_enable(). If an event arrives
while the gate is still closed, it sets the core's sticky interrupt
status. Only the ack in ptp_ocp_ts_irq() clears it:

	iowrite32(1, &reg->intr);	/* write 1 to ack */

Whether that event gets delivered once the gate opens depends on how the
LitePCIe enable mask handles inputs that are already asserted. The patch
doesn't document that behaviour.

If the gate only detects edges, would that channel deliver no more
MSI-X interrupts until userspace disables and re-enables it?

Would acknowledging the ts/signal intr status after opening the timing
gate make the probe sequence independent of how the gate behaves?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111041904.48916-1-ahmadexp%40gmail.com