[PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[min.li.xe]

From: Min Li <min.li.xe@renesas.com>

This driver supports 1588 related functions of ClockMatrix(TM)
and 82P33xxx families of timing and synchronization devices. The
supported functons are:

- set combomode
- get dpll's state
- get dpll's ffo

Please note that this driver needs to work with rsmu mfd driver
to access SMU through I2C/SPI.

Signed-off-by: Min Li <min.li.xe@renesas.com>
---
 drivers/misc/Kconfig      |   7 +
 drivers/misc/Makefile     |   3 +
 drivers/misc/rsmu_cdev.c  | 430 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/rsmu_cdev.h  |  75 ++++++++
 drivers/misc/rsmu_cm.c    | 214 +++++++++++++++++++++++
 drivers/misc/rsmu_sabre.c | 153 +++++++++++++++++
 include/uapi/linux/rsmu.h |  61 +++++++
 7 files changed, 943 insertions(+)
 create mode 100644 drivers/misc/rsmu_cdev.c
 create mode 100644 drivers/misc/rsmu_cdev.h
 create mode 100644 drivers/misc/rsmu_cm.c
 create mode 100644 drivers/misc/rsmu_sabre.c
 create mode 100644 include/uapi/linux/rsmu.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0..2ba5070 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -466,6 +466,13 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config RSMU
+	tristate "Renesas Synchronization Management Unit (SMU)"
+	depends on MFD_RSMU_I2C || MFD_RSMU_SPI
+	help
+	  This driver provides support for Renesas SMU,
+	  such as Clockmatrix and 82P33XXX series.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d23231e..3054c0d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,6 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
 obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+
+rsmu-objs			:= rsmu_cdev.o rsmu_cm.o rsmu_sabre.o
+obj-$(CONFIG_RSMU)		+= rsmu.o
diff --git a/drivers/misc/rsmu_cdev.c b/drivers/misc/rsmu_cdev.c
new file mode 100644
index 0000000..35c1f38
--- /dev/null
+++ b/drivers/misc/rsmu_cdev.c
@@ -0,0 +1,430 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * This driver is developed for the IDT ClockMatrix(TM) and 82P33xxx families of
+ * timing and synchronization devices. It exposes a char device interface in
+ * sysfs and supports file operations like  open(), close() and ioctl().
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/mfd/rsmu.h>
+#include <uapi/linux/rsmu.h>
+
+#include "rsmu_cdev.h"
+
+#define DRIVER_NAME	"rsmu"
+#define DRIVER_MAX_DEV	BIT(MINORBITS)
+
+static struct class *rsmu_class;
+static dev_t rsmu_cdevt;
+static u8 scsr_page;
+
+static int
+rsmu_set_combomode(struct rsmu_cdev *rsmu, void __user *arg)
+{
+	struct rsmu_combomode mode;
+	int err;
+
+	if (copy_from_user(&mode, arg, sizeof(mode)))
+		return -EFAULT;
+
+	mutex_lock(rsmu->lock);
+	switch (rsmu->type) {
+	case RSMU_CM:
+		err = rsmu_cm_set_combomode(rsmu, mode.dpll, mode.mode);
+		break;
+	case RSMU_SABRE:
+		err = rsmu_sabre_set_combomode(rsmu, mode.dpll, mode.mode);
+		break;
+	default:
+		err = -EINVAL;
+	}
+	mutex_unlock(rsmu->lock);
+
+	return err;
+}
+
+static int
+rsmu_get_dpll_state(struct rsmu_cdev *rsmu, void __user *arg)
+{
+	struct rsmu_get_state state_request;
+	u8 state;
+	int err;
+
+	if (copy_from_user(&state_request, arg, sizeof(state_request)))
+		return -EFAULT;
+
+	mutex_lock(rsmu->lock);
+	switch (rsmu->type) {
+	case RSMU_CM:
+		err = rsmu_cm_get_dpll_state(rsmu, state_request.dpll, &state);
+		break;
+	case RSMU_SABRE:
+		err = rsmu_sabre_get_dpll_state(rsmu, state_request.dpll,
+						&state);
+		break;
+	default:
+		err = -EINVAL;
+	}
+	mutex_unlock(rsmu->lock);
+
+	state_request.state = state;
+	if (copy_to_user(arg, &state_request, sizeof(state_request)))
+		return -EFAULT;
+
+	return err;
+}
+
+static int
+rsmu_get_dpll_ffo(struct rsmu_cdev *rsmu, void __user *arg)
+{
+	struct rsmu_get_ffo ffo_request;
+	int err;
+
+	if (copy_from_user(&ffo_request, arg, sizeof(ffo_request)))
+		return -EFAULT;
+
+	mutex_lock(rsmu->lock);
+	switch (rsmu->type) {
+	case RSMU_CM:
+		err = rsmu_cm_get_dpll_ffo(rsmu, ffo_request.dpll,
+					   &ffo_request);
+		break;
+	case RSMU_SABRE:
+		err = rsmu_sabre_get_dpll_ffo(rsmu, ffo_request.dpll,
+					      &ffo_request);
+		break;
+	default:
+		err = -EINVAL;
+	}
+	mutex_unlock(rsmu->lock);
+
+	if (copy_to_user(arg, &ffo_request, sizeof(ffo_request)))
+		return -EFAULT;
+
+	return err;
+}
+
+static ssize_t scsr_show_regs_show(struct device *dev,
+				   struct device_attribute *attr, char *page)
+{
+	struct rsmu_cdev *rsmu = dev_get_drvdata(dev);
+	char *fmt = "%04x:%02x %02x %02x %02x %02x %02x %02x %02x\n";
+	int cnt = 0;
+	u8 regs[256];
+	u16 start;
+	int rows;
+	int row;
+	int err;
+
+	mutex_lock(rsmu->lock);
+	if (rsmu->type == RSMU_CM) {
+		rows = 32;
+		start = 0xc000 + ((u16)scsr_page << 8);
+		if (scsr_page >= 16)
+			err = -EINVAL;
+		else
+			err = rsmu_read(rsmu->mfd, start, regs, 256);
+	} else if (rsmu->type == RSMU_SABRE) {
+		rows = 16;
+		start = (u16)scsr_page << 7;
+		if (scsr_page >= 7)
+			err = -EINVAL;
+		else
+			err = rsmu_read(rsmu->mfd, start, regs, 128);
+
+	} else
+		err = -EINVAL;
+	mutex_unlock(rsmu->lock);
+
+	if (err)
+		return err;
+
+	for (row = 0; row < rows; row++) {
+		u8 index = row * 8;
+
+		cnt += snprintf(page + cnt, PAGE_SIZE - cnt - 1, fmt,
+				start + index, regs[index], regs[index + 1],
+				regs[index + 2], regs[index + 3],
+				regs[index + 4], regs[index + 5],
+				regs[index + 6], regs[index + 7]);
+	}
+
+	return cnt;
+}
+static DEVICE_ATTR_RO(scsr_show_regs);
+
+static ssize_t scsr_set_page_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct rsmu_cdev *rsmu = dev_get_drvdata(dev);
+	u8 page;
+	u8 max;
+
+	if (kstrtou8(buf, 10, &page) < 0)
+		return -EINVAL;
+
+	if (rsmu->type == RSMU_CM)
+		max = 16;
+	else if (rsmu->type == RSMU_SABRE)
+		max = 7;
+	else
+		return -EINVAL;
+
+	if (page >= max)
+		return -EINVAL;
+
+	mutex_lock(rsmu->lock);
+	scsr_page = page;
+	mutex_unlock(rsmu->lock);
+
+	return count;
+}
+static DEVICE_ATTR_WO(scsr_set_page);
+
+static struct attribute *rsmu_attrs[] = {
+	&dev_attr_scsr_set_page.attr,
+	&dev_attr_scsr_show_regs.attr,
+	NULL
+};
+
+static const struct attribute_group rsmu_group = {
+	.attrs = rsmu_attrs
+};
+
+static int
+rsmu_open(struct inode *iptr, struct file *fptr)
+{
+	struct rsmu_cdev *rsmu;
+
+	rsmu = container_of(iptr->i_cdev, struct rsmu_cdev, rsmu_cdev);
+	if (!rsmu)
+		return -EAGAIN;
+
+	/* Only one open per device at a time */
+	if (!atomic_dec_and_test(&rsmu->open_count)) {
+		atomic_inc(&rsmu->open_count);
+		return -EBUSY;
+	}
+
+	fptr->private_data = rsmu;
+	return 0;
+}
+
+static int
+rsmu_release(struct inode *iptr, struct file *fptr)
+{
+	struct rsmu_cdev *rsmu;
+
+	rsmu = container_of(iptr->i_cdev, struct rsmu_cdev, rsmu_cdev);
+	if (!rsmu)
+		return -EAGAIN;
+
+	atomic_inc(&rsmu->open_count);
+	return 0;
+}
+
+static long
+rsmu_ioctl(struct file *fptr, unsigned int cmd, unsigned long data)
+{
+	struct rsmu_cdev *rsmu = fptr->private_data;
+	void __user *arg = (void __user *)data;
+	int err = 0;
+
+	if (!rsmu)
+		return -EINVAL;
+
+	switch (cmd) {
+	case RSMU_SET_COMBOMODE:
+		err = rsmu_set_combomode(rsmu, arg);
+		break;
+	case RSMU_GET_STATE:
+		err = rsmu_get_dpll_state(rsmu, arg);
+		break;
+	case RSMU_GET_FFO:
+		err = rsmu_get_dpll_ffo(rsmu, arg);
+		break;
+	default:
+		/* Should not get here */
+		dev_err(rsmu->dev, "Undefined RSMU IOCTL");
+		err = -EINVAL;
+		break;
+	}
+
+	return err;
+}
+
+static const struct file_operations rsmu_fops = {
+	.owner = THIS_MODULE,
+	.open = rsmu_open,
+	.release = rsmu_release,
+	.unlocked_ioctl = rsmu_ioctl,
+};
+
+static int
+rsmu_probe(struct platform_device *pdev)
+{
+	struct rsmu_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct rsmu_cdev *rsmu;
+	struct device *rsmu_cdev;
+	int err;
+
+	rsmu = devm_kzalloc(&pdev->dev, sizeof(*rsmu), GFP_KERNEL);
+	if (!rsmu)
+		return -ENOMEM;
+
+	rsmu->dev = &pdev->dev;
+	rsmu->mfd = pdev->dev.parent;
+	rsmu->type = pdata->type;
+	rsmu->lock = pdata->lock;
+	rsmu->index = pdata->index;
+
+	/* Save driver private data */
+	platform_set_drvdata(pdev, rsmu);
+
+	cdev_init(&rsmu->rsmu_cdev, &rsmu_fops);
+	rsmu->rsmu_cdev.owner = THIS_MODULE;
+	err = cdev_add(&rsmu->rsmu_cdev,
+		       MKDEV(MAJOR(rsmu_cdevt), 0), 1);
+	if (err < 0) {
+		dev_err(rsmu->dev, "cdev_add failed");
+		err = -EIO;
+		goto err_rsmu_dev;
+	}
+
+	if (!rsmu_class) {
+		err = -EIO;
+		dev_err(rsmu->dev, "rsmu class not created correctly");
+		goto err_rsmu_cdev;
+	}
+
+	rsmu_cdev = device_create(rsmu_class, rsmu->dev,
+				   MKDEV(MAJOR(rsmu_cdevt), 0),
+				   rsmu, "rsmu%d", rsmu->index);
+	if (IS_ERR(rsmu_cdev)) {
+		dev_err(rsmu->dev, "unable to create char device");
+		err = PTR_ERR(rsmu_cdev);
+		goto err_rsmu_cdev;
+	}
+
+	err = sysfs_create_group(&rsmu_cdev->kobj, &rsmu_group);
+	if (err) {
+		dev_err(rsmu->dev, "sysfs_create_group failed\n");
+		goto err_rsmu_cdev;
+	}
+
+	atomic_set(&rsmu->open_count, 1);
+	dev_info(rsmu->dev, "Probe SMU type %d Successful\n", rsmu->type);
+	return 0;
+
+	/* Failure cleanup */
+err_rsmu_cdev:
+	cdev_del(&rsmu->rsmu_cdev);
+err_rsmu_dev:
+	return err;
+}
+
+static int
+rsmu_remove(struct platform_device *pdev)
+{
+	struct rsmu_cdev *rsmu;
+	struct device *dev = &pdev->dev;
+
+	rsmu = platform_get_drvdata(pdev);
+	if (!rsmu)
+		return -ENODEV;
+
+	if (!rsmu_class) {
+		dev_err(dev, "rsmu_class is NULL");
+		return -EIO;
+	}
+
+	device_destroy(rsmu_class, MKDEV(MAJOR(rsmu_cdevt), 0));
+	cdev_del(&rsmu->rsmu_cdev);
+
+	return 0;
+}
+
+static const struct platform_device_id rsmu_id_table[] = {
+	{ "rsmu-cdev0", },
+	{ "rsmu-cdev1", },
+	{ "rsmu-cdev2", },
+	{ "rsmu-cdev3", },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, rsmu_id_table);
+
+static const struct of_device_id rsmu_of_match[] = {
+	{ .compatible = "renesas,rsmu-cdev0", },
+	{ .compatible = "renesas,rsmu-cdev1", },
+	{ .compatible = "renesas,rsmu-cdev2", },
+	{ .compatible = "renesas,rsmu-cdev3", },
+	{ /* end of table */ }
+};
+MODULE_DEVICE_TABLE(of, rsmu_of_match);
+
+static struct platform_driver rsmu_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = rsmu_of_match,
+	},
+	.probe = rsmu_probe,
+	.remove =  rsmu_remove,
+	.id_table = rsmu_id_table,
+};
+
+static int __init rsmu_init(void)
+{
+	int err;
+
+	rsmu_class = class_create(THIS_MODULE, DRIVER_NAME);
+	if (IS_ERR(rsmu_class)) {
+		err = PTR_ERR(rsmu_class);
+		pr_err("%s : Unable to register rsmu class", __func__);
+		return err;
+	}
+
+	err = alloc_chrdev_region(&rsmu_cdevt, 0, DRIVER_MAX_DEV, DRIVER_NAME);
+	if (err < 0) {
+		pr_err("%s : Unable to get major number", __func__);
+		goto err_rsmu_class;
+	}
+
+	err = platform_driver_register(&rsmu_driver);
+	if (err < 0) {
+		pr_err("%s Unabled to register %s driver",
+		       __func__, DRIVER_NAME);
+		goto err_rsmu_drv;
+	}
+	return 0;
+
+	/* Error Path */
+err_rsmu_drv:
+	unregister_chrdev_region(rsmu_cdevt, DRIVER_MAX_DEV);
+err_rsmu_class:
+	class_destroy(rsmu_class);
+	return err;
+}
+
+static void __exit rsmu_exit(void)
+{
+	platform_driver_unregister(&rsmu_driver);
+	unregister_chrdev_region(rsmu_cdevt, DRIVER_MAX_DEV);
+	class_destroy(rsmu_class);
+	rsmu_class = NULL;
+}
+
+module_init(rsmu_init);
+module_exit(rsmu_exit);
+
+MODULE_DESCRIPTION("Renesas SMU character device driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/rsmu_cdev.h b/drivers/misc/rsmu_cdev.h
new file mode 100644
index 0000000..4b38790a
--- /dev/null
+++ b/drivers/misc/rsmu_cdev.h
@@ -0,0 +1,75 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * This driver is developed for the IDT ClockMatrix(TM) of
+ * timing and synchronization devices.
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+#ifndef __LINUX_RSMU_CDEV_H
+#define __LINUX_RSMU_CDEV_H
+
+#include <linux/cdev.h>
+
+/**
+ * struct rsmu_cdev - Driver data for RSMU character device
+ * @dev: pointer to platform device
+ * @mfd: pointer to MFD device
+ * @open_count: Count of char device being opened
+ * @rsmu_cdev: Character device handle
+ * @lock: Mutex to protect operations from being interrupted
+ */
+struct rsmu_cdev {
+	struct device *dev;
+	struct device *mfd;
+	struct cdev rsmu_cdev;
+	struct mutex *lock;
+	enum rsmu_type type;
+	atomic_t open_count;
+	u8 index;
+};
+
+/**
+ * Enumerated type listing DPLL combination modes
+ */
+enum rsmu_dpll_combomode {
+	E_COMBOMODE_CURRENT = 0,
+	E_COMBOMODE_FASTAVG,
+	E_COMBOMODE_SLOWAVG,
+	E_COMBOMODE_HOLDOVER,
+	E_COMBOMODE_MAX,
+};
+
+/**
+ * An id used to identify the respective child class states.
+ */
+enum rsmu_class_state {
+	E_SRVLOINITIALSTATE = 0,
+	E_SRVLOUNQUALIFIEDSTATE = 1,
+	E_SRVLOLOCKACQSTATE = 2,
+	E_SRVLOFREQUENCYLOCKEDSTATE = 3,
+	E_SRVLOTIMELOCKEDSTATE = 4,
+	E_SRVLOHOLDOVERINSPECSTATE = 5,
+	E_SRVLOHOLDOVEROUTOFSPECSTATE = 6,
+	E_SRVLOFREERUNSTATE = 7,
+	E_SRVNUMBERLOSTATES = 8,
+	E_SRVLOSTATEINVALID = 9
+};
+
+extern int
+rsmu_cm_set_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 mode);
+extern int
+rsmu_cm_get_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 *mode);
+extern int
+rsmu_cm_get_dpll_state(struct rsmu_cdev *rsmu, u8 dpll, u8 *state);
+extern int
+rsmu_cm_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll, struct rsmu_get_ffo *ffo);
+extern int
+rsmu_sabre_set_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 mode);
+extern int
+rsmu_sabre_get_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 *mode);
+extern int
+rsmu_sabre_get_dpll_state(struct rsmu_cdev *rsmu, u8 dpll, u8 *state);
+extern int
+rsmu_sabre_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll,
+			struct rsmu_get_ffo *ffo);
+#endif
diff --git a/drivers/misc/rsmu_cm.c b/drivers/misc/rsmu_cm.c
new file mode 100644
index 0000000..26779e6
--- /dev/null
+++ b/drivers/misc/rsmu_cm.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * This driver is developed for the IDT ClockMatrix(TM) of
+ * timing and synchronization devices.
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/device.h>
+#include <linux/mfd/idt8a340_reg.h>
+#include <linux/mfd/rsmu.h>
+#include <uapi/linux/rsmu.h>
+
+#include "rsmu_cdev.h"
+
+int rsmu_cm_set_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 mode)
+{
+	u16 dpll_ctrl_n;
+	u8 cfg;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_ctrl_n = DPLL_CTRL_0;
+		break;
+	case 1:
+		dpll_ctrl_n = DPLL_CTRL_1;
+		break;
+	case 2:
+		dpll_ctrl_n = DPLL_CTRL_2;
+		break;
+	case 3:
+		dpll_ctrl_n = DPLL_CTRL_3;
+		break;
+	case 4:
+		dpll_ctrl_n = DPLL_CTRL_4;
+		break;
+	case 5:
+		dpll_ctrl_n = DPLL_CTRL_5;
+		break;
+	case 6:
+		dpll_ctrl_n = DPLL_CTRL_6;
+		break;
+	case 7:
+		dpll_ctrl_n = DPLL_CTRL_7;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (mode >= E_COMBOMODE_MAX)
+		return -EINVAL;
+
+	err = rsmu_read(rsmu->mfd,
+			  dpll_ctrl_n + DPLL_CTRL_COMBO_MASTER_CFG,
+			  &cfg, sizeof(cfg));
+	if (err)
+		return err;
+
+	/* Only need to enable/disable COMBO_MODE_HOLD. */
+	if (mode)
+		cfg |= COMBO_MASTER_HOLD;
+	else
+		cfg &= ~COMBO_MASTER_HOLD;
+
+	return rsmu_write(rsmu->mfd,
+			    dpll_ctrl_n + DPLL_CTRL_COMBO_MASTER_CFG,
+			    &cfg, sizeof(cfg));
+}
+
+int rsmu_cm_get_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 *mode)
+{
+	u16 dpll_ctrl_n;
+	u8 cfg;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_ctrl_n = DPLL_CTRL_0;
+		break;
+	case 1:
+		dpll_ctrl_n = DPLL_CTRL_1;
+		break;
+	case 2:
+		dpll_ctrl_n = DPLL_CTRL_2;
+		break;
+	case 3:
+		dpll_ctrl_n = DPLL_CTRL_3;
+		break;
+	case 4:
+		dpll_ctrl_n = DPLL_CTRL_4;
+		break;
+	case 5:
+		dpll_ctrl_n = DPLL_CTRL_5;
+		break;
+	case 6:
+		dpll_ctrl_n = DPLL_CTRL_6;
+		break;
+	case 7:
+		dpll_ctrl_n = DPLL_CTRL_7;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = rsmu_read(rsmu->mfd,
+			  dpll_ctrl_n + DPLL_CTRL_COMBO_MASTER_CFG,
+			  &cfg, sizeof(cfg));
+	if (err)
+		return err;
+
+	*mode = cfg & COMBO_MASTER_HOLD;
+
+	return 0;
+}
+
+int rsmu_cm_get_dpll_state(struct rsmu_cdev *rsmu, u8 dpll, u8 *state)
+{
+	u8 cfg;
+	int err;
+
+	/* 8 is sys dpll */
+	if (dpll > 8)
+		return -EINVAL;
+
+	err = rsmu_read(rsmu->mfd,
+			  STATUS + DPLL0_STATUS + dpll,
+			  &cfg, sizeof(cfg));
+	if (err)
+		return err;
+
+	switch (cfg & DPLL_STATE_MASK) {
+	case E_DPLL_STATE_FREERUN:
+		*state = E_SRVLOUNQUALIFIEDSTATE;
+		break;
+	case E_DPLL_STATE_LOCKACQ:
+	case E_DPLL_STATE_LOCKREQ:
+		*state = E_SRVLOLOCKACQSTATE;
+		break;
+	case E_DPLL_STATE_LOCKED:
+		*state = E_SRVLOTIMELOCKEDSTATE;
+		break;
+	case E_DPLL_STATE_HOLDOVER:
+		*state = E_SRVLOHOLDOVERINSPECSTATE;
+		break;
+	default:
+		*state = E_SRVLOSTATEINVALID;
+		break;
+	}
+
+	return 0;
+}
+
+int rsmu_cm_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll,
+			 struct rsmu_get_ffo *ffo)
+{
+	u8 buf[6] = {0};
+	s64 fcw = 0;
+	u16 dpll_filter_status;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_filter_status = DPLL0_FILTER_STATUS;
+		break;
+	case 1:
+		dpll_filter_status = DPLL1_FILTER_STATUS;
+		break;
+	case 2:
+		dpll_filter_status = DPLL2_FILTER_STATUS;
+		break;
+	case 3:
+		dpll_filter_status = DPLL3_FILTER_STATUS;
+		break;
+	case 4:
+		dpll_filter_status = DPLL4_FILTER_STATUS;
+		break;
+	case 5:
+		dpll_filter_status = DPLL5_FILTER_STATUS;
+		break;
+	case 6:
+		dpll_filter_status = DPLL6_FILTER_STATUS;
+		break;
+	case 7:
+		dpll_filter_status = DPLL7_FILTER_STATUS;
+		break;
+	case 8:
+		dpll_filter_status = DPLLSYS_FILTER_STATUS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = rsmu_read(rsmu->mfd, STATUS + dpll_filter_status, buf, 6);
+	if (err)
+		return err;
+
+	/* Convert to frequency control word */
+	fcw = buf[0];
+	fcw |= buf[1] << 8LL;
+	fcw |= buf[2] << 16LL;
+	fcw |= buf[3] << 24LL;
+	fcw |= buf[4] << 32LL;
+	fcw |= buf[5] << 40LL;
+	if (buf[5] & BIT(7))
+		/* Sign extend */
+		fcw |= GENMASK(63, 48);
+
+	/* FCW unit is 2 ^ -53 = 1.1102230246251565404236316680908e-16 */
+	ffo->ffo = fcw * 111;
+
+	return 0;
+}
diff --git a/drivers/misc/rsmu_sabre.c b/drivers/misc/rsmu_sabre.c
new file mode 100644
index 0000000..8b27d6e
--- /dev/null
+++ b/drivers/misc/rsmu_sabre.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * This driver is developed for the IDT 82P33XXX series of
+ * timing and synchronization devices.
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/device.h>
+#include <linux/mfd/idt82p33_reg.h>
+#include <linux/mfd/rsmu.h>
+#include <uapi/linux/rsmu.h>
+
+#include "rsmu_cdev.h"
+
+int rsmu_sabre_set_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 mode)
+{
+	u16 dpll_ctrl_n;
+	u8 cfg;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_ctrl_n = DPLL1_OPERATING_MODE_CNFG;
+		break;
+	case 1:
+		dpll_ctrl_n = DPLL2_OPERATING_MODE_CNFG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (mode >= E_COMBOMODE_MAX)
+		return -EINVAL;
+
+	err = rsmu_read(rsmu->mfd, dpll_ctrl_n, &cfg, sizeof(cfg));
+	if (err)
+		return err;
+
+	cfg &= ~(COMBO_MODE_MASK << COMBO_MODE_SHIFT);
+	cfg |= (mode << COMBO_MODE_SHIFT);
+
+	return rsmu_write(rsmu->mfd, dpll_ctrl_n, &cfg, sizeof(cfg));
+}
+
+int rsmu_sabre_get_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 *mode)
+{
+	u16 dpll_ctrl_n;
+	u8 cfg;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_ctrl_n = DPLL1_OPERATING_MODE_CNFG;
+		break;
+	case 1:
+		dpll_ctrl_n = DPLL2_OPERATING_MODE_CNFG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = rsmu_read(rsmu->mfd, dpll_ctrl_n, &cfg, sizeof(cfg));
+	if (err)
+		return err;
+
+	*mode = cfg >> COMBO_MODE_SHIFT;
+
+	return 0;
+}
+
+int rsmu_sabre_get_dpll_state(struct rsmu_cdev *rsmu, u8 dpll, u8 *state)
+{
+	u16 dpll_sts_n;
+	u8 cfg;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_sts_n = DPLL1_OPERATING_STS;
+		break;
+	case 1:
+		dpll_sts_n = DPLL2_OPERATING_STS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = rsmu_read(rsmu->mfd, dpll_sts_n, &cfg, sizeof(cfg));
+	if (err)
+		return err;
+
+	switch (cfg & OPERATING_STS_MASK) {
+	case DPLL_STATE_FREERUN:
+		*state = E_SRVLOUNQUALIFIEDSTATE;
+		break;
+	case DPLL_STATE_PRELOCKED2:
+	case DPLL_STATE_PRELOCKED:
+		*state = E_SRVLOLOCKACQSTATE;
+		break;
+	case DPLL_STATE_LOCKED:
+		*state = E_SRVLOTIMELOCKEDSTATE;
+		break;
+	case DPLL_STATE_HOLDOVER:
+		*state = E_SRVLOHOLDOVERINSPECSTATE;
+		break;
+	default:
+		*state = E_SRVLOSTATEINVALID;
+		break;
+	}
+
+	return 0;
+}
+
+int rsmu_sabre_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll,
+			    struct rsmu_get_ffo *ffo)
+{
+	u8 buf[5] = {0};
+	s64 fcw = 0;
+	u16 dpll_freq_n;
+	int err;
+
+	switch (dpll) {
+	case 0:
+		dpll_freq_n = DPLL1_HOLDOVER_FREQ_CNFG;
+		break;
+	case 1:
+		dpll_freq_n = DPLL2_HOLDOVER_FREQ_CNFG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = rsmu_read(rsmu->mfd, dpll_freq_n, buf, 5);
+	if (err)
+		return err;
+
+	/* Convert to frequency control word */
+	fcw = buf[0];
+	fcw |= buf[1] << 8LL;
+	fcw |= buf[2] << 16LL;
+	fcw |= buf[3] << 24LL;
+	fcw |= buf[4] << 32LL;
+	if (buf[4] & BIT(7))
+		/* Sign extend */
+		fcw |= GENMASK(63, 40);
+
+	/* FCW unit is 77760 / ( 1638400 * 2^48) = 1.68615121864946 * 10^-16 */
+	ffo->ffo = div_s64(fcw * 168615, 1000);
+
+	return 0;
+}
diff --git a/include/uapi/linux/rsmu.h b/include/uapi/linux/rsmu.h
new file mode 100644
index 0000000..895e383
--- /dev/null
+++ b/include/uapi/linux/rsmu.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Driver for the IDT ClockMatrix(TM) and 82p33xxx families of
+ * timing and synchronization devices.
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+
+#ifndef __UAPI_LINUX_RSMU_CDEV_H
+#define __UAPI_LINUX_RSMU_CDEV_H
+
+/* Set dpll combomode */
+struct rsmu_combomode {
+	__u8 dpll;
+	__u8 mode;
+};
+
+/* Get dpll state */
+struct rsmu_get_state {
+	__u8 dpll;
+	__u8 state;
+};
+
+/* Get dpll ffo */
+struct rsmu_get_ffo {
+	__u8 dpll;
+	__s64 ffo; /* in ppqt */
+};
+
+/*
+ * RSMU IOCTL List
+ */
+#define RSMU_MAGIC '?'
+
+/**
+ * @Description
+ * ioctl to set SMU combo mode.
+ *
+ * @Parameters
+ * pointer to struct rsmu_combomode that contains dpll combomode setting
+ */
+#define RSMU_SET_COMBOMODE  _IOW(RSMU_MAGIC, 1, struct rsmu_combomode *)
+
+/**
+ * @Description
+ * ioctl to get SMU dpll state.
+ *
+ * @Parameters
+ * pointer to struct rsmu_get_state that contains dpll state
+ */
+#define RSMU_GET_STATE  _IOW(RSMU_MAGIC, 2, struct rsmu_get_state *)
+
+/**
+ * @Description
+ * ioctl to get SMU dpll ffo.
+ *
+ * @Parameters
+ * pointer to struct rsmu_get_ffo that contains dpll fcw and granularity
+ */
+#define RSMU_GET_FFO  _IOW(RSMU_MAGIC, 3, struct rsmu_get_ffo *)
+#endif /* __UAPI_LINUX_RSMU_CDEV_H */
-- 
2.7.4

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/min-li-xe-renesas-com/misc-Add-Renesas-Synchronization-Management-Unit-SMU-support/20210211-113223
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e4b62cf7559f2ef9a022de235e5a09a8d7ded520
config: powerpc-randconfig-r022-20210209 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/2a68b783a1608476c14439dd4ad92534715fc22d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review min-li-xe-renesas-com/misc-Add-Renesas-Synchronization-Management-Unit-SMU-support/20210211-113223
        git checkout 2a68b783a1608476c14439dd4ad92534715fc22d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> error: include/uapi/linux/rsmu.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/rsmu.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1290: headers] Error 2
   make[1]: Target 'headers_install' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'headers_install' not remade because of errors.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29437 bytes --]

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Greg KH]

