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

[Wen Gu <guwen@linux.alibaba.com>]

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