* [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
@ 2021-09-02 18:45 min.li.xe
2021-09-14 9:06 ` Greg KH
0 siblings, 1 reply; 8+ messages in thread
From: min.li.xe @ 2021-09-02 18:45 UTC (permalink / raw)
To: derek.kiernan, dragan.cvetic, arnd, gregkh; +Cc: linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
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 <min.li.xe@renesas.com>
---
drivers/misc/Kconfig | 9 ++
drivers/misc/Makefile | 2 +
drivers/misc/rsmu_cdev.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/misc/rsmu_cdev.h | 77 +++++++++++++++
drivers/misc/rsmu_cm.c | 164 +++++++++++++++++++++++++++++++
drivers/misc/rsmu_sabre.c | 133 ++++++++++++++++++++++++++
include/uapi/linux/rsmu.h | 66 +++++++++++++
7 files changed, 690 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 85ba901..6ed5a18 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -469,6 +469,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 a086197..bde748d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -59,3 +59,5 @@ obj-$(CONFIG_UACCE) += uacce/
obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.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..8e856a6
--- /dev/null
+++ b/drivers/misc/rsmu_cdev.c
@@ -0,0 +1,239 @@
+// 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.
+ */
+
+#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"
+
+static DEFINE_IDA(rsmu_cdev_map);
+
+static struct rsmu_ops *ops_array[] = {
+ [0] = &cm_ops,
+ [1] = &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 -EOPNOTSUPP;
+
+ 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 -EOPNOTSUPP;
+
+ 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 -EOPNOTSUPP;
+
+ 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 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,
+ .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_ddata *ddata = dev_get_drvdata(pdev->dev.parent);
+ struct rsmu_cdev *rsmu;
+ int err;
+
+ rsmu = devm_kzalloc(&pdev->dev, sizeof(*rsmu), GFP_KERNEL);
+ if (!rsmu)
+ return -ENOMEM;
+
+ /* Save driver private data */
+ platform_set_drvdata(pdev, rsmu);
+
+ rsmu->dev = &pdev->dev;
+ rsmu->mfd = pdev->dev.parent;
+ rsmu->type = ddata->type;
+ rsmu->lock = &ddata->lock;
+ rsmu->regmap = ddata->regmap;
+ rsmu->index = ida_simple_get(&rsmu_cdev_map, 0, MINORMASK + 1, GFP_KERNEL);
+ if (rsmu->index < 0) {
+ dev_err(rsmu->dev, "Unable to get index %d\n", rsmu->index);
+ return rsmu->index;
+ }
+ snprintf(rsmu->name, sizeof(rsmu->name), "rsmu%d", rsmu->index);
+
+ err = rsmu_init_ops(rsmu);
+ if (err) {
+ dev_err(rsmu->dev, "Unknown SMU type %d", rsmu->type);
+ ida_simple_remove(&rsmu_cdev_map, rsmu->index);
+ return err;
+ }
+
+ /* Initialize and register the miscdev */
+ rsmu->miscdev.minor = MISC_DYNAMIC_MINOR;
+ rsmu->miscdev.fops = &rsmu_fops;
+ rsmu->miscdev.name = rsmu->name;
+ err = misc_register(&rsmu->miscdev);
+ if (err) {
+ dev_err(rsmu->dev, "Unable to register device\n");
+ ida_simple_remove(&rsmu_cdev_map, rsmu->index);
+ return -ENODEV;
+ }
+
+ 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);
+ ida_simple_remove(&rsmu_cdev_map, rsmu->index);
+
+ return 0;
+}
+
+static const struct platform_device_id rsmu_id_table[] = {
+ { "8a3400x-cdev", RSMU_CM},
+ { "82p33x1x-cdev", RSMU_SABRE},
+ {}
+};
+MODULE_DEVICE_TABLE(platform, rsmu_id_table);
+
+static struct platform_driver rsmu_driver = {
+ .driver = {
+ .name = "rsmu-cdev",
+ },
+ .probe = rsmu_probe,
+ .remove = rsmu_remove,
+ .id_table = rsmu_id_table,
+};
+
+module_platform_driver(rsmu_driver);
+
+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..d67f71a
--- /dev/null
+++ b/drivers/misc/rsmu_cdev.h
@@ -0,0 +1,77 @@
+/* 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>
+#include <linux/regmap.h>
+
+struct rsmu_ops;
+
+/**
+ * struct rsmu_cdev - Driver data for RSMU character device
+ * @name: rsmu device name as rsmu[index]
+ * @dev: pointer to device
+ * @mfd: pointer to MFD device
+ * @miscdev: character device handle
+ * @regmap: I2C/SPI regmap handle
+ * @lock: mutex to protect operations from being interrupted
+ * @type: rsmu device type, passed through platform data
+ * @ops: rsmu device methods
+ * @index: rsmu device index
+ */
+struct rsmu_cdev {
+ char name[16];
+ struct device *dev;
+ struct device *mfd;
+ struct miscdevice miscdev;
+ struct regmap *regmap;
+ struct mutex *lock;
+ enum rsmu_type type;
+ struct rsmu_ops *ops;
+ int 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..d1f2d7a
--- /dev/null
+++ b/drivers/misc/rsmu_cm.c
@@ -0,0 +1,164 @@
+// 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 = regmap_bulk_read(rsmu->regmap, 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 regmap_bulk_write(rsmu->regmap, 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 = regmap_bulk_read(rsmu->regmap, 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 = regmap_bulk_read(rsmu->regmap, 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..1646d6b
--- /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 = regmap_bulk_read(rsmu->regmap, dpll_ctrl_n, &cfg, sizeof(cfg));
+ if (err)
+ return err;
+
+ cfg &= ~(COMBO_MODE_MASK << COMBO_MODE_SHIFT);
+ cfg |= mode << COMBO_MODE_SHIFT;
+
+ return regmap_bulk_write(rsmu->regmap, 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 = regmap_bulk_read(rsmu->regmap, 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 = regmap_bulk_read(rsmu->regmap, 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 * 2107689, 12500);
+
+ 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..85a208b
--- /dev/null
+++ b/include/uapi/linux/rsmu.h
@@ -0,0 +1,66 @@
+/* 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.Combo mode provides physical layer frequency
+ * support from the Ethernet Equipment Clock to the PTP clock
+ *
+ * @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. Application can call this API to tell if
+ * SMU is locked to the GNSS signal
+ *
+ * @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 (fractional frequency offset).
+ *
+ * @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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-02 18:45 [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support min.li.xe
@ 2021-09-14 9:06 ` Greg KH
2021-09-14 20:43 ` Min Li
0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2021-09-14 9:06 UTC (permalink / raw)
To: min.li.xe; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
On Thu, Sep 02, 2021 at 02:45:53PM -0400, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> 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 <min.li.xe@renesas.com>
> ---
> drivers/misc/Kconfig | 9 ++
> drivers/misc/Makefile | 2 +
> drivers/misc/rsmu_cdev.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++
> drivers/misc/rsmu_cdev.h | 77 +++++++++++++++
> drivers/misc/rsmu_cm.c | 164 +++++++++++++++++++++++++++++++
> drivers/misc/rsmu_sabre.c | 133 ++++++++++++++++++++++++++
If you make this all one .c file, the .h file can go away and it will be
much simpler in the end. And will get rid of the global symbols.
> include/uapi/linux/rsmu.h | 66 +++++++++++++
> 7 files changed, 690 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 85ba901..6ed5a18 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -469,6 +469,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.
No driver name listed?
> +
> 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 a086197..bde748d 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,5 @@ obj-$(CONFIG_UACCE) += uacce/
> obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
> obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
> obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
> +rsmu-objs := rsmu_cdev.o rsmu_cm.o rsmu_sabre.o
> +obj-$(CONFIG_RSMU) += rsmu.o
Just one .c file please.
> diff --git a/drivers/misc/rsmu_cdev.c b/drivers/misc/rsmu_cdev.c
> new file mode 100644
> index 0000000..8e856a6
> --- /dev/null
> +++ b/drivers/misc/rsmu_cdev.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0+
Are you sure about "+"? I 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.
Are you sure about that date?
> + */
> +
> +#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"
> +
> +static DEFINE_IDA(rsmu_cdev_map);
> +
> +static struct rsmu_ops *ops_array[] = {
> + [0] = &cm_ops,
> + [1] = &sabre_ops,
0 and 1 don't define much. Why are they needed?
No NULL at the end of the list?
> +};
> +
> +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 -EOPNOTSUPP;
> +
> + mutex_lock(rsmu->lock);
> + err = ops->set_combomode(rsmu, mode.dpll, mode.mode);
No error checking of the userspace values?
> + 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 -EOPNOTSUPP;
No error checking of the userspace values?
> +
> + 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 -EOPNOTSUPP;
> +
> + mutex_lock(rsmu->lock);
> + err = ops->get_dpll_ffo(rsmu, ffo_request.dpll, &ffo_request);
Again, no checking of the userspace values?
> + 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 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");
Do not allow userspace to flood the kernel log with messages like this.
> + err = -EINVAL;
Wrong value.
> + break;
> + }
> +
> + return err;
> +}
What userspace tool is making these ioctl calls?
> +
> +static long rsmu_compat_ioctl(struct file *fptr, unsigned int cmd,
> + unsigned long data)
> +{
> + return rsmu_ioctl(fptr, cmd, data);
> +}
> +
Why is the compat ioctl needed at all?
> +static const struct file_operations rsmu_fops = {
> + .owner = THIS_MODULE,
> + .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_ddata *ddata = dev_get_drvdata(pdev->dev.parent);
> + struct rsmu_cdev *rsmu;
> + int err;
> +
> + rsmu = devm_kzalloc(&pdev->dev, sizeof(*rsmu), GFP_KERNEL);
> + if (!rsmu)
> + return -ENOMEM;
> +
> + /* Save driver private data */
> + platform_set_drvdata(pdev, rsmu);
> +
> + rsmu->dev = &pdev->dev;
> + rsmu->mfd = pdev->dev.parent;
> + rsmu->type = ddata->type;
> + rsmu->lock = &ddata->lock;
> + rsmu->regmap = ddata->regmap;
> + rsmu->index = ida_simple_get(&rsmu_cdev_map, 0, MINORMASK + 1, GFP_KERNEL);
> + if (rsmu->index < 0) {
> + dev_err(rsmu->dev, "Unable to get index %d\n", rsmu->index);
> + return rsmu->index;
> + }
> + snprintf(rsmu->name, sizeof(rsmu->name), "rsmu%d", rsmu->index);
> +
> + err = rsmu_init_ops(rsmu);
> + if (err) {
> + dev_err(rsmu->dev, "Unknown SMU type %d", rsmu->type);
> + ida_simple_remove(&rsmu_cdev_map, rsmu->index);
> + return err;
> + }
> +
> + /* Initialize and register the miscdev */
> + rsmu->miscdev.minor = MISC_DYNAMIC_MINOR;
> + rsmu->miscdev.fops = &rsmu_fops;
> + rsmu->miscdev.name = rsmu->name;
> + err = misc_register(&rsmu->miscdev);
> + if (err) {
> + dev_err(rsmu->dev, "Unable to register device\n");
> + ida_simple_remove(&rsmu_cdev_map, rsmu->index);
> + return -ENODEV;
> + }
> +
> + dev_info(rsmu->dev, "Probe %s successful\n", rsmu->name);
When drivers work, they are totally quiet. Please remove.
> + return 0;
> +}
> +
> +static int
> +rsmu_remove(struct platform_device *pdev)
> +{
> + struct rsmu_cdev *rsmu = platform_get_drvdata(pdev);
> +
> + misc_deregister(&rsmu->miscdev);
> + ida_simple_remove(&rsmu_cdev_map, rsmu->index);
> +
> + return 0;
> +}
> +
> +static const struct platform_device_id rsmu_id_table[] = {
> + { "8a3400x-cdev", RSMU_CM},
> + { "82p33x1x-cdev", RSMU_SABRE},
> + {}
> +};
> +MODULE_DEVICE_TABLE(platform, rsmu_id_table);
> +
> +static struct platform_driver rsmu_driver = {
> + .driver = {
> + .name = "rsmu-cdev",
> + },
> + .probe = rsmu_probe,
> + .remove = rsmu_remove,
> + .id_table = rsmu_id_table,
> +};
> +
> +module_platform_driver(rsmu_driver);
> +
> +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..d67f71a
> --- /dev/null
> +++ b/drivers/misc/rsmu_cdev.h
> @@ -0,0 +1,77 @@
> +/* 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>
> +#include <linux/regmap.h>
> +
> +struct rsmu_ops;
> +
> +/**
> + * struct rsmu_cdev - Driver data for RSMU character device
> + * @name: rsmu device name as rsmu[index]
> + * @dev: pointer to device
> + * @mfd: pointer to MFD device
> + * @miscdev: character device handle
> + * @regmap: I2C/SPI regmap handle
> + * @lock: mutex to protect operations from being interrupted
> + * @type: rsmu device type, passed through platform data
> + * @ops: rsmu device methods
> + * @index: rsmu device index
> + */
> +struct rsmu_cdev {
> + char name[16];
> + struct device *dev;
What device is this pointing to?
> + struct device *mfd;
What is this for?
> + struct miscdevice miscdev;
> + struct regmap *regmap;
> + struct mutex *lock;
> + enum rsmu_type type;
> + struct rsmu_ops *ops;
> + int index;
> +};
> +
> +/* Get dpll ffo (fractional frequency offset) in ppqt*/
Need a space at the end of your comment string.
> +struct rsmu_get_ffo {
> + __u8 dpll;
> + __s64 ffo;
> +};
> +d
> +/*
> + * RSMU IOCTL List
> + */
> +#define RSMU_MAGIC '?'
Where did you get this value from?
Where did you reserve it?
> +
> +/**
> + * @Description
What is this format? It is not kernel-doc :(
> + * ioctl to set SMU combo mode.Combo mode provides physical layer frequency
> + * support from the Ethernet Equipment Clock to the PTP clock
> + *
> + * @Parameters
Same here and elsewhere in this file.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-14 9:06 ` Greg KH
@ 2021-09-14 20:43 ` Min Li
2021-09-15 5:38 ` Greg KH
0 siblings, 1 reply; 8+ messages in thread
From: Min Li @ 2021-09-14 20:43 UTC (permalink / raw)
To: Greg KH; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
Hi Greg
Thanks for the review
> > drivers/misc/Kconfig | 9 ++
> > drivers/misc/Makefile | 2 +
> > drivers/misc/rsmu_cdev.c | 239
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/misc/rsmu_cdev.h | 77 +++++++++++++++
> > drivers/misc/rsmu_cm.c | 164 +++++++++++++++++++++++++++++++
> > drivers/misc/rsmu_sabre.c | 133 ++++++++++++++++++++++++++
>
> If you make this all one .c file, the .h file can go away and it will be much
> simpler in the end. And will get rid of the global symbols.
>
That is doable. But <linux/mfd/idt8a340_reg.h> and <linux/mfd/idt82p33_reg.h> have naming confliction.
To make this change one file, I have to include both of them and therefore I have to change them to resolve
Conflicts. Can I include this
> >
> > +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.
>
> No driver name listed?
Sorry, what do you mean by driver name in this context?
>
> > diff --git a/drivers/misc/rsmu_cdev.c b/drivers/misc/rsmu_cdev.c new
> > file mode 100644 index 0000000..8e856a6
> > --- /dev/null
> > +++ b/drivers/misc/rsmu_cdev.c
> > @@ -0,0 +1,239 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Are you sure about "+"? I have to ask.
All of our Linux kernel code have this license. I don't know what is the catch here.
> > +
> > +/**
> > + * struct rsmu_cdev - Driver data for RSMU character device
> > + * @name: rsmu device name as rsmu[index]
> > + * @dev: pointer to device
> > + * @mfd: pointer to MFD device
> > + * @miscdev: character device handle
> > + * @regmap: I2C/SPI regmap handle
> > + * @lock: mutex to protect operations from being interrupted
> > + * @type: rsmu device type, passed through platform data
> > + * @ops: rsmu device methods
> > + * @index: rsmu device index
> > + */
> > +struct rsmu_cdev {
> > + char name[16];
> > + struct device *dev;
>
> What device is this pointing to?
It is the platform device from rsmu_probe(struct platform_device *pdev)
>
> > + struct device *mfd;
>
> What is this for?
It is the multi-functional device from driver/mfd/rsmu_core.c
The mfd driver is responsible for spawn this platform device and spi/i2c
bus access
> > +/*
> > + * RSMU IOCTL List
> > + */
> > +#define RSMU_MAGIC '?'
>
> Where did you get this value from?
>
> Where did you reserve it?
No I didn't reserve it. I checked other code and they all seem to use a random character
>
> > +
> > +/**
> > + * @Description
>
> What is this format? It is not kernel-doc :(
>
> > + * ioctl to set SMU combo mode.Combo mode provides physical layer
> > + frequency
> > + * support from the Ethernet Equipment Clock to the PTP clock
> > + *
> > + * @Parameters
>
> Same here and elsewhere in this file.
I was copying the format from xilinx_sdfec.h
Is there a place that tells me how to properly document ioctl or can you give me an code example?
Thanks
Min
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-14 20:43 ` Min Li
@ 2021-09-15 5:38 ` Greg KH
2021-09-15 5:54 ` Randy Dunlap
2021-09-15 14:42 ` Min Li
0 siblings, 2 replies; 8+ messages in thread
From: Greg KH @ 2021-09-15 5:38 UTC (permalink / raw)
To: Min Li; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
On Tue, Sep 14, 2021 at 08:43:44PM +0000, Min Li wrote:
> Hi Greg
>
> Thanks for the review
>
> > > drivers/misc/Kconfig | 9 ++
> > > drivers/misc/Makefile | 2 +
> > > drivers/misc/rsmu_cdev.c | 239
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > drivers/misc/rsmu_cdev.h | 77 +++++++++++++++
> > > drivers/misc/rsmu_cm.c | 164 +++++++++++++++++++++++++++++++
> > > drivers/misc/rsmu_sabre.c | 133 ++++++++++++++++++++++++++
> >
> > If you make this all one .c file, the .h file can go away and it will be much
> > simpler in the end. And will get rid of the global symbols.
> >
>
> That is doable. But <linux/mfd/idt8a340_reg.h> and <linux/mfd/idt82p33_reg.h> have naming confliction.
> To make this change one file, I have to include both of them and therefore I have to change them to resolve
> Conflicts. Can I include this
Yes, that should be fine, do it as patch 1/X in a series.
> > >
> > > +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.
> >
> > No driver name listed?
>
> Sorry, what do you mean by driver name in this context?
Most Kconfig help texts say the driver name if it is built as a module.
> > > +
> > > +/**
> > > + * struct rsmu_cdev - Driver data for RSMU character device
> > > + * @name: rsmu device name as rsmu[index]
> > > + * @dev: pointer to device
> > > + * @mfd: pointer to MFD device
> > > + * @miscdev: character device handle
> > > + * @regmap: I2C/SPI regmap handle
> > > + * @lock: mutex to protect operations from being interrupted
> > > + * @type: rsmu device type, passed through platform data
> > > + * @ops: rsmu device methods
> > > + * @index: rsmu device index
> > > + */
> > > +struct rsmu_cdev {
> > > + char name[16];
> > > + struct device *dev;
> >
> > What device is this pointing to?
>
> It is the platform device from rsmu_probe(struct platform_device *pdev)
Then why not just point to the platform device?
> >
> > > + struct device *mfd;
> >
> > What is this for?
>
> It is the multi-functional device from driver/mfd/rsmu_core.c
> The mfd driver is responsible for spawn this platform device and spi/i2c
> bus access
Why do you need to mess with that if you have a pointer to the platform
device instead?
> > > +/*
> > > + * RSMU IOCTL List
> > > + */
> > > +#define RSMU_MAGIC '?'
> >
> > Where did you get this value from?
> >
> > Where did you reserve it?
>
> No I didn't reserve it. I checked other code and they all seem to use a random character
That's not the best way to do this.
Why do you need ioctls at all anyway? What userspace tools will be
accessing this driver? Do you have a link to where they are located at?
> > > +
> > > +/**
> > > + * @Description
> >
> > What is this format? It is not kernel-doc :(
> >
> > > + * ioctl to set SMU combo mode.Combo mode provides physical layer
> > > + frequency
> > > + * support from the Ethernet Equipment Clock to the PTP clock
> > > + *
> > > + * @Parameters
> >
> > Same here and elsewhere in this file.
>
> I was copying the format from xilinx_sdfec.h
>
> Is there a place that tells me how to properly document ioctl or can you give me an code example?
The kerneldoc format should be described in Documentation/ somewhere...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-15 5:38 ` Greg KH
@ 2021-09-15 5:54 ` Randy Dunlap
2021-09-15 14:42 ` Min Li
1 sibling, 0 replies; 8+ messages in thread
From: Randy Dunlap @ 2021-09-15 5:54 UTC (permalink / raw)
To: Greg KH, Min Li; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
On 9/14/21 10:38 PM, Greg KH wrote:
> On Tue, Sep 14, 2021 at 08:43:44PM +0000, Min Li wrote:
>> Hi Greg
>>
>> Thanks for the review
>>
>>>> drivers/misc/Kconfig | 9 ++
>>>> drivers/misc/Makefile | 2 +
>>>> drivers/misc/rsmu_cdev.c | 239
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>> drivers/misc/rsmu_cdev.h | 77 +++++++++++++++
>>>> drivers/misc/rsmu_cm.c | 164 +++++++++++++++++++++++++++++++
>>>> drivers/misc/rsmu_sabre.c | 133 ++++++++++++++++++++++++++
>>>
>>> If you make this all one .c file, the .h file can go away and it will be much
>>> simpler in the end. And will get rid of the global symbols.
>>>
>>
>
>>>> +
>>>> +/**
>>>> + * @Description
>>>
>>> What is this format? It is not kernel-doc :(
>>>
>>>> + * ioctl to set SMU combo mode.Combo mode provides physical layer
>>>> + frequency
>>>> + * support from the Ethernet Equipment Clock to the PTP clock
>>>> + *
>>>> + * @Parameters
>>>
>>> Same here and elsewhere in this file.
>>
>> I was copying the format from xilinx_sdfec.h
>>
>> Is there a place that tells me how to properly document ioctl or can you give me an code example?
>
> The kerneldoc format should be described in Documentation/ somewhere...
See Documentation/doc-guide/kernel-doc.rst.
--
~Randy
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-15 5:38 ` Greg KH
2021-09-15 5:54 ` Randy Dunlap
@ 2021-09-15 14:42 ` Min Li
2021-09-15 14:46 ` Greg KH
1 sibling, 1 reply; 8+ messages in thread
From: Min Li @ 2021-09-15 14:42 UTC (permalink / raw)
To: Greg KH; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
> > > > +/*
> > > > + * RSMU IOCTL List
> > > > + */
> > > > +#define RSMU_MAGIC '?'
> > >
> > > Where did you get this value from?
> > >
> > > Where did you reserve it?
> >
> > No I didn't reserve it. I checked other code and they all seem to use
> > a random character
>
> That's not the best way to do this.
>
> Why do you need ioctls at all anyway? What userspace tools will be
> accessing this driver? Do you have a link to where they are located at?
Hi Greg
The userspace tool is called PCM4L (PTP Clock Manager for Linux) from Renesas
https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
But the functions of this misc driver is not ptp related. It is meant for being called by pcm4l to
support GNSS assisted partial timing support (APTS), which doesn't have abstracted/dedicated
Linux Kernel API's. That is why I went for IOCTL in the first place.
Thanks
Min
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-15 14:42 ` Min Li
@ 2021-09-15 14:46 ` Greg KH
2021-09-15 15:00 ` Min Li
0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2021-09-15 14:46 UTC (permalink / raw)
To: Min Li; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
On Wed, Sep 15, 2021 at 02:42:55PM +0000, Min Li wrote:
> > > > > +/*
> > > > > + * RSMU IOCTL List
> > > > > + */
> > > > > +#define RSMU_MAGIC '?'
> > > >
> > > > Where did you get this value from?
> > > >
> > > > Where did you reserve it?
> > >
> > > No I didn't reserve it. I checked other code and they all seem to use
> > > a random character
> >
> > That's not the best way to do this.
> >
> > Why do you need ioctls at all anyway? What userspace tools will be
> > accessing this driver? Do you have a link to where they are located at?
>
> Hi Greg
>
> The userspace tool is called PCM4L (PTP Clock Manager for Linux) from Renesas
>
> https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
>
> But the functions of this misc driver is not ptp related. It is meant for being called by pcm4l to
> support GNSS assisted partial timing support (APTS), which doesn't have abstracted/dedicated
> Linux Kernel API's. That is why I went for IOCTL in the first place.
Why not work on a real set of apis for this type of thing so that all
devices of this type will work properly?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support
2021-09-15 14:46 ` Greg KH
@ 2021-09-15 15:00 ` Min Li
0 siblings, 0 replies; 8+ messages in thread
From: Min Li @ 2021-09-15 15:00 UTC (permalink / raw)
To: Greg KH; +Cc: derek.kiernan, dragan.cvetic, arnd, linux-kernel
> On Wed, Sep 15, 2021 at 02:42:55PM +0000, Min Li wrote:
> > > > > > +/*
> > > > > > + * RSMU IOCTL List
> > > > > > + */
> > > > > > +#define RSMU_MAGIC '?'
> > > > >
> > > > > Where did you get this value from?
> > > > >
> > > > > Where did you reserve it?
> > > >
> > > > No I didn't reserve it. I checked other code and they all seem to
> > > > use a random character
> > >
> > > That's not the best way to do this.
> > >
> > > Why do you need ioctls at all anyway? What userspace tools will be
> > > accessing this driver? Do you have a link to where they are located at?
> >
> > Hi Greg
> >
> > The userspace tool is called PCM4L (PTP Clock Manager for Linux) from
> > Renesas
> >
> > https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
> >
> > But the functions of this misc driver is not ptp related. It is meant
> > for being called by pcm4l to support GNSS assisted partial timing
> > support (APTS), which doesn't have abstracted/dedicated Linux Kernel
> API's. That is why I went for IOCTL in the first place.
>
> Why not work on a real set of apis for this type of thing so that all devices of
> this type will work properly?
>
> thanks,
>
> greg k-h
Hi Greg
That plan is on our roadmap. On the other hand, this is a new area that different company has its
own hw/sw solution and it takes time to abstract functions to kernel. Also, our APTS first release
was already out. Right now we are actively developing another release that we will try out best to
come up with better solution.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-15 15:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 18:45 [PATCH misc] misc: Add Renesas Synchronization Management Unit (SMU) support min.li.xe
2021-09-14 9:06 ` Greg KH
2021-09-14 20:43 ` Min Li
2021-09-15 5:38 ` Greg KH
2021-09-15 5:54 ` Randy Dunlap
2021-09-15 14:42 ` Min Li
2021-09-15 14:46 ` Greg KH
2021-09-15 15:00 ` Min Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).