On Wed, Feb 10, 2021 at 10:03:31PM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
> 
> This driver supports 1588 related functions of ClockMatrix(TM)
> and 82P33xxx families of timing and synchronization devices. The
> supported functons are:
> 
> - set combomode
> - get dpll's state
> - get dpll's ffo
> 
> Please note that this driver needs to work with rsmu mfd driver
> to access SMU through I2C/SPI.
> 
> Signed-off-by: Min Li <min.li.xe@renesas.com>

Where is the new user/kernel api you are creating here documented?  What
userspace tools use it?

thanks,

greg k-h

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Greg KH]

On Wed, Feb 10, 2021 at 10:03:31PM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
> 
> This driver supports 1588 related functions of ClockMatrix(TM)
> and 82P33xxx families of timing and synchronization devices. The
> supported functons are:
> 
> - set combomode
> - get dpll's state
> - get dpll's ffo
> 
> Please note that this driver needs to work with rsmu mfd driver
> to access SMU through I2C/SPI.
> 
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> ---
>  drivers/misc/Kconfig      |   7 +
>  drivers/misc/Makefile     |   3 +
>  drivers/misc/rsmu_cdev.c  | 430 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/misc/rsmu_cdev.h  |  75 ++++++++
>  drivers/misc/rsmu_cm.c    | 214 +++++++++++++++++++++++
>  drivers/misc/rsmu_sabre.c | 153 +++++++++++++++++
>  include/uapi/linux/rsmu.h |  61 +++++++
>  7 files changed, 943 insertions(+)
>  create mode 100644 drivers/misc/rsmu_cdev.c
>  create mode 100644 drivers/misc/rsmu_cdev.h
>  create mode 100644 drivers/misc/rsmu_cm.c
>  create mode 100644 drivers/misc/rsmu_sabre.c
>  create mode 100644 include/uapi/linux/rsmu.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index fafa8b0..2ba5070 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -466,6 +466,13 @@ config HISI_HIKEY_USB
>  	  switching between the dual-role USB-C port and the USB-A host ports
>  	  using only one USB controller.
>  
> +config RSMU
> +	tristate "Renesas Synchronization Management Unit (SMU)"
> +	depends on MFD_RSMU_I2C || MFD_RSMU_SPI
> +	help
> +	  This driver provides support for Renesas SMU,
> +	  such as Clockmatrix and 82P33XXX series.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index d23231e..3054c0d 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,3 +57,6 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
>  obj-$(CONFIG_UACCE)		+= uacce/
>  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
> +
> +rsmu-objs			:= rsmu_cdev.o rsmu_cm.o rsmu_sabre.o
> +obj-$(CONFIG_RSMU)		+= rsmu.o
> diff --git a/drivers/misc/rsmu_cdev.c b/drivers/misc/rsmu_cdev.c
> new file mode 100644
> index 0000000..35c1f38
> --- /dev/null
> +++ b/drivers/misc/rsmu_cdev.c
> @@ -0,0 +1,430 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * This driver is developed for the IDT ClockMatrix(TM) and 82P33xxx families of
> + * timing and synchronization devices. It exposes a char device interface in
> + * sysfs and supports file operations like  open(), close() and ioctl().

