From: Min Li <[email protected]>
This driver is developed for the IDT ClockMatrix(TM) and 82P33xxx families
of timing and synchronization devices.It will be used by Renesas PTP Clock
Manager for Linux (pcm4l) software to provide support to GNSS assisted
partial timing support (APTS) and other networking timing functions.
Current version provides kernel API's to support the following functions
-set combomode to enable SYNCE clock support
-read dpll's state to determine if the dpll is locked to the GNSS channel
-read dpll's ffo (fractional frequency offset) in ppqt
Signed-off-by: Min Li <[email protected]>
---
Change log
-rebase change to linux-next tree
-remove uncessary condition checks suggested by Greg
-fix compile error for x86_64
-register device through misc_register suggested by Greg
drivers/misc/Kconfig | 9 ++
drivers/misc/Makefile | 2 +
drivers/misc/rsmu_cdev.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/misc/rsmu_cdev.h | 74 +++++++++++++
drivers/misc/rsmu_cm.c | 166 +++++++++++++++++++++++++++++
drivers/misc/rsmu_sabre.c | 133 +++++++++++++++++++++++
include/uapi/linux/rsmu.h | 64 +++++++++++
7 files changed, 714 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 f532c59..49b523a 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -445,6 +445,15 @@ 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)"
+ help
+ This option enables support for the IDT ClockMatrix(TM) and 82P33xxx
+ families of timing and synchronization devices. It will be used by
+ Renesas PTP Clock Manager for Linux (pcm4l) software to provide support
+ for GNSS assisted partial timing support (APTS) and other networking
+ timing functions.
+
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 99b6f15..21b8ed4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,5 @@ 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..f7dddcc
--- /dev/null
+++ b/drivers/misc/rsmu_cdev.c
@@ -0,0 +1,266 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * This driver is developed for the IDT ClockMatrix(TM) and 82P33xxx families
+ * of timing and synchronization devices. It will be used by Renesas PTP Clock
+ * Manager for Linux (pcm4l) software to provide support to GNSS assisted
+ * partial timing support (APTS) and other networking timing functions.
+ *
+ * Please note it must work with Renesas MFD driver to access device through
+ * I2C/SPI.
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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"
+
+static struct rsmu_ops *ops_array[] = {
+ [RSMU_CM] = &cm_ops,
+ [RSMU_SABRE] = &sabre_ops,
+};
+
+static int
+rsmu_set_combomode(struct rsmu_cdev *rsmu, void __user *arg)
+{
+ struct rsmu_ops *ops = rsmu->ops;
+ struct rsmu_combomode mode;
+ int err;
+
+ if (copy_from_user(&mode, arg, sizeof(mode)))
+ return -EFAULT;
+
+ if (ops->set_combomode == NULL)
+ return -ENOTSUPP;
+
+ mutex_lock(rsmu->lock);
+ err = ops->set_combomode(rsmu, mode.dpll, mode.mode);
+ mutex_unlock(rsmu->lock);
+
+ return err;
+}
+
+static int
+rsmu_get_dpll_state(struct rsmu_cdev *rsmu, void __user *arg)
+{
+ struct rsmu_ops *ops = rsmu->ops;
+ struct rsmu_get_state state_request;
+ u8 state;
+ int err;
+
+ if (copy_from_user(&state_request, arg, sizeof(state_request)))
+ return -EFAULT;
+
+ if (ops->get_dpll_state == NULL)
+ return -ENOTSUPP;
+
+ mutex_lock(rsmu->lock);
+ err = ops->get_dpll_state(rsmu, state_request.dpll, &state);
+ 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_ops *ops = rsmu->ops;
+ struct rsmu_get_ffo ffo_request;
+ int err;
+
+ if (copy_from_user(&ffo_request, arg, sizeof(ffo_request)))
+ return -EFAULT;
+
+ if (ops->get_dpll_ffo == NULL)
+ return -ENOTSUPP;
+
+ mutex_lock(rsmu->lock);
+ err = ops->get_dpll_ffo(rsmu, ffo_request.dpll, &ffo_request);
+ mutex_unlock(rsmu->lock);
+
+ if (copy_to_user(arg, &ffo_request, sizeof(ffo_request)))
+ return -EFAULT;
+
+ return err;
+}
+
+static struct rsmu_cdev *file2rsmu(struct file *file)
+{
+ return container_of(file->private_data, struct rsmu_cdev, miscdev);
+}
+
+static int
+rsmu_open(struct inode *iptr, struct file *fptr)
+{
+ return 0;
+}
+
+static int
+rsmu_release(struct inode *iptr, struct file *fptr)
+{
+ return 0;
+}
+
+static long
+rsmu_ioctl(struct file *fptr, unsigned int cmd, unsigned long data)
+{
+ struct rsmu_cdev *rsmu = file2rsmu(fptr);
+ void __user *arg = (void __user *)data;
+ int err = 0;
+
+ 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 long rsmu_compat_ioctl(struct file *fptr, unsigned int cmd,
+ unsigned long data)
+{
+ return rsmu_ioctl(fptr, cmd, data);
+}
+
+static const struct file_operations rsmu_fops = {
+ .owner = THIS_MODULE,
+ .open = rsmu_open,
+ .release = rsmu_release,
+ .unlocked_ioctl = rsmu_ioctl,
+ .compat_ioctl = rsmu_compat_ioctl,
+};
+
+static int rsmu_init_ops(struct rsmu_cdev *rsmu)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ops_array); i++)
+ if (ops_array[i]->type == rsmu->type)
+ break;
+
+ if (i == ARRAY_SIZE(ops_array))
+ return -EINVAL;
+
+ rsmu->ops = ops_array[i];
+ return 0;
+}
+
+static int
+rsmu_probe(struct platform_device *pdev)
+{
+ struct rsmu_pdata *pdata = dev_get_platdata(&pdev->dev);
+ struct rsmu_cdev *rsmu;
+ 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);
+
+ /* Initialize and register the miscdev */
+ rsmu->miscdev.minor = MISC_DYNAMIC_MINOR;
+ rsmu->miscdev.fops = &rsmu_fops;
+ snprintf(rsmu->name, sizeof(rsmu->name), "rsmu%d", rsmu->index);
+ rsmu->miscdev.name = rsmu->name;
+ err = misc_register(&rsmu->miscdev);
+ if (err) {
+ dev_err(rsmu->dev, "Unable to register device\n");
+ return -ENODEV;
+ }
+
+ err = rsmu_init_ops(rsmu);
+ if (err) {
+ dev_err(rsmu->dev, "Unknown SMU type %d", rsmu->type);
+ return err;
+ }
+
+ dev_info(rsmu->dev, "Probe %s successful\n", rsmu->name);
+ return 0;
+}
+
+static int
+rsmu_remove(struct platform_device *pdev)
+{
+ struct rsmu_cdev *rsmu = platform_get_drvdata(pdev);
+
+ misc_deregister(&rsmu->miscdev);
+
+ 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 struct platform_driver rsmu_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+ .probe = rsmu_probe,
+ .remove = rsmu_remove,
+ .id_table = rsmu_id_table,
+};
+
+static int __init rsmu_init(void)
+{
+ int err;
+
+ err = platform_driver_register(&rsmu_driver);
+ if (err < 0)
+ pr_err("Unabled to register %s platform driver", DRIVER_NAME);
+
+ return err;
+}
+
+static void __exit rsmu_exit(void)
+{
+ platform_driver_unregister(&rsmu_driver);
+}
+
+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..5710929
--- /dev/null
+++ b/drivers/misc/rsmu_cdev.h
@@ -0,0 +1,74 @@
+/* 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/miscdevice.h>
+
+struct rsmu_ops;
+
+/**
+ * struct rsmu_cdev - Driver data for RSMU character device
+ * @name: rsmu device name
+ * @dev: pointer to platform device
+ * @mfd: pointer to MFD device
+ * @miscdev: character device handle
+ * @lock: mutex to protect operations from being interrupted
+ * @type: rsmu device type
+ * @ops: rsmu device methods
+ * @index: rsmu device index
+ */
+struct rsmu_cdev {
+ char name[16];
+ struct device *dev;
+ struct device *mfd;
+ struct miscdevice miscdev;
+ struct mutex *lock;
+ enum rsmu_type type;
+ struct rsmu_ops *ops;
+ u8 index;
+};
+
+extern struct rsmu_ops cm_ops;
+extern struct rsmu_ops sabre_ops;
+
+struct rsmu_ops {
+ enum rsmu_type type;
+ int (*set_combomode)(struct rsmu_cdev *rsmu, u8 dpll, u8 mode);
+ int (*get_dpll_state)(struct rsmu_cdev *rsmu, u8 dpll, u8 *state);
+ int (*get_dpll_ffo)(struct rsmu_cdev *rsmu, u8 dpll,
+ struct rsmu_get_ffo *ffo);
+};
+
+/**
+ * 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,
+};
+#endif
diff --git a/drivers/misc/rsmu_cm.c b/drivers/misc/rsmu_cm.c
new file mode 100644
index 0000000..d5af624
--- /dev/null
+++ b/drivers/misc/rsmu_cm.c
@@ -0,0 +1,166 @@
+// 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 <asm/unaligned.h>
+
+#include "rsmu_cdev.h"
+
+static 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));
+}
+
+static 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 DPLL_STATE_FREERUN:
+ *state = E_SRVLOUNQUALIFIEDSTATE;
+ break;
+ case DPLL_STATE_LOCKACQ:
+ case DPLL_STATE_LOCKREC:
+ *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;
+}
+
+static int rsmu_cm_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll,
+ struct rsmu_get_ffo *ffo)
+{
+ u8 buf[8] = {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 = sign_extend64(get_unaligned_le64(buf), 47);
+
+ /* FCW unit is 2 ^ -53 = 1.1102230246251565404236316680908e-16 */
+ ffo->ffo = fcw * 111;
+
+ return 0;
+}
+
+struct rsmu_ops cm_ops = {
+ .type = RSMU_CM,
+ .set_combomode = rsmu_cm_set_combomode,
+ .get_dpll_state = rsmu_cm_get_dpll_state,
+ .get_dpll_ffo = rsmu_cm_get_dpll_ffo,
+};
diff --git a/drivers/misc/rsmu_sabre.c b/drivers/misc/rsmu_sabre.c
new file mode 100644
index 0000000..ca32b2f
--- /dev/null
+++ b/drivers/misc/rsmu_sabre.c
@@ -0,0 +1,133 @@
+// 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 <asm/unaligned.h>
+
+#include "rsmu_cdev.h"
+
+static 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));
+}
+
+static 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;
+}
+
+static int rsmu_sabre_get_dpll_ffo(struct rsmu_cdev *rsmu, u8 dpll,
+ struct rsmu_get_ffo *ffo)
+{
+ u8 buf[8] = {0};
+ s64 fcw = 0;
+ u16 dpll_freq_n;
+ int err;
+
+ /*
+ * IDTDpll_GetCurrentDpllFreqOffset retrieves the FFO integrator only.
+ * In order to get Proportional + Integrator, use the holdover FFO with
+ * the filter bandwidth 0.5 Hz set by TCS file.
+ */
+ 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 = sign_extend64(get_unaligned_le64(buf), 39);
+
+ /* FCW unit is 77760 / ( 1638400 * 2^48) = 1.68615121864946 * 10^-16 */
+ ffo->ffo = div_s64(fcw * 168615, 1000);
+
+ return 0;
+}
+
+struct rsmu_ops sabre_ops = {
+ .type = RSMU_SABRE,
+ .set_combomode = rsmu_sabre_set_combomode,
+ .get_dpll_state = rsmu_sabre_get_dpll_state,
+ .get_dpll_ffo = rsmu_sabre_get_dpll_ffo,
+};
diff --git a/include/uapi/linux/rsmu.h b/include/uapi/linux/rsmu.h
new file mode 100644
index 0000000..02c9e38
--- /dev/null
+++ b/include/uapi/linux/rsmu.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * 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
+
+#include <linux/types.h>
+#include <linux/ioctl.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 (fractional frequency offset) in ppqt*/
+struct rsmu_get_ffo {
+ __u8 dpll;
+ __s64 ffo;
+};
+
+/*
+ * 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 _IOR(RSMU_MAGIC, 2, struct rsmu_get_state)
+
+/**
+ * @Description
+ * ioctl to get SMU dpll ffo.
+ *
+ * @Parameters
+ * pointer to struct rsmu_get_ffo that contains dpll ffo in ppqt
+ */
+#define RSMU_GET_FFO _IOR(RSMU_MAGIC, 3, struct rsmu_get_ffo)
+#endif /* __UAPI_LINUX_RSMU_CDEV_H */
--
2.7.4
On Wed, Apr 07, 2021 at 01:33:35PM -0400, [email protected] wrote:
> +static int
> +rsmu_open(struct inode *iptr, struct file *fptr)
> +{
> + return 0;
> +}
> +
> +static int
> +rsmu_release(struct inode *iptr, struct file *fptr)
> +{
> + return 0;
> +}
If you do nothing in an open/release function, then there is no need to
have them at all, you can remove them.
But this feels odd, how do you know what device you are using in your
ioctl command?
thanks,
greg k-h
On Wed, Apr 07, 2021 at 01:33:35PM -0400, [email protected] wrote:
> +/**
> + * struct rsmu_cdev - Driver data for RSMU character device
> + * @name: rsmu device name
Why not use the miscdev name field?
> + * @dev: pointer to platform device
So it's a parent? Why not make this a real platform_device pointer and
not a device pointer?
> + * @mfd: pointer to MFD device
You are properly handling the reference count of this and the platform
device pointer when you save it off, right? Please check.
> + * @miscdev: character device handle
> + * @lock: mutex to protect operations from being interrupted
What operations?
> + * @type: rsmu device type
> + * @ops: rsmu device methods
> + * @index: rsmu device index
Index into what?
> --- /dev/null
> +++ b/drivers/misc/rsmu_cm.c
> @@ -0,0 +1,166 @@
> +// 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.
Note, it's 2021 :)
thanks,
greg k-h
On Wed, Apr 07, 2021 at 01:33:35PM -0400, [email protected] wrote:
> From: Min Li <[email protected]>
>
> This driver is developed for the IDT ClockMatrix(TM) and 82P33xxx families
> of timing and synchronization devices.It will be used by Renesas PTP Clock
> Manager for Linux (pcm4l) software to provide support to GNSS assisted
> partial timing support (APTS) and other networking timing functions.
>
> Current version provides kernel API's to support the following functions
> -set combomode to enable SYNCE clock support
> -read dpll's state to determine if the dpll is locked to the GNSS channel
> -read dpll's ffo (fractional frequency offset) in ppqt
>
> Signed-off-by: Min Li <[email protected]>
> ---
> Change log
> -rebase change to linux-next tree
> -remove uncessary condition checks suggested by Greg
> -fix compile error for x86_64
> -register device through misc_register suggested by Greg
>
> drivers/misc/Kconfig | 9 ++
> drivers/misc/Makefile | 2 +
> drivers/misc/rsmu_cdev.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++
> drivers/misc/rsmu_cdev.h | 74 +++++++++++++
> drivers/misc/rsmu_cm.c | 166 +++++++++++++++++++++++++++++
> drivers/misc/rsmu_sabre.c | 133 +++++++++++++++++++++++
Why do you need 4 files here? Can't you do this all in one? There's no
need for such a small driver to be split up, that just causes added
complexity and makes things harder to review and understand.
> include/uapi/linux/rsmu.h | 64 +++++++++++
Where are you documenting these new custom ioctls? And why do you even
need them?
thanks,
greg k-h
>
> If you do nothing in an open/release function, then there is no need to have
> them at all, you can remove them.
>
> But this feels odd, how do you know what device you are using in your ioctl
> command?
>
The type of board is passed by mfd driver through platform data
rsmu->type = pdata->type;
>
> Why do you need 4 files here? Can't you do this all in one? There's no need
> for such a small driver to be split up, that just causes added complexity and
> makes things harder to review and understand.
>
We will add more functions and boards down the road. So the abstraction here is for future consideration
> > include/uapi/linux/rsmu.h | 64 +++++++++++
>
> Where are you documenting these new custom ioctls? And why do you even
> need them?
>
Other than comments in the header itself, no additional documenting. Do you know if Linux has specific place to document custom ioctls?
Renesas software needs to access these ioctls to provide GNSS assisted partial timing support. More specifically, RSMU_GET_STATE would tell us if a specific DPLL
is locked to GNSS and RSMU_GET_FFO would tell us how much of frequency offset for the DPLL to lock to the GNSS.
Thanks
Min
>
> Why not use the miscdev name field?
>
miscdev name field is just a char pointer and I need an static array to manipulate the name with index
>
> So it's a parent? Why not make this a real platform_device pointer and not
> a device pointer?
>
It is not parent and mfd field is the parent
>
> What operations?
>
The MFD driver will create 2 devices, one is for phc driver and another one is this driver.
The lock is to make sure these 2 driver's operations are synchronized.
>
> Index into what?
>
index is passed by mfd driver and will be used as part of device name such as "rsmu0"
On Wed, Apr 07, 2021 at 01:33:35PM -0400, [email protected] wrote:
> --- /dev/null
> +++ b/drivers/misc/rsmu_cdev.c
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0+
Do you really meen "+" here? (sorry, have to ask.)
> +/*
> + * This driver is developed for the IDT ClockMatrix(TM) and 82P33xxx families
> + * of timing and synchronization devices. It will be used by Renesas PTP Clock
> + * Manager for Linux (pcm4l) software to provide support to GNSS assisted
> + * partial timing support (APTS) and other networking timing functions.
> + *
> + * Please note it must work with Renesas MFD driver to access device through
> + * I2C/SPI.
> + *
> + * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
You should not need this as it's a driver, so you should only use the
dev_dbg() family of print message functions, no need for pr_*() calls.
> +
> +#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"
KBUILD_MODNAME instead? But you really only need this in one place, so
no need for it at all, just use KBUILD_MODNAME here:
> +static struct platform_driver rsmu_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
Right there.
> + },
> + .probe = rsmu_probe,
> + .remove = rsmu_remove,
> + .id_table = rsmu_id_table,
> +};
> +
> +static int __init rsmu_init(void)
> +{
> + int err;
> +
> + err = platform_driver_register(&rsmu_driver);
> + if (err < 0)
> + pr_err("Unabled to register %s platform driver", DRIVER_NAME);
> +
> + return err;
> +}
> +
> +static void __exit rsmu_exit(void)
> +{
> + platform_driver_unregister(&rsmu_driver);
> +}
> +
> +module_init(rsmu_init);
> +module_exit(rsmu_exit);
module_platform_driver() instead?
thanks,
greg k-h
>
> Do you really meen "+" here? (sorry, have to ask.)
>
I don't know. All of our Linux kernel code has GPL-2.0+ and I just blindly inherit it.
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> You should not need this as it's a driver, so you should only use the
> dev_dbg() family of print message functions, no need for pr_*() calls.
>
I have one call to pr_err in rsmu_init
pr_err("Unabled to register %s platform driver", DRIVER_NAME);
On Wed, Apr 07, 2021 at 07:43:44PM +0000, Min Li wrote:
> >
> > Why do you need 4 files here? Can't you do this all in one? There's no need
> > for such a small driver to be split up, that just causes added complexity and
> > makes things harder to review and understand.
> >
>
> We will add more functions and boards down the road. So the abstraction here is for future consideration
Do not add additional complexity today for stuff that you do not need
today. Add it when you need it.
> > > include/uapi/linux/rsmu.h | 64 +++++++++++
> >
> > Where are you documenting these new custom ioctls? And why do you even
> > need them?
> >
>
> Other than comments in the header itself, no additional documenting. Do you know if Linux has specific place to document custom ioctls?
> Renesas software needs to access these ioctls to provide GNSS assisted partial timing support. More specifically, RSMU_GET_STATE would tell us if a specific DPLL
> is locked to GNSS and RSMU_GET_FFO would tell us how much of frequency offset for the DPLL to lock to the GNSS.
Please provide some sort of documentation and at the least, a pointer to
the software that uses this so that we can see how it all ties together.
thanks,
greg k-h
On Wed, Apr 07, 2021 at 08:00:38PM +0000, Min Li wrote:
> >
> > Do you really meen "+" here? (sorry, have to ask.)
> >
>
> I don't know. All of our Linux kernel code has GPL-2.0+ and I just blindly inherit it.
You should ask your managers :)
>
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > You should not need this as it's a driver, so you should only use the
> > dev_dbg() family of print message functions, no need for pr_*() calls.
> >
> I have one call to pr_err in rsmu_init
>
> pr_err("Unabled to register %s platform driver", DRIVER_NAME);
>
My recommendation in another email shows that the line above is not
needed...
thanks,
greg k-h
On Wed, Apr 07, 2021 at 08:12:10PM +0000, Min Li wrote:
> >
> > Why not use the miscdev name field?
> >
> miscdev name field is just a char pointer and I need an static array to manipulate the name with index
Why?
> > So it's a parent? Why not make this a real platform_device pointer and not
> > a device pointer?
> >
>
> It is not parent and mfd field is the parent
Then why are you saving it off?
> > What operations?
> >
>
> The MFD driver will create 2 devices, one is for phc driver and another one is this driver.
> The lock is to make sure these 2 driver's operations are synchronized.
Ok, that should be documented a bit, it wasn't obvious :)
> > Index into what?
> >
>
> index is passed by mfd driver and will be used as part of device name such as "rsmu0"
So you can just look it up from the name?
thanks,
greg k-h
>
> Why?
>
Because I need to manipulate name by adding the index to it during run time and use it as miscdev's name
snprintf(rsmu->name, sizeof(rsmu->name), "rsmu%d", rsmu->index);
rsmu->miscdev.name = rsmu->name;
>
> Then why are you saving it off?
>
For things like dev_err(rsmu->dev, "Undefined RSMU IOCTL");
>
> Ok, that should be documented a bit, it wasn't obvious :)
>
Will do for the next patch
>
> So you can just look it up from the name?
>
name is based on this index in the first place
>
> Please provide some sort of documentation and at the least, a pointer to the
> software that uses this so that we can see how it all ties together.
>
Hi Greg, I will withdraw this review for misc and focus on MFD part review for now. I will re-submit the misc review after mfd change is in.
Thanks
Min