Re: [PATCH net-next v2] ptp: add Alibaba CIPU PTP clock driver

[Wen Gu]

On 2025/6/27 15:57, Andrew Lunn wrote:
> > +#define PTP_CIPU_LOG_SUB(dev, level, type, event, fmt, ...) \
> > +({ \
> > +	static DEFINE_RATELIMIT_STATE(_rs, \
> > +				      DEFAULT_RATELIMIT_INTERVAL, \
> > +				      DEFAULT_RATELIMIT_BURST); \
> > +	if (__ratelimit(&_rs)) \
> > +		dev_printk(level, dev, "[%02x:%02x]: " fmt, \
> > +			   type, event, ##__VA_ARGS__); \
> > +})
>
> Please don't use such wrappers. Just use dev_dbg_ratelimited() etc.

Agree. This was for compatibility with older kernels, I should
change it to new helpers.. Will fix in next version. Thanks!

> > +static int cipu_iowrite8_and_check(void __iomem *addr,
> > +				   u8 value, u8 *res)
> > +{
> > +	iowrite8(value, addr);
> > +	if (value != ioread8(addr))
> > +		return -EIO;
> > +	*res = value;
> > +	return 0;
> > +}
>
> This probably needs a comment. I assume the hardware is broken and
> sometimes writes don't work? You should state that.

Yes, If the cloud device thinks the written value is not what it
expected, the write will fail. I will add a comment about that, thanks.

> > +static void ptp_cipu_print_dev_events(struct ptp_cipu_ctx *ptp_ctx,
> > +				      int event)
> > +{
> > +	struct device *dev = &ptp_ctx->pdev->dev;
> > +	int type = PTP_CIPU_EVT_TYPE_DEV;
> > +
> > +	switch (event) {
> > +	case PTP_CIPU_EVT_H_CLK_ABN:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> > +				 "Atomic Clock Error Detected\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_CLK_ABN_REC:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> > +				 "Atomic Clock Error Recovered\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_MT:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> > +				 "Maintenance Exception Detected\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_MT_REC:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> > +				 "Maintenance Exception Recovered\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_MT_TOUT:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> > +				 "Maintenance Exception Failed to Recover "
> > +				 "within %d us\n", ptp_ctx->regs.mt_tout_us);
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_BUSY:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> > +				 "PHC Busy Detected\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_BUSY_REC:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> > +				 "PHC Busy Recovered\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_ERR:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> > +				 "PHC Error Detected\n");
> > +		break;
> > +	case PTP_CIPU_EVT_H_DEV_ERR_REC:
> > +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> > +				 "PHC Error Recovered\n");
>
> Are these fatal? Or can the device still be used after these errors
> occur?

The clock can't work as expected if these events happened.

The gettime operation will get an invalid timestamp whose
PTP_CIPU_M_TS_ABN bit is set and return -EIO.

> > +static int ptp_cipu_enable(struct ptp_clock_info *info,
> > +			   struct ptp_clock_request *request, int on)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_cipu_settime(struct ptp_clock_info *p,
> > +			    const struct timespec64 *ts)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_cipu_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_cipu_adjtime(struct ptp_clock_info *ptp, s64 delta)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
>
> I've not looked at the core. Are these actually required? Or if they
> are missing, does the core default to -EOPNOTSUPP?

See reply to Vadim :)

> > +static ssize_t register_snapshot_show(struct device *dev,
> > +				      struct device_attribute *attr, char *buf)
> > +{
> > +	struct ptp_cipu_ctx *ctx = pci_get_drvdata(to_pci_dev(dev));
> > +	struct ptp_cipu_regs *regs = &ctx->regs;
> > +
> > +	return sysfs_emit(buf, "%s 0x%x %s 0x%x %s 0x%x %s 0x%x "
> > +			  "%s 0x%x %s 0x%x %s 0x%x %s 0x%x %s 0x%x "
> > +			  "%s 0x%x %s 0x%x %s 0x%x\n",
> > +			  "device_features", regs->dev_feat,
> > +			  "guest_features", regs->gst_feat,
> > +			  "driver_version", regs->drv_ver,
> > +			  "environment_version", regs->env_ver,
> > +			  "device_status", regs->dev_stat,
> > +			  "sync_status", regs->sync_stat,
> > +			  "time_precision(ns)", regs->tm_prec_ns,
> > +			  "epoch_base(years)", regs->epo_base_yr,
> > +			  "leap_second(s)", regs->leap_sec,
> > +			  "max_latency(ns)", regs->max_lat_ns,
> > +			  "maintenance_timeout(us)", regs->mt_tout_us,
> > +			  "offset_threshold(us)", regs->thresh_us);
> > +}
>
> Is this debug? Maybe it should be placed in debugfs, rather than
> sysfs.

These are considered attributes of the CIPU ptp device, so I perfer
to put them in sysfs.

But I found sysfs prefers only one value per file [1]. The format
here may need to be improved.

[1] https://docs.kernel.org/filesystems/sysfs.html

Thank you for these comments!

Wen Gu

> 	Andrew