There is no "char device interface in sysfs".

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Thu, Feb 11, 2021 at 4:03 AM <min.li.xe@renesas.com> wrote:
>
> From: Min Li <min.li.xe@renesas.com>
>
> This driver supports 1588 related functions of ClockMatrix(TM)
> and 82P33xxx families of timing and synchronization devices. The
> supported functons are:
>
> - set combomode
> - get dpll's state
> - get dpll's ffo
>
> Please note that this driver needs to work with rsmu mfd driver
> to access SMU through I2C/SPI.
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> ---
>  drivers/misc/Kconfig      |   7 +
>  drivers/misc/Makefile     |   3 +
>  drivers/misc/rsmu_cdev.c  | 430 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/misc/rsmu_cdev.h  |  75 ++++++++
>  drivers/misc/rsmu_cm.c    | 214 +++++++++++++++++++++++
>  drivers/misc/rsmu_sabre.c | 153 +++++++++++++++++
>  include/uapi/linux/rsmu.h |  61 +++++++
>  7 files changed, 943 insertions(+)
>  create mode 100644 drivers/misc/rsmu_cdev.c
>  create mode 100644 drivers/misc/rsmu_cdev.h
>  create mode 100644 drivers/misc/rsmu_cm.c
>  create mode 100644 drivers/misc/rsmu_sabre.c
>  create mode 100644 include/uapi/linux/rsmu.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index fafa8b0..2ba5070 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -466,6 +466,13 @@ config HISI_HIKEY_USB
>           switching between the dual-role USB-C port and the USB-A host ports
>           using only one USB controller.
>
> +config RSMU
> +       tristate "Renesas Synchronization Management Unit (SMU)"
> +       depends on MFD_RSMU_I2C || MFD_RSMU_SPI
> +       help
> +         This driver provides support for Renesas SMU,
> +         such as Clockmatrix and 82P33XXX series.

There should probably be a description of the purpose of the
hardware both here and in the patch description.

In particular, please explain how it relates to the existing
clockmatrix driver.

> +
> +#define DRIVER_NAME    "rsmu"
> +#define DRIVER_MAX_DEV BIT(MINORBITS)
> +
> +static struct class *rsmu_class;
> +static dev_t rsmu_cdevt;

Maybe split the class support from the hardware specific parts
to let other drivers interface with the class when they provide
the same functionality through the common ioctl?

> +static u8 scsr_page;
> +
> +static int
> +rsmu_set_combomode(struct rsmu_cdev *rsmu, void __user *arg)
> +{
> +       struct rsmu_combomode mode;
> +       int err;
> +
> +       if (copy_from_user(&mode, arg, sizeof(mode)))
> +               return -EFAULT;
> +
> +       mutex_lock(rsmu->lock);
> +       switch (rsmu->type) {
> +       case RSMU_CM:
> +               err = rsmu_cm_set_combomode(rsmu, mode.dpll, mode.mode);
> +               break;
> +       case RSMU_SABRE:
> +               err = rsmu_sabre_set_combomode(rsmu, mode.dpll, mode.mode);
> +               break;
> +       default:
> +               err = -EINVAL;
> +       }

The multiplexer here already looks like this should be two drivers that
attach to the class and provide callbacks.

> +static ssize_t scsr_show_regs_show(struct device *dev,
> +                                  struct device_attribute *attr, char *page)

The user ABI needs to be documented.

> +       for (row = 0; row < rows; row++) {
> +               u8 index = row * 8;
> +
> +               cnt += snprintf(page + cnt, PAGE_SIZE - cnt - 1, fmt,
> +                               start + index, regs[index], regs[index + 1],
> +                               regs[index + 2], regs[index + 3],
> +                               regs[index + 4], regs[index + 5],
> +                               regs[index + 6], regs[index + 7]);
> +       }
> +static DEVICE_ATTR_RO(scsr_show_regs);

A pure list of register values seems neither particular portable
nor intuitive. How is a user expected to interpret these, and are
you sure that any driver for this class would have the same
interpretation at the same register index?

> +       /* Only one open per device at a time */
> +       if (!atomic_dec_and_test(&rsmu->open_count)) {
> +               atomic_inc(&rsmu->open_count);
> +               return -EBUSY;
> +       }

Can you explain the purpose of this restriction? Why is it
ok for two threads to access the same file descriptor, but not
two different file descriptors for the same device?

> +
> +static const struct file_operations rsmu_fops = {
> +       .owner = THIS_MODULE,
> +       .open = rsmu_open,
> +       .release = rsmu_release,
> +       .unlocked_ioctl = rsmu_ioctl,
> +};

There should be a .compat_ioctl entry here for 32-bit processes.
If the commands are all defined to be compatible, you can point
it to compat_ptr_ioctl

> +static const struct platform_device_id rsmu_id_table[] = {
> +       { "rsmu-cdev0", },
> +       { "rsmu-cdev1", },
> +       { "rsmu-cdev2", },
> +       { "rsmu-cdev3", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(platform, rsmu_id_table);
> +
> +static const struct of_device_id rsmu_of_match[] = {
> +       { .compatible = "renesas,rsmu-cdev0", },
> +       { .compatible = "renesas,rsmu-cdev1", },
> +       { .compatible = "renesas,rsmu-cdev2", },
> +       { .compatible = "renesas,rsmu-cdev3", },
> +       { /* end of table */ }
> +};
> +MODULE_DEVICE_TABLE(of, rsmu_of_match);

Each of these needs a device tree binding. It's usually better to
name the compatible strings according to the chips that contain
this hardware, such as "renesas,r8a1234567-rsmucdev" instead
of "renesas,rsmu-cdev0".

Since you don't seem to about the difference between the devices,
the driver can also just bind to one of them (usually the oldest)
and then the newer devices contain the string as a fallback, so
you don't have to update the driver every time another variant
gets made.

> +static struct platform_driver rsmu_driver = {
> +       .driver = {
> +               .name = DRIVER_NAME,

I'd suggest open-coding the string here, to make it easier to grep for.

> +               .of_match_table = rsmu_of_match,
> +       },

> diff --git a/drivers/misc/rsmu_cdev.h b/drivers/misc/rsmu_cdev.h
> new file mode 100644
> index 0000000..4b38790a
> --- /dev/null
> +++ b/drivers/misc/rsmu_cdev.h
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * This driver is developed for the IDT ClockMatrix(TM) of
> + * timing and synchronization devices.
> + *
> + * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
> + */
> +#ifndef __LINUX_RSMU_CDEV_H
> +#define __LINUX_RSMU_CDEV_H
> +
> +#include <linux/cdev.h>
> +
> +/**
> + * struct rsmu_cdev - Driver data for RSMU character device
> + * @dev: pointer to platform device
> + * @mfd: pointer to MFD device
> + * @open_count: Count of char device being opened
> + * @rsmu_cdev: Character device handle
> + * @lock: Mutex to protect operations from being interrupted
> + */
> +struct rsmu_cdev {
> +       struct device *dev;
> +       struct device *mfd;
> +       struct cdev rsmu_cdev;
> +       struct mutex *lock;
> +       enum rsmu_type type;
> +       atomic_t open_count;
> +       u8 index;
> +};

This should probably be part of the .c file, as no other driver needs to
interface with it.

> +extern int
> +rsmu_cm_set_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 mode);
> +extern int
> +rsmu_cm_get_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 *mode);
> +extern int
> +rsmu_cm_get_dpll_state(struct rsmu_cdev *rsmu, u8 dpll, u8 *state);
> +extern int
> +rsmu_cm_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll, struct rsmu_get_ffo *ffo);
> +extern int
> +rsmu_sabre_set_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 mode);
> +extern int
> +rsmu_sabre_get_combomode(struct rsmu_cdev *rsmu, u8 dpll, u8 *mode);
> +extern int
> +rsmu_sabre_get_dpll_state(struct rsmu_cdev *rsmu, u8 dpll, u8 *state);
> +extern int
> +rsmu_sabre_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll,
> +                       struct rsmu_get_ffo *ffo);

This tells me that you got the abstraction the wrong way: the common files
should not need to know anything about the specific implementations.

Instead, these should be in separate modules that call exported functions
from the common code.


> +/* Get dpll ffo */
> +struct rsmu_get_ffo {
> +       __u8 dpll;
> +       __s64 ffo; /* in ppqt */
> +};

Please avoid padding in uapi data structures, see
Documentation/driver-api/ioctl.rst

> +/**
> + * @Description
> + * ioctl to set SMU combo mode.
> + *
> + * @Parameters
> + * pointer to struct rsmu_combomode that contains dpll combomode setting
> + */
> +#define RSMU_SET_COMBOMODE  _IOW(RSMU_MAGIC, 1, struct rsmu_combomode *)

This does not match the code, which expects the argument to point directly
to the data, rather than an indirect pointer. By fixing the command definition,
it also becomes compatible with 32-bit processes:

#define RSMU_SET_COMBOMODE  _IOW(RSMU_MAGIC, 1, struct rsmu_combomode)

Maybe expand the documentation for each command a little to explain what
the fields exactly refer to. This also ensures that you are not making it
too specific to a particular hardware implementation.

        Arnd

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 2:40 AM Min Li <min.li.xe@renesas.com> wrote:
> > There should probably be a description of the purpose of the hardware both
> > here and in the patch description.
> >
> > In particular, please explain how it relates to the existing clockmatrix driver.
>
> I just uploaded v2 patch to provide more background info for this change.

It appears that you accidentally dropped the Cc list, adding them back in the
reply.  Also adding the PTP maintainer, as this clearly needs to be reviewed
in the context of the PTP subsystem. Please keep Richard and the netdev
list on Cc in future submissions.

> This driver is developed to be only used by Renesas PTP Clock Manager for
> Linux (pcm4l) software.
>
> This driver supports 1588/PTP releated functionalities that
> specific to Renesas devices but are not supported by general PHC framework.
> So pcm4l will use both the existing PHC driver and this driver to complete
> 1588/PTP support.

Ah, so if this is for a PTP related driver, it should probably be integrated
into the PTP subsystem rather than being a separate class.

> > A pure list of register values seems neither particular portable nor intuitive.
> > How is a user expected to interpret these, and are you sure that any driver
> > for this class would have the same interpretation at the same register index?
> >
>
> Yes we need a way to dump register values when remote debugging with customers.
> And all the Renesas SMU has similar register layout

A sysfs interface is a poor choice for this though -- how can you guarantee that
even future Renesas devices follow the exact same register layout? By
encoding the current hardware generation into the user interface, you
would end up having to emulate this on other chips you want to support later.

If it's only for debugging, best leave it out of the public interface, and only
have it in your own copy of the driver until the bugs are gone, or add a debugfs
interface.

> > Can you explain the purpose of this restriction? Why is it ok for two threads
> > to access the same file descriptor, but not two different file descriptors for
> > the same device?
> >
>
> The mutex is there to provide synchronization to the device access while PHC driver is accessing the device.
> The atomic count is to make sure only one user space program is using the driver at a time.

Then remove the atomic count, as it clearly doesn't do what you describe
when you can have multiple threads access the driver concurrently.

> > Each of these needs a device tree binding. It's usually better to name the
> > compatible strings according to the chips that contain this hardware, such as
> > "renesas,r8a1234567-rsmucdev" instead of "renesas,rsmu-cdev0".
> >
> > Since you don't seem to about the difference between the devices, the driver
> > can also just bind to one of them (usually the oldest) and then the newer
> > devices contain the string as a fallback, so you don't have to update the
> > driver every time another variant gets made.
> >
> >
>
> Actually the device is not spawned from device tree but from Renesas MFD driver (submitted in a separate thread).
> The MFD driver will call mfd_add_devices to create the platform devices. I am not sure if I still need to create binding
> In this case.

If you have an of_device_id table, it needs a binding. It sounds like you
don't need the of_device_id though.

> > This should probably be part of the .c file, as no other driver needs to
> > interface with it.
> >
>
> We actually run a unit test on the driver that needs to access this structure.
> That is why I need to put it in a header

Unit tests are good, but it's better to have them in the kernel.
Can you add the unit test into the patch then?
We now have the kunit framework for running unit tests.

> > This tells me that you got the abstraction the wrong way: the common files
> > should not need to know anything about the specific implementations.
> >
> > Instead, these should be in separate modules that call exported functions
> > from the common code.
> >
> >
>
> I got what you mean. But so far it only supports small set of functions, which is why
> I don't feet it is worth the effort to over abstract things.

Then maybe pick one of the two hardware variants and drop the abstraction you
have. You can then add more features before you add a proper abstraction
layer and then the second driver.

            Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> 
> Ah, so if this is for a PTP related driver, it should probably be integrated into
> the PTP subsystem rather than being a separate class.
> 

I was trying to add these functions to PHC subsystem but was not accepted because the functions
are specific to Renesas device and there is no place for those functions in PHC driver.

> > > A pure list of register values seems neither particular portable nor
> intuitive.
> > > How is a user expected to interpret these, and are you sure that any
> > > driver for this class would have the same interpretation at the same
> register index?
> > >
> >
> > Yes we need a way to dump register values when remote debugging with
> customers.
> > And all the Renesas SMU has similar register layout
> 
> A sysfs interface is a poor choice for this though -- how can you guarantee
> that even future Renesas devices follow the exact same register layout? By
> encoding the current hardware generation into the user interface, you would
> end up having to emulate this on other chips you want to support later.
> 
> If it's only for debugging, best leave it out of the public interface, and only
> have it in your own copy of the driver until the bugs are gone, or add a
> debugfs interface.
> 

I will drop the sysfs change in the new patch

> 
> Unit tests are good, but it's better to have them in the kernel.
> Can you add the unit test into the patch then?
> We now have the kunit framework for running unit tests.
> 

Our unit test is based on ceedling. But I will definitely look into the kunit and try to
Transfer it to kunit for the next release.

> > > This tells me that you got the abstraction the wrong way: the common
> > > files should not need to know anything about the specific
> implementations.
> > >
> > > Instead, these should be in separate modules that call exported
> > > functions from the common code.
> > >
> > >
> >
> > I got what you mean. But so far it only supports small set of
> > functions, which is why I don't feet it is worth the effort to over abstract
> things.
> 
> Then maybe pick one of the two hardware variants and drop the abstraction
> you have. You can then add more features before you add a proper
> abstraction layer and then the second driver.
> 

If I come up with a new file and move all the abstraction code there, does that work?

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 5:19 PM Min Li <min.li.xe@renesas.com> wrote:
>
> >
> > Ah, so if this is for a PTP related driver, it should probably be integrated into
> > the PTP subsystem rather than being a separate class.
> >
>
> I was trying to add these functions to PHC subsystem but was not accepted because the functions
> are specific to Renesas device and there is no place for those functions in PHC driver.

It would be useful to explain that in the patch description and link
to the original
discussion there. What exactly was the objection?

> > > > This tells me that you got the abstraction the wrong way: the common
> > > > files should not need to know anything about the specific
> > implementations.
> > > >
> > > > Instead, these should be in separate modules that call exported
> > > > functions from the common code.
> > > >
> > > >
> > >
> > > I got what you mean. But so far it only supports small set of
> > > functions, which is why I don't feet it is worth the effort to over abstract
> > things.
> >
> > Then maybe pick one of the two hardware variants and drop the abstraction
> > you have. You can then add more features before you add a proper
> > abstraction layer and then the second driver.
> >
>
> If I come up with a new file and move all the abstraction code there,
> does that work?

I think so, but it's more important to figure out a good user space
interface first. The ioctl interfaces should be written on a higher-level
abstraction, to ensure they can work with any hardware implementation
and are not specific to Renesas devices.

Can you describe on an abstract level how a user would use the
character device, and what they achieve by that?

       Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> >
> > If I come up with a new file and move all the abstraction code there,
> > does that work?
> 
> I think so, but it's more important to figure out a good user space interface
> first. The ioctl interfaces should be written on a higher-level abstraction, to
> ensure they can work with any hardware implementation and are not
> specific to Renesas devices.
> 
> Can you describe on an abstract level how a user would use the character
> device, and what they achieve by that?
> 
>        Arnd

Hi Arnd

This driver is meant to be used by Renesas PTP Clock Manager for
Linux (pcm4l) software for Renesas device only.

About how pcm4l uses the char device, pcm4l will open the device
and do the supported ioctl cmds on the device, simple like that.

At the same time, pcm4l will also open ptp hardware clock device,
which is /dev/ptp[x], to do clock adjustments.

Min 

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Tue, Feb 16, 2021 at 6:10 PM Min Li <min.li.xe@renesas.com> wrote:
> > > If I come up with a new file and move all the abstraction code there,
> > > does that work?
> >
> > I think so, but it's more important to figure out a good user space interface
> > first. The ioctl interfaces should be written on a higher-level abstraction, to
> > ensure they can work with any hardware implementation and are not
> > specific to Renesas devices.
> >
> > Can you describe on an abstract level how a user would use the character
> > device, and what they achieve by that?
>
> This driver is meant to be used by Renesas PTP Clock Manager for
> Linux (pcm4l) software for Renesas device only.
>
> About how pcm4l uses the char device, pcm4l will open the device
> and do the supported ioctl cmds on the device, simple like that.
>
> At the same time, pcm4l will also open ptp hardware clock device,
> which is /dev/ptp[x], to do clock adjustments.

I can't help but think you are evading my question I asked. If there is no
specific action that this pcm4l tool needs to perform, then I'd think
we should better not provide any interface for it at all.

I also found a reference to only closed source software at
https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
We don't add low-level interfaces to the kernel that are only
usable by closed-source software.

Once you are able to describe the requirements for what pcm4l
actually needs from the hardware, we can start discussing what
a high-level interface would look like that can be used to replace
the your current interface, in a way that would work across vendors
and with both pcm4l and open-source tools that do the same job.

      Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> 
> I can't help but think you are evading my question I asked. If there is no
> specific action that this pcm4l tool needs to perform, then I'd think we
> should better not provide any interface for it at all.
> 
> I also found a reference to only closed source software at
> https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
> We don't add low-level interfaces to the kernel that are only usable by
> closed-source software.
> 
> Once you are able to describe the requirements for what pcm4l actually
> needs from the hardware, we can start discussing what a high-level
> interface would look like that can be used to replace the your current
> interface, in a way that would work across vendors and with both pcm4l and
> open-source tools that do the same job.
> 
>       Arnd

Hi Arnd

This driver is used by pcm4l to access functionalities that cannot be accessed through PHC(ptp hardware clock) interface.

All these functions are kind of specific to Renesas SMU device and I have never heard other devices offering similar functions

The 3 functions currently provided are (more to be added in the future)

- set combomode

In Telecom Boundary Clock (T-BC) and Telecom Time Slave Clock (T-TSC) applications per ITU-T G.8275.2, two DPLLs can be used:
one DPLL is configured as a DCO to synthesize PTP clocks, and the other DPLL is configured as an EEC(Ethernet Equipment Clock)
to generate physical layer clocks. Combo mode provides physical layer frequency support from the EEC/SEC to the PTP clock.

- read DPLL's FFO

Read fractional frequency offset (FFO) from a DPLL. 

For a DPLL channel, a Frequency Control Word (FCW) is used to adjust the frequency output of the DCO. A positive value will
increase the output frequency and a negative one will decrease the output frequency.

This function will read FCW first and convert it to FFO.

-read DPLL's state

The DPLLs support four primary operating modes: Free-Run, Locked, Holdover, and DCO. In Free-Run mode the DPLLs synthesize
clocks based on the system clock alone. In Locked mode the DPLLs filter reference clock jitter with the selected bandwidth. Additionally
in Locked mode, the long-term output frequency accuracy is the same as the long-term frequency accuracy of the selected input
reference. In Holdover mode, the DPLL uses frequency data acquired while in Locked mode to generate accurate frequencies when input
references are not available. In DCO mode, the DPLL control loop is opened and the DCO can be controlled by a PTP clock recovery
servo running on an external processor to synthesize PTP clocks.

Again, at the bottom, these function are just reading/writing certain registers through I2C/SPI interface.

I am making this driver to support pcm4l since my my company, Renesas, wants to abstract hw details into the kernel. But I can not figure out
how to make this universally applied interface and I find misc is the best place to hold driver like this. On the other hand, if you have better ideas,
I am all ears.

Min

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Tue, Feb 16, 2021 at 11:14 PM Min Li <min.li.xe@renesas.com> wrote:
> > I can't help but think you are evading my question I asked. If there is no
> > specific action that this pcm4l tool needs to perform, then I'd think we
> > should better not provide any interface for it at all.
> >
> > I also found a reference to only closed source software at
> > https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
> > We don't add low-level interfaces to the kernel that are only usable by
> > closed-source software.
> >
> > Once you are able to describe the requirements for what pcm4l actually
> > needs from the hardware, we can start discussing what a high-level
> > interface would look like that can be used to replace the your current
> > interface, in a way that would work across vendors and with both pcm4l and
> > open-source tools that do the same job.
>
> This driver is used by pcm4l to access functionalities that cannot be accessed through PHC(ptp hardware clock) interface.
>
> All these functions are kind of specific to Renesas SMU device and I have never heard other devices offering similar functions
>
> The 3 functions currently provided are (more to be added in the future)
>
> - set combomode
>
> In Telecom Boundary Clock (T-BC) and Telecom Time Slave Clock (T-TSC) applications
> per ITU-T G.8275.2, two DPLLs can be used:
> one DPLL is configured as a DCO to synthesize PTP clocks, and the other DPLL is
> configured as an EEC(Ethernet Equipment Clock) to generate physical layer clocks.
> Combo mode provides physical layer frequency support from the EEC/SEC to the PTP
> clock.

Thank you for the explanation. Now, to take the question to an even
higher level, is it useful to leave it up to the user to pick one of the two
modes explicitly, or can the kernel make that decision based on some
other information that it already has, or that can be supplied to it
using a more abstract interface?

In other words, when would a user pick combomode over non-combomode
or vice versa? Would it make sense to have this configured according to
the hardware platform, e.g. in a device tree property of the device, rather
than having the user choose a mode?

Which of the two possible modes do other PTP devices use that support
DCO and EEC but are not configurable?

> - read DPLL's FFO
>
> Read fractional frequency offset (FFO) from a DPLL.
>
> For a DPLL channel, a Frequency Control Word (FCW) is used to adjust the
> frequency output of the DCO. A positive value will increase the output frequency
> and a negative one will decrease the output frequency.
>
> This function will read FCW first and convert it to FFO.

Is this related to the information in the timex->freq field? It sounds
like this would already be accessible through the existing
clock_adjtime() interface.

> -read DPLL's state
>
> The DPLLs support four primary operating modes: Free-Run, Locked,
> Holdover, and DCO. In Free-Run mode the DPLLs synthesize clocks
>  based on the system clock alone. In Locked mode the DPLLs filter
> reference clock jitter with the selected bandwidth. Additionally in
> Locked mode, the long-term output frequency accuracy is the same
> as the long-term frequency accuracy of the selected input reference.
> In Holdover mode, the DPLL uses frequency data acquired while in
> Locked mode to generate accurate frequencies when input
> references are not available. In DCO mode, the DPLL control loop
> is opened and the DCO can be controlled by a PTP clock recovery
> servo running on an external processor to synthesize PTP clocks.

Wouldn't any PTP clock run in one of these modes? If this is just
informational, it might be appropriate to have another sysfs attribute
for each PTP clock that shows the state of the DPLL, and then
have the PTP driver either fill in the current value in 'struct ptp_clock',
or provide a callback to report the state when a user reads the sysfs
attribute.

      Arnd

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
>
> I attached the G.8273.2 document, where chapter 6 is about supporting physical layer
> frequency. And combo mode is Renesas way to support this requirement. Other companies
> may come up with different ways to support it.
>
> When EEC quality is below certain level, we would wanna turn off combo mode.

Maybe this is something that could be handled inside of the device driver then?

If the driver can use the same algorithm that is in your user space software
today, that would seem to be a nicer way to handle it than requiring a separate
application.

> > > This function will read FCW first and convert it to FFO.
> >
> > Is this related to the information in the timex->freq field? It sounds like this
> > would already be accessible through the existing
> > clock_adjtime() interface.
> >
>
> They are related, but dealing with timex->freq has limitations
>
> 1) Renesas SMU has up to 8 DPLLs and only one of the them would be ptp
> clock and we want to be able to read any DPLL's FFO or state

Is this necessarily unique to Renesas SMU though? Not sure what
makes sense in terms of the phc/ptp interface. Could there just be
a separate instance for each DPLL in the phc subsystem even if it's
actually a ptp clock, or would that be an incorrect use?

> 2) timex->freq's unit is ppb and we want to read more precise ffo in smaller unit of ppqt

This also sounds like something that would not be vendor specific. If you
need a higher resolution, then at some point others would need it as well.
There is already precedence in 'struct timex' to redefine the resolution of
some fields based on a flag -- 'time.tv_usec' can either refer to microseconds
to nanoseconds.

If the range of the 'freq' field is sufficient to encode ppqt, you could add
another flag for that, otherwise another reserved field can be used.

> 3) there is no interface in the current ptp hardware clock infrastructure to read ffo back from hardware

Adding an internal interface is the easy part here, the hard part is defining
the user interface.

> > Wouldn't any PTP clock run in one of these modes? If this is just
> > informational, it might be appropriate to have another sysfs attribute for
> > each PTP clock that shows the state of the DPLL, and then have the PTP
> > driver either fill in the current value in 'struct ptp_clock', or provide a
> > callback to report the state when a user reads the sysfs attribute.
> >
>
> What you propose can work. But DPLL operating mode is not standardized
> so different vendor may have different explanation for various modes.

If it's a string, that could easily be extended to further modes, as long
as the kernel documents which names are allowed. If multiple vendors
refer to the same mode by different names, someone will have to decide
what to call it in the kernel, and everyone afterwards would use the same
name.

> Also, I thought sysfs is only for debug or informational purpose.
> Production software is not supposed to use it for critical tasks?

No, you are probably thinking of debugfs. sysfs is one of multiple
common ways to exchange this kind of data in a reliable way.

An ioctl would probably work just as well though, usually sysfs
is better when the information makes sense to human operators
or simple shell scripts, while an ioctl interface is better if performance
is important, or if the information is primarily used in C programs.

        Arnd

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Jakub Kicinski]

On Wed, 17 Feb 2021 22:30:14 +0100 Arnd Bergmann wrote:
> On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
> > I attached the G.8273.2 document, where chapter 6 is about supporting physical layer
> > frequency. And combo mode is Renesas way to support this requirement. Other companies
> > may come up with different ways to support it.
> >
> > When EEC quality is below certain level, we would wanna turn off combo mode.  
> 
> Maybe this is something that could be handled inside of the device driver then?
> 
> If the driver can use the same algorithm that is in your user space software
> today, that would seem to be a nicer way to handle it than requiring a separate
> application.

Other points sound more time than networking, so no suggestions
from me, but on using PHC for L1 freq - that seems like a good 
fit for ethtool?

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 17, 2021 4:30 PM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization
> Management Unit (SMU) support
> 
> On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
> >
> > I attached the G.8273.2 document, where chapter 6 is about supporting
> > physical layer frequency. And combo mode is Renesas way to support
> > this requirement. Other companies may come up with different ways to
> support it.
> >
> > When EEC quality is below certain level, we would wanna turn off combo
> mode.
> 
> Maybe this is something that could be handled inside of the device driver
> then?
> 
> If the driver can use the same algorithm that is in your user space software
> today, that would seem to be a nicer way to handle it than requiring a
> separate application.
> 

Hi Arnd


What is the device driver that you are referring here?

In summary of your reviews, are you suggesting me to discard this change and go back to PTP subsystem
to find a better place for things that I wanna do here?

Min

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > If the driver can use the same algorithm that is in your user space software
> > today, that would seem to be a nicer way to handle it than requiring a
> > separate application.
> >
>
> Hi Arnd
>
>
> What is the device driver that you are referring here?
>
> In summary of your reviews, are you suggesting me to discard this change
> and go back to PTP subsystem to find a better place for things that I wanna
> do here?

Yes, I mean doing it all in the PTP driver.

        Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 18, 2021 5:51 AM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization
> Management Unit (SMU) support
> 
> On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > > If the driver can use the same algorithm that is in your user space
> > > software today, that would seem to be a nicer way to handle it than
> > > requiring a separate application.
> > >
> >
> > Hi Arnd
> >
> >
> > What is the device driver that you are referring here?
> >
> > In summary of your reviews, are you suggesting me to discard this
> > change and go back to PTP subsystem to find a better place for things
> > that I wanna do here?
> 
> Yes, I mean doing it all in the PTP driver.
> 
>         Arnd

Hi Arnd

The APIs I am adding here is for our development of assisted partial timing support (APTS),
which is a Global Navigation Satellite System (GNSS) backed by Precision Time Protocol (PTP).
So it is not part of PTP but they can work together for network timing solution.

What I am trying to say is the things that I am adding here doesn't really belong to the PTP world.
For example, timex->freq is different from the ffo that I am reading from this driver since the DPLL is
Working in different mode. For PTP, DPLL is working in DCO mode. In DCO mode, the DPLL 
control loop is opened and the DCO can be controlled by a PTP clock recovery servo running on an 
external processor to synthesize PTP clocks. On the other hand for GNSS timing, the ffo I am reading here is when DPLL is
in locked mode. In Locked the long-term output frequency accuracy is the same as the long-term
frequency accuracy of the selected input reference.

For our GNSS APTS development, we have 2 DPLL channels, one channel is locked to GNSS and another channel is PTP channel.
If GNSS channel is locked, we use GNSS's channel to support network timing. Otherwise, we switch to PTP channel. 

To think about it, our device is really an multi functional device (MFD), which is why I am submitting another review for our MFD driver
on the side. We have our PTP driver and we have this for GNSS APTS and other misc functions. 

So can you take a look at this again and see if it makes sense to keep this change simply because the change is not part of PTP subsystem.
They sound like they are related. But when it comes to technicality, there is really no place in PTP to hold stuff that I am doing here.

Thanks

Min

FW: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

-----Original Message-----
From: Min Li 
Sent: February 18, 2021 11:14 AM
To: 'Arnd Bergmann' <arnd@kernel.org>
Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
Subject: RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support



> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 18, 2021 5:51 AM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic 
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh 
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking 
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization 
> Management Unit (SMU) support
> 
> On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > > If the driver can use the same algorithm that is in your user 
> > > space software today, that would seem to be a nicer way to handle 
> > > it than requiring a separate application.
> > >
> >
> > Hi Arnd
> >
> >
> > What is the device driver that you are referring here?
> >
> > In summary of your reviews, are you suggesting me to discard this 
> > change and go back to PTP subsystem to find a better place for 
> > things that I wanna do here?
> 
> Yes, I mean doing it all in the PTP driver.
> 
>         Arnd

Hi Arnd

The APIs I am adding here is for our development of assisted partial timing support (APTS), which is a Global Navigation Satellite System (GNSS) backed by Precision Time Protocol (PTP).
So it is not part of PTP but they can work together for network timing solution.

What I am trying to say is the things that I am adding here doesn't really belong to the PTP world.
For example, timex->freq is different from the ffo that I am reading from this driver since the DPLL is Working in different mode. For PTP, DPLL is working in DCO mode. In DCO mode, the DPLL control loop is opened and the DCO can be controlled by a PTP clock recovery servo running on an external processor to synthesize PTP clocks. On the other hand for GNSS timing, the ffo I am reading here is when DPLL is in locked mode. In Locked the long-term output frequency accuracy is the same as the long-term frequency accuracy of the selected input reference.

For our GNSS APTS development, we have 2 DPLL channels, one channel is locked to GNSS and another channel is PTP channel.
If GNSS channel is locked, we use GNSS's channel to support network timing. Otherwise, we switch to PTP channel. 

To think about it, our device is really an multi functional device (MFD), which is why I am submitting another review for our MFD driver on the side. We have our PTP driver and we have this for GNSS APTS and other misc functions. 

So can you take a look at this again and see if it makes sense to keep this change simply because the change is not part of PTP subsystem.
They sound like they are related. But when it comes to technicality, there is really no place in PTP to hold stuff that I am doing here.

Thanks

Min

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

Hi Arnd

This is Min again.

I just got off the meeting with our Architect and I would like to correct some confusing information that I delivered before

I said this driver supports 1588/PTP related functionalities. This is not correct. In fact, this driver is developed to support our 
"GNSS Assisted Partial Timing Support" feature, which is not in PTP domain.

So I would categorize Renesas SMU (synchronization management unit) device as a multi-functional device and we are actually having a
separate review with Lee Jones for the MFD driver alone. Above the MFD driver, we have PHC driver to support PTP and this
driver for miscellaneous functions like GNSS APTS stuff.

I also attached the diagram drawing, where left column is the old way we are doing things and right column is how we wanna do it
through MFD now.

Hopefully, this will help you reconsider my proposal. Please do not hesitate to get back to me for this issue.

Thanks 

Min  

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 18, 2021 5:51 AM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization
> Management Unit (SMU) support
> 
> On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > > If the driver can use the same algorithm that is in your user space
> > > software today, that would seem to be a nicer way to handle it than
> > > requiring a separate application.
> > >
> >
> > Hi Arnd
> >
> >
> > What is the device driver that you are referring here?
> >
> > In summary of your reviews, are you suggesting me to discard this
> > change and go back to PTP subsystem to find a better place for things
> > that I wanna do here?
> 
> Yes, I mean doing it all in the PTP driver.
> 
>         Arnd

[-- Attachment #2: rsmu.pdf --]
[-- Type: application/pdf, Size: 91967 bytes --]